feat(cli): share durable Tempo session state - #713
Conversation
commit: |
e767e35 to
5dcf969
Compare
gakonst
changed the title
f5c2847 to
07a3106
Compare
gakonst
changed the title
07a3106 to
95b59a8
Compare
95b59a8 to
b3c198b
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
@codex review |
| const signature = await account.sign({ | ||
| hash: KeyAuthorization.getSignPayload(authorization), | ||
| }) | ||
| const signatureEnvelope = SignatureEnvelope.from(signature) |
| url: z.string().optional().describe('Override the stored resource URL'), | ||
| yes: z.boolean().optional().default(false).describe('Confirm closing every session'), | ||
| }), | ||
| output: z.object({ |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad5fa3e402
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (selection.orphaned || selection.finalize || (selection.all && !selection.cooperative)) | ||
| await discover() | ||
| const selected = selectSessions(list(), selection) |
There was a problem hiding this comment.
When a grace period elapses, the persisted row remains closing until reconcile() runs, but --finalize calls only discover() and then filters for rows already marked finalizable. Since discovery skips existing channel IDs, a channel placed into closing by an earlier request-close is silently omitted from every later bulk finalize unless the user separately runs sessions sync first. Reconcile closing candidates before applying the finalizable filter.
Useful? React with 👍 / 👎.
| const challenge = Challenge.fromResponseList(challengeResponse).find(isTempoSessionChallenge) | ||
| if (!challenge || !challengeMatchesSession(challenge, record)) |
There was a problem hiding this comment.
If a service returns multiple tempo/session challenges, this selects the first one and only afterward checks whether it matches the retained channel. A first offer for another currency, recipient, chain, escrow, or protocol therefore causes cooperative close to fail even when a later offer matches exactly. Search the challenge list using both the session predicate and challengeMatchesSession instead.
Useful? React with 👍 / 👎.
| return withAdministration(options, (administration) => | ||
| administration.list().map(serializeSession), | ||
| ) |
There was a problem hiding this comment.
Without --network or an RPC override, withAdministration creates a mainnet client, and createSessionAdministration().list() always filters rows to that client's chain ID. Consequently plain mppx sessions list hides every retained testnet session, and the same chain binding means an unqualified sessions close --all --yes does not actually consider all stored sessions. Read the store directly for listing, or apply the chain filter only when the caller explicitly selected a network.
Useful? React with 👍 / 👎.
| const scopeKey = record.origin | ||
| ? scopedKey(payer, normalizeOrigin(record.origin || namespace), entryKey(record.entry)) | ||
| : null |
There was a problem hiding this comment.
When multiple retained channels share an origin and payment scope, superseded rows intentionally have scope_key = NULL, but setSession() reconstructs a non-null key for every row with an origin. During sync(), reconciliation processes all retained rows and each update clears the other scope owner, so the least-recently-used superseded channel processed last becomes the channel returned by subsequent automatic get() calls. Preserve whether the row owned the scope instead of promoting every reconciled record.
Useful? React with 👍 / 👎.
| const signature = authorization.signature | ||
| if (!signature || signature.type === 'keychain') { | ||
| if (!signature) { |
There was a problem hiding this comment.
A client that constructs and serializes a key authorization directly can still submit a keychain or multisig signature: the signing helper rejects those envelope types, but verification now checks only that a signature exists and passes the non-primitive envelope to address recovery. This regresses the previous server-side rejection and allows credentials that violate the primitive-signature requirement to be accepted; apply the same envelope-type check here as in signSubscriptionKeyAuthorization().
Useful? React with 👍 / 👎.
| export function defaultChannelDatabasePath(): string { | ||
| return join(homedir(), '.tempo', 'wallet', 'channels.db') | ||
| } |
There was a problem hiding this comment.
Upgrading switches persistence from the released ~/.local/state/mppx/sessions/v1 registry to ~/.tempo/wallet/channels.db, while the old registry implementation is deleted and no remaining code imports its records. Server bootstrap only helps when the original service remains reachable, and CLI discovery scans just the latest 100,000 blocks, so older retained channels can disappear from list/close and leave their deposits inaccessible through the CLI after this patch update. Add a one-time migration or fallback reader before abandoning the old state path.
AGENTS.md reference: AGENTS.md:L152-L154
Useful? React with 👍 / 👎.
| const signingAccount = resolveAccount | ||
| ? await resolveAccount(record.entry.descriptor.authorizedSigner) | ||
| : account | ||
| if (!canSignDescriptor(signingAccount, record.entry.descriptor)) | ||
| throw new Error('Resolved session signer does not control the configured payer account.') |
There was a problem hiding this comment.
The voucher-authority check runs before the cooperative/on-chain branch, so requestClose and withdraw are rejected when the configured payer no longer controls the descriptor's delegated authorizedSigner. Those on-chain helpers submit descriptor-only transactions from the payer and do not create a voucher signature, making this delegated-key check unnecessary for the escape hatch; otherwise losing an access key also prevents the payer from recovering the channel deposit. Restrict canSignDescriptor to cooperative close and validate only payer control for on-chain operations.
Useful? React with 👍 / 👎.
| if (!row.descriptor_json) throw new Error('v2 channel row is missing its descriptor') | ||
| const descriptor = JSON.parse(row.descriptor_json) as ChannelEntry['descriptor'] | ||
| return { |
There was a problem hiding this comment.
Rows from the shared SQLite database are trusted by casting channel_id and parsing descriptor_json without checking that the descriptor, chain, and escrow derive that ID. Reconciliation reads state using the stored ID, but requestCloseOnChain() and withdrawOnChain() operate on the descriptor, so a malformed or partially updated shared row can pass the state check and then administer a different channel. Validate the computed channel ID before exposing or administering the record.
Useful? React with 👍 / 👎.
2 participants
What this does
This PR moves durable TIP-1034 client session storage and administration out of the CLI-private implementation and into the public Node APIs. The MPPx CLI then consumes those same APIs for normal paid requests and
mppx sessionscommands.Shared Node session APIs
mppx/client/nodewith the payer-scopedcreateSqliteChannelStoreandcreateSessionAdministrationAPIs~/.tempo/wallet/channels.dband remains compatible with existing v2channelsrowsCLI behavior
--session auto,--session new, and explicit--session <channel-id>selectionmppx sessions list,view,sync, andcloseChannelOpenedlogs and reconciles retained rows with canonical precompile state--yesforsessions close --allDurable-state safety
Scope boundaries
store.jsontempo:*accounts remain on the existing Tempo Wallet plugin path; local/private-key accounts use the shared Node SQLite pathThe SQLite-backed Node APIs and persistent session CLI paths require Node.js 22.5 or newer. Other CLI commands retain the package's existing Node.js baseline.
Dependency follow-up included here
2.55.8>=2.55.7, the first published release containing the Tempo relay signature fixValidation
pnpm check:cipnpm check:typespnpm buildpnpm check:types:examplesVITE_TEMPO_NETWORK=none pnpm exec vp test run src/client/node.test.ts --project nodeVITE_TEMPO_NETWORK=none pnpm exec vp test run src/cli/sessions/request.test.ts src/cli/mcp.test.ts --project clisessions close --all --yesThe latest fix commit is signed and verified.