Skip to content

fix(mock-server): #13 refactor — typed params + shared resolve_identity + modular handlers#21

Merged
hanwencheng merged 1 commit intomainfrom
fix/issue-13
Apr 14, 2026
Merged

fix(mock-server): #13 refactor — typed params + shared resolve_identity + modular handlers#21
hanwencheng merged 1 commit intomainfrom
fix/issue-13

Conversation

@hanwencheng
Copy link
Copy Markdown
Member

@hanwencheng hanwencheng commented Apr 14, 2026 •
edited
Loading

Summary

Refactors the mock server per CLAUDE.md "Mock Server Design Principles". No user-observable behavior change except an intentional BC break on POST /auth-request/open for Recover requests.

What changed

Typed identity resolution (handlers/identity.rs)

  • New resolve_identity_typed(db, identity_type, identity_value) -> Result<String, AppError> utility. identity_type ∈ {alias, email, wallet}. wallet validates format + passes through.
  • The existing HTTP handler /identity/resolve now delegates to it.

Typed columns on auth_requests (db.rs)

  • Added nullable identity_type + identity_value columns.
  • POST /auth-request/open REQUIRES these when request_type="Recover" (400 if missing) and REJECTS them for non-Recover (400 if present).
  • approve_auth_request reads these columns typed — no more JSON-blob parsing.

Modular minting (handlers/auth_request.rs)

  • Extracted mint_pair_session, mint_recover_session, mint_scope_change_session. Each returns a MintOutput struct.
  • approve_auth_request is a dispatch-and-ownership-check — no inlined per-request-type logic.

Client updates (mock_client.rs, test_client.rs)

  • open_auth_request derives identity_type + identity_value from AgentIdentity for Recover requests.

Intentional BC break

POST /auth-request/open now returns 400 for Recover requests that omit identity_type/identity_value, and for non-Recover requests that include them. Callers must be updated to pass the typed fields. Existing Recover tests in integration.rs and both client implementations are updated in this PR; any downstream caller will break loudly.

Test plan

  • cargo test -p agentkeys-core → 6 passed
  • cargo test -p agentkeys-cli → 14 passed
  • cargo test -p agentkeys-mock-server → 45 passed (37 baseline + 8 new refactor tests)
  • 8 new tests: resolve_identity_alias_returns_wallet, _email_returns_wallet, _wallet_passthrough, _not_found_errors, _invalid_type_errors, open_auth_request_recover_requires_typed_fields, open_auth_request_pair_rejects_typed_fields, approve_recover_uses_typed_fields
  • Claude code-reviewer: running at time of PR-open
  • Codex: deferred (codex exec timing out in this session)

Issue

Closes #13.

🤖 Generated with Claude Code

  • Codex reviewer: approved (both P2 findings fixed in f8b0b4a — ENS handling + wallet-404 check)

Codex review (2026-04-14): ✅ Both P2 findings fixed in f8b0b4a after human design call on PR #21 — (1) resolve_identity_typed accepts the ens arm; mock_client + test_client open_auth_request Recover branches serialize Ens as identity_type="ens" (was "alias"). (2) wallet arm now verifies the wallet exists in accounts before returning; unknown wallets 404 instead of 500 on the later sessions FK. Branch rebased onto main (post-#10 TTL + #17 + #19). 3 new regression tests; 25 cli + 6 core + 56 mock-server passing.

@hanwencheng
Copy link
Copy Markdown
Member Author

hanwencheng commented Apr 14, 2026

Follow-up from Claude code-reviewer (HIGH, non-blocking):

AgentIdentity::Ens in mock_client.rs currently maps to identity_type="alias" — silently wrong. Either extend resolve_identity_typed with an "ens" arm (DB lookup like alias/email) or map Ens to "wallet" if ENS is always resolved to an address before this point.

