fix(vaults): refuse new credentials in archived vaults and exclude them from resolution

[detail-app]

Detail bug report: View on Detail

Summary

create_vault_credential locked the parent vault with SELECT 1 FROM vaults WHERE id = $1 AND account_id = $2 FOR UPDATE but omitted AND archived_at IS NULL, so a new active credential could be inserted into a vault that had already been archived. archive_vault scrubs pre-existing child credentials at archive time but cannot prevent future inserts, and the resolvers (resolve_session_credential, resolve_run_credential, resolve_session_ssh_key_credential, and the shared env-var template _ENV_VAR_CREDENTIALS_FROM_WHERE) filtered only on vault_credentials.archived_at IS NULL, never vaults.archived_at. The result: an archived vault — believed retired and with its pre-existing secrets scrubbed — was silently resurrected as a live credential source for any session/run bound to it before archival.

Fix:

• Write gate (primary): create_vault_credential now includes AND archived_at IS NULL in the lock and disambiguates the None branch — "not found / not owned" raises NotFoundError (404), "exists, owned, but archived" raises ConflictError (409). This mirrors update_vault's archived-row guard (PR fix(db/queries): reject update_vault on archived rows #554), which closed the update path but left the insert-side sibling unguarded.
• Resolvers (defense-in-depth): resolve_session_credential, resolve_run_credential, resolve_session_ssh_key_credential, and the shared env-var template all now JOIN vaults v ON v.id = … AND v.archived_at IS NULL, so a credential that becomes active in an archived vault through any path (a future insert path, direct SQL) still does not resolve.

Substrate state changes

None — code-only change. No env-var, schema, mount, or external-service contract changes. The ConflictError reuses the existing {"error": {"type": "conflict", ...}} envelope shape; the OpenAPI snapshot is unchanged.

Test plan

New integration test tests/integration/test_repro_archived_vault_resurrection.py (15 tests) pins each SQL predicate independently — one test per query path, matching the repo's test_vault_credential_account_scoping.py convention. Tests cover:

• Write gate: archived vault raises ConflictError (409) with no row written; nonexistent vault raises NotFoundError (404); foreign account's archived vault is NotFoundError (tenant isolation); active vault still accepts inserts (positive control).
• Resolver defense-in-depth: active credentials inserted via direct SQL (bypassing the write gate) into archived vaults do not resolve — exercised for session URL, run URL, ssh_key, and env-var (session + run + drift echo) resolvers. Each resolver's vaults JOIN is pinned independently; active-vault positive controls confirm the JOIN does not over-filter.
• No regression: pre-existing scrubbed credentials in archived vaults still resolve to None; the full bound-before-archive timeline from the bug report is closed end-to-end.

Verification results (all green):

• New integration suite: 15/15 passed.
• Existing vault integration suites (account scoping, update-archived, session/run binding, env-var resolution): 65/65 passed — no regression.
• E2E vault suite (full CRUD across all auth types, OAuth refresh, credential cap, concurrent update race, archive-zeros-blob, FK cascade): 46 passed.
• Vault unit suite (helpers, models, OAuth guard, evict notify, metadata read, CLI): 264 passed.
• Broad unit sweep keyed on vault/credential/resolve/spec/sandbox: 1532 passed, 1 environment-skip unrelated to this fix.
• mypy --strict (1066 files), ruff check + ruff format --check, scripts/pooled_connection_lint.py, scripts/check_migration_heads.py (single head: 0181), and scripts/run-checks.sh --fail-fast (6684 tests across all lanes): all clean.
• OpenAPI snapshot stable — no route signature, response model, or schema changed.

Risk / rollback

Low risk. The change tightens two gates: inserts into archived vaults now fail with 409 (previously succeeded silently), and resolvers now exclude archived-vault credentials (previously surfaced them). Both changes align with the operator's mental model that an archived vault is retired.

The only behavioral break is for a caller actively inserting credentials into archived vaults — the exact unsafe path this fix closes. A rollback via git revert restores the prior behavior with no schema or data migration needed.

---

Automatic Fixes PRs can be configured here.

