fix(update): refresh command files for tools configured without skills - #1442
Conversation
…ll version is missing
|
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:
📝 WalkthroughWalkthroughThe update pipeline now treats generated command files as configured tool artifacts, validates workflow-specific contents, and passes workflow context through status calculations. Tests and documentation cover command-only refreshes, stale command detection, repeated no-op updates, and isolated CLI configuration. ChangesCommand-aware status and update behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant UpdateCommand
participant getToolVersionStatus
participant areCommandFilesUpToDate
UpdateCommand->>getToolVersionStatus: pass selected workflows
getToolVersionStatus->>areCommandFilesUpToDate: validate generated command files
areCommandFilesUpToDate-->>getToolVersionStatus: return freshness state
getToolVersionStatus-->>UpdateCommand: return tool status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/core/update.test.ts (1)
390-403: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert every core command is refreshed.
The test creates six command files but checks only
explore.md, so it can pass while the other five remain stale. Iterate overcoreCommandIdsand verify each generated file changed and contains frontmatter.Proposed test adjustment
- const updatedContent = await fs.readFile(path.join(commandsDir, 'explore.md'), 'utf-8'); - expect(updatedContent).not.toBe('old command content'); - expect(updatedContent).toContain('---'); + for (const cmdId of coreCommandIds) { + const updatedContent = await fs.readFile( + path.join(commandsDir, `${cmdId}.md`), + 'utf-8' + ); + expect(updatedContent).not.toBe('old command content'); + expect(updatedContent).toContain('---'); + }Run with
pnpm exec vitest run test/core/update.test.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/core/update.test.ts` around lines 390 - 403, Update the test case around updateCommand.execute to iterate over every ID in coreCommandIds, read each corresponding command file, and assert its content is no longer the old content and contains frontmatter. Keep the existing commands-only configuration and setup unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/core/update.test.ts`:
- Around line 390-403: Update the test case around updateCommand.execute to
iterate over every ID in coreCommandIds, read each corresponding command file,
and assert its content is no longer the old content and contains frontmatter.
Keep the existing commands-only configuration and setup unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 953a918a-8314-4d90-9479-6ceb5244a240
📒 Files selected for processing (2)
src/core/update.tstest/core/update.test.ts
Code review — verdict: good to mergeI verified this end-to-end rather than taking the description at its word. Everything checks out. 1. Real need — yes, and I reproduced the bugThe described root cause is accurate. Reproduced against That's a genuine user-facing bug, and it's worse than "skips an update" — it reports With this PR applied, the same project correctly updates the file. 2. Does the fix work — yes, and the test is a real regression testThis is the part I most wanted to confirm, since a test that passes both with and without the fix proves nothing. It's a true regression test:
Full suite on the PR branch (rebased on 3. Breaking changes — noneThe new branch is guarded by 4. Scope — correctly surgicalFour lines in one One follow-up worth knowing about (not a blocker)Command files carry no version stamp. I checked the generated frontmatter: ---
name: "OPSX: Apply"
description: Implement tasks from an OpenSpec change (Experimental)
allowed-tools: Bash(openspec:*)
category: Workflow
tags: [workflow, artifacts, experimental]
---No So commands-only users will never see "up to date"; To be clear, I don't think this should block the merge. Always-refreshing is strictly better than never-refreshing, the rewrite is content-idempotent, and given no version is recorded anywhere there's no way for this function to do better. But it does mean the fix trades one wrong report ("up to date" when stale) for a milder one ("needs update" when current). The clean follow-up is to stamp Nit (optional)
|
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Commands-only installs have no persisted generatedBy version, so this makes every later update rewrite the commands and report unknown to current again; the affected code is unchanged from the reproduced two-run hold. Please persist or compare a version/content fingerprint and add a second-run no-op regression.
…ols when skill version is missing
|
I've updated the PR to fix the issue where commands-only installations re-triggered command file generation on every subsequent update. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/core/shared/tool-detection.ts (2)
116-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate COMMAND_IDS existence-scan logic.
toolHasAnyConfiguredCommand(Lines 119-129) and the version-lookup loop ingetToolVersionStatus(Lines 291-301) both iterateCOMMAND_IDS, resolve the adapter path, and checkfs.existsSync. Consider extracting a shared helper (e.g., returning the first existing command file path) that both call sites can reuse.♻️ Proposed extraction
+function findFirstExistingCommandFilePath( + projectPath: string, + adapter: ReturnType<typeof CommandAdapterRegistry.get> +): string | null { + if (!adapter) return null; + for (const commandId of COMMAND_IDS) { + const cmdPath = adapter.getFilePath(commandId); + const fullPath = path.isAbsolute(cmdPath) ? cmdPath : path.join(projectPath, cmdPath); + if (fs.existsSync(fullPath)) return fullPath; + } + return null; +}Also applies to: 291-301
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/shared/tool-detection.ts` around lines 116 - 132, Extract the duplicated COMMAND_IDS path-resolution and fs.existsSync scan into a shared helper that returns the first existing command file path, using the adapter and projectPath inputs. Update toolHasAnyConfiguredCommand and the version-lookup loop in getToolVersionStatus to reuse this helper while preserving their current boolean and version-status behavior.
137-159: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDrop or use
deliveryinareCommandFilesUpToDate.
deliveryis accepted on the options signatures and passed through callers, butareCommandFilesUpToDateonly readsoptions.workflows; keeping dead plumbing makes the API invite future behavior that currently can’t differ by delivery type.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/shared/tool-detection.ts` around lines 137 - 159, Remove the unused delivery property from the options parameter of areCommandFilesUpToDate and update its callers to stop passing delivery, or implement delivery-specific behavior there; prefer removing the dead plumbing since the function currently only selects workflows and does not vary by delivery.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/shared/tool-detection.ts`:
- Around line 283-310: Update the getToolVersionStatus call in the generation
loop to pass legacyWorkflowOverrides[tool.value] ?? desiredWorkflows, matching
the workflow set used for generation. Ensure tools with legacy overrides
evaluate command fingerprints and needsUpdate status against their tool-specific
workflows.
In `@test/helpers/run-cli.ts`:
- Line 151: The default XDG_CONFIG_HOME in runCLI() must be isolated for each
invocation. Replace the shared test/fixtures/.tmp-config fallback with a unique
temporary directory created per call, while preserving
options.env?.XDG_CONFIG_HOME when explicitly provided.
---
Nitpick comments:
In `@src/core/shared/tool-detection.ts`:
- Around line 116-132: Extract the duplicated COMMAND_IDS path-resolution and
fs.existsSync scan into a shared helper that returns the first existing command
file path, using the adapter and projectPath inputs. Update
toolHasAnyConfiguredCommand and the version-lookup loop in getToolVersionStatus
to reuse this helper while preserving their current boolean and version-status
behavior.
- Around line 137-159: Remove the unused delivery property from the options
parameter of areCommandFilesUpToDate and update its callers to stop passing
delivery, or implement delivery-specific behavior there; prefer removing the
dead plumbing since the function currently only selects workflows and does not
vary by delivery.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 12668755-8c21-45a7-814a-e165303009ac
📒 Files selected for processing (7)
src/core/profile-sync-drift.tssrc/core/shared/index.tssrc/core/shared/tool-detection.tssrc/core/update.tstest/core/shared/tool-detection.test.tstest/core/update.test.tstest/helpers/run-cli.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/update.ts
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The repeated-regeneration bug is fixed at 1f51ae5, including the two-run no-op. One blocker remains: the new tool-detection tests call saveGlobalConfig without isolating XDG_CONFIG_HOME, so the focused test run overwrote the real ~/.config/openspec/config.json; the new runCLI fallback also shares one repo-wide .tmp-config across parallel invocations. Please give each test/temp project an isolated config home (while preserving explicit overrides), and expand the first-run regression to assert all six command files.
|
Addressed both blockers: tool-detection tests now isolate XDG_CONFIG_HOME within each test project, and runCLI() creates and cleans a unique config home per invocation while preserving explicit overrides. The first-run regression now checks all six core command files. Full suite passes: 112 files, 2257 tests; build, lint, typecheck, and diff checks also pass. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/helpers/run-cli.ts`:
- Around line 233-236: Update the cleanup logic in runCLI’s finally callback so
failures from fs.rm are caught and reported without being rethrown. Preserve the
original CLI result or error regardless of cleanup outcome, while retaining
conditional removal of isolatedConfigHome.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 99227021-e5a8-4f26-bf17-ea2ffda2dbdc
📒 Files selected for processing (3)
test/core/shared/tool-detection.test.tstest/core/update.test.tstest/helpers/run-cli.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/core/shared/tool-detection.test.ts
| }).finally(async () => { | ||
| if (isolatedConfigHome) { | ||
| await fs.rm(isolatedConfigHome, { recursive: true, force: true }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Prevent cleanup failures from masking the CLI result.
If fs.rm(...) rejects, .finally() replaces a successful result—or the original CLI error—with the cleanup failure. Catch and report cleanup errors without rejecting the main runCLI operation.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { type ChildProcess, spawn } from 'child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/helpers/run-cli.ts` around lines 233 - 236, Update the cleanup logic in
runCLI’s finally callback so failures from fs.rm are caught and reported without
being rethrown. Preserve the original CLI result or error regardless of cleanup
outcome, while retaining conditional removal of isolatedConfigHome.
Review follow-ups on the commands-only update fix: - Only fall back to the command-content fingerprint when a tool has no skill files at all. Gating on `generatedByVersion === null` also swallowed the case where a SKILL.md exists but its version is unreadable, so a truncated or hand-edited skill file could never be repaired by `openspec update` again. - Drop the command `generatedBy` scan: command adapters emit no version stamp, so the loop was unreachable and made the fingerprint fallback read as a secondary path rather than the only one. - Compute version status with the same workflow set the generation loop writes (`legacyWorkflowOverrides[toolId] ?? desiredWorkflows`), so a legacy-upgraded tool is not fingerprinted against commands it was never given. - Remove the unread `delivery` option from the three tool-detection signatures, the leftover `getCommandConfiguredTools` / `COMMAND_IDS` imports, and the unused `toolHasAnyConfiguredCommand` re-export. - runCLI: never let temp-dir cleanup replace the CLI result or a real failure, and treat an explicitly-empty XDG_CONFIG_HOME as an override. Adds regressions for the unreadable-skill case and for a deselected workflow leaving a command file behind, and documents how "up to date" is decided. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round-two review follow-ups: - Command files are committed project files. A Windows clone with `core.autocrlf` re-materializes them with CRLF endings, which the byte-exact comparison read as drift: every fresh checkout spent one `openspec update` rewriting identical content and announcing a bogus "unknown → <version>". Normalize CRLF and a leading BOM on both sides before comparing. - Collapse `getCommandConfiguredTools`, which the widened `getConfiguredTools` made a strict subset of itself, into the single remaining caller. - Correct the new `openspec update` doc paragraph: content drift is only detected for commands-only installs, so it must not promise that hand edits are always overwritten. - Add the changeset this repo requires per fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 63a485a. The commands-only fingerprint now settles after the first rewrite, preserves skill-version drift, isolates every test config home, handles CRLF and BOM without false drift, and checks all six core command files. Build, 105 focused tests, and all CI checks pass.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/core/shared/tool-detection.test.ts`:
- Around line 289-296: Update the command-file content transformation in the
test setup loop to normalize existing CRLF and LF line endings before converting
them to CRLF, avoiding duplicated carriage returns on Windows. Preserve the BOM
prefix and the existing file iteration and write behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ce8298f4-2f2c-432b-b714-03368ca8fad7
📒 Files selected for processing (6)
.changeset/update-detects-command-only-tools.mddocs/cli.mdsrc/core/profile-sync-drift.tssrc/core/shared/tool-detection.tssrc/core/update.tstest/core/shared/tool-detection.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/cli.md
- src/core/update.ts
- src/core/shared/tool-detection.ts
`ToolVersionStatus.configured` and `.generatedByVersion` are now fed by command files too, so their comments no longer say "skills". Removes the barrel exports and the `options` parameter this change added but nothing consumes, and the import left dangling by the previous commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
clay-good
changed the title
Review round: all open comments addressedThree adversarial review passes over the branch, plus end-to-end runs of the built CLI. Pushed as Note: the PR body's Root Cause and Implementation sections describe the original approach (recomputing What shipsSkill files record the version that generated them; command files carry no such stamp. So for a tool that has commands and no skills, Blockers fixed this round
CRLF/BOM broke the fingerprint on Windows. Command files are committed project files. A clone with
Also cleaned up
ProofNew regressions, each verified to fail when its fix is reverted:
End-to-end against the built CLI, isolated
One thing to know before mergingFor |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
# Conflicts: # docs/cli.md
|
Merged Also applied CodeRabbit's last nit ( Green on all three OSes at |
|
Rewrote the PR description: the original Root Cause / Implementation sections still described the first approach (recomputing Also verified the post-merge state, which no earlier review round had seen (#1470 landed inside the same
Gates at |
The fingerprint regressions all ran against claude and the core profile, so two things were covered by reasoning rather than by an executed test: - Command paths differ in shape per adapter. Added a parametrized case over gemini (nested dir, TOML), cursor (flat opsx-* file), and cline, whose commands live in .clinerules/workflows — not in its skillsDir (.cline) at all, so a commands-only install leaves that directory absent. Each asserts detection, a clean fingerprint, and drift. Reverting the getConfiguredTools widening fails all three. - A custom profile must be fingerprinted against its own workflow subset. The new case inits with ['explore', 'apply'] and asserts the same tree reads as drifted when compared against the wider core set. Making the fingerprint ignore the caller's workflows and fall back to global config fails it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Closed the last two coverage gaps — both were previously covered by reasoning rather than by an executed test. Non- Custom profile. Every earlier test hard-coded the six core workflows, so nothing proved the fingerprint honors a narrower selection. The new case inits with Both are real guards, not decoration — each was verified by mutation:
Gates at Ready for review. |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Re-reviewed exact head ffb1fc0 after the main rebase. The fingerprint path now has direct coverage across Gemini, Cursor, Cline, and custom workflow subsets, while the CRLF/BOM, skill-drift, config-isolation, and second-run no-op guarantees remain intact. Build, lint, 109 focused tests, and all hosted checks pass.
3 participants
What was wrong
openspec updatenever refreshed a tool configured with command files but no skills — deliverycommands, or any project whose skill files were removed. It reported✓ All configured tools are up to dateon every run, forever, so command files kept whatever content the CLI that first wrote them produced.Skill files record the version that generated them (
generatedByin frontmatter); command files carry no such stamp.getToolVersionStatus()read only skill files, so a commands-only tool came back{ configured: false, generatedByVersion: null, needsUpdate: false }and got filtered out of the update set.How it was fixed
For a tool that has commands and no skills, compare the command file contents against what the CLI would generate right now. Content equality is the only "is this current?" signal available when there is no version stamp. Everything else — including any tool that has skills — keeps using the recorded version, unchanged.
Three things that approach gets wrong if done naively, each fixed here:
generatedByVersion === nullalso catches "aSKILL.mdexists but its version is unreadable." That let a truncated or hand-edited skill file be masked by matching command files, soopenspec updatecould never repair it again. Gated on!skillConfiguredinstead.core.autocrlfwould spend one spurious update per checkout rewriting identical content and announcing a bogusunknown → 1.6.0. Both sides are normalized.legacyWorkflowOverrides[toolId] ?? desiredWorkflows), or a legacy-upgraded tool is fingerprinted against commands it was never given.A command file left behind by a workflow you have since deselected now also counts as drift, and is removed.
Replication / proof
Reproduce the original bug on
main:Fixed behavior, run end to end against the built CLI with an isolated
XDG_CONFIG_HOME:update✓ All 1 tool(s) up to dateupdateUpdating 1 tool(s): claude (unknown → 1.6.0), file regeneratedupdateagain✓ All 1 tool(s) up to dateverify.md,updateRemoved: 1 command files (deselected workflows)updateagain✓ All 1 tool(s) up to datedelivery: both, truncate aSKILL.md,updateThree new regressions, each verified to fail when its own fix is reverted:
should not let matching command files mask an unreadable skill versionSKILL.mdstill forces a rewriteshould treat CRLF line endings and a BOM as up to date, not as driftshould detect needsUpdate when a deselected workflow left a command file behindnpm run build,npx tsc --noEmit,npm run lint, andnpx vitest run(114 files, 3309 tests) pass. CI green on linux-bash, macos-bash, and windows-pwsh.Notes
clinecommands-only projects work at all. ItsskillsDiris.clinebut commands go to.clinerules/workflows/, so the old directory guard made those projects reportNo configured tools found.skillsorboth— the default — only the recorded version is checked, so a hand-edited generated file whose version still matches is left alone;--forcerewrites it. Content checking applies to commands-only installs.docs/cli.mdnow says this underopenspec update.runCLI()gives every invocation its own config home, and the tool-detection tests stubXDG_CONFIG_HOME, so no test can write the developer's real~/.config/openspec/config.json.mainafter feat(update): offer to upgrade a stale CLI during openspec update #1470 landed; the only conflict was theopenspec updatedoc section, and both sides are independent additions. No source file overlapped.AI-assistance disclosure
Original fix developed with pair-programming assistance from the Antigravity AI coding assistant. Review rounds and the follow-up fixes above were done with Claude Code.