Skip to content

fix(cli): align commands/ error messages with 4-ingredient strategy#1255

Open
John-David Dalton (jdalton) wants to merge 8 commits intomainfrom
jdd/error-msg-commands
Open

fix(cli): align commands/ error messages with 4-ingredient strategy#1255
John-David Dalton (jdalton) wants to merge 8 commits intomainfrom
jdd/error-msg-commands

Conversation

@jdalton
Copy link
Copy Markdown
Contributor

@jdalton John-David Dalton (jdalton) commented Apr 22, 2026

Summary

PR 2 of a multi-PR series that rewrites socket-cli error messages to follow the four-ingredient strategy added in #1254 (What / Where / Saw vs. wanted / Fix).

This PR covers packages/cli/src/commands/. ~20 error messages, 14 source files, 11 test files.

What's fixed

Grouped by the kind of error:

Flag validation (numeric) — same treatment across all:

  • cmd-scan-create.mts--pull-request, --reach-analysis-memory-limit, --reach-analysis-timeout, --reach-concurrency
  • cmd-scan-reach.mts--reach-analysis-memory-limit, --reach-analysis-timeout, --reach-concurrency
  • cmd-scan-list.mts--page, --per-page
  • cmd-organization-dependencies.mts--limit, --offset
  • cmd-audit-log.mts--page, --per-page
  • cmd-threat-feed.mts--per-page

Before: Invalid number value for --pull-request: foo
After: --pull-request must be a non-negative integer (saw: "foo"); pass a number like --pull-request=42

Flag validation (enum)--ecosystems / --reach-ecosystems:

Before: Invalid ecosystem: "foo". Valid values are: npm, pypi, ...
After: --reach-ecosystems must be one of: npm, pypi, ... (saw: "foo"); pass a supported ecosystem like --reach-ecosystems=npm

Missing positional:

  • cmd-ask.mtsQUERY argument
  • handle-config-set.mtsVALUE argument

Interactive TTY required:

  • cmd-login.mts — clarified what "non-interactive" means and pointed at SOCKET_CLI_API_TOKEN

IO / system failures:

  • add-socket-wrapper.mtsfs.appendFile failure now names the file and surfaces the underlying error
  • postinstall-wrapper.mts — names both rc files that were attempted

Invariant / internal:

  • convert-gradle-to-maven.mts — "spawn returned no output" with the binary name
  • coana-fix.mtsfind-vulnerabilities non-array JSON names the actual type received

Error type changes

A bunch of these were throw new Error(...) for what are clearly input-validation failures. Converted to throw new InputError(...) to match the codebase convention called out in CLAUDE.md.

Tests

11 test files updated — switched to regex matching on the new phrasing. Also added a missing InputError export to one vi.mock() in postinstall-wrapper.test.mts.

All 218 test files / 2812 tests in test/unit/commands/ pass.

What I didn't touch (noted for later)

  • cmd-fix.mts uses logger.fail(...) + process.exitCode = 1; return instead of throwing InputError. CLAUDE.md calls this pattern out as discouraged ("doesn't set exit code properly" is actually fine here since process.exitCode works, but throwing is cleaner). Not in scope for this PR — just message phrasing.
  • oops/cmd-oops.mts intentionally throws a vague error — that's the command's purpose.

Test plan

  • CI green
  • Sanity: socket scan create --pull-request foo . prints the new message
  • Sanity: socket scan list --page -1 prints the new message
  • Sanity: socket ask (no args) prints the new message

Note

Low Risk
Low risk: changes are limited to user-facing CLI validation/error messages and switching some throws to InputError/FileSystemError, with minor tightening of numeric integer checks that could reject previously-accepted invalid values.

Overview
Improves Socket CLI command error handling to follow a consistent what/where/saw vs wanted/fix format, replacing several generic Error/logger messages with clearer InputError (and FileSystemError for wrapper setup) across commands like ask, scan, audit-log, threat-feed, organization dependencies, and config set.

Tightens validation for several numeric and enum flags (e.g. requiring positive/non-negative integers and clearer --reach-ecosystems/--ecosystems choices), and updates unit/integration tests to assert on the new phrasing. Also improves a couple of internal/system failure messages (gradle spawn output missing, coana JSON shape) and adds a small lockfile metadata tweak.

Reviewed by Cursor Bugbot for commit 2269113. Configure here.

Rewrites error messages across packages/cli/src/commands/ to follow
the What / Where / Saw vs. wanted / Fix strategy from CLAUDE.md's
new Error Messages section.

