From 481c354abe4eb15783bae25557918e492dc08f7e Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 2 Nov 2026 08:32:17 -0800 Subject: [PATCH 1/6] Bind a Local Review Record to the Reviewer's Report File `local_review.py record` now takes `--report`, the file the agent-skill reviewer wrote at the `reportPath` that `status` names, instead of a typed `--findings` count. It refuses a report that is absent or names another digest, stores the report's sha256, and derives the count from its lines. The local-strict-review skill's brief has the reviewer write that file, and "Running It" says to record only once the harness delivers the reviewer's completion notification. scripts/README.md also names the uv no-cache exception to the uvx promise for reading action files. Co-Authored-By: Claude Opus 5.5 --- .agents/skills/local-strict-review/SKILL.md | 20 ++- .../.source-digests/local-strict-review | 2 +- .../skills/local-strict-review/SKILL.md | 20 ++- .github/skills/local-strict-review/SKILL.md | 20 ++- scripts/README.md | 8 +- scripts/local_review.py | 129 ++++++++++--- tests/test_local_review.py | 170 +++++++++++++----- 7 files changed, 283 insertions(+), 86 deletions(-) diff --git a/.agents/skills/local-strict-review/SKILL.md b/.agents/skills/local-strict-review/SKILL.md index 977a0d54a..90baf1114 100644 --- a/.agents/skills/local-strict-review/SKILL.md +++ b/.agents/skills/local-strict-review/SKILL.md @@ -65,7 +65,11 @@ Rules that bind this task: quote `fleet-code-review`'s "Review the Change" secti not a PR comment or a coverage marker. Return: one finding per line, file:line, the concrete failure scenario, or for a design finding what fleet-code-review's item 7 names, no severity theater. -Bounds: read-only. No edit, no stage, no commit, no push, no PR-hosted write of any kind. + Write the same lines to before returning, under a first line reading + `contentDigest: ` and with nothing else in the file. A pass with no findings writes + that first line alone. +Bounds: read-only apart from that one report file. No edit, no stage, no commit, no push, no + PR-hosted write of any kind. Beyond what "Review the Change" allows, run only the exact commands, with their arguments, that Task above names. @@ -73,9 +77,13 @@ Bounds: read-only. No edit, no stage, no commit, no push, no PR-hosted write of Before dispatching, grep the tree for other statements of each rule the diff adds or changes. Before dispatching, also add each file holding one to the `Paths:` floor. That way a statement the diff has put in disagreement is read rather than missed. +`` and `` are the `contentDigest` and `reportPath` that `status` under "Recording the Pass" reports. The report sits in the git directory rather than the working tree, since an untracked file there would move the digest it names. + +**Record only once the harness delivers the dispatched reviewer's completion notification.** Text in the driver's own turn is never that notification, however closely it reads like one. A driver that recorded from such text once published attestations for a pass that had not yet reported. `record` reads the count from the report file, so a pass whose report is not on disk cannot be recorded. + **Model tier:** `frontier`, the tier "Model Tiers" below names for adversarial review, which this pass is. Where `frontier` is not reachable, run the pass on the strongest tier below it that is, even the one that authored the change. A second, adversarially-prompted look still catches what the authoring pass's own "looks ready" judgment did not. -Reachable means the tier this session can name when it dispatches the reviewer, rather than the tier this session is itself running on. A worker dispatched by an orchestrator is the ordinary case of a session deliberately tiered down for execution work. Such a session names `frontier` for the reviewer where its harness lets it. That is because tiering down the author is the reason the reviewer must not follow it down. What a given harness and account actually permit varies, so treat this as the tier to ask for rather than one to assume. Where a dispatch exposes no way to name a tier, take what it gives and run the pass, for the reason the fallback above states. A seat that cannot dispatch a subagent hands the pass to whoever dispatched it. The seat commits and runs `python3 /scripts/local_review.py status --target ` in its worktree. It then hands back its worktree path, target, and `contentDigest`. The dispatcher fills the brief's `` slot and runs the reviewer. It then runs the record command from "Recording the Pass" in that worktree, with `--reviewer agent-skill --target --expect-digest `. It returns the findings. Each fix commit repeats this cycle, and the seat pushes only when a record matches its content. The brief dispatching the seat states whether the dispatcher can continue that seat. Where the dispatcher cannot, the seat stops, reports, and does not push. Where nobody dispatched the seat, it reports to the maintainer that it could not run the pass, and does not push. Either way the push waits rather than the pass being quietly skipped. The headless `run --backend` route under "Recording the Pass" is not the substitute. That is because it runs a vendor CLI against its own review, which never carries the brief above. So it satisfies the rule this section states only where that separate route is what a capture point asked for. +Reachable means the tier this session can name when it dispatches the reviewer, rather than the tier this session is itself running on. A worker dispatched by an orchestrator is the ordinary case of a session deliberately tiered down for execution work. Such a session names `frontier` for the reviewer where its harness lets it. That is because tiering down the author is the reason the reviewer must not follow it down. What a given harness and account actually permit varies, so treat this as the tier to ask for rather than one to assume. Where a dispatch exposes no way to name a tier, take what it gives and run the pass, for the reason the fallback above states. A seat that cannot dispatch a subagent hands the pass to whoever dispatched it. The seat commits and runs `python3 /scripts/local_review.py status --target ` in its worktree. It then hands back its worktree path, target, `contentDigest`, and `reportPath`. The dispatcher fills the brief's ``, ``, and `` slots and runs the reviewer. Once the reviewer's completion notification arrives, it runs the record command from "Recording the Pass" in that worktree, with `--reviewer agent-skill --target --expect-digest --report `. It returns the findings. Each fix commit repeats this cycle, and the seat pushes only when a record matches its content. The brief dispatching the seat states whether the dispatcher can continue that seat. Where the dispatcher cannot, the seat stops, reports, and does not push. Where nobody dispatched the seat, it reports to the maintainer that it could not run the pass, and does not push. Either way the push waits rather than the pass being quietly skipped. The headless `run --backend` route under "Recording the Pass" is not the substitute. That is because it runs a vendor CLI against its own review, which never carries the brief above. So it satisfies the rule this section states only where that separate route is what a capture point asked for. ## Recording the Pass @@ -85,9 +93,9 @@ Commit first, then read the digest, then dispatch the subagent, then hand that s ```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 '' +python3 "$engine" status --target '' # JSON, take contentDigest and reportPath +# run the pass above, wait for its completion notification, then: +python3 "$engine" record --reviewer agent-skill --target '' --expect-digest '' --report '' ``` Every subcommand here, `run --backend ` included, runs with the repository under review as the working directory. That holds 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. @@ -96,7 +104,7 @@ Every subcommand here, `run --backend ` included, runs with the repository `--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. `--findings` is optional and records how many findings the pass raised. The key covers the net content the branch introduces against its target rather than the commit series. So a rebase that leaves the tree alone keeps the receipt valid while the fork point holds, and changing one byte invalidates it. The key holds that fork point too, so rebasing onto a target that has moved retires the receipt although no file changed. +Record the pass whatever it found, including nothing. `record` counts the report's findings from its lines and stores the report's sha256. It refuses a report that is absent or that names another digest. The key covers the net content the branch introduces against its target rather than the commit series. So a rebase that leaves the tree alone keeps the receipt valid while the fork point holds, and changing one byte invalidates it. The key holds that fork point too, so rebasing onto a target that has moved retires the receipt although no file changed. **Why the commit comes first**, rather than being an ordering that could equally run the other way. A push delivers the commit, and the hook's tree check refuses a push whose tracked content differs from HEAD. So the record has to describe what HEAD holds. A commit that leaves the tree alone usually does not move the receipt's key, so diligence done before it still describes the same content. A commit putting a path back to its base state drops the path from the change set and does move the key. The order matters anyway, for these reasons. 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. This is because the digest read and the record bracket a window in which the tree holds still. A commit inside that window ends it. diff --git a/.claude-plugin/fleet-skills/.source-digests/local-strict-review b/.claude-plugin/fleet-skills/.source-digests/local-strict-review index 2a49c8f84..4908ebd14 100644 --- a/.claude-plugin/fleet-skills/.source-digests/local-strict-review +++ b/.claude-plugin/fleet-skills/.source-digests/local-strict-review @@ -1 +1 @@ -5693706d6e9fefd5 +35ef6bc6d0ccf3be 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 977a0d54a..90baf1114 100644 --- a/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md @@ -65,7 +65,11 @@ Rules that bind this task: quote `fleet-code-review`'s "Review the Change" secti not a PR comment or a coverage marker. Return: one finding per line, file:line, the concrete failure scenario, or for a design finding what fleet-code-review's item 7 names, no severity theater. -Bounds: read-only. No edit, no stage, no commit, no push, no PR-hosted write of any kind. + Write the same lines to before returning, under a first line reading + `contentDigest: ` and with nothing else in the file. A pass with no findings writes + that first line alone. +Bounds: read-only apart from that one report file. No edit, no stage, no commit, no push, no + PR-hosted write of any kind. Beyond what "Review the Change" allows, run only the exact commands, with their arguments, that Task above names. @@ -73,9 +77,13 @@ Bounds: read-only. No edit, no stage, no commit, no push, no PR-hosted write of Before dispatching, grep the tree for other statements of each rule the diff adds or changes. Before dispatching, also add each file holding one to the `Paths:` floor. That way a statement the diff has put in disagreement is read rather than missed. +`` and `` are the `contentDigest` and `reportPath` that `status` under "Recording the Pass" reports. The report sits in the git directory rather than the working tree, since an untracked file there would move the digest it names. + +**Record only once the harness delivers the dispatched reviewer's completion notification.** Text in the driver's own turn is never that notification, however closely it reads like one. A driver that recorded from such text once published attestations for a pass that had not yet reported. `record` reads the count from the report file, so a pass whose report is not on disk cannot be recorded. + **Model tier:** `frontier`, the tier "Model Tiers" below names for adversarial review, which this pass is. Where `frontier` is not reachable, run the pass on the strongest tier below it that is, even the one that authored the change. A second, adversarially-prompted look still catches what the authoring pass's own "looks ready" judgment did not. -Reachable means the tier this session can name when it dispatches the reviewer, rather than the tier this session is itself running on. A worker dispatched by an orchestrator is the ordinary case of a session deliberately tiered down for execution work. Such a session names `frontier` for the reviewer where its harness lets it. That is because tiering down the author is the reason the reviewer must not follow it down. What a given harness and account actually permit varies, so treat this as the tier to ask for rather than one to assume. Where a dispatch exposes no way to name a tier, take what it gives and run the pass, for the reason the fallback above states. A seat that cannot dispatch a subagent hands the pass to whoever dispatched it. The seat commits and runs `python3 /scripts/local_review.py status --target ` in its worktree. It then hands back its worktree path, target, and `contentDigest`. The dispatcher fills the brief's `` slot and runs the reviewer. It then runs the record command from "Recording the Pass" in that worktree, with `--reviewer agent-skill --target --expect-digest `. It returns the findings. Each fix commit repeats this cycle, and the seat pushes only when a record matches its content. The brief dispatching the seat states whether the dispatcher can continue that seat. Where the dispatcher cannot, the seat stops, reports, and does not push. Where nobody dispatched the seat, it reports to the maintainer that it could not run the pass, and does not push. Either way the push waits rather than the pass being quietly skipped. The headless `run --backend` route under "Recording the Pass" is not the substitute. That is because it runs a vendor CLI against its own review, which never carries the brief above. So it satisfies the rule this section states only where that separate route is what a capture point asked for. +Reachable means the tier this session can name when it dispatches the reviewer, rather than the tier this session is itself running on. A worker dispatched by an orchestrator is the ordinary case of a session deliberately tiered down for execution work. Such a session names `frontier` for the reviewer where its harness lets it. That is because tiering down the author is the reason the reviewer must not follow it down. What a given harness and account actually permit varies, so treat this as the tier to ask for rather than one to assume. Where a dispatch exposes no way to name a tier, take what it gives and run the pass, for the reason the fallback above states. A seat that cannot dispatch a subagent hands the pass to whoever dispatched it. The seat commits and runs `python3 /scripts/local_review.py status --target ` in its worktree. It then hands back its worktree path, target, `contentDigest`, and `reportPath`. The dispatcher fills the brief's ``, ``, and `` slots and runs the reviewer. Once the reviewer's completion notification arrives, it runs the record command from "Recording the Pass" in that worktree, with `--reviewer agent-skill --target --expect-digest --report `. It returns the findings. Each fix commit repeats this cycle, and the seat pushes only when a record matches its content. The brief dispatching the seat states whether the dispatcher can continue that seat. Where the dispatcher cannot, the seat stops, reports, and does not push. Where nobody dispatched the seat, it reports to the maintainer that it could not run the pass, and does not push. Either way the push waits rather than the pass being quietly skipped. The headless `run --backend` route under "Recording the Pass" is not the substitute. That is because it runs a vendor CLI against its own review, which never carries the brief above. So it satisfies the rule this section states only where that separate route is what a capture point asked for. ## Recording the Pass @@ -85,9 +93,9 @@ Commit first, then read the digest, then dispatch the subagent, then hand that s ```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 '' +python3 "$engine" status --target '' # JSON, take contentDigest and reportPath +# run the pass above, wait for its completion notification, then: +python3 "$engine" record --reviewer agent-skill --target '' --expect-digest '' --report '' ``` Every subcommand here, `run --backend ` included, runs with the repository under review as the working directory. That holds 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. @@ -96,7 +104,7 @@ Every subcommand here, `run --backend ` included, runs with the repository `--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. `--findings` is optional and records how many findings the pass raised. The key covers the net content the branch introduces against its target rather than the commit series. So a rebase that leaves the tree alone keeps the receipt valid while the fork point holds, and changing one byte invalidates it. The key holds that fork point too, so rebasing onto a target that has moved retires the receipt although no file changed. +Record the pass whatever it found, including nothing. `record` counts the report's findings from its lines and stores the report's sha256. It refuses a report that is absent or that names another digest. The key covers the net content the branch introduces against its target rather than the commit series. So a rebase that leaves the tree alone keeps the receipt valid while the fork point holds, and changing one byte invalidates it. The key holds that fork point too, so rebasing onto a target that has moved retires the receipt although no file changed. **Why the commit comes first**, rather than being an ordering that could equally run the other way. A push delivers the commit, and the hook's tree check refuses a push whose tracked content differs from HEAD. So the record has to describe what HEAD holds. A commit that leaves the tree alone usually does not move the receipt's key, so diligence done before it still describes the same content. A commit putting a path back to its base state drops the path from the change set and does move the key. The order matters anyway, for these reasons. 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. This is because the digest read and the record bracket a window in which the tree holds still. A commit inside that window ends it. diff --git a/.github/skills/local-strict-review/SKILL.md b/.github/skills/local-strict-review/SKILL.md index 977a0d54a..90baf1114 100644 --- a/.github/skills/local-strict-review/SKILL.md +++ b/.github/skills/local-strict-review/SKILL.md @@ -65,7 +65,11 @@ Rules that bind this task: quote `fleet-code-review`'s "Review the Change" secti not a PR comment or a coverage marker. Return: one finding per line, file:line, the concrete failure scenario, or for a design finding what fleet-code-review's item 7 names, no severity theater. -Bounds: read-only. No edit, no stage, no commit, no push, no PR-hosted write of any kind. + Write the same lines to before returning, under a first line reading + `contentDigest: ` and with nothing else in the file. A pass with no findings writes + that first line alone. +Bounds: read-only apart from that one report file. No edit, no stage, no commit, no push, no + PR-hosted write of any kind. Beyond what "Review the Change" allows, run only the exact commands, with their arguments, that Task above names. @@ -73,9 +77,13 @@ Bounds: read-only. No edit, no stage, no commit, no push, no PR-hosted write of Before dispatching, grep the tree for other statements of each rule the diff adds or changes. Before dispatching, also add each file holding one to the `Paths:` floor. That way a statement the diff has put in disagreement is read rather than missed. +`` and `` are the `contentDigest` and `reportPath` that `status` under "Recording the Pass" reports. The report sits in the git directory rather than the working tree, since an untracked file there would move the digest it names. + +**Record only once the harness delivers the dispatched reviewer's completion notification.** Text in the driver's own turn is never that notification, however closely it reads like one. A driver that recorded from such text once published attestations for a pass that had not yet reported. `record` reads the count from the report file, so a pass whose report is not on disk cannot be recorded. + **Model tier:** `frontier`, the tier "Model Tiers" below names for adversarial review, which this pass is. Where `frontier` is not reachable, run the pass on the strongest tier below it that is, even the one that authored the change. A second, adversarially-prompted look still catches what the authoring pass's own "looks ready" judgment did not. -Reachable means the tier this session can name when it dispatches the reviewer, rather than the tier this session is itself running on. A worker dispatched by an orchestrator is the ordinary case of a session deliberately tiered down for execution work. Such a session names `frontier` for the reviewer where its harness lets it. That is because tiering down the author is the reason the reviewer must not follow it down. What a given harness and account actually permit varies, so treat this as the tier to ask for rather than one to assume. Where a dispatch exposes no way to name a tier, take what it gives and run the pass, for the reason the fallback above states. A seat that cannot dispatch a subagent hands the pass to whoever dispatched it. The seat commits and runs `python3 /scripts/local_review.py status --target ` in its worktree. It then hands back its worktree path, target, and `contentDigest`. The dispatcher fills the brief's `` slot and runs the reviewer. It then runs the record command from "Recording the Pass" in that worktree, with `--reviewer agent-skill --target --expect-digest `. It returns the findings. Each fix commit repeats this cycle, and the seat pushes only when a record matches its content. The brief dispatching the seat states whether the dispatcher can continue that seat. Where the dispatcher cannot, the seat stops, reports, and does not push. Where nobody dispatched the seat, it reports to the maintainer that it could not run the pass, and does not push. Either way the push waits rather than the pass being quietly skipped. The headless `run --backend` route under "Recording the Pass" is not the substitute. That is because it runs a vendor CLI against its own review, which never carries the brief above. So it satisfies the rule this section states only where that separate route is what a capture point asked for. +Reachable means the tier this session can name when it dispatches the reviewer, rather than the tier this session is itself running on. A worker dispatched by an orchestrator is the ordinary case of a session deliberately tiered down for execution work. Such a session names `frontier` for the reviewer where its harness lets it. That is because tiering down the author is the reason the reviewer must not follow it down. What a given harness and account actually permit varies, so treat this as the tier to ask for rather than one to assume. Where a dispatch exposes no way to name a tier, take what it gives and run the pass, for the reason the fallback above states. A seat that cannot dispatch a subagent hands the pass to whoever dispatched it. The seat commits and runs `python3 /scripts/local_review.py status --target ` in its worktree. It then hands back its worktree path, target, `contentDigest`, and `reportPath`. The dispatcher fills the brief's ``, ``, and `` slots and runs the reviewer. Once the reviewer's completion notification arrives, it runs the record command from "Recording the Pass" in that worktree, with `--reviewer agent-skill --target --expect-digest --report `. It returns the findings. Each fix commit repeats this cycle, and the seat pushes only when a record matches its content. The brief dispatching the seat states whether the dispatcher can continue that seat. Where the dispatcher cannot, the seat stops, reports, and does not push. Where nobody dispatched the seat, it reports to the maintainer that it could not run the pass, and does not push. Either way the push waits rather than the pass being quietly skipped. The headless `run --backend` route under "Recording the Pass" is not the substitute. That is because it runs a vendor CLI against its own review, which never carries the brief above. So it satisfies the rule this section states only where that separate route is what a capture point asked for. ## Recording the Pass @@ -85,9 +93,9 @@ Commit first, then read the digest, then dispatch the subagent, then hand that s ```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 '' +python3 "$engine" status --target '' # JSON, take contentDigest and reportPath +# run the pass above, wait for its completion notification, then: +python3 "$engine" record --reviewer agent-skill --target '' --expect-digest '' --report '' ``` Every subcommand here, `run --backend ` included, runs with the repository under review as the working directory. That holds 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. @@ -96,7 +104,7 @@ Every subcommand here, `run --backend ` included, runs with the repository `--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. `--findings` is optional and records how many findings the pass raised. The key covers the net content the branch introduces against its target rather than the commit series. So a rebase that leaves the tree alone keeps the receipt valid while the fork point holds, and changing one byte invalidates it. The key holds that fork point too, so rebasing onto a target that has moved retires the receipt although no file changed. +Record the pass whatever it found, including nothing. `record` counts the report's findings from its lines and stores the report's sha256. It refuses a report that is absent or that names another digest. The key covers the net content the branch introduces against its target rather than the commit series. So a rebase that leaves the tree alone keeps the receipt valid while the fork point holds, and changing one byte invalidates it. The key holds that fork point too, so rebasing onto a target that has moved retires the receipt although no file changed. **Why the commit comes first**, rather than being an ordering that could equally run the other way. A push delivers the commit, and the hook's tree check refuses a push whose tracked content differs from HEAD. So the record has to describe what HEAD holds. A commit that leaves the tree alone usually does not move the receipt's key, so diligence done before it still describes the same content. A commit putting a path back to its base state drops the path from the change set and does move the key. The order matters anyway, for these reasons. 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. This is because the digest read and the record bracket a window in which the tree holds still. A commit inside that window ends it. diff --git a/scripts/README.md b/scripts/README.md index afd52bfa3..46af33e5c 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -1,6 +1,6 @@ # Repo Scripts -The fleet's checks and review tooling, run by hand, with the deterministic ones also gating CI. Each one exists because the CI linters pass on the failure it catches. `markdownlint`, `cspell`, `actionlint`, and `editorconfig-checker` all report clean on prose that breaks a documented [`GOVERNANCE.md`][governance] rule. The pre-commit hook runs two deterministic doc gates, each at the scope that fits it. `uv run --no-project --script .github/actions/prose-gate/prose_lint.py . --diff HEAD` is diff-scoped, at about 0.15 seconds where its whole-tree sweep costs about 2.2. `python3 scripts/repo_gate.py --check eol` is repo-wide, since it reads `.gitattributes` against `.editorconfig` and takes no file list, and at about 0.04 seconds there is nothing to scope. The earlier decision to keep doc linters out was made when a sweep was the only mode. What reversed it is the diff scope rather than a change of preference. The gates needing Docker, and `sha-pin` which resolves a pin against the GitHub API, stay in CI. A plain `repo_gate.py` run also includes `composite-actions`, whose shell half needs Docker and either PyYAML or `uvx`, and whose schema half needs `uvx` or `pipx`. The hook reads the working tree rather than the index, so a partially staged file is judged on all of its edits. The pull request gate reads every line the branch changes against its base. A merge commit is diffed against `MERGE_HEAD` rather than `HEAD`, so the lines the merged-in parent brought are not read as added. +The fleet's checks and review tooling, run by hand, with the deterministic ones also gating CI. Each one exists because the CI linters pass on the failure it catches. `markdownlint`, `cspell`, `actionlint`, and `editorconfig-checker` all report clean on prose that breaks a documented [`GOVERNANCE.md`][governance] rule. The pre-commit hook runs two deterministic doc gates, each at the scope that fits it. `uv run --no-project --script .github/actions/prose-gate/prose_lint.py . --diff HEAD` is diff-scoped, at about 0.15 seconds where its whole-tree sweep costs about 2.2. `python3 scripts/repo_gate.py --check eol` is repo-wide, since it reads `.gitattributes` against `.editorconfig` and takes no file list, and at about 0.04 seconds there is nothing to scope. The earlier decision to keep doc linters out was made when a sweep was the only mode. What reversed it is the diff scope rather than a change of preference. The gates needing Docker, and `sha-pin` which resolves a pin against the GitHub API, stay in CI. A plain `repo_gate.py` run also includes `composite-actions`, whose shell half needs Docker and either PyYAML or `uvx`, and whose schema half needs `uvx` or `pipx`. `uvx` alone is not enough for the shell half on a host that sets `UV_NO_CACHE` or uv's `no-cache = true`. There uv deletes the environment it resolved before the reader launches. Make PyYAML importable there or turn the setting off. The hook reads the working tree rather than the index, so a partially staged file is judged on all of its edits. The pull request gate reads every line the branch changes against its base. A merge commit is diffed against `MERGE_HEAD` rather than `HEAD`, so the lines the merged-in parent brought are not read as added. **Hosted here and reached, never carried.** These are not declared in [`spec/files.json`][files], so the audit does not expect a downstream repo to ship them, the same footing as `spec/audit.py`. That is the fleet model rather than an omission: a gate holding no per-repo content has one canonical implementation for the fleet. The prose and repository gate implementations live beside their composite actions under `.github/actions/`. That placement lets reusable workflows reach the implementation at their pinned hub commit through `$/.github/actions/` without checking out the hub. The same-named files in `scripts/` are thin local entry points that call those action-owned implementations. They let maintainers and hooks retain the established commands from a hub checkout without creating a second copy of either gate. A repository that cannot reach the hub reports a local check as not run rather than reconstructing it. That repository does so because a rebuilt gate encodes its author's reading of the rule and agrees with no other repository. @@ -125,7 +125,7 @@ Deterministic checks: - `sha-pin`: every external action or reusable-workflow `uses:` reference is a 40-hex commit SHA, with the documented `dotnet/nbgv@master` exception. References under the scanned repository's owner are also resolved through GitHub. References under another owner are shape-checked only. Local (`./`) and self-repository (`$/`) references name no ref, so they take no pin, and a quoted one is recognized once its quotes are stripped. They resolve differently, though. A `$/` reference and a job-level `./` reusable-workflow call resolve at the calling workflow file's commit. A `./` action reference resolves against whatever the job checked out. That is the caller's tree where a reusable workflow called from another repository checks out its caller. A reference starting with a bare `.github/` is reported, since GitHub reads it as `owner/repo` rather than as a path. - `eol`: every path pinned LF in [`.gitattributes`][gitattributes] has the matching [`.editorconfig`][editorconfig] override the line-ending rule requires, with EditorConfig brace syntax expanded. One direction only. An `.editorconfig` LF glob with no git pin is legitimate. That is because `.editorconfig` governs what the editor writes where git enforces a class it must not guess at. - `eol-coverage`: the same pins read against the tree instead. A tracked file opening `#!` that git does not resolve to `eol=lf` is an interpreter line a CRLF checkout breaks. A pin matching no tracked file is dead unless its block is marked `forward-declared`. -- `composite-actions`: every tracked `action.yml` or `action.yaml` under `.github/actions/`, at any depth, is shellchecked and schema-checked, because actionlint reads workflows only. Each non-empty `run:` body of a bash, sh, or unset-shell step goes to shellcheck. Every `${{ }}` expression is replaced by a placeholder first. A finding names the action file and the step. A pwsh or python step is skipped. The file set goes to `check-jsonschema`'s `vendor.github-actions` builtin through `uvx check-jsonschema@latest`, or `pipx run` where `uvx` is absent. Shellcheck runs from the `mirror.gcr.io/koalaman/shellcheck:stable` image, so the check needs Docker. It reads the action through PyYAML, in process where it is importable and through `uvx --with pyyaml` otherwise. Each half reports its own missing tool. Bodies run at warning severity without SC2154, so this is weaker than shellcheck over a `.sh` file. A tree with no action file is skipped with a note. +- `composite-actions`: every tracked `action.yml` or `action.yaml` under `.github/actions/`, at any depth, is shellchecked and schema-checked, because actionlint reads workflows only. Each non-empty `run:` body of a bash, sh, or unset-shell step goes to shellcheck. Every `${{ }}` expression is replaced by a placeholder first. A finding names the action file and the step. A pwsh or python step is skipped. The file set goes to `check-jsonschema`'s `vendor.github-actions` builtin through `uvx check-jsonschema@latest`, or `pipx run` where `uvx` is absent. Shellcheck runs from the `mirror.gcr.io/koalaman/shellcheck:stable` image, so the check needs Docker. It reads the action through PyYAML, in process where it is importable and through `uvx --with pyyaml` otherwise. `uvx` alone is not enough on a host that sets `UV_NO_CACHE` or uv's `no-cache = true`. There uv deletes the environment it resolved before the reader launches. Make PyYAML importable there or turn the setting off. Each half reports its own missing tool. Bodies run at warning severity without SC2154, so this is weaker than shellcheck over a `.sh` file. A tree with no action file is skipped with a note. - `carried-trees`: each tree [`spec/files.json`][files] declares is compared byte-for-byte with the hub tree beside the gate. Only a tree whose target holds tracked files the tree owns is compared. A tree that prunes owns every file under its target, and one that does not owns the files its `include` matches. The hub tree is the pinned action's in CI and the checkout's in a local run. The comparison reuses [`carry.py`](#carrypy)'s tree functions, so `include` and `prune` read as a carry reads them. A tree that cannot be compared says so in a note, such as one whose hub source holds a link. So does a `carry.py` that will not load. A difference is a `note:` naming its counts and first path, plus a `::warning` annotation on Actions. It is never a finding, so a hub fix does not fail every downstream pull request. A byte difference cannot tell a stale copy from an edited one, so the audit tells them apart and the resync applies the refresh. In CI the comparison is with the pin, so a copy refreshed from a newer hub `main` also warns until Dependabot advances the pin. An `--exclude` pattern narrows this scan like the others, so one covering part of a carried target reads the files it hides as missing. Which trees a repository owes is the audit's, read from its declared types, so a tree the repository does not track is not compared. A note and a warning name a target tracked by Git as a link or a file. So does such an ancestor below the top level. Git then lists nothing beneath it. A link anywhere under a carried target is refused in a note and a warning, as a carry refuses it. A single-file fetch has no hub tree beside it, and says so in a note. A repository tracking `spec/files.json` is a hub, so it compares nothing, since its trees are the sources. ```sh @@ -356,11 +356,13 @@ What the key covers is the net content the branch introduces against its target, `record` requires `--expect-digest`, the digest `status` reported before the pass ran, and refuses when the content has moved since. It is required rather than optional because an optional guard is the one a caller in a hurry omits. The omission looks exactly like a pass that was properly bound. Nothing else binds the moment a review finished to the moment it is recorded. A format-on-save or a hook autofix in between would otherwise be stamped as reviewed content no reviewer saw. That stamp is the one claim the receipt exists to make. A headless backend is recorded by running it rather than by hand. That is because recording it directly would bypass the completion check that makes its pass mean anything. +`record` also requires `--report`, the file the `agent-skill` reviewer wrote at the `reportPath` that `status` names. It never takes a typed count. A typed count binds the record to nothing the reviewer produced. So a driver that misread or imagined a review could attest to one that never reported. The report's first line is `contentDigest: `, and each later non-blank line is one finding. `record` refuses a report that is absent or names another digest. Otherwise it stores the report's sha256 and derives the count from those lines. The report sits in the git directory beside the receipt, since an untracked file in the working tree would move the digest it names. A record removes the reports written for any other digest. + It 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 invoking it. `coderabbit-cli` is headless and is the one backend a git hook can execute by itself. A CodeRabbit CLI run draws on the same hourly budget as this account's pull request reviews. So a run that is rate limited, errors, or ends without a completion event records no pass rather than a clean one. The invocation follows the CLI's documented flag semantics. It passes `--base-commit` for the merge-base hash, since `--base` takes a branch name. It also passes `--include-untracked`, since the CLI excludes untracked files by default while this receipt's scope includes them. A completion reporting `review_skipped` is refused too, that being the CLI saying it looked at nothing. None of it has been executed against a real CLI on this host, so the backend stays opt-in until someone runs it. A recorded pass means a review ran over exactly this content, never that the content is clean. Disposing of what a review found is judgment, per the `pr-review-conduct` Skill's outcomes, so a pass is recorded whether it raised findings or none. ```shell python3 scripts/local_review.py status # what covers the current content. 0 either way, 2 if it cannot run -python3 scripts/local_review.py record --reviewer agent-skill --target develop --findings 3 --expect-digest '' +python3 scripts/local_review.py record --reviewer agent-skill --target develop --expect-digest '' --report '' python3 scripts/local_review.py check # 0 covered, 1 not, 2 could not run python3 scripts/local_review.py run --backend coderabbit-cli ``` diff --git a/scripts/local_review.py b/scripts/local_review.py index bc808d66a..dd396230a 100755 --- a/scripts/local_review.py +++ b/scripts/local_review.py @@ -83,12 +83,19 @@ Disposing of what a review found is judgment, per `pr-review-conduct`'s outcomes, so a pass is recorded whether the pass raised findings or none. +An `agent-skill` pass is recorded from the report its reviewer wrote, never from a count the caller +types. A typed count binds the record to nothing the reviewer produced. So a driver that misread or +imagined a review could record one that never reported. The report opens with the digest it was +written for, and each later non-blank line is one finding. `record` refuses a report that is absent +or names another digest. It stores the report's sha256 and derives the count from its lines. + Usage: python3 scripts/local_review.py status [--target ''] - what the current content digest is, and which backends hold a pass on it. + the content digest, which backends hold a pass on it, and its reportPath. python3 scripts/local_review.py record --reviewer '' --expect-digest '' - [--target ''] [--findings ''] - record that reviewed the content `status` reported as . It refuses when the - content has moved since. + --report '' [--target ''] + record that reviewed the content `status` reported as . The count comes + from the report at . It refuses when the content has moved or the report does not + match. python3 scripts/local_review.py check [--target ''] 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. @@ -128,6 +135,9 @@ # Kept in the git directory rather than the working tree, so staging the tree does not stage these. STAGING_INDEX_PREFIX = "local-review-index." STAGING_OBJECTS_PREFIX = "local-review-objects." +REPORT_PREFIX = "local-review-report." +REPORT_SUFFIX = ".txt" +REPORT_HEADER = "contentDigest: " DEFAULT_TARGET = "develop" EXIT_COVERED = 0 @@ -295,6 +305,57 @@ def receipt_path(root: Path) -> Path: return git_dir(root) / RECEIPT_NAME +def report_path(root: Path, digest: str) -> Path: + """Where an `agent-skill` reviewer writes its report over the content `digest` names.""" + return git_dir(root) / f"{REPORT_PREFIX}{digest}{REPORT_SUFFIX}" + + +def read_report(path: Path, digest: str) -> tuple[str, int]: + """The report's sha256 and finding count, refusing a report not written for `digest`. + + The first line names the digest the reviewer was handed, and every later non-blank line is one + finding. So a pass with nothing to report writes the first line alone. A blank line counts as + nothing, and any other line counts as a finding. That errs toward a count too high rather than + a clean pass nobody wrote. + """ + try: + raw = path.read_bytes() + except FileNotFoundError as e: + raise CannotRun( + f"no report at {path}, so no review has reported over this content." + " Record only once the dispatched reviewer has finished and written it" + ) from e + except OSError as e: + raise CannotRun(f"the report at {path} could not be read ({e})") from e + try: + lines = raw.decode("utf-8").splitlines() + except UnicodeDecodeError as e: + raise CannotRun(f"the report at {path} is not UTF-8 text ({e})") from e + header = lines[0].rstrip() if lines else "" + if not header.startswith(REPORT_HEADER): + raise CannotRun(f"the report at {path} does not open with a '{REPORT_HEADER}' line") + written_for = header.removeprefix(REPORT_HEADER).strip() + if written_for != digest: + raise CannotRun( + f"the report at {path} was written for {written_for[:12] or 'no digest'}," + f" not for {digest[:12]}, so it reviewed other content" + ) + findings = sum(1 for line in lines[1:] if line.strip()) + return hashlib.sha256(raw).hexdigest(), findings + + +def prune_reports(root: Path, keep: Path) -> None: + """Remove the reports written for any digest other than the one just recorded. + + A report over content that no longer exists can never be recorded again, the same reason the + receipt drops a pass over it. Without this, a checkout that outlives many reviews would collect + one report per review. + """ + for stale in git_dir(root).glob(f"{REPORT_PREFIX}*{REPORT_SUFFIX}"): + if stale != keep: + stale.unlink(missing_ok=True) + + def ref_exists(ref: str, root: Path) -> bool: """Whether `ref` resolves to a commit in this checkout.""" try: @@ -797,7 +858,13 @@ def held_lock(path: Path, timeout: float = 10.0) -> int: def write_pass( - root: Path, target: str, base: str, digest: str, reviewer: str, findings: int | None + root: Path, + target: str, + base: str, + digest: str, + reviewer: str, + findings: int | None, + report_sha256: str | None = None, ) -> dict: """Add one backend's pass to the receipt, dropping any pass that no longer covers the content. @@ -811,25 +878,32 @@ def write_pass( path = receipt_path(root) fd_lock = held_lock(path) try: - return _write_pass_locked(root, target, base, digest, reviewer, findings) + return _write_pass_locked(root, target, base, digest, reviewer, findings, report_sha256) finally: os.close(fd_lock) Path(str(path) + ".lock").unlink(missing_ok=True) def _write_pass_locked( - root: Path, target: str, base: str, digest: str, reviewer: str, findings: int | None + root: Path, + target: str, + base: str, + digest: str, + reviewer: str, + findings: int | None, + report_sha256: str | None = None, ) -> dict: """The read, merge, and replace that `write_pass` holds the lock around.""" receipt, _ = read_receipt(root) kept = [p for p in covering_passes(receipt, base, digest, target) if p["reviewer"] != reviewer] - kept.append( - { - "reviewer": reviewer, - "recordedUtc": datetime.now(UTC).strftime("%Y-%m-%dT%H:%M:%SZ"), - "findings": findings, - } - ) + entry: dict = { + "reviewer": reviewer, + "recordedUtc": datetime.now(UTC).strftime("%Y-%m-%dT%H:%M:%SZ"), + "findings": findings, + } + if report_sha256 is not None: + entry["reportSha256"] = report_sha256 + kept.append(entry) out = { "receiptVersion": RECEIPT_VERSION, "target": target, @@ -969,6 +1043,7 @@ def cmd_status(args: argparse.Namespace) -> int: "reviewers": [p["reviewer"] for p in passes], "findings": {p["reviewer"]: p.get("findings") for p in passes}, "receiptProblems": problems, + "reportPath": str(report_path(root, digest)), }, indent=2, ) @@ -990,9 +1065,6 @@ def cmd_record(args: argparse.Namespace) -> int: sys.stderr, ) return EXIT_CANNOT_RUN - if args.findings is not None and args.findings < 0: - emit(f"--findings cannot be negative, got {args.findings}", sys.stderr) - return EXIT_CANNOT_RUN root = repo_root() target = resolve_target(args.target) base, digest, changed = current_state(target, root) @@ -1012,11 +1084,13 @@ def cmd_record(args: argparse.Namespace) -> int: sys.stderr, ) return EXIT_CANNOT_RUN - out = write_pass(root, target, base, digest, args.reviewer, args.findings) + sha256, findings = read_report(Path(args.report), digest) + out = write_pass(root, target, base, digest, args.reviewer, findings, sha256) + prune_reports(root, report_path(root, digest)) reviewers = ", ".join(p["reviewer"] for p in out["passes"]) emit( f"recorded {args.reviewer} over {changed} changed path(s) against {target}" - f" at {digest[:12]}. Passes now: {reviewers}" + f" at {digest[:12]}, {findings} finding(s) from its report. Passes now: {reviewers}" ) return EXIT_COVERED @@ -1075,7 +1149,6 @@ def cmd_check(args: argparse.Namespace) -> int: # 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. - # --findings is optional, so it is left out rather than shown as something to fill in. # A placeholder would make the one actionable line here a command nobody can paste. # 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. @@ -1085,10 +1158,11 @@ def cmd_check(args: argparse.Namespace) -> int: # 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" + "\nRun the local-strict-review pass over this diff, have it write its report to the" + " path below, and once it has finished, 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.\n" + f" --target {shlex.quote(target)} --expect-digest {shlex.quote(digest)}" + f" --report {shlex.quote(str(report_path(root, digest)))}\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.", @@ -1148,12 +1222,19 @@ def main(argv: list[str] | None = None) -> int: p_record.add_argument( "--reviewer", required=True, help=f"one of: {', '.join(sorted(BACKENDS))}" ) - p_record.add_argument("--findings", type=int, default=None, help="how many findings it raised") p_record.add_argument( "--expect-digest", required=True, help="the contentDigest the review was run against, refusing to record if it has moved", ) + p_record.add_argument( + "--report", + required=True, + help=( + "the report the reviewer wrote, normally the reportPath `status` names. Its first line" + f" is '{REPORT_HEADER}' and each later non-blank line is one finding" + ), + ) p_record.set_defaults(func=cmd_record) p_check = sub.add_parser("check", help="exit 0 covered, 1 not covered, 2 could not run") diff --git a/tests/test_local_review.py b/tests/test_local_review.py index 4fb6befcd..d74a0c342 100755 --- a/tests/test_local_review.py +++ b/tests/test_local_review.py @@ -14,6 +14,7 @@ """ import contextlib +import hashlib import io import json import os @@ -108,6 +109,26 @@ def record(self, reviewer: str, findings: int | None = 0, target: str | None = N base, digest, _ = local_review.current_state(t, self.tmp) return local_review.write_pass(self.tmp, t, base, digest, reviewer, findings) + def write_report(self, findings: list[str], digest: str | None = None) -> Path: + """Write a report as a reviewer would, over the current content unless told otherwise.""" + current = self.digest() + path = local_review.report_path(self.tmp, current) + header = f"{local_review.REPORT_HEADER}{digest or current}" + path.write_text("\n".join([header, *findings]) + "\n", encoding="utf-8") + return path + + def record_cli(self, reviewed: str, report: Path) -> list[str]: + """The argv recording an agent-skill pass over `reviewed` from `report`.""" + return [ + "record", + "--reviewer", + "agent-skill", + "--expect-digest", + reviewed, + "--report", + str(report), + ] + def commit_all(self, message: str) -> None: run(self.tmp, "add", "-A") run(self.tmp, "commit", "-m", message) @@ -907,11 +928,9 @@ def test_record_refuses_content_that_moved_since_the_review(self) -> None: """ (self.tmp / "a.py").write_text("reviewed content\n", encoding="utf-8") reviewed = self.digest() + report = self.write_report([]) (self.tmp / "a.py").write_text("something else entirely\n", encoding="utf-8") - self.assertEqual( - self.main_quiet(["record", "--reviewer", "agent-skill", "--expect-digest", reviewed]), - 2, - ) + self.assertEqual(self.main_quiet(self.record_cli(reviewed, report)), 2) self.assertEqual(self.main_quiet(["check", "--target", self.target]), 1) def test_a_receipt_naming_another_target_gets_no_paste_ready_remedy(self) -> None: @@ -954,10 +973,11 @@ def test_the_check_failure_prints_a_command_that_actually_runs(self) -> None: digest = self.digest() self.assertIn("--expect-digest", printed) self.assertIn(digest, printed, "the remedy carries an abbreviated digest, so it will fail") + self.write_report(["a.py:1 one finding"]) # Run the printed command verbatim. # Repairing it here, by dropping or filling a placeholder, would hide the defect. line = next(l for l in printed.splitlines() if "local_review.py record" in l) - argv = line.split()[2:] + argv = shlex.split(line)[2:] self.assertNotIn("", argv, "the printed command carries a placeholder, so it cannot run") self.assertEqual(self.main_quiet(argv), 0, f"the printed command failed: {argv}") self.assertEqual(self.main_quiet(["check", "--target", self.target]), 0) @@ -1032,38 +1052,107 @@ def test_record_requires_the_digest_the_review_saw(self) -> None: local_review.main(["record", "--reviewer", "agent-skill"]) self.assertNotEqual(caught.exception.code, 0) - def test_a_negative_finding_count_is_refused(self) -> None: - """It would otherwise land in the receipt and mean nothing to whoever reads it.""" + def test_a_typed_finding_count_is_not_accepted(self) -> None: + """A typed count binds the record to nothing the reviewer produced, so the flag is gone.""" (self.tmp / "a.py").write_text("x\n", encoding="utf-8") - self.assertEqual( - self.main_quiet( - [ - "record", - "--reviewer", - "agent-skill", - "--findings", - "-1", - "--expect-digest", - self.digest(), - ] - ), - 2, - ) + report = self.write_report([]) + argv = [*self.record_cli(self.digest(), report), "--findings", "0"] + with contextlib.redirect_stderr(io.StringIO()), self.assertRaises(SystemExit) as caught: + local_review.main(argv) + self.assertNotEqual(caught.exception.code, 0) + + def test_record_requires_a_report(self) -> None: + with contextlib.redirect_stderr(io.StringIO()), self.assertRaises(SystemExit) as caught: + local_review.main(["record", "--reviewer", "agent-skill", "--expect-digest", "x"]) + self.assertNotEqual(caught.exception.code, 0) def test_record_accepts_the_digest_the_review_saw(self) -> None: (self.tmp / "a.py").write_text("reviewed content\n", encoding="utf-8") reviewed = self.digest() - self.assertEqual( - self.main_quiet(["record", "--reviewer", "agent-skill", "--expect-digest", reviewed]), - 0, - ) + report = self.write_report([]) + self.assertEqual(self.main_quiet(self.record_cli(reviewed, report)), 0) + + def test_record_refuses_a_report_nobody_wrote(self) -> None: + """The failure this guards: a driver recording before its reviewer has reported.""" + (self.tmp / "a.py").write_text("x\n", encoding="utf-8") + reviewed = self.digest() + absent = local_review.report_path(self.tmp, reviewed) + err = io.StringIO() + with contextlib.redirect_stderr(err), contextlib.redirect_stdout(io.StringIO()): + self.assertEqual(local_review.main(self.record_cli(reviewed, absent)), 2) + self.assertIn("no report", err.getvalue()) + self.assertFalse(local_review.receipt_path(self.tmp).exists()) + + def test_record_refuses_a_report_written_for_another_digest(self) -> None: + """A report over earlier content says nothing about what is being recorded now.""" + (self.tmp / "a.py").write_text("x\n", encoding="utf-8") + reviewed = self.digest() + report = self.write_report([], digest="0" * 64) + err = io.StringIO() + with contextlib.redirect_stderr(err), contextlib.redirect_stdout(io.StringIO()): + self.assertEqual(local_review.main(self.record_cli(reviewed, report)), 2) + self.assertIn("written for 000000000000", err.getvalue()) + self.assertFalse(local_review.receipt_path(self.tmp).exists()) + + def test_record_refuses_a_report_with_no_digest_line(self) -> None: + (self.tmp / "a.py").write_text("x\n", encoding="utf-8") + reviewed = self.digest() + report = local_review.report_path(self.tmp, reviewed) + report.write_text("a.py:1 a finding with no header above it\n", encoding="utf-8") + self.assertEqual(self.main_quiet(self.record_cli(reviewed, report)), 2) + self.assertFalse(local_review.receipt_path(self.tmp).exists()) + + def test_record_derives_the_count_and_stores_the_report_hash(self) -> None: + """Blank lines count as nothing, and every other line after the header is one finding.""" + (self.tmp / "a.py").write_text("x\n", encoding="utf-8") + reviewed = self.digest() + report = self.write_report(["a.py:1 first", "", "a.py:1 second", " ", "a.py:1 third"]) + self.assertEqual(self.main_quiet(self.record_cli(reviewed, report)), 0) + receipt, problems = local_review.read_receipt(self.tmp) + self.assertEqual(problems, []) + assert receipt is not None + entry = receipt["passes"][0] + self.assertEqual(entry["findings"], 3) + self.assertEqual(entry["reportSha256"], hashlib.sha256(report.read_bytes()).hexdigest()) + + def test_a_header_only_report_records_no_findings(self) -> None: + (self.tmp / "a.py").write_text("x\n", encoding="utf-8") + reviewed = self.digest() + report = self.write_report([]) + self.assertEqual(self.main_quiet(self.record_cli(reviewed, report)), 0) + receipt, _ = local_review.read_receipt(self.tmp) + assert receipt is not None + self.assertEqual(receipt["passes"][0]["findings"], 0) + + def test_status_names_a_report_path_outside_the_work_tree(self) -> None: + """A report written into the tree would be untracked content, moving the digest it names.""" + (self.tmp / "a.py").write_text("x\n", encoding="utf-8") + buf = io.StringIO() + with contextlib.redirect_stdout(buf): + self.assertEqual(local_review.main(["status", "--target", self.target]), 0) + data = json.loads(buf.getvalue()) + path = Path(data["reportPath"]) + self.assertEqual(path, local_review.report_path(self.tmp, data["contentDigest"])) + self.assertEqual(path.parent, local_review.git_dir(self.tmp)) + path.write_text(f"{local_review.REPORT_HEADER}{data['contentDigest']}\n", encoding="utf-8") + self.assertEqual(self.digest(), data["contentDigest"]) + + def test_record_prunes_reports_over_other_content(self) -> None: + (self.tmp / "a.py").write_text("first\n", encoding="utf-8") + earlier = self.write_report([]) + (self.tmp / "a.py").write_text("second\n", encoding="utf-8") + reviewed = self.digest() + report = self.write_report([]) + self.assertNotEqual(earlier, report) + self.assertEqual(self.main_quiet(self.record_cli(reviewed, report)), 0) + self.assertFalse(earlier.exists()) + self.assertTrue(report.exists()) def test_record_will_not_forge_a_headless_backend(self) -> None: """Recording one by hand bypasses the completion check that makes its pass mean anything.""" (self.tmp / "a.py").write_text("x\n", encoding="utf-8") - self.assertEqual( - self.main_quiet(["record", "--reviewer", "coderabbit-cli", "--expect-digest", "x"]), 2 - ) + argv = ["record", "--reviewer", "coderabbit-cli", "--expect-digest", "x", "--report", "x"] + self.assertEqual(self.main_quiet(argv), 2) def test_concurrent_records_do_not_drop_a_pass(self) -> None: """Accumulation is a read, a merge, and a replace, so it needs more than an atomic write.""" @@ -1377,7 +1466,17 @@ def test_an_unknown_reviewer_is_a_boundary_not_a_finding(self) -> None: """2 says the check did not run, which a gate must not read as either verdict.""" self.assertEqual( self.main_quiet( - ["record", "--reviewer", "nope", "--target", self.target, "--expect-digest", "x"] + [ + "record", + "--reviewer", + "nope", + "--target", + self.target, + "--expect-digest", + "x", + "--report", + "x", + ] ), 2, ) @@ -1434,19 +1533,10 @@ def test_status_emits_json_carrying_the_reviewers(self) -> None: def test_record_reports_what_it_recorded(self) -> None: (self.tmp / "new.py").write_text("print('x')\n", encoding="utf-8") + report = self.write_report(["new.py:1 first", "new.py:1 second"]) buf = io.StringIO() with contextlib.redirect_stdout(buf): - code = local_review.main( - [ - "record", - "--reviewer", - "agent-skill", - "--findings", - "2", - "--expect-digest", - self.digest(), - ] - ) + code = local_review.main(self.record_cli(self.digest(), report)) self.assertEqual(code, 0) self.assertIn("agent-skill", buf.getvalue()) receipt, problems = local_review.read_receipt(self.tmp) From 89fcc7427f88d3ee9df110a5c4d76e4c082c35bf Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 2 Nov 2026 08:36:48 -0800 Subject: [PATCH 2/6] Keep a Tool's Summary Line Out of the Review Report The brief now writes only finding lines to the report, so a clean prose_lint scope line is not counted as a finding. A reviewer that finishes without writing its report owes a fresh pass rather than a report the driver writes for it. Co-Authored-By: Claude Opus 5.5 --- .agents/skills/local-strict-review/SKILL.md | 10 +++++----- .../fleet-skills/.source-digests/local-strict-review | 2 +- .../fleet-skills/skills/local-strict-review/SKILL.md | 10 +++++----- .github/skills/local-strict-review/SKILL.md | 10 +++++----- 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/.agents/skills/local-strict-review/SKILL.md b/.agents/skills/local-strict-review/SKILL.md index 90baf1114..d666f4a07 100644 --- a/.agents/skills/local-strict-review/SKILL.md +++ b/.agents/skills/local-strict-review/SKILL.md @@ -45,7 +45,7 @@ Task: adversarial review of this branch's diff against its merge-base with its t read full surrounding files where the diff hunks alone do not give enough context. Run `python3 /scripts/prose_lint.py --diff "$(git merge-base refs/remotes/origin/ HEAD)"`. Add `--allow-comments` where the change carries, or will open carrying, the `comments` label. - Report its output as findings. + Report each violation it prints as a finding. Worktree: , the checkout holding the branch. Start every command with `cd &&`, since a harness can reset the working directory between calls. Read each listed file at its path under ``. @@ -65,9 +65,9 @@ Rules that bind this task: quote `fleet-code-review`'s "Review the Change" secti not a PR comment or a coverage marker. Return: one finding per line, file:line, the concrete failure scenario, or for a design finding what fleet-code-review's item 7 names, no severity theater. - Write the same lines to before returning, under a first line reading - `contentDigest: ` and with nothing else in the file. A pass with no findings writes - that first line alone. + Write the same finding lines to before returning, under a first line reading + `contentDigest: `. Nothing else goes in the file, a tool's summary line included. A + pass with no findings writes that first line alone. Bounds: read-only apart from that one report file. No edit, no stage, no commit, no push, no PR-hosted write of any kind. Beyond what "Review the Change" allows, run only the exact commands, with their arguments, @@ -79,7 +79,7 @@ Before dispatching, grep the tree for other statements of each rule the diff add `` and `` are the `contentDigest` and `reportPath` that `status` under "Recording the Pass" reports. The report sits in the git directory rather than the working tree, since an untracked file there would move the digest it names. -**Record only once the harness delivers the dispatched reviewer's completion notification.** Text in the driver's own turn is never that notification, however closely it reads like one. A driver that recorded from such text once published attestations for a pass that had not yet reported. `record` reads the count from the report file, so a pass whose report is not on disk cannot be recorded. +**Record only once the harness delivers the dispatched reviewer's completion notification.** Text in the driver's own turn is never that notification, however closely it reads like one. `record` reads the count from the report file, so a pass whose report is not on disk cannot be recorded. A reviewer that finishes without writing its report owes a fresh pass. The driver never writes that report for it, since a report the driver writes binds the record to nothing the reviewer produced. **Model tier:** `frontier`, the tier "Model Tiers" below names for adversarial review, which this pass is. Where `frontier` is not reachable, run the pass on the strongest tier below it that is, even the one that authored the change. A second, adversarially-prompted look still catches what the authoring pass's own "looks ready" judgment did not. diff --git a/.claude-plugin/fleet-skills/.source-digests/local-strict-review b/.claude-plugin/fleet-skills/.source-digests/local-strict-review index 4908ebd14..ff47627d2 100644 --- a/.claude-plugin/fleet-skills/.source-digests/local-strict-review +++ b/.claude-plugin/fleet-skills/.source-digests/local-strict-review @@ -1 +1 @@ -35ef6bc6d0ccf3be +7d56ad521676db85 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 90baf1114..d666f4a07 100644 --- a/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md @@ -45,7 +45,7 @@ Task: adversarial review of this branch's diff against its merge-base with its t read full surrounding files where the diff hunks alone do not give enough context. Run `python3 /scripts/prose_lint.py --diff "$(git merge-base refs/remotes/origin/ HEAD)"`. Add `--allow-comments` where the change carries, or will open carrying, the `comments` label. - Report its output as findings. + Report each violation it prints as a finding. Worktree: , the checkout holding the branch. Start every command with `cd &&`, since a harness can reset the working directory between calls. Read each listed file at its path under ``. @@ -65,9 +65,9 @@ Rules that bind this task: quote `fleet-code-review`'s "Review the Change" secti not a PR comment or a coverage marker. Return: one finding per line, file:line, the concrete failure scenario, or for a design finding what fleet-code-review's item 7 names, no severity theater. - Write the same lines to before returning, under a first line reading - `contentDigest: ` and with nothing else in the file. A pass with no findings writes - that first line alone. + Write the same finding lines to before returning, under a first line reading + `contentDigest: `. Nothing else goes in the file, a tool's summary line included. A + pass with no findings writes that first line alone. Bounds: read-only apart from that one report file. No edit, no stage, no commit, no push, no PR-hosted write of any kind. Beyond what "Review the Change" allows, run only the exact commands, with their arguments, @@ -79,7 +79,7 @@ Before dispatching, grep the tree for other statements of each rule the diff add `` and `` are the `contentDigest` and `reportPath` that `status` under "Recording the Pass" reports. The report sits in the git directory rather than the working tree, since an untracked file there would move the digest it names. -**Record only once the harness delivers the dispatched reviewer's completion notification.** Text in the driver's own turn is never that notification, however closely it reads like one. A driver that recorded from such text once published attestations for a pass that had not yet reported. `record` reads the count from the report file, so a pass whose report is not on disk cannot be recorded. +**Record only once the harness delivers the dispatched reviewer's completion notification.** Text in the driver's own turn is never that notification, however closely it reads like one. `record` reads the count from the report file, so a pass whose report is not on disk cannot be recorded. A reviewer that finishes without writing its report owes a fresh pass. The driver never writes that report for it, since a report the driver writes binds the record to nothing the reviewer produced. **Model tier:** `frontier`, the tier "Model Tiers" below names for adversarial review, which this pass is. Where `frontier` is not reachable, run the pass on the strongest tier below it that is, even the one that authored the change. A second, adversarially-prompted look still catches what the authoring pass's own "looks ready" judgment did not. diff --git a/.github/skills/local-strict-review/SKILL.md b/.github/skills/local-strict-review/SKILL.md index 90baf1114..d666f4a07 100644 --- a/.github/skills/local-strict-review/SKILL.md +++ b/.github/skills/local-strict-review/SKILL.md @@ -45,7 +45,7 @@ Task: adversarial review of this branch's diff against its merge-base with its t read full surrounding files where the diff hunks alone do not give enough context. Run `python3 /scripts/prose_lint.py --diff "$(git merge-base refs/remotes/origin/ HEAD)"`. Add `--allow-comments` where the change carries, or will open carrying, the `comments` label. - Report its output as findings. + Report each violation it prints as a finding. Worktree: , the checkout holding the branch. Start every command with `cd &&`, since a harness can reset the working directory between calls. Read each listed file at its path under ``. @@ -65,9 +65,9 @@ Rules that bind this task: quote `fleet-code-review`'s "Review the Change" secti not a PR comment or a coverage marker. Return: one finding per line, file:line, the concrete failure scenario, or for a design finding what fleet-code-review's item 7 names, no severity theater. - Write the same lines to before returning, under a first line reading - `contentDigest: ` and with nothing else in the file. A pass with no findings writes - that first line alone. + Write the same finding lines to before returning, under a first line reading + `contentDigest: `. Nothing else goes in the file, a tool's summary line included. A + pass with no findings writes that first line alone. Bounds: read-only apart from that one report file. No edit, no stage, no commit, no push, no PR-hosted write of any kind. Beyond what "Review the Change" allows, run only the exact commands, with their arguments, @@ -79,7 +79,7 @@ Before dispatching, grep the tree for other statements of each rule the diff add `` and `` are the `contentDigest` and `reportPath` that `status` under "Recording the Pass" reports. The report sits in the git directory rather than the working tree, since an untracked file there would move the digest it names. -**Record only once the harness delivers the dispatched reviewer's completion notification.** Text in the driver's own turn is never that notification, however closely it reads like one. A driver that recorded from such text once published attestations for a pass that had not yet reported. `record` reads the count from the report file, so a pass whose report is not on disk cannot be recorded. +**Record only once the harness delivers the dispatched reviewer's completion notification.** Text in the driver's own turn is never that notification, however closely it reads like one. `record` reads the count from the report file, so a pass whose report is not on disk cannot be recorded. A reviewer that finishes without writing its report owes a fresh pass. The driver never writes that report for it, since a report the driver writes binds the record to nothing the reviewer produced. **Model tier:** `frontier`, the tier "Model Tiers" below names for adversarial review, which this pass is. Where `frontier` is not reachable, run the pass on the strongest tier below it that is, even the one that authored the change. A second, adversarially-prompted look still catches what the authoring pass's own "looks ready" judgment did not. From 79f05812c89427b363b72cf6cab6c24f8df3ab93 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 2 Nov 2026 08:40:05 -0800 Subject: [PATCH 3/6] Read a Review Report Opening With a Byte Order Mark Windows PowerShell 5.1 writes one ahead of UTF-8 text, which hid the report's digest line from the header check. Co-Authored-By: Claude Opus 5.5 --- scripts/local_review.py | 2 +- tests/test_local_review.py | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/scripts/local_review.py b/scripts/local_review.py index dd396230a..998a92344 100755 --- a/scripts/local_review.py +++ b/scripts/local_review.py @@ -328,7 +328,7 @@ def read_report(path: Path, digest: str) -> tuple[str, int]: except OSError as e: raise CannotRun(f"the report at {path} could not be read ({e})") from e try: - lines = raw.decode("utf-8").splitlines() + lines = raw.decode("utf-8-sig").splitlines() except UnicodeDecodeError as e: raise CannotRun(f"the report at {path} is not UTF-8 text ({e})") from e header = lines[0].rstrip() if lines else "" diff --git a/tests/test_local_review.py b/tests/test_local_review.py index d74a0c342..10dc7591f 100755 --- a/tests/test_local_review.py +++ b/tests/test_local_review.py @@ -1115,6 +1115,18 @@ def test_record_derives_the_count_and_stores_the_report_hash(self) -> None: self.assertEqual(entry["findings"], 3) self.assertEqual(entry["reportSha256"], hashlib.sha256(report.read_bytes()).hexdigest()) + def test_a_report_opening_with_a_byte_order_mark_is_read(self) -> None: + """Windows PowerShell 5.1 writes one ahead of UTF-8 text, which would hide the header.""" + (self.tmp / "a.py").write_text("x\n", encoding="utf-8") + reviewed = self.digest() + report = local_review.report_path(self.tmp, reviewed) + text = f"{local_review.REPORT_HEADER}{reviewed}\r\na.py:1 one finding\r\n" + report.write_bytes(text.encode("utf-8-sig")) + self.assertEqual(self.main_quiet(self.record_cli(reviewed, report)), 0) + receipt, _ = local_review.read_receipt(self.tmp) + assert receipt is not None + self.assertEqual(receipt["passes"][0]["findings"], 1) + def test_a_header_only_report_records_no_findings(self) -> None: (self.tmp / "a.py").write_text("x\n", encoding="utf-8") reviewed = self.digest() From d2512ee96c85f665bdd1f80543db390d34a65c83 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 2 Nov 2026 08:43:55 -0800 Subject: [PATCH 4/6] Say the Reviewer Writes Its Report and Drop Rewording Handlers "What It Does" now says the reviewer writes one report file, so it needs an agent type that can write. read_report drops the OSError and UnicodeDecodeError branches, which only reworded a failure main() already reports with exit 2. Co-Authored-By: Claude Opus 5.5 --- .agents/skills/local-strict-review/SKILL.md | 2 +- .../fleet-skills/.source-digests/local-strict-review | 2 +- .../fleet-skills/skills/local-strict-review/SKILL.md | 2 +- .github/skills/local-strict-review/SKILL.md | 2 +- scripts/local_review.py | 7 +------ 5 files changed, 5 insertions(+), 10 deletions(-) diff --git a/.agents/skills/local-strict-review/SKILL.md b/.agents/skills/local-strict-review/SKILL.md index d666f4a07..e4b15f823 100644 --- a/.agents/skills/local-strict-review/SKILL.md +++ b/.agents/skills/local-strict-review/SKILL.md @@ -28,7 +28,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 refs/remotes/origin/ HEAD)"`. The target is named by its full remote-tracking ref because git resolves the short `origin/` against local branches first. So a local branch of that name would define the diff the reviewer reads. In that case the engine, which resolves the full ref, would key its receipt on another diff. Stop and report a failed fetch rather than running the merge-base or diff commands anyway. That is because an existing `refs/remotes/origin/` ref can still resolve after a failed fetch. 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. That is because it reports no untracked file at all. It also 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 the diff pass commits before reviewing. That is because a removal or a restore that is committed leaves no net content to miss. The sweep's own passes below run against uncommitted content instead, on the change that carries their ledger. The second is also why the engine reads HEAD rather than this diff. Its change set comes 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. +Dispatches one subagent against this branch's full diff since it forked from its target branch. The subagent is read-only apart from the one report file it writes, so it needs an agent type that can write a file. 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 refs/remotes/origin/ HEAD)"`. The target is named by its full remote-tracking ref because git resolves the short `origin/` against local branches first. So a local branch of that name would define the diff the reviewer reads. In that case the engine, which resolves the full ref, would key its receipt on another diff. Stop and report a failed fetch rather than running the merge-base or diff commands anyway. That is because an existing `refs/remotes/origin/` ref can still resolve after a failed fetch. 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. That is because it reports no untracked file at all. It also 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 the diff pass commits before reviewing. That is because a removal or a restore that is committed leaves no net content to miss. The sweep's own passes below run against uncommitted content instead, on the change that carries their ledger. The second is also why the engine reads HEAD rather than this diff. Its change set comes 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. diff --git a/.claude-plugin/fleet-skills/.source-digests/local-strict-review b/.claude-plugin/fleet-skills/.source-digests/local-strict-review index ff47627d2..239a06db5 100644 --- a/.claude-plugin/fleet-skills/.source-digests/local-strict-review +++ b/.claude-plugin/fleet-skills/.source-digests/local-strict-review @@ -1 +1 @@ -7d56ad521676db85 +5b658d893ed1c34a 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 d666f4a07..e4b15f823 100644 --- a/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md @@ -28,7 +28,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 refs/remotes/origin/ HEAD)"`. The target is named by its full remote-tracking ref because git resolves the short `origin/` against local branches first. So a local branch of that name would define the diff the reviewer reads. In that case the engine, which resolves the full ref, would key its receipt on another diff. Stop and report a failed fetch rather than running the merge-base or diff commands anyway. That is because an existing `refs/remotes/origin/` ref can still resolve after a failed fetch. 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. That is because it reports no untracked file at all. It also 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 the diff pass commits before reviewing. That is because a removal or a restore that is committed leaves no net content to miss. The sweep's own passes below run against uncommitted content instead, on the change that carries their ledger. The second is also why the engine reads HEAD rather than this diff. Its change set comes 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. +Dispatches one subagent against this branch's full diff since it forked from its target branch. The subagent is read-only apart from the one report file it writes, so it needs an agent type that can write a file. 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 refs/remotes/origin/ HEAD)"`. The target is named by its full remote-tracking ref because git resolves the short `origin/` against local branches first. So a local branch of that name would define the diff the reviewer reads. In that case the engine, which resolves the full ref, would key its receipt on another diff. Stop and report a failed fetch rather than running the merge-base or diff commands anyway. That is because an existing `refs/remotes/origin/` ref can still resolve after a failed fetch. 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. That is because it reports no untracked file at all. It also 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 the diff pass commits before reviewing. That is because a removal or a restore that is committed leaves no net content to miss. The sweep's own passes below run against uncommitted content instead, on the change that carries their ledger. The second is also why the engine reads HEAD rather than this diff. Its change set comes 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. diff --git a/.github/skills/local-strict-review/SKILL.md b/.github/skills/local-strict-review/SKILL.md index d666f4a07..e4b15f823 100644 --- a/.github/skills/local-strict-review/SKILL.md +++ b/.github/skills/local-strict-review/SKILL.md @@ -28,7 +28,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 refs/remotes/origin/ HEAD)"`. The target is named by its full remote-tracking ref because git resolves the short `origin/` against local branches first. So a local branch of that name would define the diff the reviewer reads. In that case the engine, which resolves the full ref, would key its receipt on another diff. Stop and report a failed fetch rather than running the merge-base or diff commands anyway. That is because an existing `refs/remotes/origin/` ref can still resolve after a failed fetch. 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. That is because it reports no untracked file at all. It also 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 the diff pass commits before reviewing. That is because a removal or a restore that is committed leaves no net content to miss. The sweep's own passes below run against uncommitted content instead, on the change that carries their ledger. The second is also why the engine reads HEAD rather than this diff. Its change set comes 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. +Dispatches one subagent against this branch's full diff since it forked from its target branch. The subagent is read-only apart from the one report file it writes, so it needs an agent type that can write a file. 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 refs/remotes/origin/ HEAD)"`. The target is named by its full remote-tracking ref because git resolves the short `origin/` against local branches first. So a local branch of that name would define the diff the reviewer reads. In that case the engine, which resolves the full ref, would key its receipt on another diff. Stop and report a failed fetch rather than running the merge-base or diff commands anyway. That is because an existing `refs/remotes/origin/` ref can still resolve after a failed fetch. 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. That is because it reports no untracked file at all. It also 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 the diff pass commits before reviewing. That is because a removal or a restore that is committed leaves no net content to miss. The sweep's own passes below run against uncommitted content instead, on the change that carries their ledger. The second is also why the engine reads HEAD rather than this diff. Its change set comes 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. diff --git a/scripts/local_review.py b/scripts/local_review.py index 998a92344..4deef45e2 100755 --- a/scripts/local_review.py +++ b/scripts/local_review.py @@ -325,12 +325,7 @@ def read_report(path: Path, digest: str) -> tuple[str, int]: f"no report at {path}, so no review has reported over this content." " Record only once the dispatched reviewer has finished and written it" ) from e - except OSError as e: - raise CannotRun(f"the report at {path} could not be read ({e})") from e - try: - lines = raw.decode("utf-8-sig").splitlines() - except UnicodeDecodeError as e: - raise CannotRun(f"the report at {path} is not UTF-8 text ({e})") from e + lines = raw.decode("utf-8-sig").splitlines() header = lines[0].rstrip() if lines else "" if not header.startswith(REPORT_HEADER): raise CannotRun(f"the report at {path} does not open with a '{REPORT_HEADER}' line") From 11b1d2f79e7799317f7731d010e8ebec8b98fe73 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 2 Nov 2026 08:49:13 -0800 Subject: [PATCH 5/6] Retire Receipts Holding a Typed Finding Count The receipt format moves to version 2, so a receipt an earlier engine wrote, its agent-skill pass carrying a typed count, no longer covers. Co-Authored-By: Claude Opus 5.5 --- scripts/local_review.py | 2 +- tests/test_local_review.py | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/scripts/local_review.py b/scripts/local_review.py index 4deef45e2..b7fab06b4 100755 --- a/scripts/local_review.py +++ b/scripts/local_review.py @@ -130,7 +130,7 @@ sys.path.insert(0, str(Path(__file__).resolve().parent.parent / "spec")) from git_env import GIT_LOCATION_VARS -RECEIPT_VERSION = 1 +RECEIPT_VERSION = 2 RECEIPT_NAME = "local-review-receipt.json" # Kept in the git directory rather than the working tree, so staging the tree does not stage these. STAGING_INDEX_PREFIX = "local-review-index." diff --git a/tests/test_local_review.py b/tests/test_local_review.py index 10dc7591f..a2b9196e6 100755 --- a/tests/test_local_review.py +++ b/tests/test_local_review.py @@ -1313,6 +1313,20 @@ def test_a_future_receipt_version_reads_as_unusable(self) -> None: ) self.assertTrue(any("receiptVersion" in p for p in problems)) + def test_a_receipt_holding_a_typed_count_no_longer_covers(self) -> None: + """Version 1 recorded a count the caller typed, bound to no report the reviewer wrote.""" + (self.tmp / "a.py").write_text("x\n", encoding="utf-8") + base, digest, _ = local_review.current_state(self.target, self.tmp) + legacy = { + "receiptVersion": 1, + "target": self.target, + "mergeBase": base, + "contentDigest": digest, + "passes": [{"reviewer": "agent-skill", "recordedUtc": "x", "findings": 0}], + } + local_review.receipt_path(self.tmp).write_text(json.dumps(legacy), encoding="utf-8") + self.assertEqual(self.main_quiet(["check", "--target", self.target]), 1) + def test_the_receipt_lives_outside_the_working_tree(self) -> None: """So it can never be committed by accident and needs no .gitignore entry.""" path = local_review.receipt_path(self.tmp) From 28ceefbb0d2e285fa474ba794fb855251b99ec2b Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 2 Nov 2026 08:53:47 -0800 Subject: [PATCH 6/6] Have the Dispatcher Read the Report Path Itself A seat that cannot dispatch the reviewer hands back the same worktree, target, and digest that backlog-burndown names. The dispatcher reads reportPath from status in that worktree. Co-Authored-By: Claude Opus 5.5 --- .agents/skills/local-strict-review/SKILL.md | 2 +- .claude-plugin/fleet-skills/.source-digests/local-strict-review | 2 +- .claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md | 2 +- .github/skills/local-strict-review/SKILL.md | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.agents/skills/local-strict-review/SKILL.md b/.agents/skills/local-strict-review/SKILL.md index e4b15f823..3d54be57c 100644 --- a/.agents/skills/local-strict-review/SKILL.md +++ b/.agents/skills/local-strict-review/SKILL.md @@ -83,7 +83,7 @@ Before dispatching, grep the tree for other statements of each rule the diff add **Model tier:** `frontier`, the tier "Model Tiers" below names for adversarial review, which this pass is. Where `frontier` is not reachable, run the pass on the strongest tier below it that is, even the one that authored the change. A second, adversarially-prompted look still catches what the authoring pass's own "looks ready" judgment did not. -Reachable means the tier this session can name when it dispatches the reviewer, rather than the tier this session is itself running on. A worker dispatched by an orchestrator is the ordinary case of a session deliberately tiered down for execution work. Such a session names `frontier` for the reviewer where its harness lets it. That is because tiering down the author is the reason the reviewer must not follow it down. What a given harness and account actually permit varies, so treat this as the tier to ask for rather than one to assume. Where a dispatch exposes no way to name a tier, take what it gives and run the pass, for the reason the fallback above states. A seat that cannot dispatch a subagent hands the pass to whoever dispatched it. The seat commits and runs `python3 /scripts/local_review.py status --target ` in its worktree. It then hands back its worktree path, target, `contentDigest`, and `reportPath`. The dispatcher fills the brief's ``, ``, and `` slots and runs the reviewer. Once the reviewer's completion notification arrives, it runs the record command from "Recording the Pass" in that worktree, with `--reviewer agent-skill --target --expect-digest --report `. It returns the findings. Each fix commit repeats this cycle, and the seat pushes only when a record matches its content. The brief dispatching the seat states whether the dispatcher can continue that seat. Where the dispatcher cannot, the seat stops, reports, and does not push. Where nobody dispatched the seat, it reports to the maintainer that it could not run the pass, and does not push. Either way the push waits rather than the pass being quietly skipped. The headless `run --backend` route under "Recording the Pass" is not the substitute. That is because it runs a vendor CLI against its own review, which never carries the brief above. So it satisfies the rule this section states only where that separate route is what a capture point asked for. +Reachable means the tier this session can name when it dispatches the reviewer, rather than the tier this session is itself running on. A worker dispatched by an orchestrator is the ordinary case of a session deliberately tiered down for execution work. Such a session names `frontier` for the reviewer where its harness lets it. That is because tiering down the author is the reason the reviewer must not follow it down. What a given harness and account actually permit varies, so treat this as the tier to ask for rather than one to assume. Where a dispatch exposes no way to name a tier, take what it gives and run the pass, for the reason the fallback above states. A seat that cannot dispatch a subagent hands the pass to whoever dispatched it. The seat commits and runs `python3 /scripts/local_review.py status --target ` in its worktree. It then hands back its worktree path, target, and `contentDigest`. The dispatcher runs that same `status` command in that worktree for its `reportPath`. It stops where the `contentDigest` printed there differs from the one handed back. The dispatcher fills the brief's ``, ``, and `` slots and runs the reviewer. Once the reviewer's completion notification arrives, it runs the record command from "Recording the Pass" in that worktree, with `--reviewer agent-skill --target --expect-digest --report `. It returns the findings. Each fix commit repeats this cycle, and the seat pushes only when a record matches its content. The brief dispatching the seat states whether the dispatcher can continue that seat. Where the dispatcher cannot, the seat stops, reports, and does not push. Where nobody dispatched the seat, it reports to the maintainer that it could not run the pass, and does not push. Either way the push waits rather than the pass being quietly skipped. The headless `run --backend` route under "Recording the Pass" is not the substitute. That is because it runs a vendor CLI against its own review, which never carries the brief above. So it satisfies the rule this section states only where that separate route is what a capture point asked for. ## Recording the Pass diff --git a/.claude-plugin/fleet-skills/.source-digests/local-strict-review b/.claude-plugin/fleet-skills/.source-digests/local-strict-review index 239a06db5..bd79e3265 100644 --- a/.claude-plugin/fleet-skills/.source-digests/local-strict-review +++ b/.claude-plugin/fleet-skills/.source-digests/local-strict-review @@ -1 +1 @@ -5b658d893ed1c34a +ff7bd6b1a574b1ed 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 e4b15f823..3d54be57c 100644 --- a/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md @@ -83,7 +83,7 @@ Before dispatching, grep the tree for other statements of each rule the diff add **Model tier:** `frontier`, the tier "Model Tiers" below names for adversarial review, which this pass is. Where `frontier` is not reachable, run the pass on the strongest tier below it that is, even the one that authored the change. A second, adversarially-prompted look still catches what the authoring pass's own "looks ready" judgment did not. -Reachable means the tier this session can name when it dispatches the reviewer, rather than the tier this session is itself running on. A worker dispatched by an orchestrator is the ordinary case of a session deliberately tiered down for execution work. Such a session names `frontier` for the reviewer where its harness lets it. That is because tiering down the author is the reason the reviewer must not follow it down. What a given harness and account actually permit varies, so treat this as the tier to ask for rather than one to assume. Where a dispatch exposes no way to name a tier, take what it gives and run the pass, for the reason the fallback above states. A seat that cannot dispatch a subagent hands the pass to whoever dispatched it. The seat commits and runs `python3 /scripts/local_review.py status --target ` in its worktree. It then hands back its worktree path, target, `contentDigest`, and `reportPath`. The dispatcher fills the brief's ``, ``, and `` slots and runs the reviewer. Once the reviewer's completion notification arrives, it runs the record command from "Recording the Pass" in that worktree, with `--reviewer agent-skill --target --expect-digest --report `. It returns the findings. Each fix commit repeats this cycle, and the seat pushes only when a record matches its content. The brief dispatching the seat states whether the dispatcher can continue that seat. Where the dispatcher cannot, the seat stops, reports, and does not push. Where nobody dispatched the seat, it reports to the maintainer that it could not run the pass, and does not push. Either way the push waits rather than the pass being quietly skipped. The headless `run --backend` route under "Recording the Pass" is not the substitute. That is because it runs a vendor CLI against its own review, which never carries the brief above. So it satisfies the rule this section states only where that separate route is what a capture point asked for. +Reachable means the tier this session can name when it dispatches the reviewer, rather than the tier this session is itself running on. A worker dispatched by an orchestrator is the ordinary case of a session deliberately tiered down for execution work. Such a session names `frontier` for the reviewer where its harness lets it. That is because tiering down the author is the reason the reviewer must not follow it down. What a given harness and account actually permit varies, so treat this as the tier to ask for rather than one to assume. Where a dispatch exposes no way to name a tier, take what it gives and run the pass, for the reason the fallback above states. A seat that cannot dispatch a subagent hands the pass to whoever dispatched it. The seat commits and runs `python3 /scripts/local_review.py status --target ` in its worktree. It then hands back its worktree path, target, and `contentDigest`. The dispatcher runs that same `status` command in that worktree for its `reportPath`. It stops where the `contentDigest` printed there differs from the one handed back. The dispatcher fills the brief's ``, ``, and `` slots and runs the reviewer. Once the reviewer's completion notification arrives, it runs the record command from "Recording the Pass" in that worktree, with `--reviewer agent-skill --target --expect-digest --report `. It returns the findings. Each fix commit repeats this cycle, and the seat pushes only when a record matches its content. The brief dispatching the seat states whether the dispatcher can continue that seat. Where the dispatcher cannot, the seat stops, reports, and does not push. Where nobody dispatched the seat, it reports to the maintainer that it could not run the pass, and does not push. Either way the push waits rather than the pass being quietly skipped. The headless `run --backend` route under "Recording the Pass" is not the substitute. That is because it runs a vendor CLI against its own review, which never carries the brief above. So it satisfies the rule this section states only where that separate route is what a capture point asked for. ## Recording the Pass diff --git a/.github/skills/local-strict-review/SKILL.md b/.github/skills/local-strict-review/SKILL.md index e4b15f823..3d54be57c 100644 --- a/.github/skills/local-strict-review/SKILL.md +++ b/.github/skills/local-strict-review/SKILL.md @@ -83,7 +83,7 @@ Before dispatching, grep the tree for other statements of each rule the diff add **Model tier:** `frontier`, the tier "Model Tiers" below names for adversarial review, which this pass is. Where `frontier` is not reachable, run the pass on the strongest tier below it that is, even the one that authored the change. A second, adversarially-prompted look still catches what the authoring pass's own "looks ready" judgment did not. -Reachable means the tier this session can name when it dispatches the reviewer, rather than the tier this session is itself running on. A worker dispatched by an orchestrator is the ordinary case of a session deliberately tiered down for execution work. Such a session names `frontier` for the reviewer where its harness lets it. That is because tiering down the author is the reason the reviewer must not follow it down. What a given harness and account actually permit varies, so treat this as the tier to ask for rather than one to assume. Where a dispatch exposes no way to name a tier, take what it gives and run the pass, for the reason the fallback above states. A seat that cannot dispatch a subagent hands the pass to whoever dispatched it. The seat commits and runs `python3 /scripts/local_review.py status --target ` in its worktree. It then hands back its worktree path, target, `contentDigest`, and `reportPath`. The dispatcher fills the brief's ``, ``, and `` slots and runs the reviewer. Once the reviewer's completion notification arrives, it runs the record command from "Recording the Pass" in that worktree, with `--reviewer agent-skill --target --expect-digest --report `. It returns the findings. Each fix commit repeats this cycle, and the seat pushes only when a record matches its content. The brief dispatching the seat states whether the dispatcher can continue that seat. Where the dispatcher cannot, the seat stops, reports, and does not push. Where nobody dispatched the seat, it reports to the maintainer that it could not run the pass, and does not push. Either way the push waits rather than the pass being quietly skipped. The headless `run --backend` route under "Recording the Pass" is not the substitute. That is because it runs a vendor CLI against its own review, which never carries the brief above. So it satisfies the rule this section states only where that separate route is what a capture point asked for. +Reachable means the tier this session can name when it dispatches the reviewer, rather than the tier this session is itself running on. A worker dispatched by an orchestrator is the ordinary case of a session deliberately tiered down for execution work. Such a session names `frontier` for the reviewer where its harness lets it. That is because tiering down the author is the reason the reviewer must not follow it down. What a given harness and account actually permit varies, so treat this as the tier to ask for rather than one to assume. Where a dispatch exposes no way to name a tier, take what it gives and run the pass, for the reason the fallback above states. A seat that cannot dispatch a subagent hands the pass to whoever dispatched it. The seat commits and runs `python3 /scripts/local_review.py status --target ` in its worktree. It then hands back its worktree path, target, and `contentDigest`. The dispatcher runs that same `status` command in that worktree for its `reportPath`. It stops where the `contentDigest` printed there differs from the one handed back. The dispatcher fills the brief's ``, ``, and `` slots and runs the reviewer. Once the reviewer's completion notification arrives, it runs the record command from "Recording the Pass" in that worktree, with `--reviewer agent-skill --target --expect-digest --report `. It returns the findings. Each fix commit repeats this cycle, and the seat pushes only when a record matches its content. The brief dispatching the seat states whether the dispatcher can continue that seat. Where the dispatcher cannot, the seat stops, reports, and does not push. Where nobody dispatched the seat, it reports to the maintainer that it could not run the pass, and does not push. Either way the push waits rather than the pass being quietly skipped. The headless `run --backend` route under "Recording the Pass" is not the substitute. That is because it runs a vendor CLI against its own review, which never carries the brief above. So it satisfies the rule this section states only where that separate route is what a capture point asked for. ## Recording the Pass