fix(opencode): allow Windows reviewer inspection - #2109
Conversation
📝 WalkthroughWalkthroughThe change updates OpenCode default installation and settings merging. It preserves unchanged bytes, orders reviewer permissions, supports PowerShell patterns, and improves sync snapshot and rollback handling. ChangesOpenCode reviewer settings
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Sync
participant Inject
participant OpenCodeSettings
participant ReviewerPermissions
Sync->>Inject: run sync injection steps
Inject->>OpenCodeSettings: apply defaults and merge overlay
Inject->>ReviewerPermissions: rewrite managed permissions
ReviewerPermissions-->>OpenCodeSettings: ordered deny and allow rules
Sync-->>Sync: compare managed snapshots and restore rollback state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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.go`:
- Around line 1823-1834: Update isOpenCodeReviewerPermission to accept a
precomputed pattern list and use it for both the expected-length check and
validation loop. In orderOpenCodeReviewerPermissions, call
reviewerInspectionPermissionPatterns once before iterating candidate objects and
pass the resulting slice to each isOpenCodeReviewerPermission invocation.
- Around line 1767-1790: Add validation in orderOpenCodeReviewerPermissions for
both failure modes: return an error when no managed OpenCode reviewer permission
match is found, and verify the extracted indent contains only whitespace before
rewriting. Track whether isOpenCodeReviewerPermission matched and validate the
indent derived from lineStart; preserve successful rewriting while preventing
compact or unexpectedly formatted input from being silently accepted or
corrupted. Add a test that confirms the managed reviewer object is actually
reordered and fails when the expected match is absent.
- Around line 376-384: Update the pre-run snapshot setup surrounding
defaultPlan.Apply to include the OpenCode ownership sidecar path alongside
settingsPath, ensuring Snapshotter.Create captures both opencode.json and
.gentle-ai-default-agent.json before changes. Reuse the existing
opencodedefault.OwnershipPath(settingsPath) symbol so rollback can remove newly
created sidecars and restore overwritten ones.
In `@internal/components/sdd/review_ledger_contract_test.go`:
- Around line 425-453: The permission assertions currently inspect the
production generator instead of the merged settings. In
internal/components/sdd/review_ledger_contract_test.go lines 425-453, update
openCodeBashPermissionAction to accept the merged opencode.json bytes and
evaluate rules in their serialized order; in lines 194-200, assert
deny-before-allow using raw’s serialized content rather than
orderedOpenCodeReviewerBash output.
- Around line 194-200: Update the ordering assertion in
assertOpenCodeReviewerPermission to inspect the serialized raw value being
validated, rather than calling orderedOpenCodeReviewerBash(""). Ensure the
deny-before-allow check uses the ordered bytes from raw (or receives the file
bytes directly if raw is decoded), so each label’s installed permission object
is actually verified.
🪄 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: e9a05336-7566-4cb4-b0a0-f372293f9ed7
📒 Files selected for processing (4)
internal/components/opencodedefault/ownership.gointernal/components/sdd/boundedreview.gointernal/components/sdd/inject.gointernal/components/sdd/review_ledger_contract_test.go
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/cli/sync_test.go`:
- Around line 4517-4547: Extend
TestSyncRollbackRestoresOpenCodeDefaultOwnershipSidecar to cover an existing
sidecar: create the ownership file with known bytes before snapshotSyncFiles,
overwrite it after snapshotting, then call restoreSyncFiles and assert the file
still exists with its original bytes. Preserve the current newly-created-sidecar
removal coverage, using a separate test or clearly isolated case.
🪄 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: bc8df858-2d64-434b-a96f-1825db80b0d3
⛔ Files ignored due to path filters (1)
testdata/golden/sdd-opencode-multi-settings.goldenis excluded by!testdata/**
📒 Files selected for processing (4)
internal/cli/sync.gointernal/cli/sync_test.gointernal/components/sdd/inject.gointernal/components/sdd/review_ledger_contract_test.go
|
Ready for maintainer merge. All CI, runtime, E2E, cognitive-load, approval, and CodeRabbit checks pass. Please merge #2109 first; #2110 is the green follow-up and should merge second. @Alan-TheGentleman @decode2, could one of you take the merge? Thanks. |
decode2
left a comment
decode2
left a comment
There was a problem hiding this comment.
LGTM! Excellent handling of JSON key sorting for PowerShell call-operator permissions in OpenCode.
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/cli/sync.go (1)
1363-1363: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRetain the rollback snapshot until sync success is final.
Line 1363 calls
cleanupRollbackSnapshot()before checkingresult.Execution.Err. It also runs beforechangedSyncFiles, post-sync verification, and the final state write can return errors. If rollback fails or a later step fails, this function returns an error after deleting the backup needed to restore user configuration.Keep the snapshot until all post-apply checks and state writes succeed. Retain it when rollback is unsuccessful, or invoke the existing pipeline restore step before cleanup. Add a regression test for an unsuccessful rollback.
As per path instructions, user-configuration writes require a backup/restore path.
Based on learnings, SDD restore paths must retain the pipeline snapshot until rollback completes and use the pipeline rollback restore mechanism.Suggested cleanup placement
result.Execution = orchestrator.Execute(stagePlan) - rt.state.cleanupRollbackSnapshot() if result.Execution.Err != nil { + if result.Rollback.Success { + rt.state.cleanupRollbackSnapshot() + } return result, fmt.Errorf("execute sync pipeline: %w", result.Execution.Err) } // Keep the snapshot for later error handling. ... + rt.state.cleanupRollbackSnapshot() return result, 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/cli/sync.go` at line 1363, Move cleanupRollbackSnapshot out of the pre-validation path so the rollback snapshot remains available through result.Execution.Err checks, changedSyncFiles, post-sync verification, and the final state write. Clean it up only after every post-apply step succeeds, and retain or restore it through the existing pipeline rollback mechanism when rollback or any later step fails. Add a regression test covering an unsuccessful rollback and verifying the snapshot remains restorable.Sources: Path instructions, Learnings
🤖 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/cli/sync.go`:
- Line 1363: Move cleanupRollbackSnapshot out of the pre-validation path so the
rollback snapshot remains available through result.Execution.Err checks,
changedSyncFiles, post-sync verification, and the final state write. Clean it up
only after every post-apply step succeeds, and retain or restore it through the
existing pipeline rollback mechanism when rollback or any later step fails. Add
a regression test covering an unsuccessful rollback and verifying the snapshot
remains restorable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 041d9ff6-db4b-4873-aecd-a15a8834d5ff
⛔ Files ignored due to path filters (1)
testdata/golden/sdd-opencode-multi-settings.goldenis excluded by!testdata/**
📒 Files selected for processing (4)
internal/cli/sync.gointernal/cli/sync_test.gointernal/components/sdd/boundedreview.gointernal/components/sdd/review_ledger_contract_test.go
|
Follow-up on CodeRabbit's latest outside-diff note: the cleanupRollbackSnapshot() ordering at internal/cli/sync.go is unchanged from the current base branch and was introduced on main in 2d2159d; PR #2109 only changes changedSyncFiles in that file. I did not expand this PR's scope, which now sits exactly at the enforced 400-line review budget. The rollback-sidecar findings introduced by this PR are covered and resolved; the cleanup-order observation should be tracked separately if maintainers want to change the base behavior. |
|
I am taking over the bounded Windows/OpenCode corrections now while preserving contributor history. @dnlrsls, please pause pushes to this branch until I complete the bounded takeover. |
|
Closing this because #2221 merged the approved fix for #2076. The wildcard shell-permission direction here cannot securely bind model-controlled command values to an OpenCode child session, so extending it would preserve an injection boundary rather than fix it. The merged successor fails closed before reviewer launch, keeps Kilo unchanged, and is proven by the real Windows OpenCode 1.18.10 runtime. I preserved this PR and its discussion as the contributor record for the original approach. |
3 participants
Linked Issue Closes #2076 --- ## PR Type - [x]
type:bug- Bug fix - [ ]type:feature- New feature - [ ]type:docs- Documentation only - [ ]type:refactor- Code refactoring - [ ]type:chore- Build, CI, or tooling - [ ]type:breaking-change- Breaking change --- ## Summary - Allow provider-bound reviewer inspection commands when PowerShell renders native execution with the&call operator. - Preserve deny-by-default command admission and keep user-authored OpenCode permission ordering unchanged. - Keep repeated OpenCode installation byte-idempotent. --- ## Changes | File / Area | What Changed | |-------------|--------------| |internal/components/sdd/boundedreview.go| Generate bare and PowerShell inspection permission patterns. | |internal/components/sdd/inject.go| Order only fully managed reviewer rules and preserve semantic no-op bytes. | |internal/components/opencodedefault/ownership.go| Avoid rewriting an already-correct default agent. | |internal/components/sdd/review_ledger_contract_test.go| Cover permission admission, serialization, idempotency, and user-rule boundaries. | --- ## Test Plan Changed-scope unit testsbash go test ./internal/components/sdd ./internal/components/opencodedefault -count=1 go vet ./internal/components/sdd ./internal/components/opencodedefault go run ./internal/gofmtcheck- [x] Changed-scope unit tests pass. - [x] Go format check passes. - [ ] Full repository unit suite: CI will run it; the local full CLI package has unrelated Windows fixture failures and a Git subprocess timeout. - [ ] E2E: N/A; this changes generated OpenCode configuration and has no Docker E2E surface. - [x] Benchmark validation: N/A; the documented benchmark corpus has no generated OpenCode permission scenario. --- ## Automated Checks | Check | Status | |-------|--------| | Cognitive load | 400 / 400 changed lines | | Approved issue |status:approvedpresent | | Type label |type:bug| | CI | Pending | --- ## Contributor Checklist - [x] PR is linked to issue #2076 withstatus:approved. - [x] PR stays within 400 changed lines. - [x] Exactly onetype:*label is applied. - [x] Changed-scope unit tests pass. - [x] Go format passes. - [x] Benchmark validation is not applicable, as explained above. - [x] Documentation changes are not required for this internal compatibility fix. - [x] Commit follows Conventional Commits and has noCo-Authored-Bytrailer. --- ## Notes for Reviewers Please focus on last-match permission ordering and the exact-managed-object boundary. Partial or user-extended reviewer-like permission objects must remain byte-identical. ## Chain Context | Field | Value | |-------|-------| | Chain | Windows OpenCode reviewer inspection | | Tracker PR | Not needed | | Position | 1 of 2 | | Base |main| | Depends on | None | | Follow-up | #2110 | | Review budget | 387 / 400 | | Starts at | Currentorigin/main| | Ends with | Windows reviewer inspection commands admitted without broadening shell access or rewriting user rules. | ### Chain Overviewtext main |-- [current] #2109 - Windows inspection fix `-- #2110 - incident preservation coverage### Scope - Includes: generated OpenCode permissions, managed serialization, idempotency, and direct regressions. - Excludes: incident artifact preservation coverage, delivered in #2110. ### Autonomy - [x] CI is expected to pass for this PR branch. - [x] This PR has one deliverable scope. - [x] This PR can be rolled back without unrelated changes. - [x] Tests cover this unit.Summary by CodeRabbit
Bug Fixes
Tests