refactor(arch-code-review): streamline skill and add trigger eval set

Cut rigid scoring (deduction points, fixed weights, renormalization), the mandatory 7-section report format, and numeric issue caps that burdened modern agents without improving judgment. Kept the core: minimum closed loop, principal contradiction, docs > code, facts > claims, stop rules, severity-based merge gate.

SKILL.md 266 -> 92 lines (~50% smaller); evals.json drops references to removed constraints; evals/trigger-set.json adds a derived {query, should_trigger} set for run_eval.py (5 positive + 5 near-miss).

skills/arch-code-review/SKILL.md

@@ -1,266 +1,92 @@
 ---
 name: arch-code-review
-description: Personal architecture-first merge-decision code review skill. Use only when the user explicitly asks for architectural code review, PR review, merge review, architecture review of a diff, branch review, or asks whether a change is mergeable. This is a heavy review skill: it inspects the diff against project documentation, infers the minimum closed loop, focuses on principal contradictions, runs relevant validation as evidence, and produces an English Markdown report with severity, deduction scoring, and merge gate judgment. Do not use for ordinary implementation, debugging, linting, refactoring, or casual "look at this code" requests unless the context is clearly pre-merge review.
-argument-hint: "<optional PR URL, diff, file path, base branch, reference implementation, or review focus>"
+description: Architecture-first merge-decision review. Use only on explicit request to review whether a diff/PR/branch can be merged. Infers the minimum closed loop this change must complete, inspects the diff against authoritative docs, gathers validation evidence, and decides mergeability. Not for ordinary coding, debugging, refactoring, or casual code reading.
+argument-hint: "<optional PR URL, diff, file path, base branch, or review focus>"
 ---
 
 # /arch-code-review
 
-Run an architecture-first merge decision review. The goal is not to list every possible nit. The goal is to decide whether this change completes the current minimum mergeable loop.
+An architecture-first **merge decision**. Decide whether this change completes its minimum mergeable loop. Do not list every nit.
 
-Default output language is English.
+Default output language: English. Prefer prose over Markdown tables.
 
-Do not use Markdown tables in the report.
+## When to use
 
-## Trigger discipline
+Only for explicit review requests — "review this PR", "can this be merged?", "architecture review of this diff", "review before merge". Not for ordinary coding, debugging, refactoring, or casual "look at this code".
 
-Use this skill only for explicit review requests, such as:
+## Mindset
 
-- code review
-- PR review
-- merge review
-- architecture review of a diff
-- review this branch / PR / diff
-- check whether this change can be merged
-- review before merge
+Judge every finding against the **minimum closed loop**: the smallest end-to-end behavior, boundary, data flow, or lifecycle this change must complete to be mergeable. State that loop at the top of the report, then report only **principal contradictions** that affect it. A correct-but-unimportant detail is noise; a small issue becomes reportable only when it reveals systemic risk or breaks the loop.
 
-Do not use this skill for ordinary coding work, normal debugging, casual code explanation, generic refactoring advice, or implementation tasks. This skill is intentionally heavy and should not hijack day-to-day development.
+Two authorities:
 
-## Review target
+- **Documentation > implementation.** Read the relevant docs first (README, `docs/`, `ARCHITECTURE.md`, `CONTEXT.md`, ADRs, module docs). If docs say X and code does Y, the implementation deviates. You may flag suspected-stale docs, but do not invert the judgment without user confirmation.
+- **Facts > claims.** Findings rest on code, diff, and validation output — not comments, TODOs, or PR descriptions. A finding with no code/validation fact is a follow-up observation at most.
 
-If the user provides a PR URL, explicit diff, file path, zip, architecture document, reference implementation, or base branch, use that input.
+## Inputs
 
-If no explicit target is provided, review the current repository diff:
+If the user gives a PR URL, diff, file, zip, doc, reference implementation, or base branch, use it. Otherwise review the current repo diff: full branch diff vs `main`/`master`, falling back to staged + working tree diff. If no diff is obtainable, ask.
 
-1. Prefer the full current branch diff against `main` or `master`.
-2. If the base branch cannot be detected, fall back to staged plus working tree diff.
-3. If no diff can be obtained, stop and ask for the review target.
+No review config file. Honor only parameters in the current request (output language, base branch, depth, reference implementation, whether to evaluate tests).
 
-This skill does not support a separate review config file. Accept only explicit parameters in the current user request: output language, base branch, review depth, minimum closed loop, reference implementation path, or whether to evaluate tests.
+## How to review
 
