Skip to content

ci(smart-ci): accept a merge ref regenerated against the live protected base tip - #2506

Open
Chris0Jeky wants to merge 5 commits into
mainfrom
issue-2327/merge-ref-race
Open

ci(smart-ci): accept a merge ref regenerated against the live protected base tip#2506
Chris0Jeky wants to merge 5 commits into
mainfrom
issue-2327/merge-ref-race

Conversation

@Chris0Jeky

Copy link
Copy Markdown
Owner

Root cause

Not the hypothesised two-step merge/tree read — observeMergeRef already reads all four SHAs in a
single git rev-parse (scripts/ci/smart-ci/resolve-merge-ref.mjs:82-98), so merge SHA and tree
SHA can never come from different fetches.

The real failure is a base mismatch. .github/workflows/smart-ci-shadow.yml pins
CONTROL_BASE: ${{ github.sha }} — the base branch tip at event dispatch — and the resolver demanded
the merge ref's first parent equal it exactly. GitHub regenerates refs/pull/N/merge against
whatever the base branch points at now, so any push to main between dispatch and the resolver's
fetch makes the first parent the newer tip. resolveMergeRef then burns all three attempts on the
same permanent mismatch (main does not move back), fails closed, and — because the step is
continue-on-error: true — leaves no merge-sha.txt/merge-tree-sha.txt. The Plan step omits
--merge-sha/--merge-tree-sha, and requirePullRequestMergeBinding (plan.mjs:55-61, from the #2401
fix) throws the receipt's planner-error text. Sibling PRs in the same minutes passed simply because
their resolver window did not straddle a main push.

