Code Review & Severity System

Severity Definitions

Severity	Action	Examples
Critical	Auto-rewrite — fix immediately, re-run tests, commit	SQL injection, auth bypass, hardcoded secrets, data loss risk, missing XSUAA scope check, unvalidated external input
High	Auto-rewrite — fix, re-run tests, commit	Missing error handling, spec deviation, no asserting test for an implemented clause, CDS model mismatch, missing transaction on write, tenant isolation gap
Low	Comment only — noted for human review	Naming convention, code style, opportunity for utility extraction, verbose logic, missing JSDoc, over-build, over-test (tests no clause requires — see what earns a test)

Review Output Format

## Spexso Review Report — Issue #42

### CRITICAL (0 found)

### HIGH (2 found -> auto-fixed)
- srv/leave-service.js:47 — Missing error handling on SF API call
  -> Fixed: wrapped in try/catch with proper error propagation
- srv/leave-service.js:82 — No input validation on date range
  -> Fixed: added CDS validation annotation + handler check

### LOW (3 found -> comments)
- srv/leave-service.js:12 — Consider extracting date util
- db/schema.cds:8 — Redundant annotation (already inherited)
- test/leave.test.js:30 — Test description could be more specific

### Clause rubric
- AC-1 pass — asserted by "AC-1: rejects a negative quantity"
- AC-2 fail — no test in the AC mapping (see HIGH #2)
- AC-3 not-testable — docs-only clause

### Summary
Files reviewed: 6 | Tests: 12 pass | Coverage: 84%
Auto-fixes applied: 2 | Comments: 3 | Blocking issues: 0
Ready for /spexso-finish

Clause rubric & spec-to-test traceability

Spec compliance is not an open-ended judgment call. LLM-as-judge verdicts are measurably unreliable — single-run reviews of the same code disagree with themselves (arXiv 2606.19544, arXiv 2604.16790; no judge in the RAND harness scored consistently) — so Spexso replaces the holistic "does this match the spec?" question with a deterministic, binary per-clause rubric:

• For every EARS clause in the spec/plan (AC-1, AC-2a, …) the reviewer records exactly one verdict — pass | fail | not-testable — with a one-line reason.
• The evidence is the tester's AC mapping: during the RED phase the tester ends its report with a compact clause id → test name(s) list (the plan's Covers: field names the clauses). A clause is a pass only when the diff implements it AND the mapping names at least one asserting test; a clause missing from the mapping is a fail regardless of the code. When no mapping was handed over, the reviewer reconstructs it from the AC-prefixed test names (it('AC-1: …')).
• Every fail becomes a HIGH "spec drift" finding; behavior no clause requires stays a LOW "over-build" comment.

The same anchor rule applies to all findings: each must be anchored to an EARS clause, a rulebook rule, or a deterministic check output (test run, static check, mutation gate). Single-run verdicts on style or architecture without such an anchor are comment-only, never blocking.

Mutation spot-gate (opt-in)