-## Core principles
+1. **Gather facts.** Read the diff, changed files, and nearby dependency/boundary code. Find and read the docs that govern the changed area. Track any doc/code conflict.
+2. **State the minimum closed loop.** Infer it from the diff, docs, directory structure, changed tests, public APIs, and execution paths. Say explicitly: *"This review's minimum closed loop is X."*
+3. **Inspect through the lens that matters for this change** — do not mechanically cover all of them:
+   - data structure, ownership, and flow
+   - lifecycle and destruction
+   - performance
+   - abstraction boundary
+   - tests (only when in scope — see below)
+4. **Gather validation evidence.** Detect project type and run the cheap checks first (lint, typecheck, related tests), broadening to full tests if useful. Validation is evidence, not a replacement for reading the code.
 
-### Principal contradiction principle
+**Stop instead of forcing a review** when the loop cannot be inferred, multiple unrelated loops exist and the user did not pick one (list the candidates and ask which is primary), required comparison material is inaccessible, the repo/diff cannot be read, docs and the stated goal fundamentally conflict with no basis to choose, or there are not enough facts. Output *"Cannot complete formal review"* and state what is missing. Do not invent findings.
 
-Report only issues that matter to the current merge decision. A correct but unimportant detail is noise. A small issue becomes reportable only when it reveals systemic risk or breaks the current minimum closed loop.
+## Severity
 
-### Minimum closed loop principle
+- **blocker** — must fix before merge. Breaks a core boundary or data flow, leaves lifecycle/ownership unprovable, makes the core flow wrong or non-executable, violates authoritative docs, or masks truth in tests/validation. Any blocker ⇒ not mergeable.
+- **high** — strongly recommended; several can make the change practically unmergeable.
+- **medium** — can merge if the loop is otherwise intact; should be scheduled.
+- **low** — usually omit; include only when it reveals systemic risk or affects the loop.
 
-Before formal review, identify the current project stage's minimum closed loop: the smallest end-to-end behavior, architecture boundary, data flow, lifecycle, validation path, or user-visible slice that this change must complete to be mergeable.
+## Validation and tests
 
-Judge every formal issue by whether it breaks that loop. This skill is not a static analyzer. It is a merge-readiness judgment.
+Grade validation failures by **impact on the minimum closed loop**, not mechanically. A failure that breaks compilability, the core path, data truthfulness, lifecycle safety, or validation credibility is blocker/high. A lint/format failure that does not touch the loop is medium or follow-up. Passing validation is evidence, not proof of mergeability.
 
-### Documentation authority principle
+Do not penalize missing tests by default — tests are often written after the loop stabilizes. Evaluate the test suite only when the user asks for it, the loop *is* a test/validation loop, the tests changed substantially, or test scaffolding distorts the loop. Otherwise note the test dimension as "not evaluated this round". Temporary markers (`FOR TEST`, fixtures, debug helpers) are not automatically issues.
 
-Project documentation is authoritative over implementation unless the user explicitly says otherwise.
+## Merge gate
 
-Read relevant docs before judging code. Prefer explicit user-provided docs, then repository docs such as README, `docs/`, `ARCHITECTURE.md`, `CONTEXT.md`, ADRs, design docs, module docs, and nearby package docs.
+Mergeable only when there is no blocker and the principal contradictions around the minimum closed loop are resolved. State the gate result and the reasoning (blockers, severity mix, validation facts). Judgment decides — a clean surface never makes code mergeable on its own.
 
-If docs say X and code does Y, default judgment is that the implementation deviates from requirements. If docs appear outdated, say "suspected stale docs" but do not invert the judgment without user confirmation.
+## Output
 
