Skip to content
This repository was archived by the owner on May 24, 2026. It is now read-only.

feat: expert review loop — replace self-review with full adversarial review + fix loop#785

Merged
PureWeen merged 1 commit into
mainfrom
feat/review-fix-loop
Apr 28, 2026
Merged

feat: expert review loop — replace self-review with full adversarial review + fix loop#785
PureWeen merged 1 commit into
mainfrom
feat/review-fix-loop

Conversation

@PureWeen
Copy link
Copy Markdown
Owner

Summary

Replaces the lightweight self-review in agent-fix with the full expert review pipeline, and adds an automated fix loop.

Changes

  1. Rename review.agent.mdexpert-review.md — removes dots from the workflow name, fixing the CAPI 400 tool name validation error

  2. Update agent-fix — drops the inline 3-sub-agent self-review (Steps 6-7), replaces with dispatching expert-review alongside CI workflows

  3. New fix-review-findings.md workflow — reads expert review findings on a PR, fixes each one, runs tests, and re-dispatches expert-review. Loops up to 3 rounds until zero issues found.

Flow

graph LR
    A[agent-fix] -->|creates PR| B[expert-review]
    B -->|posts findings| C[fix-review-findings]
    C -->|pushes fixes| B
    C -->|round 3 or zero findings| D[Done ✅]
    A -->|dispatches| E[verify-build]
    A -->|dispatches| F[integration-tests]
Loading

…gs workflow

Three changes:

1. Rename review.agent.md → expert-review.md to avoid dots in
   tool names (CAPI rejects dots in ^[a-zA-Z0-9_-]{1,128}$).

2. Update agent-fix to dispatch expert-review instead of doing a
   lightweight self-review. The full 3-model adversarial review
   now runs on every agent-fix PR.

3. Add fix-review-findings.md workflow — reads expert review findings,
   fixes them, runs tests, and re-dispatches expert-review. Loops
   up to 3 rounds until zero issues are found.

Flow: agent-fix creates PR → dispatches expert-review → expert-review
posts findings → fix-review-findings reads findings, pushes fixes,
re-dispatches expert-review → repeat until clean.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Copy Markdown
Contributor

Expert Code Review — PR #785

Methodology: 3 independent reviewers with adversarial consensus, plus targeted follow-up verification for disputed findings.

Findings

# Severity Consensus File Line(s) Finding
1 🔴 CRITICAL 3/3 agent-fix.md#L203 203 Broken loop — no trigger path from expert-review to fix-review-findings. expert-review has no dispatch-workflow safe output; fix-review-findings only triggers on workflow_dispatch. The automated loop described in the PR and flow diagram is completely unwired.
2 🔴 CRITICAL 3/3 fix-review-findings.md#L20 20 contents: read prevents git push. The workflow instructs the agent to commit and push fixes, but permissions only grant contents: read. Every git push will fail with HTTP 403.
3 🔴 CRITICAL 3/3 (after follow-up) fix-review-findings.md#L6 6 No PR checkout for workflow_dispatch. The compiled lock file's checkout-pr condition is false for workflow_dispatch events. The agent operates on main instead of the PR branch — fixes land on the wrong branch. Missing Checkout-GhAwPr.ps1 step (compare with review-on-open).
4 🟡 MODERATE 3/3 (after follow-up) fix-review-findings.md 106–122 Round counter never increments. fix-review-findings dispatches expert-review without a round parameter, and expert-review cannot dispatch back with an incremented round. The round >= 3 stop condition is never reached — loop would run indefinitely (if the trigger were wired).
5 🟡 MODERATE 3/3 (after follow-up) fix-review-findings.lock.yml 74 Concurrency group not PR-scoped. Uses global gh-aw-${{ github.workflow }} group, serializing runs across different PRs. Compare with expert-review which uses expert-review-${{ inputs.pr_number }}.
6 🟢 MINOR 2/3 review-on-open.agent.md, review-retro.agent.md, review-shared.md 17, 80, 4 Stale file references. Updated comments reference expert-review.agent.md but the file was renamed to expert-review.md (no .agent.). Three files point to a non-existent filename.
7 🟢 MINOR 2/3 fix-review-findings.md 108 git add -A violates project convention. .github/copilot-instructions.md prohibits blind git add -A. Could stage build artifacts or .workflow-start-time.

