feat: M9 Phase A — API stability surface + error taxonomy#297
feat: M9 Phase A — API stability surface + error taxonomy#297flyingrobots wants to merge 6 commits intomainfrom
Conversation
- test/api-surface.test.js snapshots all 97 public exports (names + types) - CI fails if exports are added, removed, or change type without updating snapshot - CONTRIBUTING.md documents the deprecation protocol (JSDoc, runtime warning, semver) Refs #206
- src/errors.js: GmindError class, ERROR_CATALOG (13 codes), ExitCode enum
- Exit codes: 0=success, 1=general, 2=usage, 3=validation, 4=not-found
- CLI error boundary outputs structured JSON in --json mode:
{ error, errorCode, exitCode, hint, schemaVersion, command }
- Converted all usage-error and not-found paths in bin/git-mind.js and
src/cli/commands.js to throw GmindError with proper codes
- GmindError, ExitCode, ERROR_CATALOG exported from public API
- 12 unit tests for error module, API snapshot updated (100 exports)
- 594 tests passing across 31 files
Refs #207
Summary by CodeRabbitRelease Notes: v5.1.1
WalkthroughCentralized error taxonomy and CLI error handling added: new Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI User
participant CLI as bin/git-mind.js
participant Cmd as src/cli/commands.js
participant Err as src/errors.js<br/>GmindError
participant Out as Output (JSON or stderr)
User->>CLI: invoke `git mind <cmd>` (--json optional)
CLI->>Cmd: dispatch command
Cmd->>Err: throws GmindError (e.g., GMIND_E_NOT_FOUND)
Err->>Err: lookup ERROR_CATALOG → set exitCode, hint
Cmd->>CLI: propagate error
CLI->>CLI: handleError / handleCommandError(err, {json})
alt opts.json === true
CLI->>Out: emit JSON envelope (err.toJSON())
else
CLI->>Out: print human-readable message to stderr
end
CLI->>User: set process.exitCode (err.exitCode)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3add2f010
ℹ️ 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".
bin/git-mind.js
Outdated
| if (command) { | ||
| console.error(`Unknown command: ${command}\n`); | ||
| handleError(new GmindError('GMIND_E_UNKNOWN_CMD', `Unknown command: ${command}`)); |
There was a problem hiding this comment.
The unknown-command path calls handleError(...) without forwarding whether --json was requested, so git mind <bad-command> --json falls back to plain-text output plus usage text instead of a structured error envelope. This breaks CLI consumers that rely on JSON parsing specifically for usage failures, and the same pattern appears in unknown content/extension subcommand branches where --json is parsed but not propagated to handleError.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in a9322e8.
- All three unknown-command
handleErrorcalls now forward{ json }(contentFlags.json,extFlags.json,args.includes('--json')) - Top-level unknown command suppresses usage dump in
--jsonmode so the output is a clean JSON envelope - Added integration tests verifying JSON error envelopes for
git mind <bad-cmd> --json,content <bad-sub> --json, andextension <bad-sub> --json
| throw new GmindError('GMIND_E_NOT_FOUND', | ||
| `No epoch marker found for "${asOf}" or any ancestor`, | ||
| { hint: 'Run "git mind process-commit" to record epoch markers' }, | ||
| ); |
There was a problem hiding this comment.
resolveContext now throws GmindError('GMIND_E_NOT_FOUND', ...), but commands that call it (view, nodes, status, export, doctor) still catch generically and force process.exitCode = 1, so the new taxonomy is dropped in practice. In missing-epoch or missing-observer scenarios (including --json mode), users get plain text and exit 1 instead of a structured not-found error and exit code 4, which undermines the commit’s API contract for error classification.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Addressed in a9322e8.
view,nodes,status,exportCmd, anddoctorcatch blocks now usehandleCommandError(err, { json: opts.json })instead of rawprocess.exitCode = 1handleCommandErrorchecks forGmindError→ propagateserr.exitCode(e.g. exit 4 for NOT_FOUND); wraps non-GmindError asGMIND_E_INTERNAL- Added integration tests:
--observer ghoston view/nodes/status/doctor all return exit code 4 + structured JSON envelope
- Pass --json flag to handleError for unknown command/subcommand handlers - Replace generic catch blocks with handleCommandError in resolveContext commands (view, nodes, status, export, doctor) so GmindError exit codes propagate correctly (e.g. exit 4 for NOT_FOUND instead of exit 1) - Suppress usage dump when --json is active for unknown commands - Add error-propagation integration tests (9 tests)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/cli/commands.js (2)
144-152:⚠️ Potential issue | 🟠 MajorInconsistent error handling:
initbypasseshandleCommandError.This command uses the legacy
console.error(error(...)) + process.exitCode = 1pattern. In--jsonmode (if ever added), this will emit plain text instead of a structured envelope. The same issue affects:link,list,remove,installHooks,processCommitCmd,importCmd,importMarkdownCmd,mergeCmd(catch block),suggest,review(catch block),set(catch block),unsetCmd(catch block),diff,contentSet,contentShow,contentMeta,contentDelete,extensionAdd(catch block),extensionRemove(catch block).Either convert all commands to use
handleCommandErroror document this as intentional technical debt for commands lacking--jsonsupport.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands.js` around lines 144 - 152, The init command currently uses legacy console.error + process.exitCode assignment in its catch block which bypasses the centralized error formatter; update init to call the shared handleCommandError(...) helper instead of console.error so errors are emitted in the structured envelope (and respect --json) — locate the init function and replace its catch handler with a call to handleCommandError(err, { command: 'init' }) (or whatever signature handleCommandError uses in this repo), and apply the same pattern to the other listed commands (link, list, remove, installHooks, processCommitCmd, importCmd, importMarkdownCmd, mergeCmd, suggest, review, set, unsetCmd, diff, contentSet, contentShow, contentMeta, contentDelete, extensionAdd, extensionRemove) or explicitly document that those commands intentionally lack --json support if you choose not to convert them.
466-469:⚠️ Potential issue | 🟡 Minor
atcommand: catch block uses legacy error handling.The
atcommand correctly throws/handlesGmindErrorfor the usage and not-found early-return paths (lines 428, 437-442) but the catch block at line 467 reverts to the oldconsole.errorpattern. This inconsistency means exceptions thrown byinitGraph,getEpochForRef, orcomputeStatuswon't emit JSON envelopes.🔧 Proposed fix
} catch (err) { - console.error(error(err.message)); - process.exitCode = 1; + handleCommandError(err, { json: opts.json }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands.js` around lines 466 - 469, The catch block for the "at" command currently falls back to console.error which breaks the JSON envelope error contract; replace that legacy handling so any exception from initGraph, getEpochForRef, or computeStatus is emitted via the same JSON error-emission helper used elsewhere (i.e., detect if err is a GmindError or wrap it in one, then call the project's JSON error emitter function used by other commands rather than console.error), and keep setting process.exitCode = 1 after emission so callers still see failure.package.json (1)
44-46:⚠️ Potential issue | 🟡 MinorNode.js version inconsistency with CONTRIBUTING.md.
package.jsonspecifiesnode >= 22.0.0, but CONTRIBUTING.md states "Node.js >= 20.0.0" in Prerequisites. These must agree or downstream contributors will be confused.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 44 - 46, The node version requirement is inconsistent: package.json's "engines": { "node": ">=22.0.0" } conflicts with CONTRIBUTING.md's "Node.js >= 20.0.0" in the Prerequisites; pick one source of truth and make both match — either update package.json's engines.node to ">=20.0.0" to align with CONTRIBUTING.md, or update the Prerequisites text in CONTRIBUTING.md to "Node.js >= 22.0.0" so it matches package.json; ensure both the "engines" field and the CONTRIBUTING.md Prerequisites line are changed accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/git-mind.js`:
- Around line 118-138: handleError currently hardcodes command: 'error' in the
JSON envelope and reads err.message directly for non-GmindError throws; update
handleError to accept/consume an attempted command name (e.g., opts.command) and
include that value in the JSON output (falling back to process.argv[2] or
'unknown' if not provided) so the envelope reflects which CLI command failed,
and normalize the error message for non-GmindError cases by deriving a safe
string (e.g., use typeof err === 'string' ? err : err.message ?? String(err))
before logging or setting process.exitCode; reference handleError, GmindError,
err, and opts.json when making this change.
- Around line 509-518: The code currently produces no output when --json is
present and no command is given; update the default branch to explicitly emit a
JSON envelope when jsonMode is true and command is falsy (instead of remaining
silent). Locate the default block handling jsonMode/command and add a branch for
(jsonMode && !command) that prints a stable JSON response (e.g., an empty object
or a usage envelope) to stdout via console.log(JSON.stringify(...)) and leaves
process.exitCode as intended; keep existing behavior for non-json mode (calling
printUsage and setting exit codes) and reuse existing symbols jsonMode, command,
handleError, printUsage, and process.exitCode to implement the change.
In `@CHANGELOG.md`:
- Around line 8-16: Update the changelog to match package.json version 5.1.1:
replace the "[5.1.0] - 2026-02-25" header with "[5.1.1] - 2026-02-25" (or move
the notes from the "[Unreleased]" section under a new "[5.1.1]" release) and
remove or adjust the "[Unreleased]" section accordingly; also add the missing
"[Unreleased]" reference link at the bottom per keepachangelog convention (so
the file contains both an Unreleased link and a [5.1.1] compare/release link).
Ensure you target the CHANGELOG.md headers "[Unreleased]" and "[5.1.0]" and the
package.json version string "5.1.1" when making the edits.
In `@CONTRIBUTING.md`:
- Around line 104-111: The markdown fenced code blocks around the deprecation
example need a blank line before and after the triple backticks and must include
a language specifier to satisfy MD031/MD040; update the block showing the
runtime warning so it is surrounded by blank lines and use a language tag (e.g.,
```text) for the console.warn example for the oldFunction deprecation message,
leaving the surrounding content and the oldFunction/newFunction names unchanged.
In `@src/cli/commands.js`:
- Around line 362-366: Create a small helper function (e.g.,
throwNotFound(message, hint?)) that constructs the GmindError with code
'GMIND_E_NODE_NOT_FOUND' and delegates to handleCommandError with the same
options object pattern (like { json: opts.json }) so callers no longer inline
new GmindError(...); update the call site in src/cli/commands.js to call
throwNotFound(`Node not found: ${opts.id}`) (passing hint or opts as needed) and
remove the inline new GmindError and explicit handleCommandError call to keep
error creation DRY and consistent.
In `@test/api-surface.test.js`:
- Around line 17-24: Update the snapshot header comment to the correct version:
change the "Last updated: v5.0.0" text to "v5.1.0" so the API_SNAPSHOT comment
reflects that GmindError, ExitCode, and ERROR_CATALOG were added in v5.1.0;
modify the comment immediately above the API_SNAPSHOT constant (reference:
API_SNAPSHOT, symbols GmindError, ExitCode, ERROR_CATALOG) to the new version
string.
In `@test/error-propagation.test.js`:
- Around line 95-107: The test for the JSON NOT_FOUND envelope should also
assert the presence and value of the hint field: after parsing stdout in the
"view --observer <missing> --json returns structured NOT_FOUND envelope" test
(where runCli is called), add an assertion that parsed.hint exists and matches
the expected hint text (or at least is defined/non-empty) to ensure the envelope
includes the spec'd hint for the NOT_FOUND error.
In `@test/errors.test.js`:
- Around line 50-55: The test for GmindError serialization only asserts
errorCode and error; update the test (in the it block that creates new
GmindError('GMIND_E_NOT_FOUND','missing')) to also assert that
JSON.parse(JSON.stringify(err)) does not include hint or exitCode (e.g.,
expect(parsed.hint).toBeUndefined() and expect(parsed.exitCode).toBeUndefined())
and/or assert the full expected envelope shape so the toJSON contract is locked;
locate the GmindError instance and parsed variable in the existing test and add
these assertions there.
---
Outside diff comments:
In `@package.json`:
- Around line 44-46: The node version requirement is inconsistent:
package.json's "engines": { "node": ">=22.0.0" } conflicts with
CONTRIBUTING.md's "Node.js >= 20.0.0" in the Prerequisites; pick one source of
truth and make both match — either update package.json's engines.node to
">=20.0.0" to align with CONTRIBUTING.md, or update the Prerequisites text in
CONTRIBUTING.md to "Node.js >= 22.0.0" so it matches package.json; ensure both
the "engines" field and the CONTRIBUTING.md Prerequisites line are changed
accordingly.
In `@src/cli/commands.js`:
- Around line 144-152: The init command currently uses legacy console.error +
process.exitCode assignment in its catch block which bypasses the centralized
error formatter; update init to call the shared handleCommandError(...) helper
instead of console.error so errors are emitted in the structured envelope (and
respect --json) — locate the init function and replace its catch handler with a
call to handleCommandError(err, { command: 'init' }) (or whatever signature
handleCommandError uses in this repo), and apply the same pattern to the other
listed commands (link, list, remove, installHooks, processCommitCmd, importCmd,
importMarkdownCmd, mergeCmd, suggest, review, set, unsetCmd, diff, contentSet,
contentShow, contentMeta, contentDelete, extensionAdd, extensionRemove) or
explicitly document that those commands intentionally lack --json support if you
choose not to convert them.
- Around line 466-469: The catch block for the "at" command currently falls back
to console.error which breaks the JSON envelope error contract; replace that
legacy handling so any exception from initGraph, getEpochForRef, or
computeStatus is emitted via the same JSON error-emission helper used elsewhere
(i.e., detect if err is a GmindError or wrap it in one, then call the project's
JSON error emitter function used by other commands rather than console.error),
and keep setting process.exitCode = 1 after emission so callers still see
failure.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
CHANGELOG.mdCONTRIBUTING.mdbin/git-mind.jspackage.jsonsrc/cli/commands.jssrc/errors.jssrc/index.jstest/api-surface.test.jstest/error-propagation.test.jstest/errors.test.js
| /** | ||
| * Handle a GmindError at the CLI boundary. | ||
| * Sets exit code and outputs structured JSON when --json is active. | ||
| * Falls back to plain stderr for non-GmindError exceptions. | ||
| * | ||
| * @param {Error} err | ||
| * @param {{ json?: boolean }} [opts] | ||
| */ | ||
| function handleError(err, opts = {}) { | ||
| if (err instanceof GmindError) { | ||
| if (opts.json) { | ||
| console.log(JSON.stringify({ ...err.toJSON(), schemaVersion: 1, command: 'error' }, null, 2)); | ||
| } else { | ||
| console.error(err.message); | ||
| } | ||
| process.exitCode = err.exitCode; | ||
| } else { | ||
| console.error(err.message ?? String(err)); | ||
| process.exitCode = 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hardcoded command: 'error' in JSON envelope loses context about what was attempted.
The command field is statically set to 'error' rather than reflecting the actual CLI command that failed. Consumers parsing these JSON envelopes might benefit from knowing which command triggered the error (e.g., 'view', 'diff', 'content').
If this is a deliberate schema decision (always emit 'error' for error responses), then consider documenting why. Otherwise, consider threading the attempted command name through.
Additionally, for non-GmindError exceptions (line 135), you're accessing err.message directly—if err is a string thrown raw (e.g., throw 'oops'), this will be undefined. The ?? String(err) fallback handles it, but it's a subtle edge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/git-mind.js` around lines 118 - 138, handleError currently hardcodes
command: 'error' in the JSON envelope and reads err.message directly for
non-GmindError throws; update handleError to accept/consume an attempted command
name (e.g., opts.command) and include that value in the JSON output (falling
back to process.argv[2] or 'unknown' if not provided) so the envelope reflects
which CLI command failed, and normalize the error message for non-GmindError
cases by deriving a safe string (e.g., use typeof err === 'string' ? err :
err.message ?? String(err)) before logging or setting process.exitCode;
reference handleError, GmindError, err, and opts.json when making this change.
There was a problem hiding this comment.
This is intentional — error envelopes always use command: 'error' to distinguish them from success responses (e.g., command: 'view', command: 'nodes'). Threading the attempted command name would require changes across many code paths and could be a future enhancement, but it's not part of the current error taxonomy scope.
There was a problem hiding this comment.
@flyingrobots, understood—thank you for clarifying the schema design. Using command: 'error' as a discriminator makes sense for consistent envelope structure. I appreciate the explanation!
✏️ Learnings added
Learnt from: flyingrobots
Repo: flyingrobots/git-mind PR: 297
File: bin/git-mind.js:118-138
Timestamp: 2026-02-26T04:58:06.175Z
Learning: In bin/git-mind.js and the error taxonomy, error JSON envelopes always use `command: 'error'` as a discriminator field to distinguish them from success responses (e.g., `command: 'view'`, `command: 'nodes'`). This is intentional schema design.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| handleCommandError( | ||
| new GmindError('GMIND_E_NODE_NOT_FOUND', `Node not found: ${opts.id}`), | ||
| { json: opts.json }, | ||
| ); | ||
| return; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inline GmindError construction is correct but verbose.
Creating new GmindError(...) inline and passing to handleCommandError works, but this pattern repeats. Consider a helper like throwNotFound(message, hint?) for commonly used error codes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/commands.js` around lines 362 - 366, Create a small helper function
(e.g., throwNotFound(message, hint?)) that constructs the GmindError with code
'GMIND_E_NODE_NOT_FOUND' and delegates to handleCommandError with the same
options object pattern (like { json: opts.json }) so callers no longer inline
new GmindError(...); update the call site in src/cli/commands.js to call
throwNotFound(`Node not found: ${opts.id}`) (passing hint or opts as needed) and
remove the inline new GmindError and explicit handleCommandError call to keep
error creation DRY and consistent.
There was a problem hiding this comment.
Acknowledged, but deferring. Only 2-3 callsites use this pattern — adding a helper would be premature abstraction. If the pattern grows, we'll extract it.
There was a problem hiding this comment.
@flyingrobots Understood—YAGNI applies here. Feel free to revisit if the pattern spreads.
- CHANGELOG: add [5.1.1] release section and link definition - test/api-surface.test.js: update stale "Last updated: v5.0.0" → v5.1.0 - CONTRIBUTING.md: fix markdown code block formatting (MD031, MD040) - test/error-propagation.test.js: add hint assertion for NOT_FOUND envelope - test/errors.test.js: add exitCode and hint assertions for JSON.stringify - bin/git-mind.js: add clarifying comment for --json no-command edge case
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/git-mind.js`:
- Around line 126-138: handleError currently prints only err.message for
non-GmindError exceptions, losing the stack trace; modify handleError (the
handleError function) to emit the full stack when available if a debug flag is
set (e.g., opts.debug or process.env.DEBUG), and also include the stack in the
JSON output when opts.json is true; keep existing behavior for normal runs (only
message and exitCode) but when opts.debug is truthy print
console.error(err.stack ?? err.message) for non-GmindError and include a stack
field in the JSON payload for GmindError/non-GmindError cases when opts.json is
enabled.
- Around line 270-273: The call to handleError is using args.includes('--json')
directly which is inconsistent with the rest of the code; instead call
parseFlags on the positional-slice (use parseFlags(args.slice(3)) as elsewhere)
to produce a flags object and pass flags.json to handleError; ensure parseFlags
is invoked before the GmindError branch so handleError(..., { json: flags.json
}) is used and preserves the same behavior while avoiding matching literal
positional values.
In `@CONTRIBUTING.md`:
- Around line 100-107: Insert a blank line before the opening fenced code block
and a blank line after the closing ``` in the "Mark deprecated" example so the
JSDoc example (the /** `@deprecated` Use newFunction() instead. Removal: v6.0.0 */
and export function oldFunction() { ... }) is separated from surrounding list
items ("1. **Mark deprecated**" and "2. **Runtime warning**") to satisfy MD031;
locate the `@deprecated` example in the deprecation protocol section and add the
blank lines immediately before the starting ``` and immediately after the ending
``` of that snippet.
In `@test/error-propagation.test.js`:
- Around line 30-36: The setupGitRepo function duplicates repo-setup logic
across tests; extract it into a shared test helper (e.g., export async function
setupGitRepo from a new or existing test helper module) preserving its behavior
(mkdtemp + git init + git config and return dir), then replace the local
definitions in affected tests (including this file and
test/contracts.integration.test.js) with an import of the shared setupGitRepo to
DRY up the code and keep behavior identical.
- Around line 49-57: Wrap the JSON.parse(stdout) call in the test "git mind
<bad-cmd> --json outputs a structured error envelope" with a defensive try/catch
(or use a small helper) so parsing failures produce an informative assertion
message; catch the SyntaxError, include the raw stdout (and stderr/status) in
the failure message, and rethrow or fail the test using
expect(...).toFail/expect(...).toBeTruthy with that contextual message. Update
the test that calls runCli(['boguscmd','--json'], tempDir) so it still asserts
parsed.errorCode, parsed.exitCode, parsed.error, and parsed.schemaVersion when
parse succeeds, but provides detailed diagnostics when parsing fails.
In `@test/errors.test.js`:
- Around line 88-107: The test currently hardcodes an expected array of error
codes which drifts from ERROR_CATALOG; update the test to derive the expected
set from ERROR_CATALOG (use Object.keys(ERROR_CATALOG) or a Set of its keys) and
assert that the documented list and the catalog are identical in both directions
(e.g., compare lengths and that every documented code is in ERROR_CATALOG and
every ERROR_CATALOG key is in the documented list) so additions/removals will
fail the test; locate the assertions around ERROR_CATALOG in this test and
replace the hardcoded 'expected' array check with a bidirectional comparison
using Object.keys(ERROR_CATALOG) and the documented list variable.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.mdCONTRIBUTING.mdbin/git-mind.jstest/api-surface.test.jstest/error-propagation.test.jstest/errors.test.js
| function handleError(err, opts = {}) { | ||
| if (err instanceof GmindError) { | ||
| if (opts.json) { | ||
| console.log(JSON.stringify({ ...err.toJSON(), schemaVersion: 1, command: 'error' }, null, 2)); | ||
| } else { | ||
| console.error(err.message); | ||
| } | ||
| process.exitCode = err.exitCode; | ||
| } else { | ||
| console.error(err.message ?? String(err)); | ||
| process.exitCode = 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
handleError silently swallows stack traces for non-GmindError exceptions.
When err is not a GmindError, you emit only err.message to stderr. The full stack trace is discarded. In production CLI tools this is often acceptable, but for debugging internal errors during development, you lose valuable diagnostic info.
Consider logging the full stack when a debug flag is present:
🔧 Suggested enhancement
} else {
- console.error(err.message ?? String(err));
+ console.error(err.message ?? String(err));
+ if (process.env.GITMIND_DEBUG) console.error(err.stack);
process.exitCode = 1;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/git-mind.js` around lines 126 - 138, handleError currently prints only
err.message for non-GmindError exceptions, losing the stack trace; modify
handleError (the handleError function) to emit the full stack when available if
a debug flag is set (e.g., opts.debug or process.env.DEBUG), and also include
the stack in the JSON output when opts.json is true; keep existing behavior for
normal runs (only message and exitCode) but when opts.debug is truthy print
console.error(err.stack ?? err.message) for non-GmindError and include a stack
field in the JSON payload for GmindError/non-GmindError cases when opts.json is
enabled.
| handleError(new GmindError('GMIND_E_USAGE', 'Usage: git mind set <nodeId> <key> <value> [--json]', { | ||
| hint: '<value> is positional and required (flags are not valid values)', | ||
| }), { json: args.includes('--json') }); | ||
| break; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Passing args.includes('--json') directly bypasses parseFlags — inconsistent pattern.
You parse flags with parseFlags(args.slice(3)) elsewhere, but here you're doing a raw args.includes('--json') on the full args array. This works, but it's inconsistent with the rest of the codebase where flags.json is used. If someone later adds a positional argument containing the literal string --json, this could misbehave (admittedly pathological).
For consistency, consider using parseFlags result:
♻️ Proposed refactor
case 'set': {
const setNodeId = args[1];
const setKey = args[2];
const setValue = args[3];
+ const setFlags = parseFlags(args.slice(4));
+ const jsonMode = setFlags.json === true;
if (!setNodeId || !setKey || setValue === undefined || setValue.startsWith('--')) {
handleError(new GmindError('GMIND_E_USAGE', 'Usage: git mind set <nodeId> <key> <value> [--json]', {
hint: '<value> is positional and required (flags are not valid values)',
- }), { json: args.includes('--json') });
+ }), { json: jsonMode });
break;
}
- await set(cwd, setNodeId, setKey, setValue, { json: args.includes('--json') });
+ await set(cwd, setNodeId, setKey, setValue, { json: jsonMode });
break;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/git-mind.js` around lines 270 - 273, The call to handleError is using
args.includes('--json') directly which is inconsistent with the rest of the
code; instead call parseFlags on the positional-slice (use
parseFlags(args.slice(3)) as elsewhere) to produce a flags object and pass
flags.json to handleError; ensure parseFlags is invoked before the GmindError
branch so handleError(..., { json: flags.json }) is used and preserves the same
behavior while avoiding matching literal positional values.
| ### Deprecation protocol | ||
|
|
||
| 1. **Mark deprecated** — Add `@deprecated` JSDoc tag with a migration note and | ||
| the target removal version (at least one minor release away): | ||
| ```js | ||
| /** @deprecated Use newFunction() instead. Removal: v6.0.0 */ | ||
| export function oldFunction() { ... } | ||
| ``` |
There was a problem hiding this comment.
Markdown lint warning: fenced code block at line 104 needs a blank line before it.
Static analysis flags MD031 at lines 104 and 107. The deprecation example code block should be surrounded by blank lines:
📝 Proposed fix
1. **Mark deprecated** — Add `@deprecated` JSDoc tag with a migration note and
the target removal version (at least one minor release away):
+
```js
/** `@deprecated` Use newFunction() instead. Removal: v6.0.0 */
export function oldFunction() { ... }
```
+
2. **Runtime warning** — Emit a one-time `console.warn` on first call:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### Deprecation protocol | |
| 1. **Mark deprecated** — Add `@deprecated` JSDoc tag with a migration note and | |
| the target removal version (at least one minor release away): | |
| ```js | |
| /** @deprecated Use newFunction() instead. Removal: v6.0.0 */ | |
| export function oldFunction() { ... } | |
| ``` | |
| ### Deprecation protocol | |
| 1. **Mark deprecated** — Add `@deprecated` JSDoc tag with a migration note and | |
| the target removal version (at least one minor release away): |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 104-104: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 107-107: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CONTRIBUTING.md` around lines 100 - 107, Insert a blank line before the
opening fenced code block and a blank line after the closing ``` in the "Mark
deprecated" example so the JSDoc example (the /** `@deprecated` Use newFunction()
instead. Removal: v6.0.0 */ and export function oldFunction() { ... }) is
separated from surrounding list items ("1. **Mark deprecated**" and "2.
**Runtime warning**") to satisfy MD031; locate the `@deprecated` example in the
deprecation protocol section and add the blank lines immediately before the
starting ``` and immediately after the ending ``` of that snippet.
| async function setupGitRepo() { | ||
| const dir = await mkdtemp(join(tmpdir(), 'gitmind-errprop-')); | ||
| execFileSync('git', ['init'], { cwd: dir, stdio: 'ignore' }); | ||
| execFileSync('git', ['config', 'user.email', 'test@test.com'], { cwd: dir, stdio: 'ignore' }); | ||
| execFileSync('git', ['config', 'user.name', 'Test'], { cwd: dir, stdio: 'ignore' }); | ||
| return dir; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
setupGitRepo is identical to helpers in other test files — consider extracting to shared fixture.
This pattern (mkdtemp + git init + config) appears in multiple test files (e.g., test/contracts.integration.test.js per the relevant snippets). DRY violation, but acceptable for test isolation clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/error-propagation.test.js` around lines 30 - 36, The setupGitRepo
function duplicates repo-setup logic across tests; extract it into a shared test
helper (e.g., export async function setupGitRepo from a new or existing test
helper module) preserving its behavior (mkdtemp + git init + git config and
return dir), then replace the local definitions in affected tests (including
this file and test/contracts.integration.test.js) with an import of the shared
setupGitRepo to DRY up the code and keep behavior identical.
| it('git mind <bad-cmd> --json outputs a structured error envelope', () => { | ||
| const { status, stdout } = runCli(['boguscmd', '--json'], tempDir); | ||
| expect(status).toBe(2); // ExitCode.USAGE | ||
| const parsed = JSON.parse(stdout); | ||
| expect(parsed.errorCode).toBe('GMIND_E_UNKNOWN_CMD'); | ||
| expect(parsed.exitCode).toBe(2); | ||
| expect(parsed.error).toMatch(/Unknown command/); | ||
| expect(parsed.schemaVersion).toBe(1); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
JSON.parse(stdout) without guard will produce an unhelpful error on malformed output.
If the CLI regresses and outputs non-JSON to stdout in --json mode, JSON.parse throws a generic SyntaxError without context. Consider wrapping with a try/catch or using a helper that provides better diagnostics:
🛡️ Defensive parsing suggestion
it('git mind <bad-cmd> --json outputs a structured error envelope', () => {
const { status, stdout } = runCli(['boguscmd', '--json'], tempDir);
expect(status).toBe(2); // ExitCode.USAGE
- const parsed = JSON.parse(stdout);
+ let parsed;
+ try {
+ parsed = JSON.parse(stdout);
+ } catch (e) {
+ expect.fail(`Expected JSON output but got: ${stdout.slice(0, 200)}`);
+ }
expect(parsed.errorCode).toBe('GMIND_E_UNKNOWN_CMD');This is a test robustness nit — the test will still fail on regression, just with a less helpful message.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/error-propagation.test.js` around lines 49 - 57, Wrap the
JSON.parse(stdout) call in the test "git mind <bad-cmd> --json outputs a
structured error envelope" with a defensive try/catch (or use a small helper) so
parsing failures produce an informative assertion message; catch the
SyntaxError, include the raw stdout (and stderr/status) in the failure message,
and rethrow or fail the test using expect(...).toFail/expect(...).toBeTruthy
with that contextual message. Update the test that calls
runCli(['boguscmd','--json'], tempDir) so it still asserts parsed.errorCode,
parsed.exitCode, parsed.error, and parsed.schemaVersion when parse succeeds, but
provides detailed diagnostics when parsing fails.
| it('has entries for all documented error codes', () => { | ||
| const expected = [ | ||
| 'GMIND_E_USAGE', | ||
| 'GMIND_E_UNKNOWN_CMD', | ||
| 'GMIND_E_NOT_INITIALIZED', | ||
| 'GMIND_E_INVALID_ID', | ||
| 'GMIND_E_INVALID_EDGE', | ||
| 'GMIND_E_VALIDATION', | ||
| 'GMIND_E_NODE_NOT_FOUND', | ||
| 'GMIND_E_NOT_FOUND', | ||
| 'GMIND_E_IMPORT', | ||
| 'GMIND_E_EXPORT', | ||
| 'GMIND_E_EXTENSION', | ||
| 'GMIND_E_CONTENT', | ||
| 'GMIND_E_INTERNAL', | ||
| ]; | ||
| for (const code of expected) { | ||
| expect(ERROR_CATALOG).toHaveProperty(code); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hardcoded expected array creates a maintenance burden — consider deriving from ERROR_CATALOG.
This test hardcodes all 13 error codes. If a new code is added to ERROR_CATALOG, this test won't catch it (the "every entry has exit and hint" test at line 75 validates structure but won't fail on missing expected codes). The current approach is fine for locking the set, but consider adding a bidirectional check:
🔒 Suggested enhancement for bidirectional validation
it('has entries for all documented error codes', () => {
const expected = [
'GMIND_E_USAGE',
// ... existing codes ...
'GMIND_E_INTERNAL',
];
for (const code of expected) {
expect(ERROR_CATALOG).toHaveProperty(code);
}
+ // Ensure no undocumented codes exist
+ const actualCodes = Object.keys(ERROR_CATALOG);
+ const unexpected = actualCodes.filter(c => !expected.includes(c));
+ expect(unexpected).toEqual([]);
});This ensures the documented list stays in sync with the actual catalog in both directions.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('has entries for all documented error codes', () => { | |
| const expected = [ | |
| 'GMIND_E_USAGE', | |
| 'GMIND_E_UNKNOWN_CMD', | |
| 'GMIND_E_NOT_INITIALIZED', | |
| 'GMIND_E_INVALID_ID', | |
| 'GMIND_E_INVALID_EDGE', | |
| 'GMIND_E_VALIDATION', | |
| 'GMIND_E_NODE_NOT_FOUND', | |
| 'GMIND_E_NOT_FOUND', | |
| 'GMIND_E_IMPORT', | |
| 'GMIND_E_EXPORT', | |
| 'GMIND_E_EXTENSION', | |
| 'GMIND_E_CONTENT', | |
| 'GMIND_E_INTERNAL', | |
| ]; | |
| for (const code of expected) { | |
| expect(ERROR_CATALOG).toHaveProperty(code); | |
| } | |
| }); | |
| it('has entries for all documented error codes', () => { | |
| const expected = [ | |
| 'GMIND_E_USAGE', | |
| 'GMIND_E_UNKNOWN_CMD', | |
| 'GMIND_E_NOT_INITIALIZED', | |
| 'GMIND_E_INVALID_ID', | |
| 'GMIND_E_INVALID_EDGE', | |
| 'GMIND_E_VALIDATION', | |
| 'GMIND_E_NODE_NOT_FOUND', | |
| 'GMIND_E_NOT_FOUND', | |
| 'GMIND_E_IMPORT', | |
| 'GMIND_E_EXPORT', | |
| 'GMIND_E_EXTENSION', | |
| 'GMIND_E_CONTENT', | |
| 'GMIND_E_INTERNAL', | |
| ]; | |
| for (const code of expected) { | |
| expect(ERROR_CATALOG).toHaveProperty(code); | |
| } | |
| // Ensure no undocumented codes exist | |
| const actualCodes = Object.keys(ERROR_CATALOG); | |
| const unexpected = actualCodes.filter(c => !expected.includes(c)); | |
| expect(unexpected).toEqual([]); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/errors.test.js` around lines 88 - 107, The test currently hardcodes an
expected array of error codes which drifts from ERROR_CATALOG; update the test
to derive the expected set from ERROR_CATALOG (use Object.keys(ERROR_CATALOG) or
a Set of its keys) and assert that the documented list and the catalog are
identical in both directions (e.g., compare lengths and that every documented
code is in ERROR_CATALOG and every ERROR_CATALOG key is in the documented list)
so additions/removals will fail the test; locate the assertions around
ERROR_CATALOG in this test and replace the hardcoded 'expected' array check with
a bidirectional comparison using Object.keys(ERROR_CATALOG) and the documented
list variable.
1 participant
Summary
test/api-surface.test.jssnapshots all 100 public exports (names + types); CI fails on undocumented API changesGmindErrorclass with 13GMIND_E_*error codes,ExitCodeenum, structured JSON error envelopesGmindErrorProblem Statement
--jsonmode had no structured error output — callers got plain stderr textADR Compliance (Required)
Relevant ADR(s)
Compliance Declaration
Exception Request (Required if deviating)
N/A
Architecture Laws Checklist (Hard Gates)
Canonical Truth & Context
--at,--observer,--trust) or deterministically defaulted.Determinism & Provenance
Artifact Hygiene
Contracts & Compatibility
Extension/Effects Safety (if applicable)
Scope Control
Backward Compatibility
--jsonerror output includes structured error envelopeTest Plan (Required)
Unit
Integration
npx vitest run # 594 tests passing across 31 filesDeterminism
Contract/Schema
Policy Gates
# Pre-commit hooks pass on all 3 commitsSecurity / Trust Impact
Performance Impact
Observability / Debuggability
Operational Notes
Linked Issues / Milestones