fix(onepassword): a failed batch read no longer outlasts the 45-second bound #35937

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

The 1Password driver now uses one 45-second timeout for the batch attempt and fallback reads. If the deadline expires, it returns values collected so far. A test uses delayed fake subprocess results to check the overall bound.

Changes: 1Password secret reads

Layer / File(s)	Summary
Read deadline and fallback behavior packages/secret-provider-onepassword/src/server/driver.ts, packages/secret-provider-onepassword/src/server/driver.test.ts	The driver applies one timeout to the batch attempt and fallback reads, and returns values collected before the deadline. The test fake supports delayed exit results, and a new test checks that the read completes within 45 seconds.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested labels: bugfix

Suggested reviewers: yordis

Merge Risk: Low · up to eb2e8

The 1Password read now has one overall time limit, which matches the issue's request. The new test checks only the time bound, not that values read before the timeout are kept. Adding that assertion would protect the behavior; it should not block merging.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title names the main change: a failed batch read no longer outlasts the 45-second bound. The raw summary and objectives confirm driver.ts now applies one 45-second timeout to the batch attempt and…
Linked Issues check		Requirements from #35930: (1) the 45-second bound must cover the whole secret read, including the sequential op read fallback after a failed batch; (2) a batch timeout must still skip fallback. The …
Out of Scope Changes check		The reviewed diff for driver.ts changes only the read path named in #35930: bounded is replaced by recover, the read deadline is moved to cover the whole read, and the partial-value result handl…
Description check		The description covers the required sections of the template. Problem: the per-call READ_TIMEOUT let a failed op inject plus sequential op read fallbacks exceed the intended bound (two references took…

Full details: Title check

Explanation

The title names the main change: a failed batch read no longer outlasts the 45-second bound. The raw summary and objectives confirm driver.ts now applies one 45-second timeout to the batch attempt and per-reference fallback reads, and the new test checks that bound. The title is specific and follows conventional commit format.

Full details: Linked Issues check

Explanation

Requirements from #35930: (1) the 45-second bound must cover the whole secret read, including the sequential op read fallback after a failed batch; (2) a batch timeout must still skip fallback. The head driver.ts diff meets both. read wraps the combined batch-plus-fallback effect in one Effect.timeoutOption(READ_TIMEOUT). The per-call bounded wrapper is replaced by recover, which only converts errors to undefined and logs them. A failed readTogether returns undefined and falls through to readOne for each reference. When the deadline expires, the code logs one 'did not answer in time' warning and returns the values read so far. Because a timeout during inject ends the whole effect, no fallback runs after a timed-out inject. A single reference runs one bounded readOne. The test change (fake op with virtual-clock delays, 40 s inject failure and 40 s reads, assert completion within 45 000 ms) targets the reported two-reference case. I did not read the test file directly. The PR states it fails at 120000 ms without the fix and passes with it, and this is consistent with the driver logic. The evidence section of the PR description is a placeholder for before/after capture in the app. That is non-coding and does not affect this verdict.

Full details: Out of Scope Changes check

Explanation

The reviewed diff for driver.ts changes only the read path named in #35930: bounded is replaced by recover, the read deadline is moved to cover the whole read, and the partial-value result handling is added. These changes are needed for the requirement. The test changes in driver.test.ts (fake op delay support, reproduction test) support the regression check for the same issue. The Layer.merge restructuring of the test provides is a test-only adjustment to satisfy a type diagnostic and does not alter behavior. No unrelated files or behavior changes were identified in the inspected evidence.

Full details: Description check

Explanation

The description covers the required sections of the template. Problem: the per-call READ_TIMEOUT let a failed op inject plus sequential op read fallbacks exceed the intended bound (two references took 120 s). Change: a single Effect.timeoutOption(READ_TIMEOUT) now wraps the combined read and fallback, and bounded became recover. Scope: it links the bug issue (pingdotgg/t3code#35930). Verification: it gives a focused regression test that fails on the base code (120000 > 45000) and passes with the fix, plus a real-subprocess reproduction with timings, typecheck, and check:changed results. It also states what was not exercised (integrated app UI, real 1Password auth, partial-result preservation under a shared deadline). It names the models that did the work, as the template asks.

Generate unit tests (beta)

• Commit to this branch
• Create a new PR