diff --git a/.github/agent-sops/task-implementer.sop.md b/.github/agent-sops/task-implementer.sop.md index f74b198f..0e49e544 100644 --- a/.github/agent-sops/task-implementer.sop.md +++ b/.github/agent-sops/task-implementer.sop.md @@ -98,7 +98,7 @@ Create a comprehensive list of test scenarios covering normal operation, edge ca - You MUST check for existing testing strategies documented in the repository documentation or your notes - You MUST cover all acceptance criteria with at least one test scenario - You MUST define explicit input/output pairs for each test case -- You MUST make note of these test scenarios +- You MUST make note of these test scenarios - You MUST design tests that will initially fail when run against non-existent implementations - You MUST NOT create mock implementations during the test design phase because tests should be written based solely on expected behavior, not influenced by implementation details - You MUST focus on test scenarios and expected behaviors rather than detailed test code in documentation @@ -234,23 +234,40 @@ If the implementation meets all requirements and follows established patterns, p If all tests are passing, draft a conventional commit message, perform the git commit, and create/update the pull request. +**PR Checklist Verification (REQUIRED):** + +Before creating or updating a PR, you MUST copy the checklist from [docs/PR.md](../../docs/PR.md) into your progress notes and explicitly verify each item. For each checklist item, you MUST: + +1. Copy the checklist item verbatim +2. Mark it as `[x]` (pass) or `[ ]` (fail) +3. If failed, revise the PR description until the item passes + +Example format in your notes: + +```markdown +## PR Description Checklist Verification + +- [x] Does the PR description target a Senior Engineer familiar with the project? +- [ ] Does the PR include a "Resolves #" in the body? → FAILED: missing issue reference, adding now +``` + +You MUST NOT create or update the PR until ALL checklist items pass. + **Constraints:** + +- You MUST read and follow the PR description guidelines in [docs/PR.md](../../docs/PR.md) when creating pull requests & commits - You MUST check that all tasks are complete before proceeding - You MUST reference your notes for the issue you are creating a pull request for -- You MUST NOT commit changes until builds AND tests have been verified because committing broken code can disrupt the development workflow and introduce bugs into the codebase +- You MUST NOT commit changes until builds AND tests have been verified because committing broken code can disrupt the development workflow and introduce bugs into the codebase - You MUST follow the Conventional Commits specification - You MUST use `git status` to check which files have been modified - You MUST use `git add` to stage all relevant files - You MUST execute the `git commit -m ` command with the prepared commit message - You MAY use `git push origin ` to push the local branch to the remote if the `GITHUB_WRITE` environment variable is set to `true` - - If the push operation is deferred, continue with PR creation and note the deferred status + - If the push operation is deferred, continue with PR creation and note the deferred status - You MUST attempt to create the pull request using the `create_pull_request` tool if it does not exist yet - If the PR creation is deferred, continue with the workflow and note the deferred status - You MUST use the task id recorded in your notes, not the issue id - - You MUST include "Resolves: #" in the body of the pull request - - You MUST NOT bold this line - - You MUST give an overview of the feature being implemented - - You MUST include any notes on key implementation decisions, ambiguity, or other information as part of the pull request description - If the `create_pull_request` tool fails (excluding deferred responses): - The tool automatically handles fallback by posting a properly URL-encoded manual PR creation link as a comment on the specified fallback issue - You MUST verify the fallback comment was posted successfully by checking the tool's return message @@ -303,6 +320,7 @@ Based on the users feedback, you will review and update your implementation plan - You MUST NOT close the parent issue - only the user should close it after the pull request is merged - You MUST not attempt to merge the pull request - You MUST use the handoff_to_user tool to inform the user you are ready for clarifying information on the pull request +- You SHOULD include additional checklist items from [docs/PR.md](../../docs/PR.md) to validate the pull request description is correct after making additional changes ## Desired Outcome @@ -396,6 +414,25 @@ If builds fail during implementation: - Use progress tracking with markdown checklists - Document decisions, assumptions, and challenges +### Checklist Verification Pattern + +When documentation files contain checklists (e.g., `docs/PR.md`), you MUST: + +1. Copy the entire checklist into your progress notes +2. Explicitly verify each item by marking `[x]` or `[ ]` +3. For any failed items, document the issue and fix it before proceeding +4. Re-verify failed items after fixes until all pass + +This pattern ensures quality gates are not skipped and provides an audit trail of verification. + +### Pull Request Best Practices + +- You MUST follow the PR description guidelines in [docs/PR.md](../../docs/PR.md) +- Focus on WHY the change is needed, not HOW it's implemented +- Document public API changes with before/after code examples +- Write for senior engineers familiar with the project +- Skip implementation details, test coverage notes, and line-by-line change lists + ### Git Best Practices - Commit early and often with descriptive messages - Follow Conventional Commits specification diff --git a/.github/agent-sops/task-refiner.sop.md b/.github/agent-sops/task-refiner.sop.md index fd1f1a86..25cd1b38 100644 --- a/.github/agent-sops/task-refiner.sop.md +++ b/.github/agent-sops/task-refiner.sop.md @@ -185,7 +185,6 @@ Record that the task review is complete and ready as a comment on the issue. - If comment posting is deferred, continue with the workflow and note the deferred status - You MUST summarize what was accomplished in your comment - You MUST confirm in your comment that the issue is ready for implementation, or explain why it is not -- You MUST record the estimated scope of work based on repository analysis - You SHOULD mention any final recommendations or considerations ## Examples @@ -257,12 +256,6 @@ Based on clarification discussion and repository analysis: - [ ] 2FA can be enabled/disabled by user - [ ] Integration tests pass - [ ] Existing auth functionality remains intact - -### Estimated Scope -- **Complexity**: Medium -- **Files Modified**: ~8-10 files -- **New Components**: 2-3 React components -- **Database Migrations**: 1-2 migrations required ``` ## Troubleshooting diff --git a/docs/PR.md b/docs/PR.md new file mode 100644 index 00000000..30d89529 --- /dev/null +++ b/docs/PR.md @@ -0,0 +1,203 @@ +# Pull Request Description Guidelines + +Good PR descriptions help reviewers understand the context and impact of your changes. They enable faster reviews, better decision-making, and serve as valuable historical documentation. + +When creating a PR, follow the [GitHub PR template](../.github/PULL_REQUEST_TEMPLATE.md) and use these guidelines to fill it out effectively. + +## Who's Reading Your PR? + +Write for senior engineers familiar with the SDK. Assume your reader: + +- Understands the SDK's architecture and patterns +- Has context about the broader system +- Can read code diffs to understand implementation details +- Values concise, focused communication + +## What to Include + +Every PR description should have: + +1. **Motivation** — Why is this change needed? +2. **Public API Changes** — What changes to the public API (with code examples)? +3. **Use Cases** (optional) — When would developers use this feature? Only include for non-obvious functionality; skip for trivial changes. +4. **Breaking Changes** (if applicable) — What breaks and how to migrate? + +## Writing Principles + +**Focus on WHY, not HOW:** + +- ✅ "The OpenAI SDK supports dynamic API keys, but we don't expose this capability" +- ❌ "Added ApiKeySetter type import from openai/client" + +**Document public API changes with examples:** + +- ✅ Show before/after code examples for API changes +- ❌ List every file or line changed + +**Be concise:** + +- ✅ Use prose over bullet lists when possible +- ❌ Create exhaustive implementation checklists + +**Emphasize user impact:** + +- ✅ "Enables secret manager integration for credential rotation" +- ❌ "Updated error message to mention 'string or function'" + +## What to Skip + +Leave these out of your PR description: + +- **Implementation details** — Code comments and commit messages cover this +- **Test coverage notes** — CI will catch issues; assume tests are comprehensive +- **Line-by-line change lists** — The diff provides this +- **Build/lint/coverage status** — CI handles verification +- **Commit hashes** — GitHub links commits automatically + +## Anti-patterns + +❌ **Over-detailed checklists:** + +```markdown +### Type Definition Updates + +- Added ApiKeySetter type import from 'openai/client' +- Updated OpenAIModelOptions interface apiKey type +``` + +❌ **Implementation notes reviewers don't need:** + +```markdown +## Implementation Notes + +- No breaking changes - all existing string-based usage continues to work +- OpenAI SDK handles validation of function return values +``` + +❌ **Test coverage bullets:** + +```markdown +### Test Coverage + +- Added test: accepts function-based API key +- Added test: accepts async function-based API key +``` + +## Good Examples + +✅ **Motivation section:** + +```markdown +## Motivation + +The OpenAI SDK supports dynamic API key resolution through async functions, +enabling use cases like credential rotation and secret manager integration. +However, our SDK currently only accepts static strings for the apiKey parameter, +preventing users from leveraging these capabilities. +``` + +✅ **Public API Changes section:** + +````markdown +## Public API Changes + +The `OpenAIModelOptions.apiKey` parameter now accepts either a string or an +async function: + +```typescript +// Before: only string supported +const model = new OpenAIModel({ + modelId: 'gpt-4o', + apiKey: 'sk-...', +}) + +// After: function also supported +const model = new OpenAIModel({ + modelId: 'gpt-4o', + apiKey: async () => await secretManager.getApiKey(), +}) +``` +```` + +The change is backward compatible—all existing string-based usage continues +to work without modification. + +```` + +✅ **Use Cases section:** +```markdown +## Use Cases + +- **API key rotation**: Rotate keys without application restart +- **Secret manager integration**: Fetch credentials from AWS Secrets Manager, Vault, etc. +- **Multi-tenant systems**: Dynamically select API keys based on context +```` + +## Template + +````markdown +## Motivation + +[Explain WHY this change is needed. What problem does it solve? What limitation +does it address? What user need does it fulfill?] + +Resolves: #[issue-number] + +## Public API Changes + +[Document changes to public APIs with before/after code examples. If no public +API changes, state "No public API changes."] + +```typescript +// Before +[existing API usage] + +// After +[new API usage] +``` +```` + +[Explain behavior, parameters, return values, and backward compatibility.] + +## Use Cases (optional) + +[Only include for non-obvious functionality. Provide 2-4 concrete use cases +showing when developers would use this feature. Skip for trivial changes.] + +## Breaking Changes (if applicable) + +[If this is a breaking change, explain what breaks and provide migration guidance.] + +### Migration + +```typescript +// Before +[old code] + +// After +[new code] +``` + +``` + +## Why These Guidelines? + +**Focus on WHY over HOW** because code diffs show implementation details, commit messages document granular changes, and PR descriptions provide the broader context reviewers need. + +**Skip test/lint/coverage details** because CI pipelines verify these automatically. Including them adds noise without value. + +**Write for senior engineers** to enable concise, technical communication without redundant explanations. + +## References + +- [Conventional Commits](https://www.conventionalcommits.org/) +- [Google's Code Review Guidelines](https://google.github.io/eng-practices/review/) +``` + +## Checklist Items + + - [ ] Does the PR description target a Senior Engineer familiar with the project? + - [ ] Does the PR description give an overview of the feature being implemented, including any notes on key implemention decisions + - [ ] Does the PR include a "Resolves #" in the body and is not bolded? + - [ ] Does the PR contain the motivation or use-cases behind the change? + - [ ] Does the PR omit irrelevent details not needed for historical reference? diff --git a/src/agent/__tests__/agent.hook.test.ts b/src/agent/__tests__/agent.hook.test.ts index eb77a825..4269fe5c 100644 --- a/src/agent/__tests__/agent.hook.test.ts +++ b/src/agent/__tests__/agent.hook.test.ts @@ -15,6 +15,7 @@ import { MockMessageModel } from '../../__fixtures__/mock-message-model.js' import { MockHookProvider } from '../../__fixtures__/mock-hook-provider.js' import { collectIterator } from '../../__fixtures__/model-test-helpers.js' import { FunctionTool } from '../../tools/function-tool.js' +import type { ToolContext } from '../../tools/tool.js' import { Message, TextBlock, ToolResultBlock } from '../../types/messages.js' describe('Agent Hooks Integration', () => { @@ -335,4 +336,366 @@ describe('Agent Hooks Integration', () => { await expect(agent.invoke('Test')).rejects.toThrow('Failure') }) }) + + describe('writable properties', () => { + describe('BeforeToolCallEvent.tool modification', () => { + it('allows hook to change tool to a different tool', async () => { + const tool1 = new FunctionTool({ + name: 'tool1', + description: 'First tool', + inputSchema: {}, + callback: () => 'Result from tool1', + }) + const tool2 = new FunctionTool({ + name: 'tool2', + description: 'Second tool', + inputSchema: {}, + callback: () => 'Result from tool2', + }) + + const hookProvider = { + registerCallbacks: (registry: HookRegistry) => { + registry.addCallback(BeforeToolCallEvent, (event: BeforeToolCallEvent) => { + if (event.tool === tool1) { + event.tool = tool2 + } + }) + }, + } + + const model = new MockMessageModel() + .addTurn({ type: 'toolUseBlock', name: 'tool1', toolUseId: 'tool-1', input: {} }) + .addTurn({ type: 'textBlock', text: 'Final response' }) + + const agent = new Agent({ + model, + tools: [tool1, tool2], + hooks: [hookProvider], + }) + + await agent.invoke('Test') + + // Verify tool2 was executed instead of tool1 + const messages = agent.messages + const toolResultMessage = messages.filter((m) => m.role === 'user').pop() // Get last user message + const toolResultBlock = toolResultMessage?.content[0] as ToolResultBlock + expect(toolResultBlock.content[0]).toEqual({ type: 'textBlock', text: 'Result from tool2' }) + }) + + it('allows hook to set tool to undefined when tool exists', async () => { + const tool = new FunctionTool({ + name: 'testTool', + description: 'Test tool', + inputSchema: {}, + callback: () => 'Tool result', + }) + + const hookProvider = { + registerCallbacks: (registry: HookRegistry) => { + registry.addCallback(BeforeToolCallEvent, (event: BeforeToolCallEvent) => { + event.tool = undefined + }) + }, + } + + const model = new MockMessageModel() + .addTurn({ type: 'toolUseBlock', name: 'testTool', toolUseId: 'tool-1', input: {} }) + .addTurn({ type: 'textBlock', text: 'Final response' }) + + const agent = new Agent({ + model, + tools: [tool], + hooks: [hookProvider], + }) + + await agent.invoke('Test') + + // Verify error result was returned for "tool not found" + const messages = agent.messages + const toolResultMessage = messages.filter((m) => m.role === 'user').pop() + const toolResultBlock = toolResultMessage?.content[0] as ToolResultBlock + expect(toolResultBlock.status).toBe('error') + expect(toolResultBlock.content[0]).toEqual({ type: 'textBlock', text: "Tool 'testTool' not found in registry" }) + }) + + it('allows hook to set tool when originally undefined', async () => { + const tool = new FunctionTool({ + name: 'testTool', + description: 'Test tool', + inputSchema: {}, + callback: () => 'Tool result', + }) + + const hookProvider = { + registerCallbacks: (registry: HookRegistry) => { + registry.addCallback(BeforeToolCallEvent, (event: BeforeToolCallEvent) => { + if (event.tool === undefined) { + event.tool = tool + } + }) + }, + } + + const model = new MockMessageModel() + .addTurn({ type: 'toolUseBlock', name: 'unknownTool', toolUseId: 'tool-1', input: {} }) + .addTurn({ type: 'textBlock', text: 'Final response' }) + + const agent = new Agent({ + model, + tools: [tool], + hooks: [hookProvider], + }) + + await agent.invoke('Test') + + // Verify tool was executed successfully + const messages = agent.messages + const toolResultMessage = messages.filter((m) => m.role === 'user').pop() + const toolResultBlock = toolResultMessage?.content[0] as ToolResultBlock + expect(toolResultBlock.status).toBe('success') + expect(toolResultBlock.content[0]).toEqual({ type: 'textBlock', text: 'Tool result' }) + }) + }) + + describe('BeforeToolCallEvent.toolUse modification', () => { + it('allows hook to modify toolUse.input', async () => { + const tool = new FunctionTool({ + name: 'calculator', + description: 'Calculator tool', + inputSchema: {}, + callback: (_input, ctx: ToolContext) => `Calculated: ${JSON.stringify(ctx.toolUse.input)}`, + }) + + const hookProvider = { + registerCallbacks: (registry: HookRegistry) => { + registry.addCallback(BeforeToolCallEvent, (event: BeforeToolCallEvent) => { + event.toolUse = { + ...event.toolUse, + input: { modified: true, value: 42 }, + } + }) + }, + } + + const model = new MockMessageModel() + .addTurn({ type: 'toolUseBlock', name: 'calculator', toolUseId: 'tool-1', input: { original: true } }) + .addTurn({ type: 'textBlock', text: 'Final response' }) + + const agent = new Agent({ + model, + tools: [tool], + hooks: [hookProvider], + }) + + await agent.invoke('Test') + + // Verify modified input was passed to tool + const messages = agent.messages + const toolResultMessage = messages.filter((m) => m.role === 'user').pop() + const toolResultBlock = toolResultMessage?.content[0] as ToolResultBlock + expect(toolResultBlock.content[0]).toEqual({ + type: 'textBlock', + text: 'Calculated: {"modified":true,"value":42}', + }) + }) + + it('allows hook to modify toolUse.name', async () => { + const tool = new FunctionTool({ + name: 'renamedTool', + description: 'Tool with modified name', + inputSchema: {}, + callback: () => 'Result', + }) + + const hookProvider = { + registerCallbacks: (registry: HookRegistry) => { + registry.addCallback(BeforeToolCallEvent, (event: BeforeToolCallEvent) => { + event.toolUse = { + ...event.toolUse, + name: 'renamedTool', + } + }) + }, + } + + const model = new MockMessageModel() + .addTurn({ type: 'toolUseBlock', name: 'originalTool', toolUseId: 'tool-1', input: {} }) + .addTurn({ type: 'textBlock', text: 'Final response' }) + + const agent = new Agent({ + model, + tools: [tool], + hooks: [hookProvider], + }) + + const result = await agent.invoke('Test') + + // Agent should complete successfully + expect(result.stopReason).toBe('endTurn') + }) + + it('allows hook to modify toolUse.toolUseId', async () => { + const tool = new FunctionTool({ + name: 'testTool', + description: 'Test tool', + inputSchema: {}, + callback: () => 'Result', + }) + + const hookProvider = { + registerCallbacks: (registry: HookRegistry) => { + registry.addCallback(BeforeToolCallEvent, (event: BeforeToolCallEvent) => { + event.toolUse = { + ...event.toolUse, + toolUseId: 'modified-id', + } + }) + }, + } + + const model = new MockMessageModel() + .addTurn({ type: 'toolUseBlock', name: 'testTool', toolUseId: 'original-id', input: {} }) + .addTurn({ type: 'textBlock', text: 'Final response' }) + + const agent = new Agent({ + model, + tools: [tool], + hooks: [hookProvider], + }) + + await agent.invoke('Test') + + // Verify result uses modified toolUseId + const messages = agent.messages + const toolResultMessage = messages.filter((m) => m.role === 'user').pop() + const toolResultBlock = toolResultMessage?.content[0] as ToolResultBlock + expect(toolResultBlock.toolUseId).toBe('modified-id') + }) + }) + + describe('AfterToolCallEvent.result modification', () => { + it('allows hook to modify result content', async () => { + const tool = new FunctionTool({ + name: 'testTool', + description: 'Test tool', + inputSchema: {}, + callback: () => 'Original result', + }) + + const hookProvider = { + registerCallbacks: (registry: HookRegistry) => { + registry.addCallback(AfterToolCallEvent, (event: AfterToolCallEvent) => { + event.result = new ToolResultBlock({ + toolUseId: event.result.toolUseId, + status: event.result.status, + content: [new TextBlock('Modified result')], + }) + }) + }, + } + + const model = new MockMessageModel() + .addTurn({ type: 'toolUseBlock', name: 'testTool', toolUseId: 'tool-1', input: {} }) + .addTurn({ type: 'textBlock', text: 'Final response' }) + + const agent = new Agent({ + model, + tools: [tool], + hooks: [hookProvider], + }) + + await agent.invoke('Test') + + // Verify modified result appears in conversation + const messages = agent.messages + const toolResultMessage = messages.filter((m) => m.role === 'user').pop() + const toolResultBlock = toolResultMessage?.content[0] as ToolResultBlock + expect(toolResultBlock.content[0]).toEqual({ type: 'textBlock', text: 'Modified result' }) + }) + + it('allows hook to change result status from success to error', async () => { + const tool = new FunctionTool({ + name: 'testTool', + description: 'Test tool', + inputSchema: {}, + callback: () => 'Success', + }) + + const hookProvider = { + registerCallbacks: (registry: HookRegistry) => { + registry.addCallback(AfterToolCallEvent, (event: AfterToolCallEvent) => { + event.result = new ToolResultBlock({ + toolUseId: event.result.toolUseId, + status: 'error', + content: [new TextBlock('Converted to error')], + }) + }) + }, + } + + const model = new MockMessageModel() + .addTurn({ type: 'toolUseBlock', name: 'testTool', toolUseId: 'tool-1', input: {} }) + .addTurn({ type: 'textBlock', text: 'Final response' }) + + const agent = new Agent({ + model, + tools: [tool], + hooks: [hookProvider], + }) + + await agent.invoke('Test') + + // Verify result status was changed to error + const messages = agent.messages + const toolResultMessage = messages.filter((m) => m.role === 'user').pop() + const toolResultBlock = toolResultMessage?.content[0] as ToolResultBlock + expect(toolResultBlock.status).toBe('error') + expect(toolResultBlock.content[0]).toEqual({ type: 'textBlock', text: 'Converted to error' }) + }) + + it('allows hook to modify error result', async () => { + const tool = new FunctionTool({ + name: 'failingTool', + description: 'A tool that fails', + inputSchema: {}, + callback: () => { + throw new Error('Tool execution failed') + }, + }) + + const hookProvider = { + registerCallbacks: (registry: HookRegistry) => { + registry.addCallback(AfterToolCallEvent, (event: AfterToolCallEvent) => { + if (event.result.error) { + event.result = new ToolResultBlock({ + toolUseId: event.result.toolUseId, + status: 'success', + content: [new TextBlock('Error converted to success')], + }) + } + }) + }, + } + + const model = new MockMessageModel() + .addTurn({ type: 'toolUseBlock', name: 'failingTool', toolUseId: 'tool-1', input: {} }) + .addTurn({ type: 'textBlock', text: 'Final response' }) + + const agent = new Agent({ + model, + tools: [tool], + hooks: [hookProvider], + }) + + await agent.invoke('Test') + + // Verify error was converted to success + const messages = agent.messages + const toolResultMessage = messages.filter((m) => m.role === 'user').pop() + const toolResultBlock = toolResultMessage?.content[0] as ToolResultBlock + expect(toolResultBlock.status).toBe('success') + expect(toolResultBlock.content[0]).toEqual({ type: 'textBlock', text: 'Error converted to success' }) + }) + }) + }) }) diff --git a/src/agent/agent.ts b/src/agent/agent.ts index 4f00483b..3e989cb9 100644 --- a/src/agent/agent.ts +++ b/src/agent/agent.ts @@ -567,33 +567,45 @@ export class Agent implements AgentData { input: toolUseBlock.input, } - yield new BeforeToolCallEvent({ agent: this, toolUse, tool }) + const beforeEvent = new BeforeToolCallEvent({ agent: this, toolUse, tool }) + yield beforeEvent - if (!tool) { + // After yielding, hooks may have modified tool and toolUse + const actualTool = beforeEvent.tool + const actualToolUse = beforeEvent.toolUse + + if (!actualTool) { // Tool not found - return error result instead of throwing const errorResult = new ToolResultBlock({ - toolUseId: toolUseBlock.toolUseId, + toolUseId: actualToolUse.toolUseId, status: 'error', - content: [new TextBlock(`Tool '${toolUseBlock.name}' not found in registry`)], + content: [new TextBlock(`Tool '${actualToolUse.name}' not found in registry`)], }) - yield new AfterToolCallEvent({ agent: this, toolUse, tool, result: errorResult }) + const afterEvent = new AfterToolCallEvent({ + agent: this, + toolUse: actualToolUse, + tool: actualTool, + result: errorResult, + }) + yield afterEvent - return errorResult + // After yielding, hooks may have modified result + return afterEvent.result } // Execute tool and collect result const toolContext: ToolContext = { toolUse: { - name: toolUseBlock.name, - toolUseId: toolUseBlock.toolUseId, - input: toolUseBlock.input, + name: actualToolUse.name, + toolUseId: actualToolUse.toolUseId, + input: actualToolUse.input, }, agent: this, } try { - const toolGenerator = tool.stream(toolContext) + const toolGenerator = actualTool.stream(toolContext) // Use yield* to delegate to the tool generator and capture the return value const toolResult = yield* toolGenerator @@ -601,33 +613,53 @@ export class Agent implements AgentData { if (!toolResult) { // Tool didn't return a result - return error result instead of throwing const errorResult = new ToolResultBlock({ - toolUseId: toolUseBlock.toolUseId, + toolUseId: actualToolUse.toolUseId, status: 'error', - content: [new TextBlock(`Tool '${toolUseBlock.name}' did not return a result`)], + content: [new TextBlock(`Tool '${actualToolUse.name}' did not return a result`)], }) - yield new AfterToolCallEvent({ agent: this, toolUse, tool, result: errorResult }) + const afterEvent = new AfterToolCallEvent({ + agent: this, + toolUse: actualToolUse, + tool: actualTool, + result: errorResult, + }) + yield afterEvent - return errorResult + // After yielding, hooks may have modified result + return afterEvent.result } - yield new AfterToolCallEvent({ agent: this, toolUse, tool, result: toolResult }) + const afterEvent = new AfterToolCallEvent({ + agent: this, + toolUse: actualToolUse, + tool: actualTool, + result: toolResult, + }) + yield afterEvent - // Tool already returns ToolResultBlock directly - return toolResult + // After yielding, hooks may have modified result + return afterEvent.result } catch (error) { // Tool execution failed with error const toolError = normalizeError(error) const errorResult = new ToolResultBlock({ - toolUseId: toolUseBlock.toolUseId, + toolUseId: actualToolUse.toolUseId, status: 'error', content: [new TextBlock(toolError.message)], error: toolError, }) - yield new AfterToolCallEvent({ agent: this, toolUse, tool, result: errorResult, error: toolError }) + const afterEvent = new AfterToolCallEvent({ + agent: this, + toolUse: actualToolUse, + tool: actualTool, + result: errorResult, + }) + yield afterEvent - return errorResult + // After yielding, hooks may have modified result + return afterEvent.result } } diff --git a/src/hooks/__tests__/events.test.ts b/src/hooks/__tests__/events.test.ts index c516e7f0..eec7fc26 100644 --- a/src/hooks/__tests__/events.test.ts +++ b/src/hooks/__tests__/events.test.ts @@ -104,10 +104,6 @@ describe('BeforeToolCallEvent', () => { }) // @ts-expect-error verifying that property is readonly event.agent = new Agent() - // @ts-expect-error verifying that property is readonly - event.toolUse = toolUse - // @ts-expect-error verifying that property is readonly - event.tool = tool }) it('creates instance with undefined tool when tool is not found', () => { @@ -133,6 +129,49 @@ describe('BeforeToolCallEvent', () => { const event = new BeforeToolCallEvent({ agent, toolUse, tool: undefined }) expect(event._shouldReverseCallbacks()).toBe(false) }) + + it('allows tool to be modified', () => { + const agent = new Agent() + const tool1 = new FunctionTool({ + name: 'tool1', + description: 'First tool', + inputSchema: {}, + callback: () => 'result1', + }) + const tool2 = new FunctionTool({ + name: 'tool2', + description: 'Second tool', + inputSchema: {}, + callback: () => 'result2', + }) + const toolUse = { name: 'tool1', toolUseId: 'id', input: {} } + const event = new BeforeToolCallEvent({ agent, toolUse, tool: tool1 }) + + // Initially has tool1 + expect(event.tool).toBe(tool1) + + // Can be changed to tool2 + event.tool = tool2 + expect(event.tool).toBe(tool2) + + // Can be set to undefined + event.tool = undefined + expect(event.tool).toBeUndefined() + }) + + it('allows toolUse to be modified', () => { + const agent = new Agent() + const originalToolUse = { name: 'original', toolUseId: 'id-1', input: { key: 'value1' } } + const event = new BeforeToolCallEvent({ agent, toolUse: originalToolUse, tool: undefined }) + + // Initially has original values + expect(event.toolUse).toEqual(originalToolUse) + + // Can be modified + const newToolUse = { name: 'modified', toolUseId: 'id-2', input: { key: 'value2' } } + event.toolUse = newToolUse + expect(event.toolUse).toEqual(newToolUse) + }) }) describe('AfterToolCallEvent', () => { @@ -170,8 +209,6 @@ describe('AfterToolCallEvent', () => { event.toolUse = toolUse // @ts-expect-error verifying that property is readonly event.tool = tool - // @ts-expect-error verifying that property is readonly - event.result = result }) it('creates instance with error property when tool execution fails', () => { @@ -206,6 +243,29 @@ describe('AfterToolCallEvent', () => { const event = new AfterToolCallEvent({ agent, toolUse, tool: undefined, result }) expect(event._shouldReverseCallbacks()).toBe(true) }) + + it('allows result to be modified', () => { + const agent = new Agent() + const toolUse = { name: 'test', toolUseId: 'id', input: {} } + const originalResult = new ToolResultBlock({ + toolUseId: 'id', + status: 'success', + content: [new TextBlock('Original')], + }) + const event = new AfterToolCallEvent({ agent, toolUse, tool: undefined, result: originalResult }) + + // Initially has original result + expect(event.result).toEqual(originalResult) + + // Can be modified to different result + const newResult = new ToolResultBlock({ + toolUseId: 'id', + status: 'error', + content: [new TextBlock('Modified')], + }) + event.result = newResult + expect(event.result).toEqual(newResult) + }) }) describe('BeforeModelCallEvent', () => { diff --git a/src/hooks/events.ts b/src/hooks/events.ts index 2fd7ab83..de09fc20 100644 --- a/src/hooks/events.ts +++ b/src/hooks/events.ts @@ -76,12 +76,12 @@ export class MessageAddedEvent extends HookEvent { export class BeforeToolCallEvent extends HookEvent { readonly type = 'beforeToolCallEvent' as const readonly agent: AgentData - readonly toolUse: { + toolUse: { name: string toolUseId: string input: JSONValue } - readonly tool: Tool | undefined + tool: Tool | undefined constructor(data: { agent: AgentData @@ -109,7 +109,7 @@ export class AfterToolCallEvent extends HookEvent { input: JSONValue } readonly tool: Tool | undefined - readonly result: ToolResultBlock + result: ToolResultBlock readonly error?: Error constructor(data: {