fix(sdd): propagate top-level permissions to orchestrator agents#956
fix(sdd): propagate top-level permissions to orchestrator agents#956decode2 wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughPropagates root ChangesOrchestrator permission propagation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 4477-4575: The merge test only covers agents without existing
read/bash rules, so it cannot catch overwrite behavior in mergeJSONFile. Extend
TestMergeJSONFile_PropagatesPermissions to include an agent that already defines
its own read and bash entries, and assert those agent-specific rules still exist
after merge alongside the propagated top-level permissions. Use mergeJSONFile
and the permission handling under agent/gentle-orchestrator and
agent/sdd-orchestrator-custom to verify merge-not-replace semantics.
🪄 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: 13af078d-c74b-40f1-85f8-2a2bf749c0d7
📒 Files selected for processing (2)
internal/components/sdd/inject.gointernal/components/sdd/inject_test.go
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)
1494-1499: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winEmpty
permission: {}blocks may be injected when no propagatable keys exist.
changedis set totrueas soon as an emptypermissionmap is created, before confirming any ofread/bash/writeare actually present intopPerms. If the top-levelpermissioncontains none of those three keys, every orchestrator agent that lacked apermissionblock gets an empty"permission": {}written and the file is re-marshaled unnecessarily. Consider creating/attaching the map lazily only when a key is actually propagated.🤖 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 1494 - 1499, The permission propagation logic in inject.go is eagerly creating an empty agentMap["permission"] and marking changed before confirming any propagatable keys exist. Update the permission handling near the agentPerms initialization so the map is only created and attached in the loop that copies read/bash/write from topPerms, and only set changed when at least one of those keys is actually propagated. This keeps orchestrator agents from getting empty permission blocks and avoids unnecessary re-marshaling.
🤖 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 1502-1507: The non-map fallback in inject.go’s merge logic is
overwriting an agent’s existing per-pattern map and marking the value changed
even when nothing actually changed. Update the branch around topMap/okTop so a
scalar top-level value like the one merged into agentPerms[permKey] only
replaces the agent value when there is no existing map to preserve, and set
changed = true only if the stored value is actually different. Keep the fix
localized to the merge path that handles permKey and agentPerms.
- Around line 1500-1537: In propagateTopLevelPermissions, exact glob collisions
for read/bash/write must let top-level deny override agent allow instead of only
filling missing keys. Update the merge logic in inject.go so when topPerms and
agentPerms contain the same glob key, a top-level deny replaces any existing
agent rule rather than leaving the agent’s value intact. Use the existing
propagateTopLevelPermissions flow and the read, bash, and write permission map
handling to locate the collision branch.
---
Outside diff comments:
In `@internal/components/sdd/inject.go`:
- Around line 1494-1499: The permission propagation logic in inject.go is
eagerly creating an empty agentMap["permission"] and marking changed before
confirming any propagatable keys exist. Update the permission handling near the
agentPerms initialization so the map is only created and attached in the loop
that copies read/bash/write from topPerms, and only set changed when at least
one of those keys is actually propagated. This keeps orchestrator agents from
getting empty permission blocks and avoids unnecessary re-marshaling.
🪄 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: 2ddcef9e-636c-4802-9fd9-93562c428d57
📒 Files selected for processing (2)
internal/components/sdd/inject.gointernal/components/sdd/inject_test.go
|
@Alan-TheGentleman I've addressed the CodeRabbit review feedback: merged permissions are now propagated lazily, scalar top-level values won't clobber pre-existing agent permission maps, and top-level denies override agent-level allows for matching globs. I also extended TestMergeJSONFile_PropagatesPermissions and added TestMergeJSONFile_NoPropagatesIfNoKeys to verify these semantics. |
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.go`:
- Around line 1502-1524: The permission merge logic in the `permKey` loop
currently lets an existing agent-side map override a top-level scalar deny,
which can bypass a global restriction. Update the merge behavior in the
`topPerms`/`agentPerms` handling so that scalar deny values like `"read":
"deny"` always win over an agent map, or explicitly overwrite the agent entry
when the scalar is restrictive. Keep the change localized to the permission
propagation block in `inject.go` and preserve the existing map-vs-scalar
handling for non-restrictive cases if that is intentional.
🪄 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: b1b6dfed-afa5-4a79-a3aa-0e956ce5206f
📒 Files selected for processing (2)
internal/components/sdd/inject.gointernal/components/sdd/inject_test.go
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/inject.go (1)
1468-1471: 🗄️ Data Integrity & Integration | 🟠 MajorAdd explicit backup and restore handling for user config writes.
The
mergeJSONFilefunction ininternal/components/sdd/inject.gopersists merged configuration usingfilemerge.WriteFileAtomic(line 1457). While the current implementation ensures atomicity and idempotency, it lacks a backup/restore path for the existing configuration file before overwriting it.Per the project guidelines: "Any write to user config must have a backup/restore path."
The current
WriteFileAtomicimplementation (lines 42-111 ininternal/components/filemerge/writer.go) writes to a temporary file and renames it directly to the target path. If the merged result is malformed or incorrect, the original user configuration is lost without a recovered path.Required fix:
- Modify the write path to copy the existing file to a backup location (e.g.,
path.bak) before performing the atomic write.- Ensure the backup is removed only after the new write is confirmed successful, or provide a mechanism for the caller to restore the backup on failure.
Do not rely solely on atomic rename for backup safety; an explicit copy operation is required.
🤖 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 1468 - 1471, The config write path in mergeJSONFile/WriteFileAtomic needs an explicit backup and restore flow, not just atomic rename. Before overwriting the user config, copy the existing file to a backup path (for example via a helper around filemerge.WriteFileAtomic or in mergeJSONFile), then only delete that backup after the new write succeeds. If the merge/write fails, restore from the backup so the original config remains recoverable, and use the existing mergeJSONFile and filemerge.WriteFileAtomic symbols to place the change.Source: Path instructions
🤖 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/inject.go`:
- Around line 1468-1471: The config write path in mergeJSONFile/WriteFileAtomic
needs an explicit backup and restore flow, not just atomic rename. Before
overwriting the user config, copy the existing file to a backup path (for
example via a helper around filemerge.WriteFileAtomic or in mergeJSONFile), then
only delete that backup after the new write succeeds. If the merge/write fails,
restore from the backup so the original config remains recoverable, and use the
existing mergeJSONFile and filemerge.WriteFileAtomic symbols to place the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b72c45e3-f61b-4575-9b2c-fc69e734d9e1
📒 Files selected for processing (2)
internal/components/sdd/inject.gointernal/components/sdd/inject_test.go
1 participant
Closes #787
Summary by CodeRabbit
Bug Fixes
read,bash,write) into targeted orchestrator agents, while preserving agent-specific overrides and task replacement entries.Tests