Repository navigation
fix(auth): preserve x402 payment parity for API keys and browser sessions - #712
Conversation
pr-reviewer Summary for #2619a12📝 2 findings Review completed. Please address the findings below. Findings by Severity
Review effort: 3/5 (Moderate) SummaryThis PR correctly restores x402 automatic payment for API-key requests (broken by the browser-login PR) and extends the same behavior to browser sessions, with a well-structured admission-denial guard that stops payment chains on 401/403/429/451. The safety-critical invariants — no credential forwarding on paid retries, PAYMENT_AMBIGUOUS escalation, policy/spending-limit checks — are all preserved and covered by 59 new parity tests. Two medium-severity issues are worth addressing before merge. Findings
|
There was a problem hiding this comment.
Independent review: PR #712
Verdict: APPROVE. No blocking defects found. Findings below are documentation drift, a minor test gap, and one server assumption to confirm before release.
Reviewed head: 9af401a094b2958781a4ad14e872dc3a5a9bab8a
Base: 5f0463b199f76fc09b92140d7a12d9b12d5c4b5f (main)
Repo: nansen-ai/nansen-cli
Writer was hulk-linus[bot] on an OpenAI GPT-6 family model. I am a separate reviewer on a different provider and family (DeepSeek), so the independence rule holds.
What the PR does
Three commits change src/api.js, its tests, and docs. API-key and browser-session selections now enter the same automatic x402 payment flow that was previously anonymous only. mayAutoPay at src/api.js:913 drops the selection.kind === 'anonymous' condition and keeps two guards: an explicit raw apikey/authorization header still blocks auto-pay, and either the instance or the request can disable payment (options.allowPayment !== false). The paid retry sends only Payment-Signature and no account credential. getAccount() now passes allowPayment: false so the free account/login check never pays.
Findings
Finding 1, documentation drift in shipped help text (low). src/cli.js:1127 still prints "NANSEN_API_KEY overrides saved authentication. Selected credentials never auto-pay." That line comes from a5f4529 (#659) and the PR does not touch it. The PR rewrites the same claim in README.md and docs/browser-login.md:42 but leaves the CLI help, which users read directly, stating the opposite of the new behavior. Update the line to say keys and browser sessions can pay a supported 402 under the same wallet policy.
Finding 2, documented skill guidance is now misleading (low). About twenty shipped files read "Anonymous x402 is a separate paid workflow: without a selected credential, the CLI can automatically spend funds from its configured wallet on a supported 402 challenge" (for example skills/nansen-token-screener/SKILL.md:24). After this PR a selected credential can also spend from the configured wallet. package.json ships skills/**, so agents load this guidance. The paragraph should state that selected credentials now pay under the same policy.
Finding 3, test gap for a documented invariant (minor). The comment at src/api.js:910-913 claims a request cannot re-enable payment when the instance disabled it. The code satisfies that (this.allowPayment && options.allowPayment !== false), but no test passes options.allowPayment: true against an instance with allowPayment: false. A one-line case would pin it.
Finding 4, unverified server assumption (not a defect here). The paid retry omits the account credential (comment at src/api.js:807-810, asserted by the new test). The whole parity feature depends on the API admitting a payment-only request for a key or session client. The mocked tests encode that assumption by returning 200. I cannot confirm it without a live call. If the API instead requires both the account credential and the payment, key/session retries would return 401/403 and the new admission-denial branch would raise PAYMENT_AMBIGUOUS on every attempt. Worth a quick confirmation with the API owner before release.
Finding 5, changeset level (withdrawn after re-reading both files). I first wrote that both changesets are patch. That is wrong. .changeset/browser-session-login.md is major, and its edited payment paragraph already describes the key and session parity in full, so the user-visible spending change is already carried by a major entry. .changeset/x402-credential-parity.md is patch, which is reasonable for the incremental fix. No changeset change is needed.
Verification I ran
npm test on the exact head: 4375 passed, 9 skipped, 0 failed. npm run lint: clean. Focused npx vitest run src/__tests__/x402-auth-parity.test.js src/__tests__/auth-login.test.js: 104 passed. No live API calls and no wallet operations.
Code paths I confirmed by reading the full changed file:
getAccount()disables payment atsrc/api.js:1259, sonansen accountand login verification cannot pay. Covered byx402-auth-parity.test.js("never pays for the free account check").- A paid retry answered 401, 403, 429, or 451 throws
PAYMENT_AMBIGUOUSatsrc/api.js:848-855, which stops the loop before a second wallet or WalletConnect signs. Covered by the "does not sign another option" cases. - The 402 gate at
src/api.js:1071(code === PAYMENT_REQUIRED && response.status === 402) keeps a non-402 body code that aliases topayment_requiredout of the payment flow. - Session error handling resets
dataand suppresses the raw challenge (src/api.js:1056,src/api.js:1147), and the new generic messages avoid echoing tokens. Covered by the malformed-challenge test. - The body-only WalletConnect challenge now survives the session
datareset viabodyPaymentRequirements(src/api.js:1150-1151). Covered by the body-only case.
Security and dead-code audit
The change sits on an auth and payment trust boundary, so I checked credential and token flow specifically. The paid retry carries no apikey or Authorization (test asserts both absent), and session errors do not echo access tokens, refresh tokens, or the raw challenge. Policy evaluation still runs before any signing (over-cap amount, unknown asset, unsupported scheme all refuse), and retryOnStatus is [429, 500, 502, 503, 504], so a 402 is never re-driven through the request loop, which would otherwise re-sign a payment.
Dead code: the only changed production file is src/api.js (2252 lines), read in full. Every added symbol has a live reader: options.allowPayment at src/api.js:913, bodyPaymentRequirements at src/api.js:1150-1151, and the status === 402 guard at src/api.js:1071. No unused import, variable, branch, or commented-out code was introduced. No dead-code detector is configured in this repo (no knip or ts-prune); eslint is the only static tool and it passed. I found no regressions: the anonymous auto-pay tests in api.test.js still pass, and the edits in auth-login.test.js correctly move the 402 case out of the "selected key never auto-pays" matrix and add allowPayment: false to the manual-challenge test.
Limitations
I did not exercise a live API or a real wallet, so the payment-only admission assumption in Finding 4 is unverified. The coordinator checked my Factory model initialization, not live browser-session behavior, so no runtime confirmation of the session path exists either. I reviewed the payment and session-error paths through the mocked suite and by reading the full file, not by running the CLI against production.
Reviewer runtime
Factory Droid, DeepSeek, deepseek-v4-pro, effort high.
Codier
left a comment
There was a problem hiding this comment.
Follow-up review: PR #712 repair
Verdict: APPROVE. The repair fixes the actionable findings from my first review, and no new defect appeared. One residual limitation remains open (server-side product behavior, not a CLI defect).
Reviewed head: 2619a1202c89ac22e1a1fe235876cc5e039e1247
Prior reviewed head: 9af401a094b2958781a4ad14e872dc3a5a9bab8a
Base: 5f0463b199f76fc09b92140d7a12d9b12d5c4b5f (main)
Writer was hulk-linus[bot] on an OpenAI GPT-6 family model. I review as Codier on DeepSeek V4 Pro, a different provider and family, so independence holds. src/api.js is byte-identical to my prior reviewed head (git diff 9af401a..2619a12 -- src/api.js is empty), so the payment implementation I approved is unchanged.
Follow-up status
Finding 1, stale CLI help. Fixed. src/cli.js:1127-1129 now reads "API keys and browser sessions can auto-pay supported HTTP 402 challenges under the same wallet controls and spending limits" and "Authentication failures never trigger payment; login verification and nansen account never auto-pay." I ran node src/index.js --help and confirmed the corrected lines print in the AUTHENTICATION block.
Finding 2, stale skill guidance. Fixed. All 23 shipped skills now carry one identical replacement paragraph describing 402 parity under existing wallet authorization, policy, and limits, with per-call cost including loops, no payment on auth or renewal failure, non-paying login and nansen account, and preserved anonymous intent. I diffed the skill changes and confirmed 23 identical added lines and zero stale copies of the old "Anonymous x402 is a separate paid workflow" claim.
Finding 3, precedence test gap. Fixed. src/__tests__/x402-auth-parity.test.js:122 adds a case that passes allowPayment: true per request against a constructor with allowPayment: false, for each of api-key, session, and anonymous. It asserts one fetch, PAYMENT_REQUIRED, and no signing. This pins the this.allowPayment && options.allowPayment !== false guard at src/api.js:913.
Finding 4, payment-only admission assumption. Resolved at source and mocked-test level, with a caveat. I read the writer's cited evidence in the nansen-api checkout at 065bfbd7da0484826d163395f1eacb10db3c3ab7. kubernetes/nansen-api-wrapper/manifest.yaml:202-244 shows Kong's key-auth anonymous fallback to anonymous-x402, plus a post-function plugin that returns 401 when an apikey header was present but invalid, so an invalid key no longer reads as a payable anonymous request. wrapper/x402_setup.py:1304 (x402_anonymous_gate) skips Bearer candidates, rejects unpriced paths with a structured 401, and otherwise dispatches to the x402 handler. wrapper/middleware/auth.py:737-864 admits a request with a verified payment_payload as an x402 payer without account authentication and without calling get_user_data. The CLI's paid retry sends payment only and no key or Bearer, which is exactly the shape that gate admits. Two tests exercise real middleware: TestAuthMiddlewareX402 against the actual AuthenticationMiddleware, and TestX402AnonymousConsumerGate::test_unpriced_anonymous_consumer_gets_structured_401 against the real FastAPI app. The writer's log at /tmp/api503-review/api-admission-tests.log shows 14 passed with sockets blocked by the runner at /tmp/api503-review/run-api-admission-tests.py. I treat the source-contract uncertainty as resolved. I do not claim deployed or live settlement confirmation.
Finding 5 was already withdrawn after I re-read both changesets. .changeset/browser-session-login.md is major and already carries the payment parity paragraph, and .changeset/x402-credential-parity.md is patch.
Residual limitation
The evidence shows a payment-only request is admitted as an x402 payer. It does not directly show that the API returns a payable 402 challenge with an accepts list to an authenticated API-key or browser-session request on a priced endpoint, rather than a 401 or 403. That is a server product decision, not a CLI defect, and it is the one thing a live end-to-end run would confirm. I could not test it here.
Checks I ran on the new head
Focused npx vitest run src/__tests__/x402-auth-parity.test.js src/__tests__/auth-login.test.js: 107 passed, up 3 from the prior head for the new precedence case. npm run lint: clean. node src/index.js --help: corrected AUTHENTICATION text prints. No live API calls and no wallet operations. I did not rerun the full suite; the writer's 4378 passed and 9 skipped is consistent with the focused run.
Dead-code audit
Changed production file: src/cli.js, and only the static help template. That text is live, proven by the --help output. src/api.js is unchanged from the head I already audited in full. No new import, variable, branch, or helper was added, so nothing can be dead. Markdown skill files are not production code. No dead-code detector is configured in this repo; eslint is the only static tool and it passed.
Limitations
No live API, wallet, or settlement run, per task constraints. Admission evidence is repository source plus middleware unit tests with network blocked, not a deployed end-to-end proof.
Reviewer runtime
Factory Droid, DeepSeek, deepseek-v4-pro, effort high.
The browser-login changes disabled automatic x402 payment whenever an API key or browser session was selected. Preserve the existing API-key behavior and give browser sessions the same handling of a supported HTTP 402 challenge, as clarified for API-503.
Both credentials use the existing local-wallet, Privy and WalletConnect dispatch with its payment policy, spending limits and duplicate-payment protection. The paid retry sends the payment signature without the account credential and leaves saved authentication unchanged. HTTP 401/403, other non-402 errors and session-acquisition failures cannot trigger signing. Login verification and account checks remain non-paying. A paid retry denied with 401/403/429/451 stops without signing another option because the payment outcome is unconfirmed.
The README, browser-login guide, CLI help, all 23 affected shipped skills and unconsumed major changeset now describe this behavior. A patch changeset records the correction. Regenerate the pending 2.0.0 release changelog after merge.
Validation at
2619a1202c89ac22e1a1fe235876cc5e039e1247:npm test: 108 files passed; 4,378 tests passed, nine skipped.npm run lint,git diff --checkand CLI help smoke check passed.allowPayment: truecannot override a client with payment disabled, for keys, sessions and anonymous requests.src/api.jsandsrc/cli.js, including full-file reads and live callers. HELP is consumed by both command help andrunCLI; this repair changes help text, docs and tests only. No new dead code. ESLint is the configured static check; no separate dead-code detector is configured.Payment-only server admission was checked against nansen-api main at
065bfbd7da0484826d163395f1eacb10db3c3ab7:The automated reviewer check passed but raised two new suggestions. Writer assessment: payment-disabled session guidance correctly says no payment was attempted; recommending wallet funding there would be misleading for non-paying endpoints. The instance and per-request flags are independent disable controls, as the comment and tests state. Both are retained. The separate Codier review approved this implementation.
AI execution metadata
Writer provider: OpenAI, Codex runtime.
Writer model: GPT-6 family. Exact backend model identifier is not exposed in this session.
Thinking level: default, no explicit reasoning override recorded.
Writer token usage: input, cached input, output, reasoning and total counters are unavailable from this runtime; none inferred.
Attribution: implementation, tests, documentation, PR preparation and the subsequent readability cleanup in this session. Writer telemetry remains unavailable for this cumulative scope.
Independent reviewer: Factory Droid, underlying provider Anthropic, exact model
claude-opus-5, high reasoning. Runtime init verified for session9a076091-4253-4560-ae0a-f77d024867b6; separate detached checkout, initial review atfb7a305aa41469e1fc0a73436608e2d8f3b30d16and repair review at31c6de9e47b01ab37e7d3f5bd13fa3dffad9e876. Repair verdict: APPROVE on that exact head, with 402 focused tests and lint passing in the independent checkout. Reviewer usage is separate from writer usage.Initial reviewer invocation reported 62 input, 2,805,695 cache-read input, 134,613 cache-creation input, 33,660 output and 8,426 thinking tokens. Values are reported separately; no total or output/thinking accounting is inferred. The resumed repair invocation reported 74 input, 3,554,878 cache-read input, 290,680 cache-creation input, 44,000 output and 12,021 thinking tokens. These are the runtime counters reported at each completion of the same resumed session; their overlap/cumulative semantics are not established, so they are not summed. All reviewer counters are separate from writer usage.
Fresh reviewer: Factory Droid, underlying provider DeepSeek, exact model
deepseek-v4-pro, high reasoning, session986081bd-a266-4db6-9a41-c352ca65c0ad. Fresh context and separate detached checkout; runtime init verified. GitHub review actorCodier, APPROVED at9af401a094b2958781a4ad14e872dc3a5a9bab8a. This is separate from OpenAI writer and prior Anthropic review usage. Initial completion reported 100,539 input tokens, 1,620,544 cache-read input tokens, 0 cache-creation input tokens and 24,143 output tokens. Reasoning-token and total counters were not exposed; none inferred.The reviewer amended its own review to correct two factual statements, without changing source or approval. The resumed completion reported 177,084 input, 2,210,606 cache-read input, 0 cache-creation input and 26,358 output tokens. These are reported completion counters from the same session; cumulative/overlap semantics are not established, so the two snapshots are not summed.
Follow-up repair scope: CLI help, 23 shipped skill paragraphs, three payment-disable precedence cases, source-contract verification and PR documentation. Same OpenAI writer runtime; writer token counters remain unavailable. DeepSeek approved the new head as Codier; see exact-head review above.
Follow-up independent review: DeepSeek
deepseek-v4-pro, high, verified runtime init in the same reviewer session, exact head2619a1202c89ac22e1a1fe235876cc5e039e1247, submitted as Codier APPROVED. Completion reported 207,939 input, 4,010,040 cache-read input, 0 cache-creation input and 35,305 output tokens; reasoning and total counters not exposed. These are another completion snapshot of the same session and are not summed with prior snapshots. Writer remained hulk-linus[bot], with no identity fallback; writer execution counters remain unavailable.