fix(tests): resolve test failures across framework and tools#7059
Merged
Conversation
📝 WalkthroughWalkthroughAdded Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Framework tests (52 -> 1 failure): - Add missing `model` attribute to mock LLM classes (MockStreamingLLM, CrashingLLM, ErrorThenSuccessLLM, etc.) to match new agent_loop.py requirement at line 624 - Update skill count assertions from 6 to 7 (new writing-hive-skills) - Fix phase compaction test to match new message format (no brackets) - Update model catalog test for current gemini model names - Fix queen memory test: set phase="building" to match prompt_building, adjust reflection trigger count to match cooldown behavior Tools tests (52 -> 0 failures): - Update csv_tool tests: remove agent_id parameter, use absolute paths, patch _ALLOWED_ROOTS instead of AGENT_SANDBOXES_DIR - Fix browser_evaluate test to allow toast wrapper around script Remaining: 1 pre-existing failure in test_worker_report where mock LLM gets stuck when scenarios are exhausted (separate bug).
Hundao
force-pushed
the
fix/test-failures
branch
from
April 16, 2026 11:40
bcce254 to
0f40a59
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
tools/tests/tools/test_csv_tool.py (2)
1-11: Missingfrom __future__ import annotationsimport.Per coding guidelines, this import should be added for modern type syntax.
"""Tests for csv_tool - Read and manipulate CSV files.""" +from __future__ import annotations + import importlib.util from pathlib import Path from unittest.mock import patch🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/tests/tools/test_csv_tool.py` around lines 1 - 11, Add the missing "from __future__ import annotations" at the top of the test module to comply with the project's typing guidelines; open the test file that contains import statements (the one importing importlib.util, Path, patch, pytest, FastMCP and register_tools) and place this future import as the first non-comment line so modern type annotations are enabled for the module.
15-29: Redundant patching of_ALLOWED_ROOTSthroughout tests.The
csv_toolsfixture already patches_ALLOWED_ROOTSwith theyieldinside thewithblock (lines 18-29), meaning the patch remains active during test execution. Each test method then redundantly patches the same value again.Since all tests use either
csv_toolsorcsv_tool_fn(which depends oncsv_tools), and both share the same function-scopedtmp_path, the per-test patches are unnecessary.Consider removing the redundant
with patch(...)blocks from individual test methods to reduce duplication. For example,test_read_basic_csvcould simplify to:def test_read_basic_csv(self, csv_tool_fn, basic_csv, tmp_path): """Read a basic CSV file successfully.""" result = csv_tool_fn(path=str(basic_csv)) assert result["success"] is True # ... rest of assertionsThis pattern repeats across all test methods in this file (~40 occurrences).
Also applies to: 79-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/tests/tools/test_csv_tool.py` around lines 15 - 29, The tests redundantly patch aden_tools.tools.file_system_toolkits.security._ALLOWED_ROOTS inside individual test functions even though the csv_tools fixture already patches it; remove the per-test with patch(...) blocks from tests that accept csv_tools or csv_tool_fn and rely on tmp_path, and simplify those tests (e.g., test_read_basic_csv) to call the tool function directly (csv_tool_fn(...)) without re-patching; ensure csv_tools (which calls register_tools(mcp) and sets _ALLOWED_ROOTS) remains function-scoped so tmp_path isolation is preserved and no other changes to register_tools, csv_tools, csv_tool_fn, or FastMCP behavior are needed.core/tests/test_llm_judge.py (1)
26-27: Align mock providermodelwith returnedLLMResponse.model.
MockLLMProvider.modelis now"mock"whilecomplete()returns"mock-model". Keeping these consistent reduces test ambiguity.Suggested tweak
- model: str = "mock" + model: str = "mock-model"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/tests/test_llm_judge.py` around lines 26 - 27, The test sets MockLLMProvider.model = "mock" but MockLLMProvider.complete() returns an LLMResponse with model "mock-model", causing inconsistency; update either MockLLMProvider.model or the returned LLMResponse.model in complete() so they match (e.g., change MockLLMProvider.model to "mock-model" or return LLMResponse.model = self.model) — locate MockLLMProvider and its complete() method and make the string values consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/tests/test_default_skills.py`:
- Line 22: The test function test_all_seven_skills_exist is missing a return
type annotation; update its signature to include an explicit return type (e.g.,
add "-> None") so the function follows the project's type-hinting guideline for
all function signatures.
- Around line 242-244: Only remove the single example placeholder so real
leftover placeholders aren't masked: assert that prompt.count("{{placeholder}}")
>= 1, then call prompt.replace("{{placeholder}}", "", 1) to remove just one
occurrence into cleaned, and keep the existing assert "{{" not in cleaned to
catch any other unremoved placeholders; reference the prompt variable, cleaned
variable, and the replace(...) call.
In `@core/tests/test_phase_compaction.py`:
- Line 215: The test asserts old_tool[0].content.startswith("Pruned tool
result") but compaction.py's detection still only checks for the prefix "[Pruned
tool result", causing a mismatch; update the compaction detection logic in
core/framework/agent_loop/internals/compaction.py (the function/logic that
checks for the pruned-tool sentinel, e.g., the is-pruned or PRUNED_TOOL_*
matching code) to accept the test prefix by matching either "Pruned tool result"
or both variants (e.g., .startswith(("Pruned tool result", "[Pruned tool
result"))), so production detection and the tests are aligned.
---
Nitpick comments:
In `@core/tests/test_llm_judge.py`:
- Around line 26-27: The test sets MockLLMProvider.model = "mock" but
MockLLMProvider.complete() returns an LLMResponse with model "mock-model",
causing inconsistency; update either MockLLMProvider.model or the returned
LLMResponse.model in complete() so they match (e.g., change
MockLLMProvider.model to "mock-model" or return LLMResponse.model = self.model)
— locate MockLLMProvider and its complete() method and make the string values
consistent.
In `@tools/tests/tools/test_csv_tool.py`:
- Around line 1-11: Add the missing "from __future__ import annotations" at the
top of the test module to comply with the project's typing guidelines; open the
test file that contains import statements (the one importing importlib.util,
Path, patch, pytest, FastMCP and register_tools) and place this future import as
the first non-comment line so modern type annotations are enabled for the
module.
- Around line 15-29: The tests redundantly patch
aden_tools.tools.file_system_toolkits.security._ALLOWED_ROOTS inside individual
test functions even though the csv_tools fixture already patches it; remove the
per-test with patch(...) blocks from tests that accept csv_tools or csv_tool_fn
and rely on tmp_path, and simplify those tests (e.g., test_read_basic_csv) to
call the tool function directly (csv_tool_fn(...)) without re-patching; ensure
csv_tools (which calls register_tools(mcp) and sets _ALLOWED_ROOTS) remains
function-scoped so tmp_path isolation is preserved and no other changes to
register_tools, csv_tools, csv_tool_fn, or FastMCP behavior are needed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0bd3047-f300-4ea2-9352-db77266c191c
📒 Files selected for processing (13)
core/tests/test_colony_runtime_overseer.pycore/tests/test_context_handoff.pycore/tests/test_default_skills.pycore/tests/test_event_loop_node.pycore/tests/test_litellm_provider.pycore/tests/test_llm_judge.pycore/tests/test_model_catalog.pycore/tests/test_phase_compaction.pycore/tests/test_queen_memory.pycore/tests/test_run_parallel_workers_tool.pycore/tests/test_skill_cli_commands.pytools/tests/test_browser_tools_comprehensive.pytools/tests/tools/test_csv_tool.py
|
|
||
| def test_all_six_skills_exist(self): | ||
| assert len(SKILL_REGISTRY) == 6 | ||
| def test_all_seven_skills_exist(self): |
There was a problem hiding this comment.
Add return type hint to the renamed test method.
Line 22 introduces a changed function signature without an explicit type hint.
Suggested fix
- def test_all_seven_skills_exist(self):
+ def test_all_seven_skills_exist(self) -> None:As per coding guidelines, "Use type hints on all function signatures".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_all_seven_skills_exist(self): | |
| def test_all_seven_skills_exist(self) -> None: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/tests/test_default_skills.py` at line 22, The test function
test_all_seven_skills_exist is missing a return type annotation; update its
signature to include an explicit return type (e.g., add "-> None") so the
function follows the project's type-hinting guideline for all function
signatures.
Comment on lines
+242
to
+244
| # Remove the known literal {{placeholder}} documentation example | ||
| cleaned = prompt.replace("{{placeholder}}", "") | ||
| assert "{{" not in cleaned |
There was a problem hiding this comment.
Current placeholder cleanup can mask real regressions.
replace("{{placeholder}}", "") removes all occurrences, so multiple unintended placeholders could slip through undetected.
Suggested fix
- # Remove the known literal {{placeholder}} documentation example
- cleaned = prompt.replace("{{placeholder}}", "")
+ # Remove exactly one known literal {{placeholder}} documentation example
+ literal_placeholder = "{{placeholder}}"
+ assert prompt.count(literal_placeholder) == 1
+ cleaned = prompt.replace(literal_placeholder, "", 1)
assert "{{" not in cleaned🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/tests/test_default_skills.py` around lines 242 - 244, Only remove the
single example placeholder so real leftover placeholders aren't masked: assert
that prompt.count("{{placeholder}}") >= 1, then call
prompt.replace("{{placeholder}}", "", 1) to remove just one occurrence into
cleaned, and keep the existing assert "{{" not in cleaned to catch any other
unremoved placeholders; reference the prompt variable, cleaned variable, and the
replace(...) call.
- Add text stop scenario to test_worker_report so worker terminates cleanly after tool_calls finish instead of replaying the last scenario forever - Remove duplicated hive home isolation fixture from test_colony_fork_live; reuse conftest autouse fixture and only add config copy on top
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/tests/test_colony_fork_live.py`:
- Around line 60-75: Add Python type hints to the fixture: annotate the
parameter _isolate_hive_home_autouse as Path and the return as Iterator[Path]
(or Generator[Path, None, None]) so the signature becomes e.g. def
isolated_hive_home(_isolate_hive_home_autouse: Path) -> Iterator[Path]: and add
the needed import from typing (Iterator) at the top of the file; keep the body
unchanged and ensure Path is the same pathlib.Path used elsewhere.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68a3c2ec-db8a-4659-8e04-d843cf488102
📒 Files selected for processing (2)
core/tests/test_colony_fork_live.pycore/tests/test_colony_runtime_overseer.py
🚧 Files skipped from review as they are similar to previous changes (1)
- core/tests/test_colony_runtime_overseer.py
Comment on lines
+60
to
75
| def isolated_hive_home(_isolate_hive_home_autouse): | ||
| """Extend the conftest autouse fixture with the user's LLM configuration. | ||
|
|
||
| Copies the user's real ~/.hive/configuration.json into the temp home | ||
| so the LLM provider config (model, api_base) is preserved. | ||
| The conftest ``_isolate_hive_home_autouse`` fixture already redirects | ||
| ``~/.hive`` to a temp directory and patches all module-level path | ||
| constants. This fixture just copies the real ``configuration.json`` | ||
| so the live integration test can pick up API keys and model config. | ||
| """ | ||
| fake_home_root = tmp_path | ||
| fake_hive = fake_home_root / ".hive" | ||
| fake_hive.mkdir() | ||
| fake_hive = _isolate_hive_home_autouse | ||
|
|
||
| # Copy LLM configuration so the framework picks up the user's model. | ||
| # Done BEFORE we monkey-patch Path.home so the source resolves correctly. | ||
| real_config = Path.home() / ".hive" / "configuration.json" | ||
| # Use os.path.expanduser since Path.home() is already patched by conftest. | ||
| real_config = Path(os.path.expanduser("~/.hive/configuration.json")) | ||
| if real_config.exists(): | ||
| shutil.copy(real_config, fake_hive / "configuration.json") | ||
|
|
||
| # Patch Path.home -> tmp_path so any call-site computation goes there. | ||
| monkeypatch.setattr(Path, "home", classmethod(lambda cls: fake_home_root)) | ||
|
|
||
| # Patch every module-level binding that captured a path constant. | ||
| for mod_name in _HIVE_PATH_CONSUMERS: | ||
| try: | ||
| mod = importlib.import_module(mod_name) | ||
| except ImportError: | ||
| continue | ||
| for attr_name, builder in _HIVE_PATH_NAMES: | ||
| if hasattr(mod, attr_name): | ||
| monkeypatch.setattr(mod, attr_name, builder(fake_hive)) | ||
|
|
||
| yield fake_hive |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import ast
from pathlib import Path
path = Path("core/tests/test_colony_fork_live.py")
tree = ast.parse(path.read_text(encoding="utf-8"))
for node in tree.body:
if isinstance(node, ast.FunctionDef) and node.name == "isolated_hive_home":
missing_args = [a.arg for a in node.args.args if a.annotation is None]
print("function:", node.name)
print("missing_arg_annotations:", missing_args)
print("missing_return_annotation:", node.returns is None)
PYRepository: aden-hive/hive
Length of output: 174
🏁 Script executed:
# Find and examine the _isolate_hive_home_autouse fixture definition
fd -t f 'conftest.py' -x grep -l '_isolate_hive_home_autouse' {}Repository: aden-hive/hive
Length of output: 82
🏁 Script executed:
# Examine the conftest.py file to find the fixture definition
cat -n ./core/tests/conftest.py | grep -A 20 '_isolate_hive_home_autouse'Repository: aden-hive/hive
Length of output: 1177
🏁 Script executed:
# Get more context to see what the fixture returns/yields
cat -n ./core/tests/conftest.py | sed -n '63,95p'Repository: aden-hive/hive
Length of output: 1204
🏁 Script executed:
# Check the actual function and its current imports at the top of the file
cat -n ./core/tests/test_colony_fork_live.py | head -80Repository: aden-hive/hive
Length of output: 3128
Add type hints to the fixture signature.
Line 60 defines isolated_hive_home without parameter and return annotations, violating the repository's Python typing requirement.
♻️ Proposed fix
import asyncio
+from collections.abc import Iterator
import json
import os
import shutil
import time
from pathlib import Path
@@
`@pytest.fixture`
-def isolated_hive_home(_isolate_hive_home_autouse):
+def isolated_hive_home(_isolate_hive_home_autouse: Path) -> Iterator[Path]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/tests/test_colony_fork_live.py` around lines 60 - 75, Add Python type
hints to the fixture: annotate the parameter _isolate_hive_home_autouse as Path
and the return as Iterator[Path] (or Generator[Path, None, None]) so the
signature becomes e.g. def isolated_hive_home(_isolate_hive_home_autouse: Path)
-> Iterator[Path]: and add the needed import from typing (Iterator) at the top
of the file; keep the body unchanged and ensure Path is the same pathlib.Path
used elsewhere.
fix(core): accept both pruned tool result sentinel formats MockStreamingLLM and _ByTaskMockLLM replay the last scenario forever when call_index exceeds the scenario list, causing worker timeouts in CI. Fix by emitting a text stop when scenarios are exhausted (scenarios mode) or already consumed (by_task mode). Also fix pruned tool result sentinel mismatch: conversation.py produces "Pruned tool result ..." but compaction.py and conversation.py only checked for "[Pruned tool result". Now both formats are accepted. Also remove duplicated hive home isolation fixture from test_colony_fork_live; reuse conftest autouse fixture instead.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/framework/agent_loop/internals/compaction.py`:
- Line 83: In microcompact(), the startswith check for tool-result placeholders
misses the unbracketed "Old tool result" variant so those placeholders get
re-compacted; update the conditional in microcompact() (the tuple passed to
msg.content.startswith) to include "Old tool result" in addition to "[Old tool
result", "[Pruned tool result", and "Pruned tool result" so both bracketed and
non-bracketed placeholder strings are recognized and skipped.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: be7b291e-2585-40af-acc6-81731f746f09
📒 Files selected for processing (4)
core/framework/agent_loop/conversation.pycore/framework/agent_loop/internals/compaction.pycore/tests/test_colony_runtime_overseer.pycore/tests/test_run_parallel_workers_tool.py
🚧 Files skipped from review as they are similar to previous changes (2)
- core/tests/test_run_parallel_workers_tool.py
- core/tests/test_colony_runtime_overseer.py
| if msg.role != "tool" or msg.is_error or msg.is_skill_content: | ||
| continue | ||
| if msg.content.startswith(("[Pruned tool result", "[Old tool result")): | ||
| if msg.content.startswith(("Pruned tool result", "[Pruned tool result", "[Old tool result")): |
There was a problem hiding this comment.
Handle bracketless "Old tool result" placeholders too.
On Line 83, microcompact() skips "[Old tool result" but not "Old tool result" (the format this file currently writes). That allows already-cleared spillover placeholders to be compacted again.
Suggested fix
- if msg.content.startswith(("Pruned tool result", "[Pruned tool result", "[Old tool result")):
+ if msg.content.startswith(
+ ("Pruned tool result", "[Pruned tool result", "Old tool result", "[Old tool result")
+ ):
continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/framework/agent_loop/internals/compaction.py` at line 83, In
microcompact(), the startswith check for tool-result placeholders misses the
unbracketed "Old tool result" variant so those placeholders get re-compacted;
update the conditional in microcompact() (the tuple passed to
msg.content.startswith) to include "Old tool result" in addition to "[Old tool
result", "[Pruned tool result", and "Pruned tool result" so both bracketed and
non-bracketed placeholder strings are recognized and skipped.
15 tasks
11 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fix 103 test failures (52 framework + 52 tools + flaky colony fork) to restore CI test checks on main. Depends on #7058 (lint fix).
Type of Change
Related Issues
Partial fix for #7057
Changes Made
Framework tests (52 -> 1 failure):
modelattribute to 11 mock LLM classes to match newagent_loop.py:624requirementwriting-hive-skillsskill)phase="building"to matchprompt_building, adjust reflection trigger countTools tests (52 -> 0 failures):
agent_idparameter, use absolute paths, patch_ALLOWED_ROOTSinstead ofAGENT_SANDBOXES_DIRRemaining: 1 pre-existing failure in
test_worker_report_emits_subagent_report_eventwhere mock LLM gets stuck when scenarios are exhausted (separate bug, not a test assertion issue).Testing
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests