1
0
Эх сурвалжийг харах

fix: address ds-review-bot findings on the projection migration

- llm-retry: validate config before registering the projection unit;
  document the branded-retry-id zod cast; start stateVersion at 1.
- agent-loop: register turnBoundary only after every config validation,
  so a rejected constructor leaves no unit behind; the defensive-cap
  test no longer needs fiber cleanup.
- agent-instructions: keep newest-first per-scope change history so the
  latest visible change survives a surface replacement shadowing the
  newest one (restores the previous scan-visible semantics); add a
  regression test for the delete-after-shadow sequence.
- tool-skill: keep catalog-message history so a shadowed newest catalog
  message still falls back to the latest visible digest.
- session-query-sqlite: drop the unused required sessionProjections
  injection.
- plan-mode: restore the command/done error-drop regression test and the
  cold-replay command/done fold; drop the inaccurate state-reference
  comment.
- tool-todo: remove a stray blank line; document the turnBoundary
  reader contract on the projection type.
_Kerman 2 сар өмнө
parent
commit
f364d6ba37

+ 14 - 8
packages/context/agent-instructions/src/state.ts

@@ -136,7 +136,7 @@ function sameInstructionChange(a: AgentInstructionChange, b: AgentInstructionCha
     && a.digest === b.digest
 }
 
-const workspaceInstructionsStateSchema = zod.record(zod.string(), zod.object({
+const workspaceInstructionsStateSchema = zod.record(zod.string(), zod.array(zod.object({
   change: zod.object({
     action: zod.enum(['set', 'replace', 'remove']),
     scope: zod.string(),
@@ -144,14 +144,14 @@ const workspaceInstructionsStateSchema = zod.record(zod.string(), zod.object({
     digest: zod.string().optional(),
   }),
   seq: zod.number().int().nonnegative(),
-}))
+})))
 
-/** Latest workspace-instruction change by scope. */
+/** Newest-first workspace-instruction change history by scope. */
 export type WorkspaceInstructionsState = zod.infer<typeof workspaceInstructionsStateSchema>
 
 declare module '@deepseek-ai/dsh-session-projection/types' {
   interface SessionProjectionStateMap {
-    /** Latest workspace-instruction change by scope. */
+    /** Newest-first workspace-instruction change history by scope. */
     workspaceInstructions: WorkspaceInstructionsState
   }
 }
@@ -171,11 +171,13 @@ export function createWorkspaceInstructionsProjection(): ProjectionDefinition<'w
       if (changes.length === 0) return state
       let next = state
       for (const change of changes) {
-        next = { ...next, [change.scope]: { change, seq: event.seq } }
+        const record = { change, seq: event.seq }
+        const history = next[change.scope]
+        next = { ...next, [change.scope]: history === undefined ? [record] : [record, ...history] }
       }
       return next
     },
-    stateVersion: 1,
+    stateVersion: 2,
   }
 }
 
