feat(harness): a PR you open is watched, not abandoned (HARNESS-072-pr-stewardship, #963) - #986
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 20 minutes 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (19)
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 |
mlorentedev
added
the
skip-sdd
…oned Fills proposal.md before any implementation, per the Discipline Gate. The gate triggers here on two counts independently of LOC: the change edits a public contract (text injected verbatim into every agent's instructions) and spans the vault plus this repo. Carries one design requirement the issue could not have known. #963 proposes the window close at "an actionable comment, or N minutes after checks settle", and that condition was falsified the same day on PR #973: every check green, and CodeRabbit replied "Review limit reached — we couldn't start this review". A comment arrived; no review happened. A babysit rule keyed on comment arrival is satisfied by the notice that says nobody looked. So AC3 requires the region to state that such a notice leaves the PR unreviewed and that the agent must say so. Detecting quota exhaustion mechanically stays out of scope — that is #906. Refs #963
…bjections
The drafted `## PR Stewardship` text was sitting uncommitted in the vault working
tree, which four sessions share. The kubelab session spotted it from the outside
before I did — I had reported it as lost, from a stale view of the file. It was
there the whole time. Vault reverted to clean; the draft lives here until the
design is settled.
Two objections from that coordination, both accepted, neither yet applied:
1. "A PR touching specs/<id>/ gets an adversarial review by default" is too
broad. In kubelab every SDD artifact lives under specs/ and tasks.md is ticked
as work proceeds, so nearly every docs PR qualifies — including one whose
whole content was adding `text` to three fenced code blocks. It also relocates
a trigger the adversarial-review skill already owns ("BEFORE archiving",
"pairs with /spec archive lock") to a far higher frequency than it was built
for. Narrowing: keep the trigger at the archive gate, or fire on changes to
proposal.md acceptance criteria.
2. The timed watch overrides a user preference by a route the user did not
choose. kubelab carries a standing instruction from the same user not to poll
CI — they notify. An enforced region lands verbatim in every agent surface, so
it is the wrong instrument for anything a project may legitimately opt out of.
Revision: express it as an obligation to TRIAGE (checks and reviewer output
dispositioned before the work is called done, however the agent learns of
them), with the timed watch demoted to the default mechanism when no better
signal exists.
proposal.md predates both and its AC3/AC4 need revising before implementation.
Refs #963
Both objections turn out to be the same failure in different clothes: an `enforced` region reaches every project, so it must not encode anything a project may legitimately do differently. AC3 (was the timed watch, stated as the rule itself). The obligation is now the disposition — checks and reviewer output dispositioned before the change is called done, however the agent learns of them — and the timed watch is demoted to the default mechanism when a project offers no signal of its own. A project that says "do not poll, I notify" now satisfies the region instead of contradicting it. AC4 (was "the PR touches specs/<id>/"). The adversarial-review trigger stays at the archive gate, where the skill that owns it was designed to fire. The old test caught nearly every docs PR in a repo whose tasks.md is ticked as work proceeds, and would have relocated the trigger to a frequency it was never built for. While revising, AC2 was widened: its stated principle is "every surface definition-of-done reaches", but that region also ships through `doctrine.inject` to the agy and codex payloads, which the old wording missed — the same producer-updated / consumer-forgotten class this spec cites BUG-077 for. Headroom checked: the doctrine payload is 4176 of 12000 permitted chars. The old AC3/AC5/AC6 keep their content and renumber to AC5/AC6/AC7. The region draft is rewritten to match; it stays parked here rather than in the shared vault tree until the wording is confirmed. Refs #963
`--check` was named as the mitigation for "a region added to `enforced` but
missing from a target's `inject` list silently misses that surface". It cannot
be. `do_check` renders its `expected` side from the target's own inject list:
mapfile -t ids < <(target_inject "$file")
render_region "${ids[@]}" > "$expected"
region_content "$REPO_ROOT/$file" > "$actual"
An id absent from `inject` is therefore absent from `expected` AND from
`actual`, the diff is clean, and the surface prints `[check] OK`. It verifies
that injected text matches its record — a consistency check — and was being read
as a coverage check. This spec's own named risk sits in its blind spot.
AC2 now takes positive per-surface evidence: grep a sentence of the region in
each rendered surface, including the two $HOME doctrine payloads after
`--deploy`. That proves this region landed, and nothing more.
The class stays unguarded for the next region, which the repo's incident-to-
guard rule does not permit to pass in silence. Recorded as an open decision with
a recommendation rather than settled unilaterally: an orphan check is cheap and
misses the partial case; declared per-surface coverage with explicit opt-outs
makes silent omission inexpressible and turns `pr-sizing`'s doctrine-only status
from convention into schema. The latter is a manifest-schema change touching all
six regions, so it is not slipped into a PR whose subject is adding one region.
Found by a second session reviewing this worktree from the outside; verified
against the source here before acting on it.
Refs #963
…y not
`--check` verified that each injected region matches its record and was read as
proof that a region reached the surfaces it should. It never was. `do_check`
renders its expected side from the target's OWN inject list:
mapfile -t ids < <(target_inject "$file")
so an id missing from that list is missing from both sides of the diff and the
surface prints `[check] OK`. It is a consistency check; coverage was unguarded,
and the failure it lets through is silent by construction — a rule that simply
never reaches an agent, reported as clean.
Adds `check_coverage`: for every enforced id x every surface (each targets[]
file, plus the reserved key "doctrine" for the shared payload), the id is either
injected or carries an `opt_out` entry naming that surface with a reason. Silence
is not an exit, which is the rule the Definition of Done already applies to
people.
Run against the tree as it stood, it immediately failed on `pr-sizing`: injected
into the doctrine payload only, deliberately, for reasons argued at length in
#830 and recorded nowhere a machine could read. That asymmetry is correct and is
now declared rather than known — the reasons in the manifest are #830's own.
Three bats cases. The first is the one that matters: with the region absent from
the second surface's inject list, the region diff reports `OK -> TARGET2.md` and
coverage reports GAP on the same run. An orphan check — "is this id used
anywhere?" — would also pass it, because the id is in use on the first surface,
and the partial case is the likelier mistake.
Refs #963
Opening a PR is treated as the end of a change and it is not: checks report afterwards, reviewers report after that, and nothing obliged an agent to still be there. On #959 every check went green and the reviewer then posted four Major findings, all of which shipped in a follow-up PR instead of the one that introduced them. Adds the `pr-stewardship` enforced region, sourced from the vault section beside `definition-of-done`, whose Review item it elaborates. Three rules: What binds is the disposition, not the waiting. Checks and reviewer output are dispositioned before the change is called done, however the agent learns they arrived. A project with its own signal — the human notifies, a hook fires — has already met it, and its instruction wins; the timed watch is only the default when nothing better exists. The first draft made the watch itself the rule, which would have overridden a standing user preference in another project by a route that user never chose. A comment is not a review. On #973 the checks went green and the reviewer posted "review limit reached — we couldn't start this review": a comment arrived and nobody looked. A notice that no review ran leaves the PR unreviewed. Proceeding is allowed; proceeding silently is not. A change that closes a spec gets an independent adversarial review before it archives — at the archive gate, the trigger `adversarial-review` already owns, and nothing wider. "Touches specs/<id>/" was the first draft and it caught nearly every docs PR in a repo whose tasks.md is ticked as work proceeds. Injected into both targets and the doctrine payload; the coverage assertion added alongside is what proves all three, rather than a hand count. Refs #963
Picked up by `--refresh` while working on an unrelated region, and kept out of that change's diff. Content-only: the dispatch skill grows a mandatory reconciliation block in its worker prompt, three systematic-debugging references and the TDD anti-patterns reference each gain a line, and the curator persona record follows its definition. Worth noting how these stayed invisible: `--check` is offline and diffs the committed records against the injected regions, never against the vault, so a record trailing its source reports clean until someone runs `--refresh`. It is the same shape as the coverage gap fixed earlier on this branch — a check answering a narrower question than the one being asked of it. Not fixed here; the vault is not reachable from CI by design (ADR-013).
…duced features.json maps all eight acceptance criteria to shell commands; all eight were run this session and exit 0. States stay `pending` with empty evidence — only the harness may write `passing`, and a claim about a run is not a run. The lesson is the one worth keeping: a check earns its citation by the question it actually asks, not by its name. `--check` was named in this spec's own Risks section as the mitigation for a risk it is structurally blind to, and the tell was that the mitigation could be stated but never demonstrated red. Refs #963
It existed because the design was unsettled and the shared vault tree was the wrong place to leave half-written text. Both conditions are gone: the section lives in pattern-change-lifecycle.md and its record in harness/enforced/. A third copy under specs/ has no source of truth to track and would rot against the two that do. Refs #963
It was staged by a `git add specs/` while resolving an unrelated conflict. The file is the artifact of the launch failure filed as #989 — empty, carrying no verdict and no error — so committing it records nothing except that something went wrong somewhere else. Ignored going forward: the transcript is per-run local evidence, regenerated by each review, and `review.md` is the deliverable that belongs in git. Refs #963
mlorentedev
removed
the
skip-sdd
mlorentedev
force-pushed
the
feat/harness-072-pr-stewardship
branch
from
August 15, 2026 07:56
1774edd to
913890c
Compare
This was referenced Aug 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #963 — deliberately not a closing keyword, changed at Manu's request while the author session was mid-work.
The SDD lifecycle ends by archiving the spec in the PR that closes its issue, and archiving needs a passing
review.mdfrom the reviewer pool. That review still cannot run.dotf spec reviewlaunches throughdotf secrets run --with no scope, and unscoped resolution is broken twice over: thedockerhubnaming drift (#985) is now fixed — the vault item was renamed and the daemon synced — butbw servestill returns a non-JSON envelope on batch reads (#988), and it fails on a different item each attempt (three consecutive runs failed ondockerhub/username, thengithub-bitacora-pat/api-tokentwice).So the archive is blocked on defects outside this change, not deferred by choice. #963 closes when the review can run and the spec archives.
Opening a PR is treated as the end of a change and it is not: checks report afterwards, reviewers report after that, and nothing obliged an agent to still be there. On #959 every check went green and the reviewer then posted four Major findings — a
| teediscarding the reviewer's exit status, ansh -cfallback that could not run on the Windows it was documented for, a pool whose deletion silently disabled a security gate, and an acceptance criterion ticked without its evidence. All four shipped in a follow-up PR instead of the one that introduced them.What lands
A new
pr-stewardshipenforced region, sourced from the vault section besidedefinition-of-done, whose Review item it elaborates. Three rules:adversarial-reviewalready owns, and nothing wider.pr-review-triageretriggered on whichever of checks and reviewers lands later. Its body already covered the quota case and isgh-only; the description still fired at "checks finished", which this change is the argument against.Two objections, both from a parallel session, both applied
Rules 1 and 3 read as they do because the first draft was wrong in the same way twice — a region reaching every project must not encode anything a project may legitimately do differently.
specs/<id>/". In a repo whosetasks.mdis ticked as work proceeds that catches nearly every docs PR, including one whose entire content was addingtextto three fenced code blocks.The guard, and why it is in this PR
The spec named
compile-harness.sh --checkas the mitigation for its own headline risk — a region added toenforcedbut forgotten in a target'sinjectlist. It could never have caught it.do_checkrenders its expected side from the target's own inject list, so a missing id is missing from both sides of the diff and the surface prints[check] OK. A consistency check was being cited as a coverage check.check_coveragecloses it: every enforced id × every surface is injected, or carries anopt_outnaming that surface with a reason — an empty reason is still a gap. Run against the tree as it stood it failed immediately onpr-sizing, doctrine-only by a decision argued in #830 and recorded nowhere machine-readable; that asymmetry is now declared instead of remembered.Taken here rather than ticketed because the repo's incident-to-guard rule requires the assertion in the same PR, and the fix-or-ticket escape does not apply to a spec still in
draft.Evidence
bats tests/*.bats→ 1238/1238, 0 failures.tests/compile-harness.bats→ 47/47.shellcheck scripts/compile-harness.sh+bash -n→ clean../scripts/compile-harness.sh --check→ exit 0.features.jsonverification commands run this session → 8/8 exit 0.AGENTS.md,ai/claude/CLAUDE.md, and after--deploy~/.claude/CLAUDE.md,~/.gemini/GEMINI.md,~/.codex/AGENTS.md. Caps hold — GEMINI 6503/12000, codex 6503/32768.The second is the case that sets the guard's shape: an orphan check would have passed it, because the id was in use on another surface.
Sizing
592 insertions, of which 47 are production code (
scripts/compile-harness.sh); the rest is tests (60), the spec folder, the lesson, and generated regions. Six unrelated records that--refreshpulled from the vault are isolated in their own commit and named as such.Not in this PR
pr-review-triagestill forbids acting without human confirmation.