Discarded Findings (1/3 — failed consensus, not in top-3 for follow-up)

  • Unexplained downgrade of review-on-open from v0.69.3 → v0.68.3 (Reviewer 2 only, 🟡)
  • noop defaults to report-as-issue: true in fix-review-findings (Reviewer 2 only, 🟡)
  • review-on-open concurrency group comment claim is inaccurate (Reviewer 1 only, 🟢)
  • agent-fix Step 6 falsely describes "automatic" pickup (Reviewer 2 only, 🟢)

CI Status

  • activation ✅ completed successfully
  • agent 🔄 in progress (this review workflow)
  • pre_activation ✅ completed successfully

Test Coverage Assessment

This PR modifies only gh-aw workflow files (.md and .lock.yml) — no application code or test files are changed. No unit test updates are needed. However, the new fix-review-findings workflow should be validated by a manual dispatch test run after the critical issues are fixed.

Summary

The rename from review.agent.mdexpert-review.md and the agent-fix simplification are clean. However, the new fix-review-findings workflow has 3 critical blockers that make it non-functional: the loop isn't wired (no dispatch path), it can't push (wrong permissions), and it doesn't checkout the PR branch. These must be fixed before merge.

Generated by Expert Code Review · 3 independent reviewers with adversarial consensus

Generated by Expert Code Review (auto) for issue #785 · ● 18.1M ·

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 3 critical findings posted inline. See full review summary in the comment below.

Generated by Expert Code Review (auto) for issue #785 · ● 18.1M

roles: [admin, maintainer, write]

permissions:
contents: read
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 CRITICAL — contents: read prevents git push (3/3 reviewers)

Steps 3–5 instruct the agent to edit files, commit, and git push to the PR branch. But permissions only grant contents: read. The compiled lock file confirms this on the agent job. Without contents: write, every git push fails with HTTP 403 — the entire fix-and-push workflow is non-functional.

Fix: Change to contents: write. If CI triggering is needed, also add github-token-for-extra-empty-commit: with a PAT/App token (per gh-aw guidelines, GITHUB_TOKEN pushes don't trigger CI).

description: "Reads expert review findings on a PR, fixes them, runs tests, and re-dispatches the expert review. Loops until zero issues."

on:
workflow_dispatch:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 CRITICAL — No PR checkout for workflow_dispatch (3/3 reviewers after follow-up)

The compiled lock file's checkout-pr step has if: github.event.pull_request || github.event.issue.pull_request — neither is true for workflow_dispatch. The agent operates on the default branch (main) instead of the PR's head branch. Fixes would land on the wrong branch or fail entirely.

Compare with review-on-open.agent.lock.yml which adds a separate "Checkout target PR (for workflow_dispatch)" step using Checkout-GhAwPr.ps1.

Fix: Add a pre-agent-steps: block to the frontmatter (or an explicit checkout step in the agent instructions) that runs Checkout-GhAwPr.ps1 when github.event_name == 'workflow_dispatch', then recompile.

@@ -223,21 +200,22 @@ dispatch_workflow({
})
```

## Step 9: Post Summary
The expert review runs a 3-model adversarial code review (Opus + Sonnet + GPT) on the PR and posts findings as review comments. If it finds issues, the **fix-review-findings** workflow will automatically pick them up, push fixes, and re-dispatch the expert review — looping until zero issues are found.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 CRITICAL — Broken loop: no automated trigger from expert-review to fix-review-findings (3/3 reviewers)

This line claims "the fix-review-findings workflow will automatically pick them up", but there is no automated trigger path. expert-review's safe-outputs (defined in shared/review-shared.md) include only add-comment, create-pull-request-review-comment, submit-pull-request-review, and noopno dispatch-workflow. Meanwhile, fix-review-findings only triggers on workflow_dispatch. The loop in the flow diagram (expert-review → fix-review-findings → expert-review) is completely unwired.

Fix: Either (a) add dispatch-workflow: { workflows: [fix-review-findings], max: 1 } to shared/review-shared.md's safe-outputs and recompile, or (b) reword this to clarify manual dispatch is required.

@PureWeen PureWeen merged commit 8a40a39 into main Apr 28, 2026
6 checks passed
@PureWeen PureWeen deleted the feat/review-fix-loop branch April 28, 2026 16:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant