fix(opencode): handle Windows project basenames#656
Conversation
|
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 OpenCode Engram plugin now derives project names with a separator-agnostic basename helper. Tests cover Windows paths in compaction recovery and migration, plus the existing POSIX fallback. ChangesOpenCode project naming
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
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.
Actionable comments posted: 2
🤖 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 `@internal/setup/plugins/opencode/engram.ts`:
- Around line 156-159: Update basename in
internal/setup/plugins/opencode/engram.ts at lines 156-159 to filter empty path
components before selecting the final segment, preserving "unknown" for empty
paths and returning the actual final directory for trailing separators. Apply
the identical change to basename in plugin/opencode/engram.ts at lines 156-159
to keep both implementations synchronized.
In `@plugin/opencode/engram.test.ts`:
- Around line 37-48: Add a test covering the Git-root fallback in the compaction
recovery flow, alongside the existing Windows basename test. Configure the Git
command mock so remote get-url fails while rev-parse --show-toplevel returns a
Windows path, then assert the generated context uses the basename as the project
name rather than the full path.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9cc683be-aaf9-41af-8791-a0b65211c58e
📒 Files selected for processing (3)
internal/setup/plugins/opencode/engram.tsplugin/opencode/engram.test.tsplugin/opencode/engram.ts
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 `@internal/setup/plugins/opencode/engram.ts`:
- Around line 156-159: Update basename in
internal/setup/plugins/opencode/engram.ts (lines 156-159) and
plugin/opencode/engram.ts (lines 156-159) to detect Windows drive-root paths
such as C:\ and C:/ and return the existing unknown fallback instead of the
drive label; add regression coverage for both root forms and ensure both
implementations behave identically.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 25b80757-60da-4ea8-abf6-265203e28bd3
📒 Files selected for processing (3)
internal/setup/plugins/opencode/engram.tsplugin/opencode/engram.test.tsplugin/opencode/engram.ts
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)
plugin/opencode/engram.test.ts (1)
11-31: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRestore mocked globals after each test.
createPluginoverwritesglobalThis.BunandglobalThis.fetchwithout cleanup, so later tests can observe these mocks and become order-dependent. Return a cleanup callback or restore both globals in anafterEach/finallypath.🤖 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 `@plugin/opencode/engram.test.ts` around lines 11 - 31, Update createPlugin to capture the original globalThis.Bun and globalThis.fetch values before installing mocks, and provide cleanup that restores both globals after each test; ensure all tests invoke this cleanup through afterEach or a finally path so mocked state cannot leak between tests.
🤖 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 `@internal/setup/plugins/opencode/engram.ts`:
- Around line 204-205: The migration key extraction in both plugin copies must
handle Windows paths. In internal/setup/plugins/opencode/engram.ts and
plugin/opencode/engram.ts, update oldProject to use basename(ctx.directory);
adjust the regression expectation in plugin/opencode/engram.test.ts so
old_project is "Blackie".
---
Outside diff comments:
In `@plugin/opencode/engram.test.ts`:
- Around line 11-31: Update createPlugin to capture the original globalThis.Bun
and globalThis.fetch values before installing mocks, and provide cleanup that
restores both globals after each test; ensure all tests invoke this cleanup
through afterEach or a finally path so mocked state cannot leak between tests.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5974931a-2e25-4a4e-8d36-ddfff73cd410
📒 Files selected for processing (3)
internal/setup/plugins/opencode/engram.tsplugin/opencode/engram.test.tsplugin/opencode/engram.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/setup/plugins/opencode/engram.ts (1)
156-158: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle Windows drive roots consistently in both copies and tests.
The helper returns
"C:"forC:\\andC:/, producing an invalid project name when OpenCode runs from a drive root.
internal/setup/plugins/opencode/engram.ts#L156-L158: return"unknown"when the only remaining segment matches/^[A-Za-z]:$/.plugin/opencode/engram.ts#L156-L158: apply the identical guard in the embedded copy.plugin/opencode/engram.test.ts#L50-L60: add regression assertions for bothC:\\andC:/.🤖 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 `@internal/setup/plugins/opencode/engram.ts` around lines 156 - 158, Update basename in internal/setup/plugins/opencode/engram.ts:156-158 and plugin/opencode/engram.ts:156-158 to return "unknown" when the extracted segment is a Windows drive root matching /^[A-Za-z]:$/. Add regression assertions in plugin/opencode/engram.test.ts:50-60 covering both C:\ and C:/ inputs.
🤖 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.
Duplicate comments:
In `@internal/setup/plugins/opencode/engram.ts`:
- Around line 156-158: Update basename in
internal/setup/plugins/opencode/engram.ts:156-158 and
plugin/opencode/engram.ts:156-158 to return "unknown" when the extracted segment
is a Windows drive root matching /^[A-Za-z]:$/. Add regression assertions in
plugin/opencode/engram.test.ts:50-60 covering both C:\ and C:/ inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6489feb6-1d9e-4aed-ace8-c951a7cf8621
📒 Files selected for processing (3)
internal/setup/plugins/opencode/engram.tsplugin/opencode/engram.test.tsplugin/opencode/engram.ts
1 participant
Linked Issue
Closes #652
PR Type
type:bug- Bug fixtype:feature- New featuretype:docs- Documentation onlytype:refactor- Code refactoring (no behavior change)type:chore- Maintenance, dependencies, toolingtype:breaking-change- Breaking changeSummary
Changes
plugin/opencode/engram.tsplugin/opencode/engram.test.tsinternal/setup/plugins/opencode/engram.tsTest Plan
go test ./...go test -tags e2e ./internal/server/...Additional verification:
node --test plugin/opencode/engram.test.ts(3 passed)node --check plugin/opencode/engram.tsnode --check internal/setup/plugins/opencode/engram.tsgo test ./internal/setup -run OpenCode -count=1go test ./...was attempted on Windows. It still hits unrelated existing Windows-specific failures and acmd/engramtimeout; the affected OpenCode-focused tests pass, and Linux CI remains authoritative for the full suite.Automated Checks
Closes #N/Fixes #N/Resolves #Nstatus:approvedlabeltype:*labelgo test ./...passesgo test -tags e2e ./internal/server/...passesContributor Checklist
Closes #N)go test ./...go test -tags e2e ./internal/server/...Co-Authored-Bytrailers in commitsNotes for Reviewers
The bounded reliability review approved this candidate with two non-blocking warnings: trailing directory separators are not normalized, and the TypeScript regression file is not yet registered in repository CI. Both are intentionally left as follow-up scope rather than changing the frozen candidate after approval.
Summary by CodeRabbit
/and\.old_project/projectvalues and correct/projects/migraterequest behavior (including skipping migration when unchanged).experimental.session.compacting, validating request payloads and triggering conditions across Windows/POSIX paths and edge cases (e.g., trailing separators, nested directories, and Git-root-derived basenames).