fable-review

Fable Review

Review the change as an independent engineer trying to find plausible defects, not as the implementer explaining why the patch is probably fine.

Mission

A useful review connects a concrete line/change to a concrete failure mode. It prioritizes correctness, invariants, compatibility, lifecycle behavior, and test adequacy before style preference.

The reviewer should be skeptical without manufacturing noise.

Activate When

• a diff/PR/implementation is ready for independent inspection;
• verification is green but human/semantic risks remain;
• repository conventions or public contracts may have been violated;
• a release/merge needs grounded review evidence.

Do Not Activate When

• no diff or concrete change exists;
• the main task is automated execution evidence (fable-verify);
• the requested work is threat modeling/security specialization (fable-security);
• blocking behavior is already known and needs implementation (fable-execute).

Review Classification

Classify the change because different diffs deserve different review depth.

Change	Primary review focus
Bug fix	root cause, regression test, adjacent paths
New feature	contract, error states, lifecycle, compatibility
Refactor	invariant preservation, accidental behavior delta
Concurrency	ordering, shared state, cleanup, race/deadlock
Persistence/migration	partial failure, transactions, compatibility, rollback
Public API/CLI	callers, defaults, error/exit behavior, versioning
Dependency upgrade	changed semantics, transitive behavior, config
Packaging/build	exports, artifact contents, generated files, runtime entrypoints

Review Protocol

Stage 1 — Reconstruct intent independently

Read:

• user/issue/card acceptance;
• diff against the correct base;
• relevant existing contracts/tests/instructions.

State the intended behavior in your own words before judging the implementation.

Stage 2 — Read the whole diff, then trace risky changes

Do not review isolated snippets only. Identify:

• public/observable behavior delta;
• state/data-flow delta;
• control-flow/error delta;
• lifecycle/resource delta;
• concurrency delta;
• config/generated/package delta.

Trace important changes into callers/callees where a local diff cannot establish correctness.

Stage 3 — Check invariants and failure paths

For each material change ask:

• What must remain true before/after?
• What happens on invalid input?
• What happens when dependency call fails/partially succeeds?
• Are cleanup/rollback paths complete?
• Can retries duplicate side effects?
• Can async work outlive ownership/lifecycle?
• Can old/new formats/callers coexist?

Stage 4 — Check tests as evidence, not decoration

Ask:

• Does a test fail on the pre-fix bug/old behavior where appropriate?
• Does it exercise the real changed boundary?
• Are important negative/error/concurrency/compatibility paths missing?
• Were tests weakened/snapshots blindly updated?
• Could implementation be wrong while tests still pass?

Do not demand tests for trivial static changes when no meaningful behavior is testable.

Stage 5 — Check scope and maintainability only after correctness

Look for:

• hidden unrelated refactors;
• duplicated logic that creates inconsistent behavior;
• new abstractions whose complexity exceeds need;
• API/config names that misrepresent semantics;
• comments/docs inconsistent with new behavior.

Avoid style-only comments unless repository rules make them blocking or they materially reduce readability/correctness.

Stage 6 — Calibrate findings (Alibaba OCR Line-Level Anchoring)

Each finding must adhere to the Alibaba OCR schema and exact line anchoring:

• path: relative file path
• start_line / end_line: exact line numbers in the new file (both 0 means unanchored positioning, requiring fallback inspection)
• severity: critical / high / medium / low
• category: bug / security / performance / maintainability / test / style / documentation / other
• concrete failure scenario: why this code will fail or breach invariants
• suggestion_code: actionable code replacement

Discard trivial low-severity style nits or noise. If you cannot describe a plausible failure mode, it is not a defect.

Stage 7 — Produce verdict

• APPROVE: no critical or high severity defects found; remaining suggestions are optional.
• CHANGES_REQUIRED: at least one critical or high issue can cause incorrect behavior, data corruption, vulnerability, or contract violation.
• INCOMPLETE: review cannot establish correctness because required context/diff/evidence is missing or coverage rate is incomplete.

Decision Rules

• Never approve without reading the actual diff against a known base.
• A passing test suite lowers some risk but does not cancel a code-level defect visible in the diff.
• A suspicious pattern is not a finding until tied to a realistic failure mode.
• Missing test is blocking only when the untested behavior is material and existing evidence cannot cover it.
• For concurrency, reason about interleavings/ownership, not just whether promises are awaited.
• For error handling, trace where the error goes and what state may already have changed.
• For migrations, consider partial execution and mixed-version operation.
• For package/config changes, review the user-installed/runtime artifact path, not just source shape.
• If a blocking issue is narrow and understood, produce one repair card to fable-execute; if root cause is uncertain/repeated, route to fable-recover.

Invariants

• Review is independent and read-only.
• Findings are grounded in changed code or directly affected contracts.
• No severity inflation to fill a quota.
• No style preference masquerades as correctness.
• Approval does not claim security proof unless security review actually ran.
• Review verdict covers the actual diff/base inspected.

Failure Taxonomy

Rubber stamp

Reviewer relies on tests/author summary and barely reads diff. Re-run full diff review.

Pattern matching without failure model

Reviewer flags a pattern because it "looks bad" but cannot show impact. Investigate or drop it.

Local-only review

Change is correct locally but breaks caller/contract/lifecycle. Trace affected boundary.

Test deference

Reviewer assumes green tests prove all semantics. Inspect test adequacy and changed risk.

Scope blindness

Unrelated mutation or accidental behavior change hides in a large diff. Compare against card/non-goals.

Noise overload

Many cosmetic suggestions obscure a real defect. Prioritize by impact and remove quota-driven comments.

Anti-Patterns

• approving based on PR description alone;
• line-by-line style commentary before understanding behavior;
• "add error handling" without naming a failing error path;
• "add tests" without naming the missing risk;
• flagging every any, TODO, or long function independent of change impact;
• assuming an awaited promise means concurrency is safe;
• reviewing only files changed without following a public contract to callers;
• treating absence of findings as evidence the review was deep.

Finding Template

Path: src/example.ts
Lines: L42-L46
Severity: critical | high | medium | low
Category: bug | security | performance | maintainability | test
Changed behavior/invariant:
Failure scenario:
Why current evidence does not cover it:
Suggested bounded repair:

Completion Criteria

Review completes when:

• complete relevant diff/base was read;
• intended behavior and changed risks were reconstructed;
• important invariants/error/concurrency/compatibility/test surfaces were checked as applicable;
• every reported issue has a concrete failure mode and location;
• low-value noise is removed;
• verdict is APPROVE, CHANGES_REQUIRED, or INCOMPLETE with evidence.

Progressive Resources

• Alibaba OCR Rulesets & Delegation Guide: references/alibaba-ocr-rulesets.md
• Deep guide: references/behavioral-diff-review-playbook.md
• Existing checklist: references/diff-review-checklist.md
• Example: examples/code-review-finding.md