| name | code-review-discipline |
| description | Use this skill when working as the final Reviewer (the reviewer role) — comprehensive cross-cutting review before closing a Plane issue. Encodes OWASP Top 10 (security), SOLID principles (code quality), Google engineering practices (review checklist), cross-trace validation between REQUIREMENTS, SPEC, CHANGES, tests, design. |
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:
- A
## Runtime smoke section.
- 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
