Code Review

3. Review for issues

Check every changed file for the following layers. Skip layers that don't apply to the change.

Security (blockers if found):

• No secrets, tokens, or credentials in code
• When persisted configuration is copied into UI or session metadata, trace it through the applicable sanitizer/redaction boundary; storage-safe values are not automatically presentation-safe.
• Input validation at system boundaries (user input, API handlers, external data)
• No SQL injection, XSS, command injection, or path traversal risks
• Authentication and authorization checks in place for new endpoints
• No insecure crypto (MD5/SHA1 for passwords, weak random)
• Workspace and office boundaries are enforced; no cross-workspace data, credentials, logs, or agent context leakage
• Agent/tool execution is constrained by code, not prompt text alone

Architectural fit (highest priority):

• Changes belong in the correct layer/module and follow the dependency direction used by the codebase
• Business/domain logic is not placed in controllers, transport handlers, repositories, data sources, or infrastructure code
• Controllers handle protocol concerns, use cases orchestrate workflows, repositories define persistence needs, and data sources handle external systems
• Domain/application code does not depend on frameworks, transport models, database models, or vendor-specific types
• Changes do not bypass existing boundaries, duplicate responsibilities, or introduce unnecessary coupling between modules or domains
• New interfaces and abstractions have clear ownership and represent a meaningful boundary, rather than wrapping a single implementation
• Compare with neighbouring features and established patterns, but flag deviations only when they create a real architectural or maintainability problem
• Treat fundamental architectural misplacement or broken dependency direction as a blocker
• Frontend: no direct data fetching in components (must go through store), shadcn imports from @kandev/ui not @/components/ui/*
• Backend: provider pattern for DI, context passed through call chains, event bus for cross-component communication
• Search docs/specs/ and docs/decisions/ for the affected subsystem; flag an accepted spec or ADR that the change makes inaccurate
• New abstractions justified — no over-engineering
• Concerns cleanly separated (single responsibility)

Data & state modelling:

• Domain entities, value objects, DTOs, persistence models, and external API models remain separate where their responsibilities differ
• State transitions and invariants are explicit and cannot create invalid or partially updated state
• There is a single clear source of truth; state or business rules are not duplicated across layers
• Nullability, optional fields, defaults, and invalid combinations are modelled deliberately
• Persistence schemas or transport types are not leaking implementation details into domain/application contracts
• Persistence conformance tests call real production stores and assert a non-zero domain write/read-back; synthetic tables and no-op SQL are not semantic coverage. Startup tests trace errors through bootstrap and auth/middleware order, not readiness alone.
• Concurrency, retries, partial failures, and duplicate requests cannot corrupt state or apply transitions more than once
• Backward compatibility, migrations, and mixed-version behaviour are considered when contracts or persisted data change

Logic & correctness:

• Edge cases handled (empty input, nil/null, zero, max values)
• Error paths covered and not silently swallowed
• For marker-delimited full-document mutations, define missing, orphaned, duplicate, and malformed-marker behavior before writing. Cleanup must no-op or fail closed when the body is not owned, and tests must cover each ownership boundary without retrying an unowned document.
• For serve-time HTML rewriting or script injection, verify browser document context rather than token presence alone. Valid HTML may omit html, head, and body, and a script token may occur inside inert template or foreign content. Cover scriptless omitted-wrapper documents and inert-content cases, preserve stored artifact bytes, and verify that the served bootstrap executes.
• Race conditions or concurrency issues in concurrent code
• Async events carry an immutable identity when they can outlive the operation that created them; stale events cannot mutate a replacement operation
• Locks protect only the atomic ownership boundary and are not held across unbounded I/O or a full asynchronous operation
• When adding an RPC deadline, trace the actual transport, server operation, and cleanup before the response. Compare nested timeout budgets, including context.WithoutCancel cleanup; test successful work followed by stalled cleanup. A client timeout or removed response-correlation ID does not stop remote work or independent stream events, so fence late effects before admitting successor work.
• Synchronous callbacks cannot re-enter a lock they already need; moving publication asynchronous also requires an immutable value snapshot, clear shutdown ownership, and protection against a delayed event changing successor state
• When a generation, token, or lease authorizes a side effect, validate and mutate within one critical section. Check every terminal path separately: success, raw error, cancellation, timeout, and disconnect.
• Detached goroutines have immutable snapshots and a real happens-before relationship before reading state that can otherwise transition underneath them
• When a system design requires telemetry for an external or provider operation, audit every early return and require exactly one completion outcome per invocation. Outcomes should use allowlisted stable IDs, duration, response shape, and typed failure category, while excluding URLs, request/response bodies, credentials, and raw upstream errors; add observer/logger assertions for success and representative timeout and invalid-descriptor failures.

Performance:

• No N+1 queries (loop with individual DB calls)
• No memory leaks (unclosed connections, streams, listeners)
• Missing database indexes for new query patterns
• Algorithm complexity appropriate for the data scale

Complexity limits (CI also enforces these, but catch them early to avoid pushing and waiting):

• Go: functions ≤80 lines, ≤50 statements, cyclomatic ≤15, cognitive ≤30, nesting ≤5
• TS: files ≤600 lines, functions ≤100 lines, cyclomatic ≤15, cognitive ≤20, nesting ≤4
• If too large or complex, split into smaller cohesive files/functions

Code quality:

• No duplicated logic — extract shared helpers or constants
• No dead code, unused imports, or commented-out code
• Check for orphaned code: if the PR refactored or removed callers, grep for functions/types/exports that lost their last consumer
• No speculative code — unused flags/options, "reserved for future" scaffolding, one-off abstractions with a single call site, options parsed but never used
• Naming clear and consistent with project conventions
• Deep nesting (>3 levels) — use early returns

Build and platform boundaries:

• For changed Makefiles, shell scripts, or CI path filters, trace each changed target through the shell and platform branches. Distinguish executable naming from recipe-shell syntax; inspect OS, MSYSTEM, and SHELL assumptions.
• When simulating a Windows Make branch from POSIX, prefer scripts/check-make-shells. If a manual make -n is necessary, neutralize its parse-time probes as that checker does (NULL_REDIR= BUILD_TIME=simulated) so POSIX does not create NUL artifacts. Compare git status --short with the initial snapshot afterward.
• Use make -n <changed-target> for every affected platform branch that is available, and confirm CI invokes the changed target. Include docs or configuration paths when a validator or test reads them.

AI slop detection:

• Comments that restate code or narrate obvious steps
• Unnecessary try/catch that swallow errors or return silent defaults in trusted internal paths
• Redundant validation where inputs are already parsed/typed
• as any or as unknown as X casts used to dodge type errors instead of fixing types
• Defensive checks abnormal for the area of the codebase — compare with surrounding code patterns

Testing (blocker if missing):

• Backend (Go): new or changed functions/methods must have corresponding *_test.go tests
• Frontend: new utilities, hooks, API clients, and store slices must have focused tests. Pure React markup may skip a unit test, but behavior-bearing components (conditional status, accessibility, store-derived state, or responsive/mobile variants) need focused *.test.tsx coverage and/or E2E. Route responsive user-facing changes through /mobile-parity.
• For conditional UI driven by store-derived data, trace branch predicates through real callers and use production-shaped props. Include required identity props and seed the empty store/data state; do not simulate "no data" by omitting props that callers always pass.
• Async UI lifecycle: loading/busy state must clear through finally or equivalent terminal cleanup for success, error, cancellation, and early/no-op returns; require focused tests for those terminal paths.
• Exceptions: config files, generated code, and pure React component markup
• Missing tests for new or changed logic is a blocker — suggest what tests to add and recommend /tdd

4. Report

When the user says not to post or modify the PR, do not make any GitHub mutation: no fixes, comments, review submissions, or thread resolution. When the user asks for a review only, or when reviewing an external contributor's branch, do not edit the checkout or push code; report findings through the channel the user requested. Do not submit or resolve reviews unless explicitly asked.

Before a read-only review ends, compare git status --short with the initial snapshot. Remove only diagnostic artifacts demonstrably created during the review; preserve all pre-existing user changes.

Report findings with a concrete suggested fix. Do not edit the checkout during a review-only request; otherwise remediate in the same primary conversation.

Before drafting or sending an author-facing review finding through any channel, including a PR comment or message_task_kandev, map each point against exact-current-head review bodies, top-level discussion comments, and all unresolved or hidden threads. If a point is already raised, omit it from the author-facing delivery but retain it in the private review summary; repeat it only when the user explicitly asks for reinforcement. Re-fetch immediately before delivery and start a new review round if headRefOid changed. Name the exact reviewed SHA in the outgoing finding.

When handing findings to another Kandev session, include the file and line, severity, concrete fix, and targeted verification.

5. Output

Use this format:

---

Findings

Blocker (must fix before merge)

Security holes, data loss risk, broken logic, crashes, missing tests for new/changed logic

1. [Title] — file.go:42
   • Issue: what's wrong
   • Why: why it matters
   • Fix: concrete suggestion or code snippet

Suggestion (recommended, doesn't block)

Performance problems, poor error handling, architectural concerns

Summary

Verdict: Ready to merge / Ready with suggestions / Blocked — fix blockers first

---

Rules:

• Only report findings you're >=80% confident about — quality over quantity
• Don't mark style preferences as blockers — linters cover formatting
• Every criticism needs a suggested fix
• Say when uncertain and recommend a specific investigation instead of guessing
• Don't give feedback on code you didn't read
• Omit empty severity sections

Not a finding (skip these):

• Pre-existing issues on lines the change didn't modify
• Things linters, typecheckers, or CI already catch (imports, types, formatting) — exception: still report complexity-limit violations since they require code changes to fix