fix(events): address Codex review — protect post-execute result, purge stale tools/execute refs
Codex's PR-C review found two (A) blockers: - tools/post-execute could corrupt the protected outcome. postExecute passed the mutable `result` to listeners and then read result.callId / spread result on the return paths, so a listener mutating the reference (flipping isError, rewriting callId, injecting an error) escaped the decision channel. Now the authoritative callId/isError/error are SNAPSHOT before the waterfall and the return value is rebuilt from the snapshot + the typed PostToolDecision — the decision is the only sanctioned way to change the outcome, and callId is always exec.callId. Added a regression test that mutates the result reference and asserts it has no effect; proven to fail red on the unfixed code. - Public docs/JSDoc still advertised the removed `tools/execute` waterfall after the split. Swept every current-state reference to tools/pre-execute + tools/post-execute: the ToolRegistry class JSDoc (and the regenerated catalog), loop.ts's ASCII flow (also added the prompt-submit/session-start steps it was missing), the package-map READMEs (packages, core, agent-core), core-data-structures core.md/tools.md, the bash + acp + invariants src/READMEs (the deferred permission gate is the tools/pre-execute deny/ask seam now), the cookbook, and the implemented RFCs whose factual seam catalog drifted. codec.ts's totality prose now lists `rejected`. Proposed-RFC references are left as-is (frozen proposals, validated when built).
This commit is contained in:
@@ -335,8 +335,9 @@ function errorInfo(error: unknown): ToolErrorInfo | undefined {
|
||||
|
||||
/**
|
||||
* Tool registry (`ctx.tools`): tool plugins register definitions; the agent
|
||||
* loop executes calls through the `tools/execute` waterfall. The registry
|
||||
* contributes its schemas into the system-prompt assembly.
|
||||
* loop executes calls through the `tools/pre-execute` → dispatch →
|
||||
* `tools/post-execute` pipeline. The registry contributes its schemas into the
|
||||
* system-prompt assembly.
|
||||
*/
|
||||
export class ToolRegistry extends Service {
|
||||
static inject = ['systemPrompt']
|
||||
@@ -464,6 +465,19 @@ export class ToolRegistry extends Service {
|
||||
* Runs inside `execute`'s outer try/catch (a throwing listener → isError).
|
||||
*/
|
||||
private async postExecute(exec: ToolExecution, result: ToolExecutionResult): Promise<ToolExecutionResult> {
|
||||
// Snapshot the protected outcome BEFORE the waterfall. A listener receives
|
||||
// the same `result` reference, so a post-waterfall read of `result.callId`/
|
||||
// `.isError`/`.error` could carry a listener's mutation — violating the
|
||||
// authoritative-call-id requirement and the "preserve the dispatched
|
||||
// isError/error" contract. The decision is the ONLY sanctioned channel for a
|
||||
// listener to change the outcome (block, or accept-with-replacement); the
|
||||
// call id is always the authoritative `exec.callId`.
|
||||
const dispatched = {
|
||||
callId: exec.callId,
|
||||
content: result.content,
|
||||
isError: result.isError,
|
||||
...result.error ? { error: result.error } : {},
|
||||
}
|
||||
const decision = await this.ctx.waterfall(
|
||||
this, 'tools/post-execute', exec, result,
|
||||
() => Promise.resolve<PostToolDecision>({ kind: 'accept' }),
|
||||
@@ -471,7 +485,7 @@ export class ToolRegistry extends Service {
|
||||
const additionalContext = decision.additionalContext
|
||||
if (decision.kind === 'block') {
|
||||
return {
|
||||
callId: result.callId,
|
||||
callId: dispatched.callId,
|
||||
content: decision.feedback,
|
||||
isError: true,
|
||||
...additionalContext ? { additionalContext } : {},
|
||||
@@ -479,7 +493,7 @@ export class ToolRegistry extends Service {
|
||||
}
|
||||
// accept: replace content if supplied, preserve the dispatched isError/error.
|
||||
return {
|
||||
...result,
|
||||
...dispatched,
|
||||
...decision.content ? { content: decision.content } : {},
|
||||
...additionalContext ? { additionalContext } : {},
|
||||
}
|
||||
|
||||
@@ -201,6 +201,30 @@ describe('ToolRegistry', () => {
|
||||
expect(result.additionalContext).toMatchObject({ content: [{ text: 'fyi' }], source: { kind: 'plugin', plugin: 'test' } })
|
||||
})
|
||||
|
||||
it('a post-execute listener mutating the result object cannot corrupt callId/isError/error', async () => {
|
||||
// The decision is the ONLY sanctioned channel to change the outcome. A
|
||||
// listener that reaches in and mutates the passed result reference (flipping
|
||||
// isError, rewriting callId, attaching a bogus error) must NOT affect what
|
||||
// execute() returns — the registry snapshots the authoritative fields before
|
||||
// the waterfall and rebuilds from the snapshot + decision.
|
||||
const ctx = await setup()
|
||||
ctx.tools.register(echoTool)
|
||||
|
||||
ctx.on('tools/post-execute', async (_exec, result, next) => {
|
||||
const mutable = result as { callId: string; isError: boolean; error?: unknown }
|
||||
mutable.callId = 'hijacked'
|
||||
mutable.isError = true
|
||||
mutable.error = { name: 'Evil', code: 'EVIL' }
|
||||
return next() // delegate to the default accept — no decision-level override
|
||||
})
|
||||
|
||||
const result = await ctx.tools.execute({ callId: CallId('c1'), name: 'echo', arguments: { text: 'hi' } })
|
||||
expect(result.callId).toBe(CallId('c1')) // authoritative exec.callId, not 'hijacked'
|
||||
expect(result.isError).toBe(false) // the real (successful) dispatch outcome
|
||||
expect(result.error).toBeUndefined() // no listener-injected error
|
||||
expect(result.content[0]).toMatchObject({ text: 'hi' })
|
||||
})
|
||||
|
||||
it('composes pre + post waterfalls around dispatch (sandbox-wrap pattern)', async () => {
|
||||
const ctx = await setup()
|
||||
ctx.tools.register(echoTool)
|
||||
|
||||
Reference in New Issue
Block a user