diff options
| author | Craig Jennings <c@cjennings.net> | 2026-07-28 21:28:29 -0500 |
|---|---|---|
| committer | Craig Jennings <c@cjennings.net> | 2026-07-28 21:28:29 -0500 |
| commit | 80624606b8a60d97d8c7600882ddc35f5225469f (patch) | |
| tree | 4c4e56089c1293ab75863fe50bafefcf5084565e | |
| parent | 3a933a26c12b6350fa46faf89bf86e99bfab0f30 (diff) | |
| download | rulesets-80624606b8a60d97d8c7600882ddc35f5225469f.tar.gz rulesets-80624606b8a60d97d8c7600882ddc35f5225469f.zip | |
feat(publish): isolate the pre-commit review and loop it to approval
The review already ran before every commit. It ran in the context that wrote the diff, which is the part that didn't work. A self-review checks the change against the author's own model of it and can't check the model, so the errors that survive are the ones that were never in the diff: a scope inherited from whoever reported the problem, a blast radius estimated instead of measured.
So the reviewer is now a subagent, on every commit including a one-line one. It gets the diff, a one-line claim of what the change does, and the ticket where one exists. It doesn't get the conversation or my reasoning, because those are the model it exists not to hold. The ticket stays in for the opposite reason: it was written before the work and is the only thing that can contradict my claim about my own diff.
Findings loop back to the same reviewer until it approves, rather than to a fresh one that can't tell an addressed finding from one that never existed. Three rounds, or a finding that recurs after being reported fixed, and it stops for a human instead. The unattended callers park the task rather than wait for an answer nobody is awake to give.
I gave the stance a substantiation floor. An agent told to attack will manufacture findings to satisfy the instruction, and a manufactured finding costs a round and teaches the author to discount the next review.
subagents.md said three separate times not to dispatch work this small, so it now carries an Isolation Override: the size heuristics assume the main thread could do the job equally well, and they lapse when its own context is what makes its answer untrustworthy. Field 2 of the Prompt Contract inverts there. Paste your context in and you hand over the very model you spawned the agent to escape.
I wrote it by running it on itself: three rounds and thirteen findings. Two were things I'd have shipped. Withholding the ticket made my own claim self-certifying, and the override reaffirmed the contract field that would have destroyed the isolation.
| -rw-r--r-- | .ai/workflows/no-approvals.org | 4 | ||||
| -rw-r--r-- | .ai/workflows/sentry.org | 2 | ||||
| -rw-r--r-- | .ai/workflows/work-the-backlog.org | 5 | ||||
| -rw-r--r-- | claude-rules/subagents.md | 72 | ||||
| -rw-r--r-- | claude-templates/.ai/workflows/no-approvals.org | 4 | ||||
| -rw-r--r-- | claude-templates/.ai/workflows/sentry.org | 2 | ||||
| -rw-r--r-- | claude-templates/.ai/workflows/work-the-backlog.org | 5 | ||||
| -rw-r--r-- | publish/SKILL.md | 164 | ||||
| -rw-r--r-- | review-code/SKILL.md | 28 | ||||
| -rw-r--r-- | todo.org | 31 |
10 files changed, 279 insertions, 38 deletions
diff --git a/.ai/workflows/no-approvals.org b/.ai/workflows/no-approvals.org index b4c7fcf..6b5c7fa 100644 --- a/.ai/workflows/no-approvals.org +++ b/.ai/workflows/no-approvals.org @@ -46,7 +46,7 @@ The interaction gates that step the workflow back to Craig for an "OK to proceed The engineering-discipline gates protect quality, not Craig's interaction time. They remain in force: -- =/review-code= against the staged diff before every commit. Critical and Important findings still block. Minor findings still surface. No "proceed anyway" override unless Craig has given it explicitly for this batch. +- =/review-code= against the staged diff before every commit, dispatched as an isolated adversarial reviewer per the =publish= skill's Step 1 — no-approvals removes *interaction* gates, never the isolation. Critical and Important findings still block, and the re-review loop still runs to approval. Minor findings still surface. No "proceed anyway" override unless Craig has given it explicitly for this batch. If the review can't reach approval — three rounds, a recurring finding, or a =Needs Discussion= verdict — that is a genuine question: park the item per step 4 and move to the next one rather than committing past a standing finding. - =/voice personal= on every publish artifact (commit messages, PR titles + bodies, PR review comments). The full pattern walk happens. The printed result just doesn't wait for approval. - The full test suite + lint + compile before commit (per =verification.md=). - Fetch-and-reconcile in the =publish= skill, Step 0. @@ -70,7 +70,7 @@ For each item: - Do the work. - Update the Session Log per the rules in =protocols.org=. -- Before any commit: run =/review-code= against the staged diff. Surface Critical and Important findings inline; fix them and re-review until clean. Minor findings show but don't block. +- Before any commit: dispatch the isolated adversarial reviewer per the =publish= skill's Step 1 — never review your own staged diff inline. Surface Critical and Important findings; fix them and send the updated diff back to the *same* reviewer until it approves. Minor findings show but don't block and never earn another round. If the review can't reach approval — three rounds, a finding that recurs after being reported fixed, or a =Needs Discussion= verdict — park the item per step 4 with the standing findings and move on; don't commit past a blocking finding. - Draft the commit message. Run =/voice personal= (the skill, or walk the patterns inline if unavailable). Print the final message inline before committing so the log shows it. - Commit and push. - One-line status between items ("Task X done, on to Y.") so Craig knows what's happening when he checks back in. diff --git a/.ai/workflows/sentry.org b/.ai/workflows/sentry.org index 0acb73c..b25fc14 100644 --- a/.ai/workflows/sentry.org +++ b/.ai/workflows/sentry.org @@ -153,7 +153,7 @@ In order. Each names its detection probe. A pass whose probe fails is one skip l 11. *Bug and refactor finding* — hunt for real bugs and worthwhile refactoring opportunities in the project's codebase: static analysis (=shellcheck= for shell, the project's own linters for its languages), config sanity checks, plus one targeted code-reading area per cycle. Rotate the area across cycles and name it in the digest, so coverage accumulates over a night instead of re-reading the same corner. Randomized property sweeps (generate-and-verify against an engine's own invariants) are good quiet-cycle work here, reaching past a frozen test corpus. Expect the pass to go honestly quiet after the first few cycles find the standing defects; a quiet hunt is a result, not a failure. (takuzu, first live run 2026-07-21: three real fixes in the first four cycles, then quiet.) This pass does *not* run the test suite — the entry baseline already ran it, and re-running it hourly is anti-pattern 5; read the entry result instead. Probe: the project carries a codebase — source under version control beyond its org and tooling files. File each verified bug as a graded task in =todo.org= per the severity × frequency matrix (=todo-format.md=), and each refactoring opportunity as a =:refactor:= task, deduped against existing tasks; an unverifiable suspicion is a digest line, not a task. *Find, never fix in this pass* — the finding files a task and stops. A fix happens only in the opt-in implementation pass below, and only after the finding is a filed task that pass then re-verifies from scratch (see the premise rule there). A freshly-found "bug" can be a misread — one was filed and retracted two cycles apart on 2026-07-23 — so the file-then-verify-then-fix pipeline is deliberate: the task is the checkpoint, not a same-breath fix. (Added at Craig's order 2026-07-21, first dogfooded in dotfiles; refactor-finding added 2026-07-24.) -12. *Solo-task implementation (opt-in — =:SENTRY_MAY_IMPLEMENT:=)* — work the backlog's solo, decision-free tasks on the branch. Probe: =.ai/notes.org= Workflow State carries =:SENTRY_MAY_IMPLEMENT: yes= *and* the project holds =:COMMIT_AUTONOMY:= (the implement pass commits). Absent the marker, skip — this pass is off by default, because it turns the morning from a two-minute merge into a code review, and that's the project owner's call. When on: invoke =work-the-backlog.org= under its unattended-loop contract (no pre-flight Q&A — there's no Craig overnight), eligibility =TODO= + =:solo:=, with the defer checklist deciding act-vs-file. The overnight-only tightening: only the *ready* bucket implements (clears every checklist item with zero open decisions); a task needing even one quick decision defers to a =VERIFY= rather than guessing, exactly as the loop caller already does. Commit each logical change to the sentry branch; *never push* — the morning review and merge is the gate, same as every other pass. The full quality bar holds (TDD, suite green before each commit, =/review-code=, =/voice=), and =/review-code= here runs the *premise check first*: reproduce the bug or confirm the problem is real before judging the diff. The review is the fact-checker that a filed claim never got, and it is what makes fixing-on-a-branch safe (Craig, 2026-07-24). A task that fails its premise check is not implemented — the finding was wrong, and that outcome is a digest line, not a commit. (Added at Craig's direction 2026-07-24: overnight implement-on-branch, gated and never-pushed.) +12. *Solo-task implementation (opt-in — =:SENTRY_MAY_IMPLEMENT:=)* — work the backlog's solo, decision-free tasks on the branch. Probe: =.ai/notes.org= Workflow State carries =:SENTRY_MAY_IMPLEMENT: yes= *and* the project holds =:COMMIT_AUTONOMY:= (the implement pass commits). Absent the marker, skip — this pass is off by default, because it turns the morning from a two-minute merge into a code review, and that's the project owner's call. When on: invoke =work-the-backlog.org= under its unattended-loop contract (no pre-flight Q&A — there's no Craig overnight), eligibility =TODO= + =:solo:=, with the defer checklist deciding act-vs-file. The overnight-only tightening: only the *ready* bucket implements (clears every checklist item with zero open decisions); a task needing even one quick decision defers to a =VERIFY= rather than guessing, exactly as the loop caller already does. Commit each logical change to the sentry branch; *never push* — the morning review and merge is the gate, same as every other pass. The full quality bar holds (TDD, suite green before each commit, the isolated adversarial review per =publish= Step 1 with its re-review loop, =/voice=), and the review here runs the *premise check first*: reproduce the bug or confirm the problem is real before judging the diff. The review is the fact-checker that a filed claim never got, and it is what makes fixing-on-a-branch safe (Craig, 2026-07-24). A task that fails its premise check is not implemented — the finding was wrong, and that outcome is a digest line, not a commit. A task whose review never reaches approval — three rounds, a recurring finding, or a =Needs Discussion= verdict — is the same shape: no commit, and a digest line naming the standing findings, so the morning review sees what the reviewer would not pass rather than finding the task silently absent. (Added at Craig's direction 2026-07-24: overnight implement-on-branch, gated and never-pushed.) (KB lesson promotion — the pass the original proposal listed eleventh — is deferred to vNext. An unattended judgment pass writing to the shared knowledge base waits until sentry has quiet weeks behind it and a designed detection heuristic. See the filed lesson-detection-heuristic task.) diff --git a/.ai/workflows/work-the-backlog.org b/.ai/workflows/work-the-backlog.org index a0b24a8..ea3f402 100644 --- a/.ai/workflows/work-the-backlog.org +++ b/.ai/workflows/work-the-backlog.org @@ -54,7 +54,7 @@ For the task set, in order, until the run cap is hit: 1. *Eligibility gate* (below). Ineligible → record =skipped-ineligible=, next task. 2. *Scope read* of the relevant code. Cheap; just enough to run the defer checklist. 3. *Defer checklist* (below). Any hit → defer: file the =VERIFY= naming the gap and record =deferred-VERIFY= (or, under the speedrun preset, route a quick-question gap to the pre-flight Q&A), next task. -4. *Implement* under the project's commit discipline: TDD red→green→refactor, then =/review-code --staged=, fix all Critical/Important findings, then close the task per =todo-format.md='s completion rules. Decompose into as many logical commits as the change needs — size is not capped. If implementation fails partway, leave the tree working, record =failed=, surface it, and continue to the next task. +4. *Implement* under the project's commit discipline: TDD red→green→refactor, then the isolated adversarial review (=publish= Step 1) with its re-review loop, fix all Critical/Important findings, then close the task per =todo-format.md='s completion rules. Decompose into as many logical commits as the change needs — size is not capped. If implementation fails partway, leave the tree working, record =failed=, surface it, and continue to the next task. 5. *Commit autonomy branch:* - =file-only= → surface the diff, do *not* commit. Record =implemented-diff-surfaced=. - =autonomous-commit= → =/voice personal= on the message, commit individually, push per the project's flow. Record =implemented-committed=. @@ -110,7 +110,8 @@ Autonomy changes who approves, not what quality means. Per task, non-negotiable: - *TDD* per =testing.md=: red first, green, refactor. The keystone checklist item already proved the failing test is writable. - *Verification* per =verification.md=: fresh evidence, full suite green before any commit. -- *=/review-code --staged=* before every commit; Critical and Important findings block until fixed. +- *Isolated adversarial review* before every commit, dispatched per the =publish= skill's Step 1 — never an inline self-review, however small the diff. Critical and Important findings block until fixed, and each fix goes back to the *same* reviewer until it approves. Minor findings never earn another round. + - *When the review can't reach approval* — three rounds without it, a finding that recurs after being reported fixed, or a =Needs Discussion= verdict — the unattended run has no one to ask. Record the task =failed= with the standing findings in its result, leave the tree working, and continue to the next task. Never commit past a blocking finding because nobody is awake to adjudicate — an unreviewed commit landing overnight is the outcome this gate exists to prevent. - *=/voice personal=* on every commit message on the =autonomous-commit= path (or the patterns walked inline if the skill is unavailable), message printed inline so the log shows what landed. - *Task closure* per =todo-format.md=: depth-based completion (keyword + =CLOSED:= at level 2, dated rewrite at level 3+). - *One logical change per commit.* A large task becomes several commits, not one omnibus. diff --git a/claude-rules/subagents.md b/claude-rules/subagents.md index 8578dea..e52d906 100644 --- a/claude-rules/subagents.md +++ b/claude-rules/subagents.md @@ -34,6 +34,66 @@ This is the same boundary the "Don't Subagent At All" section and the "Subagenting trivial work" anti-pattern draw; treat it as an explicit gate at dispatch time. +Every size-based rule in this file — the cost gate here, "Don't Subagent At +All", the trivial-work anti-pattern — is subject to the isolation override +below. + +## Isolation Override — When Size Doesn't Gate + +Every size heuristic in this file rests on one assumption: that the main +thread could do the task itself just as well, so the only question is +whether delegating is worth the overhead. When that assumption fails, the +heuristics don't apply, and a five-line task can require a subagent that a +five-hundred-line one wouldn't. + +The assumption fails whenever **the main thread is structurally disqualified +from the task** — not slower at it, disqualified. The test: would the main +thread's own context make its answer *less* trustworthy? If holding the +context is what corrupts the judgment, then doing it inline doesn't save the +overhead, it destroys the result. The isolation *is* the deliverable, and +"it's only a small diff" is not an argument against it. + +**The standing instance is the pre-commit code review** (`publish` skill, +Step 1). The author cannot review their own change, because a self-review +checks the diff against the author's own model of it and cannot check the +model. Errors that survive a self-review are the ones that were never in the +diff — an inherited scope, an estimated blast radius, a fix correct for the +case in mind and wrong for the one never considered. So that review is +dispatched on *every* commit including a one-line one, and the ~10-tool-call +floor, the single-function rule, and the trivial-work anti-pattern are all +overridden there by design. + +Other cases with the same shape: verifying a claim the main thread already +committed to in conversation, and any second opinion where the first opinion +is already in context. If you find yourself reasoning "I already know the +answer, so a subagent is wasteful," check whether already knowing it is the +problem. + +This override widens *what* gets dispatched. Scope, constraints, and output +format are still required, and arguably matter more here, since an isolated +agent can't fall back on shared context to fill a gap. + +**Field 2 of the Prompt Contract inverts under this override, and the +inversion is the whole point.** Normally field 2 says to paste the relevant +output verbatim and include what you learned in earlier turns. Do that for an +isolation dispatch and you hand over the very model you spawned the agent to +escape — a reviewer given your findings reviews your findings. So for an +isolation dispatch, field 2 is *the artifact under test and the independent +record of what was asked, and nothing else*: the diff, a one-line claim of +what it does, and the ticket or plan where one exists. The conversation, the +rationale, and the dead ends are withheld on purpose. + +Keep the requirement source in. A ticket is not your model of the change; it +was written before the work, usually by someone else, and it is the only +thing that can contradict your claim about your own diff. + +**The output is a judgment, so the review gate resolves differently.** The +Review-Gate Cadence below says subagent output is a claim to be verified +before moving on, which is right when the deliverable is *work*. When the +deliverable is *a judgment about your work*, verifying it against your own +reading reinstates exactly the bias the dispatch removed. Disagreement goes +to the user to adjudicate, not back to the author's own judgment. + ## When to Spawn a Subagent ### Parallel-safe (spawn multiple in parallel) @@ -63,11 +123,15 @@ at dispatch time. ### Don't Subagent At All +Unless the Isolation Override applies — these are efficiency rules, and they +lapse when the main thread's own context is what makes its answer untrustworthy. + - **The target is already known** and the work fits in under ~10 tool calls. - **Single-function logic** — one Read + one Edit is faster than briefing an agent. - **You can see the answer from context** — don't spawn a researcher for - something already on screen. + something already on screen. (The inverse of this one is the override's + clearest case: when *having* seen it is the disqualification, dispatch.) ## Prompt Contract @@ -138,7 +202,11 @@ fix), then dispatch the fix with a specific contract. - **Retrying a failed subagent task in the orchestrator** — pollutes context. Dispatch a fix agent instead. - **Subagenting trivial work** — one Read + one Edit doesn't need an - agent; spawn overhead exceeds benefit. + agent; spawn overhead exceeds benefit. Except under the Isolation + Override, where a one-line diff still gets its own reviewer. +- **Reviewing your own change inline** — the mirror-image failure, and the + more expensive one. Skipping a dispatch to save overhead on a small diff + costs a review that could only have come from outside your context. - **Skipping review between tasks** — compounding bugs are much harder to unwind than any single bug. - **Letting the agent decide scope** — "figure out what needs changing" diff --git a/claude-templates/.ai/workflows/no-approvals.org b/claude-templates/.ai/workflows/no-approvals.org index b4c7fcf..6b5c7fa 100644 --- a/claude-templates/.ai/workflows/no-approvals.org +++ b/claude-templates/.ai/workflows/no-approvals.org @@ -46,7 +46,7 @@ The interaction gates that step the workflow back to Craig for an "OK to proceed The engineering-discipline gates protect quality, not Craig's interaction time. They remain in force: -- =/review-code= against the staged diff before every commit. Critical and Important findings still block. Minor findings still surface. No "proceed anyway" override unless Craig has given it explicitly for this batch. +- =/review-code= against the staged diff before every commit, dispatched as an isolated adversarial reviewer per the =publish= skill's Step 1 — no-approvals removes *interaction* gates, never the isolation. Critical and Important findings still block, and the re-review loop still runs to approval. Minor findings still surface. No "proceed anyway" override unless Craig has given it explicitly for this batch. If the review can't reach approval — three rounds, a recurring finding, or a =Needs Discussion= verdict — that is a genuine question: park the item per step 4 and move to the next one rather than committing past a standing finding. - =/voice personal= on every publish artifact (commit messages, PR titles + bodies, PR review comments). The full pattern walk happens. The printed result just doesn't wait for approval. - The full test suite + lint + compile before commit (per =verification.md=). - Fetch-and-reconcile in the =publish= skill, Step 0. @@ -70,7 +70,7 @@ For each item: - Do the work. - Update the Session Log per the rules in =protocols.org=. -- Before any commit: run =/review-code= against the staged diff. Surface Critical and Important findings inline; fix them and re-review until clean. Minor findings show but don't block. +- Before any commit: dispatch the isolated adversarial reviewer per the =publish= skill's Step 1 — never review your own staged diff inline. Surface Critical and Important findings; fix them and send the updated diff back to the *same* reviewer until it approves. Minor findings show but don't block and never earn another round. If the review can't reach approval — three rounds, a finding that recurs after being reported fixed, or a =Needs Discussion= verdict — park the item per step 4 with the standing findings and move on; don't commit past a blocking finding. - Draft the commit message. Run =/voice personal= (the skill, or walk the patterns inline if unavailable). Print the final message inline before committing so the log shows it. - Commit and push. - One-line status between items ("Task X done, on to Y.") so Craig knows what's happening when he checks back in. diff --git a/claude-templates/.ai/workflows/sentry.org b/claude-templates/.ai/workflows/sentry.org index 0acb73c..b25fc14 100644 --- a/claude-templates/.ai/workflows/sentry.org +++ b/claude-templates/.ai/workflows/sentry.org @@ -153,7 +153,7 @@ In order. Each names its detection probe. A pass whose probe fails is one skip l 11. *Bug and refactor finding* — hunt for real bugs and worthwhile refactoring opportunities in the project's codebase: static analysis (=shellcheck= for shell, the project's own linters for its languages), config sanity checks, plus one targeted code-reading area per cycle. Rotate the area across cycles and name it in the digest, so coverage accumulates over a night instead of re-reading the same corner. Randomized property sweeps (generate-and-verify against an engine's own invariants) are good quiet-cycle work here, reaching past a frozen test corpus. Expect the pass to go honestly quiet after the first few cycles find the standing defects; a quiet hunt is a result, not a failure. (takuzu, first live run 2026-07-21: three real fixes in the first four cycles, then quiet.) This pass does *not* run the test suite — the entry baseline already ran it, and re-running it hourly is anti-pattern 5; read the entry result instead. Probe: the project carries a codebase — source under version control beyond its org and tooling files. File each verified bug as a graded task in =todo.org= per the severity × frequency matrix (=todo-format.md=), and each refactoring opportunity as a =:refactor:= task, deduped against existing tasks; an unverifiable suspicion is a digest line, not a task. *Find, never fix in this pass* — the finding files a task and stops. A fix happens only in the opt-in implementation pass below, and only after the finding is a filed task that pass then re-verifies from scratch (see the premise rule there). A freshly-found "bug" can be a misread — one was filed and retracted two cycles apart on 2026-07-23 — so the file-then-verify-then-fix pipeline is deliberate: the task is the checkpoint, not a same-breath fix. (Added at Craig's order 2026-07-21, first dogfooded in dotfiles; refactor-finding added 2026-07-24.) -12. *Solo-task implementation (opt-in — =:SENTRY_MAY_IMPLEMENT:=)* — work the backlog's solo, decision-free tasks on the branch. Probe: =.ai/notes.org= Workflow State carries =:SENTRY_MAY_IMPLEMENT: yes= *and* the project holds =:COMMIT_AUTONOMY:= (the implement pass commits). Absent the marker, skip — this pass is off by default, because it turns the morning from a two-minute merge into a code review, and that's the project owner's call. When on: invoke =work-the-backlog.org= under its unattended-loop contract (no pre-flight Q&A — there's no Craig overnight), eligibility =TODO= + =:solo:=, with the defer checklist deciding act-vs-file. The overnight-only tightening: only the *ready* bucket implements (clears every checklist item with zero open decisions); a task needing even one quick decision defers to a =VERIFY= rather than guessing, exactly as the loop caller already does. Commit each logical change to the sentry branch; *never push* — the morning review and merge is the gate, same as every other pass. The full quality bar holds (TDD, suite green before each commit, =/review-code=, =/voice=), and =/review-code= here runs the *premise check first*: reproduce the bug or confirm the problem is real before judging the diff. The review is the fact-checker that a filed claim never got, and it is what makes fixing-on-a-branch safe (Craig, 2026-07-24). A task that fails its premise check is not implemented — the finding was wrong, and that outcome is a digest line, not a commit. (Added at Craig's direction 2026-07-24: overnight implement-on-branch, gated and never-pushed.) +12. *Solo-task implementation (opt-in — =:SENTRY_MAY_IMPLEMENT:=)* — work the backlog's solo, decision-free tasks on the branch. Probe: =.ai/notes.org= Workflow State carries =:SENTRY_MAY_IMPLEMENT: yes= *and* the project holds =:COMMIT_AUTONOMY:= (the implement pass commits). Absent the marker, skip — this pass is off by default, because it turns the morning from a two-minute merge into a code review, and that's the project owner's call. When on: invoke =work-the-backlog.org= under its unattended-loop contract (no pre-flight Q&A — there's no Craig overnight), eligibility =TODO= + =:solo:=, with the defer checklist deciding act-vs-file. The overnight-only tightening: only the *ready* bucket implements (clears every checklist item with zero open decisions); a task needing even one quick decision defers to a =VERIFY= rather than guessing, exactly as the loop caller already does. Commit each logical change to the sentry branch; *never push* — the morning review and merge is the gate, same as every other pass. The full quality bar holds (TDD, suite green before each commit, the isolated adversarial review per =publish= Step 1 with its re-review loop, =/voice=), and the review here runs the *premise check first*: reproduce the bug or confirm the problem is real before judging the diff. The review is the fact-checker that a filed claim never got, and it is what makes fixing-on-a-branch safe (Craig, 2026-07-24). A task that fails its premise check is not implemented — the finding was wrong, and that outcome is a digest line, not a commit. A task whose review never reaches approval — three rounds, a recurring finding, or a =Needs Discussion= verdict — is the same shape: no commit, and a digest line naming the standing findings, so the morning review sees what the reviewer would not pass rather than finding the task silently absent. (Added at Craig's direction 2026-07-24: overnight implement-on-branch, gated and never-pushed.) (KB lesson promotion — the pass the original proposal listed eleventh — is deferred to vNext. An unattended judgment pass writing to the shared knowledge base waits until sentry has quiet weeks behind it and a designed detection heuristic. See the filed lesson-detection-heuristic task.) diff --git a/claude-templates/.ai/workflows/work-the-backlog.org b/claude-templates/.ai/workflows/work-the-backlog.org index a0b24a8..ea3f402 100644 --- a/claude-templates/.ai/workflows/work-the-backlog.org +++ b/claude-templates/.ai/workflows/work-the-backlog.org @@ -54,7 +54,7 @@ For the task set, in order, until the run cap is hit: 1. *Eligibility gate* (below). Ineligible → record =skipped-ineligible=, next task. 2. *Scope read* of the relevant code. Cheap; just enough to run the defer checklist. 3. *Defer checklist* (below). Any hit → defer: file the =VERIFY= naming the gap and record =deferred-VERIFY= (or, under the speedrun preset, route a quick-question gap to the pre-flight Q&A), next task. -4. *Implement* under the project's commit discipline: TDD red→green→refactor, then =/review-code --staged=, fix all Critical/Important findings, then close the task per =todo-format.md='s completion rules. Decompose into as many logical commits as the change needs — size is not capped. If implementation fails partway, leave the tree working, record =failed=, surface it, and continue to the next task. +4. *Implement* under the project's commit discipline: TDD red→green→refactor, then the isolated adversarial review (=publish= Step 1) with its re-review loop, fix all Critical/Important findings, then close the task per =todo-format.md='s completion rules. Decompose into as many logical commits as the change needs — size is not capped. If implementation fails partway, leave the tree working, record =failed=, surface it, and continue to the next task. 5. *Commit autonomy branch:* - =file-only= → surface the diff, do *not* commit. Record =implemented-diff-surfaced=. - =autonomous-commit= → =/voice personal= on the message, commit individually, push per the project's flow. Record =implemented-committed=. @@ -110,7 +110,8 @@ Autonomy changes who approves, not what quality means. Per task, non-negotiable: - *TDD* per =testing.md=: red first, green, refactor. The keystone checklist item already proved the failing test is writable. - *Verification* per =verification.md=: fresh evidence, full suite green before any commit. -- *=/review-code --staged=* before every commit; Critical and Important findings block until fixed. +- *Isolated adversarial review* before every commit, dispatched per the =publish= skill's Step 1 — never an inline self-review, however small the diff. Critical and Important findings block until fixed, and each fix goes back to the *same* reviewer until it approves. Minor findings never earn another round. + - *When the review can't reach approval* — three rounds without it, a finding that recurs after being reported fixed, or a =Needs Discussion= verdict — the unattended run has no one to ask. Record the task =failed= with the standing findings in its result, leave the tree working, and continue to the next task. Never commit past a blocking finding because nobody is awake to adjudicate — an unreviewed commit landing overnight is the outcome this gate exists to prevent. - *=/voice personal=* on every commit message on the =autonomous-commit= path (or the patterns walked inline if the skill is unavailable), message printed inline so the log shows what landed. - *Task closure* per =todo-format.md=: depth-based completion (keyword + =CLOSED:= at level 2, dated rewrite at level 3+). - *One logical change per commit.* A large task becomes several commits, not one omnibus. diff --git a/publish/SKILL.md b/publish/SKILL.md index a93f3dd..d2b906c 100644 --- a/publish/SKILL.md +++ b/publish/SKILL.md @@ -222,28 +222,142 @@ Step 0. Upstream can advance during a long session, especially across machines or with teammates pushing in parallel. Run Step 0 every time the publish flow starts. -### Step 1: local code review (mandatory) - -Run the `review-code` skill against the change: - -- Before a commit: `/review-code --staged` -- Before a PR: `/review-code` (branch diff against `main` merge-base) -- Before commenting on someone else's PR: `/review-code <PR#>` - -Surface **all** findings to the user: Critical, Important, and Minor. - -**Default block:** any Critical or Important finding stops the flow. Fix the -issues and re-run `/review-code` until the diff is clean. Minor findings are -shown but do not block. - -**Override:** the user can bypass the block with an explicit "proceed anyway" -(or equivalent wording). Without the explicit override, do not proceed to -Step 2. - -The `review-code` skill already has a Phase 0 eligibility gate that handles -trivial and ineligible diffs (whitespace-only, revert with obvious -justification, already-reviewed SHA). Trust that gate; there is no "trivial -enough to skip review" exemption on top of it. +### Step 1: adversarial review by an isolated reviewer (mandatory) + +The review runs in a **subagent**, never inline, and it runs on **every** +commit. The author does not review their own work. + +**Why isolation, not just review.** A self-review checks the diff against the +author's own model of what the diff should do. It cannot check the model. The +errors that survive a self-review are the ones that were never visible in the +diff — a scope inherited from whoever reported the problem, a blast radius +estimated instead of measured, a fix that is correct for the case the author +had in mind and wrong for the one they never considered. Only a reviewer that +does not hold the author's model catches those, so the isolation is the point +and the adversarial stance is the method. + +**Dispatch contract.** Spawn the reviewer via the Agent tool and give it these +three things, the third whenever one exists: + +1. **The diff** — `git diff --cached` for a commit, the branch diff for a PR. +2. **The claim** — one line from the author stating what the change does. Write + it before dispatching. This is the thing under test: the reviewer's job is + to check the diff against the claim. +3. **The requirement source, when one exists** — the ticket, plan, ADR, or task + body the work was done against. Pass it verbatim. + +Withhold everything else: the conversation, the exploration, the dead ends, and +above all the author's reasoning for why the change is right. Those are what +transmit the author's model, which is what the reviewer exists to not have. A +reviewer given the rationale reviews the rationale. + +**Why the requirement source is not withheld.** A ticket or plan is not the +author's model of the change — it is the independent record of what was asked, +written before the work and usually by someone else. It is the only artifact +that can contradict the author's one-line claim. Withhold it and the claim +becomes self-certifying: the reviewer checks the diff against a sentence the +author wrote, which cannot surface scope creep or a missing requirement. That +also strands `review-code`'s Intent-vs-Delivery criterion, which is skipped +outright when no intent context is supplied and is the one criterion aimed at +the inherited-scope error this whole gate exists to catch. + +Invoke the review with `/review-code --staged` (commit), `/review-code` (branch +diff against the `main` merge-base), or `/review-code <PR#>` (someone else's +PR), and tell it to run its adversarial pass. + +**Adversarial, with substantiation.** The reviewer is prompted to *refute* the +change rather than to bless it. But an agent told to attack will manufacture +findings to satisfy the instruction, so the stance carries a floor: a finding +that cannot be substantiated against the diff is not a finding and must be +dropped. `review-code`'s confidence filter and false-positive filter are what +enforce that floor — adversarial raises the appetite for looking, never the +tolerance for a weak claim. + +**Scope is every commit; the reviewer decides triviality, not the author.** +There is no "trivial enough to skip" exemption. A floor written in terms of +"small" or "mechanical" puts the judgment back with the author, whose judgment +is the thing being checked. Dispatch always, and let `review-code`'s own Phase 0 +eligibility gate return fast on a whitespace-only diff, an obvious revert, or an +already-reviewed SHA. A cheap spawn on a trivial commit is the price of the +author never getting to rule on their own diff. + +**Verdict, and the re-review loop.** The reviewer returns one of four outcomes, +and all four are defined exits: + +- **Approve** — the gate is satisfied. Proceed to Step 2. +- **Skipped** — `review-code`'s Phase 0 found the diff ineligible (whitespace + only, an obvious revert, an already-reviewed SHA). This **satisfies the gate** + and the flow proceeds. A skip is a reviewer's ruling, which is the point; what + is forbidden is the *author* ruling their own diff too trivial to look at. +- **Request Changes** — blocking findings stand. Enter the loop below. +- **Needs Discussion** — the reviewer has a disagreement it cannot settle from + the diff: an architectural objection, a question about whether the change + should exist at all. This **stops immediately and goes to the user**; it does + not enter the loop. Routing it to the loop would answer "should we do this?" + with "fix these findings," which is the wrong question and burns rounds on a + disagreement no amount of editing resolves. Unattended callers park it exactly + as they park a bound hit. + +Approval is the reviewer's to give; the author never declares their own change +clean. + +That set is closed. `review-code` emits Approve, Request Changes, or Needs +Discussion, and its Phase 0 emits Skipped; every one has a defined exit above. A +verdict outside those four means the reviewer went off-contract — surface it +rather than interpreting it. + +**The loop turns on blocking findings, not on the verdict token.** It ends when +no Critical or Important finding stands. A Minor-only result is not grounds for +another round: fix it or don't, but do not spend a round on it, and do not +escalate to the user over one. A reviewer holding only Minor findings should +return Approve and say what it left. + +On Request Changes: + +1. Surface **all** findings — Critical, Important, Minor. Critical and Important + block; Minor is shown and does not block. +2. Fix the blocking findings. +3. **Re-review, and keep re-reviewing until the reviewer approves.** A fix is a + new change and gets the same scrutiny as the original. Fixing under review + pressure is exactly when a regression gets introduced, so an unreviewed fix + is the hole this loop closes. + +**Continue the same reviewer, don't spawn a fresh one.** Send the updated diff +back to the existing reviewer (`SendMessage` with its agent ID). It holds its own +findings, so it can confirm each one is actually addressed. A fresh reviewer each +round cannot tell "addressed" from "never existed", re-litigates settled points, +and drifts to a new set of findings every round, which never converges. + +The continued reviewer must **re-verify each finding against the new diff**, not +against the author's description of the fix. "I fixed it" is a claim, and taking +it at face value is how a review round becomes a rubber stamp. + +**Bounds, so the loop terminates.** Two conditions end it early and hand the +decision to the user: + +- **Three rounds without approval** (the initial review plus two re-reviews). + This matches the two-fix-attempts limit in `subagents.md`: past that, the + problem is usually the approach rather than the diff. +- **A finding recurs after being reported fixed.** That is oscillation — the + fix for one finding reintroducing another — and another round will not + resolve it. Stop on the first recurrence rather than spending the remaining + rounds. + +In both cases, stop and surface: the standing findings, what was tried, and the +decision needed. + +**Override.** The user can bypass the block with an explicit "proceed anyway" (or +equivalent). The user is also the adjudicator when the author believes a finding +is wrong: say so with the reasoning and let the user rule. Do not resolve a +disagreement with the reviewer by overruling it silently. Without an explicit +override, do not proceed to Step 2. + +**When the Agent tool is unavailable.** Per `subagents.md`, don't block: run the +review in the main thread, but hold it to the same contract — review against the +stated claim, refute rather than bless, substantiate every finding, loop on fixes +until clean, same bounds. State plainly that the review was not isolated, because +a self-review under an adversarial prompt is weaker evidence and the user should +know which one they got. ### Step 2: draft, review, publish @@ -307,8 +421,10 @@ this file — load it when the artifact is a PR. **Exception:** trivial one-liners the user dictated verbatim in the conversation (e.g. "commit this as `chore: bump version`", "reply just 'thanks for the review'") can skip the draft-file step in Step 2. -`/review-code` in Step 1 still runs when it applies; Phase 0 of that skill -handles trivial diffs, and acknowledgment-only replies don't need it at all. +Step 1's review still runs on every commit — this carve-out is about the +draft-file step, not the review. Phase 0 is what rules a trivial diff out, and +its Skipped result satisfies the gate. An acknowledgment-only PR reply commits +nothing, so there is no diff to review. **Single-skill gate.** Each of the three subflows above runs `/voice personal` before printing the draft — the full pattern walk covering AI-writing signs, universal good-writing rules, Craig's voice patterns, and the artifact-mechanics patterns (first-person rewrite, public-artifact scope flag, praise/correction asymmetry, finding stems). Publish artifacts (commits, PR titles + bodies, PR review comments) always use personal mode; the `.ai/`-tracking check at the top of Step 2 decides only whether the approval gate fires, not which patterns run. Running the skill is mandatory; the printed draft must have been through it. When the user asks mid-flow for "the voice pass" on an in-progress draft, that means re-run the full pattern walk — not a subset. Always state that the skill ran when announcing the printed draft (e.g. "/voice personal — full pattern walk"). Skipping the pass without flagging it is a defect. The terse/omit-needless-words cut (pattern #38) is the *last* thing the skill does before the draft is printed: read each sentence and cut it in half, keeping only what changes meaning. The draft the user first sees must already be terse — if they have to ask for an Orwell pass after seeing it, the pass was skipped. diff --git a/review-code/SKILL.md b/review-code/SKILL.md index 05bc6af..559cee8 100644 --- a/review-code/SKILL.md +++ b/review-code/SKILL.md @@ -33,7 +33,31 @@ When intent context is given, the review grades "does this match what was asked? ## Execution Model -For substantive reviews on large diffs: **dispatch the perspective passes as parallel sub-agents** via the Agent tool. Each sub-agent starts with a clean context window — the reviewer shouldn't inherit the implementer's mental model. For small single-commit tweaks, run inline. +Two levels of dispatch, and conflating them is the mistake to avoid. + +**Level one — who runs this skill.** When invoked from the `publish` flow's Step 1 gate, this skill is already running inside an isolated reviewer subagent that the publish flow spawned. That isolation is not optional and not this skill's call: the author never reviews their own change. If you are reading this in the same context that wrote the diff, the publish flow was not followed. + +**Level two — how the perspectives run inside it.** For substantive reviews on large diffs, dispatch the Phase 2 perspective passes as parallel sub-agents. For small single-commit tweaks, run them inline. This is a cost decision about fan-out *within* the review and never a licence to skip level one. + +### The adversarial contract (publish-flow reviews) + +When running as the publish flow's reviewer, operate under these terms: + +- **You have three inputs, and the boundary is deliberate.** The diff; a one-line claim from the author about what it does; and, when one exists, the requirement source it was built against (ticket, plan, ADR, task body). You were *not* given the conversation, the author's reasoning, or the alternatives they rejected, because those transmit the author's model of the change and your value is in not holding it. Do not ask for them. +- **The requirement source is not leakage — use it.** It was written before the work and usually by someone else, so it is the one input that can contradict the author's claim about their own diff. Without it, the claim is self-certifying and you can only check the diff against a sentence the author wrote. It is what makes the Intent-vs-Delivery criterion below live rather than skipped, and scope creep and missing requirements are exactly what it catches. +- **Review the diff against the claim.** Does it do what the claim says? What does it do that the claim does not mention? What breaks that the claim assumes is fine? +- **Try to refute the change, not to bless it.** Assume there is something wrong and go looking. The default posture is skepticism. +- **A finding you cannot substantiate against the diff is not a finding.** Drop it. An agent told to attack will manufacture findings to satisfy the instruction, and a manufactured finding costs the author a round of work and teaches them to discount the next review. The confidence filter and false-positive filter in Phase 4 are what hold this line: adversarial raises how hard you look, never how weak a claim you will ship. +- **Verify the premise before judging the fix.** When the diff claims to fix a bug, confirm the bug is real and reproduces in the pre-change code. A fix for a misdiagnosed problem passes a diff-shaped review and is still wrong. + +### Re-review mode (continued rounds) + +The publish flow sends the updated diff back to the *same* reviewer rather than spawning a fresh one, so a continued round has your prior findings in context. On each round: + +1. **Re-verify every prior blocking finding against the new diff.** Not against the author's description of the fix. "I fixed it" is a claim like any other, and accepting it is how a re-review becomes a rubber stamp. +2. **Review the fix as a new change.** A fix written under review pressure is prime ground for a regression, so it earns the same scrutiny as the original diff, including on lines the fix touched incidentally. +3. **Say when a prior finding recurs.** If something reported fixed is back, name it as a recurrence rather than filing it fresh. The publish flow treats recurrence as oscillation and stops the loop, which is the right outcome — another round will not converge. +4. **Do not invent new findings to justify another round.** If the blocking findings are addressed and nothing new is substantiated, approve. Approval is the expected end state, not a failure to find something. ## Phase 0 — Eligibility Gate @@ -437,7 +461,7 @@ None. ## Hand-Off -- **Critical** → must be addressed before merge; author fixes, re-review via `/review-code` on the updated SHA +- **Critical** → must be addressed before merge; author fixes, then the updated diff comes back to *this* reviewer for another round (publish flow, Step 1), not to a fresh one and not to the author's own judgment - **Important** → fix, or deliberately defer with an ADR (run `/arch-decide`) - **Minor** → follow-up issues or a cleanup PR - **Intent-vs-Delivery gaps** → either file tickets for the missing pieces or update the plan to reflect reality @@ -39,6 +39,37 @@ Tags are assigned and refreshed by =task-audit=; =task-review= keeps them honest * Rulesets Open Work +** DOING [#B] Hostile subagent review before every agent commit :feature: +:PROPERTIES: +:LAST_REVIEWED: 2026-07-28 +:END: +From the roam inbox, 2026-07-28: "code reviews must occur before every commit an agent does, and they should be hostile reviews from a subagent without the agent's context." + +Two asks, and only the second is new. The publish flow already mandates a review before every commit (Step 1). What changes is *who reviews*: today the reviewing agent is the one that wrote the change, so it inherits the author's mental model, and =review-code= only *suggests* subagent dispatch, and only "for substantive reviews on large diffs". + +Decisions settled with Craig, 2026-07-28, and shipped: +- *Scope* — every commit. The reviewer's own Phase 0 rules a diff trivial and returns Skipped, which satisfies the gate; the author never rules on their own diff. +- *Stance* — "adversarial", not "hostile" (Craig's call). An agent told to attack manufactures findings, so the stance carries a substantiation floor: a finding not substantiated against the diff is dropped. +- *What the reviewer gets* — the diff, a one-line claim of what it does, and the requirement source (ticket, plan, task body) where one exists. Withheld: the conversation, the exploration, the author's rationale. The requirement source stays *in* because it is the only artifact that can contradict the author's claim; withholding it makes the claim self-certifying. +- *Loop* — re-review until the reviewer approves, turning on blocking findings rather than the verdict token. Bounded at three rounds, and stopped early on a finding that recurs after being reported fixed. Both bounds hand the decision to Craig; the unattended callers park instead. +- *Adjudication* — Craig, never the author overruling the reviewer. +- *Home* — the =publish= skill Step 1, with =review-code= carrying the adversarial contract and re-review mode. +- *The =subagents.md= tension* — resolved with an Isolation Override section: the size heuristics assume the main thread could do the task equally well, and they lapse when its own context is what makes its answer untrustworthy. + +Remaining: nothing on the design. The change shipped in this session. + +** TODO [#C] start-work Phase 7 still summarizes the old publish flow :chore:solo: +:PROPERTIES: +:LAST_REVIEWED: 2026-07-28 +:END: +=.claude/commands/start-work.md:336-339= hands off with "Follow =commits.md= exactly" and "Run =/review-code --staged= before each commit" — no isolated reviewer, no re-review loop. It was already stale before the 2026-07-28 review change, because =commits.md= moved the publish flow into the =publish= skill in an earlier commit, so the pointer names a file that no longer holds the flow. + +Grading: Cosmetic severity (a stale summary beside a correct canonical, and start-work is attended so the escalation target exists) x most users frequently = P3 = [#C]. + +Fix is to point Phase 7 at the =publish= skill rather than restate the flow, which is what let it drift in the first place. Worth a sweep for other files that restate the flow instead of pointing at it. + +Found by the adversarial reviewer during the 2026-07-28 review-flow change, and correctly filed rather than fixed there: the line was untouched by that diff. + ** DONE [#C] sentry.org calls one loop cycle a "fire" :chore:solo: CLOSED: [2026-07-28 Tue] :PROPERTIES: |
