Skip to content

fix(cli): retry pending settlements without replacement signing#80

Merged
sebayaki merged 2 commits into
mainfrom
agent/pending-settlement-one-authorization
Jul 13, 2026
Merged

fix(cli): retry pending settlements without replacement signing#80
sebayaki merged 2 commits into
mainfrom
agent/pending-settlement-one-authorization

Conversation

@clawd800

@clawd800 clawd800 commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Retry payment_settlement_pending in-process with the byte-identical signature, idempotency key, method, path, provider, and body.
  • Enforce at most one payment authorization per call invocation.
  • Refuse server-issued replacement challenges without parsing or signing a new payment.
  • Preserve do-not-repay guidance for every ambiguous failure after a signed request is sent.
  • Accept payment_settlement_failed as terminal only when it matches the invocation key and includes paid: false plus safeToStartNewCall: true.
  • Persist neither payment signatures nor upstream response data.

Why

PR #73 could turn settlement uncertainty into a second purchase by signing a fresh authorization with a new idempotency key. This pivot limits recovery to bounded reconciliation of the original authorization and prevents automatic replacement payments.

Companion backend change: Steemhunt/h402-web#609.

Supersedes #73.

Closes #43.

Validation

  • Focused settlement, API, and documentation tests: 32 passed
  • npm test: 175 CLI tests and 14 core tests passed
  • npm run typecheck
  • npm run lint
  • npm run build
  • npm run verify:pack
  • npm run smoke:pack
  • git diff --check

@h1-hunt

h1-hunt commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

GPT Reviewer

Verdict: REJECT

Reviewed head e163e013267b61e61ea8fd6374d4161c9c164f98 against review snapshot base cdee79ec2ec38b7d255ca5047547fe378defceeb using a bounded, tool-free review bundle.

A reused idempotency key can receive an ambiguous historical settlement failure before this process signs, bypassing the new do-not-repay protection and risking another charge.

Findings

  1. MAJOR — Historical settlement failures bypass the do-not-repay guard (packages/cli/src/commands.ts:553)
    • Evidence: The catch block adds uncertainty guidance only when signedRequestSent is true. If a process previously sent an authorization and a later invocation reuses its idempotency key, the later invocation's initial unsigned request can return payment_settlement_failed. A response missing the required matching key, paid: false, or safeToStartNewCall: true then receives no warning because this invocation has not signed yet. The new API test even treats a failure containing none of those proof fields as conclusive. This contradicts the documented rule that only the fully matching proof confirms non-payment and can encourage a second charge.
    • Suggested fix: When an error has code payment_settlement_failed, evaluate its proof regardless of signedRequestSent; add settlement-risk guidance unless isConclusiveSettlementFailure(error, idempotencyKey) succeeds. Add initial-response tests for missing and mismatched proof fields.

Shared exact-snapshot validation

  • diff-check: PASS (0.0s)
  • npm-ci: PASS (1.484s)
  • typecheck: PASS (2.883s)
  • lint: PASS (2.232s)
  • test: PASS (2.452s)
  • build: PASS (2.024s)
  • No paid h402/provider calls were made.

@h1-hunt h1-hunt added the rejected-gpt GPT Reviewer found issues on this PR head label Jul 13, 2026
@h1-hunt

h1-hunt commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Opus Reviewer

Verdict: PASS

Reviewed head e163e013267b61e61ea8fd6374d4161c9c164f98 against review snapshot base cdee79ec2ec38b7d255ca5047547fe378defceeb using a bounded, tool-free review bundle.

