fix(env): treat unsubstituted config templates as unconfigured - #1126

Summary

A Claude Desktop extension writes the literal string ${user_config.<field>} into the engine environment for every field the user has not filled in. The value is non-empty, so every presence check downstream counted it as a real credential: doctor reported the source healthy, permission_preflight returned ready with safe: true, and the backend sent the placeholder upstream and surfaced the vendor's 401/400/422 instead of falling back. A whole-value ${user_config.*} placeholder is now rejected once at the config boundary, so every consumer agrees the credential is unset, and both diagnostics name it as unset rather than reading it as "nothing configured".

Rejection means absent, not empty: the placeholder is cleared from the process environment and the key is re-resolved from the lower-priority sources exactly as it would be had the host never written it. A real .env, Keychain, or pass credential the placeholder was shadowing is therefore used rather than discarded.

This branch also repairs a pre-existing test failure that was blocking CI. tests/test_source_outcomes.py::test_pipeline_records_both_mode_semantic_leg_failure_as_partial let the pipeline derive its freshness window from the wall clock while its fixture item is dated 2026-08-10; once the calendar moved 30 days past that date the item was filtered out and the recorded state stayed error instead of partial. The failure reproduces on untouched main, so the default branch is currently red for this test — the repair is here so this PR's CI can go green, but a maintainer may prefer it to land independently.

Testing

• uv run pytest
• Added or updated tests that would catch a regression, or explained why not below

Full suite: 4690 passed, 6 skipped, 0 failed. Coverage 89.89% against the 84% floor in pyproject.toml, which is unmodified.

New coverage: tests/test_env_unsubstituted_template.py (the boundary predicate, the sweep, the environment clearance, the shell-default exclusions, and the three fallback behaviours below), plus additions to tests/test_doctor.py and tests/test_permission_preflight.py. Each new case was observed failing against the commit before its fix.

Validated end to end with the issue's own repro — templated credentials in the process environment:

$ SCRAPECREATORS_API_KEY='${user_config.scrapecreators_api_key}' \
  GEMINI_API_KEY='${user_config.gemini_api_key}' \
  last30days.py doctor
setup: not recorded; credentials present: none (values never shown)
note: unsubstituted config template(s), counted as unset: GEMINI_API_KEY, SCRAPECREATORS_API_KEY (set a real value or remove each)

Before the change the same run reported both keys as present and healthy.

The test repair was verified clock-independent by asserting the derived window (2026-07-21 … 2026-08-20) rather than only the outcome. A suite-wide scan for the same rot — every window-deriving call site with an unpinned window, out-of-window fixtures, and an assertion that items survived — reported the one site before the fix and none after.

Changelog

• Added changelog.d/1081.fixed.md
• Skip changelog — chore/internal only (also add the skip-changelog label)

Agent disclosure

AI review

Three review passes in total: two internal rounds (correctness, security, testing, adversarial) and one external review (Greptile) that ran on each pushed head.

The internal rounds produced findings recorded below. The external review raised two further issues, both valid and both fixed in this branch:

• A valid fallback credential was discarded. The sweep blanked a rejected placeholder instead of re-resolving from the lower-priority sources, so a real .env, Keychain, or pass credential the placeholder was shadowing was lost. Rejection now re-resolves; a lower-priority value that is itself a placeholder is not treated as a credential. This was also the first internal round's top finding, so two independent reviews converged on it.
• The fallback skipped key rotation. The ScrapeCreators multi-key rotation runs before the sweep, so a comma-separated list restored from a lower-priority source was assigned whole and the backend received k1,k2 as one credential. The rotation is now extracted and reapplied after the sweep.

Earlier in the branch, a comment claimed the extension namespace excluded shell-default syntax while the pattern matched it; the pattern was tightened and the claim made true.

Security

The change mutates the process environment (os.environ.pop) inside the config loader. Reviewed and judged safe: a value can only be popped when the resolved config value is a whole-value placeholder, which requires the environment's own value to be that placeholder or falsy — so a real credential can never be removed.

Fixture-recording redaction is not weakened. The sweep clears only placeholder strings, and the recording session also snapshots os.environ at start, so no real secret loses its redaction.

The rejected key names (never values) are rendered into diagnostics; they come from the config's own key set, so no secret can leak through them.

Notes

The fix is scoped to the placeholder namespace the issue names (${user_config.*}) and to whole-value matches. A future host that emits a placeholder with surrounding text would read as configured again.

The sweep covers the keys get_config registers. A credential read straight from the environment under a name it does not register — LAST30DAYS_API_KEY, or a bare SCRAPE_CREATORS_API_KEY spelling left behind after the canonical key resolved — keeps its placeholder. The code comment states this bound; widening the sweep is left to review.

