Queue the agent-isolation rule and the vendored-tooling surface - #546
Conversation
Two rules the day argued for, both raised by the maintainer. An agent works in its own checkout, its own directory, on its own feature branch, and never writes to a repository because it happened to be on disk. The operations that cross that boundary are the ordinary ones: a blanket git add -A sweeps another agent's uncommitted work into your commit, a reset deletes it, a branch switch carries it. This session produced the first, against a second agent's in-progress registration work, which survived only because it was rescued and restored by hand. The same rule extends to reading, which is the commoner case and the one that looks harmless. A clone on disk is not the branch it names, it is whatever it was last fetched to, so reading it answers what that clone last saw. Two instances this session, both silent: a repository reported as drifted on a file already fixed, and a repository reported as missing a file it carries. Separately, every carried file is a copy that breaks when it is not current. configure.sh is the worked example: one hub fix became a re-vendor owed by every repository holding it, and until each takes it they run different logic while reporting the same command. Hosting the tool in the hub and calling it removes the stale copy rather than detecting it, which is the same argument as the reusable-workflow entry and should be settled with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Ready to approve
The changes are limited to documentation/TODO entries, with only minor wording/grammar nits noted.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR updates TODO.md to queue two new backlog items: (1) formalizing an agent-isolation rule for working (and reading) from local checkouts, and (2) reducing the amount of tooling that is vendored downstream by preferring hub-hosted, pinned consumption where appropriate.
Changes:
- Adds a new carried-content TODO entry proposing to reduce downstream vendoring of shared tooling (with
repo-config/configure.shas the motivating example). - Adds two new Host and Environment TODO entries describing “own checkout/branch” discipline and extending the same discipline to reads (prefer API reads or fetch-before-read; record ref+commit).
File summaries
| File | Description |
|---|---|
| TODO.md | Adds two new TODO entries about agent isolation (work + read) and one new TODO entry about shrinking the vendored-tooling surface. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Both from review. The clone sentence joined two independent clauses with a comma, which is a splice rather than a style preference, and the entry is about reading carefully so it should read carefully. The hub-hosting sentence was awkwardly built around what the hub does to a downstream, and now says what each side does. It also wrote "run time" where the file uses "runtime" twice elsewhere, so the file now says runtime three times. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Ready to approve
The change is a straightforward documentation/backlog update confined to TODO.md with no correctness or consistency issues found in the new text.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
The new TODO entries include two objective prose issues (a comma splice and an ungrammatical “answers instead …” construction) that should be corrected before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
TODO.md:38
- Comma splice: “A repository that misses the sweep does not fail loudly, it quietly audits itself ...” joins two independent clauses with a comma. Split into two sentences to avoid the splice.
- Reduce the surface that is copied downstream at all, since every carried file is a copy that breaks when it is not the current one. `repo-config/configure.sh` is the worked example from this session: a defect in its check mode was fixed once in the hub and immediately turned into a re-vendor owed by every repository carrying it, and until each takes the fix they run different logic while reporting the same command. A repository that misses the sweep does not fail loudly, it quietly audits itself with an older script. The alternative is for the hub to host the tool and for each downstream repository to call it rather than hold a copy, so one edit reaches the fleet and no stale copy exists to detect, re-vendor, or classify. This is the same argument as the reusable-workflow entry below and should be settled with it rather than separately, since both are about consuming hub code at runtime instead of vendoring it. Three things to settle. What the fleet actually needs vendored, which is the content a repository is audited against and could not run without, against what only needs to be reachable. How a consumed tool is pinned, because consuming hub code at runtime is a floating dependency unless it names a commit, and the action-pinning rule already requires exactly that. And what a repository does when the hub is unreachable, since a vendored copy still works offline and a called one does not, which is the real cost being traded away.
TODO.md:64
- Grammar: “answers instead what that clone last saw” is missing a connector (e.g., “answers instead: ...” / “answers instead of ...”), which makes the sentence ungrammatical.
- Extend the same rule to **reading**, which is the more common case and the easier one to think is harmless. A clone on disk is not the branch it names. It is whatever that clone was last fetched to, so reading it to answer what a repository carries answers instead what that clone last saw. The failure is silent, because the read succeeds and returns a plausible answer. This session produced two instances. A detection pass over local clones reported one repository as still drifted on a file whose fix had already merged, because that clone's refs predated the merge, and the correction came from a fetch rather than from the read. A second pass then reported a repository as missing a file it does carry, because the local checkout sat on a branch behind the one being measured. State the working practice with the rule: read the live ref through the API where a claim will be acted on, or fetch immediately before reading, and name the ref and commit in any finding derived from a local read so a reader can tell what was actually measured. Note the exception worth keeping, which is that a local clone is the right tool for anything needing history or a build, since those are what an API read cannot give.
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Three commits. Merged as a **merge commit**, never squashed, per the branching model. | Commit | PR | What | | --- | --- | --- | | `e48d64d` | #544 | Regenerates the divergence ledger against the previously promoted `main` | | `90e3255` | #545 | Takes the ruleset bypass list out of the fleet config entirely | | `362aec8` | #546 | Queues the agent-isolation rule and the vendored-tooling surface | ## The one that matters **#545 is a behaviour change to `configure.sh`, not a doc edit.** All three ruleset payloads declared `bypass_actors: [{ actor_id: 5, actor_type: RepositoryRole, bypass_mode: always }]`, so applying them granted repository admins a standing exemption on `main` and `develop` in every repo. GitHub's documentation is explicit that a ruleset applies to administrators by default and that the bypass list starts empty. Admins are *eligible* for bypass and must be added. So the declaration was a real grant rather than a restatement of something inherent, and the fleet config was the thing handing it out. Removing the declaration alone would have been worse than leaving it. `apply` sends the payload as a `PUT`, which replaces the whole document, so a payload with no `bypass_actors` would have **cleared the live list on every run**. That is code deleting a bypass. `apply` now reads the live list and writes it back unchanged, and aborts rather than proceeding if that read fails. On create it sends none, so a new ruleset starts at GitHub's own empty default and nothing is deleted, because nothing existed. `check` reports the list on every run and asserts nothing, since no payload declares a value to compare against and inventing one would put code back in charge of a human decision. This does **not** remove the bypass entries already live on the fleet's rulesets. Removing those would itself be code deleting a bypass. They are now visible on every `check` run so they can be cleared deliberately, per repo, in the UI. ## Expected consequence, stated rather than discovered `repo-config/configure.sh` is carried `verbatim` with `appliesTo: "*"`, so this promotion makes every repo holding a copy stale against the hub. That is the third re-vendor this file has generated in one session, which is exactly the evidence the vendored-tooling entry added in #546 now records. The follow-up plan is to fold that re-vendor into the payload migration already owed by five repos, so each gets one visit that renames its `ruleset-*.json` payloads and takes the current script, rather than two. ## Verification `spec/validate.py` OK (21 cataloged), diff-scoped `prose_lint` clean, `markdownlint-cli2` clean, editorconfig clean, live read-only `check` against this repo passes with the bypass list reported and unasserted. The preserve path was proven by composing the `PUT` body against this repo's live ruleset without sending it: the payload declares nothing, the live list is `RepositoryRole 5 always`, and the composed body carries that same list back.
Closes#549, raised by the Blog agent from a Copilot review on a downstream re-vendor and declined there because `configure.sh` is carried `verbatim`. ## The defect The payload-driven ruleset comparison added in #540 built its normalizer on `walk/1`, which arrived in **jq 1.6**. On a host carrying jq 1.5 the filter does not degrade — it **fails to compile**, so `check_ruleset` reports drift on every parameterized rule it never actually compared. That is the inverse of the false clean #540 was written to close, and arguably worse: a false *failure* teaches an operator to distrust the tool, where a false pass merely fails to warn them. ## Verified on a real jq 1.5, not argued ```console jq version: jq-1.5-1-a5b5cbe --- OLD filter (calls walk): def n: walk(if type=="array" then sort else . end); n jq: 1 compile error --- NEW filter (defines its own): {"allowed_merge_methods":["merge","squash"],"nested":{"deep":[1,2,3]},"required_status_checks":[{"context":"a"},{"context":"b"}]} ``` The reported failure reproduces exactly, and the fix compiles and returns the sorted document. Output is byte-identical to jq 1.7 on the same input, including a deliberately nested array to confirm the local definition still recurses. Finding a genuine jq 1.5 took three attempts — `imega/jq:1.5` does not exist, Debian buster's archives are gone, and Alpine 3.8 ships a master build that already has `walk`. Ubuntu 18.04 has it. Worth recording, because "I could not reproduce it" would have been the wrong conclusion from the first two. ## The choice `walk` is defined inside the filter, so the script calls nothing jq 1.5 lacks and keeps one code path across versions. The alternative in the issue — assert a jq version up front and fail with a message — was considered and declined for the reason the issue itself gives: `check` is the read-only mode, and refusing to run at all is a worse outcome than running. The issue stated that as a preference rather than a decision, and I agree with it. ## Verification `bash -n` clean, diff-scoped `prose_lint` clean, and a live `check` against this repo still passes on all three parameterized rules of both rulesets. ## Downstream `configure.sh` is carried `verbatim` with `appliesTo: "*"`, so this joins the re-vendor the ledger already lists. It is the same file that generated a re-vendor three times today, which is precisely the evidence behind the vendored-tooling entry added in #546. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
) Fixes the false clean reported in #562: `scripts/pr_review.py` printed `suppressed=0` over a review body carrying `### Suppressed comments (2)`, so the gate that decides whether a review round is answered reported a clean round over findings no thread will ever surface. ## Cause The reviewer moved the section from its own `<details>` wrapper to a markdown heading nested inside the `Review details` wrapper, and `suppressed_blocks()` missed it twice over: - The primary path matched the heading against the wrapper's `<summary>`, which now reads `Review details`. - The fallback that exists for exactly this case scanned `DETAILS.sub('', body)`, which deletes every `<details>` block, including the one the heading now sits in. ## Fix Each region (every wrapper's contents, plus what is left outside them all) is scanned line by line for the heading, so the section is found as its own wrapper's `<summary>`, as a markdown heading nested inside another wrapper, or bare in the body. Neither old shape is retargeted away, since both appear across the rounds of a single pull request. A block now starts at its own heading, so `finding_count()` reads the heading's own `(N)` rather than the wrapper's, which would have floored two findings to one. ## Verification `python3 -m unittest discover -s scripts` passes (247 tests), with four cases added for the nested shape: that it reports at all, that the count is the heading's own, that both shapes report together in one run, and that the neighbouring file-summary wrapper is still not read as a finding. Replayed over the reviewer's own bodies on the last seventeen pull requests of this repository, the new parse recovers **6 findings across 5 rounds** that previously read as clean (on #546 and #561), and changes no count that was already right. The runbook section in `.github/copilot-instructions.md` gains the same fact, since its rule to match more than one *phrasing* did not say the section also *moves*, and a filter reading a wrapper's `<summary>` reports zero on the nested shape. Related: #562 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Answers the live half of #565, the six suppressed Copilot findings the digest defect fixed in #562 hid. A suppressed finding reaches no thread, so the digest was the only place it could have surfaced, and it reported `suppressed=0` for five rounds across two pull requests. ## The two prose defects Both are in `TODO.md`, both raised on #546 round 4, both still present on `develop`. | Entry | Was | Now | | --- | --- | --- | | downstream surface | `does not fail loudly, it quietly audits itself` | two sentences | | reading a local clone | `answers instead what that clone last saw` | `answers a different question, which is what that clone last saw` | ## The capitalization convention The reviewer asked for the proper noun on one bullet of #561, and that bullet is now internally consistent in lowercase. So the live question is not the line but the file-wide mix: `GOVERNANCE.md` alone carried **17 lowercase against 7 capitalized**, and the same mix is vendored fleet-wide, which is why settling it per file settles it nowhere. **`CODESTYLE.md` "Markdown and Spelling" gains item 5.** Prose capitalizes the format's name. Lowercase is for the strings a machine reads and nothing else: a tool or package name (`markdownlint`, `yzhang.markdown-all-in-one`), a settings key (`markdown.extension.toc.levels`), a heading anchor (`#markdown-and-spelling`), an identifier in code, and a file extension. What it settles is the mix rather than either spelling, since a file carrying both gives the next author no default and a reviewer a finding to raise on whichever one it wrote last. It lands in `CODESTYLE.md` because every repo carries that file, so the convention arrives with it rather than being re-decided per repo. **The sweep corrected 40 occurrences across 18 files** in documents, code comments, docstrings, and two `spec/audit.py` output strings. No identifier changed: the pattern skips a word adjacent to `.`, `-`, `_`, `[`, `#`, or a paren, so `heading_texts(markdown)` and `markdown.splitlines()` were left alone, and the result was read line by line before applying. ## For the reviewer Four `verbatim` regions changed, so every downstream repo is byte-mismatched until re-vendored: - Three `GOVERNANCE.md` sections by one word each, "Documentation Style Conventions", "Communicating with the User", and "Repository Details". - Two comment lines in `.markdownlint-cli2.jsonc`, which is `verbatim` and `whole`, so a config nothing else changed about now reports as drifted. `CODESTYLE.md` is `intent`, so its new item reaches the fleet as a rule each repo adopts in its own copy rather than as bytes to match. `TODO.md`'s re-vendor entry records all of this and now names **five files rather than three**, so the next sweep does not have to rediscover it from a diff that reads cosmetic. The three findings #565 records as already resolved were re-verified against the tree and need no change here. ## Verification | Gate | Result | | --- | --- | | `prose_lint.py` gating rules (charset, dupword, spelling) | clean tree-wide | | `test_prose_lint.py`, `test_repo_gate.py`, `test_pr_review.py` | pass (157 prose tests) | | `spec/audit.py --selftest` | PASS | | `scripts/repo_gate.py` | eol 0, sha-pin 0 | | `spec/validate.py` | 22 cataloged, 0 backlog, clean | | markdownlint-cli2 | 0 issues in 44 files | | cspell | 0 issues | | editorconfig-checker | clean | Every changed file kept its own line endings, which was checked after a first attempt flattened CRLF to LF and was reverted before anything was committed. `prose_lint.py --diff` reports **five warn-only findings** (four `comment-wrap`, one `dash`) on the three lines this diff touched in `spec/audit.py`, `spec/validate.py`, and `spec/fidelity-model.md`. All five predate this change and belong to the #519 backlog, where those files carry 101 and 8 violations respectively. Correcting the one line of each that this diff touches would leave the file no more conformant while burying a one-word sweep in rewrapped comments. ## Not in this PR - A `prose_lint` rule for the convention. The legitimate lowercase uses are the whole difficulty, so a checker is a design question rather than a follow-through, and stating the rule is what #565 asked for. - The fleet re-vendor, which is `TODO.md`'s entry and needs the maintainer to name the repos. Related: #565 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two backlog entries, both raised by the maintainer, both with worked examples from the session that produced them.
An agent works in its own checkout
Own directory, own feature branch, and never writes to a repository because it happened to be on disk.
The reason care is not enough is that the operations which cross the boundary are the ordinary ones. A blanket
git add -Asweeps another agent's uncommitted work into your commit. Agit reset --harddeletes it. A branch switch carries it into a change it has nothing to do with. Each command is correct in isolation and wrong only because of who else is in the directory.This session produced the first of those. A blanket add swept a second agent's in-progress onboarding work into an unrelated commit and pushed it. It survived only because it was rescued and restored by hand afterwards, and the same directory had already seen a
reset --hardand two branch switches that would have destroyed it outright rather than merely misfiling it.The same rule extends to reading, which is the commoner case
A clone on disk is not the branch it names. It is whatever that clone was last fetched to. Reading it to answer what does this repository carry answers instead what did this clone last see, and the failure is silent: the read succeeds and returns a plausible answer.
Two instances, both from this session, both wrong in a way nothing flagged:
fetch, not from the read.The entry states the practice alongside the rule: read the live ref through the API where a claim will be acted on, or fetch immediately before reading, and name the ref and commit in any finding derived from a local read so a reader can tell what was actually measured. It also keeps the honest exception, that a local clone is the right tool for anything needing history or a build, since an API read cannot give either.
Every carried file is a copy that breaks when it is not current
repo-config/configure.shis the worked example, and it happened twice today. A defect in its check mode was fixed once in the hub and immediately became a re-vendor owed by every repository carrying it. Until each takes the fix they run different logic while reporting the same command, and a repository that misses the sweep does not fail loudly — it quietly audits itself with an older script.Hosting the tool in the hub and having downstream call it removes the stale copy rather than detecting it. This is the same argument as the existing reusable-workflow entry, so the entry says to settle them together rather than separately. Three things to decide are recorded: what genuinely needs vendoring against what only needs to be reachable, how a consumed tool is pinned, since calling hub code at run time is a floating dependency unless it names a commit, and what a repository does when the hub is unreachable, since a vendored copy still works offline and a called one does not.
Verification
prose_lintclean,markdownlint-cli2clean, CRLF preserved. One file in the commit, staged by explicit path rather than a blanket add, for the reason the first entry describes.