[AISOS-741] Add auto review primitive to Forge

[JoshSalomon]

Summary

This PR implements the Auto-Review Primitive, a new capability that enables automated quality gates for skill execution in Forge. When a skill includes a review.md file, the system automatically spawns a reviewer agent after execution to validate output quality, providing structured feedback and retry loops until the work is approved or max retries are exhausted. This feature reduces manual review burden and catches quality issues earlier in the development cycle.

Changes

Container Review Loop Engine (containers/review.py)

• Created Verdict enum (APPROVED/REJECTED) and ReviewConfig/ReviewCycleData dataclasses
• Implemented parse_review_config() for parsing YAML frontmatter from review.md files
• Implemented detect_review_md() with project override precedence (mirrors skill resolver pattern)
• Implemented parse_verdict() for case-insensitive APPROVED/REJECTED marker extraction
• Implemented write_cycle_file() for JSON output to .forge/{step-name}/review_cycle_N.json

Container Entrypoint Integration (containers/entrypoint.py)

• Added run_review_loop() orchestrating the reviewer agent spawn, verdict parsing, and retry logic
• Added run_reviewer_agent() and run_worker_with_feedback() for feedback injection

Observability Infrastructure (src/forge/observability/)

• Created ReviewCyclePoller class with async polling loop for detecting review cycle files during container execution
• Created ReviewCycleRecorder with mode switching (log/copy/None) for recording detected cycles
• Added Prometheus metrics: forge_review_cycles_total, forge_review_verdicts_total, forge_review_duration_seconds

Container Runner Integration (src/forge/sandbox/runner.py)

• Extended ContainerResult dataclass with review_cycles field
• Added step_name parameter to ContainerRunner.run() method
• Implemented _poll_review_cycles() background task for async polling during execution
• Implemented _sweep_review_cycles() for post-execution sync sweep to catch missed files

Workflow Node Updates

• Updated all ContainerRunner-invoking nodes to pass step_name parameter:
  • implementation.py: step_name='implement_task'
  • local_reviewer.py: step_name='local_review'
  • ci_evaluator.py: step_name='analyze_ci'/'fix_ci'
  • code_review.py: step_name='code_review'
  • docs_updater.py: step_name='update_docs'
  • implement_review.py: step_name='implement_review_analyze'/'implement_review_fix'
  • plan_bug_fix.py: step_name='plan_bug_fix'
  • rca_analysis.py: step_name='analyze_bug'/'reflect_rca'
  • rebase.py: step_name='rebase'

Configuration (src/forge/config.py)

• Added AUTO_REVIEW_MAX_RETRIES: default max retry attempts (default: 3)
• Added AUTO_REVIEW_POLL_INTERVAL: polling interval in seconds (default: 5)
• Added AUTO_REVIEW_RECORD_POLLED_FILES: recording mode ('log'/'copy'/None)

Sample Review Files

• Created skills/default/implement-task/review.md with generic code review criteria
• Created skills/default/implement-task/review.md with a 5-dimension deep review (simplicity, correctness, conventions, over-engineering, task-alignment)

Documentation

• Created docs/reference/review-md-schema.md with formal schema specification
• Created docs/guide/auto-review.md with comprehensive user documentation
• Updated docs/skills/authoring.md, docs/skills/index.md, docs/skills/defaults.md to reference auto-review
• Updated docs/reference/config.md with new configuration settings

Implementation Notes

• Precedence Logic: detect_review_md() mirrors the skill resolver pattern—project-specific overrides (skills/{project}/{skill}/review.md) take precedence over defaults (skills/default/{skill}/review.md)
• Verdict Parsing: Uses position-based precedence when both APPROVED and REJECTED markers are present (first occurrence wins)
• Async + Sync Polling: The runner uses async polling during execution plus a synchronous post-execution sweep to ensure no files are missed on fast container exits
• Graceful Degradation: Malformed YAML in review.md logs a warning and falls back to defaults (per BR-006)

Testing

• Added 200+ unit tests across new modules:
  • tests/unit/containers/test_review.py: 57 tests for review.py functions
  • tests/unit/observability/test_review_poller.py: 32 tests for async polling
  • tests/unit/observability/test_review_recorder.py: 34 tests for recording modes
  • tests/unit/sandbox/test_runner_review_polling.py: 25+ tests for runner integration
• All 1741 unit tests pass

Related Tickets

• AISOS-741
• AISOS-2053
• AISOS-2054
• AISOS-2055
• AISOS-2056
• AISOS-2057
• AISOS-2058
• AISOS-2059
• AISOS-2060
• AISOS-2061
• AISOS-2062
• AISOS-2063
• AISOS-2064
• AISOS-2065
• AISOS-2066
• AISOS-2067
• AISOS-2068
• AISOS-2069
• AISOS-2070
• AISOS-2071

---

Generated by Forge SDLC Orchestrator

[eshulman2]

AI review disclosure: This is an initial automated review generated by OpenAI Codex. It is intended as an early signal for human reviewers, not a substitute for final human review.

I found two blocking runtime retry-path issues:

1. P1 — Failed corrective worker runs are reported as success. If a rejected review triggers run_worker_with_feedback() and that worker fails, the loop breaks but still returns True. The container can therefore exit successfully with partial changes. Because the last cycle is below max_cycles, review_exhausted is also false, so no warning reaches the PR. See containers/entrypoint.py lines 871–878 and src/forge/sandbox/runner.py lines 87–92.

2. P1 — Retries can consume stale review files from earlier executions. Review artifacts use the stable {task_key}__{skill_name} directory, but it is never cleared or generation-scoped before polling begins. On a workflow retry, the poller can read old files and mark their paths processed; when the current container overwrites those filenames, the new results are ignored. Metrics, exhaustion state, and PR notes may consequently describe the previous run. See src/forge/sandbox/runner.py lines 720–766 and src/forge/observability/review_poller.py lines 230–244.

CI is green, but these retry-path defects should be addressed before merge.

[eshulman2]

AI review disclosure: This is a follow-up to the initial automated review generated by OpenAI Codex. These are additional early findings for human reviewers, not a substitute for final human review.

Additional findings:

1. P2 — Multiple exhaustion reports produced within one graph node can overwrite each other. merge_review_exhaustion() replaces the local review_exhaustion_report with {key: data}. The operator.or_ reducer preserves reports returned by separate graph nodes, but it cannot recover an earlier report overwritten locally when one node invokes the helper multiple times—for example, CI analysis followed by CI fixing. Merge the new entry into state.get("review_exhaustion_report", {}) before returning. See src/forge/workflow/utils/review_report.py lines 46–60 and the two calls in src/forge/workflow/nodes/ci_evaluator.py.

2. P2 — Post-change review exhaustion is dropped instead of propagated. run_post_change_review() explicitly logs that exhaustion data is not propagated to workflow state. Consequently, auto-review exhaustion after CI or implementation fixes can be absent from the PR Auto-Review Notes, despite the generic exhaustion-reporting goal. See src/forge/workflow/nodes/code_review.py lines 74–78 and its callers in ci_evaluator.py and implement_review.py.

3. P3 — The documented/configured copy recording mode is never honored. Settings accept AUTO_REVIEW_RECORD_POLLED_FILES=copy, but _start_review_polling() always downgrades it to log because no recording directory is configured. Either implement the advertised mode or remove it from the accepted configuration until supported. See src/forge/config.py lines 338–343 and src/forge/sandbox/runner.py lines 542–552.

4. Documentation cleanup: The PR description lists ReviewJiraNotifier, test_review_notifier.py, and the aiofiles dependency, but these are not present in the final diff. Please update the description so reviewers and release notes reflect the actual implementation.

[JoshSalomon]

Thank you for the thorough automated review. Here is my response to each finding:

P1 — Blocking issues (both fixed)

P1.1 — Worker retry failure reported as success:
Fixed. When run_worker_with_feedback() returns False mid-loop, we now write a synthetic final cycle file with cycle=max_retries, max_cycles=max_retries — this ensures review_exhausted triggers on the orchestrator side and the failure surfaces in the PR body. Commit: fix: address max-effort code review findings.

P1.2 — Stale review files consumed on retry:
Fixed. In _start_review_polling(), we now clear any existing review_cycle_*.json files from the cycle directory before creating the poller. This prevents the new run's poller from marking stale filenames as processed, which previously caused new results with the same filenames to be silently dropped. Two new TDD tests cover both the clearing behavior and the end-to-end scenario where new run data replaces old. Commit: fix: address max-effort code review findings.

P2 — Additional findings

P2.1 — Multiple exhaustion reports overwrite each other within one node:
Fixed. merge_review_exhaustion() now merges into the existing state.get('review_exhaustion_report') or {} instead of replacing it with {key: data}. The operator.or_ LangGraph reducer handles cross-node merges, but same-node calls (e.g., ci_evaluator calling it for both analyze_ci and fix_ci) need local accumulation too. New TDD test confirms both entries survive. Commit: fix: address max-effort code review findings.

P2.2 — Post-change review exhaustion dropped:
Fixed. run_post_change_review() now returns tuple[bool, ContainerResult | None] instead of bool. Both callers (ci_evaluator and implement_review) unpack the result and call merge_review_exhaustion() on the ContainerResult. New TDD test verifies the ContainerResult is returned and accessible for exhaustion propagation. Commit: fix: address max-effort code review findings.

P2.3 — copy recording mode never honored:
Acknowledged. AUTO_REVIEW_RECORD_POLLED_FILES=copy is accepted by config but currently downgraded to log at runtime because no recording_dir is wired up. We have added a logger.warning so this is no longer a silent no-op (users will see the downgrade in logs). Full implementation would require a new FORGE_REVIEW_RECORDING_DIR setting and threading it through _start_review_polling() → ReviewCycleRecorder. We are deferring this to a follow-up PR as it is a non-blocking observability feature, not a correctness issue. We will remove the deceptive config option from the docs for now if that is preferred, or leave it with the warning as a forward-compatibility hook.

P2.4 — PR description references removed components:
Fixed. Updated the PR description to remove references to ReviewJiraNotifier, test_review_notifier.py, and the aiofiles dependency — all of which were removed during the refactoring phase.

---

All P1 and P2 issues except P2.3 are addressed in the latest push. Happy to discuss any of the fixes further.