[quality] Triage empty catches and silent promise rejections

[serge-ivo]

Problem

The latest VCQA scan reports substantial error-handling debt.

Scan context:

• Command: pnpm vcqa
• Error-handling score: C 63/100
• 215 issues

Dominant findings:

• 63x empty catch blocks.
• 29x .catch(() => {}) silently swallowing errors.
• 7x JSON.parse of external data without try/catch.
• 4x process.exit() in library code.
• 3x floating promises.

Representative files from the report:

• packages/browser-runner/src/coding/inspect.ts
• packages/cli/src/commands/up.ts
• packages/sdk/src/client.ts
• packages/sdk/src/voice/config.ts
• packages/sdk/src/voice/cues.ts

Acceptance criteria

• Classify each empty/silent catch as intentional best-effort, expected optional behavior, or real error path.
• For intentional best-effort paths, add narrow comments and structured/debug logging where useful.
• For real failures, propagate errors or surface them to the user/operator.
• Avoid leaking sensitive details in user-facing error messages.
• Add focused tests for at least the highest-risk SDK/CLI/browser-runner paths changed.
• pnpm vcqa error-handling issue count drops materially without suppressing real failures.

[serge-ivo]

packages/** slice — done (a47bb43). Leaving this open for workers/api, workers/mcp and store/**.

Three agents hold the other slices; this comment covers only packages/browser-runner, packages/cli and packages/sdk.

The test used, which is not "count the empty catches": does swallowing this let the system continue in a state that is wrong, or merely degraded? A dropped screenshot is degraded. Of ~110 sites in these three packages, 6 were wrong. The rest are deliberate and documented, and changing them would be the damage — so the raw finding count here drops by roughly that much and no further.

Changed — and what each would have let the system do wrong:

1. sdk/voice/config.ts — a failed load became the cached config. resolveVoiceConfig({}, false) is not an error value, it is a full plausible config. One blip silently reconfigured voice for the whole session: language → en-US, so confirmLanguage rejects the user's real language as a mis-detection and nothing they say is ever sent; Whisper → browser dictation; every control word — including "mute" — → empty. And refresh: "background" revalidates behind a cache that is already correct, so a blip replaced settings that had loaded seconds earlier. Now: defaults for that call only, known-good cache preserved, nothing cached, failure → durable error log (list_errors).
2. sdk/voice/stt.ts — the dictation half of [bug] Stop swallowing runtime errors across voice, coder, and worker paths #291. That commit fixed the Whisper recorder; the browser-recognizer branch of the same stop() kept its bare catch {}. A failed stop() leaves SpeechRecognition running and listening = false hides it (it only suppresses a restart that a failed stop never triggers) — the mic kept capturing after the user turned voice off. Falls back to abort().
3. sdk/voice/gate.ts — same shape, second live recognizer. The gate runs alongside the Whisper recorder; same fix.
4. cli/publish.ts — "Published!" when nothing was pushed. git push with nothing to push exits 0, so the catch only ever ran on a real failure, and the one thing it printed ("Push skipped (up to date or no commits)") was false every time it fired. The agent's code never left the machine while the CLI said it had shipped. Now reports the push error and exits 1. Malformed agent.json also names the file instead of throwing a bare SyntaxError (one of the 7x bare JSON.parse).
5. cli/up.ts — r restart failed silently and exited 0, telling the shell the runner had restarted while nothing was running.
6. cli/runner/relay.ts — a silent heartbeat failure. The heartbeat is the only thing keeping the console's runner badge online, so once it fails the console says "runner offline — run pags up" while that window still says CONNECTED. The documented remedy for that banner is pags up --force, which suspends coding sessions on other machines — the silence steered the user toward a destructive action. Logged once per state change, not per beat.

Deliberately kept (~85–90% of this class, as the earlier pass found): recordUsage/recordVoiceUsage, the voice STT/TTS/upload reports to the durable log, reaper.ts and the throwaway-profile cleanup, mcp-runtime's bounded teardown, browser-profile seeding, screenshots and read-back feedback, the headless resume-id store, store.ts's corrupt-file fallback, the audio cues. Each is best-effort by design and already carries the comment saying so.

Tests: 8 new, all pure unit — no new load on the browser-runner integration project. Each asserts the specific regression (cache survives a failed revalidate; failed load self-heals; recognizer aborts when stop() throws — plus the inverse, that a clean stop is not aborted; push failure fails the publish).

Shipping: @proagentstore/cli 0.4.37 — verified by unpacking the tarball, not by the green check (npm pack shows all three CLI fixes and a fresh bundled runner). The SDK voice fixes ride the console deploy, since packages/sdk/** is a deploy-host trigger path. CI, Deploy Host, Deploy API and Publish-to-npm all green on a47bb43.

[serge-ivo]

Slice: workers/mcp — 18 sites, 2 wrong, 16 left alone

Same discriminating question the earlier slices settled on: does swallowing this leave the system wrong, or merely degraded? A dropped metric is degraded. LocalRunner.close(), VoiceStt.stop(), fetchVoiceConfig and cli/publish.ts were wrong. This slice found two of the same kind.

I inventoried every error-handling site in workers/mcp/src (non-test): 15 try/catch, 2 inline .catch(), and 1 discarded result, which turns out to be the more dangerous shape in this worker.

Left alone — 16 of 18 (89%)

Deliberate, comment-documented best-effort (8). Changing these is the damage, not the fix.

Site	Why it stays
suspension.ts:42	/v1/auth/me unreachable → fail open, so an API blip does not take the whole MCP surface down. The API still enforces suspension on every real call.
index.ts:113 userGroups()	Auth/transient failure → no surface-gated tools this connection. Documented in code and in workers/mcp/CLAUDE.md's gotchas.
http.ts:44	Non-JSON body → {raw}, and a non-2xx still becomes {error: "API <status>"}. The failure is never lost.
safety.ts:130	Corrupt audit record → returns {raw}. Preserving an unparseable audit row beats dropping it.
session.ts:42	Malformed session payload → null. Fails closed.
repo-tools.ts:89	Non-JSON GitHub body → {raw}, ok/status preserved.
oauth-provider.ts:316, :317	Exchange fetch/parse failure → no session → 400. Fails closed.

The audit/observability paths behaved as expected here — audit() is awaited at every one of its call sites, and no floating promises exist in this worker.

Already surfacing the failure — not swallows at all (8). oauth-provider.ts:147, the four JSON.parse guards in storage-tools.ts, apply.ts:42, board.ts:37 (whose comment says it outright: "Surface a real failure instead of returning an empty board, which reads as 'no jobs'"), and index.ts update_agent.

Fixed — 2

1. remove_repo discarded its authedCall result (instance-tools/repo.ts).

authedCall/apiCall return {error: "API <status>"} on a non-2xx rather than throwing — so discarding the result is a silent success claim, and this was the only one of 128 authedCall sites in the worker that did it.

What the system did wrong: a failed clear answered "Removed all repositories." and wrote action:"completed" to the audit log — the one record an operator consults to decide whether a destructive removal actually happened. Up to 20 repos' vectors survive and keep being retrieved, so the caller's next question is answered from an index it has been told is gone, and the audit trail agrees with the lie. Every sibling mutating tool already checks this; coding_session_message even carries the comment explaining why ("do NOT report 'sent' for a command that never ran").

2. create_agent collapsed a malformed JSON argument to undefined (index.ts).

Accepting a JSON string where the schema says object is deliberate — models send one, and rejecting it turns a working call into a retry loop. But the swallow was wider than its rationale: a string that is not valid JSON became undefined.

What the system did wrong: capabilities is what makes this "a fully-formed agent in one call". Undefined is silently the plain chat agent the tool's own description promises you avoid — no surfaces, no runtime, no tools[] allowlist — created, and reported as Created: <id>. update_agent, thirteen lines below, already refused exactly this input. The coercion now lives once in http.ts as parseJsonArg + INVALID_JSON and both call sites share it, which is where the divergence was possible in the first place: two hand-written copies that agreed on the happy path and disagreed on the malformed one.

Notes

• No swallow in this worker was a candidate for the durable error log — workers/mcp has no D1 binding (it is a separate deployable and cannot import from workers/api), and the sites that are silent are silent deliberately, on paths where the caller is already told something true.
• 5 regression tests added, driven through the real safety helpers and real authedCall with only fetch and KV stubbed. The remove_repo pair asserts both halves: a failed clear must not be reported and must not write a completed audit event.
• workers/mcp/CLAUDE.md now records the trap: the doc already said apiCall never lets a non-2xx pass as success, which is true only if the caller looks.
• instance-tools/contract.test.ts (all 86 instance tools, gates derived by driving each handler) and pnpm docs:drift (130 tools, exact match) both pass unchanged — no gate moved.

Leaving this open for the remaining trees.

[serge-ivo]

Slices: workers/api (12 of ~619) and store/** (5 of ~105) — closing

Same discriminating question the three earlier slices settled on: does swallowing this leave the system wrong, or merely degraded? A dropped metric is degraded. LocalRunner.close(), VoiceStt.stop(), fetchVoiceConfig, cli/publish.ts and remove_repo were wrong. These two trees held 17 more of the same kind.

The sharpest shape, again: a swallow that falsifies a record someone later consults. It is strictly worse than one that only misleads in the moment, because the misleading version is corrected by the next page load and this one is what an operator reads afterwards to decide what happened.

workers/api — c5d9f82

~619 error-handling sites inspected (338 of them literally empty catch {} or .catch(() => …) no-ops; the rest catches returning a fallback). 12 wrong.

The headline is not a swallow that misreports — it is one that destroys user data and answers 200:

lib/credentials.ts. The #220 guard is narrower than the hazard its own comment documents. It fires on a MISSING key; a key that is present but WRONG — a rotated KEK, a damaged DEK/IV, an unparseable plaintext — failed inside decryptSecrets, which returned {} for both "no secrets stored" and "could not decrypt". So the merge in updateCredential saw "there was nothing here", encryptSecretsFor had nothing to encrypt, and the UPDATE wrote NULL over intact ciphertext — returning true, so the API answered 200 "saved" to a metadata edit that destroyed the user's ATS password. The two are now different answers: update and reveal refuse, listCredentials stops reporting "no password stored" about a row that has one (which is what invited the edit), and findCredentialForHost returns nothing instead of {password: undefined} — the "hasStoredLogin: true but the login doesn't work" run-burner — without stamping last_used_at for a use that never happened.

The other eleven:

Site	What the system did wrong
agent-do.ts reap	Documented idempotent because "the marker is deleted with the notice" — but the delete did not depend on the append. A failed write left live sockets showing the interruption, nothing in the transcript, and no marker to ever re-emit it: on reload, a question with no answer and no explanation. The #251 bug, restored by the swallow meant to be harmless.
workflows/job-apply.ts	The ONLY write that closes an apply task, best-effort, twenty lines under the comment explaining nothing else ever will. Lost → a permanently running board card with a Cancel button owned by nothing, and a single-flight claim that 409s every application for 4h. It is a workflow step; letting it throw makes it retry.
routes/coding.ts /signin	The board card written directly beneath "The board card MUST exist before the handoff", swallowed — along with the handoff. Card lost → the runner's if (task) is false → no needs_human, no event, nothing on the Board, while the route answers ok:true and authPromptGuidance tells the owner to open a takeover view that does not exist.
routes/coding.ts /resume	The human's answer to a blocked engine (a 2FA code) reported as delivered and discarded. The Pilot keeps polling, never sees resolved, closes the run "not resolved in time" — recorded as the human's timeout when the human did answer.
coding.ts + coding-brains.ts watcher stamp	Losing it does not cost a watcher, it mis-attributes one: the column still names the previous send, the new watcher stands down as "superseded", and the stale one announces "✅ Coder finished" against the earlier instruction.
coding-brains.ts delegation	The claim is ordered "so a refusal doesn't leave a board card announcing work that never started"; this is the mirror, and it was silent. No card, but the Pilot starts and the thread states "tracking on the board" as fact — a real run spending the user's tokens with nothing to watch or stop it from.
routes/instances-tasks.ts	#218 fixed the addressing and kept the swallow, so the other half of its symptom survived: a failed cancel still deleted the card and answered ok:true. Deleting the only handle to a live process is not best-effort.
routes/instances-apply.ts	The only way to remove an uploaded CV answered {ok:true} whether or not R2 obeyed. A deletion endpoint that cannot fail is a retention record that lies.
lib/coding-session-sweeper.ts	"Offline machine, nothing to stop" and "connected machine, /coding/end failed" were reported identically, and the timeline asserts the second never happens: it told the human "the engine process was released" about a child process still running on their laptop — the exact leak the docstring excludes, written into the session's own durable record.
lib/coding-session-open.ts	The re-attach exists so the reused path is not "quietly driving a dead pane", and both reuse branches discarded its answer twice over (throw caught, null return unread) and returned ok:true anyway. loop-drivers reads that as a live engine: claims the driver, opens the run row, bills the Pilot's reasoning against a pane that never launched.
lib/usage.ts	The one read in that file that is not observability, and the only one without the comment saying so. Read twice per iteration for after - before; a swallowed failure returned 0 and booked the iteration as free — reservation released, tree pool never depletes, the runaway cap inert, spent untrue. Its own docstring calls under-charging the unsafe direction. agent-loop.ts:201 already writes .catch(() => held.spendBefore), which only makes sense if this can reject — it could not, until now.
lib/runner-client.ts	A failed pin read became "", which means unpinned, so one D1 blip took the fallback the contract two lines below forbids: run the agent on a machine the user did not choose, in a different checkout, and report that machine online.

store/** — 9007758

~105 sites, 5 wrong. The console's version of the shape is a mutation that fails while the control keeps showing the new value, because the control IS the claim.

• "Use my defaults" swallowed its DELETE and moved the radio anyway — so the override stays live on the server, the screen says "Using your defaults", and the summary beside it renders the override's own values as the account defaults: the two halves of the lie corroborating each other. invalidateVoiceConfig() then guarantees the next mic open re-applies the settings the screen just said were gone; for translation, use-gloss re-reads on every return to the chat, so the chat visibly contradicts the settings page. Fixed in PrefOverride, which owns the radio: it awaits, leaves the control where it truthfully is, and says so.
• Browser alerts. enablePush() returns "error" for precisely the case where permission WAS granted and the subscribe/save failed — and the result was discarded. The card keys on pushPermission(), so that case reads as granted: "On — you'll get a notification (even with this tab closed)" plus "an agent waiting on you always gets through", with the Enable button replaced so nothing was left to retry — while sendPushToUser had no subscription to fan out to. Layout.tsx already reads this return value; this call site was the deviation.
• sendTestPush returned Promise<void> after swallowing, so the caller could not tell even if it wanted to, and rendered "Sent ✓" either way. This is the diagnostic control for the bug above — the one button a user presses to check push works. A false ✓ sends them to debug their OS notification settings for a failure at the platform's end.
• stopLoop cleared loopOn/loopRunId on a failed cancel — indistinguishable on screen from a stop that worked. Clearing loopRunIdRef also kills the watcher, so the run kept iterating and spending with no UI trace and no way to re-attach. pollLoop's own catch refuses to do this and states why ("a transient read failure must not kill the WATCHER — the run itself is durable"), so two catches in one component contradicted each other.

Deliberately kept

recordUsage/recordVoiceUsage ("observability, never load-bearing"), isSuspended failing open so a D1 blip cannot 403 the platform, the logEvent/logError/appendTimeline/notifyUser bridges, recordIteration, the screenshot STORAGE.puts, the three opportunistic retention DELETEs, every req.json().catch(() => ({})), relay-do.ts's socket liveness probes, pipeline-run.ts's closeRun("interrupted") (the retry re-closes it terminally), and agent-think.ts's prompt-assembly blocks. On the console: every mount-time loader that falls back to a rendered default, poll reads that keep the last good value, localStorage, clipboard, R2 part cleanup, lazy glosses — and all ~15 bare catches in SettingsTab's load effect, whose save handlers already revert and report. Changing these is the damage, not the fix.

Notes

• The vcqa count is not the target and was never going to move much — three quarters of these trees' swallows are correct and carry the comment saying so. What moved is that no swallow left in workers/api or store/console now lets a record, a board, an audit trail or a control assert something that did not happen. lib/error-log.ts was available here (it was not to workers/mcp, which has no D1) and is used where a swallow is correct but was silent — an undecryptable credential now reports itself to list_errors.
• 13 regression tests, each asserting the specific falsehood rather than the presence of a catch: ciphertext survives a wrong-KEK edit and the three reads stop under-reporting; the timeline stops claiming a stop that failed; a reused session with a dead engine stops reporting ok; a ledger read failure is not a spend of zero; an unreadable pin does not route elsewhere; a rejected R2 delete does not report the résumé gone; a rejected test push does not render "Sent ✓".
• Both pinned files touched (agent-do.ts, SettingsTab.tsx) are line-neutral — no pin was raised. In SettingsTab that was the better design anyway: pushing the failure into the component that owns the radio removed two lines instead of adding a message-state pair.
• ⚠️ Deploy API Worker is red on main, and not from this work. It broke one commit earlier, at ci(migrations): one number, one migration…: deploy-api.yml checks out at the default fetch-depth: 1 and then runs pnpm test, which now includes migration-hygiene.test.ts shelling out to check-migrations.mjs --require-history. That guard refuses to run against a shallow clone by design. One line — fetch-depth: 0 on deploy-api.yml's checkout — but it belongs to whoever owns that workflow. Until it lands, the API fixes above are merged but not deployed. (Deploy Host Worker is green, so the console fixes are live.)

All four slices are done, so closing.