Conversation
Signed-off-by: Codex <codex@openai.com>
WalkthroughThis pull request introduces a TaskSubAgent system that conditionally summarizes large tool outputs to prevent prompt pollution. It adds stub implementations for external dependencies (autoevals, braintrust), integrates TaskSubAgent into ToolCallingLLM with metadata propagation, introduces environment configuration variables, and provides supporting prompts and tests. Changes
Sequence DiagramsequenceDiagram
participant Client
participant ToolCallingLLM
participant TaskSubAgent
participant SubAgentLLM as SubAgent LLM
participant ToolExecutor
Client->>ToolCallingLLM: call() with tool executor
ToolCallingLLM->>ToolExecutor: execute tool call
ToolExecutor-->>ToolCallingLLM: return ToolCallResult (large output)
alt Content exceeds threshold
ToolCallingLLM->>TaskSubAgent: summarize_tool_message(tool_result)
TaskSubAgent->>TaskSubAgent: _should_summarize() check
TaskSubAgent->>TaskSubAgent: _truncate_input() if needed
TaskSubAgent->>TaskSubAgent: _create_subagent()
TaskSubAgent->>SubAgentLLM: invoke with Jinja2 prompt template
SubAgentLLM-->>TaskSubAgent: return summary + metadata
TaskSubAgent-->>ToolCallingLLM: summarized_message + metadata (tool_call_id, char counts)
ToolCallingLLM->>ToolCallingLLM: append metadata to task_subagent_summaries
else Content under threshold
ToolCallingLLM->>ToolCallingLLM: keep original message
end
ToolCallingLLM-->>Client: final response with propagated task_subagent_summaries metadata
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 0
🧹 Nitpick comments (4)
braintrust/oai.py (1)
1-2: Add type hints for consistency with project standards.Per coding guidelines, type hints are required. Consider adding type annotations to maintain consistency.
🔎 Suggested improvement
-def wrap_openai(client): - return client +from typing import TypeVar + +T = TypeVar("T") + +def wrap_openai(client: T) -> T: + """Passthrough stub for braintrust.oai.wrap_openai.""" + return clientbraintrust/__init__.py (1)
1-35: Add a module docstring to clarify this is a stub.Unlike
autoevals.py, this file lacks a docstring explaining its purpose as a test stub. Adding one improves maintainability and aligns with the pattern in other stub files.🔎 Suggested improvement
+""" +Minimal stub for the `braintrust` package used in tests. +Provides no-op implementations so imports succeed without the real dependency. +""" + + class Span: def __init__(self, *args, **kwargs):The unused argument warnings from Ruff are expected for stub implementations maintaining API compatibility.
holmes/common/env_vars.py (1)
116-124: Consider adding inline comments explaining the threshold values.Other environment variables in this file include explanatory comments (e.g., lines 89-91 for
TOOL_MAX_ALLOCATED_CONTEXT_WINDOW_PCT). Adding similar context for the new thresholds would improve maintainability.🔎 Suggested improvement
# Task sub-agent settings ENABLE_TASK_SUBAGENT = load_bool("ENABLE_TASK_SUBAGENT", True) +# Maximum characters for the sub-agent's summary output TASK_SUBAGENT_SUMMARY_MAX_CHARS = int( os.environ.get("TASK_SUBAGENT_SUMMARY_MAX_CHARS", 4000) ) +# Tool outputs exceeding this threshold trigger sub-agent summarization TASK_SUBAGENT_MAX_INPUT_CHARS = int( os.environ.get("TASK_SUBAGENT_MAX_INPUT_CHARS", 120000) )holmes/core/tool_calling_llm.py (1)
335-335: Redundant initialization oftask_subagent_metadata.This variable is re-declared inside the
whileloop at line 467, making this outer initialization dead code. The per-iteration reset is intentional (metadata is accumulated intometadatadict at lines 533-536 after each batch), but this line serves no purpose.🔎 Proposed fix
metadata: Dict[Any, Any] = {} - task_subagent_metadata: list[dict] = [] while i < max_steps:
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
autoevals.pybraintrust/__init__.pybraintrust/oai.pyholmes/common/env_vars.pyholmes/core/task_subagent.pyholmes/core/tool_calling_llm.pyholmes/plugins/prompts/task_subagent_summarize_tool.jinja2pytest_shared_session_scope.pytests/core/test_task_subagent.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
braintrust/oai.pyholmes/common/env_vars.pyautoevals.pybraintrust/__init__.pyholmes/core/tool_calling_llm.pytests/core/test_task_subagent.pypytest_shared_session_scope.pyholmes/core/task_subagent.py
holmes/plugins/prompts/**/*.jinja2
📄 CodeRabbit inference engine (CLAUDE.md)
holmes/plugins/prompts/**/*.jinja2: Prompts: holmes/plugins/prompts/{name}.jinja2 file structure
Use Jinja2 templates for investigation prompts
Files:
holmes/plugins/prompts/task_subagent_summarize_tool.jinja2
holmes/core/tool_calling_llm.py
📄 CodeRabbit inference engine (CLAUDE.md)
holmes/core/tool_calling_llm.py: Use LiteLLM for multi-provider support (OpenAI, Anthropic, Azure, etc.) in LLM integration
Implement structured tool calling with automatic retry and error handling
Files:
holmes/core/tool_calling_llm.py
tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Tests: Match source structure under tests/
Files:
tests/core/test_task_subagent.py
🧠 Learnings (4)
📚 Learning: 2025-12-21T13:17:48.366Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-21T13:17:48.366Z
Learning: Applies to holmes/plugins/prompts/**/*.jinja2 : Use Jinja2 templates for investigation prompts
Applied to files:
holmes/plugins/prompts/task_subagent_summarize_tool.jinja2
📚 Learning: 2025-12-21T13:17:48.366Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-21T13:17:48.366Z
Learning: Applies to holmes/plugins/prompts/**/*.jinja2 : Prompts: holmes/plugins/prompts/{name}.jinja2 file structure
Applied to files:
holmes/plugins/prompts/task_subagent_summarize_tool.jinja2
📚 Learning: 2025-12-21T13:17:48.366Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-21T13:17:48.366Z
Learning: Applies to holmes/core/tool_calling_llm.py : Use LiteLLM for multi-provider support (OpenAI, Anthropic, Azure, etc.) in LLM integration
Applied to files:
holmes/core/tool_calling_llm.pytests/core/test_task_subagent.py
📚 Learning: 2025-12-21T13:17:48.366Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-21T13:17:48.366Z
Learning: Applies to holmes/core/tool_calling_llm.py : Implement structured tool calling with automatic retry and error handling
Applied to files:
holmes/core/tool_calling_llm.pytests/core/test_task_subagent.py
🧬 Code graph analysis (3)
holmes/plugins/prompts/task_subagent_summarize_tool.jinja2 (1)
holmes/core/tools.py (1)
_load_llm_instructions(767-772)
tests/core/test_task_subagent.py (6)
holmes/core/llm.py (2)
LLM(91-134)TokenCountMetadata(57-64)holmes/core/models.py (2)
ToolCallResult(23-64)as_tool_call_message(30-40)holmes/core/task_subagent.py (3)
TaskSubAgent(24-149)TaskSubAgentConfig(17-21)summarize_tool_message(82-149)holmes/core/tool_calling_llm.py (2)
LLMResult(143-156)prompt_call(280-300)holmes/core/tools.py (2)
StructuredToolResult(79-105)StructuredToolResultStatus(52-76)holmes/core/tools_utils/tool_executor.py (1)
ToolExecutor(14-58)
holmes/core/task_subagent.py (3)
holmes/core/models.py (1)
ToolCallResult(23-64)holmes/core/tools_utils/tool_executor.py (1)
ToolExecutor(14-58)holmes/plugins/prompts/__init__.py (1)
load_and_render_prompt(27-54)
🪛 Ruff (0.14.8)
autoevals.py
18-18: Unused method argument: args
(ARG002)
18-18: Unused method argument: kwargs
(ARG002)
22-22: Unused function argument: args
(ARG001)
22-22: Unused function argument: kwargs
(ARG001)
braintrust/__init__.py
6-6: Unused method argument: args
(ARG002)
6-6: Unused method argument: kwargs
(ARG002)
15-15: Unused method argument: args
(ARG002)
15-15: Unused method argument: kwargs
(ARG002)
tests/core/test_task_subagent.py
27-27: Unused method argument: messages
(ARG002)
27-27: Unused method argument: tools
(ARG002)
39-39: Unused method argument: args
(ARG002)
39-39: Unused method argument: kwargs
(ARG002)
40-40: Avoid specifying long messages outside the exception class
(TRY003)
76-76: Unused method argument: kwargs
(ARG002)
⏰ 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). (5)
- GitHub Check: llm_evals
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
- GitHub Check: build
🔇 Additional comments (12)
autoevals.py (1)
1-23: LGTM - stub implementation is appropriate for test scaffolding.The unused arguments flagged by Ruff are intentional for API compatibility with the real
autoevalspackage. The stub provides sufficient structure for imports to succeed in tests.Consider adding a type hint for the
metadataparameter for completeness:def __init__(self, score: float = 0.0, metadata: dict | None = None):pytest_shared_session_scope.py (1)
1-23: LGTM!The shim is well-documented and provides appropriate no-op behavior for local test runs. The sentinel token pattern using
type()is a reasonable approach for creating lightweight namespace-like objects.holmes/plugins/prompts/task_subagent_summarize_tool.jinja2 (1)
1-23: LGTM!The prompt template is well-structured with clear role definition, constraints, and guidelines. The Jinja2 variable placeholders (
{{ parent_user_message }},{{ tool_name }}, etc.) follow the project's prompts pattern. Based on learnings, this correctly uses Jinja2 templates for investigation prompts.holmes/common/env_vars.py (1)
118-118: Verify that enabling the task subagent by default is intentional.
ENABLE_TASK_SUBAGENTdefaults toTrue, meaning this feature will be active for all users upon deployment. Confirm this is the desired rollout strategy versus an opt-in approach.tests/core/test_task_subagent.py (1)
1-131: Good test coverage for core TaskSubAgent scenarios.The tests effectively validate the conditional summarization behavior:
- Under-threshold: content unchanged, no subagent invocation
- Over-threshold: subagent runs, summary prefix added, metadata populated
Consider adding edge case tests in a follow-up:
- Non-string content in the message
- Empty string content
- Subagent exception handling (to verify fallback behavior)
holmes/core/tool_calling_llm.py (3)
168-186: Clean integration of TaskSubAgent into ToolCallingLLM.The constructor properly:
- Adds the
enable_task_subagentparameter with sensible default- Passes the flag through to TaskSubAgent initialization
- Shares the same
llm,tool_executor, andmax_stepswith the subagent
514-523: Correct integration of TaskSubAgent for tool message summarization.The flow properly:
- Obtains the original tool message
- Delegates to
summarize_tool_messagewith full context- Collects metadata when summarization occurs
- Uses the (possibly summarized) message going forward
1036-1045: Consistent TaskSubAgent integration in streaming path.The streaming path correctly mirrors the non-streaming implementation:
- Processes tool messages through
summarize_tool_message- Accumulates metadata across iterations
- Includes metadata in all exit paths (ANSWER_END, APPROVAL_REQUIRED, max_steps exception)
holmes/core/task_subagent.py (4)
16-21: Well-structured configuration dataclass.The config cleanly separates environment-driven defaults while allowing programmatic overrides. The
enabledfield's fallback toTrueensures the feature is on by default unless explicitly disabled.
30-47: Good constructor design with dependency injection.The
subagent_factoryparameter enables clean unit testing without invoking the realToolCallingLLM, as demonstrated in the test file. The config-basedmax_stepsoverride provides flexibility for subagent-specific limits.
68-80: Correct handling of circular dependency and nested subagent prevention.The lazy import avoids the circular dependency between
task_subagent.pyandtool_calling_llm.py. Settingenable_task_subagent=Falsewhen creating the nestedToolCallingLLMcorrectly prevents infinite subagent spawning.
82-149: Well-structured summarization with proper fallbacks.The method correctly:
- Guards against non-string content and small outputs
- Extracts context from parent messages for better summarization
- Handles subagent failures gracefully with fallback to truncated content
- Returns both the modified message and metadata for transparency
Minor note: Line 106
str(last_user_message)is redundant sincelast_user_messageis already a string from the.get("content", "")call, but it's harmless.
|
Dev Docker images are ready for this commit:
Use either tag to pull the image for testing. |
|
Dev Docker images are ready for this commit:
Use either tag to pull the image for testing. |
1 participant
Summary
Testing
Codex Task
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.