got-feedBack/feedBack
1
0
Fork 0
mirror of https://github.com/got-feedBack/feedBack.git synced 2026-07-21 12:21:49 +00:00
Code Issues Actions 2 Packages Projects Releases Wiki Activity

test(plugins): pin the plugin_context contract before carving server.py (R3b) (#898)

Browse Source 
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>
This commit is contained in:
Byron Gamatos 2026-07-12 00:29:48 +02:00 committed by GitHub
parent 756588678b
commit 1cef01d02c
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 265 additions and 50 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

60
tests/conftest.py
View File

@ -1,6 +1,8 @@
"""Shared pytest fixtures for the feedBack test suite."""
import importlib
import logging
import sys
import pytest
import structlog
@ -76,3 +78,61 @@ def isolate_logging():
lg.setLevel(original_level)
lg.propagate = original_propagate
structlog.reset_defaults()
# ── Plugin-loader isolation ─────────────────────────────────────────────────────
#
# Lifted verbatim out of tests/test_plugins.py so more than one test module can drive
# the real plugins.load_plugins(). It has to be ONE fixture, not a copy per file:
# load_plugins() mutates sys.path, sys.modules, PENDING_PLUGINS and LOADED_PLUGINS, and a
# partial restore makes the suite order- and environment-dependent (Codex [P2] on
# test_plugin_context_contract.py — it was right).
# Bare module names that this test module pre-populates into
# sys.modules to simulate the bare-import path. Saved/restored by
# the reset_plugin_state fixture so they don't leak to other test
# files. Codex / Copilot review on PR for feedBack#33.
_BARE_NAMES_USED = ("util", "extractor")
@pytest.fixture()
def reset_plugin_state(monkeypatch):
"""Clear loader module-level state and restore on teardown.
Saves and restores:
* `plugins.LOADED_PLUGINS`
* any `plugin_*` keys we add to `sys.modules`
* the bare names this module simulates (`util`, `extractor`)
* `sys.path` — `plugins.load_plugins()` mutates it
Also unsets `FEEDBACK_PLUGINS_DIR` for the test's duration
(via monkeypatch) so a CI env that pre-sets it can't leak
real user plugins into a tmp_path-driven test. Per-module
locks are owned by the standard import system
(`importlib._bootstrap._module_locks`) and are not our
responsibility to reset.
"""
monkeypatch.delenv("FEEDBACK_PLUGINS_DIR", raising=False)
plugins = importlib.import_module("plugins")
saved_loaded = list(plugins.LOADED_PLUGINS)
saved_pending = dict(plugins.PENDING_PLUGINS)
saved_modules = {k: v for k, v in sys.modules.items() if k.startswith("plugin_")}
saved_bare = {k: sys.modules[k] for k in _BARE_NAMES_USED if k in sys.modules}
saved_path = list(sys.path)
plugins.LOADED_PLUGINS.clear()
plugins.PENDING_PLUGINS.clear()
for k in list(sys.modules):
if k.startswith("plugin_") or k in _BARE_NAMES_USED:
del sys.modules[k]
try:
yield plugins
finally:
plugins.LOADED_PLUGINS.clear()
plugins.LOADED_PLUGINS.extend(saved_loaded)
plugins.PENDING_PLUGINS.clear()
plugins.PENDING_PLUGINS.update(saved_pending)
for k in list(sys.modules):
if k.startswith("plugin_") or k in _BARE_NAMES_USED:
del sys.modules[k]
sys.modules.update(saved_modules)
sys.modules.update(saved_bare)
sys.path[:] = saved_path

205
tests/test_plugin_context_contract.py Normal file
View File

@ -0,0 +1,205 @@
"""The plugin context is a THIRD-PARTY CONTRACT. Pin it.
`context` is handed to every plugin's `setup()`. Plugins — including ones we don't ship
and can't grep — read keys out of it and hold the callables as live references. Issue #48
flagged this while planning the server.py split and asked for exactly this assertion:
"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 doesn't exist yet, and server.py is about to be carved apart around the code that
builds it. This is the guard that makes the carve safe: 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.
Same lesson the frontend carve learned the hard way: a contract that only external code
reads cannot be found by a call-graph scan, so it has to be pinned by name.
WHY A LITERAL LIST AND NOT A DERIVED ONE. Deriving the expected set from the source would
assert the code equals itself. The whole point is that a human has to look at a diff and
consciously agree to change the contract.
"""
import ast
from pathlib import Path
import pytest
SERVER_PY = Path(__file__).resolve().parents[1] / "server.py"
PLUGINS_PY = Path(__file__).resolve().parents[1] / "plugins" / "__init__.py"
# The keys server.py puts in the shared context handed to register_plugin_api().
BASE_CONTEXT_KEYS = {
"config_dir",
"get_dlc_dir",
"extract_meta",
"meta_db",
"get_scan_status",
"get_art_cache_dir",
"library_providers",
"register_library_provider",
"unregister_library_provider",
"register_tuning_provider",
"unregister_tuning_provider",
"get_sloppak_cache_dir",
"register_demo_janitor_hook",
"award_xp",
"get_xp_progress",
"seed_xp",
"reset_xp",
"record_progression_event",
}
# Added PER PLUGIN by plugins/__init__.py on top of the base — so the surface a plugin
# actually sees is the union. Real shipped plugins read `log` and `load_sibling`, and
# neither is in server.py's dict; a test that pinned only the base would miss them.
PER_PLUGIN_KEYS = {"load_sibling", "log"}
FULL_CONTEXT = BASE_CONTEXT_KEYS | PER_PLUGIN_KEYS
def _plugin_context_keys() -> set:
"""The literal keys of server.py's `plugin_context = {...}`, read from the AST.
AST, not a regex: the dict spans ~40 lines and is dense with comments, lambdas and
nested calls, and the values contain braces of their own.
"""
tree = ast.parse(SERVER_PY.read_text(encoding="utf-8"))
for node in ast.walk(tree):
if (
isinstance(node, ast.Assign)
and node.targets
and isinstance(node.targets[0], ast.Name)
and node.targets[0].id == "plugin_context"
and isinstance(node.value, ast.Dict)
):
keys = set()
for k in node.value.keys:
assert isinstance(k, ast.Constant), (
"plugin_context must be built from literal string keys — a computed "
"key makes this contract un-reviewable"
)
keys.add(k.value)
return keys
pytest.fail(
"server.py no longer builds a literal `plugin_context = {...}` dict. If it moved "
"to another module, point this test at that module — do NOT delete it."
)
def test_plugin_context_keys_are_exactly_the_pinned_contract():
actual = _plugin_context_keys()
missing = BASE_CONTEXT_KEYS - actual
added = actual - BASE_CONTEXT_KEYS
assert not missing, (
f"plugin_context lost {sorted(missing)}. Every one of these is read by plugins we "
"do not control and cannot grep. Dropping one breaks them at runtime, in the "
"field, with nothing else in this suite failing."
)
assert not added, (
f"plugin_context gained {sorted(added)}. That's fine — but it is a PUBLIC API "
"addition, so add the key to BASE_CONTEXT_KEYS here deliberately, and document it "
"in docs/. This test exists to make that a conscious act rather than a side effect."
)
def test_per_plugin_keys_are_still_layered_on_top():
"""`log` and `load_sibling` are added per-plugin in plugins/__init__.py, not by
server.py — so they're invisible to the check above. Real plugins read both."""
src = PLUGINS_PY.read_text(encoding="utf-8")
for key in sorted(PER_PLUGIN_KEYS):
assert f'plugin_context["{key}"]' in src, (
f"plugins/__init__.py no longer sets plugin_context[{key!r}] — shipped plugins "
"read it"
)
def test_context_values_reach_a_REAL_plugin_by_identity(tmp_path, reset_plugin_state):
"""The contract is CALLABLE IDENTITY, not just key names.
A carve that moves these into a module and re-exports them through a wrapper (a
property, a functools.partial, a lazily-bound getter) keeps every key name intact and
STILL breaks plugins that stored the reference at setup() time.
Codex [P2] on the first cut of this test, and it was right: I originally built a dict
locally and called setup() on it, which asserts `dict(x)['k'] is x['k']` — trivially
true, and blind to everything plugins/__init__.py does. It has to go through the REAL
loader, because the real loader is exactly what copies and re-binds the context.
(That is not hypothetical: `register_library_provider` IS deliberately wrapped by the
loader, per-plugin, to force owner attribution. Pinned below so the one intentional
exception can't quietly become two.)
"""
from fastapi import FastAPI
# reset_plugin_state (tests/conftest.py) is the ONLY safe way to drive the real
# load_plugins(): it also mutates sys.path, sys.modules and PENDING_PLUGINS, and a
# hand-rolled partial restore makes the suite order- and environment-dependent.
# Codex [P2] on the first cut of this, and it was right.
plugins_mod = reset_plugin_state
plugin_dir = tmp_path / "ctxprobe"
plugin_dir.mkdir()
(plugin_dir / "plugin.json").write_text(
'{"id": "ctxprobe", "name": "ctx probe", "routes": "routes.py"}'
)
# A backend plugin's entry point is routes.py's `setup(app, ctx)` — the same shape
# tests/test_plugins.py::_make_plugin uses. The probe hands the context BACK through a
# sink in the context itself: importing the probe module by name does not work (the
# loader namespaces plugin modules), and a file/JSON channel would lose the object
# IDENTITY that is the entire point of this test.
(plugin_dir / "routes.py").write_text(
"def setup(app, ctx):\n"
" ctx['_probe_sink'].append(ctx)\n"
)
sentinel_db = object()
def sentinel_extract(_p):
return {}
def sentinel_register_library_provider(provider, *a, **kw):
return None
sink = []
context = {
"_probe_sink": sink,
"meta_db": sentinel_db,
"extract_meta": sentinel_extract,
"config_dir": tmp_path,
"register_library_provider": sentinel_register_library_provider,
}
app = FastAPI()
saved_dir = plugins_mod.PLUGINS_DIR
plugins_mod.PLUGINS_DIR = tmp_path
try:
plugins_mod.load_plugins(app, context)
finally:
plugins_mod.PLUGINS_DIR = saved_dir
assert sink, "the probe plugin's setup() never ran — the harness is not exercising the loader"
seen = sink[0]
assert seen["meta_db"] is sentinel_db, "meta_db must reach a real plugin BY IDENTITY"
assert seen["extract_meta"] is sentinel_extract, (
"extract_meta must reach a real plugin BY IDENTITY — wrapping it (partial, "
"property, re-binding getter) breaks plugins that stored the reference at setup()"
)
assert seen["config_dir"] is context["config_dir"]
# The loader adds these per-plugin; shipped plugins read both.
assert callable(seen["load_sibling"])
assert seen["log"].name == "feedBack.plugin.ctxprobe"
# THE ONE DELIBERATE WRAPPER. register_library_provider is scoped per-plugin so a
# plugin cannot forge owner attribution and impersonate another. Pinned so that the
# single intentional exception to identity cannot quietly become two.
assert seen["register_library_provider"] is not sentinel_register_library_provider, (
"register_library_provider is supposed to be wrapped per-plugin for owner "
"attribution — if that wrapper is gone, a plugin can impersonate another"
)

50
tests/test_plugins.py
View File

@ -46,56 +46,6 @@ def capture_logger(caplog, logger_name, level=logging.WARNING):
logger.propagate = orig_propagate
# Bare module names that this test module pre-populates into
# sys.modules to simulate the bare-import path. Saved/restored by
# the reset_plugin_state fixture so they don't leak to other test
# files. Codex / Copilot review on PR for feedBack#33.
_BARE_NAMES_USED = ("util", "extractor")
@pytest.fixture()
def reset_plugin_state(monkeypatch):
"""Clear loader module-level state and restore on teardown.
Saves and restores:
* `plugins.LOADED_PLUGINS`
* any `plugin_*` keys we add to `sys.modules`
* the bare names this module simulates (`util`, `extractor`)
* `sys.path` — `plugins.load_plugins()` mutates it
Also unsets `FEEDBACK_PLUGINS_DIR` for the test's duration
(via monkeypatch) so a CI env that pre-sets it can't leak
real user plugins into a tmp_path-driven test. Per-module
locks are owned by the standard import system
(`importlib._bootstrap._module_locks`) and are not our
responsibility to reset.
"""
monkeypatch.delenv("FEEDBACK_PLUGINS_DIR", raising=False)
plugins = importlib.import_module("plugins")
saved_loaded = list(plugins.LOADED_PLUGINS)
saved_pending = dict(plugins.PENDING_PLUGINS)
saved_modules = {k: v for k, v in sys.modules.items() if k.startswith("plugin_")}
saved_bare = {k: sys.modules[k] for k in _BARE_NAMES_USED if k in sys.modules}
saved_path = list(sys.path)
plugins.LOADED_PLUGINS.clear()
plugins.PENDING_PLUGINS.clear()
for k in list(sys.modules):
if k.startswith("plugin_") or k in _BARE_NAMES_USED:
del sys.modules[k]
try:
yield plugins
finally:
plugins.LOADED_PLUGINS.clear()
plugins.LOADED_PLUGINS.extend(saved_loaded)
plugins.PENDING_PLUGINS.clear()
plugins.PENDING_PLUGINS.update(saved_pending)
for k in list(sys.modules):
if k.startswith("plugin_") or k in _BARE_NAMES_USED:
del sys.modules[k]
sys.modules.update(saved_modules)
sys.modules.update(saved_bare)
sys.path[:] = saved_path
def _make_plugin(plugin_root, plugin_id, *, sibling_files=None, routes_body=None):
"""Create a minimal plugin directory under `plugin_root`.