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.

[nizar-lahlali]

Addressed two review findings in agent/src/self_review.py (commit 7542883d):

1. Read-only critic is now enforced structurally, not just by prompt text

run_self_review now sets read_only=True on the review config:

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

This flips the two enforcement points in runner.py: _resolve_allowed_tools drops Write/Edit from the critic's surface, and PolicyEngine is constructed with read_only=True so the read_only_forbid_write / read_only_forbid_edit Cedar rules fire. A prompt-injection payload embedded in the attacker-influenceable branch diff can no longer steer the critic into editing/committing tracked files that would then land in the PR via the pipeline's safety-net commit + push. Enforcement is now structural rather than prompt-deep, matching the PR's stated design.

2. _truncate_diff no longer discards the entire hunk body

For a large single-hunk diff (one big new file, or a hunk wider than the 60KB window), the only @@ marker sits near the top, so cutting back to the last hunk header left only the diff --git / index / --- / +++ file-header lines — zero hunk body. The last-hunk cut now only fires when more than one hunk header fits in the window (last_hunk != first_hunk); otherwise it falls back to the line-boundary hard-cut, preserving real content so the critic reviews the actual diff.

Tests: added test_single_large_hunk_keeps_body, test_single_wide_hunk_over_window_keeps_body, and test_review_config_is_read_only. All 49 tests in test_self_review.py pass; ruff clean.

[scottschreckengaust]

Principal Architect Review — PR #263: pre-PR self-review critic phase

Reviewed as a Principal AWS Solutions Architect against the ABCA /review_pr workflow. I read the two outstanding CHANGES_REQUESTED reviews (@isadeks, @theagenticguy) first and independently re-verified each of their blocking claims against the code at HEAD 54361d6.

1. Verdict

Comment — code is merge-ready; one documentation-drift item and a few non-blocking design nits remain. All four merge-blockers raised by the two prior reviewers are verifiably fixed at HEAD. I am not clearing their CHANGES_REQUESTED for them (they should re-review), but from a principal lens the code, tests, and CI are in good shape. blockingCount = 0 — no code-level blocker survives verification.

2. Vision alignment

Fits the north star well. The critic strengthens reviewable outcomes (a structured findings comment on every PR) without touching the fire-and-forget path: it is fail-open, advisory (on_failure: continue), and bounded by the task's remaining turn/budget allowance — so it never blocks PR creation and never widens cost blast radius beyond the existing max_turns envelope. Exposing it as a first-class self_review StepKind (rather than the original per-task API flag) is the right call and matches @krokoko's early steer and ADR-014's workflow-driven model. The read-only critic config is a genuine bounded blast radius improvement.

3. Prior blockers — re-verified at HEAD (all resolved)

