Skip to content

release: clean changesets and wire release-bot App for Version PR CI#174

Merged
vraspar merged 6 commits into
mainfrom
vraspar/changeset-cleanup
May 23, 2026
Merged

release: clean changesets and wire release-bot App for Version PR CI#174
vraspar merged 6 commits into
mainfrom
vraspar/changeset-cleanup

Conversation

@vraspar
Copy link
Copy Markdown
Contributor

@vraspar vraspar commented May 22, 2026

Summary

Two coupled changes that close the gap to a clean Version PR + auto-running CI.

Changeset cleanup

  • .changeset/config.json: switch ignore from ["x402r-sdk"] (matched no real package) to ["examples"] so the private workspace package stops being versioned into Version PRs.
  • Rewrite the five sdk-authcapture-*.md changesets as consumer-facing release notes. Removes internal migration code blocks, plan-file references, cross-repo PR refs, "Out of scope" sections, and PR-N-of-N markers.

Release-bot App wiring in release.yml

  • Add a Mint release-bot installation token step in both version and publish jobs, using actions/create-github-app-token v3.2.0 with the client-id input.
  • Swap secrets.GITHUB_TOKEN for the App's installation token in both changesets/action calls.
  • Token scope minimized at mint time: contents: write (both jobs) and pull-requests: write (version job only).

Why

Closed Version PR #171 had two structural problems: it pulled examples/ files in (dead-letter ignore entry), and its body was a 500-line concatenation of internal migration prose. Both are fixed at the source.

Separately, the prior GITHUB_TOKEN flow meant the Version PR was opened by github-actions[bot], which by GitHub's anti-recursion design doesn't trigger downstream workflows — so required CI never fired on the Version PR. Switching to a GitHub App installation token makes the PR appear from a non-github-actions[bot] identity, which triggers workflows like a human-opened PR.

The required-CI ruleset bypass list stays empty — the App identity solves the trigger problem without bypass.

Pre-merge requirement

