From b13797eef4d0aac900c901597b71edc7979acb93 Mon Sep 17 00:00:00 2001 From: serkanalgur Date: Mon, 28 Sep 2026 11:28:29 +0400 Subject: [PATCH 1/2] fix: settle a finished agent before dropping it from the state map MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nothing settled a spawned agent that stopped working outside a spawn tool's own success branch. `working` was written the moment `spawnAgent` returned and read back by nobody who could clear it, so every other exit left the row claiming to be busy for the life of the process: a rejected `session.wait` (an aborted or evicted child), a throw from a later read, a failing `storage.set`. `getState().agents` reported a dead agent as working and `sessionViews()` agreed with it, mapping `working` to `running`, so both views of one session claimed spend that had stopped. Settle through one door, `NexusOrchestrator.settleAgent`, which writes a terminal status and keeps the row. Keeping the row is the point: an agent dropped from `this.agents` takes its session row with it, and the only reason a session outlives its agent is `terminateAgent` dropping one that is still generating. Settling is not that case, and deleting here would publish a fabricated `owned: false` orphan. First terminal outcome wins; `lastActivity` is refreshed so `cleanupStaleData` owns the eviction. The spawn tool also conflated its own timeout with a dead poll, and treated both as "may still be running". A timeout is our deadline firing and the session may genuinely still be generating; a rejection is the poll dying, which is not a running session. They are tagged apart now, and a dead poll settles at each exit — `completed` when the session produced output, `failed` when it did not. `sessions[]` is the one collection a reader may treat as fully populated. Pass 1 already skipped an agent with no session; the collection-record constructor did not, so a record keyed by an empty id built a row naming no session. It is rejected there, and the publisher drops an empty id as a backstop. --- src/index.ts | 78 ++++++++++-- src/orchestrator.ts | 60 ++++++++- test/spawn-subagent-tool.test.ts | 168 +++++++++++++++++++++++++ test/state-sessions.test.ts | 204 +++++++++++++++++++++++++++++++ 4 files changed, 499 insertions(+), 11 deletions(-) diff --git a/src/index.ts b/src/index.ts index 4cfa76b..f1a5b67 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1917,8 +1917,18 @@ A design decision, in this shape: options: { codemode: true }, execute: async (input: unknown, toolCtx: any) => { const { role, task, model, wait, timeout } = input as { role: string; task: string; model?: string; wait?: boolean; timeout?: number } + // Held OUTSIDE the try so the catch below can settle it. The agent is + // in `orchestrator.agents` from the moment `spawnAgent` returns, and + // this block marks it `working`; every exit that is not a deliberate + // settle — a `session.wait` that rejects, a throw from any read, a + // failing `storage.set` — used to leave it at `working` in + // `getState().agents` for the rest of the process. A finished agent + // that still claims to be working is a lie the page cannot see + // through, so the failure path settles it rather than leaking it. + let spawned: SpawnedAgent | null = null try { const agent = await spawnAndDeliver({ role, task, model }, toolCtx) + spawned = agent agent.status = 'working' orchestrator.notifyStateChange() @@ -1927,15 +1937,34 @@ A design decision, in this shape: if (wait) { const waitTimeout = timeout || 120000 const waitPromise = ctx.session.wait({ sessionID: agent.sessionID! }) - const timeoutPromise = new Promise((_, reject) => - setTimeout(() => reject(new Error(`Timed out after ${waitTimeout}ms`)), waitTimeout) - ) + // A TIMEOUT and a REJECTED POLL arrive at the same `catch` and + // mean opposite things, so they are tagged apart here rather than + // conflated below. A timeout is our own deadline firing: the + // session was never told to stop and may still be generating. A + // rejection is `session.wait` itself dying — an aborted, evicted + // or unknown session — and a dead poll is not a running session, + // which is the distinction `executeTask` already draws when it + // wraps the same call in a guard that cannot reject. + const timeoutPromise = new Promise((_, reject) => { + const expiry = new Error(`Timed out after ${waitTimeout}ms`) + ;(expiry as Error & { nexusTimedOut?: boolean }).nexusTimedOut = true + setTimeout(() => reject(expiry), waitTimeout) + }) try { await Promise.race([waitPromise, timeoutPromise]) } catch (waitError: any) { - // Timeout or cancellation — agent may still be running - agent.status = 'working' + // A TIMEOUT means the session may still be running, and + // `working` is the honest status for it — left alone on + // purpose. A REJECTED POLL means the wait itself died, and no + // status here can be trusted to survive: this agent is not + // working, and publishing it as though it were is the stuck- + // `working` row from issue #100. What it settled AS depends on + // whether the session produced anything, so the settle happens + // at each exit below rather than here — one settle per outcome, + // whichever is actually known. + const pollDied = !(waitError as Error & { nexusTimedOut?: boolean })?.nexusTimedOut + if (!pollDied) agent.status = 'working' orchestrator.notifyStateChange() // Try to get whatever results are available @@ -1943,7 +1972,7 @@ A design decision, in this shape: const messages = await ctx.session.context({ sessionID: agent.sessionID! }) const partialText = lastAssistantText(messages) if (partialText) { - agent.status = 'completed' + if (pollDied) orchestrator.settleAgent(agent.id, 'completed') await ctx.storage.set("orchestrator-state", JSON.parse(JSON.stringify(orchestrator.getState()))) const taskPreview = task.length > 80 ? task.substring(0, 77) + '...' : task return { @@ -1961,6 +1990,7 @@ A design decision, in this shape: // Context read also failed } + if (pollDied) orchestrator.settleAgent(agent.id, 'failed') return { content: [ `${agent.name}`, @@ -1977,7 +2007,7 @@ A design decision, in this shape: const messages = await ctx.session.context({ sessionID: agent.sessionID! }) const result = lastAssistantText(messages) || 'Task completed (no output captured)' - agent.status = 'completed' + orchestrator.settleAgent(agent.id, 'completed') await ctx.storage.set("orchestrator-state", JSON.parse(JSON.stringify(orchestrator.getState()))) const taskPreview = task.length > 80 ? task.substring(0, 77) + '...' : task @@ -1993,7 +2023,7 @@ A design decision, in this shape: } } catch { // Context read failed after successful wait - agent.status = 'completed' + orchestrator.settleAgent(agent.id, 'completed') await ctx.storage.set("orchestrator-state", JSON.parse(JSON.stringify(orchestrator.getState()))) return { content: [ @@ -2007,6 +2037,13 @@ A design decision, in this shape: } // Non-wait: return spawn info with complexity analysis + // + // `working` is left in place deliberately, and is the one case where + // that is honest rather than a leak: this tool has NOT been told the + // session finished, and a non-wait spawn has no waiter to hear it. + // The caller is pointed at `nexus.result()`, which is the surface + // that reports the outcome. Inventing a terminal status here would + // be a worse lie than a running row. const complexity = orchestrator.analyzeComplexity({ id: `spawn-${Date.now()}`, name: task, @@ -2034,6 +2071,11 @@ A design decision, in this shape: ].join('\n') return { content: output } } catch (error: any) { + // The spawn itself never reached `spawned` on a spawn failure, and + // there is then no agent in the map to settle. Once it HAS spawned, + // this is a task that is over — by failure rather than by completion + // — and leaving it at `working` is the bug this settles. + if (spawned) orchestrator.settleAgent(spawned.id, 'failed') return { content: `Failed to spawn agent: ${error.message}` } } } @@ -2056,8 +2098,16 @@ A design decision, in this shape: options: { codemode: true }, execute: async (input: unknown, toolCtx: any) => { const { role, task, model, timeout } = input as { role: string; task: string; model?: string; timeout?: number } + // See the `spawn` tool's own note: the agent is in the map and marked + // `working` before anything below can throw, and this catch is the + // exit that used to leave it there. `session.wait` REJECTING is the + // ordinary way in here — an aborted or gone session rejects rather + // than timing out — and that is not a session still working, so it + // must not keep reporting one. + let spawned: SpawnedAgent | null = null try { const agent = await spawnAndDeliver({ role, task, model }, toolCtx) + spawned = agent agent.status = 'working' orchestrator.notifyStateChange() @@ -2075,7 +2125,11 @@ A design decision, in this shape: const messages = await ctx.session.context({ sessionID: agent.sessionID! }) const resultContent = lastAssistantText(messages) || (outcome === 'timeout' ? 'Timed out — agent may still be running' : 'Completed with no output') - agent.status = outcome === 'timeout' ? 'working' : 'completed' + // `timeout` leaves the agent `working` and it is right to: the + // session was never told to stop and is still spending. Only a + // completed wait is an outcome, and it settles like every other. + if (outcome === 'completed') orchestrator.settleAgent(agent.id, 'completed') + else agent.status = 'working' orchestrator.notifyStateChange() await ctx.storage.set("orchestrator-state", JSON.parse(JSON.stringify(orchestrator.getState()))) @@ -2091,7 +2145,8 @@ A design decision, in this shape: ].join('\n') } } catch { - agent.status = outcome === 'timeout' ? 'working' : 'completed' + if (outcome === 'completed') orchestrator.settleAgent(agent.id, 'completed') + else agent.status = 'working' orchestrator.notifyStateChange() return { content: [ @@ -2103,6 +2158,9 @@ A design decision, in this shape: } } } catch (error: any) { + // See the `spawn` tool's catch. A rejected `session.wait` lands here + // and the agent is not working any more. + if (spawned) orchestrator.settleAgent(spawned.id, 'failed') return { content: `Delegate failed: ${error.message}` } } } diff --git a/src/orchestrator.ts b/src/orchestrator.ts index 237fd2a..019961f 100644 --- a/src/orchestrator.ts +++ b/src/orchestrator.ts @@ -1947,7 +1947,14 @@ export class NexusOrchestrator { }) } - return [...views.values()] + // The published rows, and a guard rather than a projection. `id` is the key + // of every row and `state` is written by all three passes, so neither can be + // absent or null through those constructors — but `sessions[]` is the one + // collection a reader is entitled to treat as fully populated, and an empty + // key reaching it would be a row that names no session and claims no state. + // A row with no id is dropped here rather than published; a `running`/`settled` + // row with a real id is not a phantom and stays. + return [...views.values()].filter(view => view.id.length > 0) } /** @@ -1968,6 +1975,13 @@ export class NexusOrchestrator { observedUncollected: number } ): void { + // A record with no session id is not a session. `sessionViews()` pass 1 + // already skips an agent with no session for the same reason — there is + // nothing to key a row by, and a row with an empty `id` is a phantom in a + // list a user is meant to trust. Passes 2 and 3 did not have this check, + // which is the one place a session-keyed row could be built with a + // non-session key. + if (!record.sessionID) return const existing = views.get(record.sessionID) // `record.agentId` is deliberately NOT used to fill a null `agentId` on an // existing orphan row, and is not used to decide `owned` either: both are @@ -3887,6 +3901,50 @@ export class NexusOrchestrator { return agent } + /** + * Put a spawned agent into a TERMINAL status — and leave it in `this.agents`. + * + * WHY THIS EXISTS. `working` is written by the two spawn tools in `index.ts` + * (and by `executeTask`), and it was read back by nobody who could clear it: + * the only writer of a terminal status on the tool path was the tool body + * itself, at the end of its own success branch. Every path that reached the + * tool's OUTER `catch` — a `session.wait` that rejects rather than timing out, + * a read that throws, a `storage.set` that fails — returned "Failed to spawn + * agent" to the caller and left the agent sitting at `working` in + * `getState().agents` for the rest of the process. A finished agent that + * claims to still be working is worse than a missing one: the page's `Running` + * filter stays lit, and `sessionViews()` maps `working` to `running`, so the + * session row claims the session is spending too. + * + * WHY IT DOES NOT DELETE. The row has to survive, because the row is what + * makes the outcome readable: an agent removed from `this.agents` takes its + * session row with it (pass 1 of `sessionViews()` reads this map), and the + * only reason a session outlives its agent — the orphan that + * `SessionStateView` exists to make visible — is `terminateAgent` deleting a + * session that is STILL RUNNING. Settling is not that case, and pretending it + * were would put a fabricated `agentId: null` row on a session that has an + * owner. So: terminal status, row kept, and `cleanupStaleData()` still owns + * the removal — an hour of `lastActivity`, which this refreshes. + * + * First terminal outcome wins. `completed` and `failed` are two readings of + * the same task, not two stages of it, so a late `failed` must not overwrite a + * `completed`. `terminated` does outrank both: it is the escalation path, and + * it is what `terminateAgent` would have written. + */ + settleAgent(agentId: string, status: 'completed' | 'failed'): void { + const agent = this.agents.get(agentId) + // Unknown agent: nothing to settle. Callers pass an id they were handed, but + // a terminated agent is legitimately gone by the time a spawn tool unwinds. + if (!agent) return + if (agent.status === 'terminated') return + if (agent.status === 'completed' || agent.status === 'failed') return + agent.status = status + // Without this the row is evicted a full hour after it was SPAWNED, which + // for a long run removes rows that only just finished. + agent.lastActivity = new Date() + this.notifyStateChange() + } + async terminateAgent(agentId: string): Promise { const agent = this.agents.get(agentId) if (agent) { diff --git a/test/spawn-subagent-tool.test.ts b/test/spawn-subagent-tool.test.ts index d2f94f3..9582c63 100644 --- a/test/spawn-subagent-tool.test.ts +++ b/test/spawn-subagent-tool.test.ts @@ -376,3 +376,171 @@ describe('nexus.spawn executor — single task delivery', () => { expect(ctx.session.wait).toHaveBeenCalledWith({ sessionID: CHILD_SESSION_ID }) }) }) + +/** + * Issue #100, at the level it shipped: a spawn tool that gives up leaves the + * agent it spawned sitting at `working` in the published state forever. + * + * The tools mark the agent `working` as soon as `spawnAgent` returns, and the + * only writer of a terminal status on that path was the tool body's own success + * branch. Every other exit — and the ordinary one is a `session.wait` that + * REJECTS rather than timing out, which is what an aborted or evicted child + * session does — returned "Failed to spawn agent" / "Delegate failed" to the + * caller and dropped out with the row still `working`. The session row agreed, + * because `sessionStateOfAgent` maps `working` to `running`: one dead session, + * reported as spending in both views, with no way for a reader to tell. + * + * These read the state through the `dashboard` tool, which is `getState()` + * verbatim — the same payload `/api/state` serves. + */ +describe('a spawn tool that gives up (issue #100)', () => { + /** The published state, via the tool that serves `getState()` verbatim. */ + async function state(tools: Map): Promise { + const dashboard = tools.get('dashboard') + expect(dashboard).toBeDefined() + return JSON.parse((await dashboard.execute({})).content) + } + + /** + * Boot the plugin with a `session.wait` that rejects — the way a real child + * session does when it is aborted, evicted, or gone before it went idle. + */ + async function bootWithRejectingWait() { + const booted = await bootPlugin({ withSubagentTool: true }) + booted.ctx.session.wait = mock(() => + Promise.reject(new Error('SessionNotFoundError: ses_child_123'))) + return booted + } + + it('does not leave the spawn tool\'s agent working', async () => { + const { ctx, tools } = await bootWithRejectingWait() + + const result = await tools.get('spawn').execute( + { role: 'explorer', task: 'Find the leak', model: 'opencode/test-model', wait: true, timeout: 5000 }, + { sessionID: 'ses_parent', agent: 'nexus-orchestrator', messageID: 'msg_1', id: 'call_1' }, + ) + + // The tool still reports rather than throwing at the caller, and it reports + // through its own partial-result branch — a rejected poll reaches that + // branch, not the outer `catch`, so the copy is deliberately not asserted + // here. What is under test is what it left in the STATE. + expect(result.content).toContain(CHILD_SESSION_ID) + + const s = await state(tools) + // THE ASSERTION. Pre-fix this row read `working`, and the session row + // below read `running`, for the rest of the process. + expect(s.agents).toHaveLength(1) + expect(s.agents[0].status).not.toBe('working') + expect(s.agents[0].status).toBe('failed') + // And the session view, which reads the same field, cannot disagree. + expect(s.sessions).toHaveLength(1) + expect(s.sessions[0].state).not.toBe('running') + }) + + it('does not leave the delegate tool\'s agent working', async () => { + const { tools } = await bootWithRejectingWait() + + const result = await tools.get('delegate').execute( + { role: 'reviewer', task: 'Review the parser', model: 'opencode/test-model', timeout: 5000 }, + { sessionID: 'ses_parent', agent: 'nexus-orchestrator', messageID: 'msg_2', id: 'call_2' }, + ) + + expect(result.content).toContain('Delegate failed') + + const s = await state(tools) + expect(s.agents).toHaveLength(1) + expect(s.agents[0].status).not.toBe('working') + expect(s.sessions[0].state).not.toBe('running') + }) + + it('keeps the agent row, because a settled agent is not an orphan', async () => { + const { tools } = await bootWithRejectingWait() + + await tools.get('delegate').execute( + { role: 'reviewer', task: 'Review the parser', model: 'opencode/test-model', timeout: 5000 }, + { sessionID: 'ses_parent', agent: 'nexus-orchestrator', messageID: 'msg_2', id: 'call_2' }, + ) + + const s = await state(tools) + // The row SURVIVES, owned. Deleting the agent here — the way + // `terminateAgent` does — would publish the session as an `owned: false` + // orphan, which is reserved for a session still generating after its agent + // was escalated away. This one is finished; it is a different fact. + expect(s.agents).toHaveLength(1) + expect(s.sessions[0].owned).toBe(true) + expect(s.sessions[0].agentId).toBe(s.agents[0].id) + expect(s.sessions[0].state).toBe('settled') + }) + + it('settles when the spawn tool fails AFTER the spawn, not only on a dead poll', async () => { + // The other half of the same leak, on a different exit. A rejected `wait` + // is caught by the tool's own partial-result branch; a throw from anything + // after that — a failing `storage.set` here, a throw from a read — reaches + // the tool's OUTER `catch`, which used to return "Failed to spawn agent" + // with the row still at `working`. Two distinct exits, one invariant, so + // both are driven. + const { ctx, tools } = await bootPlugin({ withSubagentTool: true }) + const healthySet = ctx.storage.set + ctx.storage.set = mock(() => Promise.reject(new Error('storage is down'))) + + const result = await tools.get('spawn').execute( + { role: 'explorer', task: 'Find the leak', model: 'opencode/test-model', wait: true, timeout: 5000 }, + { sessionID: 'ses_parent', agent: 'nexus-orchestrator', messageID: 'msg_1', id: 'call_1' }, + ) + + expect(result.content).toContain('Failed to spawn agent') + // Restored before reading: the `dashboard` tool persists the state it + // returns through the same `storage.set`, and a read that fails is not a + // measurement. + ctx.storage.set = healthySet + const s = await state(tools) + expect(s.agents[0].status).not.toBe('working') + expect(s.sessions[0].state).not.toBe('running') + }) + + it('settles a dead poll as completed when the session did produce output', async () => { + // The other side of the dead-poll branch, and the reason the settle is at + // each exit rather than on entry to the catch. A rejected poll says nothing + // about whether the child finished; the session's own messages do. When + // they carry an answer, 'failed' would be the wrong terminal status for a + // task that produced its result — and settling to 'failed' on entry would + // have made that permanent, since the first terminal outcome wins. + const { ctx, tools } = await bootWithRejectingWait() + ctx.session.context = mock(() => Promise.resolve([ + { type: 'assistant', content: [{ type: 'text', text: 'the answer' }] }, + ])) + + const result = await tools.get('spawn').execute( + { role: 'explorer', task: 'Find the leak', model: 'opencode/test-model', wait: true, timeout: 5000 }, + { sessionID: 'ses_parent', agent: 'nexus-orchestrator', messageID: 'msg_1', id: 'call_1' }, + ) + expect(result.content).toContain('the answer') + + const s = await state(tools) + expect(s.agents[0].status).toBe('completed') + expect(s.sessions[0].state).toBe('settled') + }) + + it('publishes no session row with a missing id or state, on any path', async () => { + // The second half of #100, asserted over the whole collection rather than + // one row: `sessions[]` is the one list a reader may treat as fully + // populated, so an empty `id` or an absent `state` anywhere in it is a + // defect even when every other field on that row is fine. + for (const wait of [true, false]) { + const { tools } = await bootWithRejectingWait() + await tools.get('spawn').execute( + { role: 'explorer', task: 'Find the leak', model: 'opencode/test-model', wait, timeout: 5000 }, + { sessionID: 'ses_parent', agent: 'nexus-orchestrator', messageID: 'msg_1', id: 'call_1' }, + ) + + const s = await state(tools) + for (const row of s.sessions) { + expect(typeof row.id).toBe('string') + expect(row.id.length).toBeGreaterThan(0) + expect(row.state).not.toBeNull() + expect(row.state).not.toBeUndefined() + expect(['running', 'idle', 'abandoned', 'settled']).toContain(row.state) + } + } + }) +}) diff --git a/test/state-sessions.test.ts b/test/state-sessions.test.ts index e3c0c14..459aadd 100644 --- a/test/state-sessions.test.ts +++ b/test/state-sessions.test.ts @@ -346,6 +346,210 @@ describe('getState().sessions', () => { }) }) +/** + * A finished agent, and what `getState()` says about it afterwards. + * + * The bug this pins: nothing settled the agent. `working` is written by the + * spawn tools and by `executeTask`, and every exit that was not one of the + * tools' own success branches — a `session.wait` that REJECTS rather than + * timing out, a throw from any later read — returned to the caller and left + * the row at `working` for the life of the process. So `getState().agents` + * reported a dead agent as busy, and because `sessionStateOfAgent` maps + * `working` to `running`, the SESSION row agreed with it: one session, in two + * views, both claiming spend that had stopped. + */ +describe('a finished agent', () => { + /** Spawn, mark it working the way a spawn tool does, then let it settle. */ + async function spawnThenSettle(status: 'completed' | 'failed') { + const orchestrator = new NexusOrchestrator() + await orchestrator.initialize(makeCtx() as never) + const agent = await orchestrator.spawnAgent({ role: 'coder', model: MODEL }, { task: 't' }) + agent.status = 'working' + orchestrator.settleAgent(agent.id, status) + return { orchestrator, agent } + } + + it('is never published as `working` again once its outcome is known', async () => { + const { orchestrator } = await spawnThenSettle('completed') + + const state = orchestrator.getState() + expect(state.agents[0]?.status).toBe('completed') + // The SESSION view reads the same field, so it cannot disagree with the + // agent view. 'settled', not 'idle' and above all not 'running'. + expect(state.sessions[0]?.state).toBe('settled') + + await orchestrator.shutdown() + }) + + it('keeps its row, and its ownership, rather than dropping it from the map', async () => { + const { orchestrator, agent } = await spawnThenSettle('completed') + + // The row SURVIVES the settle, and that is the point: removing the agent + // from `this.agents` would take its session row with it, and the only + // reason a session outlives its agent — the orphan `SessionStateView` exists + // to make visible — is `terminateAgent` dropping a session that is STILL + // RUNNING. A settled agent is not that case, and deleting it here would + // publish a fabricated `agentId: null` orphan. + const state = orchestrator.getState() + expect(state.agents.map(a => a.id)).toEqual([agent.id]) + expect(state.sessions).toHaveLength(1) + // Still owned: the owner exists, so the orphan fields stay null only where + // they mean "nobody owns this". + expect(state.sessions[0]?.owned).toBe(true) + expect(state.sessions[0]?.agentId).toBe(agent.id) + expect(state.sessions[0]?.role).toBe('coder') + + await orchestrator.shutdown() + }) + + it('reports a failure as terminal too, not as an agent still going', async () => { + const { orchestrator } = await spawnThenSettle('failed') + + const state = orchestrator.getState() + expect(state.agents[0]?.status).toBe('failed') + expect(state.sessions[0]?.state).toBe('settled') + + await orchestrator.shutdown() + }) + + it('keeps the FIRST terminal outcome, and lets termination outrank it', async () => { + const { orchestrator, agent } = await spawnThenSettle('completed') + + // A late `failed` is a second reading of the same task, not a later stage + // of it, so it must not overwrite a completion. + orchestrator.settleAgent(agent.id, 'failed') + expect(orchestrator.getState().agents[0]?.status).toBe('completed') + + // `terminated` is the escalation path and is stronger than either. + agent.status = 'working' + orchestrator.settleAgent(agent.id, 'completed') + agent.status = 'working' + orchestrator.settleAgent(agent.id, 'failed') + agent.status = 'working' + await orchestrator.terminateAgent(agent.id) + + // `terminateAgent` deletes, which is its own documented behaviour: the + // session survives as the orphan the view exists for. The settle is not + // what removed it, and it must not have left a settled row behind to + // contradict that. + expect(orchestrator.getState().agents).toEqual([]) + expect(orchestrator.getState().sessions).toEqual([]) + + // And a settle that arrives after the agent is gone is a no-op, not a + // resurrection: a spawn tool unwinding after a termination must not + // re-insert the row it was about to settle. + expect(() => orchestrator.settleAgent(agent.id, 'failed')).not.toThrow() + expect(orchestrator.getState().agents).toEqual([]) + + await orchestrator.shutdown() + }) + + it('refreshes lastActivity, so a settled row is not evicted an hour after it SPAWNED', async () => { + const orchestrator = new NexusOrchestrator() + await orchestrator.initialize(makeCtx() as never) + const agent = await orchestrator.spawnAgent({ role: 'coder', model: MODEL }, { task: 't' }) + // An hour and a half ago, so the stale-cleanup sweep would drop the row + // today. This is what `cleanupStaleData` keys on. + agent.lastActivity = new Date(Date.now() - 90 * 60 * 1000) + + agent.status = 'working' + orchestrator.settleAgent(agent.id, 'completed') + orchestrator['cleanupStaleData']() + + expect(orchestrator.getState().agents.map(a => a.id)).toEqual([agent.id]) + + await orchestrator.shutdown() + }) +}) + +/** + * `sessions[]` is the one collection a reader is entitled to treat as fully + * populated, so these pin the whole-shape invariant rather than one row: every + * row names a session and states something about it. + * + * Pass 1 of `sessionViews()` already skipped an agent with no session. Passes 2 + * and 3 — the collection records, keyed by session id — did not, and a record + * with an empty id built a row whose `id` was empty: a row that names no + * session. It is not reachable from a real ledger (one is armed only for an + * agent that has a session), which is exactly why it needed asserting rather + * than assuming. + */ +describe('the shape of sessions[]', () => { + /** Assert the whole-collection invariant, so a future row type cannot skip it. */ + function expectNoHoles(state: OrchestratorState): void { + for (const row of state.sessions) { + expect(typeof row.id).toBe('string') + expect(row.id.length).toBeGreaterThan(0) + expect(row.state === null || row.state === undefined).toBe(false) + expect(['running', 'idle', 'abandoned', 'settled']).toContain(row.state) + } + } + + it('never carries a row with an empty id, from an agent or from a collection record', async () => { + const orchestrator = new NexusOrchestrator() + await orchestrator.initialize(makeCtx() as never) + const agent = await orchestrator.spawnAgent({ role: 'coder', model: MODEL }, { task: 't' }) + + // A real row exists first — the invariant is not vacuous on an empty list. + expect(orchestrator.getState().sessions).toHaveLength(1) + expectNoHoles(orchestrator.getState()) + + // Pass 1: an agent whose session never resolved. Already guarded, and + // asserted so the two passes cannot drift apart. + agent.sessionID = '' + expect(orchestrator.getState().sessions).toEqual([]) + + // Pass 3: an abandoned record keyed by nothing. Before the guard this built + // `{ id: '', state: 'abandoned' }` — a row naming no session, published + // with a spend figure attached to it. + orchestrator['uncollected'].set('', { + sessionID: '', + taskId: 'node-x', + agentId: agent.id, + model: MODEL, + lastKnownTokens: 1000, + observedUncollected: 0.003, + } as never) + + expect(orchestrator.getState().sessions).toEqual([]) + expectNoHoles(orchestrator.getState()) + + // And a well-formed sibling of that record is still published: the guard + // drops a row with no key, not every row that arrives in the same pass. + orchestrator['uncollected'].set('ses_ok', { + sessionID: 'ses_ok', + taskId: 'node-y', + agentId: agent.id, + model: MODEL, + lastKnownTokens: 1000, + observedUncollected: 0.003, + } as never) + const rows = orchestrator.getState().sessions + expect(rows.map(r => r.id)).toEqual(['ses_ok']) + expectNoHoles(orchestrator.getState()) + + await orchestrator.shutdown() + }) + + it('holds for every row a finished task leaves behind, not just the first', async () => { + const { orchestrator } = await runTask(makeCtx()) + orchestrator['uncollected'].set('ses_second', { + sessionID: 'ses_second', + taskId: 'node-2', + agentId: 'agent-gone', + model: MODEL, + lastKnownTokens: 2000, + observedUncollected: 0.006, + } as never) + + const state = orchestrator.getState() + expect(state.sessions).toHaveLength(2) + expectNoHoles(state) + + await orchestrator.shutdown() + }) +}) + /** * Time a task out, terminate its agent, and return the live orchestrator. * From eb001a5970c173cc6c58fec8d2f854220c3dfde2 Mon Sep 17 00:00:00 2001 From: serkanalgur Date: Mon, 28 Sep 2026 11:32:06 +0400 Subject: [PATCH 2/2] test: assert the dashboard markup is inlined in one bundle, not two MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `parseWebDashboardTarget` and `parseDashboardTarget` are two functions because `src/dashboard.ts:11` inlines `dashboard/index.html` with a text import. Import it from `src/tui.tsx` and ~170 KB of page markup lands in `dist/tui.js`, which serves no dashboard and reads none of it. Both sites carry a comment saying so, and the parser test above already keeps the two functions in agreement. Neither of those holds the BUNDLES apart. A comment is not a test, and a bundle test is the only place the constraint is observable, so a reader who reads the duplication as an oversight merges it and the cost lands at build time where nothing fails. Three assertions on the built artifacts: the page is inlined into `dist/index.js`, none of its markup appears in `dist/tui.js`, and the TUI bundle is smaller than the page itself. The third is what keeps the first two from being a coincidence about four strings — 72 KB against 170 KB, where a bundle that had inlined the markup would be about 240 KB. Mutation-checked: appending the page to `dist/tui.js` fails both the markup and the size test; removing it from `dist/index.js` fails the server-side one. Reads dist/ when it is present and returns early when it is not, so a source checkout that has not been built does not fail on an absent artifact. --- test/dashboard-entrypoints.test.ts | 52 +++++++++++++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-) diff --git a/test/dashboard-entrypoints.test.ts b/test/dashboard-entrypoints.test.ts index 28e8478..28d7618 100644 --- a/test/dashboard-entrypoints.test.ts +++ b/test/dashboard-entrypoints.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, afterEach, mock } from 'bun:test' -import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs' +import { existsSync, mkdtempSync, mkdirSync, readFileSync, rmSync, statSync, writeFileSync } from 'node:fs' import { join, resolve } from 'node:path' import { tmpdir } from 'node:os' import * as realOs from 'node:os' @@ -927,6 +927,56 @@ describe('dashboard entry points', () => { }) }) + // The parser test above keeps the two functions in agreement. It cannot + // keep them in separate BUNDLES, which is the constraint that made them two + // functions in the first place: `src/dashboard.ts:11` inlines + // `dashboard/index.html` with a text import, so importing that module from + // `src/tui.tsx` would pull ~170 KB of page markup into `dist/tui.js`, which + // serves no dashboard and reads none of it. The comments at both sites say + // so, and a comment is not a test — the next reader who sees the duplication + // merges them, and the cost lands at build time where nothing fails. + describe('the built bundles keep the dashboard markup in one of them', () => { + // Strings taken from `dashboard/index.html`. Short and specific enough that + // a coincidence in application code is not the reason a bundle matches, + // and from the head of the file so a later edit to the body cannot quietly + // make this pass for the wrong reason. + const MARKUP = ['--surface-hover', '0d1117', '161b22', '30363d'] as const + + const dist = (file: string): string | null => { + const path = join(import.meta.dir, '..', 'dist', file) + return existsSync(path) ? readFileSync(path, 'utf-8') : null + } + + it('inlines the dashboard page into the server bundle, which serves it', () => { + const server = dist('index.js') + if (server === null) return // a source checkout with no build; see below + for (const mark of MARKUP) expect(server).toContain(mark) + }) + + it('keeps every byte of that markup out of the TUI bundle', () => { + const tui = dist('tui.js') + if (tui === null) return + for (const mark of MARKUP) { + expect(tui).not.toContain(mark) + } + }) + + it('and the TUI bundle is nowhere near large enough to be carrying it', () => { + // The size claim is what makes the two tests above a real assertion + // rather than a coincidence about four strings. `dist/tui.js` is ~72 KB + // and the page is ~170 KB, so a bundle that had inlined the markup + // would be roughly 240 KB. The bar is set at the sum, not below it, so + // the current bundle keeps a wide margin and a regression is not one + // that has to be timed to notice. + const tui = dist('tui.js') + if (tui === null) return + const page = join(import.meta.dir, '..', 'dashboard', 'index.html') + const pageBytes = existsSync(page) ? statSync(page).size : 0 + expect(pageBytes).toBeGreaterThan(0) + expect(Buffer.byteLength(tui)).toBeLessThan(pageBytes) + }) + }) + describe('probeDashboard', () => { it('distinguishes a dashboard, a squatter and a closed port', async () => { const squatter = Bun.serve({ port: 14994, hostname: '127.0.0.1', fetch: () => new Response('') })