Address Claude review follow-ups

This commit is contained in:
Tianyi Cui
2026-06-19 00:37:12 +08:00
parent 7fa113be0e
commit 4a3f3af296
8 changed files with 92 additions and 19 deletions
+2
View File
@@ -18,6 +18,8 @@ try {
// ENOENT (no .env) is fine — rely on the ambient environment. // ENOENT (no .env) is fine — rely on the ambient environment.
} }
// Resolve relative cordis.yml paths from the repo root no matter where the
// editor launches this demo command.
process.chdir(fileURLToPath(new URL('../..', import.meta.url))) process.chdir(fileURLToPath(new URL('../..', import.meta.url)))
const ctx = new Context() const ctx = new Context()
+10 -3
View File
@@ -100,6 +100,10 @@ function internalError(detail: string): RequestError {
return RequestError.internalError(undefined, detail) return RequestError.internalError(undefined, detail)
} }
function sameWorkspaceCwd(left: string, right: string): boolean {
return resolvePath(left) === resolvePath(right)
}
/** Plugin config: the agent template ACP sessions are created from. */ /** Plugin config: the agent template ACP sessions are created from. */
export interface AcpConfig { export interface AcpConfig {
/** Model name for created agents (must have a registered adapter). */ /** Model name for created agents (must have a registered adapter). */
@@ -466,13 +470,16 @@ export function apply(ctx: Context, config: AcpConfig): void {
// (An id unknown to `list()` falls through to resume, which rejects with // (An id unknown to `list()` falls through to resume, which rejects with
// the backend's not-found error.) // the backend's not-found error.)
const meta = (await sessionPersistence.list()).find(m => m.id === params.sessionId) const meta = (await sessionPersistence.list()).find(m => m.id === params.sessionId)
if (meta !== undefined && (meta.cwd === undefined || !isAbsolute(meta.cwd))) { if (meta !== undefined) {
const persistedCwd = meta.cwd
if (persistedCwd === undefined || !isAbsolute(persistedCwd)) {
throw invalidParams( throw invalidParams(
`session ${params.sessionId} has no absolute persisted cwd; cannot determine its workspace (it predates per-session cwd, or was created without one)`, `session ${params.sessionId} has no absolute persisted cwd; cannot determine its workspace (it predates per-session cwd, or was created without one)`,
) )
} }
if (meta !== undefined && meta.cwd !== params.cwd) { if (!sameWorkspaceCwd(persistedCwd, params.cwd)) {
throw invalidParams(`session ${params.sessionId} cwd mismatch: persisted ${meta.cwd}, requested ${params.cwd}`) throw invalidParams(`session ${params.sessionId} cwd mismatch: persisted ${persistedCwd}, requested ${params.cwd}`)
}
} }
const agent = await agents.resume({ const agent = await agents.resume({
agentId: params.sessionId, agentId: params.sessionId,
+8 -1
View File
@@ -178,7 +178,7 @@ describe('acp bridge — session/load replay', () => {
.rejects.toThrow(/cwd mismatch/) .rejects.toThrow(/cwd mismatch/)
expect(loader.ctx.agents.get('elsewhere')).toBeUndefined() expect(loader.ctx.agents.get('elsewhere')).toBeUndefined()
const res = await loader.client.loadSession({ sessionId: 'elsewhere', cwd: otherCwd, mcpServers: [] }) const res = await loader.client.loadSession({ sessionId: 'elsewhere', cwd: `${otherCwd}/.`, mcpServers: [] })
expect(res).toBeDefined() expect(res).toBeDefined()
expect(loader.ctx.agents.get('elsewhere')!.session.header.cwd).toBe(otherCwd) expect(loader.ctx.agents.get('elsewhere')!.session.header.cwd).toBe(otherCwd)
}) })
@@ -190,6 +190,13 @@ describe('acp bridge — session/load replay', () => {
.rejects.toThrow(/absolute/) .rejects.toThrow(/absolute/)
}) })
it('lets persistence reject a load for an unknown id after metadata lookup misses', async () => {
loader = await makeBridgeHarness({ storageDir, script: [] })
await loader.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} })
await expect(loader.client.loadSession({ sessionId: 'missing', cwd: process.cwd(), mcpServers: [] }))
.rejects.toThrow(/Internal error/)
})
it('rejects loading a persisted session that has NO cwd (would silently run in the launch dir)', async () => { it('rejects loading a persisted session that has NO cwd (would silently run in the launch dir)', async () => {
// A legacy / externally-created session log with no header.cwd. The bridge // A legacy / externally-created session log with no header.cwd. The bridge
// must reject the load rather than accept it and let bash silently fall back // must reject the load rather than accept it and let bash silently fall back
+9
View File
@@ -502,6 +502,11 @@ async function runStep(
if (stepError) throw stepError if (stepError) throw stepError
if (assembler.finish.kind === 'max-tokens') { if (assembler.finish.kind === 'max-tokens') {
let message: Message = withoutToolCalls(assembler.message())
message = withoutToolCalls(await ctx.waterfall('agent/step-result', agent, turn, step, message, () => Promise.resolve(message)))
if (message.content.length > 0) {
session.append('assistant/message', { turn, step, content: message.content })
}
if (assembler.usage) { if (assembler.usage) {
session.append('usage', { turn, step, usage: assembler.usage }) session.append('usage', { turn, step, usage: assembler.usage })
} }
@@ -566,6 +571,10 @@ async function runStep(
return { hadToolCalls: toolCalls.length > 0, finish: assembler.finish } return { hadToolCalls: toolCalls.length > 0, finish: assembler.finish }
} }
function withoutToolCalls(message: Message): Message {
return { ...message, content: message.content.filter(block => block.type !== 'tool-call') }
}
/** The last turn number in a (possibly seeded) session log, or 0. */ /** The last turn number in a (possibly seeded) session log, or 0. */
export function lastTurnNumber(session: Session): number { export function lastTurnNumber(session: Session): number {
const lastStart = session.events.findLast(event => event.type === 'turn/start') const lastStart = session.events.findLast(event => event.type === 'turn/start')
+33 -1
View File
@@ -385,6 +385,10 @@ describe('agent loop', () => {
expect(steps).toBe(2) expect(steps).toBe(2)
expect(adapter.requests).toHaveLength(2) expect(adapter.requests).toHaveLength(2)
expect(adapter.requests[1]!.messages).toEqual([
{ role: 'user', content: [{ type: 'text', text: 'go' }] },
{ role: 'assistant', content: [{ type: 'text', text: 'first half' }] },
])
expect(reasons).toEqual([{ kind: 'max-tokens' }]) expect(reasons).toEqual([{ kind: 'max-tokens' }])
}) })
@@ -436,10 +440,38 @@ describe('agent loop', () => {
expect(executions).toBe(0) expect(executions).toBe(0)
expect(agent.session.events.some(e => e.type === 'tool/call')).toBe(false) expect(agent.session.events.some(e => e.type === 'tool/call')).toBe(false)
expect(agent.session.deriveMessages()).toEqual([{ role: 'user', content: [{ type: 'text', text: 'go' }] }])
expect(reasons).toEqual([{ kind: 'max-tokens' }])
})
it('keeps safe max-tokens assistant content while dropping truncated tool calls', async () => {
const callId = CallId('c1')
const adapter = new MockAdapter([[
{ type: 'block-start', index: 0, blockType: 'text' },
{ type: 'text-delta', index: 0, text: 'partial text' },
{ type: 'block-end', index: 0, block: { type: 'text', text: 'partial text' } },
{ type: 'block-start', index: 1, blockType: 'tool-call' },
{ type: 'tool-call-delta', index: 1, id: callId, name: 'echo', argumentsDelta: '{"text"' },
{ type: 'finish', reason: { kind: 'max-tokens' } },
]])
const ctx = await harness(adapter)
let stepResults = 0
ctx.on('agent/step-result', async (_agent, _turn, _step, message, next) => {
stepResults += 1
expect(message.content).toEqual([{ type: 'text', text: 'partial text' }])
return next()
})
const agent = ctx.agentLoop.create('a1', { model: 'mock' })
send(agent, 'go')
await waitForIdle(ctx, agent)
expect(stepResults).toBe(1)
expect(agent.session.events.some(e => e.type === 'tool/call')).toBe(false)
expect(agent.session.deriveMessages()).toEqual([ expect(agent.session.deriveMessages()).toEqual([
{ role: 'user', content: [{ type: 'text', text: 'go' }] }, { role: 'user', content: [{ type: 'text', text: 'go' }] },
{ role: 'assistant', content: [{ type: 'text', text: 'partial text' }] },
]) ])
expect(reasons).toEqual([{ kind: 'max-tokens' }])
}) })
it('stops the turn when agent/step-end listener failure has recorded an error', async () => { it('stops the turn when agent/step-end listener failure has recorded an error', async () => {
@@ -448,6 +448,7 @@ export class SessionPersistenceSqlite extends SessionPersistence {
// Dispose must reach quiescence: await every init + final drain, then close // Dispose must reach quiescence: await every init + final drain, then close
// the database, BEFORE returning, so no write lands after teardown. // the database, BEFORE returning, so no write lands after teardown.
ctx.effect(() => async () => { ctx.effect(() => async () => {
let disposeError: unknown
try { try {
const errors = [ const errors = [
...await settledErrors(this.inits.values()), ...await settledErrors(this.inits.values()),
@@ -457,9 +458,20 @@ export class SessionPersistenceSqlite extends SessionPersistence {
if (errors.length > 0) { if (errors.length > 0) {
throw new AggregateError(errors, 'session-persistence-sqlite dispose failed') throw new AggregateError(errors, 'session-persistence-sqlite dispose failed')
} }
} catch (error: unknown) {
disposeError = error
throw error
} finally { } finally {
try {
await this.ready await this.ready
this.db.close() this.db.close()
} catch (error: unknown) {
/* v8 ignore next -- open/close failure racing disposal is a defensive teardown edge */
if (disposeError === undefined) throw error
// Opening/closing the database can only add teardown context here; keep
// the already-captured init/flush/chain AggregateError as the primary
// disposal failure instead of masking it from callers.
}
} }
}, 'session-persistence-sqlite write path') }, 'session-persistence-sqlite write path')
+6 -3
View File
@@ -116,9 +116,12 @@ export class SystemPrompt extends Service {
/** /**
* Assemble the current prompt (sections sorted by order, tools collected * Assemble the current prompt (sections sorted by order, tools collected
* from all providers). Runs through the `system-prompt/assemble` waterfall, * from all providers). Section records are top-level clones (the `text`
* giving listeners the opportunity to mutate or replace the assembly before * provider may be a function and is intentionally shared); tool schemas are
* it reaches the model. Await the result before reading the assembly values — * deep-cloned because adapters and request waterfalls may mutate schema
* objects. Runs through the `system-prompt/assemble` waterfall, giving
* listeners the opportunity to mutate or replace the assembly before it
* reaches the model. Await the result before reading the assembly values —
* waterfall listeners may be async. * waterfall listeners may be async.
*/ */
assemble(): Promise<PromptAssembly> { assemble(): Promise<PromptAssembly> {
+6 -5
View File
@@ -332,11 +332,12 @@ export class ToolRegistry extends Service {
} }
/** /**
* Execute one tool call through the `tools/execute` waterfall. If the tool * Execute one tool call through the `tools/execute` waterfall. If the tool is
* is not registered, the result is an `isError` carrying a `UNKNOWN_TOOL` * not registered, the result is an `isError` carrying a `UNKNOWN_TOOL`
* structured error. If the tool throws, the error is caught and returned as * structured error. If the tool or a waterfall listener throws, the error is
* an `isError` result so the loop never sees an uncaught exception; a thrown * caught and returned as an `isError` result so the loop records a failed tool
* {@link HarnessError} surfaces its `{ name, code }` on the result. * call instead of failing the whole turn; a thrown {@link HarnessError}
* surfaces its `{ name, code }` on the result.
*/ */
async execute(exec: ToolExecution): Promise<ToolExecutionResult> { async execute(exec: ToolExecution): Promise<ToolExecutionResult> {
try { try {