This PR replaces the (superseded #73) behavior of re-signing a fresh authorization on settlement uncertainty with in-process reconciliation that resends the byte-identical signed request and refuses server-issued replacement challenges. The implementation is correct, security-conscious, and comprehensively tested.

Key verifications:

  • Single-signing invariant: the payment signature is created exactly once; the retry loop reuses captured paidHeaders and requestBody, guaranteeing byte-identical resends (same PAYMENT-SIGNATURE, idempotency-key, method, path, provider, body). All tests assert signOwsTypedData is called at most once.
  • signedRequestSent is set after signing and before the first send, so pre-send failures get no false settlement-risk guidance while every post-send failure does — matching the stated "guidance for every ambiguous failure after a signed request is sent."
  • Replacement refusal via the x-h402-replacement-idempotency-key header is checked before status/signing on both initial and signed responses; the initial-replacement path never signs.
  • isConclusiveSettlementFailure correctly gates terminal acceptance on matching idempotencyKey + paid:false + safeToStartNewCall:true; unproven/mismatched failures fall back to do-not-repay guidance.
  • No double-append of guidance; payment_settlement_failed is intentionally excluded from the money-sensitive code set so conclusive failures are not mislabeled as uncertain.
  • Documentation (SKILL.md, README.md) accurately mirrors the code; docs.test.ts regexes match both files.
  • Security: this reduces double-charge risk versus fix(cli): resume pending settlement safely #73; retries delegate dedup to the server via the stable key; nothing is persisted.

Deterministic validation (typecheck, lint, build, 175+14 tests) all pass, with no regressions. I found no actionable blocker, major, or minor issues.

Findings

None.

Shared exact-snapshot validation

  • diff-check: PASS (0.0s)
  • npm-ci: PASS (1.484s)
  • typecheck: PASS (2.883s)
  • lint: PASS (2.232s)
  • test: PASS (2.452s)
  • build: PASS (2.024s)
  • No paid h402/provider calls were made.

@h1-hunt h1-hunt added passed-claude Claude Reviewer passed this PR head needs-rework labels Jul 13, 2026
@sebayaki sebayaki removed rejected-gpt GPT Reviewer found issues on this PR head needs-rework passed-claude Claude Reviewer passed this PR head labels Jul 13, 2026
@h1-hunt

h1-hunt commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

GPT Reviewer

Verdict: PASS

Reviewed head 7646146ebdc85473a54da041d25d455666ae6763 against review snapshot base cdee79ec2ec38b7d255ca5047547fe378defceeb using a bounded, tool-free review bundle.

The updated implementation safely performs bounded reconciliation using the byte-identical signed request, creates no replacement authorization, and now conservatively handles historical settlement failures unless all non-payment proof fields match the invocation. Error guidance, documentation, and focused regression tests align with the intended payment-safety contract. Deterministic validation passes.

Findings

None.

Shared exact-snapshot validation

  • diff-check: PASS (0.0s)
  • npm-ci: PASS (1.73s)
  • typecheck: PASS (2.527s)
  • lint: PASS (2.151s)
  • test: PASS (2.599s)
  • build: PASS (1.969s)
  • No paid h402/provider calls were made.

@h1-hunt h1-hunt added the passed-gpt GPT Reviewer passed this PR head label Jul 13, 2026
@h1-hunt

h1-hunt commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Opus Reviewer

Verdict: PASS

Reviewed head 7646146ebdc85473a54da041d25d455666ae6763 against review snapshot base cdee79ec2ec38b7d255ca5047547fe378defceeb using a bounded, tool-free review bundle.

This PR replaces the superseded #73 behavior (re-signing a fresh authorization with a new idempotency key on settlement uncertainty) with in-process reconciliation that resends the byte-identical signed request and refuses server-issued replacement challenges. The implementation is correct, security-conscious, and comprehensively tested.

Key verifications:

  • Single-signing invariant: the payment signature is created exactly once; the retry loop reuses the captured paidHeaders object and requestBody string, guaranteeing byte-identical resends (same PAYMENT-SIGNATURE, idempotency-key, method, path, provider, body). All settlement tests assert signOwsTypedData runs at most once.
  • Bounded retries: fixed delays [250,1000,2000] cap signed sends at 4; only 409 payment_settlement_pending retries; no unbounded loop.
  • Replacement refusal (x-h402-replacement-idempotency-key) is checked before status/signing on the initial response and after every signed send; the initial-replacement path throws before wallet resolution/signing.
  • Historical-settlement-failure guard (the concern the GPT reviewer raised against the first commit, e163e01) is resolved by the second commit: the catch guard is now (signedRequestSent || isPaymentSettlementFailure(error)) && !isConclusiveSettlementFailure(...), so an initial unsigned request returning a non-conclusive payment_settlement_failed (missing proof or mismatched key) still gets do-not-repay guidance. New it.each tests cover both the initial and signed non-conclusive cases and assert no signing occurred.
  • isConclusiveSettlementFailure uses strict === idempotencyKey/=== false/=== true; every missing/coerced field falls to the safe (guidance-added) side, so it cannot falsely mark an uncertain settlement as safe.
  • payment_settlement_failed is intentionally excluded from the money-sensitive code set in api.ts (proof validation delegated to the call command); withSettlementRiskGuidance avoids double-appending via message.includes(...).
  • Documentation (SKILL.md, README.md, cli/README.md) accurately mirrors the code; docs.test.ts regexes match both files across the actual line wrapping.

All deterministic validation passes (typecheck, lint, build, 177 CLI + 14 core tests) with no regressions. I found no actionable blocker, major, or minor issue.

Findings

None.

Shared exact-snapshot validation

  • diff-check: PASS (0.0s)
  • npm-ci: PASS (1.73s)
  • typecheck: PASS (2.527s)
  • lint: PASS (2.151s)
  • test: PASS (2.599s)
  • build: PASS (1.969s)
  • No paid h402/provider calls were made.

@h1-hunt h1-hunt added the passed-claude Claude Reviewer passed this PR head label Jul 13, 2026
@sebayaki
sebayaki marked this pull request as ready for review July 13, 2026 09:26
@sebayaki
sebayaki merged commit 6211aa1 into main Jul 13, 2026
2 checks passed
@sebayaki
sebayaki deleted the agent/pending-settlement-one-authorization branch July 13, 2026 09:26

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7646146ebd

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

if (!isPaymentSettlementFailure(error) || !error.detail || typeof error.detail !== "object") {
return false;
}
const backendError = (error.detail as { error?: unknown }).error;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Accept flat settlement failure proofs

When the backend returns the flat error shape that backendErrorCode already accepts, e.g. { code: "payment_settlement_failed", idempotencyKey, paid: false, safeToStartNewCall: true }, this line ignores the proof because there is no nested error object. isConclusiveSettlementFailure then returns false, so the catch path appends the do-not-repay warning even though the server has confirmed the original authorization was not paid, leaving callers unable to tell that it is safe to start the replacement call.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

passed-claude Claude Reviewer passed this PR head passed-gpt GPT Reviewer passed this PR head

Development

Successfully merging this pull request may close these issues.

Idempotency-key retry guidance can cause a double-charge; the CLI cannot execute the server's own recovery instruction

3 participants