-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add checkpoints #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…isions import/set-status; wire into CLI and forwarder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/core/store.ts
Outdated
| function stableStringify(value: unknown): string { | ||
| // Deterministic JSON stringify (sort object keys) | ||
| const seen = new WeakSet<object>(); | ||
| const stringify = (v: any): string => { |
Copilot
AI
Oct 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stringify inner function uses any type for parameter v. Consider using unknown for better type safety.
src/core/store.ts
Outdated
| for await (const item of iterable as any) { | ||
| const line = JSON.stringify(item) + "\n"; | ||
| await fh.write(line); | ||
| hashUpdate(h, line); |
Copilot
AI
Oct 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"); |
src/core/store.ts
Outdated
|
|
||
| async saveCheckpoint(manifest: CheckpointManifest): Promise<string> { | ||
| await this.init(); | ||
| const id = manifest.id || this.generateCheckpointId(manifest); |
Copilot
AI
Oct 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
src/cli/splice.ts
Outdated
| const source = path.resolve(opts.source); | ||
| const outDir = path.resolve(opts.out); | ||
| const workspaceDir = path.resolve( | ||
| (opts as any).workspace || path.join(outDir, ".splice"), |
Copilot
AI
Oct 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"), |
src/cli/splice.ts
Outdated
| const decisionsPath = (opts as any).decisionsImport as | ||
| | string | ||
| | undefined; |
Copilot
AI
Oct 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/cli/splice.ts
Outdated
| let ids: string[] = Array.isArray((opts as any).ids) | ||
| ? ((opts as any).ids as string[]) | ||
| : []; |
Copilot
AI
Oct 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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