Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
mariano-aguero
changed the title
![justalab-agents[bot]](https://avatars.githubusercontent.com/u/159771991?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings outside the diff
These sit on lines this PR did not change, but this PR is what makes them wrong. They cannot be posted as inline comments.
- [WARNING]
.github:2— Node.js 20 is deprecated. The following actions target Node.js 20 but are being forced to run on Node.js 24: amannn/action-semantic-pull-request@v5. For more information see: https://github.blog/changelog/2025-09-19-deprecation-of-node-20-on-github-actions-runners/
Contributor
|
Thanks Marianno! Most of the changes here address the feedback from the last PR #254 . Approved no comments |
AngeloAyranji
approved these changes
Sep 18, 2026
…nts-batch # Conflicts: # apps/keys-jaw-id/src/lib/session-manager.test.ts
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
A release train for the agentic payments work. Every change here was reviewed and merged into this branch on its own, and they go to main as one so the release cuts once instead of twenty-four times, and so the pieces that lean on each other never sit on main half-applied.
It merges rather than squashes, deliberately:
nx releasereads the subjects underneath, and collapsing them would turn afeat(core)minor, afeat(ui,keys)minor and twentyfix(cli)patches into one bump on whichever package the squashed subject happened to name.What it delivers, in one line: the CLI can be handed to somebody who has no API key, it pays x402 challenges under caps that are counted the way the chain counts them, and its ledger and its locks survive two processes and a bad file.
What
The changes that landed on this branch one at a time, each with its own review, going to main together.
Twenty-six commits sit here without a PR of their own. One is a test in #329's area that goes through the url builder #330 introduced; the rest are what #334 left open and what two review passes over this branch turned up, and they are described under Why:
Why
Most of it is one pass over the x402 payment path in the CLI, and the pieces lean on each other.
#311 gives the ledger's two spend meters the scope each one measures. The per-period figures mirror an on-chain counter, and the chain meters per permission, so they now count over
permissionId. The session total stays per payer, because that ceiling is the user's own. A re-grant landing mid-window is where the two disagreed.#306 bounds the four RPC reads a payment makes while holding the payment lock. They ran on viem's defaults, up to 41 seconds each, and the lock serializes every payment on the machine, so a slow node stalled more than its own payment. #310 is the other half of that lock: its holder rewrites its timestamp every 30s while the work runs, so the staleness threshold could drop from 300s to 90s without a long payment being treated as abandoned. #317 takes the file reads out of the same window, reading the ledger once per payment inside the lock and handing that snapshot to the four functions that sum it. #322 stops that file growing without bound: past two megabytes a payment folds its oldest rows into a checkpoint carrying their totals and moves the originals to an archive, and a fold only covers rows every live cap either counts in full or ignores in full, so every enforced figure comes out the same afterwards.
Three review passes over the whole branch found the rest, and the fixes are worth naming because each one is a number the caps enforce. Compaction folded rows the chain had not answered yet, and a correction finds its row by nonce while a checkpoint carries none, so the ceiling it was holding became permanent. The reconcile batch took its eight slots from the oldest end, so rows that are askable and never answer held every slot forever and the rows a live cap was still counting were never reached; rows now come from both ends, and a row the chain has had a week to answer is given up on, which also lets compaction fold it. An unreadable checkpoint stopped
x402 statusoutright, which is the command a user runs to find out what is wrong; its figures are now a floor with the trouble named in the verdict. A checkpoint folded into a later one reported itself as standing in for one payment rather than three thousand, and that figure is what an agent reads over MCP. The compaction temp file existed world-readable between its write and its chmod. And the payment lock could be lost by a process that was paused rather than dead, which it now says out loud rather than leaving two payers to count the same budget in silence.#325 is what brings an
uptopayment's figure back down to reality. A settled amount arrives as the server's own claim, and until this it was believed: the row is now checked against the transaction the receipt names, reading the transfer in it, so a receipt claiming less than moved cannot talk a cap down. #319 removes the last place where the rule for "which limit binds" was written twice, and #326 warns when the EIP-712 domain in the registry and the token on chain disagree, which is the shape a wrong-chain USDC would take.#308 drops Polygon Amoy from the x402 asset registry, where the permission manager has no code, and derives the other chain lists from the registry instead of keeping four of them in step by hand. The docs commit finishes that: the supported networks table still advertised Amoy, so a reader was being pointed at a chain the CLI refuses, and the table now says what makes a chain listable rather than only which ones are. #305 is what makes the finding trustworthy: the submodule pin was from March and had started answering "not deployed" for chains where the manager has been live for months, Polygon mainnet among them.
#312 reads the payer balance back after a refill confirms and refuses before signing when it is short. The payer is charged the gas for its own refill, so a refill that pulls exactly the shortfall leaves the payment that follows signed for more than the payer holds.
#328, #329 and #330 are the fix for the wall a new session hits. The grant is supposed to leave the spender enough to pay the fee on its first operation, and it was pricing that transfer through a paymaster nothing on the CLI path configures, so the quote came back empty and the transfer was dropped in silence. #328 prices it off JAW's own paymaster, #330 puts the url for it behind one builder rather than three spellings, and #329 refuses the prefund outright when the permission cannot cover a single operation, instead of moving an allowance that buys nothing. This affects every session, not only the ones that pay.
#339 makes the session file say when it does not know.
expirywas typednumberwhile the value is read off a file a person can edit, andexpiry <= nowis false for aNaN, so an unreadable field turned the expiry check off rather than failing it. Widening it tonumber | nullput the compiler to work: it listed seventeen sites, and each answers the unknown case for its own question, because the safe answer differs by who is asking. A payment must not go out under an expiry nobody can read, and a revoke must not skip a permission that may still be live, since skipping the chain there also deletes the local record. It carries one fix that reaches sessions nobody edited:session addrestampedcreatedAtwhen the field was absent, which counts the session total from the present and hands the spend cap a clean slate, so starting a session and replacing one are now separate writes.#332, #333 and #334 take the API key out of the first step. #332 makes
apiKeyoptional in core so a connect can happen without one, #333 has keys hand the CLI the key of a workspace created for it, and #334 keeps that key in its own config field, apart from the user's, so a rotation on the deployment reaches installs that already have one.Four commits finish that. The key lands in
workspaceApiKey, and every site that decides whether a call can happen was readingapiKey: the four the paying path goes through, thensession setup,session status, the MCP session handler, and the transport every x402 chain read is made on.session setupwas the worst of them, and it is the first run of the product: the browser connected, keys handed a key over, and the command then read the field the key had not landed in and aborted with "Connected, but no API key came back". They all go through one resolver now.The other two are about the rotation. The deployment can change that key at any time, so a refusal from the proxy is how an install learns: the session bridge asks a browser that is already paired for the key it is handing out now, rebuilds the account under it, and sends again. Nothing is dropped on the way in, which the first attempt got wrong. Clearing the stored key before connecting, and then not reaching a browser, which is the normal case for an agent, left the install with no key at all and nothing short of
jaw session setupto get one back. And the refresh may never open a browser: it runs inside the payment lock, so a fresh session would sit on an approval nobody is watching while every other payer on the machine waits it out.The last of them is not about the key, and it rides here because a PR to main would release core ahead of the batch it belongs with.
grantPermissionssends its approval on chain and stores it through the relay second, and neither leg was guarded against a missing key, so a relay call refused for want of one leaves a permission approved on chain and absent from the record every reader goes through. It is unreachable today, since a caller with no key cannot reach the proxy at all and the first leg fails first. It becomes reachable the moment requests are served without one, which is what #321 does, so it lands here, ahead of that.revokePermissionneeded nothing: it reads from the relay before it touches the chain, so it already fails in the safe direction.A third review pass over the branch added the last six. Two are one refactor: both x402 front ends assembled the same payment window by hand, seven pieces in the same order written twice, and they had already drifted once when the grant-seeded policy reached the agent and not the terminal, so one session refused at the granted cap in one place and paid under the defaults in the other. They now open one window, and the
x402 paycommand got its first test along with it, since the MCP half was covered and the terminal half was not.The other four are corrections the same pass found.
saveConfigtruncated the real file in place, and the bridge turned it into something written on every connect that is handed a rotated key, so the MCP server writing beside a terminal could hand the reader a half-written file and lose the api key, the paymasters and the x402 caps; it goes through a temp file and a rename now, which is whatwriteSessionConfigalready did for the session file and for the same reason.session statusasserted an expiry it had just allowed to be null, so a hand-edited99999999999999crashed the command someone runs precisely because the file is suspect. And the route that serves the CLI its workspace key claimed a bound that does not exist, which is corrected below.The last three on the list come from driving the payment path end to end against the staging API, which turned up money the ledger could not account for. A failed
exactattempt carries a nonce and no transaction, and reconciliation read that as a question the chain cannot answer, so the row kept its ceiling for a week and nothing ever checked whether it had settled. The token answers it:authorizationStatesays whether the authorization was ever consumed, which is the move theuptopath already makes against the Permit2 bitmap. One of three failures on staging came back consumed, withAuthorizationUsedand a 5000-unit transfer in a transaction mined one second after the endpoint answered 400, so that payment settled while the caller got an error and no resource, and the row says so now instead of reading as an attempt that moved nothing. The two that were never consumed stop costing their ceiling, which was 0.01 USDC the caps were holding for no reason.x402 logreconciles too, since it printed figures onlystatushad brought up to date. The fixture change beside them is unrelated to any of that:underReportedcarried a literalat, and a row is given up on a week after it, so the suite turned red on its own once the clock passed that date.The rest sit elsewhere. #307 replaces
Record<hex, Record<string, unknown>>with a declared per-chain shape and deletes the seven casts that restated it. #313 states on each spend row of the grant screen what approving it can cost by the time the permission expires, rather than showing a rate and leaving the multiplication to the user. #309 covers theprefundSpendercapability reachingAccount.grantPermissionsas its seventh positional, which is how it got dropped once with the compiler unable to notice. #304 unblocks the pre-commit hook for anyone on Node 25 or later. #316 and #327 are documentation: the payment modules lose the commentary that narrated their own edit history, and the README describessession setupand x402, which it did not mention at all.How to test
Everything here was reviewed and verified in its own PR, so the check that matters at this level is the integration: the whole graph builds and every suite passes with all of them together, which is what the commands above answer, and
api-checkcarries the newChainCapabilities.By hand, the x402 payment path is where the pieces meet. A
jaw x402 pay --payagainst a server quoting an asset in the registry should pay, refill and append a ledger row, with the caps counted per permission and the payer balance checked after the refill. On an install whose only key is the one the browser filled in, the same run should still refill rather than warn that it cannot, andjaw session setupon a machine with no key at all should reach the grant rather than stopping on the key it was just handed. A payment on Polygon Amoy is refused before anything is signed, and anuptochallenge outside Base and Base Sepolia is refused too.Checklist
nx releasereads, which is the point of the section belowbunx prettier --check .and the run above pass;validateis green on the branch@jaw.id/core's public API changed and the report is part of this branch:etc/core.api.mdcarriesChainCapabilities, added in feat(core): type the capabilities response per chain #307permit2.tsthat still named itMerging
This one needs a merge commit. A squash collapses the subjects into one, and nx release reads only that, so the
feat(core)andfeat(ui,keys)minors and thefix(cli)patches would come out as a single bump on whichever package the subject happens to name.Also at merge
Merging this turns the injected key on. The workspace exists, it holds one key, and
JAW_CLI_API_KEYis already set on the keys deployment. None of that does anything today, because the route that serves it lives on this branch and not on main. From the moment this merges and keys redeploys, every CLI that carries no key of its own is handed that one.That is deliberate rather than an oversight, and the reasoning is worth stating for whoever reads this later. Writes pay for themselves: every userOp is charged to its sender in USDC through the ERC-20 paymaster, always, so abuse of the write path is funded by whoever abuses it, out of an account they do not control. Reads are the asymmetry, since they cost the caller nothing. The decision is to ship without a limit on them and to watch instead, rather than hold the product behind a question about quotas and plans that is bigger than this branch.
Watching is the part that has to be real. The proxy already emits
ASSETS_FETCHED,ACTIVITY_FETCHEDandPERMISSIONS_FETCHED, carrying the api-key id and the workspace. Those two fields are the same for every install once the key is shared, so a graph of them separates nothing: heavy use by many and abuse by one look identical. Whatever alerting is built needs one dimension that tells callers inside a shared workspace apart, and the source address is the one that needs no change to the published package.Clearing the environment variable is the way to ship this with the key off, if that is ever wanted: the route then answers 503, the bridge refuses a CLI that sent no key exactly as main does today, and a CLI carrying its own key is unaffected either way.
That key cannot be held secretly on either side. Anything the browser app carries is inlined into its bundle at build time, and anything the published package carries is public from the day it ships. So the key attributes nobody.
What it authorises is everything its workspace can do, and that is worth stating plainly because an earlier draft of this description said a read limit bounds it. None exists. An api key carries no scope and no quota, and the only check there is, the allowed-domain list, is matched against an
Originthe CLI does not send, so this key's workspace has to be allowlisted with*, which is the value that turns that check off. Anyone narrowing that list will find the CLI stops working. The route's own comment now says this rather than the opposite.Recorded here rather than carried in a conversation, because the step it unblocks is the one that takes the API key out of the CLI onboarding entirely.