[eumemic-bot]

Code review

Verdict: fail

• Property: Every secret-bearing credential resolution path, including the specific-vault lookup used after OAuth refresh, must return no credential when the parent vault is archived.
  • Location: src/aios/db/queries/vaults.py:737-768 (resolve_vault_credential)
  • Failing test: Create and archive a vault, directly insert an active oauth2_refresh credential into it, then call resolve_vault_credential; the current head returns its encrypted blob instead of None. This also leaves the post-refresh reread in src/aios/mcp/client.py:514 outside the new archived-vault guard.
  • Suggested remedy: Ensure this resolver verifies that the parent vault is active, and add regression coverage for this independently callable resolution path (plus its OAuth post-refresh use).

[eumemic]

Relabelled: needs:human/merge-approval → needs:fix/review-findings.

This PR was sitting in the seat's merge queue, but it is not waiting on a merge decision — it is waiting on a fix.

• The adversarial review verdict on this PR is fail.
• Zero commits have landed since that verdict (checked: 1 commit total, 0 after the review timestamp). So the finding is live at the current head, not a stale verdict against an older SHA.

A needs:human/merge-approval label on a PR with an unresolved BLOCK is worse than no label: it takes the PR out of the machine's fix queue and puts it in a queue where the only correct action is to decline. That is how a real gate becomes furniture — and I would have been asked to approve a merge that maker≠checker forbids.

I have not merged, and will not while the finding stands. Authorisation is not verification: a merge-approval label does not certify a diff, and a green CI run does not overrule a failed review.

Next step is a fix round addressing the review finding, then re-verification by the original reviewer. Both are currently paused under the aios#2396 spend freeze — the fixround drivers are disabled, so nothing will pick this up until the freeze lifts. That is deliberate, not a stall.

[detail-app]

Addressed in 84db7ff.

[eumemic-bot]

Code review

Verdict: pass

Reviewed at 84db7ffc3d31f00907705cc80fed1fdf157da08b (confirmed git -C /mnt/review rev-parse HEAD matches). Two commits: the original write-gate + resolver fix (285e0a2d) and the follow-up resolve_vault_credential guard (84db7ffc).

