plan-review

Files (6)

• SKILL.md
• exempt-lines.txt · checks/
• hygiene-check.py · checks/
• lenses.md
• report-template.md
• finding.schema.json · schema/

SKILL.md (first lines)

# fsad-harness:plan-review — Adversarial Implementation-Plan Review

**What this optimizes for:** catching the thing that makes the plan unbuildable *before* anyone starts building it. A plan is cheap to change and expensive to have half-built, so the review is worth running exactly once — here — and its value comes from measuring the plan against two things it cannot check for itself: the baseline it promised to honour, and the repository it will actually land in. What that yields in practice is defined per lens in `lenses.md`, not here.

This is the **plan-stage gate**. It runs before the code-stage gates (`fsad-harness:code-review-team`, `fsad-harness:sec-review-team`) — there is no code yet.

Review-only. The skill writes exactly two things: the findings document at `target`, and the run directory (`run_dir`, defaulting beside `target` — see Parameters). Nothing else on disk is touched.

---

## Asking the user

This rule covers every question this skill asks the user: a confirmation, a choice, a missing value, a numbered decision list, or a stop-and-ask. It applies even where a later step says "ask the user" or "ask exactly once". The user may watch the decisions pane rather than the chat, so a question that only goes to chat can go unanswered.

- **When `mcp__decision-tracker__decision` is in your tool list** (loaded, or named as a deferred tool; load a deferred one with `ToolSearch` first), log each question before you ask it:
  1. Call it with `action: "open"`, the question (ending in `?`), and `options` when the answers are a closed set. Set `required: false` when the work can go on without an answer. A numbered list of questions opens one decision per item. Keep each returned `id`.
  2. Ask the question in chat as this skill says. The open call does not replace the chat question.
  3. When the user answers, in chat or as a `Decision Dn: <answer>` message from the plugin, call the tool with `action: "close"`, the `id` and the answer.
  4. When the change the answer asked for is in place, call it with `action: "implement"`, the `id` and a `path:line`.
- **When the tool is absent**, ask in chat only.
- An `AskUserQuestion` call is logged on its own. Do not open it again.
- This rule changes where a question is logged. It does not change when this skill asks, or how many times.

## Parameters

| Parameter | Required | Accepts | Notes |
|---|---|---|---|
| `source` | **yes** | A GitHub PR (`#439`, a full URL, or `owner/repo#N`), a branch name, a file path, or a glob | The plan under review. |
| `target` | **yes** | A file path | Where the findings document is written. Parent directories are created. |
| `baseline` | no | Path(s) to a spec, ADR, prior plan, or PR that `source` is meant to honour or supersede | When omitted, candidates are inferred (Step 0.3) and the chosen one is named in the report header. |
| `depth` | no | `quick` \| `standard` (default) \| `deep` | Controls the lens roster, verifier count, and critic rounds (Step 0.4). |
| `scope` | no | Free text | Narrows the review, e.g. "schema and migration steps only". Recorded verbatim in the report header. |
| `run_dir` | no | A directory path | Where the run artefacts go. Defaults to `<directory containing target>/.plan-review/runs/<run_id>/`. Set it to keep artefacts out of the repository, or to place them somewhere the caller's tooling already collects. The internal layout (Step 0.6) is fixed wherever it points. |

**The skill prompts for any required parameter the invocation did not supply, and it never guesses a write path.** If `target` was not given, ask for it — do not infer one from the plan's location, do not default to a filename beside the plan, and do not proceed with a placeholder. If `source` was not given, ask for it — do not review "the most recent plan" or "the current branch" on your own initiative. A wrong target written confidently is worse than a question.

---

## When to skip

Say so and stop, rather than producing a report that pads a non-problem:

- **Single-step plans.** One step, one file, one acceptance criterion. There is no sequencing to audit, no cross-reference graph, and no baseline drift to measure. A normal read is cheaper and just as good.
- **Plans with no baseline and no cross-references.** If nothing resolves as `baseline` and the plan cites no ADRs, no sibling documents, no numbered artefacts, and no source files, then five of the seven lenses have nothing to check against — the review degenerates into proofreading. Offer `fsad-harness:spec-review` instead, which judges a document on its own terms.
- **Pure-copy edits.** Wording, formatting, typo, or link-text changes to an existing plan. Diff the prose; do not convene a review team.

If the user asks anyway after being told, run it — say what the reduced roster will be first.

---

## Step 0 — Resolve parameters

### 0.1 Resolve `source`

