Code review discipline

Code Review Discipline

This skill defines how the reviewer produces a final REVIEW that catches problems missed by upstream agents and validates end-to-end coherence. Inspired by:

• OWASP Top 10 (2021 edition) — the de-facto security review baseline
• SOLID principles (Robert C. Martin) — code quality / maintainability lens
• Google Engineering Practices — code review patterns (small CLs, what to look for, fast turnaround)
• CWE / SANS Top 25 — common weakness enumeration
• Smartbear / Google research — review effectiveness studies

---

the reviewer's job (vs the architect / the api-tester / the ui-tester)

Role	Reviews	When
the architect (Architect)	SPEC	After the system-analyst posts SPEC, before coders start
the api-tester	API behaviour	After Backend CHANGES
the ui-tester	UI behaviour, a11y	After Frontend CHANGES
the designer	UX intent match	After Frontend CHANGES (Mode B)
the reviewer	End-to-end coherence: REQUIREMENTS → SPEC → CHANGES → tests → design	After all of the above, before the initiator's final closure

the reviewer catches drift across boundaries:

• Did the implementation match the SPEC?
• Are all FRs from REQUIREMENTS actually implemented?
• Did testing cover the full SPEC, or skip parts?
• Are there security / quality issues in code that the architect didn't see at SPEC level?

---

Cross-trace validation — the reviewer's #1 job

Walk the chain end-to-end:

REQUIREMENTS (root description)
    ↓
SPEC §7 Traceability matrix (FR → SPEC §)
    ↓
CHANGES (Backend, Frontend) — what was actually shipped
    ↓
Test plans (API, UX) — what was actually tested
    ↓
