dev-lead fix-reviews: posts "Fixed" replies for changes that were never pushed, and edits tests to match its own code #2013

Summary

During one PR (combsmith/.github#1220, a release-gate change to scripts/canary-rollout.sh), dev-lead three times produced output that contradicted the actual state of the branch. One left "Fixed" replies standing for a push that was refused; one pushed a commit that turned CI red; one rewrote an existing test to match its own change. A human/Claude session was driving the same branch at the same time and had to undo or reconcile each one.

Verified 2026-10-02 against the PR history, the dev-lead run logs and a local bats run. The three incidents are confirmed. Two of the original hypotheses were wrong and are corrected under "What the logs show". Incident 1 is the same defect as #1959 ("dev-lead fix-reviews acts on a superseded head SHA: posts "Fixed in …" claims and dispositions for changes it never pushes"), now reproduced on a second repo.

What happened (all on combsmith/.github#1220)

1. "Fixed" replies posted before a push that was then refused (2026-10-01, 19:38Z)

Run: https://github.com/petry-projects/.github/actions/runs/36914059791 (repository_dispatch dev-lead-reviews-retry, intent review-changes, INTENT_HEAD_SHA=571a3b8, the PR's first commit).

The guard worked. The replies were posted before the push was attempted and were never retracted.

What the replies claimed, against the branch:

• Never existed on any head: startup failures routed through _run_is_stale plus two new tests; mapfile parsing; "a transport failure is not cached". The startup claim is the opposite of the branch, which deliberately counts every startup_failure and pins that with the test "startup_failure runs are never attributed to an old release — they still BLOCK".
• Already on the branch, written by the Claude session: the on-disk SHA cache and in-process parsing (51e708c, eight minutes earlier).
• Landed later, from the Claude session: the sha256 cache key (ad41f9d, 19:45Z).

2. 15a919e pushed with the existing suite red (2026-10-02, 01:05Z)

CodeRabbit offered two fixes: "Either include the ring in the success and failure log keys, or mark the agent as failed whenever any of its writes failed in the run." dev-lead took the second. It added a test but left the existing test "sync-promotion-failures gives success precedence when agent is in both logs" (combsmith/.github#1023), which the change breaks. Lint and bats went red on that commit; a local run confirms the new test passes and the existing one fails.

The same run contradicted itself:

• 01:05:03: thread reply "Fixed in scripts/canary-rollout.sh …", posted before the push, with a claim marker naming 9318339 (the parent, not the pushed commit).
• 01:05:14: @coderabbitai resolve.
• 01:05:16: run summary status=not-applied: "A commit was pushed, but it did not touch any region the review named". The review was anchored at line 1494; the edit was near line 2229.

CodeRabbit then marked the thread "✅ Confirmed as addressed by @don-petry". So a fix that broke CI cleared a review gate.

3. 13927fc rewrote an existing test to match the code (2026-10-02, 01:11Z)

A cubic finding, made against 15a919e, said: "Update the older test to assert failure precedence so the Bats suite can pass." By then the Claude session had replaced the "any failure wins" rule with a per-ring one (353cbfa, 01:10:32) and posted a correction in the thread (01:11:40). dev-lead pushed 13927fc at 01:11:45 anyway. It renamed the test to "failure precedence" and inverted its assertions, which reverses a deliberate behaviour: a failed write followed by a successful write of the same ring is a recovery.

• The rewritten test fails against the branch's logic (confirmed locally; CI's Lint and bats on this commit was cancelled by the next push, not red).
• The reply's claim marker names eb18b52, a commit that does not exist on GitHub.
• Reverted in af2555c, where all three related tests pass.

What the logs show

Confirmed:

• Replies are posted before the harness commits and pushes, and stay up when the push is refused or never happens (incident 1; same ordering in incident 2).
• The claim SHA is never the commit that carries the change. Five of five markers: 571a3b8 three times (the run's starting head), 9318339 (the parent of the pushed commit), eb18b52 (not on the remote).
• A run stays pinned to its starting head. Incident 1 worked on 571a3b8 for 14 minutes while the branch moved three times.
• A commit was pushed with a previously-passing test red (incident 2), and a bot's "change the test so it passes" was applied literally (incident 3). The qa-lead advisory below notes the "full suite green, don't edit tests to pass" rule exists only in the prompt.
• dev-lead pushes to a branch another actor is driving, including five seconds after a correction was posted in the thread (incident 3).

Refuted:

• No fetch/merge/rebase before pushing, and no abort when the remote head has moved. The harness rebases and refuses to force-push (dev-lead force-pushes from a stale base and silently discards maintainer steering commits (observed on #1604: a human commit was destroyed and its decision reverted) #1607). Nothing to add there.
• A failed push is reported as a success. The incident 1 run ended as failure and posted no success summary. The real inconsistency is in incident 2: the summary said not-applied while the thread reply said "Fixed" and @coderabbitai resolve was posted.

Proposed changes (for whoever owns dev-lead)

1. Never leave a "Fixed/addressed" reply standing unless the push landed and the pushed diff touches the cited lines. The claim SHA must be a commit made by this pass and present on the remote head. Retract the reply on every other outcome (push refused, no commit, run failed).
2. Re-read the PR head before posting anything. If it differs from the starting head, post no per-thread replies and re-queue against the new head (dev-lead fix-reviews acts on a superseded head SHA: posts "Fixed in …" claims and dispositions for changes it never pushes #1959, acceptance criterion 1). The rebase-or-refuse push guard already exists and stays as it is.
3. Run the full test suite before pushing. A previously-passing test turning red is a reason to question the change, not to edit the test. Only change an existing test with an explicit, cited justification, and escalate to a human when a bot suggestion contradicts an existing test.
4. Back off when another actor is pushing to the branch (recent non-dev-lead commits, a Claude/human session attached), or require an opt-in label for dev-lead to push to a branch it did not create.
5. Keep the thread reply, @coderabbitai resolve and the run summary consistent. Do not post "Fixed" or ask a bot to resolve when the run's own status is not-applied.

Acceptance criteria

1. No false claim stands. A refused, failed or absent push leaves no "Fixed" or dev-lead:addressed reply standing (not posted, or retracted). A successful push cites the pushed commit SHA, and that commit is on the remote head.
2. Every claim-posting intent is covered. Criteria 1 and 4 hold for fix-reviews, fix-bot-comment and review-changes (and human-pr, which shares its code path). Incident 1 was a review-changes pass. A reply for a fix the harness did push must not be retracted just because that intent's prompt told the model to cite the pre-pass head.
3. The suite gate is a harness check, not prompt text. Before pushing, the harness runs the repo's test command and refuses to push when a test that passed on the pre-pass head fails on the pass's result. A failure that already existed on the pre-pass head does not block. If no test command can be determined, the run summary says the suite was not run; it never reports or implies green.
4. No silent test rewrite. dev-lead cannot change, delete or skip an existing test without an explicit, cited justification.
5. Reproduction. Push to the branch between dev-lead's checkout and its push (a local bare remote is fine), and confirm the push guard still refuses or rebases and that no claim reply survives.
6. Tests. bats coverage for each criterion, including a fixture for the 15a919e shape: the pass adds a new test and breaks an existing one without editing it, and the push is refused and escalated.

Remaining work on PR #2019

PR #2019 ("feat: implement issue #2013 …") was reviewed against this body on 2026-10-02 at head 5f96fdb. Criteria 1, 4 and 5 are met for fix-reviews and fix-bot-comment. What is still open:

1. review-changes is half-wired (criterion 2).
   • prompts/dev-lead/review-changes.md still tells the model to cite git rev-parse HEAD (lines 80 and 90) and "Do not commit or push" (line 128). fix-reviews.md and fix-bot-comment.md were updated; this one was not.
   • retract_unlanded_claims does run for review-changes, so every claim cites the pre-pass head, fails as predates-pass and is retracted, even when the harness committed and pushed the fix. The thread then stays open under a retraction notice.
   • The test-tamper guard in commit_and_push matches only fix-reviews|fix-bot-comment.
   • Fix: give review-changes.md the same "commit before you claim" and test rules as fix-reviews.md, extend the guard's case to review-changes and human-pr, and add bats tests for both.
2. Nothing in the harness stops a push with the suite red (criterion 3). The only change is prompt text ("Run the full test suite"). The 15a919e shape passes every new check: it edits no existing test, so the tamper guard is silent, and its claim is a real in-pass commit.
3. Housekeeping.
   • The new ADR is numbered 0010, which now collides with 0010-ingress-job-level-concurrency-and-exhaustive-schema.md on main. Renumber it.
   • The ADR and the header of scripts/lib/claim-landing.sh say CodeRabbit confirmed a thread on the strength of a never-pushed reply. Per "What the logs show" above, the reply it confirmed described 15a919e, which was pushed. Correct both.
   • The PR description still quotes the old issue text ("From the issue: (observed on …)").

Update 2026-10-03: still open after re-review

Items 1–3 above are done (verified in the diff, threads resolved). The pr-review agent then escalated PR #2019 to needs-human-review at 2026-10-03T02:39Z. One finding is real and not yet fixed:

4. The owner's access token is reachable from the code the regression guard runs. _trg_run in scripts/lib/test-regression-guard.sh removes a fixed list of variables, but the test process can still get secrets.GH_PAT_DON_PETRY two ways:

   • .github/workflows/dev-lead-reusable.yml sets COPILOT_GITHUB_TOKEN: ${{ secrets.GH_PAT_DON_PETRY }} in the Run fix-reviews, Run fix-bot-comment, Run review-changes (and other) steps. It is not on the scrub list.
   • "Checkout caller repo" checks out with the same token and the default saved credentials, so any test can read it from git config.

   The Google API keys (GOOGLE_API_KEY*, GEMINI_API_KEY) are not removed either. Required:

   • Run the test command with an allowlist of environment variables (env -i plus what a test suite needs, such as PATH, HOME, LANG, TMPDIR, CI), not a denylist.
   • Make the saved git credential unreachable from the test process, for example by running the suite in a scratch copy of the tree that has no credential in its git config.
   • Add a bats test whose "suite" prints its environment and git config, and assert that no token value or credential header appears.

   Do not change other behaviour in the same pass. The escalation also raised a "weakens CI" hard stop; that is for the human reviewer to judge, not something to code around.

Not in scope for this issue; tracked in #2032 ("dev-lead claim retraction: replies from cancelled runs are never swept, and a clean rebase retracts claims for fixes that landed"): replies left by cancelled runs, and true claims retracted after a clean rebase.

References

• PR: fix(canary-rollout): old-release failures no longer block the candidate; evaluate every ring pair independently .github#1220
• Incident 1 run: https://github.com/petry-projects/.github/actions/runs/36914059791
• Bad commits: 15a919e, 13927fc (reverted in af2555c); revert explanation: fix(canary-rollout): old-release failures no longer block the candidate; evaluate every ring pair independently .github#1220 (comment)
• Related: dev-lead fix-reviews acts on a superseded head SHA: posts "Fixed in …" claims and dispositions for changes it never pushes #1959 (same defect as incident 1); dev-lead force-pushes from a stale base and silently discards maintainer steering commits (observed on #1604: a human commit was destroyed and its decision reverted) #1607 (the force-push guard, which works); dev-lead claim retraction: replies from cancelled runs are never swept, and a clean rebase retracts claims for fixes that landed (follow-up to #2013) #2032 (follow-up for the retraction sweep's two remaining gaps).
• Hard-gate context: the "Pull Request Reviews" section of AGENTS.md in petry-projects/.github requires agents to resolve all review threads; it says nothing about conflicting bot suggestions or tests.