1. Contradictory prompts (isadeks 🔴#1 / theagenticguy 🔴#1 prompt-deep): RESOLVED. _REVIEW_SYSTEM_PROMPT (agent/src/self_review.py:296-306) now instructs a READ-ONLY reviewer, agreeing with SELF_REVIEW_PROMPT. Regression tests TestReviewSystemPrompt lock it in.
2. Read-only enforced only by prompt text (theagenticguy 🔴#1): RESOLVED. run_self_review sets read_only=True on the review config (self_review.py:479-485). Verified this flips both enforcement points: runner._resolve_allowed_tools drops Write/Edit (runner.py:401-402) and PolicyEngine(read_only=True) fires read_only_forbid_write/read_only_forbid_edit in policies/hard_deny.cedar:22-38. TestReadOnlyEnforcement guards it.
3. Committed-only diff computed before the safety-net commit (isadeks 🔴#2): RESOLVED. Safety-net #1 ensure_committed now runs BEFORE self-review (pipeline.py:1112-1119), with a defensive #2 after (pipeline.py:1153-1157). TestSafetyCommitBeforeSelfReview asserts the ordering and that read-only workflows still never commit.
4. _truncate_diff discards the entire hunk body (theagenticguy 🔴#2): RESOLVED. The last-hunk cut now only fires when more than one hunk header fits (last_hunk != first_hunk, self_review.py:348); otherwise it falls back to a line-boundary hard-cut. test_single_large_hunk_keeps_body / test_single_wide_hunk_over_window_keeps_body cover it.

Security re-check (my own, via /security-review): subprocess calls in post_self_review_comment, _get_diff, and the git rm --cached cleanup all use argument-list form with no shell=True; LLM summary content flows only into the --body positional of gh pr comment, so no command injection. The .self-review-summary.md leak (isadeks 🟠#3) is now mitigated in practice: the critic is read-only (no Write/Edit) and safety-net commits use git add -u, which skips the untracked summary file. No HIGH/MEDIUM security finding survives.

4. Non-blocking suggestions / nits

1. Bash remains reachable for the read-only critic (defense-in-depth). read_only=True drops Write/Edit but not Bash, and hard_deny.cedar does not gate git add/git commit via Bash. Because Write is dropped, the critic must use Bash to author the summary file — so Bash-based mutation is reachable, and a prompt-injection payload in the attacker-influenceable diff could in principle steer a Bash edit+commit that safety-net #2 (git add -u) then pushes. Below the security-review confidence bar (reduces to "untrusted content in an LLM prompt"), but worth hardening: either drop Bash for the critic and have the pipeline write the summary, or add .self-review-summary.md to .gitignore.
2. Review runs even when the implement phase hard-errored (isadeks 🟡#7 — still open). _execute_self_review_step is called unconditionally (pipeline.py:1135); run_self_review never inspects agent_result.status. On an implement status="error", turns=0 run it computes a full turn budget and launches a second agent loop that may re-hit the same failure. Cheap guard: skip when the implement result errored.
3. on_failure verdict is inert for self_review on the coding path (isadeks 🟠#4 — still open). _execute_self_review_step discards run_workflow's WorkflowResult and returns only self_review_ran. Fine while the step is advisory-by-design, but there is no validator rule requiring on_failure: continue for self_review, so its advisory-ness rests on the author remembering the flag. Consider a validator rule.
4. Policy-engine re-init from task-start values (isadeks 🟠#5 — still open, config-contingent). The second run_agent rebuilds PolicyEngine from config.initial_approval_gate_count/initial_approvals, so for an approval-gated (HITL) workflow the review phase starts with a fresh gate budget. Low impact now (the critic is read-only and new-task-v1 uses hard/soft-deny only), but confirm before enabling self_review on any HITL-gated workflow.
5. Schema fields max_turns/prompt are unscoped (isadeks 🟠#8 — still open). They sit at the shared steps.items.properties level with no if kind == self_review conditional; max_turns on a run_agent step validates but is ignored, and the self-review-only maximum: 20 caps max_turns on every kind. Scope via a schema if/then.
6. Triplicated default 5 across runner.py:606, self_review.py:293, and the schema description (isadeks ⚪#10). Single-source it.
7. Cleanup coupled to the happy path (isadeks 🟡#6). read_self_review_summary (the only deleter) is reached only via if pr_url and self_review_ran. Low risk now the file is untracked, but making cleanup unconditional once the step ran is tidier.

5. Documentation

Drift found (should fix before merge — the repo treats doc drift as a blocking concern): docs/design/WORKFLOWS.md has a canonical "Step kinds" reference table (line ~114) enumerating every step kind — clone_repo, hydrate_context, run_agent, verify_build, verify_lint, ensure_pr, post_review, deliver_artifact — but it was not updated to add self_review, even though this PR adds it to the JSON schema enum, models.Step, the runner STEP_HANDLERS, and the validator's _REPO_ONLY_KINDS. Add a self_review row (repo-only, agentic/advisory, non-side-effecting). Because WORKFLOWS.md lives under docs/design/, regenerate the Starlight mirror (mise //docs:sync) in the same commit or CI's "Fail build on mutation" step will reject it (docs/src/content/docs/architecture/Workflows.md).

Other docs: the PR body is excellent (explains why, design decisions, test plan). No ADR needed — this is an instantiation of the ADR-014 workflow-step model, not a new architectural decision.

6. Tests & CI

Strong coverage: 713-line test_self_review.py (skip conditions, diff truncation edge cases, custom-prompt render + fail-open fallback, read-only enforcement, turn/cost accumulation, summary read/cleanup, PR-comment posting) plus 144 lines of test_pipeline.py ordering tests, plus two parity fixtures in contracts/workflow-validation/ (valid coding step + repo-less rejection). All 8 CI checks green (CodeQL, agentcore build, secrets/deps scan, analyze x3, dead-code, PR-title).

Test-integrity notes carried from @theagenticguy (non-blocking): test_review_turns_capped_at_step_max_turns and ..._uses_remaining_when_less_than_cap close the coroutine with zero assertions on the min() output — the cap logic is effectively unguarded; and the positive remaining-budget path (max_budget_usd - used_cost > 0) is uncovered. Worth tightening so a future regression doesn't ship green.

Bootstrap synth-coverage: NOT APPLICABLE — this PR is entirely within agent/ (Python runtime + workflow schema/contracts). It adds no CDK constructs, stacks, handlers, or new CloudFormation resource types, so cdk/src/bootstrap/*, the resource-action-map, BOOTSTRAP_VERSION, and DEPLOYMENT_ROLES.md are correctly untouched.

7. Review agents run (Stage 3)

• /security-review — RAN. IAM/Cedar read-only enforcement, subprocess/gh injection surface, prompt-injection via untrusted diff, summary-file leak. No HIGH/MEDIUM findings.
• code-reviewer — RAN (folded into principal judgment above): correctness of turn/cost accumulation, skip conditions, _truncate_diff edge cases, schema/models/validator parity.
• silent-failure-hunter — RAN: the module is heavily fail-open (_get_diff, run_self_review, post_self_review_comment, read_self_review_summary all swallow-and-log). Verified each is intentional and logged; flagged the one genuinely-silent path (empty-diff → silent skip) which the safety-net-before-review reordering already mitigates.
• type-design-analyzer — RAN: new Step.max_turns/Step.prompt optionals and the self_review StepKind literal. Sound; only nit is the unscoped schema fields (nit #5).
• comment-analyzer — RAN: the extensive inline comments in pipeline.py/self_review.py accurately describe the safety-net ordering and read-only rationale; no stale/contradictory comments post-fix.
• pr-test-analyzer — RAN: coverage is broad; flagged the assertion-free turn-cap tests and the uncovered positive-budget path (Section 6).
• Omitted: none in scope. (No CDK/IaC diff → no cdk-nag/bootstrap agent; no TS API-shape change → no cli/src/types.ts sync check needed.)

8. Human heuristics

• Proportionality — Pass. 300-line module + step handler is proportionate to a real pipeline phase; no premature abstraction.
• Coherence — Concern (minor): the feature is named self_review in code/schema but the PR title calls it an "isolated sub-agent critic (adversarial review)" — @theagenticguy raised this naming question and it was never resolved. Same concept should carry one term; either is fine, but pick one across title/docs/code.
• Clarity — Pass. Names communicate intent; fail-open paths log with type(e).__name__; the safety-net-ordering comments are genuinely load-bearing and accurate.
• Appropriateness — Concern (minor): the turn-cap tests assert nothing about the min() output (they only coro.close()), so they verify what the code does to the mock, not what it should compute (AI005). Tighten before this becomes the default on every coding task.

---

Governance: backing issue #262 carries the approved label and is self-assigned to the author; branch feat/262-pre-pr-self-review follows convention. #536 (configurable critic prompt) is folded in and closed. Governance gate satisfied.

[ayushtr-aws]

Principal review — building on the existing threads

Thanks for the thorough iteration on this one — the workflow-step wiring, turn/cost accounting, and fail-open discipline are all in good shape, and the _truncate_diff, custom-prompt, and cleanup fixes hold up nicely under mutation testing. Rather than re-review from scratch, I worked from @isadeks' and @theagenticguy's earlier reviews and re-verified everything below against the current HEAD (e93b9589). This comment focuses on what's new or still-open, so we don't re-litigate settled points.

🔴 The read_only=True fix doesn't quite deliver structural read-only yet

Commit 7542883 was the right instinct for @theagenticguy's finding #1, and the thread has understandably treated read-only as settled — but I think there's still a gap. read_only only drops Write/Edit:

• runner._resolve_allowed_tools (runner.py:402) removes just _WRITE_TOOLS = {Write, Edit}.
• _DISALLOWED_TOOLS (runner.py:368-377) doesn't include Bash, and allowed_tools is auto-approve only — under permission_mode="bypassPermissions" (runner.py:514) Bash stays reachable.
• The Cedar read_only_forbid_write/edit rules (hard_deny.cedar:22-38) fire on invoke_tool against Tool::"Write"/"Edit" only. policy.py:1394 routes Bash to execute_bash, which has no read_only forbid rule (just rm -rf / and DROP TABLE).

Since the critic's whole prompt is the attacker-influenceable branch diff, an injected echo … >> tracked.py && git commit -am fix (or a curl …?t=$GITHUB_TOKEN) can run via Bash — and safety-net #2 (git add -u) + ensure_pr would then push it on the non-protected feature branch, using the implement phase's token. So the injection path #1 aimed to close is, I think, still reachable.

There's also a related wrinkle worth surfacing together: prompts/self_review.py:40 asks the critic to write .self-review-summary.md, but read_only just removed its only write tools — so today it must produce the summary via Bash redirection, which means we can't simply add Bash to disallowed_tools without also moving the summary off the working tree.

Suggested direction: hard-disallow Bash/Write/Edit (and WebFetch) for the critic via disallowed_tools (reachable = Read/Grep/Glob), strip push credentials from the review env, and take the review text from AgentResult.result_text (or a small constrained tool) instead of a working-tree file — that resolves the write contradiction and closes the Bash surface in one move.

🔴 This introduces a second agentic step, which the ADR-014 invariant currently defers

ADR-014 notes: "The runner enforces exactly one agentic step for now; lifting that (multi-agent workflows) is deliberately deferred to #99." The validator enforces that by counting only run_agent kinds (_rule_2_single_run_agent, validator.py:142) — but self_review is a separate kind that invokes a second top-level run_agent loop, and WORKFLOWS.md itself labels it agentic. So the constraint is effectively bypassed rather than updated. Given the "agent contract changes → ask first" guidance in AGENTS.md, I think this wants a short ADR amendment (or a design that stays inside the existing loop) before it lands. Happy to help sketch that if useful.

🟠 The self-review comment is a side effect the workflow model doesn't account for

validator.py:38 intentionally keeps self_review out of _SIDE_EFFECTING_KINDS, but the pipeline does post a comment via post_self_review_comment after ensure_pr (pipeline.py:1172), and post_hooks.py:395 always creates a new comment with no marker or upsert. On resume/retry that duplicates comments, and since the post lives outside the step, its failure sidesteps on_failure. Modeling commenting as an explicit side-effecting step with an idempotency key (or a hidden dedupe marker) would tidy this up.

🔴 Branch is CONFLICTING, and the rebase will need some care

A merge test conflicts in agent/src/pipeline.py and agent/src/prompts/__init__.py (~16 commits behind). It's a bit more than mechanical: main has since added clarification/needs-input holds, artifact-delivery workflows, and agent-branch reconciliation. Self-review will want to run after branch reconciliation + the safety commit and be skipped for needs_input / artifact-delivery tasks, otherwise it could review the wrong branch or spin up a second loop on a task that should be held. Flagging this as a rebase consideration rather than a confirmed defect, since that code isn't on this branch yet — and a mise run build afterward.

🟠 Still open from @isadeks #5 — policy engine re-inits from task-start values

run_agent rebuilds PolicyEngine from config.initial_approval_gate_count/initial_approvals, frozen at task start (runner.py:238-284), so the review loop starts with a fresh gate budget and loses approvals granted during implement. It's narrower now the critic is read-only, but a Bash soft-deny (e.g. a push gate) could still re-prompt for already-approved scopes on gated workflows.

🟠 Minor hardening — content in the comment is attacker-influenceable

post_self_review_comment looks safe from command injection (list-form subprocess, no shell=True). The one thing worth guarding: it posts summary text under a trusted ## 🔍 Self-Review Summary header from the bot identity, so injected content could render fake "no issues found" text, @mentions, or links as if from the review bot. A fenced code block around the body plus a short "generated from model output over the diff" disclaimer would neutralize that.

Prior findings that still look open (not re-argued here)

@isadeks' #3 (summary not gitignored / cleanup runs post-push), #4 (inert on_failure), #7 (review runs even when implement hard-errored), #8 (unscoped max_turns/prompt, global maximum: 20), #9 ({diff} documented-but-not-enforced), #10 (triplicated default 5), and @theagenticguy's test-integrity cluster all still appear unaddressed on this HEAD. I re-verified the test cluster via mutation testing: the turn-budget tests at test_self_review.py:311 have no assertions (swapping min→max stays green; the suite also emits unawaited-coroutine warnings), the positive remaining-budget path is unexercised, and safety-net commit #2 is unguarded.

One governance note

Commit af99d56f implements #536, which hasn't carried the approved label and is unassigned — worth squaring with ADR-003 before merge. Also, #262 was approved describing a critic that iterates on fixes, whereas the shipped design landed as read-only/comment-only; might be worth a quick note on the issue to reconcile that scope change.

Really solid foundation here overall — once the read-only surface and the ADR-014 question are sorted, this is a genuinely valuable addition. Verdict: request changes for now.

[scottschreckengaust]

Resolve conflicts