refactor(server): carve the library scanner into lib/scan.py (R3b) - #901
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR moves background scanning into ChangesBuiltin Content and Scan Refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Server
participant scan
participant ProcessPoolExecutor
participant meta_db
participant enrichment
Server->>scan: kick_scan()
scan->>ProcessPoolExecutor: dispatch _scan_one
ProcessPoolExecutor->>meta_db: write metadata results
scan->>enrichment: trigger _kick_enrich()
Server->>scan: request status()
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
🧹 Nitpick comments (1)
lib/builtin_content.py (1)
1-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocstrings here exceed "keep docstrings minimal."
The module docstring (Line 1-25) and several function docstrings (e.g. Line 64-83, Line 186-194) are multi-paragraph design essays. As per coding guidelines,
*.pyfiles should "keep docstrings minimal." Given the security-sensitive rationale being documented, consider moving some of this narrative into a comment/ADR rather than the docstring, though the content itself is valuable.🤖 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 - 25, Reduce the module docstring and the related function docstrings in builtin_content.py, including the seed helpers, to concise descriptions of their purpose and parameters. Move the detailed server_root rationale and security-sensitive design narrative into regular comments or an ADR while preserving the necessary implementation guidance.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.
Nitpick comments:
In `@lib/builtin_content.py`:
- Around line 1-25: Reduce the module docstring and the related function
docstrings in builtin_content.py, including the seed helpers, to concise
descriptions of their purpose and parameters. Move the detailed server_root
rationale and security-sensitive design narrative into regular comments or an
ADR while preserving the necessary implementation guidance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 734639e3-8dcb-486d-9d53-492280225e4d
📒 Files selected for processing (9)
lib/appstate.pylib/builtin_content.pylib/scan.pyserver.pytests/test_builtin_diagnostic_seed.pytests/test_builtin_starter_seed.pytests/test_feedpak_extension.pytests/test_progression_api.pytests/test_settings_api.py
byrongamatos
force-pushed
the
r3b/scan
branch
2 times, most recently
from
July 11, 2026 23:04
85a51be to
a3061fe
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/conftest.py`:
- Around line 95-109: Update the reset_scan_state fixture to reset all scanner
bookkeeping fields, including _scan_thread and _scan_rescan_pending, before
yielding. During teardown, join any active scan thread before restoring the
saved _scan_status, _scan_thread, and _scan_rescan_pending values, ensuring no
scan remains running between tests.
🪄 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: 2ad9d37a-ef89-4b38-9c86-cdc75bfe8a78
📒 Files selected for processing (6)
lib/appstate.pylib/scan.pyserver.pytests/conftest.pytests/test_feedpak_extension.pytests/test_settings_api.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/test_feedpak_extension.py
- lib/appstate.py
- tests/test_settings_api.py
- server.py
Comment on lines
+95
to
+109
| @pytest.fixture() | ||
| def reset_scan_state(): | ||
| """Restore lib/scan.py's module-level state around a test that drives it directly.""" | ||
| import scan | ||
|
|
||
| saved_status = scan._scan_status | ||
| saved_thread = scan._scan_thread | ||
| saved_pending = scan._scan_rescan_pending | ||
| scan._scan_status = dict(scan._SCAN_STATUS_INIT) | ||
| try: | ||
| yield scan | ||
| finally: | ||
| scan._scan_status = saved_status | ||
| scan._scan_thread = saved_thread | ||
| scan._scan_rescan_pending = saved_pending |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C4 '\b_scan_thread\b|\b_scan_rescan_pending\b|def (background_scan|kick_scan|_scan_runner)' lib/scan.pyRepository: got-feedBack/feedBack
Length of output: 3052
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the fixture and any scan-thread teardown helpers.
rg -n -C4 'reset_scan_state|scan_thread\(|_join_background_db_threads|join\(|_scan_rescan_pending|_scan_thread' tests lib/scan.pyRepository: got-feedBack/feedBack
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the fixture and the scanner's shutdown helpers with line numbers.
sed -n '80,120p' tests/conftest.py
printf '\n----\n'
sed -n '1,220p' lib/scan.py | sed -n '1,120p'
printf '\n----\n'
sed -n '220,360p' lib/scan.pyRepository: got-feedBack/feedBack
Length of output: 11507
Reset all scanner bookkeeping and join the active scan thread in this fixture. reset_scan_state only snapshots _scan_status, _scan_thread, and _scan_rescan_pending, but it only resets _scan_status before yield. That leaves stale in-progress state for later tests, and a scan started here can keep running after teardown.
🤖 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 `@tests/conftest.py` around lines 95 - 109, Update the reset_scan_state fixture
to reset all scanner bookkeeping fields, including _scan_thread and
_scan_rescan_pending, before yielding. During teardown, join any active scan
thread before restoring the saved _scan_status, _scan_thread, and
_scan_rescan_pending values, ensuring no scan remains running between tests.
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 was referenced Jul 15, 2026
Closed
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/scan.py(326).server.py2,098 → 1,870.The background scan, its
spawnProcessPoolExecutor, 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 inlib/routers/uses, and it isn't cosmetic: tests monkeypatchCONFIG_DIRand swapmeta_db, so a value captured at import time pins the wrong one for the life of the process.CONFIG_DIRappstate.config_dirmeta_dbappstate.meta_db_default_settingsappstate.default_settings()_stat_for_cacheappstate.stat_for_cache()The scan status is rebound, not mutated
_background_scandoesglobal _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 henceappstatepublishesscan_statusas a callable.appstate.pyalready said so in a comment; this is the code that makes it true.(Same for the
plugin_contextentry, which was alreadylambda: 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_scanseeds the builtin content, which needs the directory holdingserver.py.Path(__file__).resolve().parentis correct in server.py and silently wrong anywhere underlib/— it yieldslib/, which holds nodocs/ordata/— and it fails by finding nothing rather than by raising, so the seeds would just quietly never run.#900 closed that by making
lib/builtin_content.pytake the root as a parameter. This adds the other half: server.py publishes it once asappstate.server_root, so no module underlib/ever has a reason to derive it. Documented at the slot.pyflakes, again
Two more missing imports caught on the way in (
loosefolder_mod,enrichment) — each aNameErroron 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) patchedserver._make_scan_executorto swap the spawn pool for an in-process ThreadPool; they now patch it onlib/scan.py.Worth noting why that still works: the fixtures re-import
serverper test, butscanstays cached insys.modules— and it picks up the freshCONFIG_DIRanyway, because the appstate reads are late-bound. The seam is doing exactly the job it was built for.pytest 2398 · pyflakes 0 · Codex 0.
Codex caught a test-isolation regression the carve itself created
background_scan()deliberately never setsrunning = False— ownership of that flag lives in_scan_runner, so akick_scan()racing the terminal write cannot observe a staleFalseand 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:scanstays cached insys.modulesacrosssys.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:
The suite passed anyway, on ordering luck — which is exactly how this class of bug ships.
tests/conftest.py::reset_scan_statenow snapshots and restores lib/scan.py's module state around the two fixtures that drive it directly.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes