Add browser tools with enhanced Zod schema integration #5
Conversation
🌐 Browser Tools Implementation: - Text-based web browser (SimpleTextBrowser) with viewport pagination - HTML to readable text conversion with JSDOM - Browser navigation tools: visit_page, page_down, page_up, find_on_page_ctrl_f, find_next - Comprehensive error handling and timeout management 🔧 Enhanced Zod Schema Integration: - Schema formatter utility for converting Zod schemas to LLM-friendly prompts - Enhanced tool registry with parameter names, types, descriptions, and examples - Prevents model hallucination of wrong parameter names (e.g., "param1" vs "url") - Improved ReAct prompt clarity with actual parameter names ✅ Features: - Full TypeScript support with proper type safety - Modular architecture following framework patterns - Comprehensive test suite including unit, integration, and live tests - Example usage and documentation - Punycode deprecation warning suppression 🧪 Tests Added: - browser-tools.test.ts - Unit tests for all browser tools - browser-tools-direct.test.ts - Direct tool usage without LLM - browser-tools-integration.test.ts - Full ReAct agent integration - tool-schema-validation.test.ts - Schema validation testing - schema-enhancement-demo.ts - Demo of enhanced schema features 📚 Documentation: - Updated CLAUDE.md with development commands and architecture overview - Browser tools example with usage patterns - Enhanced tool registry documentation This implementation enables AI agents to browse and analyze web content autonomously while maintaining type safety and preventing parameter naming issues.
WalkthroughThis update introduces browser automation tools and a simple text-based browser to the codebase, integrating them as part of the agent's default toolset. It provides new utilities for Zod schema formatting, enhances tool registry cataloging, and adds comprehensive examples and tests for browser tool usage. Documentation and prompt instructions are also updated. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Agent
participant ToolRegistry
participant BrowserTool
participant SimpleTextBrowser
User->>Agent: Submit task (e.g., "Visit a webpage and summarize")
Agent->>ToolRegistry: Retrieve available tools
Agent->>BrowserTool: Invoke tool (e.g., VisitPageTool) with parameters
BrowserTool->>SimpleTextBrowser: Load page, paginate, search, etc.
SimpleTextBrowser-->>BrowserTool: Return page content/metadata
BrowserTool-->>Agent: Return formatted result
Agent-->>User: Respond with answer and tool usage steps
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
test/browser-tools-direct.test.ts (1)
56-56: Remove export from test file.Exporting functions from test files is generally discouraged as it can lead to confusion about the intended use of the code.
-export { testBrowserDirectly };If the function needs to be reused in other tests, consider moving it to a shared test utility module instead.
test/browser-tools-integration.test.ts (1)
87-87: Remove export from test file.Similar to the previous test file, exporting functions from test files is generally discouraged.
-export { testBrowserWithTinyAgent };Consider moving to a shared test utility module if reuse is needed.
CLAUDE.md (1)
1-134: Excellent comprehensive documentation addition!This documentation file provides valuable guidance for developers working with TinyAgent-TS. The coverage of architecture, development commands, and configuration patterns is thorough and well-organized.
However, there are minor grammatical issues to fix:
Apply this diff to fix the grammatical issues:
-- **ReAct Mode**: Full reasoning cycle with tool usage, observation, and multi-step problem solving +- **ReAct Mode**: Full reasoning cycle with tool usage, observation, and multi-step problem-solving
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
CLAUDE.md(1 hunks)examples/browser-tools-example.ts(1 hunks)src/core/prompts/system/react.md(1 hunks)src/index.ts(1 hunks)src/tools/browser-tools.ts(1 hunks)src/tools/default-tools.ts(3 hunks)src/tools/index.ts(1 hunks)src/tools/registry.ts(2 hunks)src/tools/text-browser.ts(1 hunks)src/utils/schema-formatter.ts(1 hunks)test/browser-tools-direct.test.ts(1 hunks)test/browser-tools-integration.test.ts(1 hunks)test/browser-tools.test.ts(1 hunks)test/schema-enhancement-demo.ts(1 hunks)test/tool-schema-validation.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
test/browser-tools-integration.test.ts (2)
src/agent/types.ts (1)
Agent(98-131)src/tools/browser-tools.ts (2)
browserTools(240-247)getBrowserTools(252-254)
src/tools/default-tools.ts (1)
src/tools/browser-tools.ts (1)
getBrowserTools(252-254)
examples/browser-tools-example.ts (2)
src/agent/types.ts (1)
Agent(98-131)src/tools/browser-tools.ts (5)
browserTools(240-247)getBrowserTools(252-254)VisitPageTool(45-72)PageDownTool(77-95)FindOnPageTool(123-156)
test/schema-enhancement-demo.ts (3)
src/agent/types.ts (1)
Agent(98-131)src/tools/browser-tools.ts (2)
browserTools(240-247)getBrowserTools(252-254)src/decorators.ts (1)
tool(66-133)
🪛 Biome (1.9.4)
test/browser-tools-integration.test.ts
[error] 87-87: Do not export from a test file.
(lint/suspicious/noExportsInTest)
src/utils/schema-formatter.ts
[error] 90-90: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 93-93: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 96-96: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 101-101: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 163-163: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 166-166: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
test/browser-tools-direct.test.ts
[error] 56-56: Do not export from a test file.
(lint/suspicious/noExportsInTest)
src/tools/browser-tools.ts
[error] 82-82: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 105-105: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 166-166: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🪛 LanguageTool
CLAUDE.md
[misspelling] ~98-~98: This word is normally spelled as one.
Context: ...cycle with tool usage, observation, and multi-step problem solving ### Tool Development -...
(EN_COMPOUNDS_MULTI_STEP)
[grammar] ~98-~98: This noun or verb “problem-solving” is spelled with a hyphen.
Context: ...tool usage, observation, and multi-step problem solving ### Tool Development - Tools must impl...
(PROBLEM_SOLVE_HYPHEN)
🔇 Additional comments (32)
src/utils/schema-formatter.ts (3)
6-30: LGTM! Well-structured schema formatting function.The function properly handles both object and non-object schemas with appropriate error handling.
35-74: LGTM! Comprehensive type handling with proper unwrapping.The function correctly handles optional, nullable, and default types with proper unwrapping logic. Good fallback error handling.
111-131: LGTM! Clean example extraction implementation.Good error handling and proper type checking for ZodObject schemas.
src/tools/text-browser.ts (6)
64-78: LGTM! Well-structured constructor with sensible defaults.Good default values for viewport size (8KB) and timeout (30s). The initialization properly handles both provided and default start pages.
105-138: LGTM! Robust URL handling with proper relative path resolution.The method correctly handles special URIs, relative paths, and maintains history. Good error handling for invalid URLs.
167-209: LGTM! Well-implemented search functionality with wildcard support.The search implementation includes good features:
- Case-insensitive search
- Wildcard support with
*converted to.*- Wraparound search
- Proper regex error handling with fallback
265-289: LGTM! Smart pagination with word boundary preservation.The pagination logic properly:
- Handles empty content edge case
- Breaks pages at word boundaries for better readability
- Maintains proper start/end indices
342-395: LGTM! Robust fetch implementation with proper timeout handling.Excellent error handling:
- AbortController for request timeout
- Proper cleanup in finally block
- Informative error messages
- Support for both HTML and text content types
397-467: LGTM! Secure and structure-preserving HTML to text conversion.Excellent implementation:
- Removes potentially dangerous elements (script, style, noscript)
- Preserves document structure with headers and paragraphs
- Converts links to markdown format for better readability
- Recursive text extraction handles nested elements properly
src/index.ts (1)
24-24: LGTM! Logical placement of schema formatter export.The export is properly placed with other utility exports.
src/tools/index.ts (1)
12-14: LGTM! Well-organized browser tool exports.The browser tool exports are properly grouped and clearly labeled.
src/core/prompts/system/react.md (1)
9-12: LGTM! Clear improvement to parameter naming instructions.The changes effectively clarify that exact parameter names from tool schemas should be used instead of generic placeholders. This aligns well with the enhanced schema formatting capabilities.
test/tool-schema-validation.test.ts (1)
1-51: LGTM! Comprehensive schema validation test.This test effectively validates the parameter naming enhancement mentioned in the PR objectives. The three test scenarios clearly demonstrate that the tool now uses correct parameter names (
url) instead of generic ones (param1), addressing the core issue described in the PR.The test structure is well-organized with clear logging and proper error handling.
test/schema-enhancement-demo.ts (1)
1-57: LGTM! Excellent demonstration of schema enhancements.This script effectively showcases the enhanced Zod schema integration by comparing different catalog formats. The progression from enhanced → detailed → legacy catalogs provides a clear before-and-after view of the improvements mentioned in the PR objectives.
The secure API key handling using environment variables and comprehensive error handling are well implemented.
test/browser-tools-direct.test.ts (1)
1-49: LGTM! Well-structured direct tool testing.The test provides excellent coverage of browser tools without LLM dependencies. The sequential testing approach (visit → scroll → search) and result truncation for readability are well thought out.
The error handling is comprehensive and the realistic test scenario using
tinyagent.xyzprovides good validation.test/browser-tools-integration.test.ts (3)
1-8: Good approach to warning suppression in test environment.The selective suppression of punycode deprecation warnings while preserving other warnings is a clean solution for test environments where this specific warning isn't actionable.
14-21: Excellent conditional test execution pattern.The environment-based conditional execution (
RUN_LIVEandOPENROUTER_API_KEY) is the right approach for live integration tests that require external API access. The clear logging message helps developers understand test requirements.
24-80: Comprehensive integration test with good error handling.The test effectively validates the full agent workflow with browser tools. The detailed logging, step tracking, and comprehensive error handling provide excellent debugging capabilities for integration issues.
src/tools/default-tools.ts (3)
9-9: LGTM! Clean import of browser tools.The import follows the established pattern and integrates well with the existing tool system.
22-22: Excellent dynamic tool integration pattern.The use of spread operator with name transformation (
tool.name.replace(/_/g, '')) maintains consistency with the existing camelCase naming convention while dynamically incorporating all browser tools. This approach is both maintainable and extensible.
45-55: Well-organized tool categorization.The addition of
websearchto the existingsearchcategory and the newbrowsercategory with comprehensive tool coverage creates a logical organization that aligns with the tool functionality.src/tools/registry.ts (4)
2-2: LGTM! Good integration with schema formatting utilities.The import properly integrates the new schema formatting utilities to enhance catalog generation.
100-112: Excellent enhancement to catalog generation.The updated
getCatalog()method now provides much better schema information and examples, which directly addresses the PR objective of improving parameter naming and schema integration for language models.
114-134: Well-implemented detailed catalog method.The
getDetailedCatalog()method provides comprehensive tool documentation with markdown formatting, making it very useful for detailed tool introspection.
136-143: Good backward compatibility preservation.The
getLegacyCatalog()method maintains the simple legacy format, ensuring existing code that depends on the old catalog format continues to work.examples/browser-tools-example.ts (1)
1-116: Excellent example demonstrating browser tools integration!This example effectively showcases both agent-based and direct usage of browser tools. The implementation demonstrates:
- Proper agent configuration with ReAct mode
- Tool registration and usage patterns
- Good error handling and logging
- Educational task examples for different browsing scenarios
- Clean separation between agent-based and direct tool usage
The code follows best practices and serves as a great reference for users learning to integrate browser tools.
test/browser-tools.test.ts (1)
1-210: Comprehensive and well-structured test suite!This test suite provides excellent coverage of the browser tools functionality:
Strengths:
- Tests all tool properties, validation, and error handling
- Proper use of conditional live tests with
RUN_LIVEenvironment variable- Good integration test patterns combining multiple tools
- Appropriate timeout handling for network operations
- Thorough testing of the SimpleTextBrowser functionality
Good Testing Practices:
- Type assertions for accessing private methods are acceptable for testing
- Proper abort signal testing
- Mock data usage where appropriate
- Clear test organization and descriptions
The test coverage ensures the browser tools work correctly both individually and in integrated workflows.
src/tools/browser-tools.ts (5)
8-21: Good singleton pattern for shared browser instance.The shared browser instance with lazy initialization is well-implemented and provides appropriate configuration defaults.
26-40: Well-designed response formatting.The
formatBrowserResponsefunction provides consistent, readable output formatting across all browser tools with appropriate metadata display.
45-72: Robust VisitPageTool implementation.The tool properly handles validation, abort signals, and error cases while providing clear, formatted output.
194-235: WebSearchTool placeholder is clearly documented.The placeholder implementation is well-documented and provides clear guidance on how to integrate with real search APIs. The mock response structure demonstrates the expected output format.
240-254: Clean export pattern for browser tools.The export structure makes it easy to import either individual tools or the complete set, supporting both use cases effectively.
| description = 'Scroll the viewport DOWN one page-length in the current webpage and return the new viewport content.'; | ||
| schema = z.object({}); | ||
|
|
||
| async execute(args: {}, abortSignal?: AbortSignal): Promise<string> { |
There was a problem hiding this comment.
Fix the banned type usage.
Static analysis correctly identifies that {} as a type should be avoided. Use more explicit type definitions.
Apply this diff to fix the type annotations:
- async execute(args: {}, abortSignal?: AbortSignal): Promise<string> {
+ async execute(args: Record<string, never>, abortSignal?: AbortSignal): Promise<string> {Apply the same fix to lines 105 and 166 for PageUpTool and FindNextTool.
Also applies to: 105-105, 166-166
🧰 Tools
🪛 Biome (1.9.4)
[error] 82-82: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🤖 Prompt for AI Agents
In src/tools/browser-tools.ts at lines 82, 105, and 166, the execute method uses
the banned type annotation {} for the args parameter. Replace {} with a more
explicit and appropriate type that accurately describes the expected structure
of args for each method. Review the usage context to define or import a proper
interface or type alias instead of using the empty object type.
…t config - Wrap switch statement variable declarations in blocks to prevent scope issues - Remove unnecessary exports from test files - Add ignore patterns for dist/ and other generated files in ESLint config - Auto-fix formatting issues with Prettier 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
src/tools/browser-tools.ts (1)
91-91: Fix the banned type usage.This is a duplicate of a previous review comment. The
{}type should be replaced with a more explicit type.Also applies to: 117-117, 194-194
🧹 Nitpick comments (6)
src/default-tools/duckduckgo-search.tool.ts (1)
85-86: Remove unnecessary continue statement.The
continuestatement is unnecessary since it's at the end of the loop iteration and the loop would naturally continue to the next iteration.Apply this diff to remove the unnecessary statement:
- await new Promise((resolve) => setTimeout(resolve, 1000)); - continue; + await new Promise((resolve) => setTimeout(resolve, 1000));src/decorators.ts (1)
88-88: Address variable shadowing warnings.The variables named
constructorshadow the globalconstructorproperty, which could be confusing. Consider using more descriptive names.Apply this refactor to improve clarity:
// Add an initializer that will run when the class is instantiated if (addInitializer) { addInitializer(function (this: any) { // 'this' refers to the class instance - const constructor = this.constructor; + const classConstructor = this.constructor; // Store metadata on the class constructor const list: ToolMetadata[] = - Reflect.getMetadata(META_KEYS.TOOLS, constructor) || []; + Reflect.getMetadata(META_KEYS.TOOLS, classConstructor) || []; // Check if this tool is already registered to avoid duplicates if (!list.some((tool) => tool.name === String(name))) { list.push({ name: String(name), description, method: String(name), schema: paramSchema, }); - Reflect.defineMetadata(META_KEYS.TOOLS, list, constructor); + Reflect.defineMetadata(META_KEYS.TOOLS, list, classConstructor); } }); } // Fallback for older TypeScript versions that don't support addInitializer const target = targetOrClassMethod; -const constructor = target.constructor; +const targetConstructor = target.constructor; // Also register directly on the constructor as a fallback const directList: ToolMetadata[] = - Reflect.getMetadata(META_KEYS.TOOLS, constructor) || []; + Reflect.getMetadata(META_KEYS.TOOLS, targetConstructor) || []; if (!directList.some((tool) => tool.name === String(name))) { directList.push({ name: String(name), description, method: String(name), schema: paramSchema, }); - Reflect.defineMetadata(META_KEYS.TOOLS, directList, constructor); + Reflect.defineMetadata(META_KEYS.TOOLS, directList, targetConstructor); }Also applies to: 109-109
llm-agent-tools/setup-multi-agent.sh (1)
82-88:treefallback is good – but permissions may be too open
chmod -R 755 "$BANK_DIR"makes every file world-readable and world-executable.
Even if the memory-bank is only on a dev box, it will contain scratchpads, context dumps and possibly API keys that individual agents consider private.-chmod -R 755 "$BANK_DIR" +# Allow owner + group read/write, deny others by default. +# Change to 755 only in the sub-trees that really need to be public. +chmod -R go-rwx "$BANK_DIR" +chmod -R a+rx "$BANK_DIR"/sharedllm-agent-tools/researcher.sh (1)
262-276: Config file written world-readable – protect the API keyAfter
set-key,config.jsoninherits your umask (often644). Restrict it explicitly:- jq --arg key "$api_key" '.api_key = $key' "$CONFIG_FILE" > "$CONFIG_FILE.tmp" && \ - mv "$CONFIG_FILE.tmp" "$CONFIG_FILE" + jq --arg key "$api_key" '.api_key = $key' "$CONFIG_FILE" > "$CONFIG_FILE.tmp" && + mv "$CONFIG_FILE.tmp" "$CONFIG_FILE" && + chmod 600 "$CONFIG_FILE"llm-agent-tools/codemap.sh (1)
73-73: Fix the variable declaration to avoid masking return values.The shellcheck warning is valid. Declare and assign the variable separately to avoid masking the return value of
basename.- local base=$(basename "$file") + local base + base=$(basename "$file")llm-agent-tools/knowledge.sh (1)
98-98: Fix variable declaration to avoid masking return values.The shellcheck warning is valid. Declare and assign the timestamp variable separately to avoid masking return values.
- local timestamp=$(date -u +"%Y-%m-%dT%H:%M:%SZ") + local timestamp + timestamp=$(date -u +"%Y-%m-%dT%H:%M:%SZ")Also applies to: 118-118
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
memory-bank/locks/default.lockis excluded by!**/*.lock
📒 Files selected for processing (73)
eslint.config.js(1 hunks)examples/browser-tools-example.ts(1 hunks)examples/custom-tool-react-test.ts(3 hunks)examples/custom-tools-example.ts(2 hunks)examples/debug-modes.ts(1 hunks)examples/minimal-react-test.ts(1 hunks)examples/modes-example.ts(3 hunks)examples/python-integration-example.ts(1 hunks)examples/react-demo.ts(2 hunks)examples/simple-agent.ts(1 hunks)examples/simple-react-agent.ts(1 hunks)examples/test-imports.ts(2 hunks)llm-agent-tools/agent_tools_prompt.xml(1 hunks)llm-agent-tools/codemap.sh(1 hunks)llm-agent-tools/context.sh(1 hunks)llm-agent-tools/knowledge.sh(1 hunks)llm-agent-tools/researcher.sh(1 hunks)llm-agent-tools/scratchpad-multi.sh(1 hunks)llm-agent-tools/setup-multi-agent.sh(1 hunks)memory-bank/agents/default/knowledge.json(1 hunks)memory-bank/codemap/README.md(1 hunks)memory-bank/codemap/metadata/modules.json(1 hunks)memory-bank/shared/done_tasks/default_Clean_up_browser_tools_implementation_for_PR_-_move_tests,_remove_API_keys,_create_branch_2025-06-26_182234_scratchpad.md(1 hunks)memory-bank/shared/done_tasks/default_Enhance_ReAct_system_with_Zod_schema_integration_for_better_model_guidance_2025-06-26_181609_scratchpad.md(1 hunks)memory-bank/shared/knowledge_base.json(1 hunks)rollup.config.js(1 hunks)src/agent/index.ts(1 hunks)src/agent/types.ts(5 hunks)src/agent/unified-agent.ts(8 hunks)src/decorators.ts(3 hunks)src/default-tools/duckduckgo-search.tool.ts(3 hunks)src/index.ts(2 hunks)src/model/index.ts(1 hunks)src/model/model-manager.ts(6 hunks)src/model/openrouter-provider.ts(4 hunks)src/model/types.ts(2 hunks)src/promptEngine.ts(1 hunks)src/react/engine.ts(11 hunks)src/react/index.ts(1 hunks)src/react/parser.ts(3 hunks)src/react/schemas.ts(2 hunks)src/react/state.ts(4 hunks)src/react/types.ts(2 hunks)src/toolPresets.ts(1 hunks)src/tools/browser-tools.ts(1 hunks)src/tools/default-tools.ts(4 hunks)src/tools/duckduckgo-search-tool.ts(4 hunks)src/tools/file-tool.ts(5 hunks)src/tools/final-answer.ts(1 hunks)src/tools/grep-tool.ts(3 hunks)src/tools/human-loop-tool.ts(3 hunks)src/tools/index.ts(1 hunks)src/tools/pythonExec.ts(1 hunks)src/tools/registry.ts(5 hunks)src/tools/text-browser.ts(1 hunks)src/tools/types.ts(5 hunks)src/tools/uuid-tool.ts(2 hunks)src/utils/schema-formatter.ts(1 hunks)test-cli.js(1 hunks)test/browser-tools-direct.test.ts(1 hunks)test/browser-tools-integration.test.ts(1 hunks)test/browser-tools.test.ts(1 hunks)test/examples.test.ts(2 hunks)test/file-tool.test.ts(0 hunks)test/golden-master-react.test.ts(1 hunks)test/golden-master.test.ts(1 hunks)test/pythonExec.test.ts(6 hunks)test/react-agent.test.ts(3 hunks)test/react-final-answer.test.ts(2 hunks)test/schema-enhancement-demo.ts(1 hunks)test/simple-agent.test.ts(4 hunks)test/tool-schema-validation.test.ts(1 hunks)types/globals.d.ts(1 hunks)
💤 Files with no reviewable changes (1)
- test/file-tool.test.ts
✅ Files skipped from review due to trivial changes (43)
- src/agent/index.ts
- memory-bank/shared/knowledge_base.json
- examples/python-integration-example.ts
- src/react/index.ts
- src/model/index.ts
- test-cli.js
- rollup.config.js
- src/model/types.ts
- examples/minimal-react-test.ts
- examples/test-imports.ts
- examples/simple-agent.ts
- src/toolPresets.ts
- src/tools/final-answer.ts
- examples/modes-example.ts
- src/tools/grep-tool.ts
- memory-bank/codemap/metadata/modules.json
- examples/react-demo.ts
- examples/custom-tool-react-test.ts
- src/promptEngine.ts
- memory-bank/agents/default/knowledge.json
- test/react-agent.test.ts
- src/tools/human-loop-tool.ts
- test/simple-agent.test.ts
- src/model/model-manager.ts
- src/tools/pythonExec.ts
- src/react/schemas.ts
- src/react/parser.ts
- memory-bank/codemap/README.md
- src/react/state.ts
- types/globals.d.ts
- src/agent/types.ts
- src/tools/file-tool.ts
- src/react/types.ts
- test/react-final-answer.test.ts
- src/react/engine.ts
- test/tool-schema-validation.test.ts
- src/tools/uuid-tool.ts
- src/model/openrouter-provider.ts
- memory-bank/shared/done_tasks/default_Enhance_ReAct_system_with_Zod_schema_integration_for_better_model_guidance_2025-06-26_181609_scratchpad.md
- test/examples.test.ts
- src/tools/duckduckgo-search-tool.ts
- examples/debug-modes.ts
- examples/custom-tools-example.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- src/index.ts
- src/tools/index.ts
- src/tools/registry.ts
- test/browser-tools-integration.test.ts
- test/schema-enhancement-demo.ts
- test/browser-tools-direct.test.ts
- src/tools/default-tools.ts
- src/utils/schema-formatter.ts
- test/browser-tools.test.ts
- examples/browser-tools-example.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
llm-agent-tools/researcher.sh (2)
src/model/model-manager.ts (1)
chat(64-120)src/model/openrouter-provider.ts (1)
chat(36-100)
🪛 Biome (1.9.4)
src/agent/unified-agent.ts
[error] 101-101: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
src/decorators.ts
[error] 88-88: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
[error] 109-109: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
src/default-tools/duckduckgo-search.tool.ts
[error] 86-86: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
src/tools/browser-tools.ts
[error] 91-91: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 117-117: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 194-194: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🪛 Shellcheck (0.10.0)
llm-agent-tools/researcher.sh
[warning] 77-77: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 79-79: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 88-88: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 101-101: Declare and assign separately to avoid masking return values.
(SC2155)
llm-agent-tools/codemap.sh
[warning] 73-73: Declare and assign separately to avoid masking return values.
(SC2155)
llm-agent-tools/knowledge.sh
[warning] 98-98: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 118-118: Declare and assign separately to avoid masking return values.
(SC2155)
🪛 LanguageTool
memory-bank/shared/done_tasks/default_Clean_up_browser_tools_implementation_for_PR_-_move_tests,_remove_API_keys,_create_branch_2025-06-26_182234_scratchpad.md
[uncategorized] ~5-~5: You might be missing the article “the” here.
Context: ...gent: default [1] Moving test files to proper test directory and removing hardcoded A...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~6-~6: You might be missing the article “the” here.
Context: ...rdcoded API keys [2] Moved all tests to test directory, now creating new branch for ...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~6-~6: You might be missing the article “a” here.
Context: ...l tests to test directory, now creating new branch for browser tools feature [3] SU...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
🔇 Additional comments (34)
test/pythonExec.test.ts (1)
7-7: LGTM! Consistent trailing comma formatting.The addition of trailing commas after
timeoutMsproperties improves code consistency and makes future diffs cleaner when adding new properties.Also applies to: 19-19, 33-33, 46-46, 55-55, 69-69, 92-92, 105-105, 112-112, 119-119
eslint.config.js (1)
7-9: LGTM! Standard ESLint ignore patterns.The ignore configuration appropriately excludes build artifacts (
dist/), dependencies (node_modules/), and TypeScript declaration files (*.d.ts) from linting, which is a standard best practice.src/default-tools/duckduckgo-search.tool.ts (2)
21-25: LGTM! Improved schema formatting.The multi-line chained call formatting for the
safeSearchenum enhances readability while maintaining the same functionality.
34-36: LGTM! Better string formatting.Splitting the description string across multiple lines improves readability and maintainability.
src/tools/types.ts (1)
9-9: LGTM! Consistent whitespace formatting.The whitespace cleanup and addition of proper line breaks improves code consistency and follows TypeScript formatting conventions without affecting any functionality.
Also applies to: 12-12, 15-15, 72-72, 77-77, 82-82, 87-87, 92-92, 106-106, 115-115, 125-125, 137-137
memory-bank/shared/done_tasks/default_Clean_up_browser_tools_implementation_for_PR_-_move_tests,_remove_API_keys,_create_branch_2025-06-26_182234_scratchpad.md (1)
1-8: LGTM! Clear documentation of PR cleanup activities.The scratchpad effectively documents the key cleanup steps: test file organization, API key removal, and branch creation. This type of documentation helps track PR preparation activities and provides context for the changes.
examples/simple-react-agent.ts (3)
1-9: Well-structured example demonstrating ReAct agent capabilities.The setup is clean with proper imports, environment configuration, and tool registration. The use of
getDefaultTools()withforEachregistration is appropriate for this example.
11-13: Clear multi-step task definition.The task prompt effectively demonstrates multi-tool orchestration by combining UUID generation and Python calculation. The numbered steps provide clear guidance for the agent.
14-19: Proper result handling and output formatting.The result handling includes success status checking and properly formatted JSON output, which is helpful for debugging and understanding agent execution.
src/agent/unified-agent.ts (2)
5-10: Improved import formatting enhances readability.The multi-line import formatting makes the code more readable and easier to maintain.
137-140: Improved conditional expression formatting.The multi-line formatting of conditional expressions improves readability and makes the logic flow clearer.
Also applies to: 159-162, 171-174
test/golden-master.test.ts (1)
30-30: Verify shell script exists.The test assumes the shell script exists but doesn't verify it. Consider adding a check to ensure the script is executable.
Run this script to verify the shell script exists:
#!/bin/bash # Check if the shell script exists and is executable test_script="test/run-simple-agent.sh" if [ -f "$test_script" ]; then echo "✓ Script exists: $test_script" if [ -x "$test_script" ]; then echo "✓ Script is executable" else echo "⚠ Script is not executable" fi else echo "✗ Script not found: $test_script" fitest/golden-master-react.test.ts (1)
26-26: Verify shell script exists.Same as the previous golden master test - verify the shell script exists and is executable.
Run this script to verify both shell scripts exist:
#!/bin/bash # Check if both shell scripts exist and are executable scripts=("test/run-simple-agent.sh" "test/run-simple-react-agent.sh") for script in "${scripts[@]}"; do if [ -f "$script" ]; then echo "✓ Script exists: $script" if [ -x "$script" ]; then echo "✓ Script is executable" else echo "⚠ Script is not executable" fi else echo "✗ Script not found: $script" fi donesrc/decorators.ts (1)
71-74: Improved formatting enhances readability.The reformatted function signatures, conditional checks, and variable declarations make the code more readable and easier to maintain.
Also applies to: 76-80, 91-95
llm-agent-tools/scratchpad-multi.sh (1)
154-159: Step text loses blank lines & leading spaces
sed "s/^/[${next}] /"prefixes every line but also strips leading whitespace
matching the regex anchor. Consider usingprintfwith explicit format instead
to avoid mangling markdown / code blocks.llm-agent-tools/agent_tools_prompt.xml (1)
1-151: Prompt file looks good – no actionable issues from a tooling perspectiveSchema is static documentation; nothing blocking.
llm-agent-tools/codemap.sh (3)
58-82: Excellent modular helper functions with robust file type detection.The
init_dirs()anddetect_file_type()functions are well-implemented with comprehensive pattern matching for different file types. The initialization properly handles missing files and directories.
84-105: Smart dependency extraction with multi-language support.The
extract_deps()function effectively handles JavaScript/TypeScript and Python import patterns with proper error handling and filtering. The regex patterns are appropriate for extracting dependencies.
137-173: Efficient automated project scanning with duplicate prevention.The
mapcommand implementation efficiently scans the codebase, skips already processed files, and provides good progress feedback. The file filtering excludes appropriate directories likenode_modulesand.git.src/tools/browser-tools.ts (4)
13-21: Efficient shared browser instance pattern with good configuration.The singleton pattern for the browser instance is well-implemented with sensible defaults (8KB viewport, 30s timeout). This approach avoids unnecessary instantiation overhead.
26-40: Comprehensive browser response formatting with metadata.The
formatBrowserResponse()function provides excellent user experience by including address, title, visit history, and viewport positioning information in a clear format.
167-174: Robust error handling for search failures.The search not found logic properly handles cases where the search string isn't found and provides clear feedback to the user, maintaining the browser state context.
252-277: Clear placeholder implementation for web search.The mock search implementation properly documents the need for real search API integration and provides a functional placeholder that won't break the system. The comment clearly indicates required integrations.
src/tools/text-browser.ts (7)
1-3: Excellent dependency choices and deprecation avoidance.Using the
node:prefix for the URL import properly avoids punycode deprecation warnings. The dependency selection (node-fetch, JSDOM) is appropriate for text-based web browsing.
64-79: Well-designed constructor with sensible defaults.The constructor provides good configurability with reasonable defaults (8KB viewport, 30s timeout) and handles the initial state properly including the
about:blankfallback.
284-310: Sophisticated viewport pagination with word boundary preservation.The
splitPages()implementation is excellent - it maintains word boundaries to prevent text fragmentation while efficiently chunking content into viewport-sized pages.
324-367: Robust search implementation with wildcard support and wraparound.The search functionality is well-implemented with regex support for wildcards, proper viewport wraparound, and graceful fallback when regex compilation fails. The circular search pattern ensures all content is searchable.
372-425: Comprehensive HTTP client with proper timeout and error handling.The fetch implementation includes abort controller for timeouts, proper HTTP status checking, content-type handling, and clear error messaging. The timeout cleanup is handled correctly in both success and error paths.
438-442: Security-conscious HTML parsing with script removal.Properly removes script, style, and noscript elements before text extraction, which is essential for security and clean text output in a text-based browser context.
468-511: Thoughtful text extraction preserving document structure.The
extractTextWithStructure()method intelligently preserves semantic structure (headers, paragraphs, links) while converting to readable text. The markdown-style link formatting is particularly useful.llm-agent-tools/knowledge.sh (4)
28-40: Robust command-line argument parsing with agent support.The parameter parsing correctly handles the
--agentflag while maintaining backward compatibility. The error handling for unknown options is appropriate.
94-111: Well-structured knowledge storage with comprehensive metadata.The
store_knowledge()function creates rich metadata including timestamps, agent attribution, and tag arrays. The JSON structure is well-designed for multi-agent collaboration.
255-290: Intelligent tag-based synchronization system.The sync command implements sophisticated logic to pull shared knowledge based on matching tags from private knowledge, preventing conflicts and enabling selective knowledge sharing.
292-331: Comprehensive export functionality with tag-aware filtering.The export command creates well-formatted markdown with both private and relevant shared knowledge, using tag matching to include only pertinent shared entries.
| case 'react': | ||
| const reactResult = await this.executeReAct(input, options); | ||
| result = reactResult.finalAnswer; | ||
| steps = reactResult.steps; | ||
| break; |
There was a problem hiding this comment.
Fix switch clause variable declaration issue.
The variable declaration within the switch case should be wrapped in a block to prevent potential access issues from other cases.
Apply this fix:
case 'react':
- const reactResult = await this.executeReAct(input, options);
- result = reactResult.finalAnswer;
- steps = reactResult.steps;
- break;
+ {
+ const reactResult = await this.executeReAct(input, options);
+ result = reactResult.finalAnswer;
+ steps = reactResult.steps;
+ break;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case 'react': | |
| const reactResult = await this.executeReAct(input, options); | |
| result = reactResult.finalAnswer; | |
| steps = reactResult.steps; | |
| break; | |
| case 'react': | |
| { | |
| const reactResult = await this.executeReAct(input, options); | |
| result = reactResult.finalAnswer; | |
| steps = reactResult.steps; | |
| break; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 101-101: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
In src/agent/unified-agent.ts around lines 100 to 104, the variable declaration
inside the 'react' case of the switch statement is not enclosed in a block,
which can cause scope issues. Wrap the code inside the 'react' case with curly
braces {} to create a block scope, ensuring the declared variables do not leak
or conflict with other cases.
| it('should match the golden master output', async () => { | ||
| return new Promise<void>((resolve, _reject) => { | ||
| exec(`${__dirname}/run-simple-agent.sh`, (_error, _stdout, _stderr) => { | ||
| const actualOutput = consoleOutput.join('\n').trim(); | ||
| let expectedOutput = ''; | ||
|
|
||
| if (fs.existsSync(GOLDEN_MASTER_FILE)) { | ||
| expectedOutput = fs.readFileSync(GOLDEN_MASTER_FILE, 'utf8').trim(); | ||
| } else { | ||
| // If golden master file doesn't exist, create it with current output | ||
| fs.writeFileSync(GOLDEN_MASTER_FILE, actualOutput); | ||
| console.log(`Golden master file created at ${GOLDEN_MASTER_FILE}`); | ||
| expectedOutput = actualOutput; | ||
| } | ||
|
|
||
| // We need to ignore the UUID part of the output as it changes every time | ||
| const actualOutputWithoutUUID = actualOutput.replace( | ||
| /UUID: [0-9a-fA-F-]{36}/, | ||
| 'UUID: [UUID_PLACEHOLDER]' | ||
| ); | ||
| const expectedOutputWithoutUUID = expectedOutput.replace( | ||
| /UUID: [0-9a-fA-F-]{36}/, | ||
| 'UUID: [UUID_PLACEHOLDER]' | ||
| ); | ||
|
|
||
| expect(actualOutputWithoutUUID).toEqual(expectedOutputWithoutUUID); | ||
| resolve(); | ||
| }); | ||
| }); | ||
| }, 30000); // Increase timeout for agent execution |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and test robustness.
The golden master test implementation has several areas for improvement:
- Error handling: The
execcallback ignores errors, which could hide test failures - UUID normalization: The regex pattern could be more robust
- Async handling: Consider using async/await for cleaner code
Apply this refactor for better error handling and robustness:
it('should match the golden master output', async () => {
- return new Promise<void>((resolve, _reject) => {
- exec(`${__dirname}/run-simple-agent.sh`, (_error, _stdout, _stderr) => {
+ return new Promise<void>((resolve, reject) => {
+ exec(`${__dirname}/run-simple-agent.sh`, (error, _stdout, stderr) => {
+ if (error) {
+ reject(new Error(`Script execution failed: ${error.message}`));
+ return;
+ }
+ if (stderr) {
+ console.warn('Script stderr:', stderr);
+ }
+
const actualOutput = consoleOutput.join('\n').trim();
let expectedOutput = '';
if (fs.existsSync(GOLDEN_MASTER_FILE)) {
expectedOutput = fs.readFileSync(GOLDEN_MASTER_FILE, 'utf8').trim();
} else {
fs.writeFileSync(GOLDEN_MASTER_FILE, actualOutput);
console.log(`Golden master file created at ${GOLDEN_MASTER_FILE}`);
expectedOutput = actualOutput;
}
- // We need to ignore the UUID part of the output as it changes every time
const actualOutputWithoutUUID = actualOutput.replace(
- /UUID: [0-9a-fA-F-]{36}/,
+ /UUID: [0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}/g,
'UUID: [UUID_PLACEHOLDER]'
);
const expectedOutputWithoutUUID = expectedOutput.replace(
- /UUID: [0-9a-fA-F-]{36}/,
+ /UUID: [0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}/g,
'UUID: [UUID_PLACEHOLDER]'
);
expect(actualOutputWithoutUUID).toEqual(expectedOutputWithoutUUID);
resolve();
});
});
}, 30000);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should match the golden master output', async () => { | |
| return new Promise<void>((resolve, _reject) => { | |
| exec(`${__dirname}/run-simple-agent.sh`, (_error, _stdout, _stderr) => { | |
| const actualOutput = consoleOutput.join('\n').trim(); | |
| let expectedOutput = ''; | |
| if (fs.existsSync(GOLDEN_MASTER_FILE)) { | |
| expectedOutput = fs.readFileSync(GOLDEN_MASTER_FILE, 'utf8').trim(); | |
| } else { | |
| // If golden master file doesn't exist, create it with current output | |
| fs.writeFileSync(GOLDEN_MASTER_FILE, actualOutput); | |
| console.log(`Golden master file created at ${GOLDEN_MASTER_FILE}`); | |
| expectedOutput = actualOutput; | |
| } | |
| // We need to ignore the UUID part of the output as it changes every time | |
| const actualOutputWithoutUUID = actualOutput.replace( | |
| /UUID: [0-9a-fA-F-]{36}/, | |
| 'UUID: [UUID_PLACEHOLDER]' | |
| ); | |
| const expectedOutputWithoutUUID = expectedOutput.replace( | |
| /UUID: [0-9a-fA-F-]{36}/, | |
| 'UUID: [UUID_PLACEHOLDER]' | |
| ); | |
| expect(actualOutputWithoutUUID).toEqual(expectedOutputWithoutUUID); | |
| resolve(); | |
| }); | |
| }); | |
| }, 30000); // Increase timeout for agent execution | |
| it('should match the golden master output', async () => { | |
| return new Promise<void>((resolve, reject) => { | |
| exec(`${__dirname}/run-simple-agent.sh`, (error, _stdout, stderr) => { | |
| if (error) { | |
| reject(new Error(`Script execution failed: ${error.message}`)); | |
| return; | |
| } | |
| if (stderr) { | |
| console.warn('Script stderr:', stderr); | |
| } | |
| const actualOutput = consoleOutput.join('\n').trim(); | |
| let expectedOutput = ''; | |
| if (fs.existsSync(GOLDEN_MASTER_FILE)) { | |
| expectedOutput = fs.readFileSync(GOLDEN_MASTER_FILE, 'utf8').trim(); | |
| } else { | |
| // If golden master file doesn't exist, create it with current output | |
| fs.writeFileSync(GOLDEN_MASTER_FILE, actualOutput); | |
| console.log(`Golden master file created at ${GOLDEN_MASTER_FILE}`); | |
| expectedOutput = actualOutput; | |
| } | |
| const actualOutputWithoutUUID = actualOutput.replace( | |
| /UUID: [0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}/g, | |
| 'UUID: [UUID_PLACEHOLDER]' | |
| ); | |
| const expectedOutputWithoutUUID = expectedOutput.replace( | |
| /UUID: [0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}/g, | |
| 'UUID: [UUID_PLACEHOLDER]' | |
| ); | |
| expect(actualOutputWithoutUUID).toEqual(expectedOutputWithoutUUID); | |
| resolve(); | |
| }); | |
| }); | |
| }, 30000); // Increase timeout for agent execution |
🤖 Prompt for AI Agents
In test/golden-master.test.ts around lines 28 to 57, improve the golden master
test by adding proper error handling in the exec callback to fail the test if an
error occurs, enhance the UUID regex to more robustly match UUID formats, and
refactor the test to use async/await instead of a Promise constructor for
cleaner asynchronous code. Specifically, check for errors in the exec callback
and reject or throw accordingly, update the UUID regex to cover all valid UUID
variants, and rewrite the test function as async to await exec wrapped in a
promise.
| it('should match the golden master output', async () => { | ||
| return new Promise<void>((resolve, _reject) => { | ||
| exec( | ||
| `${__dirname}/run-simple-react-agent.sh`, | ||
| (_error, _stdout, _stderr) => { | ||
| const actualOutput = consoleOutput.join('\n').trim(); | ||
| let expectedOutput = ''; | ||
| if (fs.existsSync(GOLDEN_MASTER_FILE)) { | ||
| expectedOutput = fs.readFileSync(GOLDEN_MASTER_FILE, 'utf8').trim(); | ||
| } else { | ||
| fs.writeFileSync(GOLDEN_MASTER_FILE, actualOutput); | ||
| console.log(`Golden master file created at ${GOLDEN_MASTER_FILE}`); | ||
| expectedOutput = actualOutput; | ||
| } | ||
| const actualOutputWithoutUUID = actualOutput.replace( | ||
| /UUID: [0-9a-fA-F-]{36}/, | ||
| 'UUID: [UUID_PLACEHOLDER]' | ||
| ); | ||
| const expectedOutputWithoutUUID = expectedOutput.replace( | ||
| /UUID: [0-9a-fA-F-]{36}/, | ||
| 'UUID: [UUID_PLACEHOLDER]' | ||
| ); | ||
| expect(actualOutputWithoutUUID).toEqual(expectedOutputWithoutUUID); | ||
| resolve(); | ||
| } | ||
| ); | ||
| }); | ||
| }, 30000); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same issues as golden-master.test.ts - consider refactoring.
This test has identical issues to the previous golden master test:
- Error handling: The
execcallback ignores errors - UUID normalization: The regex pattern could be more robust
- Code duplication: Both golden master tests share the same logic
Consider creating a shared helper function to reduce duplication:
// test/utils/golden-master-helper.ts
export async function runGoldenMasterTest(
scriptName: string,
goldenMasterFile: string,
consoleOutput: string[]
): Promise<void> {
return new Promise<void>((resolve, reject) => {
exec(`${__dirname}/${scriptName}`, (error, _stdout, stderr) => {
if (error) {
reject(new Error(`Script execution failed: ${error.message}`));
return;
}
if (stderr) {
console.warn('Script stderr:', stderr);
}
const actualOutput = consoleOutput.join('\n').trim();
let expectedOutput = '';
if (fs.existsSync(goldenMasterFile)) {
expectedOutput = fs.readFileSync(goldenMasterFile, 'utf8').trim();
} else {
fs.writeFileSync(goldenMasterFile, actualOutput);
console.log(`Golden master file created at ${goldenMasterFile}`);
expectedOutput = actualOutput;
}
const normalizeUUID = (text: string) => text.replace(
/UUID: [0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}/g,
'UUID: [UUID_PLACEHOLDER]'
);
expect(normalizeUUID(actualOutput)).toEqual(normalizeUUID(expectedOutput));
resolve();
});
});
}🤖 Prompt for AI Agents
In test/golden-master-react.test.ts around lines 23 to 51, the test lacks proper
error handling for the exec callback, uses a less precise UUID regex, and
duplicates logic found in another golden master test. Refactor by creating a
shared helper function that runs the script, handles errors by rejecting the
promise if exec returns an error, logs any stderr output, reads or creates the
golden master file, and normalizes UUIDs using a more robust regex pattern
matching the full UUID format. Replace the existing test logic with a call to
this helper to reduce duplication and improve maintainability.
| BANK_DIR="memory-bank" | ||
| CONTEXT_DIR="$BANK_DIR/context" | ||
| CONTEXT_FILE="$CONTEXT_DIR/context.md" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Allow $BANK_DIR env override for consistency
Other tools (setup-multi-agent.sh, scratchpad-multi.sh) respect BANK_DIR if it’s already exported.
Hard-coding memory-bank here breaks deployments that use a custom location.
-BANK_DIR="memory-bank"
+BANK_DIR="${BANK_DIR:-memory-bank}"🤖 Prompt for AI Agents
In llm-agent-tools/context.sh around lines 16 to 19, the BANK_DIR variable is
hard-coded to "memory-bank", which prevents overriding it via environment
variables. Modify the assignment to allow BANK_DIR to be set externally by using
a default value assignment pattern, such as BANK_DIR="${BANK_DIR:-memory-bank}",
so that if BANK_DIR is already exported, it will be respected; then update
CONTEXT_DIR and CONTEXT_FILE accordingly to use the possibly overridden
BANK_DIR.
| # -------------------- add -------------------- | ||
| add) | ||
| [[ $# -ge 1 ]] || usage | ||
| command_to_run="$*" | ||
| { | ||
| echo "---" | ||
| echo "### \\\`$command_to_run\\\`" | ||
| echo "\`\`\`" | ||
| eval "$command_to_run" | ||
| echo "\`\`\`" | ||
| echo | ||
| } >> "$CONTEXT_FILE" | ||
| echo "Added output of '$command_to_run' to context." | ||
| ;; |
There was a problem hiding this comment.
Missing “active file” guard – add* can silently create a stray file
If the user forgets to run start first, add, add-file or add-text will happily create
memory-bank/context/context.md with no header, breaking the finish logic.
Add a simple check reused by all mutating commands:
+# Fail fast if no active context file
+require_active() {
+ [[ -f "$CONTEXT_FILE" ]] || { echo "ERROR: no active context – run '$0 start <title>'" >&2; exit 1; }
+}and call require_active at the top of the add, add-file, add-text, and finish branches.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In llm-agent-tools/context.sh around lines 57 to 70, the add command can create
the context file without verifying if the active context has been started, which
may cause issues later. Define a reusable function require_active that checks if
the context file exists and exits with an error if not. Then, call
require_active at the beginning of the add, add-file, add-text, and finish
command branches to prevent creating or modifying the context file without an
active session.
| messages='[{"role": "user", "content": "'"$prompt"'"}]' | ||
| response=$(api_call "$model" "$messages") | ||
|
|
||
| if [[ -n "$response" ]]; then | ||
| echo "$response" | ||
| save_to_history "$prompt" "$response" | ||
| fi |
There was a problem hiding this comment.
Unsafe string interpolation → malformed / exploitable JSON payloads
$prompt is embedded directly into the JSON. Any double-quote, backslash or newline in the
user’s prompt will break the JSON or, worse, let an attacker inject extra fields.
- messages='[{"role": "user", "content": "'"$prompt"'"}]'
+ messages=$(jq -n --arg p "$prompt" '[{role:"user",content:$p}]')Use the same pattern for every place you construct messages, including research, analyze-image and the chat loop.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| messages='[{"role": "user", "content": "'"$prompt"'"}]' | |
| response=$(api_call "$model" "$messages") | |
| if [[ -n "$response" ]]; then | |
| echo "$response" | |
| save_to_history "$prompt" "$response" | |
| fi | |
| messages=$(jq -n --arg p "$prompt" '[{role:"user",content:$p}]') | |
| response=$(api_call "$model" "$messages") | |
| if [[ -n "$response" ]]; then | |
| echo "$response" | |
| save_to_history "$prompt" "$response" | |
| fi |
🤖 Prompt for AI Agents
In llm-agent-tools/researcher.sh around lines 123 to 129, the user prompt is
directly embedded into the JSON string without escaping, which can break the
JSON structure or allow injection attacks. To fix this, properly escape special
characters like double quotes, backslashes, and newlines in the prompt before
embedding it into the JSON. Apply this escaping pattern consistently wherever
the messages JSON is constructed, including in research, analyze-image, and the
chat loop sections.
| api_call() { | ||
| local model="${1}" | ||
| local messages="${2}" | ||
| local api_key=$(get_api_key) | ||
|
|
||
| local response=$(curl -s -X POST "$API_URL" \ | ||
| -H "Authorization: Bearer $api_key" \ | ||
| -H "Content-Type: application/json" \ | ||
| -d "{ | ||
| \"model\": \"$model\", | ||
| \"messages\": $messages | ||
| }") | ||
|
|
||
| # Check for errors | ||
| local error=$(echo "$response" | jq -r '.error // empty') | ||
| if [[ -n "$error" ]]; then | ||
| echo "ERROR: API Error: $error" >&2 | ||
| return 1 | ||
| fi | ||
|
|
||
| # Extract content | ||
| echo "$response" | jq -r '.choices[0].message.content // empty' | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
HTTP status never checked & API error format mis-handled
curl -s ignores HTTP error codes; the script only looks for .error in the body.
A 401/500 will surface as an empty response.
- local response=$(curl -s -X POST "$API_URL" \
+ local response; local http
+ http=$(curl -sS -w '%{http_code}' -o /tmp/researcher_resp "$API_URL" \
-H "Authorization: Bearer $api_key" \
-H "Content-Type: application/json" \
-d "{
\"model\": \"$model\",
\"messages\": $messages
- }")
+ }")
+ response=$(cat /tmp/researcher_resp); rm -f /tmp/researcher_resp
+
+ if [[ $http -ge 400 ]]; then
+ echo "ERROR: HTTP $http – $(echo "$response" | jq -r '.error.message? // .error // .message // empty')" >&2
+ return 1
+ fi🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 77-77: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 79-79: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 88-88: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
In llm-agent-tools/researcher.sh around lines 74 to 96, the api_call function
does not check the HTTP status code from the curl request and only inspects the
JSON body for errors, which can miss HTTP errors like 401 or 500. Modify the
function to capture the HTTP status code from curl using the -w option, check if
the status code indicates an error (not 2xx), and handle it by printing an
appropriate error message and returning a failure code. Also, ensure the JSON
error parsing remains for valid responses.
| # Parse --agent parameter | ||
| while [[ $# -gt 0 ]] && [[ "$1" =~ ^-- ]]; do | ||
| case "$1" in | ||
| --agent) | ||
| AGENT_NAME="$2" | ||
| shift 2 | ||
| ;; |
There was a problem hiding this comment.
--agent parsing can seg-fault when value is missing
shift 2 is executed even if the option appears without a value, leading to an
“index out of range” and all further arguments being mis-parsed.
--agent)
- AGENT_NAME="$2"
- shift 2
+ [[ $# -ge 2 ]] || { echo "ERROR: --agent requires a value" >&2; exit 1; }
+ AGENT_NAME="$2"
+ shift 2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Parse --agent parameter | |
| while [[ $# -gt 0 ]] && [[ "$1" =~ ^-- ]]; do | |
| case "$1" in | |
| --agent) | |
| AGENT_NAME="$2" | |
| shift 2 | |
| ;; | |
| # Parse --agent parameter | |
| while [[ $# -gt 0 ]] && [[ "$1" =~ ^-- ]]; do | |
| case "$1" in | |
| --agent) | |
| [[ $# -ge 2 ]] || { echo "ERROR: --agent requires a value" >&2; exit 1; } | |
| AGENT_NAME="$2" | |
| shift 2 | |
| ;; | |
| # … other options … | |
| esac | |
| done |
🤖 Prompt for AI Agents
In llm-agent-tools/scratchpad-multi.sh around lines 31 to 37, the script shifts
two arguments after parsing the --agent option without checking if a value
follows, which can cause a segmentation fault or index out of range error. Fix
this by adding a check to ensure that a value exists after --agent before
executing shift 2; if the value is missing, handle the error gracefully or exit
with a clear message.
| safe_append() { | ||
| local file="$1" | ||
| local content="$2" | ||
| with_lock "$LOCK_FILE" bash -c "echo '$content' >> '$file'" | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
safe_append fails when content contains single quotes or newlines
Embedding $content inside single quotes inside a bash -c string breaks if the
text itself contains ', and collapses newlines. Use a heredoc to keep data
verbatim.
-with_lock "$LOCK_FILE" bash -c "echo '$content' >> '$file'"
+with_lock "$LOCK_FILE" bash -c 'cat >> "$1"' _ "$file" <<'EOF'
+$content
+EOFCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In llm-agent-tools/scratchpad-multi.sh around lines 118 to 123, the safe_append
function fails when the content contains single quotes or newlines because it
embeds the content inside single quotes in a bash -c string. To fix this,
rewrite the function to use a heredoc inside the with_lock call to append the
content verbatim to the file, preserving all special characters and newlines
without breaking the shell command.
Summary
Browser Tools Features
visit_page,page_down,page_up,find_on_page_ctrl_f,find_nextZod Schema Enhancement
Files Added
src/tools/text-browser.ts- Core browser implementationsrc/tools/browser-tools.ts- Individual browser toolssrc/utils/schema-formatter.ts- Zod schema formatting utilitiesexamples/browser-tools-example.ts- Usage examplestest/browser-tools*.test.ts- Comprehensive test suiteCLAUDE.md- Development documentationTest Coverage
Breaking Changes
None - this is purely additive functionality.
Before/After Comparison
Before (parameter naming issues):
After (enhanced with Zod):
This implementation enables AI agents to browse and analyze web content autonomously while maintaining type safety and preventing parameter naming confusion.
Summary by CodeRabbit
New Features
Documentation
Tests
Utilities
Exports
Chores