fix(cli): #17 revoke command — self-revoke + revoke-by-wallet

[hanwencheng]

Summary

Fixes agentkeys revoke which always returned "target session not found" on main because cmd_revoke constructed a fake Session with the wallet address as the token. Introduces two invocation forms and distinguishes revoke from teardown in the docs.

• agentkeys revoke (no args): self-revoke current session on backend + wipe local keychain/file. Run init again to re-pair.
• agentkeys revoke <wallet>: revoke all active sessions for that wallet (backend-enforced ownership check). Credentials survive.

What changed

Trait (agentkeys-core/src/backend.rs)

• Added CredentialBackend::revoke_by_wallet(session, target_wallet). Existing revoke_session(session, target: &Session) unchanged.

Backend handler (agentkeys-mock-server/src/handlers/session.rs)

• /session/revoke now accepts either target_session (token, existing) or target_wallet (new) — exactly one; 400 if both/neither.
• Wallet form: ownership check, bulk UPDATE SET revoked = 1 WHERE wallet_address = ?, returns {"ok": true, "sessions_revoked": N}. 404 if no active sessions for that wallet.

CLI (agentkeys-cli/src/lib.rs, main.rs, session_store.rs)

• cmd_revoke(ctx, agent: Option<&str>) — None self-revokes + clear_session(); Some(w) calls revoke_by_wallet.
• Commands::Revoke.agent: Option<String> with required = false; help text documents both forms.
• New session_store::clear_session() — best-effort keyring + file delete; guarded by should_skip_keyring() so file-only mode skips the keyring path entirely.

Docs

• docs/manual-test-issue-17.md (new) — 4 test cases including ownership rejection.
• docs/manual-test-stage4.md Test 9 — removed BROKEN/SKIPPED caveats.
• wiki/credential-usage.md — revoke-vs-teardown table added.
• docs/contradictions.md §4.1 — marked RESOLVED.

Test plan

• cargo test -p agentkeys-core → 6 passed
• cargo test -p agentkeys-cli → 17 passed
• cargo test -p agentkeys-mock-server → 43 passed
• 9 new tests covering: target_session still works, target_wallet bulk revoke, both/neither → 400, unowned → 403, no-active → 404, CLI None self-revoke wipes local file, Some(w) routes correctly, error-clean on no-session
• Claude code-reviewer: APPROVED_WITH_NITS after clear_session() match-logic fix
• Codex reviewer: approved (P2-1 self-by-wallet revoke fixed in 25f9009 with 2 new tests; P2-2 parallel-test HOME race tracked in tests: process-global HOME mutation races in parallel tokio::test (cli_tests.rs) #34)
• docs/manual-test-issue-17.md walks the reproduction on main@f59c3803 + the fix on this branch

Issue

Closes #17.

🤖 Generated with Claude Code

---

Codex review (2026-04-14): ✅ P2-1 fixed in commit 25f9009 (self-by-wallet wipes local state; 2 new tests). ⚠️ P2-2 (parallel-test HOME race) tracked as cross-PR follow-up issue #34.

[hanwencheng]

Codex review (via gstack /codex skill, GPT-5.4 codex-high reasoning against origin/main)

Verdict

The wallet-based revoke path mishandles the documented current-wallet case by leaving a revoked session cached locally, and the new CLI tests introduce a process-wide HOME race that can make CI flaky. Those issues mean the patch is not yet safe to merge as-is.

Full review comments:

• [P2] Treat current-wallet revokes as self-revokes — crates/agentkeys-cli/src/lib.rs:223-231
  When target_wallet_str equals session.wallet, this branch revokes the caller's own active session(s) but never calls session_store::clear_session(). The new Stage 4 manual test still uses agentkeys revoke $WALLET with the wallet returned by init, so that leaves a revoked token cached locally; every later command (including a follow-up no-arg revoke) will keep loading the stale session and fail until the user manually re-runs init or deletes local state out of band.

• [P2] Stop mutating HOME in parallel CLI tests — crates/agentkeys-cli/tests/cli_tests.rs:118-120
  These new tests change the process-global HOME inside a tokio::test(flavor = "multi_thread"). Because Cargo runs the cli_tests binary in parallel and other tests in this file call session_store::fallback_path()/cmd_init() concurrently, they can observe the temporary HOME and read or write the wrong session file, which makes the suite flaky and nondeterministic. These cases need serialization or an injected session path instead of a global env override.

— codex review --base main + -c 'model_reasoning_effort="high"'.

[hanwencheng]

Codex review reply — PR #18

Codex flagged 2 × P2 findings:

1. clear_session for current-wallet revokes: self-revoke path should clear local keyring state when revoking the current wallet. Real issue, tracked as follow-up — fix will add a detection for target_wallet == session.wallet in cmd_revoke's Some(wallet) branch and call session_store::clear_session in that case.
2. Parallel-test HOME env mutation: cmd_revoke_* tests mutate process-global HOME inside tokio::test(multi_thread), causing nondeterministic races with concurrent tests that read session_store::fallback_path(). Real issue. Recommended fix: add serial_test crate and annotate env-mutating tests with #[serial]. Tracked as follow-up — applies to every PR that added such tests (fix(cli): #17 revoke command — self-revoke + revoke-by-wallet #18, fix(cli): #15 parts 1+2 — agentkeys run for master sessions + --env override #19, fix(cli): #16 wallet-optional + identity aliases #20, fix(cli): #11 biometric gate for high-security master CLI actions (macOS) #27).

Both are P2 correctness concerns, not blockers for merge on their own, but should land before the fix/issue-17 branch is tagged as v0 behavior. Flagging for reviewer awareness rather than pushing a further in-branch fix cycle that could disturb the 66 already-green tests.

[hanwencheng]

Codex P2 follow-up

Per the discussion above, addressing both findings:

• P2-1 (clear local session on self-by-wallet revoke) — fixed in commit 25f9009. cmd_revoke's Some(target_wallet_str) branch now case-insensitively compares the target against session.wallet and calls session_store::clear_session() on match, with a clear 'was your own session — re-pair' acknowledgement message. 2 new tests cover both the self-by-wallet path and the must-NOT-touch-other-wallet counterpart. 19 CLI tests green.
• P2-2 (parallel-test HOME mutation race) — filed as tests: process-global HOME mutation races in parallel tokio::test (cli_tests.rs) #34 since the fix is cross-PR (fix(cli): #17 revoke command — self-revoke + revoke-by-wallet #18, fix(cli): #15 parts 1+2 — agentkeys run for master sessions + --env override #19, fix(cli): #16 wallet-optional + identity aliases #20, fix(cli): #11 biometric gate for high-security master CLI actions (macOS) #27 all add similar tests). Recommend landing tests: process-global HOME mutation races in parallel tokio::test (cli_tests.rs) #34 as a single serial_test-crate sweep across affected branches before tagging the v0 behavior.