| `source` looks like | Resolution |
|---|---|
| `#N`, `owner/repo#N`, or a PR URL | `gh pr view <N> --json number,title,headRefName,baseRefName,headRefOid,baseRefOid,body,files,url` then `gh pr diff <N>`. |
| A branch name | `git rev-parse <branch>` for the head SHA; merge-base against the default branch for the base SHA; `git diff <base>...<branch> --stat`. |
| A file path | The plan document itself. Head SHA = `git rev-parse HEAD`; no base SHA. |
| A glob | Expand it. If it matches more than one document, list the matches and **ask which is the plan** — the rest become supporting documents, not additional subjects. |

For every form, establish and record: the **head SHA**, the **base SHA** (where one exists), and **whether the base is behind the repository's default branch**. Pass all three to `consistency-checker`, which owns base drift — its brief in `lenses.md` says how to measure it and how to grade it.

Then collect the plan's supporting documents:

- The **decisions sidecar** — glob for `<plan-basename>-decisions.md` in the plan's own directory (see Path conventions). Requirements hide there.
- The **testing document** — any document the plan cross-references whose name matches `test|qa|verification`.
- For a PR, the **PR body** — it frequently names the baseline the description in the plan file omits.

Read all of them. A document not read cannot be cited, and the completeness critic will ask which ones you skipped.

### 0.2 Resolve `target` and detect a second pass

Create `target`'s parent directories. Then:

- **`target` does not exist** — first pass. Write a fresh document.
- **`target` exists** — **second pass.** Read it in full *before* Phase 1, and extract its previous findings into `<RUN_DIR>/previous-findings.json`. The new document must:
  - **Mark resolved** — a previous finding whose evidence no longer reproduces at the new head SHA. Say what changed and cite the new `path:line`.
  - **Carry forward still-open** — a previous finding that still reproduces, keeping its original identifier and noting how many passes it has survived.
  - **State what changed** — a "Since the previous pass" block naming the previous head SHA, the new head SHA, the counts resolved / still open / newly raised.

  Never silently overwrite a previous pass's findings. A finding that disappears without being marked resolved is a review failure, not a clean bill of health.

### 0.3 Resolve `baseline`

If `baseline` was supplied, use it. Otherwise infer candidates, in this order, and **state in the report header which was chosen and why**:

1. Any document the PR body or plan preamble names as the thing it implements, supersedes, or follows.
2. Documents matching `spec|prd|rfc|adr|proposal|design` under the configured spec/ADR directories (Path conventions below).
3. The immediately preceding numbered plan in the same directory, if the plan's own numbering pattern implies one.

If more than one candidate survives and no signal separates them, **ask**. If none resolves, record `baseline: none` in the header, skip `baseline-diff-auditor`, and say so in Coverage — a silent skip reads as "nothing was dropped", which is exactly the false negative this skill exists to prevent.

### 0.4 Resolve `depth` and select the roster

| Lens | `quick` | `standard` | `deep` |
|---|:---:|:---:|:---:|
| `completeness-auditor` | ● | ● | ● |
| `consistency-checker` | ● | ● | ● |
| `testability-auditor` | ○ | ● | ● |
| `logic-auditor` | | ● | ● |
| `assumption-hunter` | | ● | ● |
| `feasibility-critic` | | ○ | ● |
| `baseline-diff-auditor` | ○ | ○ | ● |

● = always runs. ○ = conditional, resolved as follows so the counts stay fixed:

- **`quick` runs exactly 3 lenses.** The third slot is `baseline-diff-auditor` when a baseline resolved, `testability-auditor` when none did.
- **`standard` runs exactly 6 lenses.** The sixth slot is `baseline-diff-auditor` when a baseline resolved, `feasibility-critic` when none did.
- **`deep` runs all 7, then a duplicate pass of all 7** with different seeds — different reading order, and an instruction to start from the sections the first pass is least likely to have reached. Duplicate-pass output goes to `<lens>-b.findings.jsonl`. 14 lens files.

`baseline-diff-auditor` is **required whenever a baseline resolves**, at every depth. It is skipped only when no baseline resolves. Every lens not run is recorded in Coverage with its reason — silent skipping is forbidden.

Verifier count and critic rounds follow depth: see Phase 3 and Phase 5.

### 0.5 Apply `scope`

If `scope` is set, pass it verbatim into every lens brief as a narrowing instruction, and record it in the report header. Findings outside scope are not raised. **Log what scope excluded** in Coverage — a narrowed review that reads as a full one is a silent cap.

From tbeack/fsad_playbook, MIT.

More from tbeack/fsad_playbook