A Fix Pass records a failure for every ready pull request whose head has no agent review #2020

Activity

1. JacobStephens2

   added

   bugSomething isn't workingSomething isn't working

   needs-triageMaintainer needs to evaluate this issueMaintainer needs to evaluate this issue

   in-progressA Claim: thirdshift has taken this issueA Claim: thirdshift has taken this issue

   and removed

   needs-triageMaintainer needs to evaluate this issueMaintainer needs to evaluate this issue

   on Nov 11, 2026
2. JacobStephens2 commented on Nov 11, 2026

   

   JacobStephens2

   on Nov 11, 2026

   OwnerAuthor

   More actions

   This was generated by AI during triage.

   Investigation summary

   Confirmed on selected Base commit de91156. A scratch reproducer ran two successive fix merge no-base-fix no-email Passes against real fixture Git repositories and fake services. Each selected a different unreviewed PR, successfully repaired CI, left the PR open and ready, then exited 1 and recorded outcome=failed. Reservations and in-progress labels were released.

   Diagnosis and evidence

   • src/fix.rs:62–90 selects candidates using needs_fix, which checks conflict/current-head CI eligibility without checking review evidence (src/github/review.rs:75–108).
   • After worktree acquisition, src/fix.rs:175–184 checks the actual adopted head and sets a Self-merge hold when evidence is absent.
   • src/pull_request.rs:550 raises MergeHold. Delivery preserves the ready PR, but src/run_ending.rs:551–568 classifies this hold as failed. src/logs/reading/inbox.rs:234 consequently identifies a failed Command for Needs you.
   • tests/standards_1795.rs:91 explicitly expects exit 1 for this hold, so the failure behavior is intentional under the current contract.
   • Evidence includes exact-head authored_head as well as reviewed_head or pushed_head from the authenticated account's trailing marker (src/github/review.rs:196–205,248–272). A fix must retain all currently accepted evidence and the acquired-head check.

   Recommended next step

   Prefer skipping heads without this evidence during merge-goal Fix Pass selection, logging the review requirement and continuing to the next candidate. Resolve the effective goal from flags and User config. Keep no-merge repair eligibility and targeted Fix behavior unchanged. Preserve the acquired-head guard against branch movement; missing or stale evidence must never permit Self-merge. This keeps ADR 0004's merge rule intact.

   The Day shift must select whether repair should instead proceed and finish successfully Ready for review despite the requested merge. That changes the outcome contract and requires different implementation and tests.

   After the decision, cover candidate ordering past an unreviewed PR, all-skipped success without failure notification, authored/reviewed evidence, merge defaults and explicit no-merge, reservation cleanup, and acquired-head races. Update the Fix-run glossary/manual and hand-written site descriptions with the chosen behavior.

   Validation

   cargo test --test standards_1795 --test fix_pass --test fix_run: 24 passed, 3 fixture entry points ignored. Standalone scratch reproducer: 1 passed, 1 fixture entry point ignored; evidence at target/triage-evidence/repro.rs and target/triage-evidence/repro.log. scripts/affected-tests main: no changed files, no affected tests. Source files and Git history remain unchanged.

   Questionnaire

   For the Day shift deciding issue #2020:

   The repository reproduces the reported failures: two successive merge-goal Fix Passes repaired two unreviewed PRs but recorded failed Commands. The exact-head review gate correctly prevented Self-merge.

   1. Should a merge-goal Fix Pass skip PRs lacking recorded authorship or review evidence and continue searching (recommended), or repair them and finish successfully Ready for review? Skipping defers repair until evidence exists; successful handoff requires changing the requested-merge outcome contract.
   2. If choosing successful handoff, should it apply only to Pass-dispatched Fix runs or also targeted Fix runs? Recommend limiting this issue to Passes and retaining existing outcomes for other policy, security and protected-path holds.
3. JacobStephens2

   added

   ready-for-humanRequires human implementationRequires human implementation

   and removed

   in-progressA Claim: thirdshift has taken this issueA Claim: thirdshift has taken this issue

   on Nov 11, 2026
