Sync SDLC reviewer conventions with PR reviewer

action/build-review-prompt.sh

@@ -54,6 +54,7 @@ fetch_review_rules() {
 - Null/undefined handling, missing error paths
 - Resource leaks (connections, file handles, memory)
 - Incorrect algorithm complexity for data size
+- End-to-end feature functionality: verify new features work in their real execution environment
 
 **Robustness**:
 - Missing input validation at trust boundaries
@@ -73,8 +74,30 @@ fetch_review_rules() {
 1. **Examine every changed file systematically**. Do not skim.
 2. **Read surrounding context**—check how changed code integrates with the rest of the codebase. Use file reads and grep liberally.
 3. **Trace data flow** from input to output, especially for security-sensitive paths.
-4. **Consider edge cases** the author may not have tested.
-5. **Research when uncertain**—look up library behavior, check documentation, verify assumptions.
+4. **Verify end-to-end functionality**: For new features, trace the complete execution path in the real deployment environment. Check that config files, environment variables, and dependencies are actually available where the code runs. A feature that reads config from a path that doesn't exist in its runtime environment is non-functional, not just suboptimal.
+5. **Consider edge cases** the author may not have tested.
+6. **Research when uncertain**—look up library behavior, check documentation, verify assumptions.
+
+### Severity Classification
+
+**Blocking** (request changes):
+- Security vulnerabilities
+- Non-functional features — the feature's core purpose does not work end-to-end
+- Logic errors that produce incorrect results
+- Breaking changes to existing functionality
+- Resource leaks or crashes
+- Pre-existing broken or inconsistent behavior in code the PR modifies
+
+**Non-blocking** (suggestions):
+- Code quality improvements (naming, structure, duplication)
+- Defense-in-depth additions
+- Missing edge case handling that doesn't affect the core feature
+- Documentation gaps
+- Style or convention deviations not caught by linters
+
+**Do not dismiss issues as "not a regression"**: If a PR modifies code that has existing broken or inconsistent behavior, the issue is blocking even if the PR didn't introduce it. A PR that adds a new code path through already-inconsistent logic makes the inconsistency worse.
+
+**Beware of false analogies**: When comparing new code to existing patterns, verify the analogy holds at the execution-model level. Two features may look structurally similar in config but have completely different execution paths. If the existing pattern works via mechanism A but the new code relies on mechanism B that doesn't exist, the comparison is invalid — classify based on actual functionality, not superficial similarity.
 
 ### Skip
 

action/review-conventions.md

@@ -30,10 +30,14 @@ shell escaping issues. Always write to a file first, then use `--body-file`.
 
 ## When to Approve vs Request Changes
 
-- **Request changes**: Security vulnerabilities, logic errors, correctness issues, missing error handling, resource leaks, breaking changes, violations of codebase patterns. When in doubt, request changes.
+- **Request changes**: Security vulnerabilities, logic errors, correctness issues, non-functional features (core purpose doesn't work end-to-end), missing error handling, resource leaks, breaking changes, violations of codebase patterns. When in doubt, request changes.
 - **Approve**: No blocking issues found after thorough review. Minor advisory suggestions are fine to include. If you include non-blocking suggestions, add `<!-- has-suggestions -->` anywhere in the review body so the feedback-addressing workflow picks them up. The marker is position-independent—it can appear at the beginning, middle, or end of the review body.
 - **Comment**: Non-blocking suggestions, questions, ideas for future improvement.
 
+**Key distinction**: A feature that doesn't work is a correctness issue, not a style issue. If the feature's core functionality is broken — not just degraded or missing edge cases — always request changes, even if the code structure looks reasonable or matches an existing pattern.
+
+**Pre-existing issues are still blocking**: If a PR modifies code that already has broken or inconsistent behavior, request changes to fix it — do not dismiss it as "not a regression." The PR is already in the area, making it the natural place to fix the issue. A PR that adds new code paths through already-broken logic makes the problem worse.
+
 ## Self-Authored PRs
 
 When reviewing a PR authored by the same bot account, **use `--comment` instead of

docs/guides/github-automation.md

@@ -34,6 +34,8 @@ Review criteria for each workflow are defined in `shared/prompts/` as markdown f
 
 Repositories can override criteria by placing a custom file in `.egg/` (e.g., `.egg/review-rules.md` overrides code review criteria, `.egg/onboarding-rules.md` overrides onboarding documentation rules).
 
+**Keeping reviewers in sync**: The PR reviewer (GitHub Action) and SDLC reviewer (orchestrator) share criteria files but have separate inline fallbacks and conventions. See [`shared/prompts/REVIEWER-SYNC.md`](../../shared/prompts/REVIEWER-SYNC.md) for the sync guide and modification checklist.
+
 ## AI Code Review
 
 **Workflow:** [`.github/workflows/on-pull-request.yml`](../../.github/workflows/on-pull-request.yml)

docs/index.md

@@ -98,6 +98,7 @@ Each major component has detailed documentation:
 | **GitHub Action setup** | [ADR: GitHub Actions](adr/in-progress/ADR-GitHub-Actions-Support.md) | [Architecture Overview](architecture/README.md) |
 | **Adding tests** | [Contributing](../CONTRIBUTING.md) | [Project Structure](development/STRUCTURE.md) |
 | **Setting up GitHub automation** | [GitHub Automation](guides/github-automation.md) | [Agent-Mode Design](guides/agent-mode-design.md), [GitHub Action](../action/README.md) |
+| **Modifying review criteria** | [Reviewer Sync Guide](../shared/prompts/REVIEWER-SYNC.md) | [GitHub Automation](guides/github-automation.md), [Code Review Criteria](../shared/prompts/code-review-criteria.md) |
 | **Using workflows in external repos** | [Reusable Workflows](guides/reusable-workflows.md) | [GitHub Automation](guides/github-automation.md), [GitHub Action](../action/README.md) |
 | **Designing agent workflows** | [Agent-Mode Design](guides/agent-mode-design.md) | [ADR: Autonomous SE](adr/in-progress/ADR-Autonomous-Software-Engineer.md) |
 | **Adding bot workflows** | [Agent-Mode Design](guides/agent-mode-design.md) | [Action README](../action/README.md), existing workflows in `.github/workflows/` |

orchestrator/routes/pipelines.py

@@ -926,7 +926,9 @@ def _get_code_review_criteria(repo_path: str | None = None) -> str:
         "- Logic errors, off-by-one, boundary conditions\n"
         "- Race conditions, deadlocks, concurrency bugs\n"
         "- Null/undefined handling, missing error paths\n"
-        "- Resource leaks (connections, file handles, memory)\n\n"
+        "- Resource leaks (connections, file handles, memory)\n"
+        "- End-to-end feature functionality: verify new features work in their "
+        "real execution environment\n\n"
         "### Robustness\n"
         "- Missing input validation at trust boundaries\n"
         "- Unhandled exceptions that could crash the system\n"
@@ -935,7 +937,35 @@ def _get_code_review_criteria(repo_path: str | None = None) -> str:
         "### Design\n"
         "- Violations of existing codebase patterns\n"
         "- Breaking changes to public interfaces\n"
-        "- Tight coupling that will hinder future changes\n"
+        "- Tight coupling that will hinder future changes\n\n"
+        "### Severity Classification\n\n"
+        "**Blocking** (request changes):\n"
+        "- Security vulnerabilities\n"
+        "- Non-functional features — the feature's core purpose does not work "
+        "end-to-end\n"
+        "- Logic errors that produce incorrect results\n"
+        "- Breaking changes to existing functionality\n"
+        "- Resource leaks or crashes\n"
+        "- Pre-existing broken or inconsistent behavior in code the PR "
+        "modifies\n\n"
+        "**Non-blocking** (suggestions):\n"
+        "- Code quality improvements (naming, structure, duplication)\n"
+        "- Defense-in-depth additions\n"
+        "- Missing edge case handling that doesn't affect the core feature\n"
+        "- Documentation gaps\n"
+        "- Style or convention deviations not caught by linters\n\n"
+        "**Do not dismiss issues as 'not a regression'**: If a PR modifies "
+        "code that has existing broken or inconsistent behavior, the issue is "
+        "blocking even if the PR didn't introduce it. A PR that adds a new "
+        "code path through already-inconsistent logic makes the inconsistency "
+        "worse.\n\n"
+        "**Beware of false analogies**: When comparing new code to existing "
+        "patterns, verify the analogy holds at the execution-model level. "
+        "Two features may look structurally similar in config but have "
+        "completely different execution paths. If the existing pattern works "
+        "via mechanism A but the new code relies on mechanism B that doesn't "
+        "exist, the comparison is invalid — classify based on actual "
+        "functionality, not superficial similarity.\n"
     )
 
 
@@ -1590,11 +1620,16 @@ def _build_review_prompt(
         lines.append(
             "4. Trace data flow from input to output, especially for security-sensitive paths"
         )
-        lines.append("5. Research when uncertain — look up library behavior, check documentation")
-        lines.append("6. Consider edge cases the author may not have tested")
-        lines.append("7. Evaluate against the criteria below")
-        lines.append(f"8. Write your verdict to `{verdict_path}` as JSON")
-        lines.append("9. Commit the verdict file")
+        lines.append(
+            "5. Verify end-to-end functionality — for new features, trace the complete "
+            "execution path in the real deployment environment. Check that config files, "
+            "environment variables, and dependencies are actually available where the code runs"
+        )
+        lines.append("6. Research when uncertain — look up library behavior, check documentation")
+        lines.append("7. Consider edge cases the author may not have tested")
+        lines.append("8. Evaluate against the criteria below")
+        lines.append(f"9. Write your verdict to `{verdict_path}` as JSON")
+        lines.append("10. Commit the verdict file")
     elif draft_path:
         # Expanded procedural steps for draft-based (non-code) reviewers
         lines.append("2. Read the draft thoroughly — do not skim")
@@ -1649,6 +1684,34 @@ def _build_review_prompt(
     )
     lines.append("")
 
+    # Verdict classification — aligned with PR reviewer's review-conventions.md
+    lines.append("### When to Use `needs_revision` vs `approved`\n")
+    lines.append(
+        "**Use `needs_revision` for**: Security vulnerabilities, logic errors, correctness "
+        "issues, non-functional features (core purpose doesn't work end-to-end), missing "
+        "error handling, resource leaks, breaking changes, violations of codebase patterns. "
+        "When in doubt, use `needs_revision`."
+    )
+    lines.append(
+        "**Use `approved` for**: No blocking issues found after thorough review. "
+        "Non-blocking suggestions belong in the `suggestions` field."
+    )
+    lines.append("")
+    lines.append(
+        "**Key distinction**: A feature that doesn't work is a correctness issue, not a "
+        "style issue. If the feature's core functionality is broken — not just degraded or "
+        "missing edge cases — always use `needs_revision`, even if the code structure looks "
+        "reasonable or matches an existing pattern."
+    )
+    lines.append(
+        "**Pre-existing issues are still blocking**: If the code being reviewed modifies "
+        "areas with existing broken or inconsistent behavior, use `needs_revision` — do not "
+        'dismiss it as "not a regression." The code is already being changed in that area, '
+        "making it the natural place to fix the issue. Code that adds new paths through "
+        "already-broken logic makes the problem worse."
+    )
+    lines.append("")
+
     # Delta review directive for re-reviews
     if is_delta_review:
         lines.append("## Delta Review\n")

shared/prompts/REVIEWER-SYNC.md

@@ -0,0 +1,66 @@
+# Reviewer Sync Guide
+
+Two surfaces run code reviews with the same criteria and standards. When modifying
+one, update the other. The only differences between them should be what's required
+by their different workflows.
+
+## The Two Reviewers
+
+| Aspect | PR Reviewer (GitHub Action) | SDLC Reviewer (Orchestrator) |
+|--------|----------------------------|------------------------------|
+| **Location** | `action/build-review-prompt.sh` + `action/review-conventions.md` | `orchestrator/routes/pipelines.py` (`_build_review_prompt()`) |
+| **Trigger** | PR opened/updated via GitHub Actions | SDLC pipeline review phase |
+| **Output** | Posts `gh pr review` (approve / request-changes / comment) | Writes JSON verdict to `.egg-state/reviews/` (approved / needs_revision) |
+| **Conventions** | External file: `action/review-conventions.md` | Inline in `_build_review_prompt()` |
+| **Reviewer types** | Code only | Code, contract, agent-design, refine, plan |
+
+## What's Shared (single source of truth)
+
+Both reviewers read from the same files in `shared/prompts/`:
+
+- `code-review-criteria.md` — security, correctness, robustness, design, severity classification
+- `contract-review-criteria.md` — task/contract verification
+- `agent-design-criteria.md` — agent-mode anti-patterns
+
+Each reviewer has an inline fallback for when the shared file can't be loaded.
+**Inline fallbacks must match the shared file content.**
+
+## What's Intentionally Different
+
+These differences exist because the workflows are different — not because the
+review standards differ:
+
+1. **Verdict format**: PR reviewer uses GitHub review actions (approve / request-changes).
+   SDLC reviewer writes a structured JSON verdict (approved / needs_revision).
+2. **Posting mechanism**: PR reviewer uses `gh pr review --body-file`.
+   SDLC reviewer commits a verdict file.
+3. **Reviewer types**: PR reviewer only does code review. SDLC reviewer also handles
+   contract, agent-design, refine, and plan reviews.
+4. **Self-authored PR handling**: PR reviewer downgrades to `--comment` for self-authored
+   PRs (GitHub restriction). Not applicable to SDLC reviewer.
+5. **Scope preambles**: SDLC reviewer has per-type scope preambles. PR reviewer's scope
+   is implicit in the prompt structure.
+
+## What Must Stay Aligned
+
+When updating review behavior, ensure both surfaces reflect the change:
+
+| Concept | PR Reviewer Location | SDLC Reviewer Location |
+|---------|---------------------|------------------------|
+| Review criteria | `shared/prompts/code-review-criteria.md` | Same file (shared) |
+| Inline fallback criteria | `action/build-review-prompt.sh` `fetch_review_rules()` | `orchestrator/routes/pipelines.py` `_get_code_review_criteria()` |
+| Quality standards (be comprehensive, specific, etc.) | `action/review-conventions.md` "Comment Quality" section | `_build_review_prompt()` inline conventions |
+| Verdict classification (what's blocking vs non-blocking) | `action/review-conventions.md` "When to Approve vs Request Changes" | `_build_review_prompt()` "When to Use needs_revision vs approved" |
+| Procedural review steps | `action/build-review-prompt.sh` "How to Proceed" / inline fallback "How to Review" | `_build_review_prompt()` procedural steps for code reviewer |
+| Severity classification | `shared/prompts/code-review-criteria.md` (shared) | Same file (shared) |
+
+## Modification Checklist
+
+When changing review criteria or conventions:
+
+- [ ] Update `shared/prompts/code-review-criteria.md` (if changing shared criteria)
+- [ ] Update the inline fallback in `action/build-review-prompt.sh` `fetch_review_rules()`
+- [ ] Update the inline fallback in `orchestrator/routes/pipelines.py` `_get_code_review_criteria()`
+- [ ] Update `action/review-conventions.md` (if changing conventions/verdict guidance)
+- [ ] Update `_build_review_prompt()` inline conventions (if changing conventions/verdict guidance)
+- [ ] Verify the procedural review steps match between both surfaces

shared/prompts/code-review-criteria.md

@@ -20,6 +20,7 @@
 - Null/undefined handling, missing error paths
 - Resource leaks (connections, file handles, memory)
 - Incorrect algorithm complexity for data size
+- **End-to-end feature functionality**: For new features, verify the feature actually works in its real execution environment, not just that the code is well-structured. Trace the full path from trigger to effect. If a feature's core functionality is broken (e.g., config is read at build time but only available at runtime), that is a blocking correctness issue regardless of code quality.
 
 **Robustness**:
 - Missing input validation at trust boundaries
@@ -48,8 +49,30 @@
 1. **Examine every changed file systematically**. Do not skim.
 2. **Read surrounding context**—check how changed code integrates with the rest of the codebase. Use file reads and grep liberally.
 3. **Trace data flow** from input to output, especially for security-sensitive paths.
-4. **Consider edge cases** the author may not have tested.
-5. **Research when uncertain**—look up library behavior, check documentation, verify assumptions.
+4. **Verify end-to-end functionality**: For new features, trace the complete execution path in the real deployment environment. Check that config files, environment variables, and dependencies are actually available where the code runs. A feature that reads config from a path that doesn't exist in its runtime environment is non-functional, not just suboptimal.
+5. **Consider edge cases** the author may not have tested.
+6. **Research when uncertain**—look up library behavior, check documentation, verify assumptions.
+
+### Severity Classification
+
+**Blocking** (request changes):
+- Security vulnerabilities
+- Non-functional features — the feature's core purpose does not work end-to-end
+- Logic errors that produce incorrect results
+- Breaking changes to existing functionality
+- Resource leaks or crashes
+- Pre-existing broken or inconsistent behavior in code the PR modifies — if the PR touches code that already has bugs, incorrect behavior, or inconsistencies (e.g., different code paths producing different results for the same input), request changes to fix it. The PR is already in the area; this is the right time.
+
+**Non-blocking** (suggestions):
+- Code quality improvements (naming, structure, duplication)
+- Defense-in-depth additions
+- Missing edge case handling that doesn't affect the core feature
+- Documentation gaps
+- Style or convention deviations not caught by linters
+
+**Do not dismiss issues as "not a regression"**: If a PR modifies code that has existing broken or inconsistent behavior, the issue is blocking even if the PR didn't introduce it. A PR that adds a new code path through already-inconsistent logic makes the inconsistency worse — it's not acceptable to ship it just because the bug was there before. The fact that the PR is already changing this code makes it the natural place to fix it.
+
+**Beware of false analogies**: When comparing new code to existing patterns, verify the analogy holds at the execution-model level. Two features may look structurally similar in config but have completely different execution paths. If the existing pattern works via mechanism A but the new code relies on mechanism B that doesn't exist, the comparison is invalid — classify based on actual functionality, not superficial similarity.
 
 ### Skip