refactor(backend): type visual intent tool requirements#624
Conversation
📝 WalkthroughWalkthroughThis PR introduces a typed visual tool requirement contract to make visual intent routing deterministic and auditable. Visual tool capabilities are classified into lanes (chart, code-studio, mermaid, legacy), and a VisualToolRequirement encapsulates whether tools should be kept during pruning. Tool collection refactors direct and code-studio paths to compute and apply requirements instead of repeating visual tool logic. Focused unit tests validate chart, app, artifact, Mermaid, and text-turn tool binding. ChangesVisual tool requirement contract and refactored tool pruning
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
maritime-ai-service/tests/unit/test_visual_intent_resolver.py (1)
329-525: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd explicit fallback-path tests for
structured_visuals_enabled=False.Current additions only pin behavior when structured visuals are enabled. Add at least one requirement test and one filter test for disabled mode to lock fallback pruning semantics and prevent accidental over-pruning regressions.
As per coding guidelines,
maritime-ai-service/app/engine/**: verify “routing correctness, source propagation, memory/tool boundaries, streaming parity, structured output robustness, and fallback behavior”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@maritime-ai-service/tests/unit/test_visual_intent_resolver.py` around lines 329 - 525, Add tests that exercise the fallback when structured visuals are disabled: call resolve_visual_intent("Vẽ biểu đồ KPI theo tháng") and then build_visual_tool_requirement(decision, structured_visuals_enabled=False) and assert that required_visual_tool_names(decision) and requirement.required_tool_names include a legacy chart capability such as "tool_generate_chart" (and that requirement.required_capabilities reflect the chart lane instead of only "structured_visual"); and add a filter test that uses filter_tools_for_visual_intent(tools, decision, structured_visuals_enabled=False) with a tools list containing _Tool("tool_generate_visual"), _Tool("tool_generate_chart"), _Tool("tool_web_search") and assert _tool_names(filtered) still includes the legacy "tool_generate_chart" (i.e. legacy chart tools are not pruned). Use resolve_visual_intent, build_visual_tool_requirement, required_visual_tool_names, filter_tools_for_visual_intent, _Tool and _tool_names to locate code under test.maritime-ai-service/app/engine/multi_agent/tool_collection.py (1)
797-812:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve retrieval tools when forcing the direct visual lane.
This replaces the bound bundle with only
visual_requirement.required_tool_names. If the same turn also bound retrieval tools—e.g.tool_knowledge_searchon Lines 631-637—those get dropped even though_direct_required_tool_names()can still require them on Lines 1003-1009. Mixed retrieval + chart/article turns will lose their source-producing path.Risk: explicit visual requests over KB-backed data can render without the retrieval tool that supplies the facts and citations. Rollback: keep the preferred visual tool, but preserve any required non-visual tools instead of narrowing to a visual-only list.
Proposed fix
if ( visual_requirement.force_tool and visual_requirement.required_tool_names and visual_requirement.presentation_intent in {"article_figure", "chart_runtime"} and not ( _needs_web_search(query) or _needs_datetime(query) or _needs_news_search(query) or _needs_legal_search(query) or _needs_lms_query(query) or web_search_forced ) ): - preferred_tools = _tools_matching_visual_requirement(_direct_tools, visual_requirement) + required_names = set(visual_requirement.required_tool_names) + required_names.update( + name + for name in _direct_required_tool_names(query, user_role) + if name not in visual_tool_capability_names(include_legacy=True) + ) + preferred_tools = [ + tool for tool in _direct_tools if _tool_name(tool) in required_names + ] if preferred_tools: _direct_tools = preferred_toolsAs per coding guidelines, "verify routing correctness, source propagation, memory/tool boundaries, streaming parity, structured output robustness, and fallback behavior".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@maritime-ai-service/app/engine/multi_agent/tool_collection.py` around lines 797 - 812, The current logic in the visual-routing block replaces _direct_tools with only visual matches returned by _tools_matching_visual_requirement, which drops previously-bound retrieval tools (e.g., tool_knowledge_search) and breaks _direct_required_tool_names checks; instead, when preferred_tools is non-empty, set _direct_tools to the union of preferred_tools and any non-visual required tools originally bound (or returned by _direct_required_tool_names), preserving retrieval/KB tools while still prioritizing the visual tool(s). Locate the conditional using visual_requirement, _tools_matching_visual_requirement, and _direct_tools and change the assignment to merge required non-visual tools (exclude duplicates) rather than overwriting the bundle.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@maritime-ai-service/app/engine/multi_agent/tool_collection.py`:
- Around line 936-943: The current replacement of _tools with preferred_tools
when visual_requirement.force_tool and presentation_intent is
"code_studio_app"/"artifact" drops non-visual tools required later; change the
logic so you compute preferred_visual =
_tools_matching_visual_requirement(_tools, visual_requirement) but then set
_tools to the union of preferred_visual plus any tools whose names are in
_code_studio_required_tool_names(...) (e.g., tool_generate_html_file,
tool_generate_word_document, tool_generate_excel_file, tool_execute_python,
tool_browser_snapshot_url) and any other non-visual capabilities originally
present; only prune unrelated visual-only tools while preserving required
non-visual tools and return/streaming-capable tools.
---
Outside diff comments:
In `@maritime-ai-service/app/engine/multi_agent/tool_collection.py`:
- Around line 797-812: The current logic in the visual-routing block replaces
_direct_tools with only visual matches returned by
_tools_matching_visual_requirement, which drops previously-bound retrieval tools
(e.g., tool_knowledge_search) and breaks _direct_required_tool_names checks;
instead, when preferred_tools is non-empty, set _direct_tools to the union of
preferred_tools and any non-visual required tools originally bound (or returned
by _direct_required_tool_names), preserving retrieval/KB tools while still
prioritizing the visual tool(s). Locate the conditional using
visual_requirement, _tools_matching_visual_requirement, and _direct_tools and
change the assignment to merge required non-visual tools (exclude duplicates)
rather than overwriting the bundle.
In `@maritime-ai-service/tests/unit/test_visual_intent_resolver.py`:
- Around line 329-525: Add tests that exercise the fallback when structured
visuals are disabled: call resolve_visual_intent("Vẽ biểu đồ KPI theo tháng")
and then build_visual_tool_requirement(decision,
structured_visuals_enabled=False) and assert that
required_visual_tool_names(decision) and requirement.required_tool_names include
a legacy chart capability such as "tool_generate_chart" (and that
requirement.required_capabilities reflect the chart lane instead of only
"structured_visual"); and add a filter test that uses
filter_tools_for_visual_intent(tools, decision,
structured_visuals_enabled=False) with a tools list containing
_Tool("tool_generate_visual"), _Tool("tool_generate_chart"),
_Tool("tool_web_search") and assert _tool_names(filtered) still includes the
legacy "tool_generate_chart" (i.e. legacy chart tools are not pruned). Use
resolve_visual_intent, build_visual_tool_requirement,
required_visual_tool_names, filter_tools_for_visual_intent, _Tool and
_tool_names to locate code under test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a051de02-a11d-4a11-b7dd-a9ff789669cb
📒 Files selected for processing (4)
maritime-ai-service/app/engine/multi_agent/tool_collection.pymaritime-ai-service/app/engine/multi_agent/visual_intent_resolver.pymaritime-ai-service/tests/unit/test_tool_collection_visual_requirements.pymaritime-ai-service/tests/unit/test_visual_intent_resolver.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Unit Tests
- GitHub Check: Backend Unit Gate
- GitHub Check: CodeQL Analyze (javascript-typescript)
- GitHub Check: CodeQL Analyze (python)
- GitHub Check: test
- GitHub Check: Build Images
🧰 Additional context used
📓 Path-based instructions (4)
maritime-ai-service/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Run backend verification:
cd maritime-ai-service && set PYTHONIOENCODING=utf-8 && pytest tests/unit/ -p no:capture --tb=short -q && ruff check app/ --select=E9,F63,F7
Files:
maritime-ai-service/tests/unit/test_tool_collection_visual_requirements.pymaritime-ai-service/tests/unit/test_visual_intent_resolver.pymaritime-ai-service/app/engine/multi_agent/tool_collection.pymaritime-ai-service/app/engine/multi_agent/visual_intent_resolver.py
maritime-ai-service/app/{auth,core,engine,repositories}/**
📄 CodeRabbit inference engine (AGENTS.md)
maritime-ai-service/app/{auth,core,engine,repositories}/**: For backend, auth, memory, tenant isolation, migration, provider/runtime, MCP, or deployment changes, include explicit risk and rollback notes
Treat auth, JWT, OAuth, LMS token exchange, organization context, tenant isolation, semantic memory, long-term memory, MCP/tool execution, provider routing, migrations, and GitHub automation as high-risk surfaces requiring P0/P1 flagging when changes expose private data, cross tenant boundaries, bypass authorization, corrupt persistent memory, break streaming contracts, weaken deployment safety, or make rollback unclear
Files:
maritime-ai-service/app/engine/multi_agent/tool_collection.pymaritime-ai-service/app/engine/multi_agent/visual_intent_resolver.py
maritime-ai-service/app/engine/**
📄 CodeRabbit inference engine (AGENTS.md)
For
maritime-ai-service/app/engine/**, verify routing correctness, source propagation, memory/tool boundaries, streaming parity, structured output robustness, and fallback behavior
Files:
maritime-ai-service/app/engine/multi_agent/tool_collection.pymaritime-ai-service/app/engine/multi_agent/visual_intent_resolver.py
maritime-ai-service/app/engine/multi_agent/**
📄 CodeRabbit inference engine (maritime-ai-service/AGENTS.md)
Agent runtime changes should preserve routing, tool loop, provider behavior, and source propagation
Files:
maritime-ai-service/app/engine/multi_agent/tool_collection.pymaritime-ai-service/app/engine/multi_agent/visual_intent_resolver.py
⚙️ CodeRabbit configuration file
maritime-ai-service/app/engine/multi_agent/**: Focus on routing correctness, streaming parity, memory/tool boundaries, structured output robustness, and fallback behavior. Flag hidden prompt or state-contract changes.
Files:
maritime-ai-service/app/engine/multi_agent/tool_collection.pymaritime-ai-service/app/engine/multi_agent/visual_intent_resolver.py
🔇 Additional comments (2)
maritime-ai-service/tests/unit/test_tool_collection_visual_requirements.py (2)
44-60: ⚡ Quick winNo change needed:
_collect_direct_toolsloadsget_chart_tools/get_visual_toolsdynamically via_load_attr(import_module(...)), so monkeypatchingchart_tools.get_chart_toolsandvisual_tools.get_visual_toolsaffects this test path.
12-79: Backend verification fortest_tool_collection_visual_requirements.py(lines 12-79)
pytestunit tests couldn’t run in this environment (No module named pytest/pytest: command not found), so unit-test execution signal is unavailable.ruff check app/ --select=E9,F63,F7succeeds.- The test’s assertions are consistent with
_collect_direct_tools: settingstate["context"]["force_skills"]=["web-search"]makesweb_search_forced=Trueand triggers_strip_visual_tool_capabilitiesafter structured-visual/chart tools are added, so the visual tool names should be absent.
meiiie
force-pushed
the
codex/623-refactor-visual-tool-requirements
branch
from
d9efc1e to
f5c663f
Compare
- add typed visual tool capability and requirement contracts for visual intent decisions - route direct and Code Studio tool pruning through the shared requirement contract - cover chart, app, artifact, Mermaid, text, and web-search precedence cases
meiiie
force-pushed
the
codex/623-refactor-visual-tool-requirements
branch
from
f5c663f to
7ff8fcc
Compare
|
Governance note: all repo-owned checks are green for this PR ( |
1 participant
Summary
required_visual_tool_names()the shared source for direct and Code Studio visual tool requirements.Closes #623.
Scope
Non-Scope
Ownership / Agents
maritime-ai-service/app/engine/multi_agent/**, focused backend tests.Verification
cd maritime-ai-serviceuv run --python 3.12 --extra dev pytest tests/unit/test_tool_collection_host_ui.py tests/unit/test_tool_collection_analytical.py tests/unit/test_visual_intent_resolver.py tests/unit/test_tool_collection_visual_requirements.py -q --tb=short-> 60 passed.uv run --python 3.12 --extra dev pytest tests/unit/test_tool_collection_host_ui.py tests/unit/test_tool_collection_analytical.py tests/unit/test_visual_intent_resolver.py tests/unit/test_tool_collection_visual_requirements.py tests/unit/test_graph_routing.py::TestCollectDirectTools tests/unit/test_sprint154_tech_debt.py::TestCodeStudioWave002 tests/unit/test_tutor_request_runtime.py tests/unit/test_direct_tool_rounds_runtime.py::test_select_direct_tool_followup_rebinds_visual_only_tools tests/unit/test_direct_tool_rounds_runtime.py::test_select_direct_tool_followup_skips_non_bindable_base_llm tests/unit/test_direct_tool_rounds_runtime.py::test_select_direct_tool_followup_keeps_auto_llm_after_visual_emits -q --tb=short-> 82 passed.uv run --python 3.12 --extra dev ruff check app/engine/multi_agent/visual_intent_resolver.py app/engine/multi_agent/tool_collection.py tests/unit/test_visual_intent_resolver.py tests/unit/test_tool_collection_visual_requirements.py tests/unit/test_tool_collection_analytical.py tests/unit/test_tool_collection_host_ui.py-> passed.uv run --python 3.12 --extra dev ruff check app/ --select=E9,F63,F7-> passed.git diff --check-> passed.git status --short --branch-> clean after commit.Note:
uv runcreatedmaritime-ai-service/uv.lockduring verification; it was removed after verifying the resolved path was exactlyE:\Sach\Sua\AI_v1_product\maritime-ai-service\uv.lock.Risk
@web-searchdoes not get narrowed into visual generation when prompts also mention charts.Rollback
Reviewer Focus
required_visual_tool_names()remains the single source for visual required tools.