Tracking as follow-up (not blocking this PR's merge since no code path currently exercises the Ens branch end-to-end). Issue to be filed after this PR merges.

@hanwencheng
Copy link
Copy Markdown
Member Author

hanwencheng commented Apr 14, 2026

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

Verdict

The refactor introduces behavior regressions in recovery flows: ENS identities are no longer handled consistently, and wallet-based recovery now returns an internal error for unknown wallets. These are user-visible breakages relative to main.

Full review comments:

  • [P2] Preserve ENS identities in resolve_identity_typed — crates/agentkeys-mock-server/src/handlers/identity.rs:88-114
    resolve_identity_typed now hard-rejects any identity_type other than alias, email, or wallet, but CredentialBackend::recover_session still serializes AgentIdentity::Ens as identity_type="ens" (crates/agentkeys-core/src/mock_client.rs:610-614). On main, ENS recovery went through the generic identity_links lookup; with this change, any ENS-linked recovery or /identity/resolve request now fails with 400 unknown identity_type instead of resolving the wallet.

  • [P2] Send the correct typed identity for ENS auth requests — crates/agentkeys-core/src/mock_client.rs:414-418
    The new typed fields for open_auth_request serialize AgentIdentity::Ens as identity_type="alias". That changes the master-approval recovery flow for ENS identities: approving a recover request now looks up the alias namespace instead of the ENS namespace, so an identity linked as ens will no longer mint a recovered session. This is a regression from the old CBOR parsing path, which mapped Ens to ens.

  • [P2] Keep the wallet-existence check in /session/recover — crates/agentkeys-mock-server/src/handlers/session.rs:199-200
    Switching recover_session to resolve_identity_typed removed the old accounts existence check for identity_type="wallet". If a caller passes a hex-looking wallet that is not present in accounts, the resolver now returns it as valid and the later INSERT INTO sessions hits the foreign-key constraint, turning a previous 404 no account found into a 500 internal error.

— codex review --base main + -c 'model_reasoning_effort="medium"' (initial high-effort run was interrupted; re-run at medium).

@hanwencheng
Copy link
Copy Markdown
Member Author

hanwencheng commented Apr 14, 2026

Codex review reply — PR #21

Codex flagged ENS identity handling (Ens(s) silently mapped to identity_type="alias") and wallet-based recovery returning 500 on unknown wallets (should be 404).

Both tracked as follow-up:

  • ENS handling: extend resolve_identity_typed with an "ens" arm OR map Ens to "wallet" if ENS is always pre-resolved. Needs a design decision on which — flagged in PR body and issue Refactor backend: typed parameters, shared identity resolution, modular handlers #13.
  • Wallet 500 → 404: resolve_identity_typed ("wallet" branch) currently returns Internal error for unknown wallets instead of NotFound. 3-line fix; not blocking on its own.

Both apply to the refactor's hot path but don't regress any existing test. Flagging rather than cycling in-branch to avoid disturbing the 45 currently-green mock-server integration tests.

@hanwencheng
fix(mock-server): #13 typed params + shared resolve_identity + modula…
f8b0b4a 
…r handlers

Rebase onto main (post-#10 30-day TTL + #17 revoke + #19 credential/list)
with ENS handling and wallet-404 fixes baked in per PR #21 human design call.

## Refactor (original #13 scope)
- `handlers/identity.rs::resolve_identity_typed(db, identity_type, identity_value)`
  — single typed resolver used by `approve_auth_request` Recover branch and
  the `/identity/resolve` endpoint.
- Typed `identity_type` + `identity_value` columns on `auth_requests`.
  POST /auth-request/open now REQUIRES them for Recover and REJECTS them
  for non-Recover (400 either way).
- Modular minting: `mint_pair_session`, `mint_recover_session`,
  `mint_scope_change_session` each return a `MintOutput`. `approve_auth_request`
  is pure dispatch — no inlined per-request-type logic.

## Rebase + codex P2 fixes (new in this push)
- mint_pair_session and mint_recover_session updated to the 30-day TTL
  (2_592_000 sec) policy from #10, not the old 1-hour default.
- `resolve_identity_typed` "ens" arm added — ENS identities now resolve
  via the same identity_links lookup as alias/email (codex P2 round 1).
- `mock_client.rs` + `test_client.rs` open_auth_request Recover branches
  now serialize AgentIdentity::Ens as identity_type="ens" (was silently
  "alias").
- `resolve_identity_typed` "wallet" arm now confirms the wallet exists in
  `accounts` before returning it; unknown wallets return 404 instead of
  flowing through to a 500 on the sessions FK (codex P2 round 2).

## Tests
- 3 new regression tests:
  - `resolve_identity_ens_returns_wallet`
  - `resolve_identity_wallet_unknown_returns_not_found`
  - (+ `resolve_identity_wallet_passthrough` updated to use a real session
    wallet so the new existence check passes)
- 8 original #13 tests preserved
- cli 25 / core 6 / mock-server 56, all passing

Closes #13.
@hanwencheng
Copy link
Copy Markdown
Member Author

hanwencheng commented Apr 14, 2026

Rebased onto main + codex P2s landed in-branch (f8b0b4a)

Per your direction, both codex P2 findings are now fixed in this PR instead of deferred:

ENS identity handling (codex P2 round 1)

  • resolve_identity_typed now accepts "ens" as a valid identity_type and resolves it through the same identity_links lookup as alias/email.
  • mock_client.rs + test_client.rs open_auth_request Recover branches serialize AgentIdentity::Ens as identity_type="ens" (was silently "alias" — the mismap codex flagged).

Wallet recovery 500 → 404 (codex P2 round 2)

  • resolve_identity_typed "wallet" arm now confirms the wallet exists in accounts before returning it. Unknown wallets get 404 no account found for wallet X instead of flowing through to a 500 on the later sessions FK.

Rebase conflicts resolved:

3 new regression tests landed:

  • resolve_identity_ens_returns_wallet
  • resolve_identity_wallet_unknown_returns_not_found
  • resolve_identity_wallet_passthrough updated to use a real session wallet so the new existence check passes
cargo test -p agentkeys-cli -p agentkeys-core -p agentkeys-mock-server -- --test-threads=1
cli: 25 passed; core: 6 passed; mock-server: 56 passed

PR state: CLEAN / MERGEABLE.

@hanwencheng hanwencheng merged commit dfe82af into main Apr 14, 2026
hanwencheng added a commit that referenced this pull request Apr 14, 2026
@hanwencheng @claude
fix(core/cli/daemon): #12 v11 — rebase onto main + post-#21 alignment
7f33f8f 
Rebase of fix/issue-12 onto current main (post-PR #18 #19 #21 #23 merges)
surfaced two concrete integration issues, fixed in this patch:

**1. Expose session_store helpers for test reuse**
- Make `agentkeys_core::session_store::fallback_path(session_id)` public
  so CLI tests can assert on the on-disk path layout.
- Re-export `agentkeys_core::session_store` at `agentkeys_cli::session_store`
  so existing test imports (`use agentkeys_cli::session_store;`) keep
  working after the old cli-local `session_store.rs` was deleted upstream.

**2. cmd_revoke clear_session calls (post-PR #18 self-revoke merge)**
PR #18 added `session_store::clear_session()` calls on self-revoke paths.
The new signature takes `session_id`, not 0 args. Pass `&ctx.session_id`
so the revoke wipes the correct namespace for any future multi-session
CLI caller.

**3. Integration tests: seed identity_links for Recover**
PR #21 tightened `resolve_identity_typed` to require the identity to
exist in `identity_links` before resolution. Two pair_tests
(`recover_full_loop`, `recover_credentials_intact`) opened Recover
requests with aliases that were never linked, so they now 404.
Added `InProcessBackend::link_identity_for_tests(identity_type,
identity_value, wallet)` that inserts directly into the mock DB's
`identity_links` table. The two failing tests now seed the alias link
before the Recover flow.

Test: cargo test -p agentkeys-core -p agentkeys-daemon -p agentkeys-cli -p agentkeys-mock-server -- --test-threads=1
core: 21; daemon: 13 + 15; cli: 30; mock-server: 56 — all passing

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@hanwencheng hanwencheng mentioned this pull request Apr 15, 2026
Merged
7 tasks
hanwencheng added a commit that referenced this pull request Apr 15, 2026
@hanwencheng @claude
fix(daemon): #22 v3 — codex P2s on rebased PR — validate + normalize …
3314b39 
…--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>
@hanwencheng hanwencheng mentioned this pull request Apr 15, 2026
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Refactor backend: typed parameters, shared identity resolution, modular handlers

1 participant

@hanwencheng