qc-code

Skill: qc-code

WHAT

Purpose and context

• Purpose: senior-style code review across four sections security · performance · naming · logic — each with a score/10 + biggest bug + how to fix — generate RED tests reproducing each logic bug, then issue an advisory verdict PASS/CẦN SỬA with JSON containing evidence.
• Trigger (when to use):
  • User wants a senior pair of eyes on the code before finalizing: "review code", "scan code", "score code".
  • Before committing a significant change — plug the option in /orca-workflow before verify-before-commit.
• Non-goals: NOT for: static security scanning (that is /orca-sec-scans — Trivy) or handling an incident that has already occurred (that is /orca-issue — repro-first). Not a general review of the /code-review built-in kind; not the entire codebase by default; the verdict does not block commits.

Mental model

phạm vi (diff từ base | file user nêu) → 4 mục chấm điểm → finding [nhóm][severity] → bug logic → test qc-* ĐỎ → verdict văn xuôi + qc-verdict.json (grounding-check) → test ghi vào dự án → qc-regression.py --run auto-chạy (0-token). Expensive = manual-call LLM review; cheap = deterministic tests via hook.

Review scope (default = current diff)

By default scan changes since base (git diff from the original commit/branch) — right before commit, fast, exactly what was just written. If the user specifies files/directories, scan those. Do NOT default to the entire codebase (slow, token-expensive, mostly unchanged).

Input and output contract

	Field	Required?	Meaning
In	scope	no (default = diff from base)	file/directory specified by the user
In	project test runner	yes if there is a logic bug	pytest/vitest/jest available
Out	4-section report	yes	each section score/10 · biggest bug · how to fix; performance adds the 3 slowest spots; naming adds a rename table
Out	test qc-<slug-bug>	each logic bug	placed in the project's standard test directory, RED on the current version
Out	verdict PASS / CẦN SỬA	yes	advisory, with a must-fix list
Out	harness/out/qc-verdict.json	yes	decision/claim/reason/required_evidence, via grounding-check exit 0

Rules and capabilities

