feat(agent): add in-pipeline pre-PR isolated sub-agent critic (adversarial review) phase

[nizar-lahlali]

Summary

• Adds an optional self-review pipeline phase where the LLM critiques its own cumulative diff before the PR is created
• New self_review.py orchestration module with run_self_review() that generates the diff, invokes run_agent() a second time with a review-focused prompt, and accumulates turns/cost
• New prompts/self_review.py with a focused review prompt template (correctness, bugs, security, style, test gaps checklist)
• Exposed through the workflow system as a self_review step kind: declaring the step in the workflow YAML is the enablement (no separate feature flag), with max_turns configured on the step (schema + Step model + validator + parity fixtures)
• Enabled in coding/new-task-v1.yaml as an advisory step (on_failure: continue, max_turns: 20)
• Self-review summary posted as a PR comment — the review agent writes .self-review-summary.md with structured findings, the pipeline posts it via gh pr comment, then deletes the file so it never appears in the diff
• NEW: Critic prompt is configurable per-workflow (feat(agent): make the self-review critic prompt configurable per-workflow (fast follow to #262) #536) — the self_review step accepts an optional prompt field rendered with {diff} / {task_description} placeholders, so workflow authors can tune the review focus (e.g. security-heavy) without a code change; the built-in prompt remains the default

Design decisions:

• Fail-open: self-review errors never block PR creation; a custom prompt that is malformed or omits {diff} falls back to the built-in prompt with a logged warning
• Uses remaining turns/budget from original max_turns allocation (capped at the step's max_turns)
• Second run_agent() call gives the model fresh context and a clean review perspective
• Critic is read-only: it reports findings in the summary file rather than editing/committing, so its turn budget goes to review depth
• Skipped for read-only workflows, empty diff, no remaining turns/budget
• Feature is opt-in per workflow (step absent ⇒ phase never runs) — no behavior change for workflows that don't declare it
• Summary file approach (vs. parsing trajectory): deterministic, decoupled from SDK internals
• Comment posted after PR creation (natural ordering — PR must exist before commenting)

Closes #262
Closes #536

Test plan

• All 44 unit tests pass (tests/test_self_review.py) — includes step-handler threading, summary reader / PR comment posting, and custom-prompt rendering + fallback tests
• Full agent suite passes (1171 tests, no regressions), including workflow loader / runner / validator and the contracts/workflow-validation/ parity corpus
• Ruff lint + format pass on all new/modified files
• Manual validation in local batch mode: phase executes and summary is posted
• Integration: deploy and submit a task through coding/new-task-v1 and verify the review comment on the resulting PR

[krokoko]

I think this is fine, just should be exposed using the new workflow system, a.k.a: add a method to do self review, and make it configurable as a step in the coding task workflow yaml file

[theagenticguy]

It might be semantics but should this be called a self review or should it be explicitly described as a isolated sub agent critic (aka adversarial review)?

[nizar-lahlali]

Fast follow tracked in #536: make the self-review critic prompt configurable per-workflow (self_review step prompt field in the workflow YAML) instead of the hardcoded SELF_REVIEW_PROMPT constant.

[nizar-lahlali]

Implemented the configurable critic prompt fast follow (#536) directly on this branch in af99d56: the self_review step now accepts an optional prompt field (rendered with {diff} / {task_description}; fail-open fallback to the built-in prompt when the template is malformed or omits {diff}).

[isadeks]

Code review — pre-PR self-review critic phase

Solid plumbing (step registration, schema, validator _REPO_ONLY_KINDS, contract fixtures, turn/cost accumulation, fail-open handling) with a good 44-test suite. The problems are in the runtime behavior contract, not the wiring. Requesting changes on the merge-blocker (#1) below.

🔴 1. The two prompts handed to the critic directly contradict each other (merge-blocker)

• System prompt (self_review.py:30-34): "Fix any issues you find directly — edit files, run the build, and commit fixes."
• Built-in user prompt (prompts/self_review.py:32-33): "You are a read-only reviewer. Do NOT modify any files, do NOT make commits, do NOT attempt to fix issues."

Both are sent to the same run_agent() call, and the shipped new-task-v1.yaml supplies no custom prompt, so every default run gets this conflict. The feature's core behavior is undefined — the model may fix+commit (mutating the PR) or only report. The PR description itself claims both ("adversarial review that fixes" vs "read-only reviewer"). This needs to resolve to one intent before merge; several findings below depend on which you pick.

🔴 2. Reviewer sees a committed-only diff, computed before the safety-net commit

_get_diff runs git diff origin/{default_branch}...HEAD (committed changes only) at pipeline.py:1061, but ensure_committed (which commits work the agent left uncommitted) doesn't run until pipeline.py:1074. Any uncommitted implement-phase work is invisible to the critic. In the worst case (agent left everything uncommitted — the turn-limit/timeout scenario where review matters most), _get_diff returns empty and the review is silently skipped (self_review.py:159). Move the safety-net commit before self-review, or have _get_diff include the working tree.

🟠 3. .self-review-summary.md can be committed and pushed into the PR — the exact leak the design claims to prevent

The summary file is not in .gitignore (verified: git check-ignore → exit 1). The critic runs with full Write/Bash tools (model_copy only overrides max_turns/max_budget_usd, so allowed_tools is preserved) and a system prompt telling it to commit. If it does git add -A && git commit, the untracked summary is committed, then ensure_pr pushes it at :1079. Cleanup (read_self_review_summary → os.remove + git rm --cached) doesn't run until :1095, after the push, and git rm --cached never commits/pushes the deletion — so it cannot un-leak an already-pushed file. Add .self-review-summary.md to .gitignore (defense-in-depth), or write it outside the repo tree.

🟠 4. A failing self_review step cannot actually gate the PR (dead on_failure)

_execute_self_review_step (pipeline.py:260) discards run_workflow's WorkflowResult and returns only bool(ctx.artifacts.get("self_review_ran")). If the step fails (ctx.setup/ctx.agent_result is None → status="failed", or the handler raises), run_workflow honors on_failure internally — but the pipeline never reads that verdict and proceeds straight to ensure_pr. The on_failure knob is inert for self_review on the coding path. There's also no validator rule requiring on_failure: continue for self_review, so its advisory-ness rests entirely on the author remembering the flag.

🟠 5. Second run_agent() re-inits the policy engine from task-start values

run_agent rebuilds PolicyEngine via _initialize_policy_engine_and_hooks, seeded from config.initial_approval_gate_count / config.initial_approvals (frozen at task start). The per-task approval-gate counter and any approvals granted during implement live only in the first engine instance and are lost. For an approval-gated (Cedar HITL) workflow, the review phase starts with a fresh gate budget (can exceed approval_gate_cap) and re-prompts for already-approved scopes. Config-contingent — worth confirming against your gated workflows.

🟡 6. Summary cleanup is coupled to the happy path

read_self_review_summary (the only deleter) is reached only through if pr_url and self_review_ran: post_self_review_comment(...) (pipeline.py:1094-1095). If ensure_pr returns None (no commits, gh failure, resolve-miss) or the critic errored after writing the file, cleanup never runs and the artifact lingers (and, if committed, in local history to be pushed on the next resume). Make cleanup unconditional once the review step ran.

🟡 7. Review runs even when the implement phase hard-errored

_execute_self_review_step is called unconditionally at pipeline.py:1061 — including when the implement run_agent threw and agent_result was set to status="error", turns=0. run_self_review never inspects agent_result.status; with turns=0 it computes a full turn budget and launches a second agent loop after the first already failed (e.g. re-hitting the same Bedrock auth error). Skip review when the implement phase errored.

🟡 8. max_turns/prompt step fields are accepted on every step kind, and maximum: 20 caps them globally

The new fields sit at the shared steps.items.properties level with no if kind == self_review conditional. Setting max_turns on a run_agent step validates but is silently ignored (run_agent uses config.max_turns); setting max_turns: 30 on any step is spuriously rejected because the self-review-only "maximum": 20 caps all kinds. Scope these fields to self_review via a schema conditional.

⚪ 9. Schema claims prompt "Must contain {diff}" but nothing enforces it

workflow.schema.json:223 documents the constraint; only minLength: 1 is enforced. A custom prompt lacking {diff} validates (and passes the contract corpus) but is silently discarded at runtime in favor of the built-in — validation vouches for a prompt the runtime never applies.

⚪ 10. Default 5 is triplicated

_DEFAULT_SELF_REVIEW_MAX_TURNS (runner.py:380), _DEFAULT_REVIEW_MAX_TURNS (self_review.py:32), and the schema description are hand-synced. The handler always passes step.max_turns or _DEFAULT_SELF_REVIEW_MAX_TURNS, making self_review.py's default param dead for the real caller. Single-source it.

Positives

• Turn/cost accumulation onto the shared agent_result is correct and tested.
• Fail-open exception handling is thorough; _truncate_diff correctly cuts at hunk boundaries.
• No asyncio-nesting bug — run_task is sync, so the second asyncio.run mirrors _handle_run_agent correctly.
• Good contract-fixture coverage for the repo-less rejection.

Bottom line

Infrastructure is solid, but #1 (contradictory prompts) is a merge-blocker — it leaves the feature's central behavior undefined — and it cascades into #3 (file leak) and #2 (diff completeness). Suggest resolving the fix-vs-report intent first, then the diff-ordering and gitignore items, then #4–#8 before this becomes the default on every coding task.

Findings verified against the feat/262-pre-pr-self-review checkout; several via direct code trace (tool-surface preservation, diff/commit ordering, prompt contradiction, cleanup-vs-push ordering).

[nizar-lahlali]

Review fixes (commit 849e6fa)

Addressed both 🔴 merge-blockers.

🔴 1. Contradictory prompts → resolved to read-only

The system prompt in self_review.py told the critic to "Fix any issues you find directly — edit files, run the build, and commit fixes", directly contradicting the built-in user prompt (prompts/self_review.py) that declares a read-only reviewer. This resolves to read-only, matching this PR's stated design ("Critic is read-only: it reports findings in the summary file rather than editing/committing") and the earlier refactor(agent): make self-review critic read-only commit that updated the user prompt but missed the system prompt.

The system prompt now instructs a READ-ONLY reviewer that reports findings to the summary file and does not modify files, commit, push, or open a PR. A regression test (TestReviewSystemPrompt) locks in that the system and user prompts agree.

🔴 2. Reviewer saw a committed-only diff computed before the safety-net commit → safety-net moved earlier

The safety-net ensure_committed now runs before self-review, so the critic's git diff origin/{default}...HEAD includes any uncommitted implement-phase work. Previously the worst case — a turn-limit/timeout run that left everything uncommitted, exactly where review matters most — produced an empty diff and silently skipped the review. A second defensive ensure_committed remains after review (normally a no-op now that the critic is read-only). Regression test TestSafetyCommitBeforeSelfReview asserts the ordering and that read-only workflows still never commit.

Verification

• tests/test_self_review.py: 46 passed
• Full agent suite: 1175 passed, no regressions
• Ruff check + format: clean

[theagenticguy]

Adversarial review across four lenses (control-flow, read-only enforcement, injection, test-integrity). Every finding below was independently re-verified against the branch at HEAD before inclusion. Two code issues block; the remainder is a confirmed test-integrity cluster where the shipping code is correct but the tests don't guard it.

🔴 Blocking

1. The "read-only critic" is enforced only by prompt text, not structurally

agent/src/self_review.py:193 — run_self_review builds the review config as:

review_config = config.model_copy(update={"max_turns": review_turns, "max_budget_usd": review_budget})

It never sets read_only=True and never trims allowed_tools. So the critic inherits coding/new-task-v1's full surface — Bash / Write / Edit — and runs under permission_mode="bypassPermissions". In runner.py, _resolve_allowed_tools drops Write/Edit only when config.read_only is true (it isn't here), _DISALLOWED_TOOLS doesn't include them, and PolicyEngine is constructed with read_only=False, so the read_only_forbid_write / read_only_forbid_edit Cedar rules (guarded by context.read_only == true) never fire. The only thing keeping the critic read-only is the prompt text (_REVIEW_SYSTEM_PROMPT + SELF_REVIEW_PROMPT).

The critic's entire input is the branch diff — attacker-influenceable content from the cloned repo / issue. A prompt-injection payload embedded in the diff (a comment or fixture string steering "reviewer mode ended, edit X to fix the issue") can make the critic Edit a tracked file or git commit via Bash. Then pipeline.py:1086 safety-net #2 (git add -u + commit) stages that change and ensure_pr (:1092, strategy create) pushes the feature branch — so the critic's mutations land in the PR. The feature branch isn't a protected branch, so the soft-deny push rules don't gate it.

This directly contradicts the PR's stated design ("Critic is read-only: it reports findings ... rather than editing/committing"). Fix: set read_only=True in the model_copy update so _resolve_allowed_tools drops the write tools and PolicyEngine enforces the read-only Cedar rules — enforcement becomes structural rather than prompt-deep.

2. _truncate_diff can discard the entire reviewable diff

agent/src/self_review.py:69 — for a large single-hunk diff (one big new file, or one hunk exceeding the 60KB window), the only \n@@ marker sits near the top of the kept window, so:

truncated = diff[:max_chars]
last_hunk = truncated.rfind("\n@@")
if last_hunk > 0:
    truncated = truncated[:last_hunk]

cuts back to that early offset, leaving only the diff --git / index / --- / +++ file-header lines — zero hunk body. Reproduced: an 88K-char single-hunk diff truncated to ~150 chars with 0 code lines kept; a 79K-char new 2000-line file truncated to ~182 chars with 0 +def lines kept. (Multi-hunk diffs are unaffected — the else hard-cut only fires when there's no \n@@ at all.) The if diff in rendered: guard in _render_review_prompt runs against the already-truncated diff, so it passes trivially and can't detect the missing body. The critic then reviews an essentially empty diff and can emit "No issues found" while the pipeline reports the review ran. Fix: when the last-hunk cut yields no hunk body, fall back to the hard-cut path (keep content up to max_chars at a line boundary) instead of discarding to the header.

🟡 Test-integrity (non-blocking, but worth fixing before merge)

The live code is correct in each case below — the tests just don't actually guard it, so a future regression ships green.

• agent/tests/test_self_review.py:272 — test_review_turns_capped_at_step_max_turns and test_review_turns_uses_remaining_when_less_than_cap both call run_self_review, pull the coroutine out of the mocked asyncio.run, and coro.close() it with zero assertions. The cap logic review_turns = min(remaining_turns, max_turns) (self_review.py:159) is never inspected — changing min to max keeps both tests green. The only turn assertion in the suite checks the value passed into run_self_review, not the min() output.
• agent/src/self_review.py:168 — the positive remaining-budget path (remaining_budget = config.max_budget_usd - used_cost) is untested; every test uses the max_budget_usd=None default or the <=0 skip branch. Inverting the subtraction ships green.
• agent/src/pipeline.py:1086 — safety-net commit #2 (post-review) is untested. The one ordering test asserts only call_order.index("ensure_committed") < call_order.index("self_review"), and .index() returns the first occurrence (satisfied by safety-net #1). Deleting #2 entirely keeps the suite green.
• agent/tests/test_self_review.py:258 — test_system_and_user_prompts_agree checks the system prompt only via disjoint substring membership ("do not" in system and "modify" in system), so a mutation-inviting prompt like "you may modify files ... do not push" passes both checks while contradicting the read-only contract.
• agent/src/workflow/runner.py:624 (minor) — the ar.num_turns += review_result.num_turns or review_result.turns fallback is never exercised; no test builds a review result with num_turns falsy while turns>0. Correct as written, just uncovered.

✅ Verified clean

Control-flow ordering (commit#1 → self_review → commit#2 → build → ensure_pr → comment) is sound; the self_review_ran flag propagation connects correctly through StepContext.record → ctx.artifacts (the feature does not silently no-op); turns/cost accumulation onto the shared agent_result has no double-count; and only_kinds={"self_review"} + on_failure: continue semantics are correct (the new StepContext omitting system_prompt/user_prompt is fine — _handle_self_review never reads them). One cosmetic note: read_self_review_summary's deletion runs after the push, so "so it never appears in the PR" is actually served by the summary file's untracked-ness (git add -u skips untracked), not by the delete — harmless today, but the comment describes a safeguard that isn't the operative one.

---

Review generated via a 4-lens adversarial workflow (find → default-to-refute verify) grounded in the checked-out branch.