refactor: decompose CLI into per-command modules (M5.T1)#27
refactor: decompose CLI into per-command modules (M5.T1)#27flyingrobots merged 21 commits intomainfrom
Conversation
…-AUDIT, v10.10.0) Adds AuditVerifierService domain service and `git warp verify-audit` CLI command that walks audit chains backward from tip to genesis, validating schema, chain linking, Git parent consistency, tick monotonicity, trailer-CBOR consistency, OID format, and tree structure. Supports `--writer` filtering, `--since` partial verification, and ref-race detection. Exit code 3 on integrity failures.
…ANDS-SPLIT, v10.11.0) Split bin/warp-graph.js (2491 LOC) into 15 focused modules: - bin/warp-graph.js: 112 LOC entrypoint (imports + COMMANDS map + dispatch) - bin/cli/infrastructure.js: EXIT_CODES, HELP_TEXT, CliError, parseArgs - bin/cli/shared.js: 12 helpers used by 2+ commands - bin/cli/types.js: JSDoc typedefs - bin/cli/commands/: 10 per-command handlers Pure refactor — no behavior changes. All 3476 unit tests pass.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces a monolithic CLI with a modular per-command dispatch, adds a verify-audit feature and AuditVerifierService, introduces robust CLI parsing/schemas and helper infra, extends persistence ports/adapters with commit→tree support, wires presenters, adds tests/benchmarks, docs, changelog and packaging updates. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (verify-audit)
participant Service as AuditVerifierService
participant Persistence as GitGraphAdapter / InMemoryGraphAdapter
participant Codec as AuditMessageCodec
CLI->>Service: verifyChain(graph, writerId, { since? })
Service->>Persistence: readRef(auditRef) / readCommit(tipSha)
Persistence-->>Service: tipCommit, commitInfo
loop for each receipt from tip → genesis
Service->>Persistence: getCommitTree(commitSha)
Persistence-->>Service: treeOid
Service->>Persistence: readTree(treeOid) / readBlob(receipt.cbor)
Persistence-->>Service: receiptBlob
Service->>Codec: decode(receiptBlob)
Codec-->>Service: receipt, decodedTrailers
Service->>Service: validateReceiptSchema(...) / validateTrailerConsistency(...)
alt has prevAuditCommit
Service->>Persistence: readCommit(prevSha)
Persistence-->>Service: prevCommitInfo
Service->>Service: _validateChainLink(prevReceipt)
end
end
Service->>Persistence: readRef(auditRef) (re-check tip for races)
Persistence-->>Service: currentTip
Service->>CLI: { status, receiptsVerified, errors, warnings, chains }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
Release Preflight
If you tag this commit as |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
43-56:⚠️ Potential issue | 🔴 Critical
bin/cli/directory is missing from thefilesarray — published package will break at runtime.
bin/warp-graph.jsimports frombin/cli/infrastructure.js,bin/cli/shared.js, andbin/cli/commands/*.js, butbin/cliis not listed in thefilesarray. Runningnpm packwill exclude the entirebin/cli/directory, causingMODULE_NOT_FOUNDerrors for any consumer.Proposed fix
"files": [ "bin/warp-graph.js", + "bin/cli", "bin/presenters", "bin/git-warp", "src",
🤖 Fix all issues with AI agents
In `@bin/cli/commands/verify-audit.js`:
- Around line 24-27: The argument parsing loop that checks for flag values
treats empty strings as absent because it uses a truthy check (args[i +
1])—update the check in the for-loop that handles '--since' and '--writer' so it
uses strict undefined comparison (args[i + 1] !== undefined) and still
increments i and assigns since/writer when the next element is an empty string;
ensure the branch that currently does `since = args[i + 1]; i++;` and the
analogous `writer = args[i + 1]; i++;` are reached when the next element is not
undefined.
- Around line 24-32: The args-parsing loop in verify-audit.js currently ignores
unrecognized flags (loop handling for --since and --writer), so update it to
reject unknown args by throwing usageError with a helpful message when an arg is
not --since or --writer or when a flag is missing its value; also add an import
for usageError at the top (currently only EXIT_CODES is imported) and use the
existing variables since and writerFilter to validate/assign values before
continuing. Ensure the thrown usageError mirrors other modules (history.js,
path.js, install-hooks.js) so typos like --writter trigger an error instead of
being silently skipped.
In `@CHANGELOG.md`:
- Line 19: The changelog claim that `bin/warp-graph.js` was "replaced" is
inaccurate because `bin/warp-graph.js` still remains in the ESLint
relaxed-complexity block; update the CHANGELOG.md entry to reflect the actual
change (e.g., "Added `bin/cli/commands/seek.js`, `bin/cli/commands/query.js`,
and other `bin/cli/` modules to the relaxed-complexity block") or adjust the
ESLint config (`eslint.config.js`) to remove `bin/warp-graph.js` if it was truly
replaced—refer to the relaxed-complexity block and the filenames
`bin/warp-graph.js`, `bin/cli/commands/seek.js`, and `bin/cli/commands/query.js`
when making the correction.
In `@docs/specs/AUDIT_RECEIPT.md`:
- Line 730: The section numbering skips 13: update the header "## 14.
Verification Output (M4.T1)" to "## 13. Verification Output (M4.T1)" or insert a
new "## 13" section before it (e.g., "## 13. [Title]" with appropriate content),
ensuring sequential numbering; specifically adjust the markdown header text "##
14. Verification Output (M4.T1)" in AUDIT_RECEIPT.md (or add a new "## 13"
block) so sections proceed 12 → 13 → 14 consistently.
In `@test/bats/cli-verify-audit.bats`:
- Around line 72-95: WRONG_PARENT is assigned but never used; either remove the
UNUSED assignment or use it to create a commit with an explicit wrong parent.
Fix by either deleting the WRONG_PARENT="$(printf '0%.0s' {1..40})" line, or
change the commit creation to include it as a parent (replace the commit-tree
invocation that sets FAKE with something like using git commit-tree -p
"${WRONG_PARENT}" "${TREE}" while keeping MSG as the commit message) so the
variable is actually referenced (symbols: WRONG_PARENT, FAKE, TREE, MSG, git
commit-tree).
🧹 Nitpick comments (12)
bin/cli/commands/view.js (1)
27-34: BroadCannot find modulecheck could mask sub-dependency failures.If
startTuithrows an error whose message contains "Cannot find module" (e.g., a broken sub-dependency within@git-stunts/git-warp-tui), it would be caught and reported as "Install with: npm install -g@git-stunts/git-warp-tui" instead of surfacing the real error.Consider tightening the string check to match the specific package name:
Suggested improvement
- if (err.code === 'ERR_MODULE_NOT_FOUND' || (err.message && err.message.includes('Cannot find module'))) { + if (err.code === 'ERR_MODULE_NOT_FOUND' && err.message?.includes('@git-stunts/git-warp-tui') + || (err.message && err.message.includes("Cannot find module '@git-stunts/git-warp-tui'"))) {bin/cli/commands/info.js (1)
75-89: Sequential patch fetching could be parallelized for view mode.The
forloop on lines 84-87 fetches patches for each writer sequentially. For graphs with many writers in view mode, this could be slow. Consider usingPromise.allfor concurrent fetching.Proposed parallel fetching
- /** `@type` {Record<string, number>} */ - const writerPatches = {}; - for (const writerId of writerIds) { - const patches = await graph.getWriterPatches(writerId); - writerPatches[/** `@type` {string} */ (writerId)] = patches.length; - } + /** `@type` {Record<string, number>} */ + const writerPatches = {}; + await Promise.all(writerIds.map(async (writerId) => { + const patches = await graph.getWriterPatches(writerId); + writerPatches[/** `@type` {string} */ (writerId)] = patches.length; + }));bin/cli/commands/install-hooks.js (1)
43-60: DuplicateHookInstallerinstantiation.
promptForOursStrategycreates a newHookInstaller(line 44) solely to read_version, buthandleInstallHooksalready creates one at line 98. Consider passing the installer (or its version) as a parameter to avoid the redundant construction and filesystem reads.Proposed fix: pass installer version through
-/** `@param` {*} classification */ -async function promptForOursStrategy(classification) { - const installer = createHookInstaller(); - if (classification.version === installer._version) { +/** + * `@param` {*} classification + * `@param` {string} currentVersion + */ +async function promptForOursStrategy(classification, currentVersion) { + if (classification.version === currentVersion) { return 'up-to-date'; } @@ .. const answer = await promptUser( - `Upgrade hook from v${classification.version} to v${installer._version}? [Y/n] `, + `Upgrade hook from v${classification.version} to v${currentVersion}? [Y/n] `, );Then update
resolveStrategyto accept and forward the version:-async function resolveStrategy(classification, hookOptions) { +async function resolveStrategy(classification, hookOptions, currentVersion) { ... if (classification.kind === 'ours') { - return await promptForOursStrategy(classification); + return await promptForOursStrategy(classification, currentVersion); }And in
handleInstallHooks:- const strategy = await resolveStrategy(classification, hookOptions); + const strategy = await resolveStrategy(classification, hookOptions, installer._version);bin/cli/commands/path.js (1)
97-103:parseMaxDepthaccepts negative values.
Number.parseInt("-1", 10)passes theNaNcheck but negative depth values are likely nonsensical for a shortest-path traversal.Proposed validation
function parseMaxDepth(value) { const parsed = Number.parseInt(value, 10); - if (Number.isNaN(parsed)) { + if (Number.isNaN(parsed) || parsed < 0) { throw usageError('Invalid value for --max-depth'); } return parsed; }bin/cli/commands/seek.js (2)
252-259: Minor UX nit:--differror message for defaultstatusaction could be confusing.When the user passes
--diffwithout any action flag,spec.actionis still'status'(the default). The resulting error message--diff cannot be used with --statusreferences a--statusflag that doesn't exist. Consider a more user-friendly message:Suggested improvement
const DIFF_ACTIONS = new Set(['tick', 'latest', 'load']); if (spec.diff && !DIFF_ACTIONS.has(spec.action)) { - throw usageError(`--diff cannot be used with --${spec.action}`); + throw usageError(`--diff requires --tick, --latest, or --load`); }
438-463:applyDiffLimitcaps in a fixed order — certain change categories may be entirely suppressed.The
cap()closure is applied sequentially: nodes added/removed → edges added/removed → props set/removed. If there are many node additions, edges and props will be entirely cut. This is acceptable for a truncation strategy, but documenting the priority (or applying a proportional budget) might improve UX for very diverse diffs. This is a design choice rather than a bug.test/bats/cli-verify-audit.bats (1)
97-120: Test manually re-creates repo setup — consider extracting a helper.This test duplicates the repo initialization logic from
setup_test_repoinhelpers/setup.bash. If the setup evolves (e.g., new git config keys), this test could drift.That said, since it needs a different seed script, the duplication is understandable and the test is self-contained.
bin/cli/infrastructure.js (2)
162-184: HardcodedKNOWN_COMMANDSlist for--viewparsing requires manual sync.The
KNOWN_COMMANDSarray on line 165 must be kept in sync with the actual command set. If a new command is added later but not added here,--view <newcommand>would attempt to interpret the command name as a view mode and throw an invalid mode error.Consider deriving this list from the
COMMANDSmap inbin/warp-graph.js, or at minimum add a comment noting the sync requirement.
75-89: Passcauseto theErrorsuper constructor for idiomatic error handling.
CliErrormanually assignsthis.cause = causeinstead of passing it through the Error constructor. Since the project requires Node.js ≥22.0.0 and support forError(message, { cause })has been available since Node.js 16.9.0, passing the cause parameter tosuper()aligns with standard error handling practices.Suggested improvement
constructor(message, { code = 'E_CLI', exitCode = EXIT_CODES.INTERNAL, cause } = {}) { - super(message); + super(message, { cause }); this.code = code; this.exitCode = exitCode; - this.cause = cause; }bin/cli/shared.js (2)
86-102: RedundantlistGraphNamescall whenoptions.graphis specified.
resolveGraphName(line 88) returnsoptions.graphdirectly when it's truthy. Then lines 89–94 calllistGraphNamesagain to validate the graph exists. This results in twolistRefsI/O calls. Consider either havingresolveGraphNamevalidate existence when an explicit graph is provided, or moving the validation beforeresolveGraphName.Suggested consolidation
export async function openGraph(options) { const { persistence } = await createPersistence(options.repo); - const graphName = await resolveGraphName(persistence, options.graph); - if (options.graph) { - const graphNames = await listGraphNames(persistence); - if (!graphNames.includes(options.graph)) { - throw notFoundError(`Graph not found: ${options.graph}`); - } + const graphNames = await listGraphNames(persistence); + let graphName; + if (options.graph) { + if (!graphNames.includes(options.graph)) { + throw notFoundError(`Graph not found: ${options.graph}`); + } + graphName = options.graph; + } else if (graphNames.length === 1) { + graphName = graphNames[0]; + } else if (graphNames.length === 0) { + throw notFoundError('No graphs found in repo; specify --graph'); + } else { + throw usageError('Multiple graphs found; specify --graph'); }
232-244:persistence.plumbingleaks the adapter implementation through the port abstraction.
wireSeekCacheaccessespersistence.plumbing(line 241), which is a property specific toGitGraphAdapter, not part of thePersistenceport type. This couples the seek cache wiring to the concrete adapter.For CLI-only code where persistence is always
GitGraphAdapter, this is pragmatically fine, but worth noting for any future adapter swap.src/domain/services/AuditVerifierService.js (1)
352-358:_addErrorsets status toERROR, then callers immediately override — consider accepting status as a parameter.At line 355,
_addErrorinternally setsresult.status = STATUS_ERROR, then line 356 immediately overwrites it withSTATUS_DATA_MISMATCH. The same pattern occurs withSTATUS_BROKEN_CHAINat lines 371, 399, 419, 425, and 575. This works correctly but is fragile — a future maintainer might forget the override after_addError.Suggested improvement
- _addError(result, code, message, commit) { + _addError(result, code, message, commit, status = STATUS_ERROR) { result.errors.push({ code, message, ...(commit ? { commit } : {}) }); if (result.status === STATUS_VALID || result.status === STATUS_PARTIAL) { - result.status = STATUS_ERROR; + result.status = status; } }Then callers become:
this._addError(result, 'TRAILER_MISMATCH', trailerErr, current, STATUS_DATA_MISMATCH); return;
…lags Extract parseVerifyAuditArgs() with strict undefined check (was truthy), reject unknown flags/positionals with usageError, and throw on missing values. Aligns verify-audit with history.js/install-hooks.js patterns. Add 10 unit tests covering happy path, empty strings, unknown flags, missing values, and unexpected positionals.
…ot replaced bin/warp-graph.js is still in the relaxed-complexity block; the new bin/cli/ modules were added alongside it, not as replacements.
Section numbering skipped from 12 to 14. Renumber Verification Output section to 13 for sequential consistency.
Variable was assigned but never referenced — the test already correctly creates an orphan commit to break the chain.
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@bin/cli/commands/verify-audit.js`:
- Around line 53-70: The current truthy check on writerFilter causes an
empty-string writer (which parseVerifyAuditArgs now accepts) to fall through to
verifyAll; change the gate to detect absence explicitly (e.g., if (writerFilter
!== undefined) or if (typeof writerFilter === 'string') ) so that an empty
string is treated as a valid writer and calls verifier.verifyChain(graphName,
writerFilter, { since }) instead of verifier.verifyAll. Ensure you reference
writerFilter, verifier.verifyChain and verifier.verifyAll when making the
change.
🧹 Nitpick comments (1)
test/unit/cli/verify-audit-args.test.js (1)
1-52: Good coverage of the core parsing paths.The tests cover happy paths, empty inputs, empty-string edge cases, and all three error branches. Two minor gaps worth considering:
- Reverse flag order (
['--writer', 'bob', '--since', 'abc']) — currently only the--since-first order is tested.- Duplicate flags (
['--since', 'a', '--since', 'b']) — the parser silently takes the last value; a test would document whether that's intentional.Neither is blocking, just nice-to-have for completeness.
The commands-split refactor moved CLI code into bin/cli/ but didn't update the files array, breaking the published package for CLI use.
- NodeCryptoAdapter → WebCryptoAdapter in shared.js, info.js, materialize.js - ClockAdapter.node() → ClockAdapter.global() in check.js - Remove `import crypto from 'node:crypto'` in seek.js; convert computeFrontierHash to async using globalThis.crypto.subtle The CLI now uses only cross-runtime APIs (Web Crypto, globalThis.performance) and can run under Node 22+, Bun, and Deno without modification.
Replace hand-rolled parseArgs in infrastructure.js with a two-pass
approach using node:util.parseArgs:
1. extractBaseArgs: scans argv for known base flags (--repo, --json,
--view, etc.) regardless of position, extracts them and identifies
the command name and per-command args.
2. preprocessView: injects 'ascii' default for --view's optional-value
semantics (not natively supported by parseArgs).
3. nodeParseArgs: strict parsing of base args only.
Return shape changes from {options, positionals} to
{options, command, commandArgs} — warp-graph.js updated accordingly.
25 new tests in test/unit/cli/parseArgs.test.js covering base option
parsing, --view optional value, command splitting, and passthrough.
Replace hand-rolled arg parsing in all 10 command handlers with node:util.parseArgs configs validated by Zod schemas. New file: bin/cli/schemas.js — Zod schemas for all commands with type coercion, enum validation, and mutual-exclusion checks (seek). Per-command changes: - history: HISTORY_OPTIONS + historySchema - install-hooks: INSTALL_HOOKS_OPTIONS + installHooksSchema - verify-audit: VERIFY_AUDIT_OPTIONS + verifyAuditSchema - path: PATH_OPTIONS + pathSchema (positionals for from/to, comma labels) - query: hybrid — extractTraversalSteps for --outgoing/--incoming optional values, then QUERY_OPTIONS + querySchema for the rest - view: VIEW_OPTIONS + viewSchema (positionals for log subcommand) - seek: SEEK_OPTIONS + seekSchema (10 flags, Zod superRefine for mutual-exclusion validation) Infrastructure: - New parseCommandArgs() helper wrapping nodeParseArgs + Zod safeParse - Removed readOptionValue and its helpers (no longer used) Tests: - 32 new tests in test/unit/cli/schemas.test.js - All 67 CLI tests pass, all 3543 unit tests pass
- CHANGELOG: new entry documenting cross-runtime support, parseArgs migration, packaging fix, and 67 new CLI tests - package.json: bump version to 10.12.0
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@bin/cli/commands/install-hooks.js`:
- Around line 80-87: The readHookContent function currently swallows all errors
from fs.readFileSync which masks permission/encoding errors; update
readHookContent to only return null for ENOENT (file not found) and re-throw any
other errors so permission/encoding issues surface. Locate readHookContent and
its fs.readFileSync call and change the catch block to inspect the thrown
error's code (e.g., err.code === 'ENOENT')—return null in that case and re-throw
the error otherwise.
In `@bin/cli/commands/view.js`:
- Around line 32-39: The catch block in the try/catch around requiring the TUI
(the error variable `err` handled in the catch) is too broad and may treat
transitive dependency failures as the TUI itself missing; update the conditional
that currently checks `err.code === 'ERR_MODULE_NOT_FOUND' || (err.message &&
err.message.includes('Cannot find module'))` to ensure the missing-module error
actually references the TUI package (check `err.specifier`, `err.url`, or ensure
`err.message` contains `@git-stunts/git-warp-tui`) before calling `usageError`
for installation, otherwise rethrow the original `err`; keep the `usageError`
call and `throw err` behavior but only when the missing specifier/url
specifically points to `@git-stunts/git-warp-tui`.
In `@bin/cli/infrastructure.js`:
- Line 102: The KNOWN_COMMANDS array in infrastructure.js is a hardcoded list
that must mirror the COMMANDS map in warp-graph.js to ensure extractBaseArgs and
isViewValue correctly distinguish commands from arguments; fix by either (A)
deriving KNOWN_COMMANDS programmatically from the keys of the COMMANDS export in
warp-graph.js (import COMMANDS and set KNOWN_COMMANDS = Object.keys(COMMANDS))
or (B) add a unit test that imports both KNOWN_COMMANDS and COMMANDS and asserts
their key sets are identical so future changes stay in sync; update
extractBaseArgs/isViewValue imports if needed to use the derived list.
In `@bin/cli/schemas.js`:
- Line 37: The schema for the 'max-depth' field currently uses
z.coerce.number().int().optional(), which allows negative integers; update the
validator for 'max-depth' in bin/cli/schemas.js (the schema entry keyed
'max-depth') to constrain values to non-negative (or strictly positive) integers
by chaining an appropriate Zod check such as .nonnegative() or .min(0) (or
.positive() if negatives and zero should be disallowed) after .int() so negative
depths are rejected.
- Around line 103-110: The current check (val.diff && !DIFF_ACTIONS &&
actions.length > 0) lets --diff pass when used alone; update the validation so
--diff is only allowed when one of the action flags is present by changing the
condition to simply val.diff && !DIFF_ACTIONS (or, if the intent is to permit
--diff with bare status, add a clarifying comment above DIFF_ACTIONS). Modify
the block that calls ctx.addIssue (referencing DIFF_ACTIONS, val.diff, and
actions.length) to use the new condition so a missing action flag triggers the
error message.
🧹 Nitpick comments (11)
test/unit/cli/schemas.test.js (1)
194-196: Consider adding a test for--diffwith no action flags (bare status).The
superRefineconditionval.diff && !DIFF_ACTIONS && actions.length > 0means{ diff: true }(no action flag, i.e. status) silently passes validation, sinceactions.lengthis 0. If--diffis only intended to work with--tick,--latest, or--load(as the comment inschemas.jsstates), this is a gap. Add a test to document whetherseekSchema.parse({ diff: true })should succeed or throw.bin/cli/commands/history.js (1)
22-33:patchTouchesNodedoesn't match edge property ops by the edge's endpoint IDs.Edge property ops (PropSet with encoded edge key) have
op.nodeset to the encoded edge key (e.g.,\x01from\x01to\x01label\x01key), not the raw node ID. If a user filters history by a node ID that is an edge endpoint, edge property ops on that edge won't be included. This may be intentional (node filter = node-level ops only), but worth documenting.bin/cli/commands/info.js (2)
75-89: SequentialgetWriterPatchesloop — consider parallelizing for repos with many writers.Each writer's patches are fetched sequentially. For graphs with many writers, this could be slow since each call involves Git I/O.
Promise.allwould speed this up.Suggested refactor
if (includeWriterPatches && writerIds.length > 0) { const graph = await WarpGraph.open({ persistence, graphName, writerId: 'cli', crypto: new WebCryptoAdapter(), }); /** `@type` {Record<string, number>} */ const writerPatches = {}; - for (const writerId of writerIds) { - const patches = await graph.getWriterPatches(writerId); - writerPatches[/** `@type` {string} */ (writerId)] = patches.length; - } + const results = await Promise.all( + writerIds.map(async (writerId) => { + const patches = await graph.getWriterPatches(/** `@type` {string} */ (writerId)); + return [/** `@type` {string} */ (writerId), patches.length]; + }) + ); + for (const [id, count] of results) { + writerPatches[/** `@type` {string} */ (id)] = /** `@type` {number} */ (count); + } info.writerPatches = writerPatches; }
94-138: Return shape is handled correctly but inconsistent with other handlers.The dispatcher in
bin/warp-graph.js(lines 70-75) has auto-wrapping logic that normalizes handler results: if a result lacks apayloadproperty, it's automatically wrapped as{ payload: result, exitCode: EXIT_CODES.OK }. This meanshandleInfo's{ repo, graphs }return will work correctly.However, other command handlers (
handlePath,handleHistory,handleView) explicitly return{ payload, exitCode }. For consistency, consider updating the return statement to match:return { payload: { repo: options.repo, graphs }, exitCode: EXIT_CODES.OK, };This makes the pattern explicit rather than relying on implicit auto-wrapping.
bin/cli/commands/install-hooks.js (1)
40-58: Accessinginstaller._versionand creating a redundantHookInstallerinstance.
promptForOursStrategycreates its ownHookInstaller(line 42) solely to read_version, whilehandleInstallHooks(line 96) creates another. The underscore-prefixed_versionsignals a private member. Consider either:
- Exposing a public
versiongetter onHookInstaller, or- Passing the version (or the installer instance) into
resolveStrategy/promptForOursStrategyto avoid the extra instantiation and the encapsulation breach.♻️ Suggested approach — pass installer down
-async function resolveStrategy(classification, hookOptions) { +async function resolveStrategy(classification, hookOptions, installer) { if (hookOptions.force) { return 'replace'; } if (classification.kind === 'none') { return 'install'; } if (classification.kind === 'ours') { - return await promptForOursStrategy(classification); + return await promptForOursStrategy(classification, installer); } return await promptForForeignStrategy(); } -async function promptForOursStrategy(classification) { - const installer = createHookInstaller(); - if (classification.version === installer._version) { +async function promptForOursStrategy(classification, installer) { + if (classification.version === installer._version) { return 'up-to-date'; } ...Then in
handleInstallHooks:- const strategy = await resolveStrategy(classification, hookOptions); + const strategy = await resolveStrategy(classification, hookOptions, installer);bin/cli/commands/seek.js (2)
195-205:countPatchesAtTickcounts patches up to the tick, not at the tick.The function name implies counting patches exactly at a given tick, but the condition
t <= tickcounts all patches from the beginning through the given tick. This appears intentional given usage context (showing cumulative patch count), but the name is misleading.Consider renaming to
countPatchesThroughorcountPatchesUpToTickfor clarity.
329-343: Diff-limit budget allocation order is implicit and may surprise users.The
capclosure sequentially consumes a sharedremainingbudget across the six diff categories in a fixed order (nodes added → nodes removed → edges added → edges removed → props set → props removed). This means if the limit is small and there are many added nodes, removed nodes and later categories get zero entries.This is acceptable for a truncation mechanism, but worth documenting the priority order (e.g., in the
--diff-limithelp text or a code comment) so users understand why certain diff categories may appear empty in truncated output.bin/cli/infrastructure.js (1)
76-90:CliErrordoesn't passcausetosuper()via the standard ES2022 mechanism.The constructor sets
this.cause = causemanually (line 88) rather than passing{ cause }tosuper(message, { cause }). While functionally equivalent for this codebase's usage, the standard mechanism enables tooling (e.g., structured logging, error serialization) to discover the cause chain automatically.♻️ Optional improvement
- constructor(message, { code = 'E_CLI', exitCode = EXIT_CODES.INTERNAL, cause } = {}) { - super(message); + constructor(message, { code = 'E_CLI', exitCode = EXIT_CODES.INTERNAL, cause } = {}) { + super(message, cause ? { cause } : undefined); this.code = code; this.exitCode = exitCode; - this.cause = cause; }bin/cli/shared.js (3)
86-102:openGraphcallslistGraphNamesredundantly when--graphis specified.When
options.graphis set,resolveGraphName(line 88) returns immediately without callinglistGraphNames. Then line 90 callslistGraphNamesfor validation. This is correct but could be tightened by havingresolveGraphNameperform the existence check itself, avoiding the extra round-trip.That said, this is a minor inefficiency in a CLI startup path, not a hot loop.
161-166:writeActiveCursoruses Node-specificBuffer.from.Given the PR's stated goal of cross-runtime compatibility (Node 22+, Bun, Deno),
Buffer.from(json, 'utf8')at line 164 is Node-specific. The seek.js module (line 70) has the same pattern (Buffer.from(json, 'utf8')) inwriteSavedCursor. While Bun and Deno provideBuffercompatibility, usingnew TextEncoder().encode(json)would be more portable (as done incomputeFrontierHashat seek.js line 217).
235-244:wireSeekCachecouples to adapter internals by accessingpersistence.plumbing.Line 241 reaches into an implementation detail—
plumbingis not part of theGraphPersistencePortinterface.InMemoryGraphAdapter, for example, has noplumbingproperty. If the persistence adapter is swapped, this code fails silently at runtime.Pass
plumbingas a separate parameter towireSeekCache, or expose it through the persistence adapter's public interface.
- verify-audit: reject empty-string --since/--writer via .min(1) schema - install-hooks: readHookContent only swallows ENOENT, rethrows others - view: narrow module-not-found catch to git-warp-tui specifier only - infrastructure: export KNOWN_COMMANDS + add sync drift-detection test - schemas: --max-depth rejects negatives (.nonnegative()); --diff alone (bare status, no --tick/--latest/--load) now correctly rejected
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@bin/cli/schemas.js`:
- Around line 73-83: The seekSchema currently allows empty strings for the
cursor name fields ('save', 'load', 'drop') which is likely unintended; update
the seekSchema's definitions for save, load, and drop (the z.string() calls) to
require at least one character (e.g., use z.string().min(1) or
z.string().nonempty()) so empty values are rejected like verifyAuditSchema does
for its string fields.
🧹 Nitpick comments (2)
test/unit/cli/schemas.test.js (1)
137-213: Good coverage of seekSchema, but consider adding edge-case tests for empty-string--save/--load/--drop.The
verifyAuditSchemaproperly rejects empty strings for--since/--writerviaz.string().min(1), butseekSchemauses barez.string().optional()for--save,--load, and--drop. An empty-string name (e.g.,--save "") would pass validation and produce a cursor with an empty name. Consider adding a test to document whether this is intentional, or adding.min(1)to those fields in the schema.bin/cli/infrastructure.js (1)
76-90: Consider using the standardcauseoption inErrorconstructor.
CliErrorsetsthis.causemanually, but ES2022+Errorsupports{ cause }natively via the super constructor. Using the standard mechanism ensures the cause integrates with runtimes and debugging tools that inspecterror.causethrough the built-in chain.♻️ Proposed change
- constructor(message, { code = 'E_CLI', exitCode = EXIT_CODES.INTERNAL, cause } = {}) { - super(message); + constructor(message, { code = 'E_CLI', exitCode = EXIT_CODES.INTERNAL, cause } = {}) { + super(message, cause ? { cause } : undefined); this.code = code; this.exitCode = exitCode; - this.cause = cause; }
Adds .min(1) validation to seek schema's save, load, and drop fields, matching the pattern already used for verify-audit's --since/--writer.
|
@coderabbitai review please |
|
✅ Actions performedReview triggered.
|
Release Preflight
If you tag this commit as |
- verify-audit: change `if (writerFilter)` to `if (writerFilter !== undefined)` so the gate is correct independent of upstream schema validation - view: tighten module-not-found check to match specifier or package name in the error message, not just any mention of 'git-warp-tui' (which would match transitive dep failures via the import path)
|
@coderabbitai review please |
|
✅ Actions performedReview triggered.
|
Release Preflight
If you tag this commit as |
Previous test compared KNOWN_COMMANDS against a hardcoded list, which couldn't detect drift if a command was added to warp-graph.js but not to infrastructure.js. Now reads warp-graph.js source and extracts command names from the COMMANDS Map literal via regex.
|
@coderabbitai review please |
|
✅ Actions performedReview triggered.
✅ Actions performedReview triggered.
|
Release Preflight
If you tag this commit as |
Moves the COMMANDS Map from warp-graph.js to bin/cli/commands/registry.js (side-effect-free). The sync test now imports COMMANDS directly instead of parsing source with regex. warp-graph.js re-imports from the registry.
Release Preflight
If you tag this commit as |
Release Preflight
If you tag this commit as |
|
@coderabbitai review please |
|
✅ Actions performedReview triggered.
✅ Actions performedReview triggered.
|
…reated The seed-audit-graph.js helper opened the graph with audit: true but never called materialize(), leaving _cachedState null. The audit receipt path in _onPatchCommitted requires non-null cached state; without it all 5 verify-audit BATS tests failed in CI.
Release Preflight
If you tag this commit as |
- Custom Zod messages for .int() and .positive(): "must be a positive integer" (was generic "Number must be greater than 0") - --diff error changed to "cannot be used without" to match BATS grep - Added --diff-limit requires --diff validation in superRefine - BATS: --diff-limit no-value test greps "argument missing" (parseArgs) - 6 new unit tests for all seek validation edge cases
Release Preflight
If you tag this commit as |
Adds InMemoryGraphAdapter to the public API so downstream packages can import it directly for their test suites instead of reaching into internal infrastructure paths.
Release Preflight
If you tag this commit as |
1 participant
Summary
bin/warp-graph.jsmonolith into 15 focused modules underbin/cli/main(), error handlerNew module structure
bin/cli/types.jsbin/cli/infrastructure.jsbin/cli/shared.jsbin/cli/commands/*.jsLargest command module is
seek.js(711 LOC, 20+ tightly interrelated helpers). Added to ESLint relaxed-complexity block.Test plan
npm run lint— cleannpx tsc --noEmit— cleannode scripts/ts-policy-check.js— passednpm run test:local— 3476 tests pass (170 files)wc -l bin/warp-graph.js— 112 LOCSummary by CodeRabbit
New Features
Refactor
Documentation
Tests
Chores