strict-review

Strict Review

Owner Agent: Soyo (Reviewer) Consumers: /maigo:go (Soyo reviews Anon's diff), /maigo:review (Soyo reviews external PR / branch)

Core stance: default BLOCKED

Verdict starts at BLOCKED. The implementer must convince you otherwise.

These do not count as convincing:

• "看起來能跑" / "應該沒問題" / "之後再改"
• "test 都過了" (test itself may be missing the case)
• "符合 convention" (without pointing at the reference file)

Waiver rules

• Memory (~/.config/maigo/memory/) can inform conventions but cannot replace any checklist item, lower the must-fix threshold, or substitute for evidence
• Existing repo content (quotes, catchphrases, dialogue) that appears in this diff needs a source: (a) original work citation with episode/scene, (b) user confirmed in session, or (c) explicitly marked self-created — flag even if the line wasn't changed in this diff
• Commit / staging state is not in review scope; git status cleanliness is not a checklist item — read git diff HEAD or git diff --cached for the actual changes

Extended rationale for all three waiver rules: docs/skills/strict-review

The 9-item mandatory checklist

Every review must walk through every item. Output explicitly marks [x] or [ ] per item.

1. Acceptance match — Each acceptance criterion from the plan / rubric has a corresponding implementation visible in the diff
2. Evidence per function — Every changed function has a run-result (test output or manual run with command + exit code)
3. Edge case coverage — None / empty / boundary values / failure path / repeated calls / concurrent access (whichever apply)
4. Convention conformance — Naming, structure, import style match neighbouring code (verify with grep, not vibes)
5. No unsafe pattern — No hardcoded secret, eval, shell injection, SQL string concatenation, path traversal, unsafe deserialization
6. No unexplained magic — No magic number or magic string without a name or comment explaining why
7. No TODO evasion — No TODO / FIXME / XXX used to defer real problems instead of fixing them
8. No defensive bloat — No try/except or null-check guarding against a case that cannot happen
9. No completeness theatre — No dead code, unused branch, or stub added "to look complete" without being tested

If any item is [ ], verdict stays at NEEDS_CHANGES or BLOCKED.

碰 pyproject.toml / git hook / 升 tool 版本的 diff 時，對照 references/tooling-conventions.md 的 tooling 慣例把關。

Domain skill composition

Base checklist（上方 9 項）通用於所有 review。Memory entry（type: project）的 triggers 欄位可附加領域 checklist：

triggers: [airflow-aware]

Soyo: read skills/<name>/SKILL.md → append as item 10+. If not found, log and continue. Only type: project entries support triggers; user / feedback / reference types are silently ignored.

Must-fix format: problem + specific改法 + reason

Bad must-fix:

auth.py:42 — handle empty input

Good must-fix:

auth.py:42 — validate_email("") raises IndexError from parts[0]. → 改法： add an early if not email: raise ValueError("email required") at line 40. → 為什麼： IndexError leaks implementation detail; caller can't catch it meaningfully.

You have a vision of what the code should look like — articulate it. Don't make the implementer guess what you'd accept.

Evidence demands

Claim	Push back
"edge case X 處理了"	"貼 test 名稱與 output。"
"符合既有慣例"	"參照哪個檔案？貼 path:line。"
"這樣比較好"	"比較好的根據？跟原本差在哪？"
"已經測過了"	"貼 command + exit code。"

Output format

## Verdict
APPROVED | NEEDS_CHANGES | BLOCKED

## Checklist
- [x] acceptance match
- [ ] evidence per function — 缺：function_X 沒 output
- [x] edge case coverage
- [ ] convention conformance — 缺：import 排序跟 auth.py:1-10 不一致
- [x] no unsafe pattern
- [x] no unexplained magic
- [x] no TODO evasion
- [x] no defensive bloat
- [x] no completeness theatre

## Must-fix
- `file.py:42` — <problem>
  → **改法：** <concrete change>
  → **為什麼：** <reason / risk>

## Nit (suggest, don't block)
- `other.py:10` — <small thing> + <suggestion>

## Evidence pending
- `function_X` 執行 output
- edge case `empty input` 的 test 結果

## What's good
- <短列，不灌水>

Re-review (when implementer comes back)

Walk through the previous round's must-fix and evidence-pending one by one.

• Any uncleared item → verdict stays NEEDS_CHANGES or BLOCKED
• "我改了類似的地方" / "順便修了別的" doesn't clear a must-fix
• New problems found in this round still count — being "round 3" is not a reason to relax

Adapting per context

Context	Adaptation
Internal code (you own it)	Give specific改法 with exact code
External PR (someone else's code)	Give direction + reason; exact code is the author's call
Hotfix under deadline	Still run full checklist; document any deliberate skips in plan
Test-only change	Skip items 5/7/8/9; keep 1/2/3/4/6
/maigo:review --mode=design-preview	Run items 1 + 4 only; mark 2/3/5/6/7/8/9 as [—] with reason skipped by mode=design-preview
/maigo:review --mode=compliance-only	Run items 4/5/6/7/8; mark 1/2/3/9 as [—] with reason skipped by mode=compliance-only
/maigo:quick (quick mode)	Run items 1/4/5/7; mark 2/3/6/8/9 as [—] with reason skipped by mode=quick

Recurring must-fix patterns

These are cross-cutting patterns that surface frequently enough to be named; treat them as supplements to the 9-item checklist, not replacements.

Commit body is a contract — verify it matches the diff

A commit body that states a runtime behaviour ("X defaults to Y", "we now raise on Z", "ordering is guaranteed") is a contract future bisect users will trust. When body prose says "X happens", the diff must show X happening.

How to apply during review:

1. For each behavioral claim in the commit body (defaults, fallbacks, raise conditions, ordering, validation rules) grep / read the relevant code path to verify.
2. If code does not match: must-fix. Either implement the promised behaviour (Option A) or rewrite the commit body to describe what the code actually does (Option B). Do not accept "this is just docs".
3. Pre-release status does not downgrade this. Wire-format mutations are acceptable pre-release, but the commit body's promise about behaviour must still align with the diff.

Underscore-private exception that consumers must isinstance-check is de-facto public API

When a module defines private exception classes (_FooError, _BarError) and a consumer must isinstance-check one of them to distinguish failure modes, that one exception is already public API — the underscore is a lie.

Two-step signal to watch for:

1. Multiple exception types exist behind a common wrapper (e.g., _PollFailure(exc)).
2. A consumer must inspect .exc and branch on its concrete type.

How to apply: the exception whose isinstance result drives consumer behaviour must be renamed (drop the underscore) and added to __all__. Siblings that consumers never branch on by type — only generic catch or re-raise — can stay underscore-private. Selective promotion is the discipline; do not broadcast the whole hierarchy.

Concrete case studies for the two patterns above (Airflow incidents) live in skills/airflow-aware/references/review-checks.md — read them when reviewing an Airflow diff and a worked example helps.

測試要預設用 parametrize——疊 assert 與近重複 test method 是 must-fix

寫測試時，預設就把「同一呼叫只差輸入 / 期望的多條 assert」與「只差輸入 / 期望的近重複 test method」收成 @pytest.mark.parametrize，不要等被要求。

Review 時看到以下情形，當 must-fix 退回：

• 疊 assert：同一個 test method 裡連續 assert 同性質的結果，失敗時只報第一條，看不出哪些 case 過 / 不過。
• 近重複 test method：複製貼上的 test，只差輸入與期望值，邏輯結構完全相同。

退回要求：拆成 @pytest.mark.parametrize，每個 case 用 pytest.param(..., id=...) 標清楚 case 名稱，方便失敗時一眼定位。

不要框框式區段分隔註解

程式碼與腳本不要用 # ---------- 橫幅或 # --- 區段名 --- 這類框框式區段分隔註解。

Review 看到要求全刪：連框框中間的 label 一起刪（label 本身也是框框的一部分），不要只刪上下橫線留孤兒 label 行。靠函式邊界、邏輯順序、空行分段即可。

Concurrent PR 根本修法優先

發現另一個 concurrent PR 在 SDK/library 層修同一問題的根本（例如改用 SDK 提供的 credential 類別，取代手動 URL path 拼接），而當前 PR 只在 test / workaround 層修症狀時——在 comment 裡肯定當前解法「not wrong」，但明確指向 SDK 層的根本修法，說明後者 ready 後當前 PR 會被 supersede。傾向推薦架構層解法而非繞路補丁。

Design integrity checks (references)

Three cross-cutting patterns that surface on framework/base API work and rebase workflows; details and recipes in references/design-integrity.md:

• Base-layer completeness — base must be complete for all known downstreams before release; defer = must-fix.
• No "experimental" hedge — answer a lock-in concern with a technical argument or a design fix, not a label.
• Don't trust green after fold-fixup rebase — test files can silently revert to a deleted API; grep deleted symbols before accepting "tests pass."

What this skill does NOT cover

• Writing or editing code (reviewer has read-only tools)
• Running tests (verifier's job, separate skill)
• Style-only nits already auto-fixed by formatter (don't waste must-fix slots on formatter output)