fix(cli): #16 wallet-optional + identity aliases by hanwencheng · Pull Request #20 · litentry/agentKeys

hanwencheng · 2026-04-14T06:01:06Z

Summary

Fixes #16 — makes the agent argument optional on store / read / run (defaulting to the current session wallet) and accepts identity aliases (non-0x strings) resolved via a new /identity/resolve backend path.

What changed

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

New CredentialBackend::resolve_identity(session, identifier) -> WalletAddress method.
Stubbed in DummyBackend.

Implementations

mock_client.rs — HTTP client calls GET /identity/resolve?identifier=<arg>.
test_client.rs — in-process implementation.

CLI

agentkeys-cli/src/lib.rs — new resolve_agent(ctx, session, agent: Option<&str>) helper:
- None → session wallet.
- Some(s) where s.starts_with("0x") → passed through as wallet address.
- Else → resolve_identity; unknown identities return "unknown identity '<arg>'. Use \agentkeys link` to create an alias or pass the 0x... wallet directly."`.
cmd_store, cmd_read, cmd_run, cmd_revoke take agent: Option<&str> and use resolve_agent.
main.rs — Run, Store, Read have agent: Option<String>.

Docs

docs/manual-test-issue-16.md (new) — 3 cases: default wallet, alias resolution, unknown identity error.
docs/contradictions.md §4.3 — marked RESOLVED.

Test plan

cargo test -p agentkeys-cli → 19 passed (5 new tests for defaults + alias + unknown identity)
cargo test -p agentkeys-core → 6 passed
cargo test -p agentkeys-mock-server → 37 passed (no new mock-server tests — the CLI integration tests exercise /identity/resolve end-to-end through InProcessBackend, which is sufficient; a pure-endpoint test can be added if desired)
Claude code-reviewer: running at time of PR open; any blocking feedback → follow-up commit
Codex reviewer: approved (3 codex passes — accepted tradeoff: keep --agent flag, migrate scripts; see footer)

Issue

Closes #16.

🤖 Generated with Claude Code

Codex review (2026-04-14): ✅ Resolved by human design call (option 1: --agent flag). 3 codex passes flipped between mutually exclusive recommendations (positional → optional positional panics clap → --agent flag breaks scripts). Owner accepted the breaking-script tradeoff over dropping the feature or hand-rolling a manual parser. Doc migration applied in commit 776b405 (wiki + main.rs long_about + contradictions.md). See latest reply for full rationale.

hanwencheng · 2026-04-14T07:42:53Z

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

Verdict — BLOCKED (1 × P1, 1 × P2)

The patch introduces a user-visible CLI regression by removing the previously supported positional agent syntax, and the new identity-resolution path breaks for valid aliases/emails containing reserved query characters. Those issues make the change unsafe to ship as-is.

Full review comments:

[P1] Preserve existing positional agent syntax — crates/agentkeys-cli/src/main.rs:45-46
Making agent a #[arg(long)] option turns the main-branch forms agentkeys store 0xAGENT ..., read 0xAGENT ..., and run 0xAGENT -- ... into parse errors or misparsed arguments. That is a breaking CLI regression for any existing scripts or docs using the current interface, and it is especially visible here because the top-level help text in this file still advertises the positional form. The wallet-default behavior should be added without removing the previously supported syntax.
[P2] Percent-encode identity values before calling /identity/resolve — crates/agentkeys-core/src/mock_client.rs:602-604
MockHttpClient::resolve_identity interpolates the raw alias/email into the query string. For identities containing reserved query characters—most commonly plus-addressed emails like bot+prod@example.com, but also values containing & or =—the server receives a different identity_value, so store/read/run --agent ... fails even though agentkeys link stored the identity successfully. Encode the query parameters before building the URL.

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

hanwencheng · 2026-04-14T08:08:18Z

Codex re-review v3 — design-tradeoff reply (not a code change)

Codex has now flagged the same issue three times from opposite angles:

v1 (initial): ⛔ remove --agent flag, keep positional (the main-branch form)
v2 (after positional): ⛔ optional Option<String> leading positional panics clap at parse time (Found non-required positional argument with a lower index than a required positional argument)
v3 (after revert to --agent flag): ⛔ --agent flag breaks scripts using positional wallet form

The design space:

Positional and --agent flag cannot coexist cleanly with clap derive macros unless both arguments are Option + all required args fill from the end. Clap's rule (first required wins arg consumption) makes agentkeys store openrouter sk-xxx ambiguous: is openrouter the agent or the service?
Manual ArgMatches dispatch would sidestep derive, but at a significant complexity cost for the whole CLI surface.
Subcommand split (agentkeys store-for <agent> ... vs agentkeys store ...) doubles the surface.

Chosen tradeoff (accept --agent flag, document the migration): Ship the flag form. Existing scripts using agentkeys store 0xABC openrouter sk-xxx break and require a one-line s/store /store --agent / sed. Docs and long_about updated to steer new users to the flag form. The URL-encoding fix (P2) and the alias-resolution feature this PR adds are preserved.

If a reviewer disagrees and wants real coexistence, the path is a manual-parser opt-in for store/read/run specifically. Flagging as a human design decision — not blocking the merge on codex's circular finding.

… wallet Codex P2 finding on PR #18: `agentkeys revoke <my-own-wallet>` revoked the session on the backend but left the dead token cached locally; every subsequent command loaded the stale revoked token and failed. cmd_revoke's `Some(target_wallet_str)` branch now case-insensitively compares target_wallet against session.wallet. On match: call session_store::clear_session() and return a 'was your own session — re-pair' message (matching the no-arg self-revoke form's UX). Tests added (19 total CLI tests, +2 from previous): - cmd_revoke_with_own_wallet_clears_local_session — verifies the new path wipes the local file and emits the 'was your own session' acknowledgement. - cmd_revoke_with_other_wallet_keeps_local_session — counterpart: revoking someone else's wallet must NOT touch the caller's session. P2-2 (parallel-test HOME race) is tracked separately as issue #34 — needs serial_test crate added cross-PR (#18, #19, #20, #27 all affected). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Per human design call on PR #20: ship the --agent flag form (option 1) and migrate every breaking script reference in our own docs. Files updated: - wiki/credential-usage.md — synopsis, examples, lifecycle, env-var mapping section all show the new form. Added a "breaking change" callout at the top with the sed migration recipe. Removed the stale "issue #15 limitation" blockquote (issue #15 is now fixed in PR #19). - wiki/key-security.md — the read escape-hatch + run preferred-usage examples updated. - wiki/data-classification.md — credential store flow diagram updated. - crates/agentkeys-cli/src/main.rs — top-level `long_about` (shown by `agentkeys --help`) now uses the --agent flag form across all examples and explicitly documents the omit-to-use-session-wallet behavior. - docs/contradictions.md §4.3 — corrected the resolution narrative (positional Option panics clap; --agent flag is the chosen disambiguation), documented the breaking change + migration sed. No code change in this commit; the actual --agent flag landed in efa1707 already. Test: cargo test -p agentkeys-cli -- --test-threads=1 → 19 passed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

hanwencheng · 2026-04-14T14:21:41Z

Human design call: option 1 (--agent flag) — script migration applied

Decision recorded: ship the --agent flag form. Owner accepted the breaking-script tradeoff over the alternatives (drop the wallet-default + alias features, or hand-roll a manual ArgMatches parser for store/read/run).

Migration applied to our own docs in commit 776b405:

wiki/credential-usage.md — synopsis, all examples, env-var mapping section, lifecycle commands. Added a "Breaking change in v0.x (CLI commands should default to session wallet + support identity aliases #16)" callout at the top with the migration sed:
```
sed -i '' -E 's/agentkeys (store|read|run) (0x[0-9a-fA-F]+|[a-zA-Z0-9._-]+@[a-zA-Z0-9._-]+|[a-zA-Z][a-zA-Z0-9_-]*) /agentkeys \1 --agent \2 /g' your-scripts/*.sh
```
wiki/key-security.md — read escape-hatch + run preferred-usage examples.
wiki/data-classification.md — credential store flow diagram.
crates/agentkeys-cli/src/main.rs — top-level long_about (shown by agentkeys --help) demonstrates the new form across all examples.
docs/contradictions.md §4.3 — resolution narrative corrected (positional Option panics clap; --agent flag is the chosen disambiguation); breaking change + migration recipe documented.

docs/spec/* retains the older positional examples — those are spec/plan documents, not user-facing migration material, and rewriting them is out of scope.

Tests still green: cargo test -p agentkeys-cli -- --test-threads=1 → 19 passed.

The PR is unblocked from the codex stalemate. Closing the human design call.

Combines the full rebase of fix/issue-16 onto main (post-#18 + #19 merges), with all conflicts resolved in a single clean commit. - `CredentialBackend::list_credentials` + `resolve_identity` trait methods live side-by-side; both `MockHttpClient` and `InProcessBackend` implement both. - `cmd_run` signature is `(ctx, agent: Option<&str>, env_overrides: &[String], cmd: &[String])`: - `agent` from #16 (defaults to session wallet when None) - `env_overrides` from #19 (HashMap cache + pre-flight validation) - `cmd_store` / `cmd_read` take `agent: Option<&str>`. - `cmd_revoke` takes `agent: Option<&str>` (signature kept from #18). - `main.rs` dispatches `Commands::{Store,Read,Run,Revoke} { agent, ... }` via `.as_deref()` since each carries `agent: Option<String>`. - clap derive `Run` has both `--agent` flag AND `--env KEY=SERVICE` flag. - Top-level `long_about` and all sub-command `long_about` strings show the new `--agent` flag form. - `wiki/credential-usage.md` / `wiki/key-security.md` / `wiki/data-classification.md` migrated to the new form with a "breaking change" callout + sed migration recipe. - `docs/contradictions.md` §4.3 resolution narrative corrected. - Integration + CLI tests: 30 cli + 6 core + 46 mock-server passing. Resolves issue #16 (wallet-optional + identity aliases) after the 3-pass codex stalemate landed on the `--agent` flag design (human call). Test: cargo test -p agentkeys-cli -p agentkeys-core -p agentkeys-mock-server -- --test-threads=1

… resolution All 3 codex findings on PR #22 addressed in-branch: **P2 — URL-encode `--parent` aliases** The old code built the query URL via `format!("{}/identity/resolve?identity_type=alias&identity_value={}", ...)` with no encoding. Aliases containing reserved characters (`+`, `&`, `%`, spaces) sent malformed requests or failed to parse. Switched to reqwest's `.query(&[...])` builder, which percent-encodes per RFC 3986. Same pattern as the fixes on PR #20 (aa0cc95) and PR #29 (7aa9e70). **P2 — `0x-`prefixed aliases misclassified as wallets** The old code treated any `--parent` value starting with `0x` as a literal wallet, but `cmd_link` allows aliases like `0x-office` / `0x+bar`. Added `looks_like_raw_wallet(s)` that demands strict `0x + 40 hex digits` before the literal fast-path triggers; everything else goes through `/identity/resolve`. 3 unit tests (accepts canonical hex, rejects 0x-hyphen aliases, rejects short/non-hex). **P3 — Eager `--parent` resolution** Resolution previously ran unconditionally at startup, so a transient backend-down would crash `agentkeys-daemon --session <token>` or `--recover --method passkey` even though those paths never use `parent_wallet`. Moved the call into `resolve_parent_if_set` and invoked only from the pair-flow and master-approval recover branches. The `--session` seam and the `--recover --method …` 2FA path now skip parent resolution entirely. Test: cargo test -p agentkeys-daemon -- --test-threads=1 unit: 3 passed; daemon_tests: 15 passed; pair_tests: 15 passed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Post-rebase review (claude code-reviewer) flagged 3 P1 issues that map directly to patterns in `.github/REVIEW_GUIDELINES.md`. All fixed in this commit plus one P2 nit that was cheap. **P1 — URL encoding via reqwest `.query()` on scope + list_credentials** `get_scope` and `list_credentials` in `mock_client.rs` built queries with raw `format!()` interpolation. Wallet strings with reserved characters (`&`, `#`, `%`, `+`, spaces) would break the request or smuggle extra params server-side. Switched both to `.get(self.url("/path")).query(&[...])` so reqwest percent-encodes per RFC 3986. Same pattern as the fix on PR #20 `aa0cc95` / PR #22 `ffdc908` / PR #29 `7aa9e70`. **P1 — Case-insensitive self-target check in `update_scope`** The self-target reject `session.wallet_address == target_wallet` was string-equal, but the backend stores wallets in lowercase while EIP-55 checksummed input preserves case. A master passing its own wallet in mixed-case form would bypass the self-target guard and silently restrict its own scope (the exact failure the guard was designed to prevent). Switched to `eq_ignore_ascii_case` matching the pattern established in PR #18's self-revoke detection. **P1 — Missing DENIED audit rows on scope endpoints** `update_scope` and `get_session_scope` returned 403 on ownership failure without inserting an audit row, breaking the cross-agent-probing-visibility invariant established for `list_credentials` / `read_credential` in PR #19. Added `'scope_update' 'DENIED'` and `'scope_read' 'DENIED'` inserts before the 403 return in both handlers. **P2 — `--add foo --remove foo` would no-op silently** `cmd_scope` accepted combinations like `--add X --remove X`. The add loop would push, then `retain(!remove.contains)` would cancel, and the backend PUT would still fire with the original scope + misleading "Scope updated" output. Added an up-front `add ∩ remove` check that lists overlapping services and rejects. **P2 — Narrow UPDATE to most-recent active session** Write-side `update_scope` UPDATEd ALL active sessions for the target wallet; read-side `get_session_scope` always returned the scope from the most-recent session via `ORDER BY created_at DESC LIMIT 1`. Read/write contract mismatched on wallets with multiple active sessions (paired + recovered). Narrowed the UPDATE to use the same ORDER/LIMIT subquery. Test: cargo test -p agentkeys-core -p agentkeys-cli -p agentkeys-mock-server -- --test-threads=1 core: 22 passed; cli: 35 passed; mock-server: 56 passed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(cli): #15 part 3 (fix-15b) — agentkeys scope command Closes part 3 of #15. Adds `agentkeys scope <AGENT>` subcommand with `--add`/`--remove`/`--set`/`--list` for editing a child agent's scope. Under the hood: new trait methods `CredentialBackend::get_scope` + `update_scope` backed by a new mock-server `PUT /session/scope` endpoint (owner-checked, updates all active sessions for the target wallet). Tests: 5 new CLI integration tests (list/add/remove/set/conflict). 62 total tests across cli/core/mock-server, all green under --test-threads=1. Minor: CommandContext::load_session made pub so integration tests can access the current session token. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(cli/mock-server): #15b v2 — address codex P1+P2 Codex P1: update_scope handler allowed a master session to target its own wallet, silently writing a restrictive scope_json onto the master's session row. Subsequent credential/read calls would start failing for services outside the accidental scope. Now rejects self-targeting with a clear bad_request. Codex P2 (URL encoding): cmd_scope's identity resolution built the /identity/resolve query string via raw interpolation, breaking aliases/emails with reserved characters ('+', '&', '=', '%', spaces). Switched to reqwest's .query() builder which percent-encodes per RFC. Codex P3 (--list exclusivity): --list now rejects combination with --add/--remove/--set up front instead of silently dropping the mutation. Deferred (tracked as follow-up): CBOR vs JSON parsing of ScopeChange request_details in mint_scope_change_session — the scope CLI uses the direct /session/scope endpoint, not the auth-request flow, so this finding doesn't affect the CLI path. Will be addressed when AuthRequest::ScopeChange approval is wired end-to-end. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(cli/mock-server): #29 v3 — claude P1+P2 review fixes after rebase Post-rebase review (claude code-reviewer) flagged 3 P1 issues that map directly to patterns in `.github/REVIEW_GUIDELINES.md`. All fixed in this commit plus one P2 nit that was cheap. **P1 — URL encoding via reqwest `.query()` on scope + list_credentials** `get_scope` and `list_credentials` in `mock_client.rs` built queries with raw `format!()` interpolation. Wallet strings with reserved characters (`&`, `#`, `%`, `+`, spaces) would break the request or smuggle extra params server-side. Switched both to `.get(self.url("/path")).query(&[...])` so reqwest percent-encodes per RFC 3986. Same pattern as the fix on PR #20 `aa0cc95` / PR #22 `ffdc908` / PR #29 `7aa9e70`. **P1 — Case-insensitive self-target check in `update_scope`** The self-target reject `session.wallet_address == target_wallet` was string-equal, but the backend stores wallets in lowercase while EIP-55 checksummed input preserves case. A master passing its own wallet in mixed-case form would bypass the self-target guard and silently restrict its own scope (the exact failure the guard was designed to prevent). Switched to `eq_ignore_ascii_case` matching the pattern established in PR #18's self-revoke detection. **P1 — Missing DENIED audit rows on scope endpoints** `update_scope` and `get_session_scope` returned 403 on ownership failure without inserting an audit row, breaking the cross-agent-probing-visibility invariant established for `list_credentials` / `read_credential` in PR #19. Added `'scope_update' 'DENIED'` and `'scope_read' 'DENIED'` inserts before the 403 return in both handlers. **P2 — `--add foo --remove foo` would no-op silently** `cmd_scope` accepted combinations like `--add X --remove X`. The add loop would push, then `retain(!remove.contains)` would cancel, and the backend PUT would still fire with the original scope + misleading "Scope updated" output. Added an up-front `add ∩ remove` check that lists overlapping services and rejects. **P2 — Narrow UPDATE to most-recent active session** Write-side `update_scope` UPDATEd ALL active sessions for the target wallet; read-side `get_session_scope` always returned the scope from the most-recent session via `ORDER BY created_at DESC LIMIT 1`. Read/write contract mismatched on wallets with multiple active sessions (paired + recovered). Narrowed the UPDATE to use the same ORDER/LIMIT subquery. Test: cargo test -p agentkeys-core -p agentkeys-cli -p agentkeys-mock-server -- --test-threads=1 core: 22 passed; cli: 35 passed; mock-server: 56 passed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(cli/mock-server): #29 v4 — claude re-review nits (dead sort, pct_encode consistency, test coverage) Follow-up on the claude-code-action re-review posted against commit 57dc9fb. All 3 substantive findings addressed; the 4th (malformed `/` doc-comments) was a false positive from pre-rebase state — grep for `^\s*/ ` returns nothing in the current tree. **Low — Redundant `sort_by` after `update_scope`** `lib.rs:769` was re-sorting `new_scope.services` after the backend call returned, but both the `--set` branch (line 749) and the `--add`/`--remove` branch (line 760) sort before the `update_scope` call. Dead op; removed. Added a comment so a future reader doesn't re-add it. **Low — `InProcessBackend::get_scope` raw URL concat** `test_client.rs:725` built the query string via `format!()` while the `resolve_identity` method right above uses `pct_encode`. Wallet strings are hex so this was safe in practice, but the inconsistency violates the URL-encoding invariant documented in `.github/REVIEW_GUIDELINES.md` pattern #3. Swapped to `pct_encode`. **Low — Missing test for `--list + --add` conflict** Added `cmd_scope_list_and_add_conflict_errors`. Also added `cmd_scope_add_remove_overlap_errors` (covering the P2 overlap guard from v3) that wasn't yet under test. Test: cargo test -p agentkeys-core -p agentkeys-cli -p agentkeys-mock-server -- --test-threads=1 core: 22 passed; cli: 37 passed (+2 new); mock-server: 56 passed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

hanwencheng force-pushed the fix/issue-16 branch from 05a7073 to 513d836 Compare April 14, 2026 06:02

This was referenced Apr 14, 2026

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

Merged

fix(daemon): #14 --parent flag to bind pair requests to a specific master #22

Merged

tests: process-global HOME mutation races in parallel tokio::test (cli_tests.rs) #34

Closed

hanwencheng force-pushed the fix/issue-16 branch from 776b405 to a021453 Compare April 14, 2026 15:15

hanwencheng merged commit d0179e5 into main Apr 14, 2026

hanwencheng mentioned this pull request Apr 15, 2026

fix(cli): #15 part 3 (fix-15b) — agentkeys scope command #29

Merged

6 tasks

hanwencheng mentioned this pull request Apr 15, 2026

fix(tests): #34 race-free parallel cli tests via ctor + #[serial] #42

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cli): #16 wallet-optional + identity aliases#20

fix(cli): #16 wallet-optional + identity aliases#20
hanwencheng merged 1 commit intomainfrom
fix/issue-16

hanwencheng commented Apr 14, 2026 •

edited

Loading

Uh oh!

hanwencheng commented Apr 14, 2026

Uh oh!

hanwencheng commented Apr 14, 2026

Uh oh!

hanwencheng commented Apr 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

hanwencheng commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What changed

Test plan

Issue

Uh oh!

hanwencheng commented Apr 14, 2026

Verdict — BLOCKED (1 × P1, 1 × P2)

Uh oh!

hanwencheng commented Apr 14, 2026

Uh oh!

hanwencheng commented Apr 14, 2026

Human design call: option 1 (--agent flag) — script migration applied

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

hanwencheng commented Apr 14, 2026 •

edited

Loading