Repository navigation
Conversation
…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>
|
ⓘ 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 (2)
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 |
|
There was a problem hiding this comment.
🟢 Approval recommended
The change preserves the required lazy-import behavior for older-Python bootstrap paths, improves performance as intended, and adds targeted test coverage for the new caching semantics.
Pull request overview
This PR promotes develop to main by landing the change that avoids repeatedly re-resolving spec/audit.py during scripts/build_dist.py runs, while preserving the intentional lazy import needed by the skills installer path.
Changes:
- Add a cached
_audit_module()accessor (viafunctools.lru_cache(maxsize=None)) and route_fence_step()through it to avoid per-linesys.pathscanning and repeated import resolution. - Strengthen tests to (a) ensure the audit module is resolved once across many
_fence_stepcalls, (b) keep coverage for thesys.pathguard by clearing the cache between calls, and (c) assert that importingbuild_distdoes not importauditin a fresh interpreter.
File summaries
| File | Description |
|---|---|
| scripts/build_dist.py | Introduces a cached lazy accessor for audit and updates _fence_step to use it, reducing repeated import resolution overhead. |
| scripts/tests/test_build_dist.py | Adds regression tests for the caching behavior and verifies build_dist remains lazy with respect to importing audit. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Promote
developtomain. One commit, #1430, which settles #1424.scripts/build_dist.py's_fence_stepreached intospec/audit.pyonce per line and re-resolvedthe module every time, scanning
sys.pathand re-enteringimport audit. The accessor is now acached zero-argument
_audit_module(), the shapescripts/carry.pytook 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
auditdirectlycosts 0.7 ms. One
--checkrun makes 18708 of these calls, four times that corpus, and it runson every pull request.
Worth reading before merging
The first draft used
functools.cache, which is 3.9+.scripts/skills_install.pyimportsbuild_distat module scope, and three install wrappers commit to Python 3.7+ because thatpath runs before the fleet's own toolchain exists to install a newer interpreter. The draft would
have broken
install-skillsat import on a 3.7 or 3.8 host, and every gate passed it, since ruffand 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.pathguard could be deleted with the whole suite still green. It now clears the cache betweencalls 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