Add coverage-of-change gate to verify and a commit-message policy - #342
Add coverage-of-change gate to verify and a commit-message policy#342justin808 wants to merge 4 commits into
Conversation
Summary: Adds a Coverage-Of-Change Gate and Commit Message Contract to skills/verify/SKILL.md (skills/tdd/SKILL.md is the routing target for missing tests), and a new canonical workflows/commit-messages.md policy for proportional commit messages and receipt placement. Why: A green verify run proved the suite passed but not that changed behavior was covered (#256); generated commit messages and PR receipts were disproportionate and duplicated into permanent Git history (#318). Fixes #256 Fixes #318
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 1 minute You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
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 |
Review SummaryThis PR adds a Coverage-Of-Change Gate to What it does well
Issues found (see inline comments for detail)
Other notes
|
Summary: Corrects an overstated seam-parity claim (never_skip_paths has no parser, unlike human_review_paths), spells out that the docs-only exemption never covers skills/** or workflows/** prose, and adds guidance for test-only diffs where the change is the test itself. Why: claude-review flagged all three in PR #342, all inside skills/verify/SKILL.md.
Review SummaryThis PR adds a Coverage-Of-Change Gate to What's good
Issues found (posted inline)
Minor observations (not blocking)
Security / performanceNot applicable — this PR only changes prose specification files ( |
Summary: Splits the Output Format example into a failing run (no receipts) and a fully green run (receipts included), and adds an explicit instruction to return to step 4 for files tdd touches before recording the COVERAGE receipt. Why: claude-review flagged both on PR #342's second head: the prior single example contradicted "stop on first failure," and nothing told the agent to re-verify files tdd just added.
Review SummaryThis PR is a pure prose/policy change (no code, no tests — as expected for a What I verified
Issues found (posted inline)
OverallThe design is well cross-referenced against existing conventions rather than inventing parallel ones, and the PR's own Decision Log is unusually transparent about scope boundaries and deferred work (#324, #338). No correctness or security concerns — the only real risk is #1 above (an open-ended reproduction loop for flaky-test fixes), which is worth tightening before merge; the other two are style nits. |
Summary: Adds a 3-attempt bound with a documented-evidence fallback for reproducing a flaky test before a fix (no more unbounded looping), splits the dense gate-trigger paragraph into three short bullets, and extends the Output Format cross-reference to steps 4, 7, and 8. Why: claude-review found an unbounded loop in the flaky-test path on PR #342's third head, plus two clarity nits worth fixing since #256's premise is that agents follow this text literally.
Review SummaryThis PR adds a Coverage-Of-Change Gate to Cross-reference verificationI checked every concrete claim the PR makes against repo state rather than taking the PR description at face value:
All checked out -- the cross-referencing is accurate, which matters a lot for a doc whose entire job is to be a reliable pointer graph for other agents. Code quality / correctness
Security / riskNo traditional security issues (this is instructional prose, not executable code). The main risk worth naming: the gate is entirely self-reported/self-graded by the agent doing the work -- the PR is explicit that PerformanceN/A -- no executable code paths changed. The gate does add real per-PR cost going forward (revert-hunk / rerun / restore / rerun, twice per distinct behavior), which is an intentional tradeoff for coverage rigor, not a performance bug. Test coverageThe PR self-applies its own new gate to its own diff ( Overall: solid, well-cross-referenced policy work with a fair amount of self-awareness about its own limitations (already flagged in the Decision Log). The inline comments are refinements, not blockers. |
QA Evidence — independent Batch QA Lane (
|
Batch handoff — AW F 08-05 08:40 (lane
|
| Gate | Result at head c6e2921827797ebcaca7effef1503af7f45f9b7f |
|---|---|
pr-ci-readiness v2 |
READY (exact head) |
autonomous-merge-eligibility |
human-approval-required — triggered: architectural-product-judgment, public-compatibility |
merge-assurance |
eligible: false, verdict: BLOCKED — "auto merge authority lacks a qualifying exact-head autonomous verdict" |
| Evidence failures | none (clean policy result, not UNKNOWN) |
| Rollback | code-only-rollback-established |
merge_authority for this batch was auto_merge_when_gates_pass. That authority does not apply here: the repo's own autonomous_merge policy in .agents/agent-workflow.yml triggers the gates above, so the batch terminates at human review rather than merging. Separately, the seam sets merge_submission: mode: merge_queue_only and no merge queue is enabled on main, so pr-merge-submit would refuse before mutation regardless.
QA: independent Batch QA lane (checker-aw-f, distinct from every maker) — qa-evidence v2 marker on this PR, status: satisfied, release_blocking: clear, bound to head c6e2921827797ebcaca7effef1503af7f45f9b7f.
Follow-up #356 records three deferred refinements to the coverage gate (NO_HARNESS dominance, multi-hunk revert fallback, multi-behavior receipts); #324 covers the deferred #318 fixture criterion. Neither blocks this PR.
coordination: unavailable — repo seam sets coordination_backend: "n/a" (.agents/agent-workflow.yml:10), single-operator source repo.
Completed-batch audit: not published. completed-batch-publication-preflight cannot run here for two independent reasons — there is no coordination status to supply (backend n/a), and no target is merged, which it refuses on its own.
Local merge-gate artifacts (CI-readiness / eligibility / assurance JSON) were transient and did not survive temp cleanup; regenerate from the helpers in skills/pr-batch/bin/ against base 282a87aaf4bf1dd602723efddc78252f7a138235.
Fixes #256
Fixes #318
Why
verifyproved the existing suite stayed green, but nothing checked whether the change under review was covered by any test — a PR could fix real behavior, pass every gate, and ship zero assertions on the thing it fixed (#256, worked example: react_on_rails PR #2489, where the fix is still uncovered four months later). Separately, a 30-commit Shakapacker audit found the dominant AI-slop signal wasn't code defects but oversized generated commit messages with decision logs and QA receipts duplicated permanently into Git history (#318).This PR adds:
skills/verify/SKILL.md: for any non-docs diff, require either a named test demonstrated to fail without the change (revert-and-watch-it-fail), or an explicit skip from a closed four-code set (MECHANICAL,GENERATED,NOT_OBSERVABLE,NO_HARNESS). Free-text "no test needed" cannot satisfy the gate. The gate routes intoskills/tdd/SKILL.md(new "Coverage-Gate Entry Point" section) instead of reimplementing the RED/GREEN loop, and emits aCOVERAGE <test path/name> fails without change/COVERAGE SKIPPED <code>receipt in theverifyoutput block. A repo seam (coverage_of_change.never_skip_pathsin.agents/agent-workflow.yml) can tighten the gate by forbidding skips on named paths, but cannot loosen or disable it.workflows/commit-messages.md: an ownership map for where commit rationale, decision logs, QA receipts, and review dispositions belong (cross-referencing the existingCodex Decision Log,qa-evidence v2, andpriority-finding-dispositions v1conventions already inworkflows/pr-processing.mdrather than duplicating them); a concise subject+Summary+Why+issue-linkage commit contract; a qualitative (non-line-count) concise mode for small diffs; duplicate-section detection/collapse guidance between commit body and PR description; and explicit provenance/changelog preservation rules.skills/verify/SKILL.mdgets a matching "Commit Message Contract" step andCOMMIT-CONTRACT <concise|full>: <reason>receipt, satisfying Keep generated commit messages proportional and store audit receipts outside Git history #318's "at least one authoring/closeout skill enforces or validates the concise commit contract" criterion.Decision Log
skills/ship/SKILL.mddoes not exist in this repo, and was not created. The batch envelope that spawned this lane listed it as a possible wiring target, but/shipbelongs to a different pack (gstack), notagent-workflows.skills/verify/SKILL.mdis this repo's actual authoring/closeout surface, so the commit-message contract is wired there instead.workflows/commit-messages.mdis the new canonical home for Keep generated commit messages proportional and store audit receipts outside Git history #318's policy, rather than folding it intoworkflows/pr-processing.md.pr-processing.mdalready defines the receipt formats this policy references (Decision Log,qa-evidence v2,priority-finding-dispositions v1) but is owned by a different lane in this batch and is large enough that adding a full commit-message contract to it would blur its scope. The new file is picked up automatically —bin/install-agent-workflowsinstallsworkflows/*andbin/agent-workflow-seam-doctorglobsworkflows/**/*.md— so no registry file needed a matching update.test/fixtures/**, which this lane does not own. Filed as #324, with the concrete direction (fixture shape, validator pattern to follow) written into the issue so it's actionable without this PR's context. Criteria 1, 2, and 4 are delivered in full.skills/tdd/SKILL.mdcarries a<!-- Keep this skill in sync with workflows/tdd.md -->marker, butworkflows/tdd.mdis owned by a different lane. The new "Coverage-Gate Entry Point" section was added as a new trailing section rather than edited into the mirrored Core Loop/Guardrails/Before Pushing/Done text, so those sections stay a byte-for-byte match withworkflows/tdd.md.workflows/tdd.mditself does not yet have an equivalent pointer back toverify's gate — a follow-up for whichever lane ownsworkflows/tdd.mdnext.bin/**helper was added for either issue. The coverage gate and commit contract are both agent-followed prose steps (revert/restore, RED/GREEN evidence, duplicate-section comparison), consistent withbin/**being outside this lane's owned paths; commit-messages: add fixtures proving QA-evidence replay survives commit-body duplication collapse #324 covers the one piece (fixtures + a validator) that would need new script surface.CHANGELOG.mdwas intentionally left untouched. It isn't in this lane's owned-paths list and wasn't part of the coordinator's deliverable steps; flagging it here so batch closeout (orupdate-changelog) can add an entry for this PR alongside the batch's other lanes.skills/pr-batch/bin/agent-coord-bounded-test.rb(unrelated to this PR, outside this lane's owned paths) was found while manually reconstructing the fullbin/validaterun and filed as #338 rather than fixed here, with full repro/non-repro evidence in the issue. See the Verification section below for the diagnostic detail.Verification
Local commands run from this branch's head, matching
AGENTS.md→ Validation..agents/bin/lintpassed clean first:bin/validate(mandatory pre-commit gate). The full script (bin/validate) runs longer than this environment's 10-minute single-command cap even under the correct locale, so instead of truncating coverage I ran every one of its constituent commands individually, in the exact orderbin/validatedefines them (verified by reading the script), each to completion with a captured exit code. Every command exited0with real "N runs, M assertions, 0 failures, 0 errors" output, except two flakes fully diagnosed below. Representative real output:Pre-existing failure #1 —
bin/push-downstream-test.rblocale issue (already tracked, not caused by this PR). Under this shell's defaultLANG=""/Clocale,test_policy_apply_compares_validated_non_ascii_policy_content_as_bytesraisesEncoding::CompatibilityError: incompatible character encodings: US-ASCII and UTF-8atbin/push-downstream-test.rb:1576. I reproduced this on a cleanorigin/maincheckout (this branch's changes stashed) and the coordinator independently reproduced it too, on base282a87a, getting the identical110 runs, 595 assertions, 0 failures, 1 errors/ sameEncoding::CompatibilityErrorat the same line. It's already tracked as #281 ("bin/push-downstream-test.rbis locale-dependent and abortsbin/validatewhenLANGis unset"). Running withLANG=en_US.UTF-8 LC_ALL=en_US.UTF-8avoids it entirely — the command above shows0 failures, 0 errorsunder that locale.Transient flake #2 —
skills/pr-batch/bin/agent-coord-bounded-test.rb(unrelated, not reproducible, newly filed as #338). This file failed 2 of 14 tests once, mid-sequence, with process-group timing signatures (fake helper did not start; empty stdout where output was expected) — consistent with system load, not a logic bug. I verified it is not caused by this PR: (a) re-running the single failing test immediately after passed (1 runs, 7 assertions, 0 failures); (b) re-running the full file in isolation on a cleanmaincheckout (this branch's changes stashed) passed clean (14 runs, 81 assertions, 0 failures, 0 errors); (c) re-running the full file in isolation on this branch (changes present, unstashed) also passed clean (14 runs, 81 assertions, 0 failures, 0 errors). This branch touches zero Ruby/bin/**/skills/pr-batch/**files, so there's no plausible causal link. Filed as #338 with full repro evidence and a suggested direction (likely a fixed short timeout that should tolerate contention).Also ran, self-applying the new gate to this PR's own diff:
COVERAGE SKIPPED NO_HARNESS: this change is prose policy in skills/*/SKILL.md and workflows/*.md; the repo's only automated checks for these files are frontmatter-shape validation (bin/validate) and markdown/prose lint (bin/lint), neither of which tests skill/workflow semantic content — no harness exists to assert on agent-followed instructions.COMMIT-CONTRACT full: multi-file policy change across two issues with several load-bearing design decisions (seam key shape, closed skip-code set, ownership map) — not a small/mechanical diff.Confidence note
High confidence in the design and cross-references (verified against the existing
Codex Decision Log/qa-evidence v2/priority-finding-dispositions v1conventions inworkflows/pr-processing.mdrather than inventing parallel ones); medium confidence in the exactcoverage_of_change.never_skip_pathsseam key name since this source pack doesn't define it for itself (single-operator mode) — a consumer repo adopting it is the real test. The one open gap is the deferred fixture criterion, tracked in #324.