Skip to content

fix: propagate session permissions correctly to sub-sessions #3542

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Piyush0049 wants to merge 8 commits into
base: main
Choose a base branch
from
+148 −37
Open

fix: propagate session permissions correctly to sub-sessions #3542

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a7c749b
fix: propagate session permissions correctly
Piyush0049 Jul 8, 2026
f90d228
fix: address review feedback on permission isolation and dispatch
Piyush0049 Jul 9, 2026
425b99a
test: update stale yolo tests and restore branch cloning
Piyush0049 Jul 9, 2026
e21bc2b
Merge remote-tracking branch 'origin/main' into fix/subsession-permis…
Piyush0049 Jul 10, 2026
7dbcbcb
Merge remote-tracking branch 'origin/main' into fix/subsession-permis…
Piyush0049 Jul 10, 2026
96d089e
chore: fix formatting for gofumpt and gci
Piyush0049 Jul 10, 2026
25a6f4a
test(runtime): fix TestForceAskOverridesYoloMode hanging on CI
Piyush0049 Jul 10, 2026
38c27a8
fix: address maintainer feedback on permission overrides
Piyush0049 Jul 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions pkg/runtime/agent_delegation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,9 @@ func newSubSession(parent *session.Session, cfg SubSessionConfig, childAgent *ag
if cfg.PinAgent {
opts = append(opts, session.WithAgentName(cfg.AgentName))
}
if parent.Permissions != nil {
opts = append(opts, session.WithPermissions(parent.Permissions))

@Sayt-0 Sayt-0 Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shares the parent's *PermissionsConfig (and its underlying slices) with the child instead of cloning it. Every other call site clones through clonePermissionsConfig (session.Clone, copySessionMetadata in branch.go, store.go). The aliasing is not benign: on the interactive transfer_task path, handleResume's ResumeTypeApproveTool case appends to sess.Permissions.Allow, so a child approving a tool would mutate the parent's permissions (and append to a shared slice). Suggest cloning here, for example by exposing the existing clonePermissionsConfig helper and passing session.WithPermissions(clone).

}
// Merge parent's excluded tools with config's excluded tools so that
// nested sub-sessions (e.g. skill → transfer_task → child) inherit
// exclusions from all ancestors and don't re-introduce filtered tools.
Expand Down Expand Up @@ -482,10 +485,6 @@ func (r *LocalRuntime) CurrentAgentSubAgentNames() []string {
// authorises all tool calls made by the sub-agent when they approve
// run_background_agent. Callers should be aware that prompt injection in
// the sub-agent's context could exploit this gate-bypass.

@Sayt-0 Sayt-0 Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this TODO looks premature. RunAgent (background agents) sets ToolsApproved: true, and Decide (pkg/runtime/toolexec/permissions.go) returns Allow on the yolo flag before evaluating any checker. So the newly inherited parent Deny/Ask rules are never consulted for a background sub-session, which is precisely the bypass this TODO called out ("rather than a single shared ToolsApproved flag"). The warning just above ("could exploit this gate-bypass") still holds, so it now contradicts the TODO removal. Options: keep the TODO, or make Deny/ForceAsk win over the yolo flag in Decide (at least for an inherited session-level Deny) and cover it with a test.

//
// TODO: propagate the parent session's per-tool permission rules once the
// runtime supports per-session permission scoping rather than a single
// shared ToolsApproved flag.
func (r *LocalRuntime) RunAgent(ctx context.Context, params agenttool.RunParams) *agenttool.RunResult {
return r.runCollecting(ctx, params.ParentSession, SubSessionConfig{
Task: params.Task,
Expand Down
25 changes: 25 additions & 0 deletions pkg/runtime/agent_delegation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,3 +266,28 @@ func TestSubSessionWithoutAttachedFilesOmitsBlock(t *testing.T) {
assert.NotContains(t, m.Content, "<attached_files>")
}
}

func TestSubSessionInheritsPermissions(t *testing.T) {
t.Parallel()

perms := &session.PermissionsConfig{
Allow: []string{"read_*"},
Deny: []string{"write_*"},
Ask: []string{"edit_*"},
}
parent := session.New(session.WithPermissions(perms))

childAgent := agent.New("worker", "")
cfg := SubSessionConfig{
Task: "refactor",
AgentName: "worker",
Title: "Refactor",
}

s := newSubSession(parent, cfg, childAgent)

require.NotNil(t, s.Permissions)
assert.Equal(t, perms.Allow, s.Permissions.Allow)
assert.Equal(t, perms.Deny, s.Permissions.Deny)
assert.Equal(t, perms.Ask, s.Permissions.Ask)
Comment on lines +297 to +300

@Sayt-0 Sayt-0 Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This checks field copying but not that inheritance changes dispatch behaviour. Consider driving the approval path instead: a parent Deny of write_* should still deny in the child, and in particular the background-agent case (ToolsApproved: true) where the inherited Deny currently has no effect. That case would document the gap flagged in agent_delegation.go.

}
Loading