Skip to content

fix: sub-session thinking state derived from child agent, not parent session #2149

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

Merged
dgageot merged 1 commit into from
Mar 18, 2026
+49 −25
Merged

fix: sub-session thinking state derived from child agent, not parent session #2149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
32 changes: 10 additions & 22 deletions pkg/runtime/agent_delegation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,6 @@ type SubSessionConfig struct {
Title string
// ToolsApproved overrides whether tools are pre-approved in the child session.
ToolsApproved bool
// Thinking propagates the parent's thinking-mode flag.
Thinking bool
// PinAgent, when true, pins the child session to AgentName via
// session.WithAgentName. This is required for concurrent background
// tasks that must not share the runtime's mutable currentAgent field.
Expand Down Expand Up @@ -110,7 +108,7 @@ func newSubSession(parent *session.Session, cfg SubSessionConfig, childAgent *ag
session.WithMaxConsecutiveToolCalls(childAgent.MaxConsecutiveToolCalls()),
session.WithTitle(cfg.Title),
session.WithToolsApproved(cfg.ToolsApproved),
session.WithThinking(cfg.Thinking),
session.WithThinking(childAgent.ThinkingConfigured()),
session.WithSendUserMessage(false),
session.WithParentID(parent.ID),
}
Expand All @@ -121,8 +119,8 @@ func newSubSession(parent *session.Session, cfg SubSessionConfig, childAgent *ag
}

// runSubSessionForwarding runs a child session within the parent, forwarding all
// events to the caller's event channel and propagating session state (tool
// approvals, thinking) back to the parent when done.
// events to the caller's event channel and propagating tool approval state
// back to the parent when done.
//
// This is the "interactive" path used by transfer_task where the parent agent
// loop is blocked while the child executes.
Expand All @@ -137,7 +135,6 @@ func (r *LocalRuntime) runSubSessionForwarding(ctx context.Context, parent, chil
}

parent.ToolsApproved = child.ToolsApproved
Copy link

@docker-agent docker-agent bot Mar 17, 2026

Choose a reason for hiding this comment

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

🔴 CRITICAL: Back-propagation of thinking state removed affects skill execution

The removal of parent.Thinking = child.Thinking from runSubSessionForwarding() is correct for task transfers (where different agents have their own thinking config), but incorrect for skills (which run as the same agent and should maintain user thinking preferences).

Impact: Skills inherit the initial thinking state via the explicit override at skill_runner.go:83, but user /think toggles during skill execution won't persist to the parent session. This creates inconsistent behavior between task transfers (correct) and skills (bug).

Recommendation: Add conditional back-propagation for skills, or refactor to use separate code paths for task transfers vs. skill execution.

parent.Thinking = child.Thinking

parent.AddSubSession(child)
evts <- SubSessionCompleted(parent.ID, child, callerAgent)
Expand Down Expand Up @@ -216,7 +213,6 @@ func (r *LocalRuntime) RunAgent(ctx context.Context, params agenttool.RunParams)
AgentName: params.AgentName,
Title: "Background agent task",
ToolsApproved: true,
Thinking: sess.Thinking,
PinAgent: true,
}