• RULE-01 (MUST): Separate expensive/cheap: LLM review (this skill) = manual call / optional workflow step. Self-generated tests = auto-run via a deterministic hook. NEVER call an LLM in a hook (overstack's hook-0-token principle).
• RULE-02 (MUST): Verdict advisory, deterministic tests gate hard — do not let the LLM verdict block the commit.
• RULE-03 (MUST): Do not step on: /orca-sec-scans = static Trivy · /code-review built-in = general · /orca-issue = repro-first incident. /qc-code = senior 4-item-scoring + test generation.
• RULE-04 (MUST): Does the review have unknowns? If a claim depends on something uncertain (prod config, unseen runtime behavior) → record debt [[150726-unknown-ledger]] instead of asserting blindly.
• RULE-05 (MUST): Security is a trust boundary — do not be lazy here (CLAUDE.md carve-out).
• RULE-06 (MUST): Each finding is attached to exactly ONE group + ONE severity; severity follows the REAL IMPACT.
• Capabilities: read diff/code; write test files into the project + harness/out/qc-verdict.json; run the available test runner. No network.

Failure boundaries

• Cannot determine base/scope → clarify (ask for file/branch), do not default to the entire codebase.
• The project has no standard test directory → partial: state it clearly and put tests next to the source file, do NOT guess the structure blindly.
• grounding-check exit 2 → verdict NOT valid, must be rewritten; do not submit an ambiguous verdict.
• Claim depends on unknown → record debt in unknown-ledger, do not assert.
• No test qc-* yet → qc-regression.py fail-open.

HOW

Main workflow

Step	Type	Inputs	Action	Outputs/exit	Failure/next
W01	deterministic	base / file stated by user	Determine scope git diff <base>..HEAD --stat, read code	scope	ambiguous → clarify
W02	judgment	code	Score four items + attach group/severity (map of 13 groups)	4 items with all 3 parts	—
W03	effect	logic bug	Generate reproduction test qc-* for each bug	runnable RED test	not red → bug not proven, downgrade to suspicion
W04	judgment	results of W02–W03	Conclusion PASS / NEEDS FIX + record harness/out/qc-verdict.json	verdict + JSON	hook exit 2 → rewrite W04
W05	effect	test	Write tests into the project + remind qc-regression.py --run auto-run	test in repo	no test directory → next to source, state clearly

Details of each step (source of truth for W01–W05):

Steps

1. Determine scope — git diff <base>..HEAD --stat (or the file the user specified). Read the code within that scope.
2. Score four items (items below). Each item: score/10 · most severe error · how to fix. → Done when all four items have all three parts.
3. Logic item: generate reproduction tests for each bug (the "logic & bug" item below). → Done when each bug has a runnable red test.
4. Conclusion — PASS (move to the next step) or CẦN SỬA (list what must be fixed before passing). → Done when the verdict is clear + the must-fix list if NEEDS FIXING.
5. Write tests into the project (the "Write tests" item below) + remind qc-regression.py --run to auto-run them.

Four items

1. Security — score/10 · most severe error · how to fix

Look for: SQL injection (strings concatenated into queries, missing parameterization) · XSS (output not escaped, innerHTML/dangerouslySetInnerHTML) · exposed API keys / secrets (hardcoded keys, secrets in logs/repo) · input validation (accepting external data without checking) · authorization (missing checks for who-can-do-what, IDOR). This is the trust boundary — don't be lazy here (CLAUDE.md carve-out).

2. Performance — score/10 · most severe error · how to fix · 3 slowest points

Look for: N+1 queries (loop calling the DB per element instead of one query) · useless nested loops (O(n²) when O(n) suffices) · silent memory leaks (listeners/timers not removed, refs held forever, unbounded cache). Point out the 3 slowest points and how to optimize each one (with magnitude estimates: O(?), number of round-trips).

3. Naming & readability — score/10 · most severe error · rename table

Look for: names that don't say what they do (function getUser that writes to the DB, variable data meaningless) · consistent conventions (camelCase/snake_case mixed, plural/singular confused). Return a table:

Old name	New name	Reason
d	dueDate	single-character name says nothing

4. Logic & bug — score/10 · most severe error · TEST reproducing each bug

Look for: edge cases (null · empty · negative numbers · overflow) · off-by-one (< vs <=, last index) · race condition (shared state, interleaved awaits, non-atomic read-then-write). For each bug found → write a RED test case reproducing the bug (prove the bug is real, not suspected). A red test is a fact; a verdict is an opinion.

Map of 13 bug groups (gstack) + severity

Each finding maps to exactly ONE group + ONE severity, format: [<nhóm>][<severity>] <mô tả> — <cách sửa>. 5 CRITICAL groups are checked first:

#	Group (CRITICAL)	What to look for
1	SQL & data safety	String concatenation into queries (even if .to_i) instead of parameterizing; non-atomic check-then-set (TOCTOU); DB writes bypassing model validation; N+1 missing eager-load.
2	Race condition & concurrency	Read-check-write without a unique constraint; find-or-create missing an index → concurrent calls create duplicate records; status transition not using WHERE old_status=? atomic; raw HTML rendering on user data (XSS).
3	LLM output trust boundary	LLM-generated values (email/URL/name) written to DB without format validation; structured output without type/shape checks; LLM-generated URLs fetched without an allowlist (SSRF); LLM output into the knowledge-base without sanitization (stored prompt-injection).
4	Shell injection	subprocess/os.system with shell=True + string interpolation — use an argument array; eval/exec on LLM-generated code without a sandbox.
5	Enum & value completeness	Adding a new enum/status/tier value → MUST read code OUTSIDE the diff: every consumer switch/filter/display of sibling values, every allowlist %w[], every string case/if-elif — missing a single consumer is a silent bug.

8 INFORMATIONAL groups (checked later, leaning toward auto-fix): async/sync mixing (calling sync blocking inside async def, time.sleep instead of asyncio.sleep) · column/field-name safety (column names in ORM queries diverging from schema → silently empty) · LLM prompt (0-indexed lists in prompts, prompts declaring tools that don't match the code, limits declared in many places that easily drift) · type coercion (values changing type across the Ruby→JSON→JS boundary; hash/digest input not type-normalized) · view/frontend (inline styles re-parsed on every render, O(n·m) lookups in views, app-side filtering instead of WHERE) · time-window safety ("today" not covering 24h, two features using two different time-bucket styles for the same data) · completeness gaps (an 80-90% version when 100% would cost only a bit more code, tests missing negative branches that are easy to add) · distribution & CI/CD (tool versions in workflows diverging from the project, hardcoded secrets, tag v1.2.3 vs 1.2.3 mismatching, non-idempotent publishing).

Severity (distilled from awesome-skills/code-review-skill): [blocking] = must fix before merge · [important] = should fix, discuss if you disagree · [nit] = minor, up to the author · [suggestion] = an alternative worth considering, not mandatory. CRITICAL groups lean toward blocking/important; INFORMATIONAL groups lean toward nit/suggestion — but severity follows the REAL IMPACT of the finding, not the group mechanically.

Verdict

One of the two, with a reason:

• PASS — no severe error in any item, proceed to the next step.
• NEEDS FIXING — list specifically what must be fixed before passing (prioritize security + logic-with-red-test before naming).

The verdict is ADVISORY — the human decides, it does not block commit. What gates hard are the reproducing tests (red→green). Don't let the user think "qc-code PASS = absolutely safe"; it is a senior pair of eyes, not proof.

Verdict JSON + grounding-check

Along with the prose verdict above, also output ONE structured JSON block — "looks fine" is not feedback, it is schema-invalid and gets blocked deterministically:

{
  "decision": "revise",
  "claim": "paginate() bỏ sót phần tử cuối khi total % size == 0",
  "reason": "off-by-one ở điều kiện `<` tại api/paginate.py:42",
  "required_evidence": [
    "test qc-off-by-one-pagination chạy ĐỎ trên bản hiện tại",
    "api/paginate.py:42 sau khi sửa dùng `<=`"
  ]
}

• PASS → decision: "approve". CẦN SỬA → decision: "revise" + required_evidence is a list of SPECIFIC evidence to be added, each item pointing to file:line or a test-case name.
• claim and reason must not be empty; unknown fields only trigger a warning, not a fail (forward-compat).
• Write the JSON to harness/out/qc-verdict.json (inside the repo — NOT /tmp). The hook PostToolUse seeing a filename ending in qc-verdict.json will run automatically grounding-check right at write time; an invalid verdict blocks the tool-call (exit 2) and stderr is returned to fix in place.

It is not about remembering to type a command — the gate lies in the structure, not in the reminder. To check manually (files outside the repo, or someone else's verdict):

python3 harness/scripts/grounding-check.py --check <file.json>

exit 0 = valid; exit 2 = verdict NOT yet valid, must be rewritten — the agent may not submit a vague or evidence-lacking verdict.

Write reproducing tests into the project

Each test in the logic item:

• Place in the project's standard test directory — auto-detect: tests/ · test/ · __tests__/ · file *_test.py · *.spec.ts · *.test.js next to the source. If none → state clearly and leave the test next to the source file, do NOT guess the structure.
• Name qc-<slug-bug> (e.g. qc-off-by-one-pagination) — distinguish tests generated by qc-code, so qc-regression.py can collect them.
• Run with the existing runner of the project (pytest/vitest/jest) — do not spawn a new test framework.
• The test MUST be red before the fix (reproduce), green after the fix (proof). If the bug returns → the test goes red again (prevents regression).

Auto-run tests (deterministic, 0-token)

python3 harness/scripts/qc-regression.py --run runs exactly the tests qc-* and reports red/green — does not call an LLM. It auto-runs at verify-before-commit (before commit); enable PostToolUse too for instant feedback if desired. Fail-open if there is no test qc-* yet. This is the "auto hook when code is edited" part — hook only the cheap deterministic part, the LLM review (this skill) stays manually invoked.

Branches

ID	Kind	Guard	Behavior	Skip / failure	Rejoin
B01	user_optional	user specifies file/folder	scan exactly that scope instead of diff	—	W02
B02	conditional_required	logic items where bugs are found	W03 generates red tests for each bug	no bugs → skip W03, W05	W04
B03	user_optional	I am the one receiving findings (from a person, an LLM reviewer, including /qc-code)	process "Receiving review — verify before fixing" (Reference section)	wrong finding → technical response with evidence	fix each item, test each item
B04	capability_optional	want immediate feedback	additionally enable PostToolUse to run qc-regression.py --run	by default only runs on verify-before-commit	—

Validation and stopping

Deterministic: test qc-* red→green; qc-verdict.json must pass python3 harness/scripts/grounding-check.py --check <file.json> (hook runs automatically on write). Judgment: score and verdict (advisory). Stop when the four items have all three parts, each bug has a red test, verdict is valid.

Examples

• Positive: "qc code" on a diff that adds paginate() → 4 items are scored; logic finds off-by-one < at api/paginate.py:42 → test qc-off-by-one-pagination RED; verdict CẦN SỬA + JSON decision: "revise" has required_evidence pointing to the test and file:line; grounding-check exit 0.
• Boundary/failure: verdict JSON {"decision":"approve","claim":"","reason":"nhìn ổn"} → claim empty → hook blocks with exit 2, stderr returned, must rewrite; project has no tests/ → test placed next to the source file and clearly reported.

Reference — Receiving review — verify before fixing

The reverse direction of this skill: when I am the one receiving findings (from a person, from an LLM reviewer, including from /qc-code itself). A finding is a CLAIM, not yet the truth (distilled from obra/superpowers receiving-code-review):

1. Read all feedback before reacting — restate the request in your own words; wherever you do not yet understand, ASK before fixing any item (items may be related — understanding half means fixing wrong).
2. Verify the claim against the real code — run or read the exact section being pointed to: is the claim correct for THIS codebase, would fixing it break anything, does the current code have a reason to exist.
3. Only fix once you have seen the error yourself — if the finding is wrong, respond with technical reasoning backed by evidence, do not silently ignore it, and do not silently comply either. If you cannot verify it, say so plainly: "could not check because X is missing".
4. Fix one item at a time, test each item — do not lump them into one pass and hope.
5. No performance of agreement — "You're absolutely right!" / praising feedback instead of action is a sign of blind-complying. Confirm technically or start doing the work, don't perform.

Blind-complying with a wrong review creates new bugs wearing a "already reviewed" face — the hardest class of bug to suspect.

Origin

• Distilled from user request 2026-07-15 (qc-code 4 items + test generation + auto-hook). The decision of "where to connect" has asked the user → option 3 (manual LLM, deterministic auto-hook test), scope of the current diff.
• Absorb via /propose → 150726-qc-code-skill, task T-260715-04.
• Absorb 2026-07-17 (adapt_mode: dissolve, T-260717-02): map of 13 error groups distilled from garrytan/gstack (review/checklist.md); severity from awesome-skills/code-review-skill; the "Receive review" section from obra/superpowers (receiving-code-review). Clone depth-1 in scratchpad/, do not vendor bytes.
• Commit: (verify-before-commit fills in)