qa-code

qa-code

qareadhands-off

Use this when: a diff needs checking against our rules + architecture

Problem it solves — Working code can still violate the project's rules and architecture. This is the conformance pass: does the diff obey the documented conventions — DTOs over arrays, FormRequest validation, naming, Wayfinder — surfacing the deviations before they calcify.

Used in workflows: QA PR & Learn · Health Check

QA code (conformance to our rules)

Does this change follow how we do things here? Not "is it good code" in the abstract — does it obey the rules and architecture the project has already written down. Scoped to the diff only; play devil's advocate, but every objection cites a rule, not a vibe.

The corpus — what "our rules" means

• .claude/rules/* — code-quality.md (always) + the path-scoped rules for the files the diff touches (php-laravel, database, security, inertia, frontend, testing, skills).
• CLAUDE.md — Key Conventions, the testing/quality-gate policy, the architecture map.
• docs/adr/* — the standing decisions (e.g. ADR-0002 DB-driven scheduling).
• fyi-tempo-domain — Todoist priority mapping, the app/Data DTO layer, naming.

The conformance pass — this is the skill

Loading the corpus + the diff is setup; the value is mapping each deviation to the specific rule or ADR it breaks. For every changed unit (git diff <base>...):

• Rule adherence — does it honour the path-scoped rule for its area? (validation in FormRequests not controllers, named scopes, N+1s fixed, feature-first Pest…)
• Architectural fit — does it sit in the right layer? app/Data DTOs over arrays, thin controllers, Wayfinder for FE→BE, no Todoist data duplicated in the DB, sources idempotent via the import ledger, scheduling stays DB-driven (ADR-0002), jobs fail-and-notify (never silently skip).
• Security lens (always fire it when the diff touches models / controllers / requests / credentials) — $hidden on every encrypted attribute (no token leaking through JSON), validated() not $request->all(), $fillable set, DB-stored credentials via CredentialStore never plaintext, no secret in a log line or pushed payload. Lensed by .claude/rules/security.md. Judge each finding by the shared protocol in docs/security/review-lens.md: reachability / attacker-control / blast-radius before the severity label, boundary named from the threat model, corpus/accepted-risk matches excluded (note "excluded by corpus" or omit silently).
• Frontend lens (always fire it when the diff touches resources/js/** / resources/css/**) — utility-first Tailwind with the scale tokens, ConfirmDialog for any destructive action, component extraction over copy-paste, Inertia render-page + redirect-after-mutation, Wayfinder routes not hardcoded URLs. Lensed by .claude/rules/frontend.md + inertia.md.
• Naming — generic App naming, no "Tempo" in class names; the priority/label conventions.
• Readability / collections — the diff optimises for human-readable, low-cognitive-load code (the code-quality.md north star). Flag an array_filter/array_map/foreach-accumulator that would read with less load as a collect() pipeline — this is the one that keeps slipping through, so check it deliberately. Not dogmatic: only flag where the collection is genuinely clearer, never collections-for-their-own-sake.
• Convention drift — what-comments a name should carry, premature abstraction, deviating from the established pattern in the sibling files.

Scope discipline: only the diff. Don't audit untouched code (that's a wider refactor, not a conformance pass).

What NOT to flag — Laravel idioms that look like smells

A conformance pass is only as useful as its false-positive rate. These are intentional in this stack — flagging them is noise, not a finding:

• Eloquent models that are mostly data — an "anemic" model carrying casts()/$fillable/relations with little behaviour is the Laravel norm, not a domain-model smell. Behaviour lives in services/jobs by design here.
• app/Data DTOs as "data bags" — readonly value objects with public props are the documented DTO layer (fyi-tempo-domain), not primitive-obsession or a god-object.
• Thin wrappers over a vendor SDK — the Todoist/Anthropic/Google clients wrap an external API to absorb churn and stay mockable; a one-method passthrough there is the seam, not a needless middle-man.
• A Http::fake/Fake client in tests* — test doubles for external deps are required by the testing rules, not "mock abuse".
• Framework scaffolding — FormRequests, resource controllers, migrations, Wayfinder output: boilerplate the framework expects, not accidental complexity.

When unsure whether something is an idiom or a smell, check the sibling files — if the pattern is established in the area, it conforms.

Severity — tag every finding

Consequence before the label (every finding, not just security). State what it bites — the cost if shipped (a wrong-layer DTO that spreads array-shape coupling, an N+1 that scales with task count, a what-comment that rots) — before you reach for the severity word. This is the security lens's "evidence before severity" generalised: name the cost, then let it justify the tier, so the label is earned, not anchored.

• critical — security hole, data integrity / corruption, an ADR violated, or a broken invariant (e.g. Todoist data duplicated in the DB, a leaked credential, a silently-swallowed job failure). Fix before the PR, no exceptions.
• major — a real rule break that will bite (N+1 on a hot path, validation in the controller, missing $hidden, FE bypassing Wayfinder). Fix before merge.
• minor — style / convention drift (a what-comment, a name, a small premature abstraction). Fix or note as follow-up.

A critical or major finding gates the PR; minors can ride or defer.

Output — surface, then fix

A table: deviation · consequence (what it bites) · severity · the rule/ADR it breaks (cite it) · file:line · suggested fix, plus any architectural concern called out separately.

When blind-reviewer is in the chain, its CANNOT_VERIFY findings get their own named bucket in the report — surfaced to the human with the attempted verification and the reason it couldn't be completed. They never auto-block (that's for FAIL), and they are never silently dropped.

Then AskUserQuestion which fixes to apply; apply only the confirmed. A clean diff is a valid result — say "conforms" and stop.

Where it sits

• Not check-reasoning — that red-teams whether the decision/approach is sound (yours or mine); this checks whether the code follows our documented rules. Conforms = "does it follow the rules", reasoning = "is it the right call".
• Not qa-tests (missing tests) or /code-review (generic reuse/simplify). This is conformance to Tempo's own standards.
• Not design-taste-frontend — that judges visual design quality; the frontend lens here checks code conventions (Tailwind tokens, ConfirmDialog, Wayfinder), not taste.
• It doesn't touch the composer ci:check gate — it's the judgment layer that catches what a linter can't (architecture, ADRs, conventions).

How this gets triggered

The suggest-skills.sh hook nudges it after a green composer ci:check (before commit). Invoke directly — "does this conform?", "check it against our rules".