From 34358cb0cb983fe098f18b769cf84c0a6d3f8350 Mon Sep 17 00:00:00 2001 From: bgagent Date: Thu, 4 Jun 2026 13:39:08 -0400 Subject: [PATCH 1/5] feat(agent): add in-pipeline pre-PR self-review phase (#262) Adds an optional self-review phase between agent execution and post-hooks where the LLM critiques its own cumulative diff before the PR is created. This improves first-pass PR quality by catching bugs, style issues, and test gaps before human review. - New self_review.py orchestration module with run_self_review() - New prompts/self_review.py with focused review prompt template - TaskConfig extended with self_review_enabled and self_review_max_turns - Fields threaded through build_config, get_config, server, pipeline - Fail-open: self-review errors never block PR creation - Uses remaining turns/budget from original allocation (capped) - Feature is opt-in (disabled by default) --- agent/src/config.py | 7 + agent/src/models.py | 3 + agent/src/pipeline.py | 21 ++ agent/src/prompts/__init__.py | 1 + agent/src/prompts/self_review.py | 40 ++++ agent/src/self_review.py | 198 +++++++++++++++++++ agent/src/server.py | 9 + agent/tests/test_self_review.py | 316 +++++++++++++++++++++++++++++++ 8 files changed, 595 insertions(+) create mode 100644 agent/src/prompts/self_review.py create mode 100644 agent/src/self_review.py create mode 100644 agent/tests/test_self_review.py diff --git a/agent/src/config.py b/agent/src/config.py index bf4ad57c..da18e90b 100644 --- a/agent/src/config.py +++ b/agent/src/config.py @@ -471,6 +471,8 @@ def build_config( initial_approval_gate_count: int = 0, approval_gate_cap: int | None = None, attachments: list[dict] | None = None, + self_review_enabled: bool = False, + self_review_max_turns: int = 5, ) -> TaskConfig: """Build and validate configuration from explicit parameters. @@ -583,6 +585,8 @@ def build_config( initial_approval_gate_count=initial_approval_gate_count, approval_gate_cap=approval_gate_cap, attachments=validated_attachments, + self_review_enabled=self_review_enabled, + self_review_max_turns=self_review_max_turns, ) @@ -607,6 +611,9 @@ def get_config() -> TaskConfig: # an unreachable ``traces//`` key. trace=os.environ.get("TRACE", "").lower() in ("1", "true", "yes"), user_id=os.environ.get("USER_ID", ""), + self_review_enabled=os.environ.get("SELF_REVIEW_ENABLED", "").lower() + in ("1", "true", "yes"), + self_review_max_turns=int(os.environ.get("SELF_REVIEW_MAX_TURNS", "5")), ) except ValueError as e: print(f"ERROR: {e}", file=sys.stderr) diff --git a/agent/src/models.py b/agent/src/models.py index 0befafda..6b4132a9 100644 --- a/agent/src/models.py +++ b/agent/src/models.py @@ -240,6 +240,9 @@ class TaskConfig(BaseModel): # Attachments from the orchestrator payload (Phase 3). Validated as # AttachmentConfig models. Empty list for tasks without attachments. attachments: list[AttachmentConfig] = Field(default_factory=list) + # Self-review: optional LLM diff critique before PR creation. + self_review_enabled: bool = False + self_review_max_turns: int = 5 # Cap on turns allocated to self-review @model_validator(mode="after") def _validate_trace_requires_user_id(self) -> Self: diff --git a/agent/src/pipeline.py b/agent/src/pipeline.py index 7718cb48..c09ea93e 100644 --- a/agent/src/pipeline.py +++ b/agent/src/pipeline.py @@ -36,6 +36,7 @@ ) from progress_writer import _ProgressWriter from prompt_builder import build_system_prompt, discover_project_config +from self_review import run_self_review from shell import log, log_error_cw from system_prompt import SYSTEM_PROMPT from telemetry import ( @@ -589,6 +590,8 @@ def run_task( trace: bool = False, user_id: str = "", attachments: list[dict] | None = None, + self_review_enabled: bool = False, + self_review_max_turns: int = 5, ) -> dict: """Run the full agent pipeline and return a serialized result dict. @@ -628,6 +631,8 @@ def run_task( initial_approval_gate_count=initial_approval_gate_count, approval_gate_cap=approval_gate_cap, attachments=attachments, + self_review_enabled=self_review_enabled, + self_review_max_turns=self_review_max_turns, ) # Inject Cedar policies into config for the PolicyEngine in runner.py @@ -1006,6 +1011,22 @@ def _on_trace_truncated(max_bytes: int, first_dropped: int) -> None: ) ensure_pr_strategy = "create" + # Self-review phase: LLM critiques its own diff before PR creation. + # Runs between cancel-check and post-hooks. Fail-open: errors here + # never block PR creation. + with task_span("task.self_review"): + review_result = run_self_review( + config, setup, agent_result, trajectory, progress + ) + if review_result is not None: + # Accumulate turns and cost from the review phase + agent_result.turns += review_result.turns + agent_result.num_turns += review_result.num_turns or review_result.turns + if review_result.cost_usd is not None: + agent_result.cost_usd = ( + (agent_result.cost_usd or 0.0) + review_result.cost_usd + ) + # Post-hooks (agent_result is guaranteed set by the try/except above) with task_span("task.post_hooks") as post_span: # Safety net: commit any uncommitted tracked changes (skip for read-only tasks) diff --git a/agent/src/prompts/__init__.py b/agent/src/prompts/__init__.py index 60c5b2c0..8334c10b 100644 --- a/agent/src/prompts/__init__.py +++ b/agent/src/prompts/__init__.py @@ -13,6 +13,7 @@ from .new_task import NEW_TASK_WORKFLOW from .pr_iteration import PR_ITERATION_WORKFLOW from .pr_review import PR_REVIEW_WORKFLOW +from .self_review import SELF_REVIEW_PROMPT as SELF_REVIEW_PROMPT from .web_research import WEB_RESEARCH_PROMPT DEFAULT_WORKFLOW_ID = "coding/new-task-v1" diff --git a/agent/src/prompts/self_review.py b/agent/src/prompts/self_review.py new file mode 100644 index 00000000..9a6d109a --- /dev/null +++ b/agent/src/prompts/self_review.py @@ -0,0 +1,40 @@ +"""Self-review prompt template for pre-PR diff critique.""" + +SELF_REVIEW_PROMPT = """\ +You are reviewing your own work before it becomes a pull request. Below is the \ +cumulative diff of all changes on this branch compared to the base branch. + + +{diff} + + +## Task context + +{task_description} + +## Review checklist + +Examine the diff carefully for: + +1. **Correctness** — Logic errors, off-by-one mistakes, missing edge cases, \ +incorrect assumptions about data shapes or API contracts. +2. **Bugs** — Null/undefined dereferences, unhandled error paths, resource leaks, \ +race conditions. +3. **Security** — Injection vulnerabilities (SQL, command, XSS), hardcoded secrets, \ +insecure defaults, OWASP Top 10 issues. +4. **Style & consistency** — Naming conventions, code style violations relative to \ +the surrounding codebase, unnecessary complexity. +5. **Test gaps** — Important behaviour that is untested, assertions that don't \ +verify the right thing, missing edge-case coverage. + +## Instructions + +- If you find issues, fix them directly: edit the files, run the build/tests to \ +verify your fixes, and commit the changes. +- If no issues are found, stop immediately — do not make changes for the sake of \ +making changes. +- Do NOT refactor code that was not part of the original diff unless it has a \ +concrete bug or security issue. +- Keep fixes minimal and focused. Each fix should be a separate commit with a \ +clear message. +""" diff --git a/agent/src/self_review.py b/agent/src/self_review.py new file mode 100644 index 00000000..a73932db --- /dev/null +++ b/agent/src/self_review.py @@ -0,0 +1,198 @@ +"""Self-review orchestration: LLM critiques its own diff before PR creation.""" + +from __future__ import annotations + +import asyncio +import subprocess +from typing import TYPE_CHECKING + +from prompts.self_review import SELF_REVIEW_PROMPT +from shell import log + +if TYPE_CHECKING: + from models import AgentResult, RepoSetup, TaskConfig + from progress_writer import _ProgressWriter + from telemetry import _TrajectoryWriter + +# Diff truncation limit (characters). Large diffs are cut at hunk boundaries. +_MAX_DIFF_CHARS = 60_000 + +# Minimal system prompt for the self-review agent invocation. +_REVIEW_SYSTEM_PROMPT = """\ +You are a code reviewer working inside the repository {repo_url} on branch {branch_name}. +Your working directory is {repo_dir}. + +You have full access to the filesystem and can run commands. Fix any issues you \ +find directly — edit files, run the build, and commit fixes. Keep changes minimal \ +and focused. + +Do NOT open a pull request or push. Just fix issues and commit locally. +""" + + +def _get_diff(repo_dir: str, default_branch: str) -> str: + """Generate the cumulative diff of the branch vs origin/{default_branch}.""" + try: + result = subprocess.run( + ["git", "diff", f"origin/{default_branch}...HEAD"], + cwd=repo_dir, + capture_output=True, + text=True, + timeout=60, + ) + if result.returncode != 0: + log("WARN", f"self_review: git diff failed (exit {result.returncode})") + return "" + return result.stdout + except (subprocess.TimeoutExpired, OSError) as e: + log("WARN", f"self_review: git diff error: {type(e).__name__}: {e}") + return "" + + +def _truncate_diff(diff: str, max_chars: int = _MAX_DIFF_CHARS) -> str: + """Truncate diff at a hunk boundary if it exceeds max_chars. + + Cuts at the last complete hunk (line starting with '@@') that fits + within the limit, appending a truncation notice. + """ + if len(diff) <= max_chars: + return diff + + # Find the last hunk header that starts before max_chars + truncated = diff[:max_chars] + last_hunk = truncated.rfind("\n@@") + if last_hunk > 0: + # Cut just before this hunk header + truncated = truncated[:last_hunk] + else: + # No hunk boundary found — hard-cut at max_chars + last_newline = truncated.rfind("\n") + if last_newline > 0: + truncated = truncated[:last_newline] + + total_lines = diff.count("\n") + kept_lines = truncated.count("\n") + truncated += ( + f"\n\n... [diff truncated: showing ~{kept_lines} of ~{total_lines} lines; " + f"{len(diff) - len(truncated)} chars omitted] ..." + ) + return truncated + + +def _build_review_system_prompt(config: TaskConfig, setup: RepoSetup) -> str: + """Build a minimal system prompt for the self-review agent.""" + return _REVIEW_SYSTEM_PROMPT.format( + repo_url=config.repo_url, + branch_name=setup.branch, + repo_dir=setup.repo_dir, + ) + + +def run_self_review( + config: TaskConfig, + setup: RepoSetup, + agent_result: AgentResult, + trajectory: _TrajectoryWriter, + progress: _ProgressWriter, +) -> AgentResult | None: + """Run the self-review phase: LLM critiques its own diff and fixes issues. + + Returns the AgentResult from the review phase, or None if skipped. + Fail-open: errors are logged but never block the pipeline. + """ + # Skip condition: feature disabled + if not config.self_review_enabled: + log("TASK", "self_review: disabled (self_review_enabled=False)") + return None + + # Skip condition: read-only workflows produce no diff to review + if config.read_only: + log("TASK", "self_review: skipped for read-only workflow") + return None + + # Compute remaining turns + used_turns = agent_result.turns or 0 + remaining_turns = config.max_turns - used_turns + review_turns = min(remaining_turns, config.self_review_max_turns) + if review_turns <= 0: + log("TASK", f"self_review: no remaining turns (used={used_turns}, max={config.max_turns})") + return None + + # Compute remaining budget + review_budget: float | None = None + if config.max_budget_usd is not None: + used_cost = agent_result.cost_usd or 0.0 + remaining_budget = config.max_budget_usd - used_cost + if remaining_budget <= 0: + log( + "TASK", + f"self_review: no remaining budget " + f"(used=${used_cost:.2f}, max=${config.max_budget_usd:.2f})", + ) + return None + review_budget = remaining_budget + + # Get the diff + diff = _get_diff(setup.repo_dir, setup.default_branch) + if not diff.strip(): + log("TASK", "self_review: no diff found — skipping") + return None + + # Truncate if needed + diff = _truncate_diff(diff) + + # Build the review prompt + task_desc = config.task_description or f"Issue #{config.issue_number}" + user_prompt = SELF_REVIEW_PROMPT.format(diff=diff, task_description=task_desc) + system_prompt = _build_review_system_prompt(config, setup) + + # Build a modified config for the review run + review_config = config.model_copy( + update={ + "max_turns": review_turns, + "max_budget_usd": review_budget, + } + ) + + log( + "TASK", + f"self_review: starting (turns={review_turns}, " + f"budget={'$' + f'{review_budget:.2f}' if review_budget else 'unlimited'}, " + f"diff_chars={len(diff)})", + ) + progress.write_agent_milestone( + "self_review_started", + f"turns={review_turns} diff_chars={len(diff)}", + ) + + try: + from runner import run_agent + + review_result = asyncio.run( + run_agent( + user_prompt, + system_prompt, + review_config, + cwd=setup.repo_dir, + trajectory=trajectory, + ) + ) + except Exception as e: + # Fail-open: self-review errors never block the pipeline + log("WARN", f"self_review: agent execution failed: {type(e).__name__}: {e}") + progress.write_agent_milestone( + "self_review_complete", + f"status=error error={type(e).__name__}: {e}", + ) + return None + + log( + "TASK", + f"self_review: complete (status={review_result.status}, " + f"turns={review_result.turns}, cost=${review_result.cost_usd or 0:.4f})", + ) + progress.write_agent_milestone( + "self_review_complete", + f"status={review_result.status} turns={review_result.turns}", + ) + return review_result diff --git a/agent/src/server.py b/agent/src/server.py index e1114374..ea615ce9 100644 --- a/agent/src/server.py +++ b/agent/src/server.py @@ -405,6 +405,8 @@ def _run_task_background( user_id: str = "", workload_access_token: str = "", attachments: list[dict] | None = None, + self_review_enabled: bool = False, + self_review_max_turns: int = 5, ) -> None: """Run the agent task in a background thread.""" global _background_pipeline_failed @@ -488,6 +490,8 @@ def _run_task_background( trace=trace, user_id=user_id, attachments=attachments, + self_review_enabled=self_review_enabled, + self_review_max_turns=self_review_max_turns, ) _background_pipeline_failed = False except Exception as e: @@ -576,6 +580,9 @@ def _extract_invocation_params(inp: dict, request: Request) -> dict: channel_source = inp.get("channel_source", "") or "" channel_metadata = inp.get("channel_metadata") or {} attachments = inp.get("attachments") or [] + # Self-review (opt-in): LLM critiques its own diff before PR creation. + self_review_enabled = inp.get("self_review_enabled") is True + self_review_max_turns = int(inp.get("self_review_max_turns", 5)) # ``trace`` is strictly opt-in (design §10.1). Accept only real # booleans from the orchestrator — a string "false" would otherwise # flip the flag on. @@ -649,6 +656,8 @@ def _extract_invocation_params(inp: dict, request: Request) -> dict: "user_id": user_id, "workload_access_token": workload_access_token, "attachments": attachments, + "self_review_enabled": self_review_enabled, + "self_review_max_turns": self_review_max_turns, } diff --git a/agent/tests/test_self_review.py b/agent/tests/test_self_review.py new file mode 100644 index 00000000..bd39741a --- /dev/null +++ b/agent/tests/test_self_review.py @@ -0,0 +1,316 @@ +"""Unit tests for self_review.py — self-review orchestration module.""" + +from unittest.mock import MagicMock, patch + +from models import AgentResult, RepoSetup, TaskConfig +from self_review import _MAX_DIFF_CHARS, _get_diff, _truncate_diff, run_self_review + + +def _make_config(**overrides) -> TaskConfig: + """Create a minimal TaskConfig for testing.""" + defaults = { + "repo_url": "owner/repo", + "github_token": "ghp_test123", + "aws_region": "us-east-1", + "task_description": "Fix the bug", + "max_turns": 10, + "self_review_enabled": True, + "self_review_max_turns": 5, + "task_type": "new_task", + } + defaults.update(overrides) + return TaskConfig(**defaults) + + +def _make_setup(**overrides) -> RepoSetup: + """Create a minimal RepoSetup for testing.""" + defaults = { + "repo_dir": "/workspace/repo", + "branch": "feat/123-fix", + "default_branch": "main", + } + defaults.update(overrides) + return RepoSetup(**defaults) + + +def _make_agent_result(**overrides) -> AgentResult: + """Create a minimal AgentResult for testing.""" + defaults = { + "status": "success", + "turns": 5, + "num_turns": 5, + "cost_usd": 0.50, + } + defaults.update(overrides) + return AgentResult(**defaults) + + +class TestSkipConditions: + """Test all conditions that cause self-review to be skipped.""" + + def test_skip_when_disabled(self): + config = _make_config(self_review_enabled=False) + setup = _make_setup() + agent_result = _make_agent_result() + trajectory = MagicMock() + progress = MagicMock() + + result = run_self_review(config, setup, agent_result, trajectory, progress) + assert result is None + + def test_skip_for_pr_review_task_type(self): + config = _make_config( + task_type="pr_review", + pr_number="42", + task_description="", + issue_number="", + ) + setup = _make_setup() + agent_result = _make_agent_result() + trajectory = MagicMock() + progress = MagicMock() + + result = run_self_review(config, setup, agent_result, trajectory, progress) + assert result is None + + def test_skip_when_no_remaining_turns(self): + config = _make_config(max_turns=5, self_review_max_turns=5) + setup = _make_setup() + agent_result = _make_agent_result(turns=5) + trajectory = MagicMock() + progress = MagicMock() + + result = run_self_review(config, setup, agent_result, trajectory, progress) + assert result is None + + def test_skip_when_no_remaining_budget(self): + config = _make_config(max_budget_usd=1.0) + setup = _make_setup() + agent_result = _make_agent_result(cost_usd=1.0) + trajectory = MagicMock() + progress = MagicMock() + + result = run_self_review(config, setup, agent_result, trajectory, progress) + assert result is None + + @patch("self_review._get_diff", return_value="") + def test_skip_when_empty_diff(self, mock_diff): + config = _make_config() + setup = _make_setup() + agent_result = _make_agent_result() + trajectory = MagicMock() + progress = MagicMock() + + result = run_self_review(config, setup, agent_result, trajectory, progress) + assert result is None + + @patch("self_review._get_diff", return_value=" \n \n ") + def test_skip_when_whitespace_only_diff(self, mock_diff): + config = _make_config() + setup = _make_setup() + agent_result = _make_agent_result() + trajectory = MagicMock() + progress = MagicMock() + + result = run_self_review(config, setup, agent_result, trajectory, progress) + assert result is None + + +class TestGetDiff: + """Test _get_diff helper.""" + + @patch("self_review.subprocess.run") + def test_returns_diff_output(self, mock_run): + mock_run.return_value = MagicMock(returncode=0, stdout="diff --git a/f\n+line\n") + result = _get_diff("/repo", "main") + assert result == "diff --git a/f\n+line\n" + mock_run.assert_called_once_with( + ["git", "diff", "origin/main...HEAD"], + cwd="/repo", + capture_output=True, + text=True, + timeout=60, + ) + + @patch("self_review.subprocess.run") + def test_returns_empty_on_failure(self, mock_run): + mock_run.return_value = MagicMock(returncode=1, stdout="") + result = _get_diff("/repo", "main") + assert result == "" + + @patch("self_review.subprocess.run") + def test_returns_empty_on_timeout(self, mock_run): + import subprocess + + mock_run.side_effect = subprocess.TimeoutExpired(cmd="git", timeout=60) + result = _get_diff("/repo", "main") + assert result == "" + + @patch("self_review.subprocess.run") + def test_uses_custom_default_branch(self, mock_run): + mock_run.return_value = MagicMock(returncode=0, stdout="some diff") + _get_diff("/repo", "develop") + mock_run.assert_called_once_with( + ["git", "diff", "origin/develop...HEAD"], + cwd="/repo", + capture_output=True, + text=True, + timeout=60, + ) + + +class TestTruncateDiff: + """Test _truncate_diff helper.""" + + def test_no_truncation_needed(self): + short_diff = "diff --git a/file.py\n@@ -1,3 +1,4 @@\n+new line\n" + result = _truncate_diff(short_diff) + assert result == short_diff + + def test_truncates_at_hunk_boundary(self): + # Build a diff that exceeds the limit with multiple hunks + hunk1 = "diff --git a/file.py\n@@ -1,3 +1,4 @@\n" + "+line\n" * 100 + hunk2 = "\n@@ -100,3 +101,4 @@\n" + "+more\n" * 100 + big_diff = hunk1 + hunk2 + result = _truncate_diff(big_diff, max_chars=len(hunk1) + 10) + # Should truncate before hunk2's @@ marker + assert "@@ -100,3" not in result + assert "truncated" in result + + def test_hard_cut_when_no_hunk_boundary(self): + # Single hunk that exceeds max + big_diff = "+x\n" * 30000 + result = _truncate_diff(big_diff, max_chars=100) + assert len(result) < len(big_diff) + assert "truncated" in result + + def test_exact_limit_no_truncation(self): + diff = "a" * _MAX_DIFF_CHARS + result = _truncate_diff(diff) + assert result == diff + + def test_one_over_limit_truncates(self): + diff = "a" * (_MAX_DIFF_CHARS + 1) + result = _truncate_diff(diff) + assert "truncated" in result + + +class TestBudgetAndTurnComputation: + """Test that budget and turn limits are computed correctly.""" + + @patch("self_review._get_diff", return_value="diff --git a/f\n+line\n") + @patch("self_review.asyncio.run") + def test_review_turns_capped_at_self_review_max_turns(self, mock_asyncio_run, mock_diff): + mock_asyncio_run.return_value = AgentResult(status="success", turns=2, num_turns=2) + config = _make_config(max_turns=100, self_review_max_turns=3) + setup = _make_setup() + agent_result = _make_agent_result(turns=5) + trajectory = MagicMock() + progress = MagicMock() + + run_self_review(config, setup, agent_result, trajectory, progress) + + # The review config passed to run_agent should have max_turns=3 + call_args = mock_asyncio_run.call_args + coro = call_args[0][0] + # Close the coroutine to avoid warnings + coro.close() + + @patch("self_review._get_diff", return_value="diff --git a/f\n+line\n") + @patch("self_review.asyncio.run") + def test_review_turns_uses_remaining_when_less_than_cap(self, mock_asyncio_run, mock_diff): + mock_asyncio_run.return_value = AgentResult(status="success", turns=1, num_turns=1) + config = _make_config(max_turns=8, self_review_max_turns=5) + setup = _make_setup() + agent_result = _make_agent_result(turns=6) # Only 2 remaining + trajectory = MagicMock() + progress = MagicMock() + + run_self_review(config, setup, agent_result, trajectory, progress) + + # Should use min(2, 5) = 2 turns + call_args = mock_asyncio_run.call_args + coro = call_args[0][0] + coro.close() + + +class TestHappyPath: + """Test the full happy path with mocked run_agent.""" + + @patch("self_review._get_diff", return_value="diff --git a/file.py\n+new code\n") + @patch("self_review.asyncio.run") + def test_returns_review_result(self, mock_asyncio_run, mock_diff): + review_agent_result = AgentResult( + status="success", turns=2, num_turns=2, cost_usd=0.10 + ) + mock_asyncio_run.return_value = review_agent_result + + config = _make_config() + setup = _make_setup() + agent_result = _make_agent_result(turns=5) + trajectory = MagicMock() + progress = MagicMock() + + result = run_self_review(config, setup, agent_result, trajectory, progress) + + assert result is not None + assert result.status == "success" + assert result.turns == 2 + assert result.cost_usd == 0.10 + + @patch("self_review._get_diff", return_value="diff --git a/file.py\n+new code\n") + @patch("self_review.asyncio.run") + def test_writes_progress_milestones(self, mock_asyncio_run, mock_diff): + mock_asyncio_run.return_value = AgentResult( + status="success", turns=1, num_turns=1, cost_usd=0.05 + ) + config = _make_config() + setup = _make_setup() + agent_result = _make_agent_result() + trajectory = MagicMock() + progress = MagicMock() + + run_self_review(config, setup, agent_result, trajectory, progress) + + # Should write both started and complete milestones + milestone_calls = progress.write_agent_milestone.call_args_list + assert len(milestone_calls) == 2 + assert milestone_calls[0][0][0] == "self_review_started" + assert milestone_calls[1][0][0] == "self_review_complete" + + @patch("self_review._get_diff", return_value="diff --git a/file.py\n+new code\n") + @patch("self_review.asyncio.run") + def test_fail_open_on_agent_error(self, mock_asyncio_run, mock_diff): + mock_asyncio_run.side_effect = RuntimeError("SDK crashed") + + config = _make_config() + setup = _make_setup() + agent_result = _make_agent_result() + trajectory = MagicMock() + progress = MagicMock() + + # Should not raise + result = run_self_review(config, setup, agent_result, trajectory, progress) + assert result is None + + # Should still write milestones + milestone_calls = progress.write_agent_milestone.call_args_list + assert milestone_calls[0][0][0] == "self_review_started" + assert milestone_calls[1][0][0] == "self_review_complete" + assert "error" in milestone_calls[1][0][1] + + @patch("self_review._get_diff", return_value="diff --git a/file.py\n+new code\n") + @patch("self_review.asyncio.run") + def test_works_with_unlimited_budget(self, mock_asyncio_run, mock_diff): + mock_asyncio_run.return_value = AgentResult( + status="success", turns=1, num_turns=1, cost_usd=0.03 + ) + config = _make_config(max_budget_usd=None) + setup = _make_setup() + agent_result = _make_agent_result(cost_usd=None) + trajectory = MagicMock() + progress = MagicMock() + + result = run_self_review(config, setup, agent_result, trajectory, progress) + assert result is not None + assert result.status == "success" From bf720e8f43a95365abaac730c9e916e7f8bb017b Mon Sep 17 00:00:00 2001 From: bgagent Date: Thu, 4 Jun 2026 15:29:18 -0400 Subject: [PATCH 2/5] feat(agent): post self-review summary as PR comment (#263) After the self-review phase completes, the agent writes a structured summary of findings to `.self-review-summary.md`. The pipeline reads this file, posts it as a PR comment via `gh pr comment`, then deletes it so it never appears in the PR diff. Fail-open: if the file is missing or the comment fails to post, the pipeline continues normally. --- agent/src/pipeline.py | 5 + agent/src/post_hooks.py | 62 +++++++++++ agent/src/prompts/self_review.py | 21 ++++ agent/src/self_review.py | 41 ++++++++ agent/tests/test_self_review.py | 170 ++++++++++++++++++++++++++++++- 5 files changed, 298 insertions(+), 1 deletion(-) diff --git a/agent/src/pipeline.py b/agent/src/pipeline.py index c09ea93e..f486aa83 100644 --- a/agent/src/pipeline.py +++ b/agent/src/pipeline.py @@ -31,6 +31,7 @@ _extract_agent_notes, ensure_committed, ensure_pr, + post_self_review_comment, verify_build, verify_lint, ) @@ -1049,6 +1050,10 @@ def _on_trace_truncated(max_bytes: int, first_dropped: int) -> None: if pr_url: progress.write_agent_milestone("pr_created", pr_url) + # Post self-review summary as PR comment (if self-review ran and produced findings) + if pr_url and review_result is not None: + post_self_review_comment(setup.repo_dir, pr_url, config) + # Memory write — capture task episode and repo learnings memory_written = False effective_memory_id = memory_id or os.environ.get("MEMORY_ID", "") diff --git a/agent/src/post_hooks.py b/agent/src/post_hooks.py index 058a1e4c..dc840657 100644 --- a/agent/src/post_hooks.py +++ b/agent/src/post_hooks.py @@ -392,6 +392,68 @@ def ensure_pr( return None +def post_self_review_comment(repo_dir: str, pr_url: str, config: TaskConfig) -> bool: + """Post the self-review summary as a PR comment. + + Reads the summary file written by the self-review agent, formats it as a + comment, and posts it via `gh pr comment`. Fail-open: exceptions are logged + but never propagated. + + Returns True if a comment was posted, False otherwise. + """ + from self_review import read_self_review_summary + + try: + summary = read_self_review_summary(repo_dir) + except Exception as e: + log("WARN", f"post_self_review_comment: failed to read summary: {type(e).__name__}: {e}") + return False + + if not summary: + log("POST", "post_self_review_comment: no summary file found — skipping") + return False + + # Extract PR number from URL (e.g. https://github.com/owner/repo/pull/123) + match = re.search(r"/pull/(\d+)", pr_url) + if not match: + log("WARN", f"post_self_review_comment: could not extract PR number from {pr_url}") + return False + pr_number = match.group(1) + + comment_body = f"## \U0001f50d Self-Review Summary\n\n{summary}" + + try: + result = subprocess.run( + [ + "gh", + "pr", + "comment", + pr_number, + "--repo", + config.repo_url, + "--body", + comment_body, + ], + cwd=repo_dir, + capture_output=True, + text=True, + timeout=60, + ) + if result.returncode == 0: + log("POST", f"Self-review summary posted as comment on PR #{pr_number}") + return True + stderr = result.stderr.strip()[:200] if result.stderr else "" + log( + "WARN", + f"post_self_review_comment: gh pr comment failed " + f"(rc={result.returncode}): {stderr}", + ) + return False + except (subprocess.TimeoutExpired, OSError) as e: + log("WARN", f"post_self_review_comment: {type(e).__name__}: {e}") + return False + + def _extract_agent_notes(repo_dir: str, branch: str, config: TaskConfig) -> str | None: """Extract the "## Agent notes" section from the PR body. diff --git a/agent/src/prompts/self_review.py b/agent/src/prompts/self_review.py index 9a6d109a..25cf47a2 100644 --- a/agent/src/prompts/self_review.py +++ b/agent/src/prompts/self_review.py @@ -37,4 +37,25 @@ concrete bug or security issue. - Keep fixes minimal and focused. Each fix should be a separate commit with a \ clear message. + +## Summary output + +After completing your review (whether you made fixes or not), write a file \ +`.self-review-summary.md` in the repository root with your findings in this format: + +```markdown +### Self-Review Summary + +**Findings:** +**Fixes applied:** + +#### Issues found + +- : — +``` + +If no issues were found, write the file with: "No issues found — code looks good." + +This file is a pipeline artifact and will be deleted automatically — it will NOT \ +appear in the pull request. """ diff --git a/agent/src/self_review.py b/agent/src/self_review.py index a73932db..69114d68 100644 --- a/agent/src/self_review.py +++ b/agent/src/self_review.py @@ -3,6 +3,8 @@ from __future__ import annotations import asyncio +import contextlib +import os import subprocess from typing import TYPE_CHECKING @@ -196,3 +198,42 @@ def run_self_review( f"status={review_result.status} turns={review_result.turns}", ) return review_result + + +_SUMMARY_FILENAME = ".self-review-summary.md" + + +def read_self_review_summary(repo_dir: str) -> str | None: + """Read and delete the self-review summary file. + + The self-review agent writes `.self-review-summary.md` in the repo root. + This function reads the content, removes the file (so it never appears in + the PR), and returns the content. Returns None if the file doesn't exist. + """ + summary_path = os.path.join(repo_dir, _SUMMARY_FILENAME) + if not os.path.isfile(summary_path): + return None + + try: + with open(summary_path) as f: + content = f.read() + except OSError as e: + log("WARN", f"self_review: failed to read summary file: {type(e).__name__}: {e}") + return None + + # Remove the file so it doesn't end up in the PR + try: + os.remove(summary_path) + except OSError as e: + log("WARN", f"self_review: failed to delete summary file: {type(e).__name__}: {e}") + + # If the file was staged by the agent, unstage it + with contextlib.suppress(subprocess.TimeoutExpired, OSError): + subprocess.run( + ["git", "rm", "--cached", "--ignore-unmatch", "-f", _SUMMARY_FILENAME], + cwd=repo_dir, + capture_output=True, + timeout=30, + ) + + return content.strip() if content.strip() else None diff --git a/agent/tests/test_self_review.py b/agent/tests/test_self_review.py index bd39741a..e2a817f1 100644 --- a/agent/tests/test_self_review.py +++ b/agent/tests/test_self_review.py @@ -1,9 +1,17 @@ """Unit tests for self_review.py — self-review orchestration module.""" +import os from unittest.mock import MagicMock, patch from models import AgentResult, RepoSetup, TaskConfig -from self_review import _MAX_DIFF_CHARS, _get_diff, _truncate_diff, run_self_review +from self_review import ( + _MAX_DIFF_CHARS, + _SUMMARY_FILENAME, + _get_diff, + _truncate_diff, + read_self_review_summary, + run_self_review, +) def _make_config(**overrides) -> TaskConfig: @@ -314,3 +322,163 @@ def test_works_with_unlimited_budget(self, mock_asyncio_run, mock_diff): result = run_self_review(config, setup, agent_result, trajectory, progress) assert result is not None assert result.status == "success" + + +class TestReadSelfReviewSummary: + """Tests for read_self_review_summary — reads and cleans up the summary file.""" + + def test_returns_content_when_file_exists(self, tmp_path): + summary_content = ( + "### Self-Review Summary\n\n" + "**Findings:** 2\n" + "**Fixes applied:** 1\n\n" + "#### Issues found\n\n" + "- Security: hardcoded token — fixed\n" + "- Style: inconsistent naming — not fixed (cosmetic)\n" + ) + (tmp_path / _SUMMARY_FILENAME).write_text(summary_content) + + result = read_self_review_summary(str(tmp_path)) + + assert result == summary_content.strip() + + def test_returns_none_when_file_missing(self, tmp_path): + result = read_self_review_summary(str(tmp_path)) + assert result is None + + def test_deletes_file_after_reading(self, tmp_path): + (tmp_path / _SUMMARY_FILENAME).write_text("No issues found — code looks good.") + + read_self_review_summary(str(tmp_path)) + + assert not (tmp_path / _SUMMARY_FILENAME).exists() + + def test_returns_none_for_empty_file(self, tmp_path): + (tmp_path / _SUMMARY_FILENAME).write_text(" \n\n ") + + result = read_self_review_summary(str(tmp_path)) + + assert result is None + + def test_returns_none_for_whitespace_only(self, tmp_path): + (tmp_path / _SUMMARY_FILENAME).write_text("\t\n ") + + result = read_self_review_summary(str(tmp_path)) + + assert result is None + + @patch("self_review.subprocess.run") + def test_runs_git_rm_cached_for_cleanup(self, mock_run, tmp_path): + (tmp_path / _SUMMARY_FILENAME).write_text("Some findings") + mock_run.return_value = MagicMock(returncode=0) + + read_self_review_summary(str(tmp_path)) + + mock_run.assert_called_once_with( + ["git", "rm", "--cached", "--ignore-unmatch", "-f", _SUMMARY_FILENAME], + cwd=str(tmp_path), + capture_output=True, + timeout=30, + ) + + @patch("self_review.subprocess.run", side_effect=OSError("git not found")) + def test_git_rm_failure_does_not_block(self, mock_run, tmp_path): + (tmp_path / _SUMMARY_FILENAME).write_text("Findings here") + + # Should not raise even if git rm fails + result = read_self_review_summary(str(tmp_path)) + + assert result == "Findings here" + + +class TestPostSelfReviewComment: + """Tests for post_hooks.post_self_review_comment.""" + + def test_posts_comment_on_success(self, tmp_path): + from post_hooks import post_self_review_comment + + (tmp_path / _SUMMARY_FILENAME).write_text("**Findings:** 1\n**Fixes applied:** 1") + + config = MagicMock() + config.repo_url = "owner/repo" + pr_url = "https://github.com/owner/repo/pull/42" + + with patch("post_hooks.subprocess.run") as mock_run: + mock_run.return_value = MagicMock(returncode=0, stdout="", stderr="") + result = post_self_review_comment(str(tmp_path), pr_url, config) + + assert result is True + call_args = mock_run.call_args[0][0] + assert call_args[0:3] == ["gh", "pr", "comment"] + assert "42" in call_args + assert "owner/repo" in call_args + + def test_returns_false_when_no_summary(self, tmp_path): + from post_hooks import post_self_review_comment + + config = MagicMock() + config.repo_url = "owner/repo" + pr_url = "https://github.com/owner/repo/pull/42" + + result = post_self_review_comment(str(tmp_path), pr_url, config) + assert result is False + + def test_returns_false_on_invalid_pr_url(self, tmp_path): + from post_hooks import post_self_review_comment + + (tmp_path / _SUMMARY_FILENAME).write_text("Some findings") + + config = MagicMock() + config.repo_url = "owner/repo" + pr_url = "https://github.com/owner/repo/issues/42" + + result = post_self_review_comment(str(tmp_path), pr_url, config) + assert result is False + + def test_returns_false_on_gh_failure(self, tmp_path): + from post_hooks import post_self_review_comment + + (tmp_path / _SUMMARY_FILENAME).write_text("**Findings:** 1") + + config = MagicMock() + config.repo_url = "owner/repo" + pr_url = "https://github.com/owner/repo/pull/99" + + with patch("post_hooks.subprocess.run") as mock_run: + mock_run.return_value = MagicMock(returncode=1, stdout="", stderr="not found") + result = post_self_review_comment(str(tmp_path), pr_url, config) + + assert result is False + + def test_fail_open_on_exception(self, tmp_path): + from post_hooks import post_self_review_comment + + (tmp_path / _SUMMARY_FILENAME).write_text("**Findings:** 1") + + config = MagicMock() + config.repo_url = "owner/repo" + pr_url = "https://github.com/owner/repo/pull/5" + + with patch("post_hooks.subprocess.run", side_effect=OSError("network error")): + result = post_self_review_comment(str(tmp_path), pr_url, config) + + assert result is False + + def test_comment_body_includes_header(self, tmp_path): + from post_hooks import post_self_review_comment + + (tmp_path / _SUMMARY_FILENAME).write_text("**Findings:** 0\nNo issues.") + + config = MagicMock() + config.repo_url = "owner/repo" + pr_url = "https://github.com/owner/repo/pull/7" + + with patch("post_hooks.subprocess.run") as mock_run: + mock_run.return_value = MagicMock(returncode=0, stdout="", stderr="") + post_self_review_comment(str(tmp_path), pr_url, config) + + call_args = mock_run.call_args[0][0] + body_idx = call_args.index("--body") + 1 + body = call_args[body_idx] + assert "Self-Review Summary" in body + assert "**Findings:** 0" in body From 32112308a0a34f990923e02d845edda4b1ac108d Mon Sep 17 00:00:00 2001 From: bgagent Date: Mon, 22 Jun 2026 16:08:03 -0400 Subject: [PATCH 3/5] refactor(agent): expose self-review as a configurable workflow step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review feedback: rather than a hardcoded inline pipeline phase gated by a per-task API flag, self-review is now a first-class workflow StepKind, configurable as a step in the coding workflow YAML. - Add `self_review` StepKind to workflow.schema.json (with a `max_turns` step field, 1-20, default 5) + models.Step; excluded from repo-less workflows (schema conditional + validator rule 3) since it diffs/re-runs the agent against the cloned tree. - Register `_handle_self_review` in the runner's STEP_HANDLERS; it wraps run_self_review and accumulates the review loop's turns/cost onto the shared agent_result. Non-side-effecting, so it may use on_failure: continue (advisory / fail-open). - Drive the step from pipeline.run_task via `_execute_self_review_step` (only_kinds={"self_review"}), mirroring _execute_agent_step. Runs after the cancel short-circuit and before post-hooks; posts the summary PR comment after ensure_pr when the step ran. - Declare the step in coding/new-task-v1.yaml (opt-in by presence; on_failure: continue). Remove it to disable; tune via max_turns. - Drop the now-redundant self_review_enabled / self_review_max_turns fields from TaskConfig, build_config, get_config, server params, and run_task — configurability lives in the workflow YAML. - Add golden validator-corpus fixtures (valid self_review step; repo-less + self_review rejected) to lock parity. Agent quality green: ruff, ty (0 errors), 1129 tests, 79.87% coverage. --- agent/src/config.py | 7 - agent/src/models.py | 3 - agent/src/pipeline.py | 84 +++++-- agent/src/post_hooks.py | 3 +- agent/src/self_review.py | 36 ++- agent/src/server.py | 9 - agent/src/workflow/models.py | 4 + agent/src/workflow/runner.py | 71 ++++++ agent/src/workflow/validator.py | 6 +- agent/tests/test_self_review.py | 208 +++++++++++++----- agent/workflows/coding/new-task-v1.yaml | 6 + agent/workflows/schema/workflow.schema.json | 10 +- .../rule3-repo-less-has-self-review.json | 69 ++++++ .../valid-coding-self-review-step.json | 94 ++++++++ 14 files changed, 496 insertions(+), 114 deletions(-) create mode 100644 contracts/workflow-validation/rule3-repo-less-has-self-review.json create mode 100644 contracts/workflow-validation/valid-coding-self-review-step.json diff --git a/agent/src/config.py b/agent/src/config.py index da18e90b..bf4ad57c 100644 --- a/agent/src/config.py +++ b/agent/src/config.py @@ -471,8 +471,6 @@ def build_config( initial_approval_gate_count: int = 0, approval_gate_cap: int | None = None, attachments: list[dict] | None = None, - self_review_enabled: bool = False, - self_review_max_turns: int = 5, ) -> TaskConfig: """Build and validate configuration from explicit parameters. @@ -585,8 +583,6 @@ def build_config( initial_approval_gate_count=initial_approval_gate_count, approval_gate_cap=approval_gate_cap, attachments=validated_attachments, - self_review_enabled=self_review_enabled, - self_review_max_turns=self_review_max_turns, ) @@ -611,9 +607,6 @@ def get_config() -> TaskConfig: # an unreachable ``traces//`` key. trace=os.environ.get("TRACE", "").lower() in ("1", "true", "yes"), user_id=os.environ.get("USER_ID", ""), - self_review_enabled=os.environ.get("SELF_REVIEW_ENABLED", "").lower() - in ("1", "true", "yes"), - self_review_max_turns=int(os.environ.get("SELF_REVIEW_MAX_TURNS", "5")), ) except ValueError as e: print(f"ERROR: {e}", file=sys.stderr) diff --git a/agent/src/models.py b/agent/src/models.py index 6b4132a9..0befafda 100644 --- a/agent/src/models.py +++ b/agent/src/models.py @@ -240,9 +240,6 @@ class TaskConfig(BaseModel): # Attachments from the orchestrator payload (Phase 3). Validated as # AttachmentConfig models. Empty list for tasks without attachments. attachments: list[AttachmentConfig] = Field(default_factory=list) - # Self-review: optional LLM diff critique before PR creation. - self_review_enabled: bool = False - self_review_max_turns: int = 5 # Cap on turns allocated to self-review @model_validator(mode="after") def _validate_trace_requires_user_id(self) -> Self: diff --git a/agent/src/pipeline.py b/agent/src/pipeline.py index f486aa83..f4c29206 100644 --- a/agent/src/pipeline.py +++ b/agent/src/pipeline.py @@ -37,7 +37,6 @@ ) from progress_writer import _ProgressWriter from prompt_builder import build_system_prompt, discover_project_config -from self_review import run_self_review from shell import log, log_error_cw from system_prompt import SYSTEM_PROMPT from telemetry import ( @@ -219,6 +218,50 @@ def _execute_agent_step( return ctx.agent_result +def _execute_self_review_step( + workflow: Workflow | None, + config, + setup, + agent_result, + hydrated, + trajectory, + progress, +) -> bool: + """Drive the workflow's ``self_review`` step (if declared) through the runner. + + Mirrors ``_execute_agent_step``: only the ``self_review`` step is dispatched + (``only_kinds={"self_review"}``) so clone / build / PR stay on the inline + path. The step's handler accumulates the review loop's turns/cost back onto + ``agent_result`` (a shared mutable model), so the terminal result reflects + implement + review. + + Returns True when the review actually ran (so the caller posts the summary + PR comment after ``ensure_pr``); False when no ``self_review`` step is + declared, the workflow failed to reload, or the review was skipped (read-only + / empty diff / no remaining turns). Fully fail-open — a review failure is + recorded as a step outcome and never propagates to block PR creation. + """ + if workflow is None or not any(s.kind == "self_review" for s in workflow.steps): + return False + + from workflow import StepContext, run_workflow + + ctx = StepContext( + workflow=workflow, + config=config, + hydrated=hydrated, + progress=progress, + trajectory=trajectory, + setup=setup, + # The implement step's result, threaded in so the handler can size the + # review's turn budget and accumulate its turns/cost onto it. + agent_result=agent_result, + ) + with task_span("task.self_review"): + run_workflow(workflow, ctx, only_kinds={"self_review"}) + return bool(ctx.artifacts.get("self_review_ran", False)) + + def _run_repoless_task( *, config, @@ -591,8 +634,6 @@ def run_task( trace: bool = False, user_id: str = "", attachments: list[dict] | None = None, - self_review_enabled: bool = False, - self_review_max_turns: int = 5, ) -> dict: """Run the full agent pipeline and return a serialized result dict. @@ -632,8 +673,6 @@ def run_task( initial_approval_gate_count=initial_approval_gate_count, approval_gate_cap=approval_gate_cap, attachments=attachments, - self_review_enabled=self_review_enabled, - self_review_max_turns=self_review_max_turns, ) # Inject Cedar policies into config for the PolicyEngine in runner.py @@ -1012,21 +1051,22 @@ def _on_trace_truncated(max_bytes: int, first_dropped: int) -> None: ) ensure_pr_strategy = "create" - # Self-review phase: LLM critiques its own diff before PR creation. - # Runs between cancel-check and post-hooks. Fail-open: errors here - # never block PR creation. - with task_span("task.self_review"): - review_result = run_self_review( - config, setup, agent_result, trajectory, progress - ) - if review_result is not None: - # Accumulate turns and cost from the review phase - agent_result.turns += review_result.turns - agent_result.num_turns += review_result.num_turns or review_result.turns - if review_result.cost_usd is not None: - agent_result.cost_usd = ( - (agent_result.cost_usd or 0.0) + review_result.cost_usd - ) + # Self-review step: if the resolved workflow declares a ``self_review`` + # step, drive it through the workflow runner (same pattern as + # ``_execute_agent_step``). The step has the LLM critique its own diff + # and fix issues, accumulating its turns/cost onto ``agent_result``. + # Runs AFTER the cancel short-circuit so a cancelled task never starts + # a second agent loop, and BEFORE post-hooks so fixes land in the PR. + # Fail-open: a review failure/skip never blocks PR creation. + self_review_ran = _execute_self_review_step( + _workflow, + config, + setup, + agent_result, + hc, + trajectory, + progress, + ) # Post-hooks (agent_result is guaranteed set by the try/except above) with task_span("task.post_hooks") as post_span: @@ -1050,8 +1090,8 @@ def _on_trace_truncated(max_bytes: int, first_dropped: int) -> None: if pr_url: progress.write_agent_milestone("pr_created", pr_url) - # Post self-review summary as PR comment (if self-review ran and produced findings) - if pr_url and review_result is not None: + # Post self-review summary as PR comment (if the self_review step ran) + if pr_url and self_review_ran: post_self_review_comment(setup.repo_dir, pr_url, config) # Memory write — capture task episode and repo learnings diff --git a/agent/src/post_hooks.py b/agent/src/post_hooks.py index dc840657..00268847 100644 --- a/agent/src/post_hooks.py +++ b/agent/src/post_hooks.py @@ -445,8 +445,7 @@ def post_self_review_comment(repo_dir: str, pr_url: str, config: TaskConfig) -> stderr = result.stderr.strip()[:200] if result.stderr else "" log( "WARN", - f"post_self_review_comment: gh pr comment failed " - f"(rc={result.returncode}): {stderr}", + f"post_self_review_comment: gh pr comment failed (rc={result.returncode}): {stderr}", ) return False except (subprocess.TimeoutExpired, OSError) as e: diff --git a/agent/src/self_review.py b/agent/src/self_review.py index 69114d68..0c7634cb 100644 --- a/agent/src/self_review.py +++ b/agent/src/self_review.py @@ -19,6 +19,9 @@ # Diff truncation limit (characters). Large diffs are cut at hunk boundaries. _MAX_DIFF_CHARS = 60_000 +# Default cap on review-loop turns when the ``self_review`` step omits max_turns. +_DEFAULT_REVIEW_MAX_TURNS = 5 + # Minimal system prompt for the self-review agent invocation. _REVIEW_SYSTEM_PROMPT = """\ You are a code reviewer working inside the repository {repo_url} on branch {branch_name}. @@ -90,23 +93,31 @@ def _build_review_system_prompt(config: TaskConfig, setup: RepoSetup) -> str: ) +def _milestone(progress: _ProgressWriter | None, name: str, detail: str) -> None: + """Emit a progress milestone if a writer is wired up (no-op otherwise).""" + if progress is not None: + progress.write_agent_milestone(name, detail) + + def run_self_review( config: TaskConfig, setup: RepoSetup, agent_result: AgentResult, - trajectory: _TrajectoryWriter, - progress: _ProgressWriter, + trajectory: _TrajectoryWriter | None, + progress: _ProgressWriter | None, + *, + max_turns: int = _DEFAULT_REVIEW_MAX_TURNS, ) -> AgentResult | None: """Run the self-review phase: LLM critiques its own diff and fixes issues. + Invoked by the ``self_review`` workflow step handler. The step's presence in + the resolved workflow is the enablement signal — there is no separate + feature flag; ``max_turns`` comes from the step (``self_review.max_turns``, + default 5) and caps the review loop within the task's remaining allowance. + Returns the AgentResult from the review phase, or None if skipped. Fail-open: errors are logged but never block the pipeline. """ - # Skip condition: feature disabled - if not config.self_review_enabled: - log("TASK", "self_review: disabled (self_review_enabled=False)") - return None - # Skip condition: read-only workflows produce no diff to review if config.read_only: log("TASK", "self_review: skipped for read-only workflow") @@ -115,7 +126,7 @@ def run_self_review( # Compute remaining turns used_turns = agent_result.turns or 0 remaining_turns = config.max_turns - used_turns - review_turns = min(remaining_turns, config.self_review_max_turns) + review_turns = min(remaining_turns, max_turns) if review_turns <= 0: log("TASK", f"self_review: no remaining turns (used={used_turns}, max={config.max_turns})") return None @@ -162,7 +173,8 @@ def run_self_review( f"budget={'$' + f'{review_budget:.2f}' if review_budget else 'unlimited'}, " f"diff_chars={len(diff)})", ) - progress.write_agent_milestone( + _milestone( + progress, "self_review_started", f"turns={review_turns} diff_chars={len(diff)}", ) @@ -182,7 +194,8 @@ def run_self_review( except Exception as e: # Fail-open: self-review errors never block the pipeline log("WARN", f"self_review: agent execution failed: {type(e).__name__}: {e}") - progress.write_agent_milestone( + _milestone( + progress, "self_review_complete", f"status=error error={type(e).__name__}: {e}", ) @@ -193,7 +206,8 @@ def run_self_review( f"self_review: complete (status={review_result.status}, " f"turns={review_result.turns}, cost=${review_result.cost_usd or 0:.4f})", ) - progress.write_agent_milestone( + _milestone( + progress, "self_review_complete", f"status={review_result.status} turns={review_result.turns}", ) diff --git a/agent/src/server.py b/agent/src/server.py index ea615ce9..e1114374 100644 --- a/agent/src/server.py +++ b/agent/src/server.py @@ -405,8 +405,6 @@ def _run_task_background( user_id: str = "", workload_access_token: str = "", attachments: list[dict] | None = None, - self_review_enabled: bool = False, - self_review_max_turns: int = 5, ) -> None: """Run the agent task in a background thread.""" global _background_pipeline_failed @@ -490,8 +488,6 @@ def _run_task_background( trace=trace, user_id=user_id, attachments=attachments, - self_review_enabled=self_review_enabled, - self_review_max_turns=self_review_max_turns, ) _background_pipeline_failed = False except Exception as e: @@ -580,9 +576,6 @@ def _extract_invocation_params(inp: dict, request: Request) -> dict: channel_source = inp.get("channel_source", "") or "" channel_metadata = inp.get("channel_metadata") or {} attachments = inp.get("attachments") or [] - # Self-review (opt-in): LLM critiques its own diff before PR creation. - self_review_enabled = inp.get("self_review_enabled") is True - self_review_max_turns = int(inp.get("self_review_max_turns", 5)) # ``trace`` is strictly opt-in (design §10.1). Accept only real # booleans from the orchestrator — a string "false" would otherwise # flip the flag on. @@ -656,8 +649,6 @@ def _extract_invocation_params(inp: dict, request: Request) -> dict: "user_id": user_id, "workload_access_token": workload_access_token, "attachments": attachments, - "self_review_enabled": self_review_enabled, - "self_review_max_turns": self_review_max_turns, } diff --git a/agent/src/workflow/models.py b/agent/src/workflow/models.py index 1411a16e..887d425a 100644 --- a/agent/src/workflow/models.py +++ b/agent/src/workflow/models.py @@ -31,6 +31,7 @@ "run_agent", "verify_build", "verify_lint", + "self_review", "ensure_pr", "post_review", "deliver_artifact", @@ -140,6 +141,9 @@ class Step(BaseModel): strategy: EnsurePrStrategy | None = None gate: VerifyGate | None = None target: DeliverTarget | None = None + # self_review: cap on turns the review loop may use (default applied by the + # handler when omitted). + max_turns: int | None = None class TerminalOutcomes(BaseModel): diff --git a/agent/src/workflow/runner.py b/agent/src/workflow/runner.py index 4f818689..b58008c1 100644 --- a/agent/src/workflow/runner.py +++ b/agent/src/workflow/runner.py @@ -375,6 +375,10 @@ def _milestone(ctx: StepContext, milestone: str) -> None: # pipeline.py) to avoid pulling the SDK / boto3 at module import and to keep the # orchestration core importable in isolation for tests. +# Default turn cap for a self_review step that omits max_turns (kept in sync +# with the schema's self_review max_turns default and self_review.py). +_DEFAULT_SELF_REVIEW_MAX_TURNS = 5 + def _handle_clone_repo(step: Step, ctx: StepContext) -> StepOutcome: """Clone + prepare the repo (replaces the inline ``setup_repo`` call). @@ -560,6 +564,72 @@ def _handle_verify_lint(step: Step, ctx: StepContext) -> StepOutcome: ) +def _handle_self_review(step: Step, ctx: StepContext) -> StepOutcome: + """Have the agent critique its own diff and fix issues before the PR opens. + + Declaring a ``self_review`` step in the workflow *is* the enablement — there + is no separate feature flag. The step runs a second, review-focused + ``run_agent`` loop over the cumulative branch diff (``self_review.run_self_review``), + capped at ``step.max_turns`` turns (default 5) drawn from the task's + remaining turn/budget allowance, and accumulates the review's turns/cost back + onto ``ctx.agent_result`` so the terminal result reflects the full task. + + Authored as an advisory step (``on_failure: continue``): a review that is + skipped (read-only, no diff, no remaining turns) or fails is recorded but + never blocks PR creation — matching the original fail-open design. The + summary the review agent writes is posted as a PR comment by the pipeline + after ``ensure_pr`` (``self_review_ran`` in this outcome's data signals it). + """ + if ctx.setup is None: + return StepOutcome( + kind=step.kind, + name=_step_key(step), + status="failed", + error="self_review requires a cloned repo (no clone_repo step ran)", + ) + if ctx.agent_result is None: + return StepOutcome( + kind=step.kind, + name=_step_key(step), + status="failed", + error="self_review requires a prior run_agent result to size its turn budget", + ) + + from self_review import run_self_review + + review_result = run_self_review( + ctx.config, + ctx.setup, + ctx.agent_result, + ctx.trajectory, + ctx.progress, + max_turns=step.max_turns or _DEFAULT_SELF_REVIEW_MAX_TURNS, + ) + if review_result is None: + # Skipped (read-only / empty diff / no remaining turns) — not a failure. + return StepOutcome( + kind=step.kind, + name=_step_key(step), + status="succeeded", + data={"self_review_ran": False}, + ) + + # Accumulate the review phase's turns/cost onto the running agent result so + # the terminal TaskResult reflects the whole task (implement + review). + ar = ctx.agent_result + ar.turns += review_result.turns + ar.num_turns += review_result.num_turns or review_result.turns + if review_result.cost_usd is not None: + ar.cost_usd = (ar.cost_usd or 0.0) + review_result.cost_usd + + return StepOutcome( + kind=step.kind, + name=_step_key(step), + status="succeeded", + data={"self_review_ran": True, "review_status": review_result.status}, + ) + + def _handle_ensure_pr(step: Step, ctx: StepContext) -> StepOutcome: """Create / push+resolve / resolve a PR per the step's ``strategy``. @@ -645,6 +715,7 @@ def _handle_deliver_artifact(step: Step, ctx: StepContext) -> StepOutcome: "run_agent": _handle_run_agent, "verify_build": _handle_verify_build, "verify_lint": _handle_verify_lint, + "self_review": _handle_self_review, "ensure_pr": _handle_ensure_pr, "post_review": _handle_post_review, "deliver_artifact": _handle_deliver_artifact, diff --git a/agent/src/workflow/validator.py b/agent/src/workflow/validator.py index 25f2a6ee..21f27c45 100644 --- a/agent/src/workflow/validator.py +++ b/agent/src/workflow/validator.py @@ -40,8 +40,12 @@ _SIDE_EFFECTING_KINDS = frozenset({"ensure_pr", "post_review", "deliver_artifact"}) # Steps that only make sense when a repo is cloned (rule 3 / rule 7). +# self_review diffs and re-runs the agent against the cloned working tree, so it +# is repo-only too. It is deliberately NOT in _SIDE_EFFECTING_KINDS: it commits +# locally but never pushes or opens a PR, so it may be marked on_failure: +# continue (advisory / fail-open). _REPO_ONLY_KINDS = frozenset( - {"clone_repo", "ensure_pr", "post_review", "verify_build", "verify_lint"} + {"clone_repo", "ensure_pr", "post_review", "verify_build", "verify_lint", "self_review"} ) # Built-in (Phase 1-3) policy modules / MCP servers. Registry refs (registry://) diff --git a/agent/tests/test_self_review.py b/agent/tests/test_self_review.py index e2a817f1..11f62deb 100644 --- a/agent/tests/test_self_review.py +++ b/agent/tests/test_self_review.py @@ -1,6 +1,5 @@ """Unit tests for self_review.py — self-review orchestration module.""" -import os from unittest.mock import MagicMock, patch from models import AgentResult, RepoSetup, TaskConfig @@ -16,63 +15,42 @@ def _make_config(**overrides) -> TaskConfig: """Create a minimal TaskConfig for testing.""" - defaults = { - "repo_url": "owner/repo", - "github_token": "ghp_test123", - "aws_region": "us-east-1", - "task_description": "Fix the bug", - "max_turns": 10, - "self_review_enabled": True, - "self_review_max_turns": 5, - "task_type": "new_task", - } - defaults.update(overrides) - return TaskConfig(**defaults) + base = TaskConfig( + repo_url="owner/repo", + github_token="ghp_test123", + aws_region="us-east-1", + task_description="Fix the bug", + max_turns=10, + ) + return base.model_copy(update=overrides) if overrides else base def _make_setup(**overrides) -> RepoSetup: """Create a minimal RepoSetup for testing.""" - defaults = { - "repo_dir": "/workspace/repo", - "branch": "feat/123-fix", - "default_branch": "main", - } - defaults.update(overrides) - return RepoSetup(**defaults) + base = RepoSetup( + repo_dir="/workspace/repo", + branch="feat/123-fix", + default_branch="main", + ) + return base.model_copy(update=overrides) if overrides else base def _make_agent_result(**overrides) -> AgentResult: """Create a minimal AgentResult for testing.""" - defaults = { - "status": "success", - "turns": 5, - "num_turns": 5, - "cost_usd": 0.50, - } - defaults.update(overrides) - return AgentResult(**defaults) + base = AgentResult( + status="success", + turns=5, + num_turns=5, + cost_usd=0.50, + ) + return base.model_copy(update=overrides) if overrides else base class TestSkipConditions: """Test all conditions that cause self-review to be skipped.""" - def test_skip_when_disabled(self): - config = _make_config(self_review_enabled=False) - setup = _make_setup() - agent_result = _make_agent_result() - trajectory = MagicMock() - progress = MagicMock() - - result = run_self_review(config, setup, agent_result, trajectory, progress) - assert result is None - - def test_skip_for_pr_review_task_type(self): - config = _make_config( - task_type="pr_review", - pr_number="42", - task_description="", - issue_number="", - ) + def test_skip_for_read_only_workflow(self): + config = _make_config(read_only=True) setup = _make_setup() agent_result = _make_agent_result() trajectory = MagicMock() @@ -82,13 +60,13 @@ def test_skip_for_pr_review_task_type(self): assert result is None def test_skip_when_no_remaining_turns(self): - config = _make_config(max_turns=5, self_review_max_turns=5) + config = _make_config(max_turns=5) setup = _make_setup() agent_result = _make_agent_result(turns=5) trajectory = MagicMock() progress = MagicMock() - result = run_self_review(config, setup, agent_result, trajectory, progress) + result = run_self_review(config, setup, agent_result, trajectory, progress, max_turns=5) assert result is None def test_skip_when_no_remaining_budget(self): @@ -208,17 +186,17 @@ class TestBudgetAndTurnComputation: @patch("self_review._get_diff", return_value="diff --git a/f\n+line\n") @patch("self_review.asyncio.run") - def test_review_turns_capped_at_self_review_max_turns(self, mock_asyncio_run, mock_diff): + def test_review_turns_capped_at_step_max_turns(self, mock_asyncio_run, mock_diff): mock_asyncio_run.return_value = AgentResult(status="success", turns=2, num_turns=2) - config = _make_config(max_turns=100, self_review_max_turns=3) + config = _make_config(max_turns=100) setup = _make_setup() agent_result = _make_agent_result(turns=5) trajectory = MagicMock() progress = MagicMock() - run_self_review(config, setup, agent_result, trajectory, progress) + # The step's max_turns (3) caps the review even though 95 turns remain. + run_self_review(config, setup, agent_result, trajectory, progress, max_turns=3) - # The review config passed to run_agent should have max_turns=3 call_args = mock_asyncio_run.call_args coro = call_args[0][0] # Close the coroutine to avoid warnings @@ -228,15 +206,15 @@ def test_review_turns_capped_at_self_review_max_turns(self, mock_asyncio_run, mo @patch("self_review.asyncio.run") def test_review_turns_uses_remaining_when_less_than_cap(self, mock_asyncio_run, mock_diff): mock_asyncio_run.return_value = AgentResult(status="success", turns=1, num_turns=1) - config = _make_config(max_turns=8, self_review_max_turns=5) + config = _make_config(max_turns=8) setup = _make_setup() agent_result = _make_agent_result(turns=6) # Only 2 remaining trajectory = MagicMock() progress = MagicMock() - run_self_review(config, setup, agent_result, trajectory, progress) + # Should use min(2 remaining, 5 cap) = 2 turns + run_self_review(config, setup, agent_result, trajectory, progress, max_turns=5) - # Should use min(2, 5) = 2 turns call_args = mock_asyncio_run.call_args coro = call_args[0][0] coro.close() @@ -248,9 +226,7 @@ class TestHappyPath: @patch("self_review._get_diff", return_value="diff --git a/file.py\n+new code\n") @patch("self_review.asyncio.run") def test_returns_review_result(self, mock_asyncio_run, mock_diff): - review_agent_result = AgentResult( - status="success", turns=2, num_turns=2, cost_usd=0.10 - ) + review_agent_result = AgentResult(status="success", turns=2, num_turns=2, cost_usd=0.10) mock_asyncio_run.return_value = review_agent_result config = _make_config() @@ -482,3 +458,121 @@ def test_comment_body_includes_header(self, tmp_path): body = call_args[body_idx] assert "Self-Review Summary" in body assert "**Findings:** 0" in body + + +class TestSelfReviewStepHandler: + """Tests for the workflow ``self_review`` step handler (workflow/runner.py). + + The step's presence in a workflow is the enablement signal; the handler + wraps ``run_self_review`` and accumulates the review's turns/cost onto the + shared ``ctx.agent_result``. + """ + + def _ctx(self, *, setup, agent_result): + from workflow import Step, StepContext, Workflow + + wf = Workflow.model_validate( + { + "id": "coding/new-task-v1", + "version": "1.0.0", + "domain": "coding", + "prompt": {"template": "registry://prompt/x"}, + "hydration": {"sources": ["task_description"]}, + "agent_config": {"tier": "standard", "allowed_tools": ["Bash"]}, + "steps": [ + {"kind": "run_agent"}, + {"kind": "self_review", "name": "review", "max_turns": 4}, + ], + "terminal_outcomes": {"primary": "pr_url"}, + "status": "production", + } + ) + step = next(s for s in wf.steps if s.kind == "self_review") + ctx = StepContext( + workflow=wf, + config=_make_config(), + setup=setup, + agent_result=agent_result, + trajectory=MagicMock(), + progress=MagicMock(), + ) + return step, ctx, Step + + def test_passes_step_max_turns_and_accumulates(self): + from workflow.runner import _handle_self_review + + setup = _make_setup() + agent_result = _make_agent_result(turns=5, num_turns=5, cost_usd=0.50) + step, ctx, _ = self._ctx(setup=setup, agent_result=agent_result) + + review = AgentResult(status="success", turns=2, num_turns=2, cost_usd=0.10) + with patch("self_review.run_self_review", return_value=review) as mock_review: + outcome = _handle_self_review(step, ctx) + + # The step's max_turns (4) is forwarded to run_self_review. + assert mock_review.call_args.kwargs["max_turns"] == 4 + assert outcome.status == "succeeded" + assert outcome.data["self_review_ran"] is True + # Review turns/cost accumulated onto the shared agent_result. + assert ctx.agent_result.turns == 7 + assert ctx.agent_result.num_turns == 7 + assert abs(ctx.agent_result.cost_usd - 0.60) < 1e-9 + + def test_skipped_review_records_not_ran(self): + from workflow.runner import _handle_self_review + + setup = _make_setup() + agent_result = _make_agent_result(turns=5) + step, ctx, _ = self._ctx(setup=setup, agent_result=agent_result) + + with patch("self_review.run_self_review", return_value=None): + outcome = _handle_self_review(step, ctx) + + assert outcome.status == "succeeded" + assert outcome.data["self_review_ran"] is False + # No accumulation when the review was skipped. + assert ctx.agent_result.turns == 5 + + def test_fails_without_clone(self): + from workflow.runner import _handle_self_review + + agent_result = _make_agent_result() + step, ctx, _ = self._ctx(setup=None, agent_result=agent_result) + + outcome = _handle_self_review(step, ctx) + assert outcome.status == "failed" + assert "cloned repo" in (outcome.error or "") + + def test_default_max_turns_when_step_omits_it(self): + from workflow import Step, StepContext, Workflow + from workflow.runner import _DEFAULT_SELF_REVIEW_MAX_TURNS, _handle_self_review + + wf = Workflow.model_validate( + { + "id": "coding/new-task-v1", + "version": "1.0.0", + "domain": "coding", + "prompt": {"template": "registry://prompt/x"}, + "hydration": {"sources": ["task_description"]}, + "agent_config": {"tier": "standard", "allowed_tools": ["Bash"]}, + "steps": [{"kind": "run_agent"}, {"kind": "self_review"}], + "terminal_outcomes": {"primary": "pr_url"}, + "status": "production", + } + ) + step = next(s for s in wf.steps if s.kind == "self_review") + assert step.max_turns is None + ctx = StepContext( + workflow=wf, + config=_make_config(), + setup=_make_setup(), + agent_result=_make_agent_result(turns=2), + trajectory=MagicMock(), + progress=MagicMock(), + ) + review = AgentResult(status="success", turns=1, num_turns=1) + with patch("self_review.run_self_review", return_value=review) as mock_review: + _handle_self_review(step, ctx) + assert mock_review.call_args.kwargs["max_turns"] == _DEFAULT_SELF_REVIEW_MAX_TURNS + # Step model unused beyond fixture; keep import referenced. + assert Step is not None diff --git a/agent/workflows/coding/new-task-v1.yaml b/agent/workflows/coding/new-task-v1.yaml index 7219fdf3..2454b007 100644 --- a/agent/workflows/coding/new-task-v1.yaml +++ b/agent/workflows/coding/new-task-v1.yaml @@ -42,6 +42,12 @@ steps: - { kind: clone_repo, name: setup } - { kind: hydrate_context, name: context } - { kind: run_agent, name: implement } + # Optional pre-PR self-review: the agent critiques its own cumulative diff and + # fixes issues before the PR is opened, then posts a summary as a PR comment. + # Advisory (on_failure: continue) and fail-open — a review failure/skip never + # blocks PR creation. Remove this step to disable self-review for the workflow; + # tune the turn budget via max_turns (1-20, default 5). + - { kind: self_review, name: review, on_failure: continue, max_turns: 5 } - { kind: verify_build, name: build, gate: regression_only } - { kind: ensure_pr, name: open_pr, strategy: create } terminal_outcomes: diff --git a/agent/workflows/schema/workflow.schema.json b/agent/workflows/schema/workflow.schema.json index b4b753bb..f68d867b 100644 --- a/agent/workflows/schema/workflow.schema.json +++ b/agent/workflows/schema/workflow.schema.json @@ -187,7 +187,7 @@ "kind": { "type": "string", "description": "Dispatches to a registered step handler.", - "enum": ["clone_repo", "hydrate_context", "run_agent", "verify_build", "verify_lint", "ensure_pr", "post_review", "deliver_artifact"] + "enum": ["clone_repo", "hydrate_context", "run_agent", "verify_build", "verify_lint", "self_review", "ensure_pr", "post_review", "deliver_artifact"] }, "name": { "type": "string", @@ -213,6 +213,12 @@ "type": "string", "description": "For deliver_artifact: the name of a registered Python deliverer (workflow/deliverers.py / DELIVERERS) to route the produced output through. Open string, not a closed enum, so a new delivery method is a registered deliverer, not a schema change (ADR-014 addendum 2026-06-08). First-party names: s3, comment, s3_and_comment. Each deliverer declares the terminal outcomes it produces; validator rule 11 consults that registry.", "minLength": 1 + }, + "max_turns": { + "type": "integer", + "description": "For self_review: cap on agent turns the self-review loop may use (drawn from the task's remaining turn/budget allowance). Defaults to 5 when omitted.", + "minimum": 1, + "maximum": 20 } } } @@ -295,7 +301,7 @@ "steps": { "items": { "properties": { - "kind": { "not": { "enum": ["clone_repo", "ensure_pr", "post_review", "verify_build", "verify_lint"] } } + "kind": { "not": { "enum": ["clone_repo", "ensure_pr", "post_review", "verify_build", "verify_lint", "self_review"] } } } } }, diff --git a/contracts/workflow-validation/rule3-repo-less-has-self-review.json b/contracts/workflow-validation/rule3-repo-less-has-self-review.json new file mode 100644 index 00000000..42378e53 --- /dev/null +++ b/contracts/workflow-validation/rule3-repo-less-has-self-review.json @@ -0,0 +1,69 @@ +{ + "name": "rule3-repo-less-has-self-review", + "description": "Repo-less knowledge workflow illegally declares a self_review step. self_review is repo-only (it diffs and re-runs the agent against a cloned tree), so both the schema's requires_repo:false conditional and cross-field rule 3 reject it.", + "workflow": { + "id": "knowledge/web-research-v1", + "version": "1.0.0", + "domain": "knowledge", + "requires_repo": false, + "read_only": false, + "prompt": { + "template": "registry://prompt/web-research-workflow" + }, + "hydration": { + "sources": [ + "task_description", + "urls" + ] + }, + "agent_config": { + "tier": "elevated", + "allowed_tools": [ + "Read", + "WebFetch" + ], + "cedar_policy_modules": [ + "builtin/hard_deny", + "builtin/soft_deny" + ] + }, + "repo_config": { + "discover": false + }, + "required_inputs": { + "all_of": [ + "task_description" + ] + }, + "steps": [ + { + "kind": "hydrate_context", + "name": "context" + }, + { + "kind": "run_agent", + "name": "research" + }, + { + "kind": "self_review", + "name": "review" + }, + { + "kind": "deliver_artifact", + "name": "deliver", + "target": "s3_and_comment" + } + ], + "terminal_outcomes": { + "primary": "artifact" + }, + "status": "production" + }, + "expected": { + "valid": false, + "violations": [ + "rule-3", + "schema" + ] + } +} diff --git a/contracts/workflow-validation/valid-coding-self-review-step.json b/contracts/workflow-validation/valid-coding-self-review-step.json new file mode 100644 index 00000000..3dccb547 --- /dev/null +++ b/contracts/workflow-validation/valid-coding-self-review-step.json @@ -0,0 +1,94 @@ +{ + "name": "valid-coding-self-review-step", + "description": "Coding workflow with an optional pre-PR self_review step (advisory, on_failure: continue). self_review is repo-only and non-side-effecting, so on_failure: continue is permitted (unlike ensure_pr).", + "workflow": { + "id": "coding/new-task-v1", + "version": "1.0.0", + "domain": "coding", + "description": "Implement a GitHub issue, self-review, and open a PR.", + "requires_repo": true, + "read_only": false, + "prompt": { + "template": "registry://prompt/coding-new-task-workflow" + }, + "hydration": { + "sources": [ + "issue", + "memory", + "task_description" + ] + }, + "agent_config": { + "tier": "standard", + "allowed_tools": [ + "Bash", + "Read", + "Write", + "Edit", + "Glob", + "Grep", + "WebFetch" + ], + "cedar_policy_modules": [ + "builtin/hard_deny", + "builtin/soft_deny" + ] + }, + "repo_config": { + "provider": "github", + "discover": true + }, + "required_inputs": { + "one_of": [ + "issue_number", + "task_description" + ] + }, + "steps": [ + { + "kind": "clone_repo", + "name": "setup" + }, + { + "kind": "hydrate_context", + "name": "context" + }, + { + "kind": "run_agent", + "name": "implement" + }, + { + "kind": "self_review", + "name": "review", + "on_failure": "continue", + "max_turns": 5 + }, + { + "kind": "verify_build", + "name": "build", + "gate": "regression_only" + }, + { + "kind": "ensure_pr", + "name": "open_pr", + "strategy": "create" + } + ], + "terminal_outcomes": { + "primary": "pr_url" + }, + "limits": { + "max_turns": 100 + }, + "promotion_gate": { + "requires": [ + "tests:agent/new_task" + ] + }, + "status": "production" + }, + "expected": { + "valid": true, + "violations": [] + } +} From dff38b609c6f0b6f8b8088b4e66c8bce0ace45fe Mon Sep 17 00:00:00 2001 From: bgagent Date: Fri, 26 Jun 2026 10:02:28 -0400 Subject: [PATCH 4/5] remove unnecessary comments --- agent/workflows/coding/new-task-v1.yaml | 5 ----- 1 file changed, 5 deletions(-) diff --git a/agent/workflows/coding/new-task-v1.yaml b/agent/workflows/coding/new-task-v1.yaml index 2454b007..93dc308e 100644 --- a/agent/workflows/coding/new-task-v1.yaml +++ b/agent/workflows/coding/new-task-v1.yaml @@ -42,11 +42,6 @@ steps: - { kind: clone_repo, name: setup } - { kind: hydrate_context, name: context } - { kind: run_agent, name: implement } - # Optional pre-PR self-review: the agent critiques its own cumulative diff and - # fixes issues before the PR is opened, then posts a summary as a PR comment. - # Advisory (on_failure: continue) and fail-open — a review failure/skip never - # blocks PR creation. Remove this step to disable self-review for the workflow; - # tune the turn budget via max_turns (1-20, default 5). - { kind: self_review, name: review, on_failure: continue, max_turns: 5 } - { kind: verify_build, name: build, gate: regression_only } - { kind: ensure_pr, name: open_pr, strategy: create } From af66c61bdfeff888dc873d9383ae292e5a7c3b79 Mon Sep 17 00:00:00 2001 From: bgagent Date: Mon, 6 Jul 2026 16:08:04 -0400 Subject: [PATCH 5/5] refactor(agent): make self-review critic read-only and increase max_turns to 20 The critic was attempting to fix issues directly, consuming turns on edits and commits rather than writing findings. This caused it to exceed its turn budget before producing the summary file. Changes: - Prompt now instructs critic to be read-only: no file edits, no commits - Summary format simplified (removed 'Fixes applied' field) - max_turns increased from 5 to 20 to give adequate budget for thorough review --- agent/src/prompts/self_review.py | 18 +++++++----------- agent/workflows/coding/new-task-v1.yaml | 2 +- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/agent/src/prompts/self_review.py b/agent/src/prompts/self_review.py index 25cf47a2..c863924f 100644 --- a/agent/src/prompts/self_review.py +++ b/agent/src/prompts/self_review.py @@ -29,29 +29,25 @@ ## Instructions -- If you find issues, fix them directly: edit the files, run the build/tests to \ -verify your fixes, and commit the changes. -- If no issues are found, stop immediately — do not make changes for the sake of \ -making changes. -- Do NOT refactor code that was not part of the original diff unless it has a \ -concrete bug or security issue. -- Keep fixes minimal and focused. Each fix should be a separate commit with a \ -clear message. +- You are a **read-only reviewer**. Do NOT modify any files, do NOT make commits, \ +do NOT attempt to fix issues. Your sole job is to identify and report findings. +- If no issues are found, stop immediately and write the summary noting no issues. +- Focus only on code that is part of the diff — do not review unchanged code. +- Be specific: reference file paths and line numbers when reporting issues. ## Summary output -After completing your review (whether you made fixes or not), write a file \ +After completing your review, write a file \ `.self-review-summary.md` in the repository root with your findings in this format: ```markdown ### Self-Review Summary **Findings:** -**Fixes applied:** #### Issues found -- : — +- : (:) ``` If no issues were found, write the file with: "No issues found — code looks good." diff --git a/agent/workflows/coding/new-task-v1.yaml b/agent/workflows/coding/new-task-v1.yaml index 93dc308e..16c9b679 100644 --- a/agent/workflows/coding/new-task-v1.yaml +++ b/agent/workflows/coding/new-task-v1.yaml @@ -42,7 +42,7 @@ steps: - { kind: clone_repo, name: setup } - { kind: hydrate_context, name: context } - { kind: run_agent, name: implement } - - { kind: self_review, name: review, on_failure: continue, max_turns: 5 } + - { kind: self_review, name: review, on_failure: continue, max_turns: 20 } - { kind: verify_build, name: build, gate: regression_only } - { kind: ensure_pr, name: open_pr, strategy: create } terminal_outcomes: