fix: avoid npx when applying profile changes#1351
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe config profile apply flow now calls ChangesConfig apply command
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/config.ts (1)
627-630: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLog the actual error message for better debuggability.
Currently, if the update fails, the underlying error is swallowed and a generic failure message is logged. Capturing and logging the actual error will help users diagnose the root cause (e.g., file permission issues, invalid configurations).
💡 Proposed fix
- } catch { - console.error('`openspec update` failed. Please run it manually to apply the profile changes.'); + } catch (error) { + const errorMessage = error instanceof Error ? error.message : String(error); + console.error(`\`openspec update\` failed: ${errorMessage}`); + console.error('Please run it manually to apply the profile changes.'); process.exitCode = 1; }🤖 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/commands/config.ts` around lines 627 - 630, Update the catch block around the openspec update operation to capture the thrown error and include its message or details in the console.error output alongside the existing failure context, while preserving process.exitCode = 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/commands/config-profile.test.ts`:
- Around line 376-378: Update the test setup around the empty-bin PATH scenario
to use Vitest’s vi.stubEnv() instead of assigning process.env.PATH directly.
Ensure the stub is restored through the test lifecycle so subsequent tests
retain the original PATH and can resolve system binaries.
---
Outside diff comments:
In `@src/commands/config.ts`:
- Around line 627-630: Update the catch block around the openspec update
operation to capture the thrown error and include its message or details in the
console.error output alongside the existing failure context, while preserving
process.exitCode = 1.
🪄 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
Run ID: 9d9fb890-ed45-4603-a338-e3b4e6fb93a4
📒 Files selected for processing (2)
src/commands/config.tstest/commands/config-profile.test.ts
Summary
Fixes the interactive
openspec config profileapply step by invokingopenspec updatedirectly instead of routing throughnpx.Closes #1304 #1306 .
Problem
After saving profile or workflow changes, the config wizard prompts:
When confirmed, it currently runs:
This unnecessarily depends on npm/npx and may fail when:
The
openspecexecutable is already available onPATH, so invoking it throughnpxis unnecessary.Changes
npx openspec updatewithopenspec updateVerification
test/commands/config-profile.test.ts: 19 tests passednode build.js: passedgit diff --check: passedSummary by CodeRabbit