feat: agents ask the user for a secret through a private card#15907

🟡 Medium orchestration-v2/Orchestrator.ts:6850

A concurrent run termination can mark a secret request cancelled after answerSecretRequest has stored the secret but before its saved update arrives, leaving the UI cancelled while an active webhook secret exists. Because this branch silently returns for any non-pending card, the subsequent saved update is discarded; serialize the save/cancel transition or make the update atomic so a completed save cannot be cancelled.

The save path checks the run first and refuses an answer once it has ended. If the run ends in the moment between that check and the record, the stored value expires unused and the hourly sweep deletes it, so it doesn't linger.

The check still leaves the interval before the save record. If cancellation wins there, the saved record is ignored while savedRef can retrieve the already-stored value directly until expiry. The transition still needs serialization/atomicity.

Fixed in f71850c. Instead of locking across the store write and the record, the save now re-reads the card after recording. If the agent's wait closed it first, the record changed nothing, so the stored value is deleted straight away and the user gets agent_stopped. savedRef can't return it after that. A test closes the card just before the save's record lands, and fails without the change.

🟠 High scheduledTasks/ScheduledTaskService.ts:1208

Concurrent answerSecretRequest calls can both pass the pending check, so a save may persist webhook_secret while a concurrent decline records the request as declined, or two saves may overwrite each other. Because the secret write and secret_request.record dispatch are separate operations, the card's terminal status is not atomically claimed; claim the pending request before applying the answer and secret.

No longer relevant as of 084cb628c174adf60120c34a9ffbe46ed8e47e3d

🟡 Medium mcp/OrchestratorMcpService.ts:1434

When the wait reaches timeoutMs, this returns status: "pending" without cancelling the secret_request turn item. The polling fiber is interrupted by Effect.timeoutOption, so a later run termination cannot mark the request cancelled and answerSecretRequest can still store a secret for the completed run. Add timeout cleanup that records cancelled before returning.

No longer relevant as of 7c080780f8cb160dcb317230856808ed0f57f8c2

🟡 Medium state/server.ts:1309

Distinct secret-request cards can share this singleFlight key, so the second answer is deduplicated while the first is in flight; its secret is never sent, but its caller receives the first command's result and may show the wrong card as answered. Encode the three identifiers structurally (for example, with JSON.stringify) so delimiter-containing IDs remain distinct.

🟡 Medium secrets/SecretRequests.ts:105

A failed answer leaves stored secret values orphaned while the request remains pending. If either Effect.all write partially succeeds, or secret_request.record dispatch fails after both writes, no cleanup removes the stored keys; retrying mints another ref, allowing private values to accumulate indefinitely. Use compensating cleanup (and preserve retry-safe state) when either write or the dispatch fails.

No longer relevant as of e6004a24337cef5394577daa6ede0035eab23e6e

🟠 High src/scheduledTask.ts:110

SecretRequests.consume returns the secret even when store.remove fails, so a failed deletion leaves the same secretRef available for repeated consumption and breaks the one-use guarantee. Propagate the removal error or atomically mark the ref consumed before accepting the secret.

Already fixed in 900aa2e: consume fails with consume_failed when the value can't be deleted, so the ref is never handed out twice. SecretRequests.test.ts covers it.

🟠 High src/secretRequest.ts:74

secretRequestAnswerInput silently removes leading and trailing whitespace from saved secrets, so a value such as " password " is stored as "password" and later authentication with the one-use ref fails. Preserve the supplied secret unchanged and only reject an actually empty string.

🟡 Medium secrets/SecretRequests.ts:130

A transient store.get failure makes savedRef return Option.none(), so request_secret reports { status: "saved" } without a usable ref; the agent cannot consume the secret and the request cannot be answered again. Effect.orElseSucceed converts storage errors into a missing-ref result, so propagate the error instead (and update the savedRef error type/callers accordingly).

No longer relevant as of e6004a24337cef5394577daa6ede0035eab23e6e

🟡 Medium secrets/SecretRequests.ts:187

Saved refs that are never consumed remain in ServerSecretStore indefinitely, so their plaintext secrets are not dropped after SECRET_REF_TTL_MS (24 hours). Expiration is checked only in consumeRef; add cleanup for orphaned refs, such as scheduled expiry or cleanup when the waiting run ends.

