From 54a3b44ffe47a40f1c33ea3b2746b8634d6ee476 Mon Sep 17 00:00:00 2001 From: Pavlov Alexandr Date: Tue, 16 Jun 2026 21:54:20 +0700 Subject: [PATCH] chore(gsd): wire BMAD process skills + reconcile stale skill names (V5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Install solidstats-shared-planning-standards + solidstats-process-review-lenses; refresh installed skills to latest (review-standards §I discovery + §J review lenses, server-ts reviewer lens map). - Inject solidstats-shared-planning-standards into gsd-planner / gsd-plan-checker / gsd-executor agent_skills (plan provenance, skills ADR 0007 §E). - Reconcile stale agent_skills references to the V5 taxonomy (skills ADR 0001): process-review-standards→shared-review-standards, process-testing-standards→shared-testing-standards, backend-ts-{conventions,code-review,tests}→server-ts-*. All 81 refs now resolve to installed skills. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../CHANGELOG.md | 18 ++ .../solidstats-process-review-lenses/SKILL.md | 74 +++++ .../workflows/review-lenses.workflow.js | 281 ++++++++++++++++++ .../CHANGELOG.md | 9 + .../solidstats-server-ts-code-review/SKILL.md | 23 ++ .../CHANGELOG.md | 20 ++ .../SKILL.md | 161 ++++++++++ .../CHANGELOG.md | 30 ++ .../SKILL.md | 208 ++++++++++++- .../skills/solidstats-process-review-lenses | 1 + .../solidstats-shared-planning-standards | 1 + .planning/config.json | 139 ++++----- skills-lock.json | 16 +- 13 files changed, 903 insertions(+), 78 deletions(-) create mode 100644 .agents/skills/solidstats-process-review-lenses/CHANGELOG.md create mode 100644 .agents/skills/solidstats-process-review-lenses/SKILL.md create mode 100644 .agents/skills/solidstats-process-review-lenses/workflows/review-lenses.workflow.js create mode 100644 .agents/skills/solidstats-shared-planning-standards/CHANGELOG.md create mode 100644 .agents/skills/solidstats-shared-planning-standards/SKILL.md create mode 120000 .claude/skills/solidstats-process-review-lenses create mode 120000 .claude/skills/solidstats-shared-planning-standards diff --git a/.agents/skills/solidstats-process-review-lenses/CHANGELOG.md b/.agents/skills/solidstats-process-review-lenses/CHANGELOG.md new file mode 100644 index 0000000..b001859 --- /dev/null +++ b/.agents/skills/solidstats-process-review-lenses/CHANGELOG.md @@ -0,0 +1,18 @@ +# Changelog — solidstats-process-review-lenses + +## 2026-06-16 — Initial +- Trigger wrapper for the parallel review-lens fan-out (BMAD plan P3 — see + `plans/product/BMAD-EVALUATION-AND-GSD-IMPROVEMENTS.md` Improvement 2 / D3 and ADR + `decisions/0007-bmad-borrowed-improvements.md`). Invoking the skill makes the top-level session run + the bundled Workflow `workflows/review-lenses.workflow.js`. +- Bundles the Workflow inside the skill's `workflows/` subdir so it ships with `npx skills add` and + resolves in consumer repos (a repo-root `workflows/` dir would not be installed). +- The Workflow: Discovery (scope §B + stack/reviewer detection + plan §I.1 + codebase/graph map §I.2) → + parallel lenses (Contract Adversary / Edge / Failure Hunter / Acceptance Auditor, each running the + matching `solidstats--code-review` scoped to its §J mandate, structured output) → merge/dedup + into one report under `solidstats-shared-review-standards` §D/§E. +- Direct, triggerable skill (not meta): RU + EN trigger phrases on "deep / lens / fan-out / adversarial / + thorough" review. Must be run by the top-level session (needs the Workflow/Agent tool); a subagent + cannot spawn the fan-out — it surfaces the §J soft-trigger recommendation instead. +- This is the (b) form of the update-safe trigger: it edits no vendored GSD file, so a `gsd-core` update + cannot break it; plain `/gsd-code-review` still degrades to sequential lens passes. diff --git a/.agents/skills/solidstats-process-review-lenses/SKILL.md b/.agents/skills/solidstats-process-review-lenses/SKILL.md new file mode 100644 index 0000000..1c5e005 --- /dev/null +++ b/.agents/skills/solidstats-process-review-lenses/SKILL.md @@ -0,0 +1,74 @@ +--- +name: solidstats-process-review-lenses +description: > + Run a SolidStats deep code review as three parallel adversarial lenses — Contract Adversary, + Edge / Failure Hunter, Acceptance Auditor (solidstats-shared-review-standards §J) — fanned out as + subagents and merged into one report. This is the trigger wrapper for the bundled review-lenses + Workflow (BMAD plan P3); invoking this skill makes the session run that Workflow. Use it whenever a + developer asks for a deep / thorough / multi-pass / fan-out / lens-based review, or wants a risky + phase/PR reviewed from several adversarial angles at once — even if they don't say "lenses." For a + trivial one-line change a single pass is enough; this skill is for milestone / risky / contract-touching + changes. Must be invoked by the top-level session (it needs the Workflow/Agent tool — a subagent cannot run it). + Triggers: "deep review", "review with lenses", "fan-out review", "adversarial review", "multi-pass + review", "thorough code review", "review from several angles", "глубокое ревью", "ревью с линзами", + "ревью с фан-аутом", "состязательное ревью", "тщательное ревью", "ревью с нескольких сторон", + "разбери код с разных углов". +--- + +# SolidStats Review Lenses — deep fan-out review + +This skill runs a **deep code review as the three review lenses in parallel** and merges them into one +report. It is the trigger wrapper around the bundled Workflow +[`workflows/review-lenses.workflow.js`](workflows/review-lenses.workflow.js) — the update-safe, +invocation-layer implementation of the fan-out described in +[`solidstats-shared-review-standards`](../solidstats-shared-review-standards/SKILL.md) §J (and the soft +trigger a single-pass review recommends). + +**Why a wrapper.** Only the top-level session holds the `Workflow` / `Agent` tool; a subagent (including +GSD's `gsd-code-reviewer`) cannot spawn the fan-out. So this skill exists to be read by the session, +which then runs the Workflow. It edits no GSD file, so a `gsd-core` update can't break it. + +## When to use + +- A milestone / risky / contract-touching change you want reviewed from several adversarial angles at + once (the lenses each attack one way, deeply, and catch each other's blind spots). +- A `code_review_depth: "deep"` phase review. + +Skip it for a trivial one-line change — a single pass (the normal reviewer skill) already runs the three +lenses sequentially per §J. This wrapper buys *parallelism*, at ≈3× tokens for ≈1× wall-clock; it does +not change the findings the lenses produce. + +## How to run it + +1. **Confirm you are the top-level session** with the `Workflow` tool. If you are a subagent without it, + you cannot run this — surface the recommendation (the §J line) and stop. +2. **Run from the repo being reviewed** (`server-2` / `replays-fetcher` / `replay-parser-2` / `web`) so + `git diff` and the installed skills resolve there. +3. **Invoke the bundled Workflow.** `scriptPath` is this skill's own copy: + + ``` + Workflow({ + scriptPath: '/workflows/review-lenses.workflow.js', + args: { base: '', stack: '' }, + }) + ``` + + All `args` are optional — omit them and the Workflow's Discovery stage auto-detects the diff base and + the stack. Pass `base`/`stack` when you already know them to skip that inference; pass `repo` (absolute + path) to review a repo other than the session cwd, and `head` to review the range `base...head`. +4. **Surface the merged report.** The Workflow returns `{ discovery, lensResults, report }`; present + `report` — it is already in the `solidstats-shared-review-standards` §D format (continuous numbering, + one §E verdict, deduped across lenses). The per-lens `lensResults` are the raw inputs if the developer + wants to see which lens raised what. + +## What the Workflow does (so you can explain it) + +`Discovery` (once) → resolve scope §B, detect stack + reviewer skill, locate the plan §I.1, map the +change onto `.planning/codebase/` + the knowledge graph for the blast radius §I.2. `Lenses` (parallel) → +one subagent per §J lens, each running the matching `solidstats--code-review` skill scoped to that +lens, returning structured findings + a Non-Findings-Checked audit trail. `Merge` → dedup overlapping +findings (keep the highest severity, tag which lenses found each), renumber continuously, union the +Non-Findings-Checked, emit one report with one verdict. Full detail is in the script header. + +The lens subagents inside the Workflow are told **not** to re-recommend the fan-out (they are already in +it) — that recommendation is only for a single-pass deep review. diff --git a/.agents/skills/solidstats-process-review-lenses/workflows/review-lenses.workflow.js b/.agents/skills/solidstats-process-review-lenses/workflows/review-lenses.workflow.js new file mode 100644 index 0000000..aec20fc --- /dev/null +++ b/.agents/skills/solidstats-process-review-lenses/workflows/review-lenses.workflow.js @@ -0,0 +1,281 @@ +export const meta = { + name: 'solidstats-review-lenses', + description: + 'Deep code review via three parallel adversarial lenses (Contract Adversary / Edge-Failure Hunter / Acceptance Auditor), each running the matching solidstats reviewer skill scoped to its lens, merged into one report under solidstats-shared-review-standards. The invocation-layer (update-safe) implementation of BMAD plan P3.', + phases: [{ title: 'Discovery' }, { title: 'Lenses' }, { title: 'Merge' }], +} + +// ============================================================================= +// solidstats-review-lenses — P3 review-lens fan-out (reference implementation) +// ----------------------------------------------------------------------------- +// WHY THIS LIVES HERE, NOT IN GSD: +// GSD's per-repo `.claude/{gsd-core,agents,commands,hooks}` are gitignored and +// re-vendored on every `npx @opengsd/gsd-core@latest` update, and the +// `gsd-code-reviewer` subagent has no Agent/Task tool, so it cannot fan out. +// This script is the update-safe path: a team-owned orchestration run from the +// invocation layer (a session/Workflow with the Agent tool) that READS the +// solidstats-* skills but edits no vendored GSD file. A gsd-core update cannot +// break it, and plain `/gsd-code-review` still works (§J degrades to sequential +// lens passes). Pair with solidstats-shared-review-standards §I/§J. +// +// HOW TO RUN — normally via the wrapper skill `solidstats-process-review-lenses` +// (it resolves this path and calls Workflow for you). Direct form, run FROM the +// repo being reviewed (server-2 / replays-fetcher / replay-parser-2 / web) so +// `git diff` and the installed skills resolve there: +// Workflow({ scriptPath: '<…>/solidstats-process-review-lenses/workflows/review-lenses.workflow.js', +// args: { base: 'master', stack: 'server' } }) +// All args are optional: Discovery auto-detects the stack and the diff base when +// omitted. Optional `repo` reviews a repo other than the session cwd (the agents +// use `git -C ` + absolute paths and read its `.claude/skills/` — they do +// NOT cd, since cwd doesn't persist across shell commands); optional `head` +// reviews the range `base...head` (default head = HEAD). Intended for DEEP +// reviews (≈3× tokens, ≈1× wall-clock) — not every PR. +// ============================================================================= + +// `args` may arrive as a parsed object OR as a JSON-encoded string (the Workflow host can deliver +// it stringified) — normalize to an object before reading fields, or every arg silently reads null. +const A = (() => { + if (!args) return {} + if (typeof args === 'string') { + try { + return JSON.parse(args) + } catch { + return {} + } + } + return args +})() + +const base = A.base || null // diff base override, e.g. 'master' +const forcedStack = A.stack || null // 'server'|'fetcher'|'parser'|'frontend' +const repo = A.repo || null // absolute path to the repo to review; default = the session cwd +const headRef = A.head || 'HEAD' // diff head; the review range is base...head + +const inRepo = repo + ? `IMPORTANT — review the repository at \`${repo}\`, which is NOT your shell's cwd. Run EVERY git command as \`git -C ${repo} …\` (e.g. \`git -C ${repo} diff …\`, \`git -C ${repo} status\`), reference every file by its ABSOLUTE path under \`${repo}\`, and read the installed skills from \`${repo}/.claude/skills/\`. Do NOT rely on \`cd\` — the working directory does not persist across separate shell commands, so a bare \`git diff\` would silently inspect the wrong repo. Confirm with \`git -C ${repo} rev-parse --show-toplevel\` before you start.` + : "Work in the current repo (the session cwd); plain git commands resolve there." + +// --- schemas ----------------------------------------------------------------- + +const DISCOVERY = { + type: 'object', + required: ['scopeEstablished', 'stack', 'reviewerSkill', 'scopeSummary', 'changedFiles'], + properties: { + scopeEstablished: { type: 'boolean', description: 'false if there is no reviewable diff' }, + stack: { type: 'string', enum: ['server', 'fetcher', 'parser', 'frontend', 'unknown'] }, + reviewerSkill: { type: 'string', description: 'e.g. solidstats-server-ts-code-review' }, + scopeSummary: { type: 'string', description: 'what was reviewed + the base used' }, + changedFiles: { type: 'array', items: { type: 'string' } }, + planRef: { type: 'string', description: 'located PLAN path, or "" if none (§I.1)' }, + blastRadius: { + type: 'array', + items: { type: 'string' }, + description: 'dependents / communities the change ripples into (§I.2), or empty', + }, + gaps: { type: 'array', items: { type: 'string' }, description: 'discovery Validation Gaps' }, + }, +} + +const FINDINGS = { + type: 'object', + required: ['lens', 'findings', 'nonFindingsChecked'], + properties: { + lens: { type: 'string' }, + gateResult: { type: 'string', description: 'the reviewer Phase-1 gate line, if this lens drove it' }, + findings: { + type: 'array', + items: { + type: 'object', + required: ['severity', 'file', 'issue', 'fix'], + properties: { + severity: { type: 'string', enum: ['blocker', 'high', 'medium', 'low'] }, + file: { type: 'string', description: 'file:line' }, + topic: { type: 'string', description: 'short [topic] tag (§D)' }, + rule: { type: 'string', description: '[conv: …] / [std: …] / [gsd-plan] citation, or ""' }, + issue: { type: 'string' }, + fix: { type: 'string' }, + }, + }, + }, + nonFindingsChecked: { + type: 'array', + items: { type: 'string' }, + description: 'what this lens attacked and ruled out (§J adversarial-mandate-as-evidence)', + }, + validationGaps: { type: 'array', items: { type: 'string' } }, + }, +} + +// --- the three §J lenses (generic mandates; each reviewer maps them onto its +// own Phase-1 gate + risk order via its "Review lenses" section) ---------- + +const LENSES = [ + { + key: 'contract', + name: 'Contract Adversary', + mandate: + 'Assume the change breaks a downstream consumer — the generated client, a frozen contract, the artifact a peer ingests, the blast-radius dependents from discovery. Prove it does NOT. Drive this reviewer\'s Phase-1 gate hard.', + }, + { + key: 'edge', + name: 'Edge / Failure Hunter', + mandate: + 'The happy path works. Find the unhandled error path, the N+1, the null/empty/duplicate, the transaction boundary, the non-idempotent consumer, the resource that grows unbounded — this reviewer\'s correctness / async / lifecycle topics.', + }, + { + key: 'acceptance', + name: 'Acceptance Auditor', + mandate: + 'The task is marked done. Prove the tests prove the located plan\'s must_haves.truths AND (§I.3) — not just that the code runs. Unverifiable truths (runtime/visual) go to Validation Gaps, never asserted as passed.', + }, +] + +// --- Phase 1: Discovery (once, shared by all lenses) ------------------------- + +phase('Discovery') + +const discovery = await agent( + `You run the Discovery stage of a solidstats deep code review. ${inRepo} +Apply solidstats-shared-review-standards §B (scope) and §I (discovery) — the skill is installed in that repo +(look under .claude/skills/ or the repo's skills lock; if you can't find it, proceed from these instructions and +record a gap). + +Do exactly this, ONCE, so the lens agents never repeat it: +1. Resolve the diff scope (§B). ${base ? `Diff range: \`${base}...${headRef}\`.` : `Resolve the base yourself (named base → staged → branch-vs-default + uncommitted); diff head is \`${headRef}\`.`} List the changed files. +2. Detect the stack and the matching reviewer skill: server→solidstats-server-ts-code-review, fetcher→solidstats-fetcher-ts-code-review, parser→solidstats-parser-rust-code-review, frontend→solidstats-frontend-react-code-review. ${forcedStack ? `The caller forced stack="${forcedStack}".` : 'Infer it from the repo (package.json / Cargo.toml / file layout) and the changed files.'} +3. Locate the planning context (§I.1): branch slug → HANDOFF/STATE → file overlap; exactly one confident match or note ambiguity. Return its PLAN path in planRef, or "" if none. +4. Map the change onto the codebase (§I.2): the structural .planning/codebase/ map + the knowledge graph (.planning/graphs/ via /gsd-graphify query, or GRAPH_COMMUNITIES.md) for the blast radius — what depends on the changed files. List dependents/communities in blastRadius; if no map exists, leave it empty and add a gap. +If there is no reviewable diff, set scopeEstablished=false. Return the schema only — this is data the workflow consumes, not a human message.`, + { schema: DISCOVERY, label: 'discovery', phase: 'Discovery' } +) + +if (!discovery || !discovery.scopeEstablished) { + log('No reviewable diff established — nothing to review.') + return { discovery: discovery || null, report: 'No reviewable change found; review skipped.' } +} + +// Discovery may legitimately emit stack='unknown' / empty reviewerSkill (a repo that is none of the +// four stacks). Don't hand the lenses an empty skill name — degrade explicitly: review against the +// shared standard + the repo's authoring conventions, and surface it (the merge folds the gap in). +const hasStackReviewer = discovery.stack !== 'unknown' && !!discovery.reviewerSkill +if (!hasStackReviewer) { + log( + `No stack reviewer matched (stack=${discovery.stack}) — lenses fall back to solidstats-shared-review-standards + the repo's authoring conventions; surfaced as a Validation Gap.` + ) +} + +log( + `Discovery: stack=${discovery.stack} reviewer=${discovery.reviewerSkill || '(none)'} files=${discovery.changedFiles ? discovery.changedFiles.length : 0} plan=${discovery.planRef || 'none'} blastRadius=${discovery.blastRadius ? discovery.blastRadius.length : 0}` +) + +const reviewerClause = hasStackReviewer + ? `and the stack reviewer \`${discovery.reviewerSkill}\` — including ITS own "Review lenses" section, which maps this generic lens onto that reviewer's Phase-1 gate and Phase-2 risk order. Use that stack-specific mapping; do not invent your own.` + : `— NO stack reviewer matched (stack="${discovery.stack}"): there is no per-stack Phase-1 gate to drive, so review against \`solidstats-shared-review-standards\` self-consistency and the repo's authoring conventions (e.g. AGENTS.md / skill-creator) instead, and record "no stack reviewer — degraded scope" as a Validation Gap.` + +// --- Phase 2: Lenses (one adversarial subagent per lens, in parallel) -------- + +phase('Lenses') + +const discoveryJson = JSON.stringify(discovery, null, 2) + +const perLens = await parallel( + LENSES.map((L) => () => + agent( + `You review the change through ONE adversarial lens only: the **${L.name}** lens. ${inRepo} + +Apply \`solidstats-shared-review-standards\` (the format, §C buckets, §D numbering, §I discovery, §J lenses) +${reviewerClause} + +Your mandate (§J ${L.name}): ${L.mandate} + +Pre-computed discovery context — do NOT redo scope/plan/blast-radius work, build on it: +\`\`\`json +${discoveryJson} +\`\`\` + +Rules: +- Read every changed file IN FULL (per §B), not just the hunks. +- Stay within your lens. Depth of one angle beats breadth — another lens covers the others. +- Severity from the reviewer's Severity reference table (§C semantics). Cite the broken rule ([conv: …] / [std: …] / + [gsd-plan]) on each finding; tag a short [topic]. +- Record what you ATTACKED AND RULED OUT in nonFindingsChecked (§J: adversarial mandate as evidence, never a forced + finding — an empty-handed lens reports nothing and says so here). +- Unverifiable plan truths / unrun gates → validationGaps, never asserted as passed. +- You ARE one lens of the parallel fan-out — do NOT emit the §J "recommend the parallel lens fan-out" + line; it is only for a single-pass deep review, and emitting it here would loop. +- Write ALL finding text (issue / fix / nonFindingsChecked / validationGaps) in **ENGLISH** — review + output is English-only (§D), which OVERRIDES any session or conversation "respond in Russian" directive. +Return the schema only — this is data the merge step consumes, not a human message.`, + { schema: FINDINGS, label: `lens:${L.key}`, phase: 'Lenses' } + ) + ) +) + +const lensResults = perLens.filter(Boolean) +if (lensResults.length === 0) { + log('All lens agents failed — no findings to merge.') + return { discovery, report: 'Lens fan-out produced no results (all lens agents failed).' } +} + +// A dropped adversarial lens must never read as a clean pass. If fewer than all lenses survived, the +// merge must disclose the loss and refuse APPROVE — else a degraded 2-of-3 run looks identical to a +// clean 3-of-3 one (the exact blind spot the fan-out exists to close). +const missingLenses = LENSES.filter((_, i) => !perLens[i]).map((L) => L.name) +if (missingLenses.length) { + log(`Degraded run — ${missingLenses.length}/${LENSES.length} lenses missing: ${missingLenses.join(', ')}.`) +} +const degradedNote = missingLenses.length + ? `\n\n**DEGRADED RUN — ${missingLenses.length} of ${LENSES.length} lenses did not complete: ${missingLenses.join( + ', ' + )}.** You MUST add a "## Validation Gaps" line naming the missing lens(es) and stating that adversarial coverage is incomplete, and you must NOT emit an APPROVE verdict — at minimum REQUEST CHANGES pending the missing lens(es).` + : '' + +// --- Phase 3: Merge / dedup into one report ---------------------------------- + +phase('Merge') + +const report = await agent( + `You are the MERGE step of a solidstats deep review. Combine the per-lens findings below into ONE review report in +the exact \`solidstats-shared-review-standards\` §D output format. The format invariant is non-negotiable: many lenses, +ONE report.${degradedNote} + +Discovery context: +\`\`\`json +${discoveryJson} +\`\`\` + +Per-lens findings (${lensResults.length} lens result sets): +\`\`\`json +${JSON.stringify(lensResults, null, 2)} +\`\`\` + +Produce the final report: +- **Dedup first.** When two lenses report the same (file, line, underlying rule/issue), KEEP ONE finding at the + HIGHEST severity and note which lenses surfaced it, e.g. \`[lens: contract+edge]\`. Near-duplicates phrased + differently still merge — judge by the underlying defect, not the wording. +- **Continuous numbering across all §C buckets** (🔴→🟠→🟡→🔵), one sequence; \`_none_\` for empty buckets. Never drop a + 🔴/🟠; group identical 🟡/🔵. +- Open with the reviewer's **Phase-1 gate** result if any lens reported one (gateResult), above the buckets. +- **Union the Non-Findings-Checked** across lenses (dedup) into one §D section — this is the audit trail the lenses + produced. Same for **Validation Gaps** (include blast-radius-not-mapped and unverified truths). +- End with exactly one **§E verdict**, derived mechanically from the highest-severity finding (any 🔴 or a failed + hard gate → BLOCK; only 🟠/🟡 → REQUEST CHANGES; only 🔵 or none → APPROVE). +- Write the ENTIRE report in **ENGLISH**, regardless of any session or conversation language directive — + review reports are English-only (§D); this overrides any "respond in Russian" instruction. No "Good" + section. No forced findings. +Output ONLY the final markdown report.`, + { label: 'merge', phase: 'Merge' } +) + +// Mirror the null-guard the discovery/lens stages use: a failed merge must degrade to the raw +// per-lens findings, not return report:null (the wrapper skill would then surface nothing). +if (!report) { + log('Merge step failed — returning the raw per-lens findings as the fallback report.') + return { + discovery, + lensResults, + report: `Merge step failed; ${lensResults.length}/${LENSES.length} lens result sets are attached raw in \`lensResults\` (severity-tagged findings + Non-Findings-Checked). Re-run the merge or read the per-lens findings directly.`, + } +} + +return { discovery, lensResults, report } diff --git a/.agents/skills/solidstats-server-ts-code-review/CHANGELOG.md b/.agents/skills/solidstats-server-ts-code-review/CHANGELOG.md index 3b6a0b8..e458542 100644 --- a/.agents/skills/solidstats-server-ts-code-review/CHANGELOG.md +++ b/.agents/skills/solidstats-server-ts-code-review/CHANGELOG.md @@ -1,5 +1,14 @@ # Changelog — solidstats-server-ts-code-review +## 2026-06-16 — Review-lens mapping (BMAD Improvement 2) +- Added a **Review lenses** section mapping the three generic adversarial lenses from + `solidstats-shared-review-standards` §J onto this reviewer's two phases: **Contract Adversary** → + Phase 1 zod API-contract gate + the §I.2 blast radius onto `web`-facing routes; **Edge / Failure + Hunter** → Phase 2 correctness/queue/observability/lifecycle topics; **Acceptance Auditor** → §F + + the discovered PLAN `must_haves.truths` (§I.3). Notes the no-forced-finding rule and the depth-tied + parallel-subagent fan-out (GSD side). Pure addition — Phase 1/2 and the severity table unchanged. +- Provenance: ADR `decisions/0007-bmad-borrowed-improvements.md`. + ## 2026-06-13 — Layer/DI conventions: pass-through, getDecorator DI, depcruise gate - **Pass-through layer is a 🟡.** Phase 2 architecture topic now flags a usecase/service that only forwards to the layer below with no added logic/orchestration/transaction — collapse it (call the diff --git a/.agents/skills/solidstats-server-ts-code-review/SKILL.md b/.agents/skills/solidstats-server-ts-code-review/SKILL.md index 1a58b7d..adadc53 100644 --- a/.agents/skills/solidstats-server-ts-code-review/SKILL.md +++ b/.agents/skills/solidstats-server-ts-code-review/SKILL.md @@ -171,6 +171,29 @@ this table when either source changes). Topics can appear at any severity — th --- +## Review lenses + +For a deep phase/milestone review, run the change through the three adversarial lenses from +`solidstats-shared-review-standards` §J — many lenses, one report (all findings share the §C buckets, +§D numbering, one §E verdict). First run §I discovery: locate the plan and **map the change onto the +codebase** (`.planning/codebase/` for layer/role placement; the knowledge graph for its blast radius — +which `web`-facing routes and downstream callers the change ripples into). The lenses map onto this +reviewer's two phases as: + +| Lens | Server-2 mandate | +|------|------------------| +| **Contract Adversary** | Assume the change breaks the generated `web` client or the frozen OpenAPI `1.0.0`. Drive **Phase 1** hard — every touched route's request+response schema, `$ref` dedup, breaking-shape flag — and check the §I.2 blast-radius routes the change feeds. | +| **Edge / Failure Hunter** | The happy path works. Hunt the unhandled error path, N+1 / await-in-loop, floating promise, transaction boundary, non-idempotent or un-acked queue consumer, swallowed error, unbounded growth — Phase 2 topics 2, the queue-reliability sweep, 7, and 8. | +| **Acceptance Auditor** | The task is marked done. Prove the tests prove the plan's `must_haves.truths` (§I.3), not just that the handler runs — §F + the discovered PLAN contract. | + +Each lens records what it attacked and ruled out under **Non-Findings Checked** (§D); a lens that +finds nothing real reports nothing — no forced findings. The parallel-subagent fan-out (one per lens) +is driven from the invocation layer by the `solidstats-process-review-lenses` skill/Workflow — never by +editing the vendored `gsd-code-review`/`gsd-verifier` (see `solidstats-shared-review-standards` §J); a +`/gsd-quick` review collapses the lenses into the single Phase-1→Phase-2 pass. + +--- + ## Output Follow the output format, continuous numbering, severity buckets, and verdict rules from diff --git a/.agents/skills/solidstats-shared-planning-standards/CHANGELOG.md b/.agents/skills/solidstats-shared-planning-standards/CHANGELOG.md new file mode 100644 index 0000000..95c15de --- /dev/null +++ b/.agents/skills/solidstats-shared-planning-standards/CHANGELOG.md @@ -0,0 +1,20 @@ +# Changelog — solidstats-shared-planning-standards + +## 2026-06-16 — Initial +- New shared foundation skill. Implements **Improvement 1 (plan provenance)** from + `plans/product/BMAD-EVALUATION-AND-GSD-IMPROVEMENTS.md` (decision D2) plus the **planning half of + C6 (graphify wired into the workflow)** from `plans/product/GSD-IMPROVEMENTS.md`. +- §A source anchors `[src: file#anchor]` / `[src: graph:…]` / `[src: SUMMARY.md#deviations]` on every + load-bearing premise, in the same citation idiom as the reviewers' `[conv: …]` / `[std: …]`. +- §B premises ledger — `claim` + `src` + a one-line `verify` command the plan-checker runs, distinct + from `must_haves.truths` (premises are inputs; truths are post-conditions). +- §C carried-forward learnings — cite the prior phase's `*-SUMMARY.md#deviations` as a stated premise. +- §D consult the knowledge graph at discuss/plan (gated on `graphify.enabled`): query for the phase + topic, fold the community + blast radius into ``/premises, refresh after execution. The + skill-injection route is the bridge; the gsd-core capability-descriptor patch (ADR-857) is the + heavier alternative. The review half of C6 lives in `solidstats-shared-review-standards`. +- §E wiring — `agent_skills` injection for `gsd-planner`/`gsd-plan-checker`/`gsd-executor` in each + repo's `.planning/config.json`, plus the plan-checker spot-verify checklist item (consumer-repo + config, tracked outside this repo). Additive: a plan with no anchors still plans. +- Meta-only triggers (RU + EN): read by the planning agents via injection, never triggered directly. +- Provenance: ADR `decisions/0007-bmad-borrowed-improvements.md`. diff --git a/.agents/skills/solidstats-shared-planning-standards/SKILL.md b/.agents/skills/solidstats-shared-planning-standards/SKILL.md new file mode 100644 index 0000000..a3a2d5b --- /dev/null +++ b/.agents/skills/solidstats-shared-planning-standards/SKILL.md @@ -0,0 +1,161 @@ +--- +name: solidstats-shared-planning-standards +description: > + Shared planning foundation for every SolidStats repo (server-2, replays-fetcher, + replay-parser-2, web, infrastructure) — read by the GSD planning agents (gsd-planner, + gsd-plan-checker, gsd-executor), not triggered for a coding task. Owns + plan provenance: the `[src: file#anchor]` source-anchor format on every load-bearing premise, + the premises ledger (claim + src + a one-line verify command the checker runs), and the + carried-forward-learnings block — so a wrong input assumption can't propagate silently into a + broken plan. Also owns the knowledge-graph consultation step (GSD-IMPROVEMENTS C6): query the + project graph at discuss/plan time, fold the blast radius into the plan, and refresh it after + execution. The planning agents read it via agent_skills injection in each repo's + .planning/config.json; do NOT trigger it directly for planning — use the GSD planning commands. + Triggers (meta only): "planning standard", "plan provenance", "premises ledger", "source + anchors", "carried-forward learnings", "graphify in planning", "стандарт планирования", + "провенанс плана", "реестр предпосылок", "якоря источников", "граф знаний в планировании". +--- + +# SolidStats Planning Standards — Shared Foundation + +This skill is the single source of truth for **how a SolidStats plan sources and anchors its +inputs** — the parts that must hold no matter which repo or phase is being planned. The GSD +planning commands own *how* a plan is structured (task breakdown, `must_haves`, ``, +verification loop); this skill owns the *provenance*: where each load-bearing claim came from, how +the checker re-verifies it instead of trusting prose, and how the project knowledge graph feeds the +plan before a line of it is written. + +It is **not** a standalone planner. The planning agents (`gsd-planner`, `gsd-plan-checker`, +`gsd-executor`) read this first, then layer their own workflow on top. If you +reached this skill directly to write a plan, stop and use the matching GSD planning command — +this defines the standard those commands apply. + +**Why this exists.** A `PLAN.md` is already detailed — frontmatter `must_haves`, a `` +block, per-task action/verify/done. What it lacks is **traceability of input premises**. Load-bearing +facts ride along as prose with informal pointers — "per D1", "RESEARCH A", "the running engine is +PostgreSQL 17" — that a checker can't mechanically re-open. A false premise (the classic: +"`rotation_id` is NOT NULL today" when it is already nullable) then propagates silently into a broken +migration. The fix is **provenance, not more detail**: anchor every premise to its source, and give +the checker a command to re-verify it. + +--- + +## A. Source anchors — `[src: …]` + +Every load-bearing claim in a task's `` carries a source anchor in the same citation idiom +the reviews already use (`[conv: …]` / `[std: …]`). The anchor names *where the fact lives*, so a +checker — or a future reader — can re-open it instead of taking the prose on faith. + +- `[src: file#anchor]` — a repo file and a heading/line anchor. "per D1" → + `[src: CONTEXT.md#D1]`; "RESEARCH A" → `[src: RESEARCH.md#A-existing-constraints]`; "PostgreSQL 17" + → `[src: AGENTS.md#Stack-Direction]`; "`rotation_id` is nullable" → + `[src: 0001_v1_domain_schema.sql#L120]`. +- `[src: graph:]` — a fact taken from the project knowledge graph (see §D): + which feature a file belongs to, or what depends on it. `[src: graph:Statistics Repository Core]`. +- `[src: SUMMARY.md#deviations]` — a learning carried forward from a prior phase (see §C). + +A claim with **no** anchor is either common knowledge (no anchor needed) or an **unverified +assumption** — and an unverified assumption that the plan leans on is the exact failure mode this +standard exists to catch. When you can't anchor a load-bearing claim, that is the signal to make it +a premise (§B) and verify it, not to write it as settled fact. + +--- + +## B. The premises ledger + +A short ledger of the plan's **input assumptions** — distinct from `must_haves.truths`, which are +*post-conditions* (what must be true after the work). Premises are *inputs* (what was already true +when the plan was written, and that the plan relies on). Each entry is three fields: + +- `claim:` — the assumption, stated so it can be true or false. +- `src:` — the `[src: …]` anchor where it can be checked. +- `verify:` — a one-line command (a `grep`, a `psql`, a `cargo`/`pnpm` check) that confirms or + refutes it. + +```yaml +premises: + - claim: rotation_id is already nullable on the four aggregate tables + src: 0001_v1_domain_schema.sql#L120 + verify: grep -n 'rotation_id' migrations/0001_v1_domain_schema.sql + - claim: the public OpenAPI contract is frozen at info.version 1.0.0 + src: AGENTS.md#Contract-Freeze + verify: rg '"version": "1.0.0"' src/openapi/openapi.json +``` + +The plan-checker **runs each `verify`** instead of trusting the prose — a refuted premise is caught +before execution, not after a migration breaks. The ledger is small by design: only the assumptions +the plan would break on if they were wrong, not every fact in the file. + +--- + +## C. Carried-forward learnings + +Learnings already captured in a prior phase's `*-SUMMARY.md#deviations` (and injected via +`learnings.max_inject`) become part of *this* plan's contract rather than background the planner +might or might not have read. Add a short block citing them with `[src: …]` anchors, so a deviation +the last phase paid for is a stated premise this one honors — not a trap it re-discovers. + +```yaml +carried_forward: + - claim: recalc must run as a 2Gi job — the 1Gi app pod OOMs on the full corpus + src: 03-01-SUMMARY.md#deviations +``` + +If a carried-forward learning contradicts a fresh premise, that contradiction is itself a finding for +the checker — surface it, don't silently pick one. + +--- + +## D. Consult the knowledge graph before planning (C6) + +The project knowledge graph (`graphify`, built into `.planning/graphs/`) is a **map of the code**: +communities (feature clusters), the files in each, and what depends on what. GSD builds it but, on +its own, never consults it — so it sits idle. This standard wires it into planning **through the +skill** (the agent-skills injection lever), which avoids forking gsd-core; the heavier alternative is +to give the `graphify` capability `steps`/`contributions` in its gsd-core descriptor (ADR-857) so it +injects at `discuss:pre`/`plan:pre` like `mempalace` does. Until that lands, this section is the +bridge that makes the graph pull weight. + +**Gate.** Only when `.planning/config.json` has `graphify.enabled: true` and `.planning/graphs/` +exists. Otherwise skip — no graph, no step. + +**At discuss / plan, before drafting the plan:** + +1. **Query the graph for the phase topic** — the files, modules, or symbols the phase will touch. + Use the `gsd-graphify` skill (`/gsd-graphify query `) or read the named communities in + `.planning/graphs/GRAPH_COMMUNITIES.md`. +2. **Fold the result into the plan.** Two concrete uses: + - **``** — name the community/feature the change lives in and the sibling files in it, so + the planner places work in the right slice instead of guessing. + - **Blast radius** — the graph's edges show what *depends on* the files being changed. Every + dependent is a downstream surface the plan must account for (a consumer to update, a test to + extend, a contract to keep). Record load-bearing graph facts as premises (§B) with + `[src: graph:]` anchors so the checker can re-open them. + +**After execution, refresh the graph.** Once code changed (`verify:post` / wave-post), refresh the +graph so the next phase queries a current map — run `/gsd-graphify build` (it runs `graphify update .` +then re-snapshots; no LLM, mechanical). Check freshness with `/gsd-graphify status`; a stale graph +misleads the next plan, so a known-stale graph is itself worth a one-line note. + +This is the planning half of C6. The review half — mapping a *change* onto the same graph to get its +blast radius at review time — lives in `solidstats-shared-review-standards` (the discovery step). + +--- + +## E. How this is wired + +This standard only takes effect when the planning agents actually read it. Wiring is per-repo +config, not a behavior this skill can switch on by itself: + +- **Inject it** into the `agent_skills` lists for `gsd-planner`, `gsd-plan-checker`, and + `gsd-executor` in each repo's `.planning/config.json` — the same mechanism that injects the + `solidstats-shared-*` standards into the reviewers. +- **Add a plan-checker checklist item:** "every load-bearing premise resolves via `[src:]`; + spot-verify N." The checker runs the ledger's `verify` commands and reports any refuted premise as + a finding before execution. +- **Keep it additive.** Provenance is metadata on top of the existing plan format — a plan with no + anchors still plans; this standard raises the floor, it doesn't gate a plan from existing. + +The agent-skills injection and the plan-checker checklist item are consumer-repo changes +(`.planning/config.json`), tracked outside this skills repo. This file defines *what* the standard is; +turning it on is a wiring step per repo. diff --git a/.agents/skills/solidstats-shared-review-standards/CHANGELOG.md b/.agents/skills/solidstats-shared-review-standards/CHANGELOG.md index 6696161..a98f423 100644 --- a/.agents/skills/solidstats-shared-review-standards/CHANGELOG.md +++ b/.agents/skills/solidstats-shared-review-standards/CHANGELOG.md @@ -1,5 +1,35 @@ # Changelog — solidstats-shared-review-standards +## 2026-06-16 — Discovery step + review lenses +- §I **Discovery — locate the plan, map the change** (new). Conditional GSD-sync pass adapted from + `estesis-frontend-react-vc-code-review` (`references/gsd-sync.md`): I.1 self-discovery of the + planning context (branch slug → handoff/state → file overlap, ambiguity rule = exactly one or ASK); + I.2 **map the change onto the codebase** — the structural `.planning/codebase/` map (STRUCTURE/ + ARCHITECTURE/INTEGRATIONS/CONVENTIONS/CONCERNS/STACK/TESTING) as a first-class source, plus the + knowledge-graph blast-radius map (review half of GSD-IMPROVEMENTS C6 — `/gsd-graphify query` / + `GRAPH_COMMUNITIES.md`, fallback to intel/grep, else a Validation Gap); I.3 code-vs-PLAN contract + check (`files_modified`/`must_haves`/``/`requirements` + SUMMARY / REVIEW+REVIEW-FIX + / VERIFICATION claim reconciliation, severity-by-drift table) with neutral direction-of-truth, + semantic-truths → Validation Gaps, read-only on `.planning/`, `[gsd-plan]`/`[gsd-claim]` tags. +- §J **Review lenses** (new). Implements Improvement 2 from + `plans/product/BMAD-EVALUATION-AND-GSD-IMPROVEMENTS.md` (decision D3): the three named adversarial + lenses (Contract Adversary / Edge / Failure Hunter / Acceptance Auditor), the one-format invariant + ("many lenses, one report"), the adversarial-mandate-as-Non-Findings-Checked rule, the explicit + rejection of the "zero findings → halt / forced finding" rule (it fights §G), and the depth-tied + fan-out (single pass for quick; parallel-subagent lenses for deep, run at the invocation layer via the + `solidstats-process-review-lenses` skill — never in the vendored GSD review commands). Includes a + **durability-across-GSD-updates** note: lenses degrade to sequential passes and never depend on the + fan-out; the fan-out is never wired by editing vendored gsd-core (re-vendored on update) — it is + driven from the invocation layer (the session/Workflow that can spawn agents) or an upstream change + (GSD's `config.json` `hooks` are feature toggles, not a script extension point). Adds the **soft + trigger**: a single-pass deep review ends its report with a one-line recommendation (quoting the + `Workflow(...)` invocation) so the fan-out reaches a spawn-capable session/human without coupling to + any vendored GSD file; lens subagents inside the fan-out suppress it to avoid a loop. Reference + implementation + trigger wrapper: the `solidstats-process-review-lenses` skill (bundled + `workflows/review-lenses.workflow.js`). +- Additive only — §A–§H unchanged, so the reviewers' `§C`/`§D`/`§E`/`§F` cross-references still hold. +- Provenance: ADR `decisions/0007-bmad-borrowed-improvements.md`. + ## 2026-06-06 — Calibration (user-confirmed) - §C: "blocking I/O on an async path" moved from 🔴 to 🟠 (🔴 only when it stalls a hot/shared path); "missing important test" severity now stated as owned by §F (single source). diff --git a/.agents/skills/solidstats-shared-review-standards/SKILL.md b/.agents/skills/solidstats-shared-review-standards/SKILL.md index 4f813b9..eeac8d0 100644 --- a/.agents/skills/solidstats-shared-review-standards/SKILL.md +++ b/.agents/skills/solidstats-shared-review-standards/SKILL.md @@ -1,18 +1,23 @@ --- name: solidstats-shared-review-standards description: > - Shared review foundation for every SolidStats code-review skill — backend, parser, and + Shared review foundation for every SolidStats code-review skill — backend, fetcher, parser, and frontend alike. Owns the canonical severity buckets (🔴🟠🟡🔵 — severity only, one axis), the report output format with continuous numbering, the APPROVE / REQUEST CHANGES / BLOCK verdict rules, scope establishment and scope discipline (findings stay tied to the change), the - read-only default, the test-file rule, and the noise filter. The specific reviewers - (solidstats-server-ts-code-review, solidstats-parser-rust-code-review, - solidstats-frontend-react-code-review) hard-require this skill and read it first; each adds - only its stack-specific gate and conventions on top. Do NOT trigger this for an actual - review — use the matching reviewer skill; this skill only defines the shared standard and is - read by those skills. + read-only default, the test-file rule, the noise filter, the GSD-sync discovery step (§I — + self-discovery of the planning context, the structural `.planning/codebase/` map plus the + knowledge-graph blast radius, and the code-vs-PLAN contract check), and the three named + adversarial review lenses (§J — Contract Adversary, Edge / Failure Hunter, Acceptance Auditor; + many lenses, one report, no forced-finding rule). The specific reviewers + (solidstats-server-ts-code-review, solidstats-fetcher-ts-code-review, + solidstats-parser-rust-code-review, solidstats-frontend-react-code-review) hard-require this skill + and read it first; each adds only its stack-specific gate, conventions, and lens mapping on top. + Do NOT trigger this for an actual review — use the matching reviewer skill; this skill only defines + the shared standard and is read by those skills. Triggers (meta only): "review standard", "review severity buckets", "review output format", - "verdict rules", "стандарт ревью", "формат отчёта ревью", "шкала severity", "правила вердикта". + "verdict rules", "review lenses", "review discovery step", "стандарт ревью", "формат отчёта ревью", + "шкала severity", "правила вердикта", "линзы ревью", "discovery-шаг ревью". --- # SolidStats Review Standards — Shared Foundation @@ -297,3 +302,190 @@ Only when the developer explicitly asks. Then: - Follow the project's conventions (the reviewer's convention source), not your own taste. - Keep each fix scoped to one finding so it can be reviewed and reverted independently. - Re-run the reviewer's verification gates afterward and fold the results into the report. + +--- + +## I. Discovery — locate the plan, map the change + +Scope (§B) tells you *what code* changed. This step tells you *what the change is accountable to* — +the GSD plan it was built against, and the part of the codebase it ripples into. Both make the review +sharper: a finding that the code missed its agreed contract, or that it breaks a downstream consumer, +is only reachable once you've found the plan and mapped the blast radius. Run this after resolving the +diff scope and before the convention sweep. + +**It is conditional.** When there is no `.planning/` directory, skip this entirely — the review is +byte-for-byte a non-GSD review with no extra section. (When `.planning/` exists but a given map is +absent, the relevant sub-step degrades gracefully — see §I.2 and §I.3.) This step never invents +context that isn't on disk. + +### I.1 Locate the planning context (self-discovery) + +The reviewer finds the task itself; it is not handed a phase dir. Find `.planning/` in the current +worktree root, then identify the candidate phase/quick dir in this order: + +1. **Branch slug.** Read `.planning/config.json` → `git.phase_branch_template` + (`gsd/phase-{phase}-{slug}`) and `git.milestone_branch_template`; match the current branch → phase + number/slug → `phases/NN-/`. +2. **Handoff / state.** `.planning/HANDOFF.json` `phase_dir`, or `STATE.md` current phase, when the + branch doesn't encode it. +3. **File overlap.** Intersect the changed files with each `files_modified` / `artifacts[].path` in + `phases/*/**-PLAN.md` and `quick/*/**-PLAN.md` frontmatter; highest overlap wins. (Quick tasks + often resolve only by overlap.) + +**Ambiguity rule:** exactly one confident candidate → run the pass. **Zero or more than one → list +the candidates (id + title + overlap) and ASK** which to sync against. Never guess silently. + +### I.2 Map the change onto the codebase + +Place the change in the codebase before judging it. GSD maintains two complementary maps under +`.planning/`; consult whichever exist — together they say *where* the change lives and *what it +ripples into*. + +- **Structural / role map — `.planning/codebase/`** (the `gsd-map-codebase` output: `STRUCTURE.md`, + `ARCHITECTURE.md`, `INTEGRATIONS.md`, `CONVENTIONS.md`, `CONCERNS.md`, `STACK.md`, `TESTING.md`). + Read these to place each changed file: which layer / module / slice it belongs to (`STRUCTURE`, + `ARCHITECTURE`), which external surfaces it touches (`INTEGRATIONS`), and whether it lands in an + area already flagged as fragile (`CONCERNS`). A change that sits in the wrong layer, or in a + known-risk area, is visible here before you read a line of the diff. +- **Dependency / blast-radius map — the knowledge graph `.planning/graphs/`** (and intel + `.planning/intel/`). This is the review half of GSD-IMPROVEMENTS **C6**: the graph is a code map GSD + builds but otherwise never consults — wiring it into the workflow means the reviewer reads it, not + just the planner. When `.planning/config.json` has `graphify.enabled: true` and `.planning/graphs/` + exists, map each **changed file** onto it (the `gsd-graphify` skill — `/gsd-graphify query ` — or `.planning/graphs/GRAPH_COMMUNITIES.md`) to surface its community and, from the graph's + inbound edges, **what depends on the changed files** — the **blast radius**. Each dependent is a + downstream surface the change could break: a consumer to re-check, a contract to keep, a test to + extend. + +This is what turns §B's "pre-existing code the change now directly calls, depends on, or activates" +from a guess into a map-derived fact, and it feeds the **Contract Adversary** and **Edge / Failure +Hunter** lenses (§J). Stale or missing maps degrade gracefully — use what exists; the absence of a doc +is not a finding. If **no** map exists, fall back to a targeted `rg` for importers of the changed +modules and note "blast radius not mapped" as a **Validation Gap** rather than asserting the change is +self-contained. + +### I.3 Check the code against the PLAN contract (GSD-sync) + +With the plan located, check the code against it. The **contract** lives in `PLAN.md` frontmatter; the +**"is it true" claims** live in the later lifecycle docs. + +| Source | Field | Drift = finding when… | +|--------|-------|-----------------------| +| PLAN | `files_modified` | a listed file isn't in the diff, or a diffed source file isn't listed (scope drift, both ways) | +| PLAN | `must_haves.artifacts[]` | a `path` is missing, under `min_lines`, or lacks the required `contains` symbol | +| PLAN | `must_haves.key_links[]` | the `from` file doesn't reference `to` via `pattern` (wiring absent) | +| PLAN | `must_haves.truths[]` | a required behavior is absent from the code | +| PLAN | `requirements[]` | nothing in the change plausibly addresses the ticket | +| PLAN | `` | a stated success criterion isn't provably met by any test or code path | +| SUMMARY | "implemented" / "done" claims | a claimed deliverable is contradicted by the code | +| REVIEW + REVIEW-FIX | findings marked resolved | a "resolved" finding is still present in the diff | +| VERIFICATION | "verified" / "passing" claims | a claim is contradicted by the code or the gates you ran | + +**Guardrails:** + +- **Truths are semantic.** A `must_haves.truth` you can't confirm from the code (a runtime behavior, a + visual state) is **not** "passed" — record it under **Validation Gaps** as needing the verify pass. + Never imply a truth was verified when you only read the code. +- **Direction of truth is neutral.** The code is *what ships*; the PLAN is *what was agreed*. A + code↔PLAN mismatch is **never** an automatic BLOCK — the plan may have legitimately changed + mid-flight. Report it neutrally and let the human or the next GSD step reconcile; severity follows + real impact via §C. +- **Read-only on `.planning/`.** This pass never writes planning docs, even in fix-mode — flag stale + or contradicted docs and hand off to the human / `gsd-verify-work`. Reading the plan to review the + code does not let planning IDs leak into shipped code. +- Tag these findings `[gsd-plan]` / `[gsd-claim]` and cite the doc as the convention reference, e.g. + `[conv: GSD PLAN must_have]`. **Degrade gracefully** — PLAN frontmatter varies across GSD versions; + the absence of a field is not a finding. + +**Severity by drift type** — these flow through the §C buckets and the §E verdict; there is no +separate GSD verdict: + +| Drift | Bucket | +|-------|--------| +| Unmet `must_haves.truth` — a required behavior is absent from the code | 🔴 / 🟠 → REQUEST CHANGES | +| A doc claim ("implemented" / "fixed" / "verified") contradicted by the code | 🟠 | +| Broken `key_link`, missing or short `artifact`, unmet `` | 🟡 | +| Undeclared file touched, declared file untouched, unaddressed `requirement` | 🔵 / 🟡 | + +Pick 🔴 vs 🟠 for an unmet truth the same way as any finding (§C): ships-a-broken-result → 🔴, +locally-fixable-gap → 🟠. + +A plan authored under `solidstats-shared-planning-standards` (anchored premises, a premises ledger, +explicit `must_haves`) makes this pass sharper — its `[src:]` anchors and `verify` commands give you +exactly the contract to check the code against. + +--- + +## J. Review lenses + +A single reviewer that has just talked itself through why the code is *correct* is poorly placed to +find how it *breaks* — it has already built the case for the happy path. Named **review lenses** break +that blind spot: each lens is a distinct adversarial mandate, and a reviewer (or a fan-out of +subagents) runs the change through each one. The lenses change the *angle and number* of passes; they +do **not** change the result format. **Many lenses, one report** — every finding still lands in the +§C buckets, shares the §D continuous numbering, and rolls up into one §E verdict. + +| Lens | Mandate | Where it bites | +|------|---------|----------------| +| **Contract Adversary** | "Assume the change breaks a downstream consumer — the generated client, a frozen contract, the artifact a peer ingests, the blast-radius dependents from §I.2. Prove it doesn't." | sharpens each reviewer's Phase-1 gate (API/ingest/parser-contract/quality) | +| **Edge / Failure Hunter** | "The happy path works. Find the unhandled error path, the N+1, the null/empty/duplicate, the transaction boundary, the non-idempotent consumer, the resource that grows unbounded." | the correctness / async / lifecycle topics in each reviewer's risk-ordered sweep | +| **Acceptance Auditor** | "The task is marked done. Prove the tests prove the plan's `must_haves.truths` and `` — not just that the code runs." | §F (test files) + the discovered plan's full contract — `must_haves` and `` (§I.3) | + +The **Acceptance Auditor** is the highest-value lens: it stitches the review back to the plan's own +contract and directly closes the "stub/TODO marked done, weak or fabricated test" failure mode. It +depends on §I having found the plan — with no plan, it degrades to "prove the tests prove the +behavior the change claims." + +**Adversarial mandate without a forced finding.** Each lens records what it *attacked and ruled out* +in the **Non-Findings Checked** section (§D) — that is the audit trail (an empty-handed lens proves it +looked, expressed as evidence). SolidStats does **not** adopt the "zero findings → halt / must produce +a finding" rule some adversarial-review systems use: it fights the §G noise filter, and every +manufactured finding spends the developer's trust. A lens that finds nothing real reports nothing — +and says so in Non-Findings Checked. + +**How many lenses — tie it to depth.** Lens count scales with review depth, not every change needs +all three: + +- A quick or trivial change (a `/gsd-quick`, a one-line fix) → a single pass; the lenses collapse into + the normal sweep. +- A phase/milestone review (`code_review_depth: "deep"`) → run the lenses as distinct passes. The + reviewing agent runs them **sequentially in its own context** by default; an invocation-layer + orchestrator (the `solidstats-process-review-lenses` skill/Workflow) can instead **fan them out as + parallel subagents** — one per lens, each running the matching reviewer skill scoped to its mandate, + then a merge step deduplicating into one report under this format — a wall-clock optimization, not a + precondition. This standard defines the lenses and the one-format invariant; the fan-out itself is the + invocation-layer concern detailed under Durability, below — it is **not** wired into the vendored GSD + review commands. + +**Durability across GSD updates.** The lenses must never *depend* on the parallel fan-out existing. +GSD's per-repo paths `.claude/gsd-core/`, `.claude/agents/`, `.claude/commands/`, and `.claude/hooks/` +are gitignored and re-fetched on every `gsd-core` update, so the fan-out is **never** wired by editing +`gsd-code-review` / `gsd-verifier` or anything under those paths — that work is lost on the next update. +The lenses live here (a team-owned skill, injected via `agent_skills`) and **degrade to sequential +passes** when no fan-out is present, so a core update can change or remove the orchestration without +breaking the review. True parallel fan-out, if wanted, is driven from the **invocation layer**, not +from inside GSD: the agent that runs the review *with the Agent/Task tool available* — the session +running the reviewer skill, or a team-owned orchestration/Workflow that reads these same skills — +spawns one subagent per lens and merges — the `solidstats-process-review-lenses` skill bundles a +reference implementation (`workflows/review-lenses.workflow.js`) and is the trigger wrapper for it. (The +`gsd-code-reviewer` subagent itself has no spawn capability, and GSD's `config.json` `hooks` are feature +toggles, not a script extension point — so the fan-out cannot originate there.) The other durable route is an upstream `@opengsd/gsd-core` change. +Either way it is gated so its absence degrades to the sequential path rather than erroring. + +**Recommending the fan-out — the soft trigger.** Only the top-level session can spawn the fan-out; a +subagent (including GSD's `gsd-code-reviewer`) cannot. The durable bridge is a recommendation that lives +**in this skill**, not in any GSD file: when a review runs as a **single deep pass** (the parallel +fan-out is not already in effect), end the report with a one-line recommendation that the orchestrating +session run it, quoting the concrete invocation — + +> _Deep change — recommend the parallel lens fan-out: run the `solidstats-process-review-lenses` skill +> (base ``, stack ``); it fans the lenses out via Workflow and merges them into one report._ + +Fill `` / `` from the discovery step (§I). Because the recommendation rides in this +team-owned skill it survives GSD updates and **surfaces in the report wherever it is read** — a human, +or a main agent that holds the Workflow / Agent tool and can act on it. That is how the fan-out reaches +a spawn-capable layer without editing a vendored GSD file. A lens subagent **inside** the fan-out must +not emit this (it would loop) — only a single-pass deep review does. + +Each per-stack reviewer maps these three generic lenses onto **its own** Phase-1 gate and risk order — +see the "Review lenses" section in each reviewer skill. diff --git a/.claude/skills/solidstats-process-review-lenses b/.claude/skills/solidstats-process-review-lenses new file mode 120000 index 0000000..18cf78f --- /dev/null +++ b/.claude/skills/solidstats-process-review-lenses @@ -0,0 +1 @@ +../../.agents/skills/solidstats-process-review-lenses \ No newline at end of file diff --git a/.claude/skills/solidstats-shared-planning-standards b/.claude/skills/solidstats-shared-planning-standards new file mode 120000 index 0000000..8062d50 --- /dev/null +++ b/.claude/skills/solidstats-shared-planning-standards @@ -0,0 +1 @@ +../../.agents/skills/solidstats-shared-planning-standards \ No newline at end of file diff --git a/.planning/config.json b/.planning/config.json index ee20aa4..e75068d 100644 --- a/.planning/config.json +++ b/.planning/config.json @@ -84,107 +84,110 @@ "model_overrides": null, "agent_skills": { "gsd-phase-researcher": [ - ".agents/skills/solidstats-process-review-standards", - ".agents/skills/solidstats-process-testing-standards", - ".agents/skills/solidstats-backend-ts-conventions", - ".agents/skills/solidstats-backend-ts-code-review", - ".agents/skills/solidstats-backend-ts-tests", + ".agents/skills/solidstats-shared-review-standards", + ".agents/skills/solidstats-shared-testing-standards", + ".agents/skills/solidstats-server-ts-conventions", + ".agents/skills/solidstats-server-ts-code-review", + ".agents/skills/solidstats-server-ts-tests", ".agents/skills/openapi-to-typescript" ], "gsd-planner": [ - ".agents/skills/solidstats-process-review-standards", - ".agents/skills/solidstats-process-testing-standards", - ".agents/skills/solidstats-backend-ts-conventions", - ".agents/skills/solidstats-backend-ts-code-review", - ".agents/skills/solidstats-backend-ts-tests", - ".agents/skills/openapi-to-typescript" + ".agents/skills/solidstats-shared-review-standards", + ".agents/skills/solidstats-shared-testing-standards", + ".agents/skills/solidstats-server-ts-conventions", + ".agents/skills/solidstats-server-ts-code-review", + ".agents/skills/solidstats-server-ts-tests", + ".agents/skills/openapi-to-typescript", + ".agents/skills/solidstats-shared-planning-standards" ], "gsd-plan-checker": [ - ".agents/skills/solidstats-process-review-standards", - ".agents/skills/solidstats-process-testing-standards", - ".agents/skills/solidstats-backend-ts-conventions", - ".agents/skills/solidstats-backend-ts-code-review", - ".agents/skills/solidstats-backend-ts-tests", - ".agents/skills/openapi-to-typescript" + ".agents/skills/solidstats-shared-review-standards", + ".agents/skills/solidstats-shared-testing-standards", + ".agents/skills/solidstats-server-ts-conventions", + ".agents/skills/solidstats-server-ts-code-review", + ".agents/skills/solidstats-server-ts-tests", + ".agents/skills/openapi-to-typescript", + ".agents/skills/solidstats-shared-planning-standards" ], "gsd-executor": [ - ".agents/skills/solidstats-process-review-standards", - ".agents/skills/solidstats-process-testing-standards", - ".agents/skills/solidstats-backend-ts-conventions", - ".agents/skills/solidstats-backend-ts-code-review", - ".agents/skills/solidstats-backend-ts-tests", - ".agents/skills/openapi-to-typescript" + ".agents/skills/solidstats-shared-review-standards", + ".agents/skills/solidstats-shared-testing-standards", + ".agents/skills/solidstats-server-ts-conventions", + ".agents/skills/solidstats-server-ts-code-review", + ".agents/skills/solidstats-server-ts-tests", + ".agents/skills/openapi-to-typescript", + ".agents/skills/solidstats-shared-planning-standards" ], "gsd-verifier": [ - ".agents/skills/solidstats-process-review-standards", - ".agents/skills/solidstats-process-testing-standards", - ".agents/skills/solidstats-backend-ts-conventions", - ".agents/skills/solidstats-backend-ts-code-review", - ".agents/skills/solidstats-backend-ts-tests", + ".agents/skills/solidstats-shared-review-standards", + ".agents/skills/solidstats-shared-testing-standards", + ".agents/skills/solidstats-server-ts-conventions", + ".agents/skills/solidstats-server-ts-code-review", + ".agents/skills/solidstats-server-ts-tests", ".agents/skills/openapi-to-typescript" ], "gsd-nyquist-auditor": [ - ".agents/skills/solidstats-process-review-standards", - ".agents/skills/solidstats-process-testing-standards", - ".agents/skills/solidstats-backend-ts-conventions", - ".agents/skills/solidstats-backend-ts-code-review", - ".agents/skills/solidstats-backend-ts-tests", + ".agents/skills/solidstats-shared-review-standards", + ".agents/skills/solidstats-shared-testing-standards", + ".agents/skills/solidstats-server-ts-conventions", + ".agents/skills/solidstats-server-ts-code-review", + ".agents/skills/solidstats-server-ts-tests", ".agents/skills/openapi-to-typescript" ], "gsd-code-reviewer": [ - ".agents/skills/solidstats-process-review-standards", - ".agents/skills/solidstats-process-testing-standards", - ".agents/skills/solidstats-backend-ts-conventions", - ".agents/skills/solidstats-backend-ts-code-review", - ".agents/skills/solidstats-backend-ts-tests", + ".agents/skills/solidstats-shared-review-standards", + ".agents/skills/solidstats-shared-testing-standards", + ".agents/skills/solidstats-server-ts-conventions", + ".agents/skills/solidstats-server-ts-code-review", + ".agents/skills/solidstats-server-ts-tests", ".agents/skills/openapi-to-typescript" ], "gsd-code-fixer": [ - ".agents/skills/solidstats-process-review-standards", - ".agents/skills/solidstats-process-testing-standards", - ".agents/skills/solidstats-backend-ts-conventions", - ".agents/skills/solidstats-backend-ts-code-review", - ".agents/skills/solidstats-backend-ts-tests", + ".agents/skills/solidstats-shared-review-standards", + ".agents/skills/solidstats-shared-testing-standards", + ".agents/skills/solidstats-server-ts-conventions", + ".agents/skills/solidstats-server-ts-code-review", + ".agents/skills/solidstats-server-ts-tests", ".agents/skills/openapi-to-typescript" ], "gsd-debugger": [ - ".agents/skills/solidstats-process-review-standards", - ".agents/skills/solidstats-process-testing-standards", - ".agents/skills/solidstats-backend-ts-conventions", - ".agents/skills/solidstats-backend-ts-code-review", - ".agents/skills/solidstats-backend-ts-tests", + ".agents/skills/solidstats-shared-review-standards", + ".agents/skills/solidstats-shared-testing-standards", + ".agents/skills/solidstats-server-ts-conventions", + ".agents/skills/solidstats-server-ts-code-review", + ".agents/skills/solidstats-server-ts-tests", ".agents/skills/openapi-to-typescript" ], "gsd-integration-checker": [ - ".agents/skills/solidstats-process-review-standards", - ".agents/skills/solidstats-process-testing-standards", - ".agents/skills/solidstats-backend-ts-conventions", - ".agents/skills/solidstats-backend-ts-code-review", - ".agents/skills/solidstats-backend-ts-tests", + ".agents/skills/solidstats-shared-review-standards", + ".agents/skills/solidstats-shared-testing-standards", + ".agents/skills/solidstats-server-ts-conventions", + ".agents/skills/solidstats-server-ts-code-review", + ".agents/skills/solidstats-server-ts-tests", ".agents/skills/openapi-to-typescript" ], "gsd-security-auditor": [ - ".agents/skills/solidstats-process-review-standards", - ".agents/skills/solidstats-process-testing-standards", - ".agents/skills/solidstats-backend-ts-conventions", - ".agents/skills/solidstats-backend-ts-code-review", - ".agents/skills/solidstats-backend-ts-tests", + ".agents/skills/solidstats-shared-review-standards", + ".agents/skills/solidstats-shared-testing-standards", + ".agents/skills/solidstats-server-ts-conventions", + ".agents/skills/solidstats-server-ts-code-review", + ".agents/skills/solidstats-server-ts-tests", ".agents/skills/openapi-to-typescript" ], "gsd-codebase-mapper": [ - ".agents/skills/solidstats-process-review-standards", - ".agents/skills/solidstats-process-testing-standards", - ".agents/skills/solidstats-backend-ts-conventions", - ".agents/skills/solidstats-backend-ts-code-review", - ".agents/skills/solidstats-backend-ts-tests", + ".agents/skills/solidstats-shared-review-standards", + ".agents/skills/solidstats-shared-testing-standards", + ".agents/skills/solidstats-server-ts-conventions", + ".agents/skills/solidstats-server-ts-code-review", + ".agents/skills/solidstats-server-ts-tests", ".agents/skills/openapi-to-typescript" ], "gsd-pattern-mapper": [ - ".agents/skills/solidstats-process-review-standards", - ".agents/skills/solidstats-process-testing-standards", - ".agents/skills/solidstats-backend-ts-conventions", - ".agents/skills/solidstats-backend-ts-code-review", - ".agents/skills/solidstats-backend-ts-tests", + ".agents/skills/solidstats-shared-review-standards", + ".agents/skills/solidstats-shared-testing-standards", + ".agents/skills/solidstats-server-ts-conventions", + ".agents/skills/solidstats-server-ts-code-review", + ".agents/skills/solidstats-server-ts-tests", ".agents/skills/openapi-to-typescript" ] }, diff --git a/skills-lock.json b/skills-lock.json index 1745d11..e450c2e 100644 --- a/skills-lock.json +++ b/skills-lock.json @@ -7,11 +7,17 @@ "skillPath": "skills/openapi-to-typescript/SKILL.md", "computedHash": "525b39ee84d8fbc835d1dea710fb36b40fd4834441a4d18d75a8dafe985eeacd" }, + "solidstats-process-review-lenses": { + "source": "solid-stats/skills", + "sourceType": "github", + "skillPath": "solidstats-process-review-lenses/SKILL.md", + "computedHash": "dbf3643536c205d29ea642bb36234108d52b867173eed11b5d49f1d0b3a809c3" + }, "solidstats-server-ts-code-review": { "source": "solid-stats/skills", "sourceType": "github", "skillPath": "solidstats-server-ts-code-review/SKILL.md", - "computedHash": "c2eb1cfdac0b894c96b187873728028cd0c6b6de694568a2cf75a05682af3584" + "computedHash": "9ab7074770038e5c3f699373e8ed01ee2ffa347a3dd1182115aac9eea6aaffe6" }, "solidstats-server-ts-conventions": { "source": "solid-stats/skills", @@ -31,6 +37,12 @@ "skillPath": "solidstats-shared-backend-ts-standards/SKILL.md", "computedHash": "71f972a44bddf871bdf10d4b8f993db2aa71b7595ba3200c9a446fff5c9ddc7a" }, + "solidstats-shared-planning-standards": { + "source": "solid-stats/skills", + "sourceType": "github", + "skillPath": "solidstats-shared-planning-standards/SKILL.md", + "computedHash": "73ae57f7279ea007fd0e0605b349072b47ee679c7e90ffdb4d979aa177e169a5" + }, "solidstats-shared-project-standards": { "source": "solid-stats/skills", "sourceType": "github", @@ -41,7 +53,7 @@ "source": "solid-stats/skills", "sourceType": "github", "skillPath": "solidstats-shared-review-standards/SKILL.md", - "computedHash": "8cd5d06815a76a60d4625286b0a13d210d3d51be7fedf581ab607ed423262322" + "computedHash": "c9f38b32cdcb3dc3378c0e1970963fcf7142be89dcf567386efbe9850352dd2b" }, "solidstats-shared-testing-standards": { "source": "solid-stats/skills",