Rebrand plugin and planning metadata#5
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates Clodex’s workflow and metadata to support the new “isolated worktree build → explicit apply” model, expands auditing into a multi-reviewer system with tracing/state persistence, and refreshes docs/templates/tests to match the new structure.
Changes:
- Introduces git-worktree isolated build workspaces plus
clodex apply <run-id>to apply an approved patch back to the source checkout. - Adds durable async task runs (start/get/cancel/list), run tracing (JSONL + DB events), and state schema v2 tables/columns.
- Updates config defaults, prompts/reviewer metadata, docs, and unit tests to reflect the revised workflow.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_clodex.py | Adds coverage for worktree isolation/apply, reviewer metadata, state schema v2, tracing, hooks, and task lifecycle. |
| skills/clodex-workflow/SKILL.md | Updates skill guidance to describe worktree builds, apply, and async task commands. |
| README.md | Documents new commands/artifacts and updated safety model (worktree isolation + apply). |
| dist/.claude/commands/clodex/clodex-build.md | Updates Claude command doc to mention worktree default and apply step. |
| clodex/workspace.py | Implements workspace selection and git-worktree preparation with dirty-tracked check. |
| clodex/workflow.py | Refactors workflow to support workspaces, apply, tracing, cancellations, and multi-reviewer agreements. |
| clodex/trace.py | Adds JSONL trace writer with optional persistence into the state DB. |
| clodex/tasks.py | Introduces async task manager that spawns a worker process and supports get/list/cancel. |
| clodex/state.py | Adds schema_version + v2 tables and run columns (artifacts/workspace/pid/error/timestamps/cancellations). |
| clodex/prompts.py | Extends audit prompt to include reviewer_id/persona metadata in the JSON contract. |
| clodex/mcp_server.py | Adds MCP “tasks/*” method handling and new task tools. |
| clodex/hooks.py | Adds hook config generation and ingest pipeline with trace logging. |
| clodex/evals.py | Adds a small local eval harness validating config/state/reviewer defaults. |
| clodex/config.py | Updates defaults (workspace, approval_profile, reviewers, mcp async, tracing) and improves scalar parsing for inline JSON. |
| clodex/commands.py | Adds approval profiles to Codex exec command (--ask-for-approval behavior + auto_review reviewer config). |
| clodex/cli.py | Adds new CLI subcommands (apply/trace/eval/hooks/task) and build flags (workspace/approval-profile/apply). |
| clodex/artifacts.py | Ensures parent dirs exist for nested artifact writes (e.g., reviewers/*.json). |
| clodex/agents.py | Adds subprocess timeout support for agent runner invocations. |
| CLODEX.md | Updates workflow contract/config defaults to reflect worktree isolation, reviewers, async tasks, and tracing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def cancel(self, run_id: str) -> WorkflowResult: | ||
| run = self.state.get_run(run_id) | ||
| if run is None: | ||
| raise ValueError(f"Unknown run: {run_id}") | ||
| self.state.request_cancel(run_id) | ||
| pid = run.get("pid") | ||
| if pid: | ||
| self._terminate(int(pid)) | ||
| self.state.complete_cancel(run_id) | ||
| updated = self.state.get_run(run_id) or run | ||
| return WorkflowResult(str(updated["status"]), run_id, updated.get("task_id"), updated.get("artifacts_dir"), {"cancel_requested": True}) | ||
|
|
| hooks_install = hooks_sub.add_parser("install") | ||
| hooks_install.add_argument("--dry-run", action="store_true", required=True) | ||
| hooks_ingest = hooks_sub.add_parser("ingest") |
| if args.hooks_command == "install": | ||
| print_output({"dry_run": True, "config": hook_config(Path.cwd())}, as_json) | ||
| return 0 |
| The server also handles MCP-style `tasks/get`, `tasks/update`, and | ||
| `tasks/cancel` JSON-RPC methods using the Clodex `run_id` as the task id. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces isolated git worktree builds, asynchronous task management, tracing, and multi-reviewer audits to Clodex, supported by database schema updates and new CLI commands. The code review feedback highlights several critical improvements for robustness: wrapping the build execution in a try-except-finally block to handle failures and release workspace locks, implementing the missing lock release method in the state store, preventing trailing path separators in PYTHONPATH, running git worktree prune to clean up stale metadata, and ensuring untracked files are handled properly during local workspace builds.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| trace = TraceWriter(artifacts.path, run_id, self.state if self.config.tracing.get("enabled") else None) | ||
| trace.event("run.start", {"command": "build", "task": task}) | ||
| try: | ||
| workspace = WorkspaceManager(self.repo_root, self.config).prepare(run_id, workspace_backend) | ||
| except DirtyWorkspaceError as exc: | ||
| self.state.update_run(run_id, "blocked", error=str(exc)) | ||
| self.state.update_task(task_id, "blocked") | ||
| trace.event("workspace.blocked", {"error": str(exc)}) | ||
| return WorkflowResult("blocked", run_id, task_id, str(artifacts.path), {"error": str(exc)}) | ||
|
|
||
| WorkspaceManager(self.repo_root, self.config).write_metadata(artifacts.path, workspace) | ||
| self.state.add_workspace_lock(run_id, str(workspace.source_path), str(workspace.path), workspace.backend) | ||
| self.state.update_run(run_id, "running", workspace_path=str(workspace.path), artifacts_dir=str(artifacts.path)) | ||
| trace.event("workspace.ready", workspace.as_dict()) | ||
|
|
||
| runner = AgentRunner(workspace.path) | ||
| if self.state.cancellation_requested(run_id): | ||
| self.state.complete_cancel(run_id) | ||
| trace.event("run.cancelled", {}) | ||
| return WorkflowResult("cancelled", run_id, task_id, str(artifacts.path), {"workspace": workspace.as_dict()}) | ||
|
|
||
| plan_json = self._run_json_with_retry(runner, claude_plan_command(self.config), plan_prompt(self.config, task), "Claude planning", trace) | ||
| artifacts.write_json("01-claude-plan.json", plan_json) | ||
| if self._cancelled(run_id, trace): | ||
| return WorkflowResult("cancelled", run_id, task_id, str(artifacts.path), {"workspace": workspace.as_dict()}) | ||
|
|
||
| implementation = runner.run(codex_exec_command(self.config, workspace.path, approval_profile=approval_profile), implementation_prompt(plan_json, task)) | ||
| trace.event("command.complete", {"name": implementation.command.name, "returncode": implementation.returncode}) | ||
| artifacts.write_text("02-codex-implementation.md", self._format_agent_report(implementation.stdout, implementation.stderr)) | ||
| if not implementation.ok: | ||
| self.state.update_run(run_id, "blocked", error="Codex implementation failed") | ||
| self.state.update_task(task_id, "blocked") | ||
| return WorkflowResult("blocked", run_id, task_id, str(artifacts.path), {"error": "Codex implementation failed", "workspace": workspace.as_dict()}) | ||
| if self._cancelled(run_id, trace): | ||
| return WorkflowResult("cancelled", run_id, task_id, str(artifacts.path), {"workspace": workspace.as_dict()}) | ||
|
|
||
| result = self._audit_loop(task_id, run_id, artifacts, plan_json, runner, workspace.path, trace, approval_profile, include_untracked=workspace.is_worktree) | ||
| result.data["workspace"] = workspace.as_dict() | ||
| if result.status == "approved" and workspace.is_worktree: | ||
| self._include_untracked(workspace.path) | ||
| patch = artifacts.write_text("apply.patch", current_diff(workspace.path)) | ||
| result.data["apply_patch"] = str(patch) | ||
| if apply_changes: | ||
| return self.apply_run(run_id) | ||
| return result |
There was a problem hiding this comment.
Wrap the core execution of _execute_build in a try...except...finally block. This ensures that:
- Any unhandled exceptions (e.g.,
RuntimeErrorfrom_run_json_with_retryorsubprocess.TimeoutExpired) are caught, the run status is updated tofailedin the database, and the failure is traced (instead of leaving the run inrunningstate indefinitely). - The workspace lock is always released (updating
released_atin theworkspace_lockstable) when the run completes, is blocked, fails, or is cancelled.
Here is how the structure would look:
try:
WorkspaceManager(self.repo_root, self.config).write_metadata(artifacts.path, workspace)
self.state.add_workspace_lock(run_id, str(workspace.source_path), str(workspace.path), workspace.backend)
self.state.update_run(run_id, "running", workspace_path=str(workspace.path), artifacts_dir=str(artifacts.path))
trace.event("workspace.ready", workspace.as_dict())
try:
runner = AgentRunner(workspace.path)
if self.state.cancellation_requested(run_id):
self.state.complete_cancel(run_id)
trace.event("run.cancelled", {})
return WorkflowResult("cancelled", run_id, task_id, str(artifacts.path), {"workspace": workspace.as_dict()})
plan_json = self._run_json_with_retry(runner, claude_plan_command(self.config), plan_prompt(self.config, task), "Claude planning", trace)
artifacts.write_json("01-claude-plan.json", plan_json)
if self._cancelled(run_id, trace):
return WorkflowResult("cancelled", run_id, task_id, str(artifacts.path), {"workspace": workspace.as_dict()})
implementation = runner.run(codex_exec_command(self.config, workspace.path, approval_profile=approval_profile), implementation_prompt(plan_json, task))
trace.event("command.complete", {"name": implementation.command.name, "returncode": implementation.returncode})
artifacts.write_text("02-codex-implementation.md", self._format_agent_report(implementation.stdout, implementation.stderr))
if not implementation.ok:
self.state.update_run(run_id, "blocked", error="Codex implementation failed")
self.state.update_task(task_id, "blocked")
return WorkflowResult("blocked", run_id, task_id, str(artifacts.path), {"error": "Codex implementation failed", "workspace": workspace.as_dict()})
if self._cancelled(run_id, trace):
return WorkflowResult("cancelled", run_id, task_id, str(artifacts.path), {"workspace": workspace.as_dict()})
result = self._audit_loop(task_id, run_id, artifacts, plan_json, runner, workspace.path, trace, approval_profile, include_untracked=workspace.is_worktree)
result.data["workspace"] = workspace.as_dict()
if result.status == "approved" and workspace.is_worktree:
self._include_untracked(workspace.path)
patch = artifacts.write_text("apply.patch", current_diff(workspace.path))
result.data["apply_patch"] = str(patch)
if apply_changes:
return self.apply_run(run_id)
return result
except Exception as exc:
self.state.update_run(run_id, "failed", error=str(exc))
self.state.update_task(task_id, "blocked")
trace.event("run.failed", {"error": str(exc)})
raise
finally:
self.state.release_workspace_lock(run_id)
except DirtyWorkspaceError as exc:
self.state.update_run(run_id, "blocked", error=str(exc))
self.state.update_task(task_id, "blocked")
trace.event("workspace.blocked", {"error": str(exc)})
return WorkflowResult("blocked", run_id, task_id, str(artifacts.path), {"error": str(exc)})| def add_workspace_lock(self, run_id: str, source_path: str, workspace_path: str, backend: str) -> None: | ||
| with self.session() as con: | ||
| con.execute( | ||
| """ | ||
| insert into workspace_locks(run_id, source_path, workspace_path, backend, created_at) | ||
| values (?, ?, ?, ?, ?) | ||
| on conflict(run_id) do update set | ||
| source_path=excluded.source_path, | ||
| workspace_path=excluded.workspace_path, | ||
| backend=excluded.backend | ||
| """, | ||
| (run_id, source_path, workspace_path, backend, now_iso()), | ||
| ) |
There was a problem hiding this comment.
The workspace locking mechanism currently lacks a way to release locks. The released_at column in the workspace_locks table is never updated. Add a release_workspace_lock method to StateStore so it can be called when the run completes or is cancelled.
def add_workspace_lock(self, run_id: str, source_path: str, workspace_path: str, backend: str) -> None:
with self.session() as con:
con.execute(
"""
insert into workspace_locks(run_id, source_path, workspace_path, backend, created_at)
values (?, ?, ?, ?, ?)
on conflict(run_id) do update set
source_path=excluded.source_path,
workspace_path=excluded.workspace_path,
backend=excluded.backend
""",
(run_id, source_path, workspace_path, backend, now_iso()),
)
def release_workspace_lock(self, run_id: str) -> None:
with self.session() as con:
con.execute(
"update workspace_locks set released_at=? where run_id=? and released_at is null",
(now_iso(), run_id),
)| env = os.environ.copy() | ||
| root = str(Path(__file__).resolve().parents[1]) | ||
| env["PYTHONPATH"] = root + os.pathsep + env.get("PYTHONPATH", "") |
There was a problem hiding this comment.
When PYTHONPATH is not set in the environment, env.get("PYTHONPATH", "") returns an empty string, resulting in a trailing path separator in PYTHONPATH. In Python, a trailing separator in PYTHONPATH is interpreted as including the current working directory ("") in sys.path, which can lead to unexpected module resolution issues or security vulnerabilities if untrusted files are present in the working directory. Only append the separator if existing_pythonpath is present and non-empty.
| env = os.environ.copy() | |
| root = str(Path(__file__).resolve().parents[1]) | |
| env["PYTHONPATH"] = root + os.pathsep + env.get("PYTHONPATH", "") | |
| env = os.environ.copy() | |
| root = str(Path(__file__).resolve().parents[1]) | |
| existing_pythonpath = env.get("PYTHONPATH") | |
| env["PYTHONPATH"] = f"{root}{os.pathsep}{existing_pythonpath}" if existing_pythonpath else root |
| if not path.exists(): | ||
| subprocess.run(["git", "worktree", "add", "--detach", str(path), "HEAD"], cwd=self.repo_root, check=True, capture_output=True, text=True) |
There was a problem hiding this comment.
If a worktree directory is manually deleted or if there are stale worktree registrations in git, git worktree add will fail because git still has the worktree registered in its metadata. Running git worktree prune before adding a new worktree cleans up any stale worktree metadata and prevents these failures.
| if not path.exists(): | |
| subprocess.run(["git", "worktree", "add", "--detach", str(path), "HEAD"], cwd=self.repo_root, check=True, capture_output=True, text=True) | |
| if not path.exists(): | |
| subprocess.run(["git", "worktree", "prune"], cwd=self.repo_root, check=False, capture_output=True) | |
| subprocess.run(["git", "worktree", "add", "--detach", str(path), "HEAD"], cwd=self.repo_root, check=True, capture_output=True, text=True) |
| if self._cancelled(run_id, trace): | ||
| return WorkflowResult("cancelled", run_id, task_id, str(artifacts.path), {"workspace": workspace.as_dict()}) | ||
|
|
||
| result = self._audit_loop(task_id, run_id, artifacts, plan_json, runner, workspace.path, trace, approval_profile, include_untracked=workspace.is_worktree) |
There was a problem hiding this comment.
For the local workspace backend, workspace.is_worktree is False, which means include_untracked is set to False. Consequently, any new files created by the implementation agent (codex) in a local workspace will remain untracked and won't be included in the audit or the final diff/patch. Consider enabling untracked file inclusion or handling for local builds as well, or at least document/warn about this limitation.
Code Review Roast 🔥Verdict: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)
🏆 Best part: The test suite actually covers the worker lifecycle and cancellation flows — I'm as shocked as you are. 💀 Worst part: The missing error handling on 📊 Overall: Like a first pancake — the shape is wrong but the ingredients are there. Files Reviewed (4 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash-20260528 · Input: 232.6K · Output: 22.7K · Cached: 1.2M |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| } | ||
| for name, kind in columns.items(): | ||
| if name not in existing: | ||
| con.execute(f"alter table runs add column {name} {kind}") |
There was a problem hiding this comment.
🔥 The Roast: You're f-string-interpolating identifiers into DDL like it's going out of style. The values are hardcoded now so this particular grenade still has the pin in, but the moment someone refactors columns to come from config or env vars, you've invited SQL injection to your database party.
🩹 The Fix: SQLite doesn't parameterize DDL, so validate name and kind against a strict allow-list before interpolation. At minimum, assert kind in {"text", "integer"} and match name against ^[a-zA-Z_][a-zA-Z0-9_]*$.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| """ | ||
| update runs set | ||
| status=?, | ||
| diff_hash=coalesce(?, diff_hash), |
There was a problem hiding this comment.
🔥 The Roast: coalesce(?, field) makes these columns forever-sticky. Once error, pid, or completed_at is set, there is absolutely no way to clear them back to NULL. It's like setting concrete and then being told you can't re-pour — even when the first pour was a mistake.
🩹 The Fix: Use CASE WHEN ? IS NOT NULL THEN ? ELSE field END instead of COALESCE, or add explicit NULL passthrough logic so callers can actually clear these fields.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| "attempt": attempt, | ||
| "diff_hash": diff_hash, | ||
| "reviewers": reviewer_status, | ||
| "claude_approved": reviewer_status.get("claude-plan", {}).get("approved", False), |
There was a problem hiding this comment.
🔥 The Roast: You hardcoded claude-plan and codex-architecture into _agreement() like reviewer IDs are set in stone. But they're dynamically derived from config! If a user customizes their reviewer IDs, both claude_approved and codex_approved will silently report False forever, and any downstream logic depending on those keys will break in the most confusing way possible.
🩹 The Fix: Compute the legacy keys from the config reviewes list dynamically, or drop the hardcoded fields entirely in favor of the generic reviewers map that already exists.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| print_output(data, as_json) | ||
| return 0 | ||
| if args.task_command == "cancel": | ||
| result = manager.cancel(args.run_id) |
There was a problem hiding this comment.
🔥 The Roast: manager.cancel(args.run_id) will raise a raw ValueError if the run doesn't exist, which means clodex task cancel <random-string> dumps a Python traceback to the user's terminal instead of a clean error message. The MCP tasks/cancel handler wraps this in try/except — the CLI deserves the same courtesy.
🩹 The Fix: Wrap in try/except and print a structured error:
| result = manager.cancel(args.run_id) | |
| if args.task_command == "cancel": | |
| try: | |
| result = manager.cancel(args.run_id) | |
| except ValueError as exc: | |
| print_output({"error": str(exc)}, as_json) | |
| return 1 | |
| print_result(result.__dict__, as_json) | |
| return 0 |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| return {"content": [{"type": "text", "text": f"Unknown run: {arguments['run_id']}"}], "isError": True} | ||
| return {"content": [{"type": "text", "text": json.dumps(data, indent=2)}], "isError": False} | ||
| elif name == "clodex_task_cancel": | ||
| result = TaskManager().cancel(str(arguments["run_id"])) |
There was a problem hiding this comment.
🔥 The Roast: clodex_task_cancel doesn't catch ValueError for unknown run IDs, so it returns isError: false with a 500-level crash instead of a proper JSON-RPC -32004 error. The tasks/cancel method three lines above this gets it right — the tool call got left behind.
🩹 The Fix: Wrap in try/except to match the protocol handler:
| result = TaskManager().cancel(str(arguments["run_id"])) | |
| elif name == "clodex_task_cancel": | |
| try: | |
| result = TaskManager().cancel(str(arguments["run_id"])) | |
| except ValueError as exc: | |
| return {"content": [{"type": "text", "text": str(exc)}], "isError": True} | |
| return {"content": [{"type": "text", "text": json.dumps(result.__dict__, indent=2)}], "isError": False} |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Summary
Testing