fix: make the maxSubtasks parameter of PlanNotebook effective #645
Conversation
Summary of ChangesHello @tcsp, 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 refactors the hint generation mechanism within the Highlights
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
This pull request effectively implements the maxSubtasks parameter by updating the hint generation logic. The changes are well-structured, using the 'Introduce Parameter Object' refactoring pattern by passing the PlanNotebook instance instead of a boolean, which improves extensibility. The new logic in DefaultPlanToHint correctly adds the subtask limit rule to the hint when no plan exists. The related tests have been updated accordingly, and a new test has been added to cover the new functionality. I have one suggestion to make the new test more specific.
| assertNotNull(hint); | ||
| assertTrue(hint.startsWith("<system-hint>")); | ||
| assertTrue(hint.endsWith("</system-hint>")); | ||
| assertTrue(hint.contains("Subtask Limit")); |
There was a problem hiding this comment.
This assertion is a bit generic. To make the test more robust and verify that the maxSubtasks value is correctly interpolated into the hint string, it's better to check for the specific text including the value. This ensures that the {max_subtasks} placeholder is correctly replaced.
| assertTrue(hint.contains("Subtask Limit")); | |
| assertTrue(hint.contains("no more than 5 subtasks")); |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@LearningGp PTAL |
AgentScope-Java Version
[The version of AgentScope-Java you are working on, e.g. 1.0.7, check your pom.xml dependency version or run
mvn dependency:tree | grep agentscope-parent:pom(only mac/linux)]Description
make the maxSubtasks parameter of PlanNotebook effective.
Checklist
Please check the following items before code is ready to be reviewed.
mvn spotless:applymvn test)