fix(api): redact P4 sync errors, restore merge on failure, close audit gaps

[tatelilith]

What

Six defects surfaced by a clean-code audit of the largest API modules. Each was verified against the surrounding code before being changed — in every case a sibling code path in the same file already did the right thing, which is what makes these bugs rather than style debt.

Credential leak — Iron Rule 5

executeP4Sync returned p4d's raw stderr, which echoes the connection string back on failure including P4PASSWD. checkP4Connection and the pre-sync verification catch in the same file both wrap through sanitizeCredentials (which has a dedicated P4PASSWD rule); this path did not. The message is persisted to scmSources.lastSyncError and shipped to the sync-error webhook, so the password left the instance entirely.

Non-atomic writes

• mergeMemoryTopics rewrote the merge target before archiving its sources, with no rollback. A mid-loop failure left the target holding a still-active source's facts, and the retry then failed permanently with TOPIC_NOT_FOUND. splitMemoryTopic already compensates; merge now does the same.
• importAgentFromZip wrote skill files after its transaction committed. An ENOSPC there left committed skills rows whose storagePath was empty, so later runs silently mounted an incomplete skill package. The writes move inside the transaction, so a failure rolls the rows back.

Audit gaps — Iron Rule 5

POST /agents/:id/regenerate-api-key, POST /agents/:id/regenerate-a2a-api-key, POST /scm-sources/:id/sync and POST /scm-sources/:id/check each mutated state or made an outbound connection with a stored credential while writing no audit entry. /check is the sharpest case: its sibling /probe audits, rate-limits, and pins the endpoint precisely so a stored credential cannot be aimed anywhere.

The sync and rerun entries are now written after their status CAS wins, matching POST /runs/:id/execute, so a request rejected for a full queue no longer leaves an audit entry claiming work that never ran.

Resource leaks

A Feishu topic-root message carrying both images and a file overwrote its single rootTempDir, leaking the image download root (30 MB per attachment) until the disk filled. The three cleanup calls were also fire-and-forget, so a run reported terminal while its directories were still being torn down.

Tests

TDD on the two behavioural fixes: a failing test first, then the fix.

• P4 redaction — asserts the password never reaches result.message
• merge rollback — injects a real EISDIR mid-archive and asserts the target and both sources are restored

Gates

typecheck green · lint 0 errors (749 warnings, unchanged from main) · arch R1–R9 pass · full suite 8401 passed, 0 failed

Not included — two known P0s remain open

1. Per-Agent worktree bypasses the removal-reservation protocol (agent-helpers.ts, resolvePerAgentWorkspace). task-queue-db.ts defers the check with the comment "Runs without an explicit name are gated later, at resolveWorkDir" — that gate does not exist. The only findPendingWorkspaceRemoval call in agent-helpers.ts is on the explicit-worktree path. An admin workspace delete can therefore run git worktree remove while an incoming message resolves the same directory.

I implemented the fix and reverted it: the added query desynchronised the mockReturnValueOnce queues in agent-helpers-workdir.test.ts, taking it from 39/39 to 15 failures (confirmed against a clean baseline — the failures were mine, not pre-existing). Fixing it properly means reworking that file's mock sequencing, which deserves its own PR rather than riding along here.

2. The two mutex keys for one directory are still inconsistent. agent-helpers.ts takes workspace:${wsPath} while git-workspace.ts takes scm-worktree:${join(wsRoot, name)}, so the locks do not exclude each other. They cannot simply be unified: withKeyedLock is not reentrant and both createWorkspace and removeWorkspace take the git-workspace key internally, so unifying deadlocks. This needs a lock-free internal variant.

Both are filed here rather than silently dropped; neither is made worse by this PR.

Review notes

• Not fixed, deliberately: POST /agents and PATCH /agents/:id resolve scmSourceId with no owner filter, while kbDocuments and skillGroups three lines away are owner-scoped. The hole is real, but exploiting it requires an authenticated colleague deliberately targeting another's resource — the exact case iron-rules.md says not to harden against. Worth fixing as consistency with the adjacent validators; flagging for a maintainer call rather than deciding unilaterally.
• pnpm lint locally reports thousands of errors for anyone who has run Playwright. They all come from playwright-report/, which is gitignored but not excluded in biome.json files.includes. CI is unaffected (no artifact). Not fixed here to keep this PR scoped, but it is a one-line addition next to the existing test-results / coverage entries.
• E2E (scripts/e2e/restart-recovery.sh) was not run. Reverting the worktree change removed the reason it mattered, but it is worth a pre-merge run.