[JacobStephens2]

Summary

Release PRs could wait indefinitely on missing workflows or a superseded head. Observe the captured PR throughout CI and reuse the existing Fix run for bounded recovery.

observe release PR/head
  conflict or completed CI failure → one Fix(URL, --no-merge, --no-base-fix)
  changed head → validate exact bump → requested Experiment → refresh notes/diff
  missing workflows → 300-second startup grace → recovery instructions
  current green head → tag → publish → merge

Cap external head changes at five and refuse changed PR identity, state, branches, origin, version or extra source content before tagging. Persist the release notes' content base across interrupted runs, refresh legacy notes, and preserve repaired merge-head recovery for failed unpublished tags. Published releases remain immutable. Ctrl-C during Fix startup prints recovery commands; a failed post-publication merge explains marking a draft PR ready.

Closes #2016

Evidence

Execution evidence

• Before: At supplied merge base acb42c1f90c8c637b7295d8ab0d321d6fb1097d1, with the branch's regression fixtures copied into the detached checkout, cargo test --test release checks::a_conflict_without_workflows_runs_one_targeted_fix_and_releases_the_repaired_head -- --exact --nocapture failed: release fixture exceeded 120s, printing waiting for CI on …/pull/4; 0 passed; 1 failed. The fixture establishes real competing Cargo version bumps and verifies their merge conflict.
  After: The same regression on the issue branch passed: 1 passed; 0 failed, releasing the repaired head through the fake publication workflow. A separate sourced merge-base probe observed 26 empty workflow reads until its three-second deadline.
• Before: cargo test --test release checks::a_signal_interrupted_fix_reports_recovery_without_tagging -- --exact --nocapture failed: signal termination produced status.code() == None and omitted recovery instructions. The reviewer's real-Cargo startup reproduction also failed with Bash exit -2.
  After: The same command passed: 1 passed; 0 failed; the release reports its PR/head and recovery commands, with no tag pushed.
• Before: cargo test --test release checks::a_published_draft_pr_explains_how_to_finish_merging_without_moving_the_tag -- --exact --nocapture failed on missing draft/ready guidance after publication.
  After: The same command passed: 1 passed; 0 failed; the published tag stays fixed and the PR remains open with gh pr ready guidance.
• Both Spec findings were reproduced by running the reviewers' commands before deciding them. The real-Cargo interruption reproduction and the state-aware draft verification pass; the retained CLI regressions exercise both fixes. Exact commands and the original draft fake's later limitation are recorded in the Spec report.
• cargo test --test release checks:: -- --test-threads=3: 22 passed, including identity mutations, extra-content refusal, head churn, missing-run expiry, Fix failure, Experiment refresh and interrupted publication recovery.
• scripts/affected-tests main passed all 68 site tests (152.80s), then stopped on unchanged dismiss::single_kind_dismissal_files_keep_their_kind_when_another_is_added: 617 passed, 1 failed, 97 skipped; the remaining tests were not run. It expected a dismissible row but observed Working. cargo test --test dismiss single_kind_dismissal_files_keep_their_kind_when_another_is_added -- --exact --nocapture then passed: 1 passed; 0 failed. No dismissal code was changed.
• cargo test --test release -- --test-threads=3: 131 passed, 2 fixture entry points ignored at fb5071d5. Repeated after the main reconciliation at 6d0153ea: 131 passed, 0 failed, 2 ignored (279.48s).
• python3 -m unittest discover -s scripts -p test_generate_release.py: 3 passed. npm run docs:check --prefix site passed after the final main merge.
• Rust CI on 6d0153ea passed. The current head is 25d92bc3: a main reconciliation followed by an empty failure-marker commit after the previous run encountered a draft PR. Current-head CI passed, including the full Rust suite, formatting, clippy, site and Console checks. Both musl artifact builds also passed on 25d92bc3. Successful exact-head CI is reused for full-suite validation under docs/testing.md.
• cargo fmt --check, bash -n scripts/release.sh and git diff --check passed.

