fix(cli): honor explicit env read allows without nested deny leak - #13143
fix(cli): honor explicit env read allows without nested deny leak#13143santhiprakash wants to merge 3 commits into
Conversation
The task tool was building child session permissions from inherited parent restrictions and primary tool denies, but did not carry the spawned subagent's own agent.permission into the child session. This caused user-configured read allow rules (e.g. *.env) to be dropped. Include next.permission as the first ruleset in the merged child permission so explicit subagent allows are preserved, while parent denials and primary tool denies still take precedence because they appear later in the ruleset. Fixes Kilo-Org#12387
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (5 files)
Fix these issues in Kilo Cloud Previous Review Summaries (2 snapshots, latest commit 8c9870d)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 8c9870d)Status: No Issues Found | Recommendation: Merge lgtm — explicit non-broad Files Reviewed (6 files)
Previous review (commit b817d08)Status: No Issues Found | Recommendation: Merge lgtm — the fix correctly exploits the last-match-wins semantics of Files Reviewed (3 files)
Reviewed by grok-4.6 · Input: 77.3K · Output: 20.7K · Cached: 848.1K Review guidance: REVIEW.md from base branch |
|
Hey @santhiprakash , I verified this change locally, and it does not solve the reported issue. The reported behavior remains the same, while the change introduces a regression where nested subagents incorrectly inherit the delegator’s deny rules. Could you please revisit the solution and include evidence of how the behavior was tested before resubmitting it for review? |
|
Thanks for checking this locally — that matches what we should have caught before asking for review. You're right on both counts: this PR does not reproduce the reporter's I am not opening another PR. I will reproduce #12387 and the nested-deny case against current
I'll comment again when that evidence is on the branch. |
Copying the subagent's full agent ruleset into session.permission did not change Permission.resolve for the reporter's .env case, and it made nested children inherit the delegator's catch-all deny. Honor an explicit non-broad read allow for .env / .env.* when resolving env reads, even if a later broad read "*" would be hardened back to ask. Leave session.permission as inherited ceilings only. Fixes Kilo-Org#12387
|
Addressed:
Regression check:
Status: ready for re-review on |
…deny leaks Do not copy the subagent's full agent ruleset into session.permission; that projected explore/plan *: deny onto nested children and still did not fix Kilo-Org#12387. Instead: - Honor explicit non-broad read allows for .env / .env.* in ReadPermission.harden so a subagent with read: { "*.env": "allow" } can read .env even when a later broad read: "*" would be hardened to ask. - Make KiloTask.inherited and deriveSubagentSessionPermission depth-aware: only the immediate child of a primary/delegating agent inherits parent session/MCP deny ceilings; deeper descendants are governed by their own agent ruleset plus default subagent restrictions. This stops nested subagents from accumulating a delegator's deny rules (#EST-1825). Fixes Kilo-Org#12387
|
@johnnyeric I revisited this and pushed the corrected fix. Here's the evidence:
The root cause was the wrong merge order in |
|
Thanks for addressing the feedback. Unfortunately, this PR still doesn’t resolve the original issue and introduces regressions. Please properly review and thoroughly test automated changes before submitting them. Otherwise, please close the PR. Thank you for understanding. |
|
Understood — thanks for testing this twice and for the clear feedback, @johnnyeric. Closing this out rather than resubmitting again without a solid fix in hand. Appreciate the time you put into reviewing it. |
Problem
Subagents still prompt on
.envreads when the user sets an explicitread: "*.env": "allow"rule. The previous revision copied the subagent's fullagent.permissionintosession.permission; that did not fix the reporter's.envcase and introduced a nested-deny regression where children inherited the delegator's catch-all and session-local denies.Root cause
Permission.resolve->ReadPermission.hardenturns a broadread: "*": "allow"intoaskfor.env/.env.*. If a later catch-all*was the last match, an earlier explicit*.envallow was ignored.next.permissionfirst inKiloTask.mergewrote the whole subagent ruleset intosession.permission.deriveSubagentSessionPermissionthen forwarded those deny rules to nested children, soexplore/plan*: deny(and edit denials inherited as ceilings) leaked to grandchildren.Fix
readallow that matches the env file, even when a later broadread: "*"would otherwise be hardened toask.session.permissionas inherited ceilings only; do not dumpnext.permissioninto it.KiloTask.inheritedandderiveSubagentSessionPermissiondepth-aware:depth === 0) inherits parent session deny andexternal_directoryceilings plus the default subagent restrictions.depth > 0) are governed by their own agent ruleset and default subagent restrictions; parent-session denies (including edit/MCP ceilings inherited by the immediate child) are not forwarded again.denystill wins.Verification
Result on this branch:
env read permissions > explicit *.env allow wins over a later broad read allow- passtool.task > execute keeps explicit subagent env allow on the production resolve path- passtool.task > nested subagent does not inherit the delegator's catch-all deny- passtool.task > nested subagent does not inherit the parent session edit deny- passSabotage checks (fail on pre-fix, pass here):
bun test test/kilocode/permission/env-read.test.ts -t "explicit *.env allow"Expected: "allow", received: "ask"bun test test/tool/task.test.ts -t "nested subagent does not inherit the parent session edit deny"Expected: not "deny"Risks
KiloTask.inheritedremain active; only deeper descendants are released from session-local deny inheritance.