test: add Object.create context regression test and tool confirmation integration test#22356
Conversation
… integration test
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the robustness and test coverage of the agent system by addressing how Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
The pull request introduces valuable test coverage for the Object.create() context pattern in policy.test.ts and a tool confirmation integration test in browser-agent.test.ts. The refactor in subagent-tool-wrapper.ts to pass the full AgentLoopContext to BrowserAgentInvocation is a positive change, improving consistency and access to context properties. All changes appear to be well-implemented and align with existing code patterns and testing methodologies. No critical or high-severity issues were identified.
Corrected Static Analysis SummaryApologies for the mangled text in the previous review. Here is the full summary: The implementation aligns perfectly with the stated goals, addressing the prototype-inherited property 'footgun' and improving integration test coverage. Key Findings:
Observation: |
Summary
Adds test coverage for the
Object.create()context pattern used by the agent scheduler, and a tool confirmation integration test for the browser agent test suite.Details
The agent scheduler creates
AgentLoopContextviaObject.create(config), which means properties likeconfigandmessageBuslive on the prototype chain — not as own properties. This is a known footgun: spreading such objects ({ ...context }) silently drops all inherited properties.This PR adds:
Unit test (
policy.test.ts): VerifiesupdatePolicyworks correctly when the context is created viaObject.create(), ensuring prototype-inherited properties are accessible.Integration test (
browser-agent.test.ts): Exercises the tool confirmation flow end-to-end usingrunInteractivewithapprovalMode: 'default'. The test triggers awrite_filecall, waits for the confirmation prompt, approves it, and verifies the response. Note:browser_agentitself cannot be tested for confirmation in integration tests because it is experimental and not registered in the test tool registry.BrowserAgentInvocation fix (
subagent-tool-wrapper.ts): Passes the fullAgentLoopContextinstead of justConfigtoBrowserAgentInvocation, aligning with the constructor's expected parameter type.Related Issues
Related to #21766
How to Validate
Run the unit test:
Verify the new
should work when context is created via Object.create (prototype chain)test passes.Run the integration tests:
Verify all 6 tests pass, including
should handle tool confirmation for write_file without crashing.Pre-Merge Checklist