feat(core): absorb parallel-cc worktree + E2B into CodeFrame namespace (#535)#541
feat(core): absorb parallel-cc worktree + E2B into CodeFrame namespace (#535)#541
Conversation
#535) - Add codeframe/core/sandbox/worktree.py re-export shim for TaskWorktree, WorktreeRegistry, MergeResult, get_base_branch - Expose worktree symbols from codeframe.core.sandbox package __init__ - Add 5 import-coverage tests in test_sandbox_context.py - Update CLAUDE.md: add core/sandbox/ and adapters/e2b/ to repo structure, add E2B_API_KEY env var note - Update docs/V2_STRATEGIC_ROADMAP.md: mark parallel-cc absorption complete, remove dependency-arc language (parallel-cc was never added as a dep) Closes #535
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a sandbox worktree re-export module and extends the sandbox package API to expose worktree types; adds tests asserting the re-exports; updates docs to reflect absorption of parallel-cc, and documents a new E2B adapter and required Changes
Sequence Diagram(s)(Skipped — changes are re-exports, tests, and documentation; no new multi-component control flow introduced requiring sequence visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 Generate unit tests (beta)
Comment |
|
Review: feat(core): absorb parallel-cc worktree + E2B into CodeFrame namespace This is a clean, focused PR that closes the namespace gap left after #533 and #534. The approach is correct — a thin re-export shim rather than duplicating logic. What is solid
One formatting issue to fix In All other rows have four columns. The extra pipe renders as a ragged table in most Markdown viewers. The checkmark should be moved into the task name cell instead: Tests The 5 import-coverage tests are appropriate for a pure re-export shim — there is no logic to unit test, so verifying each symbol resolves from the new namespace is the right scope. Minor note: Summary: approve with one non-blocking fix suggested (roadmap table column). Everything else is correct. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/V2_STRATEGIC_ROADMAP.md`:
- Line 349: The Phase 4 table row contains one too many columns: the row "|
codeframe-xz0f | parallel-cc absorption into CodeFrame | 4.B | MEDIUM | ✅ |"
must match the table header's four columns; update the row in
V2_STRATEGIC_ROADMAP.md by removing the extra checkmark column (the trailing
"✅") or adjust the header to include a fifth column if intentional so that
"codeframe-xz0f", "parallel-cc absorption into CodeFrame", "4.B", and "MEDIUM"
align with the header columns.
In `@tests/core/test_sandbox_context.py`:
- Around line 158-180: The new test cases in class TestSandboxWorktreeReexports
(test_task_worktree_importable_from_sandbox,
test_merge_result_importable_from_sandbox,
test_worktree_registry_importable_from_sandbox,
test_get_base_branch_importable_from_sandbox,
test_sandbox_worktree_module_importable) need the v2 marker; add pytest.mark.v2
by either decorating the TestSandboxWorktreeReexports class with `@pytest.mark.v2`
or by adding module-level pytestmark = pytest.mark.v2 so all tests in that
module are marked v2 and comply with the tests/core marking guideline.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9f0fb89-2d3a-432a-bc2d-48dbde8ab3e4
📒 Files selected for processing (5)
CLAUDE.mdcodeframe/core/sandbox/__init__.pycodeframe/core/sandbox/worktree.pydocs/V2_STRATEGIC_ROADMAP.mdtests/core/test_sandbox_context.py
- Move ✅ into table cell (fix 5-column row in 4-column Phase 4 table) - Add pytestmark = pytest.mark.v2 to test_sandbox_context.py per tests/core/ convention
|
Follow-up review (commit 04f2fe7): Both issues flagged in my first pass and by CodeRabbit are resolved:
Nothing new to flag. This is ready to merge. |
There was a problem hiding this comment.
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_sandbox_context.py (1)
16-22:⚠️ Potential issue | 🟠 MajorMove
pytestmarkbelow imports to fix Ruff E402.
pytestmarkat Line 16 places a statement before module imports at Lines 18–22, which triggers the reported CI failure (E402).✅ Minimal fix
import pytest - -pytestmark = pytest.mark.v2 from codeframe.core.sandbox.context import ( ExecutionContext, IsolationLevel, create_execution_context, ) + +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_sandbox_context.py` around lines 16 - 22, pytestmark is defined before module imports causing Ruff E402; move the pytestmark assignment so it appears after the import block that brings in ExecutionContext, IsolationLevel, and create_execution_context. Locate the top of the test module where pytestmark = pytest.mark.v2 is declared and cut/paste that single statement immediately below the import statements for codeframe.core.sandbox.context so imports precede the pytestmark declaration.
🧹 Nitpick comments (1)
tests/core/test_sandbox_context.py (1)
179-182: Optionally assert allsandbox.worktreere-exports in this test.Nice coverage overall. For completeness, you can also assert
MergeResultandget_base_branchonwt_modto fully lock the submodule API surface.Optional test hardening
def test_sandbox_worktree_module_importable(self): from codeframe.core.sandbox import worktree as wt_mod assert hasattr(wt_mod, "TaskWorktree") assert hasattr(wt_mod, "WorktreeRegistry") + assert hasattr(wt_mod, "MergeResult") + assert hasattr(wt_mod, "get_base_branch")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/test_sandbox_context.py` around lines 179 - 182, Extend the test_sandbox_worktree_module_importable test to also assert that the worktree submodule re-exports MergeResult and get_base_branch: in test_sandbox_worktree_module_importable (which imports codeframe.core.sandbox.worktree as wt_mod) add assertions like hasattr(wt_mod, "MergeResult") and hasattr(wt_mod, "get_base_branch") so the module API surface is fully locked down.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/core/test_sandbox_context.py`:
- Around line 16-22: pytestmark is defined before module imports causing Ruff
E402; move the pytestmark assignment so it appears after the import block that
brings in ExecutionContext, IsolationLevel, and create_execution_context. Locate
the top of the test module where pytestmark = pytest.mark.v2 is declared and
cut/paste that single statement immediately below the import statements for
codeframe.core.sandbox.context so imports precede the pytestmark declaration.
---
Nitpick comments:
In `@tests/core/test_sandbox_context.py`:
- Around line 179-182: Extend the test_sandbox_worktree_module_importable test
to also assert that the worktree submodule re-exports MergeResult and
get_base_branch: in test_sandbox_worktree_module_importable (which imports
codeframe.core.sandbox.worktree as wt_mod) add assertions like hasattr(wt_mod,
"MergeResult") and hasattr(wt_mod, "get_base_branch") so the module API surface
is fully locked down.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e3c7c245-0646-4d46-8b3e-b13b9116f977
📒 Files selected for processing (2)
docs/V2_STRATEGIC_ROADMAP.mdtests/core/test_sandbox_context.py
✅ Files skipped from review due to trivial changes (1)
- docs/V2_STRATEGIC_ROADMAP.md
|
Follow-up review (commit b723692): The third commit moves All items from my previous reviews are now addressed. Ready to merge. |
1 participant
Summary
codeframe/core/sandbox/worktree.pyre-export shim, exposingTaskWorktree,WorktreeRegistry,MergeResult, andget_base_branchfrom thecodeframe.core.sandboxnamespacecodeframe/core/sandbox/__init__.pyto re-export worktree symbolsCLAUDE.md: addscore/sandbox/andadapters/e2b/to repository structure; addsE2B_API_KEYenv vardocs/V2_STRATEGIC_ROADMAP.md: marks parallel-cc absorption complete, removes stale dependency-arc languageContext
Most of the absorption work was already completed in #533 (WorktreeRegistry in
core/worktrees.py) and #534 (E2BAgentAdapter inadapters/e2b/). This PR closes the namespace gap and updates documentation to reflect the final state.parallel-ccwas never added as a dependency — the logic was ported directly.Test plan
test_sandbox_context.py,test_worktrees.py,test_e2b_adapter.pycodeframe.core.sandboxre-exportsparallel-cc/parallel_ccimport sites found in codebaseCloses #535
Summary by CodeRabbit
New Features
Documentation
Tests