Expand Down Expand Up @@ -252,43 +248,35 @@ func (r *LocalRuntime) handleTaskTransfer(ctx context.Context, sess *session.Ses

slog.Debug("Transferring task to agent", "from_agent", a.Name(), "to_agent", params.Agent, "task", params.Task)

ca := r.CurrentAgentName()

// Emit agent switching start event
evts <- AgentSwitching(true, ca, params.Agent)
evts <- AgentSwitching(true, a.Name(), params.Agent)

r.setCurrentAgent(params.Agent)
defer func() {
r.setCurrentAgent(ca)
r.setCurrentAgent(a.Name())

// Emit agent switching end event
evts <- AgentSwitching(false, params.Agent, ca)
evts <- AgentSwitching(false, params.Agent, a.Name())

// Restore original agent info in sidebar
if originalAgent, err := r.team.Agent(ca); err == nil {
evts <- AgentInfo(originalAgent.Name(), getAgentModelID(originalAgent), originalAgent.Description(), originalAgent.WelcomeMessage())
}
evts <- AgentInfo(a.Name(), getAgentModelID(a), a.Description(), a.WelcomeMessage())
}()

// Emit agent info for the new agent
if newAgent, err := r.team.Agent(params.Agent); err == nil {
evts <- AgentInfo(newAgent.Name(), getAgentModelID(newAgent), newAgent.Description(), newAgent.WelcomeMessage())
}

slog.Debug("Creating new session with parent session", "parent_session_id", sess.ID, "tools_approved", sess.ToolsApproved, "thinking", sess.Thinking)

child, err := r.team.Agent(params.Agent)
if err != nil {
return nil, err
}
evts <- AgentInfo(child.Name(), getAgentModelID(child), child.Description(), child.WelcomeMessage())

slog.Debug("Creating new session with parent session", "parent_session_id", sess.ID, "tools_approved", sess.ToolsApproved)

cfg := SubSessionConfig{
Task: params.Task,
ExpectedOutput: params.ExpectedOutput,
AgentName: params.Agent,
Title: "Transferred task",
ToolsApproved: sess.ToolsApproved,
Thinking: sess.Thinking,
}

s := newSubSession(sess, cfg, child)
Expand Down
37 changes: 35 additions & 2 deletions pkg/runtime/agent_delegation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,15 +73,14 @@ func TestNewSubSession(t *testing.T) {
AgentName: "worker",
Title: "Test task",
ToolsApproved: true,
Thinking: true,
}

s := newSubSession(parent, cfg, childAgent)

assert.Equal(t, parent.ID, s.ParentID)
assert.Equal(t, "Test task", s.Title)
assert.True(t, s.ToolsApproved)
assert.True(t, s.Thinking)
assert.False(t, s.Thinking) // childAgent has no ThinkingConfigured
assert.False(t, s.SendUserMessage)
assert.Equal(t, 10, s.MaxIterations)
// AgentName should NOT be set when PinAgent is false
Expand Down Expand Up @@ -162,6 +161,40 @@ func TestSubSessionConfig_DefaultValues(t *testing.T) {
assert.Empty(t, s.AgentName)
}

func TestSubSessionConfig_ThinkingFromChildAgent(t *testing.T) {
parent := session.New(session.WithUserMessage("hello"))

t.Run("child agent without thinking configured gets thinking=false even if parent has thinking=true", func(t *testing.T) {
parent.Thinking = true

childAgent := agent.New("haiku-worker", "")

cfg := SubSessionConfig{
Task: "simple task",
AgentName: "haiku-worker",
Title: "Transferred task",
}

s := newSubSession(parent, cfg, childAgent)
assert.False(t, s.Thinking, "sub-session should NOT inherit parent's thinking when child agent has no thinking_budget")
})

t.Run("child agent with thinking configured gets thinking=true", func(t *testing.T) {
parent.Thinking = false

childAgent := agent.New("opus-worker", "", agent.WithThinkingConfigured(true))

cfg := SubSessionConfig{
Task: "complex task",
AgentName: "opus-worker",
Title: "Transferred task",
}

s := newSubSession(parent, cfg, childAgent)
assert.True(t, s.Thinking, "sub-session should have thinking enabled when child agent has thinking_budget")
})
}

func TestSubSessionConfig_InheritsAgentLimits(t *testing.T) {
parent := session.New(session.WithUserMessage("hello"))

Expand Down
5 changes: 4 additions & 1 deletion pkg/runtime/skill_runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,13 @@ func (r *LocalRuntime) handleRunSkill(ctx context.Context, sess *session.Session
AgentName: ca,
Title: "Skill: " + params.Name,
ToolsApproved: sess.ToolsApproved,
Thinking: sess.Thinking,
}

s := newSubSession(sess, cfg, a)
// Skills run as the same agent, so they inherit the session's current
// thinking state (which may have been toggled by the user via /think)
// rather than the agent's static config default.
s.Thinking = sess.Thinking
Copy link

@docker-agent docker-agent bot Mar 17, 2026

Choose a reason for hiding this comment

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

🔴 CRITICAL: Skill sub-session thinking state changes are lost after completion

The code explicitly overrides s.Thinking = sess.Thinking to inherit the parent session's thinking state (as documented in the comment above). However, runSubSessionForwarding() no longer propagates child.Thinking back to the parent (the line parent.Thinking = child.Thinking was removed at line 137 in agent_delegation.go).

Impact: If a user toggles thinking via /think during a skill sub-session, that state change will be lost when the skill completes. The parent session will revert to its pre-skill thinking state, breaking the expected user experience where /think toggles are session-wide.

Recommendation: Either restore back-propagation for skills specifically, or document that thinking toggles during skill execution are intentionally scoped to the skill only.

Copy link

@docker-agent docker-agent bot Mar 17, 2026

Choose a reason for hiding this comment

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

🔴 CRITICAL: Unsynchronized read of sess.Thinking creates data race

The code reads sess.Thinking without holding any lock. The session.Session struct has a mu sync.RWMutex that only protects the Messages slice. Reading sess.Thinking without synchronization is a data race under Go's memory model if another goroutine (e.g., a /think command handler or concurrent sub-session) modifies it.

Impact: Data race that could cause undefined behavior, especially under concurrent skill execution or user commands.

Recommendation: Either protect sess.Thinking with the existing mutex (expand its scope), or use atomic operations for the Thinking field. This is an architectural issue but manifests in this new code.


return r.runSubSessionForwarding(ctx, sess, s, span, evts, ca)
}
Loading