refactor(storage): use the instance plugin store - #3
Conversation
There was a problem hiding this comment.
QA panel review — WARN
code-review-structural · head 581399b49177 · formal
Low-risk PR: the change replaces a defensive try/except fallback with a bare sdk.plugin_store call, which is reasonable given the raised host floor but does lose a safety net. The most actionable item is the conftest.py gap — the host_stub fixture is missing plugin_store, so any test that exercises _data_dir without the LEARNING_WIKI_DIR override will hit an AttributeError. The panel's removed-behavior lane considered the fallback removal intentional; the correctness lane flagged it as a regression. The verifier confirmed both readings are accurate and kept the finding at minor. All three findings passed verification unchanged; no structural pass was skipped.
Findings
| Severity | Location | Finding | Verified | |
|---|---|---|---|---|
| 🟡 | minor | __init__.py:31 |
Removing the try/except Exception fallback means that if sdk.plugin_store returns None or raises, the error propagates unhandled instead of degrading to ~/.pro… | confirmed |
| 🟡 | minor | tests/conftest.py |
The host_stub fixture's fake graph.sdk module is missing the plugin_store attribute that the new _data_dir code requires, so any future test that uses host_stu… | confirmed |
| ⚪ | nit | SEAMS.md |
The new graph.sdk.plugin_store seam used by init._data_dir is not documented in the SEAMS.md "Seams exercised" table, which the file explicitly describes a… | confirmed |
findings JSON (machine-readable)
[
{
"file": "__init__.py",
"line": 31,
"severity": "minor",
"category": "correctness",
"claim": "Removing the try/except Exception fallback means that if sdk.plugin_store returns None or raises, the error propagates unhandled instead of degrading to ~/.protoagent/learning_wiki as the old code did. Flagged by correctness review; the removed-behavior review considered this intentional given the raised host floor, but the loss of the None-guard is a real (if low-probability) regression in defensive behavior.",
"evidence": "Old code: \"except Exception: # noqa: BLE001 \u2014 standalone / tests / older host\\n p = Path.home() / \".protoagent\" / \"learning_wiki\"\" \u2014 new code has no equivalent guard around \"p = sdk.plugin_store(plugin_id=\"learning_wiki\")\"; a None return would crash at \"p.mkdir(parents=True, exist_ok=True)\".",
"verdict": "confirmed",
"note": "Diff shows the try/except fallback removed; new __init__.py has bare `p = sdk.plugin_store(...)` with no None-guard before `p.mkdir(parents=True, exist_ok=True)`. Accurate."
},
{
"file": "tests/conftest.py",
"line": 0,
"severity": "minor",
"category": "cross-file",
"claim": "The host_stub fixture's fake graph.sdk module is missing the plugin_store attribute that the new _data_dir code requires, so any future test that uses host_stub and calls _data_dir without the LEARNING_WIKI_DIR override will get an AttributeError.",
"evidence": "conftest.py host_stub sets: sdk.schedule_recurring, sdk.cancel_scheduled, sdk.create_watch, sdk.record_metric, sdk.Knobs, sdk.make_knob_tools, sdk.start_goal_loop, sdk.stop_goal_loop \u2014 no sdk.plugin_store. The new __init__.py calls sdk.plugin_store(plugin_id=\"learning_wiki\").",
"verdict": "confirmed",
"note": "Read conftest.py at branch tip: host_stub sets exactly those 8 attributes on the fake sdk module, no plugin_store. The new _data_dir calls sdk.plugin_store when no override is set. Accurate."
},
{
"file": "SEAMS.md",
"line": 0,
"severity": "nit",
"category": "cross-file",
"claim": "The new graph.sdk.plugin_store seam used by __init__._data_dir is not documented in the SEAMS.md \"Seams exercised\" table, which the file explicitly describes as documenting \"every seam the learning domain genuinely needs.\"",
"evidence": "SEAMS.md \"Seams exercised\" table lists graph.sdk.schedule_recurring, graph.sdk.start_goal_loop, graph.sdk.record_metric, graph.sdk.Knobs, graph.sdk.make_knob_tools \u2014 but not graph.sdk.plugin_store, which is now called in __init__._data_dir.",
"verdict": "confirmed",
"note": "Read SEAMS.md at branch tip: the 'Seams exercised' table lists all other graph.sdk.* seams but not plugin_store. The intro claims to document 'every seam the learning domain genuinely needs.' Accurate."
}
]2 finding(s) excluded from the verdict by in-diff confinement (file not among this PR's changed paths):
tests/conftest.py(minor) — The host_stub fixture's fake graph.sdk module is missing the plugin_store attribute that the new _data_dir code requires, so any future test that uses host_stubSEAMS.md(nit) — The new graph.sdk.plugin_store seam used by init._data_dir is not documented in the SEAMS.md "Seams exercised" table, which the file explicitly describes as
Summary
sdk.plugin_storedata_dirandLEARNING_WIKI_DIRas literal overridesValidation
ruff check .pytest -q— 104 passedRefs protoLabsAI/protoAgent#3083