Real: TTL was only enforced on consume, so a ref nobody used kept the value on disk. Fixed in bf4e846: the service now deletes expired secret-request values once at startup and then hourly, so a value outlives its 24 h TTL by at most an hour. A test with a real file store checks that an expired value is removed and a fresh value, the salt and unrelated secrets are kept.

🟡 Medium orchestration-v2/Orchestrator.ts:6865

A recovered run leaves this card with secretStatus: "pending" even after its generic status becomes "cancelled", so the client continues showing the Save/Decline form and submissions are rejected. Recovery must also transition pending secret requests to a terminal secretStatus when it cancels their turn items.

Covered differently: SecretRequests.answer refuses an answer once the run has ended (agent_stopped), so a recovered run's card can't take a value. The card's form showing until the next refresh is cosmetic.

The recovery path does set the item’s generic status to cancelled without changing secretStatus; the client renders from secretStatus, so the form persists after refresh and submissions still fail with agent_stopped. This needs a terminal secretStatus update during recovery.

You're right, recovery is a separate path from the one I described. Fixed in f71850c: restart recovery now sets secretStatus: "cancelled" along with status on a pending card, so its form closes. A recovery test checks it and fails without the change.

🟠 High secrets/SecretRequests.ts:176

Concurrent consumers can both receive the same secret, violating the one-use guarantee. consumeRef performs store.get and store.remove separately, so both calls can validate the value before either removes it; because remove succeeds when the file is already missing, both calls return the secret. Use an atomic read-and-delete operation (or equivalent compare-and-delete) in ServerSecretStore.

Real. Fixed in 6a72454: consumption is serialized, so two concurrent calls with one ref can't both read it before it's deleted. A new test runs two consumers at once against a store whose read yields; it fails without the fix (both succeed) and passes with it.

🟡 Medium mcp/OrchestratorMcpService.ts:1535

A pending secret card remains answerable after timeout, interruption, or a polling failure when the secret_request.record cancellation dispatch fails or the polling error exits the generator. Effect.ignore hides that failure, and cleanup is only attached to timeout/interruption, so a later save can orphan the entered secret after the caller has received timed_out or orchestration_error; attach cancellation cleanup to every post-creation exit and surface or retry cancellation failures.

The card is closed on timeout, on interruption, and when the run ends. On a polling error the run is still live, and the server refuses an answer once the run has ended (agent_stopped), so a value can't be saved after the agent is gone. A failed close is ignored because the run ending covers it.

You're right about the polling error. Fixed in 146ce1f: the card now closes on every exit except an answer (failed read, timeout, interruption), and a failed close is logged instead of ignored. If the close itself fails, the server still refuses an answer once the run ends, and an unused value expires within its 24 h TTL plus the hourly sweep.

🟠 High orchestration-v2/Orchestrator.ts:6855

When a pending card already exists, this reconstructs it from the new command and overwrites its runId, nodeId, label, or reason. A retry or concurrent request using the same clientRequestId can therefore answer and deliver the card to a different run, violating the idempotency guarantee that retries return the original card/ref. Verify these immutable fields against the existing card and return the existing card unchanged for a matching retry (or reject a mismatch).

Real. Fixed in 900aa2e652f: recording a request again leaves an open card exactly as it was asked (same run, label and reason), and a record for a different run is refused. The integration test re-records the open card with a different label and checks it is unchanged.

🟠 High secrets/SecretRequests.ts:192

Expired refs can remain on disk indefinitely when store.remove fails, because both the expiration path and sweepExpired discard removal errors with Effect.ignore; the API still reports the ref expired while the plaintext credential remains stored, with no retry or warning. Propagate or explicitly log removal failures so cleanup is observable and retried.

🟡 Medium mcp/OrchestratorMcpService.ts:1588

A declined answer that is recorded after the wait times out is returned as timed_out, so callers lose the user's actual decision. The post-timeout reread only treats saved as an answer; include declined as an answered terminal status before falling back to timed_out.

src/secretRequest.ts:19

This newly introduced service error exposes an arbitrary message string as its only attribute, rather than structured operation/resource/category fields with a message derived from them. Please model the failure cases as structured tagged errors (preserving the existing user-facing RPC copy), and keep underlying failures in cause rather than interpolating them into messages.

No diff: callers and the RPC error contract must change together.

No longer relevant as of 13a6c31eff3b8482c2ff39acf9ae70cd6b2fe15f

