From bf727ce72443e73d4787b1d32339f3af7760621a Mon Sep 17 00:00:00 2001 From: Manu Date: Fri, 14 Aug 2026 19:06:25 -0600 Subject: [PATCH 01/12] =?UTF-8?q?docs(spec):=20propose=20HARNESS-072=20?= =?UTF-8?q?=E2=80=94=20a=20PR=20you=20open=20is=20watched,=20not=20abandon?= =?UTF-8?q?ed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- specs/HARNESS-072-pr-stewardship/proposal.md | 119 ++++++++++++++++++ specs/HARNESS-072-pr-stewardship/tasks.md | 60 +++++++++ .../verification.md | 42 +++++++ 3 files changed, 221 insertions(+) create mode 100644 specs/HARNESS-072-pr-stewardship/proposal.md create mode 100644 specs/HARNESS-072-pr-stewardship/tasks.md create mode 100644 specs/HARNESS-072-pr-stewardship/verification.md diff --git a/specs/HARNESS-072-pr-stewardship/proposal.md b/specs/HARNESS-072-pr-stewardship/proposal.md new file mode 100644 index 00000000..27be95d9 --- /dev/null +++ b/specs/HARNESS-072-pr-stewardship/proposal.md @@ -0,0 +1,119 @@ +--- +id: "HARNESS-072-pr-stewardship" +type: spec +status: draft # draft | implementing | verifying | archived +created: "2026-08-15" +issue: "mlorentedev/dotfiles#963" # repo#NNN — GitHub issue / Project item that tracks this spec +tags: [spec, proposal, harness, review] +template_version: "1.0" +--- + +# HARNESS-072-pr-stewardship + +## Why + +Opening a PR is treated as the end of a change, and it is not: checks report +afterwards, and reviewer bots report after *that*. Nothing currently obliges an +agent to still be there when they do. The worked example is PR #959 — all checks +went green, a session would have been justified in walking away, and CodeRabbit +then posted **10 comments including four Majors**: a `| tee` pipeline discarding +the reviewer's exit status, an `sh -c` fallback that cannot 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. Every one of those shipped +into a follow-up PR instead of the one that introduced them. + +The procedure to handle this already exists (`pr-review-triage`) and is correct. +What it lacks is a *binding trigger* and a *correct end condition*: it is a +skill, so an agent that does not think to load it simply does not, and its +trigger fires at "checks finished", which is demonstrably too early. + +## What + +Two changes, one obligation and one procedure. + +**1. A new `enforced` harness region.** Sourced from a section in +`pattern-change-lifecycle.md` beside `definition-of-done`, whose Review item this +elaborates, and injected verbatim into every agent surface that region reaches. +It states two rules: + +- *A PR you open is watched, not abandoned.* The window closes at the first of: + an actionable reviewer comment (attend to it), or N minutes after checks + settle. Pushing a fix reopens it, because the reviewer re-reviews. +- *A PR that touches a `specs//` folder gets an adversarial review by + default.* This names an obligation that already binds mechanically — spec-gate + refuses to merge a PR closing a spec's issue without archiving it, + `dotf spec archive` refuses without a passing `review.md`, and since #958 the + pool refuses one signed by the wrong model. Stating it converts three + mechanical refusals into one intention an agent can act on *before* being + refused by any of them. + +**2. `pr-review-triage` amended** in its vault source: the end condition covers +the reviewer bot rather than only CI, and the wait is expressed as a *contract* +with `gh` commands — agent-neutral — rather than a polling implementation, since +Claude has background tasks and pi and opencode do not. + +After this, an agent that opens a PR and walks away at CI-green is violating a +rule injected into its own instructions, rather than merely failing to load an +optional skill. + +## Out of scope + +- Making the babysit a scheduled or background mechanism. The contract is what + binds; how a given harness waits is its own business. +- Auto-applying reviewer comments. `pr-review-triage` already forbids acting + without human confirmation, and that stays. +- Detecting or working around a reviewer's quota exhaustion. The wording below + must make an agent *report* that state honestly, but building a gate that + recognises it is #906. + +## Risks / open questions + +- **The obvious end condition is already known to be wrong, and this is the main + design risk.** "An actionable comment arrived" is satisfied by a comment that + is not a review at all. Observed on PR #973 the same day this spec was written: + every check green, and CodeRabbit posted *"Review limit reached — you've + reached your PR review limit, so we couldn't start this review."* That is a + comment, it is not review activity, and a naive babysit rule treats the PR as + reviewed. The region's wording must make that non-lawyerable: **a reviewer-bot + notice that no review ran is the PR being unreviewed, and saying so out loud is + part of closing the window.** This is the same shape as BUG-077 and the + `pattern-verification-fails-toward-unproven` family — absence of review + presenting as clean review. +- **A rule injected everywhere is expensive to get wrong.** It lands verbatim in + every agent's instructions, so it must be terse and free of Claude-specific + vocabulary. The mitigation is the same one `definition-of-done` uses: bind + existing obligations, do not restate them. +- **N is a guess.** 10 minutes fits CodeRabbit's observed latency on this repo + and is short enough not to park an agent. It is a number in prose, not a + constant, so it costs nothing to revise. +- **A region added to `enforced` but missing from a target's `inject` list + silently misses that surface.** Exactly the producer-updated / + consumer-forgotten class that BUG-077 (#969) was. Mitigated by AC5 below: + `compile-harness.sh --check` is the test, not a hand count. + +## Acceptance criteria + +- [ ] **AC1** An `enforced` region exists, sourced from a vault pattern section, + and is listed in `harness/manifest.json` with an `id` and a `source`. +- [ ] **AC2** The region is injected into every surface `definition-of-done` + reaches — both `targets` entries — verified by `compile-harness.sh --check` + passing, not by counting files by hand. +- [ ] **AC3** The region's text states that a reviewer-bot notice reporting that + no review ran (rate limit, quota) leaves the PR **unreviewed**, and that + the agent must say so rather than close the window on it. +- [ ] **AC4** "Important PR" resolves to a test an agent applies without + judgement: the PR touches a `specs//` folder. +- [ ] **AC5** `pr-review-triage`'s trigger and end condition cover the reviewer + bot, not only CI, and the wait is expressed with `gh` commands runnable by + an agent with no Claude-specific primitives. +- [ ] **AC6** The vault source and the committed harness records agree — + `compile-harness.sh --check` reports no drift after `--refresh`. + +## References + +- Bitácora board: mlorentedev/dotfiles#963 +- `harness/manifest.json` — `enforced` + `targets`; `definition-of-done` is the precedent +- `harness/skills/pr-review-triage/SKILL.md` — the procedure this makes binding +- `00_meta/patterns/pattern-change-lifecycle.md` — Definition of Done §4, the parent rule +- `00_meta/patterns/pattern-verification-fails-toward-unproven.md` — why the quota-notice case is a design requirement and not an edge case +- PR #959 (CI green, then 4 Majors), PR #973 (CI green, no review at all), #906 (the quota gap) diff --git a/specs/HARNESS-072-pr-stewardship/tasks.md b/specs/HARNESS-072-pr-stewardship/tasks.md new file mode 100644 index 00000000..7d8ec477 --- /dev/null +++ b/specs/HARNESS-072-pr-stewardship/tasks.md @@ -0,0 +1,60 @@ +--- +tags: [spec, tasks, templates] +created: "2026-08-15" +--- + +# Tasks - HARNESS-072-pr-stewardship + +> TDD order. One task = one focused commit. Tick as you go. Reorder freely while spec is in `draft` state; freeze once you start `implementing`. +> +> **Inline markers** (optional, additive — borrowed from `github/spec-kit`, adapt-not-adopt per #141): +> - `[P]` — this task has **no dependency on another unchecked task**, so it is safe to run in parallel (fan out to a `Workflow`, or just batch). TDD chains (test → implement → refactor of the *same* behavior) are sequential and must NOT carry `[P]`; independent behaviors can. +> - `[AC]` — this task helps satisfy **acceptance criterion #``** from `proposal.md`. Lets `/spec check` map coverage deterministically; omit it and the check falls back to semantic judgment. + +## Setup + +- [ ] Branch created from main: `feat/HARNESS-072-pr-stewardship` +- [ ] `proposal.md` is complete and acceptance criteria are testable +- [ ] No open questions left in `proposal.md` "Risks / open questions" + +## Implementation + +> Replace these with the actual steps for this feature. Keep them small (one commit each) and in TDD order. +> The `[P]` / `[AC]` markers are optional — see the legend above. Behaviors 1 and 2 below are independent, so their *first* test task carries `[P]`. + +- [ ] [P] [AC1] Write failing test for +- [ ] [AC1] Implement to make it pass +- [ ] Refactor for clarity (extract, rename, dedupe) +- [ ] [P] [AC2] Write failing test for +- [ ] [AC2] Implement to make it pass +- [ ] ... + +## Closing + +- [ ] Every acceptance criterion from `proposal.md` is covered by at least one test +- [ ] Every acceptance criterion has a matching entry in `features.json` (see below) with a non-vacuous verification command +- [ ] Type checks pass +- [ ] Lint passes +- [ ] No unrelated changes in the diff (no scope creep) +- [ ] `verification.md` filled in +- [ ] PR opened referencing this spec folder + +## Machine-readable features + +This spec emits a sibling `features.json` (alongside this file) following [[pattern-feature-list-as-primitive]]. The JSON is the harness-facing contract: each acceptance criterion maps to ≥1 feature with `id`, `behavior`, `verification` (executable command), `state` (lifecycle), and `evidence` (harness-captured output). + +**Pass-state gating:** the agent CANNOT write `"state": "passing"` — only the harness, after running `verification` and capturing exit code 0, may set that terminal state. Reviewers must reject PRs where features.json contains `passing` entries with empty `evidence`. + +Minimal `features.json` skeleton (drop into `/specs/HARNESS-072-pr-stewardship/features.json`): + +```json +[ + { + "id": "HARNESS-072-pr-stewardship-f1", + "behavior": "", + "verification": "", + "state": "pending", + "evidence": "" + } +] +``` diff --git a/specs/HARNESS-072-pr-stewardship/verification.md b/specs/HARNESS-072-pr-stewardship/verification.md new file mode 100644 index 00000000..dbb7f38a --- /dev/null +++ b/specs/HARNESS-072-pr-stewardship/verification.md @@ -0,0 +1,42 @@ +--- +tags: [spec, verification, templates] +created: "2026-08-15" +--- + +# Verification - HARNESS-072-pr-stewardship + +## Evidence + +Map every acceptance criterion from `proposal.md` to concrete proof (commit hash, test name, or observed behavior). + +- [ ] Criterion 1 -> commit `` / test `` +- [ ] Criterion 2 -> commit `` / test `` +- [ ] Criterion 3 -> commit `` / test `` + +## Test status + +- Test suite: ` -> ` +- Manual smoke test: what was exercised, what was observed +- No regressions in existing test suite: yes / no (if no, document) + +## Decisions made during implementation + +Brief log of non-obvious trade-offs or course corrections taken during the work. Routine choices belong in commit messages, not here. + +- +- + +## Promotion candidates + +Before archiving, flag what (if anything) should be promoted to the vault. If all three are "no", archive in repo is the only persistence. + +- [ ] Lesson for the repo's `docs/lessons.md`? +- [ ] ADR-worthy decision for the repo's `docs/adr/adr-XXX.md`? +- [ ] New pattern candidate for `00_meta/patterns/`? Only if this recurs in >1 project. + +## Archive checklist + +- [ ] `proposal.md` frontmatter set to `status: archived` +- [ ] Folder moved: `specs/HARNESS-072-pr-stewardship/` -> `specs/archive/HARNESS-072-pr-stewardship/` +- [ ] Bitácora board ticket for this spec moved to Done / closed with PR link (ADR-018) +- [ ] Promotions above executed (if any) From 8ad1f1de9eb1194604e0d1cc8d03160ceee5e712 Mon Sep 17 00:00:00 2001 From: Manu Date: Fri, 14 Aug 2026 20:16:22 -0600 Subject: [PATCH 02/12] docs(spec): park HARNESS-072's region draft and record two blocking objections MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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// 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 --- specs/HARNESS-072-pr-stewardship/draft-region.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 specs/HARNESS-072-pr-stewardship/draft-region.md diff --git a/specs/HARNESS-072-pr-stewardship/draft-region.md b/specs/HARNESS-072-pr-stewardship/draft-region.md new file mode 100644 index 00000000..9a019aa4 --- /dev/null +++ b/specs/HARNESS-072-pr-stewardship/draft-region.md @@ -0,0 +1,13 @@ + + +## PR Stewardship + +> Injected verbatim into every agent's instructions (harness `enforced` id `pr-stewardship`). It elaborates Definition of Done §4 — "an open PR is not finished work" — into the two obligations that item leaves implicit: stay, and get an independent read. + +**A PR you open is watched, not abandoned.** Opening it is not the end of the change: checks report afterwards, and reviewers report after *that*. The window closes at the first of an actionable reviewer comment, or ten minutes after the checks settle. Pushing a fix reopens it, because the reviewer re-reviews. Leave with the window open and you have handed the next session a change nobody read. + +**CI green is not the end of the window, and a comment is not a review.** Both halves have been observed failing here. Checks went green on one PR and the reviewer then posted four Major findings; on another, checks went green and the reviewer posted *"review limit reached — we couldn't start this review"*. That second one is a comment arriving, and it means **nobody looked**. A notice that no review ran leaves the PR unreviewed, and closing the window on it requires saying so out loud — "merged unreviewed, reviewer quota exhausted" is a disclosure; silence is a claim of review that never happened. + +**A PR that touches a `specs//` folder gets an independent adversarial review by default.** That is the whole test — no judgement call about whether a change is "important" enough. It names an obligation that already binds mechanically, so the only question is whether you meet it deliberately or discover it as a refusal: the spec gate declines to merge a PR closing a spec's issue without archiving it, `spec archive` declines without a passing review, and the reviewer pool declines one signed by the wrong model. The reviewer must not be the implementer; that is what makes it independent, and it is the entire value. From f029a06a92c2cf653188bdbc3226c173756efb89 Mon Sep 17 00:00:00 2001 From: Manu Date: Sat, 15 Aug 2026 00:52:41 -0600 Subject: [PATCH 03/12] docs(spec): revise HARNESS-072 for the two accepted objections MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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//"). 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 --- .../draft-region.md | 14 ++-- specs/HARNESS-072-pr-stewardship/proposal.md | 79 +++++++++++++------ 2 files changed, 65 insertions(+), 28 deletions(-) diff --git a/specs/HARNESS-072-pr-stewardship/draft-region.md b/specs/HARNESS-072-pr-stewardship/draft-region.md index 9a019aa4..4ea595cc 100644 --- a/specs/HARNESS-072-pr-stewardship/draft-region.md +++ b/specs/HARNESS-072-pr-stewardship/draft-region.md @@ -1,13 +1,15 @@ + Held here instead of half-written into the shared vault tree, because the + vault working tree is shared by several sessions and an uncommitted draft + there was nearly lost once. Revised for the two kubelab objections — see + proposal.md "Risks", AC3 and AC4. --> ## PR Stewardship -> Injected verbatim into every agent's instructions (harness `enforced` id `pr-stewardship`). It elaborates Definition of Done §4 — "an open PR is not finished work" — into the two obligations that item leaves implicit: stay, and get an independent read. +> Injected verbatim into every agent's instructions (harness `enforced` id `pr-stewardship`). It elaborates Definition of Done §4 — "an open PR is not finished work" — into what that item leaves implicit: what you still owe a PR after you push it, and what does not count as having been reviewed. -**A PR you open is watched, not abandoned.** Opening it is not the end of the change: checks report afterwards, and reviewers report after *that*. The window closes at the first of an actionable reviewer comment, or ten minutes after the checks settle. Pushing a fix reopens it, because the reviewer re-reviews. Leave with the window open and you have handed the next session a change nobody read. +**What binds is the disposition, not the waiting.** Before the change is called done, the PR's checks and its reviewer output are dispositioned — each one applied, ticketed, or declined with a reason. *How* you learn they arrived is not prescribed: a project that already tells you when to look back — the human notifies, a hook fires — has met this, and its instruction wins. Absent such a signal the default mechanism is to stay: the window closes at the first of an actionable reviewer comment or ten minutes after the checks settle, and pushing a fix reopens it, because the reviewer re-reviews. Leaving with nothing dispositioned hands the next session a change nobody read. -**CI green is not the end of the window, and a comment is not a review.** Both halves have been observed failing here. Checks went green on one PR and the reviewer then posted four Major findings; on another, checks went green and the reviewer posted *"review limit reached — we couldn't start this review"*. That second one is a comment arriving, and it means **nobody looked**. A notice that no review ran leaves the PR unreviewed, and closing the window on it requires saying so out loud — "merged unreviewed, reviewer quota exhausted" is a disclosure; silence is a claim of review that never happened. +**A comment is not a review, and green checks are not the end of one.** Both halves have been observed failing here. On one PR every check went green and the reviewer then posted four Major findings. On another, 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.** Tell the two apart by content, never by author — a review names files, lines, or claims; a notice talks about the review itself. Proceeding on an unreviewed PR is allowed; proceeding silently is not. "Merged unreviewed, reviewer quota exhausted" is a disclosure; saying nothing is a claim of review that never happened. -**A PR that touches a `specs//` folder gets an independent adversarial review by default.** That is the whole test — no judgement call about whether a change is "important" enough. It names an obligation that already binds mechanically, so the only question is whether you meet it deliberately or discover it as a refusal: the spec gate declines to merge a PR closing a spec's issue without archiving it, `spec archive` declines without a passing review, and the reviewer pool declines one signed by the wrong model. The reviewer must not be the implementer; that is what makes it independent, and it is the entire value. +**A change that closes a spec gets an independent adversarial review before it archives.** The trigger is the archive gate and nothing wider — not every PR that touches a spec folder. It names an obligation that already binds mechanically, so the only question is whether you meet it deliberately or discover it as a refusal: the spec gate declines to merge a PR closing a spec's issue without archiving it, `spec archive` declines without a passing review, and the reviewer pool declines one signed by the wrong model. The reviewer must not be the implementer; that independence is the entire value. diff --git a/specs/HARNESS-072-pr-stewardship/proposal.md b/specs/HARNESS-072-pr-stewardship/proposal.md index 27be95d9..c53ce224 100644 --- a/specs/HARNESS-072-pr-stewardship/proposal.md +++ b/specs/HARNESS-072-pr-stewardship/proposal.md @@ -34,18 +34,26 @@ Two changes, one obligation and one procedure. **1. A new `enforced` harness region.** Sourced from a section in `pattern-change-lifecycle.md` beside `definition-of-done`, whose Review item this elaborates, and injected verbatim into every agent surface that region reaches. -It states two rules: - -- *A PR you open is watched, not abandoned.* The window closes at the first of: - an actionable reviewer comment (attend to it), or N minutes after checks - settle. Pushing a fix reopens it, because the reviewer re-reviews. -- *A PR that touches a `specs//` folder gets an adversarial review by - default.* This names an obligation that already binds mechanically — spec-gate - refuses to merge a PR closing a spec's issue without archiving it, - `dotf spec archive` refuses without a passing `review.md`, and since #958 the - pool refuses one signed by the wrong model. Stating it converts three - mechanical refusals into one intention an agent can act on *before* being - refused by any of them. +It states three rules: + +- *What binds is the disposition, not the waiting.* Checks and reviewer output + are dispositioned before the change is called done — applied, ticketed, or + declined with a reason. **How** an agent learns they arrived is not + prescribed: a project with its own signal (the human notifies, a hook fires) + has already met the obligation, and its instruction wins. Absent such a + signal, the default mechanism is to stay — the window closes at the first of + an actionable reviewer comment or N minutes after checks settle, and pushing a + fix reopens it, because the reviewer re-reviews. +- *A comment is not a review.* A reviewer notice reporting that no review ran + leaves the PR unreviewed. Proceeding anyway is allowed; proceeding silently is + not. +- *A change that closes a spec gets an independent adversarial review before it + archives.* The trigger is the archive gate and nothing broader. It names an + obligation that already binds mechanically — spec-gate refuses to merge a PR + closing a spec's issue without archiving it, `dotf spec archive` refuses + without a passing `review.md`, and since #958 the pool refuses one signed by + the wrong model. Stating it converts three mechanical refusals into one + intention an agent can act on *before* being refused by any of them. **2. `pr-review-triage` amended** in its vault source: the end condition covers the reviewer bot rather than only CI, and the wait is expressed as a *contract* @@ -82,10 +90,25 @@ optional skill. - **A rule injected everywhere is expensive to get wrong.** It lands verbatim in every agent's instructions, so it must be terse and free of Claude-specific vocabulary. The mitigation is the same one `definition-of-done` uses: bind - existing obligations, do not restate them. + existing obligations, do not restate them. Two objections raised from the + kubelab side, both accepted, are the reason AC3 and AC4 read as they do — and + both are the *same* failure in different clothes: a region that reaches every + project must not encode anything a project may legitimately do differently. + - *A mechanism is not an obligation.* The first draft made the timed watch + itself the rule. kubelab carries a standing instruction from the same user + not to poll CI — they notify — so the region would have overridden a user + preference by a route the user never chose. The obligation is the + disposition; the watch is the fallback when nothing better exists (AC3). + - *A trigger belongs where it was designed.* "Touches `specs//`" catches + nearly every docs PR in a repo where `tasks.md` is ticked as work proceeds — + including one whose whole content was adding `text` to three fenced code + blocks — and relocates a trigger `adversarial-review` already owns at the + archive gate to a far higher frequency than it was built for (AC4). - **N is a guess.** 10 minutes fits CodeRabbit's observed latency on this repo and is short enough not to park an agent. It is a number in prose, not a - constant, so it costs nothing to revise. + constant, so it costs nothing to revise. Demoting it to a default mechanism + lowers the cost of the guess further: a project it does not suit overrides it + without touching the region. - **A region added to `enforced` but missing from a target's `inject` list silently misses that surface.** Exactly the producer-updated / consumer-forgotten class that BUG-077 (#969) was. Mitigated by AC5 below: @@ -96,17 +119,29 @@ optional skill. - [ ] **AC1** An `enforced` region exists, sourced from a vault pattern section, and is listed in `harness/manifest.json` with an `id` and a `source`. - [ ] **AC2** The region is injected into every surface `definition-of-done` - reaches — both `targets` entries — verified by `compile-harness.sh --check` - passing, not by counting files by hand. -- [ ] **AC3** The region's text states that a reviewer-bot notice reporting that + reaches — both `targets` entries **and `doctrine.inject`**, which carries + that region to the agy and codex payloads — verified by + `compile-harness.sh --check` passing, not by counting files by hand. If a + surface is deliberately excluded, the exclusion is recorded with its + reason; `pr-sizing` is the precedent that selective injection is legitimate. + The `char_cap` of each doctrine target still holds after the addition. +- [ ] **AC3** The region binds a **disposition**, not a mechanism: its text + obliges checks and reviewer output to be dispositioned before the change is + called done, *however the agent learns of them*, and names the timed watch + only as the default when a project offers no signal of its own. A project + instruction covering the same ground overrides the mechanism without + contradicting the region. +- [ ] **AC4** The adversarial-review obligation fires at the **archive gate** — + the trigger `adversarial-review` already owns — and the region introduces + no broader one. Concretely: the text must not make "the PR touches + `specs//`" a trigger. +- [ ] **AC5** The region's text states that a reviewer-bot notice reporting that no review ran (rate limit, quota) leaves the PR **unreviewed**, and that - the agent must say so rather than close the window on it. -- [ ] **AC4** "Important PR" resolves to a test an agent applies without - judgement: the PR touches a `specs//` folder. -- [ ] **AC5** `pr-review-triage`'s trigger and end condition cover the reviewer + proceeding requires saying so out loud. +- [ ] **AC6** `pr-review-triage`'s trigger and end condition cover the reviewer bot, not only CI, and the wait is expressed with `gh` commands runnable by an agent with no Claude-specific primitives. -- [ ] **AC6** The vault source and the committed harness records agree — +- [ ] **AC7** The vault source and the committed harness records agree — `compile-harness.sh --check` reports no drift after `--refresh`. ## References From 7a812e6e65217c34944d3dd89262eeb6a0c32da8 Mon Sep 17 00:00:00 2001 From: Manu Date: Sat, 15 Aug 2026 00:55:13 -0600 Subject: [PATCH 04/12] docs(spec): AC2 cited a check that cannot fail the way it claimed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--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 --- specs/HARNESS-072-pr-stewardship/proposal.md | 54 +++++++++++++++++--- 1 file changed, 47 insertions(+), 7 deletions(-) diff --git a/specs/HARNESS-072-pr-stewardship/proposal.md b/specs/HARNESS-072-pr-stewardship/proposal.md index c53ce224..829322fd 100644 --- a/specs/HARNESS-072-pr-stewardship/proposal.md +++ b/specs/HARNESS-072-pr-stewardship/proposal.md @@ -111,8 +111,42 @@ optional skill. without touching the region. - **A region added to `enforced` but missing from a target's `inject` list silently misses that surface.** Exactly the producer-updated / - consumer-forgotten class that BUG-077 (#969) was. Mitigated by AC5 below: - `compile-harness.sh --check` is the test, not a hand count. + consumer-forgotten class that BUG-077 (#969) was. The first draft of this + section named `compile-harness.sh --check` as the mitigation. **It is not one** + — `do_check` renders its `expected` from the target's own `inject` list + (`mapfile -t ids < <(target_inject "$file")`), so an id absent from that list + is absent from both sides of the diff and the surface reports `OK`. The check + is a consistency check, not a coverage check, and this spec's own risk is in + its blind spot. AC2 therefore takes positive per-surface evidence instead. + That leaves the *class* unguarded for the next region, which is a repo rule + violation (`feedback_incident_to_guard`: a bug class encountered emits a CI + assertion in the same PR) — resolved as a decision, not silence: see + "Coverage guard" below. + +### Coverage guard — open decision + +Positive per-surface evidence (AC2) proves *this* region reached every surface. +It does nothing for the next one, and the repo's own rule is that a bug class +encountered emits an assertion in the same PR. The class here is precise: **the +harness can verify that an injected region matches its record, and cannot verify +that a region reached the surfaces it should.** Two ways to close it: + +- **(a) Orphan check.** `--check` fails when an `enforced[].id` appears in no + `targets[].inject` and not in `doctrine.inject`. ~10 lines, no schema change. + Catches a region wired nowhere; misses a region wired to one surface of two — + which is the likelier mistake and the one this spec risks. +- **(b) Declared coverage.** Every `enforced` id must appear in every surface's + inject list *unless* the manifest records an explicit opt-out with a reason, + which `--check` then reports. Silent omission stops being expressible: you + inject, or you write down why not. `pr-sizing` — deliberately doctrine-only — + becomes a declared exclusion instead of a convention someone has to know. + +**(b) is the right shape**: it converts a coverage question into a schema the +engine can answer, and it is the only one that catches the partial case. Its +cost is a manifest-schema change touching all six regions plus a bats case, so +it is a separate change from "add a region" and is **not** slipped into this PR. +Pending the user's call: ticket it, or take it here as an agreed scope +extension. ## Acceptance criteria @@ -120,11 +154,17 @@ optional skill. and is listed in `harness/manifest.json` with an `id` and a `source`. - [ ] **AC2** The region is injected into every surface `definition-of-done` reaches — both `targets` entries **and `doctrine.inject`**, which carries - that region to the agy and codex payloads — verified by - `compile-harness.sh --check` passing, not by counting files by hand. If a - surface is deliberately excluded, the exclusion is recorded with its - reason; `pr-sizing` is the precedent that selective injection is legitimate. - The `char_cap` of each doctrine target still holds after the addition. + that region to the agy and codex payloads. Evidence is a **positive + per-surface grep** for a sentence of the region in each rendered surface, + including the two `$HOME` doctrine payloads after `--deploy`. It is **not** + `compile-harness.sh --check`: that renders its `expected` from the target's + own `inject` list, so a region missing from that list is missing from both + sides of the diff and reports `OK`. The check proves the injected text + matches its record; it is structurally blind to a surface being skipped — + which is the exact risk named below. If a surface is deliberately excluded, + the exclusion is recorded with its reason; `pr-sizing` is the precedent + that selective injection is legitimate. The `char_cap` of each doctrine + target still holds after the addition. - [ ] **AC3** The region binds a **disposition**, not a mechanism: its text obliges checks and reviewer output to be dispositioned before the change is called done, *however the agent learns of them*, and names the timed watch From b0b7161cbf14c6bb0a07cffc8543c1024dc191a6 Mon Sep 17 00:00:00 2001 From: Manu Date: Sat, 15 Aug 2026 01:06:20 -0600 Subject: [PATCH 05/12] feat(harness): assert enforced regions reach every surface, or say why not MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--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 --- harness/manifest.json | 9 +++++- scripts/compile-harness.sh | 47 +++++++++++++++++++++++++++++ tests/compile-harness.bats | 60 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 115 insertions(+), 1 deletion(-) diff --git a/harness/manifest.json b/harness/manifest.json index 8ef0b43a..7372ceb7 100644 --- a/harness/manifest.json +++ b/harness/manifest.json @@ -7,7 +7,14 @@ { "id": "no-phase-references", "source": "pattern-git-workflow.md#7-message-content-policy" }, { "id": "no-auto-merge", "source": "pattern-git-workflow.md#9-merge-policy" }, { "id": "definition-of-done", "source": "pattern-change-lifecycle.md#definition-of-done" }, - { "id": "pr-sizing", "source": "pattern-git-workflow.md#10-pr-sizing-policy" } + { + "id": "pr-sizing", + "source": "pattern-git-workflow.md#10-pr-sizing-policy", + "opt_out": { + "AGENTS.md": "the targets region counters harness defaults only; a precedence rule between two of our own rules overrides none, and #824 already states this policy in this file's hand-written prose (#830)", + "ai/claude/CLAUDE.md": "same as AGENTS.md — the compact doctrine payload substitutes for the constitution agy and codex never receive, and this file's reader already gets the policy from AGENTS.md (#830)" + } + } ], "targets": [ { diff --git a/scripts/compile-harness.sh b/scripts/compile-harness.sh index d69f701e..fadbea5b 100755 --- a/scripts/compile-harness.sh +++ b/scripts/compile-harness.sh @@ -832,9 +832,56 @@ deploy_agent_presence() { done < <(jq -r '.agents.presence[] | "\(.agent)\t\(.file)"' "$MANIFEST") } +# --- coverage: every enforced region reaches every surface, or says why not --- +# +# Why this is not already covered by the region diff below: that diff renders its +# expected side FROM the target's own `inject` list (target_inject), so an id +# missing from the list is missing from BOTH sides and the target reports OK. It +# verifies that injected text matches its record — a consistency check — and is +# structurally blind to a surface being skipped entirely. That blindness is the +# bug this function exists for (HARNESS-072); the risk had been filed against +# the diff, which could never have caught it. +# +# Invariant: for every enforced id x every surface, the id is either injected or +# carries an `opt_out` entry naming that surface with a reason. Silence is not an +# exit — the same rule the Definition of Done applies to people. +# +# A surface is a `targets[].file`, plus the reserved key "doctrine" for the one +# shared inject list feeding every doctrine payload. +check_coverage() { + local id surfaces surface injected reason gap=0 + mapfile -t surfaces < <( + jq -r '.targets[].file' "$MANIFEST" + jq -e '.doctrine' "$MANIFEST" >/dev/null 2>&1 && printf 'doctrine\n' + ) + while IFS= read -r id; do + for surface in "${surfaces[@]}"; do + if [[ "$surface" == doctrine ]]; then + injected="$(jq -r --arg i "$id" \ + '[.doctrine.inject[]? | select(. == $i)] | length' "$MANIFEST")" + else + injected="$(jq -r --arg i "$id" --arg f "$surface" \ + '[.targets[] | select(.file == $f) | .inject[] | select(. == $i)] | length' "$MANIFEST")" + fi + [[ "$injected" != 0 ]] && continue + reason="$(jq -r --arg i "$id" --arg s "$surface" \ + '.enforced[] | select(.id == $i) | .opt_out[$s] // empty' "$MANIFEST")" + if [[ -z "$reason" ]]; then + printf '[GAP] enforced region "%s" reaches neither surface "%s" nor an opt_out for it\n' "$id" "$surface" >&2 + printf ' -> inject it there, or record enforced[id=%s].opt_out["%s"] with the reason\n' "$id" "$surface" >&2 + gap=1 + else + printf '[check] OK -> %s excluded from %s (%s)\n' "$id" "$surface" "$reason" + fi + done + done < <(jq -r '.enforced[].id' "$MANIFEST") + return "$gap" +} + do_check() { require_tools local file ids drift=0 expected actual + check_coverage || drift=1 while IFS= read -r file; do if ! validate_markers "$REPO_ROOT/$file"; then drift=1; continue; fi mapfile -t ids < <(target_inject "$file") diff --git a/tests/compile-harness.bats b/tests/compile-harness.bats index f3de44ad..58a7784e 100644 --- a/tests/compile-harness.bats +++ b/tests/compile-harness.bats @@ -90,6 +90,66 @@ run_refresh() { run env VAULT_PATH="$VAULT" "$SCRIPT" --refresh; } [[ "$output" == *"DRIFT"* ]] } +# --- HARNESS-072: coverage, not just consistency ------------------------------- +# The region diff renders its expected side from the target's OWN inject list, so +# an id missing from that list is missing from both sides and the target reports +# OK. These tests pin the separate coverage assertion that does catch it. + +seed_second_surface() { + printf 'intro\n\n\n\n\noutro\n' > "$REPO/TARGET2.md" +} + +# A second surface that the `demo` region is NOT injected into. $1 = the JSON +# object for the single `demo` enforced entry, so each test varies only the +# opt_out. Nothing is injected into TARGET2.md and its inject list is empty, so +# the region diff is genuinely consistent there — only coverage has anything to +# say about it. +write_two_surface_manifest() { + cat > "$REPO/harness/manifest.json" < TARGET2.md"* ]] + # And an orphan check would miss it too — the id is in use on TARGET.md. + [[ "$output" == *"[check] OK -> TARGET.md"* ]] +} + +@test "HARNESS-072: a declared opt_out with a reason satisfies coverage" { + seed_second_surface + write_two_surface_manifest '{ "id": "demo", "source": "test-pattern.md#1-demo-rule", + "opt_out": { "TARGET2.md": "this surface states the rule in hand-written prose" } }' + run_refresh; [ "$status" -eq 0 ] + run "$SCRIPT" --check + [ "$status" -eq 0 ] + [[ "$output" == *"excluded from TARGET2.md"* ]] + [[ "$output" == *"hand-written prose"* ]] +} + +@test "HARNESS-072: an opt_out with an empty reason is still a gap" { + seed_second_surface + write_two_surface_manifest '{ "id": "demo", "source": "test-pattern.md#1-demo-rule", + "opt_out": { "TARGET2.md": "" } }' + run_refresh; [ "$status" -eq 0 ] + run "$SCRIPT" --check + [ "$status" -ne 0 ] + [[ "$output" == *"GAP"* ]] +} + @test "AC3: --check works offline (no vault) from the committed record" { run_refresh; [ "$status" -eq 0 ] run env VAULT_PATH="$TMP/nonexistent" "$SCRIPT" --check From cc1830bb4105c8bc2eb17ddda3857733789c94be Mon Sep 17 00:00:00 2001 From: Manu Date: Sat, 15 Aug 2026 01:06:46 -0600 Subject: [PATCH 06/12] feat(harness): a PR you open is watched, not abandoned MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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//" 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 --- AGENTS.md | 10 +++++++++- ai/claude/CLAUDE.md | 10 +++++++++- harness/enforced/pr-stewardship.md | 8 ++++++++ harness/manifest.json | 7 ++++--- 4 files changed, 30 insertions(+), 5 deletions(-) create mode 100644 harness/enforced/pr-stewardship.md diff --git a/AGENTS.md b/AGENTS.md index ed3668fb..41295eca 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -233,7 +233,7 @@ complex. **No conversational filler.** These rules **counter agent harness defaults** that would otherwise silently win at runtime (e.g. a CLI whose default appends `Co-Authored-By` to commits). They are re-affirmed here because a default not explicitly overridden is the default that ships. Canonical source: `00_meta/patterns/pattern-git-workflow.md` §6–§9. *(Generated by the HARNESS engine via `scripts/compile-harness.sh` — edit the vault pattern, then re-run setup. Do NOT edit between the markers.)* - + - **No AI attribution** in git history or GitHub messages (commits, PRs, issues). - No `Co-Authored-By` trailers referencing AI agents. - No bot-style emojis or "Generated with" footers. @@ -257,6 +257,14 @@ Working code is not a finished change. Before saying done, each of these is true 5. **Evidence** — no completion claim without the command output that proves it, produced in this session. Any of the five may be skipped, but only as a stated decision naming which one and why. Silence is not a skip. + +> Injected verbatim into every agent's instructions (harness `enforced` id `pr-stewardship`). It elaborates Definition of Done §4 — "an open PR is not finished work" — into what that item leaves implicit: what you still owe a PR after you push it, and what does not count as having been reviewed. + +**What binds is the disposition, not the waiting.** Before the change is called done, the PR's checks and its reviewer output are dispositioned — each one applied, ticketed, or declined with a reason. *How* you learn they arrived is not prescribed: a project that already tells you when to look back — the human notifies, a hook fires — has met this, and its instruction wins. Absent such a signal the default mechanism is to stay: the window closes at the first of an actionable reviewer comment or ten minutes after the checks settle, and pushing a fix reopens it, because the reviewer re-reviews. Leaving with nothing dispositioned hands the next session a change nobody read. + +**A comment is not a review, and green checks are not the end of one.** Both halves have been observed failing here. On one PR every check went green and the reviewer then posted four Major findings. On another, 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.** Tell the two apart by content, never by author — a review names files, lines, or claims; a notice talks about the review itself. Proceeding on an unreviewed PR is allowed; proceeding silently is not. "Merged unreviewed, reviewer quota exhausted" is a disclosure; saying nothing is a claim of review that never happened. + +**A change that closes a spec gets an independent adversarial review before it archives.** The trigger is the archive gate and nothing wider — not every PR that touches a spec folder. It names an obligation that already binds mechanically, so the only question is whether you meet it deliberately or discover it as a refusal: the spec gate declines to merge a PR closing a spec's issue without archiving it, `spec archive` declines without a passing review, and the reviewer pool declines one signed by the wrong model. The reviewer must not be the implementer; that independence is the entire value. ### Interaction Discipline diff --git a/ai/claude/CLAUDE.md b/ai/claude/CLAUDE.md index d8281e7e..3a1fbef5 100644 --- a/ai/claude/CLAUDE.md +++ b/ai/claude/CLAUDE.md @@ -28,7 +28,7 @@ If session start context reports memory files needing archive (>60 days cold): ## Claude Code Tooling Notes * **Overrides of harness defaults (generated).** Sourced from the vault via `scripts/compile-harness.sh` — edit the vault pattern + re-run setup, not here: - + - **No AI attribution** in git history or GitHub messages (commits, PRs, issues). - No `Co-Authored-By` trailers referencing AI agents. - No bot-style emojis or "Generated with" footers. @@ -52,6 +52,14 @@ Working code is not a finished change. Before saying done, each of these is true 5. **Evidence** — no completion claim without the command output that proves it, produced in this session. Any of the five may be skipped, but only as a stated decision naming which one and why. Silence is not a skip. + +> Injected verbatim into every agent's instructions (harness `enforced` id `pr-stewardship`). It elaborates Definition of Done §4 — "an open PR is not finished work" — into what that item leaves implicit: what you still owe a PR after you push it, and what does not count as having been reviewed. + +**What binds is the disposition, not the waiting.** Before the change is called done, the PR's checks and its reviewer output are dispositioned — each one applied, ticketed, or declined with a reason. *How* you learn they arrived is not prescribed: a project that already tells you when to look back — the human notifies, a hook fires — has met this, and its instruction wins. Absent such a signal the default mechanism is to stay: the window closes at the first of an actionable reviewer comment or ten minutes after the checks settle, and pushing a fix reopens it, because the reviewer re-reviews. Leaving with nothing dispositioned hands the next session a change nobody read. + +**A comment is not a review, and green checks are not the end of one.** Both halves have been observed failing here. On one PR every check went green and the reviewer then posted four Major findings. On another, 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.** Tell the two apart by content, never by author — a review names files, lines, or claims; a notice talks about the review itself. Proceeding on an unreviewed PR is allowed; proceeding silently is not. "Merged unreviewed, reviewer quota exhausted" is a disclosure; saying nothing is a claim of review that never happened. + +**A change that closes a spec gets an independent adversarial review before it archives.** The trigger is the archive gate and nothing wider — not every PR that touches a spec folder. It names an obligation that already binds mechanically, so the only question is whether you meet it deliberately or discover it as a refusal: the spec gate declines to merge a PR closing a spec's issue without archiving it, `spec archive` declines without a passing review, and the reviewer pool declines one signed by the wrong model. The reviewer must not be the implementer; that independence is the entire value. * **Skills.** `~/.claude/skills//SKILL.md` auto-load via slash commands. Skill auto-loading is a Claude Code feature, not portable. Skill **content** is portable: the harness render path (`harness/skills//` with `targets[]`, deployed offline by `compile-harness.sh --deploy` — ADR-021) emits each skill as an OpenCode command at `~/.config/opencode/commands/.md`. (AI-012 shipped the original transform in PR #43; the standalone `skills-to-opencode.sh` was since retired.) * **TaskCreate / TaskUpdate / TaskList.** Use for non-trivial multi-step work (≥3 distinct steps). Mark `in_progress` BEFORE starting; mark `completed` immediately on finish. Don't batch updates. diff --git a/harness/enforced/pr-stewardship.md b/harness/enforced/pr-stewardship.md new file mode 100644 index 00000000..bc451888 --- /dev/null +++ b/harness/enforced/pr-stewardship.md @@ -0,0 +1,8 @@ + +> Injected verbatim into every agent's instructions (harness `enforced` id `pr-stewardship`). It elaborates Definition of Done §4 — "an open PR is not finished work" — into what that item leaves implicit: what you still owe a PR after you push it, and what does not count as having been reviewed. + +**What binds is the disposition, not the waiting.** Before the change is called done, the PR's checks and its reviewer output are dispositioned — each one applied, ticketed, or declined with a reason. *How* you learn they arrived is not prescribed: a project that already tells you when to look back — the human notifies, a hook fires — has met this, and its instruction wins. Absent such a signal the default mechanism is to stay: the window closes at the first of an actionable reviewer comment or ten minutes after the checks settle, and pushing a fix reopens it, because the reviewer re-reviews. Leaving with nothing dispositioned hands the next session a change nobody read. + +**A comment is not a review, and green checks are not the end of one.** Both halves have been observed failing here. On one PR every check went green and the reviewer then posted four Major findings. On another, 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.** Tell the two apart by content, never by author — a review names files, lines, or claims; a notice talks about the review itself. Proceeding on an unreviewed PR is allowed; proceeding silently is not. "Merged unreviewed, reviewer quota exhausted" is a disclosure; saying nothing is a claim of review that never happened. + +**A change that closes a spec gets an independent adversarial review before it archives.** The trigger is the archive gate and nothing wider — not every PR that touches a spec folder. It names an obligation that already binds mechanically, so the only question is whether you meet it deliberately or discover it as a refusal: the spec gate declines to merge a PR closing a spec's issue without archiving it, `spec archive` declines without a passing review, and the reviewer pool declines one signed by the wrong model. The reviewer must not be the implementer; that independence is the entire value. diff --git a/harness/manifest.json b/harness/manifest.json index 7372ceb7..49e7c702 100644 --- a/harness/manifest.json +++ b/harness/manifest.json @@ -7,6 +7,7 @@ { "id": "no-phase-references", "source": "pattern-git-workflow.md#7-message-content-policy" }, { "id": "no-auto-merge", "source": "pattern-git-workflow.md#9-merge-policy" }, { "id": "definition-of-done", "source": "pattern-change-lifecycle.md#definition-of-done" }, + { "id": "pr-stewardship", "source": "pattern-change-lifecycle.md#pr-stewardship" }, { "id": "pr-sizing", "source": "pattern-git-workflow.md#10-pr-sizing-policy", @@ -21,13 +22,13 @@ "agent": "agents", "kind": "native", "file": "AGENTS.md", - "inject": ["no-attribution", "english-only", "no-phase-references", "no-auto-merge", "definition-of-done"] + "inject": ["no-attribution", "english-only", "no-phase-references", "no-auto-merge", "definition-of-done", "pr-stewardship"] }, { "agent": "claude", "kind": "pointer", "file": "ai/claude/CLAUDE.md", - "inject": ["no-attribution", "english-only", "no-phase-references", "no-auto-merge", "definition-of-done"] + "inject": ["no-attribution", "english-only", "no-phase-references", "no-auto-merge", "definition-of-done", "pr-stewardship"] } ], "skills": { @@ -45,7 +46,7 @@ "catalog": { "agent": "copilot", "file": ".copilot/copilot-instructions.md" } }, "doctrine": { - "inject": ["no-attribution", "english-only", "no-phase-references", "no-auto-merge", "definition-of-done", "pr-sizing"], + "inject": ["no-attribution", "english-only", "no-phase-references", "no-auto-merge", "definition-of-done", "pr-stewardship", "pr-sizing"], "deploy": [ { "agent": "agy", From c0598f5e43cb0e492c4445a360cd3fd2e2312960 Mon Sep 17 00:00:00 2001 From: Manu Date: Sat, 15 Aug 2026 01:06:58 -0600 Subject: [PATCH 07/12] chore(harness): sync six committed records with their vault sources MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- harness/agents/curator/AGENT.md | 3 ++- .../skills/dispatching-parallel-agents/SKILL.md | 16 ++++++++++++++-- .../condition-based-waiting.md | 2 ++ .../systematic-debugging/defense-in-depth.md | 2 ++ .../systematic-debugging/root-cause-tracing.md | 2 ++ .../testing-anti-patterns.md | 2 ++ 6 files changed, 24 insertions(+), 3 deletions(-) diff --git a/harness/agents/curator/AGENT.md b/harness/agents/curator/AGENT.md index ed44499a..175c6434 100644 --- a/harness/agents/curator/AGENT.md +++ b/harness/agents/curator/AGENT.md @@ -1,7 +1,7 @@ --- generated: true generated_from: 00_meta/agents/definitions/curator/AGENT.md -generated_sha: 94a6cfb6a212d027 +generated_sha: 669ffa7acb345ea6 id: agent-curator type: agent status: active @@ -12,6 +12,7 @@ kind: invocable model: top capabilities: [read, search, edit, shell] skills: [vault-doctor, crystallize, insights, genre-picker, context-refresh, handoff, place-knowledge, dispose-proposals] +owner: manu --- # Curator diff --git a/harness/skills/dispatching-parallel-agents/SKILL.md b/harness/skills/dispatching-parallel-agents/SKILL.md index 461942b8..700c3d8a 100644 --- a/harness/skills/dispatching-parallel-agents/SKILL.md +++ b/harness/skills/dispatching-parallel-agents/SKILL.md @@ -1,7 +1,7 @@ --- generated: true generated_from: 00_meta/skills/dispatching-parallel-agents/SKILL.md -generated_sha: a716db8d40dd96ce +generated_sha: 4ef93031ee74f9c8 id: dispatching-parallel-agents-skill type: skill status: active @@ -81,9 +81,21 @@ Your task: 3. Fix the issue 4. Do NOT change unrelated code -Return: Summary of root cause and changes made. +Return: Summary of root cause and changes made, ending with the mandatory reconciliation block: + +```yaml:reconciliation +agent_verdict: SUCCESS # SUCCESS | FAILURE | PARTIAL +files_modified: + - path/to/file1 +tickets_created: [] +unresolved_blockers: [] +summary: "Brief 1-2 sentence human summary" ``` +## Mandatory Structured Handoff + +Every dispatched subagent MUST conclude with the `yaml:reconciliation` block so the orchestrator can mechanically verify changes against `git status` without ambiguous parsing. + ## Common Mistakes | Mistake | Fix | diff --git a/harness/skills/systematic-debugging/condition-based-waiting.md b/harness/skills/systematic-debugging/condition-based-waiting.md index 6d203a22..2874314e 100644 --- a/harness/skills/systematic-debugging/condition-based-waiting.md +++ b/harness/skills/systematic-debugging/condition-based-waiting.md @@ -3,6 +3,8 @@ id: "condition-based-waiting" type: reference status: active tags: [] +owner: manu +created: "2026-06-10" --- # Condition-Based Waiting diff --git a/harness/skills/systematic-debugging/defense-in-depth.md b/harness/skills/systematic-debugging/defense-in-depth.md index 25f40703..b34ee525 100644 --- a/harness/skills/systematic-debugging/defense-in-depth.md +++ b/harness/skills/systematic-debugging/defense-in-depth.md @@ -3,6 +3,8 @@ id: "defense-in-depth" type: reference status: active tags: [] +owner: manu +created: "2026-06-10" --- # Defense-in-Depth Validation diff --git a/harness/skills/systematic-debugging/root-cause-tracing.md b/harness/skills/systematic-debugging/root-cause-tracing.md index ab45d29c..fd1fd23e 100644 --- a/harness/skills/systematic-debugging/root-cause-tracing.md +++ b/harness/skills/systematic-debugging/root-cause-tracing.md @@ -3,6 +3,8 @@ id: "root-cause-tracing" type: reference status: active tags: [] +owner: manu +created: "2026-06-10" --- # Root Cause Tracing diff --git a/harness/skills/test-driven-development/testing-anti-patterns.md b/harness/skills/test-driven-development/testing-anti-patterns.md index 8897f21f..1ea91462 100644 --- a/harness/skills/test-driven-development/testing-anti-patterns.md +++ b/harness/skills/test-driven-development/testing-anti-patterns.md @@ -3,6 +3,8 @@ id: "testing-anti-patterns" type: reference status: active tags: [] +owner: manu +created: "2026-06-10" --- # Testing Anti-Patterns From 0d35f8d60aba5677fdaf48da47c7512bc90a1fab Mon Sep 17 00:00:00 2001 From: Manu Date: Sat, 15 Aug 2026 01:07:38 -0600 Subject: [PATCH 08/12] chore(harness): sync pr-review-triage record with its retriggered vault source Refs #963 --- harness/skills/pr-review-triage/SKILL.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/harness/skills/pr-review-triage/SKILL.md b/harness/skills/pr-review-triage/SKILL.md index bc94b34a..f9f4beb0 100644 --- a/harness/skills/pr-review-triage/SKILL.md +++ b/harness/skills/pr-review-triage/SKILL.md @@ -1,14 +1,14 @@ --- generated: true generated_from: 00_meta/skills/pr-review-triage/SKILL.md -generated_sha: 97df7c925532c797 +generated_sha: fc4a81aaec806760 id: pr-review-triage-skill type: skill status: active created: "2026-08-08" owner: manu name: pr-review-triage -description: Triage an open pull request after its checks and reviewers have run — read the CI result, read every review comment, and give each one a disposition (apply / skip / defer) with a one-line reason. Triggers on /pr-review-triage, "triage the PR", "review the review", "what did the reviewer say", "revisa los comentarios de la PR", "check CI and the bot comments", and by default once a PR you opened has finished its checks. Never applies a change or merges without explicit human confirmation. +description: Triage an open pull request after its checks and reviewers have run — read the CI result, read every review comment, and give each one a disposition (apply / skip / defer) with a one-line reason. Triggers on /pr-review-triage, "triage the PR", "review the review", "what did the reviewer say", "revisa los comentarios de la PR", "check CI and the bot comments", and by default once a PR you opened has come back — from its checks and from its reviewers, whichever lands later, because checks finishing is not the end of the window. Never applies a change or merges without explicit human confirmation. allowed-tools: [Bash, Read, Grep] --- From e17c880737b09af60057050277482469cdb8a632 Mon Sep 17 00:00:00 2001 From: Manu Date: Sat, 15 Aug 2026 01:10:46 -0600 Subject: [PATCH 09/12] docs(spec): close out HARNESS-072 with evidence and the lesson it produced MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/lessons.md | 13 +++ .../HARNESS-072-pr-stewardship/features.json | 58 +++++++++++++ specs/HARNESS-072-pr-stewardship/proposal.md | 25 ++++-- .../review-transcript.jsonl | 0 specs/HARNESS-072-pr-stewardship/tasks.md | 77 +++++++++-------- .../verification.md | 85 ++++++++++++++----- 6 files changed, 192 insertions(+), 66 deletions(-) create mode 100644 specs/HARNESS-072-pr-stewardship/features.json create mode 100644 specs/HARNESS-072-pr-stewardship/review-transcript.jsonl diff --git a/docs/lessons.md b/docs/lessons.md index 24197e34..522379c9 100644 --- a/docs/lessons.md +++ b/docs/lessons.md @@ -229,6 +229,8 @@ awk '/^## Entries$/,0' docs/lessons.md | grep '^### \[' | sed -E 's/^### \[([0-9 - [2026-08-14] A dormant declared field must be validated on the same schedule it's written, not the schedule it activates on - [2026-08-14] An agent that cannot reach the repo still writes a confident review - [2026-08-14] Widening a shared return type is a change to every consumer, and Go's zero values hide the ones you missed +- [2026-08-15] A check whose precondition the architecture forbids reports SKIP forever, and SKIP reads as nothing-to-check +- [2026-08-15] A check that cannot fail the way you cite it --- @@ -2388,3 +2390,14 @@ The blast radius was also wider than the one function: because `compile-harness. **Rule**: when an architecture change removes a *precondition* that existing checks depend on, the checks do not fail — they go quiet, in the branch that was written to mean "this is fine". Grep for consumers of what you removed (an env var, a file, a daemon, a login step) and ask of each: on a correctly configured machine after this change, which branch does it take? A check that can only take its no-op branch is not a weakened check, it is a deleted one that still prints. Two tells worth trusting: a remediation string naming a command that no longer exists is a dead check with a timestamp on it — grep the repo for the command before believing the message — and a health section that has never once complained is a claim about the section, not about the system. **Tags**: `verification`, `secrets`, `doctor`, `architecture-migration`, `observability` +### [2026-08-15] A check that cannot fail the way you cite it + +**Context**: HARNESS-072 (#963) adds an `enforced` harness region — text injected verbatim into every agent's instructions across every repo. The spec's own Risks section named the obvious failure: *"a region added to `enforced` but missing from a target's `inject` list silently misses that surface"* — the producer-updated / consumer-forgotten class that BUG-077 had been. It named the mitigation in the same breath: `compile-harness.sh --check` is the test, not a hand count. The acceptance criterion was written on that basis. + +**Problem**: `--check` cannot detect that failure and never could. `do_check` builds the expected side of its diff 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 comparison. The diff is clean, the target prints `[check] OK`, and the surface that never received the rule is indistinguishable from one that did. It is a consistency check (does the injected text match its record?) being cited as a coverage check (did the region reach the surfaces it should?). Running the new assertion against the tree as it stood produced two immediate hits on `pr-sizing`, doctrine-only by a decision argued at length in #830 and recorded nowhere a machine could read — a real exclusion that had survived on institutional memory alone. + +**Solution**: a separate `check_coverage` pass over every enforced id × every surface: injected, or an `opt_out` entry naming that surface **with a reason** — an empty reason is still a gap. The decisive test asserts both halves on one run: the region diff reports `OK -> TARGET2.md` while coverage reports `GAP` on that same file. That also picks the shape — an orphan check ("is this id used anywhere?") would pass the partial case, and the partial case is the likelier mistake. Found by a second session reading this worktree from the outside, and verified against the source before being acted on. + +**Rule**: when a spec names a command as the mitigation for a risk, open the command and find the line that would fail. A check earns its citation by the question it actually asks, and the question is usually narrower than its name suggests — `--check` also cannot see a committed record trailing its vault source, because it is offline by design (ADR-013), which is how six stale records sat clean until someone ran `--refresh`. This is the `pattern-verification-fails-toward-unproven` family in its cheapest form: not a check that ran and lied, but a check that was never capable of the answer and was trusted for it anyway. The tell is a mitigation you can state but not demonstrate red. + +**Tags**: `harness`, `verification`, `spec-driven-development`, `ci` diff --git a/specs/HARNESS-072-pr-stewardship/features.json b/specs/HARNESS-072-pr-stewardship/features.json new file mode 100644 index 00000000..88cb5ab9 --- /dev/null +++ b/specs/HARNESS-072-pr-stewardship/features.json @@ -0,0 +1,58 @@ +[ + { + "id": "HARNESS-072-pr-stewardship-f1", + "behavior": "AC1 — an enforced region `pr-stewardship` is declared in the manifest with a vault source, and its source-of-record exists and is non-empty.", + "verification": "jq -e '.enforced[] | select(.id==\"pr-stewardship\") | .source' harness/manifest.json >/dev/null && test -s harness/enforced/pr-stewardship.md", + "state": "pending", + "evidence": "" + }, + { + "id": "HARNESS-072-pr-stewardship-f2", + "behavior": "AC2 — the region reaches every surface definition-of-done reaches: positive per-surface evidence in both committed targets, plus the doctrine inject list that carries it to the agy and codex payloads. Deliberately NOT `--check`, which is blind to a skipped surface (see f8).", + "verification": "grep -q 'the disposition, not the waiting' AGENTS.md && grep -q 'the disposition, not the waiting' ai/claude/CLAUDE.md && jq -e '.doctrine.inject | index(\"pr-stewardship\")' harness/manifest.json >/dev/null", + "state": "pending", + "evidence": "" + }, + { + "id": "HARNESS-072-pr-stewardship-f3", + "behavior": "AC3 — the region binds a disposition rather than a mechanism: it states what binds, and that a project's own signal for when to look back wins over the default timed watch.", + "verification": "grep -q 'What binds is the disposition, not the waiting' harness/enforced/pr-stewardship.md && grep -q 'its instruction wins' harness/enforced/pr-stewardship.md", + "state": "pending", + "evidence": "" + }, + { + "id": "HARNESS-072-pr-stewardship-f4", + "behavior": "AC4 — the adversarial-review obligation fires at the archive gate and introduces no wider trigger.", + "verification": "grep -q 'trigger is the archive gate and nothing wider' harness/enforced/pr-stewardship.md", + "state": "pending", + "evidence": "" + }, + { + "id": "HARNESS-072-pr-stewardship-f5", + "behavior": "AC5 — a reviewer notice reporting that no review ran leaves the PR unreviewed, stated in the region itself.", + "verification": "grep -q 'A notice that no review ran leaves the PR unreviewed' harness/enforced/pr-stewardship.md", + "state": "pending", + "evidence": "" + }, + { + "id": "HARNESS-072-pr-stewardship-f6", + "behavior": "AC6 — pr-review-triage triggers on whichever of checks and reviewers lands later, and its wait is expressed in gh commands with no agent-specific primitive.", + "verification": "grep -q 'whichever lands later' harness/skills/pr-review-triage/SKILL.md && grep -q 'gh pr checks' harness/skills/pr-review-triage/SKILL.md", + "state": "pending", + "evidence": "" + }, + { + "id": "HARNESS-072-pr-stewardship-f7", + "behavior": "AC7 — vault source and committed records agree; no harness drift.", + "verification": "./scripts/compile-harness.sh --check", + "state": "pending", + "evidence": "" + }, + { + "id": "HARNESS-072-pr-stewardship-f8", + "behavior": "AC8 — --check asserts coverage separately from its region diff: a region absent from a surface fails, a declared opt_out with a reason passes, an empty reason still fails. The first case proves the diff reports OK on the very surface coverage rejects.", + "verification": "bats tests/compile-harness.bats -f 'HARNESS-072'", + "state": "pending", + "evidence": "" + } +] diff --git a/specs/HARNESS-072-pr-stewardship/proposal.md b/specs/HARNESS-072-pr-stewardship/proposal.md index 829322fd..b3a45344 100644 --- a/specs/HARNESS-072-pr-stewardship/proposal.md +++ b/specs/HARNESS-072-pr-stewardship/proposal.md @@ -123,7 +123,13 @@ optional skill. assertion in the same PR) — resolved as a decision, not silence: see "Coverage guard" below. -### Coverage guard — open decision +### Coverage guard — decided: (b), in this PR + +Taken here rather than ticketed. `feedback_incident_to_guard` is a recorded MUST +— a bug class encountered emits its assertion in the *same* PR — and the +fix-or-ticket escape ("it would break the current change's scope") does not +apply to a spec still in `draft`, whose own Risks section names the class. See +AC8. The options weighed were: Positive per-surface evidence (AC2) proves *this* region reached every surface. It does nothing for the next one, and the repo's own rule is that a bug class @@ -141,12 +147,11 @@ that a region reached the surfaces it should.** Two ways to close it: inject, or you write down why not. `pr-sizing` — deliberately doctrine-only — becomes a declared exclusion instead of a convention someone has to know. -**(b) is the right shape**: it converts a coverage question into a schema the -engine can answer, and it is the only one that catches the partial case. Its -cost is a manifest-schema change touching all six regions plus a bats case, so -it is a separate change from "add a region" and is **not** slipped into this PR. -Pending the user's call: ticket it, or take it here as an agreed scope -extension. +**(b) shipped.** It converts a coverage question into a schema the engine can +answer, and it is the only one that catches the partial case. Run against the +tree as it stood it failed immediately on `pr-sizing` — doctrine-only by a +deliberate decision argued in #830 and recorded nowhere machine-readable — so +the guard's first act was to turn a convention into a declaration. ## Acceptance criteria @@ -183,6 +188,12 @@ extension. an agent with no Claude-specific primitives. - [ ] **AC7** The vault source and the committed harness records agree — `compile-harness.sh --check` reports no drift after `--refresh`. +- [ ] **AC8** `--check` gains a **coverage** assertion distinct from its region + diff: an enforced id absent from a surface fails unless the manifest + records an `opt_out` for that surface **with a reason** (an empty reason is + still a gap). Proved by a test in which the region diff reports + `OK -> TARGET2.md` and coverage reports `GAP` on the same run — the + partial case, which an orphan check would pass. ## References diff --git a/specs/HARNESS-072-pr-stewardship/review-transcript.jsonl b/specs/HARNESS-072-pr-stewardship/review-transcript.jsonl new file mode 100644 index 00000000..e69de29b diff --git a/specs/HARNESS-072-pr-stewardship/tasks.md b/specs/HARNESS-072-pr-stewardship/tasks.md index 7d8ec477..a4256c75 100644 --- a/specs/HARNESS-072-pr-stewardship/tasks.md +++ b/specs/HARNESS-072-pr-stewardship/tasks.md @@ -5,56 +5,55 @@ created: "2026-08-15" # Tasks - HARNESS-072-pr-stewardship -> TDD order. One task = one focused commit. Tick as you go. Reorder freely while spec is in `draft` state; freeze once you start `implementing`. -> -> **Inline markers** (optional, additive — borrowed from `github/spec-kit`, adapt-not-adopt per #141): -> - `[P]` — this task has **no dependency on another unchecked task**, so it is safe to run in parallel (fan out to a `Workflow`, or just batch). TDD chains (test → implement → refactor of the *same* behavior) are sequential and must NOT carry `[P]`; independent behaviors can. -> - `[AC]` — this task helps satisfy **acceptance criterion #``** from `proposal.md`. Lets `/spec check` map coverage deterministically; omit it and the check falls back to semantic judgment. +> TDD order. One task = one focused commit. `[AC]` maps a task to an acceptance criterion in `proposal.md`; `[P]` marks a task with no dependency on another unchecked one. ## Setup -- [ ] Branch created from main: `feat/HARNESS-072-pr-stewardship` -- [ ] `proposal.md` is complete and acceptance criteria are testable -- [ ] No open questions left in `proposal.md` "Risks / open questions" +- [x] Branch created from main: `feat/harness-072-pr-stewardship` +- [x] `proposal.md` is complete and acceptance criteria are testable +- [x] No open questions left in `proposal.md` "Risks / open questions" — the two + kubelab objections are resolved into AC3 and AC4; the coverage guard is + decided (b) rather than left open ## Implementation -> Replace these with the actual steps for this feature. Keep them small (one commit each) and in TDD order. -> The `[P]` / `[AC]` markers are optional — see the legend above. Behaviors 1 and 2 below are independent, so their *first* test task carries `[P]`. - -- [ ] [P] [AC1] Write failing test for -- [ ] [AC1] Implement to make it pass -- [ ] Refactor for clarity (extract, rename, dedupe) -- [ ] [P] [AC2] Write failing test for -- [ ] [AC2] Implement to make it pass -- [ ] ... +Ordered so the guard exists before the region it protects — the region was added +under an assertion that would have caught a partial injection, not after one. + +- [x] [AC3] [AC4] [AC5] Revise the acceptance criteria for the two objections, + and rewrite the region draft to match (`37398c1`) +- [x] [AC2] Correct AC2: `--check` cannot verify coverage, and was named as the + mitigation for the one risk it is blind to (`f1641c4`) +- [x] [AC8] Write `check_coverage` + three bats cases; observe red first, on the + real tree — `pr-sizing` was doctrine-only by an undeclared convention + (`bf2eda9`) +- [x] [AC1] [AC3] [AC4] [AC5] Add the `## PR Stewardship` section to the vault + pattern beside `definition-of-done` (vault `2e351f1a`) +- [x] [AC1] [AC2] Declare the region, inject it into both targets and the + doctrine payload, `--refresh` (`334c1a7`) +- [x] [AC6] Retrigger `pr-review-triage` on whichever of checks and reviewers + lands later (vault `35d93f0a`, record `fdbe0b4`) +- [x] Keep six unrelated drifted records out of the change's diff (`b678103`) ## Closing -- [ ] Every acceptance criterion from `proposal.md` is covered by at least one test -- [ ] Every acceptance criterion has a matching entry in `features.json` (see below) with a non-vacuous verification command -- [ ] Type checks pass -- [ ] Lint passes -- [ ] No unrelated changes in the diff (no scope creep) -- [ ] `verification.md` filled in +- [x] Every acceptance criterion is covered by at least one feature with a + non-vacuous verification command +- [x] Every acceptance criterion has a matching entry in `features.json` +- [x] Lint passes (`shellcheck scripts/compile-harness.sh`, `bash -n`) +- [x] Tests pass (`bats tests/compile-harness.bats` — 47/47) +- [x] No unrelated changes in the diff — the record sync is its own commit +- [x] `verification.md` filled in - [ ] PR opened referencing this spec folder +- [ ] Adversarial review passes before archive (`dotf spec review HARNESS-072-pr-stewardship`) ## Machine-readable features -This spec emits a sibling `features.json` (alongside this file) following [[pattern-feature-list-as-primitive]]. The JSON is the harness-facing contract: each acceptance criterion maps to ≥1 feature with `id`, `behavior`, `verification` (executable command), `state` (lifecycle), and `evidence` (harness-captured output). - -**Pass-state gating:** the agent CANNOT write `"state": "passing"` — only the harness, after running `verification` and capturing exit code 0, may set that terminal state. Reviewers must reject PRs where features.json contains `passing` entries with empty `evidence`. - -Minimal `features.json` skeleton (drop into `/specs/HARNESS-072-pr-stewardship/features.json`): +`features.json` sits beside this file, one feature per acceptance criterion +(f1–f8), each with a shell command whose exit 0 is the pass condition. -```json -[ - { - "id": "HARNESS-072-pr-stewardship-f1", - "behavior": "", - "verification": "", - "state": "pending", - "evidence": "" - } -] -``` +**Pass-state gating:** the agent CANNOT write `"state": "passing"` — only the +harness, after running `verification` and capturing exit code 0, may set that +terminal state. Every entry here is `pending` with empty `evidence`; the +session's observed output lives in `verification.md`, which is a claim about a +run, not a substitute for one. diff --git a/specs/HARNESS-072-pr-stewardship/verification.md b/specs/HARNESS-072-pr-stewardship/verification.md index dbb7f38a..d144ebbe 100644 --- a/specs/HARNESS-072-pr-stewardship/verification.md +++ b/specs/HARNESS-072-pr-stewardship/verification.md @@ -7,36 +7,81 @@ created: "2026-08-15" ## Evidence -Map every acceptance criterion from `proposal.md` to concrete proof (commit hash, test name, or observed behavior). - -- [ ] Criterion 1 -> commit `` / test `` -- [ ] Criterion 2 -> commit `` / test `` -- [ ] Criterion 3 -> commit `` / test `` +| AC | Proof | Where | +|---|---|---| +| AC1 | `harness/enforced/pr-stewardship.md` written by `--refresh` from `pattern-change-lifecycle.md#pr-stewardship` | `334c1a7`, vault `2e351f1a` | +| AC2 | positive grep for *"the disposition, not the waiting"* on all five surfaces: `AGENTS.md`, `ai/claude/CLAUDE.md`, and after `--deploy` `~/.claude/CLAUDE.md`, `~/.gemini/GEMINI.md`, `~/.codex/AGENTS.md` — 1 hit each. Caps hold: GEMINI 6503/12000, codex 6503/32768 | `334c1a7` + session run | +| AC3 | region states *"What binds is the disposition, not the waiting"* and *"its instruction wins"* | `harness/enforced/pr-stewardship.md` | +| AC4 | region states *"The trigger is the archive gate and nothing wider"*; no spec-folder trigger | `harness/enforced/pr-stewardship.md` | +| AC5 | region states *"A notice that no review ran leaves the PR unreviewed"* | `harness/enforced/pr-stewardship.md` | +| AC6 | skill description now triggers on *"whichever lands later"*; body already `gh`-only | vault `35d93f0a`, record `fdbe0b4` | +| AC7 | `./scripts/compile-harness.sh --check` → exit 0, "no harness drift" | session run | +| AC8 | `bats tests/compile-harness.bats -f 'HARNESS-072'` → 3/3; guard observed **red before green** twice | `bf2eda9` | ## Test status -- Test suite: ` -> ` -- Manual smoke test: what was exercised, what was observed -- No regressions in existing test suite: yes / no (if no, document) +- `~/.local/bin/bats tests/compile-harness.bats` → **47/47 ok**, 0 failures. +- `~/.local/bin/shellcheck scripts/compile-harness.sh` → clean. `bash -n` → clean. +- `./scripts/compile-harness.sh --check` → exit 0. +- All eight `features.json` verification commands executed this session → 8/8 exit 0. -## Decisions made during implementation +**Red observed before green, on real state and not a fixture:** -Brief log of non-obvious trade-offs or course corrections taken during the work. Routine choices belong in commit messages, not here. +``` +$ ./scripts/compile-harness.sh --check # before declaring pr-sizing's exclusion +[GAP] enforced region "pr-sizing" reaches neither surface "AGENTS.md" nor an opt_out for it +[GAP] enforced region "pr-sizing" reaches neither surface "ai/claude/CLAUDE.md" nor an opt_out for it -- -- +$ ./scripts/compile-harness.sh --check # with pr-stewardship wired to AGENTS.md only +[GAP] enforced region "pr-stewardship" reaches neither surface "ai/claude/CLAUDE.md" nor an opt_out for it +``` -## Promotion candidates +The second is the case that justifies the guard's shape: an orphan check would +have passed it, because `pr-stewardship` was in use on another surface. -Before archiving, flag what (if anything) should be promoted to the vault. If all three are "no", archive in repo is the only persistence. +## Decisions made during implementation + +- **The obligation is the disposition; the timed watch is only a default.** The + first draft made the watch the rule, which would have overridden a standing + user preference in another project ("do not poll CI, I notify") by a route the + user never chose. A project signal now *satisfies* the region instead of + contradicting it. Rejected a hardening of this opt-out ("only a signal written + in the project's instructions counts") — it would have ranked a file above a + live instruction from the user, a worse bug than the lawyering it prevented. +- **The adversarial-review trigger stays at the archive gate.** "Touches + `specs//`" caught nearly every docs PR in a repo whose `tasks.md` is ticked + as work proceeds. +- **AC2's original verification was vacuous and is the reason AC8 exists.** + `--check` renders its expected side from the target's own `inject` list, so a + region missing from that list is missing from both sides of the diff and the + surface reports `OK`. The spec had named that check as the mitigation for + precisely the risk it cannot see. Found by a second session reviewing this + worktree from the outside; verified against the source before acting on it. +- **The coverage guard was taken in this PR rather than ticketed.** + `feedback_incident_to_guard` requires the assertion in the same PR, and the + fix-or-ticket escape does not apply to a spec still in `draft`. +- **`--check` is offline by design (ADR-013) and cannot see a record trailing its + vault source.** Six records were found stale only by running `--refresh`. Same + shape as the coverage gap — a check answering a narrower question than the one + asked of it — but not fixed here: CI has no vault, on purpose. + +## Promotion candidates -- [ ] Lesson for the repo's `docs/lessons.md`? -- [ ] ADR-worthy decision for the repo's `docs/adr/adr-XXX.md`? -- [ ] New pattern candidate for `00_meta/patterns/`? Only if this recurs in >1 project. +- [x] Lesson for the repo's `docs/lessons.md`? **yes** — "a check that cannot + fail the way you cite it": `--check` was named as the mitigation for a risk + it is structurally blind to. Generalises past this spec. +- [ ] ADR-worthy decision for the repo's `docs/adr/`? no — the manifest gains a + field, not an architecture. +- [ ] New pattern candidate for `00_meta/patterns/`? no — the cross-project half + already exists as `pattern-verification-fails-toward-unproven`; this is one + more instance of it, not a new pattern. ## Archive checklist +- [ ] Adversarial review passes (`dotf spec review HARNESS-072-pr-stewardship`) - [ ] `proposal.md` frontmatter set to `status: archived` -- [ ] Folder moved: `specs/HARNESS-072-pr-stewardship/` -> `specs/archive/HARNESS-072-pr-stewardship/` -- [ ] Bitácora board ticket for this spec moved to Done / closed with PR link (ADR-018) -- [ ] Promotions above executed (if any) +- [ ] Folder moved to specs/archive/HARNESS-072-pr-stewardship/ (plain text: the + path does not exist yet, and a backticked one is a live claim the doc-path + guard checks) +- [ ] Bitácora #963 closed with the PR link (ADR-018) +- [ ] Promotion above executed From 5533829b24a2615d6a2da41e5f3a9d328750d51c Mon Sep 17 00:00:00 2001 From: Manu Date: Sat, 15 Aug 2026 01:10:59 -0600 Subject: [PATCH 10/12] docs(spec): drop the parked region draft now that it has a real home 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 --- specs/HARNESS-072-pr-stewardship/draft-region.md | 15 --------------- 1 file changed, 15 deletions(-) delete mode 100644 specs/HARNESS-072-pr-stewardship/draft-region.md diff --git a/specs/HARNESS-072-pr-stewardship/draft-region.md b/specs/HARNESS-072-pr-stewardship/draft-region.md deleted file mode 100644 index 4ea595cc..00000000 --- a/specs/HARNESS-072-pr-stewardship/draft-region.md +++ /dev/null @@ -1,15 +0,0 @@ - - -## PR Stewardship - -> Injected verbatim into every agent's instructions (harness `enforced` id `pr-stewardship`). It elaborates Definition of Done §4 — "an open PR is not finished work" — into what that item leaves implicit: what you still owe a PR after you push it, and what does not count as having been reviewed. - -**What binds is the disposition, not the waiting.** Before the change is called done, the PR's checks and its reviewer output are dispositioned — each one applied, ticketed, or declined with a reason. *How* you learn they arrived is not prescribed: a project that already tells you when to look back — the human notifies, a hook fires — has met this, and its instruction wins. Absent such a signal the default mechanism is to stay: the window closes at the first of an actionable reviewer comment or ten minutes after the checks settle, and pushing a fix reopens it, because the reviewer re-reviews. Leaving with nothing dispositioned hands the next session a change nobody read. - -**A comment is not a review, and green checks are not the end of one.** Both halves have been observed failing here. On one PR every check went green and the reviewer then posted four Major findings. On another, 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.** Tell the two apart by content, never by author — a review names files, lines, or claims; a notice talks about the review itself. Proceeding on an unreviewed PR is allowed; proceeding silently is not. "Merged unreviewed, reviewer quota exhausted" is a disclosure; saying nothing is a claim of review that never happened. - -**A change that closes a spec gets an independent adversarial review before it archives.** The trigger is the archive gate and nothing wider — not every PR that touches a spec folder. It names an obligation that already binds mechanically, so the only question is whether you meet it deliberately or discover it as a refusal: the spec gate declines to merge a PR closing a spec's issue without archiving it, `spec archive` declines without a passing review, and the reviewer pool declines one signed by the wrong model. The reviewer must not be the implementer; that independence is the entire value. From b639b01063ea28442e0817e1c094185f8cabc9df Mon Sep 17 00:00:00 2001 From: Manu Date: Sat, 15 Aug 2026 01:31:12 -0600 Subject: [PATCH 11/12] docs(spec): HARNESS-072 moves to verifying Implementation is complete and its evidence is recorded; what remains is the adversarial review, which cannot launch until #985 lands. The frontmatter should say that rather than still claiming draft against ten commits and an open PR. Refs #963 --- specs/HARNESS-072-pr-stewardship/proposal.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/specs/HARNESS-072-pr-stewardship/proposal.md b/specs/HARNESS-072-pr-stewardship/proposal.md index b3a45344..39e31375 100644 --- a/specs/HARNESS-072-pr-stewardship/proposal.md +++ b/specs/HARNESS-072-pr-stewardship/proposal.md @@ -1,7 +1,7 @@ --- id: "HARNESS-072-pr-stewardship" type: spec -status: draft # draft | implementing | verifying | archived +status: verifying # draft | implementing | verifying | archived created: "2026-08-15" issue: "mlorentedev/dotfiles#963" # repo#NNN — GitHub issue / Project item that tracks this spec tags: [spec, proposal, harness, review] From 913890c341089fe23987d5b51adf5e8593862d7f Mon Sep 17 00:00:00 2001 From: Manu Date: Sat, 15 Aug 2026 01:56:20 -0600 Subject: [PATCH 12/12] chore(spec): drop the 0-byte transcript a dead review launch left behind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .gitignore | 1 + specs/HARNESS-072-pr-stewardship/review-transcript.jsonl | 0 2 files changed, 1 insertion(+) delete mode 100644 specs/HARNESS-072-pr-stewardship/review-transcript.jsonl diff --git a/.gitignore b/.gitignore index 484f2355..8ad215f6 100644 --- a/.gitignore +++ b/.gitignore @@ -69,3 +69,4 @@ CLAUDE.md # Added by ggshield .cache_ggshield +specs/*/review-transcript.jsonl diff --git a/specs/HARNESS-072-pr-stewardship/review-transcript.jsonl b/specs/HARNESS-072-pr-stewardship/review-transcript.jsonl deleted file mode 100644 index e69de29b..00000000