[JacobStephens2]

Summary

A Review run can now Self-merge, but only when asked. Either the command says merge (or --merge), or the User config sets [review] merge = true. no-merge / --no-merge turns it off for one command. The command word wins over [review] merge, which defaults to off. merge.always still never applies to Review runs. A recommend-only review ignores [review] merge, and --recommend-only merge is an argument error (exit 2).

 processes/review-run.toml
   review-and-address
   security-review
   delivery            # merge request: a head moved after review is a MergeHold
+  self-merge          # runs only when merge is asked for and the Process doesn't forbid Self-merge
+    hold unless every configured Security review succeeded with no unaddressed findings
+    hand the reviewed checkout to the Fix run's Delivery::fix_run (Goal::Merged)
+      require_unstacked_base → hold if the base is another open PR
+      reviewDecision gate, Foreign-commit review, merge-time conflict and CI Repairs
+      gh pr merge --merge --match-head-commit <reviewed head>

outcome
  merged                 → Run notification, Command log and Inbox show "merged"
  hold / policy refusal  → PR stays Ready for review
    review --author … merge   → logs "Self-merge held: …", exit 0
    review <PR URL> merge     → exit 1, prints "Recovery: thirdshift review <url> merge"

• A merge request saves the effective choice in its Continuation, so Recovery merges the same way even after the config changes. Runs that don't ask to merge record no Continuation, as before.
• Custom pull-request Processes can add self-merge straight after delivery.
• Docs updated: docs/guide.md (Review runs, User config reference, usage), the GLOSSARY.md Review run entry, site/index.html, site/how-it-works/, site/architecture/ and docs/code-navigation.md. site/docs/index.html was regenerated. No prompt sources changed.

Evidence

Execution evidence

The previous run was killed (SIGKILL) during implementation, so this one picked up its branch. Main was merged in first.

• Before: cargo test --bin thirdshift setup::tests

  test setup::tests::on_a_terminal_pressing_enter_throughout_keeps_an_existing_user_config ... FAILED
    left: "wrote your answers to the User config /home/me/.thirdshift/config.toml"
   right: "the User config /home/me/.thirdshift/config.toml already holds your answers"
  

  The complete-config fixture had no [review] merge.
  After: test result: ok. 52 passed; 0 failed

• Before: cargo test --test review_head_lag. review_times_out_on_a_head_that_never_reflects_its_push panicked at tests/review_head_lag.rs:60. Delivery reported "head moved" instead of the timeout, because the head-move check had been moved ahead of the readiness check for every run.
  After: that reorder applies only to merge requests. test result: ok. 4 passed; 0 failed

• Before: cargo test --test review_merge, with the review fixes reverted:

  test review_merge_config_does_not_break_recommend_only ... FAILED   (tests/review_merge.rs:178)
  test recovery_freezes_the_effective_review_merge_choice ... FAILED  (tests/review_merge.rs:162)
  test custom_review_self_merge_logs_under_its_own_step_number ... FAILED (tests/review_merge.rs:413)
  

  After: test result: ok. 11 passed; 0 failed; 2 ignored

• Final validation: scripts/affected-tests main selected the full suite and passed on this head:

  PASS npm test --prefix site
  PASS cargo test
  

  npm run docs:check --prefix site and cargo test prompts_page also pass.

Visual evidence

None captured. The three hand-written site pages changed one or two sentences of paragraph text each, with no markup, CSS or layout changes, so screenshots would show only the new wording.

Merge Danger

Door: two-way

The default is off, so nothing merges until a machine opts in with merge or [review] merge = true. Reverting this PR takes the feature away.

Blast Radius: opt-in

Machines that opt in will merge PRs made outside thirdshift, but only the head a Review run reviewed and watched go green, under ADR 0004's gates. Review runs without merge behave as before, including with merge.always = true.

Unaddressed findings

Standards

• Duplicated open-PR query in review_base_is_open_pr vs review_base_fixes (src/github/review.rs). Declined: it's a judgement call. The two readers return different shapes: a boolean, and fix commits on a matching PR. Sharing one would mean an extra abstraction for an eight-line filter.
• Duplicated --recommend-only/merge ensure! (src/args.rs and src/processes/review.rs). Declined. The parse-time check is the argument error with exit 2. The runtime check keeps the invariant where the effective goal is resolved, for any future caller of the shared Review process. Since this PR's fix, the config no longer feeds it (Goal::from_flag(flags.goal, config.review.merge && mode == Mode::Address)).
• Feature Envy in SkillWorktree::take_for_delivery (src/worktree/skill.rs). Declined: it's a judgement call. It is the only place a Skill checkout turns into a Delivery Worktree. The comment above it explains why it must not reacquire origin.
• let pass = reservation.is_some() as a proxy for an author Pass (src/processes/review.rs). Declined. In run(), Target::PullRequest gives (pr, None) and only Target::Author returns a reservation, so the value is exact for the Built-in Review run. Exit codes are covered by merge_holds_leave_ready_with_pass_specific_exit.

Spec

• A commit pushed after Delivery, arriving at the merge request, is reviewed by the Repair loop and then merged. Declined. The issue's own list of Self-merge rules includes "Foreign-commit handling, so a commit pushed after the review is never merged unreviewed … as a Fix run's Self-merge does". That handling is ADR 0004's Foreign-commit review. A push before Delivery completes holds the merge (src/processes/review/delivery.rs, unchanged_head) and leaves the PR ready with exit 0. That case is covered by merge_holds_leave_ready_with_pass_specific_exit. The behaviour the finding describes is pinned by cargo test --test review_merge review_merge_reviews_foreign_commits_that_arrive_at_the_merge_request, which passes.

Security review outcome

No unaddressed introduced findings.

Review coverage

• site/docs/index.html is generated and was intentionally not read by either reviewer. It was checked with npm run docs:check --prefix site, which exits 0.
• docs/code-navigation.md was changed by the review fixes after the Spec review, so the Spec reviewer did not read it. It is a one-row pointer update.

Closes #2219

Built with claude 2.1.314 · opus · default effort