feat: IRONCLAD M9.T1 — CLI JSON Schema contracts#217
Conversation
Add outputJson(command, data) helper that injects schemaVersion: 1 and
command fields into every JSON output. Replace all 15 console.log(JSON.stringify)
call sites. Wrap bare arrays: nodes --json returns { nodes: [...] },
review --json returns { pending: [...] }.
Create 13 Draft 2020-12 JSON Schema files in docs/contracts/cli/ covering every --json command output. Strict additionalProperties: false at top level; open objects for extensible fields (node properties, prefix maps). Add CLI_CONTRACTS.md with version policy, command-to-schema table, programmatic validation example, and migration guide.
Add ajv (devDep) for Draft 2020-12 JSON Schema validation. test/contracts.test.js: 17 unit tests — schema compilation, envelope requirements, sample payloads, optional field handling. test/contracts.integration.test.js: 7 CLI canary tests — execute the real binary and validate output against schemas. 366 tests across 22 files, all green.
Generalize JSON output section in GUIDE.md to cover all --json commands. Add [Unreleased] CHANGELOG entry with breaking change notes and migration snippets for nodes --json and review --json.
Breaking changes to nodes --json and review --json output format (bare arrays → wrapped objects with schemaVersion envelope).
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds 13 JSON Schema files and a human-readable CLI contracts doc, centralizes CLI JSON envelope output via a new helper, adds Ajv as a devDependency, and introduces unit + integration tests that validate CLI Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 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 |
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@docs/contracts/CLI_CONTRACTS.md`:
- Around line 65-71: The doc references "v2.0.0-alpha.5 and earlier" which
appears incorrect; update the version string in CLI_CONTRACTS.md (both
occurrences currently at the two places you noted) to the correct published
prior version (e.g., "v2.0.0-alpha.3 and earlier") or to the accurate range
determined from package.json/tags, ensuring both references match and adding a
brief parenthetical if alpha.5 was never published to avoid confusion; update
the two matching occurrences so the before/after example and surrounding text
are consistent.
- Around line 28-42: Add a new table row mapping the CLI command "diff --json"
to its schema file: include a line similar to the other entries with the command
`diff --json` and link to [`diff.schema.json`](cli/diff.schema.json) so the
Command-to-Schema table includes the diff command and points to
cli/diff.schema.json.
In `@docs/contracts/cli/node-detail.schema.json`:
- Around line 7-11: The node-detail and node-list schemas both set "command":
"nodes", which prevents a router from selecting the right schema by command
alone; update CLI_CONTRACTS.md to document the disambiguation rule: when command
=== "nodes" consumers must inspect the payload shape — choose node-list if a
top-level "nodes" array is present and node-detail if an "id" field is present —
and include examples referencing the node-detail and node-list schemas and the
specific discriminating fields ("nodes" array vs "id") so implementers know to
validate by shape not just the command field.
In `@src/cli/commands.js`:
- Around line 308-317: The JSON output includes recordedAt (epoch.recordedAt)
but the at.schema.json lists "recordedAt" as ["string","null"] without adding it
to the required array; decide whether recordedAt is always present and then
update the schema or the emission: if recordedAt is guaranteed, add "recordedAt"
to the required list in at.schema.json to tighten the contract; if it can be
genuinely absent, leave the schema as-is and ensure outputJson call (in function
outputJson) can emit recordedAt as null/omit it consistently — reference
epoch.recordedAt, outputJson('at', ...) and at.schema.json when making the
change.
In `@test/contracts.integration.test.js`:
- Around line 54-56: The afterAll hook calls rm(tempDir, ...) without ensuring
tempDir was created; if mkdtemp or beforeAll fails tempDir may be undefined and
rm will throw. Update the afterAll handler (the afterAll closure that calls rm)
to guard on tempDir (e.g., check that tempDir is truthy/non-null) before calling
rm, or wrap the rm call in a conditional/try so rm is only invoked when tempDir
was successfully set by beforeAll/mkdtemp; reference the afterAll function,
tempDir variable, beforeAll/mkdtemp flow, and the rm call when making the
change.
- Around line 24-32: The runCli function currently calls JSON.parse(stdout)
directly which yields an unhelpful SyntaxError if the CLI emits non-JSON; wrap
the parse in a try/catch inside runCli, and on failure throw a new Error that
includes the exact command invoked (process.execPath and BIN plus args), the
cwd, and the raw stdout (and optionally stderr if captured) so the test output
shows the failing command and its output for easier debugging; reference the
runCli function to locate where to add the try/catch and enriched error message.
In `@test/contracts.test.js`:
- Around line 159-241: Replace the in-test for-loops that iterate over the
schemas with per-schema table-driven tests using vitest's it.each (or
describe.each) so each schema is run as an individual test; specifically update
the blocks referencing schemas, ajv.compile(schema), VALID_SAMPLES[file], and
the test names like 'every .schema.json compiles as valid JSON Schema', 'every
schema requires schemaVersion', 'every schema requires command', 'every schema
defines schemaVersion as integer const 1', and the sample validation/negative
tests to use it.each(schemas) (or a mapped array of [file, schema]) so failures
report per-file instead of aborting the remaining iterations. Ensure you
preserve existing custom failure messages (e.g., `${file} ...`) and the use of
structuredClone where used.
- Around line 173-178: The test callbacks (e.g., the it block titled "every
.schema.json compiles as valid JSON Schema") are marked async but do not use
await; remove the unnecessary async keyword from these test functions (all
listed it callbacks) so they are plain synchronous callbacks—locate each it(...)
declaration (such as the one compiling schemas with ajv.compile) and delete the
async modifier to avoid the implicit promise wrapper and clarify intent.
| ```bash | ||
| # Before (v2.0.0-alpha.5 and earlier): | ||
| git mind nodes --json | jq '.[]' | ||
|
|
||
| # After: | ||
| git mind nodes --json | jq '.nodes[]' | ||
| ``` |
There was a problem hiding this comment.
Verify the "before" version reference.
Line 66 and 78 reference "v2.0.0-alpha.5 and earlier", but the previous package.json version was 2.0.0-alpha.3. If alpha.5 was never published, this may confuse users trying to identify which versions are affected.
🤖 Prompt for AI Agents
In `@docs/contracts/CLI_CONTRACTS.md` around lines 65 - 71, The doc references
"v2.0.0-alpha.5 and earlier" which appears incorrect; update the version string
in CLI_CONTRACTS.md (both occurrences currently at the two places you noted) to
the correct published prior version (e.g., "v2.0.0-alpha.3 and earlier") or to
the accurate range determined from package.json/tags, ensuring both references
match and adding a brief parenthetical if alpha.5 was never published to avoid
confusion; update the two matching occurrences so the before/after example and
surrounding text are consistent.
| "required": ["schemaVersion", "command", "id", "prefix", "prefixClass", "properties"], | ||
| "additionalProperties": false, | ||
| "properties": { | ||
| "schemaVersion": { "type": "integer", "const": 1 }, | ||
| "command": { "type": "string", "const": "nodes" }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Both node-detail and node-list schemas share command: "nodes" — consumers must disambiguate by payload shape.
This means a generic schema router based solely on the command field can't select the correct schema. Consumers will need to check for the presence of the nodes array vs. the id field. Consider documenting this disambiguation rule in CLI_CONTRACTS.md if not already covered.
🤖 Prompt for AI Agents
In `@docs/contracts/cli/node-detail.schema.json` around lines 7 - 11, The
node-detail and node-list schemas both set "command": "nodes", which prevents a
router from selecting the right schema by command alone; update CLI_CONTRACTS.md
to document the disambiguation rule: when command === "nodes" consumers must
inspect the payload shape — choose node-list if a top-level "nodes" array is
present and node-detail if an "id" field is present — and include examples
referencing the node-detail and node-list schemas and the specific
discriminating fields ("nodes" array vs "id") so implementers know to validate
by shape not just the command field.
| if (opts.json) { | ||
| console.log(JSON.stringify({ | ||
| outputJson('at', { | ||
| ref, | ||
| sha: sha.slice(0, 8), | ||
| fullSha: sha, | ||
| tick: epoch.tick, | ||
| nearest: epoch.nearest ?? false, | ||
| recordedAt: epoch.recordedAt, | ||
| status: statusResult, | ||
| }, null, 2)); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
recordedAt is emitted but not in the schema's required list — confirm this is intentional.
The at.schema.json defines recordedAt as ["string", "null"] but doesn't include it in required. If epoch.recordedAt is always present in practice, consider adding it to required for a tighter contract. If it can genuinely be absent (not just null), the current approach is correct.
🤖 Prompt for AI Agents
In `@src/cli/commands.js` around lines 308 - 317, The JSON output includes
recordedAt (epoch.recordedAt) but the at.schema.json lists "recordedAt" as
["string","null"] without adding it to the required array; decide whether
recordedAt is always present and then update the schema or the emission: if
recordedAt is guaranteed, add "recordedAt" to the required list in
at.schema.json to tighten the contract; if it can be genuinely absent, leave the
schema as-is and ensure outputJson call (in function outputJson) can emit
recordedAt as null/omit it consistently — reference epoch.recordedAt,
outputJson('at', ...) and at.schema.json when making the change.
There was a problem hiding this comment.
Intentionally optional. recordedAt comes from a Map.get() on epoch node properties (src/epoch.js:75). While all current code paths set it, older epoch markers created before the field existed could lack it. The schema correctly models it as optional + nullable to handle both cases without breaking.
There was a problem hiding this comment.
@flyingrobots Thank you for the clarification! That makes perfect sense — modeling it as optional + nullable correctly handles the backward compatibility case where older epoch markers may not have the field. The schema design is spot-on for this scenario.
✏️ Learnings added
Learnt from: flyingrobots
Repo: neuroglyph/git-mind PR: 217
File: src/cli/commands.js:308-317
Timestamp: 2026-02-14T08:13:47.069Z
Learning: In `src/cli/commands.js` and `docs/contracts/cli/at.schema.json`, the `recordedAt` field in the `at` command JSON output is intentionally optional (not in the required list) and nullable because older epoch markers created before the field existed may lack it. The field comes from a `Map.get()` on epoch node properties (src/epoch.js:75), and the schema correctly models backward compatibility by allowing it to be absent or null.
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.
🧠 Learnings used
Learnt from: CR
Repo: neuroglyph/git-mind PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-10T04:44:40.960Z
Learning: Applies to **/cli/**/*.{js,mjs} : Use chalk and figures for terminal output formatting
Learnt from: flyingrobots
Repo: neuroglyph/git-mind PR: 169
File: docs/templates/code-review-feedback.md:26-33
Timestamp: 2025-09-16T17:10:27.477Z
Learning: The user (flyingrobots) uses Obsidian to view/edit Markdown files and prefers to keep Obsidian-specific syntax like collapsible callout markers (e.g., `[!NOTE]-` for collapsible callouts) even if they're not GitHub-compatible.
- Add missing diff --json row to CLI_CONTRACTS.md command table - Fix version references to "prior to v3.0.0" - Add nodes command disambiguation note - Guard afterAll rm against undefined tempDir - Wrap runCli JSON.parse in try/catch for better diagnostics - Remove unnecessary async from test callbacks
|
@coderabbitai review please 🥕 |
|
This looks like a substantial PR with JSON Schema contracts, breaking changes, and comprehensive test coverage. I'll examine the schema definitions, test suite, and migration guidance. 🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@test/contracts.integration.test.js`:
- Around line 85-100: The test assumes listOutput.nodes[0] exists which is
fragile; before reading index 0, add an explicit guard that checks
listOutput.nodes is an array and has length > 0 (e.g., assert
expect(listOutput.nodes.length).toBeGreaterThan(0) or throw a clear Error) so
the failure message indicates the seed fixture is empty or import failed; update
the lines around runCli(['nodes','--json']) / knownId assignment (symbols:
runCli, listOutput.nodes, knownId) to perform that check and only then set
knownId = listOutput.nodes[0].
- Around line 42-56: The beforeAll setup currently calls execFileSync with BIN
and FIXTURE without guarding for a missing fixture or reporting helpful
diagnostics; wrap or precede the import step in a check and/or try/catch to
produce a clear error if FIXTURE is missing or the import fails: before running
execFileSync(process.execPath, [BIN, 'import', FIXTURE], { cwd: tempDir, ... })
verify the fixture file exists (e.g., fs.existsSync(FIXTURE)) and throw or log a
descriptive message including FIXTURE and tempDir if not found, and/or catch
errors from execFileSync to rethrow a new Error that includes the original error
message plus context mentioning BIN, FIXTURE and tempDir so failures during the
import step are easy to diagnose.
In `@test/contracts.test.js`:
- Around line 283-289: The test "import schema accepts dryRun field" currently
mutates VALID_SAMPLES['import.schema.json'] to set sample.dryRun = true, but
since the base sample already contains dryRun: false this doesn't verify the
field is optional; update the test to remove the field instead: use the same
schema and sample variables (schema, sample from
VALID_SAMPLES['import.schema.json']), delete sample.dryRun (or construct a
sample without that key) and then compile with ajv.compile(schema) and assert
validate(sample) returns true; optionally rename the test to "import schema
allows missing dryRun (optional)" to reflect the intent.
- Around line 172-241: Tests repeatedly call ajv.compile(schema) for each schema
inside every it block; precompile once in a beforeAll to improve speed and
clarity. Create a Map (e.g., validators = new Map()) in a beforeAll that
iterates over schemas and stores ajv.compile(schema) keyed by file, then update
all uses of ajv.compile(schema) in the tests to fetch the ValidateFunction from
validators.get(file) (reference symbols: schemas, ajv.compile, beforeAll,
validators Map, VALID_SAMPLES) so each test reuses the precompiled validator
instead of recompiling.
- Around line 159-170: The test currently asserts a hardcoded number of schema
files (expect(schemas).toHaveLength(13)), which is brittle; replace this with a
dynamic check that every loaded schema from loadSchemas() has a corresponding
entry in VALID_SAMPLES (or alternatively assert schemas.length ===
Object.keys(VALID_SAMPLES).length) so adding/removing schema files won't break
the test; locate the test block using symbols schemas, loadSchemas, and
VALID_SAMPLES and update the it('has exactly 13 schema files') assertion to
iterate schemas and assert VALID_SAMPLES has a key for each schema filename (or
compare counts of schemas and VALID_SAMPLES) instead of the fixed 13.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/contracts.integration.test.js`:
- Around line 62-148: Add lightweight integration canary tests for missing
commands by following the existing pattern in
test/contracts.integration.test.js: for each command name (e.g., 'at', 'diff',
'suggest', 'review', 'review --batch', 'merge') add an it(...) that loads the
corresponding schema via loadSchema('<command>.schema.json'), invokes
runCli([...command..., '--json'] or a minimal deterministic variant such as
'--dry-run' or a simple fixture path using FIXTURE and tempDir), asserts
output.schemaVersion === 1 and output.command matches, then compiles and
validates with ajv.compile(schema) and expect(validate(output),
JSON.stringify(validate.errors)).toBe(true); for commands that need extra setup
(multiple commits for 'diff'/'at' or LLM mocking for 'suggest') create minimal
deterministic fixtures or use dry-run flags so the test remains stable while
covering the contract.
| it('status --json validates against status.schema.json', async () => { | ||
| const schema = await loadSchema('status.schema.json'); | ||
| const output = runCli(['status', '--json'], tempDir); | ||
|
|
||
| expect(output.schemaVersion).toBe(1); | ||
| expect(output.command).toBe('status'); | ||
|
|
||
| const validate = ajv.compile(schema); | ||
| expect(validate(output), JSON.stringify(validate.errors)).toBe(true); | ||
| }); | ||
|
|
||
| it('nodes --json validates against node-list.schema.json', async () => { | ||
| const schema = await loadSchema('node-list.schema.json'); | ||
| const output = runCli(['nodes', '--json'], tempDir); | ||
|
|
||
| expect(output.schemaVersion).toBe(1); | ||
| expect(output.command).toBe('nodes'); | ||
| expect(Array.isArray(output.nodes)).toBe(true); | ||
|
|
||
| const validate = ajv.compile(schema); | ||
| expect(validate(output), JSON.stringify(validate.errors)).toBe(true); | ||
| }); | ||
|
|
||
| it('nodes --id <known> --json validates against node-detail.schema.json', async () => { | ||
| // Get a known node from the list first | ||
| const listOutput = runCli(['nodes', '--json'], tempDir); | ||
| const knownId = listOutput.nodes[0]; | ||
| expect(knownId).toBeDefined(); | ||
|
|
||
| const schema = await loadSchema('node-detail.schema.json'); | ||
| const output = runCli(['nodes', '--id', knownId, '--json'], tempDir); | ||
|
|
||
| expect(output.schemaVersion).toBe(1); | ||
| expect(output.command).toBe('nodes'); | ||
| expect(output.id).toBe(knownId); | ||
|
|
||
| const validate = ajv.compile(schema); | ||
| expect(validate(output), JSON.stringify(validate.errors)).toBe(true); | ||
| }); | ||
|
|
||
| it('doctor --json validates against doctor.schema.json', async () => { | ||
| const schema = await loadSchema('doctor.schema.json'); | ||
| const output = runCli(['doctor', '--json'], tempDir); | ||
|
|
||
| expect(output.schemaVersion).toBe(1); | ||
| expect(output.command).toBe('doctor'); | ||
|
|
||
| const validate = ajv.compile(schema); | ||
| expect(validate(output), JSON.stringify(validate.errors)).toBe(true); | ||
| }); | ||
|
|
||
| it('export --json validates against export-data.schema.json', async () => { | ||
| const schema = await loadSchema('export-data.schema.json'); | ||
| const output = runCli(['export', '--json'], tempDir); | ||
|
|
||
| expect(output.schemaVersion).toBe(1); | ||
| expect(output.command).toBe('export'); | ||
| expect(output.version).toBe(1); | ||
|
|
||
| const validate = ajv.compile(schema); | ||
| expect(validate(output), JSON.stringify(validate.errors)).toBe(true); | ||
| }); | ||
|
|
||
| it('export --file <path> --json validates against export-file.schema.json', async () => { | ||
| const schema = await loadSchema('export-file.schema.json'); | ||
| const outPath = join(tempDir, 'export-test.yaml'); | ||
| const output = runCli(['export', '--file', outPath, '--json'], tempDir); | ||
|
|
||
| expect(output.schemaVersion).toBe(1); | ||
| expect(output.command).toBe('export'); | ||
| expect(output.path).toBe(outPath); | ||
|
|
||
| const validate = ajv.compile(schema); | ||
| expect(validate(output), JSON.stringify(validate.errors)).toBe(true); | ||
| }); | ||
|
|
||
| it('import --dry-run --json validates against import.schema.json', async () => { | ||
| const schema = await loadSchema('import.schema.json'); | ||
| const output = runCli(['import', '--dry-run', FIXTURE, '--json'], tempDir); | ||
|
|
||
| expect(output.schemaVersion).toBe(1); | ||
| expect(output.command).toBe('import'); | ||
| expect(output.valid).toBe(true); | ||
|
|
||
| const validate = ajv.compile(schema); | ||
| expect(validate(output), JSON.stringify(validate.errors)).toBe(true); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding integration canaries for remaining commands.
The current suite covers status, nodes (list + detail), doctor, export (stdout + file), and import (dry-run) — 7 tests total. Commands like at, diff, suggest, review, review --batch, and merge don't have integration coverage. These may require more complex setup (e.g., multiple commits for diff/at, LLM mocking for suggest), but even partial canaries would strengthen confidence in the contract enforcement end-to-end.
🤖 Prompt for AI Agents
In `@test/contracts.integration.test.js` around lines 62 - 148, Add lightweight
integration canary tests for missing commands by following the existing pattern
in test/contracts.integration.test.js: for each command name (e.g., 'at',
'diff', 'suggest', 'review', 'review --batch', 'merge') add an it(...) that
loads the corresponding schema via loadSchema('<command>.schema.json'), invokes
runCli([...command..., '--json'] or a minimal deterministic variant such as
'--dry-run' or a simple fixture path using FIXTURE and tempDir), asserts
output.schemaVersion === 1 and output.command matches, then compiles and
validates with ajv.compile(schema) and expect(validate(output),
JSON.stringify(validate.errors)).toBe(true); for commands that need extra setup
(multiple commits for 'diff'/'at' or LLM mocking for 'suggest') create minimal
deterministic fixtures or use dry-run flags so the test remains stable while
covering the contract.
- Replace hardcoded schema count (13) with dynamic sample coverage check - Precompile all schemas once in beforeAll Map, reuse across tests - Fix dryRun test to verify field optionality (delete vs set true) - Add existsSync guard for fixture file in beforeAll - Improve nodes[0] assertion with descriptive diagnostic message - Add review --json integration canary test
|
@coderabbitai review please 🥕 |
|
Looking at the CLI JSON Schema contracts, breaking changes, test coverage, and migration guidance now. 🧠 Learnings used✅ Actions performedReview triggered.
|
Summary
schemaVersion: 1andcommandfields to all 15--jsonCLI outputs viaoutputJson()helperdocs/contracts/cli/covering every commandnodes --jsonandreview --json(bare arrays → wrapped objects)Breaking changes:
nodes --jsonnow returns{ schemaVersion: 1, command: "nodes", nodes: [...] }instead of a bare arrayreview --jsonnow returns{ schemaVersion: 1, command: "review", pending: [...] }instead of a bare arrayMigration snippets in
docs/contracts/CLI_CONTRACTS.md.Closes #205
Test plan
git mind status --json | jq '.schemaVersion'→1git mind nodes --json | jq '.nodes'→ arraySummary by CodeRabbit
New Features
Documentation
Tests
Chores