Tests that assert the wrong thing pass the rubric on paper; mutation testing is the one substantiated way to verify tests actually catch faults (Meta's ACH deployed it at scale, arXiv 2501.12862). Set mutationCommand in .spexso/config.json to enable the gate:

{ "mutationCommand": "npx stryker run --mutate {files}" }

Before spawning the reviewer, /spexso-review runs the command with {files} = the issue's changed source files (test files excluded) and hands the surviving-mutant output to the reviewer as evidence — a clause whose tests kill no mutants in the code they cover is a fail on that clause. Absent or null (the default), the gate is skipped silently; it is the most expensive verification step, so keep it for projects where a mutation runner (Stryker, mutmut) is already set up.

Convergence check (--converge, opt-in)

/spexso-review --converge adds one pass after the normal review: the shipped codebase is compared against the spec's acceptance criteria, and every criterion the code does not meet is listed as a candidate follow-up issue (title + one-line rationale). In yolo mode they are created directly with stage::opportunity and a [converge #{id}] marker in the body; in pilot/copilot the list is printed for the developer. Off by default — it costs tokens and is a deliberate brownfield-drift act. See /spexso-review.

Domain rulebooks

Before reviewing files in a recognized SAP domain, the reviewer reads the matching project rulebook from .spexso/rules/ and uses its rules as additional review criteria:

Files in diff	Rulebook
any .ts/.tsx/.js/.jsx (base layer)	.spexso/rules/typescript.md
.cds, srv/**, db/**	.spexso/rules/cap.md
*.view.xml, *.fragment.xml, webapp/controller/**, manifest.json	.spexso/rules/ui5.md
*.view.xml, *.fragment.xml, webapp/**, manifest.json (Fiori UX dimension)	.spexso/rules/ux.md
*.test.ts/js, test/**, QUnit/OPA5	.spexso/rules/test.md
*.abap	.spexso/rules/abap.md

These are the same rulebooks the builder and tester read — one source of truth, multiple consumers. The builder/tester use them to prevent issues; the reviewer uses them to catch what slipped through. Rulebook violations are classified by the standard severity table above (a CQL-injection rule → CRITICAL/HIGH auto-rewrite; a naming rule → LOW comment-only). Missing rulebook files are skipped silently.

UX findings are advisory. The ux.md rulebook is the consolidated Fiori design lens (floorplan & layout choice, the control for each data shape — §5b and its "plain-UI smells", e.g. a history in a table instead of a timeline —, action placement, control/element states, status semantics, copy, accessibility, localization) — the same file the planner uses for floorplan/layout choices and the builder follows while building. The reviewer's findings from it are surfaced as comments with concrete suggestions and are never auto-rewritten — a second exception alongside static-check findings, because UX is a design judgement for a human. Code-level UI5 findings from ui5.md still follow the normal auto-rewrite rules.

Floorplan compliance. On any UI5 diff, the reviewer checks the built app against the floorplan chosen during planning (ux.md Part A) — missing the floorplan's mandatory root layout or controls is flagged as HIGH "floorplan drift", since a silent floorplan switch should instead go back through the planner. Unlike the advisory UX-design findings above, floorplan drift is a structural compliance check against the plan's contracted floorplan — normal severity rules apply (HIGH), not the comment-only advisory treatment.

UI contract compliance. /spexso-review passes the reviewer the UI contract: every task's UI: line from the plan (.spexso/plans/issue-{id}.md, which /spexso-plan keeps on disk) and the spec's UI table. The reviewer checks each named control and header element against the diff: the view or fragment for freestyle, the annotations (UI.HeaderFacets, UI.DataPoint and its Visualization, UI.Chart, Criticality) for Fiori Elements. A contracted control that is missing or replaced (a table where the plan says Timeline, a header with only a title where the plan lists key figures) is HIGH "UI contract drift" and is auto-fixed by building the contracted control. The UX trigger includes Fiori Elements annotations (annotations.cds, app/**/*.cds, webapp/annotations/*.xml): in a CAP project that is where the whole Fiori Elements UI lives. §5b smells on UI that no contract covers stay advisory.

See rulebooks for how to tune them.

Static checks (project-wide)

In addition to the diff-walk and severity classification described above, the reviewer runs every command in .spexso/config.json → staticChecks across the full project (with {files} replaced by empty string). Each non-zero exit is logged as a finding with severity HIGH.

Static-check findings are the ONE exception to the auto-rewrite-on-HIGH rule. Lint corrections require human judgement — the right fix often isn't the obvious one (a namespace typo may indicate deeper API misuse). The reviewer surfaces these failures with full output and asks the developer to fix manually before merge.

If staticChecks is empty or missing, the reviewer skips this step entirely.

See static checks for configuration details.

Calibration

Over-flagging trains developers to ignore reviews, so the reviewer follows seven calibration rules: judge severity honestly (CRITICAL/HIGH only for correctness, security, or spec breaks — style is never above LOW); make every finding actionable with a copy-pasteable suggestion and a file:line location; read the surrounding code before flagging it, since an established project pattern isn't a bug; collapse several symptoms of one root cause into a single finding rather than one per symptom; ground every CRITICAL/HIGH finding in a cited source — a spec clause, a rulebook rule, or an official guideline, never opinion alone; anchor every finding to an EARS clause, a rulebook rule, or a deterministic check output, since unanchored single-run judgments on style or architecture are noise-prone and stay comment-only; and treat a deliberate business/UX trade-off that conflicts with a guideline as a NOTE/LOW observation, never a CRITICAL/HIGH escalation.

Rulebook promotion (systemic lessons)

Some findings aren't one-off slips — they're a SYSTEMIC class of mistake that will recur on future tasks because no rule prevents it. For those, the reviewer may propose a rulebook addition, marked RULEBOOK PROPOSAL. In hindsight every bug looks systemic, so the default is no proposal, and the reviewer proposes only when a lesson passes an admission test:

1. it backs a CRITICAL or HIGH finding (never a LOW);
2. it names no identifier of this codebase — an entity, field or function name makes it an invariant of this code, which belongs in a test or a code comment;
3. no rulebook line covers it yet (the reviewer quotes the nearest existing rule);
4. it isn't general knowledge that a competent developer, tsc or the linter already catches;
5. it is checkable against a diff.

At most one proposal per review, tagged with a scope:

Scope	Meaning	Goes to
project	a convention of this project	.spexso/rules/{domain}.local.md
stack	true for every CAP/UI5/ABAP project	the same local file, flagged as worth sending upstream
spexso	a gap or defect in Spexso itself	no rulebook line; reported with /spexso-feedback

The reviewer never edits a rulebook itself — the orchestrator owns the human gate. Every project has one rulebook inbox: an open issue titled [rulebook-inbox] … (type::tech-debt) whose body is the ledger of candidate lessons:

## Ready          ← seen 2× or more, one checkbox each
- [ ] **[cap] cap § Errors** — Call `req.error` once per request … · seen 2× — #12, #19
## Candidates     ← seen once
## Rejected       ← turned down by a human; never proposed again
- **[ux] ux § Copy** — … · rejected 2026-09-29

Before anything else, a new proposal is compared with ## Rejected: a lesson a human already turned down is dropped silently, in every mode. Then the gate depends on the mode:

• pilot / copilot — /spexso-review shows the proposal as a concrete diff (target file, section, the exact line(s) to append) and asks: apply, edit, or reject. On approval the line is appended to .spexso/rules/{domain}.local.md (never the shipped baseline) and committed as its own change (e.g. chore(rules): promote systemic lesson — {short}). A rejection is recorded under ## Rejected. stack and spexso proposals come with an offer to report them via /spexso-feedback.
• yolo — nothing is written to a rulebook, and no issue is filed per proposal. The proposal goes into the inbox. A lesson that is already there gains a sighting (seen 2× — #12, #19) instead of a new entry. Only entries seen at least twice move under ## Ready, so one-off lessons never reach a human.

Deciding: /spexso-rulebook or by hand in GitLab

When the inbox holds ready or ticked lessons, every review summary and the marathon summary end with one line: 📥 {n} rulebook lesson(s) ready (#{inbox}) → /spexso-rulebook.

/spexso-rulebook asks whether to go one by one (apply / edit / reject / skip, each shown as a diff) or to approve all ready lessons at once. It applies the approved lessons on a fresh branch, one commit per lesson, and opens a merge request — it never merges it, so a rule change goes through the team's normal review. Rejected lessons move under ## Rejected.

The same decisions work by hand in the GitLab issue:

In the inbox issue	Effect
Tick an entry (- [x])	Approved — the next /spexso-rulebook applies it without asking
Edit an entry's text, then tick it	Approved with your wording
Move an entry under ## Rejected	Rejected — /spexso-review never proposes it again
Delete an entry	Gone for now — but the lesson can come back if a review finds it again

Two writers of the inbox at the same moment (parallel reviews, or a review racing /spexso-rulebook) can drop one change. That is cheap: a lesson that truly recurs is proposed again, and a lost rejection at worst returns the lesson once more.

Either way, the rulebook only grows through curated accretion with a human in the loop — never a silent auto-write.

Why an inbox instead of one issue per lesson

Earlier versions of yolo filed a [rulebook-lesson] issue per proposal. In real projects that produced dozens of open tickets within days, most of them code-specific invariants or generic advice, and nobody triaged them. New lessons go to the inbox; the first /spexso-rulebook run migrates the old [rulebook-lesson] issues — it filters them with the admission test, merges duplicates as sightings, and closes each one with a note once you confirm the plan.