diff --git a/CHANGELOG.md b/CHANGELOG.md index 18b2a40..1b2689e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ Please choose versions by [Semantic Versioning](http://semver.org/). * MINOR version when you add functionality in a backwards-compatible manner, and * PATCH version when you make backwards-compatible bug fixes. +## Unreleased + +- feat: Wire the `--security` security-review mode into the review commands (`/coding:pr-review`, `/coding:code-review`, `/coding:local-review`) — activates the dormant selector-mode security extension (recon-derived session-local `/tmp/security-model.json`, six classifier trait groups with non-negotiable authz over-selection and deterministic invariant selection, verifier-gated high-severity findings, derived blocking, `SECURITY_MODEL_FILE` citation validation); JSON-rendered `Security Findings` + `Security Model` provenance report sections; PR-mode diff anchoring (pr-review/local-review) / whole-repo audit scope (code-review); fail-closed dependency toolchain pass; finalize acceptance scenarios 007-010 and register the mode in README.md + ## v0.48.0 - feat: Add agents/security-verifier.md — post-adjudication falsification gate (7-item checklist, verdict confirmed|plausible|rejected, counterevidence_checked on survivors) — and extend scripts/validate-citations.sh to the polymorphic citation contract (kind rule/invariant/toolchain, SECURITY_MODEL_FILE, fail-closed on absent model); register the agent and land three draft security-mode scenario fixtures diff --git a/README.md b/README.md index b23eb15..3159efa 100644 --- a/README.md +++ b/README.md @@ -33,6 +33,7 @@ Review your current branch against all guidelines: ``` /coding:pr-review +/coding:pr-review --security # security review mode: derived model + six trait groups + verifier gate + Security Findings report ``` Review local uncommitted changes in selector mode (default — zero LLM spawns, in-session classify + adjudicate) or full mode (per-owner dispatch, concurrent agents): @@ -66,9 +67,9 @@ Commit with changelog and version bump: | Command | Description | |---------|-------------| -| `/coding:pr-review` | Branch diff vs target — selector mode default; full mode = per-owner dispatch | -| `/coding:local-review [short\|selector\|full]` | Local uncommitted/recent diff vs `HEAD~1` — selector mode default | -| `/coding:code-review [--include-optional] [--refresh-baseline]` | Whole-codebase audit — severity-filtered (Must + Should) + baseline-aware (`.code-review-baseline.yaml`) | +| `/coding:pr-review` | Branch diff vs target — selector mode default; full mode = per-owner dispatch; add `--security` to run the security review pipeline | +| `/coding:local-review [short\|selector\|full]` | Local uncommitted/recent diff vs `HEAD~1` — selector mode default; add `--security` for the security review pipeline | +| `/coding:code-review [--include-optional] [--refresh-baseline]` | Whole-codebase audit — severity-filtered (Must + Should) + baseline-aware (`.code-review-baseline.yaml`); add `--security` for the security review pipeline | | `/coding:architecture-review [directory]` | Deep whole-codebase architectural review — top-down + dimensions, consolidated Must/Should/Could | | `/coding:check-guides "task"` | Find relevant guides before implementation | | `/coding:commit` | Git commit with changelog and versioning | @@ -284,6 +285,10 @@ End-to-end acceptance walks for the doc-driven review pipeline, following the [d | 002 | [clean-pr-zero-findings](scenarios/002-clean-pr-zero-findings.md) | `/coding:local-review master` against a zero-violation diff produces empty Must Fix / Should Fix / Nice to Have (no LLM hallucination) | | 003 | [scaling-funnel-100-files](scenarios/003-scaling-funnel-100-files.md) | 100-file synthetic fixture: mechanical funnel ≤30s, distinct Owners ≤30 (structural ceiling on Step 4b LLM calls) | | 004 | [findings-exist-path](scenarios/004-findings-exist-path.md) | `/coding:pr-review` against the stable test PR [bborbe/maintainer#2](https://github.com/bborbe/maintainer/pull/2): Step 4a surfaces ≥4 findings, every Owner has an agent file, citation discipline holds | +| 007 | [security-idor-confirmed](scenarios/007-security-idor-confirmed.md) | `--security` review of an order app with a seeded ownership-check bypass: verifier confirms the invariant IDOR finding (counterevidence_checked populated) and blocking holds | +| 008 | [security-idor-rejected-by-verifier](scenarios/008-security-idor-rejected-by-verifier.md) | `--security` review of an order app guarded by a service-layer ownership check: verifier rejects the naive IDOR claim (reject_reason recorded), no finding, no blocking | +| 009 | [security-toolchain-fail-closed](scenarios/009-security-toolchain-fail-closed.md) | toolchain findings pass citation validation; invariant findings fail closed without a model; a `--security` deps pass surfaces a vulnerable dependency as Must-Fix, never a silent skip | +| 010 | [security-zero-findings](scenarios/010-security-zero-findings.md) | `--security` review of a clean generic Go app: Security Findings section with zero findings + Security Model provenance block, no blocking | ## Contributing diff --git a/commands/code-review.md b/commands/code-review.md index 7b4c85e..e962624 100644 --- a/commands/code-review.md +++ b/commands/code-review.md @@ -1,6 +1,6 @@ --- allowed-tools: Task, Bash(git ls-files:+), Bash(git status:+), Bash(git log:+), Bash(git branch:+) -argument-hint: "[short|full|selector] [directory] [--include-optional] [--refresh-baseline]" +argument-hint: "[short|full|selector] [directory] [--include-optional] [--refresh-baseline] [--security]" description: Whole-codebase audit — severity-filtered + baseline-aware --- @@ -28,6 +28,9 @@ Design rationale: see `docs/three-command-review-split.md`. - Second positional → directory (default: current) - `--include-optional` → include `Nice to Have` findings (default: filtered out) - `--refresh-baseline` → write current finding set to `.code-review-baseline.yaml` and exit (no report) +- `--security` → set `SECURITY_REVIEW=1`, activating the dormant `## Security Extension (dormant)` in `docs/selector-mode-guide.md` over the reviewed scope. It is a position-independent boolean flag, independent of the mode token and the other flags (`--include-optional`, `--refresh-baseline`). When set, the security pipeline (Step 4 Security mode) runs in-session over the whole repo regardless of mode token — the existing short-mode "skip Step 5 entirely" directive applies only to the non-security adjudication. The flag is never silently ignored. + +**`--refresh-baseline` × `--security`**: when `--refresh-baseline` is set, the command writes the current finding set to `.code-review-baseline.yaml` and exits WITHOUT a report (existing behavior, unchanged) — on such an invocation no security pipeline runs, because there is no review being performed; this is not a silent ignore of the flag, it is the maintenance-mode exit that produces no findings at all. Defaults are conservative — `selector` mode + Must Fix + Should Fix only — because whole-codebase output on a mature codebase is otherwise overwhelming. @@ -86,6 +89,18 @@ RUNNER="${CLAUDE_PLUGIN_ROOT:-$HOME/.claude/plugins/marketplaces/coding}/scripts The runner is scope-agnostic — it processes whatever file list it receives. We pass the whole codebase. +#### Security mode (under `--security` only) + +This subsection runs only when `SECURITY_REVIEW` is set (Step 0). It activates the dormant `## Security Extension (dormant)` in `docs/selector-mode-guide.md` — when the signal is absent, the existing Steps 0–10 above run byte-for-byte as written. It references the frozen pipeline guide (`docs/security/security-review-pipeline.md`), the selector-mode security extension, the verifier agent (`agents/security-verifier.md`), and the polymorphic validator (`scripts/validate-citations.sh`) by name — it does NOT re-implement or re-document the procedure. In audit mode the scope is the whole repo — no diff anchoring — and the security pipeline runs in every mode (short, selector, full); it never bypasses or softens the existing Step 3 toolchain fail-fast (ast-grep/sg preflight → "Must Fix toolchain failure") or the Step 4 mechanical funnel. + +1. **Scope and recon** — audit scope is the whole tracked codebase as of HEAD, so there is no diff anchoring: every security finding is in scope regardless of the baseline diff or the severity filter. Per `docs/security/security-review-pipeline.md` (the derivation contract is frozen there), enumerate entry points over the whole codebase (recall-oriented), resolve identities and auth mechanisms, resolve resources and their `authorization_functions`, and derive invariants as `resource → identifier → authorization_function` with `file:line` evidence. Write the model to `/tmp/security-model.json` (session-local, mirroring `/tmp/code-review-findings.json`) and never to any path inside the reviewed repo — if the model is accidentally written in-tree, delete it and rewrite it to the session-local path. Apply the freshness gate (changed-evidence entries re-derived, unchanged-evidence entries carried forward; stale entries whose evidence no longer resolves are dropped and surfaced with the literal `model refresh:` line) and note whole-repo truncation on large repos in the report. Record the attack-surface inventory counts. +2. **Classifier trait groups** — per the dormant extension: exactly six groups `authz`, `input-origin`, `data-to-sink`, `external-io`, `crypto`, `secrets`. `authz` over-selection is non-negotiable: a whole-repo file cited as evidence by an entry point operating on a modeled resource, or cited as evidence by a modeled resource's `authorization_function`, MUST select `authz`. Deterministic invariant selection: any invariant whose `attack_surfaces` or `evidence` source resolves in scope forces that `invariant_id` into the applicable set — no LLM judgment, never skipped. The HARD INVARIANT holds: the applicable set is a subset of the Step 5 candidate set (the whole-codebase judgment-rule candidates this command computes in Step 5); trait groups never add a rule the glob did not produce. +3. **Adjudicator inputs** — the Step 5 selector-mode adjudication (the same 4d-sel contract) gains the whole-repo model subset, the applicable invariants with their evidence authorization functions, and the attack-surface inventory as a drift signal. Each applicable invariant is judged with the single question: does the code preserve the invariant? Invariant-kind findings cite `invariant_id`. +4. **Verifier gate** — after adjudication, before emission, run the falsification gate per `agents/security-verifier.md` (7-item falsification checklist; verdict `confirmed | plausible | rejected` with `confidence`, `exploitability`, `impact`, `attack_preconditions`, `attack_path`, `security_boundary_missing`, `counterevidence_checked`, and `reject_reason` required on rejection). It is a hard pre-emission step for `severity=critical` and for `severity=major` when `confidence=confirmed`; every surviving high-severity finding carries a populated `counterevidence_checked`. The execution mechanism (in-session role vs sub-agent spawn) is an implementation detail the session decides, preserving the gate's hard pre-emission property. Residual false kills are caught by the `ai_review` post-post backstop (dismiss + COMMENT + human_review). +5. **Blocking derived, never stored as severity** — `blocking = confidence==confirmed ∧ exploitability==high ∧ impact≥medium`; no per-surface config fields, no opt-out flags. A `plausible` critical does NOT block — it is reported as a required human review item. +6. **Toolchain/deps pass (fail-closed)** — run a dependency scan over the whole-repo dependency manifests (osv-scanner / trivy / govulncheck, whichever are available; govulncheck for Go modules), executed via the `coding:go-security-specialist` agent or in-session per the selector-mode zero-spawn property, and emit findings as `kind=toolchain` carrying `tool`, `package`, `version`, `advisory`, `file`, `line`. A scan failure, a database-fetch timeout, or a flagged vulnerability surfaces as a Must-Fix toolchain finding in the report — never a silent skip. +7. **Citation validation** — the Step 5 citation-validation invocation passes `SECURITY_MODEL_FILE=/tmp/security-model.json` so invariant-kind findings resolve against the session model's `invariants[].id`. Each finding resolves exactly one provenance (`kind=rule` → `rule_id` in `rules/index.json`; `kind=invariant` → `invariant_id` in the model; `kind=toolchain` → no id). Absent an unset/missing/unreadable/unparseable model, invariant findings drop fail-closed (WARN to stderr) and are never kept — the validator enforces this; the command only supplies the model file. + ## Step 5: Adjudication Selector mode (default) follows `docs/selector-mode-guide.md` § Step 4c-sel CLASSIFY + Step 4d-sel ADJUDICATE — identical to `/coding:pr-review`'s Step 4, with one change: @@ -175,6 +190,14 @@ Three buckets (per `/coding:pr-review` Step 5): Per `docs/selector-mode-guide.md` § Traceability Report Section. +#### Security Findings (under `--security` only) + +When `SECURITY_REVIEW` is set, list every security finding (rule, invariant, and toolchain) with `file:line`, provenance (`kind` + `rule_id`/`invariant_id`), verifier verdict fields (`confidence`, `exploitability`, `impact`, `counterevidence_checked`), and the derived blocking state. **Render each finding's verdict fields as JSON** (`"confidence": "confirmed"`, `"exploitability": "high"`, `"impact": "high"`, `"counterevidence_checked": [...]`, `"blocking": true`) — the fields must be machine-greppable exactly as the scenarios assert. Toolchain findings additionally render their `tool`, `package`, `version`, and `advisory` fields. **Render the verifier gate's outcome for every candidate that passed through it — including rejected and plausible candidates — as a JSON verdict block** (`"confidence": "rejected"` + `reject_reason`, `"confidence": "plausible"`), so the rejected-candidate and toolchain evidence the scenarios grep for is machine-greppable. This section is a **baseline-independent whole-repo inventory** — it lists every security finding regardless of the Step 6 baseline diff and the Step 7 severity filter, which continue to govern only the normal severity buckets above (`--include-optional` does not suppress a security finding); derived blocking applies to every listed security finding. Security findings are NOT classified into the baseline's NEW/CARRIED/FIXED buckets. + +#### Security Model (under `--security` only) + +When `SECURITY_REVIEW` is set, record the provenance block: `derived_from` (repo, head, review_id), the entry-point count, each attack-surface inventory count, and any `model refresh:` lines verbatim. A whole-repo scope containing Go source always produces this block (never a silent skip); a scope with no Go source states `no Go source — security model not derived` explicitly instead of omitting the section. + ## Step 10: Next steps - If `--refresh-baseline` was just set up: commit `.code-review-baseline.yaml` so subsequent runs know the starting point. diff --git a/commands/local-review.md b/commands/local-review.md index 5e4b235..6d4973f 100644 --- a/commands/local-review.md +++ b/commands/local-review.md @@ -1,6 +1,6 @@ --- allowed-tools: Task, Bash(git diff:+), Bash(git log:+), Bash(git status:+), Bash(git ls-files:+) -argument-hint: "[short|full|selector] [directory]" +argument-hint: "[short|full|selector] [directory] [--security]" description: Perform a comprehensive review of local uncommitted/recent changes --- @@ -22,6 +22,8 @@ Parse the first argument to determine mode: - If first arg is `full|comprehensive|complete` → **Full mode** (all agents, per-owner dispatch) - Otherwise (including `standard`, `selector`, `--selector`, or no token) → **Selector mode (default)** (in-session classify + adjudicate, zero sub-agent spawns) +`--security` is a position-independent boolean flag that may appear anywhere in the argument list (before or after the mode token and the directory); when present, set `SECURITY_REVIEW=1`, independent of the mode token. It is recognized as a flag and is NOT treated as the directory path by the "Any remaining arguments are treated as the directory path" rule. The flag is never silently ignored — the security pipeline runs in-session over the reviewed scope regardless of mode token, including in short mode where the Step 4b-i candidate computation the security classifier's HARD INVARIANT requires still runs; the existing short-mode "skip Step 4" directive applies only to the non-security funnel. + Any remaining arguments are treated as the directory path. ### Step 2: Project Detection @@ -156,6 +158,19 @@ On the guide's short-circuit condition the report line is `selector clean — no Include the traceability section per `docs/selector-mode-guide.md` § Traceability Report Section. +#### Security mode (under `--security` only) + +This subsection runs only when `SECURITY_REVIEW` is set (Step 1). It activates the dormant `## Security Extension (dormant)` in `docs/selector-mode-guide.md` — when the signal is absent, Steps 4c-sel/4d-sel run byte-for-byte as the existing procedure above. It references the frozen pipeline guide (`docs/security/security-review-pipeline.md`), the selector-mode security extension, the verifier agent (`agents/security-verifier.md`), and the polymorphic validator (`scripts/validate-citations.sh`) by name — it does NOT re-implement or re-document the procedure. It runs in **every** mode (short, selector, full) and never bypasses or softens the existing Step 4.0 toolchain fail-fast (ast-grep/sg preflight → "Must Fix toolchain failure") or the Step 4a mechanical funnel — a `--security` review without the mechanical funnel would silently miss every MUST-tier finding. + +1. **Recon and model derivation** — per `docs/security/security-review-pipeline.md` (the derivation contract is frozen there): the recon and the adjudicator consume the local diff `git diff HEAD~1` (or the directory diff parsed in Step 1); enumerate entry points from the diff's touched packages (recall-oriented), resolve identities and auth mechanisms, resolve resources and their `authorization_functions`, and derive invariants as `resource → identifier → authorization_function` with `file:line` evidence. Write the model to `/tmp/security-model.json` (session-local, mirroring `/tmp/local-review-findings.json`) and never to any path inside the reviewed directory — if the model is accidentally written in-tree, delete it and rewrite it to the session-local path. Apply the freshness gate (changed-evidence entries re-derived, unchanged carried forward; stale entries whose evidence no longer resolves are dropped and surfaced with the literal `model refresh:` line) and diff-relevant truncation on large repos (note the truncation in the report). Record the attack-surface inventory counts. +2. **Classifier trait groups** — per the dormant extension: exactly six groups `authz`, `input-origin`, `data-to-sink`, `external-io`, `crypto`, `secrets`. `authz` over-selection is non-negotiable: a diff touching a file cited as evidence by an entry point operating on a modeled resource, or cited as evidence by a modeled resource's `authorization_function`, MUST select `authz`. Deterministic invariant selection: a diff touching an invariant's `attack_surfaces` or its `evidence` source forces that `invariant_id` into the applicable set — no LLM judgment, never skipped. The HARD INVARIANT holds: the applicable set is a subset of the Step 4b-i candidate set; trait groups never add a rule the glob did not produce. +3. **Adjudicator inputs** — the Step 4d-sel adjudicator input gains the diff-relevant model subset, the applicable invariants with their evidence authorization functions, and the attack-surface inventory as a drift signal. Each applicable invariant is judged against the diff slice with the single question: does this change preserve the invariant? Invariant-kind findings cite `invariant_id`. +4. **Verifier gate** — after adjudication, before emission, run the falsification gate per `agents/security-verifier.md` (7-item falsification checklist; verdict `confirmed | plausible | rejected` with `confidence`, `exploitability`, `impact`, `attack_preconditions`, `attack_path`, `security_boundary_missing`, `counterevidence_checked`, and `reject_reason` required on rejection). It is a hard pre-emission step for `severity=critical` and for `severity=major` when `confidence=confirmed`; every surviving high-severity finding carries a populated `counterevidence_checked`. The execution mechanism (in-session role vs sub-agent spawn) is an implementation detail the session decides, preserving the gate's hard pre-emission property. Residual false kills are caught by the `ai_review` post-post backstop (dismiss + COMMENT + human_review). +5. **Blocking derived, never stored as severity** — `blocking = confidence==confirmed ∧ exploitability==high ∧ impact≥medium`; no per-surface config fields, no opt-out flags. A `plausible` critical does NOT block merge — it is reported as a required human review item. +6. **Diff anchoring** — in PR mode (the scope class local-review shares with pr-review), report security findings ONLY on diff-changed lines or invariants whose attack surface the diff touched; whole-file context is permitted for reasoning, never for gating. +7. **Toolchain/deps pass (fail-closed)** — run a dependency scan over the local repo's dependency manifests (osv-scanner / trivy / govulncheck, whichever are available; govulncheck for Go modules), executed via the `coding:go-security-specialist` agent or in-session per the selector-mode zero-spawn property, and emit findings as `kind=toolchain` carrying `tool`, `package`, `version`, `advisory`, `file`, `line`. A scan failure, a database-fetch timeout, or a flagged vulnerability surfaces as a Must-Fix toolchain finding in the report — never a silent skip. +8. **Citation validation** — the Step 4d-sel citation-validation invocation passes `SECURITY_MODEL_FILE=/tmp/security-model.json` so invariant-kind findings resolve against the session model's `invariants[].id`. Each finding resolves exactly one provenance (`kind=rule` → `rule_id` in `rules/index.json`; `kind=invariant` → `invariant_id` in the model; `kind=toolchain` → no id). Absent an unset/missing/unreadable/unparseable model, invariant findings drop fail-closed (WARN to stderr) and are never kept — the validator enforces this; the command only supplies the model file. + #### Full mode: per-owner dispatch **Full mode only** — skip this section in selector and short mode. @@ -238,6 +253,14 @@ Agent-reported minor issues (style, documentation, naming conventions, version u Include the traceability section per `docs/selector-mode-guide.md` § Traceability Report Section. +#### Security Findings (under `--security` only) + +When `SECURITY_REVIEW` is set, list every security finding (rule, invariant, and toolchain) with `file:line`, provenance (`kind` + `rule_id`/`invariant_id`), verifier verdict fields (`confidence`, `exploitability`, `impact`, `counterevidence_checked`), and the derived blocking state. **Render each finding's verdict fields as JSON** (`"confidence": "confirmed"`, `"exploitability": "high"`, `"impact": "high"`, `"counterevidence_checked": [...]`, `"blocking": true`) — the fields must be machine-greppable exactly as the scenarios assert. Toolchain findings additionally render their `tool`, `package`, `version`, and `advisory` fields. **Render the verifier gate's outcome for every candidate that passed through it — including rejected and plausible candidates — as a JSON verdict block** (`"confidence": "rejected"` + `reject_reason`, `"confidence": "plausible"`), so the rejected-candidate and toolchain evidence the scenarios grep for is machine-greppable. + +#### Security Model (under `--security` only) + +When `SECURITY_REVIEW` is set, record the provenance block: `derived_from` (repo, head, review_id), the entry-point count, each attack-surface inventory count, and any `model refresh:` lines verbatim. A scope containing Go source always produces this block (never a silent skip); a scope with no Go source — including when the Step 4 early-exit fires on a non-rule-relevant diff — states `no Go source — security model not derived` explicitly instead of omitting the section. + ### Step 6: Next Steps If `go-test-coverage-assistant` reported missing tests, suggest: diff --git a/commands/pr-review.md b/commands/pr-review.md index 6f01d3b..845539a 100644 --- a/commands/pr-review.md +++ b/commands/pr-review.md @@ -1,6 +1,6 @@ --- allowed-tools: Task, Bash(git diff:+), Bash(git log:+), Bash(git status:+), Bash(git ls-files:+), Bash(git fetch:+), Bash(git worktree:+), Bash(git branch:+), Bash(rm -rf:+) -argument-hint: " [short|full|selector]" +argument-hint: " [short|full|selector] [--security]" description: Review current branch diff against target branch (excludes vendor/node_modules) --- @@ -23,6 +23,7 @@ For Bitbucket PRs, use `/bitbucket-pr-review ` instead. - Second argument: mode (see Step 1) - `REPO_DIR` = current directory - `SOURCE_BRANCH` = current branch +- `--security` is a position-independent boolean flag that may appear anywhere in the argument list (before or after `TARGET_BRANCH` and the mode token); when present, set `SECURITY_REVIEW=1`. It is independent of the mode token and `TARGET_BRANCH`. #### 0a-pre: Short-circuit — skip worktree creation if already at PR head @@ -81,6 +82,7 @@ cd && git worktree remove /tmp/pr-review-- --for - `short|quick|fast` → **Short mode** (manual review only) - `full|comprehensive|complete` → **Full mode** (all agents, per-owner dispatch) - Otherwise (including `standard`, `selector`, `--selector`, or no token) → **Selector mode (default)** (in-session classify + adjudicate, zero sub-agent spawns) +- When `SECURITY_REVIEW` is set, the security-mode steps in Step 4 run in **every** mode — including short mode, which otherwise skips Step 4 (the existing short-mode "skip Step 4" directive applies only to the non-security funnel). This includes the Step 4b-i candidate computation the security classifier's HARD INVARIANT requires — the candidate set is produced and the security pipeline runs in full; the flag is never silently ignored. ### Step 2: Project Detection @@ -191,6 +193,19 @@ On the guide's short-circuit condition the report line is `selector clean — no Include the traceability section per `docs/selector-mode-guide.md` § Traceability Report Section. +#### Security mode (under `--security` only) + +This subsection runs only when `SECURITY_REVIEW` is set (Step 0a). It activates the dormant `## Security Extension (dormant)` in `docs/selector-mode-guide.md` — when the signal is absent, Steps 4c-sel/4d-sel run byte-for-byte as the existing procedure above. It references the frozen pipeline guide (`docs/security/security-review-pipeline.md`), the selector-mode security extension, the verifier agent (`agents/security-verifier.md`), and the polymorphic validator (`scripts/validate-citations.sh`) by name — it does NOT re-implement or re-document the procedure. It runs in **every** mode (short, selector, full) and never bypasses or softens the existing Step 4.0 toolchain fail-fast (ast-grep/sg preflight → "Must Fix toolchain failure") or the Step 4a mechanical funnel — a `--security` review without the mechanical funnel would silently miss every MUST-tier finding. + +1. **Recon and model derivation** — per `docs/security/security-review-pipeline.md` (the derivation contract is frozen there): enumerate entry points from the diff's touched packages (recall-oriented), resolve identities and auth mechanisms, resolve resources and their `authorization_functions`, and derive invariants as `resource → identifier → authorization_function` with `file:line` evidence. Write the model to `/tmp/security-model.json` (session-local, mirroring `/tmp/pr-review-findings.json`) and never to any path inside the reviewed repo — if the model is accidentally written in-tree, delete it and rewrite it to the session-local path. Apply the freshness gate (changed-evidence entries re-derived, unchanged carried forward; stale entries whose evidence no longer resolves are dropped and surfaced with the literal `model refresh:` line) and diff-relevant truncation on large repos (note the truncation in the report). Record the attack-surface inventory counts. +2. **Classifier trait groups** — per the dormant extension: exactly six groups `authz`, `input-origin`, `data-to-sink`, `external-io`, `crypto`, `secrets`. `authz` over-selection is non-negotiable: a diff touching a file cited as evidence by an entry point operating on a modeled resource, or cited as evidence by a modeled resource's `authorization_function`, MUST select `authz`. Deterministic invariant selection: a diff touching an invariant's `attack_surfaces` or its `evidence` source forces that `invariant_id` into the applicable set — no LLM judgment, never skipped. The HARD INVARIANT holds: the applicable set is a subset of the Step 4b-i candidate set; trait groups never add a rule the glob did not produce. +3. **Adjudicator inputs** — the Step 4d-sel adjudicator input gains the diff-relevant model subset, the applicable invariants with their evidence authorization functions, and the attack-surface inventory as a drift signal. Each applicable invariant is judged against the diff slice with the single question: does this change preserve the invariant? Invariant-kind findings cite `invariant_id`. +4. **Verifier gate** — after adjudication, before emission, run the falsification gate per `agents/security-verifier.md` (7-item falsification checklist; verdict `confirmed | plausible | rejected` with `confidence`, `exploitability`, `impact`, `attack_preconditions`, `attack_path`, `security_boundary_missing`, `counterevidence_checked`, and `reject_reason` required on rejection). It is a hard pre-emission step for `severity=critical` and for `severity=major` when `confidence=confirmed`; every surviving high-severity finding carries a populated `counterevidence_checked`. The execution mechanism (in-session role vs sub-agent spawn) is an implementation detail the session decides, preserving the gate's hard pre-emission property. Residual false kills are caught by the `ai_review` post-post backstop (dismiss + COMMENT + human_review). +5. **Blocking derived, never stored as severity** — `blocking = confidence==confirmed ∧ exploitability==high ∧ impact≥medium`; no per-surface config fields, no opt-out flags. A `plausible` critical does NOT block merge — it is reported as a required human review item. +6. **Diff anchoring** — in PR mode, report security findings ONLY on diff-changed lines or invariants whose attack surface the diff touched; whole-file context is permitted for reasoning, never for gating. +7. **Toolchain/deps pass (fail-closed)** — run a dependency scan over the reviewed repo (osv-scanner / trivy / govulncheck, whichever are available; govulncheck for Go modules), executed via the `coding:go-security-specialist` agent or in-session per the selector-mode zero-spawn property, and emit findings as `kind=toolchain` carrying `tool`, `package`, `version`, `advisory`, `file`, `line`. A scan failure, a database-fetch timeout, or a flagged vulnerability surfaces as a Must-Fix toolchain finding in the report — never a silent skip. +8. **Citation validation** — the Step 4d-sel citation-validation invocation passes `SECURITY_MODEL_FILE=/tmp/security-model.json` so invariant-kind findings resolve against the session model's `invariants[].id`. Each finding resolves exactly one provenance (`kind=rule` → `rule_id` in `rules/index.json`; `kind=invariant` → `invariant_id` in the model; `kind=toolchain` → no id). Absent an unset/missing/unreadable/unparseable model, invariant findings drop fail-closed (WARN to stderr) and are never kept — the validator enforces this; the command only supplies the model file. + #### Full mode: per-owner dispatch **Full mode only** — skip this section in selector and short mode. @@ -282,6 +297,14 @@ This applies to every severity section and to all three modes. Include the traceability section per `docs/selector-mode-guide.md` § Traceability Report Section. +#### Security Findings (under `--security` only) + +When `SECURITY_REVIEW` is set, list every security finding (rule, invariant, and toolchain) with `file:line`, provenance (`kind` + `rule_id`/`invariant_id`), verifier verdict fields (`confidence`, `exploitability`, `impact`, `counterevidence_checked`), and the derived blocking state. **Render each finding's verdict fields as JSON** (`"confidence": "confirmed"`, `"exploitability": "high"`, `"impact": "high"`, `"counterevidence_checked": [...]`, `"blocking": true`) — the fields must be machine-greppable exactly as the scenarios assert. Toolchain findings additionally render their `tool`, `package`, `version`, and `advisory` fields. **Render the verifier gate's outcome for every candidate that passed through it — including rejected and plausible candidates — as a JSON verdict block** (`"confidence": "rejected"` + `reject_reason`, `"confidence": "plausible"`), so the rejected-candidate and toolchain evidence the scenarios grep for is machine-greppable. + +#### Security Model (under `--security` only) + +When `SECURITY_REVIEW` is set, record the provenance block: `derived_from` (repo, head, review_id), the entry-point count, each attack-surface inventory count, and any `model refresh:` lines verbatim. A scope containing Go source always produces this block (never a silent skip); a scope with no Go source — including when the Step 4 early-exit fires on a non-rule-relevant diff — states `no Go source — security model not derived` explicitly instead of omitting the section. + ### Step 6: Next Steps Recommendation If test coverage gaps found, suggest `/go-write-test` commands. diff --git a/prompts/completed/042-spec-010-wire-security-into-pr-review.md b/prompts/completed/042-spec-010-wire-security-into-pr-review.md new file mode 100644 index 0000000..ccb5b9b --- /dev/null +++ b/prompts/completed/042-spec-010-wire-security-into-pr-review.md @@ -0,0 +1,132 @@ +--- +status: completed +spec: [010-security-review-command-wiring] +summary: 'Wired the --security flag into commands/pr-review.md (flag parsing, Security mode subsection referencing the frozen pipeline guide/verifier/validator, Security Findings + Security Model report sections) and added a feat: CHANGELOG entry; make precommit exits 0 and all frozen contracts are byte-unchanged' +execution_id: coding-security-pr-review-command-exec-042-spec-010-wire-security-into-pr-review +dark-factory-version: dev +created: "2026-08-23T17:15:00Z" +queued: "2026-08-23T14:49:32Z" +started: "2026-08-23T14:49:34Z" +completed: "2026-08-23T14:52:08Z" +branch: dark-factory/security-review-command-wiring +--- + +# Wire --security into commands/pr-review.md + + +- The PR review command accepts a position-independent `--security` flag; when set it activates the dormant security review mode on top of the normal selector flow +- The review session derives a session-local security model under `/tmp`, runs the six security trait groups with mandatory `authz` over-selection, judges invariants deterministically, and gates high-severity findings through the falsification verifier before they may emit +- Merge blocking is derived from the verifier verdict (never stored as severity), and security findings are anchored to the changed lines of the diff +- The report gains a `Security Findings` section and a `Security Model` provenance block while the normal severity buckets and selector traceability still appear +- The command stays a thin wrapper — it references the frozen pipeline guide, the selector-mode security extension, the verifier agent, and the polymorphic validator by name instead of re-implementing them +- The citation validator receives the session model file so invariant-kind findings resolve against it and fail closed (never silently kept) when the model is absent +- A fail-closed dependency toolchain pass surfaces scan failures or flagged vulnerabilities as Must-Fix items, never a silent skip +- The six frozen security contract files are proven byte-unchanged, and existing selector / short / full behavior is untouched when the flag is absent +- The acceptance-suite invariants on the command (short-mode skip, runner reference, selector routing and short-circuit string, step labels) all stay green + + + +Wire the `--security` flag into `commands/pr-review.md` so a PR review can run the full security pipeline — recon-derived session model, six trait groups with deterministic invariant selection, verifier-gated findings, derived blocking, PR-mode diff anchoring, a dependency toolchain pass, and the `Security Findings` + `Security Model` report sections — by referencing the already-shipped, frozen security contract (pipeline guide, selector-mode extension, verifier agent, polymorphic validator), never by re-implementing it. + + + +Read `CLAUDE.md` (repo root) — the "Command = Thin Wrapper" and "Plugin Namespacing" rules (commands parse arguments and delegate; all agent references use the `coding:` prefix; no inline rules) and the generic-content rule (User, Order, Product, Customer only). + +Read `commands/pr-review.md` (full) — the file to edit. Note the structure you must preserve: frontmatter `argument-hint: " [short|full|selector]"`, Step 0a argument parsing, Step 1 mode parsing, Step 2 project detection, Step 3 automated checks, Step 4 dispatcher (4.0 toolchain preflight → 4a mechanical funnel → 4b-i candidate computation → selector-mode 4c-sel/4d-sel block → full-mode dispatch → 4c context conventions → 4d citation validation), Step 5 consolidated report, Step 6 next steps, Step 7 manual review. + +Read `docs/selector-mode-guide.md` (full) — the dormant `## Security Extension (dormant)` section (classifier sub-step trait groups, deterministic invariant selection, adjudicator input extension, verifier gate, blocking model, polymorphic finding contract) and the Step 4d-sel citation-validation invocation. This file is FROZEN — do not edit it; the command references it. + +Read `docs/security/security-review-pipeline.md` (full) — the recon procedure, model schema, freshness gate, diff-relevant truncation, attack-surface inventory drift bridge, and report contract. FROZEN — reference, never re-document. + +Read `agents/security-verifier.md` (full) — the 7-item falsification checklist and the verdict contract (`confirmed | plausible | rejected` with `confidence`, `exploitability`, `impact`, `attack_preconditions`, `attack_path`, `security_boundary_missing`, `counterevidence_checked`, and `reject_reason` required on rejection). FROZEN. + +Read `scripts/validate-citations.sh` (full) — the polymorphic validator: `SECURITY_MODEL_FILE` env var, `kind` rule|invariant|toolchain resolution, fail-closed absent-model drop path (exit 1, `WARN: dropped` on stderr). FROZEN. + +Read `docs/security/security-review-guide.md` (skim the RULE ids) — the 5 mechanical rules whose finding ids the classifier `crypto`/`secrets` selection references (`go-security/crypto-insecure-random`, `go-security/crypto-weak-algorithm`, `go-security/hardcoded-secret`). + +Read `scripts/acceptance.sh` (the checks on `commands/pr-review.md`) — the check-acceptance assertions that must stay green after your edit: the short-mode "No agents / skip Step 4" directive, the `scripts/ast-grep-runner.sh` reference, the `Selector mode (the default)` routing, the `--selector`/`selector.*mode` token, the `selector clean — no adjudication needed` short-circuit string, `GUIDE_OK`/`GUIDE_MISSING`, the `selector-mode-guide.md` filename, the `4c-sel`/`4d-sel` labels, the Step 2.5 context-doc mappings (`teamvault-conventions.md`, `go-k8s-binary-conventions.md`, `k8s-manifest-guide.md`, `changelog-guide.md`), and the `coding:`/`findings_by_owner` dispatch block. Preserve all of them. + + + +1. **Record the frozen-contract baseline FIRST.** Before making any edit, run: + `sha256sum docs/selector-mode-guide.md scripts/validate-citations.sh agents/security-verifier.md agents/go-security-specialist.md docs/security/security-review-pipeline.md docs/security/security-review-guide.md > /tmp/df010-frozen.sha256` + Keep this file for the AC6 check in ``. If any of the six files is missing, STOP and report the missing path — the frozen contract is incomplete and must not be edited or recreated. + +2. **Flag parsing (AC2).** + a. In the frontmatter, change the `argument-hint` line to: `argument-hint: " [short|full|selector] [--security]"`. + b. In Step 0a (Parse arguments), add a bullet: `--security` is a position-independent boolean flag that may appear anywhere in the argument list (before or after `TARGET_BRANCH` and the mode token); when present, set `SECURITY_REVIEW=1`. It is independent of the mode token and `TARGET_BRANCH`. + c. In Step 1 (Parse Mode Argument), add: when `SECURITY_REVIEW` is set, the security-mode steps in Step 4 run in every mode — including short mode, which otherwise skips Step 4 (the existing short-mode "skip Step 4" directive applies only to the non-security funnel) — the flag is never silently ignored. + +3. **Insert a new subsection — `#### Security mode (under \`--security\` only)`** — positioned immediately AFTER the "Selector mode (the default): Steps 4c-sel and 4d-sel" block (which ends with the sentence "Include the traceability section per `docs/selector-mode-guide.md` § Traceability Report Section.") and immediately BEFORE the "#### Full mode: per-owner dispatch" heading. The subsection's intro states: it runs only when `SECURITY_REVIEW` is set; it activates the dormant `## Security Extension (dormant)` in `docs/selector-mode-guide.md` (steps 4c-sel/4d-sel run byte-for-byte as before when the signal is absent); it references the frozen guides/agents by name — it does NOT re-implement or re-document the procedure; and it runs in every mode (short, selector, full) without bypassing or softening the existing Step 4.0 toolchain fail-fast (ast-grep/sg preflight → "Must Fix toolchain failure") or the Step 4a mechanical funnel — a `--security` review without the mechanical funnel would silently miss every MUST-tier finding. It contains these numbered steps: + + 1. **Recon and model derivation** — per `docs/security/security-review-pipeline.md` (the derivation contract is frozen there): enumerate entry points from the diff's touched packages (recall-oriented), resolve identities and auth mechanisms, resolve resources and their `authorization_functions`, and derive invariants as `resource → identifier → authorization_function` with `file:line` evidence. Write the model to `/tmp/security-model.json` (session-local, mirroring `/tmp/pr-review-findings.json`) and never to any path inside the reviewed repo — if the model is accidentally written in-tree, delete it and rewrite it to the session-local path. Apply the freshness gate (changed-evidence entries re-derived, unchanged carried forward; stale entries whose evidence no longer resolves are dropped and surfaced with the literal `model refresh:` line) and diff-relevant truncation on large repos (note the truncation in the report). Record the attack-surface inventory counts. + 2. **Classifier trait groups** — per the dormant extension: exactly six groups `authz`, `input-origin`, `data-to-sink`, `external-io`, `crypto`, `secrets`. `authz` over-selection is non-negotiable: a diff touching a file cited as evidence by an entry point operating on a modeled resource, or cited as evidence by a modeled resource's `authorization_function`, MUST select `authz`. Deterministic invariant selection: a diff touching an invariant's `attack_surfaces` or its `evidence` source forces that `invariant_id` into the applicable set — no LLM judgment, never skipped. The HARD INVARIANT holds: the applicable set is a subset of the Step 4b-i candidate set; trait groups never add a rule the glob did not produce. + 3. **Adjudicator inputs** — the Step 4d-sel adjudicator input gains the diff-relevant model subset, the applicable invariants with their evidence authorization functions, and the attack-surface inventory as a drift signal. Each applicable invariant is judged against the diff slice with the single question: does this change preserve the invariant? Invariant-kind findings cite `invariant_id`. + 4. **Verifier gate** — after adjudication, before emission, run the falsification gate per `agents/security-verifier.md` (7-item falsification checklist; verdict `confirmed | plausible | rejected` with `confidence`, `exploitability`, `impact`, `attack_preconditions`, `attack_path`, `security_boundary_missing`, `counterevidence_checked`, and `reject_reason` required on rejection). It is a hard pre-emission step for `severity=critical` and for `severity=major` when `confidence=confirmed`; every surviving high-severity finding carries a populated `counterevidence_checked`. The execution mechanism (in-session role vs sub-agent spawn) is an implementation detail the session decides, preserving the gate's hard pre-emission property. Residual false kills are caught by the `ai_review` post-post backstop (dismiss + COMMENT + human_review). + 5. **Blocking derived, never stored as severity** — `blocking = confidence==confirmed ∧ exploitability==high ∧ impact≥medium`; no per-surface config fields, no opt-out flags. A `plausible` critical does NOT block merge — it is reported as a required human review item. + 6. **Diff anchoring** — in PR mode, report security findings ONLY on diff-changed lines or invariants whose attack surface the diff touched; whole-file context is permitted for reasoning, never for gating. + 7. **Toolchain/deps pass (fail-closed)** — run a dependency scan over the reviewed repo (osv-scanner / trivy / govulncheck, whichever are available; govulncheck for Go modules), executed via the `go-security-specialist` agent or in-session per the zero-spawn property, and emit findings as `kind=toolchain` carrying `tool`, `package`, `version`, `advisory`, `file`, `line`. A scan failure, a database-fetch timeout, or a flagged vulnerability surfaces as a Must-Fix toolchain finding in the report — never a silent skip. + 8. **Citation validation** — the Step 4d-sel citation-validation invocation passes `SECURITY_MODEL_FILE=/tmp/security-model.json` so invariant-kind findings resolve against the session model's `invariants[].id`. Each finding resolves exactly one provenance (`kind=rule` → `rule_id` in `rules/index.json`; `kind=invariant` → `invariant_id` in the model; `kind=toolchain` → no id). Absent an unset/missing/unreadable/unparseable model, invariant findings drop fail-closed (WARN to stderr) and are never kept — the validator enforces this; the command only supplies the model file. + + Each of the literal strings `security-model.json`, `/tmp/security-model.json`, `SECURITY_MODEL_FILE`, `security-verifier`, and `model refresh` must appear at least once in this subsection (AC3/AC4 evidence). + +4. **Report sections (AC4/AC5).** In Step 5 (Consolidated Report), add under the signal: a `Security Findings` section listing every security finding (rule, invariant, and toolchain) with `file:line`, provenance (`kind` + `rule_id`/`invariant_id`), verifier verdict fields, and derived blocking state; and a `Security Model` provenance block recording `derived_from` (repo, head, review_id), the entry-point count, each attack-surface inventory count, and any `model refresh:` lines verbatim. A scope containing Go source always produces the Security Model block (never a silent skip); a scope with no Go source — including when the Step 4 early-exit fires on a non-rule-relevant diff — states `no Go source — security model not derived` explicitly instead of omitting the section. The normal severity buckets and the selector traceability section still appear. The literal strings `Security Findings`, `Security Model`, and `model refresh` must each appear at least once in the command (AC4 evidence); the string `changed lines` must appear at least once in the diff-anchoring wording (AC5 evidence — the phrase "diff-changed lines" satisfies it). + +5. **Preserve existing behavior (no regression).** The existing Steps 0a–0d, 1, 2, 3, 4.0, 4a, 4b-i, the selector-mode block, the full-mode dispatch block, 4c, 4d, the existing Step 5 sections, Step 6, and Step 7 are preserved unchanged except the targeted additions in requirements 2–4. All of the `scripts/acceptance.sh` assertions on this file must still pass after your edit (listed in ``). Do NOT touch `commands/code-review.md`, `commands/local-review.md`, any `docs/*`, `scripts/*`, `agents/*`, `rules/*`, `README.md`, `llms.txt`, `scenarios/*`, or `CHANGELOG.md` (sibling prompts own them). + +6. **Dormant contract and shipped artifacts untouched (AC6).** Do not edit `docs/selector-mode-guide.md` (the dormant Security Extension and steps 4c-sel/4d-sel stay byte-identical), `scripts/validate-citations.sh`, `agents/security-verifier.md`, `agents/go-security-specialist.md`, `docs/security/security-review-pipeline.md`, or `docs/security/security-review-guide.md` — the command only references them. No `### RULE` heading anywhere in your edit. Generic examples only (User, Order, Product, Customer); never trading/project-specific content; no personal paths. + + + +- Do NOT commit — dark-factory handles git. Do NOT run `git` commands; the git-based negative guard (AC6 `git diff origin/master` / `git status --porcelain`) runs on the operator side of the spec's Verification ladder — the container form is the checksum baseline in requirement 1. +- The `## Security Extension (dormant)` in `docs/selector-mode-guide.md` is the frozen classify/adjudicate contract: steps 4c-sel/4d-sel and the extension text MUST NOT be renumbered, rewritten, or edited; the `--security` wiring only sets the security-review signal that activates it, and when the signal is absent the existing procedure runs byte-for-byte. +- Shipped artifacts not re-authored: `docs/security/security-review-pipeline.md`, `docs/security/security-review-guide.md`, `agents/security-verifier.md`, `agents/go-security-specialist.md`, and `scripts/validate-citations.sh` already exist and are NOT modified. No new security detection rules. +- Blocking: derived per `blocking = confidence==confirmed ∧ exploitability==high ∧ impact≥medium`, never stored as severity; per-surface configurable thresholds are out of scope — no config fields, no opt-out flags. +- Finding provenance: exactly one of `rule_id ∈ rules/index.json`, `invariant_id ∈ derived model`, or `kind=toolchain` (no id). No new validation-script work. +- Model location: `security-model.json` is session-local under `/tmp`, never committed to any repo; if it is ever written inside the reviewed repo tree it is deleted and rewritten to the session-local path. +- Commands are thin wrappers: no inline rules; all agent references use the `coding:` prefix; the classify/adjudicate/verifier procedure is executed per the guides, not re-implemented in the command. +- HARD INVARIANT preserved: the applicable set is a subset of the Step 4b-i candidate set; trait groups never add a rule the glob did not produce. +- Selector-mode zero-sub-agent-spawn property preserved for the classify/adjudicate steps; the verifier execution mechanism is the only deviation point and is reversible (the session decides at review time). +- The `--security` mode must not bypass or soften the existing Step 4.0 toolchain fail-fast (ast-grep/sg preflight) or the Step 4a mechanical funnel — a review without the mechanical funnel would silently miss every MUST-tier finding. +- Generic content only: fixtures and examples use User, Order, Product, Customer — never trading/project-specific content, never personal paths. +- Existing tests must still pass: `make precommit` (incl. `check-acceptance`) exits 0. + + + +Run from repo root. All commands are container-executable (no git). +```bash +# --- AC1: repo integrity --- +make precommit # must exit 0 + +# --- AC2: --security flag wired --- +grep -n -- '--security' commands/pr-review.md # >= 1 +grep -n 'argument-hint:.*--security' commands/pr-review.md # >= 1 + +# --- AC3: security pipeline steps wired --- +grep -n 'security-model.json' commands/pr-review.md # >= 1 +grep -n '/tmp/security-model.json' commands/pr-review.md # >= 1 +grep -n 'SECURITY_MODEL_FILE' commands/pr-review.md # >= 1 +grep -n 'security-verifier' commands/pr-review.md # >= 1 + +# --- AC4: report sections encoded --- +grep -n 'Security Findings' commands/pr-review.md # >= 1 +grep -n 'Security Model' commands/pr-review.md # >= 1 +grep -n 'model refresh' commands/pr-review.md # >= 1 + +# --- AC5: PR-mode diff anchoring --- +grep -n 'changed lines' commands/pr-review.md # >= 1 + +# --- AC6: frozen contract byte-unchanged (container form; the repo diff/status guard runs operator-side) --- +sha256sum -c /tmp/df010-frozen.sha256 # all files: OK + +# --- acceptance-suite invariants preserved (check-acceptance must stay green) --- +grep -n 'Selector mode (the default)' commands/pr-review.md # >= 1 +grep -n 'selector clean — no adjudication needed' commands/pr-review.md # >= 1 +grep -n '4c-sel' commands/pr-review.md # >= 1 +grep -n '4d-sel' commands/pr-review.md # >= 1 +grep -n 'scripts/ast-grep-runner.sh' commands/pr-review.md # >= 1 +grep -n 'Short Mode.*No agents' commands/pr-review.md # >= 1 +``` + + + diff --git a/prompts/completed/043-spec-010-mirror-security-into-code-review.md b/prompts/completed/043-spec-010-mirror-security-into-code-review.md new file mode 100644 index 0000000..606cc51 --- /dev/null +++ b/prompts/completed/043-spec-010-mirror-security-into-code-review.md @@ -0,0 +1,128 @@ +--- +status: completed +spec: [010-security-review-command-wiring] +summary: 'Mirrored the --security wiring from commands/pr-review.md into commands/code-review.md — frontmatter argument-hint, Step 0 flag parsing (SECURITY_REVIEW=1 + --refresh-baseline interaction), a Step 4 Security mode subsection with audit-scope whole-repo semantics (no diff anchoring, whole-repo recon/model under /tmp/security-model.json, six trait groups, verifier gate, derived blocking, fail-closed deps pass, SECURITY_MODEL_FILE citation validation), and baseline-independent Security Findings + Security Model report sections — plus a CHANGELOG.md ## Unreleased entry, with all frozen contracts verified byte-unchanged' +execution_id: coding-security-pr-review-command-exec-043-spec-010-mirror-security-into-code-review +dark-factory-version: dev +created: "2026-08-23T17:16:00Z" +queued: "2026-08-23T14:59:59Z" +started: "2026-08-23T15:00:01Z" +completed: "2026-08-23T15:01:26Z" +branch: dark-factory/security-review-command-wiring +--- + +# Mirror --security into commands/code-review.md (audit mode) + + +- The whole-codebase audit command accepts a `--security` flag that activates the same dormant security mode as the PR review command, adapted to audit scope +- In audit mode the security model is derived over the whole repo, security findings are NOT diff-anchored (whole-repo scope), and the recon, classifier, adjudicator, verifier, blocking, deps pass, and report sections mirror the canonical wiring from the PR review command +- The `Security Findings` section is a baseline-independent whole-repo inventory — it lists every security finding regardless of the baseline diff and the severity filter, which continue to govern only the normal severity buckets +- The command stays a thin wrapper — it replicates the proven security-mode section from the PR review command and references the frozen guides and agents by name +- The frozen security contract files are proven byte-unchanged, and existing selector / short / full / baseline behavior is untouched when the flag is absent +- The report gains a `Security Findings` section and a `Security Model` provenance block alongside the existing severity buckets and baseline traceability + + + +Mirror the `--security` wiring from `commands/pr-review.md` (prompt 1's deliverable) into `commands/code-review.md` so the whole-repo audit can also run the full security pipeline — whole-repo recon and model, six trait groups with deterministic invariant selection, verifier-gated findings, derived blocking, a fail-closed deps pass, and the `Security Findings` + `Security Model` report sections — with audit-scope semantics (whole repo, no diff anchoring). + + + +Read `CLAUDE.md` (repo root) — the "Command = Thin Wrapper" and "Plugin Namespacing" rules and the generic-content rule. + +Read `commands/code-review.md` (full) — the file to edit. Note the structure you must preserve: frontmatter `argument-hint: "[short|full|selector] [directory] [--include-optional] [--refresh-baseline]"`, Step 0 argument parsing, Step 1 codebase walk, Step 2 project detection, Step 3 toolchain preflight, Step 4 mechanical funnel (whole codebase, `code-review-findings.json`), Step 5 adjudication (selector / full / short), Step 6 baseline diff, Step 7 severity filter + dedup, Step 8 `--refresh-baseline`, Step 9 consolidated report, Step 10 next steps, Constraints. This command is NOT covered by `scripts/acceptance.sh`, but preserve its content anyway. + +Read `commands/pr-review.md` (the `#### Security mode (under \`--security\` only)` subsection inserted by prompt 1) — the canonical security-mode section you must replicate. It is the single source for the recon/classifier/adjudicator/verifier/blocking/deps-pass/citation-validation contract wording. + +Read `docs/selector-mode-guide.md` (the dormant `## Security Extension (dormant)` section) — FROZEN; the command references it, never edits it. + +Read `docs/security/security-review-pipeline.md` (full) — the recon procedure, model schema, freshness gate, diff-relevant truncation, attack-surface inventory drift bridge, report contract. FROZEN — reference, never re-document. + +Read `agents/security-verifier.md` (full) — the 7-item falsification checklist and verdict contract. FROZEN. + +Read `scripts/validate-citations.sh` (full) — the polymorphic validator (`SECURITY_MODEL_FILE`, `kind` resolution, fail-closed absent-model drop path). FROZEN. + + + +1. **Hard dependency guard.** Verify prompt 1 shipped the canonical wiring before editing anything: `grep -n '#### Security mode (under \`--security\` only)' commands/pr-review.md` must return a line. If it does not, STOP and report that prompt 1 must execute first — do NOT invent an alternative security-mode shape (the report-section wording and the diff-anchoring rule must be consistent across all three commands). + +2. **Record the frozen-contract baseline FIRST.** Before making any edit, run: + `sha256sum docs/selector-mode-guide.md scripts/validate-citations.sh agents/security-verifier.md agents/go-security-specialist.md docs/security/security-review-pipeline.md docs/security/security-review-guide.md > /tmp/df010-frozen.sha256` + Keep it for the AC6 check in ``. If any of the six files is missing, STOP and report the missing path. + +3. **Flag parsing (AC2).** + a. In the frontmatter, change the `argument-hint` line to: `argument-hint: "[short|full|selector] [directory] [--include-optional] [--refresh-baseline] [--security]"`. + b. In Step 0 (Parse Arguments), add `--security` to the recognized boolean flags: when present, set `SECURITY_REVIEW=1`, independent of the mode token and the other flags (`--include-optional`, `--refresh-baseline`). The flag is never silently ignored — the security pipeline runs in-session over the reviewed scope regardless of mode token; the existing short-mode "skip Step 5 entirely" directive applies only to the non-security adjudication. + c. Bind the `--security` × `--refresh-baseline` interaction: when `--refresh-baseline` is set, the command writes the current finding set to `.code-review-baseline.yaml` and exits WITHOUT a report (existing behavior, unchanged) — on such an invocation no security pipeline runs, because there is no review being performed; this is not a silent ignore of the flag, it is the maintenance-mode exit that produces no findings at all. State this in Step 0. + +4. **Insert a new subsection — `#### Security mode (under \`--security\` only)`** — positioned immediately AFTER the Step 4 mechanical-funnel block (which ends with the sentence "The runner is scope-agnostic — it processes whatever file list it receives. We pass the whole codebase.") and immediately BEFORE the `## Step 5: Adjudication` heading. Replicate the canonical security-mode section from `commands/pr-review.md` (same wording and step structure for recon/model derivation, classifier trait groups, adjudicator inputs, verifier gate, blocking, deps pass, and citation validation), with these audit-scope adaptations: + + - **Scope and diff anchoring** — in audit mode the scope is the whole repo — no diff anchoring — and the recon derives a whole-repo model (AC5: the string `whole` must appear at least once; the literal phrase "the scope is the whole repo — no diff anchoring" is required). Do NOT include the PR-mode "diff-changed lines" gating rule from prompt 1; whole-repo findings are in scope regardless of the diff. + - **Recon** — enumerate entry points and resolve identities / auth mechanisms / resources / invariants over the whole codebase (not the diff's touched packages), per `docs/security/security-review-pipeline.md`. + - **Freshness gate** — unchanged-evidence entries carried forward, changed-evidence entries re-derived, stale entries dropped and surfaced with the literal `model refresh:` line; large-repo truncation noted in the report. + - **Adjudicator inputs** — the Step 5 selector-mode adjudication (the same 4d-sel contract) gains the whole-repo model subset, the applicable invariants with their evidence authorization functions, and the attack-surface inventory as a drift signal. + - **Model path** — still `/tmp/security-model.json`, session-local, never inside the reviewed repo (mirror of `/tmp/code-review-findings.json`). + - **Deps pass** — run over the whole-repo dependency manifests; fail-closed (scan failure, DB-fetch timeout, or flagged vulnerability → Must-Fix toolchain finding, never a silent skip). + - **Citation validation** — the Step 5 citation-validation invocation passes `SECURITY_MODEL_FILE=/tmp/security-model.json`; invariant findings fail closed on an absent model. + + Each of the literal strings `security-model.json`, `/tmp/security-model.json`, `SECURITY_MODEL_FILE`, `security-verifier`, and `model refresh` must appear at least once in this subsection (AC3/AC4 evidence). + +5. **Report sections (AC4).** In Step 9 (Consolidated Report), add under the signal: a `Security Findings` section listing every security finding (rule, invariant, and toolchain) with `file:line`, provenance (`kind` + `rule_id`/`invariant_id`), verifier verdict fields, and derived blocking state; and a `Security Model` provenance block recording `derived_from` (repo, head, review_id), the entry-point count, each attack-surface inventory count, and any `model refresh:` lines verbatim. A whole-repo scope containing Go source always produces the Security Model block (never a silent skip); a scope with no Go source states `no Go source — security model not derived` explicitly. The normal severity buckets, the baseline traceability section, and the selector traceability section still appear. The literal strings `Security Findings`, `Security Model`, and `model refresh` must each appear at least once in the command (AC4 evidence). + + **Security Findings is a baseline-independent whole-repo inventory per the spec's report contract** — it lists every security finding regardless of the Step 6 baseline diff and Step 7 severity filter (the baseline and severity filter continue to govern only the normal severity buckets; `--include-optional` does not suppress a security finding); derived blocking applies to every listed security finding. Do not classify security findings into the baseline's NEW/CARRIED/FIXED buckets. + +6. **Preserve existing behavior (no regression).** The existing Steps 0–10 and the Constraints section are preserved unchanged except the targeted additions in requirements 3–5 (the `--refresh-baseline` mode, the baseline diff, the severity filter + dedup, and the selector/full/short adjudication all keep working; the security mode runs in addition to them, never replacing them). Do NOT touch `commands/pr-review.md`, `commands/local-review.md`, any `docs/*`, `scripts/*`, `agents/*`, `rules/*`, `README.md`, `llms.txt`, `scenarios/*`, or `CHANGELOG.md` (sibling prompts own them). + +7. **Dormant contract and shipped artifacts untouched (AC6).** Do not edit `docs/selector-mode-guide.md`, `scripts/validate-citations.sh`, `agents/security-verifier.md`, `agents/go-security-specialist.md`, `docs/security/security-review-pipeline.md`, or `docs/security/security-review-guide.md` — the command only references them. No `### RULE` heading anywhere in your edit. Generic examples only (User, Order, Product, Customer); never trading/project-specific content; no personal paths. + + + +- Do NOT commit — dark-factory handles git. Do NOT run `git` commands; the git-based negative guard (AC6 `git diff origin/master` / `git status --porcelain`) runs on the operator side of the spec's Verification ladder — the container form is the checksum baseline in requirement 2. +- The `## Security Extension (dormant)` in `docs/selector-mode-guide.md` is the frozen classify/adjudicate contract: steps 4c-sel/4d-sel and the extension text MUST NOT be renumbered, rewritten, or edited; the `--security` wiring only sets the security-review signal that activates it, and when the signal is absent the existing procedure runs byte-for-byte. +- Shipped artifacts not re-authored: `docs/security/security-review-pipeline.md`, `docs/security/security-review-guide.md`, `agents/security-verifier.md`, `agents/go-security-specialist.md`, and `scripts/validate-citations.sh` already exist and are NOT modified. No new security detection rules. +- Blocking: derived per `blocking = confidence==confirmed ∧ exploitability==high ∧ impact≥medium`, never stored as severity; per-surface configurable thresholds are out of scope — no config fields, no opt-out flags. +- Finding provenance: exactly one of `rule_id ∈ rules/index.json`, `invariant_id ∈ derived model`, or `kind=toolchain` (no id). No new validation-script work. +- Model location: `security-model.json` is session-local under `/tmp`, never committed to any repo; if it is ever written inside the reviewed repo tree it is deleted and rewritten to the session-local path. +- Commands are thin wrappers: no inline rules; all agent references use the `coding:` prefix; the classify/adjudicate/verifier procedure is executed per the guides, not re-implemented in the command. +- HARD INVARIANT preserved: the applicable set is a subset of the Step 4b-i candidate set; trait groups never add a rule the glob did not produce. +- Selector-mode zero-sub-agent-spawn property preserved for the classify/adjudicate steps; the verifier execution mechanism is the only deviation point and is reversible (the session decides at review time). +- The `--security` mode must not bypass or soften the existing Step 3 toolchain preflight (ast-grep/sg fail-fast) or the Step 4 mechanical funnel. +- `--refresh-baseline` wins over `--security`: a baseline-refresh invocation writes the baseline and exits without a review, so no security pipeline runs on it (maintenance write, not a silent flag ignore). +- Audit scope is the whole tracked codebase as of HEAD (`git ls-files`), vendored/generated files always excluded — unchanged from the existing command constraints. +- Generic content only: fixtures and examples use User, Order, Product, Customer — never trading/project-specific content, never personal paths. +- Existing tests must still pass: `make precommit` (incl. `check-acceptance`) exits 0. + + + +Run from repo root. All commands are container-executable (no git). +```bash +# --- AC1: repo integrity --- +make precommit # must exit 0 + +# --- AC2: --security flag wired --- +grep -n -- '--security' commands/code-review.md # >= 1 +grep -n 'argument-hint:.*--security' commands/code-review.md # >= 1 + +# --- AC3: security pipeline steps wired --- +grep -n 'security-model.json' commands/code-review.md # >= 1 +grep -n '/tmp/security-model.json' commands/code-review.md # >= 1 +grep -n 'SECURITY_MODEL_FILE' commands/code-review.md # >= 1 +grep -n 'security-verifier' commands/code-review.md # >= 1 + +# --- AC4: report sections encoded --- +grep -n 'Security Findings' commands/code-review.md # >= 1 +grep -n 'Security Model' commands/code-review.md # >= 1 +grep -n 'model refresh' commands/code-review.md # >= 1 + +# --- AC5: audit-mode whole-repo scope (no diff anchoring) --- +grep -n 'whole' commands/code-review.md # >= 1 +grep -n 'no diff anchoring' commands/code-review.md # >= 1 (audit scope literal) + +# --- AC6: frozen contract byte-unchanged (container form; the repo diff/status guard runs operator-side) --- +sha256sum -c /tmp/df010-frozen.sha256 # all files: OK + +# --- no PR-mode diff-anchoring wording in the audit command --- +grep -n 'changed lines' commands/code-review.md # must return 0 lines +``` + + + diff --git a/prompts/completed/044-spec-010-mirror-security-into-local-review.md b/prompts/completed/044-spec-010-mirror-security-into-local-review.md new file mode 100644 index 0000000..665c388 --- /dev/null +++ b/prompts/completed/044-spec-010-mirror-security-into-local-review.md @@ -0,0 +1,128 @@ +--- +status: completed +spec: [010-security-review-command-wiring] +summary: 'Mirrored the --security wiring from commands/pr-review.md into commands/local-review.md: position-independent SECURITY_REVIEW=1 flag parsing, a Security mode subsection (recon over git diff HEAD~1, /tmp/security-model.json, six trait groups, verifier gate, derived blocking, diff-changed-lines anchoring, fail-closed deps pass, SECURITY_MODEL_FILE citation validation), and Security Findings + Security Model report sections, with all AC1-AC6 greps, frozen-file checksums, and the acceptance suite green (make precommit exit 0)' +execution_id: coding-security-pr-review-command-exec-044-spec-010-mirror-security-into-local-review +dark-factory-version: dev +created: "2026-08-23T17:17:00Z" +queued: "2026-08-23T15:13:28Z" +started: "2026-08-23T15:14:01Z" +completed: "2026-08-23T15:16:11Z" +branch: dark-factory/security-review-command-wiring +--- + +# Mirror --security into commands/local-review.md (local mode) + + +- The local uncommitted/recent-changes review command accepts a `--security` flag that activates the same dormant security mode as the PR review command, with PR-mode diff anchoring over `HEAD~1` +- The recon, six trait groups with deterministic invariant selection, verifier gate, derived blocking, fail-closed deps pass, citation validation, and `Security Findings` + `Security Model` report sections mirror the canonical wiring from the PR review command +- The command stays a thin wrapper — it replicates the proven security-mode section from `commands/pr-review.md` and references the frozen guides and agents by name +- The frozen security contract files are proven byte-unchanged, and existing selector / short / full behavior is untouched when the flag is absent +- The acceptance-suite invariants on the command (short-mode skip, runner reference, selector routing and short-circuit string, step labels, context-doc mappings) all stay green + + + +Mirror the `--security` wiring from `commands/pr-review.md` (prompt 1's deliverable) into `commands/local-review.md` so the local uncommitted/recent-changes review can run the full security pipeline — recon-derived session model over the local diff, six trait groups with deterministic invariant selection, verifier-gated findings, derived blocking, a fail-closed deps pass, and the `Security Findings` + `Security Model` report sections — with PR-mode diff anchoring over the `HEAD~1` diff. + + + +Read `CLAUDE.md` (repo root) — the "Command = Thin Wrapper" and "Plugin Namespacing" rules and the generic-content rule. + +Read `commands/local-review.md` (full) — the file to edit. Note the structure you must preserve: frontmatter `argument-hint: "[short|full|selector] [directory]"`, Step 1 argument parsing, Step 2 project detection, Step 3 automated checks, Step 4 dispatcher (4.0 toolchain preflight → 4a mechanical funnel → 4b-i candidate computation → selector-mode 4c-sel/4d-sel block → full-mode dispatch → 4c context conventions → 4d citation validation), Step 5 consolidated report, Step 6 next steps, Step 7 manual review. The DIFF for this command is `git diff HEAD~1` (or the directory diff parsed in Step 1). + +Read `commands/pr-review.md` (the `#### Security mode (under \`--security\` only)` subsection inserted by prompt 1) — the canonical security-mode section you must replicate. It is the single source for the recon/classifier/adjudicator/verifier/blocking/deps-pass/citation-validation contract wording. + +Read `docs/selector-mode-guide.md` (the dormant `## Security Extension (dormant)` section) — FROZEN; the command references it, never edits it. + +Read `docs/security/security-review-pipeline.md` (full) — the recon procedure, model schema, freshness gate, diff-relevant truncation, attack-surface inventory drift bridge, report contract. FROZEN — reference, never re-document. + +Read `agents/security-verifier.md` (full) — the 7-item falsification checklist and verdict contract. FROZEN. + +Read `scripts/validate-citations.sh` (full) — the polymorphic validator (`SECURITY_MODEL_FILE`, `kind` resolution, fail-closed absent-model drop path). FROZEN. + +Read `scripts/acceptance.sh` (the checks on `commands/local-review.md`) — the check-acceptance assertions that must stay green after your edit: the short-mode "No agents / skip Step 4" directive, the `scripts/ast-grep-runner.sh` reference, at least 4 of the legacy-path conditional agents (`license-assistant`, `readme-quality-assistant`, `shellcheck-assistant`, `context7-library-checker`, `go-version-manager`, `go-tooling-assistant`), the `Selector mode (the default)` routing, the `--selector`/`selector.*mode` token, the `selector clean — no adjudication needed` short-circuit string, `GUIDE_OK`/`GUIDE_MISSING`, the `selector-mode-guide.md` filename, the `4c-sel`/`4d-sel` labels, the Step 2.5 context-doc mappings (`teamvault-conventions.md`, `go-k8s-binary-conventions.md`, `k8s-manifest-guide.md`, `changelog-guide.md`), and the `coding:`/`findings_by_owner` dispatch block. Preserve all of them. + + + +1. **Hard dependency guard.** Verify prompt 1 shipped the canonical wiring before editing anything: `grep -n '#### Security mode (under \`--security\` only)' commands/pr-review.md` must return a line. If it does not, STOP and report that prompt 1 must execute first — do NOT invent an alternative security-mode shape (the report-section wording and the diff-anchoring rule must be consistent across all three commands). + +2. **Record the frozen-contract baseline FIRST.** Before making any edit, run: + `sha256sum docs/selector-mode-guide.md scripts/validate-citations.sh agents/security-verifier.md agents/go-security-specialist.md docs/security/security-review-pipeline.md docs/security/security-review-guide.md > /tmp/df010-frozen.sha256` + Keep it for the AC6 check in ``. If any of the six files is missing, STOP and report the missing path. + +3. **Flag parsing (AC2).** + a. In the frontmatter, change the `argument-hint` line to: `argument-hint: "[short|full|selector] [directory] [--security]"`. + b. In Step 1 (Parse Arguments), add: `--security` is a position-independent boolean flag that may appear anywhere in the argument list (before or after the mode token and the directory); when present, set `SECURITY_REVIEW=1`, independent of the mode token. It is recognized as a flag and is NOT treated as the directory path by the "Any remaining arguments are treated as the directory path" rule. The flag is never silently ignored — the security pipeline runs in-session over the reviewed scope regardless of mode token; the existing short-mode "skip Step 4" directive applies only to the non-security funnel. + +4. **Insert a new subsection — `#### Security mode (under \`--security\` only)`** — positioned immediately AFTER the "Selector mode (the default): Steps 4c-sel and 4d-sel" block (which ends with the sentence "Include the traceability section per `docs/selector-mode-guide.md` § Traceability Report Section.") and immediately BEFORE the "#### Full mode: per-owner dispatch" heading. Replicate the canonical security-mode section from `commands/pr-review.md` (same wording and step structure for recon/model derivation, classifier trait groups, adjudicator inputs, verifier gate, blocking, deps pass, and citation validation), with these local-mode specifics: + + - **Diff source** — the recon and the adjudicator consume the local diff `git diff HEAD~1` (or the directory diff parsed in Step 1); the session model mirrors `/tmp/local-review-findings.json`. + - **Diff anchoring** — in local mode (PR mode), report security findings ONLY on diff-changed lines or invariants whose attack surface the diff touched; whole-file context is permitted for reasoning, never for gating (AC5: the string `changed lines` must appear at least once — the phrase "diff-changed lines" satisfies it). + - **Freshness gate** — unchanged-evidence entries carried forward, changed-evidence entries re-derived, stale entries dropped and surfaced with the literal `model refresh:` line; large-repo truncation noted in the report. + - **Model path** — `/tmp/security-model.json`, session-local, never inside the reviewed directory. + - **Deps pass** — run over the local repo's dependency manifests; fail-closed (scan failure, DB-fetch timeout, or flagged vulnerability → Must-Fix toolchain finding, never a silent skip). + - **Citation validation** — the Step 4d-sel citation-validation invocation passes `SECURITY_MODEL_FILE=/tmp/security-model.json`; invariant findings fail closed on an absent model, and the review continues with rule/toolchain findings (never a hard abort). + + Each of the literal strings `security-model.json`, `/tmp/security-model.json`, `SECURITY_MODEL_FILE`, `security-verifier`, and `model refresh` must appear at least once in this subsection (AC3/AC4 evidence). + +5. **Report sections (AC4/AC5).** In Step 5 (Consolidated Report), add under the signal: a `Security Findings` section listing every security finding (rule, invariant, and toolchain) with `file:line`, provenance (`kind` + `rule_id`/`invariant_id`), verifier verdict fields, and derived blocking state; and a `Security Model` provenance block recording `derived_from` (repo, head, review_id), the entry-point count, each attack-surface inventory count, and any `model refresh:` lines verbatim. A scope containing Go source always produces the Security Model block (never a silent skip); a scope with no Go source — including when the Step 4 early-exit fires on a non-rule-relevant diff — states `no Go source — security model not derived` explicitly instead of omitting the section. The normal severity buckets and the selector traceability section still appear. The literal strings `Security Findings`, `Security Model`, and `model refresh` must each appear at least once in the command (AC4 evidence). + +6. **Preserve existing behavior (no regression).** The existing Steps 1–7, 4.0, 4a, 4b-i, the selector-mode block, the full-mode dispatch block, 4c, 4d, and the existing Step 5 sections are preserved unchanged except the targeted additions in requirements 3–5. All of the `scripts/acceptance.sh` assertions on this file must still pass after your edit (listed in ``). Do NOT touch `commands/pr-review.md`, `commands/code-review.md`, any `docs/*`, `scripts/*`, `agents/*`, `rules/*`, `README.md`, `llms.txt`, `scenarios/*`, or `CHANGELOG.md` (sibling prompts own them). + +7. **Dormant contract and shipped artifacts untouched (AC6).** Do not edit `docs/selector-mode-guide.md`, `scripts/validate-citations.sh`, `agents/security-verifier.md`, `agents/go-security-specialist.md`, `docs/security/security-review-pipeline.md`, or `docs/security/security-review-guide.md` — the command only references them. No `### RULE` heading anywhere in your edit. Generic examples only (User, Order, Product, Customer); never trading/project-specific content; no personal paths. + + + +- Do NOT commit — dark-factory handles git. Do NOT run `git` commands; the git-based negative guard (AC6 `git diff origin/master` / `git status --porcelain`) runs on the operator side of the spec's Verification ladder — the container form is the checksum baseline in requirement 2. +- The `## Security Extension (dormant)` in `docs/selector-mode-guide.md` is the frozen classify/adjudicate contract: steps 4c-sel/4d-sel and the extension text MUST NOT be renumbered, rewritten, or edited; the `--security` wiring only sets the security-review signal that activates it, and when the signal is absent the existing procedure runs byte-for-byte. +- Shipped artifacts not re-authored: `docs/security/security-review-pipeline.md`, `docs/security/security-review-guide.md`, `agents/security-verifier.md`, `agents/go-security-specialist.md`, and `scripts/validate-citations.sh` already exist and are NOT modified. No new security detection rules. +- Blocking: derived per `blocking = confidence==confirmed ∧ exploitability==high ∧ impact≥medium`, never stored as severity; per-surface configurable thresholds are out of scope — no config fields, no opt-out flags. +- Finding provenance: exactly one of `rule_id ∈ rules/index.json`, `invariant_id ∈ derived model`, or `kind=toolchain` (no id). No new validation-script work. +- Model location: `security-model.json` is session-local under `/tmp`, never committed to any repo; if it is ever written inside the reviewed repo tree it is deleted and rewritten to the session-local path. +- Commands are thin wrappers: no inline rules; all agent references use the `coding:` prefix; the classify/adjudicate/verifier procedure is executed per the guides, not re-implemented in the command. +- HARD INVARIANT preserved: the applicable set is a subset of the Step 4b-i candidate set; trait groups never add a rule the glob did not produce. +- Selector-mode zero-sub-agent-spawn property preserved for the classify/adjudicate steps; the verifier execution mechanism is the only deviation point and is reversible (the session decides at review time). +- The `--security` mode must not bypass or soften the existing Step 4.0 toolchain preflight (ast-grep/sg fail-fast) or the Step 4a mechanical funnel. +- Local scope is the uncommitted / `HEAD~1` diff — unchanged from the existing command constraints. +- Generic content only: fixtures and examples use User, Order, Product, Customer — never trading/project-specific content, never personal paths. +- Existing tests must still pass: `make precommit` (incl. `check-acceptance`) exits 0. + + + +Run from repo root. All commands are container-executable (no git). +```bash +# --- AC1: repo integrity --- +make precommit # must exit 0 + +# --- AC2: --security flag wired --- +grep -n -- '--security' commands/local-review.md # >= 1 +grep -n 'argument-hint:.*--security' commands/local-review.md # >= 1 + +# --- AC3: security pipeline steps wired --- +grep -n 'security-model.json' commands/local-review.md # >= 1 +grep -n '/tmp/security-model.json' commands/local-review.md # >= 1 +grep -n 'SECURITY_MODEL_FILE' commands/local-review.md # >= 1 +grep -n 'security-verifier' commands/local-review.md # >= 1 + +# --- AC4: report sections encoded --- +grep -n 'Security Findings' commands/local-review.md # >= 1 +grep -n 'Security Model' commands/local-review.md # >= 1 +grep -n 'model refresh' commands/local-review.md # >= 1 + +# --- AC5: PR-mode diff anchoring --- +grep -n 'changed lines' commands/local-review.md # >= 1 + +# --- AC6: frozen contract byte-unchanged (container form; the repo diff/status guard runs operator-side) --- +sha256sum -c /tmp/df010-frozen.sha256 # all files: OK + +# --- acceptance-suite invariants preserved (check-acceptance must stay green) --- +grep -n 'Selector mode (the default)' commands/local-review.md # >= 1 +grep -n 'selector clean — no adjudication needed' commands/local-review.md # >= 1 +grep -n '4c-sel' commands/local-review.md # >= 1 +grep -n '4d-sel' commands/local-review.md # >= 1 +grep -n 'scripts/ast-grep-runner.sh' commands/local-review.md # >= 1 +grep -n 'teamvault-conventions.md' commands/local-review.md # >= 1 +``` + + + diff --git a/prompts/completed/045-spec-010-author-security-acceptance-scenarios.md b/prompts/completed/045-spec-010-author-security-acceptance-scenarios.md new file mode 100644 index 0000000..e2a8d82 --- /dev/null +++ b/prompts/completed/045-spec-010-author-security-acceptance-scenarios.md @@ -0,0 +1,128 @@ +--- +status: completed +spec: [010-security-review-command-wiring] +summary: 'Finalized scenarios 007/008/009 (replaced TBD fixture blocks with inline-built generic Go fixtures + exact /coding:local-review --security invocation, zero TBD) and authored new 010 security-zero-findings; all four stay status: draft with the five-section format, observable assertions, security-model.json provenance guards, and closing walk-status lines; CHANGELOG ## Unreleased extended.' +execution_id: coding-security-pr-review-command-exec-045-spec-010-author-security-acceptance-scenarios +dark-factory-version: dev +created: "2026-08-23T17:18:00Z" +queued: "2026-08-23T16:00:10Z" +started: "2026-08-23T16:00:11Z" +completed: "2026-08-23T16:03:00Z" +branch: dark-factory/security-review-command-wiring +--- + +# Author the four security-mode acceptance scenarios (007/008/009 finalize, 010 new) + + +- The three draft security scenarios (007 IDOR-confirmed, 008 IDOR-rejected-by-verifier, 009 toolchain-fail-closed) are finalized: their `TBD — task 4` fixture blocks are replaced with inline-built generic fixture instructions and the exact `--security` invocation, so they become walkable end-to-end acceptance walks +- A fourth scenario (010 security-zero-findings) is authored for the clean-app walk that proves a `--security` review on a clean generic Go app reports zero findings with the Security Model provenance block and no merge blocking +- Each scenario builds its fixture inline in Setup (minimal generic Go app scaffolded and committed in a temp dir) per the 002 precedent — no new perpetual fixture PR required +- All four scenarios stay `status: draft`, keep the five-section format (Test PR / Setup / Action / Expected / Cleanup), carry only observable assertions, and each contains the literal `--security` flag +- Scenario 009 preserves its container-executable validator half (toolchain findings pass, invariant findings fail closed without a model) and adds the dependency-flag walk half +- The session-local security model is asserted to never land inside the fixture repo in every walk + + + +Produce the four mandated acceptance scenarios under `scenarios/` — finalize the existing drafts 007 (IDOR-confirmed), 008 (IDOR-rejected-by-verifier), 009 (toolchain-fail-closed) by replacing their `TBD — task 4` fixture blocks with inline-built generic fixtures plus the exact `--security` invocation, and author the new 010 (security-zero-findings) — so each is a walkable end-to-end checklist with `status: draft`, observable assertions only, and the exact evidence strings the spec's operator rung greps for. + + + +Read `CLAUDE.md` (repo root) — the generic-content rule (User, Order, Product, Customer only; no trading terms, no personal paths). + +Read the four scenario files in full: `scenarios/007-security-idor-confirmed.md`, `scenarios/008-security-idor-rejected-by-verifier.md`, `scenarios/009-security-toolchain-fail-closed.md` (the drafts to finalize) and `scenarios/002-clean-pr-zero-findings.md` (the inline-fixture precedent: `WORK=$(mktemp -d) && cd "$WORK" && git clone ... . && git checkout -b ` then apply an edit, commit, and diff — Setup / Action / Expected / Cleanup checkbox format). Also skim `scenarios/005-selector-clean-short-circuit.md` for the walk format (`tee stdout to /tmp/scenNNN-stdout.log, stderr to /tmp/scenNNN-stderr.log, capture exit code to /tmp/scenNNN-exit`). Note the 002/005 precedent that the walk runs in-place on a local-only branch — no origin remote, so `/coding:pr-review`'s worktree flow does not apply. + +Read the scenario format guide at the in-container path `/home/node/.claude/plugins/marketplaces/dark-factory/docs/rules/scenario-writing.md` — frontmatter `status: draft`, H1 `# Scenario NNN: `, a one-sentence "Validates that ..." description, checkbox sections, observable outcomes only, self-contained, one journey per file. + +Read `docs/security/security-review-pipeline.md` (the model schema — `entry_points`, `resources[].authorization_functions`, `invariants[]` with `id`/`statement`/`evidence`/`attack_surfaces`; the session-local `/tmp/security-model.json` lifecycle) and `docs/security/security-review-guide.md` (the rule ids) — the concrete fixture details the scenarios cite. + +Read `agents/security-verifier.md` (the verdict contract — `confirmed | plausible | rejected`, `counterevidence_checked`, `reject_reason`) and `scripts/validate-citations.sh` (the `SECURITY_MODEL_FILE` env var and the fail-closed `WARN: dropped` stderr line) — the exact strings the scenarios' Expected sections grep for. + +Read the wired commands to confirm the surface the scenarios walk: `commands/pr-review.md` and `commands/local-review.md` (the `#### Security mode (under \`--security\` only)` subsection shipped by prompts 1 and 3) — the exact `--security` invocation and the Security Findings / Security Model report-section names. + + + +1. **Hard dependency guard.** Verify all three command wirings shipped before editing the scenarios: `grep -l -- '--security' commands/pr-review.md commands/code-review.md commands/local-review.md | wc -l` must return `3`. If any command lacks the flag, STOP and report that prompts 1–3 must execute first — the walks exercise the wired commands and will fail without them. + +2. **Shared format for all four scenario files.** Each file has frontmatter `status: draft`; an H1 `# Scenario NNN: `; a one-sentence "Validates that ..." description; EXACTLY these five `## ` sections in this order: `## Test PR`, `## Setup`, `## Action`, `## Expected`, `## Cleanup`; and a closing walk-status line. Each `## Test PR` section describes the INLINE-BUILT generic fixture (no perpetual fixture PR, no TBD). Each `## Setup` / `## Action` / `## Expected` block is a list of `- [ ]` checkboxes with observable outcomes only (files on disk, git state, command output, exit codes, grep counts — never internal reasoning). Each file must contain at least 8 unchecked checkboxes total across Setup/Action/Expected/Cleanup, and at least one literal `--security` token in the Action block. Generic content only (User, Order, Product, Customer); no trading/project-specific content; no personal paths; no `~/`-only paths in fixture commands (use `$WORK` under `mktemp -d`). No `TBD` string anywhere in the 007/008/009 files (the fixture-PR TBD blocks are replaced). + +3. **Scenario 007 — IDOR confirmed (finalize `scenarios/007-security-idor-confirmed.md`).** + - `## Test PR`: replace the TBD block — the fixture is built inline: a minimal generic Go order app whose handler `pkg/handler/order.go` exposes `GET /orders/{order_id}` and resolves the order by ID with NO ownership authorization check (the IDOR bypass), while `pkg/authz/order.go` defines the `RequireOrderAccess` authorization function the handler should call but does not; the derived model registers invariant `INV-1` ("a member may only read orders they own") with `evidence: pkg/authz/order.go`. + - `## Setup`: build the fixture inline. Use a two-commit shape so the review has a diff: on `master` commit the app WITH the ownership check called in the handler; on a feature branch remove the `RequireOrderAccess` call in `pkg/handler/order.go` and commit — the diff touches `pkg/handler/order.go` (the invariant's attack surface), forcing `INV-1` into the applicable set (deterministic invariant selection). Scaffold with `WORK=$(mktemp -d) && cd "$WORK" && go mod init example.com/order-app && mkdir -p pkg/handler pkg/authz && git init -q && git config user.email fixture@example.com && git config user.name fixture`. Include the Setup precondition that the session model carries `resources[order].authorization_functions` and `INV-1`, and that `SECURITY_MODEL_FILE` points at the session model so `scripts/validate-citations.sh` resolves `INV-1` (exit 0, kept). + - `## Action`: run `/coding:local-review --security` in a fresh Claude Code session against `$WORK` (in-place — the fixture is a local-only branch with no origin remote; pr-review's worktree flow requires an origin, per the 005 precedent; plugin pinned to the branch under test); tee stdout to `/tmp/scen007-stdout.log`, stderr to `/tmp/scen007-stderr.log`, capture exit code to `/tmp/scen007-exit`. Confirm the verifier gate runs post-adjudication, pre-emission on the severity=critical candidate. With the two-commit shape, `HEAD~1` (local-review's diff) is the commit that removed the check, so the diff-relevant assertions hold. + - `## Expected` (exact grep strings, matching AC10): `cat /tmp/scen007-exit` prints `0`; `grep -c '"confidence": "confirmed"' /tmp/scen007-stdout.log` ≥ 1; `grep -c 'attack_path' /tmp/scen007-stdout.log` ≥ 1; `grep -c 'counterevidence_checked' /tmp/scen007-stdout.log` ≥ 1; `grep -c '"exploitability": "high"' /tmp/scen007-stdout.log` ≥ 1 AND `grep -c '"impact": "high"' /tmp/scen007-stdout.log` ≥ 1; `grep -c '"blocking": true' /tmp/scen007-stdout.log` ≥ 1 (blocking formula holds); the finding survives at `severity=critical` in the final report; no `security-model.json` exists anywhere inside `$WORK` (`find "$WORK" -name security-model.json` returns nothing — the session model was never committed to the fixture repo); run completes in under 10 minutes (wall clock `real` < 10m). + - `## Cleanup`: `rm -rf "$WORK" /tmp/scen007-*`. + +4. **Scenario 008 — IDOR rejected by verifier (finalize `scenarios/008-security-idor-rejected-by-verifier.md`).** + - `## Test PR`: replace the TBD block — the fixture is built inline: a minimal generic Go order app whose handler `pkg/handler/order.go` exposes `GET /orders/{order_id}` and IS guarded by a service-layer ownership check `RequireOrderAccess(orderID, userID)` defined in `pkg/authz/order.go`; the derived model's `resources[order].authorization_functions` lists the check with its `file:line` evidence and registers invariant `INV-1`. + - `## Setup`: build the fixture inline (same scaffold shape as 007). Two-commit shape: on `master` commit the guarded app; on a feature branch touch `pkg/handler/order.go` (e.g. a cosmetic change that still exercises the handler path) and commit — the diff touches the invariant's attack surface so `INV-1` lands in the applicable set, and a candidate invariant-kind IDOR finding is raised during adjudication (`kind=invariant`, `invariant_id=INV-1`, `severity=major`, claiming `GET /orders/{order_id}` lacks an ownership check). Include the Setup precondition that `SECURITY_MODEL_FILE` points at the session model and the model's `resources[order].authorization_functions` lists the service-layer ownership check. + - `## Action`: run `/coding:local-review --security` in a fresh Claude Code session against `$WORK` (in-place — the fixture is a local-only branch with no origin remote; pr-review's worktree flow requires an origin, per the 005 precedent; plugin pinned to the branch under test); tee stdout to `/tmp/scen008-stdout.log`, stderr to `/tmp/scen008-stderr.log`, capture exit code to `/tmp/scen008-exit`. Confirm the verifier gate runs on the candidate (security signal set, severity=major). + - `## Expected` (exact grep strings, matching AC11): `cat /tmp/scen008-exit` prints `0`; `grep -c '"confidence": "rejected"' /tmp/scen008-stdout.log` ≥ 1; `grep -c 'reject_reason' /tmp/scen008-stdout.log` ≥ 1; the rejected finding does NOT emit — no `INV-1` entry appears in the final report's findings (absence assertion: `grep -c 'INV-1' /tmp/scen008-stdout.log` inside the findings region returns 0, or the report's findings list contains no INV-1 finding); `grep -c '"blocking": true' /tmp/scen008-stdout.log` returns 0 (a `rejected` verdict never satisfies the blocking formula); no `security-model.json` exists anywhere inside `$WORK`; run completes in under 10 minutes. + - `## Cleanup`: `rm -rf "$WORK" /tmp/scen008-*`. + +5. **Scenario 009 — toolchain fail-closed (finalize `scenarios/009-security-toolchain-fail-closed.md`).** + - The validator half (already container-executable) stays: build a toolchain findings JSON `[{"kind": "toolchain", "tool": "osv-scanner", "package": "golang.org/x/crypto", "version": "v0.17.0", "advisory": "GHSA-45x7-px36-r8r7", "file": "go.mod", "line": 5}]` and an invariant findings JSON `[{"kind": "invariant", "invariant_id": "INV-1", "file": "pkg/authz/order.go", "line": 33}]`; run the validator over each with and without `SECURITY_MODEL_FILE`; assert toolchain passes (exit 0, kept, `jq '.findings | length'` = 1, no `rule_id` required) and the invariant run WITHOUT a model drops fail-closed (exit 1, `WARN: dropped` on stderr naming `INV-1`, kept set empty). + - `## Test PR`: replace the TBD block — the walk half fixture is built inline: a minimal generic Go app whose `go.mod` requires the vulnerable `golang.org/x/crypto v0.17.0` (imported, e.g. `golang.org/x/crypto/bcrypt`, so govulncheck/osv-scanner flag it) plus a minimal `main.go`; the derived model registers invariant `INV-1`. + - `## Setup`: include both the validator JSON fixtures (from the existing draft) and the inline-built vulnerable-app fixture (scaffold shape as in requirement 3; on `master` commit the app with the vulnerable dependency). + - `## Action`: run the two validator invocations exactly as the existing draft does (`bash scripts/validate-citations.sh toolchain-findings.json` and `bash scripts/validate-citations.sh invariant-findings.json` with `SECURITY_MODEL_FILE` UNSET for the invariant run), capturing exit codes and stdout/stderr to `/tmp/scen009-*`; AND run `/coding:local-review --security` in a fresh Claude Code session against the vulnerable-app fixture `$WORK` (plugin pinned to the branch under test), teeing stdout to `/tmp/scen009-stdout.log`, stderr to `/tmp/scen009-stderr.log`, exit code to `/tmp/scen009-exit`. + - `## Expected` (exact grep strings, matching AC12): the validator half — `cat /tmp/scen009-toolchain-exit` prints `0`, `jq '.findings | length' /tmp/scen009-toolchain-out.json` prints `1`; `cat /tmp/scen009-invariant-exit` prints `1`; `grep -c 'WARN: dropped' /tmp/scen009-invariant-err` ≥ 1 AND `grep -c 'INV-1' /tmp/scen009-invariant-err` ≥ 1; `jq '.findings | length' /tmp/scen009-invariant-out.json` prints `0`. The walk half — `cat /tmp/scen009-exit` prints `0`; the toolchain finding surfaces as a Must-Fix item in the report, never a silent skip: `grep -c 'GHSA-45x7-px36-r8r7' /tmp/scen009-stdout.log` ≥ 1 AND `grep -c 'golang.org/x/crypto' /tmp/scen009-stdout.log` ≥ 1; no `security-model.json` exists anywhere inside `$WORK`; run completes in under 10 minutes. + - `## Cleanup`: `rm -rf "$WORK" /tmp/scen009-*`. + +6. **Scenario 010 — security-zero-findings (author new `scenarios/010-security-zero-findings.md`).** + - `## Test PR`: the fixture is built inline — a clean minimal generic Go HTTP app with a properly guarded order resource (ownership check called in the handler), no weak crypto, no hardcoded secrets, no string-interpolated SQL — a diff with no security-relevant violations. + - `## Setup`: scaffold as in requirement 3; on `master` commit the clean app; on a feature branch make a benign Go change that introduces nothing security-relevant (e.g. add a pure helper function or a doc comment) and commit, so the review has a Go diff with no security findings. + - `## Action`: run `/coding:local-review --security` in a fresh Claude Code session against `$WORK` (in-place — the fixture is a local-only branch with no origin remote; pr-review's worktree flow requires an origin, per the 005 precedent; plugin pinned to the branch under test); tee stdout to `/tmp/scen010-stdout.log`, stderr to `/tmp/scen010-stderr.log`, capture exit code to `/tmp/scen010-exit`. + - `## Expected` (exact grep strings, matching AC9): `cat /tmp/scen010-exit` prints `0`; `grep -c 'Security Findings' /tmp/scen010-stdout.log` ≥ 1 (the section is present with zero rule/invariant findings — no finding blocks); `grep -c 'Security Model' /tmp/scen010-stdout.log` ≥ 1 (the provenance block is present — `derived_from`, entry-point count, inventory counts); `grep -c '"blocking": true' /tmp/scen010-stdout.log` returns 0; no `security-model.json` exists anywhere inside `$WORK`; run completes in under 10 minutes. + - `## Cleanup`: `rm -rf "$WORK" /tmp/scen010-*`. + - The H1 title follows the other security scenarios, e.g. `# Scenario 010: Security mode on a clean generic Go app reports zero findings with a Security Model provenance block`. + +7. **Closing walk-status lines.** Each of the four files ends with a status line reflecting that they are now walkable, e.g. `**Status**: draft — walkable after the \`--security\` command wiring (spec 010) merges; not promoted to active.` (no `TBD`, no `active`). Do NOT promote any scenario to `active` — they are walked on the operator rung after merge. + +8. **Do NOT touch** (beyond the four scenario files): any `commands/*.md`, any `docs/*`, `scripts/*`, `agents/*`, `rules/*`, `README.md`, `llms.txt`, `CHANGELOG.md`, or any other `scenarios/*.md`. Do NOT add the new scenario files to the README or llms scenario tables (prompt 5 owns the README rows). + + + +- Do NOT commit — dark-factory handles git. Do NOT run `git` commands inside this prompt's container verification; the `git clone`/`git init`/`git commit` commands the scenarios' Setup sections describe are instructions for the HOST-side walker (operator rung), not commands this prompt executes. +- Scenarios live in `scenarios/` (next after 006); the existing drafts 007/008/009 are finalized in place (same filenames), 010 is new. Fixtures are built inline in each scenario's Setup (scaffold a minimal generic Go app + apply the violating code, per the 002 precedent) — no new perpetual fixture PR required. +- Each scenario stays `status: draft` — none is promoted to `active` (walkable only after the `--security` wiring merges and is walked on the host). +- Scenario 007/008/009 must contain ZERO `TBD` (the `TBD — task 4` fixture blocks are replaced). Each of the four files must contain ≥1 literal `--security` token and ≥8 unchecked checkboxes. +- The session-local security model (`security-model.json`) is never committed to any repo — every walk's Expected block asserts no `security-model.json` inside the fixture `$WORK`. +- Generic content only: fixtures and examples use User, Order, Product, Customer — never trading/project-specific content, never personal paths (`~/`/`/Users/bborbe/`), and fixture commands use `$WORK` under `mktemp -d`. +- Observable assertions only — no internal struct/function checks, no references to unreleased internals. +- The scenario WALKS (AC9–AC12) are operator-executable on the host after merge — they do NOT run inside this prompt's container. This prompt only authors the four files. +- Existing tests must still pass: `make precommit` (incl. `check-links`, which validates README/llms links only — the scenario files are not linked until prompt 5 adds the README rows) exits 0. + + + +Run from repo root. All commands are container-executable (no git). +```bash +# --- AC7: scenario files present and finalized --- +ls scenarios/010-security-zero-findings.md # must succeed +grep -c 'status: draft' scenarios/010-security-zero-findings.md # >= 1 + +# No TBD anywhere in the three finalized drafts (count must print 0 per file) +grep -c 'TBD' scenarios/007-security-idor-confirmed.md scenarios/008-security-idor-rejected-by-verifier.md scenarios/009-security-toolchain-fail-closed.md +! grep -q 'TBD' scenarios/007-security-idor-confirmed.md scenarios/008-security-idor-rejected-by-verifier.md scenarios/009-security-toolchain-fail-closed.md + +# Per-file: --security present, >=8 unchecked checkboxes, the five required sections +for f in 007-security-idor-confirmed 008-security-idor-rejected-by-verifier 009-security-toolchain-fail-closed 010-security-zero-findings; do + echo "== $f ==" + grep -c -- '--security' "scenarios/$f.md" # >= 1 per file + grep -c '^- \[ \]' "scenarios/$f.md" # >= 8 per file + grep -c '^## Test PR' "scenarios/$f.md" # == 1 per file + grep -c '^## Setup' "scenarios/$f.md" # == 1 per file + grep -c '^## Action' "scenarios/$f.md" # == 1 per file + grep -c '^## Expected' "scenarios/$f.md" # == 1 per file + grep -c '^## Cleanup' "scenarios/$f.md" # == 1 per file +done + +# Every walk asserts the session model never lands in the fixture repo +for f in 007-security-idor-confirmed 008-security-idor-rejected-by-verifier 009-security-toolchain-fail-closed 010-security-zero-findings; do + grep -c 'security-model.json' "scenarios/$f.md" # >= 1 per file +done + +# --- AC1: repo integrity --- +make precommit # must exit 0 +``` + + + diff --git a/prompts/completed/046-spec-010-repo-surface-integration.md b/prompts/completed/046-spec-010-repo-surface-integration.md new file mode 100644 index 0000000..7d45ebc --- /dev/null +++ b/prompts/completed/046-spec-010-repo-surface-integration.md @@ -0,0 +1,134 @@ +--- +status: completed +spec: [010-security-review-command-wiring] +summary: 'Wired the --security mode into the repo surface: README Commands table + Quick Start + Acceptance Scenarios rows 007-010, verified llms.txt and agent-table alignment (no new guide/agent), and extended the existing ## Unreleased with a feat: changelog bullet above v0.48.0 — version strings untouched, make precommit and make check-versions both exit 0.' +execution_id: coding-security-pr-review-command-exec-046-spec-010-repo-surface-integration +dark-factory-version: dev +created: "2026-08-23T17:19:00Z" +queued: "2026-08-23T16:13:28Z" +started: "2026-08-23T16:14:07Z" +completed: "2026-08-23T16:15:09Z" +branch: dark-factory/security-review-command-wiring +--- + +# Repo-surface integration: README, llms.txt alignment, agent tables, changelog + + +- The README commands table and Quick Start document the new `--security` flag on all three review commands, so operators discover the mode from the plugin's front page +- The README Acceptance Scenarios table gains the four security scenario rows (007–010), linking to the scenario files landed by prompt 4 +- The llms.txt index is verified to still reference both the security pipeline guide and the verifier agent — no new llms entry is required because the mode adds no new guide or agent +- The agent tables are verified unchanged: `security-verifier` stays registered, and no new agent file appears under the agents directory +- A changelog entry records the `--security` wiring under `## Unreleased`, placed above whichever version is currently the topmost released section +- The four plugin version strings stay untouched, and the full precommit (link + JSON + index + rule-test gates) stays green + + + +Finish the spec's repo-surface integration: update `README.md` (commands table, Quick Start, Acceptance Scenarios rows 007–010), verify `llms.txt` and the agent tables stay aligned (the mode adds no new guide or agent), and add the `## Unreleased` changelog entry — with the plugin version strings untouched and `make precommit` green. + + + +Read `CLAUDE.md` (repo root) — the "Adding a new guide"/"Adding a new agent" checklists (this prompt adds neither), the "When Changing Files" cross-reference rules for README/llms/CHANGELOG, the generic-content rule, and the "Dark Factory Workflow" note that this repo's releases are manual (autoRelease false; the four version strings are touched only at release, never by prompts). + +Read `README.md` (full) — the `## Quick Start` block (~lines 30–63), the `## Commands` table (~lines 65–86, rows for `/coding:pr-review`, `/coding:local-review`, `/coding:code-review`), the `## Agents` section with the `
Other agents` table (which already contains the `security-verifier` row at ~line 273), and the `## Acceptance Scenarios` table (~lines 277–287, currently rows 001–004 only — rows 005/006 exist on disk but are intentionally not listed; do NOT backfill them, per the spec Non-goal). + +Read `llms.txt` (full) — the `## Go — Testing & Quality` section currently has the Security Review Guide bullet (~line 28), the Security Review Pipeline bullet (~line 29), and the `security-verifier` bullet (~line 30); the `## Acceptance Scenarios` section lists 001–004 only. The mode adds no new guide or agent, so no new llms entry is required — this is an alignment VERIFICATION, not an addition. + +Read `CHANGELOG.md` (head only) — the topmost versioned section is resolved at execution time (do NOT hardcode it: it moves with every release). There is currently no `## Unreleased` section. Insert `## Unreleased` directly above the topmost `## vX.Y.Z` line, after the frozen preamble header block. + +Verify the deliverable files exist before wiring the README links: `scenarios/007-security-idor-confirmed.md`, `scenarios/008-security-idor-rejected-by-verifier.md`, `scenarios/009-security-toolchain-fail-closed.md`, `scenarios/010-security-zero-findings.md` (prompt 4's deliverables — `check-links` fails on README links to missing files). + +Read `docs/changelog-guide.md` — the `## Unreleased` entry format (conventional prefix required; one bullet per logical change; the `## Unreleased` section goes directly above the highest `## vX.Y.Z`; never insert above or inside the frozen preamble). + + + +1. **Record baselines FIRST.** Before making any edit, run: + - `ls agents | sort > /tmp/df010-agents-baseline.txt` (the agent-file inventory — AC8: no new agent file) + - `sha256sum agents/security-verifier.md agents/go-security-specialist.md > /tmp/df010-agents-frozen.sha256` (the two security agents stay byte-unchanged) + - `sha256sum docs/selector-mode-guide.md scripts/validate-citations.sh docs/security/security-review-pipeline.md docs/security/security-review-guide.md > /tmp/df010-frozen.sha256` (the remaining frozen contract paths stay byte-unchanged — AC6/AC8 container backstop; the git diff/status guards run operator-side) + Keep all three for the AC8 checks in ``. + +2. **README commands table (AC8).** In the `## Commands` table, add the `--security` flag to the three review-command rows so the flag is discoverable: + - `/coding:pr-review` row: append `— add \`--security\` to run the security review pipeline` (or equivalent wording naming `--security`). + - `/coding:local-review [short\|selector\|full]` row: append `; add \`--security\` for the security review pipeline`. + - `/coding:code-review [...]` row: append `; add \`--security\` for the security review pipeline`. + Match the existing row formatting exactly (same column shape, no trailing whitespace). At least one of these rows (or the Quick Start addition in requirement 3) must contain the literal `--security` token. + +3. **README Quick Start (AC8).** In the `## Quick Start` block, add one example line showing the mode, e.g.: + ``` + /coding:pr-review --security # security review mode: derived model + six trait groups + verifier gate + Security Findings report + ``` + The line must contain the literal `--security` token. + +4. **README Acceptance Scenarios table (AC8).** Add four rows to the `## Acceptance Scenarios` table, after the existing 004 row, following the exact existing row shape (`| NNN | [name](scenarios/NNN-name.md) | |`): + - 007 — `[security-idor-confirmed](scenarios/007-security-idor-confirmed.md)` — `\`--security\` review of an order app with a seeded ownership-check bypass: verifier confirms the invariant IDOR finding (counterevidence_checked populated) and blocking holds` + - 008 — `[security-idor-rejected-by-verifier](scenarios/008-security-idor-rejected-by-verifier.md)` — `\`--security\` review of an order app guarded by a service-layer ownership check: verifier rejects the naive IDOR claim (reject_reason recorded), no finding, no blocking` + - 009 — `[security-toolchain-fail-closed](scenarios/009-security-toolchain-fail-closed.md)` — `toolchain findings pass citation validation; invariant findings fail closed without a model; a \`--security\` deps pass surfaces a vulnerable dependency as Must-Fix, never a silent skip` + - 010 — `[security-zero-findings](scenarios/010-security-zero-findings.md)` — `\`--security\` review of a clean generic Go app: Security Findings section with zero findings + Security Model provenance block, no blocking` + The row for 010 must make the link target `scenarios/010-security-zero-findings.md` resolvable (check-links). Do NOT backfill rows for 005/006 (spec Non-goal: pre-existing staleness). + +5. **llms.txt alignment check (AC8).** Verify (do not add): `grep -c 'security-review-pipeline' llms.txt` ≥ 1 AND `grep -c 'security-verifier' llms.txt` ≥ 1. Both are expected to already be present (the Security Review Pipeline and `security-verifier` bullets in the `## Go — Testing & Quality` section). If either is missing, restore the corresponding bullet from the current `docs/`/`agents/` content. The mode adds no new guide and no new agent, so no new llms entry is added. Do NOT edit the llms `## Acceptance Scenarios` section (prompt 4's scenarios are not listed there, matching the 005/006 precedent). + +6. **Agent-table verification (AC8).** Verify (do not add): `grep -n 'security-verifier' README.md` ≥ 1 (the `Other agents` table row stays registered — it is NOT added to any Go Quality dispatch table and no command dispatch list references it). No new agent file appears under `agents/` (diff `/tmp/df010-agents-baseline.txt` against the current `ls agents` output — must be empty). The two security agent files stay byte-unchanged (`sha256sum -c /tmp/df010-agents-frozen.sha256`). + +7. **CHANGELOG (repo convention).** Resolve the topmost released version at execution time — never hardcode it: + ```bash + grep -n -m1 '^## v' CHANGELOG.md # the first released section, whatever it is + grep -n -m1 '^## Unreleased' CHANGELOG.md ; echo "unreleased-exit=$?" + ``` + If `## Unreleased` already exists (verify first; do not expect it), append the bullet to it — never create a second section. If it does not, insert `## Unreleased` immediately above the line the first `grep` reported (directly above the highest `## vX.Y.Z`, below the frozen preamble — never above or inside the preamble). Add exactly one `feat:` bullet (per `docs/changelog-guide.md`), naming the `--security` wiring, e.g.: + ``` + ## Unreleased + + - feat: Wire the --security flag into commands/pr-review.md, commands/code-review.md, and commands/local-review.md — activating the dormant security review mode (session-derived /tmp/security-model.json, six trait groups with non-negotiable authz over-selection and deterministic invariant selection, verifier-gated emission, derived blocking, diff anchoring in PR mode / whole-repo scope in audit mode, fail-closed dependency toolchain pass, Security Findings + Security Model report sections); register the mode in README.md and finalize acceptance scenarios 007-010 + ``` + Do NOT touch the four version strings (the top `## vX.Y.Z` entry, `.claude-plugin/plugin.json` `version`, both `.claude-plugin/marketplace.json` fields). + +8. **Do NOT touch** (beyond the files named above): any `commands/*.md` (wired by prompts 1–3), any `docs/*` (frozen contract + guides), `scripts/*`, `agents/*` (verification only, no edits), `rules/*`, `specs/*`, `scenarios/*` (landed by prompt 4), `CLAUDE.md`. No `### RULE` heading anywhere in your edit. Generic content only; no personal paths. + + + +- Do NOT commit — dark-factory handles git. Do NOT run `git` commands; the git-based negative guards (AC8 `git diff origin/master --stat -- agents/` and `git status --porcelain -- agents/`) run on the operator side of the spec's Verification ladder — the container forms are the baseline diff and checksum checks in requirements 1 and 6. +- Repo surfaces only: README.md, llms.txt (verification), CHANGELOG.md. No command, guide, agent, script, rule, or scenario edits. +- Acceptance Scenarios table rows: 007–010 only; do NOT backfill 005/006 (spec Non-goal — pre-existing staleness, not part of this mode). +- No new agent and no new guide: the `security-verifier` row stays in the README `Other agents` table only; llms.txt gains no new entry. +- CHANGELOG: `## Unreleased` entry with a `feat:` prefix, placed directly above the highest `## vX.Y.Z` resolved at execution time (create the section if absent); the four version strings are NOT touched (releases are manual, handled by maintainer-agent-releaser). +- Generic content only (User, Order, Product, Customer); never trading/project-specific content; no personal paths. No version-existence claims. +- Existing tests must still pass: `make precommit` (incl. `check-links`, which validates every README/llms link against an existing file) exits 0. + + + +Run from repo root. All commands are container-executable (no git). +```bash +# --- AC1: repo integrity --- +make precommit # must exit 0 (check-links validates the new scenario links against existing files) + +# --- AC8: --security documented in README (commands table and/or Quick Start) --- +grep -n -- '--security' README.md # >= 1 + +# --- AC8: Acceptance Scenarios table gains the 4 security rows --- +grep -n '010-security-zero-findings' README.md # >= 1 +grep -n '007-security-idor-confirmed' README.md # >= 1 +grep -n '008-security-idor-rejected-by-verifier' README.md # >= 1 +grep -n '009-security-toolchain-fail-closed' README.md # >= 1 + +# --- AC8: llms.txt stays aligned (no new entry required; both already present) --- +grep -c 'security-review-pipeline' llms.txt # >= 1 +grep -c 'security-verifier' llms.txt # >= 1 + +# --- AC8: agent tables unchanged, no new agent file --- +grep -n 'security-verifier' README.md # >= 1 (Other agents table) +ls agents | sort > /tmp/df010-agents-after.txt +diff -q /tmp/df010-agents-baseline.txt /tmp/df010-agents-after.txt # must be identical (no new agent) +sha256sum -c /tmp/df010-agents-frozen.sha256 # both agents OK +sha256sum -c /tmp/df010-frozen.sha256 # frozen contract paths unchanged + +# --- Changelog --- +grep -n -m1 '^## Unreleased' CHANGELOG.md # must return a line +grep -n -- '--security' CHANGELOG.md # >= 1 + +# --- version strings untouched (check-versions is container-safe: pure file reads, no git) --- +make check-versions # must exit 0 +``` + + + diff --git a/scenarios/007-security-idor-confirmed.md b/scenarios/007-security-idor-confirmed.md index bc173b7..cc86a45 100644 --- a/scenarios/007-security-idor-confirmed.md +++ b/scenarios/007-security-idor-confirmed.md @@ -8,18 +8,21 @@ Validates that in security mode, the verifier gate confirms an invariant-kind ID ## Test PR -TBD — task 4 names the security-mode fixture PR; expected shape: a public fixture repo whose `order` resource handler (e.g. `pkg/handler/order.go` exposing `GET /orders/{order_id}`) resolves the order by ID with no ownership authorization check, and whose derived security model registers the invariant `INV-1` ("a member may only read orders they own") with evidence in `pkg/authz/order.go`. +The fixture is built inline in Setup (no perpetual fixture PR): a minimal generic Go order app scaffolded under `$WORK` whose handler `pkg/handler/order.go` exposes `GET /orders/{order_id}` and resolves the order by ID with NO ownership authorization check (the IDOR bypass), while `pkg/authz/order.go` defines the `RequireOrderAccess` authorization function the handler should call but does not. The derived session security model registers invariant `INV-1` ("a member may only read orders they own") with `evidence: pkg/authz/order.go`. The review's diff (feature branch vs `HEAD~1`) is the commit that removed the ownership check from `pkg/handler/order.go`, so the invariant's attack surface is touched. ## Setup -- [ ] Derive the session security model per `docs/security/security-review-pipeline.md` — the model carries `resources[order].authorization_functions` and the invariant `INV-1` with `evidence: pkg/authz/order.go:33` -- [ ] Diff touches `pkg/handler/order.go` — the invariant's attack surface — so `INV-1` lands in the applicable set (deterministic invariant selection) +- [ ] Scaffold the fixture repo: `WORK=$(mktemp -d) && cd "$WORK" && go mod init example.com/order-app && mkdir -p pkg/handler pkg/authz && git init -q && git config user.email fixture@example.com && git config user.name fixture` +- [ ] Write the guarded app on `master`: `pkg/handler/order.go` exposes `GET /orders/{order_id}` and calls `RequireOrderAccess` before resolving the order; `pkg/authz/order.go` defines the ownership authorization function; commit `git add -A && git commit -qm 'order app with ownership check'` +- [ ] Create the feature branch and remove the `RequireOrderAccess` call in `pkg/handler/order.go` so the handler resolves orders by ID unguarded: `git checkout -b idor-removed && && git add pkg/handler/order.go && git commit -qm 'remove ownership check'` +- [ ] The review's diff touches `pkg/handler/order.go` — the invariant's attack surface — so `INV-1` lands in the applicable set (deterministic invariant selection), and `HEAD~1` (local-review's diff scope) is the commit that removed the check +- [ ] Derive the session security model per `docs/security/security-review-pipeline.md` — the model carries `resources[order].authorization_functions` and the invariant `INV-1` with `evidence: pkg/authz/order.go` - [ ] Adjudication emits an invariant-kind finding: `kind=invariant`, `invariant_id=INV-1`, `severity=critical`, `file=pkg/handler/order.go`, citing `GET /orders/{order_id}` as the sink -- [ ] `SECURITY_MODEL_FILE` points at the session model so `scripts/validate-citations.sh` resolves `INV-1` (exit 0, kept) +- [ ] `SECURITY_MODEL_FILE` points at the session model (`/tmp/security-model.json`) so `scripts/validate-citations.sh` resolves `INV-1` (exit 0, kept) ## Action -- [ ] Run the security-mode review over the fixture repo in a fresh Claude Code session; tee stdout to `/tmp/scen007-stdout.log`, stderr to `/tmp/scen007-stderr.log`, capture exit code to `/tmp/scen007-exit` +- [ ] Run the security-mode review over `$WORK` in a fresh Claude Code session (in-place — the fixture is a local-only branch with no origin remote, which `/coding:pr-review`'s worktree flow does not support; plugin pinned to the branch under test): `/coding:local-review --security`; tee stdout to `/tmp/scen007-stdout.log`, stderr to `/tmp/scen007-stderr.log`, capture exit code to `/tmp/scen007-exit` - [ ] Confirm the verifier gate runs post-adjudication, pre-emission on the severity=critical candidate (per the `docs/selector-mode-guide.md` Verifier gate section) ## Expected @@ -27,9 +30,11 @@ TBD — task 4 names the security-mode fixture PR; expected shape: a public fixt - [ ] `cat /tmp/scen007-exit` prints `0` - [ ] The finding's verifier verdict is `confirmed`: `grep -c '"confidence": "confirmed"' /tmp/scen007-stdout.log` ≥ 1 - [ ] The verdict carries a concrete step-by-step attacker scenario in `attack_path`: `grep -c 'attack_path' /tmp/scen007-stdout.log` ≥ 1 -- [ ] `counterevidence_checked` is populated with the counterevidence actually checked and why it does not hold (no route middleware guard on the sink; the ownership check covers another path only): `grep -c 'counterevidence_checked' /tmp/scen007-stdout.log` ≥ 1 +- [ ] `counterevidence_checked` is populated with the counterevidence actually checked and why it does not hold (no route middleware guard on the sink; the ownership check is no longer called from this handler): `grep -c 'counterevidence_checked' /tmp/scen007-stdout.log` ≥ 1 - [ ] The finding survives at `severity=critical` in the final report - [ ] The blocking model blocks merge — `confidence==confirmed ∧ exploitability==high ∧ impact≥medium` holds: `grep -c '"exploitability": "high"' /tmp/scen007-stdout.log` ≥ 1 and `grep -c '"impact": "high"' /tmp/scen007-stdout.log` ≥ 1 +- [ ] The derived blocking state is `blocking=true` for the finding: `grep -c '"blocking": true' /tmp/scen007-stdout.log` ≥ 1 +- [ ] The session security model never lands in the fixture repo — `find "$WORK" -name security-model.json` returns nothing - [ ] Run completes in under 10 minutes (wall clock `real` < 10m) ## Cleanup @@ -38,4 +43,4 @@ TBD — task 4 names the security-mode fixture PR; expected shape: a public fixt --- -**Status**: draft — not walked; walkable after task 4 wires the security-review signal. Not promoted to active. +**Status**: draft — walkable after the `--security` command wiring (spec 010) merges; not promoted to active. diff --git a/scenarios/008-security-idor-rejected-by-verifier.md b/scenarios/008-security-idor-rejected-by-verifier.md index 53fd33e..8e68a49 100644 --- a/scenarios/008-security-idor-rejected-by-verifier.md +++ b/scenarios/008-security-idor-rejected-by-verifier.md @@ -8,17 +8,19 @@ Validates that in security mode, the verifier gate rejects an invariant-kind IDO ## Test PR -TBD — task 4 names the security-mode fixture PR; expected shape: a public fixture repo whose `order` resource handler (e.g. `pkg/handler/order.go` exposing `GET /orders/{order_id}`) looks like it exposes cross-member data but is actually guarded by a service-layer ownership authorization check (e.g. `RequireOrderAccess(orderID, userID)` in `pkg/authz/order.go`), and whose derived security model registers the invariant `INV-1`. +The fixture is built inline in Setup (no perpetual fixture PR): a minimal generic Go order app scaffolded under `$WORK` whose handler `pkg/handler/order.go` exposes `GET /orders/{order_id}` and IS guarded by a service-layer ownership check `RequireOrderAccess(orderID, userID)` defined in `pkg/authz/order.go`. The derived security model's `resources[order].authorization_functions` lists the check with its `file:line` evidence and registers invariant `INV-1`. The review's diff (feature branch vs `HEAD~1`) touches the guarded handler but introduces no authorization gap. ## Setup -- [ ] Diff touches `pkg/handler/order.go`, so the model's invariant `INV-1` lands in the applicable set (deterministic invariant selection) -- [ ] A candidate IDOR finding is raised: `kind=invariant`, `invariant_id=INV-1`, `severity=major`, `file=pkg/handler/order.go`, claiming `GET /orders/{order_id}` lacks an ownership check +- [ ] Scaffold the fixture repo: `WORK=$(mktemp -d) && cd "$WORK" && go mod init example.com/order-app && mkdir -p pkg/handler pkg/authz && git init -q && git config user.email fixture@example.com && git config user.name fixture` +- [ ] Write the guarded app on `master`: `pkg/handler/order.go` exposes `GET /orders/{order_id}` and calls `RequireOrderAccess(orderID, userID)` before resolving; `pkg/authz/order.go` defines the service-layer ownership check; commit `git add -A && git commit -qm 'guarded order app'` +- [ ] Create the feature branch and touch `pkg/handler/order.go` (a cosmetic change that still exercises the handler path): `git checkout -b cosmetic-change && && git add pkg/handler/order.go && git commit -qm 'cosmetic handler change'` +- [ ] The diff touches `pkg/handler/order.go` — the invariant's attack surface — so `INV-1` lands in the applicable set (deterministic invariant selection), and a candidate IDOR finding is raised during adjudication: `kind=invariant`, `invariant_id=INV-1`, `severity=major`, `file=pkg/handler/order.go`, claiming `GET /orders/{order_id}` lacks an ownership check - [ ] `SECURITY_MODEL_FILE` points at the session model; the model's `resources[order].authorization_functions` lists the service-layer ownership check with its `file:line` evidence ## Action -- [ ] Run the security-mode review over the fixture repo in a fresh Claude Code session; tee stdout to `/tmp/scen008-stdout.log`, stderr to `/tmp/scen008-stderr.log`, capture exit code to `/tmp/scen008-exit` +- [ ] Run the security-mode review over `$WORK` in a fresh Claude Code session (in-place — the fixture is a local-only branch with no origin remote, which `/coding:pr-review`'s worktree flow does not support; plugin pinned to the branch under test): `/coding:local-review --security`; tee stdout to `/tmp/scen008-stdout.log`, stderr to `/tmp/scen008-stderr.log`, capture exit code to `/tmp/scen008-exit` - [ ] Confirm the verifier gate runs on the candidate (security signal set, severity=major) ## Expected @@ -26,8 +28,9 @@ TBD — task 4 names the security-mode fixture PR; expected shape: a public fixt - [ ] `cat /tmp/scen008-exit` prints `0` - [ ] The candidate's verifier verdict is `rejected`: `grep -c '"confidence": "rejected"' /tmp/scen008-stdout.log` ≥ 1 - [ ] `reject_reason` is recorded naming the checklist item or counterevidence that killed the finding — item 3 (authorization-absence search) found the service-layer ownership check: `grep -c 'reject_reason' /tmp/scen008-stdout.log` ≥ 1 -- [ ] The rejected finding does NOT emit — no `INV-1` entry appears in the final report's findings -- [ ] No merge block is produced for this finding — the blocking formula `confidence==confirmed ∧ exploitability==high ∧ impact≥medium` is not satisfied by a `rejected` verdict +- [ ] The rejected finding does NOT emit — the final report's findings list contains no `INV-1` finding: `awk '/^#### Security Findings/{flag=1; next} /^#### /{flag=0} flag' /tmp/scen008-stdout.log | grep -c 'INV-1'` prints `0` (or the report's findings list contains no `INV-1` entry) +- [ ] No merge block is produced — a `rejected` verdict never satisfies `confidence==confirmed ∧ exploitability==high ∧ impact≥medium`: `grep -c '"blocking": true' /tmp/scen008-stdout.log` returns `0` +- [ ] The session security model never lands in the fixture repo — `find "$WORK" -name security-model.json` returns nothing - [ ] Run completes in under 10 minutes (wall clock `real` < 10m) ## Cleanup @@ -36,4 +39,4 @@ TBD — task 4 names the security-mode fixture PR; expected shape: a public fixt --- -**Status**: draft — not walked; walkable after task 4 wires the security-review signal. Not promoted to active. +**Status**: draft — walkable after the `--security` command wiring (spec 010) merges; not promoted to active. diff --git a/scenarios/009-security-toolchain-fail-closed.md b/scenarios/009-security-toolchain-fail-closed.md index 1a7d9a0..fb2a5f6 100644 --- a/scenarios/009-security-toolchain-fail-closed.md +++ b/scenarios/009-security-toolchain-fail-closed.md @@ -4,22 +4,26 @@ status: draft # Scenario 009: Toolchain findings pass citation validation while invariant findings fail closed without a security model -Validates that the polymorphic citation contract holds both ways: a `kind: toolchain` finding (osv-scanner/trivy output) passes `scripts/validate-citations.sh` with no `rule_id` (exit 0, kept), while a `kind: invariant` finding run without `SECURITY_MODEL_FILE` drops fail-closed (exit 1, `WARN: dropped` on stderr naming the `invariant_id`). +Validates that the polymorphic citation contract holds both ways: a `kind: toolchain` finding (osv-scanner/trivy output) passes `scripts/validate-citations.sh` with no `rule_id` (exit 0, kept), while a `kind: invariant` finding run without `SECURITY_MODEL_FILE` drops fail-closed (exit 1, `WARN: dropped` on stderr naming the `invariant_id`); and that a `--security` review surfaces the toolchain finding as a Must-Fix item in the report, never a silent skip. ## Test PR -TBD — task 4 names the security-mode fixture PR; expected shape: a public fixture repo whose `go.mod` carries a vulnerable dependency flagged by osv-scanner/trivy (the toolchain finding) and an `order` resource handler whose derived security model registers the invariant `INV-1` (the invariant finding). +The fixture is built inline in Setup (no perpetual fixture PR): a minimal generic Go app scaffolded under `$WORK` whose `go.mod` requires the vulnerable `golang.org/x/crypto v0.17.0`, imported (e.g. via `golang.org/x/crypto/bcrypt` in `main.go`) so govulncheck/osv-scanner flag it, plus a minimal `main.go`. The derived security model registers invariant `INV-1`. The validator half needs no fixture repo — it runs over two hand-built findings JSON files from the coding repo root. ## Setup -- [ ] Build a toolchain findings JSON from osv-scanner/trivy output: `[{"kind": "toolchain", "tool": "osv-scanner", "package": "golang.org/x/crypto", "version": "v0.17.0", "advisory": "GHSA-45x7-px36-r8r7", "file": "go.mod", "line": 5}]` -- [ ] Build an invariant findings JSON: `[{"kind": "invariant", "invariant_id": "INV-1", "file": "pkg/authz/order.go", "line": 33}]` +- [ ] Build the toolchain findings JSON: `printf '%s' '[{"kind": "toolchain", "tool": "osv-scanner", "package": "golang.org/x/crypto", "version": "v0.17.0", "advisory": "GHSA-45x7-px36-r8r7", "file": "go.mod", "line": 5}]' > /tmp/scen009-toolchain-findings.json` +- [ ] Build the invariant findings JSON: `printf '%s' '[{"kind": "invariant", "invariant_id": "INV-1", "file": "pkg/authz/order.go", "line": 33}]' > /tmp/scen009-invariant-findings.json` - [ ] Do NOT set `SECURITY_MODEL_FILE` for the invariant run — the absent model is the fail-closed condition +- [ ] Scaffold the vulnerable-app fixture: `WORK=$(mktemp -d) && cd "$WORK" && go mod init example.com/order-app && git init -q && git config user.email fixture@example.com && git config user.name fixture` +- [ ] Write `go.mod` requiring `golang.org/x/crypto v0.17.0` and a minimal `main.go` importing `golang.org/x/crypto/bcrypt`; commit on `master`: `git add -A && git commit -qm 'app with vulnerable x/crypto'` +- [ ] Derive the session security model per `docs/security/security-review-pipeline.md` — the model registers invariant `INV-1` ## Action -- [ ] Run the validator over the toolchain findings: `bash scripts/validate-citations.sh toolchain-findings.json > /tmp/scen009-toolchain-out.json; echo $? > /tmp/scen009-toolchain-exit` -- [ ] Run the validator over the invariant findings without a model: `bash scripts/validate-citations.sh invariant-findings.json > /tmp/scen009-invariant-out.json 2> /tmp/scen009-invariant-err; echo $? > /tmp/scen009-invariant-exit` +- [ ] Run the validator over the toolchain findings (from the coding repo root, where `rules/index.json` resolves — NOT inside `$WORK`): `bash scripts/validate-citations.sh /tmp/scen009-toolchain-findings.json > /tmp/scen009-toolchain-out.json 2> /tmp/scen009-toolchain-err; echo $? > /tmp/scen009-toolchain-exit` +- [ ] Run the validator over the invariant findings without a model: `bash scripts/validate-citations.sh /tmp/scen009-invariant-findings.json > /tmp/scen009-invariant-out.json 2> /tmp/scen009-invariant-err; echo $? > /tmp/scen009-invariant-exit` +- [ ] Run the security-mode review over `$WORK` in a fresh Claude Code session (in-place — the fixture is a local-only branch with no origin remote, which `/coding:pr-review`'s worktree flow does not support; plugin pinned to the branch under test): `/coding:local-review --security`; tee stdout to `/tmp/scen009-stdout.log`, stderr to `/tmp/scen009-stderr.log`, capture exit code to `/tmp/scen009-exit` ## Expected @@ -27,6 +31,9 @@ TBD — task 4 names the security-mode fixture PR; expected shape: a public fixt - [ ] Invariant findings without a model drop fail-closed: `cat /tmp/scen009-invariant-exit` prints `1` - [ ] The drop is logged to stderr as `WARN: dropped` naming the `invariant_id`: `grep -c 'WARN: dropped' /tmp/scen009-invariant-err` ≥ 1 and `grep -c 'INV-1' /tmp/scen009-invariant-err` ≥ 1 - [ ] The kept set is empty for the fail-closed run: `jq '.findings | length' /tmp/scen009-invariant-out.json` prints `0` +- [ ] The walk completes cleanly: `cat /tmp/scen009-exit` prints `0` +- [ ] The toolchain finding surfaces as a Must-Fix item in the report, never a silent skip: `grep -c 'GHSA-45x7-px36-r8r7' /tmp/scen009-stdout.log` ≥ 1 and `grep -c 'golang.org/x/crypto' /tmp/scen009-stdout.log` ≥ 1 +- [ ] The session security model never lands in the fixture repo — `find "$WORK" -name security-model.json` returns nothing - [ ] Run completes in under 10 minutes (wall clock `real` < 10m) ## Cleanup @@ -35,4 +42,4 @@ TBD — task 4 names the security-mode fixture PR; expected shape: a public fixt --- -**Status**: draft — not walked; the validator half is container-executable now, the fixture-PR half becomes walkable after task 4 wires the security-review signal. Not promoted to active. +**Status**: draft — walkable after the `--security` command wiring (spec 010) merges; not promoted to active. diff --git a/scenarios/010-security-zero-findings.md b/scenarios/010-security-zero-findings.md new file mode 100644 index 0000000..4d54eba --- /dev/null +++ b/scenarios/010-security-zero-findings.md @@ -0,0 +1,41 @@ +--- +status: draft +--- + +# Scenario 010: Security mode on a clean generic Go app reports zero findings with a Security Model provenance block + +Validates that in security mode, `/coding:local-review --security` over a clean generic Go HTTP app with a properly guarded order resource and a diff with no security-relevant violations reports a `Security Findings` section with zero rule/invariant findings, still emits the `Security Model` provenance block (`derived_from`, entry-point count, inventory counts), and blocks nothing. + +## Test PR + +The fixture is built inline in Setup (no perpetual fixture PR): a clean minimal generic Go HTTP app scaffolded under `$WORK` with a properly guarded order resource — the ownership check is called in the handler, no weak crypto, no hardcoded secrets, no string-interpolated SQL — and a feature-branch diff that introduces nothing security-relevant. + +## Setup + +- [ ] Scaffold the fixture repo: `WORK=$(mktemp -d) && cd "$WORK" && go mod init example.com/order-app && mkdir -p pkg/handler pkg/authz && git init -q && git config user.email fixture@example.com && git config user.name fixture` +- [ ] Write the clean app on `master`: `pkg/handler/order.go` exposes `GET /orders/{order_id}` and calls `RequireOrderAccess(orderID, userID)` before resolving; `pkg/authz/order.go` defines the service-layer ownership check; no weak crypto, no hardcoded secrets, no string-interpolated SQL anywhere; commit `git add -A && git commit -qm 'clean guarded order app'` +- [ ] Create the feature branch and make a benign Go change that introduces nothing security-relevant (e.g. add a pure helper function or a doc comment): `git checkout -b benign-helper && && git add -A && git commit -qm 'add benign helper'` +- [ ] The review's diff (`HEAD~1`) is a Go diff with no security-relevant violations +- [ ] Derive the session security model per `docs/security/security-review-pipeline.md` — the model derives with the guarded resource and its authorization function + +## Action + +- [ ] Run the security-mode review over `$WORK` in a fresh Claude Code session (in-place — the fixture is a local-only branch with no origin remote, which `/coding:pr-review`'s worktree flow does not support; plugin pinned to the branch under test): `/coding:local-review --security`; tee stdout to `/tmp/scen010-stdout.log`, stderr to `/tmp/scen010-stderr.log`, capture exit code to `/tmp/scen010-exit` + +## Expected + +- [ ] `cat /tmp/scen010-exit` prints `0` +- [ ] The `Security Findings` section is present: `grep -c 'Security Findings' /tmp/scen010-stdout.log` ≥ 1 +- [ ] The `Security Findings` section lists zero rule/invariant findings — no finding blocks with `file:` evidence inside the region (or the section reads `None.`): `awk '/^#### Security Findings/{flag=1; next} /^#### /{flag=0} flag' /tmp/scen010-stdout.log | grep -c 'file:'` prints `0` +- [ ] The `Security Model` provenance block is present — `derived_from`, entry-point count, inventory counts: `grep -c 'Security Model' /tmp/scen010-stdout.log` ≥ 1 +- [ ] No finding is blocking — no verifier verdict satisfies `confidence==confirmed ∧ exploitability==high ∧ impact≥medium`: `grep -c '"blocking": true' /tmp/scen010-stdout.log` returns `0` +- [ ] The session security model never lands in the fixture repo — `find "$WORK" -name security-model.json` returns nothing +- [ ] Run completes in under 10 minutes (wall clock `real` < 10m) + +## Cleanup + +- `rm -rf "$WORK" /tmp/scen010-*` + +--- + +**Status**: draft — walkable after the `--security` command wiring (spec 010) merges; not promoted to active. diff --git a/specs/completed/010-security-review-command-wiring.md b/specs/completed/010-security-review-command-wiring.md new file mode 100644 index 0000000..babca59 --- /dev/null +++ b/specs/completed/010-security-review-command-wiring.md @@ -0,0 +1,153 @@ +--- +status: completed +tags: + - dark-factory + - spec +approved: "2026-08-23T14:23:48Z" +generating: "2026-08-23T14:23:49Z" +prompted: "2026-08-23T14:40:03Z" +verifying: "2026-08-23T14:52:08Z" +completed: "2026-08-23T16:59:08Z" +branch: dark-factory/security-review-command-wiring +--- + +## Summary + +- Add a `--security` flag to `/coding:pr-review`, `/coding:code-review`, and `/coding:local-review`. When set, the review runs the full security pipeline on top of the normal selector flow: derived security model, six-classifier trait groups with non-negotiable authz over-selection, deterministic invariant selection, an adjudicator fed the model subset, a falsification-gate verifier, derived blocking, and a Security Findings report section. +- The `--security` wiring only sets the **security-review signal** that activates the already-documented dormant `## Security Extension` in `docs/selector-mode-guide.md`. The classify/adjudicate steps 4c-sel/4d-sel and the shipped pipeline/verifier/blocking docs and agents are NOT re-authored. +- The per-review security model (`security-model.json`) is derived in-session, written under `/tmp` (mirroring `/tmp/pr-review-findings.json`), and never committed to any repo. +- Blocking is derived from the verifier verdict (`confidence==confirmed ∧ exploitability==high ∧ impact≥medium`), never stored as a severity label. +- Diff anchoring: PR mode (pr-review, local-review) reports findings only on diff-changed lines / invariants whose attack surface the diff touched; audit mode (code-review) is whole-repo. +- Ships the 4 mandated acceptance scenarios in `scenarios/`: finalizes the existing drafts 007 (IDOR-confirmed), 008 (IDOR-rejected-by-verifier), 009 (toolchain-fail-closed) by replacing their `TBD — task 4` fixture blocks with inline-built generic fixtures and the exact `--security` invocation, and authors the new 010 (security-zero-findings). Plus README.md / llms.txt / agent-list integration. + +## Problem + +The security pipeline — recon, evidence-pointered security model, six security trait groups, deterministic invariant selection, the `security-verifier` falsification gate, derived blocking, and the polymorphic finding contract — is fully documented and shipped (docs, agents, validator), but no slash command sets the **security-review signal** that activates it. The dormant Security Extension in `docs/selector-mode-guide.md` is inert. As a result the pipeline is unexercisable: an operator cannot run a security review on a real diff, the verifier gate never fires, and a future `security-review-agent` Go program has no callable entry point. Worse, the wiring is a silent-failure hazard — a `--security` review that runs the pipeline partially (mechanical findings only, no recon, no verifier) looks complete but ships unverified security findings. This task makes the pipeline a usable review domain behind a single flag. + +## Goal + +After this work, running `/coding:pr-review --security` (and the code-review / local-review equivalents) on a real repo diff produces a security review: the session derives a per-review evidence-pointered security model, the classifier activates the six security trait groups and deterministically selects invariants, the adjudicator judges the applicable invariants against the diff, high-severity findings pass the verifier falsification gate before emission, blocking is derived from the verdict (never stored as severity), findings are diff-anchored in PR mode and whole-repo in audit mode, and the report carries a Security Findings section plus a Security Model provenance block, alongside the normal traceability section. The four acceptance scenarios exist and pass, and README.md / llms.txt / agent tables reflect the new mode. + +## Non-goals + +- **The Go `security-review-agent` program (task 5)** — this is the slash-command surface that program calls; the program itself is a separate task and out of scope. +- **The comprehensive judgment/invariant-tier security rules (task 6)** — no new detection rules beyond wiring the existing 5 mechanical rules and the dormant judgment/invariant tier contract. +- **Bot deployment / `.maintainer.yaml`-gated watcher (iteration 2)** — local plugin surface only. +- **Runtime / network probing (design phase 2)** — static review only. +- **Re-authoring `scripts/validate-citations.sh`** — it is already polymorphic (`kind` rule|invariant|toolchain, `SECURITY_MODEL_FILE`, fail-closed); the wiring only passes the model file to it. +- **Editing `docs/selector-mode-guide.md` steps 4c-sel/4d-sel or the dormant Security Extension** — the extension is the frozen classify/adjudicate contract; the `--security` wiring activates it without editing it. +- **Per-surface configurable blocking thresholds** — the v1 fixed formula applies to all surfaces; tuning is a separate task. +- **Backfilling README scenario-table rows for 005/006** — pre-existing staleness, not part of this mode. + +## Acceptance Criteria + +**Container-executable rung** (runs in the YOLO container at prompt time; plugin repo — no Post-Deploy markers, no cluster): + +- [ ] **AC1 — repo integrity:** `make precommit` exits 0 in the worktree — evidence: exit code (link validation + JSON syntax + check-rule-tests). +- [ ] **AC2 — `--security` flag wired into all three commands:** `grep -n -- '--security' commands/pr-review.md commands/code-review.md commands/local-review.md` returns ≥1 matching line per file, and `grep -n 'argument-hint:.*--security' commands/pr-review.md commands/code-review.md commands/local-review.md` returns ≥1 per file (each `argument-hint` names the flag) — evidence: file-content grep. +- [ ] **AC3 — security pipeline steps wired in the commands:** `grep -n 'security-model.json' commands/pr-review.md commands/code-review.md commands/local-review.md` returns ≥1 per file; `grep -n '/tmp/security-model.json' commands/*.md` returns ≥1 (session-local model path); `grep -n 'SECURITY_MODEL_FILE' commands/*.md` returns ≥1 (validator receives the session model); `grep -n 'security-verifier' commands/*.md` returns ≥1 (the falsification gate is invoked) — evidence: grep counts. +- [ ] **AC4 — report sections encoded:** `grep -n 'Security Findings' commands/*.md` returns ≥1 (the findings section with provenance + derived blocking); `grep -n 'Security Model' commands/*.md` returns ≥1 (the provenance block: `derived_from`, entry-point count, inventory counts, `model refresh:` lines); `grep -n 'model refresh' commands/*.md` returns ≥1 — evidence: grep counts. +- [ ] **AC5 — diff anchoring encoded:** `grep -n 'changed lines' commands/pr-review.md commands/local-review.md` returns ≥1 per file (PR mode: findings only on diff-changed lines / invariants whose attack surface the diff touched) and `grep -n 'whole' commands/code-review.md` returns ≥1 (audit mode: whole-repo, no diff anchoring) — evidence: grep counts. +- [ ] **AC6 — dormant contract and shipped artifacts untouched (negative):** `git diff origin/master -- docs/selector-mode-guide.md scripts/validate-citations.sh agents/security-verifier.md agents/go-security-specialist.md docs/security/` returns empty AND `git status --porcelain -- docs/selector-mode-guide.md scripts/validate-citations.sh agents/security-verifier.md agents/go-security-specialist.md docs/security/` returns empty (no modified, no untracked) — evidence: empty git diff + empty git status (steps 4c-sel/4d-sel and the Security Extension are not renumbered or rewritten; the validator, verifier, and rule-base docs are not re-authored). +- [ ] **AC7 — scenario files present and finalized:** `ls scenarios/010-security-zero-findings.md` succeeds and `grep -c 'status: draft' scenarios/010-security-zero-findings.md` returns ≥1; `grep -c 'TBD' scenarios/007-security-idor-confirmed.md scenarios/008-security-idor-rejected-by-verifier.md scenarios/009-security-toolchain-fail-closed.md` returns 0 (the `TBD — task 4` fixture blocks are replaced); `grep -c -- '--security' scenarios/007-security-idor-confirmed.md scenarios/008-security-idor-rejected-by-verifier.md scenarios/009-security-toolchain-fail-closed.md scenarios/010-security-zero-findings.md` returns ≥1 per file (each names the exact `--security` invocation); each of the 4 files has ≥8 unchecked checkboxes across Setup/Action/Expected/Cleanup (`grep -c '^- \[ \]'` ≥8 per file) — evidence: file presence + grep counts. +- [ ] **AC8 — repo surfaces updated:** `grep -n -- '--security' README.md` returns ≥1 (commands table and/or Quick Start); `grep -n '010-security-zero-findings' README.md` returns ≥1 (Acceptance Scenarios table gains the 4 security scenario rows); `grep -c 'security-review-pipeline' llms.txt` ≥1 and `grep -c 'security-verifier' llms.txt` ≥1 (llms.txt stays aligned; the mode adds no new guide or agent, so no new llms entry is required); `git diff origin/master --stat -- agents/` shows no new files AND `git status --porcelain -- agents/` returns empty (no modified, no untracked; no new agent; `security-verifier` stays registered in the README agent tables) — evidence: grep counts + git diff + git status. + +**Operator-executable rung** (host-side scenario walks after merge; each against an inline-built generic fixture in a fresh Claude Code session, plugin pinned to the branch under test): + +- [ ] **AC9 — security-zero-findings walk passes:** a `--security` review of a clean generic Go app reports the `Security Findings` section with zero rule/invariant findings and the `Security Model` provenance block present, and no finding blocks — evidence: scenario 010 Expected checkboxes + `cat /tmp/scen010-exit` prints 0 and stdout greps (`Security Findings` ≥1, `Security Model` ≥1, `grep -c '"blocking": true' /tmp/scen010-stdout.log` returns 0). +- [ ] **AC10 — IDOR-confirmed walk passes:** a `--security` review of a generic order app with a seeded ownership-check bypass emits the invariant-kind IDOR finding, the verifier verdict is `confirmed` with `counterevidence_checked` populated, and the finding blocks (formula holds) — evidence: scenario 007 Expected checkboxes + stdout greps (`"confidence": "confirmed"` ≥1, `counterevidence_checked` ≥1, `"exploitability": "high"` ≥1, `"impact": "high"` ≥1, `grep -c '"blocking": true' /tmp/scen007-stdout.log` ≥1). +- [ ] **AC11 — IDOR-rejected walk passes:** a `--security` review of a generic order app guarded by a service-layer ownership check rejects the naive IDOR claim — verdict `rejected`, `reject_reason` recorded, the finding does NOT emit, no blocking — evidence: scenario 008 Expected checkboxes + stdout greps (`"confidence": "rejected"` ≥1, `reject_reason` ≥1, absence grep for the finding id = 0, `grep -c '"blocking": true' /tmp/scen008-stdout.log` returns 0). +- [ ] **AC12 — toolchain-fail-closed walk passes:** a `--security` review whose dependency toolchain pass flags a vulnerable dependency reports the toolchain finding as a Must-Fix item (never a silent skip), and the validator half of scenario 009 still passes toolchain findings and fail-closes invariant findings without a model — evidence: scenario 009 Expected checkboxes + stdout greps (toolchain finding id/advisory ≥1 in the report, `WARN: dropped` on the no-model run). + +## Verification + +No Post-Deploy markers — plugin repo, no cluster. + +## Container-executable (runs inside the YOLO container at prompt time) + +- `make precommit` — link validation + JSON syntax + check-rule-tests clean. +- `grep -n -- '--security' commands/pr-review.md commands/code-review.md commands/local-review.md` — ≥1 line per file. +- `grep -n 'security-model.json' commands/pr-review.md commands/code-review.md commands/local-review.md` — ≥1 per file; `grep -n '/tmp/security-model.json' commands/*.md` — ≥1. +- `grep -n 'SECURITY_MODEL_FILE' commands/*.md` — ≥1; `grep -n 'security-verifier' commands/*.md` — ≥1. +- `grep -n 'Security Findings' commands/*.md` — ≥1; `grep -n 'Security Model' commands/*.md` — ≥1; `grep -n 'model refresh' commands/*.md` — ≥1. +- `grep -n 'changed lines' commands/pr-review.md commands/local-review.md` — ≥1 each; `grep -n 'whole' commands/code-review.md` — ≥1. +- `git diff origin/master -- docs/selector-mode-guide.md scripts/validate-citations.sh agents/security-verifier.md agents/go-security-specialist.md docs/security/` — empty. +- `ls scenarios/010-security-zero-findings.md` — succeeds; `grep -c 'TBD' scenarios/007-security-idor-confirmed.md scenarios/008-security-idor-rejected-by-verifier.md scenarios/009-security-toolchain-fail-closed.md` — 0; `grep -c -- '--security' scenarios/007-*.md scenarios/008-*.md scenarios/009-*.md scenarios/010-*.md` — ≥1 per file. +- `grep -n -- '--security' README.md` — ≥1; `grep -n '010-security-zero-findings' README.md` — ≥1; `grep -c 'security-review-pipeline' llms.txt` — ≥1; `git diff origin/master --stat -- agents/` — no new files. + +## Operator-executable (runs on the host after PR merge, spec verification ladder) + +All four walks run `/coding:local-review --security` (in-place) against the inline-built fixtures — the fixtures are local-only branches with no origin remote, and pr-review's worktree flow unconditionally runs `git fetch origin` (commands/pr-review.md Step 0b; precedent: scenario 005). With the two-commit fixture shape, `HEAD~1` (local-review's diff) equals `master..HEAD` (pr-review's diff), so the diff-relevant assertions hold. + +- Walk scenario 010 (security-zero-findings): inline-build a clean generic Go app, run `/coding:local-review --security`, assert clean Security Findings + Security Model provenance + no blocking. +- Walk scenario 007 (IDOR-confirmed): inline-build a generic order app with a seeded ownership-check bypass, run `/coding:local-review --security`, assert verifier `confirmed` + `counterevidence_checked` + blocking. +- Walk scenario 008 (IDOR-rejected): inline-build a generic order app with a service-layer ownership check, run `/coding:local-review --security`, assert verifier `rejected` + `reject_reason` + no emission + no blocking. +- Walk scenario 009 (toolchain-fail-closed): run the validator half (toolchain passes, invariant fail-closed without model) plus a `/coding:local-review --security` review walk whose dependency scan flags a vulnerable dependency; assert the toolchain finding surfaces as Must-Fix, never a silent skip. + +## Desired Behavior + +1. **`--security` flag parsing and signal (all three commands).** Each command parses `--security` as a position-independent boolean flag from its arguments (alongside the existing mode token — short/full/selector, selector default). When present, the command sets the security-review signal, which activates the dormant `## Security Extension` in `docs/selector-mode-guide.md` (steps 4c-sel/4d-sel run byte-for-byte as before when the signal is absent). The `argument-hint` of each command gains the flag. Setting `--security` must never be silently ignored: the mode runs the security pipeline for the reviewed scope, and a scope containing Go source always produces the Security Model provenance block (never a silent skip). +2. **Security recon and the session-local derived model.** Before classify, the command runs the recon pass per `docs/security/security-review-pipeline.md`: enumerate entry points (recall-oriented), resolve identities and auth mechanisms, resolve resources and their `authorization_functions`, and derive invariants (`resource → identifier → authorization_function`) with `file:line` evidence. The result is written to `/tmp/security-model.json` (session-local, mirroring `/tmp/pr-review-findings.json`) and never to any path inside the reviewed repo. The model carries `derived_from` (repo, head, review_id), the freshness gate (changed-evidence entries re-derived, unchanged carried forward), diff-relevant truncation on large repos, and the countable attack-surface inventory. +3. **Classifier security trait groups and deterministic invariant selection.** Under the signal, security-relevant selection uses exactly the six trait groups `authz`, `input-origin`, `data-to-sink`, `external-io`, `crypto`, `secrets`. The `authz` group is non-negotiable: any diff touching a file cited as evidence by an entry point operating on a modeled resource, or cited as evidence by a modeled resource's `authorization_function`, MUST select `authz`. Invariant selection is fully deterministic via the drift bridge and the invariant's `attack_surfaces`: the diff touching an invariant's attack surface or its evidence source forces that `invariant_id` into the applicable set (no LLM judgment, never skipped). The HARD INVARIANT holds: the applicable set is a subset of the Step 4b-i candidate set — trait groups never add a rule the glob did not produce. +4. **Adjudicator extension and the verifier gate.** The Step 4d-sel adjudicator input gains the diff-relevant model subset, the applicable invariants with their evidence authorization functions, and the attack-surface inventory as a drift signal. Each applicable invariant is judged against the diff slice with the single question: does this change preserve the invariant? Invariant-kind findings cite `invariant_id`. After adjudication, before emission, a verifier gate runs per the contract in `agents/security-verifier.md` (7-item falsification checklist; verdict `confirmed | plausible | rejected` with `confidence`, `exploitability`, `impact`, `attack_preconditions`, `attack_path`, `security_boundary_missing`, `counterevidence_checked`, and `reject_reason` required on rejection). The gate is a hard pre-emission step for `severity=critical` and for `severity=major` when `confidence=confirmed`; every surviving high-severity finding carries a populated `counterevidence_checked`. Execution mechanism (in-session role vs sub-agent spawn) is an implementation detail — agent decides at impl time, preserving the gate's hard pre-emission property. +5. **Blocking derived, never stored as severity.** Under the signal, each finding's blocking state is derived as `blocking = confidence==confirmed ∧ exploitability==high ∧ impact≥medium`. The formula is the uniform v1 default for all three surfaces (local, bot, audit); there are no per-surface config fields and no opt-out flags. Blocking never consults the finding's `severity` label; a `plausible` critical does not block merge and is reported as a required human review item. +6. **Diff anchoring and scope.** In PR mode (pr-review, local-review), findings are reported ONLY for lines the diff changed or invariants whose attack surface the diff touched; whole-file context is permitted for reasoning, never for gating. In audit mode (code-review), the scope is the whole repo — no diff anchoring — and the recon derives a whole-repo model. +7. **Report: Security Findings + Security Model provenance + traceability.** Under the signal, the Step 5 report gains a `Security Findings` section listing every security finding (rule and invariant, and toolchain findings from the deps pass) with its `file:line`, provenance (`kind` + `rule_id`/`invariant_id`), verifier verdict fields, and derived blocking state, plus a `Security Model` provenance block recording `derived_from`, entry-point count, each attack-surface inventory count, and any `model refresh:` lines verbatim. The normal severity buckets and the selector traceability section still appear. Whether individual security findings are additionally duplicated into the severity buckets is an implementation detail — agent decides at impl time; the Security Findings section is the authoritative carrier of verdict and blocking data. +8. **Citation validation integration.** The mode's citation-validation invocation passes `SECURITY_MODEL_FILE` pointing at `/tmp/security-model.json`, so invariant-kind findings resolve against the session model's `invariants[].id`. A finding cites exactly one provenance (`kind=rule` → `rule_id` in `rules/index.json`; `kind=invariant` → `invariant_id` in the model; `kind=toolchain` → no id). Absent an unset/missing/unreadable/unparseable model, invariant findings drop fail-closed with a WARN to stderr and are never kept — the existing validator enforces this; the command only supplies the model file. The toolchain/deps pass is fail-closed: a dependency-scan failure surfaces as a Must-Fix toolchain finding in the report, never a silent skip. + +## Constraints + +- **Dormant contract frozen:** the `## Security Extension` in `docs/selector-mode-guide.md` is the classify/adjudicate contract for this mode; steps 4c-sel/4d-sel and the extension text MUST NOT be renumbered, rewritten, or edited. The `--security` command wiring sets the security-review signal that activates it; when the signal is absent the existing procedure runs byte-for-byte. +- **Shipped artifacts not re-authored:** `docs/security/security-review-pipeline.md`, `docs/security/security-review-guide.md`, `agents/security-verifier.md`, `agents/go-security-specialist.md`, and `scripts/validate-citations.sh` already exist and are NOT modified by this spec. No new security detection rules (the 5 mechanical rules + the dormant judgment/invariant tier are the full rule set). +- **Blocking:** derived per formula, never stored as severity; per-surface configurable thresholds are out of scope (v1 fixed formula, no config fields, no opt-out flags). +- **Finding provenance:** exactly one of `rule_id ∈ rules/index.json`, `invariant_id ∈ derived model`, or `kind=toolchain` (no id). No new validation-script work. +- **Model location:** `security-model.json` is session-local under `/tmp`, never committed to any repo; if it is ever written inside the reviewed repo tree it is deleted and rewritten to the session-local path. +- **Commands are thin wrappers:** no inline rules; all agent references use the `coding:` prefix; the classify/adjudicate/verifier procedure is executed per the guides, not re-implemented in the command. +- **HARD INVARIANT preserved:** the applicable set is a subset of the Step 4b-i candidate set; trait groups never add a rule the glob did not produce. +- **Selector-mode zero-sub-agent-spawn property preserved** for the classify/adjudicate steps; the verifier execution mechanism is the only deviation point and is reversible (agent decides at impl time). +- **Generic content only:** fixtures and examples use User, Order, Product, Customer — never trading/project-specific content, never personal paths. +- **Scenario location and numbering:** scenarios live in `scenarios/` (next after 006); existing drafts 007/008/009 are finalized, 010 is new. Fixtures are built inline in each scenario's Setup (clone/scaffold a minimal generic Go app + apply the violating code, per 002 precedent) — no new perpetual fixture PR required. + +## Failure Modes + +| Trigger | Expected behavior | Recovery | +|---------|-------------------|----------| +| Recon aborts mid-derivation or the model is unreadable/unparseable | The partial model is discarded; invariant-kind findings drop fail-closed (WARN to stderr, never kept); the review continues with rule/toolchain findings | Re-run the recon with a wider package scope, merge into the model, re-run the review | +| `SECURITY_MODEL_FILE` unset/missing at validation | Invariant findings fail closed (exit 1, `WARN: dropped` naming the `invariant_id`), never kept — never a silent pass | Confirm the command passes `/tmp/security-model.json` to the validator | +| ast-grep/sg not in PATH (Step 4.0) | Existing fail-fast: report "Must Fix toolchain failure" and stop the funnel — the `--security` mode must not bypass or soften this | Install ast-grep, re-run | +| Toolchain/deps pass fails (osv-scanner/trivy/govulncheck error or flagged vulnerability) | Fail-closed: the toolchain failure or the dependency finding surfaces as a Must-Fix item in the report — never a silent skip | Fix the dependency or toolchain, re-run | +| The model is accidentally written inside the reviewed repo tree | The pipeline detects the in-tree path and rewrites the model to the session-local path | Delete the in-tree file and re-run; the scenario walk asserts no `security-model.json` inside the fixture repo | +| Model stale (an entry's evidence lines changed in the diff, or an evidence symbol no longer resolves) | Freshness gate re-derives the changed entry / drops the non-resolving one; the report carries the `model refresh:` line verbatim | Re-run the review with the refreshed model; no manual model maintenance | +| The verifier kills a real vulnerability (false kill) | Residual risk is caught by the `ai_review` post-post backstop (dismiss + COMMENT + human_review) | Human review resolves the finding; the judge's recall covers the miss direction by design | +| Mega-PR too large to enumerate | Model derivation is limited to the touched entry points/invariants; the report notes the truncation | Re-run the recon with a wider package scope and merge | +| Two concurrent review sessions on the same repo | Each session derives its own session-local model under `/tmp`; there is no shared state to race on | None — by construction | +| osv-scanner/trivy database fetch times out or is throttled during the walk | The toolchain pass reports the failure as a Must-Fix toolchain item (fail-closed), not a silent empty scan | Retry the scan with the local cache warm, re-run | + +## Security / Abuse Cases + +This feature reads and analyzes repo content (potentially attacker-authored PRs) and produces advisory findings; it never executes reviewed code. + +- **What an attacker controls:** the contents of the reviewed repo — file text, diff lines, and any file referenced in evidence. An attacker-authored PR can attempt to steer the recon/adjudicator through adversarial content. +- **What crosses a trust boundary:** reviewed-repo content enters the review session and the LLM-derived model. The model is never executed, never parsed as code, never fed into a shell or build step; `file:line` evidence strings are display-only references and the recon writes no files to those paths. +- **No invented security policy:** every finding resolves exactly one provenance (`rule_id` in `rules/index.json`, `invariant_id` in the derived model, or `kind=toolchain`); the validator drops anything unprovenanced. An attacker cannot inject a fake `rule_id` or `invariant_id` into a passing report. +- **Blocking is verifier-gated:** a finding blocks only when `confidence==confirmed ∧ exploitability==high ∧ impact≥medium`, and a confirmation requires a concrete attacker scenario plus a populated `counterevidence_checked`. An attacker cannot self-certify a finding or force a merge block through planted plausible findings; conversely a naive claim is rejected when the verifier finds real authorization. +- **What can hang/race:** toolchain scans (osv-scanner/trivy DB fetches) can hang on the network — bounded by the fail-closed toolchain reporting; two concurrent sessions derive independent session-local models (no shared state). +- **Validation discipline:** the verifier's counterevidence must resolve in code or in the model's evidence — a hopeful reference is not counterevidence; findings cite only paths seen in the reviewed scope. + +## Suggested Decomposition + +Prompts are generated in this order — each row is a single prompt with a clear scope. This spec spans 3 command files + scenarios + repo surfaces (5 layers), so the decomposition is mandatory. + +| # | Prompt focus | Covers DBs | Covers ACs | Depends on | +|---|---|---|---|---| +| 1 | Wire `--security` into `commands/pr-review.md`: flag parsing + signal, recon → `/tmp/security-model.json` derivation, classifier trait groups + authz over-selection + deterministic invariant selection, adjudicator inputs, verifier gate invocation, blocking derivation, PR-mode diff anchoring, Security Findings + Security Model report sections, `SECURITY_MODEL_FILE`-passed citation validation | 1, 2, 3, 4, 5, 6 (PR), 7, 8 | 1-6 | — | +| 2 | Mirror the wiring into `commands/code-review.md` (audit mode: whole-repo scope, no diff anchoring, whole-repo model) | 1, 2, 3, 4, 5, 6 (audit), 7, 8 | 2, 3, 4, 5, 6 | prompt 1 | +| 3 | Mirror the wiring into `commands/local-review.md` (local mode: diff anchoring) | 1, 2, 3, 4, 5, 6 (PR), 7, 8 | 2, 3, 4, 5, 6 | prompt 1 | +| 4 | Author the 4 acceptance scenarios in `scenarios/`: finalize 007/008/009 (replace `TBD — task 4` fixture blocks with inline-built generic fixtures + the exact `--security` invocation) and author 010 security-zero-findings; each with `status: draft`, Setup/Action/Expected/Cleanup checkboxes, observable assertions only | 1-8 (end-to-end) | 7, 9-12 | prompts 1-3 (the walks exercise the wired commands) | +| 5 | Repo-surface integration: README.md commands table + Quick Start + Acceptance Scenarios table rows (007-010), llms.txt alignment check, agent-table verification (no new agent) | — | 8 | — | + +Rationale: prompt 1 establishes the canonical full pipeline in the reference command (pr-review), so prompts 2 and 3 mirror a proven shape rather than each re-deriving the security procedure — this keeps each command independently reviewable and prevents three divergent interpretations of the dormant extension. Prompts 2 and 3 depend on 1 because the Security Findings/model-provenance/report-section wording and the diff-anchoring rule must be consistent across all three commands. Every wiring prompt (1-3) claims AC6 (the frozen-contract negative guard) and the command-spanning AC2/AC4 so the daemon's AC-coverage pass never orphans them. Prompt 4 requires all three commands wired before its walks can pass. Prompt 5 is independent of the command internals and can run last (or in parallel). + +## Do-Nothing Option + +If this task is not done, the security pipeline remains documented but inert: the dormant Security Extension stays dormant, no operator can run a security review on a real diff, the verifier gate and derived blocking never fire, and the future `security-review-agent` Go program has no callable slash-command entry. The shipped docs/agents/validator carry a one-time sunk cost with zero exercised value, and the wiring gap would eventually be discovered only when a partial security review silently ships unverified findings. The current state is not acceptable for the security-review rollout: the pipeline exists but is unusable, so the mode must be wired.