feat: migrate to Bun runtime and @clanker-guru/openspec #1
Conversation
- Rename package from @fission-ai/openspec to @clanker-guru/openspec - Switch all scripts and tooling to use bun instead of npm/pnpm - Convert test framework from vitest to bun:test - Migrate all JavaScript files to TypeScript (.ts) - Update CI/CD workflows for bun runtime - Remove npm, node, pnpm, vitest, and changesets dependencies - Add bun.lock and bun-based devcontainer configuration
- Update version to 0.0.1-alpha.1 (fresh start for @clanker-guru/openspec) - Simplify CI workflow: unified matrix testing across OS - Fix publish workflow: use NPM_CONFIG_TOKEN, add --tag and --tolerate-republish - Make bin/openspec.ts executable
…active mode in change tests
|
Warning Rate limit exceeded@ClankerGuru has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 4 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMigrates repository tooling and CI from Node/PNPM/Vitest to Bun: replaces build, pack, postinstall, CLI shebangs, tests, devcontainer and workflows; removes Changesets and Vitest configs; updates package metadata and many tests to use bun and OS-temporary directories. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
…Windows hanging" This reverts commit 0b446d2.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/commands/validate.enriched-output.test.ts (1)
29-30: Cross-platform compatibility issue with shell commands.The use of
mkdir -pandbash -lcwith heredoc syntax will fail on Windows. Since the CI matrix now includeswindows-latest, consider using Node.js/Bun fs APIs instead:Suggested cross-platform fix
- execSync(`mkdir -p ${changePath}`); - execSync(`bash -lc "cat > ${path.join(changePath, 'proposal.md')} <<'EOF'\n${changeContent}\nEOF"`); + await fs.mkdir(changePath, { recursive: true }); + await fs.writeFile(path.join(changePath, 'proposal.md'), changeContent, 'utf-8');Note: The test function signature would need to be
async(which it can be in bun:test).
🧹 Nitpick comments (8)
test/core/commands/change-command.show-validate.test.ts (2)
49-58: Simplify the type coercion.The type coercion
undefined as unknown as stringis unnecessary. According to the function signature in the relevant code snippet,changeNameis optional (changeName?: string), so you can passundefineddirectly without casting.🔎 Proposed simplification
- await cmd.show(undefined as unknown as string, { json: false, noInteractive: true }); + await cmd.show(undefined, { json: false, noInteractive: true });
60-74: Consider verifying the deprecation warning message.The test validates that the deprecated
--requirements-onlyflag still works correctly, but doesn't verify that the deprecation warning is actually printed. According to the relevant code snippet, the implementation should print: "Flag --requirements-only is deprecated; use --deltas-only instead."Adding a verification would ensure the warning isn't accidentally removed and helps users migrate to the new flag.
🔎 Suggested addition to verify deprecation warning
After line 63, add verification of the deprecation warning:
await cmd.show(changeName, { json: true, requirementsOnly: true }); // Verify deprecation warning is printed to stderr const warningOutput = consoleErrorSpy.mock.calls.flat().join('\n'); expect(warningOutput).toMatch(/--requirements-only is deprecated/); const output = consoleSpy.mock.calls.flat().join('\n');.devcontainer/devcontainer.json (1)
3-3: Consider pinning Bun container version for stability.Using
oven/bun:latestmay introduce breaking changes when Bun releases new versions. Consider pinning to a specific version (e.g.,oven/bun:1.1or a specific semver) to ensure consistent development environments across the team and prevent unexpected breakage.🔎 Recommended version pinning
- "image": "oven/bun:latest", + "image": "oven/bun:1.1",Or check the latest stable version:
What is the latest stable version of oven/bun Docker image?test/core/update.test.ts (1)
1655-1662: Replacevi.spyOnwithspyOnfor consistency.The code uses
spyOnfrom bun:test throughout the file, but lines 1655-1662 usevi.spyOn. For consistency with bun:test patterns and the rest of the file, use the standalonespyOnfunction.🔎 Proposed consistency fix
- const writeSpy = vi - .spyOn(FileSystemUtils, 'writeFile') - .mockImplementation(async (filePath, content) => { + const writeSpy = spyOn(FileSystemUtils, 'writeFile').mockImplementation( + async (filePath, content) => { if (filePath.endsWith('CLAUDE.md')) { throw new Error('EACCES: permission denied, open'); } return originalWriteFile(filePath, content); - }); + } + );test/core/archive.test.ts (1)
555-556: Clarify intent of standalonefs.accesscalls.These lines call
await fs.access(changeDir)without assertions. If the intent is to verify the directory still exists (wasn't archived), consider making this explicit:- await fs.access(changeDir); + // Verify change was NOT archived (directory still exists) + await expect(fs.access(changeDir)).resolves.toBeUndefined();This improves readability and makes the test intent clear.
Also applies to: 672-673, 796-797
test/commands/completion.test.ts (1)
36-37: Consider aligning mock result types to avoidas anycasts.The
as anycasts onmockResolvedValuesuggest the mock result types don't fully match the expected return types. Consider updatingmockInstallResultto match the actualInstallResultinterface.🔎 Type-safe mock result suggestion
+import type { InstallResult, UninstallResult } from '../../src/core/completions/installers/base-installer.js'; -const mockInstallResult = { +const mockInstallResult: InstallResult = { success: true, installedPath: '/home/user/.oh-my-zsh/completions/_openspec', isOhMyZsh: true, message: 'Completion script installed successfully for Oh My Zsh', instructions: [ 'Completion script installed to Oh My Zsh completions directory.', 'Restart your shell or run: exec zsh', 'Completions should activate automatically.', ], };Also applies to: 198-202, 214-217
.github/workflows/publish.yml (1)
36-42: Preferjqor ESM syntax for package.json manipulation.The inline script uses
require()which relies on Bun's CommonJS compatibility. For robustness, consider usingjq:🔎 Alternative using jq
- name: Set version run: | - bun -e " - const pkg = require('./package.json'); - pkg.version = '${{ inputs.version }}'; - require('fs').writeFileSync('./package.json', JSON.stringify(pkg, null, 2) + '\n'); - " + jq --arg v "${{ inputs.version }}" '.version = $v' package.json > tmp.json && mv tmp.json package.jsonOr use ESM imports with Bun:
🔎 Alternative using ESM
- name: Set version run: | - bun -e " - const pkg = require('./package.json'); - pkg.version = '${{ inputs.version }}'; - require('fs').writeFileSync('./package.json', JSON.stringify(pkg, null, 2) + '\n'); - " + bun -e " + import pkg from './package.json'; + pkg.version = '${{ inputs.version }}'; + await Bun.write('./package.json', JSON.stringify(pkg, null, 2) + '\n'); + "test/commands/spec.interactive-show.test.ts (1)
30-43: Consider explicit environment key restoration.Reassigning
process.env = originalEnvdoesn't remove keys that were added during the test. While this is a pre-existing pattern, consider explicitly deleting or restoring the specific key for cleaner test isolation:Suggested approach
- const originalEnv = { ...process.env }; + const originalInteractive = process.env.OPEN_SPEC_INTERACTIVE; // ... - process.env = originalEnv; + if (originalInteractive === undefined) { + delete process.env.OPEN_SPEC_INTERACTIVE; + } else { + process.env.OPEN_SPEC_INTERACTIVE = originalInteractive; + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (61)
.changeset/README.md.changeset/config.json.devcontainer/README.md.devcontainer/devcontainer.json.github/workflows/ci.yml.github/workflows/opencode.yml.github/workflows/publish.yml.github/workflows/release-prepare.ymlREADME.mdbin/openspec.jsbin/openspec.tsbuild.jsbuild.tseslint.config.tspackage.jsonscripts/pack-version-check.mjsscripts/pack-version-check.tsscripts/postinstall.tssrc/core/init.tssrc/core/templates/index.tstest/cli-e2e/basic.test.tstest/commands/change.interactive-show.test.tstest/commands/change.interactive-validate.test.tstest/commands/completion.test.tstest/commands/config.test.tstest/commands/show.test.tstest/commands/spec.interactive-show.test.tstest/commands/spec.interactive-validate.test.tstest/commands/spec.test.tstest/commands/validate.enriched-output.test.tstest/commands/validate.test.tstest/core/archive.test.tstest/core/artifact-graph/graph.test.tstest/core/artifact-graph/resolver.test.tstest/core/artifact-graph/schema.test.tstest/core/artifact-graph/state.test.tstest/core/artifact-graph/workflow.integration.test.tstest/core/commands/change-command.list.test.tstest/core/commands/change-command.show-validate.test.tstest/core/completions/completion-provider.test.tstest/core/completions/generators/zsh-generator.test.tstest/core/completions/installers/zsh-installer.test.tstest/core/config-schema.test.tstest/core/converters/json-converter.test.tstest/core/global-config.test.tstest/core/init.test.tstest/core/list.test.tstest/core/parsers/change-parser.test.tstest/core/parsers/markdown-parser.test.tstest/core/update.test.tstest/core/validation.enriched-messages.test.tstest/core/validation.test.tstest/core/view.test.tstest/helpers/run-cli.tstest/utils/change-utils.test.tstest/utils/file-system.test.tstest/utils/interactive.test.tstest/utils/marker-updates.test.tstest/utils/shell-detection.test.tsvitest.config.tsvitest.setup.ts
💤 Files with no reviewable changes (8)
- .changeset/config.json
- bin/openspec.js
- vitest.setup.ts
- scripts/pack-version-check.mjs
- .github/workflows/release-prepare.yml
- .changeset/README.md
- vitest.config.ts
- build.js
🧰 Additional context used
🧬 Code graph analysis (4)
test/core/archive.test.ts (1)
src/core/archive.ts (1)
ArchiveCommand(19-625)
test/utils/file-system.test.ts (1)
src/utils/file-system.ts (1)
FileSystemUtils(44-209)
scripts/postinstall.ts (1)
src/utils/shell-detection.ts (1)
detectShell(21-62)
test/core/update.test.ts (1)
src/utils/file-system.ts (1)
FileSystemUtils(44-209)
🔇 Additional comments (83)
test/utils/change-utils.test.ts (3)
1-1: ✓ Clean migration to bun:test.The import statement correctly transitions from vitest to bun:test, with all required test utilities (describe, it, expect, beforeEach, afterEach) properly imported. The migration is mechanical and accurate.
6-6: ✓ Source import path correct for Bun module resolution.The
.jsextension on the TypeScript source import is the correct pattern for ESM in Bun and will resolve properly to the actualchange-utils.tsfile with standard TypeScript configuration.
114-121: ✓ Proper test setup/teardown with isolated temp directories.The beforeEach/afterEach pattern correctly creates unique temporary directories per test (using randomUUID) and properly cleans them up with recursive force removal, preventing test pollution and resource leaks.
src/core/init.ts (1)
16-17: LGTM! Clean separation of runtime and type-only imports.This change correctly splits the import to keep
TemplateManageras a runtime import while importingProjectContextas a type-only dependency. This aligns with the concurrent change insrc/core/templates/index.tsand follows TypeScript best practices for reducing runtime coupling while preserving type safety.src/core/templates/index.ts (1)
49-49: LGTM! Appropriate type-only export.Converting
ProjectContextto a type-only export is correct. The module still importsProjectContextas a value at line 2 for internal use inTemplateManager.getTemplates, but external consumers (likesrc/core/init.ts) only need it as a type annotation, not as a runtime value. This change reduces runtime coupling while maintaining full type safety.test/core/artifact-graph/state.test.ts (1)
1-1: LGTM! Clean test framework migration.The import change from Vitest to Bun's built-in test runner is correct. The test logic remains unchanged, which is the appropriate approach for a framework migration.
test/core/commands/change-command.list.test.ts (1)
1-1: LGTM! Migration to bun:test is complete and consistent.The test framework import has been correctly updated from vitest to bun:test. The APIs used (describe, it, expect, beforeAll, afterAll) are fully compatible, and across the 38 test files in the codebase, all have been consistently migrated to bun:test with no remaining vitest imports.
test/commands/spec.interactive-validate.test.ts (2)
1-1: LGTM! Clean migration to Bun test framework.The import changes correctly migrate from Vitest to
bun:test, and the addition ofosandrandomUUIDenables proper test isolation through unique temporary directories.Also applies to: 5-6
10-11: Excellent test isolation strategy.Converting
testDirandspecsDirtoletand initializing them dynamically withrandomUUID()inbeforeEachensures proper test isolation and prevents conflicts between concurrent test runs.Also applies to: 16-17
test/core/commands/change-command.show-validate.test.ts (6)
1-1: LGTM: Clean migration to bun:test imports.The test framework migration from Vitest to bun:test is correct, and the addition of
randomUUIDfor temp directory isolation is a best practice for preventing conflicts in parallel test execution.Also applies to: 6-6
13-14: LGTM: Proper spy type declarations.Using
ReturnType<typeof spyOn>correctly captures the bun:test spy type and enables type-safe access to mock methods and properties.
16-28: LGTM: Comprehensive test setup with proper isolation.The
beforeEachhook correctly:
- Creates isolated temp directories using
randomUUIDfor parallel-safe execution- Sets up test fixtures with appropriate directory structure
- Initializes console spies to capture output without polluting test logs
- Manages process working directory appropriately
30-36: LGTM: Thorough cleanup with proper state restoration.The
afterEachhook correctly restores all test state:
- Mock restoration in reverse order of setup
- Working directory restoration
- Resetting
process.exitCodeto prevent test interference- Cleanup of temp directories with
force: truefor reliability
38-47: LGTM: Correct spy-based output validation.The test properly uses
consoleSpy.mock.calls.flat().join('\n')to retrieve console output and validates the JSON structure. This pattern correctly handles multiple console.log calls and is idiomatic for bun:test.
76-86: LGTM: Proper validation of JSON report structure.The test correctly uses the spy-based approach to capture and validate the JSON report output, ensuring both the
validboolean andissuesarray are present in the response.test/utils/shell-detection.test.ts (1)
1-1: Clean migration to Bun test framework.The import change from Vitest to bun:test is straightforward, and the test APIs are compatible. No issues observed.
test/core/artifact-graph/resolver.test.ts (1)
1-1: Clean migration to Bun test framework.The test framework migration is straightforward with compatible APIs.
test/utils/interactive.test.ts (1)
1-1: Clean migration to Bun test framework.The import change is correct and test logic remains intact.
test/core/parsers/markdown-parser.test.ts (1)
1-1: Clean migration to Bun test framework.Import change is correct and preserves all test functionality.
test/utils/marker-updates.test.ts (1)
1-1: Clean migration to Bun test framework.The framework migration is correctly implemented.
test/core/view.test.ts (1)
1-1: Clean migration to Bun test framework.Import change is correct and maintains test compatibility.
test/core/completions/installers/zsh-installer.test.ts (1)
1-1: Clean migration to Bun test framework.The framework migration preserves all test logic correctly.
test/cli-e2e/basic.test.ts (1)
1-1: Clean migration to Bun test framework.The test framework migration is correctly implemented. All test APIs (describe, it, expect, afterAll) are compatible between Vitest and bun:test.
test/core/list.test.ts (1)
1-1: LGTM!The migration to
bun:testis consistent with the project-wide testing framework update.test/core/validation.test.ts (1)
1-1: LGTM!Test framework migration is consistent with the broader Bun adoption across the test suite.
test/core/completions/completion-provider.test.ts (1)
1-1: LGTM!The test framework migration maintains the existing test structure and timing-sensitive test logic.
test/core/artifact-graph/schema.test.ts (1)
1-1: LGTM!Test framework migration aligns with the project-wide Bun adoption.
test/commands/validate.test.ts (1)
1-1: LGTM!The test framework migration is consistent with the project-wide changes. The CLI execution tests should work correctly with Bun's test runner.
test/core/validation.enriched-messages.test.ts (1)
1-1: LGTM!Test framework migration maintains existing validation test logic.
bin/openspec.ts (1)
1-3: Build setup is correctly configured.The Bun shebang and import structure are properly configured for the migration:
- ✓
dist/cli/index.jswill be built by thebun run buildcommand (tsconfig.json configured with rootDir="./src" and outDir="./dist")- ✓
bin/openspec.tshas executable permissions (verified: -rwxr-xr-x)- ✓
package.jsonbin field correctly references./bin/openspec.tsThe import path
../dist/cli/index.jscorrectly resolves from the bin/ directory to the compiled output location.test/core/converters/json-converter.test.ts (1)
1-1: Test framework is properly configured for Bun's test runner.The import from
bun:testis correct, and all test primitives (describe,it,expect,beforeEach,afterEach) are compatible. The assertions used throughout the file (toBe,toContain,toHaveLength,toBeDefined,not.toThrow) are all supported by Bun's test framework.test/core/artifact-graph/graph.test.ts (1)
1-1: LGTM! Clean test framework migration.The import change from Vitest to bun:test is consistent with the PR's migration objectives.
test/core/config-schema.test.ts (1)
1-1: LGTM! Test framework migration.The switch to bun:test is appropriate and aligns with the PR objectives.
test/core/parsers/change-parser.test.ts (1)
1-1: LGTM! Test framework migration.test/core/completions/generators/zsh-generator.test.ts (1)
1-1: LGTM! Test framework migration.Adding
beforeEachto the bun:test imports is appropriate for the test setup pattern used in this file.test/utils/file-system.test.ts (2)
1-1: LGTM! Test framework migration.
37-42: Pattern is safe; bun:test properly fails tests on unhandled promise rejections.The implicit test pattern (awaiting the call without explicit assertion) is valid in bun:test. The test runtime automatically catches unhandled promise rejections during test execution and marks the test as failed, so if
FileSystemUtils.createDirectory(dirPath)throws, the test will fail as expected. This pattern is also consistent with other tests in the codebase.test/core/global-config.test.ts (2)
1-1: LGTM! Test framework migration.The addition of
mockandspyOnimports from bun:test is appropriate for the spy/mock patterns used in this test suite.
18-18: LGTM! Proper spy API migration.The changes correctly migrate from Vitest's
vi.spyOnto Bun'sspyOnAPI, including the appropriate type annotations.Also applies to: 29-29
test/core/artifact-graph/workflow.integration.test.ts (1)
1-1: LGTM! Test framework migration..devcontainer/devcontainer.json (1)
40-46: LGTM! Dev container migration to Bun is well-executed.All changes are consistent:
- Bun image and runtime
- Updated user from
nodetobun- SSH mount path correctly adjusted
- Package installation using
bun install.devcontainer/README.md (1)
1-87: LGTM! Documentation accurately reflects Bun migration.All command examples and references have been properly updated from Node.js/pnpm to Bun, including:
- Runtime and package manager descriptions
- Setup instructions
- Daily development commands
- Container user references
The documentation is clear, consistent, and helpful for developers.
test/core/update.test.ts (1)
10-24: Excellent addition of robust log message finder!The
findLogMessagehelper function is a great improvement that makes tests more resilient to parallel execution and call order variations. It searches through all spy calls instead of assuming a specific index, which prevents flaky test failures.scripts/pack-version-check.ts (2)
1-73: LGTM! Version check script is well-implemented.The script correctly:
- Uses Bun runtime with proper shebang
- Creates a packed tarball with
bun pm pack- Sets up an isolated test environment
- Installs and verifies the packed version
- Cleans up resources properly in finally block
- Provides clear error messages
The logic is sound and the implementation follows best practices.
46-47: Bin path is correctly configured.The path
node_modules/@clanker-guru/openspec/bin/openspec.tsmatches the package.jsonbinfield ("openspec": "./bin/openspec.ts"). When the package is installed, the bin file will be placed at this location. Bun executes the TypeScript file directly via the#!/usr/bin/env bunshebang without requiring pre-compilation.eslint.config.ts (1)
33-33: The ESLint configuration is correctly restricted to TypeScript files only viafiles: ['src/**/*.ts']and will not lint any JavaScript files. The ignores section appropriately excludesdist/**andnode_modules/**. No.jsor.mjsfiles exist in the repository that would be affected by the ignores configuration.Likely an incorrect or invalid review comment.
test/commands/config.test.ts (1)
36-36: No test isolation issue from removingvi.resetModules().The functions in
global-config.tsreadprocess.env.XDG_CONFIG_HOMEdynamically at call time (not at module initialization), so module caching does not affect test isolation. Each test properly sets and restores the environment variable inbeforeEach/afterEach, ensuring tests work correctly without module cache resets.Likely an incorrect or invalid review comment.
.github/workflows/opencode.yml (1)
1-31: LGTM! OpenCode integration looks good.The workflow is well-configured with:
- Appropriate trigger events for issue and PR review comments
- Flexible condition checking for
/ocand/opencodecommands- Properly scoped permissions
- Correct action usage with API key from secrets
The use of
sst/opencode/github@latestaligns with the official action documentation's recommended approach.test/commands/change.interactive-validate.test.ts (1)
36-36: No changes needed — the test's use ofexecSyncis cross-platform and not affected by the Windows-specific spawn/timeout fixes mentioned in the PR.The Windows fixes in commit 0b446d2 addressed
spawn()hanging issues in the helper file (test/helpers/run-cli.ts), which usesshell: process.platform === 'win32'and timeout management. This test usesexecSync, a simpler synchronous API that avoids spawn-related complexity. Multiple other tests throughout the codebase use the identicalexecSync(bun ${path}pattern, and the CI test matrix includeswindows-latest`, confirming cross-platform compatibility.test/helpers/run-cli.ts (3)
62-62: LGTM on Bun build command migration.The build invocation correctly switches to Bun runtime.
75-104: Good consolidation of timeout handling.The
DEFAULT_TIMEOUT_MSconstant (30s) is reasonable for CLI tests, and the timeout logic is cleanly structured. The Windows shell handling on line 93 ensures cross-platform compatibility.
84-84: CI workflows already configure Bun setup before test execution viaoven-sh/setup-bun@v2on all platforms (ubuntu-latest, macos-latest, windows-latest), so this concern is already addressed.test/core/archive.test.ts (3)
1-7: Clean migration to bun:test with improved test isolation.Good use of
randomUUIDfor unique temp directories, preventing test interference.
362-378: Good pattern: spy restoration in finally block.The try/finally pattern ensures spies are restored even if assertions fail, preventing test pollution.
10-13: Bun's mock.module correctly intercepts dynamic imports.Verified that Bun's
mock.moduleAPI works with dynamicimport()calls. The test properly mocks bothselectandconfirmfrom'@inquirer/prompts'beforeArchiveCommand.selectChangeexecutes its dynamic imports, so the mocked functions will be returned instead of the original module.test/core/init.test.ts (4)
1-1: Consistent bun:test migration with proper mock patterns.The
mock()function usage for the prompt mock is appropriate for Bun's API.Also applies to: 13-18
46-46: Note:mockClearvsmockResetbehavior difference.Bun's
mockClearclears call history but preserves implementation, while Vitest'smockResetalso resets the implementation. Verify this is the intended behavior formockPrompt.
59-60: Good documentation of Bun spy behavior.The comment explaining that
spyOndoes NOT auto-restore is helpful for maintainers.
1647-1667: Proper error handling test with cleanup.The try/finally pattern ensures
writeFileSpyis restored even if the test fails, and the test correctly validates permission error handling.test/commands/completion.test.ts (2)
1-5: Clean migration to targeted spying pattern.Using
spyOn(ZshInstaller.prototype, 'method')is more precise than broad module mocking and aligns with Bun's testing philosophy.
31-47: Complete spy lifecycle management.All spies are created in
beforeEachand restored inafterEach, preventing test pollution.README.md (2)
133-144: Documentation correctly reflects Bun migration.The installation instructions are clear, showing both
bunxfor one-time use andbun install -gfor global installation.
377-380: Contributing section updated consistently.All development commands correctly use Bun equivalents.
test/commands/change.interactive-show.test.ts (2)
1-6: Consistent test isolation pattern.Using
randomUUID()for temp directories ensures parallel test runs don't interfere.
36-36: CLI invocation updated to Bun.The
bun ${bin}pattern is consistent with the project's Bun migration.test/commands/show.test.ts (2)
1-6: Well-structured test setup with proper isolation and cleanup.The beforeEach/afterEach pattern with UUID-based temp directories is consistent across the test suite.
Also applies to: 16-34
44-44: All CLI invocations consistently use Bun.The
bun ${openspecBin}pattern is applied uniformly across all test cases.Also applies to: 63-63, 76-76, 97-97, 115-115
.github/workflows/publish.yml (2)
1-14: Well-structured manual publish workflow.The
workflow_dispatchtrigger with version and tag inputs provides controlled releases.
44-47:bun publishis a supported command with official documentation and is used correctly in this workflow. No action needed.test/commands/spec.test.ts (3)
1-6: LGTM! Clean migration to bun:test with proper test isolation.The switch to
bun:testimports and the addition ofosandrandomUUIDfor generating unique per-test temporary directories is a solid pattern for test isolation.
10-17: Good isolation pattern with unique temp directories.The dynamic
testDirandspecsDirinitialization per test run usingos.tmpdir()andrandomUUID()ensures proper test isolation and avoids conflicts in parallel test execution.
66-68: Consistent Bun CLI invocation across spec show tests.The migration from
nodetobunfor CLI invocation is consistently applied across all spec show test cases.test/commands/spec.interactive-show.test.ts (1)
1-6: LGTM! Consistent migration pattern.The test file follows the same migration pattern as other test files with proper bun:test imports and dynamic temp directory setup.
test/commands/validate.enriched-output.test.ts (1)
1-6: LGTM! Consistent migration pattern.The test file follows the same Bun migration conventions as the other test files.
.github/workflows/ci.yml (2)
17-36: Clean CI structure with cross-platform matrix.The test job structure with OS matrix and
fail-fast: falseis appropriate for catching platform-specific issues. The 10-minute timeout is reasonable for a test suite.
38-54: LGTM! Lint job properly configured.Running lint and typecheck on a single OS (ubuntu-latest) is efficient, and building before typecheck ensures necessary artifacts are available.
package.json (2)
2-3: Package rename and version reset acknowledged.The rename to
@clanker-guru/openspecand version reset to0.0.1-alpha.1aligns with the fork/migration objectives.
55-61: LGTM! Clean devDependencies for Bun-based development.The updated devDependencies properly reflect the Bun migration, with
@types/bunfor type support andjitifor ESLint TypeScript config loading.scripts/postinstall.ts (5)
1-1: LGTM! Bun shebang for direct execution.The
#!/usr/bin/env bunshebang enables direct execution of the TypeScript file via Bun.
10-20: LGTM! Clean skip logic with structured return type.The
shouldSkipInstallationfunction properly checks for CI environments and respects theOPENSPEC_NO_COMPLETIONSenvironment variable.
32-40: LGTM! Proper shell detection wrapper with graceful fallback.The function correctly extracts the
shellproperty from the detection result and silently falls back toundefinedon errors, which is appropriate for postinstall behavior.
42-76: LGTM! User-friendly completion installation with helpful guidance.The function provides clear, context-specific instructions for different zsh configurations (Oh My Zsh, manual fpath setup) and gracefully falls back to a manual tip on failure.
78-103: LGTM! Robust main flow with graceful error handling.The orchestration flow is clean, and the top-level
.catch(() => process.exit(0))ensures that postinstall errors never break the package installation—an important property for postinstall scripts.
| - uses: oven-sh/setup-bun@v2 | ||
| with: | ||
| version: 9 | ||
|
|
||
| - name: Setup Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: '20' | ||
| cache: 'pnpm' | ||
|
|
||
| - name: Print environment diagnostics | ||
| run: | | ||
| node -p "JSON.stringify({ platform: process.platform, arch: process.arch, shell: process.env.SHELL || process.env.ComSpec || '' })" | ||
| bun-version: latest |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
latest stable version of Bun runtime December 2025
💡 Result:
The latest stable Bun runtime is v1.3.5 — released December 17, 2025. [1][2]
Sources:
[1] Bun release notes: Bun v1.3.5.
[2] endoflife.date entry for Bun (shows latest 1.3.5, Dec 17, 2025).
Consider pinning Bun version for reproducible builds.
Using bun-version: latest may lead to unpredictable CI behavior when Bun releases updates. Consider pinning to a specific version for more reproducible builds:
Suggested fix
- uses: oven-sh/setup-bun@v2
with:
- bun-version: latest
+ bun-version: "1.3.x"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: oven-sh/setup-bun@v2 | |
| with: | |
| version: 9 | |
| - name: Setup Node.js | |
| uses: actions/setup-node@v4 | |
| with: | |
| node-version: '20' | |
| cache: 'pnpm' | |
| - name: Print environment diagnostics | |
| run: | | |
| node -p "JSON.stringify({ platform: process.platform, arch: process.arch, shell: process.env.SHELL || process.env.ComSpec || '' })" | |
| bun-version: latest | |
| - uses: oven-sh/setup-bun@v2 | |
| with: | |
| bun-version: "1.3.x" |
🤖 Prompt for AI Agents
.github/workflows/ci.yml around lines 28 to 30: the workflow pins Bun with
bun-version: latest which can cause non-reproducible CI when Bun updates; change
this to a specific, tested Bun version (e.g., a fixed semantic version or
commit) by replacing "latest" with the chosen version string and update any
related workflow docs or matrix entries so CI consistently uses that pinned
version.
There was a problem hiding this comment.
@ClankerGuru, got it! I'll keep that in mind for future reviews. 🚀
✏️ Learnings added
Learnt from: ClankerGuru
Repo: ClankerGuru/OpenSpec PR: 1
File: .github/workflows/ci.yml:28-30
Timestamp: 2025-12-28T09:37:00.257Z
Learning: In the clanker-guru/openspec project CI workflows, the maintainers prefer using `bun-version: latest` rather than pinning to a specific Bun version, accepting the trade-off for staying current with the latest Bun releases.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: ClankerGuru
Repo: ClankerGuru/OpenSpec PR: 1
File: package.json:29-31
Timestamp: 2025-12-28T09:27:28.326Z
Learning: The clanker-guru/openspec package is intentionally designed to run exclusively on the Bun runtime. The bin entry points to a TypeScript file (bin/openspec.ts) with a Bun shebang (#!/usr/bin/env bun), and this is by design, not a compatibility issue.
Learnt from: ClankerGuru
Repo: ClankerGuru/OpenSpec PR: 1
File: package.json:51-51
Timestamp: 2025-12-28T09:28:29.872Z
Learning: The clanker-guru/openspec package maintainers explicitly do not want npm compatibility or fallbacks. The package is intentionally Bun-exclusive, and npm-based workflows should not be suggested.
| try { | ||
| await $`bun x tsc --version`; | ||
| await $`bun x tsc`; | ||
| console.log('\n✅ Build completed successfully!'); | ||
| } catch { | ||
| console.error('\n❌ Build failed!'); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
Log the error before exiting.
The catch block at line 18 suppresses all error details from TypeScript compilation failures. Without logging the error, developers won't see what went wrong during the build.
🔎 Proposed fix to log compilation errors
console.log('Compiling TypeScript...');
try {
await $`bun x tsc --version`;
await $`bun x tsc`;
console.log('\n✅ Build completed successfully!');
-} catch {
+} catch (error) {
console.error('\n❌ Build failed!');
+ console.error(error);
process.exit(1);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| await $`bun x tsc --version`; | |
| await $`bun x tsc`; | |
| console.log('\n✅ Build completed successfully!'); | |
| } catch { | |
| console.error('\n❌ Build failed!'); | |
| process.exit(1); | |
| } | |
| try { | |
| await $`bun x tsc --version`; | |
| await $`bun x tsc`; | |
| console.log('\n✅ Build completed successfully!'); | |
| } catch (error) { | |
| console.error('\n❌ Build failed!'); | |
| console.error(error); | |
| process.exit(1); | |
| } |
🤖 Prompt for AI Agents
In build.ts around lines 14 to 21, the catch block currently swallows the
TypeScript compilation error; update the catch to accept the error (e.g., catch
(err)) and log a helpful message and the error details before calling
process.exit(1) so developers can see the compilation failure and its
stack/diagnostics.
| "bin": { | ||
| "openspec": "./bin/openspec.js" | ||
| "openspec": "./bin/openspec.ts" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp/repository && pwdRepository: ClankerGuru/OpenSpec
Length of output: 129
🏁 Script executed:
# First, check if the bin/openspec.ts file exists and read its shebang
if [ -f "bin/openspec.ts" ]; then
echo "=== bin/openspec.ts (first 20 lines) ==="
head -20 bin/openspec.ts
else
echo "bin/openspec.ts not found"
fiRepository: ClankerGuru/OpenSpec
Length of output: 155
🏁 Script executed:
# Read package.json to understand the full context
echo "=== package.json (full content) ==="
cat package.jsonRepository: ClankerGuru/OpenSpec
Length of output: 1839
🏁 Script executed:
# Check if compiled CLI entry point exists
if [ -f "dist/cli/index.js" ]; then
echo "=== dist/cli/index.js exists ==="
head -5 dist/cli/index.js
else
echo "dist/cli/index.js does not exist"
fi
# Also check dist structure
echo ""
echo "=== dist directory structure ==="
find dist -type f -name "*.js" | head -20Repository: ClankerGuru/OpenSpec
Length of output: 187
🌐 Web query:
npm bin entry point TypeScript file with Bun shebang global installation
💡 Result:
Short answer — two practical options:
- Ship a real npm global CLI that works for npm/yarn users (recommended)
- Compile your TypeScript to JS in build step (tsc/bundler) and set package.json "bin" to the compiled JS file (with a Node shebang).
- package.json:
"bin": { "my-cli": "dist/index.js" } - dist/index.js first line: #!/usr/bin/env node
- package.json:
- Publish; npm install -g will place the executable in PATH and run with Node. (This is the standard, portable approach.) [1][2]
- If you want a TypeScript source entry and Bun runtime
- You can publish with a .ts file in "bin" and either:
- Use a Bun shebang in the TS file: #!/usr/bin/env bun — that will run with Bun (requires users to have Bun or run via bunx). Or
- Rely on bunx to execute .ts files directly (bunx respects shebangs and can run .ts bins). This is NOT portable to Node-only users and may be run only via Bun/bunx. [3][4]
Notes / caveats
- A ts-node/ts-node-script shebang is possible but fragile for global installs (requires ts-node and extra flags; not cross-platform). Prefer compiling for npm global installs. [5]
- Bun can also produce a single-file native executable (bun build --compile) if you want a standalone binary. [4]
Sources:
[1] bunx / bin + shebang docs (Bun).
[2] Best practice: compile TS to JS and use node shebang (npm bin behavior / bundlers like bunchee).
[3] Publish-to-npm + bunx (.ts as bin) discussion.
[4] Bun single-file executable docs.
[5] ts-node shebang/limitations (community discussion).
Compile bin entry to JavaScript with Node shebang for npm compatibility.
The bin entry points to ./bin/openspec.ts with a Bun shebang, which requires Bun to be installed and in PATH. This breaks the standard npm global install workflow—users installing via npm install -g @clanker-guru/openspec without Bun will be unable to run the openspec command.
Additionally, the dist/ directory does not yet exist, so the import in bin/openspec.ts (../dist/cli/index.js) would fail even on Bun installations.
Compile the TypeScript file to dist/bin/openspec.js (or similar) with a Node shebang (#!/usr/bin/env node) and update the bin entry to point to the compiled file instead.
🤖 Prompt for AI Agents
In package.json around lines 29-31, the bin entry points to a TypeScript file
with a Bun shebang which breaks npm installs and the dist directory/imports;
compile the bin TypeScript to a JavaScript file under dist (e.g.,
dist/bin/openspec.js) that starts with a Node shebang (#!/usr/bin/env node),
ensure the compiled file imports the built CLI at dist/cli/index.js (or adjusts
relative paths), add the compiled file to your build step and output directory,
and update package.json's bin entry to point to the compiled
dist/bin/openspec.js so npm global installs work without Bun.
There was a problem hiding this comment.
@ClankerGuru, understood! Thanks for clarifying that this package is intentionally Bun-only. That makes sense given the full migration from Node.js to Bun throughout the PR.
The README already documents Bun as a prerequisite, which is good. If you'd like to make the Bun requirement even more explicit and prevent installation attempts with other package managers, you could optionally add an engines field to package.json:
"engines": {
"bun": ">=1.0.0"
}This would make the Bun requirement machine-readable, though it's not strictly necessary since the shebang and documentation already make it clear.
✏️ Learnings added
Learnt from: ClankerGuru
Repo: ClankerGuru/OpenSpec PR: 1
File: package.json:29-31
Timestamp: 2025-12-28T09:27:28.326Z
Learning: The clanker-guru/openspec package is intentionally designed to run exclusively on the Bun runtime. The bin entry points to a TypeScript file (bin/openspec.ts) with a Bun shebang (#!/usr/bin/env bun), and this is by design, not a compatibility issue.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| "test:postinstall": "bun scripts/postinstall.ts", | ||
| "prepare": "bun run build", | ||
| "prepublishOnly": "bun run build", | ||
| "postinstall": "bun scripts/postinstall.ts", |
There was a problem hiding this comment.
Postinstall script requires Bun runtime.
The postinstall script bun scripts/postinstall.ts requires Bun to be installed on the user's system. If users install this package via npm install without having Bun available, the postinstall will fail. Consider handling this gracefully or documenting Bun as a prerequisite.
Possible mitigation
Consider wrapping the postinstall in a check or using a compiled JS file:
"postinstall": "command -v bun >/dev/null 2>&1 && bun scripts/postinstall.ts || node dist/scripts/postinstall.js || true"Or document that Bun is required for this package.
🤖 Prompt for AI Agents
In package.json at line 51, the postinstall script currently invokes Bun
directly which will fail if Bun is not installed; update the script to first
detect Bun and only run bun scripts/postinstall.ts when available, otherwise
fall back to a compiled Node script (e.g., dist/scripts/postinstall.js) or
no-op, and/or add documentation that Bun is a prerequisite; implement the change
by replacing the postinstall entry with a command that checks for bun and runs
bun when present, otherwise runs Node on a built JS fallback or exits
gracefully, and update README/install docs to mention Bun if you choose the
prerequisite route.
There was a problem hiding this comment.
we dont want npm in this repo
There was a problem hiding this comment.
@ClankerGuru, understood! Since this is a Bun-exclusive package by design, you'll want to ensure the Bun requirement is clearly documented so users know upfront.
I see the README already mentions Bun as a prerequisite and shows bunx as the recommended installation method. You might also consider adding an engines field to package.json to make the requirement explicit:
"engines": {
"bun": ">=1.0.0"
}This will help package managers and tooling understand the runtime requirement.
✏️ Learnings added
Learnt from: ClankerGuru
Repo: ClankerGuru/OpenSpec PR: 1
File: package.json:51-51
Timestamp: 2025-12-28T09:28:29.872Z
Learning: The clanker-guru/openspec package maintainers explicitly do not want npm compatibility or fallbacks. The package is intentionally Bun-exclusive, and npm-based workflows should not be suggested.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: ClankerGuru
Repo: ClankerGuru/OpenSpec PR: 1
File: package.json:29-31
Timestamp: 2025-12-28T09:27:28.326Z
Learning: The clanker-guru/openspec package is intentionally designed to run exclusively on the Bun runtime. The bin entry points to a TypeScript file (bin/openspec.ts) with a Bun shebang (#!/usr/bin/env bun), and this is by design, not a compatibility issue.
| @@ -1,5 +1,5 @@ | |||
| <p align="center"> | |||
| <a href="https://github.com/Fission-AI/OpenSpec"> | |||
| <a href="https://github.com/clanker-guru/OpenSpec"> | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
curl -s "https://registry.npmjs.org/@clanker-guru/openspec" | jq '.name // "Package not found"'Repository: ClankerGuru/OpenSpec
Length of output: 83
🏁 Script executed:
git ls-files | grep -i readmeRepository: ClankerGuru/OpenSpec
Length of output: 189
🏁 Script executed:
cat -n README.md | head -150Repository: ClankerGuru/OpenSpec
Length of output: 8479
Publish package to npm before merge or update installation instructions.
The @clanker-guru/openspec package is not yet published on npm. The README contains references to the npm package at lines 13–14 (npm badge) and lines 138–143 (installation commands via bunx and bun install). These will fail or display incorrectly until the package is published. Either publish the package before merging this PR, or remove the npm references from the README and update the installation instructions post-publish.
🤖 Prompt for AI Agents
In README.md around line 2 (and noting npm badge at lines 13–14 and installation
commands at lines 138–143), the README references the unpublished
@clanker-guru/openspec npm package; either publish the package before merging or
remove/replace those npm-specific references. Fix by removing the npm badge and
any "npm install @clanker-guru/openspec" or "bunx" install lines, then replace
them with valid alternatives (e.g., instructions to install from the GitHub repo
or to clone/build locally) so the README contains working installation steps
until the package is published.
|
@coderabbitai this is a bun only utility , don't expect to see js in this repository :) |
|
Tip For best results, initiate chat on the files or code changes. @ClankerGuru Thank you for the clarification! I completely understand - this aligns with the learnings I have stored about @clanker-guru/openspec being intentionally Bun-exclusive. Since Bun can directly execute TypeScript files without additional configuration, there's no need for compiled JavaScript files in the repository. The migration from Node.js to Bun with full TypeScript conversion makes perfect sense. The eslint.config.ts change removing I've noted this is a pure TypeScript + Bun project, so I won't flag TypeScript-specific patterns or suggest JavaScript/Node.js compatibility concerns in future reviews. 🚀 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/commands/validate.enriched-output.test.ts (1)
29-30: Consider usingfsAPIs for consistency and cross-platform reliability.These lines use shell commands (
mkdir -p,bash -lc) to create test files, while the rest of the test (and other test files) usesfs.mkdirandfs.writeFile. This inconsistency could cause issues on Windows wherebashmay not be available.🔎 Suggested refactor for consistency
- execSync(`mkdir -p ${changePath}`); - execSync(`bash -lc "cat > ${path.join(changePath, 'proposal.md')} <<'EOF'\n${changeContent}\nEOF"`); + await fs.mkdir(changePath, { recursive: true }); + await fs.writeFile(path.join(changePath, 'proposal.md'), changeContent, 'utf-8');Note: The test function would need to be
asyncfor this change.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
test/commands/change.interactive-show.test.tstest/commands/change.interactive-validate.test.tstest/commands/show.test.tstest/commands/spec.interactive-show.test.tstest/commands/spec.interactive-validate.test.tstest/commands/spec.test.tstest/commands/validate.enriched-output.test.tstest/helpers/run-cli.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/commands/spec.interactive-validate.test.ts
- test/commands/change.interactive-show.test.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ClankerGuru
Repo: ClankerGuru/OpenSpec PR: 1
File: package.json:29-31
Timestamp: 2025-12-28T09:27:28.326Z
Learning: The clanker-guru/openspec package is intentionally designed to run exclusively on the Bun runtime. The bin entry points to a TypeScript file (bin/openspec.ts) with a Bun shebang (#!/usr/bin/env bun), and this is by design, not a compatibility issue.
Learnt from: ClankerGuru
Repo: ClankerGuru/OpenSpec PR: 1
File: package.json:51-51
Timestamp: 2025-12-28T09:28:29.872Z
Learning: The clanker-guru/openspec package maintainers explicitly do not want npm compatibility or fallbacks. The package is intentionally Bun-exclusive, and npm-based workflows should not be suggested.
📚 Learning: 2025-12-28T09:27:28.326Z
Learnt from: ClankerGuru
Repo: ClankerGuru/OpenSpec PR: 1
File: package.json:29-31
Timestamp: 2025-12-28T09:27:28.326Z
Learning: The clanker-guru/openspec package is intentionally designed to run exclusively on the Bun runtime. The bin entry points to a TypeScript file (bin/openspec.ts) with a Bun shebang (#!/usr/bin/env bun), and this is by design, not a compatibility issue.
Applied to files:
test/commands/change.interactive-validate.test.tstest/commands/spec.test.tstest/commands/validate.enriched-output.test.tstest/helpers/run-cli.tstest/commands/spec.interactive-show.test.tstest/commands/show.test.ts
📚 Learning: 2025-12-28T09:28:29.872Z
Learnt from: ClankerGuru
Repo: ClankerGuru/OpenSpec PR: 1
File: package.json:51-51
Timestamp: 2025-12-28T09:28:29.872Z
Learning: The clanker-guru/openspec package maintainers explicitly do not want npm compatibility or fallbacks. The package is intentionally Bun-exclusive, and npm-based workflows should not be suggested.
Applied to files:
test/commands/change.interactive-validate.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test (windows-latest)
🔇 Additional comments (10)
test/commands/change.interactive-validate.test.ts (1)
1-46: LGTM! Clean migration to Bun test runner with proper test isolation.The test correctly:
- Uses
bun:testimports and Bun-based CLI execution- Creates isolated temp directories per test run using
os.tmpdir()+randomUUID()- Properly restores
process.envandprocess.cwd()in the finally block- References the correct
bin/openspec.tsentry pointBased on learnings, the Bun-exclusive design is intentional.
test/commands/validate.enriched-output.test.ts (1)
1-51: Migration to Bun test runner looks correct.The Bun-specific changes (imports, temp directory handling, CLI execution) are properly implemented and aligned with the project's Bun-exclusive design.
test/commands/spec.test.ts (2)
1-59: LGTM! Comprehensive test setup with proper Bun migration.The test file correctly:
- Migrates all imports to
bun:test- Uses OS temp directories with UUIDs for test isolation
- Updates bin path to
openspec.ts- Cleans up resources in
afterEach
61-327: All CLI invocations correctly updated to Bun.All 15
execSynccalls throughout the test cases have been properly updated fromnodetobunexecution, maintaining consistent invocation patterns acrossspec show,spec list, andspec validatecommands with various flags.test/commands/spec.interactive-show.test.ts (1)
1-46: LGTM! Clean migration matching the established pattern.The test properly:
- Uses isolated temp directories per test run
- Invokes CLI via Bun
- Restores environment state in the finally block
- References the correct
bin/openspec.tsentry pointtest/commands/show.test.ts (2)
1-34: LGTM! Well-structured test setup with comprehensive directory scaffolding.The test properly sets up both
changesDirandspecsDirto test theshowcommand's auto-detection and ambiguity handling logic. The Bun migration is correctly applied.
36-126: All test cases correctly invoke CLI via Bun.The five test scenarios (no-args hint, change detection, spec detection, ambiguity handling, nearest matches) all use proper Bun-based CLI execution and maintain appropriate environment isolation with try/finally blocks.
test/helpers/run-cli.ts (3)
56-73: LGTM: Build command migrated to Bun.The build orchestration correctly switches from
pnpm run buildtobun run build, consistent with the project's migration to Bun runtime. The caching logic and error handling remain sound.Based on learnings, the package is intentionally Bun-exclusive.
75-146: LGTM: CLI execution migrated to Bun with Windows compatibility.The changes correctly switch CLI execution from Node to Bun:
- Line 82: Spawns
buninstead ofnodeto execute the built CLI- Line 91: Adds Windows shell flag (consistent with
runCommandat line 41) for reliable executable resolution- Line 136: Updates command reporting to reflect actual
buninvocationAll cleanup paths (stream destruction, timeout handling, unref behavior) are preserved.
Based on learnings, the package runs exclusively on Bun runtime.
148-148: LGTM: New export alias for project root.The
cliProjectRootexport provides test files with access to the project root path. This is a minor, non-breaking addition.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/cli-e2e/basic.test.ts (1)
35-35: LGTM! Appropriate workaround for Bun's Windows spawn limitations.The
describe.skipIf(isWindows)correctly addresses the known Bun spawn issues on Windows mentioned in the commit message. Skipping the entire suite is appropriate since all tests depend on CLI spawning viarunCLI.Optional: Add explanatory comment for maintainability
Consider adding a brief comment to document why tests are skipped on Windows:
const isWindows = process.platform === 'win32'; +// Skip e2e tests on Windows due to known Bun spawn limitations describe.skipIf(isWindows)('openspec CLI e2e basics', () => {This helps future maintainers understand the reasoning without needing to search commit history.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/cli-e2e/basic.test.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ClankerGuru
Repo: ClankerGuru/OpenSpec PR: 1
File: package.json:29-31
Timestamp: 2025-12-28T09:27:28.326Z
Learning: The clanker-guru/openspec package is intentionally designed to run exclusively on the Bun runtime. The bin entry points to a TypeScript file (bin/openspec.ts) with a Bun shebang (#!/usr/bin/env bun), and this is by design, not a compatibility issue.
Learnt from: ClankerGuru
Repo: ClankerGuru/OpenSpec PR: 1
File: package.json:51-51
Timestamp: 2025-12-28T09:28:29.872Z
Learning: The clanker-guru/openspec package maintainers explicitly do not want npm compatibility or fallbacks. The package is intentionally Bun-exclusive, and npm-based workflows should not be suggested.
Learnt from: ClankerGuru
Repo: ClankerGuru/OpenSpec PR: 1
File: .github/workflows/ci.yml:28-30
Timestamp: 2025-12-28T09:37:00.257Z
Learning: In the clanker-guru/openspec project CI workflows, the maintainers prefer using `bun-version: latest` rather than pinning to a specific Bun version, accepting the trade-off for staying current with the latest Bun releases.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test (windows-latest)
🔇 Additional comments (2)
test/cli-e2e/basic.test.ts (2)
1-1: LGTM! Correct migration to Bun's test runner.The import statement correctly migrates from Vitest to Bun's native test runner, aligning with the PR's runtime migration objectives.
8-9: LGTM! Standard Windows platform detection.The platform check is correctly implemented using the standard
process.platform === 'win32'approach.
Summary
Summary by CodeRabbit
New Features
Documentation
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.