Skip to content

fix(cli): name the field and the read failure in --from request errors - #459

Merged
theCodeDrift merged 2 commits into
mainfrom
worktree-456---can-we
Oct 6, 2026
Merged

theCodeDrift merged 2 commits into
mainfrom
worktree-456---can-we

Conversation

@theCodeDrift

@theCodeDrift theCodeDrift commented Oct 6, 2026 •

Copy link
Copy Markdown
Member

rule create --from, rule improve --from and feedback send --from read a JSON request that an agent usually writes. When that file was wrong, the error did not say which field was wrong or why the file could not be read.

What changed

  • One reader for all three commands. readJsonInput() in src/util/json-input.ts replaces three copies of the same read → parse → validate block.
    • Read failures give the resolved path (which takes --dir into account) and the reason, for example ENOENT: no such file or directory or EISDIR.
    • JSON errors include the parser's message and position.
    • Validation errors name each field path. Issues are separated by ; because zod's messages contain commas, and zod's own Invalid input: lead is dropped per issue so it prints once.
  • rule meta uses the same formatter, so it names fields too.
  • zod's English messages are registered in src/index.ts. zod 4.3.6 declares "sideEffects": false, so the bundler removed its own config(en()) call. As a result every zod issue printed by the built CLI, feedback send included, read as a bare Invalid input. 4.5.4 only lists compile.js as a side effect, so upgrading zod is not a clear fix.

Output from the built CLI after this change:

Error: Invalid input: prompt: expected string, received undefined; successCases.0: expected string, received number
Error: Could not read file "/abs/missing.json": ENOENT: no such file or directory
Error: "/abs/bad.json" is not valid JSON: Expected property name or '}' in JSON at position 1 (line 1 column 2)

Tests

  • New test/json-input.test.ts covers ENOENT, EISDIR, invalid JSON, field paths, and an error on the whole document (labelled payload).
  • test/rule-from.test.ts now runs the built CLI and asserts Invalid input: prompt: expected string, received undefined. That guards both the field path and the locale fix in the bundle.
  • pnpm typecheck and pnpm lint are clean, and all 2,020 CLI tests pass.

Fixes #456

@theCodeDrift

Copy link
Copy Markdown
Member Author

@claude /review

@claude

claude Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Claude finished @theCodeDrift's task in 2m 12s —— View job


Review: fix(cli): name the field and the read failure in --from request errors

Review mode: incremental — read 0 prior review thread(s) before reviewing. The .prior-review.json snapshot contains no review threads, review bodies, or prior top-level comments (only the @claude /review trigger comment itself), so this is the first review of this PR. Nothing to carry forward as addressed/still-open.

  • Read prior-review data (empty)
  • Fetch PR diff and metadata
  • Read full contents of changed files (rules.ts, feedback.ts, index.ts) for context
  • Review correctness, security, performance, test adequacy, clarity
  • Post inline comments
  • Post final summary

Summary

[New] This is a clean, well-scoped fix. Highlights:

  • readJsonInput() in src/util/json-input.ts correctly consolidates the three read→parse→validate blocks, and all three call sites (feedback.ts, rules.ts create/improve) now use the resolved absolute path in error messages rather than the raw args.from value the old code used — a genuine improvement for --dir users, exactly as the PR body claims.
  • readReason()'s stripping of , ${syscall} '${path}' from Node's fs error message is a plain string replace (not a regex), so it's safe against path strings containing regex metacharacters, and it correctly isolates ENOENT/EISDIR from the message.
  • formatZodIssues() correctly falls back to payload for an empty path (top-level issues), joins with ; to avoid colliding with zod's own comma-bearing messages, and is reused by rule meta for consistency.
  • The z.config(z.locales.en()) fix in src/index.ts is placed in the actual bin entry (./dist/index.js, built from src/index.ts), runs at module-evaluation time, and I confirmed there are no module-level (import-time) .parse() calls elsewhere in src/ that could run before this line executes — validation only happens inside command run() handlers, which execute after this line, so the ordering is safe.
  • Test coverage is good: json-input.test.ts covers ENOENT, EISDIR, invalid JSON, per-field paths, and the payload fallback; rule-from.test.ts now spawns the built CLI and asserts on the field-path text, which is the only way to actually guard the sideEffects: false tree-shaking regression (a unit test importing from source wouldn't exercise the bundle at all).
  • Changeset is correctly scoped as patch (bug fix, not new surface).

Minor, non-blocking observation

[New] The composed error text double-prints "Invalid input" for zod's built-in type-mismatch messages, e.g.:

Invalid input: prompt: Invalid input: expected string, received undefined

This comes from readJsonInput's own Invalid input: prefix wrapping a per-issue message that, for zod's default type-check failures, already starts with "Invalid input:" (that's zod's own wording for e.g. expected string, received undefined). It's exactly what the PR body's own example output shows, and it's covered by tests, so this is clearly intentional rather than missed — but for a custom .refine() message (which wouldn't start with "Invalid input:"), the current wrapping reads better. Worth a quick gut-check on whether the outer prefix should be dropped when the first issue's message already begins with "Invalid input:", purely for agent-readability — not a bug, just a legibility nit on messages whose entire purpose is being read by an agent.

No other correctness, security, or performance issues found. No inline comments needed beyond the note above.

@theCodeDrift

Copy link
Copy Markdown
Member Author

Re: @claude — "Minor, non-blocking observation: The composed error text double-prints "Invalid input" for zod's built-in type-mismatch messages…"
#459 (comment)

Fixed in 5dd125f: rather than dropping the outer prefix, formatZodIssues strips zod's own Invalid input: lead per issue, so the line reads Invalid input: prompt: expected string, received undefined and custom messages pass through unchanged (both now asserted in tests).

— AI Coding Agent

rule create/improve and feedback send share one reader that reports the
resolved path with the read error's code, the JSON parser's message, and
each zod issue's field path. The CLI entry registers zod's English locale,
which bundling dropped because zod declares itself side-effect free.

Fixes #456
zod opens its built-in messages with "Invalid input: ", which the caller
already prints as the line's lead. Strip it per issue; custom messages
pass through unchanged.
@theCodeDrift
theCodeDrift force-pushed the worktree-456---can-we branch from 5dd125f to c2a82ea Compare October 6, 2026 04:53
@theCodeDrift
theCodeDrift merged commit cf7bf82 into main Oct 6, 2026
4 checks passed
@theCodeDrift
theCodeDrift deleted the worktree-456---can-we branch October 6, 2026 05:07
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.

rule create/improve input errors don't name the field or the read failure

1 participant