Classify non-functional features as blocking in reviewer

[james-in-a-box]

Classify non-functional and pre-existing broken behavior as blocking

Non-functional features (where the core purpose doesn't work end-to-end) were
being classified as "non-blocking" by the reviewer. This happened in PR #951
where build_commands was flagged as a design gap but marked non-blocking
because it superficially matched the extra_packages pattern — despite having
a fundamentally different (and broken) execution model.

Similarly, in PR #960 the reviewer identified a display label inconsistency
("refine" vs "analysis" depending on code path) in code the PR was actively
modifying, but dismissed it as "not blocking" because it wasn't a regression.
The PR then added a new code path that further exposed the inconsistency.

Changes ensure both classes of issue are always treated as blocking:

1. shared/prompts/code-review-criteria.md — Added end-to-end feature
   functionality as an explicit correctness check. Added "Severity
   Classification" section with blocking vs non-blocking distinction. Added
   "pre-existing broken or inconsistent behavior in code the PR modifies" to
   the blocking list. Added explicit "do not dismiss as not a regression" rule.
   Warns against false analogies.

2. action/review-conventions.md — Added "non-functional features" and
   "pre-existing issues are still blocking" to the request-changes criteria.

3. action/build-review-prompt.sh — Updated the inline fallback criteria
   to match.

4. orchestrator/routes/pipelines.py — Updated the inline fallback
   criteria to match.

Closes #954

Test plan:

• Review the updated criteria files for clarity and completeness
• Verify the severity classification covers both scenarios (Add build_commands for per-repo dependency caching #951 and Fix HITL decisions missing phase and options #960)
• On next PR review, confirm the reviewer flags pre-existing broken behavior
  in modified code as blocking rather than dismissing it as "not a regression"

Authored-by: egg

[egg-reviewer]

No agent-mode design concerns.

The changes add domain context (severity classification, end-to-end functionality checks) to review criteria files — this is orienting guidance that helps the agent make better judgment calls, not procedural constraints. Aligns with guideline #4 (prefer what over how).

— Authored by egg

[egg-reviewer]

Review: Classify non-functional features as blocking in reviewer

The motivation is sound — PR #951 exposed a real gap where a non-functional feature was classified as non-blocking. The changes to code-review-criteria.md are well-written and the severity classification framework is a good addition.

Two issues, one blocking:

---

1. Inline fallback divergence — blocking

The inline fallback in build-review-prompt.sh (lines 37-85) only received a one-line addition to the Correctness section. It is now missing:

• The "Verify end-to-end functionality" step in the "How to Review" procedure (step 4 in the shared file)
• The entire "Severity Classification" section (blocking vs. non-blocking categories, false analogies warning)
• The renumbered steps 5-6

This means when the inline fallback is used (shared file unavailable), the reviewer gets a watered-down version of the criteria that lacks the core fix this PR is trying to make. The severity classification and false-analogies warning are arguably the most important additions — they're exactly what would have prevented the PR #951 misclassification.

Additionally, there is a third fallback in orchestrator/routes/pipelines.py:919-939 (_get_code_review_criteria()) that was not updated at all. It doesn't even have the one-line correctness bullet. This is the same pattern: if the shared file isn't found, the orchestrator's reviewer agent gets zero benefit from this PR.

Suggested fix: Either:

• (a) Add the severity classification and procedure step to both inline fallbacks, or
• (b) Remove the inline fallbacks entirely and fail loudly when the shared file is missing (since the fallbacks are increasingly divergent and hard to maintain in sync). The build-review-prompt.sh already prefers the shared file via fetch_review_rules(), so the fallback only fires in degraded deployments where having stale criteria is arguably worse than failing.

Option (a) is the safer incremental choice. Option (b) is better long-term.

2. Orchestrator fallback not updated — related to #1

orchestrator/routes/pipelines.py:919-939 has its own inline fallback for code review criteria. This PR didn't touch it. Even if you only add the one-liner, the three sources of truth (shared file, shell fallback, Python fallback) should stay in sync.

---

Everything else looks good. The code-review-criteria.md changes are clear and well-structured. The review-conventions.md additions correctly frame non-functional features as correctness issues. The "false analogies" paragraph directly addresses the root cause from #954.

— Authored by egg

[james-in-a-box]

Agreed. Both inline fallbacks are now synced with the shared code-review-criteria.md:

1. build-review-prompt.sh — Added the "Verify end-to-end functionality" procedure step (step 4), the full Severity Classification section (blocking vs non-blocking categories), the false analogies warning, and renumbered steps 5-6.

2. orchestrator/routes/pipelines.py (_get_code_review_criteria()) — Added the end-to-end functionality bullet under Correctness, and the full Severity Classification section with false analogies warning.

Went with option (a) — keeping the inline fallbacks as a safety net while syncing their content. All three sources of truth (shared file, shell fallback, Python fallback) now contain the core additions from this PR.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns in the delta.

The new commit syncs inline fallbacks with the shared criteria — all additions are domain-level review guidance (severity classification, end-to-end functionality checks, false analogies warning). This is orienting context that helps the agent make better judgment calls, not procedural constraints. Previous approval still holds.

— Authored by egg

[egg-reviewer]

Re-review: Sync inline fallbacks with shared review criteria

The previous review raised one blocking issue: both inline fallbacks (shell and Python) were missing the core additions from this PR — the Severity Classification section, the end-to-end functionality step, and the false analogies warning. The new commit addresses this.

Verification of previous feedback

Shell fallback (build-review-prompt.sh): Properly synced. The Severity Classification section, false analogies paragraph, end-to-end functionality procedure step (step 4), and renumbered steps 5-6 are all present and match the shared file verbatim.

Python fallback (orchestrator/routes/pipelines.py): The core additions are present — end-to-end bullet under Correctness, full Severity Classification section, and the false analogies warning. This addresses the blocking concern.

Remaining divergences (non-blocking, pre-existing)

The Python fallback was already a stripped-down version before this PR. The following gaps are pre-existing technical debt, not regressions from this commit:

1. Missing entire sections: "How to Review" procedure (6 steps), Testing, Documentation, Skip, preamble ("Be extremely thorough..."). These were never in the Python fallback.
2. Missing individual bullets: "Insecure cryptography or randomness" (Security), "Incorrect algorithm complexity for data size" (Correctness), "State corruption scenarios" (Robustness), "Missing or incorrect abstractions" (Design).
3. Shortened wording: The end-to-end bullet and false analogies paragraph use abbreviated versions compared to the canonical file.

None of these are regressions — the Python fallback improved from having zero severity guidance to having the full classification framework.

One minor observation

The false analogies paragraph in the Python fallback is truncated compared to the shared file and shell fallback. The shared file says: "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." The Python version drops the conditional sentence and only keeps "Classify based on actual functionality, not superficial similarity." This removes the concrete illustration of why a false analogy occurs, which was arguably the most instructive part of that paragraph. Not blocking since the Python fallback is a degraded-mode safety net, but worth noting for future sync efforts.

No blocking issues. The core feedback from the previous review has been properly addressed.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns in the delta.

The single new commit expands the false analogies paragraph in the Python fallback to match the shared file — still domain-level review guidance, no agent-mode implications. Previous approval stands.

— Authored by egg

[egg-reviewer]

Re-review: Expand false analogies in Python fallback

The delta since the last review is a single commit (12e8b64) that expands the false analogies paragraph in the Python fallback (orchestrator/routes/pipelines.py). This addresses the non-blocking suggestion from the previous review about the truncated conditional sentence.

Verification

The false analogies paragraph now matches verbatim across all three sources of truth:

• shared/prompts/code-review-criteria.md (canonical)
• action/build-review-prompt.sh (shell fallback)
• orchestrator/routes/pipelines.py (Python fallback)

The conditional sentence ("If the existing pattern works via mechanism A but the new code relies on mechanism B that doesn't exist, the comparison is invalid") is now present in all three. The change is clean — only the intended lines were modified, no surrounding code was altered.

Pre-existing divergences (unchanged, non-blocking)

The Python fallback still omits some sections present in the shared file and shell fallback (the full "How to Review" procedure, Testing, Documentation, Skip sections, and a few individual bullets). These were noted in the previous review as pre-existing technical debt, not regressions from this PR. They remain unchanged.

No blocking issues. Previous feedback has been fully addressed.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns in the delta.

The new additions ("Do not dismiss issues as 'not a regression'" paragraph, "Pre-existing broken or inconsistent behavior" blocking bullet, and the matching review-conventions.md paragraph) are domain-level review guidance that helps the agent make better severity classification decisions. All three sources of truth (shared file, shell fallback, Python fallback) are properly synced. Previous approval stands.

— Authored by egg

[egg-reviewer]

Re-review: Classify pre-existing broken behavior as blocking

The branch was squashed since my last review (12e8b64), so I re-reviewed the full PR diff against main. The PR now covers two scenarios (originally just #951, now also #960): non-functional features and pre-existing broken behavior in modified code.

Previous feedback status

Both blocking issues from my first review are resolved:

1. Shell fallback sync — The build-review-prompt.sh inline fallback now includes the Severity Classification section, end-to-end procedure step, and false analogies paragraph. Verified.
2. Python fallback sync — The orchestrator/routes/pipelines.py inline fallback now includes the end-to-end correctness bullet, full Severity Classification section, and the complete false analogies paragraph (including the conditional sentence that was previously truncated). Verified.

New content review (#960 additions)

The #960-related additions are well-written and address a real gap:

• Canonical file: "Pre-existing broken or inconsistent behavior in code the PR modifies" as a blocking item, with detailed explanation and example. The "Do not dismiss issues as 'not a regression'" paragraph adds the stronger framing.
• review-conventions.md: "Pre-existing issues are still blocking" paragraph — clear, direct, well-placed under the existing request-changes criteria.
• Shell fallback: Has the blocking bullet (abbreviated) and the "do not dismiss" paragraph (abbreviated).
• Python fallback: Same as shell.

Remaining divergences (non-blocking, pre-existing)

For completeness, documenting the current state of fallback divergence. None of these are regressions from this PR — all are pre-existing gaps:

Shell fallback vs canonical:

• Missing "Testing", "Documentation", and "Skip" sections
• "Pre-existing broken..." blocking bullet omits the detailed explanation (— if the PR touches code that already has bugs...)
• "Do not dismiss" paragraph is truncated (missing: 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.)
• Correctness end-to-end bullet is simplified (no example about build-time vs runtime config)

Python fallback vs canonical (same as shell, plus):

• Missing entire "How to Review" procedure (steps 1-6)
• Missing "Be extremely thorough" preamble
• Missing "State corruption scenarios" bullet under Robustness
• Single quotes instead of double quotes in "not a regression" (cosmetic, due to Python string escaping)

These are not actionable for this PR — each fallback was already an abbreviated version before this PR, and the core additions (Severity Classification, false analogies, end-to-end) are present in all three.

Verdict

No blocking issues. The PR correctly addresses both #951 and #960 scenarios. All three sources of truth contain the core additions. The content is well-written and directly addresses the root causes of the misclassifications.

— Authored by egg

[james-in-a-box]

SDLC reviewer synced with PR reviewer

The SDLC reviewer in _build_review_prompt() was missing several conventions that action/review-conventions.md had. This commit adds:

orchestrator/routes/pipelines.py:

• Added "Verify end-to-end functionality" to the procedural review steps for code reviewers (was missing between "trace data flow" and "research when uncertain")
• Added "When to Use needs_revision vs approved" section to inline conventions, matching the PR reviewer's "When to Approve vs Request Changes" guidance:
  • Explicit list of blocking conditions (including non-functional features, pre-existing issues)
  • "Key distinction" paragraph about non-functional features
  • "Pre-existing issues are still blocking" paragraph

shared/prompts/REVIEWER-SYNC.md (new):

• Guidance doc explaining the two reviewer surfaces, what's shared, what's intentionally different, and a modification checklist for keeping them in sync

docs/guides/github-automation.md + docs/index.md:

• Added references to the sync guide

— Authored by egg

[egg-reviewer]

No agent-mode design concerns in the delta.

The two new commits since 5626849:

1. SDLC reviewer sync (e521ee4): Adds verdict classification guidance and an end-to-end verification step to _build_review_prompt(), plus a new REVIEWER-SYNC.md guide. These are domain-level review standards that orient the agent on what constitutes blocking vs non-blocking — not procedural constraints on how to explore or reason. The REVIEWER-SYNC.md is a maintenance guide for humans/agents modifying review criteria, not a prompt or pre-fetched context.

2. Re-trigger (19c7445): Empty commit, no design implications.

Previous approval stands.

— Authored by egg

[egg-reviewer]

Re-review: SDLC reviewer sync + REVIEWER-SYNC.md

The branch was squashed since the last review (5626849), so I reviewed the full PR diff against main and cross-referenced the previous review thread. Two issues, one blocking.

Previous feedback status

All previously raised blocking issues are resolved:

1. Shell fallback sync — build-review-prompt.sh inline fallback contains Severity Classification, end-to-end procedure step, false analogies paragraph. Verified.
2. Python fallback sync — _get_code_review_criteria() inline fallback contains end-to-end bullet, Severity Classification, and the full false analogies paragraph (including the conditional sentence). Verified.

New content review

1. Verdict classification emitted for all reviewer types — blocking

_build_review_prompt() now unconditionally emits the "When to Use needs_revision vs approved" section for all reviewer types. The content is code-specific:

"non-functional features (core purpose doesn't work end-to-end), missing error handling, resource leaks, breaking changes, violations of codebase patterns"

This is correct for reviewer_type == "code", but plan, refine, contract, and agent-design reviewers will receive code-specific blocking criteria that don't apply to their review scope. A plan reviewer should classify based on analysis quality and task breakdown, not "resource leaks." An agent-design reviewer should classify based on anti-pattern violations, not "missing error handling."

The conventions preamble already differentiates (if reviewer_type == "code" on line ~1658). The verdict classification section should follow the same pattern.

Suggested fix: Wrap the verdict classification in a if reviewer_type == "code": guard, or generalize the language for non-code reviewers. Option 1 is simpler:

# Verdict classification — only for code reviewers (aligned with review-conventions.md)
if reviewer_type == "code":
    lines.append("### When to Use `needs_revision` vs `approved`\n")
    lines.append(
        "**Use `needs_revision` for**: Security vulnerabilities, logic errors, ..."
    )
    # ... rest of section
    lines.append("")

Non-code reviewers already get appropriate guidance from the generic verdict format section ("If significant issues remain, set verdict to needs_revision") and their type-specific criteria (e.g., _get_plan_review_criteria() already says "flag as needs_revision" for missing HITL decisions).

2. REVIEWER-SYNC.md: misleading "shared files" claim — non-blocking

The "What's Shared" section states:

"Both reviewers read from the same files in shared/prompts/:"

and lists all three criteria files (code-review-criteria.md, contract-review-criteria.md, agent-design-criteria.md). The PR reviewer only reads code-review-criteria.md — it does not read the other two because it only does code review.

The wording implies both reviewers consume all three files. A more accurate framing would be:

"Both reviewers read code-review-criteria.md for code reviews. The SDLC reviewer also reads contract-review-criteria.md and agent-design-criteria.md for its additional reviewer types."

This matters because someone following the sync guide might incorrectly add a contract-criteria change to the PR reviewer's fallback, thinking both surfaces consume it.

3. Everything else looks good

• The "Verify end-to-end functionality" procedural step (step 5) in _build_review_prompt() correctly matches the shell fallback and shared file content.
• The "Key distinction" and "Pre-existing issues" paragraphs correctly adapt review-conventions.md language to the SDLC verdict terminology.
• The REVIEWER-SYNC.md structure (what's shared, what's different, modification checklist) is useful and accurately describes the dual-surface architecture.
• The docs/index.md and docs/guides/github-automation.md references are correctly placed and linked.
• The step renumbering (5→6, 6→7, ... 9→10) is correct.

— Authored by egg

[james-in-a-box]

Automated feedback loop has reached the maximum of 5 rounds. Human review is needed to make further progress on this PR.

[james-in-a-box]

Both issues addressed in 8c6bbd9:

1. Verdict classification scoped to code reviewer (blocking) — Wrapped the entire "When to Use needs_revision vs approved" section in a if reviewer_type == "code": guard. Non-code reviewers already get appropriate verdict guidance from their type-specific criteria (e.g., _get_plan_review_criteria() says "flag as needs_revision" for missing HITL decisions).

2. REVIEWER-SYNC.md shared files claim (non-blocking) — Reworded to clarify that only code-review-criteria.md is consumed by both reviewers. The other two files (contract-review-criteria.md, agent-design-criteria.md) are now explicitly marked as "SDLC reviewer only."

— Authored by egg

[james-in-a-box]

Automated feedback loop has reached the maximum of 5 rounds. Human review is needed to make further progress on this PR.

[egg-reviewer]

No agent-mode design concerns in the delta.

The new commit scopes the verdict classification section to code reviewers only (if reviewer_type == "code": guard) and corrects the REVIEWER-SYNC.md wording about which shared files each reviewer consumes. Both are domain-level improvements — the scoping change actually improves agent-mode alignment by preventing non-code reviewers from receiving irrelevant code-specific guidance. Previous approval stands.

— Authored by egg

[egg-reviewer]

Re-review: Scope verdict classification to code reviewer

The delta since the last review is a single squashed commit (8c6bbd9) that addresses both issues from the previous review.

Verification of previous feedback

1. Verdict classification scoped to code reviewer (was blocking): The entire "When to Use needs_revision vs approved" section is now wrapped in if reviewer_type == "code":. Non-code reviewers (plan, contract, agent-design, refine) no longer receive code-specific blocking criteria. Verified at orchestrator/routes/pipelines.py:1687.

2. REVIEWER-SYNC.md shared files claim (was non-blocking): The "What's Shared" section now correctly states: "Both reviewers read code-review-criteria.md for code reviews. The SDLC reviewer also reads contract-review-criteria.md and agent-design-criteria.md for its additional reviewer types." Each file in the bullet list is explicitly tagged "(both reviewers)" or "(SDLC reviewer only)."

Cross-source consistency

Verified all three sources of truth for the new content:

• False analogies paragraph: Identical across shared file, shell fallback, and Python fallback (including the conditional sentence about mechanism A/B).
• Severity Classification section: Present in all three, with consistent blocking/non-blocking categories.
• End-to-end functionality: Present as both a correctness bullet and a procedural step in all three.
• "Do not dismiss" paragraph: Present in all three. The fallbacks use an abbreviated version (missing the last two sentences from the shared file), but this is consistent with the pre-existing abbreviation pattern documented in prior reviews.

No blocking issues. Previous feedback has been properly addressed.

— Authored by egg

[james-in-a-box]

Automated feedback loop has reached the maximum of 5 rounds. Human review is needed to make further progress on this PR.

[james-in-a-box]

egg review completed. View run logs

22 previous review(s) hidden.