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).