Conversation
…isions import/set-status; wire into CLI and forwarder
There was a problem hiding this comment.
Pull Request Overview
This PR introduces checkpointing and decisions capabilities to the splice tool. It refactors the monolithic splice.ts into a modular CLI with persistent workspace artifacts (JSONL-based), enabling resumable pipelines and laying the foundation for an interactive UI to review and curate items.
- Adds JSONL-based checkpointing system with content-addressed storage (FsStore) under
<workspace>/objectsand manifests under<workspace>/checkpoints - Introduces decisions JSONL to track manual statuses (
unread/export/skip) per item for interactive review workflows - Refactors entry point (
splice.ts) to a thin forwarder that loads the modular CLI (src/cli/splice.ts), ensuring the checkpoint code path runs during dev
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/core/types.ts |
Adds CLI options for workspace, checkpoint, decisions import, and status management |
src/core/store.ts |
Implements FsStore with content-addressed objects, checkpoint manifests, and helpers for JSONL persistence |
src/core/decisions.ts |
Pure functions for folding decision streams, applying statuses to items, and generating decision records |
src/cli/splice.ts |
Wires checkpoint creation into CLI run; stores artifacts, processes decisions flags, and chains manifests |
splice.ts |
Refactored to a thin forwarder that loads TypeScript or compiled JavaScript CLI entry |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| function stableStringify(value: unknown): string { | ||
| // Deterministic JSON stringify (sort object keys) | ||
| const seen = new WeakSet<object>(); | ||
| const stringify = (v: any): string => { |
There was a problem hiding this comment.
The stringify inner function uses any type for parameter v. Consider using unknown for better type safety.
| for await (const item of iterable as any) { | ||
| const line = JSON.stringify(item) + "\n"; | ||
| await fh.write(line); | ||
| hashUpdate(h, line); |
There was a problem hiding this comment.
Type casting to any bypasses type safety. Consider using a proper type guard or conditional type handling for the iterable.
| for await (const item of iterable as any) { | |
| const line = JSON.stringify(item) + "\n"; | |
| await fh.write(line); | |
| hashUpdate(h, line); | |
| if (typeof (iterable as AsyncIterable<T>)[Symbol.asyncIterator] === "function") { | |
| for await (const item of iterable as AsyncIterable<T>) { | |
| const line = JSON.stringify(item) + "\n"; | |
| await fh.write(line); | |
| hashUpdate(h, line); | |
| } | |
| } else if (typeof (iterable as Iterable<T>)[Symbol.iterator] === "function") { | |
| for (const item of iterable as Iterable<T>) { | |
| const line = JSON.stringify(item) + "\n"; | |
| await fh.write(line); | |
| hashUpdate(h, line); | |
| } | |
| } else { | |
| throw new TypeError("putJSONL: Provided value is not Iterable or AsyncIterable"); |
|
|
||
| async saveCheckpoint(manifest: CheckpointManifest): Promise<string> { | ||
| await this.init(); | ||
| const id = manifest.id || this.generateCheckpointId(manifest); |
There was a problem hiding this comment.
Empty string '' is falsy and will trigger ID generation even if explicitly set. Use manifest.id ?? this.generateCheckpointId(manifest) to handle only nullish values.
| const id = manifest.id || this.generateCheckpointId(manifest); | |
| const id = manifest.id ?? this.generateCheckpointId(manifest); |
| const source = path.resolve(opts.source); | ||
| const outDir = path.resolve(opts.out); | ||
| const workspaceDir = path.resolve( | ||
| (opts as any).workspace || path.join(outDir, ".splice"), |
There was a problem hiding this comment.
Using (opts as any).workspace bypasses type safety. Define workspace and checkpoint in the CLIOptions type (they appear to be added in types.ts) and remove the cast.
| (opts as any).workspace || path.join(outDir, ".splice"), | |
| opts.workspace || path.join(outDir, ".splice"), |
| const decisionsPath = (opts as any).decisionsImport as | ||
| | string | ||
| | undefined; |
There was a problem hiding this comment.
Multiple uses of (opts as any) throughout this section bypass type safety. The CLIOptions type should include decisionsImport, setStatus, ids, and idsFile fields to avoid casting.
| let ids: string[] = Array.isArray((opts as any).ids) | ||
| ? ((opts as any).ids as string[]) | ||
| : []; |
There was a problem hiding this comment.
Redundant type casting. If opts.ids is typed as string[] in CLIOptions, this can be simplified to let ids: string[] = opts.ids || [];.
| let ids: string[] = Array.isArray((opts as any).ids) | |
| ? ((opts as any).ids as string[]) | |
| : []; | |
| let ids: string[] = (opts as any).ids || []; |
What