fix(daemon): #14 --parent flag to bind pair requests to a specific master#22
fix(daemon): #14 --parent flag to bind pair requests to a specific master#22hanwencheng merged 3 commits intomainfrom
Conversation
|
Codex review (via gstack Verdict — NITS (2 × P2, 1 × P3)The Full review comments:
— codex |
|
Codex review reply — PR #22 Codex flagged 3 findings: 2 × P2 + 1 × P3.
All 3 are real concerns but none regress existing tests. Tracked for a follow-up PR. |
… 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>
All 3 codex findings fixed in-branch (2d261dc)Per the human design call, all three findings addressed rather than deferred. P2 — URL-encode P2 — P3 — Eager PR should now be ready to merge without follow-up tracking issues. |
…ster Closes #14. Daemon now accepts `--parent <ALIAS|WALLET>` to bind the pair request it opens to a specific master. Backend refuses approval from any other master. Without `--parent` the existing first-come-first-served behavior is preserved. Trait change: `CredentialBackend::open_auth_request` adds an optional `parent_wallet: Option<&WalletAddress>` parameter. Both implementations (MockHttpClient, InProcessBackend) pass it through. DummyBackend stub updated. Tests: 15 new integration tests in `crates/agentkeys-daemon/tests/pair_tests.rs`. Daemon total: 28 passed. Mock-server: 37 (unchanged — daemon tests cover the /auth-request/open parent_wallet path end-to-end). CLI: 14. Core: 6. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… 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>
hanwencheng
force-pushed
the
fix/issue-14
branch
from
2d261dc to
ffdc908
Compare
Rebased onto main + all 3 codex P2/P3 fixes live (ffdc908)Main had advanced significantly (PR #23 TTL → 30 days, PR #18 self-revoke, PR #19 credential list, PR #21 typed-identity Recover, PR #24 session namespacing). The branch rebased in 2 commits with 3 conflict-file resolutions:
Post-rebase test results: Running codex review against rebased HEAD. |
…--parent Two new P2s from codex review of the rebased PR #22: **P2 — Unknown `--parent` wallets accepted without backend validation** The v1 `looks_like_raw_wallet` fast path returned `0x` + 40-hex input as a literal wallet with no existence check. Any syntactically valid address, even one that doesn't correspond to any account, was accepted — the daemon then opened a pair request with a `parent_wallet` that no session could ever match, so the flow hung until timeout instead of failing immediately. Route raw wallets through `/identity/resolve` with `identity_type="wallet"`; the backend's post-PR-#21 wallet arm validates existence via the `accounts` table and 404s unknown values. **P2 — Mixed-case `--parent` wallets fail approval** EIP-55 checksummed addresses travel verbatim through the old fast path, but the mock backend stores wallets in lowercase. `parent_wallet` equality check at approval time compared the mixed-case input to the stored lowercase value, so valid checksummed addresses timed out. Normalize raw wallets to lowercase (`to_ascii_lowercase`) before sending to `/identity/resolve`; the backend's response comes back in canonical lowercase, so the stored `parent_wallet` matches what subsequent approve calls compare against. The old `looks_like_raw_wallet` helper is kept — it now just selects between `identity_type="wallet"` (with lowercase normalization) and `identity_type="alias"` (verbatim). Aliases with reserved characters still percent-encode via reqwest's `.query()` builder. Test: cargo test -p agentkeys-daemon --test pair_tests -- --test-threads=1 pair_tests: 15 passed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codex v2 P2s landed (3314b39)Both new findings from the post-rebase review fixed in-branch: P2 — Unknown P2 — Mixed-case The old Re-running codex to confirm. |
Codex v3 — approved, no findings (3314b39)
Both post-rebase P2s addressed cleanly, no new findings. PR is ready to merge. |
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>
1 participant
Summary
Adds
--parent <ALIAS|WALLET>to the agentkeys-daemon. When set, the pair request the daemon opens is bound to the named master; the backend refuses approval from any other master. Without--parentthe existing first-come-first-served behavior is preserved.What changed
Trait (
agentkeys-core/src/backend.rs)open_auth_requestsignature gainsparent_wallet: Option<&WalletAddress>.Daemon (
agentkeys-daemon/src/main.rs,pairing.rs)--parent <ALIAS|WALLET>clap arg.0x→ used as wallet directly. Else → resolved via the backend's identity lookup.open_auth_requestcall.Docs
docs/manual-test-issue-14.md(new) — 3 cases (wallet bind, alias bind, no-parent backward compat).Test plan
cargo test -p agentkeys-daemon→ 28 passed (13 baseline + 15 new pair_tests for--parentscenarios)cargo test -p agentkeys-core→ 6 passedcargo test -p agentkeys-cli→ 14 passedcargo test -p agentkeys-mock-server→ 37 passed (unchanged)Issue
Closes #14.
🤖 Generated with Claude Code
Codex review (2026-04-14): ✅ Approved after in-branch fix of all 3 findings (2d261dc). P2: URL-encode aliases via reqwest .query(). P2: strict 0x+40hex wallet check before literal-passthrough so 0x-aliases resolve correctly. P3: lazy --parent resolution scoped to pair/master-recover branches only.