Test seams: the real release CLI, Git refs, PR/workflow observations through fake GitHub, and targeted Fix/Experiment/summary commands through fake Cargo/Harness adapters. Fixture children have bounded lifetimes; tests launch no real agents or releases.

Visual evidence

Release documentation at matching viewports with the release section open. Before is the supplied merge base; After is the reconciled branch. Captures contain no personal data.

Release manual, 1440×900:

Release manual, 390×844:

Homepage, 1440×900:

Homepage, 390×844:

How it works, 1440×900:

How it works, 390×844:

Architecture, 1440×900:

Architecture, 390×844:

Review coverage

Standards: 2 P3 findings, both addressed by sharing the Experiment Cargo stand-in and the existing child-lifetime runner. Spec: 2 P2 findings, both reproduced and fixed with retained regressions for signal interruption and published draft recovery.

Both reviewers read all nine changed files, including generated site/docs/index.html, and reviewed the later draft-recovery additions through fb5071d5. Generated exclusions: none. Changed files left unread: none. Final reports and summary.json ({"standards":2,"spec":2}) are retained with the Session trace.

The final upstream reconciliation 6d0153ea exactly matches git merge-tree --write-tree fb5071d5 b02e7ae2 (tree b113e2791afee887747ac6fc3c6e3635786317d4); the PR-owned diff still contains the same nine reviewed files. The earlier parallel review's S1, S7 and S8 and P1 are also addressed. Its remaining declined judgements are preserved below.

Unaddressed findings

The current review's two Standards and two Spec findings are all fixed. The earlier parallel review left these judgements declined:

Standards

• S2 — refusal names: Keep release_blocked for recoverable blockages and release_stopped for identity/origin/release-boundary changes. The comment at scripts/release.sh:720 explicitly defines why the latter offers no Fix run.
• S3 — repeated content-base calculations: write_summary intentionally uses git merge-base "$head" origin/main (scripts/release.sh:523) for a new re-cut bump, including while the previous PR is still recorded as MERGED. release_content_base uses $merge^1 to validate the old merged PR. recovery::an_old_merged_bump_can_be_recut_with_a_new_bump_pr exercises this distinction. Within the open-PR wait, the expressions agree.
• S4 — repeated observation/exit pattern: Keep the head guards around operations that can yield, including scripts/release.sh:634 and :655. checks::an_external_head_change_during_a_green_read_is_adopted_without_a_fix exercises the race these guards close. The two recovery messages deliberately offer different actions: release_blocked offers a targeted Fix; release_stopped asks for inspection.
• S5 — globals and wait-function size: Retain one release wait loop alongside the script's existing release state. Its shared fix_attempted=1 (scripts/release.sh:706) and five-update cap (:607) own invocation-wide budgets. The bounded-repeat and head-churn regressions exercise those budgets; a larger shell-state refactor is outside this bug fix.
• S6 — Python JSON instead of jq: The strict JSON boolean check at scripts/release.sh:736 handles both real GitHub and the fake adapter, whose jq boolean output is Python-style True. a_draft_bump_pr_after_publication_names_the_ready_step passes through this check.
• S9 — repeated site paragraph: AGENTS.md explicitly requires updating each hand-written site page. Each page already carries its own release description; keeping its recovery paragraph adjacent follows that layout.

Spec

• P2 — refresh unmarked legacy notes once: Without a recorded content base, the old summary cannot establish which content it describes. release_notes_refresh=1 (scripts/release.sh:567) conservatively regenerates it. checks::an_unmarked_legacy_pr_repaired_before_resume_refreshes_its_notes exercises the stale-notes case, and the manual documents the one-time refresh.
• P3 — pre-tag draft refusal and mergeability grace: The issue's supplemental draft failure establishes that draft readiness can block the final merge. Refusing before publication exposes gh pr ready earlier. Mergeability's clock starts only after all workflows complete (settling=${settling:-$SECONDS}, scripts/release.sh:667); it places no deadline on queued/running CI. checks::green_ci_with_mergeability_stuck_unknown_stops_instead_of_waiting_forever and checks::pending_and_healthy_ci_never_start_a_fix_even_after_startup_grace exercise both sides.

