From 828f44d3d6ff403c14b28115d8cfb4a2f3e19bc1 Mon Sep 17 00:00:00 2001 From: Mackenzie Zastrow Date: Mon, 8 Dec 2025 12:08:38 -0500 Subject: [PATCH 1/3] Guide agents(s) to be more concise on issue & PR descriptions --- .github/agent-sops/task-implementer.sop.md | 51 +++++- .github/agent-sops/task-refiner.sop.md | 7 - docs/PR.md | 203 +++++++++++++++++++++ 3 files changed, 247 insertions(+), 14 deletions(-) create mode 100644 docs/PR.md 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? From 1504523755b18d64e7516df4862fda6e0d71931b Mon Sep 17 00:00:00 2001 From: Strands Agent <217235299+strands-agent@users.noreply.github.com> Date: Mon, 8 Dec 2025 17:32:03 +0000 Subject: [PATCH 2/3] feat: enable writable hooks for tool and result modification Remove readonly modifiers from BeforeToolCallEvent (tool, toolUse) and AfterToolCallEvent (result) properties. Agent now reads potentially modified values after yielding hook events, enabling hooks to change tool execution behavior and modify tool results. Resolves #17 --- src/agent/__tests__/agent.hook.test.ts | 363 +++++++++++++++++++++++++ src/agent/agent.ts | 52 ++-- src/hooks/__tests__/events.test.ts | 72 ++++- src/hooks/events.ts | 6 +- 4 files changed, 464 insertions(+), 29 deletions(-) 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..bc1c566f 100644 --- a/src/agent/agent.ts +++ b/src/agent/agent.ts @@ -567,33 +567,40 @@ 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 +608,38 @@ 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: { From 569d8d9425ec404bbad61ee6ee7c6efe4620bc0a Mon Sep 17 00:00:00 2001 From: Strands Agent <217235299+strands-agent@users.noreply.github.com> Date: Mon, 8 Dec 2025 17:34:03 +0000 Subject: [PATCH 3/3] Additional changes from write operations --- src/agent/agent.ts | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/src/agent/agent.ts b/src/agent/agent.ts index bc1c566f..3e989cb9 100644 --- a/src/agent/agent.ts +++ b/src/agent/agent.ts @@ -582,7 +582,12 @@ export class Agent implements AgentData { content: [new TextBlock(`Tool '${actualToolUse.name}' not found in registry`)], }) - const afterEvent = new AfterToolCallEvent({ agent: this, toolUse: actualToolUse, tool: actualTool, result: errorResult }) + const afterEvent = new AfterToolCallEvent({ + agent: this, + toolUse: actualToolUse, + tool: actualTool, + result: errorResult, + }) yield afterEvent // After yielding, hooks may have modified result @@ -613,14 +618,24 @@ export class Agent implements AgentData { content: [new TextBlock(`Tool '${actualToolUse.name}' did not return a result`)], }) - const afterEvent = new AfterToolCallEvent({ agent: this, toolUse: actualToolUse, tool: actualTool, result: errorResult }) + const afterEvent = new AfterToolCallEvent({ + agent: this, + toolUse: actualToolUse, + tool: actualTool, + result: errorResult, + }) yield afterEvent // After yielding, hooks may have modified result return afterEvent.result } - const afterEvent = new AfterToolCallEvent({ agent: this, toolUse: actualToolUse, tool: actualTool, result: toolResult }) + const afterEvent = new AfterToolCallEvent({ + agent: this, + toolUse: actualToolUse, + tool: actualTool, + result: toolResult, + }) yield afterEvent // After yielding, hooks may have modified result @@ -635,7 +650,12 @@ export class Agent implements AgentData { error: toolError, }) - const afterEvent = new AfterToolCallEvent({ agent: this, toolUse: actualToolUse, tool: actualTool, result: errorResult }) + const afterEvent = new AfterToolCallEvent({ + agent: this, + toolUse: actualToolUse, + tool: actualTool, + result: errorResult, + }) yield afterEvent // After yielding, hooks may have modified result