Design brief (the designer's intent)

For every FR / NFR / Acceptance Criterion in REQUIREMENTS:

• ✓ Specified in SPEC?
• ✓ Implemented in CHANGES (Backend + Frontend as applicable)?
• ✓ Tested by the api-tester (if API) and / or the ui-tester (if UI)?
• ✓ Design intent honored (the designer UX review APPROVED)?

Any ✗ at any link in the chain = finding. Severity depends on the gap.

---

OWASP Top 10 (2021) — security review checklist

Run through these 10 for every CHANGES that touches handling of data:

A01:2021 — Broken Access Control

• Multitenancy: every queryset filters by company=request.user.company
• Permission classes on every view (no anonymous access where forbidden)
• No company_id accepted as parameter
• IDOR (Insecure Direct Object Reference): UUIDs not sequential IDs in user-facing URLs
• Vertical / horizontal privilege escalation paths checked

A02:2021 — Cryptographic Failures

• Secrets in env, never in code / logs / git history
• TLS for all transports
• Password hashing via Django's PBKDF2 / Argon2 — never raw
• Sensitive PII not logged
• Tokens / API keys rotated on suspicion

A03:2021 — Injection

• ORM used; raw SQL only via parameterized queries
• HTML escaping by default in templates / Vue / React (don't bypass without reason)
• Command injection: avoid os.system() / subprocess(shell=True) with user input
• LDAP / NoSQL / XPath injection if applicable to the change

A04:2021 — Insecure Design

• Threat model considered for new flows? (E.g. password reset must rate-limit, must invalidate token after use)
• Defense in depth — not relying on one layer

A05:2021 — Security Misconfiguration

• Production defaults secure (DEBUG=False, ALLOWED_HOSTS strict)
• No verbose error pages exposed
• Unused features / endpoints disabled

A06:2021 — Vulnerable and Outdated Components

• Dependency updates: pip / npm / yarn — no known CVEs for added packages
• Pin versions, don't use unbounded ranges in production

A07:2021 — Identification and Authentication Failures

• Session management: secure cookies, expiry
• 2FA / MFA where applicable
• Brute-force protection (rate limit on login / password reset)

A08:2021 — Software and Data Integrity Failures

• HMAC verification on incoming webhooks (CDEK, payment gateways)
• CI / CD: signed releases, immutable artifacts
• Untrusted deserialization avoided (no pickle.loads() on user input)

A09:2021 — Security Logging and Monitoring Failures

• Audit log for sensitive actions (admin role change, refund, etc.)
• Logs don't contain secrets / PII
• Alerts on suspicious patterns

A10:2021 — Server-Side Request Forgery

• Outgoing requests to URLs derived from user input — validated against allowlist
• Don't expose metadata endpoints (cloud) accessible from app

For full reference: https://owasp.org/Top10/

---

SOLID — code quality lens

Single Responsibility Principle (SRP)

• Class / function / module does one thing
• 200-line model with 50 methods → split (extract service, query manager, mixin)
• 300-line view that does parsing + validation + persistence + email sending → split

Open / Closed Principle

• Extension via new types, not modification of existing
• New status added by extending choices, not by if status == 'new_status' everywhere
• New payment provider = new class implementing the interface, not a series of if provider_name == 'stripe' / 'tinkoff' / ...

Liskov Substitution

• Subclasses honor parent contracts (don't strengthen preconditions / weaken postconditions)
• If a base view returns 404 on missing object, the subclass should too — not raise InternalServerError

Interface Segregation

• Don't force clients to depend on methods they don't use
• Big "Manager" classes split into specific protocols (OrderQueryRepository / OrderCommandRepository)

Dependency Inversion

• Depend on abstractions, not concretions
• Service depends on EmailSender protocol, not SMTPSender directly
• Especially important for testability — mocks at the interface boundary

In Django context, SOLID often means: thin views, fat models, services layer for cross-cutting flows, manager / queryset for query logic, signals only for true cross-context concerns (audit log, cache invalidation).

---

Conventions audit (rule-by-rule against kb/conventions.md)

Architecture (the architect's 7-area lens) covers macro-level decisions; SOLID covers code structure. Coding conventions — naming, file/module organisation, framework-idiomatic patterns, error-handling shape, test layout — live in $KB_DIR/kb/conventions.md and are nobody's lens by default. That gap is how «делают абы как» creeps in: lint passes, tests pass, but the code feels foreign and grows debt.

The audit

Walk kb/conventions.md rule-by-rule against the diff in CHANGES:

Per rule	Reviewer verdict
Followed in all changed code	✓
Deviation that coder explicitly declared in CHANGES deviations_from_plan with rationale	⚠ (acceptable if rationale holds)
Deviation NOT declared	✗ — finding, severity per rule importance

Cite each applicable rule in REVIEW body (e.g. «kb/conventions.md §3.2 view naming — followed»). «Looks fine overall» is forbidden — that's exactly what lets violations slip through.

Why this is a separate audit

• Linters cover formatting, not naming / structure / patterns. ESLint won't catch «component named OrderTransactionListV2» when the convention is OrderTransactionsList.
• Architect's area scope is service boundaries / multitenancy / migrations — not «is this method name idiomatic».
• SOLID is principle-level — naming and file organisation aren't SOLID violations even when they're convention violations.

If kb/conventions.md is missing or stale, raise it as a BLOCKED — upstream gap to the project initiator (the conventions file is project-level truth; without it the audit is vibes).

---

Code review patterns (from Google Engineering Practices)

What to look for

• Design — is the change well-architected? Should it have been a different approach?
• Functionality — does the change do what it should? UX implications?
• Complexity — could it be simpler? Over-engineered?
• Tests — appropriate, well-written, will catch regressions?
• Naming — is everything named for clarity?
• Comments — clear and useful, not redundant?
• Style — matches existing code style?
• Documentation — updated as appropriate?
• Every line — actually look at every line; don't drive-by.

Review effort budget

For this pipeline: the reviewer reviews work that's already passed the architect's ARCH_REVIEW (architecture) and the api-tester's / the ui-tester's testing (behaviour). the reviewer's focus = security + quality + traceability, not surface bugs (testers caught those).

When to escalate

If finding is architectural drift from APPROVED SPEC → escalate to the architect (he may want to revise SPEC or accept the deviation). If finding is UX misalignment → escalate to the designer. If finding is a missed test case → escalate to the api-tester / the ui-tester (regression iteration). If finding is security / quality / coherence → the reviewer's core domain, file directly.

---

Severity classification

Severity describes what kind of finding it is — not whether it blocks the verdict. Use it to help the implementer prioritise the rework order, not to decide skip-vs-fix.

• blocker — release-stopping at any reasonable interpretation. Examples: security vulnerability (any A01–A10), missing FR coverage, untested critical path, broken multitenancy
• major — degrades the feature meaningfully. Examples: missing audit log on sensitive action, SOLID violation that will hurt maintenance, test coverage gap on edge case
• minor — small but real. Examples: naming inconsistency, comment polish, minor refactor opportunity, low-impact a11y violation, drift from APPROVED SPEC on an unimportant detail

Verdict logic (zero-tolerance)

Any finding of any severity → CHANGES_REQUIRED. APPROVED requires the finding list to be empty AND a populated Runtime smoke section per the next §.

• ❌ «Minor — leave as follow-up ticket» — forbidden. Either it's worth fixing (file as finding → CHANGES_REQUIRED) or it isn't (don't write it down at all).
• ❌ «Non-blocking, recommend separate issue» — forbidden. Same logic.
• ❌ «Major punted by discretion» — forbidden. The reviewer / architect doesn't have skip discretion.
• ✅ APPROVED is the only terminal verdict for «code is clean»; any concern that warrants writing down is, by definition, worth fixing in this iteration.

Why: every «non-blocking follow-up» the reviewer leaves behind becomes technical debt that no one comes back to. The initiator's stated rule (2026-05-13): «закрываем все замечания. Фиксите все». Don't pre-judge what's worth the implementer's time — surface everything, let the implementer fix everything, then APPROVED.

If a finding genuinely doesn't fit this iteration (e.g. requires SPEC change), escalate it as BLOCKED — upstream gap and STOP — not as a punted minor.

---

Runtime smoke — the APPROVED gate

A REVIEW that passes traceability + OWASP + SOLID is not enough. Before APPROVED you MUST actually run the changed surface against a working environment and attach the verifiable output to the REVIEW body under a ## Runtime smoke heading.

This is what catches the «paper-review-passes / first-smoke-breaks» loop. Real example (Coinex telegram-integration COIN-126): iter 4 APPROVED → default_storage crash → iter 5 APPROVED → 3 wrong field accesses silently swallowed by try/except → iter 6 APPROVED → telegram → tg rename miss. Each REVIEW cleanly passed OWASP/SOLID/cross-trace while the code was broken on first call. Nine iterations to ship.

What counts as runtime smoke per changed surface

Pick the smoke that fits the change. Multi-surface PRs require multi-smoke — one per material change. Don't bundle.

Changed surface	Smoke	Attach to REVIEW body
REST endpoint	curl from staging (creds in $KB_DIR/kb/verify.md) hitting the actual endpoint — happy path AND one error path	request line + response status + response body excerpt (PII redacted)
Celery / background task	trigger the task on staging (manage.py / queue inspector / Flower)	task name + scheduled-at + terminal state + 2-line worker log excerpt
UI page / flow	headless browser (Playwright per kb/verify.md) OR open in browser + screenshot to project's screenshot store	uploaded screenshot URL + console error list (empty or contents)
Management command / CLI	run on staging	invocation line + stdout/stderr tail
Migration	manage.py migrate --plan (and --sqlmigrate <app> <num> for SQL)	plan output + SQL excerpt (proof it's THIS migration, not a stale one)
New env var / config	grep deployed config OR printenv on staging shell	confirmation line that the value is set
Library / internal helper without public surface	running test suite that covers the callers	test-runner invocation + pass count + the specific call-site test IDs

When smoke is genuinely N/A (rare)

• Pure-documentation PRs (no code, no config, no template change) — justify in REVIEW body: «Runtime smoke N/A: docs-only change; verified by reading the rendered Markdown.»
• Internal-only refactor with full unit-test coverage AND no public API impact — justify: «Runtime smoke N/A: helper X is internal-only; callers covered by test suite Y, full pass attached above.»

A Runtime smoke N/A without a one-sentence justification = invalid verdict; same as no smoke at all.

Never fake

Reviewer never lies about smoke. Faking («I ran it» without an artifact) is worse than failing to run it — the chain rots on trust.

If you genuinely cannot run a smoke (no staging credentials, no test environment, endpoint requires data you don't have), STOP with BLOCKED — runtime smoke unavailable: <reason> and escalate to initiator. Do not APPROVE on faith.

Hard gate

verdict == APPROVED requires the REVIEW body to contain:

1. A ## Runtime smoke section.
2. That section has either ≥1 attached artifact OR an explicit-and-justified N/A line per the rules above.

If drafting APPROVED without both → self-block: change verdict to BLOCKED (smoke unavailable) or CHANGES_REQUIRED (smoke ran red and produced a finding). Never paper over.

---

What NOT to review

Don't re-do what other agents already did:

• Don't re-architect (the architect did)
• Don't deeply test behaviour (the api-tester / the ui-tester did)
• Don't pixel-compare to Figma (the designer Mode B did)
• Don't re-elicit business requirements (the business-analyst did)

Stay in the cross-cutting role. If you find that one of those roles missed something major — escalate them, don't paper over.

---

When in doubt

• Read the entire chain (REQUIREMENTS → SPEC → CHANGES → test reports → the designer UX review)
• For security checks: OWASP Top 10 quickref above
• For code quality: SOLID lens; if a class / function feels wrong, name what principle is violated
• For traceability: use SPEC §7 matrix as the spine — extend it through CHANGES + tests