test(plugins): pin the plugin_context contract before carving server.py (R3b) - #898
Merged
Merged
Conversation
…py (R3b) tests/test_plugin_context_contract.py (3 tests). No production code changes. server.py is about to be carved apart around startup_events(), and `plugin_context` — the 20-key dict handed to every plugin's setup() — is built inline inside it. Issue #48 flagged this while planning the split and asked for exactly this guard: "Plugin context[...] are passed as live references into already-loaded plugins. Refactoring must preserve the exact callables — moving them to a new module is fine, but renaming or wrapping them breaks third-party plugins. We'd want a 'plugin context unchanged' assertion in CI." It never got written. Writing it FIRST, because a key silently dropped or renamed by a move is invisible to every other test in the suite — nothing in-tree reads most of these — and would break plugins at runtime, in the field. This is the backend's version of the window contract, and the frontend carve just taught me what that costs: 43 of library.js's exports were referenced ONLY from app.js's top-level window block, invisible to any call-graph scan, and trusting the scan would have shipped a dead A-Z rail with CI fully green. A contract only external code reads has to be pinned BY NAME, before the move, not after. THE SURFACE IS BIGGER THAN server.py's DICT. Shipped plugins read `log` and `load_sibling`, and neither is in it — plugins/__init__.py layers them on per-plugin. A test pinning only server.py's 18 keys would have missed both. ━━━ CODEX CAUGHT ME WRITING A VACUOUS ASSERTION ━━━ My first identity test built a dict locally and called setup() on it — asserting `dict(x)['k'] is x['k']`, which is trivially true and blind to everything the loader does. [P2], and correct. It now drives the REAL plugins.load_plugins() with a probe plugin, which matters: the loader DOES deliberately wrap one key (register_library_provider is scoped per-plugin so a plugin cannot forge owner attribution and impersonate another). The test pins that single intentional exception so it cannot quietly become two. Codex then caught [P2] number two: my hand-rolled teardown restored only PLUGINS_DIR and LOADED_PLUGINS, while load_plugins() also mutates sys.path, sys.modules and PENDING_PLUGINS — order- and environment-dependent. tests/test_plugins.py already had a fixture that does this properly, so `reset_plugin_state` moved to tests/conftest.py: ONE copy, shared, rather than a second that will drift. BITE-TESTED IN FIVE DIRECTIONS — drop a key, rename a key, drop a per-plugin key, wrap extract_meta in the loader (all key names intact, identity broken), and remove the register_library_provider scoping (the impersonation guard). Each fails. pytest 2399, Codex 0. Refs #48 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 (3)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughChangesPlugin context contract
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
byrongamatos
added a commit
that referenced
this pull request
Jul 11, 2026
…ent.py (R3b) (#900) 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>
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/demo_mode.py (342). server.py 1,870 -> 1,649.
The read-only request guard (its 96-entry blocked-route table + the middleware) and the
hourly session janitor (registry, hook runner, thread). Bodies VERBATIM.
THE MIDDLEWARE NEEDS `app`, SO THE MODULE TAKES IT. _demo_mode_guard is an
@app.middleware("http") and cannot exist without an app object. Rather than have a module
under lib/ reach for a global, it exposes install(app) and server.py — which owns the app —
hands it over. The janitor is symmetrical: start_janitor() / stop_janitor(), called from
server.py's startup and shutdown hooks, where the process lifecycle actually lives.
register_demo_janitor_hook IS PART OF THE PLUGIN CONTRACT. It is a key in plugin_context,
so plugins hold it as a LIVE REFERENCE from setup(). server.py imports this exact object
and puts it in the dict unchanged — identity preserved, and
tests/test_plugin_context_contract.py (#898, merged) fails if that ever stops being true.
This is the first carve that guard has actually protected.
━━━ stop_janitor()'s ORDER IS LOAD-BEARING ━━━
The obvious way to write it — clear the "started" flag, then join — is WRONG, and I wrote
it that way first. server.py's original deliberately returns EARLY, leaving
_DEMO_JANITOR_STARTED True and the thread handle intact, when the thread outlives the join:
# Leave _DEMO_JANITOR_STARTED True so a new janitor is not
# spawned by a subsequent startup while the old one is alive.
Clearing the flag first quietly reintroduces exactly the double-janitor leak the flag
exists to prevent. Preserved byte-for-byte, and the reason is now written down at the
function rather than only at its single call site.
━━━ A BUG MOVED VERBATIM, ON PURPOSE ━━━
if getenv_compat("FEEDBACK_DEMO_MODE") or getenv_compat("FEEDBACK_DEMO_MODE") == "1" \
and not _DEMO_JANITOR_STARTED:
`and` binds tighter than `or`, so this is `A or (B and C)` — the not-already-started
re-entry guard is DEAD whenever the env var is truthy, which is the only case that runs.
A second startup leaks a janitor thread (the handle is overwritten, so shutdown joins only
the last). Verified. Preserved exactly and filed as issue #902: a carve whose whole value
is being provably behaviour-neutral is not the place to change behaviour.
pyflakes caught three more missing imports on the way in (uuid, warnings x2). Five carves,
ten missing imports, every one a NameError on a live path.
pytest 2399, 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/demo_mode.py (342). server.py 1,870 -> 1,649.
The read-only request guard (its 96-entry blocked-route table + the middleware) and the
hourly session janitor (registry, hook runner, thread). Bodies VERBATIM.
THE MIDDLEWARE NEEDS `app`, SO THE MODULE TAKES IT. _demo_mode_guard is an
@app.middleware("http") and cannot exist without an app object. Rather than have a module
under lib/ reach for a global, it exposes install(app) and server.py — which owns the app —
hands it over. The janitor is symmetrical: start_janitor() / stop_janitor(), called from
server.py's startup and shutdown hooks, where the process lifecycle actually lives.
register_demo_janitor_hook IS PART OF THE PLUGIN CONTRACT. It is a key in plugin_context,
so plugins hold it as a LIVE REFERENCE from setup(). server.py imports this exact object
and puts it in the dict unchanged — identity preserved, and
tests/test_plugin_context_contract.py (#898, merged) fails if that ever stops being true.
This is the first carve that guard has actually protected.
━━━ stop_janitor()'s ORDER IS LOAD-BEARING ━━━
The obvious way to write it — clear the "started" flag, then join — is WRONG, and I wrote
it that way first. server.py's original deliberately returns EARLY, leaving
_DEMO_JANITOR_STARTED True and the thread handle intact, when the thread outlives the join:
# Leave _DEMO_JANITOR_STARTED True so a new janitor is not
# spawned by a subsequent startup while the old one is alive.
Clearing the flag first quietly reintroduces exactly the double-janitor leak the flag
exists to prevent. Preserved byte-for-byte, and the reason is now written down at the
function rather than only at its single call site.
━━━ A BUG MOVED VERBATIM, ON PURPOSE ━━━
if getenv_compat("FEEDBACK_DEMO_MODE") or getenv_compat("FEEDBACK_DEMO_MODE") == "1" \
and not _DEMO_JANITOR_STARTED:
`and` binds tighter than `or`, so this is `A or (B and C)` — the not-already-started
re-entry guard is DEAD whenever the env var is truthy, which is the only case that runs.
A second startup leaks a janitor thread (the handle is overwritten, so shutdown joins only
the last). Verified. Preserved exactly and filed as issue #902: a carve whose whole value
is being provably behaviour-neutral is not the place to change behaviour.
pyflakes caught three more missing imports on the way in (uuid, warnings x2). Five carves,
ten missing imports, every one a NameError on a live path.
pytest 2399, 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/demo_mode.py (342). server.py 1,870 -> 1,649.
The read-only request guard (its 96-entry blocked-route table + the middleware) and the
hourly session janitor (registry, hook runner, thread). Bodies VERBATIM.
THE MIDDLEWARE NEEDS `app`, SO THE MODULE TAKES IT. _demo_mode_guard is an
@app.middleware("http") and cannot exist without an app object. Rather than have a module
under lib/ reach for a global, it exposes install(app) and server.py — which owns the app —
hands it over. The janitor is symmetrical: start_janitor() / stop_janitor(), called from
server.py's startup and shutdown hooks, where the process lifecycle actually lives.
register_demo_janitor_hook IS PART OF THE PLUGIN CONTRACT. It is a key in plugin_context,
so plugins hold it as a LIVE REFERENCE from setup(). server.py imports this exact object
and puts it in the dict unchanged — identity preserved, and
tests/test_plugin_context_contract.py (#898, merged) fails if that ever stops being true.
This is the first carve that guard has actually protected.
━━━ stop_janitor()'s ORDER IS LOAD-BEARING ━━━
The obvious way to write it — clear the "started" flag, then join — is WRONG, and I wrote
it that way first. server.py's original deliberately returns EARLY, leaving
_DEMO_JANITOR_STARTED True and the thread handle intact, when the thread outlives the join:
# Leave _DEMO_JANITOR_STARTED True so a new janitor is not
# spawned by a subsequent startup while the old one is alive.
Clearing the flag first quietly reintroduces exactly the double-janitor leak the flag
exists to prevent. Preserved byte-for-byte, and the reason is now written down at the
function rather than only at its single call site.
━━━ A BUG MOVED VERBATIM, ON PURPOSE ━━━
if getenv_compat("FEEDBACK_DEMO_MODE") or getenv_compat("FEEDBACK_DEMO_MODE") == "1" \
and not _DEMO_JANITOR_STARTED:
`and` binds tighter than `or`, so this is `A or (B and C)` — the not-already-started
re-entry guard is DEAD whenever the env var is truthy, which is the only case that runs.
A second startup leaks a janitor thread (the handle is overwritten, so shutdown joins only
the last). Verified. Preserved exactly and filed as issue #902: a carve whose whole value
is being provably behaviour-neutral is not the place to change behaviour.
pyflakes caught three more missing imports on the way in (uuid, warnings x2). Five carves,
ten missing imports, every one a NameError on a live path.
pytest 2399, 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. No production code changes — this is a guard, shipped before the thing it guards gets moved.
server.pyis about to be carved apart aroundstartup_events(), andplugin_context— the 20-key dict handed to every plugin'ssetup()— is built inline inside it.Issue #48 flagged this while planning the split and asked for exactly this:
It never got written. Writing it first, because a key silently dropped or renamed by a move is invisible to every other test in the suite — nothing in-tree reads most of these — and would break plugins at runtime, in the field.
This is the backend's version of the window contract, and the frontend carve just taught me what that costs: 43 of
library.js's exports were referenced only from app.js's top-levelwindowblock, invisible to any call-graph scan, and trusting the scan would have shipped a dead A–Z rail with CI fully green. A contract only external code reads has to be pinned by name, before the move.The surface is bigger than server.py's dict
Shipped plugins read
logandload_sibling— and neither is in it.plugins/__init__.pylayers them on per-plugin. A test pinning only server.py's 18 keys would have missed both.Codex caught me writing a vacuous assertion
My first identity test built a dict locally and called
setup()on it — assertingdict(x)['k'] is x['k'], which is trivially true and blind to everything the loader does. [P2], and correct.It now drives the real
plugins.load_plugins()with a probe plugin. That matters, because the loader does deliberately wrap one key:register_library_provideris scoped per-plugin so a plugin cannot forge owner attribution and impersonate another. The test pins that single intentional exception, so it can't quietly become two.Codex then caught a second [P2]: my hand-rolled teardown restored only
PLUGINS_DIRandLOADED_PLUGINS, whileload_plugins()also mutatessys.path,sys.modulesandPENDING_PLUGINS— making the suite order- and environment-dependent.tests/test_plugins.pyalready had a fixture that does this properly, soreset_plugin_statemoved totests/conftest.py: one shared copy rather than a second that will drift.Bite-tested in five directions
plugin_contextlog)extract_metain the loader — all key names intact, identity brokenregister_library_providerscoping — the impersonation guardpytest 2399 · Codex 0.
🤖 Generated with Claude Code
Summary by CodeRabbit