Implement Phase 2: specialized review modes - #152
Conversation
|
@james-in-a-box get this pr up to date with main and fix merge conflicts |
Rebased onto main and resolved merge conflicts.
e3bfc6d to
7d62fd8
Compare
|
Rebased the branch onto main and resolved merge conflicts in 8 files:
The PR now includes both this branch's Phase 2 specialized review modes and the fixes that were merged to main since (PR #150). — Authored by egg |
|
egg finished successfully. View run logs |
|
@james-in-a-box fix lint checks on this pr |
Remove unused variables MAX_DIFF_SUMMARY_CHARS and head_sha that were flagged by shellcheck SC2034. Authored-by: egg
|
Fixed the shellcheck lint warnings in
Both were flagged by shellcheck SC2034. — Authored by egg |
|
egg finished successfully. View run logs |
|
@james-in-a-box let's change this slightly. Default to having bots 1-3 run. |
On PR open/update, the three specialized review modes now run in parallel by default instead of just a standard review. Deep review remains manually triggered only. Specific modes can be run individually via workflow_dispatch by selecting a mode from the dropdown. Authored-by: egg
|
Updated the workflow to run bots 1-3 by default on every PR: Changes:
How it works:
— Authored by egg |
|
egg finished successfully. View run logs |
|
@james-in-a-box fix lint issues on this pr |
- Replace empty string option with 'all' sentinel value (actionlint doesn't allow empty strings in choice options) - Break long matrix expression into multi-line YAML block scalar to stay under 120 char line limit
|
egg finished successfully. View run logs |
![james-in-a-box[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Phase 2 Specialized Review Modes
Overall this PR implements the Phase 2 requirements well. The specialized prompt templates are comprehensive and the workflow structure is sound. I found a few issues worth addressing:
Issues
1. Plan mode runs unnecessarily on every PR (.github/workflows/on-pull-request.yml)
The default matrix includes plan mode:
|| fromJson('["security", "plan", "outsider"]')But according to the plan document, plan verification should only auto-trigger "when PR description links to an issue or JIRA ticket." Running plan mode without a LINKED_ISSUE produces a review with empty linked_content, which is wasteful and potentially confusing to PR authors.
Suggestion: Either:
- Remove
planfrom the default matrix and make it workflow_dispatch-only - Or add logic to detect linked issues from PR description (parse
#123or JIRA links) and only include plan mode when found
2. Security pattern lists are inconsistent (build-review-prompt.sh:29-46 vs build-deep-review-prompt.sh:237)
build-review-prompt.sh uses SECURITY_PATTERNS array with patterns like login, permission, role, ci/, .github/.
build-deep-review-prompt.sh uses an inline regex:
if [[ "$filename" =~ (auth|middleware|security|password|token|secret|cred|api|endpoint|route|handler|docker|workflow|yml|yaml) ]]; thenThese don't match. Consider extracting to a shared patterns file or at minimum synchronizing them.
3. INPUT_REVIEW_MODE appears unused (action/action.yml)
The new review-mode input is defined and passed to entrypoint as INPUT_REVIEW_MODE, but the workflow doesn't pass this input to the action. The mode is instead passed via REVIEW_MODE env var to the prompt builder scripts. Either:
- Remove the unused input from action.yml
- Or document when/how this input should be used
Minor Observations
- The prompt templates are well-structured with clear output formats
- The 30-minute timeout for deep review and 10-minute default is appropriate
- The trusted checkout security pattern is correctly maintained
- Good use of
fail-fast: falseto allow parallel review modes to complete independently
Test Plan Verification
The implementation satisfies the test plan items:
- ✅ Security review prompt loads via
load_specialized_prompt "security" - ✅ Plan verification mode fetches linked issue via
fetch_issue_content - ✅ Outsider mode excludes review rules (
if [[ "$review_mode" != "outsider" ]]) - ✅ Deep review mode uses 30-minute timeout (outputs
timeout=30) - ✅ Workflow dispatch with each mode works via matrix expression
— Authored by egg
|
@james-in-a-box let's also include the 'deep review" by default. This will end up being the "general reviewer". Also go ahead and pull in main. |
|
Added deep review to the default review modes matrix. All four review modes (security, plan, outsider, deep) now run in parallel automatically on PR open/update. Also merged in latest changes from main. Changes:
— Authored by egg |
|
egg finished successfully. View run logs |
There was a problem hiding this comment.
AI Code Review
Well-implemented specialized review modes with good prompt templates and workflow integration. Main concerns: (1) Default behavior runs 4 parallel reviews per PR which is costly and noisy, contradicting the single-agent design principle; (2) Plan mode runs in the default matrix but requires manual LINKED_ISSUE input to be useful; (3) Deep review has conflicting instructions about whether to post comments directly or output JSON.
This is an automated review. Please evaluate suggestions carefully.
— Authored by egg
|
see #161. We're going to rework the strategy here. |
1 participant
Summary
Implements Phase 2 of the AI-powered code review bots plan (#134, PR #140).
This PR adds four specialized review modes:
Security-focused review (
action/prompts/security-review.md)Plan verification review (
action/prompts/plan-verify.md)linked_issueinputOutsider review (
action/prompts/outsider-review.md)Deep review mode (
action/build-deep-review-prompt.sh)Changed Files
action/prompts/security-review.md— Security-focused review promptaction/prompts/plan-verify.md— Plan verification review promptaction/prompts/outsider-review.md— Outsider/clarity review promptaction/build-deep-review-prompt.sh— Deep review prompt builderaction/build-review-prompt.sh— Updated for mode detection and specialized prompts.github/workflows/on-pull-request.yml— Added workflow_dispatch inputs for modesaction/action.yml— Addedreview-modeinput parameterdocs/plans/ai-code-review-bots-plan.md— Updated status to mark Phase 2 completeHow to Use
Standard Review (automatic)
PRs are automatically reviewed on open/update. Security mode auto-triggers if 3+ security-sensitive files change.
Manual Review Modes
Use the workflow_dispatch trigger with:
review_mode:security,plan,outsider, ordeeplinked_issue: Issue number (for plan verification mode)Example via CLI:
gh workflow run "egg: Code Review" -f pr_number=123 -f review_mode=securityIssue: #134
Related: PR #140, PR #146
Test plan
Authored-by: egg