These are implementation judgements supported by the current code and regressions; none is deferred to a Spec or Day shift decision.

Security review outcome

No unaddressed introduced findings.

Takeover verification

Reviewed head 25d92bc3 against pinned main 4a301fab. The failure-marker commit changes no files. Marked the PR ready after review; no code changes were necessary.

• CARGO_TARGET_DIR=/home/jacob/repos/thirdshift/target cargo test --test release checks:: -- --test-threads=3: 23 passed, 0 failed (71.94s after compilation).
• python3 -m unittest discover -s scripts -p test_generate_release.py: 3 passed.
• npm run docs:check --prefix site, bash -n scripts/release.sh, and git diff --check origin/main...HEAD: passed.

Standards

No actionable findings. The final nine-file change follows the documented standards, uses the existing fake adapters, and bounds fixture children. Generated manual consistency was checked with docs:check. 0 findings; no blocker.

Spec

No findings against issue #2016. The final change covers bounded targeted Fix recovery, current-head validation, startup grace, notes and requested Experiment refresh, interrupted/draft recovery, and publication safeguards. 0 findings; no blocker.

Merge Danger

Door: two-way

The script changes can be reverted. Publication retains the existing immutable-release and publication-before-merge safeguards.

Blast Radius: releases

Maintainer release orchestration and its documentation/fixtures. One automatic targeted Fix run can consume agent time when a release PR has a conflict or completed failing CI.

[JacobStephens2]

Review run. Pushed fixes to issue-2016 at 63be9d1. Spec axis used the linked closing issues.

Review of PR #2021 (issue #2016): release PR recovery

The review found 8 Standards findings and 3 Spec findings. I fixed 3 confirmed bugs plus 4 of the code-quality findings, and declined the rest. Everything is in one commit, 63be9d16, which is not pushed. Final validation against origin/main passed.

Fixed point: the ref refs/remotes/origin/895f4b76… doesn't exist in this clone, so I used its SHA, 895f4b76754d21af25dc490614a0eff11dd4c107. That is origin/main, the PR's base.

Spec source: linked closing issue #2016 and its Agent Brief, from .thirdshift-review-input.md.

PR state: open and not draft at every check.

Standards (8 findings: 2 P2, 6 P3; no hard violation of a documented standard)

• S1 (P2): in tests/release/checks.rs, run gave each whole release.sh run a 10-second bound. docs/testing.md documents the shared 120-second WAIT_BOUND for these fixtures, and CODING_STANDARDS.md "Deadlines in tests" warns about short bounds on loaded runners.
• S2 (P2): a_base_change_during_the_experiment_is_measured_before_tagging had its real asserts inside if tag == repaired. A run that exits 1 without tagging would still pass.
• S3 (P3, Duplicated Code): forge_new_base_during_experiment copies the Experiment stub from experiments::cargo_reports and repeats adopt_script's lines.
• S4 (P3, Duplicated Code): remember_notes_claim and the top of wait_for_pr_checks repeated the same notes-base lookup.
• S5 (P3, Divergent Change): wait_for_pr_checks grew from about 10 lines to about 95 and handles eight concerns.
• S6 (P3, Data Clumps): trios of globals travel together, and pr_body depends on summary_base having been set earlier by write_summary.
• S7 (P3, Mysterious Name):
  • fix_runs installs a simulated Fix but reads like a query.
  • One test has a spec_ prefix.
  • The local run wrapper hides run_script naming.
  • An inline checks literal sits beside the GREEN/RED constants.
• S8 (P3, documentation): the manual didn't mention that a resumed PR without a notes marker has its summary regenerated, or the local notes ref.

Spec (3 findings, all P3)

• SP1: with green CI and GitHub reporting mergeability as UNKNOWN, the wait loop never ends.
• SP2: a resumed PR without the marker always regenerates its summary, even when its content hasn't changed. The spec asks for a refresh only "when the release content changes".
• SP3: every blocked exit says the PR "is left open" and recommends thirdshift fix. That advice is wrong when the PR is closed or retargeted, origin changed, or the content base predates the latest release.

