Skip to content

Reject malformed numeric CLI options#17

Open
xy200303 wants to merge 4 commits into
stepfun-ai:mainfrom
xy200303:fix/strict-cli-option-parsing
Open

Reject malformed numeric CLI options#17
xy200303 wants to merge 4 commits into
stepfun-ai:mainfrom
xy200303:fix/strict-cli-option-parsing

Conversation

@xy200303

@xy200303 xy200303 commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Reject malformed numeric CLI inputs instead of accepting partial parseInt / parseFloat parses.
  • Reuse the shared non-negative integer parser for step artifacts list --limit.
  • Keep the branch current with main and make the newly merged Windows-sensitive tests pass under normal Windows permissions.

Verification

  • pnpm exec vitest run src/commands/option-parsers.test.ts
  • pnpm exec vitest run packages/utils/src/path.test.ts packages/utils/src/shell.test.ts
  • pnpm exec prettier --check src/commands/option-parsers.ts src/commands/artifacts-command.ts src/commands/option-parsers.test.ts
  • pnpm exec tsc --noEmit --pretty false
  • pnpm check

Notes

@xy200303
xy200303 force-pushed the fix/strict-cli-option-parsing branch from 236395e to fadf4e3 Compare June 12, 2026 05:32
@ZouR-Ma

ZouR-Ma commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the contribution!

To get this ready to merge:

  • Reference an issue in the description with Closes #<number> (our link-check CI requires it; open one first if needed).
  • Make sure pnpm check passes locally (lint, type-check, dependency/dead-code guards, formatting, and tests — also enforced in CI).

Full conventions:
https://github.com/stepfun-ai/Step-Realtime-CLI/blob/main/CONTRIBUTING.md

@github-actions github-actions Bot added area/cli src/cli, src/commands, src/runtime, src/bootstrap, src/*.ts area/utils packages/utils labels Jun 18, 2026
@xy200303

Copy link
Copy Markdown
Contributor Author

Thanks! I've addressed the feedback on this PR:

  • Opened issue Reject malformed numeric CLI options #66 and linked it from the PR description with Closes #66.
  • Updated the PR description with the current verification steps.
  • Confirmed pnpm check passes locally and all PR checks are green.

This should be ready for review/merge when you have a chance.

@ZouR-Ma

ZouR-Ma commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator

Thanks for this PR — plugging the partial-parse hole (previously --limit 12abc was silently accepted as 12) and consolidating the duplicate parser in artifacts-command.ts into the shared module are both exactly the right calls. Two things before this can land:

  1. (Must) Take packages/utils/src/path.test.ts and shell.test.ts out of this PR. Those two files address Windows local-test compatibility, which is unrelated to this PR's topic — and #75 is already handling exactly these two files, using the vi.skipIf pattern that CONTRIBUTING.md prescribes (the process.platform === "win32" ? it.skip : it approach here is the hardcoded platform check the guideline advises against). Dropping them won't break CI — those failures only occur on local Windows without Developer Mode; the GitHub Windows lane is green on main. Acceptance criterion: the file list shrinks to option-parsers, artifacts-command, and their tests.
  2. (Must) Add tests for the new behavior. The existing cases in option-parsers.test.ts only cover fully non-numeric input ("abc"), which the old code rejected too. The rejections this PR actually introduces — "12abc", "3.14xyz", "1e3" as an integer, values beyond the safe-integer range — currently have zero coverage. Please add cases for these. Acceptance criterion: new test cases cover the partial-parse scenarios and pass.

Ping us after you push — once slimmed down, this can land quickly.

@ZouR-Ma

ZouR-Ma commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator

Thanks for this PR — the new theme.test.ts deserves a special mention: a proper WCAG relative-luminance contrast check with well-chosen thresholds (7:1 for body text, 4.5:1 for muted text, 3:1 for structural lines), which gives every future theme change a regression guard. The brightened muted / line values across the five builtin themes and the improved /theme recovery copy are also the right calls. Two things before this lands:

  1. (Must) Rebase onto the latest main and drop the changes to packages/utils/src/path.test.ts and shell.test.ts. Windows test compatibility has been handled by #75, which landed on main using the vi.skipIf pattern that docs/TESTING.md prescribes. The equivalent changes in this branch will conflict on rebase — resolve by keeping main's version and discarding this branch's. Acceptance criterion: the file list shrinks to the three files under src/tui/ (the one-line docs fix is fine to keep).

  2. (Must) Keep the role colors — errors must stay red. The current change renders all badge text in theme.foreground and all rails in the line color. Contrast passes, but the transcript loses its color coding: user/assistant/tool entries become indistinguishable by color and error entries are no longer red, which is not what we want. Please restore per-tone colors in resolveTranscriptBadgeStyle, picking contrast-passing brighter variants per theme — danger stays red, warning stays yellow, success stays green; only brighten the values that are too dark on the badge backgrounds. Also add contrast assertions for these tone colors against the badge backgrounds to theme.test.ts (we'd suggest ≥ 4.5:1), so "role colors and legibility" is guaranteed by tests. Acceptance criteria: across all five themes the danger badge and rail read visibly red, and the new tone-color contrast assertions pass.

Ping us after you push — and we'd suggest running pnpm tui:dev and cycling through the five themes with /theme to sanity-check the look.

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

Labels

area/cli src/cli, src/commands, src/runtime, src/bootstrap, src/*.ts area/utils packages/utils

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reject malformed numeric CLI options

2 participants