Repository navigation
State the host-tools.json Carry Rule and Stop the Empty-Overlay Warning - #1486
Merged
Merged
Conversation
Every repository carries a root host-tools.json, an empty tools list where it adds nothing, on the same footing as OPERATIONS.md, so a maintainer who needs to declare a floor finds the file rather than having to know one may be created. STANDUP.md and the hub's own host-tools.json note already said so, and three surfaces said otherwise or said nothing. scripts/host_gate.py's comment at the point it loads an overlay called carrying no local file the common case, which is the reverse of the rule spec/files.json encodes as a baseline entry applying to every repository. docs/host-setup.md and scripts/README.md each described the file as one a repository adds when it needs more than the fleet does, which reads as conditional creation. All three now state the rule and why it is the rule, and the gate's comment says why an absence is silent there anyway, that being a conformance question spec/audit.py answers rather than a host one. overlay_above() named any ancestor carrying the file, so a bare run started in a subdirectory of a repository whose overlay declares nothing told the operator to re-run with --repo so that no floor would count. It now reads the overlay and passes over one declaring no tool, while still naming one it cannot read, since a bare run cannot tell an unreadable declaration from an empty one and the re-run is what turns the guess into a diagnostic. The walk still stops at the nearest carrier, a further ancestor being a different repository. The issue also proposed dropping the manifest entry or reporting a present but empty file as DRIFT. Both are declined: the stub is the conformant state rather than a leftover, which is what the carry rule says. Refs #1483
Ten findings from the strict local pass, every one against text the first commit wrote or left false elsewhere. STANDUP.md's re-run paragraph told the operator the gate warns whenever its working directory sits inside a repo whose overlay it did not read, which the same document's section 2 then makes false: the stub it carries draws no warning now. It names both residual cases instead, and docs/fleet-map.md records the pair rather than the one it had. docs/host-setup.md claimed the gate says nothing where it finds no file, which the ancestor warning contradicts, and the new paragraph left the next one's "declares it in that file" binding to AUDIT.md rather than to host-tools.json. The surviving sentence six lines above still described a repository creating a file of its own, which is the reading the change exists to retire. The warning said the re-run makes a declaration count, which is false for the unreadable overlay this same change routes to it: there the re-run exits 2 with a diagnostic. It now says the re-run reads it, true of both. scripts/README.md justified passing over an empty overlay by a floor-impact criterion the code does not implement, since it branches on the tools list being empty, and a name-only override changes no floor and still warns. overlay_above's summary line claimed it names an overlay this run would read differently, which a no-op override falsifies, and its closing sentence justified stopping the walk by every further carrier being another repository, which this repository's own spec/host-tools.json falsifies. The first is narrowed to what the function decides and the second is deleted rather than re-derived, the behavior stated without a rationale that does not hold. isinstance(tools, str) or tools was one expression with an unreachable arm, a diagnostic being a non-empty and so truthy string, and no test could pin it. It is two statements now, each separately readable. The nearest-carrier test built one carrier and so proved only that a carrier is found, so a second case builds two declaring ones and asserts the nearer wins. Declined: the is_file and read pair in overlay_above is a TOCTOU window, and an overlay removed between them is named by a warning that was correct about what it observed, the same outcome the single-stat version reached. Left alone: a bare run started under spec/ names spec/host-tools.json as an overlay, which predates this change and needs path knowledge the function does not have.
The second local review pass raised eight findings, every one against text the previous round wrote in answer to the first pass. The warning promised an outcome, and both wordings tried so far were false somewhere. "So its floors count" was false for the unreadable overlay, and "to read it" was false for every case, since overlay_above now parses the file to decide whether to warn at all. It states what happened and names the re-run without promising what the re-run yields: the overlay went unapplied, and here is the flag that applies it. scripts/README.md, docs/host-setup.md, and docs/fleet-map.md each carried a form of the same promise and now say unapplied too. Three surfaces said the gate names an ancestor's overlay only where that overlay declares a tool, which the unreadable branch this change added falsifies and a test already asserts. Each names both cases. STANDUP.md justified the absent warning by the stub declaring nothing, and the mechanism cannot engage there at all: the section 0 command runs from a hub checkout, whose own root carries a declaration, and main() walks ancestors only where the working directory carries none. The target is not an ancestor of the hub checkout either way. It gives that reason instead. The sentence deleted from overlay_above last round as false, that a further carrier is a different repository, survived verbatim as a test docstring, where this repository's own spec/host-tools.json falsifies it just as well. Both docstrings in that pair now say what their test asserts. overlay_above's docstring said a bare run cannot tell an unreadable declaration from an empty one, three lines above the branch that tells them apart. The reason it names an unparsable overlay is that the failed parse leaves open whether it declares anything, which is what it says now. The isinstance branch remains unpinnable, a diagnostic being truthy, so no test distinguishes it from the truthiness check below. It stays written out for the reader, and a comment says so rather than leaving the next pass to find it a third time.
The third local review pass raised three findings on text the second round wrote, each a claim about behavior rather than the behavior itself. Pieter authorized a round past the two-round budget to settle them. overlay_above's summary line said it returns the ancestor this bare run should name, which the branch's own nested-overlay test falsifies: an outer carrier declaring a tool goes unnamed when the inner one declares nothing, so a reader taking the summary as the contract writes that shape down as covered. It states what the walk returns instead, the nearest carrier unless that one declares nothing, and the body already carries the rest. The comment beside the isinstance branch claimed no test can tell it from the truthiness check under it, which is false: patching read_declaration to return an empty diagnostic separates them, verified rather than argued. The clause is gone, and what remains is the reason the branch is there, an empty diagnostic being the case truthiness alone would read as an empty declaration. docs/fleet-map.md still said the warning names the re-run that applies the overlay, which the sentence beside it falsifies, since a re-run against an unparseable one exits 2 with a diagnostic rather than applying anything. It names the re-run and promises nothing about it, matching the message. Declined, all four classed style by the pass and none a false claim: the G2 bullet reaching its stub carve-out two sentences after the clause it narrows, STANDUP.md offering a mechanism alongside the sufficient reason beside it, the is_file and parse pair in overlay_above being two syscalls where one stat stood before, and the tighten-only rule appearing twice in one section of docs/host-setup.md, which predates this change on both surfaces.
The fourth pass raised four findings, again all against text a previous round wrote. Pieter's call was to answer them by deleting rather than rewriting, since across four rounds the code settled after the first and every round since found defects only in prose describing it. Two were qualifiers each fix had narrowed and got wrong in a new way. overlay_above's summary line named one of its two None paths and dropped the other, having traded the reverse omission a round earlier, so the qualifier is gone and the body carries the cases. STANDUP.md offered the walk being disabled as the cause of a warning that is absent for an independent reason, which invites the false counterfactual that a hub root carrying no declaration would name the target. It states the sufficient reason alone, that the target is not an ancestor of the checkout the run starts in. Removing "so its declaration counts" from the warning last round left test_a_spaced_overlay_path_is_quoted_in_the_re_run asserting the absence of an unquoted path followed by a space, and nothing can follow the path now. Verified by neutering quote_argument: the case passed anyway. Anchored on the newline it fails again, verified the same way. The isinstance branch is pinned rather than argued about. Patching read_declaration to return an empty diagnostic separates it from the truthiness check under it, which the third round's message asserted without adding the test, so the test is here and fails with the branch removed. The helper docstring claimed an empty default payload would make every case pass for the wrong reason, true of the three asserting silence and not of the two asserting a warning, which fail outright.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe host gate now skips empty ancestor overlays, reports declaring or unreadable overlays, and preserves nearest-overlay selection. Tests cover these cases. Documentation describes repository stubs, layering, warnings, and rerun guidance. ChangesHost overlay behavior
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The host gate now suppresses warnings for empty repository overlays while continuing to identify declaring or unreadable overlays, with matching tests and guidance. No merge-blocking risk is evident. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 2 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 |
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The behavioral change is narrowly scoped, is covered by new/updated tests, and the documentation updates are consistent with the code and manifest contract.
Pull request overview
This PR clarifies and codifies the unconditional “carry host-tools.json” rule across documentation and code, and updates host_gate.py behavior so a repository’s empty overlay stub does not trigger a misleading “re-run with --repo” warning.
Changes:
- Update
scripts/host_gate.pyoverlay detection and warning text to skip empty overlays while still warning on unparseable ones. - Expand
scripts/tests/test_host_gate.pycoverage for empty, malformed, and nested overlay-carrier scenarios. - Align procedural and reference docs (
STANDUP.md,docs/host-setup.md,scripts/README.md,docs/fleet-map.md) with the clarified carry rule and the warning’s updated semantics.
File summaries
| File | Description |
|---|---|
| STANDUP.md | Updates the standup guidance to explain why the residual “no --repo” case cannot be covered by a warning. |
| scripts/tests/test_host_gate.py | Adds/adjusts tests to pin the new empty-overlay silence behavior and malformed-overlay warning behavior. |
| scripts/README.md | Updates the host_gate.py description to state the unconditional carry rule and the refined warning behavior. |
| scripts/host_gate.py | Implements empty-overlay skip in overlay_above() and refines warning wording (“did not apply”). |
| docs/host-setup.md | Documents the carry rule explicitly and explains why absence is silent in host_gate.py (audit reports baseline misses). |
| docs/fleet-map.md | Updates the gap-resolution narrative to match the new warning behavior and the empty-overlay exception. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
ptr727
added a commit
that referenced
this pull request
Sep 10, 2026
…g, and the Release-Trigger Retirement (#1489) Promotes thirteen merged pull requests from `develop` to `main`. ## What this promotes - **#1486** State the `host-tools.json` carry rule and stop the empty-overlay warning. `spec/files.json` now requires the declaration unconditionally, and `host_gate.py` no longer warns on an empty one. - **#1484** Fan the validator's pytest leg out over a `python-versions` input, so a caller with more than one interpreter no longer silently loses its other legs to a pinned 3.13, with an entry guard that refuses a value `fromJson` would otherwise admit. - **#1476** Retire the publish-on-merge release trigger, which `spec/files.json` applied to an interface contract whose task has no publish-on-merge path. - **#1479** Ship the release caller snippet and retire the not-yet-released claim. - **#1474** Exclude `node_modules` from markdownlint and Docker lint discovery at any depth. - **#1468** Move the carried-surface cluster off decision, recording the measurements and what stays unmeasured on the issues that hold them. - **#1498** Correct the release-caller snippet's publish-on-merge claim, raised by CodeRabbit against this promotion and fixed on its own branch, since a promotion PR's head is `develop`. - **#1494** Settle the markdown lint exclusion seam as a nested config. - **#1492** Check declared deployment environments and fix the carried-payload rule. - **#1501** Move the session handoff to a chain of labeled issues. Two of its units are verbatim fidelity, `AGENTS.md` "Session Scope" and `GOVERNANCE.md` "Communicating with the User", so every fleet repo picks them up on its next resync. - **#1505** Refuse a bare handoff link rather than reading it as no successor, answering a finding raised against this promotion. - **#1504** Reject a deployment environment name or branch carrying surrounding whitespace, answering two more. - **#1506** Encode an environment name before it becomes a path segment, delete a branch that cannot fire, and dedupe a registry name by case, answering two more and declining one on a reproduction. ## Closing references A `develop`-targeted pull request registers no closing reference, so the issues those pull requests settled close with this promotion. #1470 is the one that needs a word. Two of its seven acceptance items are not met as literally written, and the maintainer's decision to close it here anyway, with the basis for both, is recorded on the issue itself. Closes #1483 Closes #1473 Closes #1487 Closes #1482 Closes #1475 Closes #1470 ## Left open deliberately - **#1499**, **#1500** follow-ups #1501 filed rather than fixed. - **#1503** the validator-looser-than-its-consumer class, filed from findings on this promotion and deliberately not closed by it. Five instances recorded, and the fix is a design decision rather than a check. - **#1497** the Docker delta list never says to remove the `push:` block. - **#1472** the fleet-level pin call, deliberately not pre-empted by #1479. - **#1321**, **#1464** carry #1468's decision record and remain in their own scope. ## Verification `origin/main..origin/develop` merges clean, no end-of-line conflict. Each promoted pull request ran the fleet gate set on its own branch before merging; this promotion adds no content of its own. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01817rckKywjfoaEV5WF9TKs
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.
The decision
#1483 posed a fork: make the
spec/files.jsonentry conditional, orconfirm the unconditional requirement is deliberate and state it where a
reader looks. This repository has already settled it.
STANDUP.mdand thehub's own
host-tools.jsonnote both say a repository with nothing to addcarries the stub with an empty
toolslist, on the same footing asOPERATIONS.md, so the declaration sits where a reader finds it rather thanwhere they must know to look. A committed rule outranks a suggestion, so
this takes the second branch and fixes what contradicted it.
Declined, from the issue's own suggestions: dropping the manifest entry, and
reporting a present-but-empty file as
DRIFTso repositories can shed it.The stub is the conformant state rather than a leftover, which is what the
carry rule says.
What changed
Three surfaces were wrong or silent about the rule.
scripts/host_gate.py's comment at the overlay load called carrying nolocal file the common case, the reverse of what the manifest encodes. It
states the rule, and says why an absence is silent there anyway: the gate
measures a host against floors, and
spec/audit.pyis what reports abaseline file a repository does not carry.
docs/host-setup.mdsaid nothing about the carry rule. It leads with itand gives the reason.
scripts/README.mddescribed the file as one a repository adds when itneeds more than the fleet does, which reads as conditional creation.
The manufactured warning is fixed.
overlay_above()named any ancestorcarrying the file, so a bare run started inside a repository whose overlay
declares nothing told the operator to re-run with
--reposo that no floorwould count. It reads the overlay and passes over one declaring no tool,
while still naming one it cannot parse, since the failed parse is what
leaves open whether it declares anything. The warning names the re-run and
promises nothing about its outcome, because a re-run against an unparseable
overlay exits 2 with a diagnostic rather than applying anything.
STANDUP.mdanddocs/fleet-map.mdeach stated when the gate warns, andthe behaviour change made both false. Both now match it.
Verification
ruff format --check,ruff check,mypy,spec/validate.py,scripts/prose_lint.py,scripts/repo_gate.py(eol,eol-coverage, sha-pin),
scripts/build_dist.py --check, andscripts/docker_lint.py(markdownlint, cspell, editorconfig-checker) allclean.
reasoned about: an empty overlay is silent, a declaring one and an
unparseable one each warn.
Removing the empty-overlay skip fails two cases, removing the
isinstancebranch fails the case that patches
read_declarationto an emptydiagnostic, and neutering
quote_argumentfails the spaced-path case.asserting the absence of an unquoted path followed by a space, and nothing
can follow the path any more, so it passed with the quoting neutered. It
is anchored on the newline and fails again.
after the first pass and every round since found defects only in prose
describing it, so the last round answered by deleting the qualifiers that
kept drawing findings rather than rewriting them again.
Settles #1483. A
develop-targeted pull request registers no closingreference, so the issue closes with the promotion.
🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Bug Fixes