feat: architect phase and reviewed sub-issue breakdown for /grill-rfc

[SamuelDenani]

What

Three additions to /grill-rfc, which previously went from a settled design tree straight to task sub-issues in one unreviewed step.

1. An architect phase (§2c). Extracts a boundary ledger from the settled design — a decision earns a place only if it creates or changes something two parties must agree on, and decisions group by seam rather than by decision, which is what caps the diagram count. The EXCLUDED list is mandatory and reasoned, so the anti-spam judgment is auditable rather than silent. boundary-reviewer checks the extraction before anything is drawn. Then one mermaid diagram per boundary, posted as a single issue comment for visual approval.

2. A reviewed sub-issue phase (§4/4b/4c). Sub-issues are drafted as scratch files — never GitHub issues — because splitting a file is free and splitting a real issue orphans relationships. They are drafted granular by construction, then reviewed set-first (set-reviewer, sees everything, judges coverage/overlap/minimality/edges) and only then per-draft (sub-issue-reviewer, N in parallel, each blind to its siblings). Issues are created only after approval.

3. A native task spine (§1, §5). Seven phase tasks created at load, plus one task per agent dispatch. Grill rounds stay in the task label rather than spawning tasks. A phase completes when its output is adjudicated, not when an agent replies.

Why the architecture lives in a comment and never in the RFC body

planner.md reads the parent RFC via gh issue view, which returns the body and not comments. Keeping diagrams out of the body means a drifted diagram cannot become the input a later planner slices from — the ADR-as-stale-planning-input failure. The architecture is scaffolding: its consumers are the slicing step and the human eye, and its job ends when the sub-issues are created.

The same reasoning drove a late fix: §3 writes the body's Task breakdown before drafting, and the review rounds exist to change which sub-issues there are — so §4c now reconciles that section after creation. It was the one stale artifact sitting in the surface the planner actually reads.

Why more agents is not just a regress

A second opinion only buys something when its failure mode differs from the first's. Each added reviewer is justified on that basis, and the design leans on countable rules over opinions wherever possible:

Reviewer	Independence	Basis
set-reviewer	Strong	Sees what a per-draft view structurally cannot contain
sub-issue-reviewer	Medium	Fresh context, no sunk cost, and its rule is countable
boundary-reviewer	Weak	Same input and model, only the objective inverted

All three return falsifiable verdicts — every finding must name a specific artifact, and no change is valid and expected, which is what stops a reviewer inventing work to look useful. Verdicts never reach the user raw; they see one changelog line per round. boundary-reviewer is explicitly nominated as the first thing to cut if it reports no change across three RFCs.

Review trail

Every task passed a fresh-context review. Two needed a fix round:

• §2c bypassed its own reviewer on the zero-boundary path. Declaring everything internal was the cheapest way to skip the phase, and that was the case boundary-reviewer was best placed to catch.
• The Architect phase was never explicitly closed on its mainline path. The zero-boundary shortcut was the only worked example of closing it, and it closes at the wrong moment — training the wrong inference.

The whole-branch review then found four more, fixed in one wave: boundary-reviewer's prompt still primed against BOUNDARIES in the empty-ledger case it had just been given; the Task-breakdown staleness above; draft-slug dependencies leaking into published issue bodies; and no changelog line being emitted for the very evidence the deletion rule depends on.

Rulings taken during execution

Full list with cost-if-wrong is in the session transcript. The two worth naming here:

• I ruled that §1's general "completed when adjudicated" convention closed the Architect phase, then reversed it when a reviewer showed the misleading precedent. The explicit close at the approval gate is the result.
• I declined to add explicit completions for the other six phases — only Architect had a precedent pointing at the wrong moment.

Two decisions deliberately left open

• boundary-reviewer is on opus. Arguably it should be sonnet: opus makes it the same model as the orchestrator whose ledger it checks, which is the independence weakness the spec itself names, and it is already the nominated first deletion candidate. Left as a call for the author.
• npm run quality is RED locally — 77 lint errors, all inside .claude/worktrees/vendorable-layer, an untracked sibling worktree that eslint.config.js does not ignore. This branch changed 8 files, none there; every other metric is clean. Unaffected in CI, where that worktree does not exist. The fix is one entry in eslint's ignores, deliberately not taken here because there is uncommitted work in that area.

