feat(registry): add opt-out for entry-point plugin autoloading - #164
feat(registry): add opt-out for entry-point plugin autoloading#1640xjunger wants to merge 2 commits into
Conversation
Discovery imports every installed inspect_robots.* entry point on first lookup (list/resolve). Locked-down and reproducibility-sensitive eval environments have no way to turn that off. Add INSPECT_ROBOTS_DISABLE_PLUGIN_AUTOLOAD: any non-empty value skips entry-point discovery so only in-tree builtins and hand-registered components resolve. Mirrors pytest's PYTEST_DISABLE_PLUGIN_AUTOLOAD. The flag is read at discovery time and deliberately not latched, so clearing it re-enables discovery within the same process. This is a defense-in-depth and reproducibility switch, not a security boundary: an installed package can still run code when imported. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Can you open an issue describing the situation that prompted this? I'd like the use case on record before committing to an env var, since that contract is permanent once it ships. The implementation does work, I checked rather than assumed. With the var set, What gives me pause is the motivation. An environment locked down enough to care about discovery-time imports also controls what gets pip-installed, and the PR says up front this isn't a security boundary. The case I find more convincing is the mundane one: muting a misbehaving installed plugin without uninstalling it, and keeping eval manifests independent of whatever happens to be in the venv. If that's closer to what you're actually hitting, say so in the issue and this gets a lot easier to say yes to. One thing worth fixing regardless. Setting the var breaks two existing tests, CONTRIBUTING.md:131 also wants a CHANGELOG entry under Unreleased, and docs/guide/plugins.md is where people would look for this. |
jeqcho
left a comment
There was a problem hiding this comment.
Thanks for this — the feature is well-motivated, the pytest precedent is a good one to mirror, and the core design is sound and pleasantly minimal. The not-latched semantics in src/inspect_robots/registry.py:104-106 are implemented correctly (the early return happens before _loaded_entrypoints is set, so clearing the variable re-enables discovery), builtins load unconditionally, INSPECT_ROBOTS_DISABLE_PLUGIN_AUTOLOAD is consistent with the existing INSPECT_ROBOTS_POLICY / INSPECT_ROBOTS_EMBODIMENT naming, and the module docstring is honest about this being defense-in-depth rather than a security boundary. registry.py is also the only discovery point in the package, so there is no second cache to invalidate. CI is green across the matrix and the full suite passes locally.
Three things before this merges:
-
Missing
CHANGELOG.mdentry. CONTRIBUTING.md (step 4 under "Submitting changes") asks for an entry under "Unreleased" — this is a user-visible feature, so it should get an### Addedline. -
No user-facing documentation. The env var currently only lives in the module docstring.
docs/guide/plugins.mdis where users will look for plugin behavior, anddocs/guide/cli.md:34already documents the otherINSPECT_ROBOTS_*variables. A short section inplugins.md(a couple of sentences — what it does, that builtins still load, that it mirrors pytest's variable, and that it's not a security boundary) would close the gap. Worth stating there that the switch is read at discovery time, so setting it after discovery has already run does not unregister plugins that were loaded earlier in the process. -
test_autoload_opt_out_skips_entrypoints_but_keeps_builtinscannot fail on regression (tests/test_registry_cli.py:80-84). The_FakeEP.load()tripwire raisesAssertionError, but_ensure_loadedwrapsep.load()in a blanketexcept Exceptionthat converts it to aRuntimeWarning— and the suite does not run with warnings-as-errors. Verified empirically: with_autoload_disabled()hardcoded toFalse(simulating a broken opt-out), this test still passes; onlytest_autoload_opt_out_is_not_latchedcatches it. Suggested fix: haveload()set acalledflag (or return a real factory) and assertnot called/probe not in policies, or wrap the call inpytest.warns/recwarnassertions. As written, the builtins assertion is the only part of this test doing real work.
Two optional notes, no action required:
- Per the docstring, any non-empty value disables autoload, so
INSPECT_ROBOTS_DISABLE_PLUGIN_AUTOLOAD=0also disables it. That matches pytest's behavior and is documented, but it is a common surprise — worth calling out explicitly in the guide docs from item 2. - The pre-existing sentence at
registry.py:5-7("appears ininspect-robots listwithout being imported first") contradicts what discovery actually does —ep.load()imports the plugin — and your new paragraph sits directly beneath it, making the tension more visible. Not this PR's bug, but since you are already editing the docstring, tightening that sentence would be a welcome drive-by.
Happy to re-review once the changelog, docs, and test tweak land — the core implementation itself looks ready. Thanks again! 🙏
Motivation
registry._ensure_loaded()imports every installedinspect_robots.*entrypoint on first discovery (
list/resolve). Locked-down orreproducibility-sensitive eval environments currently have no way to disable
that automatic import.
Change
Adds
INSPECT_ROBOTS_DISABLE_PLUGIN_AUTOLOAD. Any non-empty value skipsentry-point discovery, so only in-tree builtins and hand-registered components
resolve. This mirrors pytest's
PYTEST_DISABLE_PLUGIN_AUTOLOAD.discovery within the same process.
Scope / non-goals
This is a defense-in-depth and reproducibility switch, not a security
boundary: an installed package can still run code when imported. It only stops
the framework from importing plugins on your behalf during discovery.
Tests
Two new tests covering the skip path and the not-latched behavior. Full core
suite passes at 100% coverage.