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/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/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/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/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 8ef0b43a..49e7c702 100644 --- a/harness/manifest.json +++ b/harness/manifest.json @@ -7,20 +7,28 @@ { "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-stewardship", "source": "pattern-change-lifecycle.md#pr-stewardship" }, + { + "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": [ { "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": { @@ -38,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", 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/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] --- 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 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/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 new file mode 100644 index 00000000..39e31375 --- /dev/null +++ b/specs/HARNESS-072-pr-stewardship/proposal.md @@ -0,0 +1,205 @@ +--- +id: "HARNESS-072-pr-stewardship" +type: spec +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] +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 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* +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. 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. 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. 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 — 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 +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) 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 + +- [ ] **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 **and `doctrine.inject`**, which carries + 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 + 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 + 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. +- [ ] **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 + +- 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..a4256c75 --- /dev/null +++ b/specs/HARNESS-072-pr-stewardship/tasks.md @@ -0,0 +1,59 @@ +--- +tags: [spec, tasks, templates] +created: "2026-08-15" +--- + +# Tasks - HARNESS-072-pr-stewardship + +> 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 + +- [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 + +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 + +- [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 + +`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. + +**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 new file mode 100644 index 00000000..d144ebbe --- /dev/null +++ b/specs/HARNESS-072-pr-stewardship/verification.md @@ -0,0 +1,87 @@ +--- +tags: [spec, verification, templates] +created: "2026-08-15" +--- + +# Verification - HARNESS-072-pr-stewardship + +## Evidence + +| 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 + +- `~/.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. + +**Red observed before green, on real state and not a fixture:** + +``` +$ ./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 +``` + +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. + +## 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 + +- [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 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 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