From da65a356cfa7801cea5d85bac4c22d372cf81ea0 Mon Sep 17 00:00:00 2001 From: garrytan Date: Thu, 29 Oct 2026 16:15:50 +0000 Subject: [PATCH] fix(review): severity follows the blocking floor, not the checklist pass or category (GSTA-160) The checklist bound severity to its passes (Pass 1 CRITICAL, Pass 2 INFORMATIONAL) and the core floor had no security clause, so GSTA-11's replay saw real bugs filed INFORMATIONAL. One floor sentence now reaches every reviewer prompt; the passes set check order only; shell injection covers shell sinks; a state-swap-before-failure check joins Pass 1 (GSTA-159); the Testing specialist reports a floor-level product defect as category correctness so classify keeps it blocking. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- docs/review-policy.md | 13 ++- docs/ship-policy.md | 5 +- review/SKILL.md | 8 +- review/SKILL.md.tmpl | 8 +- review/checklist.md | 28 +++-- review/sections/review-army.md | 7 +- review/specialists/testing.md | 1 + scripts/resolvers/fix-check.ts | 5 +- scripts/resolvers/review-army.ts | 8 +- scripts/resolvers/review-floor.ts | 6 + ship/SKILL.md | 10 +- ship/SKILL.md.tmpl | 10 +- ship/sections/review-army.md | 7 +- test/fixtures/golden/agy-ship-SKILL.md | 10 +- test/fixtures/golden/claude-ship-SKILL.md | 10 +- test/fixtures/golden/codex-ship-SKILL.md | 10 +- test/fixtures/golden/factory-ship-SKILL.md | 10 +- test/fixtures/skill-md-size-allowlist.json | 10 +- test/review-scope-followups.test.ts | 2 +- test/review-severity-floor.test.ts | 103 ++++++++++++++++++ test/review-workflow-clarity.test.ts | 6 +- .../ship/gsta160-severity-floor.json | 4 + test/skill-validation.test.ts | 12 +- 23 files changed, 211 insertions(+), 82 deletions(-) create mode 100644 scripts/resolvers/review-floor.ts create mode 100644 test/review-severity-floor.test.ts create mode 100644 test/size-ledger/ship/gsta160-severity-floor.json diff --git a/docs/review-policy.md b/docs/review-policy.md index 032d87fea..bc43fdeee 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -10,11 +10,14 @@ instead of restating it. A one-line crew rule is enough: ## The blocking floor -A finding is **blocking** when it is a CRITICAL defect: it would fail a test, -break production, or lose data or secrets. Everything else is below the floor: -an advisory, an informational item, a test finding (category `testing`: a gap, -a weak or flaky test), a wording or naming note, a style or simplification -suggestion. Below-floor findings are reported, never fixed after pass 1 of a +A finding is **blocking** when it is a CRITICAL defect. CRITICAL is the blocking +floor: a finding that would fail a test, break production, lose data or secrets, or +open a security hole (injection, auth bypass, secret exposure). Everything else is +INFORMATIONAL. Below the floor are an advisory, an informational item, a test +finding (category `testing`: a gap, a weak or flaky test), a wording or naming +note, a style or simplification suggestion. A checklist pass, category or +specialist lens never sets severity; a product defect the Testing specialist finds +at the floor carries category `correctness`, so it is classified as a defect. Below-floor findings are reported, never fixed after pass 1 of a ship, and never earn another review pass. Three words cover every finding, and the loop budgets by them: a **blocker** is diff --git a/docs/ship-policy.md b/docs/ship-policy.md index 025ada534..9ef4fe759 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -984,8 +984,9 @@ construction: `gstack-review-scope classify --base --pass 1 --findings `, which gives each one a `locus` (`diff`: a hunk the PR changed, with three lines of context; `outside`: anything else), a `kind`, `blocking` and a `scope`. - `blocking` is a CRITICAL defect: a test that would fail, a production break, a - security or data-loss defect. Everything else (an advisory, an informational + `blocking` is a CRITICAL defect. CRITICAL is the blocking floor: a finding that + would fail a test, break production, lose data or secrets, or open a security hole + (injection, auth bypass, secret exposure). Everything else is INFORMATIONAL. Everything else (an advisory, an informational item, a test gap, a wording note) is below the floor. Pass 1 fixes findings in the diff (`scope: fix`). A blocking finding in code the PR did not change is `scope: defer`: `gstack-review-scope defer` files it as a GitHub issue (reusing diff --git a/review/SKILL.md b/review/SKILL.md index d8aba2c97..daa69f788 100644 --- a/review/SKILL.md +++ b/review/SKILL.md @@ -457,10 +457,10 @@ Use this host's installation, never the product tree. If missing or unreadable, QA's `sections/...` and `templates/...` paths resolve from installed QA SKILL.md, not caller/product. -Apply both checklist passes in order: CRITICAL, then INFORMATIONAL. Respect its suppressions. -CRITICAL is the blocking floor (docs/review-policy.md): a finding that would fail a -test, break production, or lose data or secrets. Everything else is INFORMATIONAL and -never earns another pass. `NO FINDINGS` is a complete result from any reviewer. +Apply both checklist passes in order with its suppressions; neither sets severity. +CRITICAL is the blocking floor: a finding that would fail a test, break production, +lose data or secrets, or open a security hole (injection, auth bypass, secret exposure). +Everything else is INFORMATIONAL. `NO FINDINGS` is a complete result from any reviewer. **Sideways sweep (every finding; reads code OUTSIDE the diff).** Grep the finding's pattern at every other ingress, command and call site, Read each match and rate severity on the worst sibling; a new enum value, status, tier or type constant is traced through every consumer the same way (checklist: Sideways sweep). Data loss, a trust-boundary bypass or uncapped spend is forced CRITICAL: no lane, specialist, dedup or saved decision downgrades it. Keep findings anchored to changed code. diff --git a/review/SKILL.md.tmpl b/review/SKILL.md.tmpl index b22eaf442..45f0fce03 100644 --- a/review/SKILL.md.tmpl +++ b/review/SKILL.md.tmpl @@ -169,10 +169,10 @@ the core review. {{QA_REVIEW_PREFLIGHT}} -Apply both checklist passes in order: CRITICAL, then INFORMATIONAL. Respect its suppressions. -CRITICAL is the blocking floor (docs/review-policy.md): a finding that would fail a -test, break production, or lose data or secrets. Everything else is INFORMATIONAL and -never earns another pass. `NO FINDINGS` is a complete result from any reviewer. +Apply both checklist passes in order with its suppressions; neither sets severity. +CRITICAL is the blocking floor: a finding that would fail a test, break production, +lose data or secrets, or open a security hole (injection, auth bypass, secret exposure). +Everything else is INFORMATIONAL. `NO FINDINGS` is a complete result from any reviewer. {{SIDEWAYS_SWEEP}} diff --git a/review/checklist.md b/review/checklist.md index b1dda2e75..3b284f31b 100644 --- a/review/checklist.md +++ b/review/checklist.md @@ -5,8 +5,9 @@ Review the merge-base diff from the caller, including its selected uncommitted and new source. Use the caller's detected base, not a hardcoded branch. Cite `file:line` and suggest fixes. Only flag real problems. **Two-pass review:** -- **Pass 1 (CRITICAL):** Run SQL & Data Safety, Race Conditions, LLM Output Trust Boundary, Shell Injection, Enum Completeness, and Gate Integrity first. Highest severity. -- **Pass 2 (INFORMATIONAL):** Run remaining categories below. Lower severity but still actioned. +- **Pass 1 (check first):** Run SQL & Data Safety, Race Conditions, LLM Output Trust Boundary, Shell Injection, Enum Completeness, and Gate Integrity first. These categories hold blocking defects most often. +- **Pass 2 (check second):** Run remaining categories below. Still actioned. +- **The pass never sets severity.** Rate every finding from either pass by the blocking floor (Severity Classification below): a Pass 2 finding that would fail a test, break production, lose data or secrets, or open a security hole is CRITICAL, and a Pass 1 finding that would not is INFORMATIONAL. - **Specialist categories (handled by parallel subagents, NOT this checklist):** Test Gaps, Dead Code, Magic Numbers, Conditional Side Effects, Performance & Bundle Impact, Crypto & Entropy, Simplification (unrequested structure). See `review/specialists/` for these. Completeness Gaps and Simplification are orthogonal, not contradictory: Completeness pushes coverage UP (tests, edge cases, error paths), Simplification pushes unrequested structure DOWN (one-implementation abstractions, hand-rolled stdlib, dead flexibility). The same diff can legitimately receive both. @@ -35,7 +36,7 @@ Be terse. For each issue: one line describing the problem, one line with the fix ## Review Categories -### Pass 1 — CRITICAL +### Pass 1 — check first #### SQL & Data Safety - String interpolation in SQL (even if values are `.to_i`/`.to_f` — use parameterized queries (Rails: sanitize_sql_array/Arel; Node: prepared statements; Python: parameterized queries)) @@ -48,6 +49,7 @@ Be terse. For each issue: one line describing the problem, one line with the fix - find-or-create without unique DB index — concurrent calls can create duplicates - Status transitions that don't use atomic `WHERE old_status = ? UPDATE SET new_status` — concurrent updates can skip or double-apply transitions - Unsafe HTML rendering (Rails: .html_safe/raw(); React: dangerouslySetInnerHTML; Vue: v-html; Django: |safe/mark_safe) on user-controlled data (XSS) +- State swapped before the step that can fail: current handles, pointers or registered handlers (a browser, connection or context, an exit-on-disconnect listener) replaced before a restore, connect or write that can throw, with no snapshot and rollback, so a failure leaves the object on closed or partial state or fires a handler meant for the new state. Swap after the step succeeds, or snapshot and restore in the catch. #### LLM Output Trust Boundary - LLM-generated values (emails, URLs, names) written to DB or passed to mailers without format validation. Add lightweight guards (`EMAIL_REGEXP`, `URI.parse`, `.strip`) before persisting. @@ -55,10 +57,11 @@ Be terse. For each issue: one line describing the problem, one line with the fix - LLM-generated URLs fetched without allowlist — SSRF risk if URL points to internal network (Python: `urllib.parse.urlparse` → check hostname against blocklist before `requests.get`/`httpx.get`) - LLM output stored in knowledge bases or vector DBs without sanitization — stored prompt injection risk -#### Shell Injection (Python-specific) -- `subprocess.run()` / `subprocess.call()` / `subprocess.Popen()` with `shell=True` AND f-string/`.format()` interpolation in the command string — use argument arrays instead +#### Shell Injection +- Python: `subprocess.run()` / `subprocess.call()` / `subprocess.Popen()` with `shell=True` AND f-string/`.format()` interpolation in the command string — use argument arrays instead - `os.system()` with variable interpolation — replace with `subprocess.run()` using argument arrays - `eval()` / `exec()` on LLM-generated code without sandboxing +- Shell scripts: `eval`, `source`/`.`, `sh -c`/`bash -c`, or output a caller evals or sources (`eval "$(tool)"`), built from a value read from a file, environment variable, argument or network that sibling paths validate or sanitize and this path does not (a cached value echoed without the filter the compute path applies). Name the reaching input. Not a finding by itself: a quoted expansion passed as one argument (`"$x"`), or shell metacharacters inside a literal. #### Enum & Value Completeness When the diff introduces a new enum value, status string, tier name, or type constant, or loosens what an input accepts (a new MIME type, extension or flag, or a widened validator/parser/guard): @@ -84,7 +87,7 @@ A gate is anything that would have made this change fail: a test, an assertion, - Run `~/.claude/skills/gstack/bin/gstack-approval-classes check --base `. Each declared `APPROVAL_CLASS:` line (an upgrade-time row rewrite, a trust or visibility change, an existing-user default flip, a data repair, as the repo declares them) is a finding that needs the owner's approval: name its `APPROVAL_SCOPE: @` token, because /ship's receipt refuses (`APPROVAL_CITATION_MISSING`) until the PR body cites an approval bound to it. The approval is the owner's, never the reviewer's. `(built-in, report-only)` lines are informational; `none` means no declared classes. - A data repair must be explicit (its own command or flag), preview-first (a dry run that lists the rows before any write) and unreachable from upgrade, migration or startup paths. Flag a repair that runs by default, writes without a preview, or is called from an upgrade or migration. -### Pass 2 — INFORMATIONAL +### Pass 2 — check second #### Async/Sync Mixing (Python-specific) - Synchronous `subprocess.run()`, `open()`, `requests.get()` inside `async def` endpoints — blocks the event loop. Use `asyncio.to_thread()`, `aiofiles`, or `httpx.AsyncClient` instead. @@ -161,8 +164,17 @@ A finding is a defect class, not a line. Before rating any finding from either p ## Severity Classification +CRITICAL is the blocking floor: a finding that would fail a test, break production, +lose data or secrets, or open a security hole (injection, auth bypass, secret exposure). +Everything else is INFORMATIONAL. + +A category says what to check and in which order; it never sets severity. Rate every +finding (core Pass 1 or Pass 2, a specialist, the Red Team) by the floor above. Gate +Integrity: a read hunk that fails its one question is CRITICAL (the relaxation silences +a failing gate); a hunk that passes it is not a finding. + ``` -CRITICAL (highest severity): INFORMATIONAL (main agent): SPECIALIST (parallel subagents): +PASS 1 (check first): PASS 2 (check second): SPECIALIST (parallel subagents): ├─ SQL & Data Safety ├─ Async/Sync Mixing ├─ Testing specialist ├─ Race Conditions & Concurrency ├─ Column/Field Name Safety ├─ Maintainability specialist ├─ LLM Output Trust Boundary ├─ Dead Code (version only) ├─ Security specialist @@ -211,7 +223,7 @@ the fix, it's ASK. **Informational findings default toward AUTO-FIX** (they're more mechanical). **Severity glossary** (the words /ship's review loop budgets by; docs/review-policy.md): -- **blocker**: the intended case is wrong. A test would fail, production breaks, or data or secrets are lost. CRITICAL; fixed on every pass, or named to the owner at the cap. +- **blocker**: the intended case is wrong. A test would fail, production breaks, data or secrets are lost, or a security hole opens. CRITICAL; fixed on every pass, or named to the owner at the cap. - **should**: a real bug a caller will hit, inside the intended case. Fixed when it is in the diff on pass 1; filed and deferred when it is in code the PR did not change. - **nit**: everything else, including style, naming, wording and "I would have done it differently". A named note in the PR body; never a reason for another pass. Two stop rules follow from the budget: if a pass's fixes grew the diff past what the request needs, cut that growth first; and reaching the cap approves nothing, so a standing blocker is reported, not waved through. diff --git a/review/sections/review-army.md b/review/sections/review-army.md index fa3d1de5b..4e6f808ba 100644 --- a/review/sections/review-army.md +++ b/review/sections/review-army.md @@ -106,14 +106,13 @@ Every other specialist, including Testing, reads the full diff with fixtures. Required fields: severity, confidence, path, category, summary, specialist. Optional: line, fix, fingerprint, evidence, test_stub, advisory, evidence_paths, helper_target, cause. -CRITICAL means a test that would fail, a production break, or a security or data-loss defect; everything else is INFORMATIONAL. +CRITICAL is the blocking floor: a finding that would fail a test, break production, lose data or secrets, or open a security hole (injection, auth bypass, secret exposure). Everything else is INFORMATIONAL. A defect the diff causes but that shows elsewhere (a caller broken by a changed signature, a consumer of a removed export) keeps the victim as `path` and names the changed line in `cause`: {\"cause\":{\"path\":\"file\",\"line\":N}}. Without it the finding reads as pre-existing code. Leave extraction advice to the core shared-code check; report real defects in duplicated code independently, keep advisory metadata on structural advice, and never call a demonstrated defect advisory because a shared helper could fix it. -If you can write a test that would catch this issue, include it in the `test_stub` field. -Use the detected test framework ({TEST_FW}). Write a minimal skeleton — describe/it/test -blocks with clear intent. Skip test_stub for architectural or design-only findings. +For a finding a test could catch, add a minimal `test_stub` skeleton in the detected test +framework ({TEST_FW}); skip it for architectural or design-only findings. Output nothing else: no preamble, no summary or commentary. diff --git a/review/specialists/testing.md b/review/specialists/testing.md index b1ca5cb40..731b3b982 100644 --- a/review/specialists/testing.md +++ b/review/specialists/testing.md @@ -4,6 +4,7 @@ Scope: Always-on (every review) Output: JSON objects, one finding per line. Schema: {"severity":"CRITICAL|INFORMATIONAL","confidence":N,"path":"file","line":N,"category":"testing","summary":"...","fix":"...","fingerprint":"path:line:testing","specialist":"testing"} Optional: line, fix, fingerprint, evidence, test_stub. +Category `testing` is for test findings (a missing, weak or flaky test), which sit below the blocking floor. A product-code defect you find that meets the floor uses category `correctness` and fingerprint `path:line:correctness`, with its CRITICAL severity, so it is classified as a defect, not a test gap. If no findings: output `NO FINDINGS` and nothing else. If the caller explicitly asks for an ASCII coverage diagram, first read the diff --git a/scripts/resolvers/fix-check.ts b/scripts/resolvers/fix-check.ts index 58775b2f3..f41d99ab6 100644 --- a/scripts/resolvers/fix-check.ts +++ b/scripts/resolvers/fix-check.ts @@ -3,6 +3,8 @@ * step whose fixes do not earn a full pass. It reads the fixes, never the whole diff. The parent * writes every input in literally: a reviewer never sees the parent's shell. */ +import { BLOCKING_FLOOR } from './review-floor'; + export interface FixCheckInput { fixList: string; touchedHunks: string; callers: string; testResults: string } export const FIX_CHECK_INSTRUCTIONS = `You are the fix-check reviewer for a pre-landing review. An earlier review pass found @@ -14,8 +16,7 @@ fails open (accepts what it refused) or fails closed (refuses what it accepted), behaviour the code had before the fix, an error or unreadable state now read as success, a test made weaker. Name the input or call site that reaches each defect. -Rate each finding against the blocking floor: CRITICAL only when it would fail a test, break -production, or lose data or secrets; everything else is INFORMATIONAL. Output one JSON object per +Rate each finding against the blocking floor. ${BLOCKING_FLOOR} Output one JSON object per line, {"severity":"CRITICAL|INFORMATIONAL","path":"","line":,"summary":""}, or the two words NO FINDINGS when the fixes are sound.`; diff --git a/scripts/resolvers/review-army.ts b/scripts/resolvers/review-army.ts index f4142ee0e..0209500a5 100644 --- a/scripts/resolvers/review-army.ts +++ b/scripts/resolvers/review-army.ts @@ -13,6 +13,7 @@ import type { TemplateContext } from './types'; import { CC_BACKGROUND_DEFAULT_SINCE, FOREGROUND_IF_AVAILABLE, BACKGROUND_RECOVERY } from './constants'; import { learningsCapture, LEARNINGS_VERDICT } from './learnings'; import { DEFENSIVE_REVIEW_FRAMING, FIXTURE_SUMMARY_MODE } from './defensive-review'; +import { BLOCKING_FLOOR } from './review-floor'; function generateSpecialistSelection(ctx: TemplateContext): string { const isShip = ctx.skillName === 'ship'; @@ -146,14 +147,13 @@ Every other specialist, including Testing, reads the full diff with fixtures. Required fields: severity, confidence, path, category, summary, specialist. Optional: line, fix, fingerprint, evidence, test_stub, advisory, evidence_paths, helper_target, cause. -CRITICAL means a test that would fail, a production break, or a security or data-loss defect; everything else is INFORMATIONAL. +${BLOCKING_FLOOR} A defect the diff causes but that shows elsewhere (a caller broken by a changed signature, a consumer of a removed export) keeps the victim as \`path\` and names the changed line in \`cause\`: {\\"cause\\":{\\"path\\":\\"file\\",\\"line\\":N}}. Without it the finding reads as pre-existing code. Leave extraction advice to the core shared-code check; report real defects in duplicated code independently, keep advisory metadata on structural advice, and never call a demonstrated defect advisory because a shared helper could fix it. -If you can write a test that would catch this issue, include it in the \`test_stub\` field. -Use the detected test framework ({TEST_FW}). Write a minimal skeleton — describe/it/test -blocks with clear intent. Skip test_stub for architectural or design-only findings. +For a finding a test could catch, add a minimal \`test_stub\` skeleton in the detected test +framework ({TEST_FW}); skip it for architectural or design-only findings. Output nothing else: no preamble, no summary or commentary. diff --git a/scripts/resolvers/review-floor.ts b/scripts/resolvers/review-floor.ts new file mode 100644 index 000000000..9336c385b --- /dev/null +++ b/scripts/resolvers/review-floor.ts @@ -0,0 +1,6 @@ +/** + * The blocking floor every gstack reviewer prompt carries (docs/review-policy.md#the-blocking-floor). + * One sentence, so the core checklist, each specialist, the Red Team and the fix-check rate + * severity the same way. test/review-severity-floor.test.ts keeps the markdown copies in step. + */ +export const BLOCKING_FLOOR = 'CRITICAL is the blocking floor: a finding that would fail a test, break production, lose data or secrets, or open a security hole (injection, auth bypass, secret exposure). Everything else is INFORMATIONAL.'; diff --git a/ship/SKILL.md b/ship/SKILL.md index 9fe409012..c3e02da49 100644 --- a/ship/SKILL.md +++ b/ship/SKILL.md @@ -208,11 +208,11 @@ token; `--finish` stamps the binding fields automatically. Never borrow or repla not a commit ID. Use `git diff ` to compare these snapshots. **Review policy** (docs/review-policy.md): only a finding that would fail a test, -break production, or lose data or secrets blocks a pass; the rest is a PR-body -note. Step 9's plan sets both reviews' depth (`--depth-floor standard|full` only -raises it). Pass 1 reads the diff, later passes the range; the cap is two full -passes and a fix-check. `NO FINDINGS` is complete. `ship: review_scope` (Step 3) -is the knob; the receipt's `review_loop` records the outcome. +break production, lose data or secrets, or open a security hole blocks a pass; the +rest is a PR-body note. Step 9's plan sets both reviews' depth (`--depth-floor +standard|full` only raises it). Pass 1 reads the diff, later passes the range; the +cap is two full passes and a fix-check. `NO FINDINGS` is complete. `ship: +review_scope` (Step 3) is the knob. ### Ship control flow diff --git a/ship/SKILL.md.tmpl b/ship/SKILL.md.tmpl index 8eaf72403..bbf6d4861 100644 --- a/ship/SKILL.md.tmpl +++ b/ship/SKILL.md.tmpl @@ -74,11 +74,11 @@ token; `--finish` stamps the binding fields automatically. Never borrow or repla not a commit ID. Use `git diff ` to compare these snapshots. **Review policy** (docs/review-policy.md): only a finding that would fail a test, -break production, or lose data or secrets blocks a pass; the rest is a PR-body -note. Step 9's plan sets both reviews' depth (`--depth-floor standard|full` only -raises it). Pass 1 reads the diff, later passes the range; the cap is two full -passes and a fix-check. `NO FINDINGS` is complete. `ship: review_scope` (Step 3) -is the knob; the receipt's `review_loop` records the outcome. +break production, lose data or secrets, or open a security hole blocks a pass; the +rest is a PR-body note. Step 9's plan sets both reviews' depth (`--depth-floor +standard|full` only raises it). Pass 1 reads the diff, later passes the range; the +cap is two full passes and a fix-check. `NO FINDINGS` is complete. `ship: +review_scope` (Step 3) is the knob. ### Ship control flow diff --git a/ship/sections/review-army.md b/ship/sections/review-army.md index 179588813..160d7b5be 100644 --- a/ship/sections/review-army.md +++ b/ship/sections/review-army.md @@ -406,14 +406,13 @@ Every other specialist, including Testing, reads the full diff with fixtures. Required fields: severity, confidence, path, category, summary, specialist. Optional: line, fix, fingerprint, evidence, test_stub, advisory, evidence_paths, helper_target, cause. -CRITICAL means a test that would fail, a production break, or a security or data-loss defect; everything else is INFORMATIONAL. +CRITICAL is the blocking floor: a finding that would fail a test, break production, lose data or secrets, or open a security hole (injection, auth bypass, secret exposure). Everything else is INFORMATIONAL. A defect the diff causes but that shows elsewhere (a caller broken by a changed signature, a consumer of a removed export) keeps the victim as `path` and names the changed line in `cause`: {\"cause\":{\"path\":\"file\",\"line\":N}}. Without it the finding reads as pre-existing code. Leave extraction advice to the core shared-code check; report real defects in duplicated code independently, keep advisory metadata on structural advice, and never call a demonstrated defect advisory because a shared helper could fix it. -If you can write a test that would catch this issue, include it in the `test_stub` field. -Use the detected test framework ({TEST_FW}). Write a minimal skeleton — describe/it/test -blocks with clear intent. Skip test_stub for architectural or design-only findings. +For a finding a test could catch, add a minimal `test_stub` skeleton in the detected test +framework ({TEST_FW}); skip it for architectural or design-only findings. Output nothing else: no preamble, no summary or commentary. diff --git a/test/fixtures/golden/agy-ship-SKILL.md b/test/fixtures/golden/agy-ship-SKILL.md index 837cec340..2bfab079d 100644 --- a/test/fixtures/golden/agy-ship-SKILL.md +++ b/test/fixtures/golden/agy-ship-SKILL.md @@ -208,11 +208,11 @@ token; `--finish` stamps the binding fields automatically. Never borrow or repla not a commit ID. Use `git diff ` to compare these snapshots. **Review policy** (docs/review-policy.md): only a finding that would fail a test, -break production, or lose data or secrets blocks a pass; the rest is a PR-body -note. Step 9's plan sets both reviews' depth (`--depth-floor standard|full` only -raises it). Pass 1 reads the diff, later passes the range; the cap is two full -passes and a fix-check. `NO FINDINGS` is complete. `ship: review_scope` (Step 3) -is the knob; the receipt's `review_loop` records the outcome. +break production, lose data or secrets, or open a security hole blocks a pass; the +rest is a PR-body note. Step 9's plan sets both reviews' depth (`--depth-floor +standard|full` only raises it). Pass 1 reads the diff, later passes the range; the +cap is two full passes and a fix-check. `NO FINDINGS` is complete. `ship: +review_scope` (Step 3) is the knob. ### Ship control flow diff --git a/test/fixtures/golden/claude-ship-SKILL.md b/test/fixtures/golden/claude-ship-SKILL.md index 9fe409012..c3e02da49 100644 --- a/test/fixtures/golden/claude-ship-SKILL.md +++ b/test/fixtures/golden/claude-ship-SKILL.md @@ -208,11 +208,11 @@ token; `--finish` stamps the binding fields automatically. Never borrow or repla not a commit ID. Use `git diff ` to compare these snapshots. **Review policy** (docs/review-policy.md): only a finding that would fail a test, -break production, or lose data or secrets blocks a pass; the rest is a PR-body -note. Step 9's plan sets both reviews' depth (`--depth-floor standard|full` only -raises it). Pass 1 reads the diff, later passes the range; the cap is two full -passes and a fix-check. `NO FINDINGS` is complete. `ship: review_scope` (Step 3) -is the knob; the receipt's `review_loop` records the outcome. +break production, lose data or secrets, or open a security hole blocks a pass; the +rest is a PR-body note. Step 9's plan sets both reviews' depth (`--depth-floor +standard|full` only raises it). Pass 1 reads the diff, later passes the range; the +cap is two full passes and a fix-check. `NO FINDINGS` is complete. `ship: +review_scope` (Step 3) is the knob. ### Ship control flow diff --git a/test/fixtures/golden/codex-ship-SKILL.md b/test/fixtures/golden/codex-ship-SKILL.md index 900201136..70f6a7018 100644 --- a/test/fixtures/golden/codex-ship-SKILL.md +++ b/test/fixtures/golden/codex-ship-SKILL.md @@ -206,11 +206,11 @@ token; `--finish` stamps the binding fields automatically. Never borrow or repla not a commit ID. Use `git diff ` to compare these snapshots. **Review policy** (docs/review-policy.md): only a finding that would fail a test, -break production, or lose data or secrets blocks a pass; the rest is a PR-body -note. Step 9's plan sets both reviews' depth (`--depth-floor standard|full` only -raises it). Pass 1 reads the diff, later passes the range; the cap is two full -passes and a fix-check. `NO FINDINGS` is complete. `ship: review_scope` (Step 3) -is the knob; the receipt's `review_loop` records the outcome. +break production, lose data or secrets, or open a security hole blocks a pass; the +rest is a PR-body note. Step 9's plan sets both reviews' depth (`--depth-floor +standard|full` only raises it). Pass 1 reads the diff, later passes the range; the +cap is two full passes and a fix-check. `NO FINDINGS` is complete. `ship: +review_scope` (Step 3) is the knob. ### Ship control flow diff --git a/test/fixtures/golden/factory-ship-SKILL.md b/test/fixtures/golden/factory-ship-SKILL.md index 4c62cb563..977e5421d 100644 --- a/test/fixtures/golden/factory-ship-SKILL.md +++ b/test/fixtures/golden/factory-ship-SKILL.md @@ -208,11 +208,11 @@ token; `--finish` stamps the binding fields automatically. Never borrow or repla not a commit ID. Use `git diff ` to compare these snapshots. **Review policy** (docs/review-policy.md): only a finding that would fail a test, -break production, or lose data or secrets blocks a pass; the rest is a PR-body -note. Step 9's plan sets both reviews' depth (`--depth-floor standard|full` only -raises it). Pass 1 reads the diff, later passes the range; the cap is two full -passes and a fix-check. `NO FINDINGS` is complete. `ship: review_scope` (Step 3) -is the knob; the receipt's `review_loop` records the outcome. +break production, lose data or secrets, or open a security hole blocks a pass; the +rest is a PR-body note. Step 9's plan sets both reviews' depth (`--depth-floor +standard|full` only raises it). Pass 1 reads the diff, later passes the range; the +cap is two full passes and a fix-check. `NO FINDINGS` is complete. `ship: +review_scope` (Step 3) is the knob. ### Ship control flow diff --git a/test/fixtures/skill-md-size-allowlist.json b/test/fixtures/skill-md-size-allowlist.json index 03d779fc1..7a161425f 100644 --- a/test/fixtures/skill-md-size-allowlist.json +++ b/test/fixtures/skill-md-size-allowlist.json @@ -22,13 +22,13 @@ "cursor/gstack-plan-design-review": 108900, "cursor/gstack-plan-devex-review": 115228, "cursor/gstack-plan-eng-review": 143662, - "cursor/gstack-review": 121614, + "cursor/gstack-review": 121594, "factory/gstack-autoplan": 110763, "factory/gstack-land-and-deploy": 100105, "factory/gstack-plan-design-review": 111675, "factory/gstack-plan-devex-review": 118591, "factory/gstack-plan-eng-review": 146770, - "factory/gstack-review": 125387, + "factory/gstack-review": 125367, "gbrain/gstack-autoplan": 108892, "gbrain/gstack-plan-eng-review": 126289, "hermes/gstack-autoplan": 108891, @@ -37,18 +37,18 @@ "kiro/gstack-plan-design-review": 108752, "kiro/gstack-plan-devex-review": 115080, "kiro/gstack-plan-eng-review": 143524, - "kiro/gstack-review": 121438, + "kiro/gstack-review": 121418, "openclaw/gstack-autoplan": 108715, "openclaw/gstack-plan-eng-review": 124804, "opencode/gstack-autoplan": 109234, "opencode/gstack-plan-design-review": 110187, "opencode/gstack-plan-devex-review": 115836, "opencode/gstack-plan-eng-review": 144931, - "opencode/gstack-review": 122236, + "opencode/gstack-review": 122216, "slate/gstack-autoplan": 108372, "slate/gstack-plan-design-review": 108826, "slate/gstack-plan-devex-review": 115154, "slate/gstack-plan-eng-review": 143593, - "slate/gstack-review": 121526 + "slate/gstack-review": 121506 } } diff --git a/test/review-scope-followups.test.ts b/test/review-scope-followups.test.ts index ec72f31c5..eec0547ec 100644 --- a/test/review-scope-followups.test.ts +++ b/test/review-scope-followups.test.ts @@ -120,7 +120,7 @@ describe('checklist rules with ids (docs/review-policy.md)', () => { }); test('test-keyboard-count-loop is a Pass 2 informational rule that keeps a counted loop only when the count is the behavior', () => { - const pass2 = section('### Pass 2 — INFORMATIONAL', '## Sideways sweep'); + const pass2 = section('### Pass 2 — check second', '## Sideways sweep'); expect(pass2).toContain('#### Keyboard-Count Test Loops'); expect(pass2).toContain('(`test-keyboard-count-loop`)'); expect(pass2).toContain('Locate the target by role, label or test id'); diff --git a/test/review-severity-floor.test.ts b/test/review-severity-floor.test.ts new file mode 100644 index 000000000..3e349af68 --- /dev/null +++ b/test/review-severity-floor.test.ts @@ -0,0 +1,103 @@ +/** + * GSTA-160: /review's severity follows the blocking floor, never the checklist pass, + * category or specialist lens. In GSTA-11's diff-only replay the core reviewer filed a + * real /retro date-window bug INFORMATIONAL in 3 of 3 trials under the Pass 2 category + * name, and a cache-file shell injection as a "security-invariant" note, because the + * checklist bound severity to category and the core floor had no security clause. + * One floor sentence (scripts/resolvers/review-floor.ts) now reaches every reviewer. + */ +import { afterEach, describe, expect, test } from 'bun:test'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import { EXIT } from '../lib/headless-artifacts'; +import { type ScopedFinding } from '../lib/review-scope'; +import { BLOCKING_FLOOR } from '../scripts/resolvers/review-floor'; +import { FIX_CHECK_INSTRUCTIONS } from '../scripts/resolvers/fix-check'; +import { cleanup, makeFixtureRepo, runBin, type FixtureRepo } from './helpers/restamp-fixture'; + +const ROOT = path.resolve(import.meta.dir, '..'); +const flat = (s: string) => s.replace(/\s+/g, ' '); +const read = (p: string) => flat(fs.readFileSync(path.join(ROOT, p), 'utf8')); + +const repos: FixtureRepo[] = []; +afterEach(() => { while (repos.length) cleanup(repos.pop()!); }); + +describe('one blocking floor in every reviewer prompt', () => { + test('the floor names security and keeps the 2026-10-26 words', () => { + expect(BLOCKING_FLOOR).toContain('would fail a test, break production, lose data or secrets, or open a security hole'); + }); + + test.each([ + 'review/SKILL.md.tmpl', 'review/SKILL.md', 'review/checklist.md', 'review/sections/review-army.md', + 'ship/sections/review-army.md', 'docs/review-policy.md', 'docs/ship-policy.md', + ])('%s carries the floor sentence verbatim', file => { + expect(read(file)).toContain(BLOCKING_FLOOR); + }); + + test('the ship skeleton Review policy line names the same four outcomes', () => { + for (const file of ['ship/SKILL.md.tmpl', 'ship/SKILL.md']) expect(read(file)).toContain('only a finding that would fail a test, break production, lose data or secrets, or open a security hole blocks a pass'); + }); + + test('the fix-check prompt carries it too', () => { + expect(flat(FIX_CHECK_INSTRUCTIONS)).toContain(BLOCKING_FLOOR); + }); +}); + +describe('the checklist sets check order, never severity', () => { + const checklist = read('review/checklist.md'); + test('no pass or category is named as a severity', () => { + for (const bound of ['Pass 1 (CRITICAL)', 'Pass 2 (INFORMATIONAL)', 'Pass 1 — CRITICAL', 'Pass 2 — INFORMATIONAL', 'Highest severity', 'Lower severity', 'CRITICAL (highest severity)', 'INFORMATIONAL (main agent)']) { + expect(checklist).not.toContain(bound); + } + expect(checklist).toContain('The pass never sets severity.'); + expect(checklist).toContain('A category says what to check and in which order; it never sets severity.'); + expect(read('review/SKILL.md.tmpl')).toContain('Apply both checklist passes in order with its suppressions; neither sets severity.'); + expect(read('review/SKILL.md.tmpl')).not.toContain('CRITICAL, then INFORMATIONAL'); + }); + + test('shell injection is not Python-only and names shell sinks with a reaching input', () => { + expect(checklist).not.toContain('Shell Injection (Python-specific)'); + expect(checklist).toContain('Shell scripts: `eval`, `source`/`.`, `sh -c`/`bash -c`'); + expect(checklist).toContain('Name the reaching input.'); + expect(checklist).toContain('Not a finding by itself: a quoted expansion passed as one argument'); + }); + + test('state swapped before a step that can fail is a Pass 1 check (GSTA-159, gstack #201)', () => { + const pass1 = checklist.slice(checklist.indexOf('### Pass 1 — check first'), checklist.indexOf('### Pass 2 — check second')); + expect(pass1).toContain('State swapped before the step that can fail'); + expect(pass1).toContain('with no snapshot and rollback'); + }); +}); + +describe('a floor-level defect the Testing specialist finds still blocks', () => { + test('testing.md routes product defects to category correctness', () => { + expect(read('review/specialists/testing.md')).toContain('A product-code defect you find that meets the floor uses category `correctness`'); + }); + + test('gstack-review-scope classify: a CRITICAL correctness or Pass 2-category finding blocks; a testing-category gap does not', () => { + const repo = makeFixtureRepo({ prefix: 'review-floor-' }); + repos.push(repo); + const src = path.join(repo.work, 'browse', 'src'); + fs.mkdirSync(src, { recursive: true }); + fs.writeFileSync(path.join(src, 'cookie-picker-ui.ts'), Array.from({ length: 20 }, (_, i) => `line ${i + 1}`).join('\n') + '\n'); + repo.git('add', '.'); repo.git('commit', '-qm', 'seed'); repo.git('push', '-q', 'origin', 'main'); + repo.git('checkout', '-qb', 'feat'); + const lines = fs.readFileSync(path.join(src, 'cookie-picker-ui.ts'), 'utf8').split('\n'); + lines[9] = "const AUTH_TOKEN = '${authToken}';"; + fs.writeFileSync(path.join(src, 'cookie-picker-ui.ts'), lines.join('\n')); + repo.git('commit', '-qam', 'inline the token'); + const at = 'browse/src/cookie-picker-ui.ts'; + const findings = [ + { fingerprint: at + ':10:correctness', path: at, line: 10, severity: 'CRITICAL', confidence: 7, category: 'correctness', specialist: 'testing', summary: 'The unauthenticated GET /cookie-picker serves HTML containing AUTH_TOKEN.' }, + { fingerprint: at + ':10:time-window-safety', path: at, line: 10, severity: 'CRITICAL', confidence: 7, category: 'time-window-safety', specialist: 'core', summary: 'A bare --since date drops most of the first day.' }, + { fingerprint: at + ':10:testing', path: at, line: 10, severity: 'CRITICAL', confidence: 7, category: 'testing', specialist: 'testing', summary: 'No test covers the token-free page.' }, + ]; + const f = path.join(repo.root, 'findings.json'); + fs.writeFileSync(f, JSON.stringify(findings)); + const r = runBin(repo, 'gstack-review-scope', ['classify', '--base', 'main', '--pass', '1', '--findings', f, '--json']); + expect(r.status).toBe(EXIT.ok); + const got = JSON.parse(r.out).findings.map((x: ScopedFinding) => `${x.category}=${x.kind}/${x.blocking}`); + expect(got).toEqual(['correctness=defect/true', 'time-window-safety=defect/true', 'testing=test-gap/false']); + expect(r.err).toContain('blocking=2'); + }); +}); diff --git a/test/review-workflow-clarity.test.ts b/test/review-workflow-clarity.test.ts index 5637fbb41..dda22ba88 100644 --- a/test/review-workflow-clarity.test.ts +++ b/test/review-workflow-clarity.test.ts @@ -620,11 +620,11 @@ test('security specialist checks authorization paths a route-guard read misses', // sits in the core CRITICAL pass every diff size gets, its findings reach a // human through one four-option question, and the record carries identity // fields a later reader can act on. Tokens and order only; the prose is free. -test('Gate Integrity is a Pass 1 category that is ASK-only and keeps the three levels apart', () => { +test('Gate Integrity is a Pass 1 (check-first) category that is ASK-only and keeps the three levels apart', () => { const checklist = readFileSync(join(root, 'review/checklist.md'), 'utf8'); - const pass1 = between(checklist, '### Pass 1 — CRITICAL', '### Pass 2 — INFORMATIONAL'); + const pass1 = between(checklist, '### Pass 1 — check first', '### Pass 2 — check second'); expect(pass1).toContain('#### Gate Integrity'); - expect(between(checklist, '**Two-pass review:**', '**Output format:**')).toMatch(/Pass 1 \(CRITICAL\):[^\n]*Gate Integrity/); + expect(between(checklist, '**Two-pass review:**', '**Output format:**')).toMatch(/Pass 1 \(check first\):[^\n]*Gate Integrity/); const category = compact(between(pass1, '#### Gate Integrity')); expectTokens(category, ['GATE_SUMMARY:', 'RH-1', 'RH-3', 'RH-4', 'RH-12', 'RH-13', 'RH-14', 'RH-15', 'RH-16', 'RH-2', 'RH-5', '**Listed**', '**Read**', '**Finding**', 'gstack-decision-search --query ""'], 'Gate Integrity category'); diff --git a/test/size-ledger/ship/gsta160-severity-floor.json b/test/size-ledger/ship/gsta160-severity-floor.json new file mode 100644 index 000000000..75b63c274 --- /dev/null +++ b/test/size-ledger/ship/gsta160-severity-floor.json @@ -0,0 +1,4 @@ +{ + "bytes": -49, + "why": "GSTA-160: the blocking floor names security; the Review policy line and the specialist schema line were tightened to fit" +} diff --git a/test/skill-validation.test.ts b/test/skill-validation.test.ts index e4feb7a26..ff7e91a03 100644 --- a/test/skill-validation.test.ts +++ b/test/skill-validation.test.ts @@ -927,8 +927,8 @@ describe('Enum & Value Completeness in review checklist', () => { expect(checklist).toContain('Enum & Value Completeness'); }); - test('Enum & Value Completeness is classified as CRITICAL', () => { - // It should appear under Pass 1 — CRITICAL, not Pass 2 + test('Enum & Value Completeness is a Pass 1 (check-first) category', () => { + // It should appear under Pass 1 — check first, not Pass 2 const pass1Start = checklist.indexOf('### Pass 1'); const pass2Start = checklist.indexOf('### Pass 2'); const enumStart = checklist.indexOf('Enum & Value Completeness'); @@ -952,13 +952,13 @@ describe('Enum & Value Completeness in review checklist', () => { expect(stale.slice(0, stale.indexOf('\n### ', 5))).toContain('User-facing strings (errors, toasts, labels) whose guarding condition changed'); }); - test('Enum & Value Completeness is in the severity classification as CRITICAL', () => { + test('Enum & Value Completeness is in the Pass 1 column of the check-order tree', () => { const gateSection = checklist.slice(checklist.indexOf('## Severity Classification')); - // The ASCII art has CRITICAL on the left and INFORMATIONAL on the right - // Enum & Value Completeness should appear on a line with the CRITICAL tree (├─ or └─) + // The ASCII art has Pass 1 (check first) on the left and Pass 2 on the right + // Enum & Value Completeness should appear on a line of the Pass 1 tree (├─ or └─) const enumLine = gateSection.split('\n').find(l => l.includes('Enum & Value Completeness')); expect(enumLine).toBeDefined(); - // It's on the left (CRITICAL) side — starts with ├─ or └─ + // It's on the left (Pass 1) side — starts with ├─ or └─ expect(enumLine!.trimStart().startsWith('├─') || enumLine!.trimStart().startsWith('└─')).toBe(true); });