diff --git a/.agents/skills/drive-pr/SKILL.md b/.agents/skills/drive-pr/SKILL.md index 7a4bddb79..1370c5943 100644 --- a/.agents/skills/drive-pr/SKILL.md +++ b/.agents/skills/drive-pr/SKILL.md @@ -165,9 +165,11 @@ maintainer when". Disprove a wrong finding with the command and its output, the code path that makes it impossible, or the rule that governs it. A finding that is factually correct but not this repo's to fix (a verbatim-fidelity manifest entry byte-locking the section, ownership that - sits elsewhere) declines the same way: name the boundary and cite what proves it. Either shape - closes the thread on its own evidence, and the agent resolves such a thread itself rather than - leaving it for the maintainer. What makes that safe is the evidence being checkable by anyone, + sits elsewhere) declines the same way: name the boundary and cite what proves it. So does a + hardening finding that `GOVERNANCE.md` "Trust Boundaries and Hardening Effort" rules out, + citing that section and the input or deployment it turns on. Each shape closes the thread on + its own evidence, and the agent resolves such a thread itself rather than leaving it for the + maintainer. What makes that safe is the evidence being checkable by anyone, a command and its output, the code path, the quoted rule, a byte-identical diff, so a decline resting on anything weaker is not one of these. An assertion ("this is fine") does not close a finding, and outcome 3's value call is the maintainer's, so that thread stays open until they diff --git a/.agents/skills/fleet-code-review/SKILL.md b/.agents/skills/fleet-code-review/SKILL.md index 42d3cda9d..b218bd7c9 100644 --- a/.agents/skills/fleet-code-review/SKILL.md +++ b/.agents/skills/fleet-code-review/SKILL.md @@ -41,6 +41,24 @@ For each candidate finding: 3. Confirm that the repository does not already prevent it elsewhere. 4. Prefer one root-cause finding over several symptoms of the same defect. 5. Omit pure preferences that no repository rule or user-visible risk supports. +6. Omit a hardening finding that `GOVERNANCE.md` "Trust Boundaries and Hardening Effort", + carried below, declines. + + + +Hardening is spent where a failure can actually occur. A defense against a case that cannot happen still costs a review round, a test, and the upkeep of both. So the effort an input gets is set by where it comes from and what its failure would do. + +- **Every input is trusted or untrusted, and the class sets the effort.** Trusted input is the maintainer's and what the maintainer controls. That covers files, images, and data handed to a session, settings and secrets files the maintainer writes, and command-line arguments the maintainer types. It covers this repository's content and the hub's, and the maintainer's own hosts with the output read back from them. It covers state files the repository's own scripts write. Untrusted input arrives from outside. That covers a request from an internet client and every field it controls, and upstream release metadata and downloads. It covers a third-party API response, content from an outside contributor, and a bot-authored pull request. +- **The class follows where data originates, not the channel carrying it.** Any channel holding content an outsider supplied is untrusted, whatever the list above names. That covers an argument or a checked-out tree holding a pull request's content. It covers a host's log line recording an outside request, and a state file caching a third-party response. The same channel carrying the maintainer's input stays trusted. +- **Against trusted input the only threat is a maintainer's mistake.** Guard a realistic mistake, such as a typo, CRLF line endings, a missing file, or a wrong unit. Guard it only where it would fail silently or destructively, with one check at the point of read that says what is wrong. Do not build a parser for adversarial shapes or test injection through a trusted file. Do not sanitize trusted text read back from the maintainer's own hosts against injection, or file an issue for an attack on trusted input. What agent-authored text may quote from those hosts is still "Representative Data in Agent-Authored Text"'s to decide. +- **Untrusted input is validated once, at the boundary where it enters**, in proportion to what it can reach from there. +- **Effort follows likelihood times consequence, for robustness as much as for trust.** Code that fails on its ordinary path in its deployment is a bug rather than a hardening case, and is fixed. A hardening case earns a fix or an issue only where it can realistically occur in the code's actual deployment. Its failure must also be silent or destructive, or be one an outsider can trigger. Every other case is declined with that reason, neither fixed nor filed. +- **A review finding asking for hardening the bullets above rule out is itself declined**, with no fix and no issue. The decline cites this section and the input or deployment that rules the case out. This binds the reviewer as much as the author, so a reviewer does not raise such a finding. A review brief asks after races, coercions, and platform differences only where the code's deployment can produce one. +- **A repository names its own boundaries where the classes above leave them unclear.** Its `OPERATIONS.md` lists which concrete inputs are trusted and which are not, under `Configuration Layout`. A session or a reviewer then applies this section without re-deriving it. A repository naming none uses the classes above. + +`GOVERNANCE.md` "Trust Boundaries and Hardening Effort" keeps the full rules, and the `fleet-code-review` Skill at `.agents/skills/fleet-code-review/SKILL.md` in the hub, not a repo-relative link since that path is hub-local and not carried into every fleet repo, carries it whole as a generated include and surfaces it wherever a change is reviewed. + + Review carried fleet content by intent and fidelity. A byte-locked reference to a path that one downstream repository does not carry is not a broken link. A substantive defect in canonical diff --git a/.agents/skills/local-strict-review/SKILL.md b/.agents/skills/local-strict-review/SKILL.md index 3b4f9e9d8..296809a51 100644 --- a/.agents/skills/local-strict-review/SKILL.md +++ b/.agents/skills/local-strict-review/SKILL.md @@ -33,7 +33,7 @@ Dispatches one read-only subagent against this branch's full diff since it forke The subagent reads the full content of every file the diff and the untracked-file list touch, not just the hunks, since cross-file and whole-file context is exactly what incremental review misses. It reports findings only. It never fixes, stages, or commits anything. -Review criteria are `fleet-code-review`'s "Review the Change" section, reused rather than restated here, plus three traps worth calling out explicitly for a pass that runs before a human or a PR-hosted reviewer ever sees the diff: unguarded type coercions, TOCTOU/race conditions, and platform-specific behavior differences. `fleet-code-review`'s separate "Publish Every Finding" section does not apply here: this skill has no PR to post a comment on and no coverage marker to close a review with, so its own report contract below replaces that section rather than extending it. +Review criteria are `fleet-code-review`'s "Review the Change" section, reused rather than restated here, plus three traps worth calling out explicitly for a pass that runs before a human or a PR-hosted reviewer ever sees the diff: unguarded type coercions, TOCTOU/race conditions, and platform-specific behavior differences. Each is called out only where the code's actual deployment can produce it, per `GOVERNANCE.md` "Trust Boundaries and Hardening Effort". `fleet-code-review`'s separate "Publish Every Finding" section does not apply here: this skill has no PR to post a comment on and no coverage marker to close a review with, so its own report contract below replaces that section rather than extending it. ## Running It @@ -55,8 +55,9 @@ Paths: the files `git diff --name-only "$(git merge-base refs/remotes/origin/ diff --git a/.agents/skills/pr-review-conduct/SKILL.md b/.agents/skills/pr-review-conduct/SKILL.md index 2a89c8c16..bb5f1c0fa 100644 --- a/.agents/skills/pr-review-conduct/SKILL.md +++ b/.agents/skills/pr-review-conduct/SKILL.md @@ -244,9 +244,11 @@ After an authorized merge, run the `repo-worktree` post-merge cleanup procedure Disprove a wrong finding with the command and its output, the code path that makes it impossible, or the rule that governs it. A finding that is factually correct but not this repo's to fix (a verbatim-fidelity manifest entry byte-locking the section, ownership that - sits elsewhere) declines the same way: name the boundary and cite what proves it. Either shape - closes the thread on its own evidence, and the agent resolves such a thread itself rather than - leaving it for the maintainer. What makes that safe is the evidence being checkable by anyone, + sits elsewhere) declines the same way: name the boundary and cite what proves it. So does a + hardening finding that `GOVERNANCE.md` "Trust Boundaries and Hardening Effort" rules out, + citing that section and the input or deployment it turns on. Each shape closes the thread on + its own evidence, and the agent resolves such a thread itself rather than leaving it for the + maintainer. What makes that safe is the evidence being checkable by anyone, a command and its output, the code path, the quoted rule, a byte-identical diff, so a decline resting on anything weaker is not one of these. An assertion ("this is fine") does not close a finding, and outcome 3's value call is the maintainer's, so that thread stays open until they @@ -284,6 +286,8 @@ instance's outcome is context for the new one, never a standing answer to reuse never against its confidence label. Classify before responding: - **Bug**, wrong behavior, missing coverage, a real code or doc divergence. Fix it. +- **Hardening that `GOVERNANCE.md` "Trust Boundaries and Hardening Effort" rules out**, including + a test it says not to write. Decline it under outcome 2, even where it reads as missing coverage. - **Style or convention**. If the cited rule matches the existing tree, fix the code. If the rule contradicts the tree or industry norm, **fix the rule, not the code**, and take it to the maintainer (outcome 5) rather than bouncing the same code across rounds. diff --git a/.claude-plugin/fleet-skills/.source-digests/drive-pr b/.claude-plugin/fleet-skills/.source-digests/drive-pr index 4ff80e0da..aa1f34b9c 100644 --- a/.claude-plugin/fleet-skills/.source-digests/drive-pr +++ b/.claude-plugin/fleet-skills/.source-digests/drive-pr @@ -1 +1 @@ -03ce7ed14f270d13 +4fbde6878a3ce9e7 diff --git a/.claude-plugin/fleet-skills/.source-digests/fleet-code-review b/.claude-plugin/fleet-skills/.source-digests/fleet-code-review index 8fa84a84e..ab1fc3c6f 100644 --- a/.claude-plugin/fleet-skills/.source-digests/fleet-code-review +++ b/.claude-plugin/fleet-skills/.source-digests/fleet-code-review @@ -1 +1 @@ -d23962efb51cbe7e +218937ec2fa571fd diff --git a/.claude-plugin/fleet-skills/.source-digests/local-strict-review b/.claude-plugin/fleet-skills/.source-digests/local-strict-review index f92b95676..ce6572444 100644 --- a/.claude-plugin/fleet-skills/.source-digests/local-strict-review +++ b/.claude-plugin/fleet-skills/.source-digests/local-strict-review @@ -1 +1 @@ -3e0a74b0a99873ca +cbd0582547eb3f52 diff --git a/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct b/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct index c9a0a5bef..40c066bf5 100644 --- a/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct +++ b/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct @@ -1 +1 @@ -e3b78814c0099384 +92a8ca385cf20a5a diff --git a/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md b/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md index 7a4bddb79..1370c5943 100644 --- a/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md @@ -165,9 +165,11 @@ maintainer when". Disprove a wrong finding with the command and its output, the code path that makes it impossible, or the rule that governs it. A finding that is factually correct but not this repo's to fix (a verbatim-fidelity manifest entry byte-locking the section, ownership that - sits elsewhere) declines the same way: name the boundary and cite what proves it. Either shape - closes the thread on its own evidence, and the agent resolves such a thread itself rather than - leaving it for the maintainer. What makes that safe is the evidence being checkable by anyone, + sits elsewhere) declines the same way: name the boundary and cite what proves it. So does a + hardening finding that `GOVERNANCE.md` "Trust Boundaries and Hardening Effort" rules out, + citing that section and the input or deployment it turns on. Each shape closes the thread on + its own evidence, and the agent resolves such a thread itself rather than leaving it for the + maintainer. What makes that safe is the evidence being checkable by anyone, a command and its output, the code path, the quoted rule, a byte-identical diff, so a decline resting on anything weaker is not one of these. An assertion ("this is fine") does not close a finding, and outcome 3's value call is the maintainer's, so that thread stays open until they diff --git a/.claude-plugin/fleet-skills/skills/fleet-code-review/SKILL.md b/.claude-plugin/fleet-skills/skills/fleet-code-review/SKILL.md index 42d3cda9d..b218bd7c9 100644 --- a/.claude-plugin/fleet-skills/skills/fleet-code-review/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/fleet-code-review/SKILL.md @@ -41,6 +41,24 @@ For each candidate finding: 3. Confirm that the repository does not already prevent it elsewhere. 4. Prefer one root-cause finding over several symptoms of the same defect. 5. Omit pure preferences that no repository rule or user-visible risk supports. +6. Omit a hardening finding that `GOVERNANCE.md` "Trust Boundaries and Hardening Effort", + carried below, declines. + + + +Hardening is spent where a failure can actually occur. A defense against a case that cannot happen still costs a review round, a test, and the upkeep of both. So the effort an input gets is set by where it comes from and what its failure would do. + +- **Every input is trusted or untrusted, and the class sets the effort.** Trusted input is the maintainer's and what the maintainer controls. That covers files, images, and data handed to a session, settings and secrets files the maintainer writes, and command-line arguments the maintainer types. It covers this repository's content and the hub's, and the maintainer's own hosts with the output read back from them. It covers state files the repository's own scripts write. Untrusted input arrives from outside. That covers a request from an internet client and every field it controls, and upstream release metadata and downloads. It covers a third-party API response, content from an outside contributor, and a bot-authored pull request. +- **The class follows where data originates, not the channel carrying it.** Any channel holding content an outsider supplied is untrusted, whatever the list above names. That covers an argument or a checked-out tree holding a pull request's content. It covers a host's log line recording an outside request, and a state file caching a third-party response. The same channel carrying the maintainer's input stays trusted. +- **Against trusted input the only threat is a maintainer's mistake.** Guard a realistic mistake, such as a typo, CRLF line endings, a missing file, or a wrong unit. Guard it only where it would fail silently or destructively, with one check at the point of read that says what is wrong. Do not build a parser for adversarial shapes or test injection through a trusted file. Do not sanitize trusted text read back from the maintainer's own hosts against injection, or file an issue for an attack on trusted input. What agent-authored text may quote from those hosts is still "Representative Data in Agent-Authored Text"'s to decide. +- **Untrusted input is validated once, at the boundary where it enters**, in proportion to what it can reach from there. +- **Effort follows likelihood times consequence, for robustness as much as for trust.** Code that fails on its ordinary path in its deployment is a bug rather than a hardening case, and is fixed. A hardening case earns a fix or an issue only where it can realistically occur in the code's actual deployment. Its failure must also be silent or destructive, or be one an outsider can trigger. Every other case is declined with that reason, neither fixed nor filed. +- **A review finding asking for hardening the bullets above rule out is itself declined**, with no fix and no issue. The decline cites this section and the input or deployment that rules the case out. This binds the reviewer as much as the author, so a reviewer does not raise such a finding. A review brief asks after races, coercions, and platform differences only where the code's deployment can produce one. +- **A repository names its own boundaries where the classes above leave them unclear.** Its `OPERATIONS.md` lists which concrete inputs are trusted and which are not, under `Configuration Layout`. A session or a reviewer then applies this section without re-deriving it. A repository naming none uses the classes above. + +`GOVERNANCE.md` "Trust Boundaries and Hardening Effort" keeps the full rules, and the `fleet-code-review` Skill at `.agents/skills/fleet-code-review/SKILL.md` in the hub, not a repo-relative link since that path is hub-local and not carried into every fleet repo, carries it whole as a generated include and surfaces it wherever a change is reviewed. + + Review carried fleet content by intent and fidelity. A byte-locked reference to a path that one downstream repository does not carry is not a broken link. A substantive defect in canonical 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 3b4f9e9d8..296809a51 100644 --- a/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md @@ -33,7 +33,7 @@ Dispatches one read-only subagent against this branch's full diff since it forke The subagent reads the full content of every file the diff and the untracked-file list touch, not just the hunks, since cross-file and whole-file context is exactly what incremental review misses. It reports findings only. It never fixes, stages, or commits anything. -Review criteria are `fleet-code-review`'s "Review the Change" section, reused rather than restated here, plus three traps worth calling out explicitly for a pass that runs before a human or a PR-hosted reviewer ever sees the diff: unguarded type coercions, TOCTOU/race conditions, and platform-specific behavior differences. `fleet-code-review`'s separate "Publish Every Finding" section does not apply here: this skill has no PR to post a comment on and no coverage marker to close a review with, so its own report contract below replaces that section rather than extending it. +Review criteria are `fleet-code-review`'s "Review the Change" section, reused rather than restated here, plus three traps worth calling out explicitly for a pass that runs before a human or a PR-hosted reviewer ever sees the diff: unguarded type coercions, TOCTOU/race conditions, and platform-specific behavior differences. Each is called out only where the code's actual deployment can produce it, per `GOVERNANCE.md` "Trust Boundaries and Hardening Effort". `fleet-code-review`'s separate "Publish Every Finding" section does not apply here: this skill has no PR to post a comment on and no coverage marker to close a review with, so its own report contract below replaces that section rather than extending it. ## Running It @@ -55,8 +55,9 @@ Paths: the files `git diff --name-only "$(git merge-base refs/remotes/origin/ diff --git a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md index 2a89c8c16..bb5f1c0fa 100644 --- a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md @@ -244,9 +244,11 @@ After an authorized merge, run the `repo-worktree` post-merge cleanup procedure Disprove a wrong finding with the command and its output, the code path that makes it impossible, or the rule that governs it. A finding that is factually correct but not this repo's to fix (a verbatim-fidelity manifest entry byte-locking the section, ownership that - sits elsewhere) declines the same way: name the boundary and cite what proves it. Either shape - closes the thread on its own evidence, and the agent resolves such a thread itself rather than - leaving it for the maintainer. What makes that safe is the evidence being checkable by anyone, + sits elsewhere) declines the same way: name the boundary and cite what proves it. So does a + hardening finding that `GOVERNANCE.md` "Trust Boundaries and Hardening Effort" rules out, + citing that section and the input or deployment it turns on. Each shape closes the thread on + its own evidence, and the agent resolves such a thread itself rather than leaving it for the + maintainer. What makes that safe is the evidence being checkable by anyone, a command and its output, the code path, the quoted rule, a byte-identical diff, so a decline resting on anything weaker is not one of these. An assertion ("this is fine") does not close a finding, and outcome 3's value call is the maintainer's, so that thread stays open until they @@ -284,6 +286,8 @@ instance's outcome is context for the new one, never a standing answer to reuse never against its confidence label. Classify before responding: - **Bug**, wrong behavior, missing coverage, a real code or doc divergence. Fix it. +- **Hardening that `GOVERNANCE.md` "Trust Boundaries and Hardening Effort" rules out**, including + a test it says not to write. Decline it under outcome 2, even where it reads as missing coverage. - **Style or convention**. If the cited rule matches the existing tree, fix the code. If the rule contradicts the tree or industry norm, **fix the rule, not the code**, and take it to the maintainer (outcome 5) rather than bouncing the same code across rounds. diff --git a/.github/skills/drive-pr/SKILL.md b/.github/skills/drive-pr/SKILL.md index 7a4bddb79..1370c5943 100644 --- a/.github/skills/drive-pr/SKILL.md +++ b/.github/skills/drive-pr/SKILL.md @@ -165,9 +165,11 @@ maintainer when". Disprove a wrong finding with the command and its output, the code path that makes it impossible, or the rule that governs it. A finding that is factually correct but not this repo's to fix (a verbatim-fidelity manifest entry byte-locking the section, ownership that - sits elsewhere) declines the same way: name the boundary and cite what proves it. Either shape - closes the thread on its own evidence, and the agent resolves such a thread itself rather than - leaving it for the maintainer. What makes that safe is the evidence being checkable by anyone, + sits elsewhere) declines the same way: name the boundary and cite what proves it. So does a + hardening finding that `GOVERNANCE.md` "Trust Boundaries and Hardening Effort" rules out, + citing that section and the input or deployment it turns on. Each shape closes the thread on + its own evidence, and the agent resolves such a thread itself rather than leaving it for the + maintainer. What makes that safe is the evidence being checkable by anyone, a command and its output, the code path, the quoted rule, a byte-identical diff, so a decline resting on anything weaker is not one of these. An assertion ("this is fine") does not close a finding, and outcome 3's value call is the maintainer's, so that thread stays open until they diff --git a/.github/skills/fleet-code-review/SKILL.md b/.github/skills/fleet-code-review/SKILL.md index 42d3cda9d..b218bd7c9 100644 --- a/.github/skills/fleet-code-review/SKILL.md +++ b/.github/skills/fleet-code-review/SKILL.md @@ -41,6 +41,24 @@ For each candidate finding: 3. Confirm that the repository does not already prevent it elsewhere. 4. Prefer one root-cause finding over several symptoms of the same defect. 5. Omit pure preferences that no repository rule or user-visible risk supports. +6. Omit a hardening finding that `GOVERNANCE.md` "Trust Boundaries and Hardening Effort", + carried below, declines. + + + +Hardening is spent where a failure can actually occur. A defense against a case that cannot happen still costs a review round, a test, and the upkeep of both. So the effort an input gets is set by where it comes from and what its failure would do. + +- **Every input is trusted or untrusted, and the class sets the effort.** Trusted input is the maintainer's and what the maintainer controls. That covers files, images, and data handed to a session, settings and secrets files the maintainer writes, and command-line arguments the maintainer types. It covers this repository's content and the hub's, and the maintainer's own hosts with the output read back from them. It covers state files the repository's own scripts write. Untrusted input arrives from outside. That covers a request from an internet client and every field it controls, and upstream release metadata and downloads. It covers a third-party API response, content from an outside contributor, and a bot-authored pull request. +- **The class follows where data originates, not the channel carrying it.** Any channel holding content an outsider supplied is untrusted, whatever the list above names. That covers an argument or a checked-out tree holding a pull request's content. It covers a host's log line recording an outside request, and a state file caching a third-party response. The same channel carrying the maintainer's input stays trusted. +- **Against trusted input the only threat is a maintainer's mistake.** Guard a realistic mistake, such as a typo, CRLF line endings, a missing file, or a wrong unit. Guard it only where it would fail silently or destructively, with one check at the point of read that says what is wrong. Do not build a parser for adversarial shapes or test injection through a trusted file. Do not sanitize trusted text read back from the maintainer's own hosts against injection, or file an issue for an attack on trusted input. What agent-authored text may quote from those hosts is still "Representative Data in Agent-Authored Text"'s to decide. +- **Untrusted input is validated once, at the boundary where it enters**, in proportion to what it can reach from there. +- **Effort follows likelihood times consequence, for robustness as much as for trust.** Code that fails on its ordinary path in its deployment is a bug rather than a hardening case, and is fixed. A hardening case earns a fix or an issue only where it can realistically occur in the code's actual deployment. Its failure must also be silent or destructive, or be one an outsider can trigger. Every other case is declined with that reason, neither fixed nor filed. +- **A review finding asking for hardening the bullets above rule out is itself declined**, with no fix and no issue. The decline cites this section and the input or deployment that rules the case out. This binds the reviewer as much as the author, so a reviewer does not raise such a finding. A review brief asks after races, coercions, and platform differences only where the code's deployment can produce one. +- **A repository names its own boundaries where the classes above leave them unclear.** Its `OPERATIONS.md` lists which concrete inputs are trusted and which are not, under `Configuration Layout`. A session or a reviewer then applies this section without re-deriving it. A repository naming none uses the classes above. + +`GOVERNANCE.md` "Trust Boundaries and Hardening Effort" keeps the full rules, and the `fleet-code-review` Skill at `.agents/skills/fleet-code-review/SKILL.md` in the hub, not a repo-relative link since that path is hub-local and not carried into every fleet repo, carries it whole as a generated include and surfaces it wherever a change is reviewed. + + Review carried fleet content by intent and fidelity. A byte-locked reference to a path that one downstream repository does not carry is not a broken link. A substantive defect in canonical diff --git a/.github/skills/local-strict-review/SKILL.md b/.github/skills/local-strict-review/SKILL.md index 3b4f9e9d8..296809a51 100644 --- a/.github/skills/local-strict-review/SKILL.md +++ b/.github/skills/local-strict-review/SKILL.md @@ -33,7 +33,7 @@ Dispatches one read-only subagent against this branch's full diff since it forke The subagent reads the full content of every file the diff and the untracked-file list touch, not just the hunks, since cross-file and whole-file context is exactly what incremental review misses. It reports findings only. It never fixes, stages, or commits anything. -Review criteria are `fleet-code-review`'s "Review the Change" section, reused rather than restated here, plus three traps worth calling out explicitly for a pass that runs before a human or a PR-hosted reviewer ever sees the diff: unguarded type coercions, TOCTOU/race conditions, and platform-specific behavior differences. `fleet-code-review`'s separate "Publish Every Finding" section does not apply here: this skill has no PR to post a comment on and no coverage marker to close a review with, so its own report contract below replaces that section rather than extending it. +Review criteria are `fleet-code-review`'s "Review the Change" section, reused rather than restated here, plus three traps worth calling out explicitly for a pass that runs before a human or a PR-hosted reviewer ever sees the diff: unguarded type coercions, TOCTOU/race conditions, and platform-specific behavior differences. Each is called out only where the code's actual deployment can produce it, per `GOVERNANCE.md` "Trust Boundaries and Hardening Effort". `fleet-code-review`'s separate "Publish Every Finding" section does not apply here: this skill has no PR to post a comment on and no coverage marker to close a review with, so its own report contract below replaces that section rather than extending it. ## Running It @@ -55,8 +55,9 @@ Paths: the files `git diff --name-only "$(git merge-base refs/remotes/origin/ diff --git a/.github/skills/pr-review-conduct/SKILL.md b/.github/skills/pr-review-conduct/SKILL.md index 2a89c8c16..bb5f1c0fa 100644 --- a/.github/skills/pr-review-conduct/SKILL.md +++ b/.github/skills/pr-review-conduct/SKILL.md @@ -244,9 +244,11 @@ After an authorized merge, run the `repo-worktree` post-merge cleanup procedure Disprove a wrong finding with the command and its output, the code path that makes it impossible, or the rule that governs it. A finding that is factually correct but not this repo's to fix (a verbatim-fidelity manifest entry byte-locking the section, ownership that - sits elsewhere) declines the same way: name the boundary and cite what proves it. Either shape - closes the thread on its own evidence, and the agent resolves such a thread itself rather than - leaving it for the maintainer. What makes that safe is the evidence being checkable by anyone, + sits elsewhere) declines the same way: name the boundary and cite what proves it. So does a + hardening finding that `GOVERNANCE.md` "Trust Boundaries and Hardening Effort" rules out, + citing that section and the input or deployment it turns on. Each shape closes the thread on + its own evidence, and the agent resolves such a thread itself rather than leaving it for the + maintainer. What makes that safe is the evidence being checkable by anyone, a command and its output, the code path, the quoted rule, a byte-identical diff, so a decline resting on anything weaker is not one of these. An assertion ("this is fine") does not close a finding, and outcome 3's value call is the maintainer's, so that thread stays open until they @@ -284,6 +286,8 @@ instance's outcome is context for the new one, never a standing answer to reuse never against its confidence label. Classify before responding: - **Bug**, wrong behavior, missing coverage, a real code or doc divergence. Fix it. +- **Hardening that `GOVERNANCE.md` "Trust Boundaries and Hardening Effort" rules out**, including + a test it says not to write. Decline it under outcome 2, even where it reads as missing coverage. - **Style or convention**. If the cited rule matches the existing tree, fix the code. If the rule contradicts the tree or industry norm, **fix the rule, not the code**, and take it to the maintainer (outcome 5) rather than bouncing the same code across rounds. diff --git a/AGENTS.md b/AGENTS.md index 22d505ab5..783f8e559 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -95,6 +95,7 @@ Every rule below is a level-two section of [`GOVERNANCE.md`](./GOVERNANCE.md) un | Writing a commit message or pull request title | `Pull Request Title and Commit Message Conventions`, packaged as the `comment-and-doc-style` Skill | | Any prose, comment, doc, or line-ending change | `Documentation Style Conventions`, packaged as the `comment-and-doc-style` Skill | | Proving work actually happened | `Verification Discipline`, surfaced at its decision moment by the `agent-conduct` Skill, and the section keeps the full rules | +| Hardening code against an input or a failure, or triaging a finding that asks for it | `Trust Boundaries and Hardening Effort`, surfaced wherever a change is reviewed by the `fleet-code-review` Skill, and the section keeps the full rules | | Editing rule text, a Skill, or any other content other repos carry | `Verification Discipline`'s carried-content rule, which asks nothing of the change itself, its passes being run by the `local-strict-review` Skill against the units a periodic sweep names and recorded by the hub-hosted `scripts/canonical_review.py` | | Opening a pull request, or requesting, monitoring, answering, or closing a review | `PR Review Etiquette`, packaged as the `pr-review-conduct` Skill | | Reviewing a pull request, patch, or change set | No section of its own: the `fleet-code-review` Skill, which routes to the applicable general, language, documentation, and workflow skills | diff --git a/GOVERNANCE.md b/GOVERNANCE.md index 60b1a08f2..625da237a 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -210,6 +210,20 @@ The checks that separate work actually done from work that merely reports succes `GOVERNANCE.md` "Verification Discipline" keeps the full rules, and the `agent-conduct` Skill at `.agents/skills/agent-conduct/SKILL.md` in the hub, not a repo-relative link since that path is hub-local and not carried into every fleet repo, carries it whole as a generated include and surfaces it at its decision moment. +## Trust Boundaries and Hardening Effort + +Hardening is spent where a failure can actually occur. A defense against a case that cannot happen still costs a review round, a test, and the upkeep of both. So the effort an input gets is set by where it comes from and what its failure would do. + +- **Every input is trusted or untrusted, and the class sets the effort.** Trusted input is the maintainer's and what the maintainer controls. That covers files, images, and data handed to a session, settings and secrets files the maintainer writes, and command-line arguments the maintainer types. It covers this repository's content and the hub's, and the maintainer's own hosts with the output read back from them. It covers state files the repository's own scripts write. Untrusted input arrives from outside. That covers a request from an internet client and every field it controls, and upstream release metadata and downloads. It covers a third-party API response, content from an outside contributor, and a bot-authored pull request. +- **The class follows where data originates, not the channel carrying it.** Any channel holding content an outsider supplied is untrusted, whatever the list above names. That covers an argument or a checked-out tree holding a pull request's content. It covers a host's log line recording an outside request, and a state file caching a third-party response. The same channel carrying the maintainer's input stays trusted. +- **Against trusted input the only threat is a maintainer's mistake.** Guard a realistic mistake, such as a typo, CRLF line endings, a missing file, or a wrong unit. Guard it only where it would fail silently or destructively, with one check at the point of read that says what is wrong. Do not build a parser for adversarial shapes or test injection through a trusted file. Do not sanitize trusted text read back from the maintainer's own hosts against injection, or file an issue for an attack on trusted input. What agent-authored text may quote from those hosts is still "Representative Data in Agent-Authored Text"'s to decide. +- **Untrusted input is validated once, at the boundary where it enters**, in proportion to what it can reach from there. +- **Effort follows likelihood times consequence, for robustness as much as for trust.** Code that fails on its ordinary path in its deployment is a bug rather than a hardening case, and is fixed. A hardening case earns a fix or an issue only where it can realistically occur in the code's actual deployment. Its failure must also be silent or destructive, or be one an outsider can trigger. Every other case is declined with that reason, neither fixed nor filed. +- **A review finding asking for hardening the bullets above rule out is itself declined**, with no fix and no issue. The decline cites this section and the input or deployment that rules the case out. This binds the reviewer as much as the author, so a reviewer does not raise such a finding. A review brief asks after races, coercions, and platform differences only where the code's deployment can produce one. +- **A repository names its own boundaries where the classes above leave them unclear.** Its `OPERATIONS.md` lists which concrete inputs are trusted and which are not, under `Configuration Layout`. A session or a reviewer then applies this section without re-deriving it. A repository naming none uses the classes above. + +`GOVERNANCE.md` "Trust Boundaries and Hardening Effort" keeps the full rules, and the `fleet-code-review` Skill at `.agents/skills/fleet-code-review/SKILL.md` in the hub, not a repo-relative link since that path is hub-local and not carried into every fleet repo, carries it whole as a generated include and surfaces it wherever a change is reviewed. + ## PR Review Etiquette The provider-agnostic review-loop contract every fleet repo follows starts when a pull request opens. Open every fleet-owned pull request ready for review. Draft state is reserved for the separately documented upstream contribution workflow while a third-party contribution is still being prepared. Creating the pull request is not a terminal handoff. Run the review status once in the foreground. Then start the bounded review wait in a background process. Request a review at defined moments rather than on every push: when the pull request opens, and on the head of a pull request into the default branch, a promotion among them. On a pull request into any other branch, a fix push after the first round is covered instead by the recorded local pass the push already owes, published to the pull request so the review status can read it. Confirm that a review, or for such a fix push that published pass, covers the current head SHA, and that a review covers the full diff rather than only part of it. Where the round covering the head states no coverage at all, read the newest round that does state some as covering this head only where the pull request changes the same set of files at both commits, a head round's own statement always winning over a carried one. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files covers it, which is the reading a Copilot round at Balanced review effort gives, and a statement that reaches the head, stated on it or carried to it, wins over that table. Where no round covering the head carries a table of its own, the newest round that does, its table naming exactly the changed files, stands in under the bound a statement carries under, the pull request changing the same set of files at both commits. The table stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, so a pull request that ever had a partial round goes to the maintainer. Triage every finding, including low-confidence findings collapsed into the review body rather than threads. Reply to and resolve every addressed finding. Repeat after every fix until the checks are green, the current head is covered, and no finding is left open. Only an explicit maintainer instruction may stop, defer, or alter this default. Silence or a request that says only "open a PR" is not such an instruction. Never merge on a green or CLEAN merge state alone. That state does not prove the review covered the current head SHA and full diff. It also does not expose unanswered low-confidence findings that opened no thread. diff --git a/spec/files.json b/spec/files.json index 3956e45eb..ef5119f1b 100644 --- a/spec/files.json +++ b/spec/files.json @@ -8,7 +8,7 @@ { "path": "CLAUDE.md", "fidelity": "verbatim", "whole": true, "appliesTo": "*" }, { "path": "AGENTS.md", "fidelity": "intent", "sections": [{ "name": "Fleet Bootstrap", "fidelity": "verbatim" }, { "name": "Context and Delegation Discipline", "fidelity": "verbatim" }, { "name": "Where the Rules Live", "fidelity": "verbatim" }], "intentRef": "AGENTS.md", "appliesTo": "*" }, - { "path": "GOVERNANCE.md", "fidelity": "intent", "sections": [{ "name": "Foundational Principles", "fidelity": "verbatim" }, { "name": "Durable Knowledge and Self-Improvement", "fidelity": "verbatim" }, { "name": "Repository Boundaries and Write Safety", "fidelity": "verbatim" }, { "name": "Representative Data in Agent-Authored Text", "fidelity": "verbatim" }, { "name": "Git and Commit Rules", "fidelity": "verbatim" }, { "name": "Branching Model", "fidelity": "verbatim" }, { "name": "Release Model", "fidelity": "verbatim" }, { "name": "Operational Repositories", "fidelity": "verbatim" }, { "name": "Hub-Hosted Tooling", "fidelity": "verbatim" }, { "name": "Pull Request Title and Commit Message Conventions", "fidelity": "verbatim" }, { "name": "Documentation Style Conventions", "fidelity": "verbatim" }, { "name": "Verification Discipline", "fidelity": "verbatim" }, { "name": "PR Review Etiquette", "fidelity": "verbatim" }, { "name": "Communicating with the User", "fidelity": "verbatim" }, { "name": "Workflow YAML Conventions", "fidelity": "verbatim" }, { "name": "Supported Development Platforms", "fidelity": "verbatim" }, { "name": "Devcontainer", "fidelity": "intent" }, { "name": "Editor and Tasks", "fidelity": "verbatim" }, { "name": "Repository Details", "fidelity": "verbatim" }, { "name": "Repository Layout", "fidelity": "intent" }], "intentRef": "GOVERNANCE.md", "appliesTo": "*" }, + { "path": "GOVERNANCE.md", "fidelity": "intent", "sections": [{ "name": "Foundational Principles", "fidelity": "verbatim" }, { "name": "Durable Knowledge and Self-Improvement", "fidelity": "verbatim" }, { "name": "Repository Boundaries and Write Safety", "fidelity": "verbatim" }, { "name": "Representative Data in Agent-Authored Text", "fidelity": "verbatim" }, { "name": "Git and Commit Rules", "fidelity": "verbatim" }, { "name": "Branching Model", "fidelity": "verbatim" }, { "name": "Release Model", "fidelity": "verbatim" }, { "name": "Operational Repositories", "fidelity": "verbatim" }, { "name": "Hub-Hosted Tooling", "fidelity": "verbatim" }, { "name": "Pull Request Title and Commit Message Conventions", "fidelity": "verbatim" }, { "name": "Documentation Style Conventions", "fidelity": "verbatim" }, { "name": "Verification Discipline", "fidelity": "verbatim" }, { "name": "Trust Boundaries and Hardening Effort", "fidelity": "verbatim" }, { "name": "PR Review Etiquette", "fidelity": "verbatim" }, { "name": "Communicating with the User", "fidelity": "verbatim" }, { "name": "Workflow YAML Conventions", "fidelity": "verbatim" }, { "name": "Supported Development Platforms", "fidelity": "verbatim" }, { "name": "Devcontainer", "fidelity": "intent" }, { "name": "Editor and Tasks", "fidelity": "verbatim" }, { "name": "Repository Details", "fidelity": "verbatim" }, { "name": "Repository Layout", "fidelity": "intent" }], "intentRef": "GOVERNANCE.md", "appliesTo": "*" }, { "path": "CODESTYLE.md", "fidelity": "intent", "whole": true, "placeholders": ["InternalsVisibleTo project names"], "intentRef": "CODESTYLE.md", "appliesTo": "*" }, { "path": "WORKFLOW.md", "fidelity": "intent", "whole": true, "intentRef": "WORKFLOW.md", "appliesTo": "*" }, { "path": "README.md", "appliesTo": "*" }, diff --git a/spec/section-model.md b/spec/section-model.md index b0c813774..35ee01052 100644 --- a/spec/section-model.md +++ b/spec/section-model.md @@ -38,6 +38,7 @@ A section is one of the following. Fidelity is declared in [files.json][files], | Pull Request Title and Commit Message Conventions | `GOVERNANCE.md` | verbatim | universal, with generic examples | | Documentation Style Conventions | `GOVERNANCE.md` | verbatim | all rule text, including the Line Endings *rule* (a repo's `.editorconfig` value is not here) | | Verification Discipline | `GOVERNANCE.md` | verbatim | universal verification law | +| Trust Boundaries and Hardening Effort | `GOVERNANCE.md` | verbatim | universal law on where hardening effort is spent | | PR Review Etiquette | `GOVERNANCE.md` | verbatim | universal review-loop contract | | Communicating with the User | `GOVERNANCE.md` | verbatim | universal | | Workflow YAML Conventions | `GOVERNANCE.md` | verbatim | universal conventions, applied to whatever workflows a repo has |