Give the Verbatim Section Re-Vendor a Tested Tool - #1423
Conversation
Every resync hand-rolls the verbatim section re-vendor. `carry.py` owns whole trees and `spec/audit.py` names a stale section without writing anything, and between them sits the one apply step in `RESYNC.md` with no tool behind it, which is the step the AGENTS.md-overwrite incident happened on. Add `check-sections` and `apply-sections` to `carry.py`. They read `spec/files.json` for the sections a repository carries at verbatim fidelity, replace only those regions, and then re-read the file to assert that the level-two heading sequence is unchanged, that every line outside the replaced regions is byte-identical to what was there before, and that each declared section now matches the hub's canonical. The second assertion is the one that earns its keep. The 2.0.577 pilot's throwaway replacer dropped one blank line between the preamble and the first heading, which is invisible in a diff review, breaks no renderer, and no gate in this repository flags it. A mutation test proves each of the six new guards fails its own case when removed. Region boundaries come from `spec/audit.py`'s own fence reading rather than a second reading of CommonMark, so a heading shown inside a code sample cannot end a region for this tool and not for the check that judges the result. The replacement carries the terminator the section's own heading line carried, so a CRLF repository keeps its endings. Four states refuse rather than guess: a declared section the repository does not carry, an absent file, a file declaring both placeholders and verbatim sections, and a region holding content the fidelity comparison normalizes away. Verified read-only against three fleet repositories at their fetched origin/develop: 63 sections compared, zero disagreements with `spec/audit.py`'s own extractor, and the verdicts reproduce the known state, Utilities and PlexCleaner clean after their resyncs and HomeAutomation-Config ten sections behind on its frozen hub pin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ten findings from the pre-push adversarial pass, all on text this change wrote. Refuse a section that ends the file in one copy and not the other. A region runs to the next level-two heading, so the blank line before that heading is inside it and a section ending the file has none. The hub's `Where the Rules Live` is its last, so re-vendoring it into a repository carrying a local section after it would run that heading onto the region's last line, and padding the region would leave bytes the fidelity comparison reports as drift forever. Both are wrong, so this names the conflict rather than picking one. No fleet repository is in that state today, and `spec/section-model.md` describes exactly the migration that produces one. Fold case in the heading-sequence postcondition, which `section_span` deliberately does when it locates a region. A downstream heading differing only in case is drift this tool fixes, and the check read the corrected casing as the section set having changed and refused a file it had just made correct. Fold case when deduplicating a section name too, since the lookup folds. Two spellings of one heading survived an exact-string dedupe and resolved to one region, which spliced its replacement in twice against indices the first splice had already moved. `replace_sections` now refuses overlapping regions outright. Plan every unit before writing any of them, so a refusal reached partway through no longer leaves one file re-vendored and the next not, reported by the same exit code as a refusal that touched nothing. Exempt no owned root from the unrelated-changes check. A tree's root holds only hub-owned content, where these files hold the repository's own sections too, so the exemption would have let a write land on an uncommitted edit to a section the tool never touches. Refuse a bare carriage return, which `spec/audit.py`'s `normalize` reads as a line break and this tool does not, rather than misdiagnosing the file as missing a section the audit can see. Carry the two new menu entries into `menu.ps1`, verified with the PowerShell parser and PSScriptAnalyzer. Use `selector_set` and `HUB_NAME` in `run` rather than the inline copy and the string literal each replaced. Seventeen guards, each mutation-tested. The harness itself had to be fixed first: two mutations that delete equal-length text produce equal-sized files, and CPython validates a cached `.pyc` on modification time and size alone, so within one tick the second run imported the first mutation's bytecode and reported a guard as unguarded that is guarded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The second review pass disproved the refusal the first pass produced. It claimed that padding a re-vendored region with the blank line a following heading needs would leave bytes the fidelity comparison reports as drift forever. It does not. `spec/audit.py`'s `extract_section` splits the document on LF and joins the region back with LF, so a file-final newline and a blank line before the next heading both render as one trailing empty element, and the two forms compare equal. Verified directly against that function. So the refusal goes, and with it the premise it rested on. The comparison now uses `comparable_region`, which reproduces `extract_section` exactly, and the write reconciles the boundary to what the target document needs: a section that ends the hub's file keeps a blank line before whatever follows it downstream, and one that ends the target's file gains no trailing blank line. Both render identically to the check that judges them, and only one of them leaves a document a Markdown linter accepts. The rest of that pass, all on text this branch wrote: Move the section modes to `RESYNC.md` step 1, where the instruction set's verbatim sections are already ordered. Step 3 was wrong twice over: it put the instruction set after the deletions and the tree carry, which step 1 exists to prevent, and it put a clean-worktree requirement after the step that dirties the worktree, so the command could not run. Extract the write loop as `apply_plans`, and report a write failure rather than letting an `OSError` out as a traceback. Sweep placeholders across every applicable entry for a path rather than only the entry that also carried the verbatim sections, since the two can be declared separately and the file is written once. Say in `scripts/README.md` what a refusal actually guarantees, which is that a refused run writes nothing, not that no run can fail partway. Name the section modes in the menu contract and in both menus' help text. Twenty guards, each mutation-tested. End to end against `HomeAutomation-Config`'s real ten stale sections in a scratch copy: all ten cleared, zero disagreements with `spec/audit.py` across the 21 declared sections, and the heading sequence and every line outside the declared regions byte-identical. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Warning Review limit reachedNext included review available in 49 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughAdded ChangesVerbatim section resynchronization
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: ⚪ Minimal · up to The section synchronization workflow is covered by validation and tests, with no actionable merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 51.56% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 64 functions across 3 files. (4 skipped: 4 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
|
`comparable_region` replaced `region_text` in both comparisons, and the write path builds its own lines because it needs the target's own terminators, so nothing in the module called `region_text` any more. Its docstring reference, which claimed it was the form to write, was wrong for the same reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
|
There was a problem hiding this comment.
🟢 Approval recommended
Core behavior changes are well-guarded by comprehensive tests, with only minor user-facing help-text typos noted.
Pull request overview
Adds a first-class, tested implementation for re-vendoring verbatim Markdown sections inside mixed-fidelity files (e.g., AGENTS.md/GOVERNANCE.md) as part of fleet resync operations, closing the tooling gap described in #1401.
Changes:
- Extend
scripts/carry.pywithcheck-sections/apply-sectionsto re-vendor only manifest-declared verbatim##regions and assert postconditions (heading sequence unchanged, outside-bytes unchanged, and sections match hub canonical). - Add extensive unit tests covering section parsing, replacement behavior, boundary/EOL edge cases, and refusal modes.
- Update operator documentation and host-setup menus to surface the new section modes.
File summaries
| File | Description |
|---|---|
| scripts/tests/test_carry.py | Adds unit tests for section-span extraction, replacement, planning, refusal behavior, and post-apply assertions. |
| scripts/README.md | Documents the new check-sections / apply-sections modes and their guarantees/refusals. |
| scripts/carry.py | Implements section re-vendor planning, replacement, assertions, and CLI dispatch for section modes. |
| RESYNC.md | Updates resync step ordering/instructions to use the new section modes early in the apply order. |
| host-setup/README.md | Updates host-setup documentation to mention mixed-file verbatim section pulls via carry.py. |
| host-setup/menu.sh | Adds downstream menu options for checking/applying verbatim section re-vendors and updates usage text. |
| host-setup/menu.ps1 | Adds downstream menu options for checking/applying verbatim section re-vendors and updates usage text. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟢 Approval recommended
The changes are well-tested and consistent with the stated resync workflow, with only a minor error-message wording nit noted.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
scripts/carry.py:515
- Avoid the mid-sentence semicolon in this user-facing error message; repo prose rules require recasting semicolons into a comma or two sentences, and this will also read more clearly in stderr output.
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
Recast the semicolon in the governed-drift refusal as two sentences. `GOVERNANCE.md` "Documentation Style Conventions" bans one in agent-authored text, and `prose_lint.py` reads Markdown rather than Python string literals, so nothing local caught it. The rest of this branch's added prose was swept for the same shape and carries none. Reword the help text both menus end on. "The menu shows each the tasks that apply to it" is pre-existing on `develop` in both files, and this change only rewrapped the paragraph around it, but it was raised against both files here and it sits in the sentence this change rewrote, so it is cheaper to fix than to file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Answering the suppressed finding, which opens no thread to reply in.
Real, and fixed in 68f7540. The refusal now reads as two sentences:
Also fixed in the same commit, from the two threads above: the help-text sentence both menus end on. It is pre-existing on |
There was a problem hiding this comment.
🔵 Needs a closer look
replace_sections() can introduce mixed line endings when the target section heading line is unterminated, breaking the “preserve target EOL style” guarantee.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
scripts/carry.py:613
- When the target section heading line is unterminated (common when a file has no final newline),
ending = terminator(out[start]) or "\n"forces LF even if the rest of the target file uses CRLF. That can introduce mixed line endings in the rewritten region (and potentially in the whole file), contradicting the stated goal of preserving the target's EOL style.
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
A replacement takes its ending from the section's own heading line, and that line carries no terminator when the heading is the last line of a file with no final newline. The fallback was LF, so a CRLF file in that shape came back mixed, which is the opposite of what this tool states it does with line endings. Reproduced before fixing: a CRLF target ending `## X` with no final newline came back as `# T\r\n\r\n## X\n\nhub body\n`, two CRLF lines and three LF. The file now answers for itself, read from its first terminated line, and LF is the answer only for a file that has no terminated line at all. Raised by Copilot as a suppressed finding, which opens no thread. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Answering the suppressed finding raised on head
Real, and fixed in a5a969d. Reproduced first, on a CRLF target whose declared heading is the last line of a file with no final newline: The fallback now reads the file's own ending from its first terminated line, and LF is the answer only for a file that has no terminated line at all. The other suppressed finding this round is the semicolon one, already answered above and fixed in 68f7540. It reappeared because it was raised against the earlier head. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🔵 Needs a closer look
scripts/carry.py repeatedly re-runs audit_module() during per-line section scans, and caching the imported module once avoids unnecessary overhead in large documents.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
scripts/carry.py:375
- Avoid repeated
sys.pathscans and repeatedimport auditstatements on every line processed.fence_step()callsaudit_module()for each line in large documents, so caching the imported module once per process reduces overhead and makes the "imported on first use" contract explicit.
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
… Line `fence_step` calls `audit_module` once per line, and that function scanned `sys.path` and re-entered `import audit` on every one of them. Measured on this repository's own `GOVERNANCE.md`, 333 lines and 22 level-two sections: 28.8 ms to locate every section against 0.9 ms with the module held, over 7326 calls, and a re-vendor walks each file several times. `functools.cache` on the accessor, which keeps the import lazy while making "on first use" mean once rather than every time. Raised by Copilot as a suppressed finding, which opens no thread. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Answering the suppressed finding raised on head
Real and material, and fixed in e8a9017. Measured before fixing, on this repository's own
|
|
@coderabbitai review |
|
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces new write-and-assert behavior in a fleet-wide resync-critical tool, and should receive final human review despite strong automated coverage.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
… tool (#1426) Promote `develop` to `main`. One commit, #1423, which settles #1401. `scripts/carry.py` owned whole trees and `spec/audit.py` named a stale section without writing anything. Between them sat the verbatim section re-vendor, the one apply step in `RESYNC.md` with no tested tool behind it, hand-rolled on every resync, and the step the AGENTS.md-overwrite incident happened on. `check-sections` and `apply-sections` close it. They read `spec/files.json` for the sections a repository carries at `verbatim` fidelity inside an otherwise `intent`-fidelity file, replace only those heading regions, and re-read the file to assert that the level-two heading sequence is unchanged, that every line outside the replaced regions is byte-identical to what was there before, and that each declared section now matches the hub's canonical. `RESYNC.md` step 1 names the commands, and both menus expose them. Twenty-one guards, each mutation-tested. Verified end to end against `HomeAutomation-Config`'s real ten stale sections in a scratch copy: all ten cleared, zero disagreements with `spec/audit.py`'s own extractor across the 21 declared sections, and everything outside those sections untouched. Copilot reviewed every head with full coverage. Three of its findings arrived as suppressed comments carrying no thread, and all three were real: a banned semicolon in an error message, a mixed line-ending defect on a CRLF file whose declared heading is its unterminated last line, and a per-line re-import costing 28.8 ms against 1.1 ms on this repository's own `GOVERNANCE.md`. Each is answered in #1423's conversation with its evidence. CodeRabbit was rate limited throughout and is advisory here. Closes #1401 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…or It Per Line (#1430) Closes #1424. `scripts/build_dist.py`'s `_fence_step` calls into `spec/audit.py` once per line, and it scanned `sys.path` and re-entered `import audit` on every one of them. The accessor splits out as a cached zero-argument `_audit_module()`, the shape `scripts/carry.py` took for the same defect on #1423. ## Measured Over the 24 authored skills, 4644 lines across the 40 Markdown files they comprise, warm in one process, best of five: | | one pass | | --- | --- | | re-resolved per line | 17.5 ms | | module held | 0.9 ms | | calling `audit` directly | 0.7 ms | One `--check` run makes **18708** of these calls, four times that corpus, because it scans the authored tree and both generated trees. It runs on every pull request. ## The part that was nearly a regression The first draft used `functools.cache`. That is 3.9+, and `scripts/skills_install.py` imports this module **at module scope**, while `host-setup/linux/install-skills.sh`, `host-setup/windows/install-skills.ps1` and `scripts/skills_install.sh` each commit to **Python 3.7+**, because that path runs before the fleet's own toolchain exists to install a newer interpreter. `spec/host-tools.json`'s `python3` entry records that floor and why it sits below the fleet's own. So the draft would have broken `install-skills` at import on a 3.7 or 3.8 host, on the one path whose whole purpose is surviving an old one, and the laziness this function already had was there for exactly that reason. Verified by running it rather than by reading a version table, on a real CPython 3.8.20: ``` this branch import build_dist: OK import skills_install: OK audit imported? False functools.cache import build_dist: BROKE -> module 'functools' has no attribute 'cache' ``` `functools.lru_cache(maxsize=None)` is 3.2+ and identical for a zero-argument function. Ruff's own `py313` target wants to rewrite it back to `functools.cache`, so the `noqa: UP033` carries that reason inline. ## Tests Three, each proven by removing what it guards: - The module is resolved **once** however many lines are read. - Importing this module still does **not** import `audit`, run in a fresh interpreter. This is the laziness the install path depends on, and it was load-bearing and untested before now. - The existing `sys.path` case. Holding the module made it **vacuous**: the accessor's body then runs once per process, so the `not in sys.path` guard could be deleted with the whole suite still green. It now clears the cache between calls and covers that guard again. Caught by the local review pass, which mutation-proved it on both sides of the merge base. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved build checks by reusing audit functionality efficiently across repeated validation runs. * Preserved compatibility with supported Python versions by loading audit functionality only when needed. * **Tests** * Added coverage for repeated validation calls and fresh-interpreter import behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…le (#1432) Promote `develop` to `main`. One commit, #1430, which settles #1424. `scripts/build_dist.py`'s `_fence_step` reached into `spec/audit.py` once per line and re-resolved the module every time, scanning `sys.path` and re-entering `import audit`. The accessor is now a cached zero-argument `_audit_module()`, the shape `scripts/carry.py` took for the same defect on #1423. Over the 24 authored skills, 4644 lines across the 40 Markdown files they comprise, warm in one process, best of five: **17.5 ms re-resolved against 0.9 ms held**, where calling `audit` directly costs 0.7 ms. One `--check` run makes **18708** of these calls, four times that corpus, and it runs on every pull request. ## Worth reading before merging The first draft used `functools.cache`, which is 3.9+. `scripts/skills_install.py` imports `build_dist` **at module scope**, and three install wrappers commit to **Python 3.7+** because that path runs before the fleet's own toolchain exists to install a newer interpreter. The draft would have broken `install-skills` at import on a 3.7 or 3.8 host, and every gate passed it, since ruff and mypy both target 3.13. Proven on a real CPython 3.8.20 rather than argued from a version table, and fixed with `lru_cache(maxsize=None)`. Nothing gates that floor, which is now filed as #1431. The local review also caught that holding the module made an **existing** test vacuous: the `sys.path` guard could be deleted with the whole suite still green. It now clears the cache between calls and covers that guard again. CodeRabbit reviewed this one after being rate limited on #1423 and #1426 all evening, and raised that the laziness test read the child interpreter's absolute state rather than the import delta, which a startup hook could break. Fixed, with a skip rather than a vacuous pass where the delta cannot be read at all. Closes #1424 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Closes #1401.
scripts/carry.pyowns whole trees andspec/audit.pynames a stale section without writinganything. Between them sits the verbatim section re-vendor, the one apply step in
RESYNC.mdwithno tested tool behind it, hand-rolled on every resync, and the step the AGENTS.md-overwrite
incident happened on.
What lands
check-sectionsandapply-sectionsoncarry.py. They readspec/files.jsonfor the sections arepository carries at
verbatimfidelity inside an otherwiseintent-fidelity file, replace onlythose
## headingregions, and then re-read the file to assert three things: the level-two headingsequence is unchanged, every line outside the replaced regions is byte-identical to what was there
before, and each declared section now matches the hub's canonical.
The second assertion is the one that earns its keep. The 2.0.577 pilot's throwaway replacer dropped
one blank line between the preamble and the first heading. That is invisible in a diff review, it
breaks no renderer, and no gate in this repository flags it, so it would have shipped and the next
audit would have reported drift nobody could see. The issue's argument is that a hand-rolled step
gets that assertion only when whoever wrote it happened to write one.
Region boundaries come from
spec/audit.py's own fence reading rather than a second reading ofCommonMark, so a heading shown inside a code sample cannot end a region for this tool and not for
the check that judges the result. The replacement carries the terminator the section's own heading
line carried, so a CRLF repository keeps its endings.
Verification
Twenty guards, each mutation-tested: the guard is removed, a named test must fail, and the file
is restored from a byte copy rather than from git. All twenty caught.
End to end against real repositories. Read-only across three fleet repositories at their fetched
origin/develop: 63 sections compared, zero disagreements withspec/audit.py's own extractor, andthe verdicts reproduce the known state,
UtilitiesandPlexCleanerclean after their resyncs andHomeAutomation-Configten sections behind on its frozen hub pin. Then a real re-vendor of thoseten in a scratch copy: all ten cleared, zero disagreements across the 21 declared sections, and the
heading sequence and every line outside the declared regions byte-identical.
Full local gate set green:
ruff format --check,ruff check,mypy, 1142 unit tests,spec/validate.py,prose_lint.py,repo_gate.py,build_dist.py --check,shellcheck,shfmt,and for
menu.ps1the PowerShell parser and PSScriptAnalyzer.Two review rounds, and what the second one overturned
The first local pass raised ten findings, all answered in the second commit. The second pass
disproved one of those answers, which is why the third commit exists.
That answer had added a refusal for a section ending the file in one copy and not the other,
claiming that padding the region with the blank line a following heading needs would leave bytes the
fidelity comparison reports as drift forever. It does not.
spec/audit.py'sextract_sectionsplits the document on LF and joins the region back with LF, so a file-final newline and a blank
line before the next heading both render as one trailing empty element, and the two forms compare
equal. The refusal and its premise are gone. The comparison now uses a form that reproduces
extract_sectionexactly, and the write reconciles the boundary to what the target document needs.The same pass also caught that
RESYNC.mdstep 3 was the wrong home for these modes twice over: itput the instruction set after the deletions and the tree carry, which step 1 exists to prevent, and
it put a clean-worktree requirement after the step that dirties the worktree, so the command could
not have run.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation