Skip to content

Reject malformed keyring integer fields#593

Open
samsamtrum wants to merge 1 commit into
tempoxyz:mainfrom
samsamtrum:reject-malformed-keyring-integers
Open

Reject malformed keyring integer fields#593
samsamtrum wants to merge 1 commit into
tempoxyz:mainfrom
samsamtrum:reject-malformed-keyring-integers

Conversation

@samsamtrum

Copy link
Copy Markdown

The CLI keyring parser was using Number.parseInt for chain_id and expiry, which accepts partial strings like 123abc as 123.

This rejects malformed integer fields instead, so edited or corrupted keyring entries do not get accepted with truncated values.

@vercel

vercel Bot commented May 31, 2026

Copy link
Copy Markdown

@samsamtrum is attempting to deploy a commit to the Tempo Team on Vercel.

A member of the Team first needs to authorize it.

@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: fc4c9a5719

ℹ️ 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".

Comment thread src/cli/keyring.ts
}

function parse(text: string): readonly Entry[] {
export function parse(text: string): readonly Entry[] {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Document the new exported parser

AGENTS.md says “JSDoc on all exports — every exported function, type, and constant gets a JSDoc comment.” This change makes parse exported without adding docs, so the module now violates the documented export contract; either add a JSDoc comment or avoid exporting it just for the test seam.

Useful? React with 👍 / 👎.

Comment thread src/cli/keyring.test.ts
expiry = 2
`

expect(() => Keyring.parse(content)).toThrow('Invalid chain_id')

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Snapshot the thrown error

AGENTS.md Testing Conventions says to use toThrowErrorMatchingInlineSnapshot() for error assertions rather than direct .toThrow() checks. This newly added error test bypasses that convention and only checks a substring, so please snapshot the thrown message in the project’s standard form.

Useful? React with 👍 / 👎.

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.

1 participant