-### Fact over claims
-
-Use code, diff, validation output, and documentation facts. Comments, PR descriptions, TODOs, and human claims are useful context but cannot replace code facts.
-
-A formal issue without code or validation facts is not a formal issue. Put it in follow-up observations only if it may become a principal contradiction later.
-
-## Review depth
-
-Default depth is merge decision review:
-
-- Decide whether the current diff reaches a mergeable minimum closed loop.
-- Do not audit the entire repository or expand into old technical debt.
-- Inspect historical or surrounding code only when it is a dependency, boundary, reference implementation, or documentation constraint for this diff.
-
-If the user explicitly asks for architecture audit, expand cross-module tracing as needed, but formal issue volume remains capped except for blockers. Still preserve the principal contradiction principle.
-
-## Stop rules
-
-Stop before producing a formal review report when a valid merge decision cannot be made.
-
-Output "Cannot complete formal review" and list the missing inputs when:
-
-- the minimum closed loop cannot be inferred;
-- the diff contains multiple unrelated possible closed loops and the user did not specify the primary one;
-- the user requires comparison with a PR, zip, reference implementation, or document that is inaccessible;
-- the repository or diff cannot be read;
-- project documentation and the user's stated goal fundamentally conflict and there is no basis for choosing authority;
-- there are not enough facts to support formal issues.
-
-If multiple possible loops exist, list the candidate loops and ask the user to choose one. Do not review all of them simultaneously.
-
-## Workflow
-
-1. Gather the review target.
-   - Read the diff, changed files, and nearby code.
-   - Detect the base branch if needed.
-   - Read explicit user-provided context first.
-
-2. Read project requirements.
-   - Search for relevant architecture and requirement docs.
-   - Extract the requirements that govern the changed area.
-   - Track any doc/code conflict.
-
-3. Infer the minimum closed loop.
-   - Use the diff, docs, directory structure, changed tests, entry paths, public APIs, and execution paths.
-   - At the start of the report, state: "I believe this review's minimum closed loop is X, so I judge principal contradictions around X."
-
-4. Review the code through the scoring directions.
-   - Data structure, management, and flow
-   - Data ownership, borrowing, and destruction
-   - Performance
-   - Abstraction level and abstraction form
-   - Test suite, only when test evaluation is in scope
-
-5. Run validation as evidence when feasible.
-   - Detect project type and scripts.
-   - Prefer low-cost checks first: lint, typecheck, related tests.
-   - In large projects, run related tests before all tests.
-   - Treat validation as fact gathering, not a mechanical replacement for review.
-
-6. Produce the report.
-   - Default English.
-   - No Markdown tables.
-   - No generic praise section.
-   - At most seven formal issues, except blockers are uncapped.
-
-## Validation failure rule
-
-Validation failures are graded by their effect on the minimum closed loop.
-
-- If a failure breaks compilability, core path execution, data truthfulness, lifecycle safety, or validation credibility, it enters formal issues as blocker/high/medium according to impact.
-- If related tests fail and expose a core flow error, classify as blocker or high.
-- If lint or formatting failures do not affect the loop, they are medium or follow-up at most.
-- If a validation failure blocks official CI merge, it is at least high. If it makes the code undeliverable, it is blocker.
-- Passing validation is evidence, not proof of mergeability.
-
-## Test evaluation rule
-
-Do not penalize missing tests by default.
-
-Tests are often written after the minimum closed loop stabilizes. If the current loop is not about tests/validation, and the diff has no substantial test changes, mark the test dimension as "not evaluated in this round".
-
-Evaluate the test suite only when:
-
-- the user explicitly asks for test evaluation;
-- the minimum closed loop is a test or validation loop;
-- test code changed substantially;
-- test helpers, temporary markers, or test-only paths affect the truthfulness, maintainability, architecture boundary, or validation credibility of the current loop.
-
-Temporary markers such as `FOR TEST`, debug helpers, fixtures, or test scaffolding are not automatically issues. They matter only if they distort the current closed loop.
-
-## Severity levels
-
-Use exactly these severity levels:
-
-### blocker
-
-Must fix before merge. Examples:
-
-- violates authoritative architecture docs;
-- breaks a core boundary or core data flow;
-- leaves lifecycle, ownership, or destruction unprovable;
-- makes the core flow incorrect or non-executable;
-- masks truth in tests or validation.
-
-Any blocker makes the change not mergeable regardless of score.
-
-### high
-
-Strongly recommended before merge. Multiple high issues can make the change practically unmergeable even without a blocker.
-
-### medium
-
-Can merge if the minimum closed loop is otherwise intact, but should be scheduled.
-
-### low
-
-Normally omitted. Include only when it enters the formal issue list because it affects the minimum closed loop or reveals systemic risk.
-
-## Scoring model
-
-Use deduction scoring.
-
-Each evaluated direction starts from 100. Only formal issues that affect the current minimum closed loop deduct points. Follow-up observations do not deduct.
-
-Deduction defaults:
-
-- blocker: deduct 20 points and automatically mark not mergeable
-- high: deduct 10 points
-- medium: deduct 5 points
-- low: deduct 2 points only if included as a formal issue
-- follow-up observation: deduct 0 points
-
-Avoid double counting. Merge the same root cause across multiple manifestations and deduct once unless it truly damages separate scoring directions. Scores cannot go below 0.
-
-Default weights:
-
-- Data structure, management, and flow: 0.30
-- Data ownership, borrowing, and destruction: 0.20
-- Performance: 0.20
-- Abstraction level and abstraction form: 0.15
-- Test suite: 0.15, only when evaluated
-
-If a direction is intentionally not evaluated, exclude its weight and renormalize the remaining weights for the total score. Still output the total score and explicitly list excluded dimensions with reasons.
-
-Merge gate:
-
-- every evaluated direction must be at least 60;
-- the weighted total over evaluated directions must be at least 60;
-- there must be no blocker issues.
-
-A score alone never makes code mergeable.
-
-## Formal issue format
-
-Each formal issue must include these fields:
-
-- Severity
-- Affected direction
-- Code/validation facts
-- Why it breaks the minimum closed loop
-- Repair direction
-- Deduction
-
-Every formal issue must include a short repair direction. Keep it strategic, not a patch. Prefer explaining the data flow, ownership/lifecycle boundary, abstraction boundary, scheduling/state-machine constraint, or validation change that should be corrected. Do not output full patches unless the user explicitly asks.
-
-Do not recommend adding logs, comments, or renaming variables unless that directly affects the current minimum closed loop.
-
-## Report structure
-
-Use this structure exactly. If a section has no content, write "None". Do not omit gate information.
+A focused report that lets a maintainer make the call. Suggested shape — adapt to the change, but always state the loop and the gate:
 
 ```markdown
 # Architecture Code Review
 
-## 1. Minimum closed loop judgment
-[State the inferred minimum closed loop and why this is the loop. If not inferable, stop instead of using this report.]
+## 1. Minimum closed loop
+[the inferred loop and why; or "Cannot complete formal review" + what's missing]
 
-## 2. Overall conclusion and merge gate
-[Mergeable / Not mergeable / Needs user decision for policy-only cases. Missing loop, missing facts, or missing required inputs must use the stop rules instead. Explain using blocker presence, direction scores, total score, and validation facts.]
+## 2. Merge decision
+[Mergeable / Not mergeable / Needs user decision, with reasoning]
 
-## 3. Scoring and deduction notes
-[State evaluated dimensions, excluded dimensions with reasons, per-direction scores, weighted total, and deduction root causes. No Markdown tables.]
+## 3. Findings
+[Each: Severity · affected area · code/validation facts · why it breaks the loop · repair direction.
+Lead with blockers and highs. Give a strategic repair direction, not a full patch unless asked.]
 
-## 4. Formal issues
-[Maximum seven formal issues, except blockers are uncapped. Each issue follows the fixed format.]
+## 4. Validation evidence
+[commands run + results + how failures affect the loop; or why not run]
 
-## 5. Validation results
-[Commands run, results, and how failures affect the closed loop. If not run, explain why.]
+## 5. Doc/code conflicts
+[docs vs implementation; or None]
 
-## 6. Documentation/code conflicts
-[Conflicts between authoritative docs and implementation. If none, write None.]
-
-## 7. Follow-up observations
-[At most three restrained observations. Only include current non-blockers that may become principal contradictions in the next stage. No style preferences or micro-optimizations. No score impact.]
+## 6. Follow-ups
+[few restrained notes — current non-blockers that may become principal contradictions later; no gate impact]
 ```
 
-Do not include a standalone "Strengths", "What looks good", or praise section. If a key design is valid and affects the merge judgment, mention it briefly in the overall conclusion.
+No generic praise section. If a key design is valid and matters for the gate, say it briefly in the decision. Be direct and evidence-based — no flattery, no insults.
 
-## Large blocker handling
-
-If blockers exceed seven, do not expand every issue. State that the code is broadly unmergeable, detail the most representative blockers, and summarize the remaining blockers by category.
-
-## Output tone
-
-Be rational, direct, and code-fact based. Do not flatter. Do not insult. The report should be understandable to a human maintainer and strict enough to support a merge decision.
+If blockers are numerous, do not enumerate every one: call the change broadly unmergeable, detail the representative blockers, and summarize the rest by category.

skills/arch-code-review/evals/evals.json

@@ -4,13 +4,13 @@
     {
       "id": 1,
       "prompt": "Review the current branch against main and decide whether it can be merged. Use the default English output. Do not judge only by whether tests pass; first identify this change's minimum closed loop.",
-      "expected_output": "An English merge-decision review that infers the minimum closed loop, reads project docs, reports merge gate status, uses deduction scoring, avoids Markdown tables, and does not include a generic praise section.",
+      "expected_output": "An English merge-decision review that infers and states the minimum closed loop up front, reads project docs, gives a merge gate decision based on judgment (not tests alone), and reports focused findings with code facts.",
       "files": [],
       "expectations": [
-        "The report states the inferred minimum closed loop before formal issues.",
-        "The report includes a merge gate decision that is not based on score alone.",
-        "The report uses the seven required sections and no Markdown tables.",
-        "The report excludes the test dimension when tests are not in scope and explains the excluded weight."
+        "The report states the inferred minimum closed loop before the findings.",
+        "The report gives a merge gate decision with reasoning, not based on test results alone.",
+        "The report is a focused merge-decision review in prose, not Markdown tables.",
+        "The report handles the test dimension sensibly — evaluates it when in scope, or notes it as not evaluated with a reason."
       ]
     },
     {
@@ -21,7 +21,7 @@
       "expectations": [
         "The review searches for and applies relevant architecture documentation before judging code.",
         "Documentation/code conflicts are treated as implementation deviations unless explicitly marked as suspected stale docs.",
-        "Formal issues include code or validation facts, closed-loop impact, repair direction, and deduction.",
+        "Each finding includes code or validation facts, the closed-loop impact, and a strategic repair direction.",
         "Style-only issues are omitted unless tied to systemic risk."
       ]
     },
