review-criteria: recalibrate severity per #2128

[jwbron]

Summary

Closes #2128 (option C from postmortem #2125). Updates the two base review-criteria files so the four classes that slipped past the in-pipeline review on PR #2105 are explicitly blocking. Option F (reviewer_code_holistic, #2126/#2130, reshaped in #2152) already owns the long-form verification recipes for these patterns for the BRC pipeline; this change moves the severity classification into the base files so non-BRC consumers (GHA reviewer, sequential SDLC reviewers, the in-pipeline reviewer_code that does not load the holistic file) inherit the recalibrated floor.

shared/prompts/code-review-criteria.md

• Blocking list now calls out:
  • Primary advertised use case silently no-ops — the synthetic-key dead-end class (__checkout__ shape on PR Simplify repository configuration — schema cleanup, layered repo+local config, onboard skill, validator #2105). Distinct from the existing "non-functional features" bullet because each unit looks fine; the cross-module wiring dead-ends.
  • Operator-facing misconfiguration produces no signal — silent except Exception: fallbacks, None-on-error returns, no-op default branches that mask invalid operator input. The safety floor holding does not make this non-blocking.
• Testing section grew three blocking patterns:
  • Self-seeding goldens — first-run-from-impl pins the bug.
  • Hand-built fixtures that bypass the production code path — a regression in the helper would not break the test.
  • Name-vs-behaviour contradictions — e.g. test_zero_major_hard_fails accepting '0.1'.

shared/prompts/security-review-criteria.md

• New §8 Agent-supplied paths flowing into read-only file access explicitly recalibrates RC2 from postmortem BRC review missed 14 GHA blockers on PR #2105 — fan out lens reviewers, expand cross-partition pass categories #2125: read access of attacker-chosen paths is path traversal regardless of whether the handler also writes or shells out. Includes verification recipe (workspace-root prefix check, symlink resolution on the resolved path).
• §5 (credential shims) now cross-references §8 for the read-only analogue to wrapper writes.

Out of scope

• Doc-correctness lens (option D from BRC review missed 14 GHA blockers on PR #2105 — fan out lens reviewers, expand cross-partition pass categories #2125), fan-out lenses (option A), enumeration of cross-partition pass categories (option B) — separate from BRC: recalibrate severity in code/security review criteria files #2128's scope.
• The __checkout__-style and silent-fallback verification recipes are not duplicated into the base files; they continue to live in code-review-holistic-criteria.md. The base file states severity, the holistic file owns the procedure.

Test plan

• Self-review the rendered criteria files in the PR diff to confirm the new bullets read cleanly alongside the existing ones (no duplication, no ordering glitches).
• Spot-check that code-review-holistic-criteria.md still reads as the source of truth for how to look for synthetic-key / silent-fallback patterns; the base file is the source of truth for severity.
• Confirm pre-commit hooks pass (already passed locally).

Refs: #2128, #2125 (RC1, RC2, RC6), #2105, #2126.

[egg-reviewer]

No agent-mode design concerns.

This PR only touches two reviewer-criteria prompt files to recalibrate severity classifications. The new bullets state what bug classes are blocking; the verification recipes describe what to verify (workspace-root prefix check, independently authored goldens) rather than micromanaging how the reviewer must investigate. That's appropriate orienting context — it sharpens the reviewer's bar without constraining its exploration.

No pre-fetching, no structured-output requirements, no API/SDK bypasses, no hardcoded model IDs.

— Authored by egg

[egg-reviewer]

Review: PR #2197 — review-criteria severity recalibration

Prompt-text-only change to two markdown criteria files. I verified the PR's claims against postmortem #2125, the canonical PR #2105, the holistic-criteria split (#2126/#2130), and how the GHA reviewer + sequential SDLC reviewers load these files at runtime. The architecture (severity in base files, verification recipes in code-review-holistic-criteria.md) holds: non-BRC consumers — GHA egg-reviewer, sequential SDLC reviewers, and the in-pipeline reviewer_code that does not load the holistic file — will now inherit the recalibrated floor without needing the holistic file.

No blocking issues. A handful of non-blocking wording observations below.

Non-blocking

1. Overlap between the two new "Blocking" bullets and the pre-existing "Non-functional features" bullet (code-review-criteria.md:62-66).

The new "Primary advertised use case silently no-ops" bullet sits next to the existing "Non-functional features — the feature's core purpose does not work end-to-end" bullet. The parenthetical "Distinct from the bullet above…" carries the load of the distinction. That works, but a casual reader scanning the list will see two near-identical bullet headers. Consider tightening the existing bullet to "single-module non-functional features" (or similar) so the cross-module variant reads as a clean second axis rather than a clarification. Equivalently, fold both into a single bullet with two named sub-cases. Either is fine; the current shape is just slightly noisier than the rest of the list.

2. §8 verification-recipe enumeration is narrower than the threat-model claim.

§8 claims "Read access to attacker-chosen workspace-readable targets is a path-traversal bug class regardless of whether the handler also writes or shells out." Step 1 of the recipe enumerates open(), Path.read_text / read_bytes, os.scandir, glob, pathlib.Path.iterdir, shutil.copy. Missing from the list (and in scope of the threat-model claim):

• os.walk, os.listdir — directory enumeration leaks names of files outside the workspace.
• Path.exists(), Path.is_file(), Path.stat(), os.path.exists(), os.path.isdir() — existence/metadata oracles. An MCP tool that returns True/False based on Path(agent_path).exists() leaks filesystem layout.
• Path.is_symlink(), os.readlink() — readlink leaks the symlink target outside the workspace.
• Loaders that take a Path: yaml.safe_load(Path(p).read_text()), json.load(open(p)), tomllib.load(open(p, "rb")), configparser.read().

Suggest either making step 1 a non-exhaustive list ("…and any other API that opens the file or returns metadata about it") or expanding the inventory.

3. §8 step 2 phrasing on symlink resolution.

"Symlink resolution must happen on the resolved path, not on the raw string the agent supplied."

The intent (call .resolve() first, then is_relative_to) is right and the example a line earlier shows it correctly. But the literal phrasing is circular — "resolution must happen on the resolved path" — and "the raw string" is not the contrast you actually mean (the contrast is "before is_relative_to" vs "after .resolve()"). Suggest:

".resolve() must run before the prefix check, so symlinks and .. segments are collapsed first; checking is_relative_to on the unresolved Path lets a symlink inside the workspace point outside it."

4. "Self-seeding goldens" definition is narrower than the failure mode it should catch.

"a golden fixture written on the test's first run from the implementation under test"

The "first run" framing leaves room for a producer to argue "we regenerated the golden after a fix, so it's not a first run." The actual invariant is: the golden was generated by running the same code path the test exercises, regardless of whether that run was the first one or the tenth. Suggest dropping "first run" and saying:

"a golden fixture generated by running the implementation under test (whether on first run or after a regeneration) pins whatever output that implementation produced. Goldens must be authored against an independently verified expectation."

5. Operator-misconfiguration bullet — qualifier is doing important work; consider promoting it.

The new bullet's body lists "silent exception fallbacks", "None-on-error returns", "no-op default branches". Without the operator-facing misconfiguration qualifier in the header, those patterns are common and often legitimate (e.g., optional config, graceful degradation, defensive nulling). The header carries the qualifier and the closing sentence ("denied paths a user deliberately set") nails it. Reviewers skimming the bullet body in isolation might over-trigger. Consider repeating the qualifier in the body — e.g. "…that mask invalid operator-supplied input" — so the bullet stands alone if quoted.

Verified

• #2125 RC1/RC2/RC6 are accurately reflected. Option C scope (severity recalibration in base files only) matches what landed; verification recipes correctly stay in code-review-holistic-criteria.md to avoid duplication.
• §5 cross-reference to §8 is in the right place — under the "credential-shim writes" sentence, where a reviewer mentally pivots from "what about reads?".
• §8's framing (read access without write/shellout is still path traversal) is the exact threat-model recalibration #2125 RC2 demanded.
• New testing patterns (self-seeding goldens, hand-built fixtures, name-vs-behaviour contradictions) are all rooted in concrete #2105 / #2125 evidence.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns.

The delta since my last review (commit 6135b8d) refines wording in the two reviewer-criteria prompt files — folding two near-identical bullets into one with named sub-cases, promoting the "operator-supplied" qualifier into the bullet body, closing the "regeneration" escape hatch on self-seeding goldens, and expanding §8's API enumeration with a non-exhaustive disclaimer. All five general-review suggestions were addressed without introducing pre-fetched diffs, structured-output requirements, post-processing pipelines, rigid procedures, prompt-level security, direct API calls, or hardcoded model IDs. The criteria continue to orient the reviewer on severity while the holistic file owns how to investigate.

— Authored by egg

[egg-reviewer]

Re-review: PR #2197 — review-criteria severity recalibration (delta ebcc4e7 → 6135b8d)

The new commit addresses all 5 non-blocking suggestions from my prior review. I verified each against the current files, not just the commit message.

Verified addressed

1. Bullet overlap (code-review-criteria.md:64-66). The two near-identical bullet headers ("Non-functional features" + "Primary advertised use case silently no-ops") are now folded into one bullet with named sub-cases — Single-module break and Cross-module silent no-op. The "Caught by reading the file" / "Caught only by tracing data flow" tags make the axis explicit. Cleaner than the original two-bullet shape.

2. Operator-supplied qualifier (code-review-criteria.md:67). Promoted from the header into the body: "patterns that mask invalid operator-supplied input". The follow-up sentence ("These same patterns are often legitimate for non-operator inputs…") explicitly carves out the false-positive case I was worried about. Bullet now stands alone if quoted.

3. Self-seeding goldens regeneration escape hatch (code-review-criteria.md:43). Reframed around "generated by running the implementation under test (whether on first run or after a regeneration)" with the explicit clause "'we regenerated it after a fix' does not rescue this." Closes the loophole.

4. §8 step 1 enumeration (security-review-criteria.md:202-219). Expanded from 6 functions to 5 categorised groups (reads/opens, directory enumeration, existence/metadata oracles, symlink inspection, loaders that take a Path) with the non-exhaustive disclaimer up front. Each category names the leak channel it covers (e.g., "leaks names of files outside the workspace"), which matches the threat-model framing.

5. §8 step 2 circular phrasing (security-review-criteria.md:220-225). Replaced verbatim with the suggested phrasing. Failure mode now named directly: "checking is_relative_to on the unresolved Path lets a symlink inside the workspace point outside it."

Independent re-read of the delta

I read both files end-to-end at the new commit looking for issues introduced by the rewording:

• code-review-criteria.md:64-66's sub-case taxonomy doesn't conflict with the pre-existing "End-to-end feature functionality" bullet under Correctness (line 23) — the latter names the bug class, the former classifies severity. No double-counting.
• §5 cross-reference to §8 (security-review-criteria.md:119) is intact: "of agent-supplied paths is covered separately in §8." Still in the right place.
• The non-exhaustive disclaimer in §8 step 1 is positioned before the list ("The list is non-exhaustive — flag any API that opens the file or returns metadata about it"), so a reviewer skimming for a function name they don't see in the list will read the disclaimer first. Good positioning.
• No new typos, dead links, or structural breakage.

Architecture still holds

Severity classifications in the base file, verification recipes in code-review-holistic-criteria.md — non-BRC consumers (GHA egg-reviewer, sequential SDLC reviewers, in-pipeline reviewer_code that does not load the holistic file) inherit the recalibrated floor without needing the holistic file. The delta does not change this split.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

5 previous review(s) hidden.