Relationship to this change

• None
• Yes — disclosure:

Related issues

Fixes #1081

Session-settled decisions carried from planning: reject the template once at the config boundary rather than at each presence check (user-directed, over patching only doctor._setup_block).

Unapplied review findings

Reviewed by correctness, security, testing, adversarial, and Greptile. Every bullet below sits
at confidence anchor 50 — below the bar for unattended application — so none were applied. They are
surfaced here for the reviewer to fix in this branch, dismiss, or carry past merge.

Round 1

• P2 — skills/last30days/scripts/lib/env.py — Template is blanked, not unset; lower-priority credentials lost
  Fixed — the sweep now re-resolves from the lower-priority sources. Also raised independently by
  Greptile as P1.

• P2 — skills/last30days/scripts/lib/env.py — Hosted bearer LAST30DAYS_API_KEY escapes the sweep
  The sweep scans only registered config keys, but LAST30DAYS_API_KEY is deliberately not one
  (see tests/test_env_doc_contract.py DOC_ONLY_KEYS) and is read straight from the environment by
  hosted.py, then sent as Authorization: Bearer <key>. Not reachable via today's mcp/manifest.json.
  Suggested fix: scan os.environ for whole-value placeholders rather than only config keys.

• P3 — skills/last30days/scripts/lib/env.py — OPENAI_AUTH_STATUS/SOURCE left stale after the key is emptied
  Fixed — a rejected OPENAI_API_KEY now resets the pair to missing / none.

• P3 — tests/test_doctor.py — Doctor "not listed as present" assertion can never fail
  The test injects only the rejected-names record, with no credential value, so keys_present is
  deterministically False and the negative assertion cannot fail for any input. Suggested fix:
  drive it from a real env.get_config() with the templated env var set.

• P3 — skills/last30days/scripts/lib/env.py — Emptied config contradicts absent-env for the one empty-means-disable key
  For LAST30DAYS_YT_PLAYER_CLIENT, '' is a documented disable while absent-from-env means the
  android default, so config and environment can disagree for that key. Suggested fix: exempt keys
  whose unset state is an empty string.

• P3 — skills/last30days/scripts/lib/env.py — os.environ.pop makes the rejection non-idempotent across a second get_config()
  Addressed in substance — the record now lists only keys left genuinely unset, so a second load
  that finds no placeholder no longer claims a stale templated state. Still worth deciding whether the
  record should survive a reload.

Round 2

• P2 — skills/last30days/scripts/lib/env.py — The branch ships a global mutation whose replacement semantics are still undecided
  Raised as a merge-strategy call, not a code defect. The sweep rewrites config values and clears
  process-environment entries for every invocation. Suggested resolution: land the read-only half (the
  predicate, the record, and the two diagnostics) separately from the mutation, or decide the
  replacement semantics first and gate the mutation.

• P3 — skills/last30days/scripts/lib/env.py — Process-environment clearance is name-bounded
  A SCRAPE_CREATORS_API_KEY spelling left behind after the canonical key resolved still holds its
  placeholder in the environment, and so does any unregistered name. The code comment now states this
  bound; widening the sweep (or clearing known aliases) remains open.

• P3 — tests/test_source_outcomes.py — The range_from assertion does not guard its pin
  lookback_days=30 equals pipeline.run's own default, so deleting that keyword leaves
  range_from unchanged and the test still passes; only the range_to assertion is load-bearing.
  Suggested fix: state that in the test, or pin a non-default width.

• P3 — tests/test_doctor.py — The new doctor assertion proves render plumbing, not the sweep→record coupling
  It hand-builds {env.TEMPLATE_CONFIG_KEYS: [...]}, so a mismatch between the key the sweep writes
  and the key the renderer reads would not be caught. Suggested fix: drive it through env.get_config().

• P3 — tests/test_env_unsubstituted_template.py — No test asserts no environment name still holds a placeholder after get_config()
  The suite checks the specific keys it sets. Suggested fix: assert over os.environ for any
  remaining whole-value ${user_config.*} placeholder among the extension-wired names.

• P3 — coverage — No end-to-end test that a rejected credential makes a backend fall back
  The changelog's central claim is tested indirectly (config emptied, one provider flag false).
  Suggested fix: assert env.get_reddit_source(config) is None after a templated
  SCRAPECREATORS_API_KEY, or that a templated Google key removes the provider from diagnose().

Review runs: cr-1081-1789061901 (internal round 1), cr-1126-fix2-1789068256 (internal round 2), plus Greptile on each pushed head.