feat(core): enhance TaskContextPackager for external agents#432
Conversation
…d utility methods (#410) Adds three new methods to the existing TaskContextPackager: - build_agent_context(): bridges TaskContext → AgentContext for adapter protocol - to_file_list(): extracts relevant file paths for file-based engines (Aider) - to_task_file(): writes prompt to file for file-reading engines Also adds retry support to build() with attempt/previous_errors params that produce a "Previous Attempt Errors" section in the prompt. Closes #410
WalkthroughAdds retry-aware agent-context packaging: TaskContextPackager now builds AgentContext (including retry/error details), exposes file-list and task-file exporters, and appends retry sections into rendered prompts. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Packager as TaskContextPackager
participant Loader as TaskContext (source)
participant FS as Filesystem
Caller->>Packager: build_agent_context(task_id, attempt, previous_errors, gate_names)
Packager->>Loader: load task context & PRD, files, blockers, preferences
Loader-->>Packager: TaskContext, FileInfo, loaded_files
Packager->>Packager: _build_retry_section(attempt, previous_errors)
Packager->>FS: to_task_file(packaged, path) (optional write)
FS-->>Packager: Path written
Packager-->>Caller: AgentContext (prd_content, files, gates, retry info)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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 unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/core/test_context_packager.py (1)
3-10:⚠️ Potential issue | 🟠 MajorMark these new tests as v2.
This file adds coverage for the new packager surface, but none of the tests are marked with
@pytest.mark.v2and there is no module-levelpytestmark. That makes the suite inconsistent with the repo's v2 test-selection rules.As per coding guidelines, `tests/**/*.py`: `New v2 Python tests must be marked with `@pytest.mark.v2` decorator or pytestmark = pytest.mark.v2`.💡 Proposed fix
import pytest from pathlib import Path from unittest.mock import MagicMock, patch from codeframe.core.context_packager import TaskContextPackager, PackagedContext from codeframe.core.context import TaskContext, FileInfo from codeframe.core.adapters.agent_adapter import AgentContext + +pytestmark = pytest.mark.v2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/test_context_packager.py` around lines 3 - 10, This test module (the one importing TaskContextPackager, PackagedContext, TaskContext, FileInfo and AgentContext) is missing the v2 marker required for new v2 tests; add either a module-level marker (pytestmark = pytest.mark.v2) near the top of the file or decorate each test function/class with `@pytest.mark.v2` so pytest selects these as v2 tests (keep the existing import pytest at the top).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@codeframe/core/context_packager.py`:
- Around line 111-123: The returned AgentContext from build_agent_context is
missing the dependency_context field so downstream adapters lose dependency
guidance; update the return in build_agent_context to populate
dependency_context from the incoming context (e.g., context.dependency_context
or a safe default like {}) so AgentContext.dependency_context is set alongside
task_id, prd_content, relevant_files, etc., ensuring adapters consuming
AgentContext receive the dependency information.
- Around line 126-135: The docstring promises files "sorted by relevance score"
but to_file_list currently just returns packaged.context.relevant_files in
whatever order it has; update to_file_list to sort
packaged.context.relevant_files by each item's relevance_score (e.g.,
reverse=True to place highest relevance first) before mapping to fi.path, or if
you prefer to relax the contract instead, update the docstring to remove the
sorting claim; reference the to_file_list method,
PackagedContext.packaged.context.relevant_files entries (their relevance_score),
and fi.path when making the change and ensure the sort is stable and handles
missing/None relevance_score safely.
- Around line 59-60: The code currently uses "gate_names or ['pytest', 'ruff']"
which treats an explicit empty list as falsy and thus ignores caller intent; in
both build() and build_agent_context() replace that expression with an explicit
None check (e.g., if gate_names is None: gates = ['pytest','ruff'] else: gates =
gate_names) so that an empty list is honored, then pass gates into
self._build_gate_section; update the same pattern where gate_names is used
(refer to the build, build_agent_context, and _build_gate_section calls).
---
Outside diff comments:
In `@tests/core/test_context_packager.py`:
- Around line 3-10: This test module (the one importing TaskContextPackager,
PackagedContext, TaskContext, FileInfo and AgentContext) is missing the v2
marker required for new v2 tests; add either a module-level marker (pytestmark =
pytest.mark.v2) near the top of the file or decorate each test function/class
with `@pytest.mark.v2` so pytest selects these as v2 tests (keep the existing
import pytest at the top).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6aeac56-75ea-420a-a51a-d758805cc85e
📒 Files selected for processing (2)
codeframe/core/context_packager.pytests/core/test_context_packager.py
| return AgentContext( | ||
| task_id=task_id, | ||
| task_title=context.task.title, | ||
| task_description=context.task.description or "", | ||
| prd_content=prd_content, | ||
| tech_stack=context.tech_stack, | ||
| project_preferences=project_preferences, | ||
| relevant_files=relevant_files, | ||
| file_contents=file_contents, | ||
| blocker_history=blocker_history, | ||
| verification_gates=gates, | ||
| attempt=attempt, | ||
| previous_errors=previous_errors or [], |
There was a problem hiding this comment.
build_agent_context() drops dependency context.
AgentContext exposes a dependency_context field, and the packager contract here is to bridge task context into that adapter shape, but the returned object never populates it. Any adapter that consumes AgentContext instead of the rendered prompt will silently lose dependency guidance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/core/context_packager.py` around lines 111 - 123, The returned
AgentContext from build_agent_context is missing the dependency_context field so
downstream adapters lose dependency guidance; update the return in
build_agent_context to populate dependency_context from the incoming context
(e.g., context.dependency_context or a safe default like {}) so
AgentContext.dependency_context is set alongside task_id, prd_content,
relevant_files, etc., ensuring adapters consuming AgentContext receive the
dependency information.
|
Code Review - PR 432: Enhance TaskContextPackager for External Agents Good implementation of 410. The anti-corruption layer pattern is correct, the test class structure is clean, and the retry section is a useful addition. Five issues and some minor items follow in subsequent comments. |
|
Issue 1 - dependency_context field not populated (gap): AgentContext defines dependency_context: str | None = None at codeframe/core/adapters/agent_adapter.py:53, but build_agent_context() never populates it. If a task has dependency context loaded (e.g., from the dependency graph / DAG analysis), it will be silently dropped when crossing the boundary to an external adapter. Either: (a) Populate it from context if a source exists in TaskContext, or (b) add a comment/TODO acknowledging the gap now (e.g., TODO(431): dependency_context not yet wired). |
|
Issue 2 - Asymmetric retry behavior between build() and build_agent_context(): In build() (line 56), the retry section is silently skipped if attempt > 0 but previous_errors is empty/None. In build_agent_context(), attempt and previous_errors are passed straight through to AgentContext with no such guard - so build_agent_context(attempt=1) produces AgentContext(attempt=1, previous_errors=[]). These two paths behave differently for the same inputs. The behavior of build() (skip retry section when errors are empty) seems right, but it should be consistent or at least documented. |
|
Issue 3 - Multiline errors break markdown in _build_retry_section: The code does: for error in errors: lines.append(f'- {error}'). Real pytest/ruff errors are frequently multiline. If an error contains newlines, this produces broken markdown (a list item that spans lines without continuation indent). Consider stripping or collapsing to a single line for the prompt context. |
|
Issue 4 - Double context load when using both paths: Both build() and build_agent_context() independently call self._loader.load(task_id). For adapters needing both a prompt string and structured AgentContext, the context loads twice. Fine for the current separate-path design, but worth a note if both are ever needed for the same task. |
|
Issue 5 - Missing test for custom gate_names in build_agent_context(): There is a test verifying verification_gates defaults to [pytest, ruff], but no test for the gate_names override parameter. Easy to add: def test_custom_gate_names(self, mock_workspace, rich_task_context): with patch('codeframe.core.context_packager.ContextLoader') as MockLoader: MockLoader.return_value.load.return_value = rich_task_context; packager = TaskContextPackager(mock_workspace); ctx = packager.build_agent_context('task-99', gate_names=['build']); assert ctx.verification_gates == ['build'] |
|
Minor items: (1) test_defaults_for_optional_fields builds a full bare_ctx inline - a dedicated bare_task_context fixture would be more reusable. (2) to_file_list() docstring claims 'sorted by relevance score' but the order depends on what ContextLoader returns - worth verifying or softening the claim. Summary: The core design is solid - the build_agent_context() to AgentContext bridge is exactly the right pattern for the adapter layer, and the 16 new tests cover the happy paths well. Main actionable items: (1) Address the dependency_context gap, (2) Fix multiline error rendering in _build_retry_section, (3) Document or align the retry-section asymmetry between build() and build_agent_context(). Items 4 and 5 are optional improvements. |
…ring - Fix gate_names=[] being treated as falsy (use `is not None` check) - Collapse multiline errors to single lines in retry section - Fix to_file_list docstring to not claim explicit sorting - Add tests for custom gate_names and multiline error collapsing
Review: feat(core): enhance TaskContextPackager for external agentsThis is a solid implementation of the context packaging layer for Phase 4 agent adapters. The anti-corruption layer design is sound, the gate_names Three items to address before merging: 1. Missing
pytestmark = pytest.mark.v22.
3. Minor: test setup duplication Ten of the eleven Summary
The gate_names handling is correct — both Item 1 is the only hard blocker. |
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 (1)
tests/core/test_context_packager.py (1)
1-9:⚠️ Potential issue | 🟠 MajorMissing
@pytest.mark.v2marker for v2 functionality tests.This file tests new Phase 4 TaskContextPackager functionality but lacks the required v2 marker. As per coding guidelines,
tests/**/*.py: "New v2 Python tests must be marked with@pytest.mark.v2decorator orpytestmark = pytest.mark.v2".🔧 Proposed fix: Add module-level pytestmark
"""Tests for TaskContextPackager.""" import pytest from pathlib import Path from unittest.mock import MagicMock, patch +pytestmark = pytest.mark.v2 + from codeframe.core.context_packager import TaskContextPackager, PackagedContext from codeframe.core.context import TaskContext, FileInfo from codeframe.core.adapters.agent_adapter import AgentContext🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/test_context_packager.py` around lines 1 - 9, Add the required v2 pytest marker at module scope so these tests for TaskContextPackager/PackagedContext (which use TaskContext, FileInfo and AgentContext) are marked as v2; add a module-level pytestmark = pytest.mark.v2 (or decorate the test functions with `@pytest.mark.v2`) at the top of the test file to satisfy the new v2 test requirement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/core/test_context_packager.py`:
- Around line 363-371: Add the module-level pytest v2 marker by defining
pytestmark = pytest.mark.v2 at the top of the test module so the file is marked
as v2; ensure pytest is imported if not already. Locate the
tests/core/test_context_packager.py module (where
test_returns_relevant_file_paths, TaskContextPackager, and to_file_list are
defined) and add the single-line marker near the top of the file (after imports)
to match other test modules.
---
Outside diff comments:
In `@tests/core/test_context_packager.py`:
- Around line 1-9: Add the required v2 pytest marker at module scope so these
tests for TaskContextPackager/PackagedContext (which use TaskContext, FileInfo
and AgentContext) are marked as v2; add a module-level pytestmark =
pytest.mark.v2 (or decorate the test functions with `@pytest.mark.v2`) at the top
of the test file to satisfy the new v2 test requirement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 401648cf-9b97-4727-9e48-f5ab92e4b0f5
📒 Files selected for processing (2)
codeframe/core/context_packager.pytests/core/test_context_packager.py
🚧 Files skipped from review as they are similar to previous changes (1)
- codeframe/core/context_packager.py
| def test_returns_relevant_file_paths(self, mock_workspace, rich_task_context): | ||
| with patch("codeframe.core.context_packager.ContextLoader") as MockLoader: | ||
| MockLoader.return_value.load.return_value = rich_task_context | ||
|
|
||
| packager = TaskContextPackager(mock_workspace) | ||
| packaged = packager.build("task-99") | ||
| files = packager.to_file_list(packaged) | ||
|
|
||
| assert files == ["src/auth.py", "tests/test_auth.py"] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify to_file_list implementation to check if ordering is preserved
ast-grep --pattern $'def to_file_list($_, $_):
$$$
'Repository: frankbria/codeframe
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Search for to_file_list method definition
rg "def to_file_list" -A 10 --type py
echo "---"
# Also search for rich_task_context fixture
rg "def rich_task_context" -A 5 --type py
echo "---"
# Check the relevant_files in context
rg "relevant_files" tests/core/test_context_packager.py -B 2 -A 2Repository: frankbria/codeframe
Length of output: 2970
🏁 Script executed:
#!/bin/bash
# Check if the test has `@pytest.mark.v2` decorator or module-level pytestmark
sed -n '360,375p' tests/core/test_context_packager.py
echo "---"
# Check for pytestmark at module level
head -30 tests/core/test_context_packager.py | grep -n pytestmarkRepository: frankbria/codeframe
Length of output: 781
🏁 Script executed:
#!/bin/bash
# Check the full file structure and look for pytest markers
head -50 tests/core/test_context_packager.py
echo "---"
# Search for all pytest.mark decorators in the file
rg "@pytest\.mark" tests/core/test_context_packager.pyRepository: frankbria/codeframe
Length of output: 1746
🏁 Script executed:
#!/bin/bash
# Verify if there are any existing test classes/methods with `@pytest.mark.v2` in other test files
rg "@pytest\.mark\.v2|pytestmark.*v2" tests/ --type py | head -20Repository: frankbria/codeframe
Length of output: 1439
Add v2 test marker per coding guidelines.
The test file is missing the required pytestmark = pytest.mark.v2 module-level marker. Add this to the top of tests/core/test_context_packager.py to comply with the v2 test marking requirements, consistent with other test files in the repository.
The ordering assertion in test_returns_relevant_file_paths is correct and not brittle—the list comprehension in to_file_list preserves insertion order of relevant_files, and the docstring correctly documents this as "relevance order."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/test_context_packager.py` around lines 363 - 371, Add the
module-level pytest v2 marker by defining pytestmark = pytest.mark.v2 at the top
of the test module so the file is marked as v2; ensure pytest is imported if not
already. Locate the tests/core/test_context_packager.py module (where
test_returns_relevant_file_paths, TaskContextPackager, and to_file_list are
defined) and add the single-line marker near the top of the file (after imports)
to match other test modules.
1 participant
Summary
Implements #410: [Phase 4] Task Context Packager for External Agents
Enhances the existing
TaskContextPackager(from PR #408) with the methods specified in issue #410:build_agent_context()— BridgesTaskContext(internal Python objects) →AgentContext(protocol, all strings/primitives). This is the anti-corruption layer between CodeFrame internals and the adapter protocol.to_file_list()— Extracts relevant file paths for engines that take explicit file lists (e.g., Aider)to_task_file()— Writes prompt to a file for engines that read task filesbuild()now acceptsattempt/previous_errorsparams, producing a "Previous Attempt Errors" sectionAcceptance Criteria
ContextPackagerassemblesAgentContextfrom all CodeFrame sourcesto_prompt_string()produces clean, engine-agnostic prompt (exists asbuild().prompt, enhanced with retry)to_file_list()extracts relevant file pathsto_task_file()writes context to temp file for file-based enginesTest Plan
Implementation Notes
context_packager.pyto avoid duplication (lesson from [Phase 4] Agent Adapter Protocol Definition #409)AgentContextimported from canonicalcodeframe.core.adapters.agent_adapterCloses #410
Summary by CodeRabbit
New Features
Tests