Not fixed (pre-existing)

§4c notes that "inline quoting breaks in fish" and then shows three inline single-quoted GraphQL examples. Byte-identical to the branch point and outside this change's scope, but it will bite someone.

Test plan

These deliverables are prose prompt documents; nothing here is reached by typecheck, lint or the test suite. Verification was mechanical shell assertion per task (file existence, frontmatter parsing, cross-reference resolution, containment of each hunk) plus fresh-context review of every diff. npm test passes 1/1 and all non-lint gate metrics are clean.

The real acceptance test is the first live run of /grill-rfc on a real RFC.

🤖 Generated with Claude Code

[github-actions]

✅ Quality gate passed

commit 18d4049 · baseline ece7656 · 1 warning(s) · 3 improvement(s) 📈

⚠️ 1 warning(s) — not blocking

	Check	Baseline	Now	Verdict
⚠️	TypeScript errors	—	n/a	collector not configured — see docs/loopwright/quality-gate.md

📊 All metrics

	Check	Baseline	Now	Verdict
✅	Failing tests	0	0	holding
✅	Failing test suites	0	0	holding
⚠️	TypeScript errors	—	n/a	collector not configured — see docs/loopwright/quality-gate.md
✅	ESLint errors	0	0	holding
✅	ESLint warnings	0	0	holding
✅	Critical advisories	0	0	holding
✅	High advisories	0	0	holding
✅	Suppressed advisories	0	0	holding
📈	Line coverage	84.67%	84.69%	improved 0.02%
📈	Branch coverage	78.93%	79.06%	improved 0.13%
✅	Function coverage	89.65%	89.65%	holding
📈	Statement coverage	85.01%	85.03%	improved 0.02%
✅	Duplicated code	0.90%	0.90%	holding
✅	Highest function complexity	14	14	holding
✅	Average function complexity	1.81	1.81	holding
✅	Oversized files	0	0	holding
✅	Skipped tests	0	0	holding
✅	Focused tests (.only)	0	0	holding
✅	Tests with no assertion	0	0	holding
✅	Coverage-ignore hints	1	1	holding
✅	Type suppressions (@ts-ignore etc.)	2	2	holding
✅	Inline eslint-disable	2	2	holding
✅	Empty catch blocks	0	0	holding

📈 This PR improves 3 metric(s). Run node .loopwright/scripts/quality-gate.mjs --update-baseline and commit .loopwright/baseline.json to lock the gain in.

---

_{Generated by .loopwright/scripts/quality-gate.mjs · reproduce locally with node .loopwright/scripts/run-report.mjs --all && node .loopwright/scripts/quality-gate.mjs · full reports are in the workflow artifacts.}

[claude]

Code review

Reviewed the diff (agent prompt files + /grill-rfc skill + docs) against the repo's CLAUDE.md and for internal consistency/logic issues. No CLAUDE.md violations — this PR touches no code, tests, or .loopwright/ files, so none of its rules apply. Four issues found in the prose instructions/docs themselves:

1. §4c's RFC-body reconciliation can silently discard a user's in-flight edit to the issue. .claude/skills/grill-rfc/SKILL.md#L349-L358 pushes gh issue edit <N> --body-file refined.md, reusing the same local refined.md written back in §3 — with no re-fetch of the current issue body first. Between §3's write and this push sits the entire drafting/review/approval pipeline (an unbounded, user-facing window in which the user is actively looking at and commenting on the issue per the §2c/§4c approval gates). Any edit the user makes to the RFC body during that window is silently overwritten, with no diff shown and no gate — unlike §3's first body replacement, which is preceded by an explicit preservation-comment step. Consider re-fetching the current body (or at least the sections outside "Task breakdown") immediately before this push.

2. The --edit-last fallback trigger described in §2c is backwards. .claude/skills/grill-rfc/SKILL.md#L196-L206 says: "--edit-last targets your most recent comment. If the user has commented since, capture the id... and patch it directly instead." Per gh's own docs, --edit-last edits the last comment by the current authenticated user, not literally the issue's last comment — so a comment from the human user in between has no effect on what gets targeted. The real hazard is the opposite case: if the agent's own account posts any other comment after the architecture post, --edit-last will silently overwrite that other comment instead. The suggested PATCH-by-id fallback is sound, but the stated trigger for using it would lead an agent to relax exactly when the risk is nil, and skip the fallback exactly when the risk is real.