@@ -33,8 +33,8 @@
       "expectations": [
         "The skill stops formal review when multiple unrelated loops are present and no primary loop is specified.",
         "The response lists candidate minimum closed loops.",
-        "The response asks for the missing decision rather than producing a forced score.",
-        "The response does not invent formal issues without sufficient facts."
+        "The response asks for the missing decision rather than producing a forced review.",
+        "The response does not invent findings without sufficient facts."
       ]
     },
     {
@@ -45,7 +45,7 @@
       "expectations": [
         "The output is in English by default.",
         "The review treats validation output as evidence and classifies the failure by closed-loop impact.",
-        "The review does not automatically mark every validation failure as blocker.",
+        "The review does not automatically mark every validation failure as a blocker.",
         "The merge gate explanation still accounts for validation facts."
       ]
     },
@@ -57,8 +57,8 @@
       "expectations": [
         "The arch-code-review skill should not trigger for this prompt by description alone.",
         "The prompt is treated as ordinary coding or debugging help, not a merge-decision review.",
-        "No seven-section merge review report is produced.",
-        "No deduction scoring or merge gate is invented."
+        "No merge-decision review report is produced.",
+        "No severity levels, merge gate, or minimum closed loop framing is invented."
       ]
     }
   ]

skills/arch-code-review/evals/trigger-set.json

@@ -0,0 +1,52 @@
+[
+  {
+    "query": "Review the current branch against main and decide whether it can be merged. Use the default English output. Do not judge only by whether tests pass; first identify this change's minimum closed loop.",
+    "should_trigger": true,
+    "source": "derived from evals.json #1"
+  },
+  {
+    "query": "Run an architecture-audit-level PR review. Focus on whether this diff violates boundaries defined in docs/ARCHITECTURE.md, and do not list style nits.",
+    "should_trigger": true,
+    "source": "derived from evals.json #2"
+  },
+  {
+    "query": "This is a large branch that may include UI, scheduler, and test-tool changes. Please code review whether it can be merged.",
+    "should_trigger": true,
+    "source": "derived from evals.json #3"
+  },
+  {
+    "query": "Review this PR. The related typecheck fails in one changed package, but the failure appears outside the current minimum closed loop. Decide whether that blocks merge.",
+    "should_trigger": true,
+    "source": "derived from evals.json #4"
+  },
+  {
+    "query": "Can we merge this branch into main yet?",
+    "should_trigger": true,
+    "source": "extra positive variant"
+  },
+  {
+    "query": "Take a look at this function and help me fix the obvious problems.",
+    "should_trigger": false,
+    "source": "derived from evals.json #5 (near-miss)"
+  },
+  {
+    "query": "Refactor this module to reduce duplication.",
+    "should_trigger": false,
+    "source": "extra near-miss: refactoring (explicitly excluded)"
+  },
+  {
+    "query": "Debug why this endpoint returns 500.",
+    "should_trigger": false,
+    "source": "extra near-miss: debugging (explicitly excluded)"
+  },
+  {
+    "query": "Explain what auth.ts does.",
+    "should_trigger": false,
+    "source": "extra near-miss: casual code reading (explicitly excluded)"
+  },
+  {
+    "query": "Write a new notification service for our app.",
+    "should_trigger": false,
+    "source": "extra near-miss: ordinary implementation (explicitly excluded)"
+  }
+]