fix(opencode): remove invalid top-level theme config#1287
Conversation
OpenCode's opencode.json schema is strict and rejects an unrecognized
top-level "theme" key (ConfigInvalidError), which breaks runtime config
updates such as profile activation. The theme component wrote
{"theme": "gentleman-kanagawa"} into opencode.json for OpenCode.
Gate theme injection behind a new optional ThemeInjectionController
adapter capability (mirroring EffectiveCodeGraphWiringDetector). The
OpenCode adapter opts out, and a single shared helper drives both
theme injection and the post-apply verification path, so OpenCode
neither gets the key written nor requires the file to exist. Every
other agent is unaffected.
Refs Gentleman-Programming#497
|
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:
📝 WalkthroughWalkthroughOpenCode now opts out of top-level theme injection through an adapter capability. Existing theme keys can be migrated out, verification paths account for optional settings files, tests validate no OpenCode theme key is written, and documentation reflects the schema constraint. ChangesOpenCode theme support
Estimated code review effort: 3 (Moderate) | ~25 minutes 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 |
Alan-TheGentleman
left a comment
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
The prevention path is solid, but I'm requesting changes because existing OpenCode installations can retain the invalid top-level theme key. Inject now returns before reading the settings file, and post-apply verification skips that file, so an install can report success while profile activation remains broken. Please migrate existing configurations by removing only the stale top-level theme key, preserve every other setting, add a regression test that starts from an existing opencode.json, and add the required type:* label. Push the update and I'll re-review.
Remove only stale top-level theme keys from existing OpenCode settings while preserving unrelated content, permissions, and malformed-file safety. Keep fresh installs as no-ops and cover migration and verification paths with regression tests.
![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/theme/inject.go (1)
42-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAccumulate the migration result instead of returning early.
If a future adapter supports theme injection AND implements
ThemeSettingsMigrator, returning early here will skip the actual injection phase during the first run. This breaks idempotency (requiring two runs to reach the final state) and contradicts the intent of running migration before the current injection behavior is applied.Consider accumulating the
changedstate and merging it with the injection result to ensure both operations can complete in a single pass.♻️ Proposed refactor
func Inject(homeDir string, adapter agents.Adapter) (InjectionResult, error) { - if migrator, ok := adapter.(agents.ThemeSettingsMigrator); ok { - path, changed, err := migrator.MigrateThemeSettings(homeDir) - if err != nil { - return InjectionResult{}, err - } - if changed { - return InjectionResult{Changed: true, Files: []string{path}}, nil - } - } - if !agents.SupportsThemeInjection(adapter) { - return InjectionResult{}, nil - } + var migrated bool + var migratedPath string + if migrator, ok := adapter.(agents.ThemeSettingsMigrator); ok { + var err error + migratedPath, migrated, err = migrator.MigrateThemeSettings(homeDir) + if err != nil { + return InjectionResult{}, err + } + } + if !agents.SupportsThemeInjection(adapter) { + if migrated { + return InjectionResult{Changed: true, Files: []string{migratedPath}}, nil + } + return InjectionResult{}, nil + } settingsPath := adapter.SettingsPath(homeDir) if settingsPath == "" { - return InjectionResult{}, nil + if migrated { + return InjectionResult{Changed: true, Files: []string{migratedPath}}, nil + } + return InjectionResult{}, nil } writeResult, err := mergeJSONFile(settingsPath, themeOverlayJSON) if err != nil { return InjectionResult{}, err } - return InjectionResult{Changed: writeResult.Changed, Files: []string{settingsPath}}, nil + return InjectionResult{Changed: writeResult.Changed || migrated, Files: []string{settingsPath}}, nil }🤖 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/theme/inject.go` around lines 42 - 66, Update Inject to accumulate the migrator’s changed state and migrated file path instead of returning immediately when MigrateThemeSettings reports changed. Continue through SupportsThemeInjection, SettingsPath, and mergeJSONFile so both migration and injection run in one pass, then combine their Changed values and include both affected paths in the returned InjectionResult without duplicating paths.
🤖 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/theme/inject.go`:
- Around line 42-66: Update Inject to accumulate the migrator’s changed state
and migrated file path instead of returning immediately when
MigrateThemeSettings reports changed. Continue through SupportsThemeInjection,
SettingsPath, and mergeJSONFile so both migration and injection run in one pass,
then combine their Changed values and include both affected paths in the
returned InjectionResult without duplicating paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b76ea49f-4aa5-49b0-b67a-59a593c110b0
📒 Files selected for processing (7)
internal/agents/interface.gointernal/agents/opencode/adapter.gointernal/cli/run.gointernal/cli/run_component_paths_test.gointernal/components/filemerge/json_remove.gointernal/components/theme/inject.gointernal/components/theme/inject_test.go
Alan-TheGentleman
left a comment
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
The migration now addresses the original failure, but I'm keeping changes requested because the remover still does not preserve unrelated content or fail closed for every malformed file. When theme is the first top-level member, the removal range also deletes preceding JSONC comments; an unterminated trailing block comment can also be normalized away before validation, allowing a malformed file to be rewritten instead of left untouched. Please add regression tests for both cases and adjust the parser and removal boundaries so unrelated comments are preserved and every malformed input returns without mutation. The required type:* label is also still missing, and its policy check is failing. Push the update and I'll re-review.
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/filemerge/json_remove.go`:
- Around line 150-151: The three callers of skipJSONComment—skipJSONTrivia,
scanJSONValueDelimiter, and removeTopLevelJSONKeyOnce—must handle an unchanged
index when an isolated slash is encountered. Add an advancement check at each
site: return or otherwise terminate the current scan when skipJSONComment
returns its input index, preserving normal comment-skipping behavior when the
index advances. Apply this in internal/components/filemerge/json_remove.go at
lines 150-151, 128-129, and 69-85.
🪄 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: f7e5cdb1-0b71-4430-9015-2df302ca3281
📒 Files selected for processing (10)
docs/components.mde2e/e2e_test.shinternal/agents/interface.gointernal/agents/opencode/adapter.gointernal/catalog/components.gointernal/cli/run.gointernal/components/filemerge/json_remove.gointernal/components/theme/inject_test.gointernal/tui/testdata/preset-custom-no-opencode-next.goldeninternal/tui/testdata/preset-custom-opencode-next.golden
|
Please make the permission-preservation contract portable on Windows, or scope the POSIX mode assertion to platforms where those bits are meaningful. This test is candidate-only and does not exist in frozen base tree Validated patchThe minimal test-only fix is to keep the permission assertion on platforms where POSIX mode bits are meaningful: diff --git a/internal/components/theme/inject_test.go b/internal/components/theme/inject_test.go
index 1e47f1d..84851fe 100644
--- a/internal/components/theme/inject_test.go
+++ b/internal/components/theme/inject_test.go
@@ -4,6 +4,7 @@ import (
"encoding/json"
"os"
"path/filepath"
+ "runtime"
"testing"
"github.com/gentleman-programming/gentle-ai/internal/agents"
@@ -145,8 +146,10 @@ func TestInjectRemovesLegacyOpenCodeThemeOnly(t *testing.T) {
if err != nil {
t.Fatalf("Stat(settings) error = %v", err)
}
- if got := info.Mode().Perm(); got != 0o600 {
- t.Fatalf("settings mode = %o, want preserved 600", got)
+ if runtime.GOOS != "windows" {
+ if got := info.Mode().Perm(); got != 0o600 {
+ t.Fatalf("settings mode = %o, want preserved 600", got)
+ }
}
second, err := Inject(home, opencodeAdapter())Validated against commit |
…e-ai into fix/opencode-theme-config
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 `@e2e/e2e_test.sh`:
- Around line 1068-1081: The Node-unavailable simulation contains an unused
python3 override. Remove the `python3()` stub from the test block around
`jsonc_has_no_root_key`, while preserving the `command()` override because
`jsonc_has_no_root_key` relies on it indirectly through `command -v node`.
In `@internal/components/theme/inject.go`:
- Around line 63-70: Update the mergeJSONFile error path in MigrateThemeSettings
to return the accumulated result alongside the error instead of a fresh
InjectionResult{}. Preserve any Changed flag and Files entries recorded by the
preceding migration while continuing to propagate the merge failure.
🪄 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: 367c6ce3-d55d-4a59-99c5-aaf85e5665eb
📒 Files selected for processing (6)
e2e/e2e_test.she2e/lib.shinternal/cli/run.gointernal/cli/run_component_paths_test.gointernal/components/theme/inject.gointernal/components/theme/inject_test.go
|
@coderabbitai review Updated in
Fresh checks pass for Go Format, Unit Tests, Windows Runtime, and E2E on Ubuntu, Arch, and Fedora. |
|
✅ Action performedReview finished.
|
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)
e2e/e2e_test.sh (1)
1532-1548: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
assert_jsonc_has_no_root_keyfor consistency with the schema-aware check.Lines 1539 and 1545 use
assert_file_not_contains ... '"theme"', which forbids the substring anywhere in the file — stricter than the root-only semantics this PR establishes viaassert_jsonc_has_no_root_keyelsewhere (test_oc_theme_injection,test_full_preset_opencode,test_edge_multiple_json_overlays). Currently harmless since this test only installs--component theme, but it would false-fail if this test is ever extended to a component that legitimately writes a nestedthemekey.♻️ Proposed fix
if $BINARY install --agent opencode --component theme --persona neutral 2>&1; then - assert_file_not_contains "$settings" '"theme"' "No theme key after first run" + assert_jsonc_has_no_root_key "$settings" "theme" "No root theme key after first run" else log_fail "First OpenCode theme install exited non-zero (should be a clean no-op)" fi if $BINARY install --agent opencode --component theme --persona neutral 2>&1; then - assert_file_not_contains "$settings" '"theme"' "No theme key after second run" + assert_jsonc_has_no_root_key "$settings" "theme" "No root theme key after second run" else log_fail "Second OpenCode theme install exited non-zero (should be a clean no-op)" fi🤖 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 `@e2e/e2e_test.sh` around lines 1532 - 1548, Replace both assert_file_not_contains checks in test_idempotent_theme_opencode with the schema-aware assert_jsonc_has_no_root_key helper, passing the settings file and theme key. Preserve the existing success/failure flow and messages while validating only the root-level theme property.
🤖 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 `@e2e/e2e_test.sh`:
- Around line 1532-1548: Replace both assert_file_not_contains checks in
test_idempotent_theme_opencode with the schema-aware
assert_jsonc_has_no_root_key helper, passing the settings file and theme key.
Preserve the existing success/failure flow and messages while validating only
the root-level theme property.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: eb6bd6f8-66d2-4ad5-bc66-a2259e0ca0b0
📒 Files selected for processing (6)
e2e/e2e_test.she2e/lib.shinternal/cli/run.gointernal/cli/run_component_paths_test.gointernal/components/theme/inject.gointernal/components/theme/inject_test.go
|
@Alan-TheGentleman PR #1287 is ready for re-review at All reviewer and CodeRabbit follow-ups are addressed. Fresh checks pass for Go Format, Unit Tests, Windows Runtime, and E2E on Ubuntu, Arch, and Fedora. Please apply |
Alan-TheGentleman
added
type:bug
3 participants
🔗 Linked Issue
Closes #497
🏷️ 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📝 Summary
themekey into OpenCode'sopencode.json.themeentries while preserving nested settings, file permissions, and covered JSONC comments and formatting.📂 Changes
themeentries🧪 Test Plan
Focused tests
go test ./internal/components/filemerge/... ./internal/components/theme/... -count=1Full unit suite
go test ./... -count=1Docker E2E
Results:
filemergeandthemetests pass locally🤖 Automated Checks
size:exceptionCloses #497status:approvedtype:*Labeltype:bug✅ Contributor Checklist
status:approvedsize:exceptionwith the rationale documented belowtype:*label to this PRgo test ./...)cd e2e && ./docker-test.sh)Co-Authored-Bytrailers💬 Notes for Reviewers
Suggested review path
Review-size rationale
Requesting
size:exceptionfor one cohesive migration and safety boundary.A chained PR was considered. It would separate legacy migration from JSONC safety and the regression proof required to review it confidently, leaving incomplete intermediate states.
Please apply the maintainer-managed
size:exceptionlabel.Label permissions
I have read-only permission on the upstream repository and cannot apply PR labels.
Please add:
type:bugsize:exceptionPreservation guarantees
thememember.thememembers.Intentionally out of scope
themekeytui.jsonjson_merge.gobehaviorReview follow-up
919617eaddressed the isolated-slash scanner finding, and CodeRabbit marked the thread as resolved.2e04de2made the Windows permission assertion platform-aware, accumulated migration and injection results, deduplicated affected paths, and added root-only OpenCode theme validation.7f1f963removed the dead Python test stub and preserves accumulated migration results when later theme injection fails.52aa4d2aligns the idempotent OpenCode theme checks with root-only JSONC semantics.Summary by CodeRabbit
Bug Fixes
themekey inopencode.json.themekey while preserving the rest of the file (including JSONC formatting).Documentation
Tests
themekey.