3. Two docs added/modified in this same PR contradict each other on whether draft files survive a crashed grill. docs/loopwright/loop-harness.md#L60-L66 says drafts are "session-scoped, so a crashed grill restarts" (implying they're lost), while docs/superpowers/specs/2026-08-17-grill-rfc-architect-and-sub-issue-review-design.md#L375-L381 says "the draft files survive" a crashed session. Since drafts are written to disk as draft-<slug>.md (per the same PR's SKILL.md changes), the spec doc appears correct and loop-harness.md's wording is the one to fix.

4. The new planning doc repeatedly references a file path that doesn't exist. docs/superpowers/plans/2026-08-17-grill-rfc-architect-and-sub-issue-review.md#L879-L897 (Task 7, plus the File Structure table and the spec's Files section) refers to docs/loop-harness.md, but the file this PR actually modifies — and the only one that exists in the repo — is docs/loopwright/loop-harness.md. Minor since this is a completed historical plan doc rather than the operative skill, but the grep/git add assertions in it are wrong as written.

(Also observed: this branch's local checkout had .claude/agents/boundary-reviewer.md, .claude/agents/set-reviewer.md, .claude/agents/sub-issue-reviewer.md present as uncommitted deletions and SKILL.md reverted in the working tree at review time — that's local review-environment state, not part of the PR's committed diff, and not something to act on here.)

[claude]

Code review

Reviewed the diff (three new read-only reviewer agents, the grill-rfc skill rewrite, the pre-commit hook fix, and the new plan/spec docs) against the repo's CLAUDE.md and for bugs/logic issues. Two independent CLAUDE.md-compliance passes found nothing to flag — the diff is almost entirely prose plus a small pre-commit/.gitignore change, and none of it touches the areas CLAUDE.md's "Do not do these" list governs (tests, type suppressions, lint disables, coverage ignores, the baseline). Two independent bug passes converged on the following, which I verified directly:

1. New plan/spec docs point at a file path this same PR doesn't create — docs/loop-harness.md instead of the actual docs/loopwright/loop-harness.md (which this PR itself edits). The plan's own header requires it to be executed "task-by-task" with literal shell assertions (grep -q ... docs/loop-harness.md, git add docs/loop-harness.md); run as written those fail against the real path.

   • loopwright/docs/superpowers/plans/2026-08-17-grill-rfc-architect-and-sub-issue-review.md

     Lines 880 to 897 in 3b997a0

     	### Task 7: Update `docs/loop-harness.md`
     	**Files:**
     	- Modify: `docs/loop-harness.md` — the `RFC issue (label: rfc)` block of the Flow diagram, and the Agents row of the Pieces table.
     	**Interfaces:**
     	- Consumes: everything. Documentation task, runs last.
     	- Produces: nothing.
     	- [ ] **Step 1: Write the failing assertion**
     	```bash
     	cd /Users/samuel/Code/loopwright
     	grep -q 'boundary-reviewer' docs/loop-harness.md \
     	&& grep -q 'set-reviewer' docs/loop-harness.md \
     	&& grep -q 'sub-issue-reviewer' docs/loop-harness.md \
     	&& echo PASS || echo FAIL

   • loopwright/docs/superpowers/specs/2026-08-17-grill-rfc-architect-and-sub-issue-review-design.md

     Lines 369 to 370 in 3b997a0

     	**Modified** — `.claude/skills/grill-rfc/SKILL.md` (new §2c, rewritten §4,
     	task spine in §1). `docs/loop-harness.md` (flow diagram and Pieces table).

2. The task-spine rule in SKILL.md §1 contradicts itself in the same paragraph. It says "One task per agent dispatch, and nowhere else — including the fact-finding sub-agents §2 dispatches, which get a task like any other dispatch," then justifies not giving Grill a task per round because "a task per round is unbounded and would bury the spine." But §2's fact-finding sub-agent dispatches are themselves per-round and unbounded (the grill can dispatch one whenever a frontier question needs an environment fact, every round) — so as written, the rule mandates exactly the unbounded per-round task creation the same sentence warns against.

   • loopwright/.claude/skills/grill-rfc/SKILL.md

     Lines 34 to 42 in 3b997a0

     	**One task per agent dispatch, and nowhere else** — including the
     	fact-finding sub-agents §2 dispatches, which get a task like any other
     	dispatch. Agent fan-outs are the only
     	stretches where the user is in the dark — during the grill they are being
     	asked questions. So `Grill` and `Architect` carry their progress in the task
     	label ("Grilling: round 3, 4 questions open") rather than spawning a task per
     	round: a task per round is unbounded and would bury the spine.

3. The pre-commit fixture exclusion doesn't match a root-level fixtures/ directory. ':(exclude)**/fixtures/**' is added to mirror .loopwright/config.json's sources.ignore: ["**/fixtures/**"]. Verified empirically in this repo (git ls-files "**/CLAUDE.md" returns nothing, while git ls-files "CLAUDE.md" matches) — a leading **/ in a git pathspec requires at least one directory component, so the exclusion only reaches fixtures nested under another directory (e.g. .loopwright/tests/fixtures/), not a top-level fixtures/. Harmless for this repo today (its only fixtures dir is nested), but the hook is vendored into consumer repos via setup.sh, and a consumer with a root-level fixtures/ wouldn't get the exclusion the comment claims.

   • loopwright/.githooks/pre-commit

     Lines 13 to 19 in 3b997a0

     	# Each of these blocks the PR in CI; rejecting them here saves the round-trip.
     	# Fixtures are excluded to mirror `sources.ignore` in .loopwright/config.json:
     	# they exist to give the gate's detectors something to detect, so a fixture
     	# carrying a .skip or an @ts-ignore is the point, not a defect. Without this,
     	# merging main into a branch trips the hook on files nobody edited.
     	added=$(git diff --cached -U0 -- '*.ts' '*.tsx' '*.js' '*.jsx' '*.mjs' ':(exclude)**/fixtures/**' | grep '^+[^+]' || true)
     	if [ -n "$added" ]; then

[SamuelDenani]

All three findings were real and are fixed in a07db82. Verified each before acting rather than taking them on faith — details below.

1. Plan/spec pointed at docs/loop-harness.md — fixed

Correct, and worth naming the cause: the vendorable-layer restructure landed on main while this branch was being built, moving the file to docs/loopwright/loop-harness.md. The branch merged that restructure in, but the plan and spec still carried the pre-move path — including in literal shell assertions that would fail if anyone ran the plan as written.

Replaced all 11 occurrences (10 in the plan, 1 in the spec). Confirmed zero remain and no docs/loopwright/loopwright/ was produced.

2. §1's task rule contradicted itself — fixed, and this one was self-inflicted

Correct, and the diagnosis is exact: the rule mandated per-dispatch tasks for §2's fact-finders while the same paragraph justified denying Grill per-round tasks on the grounds that per-round is unbounded. The fact-finders are per-round and unbounded.

For the record on how it got there: an earlier review flagged this same sentence as ambiguous, I ruled it "not a defect" because §1's rule is global, then folded in a clarifying clause anyway — and the clause is what created the contradiction. Worse outcome than either leaving it alone or fixing it properly.

The paragraph now states the intended semantics: bounded review fan-outs (boundary-reviewer, set-reviewer, the per-draft sub-issue-reviewers) each get a native task because those are the stretches where the user is in the dark; §2's fact-finders fold into the Grill task's label, like the rounds themselves, because during the grill the user is being asked questions rather than waiting.

3. **/fixtures/** missed a root-level fixtures/ — fixed

Verified the pathspec claim independently before changing anything:

git ls-files '**/CLAUDE.md'  -> (empty)
git ls-files 'CLAUDE.md'     -> CLAUDE.md

Confirms a leading **/ in a git pathspec requires at least one directory component. The point about setup.sh vendoring this hook into consumer repos is what makes it matter — .loopwright/config.json's matcher treats **/ as matching zero directories, so the hook was quietly narrower than the comment claimed it mirrored.

Both forms are now excluded, with the comment extended to say why. Three checks, all run:

1. git ls-files 'fixtures/**' ':(exclude)fixtures/**' — sane, exit 0.
2. Nested fixture still excluded — the legacy.jsx case that motivated the original fix: 0.
3. A real .skip outside fixtures/ is still caught: 1.

Check 3 is the one that mattered. The whole risk in this change was weakening the guard rather than narrowing a false positive, so it was verified explicitly rather than assumed.

[claude]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.