Skip to content

fix(local,keychain,api): prefer keychain for secrets, disable stdio MCP by default, surface approval-record failures - #1891

Open
ra-co88 wants to merge 1 commit into
UsefulSoftwareCo:mainfrom
ra-co88:fix/secrets-provider-defaults
Open

fix(local,keychain,api): prefer keychain for secrets, disable stdio MCP by default, surface approval-record failures#1891
ra-co88 wants to merge 1 commit into
UsefulSoftwareCo:mainfrom
ra-co88:fix/secrets-provider-defaults

Conversation

@ra-co88

@ra-co88 ra-co88 commented Aug 30, 2026

Copy link
Copy Markdown

What

Three defaults change in the shipped local config:

  1. The keychain plugin registers before the file store — minted OAuth tokens default to the OS keychain where one is reachable.
  2. dangerouslyAllowStdioMCP defaults to false — stdio MCP servers are off unless explicitly enabled.
  3. Approval-record persistence failures are surfaced through the error-capture channel instead of being silently swallowed.

Why

Tokens defaulted to the plaintext file store even on platforms with a durable keychain — the weaker storage became the default by registration order, not by decision. Stdio servers spawn local processes; that is not a default a security-conscious distribution should ship. And a swallowed approval-record failure meant a pause that never recorded its approval — invisible breakage.

What changed

  • describeKeychainAvailability() encodes the platform truth (reachable/unreachable/unsupported) driving the ordering; the config consults it.
  • The MCP plugin already rejects stdio with a clear error when disabled — the default now takes the safe side.
  • recordPendingApproval failures report through the host's error capture; execution behavior is unchanged (the pause still returns paused, not a 500).

Headless/Linux behavior is unchanged: where the keychain probe fails, the file store remains the default.

Test plan

  • Availability helper platform-truth encoding (3 tests).
  • Config contract: ordering + stdio default (3 tests).
  • Approval-record failure surfacing (2 tests).

All green against current main.

…CP by default, surface approval-record failures
@ra-co88
ra-co88 force-pushed the fix/secrets-provider-defaults branch from bff2366 to 4400942 Compare August 30, 2026 17:14
@ra-co88

ra-co88 commented Aug 30, 2026

Copy link
Copy Markdown
Author

Heads-up on the red E2E (cloud 13of16) check here: it's failing on main itself (e.g. the Version Packages runs), so it's pre-existing rather than from this PR. It's the cap-eviction scenario tripping over workerd resetting session Durable Objects mid-initialize when the test opens its burst of sessions — diagnosis and a proposed fix in #1895.

@ra-co88 ra-co88 left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: approve — secrets-defaults hardening, correctly designed and tested. (Comment review: GitHub blocks formal self-approval on your own PR.)

Supersession check (upstream/main @ 2dc399e): the keychain plugin has no availability-probe ordering upstream (secrets.ts has zero keychain markers; the file store still wins by registration order), dangerouslyAllowStdioMCP still defaults open, and approval-record failures are still swallowed. Not superseded on any of the three changes.

All three defaults take the safe side, and the mechanism is a probe, not a platform guess:

  • describeKeychainAvailability() encodes reachable/unreachable/unsupported and the config consults it — the keychain registers ahead of the file store only where the platform truth says it works. Headless/Linux behavior is unchanged (probe fails ⇒ file store, exactly as today). The weaker store stops being the default by accident of registration order.
  • dangerouslyAllowStdioMCP defaults false — the name carries the risk posture, and the MCP plugin already rejects with a clear error when disabled, so the off-by-default is self-explanatory at the failure site.
  • Approval-record persistence failures surface through error capture while execution behavior is unchanged (pause still returns paused) — invisible breakage becomes visible without turning a recording failure into a user-facing 500. That separation (report vs. fail) is the right call for a best-effort record.

Test discipline: the availability probe has its own suite (keyring.availability.test.ts), the config ordering is asserted in executor.config.test.ts, and the approval-logging suite (150 lines) covers the surfaced-failure path. e2e local-server wiring included.

CI caveat, not blocking: the single failing shard (E2E cloud 13of16, run 2026-08-30) is the same cap-eviction openSession flake #1895 fixes — unrelated to this payload.

Good to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants