mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-05-13 07:46:47 +00:00
🪛 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:
parent
2d82dc2507
commit
007f32341e
2 changed files with 53 additions and 10 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue