fix(opencode): use relative SDD prompt paths#1252
Conversation
|
Maintainers, please add the |
📝 WalkthroughWalkthroughSDD profile overlays and OpenCode prompt injection now generate settings-relative ChangesPortable prompt reference generation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Inject
participant GenerateProfileOverlay
participant SharedPromptFileRef
participant OpenCodeSettings
Inject->>GenerateProfileOverlay: pass settingsPath
GenerateProfileOverlay->>SharedPromptFileRef: build phase reference
SharedPromptFileRef-->>GenerateProfileOverlay: return relative {file:...}
GenerateProfileOverlay->>OpenCodeSettings: write overlay
Inject->>SharedPromptFileRef: build inlined prompt reference
SharedPromptFileRef-->>Inject: return relative {file:...}
Suggested labels: 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 |
ff49243 to
2242051
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/components/sdd/profiles_test.go (1)
778-797: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert portability independently of the production helper.
The test computes
expectedRefwith the sameSharedPromptFileRefused byGenerateProfileOverlay, so a regression that reintroduces absolute home paths can make both sides agree. Also assert that generated references are relative and contain nohomepath.🤖 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/components/sdd/profiles_test.go` around lines 778 - 797, Update the test around GenerateProfileOverlay and its phase loop to validate portability independently of SharedPromptFileRef: assert each generated prompt reference is relative and does not contain the home path before or alongside comparing expectedRef. Keep the existing expected-reference comparison, but ensure the assertions would fail if GenerateProfileOverlay emits an absolute home-based path.
🤖 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.
Outside diff comments:
In `@internal/components/sdd/profiles_test.go`:
- Around line 778-797: Update the test around GenerateProfileOverlay and its
phase loop to validate portability independently of SharedPromptFileRef: assert
each generated prompt reference is relative and does not contain the home path
before or alongside comparing expectedRef. Keep the existing expected-reference
comparison, but ensure the assertions would fail if GenerateProfileOverlay emits
an absolute home-based path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 917aaee2-f391-4db7-843a-71f771119398
⛔ Files ignored due to path filters (1)
testdata/golden/sdd-opencode-multi-settings.goldenis excluded by!testdata/**
📒 Files selected for processing (9)
internal/cli/sync_test.gointernal/components/golden_test.gointernal/components/sdd/inject.gointernal/components/sdd/inject_test.gointernal/components/sdd/profiles.gointernal/components/sdd/profiles_lifecycle_test.gointernal/components/sdd/profiles_test.gointernal/components/sdd/prompts.gointernal/components/sdd/prompts_test.go
dnlrsls
left a comment
dnlrsls
left a comment
There was a problem hiding this comment.
SharedPromptFileRef is still not portable for OpenCode's supported XDG layout. SettingsPath can be $XDG_CONFIG_HOME/opencode/opencode.json, while SharedPromptDir remains $HOME/.config/opencode/prompts/sdd. On Unix this can generate a relative path containing the original home/user topology, and on Windows different config and home volumes make filepath.Rel fail and abort injection.
Please derive settings and shared prompt locations from one resolved config-root model, or otherwise handle roots that cannot produce a portable relative reference. A focused test for absolute XDG_CONFIG_HOME plus Windows different-volume behavior would establish the contract. The standard OpenCode and Kilocode layouts otherwise look correct.
2242051 to
6ddf8d1
Compare
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/components/sdd/inject_test.go`:
- Around line 2388-2391: Update the expected prompt value in the test assertion
to use the existing SharedPromptFileRef symbol instead of a hardcoded path
string, preserving the comparison and failure message so path separators remain
platform-independent.
🪄 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: 9b13106d-814f-4a34-bdfa-d97f97eee299
⛔ Files ignored due to path filters (1)
testdata/golden/sdd-opencode-multi-settings.goldenis excluded by!testdata/**
📒 Files selected for processing (10)
internal/cli/sync_test.gointernal/components/golden_test.gointernal/components/sdd/inject.gointernal/components/sdd/inject_test.gointernal/components/sdd/issue557_test.gointernal/components/sdd/profiles.gointernal/components/sdd/profiles_lifecycle_test.gointernal/components/sdd/profiles_test.gointernal/components/sdd/prompts.gointernal/components/sdd/prompts_test.go
| want := "{file:../opencode/prompts/sdd/sdd-apply.md}" | ||
| if prompt != want { | ||
| t.Fatalf("sdd-apply prompt = %q, want %q", prompt, want) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use SharedPromptFileRef instead of a hardcoded string.
Hardcoding the expected path with forward slashes may cause the test to fail on Windows if the injected prompt uses OS-specific separators. Using SharedPromptFileRef ensures cross-platform consistency and aligns with the approach taken in TestInjectOpenCodeSubagentPromptsStayExecutorScoped.
♻️ Proposed refactor
- want := "{file:../opencode/prompts/sdd/sdd-apply.md}"
+ want, err := SharedPromptFileRef(adapter.SettingsPath(home), home, "sdd-apply")
+ if err != nil {
+ t.Fatalf("SharedPromptFileRef() error = %v", err)
+ }
if prompt != want {📝 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.
| want := "{file:../opencode/prompts/sdd/sdd-apply.md}" | |
| if prompt != want { | |
| t.Fatalf("sdd-apply prompt = %q, want %q", prompt, want) | |
| } | |
| want, err := SharedPromptFileRef(adapter.SettingsPath(home), home, "sdd-apply") | |
| if err != nil { | |
| t.Fatalf("SharedPromptFileRef() error = %v", err) | |
| } | |
| if prompt != want { | |
| t.Fatalf("sdd-apply prompt = %q, want %q", prompt, want) | |
| } |
🤖 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/components/sdd/inject_test.go` around lines 2388 - 2391, Update the
expected prompt value in the test assertion to use the existing
SharedPromptFileRef symbol instead of a hardcoded path string, preserving the
comparison and failure message so path separators remain platform-independent.
|
Updated in The full Go suite, vet, Windows runtime, and E2E checks pass. This is ready for re-review. The only remaining required check is the maintainer-applied |
2 participants
Linked Issue
Closes #723
PR Type
type:bug- Bug fix (non-breaking change that fixes an issue)type:feature- New feature (non-breaking change that adds functionality)type:docs- Documentation onlytype:refactor- Code refactoring (no functional changes)type:chore- Build, CI, or tooling changestype:breaking-change- Breaking change (fix or feature that changes existing behavior)Summary
Changes
internal/components/sddinternal/components/golden_test.gotestdata/golden/sdd-opencode-multi-settings.goldenTest Plan
Passed:
go test ./internal/components/sdd/...go test ./internal/components -run '^TestGoldenSDD_OpenCode_Multi$'go test ./internal/componentsgit diff --checkRuntime validation: N/A. The affected output is deterministic generated JSON and is covered by focused unit and golden tests; there is no separate runtime boundary.
Automated Checks
The repository checks will run on this PR.
Contributor Checklist
status:approvedtype:*label is appliedCo-Authored-BytrailersNotes for Reviewers
Size: 9 files, +128/-50 (178 changed lines), under the 400-line review budget.
Summary by CodeRabbit
Bug Fixes
{file:...}paths in generatedopencode.jsonoverlays and inline prompts.homeand absolute temp-directory paths from being embedded, improving reliability across OpenCode and Kilocode settings layouts (including XDG config home resolution).Tests