Repository navigation
Conversation
Closes #1401. `scripts/carry.py` owns whole trees and `spec/audit.py` names a stale section without writing anything. Between them sits 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. ## What lands `check-sections` and `apply-sections` on `carry.py`. 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 then re-read the file to assert three things: the level-two heading sequence 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 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. ## 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 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. Then a real re-vendor of those ten 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.ps1` the 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`'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. The refusal and its premise are gone. The comparison now uses a form that reproduces `extract_section` exactly, and the write reconciles the boundary to what the target document needs. The same pass also caught that `RESYNC.md` step 3 was the wrong home for these modes 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 have run. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added section-level synchronization for verbatim Markdown rule sections within mixed-ownership files. - Added commands to check for drift or apply updates while preserving unrelated content and line endings. - Added downstream menu options for checking and pulling these sections, including dry-run support. - Updates validate section boundaries, encoding, and final content before completing. - **Documentation** - Expanded setup and command-line documentation with workflows, requirements, fidelity behavior, and usage examples. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- 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 1 minute. 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 (7)
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 |
Owner
Author
|
@coderabbitai review |
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are a few user-facing/help-text and consistency issues in the updated menus and carry.py error messaging that should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This promotion brings develop to main by landing the new scripts/carry.py section re-vendor capability (check-sections / apply-sections) and wiring it into the resync and host-setup workflows, so verbatim ##-section updates in mixed-ownership Markdown files are now tool-driven and postcondition-checked rather than hand-rolled.
Changes:
- Add section-level re-vendor commands to
scripts/carry.py, including strict planning/refusal guards and post-apply assertions. - Add comprehensive unit coverage for section re-vendor behavior and edge cases in
scripts/tests/test_carry.py. - Document and expose the new modes via
RESYNC.md,scripts/README.md, and the host-setup menus.
File summaries
| File | Description |
|---|---|
| scripts/carry.py | Implements check-sections/apply-sections modes with guarded region replacement and post-apply invariants. |
| scripts/tests/test_carry.py | Adds focused tests covering section boundary detection, EOL handling, refusals, and assertions. |
| scripts/README.md | Documents the new carry modes and adds the new test to the scripts test list. |
| RESYNC.md | Updates resync step ordering/instructions to use the new section carry commands early. |
| host-setup/README.md | Updates menu documentation to mention section-level re-vendor support. |
| host-setup/menu.sh | Adds menu entries for section check/apply and updates usage text accordingly. |
| host-setup/menu.ps1 | Adds menu entries for section check/apply and updates usage text accordingly. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… Menu Filters For (#1427) Answers three findings raised on promotion PR #1426, whose head is `develop`, so they land here first. ## The hub's name, in five places rather than one The finding was that `carry.py`'s tree-carry guard tests `name == HUB_NAME` and then names the hub as a literal in the message it raises. Fixing that line alone would have left the worse instance standing: `verify_hub` resolved the registry entry by literal too, so renaming the hub would have failed there, in every mode, before ever reaching the guard the finding was about. The origin-mismatch message beside it had the same shape. Both menus hard-coded the slug in a section header while holding it in `HUB_REPO` two lines above, so any fork would render two different hub names on one screen. Verified by rendering the menu rather than by reading it: ``` Hub: ptr727/ProjectTemplate (/home/pieter/repos/worktrees/ProjectTemplate-menu-grammar) ... Hub, ptr727/ProjectTemplate: ``` ## A guard with no test The guard whose message this change edits had none, because it sits behind `verify_hub` and a unit test would have needed a real hub checkout to reach it. It is now `refuse_hub_tree_target`, exercised directly, and the test fails when the guard is removed. Its sibling in `run_sections` already had one, so this was the untested half of a matched pair. ## The usage sentence, rewritten once more "Each is shown the tasks that apply to it" leaves "each" pointing at nothing the preceding sentence named, which is what the reviewer read as a missing subject. It now names the referent: "Only the tasks that apply to that checkout are shown." The local review pass was asked specifically whether this wording is correct and unambiguous rather than for another variant, since the sentence has now been rewritten twice, and it confirmed the referent binds and that the claim matches what the menus actually do. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
|
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The new section re-vendor modes are supported by thorough unit tests and consistent procedure/menu documentation, and I did not find correctness or contract issues in the reviewed changes.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
This was referenced Sep 7, 2026
ptr727
added a commit
that referenced
this pull request
Sep 7, 2026
…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)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Promote
developtomain. One commit, #1423, which settles #1401.scripts/carry.pyowned whole trees andspec/audit.pynamed a stale section without writinganything. Between them sat the verbatim section re-vendor, the one apply step in
RESYNC.mdwith notested tool behind it, hand-rolled on every resync, and the step the AGENTS.md-overwrite incident
happened on.
check-sectionsandapply-sectionsclose it. They readspec/files.jsonfor the sections arepository carries at
verbatimfidelity inside an otherwiseintent-fidelity file, replace onlythose 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.mdstep 1 names thecommands, and both menus expose them.
Twenty-one guards, each mutation-tested. Verified end to end against
HomeAutomation-Config's realten stale sections in a scratch copy: all ten cleared, zero disagreements with
spec/audit.py's ownextractor 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 isanswered in #1423's conversation with its evidence. CodeRabbit was rate limited throughout and is
advisory here.
Closes #1401
🤖 Generated with Claude Code