fix(orchestrator): guard trusted exploration handoffs - #1966
Open
blak0p wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds trusted exploration handoff guardrails to every SDD orchestrator asset, updates preserved OpenCode delegation-hard-gates migration, and adds contract tests for asset coverage and migration behavior. ChangesTrusted exploration handoff safeguards
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
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 |
2 tasks
![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
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/inject.go (1)
1121-1137: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winValidate the marker-delimited managed block, not the entire prompt.
Global string checks conflict with the contract that preserves arbitrary user-owned tails and can either accept stale policy content or reject valid prompts.
internal/components/sdd/inject.go#L1121-L1137: extract the managed block and require the complete trusted-exploration clause set before returning through the fast path.internal/components/sdd/inject_test.go#L952-L956: count policy markers only inside that block and assert the preserved user tail byte-for-byte at its expected boundary.🤖 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.go` around lines 1121 - 1137, The fast-path validation in internal/components/sdd/inject.go around the marker checks must inspect only the marker-delimited managed block, requiring the complete trusted-exploration clause set there before returning; do not validate arbitrary prompt-tail content. Update the corresponding test in internal/components/sdd/inject_test.go to count policy markers within that block and assert the user-owned tail is preserved byte-for-byte at its expected boundary.
🤖 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/assets/assets_test.go`:
- Around line 125-130: Update the bounded mandatory delegation section parsing
in the test to validate start before slicing content. Check whether the first
strings.Index call found "#### Mandatory Delegation Triggers", fail immediately
when absent, then compute the relative end index for "#### Native Checking
Contract" and retain the existing missing-section failure and delegation slice
behavior.
In `@internal/components/sdd/inject_test.go`:
- Around line 952-956: Update the migration assertions around
ensurePreservedOpenCodeDelegationHardGates to extract the managed block before
counting the corrected handoff phrase and migration marker, so user-owned
content cannot affect uniqueness checks. Replace the broad strings.Contains(got,
userTail) assertion with a byte-exact boundary check using the appropriate
suffix assertion, while retaining the idempotence comparison.
---
Outside diff comments:
In `@internal/components/sdd/inject.go`:
- Around line 1121-1137: The fast-path validation in
internal/components/sdd/inject.go around the marker checks must inspect only the
marker-delimited managed block, requiring the complete trusted-exploration
clause set there before returning; do not validate arbitrary prompt-tail
content. Update the corresponding test in internal/components/sdd/inject_test.go
to count policy markers within that block and assert the user-owned tail is
preserved byte-for-byte at its expected boundary.
🪄 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: de101d11-a5c7-4cc3-ba14-ba7158ca0bf1
⛔ Files ignored due to path filters (13)
testdata/golden/combined-claude-claudemd.goldenis excluded by!testdata/**testdata/golden/combined-windsurf-global-rules.goldenis excluded by!testdata/**testdata/golden/sdd-antigravity-rulesmd.goldenis excluded by!testdata/**testdata/golden/sdd-claude-claudemd.goldenis excluded by!testdata/**testdata/golden/sdd-codex-agentsmd-lowcost.goldenis excluded by!testdata/**testdata/golden/sdd-codex-agentsmd-powerful.goldenis excluded by!testdata/**testdata/golden/sdd-codex-agentsmd.goldenis excluded by!testdata/**testdata/golden/sdd-cursor-rules.goldenis excluded by!testdata/**testdata/golden/sdd-gemini-geminimd.goldenis excluded by!testdata/**testdata/golden/sdd-kiro-instructions.goldenis excluded by!testdata/**testdata/golden/sdd-opencode-multi-settings.goldenis excluded by!testdata/**testdata/golden/sdd-vscode-instructions.goldenis excluded by!testdata/**testdata/golden/sdd-windsurf-global-rules.goldenis excluded by!testdata/**
📒 Files selected for processing (15)
internal/assets/antigravity/sdd-orchestrator.mdinternal/assets/assets_test.gointernal/assets/claude/sdd-orchestrator.mdinternal/assets/codex/sdd-orchestrator.mdinternal/assets/cursor/sdd-orchestrator.mdinternal/assets/gemini/sdd-orchestrator.mdinternal/assets/generic/sdd-orchestrator.mdinternal/assets/hermes/sdd-orchestrator.mdinternal/assets/kimi/sdd-orchestrator.mdinternal/assets/kiro/sdd-orchestrator.mdinternal/assets/opencode/sdd-orchestrator.mdinternal/assets/qwen/sdd-orchestrator.mdinternal/assets/windsurf/sdd-orchestrator.mdinternal/components/sdd/inject.gointernal/components/sdd/inject_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/components/sdd/inject_test.go`:
- Around line 982-987: Update userTail in
TestEnsurePreservedOpenCodeDelegationHardGatesIgnoresCompleteLookingUserTail to
include every required fast-path phrase, including the 4-file rule, Write rule,
Semantic guard, targeted verification, and handoff prohibition. Keep the managed
incomplete block unchanged so the test verifies fast-path checks use only
user-owned content rather than the whole prompt.
🪄 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: d0ddccf8-10c4-45d8-af6c-2918b17e3b2b
📒 Files selected for processing (3)
internal/assets/assets_test.gointernal/components/sdd/inject.gointernal/components/sdd/inject_test.go
Comment on lines
+982
to
+987
| func TestEnsurePreservedOpenCodeDelegationHardGatesIgnoresCompleteLookingUserTail(t *testing.T) { | ||
| const incomplete = "<!-- gentle-ai:delegation-hard-gates-migration -->\n" + | ||
| "### Mandatory Delegation Triggers (Non-Skippable)\n" + | ||
| "<!-- /gentle-ai:delegation-hard-gates-migration -->" | ||
| const userTail = "\n\n#### User-Owned Policy\nfully mandatory\nBounded read rule\n" + | ||
| "complete, current exploration handoff\n" + nativeReviewAuthorityRuleText |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the user tail satisfy every fast-path clause.
The fixture omits rules such as 4-file rule, Write rule, Semantic guard, targeted verification, and the handoff prohibition. A regression that checks the whole prompt instead of managed would still fail its incomplete block requirements and migrate, so this test would not detect it. Populate userTail with every required fast-path phrase while keeping the managed block incomplete.
🤖 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 982 - 987, Update
userTail in
TestEnsurePreservedOpenCodeDelegationHardGatesIgnoresCompleteLookingUserTail to
include every required fast-path phrase, including the 4-file rule, Write rule,
Semantic guard, targeted verification, and handoff prohibition. Keep the managed
incomplete block unchanged so the test verifies fast-path checks use only
user-owned content rather than the whole prompt.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Linked Issue
Closes #1178
🏷️ PR Type
type:bug— Bug fix (non-breaking change that fixes an issue)type:featuretype:docstype:refactortype:choretype:breaking-change📝 Summary
Adds a guarded trusted-exploration handoff policy so complete, current exploration results can suppress broad redundant rereads while still requiring targeted verification. Incomplete or untrustworthy handoffs trigger narrowly scoped exploration instead, and the trust boundary does not extend to implementation, review, apply, or verification handoffs.
📂 Changes
28 files changed: 114 additions, 1 deletion, 113 net lines.
internal/assets/*/sdd-orchestrator.md(12 files)internal/assets/assets_test.gointernal/components/sdd/inject.gointernal/components/sdd/inject_test.gotestdata/golden/*(13 files)🧪 Test Plan
Exact verification run locally on Fedora/Linux:
go test ./... -count=1— PASSgo build ./... && go vet ./...— PASSgo test ./internal/components/sdd ./internal/assets -count=1— PASSgo run ./internal/gofmtcheck— PASS11.17.0, Nodev26.5.0, prefix/home/linuxbrew/.linuxbrew; no npm check applies to this Go-only change.🤖 Automated Checks
The PR workflow will evaluate cognitive load, issue linkage and approval, the exact type label, unit tests, Go format, and E2E status. Review-driven development is disabled globally for this delivery; no review receipt or approval is being claimed.
✅ Contributor Checklist
status:approvedtype:*label — maintainer action may be required; see belowgo test ./... -count=1)go run ./internal/gofmtcheck)Co-Authored-Bytrailer💬 Notes for Reviewers
Please verify the trust boundary: only complete, current exploration handoffs suppress broad rereads; targeted verification remains required, and trust never transfers to later implementation/review/apply/verification handoffs.
Pending maintainer actions
type:bug, is applied if the API did not apply it automatically. Contributor permissions do not allow applying PR labels, so no label mutation was attempted.Summary by CodeRabbit
New Features
Bug Fixes
Tests