fix: address PR #1802 review round
- listChildren reads the session store via strict ctx.get (property proxy is caller-scoped), orders candidates branchlessly, narrows the cold-read return type, and pins the cost model and store/registry composition gaps with tests; per-file coverage restored - acp-agent and headless-agent compositions mount session-projection; a keyless snapshot pins the descriptor-less diagnostic row - api-proxy cold spec pins header-origin ownership and the legacy descriptor-only opt-out - design note ships as implemented with its English pairing; companion notes and core-data-structures pages synced
This commit is contained in:
@@ -190,6 +190,7 @@ describe('subagent ownership fence', () => {
|
||||
const meta = header('session-child', 1000, {
|
||||
parentSession: sid('session-parent'),
|
||||
seedLength: 0,
|
||||
origin: 'subagent',
|
||||
})
|
||||
const events = [
|
||||
{ type: 'turn/start', seq: 0, time: 1, data: { turn: 1, trigger: { kind: 'message', source: { kind: 'user' } } } },
|
||||
@@ -245,6 +246,47 @@ describe('subagent ownership fence', () => {
|
||||
expect(inspect).toHaveBeenCalledTimes(3)
|
||||
})
|
||||
|
||||
it('no longer treats a descriptor-only cold child without origin as subagent-owned', async () => {
|
||||
const ctx = new Context()
|
||||
await ctx.plugin(SessionStore)
|
||||
await ctx.plugin(AgentRegistry)
|
||||
await ctx.plugin(UserInteractionService)
|
||||
const sessionId = sid('session-legacy-child')
|
||||
const meta = header('session-legacy-child', 1000, {
|
||||
parentSession: sid('session-parent'),
|
||||
seedLength: 0,
|
||||
})
|
||||
const events = [
|
||||
{
|
||||
type: 'subagent/descriptor',
|
||||
seq: 0,
|
||||
time: 1,
|
||||
data: { version: 2, mode: 'continuable', provider: 'spawn', label: 'child' },
|
||||
},
|
||||
] as SessionEvent[]
|
||||
ctx.provide('sessionPersistence', {
|
||||
list: () => Promise.resolve([meta]),
|
||||
inspect: () => Promise.resolve({ meta, events }),
|
||||
locate: () => undefined,
|
||||
} as never)
|
||||
// Pre-#1569 stores classify a child only through the descriptor event and
|
||||
// carry no header `origin`; the pre-release decision stops recognizing
|
||||
// them, so the ownership fence lets generic resume reach the registry
|
||||
// instead of answering `agent-busy`.
|
||||
const resume = vi.spyOn(ctx.agents, 'resume')
|
||||
.mockRejectedValue(new Error('registry unavailable in this bench'))
|
||||
const api = createApiProxy(ctx, { provider: 'p', model: 'm', cwd: '/tmp', workspaceRoot: '/tmp' })
|
||||
|
||||
const prompt = await api.sessions.prompt(request({
|
||||
sessionId,
|
||||
mode: 'queue',
|
||||
content: [{ type: 'text', text: 'follow up' }],
|
||||
}))
|
||||
expect(resume).toHaveBeenCalledTimes(1)
|
||||
expect(prompt.result.ok).toBe(false)
|
||||
if (!prompt.result.ok) expect(prompt.result.error.code).toBe('internal')
|
||||
})
|
||||
|
||||
it('rejects origin-marked and runtime-owned live children from generic controls', async () => {
|
||||
const ctx = new Context()
|
||||
await ctx.plugin(SessionStore)
|
||||
|
||||
@@ -304,8 +304,8 @@ export class SubagentService extends Service {
|
||||
* @param signal - caller-owned cancellation forwarded to persistence reads
|
||||
* and observed around every read await.
|
||||
* @returns children and per-child diagnostics ordered by `createdAt`, then id.
|
||||
* @throws {@link SubagentError} when the projection registry is not mounted
|
||||
* or the caller cancels the listing.
|
||||
* @throws {@link SubagentError} when the projection registry or the session
|
||||
* store is not mounted, or the caller cancels the listing.
|
||||
*/
|
||||
listChildren(parentSessionId: SessionId, signal?: AbortSignal): Promise<SubagentListEntry[]> {
|
||||
return listSubagentChildren(this.ctx, parentSessionId, signal)
|
||||
|
||||
@@ -22,7 +22,11 @@ import type { SessionProjectionRegistry } from '@deepseek-ai/dsh-session-project
|
||||
import { SubagentError } from './error.ts'
|
||||
import type { SubagentIdentityProjection } from './projection-types.ts'
|
||||
|
||||
/** Concurrent cold inspections per listing; a constant because it bounds one read-only scan, not deployment behavior. */
|
||||
/**
|
||||
* Concurrent cold inspections per listing; a constant because it bounds one
|
||||
* read-only scan of local media, not deployment behavior. Should a networked
|
||||
* persistence backend appear, promote it to a validated `Config` field.
|
||||
*/
|
||||
const COLD_READ_CONCURRENCY = 4
|
||||
|
||||
/**
|
||||
@@ -90,8 +94,8 @@ export type SubagentListEntry =
|
||||
* @param parentSessionId - parent session whose direct children are listed.
|
||||
* @param signal - caller-owned cancellation observed around every persistence read.
|
||||
* @returns children and per-child diagnostics ordered by `createdAt`, then id.
|
||||
* @throws {@link SubagentError} when the projection registry is not mounted
|
||||
* or the caller cancels the listing.
|
||||
* @throws {@link SubagentError} when the projection registry or the session
|
||||
* store is not mounted, or the caller cancels the listing.
|
||||
*/
|
||||
export async function listChildren(
|
||||
ctx: Context,
|
||||
@@ -99,7 +103,6 @@ export async function listChildren(
|
||||
signal?: AbortSignal,
|
||||
): Promise<SubagentListEntry[]> {
|
||||
const projections = ctx.get('sessionProjections')
|
||||
const sessions = ctx.get('sessions')
|
||||
// Checked before any read, even with zero candidates: mode/label are the
|
||||
// row's strong contract, so a missing fold capability is a deterministic
|
||||
// deployment configuration error, never an empty success.
|
||||
@@ -109,10 +112,14 @@ export async function listChildren(
|
||||
'SUBAGENT_CONTROL_PROJECTIONS_UNAVAILABLE',
|
||||
)
|
||||
}
|
||||
// Strict global read, never the `ctx.sessions` property proxy: the proxy is
|
||||
// caller-scope bound, so a consumer plugin without its own `sessions`
|
||||
// injection (the model-facing tool, the API proxy) would throw on access.
|
||||
const sessions = ctx.get('sessions')
|
||||
if (sessions === undefined) {
|
||||
throw new SubagentError(
|
||||
'listing subagents requires the sessions registry (load @deepseek-ai/dsh-session)',
|
||||
'SUBAGENT_CONTROL_PROJECTIONS_UNAVAILABLE',
|
||||
'listing subagents requires the session store (load @deepseek-ai/dsh-session)',
|
||||
'SUBAGENT_CONTROL_SESSION_STORE_UNAVAILABLE',
|
||||
)
|
||||
}
|
||||
assertListingNotCancelled(signal)
|
||||
@@ -146,7 +153,7 @@ export async function listChildren(
|
||||
.filter(record => record.header.parentSession === parentSessionId
|
||||
&& record.header.origin === 'subagent')
|
||||
.sort((a, b) => a.header.createdAt - b.header.createdAt
|
||||
|| (a.header.id < b.header.id ? -1 : a.header.id > b.header.id ? 1 : 0))
|
||||
|| a.header.id.localeCompare(b.header.id))
|
||||
|
||||
const rows: (SubagentListEntry | undefined)[] = Array.from({ length: candidates.length })
|
||||
const coldReads: { index: number; id: SessionId }[] = []
|
||||
@@ -196,7 +203,7 @@ async function inspectColdIdentity(
|
||||
childId: SessionId,
|
||||
hasChildren: boolean,
|
||||
signal: AbortSignal | undefined,
|
||||
): Promise<SubagentListEntry | undefined> {
|
||||
): Promise<SubagentListEntry> {
|
||||
assertListingNotCancelled(signal)
|
||||
let events: readonly SessionEvent[]
|
||||
try {
|
||||
|
||||
@@ -139,9 +139,11 @@ ProjectionDefinition<'subagent', IdentityState> = {
|
||||
const identity = descriptorIdentity(event)
|
||||
return identity === undefined ? {} : { identity }
|
||||
},
|
||||
// A no-value log serves `undefined` (the schema's optional side); the map
|
||||
// entry stays non-optional because every consumer reads through `Partial`
|
||||
// snapshot values, where absence is already the type.
|
||||
// The assertion deliberately widens: a log without a descriptor serves
|
||||
// `undefined` at runtime, which the schema's `.optional()` accepts, and
|
||||
// every registry read face already returns `Partial` snapshot values where
|
||||
// absence is the type. The map entry stays non-optional so a child row's
|
||||
// served identity remains a strong contract for consumers.
|
||||
view: state => state.identity as SubagentIdentityProjection,
|
||||
stateVersion: 1,
|
||||
}
|
||||
|
||||
@@ -136,6 +136,15 @@ describe('SubagentService.listChildren', () => {
|
||||
)
|
||||
})
|
||||
|
||||
it('fails loud when the session store is not mounted', async () => {
|
||||
const ctx = new Context()
|
||||
await ctx.plugin(SessionProjectionRegistry)
|
||||
await ctx.plugin(SubagentService)
|
||||
await expect(ctx.subagents.listChildren(SessionId('no-store-parent'))).rejects.toThrow(
|
||||
expect.objectContaining({ code: 'SUBAGENT_CONTROL_SESSION_STORE_UNAVAILABLE' }) as Error,
|
||||
)
|
||||
})
|
||||
|
||||
it('lists a persisted continuable child as inactive with its durable label', async () => {
|
||||
const { ctx, parent } = await setup([textResponse('done')])
|
||||
const childId = await startChild(ctx, parent, 'summarize the doc')
|
||||
@@ -207,31 +216,57 @@ describe('SubagentService.listChildren', () => {
|
||||
|
||||
it('orders children by createdAt then id without listing ordinary forks', async () => {
|
||||
const { ctx, parent } = await setup([])
|
||||
// Authored headers pin the ordering key deterministically: same createdAt
|
||||
// ties break on id, different createdAt orders ascending.
|
||||
const late = await authorChild(ctx, '00000000-0000-4000-8000-000000000003', {
|
||||
parentSession: parent.id,
|
||||
createdAt: 9,
|
||||
origin: 'subagent',
|
||||
}, childEvents(descriptorPayload('late child')))
|
||||
const tieB = await authorChild(ctx, '00000000-0000-4000-8000-000000000002', {
|
||||
parentSession: parent.id,
|
||||
createdAt: 5,
|
||||
origin: 'subagent',
|
||||
}, childEvents(descriptorPayload('tie b')))
|
||||
const tieA = await authorChild(ctx, '00000000-0000-4000-8000-000000000001', {
|
||||
parentSession: parent.id,
|
||||
createdAt: 5,
|
||||
origin: 'subagent',
|
||||
}, childEvents(descriptorPayload('tie a')))
|
||||
/** Publish one live child with a pinned header ordering key. */
|
||||
const liveChild = (parentId: SessionId, id: string, createdAt: number, label: string): SessionId => {
|
||||
const session = ctx.sessions.create(SessionId(id), {
|
||||
meta: { parentSession: parentId, origin: 'subagent', createdAt },
|
||||
})
|
||||
session.append('turn/start', { turn: 1 })
|
||||
session.append('subagent/descriptor', descriptorPayload(label))
|
||||
return session.header.id
|
||||
}
|
||||
// Live creation order is deliberately shuffled against the expected
|
||||
// result: same-createdAt ties break on id, different createdAt orders
|
||||
// ascending.
|
||||
const late = liveChild(parent.id, '00000000-0000-4000-8000-000000000009', 9, 'late child')
|
||||
const tieB = liveChild(parent.id, '00000000-0000-4000-8000-000000000002', 5, 'tie b')
|
||||
const tieA = liveChild(parent.id, '00000000-0000-4000-8000-000000000001', 5, 'tie a')
|
||||
// An ordinary session fork shares parentSession but has no subagent origin.
|
||||
const fork = ctx.sessions.fork(parent.session, undefined, SessionId('plain-fork'))
|
||||
await ctx.sessions.flush(fork)
|
||||
const inspect = vi.spyOn(ctx.sessionPersistence, 'inspect')
|
||||
const entries = await ctx.subagents.listChildren(parent.id)
|
||||
expect(entries.map(entry => entry.id)).toEqual([tieA, tieB, late])
|
||||
expect(entries.every(entry => entry.kind === 'child')).toBe(true)
|
||||
expect(inspect).not.toHaveBeenCalledWith(fork.id, expect.anything())
|
||||
})
|
||||
|
||||
it('omits a live child that has not appended its descriptor yet', async () => {
|
||||
const { ctx, parent } = await setup([])
|
||||
const pending = ctx.sessions.create(SessionId('creation-window-child'), {
|
||||
meta: { parentSession: parent.id, origin: 'subagent' },
|
||||
})
|
||||
pending.append('turn/start', { turn: 1 })
|
||||
// The creation window: the establishing provider has not appended the
|
||||
// descriptor yet, so the row is omitted rather than diagnosed.
|
||||
await expect(ctx.subagents.listChildren(parent.id)).resolves.toEqual([])
|
||||
})
|
||||
|
||||
it('lists a one-shot child with its durable creation label', async () => {
|
||||
const { ctx, parent } = await setup([])
|
||||
const labeled = await authorChild(ctx, '00000000-0000-4000-8000-00000000ab02', {
|
||||
parentSession: parent.id,
|
||||
origin: 'subagent',
|
||||
}, childEvents({
|
||||
version: SUBAGENT_DESCRIPTOR_VERSION,
|
||||
mode: 'one-shot',
|
||||
provider: 'spawn',
|
||||
label: 'labeled one-shot',
|
||||
}))
|
||||
await expect(ctx.subagents.listChildren(parent.id)).resolves.toEqual([
|
||||
{
|
||||
kind: 'child', id: labeled, mode: 'one-shot', label: 'labeled one-shot',
|
||||
activity: 'inactive', hasChildren: false,
|
||||
},
|
||||
])
|
||||
})
|
||||
|
||||
it('reports a live child as running while keeping settled siblings complete', async () => {
|
||||
@@ -462,6 +497,35 @@ describe('SubagentService.listChildren', () => {
|
||||
expect(inspected).not.toContain(grandchildId)
|
||||
})
|
||||
|
||||
it('inspects each cold child exactly once and a live child never', async () => {
|
||||
const { ctx, parent } = await setup([textResponse('done')])
|
||||
const coldStarted = await startChild(ctx, parent, 'cold started child')
|
||||
const coldAuthored = await authorChild(ctx, '00000000-0000-4000-8000-00000000ab01', {
|
||||
parentSession: parent.id,
|
||||
origin: 'subagent',
|
||||
}, childEvents(descriptorPayload('cold authored child')))
|
||||
const liveId = SessionId('live-mixed-child')
|
||||
const live = ctx.sessions.create(liveId, {
|
||||
meta: { parentSession: parent.id, origin: 'subagent' },
|
||||
})
|
||||
live.append('turn/start', { turn: 1 })
|
||||
live.append('subagent/descriptor', descriptorPayload('live mixed child'))
|
||||
|
||||
const inspected: SessionId[] = []
|
||||
const original = ctx.sessionPersistence.inspect.bind(ctx.sessionPersistence)
|
||||
ctx.sessionPersistence.inspect = (sessionId, signal) => {
|
||||
inspected.push(sessionId)
|
||||
return original(sessionId, signal)
|
||||
}
|
||||
const entries = await ctx.subagents.listChildren(parent.id)
|
||||
expect(entries).toHaveLength(3)
|
||||
// The cost model: one inspection per cold child, none for a live child,
|
||||
// whose identity is served from the registry's watermark cache.
|
||||
expect(inspected.filter(id => id === coldStarted)).toHaveLength(1)
|
||||
expect(inspected.filter(id => id === coldAuthored)).toHaveLength(1)
|
||||
expect(inspected).not.toContain(liveId)
|
||||
})
|
||||
|
||||
it('does not count an ordinary grandchild without subagent origin', async () => {
|
||||
const { ctx, parent } = await setup([textResponse('done')])
|
||||
const childId = await startChild(ctx, parent, 'direct child')
|
||||
|
||||
@@ -46,6 +46,7 @@
|
||||
"@deepseek-ai/dsh-session": "workspace:^",
|
||||
"@deepseek-ai/dsh-session-persistence": "workspace:^",
|
||||
"@deepseek-ai/dsh-session-persistence-jsonl": "workspace:^",
|
||||
"@deepseek-ai/dsh-session-projection": "workspace:^",
|
||||
"@deepseek-ai/dsh-subagent": "workspace:^",
|
||||
"@deepseek-ai/dsh-subagent-spawn": "workspace:^",
|
||||
"@deepseek-ai/dsh-tools": "workspace:^",
|
||||
|
||||
@@ -8,6 +8,7 @@ import AgentLoop from '@deepseek-ai/dsh-agent-loop'
|
||||
import { mountAgentLoopTestDependencies } from '@deepseek-ai/dsh-agent-loop-testkit'
|
||||
import { SessionId } from '@deepseek-ai/dsh-session'
|
||||
import JsonlSessionPersistence from '@deepseek-ai/dsh-session-persistence-jsonl'
|
||||
import SessionProjectionRegistry from '@deepseek-ai/dsh-session-projection'
|
||||
import SubagentService from '@deepseek-ai/dsh-subagent'
|
||||
import type { SubagentListEntry } from '@deepseek-ai/dsh-subagent'
|
||||
import * as SubagentSpawn from '@deepseek-ai/dsh-subagent-spawn'
|
||||
@@ -28,6 +29,7 @@ async function setup(script: ConstructorParameters<typeof MockAdapter>[0]) {
|
||||
roots.push(root)
|
||||
await ctx.plugin(JsonlSessionPersistence, { root })
|
||||
await ctx.plugin(AgentLoop, { agents: [] })
|
||||
await ctx.plugin(SessionProjectionRegistry)
|
||||
await ctx.plugin(SubagentService)
|
||||
await ctx.plugin(SubagentSpawn, { providerName: 'spawn' })
|
||||
await ctx.plugin(tool)
|
||||
|
||||
@@ -8,6 +8,7 @@ import AgentLoop from '@deepseek-ai/dsh-agent-loop'
|
||||
import { mountAgentLoopTestDependencies } from '@deepseek-ai/dsh-agent-loop-testkit'
|
||||
import { SessionId } from '@deepseek-ai/dsh-session'
|
||||
import JsonlSessionPersistence from '@deepseek-ai/dsh-session-persistence-jsonl'
|
||||
import SessionProjectionRegistry from '@deepseek-ai/dsh-session-projection'
|
||||
import SubagentService from '@deepseek-ai/dsh-subagent'
|
||||
import * as SubagentSpawn from '@deepseek-ai/dsh-subagent-spawn'
|
||||
import { MockAdapter, textResponse } from '../../../core/agent-loop/tests/mock-adapter.ts'
|
||||
@@ -27,6 +28,7 @@ async function setup(script: ConstructorParameters<typeof MockAdapter>[0]) {
|
||||
roots.push(root)
|
||||
await ctx.plugin(JsonlSessionPersistence, { root })
|
||||
await ctx.plugin(AgentLoop, { agents: [] })
|
||||
await ctx.plugin(SessionProjectionRegistry)
|
||||
await ctx.plugin(SubagentService)
|
||||
await ctx.plugin(SubagentSpawn, { providerName: 'spawn' })
|
||||
await ctx.plugin(tool)
|
||||
|
||||
Reference in New Issue
Block a user