fix(secrets): isolate plugin bootstrap before hydration - #82607
fix(secrets): isolate plugin bootstrap before hydration#82607alexgunsberg wants to merge 3 commits into
Conversation
94c8ce7 to
3ac3941
Compare
Bartok9
left a comment
There was a problem hiding this comment.
Collab review (Proton Pass / first-process hazard)
I pulled pr-82607, read the full diff against #81960, and ran:
python3 -m pytest tests/test_external_secret_source_plugin_startup.py -q
# 6 passed in ~1.6s
Verdict
Prefer this PR’s shape over #81960’s post-discovery re-pull for the hazard you called out: full PluginManager import before secrets are hydrated can cache a register() failure when an unrelated plugin reads a plugin-sourced env var. Targeted bootstrap (manifest filter / AST legacy probe → restricted context → apply_all → ordinary discovery later, _discovered untouched) is the right seam.
Live probe I added on top of your suite (not in-tree yet): secret-source plugin hydrates VAULT_TOKEN, then full discover_plugins() loads a consumer plugin whose register() requires that env — passes on this branch. That is the real agent-to-agent analogue of the Proton Pass failure mode.
What already looks solid
- Explicit
hermes_home=scoping (only scans that home’splugins/+ that home’splugins.enabled) - Unrelated enabled plugins not imported during bootstrap (your marker test)
- Omitted
secrets.sourcesstill discovers via enabledsecrets.<name>sections - Fail-open + redacted bootstrap/fetch errors (no exception text interpolation)
- Benign config read does not import plugins
- Fetch-exception redaction in
registry.py
Gaps I’d close before merge (happy to send a tiny follow-up PR if you want)
1. Silent re-register on full discovery (noise, not correctness)
Bootstrap already registers the source. Full discovery re-imports the module (same sys.modules name) and calls register() again with a full PluginContext. register_source then logs:
Secret source '…' already registered; ignoring duplicate
On a clean Proton Pass boot that warning is a false alarm. Minimal fix in PluginContext.register_secret_source (or register_source when the existing entry is the same name and replace=False): treat “already registered” as success / debug, not warning.
2. Missing regression: consumer plugin after hydration
Your suite proves the source plugin path and “unrelated not imported early,” but not “later full discovery register() sees credentials.” Suggest adding something like:
def test_full_discovery_consumer_plugin_sees_hydrated_secret(tmp_path):
_write_secret_source_plugin(
tmp_path,
fetch_body=(
" return FetchResult(secrets={'STARTUP_TEST_API_KEY': 'ready'})"
),
)
consumer = tmp_path / "plugins" / "consumer-plugin"
consumer.mkdir()
(consumer / "plugin.yaml").write_text(
"name: consumer-plugin\nkind: standalone\nversion: 1.0.0\n",
encoding="utf-8",
)
(consumer / "__init__.py").write_text(
"import os\n"
"from pathlib import Path\n"
"def register(ctx):\n"
" assert os.environ.get('STARTUP_TEST_API_KEY') == 'ready'\n"
" Path(os.environ['HERMES_HOME'], 'consumer-ok').write_text('ok')\n",
encoding="utf-8",
)
# enable both plugins + secrets section …
# load_hermes_dotenv → discover_plugins → assert consumer-okThat locks the ordering contract against a future “optimize” that moves bootstrap after general import again.
3. Optional: only treat known source-shaped keys as configured
_discover_configured_secret_source_plugins currently treats every secrets.* dict value as a source name. Today that’s probably fine; if secrets: ever gains non-source nested maps, bootstrap would scan for unknown names. Low urgency — flag only.
4. vs #81960
Keep origin tracking / docs polish from #81960 if useful, but do not land post-discovery re-pull instead of this bootstrap for the first-process consumer hazard. Re-pull can remain a belt for child processes; it is not a substitute for hydrate-before-unrelated-import.
Docs nit
PluginContext.register_secret_source NOTE now says discovery runs when secrets.sources contains an unregistered name — accurate for the list path; also mention enabled secrets.<source> sections (your omitted-list test).
I’m not opening a competing PR. If you want the consumer test + quiet re-register as a patch on your branch, say the word and I’ll push a PR against alexgunsberg:fix/plugin-secret-source-startup or paste a patch. @alexgunsberg
Collab review (Proton Pass / first-process hazard)I pulled VerdictPrefer this PR’s shape over #81960’s post-discovery re-pull for the hazard you called out: full Live probe I added on top of your suite (not in-tree yet): secret-source plugin hydrates What already looks solid
Gaps I’d close before merge (happy to send a tiny follow-up PR if you want)1. Silent re-register on full discovery (noise, not correctness)
On a clean Proton Pass boot that warning is a false alarm. Minimal fix in 2. Missing regression: consumer plugin after hydration def test_full_discovery_consumer_plugin_sees_hydrated_secret(tmp_path):
_write_secret_source_plugin(
tmp_path,
fetch_body=(
" return FetchResult(secrets={'STARTUP_TEST_API_KEY': 'ready'})"
),
)
consumer = tmp_path / "plugins" / "consumer-plugin"
consumer.mkdir()
(consumer / "plugin.yaml").write_text(
"name: consumer-plugin\nkind: standalone\nversion: 1.0.0\n",
encoding="utf-8",
)
(consumer / "__init__.py").write_text(
"import os\n"
"from pathlib import Path\n"
"def register(ctx):\n"
" assert os.environ.get('STARTUP_TEST_API_KEY') == 'ready'\n"
" Path(os.environ['HERMES_HOME'], 'consumer-ok').write_text('ok')\n",
encoding="utf-8",
)
# enable both plugins + secrets section …
# load_hermes_dotenv → discover_plugins → assert consumer-okThat locks the ordering contract against a future “optimize” that moves bootstrap after general import again. 3. Optional: only treat known source-shaped keys as configured 4. vs #81960 Docs nit
I’m not opening a competing PR. If you want the consumer test + quiet re-register as a patch on your branch, say the word and I’ll push a PR against |
3ac3941 to
30e60da
Compare
|
Thanks for the follow-up push ( Closed
Still optional (not blocking from my side)
No competing PR from me. LGTM on the bootstrap isolation direction — happy to re-run the suite if you want another pair of eyes after any further polish. |
|
@alexgunsberg — quick human note on top of the technical review: This is really strong work. The targeted bootstrap-before-hydration design is the right fix for the Proton Pass / first-process hazard, and the follow-up isolation commit tightened it further. We want to see this land. We’re staying on the thread (not dropping until it’s merged). If anything would help — rebase onto main, a small patch on your branch, another suite run, CI triage, wording nits — just say the word and we’ll jump on it. No pressure and no competing PR from us; happy to support your PR until it’s published. Thanks for doing this carefully. |
Weekly collab check-inStill watching this until merge — no pressure, no competing PR. Status snapshot (2026-08-10):
Happy to help if useful:
Just ping @Bartok9 anytime. Thanks again for the careful work here. |
Weekly collab check-inStill watching until merge — no pressure, no competing PR. Status snapshot (2026-08-17):
Happy to help if useful:
Just ping @Bartok9 anytime. Thanks again for the careful work here. |
Problem
An external secret-source plugin can be configured in
secrets.sources, but dotenv loading runs before ordinary plugin discovery. The first process pass therefore treats the source as unknown and skips its credentials.Fix
Add a restricted bootstrap phase before secret hydration:
provides_secret_sources, with syntax-only compatibility for legacy direct-registration pluginsregister_secret_sourcesecrets.sourcesand enabledsecrets.<source>sectionsThis is a targeted alternative to #81960. That PR re-pulls secrets after full general plugin discovery; this PR hydrates declared source plugins before unrelated plugins import, so plugins that need those credentials during
register()do not cache a first-process failure.Regression proof
On unmodified upstream, the added tests fail because the source is unknown on first load. On this branch they verify:
secrets.sourcesstill works through enabled source configVerification
scripts/run_tests.shrelated plugin/env/config suite: 148 passedgit diff --check: passed