fix(api): explicit scope, audit log and atomic sweep for DELETE /v1/batches/delete-completed (omni-code-review battery on #12969)

[diegosouzapw]

Findings surfaced by the /omni-code-review battery over PR #12969 (fix/batches-delete-completed-authz, 3 files) reviewed against release/v3.8.51 — 3 Opus reviewers (two-axis code-review, pr-review-toolkit:silent-failure-hunter, quality-scan), 12 raw findings → 10 ledger entries, owner-approved plan. Ledger and per-reviewer reports: _tasks/reviews/omni-code-review/2026-09-10_test-omni-pr12969_vs_release-v3.8.51/.

Resolved

• LEDGER-4 (medium, src/lib/db/batches.ts + route) — the instance-wide sweep was the helper's default (deleteCompletedBatches() / falsy id). New explicit scope { apiKeyId } | { allTenants: true }; a missing/empty id throws instead of widening; the route branches on scope.isSessionAuth explicitly (absorbs LEDGER-5 and LEDGER-6). Tests: batches-delete-completed-ownership-wvxc.test.ts, batch-deletion.test.ts (callers migrated, assertions unchanged).
• LEDGER-1 (medium, test-gap) — route-level regression test tests/unit/batches-delete-completed-route-scope.test.ts through the real handler (real createApiKey keys + jose session cookie): key A cannot sweep key B's batch; a session (even with a bearer key) sweeps the instance; no creds → 401; injected failure → sanitized 500 with nothing deleted. Flip proof: forcing the route to { allTenants: true } fails the test.
• LEDGER-2 (medium, correctness → owner decision: keep strict) — batches with api_key_id IS NULL stay out of a key-scoped sweep on purpose (diverges from scopeCheck in the single-batch route); documented in the JSDoc and pinned by a test.
• LEDGER-3 (medium, security) — every sweep is logged (warn for instance-wide, info for key-scoped: route, mode, apiKeyId, counts); the helper call is wrapped and a failure returns buildErrorBody(500, …) (Hard Rule fix(ui): fix Select dropdown dark theme inconsistency #12) — no stack, no raw SQLite text.
• LEDGER-8 (low, security → owner decision: log only) — the empty catch around deleteFile now logs deleteCompletedBatches: file soft-delete failed with the file id; counting unchanged. Scoping the file half by owner is deliberately left for a follow-up.
• LEDGER-9 (low, correctness) — the SELECT, file soft-deletes, checkpoint DELETE and batches DELETE run in one db.transaction (present on all four adapters); test injects a RAISE(ABORT) trigger on the final DELETE and asserts the file content rolls back.
• LEDGER-10 (low, test-gap) — both test files set a temp DATA_DIR before loading @/lib/db/* (top-level await import), so running them without the isolateDataDir shim never touches ~/.omniroute (verified by sqlite mtimes).
• LEDGER-7 (low, docs) — changelog.d/fixes/12969-batches-delete-completed-ownership.md.

Round 2 (re-review of the fix diff, 3 lenses) — resolved

• Key-first precedence — a request carrying BOTH a dashboard session cookie and an API key was escalated to an instance-wide sweep (session-first), while GET /v1/batches is key-first. Now a presented key always scopes the sweep; only a session WITHOUT a key sweeps the instance (parity with list/count, least privilege for a destructive path). Test: "a request carrying BOTH a session cookie and an API key is scoped to the key".
• Mixed scope { apiKeyId, allTenants: true } silently widened — now rejected ("mutually exclusive") + test.
• Audit at info vanished under APP_LOG_LEVEL=warn — both sweep modes log at warn.
• Failure log carried only err.message — now { message, stack }.
• Changelog fragment links the PR; the ownership test pins APP_LOG_LEVEL so its log assertion is environment-independent.

Acknowledged / owner decisions (not in this PR)

• LEDGER-5, LEDGER-6 — duplicates of LEDGER-4.
• File half of the sweep is not scoped by owner (round 2 raised it to high with an executed probe: an unowned file attached to key A's completed batch and to key B's in-progress batch has its content nulled by key A's sweep). Owner decision at P3: log-only in this PR; a follow-up should filter fileIds by api_key_id = ? for key-scoped sweeps (NULL-owned files left to allTenants).
• Partial file-soft-delete failures are logged but not surfaced in the response (owner decision: log only; follow-up: filesFailed in the body).
• Single transaction holds the write lock for the whole instance-wide sweep — follow-up: chunk the allTenants path.

Validation

• Focused: ownership-wvxc 7/7 · route-scope 4/4 · batch-deletion 8/8 · batch-deletion-route-logic 11/11 · dashboard list-regression.test.tsx 20/20 (vitest).
• Full suite on 43aba7dd (round-1 fix) on the 32-core box: test:vitest 51 files ✅ · build ✅ · test:unit 37 704 pass / 42 fail — 0 related to batches; all 27 failing files fail IDENTICALLY on the PR head 3355012 (base-vs-fix discriminator, same box) → pre-existing on the branch, not introduced here.
• Round-2 delta (becb5954, c82c7bed, 5 files): focused suites 31/31 ✅ (route-scope 5, ownership-wvxc 7, batch-deletion 8, batch-deletion-route-logic 11), eslint/prettier/typecheck ✅.
• Gates on the fix worktree: eslint(changed) · prettier · typecheck:core · check:dashboard-typecheck · file-size · complexity · cognitive-complexity · test-discovery · db-rules · error-helper ✅; changelog-integrity ✅ against this PR's base (the local fallback compares with release/v3.8.51, 44 commits ahead of the source branch).
• Re-review rounds: 3 (max). Round 3 (over the round-2 fixes) surfaced one real gap, fixed in 62a3c815 and validated by focused tests (33/33) + eslint/prettier/typecheck, not re-reviewed (round cap):
  • fail closed on an unresolvable key — a presented API key the DB does not know (deleted/rotated/mistyped) together with a session cookie fell through to the instance-wide branch; now 401 Invalid API key, nothing deleted (test added).
  • no-op key-scoped sweeps log at info (instance-wide and any sweep that removed rows stay at warn); JSDoc documents the mixed-scope rejection; the route-test docblock matches the shipped precedence; mixed-scope test split into its own case.
  • Acknowledged (follow-up): persist the sweep audit through the compliance audit sink instead of the app log level; a route-level assertion on the log line is not feasible here (pino worker-thread transport, mock.module unreliable in this harness).

⚠️ base-red inherited: #12732 — ESLint errors ×3 · Docs sync strict (README migrations count) — pre-existing on release/v3.8.51, not introduced here.