feat: CodeFRAME v2 - Headless CLI-first architecture - #299
Conversation
Phase 1 - Workspace & Events: - New Typer CLI at codeframe/cli/app.py with domain-grouped commands - Workspace management with SQLite state storage in .codeframe/ - Append-only event log for all workspace activity - Updated pyproject.toml entry point Phase 2 - PRD & Task Management: - PRD storage with title extraction and metadata - Task state machine (BACKLOG→READY→IN_PROGRESS→BLOCKED→DONE→MERGED) - LLM-powered task generation from PRD (with simple fallback) - Status transitions with validation Test coverage: - 28 state machine unit tests - 17 workspace unit tests - 11 integration tests covering full Phase 1-2 flow
Shows workspace summary including: - PRD info (title and date) - Task counts by status with color-coding - Recent activity from event log - Configurable event count with --events/-e flag Emits STATUS_VIEWED event for activity tracking.
New runtime module (codeframe/core/runtime.py): - Run lifecycle management (start, stop, complete, fail, block, resume) - RunStatus enum (RUNNING, COMPLETED, FAILED, BLOCKED) - Stub agent execution loop that emits events Work CLI commands: - work start: Creates run, transitions task to IN_PROGRESS - work stop: Gracefully stops run, returns task to READY - work resume: Resumes a blocked run - work status: Shows active runs The --execute flag on work start runs the stub agent, emitting AGENT_STEP_STARTED and AGENT_STEP_COMPLETED events for testing.
New blockers module (codeframe/core/blockers.py): - BlockerStatus enum (OPEN, ANSWERED, RESOLVED) - Blocker CRUD operations - Partial ID matching for convenience Blocker CLI commands: - blocker list: Show open blockers (--all for all) - blocker show: View blocker details with question/answer - blocker create: Manually create blockers for testing - blocker answer: Provide answer to unblock work - blocker resolve: Mark blocker as resolved Emits BLOCKER_CREATED, BLOCKER_ANSWERED, BLOCKER_RESOLVED events.
New gates module (codeframe/core/gates.py): - Auto-detect available gates (pytest, ruff, mypy, npm-test, npm-lint) - Run gates with configurable verbosity - Capture output, exit codes, and timing - GateStatus enum (PASSED, FAILED, SKIPPED, ERROR) Review CLI command: - codeframe review: Run all detected gates - --gate/-g: Run specific gates only - --verbose/-v: Show full gate output Emits GATES_STARTED and GATES_COMPLETED events. Also: Added .codeframe/ to .gitignore
New artifacts module (codeframe/core/artifacts.py): - export_patch: Export git diff as a .patch file - create_commit: Create git commits with proper validation - get_status: Get git status summary - list_patches: List previously exported patches Patch CLI commands: - patch export: Export changes to .codeframe/patches/ - patch list: List exported patches - patch status: Show git status summary Commit CLI commands: - commit create: Create commits with -m message - commit create --all: Stage all changes before committing Emits PATCH_EXPORTED and COMMIT_CREATED events.
Adds checkpoint module for state snapshots and updates summary command to display workspace overview. Completes Golden Path CLI implementation.
Tracks the work needed to replace execute_stub() with a fully functional agent that can read context, plan, and execute code changes.
Adds codeframe/adapters/llm/ with: - base.py: Protocol, ModelSelector, LLMResponse, Tool/ToolCall types - anthropic.py: Claude provider with tool use and streaming support - mock.py: Test provider with call tracking and queued responses Task-based model selection heuristic: - Planning/reasoning → Sonnet - Execution → Sonnet - Generation → Haiku
Adds codeframe/core/context.py with: - TaskContext: dataclass holding task, PRD, blockers, and file contents - ContextLoader: loads and scores relevant files within token budget - Keyword extraction and relevance scoring for file selection - Token budgeting to maximize useful context Also adds list_for_task() helper to blockers module.
Adds codeframe/core/planner.py with: - Planner: transforms TaskContext into ImplementationPlan via LLM - ImplementationPlan: structured plan with steps, files, complexity - PlanStep: individual step with type, target, dependencies - StepType enum: file_create, file_edit, shell_command, verification Uses Purpose.PLANNING to select stronger model for reasoning tasks.
Adds codeframe/core/executor.py with: - Executor: executes plan steps via LLM-driven code generation - File operations: create, edit, delete with rollback tracking - Shell commands: sandboxed execution with dangerous pattern blocking - Dry-run mode for previewing changes without applying them - Full rollback capability for all file changes Uses Purpose.EXECUTION for balanced model selection during code generation.
Adds codeframe/core/agent.py with: - Agent: main orchestrator coordinating context, planning, execution - AgentState: serializable state for pause/resume - Blocker detection: creates blockers for failures needing human input - Gate integration: runs verification after file changes - Event emission: callback-based event system for monitoring Patterns detected for blocker creation: - Consecutive failures exceeding threshold - 'not found', 'missing', 'credentials' errors - Verification failures after max attempts
Adds execute_agent() to runtime.py: - Integrates full agent orchestration (context, plan, execute, verify) - Requires ANTHROPIC_API_KEY for real execution - Emits workspace events for monitoring Updates CLI work start command: - --execute: runs the real AI agent - --dry-run: preview changes without applying - --stub: legacy stub execution for testing The Golden Path is now fully functional from PRD to committed code.
- GateResult has `passed` (bool), not `status` - GateCheck has `name`, not `gate` Fixes AttributeError during agent execution verification.
Task status is now only updated by runtime.complete_run(), avoiding DONE -> DONE transition error.
- Python files: check existence and syntax - Commands: execute as shell - Other paths: check existence Fixes issue where 'task_tracker.py' was run as a command instead of verified.
- Update status badge to reflect v2 completion - Add "What's New" section for v2 agent implementation - Document CLI-first workflow as recommended approach - Update architecture diagram to show CLI/Agent orchestrator - Add complete CLI command reference - Move previous updates to collapsible sections - Update roadmap with completed items - Add links to v2 documentation (Golden Path, Agent Tasks)
- Update status to v2 Agent Implementation Complete - Add agent system architecture section with component table - Add execution flow diagram for agent orchestration - Document critical state separation pattern (Agent→AgentState, Runtime→TaskStatus) - Add recent updates section with bug fixes
…al errors Previously, the agent would create blockers for any error matching patterns like "not found" or "missing". This caused technical errors (syntax errors, file not found, import errors) to block execution when the agent should solve them automatically. Changes: - Add HUMAN_INPUT_PATTERNS for genuine human-needed situations (credentials, unclear requirements, design decisions) - Add TECHNICAL_ERROR_PATTERNS for errors agent can self-correct (file not found, syntax errors, import errors) - Add _classify_error() to categorize errors - Add _attempt_self_correction() to use LLM to fix technical errors - Update _execute_plan() to try self-correction before creating blockers - Update tests to reflect new behavior The agent now: 1. Classifies errors as "technical" or "human" 2. For technical errors: tries self-correction (up to 2 attempts) 3. Only creates blockers for human-input-needed situations or after exhausting self-correction attempts
When a blocker is answered, the associated task is now automatically reset to READY status. This eliminates the need for separate "work stop" and "work resume" commands. Flow is now: 1. Task runs → hits blocker → status becomes BLOCKED 2. User answers blocker: `cf blocker answer <id> "answer"` 3. Task automatically resets to READY 4. User can restart: `cf work start <id> --execute` The blocker answer includes the user's input, so the agent will have access to it when the task is restarted.
The previous code used Python's while...else construct, but when _attempt_self_correction returned None, we'd break out of the loop and skip the else block, which meant current_step was never incremented and the same step would be retried forever. Fixed by using a flag to track self-correction success and handling the failure case unconditionally after the loop ends.
…e edit Previously, when a file was written successfully but verification (ruff) detected a syntax error, the agent would: 1. Try ruff --fix (which can't fix syntax errors) 2. Just increment consecutive_failures and move on This left broken code in the file and continued to the next step. Now the agent: 1. Detects verification failure after successful file write 2. Triggers self-correction to fix the syntax/code error 3. Re-runs verification after each correction attempt 4. Creates a blocker if self-correction can't fix it This ensures syntax errors caught by linting get the same self-correction treatment as other technical errors.
…orrection When a VERIFICATION step fails (e.g., ast.parse catches a syntax error), we were trying to "self-correct" the verification step itself, which doesn't make sense. Now we convert it to a FILE_EDIT step targeting the same file, so self-correction actually fixes the broken code. This fixes the case where: 1. File is written with syntax error 2. Ruff doesn't catch it (ruff misses some errors that ast catches) 3. Verification step catches the syntax error 4. Self-correction can now actually fix the file
- Add BATCH_EXECUTION_PLAN.md with phased approach: - Phase 1: Serial batch execution via conductor - Phase 2: Parallel execution with dependency analysis - Phase 3: Observability and websocket streaming - Update CLI_WIREFRAME.md: - Add conductor.py and dependency_analyzer.py to module layout - Add cf work batch commands (batch, status, cancel) - Update implementation order with batch phases Design decisions: - Subprocess-based execution (isolation, crash-safe) - No server required (CLI-first) - Serial by default, parallel opt-in
Critical v1→v2 Migration Path MissingThis PR implements an excellent v2 architecture, but there's a blocking issue for existing users: no migration path from v1 to v2 data schemas. The ProblemUsers with existing v1 workspaces (containing tables like , , , ) will find their data completely inaccessible when running any v2 command. The function only adds missing columns to existing v2 tables - it does not migrate v1 schemas to v2 structure. Evidencecodeframe/core/workspace.py:324-331
The function handles v2→v2 upgrades (e.g., adding , columns) but has no v1→v2 migration logic. Recommended Action
SummaryThe v2 architecture is well-implemented and follows the Golden Path. However, without a documented migration strategy, existing users will experience data loss. This is the only issue that should block merge; all other concerns identified by CodeRabbit/Macroscope are valid but can be addressed post-merge. Please clarify the intended handling for existing v1 workspaces in the PR description or add a migration path before merging. |
|
Posted critical migration blocker comment to PR #299. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@codeframe/core/artifacts.py`:
- Around line 68-128: The fallback from "git diff --cached" to "git diff" in
export_patch can produce patch_content that doesn't match the original
staged_only flag used later to compute stats; update export_patch to track which
diff was actually used (e.g., introduce a boolean like actual_staged_used
initialized from staged_only and set to False when you fall back to running
["git", "diff"]) and then use that actual_staged_used value (instead of the
original staged_only) when computing stats and any metadata returned in
PatchInfo so the reported stats match the patch_content generated by diff_cmd
and patch_content.
In `@codeframe/core/dependency_graph.py`:
- Around line 150-166: Remove the dead initialization and no-op loop that
precomputes in_degree then iterates with a for loop containing only pass;
specifically delete the initial in_degree: dict[str, int] = {node: 0 for node in
graph} and the subsequent for node in graph: for dep in graph[node]: ... pass
block, and rely solely on the final in_degree = {node: len(graph.get(node, []))
for node in graph} used by the Kahn topological sort (references: graph and
in_degree).
In `@codeframe/core/events.py`:
- Around line 217-270: The list_recent function opens a DB connection via
get_db_connection and creates a cursor but doesn't guarantee conn (and cursor)
are closed on exceptions; wrap the DB operations in a try/finally (or use a
context manager) inside list_recent so that cursor.close() and conn.close()
always run even if cursor.execute or fetchall raises, keeping the existing
SELECT logic and JSON/datetime parsing intact; reference the function name
list_recent and the get_db_connection call to locate where to add the
try/finally and ensure both cursor and conn are closed in the finally block.
- Around line 110-165: The emit function opens a DB connection via
get_db_connection(workspace) but may leak it if an exception occurs before
conn.close(); wrap the DB operations (cursor creation, execute, retrieving
lastrowid, and conn.commit()) in a try/finally so that conn.close() is always
called in the finally block, preserving the commit on success and re-raising the
exception after cleanup; update emit to use this try/finally around the
cursor/execute/commit sequence (referencing emit, get_db_connection, conn,
cursor, conn.close) to ensure the connection is closed even on errors.
In `@codeframe/core/gates.py`:
- Around line 54-85: The summary property on GateResult currently omits
ERROR-status checks so when all checks error it returns "no checks run"; update
GateResult.summary to count GateStatus.ERROR (e.g., error_count = sum(1 for c in
self.checks if c.status == GateStatus.ERROR)) and add an entry for errors in the
parts list (e.g., parts.append(f"{error_count} errors") when error_count > 0) so
the human-readable summary includes error counts alongside
passed/failed/skipped.
- Around line 88-167: The run function currently treats unknown gate names as
SKIPPED and uses `gates or ["auto"]` when emitting the GATES_STARTED event which
misreports an explicitly empty list; update run to (1) change the GATES_STARTED
payload to use the actual provided gates list when gates is not None (e.g. use
`gates if gates is not None else ["auto"]`) so an explicit empty list is
reported correctly, and (2) when iterating gates, if a gate_name is not one of
the known names (pytest, ruff, mypy, npm-test, npm-lint) create a GateCheck with
GateStatus.FAILED and a clear output message (instead of SKIPPED) so
mistyped/explicitly requested unknown gates cause the run to fail (update the
branch that currently constructs GateCheck(..., status=GateStatus.SKIPPED, ...)
to use GateStatus.FAILED when gates was explicitly provided).
🧹 Nitpick comments (8)
codeframe/core/events.py (2)
168-214: Same connection leak issue and code duplication.This function has the same resource leak risk as
emit(). Additionally, there's significant code duplication betweenemit()andemit_for_workspace().Consider extracting shared logic into a private helper:
♻️ Proposed refactor to reduce duplication
def _insert_event( workspace: Workspace, workspace_id: str, event_type: str, payload: dict[str, Any], print_event: bool, ) -> Event: """Internal helper to insert and return an event.""" now = _utc_now().isoformat() payload_json = json.dumps(payload) conn = get_db_connection(workspace) try: cursor = conn.cursor() cursor.execute( "INSERT INTO events (workspace_id, event_type, payload, created_at) VALUES (?, ?, ?, ?)", (workspace_id, event_type, payload_json, now), ) event_id = cursor.lastrowid conn.commit() finally: conn.close() event = Event( id=event_id, workspace_id=workspace_id, event_type=event_type, payload=payload, created_at=datetime.fromisoformat(now), ) if print_event: _print_event(event) return eventThen both public functions can delegate to this helper.
273-303: Infinite polling loop with no exit condition.The
tail()generator runs forever withwhile True. There's no mechanism for the caller to signal shutdown gracefully, and if an exception occurs inlist_recent(), it will propagate without cleanup. Consider:
- Adding a timeout or stop event parameter
- Documenting that callers must handle
GeneratorExitAlso, the
import timeat line 290 should be moved to the top-level imports for consistency.♻️ Move import to top-level
import json +import time from dataclasses import dataclassAnd remove line 290.
codeframe/core/artifacts.py (3)
1-21: Duplicate_utc_nowhelper.This is duplicated from
events.pyand also appears in test files. Consider extracting to a shared utility module.♻️ Suggestion
Create
codeframe/core/utils.py:from datetime import datetime, timezone def utc_now() -> datetime: """Get current UTC time as timezone-aware datetime.""" return datetime.now(timezone.utc)Then import from there in both modules.
262-303: Missing git availability check.Unlike
export_patchandcreate_commit,get_statusdoesn't verify git is available before running commands. If git is missing, the error will be less informative.♻️ Add consistency check
def get_status(repo_path: Path) -> dict: """Get git status summary. ... """ + if not shutil.which("git"): + raise ValueError("git not found in PATH") + result = subprocess.run(
338-378: Moveimport reto top-level.The
import reat line 364 is inside the function. Per Python best practices, imports should be at module level for clarity and to avoid repeated import overhead (though negligible here).♻️ Move import to top
import subprocess import shutil +import re from dataclasses import dataclassThen remove line 364.
codeframe/core/dependency_graph.py (2)
99-99: Remove unused variableparent.The
parentdict is declared and assigned within the loop but never read. This is dead code.🧹 Remove unused variable
# States: 0 = unvisited, 1 = visiting, 2 = visited state: dict[str, int] = {node: 0 for node in graph} - parent: dict[str, Optional[str]] = {node: None for node in graph} def dfs(node: str, path: list[str]) -> Optional[list[str]]: state[node] = 1 # visiting path.append(node) for dep in graph.get(node, []): if dep not in state: continue # dependency not in our graph if state[dep] == 1: # back edge - cycle found # Find where the cycle starts cycle_start = path.index(dep) cycle = path[cycle_start:] + [dep] return cycle if state[dep] == 0: # unvisited - parent[dep] = node result = dfs(dep, path)
172-182: Consider usingdequefor O(1) queue operations.
queue.pop(0)on a list is O(n). Usingcollections.dequewithpopleft()provides O(1) performance.⚡ Use deque for better performance
+from collections import deque + # ... # Start with nodes that have no dependencies - queue = [node for node in graph if in_degree[node] == 0] + queue = deque(node for node in graph if in_degree[node] == 0) result = [] while queue: # Take a node with no remaining dependencies - node = queue.pop(0) + node = queue.popleft() result.append(node)codeframe/core/gates.py (1)
170-203: Consider scanning subdirs for Python files when detecting gates.
repo_path.glob("*.py")only checks the repo root (Line 185), which can skip Python projects usingsrc/or nested packages.♻️ Proposed tweak
- any(repo_path.glob("*.py")) + any(repo_path.rglob("*.py"))
- Comment out frontend-tests, e2e-smoke-tests jobs (web-ui is legacy) - Remove Node.js setup from code-quality job - Add skip checks for web-ui/src in hardcoded-urls job - Update test-summary to remove frontend-tests dependency The web-ui package.json is missing; re-enable these jobs when the frontend is restored.
Code Quality and Architecture Review SummaryThis PR represents an impressive 33K+ line refactor introducing a well-structured v2 architecture with clear separation of concerns (headless core, CLI adapters, LLM abstraction). The extensive reviews from Macroscope, CodeRabbit, and github-actions have thoroughly documented both strengths and areas for improvement. Critical Blocker: v1→v2 Migration PathThe most significant concern raised by github-actions is the missing v1→v2 data migration path. Users with existing v1 workspaces (containing tables like Recommendation: Before merge, implement:
This should be the highest priority item to address. Additional ObservationsAddressed by previous reviews (commit 65f2ddb):
Well-documented strengths:
One final high-level recommendation: Overall, this is a substantial architectural improvement that sets a solid foundation for the v2 CLI-first vision. Once the migration path is documented and implemented, this should be ready for merge. |
|
I've posted a comprehensive review comment to PR #299 that acknowledges the extensive existing analysis and focuses on the critical v1→v2 migration blocker as the main concern. The comment also notes that most CodeRabbit findings have been addressed (commit 65f2ddb) and provides recommendations for implementing a migration path with clear documentation before merge. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| # Update tasks | ||
| updated_count = 0 | ||
| skipped_count = 0 | ||
| for task in matching: |
There was a problem hiding this comment.
Bulk update with --all catches InvalidTransitionError in the generic handler, leaving users unaware which tasks succeeded before the failure. Consider wrapping individual update_status() calls in try/except, collecting failures, and reporting both successes and failures at the end.
🚀 Want me to fix this? Reply ex: "fix it for me".
artifacts.py: - Track which diff was actually used when falling back from staged to unstaged, ensuring stats match the exported patch content dependency_graph.py: - Remove dead no-op loop in topological_sort that computed in_degree but only contained pass statements events.py: - Add try/finally to emit() to ensure DB connection closes on exception - Add try/finally to emit_for_workspace() for same reason - Add try/finally to list_recent() to ensure DB connection closes gates.py: - Add ERROR status count to GateResult.summary property - Fix GATES_STARTED event to report actual empty list vs ["auto"] - Make unknown gates FAILED (not SKIPPED) when explicitly requested, with helpful error message listing valid gate names
Critical v1→v2 Migration BlockerThis is an impressive architectural refactor with excellent separation of concerns (headless core, CLI adapters, LLM abstraction). However, there's one critical blocker that must be addressed before merge. 🚨 Critical Issue: No v1→v2 Migration PathUsers with existing v1 workspaces will lose access to their data when running any v2 command. The schema is incompatible: v1 schema: , , , , The function in only handles v2→v2 upgrades (adding columns like ), not v1→v2 migration. When a v1 workspace exists, the CLI assumes it's compatible and proceeds, but v1 tables won't have the required v2 columns. RecommendationAdd a migration check in : Then implement CLI command in . Other Critical IssuesSecurity (from CodeRabbit):
Database Leaks (from CodeRabbit):
Architecture (from CodeRabbit):
Positive Notes✅ Excellent architectural separation (headless core, CLI adapters, LLM abstraction) SummaryThe v2 architecture is well-designed and follows the documented Golden Path. The migration issue is the only blocking concern - without it, existing users will experience data loss. Please add a command before merging, or at minimum warn users with v1 workspaces before proceeding with v2 commands. The security and DB leak issues are important but could be addressed in follow-up PRs. |
Critical v1→v2 Migration BlockerThis is an impressive architectural refactor with excellent separation of concerns (headless core, CLI adapters, LLM abstraction). However, there's one critical blocker that must be addressed before merge. 🚨 Critical Issue: No v1→v2 Migration PathUsers with existing v1 workspaces will lose access to their data when running any v2 command. The schema is incompatible: v1 schema: , , , , The function in only handles v2→v2 upgrades (adding columns like ), not v1→v2 migration. When a v1 workspace exists, CLI assumes it's compatible and proceeds, but v1 tables won't have required v2 columns. RecommendationAdd migration detection in : Then implement CLI command in . Other Critical Issues (from CodeRabbit/github-actions)Security:
Database Leaks:
Architecture:
Code Quality:
Positive Notes✅ Excellent architectural separation (headless core, CLI adapters, LLM abstraction) SummaryThe v2 architecture is well-designed and follows the documented Golden Path. The migration issue is the only blocking concern - without it, existing users will experience data loss. Please add a command before merging, or at minimum warn users with v1 workspaces before proceeding with v2 commands. The security and DB leak issues are important but could be addressed in follow-up PRs. |
|
I've posted a comprehensive review comment to PR #299 highlighting the critical v1→v2 migration blocker while acknowledging the excellent work done on this architectural refactor. The comment provides a concrete recommendation for adding migration detection and a |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@codeframe/core/artifacts.py`:
- Around line 120-130: The fallback path that sets actual_staged_used=False can
produce a patch but leaves stats computed by _get_diff_stats(repo_path,
staged_only=False) which still runs "git diff HEAD --stat" and may return zeros;
instead, when you detect you fell back to unstaged (actual_staged_used is False)
parse the stats directly from the actual patch content via
_parse_patch_stats(patch_content) (or modify _get_diff_stats to accept the raw
patch and use _parse_patch_stats when provided) so functions like the block that
builds patch_content, actual_staged_used, and the subsequent _get_diff_stats
call produce accurate stats.
In `@codeframe/core/dependency_graph.py`:
- Around line 212-217: The code in calculate_level uses max(calculate_level(dep)
for dep in deps if dep in graph) which can raise ValueError when that generator
is empty; change calculate_level to handle the case where none of deps are
present in graph (e.g., compute dep_levels = [calculate_level(dep) for dep in
deps if dep in graph], then set max_dep_level = max(dep_levels, default=0) or
treat missing deps as level 0) and then assign levels[node] = max_dep_level + 1
so nodes with no valid in-graph deps don't error; update references to
calculate_level, deps, levels, graph, and node accordingly.
♻️ Duplicate comments (1)
codeframe/core/gates.py (1)
83-84: Pluralization inconsistency for error count.The summary uses
f"{error_count} errors"unconditionally, which produces "1 errors" for a single error. Other counts (passed, failed, skipped) don't have this issue because they use bare nouns.🐛 Proposed fix
if error_count: - parts.append(f"{error_count} errors") + parts.append(f"{error_count} error{'s' if error_count != 1 else ''}")
🧹 Nitpick comments (6)
codeframe/core/gates.py (3)
21-23: Consider consolidating_utc_now()helper.This function is duplicated across multiple core modules (
gates.py,events.py,artifacts.py). Consider extracting to a shared utility module to maintain DRY.
209-218: Bare exception handler may hide parsing issues.The
except Exception: passblock silently swallows all errors when parsingpackage.json, including issues like permission errors or malformed JSON. Consider logging at debug level for troubleshooting.
348-353: Consider addinguv runfallback for mypy.The
_run_mypyfunction only checks forshutil.which("mypy")directly, unlike_run_pytestand_run_ruffwhich also support running viauv run. For consistency, consider adding the same fallback pattern.♻️ Suggested change
- if not shutil.which("mypy"): + if not shutil.which("mypy") and not shutil.which("uv"): return GateCheck( name="mypy", status=GateStatus.SKIPPED, output="mypy not found", ) try: - result = subprocess.run( - ["mypy", "."], + if shutil.which("uv"): + cmd = ["uv", "run", "mypy", "."] + else: + cmd = ["mypy", "."] + + result = subprocess.run( + cmd,codeframe/core/dependency_graph.py (2)
99-99: Unusedparentdictionary.The
parentdict is initialized and populated but never read. This appears to be leftover from an alternative cycle reconstruction approach.🧹 Remove unused code
# States: 0 = unvisited, 1 = visiting, 2 = visited state: dict[str, int] = {node: 0 for node in graph} - parent: dict[str, Optional[str]] = {node: None for node in graph} def dfs(node: str, path: list[str]) -> Optional[list[str]]: state[node] = 1 # visiting @@ -113,7 +112,6 @@ return cycle if state[dep] == 0: # unvisited - parent[dep] = node result = dfs(dep, path) if result: return resultAlso applies to: 116-116
160-170: Performance: O(n²) topological sort implementation.The current implementation uses
queue.pop(0)which is O(n), and iterates all nodes to find dependents (lines 166-170), making the overall complexity O(n²). For small task sets this is acceptable, but consider optimizing if large batches are expected.♻️ More efficient implementation using deque and reverse adjacency
+from collections import deque + def topological_sort(graph: dict[str, list[str]]) -> list[str]: ... in_degree = {node: len(graph.get(node, [])) for node in graph} + # Build reverse adjacency: who depends on each node + dependents: dict[str, list[str]] = {node: [] for node in graph} + for node, deps in graph.items(): + for dep in deps: + if dep in dependents: + dependents[dep].append(node) + # Start with nodes that have no dependencies - queue = [node for node in graph if in_degree[node] == 0] + queue = deque(node for node in graph if in_degree[node] == 0) result = [] while queue: # Take a node with no remaining dependencies - node = queue.pop(0) + node = queue.popleft() result.append(node) # For each node that depends on this node, reduce its in_degree - for other_node in graph: - if node in graph.get(other_node, []): - in_degree[other_node] -= 1 - if in_degree[other_node] == 0: - queue.append(other_node) + for dependent in dependents[node]: + in_degree[dependent] -= 1 + if in_degree[dependent] == 0: + queue.append(dependent)codeframe/core/events.py (1)
138-140: Implicit fallback tocwdmay cause unexpected behavior.When
repo_pathisNone, the function silently defaults toPath.cwd(). If the current directory isn't a workspace,get_workspace()will raiseFileNotFoundError. Consider documenting this behavior or requiringrepo_pathexplicitly.
- Register auth_app from auth_commands.py in main CLI app - Fix test_credential_commands.py tests to mock get_credential_source - Skip test_serve_command.py tests (serve is stub during v2 refactor) - Skip test_cli_session.py tests (session management not in v2 Golden Path)
|
|
||
| console.print(f"\n[bold]Diff: v{version1} → v{version2}[/bold]\n") | ||
| # Color the diff output | ||
| for line in diff.splitlines(): |
There was a problem hiding this comment.
Rich is interpreting bracketed user/content text as markup, causing MarkupError or corrupted output. Suggest treating it as plain text: use rich.markup.escape() or pass markup=False when printing arbitrary lines/output.
🚀 Want me to fix this? Reply ex: "fix it for me".
These tests require a running FastAPI server with full WebSocket support, but the v2 serve command is a stub. The server adapter will be implemented post-Golden Path.
Critical Blocker: v1→v2 Migration PathThe most significant concern is the missing migration path for existing v1 workspaces. Users with existing v1 data will find their data inaccessible when running v2 commands because v1 and v2 schemas are incompatible. Critical Issues Requiring Action:
What's Already Fixed: Positive Notes: Recommendation: Consider running Golden Path end-to-end test before merging to validate complete workflow, and address migration path as blocking issue. |
|
I reviewed PR #299 and posted a comprehensive review highlighting: Critical Blocker: No v1→v2 migration path for existing workspaces (potential data loss) Key Issues:
Positives:
Recommendation: Add |
artifacts.py: - When falling back to plain unstaged diff (git diff without HEAD), parse stats directly from patch content via _parse_patch_content_stats() - _get_diff_stats with staged_only=False runs "git diff HEAD --stat" which may return zeros for pure working tree changes dependency_graph.py: - Fix ValueError when max() is called on empty generator in calculate_level() - Use max(dep_levels, default=-1) to handle nodes with deps not in graph - Nodes with no valid in-graph deps are treated as level 0 (root nodes)
Outstanding Critical Issue: v1→v2 Migration PathThis is an impressive architectural refactor introducing a headless, CLI-first v2 system. The existing reviews from CodeRabbit, Macroscope, and github-actions have thoroughly documented both the strengths and concerns. ✅ Strengths Already AddressedMost CodeRabbit findings were already fixed in commit 65f2ddb:
🚨 Critical Blocker: Missing v1→v2 Migration PathThe most significant concern is the absence of a migration path for existing v1 workspaces: Problem: Users with existing v1 workspaces (tables: Evidence (codeframe/core/workspace.py:324-331):
Recommendation: Before merging, implement one of:
The security concerns (command injection, path traversal) and database leaks are valid but secondary to the migration blocker. The architectural refactor is excellent and follows the documented Golden Path approach. |
|
I've posted a review focusing on the critical v1→v2 migration blocker that could cause data loss for existing users. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@codeframe/core/artifacts.py`:
- Around line 102-135: The current diff logic fails for repos without a HEAD
(new repos) because "git diff HEAD" can error or omit staged changes; update the
logic in the artifact generation code that uses staged_only, diff_cmd,
actual_staged_used, fell_back_to_plain_unstaged and patch_content to explicitly
detect a missing HEAD (e.g., run "git rev-parse --verify HEAD" or inspect the
subprocess result for the fatal/exit code) and, when HEAD is missing or the diff
call returns empty/error, produce patch_content by concatenating the staged (git
diff --cached) and unstaged (git diff) outputs so stats are computed from the
combined content; ensure actual_staged_used and fell_back_to_plain_unstaged are
set appropriately when using the concatenated diffs.
🧹 Nitpick comments (3)
codeframe/core/dependency_graph.py (3)
99-99: Remove unusedparentdict.The
parentdictionary is initialized and assigned to on line 116, but never read. This is dead code that can be safely removed.🧹 Proposed cleanup
# States: 0 = unvisited, 1 = visiting, 2 = visited state: dict[str, int] = {node: 0 for node in graph} - parent: dict[str, Optional[str]] = {node: None for node in graph} def dfs(node: str, path: list[str]) -> Optional[list[str]]: state[node] = 1 # visiting path.append(node) for dep in graph.get(node, []): if dep not in state: continue # dependency not in our graph if state[dep] == 1: # back edge - cycle found # Find where the cycle starts cycle_start = path.index(dep) cycle = path[cycle_start:] + [dep] return cycle if state[dep] == 0: # unvisited - parent[dep] = node result = dfs(dep, path)
160-170: Consider usingdequeand a reverse adjacency list for better performance.
queue.pop(0)is O(n). Additionally, the inner loop (lines 166-170) scans all nodes to find dependents, making the algorithm O(V²) instead of optimal O(V+E). For small task graphs this is fine, but if graphs grow larger, precomputing a reverse adjacency map and usingcollections.dequewould improve performance.♻️ Optional optimization
+from collections import deque + def topological_sort(graph: dict[str, list[str]]) -> list[str]: ... in_degree = {node: len(graph.get(node, [])) for node in graph} + + # Build reverse adjacency: node -> list of nodes that depend on it + dependents: dict[str, list[str]] = {node: [] for node in graph} + for node, deps in graph.items(): + for dep in deps: + if dep in dependents: + dependents[dep].append(node) # Start with nodes that have no dependencies - queue = [node for node in graph if in_degree[node] == 0] + queue = deque(node for node in graph if in_degree[node] == 0) result = [] while queue: - node = queue.pop(0) + node = queue.popleft() result.append(node) - for other_node in graph: - if node in graph.get(other_node, []): - in_degree[other_node] -= 1 - if in_degree[other_node] == 0: - queue.append(other_node) + for dependent in dependents[node]: + in_degree[dependent] -= 1 + if in_degree[dependent] == 0: + queue.append(dependent)
289-290: Consider narrowing the exception catch.Catching bare
Exceptionmay hide unexpected bugs. Sincebuild_graphanddetect_cycleare the only operations, consider catching more specific exceptions or at least excluding base exceptions likeKeyboardInterrupt.♻️ Optional refinement
- except Exception as e: + except (CycleDetectedError, KeyError, ValueError) as e: return False, str(e)Alternatively, if you want to remain defensive but avoid swallowing critical errors:
- except Exception as e: + except Exception as e: # noqa: BLE001 - intentionally broad for validation return False, str(e)
| # Generate patch content | ||
| # Track which diff was actually used for stats calculation | ||
| actual_staged_used = staged_only | ||
|
|
||
| if staged_only: | ||
| diff_cmd = ["git", "diff", "--cached"] | ||
| else: | ||
| diff_cmd = ["git", "diff", "HEAD"] | ||
|
|
||
| result = subprocess.run( | ||
| diff_cmd, | ||
| cwd=repo_path, | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
|
|
||
| patch_content = result.stdout | ||
|
|
||
| # Track if we fell back to plain unstaged diff (git diff without HEAD) | ||
| fell_back_to_plain_unstaged = False | ||
|
|
||
| if not patch_content.strip(): | ||
| # Try just unstaged changes (git diff without HEAD) | ||
| result = subprocess.run( | ||
| ["git", "diff"], | ||
| cwd=repo_path, | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| patch_content = result.stdout | ||
| # We fell back to unstaged diff, so update the tracking flag | ||
| actual_staged_used = False | ||
| fell_back_to_plain_unstaged = True | ||
|
|
There was a problem hiding this comment.
Handle repos without HEAD to avoid missing staged changes.
In a repo with no commits, git diff HEAD fails and the fallback to git diff can omit staged changes entirely (or raise "No changes"). Consider detecting missing HEAD and concatenating staged + unstaged diffs, then computing stats from the combined content.
🐛 Proposed fix
- if staged_only:
- diff_cmd = ["git", "diff", "--cached"]
- else:
- diff_cmd = ["git", "diff", "HEAD"]
+ head_exists = (
+ subprocess.run(
+ ["git", "rev-parse", "--verify", "HEAD"],
+ cwd=repo_path,
+ capture_output=True,
+ text=True,
+ ).returncode
+ == 0
+ )
+
+ if staged_only:
+ diff_cmds = [["git", "diff", "--cached"]]
+ elif head_exists:
+ diff_cmds = [["git", "diff", "HEAD"]]
+ else:
+ # No HEAD yet: include staged + unstaged
+ diff_cmds = [["git", "diff", "--cached"], ["git", "diff"]]
- result = subprocess.run(
- diff_cmd,
- cwd=repo_path,
- capture_output=True,
- text=True,
- )
-
- patch_content = result.stdout
+ patch_chunks = []
+ for diff_cmd in diff_cmds:
+ result = subprocess.run(
+ diff_cmd,
+ cwd=repo_path,
+ capture_output=True,
+ text=True,
+ )
+ if result.returncode != 0:
+ raise ValueError(f"git diff failed: {result.stderr}")
+ patch_chunks.append(result.stdout)
+ patch_content = "".join(patch_chunks)
@@
- if fell_back_to_plain_unstaged:
+ if fell_back_to_plain_unstaged or (not staged_only and not head_exists):
stats = _parse_patch_content_stats(patch_content)
else:
stats = _get_diff_stats(repo_path, actual_staged_used)Also applies to: 150-155
🤖 Prompt for AI Agents
In `@codeframe/core/artifacts.py` around lines 102 - 135, The current diff logic
fails for repos without a HEAD (new repos) because "git diff HEAD" can error or
omit staged changes; update the logic in the artifact generation code that uses
staged_only, diff_cmd, actual_staged_used, fell_back_to_plain_unstaged and
patch_content to explicitly detect a missing HEAD (e.g., run "git rev-parse
--verify HEAD" or inspect the subprocess result for the fatal/exit code) and,
when HEAD is missing or the diff call returns empty/error, produce patch_content
by concatenating the staged (git diff --cached) and unstaged (git diff) outputs
so stats are computed from the combined content; ensure actual_staged_used and
fell_back_to_plain_unstaged are set appropriately when using the concatenated
diffs.
Critical v1→v2 Migration BlockerThis is an impressive architectural refactor introducing a well-structured v2 system. The 33K+ lines demonstrate excellent separation of concerns with a headless core, CLI adapters, and comprehensive orchestration. 🚨 Critical Issue: No v1→v2 Migration PathThis is the only blocker that should prevent merge: Users with existing v1 workspaces will lose access to their data when running any v2 command. The v1 schema (tables: , , , , etc.) is incompatible with v2 (tables: , , , , etc.). Evidence:
Recommended Action: Add a migration detection and command before merging. This should:
Positive Notes✅ Excellent headless core architecture The architectural refactor follows the documented Golden Path approach and sets a solid foundation for v2. Once the migration path is resolved, this should be ready to merge. |
Critical v1→v2 Migration BlockerThis is an impressive architectural refactor introducing a well-structured v2 system. The 33K+ lines demonstrate excellent separation of concerns with a headless core, CLI adapters, and comprehensive orchestration. 🚨 Critical Issue: No v1→v2 Migration PathThis is the only blocker that should prevent merge: Users with existing v1 workspaces will lose access to their data when running any v2 command. The v1 schema (tables: Evidence:
Recommended Action: Add migration detection and a
Positive Notes✅ Excellent headless core architecture The architectural refactor follows the documented Golden Path approach and sets a solid foundation for v2. Once a migration path is implemented, this should be ready to merge. |
Critical v1→v2 Migration BlockerThis is an impressive architectural refactor introducing a well-structured v2 system. The 33K+ lines demonstrate excellent separation of concerns with a headless core, CLI adapters, and comprehensive orchestration. 🚨 Critical Issue: No v1→v2 Migration PathThis is the only blocker that should prevent merge: Users with existing v1 workspaces will lose access to their data when running any v2 command. The v1 schema (tables: Evidence:
Recommended Action: Add a migration detection and
Positive Notes✅ Excellent headless core architecture The architectural refactor follows the documented Golden Path approach and sets a solid foundation for v2. Once a migration path is implemented, this should be ready to merge. |
|
I've posted a comprehensive review comment on PR #299 highlighting the critical v1→v2 migration blocker as the main concern, while acknowledging the excellent architectural work done. |
1 participant
Summary
This PR introduces the CodeFRAME v2 refactor - a complete architectural shift to a headless, CLI-first design following the "strangler fig" pattern. The v2 architecture enables fully autonomous AI-driven development without requiring a running server.
Key Changes
codeframe/core/with no FastAPI/HTTP dependenciescodeframe init,prd add,tasks generate,work start --execute)Architecture Highlights
core/agent.pycore/planner.pycore/executor.pycore/conductor.pycore/dependency_graph.pyadapters/llm/CLI Commands (v2)
Testing
pytest -m v2)Test plan
uv run pytest -m v2- all tests passuv run ruff check .- no linting errorscf init . --detectcf prd add docs/sample.mdcf tasks generatecf work start <task-id> --execute --verbosepython -m codeframe --helpshows correct program namecf work batch run --all-readySummary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.