Confirmed in the Plan job log of run 33831258567 (PR #2485):

merge ref attempt 1/3 did not match: base mismatch; retrying
merge ref attempt 2/3 did not match: base mismatch; retrying
merge ref resolution failed closed after 3 attempts: base mismatch
planner error (escalating to the full hosted plan): Error: pull-request planning requires merge SHA and tree SHA from the same fetched merge ref

Same shape in runs 33833016055 (PR #2485) and 33832960392 (PR #2496).

The second receipt line, trust-mismatch — plan says T3, the event re-derives T1, is purely a
symptom: errorPlan pins trust: 'T3' by construction (lib/plan.mjs:362) and evaluateGate
re-derives the real class from the event and reports the difference as a failure
(lib/plan.mjs:473-474). It is not an independent defect.

Fix

  1. resolve-merge-ref.mjs — new observeBaseTip() reads the base branch's live tip
    (git fetch --depth=1 origin refs/heads/<baseRef> + rev-parse). resolveMergeRef accepts a
    resolveBaseTip callback: when the only discrepancy is the first parent, the observation is
    accepted if and only if that parent is the live protected base tip. The event head must still
    match exactly — a head mismatch (base and head mismatch) never consults the base tip. Both
    accepted first parents are protected-branch heads, so nothing untrusted enters the binding.
    Anything else, an unreadable tip, or a head mismatch stays fail-closed with no output files.
  2. Receipt wording — an accepted move logs merge-ref-moved — the base advanced from <sha> to the live protected tip <sha> … and, via the new --note-out, is threaded into the plan receipt
    as a planner note, so the receipt says what happened rather than nothing.
  3. smart-ci-shadow.yml — passes --base-ref "$BASE_REF" and --note-out, and forwards the
    note to plan.mjs --note.
  4. lib/plan.mjs — when plan.plannerError is set, the trust comparison becomes a note
    (error-plan trust is pinned to T3; the event would classify T1) instead of a trust-mismatch
    failure. The run is already red on planner-error; the second line only misdescribed it.
    trust-mismatch remains a hard failure for real plans.

requirePullRequestMergeBinding and its ordering (#2401 / PR #2440) are untouched.

Refs #2327

Verification

  • node --test scripts/ci/smart-ci/*.test.mjs96 tests, 96 pass, 0 fail (was 91 before this
    branch; 5 new).
    • resolve-merge-ref.test.mjs: a merge ref regenerated against the live protected base tip resolves as merge-ref-moved (reproduces the failing sequence and proves the new behaviour),
      plus three fail-closed guards — a first parent that is not the live tip, a head mismatch that
      never consults the base tip, and an unreadable base tip.
    • plan.test.mjs: an error plan's pinned trust is a note, while a real plan with a tampered
      trust class still fails trust-mismatch.
  • node scripts/check-docs-governance.mjs — passed.
  • node --check scripts/ci/smart-ci/resolve-merge-ref.mjs — passed.

Not verified locally: the hosted path. observeBaseTip's actual git fetch/rev-parse against
GitHub, the workflow YAML as executed by Actions, and the end-to-end behaviour under a real base
push are proven only by this PR's own hosted Smart CI / Planner Self-Test and ci-required runs —
pending at the time of writing. Policy fixtures untouched.

Risks

  • The accepted first-parent set widens from one SHA to two (dispatch-time control base, live
    protected base tip). Both are heads of the protected base branch; the untrusted head parent is
    still matched exactly. If the base ref were unprotected this would be weaker — Taskdeck's main
    is protected.
  • One extra shallow git fetch in the Plan job, only on the base-mismatch path.
  • Suppressing trust-mismatch under an error plan is scoped to plan.plannerError being set; every
    such plan is already red on planner-error, so no run can turn green through this change.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@Chris0Jeky Chris0Jeky changed the title ci(smart-ci): resolve merge SHA and tree atomically so a moved merge ref is not a planner error ci(smart-ci): accept a merge ref regenerated against the live protected base tip Sep 4, 2026
@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Review gate (Codex credits exhausted, SC-9): one fresh-context reviewer with a trust-boundary lens confirmed the relaxation is one-sided — the head must still match exactly on every path (mismatchReason only yields 'base mismatch' when headSha === expectedHead), the live base tip is read from origin refs/heads/<base.ref> with the same token plumbing and cannot be moved by a fork PR, the merge/tree pair stays atomic, every non-accepting path still fails closed into requirePullRequestMergeBinding -> errorPlan -> red, and the trust-mismatch-to-note demotion is fenced behind plannerError which is an unconditional red in both modes. Verdict SHIP. Round 2 (test/doc-only, no re-review owed): observeBaseTip argv/token guard test + CLI wiring test, 'protected' wording corrected, literal newlines restored to escapes. MEDIUM on a machine-readable mergeBaseSha for the CI-03 verifier tracked separately (issue linked above). R4 proof = this PR's hosted Smart CI / Planner Self-Test + ci-required at the final head.

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Round 2 pushed as b317280dd — test/doc-only tightenings, no behaviour change to the resolution rule.

  1. MEDIUM — observeBaseTip had no unit test. Added two, mirroring the existing observeMergeRef guard: it asserts the fetch argv is exactly ['fetch', '--no-tags', '--depth=1', 'origin', 'refs/heads/main'] after the --config-env=http.extraHeader= prefix, that rev-parse FETCH_HEAD^{commit} is the only other call, that the token never appears anywhere in argv, and that it travels solely in the fetch step's environment (AUTH_HEADER_ENV set on the fetch, absent on the rev-parse). A second test covers the missing-base-ref and non-SHA rejections. For the CLI wiring, the note writer is now the exported writeMergeRefNote() that main() calls, so a third test proves an accepted moved base writes the expected merge-ref-moved: … line containing both SHAs, LF-terminated and CR-free.

  2. LOW — unverified "protected" claim. Nothing in the resolver checks branch protection, so docs/ci/SMART_CI.md and the code comments/reasons now say "the base branch's live tip on origin — the same branch whose commit already supplies the control-plane tooling". Reworded in the two doc sentences, the observeBaseTip and resolveMergeRef JSDoc, the inline comment, both fail-closed reason strings, the merge-ref-moved log line, the guard message, and the matching test names/regexes.

  3. LOW — literal newlines in template literals. Residue of a Bash-heredoc corruption during the first round; the log template and the note template now carry real \n escapes, so the note's EOL no longer depends on the checkout's line endings (the new CLI test asserts that).

Verification: node --test scripts/ci/smart-ci/*.test.mjs99 tests, 99 pass, 0 fail (was 96; 3 new). node scripts/check-docs-governance.mjs → passed. node --check scripts/ci/smart-ci/resolve-merge-ref.mjs → passed. Still not verified locally: the hosted path — this PR's own Smart CI / Planner Self-Test and ci-required runs remain the proof.

Chris0Jeky added a commit that referenced this pull request Sep 4, 2026
Re-measured against live GitHub on 2026-09-04:

- SC-4 clock: a09d986 did not leave a clean planner. Five same-shape shadow
  false reds landed 2026-09-04 (PR #2485 twice, #2496, #2515, #2500) from the
  CONTROL_BASE/merge-ref race, so the clock restarts when PR #2506 lands. #2506
  becomes the first open blocker in the clause-5 chain, ahead of #2327.
- Human-gate table: add the missing SC-9 and SC-10 rows from OUTSTANDING_TASKS
  section J, with the five SC-10 PRs measured open and #2522 CONFLICTING.
- Clause-4 risks: #2425 (PR #2447) and #2399 (PR #2454) are closed; the open
  flake pair is #2489 and #2378, and the clause-4 row count follows.
- Decision labels: #1936 is closed and #2004 no longer carries decision; the
  open decision-labelled milestone issues are #2324 and #1772.
- Label split: 12 dogfooding / 17 ci / 22 other = 51, with the ci sub-breakdown,
  the ordinary-backlog list and the gated/un-gated arithmetic following it.
Chris0Jeky added a commit that referenced this pull request Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Pending

Development

Successfully merging this pull request may close these issues.

1 participant