🪛 fix: FIFO Walk Order in buildInitialToolSessions (P3 review)

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).
This commit is contained in:
Danny Avila 2026-04-26 18:36:30 -04:00
parent 2d82dc2507
commit 007f32341e
2 changed files with 53 additions and 10 deletions

View file

@ -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

View file

@ -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<CodeFilesAgent>) 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<CodeFilesAgent>`)
* 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<CodeFilesAgent>();
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);
}
}
}