@@ -187,8 +189,12 @@ function visibleInstructionChanges(
   const visible = new Map<string, AgentInstructionChange>()
   const folded = agent.ctx.get('sessionProjections')?.stateOf(agent.session, 'workspaceInstructions')
   if (folded === undefined) throw new Error('workspaceInstructions projection is not registered')
-  for (const [scope, record] of Object.entries(folded)) {
-    if (visibleSeqs.has(record.seq)) visible.set(scope, record.change)
+  for (const [scope, history] of Object.entries(folded)) {
+    // History is newest-first; the latest visible record restores the previous
+    // scan-visible semantics when a surface replacement shadows the newest
+    // change but an older one stays visible.
+    const latestVisible = history.find(record => visibleSeqs.has(record.seq))
+    if (latestVisible !== undefined) visible.set(scope, latestVisible.change)
   }
   for (const message of authorityMessages) {
     if (!isWorkspaceContextSource(message.source)) continue

+ 66 - 0
packages/context/agent-instructions/tests/agent-instructions.spec.ts

@@ -3626,6 +3626,72 @@ describe('dynamic nested workspace context injection', () => {
     }
   })
 
+  it('still removes a deleted instruction whose latest update is shadowed while an older one stays visible', async () => {
+    const root = await tempRepo()
+    const home = await tempRepo()
+    try {
+      await mkdir(join(root, '.git'), { recursive: true })
+      await write(join(root, 'pkg/AGENTS.md'), 'first nested rule')
+      await write(join(root, 'pkg/deep/file.txt'), 'hello')
+      const ctx = new Context()
+      await mountFileToolsAndWorkspaceContext(ctx, { dshHome: home, maxBytes: 65536 })
+      const agent = stubAgent(root)
+
+      const first = await ctx.tools.execute({
+        signal: testToolSignal,
+        callId: CallId('read-before-update'),
+        name: 'read',
+        arguments: { file_path: join('pkg', 'deep', 'file.txt') },
+        agent,
+      })
+      const firstSeq = (await appendAdditionalContexts(ctx, agent))!
+
+      await write(join(root, 'pkg/AGENTS.md'), 'second nested rule')
+      const second = await ctx.tools.execute({
+        signal: testToolSignal,
+        callId: CallId('read-after-update'),
+        name: 'read',
+        arguments: { file_path: join('pkg', 'deep', 'file.txt') },
+        agent,
+      })
+      const secondSeq = (await appendAdditionalContexts(ctx, agent))!
+      expect(agent.session.surface.nodes).toContain(firstSeq)
+      expect(agent.session.surface.nodes).toContain(secondSeq)
+
+      // A surface replacement shadows only the latest update; the older one
+      // stays visible, so the scope still has visible state to lose.
+      agent.session.append('user/message', createUserMessage({
+        content: [{ type: 'text', text: 'compacted summary' }],
+        source: { kind: 'plugin', plugin: 'compact' },
+      }), {
+        surfaceOp: { op: 'replace', start: secondSeq, end: secondSeq },
+        sourceEventSeqs: [secondSeq],
+      })
+
+      await rm(join(root, 'pkg/AGENTS.md'))
+      await ctx.tools.execute({
+        signal: testToolSignal,
+        callId: CallId('read-after-delete'),
+        name: 'read',
+        arguments: { file_path: join('pkg', 'deep', 'file.txt') },
+        agent,
+      })
+      await appendAdditionalContexts(ctx, agent)
+
+      expect(first.additionalContexts).toBeUndefined()
+      expect(second.additionalContexts).toBeUndefined()
+      const removal = agent.session.events.find(event => event.type === 'user/message'
+        && event.data.source.kind === 'agent-instructions'
+        && event.data.source.changes.some(change => change.action === 'remove'))
+      expect(removal?.type === 'user/message' ? removal.data.source : undefined).toMatchObject({
+        changes: [{ action: 'remove', scope: sk('pkg', 'AGENTS.md'), path: join('pkg', 'AGENTS.md') }],
+      })
+    } finally {
+      await rm(root, { recursive: true, force: true })
+      await rm(home, { recursive: true, force: true })
+    }
+  })
+
   it('re-arms an unchanged baseline after compaction removes it from the surface', async () => {
     const root = await tempRepo()
     const home = await tempRepo()

+ 3 - 2
packages/core/agent-loop/src/index.ts

@@ -378,8 +378,6 @@ export class AgentLoop extends Service implements AgentFactory {
   constructor(ctx: Context, config: Config) {
     super(ctx, 'agentLoop')
 
-    ctx.sessionProjections.register(turnBoundaryProjectionDefinition)
-
     const entry: AgentLoopSettings = {
       maxParallelToolCalls: resolveMaxParallelToolCalls(config.maxParallelToolCalls),
     }
@@ -406,6 +404,9 @@ export class AgentLoop extends Service implements AgentFactory {
       onChange: () => {},
     })
     validateConfiguredAgents(this.config.agents)
+    // Register only after every config validation above has passed, so a
+    // rejected constructor leaves no projection unit behind.
+    ctx.sessionProjections.register(turnBoundaryProjectionDefinition)
     this.ownership = new FactoryOwnership(ctx.fiber)
     this.runtime = { ctx }
     ctx.effect(() => () => this.ownership.dispose(), 'agentLoop.transactions()')

+ 5 - 9
packages/core/agent-loop/tests/tool-calls.spec.ts

@@ -269,17 +269,13 @@ describe('tool-call scheduler: rolling pool honors maxParallelToolCalls', () =>
     await expect(harness(new MockAdapter([]), 1.5)).rejects.toThrow()
   })
 
-  it('defensively rejects invalid caps when direct construction bypasses the config schema', async () => {
-    const ctx = new Context()
-    await ctx.plugin(SessionProjectionRegistry)
-    expect(() => new AgentLoop(ctx, { agents: [], maxParallelToolCalls: 0 }))
+  it('defensively rejects invalid caps when direct construction bypasses the config schema', () => {
+    // Validation precedes the turnBoundary registration, so a rejected
+    // constructor registers nothing and needs no fiber cleanup.
+    expect(() => new AgentLoop(new Context(), { agents: [], maxParallelToolCalls: 0 }))
       .toThrow('maxParallelToolCalls must be a positive integer')
-    await ctx.fiber.dispose()
-    const other = new Context()
-    await other.plugin(SessionProjectionRegistry)
-    expect(() => new AgentLoop(other, { agents: [], maxParallelToolCalls: 1.5 }))
+    expect(() => new AgentLoop(new Context(), { agents: [], maxParallelToolCalls: 1.5 }))
       .toThrow('maxParallelToolCalls must be a positive integer')
-    await other.fiber.dispose()
   })
 
   it('defaults the cap when direct construction bypasses the config schema', async () => {

+ 8 - 1
packages/core/agent/src/types.ts

@@ -9,7 +9,14 @@ import type { UserMessage } from '@deepseek-ai/dsh-llm/types'
 /** One of the two ordered pending-message lists owned by an agent. */
 export type InboxTarget = 'next-turn' | 'next-step'
 
-/** Turn and step boundaries folded from one agent session log. */
+/**
+ * Turn and step boundaries folded from one agent session log.
+ *
+ * Reader contract: the key is registered by `dsh-agent-loop` and absent
+ * otherwise. Without agent-loop no turn events exist, so readers treat an
+ * absent key as "no open turn / no boundaries" — capability absence, not a
+ * corrupt state — and never treat it as an error.
+ */
 export interface TurnBoundaryProjection {
   /** Open turn number, or null between turns. */
   readonly openTurn: number | null

+ 3 - 2
packages/llm/llm-retry/src/index.ts

@@ -108,6 +108,7 @@ interface RetryStateEntry {
 
 type LlmRetryState = Record<string, RetryStateEntry>
 
+// Zod cannot express the branded retry id without a runtime transform.
 const llmRetryStateSchema: zod.ZodType<LlmRetryState> = zod.record(zod.string(), zod.object({
   retry: zod.number().int().nonnegative(),
   retryId: zod.string(),
@@ -120,9 +121,10 @@ declare module '@deepseek-ai/dsh-session-projection/types' {
 }
 
 export function apply(ctx: Context, config: Config = {}, internals: RetryInternals = {}): void {
+  validateConfig(config)
   ctx.sessionProjections.register({
     key: 'llmRetry',
-    stateVersion: 2,
+    stateVersion: 1,
     stateSchema: llmRetryStateSchema,
     init: () => ({}),
     apply: (state, event) => {
@@ -134,7 +136,6 @@ export function apply(ctx: Context, config: Config = {}, internals: RetryInterna
       return { ...state, [key]: { retry: event.data.retry, retryId: event.data.retryId } }
     },
   })
-  validateConfig(config)
   const random = internals.random ?? Math.random
   const lifetime = new AbortController()
   const active = new Set<Promise<RequestErrorAction>>()

+ 21 - 6
packages/plan/plan-mode/tests/projection.spec.ts

@@ -38,13 +38,20 @@ async function harness(withPlanMode: boolean): Promise<Bench> {
 }
 
 /** Append one logged /plan selection record (the executor's command/run shape). */
-function runPlanCommand(session: Session, args: string, index: number): void {
+function runPlanCommand(session: Session, args: string, index: number): CommandId {
+  const commandId = CommandId(`plan-proj-${String(index)}`)
   session.append('command/run', {
-    commandId: CommandId(`plan-proj-${String(index)}`),
+    commandId,
     name: 'plan',
     args,
     source: { kind: 'user' },
   })
+  return commandId
+}
+
+/** Append the paired settlement for one projected plan command. */
+function settlePlanCommand(session: Session, commandId: CommandId, kind: 'success' | 'error'): void {
+  session.append('command/done', { commandId, kind })
 }
 
 /** Commit one plan/mode flip inside an open turn (the invariant's turn-enclosure rule). */
@@ -62,15 +69,23 @@ describe('plan projection unit', () => {
 
   it('a logged /plan selection reads pending until plan/mode records it', async () => {
     const bench = await harness(true)
-    runPlanCommand(bench.session, '', 0)
+    const commandId = runPlanCommand(bench.session, '', 0)
     expect(bench.values().plan).toEqual({ active: false, pending: true })
-    // A repeated identical selection returns the same state reference (no frame).
-    runPlanCommand(bench.session, '', 1)
+    settlePlanCommand(bench.session, commandId, 'success')
     expect(bench.values().plan).toEqual({ active: false, pending: true })
     commitPlanMode(bench.session, true, 0)
     expect(bench.values().plan).toEqual({ active: true, pending: false })
   })
 
+  it('drops a plan selection when its command settles with an error', async () => {
+    const bench = await harness(true)
+    commitPlanMode(bench.session, true, 0)
+    const commandId = runPlanCommand(bench.session, 'off', 0)
+    expect(bench.values().plan).toEqual({ active: true, pending: true })
+    settlePlanCommand(bench.session, commandId, 'error')
+    expect(bench.values().plan).toEqual({ active: true, pending: false })
+  })
+
   it('folds `off` args and non-plan commands correctly, and a matching selection is not pending', async () => {
     const bench = await harness(true)
     commitPlanMode(bench.session, true, 0)
@@ -141,7 +156,7 @@ describe('plan projection unit', () => {
     // memory involved, the fold alone answers {active:false, pending:true}.
     const cold = await harness(true)
     for (const event of bench.session.events) {
-      if (event.type === 'command/run' || event.type === 'plan/mode') {
+      if (event.type === 'command/run' || event.type === 'command/done' || event.type === 'plan/mode') {
         cold.session.append(event.type, event.data)
       }
     }

+ 1 - 1
packages/session-query/session-query-sqlite/src/index.ts

@@ -194,7 +194,7 @@ interface CursorPayload {
 
 /** Concrete SQLite owner of the combined `ctx.sessionQuery` service. */
 export class SqliteSessionQueryEngine extends SessionQueryEngine {
-  static override inject = ['sessions', 'sessionProjections']
+  static override inject = ['sessions']
 
   static Config: z<Config> = z.object({
     path: z.string().required(),

+ 12 - 7
packages/skill/tool-skill/src/index.ts

@@ -74,10 +74,10 @@ export const Config: z<Config> = z.object({
  * The skill-catalog projection's state schema — the one definition of the
  * state shape; the type is inferred from it (state equals the public shape).
  */
-const skillCatalogStateSchema = zod.object({
+const skillCatalogStateSchema = zod.array(zod.object({
   digest: zod.string().min(1),
   seq: zod.number().int().nonnegative(),
-}).nullable()
+})).nullable()
 
 type SkillCatalogState = zod.infer<typeof skillCatalogStateSchema>
 
@@ -89,7 +89,7 @@ type SkillCatalogState = zod.infer<typeof skillCatalogStateSchema>
  */
 declare module '@deepseek-ai/dsh-session-projection/types' {
   interface SessionProjectionStateMap {
-    /** The latest published skill-catalog message (digest + seq); null before the first publication. */
+    /** Newest-first published skill-catalog messages (digest + seq); null before the first publication. */
     skillCatalog: SkillCatalogState
   }
 }
@@ -225,14 +225,15 @@ export function apply(ctx: Context, config: Config = {}): void {
 
   ctx.sessionProjections.register({
     key: 'skillCatalog',
-    stateVersion: 1,
+    stateVersion: 2,
     stateSchema: skillCatalogStateSchema,
     init: () => null,
     apply: (state, event) => {
       if (event.type !== 'user/message' || event.data.source.kind !== 'skill-catalog') return state
       const entries = readCatalogEntries(event.data.source)
       if (entries === undefined) return state
-      return { digest: digestCatalogEntries(entries), seq: event.seq }
+      const record = { digest: digestCatalogEntries(entries), seq: event.seq }
+      return state === null ? [record] : [record, ...state]
     },
   })
 
@@ -395,8 +396,12 @@ function catalogHistory(ctx: Context, agent: Agent): { visibleDigest?: string; p
   const visible = new Set(agent.session.surface.nodes)
   const state = ctx.sessionProjections.stateOf(agent.session, 'skillCatalog') ?? null
   if (state === null) return { published: false }
-  return visible.has(state.seq)
-    ? { visibleDigest: state.digest, published: true }
+  // History is newest-first; the latest visible record restores the previous
+  // scan-visible semantics when a surface replacement shadows the newest
+  // catalog message but an older one stays visible.
+  const latestVisible = state.find(record => visible.has(record.seq))
+  return latestVisible !== undefined
+    ? { visibleDigest: latestVisible.digest, published: true }
     : { published: true }
 }
 

+ 0 - 1
packages/todo/tool-todo/src/index.ts

@@ -136,7 +136,6 @@ export function apply(ctx: Context, config: Config): void {
       return state
     },
     wire: { viewSchema: todosProjectionSchema, view: state => state },
-
   })
   ctx.tools.register(defineTool({
     name: 'todo_write',