Sources:
- scan/cmd-scan-create.mts: 5 throws (ecosystems + 4 numeric flags)
- scan/cmd-scan-reach.mts: 4 throws (ecosystems + 3 numeric flags)
- scan/cmd-scan-list.mts: 2 throws (--page, --per-page)
- organization/cmd-organization-dependencies.mts: 2 throws (--limit, --offset)
- audit-log/cmd-audit-log.mts: 2 throws (--page, --per-page)
- threat-feed/cmd-threat-feed.mts: 1 throw (--per-page)
- fix/cmd-fix.mts: 1 logger.fail (--ecosystems)
- ask/cmd-ask.mts: 1 InputError (missing QUERY)
- login/cmd-login.mts: 1 InputError (non-interactive TTY)
- wrapper/add-socket-wrapper.mts: 1 throw (fs.appendFile failure)
- wrapper/postinstall-wrapper.mts: 1 throw (nested wrapper setup)
- manifest/convert-gradle-to-maven.mts: 1 throw (spawn returned no output)
- fix/coana-fix.mts: 1 throw (coana returned non-array JSON)
- config/handle-config-set.mts: 1 throw (missing VALUE)

Tests updated to match new substrings (regex patterns for easier
future evolution).

All throws previously typed as plain Error that represent user input
validation have been converted to InputError, following the codebase
convention.

Follows strategy landed in #1254.
Switch `(e as Error).message` to `e instanceof Error ? e.message : String(e)` so that when a non-Error value is thrown (strings, objects, null) the error message stays informative instead of becoming 'undefined'.

Same fix as applied to #1260 (iocraft.mts) after Cursor bugbot flagged the pattern on that PR.
Switch the inline `e instanceof Error ? e.message : String(e)` to
getErrorCause(e), matching the fleet helper pattern. Same behavior
for Error throws; non-Error throws now produce 'Unknown error'
instead of '[object Object]'.
@jdalton
Copy link
Copy Markdown
Contributor Author

bugbot run

Comment thread packages/cli/src/commands/wrapper/add-socket-wrapper.mts
Cursor bugbot flagged both add-socket-wrapper.mts and
postinstall-wrapper.mts for rendering I/O errors with the "Invalid
input" title and "Check command syntax with --help" recovery hint
(InputError's display mapping). fs.appendFile failures are permission /
disk / path problems — FileSystemError renders them as "File system
error" with contextual recovery (check file permissions, disk space,
etc.).

Pass the file path (where available) and the ErrnoException code
through so FileSystemError can surface EACCES/ENOSPC/ENOENT-specific
recovery text.

Reported on PR #1255.
The previous commit swapped InputError → FileSystemError in
postinstall-wrapper.mts but left the test's vi.mock factory still
exporting InputError, so the source couldn't resolve its
FileSystemError import under vitest:

  [vitest] No "FileSystemError" export is defined on the
  ".../src/utils/error/errors.mts" mock

Replace the mock's InputError stub with a FileSystemError stub that
matches the real class's shape (path, code, recovery) so the source
code resolves correctly. The existing regex assertion on the error
message is unaffected.
@jdalton
Copy link
Copy Markdown
Contributor Author

bugbot run

Comment thread packages/cli/src/commands/wrapper/add-socket-wrapper.mts
Comment thread packages/cli/src/commands/scan/cmd-scan-create.mts
Two issues flagged on the fresh review of HEAD:

- FileSystemError duplicates the filepath: the message embeds ${file}
  AND the constructor receives `file` as the path argument. Since
  display.formatErrorForDisplay appends \`(\${error.path})\` automatically,
  this surfaced the path twice. Drop the \${file} interpolation from
  the message — the path arg alone is enough.

- Scan command numeric flag errors overstated validation: \`--pull-request\`
  claimed "non-negative integer" and \`--reach-concurrency\` claimed
  "positive integer," but the checks only rejected NaN. Negatives and
  floats passed through silently. Tighten the validation to match what
  the error messages promise — Number.isInteger + range check at every
  call site (cmd-scan-create.mts for both flags, cmd-scan-reach.mts for
  reach-concurrency).

The third flagged item (FileSystemError vs InputError in the wrapper
commands) was cursor re-flagging a finding already addressed in commit
769170f — the current code already uses FileSystemError. No action.
@jdalton
Copy link
Copy Markdown
Contributor Author

bugbot run

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is ON. A cloud agent has been kicked off to fix the reported issue.

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 2269113. Configure here.

Comment thread packages/cli/test/unit/commands/wrapper/add-socket-wrapper.test.mts Outdated
Previous commit (2269113) dropped the \${file} interpolation from
FileSystemError's message to avoid display.formatErrorForDisplay
double-printing the path (which it appends from error.path). The test
regex still matched the pre-change format — /failed to append socket
aliases to \/etc\/protected-file/ — and wouldn't have matched the new
message.

Assert on the new shape instead:
  - regex matches `failed to append socket aliases (Permission denied)`
  - toMatchObject pins name='FileSystemError' and path='/etc/protected-file'

Drive-by: the existing regex had \./etc\/protected-file/ but the
message never contained that substring after the FileSystemError swap
— so the assertion was silently wrong. Flagged by cursor on #1255.
@jdalton

This comment was marked as resolved.

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