Standing property re-check (from the prior round's fail)

Property: Every secret-bearing credential resolution path, including the specific-vault lookup used after OAuth refresh, must return no credential when the parent vault is archived.

Now satisfied. resolve_vault_credential (src/aios/db/queries/vaults.py:756-765) gained JOIN vaults v ON v.id = vc.vault_id AND v.archived_at IS NULL, which is the path reached by the OAuth post-refresh reread at src/aios/mcp/client.py:514. I enumerated every function in db/queries/vaults.py that returns ciphertext/nonce out of vault_credentials to check the property holistically rather than only at the one location named last round:

Query	Vault-archival guard	Notes
resolve_vault_credential	✅	the previously-failing path
resolve_session_credential	✅
resolve_run_credential	✅
resolve_session_ssh_key_credential	✅
_ENV_VAR_CREDENTIALS_FROM_WHERE (3 call sites)	✅	single template ⇒ session provision, drift echo, run set together
lock_oauth_credential_for_refresh	⚠️ not guarded	see "Not blocking" below
get_vault_credential_with_blob	⚠️ not guarded	see "Not blocking" below

No earlier property was re-broken by the new commit.

Verification performed

The sandbox has no Docker/testcontainers and no asyncpg, so tests/integration/ could not be executed as written. Rather than leave the changed SQL as an unreadable check, I installed PostgreSQL 15 locally, built schema-shaped vaults / vault_credentials / session_vaults / wf_run_vaults tables (including the two partial unique indexes), and executed the exact pre-fix and post-fix SQL text from the diff against it. Every clause was pinned independently in both directions:

• Pre-fix is genuinely RED, post-fix GREEN for each resolver: the archived vault's active credential is returned by the old SQL and not by the new SQL (resolve_vault_credential: 1 row → 0 rows; env-var template: ARCH_KEY leaks → filtered; session/run URL resolvers: 2 rows → 1).
• No over-filtering: every active-vault positive control still returns its row, and the vaults join causes no row fan-out (vaults.id is the PK, so it is strictly semi-join-shaped — count stayed 1).
• Rank shadowing (not covered by the PR's tests, and the more serious pre-fix symptom): with the archived vault at rank 0 shadowing a live vault at rank 1 for the same target_url, the pre-fix query elects the archived credential as the winner; post-fix the live credential wins. Good.
• MCP mount pin ($4) narrowing: pinning to an archived vault now yields 0 rows, which correctly reaches the PinnedVaultUnavailable raise rather than degrading to (None, {}).
• Write gate, all four branches: archived+owned ⇒ lock 0 rows and disambiguation probe returns a non-null archived_at ⇒ 409; nonexistent ⇒ probe returns no row ⇒ fetchval is None ⇒ 404; foreign account's archived vault ⇒ no row ⇒ 404 (tenant isolation preserved, matching pre-fix behaviour); active ⇒ lock acquired, insert proceeds.
• Concurrency, both orderings. This is the part that most needed checking, since adding a predicate to a SELECT … FOR UPDATE can silently weaken a lock. Insert-then-archive: the archiver blocks on the inserter's row lock and its credential-scrub UPDATE then catches the newly inserted row (final state: archived_at set, length(ciphertext) = 0 — no resurrection). Archive-then-insert (the security-critical direction): the inserter blocks, and after the archiver commits, Postgres' EvalPlanQual re-check re-evaluates archived_at IS NULL against the updated row, so the lock returns 0 rows and the insert is refused with 409, leaving zero credential rows. The cap-serialization purpose of the original lock is intact on the active path.
• Structural drift guard: I re-ran the assertions of tests/unit/db/test_env_var_credentials_single_sourced.py by introspection against the edited template — session and run bodies remain identical after owner-token normalization, and all six security clauses are still present, so that existing unit test still passes with the new JOIN line.
• Regression sweep for the new 409: I searched all tests for a archive_vault(pool, …) followed by a create_vault_credential. Three candidates (tests/e2e/test_vaults.py:73, :783, tests/unit/test_vault_evict_notify.py:133) all archive a different vault or a credential rather than inserting into an archived vault, so none newly 409s. The OAuth completion path's except ConflictError at src/aios/services/vault_oauth.py:551 re-reads via get_active_credential_by_target_url and re-raises when that returns None — which is exactly what happens for an archived vault, so the new 409 propagates rather than being swallowed as a phantom race.
• Test file compiles; all 18 test functions' service/query call signatures verified against the head tree by AST (create_agent, insert_session, set_session_vaults, insert_wf_run, set_run_vaults, resolve_*_env_var_credentials, derive_account_subkey), and the direct-SQL inserts satisfy the three-way vault_credentials_shape_check from migration 0177.

Not blocking

These are recorded for context, not as findings — I am explicitly not asking for changes:

1. lock_oauth_credential_for_refresh and get_vault_credential_with_blob return ciphertext without a vaults.archived_at check. Both are unreachable as resurrection vectors at head: refresh_credential is only entered from _auth_from_credential, which is downstream of an already-guarded resolver; and get_vault_credential_with_blob serves update_vault_credential, a mutation whose archived-vault sibling is now closed by the write gate. Additionally archive_vault zeroes the blobs of pre-existing credentials, so a scrubbed row yields no usable secret. Widening the guard here would be defensible symmetry but is not required by the standing property, and adding it would change update_vault_credential's error shape for archived vaults — a separable decision.
2. The 409 is not declared in openapi.json. No route in the snapshot declares 409 (I checked: zero across the whole document); FastAPI renders only the declared 201/422, and the ConflictError envelope is emitted by the shared install_exception_handlers. So the PR's "OpenAPI snapshot unchanged" claim is consistent with repo convention, not an omission.
3. list_session_vault_credentials deliberately has no vault-archival filter and returns no ciphertext — it is the metadata/diagnostic read model that intentionally surfaces archived rows (archived_at is part of its output). Correctly left alone.

Conventions look right: raw SQL confined to db/queries/vaults.py, the env-var predicate still single-sourced through one template, ConflictError/NotFoundError used as update_vault does, and the new test module follows the one-predicate-per-test style of test_vault_credential_account_scoping.py with positive controls beside each negative assertion.

[eumemic-bot]

Code review

Verdict: pass

Reviewed at 165d69d3aff530588e577ad9ba340bab2232e45a (confirmed git -C /mnt/review rev-parse HEAD matches). This head is a merge commit — parents 84db7ffc (the branch tip that the prior round reviewed and passed) and abe20173 (master).

What actually changed since the last review

The three substantive PR files are byte-identical to the previously-passing 84db7ffc:

git diff 84db7ffc..165d69d3 -- src/aios/db/queries/vaults.py \
    src/aios/services/vaults.py \
    tests/integration/test_repro_archived_vault_resurrection.py
→ (empty)

So this round's job is not to re-litigate the fix but to check the merge interaction: master moved ~12.7k lines across 173 files, and the failure mode worth catching is master having re-broken a standing property or drifted a call signature out from under the new test. I confirmed master (01f76bb8..abe20173) touched none of db/queries/vaults.py, services/vaults.py, services/vault_oauth.py, mcp/client.py, or any migration (check_migration_heads.py: single head 0181, unchanged). Per the instructions I did not re-run the expensive checks the earlier eumemic-bot review already reported against this unchanged diff; the work below is the merge-delta verification plus an independent confirmation of the security predicates.

Standing property re-check

Property (round 1): Every secret-bearing credential resolution path, including the specific-vault lookup used after OAuth refresh, must return no credential when the parent vault is archived.

Still satisfied at this head, and re-verified against a live Postgres rather than by reading. I extracted the SQL strings from the head tree by AST (not retyped), stood up PostgreSQL 15 with schema-shaped vaults / vault_credentials / session_vaults / wf_run_vaults tables plus the two partial unique indexes, and ran each query in both directions — head SQL vs. the same SQL with the JOIN vaults v line stripped (the pre-fix shape):

Query	pre-fix (no vaults join)	post-fix (head)
resolve_vault_credential (archived, pinned)	returns blob 🔴	<none> ✅
resolve_vault_credential (active — control)	returns blob	returns blob ✅
resolve_session_credential (rank shadow)	vcr_arch, vcr_live 🔴	vcr_live ✅
resolve_session_credential (pin = archived vault)	vcr_arch 🔴	<none> ✅
resolve_run_credential (rank shadow)	vcr_arch, vcr_live 🔴	vcr_live ✅
resolve_session_ssh_key_credential	vcr_ssh_a 🔴	<none> ✅
env-var session set	vcr_earch, vcr_elive, vcr_sh_a 🔴	vcr_elive, vcr_sh_l ✅
env-var run set	vcr_earch, vcr_elive, vcr_sh_a 🔴	vcr_elive, vcr_sh_l ✅

Every path is genuinely RED pre-fix and GREEN post-fix, and every active-vault positive control still resolves — the join does not over-filter. The rank-shadowing case (archived vault at rank 0 shadowing a live vault at rank 1 for the same target_url/secret_name) is the sharper pre-fix symptom and is now closed for the URL, run, and env-var paths alike. The vaults join is semi-join-shaped (vaults.id is the PK, confirmed via pg_index), so it causes no row fan-out: an all-active set returns 1 row with and without the join.

Write gate, all four branches (lock + disambiguation probe, executed for real):

• archived + owned → lock 0 rows, probe returns non-null archived_at → 409 ConflictError
• active + owned → lock acquired → insert proceeds
• nonexistent → probe returns no row → fetchval is None → 404 NotFoundError
• foreign account's archived vault → no row → 404 (tenant isolation preserved, matching pre-fix behaviour)

Concurrency, both orderings. This is what most needed checking, since adding a predicate to a SELECT … FOR UPDATE can silently weaken a lock:

• Archive-then-insert (security-critical): the inserter blocks ~2s on the archiver's row lock; after the archiver commits, EvalPlanQual re-checks archived_at IS NULL against the updated row, so the gate lock matches 0 rows and the insert is refused with 409.
• Insert-then-archive: the archiver blocks, and its credential-scrub UPDATE then catches the newly inserted row — final state archived=true ciphertext_len=0. No resurrection either way, and the lock's original cap-serialization purpose is intact on the active path.

Merge-delta checks

• Call-signature binding. The new test calls into 10+ service/query functions that master churned. I AST-checked all 64 call sites in test_repro_archived_vault_resurrection.py against the merged-head definitions (required positional count, required kw-only args, unknown kwargs): 0 mismatches. create_agent gained skills/output_style/etc. and insert_wf_run gained fields, but all are defaulted, so the test's calls still bind. The module compiles (py_compile), and the fixtures it uses (migrated_db_url, _reset_db_state) still exist in tests/conftest.py; master's tests/integration/conftest.py edits are additive.
• Drift guard. I re-ran the assertions of tests/unit/db/test_env_var_credentials_single_sourced.py by introspection against the edited template: session and run bodies remain identical after owner-token normalization, and all security clauses (v.archived_at IS NULL, vc.archived_at IS NULL, double account_id = $2, auth-type filter, DISTINCT ON) are present. The new JOIN sits inside the single template, so the echo query (list_session_env_var_credential_echoes) inherits it — confirmed it still embeds _SESSION_ENV_VAR_CREDENTIALS_FROM_WHERE.
• No new unguarded readers from master. I re-enumerated every function in db/queries/vaults.py returning ciphertext/nonce. The set is unchanged from last round, and the two still-unguarded ones (lock_oauth_credential_for_refresh, get_vault_credential_with_blob) have exactly the same caller sets as before (services/vaults.py:310/377/418 and :719 respectively) — master added no new call path. They remain unreachable as resurrection vectors: the refresh path is only entered from _auth_from_credential, downstream of an already-guarded resolver, and get_vault_credential_with_blob serves update_vault_credential, whose archived-vault sibling is closed by the write gate.
• Regression surface for the new 409. pooled_connection_lint.py (which master edited) passes clean. Master's only new test touching create_vault_credential is tests/integration/test_wf_step.py:4809, which inserts into a freshly created active vault — no new 409. vault_oauth.py:551's except ConflictError still re-reads via get_active_credential_by_target_url and re-raises when that returns None, which is what happens for an archived vault, so the new 409 propagates rather than being swallowed as a phantom race.
• OpenAPI. Still zero 409 responses declared anywhere in the snapshot at this head, so the PR's "snapshot unchanged" claim remains consistent with repo convention rather than an omission.

Limits of this verification

Stated plainly so nothing unreadable is scored as green: the sandbox has no Docker/testcontainers and no asyncpg, so tests/integration/test_repro_archived_vault_resurrection.py could not be executed as written. I substituted the strongest available check — the exact head SQL against a real PostgreSQL with the relevant schema shape, plus AST binding of every call site — which covers the predicates and the error-branch logic, but not the asyncpg round-trip or the pytest fixtures end to end. CI runs the suite itself. This is a substitution of method, not a claim of equivalence; the changed SQL is verified, the harness is not.

Not blocking

Recorded for context, not as findings — I am not asking for changes:

1. lock_oauth_credential_for_refresh and get_vault_credential_with_blob still return ciphertext without a vaults.archived_at check. Unreachable as resurrection vectors at head (see above), and archive_vault zeroes pre-existing blobs. Widening the guard would be defensible symmetry but would change update_vault_credential's error shape for archived vaults — a separable decision.
2. list_session_vault_credentials deliberately has no vault-archival filter and returns no ciphertext; it is the metadata read model that intentionally surfaces archived rows. Correctly left alone.

Conventions look right: raw SQL confined to db/queries/vaults.py, the env-var predicate single-sourced through one template, ConflictError/NotFoundError used as update_vault does, and the test module follows the one-predicate-per-test style of test_vault_credential_account_scoping.py with a positive control beside each negative assertion.

Structured findings: none — issues: [], verdict: pass, artifact_posted: true (this comment).