fix(update): mark command-configured tools as needing update when skill version is missing - #1442
fix(update): mark command-configured tools as needing update when skill version is missing#1442hsusul wants to merge 4 commits into
Conversation
…ll version is missing
📝 WalkthroughWalkthroughThe update pipeline now treats generated command files as configured tool artifacts, validates workflow-specific contents, and passes delivery context through status calculations. Tests 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 ToolVersionStatus
participant CommandFiles
UpdateCommand->>ToolVersionStatus: pass workflows and delivery
ToolVersionStatus->>CommandFiles: inspect generated command files
CommandFiles-->>ToolVersionStatus: configured and freshness state
ToolVersionStatus-->>UpdateCommand: return update status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
| const skillConfigured = getToolSkillStatus(projectRoot, toolId).configured; | ||
| const commandConfigured = toolHasAnyConfiguredCommand(projectRoot, toolId); | ||
| const configured = skillConfigured || commandConfigured; | ||
|
|
||
| // 2. If version is not found in skills, check command files | ||
| if (generatedByVersion === null && commandConfigured) { | ||
| const adapter = CommandAdapterRegistry.get(toolId); | ||
| if (adapter) { | ||
| for (const commandId of COMMAND_IDS) { | ||
| const cmdPath = adapter.getFilePath(commandId); | ||
| const fullPath = path.isAbsolute(cmdPath) ? cmdPath : path.join(projectRoot, cmdPath); | ||
| if (fs.existsSync(fullPath)) { | ||
| const version = extractGeneratedByVersion(fullPath); | ||
| if (version !== null) { | ||
| generatedByVersion = version; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (generatedByVersion === null) { | ||
| if (areCommandFilesUpToDate(projectRoot, toolId, options)) { | ||
| generatedByVersion = currentVersion; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how getToolVersionStatus is invoked relative to legacyWorkflowOverrides in update.ts
rg -n "legacyWorkflowOverrides|desiredWorkflows|getToolVersionStatus" src/core/update.ts -C3Repository: Fission-AI/OpenSpec
Length of output: 5283
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tool-detection outline =="
ast-grep outline src/core/shared/tool-detection.ts --view expanded || true
echo "== relevant tool-detection sections =="
sed -n '240,330p' src/core/shared/tool-detection.ts
printf '\n'
rg -n "function areCommandFilesUpToDate|const areCommandFilesUpToDate|areCommandFilesUpToDate|extractGeneratedByVersion|getCommandContents|generate.*Command|CommandAdapterRegistry|COMMAND_IDS|workflow" src/core/shared/tool-detection.ts src/core -g '*.ts' -C2 | head -n 220
echo "== generate command functions =="
rg -n "function getCommandContents|getCommandContents|generateCommand" src/core/command-generation src/core/shared -g '*.ts' -C4 | head -n 260Repository: Fission-AI/OpenSpec
Length of output: 24564
Compute getToolVersionStatus with tool-specific workflow overrides.
Lines 166-170 call getToolVersionStatus(..., { workflows: desiredWorkflows }), but the generation loop uses legacyWorkflowOverrides[tool.value] ?? desiredWorkflows. For tools with a legacy override, the command fingerprint is checked against the wrong workflow set and can yield an incorrect needsUpdate result. Pass the tool-specific workflows to the version-status call, or compute override-aware status inside getToolVersionStatus.
🤖 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 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.
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.
3 participants
Summary
Fixes an issue where
openspec update(without--force) skips updating tools that are configured via command files when no skill files exist on disk (such as underdelivery: "commands"or when only slash command files exist).Reproduction
delivery: "commands"(or a tool having only command files present under.claude/commands/opsx/and no skill files under.claude/skills/).openspec update(without--force).UpdateCommandcomputesstatus.configured = falseingetToolVersionStatus()(since skill files are absent), sostatus.needsUpdateis computed asfalse.update.tspatches{ ...status, configured: true }, butneedsUpdateremainsfalse. As a result,toolsNeedingVersionUpdatefilters out the tool,toolsToUpdateSetis empty, andopenspec updatereports "All configured tools are up to date" without updating the command files.openspec updatedetects that the command-configured tool requires an update (since its version is missing/untracked) and updates its command files to the current OpenSpec version.Root Cause
In
src/core/update.ts:toolStatusesmaps overconfiguredToolsand callsgetToolVersionStatus(). BecausegetToolVersionStatus()inspects only skill directories to findgeneratedByVersionand setconfigured, a tool configured purely by command files returns{ configured: false, generatedByVersion: null, needsUpdate: false }.When
update.tsoverridesconfigured: trueviacommandConfiguredSet.has(toolId), it did not recomputeneedsUpdate. Thus,needsUpdateremainedfalse, causingopenspec updateto bypass the tool during version updates.Implementation
In
src/core/update.ts, when mappingtoolStatuses, if!status.configured && commandConfiguredSet.has(toolId), we setconfigured: trueAND re-evaluateneedsUpdate(status.generatedByVersion === null || status.generatedByVersion !== OPENSPEC_VERSION).Regression Coverage
Added a unit test in
test/core/update.test.ts(should update command files when tool is configured via commands-only delivery without skills) that verifiesopenspec updatecorrectly updates command files for a command-configured tool when no skill files are present.Affected Adapters / Platforms
All tool command adapters when operating in commands-only delivery mode or when only command files are present. Tested cross-platform on Node 20+.
Validation Results
Compatibility Impact
None. Preserves all existing CLI and adapter contracts while ensuring command files are properly refreshed during
openspec update.AI-Assistance Disclosure
Developed with pair-programming assistance from Antigravity AI coding assistant.
Unrelated Changes Confirmation
Confirmed no unrelated changes, lockfile churn, or extra files were included.
Summary by CodeRabbit