Skip to content

fix(secrets): warn on legacy config shapes that silently load no secrets - #78575

Open
0xr00tf3rr3t wants to merge 1 commit into
NousResearch:mainfrom
0xr00tf3rr3t:fix/secrets-legacy-config-silent
Open

fix(secrets): warn on legacy config shapes that silently load no secrets#78575
0xr00tf3rr3t wants to merge 1 commit into
NousResearch:mainfrom
0xr00tf3rr3t:fix/secrets-legacy-config-silent

Conversation

@0xr00tf3rr3t

Copy link
Copy Markdown
Contributor

What does this PR do?

Two secrets: config shapes are read by nothing on current main and emit no diagnostic whatsoever. The user gets zero credentials and no explanation for why.

  1. secrets.provider — the single-backend selector that predates source composition. Nothing reads this key.
  2. secrets.<source> as a scalar instead of a mapping — coerced to {} at agent/secret_sources/registry.py:286, so is_enabled() returns False and the source never runs.

Both are exactly what a config written against the earlier iteration of the secrets work looks like, so this lands on the people who adopted secret sources earliest.

This adds a warning for each. Diagnostics only — no resolution behavior changes, no config keys added, no new surface.

Reproducing on current main

from agent.secret_sources import registry

env = {}
report = registry.apply_all(
    {"provider": "command", "command": "printf 'MY_API_KEY=secret123\n'"},
    home, environ=env,
)
env vars applied : []
log messages     : []
report.errors    : None
report.warnings  : None

Silence. The helper is never run, MY_API_KEY never loads, and nothing says why.

The failure mode that makes this worth fixing: it is especially hard to spot when the same env file is also injected by a systemd EnvironmentFile=. Everything keeps working while the Hermes source contributes nothing — the config looks alive and is dead. You discover it the day you run hermes chat from a plain shell, or move the profile to a host without that unit. I hit exactly this on my own deployment; the block had been inert for weeks while systemd quietly did the work.

Related Issue

No existing issue — found while migrating a config from the pre-composition schema.

Context: the secrets.provider selector was intentionally dropped during the salvage of #44509 into #69266, because mutually-exclusive selection would have regressed multi-source composition. That was the right call. This PR is only about the migration edge it left behind: a config written against the old shape fails closed and silent.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

agent/secret_sources/registry.py (+21, one function, purely additive)

  • _ordered_enabled_sources() warns once when secrets.provider is present, naming the working replacement (secrets.<source>.enabled: true).
  • The per-source loop warns when a section is present but not a mapping, reporting the offending type.

Both follow the existing secrets.sources names unknown source(s) precedent in the same function — same logger, same phrasing style, same place.

tests/secret_sources/test_legacy_secrets_config.py (new, 14 tests)

How to Test

pytest tests/secret_sources/test_legacy_secrets_config.py -q

Tests run against the real orchestrator with a real temp HERMES_HOME and real helper subprocesses (chmod +x shell scripts) — no mocks, per the rubric's E2E guidance.

Verification that the tests actually test the fix:

with patch patch reverted
5 warning assertions pass fail
9 behavior-unchanged assertions pass pass

The 9 that pass either way are the point: they pin that resolution is unchanged — legacy shapes stay inert (they do not start working), the current schema still loads, a half-migrated config (provider alongside a valid command mapping) still loads its valid section, and a disabled source stays disabled.

One test asserts the warning never echoes the configured value, since a command section's value is a shell string that can name sensitive paths.

Regression surface:

pytest tests/secret_sources/ tests/test_bitwarden_secrets.py \
       tests/test_env_loader_secret_sources.py \
       tests/test_command_secret_source.py tests/test_onepassword_secrets.py -q
114 passed

pytest tests/ -k "env_loader or secret or secrets" --ignore=tests/gateway/relay -q
549 passed, 5 skipped

Pre-existing failures unrelated to this change, present identically on unmodified main: tests/tools/test_browser_secret_exfil.py and tests/tools/test_image_source.py (verified by stashing the patch and re-running), plus tests/gateway/relay/ collection errors from a missing pytest_asyncio in my environment.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(secrets):)
  • I searched existing PRs and issues to make sure this isn't a duplicate
  • My PR contains only changes related to this fix (2 files, +190/-0)
  • I've run the suite and all tests pass
  • I've added tests for my changes
  • I've tested on my platform: Ubuntu (Linux)

Documentation & Housekeeping

  • No new config keys — nothing to add to cli-config.yaml.example
  • Docstring/comments explain why each shape is unusable, not just what the code does
  • Cross-platform: pure-Python logging, no platform-specific behavior
  • README / docs — N/A (no user-facing surface change; the warnings are self-describing)

Notes for reviewers

Two judgment calls worth flagging:

Warn rather than raise. A hard failure would be more visible, but the secret-source design is deliberately fail-open — apply_all never blocks startup, and every source degrades to "no value." Raising here would break that contract for a config that is merely stale. The warning is emitted once per startup via the same logger as the neighbouring secrets.sources check.

Nothing is "fixed" into working. A legacy config keeps loading nothing; it just says so now. Silently starting to honor secrets.provider would resurrect the mutually-exclusive selector that was intentionally removed, and would change behavior for anyone whose stale key names a source they no longer want enabled.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint area/auth Authentication, OAuth, credential pools area/config Config system, migrations, profiles sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Aug 4, 2026
Two `secrets:` shapes are read by nothing on current main and emit no
diagnostic whatsoever — the user gets zero credentials and no explanation.

1. `secrets.provider` — the single-backend selector that predates source
   composition. Nothing reads it.
2. `secrets.<source>` as a scalar rather than a mapping — coerced to {} in
   _ordered_enabled_sources (registry.py:286), so is_enabled() returns
   False and the source never runs.

Both are exactly what a config written against the earlier iteration looks
like, so this lands on the users who adopted secret sources earliest. It is
especially hard to spot when systemd also injects the same env file via
EnvironmentFile: everything keeps working while the source contributes
nothing, so the config looks alive and is dead until you start Hermes from
a plain shell or move to another host.

Adds a warning for each, following the existing 'secrets.sources names
unknown source(s)' precedent in the same function. Diagnostics only — no
resolution behavior changes, no config keys added, no new surface.

Tests exercise the real orchestrator against a real temp HERMES_HOME with
real helper subprocesses. The 5 warning assertions fail without this patch
and pass with it; 9 further tests pin that resolution is unchanged (legacy
stays inert, current schema still loads, half-migrated configs still work,
disabled stays disabled) and pass either way. One test asserts the warning
never echoes the configured value, which can be a command string.
@0xr00tf3rr3t
0xr00tf3rr3t force-pushed the fix/secrets-legacy-config-silent branch from 2e3fde4 to ad91633 Compare August 4, 2026 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools area/config Config system, migrations, profiles comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants