Skip to content

feat(mcp): request_secret returns usable multi-field secrets as private files - #551

Merged
lukemaj merged 2 commits into
mainfrom
feat/550-usable-secret-requests
Oct 19, 2026
Merged

lukemaj merged 2 commits into
mainfrom
feat/550-usable-secret-requests

Conversation

@lukemaj

@lukemaj lukemaj commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

What: request_secret now hands the agent a usable secret: one card can ask for 1-5 fields, and the result gives a private file path for each field.
Why: Before this, a saved value could only be used as a one-use secretRef, and only schedule_task's webhook signature accepted one, so agents didn't use the card for ordinary credentials.
So what: Ready for the user's merge approval: independent review passed and CI is green on 410d91d. Before/after images of the web card still need a browser pass, which this run was not allowed to do.

Closes #550

Problem

An agent asking for a token, API key or login could only get a secretRef, and nothing used it except webhook signing. So agents either asked in chat or gave up.

Fix

  • Contracts: request_secret takes optional fields (1-5 {name, label, placeholder?}; names are unique and match [a-z0-9_-]+). The turn item and the internal secret_request.record command carry the field metadata, never values. secrets.answerRequest accepts {type:"save", secret} for single-field cards, as before, and {type:"save", secrets:{name: value}} for multi-field cards. A new fields_mismatch error covers a missing or extra field. Values are kept exactly as typed, spaces included; only blank values are refused. Field names cannot be __proto__.
  • Server (SecretRequests): a save first checks that every field is present (all or nothing). It then writes each value to its own 0600 file inside a 0700 directory, <stateDir>/secret-requests/<ref-hex>/<field>. That path is in the environment's T3 home userdata tree, never a worktree. Answers run one at a time under a lock. The files are written to a temp directory and renamed into place, so the agent never sees half a save. A leftover from a failed earlier save of the same card is replaced. A declined, cancelled or lost-race request, or a failed record, leaves no files.
  • Tool result: {status:"saved", files:{name: absolutePath}, secretRef?}. A request without fields returns the key secret.
  • Clients: web and mobile cards keep typed values in a Map, so a field named like an Object property, such as constructor, reads its own value, and render one input per field through the shared secretRequestFields / secretRequestAnswerInput helpers in client-runtime. A single-field card looks the same as before. The privacy note now says "Sent privately to the agent, never shown in chat".
  • Agent guidance: the tool description and the orchestration instructions (packages/provider-core/src/server/orchestrationInstructions.ts on current main) tell agents to use request_secret for any secret, to use the file without printing it ("$(cat path)", bao kv put … field=-, rbw add), and to delete it when done.