Repo configuration that must be in place before the next workflow_dispatch:

  • Variable: RELEASE_APP_CLIENT_ID (Client ID from the App's General settings page — a string starting with Iv23li or Iv1., distinct from the numeric App ID)
  • Secret: RELEASE_APP_PRIVATE_KEY (App private key .pem content, unchanged from prior round)

The previously-used RELEASE_APP_ID variable can be deleted after the next successful dispatch verifies the new wiring works. (Earlier round of this PR used app-id: ${{ vars.RELEASE_APP_ID }}; updated commit ac86048 switches to client-id: ${{ vars.RELEASE_APP_CLIENT_ID }} to drop the upstream deprecation warning.)

Updated after review

Two reviews landed (one APPROVED, one COMMENTED non-blocking) with 8 inline comments and an audit-gap concern that nobody had verified the rename tables match current SDK exports. Plus a separate forward-looking ask to fix the app-id deprecation now rather than later. Addressed via three new commits (fd722d4, 0ca1126, ac86048).

Audit-driven correctness fixes (grep'd each rename claim against the current SDK source):

  • lift.md: dropped fictitious ReleaseLocked → CaptureLocked bullet — neither error exists in the ABI or source.
  • autocapture-wireformat.md: dropped @x402r/core from the @x402r/evm widening bullet — core has no peerDependencies block at all. Generalized phrasing to "widen @x402r/evm to ..." since @x402r/cli has it as a dep, not peerDep.
  • autocapture-wireformat.md: deleted toPaymentInfo() signature-change bullet — toPaymentInfo was actually removed entirely in this release, not signature-changed.
  • Deleted sdk-authcapture-topaymentinfo-relocation.md — described an intermediate "relocate from core to helpers" state that didn't reach consumers (the companion changeset removes it from helpers too). Net effect for users is captured in reconstruct-payment-info.md, which now includes a bullet about @x402r/core no longer declaring @x402r/evm as a peer dependency.

Style trims per inline comments:

  • lift.md: cut "Clean break — no shims, no back-compat aliases." opening; dropped "(see below)" cross-reference; dropped "(same pattern for charge/capture/void/refund)" parenthetical on slot getters.
  • permit2.md: trimmed "Parallels signReceiveAuthorization" framing and "65-byte EOA signature" byte-layout detail.
  • reconstruct-payment-info.md: trimmed "Handles the wire-to-struct field renames and EIP-3009 / Permit2 branch internally" implementation detail.
  • release.yml: replaced the 8-line top-of-file header (which restated RELEASING.md) with a single-line pointer.

Forward-looking fix in ac86048 (drops a per-run deprecation warning):

  • release.yml: switched both app-id: inputs to client-id:, using new variable RELEASE_APP_CLIENT_ID. actions/create-github-app-token's app-id input is upstream-deprecated; client-id accepts a different value (the App's Client ID, not the App ID — separate identifiers on the App settings page). x402r-notes/sdk/RELEASING.md updated on main to match (variable name in the App-wiring section, troubleshooting check, rotation note).

Verified by re-review:

  • Migration snippet in lift.md compiles cleanly against current payment.getAmounts / payment.capture / payment.voidPayment signatures.
  • cache: pnpm no-share posture is consistent across both release.yml jobs. One residual exposure in ci.yml's test job (Codecov OIDC + cache) has present mitigations (--ignore-scripts, narrow audience, no write to npm/contents). Acceptable as-is.
  • All renamed symbols claimed in changeset bodies confirmed to exist in current packages/*/src/. Old names confirmed absent.
  • app-idclient-id swap surgically two-line; no other variable references in the repo; deprecation-message upstream confirmed against action.yml.

Deferred (not blocking, surfaced explicitly):

  • Optional follow-up: drop cache: pnpm from ci.yml's test job for full match to release.yml posture.

Test plan

  • Pre-commit hook (biome check + build + typecheck) passed locally on all commits.
  • Audit pass: all rename claims grep'd against current SDK source; migration snippet verified to compile.
  • client-id input verified against upstream action.yml and README; new variable name follows peer convention.
  • Maintainer adds RELEASE_APP_CLIENT_ID variable in repo Settings.
  • On merge: dispatch the workflow; verify a Version PR opens authored by the release-bot App (not github-actions[bot]) and ci.yml fires automatically.
  • After CI green on the Version PR, merge it + dispatch the workflow a second time to publish 0.3.0-alpha.0 via npm Trusted Publishing.
  • Verify the resulting Version PR body is short and consumer-facing (no internal plan refs, no examples/ files, no PR-N-of-N markers).
  • After successful dispatch, delete the old RELEASE_APP_ID variable.

🤖 Generated with Claude Code

vraspar and others added 2 commits May 21, 2026 22:52
Fix .changeset/config.json ignore list: the prior "x402r-sdk" value matched
no real package; switch to "examples" so the private workspace package
stops being versioned into Version PRs.

Rewrite the five sdk-authcapture-* changesets as consumer-facing release
notes grounded in peer-changeset style. Removes internal migration code
blocks, plan-file references, cross-repo PR refs, "Out of scope" sections,
and PR-N-of-N markers.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
The default GITHUB_TOKEN cannot trigger downstream workflows from PRs it
opens (GitHub Actions anti-recursion). That meant the auto-generated
Version Packages PR never fired required CI, blocking the alpha release
behind a manual empty-commit workaround.

Mint a GitHub App installation token in both the version and publish
jobs via actions/create-github-app-token v3.2.0, and pass it as
GITHUB_TOKEN to changesets/action. The App identity triggers downstream
workflows like a human-opened PR, so required CI runs automatically.

Requires repo variable RELEASE_APP_ID and secret RELEASE_APP_PRIVATE_KEY
to be configured. Token scope minimized to contents:write (both jobs)
and pull-requests:write (version job only).

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
@vraspar vraspar requested a review from A1igator as a code owner May 22, 2026 07:07
@codecov
Copy link
Copy Markdown

codecov Bot commented May 22, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Fix bug surfaced by industry-standards review: changesets/action and
changeset publish push commits/tags via git CLI under the hood, not via
octokit. With persist-credentials: false on actions/checkout and only
env GITHUB_TOKEN set, the CLI push has no auth — env GITHUB_TOKEN flows
only to octokit calls and user scripts, never to the git push subprocess.

Pass the App installation token to actions/checkout in both version and
publish jobs (token: input). Drop persist-credentials: false so the
token is written to local git config and reused by the CLI push for the
version-bump commit (version job) and tag push (publish job).

Matches the pattern used by other small-monorepo projects that wire
changesets/action with a GitHub App.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
@vraspar
Copy link
Copy Markdown
Contributor Author

vraspar commented May 22, 2026

Review summary

Chained review-sdk + industry-standards check on this PR.

Critical (fixed in a583452)

Bug: `changesets/action` and `changeset publish` push commits/tags via git CLI under the hood, not via octokit. With `persist-credentials: false` on `actions/checkout` and only env `GITHUB_TOKEN` set on the changesets step, the CLI push would have no auth — env `GITHUB_TOKEN` only flows to octokit calls and user scripts, not to the git push subprocess. The version job's commit push and the publish job's tag push would have failed at runtime.

Fix: pass `token: ${{ steps.app-token.outputs.token }}` to `actions/checkout` in both jobs, drop `persist-credentials: false` so the App token persists in local git config and is used by the CLI push. Matches the pattern used by other small TS monorepos wiring `changesets/action` with a custom GitHub App.

Verified clean

  • Pinned SHAs are real: `actions/create-github-app-token@bcd2ba49…` = v3.2.0 tag; `changesets/action@63a615b9…` = v1.8.0 tag.
  • `permission-contents` and `permission-pull-requests` are real inputs on the pinned `create-github-app-token` action.
  • Permission scoping is correctly minimized per job (version needs both; publish needs only `contents:write`).
  • `changesets/action`'s `setupGitUser` (default `true`) handles `git config user.name/email`, so no missing step.
  • `.changeset/config.json` `ignore: ["examples"]` matches the actual `private: true` package at `examples/`.
  • All 5 changeset rewrites preserve front-matter and semver bumps. Linked-group invariant (`core`/`sdk`/`helpers` bumped together) holds. No leftover internal refs (PR-N-of-N markers, plan paths, "Out of scope" sections, cross-repo refs).

Below-threshold nits (not blocking)

  • `app-id` input on `actions/create-github-app-token` carries an upstream deprecation message in favor of `client-id`. Works today; will emit a per-run warning. Worth renaming `RELEASE_APP_ID` → `RELEASE_APP_CLIENT_ID` (different value from App ID — Client ID is a separate identifier on the App settings page) at the next App-config touch.
  • Partial-refund recovery note (`reclaim(paymentInfo)`) was dropped from `sdk-authcapture-lift.md`. Style call — peers keep changelogs terse and surface recovery in docs. Worth ensuring docs cover the 2-tx failure mode.

Maintainer-only verifications

  • The release-bot GitHub App is installed on this repo with `Contents: write`, `Pull requests: write`, `Metadata: read`.
  • `RELEASE_APP_ID` is set as an Actions variable; `RELEASE_APP_PRIVATE_KEY` is set as an Actions secret.
  • Required-CI ruleset bypass list is empty (the App-identity solves the trigger problem without bypass).

Follow-up

`x402r-notes/sdk/RELEASING.md` doesn't yet document the App-token flow, the required repo variable + secret names, or the private-key rotation procedure. That belongs in the runbook update separately (direct-to-main per x402r-notes convention).

A1igator
A1igator previously approved these changes May 22, 2026
Copy link
Copy Markdown
Contributor

@A1igator A1igator left a comment

Choose a reason for hiding this comment

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

Industry-standards / upstream comparison

Compared the release wiring against upstream x402-foundation/x402 (which doesn't use Changesets but ships the same OIDC / Trusted-Publishing primitives) and the two reference changesets/action monorepos (viem, wagmi).

Verdict

Matches or exceeds the industry pattern.

Strengths over viem / wagmi

  • All actions SHA-pinned. viem leaves changesets/action@v1 unpinned on its version job.
  • step-security/harden-runner with egress audit. Neither viem nor wagmi run it.
  • concurrency.cancel-in-progress: false on the publish job. viem and wagmi default to true, which can leave a half-published release.
  • Per-job permission minimization (publish drops pull-requests:write).
  • Upfront publint + attw gate (pnpm test:pkg) before any package publishes. viem and wagmi rely on per-package prepublishOnly, which can leave a partial release if a late package fails.

Intentional divergences (justified)

  • App installation token vs default GITHUB_TOKEN. viem and wagmi use the default because their Version PR isn't gated by required CI. This repo's Version PR is gated, so the App-token path is the changesets/action-recommended pattern for that scenario.
  • workflow_dispatch only (vs push: branches: [main] in viem / wagmi). More conservative; paired with the ref guard.
  • Two-job split vs a single self-selecting job. Granular permissions vs simplicity. Both valid.

Reuse from upstream x402-foundation/x402

Same checkout / pnpm / setup-node action SHAs, same [email protected] pin, same id-token: write + named-environment OIDC pattern, same if: github.ref == 'refs/heads/main' ref guard. Changesets adoption and the two-job split are additive on top of those primitives.

Nits (not blocking)

  1. app-id deprecated in favor of client-id. actions/create-github-app-token is moving the input name upstream; current value works today but emits a per-run warning. Worth renaming RELEASE_APP_ID to RELEASE_APP_CLIENT_ID (different value: Client ID is a separate identifier on the App settings page) at the next App-config touch.

  2. Changeset verbosity. sdk-authcapture-lift.md is ~40 lines of bullets after the rewrite. The reference SDKs keep changesets to 1-3 sentences (wagmi's bump-connect-evm-1-3-1.md is one line). Defensible for an alpha-stage SDK where consumers need migration breadcrumbs; flagging as a style call, not a defect.

Approving

Workflow correctness verified by the prior review-sdk round (commit a583452 fixed the persist-credentials / CLI-push auth bug). No new findings above threshold.


Generated with Claude Code using review-sdk skill

Copy link
Copy Markdown
Contributor Author

@vraspar vraspar left a comment

Choose a reason for hiding this comment

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

Follow-on read of the changesets-cleanup half of this PR plus an industry-norm audit of the release.yml restructure. Everything below is nits — nothing here blocks merge.

Changeset audit verdict

The 5 rewritten changesets sit at the upper-bound peer norm for rename-heavy minors on a typed SDK. Bolded **Breaking** / **New** sub-headers inside a single changeset are real peer practice for monorepos with substantive surface-area moves; the official changesets/action README example uses one-liners, but it's modeling a publish workflow, not changeset prose, and consumer-monorepo practice in the wider ecosystem (large viem/wagmi-style typed-SDK monorepos) skews longer when the change is rename-heavy. The 47-line sdk-authcapture-lift.md is long but proportionate to its fan-out — operator methods + plugin terminology + types/slots/events/errors + chains/addresses + query scoping. The 4-line ts migration code block in lift.md is appropriate because there's no single-call replacement for partial in-escrow refund; readers need the two-call pattern inline.

Five specific cuttable lines flagged below as inline anchors. None of them are wrong, they're just where the prose tips from describing the change into editorializing about it.

release.yml restructure verdict

The restructure diverges from the changesets/action README's "happy path" (one job + push: main + GITHUB_TOKEN) on three axes:

  1. workflow_dispatch trigger instead of push: main.
  2. Two-job split gated on the hasChangesets output.
  3. GitHub App installation token swap in place of GITHUB_TOKEN.

Each divergence is independently community-standard and supported in the action's own docs — the hasChangesets output is explicitly documented as the hook for custom publish gating, and the App-token swap is the standard workaround for the well-known trigger-recursion problem where github-actions[bot]-opened PRs don't fire downstream workflows (so required CI would skip on the Version PR). The combination here is a security-conscious posture, not a whack one. The ref guard (if: github.ref == 'refs/heads/main') on workflow_dispatch is a real defense against dispatch from a stale feature branch minting a token against an unintended ref.

The app-id deprecation flagged in the prior review summary stands — app-id is the v2 input name and v3 prefers application_id, but app-id still works as an alias; not blocking.

On the inline comments

The cache: pnpm removal is defensible (privileged-runner-reading-untrusted-PR-cache is a real poisoning surface) but stricter than peer norm — most monorepo changesets workflows keep cache: pnpm and rely on cache-key scoping to mitigate. Flagging as a deliberate-vs-inherited check, not a fix request.

The 8-line workflow-level header comment plus the "Industry pattern: ..." parenthetical in release.yml lean cosmetic and could move to RELEASING.md — the workflow file is read line-by-line by the next operator on-call, and peer-project justification doesn't help them debug the job.

Audit gap to call out separately

Nobody has re-verified that the rename tables in the changesets actually match the current SDK exports (getRecorderPaymentInfo → getHookPaymentInfo, the recorderCombinator* → hookCombinator* family, the feeRecipient → feeReceiver slot rename, the AUTHORIZE_CONDITION → AUTHORIZE_PRE_ACTION_CONDITION getter family) or that the partial-refund snippet in sdk-authcapture-lift.md compiles against the current payment.capture / payment.voidPayment signatures. The prior review summary on this PR ran a structural check (front-matter preserved, linked-group invariant, no leftover plan-paths) but didn't run the snippets or grep the renames against the source tree. Worth running grep -r for each renamed symbol + tsc --noEmit against an example file before tagging 0.3.0-alpha.0.

Nothing here blocks merge.

Comment thread .changeset/sdk-authcapture-lift.md Outdated
---

Lift SDK to the authCapture contract surface (BackTrackCo/x402r-contracts#34, merged at `9786579`).
Lift the SDK to the authCapture contract surface. Clean break — no shims, no back-compat aliases.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Tone editorializing. Peer changesets describe the change, not their own posture toward it. Suggest cutting the sentence — the **Breaking** headers below already establish that this is a break.

Comment thread .changeset/sdk-authcapture-lift.md Outdated
- `refundPostEscrow()` → `refund()` on both SDK and contract. Renamed helpers: `approvePostEscrowRefund` → `approveRefundAllowance`, `getPostEscrowRefundAllowance` → `getRefundAllowance`.
- `feeRecipient` → `feeReceiver` on `OperatorConfig` (auto-derived from new ABI). `OperatorSlots` return shape from `getOperatorConfig()` also renamed to short-form fields (`authorizeHook`, `captureCondition`, `feeReceiver`, etc.).
- `release()` → `capture()` (SDK + on-chain).
- `refundInEscrow(paymentInfo, amount, data)` → `voidPayment(paymentInfo, data?)`. `void` is full-only and drops the `amount` argument; use partial capture + void remainder for the old partial-refund flow (see below).
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Minor: cross-reference inside a 47-line entry. The migration code block at the bottom is findable without the parenthetical — (see below) is the kind of in-doc nav peers don't write. Suggest dropping the parenthetical.

Comment thread .changeset/sdk-authcapture-lift.md Outdated
- `VoidExecuted` no longer carries an `amount` field.
- `FeesDistributed.arbiterAmount` → `operatorAmount`.
- `ConditionConfig` (constructor plugin-config arg) → `PluginConfig`. Shape is now `{authorize, charge, capture, void, refund} × {PreActionCondition, PostActionHook}` per action plus `feeReceiver` and `feeCalculator`. Use named-field syntax.
- Slot getters renamed: `AUTHORIZE_CONDITION` → `AUTHORIZE_PRE_ACTION_CONDITION`, `AUTHORIZE_RECORDER` → `AUTHORIZE_POST_ACTION_HOOK` (same pattern for charge/capture/void/refund). `FEE_RECIPIENT` → `FEE_RECEIVER`.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the half-collapsed form. Either spell each of the four slot renames out (matches the recorder→hook bullet style above) or trust the reader and drop the parenthetical. The current state is the weakest version — readers either have to imagine the renames or skim past.

- `reconstructPaymentInfoWire(context)` in `@x402r/helpers` — builds the `PaymentInfoWire` JSON form from a verified `SettleResultContext`. Handles the 6 wirestruct field renames and the EIP-3009/Permit2 branch internally.
- `PaymentInfo` is now both a type and a namespace const in `@x402r/core` (re-exported from `@x402r/sdk`). Use `PaymentInfo.fromWire(wire)` to convert a JSON-form `PaymentInfoWire` to the bigint `PaymentInfo`, and `PaymentInfo.toWire(info)` for the reverse.
- `PaymentInfoWire` type in `@x402r/core` — derived from the contract ABI, stays in sync at compile time when the ABI changes.
- `reconstructPaymentInfoWire(context)` in `@x402r/helpers` — builds the wire JSON form from a verified `SettleResultContext`. Handles the wire-to-struct field renames and the EIP-3009 / Permit2 branch internally.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Implementation detail — describes how the helper is internally branched. Consumers care what changed for them, not how the helper switches. Suggest cutting; the **New** bullet above already says what the helper does.

Comment thread .changeset/sdk-authcapture-permit2.md Outdated
- `@x402r/sdk`: re-exports the four `@x402r/core` Permit2 surfaces above.
- `@x402r/cli`: `--asset-transfer-method <eip3009|permit2>` flag — filters `accepts[]` in addition to `--chain`. Errors on unknown value or empty match set with a `Malformed402Error` (exit code 2).
- New scenario: `examples/scenarios/permit2-charge.ts`. Demonstrates the one-time `ERC20.approve(PERMIT2, MAX)` step and an atomic Permit2 charge with balance-delta assertions.
- `@x402r/core/payment/permit2`: `signPermit2Authorization`, `createPermit2ApprovalTx`, `getPermit2AllowanceReadParams`, and the `PERMIT2_ADDRESS` constant. Parallels `signReceiveAuthorization` — returns `{collectorData, tokenCollector}` suitable for `payment.charge` / `payment.authorize`. `collectorData` is the raw 65-byte EOA signature.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reference-doc detail bleeding into changelog. The Parallels signReceiveAuthorization framing and the 65-byte EOA-signature encoding belong in API docs. Suggest trimming to: list the four exports + that they return {collectorData, tokenCollector} suitable for payment.charge / payment.authorize. Drop the byte-layout.

@@ -46,10 +46,30 @@ jobs:
with:
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Re: top-of-file block comment (lines 3-11, not in this diff so anchoring to the next in-diff line). Mostly restates the operator runbook in x402r-notes/sdk/RELEASING.md. Suggest trimming to one line pointing at the runbook — the rest will drift out of sync the first time the runbook changes.

@@ -46,10 +46,30 @@ jobs:
with:
egress-policy: audit
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Re: line 10 — # Industry pattern: changesets/changesets uses the same two-job split. (not in this diff, anchoring nearby). Editorializing about peer projects inside YAML. The justification holds, but it belongs in the PR body or RELEASING.md, not in the workflow file where the next reader on-call has to evaluate it. Suggest moving.

token: ${{ steps.app-token.outputs.token }}
# Full history so changesets can generate complete changelogs.
fetch-depth: 0

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Re: the cache: pnpm removal comment block (lines 79-83 in the version job, repeated 165-169 in the publish job — neither in this diff, anchoring to the closest in-diff line). Confirming this is a deliberate posture, not inherited copy-paste? The poisoning vector is real — privileged release runner deserializing a cache that ci.yml writes to from untrusted PR runs — but most monorepo changesets workflows keep cache: pnpm and rely on the cache-key scoping to mitigate. The ~10s cold-install cost is fine; just want to make sure the choice is intentional and that the same pattern is being applied to any other privileged workflows in the repo, not just this one.

vraspar and others added 2 commits May 22, 2026 14:45
Three correctness fixes flagged by an audit pass that grep'd each
claimed rename against the current SDK source:

- lift.md: drop the "ReleaseLocked → CaptureLocked" bullet. Neither
  error exists in the ABI or source; it was a fictitious rename.
- autocapture-wireformat.md: drop @x402r/core from the @x402r/evm
  range-widening bullet. Core has no peerDependencies block at all.
- autocapture-wireformat.md: drop the toPaymentInfo() bullet entirely.
  toPaymentInfo was removed in this same release (per
  reconstruct-payment-info.md); claiming a signature change is wrong.

Plus restructure of the same release's relocation story:

- Delete sdk-authcapture-topaymentinfo-relocation.md. It describes an
  intermediate "relocate from core to helpers" state that never reaches
  consumers — the companion changeset removes toPaymentInfo from
  helpers too. Net effect for users is captured in the
  reconstruct-payment-info entry.
- reconstruct-payment-info.md: add a new bullet noting that
  @x402r/core no longer declares @x402r/evm as a peer dependency
  (folded in from the deleted file).

Style trims from inline review comments (no semantic change):

- lift.md: cut "Clean break — no shims, no back-compat aliases."
  editorial sentence; the Breaking headers establish it.
- lift.md: drop "(see below)" cross-reference for the migration block.
- lift.md: drop "(same pattern for charge/capture/void/refund)"
  parenthetical on slot getters; two concrete examples are enough.
- permit2.md: drop "Parallels signReceiveAuthorization" framing and
  the "65-byte EOA signature" byte-layout — reference-doc territory.
- reconstruct-payment-info.md: trim "Handles the wire-to-struct field
  renames and EIP-3009 / Permit2 branch internally" — implementation
  detail consumers don't care about.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Drop the 8-line top-of-file block comment that restated the operator
runbook (when to dispatch, what each job does) and added a peer-project
justification line. Both are kept in x402r-notes/sdk/RELEASING.md, which
is the canonical reference. Peer-project justification belongs in PR
history or the runbook, not in YAML where the next operator on-call
has to skim it.

Replace with a single line pointing at the runbook.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
@vraspar
Copy link
Copy Markdown
Contributor Author

vraspar commented May 22, 2026

Re-review chain complete

Addressed both reviews in two new commits.

fd722d4 — changeset content fixes

Audit pass (grep'd each rename claim against the current SDK source) found 3 wrong claims, which this commit fixes:

  • lift.md: dropped fictitious ReleaseLocked → CaptureLocked bullet. Neither error exists in the ABI or source.
  • autocapture-wireformat.md: dropped @x402r/core from the @x402r/evm widening bullet. Core has no peerDependencies block. Generalized phrasing since @x402r/cli uses dep, not peerDep.
  • autocapture-wireformat.md: deleted toPaymentInfo() signature-change bullet. toPaymentInfo was removed entirely in this release.
  • Deleted sdk-authcapture-topaymentinfo-relocation.md — described an intermediate "relocate" state that never reaches consumers. Net effect (@x402r/core no longer declares @x402r/evm peerDep) folded into reconstruct-payment-info.md.

Style trims addressing all 5 changeset inline comments — lift.md opening sentence cut, (see below) dropped, slot-getter parenthetical dropped, permit2.md byte-layout/framing trimmed, reconstruct-payment-info.md implementation detail trimmed.

0ca1126 — release.yml header trim

Replaced the 8-line top-of-file block (restated RELEASING.md + a peer-project justification line) with a single-line pointer to the runbook.

Replies to inline questions

cache:pnpm consistency — confirmed deliberate posture, applied consistently in release.yml (both jobs no-cache). Audit of all workflows: only ci.yml and release.yml exist. One residual: ci.yml's test job pairs id-token: write (Codecov OIDC) with cache: pnpm. Mitigations in place (--ignore-scripts, narrow OIDC audience, no write to npm/contents); other six CI jobs are contents: read only. Acceptable as-is. Optional follow-up to drop the cache from the test job specifically if we want full match.

Migration snippet — verified to compile against current payment.getAmounts / payment.capture / payment.voidPayment signatures. { capturableAmount } destructure works; capture(paymentInfo, bigint, Hex) and voidPayment(paymentInfo) signatures match.

Re-review verdict

Fresh review-sdk pass on HEAD 0ca1126: READY-TO-LAND. All audit fixes re-verified against SDK source; all 8 prior inline comments resolved; no new findings above threshold.

Deferred (not blocking)

  • app-idclient-id rename on actions/create-github-app-token (works today, emits warning; rename at next App-config touch since Client ID is a separate identifier).
  • Drop cache: pnpm from ci.yml's test job (optional, full-match polish).

@A1igator
Copy link
Copy Markdown
Contributor

SDK Review: Round 2

Found 0 new issues since commit a5834521. Verified the round-2 corrections against the SDK source on vraspar/changeset-cleanup.

Resolved since last round:

  • Changeset verbosity (round-1 nit): substantially trimmed. sdk-authcapture-lift.md shortened, editorial sentences cut across the set ("Clean break — no shims", "Parallels signReceiveAuthorization", byte-layout detail, "(same pattern for …)" parentheticals, "(see below)" cross-refs). Closer to viem / wagmi style.

Round-2 corrections I missed in round 1 (verified):

  • Dropped fictitious ReleaseLocked → CaptureLocked rename in sdk-authcapture-lift.md. Confirmed: neither name appears in packages/core/src/abis/generated.ts.
  • Dropped fictitious @x402r/core peerDep-widening claim in sdk-authcapture-autocapture-wireformat.md. Confirmed: packages/core/package.json has no peerDependencies block at all.
  • Dropped fictitious toPaymentInfo() signature-change bullet. Confirmed: toPaymentInfo and ToPaymentInfoReturnType have 0 matches in packages/{core,helpers,sdk}/src/.
  • Deleted sdk-authcapture-topaymentinfo-relocation.md and folded the core-peerDep-drop into sdk-authcapture-reconstruct-payment-info.md. Correct consolidation: the relocation (core → helpers) was intermediate since helpers also removes toPaymentInfo per the reconstruct changeset, so net consumer-facing state lands in one place.

Release.yml trim (0ca11262): drop of the 8-line header comment matches viem / wagmi style (their changesets.yml files have no header comments). Runbook content stays in x402r-notes/sdk/RELEASING.md.

Verified the rest of the rename claims still hold:

Grep-audited 25 claimed renames against packages/*/src/. All matched. Suspicious matches resolved as benign:

  • feeRecipient (23 hits): all on AuthCaptureExtra.feeRecipient (a distinct type from OperatorConfig.feeRecipient, which is correctly renamed to feeReceiver).
  • AuthorizationCreated / ConditionNotMet (test-only hits): in it() descriptions and error-wrapper-input string literals. Stale test descriptions, not source-of-truth references. The ABI in generated.ts carries the new names AuthorizeExecuted and PreActionConditionNotMet as claimed. Pre-existing test cleanup, out of scope for this PR.
  • actions/recorder / getRecorderPaymentInfo etc.: only in stale coverage-final.json. On disk, packages/core/src/actions/hook/ is the live tree.

Still open from round 1 (out of scope here): app-idclient-id deprecation on actions/create-github-app-token. Round-1 scope was "next App-config touch", and this PR is not one.

Self-correction: the round-1 approval shipped without grepping each claimed rename against packages/*/src/. The author's audit pass caught three real correctness issues that would have misled consumers reading the CHANGELOG. Adding that grep step to the review-sdk Dead Code / Stale-docs axis for changeset-content PRs going forward.


Generated with Claude Code using review-sdk skill

A1igator
A1igator previously approved these changes May 22, 2026
Copy link
Copy Markdown
Contributor

@A1igator A1igator left a comment

Choose a reason for hiding this comment

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

Round-2 verified. Approving. See #174 (comment) for round-2 findings (all author corrections verified against SDK source; 0 new issues; round-1 approval dismissed by branch protection on the new commits).

The app-id input on actions/create-github-app-token is deprecated
upstream in favor of client-id. Works today, but emits a per-run
deprecation warning that will eventually become a hard error.

Switch both job invocations from:
  app-id: ${{ vars.RELEASE_APP_ID }}
to:
  client-id: ${{ vars.RELEASE_APP_CLIENT_ID }}

The two inputs accept different values — Client ID is a string like
"Iv23li…" or "Iv1.…", distinct from the numeric App ID. Requires a
new repo variable RELEASE_APP_CLIENT_ID with the Client ID from the
App's General settings page. The old RELEASE_APP_ID variable can be
deleted after the next successful workflow dispatch.

x402r-notes/sdk/RELEASING.md updated in parallel to match (App-token
wiring section, troubleshooting entry, rotation procedure note).

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Copy link
Copy Markdown
Contributor

@A1igator A1igator left a comment

Choose a reason for hiding this comment

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

SDK Review: Round 3

Round-1 nit (app-id deprecation) resolved in ac86048f. 0 new issues.

Verified upstream at the pinned SHA (actions/create-github-app-token@bcd2ba49…, v3.2.0):

  • client-id is an accepted input on action.yml (description: "GitHub App Client ID", required: false).
  • app-id carries deprecationMessage: "Use 'client-id' instead." Confirms the nit was real, not speculative.
  • Both job invocations (version + publish) switched. Variable name RELEASE_APP_IDRELEASE_APP_CLIENT_ID matches the input rename.

Maintainer pre-merge step (blocking): set repo variable RELEASE_APP_CLIENT_ID to the Client ID from the App's General settings page (distinct value from the numeric App ID; Client ID strings start with Iv1. or Iv23li…). If RELEASE_APP_CLIENT_ID is unset on first dispatch after merge, the action fails with empty client-id and no token is minted. The old RELEASE_APP_ID variable can be deleted after the next successful dispatch.

Approving.


Generated with Claude Code using review-sdk skill

@vraspar vraspar merged commit 6f013f2 into main May 23, 2026
9 checks passed
@vraspar vraspar deleted the vraspar/changeset-cleanup branch May 23, 2026 01:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants