docs(skills): harden goal-pr-review for concurrent review lanes - #7500
Conversation
Add concurrent-writers contract (active-writer window, advisory review-claim marker, content-identical head-move rule, label sweeps as authoritative, successor/superset detection), a shared-main baseline check before per-PR CI diagnosis (shared_main_blocked), decision-required triage with an author_decision parking state and bounded decision-packet format, body narrative/SHA-binding checks ahead of merge-ready, and a shared resource budget section. Terminal states join the output contract. Refs #7448, #7491 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔍 PR Contract Check SummaryOverall Status: 🟢 PASSED
ℹ️ Info
This check is mechanized. Please resolve any blockers to pass CI. |
Self-review: exact head
|
ll7
left a comment
ll7
left a comment
There was a problem hiding this comment.
Independent exact-head review — ff85bad12d75763ae861c6d6b0e6471902b96036
Reviewed under the maintainer ruling 2026-08-18, see #7506 (closure-lane demotions of
evidence-current PRs reversed; this lane owns the guarded merge path).
Scope verified against the diff, not the body. git diff origin/main...ff85bad --stat is exactly
two files: .agents/skills/goal-pr-review/SKILL.md (+140/-5) and CHANGELOG.md (+10). No runtime
code, no scheduler, benchmark, metric, evidence-admission, claim-ledger, release, settings, or
secrets surface is touched.
Not author-reserved. Against the four hard-stop categories: (a) no claim-status or
evidence-admission change — the file records review workflow only; (b) no preregistration
authorization; (c) no release/tag/settings/secrets/destructive op; (d) not authority expansion —
the new ## Decision-Required Triage section preserves all four author-reserved categories verbatim
and tightens two of them ((b) now excludes packets whose validator hard-requires
domain_approval.status: pending, (d) now requires a provable fail-closed-direction argument with
file:line evidence before a narrowing change counts as reviewable). Every other added section
(Concurrent Writers, Shared-Main Baseline, body narrative/SHA binding ahead of merge-ready,
Shared Resource Budget) adds preconditions to promotion; none removes one. Docs-only, fail-closed
direction.
Body ↔ diff reconciliation. Live title/body describe exactly the shipped delta; the "Validation /
Proof" claims are reproducible (below). Zero 40-hex SHAs in the body, so no unresolvable or
prefix-completed SHA carrier. The phrase "not merge-ready" occurs once, inside the Why It Matters
incident narrative quoting the #7374/#7435 bodies — it is not a self-referential readiness claim, so
the #7448/#7491 re-narration rule is satisfied without a body edit. No reconcile write was needed
(gh_pr_body_rest.py --reconcile would be a no-op).
Validation executed at this head (worktree .worktrees/res-7500-20260818 @ ff85bad12):
uv run python scripts/dev/check_skills.py→Validated 55 skills, typed registry, generated README, and routing tests.(reproduces the body claim)uv run python scripts/dev/check_base_sensitive_gates.py --pr 7500 --json→{"gate_required": false}→ ordinary base policy, no base-sensitive file changed.uv run python scripts/dev/check_pr_ci_status.py 7500→checks: success | 6 total | success=6 | completed=6atff85bad12d75763ae861c6d6b0e6471902b96036(docs-evidence-integrity, pr-contract-check, pr-body-contracts, promoted-planner-smoke, route-coderabbit, CodeRabbit).
Threads / reviewers. No unresolved actionable review threads; no outstanding explicitly requested
external reviewer. CodeRabbit posted no blocking finding. The prior owner self-review at this same
head is superseded by this independent exact-head review; its caveat ("hosted checks remain
required") is now discharged — all 6 checks are green at this exact SHA.
Confidence: High. Docs-only, deterministic validator green, no runtime surface.
gate-verdict: accepted @ ff85bad
base-policy: ordinary-cas @ ff85bad
pr-metadata: reconciled @ 59fd0eea6af046fec9e85cf84e4af5179c0323f95d381ca4df3afccdadc49f56
…7508) (#7519) ## Summary Make `scripts/dev/pr_loop_policy.py` machine-recognize the two workflow markers that PR #7500 introduced to `goal-pr-review` (docs-only): the advisory `review-claim: <lane> @ <head> until <UTC>` comment marker and the `author_decision` parking state (`decision-required` label + `### Decision packet` comment at the live head). PRs parked by either are classified `active_writer` / `author_decision` with recommended action `no_action`/stop, distinct from `blocked_preflight`, so reviewer lanes and the factory stop racing on the same branch. ## Linked Issues - Closes `#7508` - Refs `#7500` (defines the marker formats), `#7491` (not-ready-sentinel half, intentionally NOT implemented here) ## Stack / Dependency - Base dependency: none - Required prior PRs: #7500 (merged, defines the marker contract) - Stack follow-up issues: none - Safe to review independently: yes - Review dependency reason, if any: the marker format is defined by #7500's docs. ## What Changed - `scripts/dev/pr_loop_policy.py`: - `VALID_STATES` gains `active_writer` and `author_decision`. - New marker parsing: `_REVIEW_CLAIM_RE` (`review-claim: <lane> @ <sha> until <UTC>`), `_REVIEW_CLAIM_RELEASED_RE` (`review-claim: released @ <sha>`), `_DECISION_PACKET_HEADING_RE` (`### Decision packet`), plus a `ReviewClaim` dataclass. - New pure helpers: `_parse_review_claim_marker`, `_review_claim_released_shas`, `_has_decision_packet_heading`, `_trusted_marker_comments` (OWNER/MEMBER/COLLABORATOR only, reading both `comments` and `reviews` bodies), `active_review_claim` (unexpired + unreleased + head-bound, `now=None` -> `datetime.now(UTC)`), `has_author_decision_packet` (live-head binding; review-body packets must name the live head). - `classify_pr_state` takes an explicit `now` kwarg; draft/error/closed still return `no_action` first; `active_writer`/`author_decision` are checked before `blocked_preflight` and the rest of the pipeline (precedence per issue). - `recommend_action` maps both new states to `no_action` with named reasons; flow decision is `stop` for both. - Module docstring documents the lane-identity limitation (snapshot cannot distinguish lanes, so any unexpired unreleased trusted marker parks) and links **#7491 as explicitly out of scope** — no not-ready-sentinel implementation was added. - `tests/dev/test_pr_loop_policy.py`: 24 new tests with fixed timestamps (no monkeypatching): park on unexpired trusted claim, untrusted does not park, released clears, expiry, `now == until` is expiry, head mismatch, unparseable timestamp fails closed, claim in review body, parser round-trips, author_decision with/without label and packet, live vs stale head, untrusted packet, precedence over `blocked_preflight` and over ready-to-merge, draft still `no_action`, VALID_STATES contract, queue-level evaluation. - `CHANGELOG.md`: Unreleased -> Changed entry. ## Why It Matters - Added value: the loop tooling now parks PRs with an active review-claim or author-reserved decision instead of racing the branch, which was the observed 2026-08-18 failure mode (6 PRs had exact-head evidence voided by mid-run factory rebases; #7357 and #7347 raced within minutes). - Expected impact: workflow reliability only; no benchmark/planner/evidence semantics change. - Why this is worth merging now: closes the machine-recognition gap left by #7500 so the advisory markers become enforceable. ## Research Result Guidance - Target claim / hypothesis / blocker this should affect: NA - support/tooling-only; no research claim. - Comparator or baseline, if applicable: NA - Evidence tier: NA - support helper. - Result classification: NA - Decision or stop rule, if applicable: NA - Parent issue, claim map, registry, context note, or synthesis surface to update: NA - New research/benchmark/metric/paper-facing analysis tool, if any: NA - support helper. ## Domain-Aware Approval - Required for this PR: no - workflow/tooling classification change; no evidence classification, comparison methodology, figure eligibility, benchmark interpretation, or paper-facing claim change. - Domains reviewed: NA - Status: not required - Approver/review source or waiver: NA - Validity checklist (machine-detected labels unchanged): - Target claim/hypothesis: NA - Comparator or split/evidence validity: NA - Fallback/degraded exclusions: NA - Claim boundary: NA - Implementation integrity vs experimental validity: implementation integrity only. ## Falsification / Non-Transfer Check - Did the mechanism activate? NA - Did the intervention change command source, selected command, trajectory, or route progress? NA - Did the scenario actually contain the targeted failure mode? NA - Result route: NA - Follow-up question or issue for weak, negative, or non-transfer results: NA ## Next Empirical Action - Rerun needed: no - Extractor or analysis tool needed: no - Artifact missing or unavailable: none - Stop / revise / continue decision: NA - Proposed child issue or existing follow-up: none ## Validation / Proof - Commands run (worktree root): - `uv run pytest tests/dev/test_pr_loop_policy.py -q` — 174 passed (150 pre-existing + 24 new). - `uv run pytest tests/dev/test_snapshot_pr_queue.py -q` — 55 passed (companion). - `uv run ruff check scripts/dev/pr_loop_policy.py tests/dev/test_pr_loop_policy.py` — clean. - `uv run ruff format --check scripts/dev/pr_loop_policy.py tests/dev/test_pr_loop_policy.py` — clean. - Smoke: real `snapshot_pr_queue --active | pr_loop_policy --stdin --json` — exit 0, no crash; synthetic snapshot emits `active_writer` and `author_decision` states via the CLI. - Evidence that the change works here: 24 focused tests cover the marker grammar, expiry/release semantics, live-head binding, precedence, and queue-level behavior with fixed timestamps. - Benchmarks or smoke tests, if applicable: none (workflow tooling). ## Performance Evidence - (Not a `perf` change; section deleted per template guidance.) ## Risks / Rollout - Compatibility risks: additive states and an explicit `now` kwarg (default preserves current behavior); existing classifications re-tested (draft still no_action, ready_to_merge precedence preserved). - Failure modes: unparseable claim timestamps fail closed (documented); lane identity is not distinguishable in the snapshot, so any unexpired unreleased trusted marker parks (documented limitation). - Rollback or fallback plan: revert the single commit; the new states are purely additive. ## Docs / Provenance - Updated docs: module docstring in `pr_loop_policy.py` documents the marker grammar, precedence, lane-identity limitation, and the #7491 out-of-scope boundary; CHANGELOG entry. - Relevant design or provenance notes: marker grammar comes from the merged #7500 goal-pr-review docs. - Any assumptions that need to be preserved: the #7491 not-ready-sentinel half must remain tracked there, not duplicated here. ## Downstream Propagation - Parent issue updated (yes/no/NA): yes - issue #7508 will be closed by this PR. - Claim map / benchmark report updated (yes/no/NA): NA - Leaderboard / artifact catalog updated (yes/no/NA): NA - Registry or config index updated (yes/no/NA): NA - Context index / memory note updated (yes/no/NA): NA - Follow-up issue opened for deferred propagation (yes/no/NA): no - Not applicable because: workflow/tooling-only change with no research-facing downstream surface. ## Follow-Up Issues - Deferred work: the #7491 not-ready-sentinel machine check (tracked there). - Issues opened for follow-up: none. ## Reviewer Notes - Anything a reviewer should verify closely: the marker regex grammar matches the #7500 docs exactly (`review-claim: <lane-id> @ <head-sha> until <UTC>`); precedence of `active_writer`/`author_decision` over `blocked_preflight`; the live-head binding for author_decision packets. - Any known limitations: snapshot does not carry lane identity, so the active-writer check cannot exclude the claiming lane itself; documented in the module docstring. - Shared-helper migration: NA - single-file CLI helper change.
1 participant
Summary
Harden the
goal-pr-reviewskill for the situation observed on 2026-08-18: many reviewer lanes plus the autonomous factory acting on the same PR set, a red sharedmain,decision-requiredlabels that were mostly reducible, and PR bodies whose metadata was reconciled byte-wise but not truth-wise. Docs-only; no runtime behavior changes.Linked Issues
#7448(fabricated exact-head SHAs in gate-verdict trailers)#7491(merge-ready + stale "not merge-ready" narrative reaching main)#7482(shared-main namespace baseline that made unrelated PRs look red)Stack / Dependency
What Changed
.agents/skills/goal-pr-review/SKILL.md(Codex mirror is a directory symlink; no separate copy):review-claim: <lane> @ <head> until <UTC>marker; content-identical head moves (main refresh only) transfer findings but require re-publishing exact-head carriers after green CI; factory/owner label sweeps are authoritative ("one label away" reporting, no re-apply in-run); successor/superset detection across open PRs on the same issue.origin/mainfirst; classify matching PR failures asshared_main_blocked; route one bounded repair instead of per-PR "fixes".author_decisionparking state mapped toAUTHOR_DECISION_REQUIRED; ≤25-line decision-packet format; keep the branch mergeable while parked.merge-ready.CHANGELOG.md: Unreleased/Changed entry.Why It Matters
merge-readysweep while merges were queued; friction: refresh benchmark namespace inventory after merged trace-dossier module #7482 re-diagnosed per PR; 12 of 15decision-requiredPRs proved reducible; fix(ci): retry transient review-bot routing API errors #7374/fix(ci): require exact-head changed coverage for admission #7435 bodies said "not merge-ready" while labeled/merged; API quota exhausted twice;/dev/shmfull twice).Research Result Guidance
Domain-Aware Approval
Falsification / Non-Transfer Check
NA — no empirical claim.
Next Empirical Action
NA.
Validation / Proof
uv run python scripts/dev/check_skills.py→ Validated 55 skills, typed registry, generated README, and routing tests.uv run pytest tests/dev/test_check_skills.py tests/dev/test_factory_v2_skill_contract.py tests/dev/test_token_efficient_thread_profile_snapshot_command.py -q→ 64 passed, 1 skipped.uv run python scripts/tools/sync_ai_config.py --check→ 7 symlinks validated.uv run python scripts/dev/check_docs_evidence_integrity.py --files …→ 2 changed files passed.uv run pre-commit run --files .agents/skills/goal-pr-review/SKILL.md CHANGELOG.md→ passed.Performance Evidence
NA — docs only.
Risks / Rollout
review-claimmarker is advisory until tooling (factory /pr_loop_policy.py) recognizes it; the text says so.author_decisionis a documented parking state, not a newpr_loop_policy.pyclassification; a follow-up could add machine detection ofdecision-required+ packet presence.Docs / Provenance
Downstream Propagation
NA — not an evidence-producing PR.
Follow-Up Issues
pr_loop_policy.py/ merge gate to recognize thereview-claimmarker and the not-ready-sentence check (friction(merge-gate): merge-ready + stale 'not merge-ready' body narrative reaches main (#7374, #7435) #7491 already tracks the gate side).Reviewer Notes
Diff is additive apart from one table row (
failed_cimapping) and the Output Requirements list. Long lines are inside Markdown table rows, matching the existing file style.