From 26af0619d011299ec11795211a59160adbd7e0db Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sat, 8 Aug 2026 07:50:30 -0700 Subject: [PATCH 1/3] Gate Every Prose Rule Now That the Backlog Is Zero (#620) Closes the last piece of [#519](https://github.com/ptr727/ProjectTemplate/issues/519), which is now closed with its evidence quoted on the issue. ## Why now The prose backlog reached zero at `20916ad`. `semicolon` and `dash` were warn-only because a whole-tree gate on them would have failed every run while several hundred findings sat in the tree. That reason has expired, and a warn-only tier over a clean tree is how the backlog grows back. Measured on this branch before writing the change, and again after: ```text python3 scripts/prose_lint.py . --check charset --check semicolon --check dash --check dupword \ --check spelling --check comment-wrap --check comment-case --check home-path 0 violation(s) across 0 file(s) ``` ## `home-path` gated nothing anywhere, which is the substantive fix `home-path` has been in `DEFAULT_RULES` since it was written, so a bare local run has always included it. It was named by neither CI step, so the rule that catches an absolute home path naming a real account ran on a developer's machine and gated nothing in CI. That is the pattern-detectable sliver of [`GOVERNANCE.md`](https://github.com/ptr727/ProjectTemplate/blob/develop/GOVERNANCE.md) "Representative Data in Agent-Authored Text", the section that exists because real paths carrying real names reached a public comment. [`OPERATIONS.md`](https://github.com/ptr727/ProjectTemplate/blob/develop/OPERATIONS.md) already recorded the hole, correctly and in detail. This closes it and drops the record, rather than leaving a runbook describing a gap that no longer exists. ## What stays warn-only, and why that is not an oversight `charset-unknown` alone. A finding there names a character no tier covers, and classifying one is a fleet-law edit rather than something the change that happened to type it can fix. Blocking on it would make an un-tiered character an unmergeable change instead of a question for the maintainer. The step keeps `continue-on-error: true` and is renamed from "Report prose backlog step" to "Report unclassified characters step", since there is no backlog left for it to report. `sentence-split` remains named by no invocation and stays that way here. It is deliberately outside `DEFAULT_RULES`, so promoting it is a separate decision rather than a consequence of this one, and `OPERATIONS.md` still records it as a gap. ## The composite action is unaffected [`.github/actions/prose-gate`](https://github.com/ptr727/ProjectTemplate/blob/develop/.github/actions/prose-gate/action.yml) runs `python3 "$SCRIPT" --diff "$BASE"` with no `--check` list, so it already gates the full default set, `home-path` included, over the lines a change touches. A downstream caller sees no behavior change from this pull request. Only the hub's own whole-tree steps move. ## Documentation kept level with the change - [`OPERATIONS.md`](https://github.com/ptr727/ProjectTemplate/blob/develop/OPERATIONS.md) carries the two CI invocations verbatim so a local run matches CI rather than exceeding it, so both lines are updated, the three-gap paragraph becomes two, and the sentence describing the second invocation as a backlog report is corrected. - [`scripts/README.md`](https://github.com/ptr727/ProjectTemplate/blob/develop/scripts/README.md) claimed five rules gate and the rest report. It now states the tree is zero and every default rule gates but one. It also names `home-path` for the first time, including the limit that it closes a sliver of its section and nothing more, because the exposure that section exists for was name-shaped and no pattern finds a name. - `TODO.md` loses the #519 entry and its link definition. ## Three corrections the closing comment carries rather than this diff The `TODO.md` entry being removed held three wrong figures, each re-measured before the issue was closed: | Claim in the entry | Measured | | --- | --- | | Fixed by `f7a6a13` (snippets) | Not a valid object in this repository. The snippets batch is `b002fac` (#600). | | 557 across 45 to zero, 184 in snippets | 553 across 44 to zero, batches 181, 241, 90 and 41, with today's checker run at every point | | #573 accounts for 37 of the 38 carried findings | 38 of 38. Pre-#573 checker reports 0 and post-#573 reports 38 over the identical bytes at `69688ec`. | ## Verification | Check | Result | | --- | --- | | New gating invocation, whole tree | 0 violations across 0 files | | `charset-unknown`, whole tree | 0 violations across 0 files | | `python3 scripts/test_prose_lint.py` | 198 tests, OK | | `python3 scripts/repo_gate.py` | `eol` 0, `sha-pin` 0 | | `actionlint` | exit 0 | | `editorconfig-checker` | exit 0 | | `python3 spec/validate.py` | 22 cataloged, 0 backlog repos classify cleanly | The four edited files keep their declared line endings, CRLF for the three Markdown files and LF for the workflow, which `editorconfig-checker` confirms. --------- Co-authored-by: Claude Opus 5 (1M context) --- .github/workflows/validate-task.yml | 12 +++++------ OPERATIONS.md | 10 +++++----- TODO.md | 31 ++++++++++++++++++++++------- scripts/README.md | 6 +++--- scripts/test_prose_lint.py | 2 +- 5 files changed, 39 insertions(+), 22 deletions(-) diff --git a/.github/workflows/validate-task.yml b/.github/workflows/validate-task.yml index 92c65133..96451b0e 100644 --- a/.github/workflows/validate-task.yml +++ b/.github/workflows/validate-task.yml @@ -68,13 +68,13 @@ jobs: - name: Check repo gates step run: python3 scripts/repo_gate.py - # The charset, duplicate-word, spelling and comment rules are clean tree-wide, so they gate. - # Every other prose rule reports in the step below without gating. + # Every default prose rule is clean tree-wide, so each one gates rather than reporting a backlog. + # `charset-unknown` is clean too and still reports, for the reason the step below gives. - name: Check prose step - run: python3 scripts/prose_lint.py . --check charset --check dupword --check spelling --check comment-wrap --check comment-case + run: python3 scripts/prose_lint.py . --check charset --check semicolon --check dash --check dupword --check spelling --check comment-wrap --check comment-case --check home-path # Warn-only, and visible rather than absent: an unrun check is one nobody acts on. - # The backlog is corrected as each file is next edited, or cleared in a deliberate batch. - - name: Report prose backlog step + # A finding here names a character no tier covers, and classifying it is a fleet-law edit rather than a prose fix. + - name: Report unclassified characters step continue-on-error: true - run: python3 scripts/prose_lint.py . --check charset-unknown --check semicolon --check dash --summary + run: python3 scripts/prose_lint.py . --check charset-unknown --summary diff --git a/OPERATIONS.md b/OPERATIONS.md index 29e621ba..d6cc9bc4 100644 --- a/OPERATIONS.md +++ b/OPERATIONS.md @@ -8,7 +8,7 @@ What verifying a change here requires, including the part CI cannot perform. The ### Run the gates the way CI runs them -CI passes explicit `--check` lists, and a bare `python3 scripts/prose_lint.py [file]` runs `DEFAULT_RULES`, which is those two lists plus `home-path`. What differs is the exit code rather than the coverage: CI gates on `charset`, `dupword`, `spelling`, `comment-wrap` and `comment-case` and reports the other three warn-only, where a bare run exits non-zero on any of the nine. `sentence-split` is in neither and is asked for by name. Run the CI invocations: +CI passes explicit `--check` lists, and a bare `python3 scripts/prose_lint.py [file]` runs `DEFAULT_RULES`, which is those two lists together. What differs is the exit code rather than the coverage: CI gates on eight of the nine and reports `charset-unknown` warn-only, where a bare run exits non-zero on any of the nine. `sentence-split` is in neither, so nothing below runs it and a local run reaches it only by naming it. Run the CI invocations: ```sh python3 scripts/test_prose_lint.py @@ -17,18 +17,18 @@ python3 scripts/test_pr_review.py python3 spec/audit.py --selftest python3 host-setup/agent-safety/gh-write-guard.py --selftest python3 scripts/repo_gate.py -python3 scripts/prose_lint.py . --check charset --check dupword --check spelling --check comment-wrap --check comment-case -python3 scripts/prose_lint.py . --check charset-unknown --check semicolon --check dash --summary +python3 scripts/prose_lint.py . --check charset --check semicolon --check dash --check dupword --check spelling --check comment-wrap --check comment-case --check home-path +python3 scripts/prose_lint.py . --check charset-unknown --summary for f in registry/*.json spec/*.json repo-config/*.json; do jq empty "$f"; done python3 spec/validate.py docker run --rm --pull=always -v "$PWD":/check --workdir /check mstruebing/editorconfig-checker:latest ``` -Three gaps in that list are CI's rather than this runbook's, reproduced here so a local run matches CI rather than quietly exceeding it. The `jq` glob covers `repo-config/*.json` and does not reach `repo-config/operational/develop.json`, so a malformed operational payload passes. And `sentence-split` is implemented and tested but named by no invocation, so nothing runs it. The third is `home-path`, which is in `DEFAULT_RULES` and so runs on every bare local run, yet is named by neither CI list, so the pattern-detectable half of the representative-data rule gates nothing in CI. It is clean tree-wide today, which is why the gap is a hole rather than a backlog. +Two gaps in that list are CI's rather than this runbook's, reproduced here so a local run matches CI rather than quietly exceeding it. The `jq` glob covers `repo-config/*.json` and does not reach `repo-config/operational/develop.json`, so a malformed operational payload passes. The second is that `sentence-split` is implemented and tested but named by no invocation, so nothing runs it. Run the `editorconfig-checker` line before pushing a new file, and before pushing an existing file that a script rewrote rather than an editor. This repository defaults to CRLF and most tooling writes LF, so a new file fails that check on its first CI run rather than locally. A scripted rewrite is the same hazard on a file that was already correct, since reading and rewriting a whole file in text mode converts every line ending in it, which no prose or Markdown gate reports. -The first prose invocation gates. The second reports the backlog that is corrected as each file is next edited, or cleared in a deliberate batch, and it exits non-zero locally whenever findings exist. It is warn-only in CI because the workflow step sets `continue-on-error: true`, not because the command is lenient, so a non-zero exit locally is the expected result rather than a problem. +The first prose invocation gates. The second reports a character that no tier covers, and it exits non-zero locally whenever findings exist. It is warn-only in CI because the workflow step sets `continue-on-error: true`, not because the command is lenient, so a non-zero exit locally is the expected result rather than a problem. Scope a run to what changed, which matches the correct-as-next-edited rule: diff --git a/TODO.md b/TODO.md index d265ffe8..76ab186d 100644 --- a/TODO.md +++ b/TODO.md @@ -322,6 +322,29 @@ One pull request, after a survey, deciding whether anything stands between this - **Settled** - The reviewer counts the file and does not read it, rather than losing it earlier. The stated denominator equals the API's own `changedFiles` on **103 of 104** pull requests, the exception being one whose branch shrank between rounds. - **Settled** - Splitting remains a real remedy for a feature branch and is unavailable for a promotion, whose head is `develop`, so a promotion carrying a partial round is a maintainer decision by construction. +### A Resolve the Loop Cannot Perform and a Thread Nobody Can Find + +The review loop ends by replying on a thread and resolving it, and both halves failed on one pull request in ways the runbook describes nowhere. The resolve mutation was refused by the agent harness's own permission layer before any request left the machine, seconds after the reply mutation carrying the identical thread id had succeeded, so the refusal was neither GitHub's nor the id's. Handing the resolve to the maintainer then failed a second time, because the digest names a thread by its `PRRT_` node id, that id appears nowhere in the GitHub interface, and the person asked to resolve it could not find what to click. + +**State** `ready`. **Touches** `scripts/pr_review.py`, the runbook section in [`.github/copilot-instructions.md`][copilot-instructions], and [`OPERATIONS.md`][operations]. **Cost** one pull request, since the query change is one field and the runbook change is one paragraph. + +- **Carry a thread's own web address beside its node id, so a resolve can be handed to a person.** `Q_THREADS` selects `id`, `isResolved`, `path`, `line` and the first comment's `author` and `body`, and not its `url`, so the digest can name a thread and cannot point at it. Selecting `url` and printing it beside the id makes the hand-off one click. + - **Blocked by** - Nothing. + - **Checked** - `develop` at `0e4a1c2` on 2026-08-08, reading `Q_THREADS` in `scripts/pr_review.py` against the digest line that consumes it. + - **Detail** - The two identifiers are not interchangeable and neither is derivable from the other without a query. A `PRRT_` node id is what a mutation takes, and a `#discussion_r` fragment is what the web page anchors on. + - **Detail** - The evidence is [#620][pr-620], where a thread was handed over by node id and the reply was that it could not be found. + +- **Give the runbook a shape for a write the harness refuses, which it currently has none for.** Its list of dead paths is entirely GitHub's own refusals, a silent no-op, a 422, and the wrong bot login for the API in use, so a local refusal matches none of them and reads as a bad identifier, which invites the retry a blocked write must never get. + - **Blocked by** - Nothing. + - **Checked** - `develop` at `0e4a1c2` on 2026-08-08, against the known-non-working-paths list in the runbook. + - **Detail** - The distinguishing evidence is that a reply on the same thread id, in the same session, had already succeeded and returned a comment url, so the identifier was demonstrably good. + - **Detail** - What cleared it was a permalink and a human click, and the durable remedy is a permission rule in host settings. That is host state rather than repo content, so it belongs in the runbook as a note rather than in a committed configuration file. + +- **Confirm a resolve by re-reading the thread rather than by the mutation returning.** `reply` already exits 63 where the resolve did not report the thread resolved, which is the right shape, and a loop driving `gh api` by hand gets no exit code at all and so cannot notice. The rule worth writing down is that the state is the evidence. + - **Blocked by** - Nothing. + - **Checked** - `develop` at `0e4a1c2` on 2026-08-08, reading the exit-code table in the `scripts/pr_review.py` module docstring. + - **Detail** - This is the failure the suppressed-findings count already exists for, where a step that stopped running reads exactly like a step that passed. + ## Standalone Chores Small work with no research to preserve, selectable one bullet at a time. @@ -418,12 +441,6 @@ Actions on issues that are the maintainer's to take, each carrying its evidence Each was checked against the tree and has nothing left to do anywhere. Closing is the maintainer's call, and each wants the evidence quoted in the closing comment rather than a bare close. -- **[#519][issue-519], the hub's own tree does not pass the prose gate it ships.** Complete on the prose and on both questions. - - **Fixed by** - `f7a6a13` (snippets), `c9c92dd` (comments), `d791930` (hub-only Markdown), and the carried batch on `prose/carried-semicolons`. - - **Checked** - `develop` at `d791930` on 2026-08-07, where `python3 scripts/prose_lint.py --summary` reported 41 across 6 files, and 0 across 0 with the carried batch applied. - - **Closing evidence** - The whole-tree figure went 557 across 45 to zero, in four batches split by surface, being 184 in `catalog/snippets/`, 241 in non-Markdown comments, 90 in hub-only Markdown and 41 in the six carried files. Question 1 is answered by `reports/` being exempt as a generated tree, and question 2 by the snippets leading, since a non-conformant snippet seeds its violations into every repo that adopts it. - - **Closing evidence** - The issue's claim that the governance files were clean, and that this was therefore not a carry problem, was true of the checker of the day and false of the tree. Today's checker reports 38 findings against the same six files as they stood at `69688ec`, the commit the issue measured, while that commit's own checker reports zero. Scoping the list exemption to a sentence rather than a whole bullet accounts for 37 of the 38, because a colon anywhere ahead of the first semicolon had exempted every semicolon after it. The carry problem was real throughout and invisible, which is the stale-exemption hazard running in the loose direction. - - **[#557][issue-557], the agent-isolation rule and its two open questions.** Complete on the rule and on both questions. - **Fixed by** - `9d85941`. - **Checked** - `develop` at `9d85941` on 2026-08-06. @@ -447,7 +464,6 @@ Each was checked against the tree and has nothing left to do anywhere. Closing i [issue-483]: https://github.com/ptr727/ProjectTemplate/issues/483 [issue-489]: https://github.com/ptr727/ProjectTemplate/issues/489 [issue-509]: https://github.com/ptr727/ProjectTemplate/issues/509 -[issue-519]: https://github.com/ptr727/ProjectTemplate/issues/519 [issue-521]: https://github.com/ptr727/ProjectTemplate/issues/521 [issue-523]: https://github.com/ptr727/ProjectTemplate/issues/523 [issue-550]: https://github.com/ptr727/ProjectTemplate/issues/550 @@ -464,6 +480,7 @@ Each was checked against the tree and has nothing left to do anywhere. Closing i [pr-591]: https://github.com/ptr727/ProjectTemplate/pull/591 +[pr-620]: https://github.com/ptr727/ProjectTemplate/pull/620 diff --git a/scripts/README.md b/scripts/README.md index 7b9188c4..b5d23876 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -18,7 +18,7 @@ uvx coverage@latest run --source=. -m unittest discover -s scripts && uvx covera ## `prose_lint.py` -Enforces the [`GOVERNANCE.md`][governance] "Documentation Style Conventions" rules that no linter checks: non-ASCII judged against the charset rule's three tiers, a semicolon in prose, a spaced hyphen joining or interrupting a sentence, a duplicated consecutive word, a British spelling, and the shape of a comment's prose. +Enforces the [`GOVERNANCE.md`][governance] "Documentation Style Conventions" rules that no linter checks: non-ASCII judged against the charset rule's three tiers, a semicolon in prose, a spaced hyphen joining or interrupting a sentence, a duplicated consecutive word, a British spelling, and the shape of a comment's prose. It carries one rule from elsewhere in that document, `home-path`, which comes from "Representative Data in Agent-Authored Text" and catches an absolute home path naming a real account. That rule closes the pattern-detectable sliver of its section and nothing beyond it, since the exposure the section exists for was name-shaped and no pattern finds a name. It is the one rule a checkout can turn off: an operational repository's runbook carries the literal path an operator types, so a run there drops `home-path` and says so on stderr rather than going quiet. A repository states its own model by which configuration payload it carries, and the hub carries both, so it reads as a release repository and the rule gates here. The tiers decide by context rather than by a flat ban. Tier 1 carries no meaning its ASCII form loses and always flags. Tier 2 is an operator, kept next to a figure or another operator and replaced between words, so a threshold table reads as the range it is. Tier 3 is a unit or scientific symbol whose ASCII form would be a lie and never flags. Developer-typed characters such as emoji are preserved regardless of tier, and an un-tiered one is still reported as `charset-unknown` until it is classified. @@ -30,7 +30,7 @@ Run it scoped to changed lines, matching the standing rule that existing prose i python3 scripts/prose_lint.py . --diff origin/develop ``` -Whole-tree (`python3 scripts/prose_lint.py .`) reports the legacy backlog as well, which is informational rather than a gate. `charset`, `dupword`, `spelling`, `comment-wrap` and `comment-case` are clean tree-wide, so CI gates those five and reports the rest warn-only. +Whole-tree (`python3 scripts/prose_lint.py .`) reports zero, so a finding is a line the change under review wrote rather than backlog it inherited. CI gates every rule in the default set on that basis, except `charset-unknown`, which reports warn-only because classifying a character is a fleet-law edit rather than a prose fix. The default rule set covers comment shape (`comment-wrap` and `comment-case`) alongside the prose rules. It did not, which meant a run nobody parameterized reported clean on a wrapped comment while the rule read as enforced, and comment shape is the most frequently regressed rule in agent-authored work. Reading the backlog it exposes needs no flag now, and gating it needed `--diff` while the tree carried several hundred of them. That backlog is cleared, so both comment rules gate whole-tree, and `--diff` is now about scoping a run rather than about surviving one. @@ -72,7 +72,7 @@ A comment sentence also has to start with a capital, which `comment-case` checks **A comment whose whole body is a URI is a reference rather than a sentence**, and neither rule applies to it. It cannot be capitalized or restructured without corrupting the address it exists to carry, so before the exemption every repo carrying a reference block inherited a finding no edit could answer. Consecutive reference lines are separate addresses rather than one sentence wrapping, which is why the exemption also stops the line below a URI from reading as its continuation. A URI inside a sentence is still prose, so the exemption requires the whole body to be the address and nothing else. -`charset`, `dupword`, `spelling`, `comment-wrap`, and `comment-case` are clean tree-wide and gate CI. `charset-unknown`, `semicolon`, and `dash` run as one warn-only CI step, so the remaining backlog is visible without blocking and is corrected as each file is next edited, or cleared in a deliberate batch. +Every rule in the default set is clean tree-wide, which is what lets CI gate them over the whole tree rather than over a diff. `charset-unknown` reports instead of blocking, for the reason the tier discussion above gives. ## `repo_gate.py` diff --git a/scripts/test_prose_lint.py b/scripts/test_prose_lint.py index cbe6b2ad..96765401 100644 --- a/scripts/test_prose_lint.py +++ b/scripts/test_prose_lint.py @@ -1406,7 +1406,7 @@ class TestChangedLines(unittest.TestCase): The repo policy is that existing prose is corrected as each file is next edited rather than swept, and this parse is the whole mechanism behind it. A parse that returns too little makes - the warn-only step silently stop reporting, and one that returns too much reports the backlog + a diff-scoped run silently stop reporting, and one that returns too much reports the backlog as if the change introduced it. """ From 039be28b40343893eecb49ecf94c40f8056f10ad Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sat, 8 Aug 2026 08:50:52 -0700 Subject: [PATCH 2/3] Name the Exception in the Line That Claims Every Rule Gates (#622) Fixes a defect the [#621](https://github.com/ptr727/ProjectTemplate/pull/621) promotion's review found. A promotion's head is `develop`, so the fix cannot land on that pull request and takes its own branch, which the promotion then carries. ## The defect The gating step's comment claimed every default prose rule gates, one line above the comment naming `charset-unknown` as the one that reports instead. Both cannot be true, and the second is. ```text # Every default prose rule is clean tree-wide, so each one gates rather than reporting a backlog. # `charset-unknown` is clean too and still reports, for the reason the step below gives. ``` ## Where the overstatement came from It is the previous fix's. Round 2 of [#620](https://github.com/ptr727/ProjectTemplate/pull/620) narrowed "every prose rule" to "every default prose rule", to stop the claim covering `sentence-split`, which sits outside `DEFAULT_RULES` and gates nowhere. That narrowing walked straight into `charset-unknown`, which sits inside `DEFAULT_RULES` and still does not gate. | Rule | In `DEFAULT_RULES` | Gates in CI | | --- | --- | --- | | `charset`, `semicolon`, `dash`, `dupword`, `spelling`, `comment-wrap`, `comment-case`, `home-path` | yes | yes | | `charset-unknown` | yes | no, warn-only by design | | `sentence-split` | no | no | One phrase cannot carry both exclusions. The line now states the count and lets the next line name which rule it is. ## Swept rather than patched at the reported line The same shape was in `scripts/README.md`, where one sentence said being clean is what lets CI gate them and the next sentence excepted one of them. That instance was not reported and is corrected here, since a claim written in two places drifts in two places. ## Verification | Check | Result | | --- | --- | | Gating invocation, whole tree | 0 violations across 0 files | | `python3 scripts/test_prose_lint.py` | 198 tests, OK | | `actionlint` | exit 0 | | `editorconfig-checker` | exit 0 | --------- Co-authored-by: Claude Opus 5 (1M context) --- .github/workflows/validate-task.yml | 4 ++-- scripts/README.md | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/validate-task.yml b/.github/workflows/validate-task.yml index 96451b0e..a26b53bf 100644 --- a/.github/workflows/validate-task.yml +++ b/.github/workflows/validate-task.yml @@ -68,8 +68,8 @@ jobs: - name: Check repo gates step run: python3 scripts/repo_gate.py - # Every default prose rule is clean tree-wide, so each one gates rather than reporting a backlog. - # `charset-unknown` is clean too and still reports, for the reason the step below gives. + # Every default prose rule is clean tree-wide, so all but one block a change that adds a finding. + # `charset-unknown` is the exception and reports in the step below, for the reason given there. - name: Check prose step run: python3 scripts/prose_lint.py . --check charset --check semicolon --check dash --check dupword --check spelling --check comment-wrap --check comment-case --check home-path diff --git a/scripts/README.md b/scripts/README.md index b5d23876..b0315e89 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -72,7 +72,7 @@ A comment sentence also has to start with a capital, which `comment-case` checks **A comment whose whole body is a URI is a reference rather than a sentence**, and neither rule applies to it. It cannot be capitalized or restructured without corrupting the address it exists to carry, so before the exemption every repo carrying a reference block inherited a finding no edit could answer. Consecutive reference lines are separate addresses rather than one sentence wrapping, which is why the exemption also stops the line below a URI from reading as its continuation. A URI inside a sentence is still prose, so the exemption requires the whole body to be the address and nothing else. -Every rule in the default set is clean tree-wide, which is what lets CI gate them over the whole tree rather than over a diff. `charset-unknown` reports instead of blocking, for the reason the tier discussion above gives. +Every rule in the default set is clean tree-wide, which is what lets the CI gate read the whole tree rather than a diff. `charset-unknown` is the one that reports instead of blocking, for the reason the tier discussion above gives. ## `repo_gate.py` From 674a27aa6be545600e2c9ae215b1374b40ff57d2 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sat, 8 Aug 2026 10:05:01 -0700 Subject: [PATCH 3/3] Carry the Permission Rules the Review Loop Needs in the Installer (#624) The kit configured `settings.json` for the hook alone, so a permission rule added on one machine reached none of the others. The installer now carries the rules it owns, which makes re-running it the way those rules reach every host. ## What changed `install.py` gains a `MANAGED_PERMISSIONS` list, each entry a prefix and a rule, merged in the same pass that registers the hook. The settings file is still read once and written once, so the hook and the rules land together or not at all. | Step | Before | After | | --- | --- | --- | | 1 | self-test the hook | unchanged | | 2 | register the `PreToolUse` hook | unchanged | | 3 | write the `CLAUDE.md` blocks | **merge `permissions.allow`** | | 4 | | write the `CLAUDE.md` blocks | ## Why a prefix rather than a plain append A re-run drops every rule under the prefix before adding the current one, which is the same strip-then-register the hook registration already uses. A rule whose spelling changes therefore updates in place rather than accumulating beside the version it replaced, and the prefix bounds what the installer owns so a rule written by hand outside it is never touched. ## The one rule it carries today `Bash(python3 scripts/pr_review.py:*)`, for the review loop's reply and resolve. Driving that loop by hand needs a raw GraphQL mutation, which is the shape that reached a stranger's repository. `scripts/pr_review.py` queries the thread id itself, takes no argument an id fits in, and refuses to resolve a thread whose reply did not land, so allowing the script is narrower than allowing the mutation it replaces. These widen rather than restrict, so they are their own component for the same reason the `fleet-bootstrap` block is separate from the `agent-safety` one: the safety half's own text says nothing in it widens a permission. Keeping a grant in its own step preserves that, where folding it into the hook registration would not. ## Verification, run against throwaway `CLAUDE_HOME` trees | Case | Result | | --- | --- | | Fresh install, no `settings.json` at all | `added`, list holds the one rule | | Re-run, unchanged | `already current`, no duplicate rule, hook count still 1 | | One older spelling present | `updated, superseding 1` | | Two older spellings present | `updated, superseding 2`, collapsed to one rule | | Unrelated rules and other keys | `Bash(git -C * log *)`, `additionalDirectories` and `effortLevel` all preserved | | `settings.json` is not valid JSON | exits 1, names the file, writes nothing | Gates on this branch: prose gate 0 whole-tree, `gh-write-guard.py --selftest` PASS, `repo_gate.py` clean, `editorconfig-checker` exit 0, `markdownlint-cli2` 0 issues. The prose gate caught two wrapped comment sentences in the new code before the first push, which is the gate promoted to blocking earlier today doing the job it was promoted for. ## What this does not do It installs nothing anywhere. Running it on each host stays the maintainer's action, tracked by the per-machine rollout in [#365](https://github.com/ptr727/ProjectTemplate/issues/365). --------- Co-authored-by: Claude Opus 5 (1M context) --- host-setup/agent-safety/README.md | 11 ++- host-setup/agent-safety/install.py | 131 +++++++++++++++++++++++++++-- 2 files changed, 135 insertions(+), 7 deletions(-) diff --git a/host-setup/agent-safety/README.md b/host-setup/agent-safety/README.md index 98a37f80..b97c6ebd 100644 --- a/host-setup/agent-safety/README.md +++ b/host-setup/agent-safety/README.md @@ -10,6 +10,7 @@ Into `~/.claude/` (or `%USERPROFILE%\.claude\` on Windows): - **Two marker-delimited blocks of host-wide content in `CLAUDE.md`**, loaded into every session on the machine (including ad-hoc work outside any project), installed and updated independently so one can change without rewriting the other. - The `agent-safety` block carries restrictions alone, so nothing in it can widen a permission. `Repository Boundaries and Write Safety` mirrors the committed `GOVERNANCE.md` section of the same name, carrying its write rules and its rule that a task works in its own checkout as behavioral guidance. `Data in Agent-Authored Text` mirrors `GOVERNANCE.md` "Representative Data in Agent-Authored Text", so an agent illustrates with constructed data rather than data observed on the machine. `Authorization Scope and Memory Hygiene` bounds how a granted permission is recorded and read. Each mirrors a rule that otherwise reaches fleet repos only. - The `fleet-bootstrap` block carries `Fleet Governance Entry Point`, which names the template repository and routes by the state a repository is actually in. It is separate precisely because it enables rather than restricts, and the safety block's own text says nothing in it widens a permission, so merging the two would contradict that. It is host-wide rather than per repository because the repositories that most need it are the ones carrying no instruction set to point the way, and it mirrors the byte-locked `AGENTS.md` "Fleet Bootstrap" section that a conformant repository carries. +- **The permission rules this kit owns, merged into `settings.json`** beside the hook registration. Each is declared as a prefix and a rule, and a re-run drops every rule the prefix owns before adding the current one, so a rule whose spelling changes updates in place rather than accumulating beside the version it replaced. Ownership requires a rule-syntax delimiter after the prefix, since the prefix ends at the script name and a bare prefix test would also claim a longer path such as `pr_review.py-custom`, so a rule written by hand for a different script is never touched. These widen rather than restrict, which is why they are their own component for the same reason the `fleet-bootstrap` block is separate from the `agent-safety` one. Today the list holds one rule, for `scripts/pr_review.py`, the review loop's reply and resolve. Driving that loop by hand needs a raw GraphQL mutation carrying a node id, which is the shape that reached a stranger's repository, where the script queries the id itself and takes no argument an id fits in. What the rule decides is which command runs without a prompt, and it matches the command text rather than the directory the command runs in, so it reaches a `scripts/pr_review.py` in any checkout that carries one. An absolute path would not narrow that, since the hub is reached as a checkout of the caller's own and its location differs per task, so pinning one path would name a checkout the next task does not use. What bounds it is the rule that an agent reaches the hub as a checkout of its own, fetched immediately before it is read, rather than a copy it happens to find on disk, which the `fleet-bootstrap` block beside this carries and [`GOVERNANCE.md`][governance] "Hub-Hosted Tooling" states in full. The hook is the mechanical backstop. The CLAUDE.md rules and the carried GOVERNANCE.md rules are the behavioral layer. Prose alone is not enough, since the incident happened under prose rules, so both ship. The GitHub write rules have a hook behind them, as do the git operations that bypass a repository protection, while which checkout a command belongs in, the data a line of text quotes, and how an authorization is recorded are prose only, since no hook can see any of the three. @@ -25,7 +26,7 @@ host-setup/agent-safety/install.sh .\host-setup\agent-safety\install.ps1 ``` -Both are thin wrappers around `install.py`, so every OS runs one tested code path. The installer self-tests the hook before registering it, merges the settings.json entry without clobbering other keys, and updates each CLAUDE.md block in place by its own markers rather than duplicating it, so the two blocks move independently. +Both are thin wrappers around `install.py`, so every OS runs one tested code path. The installer self-tests the hook before registering it, merges the settings.json hook entry and the permission rules without clobbering other keys, and updates each CLAUDE.md block in place by its own markers rather than duplicating it, so the two blocks move independently. The settings file is read once and written once, so the hook and the permission rules land together or not at all. **Restart Claude Code sessions on the machine afterward** so the new hook and CLAUDE.md load. @@ -39,6 +40,7 @@ The deployed copy on each machine is a snapshot, so when the guard changes upstr python3 ~/.claude/hooks/gh-write-guard.py --selftest # decision matrix: all cases pass grep -c 'agent-safety v' ~/.claude/CLAUDE.md # expect 2 (start + end marker) grep -c 'fleet-bootstrap v' ~/.claude/CLAUDE.md # expect 2 (start + end marker) +grep -cF 'Bash(python3 scripts/pr_review.py:*)' ~/.claude/settings.json # expect 1 (never duplicated) ``` On Windows PowerShell: @@ -47,6 +49,7 @@ On Windows PowerShell: py -3 "$env:USERPROFILE\.claude\hooks\gh-write-guard.py" --selftest # all cases pass (Select-String 'agent-safety v' "$env:USERPROFILE\.claude\CLAUDE.md").Count # expect 2 (Select-String 'fleet-bootstrap v' "$env:USERPROFILE\.claude\CLAUDE.md").Count # expect 2 +(Select-String -SimpleMatch 'Bash(python3 scripts/pr_review.py:*)' "$env:USERPROFILE\.claude\settings.json").Count # expect 1 ``` Live end-to-end (in any repo): attempt a discarded-output write and confirm the Bash tool is blocked: @@ -65,10 +68,15 @@ The installer writes this. It is here so you can inspect or hand-place it: "PreToolUse": [ { "matcher": "Bash", "hooks": [ { "type": "command", "command": "\"python3\" \"/.claude/hooks/gh-write-guard.py\"" } ] } ] + }, + "permissions": { + "allow": [ "Bash(python3 scripts/pr_review.py:*)" ] } } ``` +Every other key in the file is left as it stands, `permissions.allow` included, apart from the rules whose prefix this kit owns. + ## Scope and Limits - **Per-machine.** `~/.claude/` does not travel, so run the installer on each box. This is the rollout that [#365][issue-365] tracks. @@ -80,4 +88,5 @@ The installer writes this. It is here so you can inspect or hand-place it: - **Not a credential control.** A fine-grained PAT limited to owned repositories is a separate, stronger structural guard (a hard `403` on any non-owned repo) and is left to per-machine credential setup, out of this kit. +[governance]: ../../GOVERNANCE.md [issue-365]: https://github.com/ptr727/ProjectTemplate/issues/365 diff --git a/host-setup/agent-safety/install.py b/host-setup/agent-safety/install.py index 5a02a696..8858c02f 100644 --- a/host-setup/agent-safety/install.py +++ b/host-setup/agent-safety/install.py @@ -1,8 +1,9 @@ #!/usr/bin/env python3 """Install the agent write-safety kit for the current user account. Cross-platform, idempotent. -Deploys the PreToolUse hook, registers it in the user settings.json, adds the safety rules to the user -CLAUDE.md (marker-delimited so re-runs update in place), and self-tests the hook before registering it. +Deploys the PreToolUse hook, registers it in the user settings.json, merges the permission rules this +kit owns into the same file, adds the safety rules to the user CLAUDE.md (marker-delimited so re-runs +update in place), and self-tests the hook before registering it. The bash and PowerShell wrappers both call this, so every OS runs one tested code path. Usage: python3 install.py (installs to ~/.claude) @@ -18,6 +19,42 @@ HERE = pathlib.Path(__file__).resolve().parent +# Distinguishes an absent key from one holding an explicit null, which `dict.get` reports alike. +# The two need different answers, since a gap is filled and a null is a settings error. +MISSING = object() + + +def owns(entry, prefix): + """Whether an allow rule names the script the prefix identifies, rather than a longer path. + + The prefix ends at the script name, so a bare `startswith` also claims `pr_review.py-custom`, + and dropping that would delete a hand-written rule for a different script. What separates the + two is the character after the name: a rule that invokes this script continues with a rule-syntax + delimiter, where a different script continues with more of its own path. + """ + return entry.startswith(prefix) and entry[len(prefix):len(prefix) + 1] in (":", " ", ")") + + +def at(data, path): + """The value at a slash-separated key path, or MISSING where any step of it is absent.""" + node = data + for part in path.split("/"): + if not isinstance(node, dict) or part not in node: + return MISSING + node = node[part] + return node + +# Permission rules this kit installs, each as (owned prefix, rule). +# A re-run drops every rule the prefix owns before adding the current one, so a changed rule updates in place. +# Ownership needs a delimiter after the prefix, so a longer path such as `pr_review.py-custom` is not claimed. +# These widen rather than restrict, so they stay their own step for the reason the two CLAUDE.md blocks stay separate. +MANAGED_PERMISSIONS = [ + # The review loop's reply and resolve, the one write in that loop an agent performs. + # Driving it by hand needs a raw GraphQL mutation carrying a node id, which is the shape to avoid. + # The rule decides which command skips a prompt, and it bounds no checkout, since it matches the text. + ("Bash(python3 scripts/pr_review.py", "Bash(python3 scripts/pr_review.py:*)"), +] + def hook_launcher(): """A python invocation for the settings.json command. Prefer a bare `python3` (portable and @@ -61,15 +98,63 @@ def main(): launcher = hook_launcher() # Quote the launcher too: the sys.executable fallback can contain spaces (e.g. C:\Program Files\...). hook_cmd = f'"{launcher}" "{hook_dst}"' + # Read into a variable rather than twice off disk, once to test for content and once to parse. + # Two reads can also disagree, since another process may write between them. data = {} - if settings.exists() and settings.read_text(encoding="utf-8").strip(): + raw = settings.read_text(encoding="utf-8") if settings.exists() else "" + if raw.strip(): try: - data = json.loads(settings.read_text(encoding="utf-8")) + data = json.loads(raw) except json.JSONDecodeError as e: sys.stderr.write( f"{settings} exists but is not valid JSON ({e}). Fix or remove it, then re-run.\n" ) return 1 + # A settings file is an object, and any other JSON value parses cleanly and breaks every lookup below. + # The root is therefore checked before the keys under it are. + if not isinstance(data, dict): + sys.stderr.write( + f"{settings} is valid JSON but holds {type(data).__name__} at its root where an object " + "is required. Fix or remove it, then re-run. This file is unchanged, so the hook is " + "deployed but not registered.\n" + ) + return 1 + # Every container this installer descends into is checked before it is used. + # A key holding an unexpected type would otherwise raise a traceback mid-edit. + # That reads as a crash rather than as the settings problem it is. + # The invalid-JSON refusal above is the shape this file already answers a malformed file with. + def reject(where, held, want): + sys.stderr.write( + f"{settings} has `{where}` as {type(held).__name__} where {want.__name__} is required. " + "Fix or remove that key, then re-run. This file is unchanged, so the hook is deployed " + "but not registered.\n" + ) + + for path, want in (("hooks", dict), ("hooks/PreToolUse", list), + ("permissions", dict), ("permissions/allow", list)): + held = at(data, path) + # An explicit null is present rather than absent, and `setdefault` hands back the null it found. + # It is therefore rejected here rather than read as a gap the default fills. + if held is not MISSING and not isinstance(held, want): + reject(path.replace("/", "."), held, want) + return 1 + + # A list of the right type can still hold the wrong elements. + # The registration below reads each group as an object, and each group's `hooks` as a list it appends to. + groups = at(data, "hooks/PreToolUse") + if groups is not MISSING: + for i, g in enumerate(groups): + if not isinstance(g, dict): + reject(f"hooks.PreToolUse[{i}]", g, dict) + return 1 + if "hooks" in g and not isinstance(g["hooks"], list): + reject(f"hooks.PreToolUse[{i}].hooks", g["hooks"], list) + return 1 + for j, h in enumerate(g.get("hooks") or []): + if not isinstance(h, dict): + reject(f"hooks.PreToolUse[{i}].hooks[{j}]", h, dict) + return 1 + pre = data.setdefault("hooks", {}).setdefault("PreToolUse", []) # Strip the hook from every existing group first, so a re-run leaves no duplicate behind. # That matters when settings.json already carries more than one Bash group. @@ -83,10 +168,40 @@ def main(): group = {"matcher": "Bash", "hooks": []} pre.append(group) group.setdefault("hooks", []).append({"type": "command", "command": hook_cmd}) + done = ["PreToolUse/Bash hook registered"] + + # 3. Permission rules, merged under the prefixes this installer owns. + # The strip-then-register shape is the hook registration's above, applied to a flat list. + # Written in the same pass as the hook, so the file is read once and written once. + allow = data.setdefault("permissions", {}).setdefault("allow", []) + for prefix, rule in MANAGED_PERMISSIONS: + matched = [a for a in allow if isinstance(a, str) and owns(a, prefix)] + allow[:] = [a for a in allow if a not in matched] + [rule] + # Counted over what the write removes rather than over what the prefix matched. + # The current rule matches its own prefix, so a match-set count reports it as superseded. + # A duplicate of it is removed too, and both can happen at once, so both are named. + older = [a for a in matched if a != rule] + duplicates = max(0, len(matched) - len(older) - 1) + changes = [] + if older: + changes.append(f"superseding {len(older)}") + if duplicates: + changes.append(f"removing {duplicates} duplicate" + ("s" if duplicates > 1 else "")) + if not matched: + action = "added" + elif changes: + action = "updated, " + " and ".join(changes) + else: + action = "already current" + done.append(f"permission {rule}: {action}") + + # Reported after the write rather than as each edit is made, since both edits share one write. + # A line printed before it claims a change that a later failure would leave unmade. settings.write_text(json.dumps(data, indent=2) + "\n", encoding="utf-8") - print(f" settings -> {settings} (PreToolUse/Bash hook registered)") + for line in done: + print(f" settings -> {settings} ({line})") - # 3. CLAUDE.md carries one marker block per snippet, replaced where present and appended where not. + # 4. CLAUDE.md carries one marker block per snippet, replaced where present and appended where not. # The two blocks install and update independently, so one can change without rewriting the other. # The safety block states restrictions only. # The fleet block enables, so it stays separate from a block whose own text says nothing in it widens a permission. @@ -113,6 +228,10 @@ def main(): print(f" {launcher} \"{hook_dst}\" --selftest") print(f" grep -c 'agent-safety v' \"{claude_md}\" # expect 2") print(f" grep -c 'fleet-bootstrap v' \"{claude_md}\" # expect 2") + # One line per rule, matching the rule itself rather than a word inside it. + # A hint naming a fixed word would stop matching the moment a rule that lacks it is added. + for _, rule in MANAGED_PERMISSIONS: + print(f" grep -cF '{rule}' \"{settings}\" # expect 1") print("Restart Claude Code sessions on this machine so the hook and CLAUDE.md load.") return 0