Feat/enhance finresearch #809
Conversation
…at/enhance_finresearch
Summary of ChangesHello @alcholiclg, 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 significantly enhances 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements to the ms-agent framework, primarily by adding a new AgentTool that allows agents to call other agents as sub-tools, improving modularity and reusability. It also integrates a JinaReader for robust asynchronous fetching and processing of web content with retry logic and timeouts. The document loading (doc_loader.py) is made more resilient and concurrent by adjusting requests timeouts and utilizing ThreadPoolExecutor for URL validation. The fin_research application's UI is updated with a new introductory section, an 'Examples' link, and refined placeholder/status messages. Crucially, the aggregator.yaml agent's prompt is updated to enforce a multi-phase workflow, requiring continuous tool usage between phases, and introduces a new spec_loader tool to replace the principle_skill for retrieving both principle and writing style specifications. Review comments highlighted areas for improvement in AgentTool's message role handling, JinaReader's broad exception catching, and the aggregator agent's tool calling protocol to ensure clarity and robustness, along with a suggestion to return structured sections from spec_loader for better LLM processing.
| Message( | ||
| role=msg.get('role', 'user'), | ||
| content=msg.get('content', ''), | ||
| tool_calls=msg.get('tool_calls', []), | ||
| tool_call_id=msg.get('tool_call_id'), | ||
| name=msg.get('name'), | ||
| reasoning_content=msg.get('reasoning_content', ''), | ||
| ) for msg in raw_messages # TODO: Change role to user or not | ||
| ] |
There was a problem hiding this comment.
The TODO comment on line 273 (# TODO: Change role to user or not) suggests some uncertainty about how message roles should be handled when passing them to a sub-agent. While the current implementation defaults to the 'user' role if it's missing, this might not be appropriate for all sub-agents, which could expect a more structured conversation history with alternating user and assistant roles.
To improve robustness and clarity, it would be beneficial to resolve this TODO. If the sub-agent is designed to process a sequence of messages, preserving the original roles is crucial. If a role is missing from the input, logging a warning could help diagnose issues during integration.
| except Exception: | ||
| # Unknown error; do not loop excessively | ||
| if attempt <= config.retries: | ||
| sleep_s = min(config.backoff_max, | ||
| config.backoff_base * (2**(attempt - 1))) | ||
| sleep_s *= random.uniform(0.7, 1.4) | ||
| time.sleep(sleep_s) | ||
| continue | ||
| return '' |
There was a problem hiding this comment.
The except Exception: block catches all exceptions and retries. This is generally too broad and can mask underlying bugs in the code (like TypeError, ValueError, etc.) by treating them as transient network errors. It's better to catch specific, expected exceptions that are known to be retryable.
I recommend refining the exception handling to catch more specific, retryable exceptions, such as socket.timeout or other connection-related errors from urllib. Unexpected exceptions should be allowed to propagate or at least be logged as errors before returning, so they can be debugged and fixed.
except Exception as e:
# Unknown error; fail fast and log it for debugging.
# from ms_agent.utils import get_logger
# logger = get_logger()
# logger.warning("Unexpected error fetching URL %s: %s", url, e)
return ''| <Tool_Calling_Protocol> | ||
| - Use standard OpenAI function calling to invoke tools. \ | ||
| Do NOT output code in assistant's natural language output. | ||
| - Every turn MUST include at least one tool call, unless you're providing the FINAL summary. | ||
| - Use standard OpenAI function calling to invoke tools. Do NOT output code in assistant's natural language output. | ||
| - If you use [ACT=code], [ACT=collect], or [ACT=fix], you MUST include at least one tool call in that turn. | ||
| - If you use [ACT=report], you MUST output the comprehensive summary in markdown format and MUST NOT call any tools in that turn. | ||
| - After each tool call, carefully review the output. | ||
| - State explicitly what you learned and what comes next. | ||
| - Continue calling tools until you have sufficient evidence to conclude. | ||
| - When analysis is complete and you need to provide a comprehensive summary, \ | ||
| you can use [ACT=report] without tools and stop. | ||
| </Tool_Calling_Protocol> |
There was a problem hiding this comment.
The new tool calling protocol is more specific, which is great. However, by removing the old rule (Every turn MUST include at least one tool call, unless you're providing the FINAL summary.), a potential ambiguity is introduced. The new rules only cover turns with [ACT=code], [ACT=collect], [ACT=fix], or [ACT=report].
If the agent produces a response without one of these tags, the protocol is now ambiguous. To make the instructions more robust against unexpected agent behavior, consider adding a catch-all rule.
For example, you could add:
Unless you are using [ACT=report] to provide the final summary, every turn MUST include at least one tool call.
| return json.dumps( | ||
| { | ||
| 'success': True, | ||
| 'sections': join_with.join(sections) | ||
| }, | ||
| ensure_ascii=False, | ||
| indent=2) |
There was a problem hiding this comment.
In load_specs, when format is 'markdown', the sections list is joined into a single string. This differs from the previous implementation in principle_skill.py, which returned a list of strings.
While this might be intentional, returning a single block of text can make it harder for the LLM to distinguish between different specs. Providing a list of sections offers more structure, which can help the agent process each spec individually. If a single string is needed, the agent can easily join the list itself.
| return json.dumps( | |
| { | |
| 'success': True, | |
| 'sections': join_with.join(sections) | |
| }, | |
| ensure_ascii=False, | |
| indent=2) | |
| return json.dumps( | |
| { | |
| 'success': True, | |
| 'sections': sections | |
| }, | |
| ensure_ascii=False, | |
| indent=2) |
…at/enhance_finresearch
1 participant
Change Summary
Related issue number
Checklist
pre-commit installandpre-commit run --all-filesbefore git commit, and passed lint check.