Code review rubric

State: open/active

Summary

• A diff to any source file in this repo is judged on eight dimensions — functions, modularity/boundaries, duplication, comments, structure, naming, dead code, tests — each scored with the violation / tension / drift severities sdlc verify changes already emits, never an invented scale.

Rule

Judge the diff, not the whole file, against each of the eight dimensions below. For each dimension where the diff gives grounds for a finding, emit one at the severity that fits — never invent a fourth level, and never emit a finding “for completeness” when the diff gives no grounds.

1. Functions. A function added or changed in the diff does one thing, and its body fits on one screen without scrolling to hold the whole thing in mind. A function that mixes unrelated concerns (parsing and I/O, validation and rendering) or accretes past ~40-60 lines in one diff should be flagged — the number is a symptom, not the rule; a long function with one linear responsibility is not automatically a finding, and a short function doing three unrelated things is.
2. Modularity / boundaries. The diff respects the module and package boundaries already declared for the package it touches — where a package defines one (e.g. no-restricted-imports in .oxlintrc.json bounds import depth inside solutions/ontological/lib/**; scripts/guard-solution-boundary.ts bounds cross-solution reach repo-wide). A new import that reaches across a boundary that a mechanism like those already polices is that mechanism’s job, not this rubric’s, and should not be double-counted here — flag it only when no mechanical guard covers the package, or when the guard couldn’t see it (e.g. a boundary violation in intent that no import-shape rule catches, such as a domain-blind component reading a domain concept without importing it directly).
3. Duplication. A helper, type, or block of logic re-declared instead of reused, or copy-pasted with minor edits instead of factored, is a finding — new code should extend or call existing logic, not clone it.
4. Comments. A comment added or left standing in the diff is accurate and necessary. A comment describing behavior the diff just changed away from, or a comment restating what the code already says with no added information, is a finding.
5. Structure. New files and functions land where a reader would look for them, consistent with how neighboring code in the same directory is organized. A new concept dropped into an unrelated file, or a file split for no reason apparent from the diff, is a finding.
6. Naming. Names in the diff say what the thing is or does, match the vocabulary already used by the surrounding code (the same concept under two names, or a name that means something different elsewhere in the same module), and are not misleading relative to the implementation.
7. Dead code. An export, branch, parameter, or file that the diff introduces or leaves behind with no live caller — including a variant made unreachable by the same diff’s other changes — is a finding.
8. Tests. A behavior change in the diff has a test that would fail without it. A test that only re-exercises the happy path for nontrivial new logic (error paths, boundary values, the branch the diff added) is a finding; a diff with no plausible test seam (formatting, comments-only, generated output) is not.

Why

This rubric exists so a diff gets the same judgment whether the reviewer is a human or sdlc verify changes’s LLM judge, and so that judgment is argued from a fixed list of concerns instead of relitigated per PR.

• Functions / modularity. Some packages already have mechanical guards for import-shape boundaries — for example no-restricted-imports (solutions/ontological/lib/CLAUDE.md, “Imports climb one directory at most”) — and scripts/guard-solution-boundary.ts bounds cross-solution reach repo-wide. This rubric does not re-implement any of them; it exists for the boundary violations in intent that an import-shape scan cannot see, for packages with no mechanical guard at all, and for function-level cohesion, which no mechanical tool in this repo checks anywhere.
• Duplication. docs/PRINCIPLES.md’s “Drop Duplication” principle: when a feature or concept generalizes, the original bespoke implementation should be cleaned up rather than left standing beside its replacement. Duplication found in review is the same failure mode one diff earlier — the clone gets left in place instead of factored.
• Comments. Root CLAUDE.md: “Fix comments as inaccuracies are found, regardless of current task. Do not leave a stale comment for later cleanup.” A diff is exactly where a comment either gets updated to match the code it describes or gets left to drift.
• Structure, naming, dead code, tests. None of these has a dedicated mechanical gate in this repo today. Structure and naming are what let a reader navigate the corpus without re-deriving it each time; dead code is cost with no offsetting value; and a diff whose only coverage is the happy path defers the real test of the change to whoever hits the untested branch in production.

How to apply

• Scope the read to the diff. Judge lines the diff adds or changes, and code the diff’s own changes make relevant (a caller of a function the diff altered, a test file paired with a source file the diff touched). Do not flag pre-existing code the diff leaves untouched — that is this standard’s scope catching up with old debt, not a finding about this change.
• One finding per concrete instance, not one finding per dimension. A diff with three duplicated helpers is three duplication findings (or one finding naming all three sites), not a single vague “duplication present” note.
• Cite evidence. Every finding names the file (and line, when the finding is line-addressable) and quotes or paraphrases the specific code that grounds it — “this function” or “this pattern” without a pointer is not an actionable finding.
• Pick the severity that fits, using this repo’s existing three-level scale (packages/ts/agent-dev/src/types.ts, Severity) — the same vocabulary sdlc verify changes already renders and gates on. Do not invent blocking/major/minor or any other scale.

Severity	Meaning	Example
violation	The diff contradicts a dimension outright.	A duplicated helper with an existing equivalent already in scope; a stale comment describing behavior the diff just removed; a dead export the diff introduces; a behavior change with no test that would catch its regression.
tension	Defensible given constraints visible in the diff, but pulls against a dimension’s intent.	A function doing two things because splitting it now would require a larger refactor out of scope for this diff; a test that covers the new branch but not its edge cases.
drift	The diff complies today but moves the code toward a future finding.	A function that is still one responsibility but is growing toward the point it won’t be; a naming choice that is locally fine but starts a second vocabulary for a concept the module already names differently elsewhere.

• Validator context. This standard is read by sdlc verify changes (solutions/ontological/lib/services/verify/ops/changes.ts), which discovers it via applies_to.paths (matched by solutions/ontological/lib/services/verify/providers/standards.ts) and has an LLM judge the diff against this text directly — the eight dimensions and the severity guidance above are instructions to that judge, not background reading. Findings are advisory unless verify.blocking is set in sdlc.yaml or --blocking is passed for the run; the judge verb passes --blocking so a violation-level finding here fails that verb.

Anti-examples

Example	Dimension	Severity
A 90-line function in the diff that parses a payload, validates it, writes it to disk, and formats a report — four responsibilities in one function.	Functions	violation
A new formatDate helper added when lib/util/date.ts already exports todayUtc and a compatible formatter the diff could have imported instead.	Duplication	violation
A comment reading ”// falls back to the default branch when unset” left standing over code the same diff changed to fall back to main unconditionally.	Comments	violation
A new export function computeLegacyScore added but never called from the diff or anywhere else in the tree.	Dead code	violation
A diff that adds error handling for a null response but whose only new test asserts the success path.	Tests	violation
A diff that renames a variable from data to rawPayload in one function of a module where three sibling functions still call the same shape data — locally clearer, but starts a second name for the same concept in the same file.	Naming	drift

Scope

applies_to.paths is one glob with brace alternation (**/*.{ts,tsx,js,jsx,mjs,cjs,vue,rs,py,sh,astro,swift,sql}, @sksizer/intersect syntax — standards.ts (solutions/ontological/lib/services/verify/providers/standards.ts) reads applies_to.paths off the frontmatter and passes the raw glob strings through as RuleSource.patterns; the brace-alternation parsing and matching itself (index, matches) lives in packages/ts/agent-dev/src/plan.ts), matched against every changed file no matter which solution, app, or package it lands in. This rubric governs source code repo-wide: every matching-extension file under solutions/**, apps/**, packages/**, and scripts/** alike. It does not govern markdown, JSON/YAML/TOML config, or other non-code data files — the eight dimensions (functions, dead code, naming, …) have no grip on declarative data; a rubric for prose or config, if wanted, is a separate standard with its own applies_to.paths.

---

← Back to Standards