orchestration-v2/runtimeLayer.ts:54

This new service dependency is imported as a renamed layer, erasing the service module namespace. Please import * as SecretRequests and compose SecretRequests.layer here; the existing aliases on neighboring, unchanged imports need not be cleaned up.

No longer relevant as of 13a6c31eff3b8482c2ff39acf9ae70cd6b2fe15f

🟠 High scheduledTasks/ScheduledTaskService.ts:1615

A relay delivery can remain permanently claimed without ever forking runTask, so the request returns 500 (or a restart leaves the claim persisted) and the relay retry receives 202 duplicate while the webhook is silently dropped. The Effect.onError at log("accepted") only releases webhookQueued; it does not release the persisted delivery claim, and a crash between that insert and Effect.forkIn(serviceScope) bypasses the handler entirely. Release the relay claim when the accepted-log write fails, and make the claim-to-fork handoff crash-safe so an unstarted delivery can be retried or reconciled.

🟠 High scheduledTasks/ScheduledTaskService.ts:1614

The accepted delivery log persists rendered.prompt verbatim, so {{headers.authorization}} and {{query.access_token}} expose bearer tokens or API keys through getWebhookDelivery. Redact credential substitutions before logging (while retaining the raw prompt only for execution), or omit the rendered prompt from the delivery log.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

A retry after the card is cancelled or timed out returns cancelled immediately and does not show a new card.

Line 1503 derives turnItemId from clientRequestId. The pending record goes through secret_request.record. In dispatchSecretRequestRecord, an existing card with a non-pending secretStatus stays unchanged. Suppose the first call timed out and recorded cancelled. A retry with the same clientRequestId then finds the cancelled card, and the poll returns cancelled without asking the user again. The timed_out contract tells the agent to "ask again if still needed". An agent that reuses its clientRequestId, as the tool description recommends for retries, can never get a new card. The fix is to tell the agent to use a new clientRequestId after timed_out or cancelled, or to report the existing terminal status explicitly.

✅ Confirmed as addressed by @juliusmarminge

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

A failed status dispatch after a save leaves the request stuck.

answer creates the store entry before it dispatches secret_request.record. Suppose the dispatch fails, for example because the run ends between the status check and the dispatch, or because of a transient orchestration error. The value then stays on disk, and the item stays pending. Any retry from the card hits store.create and gets AlreadyExists, which maps to already_answered. The user cannot finish the save. The agent never sees saved, so it never receives the ref. The value stays on disk until the 24-hour sweep removes it.

Fix: if the record dispatch fails, remove the stored entry. Another option: when create hits AlreadyExists and the item is still pending, retry only the dispatch.

✅ Confirmed as addressed by @juliusmarminge

🟠 High secrets/SecretRequests.ts:141

A successful create followed by a failed threadManagement.dispatch leaves the request pending with its secret already stored. Retrying Save then maps isSecretAlreadyExistsError to already_answered, so it never retries the deterministic record command and the agent eventually times out. Treat an existing value for this request as stored and continue to dispatch (while preserving the race-safety checks) instead of returning already_answered.

No longer relevant as of 6eee1e72cb0b710a6fd0eded9d1472ba576c66f1

🟠 High chat/SecretRequestCard.tsx:132

type="text" leaves the secret visible in browsers that do not implement -webkit-text-security, so anyone viewing the screen can read the value while it is entered. Use a real password input (or another cross-browser masking mechanism) instead of relying solely on this non-standard CSS property.

Intentional. This was a password field until 54bddc0, which changed it to masked text so browsers stop offering to save a webhook secret as a login. -webkit-text-security is supported in every current engine: Chrome, Safari and Edge, plus Firefox since 114 (June 2023). The only exposure is a pre-2023 Firefox showing the value while it's typed, and nothing is stored or sent differently. Keeping it.

🟠 High src/orchestratorMcp.ts:613

OrchestratorMcpRequestSecretResult accepts { status: "saved" } without a secretRef, so an agent can believe the secret was saved but has no reference it can consume. Because secretRef is optional independently of status, this also occurs when the service's savedRef lookup returns None (including after a storage read error); model the result as a discriminated union that requires secretRef whenever status is "saved".

Real. Fixed in cae32ed: the result is now a union, and saved requires secretRef. A saved card whose value can't be read fails the call with orchestration_error, which tells the agent to ask again, instead of returning saved with no ref.