Full reports are in .thirdshift-review-ongm3I/standards.md and spec.md. summary.json is {"standards":8,"spec":3}.

What I fixed

• SP1: I added the reviewer's test unchanged. Before the fix it hung until the bound ran out ("release fixture exceeded 10s"). wait_for_pr_checks now gives GitHub's mergeability report the same startup grace once checks pass, then stops with no tag. The test now passes and also checks the message and that no Fix ran.
• SP3: I added the reviewer's test. Before the fix it failed, printing "the PR is left open … Recover with thirdshift fix …" for a closed PR. A new release_stopped message now covers origin, identity/state, branch-head and stale-base stops. It says "Inspect the PR, then rerun scripts/release.sh …". The test passes.
• S1: the check tests use the shared WAIT_BOUND, and I removed the now-unused run_script_with_bound.
• S2: success, the tag and the measured base are now asserted unconditionally.
• S4: I extracted a read_notes_claim helper that both callers use.
• S7: I renamed fix_runs to simulate_fix.
• S8:
  • docs/guide.md now covers the mergeability grace, the non-Fix recovery advice and the one-time refresh for PRs without the marker. I regenerated site/docs/index.html.
  • The three hand-written site pages now mention the mergeability grace.

Unaddressed findings

Standards

• S3: the hook has to fire in the middle of an Experiment. Adding a hook parameter to the shared cargo_reports would change the fixture for every Experiment test to serve one.
• S5: this is a judgement call. S4's extraction shortens the function; a fuller split would be a refactor outside this review's fixes.
• S6: this is a judgement call. The globals match the script's existing style, and every current caller keeps the right order.
• S7, partly declined:
  • The spec_ test name is cited in the PR body's evidence, so I didn't rename it.
  • The checks literal appears only once in checks.rs (line 564), so a constant wouldn't remove any duplication.

Spec

• SP2: the PR body explains this departure: "Refresh unmarked legacy notes before tagging repaired content". The comment in read_notes_claim adds that without a marker, the current head alone cannot establish what the summary describes. I listed it here instead of fixing it, filed no issue, and documented the behaviour in the manual (S8).
• SP1, second half: a real conflict that GitHub still reports as UNKNOWN could use up the startup grace. I didn't reproduce this. GitHub settles the value to CONFLICTING, and the release now stops either way.

Coverage

• Both reviewers read all 9 changed files, including generated site/docs/index.html.
• Generated-output exclusions: none.
• Changed files left unread: none.

Validation

Command	Result
npm ci --prefix site, needed before the docs check	exit 0
npm run docs:check --prefix site, before and after regenerating	exit 0
cargo test --test release -- --test-threads=3, before the fixes	125 passed, 2 ignored
cargo test --test release -- checks::green_ci_… checks::a_pr_closed_…, before the fixes	exit 101: both failed as the findings described
cargo test --test release checks::, after the fixes	20 passed
cargo test --test release -- --test-threads=3, after the fixes	127 passed, 2 ignored
cargo fmt --check	passed
cargo clippy --all-targets -- -D warnings	exit 0
scripts/affected-tests main	invalid run; see below
scripts/affected-tests origin/main	exit 0, details below

The scripts/affected-tests main run doesn't count. Local main is stale (6bcde725, behind origin/main), so the selector picked up unrelated changes and the Console browser tests:

• npm test --prefix site and cargo build --locked passed.
• npm test --prefix console failed in 1.45s: every test file failed at once. console/node_modules was never installed, and the PR doesn't touch the Console.

scripts/affected-tests origin/main is the real final validation:

• npm test --prefix site: PASS (31s).
• cargo test, the full Rust suite: PASS (833s). 4,255 passed, 0 failed, 97 ignored. It fell back to cargo test because nextest isn't installed.
• python3 -m unittest discover -s scripts -p test_generate_release.py: PASS.

The extra-command list was empty, so there was nothing else to run.

Security

No unaddressed introduced findings.

Delivery failed

pull request changed during Review run