Repository navigation
fix: edge case handling for update, add, and compose - #13
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses edge cases discovered during manual testing across the update, add, and compose commands in instruct-sync, improving behavior when files are missing and when users try to add already-installed packs.
Changes:
update: restores missing target files even when the remote SHA hasn’t changed, and logs “restored” vs “updated”.compose: skips missing pack files with clearer warnings and reports composed vs skipped counts.add: attempts to prevent reinstalling an already-installed pack; README updated with common scenarios; new Vitest coverage added forupdate.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/update.test.ts | Adds coverage for the “restore missing file even when SHA unchanged” and “skip when unchanged + file exists” update behaviors. |
| src/commands/update.ts | Adds existence check for targets to decide between “already up to date” vs “restored/updated”. |
| src/commands/compose.ts | Tracks composed count and improves warnings/logging when some target files are missing. |
| src/commands/add.ts | Adds an “already installed” early-return path before fetching/writing. |
| README.md | Documents common scenarios and expected CLI outputs for restore/add/compose/upgrade flows. |
Comments suppressed due to low confidence (1)
src/commands/update.ts:6
../lockfile.jsis imported twice (once forreadLockfileand again foraddEntry). Consolidating into a single import avoids duplication and keeps the module imports consistent.
import { readLockfile } from "../lockfile.js";
import { parseSource, fetchFile } from "../github.js";
import { writeInstructions } from "../writer.js";
import { addEntry } from "../lockfile.js";
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (composed < entries.length) { | ||
| console.log(`✓ Composed ${composed} of ${entries.length} pack(s) → ${OUTPUT} (${entries.length - composed} skipped — run: instruct-sync update)`); | ||
| } else { | ||
| console.log(`✓ Composed ${composed} pack(s) → ${OUTPUT}`); | ||
| } |
There was a problem hiding this comment.
compose() now changes user-facing behavior by tracking how many packs were actually composed and adjusting the final log line when some are skipped. There are no tests for compose currently; adding a Vitest test for the “missing file(s) → skipped count in output” case would help lock this behavior in.
| const name = nameOrSource.startsWith("github:") | ||
| ? nameOrSource.split("/").pop()!.replace(/\.md$/, "").replace(/^github:/, "") | ||
| : nameOrSource; | ||
|
|
||
| const existing = readLockfile().packs[name]; | ||
| if (existing) { | ||
| console.log(`"${name}" is already installed at ${existing.target}. Run: instruct-sync update`); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The derived name for github: sources is computed from the raw input string, so it can include the @ref suffix (e.g. file.md@main) and won’t strip .md. This will create incorrect targets (like .github/instructions/file.md@main.instructions.md) and also bypass the “already installed” check for the same GitHub source. Derive the name from parseSource(source).path (basename) after parsing, so refs don’t affect naming.
| ```bash | ||
| npm install -g instruct-sync@latest | ||
| ``` | ||
|
|
||
| `npm install -g instruct-sync` without `@latest` will keep the cached version. |
There was a problem hiding this comment.
The README claims npm install -g instruct-sync without @latest “will keep the cached version”, but earlier in the README the same command is presented as the normal install method. npm install -g <pkg> already installs the latest dist-tag by default; consider removing or rewording this to avoid giving incorrect upgrade guidance.
| @@ -0,0 +1,69 @@ | |||
| import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; | |||
| import { mkdtempSync, mkdirSync, writeFileSync, existsSync } from "node:fs"; | |||
There was a problem hiding this comment.
Unused import: existsSync is imported from node:fs but never used in this test file. Removing it will keep the test code clean and avoid unused-import lint failures if linting is added/enabled.
| import { mkdtempSync, mkdirSync, writeFileSync, existsSync } from "node:fs"; | |
| import { mkdtempSync, mkdirSync, writeFileSync } from "node:fs"; |
| const existing = readLockfile().packs[name]; | ||
| if (existing) { | ||
| console.log(`"${name}" is already installed at ${existing.target}. Run: instruct-sync update`); | ||
| return; | ||
| } |
There was a problem hiding this comment.
New behavior added to short-circuit add when a pack is already installed, but there are no tests covering this branch (including the expected console output / lack of network fetch). Adding a Vitest test for this case would prevent regressions.
Fixes 3 bugs found during manual testing. 18 tests passing.