diff --git a/.agents/skills/agent-conduct/SKILL.md b/.agents/skills/agent-conduct/SKILL.md index 060267c4..0f0dc8f4 100644 --- a/.agents/skills/agent-conduct/SKILL.md +++ b/.agents/skills/agent-conduct/SKILL.md @@ -24,7 +24,7 @@ Read `GOVERNANCE.md` "Verification Discipline" before reporting success on anyth - **A `raw.githubusercontent.com` 404 does not distinguish a private repository from a missing file.** Where visibility is not confirmed public, read content via `gh api "repos///contents/?ref="`, capturing the result before decoding it (`content=$(gh api ... --jq '.content') && printf '%s' "$content" | base64 -d`) rather than piping straight into `base64 -d`, whose own exit status is all a direct pipe reports, letting a failed fetch decode as an empty success. Never `2>&1` either form, which corrupts the decode with the error text instead of the payload. Verify the ref resolves before reading either failure as proof the content itself does not exist. - **A test asserts the mechanism it names, and a gate has to be watched failing.** A case that passes for an incidental reason is worse than no case, because it is later cited as evidence. - **Platform-specific code is verified only on the platform it runs on.** Reasoning about PowerShell, macOS, or WSL-specific behavior from a different host is not verification, however closely it matches an already-tested equivalent elsewhere. State an untested structural match as exactly that, never in the words used for a tested fact, and when no agent in the loop has access to the target platform, say so and defer or ship it labeled unverified. -- **PR-bound work runs `local-strict-review` before the claim.** Claiming a unit of work done, verified, green, or fixed for work that will become, or already is, a pull request means running `local-strict-review` against the branch's diff first, before a PR-hosted reviewer finds the same gap. +- **PR-bound work runs `local-strict-review` before the claim, and records the pass.** Claiming a unit of work done, verified, green, or fixed for work that will become, or already is, a pull request means running `local-strict-review` against the branch's diff first, before a PR-hosted reviewer finds the same gap, and recording that pass with `scripts/local_review.py` so a capture point can check it. Every push toward a pull request owes one, the fix pushes answering review findings included, which is the round it is most often skipped on. Claims about a pull request being reviewed, clean, or mergeable are owned by the `pr-review-conduct` skill, and claims that a commit landed by `git-commit-conventions`. diff --git a/.agents/skills/drive-pr/SKILL.md b/.agents/skills/drive-pr/SKILL.md index 1e3fb1f7..5d624ca2 100644 --- a/.agents/skills/drive-pr/SKILL.md +++ b/.agents/skills/drive-pr/SKILL.md @@ -55,8 +55,10 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. ## The Drive Loop 1. Isolate into a worktree per repo-worktree, based on develop, before the first edit. -2. Run `local-strict-review` against the branch's current diff, then push the branch and open - the feature -> develop PR if it does not exist yet. +2. Commit the work, run `local-strict-review` and record its pass in the order that skill gives, + then push the branch and open the feature -> develop PR if it does not exist yet. A push the + `pre-push` hook refuses is that gate working rather than an obstacle to route around, and that + skill's refusal table says what each refusal means and what clears it. 3. Drive pr-review-conduct's review loop on it to the Merge Gate, disposing of every finding per "Disposing of Every Finding" below. 4. Capture the branch's own tip before merging, `gh pr view [number] --json headRefOid --jq @@ -102,8 +104,10 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. pr-review-conduct's five outcomes are the actual rule, this is the mapping to use while driving: -- Real, so fix it. Run `local-strict-review` against the branch's current diff, push it, reply - with its commit SHA (outcome 1). +- Real, so fix it, then step 2's own order again before replying with the fixing commit SHA + (outcome 1). This is the round the pass is most often skipped on, since the fix looks small and + the branch was already reviewed once, and a fix push carries content no pass has read exactly as + the first push did. - Not real, or real but out of scope here, so decline in the thread with evidence: the command and its output, the code path, or the rule that governs it. An assertion never closes a finding on its own (outcome 2). diff --git a/.agents/skills/local-strict-review/SKILL.md b/.agents/skills/local-strict-review/SKILL.md index cad9a06e..15797d52 100644 --- a/.agents/skills/local-strict-review/SKILL.md +++ b/.agents/skills/local-strict-review/SKILL.md @@ -22,7 +22,7 @@ A coding agent that finishes a unit of work, judges it ready, and opens the pull ## What It Does -Dispatches one read-only subagent against this branch's full diff since it forked from its target branch. Resolve `` once, `develop` unless `repo-worktree`'s base-branch rule put this branch on `main` instead, then fetch it, `git fetch origin `, and diff against the merge-base, `git diff "$(git merge-base origin/ HEAD)"`. Stop and report a failed fetch rather than running the merge-base or diff commands anyway: an existing local `origin/` ref can still resolve after a failed fetch, and reviewing against it silently trades the current target for a stale one. Use the same resolved `` in every command below, never a literal `develop` alongside it. Naming the target branch explicitly matters: the branch's own `@{u}` tracking ref points at the branch's own remote once it has been pushed, not at the branch it targets, so anchoring there silently narrows a later run to only the diff since the last push instead of the full accumulated diff. That merge-base diff covers every commit already on the branch plus whatever is currently staged or unstaged, so it is never empty and never reviews only the latest increment, at any of the moments this skill is invoked from. A fresh review of the full accumulated diff is what catches what per-push review misses, the exact evidence this skill exists to act on. +Dispatches one read-only subagent against this branch's full diff since it forked from its target branch. Resolve `` once, `develop` unless `repo-worktree`'s base-branch rule put this branch on `main` instead, then fetch it, `git fetch origin `, and diff against the merge-base, `git diff "$(git merge-base origin/ HEAD)"`. Stop and report a failed fetch rather than running the merge-base or diff commands anyway: an existing local `origin/` ref can still resolve after a failed fetch, and reviewing against it silently trades the current target for a stale one. Use the same resolved `` in every command below, never a literal `develop` alongside it. Naming the target branch explicitly matters: the branch's own `@{u}` tracking ref points at the branch's own remote once it has been pushed, not at the branch it targets, so anchoring there silently narrows a later run to only the diff since the last push instead of the full accumulated diff. That merge-base diff covers every commit already on the branch plus whatever is currently staged or unstaged, so it never reviews only the latest increment, at any of the moments this skill is invoked from. An empty diff is not the same as nothing to review, and it is never the signal to stop: it reports no untracked file at all, and it reports nothing for content a commit carries that the working tree has since put back. The untracked-file list below covers the first of those. The second is why this skill commits before reviewing, since a removal or a restore that is committed leaves no net content to miss, and why the engine reads HEAD rather than this diff, its change set coming from the merge base against HEAD, the index and the working tree, so the two answer different questions. A fresh review of the full accumulated diff is what catches what per-push review misses, the exact evidence this skill exists to act on. `git diff` never reports a path `git add` has not touched, so a newly created file sitting untracked would otherwise go unread. List it explicitly, `git ls-files --others --exclude-standard`, and read each result in full alongside the diff, the same as any other file the diff touches. @@ -53,9 +53,34 @@ Bounds: read-only. No edit, no stage, no commit, no push, no PR-hosted write of **Model tier:** the strongest tier this session can reach, per `AGENTS.md` "Match the model tier to the judgment" and "Never tier down the seat holding the judgment", applied here to the reviewer rather than the author. Run the pass on the same tier that authored the change when only one tier is reachable, a second, adversarially-prompted look still catches what the authoring pass's own "looks ready" judgment did not. +## Recording the Pass + +`scripts/local_review.py` is what makes this rule checkable rather than something each session has to remember. It holds no review logic: the pass above is the review, and the engine records that it happened, keyed on the content the reviewer actually saw. That receipt is what a capture point reads, this repository's `.husky/pre-push` hook being the first of them. + +Commit first, then read the digest, then dispatch the subagent, then hand that same value back. Nothing may change the tree between the read and the record. Staging a modified tracked file is such a change, moving the digest although the content did not, and a commit can move it too, since HEAD decides which paths are in the change set at all. Reading after the commit is what leaves neither of them between the read and the record. + +```sh +engine="/scripts/local_review.py" # in the hub itself, scripts/local_review.py +python3 "$engine" status --target # JSON, take contentDigest +# run the pass above, then: +python3 "$engine" record --reviewer agent-skill --target --expect-digest [--findings N] +``` + +Both commands run with the repository under review as the working directory, whichever repository that is. The engine takes no `--repo` and reads whichever repository it is run in, so the path names where the script lives and the working directory names what it measures. + +`` is the same branch "What It Does" resolved for the review, passed to both commands. Leaving it off defaults them to `develop`, and on a `main`-based branch that computes the digest against a merge base the reviewer never read, so the receipt would attest to a change set nobody looked at. A receipt is only valid against the target it names, so the two have to agree. + +`--expect-digest` is required rather than optional, and binding it to the earlier read is the whole point. A format-on-save or a hook autofix between the review and the record would otherwise be stamped as reviewed by a pass that never saw it. A refusal there is the content having moved, so the answer is another pass over the current content rather than another read of the digest. + +Record the pass whatever it found, including nothing. The key covers the net content the branch introduces against its target rather than the commit series, so an interactive rebase that leaves the tree alone keeps the receipt valid, and changing one byte invalidates it. + +**Why the commit comes first**, rather than being an ordering that could equally run the other way. A push delivers the commit, so a receipt recorded over uncommitted work describes something else, and a capture point gating a push refuses on exactly that. Two smaller reasons point the same way: staging a modified tracked file moves the key even though its content did not change, and a commit made after the record can carry content the pass never read. Reviewing earlier than this is still worth doing as ordinary diligence, and it does not substitute for the recorded pass: the digest read and the record bracket a window in which the tree holds still, and a commit inside that window ends it. + +The engine is hub-hosted per `GOVERNANCE.md` "Hub-Hosted Tooling", so a downstream repository reaches a hub checkout's copy rather than carrying one, which is what the path above is for. + ## Disposing of Findings -Every finding maps to one of `pr-review-conduct`'s five outcomes before the pull request opens: fixed, evidence-disproven, filed as a deferred issue, escalated to the maintainer for an explicit call, or, if it keeps recurring, taken as a signal to fix the class. A finding this pass raised and not fixed is never the agent's own call to just leave. Per outcome 3, that decision needs the maintainer's explicit answer, the same way a PR-hosted finding would. Running this pass is expected before every push toward a pull request, per `agent-conduct`. Its findings stay advisory: a finding it raises does not by itself block `git commit` or `gh pr create`, the disposition above is what closes it, the same posture local lint holds today. It posts nothing to GitHub, it only reports to the session driving the work. A finding raised here and not fixed is not thereby resolved: the same finding shape reaching a PR-hosted reviewer later still gets its own fresh disposition, per `pr-review-conduct`'s "a disposition decided on one PR does not carry to the next." +Every finding maps to one of `pr-review-conduct`'s five outcomes before the pull request opens: fixed, evidence-disproven, filed as a deferred issue, escalated to the maintainer for an explicit call, or, if it keeps recurring, taken as a signal to fix the class. A finding this pass raised and not fixed is never the agent's own call to just leave. Per outcome 3, that decision needs the maintainer's explicit answer, the same way a PR-hosted finding would. Running this pass is required before every push toward a pull request, per `agent-conduct`. Two claims sit next to each other here and they point opposite ways, so they are stated apart rather than in one sentence. **The pass is mandatory**, and where a capture point enforces it, a push carrying content no recorded pass covers is refused. That refusal is the gate working rather than a fault to route around. **The findings stay advisory**, and the count a pass raises gates nothing at all, since a pass records that a review ran and never that the content is clean. The disposition above is what closes each finding, the same posture local lint holds today. It posts nothing to GitHub, it only reports to the session driving the work. A finding raised here and not fixed is not thereby resolved: the same finding shape reaching a PR-hosted reviewer later still gets its own fresh disposition, per `pr-review-conduct`'s "a disposition decided on one PR does not carry to the next." ## When to Run It @@ -63,9 +88,23 @@ Every finding maps to one of `pr-review-conduct`'s five outcomes before the pull - Before pushing a fix for a reviewer finding, the same self-review blind spot applies to a fix as to the original diff (`drive-pr`'s "Disposing of Every Finding", `pr-review-conduct`'s outcome 1). - Whenever `agent-conduct`'s "about to claim work is done, verified, green, or fixed" trigger fires for work that will become, or already is, a pull request. +This repository's `.husky/pre-push` hook checks the receipt at the push itself, so the moments above are where the pass is run rather than the only places it is noticed. A blocked push usually means one of them was skipped. The hook is a backstop under this skill and not a replacement for it: it fires only in a clone that enabled `core.hooksPath`, it says nothing about a repository that carries no such hook, and it is bypassable by design, `--no-verify` being the documented route for a genuine pickle rather than for a diff nobody read. + +**Not every refusal is a missing pass, and re-running the pass at one of these does nothing.** Read the refusal itself, which names its own case. Some of these the hook decides before the engine runs, so there is no engine message under them, and the rows below say where each one's detail comes from. + +| The refusal says | What it means | What clears it | +| --- | --- | --- | +| Tracked content differs from HEAD | A push delivers HEAD while a receipt covers the index and working tree, so the receipt does not describe this push | Commit what is being pushed, then the pass, then the record | +| The commit is not this worktree's HEAD | `git push origin some-other-branch` from a checkout sitting elsewhere, and the engine reads the checkout it runs in | Check the pushed branch out in its own worktree, per `repo-worktree` | +| Any wording saying the gate did not or could not run | An execution boundary rather than a verdict, which blocks because a gate that waves a push through when it could not run has stopped gating. The cause is named in that same message or in the engine error printed above it, and it is a missing Python interpreter, an unresolvable target, an unreadable receipt, a git command that failed, or any unexpected failure | Whatever the message names, most often installing an interpreter per `docs/host-setup.md` or fetching the target branch. Never another pass | +| A recorded pass names a branch the check did not measure | The hook reads `develop` and nothing else, so a branch based elsewhere is measured against `develop` whatever the pass targeted, and the engine deliberately prints no record command, since the one it would print records a pass over a diff nobody read | One more pass against the branch this work actually targets, where it does target the measured one. Where it does not, the gate cannot judge the branch at all and the bypass is its answer | + +This table is the fleet's one enumeration of these, and every other surface states the principle and routes here rather than listing shapes or counting them. That is deliberate: through this skill's own review the count went from two to four, and every round left at least one restatement behind. + ## Mechanics Live Elsewhere - Review criteria: `code-review`. - Delegation shape and model-tier discipline: `AGENTS.md` "Context and Delegation Discipline". - Branch base rule (`develop` unless the task is explicitly `main`-only): `repo-worktree`. - Finding disposition once a pull request exists, the Merge Gate, `scripts/pr_review.py`: `pr-review-conduct`, `drive-pr`. +- The receipt's key, its backends, and the three-valued exit contract a capture point folds: `scripts/README.md` "`local_review.py`". diff --git a/.agents/skills/pr-review-conduct/SKILL.md b/.agents/skills/pr-review-conduct/SKILL.md index 810955eb..3f7b9dbb 100644 --- a/.agents/skills/pr-review-conduct/SKILL.md +++ b/.agents/skills/pr-review-conduct/SKILL.md @@ -78,7 +78,7 @@ that says only "open a PR" is not such an instruction. Run every `scripts/pr_review.py` command below from a hub checkout. The script is hosted there and is never carried into a downstream repository. -Run `local-strict-review` against the branch's current diff before step 1's push, and again before any fix push under outcome 1 below. +Run `local-strict-review` against the branch's current diff before step 1's push, and again before any fix push under outcome 1 below. Follow that skill's own ordering and record each pass, which is what a capture point such as this repository's `pre-push` hook reads. A push that hook refuses is the gate working rather than an obstacle to route around, and that skill's refusal table says what each refusal means and what clears it. 1. Push changes to the PR branch and open the pull request when it does not exist. 2. Run `scripts/pr_review.py status` once in the foreground and read its output. @@ -105,8 +105,10 @@ After an authorized merge, run the `repo-worktree` post-merge cleanup procedure ## Every finding ends in one of five outcomes -1. **Real, so fix it.** Run `local-strict-review` against the branch's current diff before pushing - the fix, then reply with the fixing commit SHA. For a finding on platform-specific code +1. **Real, so fix it.** Take the fix through `local-strict-review` the same way step 1's push + went, then reply with the fixing commit SHA. A branch already reviewed once + has not been reviewed for the fix, which is the round this gets dropped on and the churn + `local-strict-review` exists to stop. For a finding on platform-specific code (PowerShell, a macOS- or WSL-only path), "fixed" means executed on that platform, per `agent-conduct` "Before Claiming Done": a fix reasoned out by analogy to a tested equivalent elsewhere is not yet fixed, and the reply says so rather than claiming the SHA closes it. diff --git a/.claude-plugin/fleet-skills/.source-digest b/.claude-plugin/fleet-skills/.source-digest index c46d6208..398a17a6 100644 --- a/.claude-plugin/fleet-skills/.source-digest +++ b/.claude-plugin/fleet-skills/.source-digest @@ -1 +1 @@ -76568e633c063da3 +438bd561ef5f233b diff --git a/.claude-plugin/fleet-skills/skills/agent-conduct/SKILL.md b/.claude-plugin/fleet-skills/skills/agent-conduct/SKILL.md index 060267c4..0f0dc8f4 100644 --- a/.claude-plugin/fleet-skills/skills/agent-conduct/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/agent-conduct/SKILL.md @@ -24,7 +24,7 @@ Read `GOVERNANCE.md` "Verification Discipline" before reporting success on anyth - **A `raw.githubusercontent.com` 404 does not distinguish a private repository from a missing file.** Where visibility is not confirmed public, read content via `gh api "repos///contents/?ref="`, capturing the result before decoding it (`content=$(gh api ... --jq '.content') && printf '%s' "$content" | base64 -d`) rather than piping straight into `base64 -d`, whose own exit status is all a direct pipe reports, letting a failed fetch decode as an empty success. Never `2>&1` either form, which corrupts the decode with the error text instead of the payload. Verify the ref resolves before reading either failure as proof the content itself does not exist. - **A test asserts the mechanism it names, and a gate has to be watched failing.** A case that passes for an incidental reason is worse than no case, because it is later cited as evidence. - **Platform-specific code is verified only on the platform it runs on.** Reasoning about PowerShell, macOS, or WSL-specific behavior from a different host is not verification, however closely it matches an already-tested equivalent elsewhere. State an untested structural match as exactly that, never in the words used for a tested fact, and when no agent in the loop has access to the target platform, say so and defer or ship it labeled unverified. -- **PR-bound work runs `local-strict-review` before the claim.** Claiming a unit of work done, verified, green, or fixed for work that will become, or already is, a pull request means running `local-strict-review` against the branch's diff first, before a PR-hosted reviewer finds the same gap. +- **PR-bound work runs `local-strict-review` before the claim, and records the pass.** Claiming a unit of work done, verified, green, or fixed for work that will become, or already is, a pull request means running `local-strict-review` against the branch's diff first, before a PR-hosted reviewer finds the same gap, and recording that pass with `scripts/local_review.py` so a capture point can check it. Every push toward a pull request owes one, the fix pushes answering review findings included, which is the round it is most often skipped on. Claims about a pull request being reviewed, clean, or mergeable are owned by the `pr-review-conduct` skill, and claims that a commit landed by `git-commit-conventions`. diff --git a/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md b/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md index 1e3fb1f7..5d624ca2 100644 --- a/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md @@ -55,8 +55,10 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. ## The Drive Loop 1. Isolate into a worktree per repo-worktree, based on develop, before the first edit. -2. Run `local-strict-review` against the branch's current diff, then push the branch and open - the feature -> develop PR if it does not exist yet. +2. Commit the work, run `local-strict-review` and record its pass in the order that skill gives, + then push the branch and open the feature -> develop PR if it does not exist yet. A push the + `pre-push` hook refuses is that gate working rather than an obstacle to route around, and that + skill's refusal table says what each refusal means and what clears it. 3. Drive pr-review-conduct's review loop on it to the Merge Gate, disposing of every finding per "Disposing of Every Finding" below. 4. Capture the branch's own tip before merging, `gh pr view [number] --json headRefOid --jq @@ -102,8 +104,10 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. pr-review-conduct's five outcomes are the actual rule, this is the mapping to use while driving: -- Real, so fix it. Run `local-strict-review` against the branch's current diff, push it, reply - with its commit SHA (outcome 1). +- Real, so fix it, then step 2's own order again before replying with the fixing commit SHA + (outcome 1). This is the round the pass is most often skipped on, since the fix looks small and + the branch was already reviewed once, and a fix push carries content no pass has read exactly as + the first push did. - Not real, or real but out of scope here, so decline in the thread with evidence: the command and its output, the code path, or the rule that governs it. An assertion never closes a finding on its own (outcome 2). diff --git a/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md b/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md index cad9a06e..15797d52 100644 --- a/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md @@ -22,7 +22,7 @@ A coding agent that finishes a unit of work, judges it ready, and opens the pull ## What It Does -Dispatches one read-only subagent against this branch's full diff since it forked from its target branch. Resolve `` once, `develop` unless `repo-worktree`'s base-branch rule put this branch on `main` instead, then fetch it, `git fetch origin `, and diff against the merge-base, `git diff "$(git merge-base origin/ HEAD)"`. Stop and report a failed fetch rather than running the merge-base or diff commands anyway: an existing local `origin/` ref can still resolve after a failed fetch, and reviewing against it silently trades the current target for a stale one. Use the same resolved `` in every command below, never a literal `develop` alongside it. Naming the target branch explicitly matters: the branch's own `@{u}` tracking ref points at the branch's own remote once it has been pushed, not at the branch it targets, so anchoring there silently narrows a later run to only the diff since the last push instead of the full accumulated diff. That merge-base diff covers every commit already on the branch plus whatever is currently staged or unstaged, so it is never empty and never reviews only the latest increment, at any of the moments this skill is invoked from. A fresh review of the full accumulated diff is what catches what per-push review misses, the exact evidence this skill exists to act on. +Dispatches one read-only subagent against this branch's full diff since it forked from its target branch. Resolve `` once, `develop` unless `repo-worktree`'s base-branch rule put this branch on `main` instead, then fetch it, `git fetch origin `, and diff against the merge-base, `git diff "$(git merge-base origin/ HEAD)"`. Stop and report a failed fetch rather than running the merge-base or diff commands anyway: an existing local `origin/` ref can still resolve after a failed fetch, and reviewing against it silently trades the current target for a stale one. Use the same resolved `` in every command below, never a literal `develop` alongside it. Naming the target branch explicitly matters: the branch's own `@{u}` tracking ref points at the branch's own remote once it has been pushed, not at the branch it targets, so anchoring there silently narrows a later run to only the diff since the last push instead of the full accumulated diff. That merge-base diff covers every commit already on the branch plus whatever is currently staged or unstaged, so it never reviews only the latest increment, at any of the moments this skill is invoked from. An empty diff is not the same as nothing to review, and it is never the signal to stop: it reports no untracked file at all, and it reports nothing for content a commit carries that the working tree has since put back. The untracked-file list below covers the first of those. The second is why this skill commits before reviewing, since a removal or a restore that is committed leaves no net content to miss, and why the engine reads HEAD rather than this diff, its change set coming from the merge base against HEAD, the index and the working tree, so the two answer different questions. A fresh review of the full accumulated diff is what catches what per-push review misses, the exact evidence this skill exists to act on. `git diff` never reports a path `git add` has not touched, so a newly created file sitting untracked would otherwise go unread. List it explicitly, `git ls-files --others --exclude-standard`, and read each result in full alongside the diff, the same as any other file the diff touches. @@ -53,9 +53,34 @@ Bounds: read-only. No edit, no stage, no commit, no push, no PR-hosted write of **Model tier:** the strongest tier this session can reach, per `AGENTS.md` "Match the model tier to the judgment" and "Never tier down the seat holding the judgment", applied here to the reviewer rather than the author. Run the pass on the same tier that authored the change when only one tier is reachable, a second, adversarially-prompted look still catches what the authoring pass's own "looks ready" judgment did not. +## Recording the Pass + +`scripts/local_review.py` is what makes this rule checkable rather than something each session has to remember. It holds no review logic: the pass above is the review, and the engine records that it happened, keyed on the content the reviewer actually saw. That receipt is what a capture point reads, this repository's `.husky/pre-push` hook being the first of them. + +Commit first, then read the digest, then dispatch the subagent, then hand that same value back. Nothing may change the tree between the read and the record. Staging a modified tracked file is such a change, moving the digest although the content did not, and a commit can move it too, since HEAD decides which paths are in the change set at all. Reading after the commit is what leaves neither of them between the read and the record. + +```sh +engine="/scripts/local_review.py" # in the hub itself, scripts/local_review.py +python3 "$engine" status --target # JSON, take contentDigest +# run the pass above, then: +python3 "$engine" record --reviewer agent-skill --target --expect-digest [--findings N] +``` + +Both commands run with the repository under review as the working directory, whichever repository that is. The engine takes no `--repo` and reads whichever repository it is run in, so the path names where the script lives and the working directory names what it measures. + +`` is the same branch "What It Does" resolved for the review, passed to both commands. Leaving it off defaults them to `develop`, and on a `main`-based branch that computes the digest against a merge base the reviewer never read, so the receipt would attest to a change set nobody looked at. A receipt is only valid against the target it names, so the two have to agree. + +`--expect-digest` is required rather than optional, and binding it to the earlier read is the whole point. A format-on-save or a hook autofix between the review and the record would otherwise be stamped as reviewed by a pass that never saw it. A refusal there is the content having moved, so the answer is another pass over the current content rather than another read of the digest. + +Record the pass whatever it found, including nothing. The key covers the net content the branch introduces against its target rather than the commit series, so an interactive rebase that leaves the tree alone keeps the receipt valid, and changing one byte invalidates it. + +**Why the commit comes first**, rather than being an ordering that could equally run the other way. A push delivers the commit, so a receipt recorded over uncommitted work describes something else, and a capture point gating a push refuses on exactly that. Two smaller reasons point the same way: staging a modified tracked file moves the key even though its content did not change, and a commit made after the record can carry content the pass never read. Reviewing earlier than this is still worth doing as ordinary diligence, and it does not substitute for the recorded pass: the digest read and the record bracket a window in which the tree holds still, and a commit inside that window ends it. + +The engine is hub-hosted per `GOVERNANCE.md` "Hub-Hosted Tooling", so a downstream repository reaches a hub checkout's copy rather than carrying one, which is what the path above is for. + ## Disposing of Findings -Every finding maps to one of `pr-review-conduct`'s five outcomes before the pull request opens: fixed, evidence-disproven, filed as a deferred issue, escalated to the maintainer for an explicit call, or, if it keeps recurring, taken as a signal to fix the class. A finding this pass raised and not fixed is never the agent's own call to just leave. Per outcome 3, that decision needs the maintainer's explicit answer, the same way a PR-hosted finding would. Running this pass is expected before every push toward a pull request, per `agent-conduct`. Its findings stay advisory: a finding it raises does not by itself block `git commit` or `gh pr create`, the disposition above is what closes it, the same posture local lint holds today. It posts nothing to GitHub, it only reports to the session driving the work. A finding raised here and not fixed is not thereby resolved: the same finding shape reaching a PR-hosted reviewer later still gets its own fresh disposition, per `pr-review-conduct`'s "a disposition decided on one PR does not carry to the next." +Every finding maps to one of `pr-review-conduct`'s five outcomes before the pull request opens: fixed, evidence-disproven, filed as a deferred issue, escalated to the maintainer for an explicit call, or, if it keeps recurring, taken as a signal to fix the class. A finding this pass raised and not fixed is never the agent's own call to just leave. Per outcome 3, that decision needs the maintainer's explicit answer, the same way a PR-hosted finding would. Running this pass is required before every push toward a pull request, per `agent-conduct`. Two claims sit next to each other here and they point opposite ways, so they are stated apart rather than in one sentence. **The pass is mandatory**, and where a capture point enforces it, a push carrying content no recorded pass covers is refused. That refusal is the gate working rather than a fault to route around. **The findings stay advisory**, and the count a pass raises gates nothing at all, since a pass records that a review ran and never that the content is clean. The disposition above is what closes each finding, the same posture local lint holds today. It posts nothing to GitHub, it only reports to the session driving the work. A finding raised here and not fixed is not thereby resolved: the same finding shape reaching a PR-hosted reviewer later still gets its own fresh disposition, per `pr-review-conduct`'s "a disposition decided on one PR does not carry to the next." ## When to Run It @@ -63,9 +88,23 @@ Every finding maps to one of `pr-review-conduct`'s five outcomes before the pull - Before pushing a fix for a reviewer finding, the same self-review blind spot applies to a fix as to the original diff (`drive-pr`'s "Disposing of Every Finding", `pr-review-conduct`'s outcome 1). - Whenever `agent-conduct`'s "about to claim work is done, verified, green, or fixed" trigger fires for work that will become, or already is, a pull request. +This repository's `.husky/pre-push` hook checks the receipt at the push itself, so the moments above are where the pass is run rather than the only places it is noticed. A blocked push usually means one of them was skipped. The hook is a backstop under this skill and not a replacement for it: it fires only in a clone that enabled `core.hooksPath`, it says nothing about a repository that carries no such hook, and it is bypassable by design, `--no-verify` being the documented route for a genuine pickle rather than for a diff nobody read. + +**Not every refusal is a missing pass, and re-running the pass at one of these does nothing.** Read the refusal itself, which names its own case. Some of these the hook decides before the engine runs, so there is no engine message under them, and the rows below say where each one's detail comes from. + +| The refusal says | What it means | What clears it | +| --- | --- | --- | +| Tracked content differs from HEAD | A push delivers HEAD while a receipt covers the index and working tree, so the receipt does not describe this push | Commit what is being pushed, then the pass, then the record | +| The commit is not this worktree's HEAD | `git push origin some-other-branch` from a checkout sitting elsewhere, and the engine reads the checkout it runs in | Check the pushed branch out in its own worktree, per `repo-worktree` | +| Any wording saying the gate did not or could not run | An execution boundary rather than a verdict, which blocks because a gate that waves a push through when it could not run has stopped gating. The cause is named in that same message or in the engine error printed above it, and it is a missing Python interpreter, an unresolvable target, an unreadable receipt, a git command that failed, or any unexpected failure | Whatever the message names, most often installing an interpreter per `docs/host-setup.md` or fetching the target branch. Never another pass | +| A recorded pass names a branch the check did not measure | The hook reads `develop` and nothing else, so a branch based elsewhere is measured against `develop` whatever the pass targeted, and the engine deliberately prints no record command, since the one it would print records a pass over a diff nobody read | One more pass against the branch this work actually targets, where it does target the measured one. Where it does not, the gate cannot judge the branch at all and the bypass is its answer | + +This table is the fleet's one enumeration of these, and every other surface states the principle and routes here rather than listing shapes or counting them. That is deliberate: through this skill's own review the count went from two to four, and every round left at least one restatement behind. + ## Mechanics Live Elsewhere - Review criteria: `code-review`. - Delegation shape and model-tier discipline: `AGENTS.md` "Context and Delegation Discipline". - Branch base rule (`develop` unless the task is explicitly `main`-only): `repo-worktree`. - Finding disposition once a pull request exists, the Merge Gate, `scripts/pr_review.py`: `pr-review-conduct`, `drive-pr`. +- The receipt's key, its backends, and the three-valued exit contract a capture point folds: `scripts/README.md` "`local_review.py`". diff --git a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md index 810955eb..3f7b9dbb 100644 --- a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md @@ -78,7 +78,7 @@ that says only "open a PR" is not such an instruction. Run every `scripts/pr_review.py` command below from a hub checkout. The script is hosted there and is never carried into a downstream repository. -Run `local-strict-review` against the branch's current diff before step 1's push, and again before any fix push under outcome 1 below. +Run `local-strict-review` against the branch's current diff before step 1's push, and again before any fix push under outcome 1 below. Follow that skill's own ordering and record each pass, which is what a capture point such as this repository's `pre-push` hook reads. A push that hook refuses is the gate working rather than an obstacle to route around, and that skill's refusal table says what each refusal means and what clears it. 1. Push changes to the PR branch and open the pull request when it does not exist. 2. Run `scripts/pr_review.py status` once in the foreground and read its output. @@ -105,8 +105,10 @@ After an authorized merge, run the `repo-worktree` post-merge cleanup procedure ## Every finding ends in one of five outcomes -1. **Real, so fix it.** Run `local-strict-review` against the branch's current diff before pushing - the fix, then reply with the fixing commit SHA. For a finding on platform-specific code +1. **Real, so fix it.** Take the fix through `local-strict-review` the same way step 1's push + went, then reply with the fixing commit SHA. A branch already reviewed once + has not been reviewed for the fix, which is the round this gets dropped on and the churn + `local-strict-review` exists to stop. For a finding on platform-specific code (PowerShell, a macOS- or WSL-only path), "fixed" means executed on that platform, per `agent-conduct` "Before Claiming Done": a fix reasoned out by analogy to a tested equivalent elsewhere is not yet fixed, and the reply says so rather than claiming the SHA closes it. diff --git a/.github/skills/agent-conduct/SKILL.md b/.github/skills/agent-conduct/SKILL.md index 060267c4..0f0dc8f4 100644 --- a/.github/skills/agent-conduct/SKILL.md +++ b/.github/skills/agent-conduct/SKILL.md @@ -24,7 +24,7 @@ Read `GOVERNANCE.md` "Verification Discipline" before reporting success on anyth - **A `raw.githubusercontent.com` 404 does not distinguish a private repository from a missing file.** Where visibility is not confirmed public, read content via `gh api "repos///contents/?ref="`, capturing the result before decoding it (`content=$(gh api ... --jq '.content') && printf '%s' "$content" | base64 -d`) rather than piping straight into `base64 -d`, whose own exit status is all a direct pipe reports, letting a failed fetch decode as an empty success. Never `2>&1` either form, which corrupts the decode with the error text instead of the payload. Verify the ref resolves before reading either failure as proof the content itself does not exist. - **A test asserts the mechanism it names, and a gate has to be watched failing.** A case that passes for an incidental reason is worse than no case, because it is later cited as evidence. - **Platform-specific code is verified only on the platform it runs on.** Reasoning about PowerShell, macOS, or WSL-specific behavior from a different host is not verification, however closely it matches an already-tested equivalent elsewhere. State an untested structural match as exactly that, never in the words used for a tested fact, and when no agent in the loop has access to the target platform, say so and defer or ship it labeled unverified. -- **PR-bound work runs `local-strict-review` before the claim.** Claiming a unit of work done, verified, green, or fixed for work that will become, or already is, a pull request means running `local-strict-review` against the branch's diff first, before a PR-hosted reviewer finds the same gap. +- **PR-bound work runs `local-strict-review` before the claim, and records the pass.** Claiming a unit of work done, verified, green, or fixed for work that will become, or already is, a pull request means running `local-strict-review` against the branch's diff first, before a PR-hosted reviewer finds the same gap, and recording that pass with `scripts/local_review.py` so a capture point can check it. Every push toward a pull request owes one, the fix pushes answering review findings included, which is the round it is most often skipped on. Claims about a pull request being reviewed, clean, or mergeable are owned by the `pr-review-conduct` skill, and claims that a commit landed by `git-commit-conventions`. diff --git a/.github/skills/drive-pr/SKILL.md b/.github/skills/drive-pr/SKILL.md index 1e3fb1f7..5d624ca2 100644 --- a/.github/skills/drive-pr/SKILL.md +++ b/.github/skills/drive-pr/SKILL.md @@ -55,8 +55,10 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. ## The Drive Loop 1. Isolate into a worktree per repo-worktree, based on develop, before the first edit. -2. Run `local-strict-review` against the branch's current diff, then push the branch and open - the feature -> develop PR if it does not exist yet. +2. Commit the work, run `local-strict-review` and record its pass in the order that skill gives, + then push the branch and open the feature -> develop PR if it does not exist yet. A push the + `pre-push` hook refuses is that gate working rather than an obstacle to route around, and that + skill's refusal table says what each refusal means and what clears it. 3. Drive pr-review-conduct's review loop on it to the Merge Gate, disposing of every finding per "Disposing of Every Finding" below. 4. Capture the branch's own tip before merging, `gh pr view [number] --json headRefOid --jq @@ -102,8 +104,10 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. pr-review-conduct's five outcomes are the actual rule, this is the mapping to use while driving: -- Real, so fix it. Run `local-strict-review` against the branch's current diff, push it, reply - with its commit SHA (outcome 1). +- Real, so fix it, then step 2's own order again before replying with the fixing commit SHA + (outcome 1). This is the round the pass is most often skipped on, since the fix looks small and + the branch was already reviewed once, and a fix push carries content no pass has read exactly as + the first push did. - Not real, or real but out of scope here, so decline in the thread with evidence: the command and its output, the code path, or the rule that governs it. An assertion never closes a finding on its own (outcome 2). diff --git a/.github/skills/local-strict-review/SKILL.md b/.github/skills/local-strict-review/SKILL.md index cad9a06e..15797d52 100644 --- a/.github/skills/local-strict-review/SKILL.md +++ b/.github/skills/local-strict-review/SKILL.md @@ -22,7 +22,7 @@ A coding agent that finishes a unit of work, judges it ready, and opens the pull ## What It Does -Dispatches one read-only subagent against this branch's full diff since it forked from its target branch. Resolve `` once, `develop` unless `repo-worktree`'s base-branch rule put this branch on `main` instead, then fetch it, `git fetch origin `, and diff against the merge-base, `git diff "$(git merge-base origin/ HEAD)"`. Stop and report a failed fetch rather than running the merge-base or diff commands anyway: an existing local `origin/` ref can still resolve after a failed fetch, and reviewing against it silently trades the current target for a stale one. Use the same resolved `` in every command below, never a literal `develop` alongside it. Naming the target branch explicitly matters: the branch's own `@{u}` tracking ref points at the branch's own remote once it has been pushed, not at the branch it targets, so anchoring there silently narrows a later run to only the diff since the last push instead of the full accumulated diff. That merge-base diff covers every commit already on the branch plus whatever is currently staged or unstaged, so it is never empty and never reviews only the latest increment, at any of the moments this skill is invoked from. A fresh review of the full accumulated diff is what catches what per-push review misses, the exact evidence this skill exists to act on. +Dispatches one read-only subagent against this branch's full diff since it forked from its target branch. Resolve `` once, `develop` unless `repo-worktree`'s base-branch rule put this branch on `main` instead, then fetch it, `git fetch origin `, and diff against the merge-base, `git diff "$(git merge-base origin/ HEAD)"`. Stop and report a failed fetch rather than running the merge-base or diff commands anyway: an existing local `origin/` ref can still resolve after a failed fetch, and reviewing against it silently trades the current target for a stale one. Use the same resolved `` in every command below, never a literal `develop` alongside it. Naming the target branch explicitly matters: the branch's own `@{u}` tracking ref points at the branch's own remote once it has been pushed, not at the branch it targets, so anchoring there silently narrows a later run to only the diff since the last push instead of the full accumulated diff. That merge-base diff covers every commit already on the branch plus whatever is currently staged or unstaged, so it never reviews only the latest increment, at any of the moments this skill is invoked from. An empty diff is not the same as nothing to review, and it is never the signal to stop: it reports no untracked file at all, and it reports nothing for content a commit carries that the working tree has since put back. The untracked-file list below covers the first of those. The second is why this skill commits before reviewing, since a removal or a restore that is committed leaves no net content to miss, and why the engine reads HEAD rather than this diff, its change set coming from the merge base against HEAD, the index and the working tree, so the two answer different questions. A fresh review of the full accumulated diff is what catches what per-push review misses, the exact evidence this skill exists to act on. `git diff` never reports a path `git add` has not touched, so a newly created file sitting untracked would otherwise go unread. List it explicitly, `git ls-files --others --exclude-standard`, and read each result in full alongside the diff, the same as any other file the diff touches. @@ -53,9 +53,34 @@ Bounds: read-only. No edit, no stage, no commit, no push, no PR-hosted write of **Model tier:** the strongest tier this session can reach, per `AGENTS.md` "Match the model tier to the judgment" and "Never tier down the seat holding the judgment", applied here to the reviewer rather than the author. Run the pass on the same tier that authored the change when only one tier is reachable, a second, adversarially-prompted look still catches what the authoring pass's own "looks ready" judgment did not. +## Recording the Pass + +`scripts/local_review.py` is what makes this rule checkable rather than something each session has to remember. It holds no review logic: the pass above is the review, and the engine records that it happened, keyed on the content the reviewer actually saw. That receipt is what a capture point reads, this repository's `.husky/pre-push` hook being the first of them. + +Commit first, then read the digest, then dispatch the subagent, then hand that same value back. Nothing may change the tree between the read and the record. Staging a modified tracked file is such a change, moving the digest although the content did not, and a commit can move it too, since HEAD decides which paths are in the change set at all. Reading after the commit is what leaves neither of them between the read and the record. + +```sh +engine="/scripts/local_review.py" # in the hub itself, scripts/local_review.py +python3 "$engine" status --target # JSON, take contentDigest +# run the pass above, then: +python3 "$engine" record --reviewer agent-skill --target --expect-digest [--findings N] +``` + +Both commands run with the repository under review as the working directory, whichever repository that is. The engine takes no `--repo` and reads whichever repository it is run in, so the path names where the script lives and the working directory names what it measures. + +`` is the same branch "What It Does" resolved for the review, passed to both commands. Leaving it off defaults them to `develop`, and on a `main`-based branch that computes the digest against a merge base the reviewer never read, so the receipt would attest to a change set nobody looked at. A receipt is only valid against the target it names, so the two have to agree. + +`--expect-digest` is required rather than optional, and binding it to the earlier read is the whole point. A format-on-save or a hook autofix between the review and the record would otherwise be stamped as reviewed by a pass that never saw it. A refusal there is the content having moved, so the answer is another pass over the current content rather than another read of the digest. + +Record the pass whatever it found, including nothing. The key covers the net content the branch introduces against its target rather than the commit series, so an interactive rebase that leaves the tree alone keeps the receipt valid, and changing one byte invalidates it. + +**Why the commit comes first**, rather than being an ordering that could equally run the other way. A push delivers the commit, so a receipt recorded over uncommitted work describes something else, and a capture point gating a push refuses on exactly that. Two smaller reasons point the same way: staging a modified tracked file moves the key even though its content did not change, and a commit made after the record can carry content the pass never read. Reviewing earlier than this is still worth doing as ordinary diligence, and it does not substitute for the recorded pass: the digest read and the record bracket a window in which the tree holds still, and a commit inside that window ends it. + +The engine is hub-hosted per `GOVERNANCE.md` "Hub-Hosted Tooling", so a downstream repository reaches a hub checkout's copy rather than carrying one, which is what the path above is for. + ## Disposing of Findings -Every finding maps to one of `pr-review-conduct`'s five outcomes before the pull request opens: fixed, evidence-disproven, filed as a deferred issue, escalated to the maintainer for an explicit call, or, if it keeps recurring, taken as a signal to fix the class. A finding this pass raised and not fixed is never the agent's own call to just leave. Per outcome 3, that decision needs the maintainer's explicit answer, the same way a PR-hosted finding would. Running this pass is expected before every push toward a pull request, per `agent-conduct`. Its findings stay advisory: a finding it raises does not by itself block `git commit` or `gh pr create`, the disposition above is what closes it, the same posture local lint holds today. It posts nothing to GitHub, it only reports to the session driving the work. A finding raised here and not fixed is not thereby resolved: the same finding shape reaching a PR-hosted reviewer later still gets its own fresh disposition, per `pr-review-conduct`'s "a disposition decided on one PR does not carry to the next." +Every finding maps to one of `pr-review-conduct`'s five outcomes before the pull request opens: fixed, evidence-disproven, filed as a deferred issue, escalated to the maintainer for an explicit call, or, if it keeps recurring, taken as a signal to fix the class. A finding this pass raised and not fixed is never the agent's own call to just leave. Per outcome 3, that decision needs the maintainer's explicit answer, the same way a PR-hosted finding would. Running this pass is required before every push toward a pull request, per `agent-conduct`. Two claims sit next to each other here and they point opposite ways, so they are stated apart rather than in one sentence. **The pass is mandatory**, and where a capture point enforces it, a push carrying content no recorded pass covers is refused. That refusal is the gate working rather than a fault to route around. **The findings stay advisory**, and the count a pass raises gates nothing at all, since a pass records that a review ran and never that the content is clean. The disposition above is what closes each finding, the same posture local lint holds today. It posts nothing to GitHub, it only reports to the session driving the work. A finding raised here and not fixed is not thereby resolved: the same finding shape reaching a PR-hosted reviewer later still gets its own fresh disposition, per `pr-review-conduct`'s "a disposition decided on one PR does not carry to the next." ## When to Run It @@ -63,9 +88,23 @@ Every finding maps to one of `pr-review-conduct`'s five outcomes before the pull - Before pushing a fix for a reviewer finding, the same self-review blind spot applies to a fix as to the original diff (`drive-pr`'s "Disposing of Every Finding", `pr-review-conduct`'s outcome 1). - Whenever `agent-conduct`'s "about to claim work is done, verified, green, or fixed" trigger fires for work that will become, or already is, a pull request. +This repository's `.husky/pre-push` hook checks the receipt at the push itself, so the moments above are where the pass is run rather than the only places it is noticed. A blocked push usually means one of them was skipped. The hook is a backstop under this skill and not a replacement for it: it fires only in a clone that enabled `core.hooksPath`, it says nothing about a repository that carries no such hook, and it is bypassable by design, `--no-verify` being the documented route for a genuine pickle rather than for a diff nobody read. + +**Not every refusal is a missing pass, and re-running the pass at one of these does nothing.** Read the refusal itself, which names its own case. Some of these the hook decides before the engine runs, so there is no engine message under them, and the rows below say where each one's detail comes from. + +| The refusal says | What it means | What clears it | +| --- | --- | --- | +| Tracked content differs from HEAD | A push delivers HEAD while a receipt covers the index and working tree, so the receipt does not describe this push | Commit what is being pushed, then the pass, then the record | +| The commit is not this worktree's HEAD | `git push origin some-other-branch` from a checkout sitting elsewhere, and the engine reads the checkout it runs in | Check the pushed branch out in its own worktree, per `repo-worktree` | +| Any wording saying the gate did not or could not run | An execution boundary rather than a verdict, which blocks because a gate that waves a push through when it could not run has stopped gating. The cause is named in that same message or in the engine error printed above it, and it is a missing Python interpreter, an unresolvable target, an unreadable receipt, a git command that failed, or any unexpected failure | Whatever the message names, most often installing an interpreter per `docs/host-setup.md` or fetching the target branch. Never another pass | +| A recorded pass names a branch the check did not measure | The hook reads `develop` and nothing else, so a branch based elsewhere is measured against `develop` whatever the pass targeted, and the engine deliberately prints no record command, since the one it would print records a pass over a diff nobody read | One more pass against the branch this work actually targets, where it does target the measured one. Where it does not, the gate cannot judge the branch at all and the bypass is its answer | + +This table is the fleet's one enumeration of these, and every other surface states the principle and routes here rather than listing shapes or counting them. That is deliberate: through this skill's own review the count went from two to four, and every round left at least one restatement behind. + ## Mechanics Live Elsewhere - Review criteria: `code-review`. - Delegation shape and model-tier discipline: `AGENTS.md` "Context and Delegation Discipline". - Branch base rule (`develop` unless the task is explicitly `main`-only): `repo-worktree`. - Finding disposition once a pull request exists, the Merge Gate, `scripts/pr_review.py`: `pr-review-conduct`, `drive-pr`. +- The receipt's key, its backends, and the three-valued exit contract a capture point folds: `scripts/README.md` "`local_review.py`". diff --git a/.github/skills/pr-review-conduct/SKILL.md b/.github/skills/pr-review-conduct/SKILL.md index 810955eb..3f7b9dbb 100644 --- a/.github/skills/pr-review-conduct/SKILL.md +++ b/.github/skills/pr-review-conduct/SKILL.md @@ -78,7 +78,7 @@ that says only "open a PR" is not such an instruction. Run every `scripts/pr_review.py` command below from a hub checkout. The script is hosted there and is never carried into a downstream repository. -Run `local-strict-review` against the branch's current diff before step 1's push, and again before any fix push under outcome 1 below. +Run `local-strict-review` against the branch's current diff before step 1's push, and again before any fix push under outcome 1 below. Follow that skill's own ordering and record each pass, which is what a capture point such as this repository's `pre-push` hook reads. A push that hook refuses is the gate working rather than an obstacle to route around, and that skill's refusal table says what each refusal means and what clears it. 1. Push changes to the PR branch and open the pull request when it does not exist. 2. Run `scripts/pr_review.py status` once in the foreground and read its output. @@ -105,8 +105,10 @@ After an authorized merge, run the `repo-worktree` post-merge cleanup procedure ## Every finding ends in one of five outcomes -1. **Real, so fix it.** Run `local-strict-review` against the branch's current diff before pushing - the fix, then reply with the fixing commit SHA. For a finding on platform-specific code +1. **Real, so fix it.** Take the fix through `local-strict-review` the same way step 1's push + went, then reply with the fixing commit SHA. A branch already reviewed once + has not been reviewed for the fix, which is the round this gets dropped on and the churn + `local-strict-review` exists to stop. For a finding on platform-specific code (PowerShell, a macOS- or WSL-only path), "fixed" means executed on that platform, per `agent-conduct` "Before Claiming Done": a fix reasoned out by analogy to a tested equivalent elsewhere is not yet fixed, and the reply says so rather than claiming the SHA closes it. diff --git a/.husky/pre-push b/.husky/pre-push new file mode 100755 index 00000000..8f2ff39d --- /dev/null +++ b/.husky/pre-push @@ -0,0 +1,99 @@ +#!/usr/bin/env bash +# Refuses a branch push that no recorded local review pass covers, per GOVERNANCE.md "Verification Discipline". +# Enable it per clone with `git config core.hooksPath .husky`, and see the pre-commit hook for why `_/husky.sh` is not sourced. +set -Eeuo pipefail + +# Belt and braces for an invocation that does not come from git, which always runs a hook from the top level. +cd "$(git rev-parse --show-toplevel)" + +# A branch update whose object id is not HEAD is content this checkout does not hold. +# The all-zero id is a delete, whose length differs between a SHA-1 and a SHA-256 repository. +# Reading stdin without a pipeline keeps the flags in this shell rather than in a subshell. +head_oid="$(git rev-parse HEAD)" +pushes_a_branch=no +pushes_a_stranger=no +while read -r _ local_oid remote_ref _; do + case "$remote_ref" in + refs/heads/*) ;; + *) continue ;; + esac + case "$local_oid" in + *[!0]*) ;; + *) continue ;; + esac + pushes_a_branch=yes + if [ "$local_oid" != "$head_oid" ]; then + pushes_a_stranger=yes + fi +done +if [ "$pushes_a_branch" = no ]; then + exit 0 +fi +if [ "$pushes_a_stranger" = yes ]; then + echo "pre-push: this push carries a commit that is not this worktree's HEAD, so the gate cannot speak for it." >&2 + echo "pre-push: check out the branch being pushed, in its own worktree, and push from there." >&2 + exit 1 +fi + +# A push delivers HEAD while the engine's key covers the index and the working tree. +# Plumbing rather than `git diff`, which consults diff drivers. +# A lossy textconv makes differing content compare equal, and off-switches fail open on the next driver. +# --refresh first, since diff-index reads stat information and a touched file otherwise reads dirty. +# --ignore-submodules=none because diff.ignoreSubmodules would otherwise hide a differing gitlink. +# 1 is a real difference, and any other status is git failing, which is a boundary rather than a verdict. +refresh_status=0 +git update-index -q --refresh || refresh_status=$? +tree_status=0 +git diff-index --quiet --ignore-submodules=none HEAD -- || tree_status=$? +if [ "$tree_status" -eq 1 ]; then + echo "pre-push: this worktree holds tracked content that differs from HEAD, so the gate cannot speak for the push." >&2 + # An unresolved merge exits the refresh 1 with the refresh having worked, so it is read here rather than doubted below. + if [ -n "$(git ls-files --unmerged)" ]; then + echo "pre-push: this worktree has an unresolved merge, so resolve and commit it before pushing." >&2 + else + echo "pre-push: commit what is being pushed, then run the pass and record it, then push." >&2 + # Past 1 the refresh did not complete, leaving stat information that reads dirty on unchanged content. + if [ "$refresh_status" -gt 1 ]; then + echo "pre-push: 'git update-index --refresh' exited $refresh_status, so run 'git status' and retry before believing this." >&2 + fi + fi + exit 1 +elif [ "$tree_status" -ne 0 ]; then + echo "pre-push: 'git diff-index --quiet --ignore-submodules=none HEAD --' failed with $tree_status, so the gate did not run." >&2 + exit 1 +fi + +# The interpreter is chosen by running the probes spec/host-tools.json declares, in its order. +# The pre-commit hook states why a presence test picks the wrong name on native Windows. +if python3 --version >/dev/null 2>&1; then + run_py() { python3 "$@"; } + py_name=python3 +elif py -3 --version >/dev/null 2>&1; then + run_py() { py -3 "$@"; } + py_name="py -3" +else + echo "pre-push: neither 'python3 --version' nor 'py -3 --version' ran, so the review gate did not run." >&2 + echo "pre-push: see docs/host-setup.md 'What a Host Must Provide'." >&2 + exit 1 +fi + +# No --target: a target read from the environment is set inline on the very command being gated. +status=0 +run_py scripts/local_review.py check || status=$? +if [ "$status" -eq 0 ]; then + exit 0 +fi +if [ "$status" -eq 1 ]; then + echo "" >&2 + # The engine names its own case, and only some of them are a missing pass. + if [ "$py_name" != python3 ]; then + echo "pre-push: this host runs the engine as '$py_name', so read any 'python3' above as '$py_name'." >&2 + fi + echo "pre-push: this push is blocked, which is the gate working. Read the engine message above for the case." >&2 + echo "pre-push: the local-strict-review skill's refusal table says what clears each one." >&2 + exit 1 +fi +echo "" >&2 +echo "pre-push: the local review gate could not run, which is not a verdict either way." >&2 +echo "pre-push: fix what it reported above, most often by fetching the target branch." >&2 +exit 1 diff --git a/AGENTS.md b/AGENTS.md index 33bf3dce..8b8cc5b8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -110,4 +110,4 @@ Adding or changing a managed host tool is packaged as `add-host-tool`. It keeps Driving a pull request through its review loop, from a feature branch into `develop` and, when asked, on to a mergeable `develop -> main` promotion PR, disposing of every reviewer finding along the way per `pr-review-conduct`, is packaged as `drive-pr`, new content rather than a rule extracted from a section. Merging a ready promotion PR and dispatching the release it unblocks, refreshing this machine's installed Skills first when the repo is this hub, is `merge-and-release`, its own new-content package, invoked separately from `drive-pr` so the promotion merge and the release dispatch each keep their own explicit go-ahead. -Running one read-only, adversarial review pass against a branch's current diff against its target branch, full file context included, on the strongest model tier the session can reach, before a unit of PR-bound work is pushed toward a pull request or claimed done, is packaged as `local-strict-review`, new content rather than a rule extracted from a section. `drive-pr`, `pr-review-conduct`, and `agent-conduct` each reference it at the moment they already govern, rather than restating what it does. +Running one read-only, adversarial review pass against a branch's current diff against its target branch, full file context included, on the strongest model tier the session can reach, before a unit of PR-bound work is pushed toward a pull request or claimed done, is packaged as `local-strict-review`, new content rather than a rule extracted from a section. `drive-pr`, `pr-review-conduct`, and `agent-conduct` each reference it at the moment they already govern, rather than restating what it does. The rule itself lives in [`GOVERNANCE.md`](./GOVERNANCE.md) "Verification Discipline", `scripts/local_review.py` is the engine that records a pass so a capture point can check one, and a repository carrying a `.husky/pre-push` hook enforces it at the push itself, the skill staying the primary and agent-agnostic layer with the hook a bypassable backstop under it. diff --git a/GOVERNANCE.md b/GOVERNANCE.md index cfc04bb5..71ae2065 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -17,7 +17,7 @@ The specific rules in this file implement a few governing principles. Read these - **Durable knowledge lives in the committed docs, not in agent memory.** Anything a future agent must honor (a rule, a contract, a hard-won gotcha, a pattern worth repeating or one to avoid) belongs in a committed governance file (`AGENTS.md`, `CODESTYLE.md`, `WORKFLOW.md`, or a committed backlog such as a `README.md` TODO section). Agent memory does not survive a new session, a new machine, or a new environment, so it holds only environment-specific nuance and in-flight session state, never anything whose loss on reset would matter. A durable lesson left only in memory is lost to the next agent. - **Keep the governance current as you work.** When work surfaces something durable (a rule worth enforcing, a recurring gotcha, a positive pattern to repeat, a negative one to design out), record it in the governance docs as part of that change, rather than leaving it in a local note or routing around it with a one-off workaround. Where the governing doc is carried from a template this repo cannot edit directly, propose the change upstream instead of only fixing it locally. Governance is not static: it improves by agents folding good patterns in and designing bad ones out. -- **A durable rule earns a mechanical hook only where a hook can actually decide it, otherwise it stays prose.** Three conditions together, not any one alone. The failure recurs even after the governing prose was demonstrably read and understood, so it is not a discovery or loading problem a structural fix (getting the rule into context at all) would already solve. The triggering shape is decidable from the tool call's own text, arguments, and working directory alone, with no semantic or contextual judgment required. And the failure is destructive or hard to reverse rather than a quality miss. A worktree-isolation lapse met all three (the #1073 incident happened under prose the agent had already read, "is this command's target a primary checkout" is a plain directory comparison, and the harm is another task's swept or reverted work), so it was promoted to a `gh-write-guard` hook rule. A skill's own trigger going unread by the session at all, by contrast, is a loading problem, fixed by getting the rule into context (the `CLAUDE.md` importing `AGENTS.md`), not by a hook. And "was this review finding actually evidence-backed" fails the second condition outright: a hook sees only the command text, never the judgment call itself, so it can only ever nag, not decide, and that class of rule stays prose and a chained Skill trigger. +- **A durable rule earns a mechanical hook only where a hook can actually decide it, otherwise it stays prose.** Three conditions together, not any one alone. The failure recurs even after the governing prose was demonstrably read and understood, so it is not a discovery or loading problem a structural fix (getting the rule into context at all) would already solve. The triggering shape is decidable from the tool call's own text, arguments, and working directory alone, with no semantic or contextual judgment required. And the failure is destructive or hard to reverse rather than a quality miss. A worktree-isolation lapse met all three (the #1073 incident happened under prose the agent had already read, "is this command's target a primary checkout" is a plain directory comparison, and the harm is another task's swept or reverted work), so it was promoted to a `gh-write-guard` hook rule. A skill's own trigger going unread by the session at all, by contrast, is a loading problem, fixed by getting the rule into context (the `CLAUDE.md` importing `AGENTS.md`), not by a hook. And "was this review finding actually evidence-backed" fails the second condition outright: a hook sees only the command text, never the judgment call itself, so it can only ever nag, not decide, and that class of rule stays prose and a chained Skill trigger. Those three conditions gate promotion to a **host** hook, the involuntary layer that fires in every session under the maintainer's own credentials and cannot be opted out of, which is why the bar there is destructive harm. A **committed** hook in the repository's own tree is a third layer between prose and that one, and it is earned on weaker grounds: it is opt-in per clone, visible in the tree, bypassable by design, and it therefore fits a rule whose harm is a quality miss rather than a destruction. The second condition still binds it, since a hook that cannot decide its own trigger is a hook that nags, so what earns the layer is finding the decidable half of a rule whose other half is judgment. The local-review rule under "Verification Discipline" is the worked example: whether a review's findings were rightly disposed of is judgment no hook can decide and stays prose, while whether a review pass ran over exactly the content being pushed is a receipt comparison, which the hub's own `.husky/pre-push` decides. This section keeps the full rules and is surfaced at its decision moment by the `agent-conduct` Skill at `.agents/skills/agent-conduct/SKILL.md` in the hub, not a repo-relative link since that path is hub-local and not carried into every fleet repo. @@ -181,6 +181,7 @@ The checks that separate work actually done from work that merely reports succes - **Gates, filters, and gate-like watchers fail loud, never narrow quietly.** A pattern that silently matches less, an allowlist that silently stops matching, or a gate that silently stops gating all report success while doing nothing. When a construct exists to notice something, make the not-noticing case produce an error or an annotation. An identity allowlist used as a gate, for one, must raise an error when its list stops matching, not silently pass everything through. - **Config with a uniqueness rule is validated on read, and its consumers assert what it promised.** A repeated key in a lookup table is not a precedence question to settle quietly, it is two answers to one question, and keeping whichever came last picks one of them where the reader sees no choice being made. Fail on the duplicate at the point the config is read, so the code downstream can rely on the invariant instead of re-deriving it. - **Validate and read on the same normalized key.** A guard that compares stripped names while the join looks up the raw one passes a padded key and then matches nothing, so the exact fault the guard exists to stop is sitting inside the guard. Normalize once at the boundary and use that one value for both the check and the lookup. +- **Every push toward a pull request is preceded by a local adversarial review of the branch's whole diff, and the pass is recorded.** The rule binds every push rather than the first one, so a fix push answering a reviewer's finding owes a pass exactly as the branch's first push did, and that is the round it is actually skipped on: the fix looks small, the branch was reviewed once already, and what goes up is content no review has read. Skipping it does not save the round, it moves it, into the fix-commit and review-comment cycle that spends wall-clock, Actions runtime, and agent tokens finding what a local pass would have. The pass itself, its delegation shape, and its model tier are the `local-strict-review` Skill's, and `scripts/local_review.py` records it keyed on the content the reviewer actually saw, so a capture point can ask whether a receipt still covers what is about to be pushed rather than trusting the rule to have been remembered. The pass is mandatory and its findings are advisory, which are opposite claims worth keeping apart: a pass is recorded whether it raised ten findings or none, and disposing of each one is judgment, per "PR Review Etiquette" below. - **Run the repo's whole lint gate before every push, not the parts that look relevant.** CI runs all of them, so a partial local run only defers the failure, and the tool most likely to catch a given change is often the one it seems least about (an edit that manipulates line endings is exactly when `editorconfig-checker` matters). The repo documents each linter's known-working invocation, and this rule is that **all** of them run. - **Editing CRLF files programmatically: `.` matches `\r` in a regex**, so a captured line keeps its carriage return and rejoining with `\r\n` yields `CRCRLF`. A text-mode rewrite has the mirror failure, silently flattening CRLF to LF. Prefer line-based edits (`splitlines(keepends=True)`) or literal replacement over regex reassembly. In Python the text-mode failure is the *default*: `Path.read_text()` decodes through universal newlines and `write_text()` writes `\n` back, so a read-edit-write round trip flattens the whole file while the edit itself looks correct. Pass `newline=''` to both, or work in bytes. This is the mechanism behind the Line Endings warning above, and it is worth naming because the corruption is invisible in a rendered diff. - **Scope a check by what the project declares, not by the file that prompted it.** A check written while editing one file tends to cover that file's language and stop, and then reports success on every other surface the rule governs. Read the declared types, or the config that enumerates them, and cover each one, then assert a floor per surface so a table that narrows fails loudly instead of passing quietly. A rule about comments means every comment syntax the project ships, and a format that carries comments in practice counts even where its specification says otherwise. @@ -245,6 +246,7 @@ CI runs the full lint set, but run the linters locally before pushing to catch i - **A working local hook is strongly suggested fleet-wide, and its absence is a measured audit finding, not an invisible gap.** `spec/project-types.json`'s `parity.hooks` check reads this section for its rationale, judged by hand during an `AUDIT.md` run like every sibling check in its dimension, never mechanized by `spec/audit.py`: a repo with no local hook mechanism wired at all is a `linter-parity` defect, the same severity a missing markdownlint config already gets, while a repo mid-convergence (below) stays operational. CI remains the authoritative run regardless. Two catalog snippets carry the canonical shape, `catalog/snippets/husky/` (Husky.Net, for .NET or any project including Python) and `catalog/snippets/pre-commit/` (the Python `pre-commit` framework, for a repo with no `.husky/` tree), each carrying a copy of `catalog/snippets/hub-fetch-run.py` alongside it. - **The hook** runs **language formatting** and the **diff-scoped doc gates**, never Docker, so it stays fast. The formatting half is whatever the repo's own language needs, CSharpier and `dotnet format` for .NET or ruff and the repo's type checker for Python, via native tooling. A repo adds each half once its tree passes that half, since a gate that fails on the corpus it guards blocks every commit from the moment it lands, so a hook running one half is a repo mid-convergence rather than a repo out of conformance. The doc half runs each gate at the scope that fits it. The prose gate is scoped to the working tree diff against `HEAD` rather than swept over the tree, which is the difference between about 2.2 seconds and about 0.13 and is what makes it affordable in a hook at all. That scope is the working tree rather than the staged index, so a partially staged file is judged on all of its edits, not only the staged ones, and CI re-checks the whole tree regardless, which is what makes that scope affordable in a hook. A whole-repo check belongs there too when it is already fast and takes no file list, which the line-ending consistency check is, so scope is a property of the gate rather than a rule the hook applies to all of them. `repo_gate.py --check sha-pin` stays out of the hook regardless, since it resolves a same-owner pin against the GitHub API. - **The doc gates reach a downstream repo by fetching `.github/actions/prose-gate/prose_lint.py` and `.github/actions/repo-gate/repo_gate.py` fresh from `ptr727/ProjectTemplate`'s `main` branch, via `hub-fetch-run.py`, never vendored and never pinned.** Pinning anything Dependabot does not maintain goes stale by construction, and CI (this repo's own, and the hub's) is the backstop that catches a change landing broken on `main` before a locally fetched copy does real damage. These are the only network calls the doc gates make, one per fetched script. A Python repo's `uvx`-run ruff and type checker can also reach the network, to resolve `@latest` on a cache miss or refresh, the same category of dependency as the Docker pulls the VS Code Lint tasks already do routinely, not a new one. A fetch failure fails the commit, and it never silently skips the gate. The **hub's own** `.husky/pre-commit` is the one exception, staying local and offline, since it already carries `scripts/prose_lint.py` and `scripts/repo_gate.py` directly and has no hub to reach. A repo enables its hook per clone with `git config core.hooksPath .husky` or (`uv tool install pre-commit` once, then) `pre-commit install`. The Husky.Net snippet needs one more step per clone, `dotnet tool restore` then `dotnet husky install`, which generates `.husky/_/husky.sh`, the file the hook sources. CI remains the authoritative run either way. +- **The hub carries a second hook, `.husky/pre-push`, gating the local-review rule rather than lint.** It runs `scripts/local_review.py check` and refuses a branch push carrying content no recorded review pass covers, per "Verification Discipline" above. It fires only for a branch update, so a tag push and a branch delete pass through, and a branch holding no net content against its target has nothing for a review to cover. It refuses rather than guesses in every state it cannot speak for, since the engine reads the checkout it runs in while a push delivers a commit, and those describe the same content only sometimes. The order that keeps them describing the same content, and the one every capture point is built around, is to commit, then review, then record, then push. It reads `develop` and accepts no override from the environment, since an environment variable is set inline on the very command being gated, by whoever is being gated, which is the same reason an authorization is never read from a channel the agent itself can set. The cost is that it measures a branch based on anything but `develop` against `develop` anyway, so its refusal there is not a verdict about that branch. Each refusal names its own case, and the `local-strict-review` Skill carries the fleet's one enumeration of them with what clears each, deliberately in one place rather than restated here. It folds the engine's three-valued exit honestly: a check that could not run blocks too, and says so in different words than a check that ran and found no pass, because a gate that waves a push through when it could not run is a gate that stopped gating. It is a backstop rather than a seal, and it is bypassable by construction: `--no-verify` is the documented route out of a pickle and is not the only one, since a git hook cannot police its own invocation. A Claude Code session meets a narrower surface, that flag being denied unconditionally by the agent-safety hook's explicit-bypass rule, and that denial is Claude Code's alone, since Codex and opencode carry no such hook yet. So the committed hook raises the cost of skipping the rule for one agent and lowers it for none, and the prose layer above stays the agent-agnostic one that actually binds. It is hub-only for now. `local_review.py` is hub-hosted per "Hub-Hosted Tooling", and a downstream repo reaches it as a hub checkout's copy run with its own worktree as the working directory, so a catalog snippet carrying this hook fleet-wide is a later step rather than part of this one. - **The VS Code Lint tasks** run the full doc-lint set via Docker `:latest` on demand, the local surface for Markdown, spelling, workflow, and EditorConfig checks, plus `Lint: Prose` and `Lint: EOL`, the same two hook gates in whole-repo mode rather than diff-scoped, for on-demand full-tree validation. The Docker invocations below run the same tools and configs as the VS Code tasks. Their headless form separates the image pull and minimizes repository exposure for an agent executor. diff --git a/docs/fleet-map.md b/docs/fleet-map.md index a60fc81a..a716a0d6 100644 --- a/docs/fleet-map.md +++ b/docs/fleet-map.md @@ -107,12 +107,14 @@ flowchart TD start["session start"] -->|"restated-rule symptom"| stale["skills_install --report, per the documented cadence"] start --> work["work: codestyle, commit, and doc skills fire by trigger"] work --> gates["pre-commit gates: prose lint, eol"] - gates --> pr["pull request"] + gates --> review1["local-strict-review, recorded via local_review.py"] + review1 --> push["push, gated by pre-push in the hub"] + push --> pr["pull request"] pr --> review["Copilot loop via pr_review.py"] review --> merge["merge per registry workflowModel"] ``` -Owned by the per-language sections of [`CODESTYLE.md`][codestyle] and the conduct skills. The gates node is the [`OPERATIONS.md`][operations] "Run the gates the way CI runs them" section, and the review node runs through [`scripts/pr_review.py`][pr-review]. No open gaps sit on this path: G6, the unwired staleness check, and G7, the operational direct-commit allowance, are closed and their rows record the resolutions. +Owned by the per-language sections of [`CODESTYLE.md`][codestyle] and the conduct skills. The gates node is the [`OPERATIONS.md`][operations] "Run the gates the way CI runs them" section, the local review node is the `local-strict-review` skill recorded through `scripts/local_review.py`, and the review node runs through [`scripts/pr_review.py`][pr-review]. The push node's gate is the hub's own `.husky/pre-push`, so on a downstream repo that edge is prose alone until a catalog snippet carries the hook. One open gap sits on this path: G13, the local review gate reaching the hub only, which is why the push node's own edge reads as prose on every other repository. G6, the unwired staleness check, and G7, the operational direct-commit allowance, are closed and their rows record the resolutions. ### Hub-Side Operations @@ -162,6 +164,7 @@ Four wiring points close the model, and each is in place: | G10 | The skill lifecycle itself has no skill | skill | closed | | G11 | Peer messaging is live but undeclared | doc | closed | | G12 | General conduct rules have no skill | skill | closed | +| G13 | The local review gate reaches the hub only | script + doc | open | Each gap's handoff below states who detects it, what closes it, and the test that proves it closed. The handoff sentence is the contract the closing pull request implements. @@ -242,6 +245,12 @@ flowchart LR - **Gap** - The conduct layer (ask when unsure, never assume, verification before claiming done, delegation and token discipline) lived in carried [`AGENTS.md`][agents] sections and doc-only GOVERNANCE sections, with no skill firing at the moments those rules are violated. - **Resolution** - The `agent-conduct` skill ships with the narrow decision-moment triggers the proposal specifies (about to claim done, about to assume, a failure just surfaced a lesson), summarizing `Verification Discipline`, `Communicating with the User`, and `Durable Knowledge and Self-Improvement`, which keep the full rules and carry the surfacing pointer, while the carried AGENTS.md sections stay the always-on layer. +### G13: The Local Review Gate Reaches the Hub Only (Open) + +- **Gap** - `GOVERNANCE.md` "Verification Discipline" requires a recorded local review pass before every push toward a pull request, and only the hub carries the [`.husky/pre-push`][pre-push] hook that checks the receipt. Every other repository has the prose layer and no capture point, which is the layering working as designed but leaves the mechanical half unbuilt where most of the work happens. +- **Handoff** - `scripts/local_review.py` is hub-hosted and takes no `--repo`, reading whichever repository the working directory sits in, so a downstream repo reaches it as a hub checkout's copy run from its own worktree. Closing this means a `catalog/snippets/` pre-push companion to the existing pre-commit snippets, reaching the engine the way those reach the doc gates. The closing test is a downstream repo whose unreviewed push is refused and whose recorded push is not. +- **Known limits the snippet inherits** - The hook refuses rather than guesses in several states it cannot speak for, and every one of those is deliberate rather than incidental, so a snippet reproduces them rather than re-deciding them. The `local-strict-review` Skill's refusal table is the enumeration, kept in one place on purpose. Two of them read as shortcomings a snippet should fix and are not, so they are named here with the reason. **The refusal while the working tree holds tracked content differing from HEAD** reads as an unrelated cleanliness check: a push delivers HEAD while the engine's key covers the index and the working tree, so without it a fix staged over an unreviewed commit passes the gate while the push delivers the commit. **The fixed `develop` target, with no environment override**, reads as a gap to close for a repo whose working branch differs. Adding one reopens a measured bypass, since the variable is set inline on the very command being gated, by whoever is being gated, and naming the pushed branch as its own target collapses the change set to nothing and exits 0. A snippet needing another branch hard-codes it rather than reading it from the environment. + ## Proposed Skills Four skills close G9, G10, and G12, shipped through the [`.agents/skills/`][skills-readme] pipeline with `skill-lifecycle` authored first so the other three follow its procedure (in one pull request at the maintainer's direction, per the G9 provenance note). Scope and overlap were settled here before authoring, and each block below is the contract its skill implements. @@ -327,6 +336,7 @@ Design-doc first: this doc merges, then each unchecked item becomes an issue lin ### P4: Steady State +- [ ] Local review gate carried to the fleet as a `catalog/snippets/` pre-push companion, closing G13 - [ ] Refresh cadence observed in practice, revisited if the manual cadence fails - [ ] Register rows retired as they close, per the maintenance rule - [ ] Peer-messaging promotion re-evaluated after cross-host verification @@ -358,6 +368,7 @@ Design-doc first: this doc merges, then each unchecked item becomes an issue lin [host-tools]: ../spec/host-tools.json [operations]: ../OPERATIONS.md [peer-messaging]: ./peer-messaging.md +[pre-push]: ../.husky/pre-push [pr-review]: ../scripts/pr_review.py [prose-lint]: ../scripts/prose_lint.py [readme]: ../README.md diff --git a/host-setup/agent-safety/README.md b/host-setup/agent-safety/README.md index d8225bee..8c295359 100644 --- a/host-setup/agent-safety/README.md +++ b/host-setup/agent-safety/README.md @@ -204,10 +204,14 @@ flowchart LR subgraph layer2["Prose layer"] L2["GOVERNANCE.md / AGENTS.md /\nSkill trigger descriptions --\nread and followed by judgment"] end - subgraph layer3["Hook layer"] + subgraph layer2b["Committed hook layer"] + L2B["The repo's own .husky hooks --\nmechanically decided, opt-in per\nclone, bypassable by design"] + end + subgraph layer3["Host hook layer"] L3["This spec's requirements --\nmechanically decided from the\ntool call alone, no judgment"] end L1 -->|"fixed by a structural\nload/import fix, not a hook"| L2 + L2 -->|"mechanically decidable check,\nharm is a quality miss,\nthe bypass stays available"| L2B L2 -->|"promoted here only when\nmechanically decidable +\ndestructive, per GOVERNANCE.md\n'Durable Knowledge'"| L3 ``` @@ -216,11 +220,28 @@ The first diagram is this spec's actual decision flow, generalized from `claude/ reached the session at all is a loading bug, fixed the way PR #1081 fixed `local-strict-review`'s missed trigger, by wiring `CLAUDE.md` to import `AGENTS.md`. A rule that reached the session and was still not followed, where the trigger is mechanically decidable and the harm is destructive, -is promoted to a hook (requirement 6, above, tracked at [issue #1073][issue-1073], is the worked -example). A rule whose violation can only be judged, not mechanically decided (was a +is promoted to a host hook (requirement 6, above, tracked at [issue #1073][issue-1073], is the +worked example). A rule whose violation can only be judged, not mechanically decided (was a review finding actually evidence-backed?), stays prose and a chained Skill trigger, since a hook there could only nag, never decide. +The committed layer between those two is not this spec's, and it is drawn because leaving it out +made the picture read as a binary it is not. A hook the repository carries in its own tree is +opt-in per clone, visible to anyone reading the repo, and bypassable on purpose, so it can carry a +rule whose harm is a quality miss rather than a destruction, which the host layer's bar excludes. +The hub's own `.husky/pre-push` is the worked example there, refusing a branch push that no +recorded local review pass covers, per [`GOVERNANCE.md`][governance] "Verification Discipline". +The two layers meet at requirement 4, which denies `--no-verify` unconditionally, so a Claude Code +session meets a committed hook it cannot wave through with that flag while a human keeps the escape +hatch. They do not compose into a seal, and saying so would be the more comfortable claim rather +than the true one. A committed hook is bypassable by construction, since it cannot police its own +invocation, and `--no-verify` is the documented route rather than the only one. This requirement +list also reaches Claude Code alone today, per the Per-Agent Status table below, so a Codex or +opencode session meets the committed hook with every route still open. What a committed hook buys +is a rule that fails loudly at the moment it is broken instead of silently, for the agent whose +bypasses this spec covers. It is not a substitute for the prose layer, which is the one that binds +every agent. + ## Per-Agent Status | Agent | Status | Implementation | diff --git a/scripts/README.md b/scripts/README.md index 15f4a5f8..30a92eec 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -220,9 +220,9 @@ The match is on the block's heading rather than anywhere in the body, and on the ## `local_review.py` -Records and verifies that a local review pass covered a branch's current content. [`AGENTS.md`][agents] and the `local-strict-review` Skill hold the review-before-every-push rule itself, and this gives that rule something a capture point can actually check rather than restating it. A pass records a receipt keyed on the content it reviewed, and a capture point asks whether that receipt still covers what is about to be pushed. The capture points themselves (a git pre-push hook, the Skill steps that invoke the pass) are a separate change, so this script is the capability they will call rather than a gate that is already wired up. +Records and verifies that a local review pass covered a branch's current content. [`AGENTS.md`][agents] and the `local-strict-review` Skill hold the review-before-every-push rule itself, and this gives that rule something a capture point can actually check rather than restating it. A pass records a receipt keyed on the content it reviewed, and a capture point asks whether that receipt still covers what is about to be pushed. Two capture points call it. This repository's [`.husky/pre-push`][pre-push] hook runs `check` and refuses a branch push that no recorded pass covers, and the `local-strict-review` Skill's own steps read `status` before the pass and `record` it afterwards. Neither is load-bearing alone: the Skill is the primary, agent-agnostic layer and the hook is a backstop under it, opt-in per clone and bypassable with `--no-verify` by design. -The key is over the content of the changed files, plus the merge-base commit that fixes what "changed" means. It is deliberately not over the diff text and not over `HEAD`, which is what lets a review run before the commit while the check runs at the push: reviewing untracked work and then committing it unchanged leaves the key identical. Each path contributes the state it is in on each side, the working tree's and the index's, both as git's own mode-and-blob identity so the two compare directly. Where they agree the path reads as one state, which is what makes `git add` and `git commit` invisible. Where they diverge, because content is staged that the working tree no longer holds, or because a file mode changed, the key moves. Reading the index rather than the working tree alone is what stops content being committed that the key never saw. +The key is over the content of the changed files, plus the merge-base commit that fixes what "changed" means. It is deliberately not over the diff text. Which paths count is read from three sides, the commit at `HEAD`, the index, and the working tree, and each path's recorded state is read from the last two alone. That split is what lets a review run before the commit while the check runs at the push: reviewing untracked work and then committing it unchanged leaves the key identical. Each path contributes the state it is in on each of those last two sides, both as git's own mode-and-blob identity so they compare directly. Where they agree the path reads as one state, which is what makes `git add` on an untracked file and `git commit` invisible. Where they diverge, because content is staged that the working tree no longer holds, or because a file mode changed, the key moves. Reading the index rather than the working tree alone is what stops content being staged that the key never saw. `HEAD` is read for membership and never for a state, which is what stops content already committed and then undone in the tree from leaving the changed set: `git rm` a file the branch added and the index, the working tree, and the merge base all agree it is absent while the commit a push delivers still carries it. That inclusion has one visible consequence worth stating rather than discovering. A path in the set only because `HEAD` disagrees with the base leaves it when that undo is itself committed, and the key moves, which is right, since before that commit a push delivers the change and after it a push delivers the undo. The key is still not the content a push delivers, and does not try to be. It moves where a push would carry nothing new, staging a modified tracked file being the plainest example, so `git add` is invisible for an untracked file rather than in general, and the ordering that follows is to record a pass over what is already committed. It also holds still while `HEAD`'s own content changes under a fixed index and working tree, which is why a caller gating a push tests the working tree against `HEAD` before trusting this key to describe that push. [`.husky/pre-push`][pre-push] is that test. Every identity is computed by git rather than reconstructed, which matters more than it sounds: a blob id built by hashing raw working-tree bytes cannot equal the one git stores wherever a `text` attribute or any clean filter sits between them, and this repository applies one to every text file, so `git add` moved the key on any CRLF file until that was fixed. The working tree's side is therefore read by staging it into a throwaway index that git builds, with its object writes redirected into a throwaway object directory and the real one attached as an alternate. That redirection is not incidental: staging writes blobs, so without it a read would permanently deposit the content of every unignored untracked file, a stray secret among them, into the repository. Nothing about the real index, the working tree, or the object database is modified. One requirement this places on a repository is that a `clean` filter be deterministic, since the same content is staged once when a pass is recorded and again when it is checked. @@ -239,7 +239,7 @@ python3 scripts/local_review.py check # 0 python3 scripts/local_review.py run --backend coderabbit-cli ``` -The exit code is three-valued on purpose, per [`AGENTS.md`][agents] "Report an execution boundary separately from a check finding". `0` and `1` are findings a caller acts on, and `2` means the check itself never ran, whether because no git repository resolved, the target ref is absent from this checkout, the receipt could not be read off disk, or a backend binary is not installed. Any unexpected failure reports `2` as well, rather than falling through to the interpreter's own exit `1`, which a caller would read as the "not covered" verdict. `status` reports rather than gating, so it exits `0` whether or not the content is covered, and only reports `2` where it could not run at all. +The exit code is three-valued on purpose, per [`AGENTS.md`][agents] "Report an execution boundary separately from a check finding". `0` and `1` are findings a caller acts on, and `2` means the check itself never ran, whether because no git repository resolved, the target ref is absent from this checkout, the receipt could not be read off disk, or a backend binary is not installed. Any unexpected failure reports `2` as well, rather than falling through to the interpreter's own exit `1`, which a caller would read as the "not covered" verdict. `status` reports rather than gating, so it exits `0` whether or not the content is covered, and only reports `2` where it could not run at all. `check` alone treats a branch holding no net content against its target as covered, there being nothing for a review to read and no push git would have anything to reject over, while `status` keeps counting recorded passes and reports that same branch as not covered with zero changed paths. The two answer different questions, and reading either as the other is the mistake that wording guards against. ## `build_dist.py` @@ -281,6 +281,7 @@ Installs the fleet's Skills for the current machine, cross-platform and idempote [host-tools]: ../spec/host-tools.json [marketplace]: ../.claude-plugin/marketplace.json [operations]: ../OPERATIONS.md +[pre-push]: ../.husky/pre-push [prose-gate-action]: ../.github/actions/prose-gate/action.yml [repos]: ../registry/repos.json [section-model]: ../spec/section-model.md diff --git a/scripts/local_review.py b/scripts/local_review.py index baf5e621..d2fa51a3 100755 --- a/scripts/local_review.py +++ b/scripts/local_review.py @@ -8,9 +8,11 @@ be pushed. The key is over the content of the changed paths, plus the merge-base commit that fixes what -"changed" means. It is deliberately not over the diff text and not over HEAD, which is what lets a -review run before the commit while the check runs at the push: reviewing untracked work and then -committing it unchanged leaves the key identical, while changing one byte moves it. +"changed" means. It is deliberately not over the diff text. Which paths count is read from three +sides, the commit at HEAD, the real index, and the staged working tree, and each path's recorded +state is read from the last two alone. That split is what lets a review run before the commit while +the check runs at the push: reviewing untracked work and then committing it unchanged leaves the key +identical, while changing one byte moves it. Every identity in the key is computed by git rather than reconstructed here, which is the point worth stating plainly, because reconstructing one is where this went wrong twice. A blob id read @@ -27,7 +29,21 @@ `git commit` invisible to the key. When they diverge, because content is staged that the working tree no longer holds, or because a mode changed, the set has more than one member and the key moves. Reading the real index rather than the working tree alone is what stops content being -committed that the key never saw. +staged that the key never saw. + +That invisibility is a property of an untracked file rather than of `git add` in general. Staging a +**modified tracked** file collapses a state that named the index separately, since the index held +the base content and now holds the working tree's, so the key moves although no content changed. +The consequence for a caller is an ordering one: record a pass over what is already committed, and +staging can no longer move the key underneath it. + +HEAD is read for membership only, and never for a path's state. Without it, content already +committed and then undone in the tree leaves the changed set entirely, since the index, the working +tree, and the merge base then all agree the path is absent or unchanged while the commit a push +delivers still carries it. `git rm` a file the branch added is the shortest way there, and where it +was the only changed path the whole set empties. Keeping HEAD out of the state is what leaves the +ordinary commit invisible: a path already in the set through its index and working-tree state keeps +the same recorded state once HEAD holds that state too. What the key covers is the net content this branch introduces against its target, not the series of commits that produced it, and the difference is worth stating plainly because two consequences @@ -42,11 +58,22 @@ review. But a capture point built on this must not be described as covering the commit series, and a reviewer who needs to see intermediate churn has to read the range itself. -Two known cases move the key though nothing that would be pushed changed, both erring toward +Three known cases move the key though nothing that would be pushed changed, all erring toward demanding another review rather than skipping one. An intent-to-add entry (`git add -N`, which some tooling issues implicitly) sits in the real index as the empty blob while the throwaway index -holds the real content. And a new untracked file that `.gitignore` does not cover enters the -changed set, since a file not yet added is exactly what a review has to read. +holds the real content. A new untracked file that `.gitignore` does not cover enters the changed +set, since a file not yet added is exactly what a review has to read. And staging a modified tracked +file moves the key for the reason the paragraph above gives. + +What this key does not do, and what a push-time caller has to add for itself, is tie the content it +describes to the commit a push delivers. It reads the index and the working tree, and a push carries +HEAD, so the two describe the same content only while the working tree agrees with HEAD. Where they +disagree, HEAD can hold content the tree has since replaced, which a pass over the tree never read. +Membership puts that path in the digest and stops there, and a pass recorded in that state is +recorded over that same digest and covers it, so `check` answers covered and nothing here refuses. +Closing it is the caller's own test, not a better message from this one: `.husky/pre-push` compares +the working tree against HEAD and refuses the push itself, and a capture point that skips that test +has no protection against this whatever `check` says. This script holds no review logic. It drives backends: `agent-skill` is the `local-strict-review` subagent pass, which only a live agent session can run, so the engine records it rather than @@ -63,7 +90,8 @@ record that reviewed the content `status` reported as , refusing when the content has moved since. python3 scripts/local_review.py check [--target ] - exit 0 covered, 1 not covered, 2 could not run. + exit 0 covered, 1 not covered, 2 could not run. A branch holding no net content + against its target is covered, there being nothing for a review to read. python3 scripts/local_review.py run --backend coderabbit-cli [--target ] execute a headless backend and record its pass. @@ -414,20 +442,39 @@ def state_mark(index: set[str], work: set[str]) -> str: def fingerprints(base: str, root: Path) -> dict[str, str]: """Every changed path and the state it is in. - A path is changed when either side, the real index or the staged working tree, disagrees with - the merge base. Comparing states rather than reading a diff listing means renames, deletions, - mode changes, and submodule bumps are all just a state that differs, with no per-case handling - and no rename detection to disable. + A path is changed when any of three sides, the commit at HEAD, the real index, or the staged + working tree, disagrees with the merge base. Comparing states rather than reading a diff + listing means renames, deletions, mode changes, and submodule bumps are all just a state that + differs, with no per-case handling and no rename detection to disable. + + HEAD decides membership and contributes nothing to the mark, which is what callers can rely on + in both directions. A path already differing from the base by its index and working-tree state + keeps its mark once HEAD holds that state too, so `git commit` alone never moves the key. That + is `git commit` alone: `git add` on a modified tracked file does move it, by collapsing a mark + that named the index separately, so the invisible-commit property covers untracked work rather + than every edit. And a path in the set only because HEAD disagrees with the base leaves the set + when that undo is committed, so the key does move there, correctly, since a push delivered the + change before that commit and delivers the undo after it. + + What this is not is the content a push would deliver, which is HEAD's alone. It moves where a + push would carry nothing new, staging a modified tracked file being the plainest example, and it + holds still while HEAD's own content changes under a fixed index and working tree. A caller + gating a push closes that second gap itself, by testing the working tree against HEAD. """ at_base = base_states(base, root) + # HEAD is read so content committed and then undone in the tree cannot leave the changed set. + # `git rm` a committed file and the other three sides all agree it is absent. + # The commit a push delivers still carries it, emptying the set where it was the only path. + at_head = base_states("HEAD", root) at_index = index_states(root) at_work = worktree_states(root) out = {} - for path in {*at_base, *at_index, *at_work}: + for path in {*at_base, *at_head, *at_index, *at_work}: base_state = {at_base[path]} if path in at_base else set() + head_state = {at_head[path]} if path in at_head else set() index = at_index.get(path, set()) work = at_work.get(path, set()) - if index == work == base_state: + if index == work == base_state == head_state: continue out[path] = state_mark(index, work) return out @@ -839,7 +886,17 @@ def cmd_check(args: argparse.Namespace) -> int: root = repo_root() target = resolve_target(args.target) base, digest, changed = current_state(target, root) + # Read before the empty-change answer below, so an unreadable receipt still reports the boundary. + # Skipping it into a verdict would report covered for a check that never ran. receipt, problems = read_receipt(root) + # A branch holding no net content against its target introduces nothing for a review to read. + # The digest is then over the empty set, and a receipt against it would attest to nothing. + # Gating that would demand a review of an empty diff before a push git has nothing to reject. + # `status` keeps counting recorded passes only, so it reports covered false with changedPaths 0. + # The two are consistent, since `status` says what is recorded and this says what is left to gate. + if changed == 0: + emit(f"No net content against {target}, so there is nothing for a local review to cover.") + return EXIT_COVERED if covering_passes(receipt, base, digest, target): return EXIT_COVERED emit( @@ -857,6 +914,25 @@ def cmd_check(args: argparse.Namespace) -> int: ) for problem in problems: emit(f" receipt {problem}", sys.stderr) + # A receipt naming another branch is a scope disagreement, not a missing pass. + # The reviewer read a diff against the branch the receipt names, and this check measured another. + # Printing the ordinary remedy here hands over a line that records a pass over content nobody read. + # It runs, it succeeds, it replaces the correctly scoped receipt, and the next check passes. + # So the remedy is withheld and the disagreement is named, since only the caller knows which is right. + if receipt and not same_target(str(receipt.get("target")), target): + emit( + f"\nThe recorded pass was run against {receipt['target']} and this check measured" + f" {target}, so one of the two is the wrong scope.\n" + "No record command is offered here on purpose: recording this check's scope would" + " attest to a change set the review never read.\n" + f"If this branch does target {target}, the pass was run against the wrong base, and one" + f" more pass over the diff against {target}, recorded with --target" + f" {shlex.quote(target)}, clears this.\n" + f"If it does not target {target}, this check cannot judge it at all, and a caller that" + " gates on this exit code has to be bypassed rather than satisfied.", + sys.stderr, + ) + return EXIT_NOT_COVERED # Printed so it runs exactly as it stands. # That means the full digest rather than the abbreviated form above, the required # --expect-digest present, and no placeholder left to substitute. @@ -865,11 +941,18 @@ def cmd_check(args: argparse.Namespace) -> int: # The target is quoted because git permits a ref name to hold shell metacharacters. # Confirmed against a real repository for a semicolon, a command substitution, and an ampersand. # Interpolating one raw into a line the reader is invited to paste would let a branch name run commands. + # The caveat rides with the command rather than sitting in a doc, because this is the moment. + # Nothing here knows which branch the work targets, only which one this invocation measured. + # A branch based on anything else has a change set this scope does not describe. + # The line below would then record a pass over paths no reviewer was shown. emit( "\nRun the local-strict-review pass over this diff, then record it:\n" f" python3 scripts/local_review.py record --reviewer agent-skill" f" --target {shlex.quote(target)} --expect-digest {shlex.quote(digest)}\n" - "Add --findings to record how many it raised.", + "Add --findings to record how many it raised.\n" + f"That line records a pass against {target}, which is the branch this check measured rather" + " than one it discovered. Where the review read this branch against a different base, that" + " is the wrong scope and the line records content nobody looked at.", sys.stderr, ) return EXIT_NOT_COVERED diff --git a/scripts/tests/test_local_review.py b/scripts/tests/test_local_review.py index e85bbf5b..af48bbeb 100755 --- a/scripts/tests/test_local_review.py +++ b/scripts/tests/test_local_review.py @@ -456,6 +456,70 @@ def test_the_engine_is_correct_from_a_subdirectory(self) -> None: self.assertEqual(self.digest(), from_root, "the key changed with the working directory") +class HeadContentCase(RepoCase): + """Content the pushed commit carries that the index and working tree no longer show. + + The index and the working tree describe what is about to be committed, never what is already + committed, so a key built from those two alone stops seeing a path the moment the tree agrees + with the merge base again. The commit still carries it, and a push still delivers it. + """ + + def test_a_committed_file_removed_in_the_tree_stays_in_the_key(self) -> None: + (self.tmp / "payload.txt").write_text("secret\n", encoding="utf-8") + self.commit_all("add payload") + run(self.tmp, "rm", "-f", "payload.txt") + # Index, working tree, and merge base now all agree the path is absent. + # The commit that a push would deliver still holds it, which is what has to keep it in. + self.assertIn("payload.txt", self.marks()) + self.assertEqual(self.main_quiet(["check", "--target", self.target]), 1) + + def test_a_committed_edit_reverted_in_the_tree_invalidates_the_pass(self) -> None: + (self.tmp / "base.txt").write_text("changed\n", encoding="utf-8") + self.commit_all("edit base") + self.record("agent-skill") + self.assertEqual(self.main_quiet(["check", "--target", self.target]), 0) + # The commit keeps the edit, so the reviewed content is still what a push delivers. + # Restoring the file from the merge base is what a key blind to HEAD reads as no change. + run(self.tmp, "checkout", self.target, "--", "base.txt") + self.assertEqual(self.main_quiet(["check", "--target", self.target]), 1) + + def test_a_path_held_by_head_alone_leaves_the_set_when_the_undo_is_committed(self) -> None: + """The boundary the HEAD read actually moved, asserted rather than assumed harmless. + + A path in the set only because HEAD disagrees with the base is one the branch committed and + the tree has since put back. Committing that undo drops it, and the key moves. That is the + key following what a push delivers rather than the commit-boundary property breaking: before + the commit a push delivers the change, and after it a push delivers the undo. + """ + (self.tmp / "base.txt").write_text("changed\n", encoding="utf-8") + self.commit_all("edit base") + run(self.tmp, "checkout", self.target, "--", "base.txt") + held_by_head = self.marks() + self.assertIn("base.txt", held_by_head, "HEAD is no longer deciding membership") + before = self.digest() + self.commit_all("commit the undo") + self.assertNotIn("base.txt", self.marks()) + self.assertNotEqual(self.digest(), before, "the pushed content changed and the key did not") + + def test_the_commit_itself_still_leaves_the_key_alone(self) -> None: + """The half of the property that must survive, next to the half above that moved. + + Staged first, so the only thing between the two digests is the commit, which is the one + operation the HEAD read newly sees. Staging is left out on purpose: `git add` on a modified + tracked file collapses a mark that named the index separately, which moves the key by the + existing design `state_mark` documents and would hide what this case is measuring. + + It is a floor rather than a proof of the HEAD read, and it passes with that read reverted, + since both paths are in the set through their working-tree state either way. + """ + (self.tmp / "new.py").write_text("print('x')\n", encoding="utf-8") + (self.tmp / "base.txt").write_text("edited\n", encoding="utf-8") + run(self.tmp, "add", "-A") + before = self.digest() + run(self.tmp, "commit", "-m", "commit the reviewed work") + self.assertEqual(self.digest(), before) + + class StagingIsolationCase(RepoCase): def test_reading_the_worktree_does_not_touch_the_real_index(self) -> None: """The read stages the tree into a throwaway index, which must stay throwaway.""" @@ -482,6 +546,49 @@ def test_check_reports_not_covered_then_covered(self) -> None: self.record("agent-skill") self.assertEqual(self.main_quiet(["check", "--target", self.target]), 0) + def test_an_empty_change_set_needs_no_pass(self) -> None: + """A branch with no net content against its target has nothing for a review to read. + + Gating it would demand a review of an empty diff, and a receipt against that empty digest + would attest to nothing. `status` still reports what is recorded, which is nothing, so the + two answers must be read as the different questions they are. + """ + self.assertEqual(len(self.marks()), 0, "the fixture branch already carries content") + self.assertEqual(self.main_quiet(["check", "--target", self.target]), 0) + out = io.StringIO() + with contextlib.redirect_stdout(out), contextlib.redirect_stderr(io.StringIO()): + self.assertEqual(local_review.main(["status", "--target", self.target]), 0) + reported = json.loads(out.getvalue()) + self.assertEqual(reported["changedPaths"], 0) + self.assertFalse(reported["covered"], "status counts recorded passes, and none was") + + def test_an_unreadable_receipt_reports_the_boundary_even_with_nothing_to_gate(self) -> None: + """A receipt that cannot be read off disk is the check not running, whatever the change set. + + The empty-change answer is a verdict, so reaching it without having tried to read the + receipt would convert an execution boundary into a cheerful covered, which is the one + reading a gate must never take. + """ + self.assertEqual(len(self.marks()), 0, "the fixture branch already carries content") + path = local_review.receipt_path(self.tmp) + path.write_text("{}", encoding="utf-8") + path.chmod(0o000) + self.addCleanup(path.unlink) + self.addCleanup(path.chmod, 0o600) + if os.access(path, os.R_OK): + self.skipTest("this user reads a mode 000 file, so the boundary cannot be provoked") + self.assertEqual(self.main_quiet(["check", "--target", self.target]), 2) + + def test_the_empty_change_set_exemption_is_the_only_reason_that_push_passes(self) -> None: + """Proves the case above by the one byte that separates it from a gated branch. + + Without this, a branch that happened to carry no content would pass for the same reason a + reviewed one does, and the exemption would be untested where it actually fires. + """ + self.assertEqual(self.main_quiet(["check", "--target", self.target]), 0) + (self.tmp / "new.py").write_text("print('x')\n", encoding="utf-8") + self.assertEqual(self.main_quiet(["check", "--target", self.target]), 1) + def test_a_stale_pass_is_dropped_rather_than_carried(self) -> None: (self.tmp / "new.py").write_text("print('x')\n", encoding="utf-8") self.record("agent-skill") @@ -598,6 +705,32 @@ def test_record_refuses_content_that_moved_since_the_review(self) -> None: ) self.assertEqual(self.main_quiet(["check", "--target", self.target]), 1) + def test_a_receipt_naming_another_target_gets_no_paste_ready_remedy(self) -> None: + """The remedy is withheld precisely where following it would stamp unread content. + + A pass recorded against one branch and a check measuring another disagree about scope. The + ordinary remedy line records this check's scope, which runs, succeeds, replaces the + correctly scoped receipt, and passes the next check over a diff no reviewer read. + """ + (self.tmp / "new.py").write_text("print('x')\n", encoding="utf-8") + self.record("agent-skill", target="main") + err = io.StringIO() + with contextlib.redirect_stderr(err), contextlib.redirect_stdout(io.StringIO()): + self.assertEqual(local_review.main(["check", "--target", self.target]), 1) + printed = err.getvalue() + self.assertNotIn("local_review.py record", printed, "the unsafe remedy was offered anyway") + self.assertIn("wrong scope", printed) + + def test_the_remedy_still_appears_when_the_targets_agree(self) -> None: + """The floor under the case above, so withholding cannot quietly become withholding always.""" + (self.tmp / "new.py").write_text("print('x')\n", encoding="utf-8") + self.record("agent-skill") + (self.tmp / "new.py").write_text("print('y')\n", encoding="utf-8") + err = io.StringIO() + with contextlib.redirect_stderr(err), contextlib.redirect_stdout(io.StringIO()): + self.assertEqual(local_review.main(["check", "--target", self.target]), 1) + self.assertIn("local_review.py record", err.getvalue()) + def test_the_check_failure_prints_a_command_that_actually_runs(self) -> None: """The remedy line is the one actionable thing the failure emits.