Skip to content

Report review-required policy refusals before Self-merge - #708

Merged
JacobStephens2 merged 4 commits into
mainfrom
issue-707
Oct 16, 2026
Merged

JacobStephens2 merged 4 commits into
mainfrom
issue-707

Conversation

@JacobStephens2

@JacobStephens2 JacobStephens2 commented Oct 16, 2026 •

Copy link
Copy Markdown
Owner

Summary

before each Self-merge (Run or Spec PR)
  read reviewDecision for the captured PR
  REVIEW_REQUIRED / CHANGES_REQUESTED → review required policy refusal
  approved / empty / unknown / failed read → existing guarded merge

Review-required refusals exit 1, leave the PR ready with its Claim kept, and skip the merge attempt and Repair round. stderr, Run notifications and the Activity log name the reason.

Closes #707

Evidence

  • Before: the initial focused CLI regression failed: REVIEW_REQUIRED still merged the PR and exited 0 instead of 1.
    After: cargo test --test merge_run a_required_review_refuses_the_merge_without_another_round_and_names_the_outcome -- --exact exercises the consolidated regression for both blocking decisions: no merge/Repair, PR stays ready, Claim kept, cleanup completes. Coverage includes another read after Repair, unknown/null decisions, and failed reads with successful merges or other policy refusals.
  • Before: cargo test --test run_notification a_review_required_notification_and_summary_name_the_refusal -- --exact failed with the subject ending : failed.
    After: it passes with review required in the notification and Activity log. The Spec PR test also passes for both blocking decisions.
  • Validation: cargo check --all-targets, cargo fmt --check, and cargo clippy --all-targets -- -D warnings pass. cargo test -- --test-threads=4 passes for the full Rust suite.

Merge Danger

Door: two-way

Reversible code and documentation change; no migrations.

Blast Radius: Self-merge

Adds one best-effort GitHub read immediately before each merge request. Other policy refusals retain their existing handling.

Test seams

  • The thirdshift CLI against real Git and fake gh: Self-merge decisions, Repair, PR readiness, Claim retention, cleanup, stderr/stdout and exit status, for Run and Spec run.
  • Run notification delivery through the local Resend stand-in and the Activity log summary.

Unaddressed findings

Standards

  • STD-001 / S1 (same optional Low finding; declined): Retain the existing string-based reporting interface (src/run_ending.rs:94) for this scoped outcome addition. This is an implementation plan decision: Account already carries the display outcome; summary reuses it at lines 279–281, while the policy decision itself remains typed as PolicyRefusal::ReviewRequired at lines 295–299. Adding a second reporting category or stored summary prefix solely for this label would duplicate that state. The reviewer explicitly identified a maintenance judgment, not incorrect behavior. No Spec or Day shift decision is needed.

Spec

None.

Security

None. Single-session guidance review of all 13 changed files and supporting PR identity, guarded merge, interruption, failure cleanup, and notification code against pinned merge base 3574ae7267507e8fd67403538e0a851d8030f1d6 found no introduced security findings. Read the relevant attack-class guidance; no repository SECURITY.md or threat model was present.

Validation: 29 review-focused tests across merge_run, spec_run, and run_notification passed in a network-isolated sandbox using local stand-ins. No source fixes were needed.

Review coverage

Reviewed against main with fresh, independent Standards and Spec reviewers after reconciling the branch. The final pass reported no findings; the initial pass's optional STD-001 remains documented above. Both read all 13 changed files; unread changed files: none. Reports are retained in .thirdshift-review-1tPh0F/standards.md and spec.md in the Run worktree.

Built with codex · gpt-6.1-sol · high

Review since 506f9b7 with independent
Standards and Spec reviewers. All three changed files read on both axes.
Standards: one low-severity Primitive Obsession judgement declined with
locality evidence in PR #708. Spec: no findings. No source fixes warranted.

Review-only empty commit; reports retained in .thirdshift-review-ClfBLi/.
Validation: formatting, Clippy, 23 ending tests, 26 Merge run tests,
31 notification tests and 18 Spec review tests passed.
@JacobStephens2
JacobStephens2 merged commit 259b944 into main Oct 16, 2026
18 checks passed
@JacobStephens2
JacobStephens2 deleted the issue-707 branch October 16, 2026 20:53
@JacobStephens2
JacobStephens2 restored the issue-707 branch October 16, 2026 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A Merge run reports "review required" without trying a merge it knows will be refused

1 participant