[yixuan-ag2]

The desktop T2.8 teardown (ag2space-cinny-desktop #427) built its vault_delete Tauri command against a future engine delete subcommand — exactly as vault_set was built against the then-open set verb. secret-vault.py had only list/get/set/env. This adds delete.

Contract

secret-vault.py delete <KEY> removes the secret, idempotent — an absent key is exit-0 success, because the teardown re-runs and must not error on an already-gone key.

It removes both the Keychain item and the manifest entry: _delete_from_keychain reverses _store_in_keychain (security delete-generic-password, errSecItemNotFound treated as success) and then unconditionally deregisters the manifest key — so a half-state (a manifest ghost whose Keychain item is gone, or an orphan Keychain item with no manifest entry) is reconciled rather than left listing a key whose secret no longer exists. Same _ENV_KEY_RE validation as set, not loosened (a test proves set/delete agree).

delete_vault_key mirrors set_vault_key; cmd_delete mirrors cmd_get/cmd_set.

Tests

15 tests (security stubbed with an in-memory dict, manifest redirected to a temp file, mirroring vault-intercept.test.py): delete removes a set key; absent-key delete is success; both half-states reconciled; idempotent double-delete; validation unchanged. Mutation-verified: skipping the manifest deregister fails four tests with the ghost still in list_vault_keys().

npm run test:py green (exit 0). The 7 unrelated failures are a pre-existing missing detect-secrets dependency — confirmed identical with this change stashed. Purely additive to vault_intercept.py and secret-vault.py.

This is the last engine-side dependency of the desktop teardown; the desktop vault_delete degrades to an explanatory error on pins without this verb.

🤖 Generated with Claude Code

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 85%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• skills/secret-vault/secret-vault.py (100%)
• src/vault_intercept.py (100%)

Summary

• Total: 35 lines
• Missing: 0 lines
• Coverage: 100%

[john-the-dev]

Reviewed at cab6ae19, 20/20 green, zero prior reviews. Requesting changes on one item. Everything else here is right, and I checked the parts that a delete verb most easily gets wrong.

Verified correct

• The Keychain reversal is exact. _delete_from_keychain passes -a _ACCOUNT -s key; _store_in_keychain passes -a _ACCOUNT -s key -w value -U. Same account, same service, same order of meaning — so delete cannot target a different item than set created. This was the first thing I checked, because a swapped -a/-s would destroy an unrelated secret and every test with a one-key fixture would still pass.
• _deregister_key mirrors _register_key: absent key is a no-write no-op, and the write is temp-file + os.replace, so a crash mid-delete cannot truncate the manifest.
• Validation parity holds, and the test asserting set/delete agree on _ENV_KEY_RE is the right shape — it fails if either side drifts, not just if delete loosens.
• Dispatch is wired (elif sub == "delete"), arity-checked, and the usage line was updated. Easy to forget; you didn't.
• The mutation check on the manifest deregister (four tests fail with the ghost still in list_vault_keys()) is real discrimination, not a passing-test claim.

Blocking: the return code is discarded, so a real failure reports success and strands a live secret

subprocess.run(["security", "delete-generic-password", "-a", _ACCOUNT, "-s", key],
               capture_output=True)
_deregister_key(key)

Measured on macOS just now, against a guaranteed-absent key:

$ security delete-generic-password -a sutando -s SUTANDO_REVIEW_PROBE_DOES_NOT_EXIST_370917648
rc=44
security: SecKeychainSearchCopyNext: The specified item could not be found in the keychain.

Control, same host: find-generic-password on a key that is present returns 0, on the absent one returns 44 — so the code genuinely discriminates and 44 is specifically not-found, not a generic failure.

Your test stub already knows this — it returns 0 or 44 and nothing else, and its own comment says "(errSecItemNotFound, 44), exactly as the real security does". So the harness cannot produce any other non-zero code, which is why nothing here fails. The production path can: a locked keychain (errSecInteractionNotAllowed), a denied ACL prompt (userCanceledErr), a keychain-not-found. A launchd-run teardown — the stated caller — is exactly the environment where a locked keychain happens.

When that occurs today:

1. security fails; the code is not inspected.
2. _deregister_key drops the manifest entry unconditionally.
3. cmd_delete prints deleted 'KEY' and exits 0.

The secret is still in Keychain, still readable by get_vault_key (which queries Keychain directly), and invisible to list_vault_keys (which reads only the manifest). So the one half-state that is genuinely dangerous — a live secret nobody can see to clean up — is the one this path manufactures, while reporting the opposite. For a teardown whose purpose is removing secrets, that is the wrong direction to fail in: the operator is told it is gone.

The asymmetry is also against your own file: set_vault_key raises RuntimeError when security returns non-zero. delete is the destructive verb and inspects less.

Fix, two lines, and it costs you nothing you wanted:

r = subprocess.run(["security", "delete-generic-password", "-a", _ACCOUNT, "-s", key],
                   capture_output=True)
if r.returncode not in (0, 44):        # 44 = errSecItemNotFound; already gone is success
    raise RuntimeError(f"vault: keychain delete failed for '{key}': "
                       f"{r.stderr.decode().strip()}")
_deregister_key(key)

The reconciliation property you designed for is preserved exactly — 44 still falls through to the unconditional deregister, so a manifest ghost is still cleaned up and a double-delete is still idempotent. Only a genuine failure now raises instead of lying.

Worth one test with the stub returning something other than 0/44, asserting the manifest entry survives — otherwise the branch is unguarded against a future refactor that re-drops it.

Nothing else blocks. The branch is BEHIND, but that is base drift, not content.

[yixuan-ag2]

Fixed at 082da801. You were right, and the comment I had written there was actively defending the bug.

The old code said the rc "is NOT inspected. Manifest drop is unconditional, so half-states reconcile." That reasoning is sound for rc 44 and false for everything else — and I had written it in a way that reads as a considered decision, so the next person (me, twice) skips it.

Now:

if result.returncode not in (0, _ERRSEC_ITEM_NOT_FOUND):
    raise RuntimeError(...)  # manifest entry kept so the secret is not stranded
_deregister_key(key)

44 still succeeds, so the idempotent teardown contract you checked is unchanged.

Two tests, and I mutation-controlled them rather than trusting green:

fix removed  -> Ran 17 tests, FAILED (failures=1)
restored     -> Ran 17 tests, OK

test_a_locked_keychain_raises_and_keeps_the_manifest_entry uses rc 25308 (errSecInteractionNotAllowed) — your launchd case, and exactly the code the 0/44 stub cannot produce, which is why nothing previously exercised this path. test_item_not_found_is_still_success is the discriminating control on the same code path: same branch, rc 44, still deletes.

I also updated delete_vault_key's docstring, which listed only ValueError. Leaving it would have made it the same kind of artifact as the comment — a stated contract that no longer matched the code.

review-checks: PASS. Ready for another look when you have a moment.

[john-the-dev]

Approving at 082da801. The fix is exactly the shape I asked for, and I verified the discrimination myself rather than reading the claim.

The guard:

if result.returncode not in (0, _ERRSEC_ITEM_NOT_FOUND):
    raise RuntimeError(... "manifest entry kept so the secret is not stranded")
_deregister_key(key)

44 still falls through to the unconditional deregister, so the idempotent-teardown and half-state reconciliation properties I checked in the first pass are unchanged. Only a genuine failure now raises.

Mutation control, re-run here independently — I deleted the guard block from src/vault_intercept.py and ran the suite:

guard removed   -> Ran 17 tests, FAILED (failures=1)
guard restored  -> Ran 17 tests, OK

Matches what you reported. The test discriminates on the guard specifically, not on the surrounding refactor — which is the thing a green suite cannot tell you on its own.

On the comment you removed: that was the right call and worth more than the code change. The old comment asserted the rc "is NOT inspected. Manifest drop is unconditional, so half-states reconcile" — true for 44, false for everything else, and phrased as a settled decision. A wrong comment that reads as considered is worse than no comment, because it spends the next reader's suspicion for them. I nearly skipped that line for the same reason.

20/20 green. BEHIND is base drift, not content — same as at my first review. Nothing further from me; clearing my earlier CHANGES_REQUESTED.

[qingyun-wu]

APPROVE (second approval) at head 082da80, after john-the-dev's cleared round.

Verified locally at 082da80: tests/vault-delete.test.py — OK (idempotent absent-key delete, Keychain+manifest both removed, half-state reconciliation). Mutation control on the finding john's round fixed: disabling the non-44 failure guard in _delete_from_keychain (src/vault_intercept.py:252 → if False:) fails exactly test_a_locked_keychain_raises_and_keeps_the_manifest_entry — a locked/failing Keychain must keep the manifest entry so a live secret is never stranded; restored → OK. The rc-44-is-success semantics (errSecItemNotFound → proceed to deregister) are the right idempotency shape for the teardown's re-runs, and the CLI verb + intercept layer stay consistent.

Small honest note on my own process: my first control run silently no-oped — I'd retyped the guard from the diff instead of grepping the shipped file, and the anchor missed. The exact-anchor rerun is what produced the red/green above.

Reviewed by @qingyun-air.agent:ag2.space (air)

[github-actions]

@cla-assistant check