feat: add structured errors module and home-path collapsing#6
Conversation
Adopt two agent-DX features from axi-sdk-js into the toolkit:
- errors module: CliError (machine-readable code + suggestions),
exitCodeForError (VALIDATION_ERROR -> 2, else 1), and errorOutput/
toErrorOutput for a stable { error, code, help } payload. Exposed via
the new ./errors subpath export.
- output: collapseHomeDirectory(path, homeDir?) replaces a leading home
prefix with ~ (path-boundary aware) for portable, deterministic output.
Wire all validators (text, numeric, output format) to throw CliError
tagged VALIDATION_ERROR. CliError extends Error, so existing try/catch
and message assertions keep working.
Also pin packageManager to bun@1.3.14.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new structured errors module (CliError, exitCodeForError, toErrorOutput) to provide machine-readable error codes and suggestions for CLI tools, and integrates it across existing validation and output modules. It also adds a collapseHomeDirectory utility to normalize absolute home paths to ~. The review feedback suggests improving the robustness of collapseHomeDirectory by handling Windows path separators and trailing slashes, and recommends using duck typing instead of instanceof CliError to prevent failures in multi-instance package environments.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughAdds structured CLI error utilities, migrates validators to ChangesStructured Error System and Output Helpers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 15 files
Architecture diagram
sequenceDiagram
participant Client as CLI Consumer
participant App as App Code
participant Validators as Validators Module
participant Errors as Errors Module
participant Output as Output Module
Note over Client,Output: New: Structured Error Handling Flow
Client->>App: Call command with invalid input
App->>Validators: validateNonEmptyString() / validatePattern() / etc.
alt Validation fails
Validators->>Errors: NEW: throw new CliError(msg, VALIDATION_ERROR)
Errors-->>Validators: CliError instance
Validators-->>App: CliError thrown
App->>Errors: catch(error)
App->>Errors: toErrorOutput(error)
Errors-->>App: { error, code, help? }
alt Validation error (code === VALIDATION_ERROR)
App->>Errors: exitCodeForError(error)
Errors-->>App: 2
else Other CliError or non-CliError
App->>Errors: exitCodeForError(error)
Errors-->>App: 1
end
App->>Output: outputData(errorOutput, format)
Output-->>App: Rendered JSON/TOON error payload
App-->>Client: Stable error output + exit code
else Validation passes
Validators-->>App: Valid result
App-->>Client: Success
end
Note over Client,Output: New: Home Path Collapsing
Client->>App: Request output with absolute path
App->>Output: collapseHomeDirectory(path, homeDir?)
alt Path starts with home directory
Output->>Output: NEW: Check path boundary after prefix
alt Path boundary after home (separator or exact match)
Output-->>App: ~/rest/of/path
else Sibling dir matching basename (e.g. /Users/alicebob vs /Users/alice)
Output-->>App: Original path unchanged
end
else Path outside home directory
Output-->>App: Original path unchanged
end
App->>Output: outputData(result, format)
Output-->>App: Rendered output with collapsed paths
App-->>Client: Portable/deterministic output
Note over Client,Output: Runtime behavior with existing code still works
Note over Validators: CliError extends Error – existing try/catch unbroken
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/output/path.ts (1)
26-38: 🎯 Functional Correctness | 🔵 TrivialNormalize the home separator before comparing
rest.startsWith('/')only handles POSIX separators, and a trailing slash inhomeDirprevents subpaths from collapsing. If this helper should cover Windows too, normalizehomeDirand compare againstpath.sepinstead of hardcoding/.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/output/path.ts` around lines 26 - 38, Update collapseHomeDirectory in path.ts so it does not hardcode POSIX separators when checking the boundary after homeDir. Normalize the homeDir prefix before comparing, and use the platform separator (path.sep) when deciding whether the remaining path is a valid subpath; make sure trailing slashes in homeDir still allow child paths to collapse correctly while preserving the sibling-directory guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Around line 35-36: The build setup is creating separate bundled copies of
CliError via each subpath entrypoint, so the same error class can have different
identities across `@pleaseai/cli-toolkit/errors`, validation, and output. Update
the package exports and build layout so all subpaths share the same CliError
implementation from the errors module, and add explicit types conditions to each
exports entry so TypeScript resolves the correct declarations for subpath
imports. Focus on the package.json exports map and the shared error entrypoints
to avoid multiple bundled definitions.
In `@test/output/path.test.ts`:
- Around line 25-29: The test for collapseHomeDirectory is using
process.env.HOME to derive the expected value, but the function’s default
behavior is based on os.homedir(). Update the assertion in the
collapseHomeDirectory test to use os.homedir() when building the expected path
so the test matches the actual default-home source and stays stable across
environments.
---
Nitpick comments:
In `@src/output/path.ts`:
- Around line 26-38: Update collapseHomeDirectory in path.ts so it does not
hardcode POSIX separators when checking the boundary after homeDir. Normalize
the homeDir prefix before comparing, and use the platform separator (path.sep)
when deciding whether the remaining path is a valid subpath; make sure trailing
slashes in homeDir still allow child paths to collapse correctly while
preserving the sibling-directory guard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b2fe6e1a-5fbc-4c93-88f7-b3e1b101e1e5
📒 Files selected for processing (15)
README.mdpackage.jsonsrc/errors/error.tssrc/errors/index.tssrc/errors/output.tssrc/index.tssrc/output/index.tssrc/output/json.tssrc/output/path.tssrc/validation/numeric.tssrc/validation/text.tstest/errors/error.test.tstest/errors/output.test.tstest/errors/validation-integration.test.tstest/output/path.test.ts
- collapseHomeDirectory: handle Windows \ separator and trailing slash in homeDir - add isCliError duck-typing guard so error detection survives multiple package copies/versions; use it in exitCodeForError and toErrorOutput - package.json: add explicit types conditions to subpath exports - tests: derive default-home expectation from os.homedir(); cover Windows paths and cross-copy CliError detection
Greptile SummaryIntroduces a
Confidence Score: 4/5Safe to merge; changes are backward compatible and well-tested, with two minor design rough edges that don't affect current test cases. The implementation is clean and all 164 tests pass. Two issues are worth tracking: UNKNOWN_ERROR's string value is 'UNKNOWN' (not 'UNKNOWN_ERROR'), creating an asymmetry with VALIDATION_ERROR that could trip up consumers who pattern-match on string literals. Additionally, the ./output bundle now contains its own bundled copy of CliError (because json.ts imports from ../errors/error.ts), so direct instanceof CliError checks across the two subpath imports silently return false — users need to reach for isCliError instead, and that guidance is not yet surfaced where the error is thrown. src/errors/error.ts (UNKNOWN_ERROR value asymmetry) and src/output/json.ts (cross-bundle CliError copy, stale JSDoc inline comment). Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant CLI as CLI Command
participant V as Validator (text/numeric/output)
participant CE as CliError
participant EO as errors/output
participant P as process
CLI->>V: validateNonEmptyString('') / validateFormat('xml') / etc.
V-->>CE: throw new CliError(message, VALIDATION_ERROR)
CE-->>CLI: caught in catch(error)
CLI->>EO: toErrorOutput(error)
EO->>EO: isCliError(error) → true (duck-type or instanceof)
EO-->>CLI: "{ error, code, help? }"
CLI->>P: "process.exitCode = exitCodeForError(error) → 2"
CLI->>CLI: outputData(payload, 'toon')
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant CLI as CLI Command
participant V as Validator (text/numeric/output)
participant CE as CliError
participant EO as errors/output
participant P as process
CLI->>V: validateNonEmptyString('') / validateFormat('xml') / etc.
V-->>CE: throw new CliError(message, VALIDATION_ERROR)
CE-->>CLI: caught in catch(error)
CLI->>EO: toErrorOutput(error)
EO->>EO: isCliError(error) → true (duck-type or instanceof)
EO-->>CLI: "{ error, code, help? }"
CLI->>P: "process.exitCode = exitCodeForError(error) → 2"
CLI->>CLI: outputData(payload, 'toon')
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| * | ||
| * Maps to process exit code `1` via {@link exitCodeForError}. | ||
| */ | ||
| export const UNKNOWN_ERROR = 'UNKNOWN' |
There was a problem hiding this comment.
UNKNOWN_ERROR value doesn't match its export name
VALIDATION_ERROR = 'VALIDATION_ERROR' (name == value), but UNKNOWN_ERROR = 'UNKNOWN' (name has an _ERROR suffix the value lacks). A consumer who writes error.code === 'UNKNOWN_ERROR' — a natural guess from the export name — will never match because the actual runtime value is 'UNKNOWN'. Using the constant itself is always safe, but the asymmetry is a silent footgun when consumers switch to string literals (e.g. in switch cases or JSON comparisons). Aligning the value to 'UNKNOWN_ERROR' would make the two constants consistent.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/errors/error.ts
Line: 14
Comment:
**`UNKNOWN_ERROR` value doesn't match its export name**
`VALIDATION_ERROR = 'VALIDATION_ERROR'` (name == value), but `UNKNOWN_ERROR = 'UNKNOWN'` (name has an `_ERROR` suffix the value lacks). A consumer who writes `error.code === 'UNKNOWN_ERROR'` — a natural guess from the export name — will never match because the actual runtime value is `'UNKNOWN'`. Using the constant itself is always safe, but the asymmetry is a silent footgun when consumers switch to string literals (e.g. in switch cases or JSON comparisons). Aligning the value to `'UNKNOWN_ERROR'` would make the two constants consistent.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Good catch — aligned the value to 'UNKNOWN_ERROR' to match the export name and stay consistent with VALIDATION_ERROR. Since this changes the code field for fallback/unknown errors (a breaking change to the output contract), it's handled in a follow-up PR rather than amending the merged one: #8. Acceptable pre-1.0; consumers using the UNKNOWN_ERROR constant are unaffected.
| import { CliError, VALIDATION_ERROR } from '../errors/error.ts' | ||
| import { filterFields } from './filter.ts' | ||
| import { outputToon } from './toon.ts' |
There was a problem hiding this comment.
instanceof CliError silently returns false
Because src/output/json.ts imports CliError from ../errors/error.ts, Bun will bundle a separate copy of CliError into dist/output/index.js when building the ./output subpath. A consumer who imports from both subpaths simultaneously will hold two distinct class references, so error instanceof CliError (from ./errors) silently returns false for errors thrown by validateFormat (from ./output). The isCliError type guard is the correct fix — but instanceof is the natural TypeScript pattern, and the README's error-handling example doesn't warn against it. Consider adding a note in the JSDoc of validateFormat (or the module-level README section) that direct instanceof CliError checks across subpath boundaries require isCliError instead.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/output/json.ts
Line: 2-4
Comment:
**Cross-bundle `instanceof CliError` silently returns `false`**
Because `src/output/json.ts` imports `CliError` from `../errors/error.ts`, Bun will bundle a separate copy of `CliError` into `dist/output/index.js` when building the `./output` subpath. A consumer who imports from both subpaths simultaneously will hold two distinct class references, so `error instanceof CliError` (from `./errors`) silently returns `false` for errors thrown by `validateFormat` (from `./output`). The `isCliError` type guard is the correct fix — but `instanceof` is the natural TypeScript pattern, and the README's error-handling example doesn't warn against it. Consider adding a note in the JSDoc of `validateFormat` (or the module-level README section) that direct `instanceof CliError` checks across subpath boundaries require `isCliError` instead.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Agreed — the isCliError guard already exists, the gap was documentation. Added a note in the README error-handling section warning against bare instanceof CliError across subpaths (each bundles its own copy) and pointing to isCliError, plus added it to the API reference. Follow-up PR: #8.
1 participant
Summary
Adopts two agent-DX features from
axi-sdk-jsinto the toolkit (features 1 & 2 from the prior analysis):./errorssubpath)CliError—Errorsubclass carrying a machine-readablecode+suggestions[]exitCodeForError(error)— mapsVALIDATION_ERROR→ exit2, everything else →1errorOutput(message, code, suggestions?)/toErrorOutput(error)— produce a stable{ error, code, help? }payload renderable as JSON/TOONVALIDATION_ERROR/UNKNOWN_ERRORconstantscollapseHomeDirectory(path, homeDir?)(output module) — replaces a leading home prefix with~, path-boundary aware (a sibling like/Users/alicebobis not collapsed against home/Users/alice), for portable/deterministic output.All existing validators (
validation/text.ts,validation/numeric.ts,output/json.tsformat check) now throwCliErrortaggedVALIDATION_ERROR. SinceCliError extends Error, existingtry/catchandtoThrow(message)assertions keep working unchanged.Also pins
packageManagertobun@1.3.14.Why
Raises the toolkit's agent-DX: structured, machine-parseable errors with stable exit codes — directly aligned with the
agent-dx-cli-scalerubric (structured JSON errors, safety rails) added in #5.Files
src/errors/{error,output,index}.ts,src/output/path.ts+ testsCliError;package.json(./errorsexport, build steps,packageManager);src/index.ts/src/output/index.tsbarrels;README.mdTest plan
bun test— 164 pass, 0 fail (32 new tests incl. sibling-prefix boundary regression + end-to-end catch→code→exit-code across all 7 validators)bun run type-check— cleanbun run lint— cleanbun run build— emitsdist/errors/with.d.ts/review:review-code— 6 aspects; only one Important (README example accuracy), fixed in this branchNotes
CliErroris anError; no message text changed.@byjohann/toon;axi-sdk-jsuses@toon-format/toon— kept the toolkit's existing dependency.Summary by cubic
Adds structured, machine-readable errors and home-path collapsing to make CLI output agent-friendly and portable. Validators now throw
CliError, enabling stable exit codes and consistent JSON/TOON error payloads; path collapsing is boundary-aware and cross-OS../errorsmodule:CliError(code + suggestions),exitCodeForError(validation → 2, else 1),errorOutput/toErrorOutput, andisCliErrorfor robust detection across multiple package copies.text,numeric,validateFormat) now throwCliErrorwithVALIDATION_ERROR.collapseHomeDirectory(path, homeDir?)to replace a leading home prefix with~(path-boundary aware, handles Windows\and trailing slash inhomeDir).Written for commit 9c030e2. Summary will update on new commits.
Summary by CodeRabbit
New Features
collapseHomeDirectoryfor portable~formatting in output.Bug Fixes
Documentation
Tests
Chores