Decisions the Issue left to the worker

  • secretRef mapping: only a request with exactly one field gets a secretRef. That covers no fields, or fields with one entry. A ref has always carried exactly one value, and schedule_task's signature needs exactly one. Multi-field requests get only files. Because answers are serialized, a retried save replaces both the ref and the file, so they always hold the same value, and a failed answer cannot delete what another answer saved. ScheduledTaskService needs no change.
  • Cleanup trigger: the existing sweep (once at startup, then hourly) now also covers request directories. It removes a directory when its newest file is older than the 24 h TTL, when the agent has emptied it, or when it is a temp directory left by a save that stopped midway. A save that loses a race or fails to record deletes its files right away. No new timer or scheduler was added.
  • Peer routing: request_secret is still forwarded by the generic McpRouting path, unchanged. A forwarded call returns paths on the destination environment, which only an agent on that host can read. Cross-machine routing stays out of scope (feat(peer): request destination secrets and create signed webhooks without a destination agent #385).

Upstream

pingdotgg/t3code #15907 (merged) is the current card, and #17410 (closed) was a mobile paste button. Upstream has no work on returning usable values or on multi-field cards.

Elon record

  • Requirements and who asked: The user (2026-10-19) wants agents to ask for a secret through a card, optionally with two fields such as an ID and a key, and to use it for anything: copy it to OpenBao or Bitwarden, or use it once.
  • Deleted: Rakazo-style HTTP injection proxy, a named credential store and settings UI, a separate two-step question flow (one card with several fields covers it), cross-machine routing (feat(peer): request destination secrets and create signed webhooks without a destination agent #385), and a new cleanup scheduler (the existing hourly sweep covers files).
  • Bottleneck: Saved values had no consumer except webhook signatures, so the card went unused for ordinary credentials.
  • Checked myself: Read SecretRequests.ts, secretRequest.ts, the request_secret tool and handler, ScheduledTaskService.ts (the secretRef consumer), McpRouting.ts (peer forwarding) and both cards. Ran the focused tests and typechecks below on this branch.

Proof (head 410d91d: feature commit 99070d3 plus a fork-registry commit, on origin/main e5d864c)

The first three heads (243e98d, 540853a, c5745de) were built on a stale local main that had diverged from origin/main, so the PR showed 61 commits and 610 files. The change is now one commit replayed onto origin/main. On main, these files had only lost comments, mobile imports had moved under components/ui, and the instructions file had moved to packages/provider-core. The old history is kept locally on backup/550-on-stale-main.

  • vp test run apps/server/src/secrets/SecretRequests.test.ts packages/client-runtime/src/secretRequest.test.ts packages/contracts/src/orchestratorMcp.test.ts apps/server/src/mcp/OrchestratorMcpToolkit.integration.test.ts apps/server/src/scheduledTasks/ScheduledTaskService.webhook.test.ts apps/server/src/peer/PeerToolRouting.test.ts apps/server/src/mcp/OrchestratorMcpService.test.ts apps/server/src/mcp/toolkits/core.test.ts apps/server/src/orchestration-v2/ProjectionStore.test.ts: 9 files, 194 tests passed. They cover a single-field save that also leaves a file; a multi-field save with file contents, 0600 file modes, a 0700 directory, no ref, and no values in the recorded command; a missing or extra field writing nothing; decline and a lost race to the card closing leaving no files; values kept exactly as typed; retries after a failed record replacing a leftover ref and a leftover directory; a failed racing answer leaving the winner intact; the sweep removing expired and emptied directories and skipping an unreadable one; field-name schema limits; Object-property field names; and, at the MCP seam, values absent from the tool result and from the whole thread projection, with the files readable.
  • Failing-before checks, run on the earlier heads: the round-1 regressions fail on 243e98d, and the round-2 regressions fail on 540853a.
  • vp run typecheck exits 0 in packages/contracts, packages/client-runtime, packages/provider-core, apps/server, apps/web and apps/mobile. vp fmt --check and vp lint on the touched files are clean. The only lint warning, Orchestrator.ts layerUnavailable, was already on main.
  • Independent review: rounds 1 and 2 returned REQUEST_CHANGES, and all findings were fixed. Round 3 approved c5745de. Round 4 approved the rebased 99070d3 as equivalent, with one optional P3 about a restored doc comment, left as is. Round 5 approved the fork-registry commit 410d91d, and review/independent is success on that head.
  • CI at 410d91d: 21 checks passed and 4 were skipped, including Fork Stack Model. That check had failed on 99070d3 until scripts/fork-upstream-edits.txt and docs/fork-features.md registered the 8 edited upstream files.
  • Not done: before/after images of the web card. This run was told not to launch dev servers or browsers, so that needs a separate approved pass.

Website impact: none. This changes an agent tool and an in-app card, with no marketing-site content or URLs.

Model and harness: Claude Opus 5.5 in Claude Code (T3 Code).

🤖 Generated with Claude Code

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:XXL labels Oct 19, 2026
@lukemaj

lukemaj commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

Independent review, round 1 (head 243e98d), relayed unchanged

Reviewer: Prism reviewer (codex, gpt-6.1-sol). Verdict: REQUEST_CHANGES. The focused tests passed: 3 files, 25 tests.

  1. P1: Files and secretRef can hold different values. SecretRequests.ts:229. Concurrent single-field saves pick their winners independently. Save A publishes its files; save B skips writing because the directory exists, then creates the ref before A does. Both saves succeed, and the agent gets A's file alongside a ref holding B's value. The same mismatch happens when a record fails, ref removal fails, directory removal succeeds, and the retry uses a changed value.
  2. P2: Valid field names crash both cards. Web SecretRequestCard.tsx:76, mobile SecretRequestCard.tsx:80. The names constructor and __proto__ pass the schema, but the cards' initial {} state inherits properties with those names, so .trim() throws on render. Assigning __proto__ into a plain object also does not create an own property.
  3. P2: The multi-field path changes secret values. client-runtime/secretRequest.ts:99, contracts/secretRequest.ts:35. The client helper and the RPC's TrimmedNonEmptyString both trim, so " password " is stored as "password".
  4. P2: One filesystem error stops cleanup for every other request. SecretRequests.ts:395. A dangling symlink makes stat fail on every sweep. The error handler wraps the whole loop, so later expired directories are never processed.

Author adjudication: all four are accepted, and fixes are coming in the next push.

lukemaj added a commit that referenced this pull request Oct 19, 2026
Review round 1 on #551:
- A one-field save now stores its ref first and writes the file from the
  value the ref holds, so a racing or retried save can never leave the file
  and the ref with different values.
- Cards keep typed values in a Map, so field names such as `constructor`
  read their own value; the schema rejects `__proto__`.
- Values are kept exactly as typed; only blank values are refused.
- The file sweep skips a directory it cannot read or remove instead of
  stopping for every other request.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
lukemaj added a commit that referenced this pull request Oct 19, 2026
…p cannot collide

Review round 2 on #551:
- Answers run under one lock, so two clients answering one card see one
  answer win, and a failed answer's cleanup cannot delete what another
  answer saved.
- With answers serialized, a ref or directory already present on a pending
  card is a failed earlier save's leftover; a new save replaces both (set
  and replace instead of create and first-wins), so the file and the ref
  always hold this save's value.
- A ref whose file could not be written is removed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@lukemaj

lukemaj commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

Independent review, round 2 (head 540853a), relayed

Reviewer: Prism reviewer (codex, gpt-6.1-sol). Verdict: REQUEST_CHANGES. The focused tests passed: 4 files, 36 tests.

  1. P1: A retry can still leave the file and the ref with different values. SecretRequests.ts:131. Save "first", fail the record, remove its ref successfully, but fail to remove its directory. A retry with "second" creates a ref holding "second", while writeFiles sees the surviving directory and keeps the "first" file.
  2. P1: A failed racing answer can delete the files of the answer that succeeded. SecretRequests.ts:273. Two clients pass the pending check. One answer's record fails while the other records saved, and the failed answer's cleanup then removes the shared ref and directory.

Round-1 status per the reviewer: Object-property field names are fixed, trimming is fixed, and the sweep is fixed. The file/ref mismatch was not fully fixed; see 1 above.

Author adjudication: both findings are accepted and fixed in c5745de. Answers now run under one lock. With answers serialized, any ref or directory left on a pending card can only be a failed earlier save's leftover, so a new save replaces both. The ref is now set rather than created, and the directory is replaced rather than kept. Three new regressions fail on 540853a and pass on c5745de.

@lukemaj

lukemaj commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

Independent review, round 3 (head c5745de), relayed

Reviewer: Prism reviewer (codex, gpt-6.1-sol). Verdict: APPROVE. It found no blocking findings in the full PR or in the round-3 delta. The focused tests passed: 4 files, 37 tests.

All six prior findings are fixed: the round-1 file/ref race, Object-property field names, trimmed values and the sweep abort, and the round-2 leftover directory and failed-racing-answer cleanup.

The reviewer traced these and found them sound:

  • Cancel record: closeCard against the post-dispatch re-check.
  • Deadlock: no nested answer call or reverse lock acquisition, and the semaphore releases on every exit.
  • Global lock: it waits on no human answer, so the tradeoff is acceptable.
  • Remove-then-rename window: a saved card fails the pending check first.

It found no new value-bearing output in the cards, helpers, schemas, RPC, spans, recorded commands, MCP results, file modes or cleanup paths.

The reviewer did not check visual behavior.

…te files

request_secret gains optional fields (1-5 {name, label, placeholder?}). One
card shows every field on web and mobile, and saving is all or nothing. The
result maps each field name to a 0600 file in a 0700 per-request directory
under the environment's state directory, so the agent can use the value
without it entering the transcript. A one-field request still gets a
secretRef for schedule_task. Values are kept exactly as typed. Answers run
one at a time, so a retried or racing save always leaves the file and the
ref in agreement and never deletes another answer's save. Files expire with
the 24 hour ref TTL through the existing startup and hourly sweep, which
skips a directory it cannot read.

Rebased onto origin/main from 243e98d, 540853a and c5745de, which
were built on a stale local main.

Closes #550

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@lukemaj
lukemaj force-pushed the feat/550-usable-secret-requests branch from c5745de to 99070d3 Compare October 19, 2026 19:04
@github-actions github-actions Bot added size:L and removed size:XXL labels Oct 19, 2026
@lukemaj

lukemaj commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

Independent review, round 4 (rebased head 99070d3), relayed

Reviewer: Prism reviewer (codex, gpt-6.1-sol). Verdict: APPROVE. It found no blocking findings. The focused tests passed: 4 files, 38 tests.

  • P3, nonblocking: SecretRequests.ts:80 puts back the consume JSDoc that upstream pruned. It changes no behavior.
  • Equivalence: the executable changes match the approved c5745de change, apart from the moved orchestration instructions and the components/ui/ mobile imports. The reviewer found no lost or garbled hunks, and the parent is exactly e5d864c.
  • New code on main: the reviewer traced every SecretRequests caller and found no stale savedRef. Restart and recovery handling and projection decoding keep the field metadata. The Promachos request-answering path (feat(orchestration): the Promachos answers requests no parent can #533) handles approvals and questions only, so it needs no change.
  • Visual behavior: not verified.

Author adjudication: the P3 is optional and stays as is. Removing it would only produce a new head that needs another review.

@lukemaj

lukemaj commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

Agent cost: agent-observer publish --task toolboxmd/chromeria#551 --repo toolboxmd/chromeria --pr 551 reported "nothing to publish: no sessions in scope", so this PR has no cost comment.

The Fork Stack Model check requires every edited upstream file to be
allowlisted and owned. Adds the eight secret-request files to
scripts/fork-upstream-edits.txt and a usable-secret-requests entry in
docs/fork-features.md, sharing the files other features already own.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added size:XL and removed size:L labels Oct 19, 2026
@lukemaj

lukemaj commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

Independent review, round 5 (head 410d91d, fork registry delta), relayed

Reviewer: Prism reviewer (codex, gpt-6.1-sol). Verdict: APPROVE. It found no blocking findings.

  • The delta touches only docs/fork-features.md and scripts/fork-upstream-edits.txt, 45 added lines.
  • Each of the 8 upstreamFiles has exactly one owner, and all 7 sharedFiles belong to other features.
  • All 15 files the PR edits against origin/main are allowlisted.
  • The purpose, Issue, PR and keywords match the implementation.
  • node scripts/fork-features.mjs check reports OK (54 features), and git diff --check is clean.
  • The reviewer could not run the full fork-check.sh locally because there is no upstream remote. CI's Fork Stack Model check covers it.

@github-actions

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 5.0 KiB 5.0 KiB 0 B (0.0%) 6.8 KiB ✅
Codex Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Codex Live turn WebSocket wire 1.2 KiB 1.2 KiB 0 B (0.0%) 2.0 KiB ✅
Codex Live turn WebSocket decoded 20.9 KiB 20.9 KiB 0 B (0.0%) 29.3 KiB ✅
Codex Live turn messages 2 2 0 (0.0%) 8 ✅
Claude Total thread wire 5.0 KiB 5.0 KiB 0 B (0.0%) 6.8 KiB ✅
Claude Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Claude Live turn WebSocket wire 1.2 KiB 1.2 KiB 0 B (0.0%) 2.0 KiB ✅
Claude Live turn WebSocket decoded 21.2 KiB 21.2 KiB 0 B (0.0%) 29.3 KiB ✅
Claude Live turn messages 2 2 0 (0.0%) 8 ✅

Baseline: e5d864c · PR result: 410d91d · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@lukemaj

lukemaj commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor Author

Before/after (web, real dev client)

I ran 410d91d107 and origin/main (e5d864cbce) in isolated worktree dev servers. A real Codex agent made each request_secret call.

Before (main): one field only, and the card tells the user the value is never shown to the agent.
before

After (this PR): the agent asked for client_id and client_secret, and one card shows both fields.
after

Filled values are masked:
filled

After saving, the thread only shows "Saved securely and kept private":
saved

End-to-end check after saving (dummy values):

  • .t3/userdata/secret-requests/ is drwx------, the request directory is drwx------, and client_id and client_secret are each -rw-------. Each file holds exactly the value that was typed.
  • Grepping the worktree's SQLite databases and WAL, the dev server log, and ~/.codex/sessions for both values found 0 hits.

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

Labels

size:XL vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(mcp): request_secret returns usable multi-field secrets to the agent

1 participant