From 007f32341e8bf018e7281a0c3919758c36d7c650 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sun, 26 Apr 2026 18:36:30 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=AA=9B=20fix:=20FIFO=20Walk=20Order=20in?= =?UTF-8?q?=20buildInitialToolSessions=20(P3=20review)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The traversal used `Array.pop()` (LIFO), which visited the LAST top-level agent first. The docstring says "primary first"; the code contradicted it. When no skill seed exists the first-visited agent's first file supplies the representative `session_id` written to `Graph.sessions[EXECUTE_CODE]` — so a LIFO walk silently flipped which agent that came from. `ToolNode` ultimately uses per-file `session_id`s for runtime injection (so behavior was indistinguishable for current callers), but the discrepancy was a footgun for any future consumer that read the representative. Switch to FIFO via `Array.shift()` to match both the docstring and the existing `loadSubagentsFor` walk pattern in `Endpoints/agents/initialize.js`. Add a regression test that asserts the primary's `session_id` is the representative (and that all three agents' files still contribute, with per-file `session_id`s preserved). --- .../api/src/agents/codeFilesSession.spec.ts | 26 +++++++++++++ packages/api/src/agents/codeFilesSession.ts | 37 ++++++++++++++----- 2 files changed, 53 insertions(+), 10 deletions(-) diff --git a/packages/api/src/agents/codeFilesSession.spec.ts b/packages/api/src/agents/codeFilesSession.spec.ts index 5e3d500e44..3afc3b5d87 100644 --- a/packages/api/src/agents/codeFilesSession.spec.ts +++ b/packages/api/src/agents/codeFilesSession.spec.ts @@ -198,6 +198,32 @@ describe('buildInitialToolSessions', () => { expect(entry.files![0].name).toBe('x.txt'); }); + it('walks primary-first so the primary supplies the representative session_id (no skill seed)', () => { + /** + * Regression: a LIFO stack would visit the last top-level agent + * first, flipping which agent's first file becomes the + * representative `session_id` written to the EXECUTE_CODE entry. + * The walk is FIFO so the primary always lands first. + */ + const primary = agent('primary', [file('p1', 'sess-PRIMARY', 'top.txt')]); + const handoffA = agent('handoff-A', [file('a1', 'sess-A', 'a.txt')]); + const handoffB = agent('handoff-B', [file('b1', 'sess-B', 'b.txt')]); + + const result = buildInitialToolSessions({ + agents: [primary, handoffA, handoffB], + }); + + const entry = result!.get(Constants.EXECUTE_CODE) as CodeSessionContext; + expect(entry.session_id).toBe('sess-PRIMARY'); + /* All three agents still contributed their files into the merged set. */ + expect(entry.files!.map((f) => f.name).sort()).toEqual(['a.txt', 'b.txt', 'top.txt']); + /* And the per-file session_ids are preserved (ToolNode injects per-file). */ + const byName = new Map(entry.files!.map((f) => [f.name, f.session_id])); + expect(byName.get('top.txt')).toBe('sess-PRIMARY'); + expect(byName.get('a.txt')).toBe('sess-A'); + expect(byName.get('b.txt')).toBe('sess-B'); + }); + it('deduplicates a single agent referenced as both primary and a subagent', () => { /** * `agentConfigs` may include an agent that is also a subagent of diff --git a/packages/api/src/agents/codeFilesSession.ts b/packages/api/src/agents/codeFilesSession.ts index c26bc050d5..1118529e7c 100644 --- a/packages/api/src/agents/codeFilesSession.ts +++ b/packages/api/src/agents/codeFilesSession.ts @@ -76,11 +76,19 @@ export function seedCodeFilesIntoSessions( * sessions); changing only this helper would diverge from how the * sandbox actually behaves at runtime. * - * **Walk:** primary first, then `agentConfigs` (handoff/addedConvo), - * then recurse into each config's `subagentAgentConfigs`. The visited - * set is keyed by object identity (Set) so cycles in - * a malformed agent graph (a subagent that points back at its parent) - * can't infinite-loop the seed. + * **Walk order:** primary first, then `agentConfigs` (handoff/addedConvo) + * in iteration order, then recurse into each config's + * `subagentAgentConfigs` breadth-first. Order matters because when no + * skill sessions exist, the FIRST agent's first file supplies the + * representative `session_id` written to `Graph.sessions[EXECUTE_CODE]`. + * `ToolNode` ultimately uses per-file `session_id`s for injection so + * the representative is informational rather than load-bearing, but + * primary-first keeps it predictable and matches the existing + * `loadSubagentsFor` walk pattern in `Endpoints/agents/initialize.js`. + * + * The visited set is keyed by object identity (`Set`) + * so cycles in a malformed agent graph (a subagent that points back at + * its parent) can't infinite-loop the seed. * * @param skillSessions - Output of `primeInvokedSkills` — already * contains an `EXECUTE_CODE` entry when skill files were primed; new @@ -97,12 +105,21 @@ export function buildInitialToolSessions(params: { const { skillSessions, agents } = params; let sessions = skillSessions; const visited = new Set(); - const stack: CodeFilesAgent[] = []; + /** + * FIFO queue: primary lands at index 0 and gets visited first, so its + * first file is what `seedCodeFilesIntoSessions` records as the + * representative `session_id` (when no skill seed exists). A LIFO + * stack (`pop()`) would visit the last top-level agent first and + * silently flip which agent supplies that id. `Array.shift()` is + * O(n); the agent set is small (handoff + subagents, typically <20) + * so the overhead is negligible vs. the readability win. + */ + const queue: CodeFilesAgent[] = []; for (const a of agents) { - if (a) stack.push(a); + if (a) queue.push(a); } - while (stack.length > 0) { - const agent = stack.pop()!; + while (queue.length > 0) { + const agent = queue.shift()!; if (visited.has(agent)) continue; visited.add(agent); if (agent.primedCodeFiles && agent.primedCodeFiles.length > 0) { @@ -110,7 +127,7 @@ export function buildInitialToolSessions(params: { } if (agent.subagentAgentConfigs && agent.subagentAgentConfigs.length > 0) { for (const child of agent.subagentAgentConfigs) { - if (child && !visited.has(child)) stack.push(child); + if (child && !visited.has(child)) queue.push(child); } } }