feat(skills): add findings-first /secreview skill — emit before exploring (#12951)

[mrveiss]

Thinking Path

Usage analysis over 59 sessions found security_review was the most-requested
goal (11 sessions) and 8 of the 11 delivered nothing — every one abandoned
during Read/Grep exploration, before a single finding was printed. The
reviews that did land found real defects, so the quality was never the problem:
the ordering was. Absent a constraint, a review optimises for completeness —
read everything, then report — which is right for an unbounded audit and wrong
for a diff, where partial value delivered early beats total value delivered late.

Audited existing coverage before adding anything. review-fleet is the
heavyweight 10-agent PR path, review is CI triage, security-auditor is
whole-codebase, pre-merge-validate has no security dimension. A fast,
diff-only, findings-first path was genuinely absent, and no rule for review
ordering existed anywhere:

$ grep -rniE "security review|findings.first|findings table" CLAUDE.md docs/developer/*.md
(no matches)

What Changed

• .claude/skills/secreview/SKILL.md — a non-negotiable ordering contract:
  one orientation call (the diff, nothing else) → findings table within 3 tool
  calls → verify after → verdict. Unconfirmed rows are marked (unverified),
  which is what makes early emission safe.
• Routing table pointing at review-fleet / security-auditor /
  pre-merge-validate so the fast path is not misapplied to work that needs depth.
• Checklist weighted to the categories that produced real hits here — authz gaps,
  secrets on disk/in logs, injection, TOCTOU, rate-limit bypass, and refactor
  fallout (renames with un-updated call sites), historically the highest-yield
  category.
• CLAUDE.md — one line under Workflow Quick Rules, so the ordering rule
  applies even when the skill is not explicitly invoked.

Long analyses to a file, not the response (#12955)

Research sessions were dying to output token maximum exceeded and losing the
whole analysis. This turned out not to be a missing rule but an active one
pointing the wrong way — research/SKILL.md:199 mandated the failing behaviour:

- Output goes to chat only — no file writing unless user asks

• .claude/skills/research/SKILL.md — inverted: findings are written to
  docs/research/<topic>.md incrementally, so Phase 1 is on disk before the
  Phase 2 approval gate and an unapproved or interrupted run still leaves the
  analysis behind. Chat reply is path + summary.
• CLAUDE.md — the general form, for any long analysis.

Verification

Dogfooded against a real branch diff (PR #12950). The diff was read in one
tool call and the findings table was emitted before any other read — the
contract held. Three findings; two confirmed on verification:

Sev	Finding	Verdict
MEDIUM	rglob("*.py") descends into nested subpackages but applies only the top-level package prefix	CONFIRMED
MEDIUM	Docstring claims per-submodule include_router matching; the code checks the package mounts anything, then includes every router submodule	CONFIRMED
LOW	Package path built from registry module_path could escape backend_dir via ..	Downgraded to NIT — registry is code-controlled, not user input

Confirmation for the first, showing the nested submodule is yielded while the
nested __init__.py that carries its prefix is filtered out:

rglob:                          ['__init__.py', 'a.py', 'sub/__init__.py', 'sub/b.py']
filtered (not startswith __):   ['a.py', 'sub/b.py']

Both findings reported to #12950. Notably this is the same defect class that PR
is fixing — inventing endpoints under a wrong prefix.

Scope of the second fix confirmed to be a single line — no sibling skill carries
the same instruction, and canonical-audit already writes its report to disk:

$ grep -rniE "chat only|no file writing|output goes to chat" .claude/skills/
.claude/skills/research/SKILL.md:199:- Output goes to chat only — no file writing unless user asks

docs/research/ already exists, so this adopts an existing convention rather
than inventing one.

No product code touched; this PR changes skills and two CLAUDE.md lines.

Closes #12951, #12955

Model Used

Claude Opus 5 (1M context)

[github-actions]

✅ SSOT Configuration Compliance: Passing

🎉 No hardcoded values detected that have SSOT config equivalents!