4. JacobStephens2 commented on Nov 11, 2026

   

   JacobStephens2

   on Nov 11, 2026

   OwnerAuthor

   More actions

   This was generated by AI during triage.

   Additional fleet evidence from the 2026-11-11 retrospective:

   On thirdshift.app, crontab invokes thirdshift fix base main without the merge keyword, yet [merge] always = true in ~/.thirdshift/config.toml still promotes every Fix Pass to Goal::Merged. That caused three long Fix runs (#1935, #1990, #2010, 42m–70m each) to fail into Needs you with adopted head <sha> has no recorded agent review even though the crontab line itself did not ask for merge.

   Whatever behavior is chosen here (passing over unreviewed ready pull requests in a Fix Pass or ending ready for review instead of Failed) should cover Goal::Merged coming from merge.always = true as well as CLI merge.
5. JacobStephens2 commented on Nov 11, 2026

   

   JacobStephens2

   on Nov 11, 2026

   OwnerAuthor

   More actions

   This was generated by AI during triage.

   Agent Brief

   Category: bug
   Summary: When a Fix Pass repairs a pull request whose adopted head has no recorded agent review, hold Self-merge and complete the run as Ready for review (exit 0) instead of recording a failed Fix run in Needs you, while keeping explicit thirdshift fix <PR URL> merge as an error.

   Current behavior:

   In fix::targeted, when asks.goal == Goal::Merged (either from the merge CLI word or from merge.always = true in UserConfig) and github.authored_or_reviewed_head(&pr, &adopted_head) is false, target.hold_self_merge(...) sets a merge hold while keeping Delivery.goal == Goal::Merged.

   After Delivery::fix_run repairs the merge conflict / red CI, pushes the fixed head, waits for green CI, and marks the pull request ready for review, the repair loop reaches Stage::SelfMerge, sees merge_hold(), and returns Err(MergeHold { reason }). That turns every successful Fix Pass repair of an unreviewed pull request into a Failed Fix run in Needs you—both when cron passes merge and when merge.always = true is configured for Pickup runs.

   Desired behavior:

   • In a Fix Pass (thirdshift fix / thirdshift fix base <branch> / thirdshift fix --author <login>, with merge or merge.always = true), when adopted_head has no recorded agent review (!github.authored_or_reviewed_head(&pr, &adopted_head)?):
     • Still repair the pull request's conflict / red CI and wait for checks so the pull request becomes mergeable and green (needs_fix becomes false, avoiding a deadlock with Review Pass which requires mergeable pull requests).
     • Do not Self-merge; finish Delivery with Goal::ReadyForReview (Resolution::PullRequest { url, goal: Goal::ReadyForReview }), exiting 0 with outcome "Ready for review" rather than "Failed".
     • Log a clear step/message in the Command log / Activity output explaining that Self-merge was held and the pull request was left ready for review because adopted_head has no recorded agent review.
   • In an explicit targeted command where the operator asked for merge (thirdshift fix <PR URL> merge, i.e. targeted invocation with command.flags.goal == Some(Goal::Merged)), continue to fail with MergeHold (non-zero exit) because the caller explicitly requested merging that specific pull request. When targeted thirdshift fix <PR URL> is invoked without merge on the CLI and Goal::Merged came only from merge.always = true, also leave the repaired pull request "Ready for review" (exit 0) with the logged hold message rather than failing.
   • Update docs/guide.md (Fix runs section) and regenerate site/docs/index.html (npm run docs --prefix site) to state that a Fix Pass (or merge.always Fix run) repairs unreviewed pull requests and leaves them ready for review instead of failing when Self-merge is held for review.

   Key interfaces:

   • fix::targeted (distinguishing a Pass / merge.always run from an explicit thirdshift fix <PR URL> merge where command.flags.goal == Some(Goal::Merged)).
   • Delivery::fix_run / PullRequest::hold_self_merge / Goal: ensure Self-merge remains impossible on unreviewed adopted heads while allowing non-explicit-merge Fix runs to succeed at Goal::ReadyForReview.
   • User manual (docs/guide.md and site/docs/index.html).

   Acceptance criteria:

   • thirdshift fix base <branch> merge on a broken pull request whose adopted head has no agent review repairs the pull request, does not merge it, logs why Self-merge was held, exits 0, and records outcome "Ready for review" (not "Failed" / Needs you).
   • The same holds when thirdshift fix base <branch> (or thirdshift fix <PR URL> without merge) runs with merge.always = true in UserConfig.
   • On the next Fix Pass, once the repaired pull request is mergeable and green, needs_fix is false so the Pass does not pick it up again.
   • Explicit thirdshift fix <PR URL> merge on a pull request whose adopted head has no agent review still refuses Self-merge and exits non-zero with the MergeHold message.
   • When adopted_head does have an authored_head or reviewed_head marker, thirdshift fix base <branch> merge still Self-merges as before.
   • CLI integration tests against the fake gh cover these cases, and npm run docs:check --prefix site passes.

   Out of scope:

   • Separate configuration for Self-merging Base-branch repairs while leaving reviewed feature pull requests unmerged (Let a Fix Pass Self-merge its Base branch repair without Self-merging the pull requests it fixes #2029).
6. JacobStephens2

   marked this as blocked on Nov 11, 2026

   • Fix Pass repeatedly fails on a locally checked-out pull request branch instead of passing over it #2045
7. JacobStephens2

   added

   ready-for-agentFully specified, ready for an AFK agentFully specified, ready for an AFK agent

   in-progressA Claim: thirdshift has taken this issueA Claim: thirdshift has taken this issue

   and removed

   ready-for-humanRequires human implementationRequires human implementation

   ready-for-agentFully specified, ready for an AFK agentFully specified, ready for an AFK agent

   in-progressA Claim: thirdshift has taken this issueA Claim: thirdshift has taken this issue

   on Nov 11, 2026
8. JacobStephens2

   closed this as completedin #2088on Nov 12, 2026
9. JacobStephens2

   removed

   in-progressA Claim: thirdshift has taken this issueA Claim: thirdshift has taken this issue

   on Nov 12, 2026