refactor(server): carve builtin-content seeding into lib/builtin_content.py (R3b) - #900
Merged
Conversation
…ent.py (R3b)
lib/builtin_content.py (321 lines moved). server.py 2,418 -> 2,098.
The calibration/diagnostic sloppaks and the starter library: _copy_builtin_packs,
_write_builtin_pack, the two seed helpers, their source tables, and the seed marker.
━━━ THE ONE SIGNATURE CHANGE, AND WHY THE CARVE IS UNSAFE WITHOUT IT ━━━
server.py has:
def _feedBack_server_root() -> Path:
return Path(__file__).resolve().parent
That is correct IN server.py: the repo root in dev, resources/feedBack when bundled — the
tree that actually holds docs/ and data/.
Move that body into lib/ unchanged and it keeps working, silently, and returns lib/. There
is no docs/diagnostics under lib/, so every seed would find nothing, log "source missing"
at debug, and return. Nothing raises. Nothing fails. The starter library simply never
appears, and the calibration sloppak is never seeded — on a fresh install, in the field.
A verbatim move whose MEANING changed because __file__ did.
So this module cannot compute a root: `server_root` is a PARAMETER, and server.py — the
only place that legitimately knows where it lives — passes it in. The trap is now
structurally impossible rather than merely avoided. (_copy_builtin_packs already took the
root that way; the two seed helpers now do too.)
Everything else is byte-identical. CONFIG_DIR is read late as appstate.config_dir and the
DLC root through dlc_paths._get_dlc_dir — the same seam every router in lib/routers/ uses,
late-bound because tests monkeypatch it.
━━━ PYFLAKES FOUND THREE MISSING IMPORTS THE TESTS WOULD HAVE FOUND ONE AT A TIME ━━━
The moved code uses `secrets`, `stat` and `tempfile`; none was in my import block. Each is
a NameError on a live path. `python3 -m pyflakes` names all three in one shot — this is the
Python twin of the no-undef gate that guarded every frontend carve, and it should run on
every server.py slice from here.
It also flagged a PRE-EXISTING one I deliberately did not touch: server.py's
TuningProviderRegistry.get_merged() calls `logger.exception(...)` in an except handler and
there is no `logger` in the module (it is `log`). So a raising tuning provider takes down
the merged-tunings call for everyone, with a NameError naming the wrong problem. Filed as
issue #899 rather than smuggled into a carve whose whole value is being behaviour-neutral.
The constants lost their underscore prefix: they cross a module boundary now (the seed
tests read them), so `_BUILTIN_STARTER_SOURCES` was a lie.
pytest 2397, pyflakes 0, Codex 0. Guarded by the plugin_context contract test (#898).
Refs #48
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughA new ChangesBuilt-in content seeding
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/builtin_content.py (1)
1-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim the historical and implementation-heavy docstrings.
Keep the module responsibility and
server_rootinvariant; move migration history and algorithm details to comments or an ADR.As per coding guidelines,
*.py: “keep docstrings minimal.”Also applies to: 64-82, 186-193, 261-268, 314-320
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/builtin_content.py` around lines 1 - 24, Trim the module and referenced function docstrings in builtin_content.py to concise descriptions of their responsibilities, retaining only the server_root parameter/invariant where relevant. Remove migration history, rationale, and algorithm-level implementation details from docstrings; preserve that context only as comments or an ADR if needed, including the sections around the affected seed helpers.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/builtin_content.py`:
- Around line 84-106: Update the seeding flow around the destination-directory
setup and starter marker write to anchor the DLC/config root with an O_NOFOLLOW
directory fd before opening any child paths. Resolve dest_dir, pack files, and
the marker relative to that anchored root using dir_fd-based operations,
preserving the existing fallback only on platforms without the required support;
do not retain path-based operations that allow ancestor symlink swaps.
---
Nitpick comments:
In `@lib/builtin_content.py`:
- Around line 1-24: Trim the module and referenced function docstrings in
builtin_content.py to concise descriptions of their responsibilities, retaining
only the server_root parameter/invariant where relevant. Remove migration
history, rationale, and algorithm-level implementation details from docstrings;
preserve that context only as comments or an ADR if needed, including the
sections around the affected seed helpers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a7900d4e-bd27-41ec-979d-6e91a3dbeeae
📒 Files selected for processing (5)
lib/builtin_content.pyserver.pytests/test_builtin_diagnostic_seed.pytests/test_builtin_starter_seed.pytests/test_progression_api.py
Comment on lines
+84
to
+106
| # Refuse a symlinked seed directory: mkdir(exist_ok=True) would accept it | ||
| # and copies would land at the link target, outside the DLC tree. The | ||
| # per-file symlink guard below cannot catch this. | ||
| if dest_dir.is_symlink(): | ||
| log.warning("%s: %s is a symlink, skipping all seeding", label, dest_dir.name) | ||
| return 0 | ||
| dest_dir.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| # Pin the seed directory by an O_NOFOLLOW fd so a symlink swapped in for | ||
| # dest_dir *after* the check above cannot redirect the per-file stat / | ||
| # temp-create / replace outside the DLC tree (parent-directory TOCTOU). | ||
| # os.replace accepts dir_fd on POSIX even though it isn't listed in | ||
| # os.supports_dir_fd, so gate on os.rename (the reliable proxy); platforms | ||
| # without dir_fd/O_NOFOLLOW (e.g. Windows) fall back to path-based ops. | ||
| dir_fd = None | ||
| if ( | ||
| hasattr(os, "O_NOFOLLOW") | ||
| and hasattr(os, "O_DIRECTORY") | ||
| and os.open in os.supports_dir_fd | ||
| and os.rename in os.supports_dir_fd | ||
| ): | ||
| try: | ||
| dir_fd = os.open(dest_dir, os.O_RDONLY | os.O_NOFOLLOW | os.O_DIRECTORY) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import os
import shutil
import tempfile
from pathlib import Path
if not all(hasattr(os, name) for name in ("O_NOFOLLOW", "O_DIRECTORY")):
raise SystemExit("Platform lacks required flags")
with tempfile.TemporaryDirectory() as td:
root = Path(td)
dlc = root / "dlc"
seed = dlc / "diagnostics-builtin"
seed.mkdir(parents=True)
outside = root / "outside"
outside_seed = outside / "diagnostics-builtin"
outside_seed.mkdir(parents=True)
assert not seed.is_symlink()
shutil.rmtree(dlc)
dlc.symlink_to(outside, target_is_directory=True)
fd = os.open(seed, os.O_RDONLY | os.O_NOFOLLOW | os.O_DIRECTORY)
try:
assert os.path.samestat(os.fstat(fd), os.stat(outside_seed))
print("O_NOFOLLOW followed the swapped intermediate component")
finally:
os.close(fd)
PYRepository: got-feedBack/feedBack
Length of output: 214
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file structure first.
ast-grep outline lib/builtin_content.py --view expanded || true
# Show the relevant ranges with line numbers.
wc -l lib/builtin_content.py
sed -n '1,220p' lib/builtin_content.py | cat -n
printf '\n---\n'
sed -n '320,390p' lib/builtin_content.py | cat -nRepository: got-feedBack/feedBack
Length of output: 14897
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the starter-content region around the second reported location.
sed -n '313,378p' lib/builtin_content.py | cat -nRepository: got-feedBack/feedBack
Length of output: 3807
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '313,378p' lib/builtin_content.py | cat -nRepository: got-feedBack/feedBack
Length of output: 3807
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the rest of the copy helper and any other symlink-sensitive writes.
sed -n '180,260p' lib/builtin_content.py | cat -n
printf '\n---\n'
rg -n "O_NOFOLLOW|dir_fd|os\.open\(|os\.replace\(|os\.lstat\(" lib/builtin_content.pyRepository: got-feedBack/feedBack
Length of output: 5826
Anchor the root fd before seeding child paths.
os.open(dest_dir, O_NOFOLLOW | O_DIRECTORY)still follows symlinks in ancestor components, so swappingdlc/CONFIG_DIRbefore the open can bind the fd to the wrong tree.- The starter marker write later in the file has the same path-based TOCTOU. Open the DLC/config root once and resolve
dest_dir, pack files, and the marker relative to it; otherwise this fallback isn’t symlink-safe.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/builtin_content.py` around lines 84 - 106, Update the seeding flow around
the destination-directory setup and starter marker write to anchor the
DLC/config root with an O_NOFOLLOW directory fd before opening any child paths.
Resolve dest_dir, pack files, and the marker relative to that anchored root
using dir_fd-based operations, preserving the existing fallback only on
platforms without the required support; do not retain path-based operations that
allow ancestor symlink swaps.
byrongamatos
added a commit
that referenced
this pull request
Jul 11, 2026
lib/scan.py (326). server.py 2,098 -> 1,870.
The background scan, its spawn ProcessPoolExecutor, and the kick/runner plumbing that
serialises passes. Bodies VERBATIM except the seam reads.
Everything shared is read LATE off appstate — the same contract every module in
lib/routers/ uses, and it is not cosmetic: tests monkeypatch CONFIG_DIR and swap meta_db,
so a value captured at import time pins the wrong one for the life of the process.
CONFIG_DIR -> appstate.config_dir
meta_db -> appstate.meta_db
_default_settings -> appstate.default_settings()
_stat_for_cache -> appstate.stat_for_cache()
━━━ THE SCAN STATUS IS REBOUND, NOT MUTATED ━━━
_background_scan does `global _scan_status; _scan_status = {**INIT, ...}` at every stage
transition. It REPLACES the dict; it never updates it in place. So nothing may hold that
dict by value — a reference captured once goes permanently stale at the first stage change
and would report "listing" forever while the scan ran to completion.
Hence `scan.status()`, a getter, and hence appstate publishes scan_status as a CALLABLE.
appstate.py already said so in a comment; this is the code that makes it true. (Same for
the plugin_context entry, which was already `lambda: dict(_scan_status)` — late-bound, so
it survives the move unchanged. The contract test from #898 covers it.)
━━━ appstate.server_root: A TRAP CLOSED PERMANENTLY ━━━
_background_scan seeds the builtin content, which needs the directory holding server.py.
`Path(__file__).resolve().parent` is correct in server.py and silently WRONG anywhere under
lib/ — it yields lib/, which holds no docs/ or data/ — and it fails by finding NOTHING
rather than by raising, so the seeds would just quietly never run.
lib/builtin_content.py (#900) closed that by taking the root as a parameter. This adds the
other half: server.py publishes it ONCE as appstate.server_root, so no module under lib/
ever has a reason to derive it. Documented at the slot.
pyflakes caught two more missing imports on the way in (loosefolder_mod, enrichment) —
each a NameError on a live scan path, and the suite would have handed them over one failure
at a time. It stays part of every server.py slice.
TESTS. The two scan fixtures (test_settings_api::scan_module,
test_feedpak_extension::scan_server) patched server._make_scan_executor to swap the spawn
pool for an in-process ThreadPool; they now patch it on lib/scan.py. Worth noting WHY that
still works: the fixtures re-import `server` per test, but `scan` stays cached in
sys.modules — and it picks up the fresh CONFIG_DIR anyway, because the appstate reads are
late-bound. The seam is doing exactly the job it was built for.
pytest 2398, pyflakes 0, Codex 0.
Refs #48
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
━━━ TEST ISOLATION: A REGRESSION THE CARVE ITSELF CREATED (Codex [P2]) ━━━
background_scan() deliberately NEVER sets running=False — ownership of that flag lives in
_scan_runner, so a kick_scan() racing the terminal write cannot see a stale False and start
a second runner. Correct in production.
But the scan fixtures call background_scan() DIRECTLY, skipping the runner. That was
harmless while the state lived on , which the fixtures RE-IMPORT per test. It is not
harmless now: stays cached in sys.modules across sys.modules.pop("server"), so the
status dict OUTLIVES the test. One direct call leaves the shared scanner marked "running"
forever, and every later scan or rescan returns "already in progress" and quietly does
nothing.
Verified: after a direct call, kick_scan() returns False and starts no scan.
The suite passed anyway, on ordering luck. tests/conftest.py::reset_scan_state now snapshots
and restores lib/scan.py's module state around the two fixtures that drive it directly.
pytest 2398, pyflakes 0, Codex 0.
byrongamatos
added a commit
that referenced
this pull request
Jul 11, 2026
lib/scan.py (326). server.py 2,098 -> 1,870.
The background scan, its spawn ProcessPoolExecutor, and the kick/runner plumbing that
serialises passes. Bodies VERBATIM except the seam reads.
Everything shared is read LATE off appstate — the same contract every module in
lib/routers/ uses, and it is not cosmetic: tests monkeypatch CONFIG_DIR and swap meta_db,
so a value captured at import time pins the wrong one for the life of the process.
CONFIG_DIR -> appstate.config_dir
meta_db -> appstate.meta_db
_default_settings -> appstate.default_settings()
_stat_for_cache -> appstate.stat_for_cache()
━━━ THE SCAN STATUS IS REBOUND, NOT MUTATED ━━━
_background_scan does `global _scan_status; _scan_status = {**INIT, ...}` at every stage
transition. It REPLACES the dict; it never updates it in place. So nothing may hold that
dict by value — a reference captured once goes permanently stale at the first stage change
and would report "listing" forever while the scan ran to completion.
Hence `scan.status()`, a getter, and hence appstate publishes scan_status as a CALLABLE.
appstate.py already said so in a comment; this is the code that makes it true. (Same for
the plugin_context entry, which was already `lambda: dict(_scan_status)` — late-bound, so
it survives the move unchanged. The contract test from #898 covers it.)
━━━ appstate.server_root: A TRAP CLOSED PERMANENTLY ━━━
_background_scan seeds the builtin content, which needs the directory holding server.py.
`Path(__file__).resolve().parent` is correct in server.py and silently WRONG anywhere under
lib/ — it yields lib/, which holds no docs/ or data/ — and it fails by finding NOTHING
rather than by raising, so the seeds would just quietly never run.
lib/builtin_content.py (#900) closed that by taking the root as a parameter. This adds the
other half: server.py publishes it ONCE as appstate.server_root, so no module under lib/
ever has a reason to derive it. Documented at the slot.
pyflakes caught two more missing imports on the way in (loosefolder_mod, enrichment) —
each a NameError on a live scan path, and the suite would have handed them over one failure
at a time. It stays part of every server.py slice.
TESTS. The two scan fixtures (test_settings_api::scan_module,
test_feedpak_extension::scan_server) patched server._make_scan_executor to swap the spawn
pool for an in-process ThreadPool; they now patch it on lib/scan.py. Worth noting WHY that
still works: the fixtures re-import `server` per test, but `scan` stays cached in
sys.modules — and it picks up the fresh CONFIG_DIR anyway, because the appstate reads are
late-bound. The seam is doing exactly the job it was built for.
━━━ TEST ISOLATION: A REGRESSION THE CARVE ITSELF CREATED (Codex [P2]) ━━━
background_scan() deliberately NEVER sets running=False — ownership of that flag lives in
_scan_runner, so a kick_scan() racing the terminal write cannot observe a stale False and
start a second runner. Correct in production.
But the scan fixtures call background_scan() DIRECTLY, skipping the runner. That was
harmless while the state lived on `server`, which the fixtures RE-IMPORT per test. It is
NOT harmless now: `scan` stays cached in sys.modules across sys.modules.pop("server"), so
the status dict OUTLIVES the test. One direct call leaves the shared scanner marked
"running" forever, and every later scan or rescan returns "already in progress" and quietly
does nothing.
Verified: after a direct call, kick_scan() returns False and starts no scan at all.
The suite passed anyway, on ordering luck — which is exactly how this class of bug ships.
tests/conftest.py::reset_scan_state now snapshots and restores lib/scan.py's module state
around the two fixtures that drive it directly.
pytest 2398, pyflakes 0, Codex 0.
Refs #48
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
added a commit
that referenced
this pull request
Jul 11, 2026
lib/scan.py (326). server.py 2,098 -> 1,870.
The background scan, its spawn ProcessPoolExecutor, and the kick/runner plumbing that
serialises passes. Bodies VERBATIM except the seam reads.
Everything shared is read LATE off appstate — the same contract every module in
lib/routers/ uses, and it is not cosmetic: tests monkeypatch CONFIG_DIR and swap meta_db,
so a value captured at import time pins the wrong one for the life of the process.
CONFIG_DIR -> appstate.config_dir
meta_db -> appstate.meta_db
_default_settings -> appstate.default_settings()
_stat_for_cache -> appstate.stat_for_cache()
━━━ THE SCAN STATUS IS REBOUND, NOT MUTATED ━━━
_background_scan does `global _scan_status; _scan_status = {**INIT, ...}` at every stage
transition. It REPLACES the dict; it never updates it in place. So nothing may hold that
dict by value — a reference captured once goes permanently stale at the first stage change
and would report "listing" forever while the scan ran to completion.
Hence `scan.status()`, a getter, and hence appstate publishes scan_status as a CALLABLE.
appstate.py already said so in a comment; this is the code that makes it true. (Same for
the plugin_context entry, which was already `lambda: dict(_scan_status)` — late-bound, so
it survives the move unchanged. The contract test from #898 covers it.)
━━━ appstate.server_root: A TRAP CLOSED PERMANENTLY ━━━
_background_scan seeds the builtin content, which needs the directory holding server.py.
`Path(__file__).resolve().parent` is correct in server.py and silently WRONG anywhere under
lib/ — it yields lib/, which holds no docs/ or data/ — and it fails by finding NOTHING
rather than by raising, so the seeds would just quietly never run.
lib/builtin_content.py (#900) closed that by taking the root as a parameter. This adds the
other half: server.py publishes it ONCE as appstate.server_root, so no module under lib/
ever has a reason to derive it. Documented at the slot.
pyflakes caught two more missing imports on the way in (loosefolder_mod, enrichment) —
each a NameError on a live scan path, and the suite would have handed them over one failure
at a time. It stays part of every server.py slice.
TESTS. The two scan fixtures (test_settings_api::scan_module,
test_feedpak_extension::scan_server) patched server._make_scan_executor to swap the spawn
pool for an in-process ThreadPool; they now patch it on lib/scan.py. Worth noting WHY that
still works: the fixtures re-import `server` per test, but `scan` stays cached in
sys.modules — and it picks up the fresh CONFIG_DIR anyway, because the appstate reads are
late-bound. The seam is doing exactly the job it was built for.
━━━ TEST ISOLATION: A REGRESSION THE CARVE ITSELF CREATED (Codex [P2]) ━━━
background_scan() deliberately NEVER sets running=False — ownership of that flag lives in
_scan_runner, so a kick_scan() racing the terminal write cannot observe a stale False and
start a second runner. Correct in production.
But the scan fixtures call background_scan() DIRECTLY, skipping the runner. That was
harmless while the state lived on `server`, which the fixtures RE-IMPORT per test. It is
NOT harmless now: `scan` stays cached in sys.modules across sys.modules.pop("server"), so
the status dict OUTLIVES the test. One direct call leaves the shared scanner marked
"running" forever, and every later scan or rescan returns "already in progress" and quietly
does nothing.
Verified: after a direct call, kick_scan() returns False and starts no scan at all.
The suite passed anyway, on ordering luck — which is exactly how this class of bug ships.
tests/conftest.py::reset_scan_state now snapshots and restores lib/scan.py's module state
around the two fixtures that drive it directly.
pytest 2398, pyflakes 0, Codex 0.
Refs #48
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
added a commit
that referenced
this pull request
Jul 11, 2026
) lib/scan.py (326). server.py 2,098 -> 1,870. The background scan, its spawn ProcessPoolExecutor, and the kick/runner plumbing that serialises passes. Bodies VERBATIM except the seam reads. Everything shared is read LATE off appstate — the same contract every module in lib/routers/ uses, and it is not cosmetic: tests monkeypatch CONFIG_DIR and swap meta_db, so a value captured at import time pins the wrong one for the life of the process. CONFIG_DIR -> appstate.config_dir meta_db -> appstate.meta_db _default_settings -> appstate.default_settings() _stat_for_cache -> appstate.stat_for_cache() ━━━ THE SCAN STATUS IS REBOUND, NOT MUTATED ━━━ _background_scan does `global _scan_status; _scan_status = {**INIT, ...}` at every stage transition. It REPLACES the dict; it never updates it in place. So nothing may hold that dict by value — a reference captured once goes permanently stale at the first stage change and would report "listing" forever while the scan ran to completion. Hence `scan.status()`, a getter, and hence appstate publishes scan_status as a CALLABLE. appstate.py already said so in a comment; this is the code that makes it true. (Same for the plugin_context entry, which was already `lambda: dict(_scan_status)` — late-bound, so it survives the move unchanged. The contract test from #898 covers it.) ━━━ appstate.server_root: A TRAP CLOSED PERMANENTLY ━━━ _background_scan seeds the builtin content, which needs the directory holding server.py. `Path(__file__).resolve().parent` is correct in server.py and silently WRONG anywhere under lib/ — it yields lib/, which holds no docs/ or data/ — and it fails by finding NOTHING rather than by raising, so the seeds would just quietly never run. lib/builtin_content.py (#900) closed that by taking the root as a parameter. This adds the other half: server.py publishes it ONCE as appstate.server_root, so no module under lib/ ever has a reason to derive it. Documented at the slot. pyflakes caught two more missing imports on the way in (loosefolder_mod, enrichment) — each a NameError on a live scan path, and the suite would have handed them over one failure at a time. It stays part of every server.py slice. TESTS. The two scan fixtures (test_settings_api::scan_module, test_feedpak_extension::scan_server) patched server._make_scan_executor to swap the spawn pool for an in-process ThreadPool; they now patch it on lib/scan.py. Worth noting WHY that still works: the fixtures re-import `server` per test, but `scan` stays cached in sys.modules — and it picks up the fresh CONFIG_DIR anyway, because the appstate reads are late-bound. The seam is doing exactly the job it was built for. ━━━ TEST ISOLATION: A REGRESSION THE CARVE ITSELF CREATED (Codex [P2]) ━━━ background_scan() deliberately NEVER sets running=False — ownership of that flag lives in _scan_runner, so a kick_scan() racing the terminal write cannot observe a stale False and start a second runner. Correct in production. But the scan fixtures call background_scan() DIRECTLY, skipping the runner. That was harmless while the state lived on `server`, which the fixtures RE-IMPORT per test. It is NOT harmless now: `scan` stays cached in sys.modules across sys.modules.pop("server"), so the status dict OUTLIVES the test. One direct call leaves the shared scanner marked "running" forever, and every later scan or rescan returns "already in progress" and quietly does nothing. Verified: after a direct call, kick_scan() returns False and starts no scan at all. The suite passed anyway, on ordering luck — which is exactly how this class of bug ships. tests/conftest.py::reset_scan_state now snapshots and restores lib/scan.py's module state around the two fixtures that drive it directly. pytest 2398, pyflakes 0, Codex 0. Refs #48 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Refs #48.
lib/builtin_content.py— 321 lines moved.server.py2,418 → 2,098.The calibration/diagnostic sloppaks and the starter library:
_copy_builtin_packs,_write_builtin_pack, the two seed helpers, their source tables, and the seed marker.The one signature change — and why the carve is unsafe without it
server.pyhas:That is correct in server.py: the repo root in dev,
resources/feedBackwhen bundled — the tree that actually holdsdocs/anddata/.Move that body into
lib/unchanged and it keeps working, silently, and returnslib/. There is nodocs/diagnosticsunderlib/, so every seed would find nothing, log "source missing" at debug level, and return. Nothing raises. Nothing fails. The starter library simply never appears, and the calibration sloppak is never seeded — on a fresh install, in the field.A verbatim move whose meaning changed because
__file__did.So this module cannot compute a root:
server_rootis a parameter, and server.py — the only place that legitimately knows where it lives — passes it in. The trap is now structurally impossible rather than merely avoided. (_copy_builtin_packsalready took the root that way; the two seed helpers now do too.)Everything else is byte-identical.
CONFIG_DIRis read late asappstate.config_dir, and the DLC root throughdlc_paths._get_dlc_dir— the same seam every router inlib/routers/uses, late-bound because tests monkeypatch it.pyflakes found three missing imports that the tests would have surfaced one at a time
The moved code uses
secrets,statandtempfile. None was in my import block. Each is aNameErroron a live path, and the test suite would have handed them to me one failure at a time.python3 -m pyflakesnames all three in one shot.This is the Python twin of the
no-undefgate that guarded every frontend carve, and it should run on everyserver.pyslice from here.It also flagged a pre-existing one I deliberately did not touch:
TuningProviderRegistry.get_merged()callslogger.exception(...)in anexcepthandler, and there is nologgerin the module (it'slog). So a raising tuning provider takes down the merged-tunings call for everyone, with aNameErrornaming the wrong problem. Filed as #899 rather than smuggled into a carve whose whole value is being behaviour-neutral.Notes
The constants lost their underscore prefix — they cross a module boundary now (the seed tests read them), so
_BUILTIN_STARTER_SOURCESwas a lie.Guarded by the
plugin_contextcontract test from #898.pytest 2397 · pyflakes 0 · Codex 0.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes