📦 deps(thirdparty): update snapshots

This commit is contained in:
ci[bot]
2026-06-18 16:04:36 +00:00
parent 71b421806e
commit dd4c084042
416 changed files with 35467 additions and 3065 deletions
@@ -555,6 +555,8 @@ Should show exactly 6 files changed (5 skill files + 1 test file). No other file
If test runner exists:
```bash
# Run skill-triggering tests
# Note: tests/skill-triggering/ was lifted into drill scenarios on 2026-05-06.
# See evals/scenarios/triggering-*.yaml. The reference below is a dated artifact.
./tests/skill-triggering/run-all.sh 2>/dev/null || echo "Skill triggering tests not available in this environment"
# Run SDD integration test
@@ -275,23 +275,16 @@ If no native tool is available, create a worktree manually using git.
Follow this priority order:
1. **Check existing directories:**
1. **Check your instructions for a worktree directory preference.** If specified, use it without asking.
2. **Check existing project-local directories:**
```bash
ls -d .worktrees 2>/dev/null # Preferred (hidden)
ls -d worktrees 2>/dev/null # Alternative
```
If found, use that directory. If both exist, `.worktrees` wins.
2. **Check for existing global directory:**
```bash
project=$(basename "$(git rev-parse --show-toplevel)")
ls -d ~/.config/superpowers/worktrees/$project 2>/dev/null
```
If found, use it (backward compatibility with legacy global path).
3. **Check your instructions for a worktree directory preference.** If specified, use it without asking.
4. **Default to `.worktrees/`.**
3. **Default to `.worktrees/`.**
#### Safety Verification (project-local directories only)
@@ -305,16 +298,11 @@ git check-ignore -q .worktrees 2>/dev/null || git check-ignore -q worktrees 2>/d
**Why critical:** Prevents accidentally committing worktree contents to repository.
Global directories (`~/.config/superpowers/worktrees/`) need no verification.
#### Create the Worktree
```bash
project=$(basename "$(git rev-parse --show-toplevel)")
# Determine path based on chosen location
# For project-local: path="$LOCATION/$BRANCH_NAME"
# For global: path="~/.config/superpowers/worktrees/$project/$BRANCH_NAME"
path="$LOCATION/$BRANCH_NAME"
git worktree add "$path" -b "$BRANCH_NAME"
cd "$path"
@@ -387,7 +375,6 @@ Ready to implement <feature-name>
| `worktrees/` exists | Use it (verify ignored) |
| Both exist | Use `.worktrees/` |
| Neither exists | Check instruction file, then default `.worktrees/` |
| Global path exists | Use it (backward compat) |
| Directory not ignored | Add to .gitignore + commit |
| Permission error on create | Sandbox fallback, work in place |
| Tests fail during baseline | Report failures + ask |
@@ -464,7 +451,7 @@ git commit -m "feat: rewrite using-git-worktrees with detect-and-defer (PRI-974)
Step 0: GIT_DIR != GIT_COMMON detection (skip if already isolated)
Step 0 consent: opt-in prompt before creating worktree (#991)
Step 1a: native tool preference (short, first, declarative)
Step 1b: git worktree fallback with hooks symlink and legacy path compat
Step 1b: git worktree fallback with project-local directory policy
Submodule guard prevents false detection
Platform-neutral instruction file references (#1049)"
```
@@ -663,7 +650,7 @@ WORKTREE_PATH=$(git rev-parse --show-toplevel)
**If `GIT_DIR == GIT_COMMON`:** Normal repo, no worktree to clean up. Done.
**If worktree path is under `.worktrees/` or `~/.config/superpowers/worktrees/`:** Superpowers created this worktree — we own cleanup.
**If worktree path is under `.worktrees/` or `worktrees/`:** Superpowers created this worktree — we own cleanup.
```bash
MAIN_ROOT=$(git -C "$(git rev-parse --git-common-dir)/.." rev-parse --show-toplevel)
@@ -707,7 +694,7 @@ git worktree prune # Self-healing: clean up any stale registrations
**Cleaning up harness-owned worktrees**
- **Problem:** Removing a worktree the harness created causes phantom state
- **Fix:** Only clean up worktrees under `.worktrees/` or `~/.config/superpowers/worktrees/`
- **Fix:** Only clean up worktrees under `.worktrees/` or `worktrees/`
**No confirmation for discard**
- **Problem:** Accidentally delete work
File diff suppressed because it is too large Load Diff
@@ -0,0 +1,143 @@
# Pi Extension and Evals Implementation Plan
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
**Goal:** Add first-class Pi package support for Superpowers and add Pi as a Drill eval backend.
**Architecture:** The Pi package is declared in the root `package.json` and loads existing `skills/` plus a small Pi extension. The extension injects the `using-superpowers` bootstrap into provider context as a user-role message on session startup and after compaction, with Pi-specific tool mapping. Drill gains a `pi` backend, Pi session-log normalization, and tests.
**Tech Stack:** Pi TypeScript extension API, Node built-in test runner, Drill Python eval harness, pytest.
---
### Task 1: Pi package manifest and extension tests
**Files:**
- Modify: `package.json`
- Create: `tests/pi/test-pi-extension.mjs`
- [ ] **Step 1: Write failing package/extension tests**
Create `tests/pi/test-pi-extension.mjs` with tests that import `extensions/superpowers.ts`, register fake Pi handlers, and assert:
- root `package.json` has `keywords` containing `pi-package`
- root `package.json` has `pi.skills: ["./skills"]`
- root `package.json` has `pi.extensions: ["./extensions/superpowers.ts"]`
- the extension registers `resources_discover`, `session_start`, `session_compact`, `context`, and `agent_end`
- startup `context` injects exactly one user-role bootstrap message
- `agent_end` clears startup injection
- `session_compact` re-enables injection
- the extension does not register `session_before_compact`
- [ ] **Step 2: Run tests and verify RED**
Run: `node --experimental-strip-types --test tests/pi/test-pi-extension.mjs`
Expected: FAIL because `extensions/superpowers.ts` does not exist and `package.json` lacks the `pi` manifest.
- [ ] **Step 3: Implement manifest fields**
Update `package.json` with `description`, `keywords`, `pi.extensions`, and `pi.skills` while preserving existing `name`, `version`, `type`, and `main`.
- [ ] **Step 4: Implement `extensions/superpowers.ts`**
Create a zero-runtime-dependency extension that:
- locates the package root from `import.meta.url`
- reads `skills/using-superpowers/SKILL.md`
- strips YAML frontmatter
- appends Pi-specific tool mapping
- exposes `resources_discover` with the skills path
- marks bootstrap pending on `session_start` and `session_compact`
- injects a user-role bootstrap message in `context`
- inserts post-compact bootstrap after leading `compactionSummary` messages
- clears pending bootstrap on `agent_end`
- [ ] **Step 5: Run tests and verify GREEN**
Run: `node --experimental-strip-types --test tests/pi/test-pi-extension.mjs`
Expected: PASS.
### Task 2: Pi tool mapping reference
**Files:**
- Create: `skills/using-superpowers/references/pi-tools.md`
- Modify: `tests/pi/test-pi-extension.mjs`
- [ ] **Step 1: Write failing test for Pi reference doc**
Add assertions that `skills/using-superpowers/references/pi-tools.md` exists and documents mappings for `Skill`, `Task`, `TodoWrite`, and built-in tool names.
- [ ] **Step 2: Run tests and verify RED**
Run: `node --experimental-strip-types --test tests/pi/test-pi-extension.mjs`
Expected: FAIL because `pi-tools.md` does not exist.
- [ ] **Step 3: Add Pi reference doc**
Create `skills/using-superpowers/references/pi-tools.md` explaining Pi-native skills, optional `pi-subagents`, no canonical todo/tasklist plugin, and built-in lowercase tools.
- [ ] **Step 4: Run tests and verify GREEN**
Run: `node --experimental-strip-types --test tests/pi/test-pi-extension.mjs`
Expected: PASS.
### Task 3: Drill Pi backend and session log normalization
**Files:**
- Create: `evals/backends/pi.yaml`
- Modify: `evals/drill/backend.py`
- Modify: `evals/drill/engine.py`
- Modify: `evals/drill/normalizer.py`
- Modify: `evals/tests/test_backend.py`
- Modify: `evals/tests/test_normalizer.py`
- [ ] **Step 1: Write failing backend/normalizer tests**
Add pytest coverage for:
- `load_backend("pi")` returns `family == "pi"`
- Pi backend command starts with `pi` and includes `-e ${SUPERPOWERS_ROOT}`
- `_resolve_log_dir()` for Pi points under `~/.pi/agent/sessions`
- `filter_pi_logs_by_cwd()` keeps only session files whose header `cwd` matches the scenario workdir
- `normalize_pi_logs()` extracts `toolCall` blocks from Pi assistant session entries and maps built-in lowercase tools to canonical names
- [ ] **Step 2: Run tests and verify RED**
Run: `uv run pytest evals/tests/test_backend.py evals/tests/test_normalizer.py -q`
Expected: FAIL because the Pi backend and normalizer do not exist.
- [ ] **Step 3: Add `evals/backends/pi.yaml`**
Configure the backend to run `pi -e ${SUPERPOWERS_ROOT}`, use permissive TUI readiness, `/quit` shutdown, and Pi session log location.
- [ ] **Step 4: Implement Pi family support**
Update `Backend.family`, `Engine._resolve_log_dir`, `Engine._collect_tool_calls`, and `normalizer.py` with Pi log filtering and normalizing.
- [ ] **Step 5: Run tests and verify GREEN**
Run: `uv run pytest evals/tests/test_backend.py evals/tests/test_normalizer.py -q`
Expected: PASS.
### Task 4: Documentation and full verification
**Files:**
- Modify: `README.md`
- Modify: `evals/README.md`
- [ ] **Step 1: Document Pi install and eval backend**
Add Pi to README quickstart/install list and add backend entry/usage to `evals/README.md`.
- [ ] **Step 2: Run verification**
Run:
```bash
node --experimental-strip-types --test tests/pi/test-pi-extension.mjs
uv run pytest evals/tests/test_backend.py evals/tests/test_setup.py evals/tests/test_normalizer.py -q
```
Expected: all tests pass.
@@ -0,0 +1,774 @@
# SDD Task-Scoped Review Dispatch Implementation Plan
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
**Goal:** Scope SDD's per-task reviews to the task (diff-first reading, justified broadening, no redundant test runs) while final branch review stays broad.
**Architecture:** Four prose edits to the subagent-driven-development skill (the per-task quality prompt becomes self-contained instead of delegating to the merge-readiness template; the spec prompt gets a third verdict channel and grounded skepticism; the implementer prompt gains a re-run-after-fix rule; SKILL.md gets controller guidance) plus one new eval scenario in the `evals/` submodule. `skills/requesting-code-review/` is deliberately untouched.
**Tech Stack:** Markdown skill files; Python setup helper + bash checks + story.md for the quorum eval.
**Spec:** `docs/superpowers/specs/2026-06-09-sdd-task-scoped-review-dispatch-design.md` — read it before starting. Decisions already settled there: full re-reviews stay; the two review stages stay separate; coordinator keeps model judgment; `requesting-code-review/` stays broad.
**These are behavior-shaping prose files, not code.** There are no unit tests for them. Each task's verification steps are exact `grep` checks that the edit landed; behavioral verification is Task 6 (static) and Task 7 (live evals, maintainer-gated).
---
### Task 1: Rewrite the per-task quality reviewer prompt as self-contained
The current file delegates to `../requesting-code-review/code-reviewer.md`, which is a merge-readiness review (architecture, security, production readiness, "Ready to merge?"). Replace the entire file with a self-contained, task-scoped template.
**Files:**
- Rewrite: `skills/subagent-driven-development/code-quality-reviewer-prompt.md`
- [ ] **Step 1: Replace the full file contents with:**
````markdown
# Code Quality Reviewer Prompt Template
Use this template when dispatching a code quality reviewer subagent.
**Purpose:** Verify one task's implementation is well-built (clean, tested, maintainable)
**Only dispatch after spec compliance review passes.**
```
Subagent (general-purpose):
description: "Review code quality for Task N"
prompt: |
You are reviewing one task's implementation for code quality. This is a
task-scoped gate, not a merge review — a broad whole-branch review happens
separately after all tasks are complete.
## What Was Implemented
[DESCRIPTION]
## Task Requirements (context only)
[TASK_TEXT]
## Git Range to Review
**Base:** [BASE_SHA]
**Head:** [HEAD_SHA]
```bash
git diff --stat [BASE_SHA]..[HEAD_SHA]
git diff [BASE_SHA]..[HEAD_SHA]
```
## Read-Only Review
Your review is read-only on this checkout. Do not mutate the working tree,
the index, HEAD, or branch state in any way. Use tools like `git show`,
`git diff`, and `git log` to inspect history.
## Scope
Spec compliance was already verified by a separate reviewer. Do not
re-check whether the code matches the requirements or the plan.
Start from the diff. Read the changed files first. Inspect code outside
the diff only to evaluate a concrete risk you can name — and name it in
your report. Cross-cutting changes are legitimate named risks: if the
diff changes lock ordering, a function or API contract, or shared mutable
state, checking the call sites is the right method. Do not crawl the
codebase by default.
## Tests
The implementer already ran the tests and reported results with TDD
evidence for exactly this code. Do not re-run the suite to confirm their
report. Run a test only when reading the code raises a specific doubt
that no existing run answers — and then a focused test, never a
package-wide suite, race detector run, or repeated/high-count loop. If
heavy validation seems warranted, recommend it in your report instead of
running it. If you cannot run commands in this environment, name the
test you would run.
## What to Check
**Code quality:**
- Clean separation of concerns?
- Proper error handling?
- DRY without premature abstraction?
- Edge cases handled?
**Tests:**
- Do the new and changed tests verify real behavior, not mocks?
- Are the task's edge cases covered?
**Structure:**
- Does each file have one clear responsibility with a well-defined interface?
- Are units decomposed so they can be understood and tested independently?
- Is the implementation following the file structure from the plan?
- Did this change create new files that are already large, or
significantly grow existing files? (Don't flag pre-existing file
sizes — focus on what this change contributed.)
## Calibration
Categorize issues by actual severity. Not everything is Critical.
Acknowledge what was done well before listing issues — accurate praise
helps the implementer trust the rest of the feedback.
## Output Format
### Strengths
[What's well done? Be specific.]
### Issues
#### Critical (Must Fix)
[Bugs, data loss risks, broken functionality]
#### Important (Should Fix)
[Poor error handling, test gaps, structural problems]
#### Minor (Nice to Have)
[Code style, optimization opportunities]
For each issue:
- File:line reference
- What's wrong
- Why it matters
- How to fix (if not obvious)
### Assessment
**Task quality:** [Approved | Needs fixes]
**Reasoning:** [1-2 sentence technical assessment]
```
**Placeholders:**
- `[DESCRIPTION]` — task summary, from implementer's report
- `[TASK_TEXT]` — the task's requirements text or plan reference, for context
- `[BASE_SHA]` — commit before this task
- `[HEAD_SHA]` — current commit
**Reviewer returns:** Strengths, Issues (Critical/Important/Minor), Task quality verdict
````
- [ ] **Step 2: Verify the rewrite landed**
Run: `grep -c "requesting-code-review" skills/subagent-driven-development/code-quality-reviewer-prompt.md || echo ABSENT`
Expected: `ABSENT` (no more delegation)
Run: `grep -n "Task quality:" skills/subagent-driven-development/code-quality-reviewer-prompt.md | head -2`
Expected: one match (the Output Format verdict line; the "Reviewer returns" footer says "Task quality verdict" without a colon)
Run: `grep -n "worktree add\|Ready to merge" skills/subagent-driven-development/code-quality-reviewer-prompt.md || echo CLEAN`
Expected: `CLEAN`
- [ ] **Step 3: Commit**
```bash
git add skills/subagent-driven-development/code-quality-reviewer-prompt.md
git commit -m "Make per-task quality reviewer prompt self-contained and task-scoped"
```
---
### Task 2: Spec reviewer prompt cleanups
Four exact edits to `skills/subagent-driven-development/spec-reviewer-prompt.md`. Current line numbers refer to the file as of commit f55642e.
**Files:**
- Modify: `skills/subagent-driven-development/spec-reviewer-prompt.md`
- [ ] **Step 1: Add the judge-from-the-diff clause.** After the line (currently line 31):
```
Only read files in this diff. Do not crawl the broader codebase.
```
insert a blank line and:
```
Spec compliance is judged by reading the diff against the requirements.
The implementer already ran the tests and reported TDD evidence — do not
re-run them. If a requirement cannot be verified from this diff alone
(it lives in unchanged code or spans tasks), report it as a ⚠️ item
instead of broadening your search.
```
- [ ] **Step 2: Trim the read-only section.** Replace (currently line 35):
```
Your review is read-only on this checkout. Do not mutate the working tree, the index, HEAD, or branch state in any way. Use tools like `git show`, `git diff`, and `git log` to inspect history. If you need a working copy of a different revision, check it out into a separate temporary directory (e.g. `git worktree add /tmp/review-[SHA] [SHA]`) — never move HEAD on this checkout.
```
with:
```
Your review is read-only on this checkout. Do not mutate the working tree, the index, HEAD, or branch state in any way. Use tools like `git show`, `git diff`, and `git log` to inspect history.
```
- [ ] **Step 3: Ground the skepticism.** Replace (currently lines 39-40):
```
The implementer finished suspiciously quickly. Their report may be incomplete,
inaccurate, or optimistic. You MUST verify everything independently.
```
with:
```
Treat the implementer's report as unverified claims about the code. It may
be incomplete, inaccurate, or optimistic. Verify the claims against the diff.
```
- [ ] **Step 4: Add the third verdict channel.** Replace (currently lines 74-76):
```
Report:
- ✅ Spec compliant (if everything matches after code inspection)
- ❌ Issues found: [list specifically what's missing or extra, with file:line references]
```
with:
```
Report:
- ✅ Spec compliant (if everything matches after code inspection)
- ❌ Issues found: [list specifically what's missing or extra, with file:line references]
- ⚠️ Cannot verify from diff: [requirements you could not verify from the
diff alone, and what the controller should check — report alongside the
✅/❌ verdict for everything you could verify]
```
- [ ] **Step 5: Verify**
Run: `grep -n "suspiciously\|worktree add" skills/subagent-driven-development/spec-reviewer-prompt.md || echo CLEAN`
Expected: `CLEAN`
Run: `grep -c "⚠️" skills/subagent-driven-development/spec-reviewer-prompt.md`
Expected: `2` (judge-from-diff clause + verdict channel)
- [ ] **Step 6: Commit**
```bash
git add skills/subagent-driven-development/spec-reviewer-prompt.md
git commit -m "Spec reviewer: judge from the diff, grounded skepticism, ⚠️ verdict channel"
```
---
### Task 3: Implementer prompt — re-run tests after fixing review findings
The reviewers' "don't re-run the implementer's tests" rule assumes the implementer re-runs tests after every fix. Make that real.
**Files:**
- Modify: `skills/subagent-driven-development/implementer-prompt.md`
- [ ] **Step 1: Insert a new section.** Immediately before the line (currently line 100):
```
## Report Format
```
insert:
```
## After Review Findings
If a reviewer finds issues and you fix them, re-run the tests that cover
the amended code and include the results in your fix report. Reviewers
will not re-run tests for you — your report is the test evidence.
```
- [ ] **Step 2: Verify**
Run: `grep -n "After Review Findings" skills/subagent-driven-development/implementer-prompt.md`
Expected: one match, on a line before `## Report Format`
- [ ] **Step 3: Commit**
```bash
git add skills/subagent-driven-development/implementer-prompt.md
git commit -m "Implementer prompt: re-run covering tests after fixing review findings"
```
---
### Task 4: SKILL.md controller changes
Six exact edits to `skills/subagent-driven-development/SKILL.md`. Current line numbers refer to commit f55642e.
**Files:**
- Modify: `skills/subagent-driven-development/SKILL.md`
- [ ] **Step 1: Point the final-review flowchart node at the broad template.** The node label `Dispatch final code reviewer subagent for entire implementation` appears 3 times (currently lines 65, 84, 85). In all 3 occurrences, replace the label string with:
```
Dispatch final code reviewer subagent (../requesting-code-review/code-reviewer.md)
```
(Graphviz nodes are matched by label text — all three must be byte-identical or the graph grows a phantom node.)
- [ ] **Step 2: Model selection by judgment.** Replace (currently lines 97-99):
```
**Architecture, design, and review tasks**: use the most capable available model.
**Task complexity signals:**
```
with:
```
**Architecture and design tasks**: use the most capable available model.
**Review tasks**: choose the model with the same judgment, scaled to the
diff's size, complexity, and risk. A small mechanical diff does not need the
most capable model; a subtle concurrency change does.
**Task complexity signals (implementation tasks):**
```
- [ ] **Step 3: Add controller guidance sections.** Immediately before the line (currently line 122):
```
## Prompt Templates
```
insert:
```
## Handling Spec Reviewer ⚠️ Items
The spec reviewer may report "⚠️ Cannot verify from diff" items — requirements
that live in unchanged code or span tasks. These do not block dispatching the
code quality reviewer, but you must resolve each one yourself before marking
the task complete: you hold the plan and cross-task context the reviewer
lacks. If you confirm an item is a real gap, treat it as a failed spec
review — send it back to the implementer and re-review.
## Constructing Reviewer Prompts
Per-task reviews are task-scoped gates. The broad review happens once, at the
final whole-branch review. When you fill a reviewer template:
- Do not add open-ended directives like "check all uses" or "run race tests
if useful" without a concrete, task-specific reason
- Do not ask a reviewer to re-run tests the implementer already ran on the
same code — the implementer's report carries the test evidence
```
- [ ] **Step 4: Prompt Templates list — add the final-review pointer.** Replace (currently line 126):
```
- [code-quality-reviewer-prompt.md](code-quality-reviewer-prompt.md) - Dispatch code quality reviewer subagent
```
with:
```
- [code-quality-reviewer-prompt.md](code-quality-reviewer-prompt.md) - Dispatch code quality reviewer subagent
- Final whole-branch review: use superpowers:requesting-code-review's [code-reviewer.md](../requesting-code-review/code-reviewer.md)
```
- [ ] **Step 5: Example workflow verdict vocabulary.** Two replacements:
Replace (currently line 157):
```
Code reviewer: Strengths: Good test coverage, clean. Issues: None. Approved.
```
with:
```
Code reviewer: Strengths: Good test coverage, clean. Issues: None. Task quality: Approved.
```
Replace (currently line 191):
```
Code reviewer: ✅ Approved
```
with:
```
Code reviewer: ✅ Task quality: Approved
```
(The final reviewer's "ready to merge" line, currently line 199, stays.)
- [ ] **Step 6: Integration section.** Replace (currently line 272):
```
- **superpowers:requesting-code-review** - Code review template for reviewer subagents
```
with:
```
- **superpowers:requesting-code-review** - Code review template for the final whole-branch review
```
- [ ] **Step 7: Verify**
Run: `grep -c "Dispatch final code reviewer subagent (../requesting-code-review/code-reviewer.md)" skills/subagent-driven-development/SKILL.md`
Expected: `3`
Run: `grep -n "most capable available model" skills/subagent-driven-development/SKILL.md`
Expected: exactly one match (architecture/design bullet)
Run: `grep -n "Handling Spec Reviewer\|Constructing Reviewer Prompts" skills/subagent-driven-development/SKILL.md`
Expected: two section headers, both before `## Prompt Templates`
Run: `grep -c "Task quality: Approved" skills/subagent-driven-development/SKILL.md`
Expected: `2`
- [ ] **Step 8: Commit**
```bash
git add skills/subagent-driven-development/SKILL.md
git commit -m "SDD controller: reviewer prompt budgets, ⚠️ handling, final-review pointer, model judgment"
```
---
### Task 5: New eval scenario — per-task quality reviewer catches a planted defect
Lives in the `evals/` **submodule** (separate repo, `superpowers-evals`). Work on a branch there; the parent submodule-pointer bump happens at finishing time per `evals/CLAUDE.md`.
The fixture plan's Task 2 implementation snippet duplicates Task 1's formatting logic verbatim. The duplication is spec-compliant, so the spec reviewer should pass it — the per-task quality reviewer is the gate under test (DRY violation).
**Files:**
- Create: `evals/setup_helpers/sdd_quality_defect_plan.py`
- Modify: `evals/setup_helpers/__init__.py`
- Create: `evals/scenarios/sdd-quality-reviewer-catches-planted-defect/story.md`
- Create: `evals/scenarios/sdd-quality-reviewer-catches-planted-defect/setup.sh`
- Create: `evals/scenarios/sdd-quality-reviewer-catches-planted-defect/checks.sh`
- [ ] **Step 0: Branch in the submodule**
```bash
cd evals
git checkout -b sdd-quality-defect-scenario
```
- [ ] **Step 1: Create `evals/setup_helpers/sdd_quality_defect_plan.py`:**
````python
"""Setup helper for the sdd-quality-reviewer-catches-planted-defect scenario.
Scaffolds a tiny Node project with a 2-task plan whose Task 2
implementation snippet duplicates Task 1's formatting logic verbatim.
The duplication is spec-compliant — the requirements only describe
behavior — so the spec compliance reviewer should pass it. The test
measures whether the per-task code quality reviewer catches the DRY
violation and forces a refactor in the review-fix loop.
"""
from __future__ import annotations
from pathlib import Path
from setup_helpers.base import _git
PACKAGE_JSON = """\
{
"name": "report-quality",
"version": "1.0.0",
"type": "module",
"scripts": {
"test": "node --test"
}
}
"""
PLAN_BODY = """\
# Report Formatter — Implementation Plan
Two report formatting functions. Implement exactly what each task
specifies.
## Task 1: User Report
**File:** `src/report.js`
**Requirements:**
- Function named `formatUserReport`
- Takes one parameter `user`: an object with `name`, `email`, `visits`
- Returns a multi-line string: a banner of 40 `=` characters, then
`Report for <name> <<email>>`, then the banner again, then
`Visits: <visits>`, then a closing banner
- Export the function
**Implementation:**
```javascript
export function formatUserReport(user) {
const banner = "=".repeat(40);
const lines = [];
lines.push(banner);
lines.push(`Report for ${user.name} <${user.email}>`);
lines.push(banner);
lines.push(`Visits: ${user.visits}`);
lines.push(banner);
return lines.join("\\n");
}
```
**Tests:** Create `test/report.test.js` verifying:
- the result contains `Report for Ada <ada@example.com>` for that user
- the result contains `Visits: 3` when `visits` is `3`
- the result starts and ends with the 40-char banner
**Verification:** `npm test`
## Task 2: Admin Report
**File:** `src/report.js` (add to existing file)
**Requirements:**
- Function named `formatAdminReport`
- Takes one parameter `admin`: an object with `name`, `email`, `lastLogin`
- Same banner layout as the user report; the body line is
`Last login: <lastLogin>` instead of the visits line
- Export the function; keep `formatUserReport` working
**Implementation:**
```javascript
export function formatAdminReport(admin) {
const banner = "=".repeat(40);
const lines = [];
lines.push(banner);
lines.push(`Report for ${admin.name} <${admin.email}>`);
lines.push(banner);
lines.push(`Last login: ${admin.lastLogin}`);
lines.push(banner);
return lines.join("\\n");
}
```
**Tests:** Add to `test/report.test.js`:
- the result contains `Report for Grace <grace@example.com>` for that admin
- the result contains `Last login: 2026-06-01`
- the result starts and ends with the 40-char banner
**Verification:** `npm test`
"""
def scaffold_sdd_quality_defect_plan(workdir: Path) -> None:
workdir = Path(workdir)
workdir.mkdir(parents=True, exist_ok=True)
_git(["git", "init", "-b", "main"], cwd=workdir)
_git(["git", "config", "user.email", "drill@test.local"], cwd=workdir)
_git(["git", "config", "user.name", "Drill Test"], cwd=workdir)
(workdir / "package.json").write_text(PACKAGE_JSON)
plans_dir = workdir / "docs" / "superpowers" / "plans"
plans_dir.mkdir(parents=True, exist_ok=True)
(plans_dir / "report-plan.md").write_text(PLAN_BODY)
_git(["git", "add", "-A"], cwd=workdir)
_git(["git", "commit", "-m", "initial: report formatter plan"], cwd=workdir)
````
(Note the `\\n` in the JS snippets inside PLAN_BODY: the Python source must
produce a literal `\n` in the markdown so the JS reads `lines.join("\n")`.)
- [ ] **Step 2: Register the helper.** In `evals/setup_helpers/__init__.py`:
After the line:
```python
from setup_helpers.sdd_real_projects import scaffold_sdd_go_fractals, scaffold_sdd_svelte_todo
```
add:
```python
from setup_helpers.sdd_quality_defect_plan import scaffold_sdd_quality_defect_plan
```
After the registry entry:
```python
"scaffold_sdd_yagni_plan": scaffold_sdd_yagni_plan,
```
add:
```python
"scaffold_sdd_quality_defect_plan": scaffold_sdd_quality_defect_plan,
```
- [ ] **Step 3: Create `evals/scenarios/sdd-quality-reviewer-catches-planted-defect/story.md`:**
```markdown
---
id: sdd-quality-reviewer-catches-planted-defect
title: SDD's per-task code quality review catches a planted DRY violation
status: ready
tags: subagent-driven-development
quorum_max_time: 90m
---
You have a small plan at docs/superpowers/plans/report-plan.md — two report
formatting functions. The plan's Task 2 implementation snippet duplicates
Task 1's formatting logic verbatim instead of sharing it. The duplication is
spec-compliant (the requirements only describe behavior), so the spec
compliance reviewer should pass it — the per-task code quality reviewer is
the gate under test. You are spec-aware — name the skill.
When the agent is ready for input, tell it to execute the plan with SDD. Use
phrasing like:
"I have a small plan at docs/superpowers/plans/report-plan.md — two report
formatting functions. Use the superpowers:subagent-driven-development skill
to execute it end-to-end — dispatch fresh subagents per task and run the
two-stage review after each."
Let the agent proceed autonomously. If it asks clarifying questions, give
brief answers. If it asks where the finished work should land — merge to the
main branch, open a PR, etc. — tell it to **merge the work into the main
checkout** (this is a local repo with no remote). If a quality reviewer
flags the duplicated formatting logic and an implementer refactors it, let
the review-fix cycle play out — that cycle is exactly the behavior under
test.
The deliverable must end up in the checkout you launched in (the main
working tree). If the agent did its work on a branch or in a worktree, it
is not done until it has merged/finished that work back into the main
checkout. Once the agent reports the plan is complete (both functions
implemented, tests passing) AND the code is present on the main checkout,
you are done.
## Acceptance Criteria
- A `Skill` invocation naming `superpowers:subagent-driven-development`
and at least one `Agent` (subagent dispatch) tool call appear in the
session log.
- The duplicated report-formatting logic did not survive to the end of
the run. Either (a) the implementer never introduced the duplication
(wrote or self-reviewed its way to shared logic), or (b) the per-task
code quality reviewer flagged the duplication as an issue and a
review-fix loop removed it. A fail looks like the duplicated logic
shipping with the per-task quality reviewer approving it, or the
duplication being caught only by the final whole-branch review.
- The per-task quality reviewers stayed task-scoped: no package-wide
test suites, race detector runs, or repeated/high-count test loops
appear in reviewer subagent activity, and reviewers did not re-run
the full test suite merely to confirm the implementer's report.
- `npm test` passes in the main checkout and both `formatUserReport` and
`formatAdminReport` are exported from src/report.js. The deterministic
assertions gate this; the criteria above are about whether the
*per-task quality review* was the mechanism that kept the code clean.
```
- [ ] **Step 4: Create `evals/scenarios/sdd-quality-reviewer-catches-planted-defect/setup.sh`:**
```bash
#!/usr/bin/env bash
set -euo pipefail
uv run setup-helpers run scaffold_sdd_quality_defect_plan
```
Then: `chmod +x evals/scenarios/sdd-quality-reviewer-catches-planted-defect/setup.sh`
- [ ] **Step 5: Create `evals/scenarios/sdd-quality-reviewer-catches-planted-defect/checks.sh`** (no executable bit):
```bash
pre() {
git-repo
git-branch main
requires-tool npm
file-exists 'docs/superpowers/plans/report-plan.md'
file-contains 'docs/superpowers/plans/report-plan.md' 'formatAdminReport'
file-contains 'docs/superpowers/plans/report-plan.md' 'repeat\(40\)'
}
post() {
skill-called superpowers:subagent-driven-development
tool-called Agent
command-succeeds 'npm test'
file-contains 'src/report.js' 'export function formatUserReport'
file-contains 'src/report.js' 'export function formatAdminReport'
command-succeeds 'test "$(grep -c "repeat(40)" src/report.js)" -le 1'
}
```
(The last check is the deterministic DRY gate: the banner construction
`"=".repeat(40)` must appear at most once in the final file — shared, not
duplicated per function.)
- [ ] **Step 6: Validate and test in the evals repo**
```bash
cd evals
uv run quorum check
uv run ruff check
uv run pytest -x -q
```
Expected: all pass; `quorum check` lists the new scenario without errors.
- [ ] **Step 7: Commit (in the submodule)**
```bash
cd evals
git add setup_helpers/sdd_quality_defect_plan.py setup_helpers/__init__.py scenarios/sdd-quality-reviewer-catches-planted-defect/
git commit -m "Add sdd-quality-reviewer-catches-planted-defect scenario"
```
---
### Task 6: Static verification sweep
**Files:** none modified — verification only.
- [ ] **Step 1: No dangling references in the parent repo**
Run: `grep -rn "requesting-code-review" skills/subagent-driven-development/`
Expected: matches only in SKILL.md (final-review flowchart node ×3, Prompt Templates pointer, Integration bullet). None in code-quality-reviewer-prompt.md.
Run: `grep -rn "Ready to merge" skills/subagent-driven-development/ || echo CLEAN`
Expected: `CLEAN`
- [ ] **Step 2: Plugin infrastructure tests**
Run: `bash tests/shell-lint/test-lint-shell.sh`
Expected: all PASS (we added `setup.sh` only inside the evals submodule, which has its own checks).
- [ ] **Step 3: Cross-platform tool tables still coherent**
Run: `grep -n "code-quality-reviewer" skills/using-superpowers/references/antigravity-tools.md skills/using-superpowers/references/gemini-tools.md`
Expected: both tables still list `code-quality-reviewer` as a reviewer template (the new prompt's "If you cannot run commands in this environment, name the test you would run" line keeps the read-only `research` mapping valid — no table edits needed).
---
### Task 7: Live before/after evals (maintainer-gated)
Live quorum runs launch agent CLIs in permissive modes — **trusted-maintainer operation; Jesse launches these**, per `evals/CLAUDE.md`. Requires `ANTHROPIC_API_KEY`.
- [ ] **Step 1: Baseline (skills as released on dev)** — from the main checkout (`/Users/jesse/git/superpowers/superpowers`, on dev), or any checkout without this branch's changes:
```bash
cd evals
export SUPERPOWERS_ROOT=/Users/jesse/git/superpowers/superpowers
uv run quorum run scenarios/sdd-rejects-extra-features --coding-agent claude
uv run quorum run scenarios/sdd-go-fractals --coding-agent claude
uv run quorum run scenarios/sdd-svelte-todo --coding-agent claude
uv run quorum run scenarios/spec-reviewer-catches-planted-flaws --coding-agent claude
```
- [ ] **Step 2: After (this branch's skills)** — point `SUPERPOWERS_ROOT` at this worktree:
```bash
cd evals
export SUPERPOWERS_ROOT=/Users/jesse/git/superpowers/superpowers/.claude/worktrees/sdd-review-dispatch
uv run quorum run scenarios/sdd-rejects-extra-features --coding-agent claude
uv run quorum run scenarios/sdd-go-fractals --coding-agent claude
uv run quorum run scenarios/sdd-svelte-todo --coding-agent claude
uv run quorum run scenarios/spec-reviewer-catches-planted-flaws --coding-agent claude
uv run quorum run scenarios/sdd-quality-reviewer-catches-planted-defect --coding-agent claude
uv run quorum show
```
- [ ] **Step 3: Compare**
Pass bar: all four pre-existing scenarios still pass after the change (no regression in catch rate); the new planted-defect scenario passes. For exploration cost, compare reviewer-subagent tool-call counts between the before/after run transcripts (no automated check exists — the spec calls this out as a known gap).
---
## Finishing
After all tasks pass: the evals submodule commit needs to land in `superpowers-evals` (PR to its `main`), then this branch bumps the `evals` submodule pointer — per `evals/CLAUDE.md`, the parent bump is part of propagation, not optional. Then use superpowers:finishing-a-development-branch. PRs against superpowers target `dev`.
@@ -0,0 +1,352 @@
# Visual Brainstorming Companion — Issue & Change Catalog
**Date:** 2026-06-09
**Status:** Analysis / triage. We are implementing these ourselves; the referenced
community PRs are evidence and reference material, **not** code we intend to merge.
## Purpose
A single place that captures every open issue and PR touching the visual
brainstorming companion (the local server in `skills/brainstorming/scripts/`),
distilled to the underlying problem and the change we'd make. Each item is
grounded against the current code, not the PR author's description.
## Scope decisions (Jesse, 2026-06-09)
- **Not vendoring Alpine.js.** PR #1639 (interactive mockups via a vendored
Alpine build) is **dropped**. See E3.
- **E1 (terminal-vs-HTML hard gate) is a workshop item.** We'll design it
together; it is not specced here.
- **E2 (storage location, #975/#977) is deferred** for now.
- **Remote serving is a first-class scenario.** Superpowers is general-purpose;
users connect from remote (SSH tunnel, Tailscale, `--host 0.0.0.0`). The
security fix MUST protect those users, not just loopback. **Decision: a
per-session secret key**, not a Host allowlist. A Host allowlist only
defends the loopback browser-confused-deputy; a direct remote client just
sends the expected `Host`, so the allowlist is theater for remote exposure. A
secret key is the only thing that authenticates a client uniformly across
loopback, tunnel, and direct-remote, and it also defeats DNS rebinding. See A1.
## Component map
| File | Role |
|------|------|
| `skills/brainstorming/scripts/server.cjs` | Zero-dep HTTP + WebSocket server (RFC 6455 hand-rolled). Serves the newest screen, watches `content/`, records events to `state/events`. |
| `skills/brainstorming/scripts/helper.js` | Injected into every page. WebSocket client, click capture, `window.brainstorm` API. |
| `skills/brainstorming/scripts/frame-template.html` | Frame (header, theme CSS, status dot, indicator bar) wrapped around content fragments. |
| `skills/brainstorming/scripts/start-server.sh` | Launch wrapper. Session dir, host/url-host, owner-PID resolution, platform backgrounding. |
| `skills/brainstorming/scripts/stop-server.sh` | Kills the server by PID file, cleans `/tmp` sessions. |
| `skills/brainstorming/visual-companion.md` | Operator guide the agent reads when it accepts the companion. |
| `skills/brainstorming/SKILL.md` | Where the companion is offered and the per-question decision lives. |
## Disposition summary
| ID | Item | Source | Disposition |
|----|------|--------|-------------|
| A1 | Per-session secret key on `/`, `/files/*`, and WS (supersedes Host allowlist) | issues #1014, PRs #1110/#1553 | **Do** — chosen approach |
| A2 | Host allowlist; browser WS Origin check | PRs #1110/#1553 | Host allowlist dropped; WS Origin check retained after auth for browser confused-deputy defense |
| A3 | Crash on `null` / non-object WS payload | PR #1504 | Do |
| A4 | Frame-length bound in `decodeFrame` | issue #1446 | Already fixed — verify/close |
| B1 | Dotfile screens served as content (`._*.html`) | PR #950 | Do |
| B2 | `stop-server.sh` kills reused/stale PID | PR #1703 | Do |
| B3 | WS client reconnect backoff + status indicator | PR #856 | Do |
| C1 | Idle timeout too short / not configurable; WS not closed on shutdown | issue #1237 (PR #1689) | Do |
| C2 | Server death is invisible to user/agent | issue #1237 (residual) | Do |
| D1 | Permanent opt-out of the companion | issue #892 | Deferred - not in PR #1720 |
| D2 | Free-text feedback from the browser | issue #957 | Deferred - not in PR #1720 |
| D3 | Auto-open the companion URL | PR #759 (#755) | Done in PR #1720 via `--open` |
| D4 | Light/dark contrast helpers in the frame | PR #1683 | Deferred - not in PR #1720 |
| E1 | Hard-gate terminal-vs-HTML per question | PR #1037 | **Workshop** |
| E2 | Move session state out of the working tree | issue #975 (PR #977) | **Deferred** |
| E3 | Vendor Alpine.js for interactive mockups | PR #1639 | **Dropped** |
| E4 | Shell-lint warnings in start/stop scripts | PR #1677 | Opportunistic only |
---
## A. Server security hardening (`server.cjs`)
### A1 — Per-session secret key (chosen approach)
**Threat model.** Two assets: confidentiality of the served screen (`/`) and
files (`/files/*`), and integrity of `state/events` — a WebSocket client with a
truthy `choice` writes there (`server.cjs:243-246`), and the agent reads it next
turn as the user's selection, i.e. **prompt injection into a live session with
full tool access**. Reachers: with the default `127.0.0.1` bind, a malicious
page in the user's browser (a confused deputy — runs attacker JS *and* can reach
loopback); with a remote bind (`--host 0.0.0.0`, tailnet/LAN), any host that can
route to the port, directly, with no same-origin policy in the way. Today
`handleUpgrade` (`server.cjs:176`) checks only `Sec-WebSocket-Key`, and
`handleRequest` (`server.cjs:138`) checks nothing — both are wide open.
**Why a key, not a Host allowlist.** A Host allowlist only defends the
loopback browser-deputy. A direct remote client just sends the expected `Host`
and forges/omits `Origin`, so the allowlist is theater for exactly the remote
case we must protect. A per-session secret authenticates the client uniformly
across loopback, SSH tunnel, and direct-remote, and it also kills DNS rebinding
(the rebound page neither knows the key nor receives the host-scoped cookie).
So the key **supersedes** A1/A2's Host allowlist entirely — no `BRAINSTORM_ALLOWED_HOSTS`.
**Design.** Random token (`crypto.randomBytes(32)` hex), generated in
`server.cjs` at startup (overridable via `BRAINSTORM_TOKEN` for deterministic
tests):
1. **URL carries it** as `?key=<token>`. The server already builds `url` in its
`server-started` JSON (`server.cjs:351`) and writes it to `state/server-info`
— appending `?key=` there means `start-server.sh` (greps and prints that
JSON) and the skill (hands the user that URL) need **no change**.
2. **Cookie bootstrap.** A valid `?key` on `/` sets
`brainstorm-key-<port>=<token>; HttpOnly; SameSite=Strict; Path=/`. The
browser then auto-attaches it to same-origin subresources (`/files/*`) and
the WebSocket handshake, so the agent can write any URL style and it works,
and `helper.js` needs no change. Cookie name is **per-port** to avoid the
Jupyter multi-server collision (cookies aren't port-scoped).
`SameSite=Strict` is safe for CDN/Unsplash content — that cookie is host-
scoped, so outbound CDN requests never carry it; SameSite only governs
requests back to our origin, which are all same-site.
3. **Auth gate** = valid `?key` **OR** valid cookie (compared with
`crypto.timingSafeEqual`) on `/`, `/files/*`, and the WS upgrade. Missing/bad
key → friendly **403 HTML page** ("this page needs the full URL your coding
agent gave you, including `?key=…`" — generic "coding agent", not "Claude",
since this ships on Codex/Gemini/Copilot too). WS upgrade → destroy socket.
The query token is the source of truth; the cookie is a convenience that never
bears initial-auth load.
**Blast radius.** `server.cjs` (all logic). `helper.js` optional one-liner
(append `?key=` from `location.search` to the WS URL as a cookie-blocked
fallback). `start-server.sh` none. `visual-companion.md` doc note (URL now has
`?key=`; don't strip it). Tests updated to pass the token.
### A2 — Host allowlist dropped; browser WS Origin retained
Subsumed by A1. The secret key closes the WS-injection vector (#1014), the
HTTP/WS DNS-rebinding read vector (PR #1553), and the cross-origin WS vector
(PR #1110) in one mechanism, and unlike an allowlist it actually protects the
remote-bind case. No `BRAINSTORM_ALLOWED_HOSTS` and no Host allowlist. The final
implementation still checks browser WebSocket `Origin` after session auth so a
cross-origin localhost tab cannot ride the companion cookie.
### A3 — Server crashes on `null` / primitive WS payload
**Problem.** `handleMessage` (`server.cjs:233`) does `JSON.parse(text)` then
`if (event.choice)` at `server.cjs:243`. A client that sends the 4-byte text
frame `null` yields `event === null`, and `null.choice` throws. The throw is
**not** caught — `handleMessage` is called from the `socket.on('data')` handler
(`server.cjs:207`) outside the `try/catch`, which only wraps `decodeFrame`. The
result is an uncaught exception and process exit. Any local client can kill the
server.
**Change.** Guard the access: `if (event && event.choice)`. Minimal and exact —
`JSON.parse` can't produce `undefined`, and primitives return `undefined` for
`.choice` without throwing, so only `null` is the live hazard. (Avoid the
broader fixes — a top-level `try/catch` or `process.on('uncaughtException')`
would mask other bugs.)
### A4 — Frame-length bound in `decodeFrame` (adjacent)
Referenced by PR #1504 as #1446. The current code **already** bounds extended
frame lengths: `MAX_FRAME_PAYLOAD_BYTES = 10MB` (`server.cjs:10`) is enforced at
`server.cjs:58-67` before any `Buffer.alloc`. Action: verify #1446 against
current `dev` and close if already resolved, rather than re-implementing.
---
## B. Server robustness / correctness
### B1 — macOS resource-fork dotfiles served as screen content
**Problem.** The newest-screen selector filters on `f.endsWith('.html')` only
(`server.cjs:127-128`). On macOS/ExFAT, `._screen.html` resource-fork files pass
that filter and, being written alongside the real file, can sort newest — so the
browser gets binary metadata instead of the mockup. Four read sites share the
weak filter: `getNewestScreen` (`server.cjs:127`), `knownFiles` init
(`server.cjs:279`), the `fs.watch` handler (`server.cjs:286`), and the `/files/`
endpoint (`server.cjs:154-156`).
**Change.** Reject dotfiles (`!f.startsWith('.')`) at all four sites. Covers
`._*`, `.DS_Store`, etc.
### B2 — `stop-server.sh` can kill a reused PID
**Problem.** `stop-server.sh` reads the PID from `state/server.pid`
(`stop-server.sh:20`) and `kill`s it (`:23`, escalating to `-9` at `:35`)
without confirming the PID still belongs to our server. After a reboot or PID
wraparound the file can point at an unrelated process, which we'd then SIGKILL.
**Change.** Before signalling, verify ownership — the PID's command is `node`
running our `server.cjs`, ideally matching this session. If ownership can't be
proven, fail closed (report `stale_pid`, don't kill). Keep the existing
`stopped` / `not_running` outputs for the real cases.
### B3 — WebSocket client: silent reconnect, stale "Connected"
**Problem.** `helper.js` reconnects on a fixed 1s timer (`helper.js:21-23`),
has no `onerror` handler, never nulls `ws` on close, and never clears a pending
reconnect timer. The frame's status element is hardcoded to "Connected" with the
dot pinned to `var(--success)` (`frame-template.html:77,200`). When the laptop
sleeps or the server restarts, the page shows "Connected" over a dead socket and
queues events with no feedback.
**Change.**
- `helper.js`: exponential backoff (500ms → ×2 → cap 30s, reset on open);
`onerror` delegating to `onclose`; `ws = null` on close; `clearTimeout` before
reconnecting.
- `frame-template.html`: drive the status dot from a `--status-color` custom
property so JS can switch Connected (green) / Reconnecting (yellow) /
Disconnected (red).
---
## C. Lifecycle / timeout (issue #1237)
### C1 — Idle timeout too short, not configurable, WS keeps process alive
**Problem.** `IDLE_TIMEOUT_MS` is hardcoded to 30 minutes (`server.cjs:258`),
enforced by the 60s lifecycle check (`server.cjs:329-332`). A single brainstorm
question can sit longer than 30 min while the user thinks or steps away, so the
server dies mid-session. Separately, `shutdown()` (`server.cjs:310-321`) calls
`server.close()` but never closes the upgraded sockets in `clients`
(`server.cjs:174`), so an open browser connection can keep the Node process
alive past shutdown.
**Change.**
- Raise the default to 4 hours and make it configurable:
`--idle-timeout-minutes` in `start-server.sh` → an env var → `IDLE_TIMEOUT_MS`,
with validation against Node timer overflow.
- Expose the effective timeout in the startup JSON / `state/server-info`.
- In `shutdown()`, close every socket in `clients` so the process actually
exits.
### C2 — Server death is invisible
**Problem.** When the server exits it writes `state/server-stopped` and removes
`state/server-info` (`server.cjs:312-317`), and the skill is *told* to check
those files (`visual-companion.md:108`) — but it's soft guidance the model skips,
and the browser just shows a generic "can't be reached." The user diagnoses it
manually; the agent keeps referring to a dead URL.
**Change (two parts, independent of C1):**
- **Browser-facing tombstone.** Leave something at the last-served URL that says
"this companion expired — ask Claude to restart it" instead of a connection
error. Options to weigh: `helper.js` rendering a banner when the socket stays
down past backoff (works only while the page is loaded), vs. a more involved
approach that keeps a minimal responder alive to serve a tombstone page.
- **Harder skill check.** Tighten `visual-companion.md` / `SKILL.md` so
"check `server-info`/`server-stopped` before referring to the URL or pushing a
screen" is a required step, not a note. Keep it lightweight — possibly a
one-line helper the agent always runs.
---
## D. Features
### D1 — Permanent opt-out of the visual companion (issue #892)
**Problem.** The companion is offered as its own message every session
(`SKILL.md:25,151-152`). A user who never wants it pays that round-trip — and
HTML generation — every time. There's no way to say "never offer this."
**Change.** Before the offer step, the skill checks a user-level setting and
skips the offer entirely when opt-out is set.
**Design choice open.** Mechanism isn't settled:
- Env var (e.g. `SUPERPOWERS_VISUAL_COMPANION=off`) the skill is told to read —
simplest, matches what the issue asks for, lives in `.zshrc`.
- A plugin-settings file (`.claude/superpowers.local.md` frontmatter) — more
structured, per-project capable, but heavier and project-scoped.
- Reliability caveat from the issue: a separate "no-companion" skill competes on
trigger words and isn't reliable — rejected.
Pick the mechanism, then it's a small `SKILL.md` change plus a documented knob.
### D2 — Free-text feedback from the browser (issue #957)
**Problem.** The client only captures clicks on `[data-choice]`
(`helper.js:36-62`). A user who wants to annotate a mockup ("wrong shade of
blue") has to switch to the terminal, breaking the visual flow.
**Change.** Add a feedback `<textarea>` whose submit emits
`{"type":"feedback","text":...,"timestamp":...}` via the existing
`window.brainstorm.send` path (`helper.js:82-85`).
**Cross-cutting — server change required.** `handleMessage` only persists events
when `event.choice` is truthy (`server.cjs:243`). A `feedback` event has no
`choice`, so today it would be logged but **never written to `state/events`**,
and the agent wouldn't see it. The persistence condition must also accept
`feedback` events. Document the new event shape in `visual-companion.md`
(Browser Events Format, `:247-259`). Decide the submit trigger (button vs blur
vs both) and where the textarea renders (frame-level vs opt-in per screen).
### D3 — Auto-open the companion URL (PR #759, issue #755)
**Problem.** `start-server.sh` only prints the URL; the user opens it manually.
In WSL2 especially, people expect the browser to open.
**Change.** Best-effort opener after the `server-started` JSON is parsed:
Windows/WSL → `rundll32.exe url.dll,FileProtocolHandler <url>`, macOS → `open`,
Linux → `xdg-open` only when `DISPLAY`/`WAYLAND_DISPLAY` is set. Swallow
failures, never block startup, keep echoing the URL. Document in
`visual-companion.md`. (Consider an opt-out for headless/remote runs where
popping a browser is wrong — ties into D1's config mechanism.)
### D4 — Light/dark contrast helpers (PR #1683)
**Problem.** Content fragments are wrapped in the OS-aware frame
(`frame-template.html`). In dark mode, quick mockups often use white inline
backgrounds while inheriting low-contrast frame text, making cards/panels hard
to read.
**Change.** Add `.light-surface` / `.dark-surface` helper classes plus a
conservative fallback for common inline light backgrounds, and document them in
`visual-companion.md`'s CSS reference. Pure CSS in `frame-template.html`.
---
## E. Workshop / deferred / dropped
### E1 — Hard-gate terminal-vs-HTML per question (PR #1037) — WORKSHOP
The soft guidance already exists: "decide per-question," with browser-vs-terminal
tests in `SKILL.md:156-161` and `visual-companion.md:5-25`. The complaint is that
the model renders HTML for purely textual content (A/B lists, clarifying
questions), wasting tokens and a turn. PR #1037 wraps the decision in a
`<HARD-GATE>`. **Per Jesse, we'll workshop the wording/mechanism together**
this is behavior-shaping skill content and not specced here.
### E2 — Move session state out of the working tree (issue #975 / PR #977) — DEFERRED
Today `--project-dir` writes session state to `<project>/.superpowers/brainstorm/`
(`start-server.sh:80-84`) and the skill tells the user to gitignore it
(`visual-companion.md:58`). The ask is a `--state-dir` / `SUPERPOWERS_STATE_DIR`
default outside the repo (XDG), keeping `--project-dir` as an alias.
**Deferred by Jesse for now.** Captured so it isn't lost.
### E3 — Vendor Alpine.js for interactive mockups (PR #1639) — DROPPED
Adds a vendored Alpine build so mockups can be interactive (tabs, accordions,
forms) without hand-rolled JS. **Dropped per Jesse** — we are not taking on a
vendored third-party dependency in the companion runtime. The underlying need
(interactive mockups) is not being pursued via this route.
### E4 — Shell-lint warnings (PR #1677) — OPPORTUNISTIC
SC2034 (and friends) in `start-server.sh` / `stop-server.sh`. Trivial; fold into
B2/C1/D3 when we're already editing those scripts rather than as its own change.
---
## Suggested grouping for implementation
These cluster into a few coherent passes (each independently testable against
`tests/brainstorm-server/`):
1. **Security pass** (IN PROGRESS, branch `brainstorm-companion-session-key`) —
A1 per-session key (supersedes A2) + A3 null-crash guard. Verify/close A4.
*Highest priority.*
2. **Lifecycle pass** — C1 + C2 together (both touch `shutdown()` and the
server-death story).
3. **Robustness pass** — B1, B2, B3 (independent, small).
4. **Deferred feature pass** - D1, D2, D4 are not part of PR #1720. D3 is
shipped through the `--open` flow.
E1 is a separate workshop session. E2/E3 are out of scope for this round.
@@ -0,0 +1,785 @@
# Visual Companion Auth Hardening Implementation Plan
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
**Goal:** Harden the brainstorming visual companion auth and reconnect flow while preserving trusted same-origin screen JavaScript and future vendored UI libraries.
**Architecture:** Keyed root loads become a bootstrap step that sets the cookie, stores the key in tab-scoped `sessionStorage`, and navigates to a bare `/` screen URL. WebSockets require valid auth plus browser same-origin `Origin`, while `/files/*` uses realpath containment to prevent content-directory escapes.
**Tech Stack:** Node.js built-ins (`http`, `fs`, `path`, `crypto`), zero runtime dependencies, existing `ws` test dependency, Bash start/stop scripts, repo shell lint script.
**Important:** Do not commit during execution unless Drew explicitly asks. This repository's instructions override the generic plan template's commit cadence.
---
## File Map
- Modify: `skills/brainstorming/scripts/server.cjs`
- Add bootstrap response.
- Add shared security headers.
- Add WebSocket Origin validation.
- Add `/files/*` realpath containment.
- Modify: `skills/brainstorming/scripts/helper.js`
- Read the stored session key and append it to the WebSocket URL.
- Modify: `tests/brainstorm-server/auth.test.js`
- Add bootstrap, header, same-origin WS, cross-origin WS, and cookie/file auth regressions.
- Modify: `tests/brainstorm-server/helper.test.js`
- Add mocked-browser coverage for sessionStorage-backed WS URLs.
- Modify: `tests/brainstorm-server/server.test.js`
- Add symlink containment regression for `/files/*`.
- Modify: `tests/brainstorm-server/lifecycle.test.js`
- Make the start-server timeout flag test force background mode.
- Add restart reconnect credential coverage if it fits the existing lifecycle helper.
- Modify: `skills/brainstorming/scripts/start-server.sh`
- Fix shell lint.
- Modify: `skills/brainstorming/scripts/stop-server.sh`
- Fix shell lint.
- Modify: `.gitignore`
- Add `.superpowers/`.
- Optional docs update: `skills/brainstorming/visual-companion.md`
- Mention bootstrap URL stripping and trusted same-origin screen JS if the code behavior changes need operator-facing explanation.
## Task 1: Bootstrap Keyed Root Loads
**Files:**
- Modify: `tests/brainstorm-server/auth.test.js`
- Modify: `skills/brainstorming/scripts/server.cjs`
- [ ] **Step 1: Add RED tests for bootstrap behavior**
In `tests/brainstorm-server/auth.test.js`, add tests after the existing valid-key root test:
```js
await test('GET / with valid query returns bootstrap instead of screen content', async () => {
const res = await get('/', { key: TOKEN });
assert.strictEqual(res.status, 200);
assert(res.body.includes('sessionStorage'), 'bootstrap should store the session key in tab storage');
assert(res.body.includes('location.replace'), 'bootstrap should navigate to the bare root URL');
assert(!res.body.includes('Secret screen'), 'bootstrap must not serve screen HTML at the keyed URL');
});
await test('GET / with valid cookie serves the screen after bootstrap', async () => {
const res = await get('/', { cookie: `${COOKIE_NAME}=${TOKEN}` });
assert.strictEqual(res.status, 200);
assert(res.body.includes('Secret screen'), 'cookie-authenticated bare root should serve the screen');
assert(!res.body.includes('sessionStorage'), 'bare screen response should not be the bootstrap page');
});
```
Keep the existing cookie test if present; merge assertions rather than duplicating the same test name.
- [ ] **Step 2: Verify RED**
Run:
```bash
cd /Users/drewritter/prime-rad/superpowers/tests/brainstorm-server
node auth.test.js
```
Expected: the new bootstrap test fails because current `GET /?key=...` serves `Secret screen` directly and does not include the bootstrap `sessionStorage`/`location.replace` code.
- [ ] **Step 3: Implement minimal bootstrap response**
In `skills/brainstorming/scripts/server.cjs`, add a helper near the page constants:
```js
function bootstrapPage(key) {
const jsonKey = JSON.stringify(String(key));
return `<!DOCTYPE html>
<html>
<head><meta charset="utf-8"><title>Opening Brainstorm Companion</title></head>
<body>
<script>
sessionStorage.setItem('brainstorm-session-key', ${jsonKey});
location.replace('/');
</script>
</body>
</html>`;
}
```
Then in `handleRequest`, after authorization and cookie setting but before serving screen HTML, detect a valid query key on root:
```js
function queryKey(url) {
const q = url.indexOf('?');
if (q < 0) return null;
return new URLSearchParams(url.slice(q + 1)).get('key');
}
```
Use it in `handleRequest`:
```js
const pathname = pathnameOf(req.url);
const keyFromQuery = queryKey(req.url);
if (req.method === 'GET' && pathname === '/' && keyFromQuery && timingSafeEqualStr(keyFromQuery, TOKEN)) {
res.writeHead(200, securityHeaders({ 'Content-Type': 'text/html; charset=utf-8' }));
res.end(bootstrapPage(keyFromQuery));
return;
}
```
This assumes Task 4 will introduce `securityHeaders`. If implementing Task 1 first, temporarily use:
```js
res.writeHead(200, { 'Content-Type': 'text/html; charset=utf-8' });
```
and replace it in Task 4.
- [ ] **Step 4: Verify GREEN**
Run:
```bash
cd /Users/drewritter/prime-rad/superpowers/tests/brainstorm-server
node auth.test.js
```
Expected: all auth tests pass, including the new bootstrap tests.
## Task 2: WebSocket Origin Enforcement
**Files:**
- Modify: `tests/brainstorm-server/auth.test.js`
- Modify: `skills/brainstorming/scripts/server.cjs`
- [ ] **Step 1: Add RED tests for same-origin and cross-origin WS**
In `tests/brainstorm-server/auth.test.js`, extend `wsConnect` to accept an `origin` option:
```js
function wsConnect({ key, cookie, origin } = {}) {
const url = `ws://localhost:${TEST_PORT}/` + (key !== undefined ? `?key=${key}` : '');
const headers = {};
if (cookie) headers['Cookie'] = cookie;
if (origin) headers['Origin'] = origin;
const ws = new WebSocket(url, Object.keys(headers).length ? { headers } : {});
return new Promise((resolve) => {
let settled = false;
const done = (outcome) => { if (!settled) { settled = true; resolve({ outcome, ws }); } };
ws.on('open', () => done('opened'));
ws.on('error', () => done('rejected'));
ws.on('close', () => done('rejected'));
setTimeout(() => done('rejected'), 1500);
});
}
```
Then add:
```js
await test('WS upgrade with valid cookie and same-origin Origin opens', async () => {
const { outcome, ws } = await wsConnect({
cookie: `${COOKIE_NAME}=${TOKEN}`,
origin: `http://localhost:${TEST_PORT}`
});
ws.close();
assert.strictEqual(outcome, 'opened');
});
await test('WS upgrade with valid cookie but cross-origin Origin is rejected', async () => {
const eventsFile = path.join(TEST_DIR, 'state', 'events');
if (fs.existsSync(eventsFile)) fs.unlinkSync(eventsFile);
const { outcome, ws } = await wsConnect({
cookie: `${COOKIE_NAME}=${TOKEN}`,
origin: 'http://localhost:9999'
});
if (outcome === 'opened') {
ws.send(JSON.stringify({ type: 'choice', choice: 'attacker-injected', text: 'local attacker probe' }));
await sleep(300);
}
ws.close();
assert.strictEqual(outcome, 'rejected', 'cross-origin browser WS must not open even with cookie');
assert(!fs.existsSync(eventsFile), 'cross-origin WS must not write state/events');
});
```
- [ ] **Step 2: Verify RED**
Run:
```bash
cd /Users/drewritter/prime-rad/superpowers/tests/brainstorm-server
node auth.test.js
```
Expected: cross-origin cookie WS test fails because current server accepts any cookie-authenticated WS regardless of Origin.
- [ ] **Step 3: Implement Origin check**
In `skills/brainstorming/scripts/server.cjs`, add:
```js
function isAllowedWebSocketOrigin(req) {
const origin = req.headers.origin;
if (!origin) return true; // non-browser clients still need the session key
const host = req.headers.host;
if (!host) return false;
return origin === 'http://' + host;
}
```
Then update `handleUpgrade`:
```js
function handleUpgrade(req, socket) {
if (!isAuthorized(req) || !isAllowedWebSocketOrigin(req)) { socket.destroy(); return; }
```
- [ ] **Step 4: Verify GREEN**
Run:
```bash
cd /Users/drewritter/prime-rad/superpowers/tests/brainstorm-server
node auth.test.js
```
Expected: auth tests pass; cross-origin WS is rejected; same-origin and direct key WS still open.
## Task 3: Helper Uses Stored Key For Reconnect
**Files:**
- Modify: `tests/brainstorm-server/helper.test.js`
- Modify: `skills/brainstorming/scripts/helper.js`
- [ ] **Step 1: Add RED test for WebSocket URL key**
In `tests/brainstorm-server/helper.test.js`, add a mocked-browser test near the reconnect state-machine tests:
```js
test('uses sessionStorage key in the WebSocket URL when present', () => {
const e = makeEnv();
e.state.sessionKey = 'stored-key-abc';
e.boot();
assert.strictEqual(e.sockets[0].url, 'ws://localhost:7777/?key=stored-key-abc');
});
```
Update `makeEnv()` so the returned object exposes `sockets`, and the mock window includes sessionStorage:
```js
window: {
location: { host: 'localhost:7777', reload() { state.reloads++; } },
sessionStorage: { getItem: (key) => key === 'brainstorm-session-key' ? state.sessionKey : null }
},
```
Also add a fallback test:
```js
test('uses cookie-only WebSocket URL when no sessionStorage key is present', () => {
const e = makeEnv();
e.state.sessionKey = null;
e.boot();
assert.strictEqual(e.sockets[0].url, 'ws://localhost:7777');
});
```
- [ ] **Step 2: Verify RED**
Run:
```bash
cd /Users/drewritter/prime-rad/superpowers/tests/brainstorm-server
node helper.test.js
```
Expected: stored-key test fails because current helper uses `ws://localhost:7777`.
- [ ] **Step 3: Implement stored-key WS URL**
In `skills/brainstorming/scripts/helper.js`, replace:
```js
const WS_URL = 'ws://' + window.location.host;
```
with:
```js
function websocketUrl() {
let key = null;
try { key = window.sessionStorage && window.sessionStorage.getItem('brainstorm-session-key'); } catch (e) {}
return 'ws://' + window.location.host + (key ? '/?key=' + encodeURIComponent(key) : '');
}
```
Then replace:
```js
ws = new WebSocket(WS_URL);
```
with:
```js
ws = new WebSocket(websocketUrl());
```
- [ ] **Step 4: Verify GREEN**
Run:
```bash
cd /Users/drewritter/prime-rad/superpowers/tests/brainstorm-server
node helper.test.js
```
Expected: helper tests pass.
## Task 4: Security Headers
**Files:**
- Modify: `tests/brainstorm-server/auth.test.js`
- Modify: `skills/brainstorming/scripts/server.cjs`
- [ ] **Step 1: Add RED header tests**
In `tests/brainstorm-server/auth.test.js`, add:
```js
await test('HTML responses include leak-reduction and anti-framing headers', async () => {
const res = await get('/', { key: TOKEN });
assert.strictEqual(res.headers['referrer-policy'], 'no-referrer');
assert.strictEqual(res.headers['cache-control'], 'no-store');
assert.strictEqual(res.headers['x-frame-options'], 'DENY');
assert.strictEqual(res.headers['content-security-policy'], "frame-ancestors 'none'");
assert.strictEqual(res.headers['cross-origin-resource-policy'], 'same-origin');
});
await test('403 responses include leak-reduction and anti-framing headers', async () => {
const res = await get('/');
assert.strictEqual(res.status, 403);
assert.strictEqual(res.headers['referrer-policy'], 'no-referrer');
assert.strictEqual(res.headers['cache-control'], 'no-store');
assert.strictEqual(res.headers['x-frame-options'], 'DENY');
assert.strictEqual(res.headers['content-security-policy'], "frame-ancestors 'none'");
assert.strictEqual(res.headers['cross-origin-resource-policy'], 'same-origin');
});
```
- [ ] **Step 2: Verify RED**
Run:
```bash
cd /Users/drewritter/prime-rad/superpowers/tests/brainstorm-server
node auth.test.js
```
Expected: header tests fail because current responses do not include these headers.
- [ ] **Step 3: Implement shared header helper**
In `skills/brainstorming/scripts/server.cjs`, add:
```js
function securityHeaders(headers = {}) {
return {
'Referrer-Policy': 'no-referrer',
'Cache-Control': 'no-store',
'X-Frame-Options': 'DENY',
'Content-Security-Policy': "frame-ancestors 'none'",
'Cross-Origin-Resource-Policy': 'same-origin',
...headers
};
}
```
Update response writes in `handleRequest`:
```js
res.writeHead(403, securityHeaders({ 'Content-Type': 'text/html; charset=utf-8' }));
```
```js
res.writeHead(200, securityHeaders({ 'Content-Type': 'text/html; charset=utf-8' }));
```
```js
res.writeHead(200, securityHeaders({ 'Content-Type': contentType }));
```
For 404s:
```js
res.writeHead(404, securityHeaders());
```
- [ ] **Step 4: Verify GREEN**
Run:
```bash
cd /Users/drewritter/prime-rad/superpowers/tests/brainstorm-server
node auth.test.js
```
Expected: auth tests pass and header assertions are green.
## Task 5: `/files/*` Realpath Containment
**Files:**
- Modify: `tests/brainstorm-server/server.test.js`
- Modify: `skills/brainstorming/scripts/server.cjs`
- [ ] **Step 1: Add RED symlink escape test**
In `tests/brainstorm-server/server.test.js`, after the `/files/` empty-name test, add:
```js
await test('does not serve symlinks that escape content dir via /files/', async () => {
const target = path.join(STATE_DIR, 'server-info');
const link = path.join(CONTENT_DIR, 'linked-server-info.txt');
try { fs.unlinkSync(link); } catch (e) {}
fs.symlinkSync(target, link);
const res = await fetch(`http://localhost:${TEST_PORT}/files/linked-server-info.txt`);
assert.strictEqual(res.status, 404, 'symlink to state/server-info must not be served');
assert(!res.body.includes('server-started'), 'response must not include server-info body');
});
```
- [ ] **Step 2: Verify RED**
Run:
```bash
cd /Users/drewritter/prime-rad/superpowers/tests/brainstorm-server
node server.test.js
```
Expected: symlink test fails because current `/files/*` follows symlinks and serves `server-info`.
- [ ] **Step 3: Implement containment helper**
In `skills/brainstorming/scripts/server.cjs`, add:
```js
function isRegularFileInsideContentDir(filePath) {
let stat, realContentDir, realFilePath;
try {
stat = fs.lstatSync(filePath);
if (stat.isSymbolicLink()) return false;
if (!stat.isFile()) return false;
realContentDir = fs.realpathSync(CONTENT_DIR);
realFilePath = fs.realpathSync(filePath);
} catch (e) {
return false;
}
return realFilePath.startsWith(realContentDir + path.sep);
}
```
Replace the `/files/*` guard with:
```js
if (!fileName || fileName.startsWith('.') || !isRegularFileInsideContentDir(filePath)) {
res.writeHead(404, securityHeaders());
res.end('Not found');
return;
}
```
- [ ] **Step 4: Verify GREEN**
Run:
```bash
cd /Users/drewritter/prime-rad/superpowers/tests/brainstorm-server
node server.test.js
```
Expected: server tests pass, including symlink rejection.
## Task 6: Restart Reconnect Regression
**Files:**
- Modify: `tests/brainstorm-server/lifecycle.test.js`
- Modify: `skills/brainstorming/scripts/server.cjs`
- Modify: `skills/brainstorming/scripts/helper.js`
- [ ] **Step 1: Add RED integration test for same key over WS after restart**
In `tests/brainstorm-server/lifecycle.test.js`, add a test after the port/token persistence test:
```js
await test('stored key can authenticate WebSocket after same-port restart', async () => {
const dir = fs.mkdtempSync('/tmp/bs-reconnect-');
const portFile = path.join(dir, '.last-port');
const tokenFile = path.join(dir, '.last-token');
const env = { ...process.env, BRAINSTORM_PORT_FILE: portFile, BRAINSTORM_TOKEN_FILE: tokenFile, BRAINSTORM_LIFECYCLE_CHECK_MS: 100000 };
const a = spawn('node', [SERVER], { env: { ...env, BRAINSTORM_DIR: path.join(dir, 's1') } });
let outA = ''; a.stdout.on('data', d => outA += d.toString());
for (let i = 0; i < 60 && !outA.includes('server-started'); i++) await sleep(50);
const infoA = firstServerStarted(outA);
const keyA = new URL(infoA.url).searchParams.get('key');
a.kill(); await sleep(400);
const b = spawn('node', [SERVER], { env: { ...env, BRAINSTORM_DIR: path.join(dir, 's2') } });
let outB = ''; b.stdout.on('data', d => outB += d.toString());
for (let i = 0; i < 60 && !outB.includes('server-started'); i++) await sleep(50);
const infoB = firstServerStarted(outB);
const ws = new WebSocket(`ws://localhost:${infoB.port}/?key=${keyA}`, {
headers: { Origin: `http://localhost:${infoB.port}` }
});
const opened = await new Promise(resolve => {
ws.on('open', () => resolve(true));
ws.on('error', () => resolve(false));
setTimeout(() => resolve(false), 1500);
});
try {
assert.strictEqual(infoB.port, infoA.port, 'restart should reuse same port');
assert(opened, 'stored key should authenticate WS after restart');
} finally {
try { ws.close(); } catch (e) {}
b.kill(); await sleep(100);
fs.rmSync(dir, { recursive: true, force: true });
}
});
```
This test may already pass once Tasks 2 and 3 are implemented. If it passes before code changes, keep it as coverage but do not call it RED. The real browser reconnect behavior is primarily covered by Task 3 plus final manual/headless browser verification.
- [ ] **Step 2: Verify behavior**
Run:
```bash
cd /Users/drewritter/prime-rad/superpowers/tests/brainstorm-server
node lifecycle.test.js
```
Expected after Tasks 2 and 3: lifecycle tests pass. If this fails, fix the auth/restart path before continuing.
## Task 7: Lifecycle Hang And Shell Lint
**Files:**
- Modify: `tests/brainstorm-server/lifecycle.test.js`
- Modify: `skills/brainstorming/scripts/start-server.sh`
- Modify: `skills/brainstorming/scripts/stop-server.sh`
- [ ] **Step 1: Reproduce shell lint failure**
Run:
```bash
cd /Users/drewritter/prime-rad/superpowers
scripts/lint-shell.sh skills/brainstorming/scripts/start-server.sh skills/brainstorming/scripts/stop-server.sh tests/brainstorm-server/stop-server.test.sh
```
Expected current failure:
```text
SC2164: skills/brainstorming/scripts/start-server.sh line 128: cd "$SCRIPT_DIR"
SC2034: skills/brainstorming/scripts/start-server.sh line 166: for i in {1..50}
SC2034: skills/brainstorming/scripts/stop-server.sh line 57: for i in {1..20}
```
- [ ] **Step 2: Fix shell lint minimally**
In `skills/brainstorming/scripts/start-server.sh`, change:
```bash
cd "$SCRIPT_DIR"
```
to:
```bash
cd "$SCRIPT_DIR" || exit 1
```
Change unused loop variables from `i` to `_` where they are not read:
```bash
for _ in {1..50}; do
```
In `skills/brainstorming/scripts/stop-server.sh`, change:
```bash
for i in {1..20}; do
```
to:
```bash
for _ in {1..20}; do
```
- [ ] **Step 3: Fix lifecycle start-server hang**
In `tests/brainstorm-server/lifecycle.test.js`, update the `start-server.sh --idle-timeout-minutes sets the timeout` test command:
```js
const out = execFileSync('bash', [START, '--project-dir', dir, '--idle-timeout-minutes', '5', '--background'], { encoding: 'utf8' });
```
This keeps the test from hanging when `CODEX_CI` triggers start-server foreground mode.
- [ ] **Step 4: Verify lint and lifecycle**
Run:
```bash
cd /Users/drewritter/prime-rad/superpowers
scripts/lint-shell.sh skills/brainstorming/scripts/start-server.sh skills/brainstorming/scripts/stop-server.sh tests/brainstorm-server/stop-server.test.sh
cd tests/brainstorm-server
node lifecycle.test.js
```
Expected: shell lint exits 0; lifecycle tests exit 0 without hanging.
## Task 8: Gitignore Durable Companion State
**Files:**
- Modify: `.gitignore`
- [ ] **Step 1: Verify current ignore gap**
Run:
```bash
cd /Users/drewritter/prime-rad/superpowers
git check-ignore .superpowers/brainstorm/.last-token || true
```
Expected current output: no matching ignore rule.
- [ ] **Step 2: Add ignore rule**
Add this line to `.gitignore`:
```gitignore
.superpowers/
```
- [ ] **Step 3: Verify GREEN**
Run:
```bash
cd /Users/drewritter/prime-rad/superpowers
git check-ignore .superpowers/brainstorm/.last-token
```
Expected output:
```text
.superpowers/brainstorm/.last-token
```
## Task 9: Full Automated Verification
**Files:**
- No code changes in this task.
- [ ] **Step 1: Run focused suites**
Run:
```bash
cd /Users/drewritter/prime-rad/superpowers/tests/brainstorm-server
node auth.test.js
node helper.test.js
node server.test.js
node lifecycle.test.js
```
Expected: all four commands exit 0.
- [ ] **Step 2: Run full brainstorm-server suite**
Run:
```bash
cd /Users/drewritter/prime-rad/superpowers/tests/brainstorm-server
npm test
```
Expected: all tests pass, including ws-protocol, helper, auth, server, lifecycle, and stop-server.
- [ ] **Step 3: Repeat suite for lifecycle/watch flake**
Run:
```bash
cd /Users/drewritter/prime-rad/superpowers/tests/brainstorm-server
for i in 1 2 3; do npm test || exit 1; done
```
Expected: all three repeats pass without hanging.
- [ ] **Step 4: Run shell lint**
Run:
```bash
cd /Users/drewritter/prime-rad/superpowers
scripts/lint-shell.sh skills/brainstorming/scripts/start-server.sh skills/brainstorming/scripts/stop-server.sh tests/brainstorm-server/stop-server.test.sh
```
Expected: exits 0.
## Task 10: Re-run Security Probes
**Files:**
- No code changes in this task.
- [ ] **Step 1: Recreate the cross-origin attacker probe**
Use the previous scratch probe if available:
```bash
node /tmp/superpowers-pr1720-security-drewritter/probe-pr1720.cjs
```
If the scratch probe is unavailable, recreate a minimal probe under `/tmp` that:
- starts the companion with a fixed token
- loads the keyed URL in headless Chrome
- starts an attacker page on a different localhost port
- attempts `new WebSocket('ws://localhost:<companion-port>/')`
- sends `{"type":"choice","choice":"attacker-injected"}`
- checks `state/events`
Expected after fixes:
- keyless and wrong-key HTTP still return 403
- same-origin helper reaches Connected
- cross-origin WebSocket does not open
- `state/events` does not contain `attacker-injected`
- symlink-to-`server-info` returns 404
- keyed browser load ends on bare `/`
- [ ] **Step 2: Re-run manual/browser flow only after automated probes pass**
Manual flow:
1. start the companion with `--project-dir --open`
2. push a screen
3. confirm URL strips to `/`
4. confirm status reaches Connected
5. click a choice and verify `state/events`
6. stop and restart same project
7. verify the open tab reconnects automatically
Expected: all steps pass without manual URL reload.
## Self-Review Checklist
- Spec coverage: every design requirement maps to at least one task.
- Placeholder scan: this plan contains no unresolved placeholder markers or unspecified edge-case steps.
- TDD order: every production change task starts with a focused failing test or a command that demonstrates the current failure.
- Trust model: the plan preserves trusted same-origin screen JavaScript and future same-origin vendored libraries.
- No-commit rule: execution does not commit unless Drew explicitly asks.
@@ -46,7 +46,7 @@ The skill describes the goal ("ensure work happens in an isolated workspace") an
### Provenance-based ownership
Whoever creates the worktree owns its cleanup. If the harness created it, superpowers doesn't touch it. If superpowers created it (via git fallback), superpowers cleans it up. The heuristic: if the worktree lives under `.worktrees/` or `~/.config/superpowers/worktrees/`, superpowers owns it. Anything else (`.claude/worktrees/`, `~/.codex/worktrees/`, `.gemini/worktrees/`) belongs to the harness.
Whoever creates the worktree owns its cleanup. If the harness created it, superpowers doesn't touch it. If superpowers created it (via git fallback), superpowers cleans it up. The heuristic: if the worktree lives under `.worktrees/` or `worktrees/`, superpowers owns it. Anything else (`.claude/worktrees/`, `~/.codex/worktrees/`, `.gemini/worktrees/`, or old user-global Superpowers paths) belongs to the harness or user and is left alone.
## Design
@@ -110,12 +110,11 @@ File splitting (Step 1b in a separate skill) was tested and proven unnecessary.
When no native tool is available, create a worktree manually.
**Directory selection** (priority order):
1. Check for existing `.worktrees/` or `worktrees/` directory — if found, use it. If both exist, `.worktrees/` wins.
2. Check for existing `~/.config/superpowers/worktrees/<project>/` directory — if found, use it (backward compatibility with legacy global path).
3. Check the project's agent instruction file (CLAUDE.md, GEMINI.md, AGENTS.md, .cursorrules, or equivalent) for a worktree directory preference.
4. Default to `.worktrees/`.
1. Check the project's agent instruction file (CLAUDE.md, GEMINI.md, AGENTS.md, .cursorrules, or equivalent) for a worktree directory preference.
2. Check for existing `.worktrees/` or `worktrees/` directory — if found, use it. If both exist, `.worktrees/` wins.
3. Default to `.worktrees/`.
No interactive directory selection prompt. The global path (`~/.config/superpowers/worktrees/`) is no longer offered as a choice to new users, but existing worktrees at that location are detected and used for backward compatibility.
No interactive directory selection prompt. Old user-global Superpowers worktree paths are not detected or offered; new manual worktrees are project-local unless the user explicitly specifies another location.
**Safety verification** (project-local directories only):
@@ -232,7 +231,7 @@ if GIT_DIR == GIT_COMMON:
# Normal repo, no worktree to clean up
done
if worktree path is under .worktrees/ or ~/.config/superpowers/worktrees/:
if worktree path is under .worktrees/ or worktrees/:
# Superpowers created it — we own cleanup
cd to main repo root # Bug #238 fix
git worktree remove <path>
@@ -318,7 +317,7 @@ As of 2026-04-06, Claude Code is the only harness with an agent-callable mid-ses
### Provenance heuristic
The `.worktrees/` or `~/.config/superpowers/worktrees/` = ours, anything else = hands off` heuristic works for every current harness. If a future harness adopts `.worktrees/` as its convention, we'd have a false positive (superpowers tries to clean up a harness-owned worktree). Similarly, if a user manually runs `git worktree add .worktrees/experiment` without superpowers, we'd incorrectly claim ownership. Both are low risk — every harness uses branded paths, and manual `.worktrees/` creation is unlikely — but worth noting.
The `.worktrees/` or `worktrees/` = ours, anything else = hands off` heuristic works for every current harness. If a future harness adopts one of those project-local directories as its convention, we'd have a false positive (superpowers tries to clean up a harness-owned worktree). Similarly, if a user manually runs `git worktree add .worktrees/experiment` without superpowers, we'd incorrectly claim ownership. Both are low risk — every harness uses branded paths, and manual `.worktrees/` creation is unlikely — but worth noting.
### Detached HEAD finishing
@@ -0,0 +1,77 @@
# Platform-neutral config-file references — Phase B design
## Background
Phase A (see `2026-05-05-platform-neutral-prose-design.md`) replaced generic third-person "Claude" prose with agent-neutral forms. This phase tackles the next category: references to the per-platform instruction file (CLAUDE.md, AGENTS.md, GEMINI.md) inside skills.
The plugin runs on multiple harnesses, and each one reads its own instruction file. Where a skill names CLAUDE.md as if it were the only file, that's a Claude-Code-centric assumption that doesn't hold on Codex / Gemini CLI / OpenCode.
## In scope
Two specific lines in active skills:
1. **`skills/writing-skills/SKILL.md:58`** — `Project-specific conventions (put in CLAUDE.md)`
2. **`skills/receiving-code-review/SKILL.md:30`** — `"You're absolutely right!" (explicit CLAUDE.md violation)`
## Out of scope
- **`skills/using-superpowers/SKILL.md:22, 26`** — instruction-priority list. The list already names all three (CLAUDE.md, GEMINI.md, AGENTS.md) inclusively, which is correct: the section is making a real claim about *what counts as user instruction* on a multi-platform plugin. No change needed.
- **Historical / example artifacts**:
- `skills/systematic-debugging/CREATION-LOG.md` — attribution path (`~/.claude/CLAUDE.md`) is a historical fact.
- `skills/writing-skills/examples/CLAUDE_MD_TESTING.md` — the entire file is a worked example testing CLAUDE.md content variants. The filename, body, and the reference from `testing-skills-with-subagents.md` all stay; normalizing them defeats the example.
- **Platform-tooling references** — Phase D candidates:
- `skills/using-superpowers/SKILL.md:40` (Gemini CLI tool mapping note about GEMINI.md)
- `skills/using-superpowers/references/gemini-tools.md` (`save_memory` persists to GEMINI.md)
## Substitution rules
Two distinct calls, one per in-scope line.
### Rule 1: "where to put project-specific conventions"
`writing-skills/SKILL.md:58`:
- **Before:** `Project-specific conventions (put in CLAUDE.md)`
- **After:** `Project-specific conventions (put in your instructions file)`
Use a generic phrase rather than picking one filename. Different harnesses read different files (CLAUDE.md, AGENTS.md, GEMINI.md, etc.) and the skill should not assume one. The platform-tools reference docs (`references/{codex,copilot,gemini}-tools.md`) are the right place to name each platform's preferred file.
### Rule 2: the "(explicit CLAUDE.md violation)" parenthetical
`receiving-code-review/SKILL.md:30`:
- **Before:** `"You're absolutely right!" (explicit CLAUDE.md violation)`
- **After:** `"You're absolutely right!" (explicit instruction-file violation)`
The parenthetical is doing real work — it signals this phrase isn't just stylistically bad, it actively violates rules many users put in their instruction files. "Instruction file" is the natural cross-platform term covering AGENTS.md / CLAUDE.md / GEMINI.md collectively, and keeps the original signal without picking one filename or softening to "common".
## Commit plan
Atomic commits, in order:
1. **`writing-skills/SKILL.md`** — CLAUDE.md → "your instructions file" in the "where to put project conventions" line
2. **`receiving-code-review/SKILL.md`** — CLAUDE.md → instruction-file in the violation parenthetical
3. **Platform-tools reference docs** — add the preferred per-platform instructions filename (CLAUDE.md, AGENTS.md, GEMINI.md, etc.) to each `references/{codex,copilot,gemini}-tools.md` so readers can resolve "your instructions file" to a real filename.
Each commit message names "Phase B" and the slice.
## Verification
After each commit:
- Read the surrounding paragraph to confirm grammar and meaning still parse.
- `grep -n "CLAUDE\.md" <touched-file>` — no remaining hits in active prose (carve-outs already documented).
After both commits:
- `grep -rn "CLAUDE\.md" skills/` should return only the documented carve-outs (CREATION-LOG, CLAUDE_MD_TESTING and its inbound reference, the priority list in using-superpowers).
## Non-goals
- Do not touch the priority list ordering in `using-superpowers/SKILL.md`. Reordering CLAUDE.md / GEMINI.md / AGENTS.md is an aesthetic change, not a substitution, and out of scope here.
- Do not rename `examples/CLAUDE_MD_TESTING.md` or change its content.
- Do not modify Gemini-CLI-specific tooling references (Phase D candidates).
## Implementation note
Phase B as written here covered three commits and the three non-Claude-Code platform-tools refs. Implementation went one step further: a fourth ref, `references/claude-code-tools.md`, was added in commit `8505703` for symmetry, so Claude Code's instructions-file conventions and tool-name list live alongside the others rather than implicitly in the surrounding skill prose. That addition wasn't anticipated in this spec but is consistent with its intent.
@@ -0,0 +1,94 @@
# Platform-neutral prose — Phase A design
## Background
Superpowers ships to multiple agent runtimes (Claude Code, Codex, Cursor, OpenCode, Copilot CLI, Gemini CLI). Skill content and supporting docs were written first for Claude Code and use "Claude" in places where any runtime's agent applies. OpenAI's vendored fork (openai/plugins#217) attempted a wholesale rewrite that was actively wrong in places — rewriting historical attribution paths, model names, and platform-specific install instructions — and we want to avoid that mistake while still removing platform-centric prose where it is genuinely incidental.
The full effort is broken into phases by reference category. **This spec covers Phase A only:** generic third-person prose mentioning "Claude" in non-platform-specific contexts. Later phases (config-file references, marketing copy, tool-name references) are out of scope here and will get their own specs.
## In scope
Generic prose mentions of "Claude" in:
- `skills/*/SKILL.md` and supporting `.md` files in active skill directories
- `skills/writing-skills/anthropic-best-practices.md`
- `README.md` (only where the mention is generic prose, not platform marketing)
Plus one coined-term rename: **Claude Search Optimization (CSO) → Skill Discovery Optimization (SDO)** in `skills/writing-skills/SKILL.md`.
## Out of scope
- **Platform/runtime statements** — "In Claude Code:", install instructions, tool-mapping references. (Phase D candidate.)
- **Config-file references** — CLAUDE.md, AGENTS.md, GEMINI.md priority lists and "where to put project conventions" callouts. (Phase B.)
- **Tool-name references** — `Skill`, `Bash`, `Read`, `Task`, `TodoWrite`. Skills are written in Claude Code's tool vocabulary; the existing `references/{codex,copilot,gemini}-tools.md` files map them. (At the time this spec was written, the plan was to defer or skip these. Phase E ended up doing them — replacing tool names with action language across active skills and unifying the platform-tools refs around the same vocabulary.)
- **Marketing copy** in README — "Superpowers for Claude Code", platform-named install sections. (Phase C.)
- **Historical artifacts** — `docs/plans/*.md`, `docs/superpowers/specs/*.md`, `CREATION-LOG.md`. These are dated, point-in-time documents; rewriting them rewrites history.
- **Model identifiers** — Claude Haiku / Sonnet / Opus. These are real product names.
- **Filename / URL references** — `CLAUDE.md`, `claude.com`, `claude-plugin/`, paths under `~/.claude/`.
- **`anthropic-best-practices.md` filename** — the file remains named after its source even though we rewrite the prose inside it.
## Replacement style
Use a mix that reads naturally in English:
- **Second person — "your agent"** when addressing the skill author about *their* runtime
- "your agent reads the description"
- **Third person — "the agent" / "agents" / "an agent"** when describing system behavior generically
- "Future agents find your skills"
- "Use words an agent would search for"
- "Agents read SKILL.md only when the skill becomes relevant"
Pick whichever fits the surrounding sentence; do not force consistency at the cost of awkward phrasing. Pluralize when natural ("future agents", "agents read") rather than always saying "the agent".
### Carve-outs that stay as "Claude"
- Model names: Claude Haiku, Claude Sonnet, Claude Opus
- Filenames and URLs: `CLAUDE.md`, `claude.com`, `~/.claude/`
- Branded platform name "Claude Code" wherever it refers to the runtime as such (handled in later phases)
### Coined-term rename
- **Claude Search Optimization (CSO) → Skill Discovery Optimization (SDO)**
- Appears in `skills/writing-skills/SKILL.md` as a section heading and in nearby prose. Rename the heading, the acronym, and any in-file cross-references.
## Files affected
Approximate counts based on a `grep` filtered to exclude carve-outs:
| File | Generic-prose mentions |
|------|------------------------|
| `skills/writing-skills/SKILL.md` | ~12 (includes CSO heading + body) |
| `skills/writing-skills/anthropic-best-practices.md` | ~30 |
| `skills/writing-skills/examples/CLAUDE_MD_TESTING.md` | ~1 — filename stays (it's a CLAUDE.md test artifact); the "Variant C: Claude.AI Emphatic Style" heading also stays (it's a label naming a specific style) |
| `README.md` | ~1 |
Final list confirmed during implementation by re-running the filtered grep.
## Commit plan
Four atomic commits, in order:
1. **Rename CSO → SDO** in `skills/writing-skills/SKILL.md`. Mechanical, isolated, easy to revert if we change our minds about the term.
2. **Active skills prose** — generic "Claude" → "agent" forms across `skills/*/SKILL.md` and supporting `.md`, excluding `anthropic-best-practices.md`.
3. **`anthropic-best-practices.md` prose** — same substitution rules. Separate commit because this file is a vendored adaptation of an external doc; isolating the change makes future reconciliation with upstream easier to read.
4. **README.md prose** *(only if any generic-prose mentions remain after filtering)*. Skipped if empty.
Each commit message names the phase ("Phase A") and the slice ("rename CSO to SDO", "agent prose in active skills", etc.) so the series is self-documenting.
## Verification
After each commit:
- `grep -rn "Claude" <touched-paths>` — every remaining hit must fall into a documented carve-out (model name, filename, URL, "Claude Code" platform name, historical artifact).
- Read the touched file end-to-end — substitutions should not have broken sentence flow, pronoun agreement, or list parallelism.
- No tests to run; this is prose-only.
After the final commit:
- Skim each modified skill in a live session to confirm nothing reads awkwardly.
## Non-goals
- Do not change behavior, structure, headings (other than CSO→SDO), examples, code blocks, or YAML frontmatter.
- Do not introduce new sections, callouts, or compatibility notes.
- Do not "improve" prose beyond the substitution while editing.
@@ -0,0 +1,47 @@
# Platform-neutral README ordering — Phase C design
## Background
Phases A and B (see `2026-05-05-platform-neutral-prose-design.md` and `2026-05-05-platform-neutral-config-refs-design.md`) already neutralized generic Claude prose and config-file references in the README. The remaining platform-leaning signal is layout: the README's two platform listings put Claude Code first and aren't strictly alphabetical elsewhere.
This phase fixes the ordering. No prose changes.
## In scope
1. **Quickstart platform list** (`README.md:7`) — the inline link list of supported harnesses
2. **Installation section ordering** (`README.md:35152`) — the per-harness install sub-sections
## Out of scope
- Prose, marketplace names, plugin IDs, URLs — all factually correct as-is.
- Visual weight of the Claude Code section (which has two sub-sections — official Anthropic marketplace and Superpowers marketplace). Both are real install paths; collapsing them would hide accurate info.
- Section headings and content within each install block — only the ordering of the blocks changes.
## Substitution
Both listings reorder to strict alphabetical:
| Old order | New order |
|-----------|-----------|
| Claude Code | Claude Code |
| Codex CLI | Codex App |
| Codex App | Codex CLI |
| Factory Droid | Cursor |
| Gemini CLI | Factory Droid |
| OpenCode | Gemini CLI |
| Cursor | GitHub Copilot CLI |
| GitHub Copilot CLI | OpenCode |
Three moves: Codex App swaps with Codex CLI; Cursor moves up two slots; GitHub Copilot CLI moves up one.
Claude Code remains first by alphabetical chance (`Cl…` precedes `Co…`).
## Commit plan
One atomic commit covering both listings, since changing one without the other would create inconsistency between the quickstart and the installation section.
## Verification
- Quickstart anchors (`#claude-code`, `#codex-app`, etc.) still resolve to existing `### …` headings — no headings renamed.
- Each install sub-section's body is byte-identical pre/post; only positions changed.
- `git diff README.md` shows section moves only, no content edits.
@@ -0,0 +1,247 @@
# Lift drill into superpowers as `evals/` — design
## Background
Drill is a Python skill-compliance benchmark that lives in its own repo at `obra/drill`. It drives real tmux sessions, runs an LLM actor as a simulated user, runs an LLM verifier on the resulting transcript, and reports pass/fail per scenario. It supports Claude Code, Codex, Gemini CLI, and (per recent commits) OpenCode and Copilot CLI.
Drill is already the *de facto* eval harness for superpowers. The PRI-1397 commit series in the drill repo lifted ~22 superpowers bash tests into drill scenarios, and the most recent superpowers commit (`a2292c5`) explicitly removed a redundant bash test with the message *"replaced by drill behavioral coverage"*. Migration momentum exists; this spec completes it.
This work moves drill into superpowers under `evals/`, deletes the redundant bash tests after per-file verification of drill scenario coverage, and updates docs so contributors land on the new structure.
## Goals
1. `evals/` is the canonical eval harness in superpowers — full drill source, scenarios, fixtures, prompts, backend configs, and tests.
2. Bash tests in `superpowers/tests/` that have been individually verified as 100% covered by drill scenarios are deleted; the rest are preserved.
3. The split between `tests/` (plugin infrastructure: bash + node + python integration tests) and `evals/` (LLM behavior with actor + verifier) is meaningful and documented.
4. Top-level docs (`README.md`, `CLAUDE.md`, `docs/testing.md`) point contributors at the right place.
5. The standalone `obra/drill` repo continues to exist (this PR does not touch it) and gets archived as a separate manual step after this PR merges.
## Non-goals
- **CI integration.** Manual-only here. The natural follow-up is "tiered": fast subset on every PR, full sweep nightly + on-demand. That requires API budget decisions, GitHub Actions secrets, and a runner image with `tmux` + `node` + `python` + `claude` / `codex` / `gemini` CLIs installed. Out of scope.
- **Scenario co-location with skills.** Scenarios stay centralized at `evals/scenarios/`. If we later decide each skill should own its scenarios, that's a path-find-and-rename operation; the YAML format does not change.
- **Renaming the internal Python package** (`drill``evals`). The directory is `evals/` (user-facing); the Python package keeps its `drill` name to keep the diff small. A short note in `evals/README.md` explains.
- **Drill repo archival.** This PR does not touch `obra/drill`. After merge, the drill repo is archived manually (read-only on GitHub, README pointer to `obra/superpowers/evals/`).
- **Lifting `tests/claude-code/analyze-token-usage.py` into `evals/bin/`.** Useful utility, not test code. Can move later; not required by this PR.
## Branching
Branch off `dev` as `f/evals-lift`. This work is independent of the open `f/cross-platform` PR — no shared file changes besides possibly `README.md`, which is small enough to resolve at merge time if it conflicts.
## Architecture after the move
```
superpowers/
evals/ ← NEW (full drill copy)
pyproject.toml (Python 3.11, uv-managed)
uv.lock
.gitignore (drill's own; results/, .venv/, .env)
README.md (was drill's README; install instructions updated)
CLAUDE.md (was drill's CLAUDE.md; paths updated)
docs/
design.md (drill's design — preserved verbatim, cross-linked from this spec)
manual-testing.md
pressure-and-red-testing.md
drill/ (Python package; name kept; cli, engine, actor, verifier, etc.)
backends/ (claude-*.yaml, codex.yaml, gemini.yaml)
scenarios/ (32+ YAML scenarios)
setup_helpers/ (15 Python helpers; create_base_repo, sdd_*, spec_*, worktree, etc.)
fixtures/ (template-repo, sdd-go-fractals, sdd-svelte-todo)
prompts/ (actor.md, verifier.md)
bin/ (assertion helper scripts: tool-called, tool-count, etc.)
tests/ (drill's own pytest suite)
tests/ ← bash tests preserved by default
brainstorm-server/ ← KEEP (node tests for brainstorm-server JS code)
opencode/ ← KEEP (plugin loading tests)
codex-plugin-sync/ ← KEEP (sync verification)
claude-code/ ← MOSTLY KEEP — see deletion gate
explicit-skill-requests/ ← KEEP unless verified replaced
skill-triggering/ ← KEEP unless verified replaced
subagent-driven-dev/ ← KEEP unless verified replaced
docs/
testing.md ← UPDATED (split into "Plugin tests" + "Skill behavior evals")
superpowers/
specs/
2026-05-06-lift-drill-into-evals-design.md ← THIS SPEC
README.md ← small Contributing-section pointer to evals/
CLAUDE.md ← one-line "Eval harness lives at evals/" pointer
```
The `tests/` and `evals/` directories serve clearly distinct roles after this PR:
- **`tests/`** — does the plugin's non-LLM code work? Unit and integration tests for the brainstorm-server JS code, OpenCode plugin loading, codex-plugin-sync sync verification. Bash + node + python.
- **`evals/`** — do agents behave correctly on real LLM sessions? Drill scenarios with actor + verifier. Python-only, runs real tmux sessions.
## Deletion gate (per bash test)
A bash test is deleted *only if* a drill scenario verifiably covers every assertion it makes. The implementation plan documents this verification per file: read the bash test, list its checks, find the drill scenario, confirm each check has a matching `verify.assertions` or `verify.criteria` entry. If even one check is missing, the option is to either extend the drill scenario or keep the bash test. Default keeps it.
**Tentative coverage map** (commit-message-based; needs per-file verification before any deletion):
| Bash test | Claimed drill replacement | Coverage status |
|-----------|---------------------------|-----------------|
| `tests/skill-triggering/prompts/*` (6 prompt files) | `triggering-*.yaml` (6 scenarios) | candidate — verify per-prompt before deleting |
| `tests/skill-triggering/run-test.sh`, `run-all.sh` | n/a (runners, not tests) | **keep** — runner scripts |
| `tests/explicit-skill-requests/prompts/please-use-brainstorming.txt` | needs verification — drill has no obvious counterpart yet | likely **keep** unless drill scenario added |
| `tests/explicit-skill-requests/prompts/use-systematic-debugging.txt` | needs verification — drill has no obvious counterpart | likely **keep** unless drill scenario added |
| `tests/explicit-skill-requests/run-claude-describes-sdd.sh` | partially → `mid-conversation-skill-invocation.yaml` | candidate — verify per-script |
| `tests/explicit-skill-requests/run-haiku-test.sh` | no drill scenario covers Haiku-specific behavior | **keep** |
| `tests/explicit-skill-requests/run-multiturn-test.sh`, `run-extended-multiturn-test.sh` | no drill scenario covers multi-turn build-up | **keep** unless drill scenarios added |
| `tests/explicit-skill-requests/run-test.sh`, `run-all.sh` | n/a (runners) | **keep** |
| `tests/subagent-driven-dev/go-fractals/`, `tests/subagent-driven-dev/svelte-todo/` | `sdd-go-fractals.yaml`, `sdd-svelte-todo.yaml` | candidate — verify before deleting (these include real assertions about test suites passing) |
| `tests/claude-code/test-document-review-system.sh` | `spec-reviewer-catches-planted-flaws.yaml` | candidate — verify before deleting |
| `tests/claude-code/test-requesting-code-review.sh` | `code-review-catches-planted-bugs.yaml` | candidate — verify before deleting |
| `tests/claude-code/test-subagent-driven-development-integration.sh` | `sdd-rejects-extra-features.yaml` (YAGNI subset) | **partial** — bash test also asserts ≥3 commits / `npm test` passes / runs `analyze-token-usage.py`. Drill scenario asserts forbidden-exports + reviewer-as-gate. Mostly disjoint — almost certainly **keep + extend drill scenario**. |
| `tests/claude-code/test-subagent-driven-development.sh` | meta/documentation test (asks agent to *describe* SDD); no drill scenario covers description tests | **keep** unless drill scenario added |
| `tests/claude-code/test-worktree-native-preference.sh` | `worktree-creation-under-pressure.yaml` | candidate — verify before deleting |
| `tests/claude-code/test-helpers.sh`, `run-skill-tests.sh`, `analyze-token-usage.py` | n/a (utilities, not tests) | **keep** — libraries/tools |
## Verification protocol (subagent-gated)
Every change in the implementation plan gets cross-checked by an independent subagent before commit.
| Change category | Subagent verification |
|----------------|----------------------|
| Each bash-test deletion | Dispatch a subagent with: (a) the bash test file content, (b) the candidate drill scenario YAML, (c) the prompt: *"List every assertion the bash test makes. List every verify entry in the drill scenario. For each bash assertion, find a matching drill check or report it as unmatched. Output a per-assertion table."* The subagent's output is the gate — only delete if every bash assertion has a match. |
| Initial `evals/` copy | Subagent verifies: (a) drill SHA being copied is recorded in the lift commit message so provenance is auditable; (b) **per-file SHA-256 checksum** matches drill repo for every file (not just file count); (c) excluded paths (`.git/`, `.venv/`, `results/`, `.env`, `__pycache__/`, `*.egg-info/`, any `.private-journal/`) are absent from `evals/`; (d) all backend YAMLs reference paths that exist post-move; (e) `pyproject.toml`, `uv.lock`, `.gitignore` are intact. |
| Drill's own pytest suite | Subagent runs `cd evals && uv run pytest` after the path-default change. Drill ships its own pytest suite at `evals/tests/` including `test_backend.py` which exercises `SUPERPOWERS_ROOT` env-var behavior — these tests must update to match the helper and continue to pass. |
| Reference scrubbing after deletion | Subagent greps the entire superpowers tree (excluding `node_modules/`, `.venv/`, and `evals/`) for references to deleted bash test paths. Search targets: `docs/`, `docs/superpowers/plans/`, `RELEASE-NOTES.md`, `CLAUDE.md`, `GEMINI.md`, `AGENTS.md`, `README.md`, `.github/`, `scripts/`, `.opencode/INSTALL.md`, `.codex-plugin/INSTALL.md`, `lefthook.yml`. Any hit is either updated or surfaces a missed dependency. |
| Path defaults change (`SUPERPOWERS_ROOT` default) | Subagent runs at least one cheap drill scenario after the path changes (e.g., `triggering-test-driven-development`) and confirms it still passes. Real validation, not just code review. |
| Final pre-PR adversarial review | Two subagents in parallel, "5 points to whoever finds the most legitimate issues" framing — same protocol used on the cross-platform PR. Verify both source code and behavior. |
Each subagent task gets its own bullet in the implementation plan with explicit inputs and pass criteria. The subagent's output is summarized in the relevant commit message ("Subagent verification: …") so the trail is auditable.
## Concrete path/config edits
**Verified prior to writing this spec.** `drill/cli.py` defines `PROJECT_ROOT = Path(__file__).parent.parent`. After the move, `cli.py` lives at `evals/drill/cli.py`, so `PROJECT_ROOT` resolves to `evals/` and `PROJECT_ROOT.parent` resolves to the superpowers repo root. That's the value `SUPERPOWERS_ROOT` should take by default.
**YAML substitution audit.** Only the four `claude*.yaml` backend configs interpolate `${SUPERPOWERS_ROOT}` into `args` (for the `--plugin-dir` flag); `codex.yaml` and `gemini.yaml` only list `SUPERPOWERS_ROOT` in `required_env` (consumed by `engine.py:233` / `setup.py:25`'s `os.environ["SUPERPOWERS_ROOT"]` lookups in pre/post-run hooks). The helper's `os.environ` mutation covers both code paths.
| File | Current | After |
|------|---------|-------|
| `drill/cli.py` | `load_dotenv(PROJECT_ROOT / ".env")` at module import; nothing about `SUPERPOWERS_ROOT` | After `load_dotenv`, call new helper `_set_superpowers_root_default()` that sets `os.environ["SUPERPOWERS_ROOT"]` to `str(PROJECT_ROOT.parent)` if and only if not already set. Order: `load_dotenv` → set default → click group definitions. |
| `drill/engine.py:233`, `drill/setup.py:25` | Direct `os.environ["SUPERPOWERS_ROOT"]` access (KeyError if unset) | Unchanged. The CLI startup hook guarantees the env var is set by the time the engine/setup execute. |
| `backends/claude*.yaml` (5 files) | `${SUPERPOWERS_ROOT}` substituted in `args` for `--plugin-dir` | Unchanged. YAML substitution reads `os.environ` at backend-load time, which is after CLI startup. |
| `backends/codex.yaml`, `backends/gemini.yaml` | `SUPERPOWERS_ROOT` in `required_env` only | Drop from `required_env` (the helper supplies it). `claude*.yaml` keep `required_env` for backward compat (env var works as override). |
| `evals/tests/test_backend.py` | Tests assert `SUPERPOWERS_ROOT` is in `required_env` lists, plus path-resolution tests | Update tests to match the new contract: helper-supplied default, env override still works, `required_env` no longer required for codex/gemini. |
| `evals/README.md` | "export SUPERPOWERS_ROOT=/path/to/superpowers" | Drop the export line; note that the env var auto-defaults to the parent of `evals/`; mention the only required setup is `ANTHROPIC_API_KEY` (or `OPENAI_API_KEY` / Gemini auth). |
| `evals/CLAUDE.md` | Same | Same |
| `evals/.gitignore` | drill's existing patterns (`results/`, `.venv/`, `__pycache__/`, `.env`, `*.pyc`, `*.egg-info/`, `dist/`, `build/`, `.claude/`) | Copied verbatim. Patterns are relative to file location, so they apply correctly under `evals/`. |
| `evals/lefthook.yml` | drill ships `lefthook.yml` defining `pre-commit: uv run ruff check && uv run ty check` | Move to `evals/lefthook.yml`. Either (a) install lefthook at the superpowers root and have it federate to `evals/lefthook.yml`, or (b) document that contributors run `cd evals && lefthook run pre-commit` manually. **Decision in implementation: option (b) for simplicity** — superpowers' top-level workflow doesn't change. |
`.env` placement: keep `evals/.env` (gitignored). Contributors source it from there or set `ANTHROPIC_API_KEY` in their shell environment.
**Top-level superpowers files needing small additions:**
- `superpowers/.gitignore`: add `evals/results/`, `evals/.venv/`, `evals/.env` (belt-and-suspenders; evals/.gitignore already covers these locally).
- `superpowers/CLAUDE.md`: add a one-line pointer "Eval harness lives at `evals/` — see `evals/README.md`" so agents discover it.
- `superpowers/docs/testing.md`: split into "## Plugin tests" (existing tests/ content, with the deleted-test references trimmed) and "## Skill behavior evals" (one-paragraph summary + pointer to `evals/`).
- `superpowers/README.md`: add a single line in the Contributing section pointing at `evals/` for skill-behavior testing.
## Migration ordering
Each step is a separate commit (or small group of commits). Step 2 is the biggest single commit (the verbatim drill copy); subsequent steps are small and atomic.
```
1. Branch off `dev` (f/evals-lift)
2. Copy drill repo into evals/ (single commit, easy to revert)
├─ Record drill SHA at copy time → commit message
├─ Use `rsync -a --exclude=.git --exclude=.venv --exclude=results
│ --exclude=.env --exclude=__pycache__ --exclude='*.egg-info'
│ --exclude=.private-journal /path/to/drill/ evals/`
│ (rsync chosen over `cp -r` for explicit excludes; verify with
│ `find evals -name '.git' -type d` returns nothing)
├─ Subagent gate: per-file SHA-256 checksum matches drill repo for every
│ non-excluded file; excluded paths absent from evals/
└─ Smoke check: `cd evals && uv sync` succeeds (proves install only;
not a behavioral test)
3. Update path defaults
├─ Add _set_superpowers_root_default() helper to drill/cli.py
├─ Wire it after load_dotenv, before click group definition
├─ Update evals/README.md and evals/CLAUDE.md (drop SUPERPOWERS_ROOT install step)
├─ Drop SUPERPOWERS_ROOT from required_env in codex.yaml/gemini.yaml
│ (keep in claude*.yaml as override)
└─ Update evals/tests/test_backend.py to match new contract
4. Validate from new location (TWO checks)
├─ Run drill's own pytest: `cd evals && uv run pytest` — must pass
└─ Run cheap drill scenario: `cd evals && uv run drill run
triggering-test-driven-development -b claude` — must pass.
Real behavioral validation, not just code review.
5. Bash test deletion phase — per-file with subagent gate
For each file in the candidate-deletion list:
a. Subagent compares bash test assertions vs drill scenario verify block
b. Pass criterion: every bash assertion has a matching drill check
c. If pass → delete the bash test file (one commit per file or per
coherent group)
d. If fail → either extend drill scenario (separate commit + verify) or
keep the bash test (no commit)
6. Stale-reference scrub
├─ Subagent greps the superpowers tree (excluding node_modules/, .venv/,
│ evals/) for deleted file paths
├─ Search targets: docs/, docs/superpowers/plans/, RELEASE-NOTES.md,
│ CLAUDE.md, GEMINI.md, AGENTS.md, README.md, .github/, scripts/,
│ .opencode/INSTALL.md, .codex-plugin/INSTALL.md, lefthook.yml
├─ Update active references (e.g., docs/testing.md, README.md install)
└─ Historical references in docs/superpowers/plans/*.md and
RELEASE-NOTES.md are PRESERVED with a brief annotation
("(test removed; behavior covered by drill scenario X)") rather
than rewritten — these are dated artifacts, not living docs.
7. Top-level docs
├─ docs/testing.md split
├─ CLAUDE.md pointer
└─ README.md Contributing section
8. Re-run smoke checks (regression gate)
├─ `cd evals && uv run pytest`
└─ `cd evals && uv run drill run triggering-test-driven-development -b claude`
9. Final adversarial review
└─ Two parallel subagents, full diff, "5 points to whoever finds the
most legitimate issues" framing. Address findings before push.
10. Push branch + open PR against dev
└─ PR description includes: drill SHA pinned at copy, archival action
item ("after merge: archive obra/drill, add README pointer to
obra/superpowers/evals/"), per-deleted-file coverage receipts.
```
## Verification (post-implementation)
The implementation plan must show:
- All non-excluded drill source files present at `evals/` after step 2 (subagent **per-file SHA-256 checksum diff** vs `obra/drill@<recorded-sha>`).
- Excluded paths (`.git/`, `.venv/`, `results/`, `.env`, `__pycache__/`, `*.egg-info/`, `.private-journal/`) absent from `evals/`.
- The step-2 commit message records the drill source SHA.
- `cd evals && uv sync` succeeds without `SUPERPOWERS_ROOT` set.
- `cd evals && uv run pytest` passes (drill's own pytest suite).
- `cd evals && uv run drill list` returns the same scenario count as the standalone drill repo at the recorded SHA.
- `cd evals && uv run drill run triggering-test-driven-development -b claude` passes (proves path defaults work end-to-end).
- For each deleted bash test: subagent verification table in the commit message showing every assertion mapped to a drill check.
- Grep for deleted file paths returns zero hits across living superpowers docs (post step 6); historical refs in `docs/superpowers/plans/*.md` and `RELEASE-NOTES.md` are annotated, not rewritten.
- `docs/testing.md` has both "Plugin tests" and "Skill behavior evals" sections.
- The drill repo's history is untouched; `obra/drill` is unaffected by this PR.
- PR description names the action item to archive `obra/drill` after merge.
## Open questions
None. All clarifying decisions have been made:
| Question | Decision |
|----------|----------|
| Where does drill live in superpowers? | `evals/` (rename from drill); standalone repo archived as separate step |
| Fate of redundant bash tests? | Delete per-file with subagent verification of coverage; default keep |
| Scenarios layout? | Centralized at `evals/scenarios/` |
| Python toolchain placement? | Self-contained at `evals/` |
| CI integration? | Manual-only this PR; documented future path |
| Migration mechanics? | Plain copy; drill repo's history preserved in archived repo, not in-tree |
| Internal Python package name? | Keep as `drill` (directory is `evals/`) |
| Branching strategy? | Independent off `dev` (not stacked on `f/cross-platform`) |
@@ -0,0 +1,160 @@
# SDD Task-Scoped Review Dispatch
Make subagent-driven-development's per-task reviews cheaper and faster without weakening them, by scoping per-task review prompts to the task and stopping redundant work — while final branch review stays broad.
## Problem
Per-task code quality reviewers in SDD routinely do branch-review-scale work on single-task diffs. Evidence from two real local SDD sessions: `a1a6719a-6109-453a-9933-34ae396f5bae` (sen-core-v2) and `0cc1a12d-9984-4c35-8615-9d42dadb2c47` (serf), both under `~/.claude/projects/`:
- In the sen-core-v2 session, 7/8 quality reviewers ran repo-wide greps; the most expensive ran 50+ Bash commands over ~200 seconds. Across both sessions, quality reviewers cost 4-8× what spec reviewers cost on the same tasks.
- Spec reviewers, whose prompt contains "Only read files in this diff. Do not crawl the broader codebase," stayed tight: 6-16 tool calls, 14-65 seconds.
- No reviewer ran heavy tests autonomously. Every package-wide or repeated test run observed was explicitly requested by a controller-written prompt ("check all uses," "run tests if useful, especially race-focused ones," "does anything else read `Meta()`?").
Root causes, in order of impact:
1. **The per-task quality prompt inherits a merge-readiness review.** `code-quality-reviewer-prompt.md` delegates to `requesting-code-review/code-reviewer.md`, which asks about architecture, scalability, security, production readiness, and ends with "Ready to merge?" That frame licenses branch-level breadth on a one-task diff. The spec prompt's diff-scope guard was never carried over.
2. **The controller gets no guidance on writing reviewer prompts**, so it invents open-ended directives ("check all uses") that reviewers interpret literally.
3. **Duplicated work across the pipeline.** The quality template's "Plan alignment" dimension re-checks what the spec reviewer just verified. Reviewers re-run test suites the implementer already ran (and reported, with TDD evidence) on identical code.
4. **Per-task and final review share one template**, so there is no representation of "per-task narrow, final broad" anywhere.
A field report (`~/2026-06-09-code-quality-reviewer-scope-budget-issue.md`) first flagged this. Its cited session and headline numbers could not be verified, but its qualitative diagnosis was confirmed against two real local sessions. One correction to it: cross-cutting audits (lock ordering, changed contracts) are sometimes the *correct* review method — the fix must gate breadth behind a stated concrete risk, not forbid it.
## Goals
- Per-task reviews scoped to the task: diff-first reading, justified broadening, no redundant test runs.
- Final whole-branch review keeps its current breadth.
- No reduction in what reviews catch.
## Non-goals / explicitly preserved
- **Full re-reviews stay.** When a reviewer re-reviews after a fix, it still reviews the whole task at full reading breadth. (It does not re-run tests the implementer just ran on the amended code.) This deliberately rejects the field report's "re-review budget" remedy: the cost of its worst cited example (a re-review running `-race` and `-count=100` loops) is curbed by the test budget below, not by narrowing what re-reviewers read.
- ~~**The two review stages stay separate.** Spec compliance and code quality remain independent subagents, serially gated. No merging.~~ **Superseded by the cost iterations below**: live eval economics showed per-dispatch overhead dominating cost, and the maintainer put everything on the table. The per-task stages are now one task reviewer with two verdicts; the independent broad final review remains.
- **The coordinator keeps model judgment.** No forced model tier for reviews, in either direction.
- **`requesting-code-review/` is untouched.** It remains the broad template for final branch review and ad-hoc review.
- Verdict ordering (spec compliance reported before quality), the fix-and-re-review loops, and the requirement to fix Critical/Important findings are unchanged.
## Cost iterations (post-launch eval economics)
Live before/after runs surfaced a cost regression once the quality-hardening
prose (evidence rule, constraint carrying, pristine output) landed: go-fractals
went from 42.8 min / 14.5M tokens (first task-scoped version) to 69.9 min /
32.2M (hardened version) while reaching baseline-parity quality (blind-judged
8.5 vs 8.5). Per-subagent turn profiling attributed cost to, in order: cheap
models taking 2-3× the turns on multi-step work (678 of 1197 subagent turns
were haiku), per-dispatch overhead (3 subagent spin-ups per task, each
re-deriving the diff; controller coordination was half the dollars), and
evidence-rule narration.
- **Iteration 1:** turn-count-beats-token-price model guidance (mid-tier floor
for multi-step work), optional inline diffs, cite-don't-narrate evidence,
Important = cannot-trust-until-fixed, fixes dispatched only for
Critical/Important. Result: 68.2 min / 22.9M — tokens down 29%, wall-clock
flat; controllers pasted the diff in only 2 of 22 review dispatches when
phrasing was optional.
- **Iteration 2:** per-task spec and quality reviews merged into one
`task-reviewer-prompt.md` (one reviewer, one reading of the diff, two
verdicts; one fix dispatch addresses both kinds of findings); implementers
run the focused test while iterating, full suite once before commit.
Result (go-fractals): 47.5 min / 15.7M / $13.55 — beat baseline on every
axis, blind-judged 9/10 vs baseline 7/10.
- **Iteration 3:** Calibration names merge-blocking maintainability damage
(verbatim duplication, swallowed errors, assertion-free tests) as
Important and Minor findings must be pasted into the final review for
triage; reviewer skepticism extended to the implementer's design
rationales ("left it per YAGNI" is a claim, not a verdict); diff handed
to reviewers as a file (`git diff > /tmp/sdd-task-N.diff`, redirected so
it never enters the controller's context; one Read call for the
reviewer) after paste-into-prompt guidance went unadopted (0-6 of 11-17
dispatches) for locally-rational context-economics reasons.
- **Final frozen config (e355795), all five scenarios pass:** go-fractals
44.4 min / 13.4M / $11.67 (-32% time, -37% tokens, -27% dollars vs
baseline); svelte-todo 62.8 / 19.7M / $15.76 (-21% / -28% / -25%);
rejects-extra-features $1.31 (vs $1.88); spec-reviewer-flaws flat; the
planted-defect scenario (v3: open-flag transparency bar for judgment
calls, must-fix bar for a test whose name promises verification it
never performs) passes with the defect caught and fixed.
### Iterations 4-5 (2026-06-10): variance honesty, structural fixes, positive recipes
A same-config re-run exposed run-to-run variance (44.4→57.1 min on
identical prompts; reviewer escape-hatch appetite swung 1.0→6.3 tool
calls/review), so all subsequent claims use ranges. Five parallel
experiment variants on go-fractals plus transcript mining of real local
sessions (full log with negative results:
`evals/docs/experiments/2026-06-10-sdd-cost-experiments.md`) produced the
final config:
- **Adopted:** final-review package (final reviewer 33→6 turns at
controller-model prices); REQUIRED `model:` line in both templates
(prose guidance decayed mid-session once, inheriting opus for 17
dispatches, +$5); task-brief + report files (`scripts/task-brief`;
fidelity anchor, modest context savings); progress ledger in
`<git-dir>/sdd/progress.md` (real sessions re-dispatched entire
completed task sequences after compaction — 269 dispatches for ~22
tasks); omnibus final fixer (a real session's per-finding fix wave cost
more than all its tasks); scoped fix tests; unique SHA-range collateral
names (worktree/submodule-safe); dispatch-composition recipe and
reviewer named-risk budget (micro-tested: positive recipe 3.0
transcribed values vs prohibition 4.4 vs control 3.6 — prohibitions can
backfire; see `2026-06-10-positive-instruction-redesign-design.md`).
- **Tested and declined:** controller turn batching and parallel-call
pipelining (controller emits exactly one tool call per message — 0
multi-tool messages in every run; 46% of its turns are
thinking/narration, a prompt-immune floor); background-dispatch
pipelining (mechanism adopted 7/28 but benefit below the ±6 min noise
floor on these scenarios).
- **Final validated config (b81f35b family), all gates pass:** go-fractals
54.1-54.7 min / 14.4-16.6M / $12.81-14.31 (baseline 64.9 / 21.2M /
$16.07); svelte-todo 55.0 min / 19.3M / $14.99 (baseline 79.7 / 27.3M /
$20.98); planted-defect pass / $2.77. Across all 8 same-design fractals
runs: 44.4-57.1 min / 13.4-20.0M / $11.67-14.84 — the worst draw beats
baseline on every axis; typical mid-band savings ~20-25%.
## Design
### Shared principle: don't re-run tests on code that hasn't changed
The implementer's report includes test results and TDD RED/GREEN evidence for exactly the code under review. Reviewers verify by reading. A reviewer runs a test only when reading raises a specific doubt that no existing run answers — and then a focused test, not a suite. On harnesses where reviewer subagents are read-only (e.g., Antigravity maps reviewer templates to the `research` type, which has no command access), the reviewer instead names the test it would run in its report.
After a fix, the implementer re-runs the tests covering the amended code; the re-reviewer does not repeat that run. Today nothing enforces that premise: `implementer-prompt.md` describes the initial implement-test-commit flow only, with no fix-iteration instruction. This spec therefore also adds to `implementer-prompt.md`: after fixing a review finding, re-run the tests that cover the amended code and include the results in the fix report.
This principle appears in both reviewer prompts, the implementer prompt, and the controller guidance.
### 1. New file: `skills/subagent-driven-development/code-quality-reviewer-prompt.md` becomes self-contained
Stop delegating to `requesting-code-review/code-reviewer.md`. The per-task quality reviewer gets its own scoped prompt template:
- **Framing:** "You are reviewing one task's implementation for code quality." A task-scoped gate, not a merge review.
- **Spec compliance is settled:** spec review already passed; do not re-litigate requirements or plan alignment.
- **Review dimensions kept:** code quality (clarity, duplication, error handling), test quality (real behavior, not mocks), maintainability, and the existing SDD-specific checks (single responsibility, independent testability, file structure from plan, file growth contributed by this change). Dropped: plan alignment, security/scalability/production-readiness dimensions, merge verdict.
- **Scope budget:** start from `git diff BASE..HEAD`; read changed files first; inspect adjacent code only to evaluate a concrete risk you can name. Cross-cutting changes — lock ordering, changed function/API contracts, shared mutable state — are legitimate named risks that justify checking call sites. Do not crawl the codebase by default.
- **Test budget:** the shared principle above, plus: no package-wide suites, race detectors, or repeated/high-count runs unless you have first named a specific suspected flake or race. Otherwise, recommend heavy validation in the report instead of running it. Warnings or noise in the implementer's reported test output are findings — output should be pristine (the implementer's self-review checks this too).
- **Evidence rule:** reviewers answer each What-to-Check item with file:line evidence, not bare yes/no. (Added after live eval runs showed reviewers passing defects the prompt had pointed them at — an accessible-name check and a temp-dir-cleanup check both got unsupported "yes" answers while the defect sat in the reviewed diff.)
- **Read-only rule** kept in trimmed form: no mutating the working tree, index, HEAD, or branch state. The `git worktree add` how-to sentence from the current templates is NOT carried into this file — a diff-scoped review never needs a checkout of another revision (same rationale as the spec-prompt cleanup below).
- **Verdict:** Strengths / Issues (Critical/Important/Minor) / "Task quality: Approved | Needs fixes."
### 2. `skills/subagent-driven-development/spec-reviewer-prompt.md` cleanups
- Remove the `git worktree add` how-to sentence. The read-only rule stays; a diff-scoped spec review never needs a checkout of another revision.
- Resolve the tension between the diff-only guard and "verify everything independently": spec compliance is judged by reading the diff against the requirements. The implementer's TDD evidence covers "it runs" — apply the shared test principle.
- New third verdict channel: requirements that cannot be verified from the diff (live in unchanged code, span tasks) are reported as explicit "⚠️ Cannot verify from diff — controller should check X" items, instead of either crawling or silently passing. The flowchart's binary pass/fail diamond cannot route this, so the controller guidance (§3) defines the handling: ⚠️ items do not block dispatching the quality reviewer, but the controller must resolve each one itself (it holds the plan and cross-task context) before marking the task complete; an item the controller confirms is a real gap is treated as a failed spec review and goes back to the implementer.
- Replace the fabricated premise "The implementer finished suspiciously quickly" with grounded skepticism: treat the implementer's report as unverified claims about the code. Same distrust, no invented fact.
### 3. `skills/subagent-driven-development/SKILL.md` controller changes
- **Model Selection:** replace "Architecture, design, and review tasks: use the most capable available model" with judgment guidance — pick reviewer models the way implementer models are picked, scaled to the diff's size, complexity, and risk. The "Task complexity signals" list is rescoped to make clear its bullets describe implementation tasks; reviewer model choice follows the same judgment, so a narrow diff review does not automatically map to "broad codebase understanding → most capable model."
- **Reviewer prompt construction** (new guidance near Red Flags): when dispatching reviewers, do not write open-ended directives ("check all uses," "run race tests if useful") without a concrete task-specific reason; do not ask reviewers to re-run tests the implementer already ran on the same code; do not pre-judge findings for the reviewer (never instruct a reviewer to ignore or not flag a specific issue — adjudicate suspected false positives in the review loop instead); per-task reviews are task-scoped gates — the broad review happens once, at the final whole-branch review. (The pre-judging rule was added after a live eval run caught the controller fabricating a "the plan forbids a shared helper" claim and instructing the quality reviewer not to flag a planted DRY violation.) Controllers must also include the spec/design's global constraints that bind the task — version floors, naming and copy rules, platform requirements — in the requirements they paste: a live run shipped a `go 1.26.1` module floor against a "Go 1.21+" design because no reviewer ever saw the constraint. And controllers must specify a model explicitly on every dispatch — an omitted model inherits the session's (usually most expensive) model, which silently defeats model selection.
- **Handling spec-reviewer ⚠️ items** (new guidance, alongside Handling Implementer Status): the controller resolves each "cannot verify from diff" item itself before marking the task complete; confirmed gaps go back to the implementer as failed spec review.
- **Final review stays broad, explicitly:** the final whole-branch reviewer dispatch node gains an explicit pointer to `../requesting-code-review/code-reviewer.md`. (Today that template is reachable only through the per-task quality prompt's delegation; once that delegation is removed, an unreferenced final-review template would be orphaned.) The Integration section's note that `superpowers:requesting-code-review` provides "the code review template for reviewer subagents" is corrected to apply to the final review only.
- **Example workflow:** the quality-reviewer lines in the example are updated to the new verdict vocabulary ("Task quality: Approved"); the final reviewer's "ready to merge" line stays.
- Flowchart topology is unchanged; the ⚠️ channel is handled by controller guidance, not a new graph branch.
## What this does not fix (known, deferred)
The spec reviewer judges against task text the controller pasted; it cannot catch requirements dropped during the controller's extraction from the plan. That is an architectural property of "controller provides full text," not a prompt problem, and is out of scope here.
## Verification
- Plugin infrastructure tests (`tests/`) still pass.
- Run the SDD skill-behavior evals (`git submodule update --init evals`, then per `evals/README.md`) before and after the change. Specifically: `sdd-go-fractals`, `sdd-svelte-todo`, `sdd-rejects-extra-features` (end-to-end SDD including the spec reviewer's YAGNI gate), and `spec-reviewer-catches-planted-flaws`.
- Known eval gaps this change exposes: no existing scenario plants a code-quality defect inside a single SDD task and asserts the per-task quality reviewer catches it, and no scenario measures per-reviewer exploration cost (tool-call/grep counts). Add one scenario covering the first gap (planted single-task quality defect → per-task reviewer must flag it before final review). For exploration cost, compare reviewer subagent tool-call counts manually across the before/after eval transcripts.
@@ -0,0 +1,178 @@
# Positive-Instruction Redesign of Skill Guidance — Design Spec
**Status:** Proposed (follow-up to the 2026-06-09 SDD review-dispatch work; separate PR per the one-problem-per-PR rule)
**Driver:** Measured evidence (2026-06-10) that some negative instructions in skill prose backfire, while others work — and that the difference is predictable.
## The measured finding this spec generalizes
Micro-tests on 2026-06-10 (opus, 5 reps per phrasing, programmatic scoring;
harness described below) measured how guidance phrasing changes what a
controller composes:
| Case | Phrasing | Result |
|---|---|---|
| Dispatch composition ("don't restate the brief") | prohibition | **4.4** spec values re-typed — *worse than no guidance* (3.6) |
| Dispatch composition | positive recipe ("your dispatch should contain: (1)…(5)") | **3.0, zero variance** — adopted |
| Dispatch composition | recipe + nuance clause ("quote only the fragment…") | 3.8, noisy — nuance dilutes recipes |
| Test-rerun directive ("do not ask reviewer to re-run tests") | prohibition | **0/5 violations** — works fine (control: 3/5) |
| Test-rerun directive | positive recipe | 0/5 — equal, but longer |
**The doctrine** (use this to classify any negative instruction):
1. **Tripwires work.** Phrase-level self-checks on concrete tokens ("if the
prompt you are writing contains 'do not flag' … stop") fire reliably.
2. **Recognition tables work.** Red-Flags/rationalization tables read at
decision time, not composition time.
3. **Discrete-directive prohibitions work.** "Do not ask X to do Y" holds
when the model has no competing incentive to do Y.
4. **Composition prohibitions backfire** when the model has its own agenda
for the output (e.g., restating specs feels like helpful curation).
Only a positive composition recipe moves these — and adding nuance
clauses to a winning recipe makes it worse, not better.
5. **Ties go to the shorter phrasing.** Codex re-reads SKILL.md ~500× per
long session (measured 2026-06-10); prose length is a real cost.
## Audit results (2026-06-10, all ~30 skills + prompt templates)
Counts: 3 tripwires (keep), 14 recognition tables (keep), ~20 policy gates
(keep — "never push without permission" is policy, not composition
shaping), 5 composition-prohibitions:
| # | Location | Disposition |
|---|---|---|
| 1 | `subagent-driven-development/task-reviewer-prompt.md` — "Cite, don't narrate" | **Queued in PR #1717 batch**: lead with the positive half ("Your report should point at evidence: file:line for every finding…"), drop the prohibition half (dead weight — the positive half already exists and carries the load) |
| 2 | `subagent-driven-development/SKILL.md` — "Do not add open-ended directives" | **Keep as-is**: micro-test could not elicit the failure in 15 samples; no evidence either way; shorter wins |
| 3 | `subagent-driven-development/SKILL.md` — "Do not ask a reviewer to re-run tests" | **Keep as-is**: measured 0/5 violations; the prohibition also usefully propagates itself into dispatches |
| 4 | `subagent-driven-development/SKILL.md` — "do not re-review on top of it" | **Queued in PR #1717 batch**: replace with the three-element checklist ("Before re-dispatching the reviewer, confirm the fix report contains: the covering tests, the command run, and the output") |
| 5 | `writing-plans/SKILL.md` — the "No Placeholders" banned-patterns list | **This spec's main subject** — see below |
Borderline, deferred with #5: `task-reviewer-prompt.md` "Don't flag
pre-existing file sizes — focus on what this change contributed" (positive
half present and load-bearing; low impact; test alongside #5 if convenient).
## The writing-plans change (deferred item #5)
### Current state
`skills/writing-plans/SKILL.md`, "No Placeholders": one positive sentence
("Every step must contain the actual content an engineer needs") followed
by a six-bullet banned-patterns list ("never write them: 'TBD', 'TODO',
'Add appropriate error handling', 'Write tests for the above', 'Similar to
Task N', …").
### Why it matters and why it is genuinely uncertain
- Plans are the **largest generated artifact** in the workflow, and the
model has a real competing incentive to emit placeholders (they are the
path of least effort under length pressure) — the incentive structure of
the case where prohibition measurably backfired.
- But the banned items are **discrete, recognizable tokens** — the shape
of the case where prohibition measurably held.
- **The list is load-bearing elsewhere:** the skill's Self-Review section
references it ("Placeholder scan: search your plan for red flags — any
of the patterns from the 'No Placeholders' section above"). The tokens
double as the review-time scan inventory, and review-time recognition is
the category that works. A naive swap to a positive checklist breaks
that reference and discards good tripwire tokens.
### Variants to test
- **V0 (current):** positive sentence + banned list at composition time;
Self-Review references the list.
- **V1 (auditor's checklist):** composition-time positive recipe only —
"Before finalizing a step, confirm it has: the literal code to write, a
runnable command with expected output, types and method names defined
within this plan, error handling shown explicitly. A step is complete
when an engineer could implement it without asking any follow-up
questions." Self-Review keeps a generic placeholder scan.
- **V2 (restructure by mechanism — predicted winner):** composition time
gets only V1's positive recipe; the named patterns move wholesale into
the Self-Review placeholder-scan step, reframed as recognition ("when
you scan, look for: 'TBD', 'TODO', 'Similar to Task N', …"). Same
tokens, relocated from the category that primes to the category that
detects.
- **V3 (control):** positive sentence only, no list anywhere.
### Micro-test design
- **Task:** opus writes a 2-3 task implementation plan from a deliberately
under-specified spec (under-specification is what tempts placeholders).
Use a fixture spec with: one well-specified task, one task whose error
handling the spec hand-waves, one task similar to the first (tempting
"Similar to Task 1").
- **Sampling:** 5+ reps per variant, default temperature, model
`claude-opus-4-8` (the model that writes plans in practice).
- **Programmatic scoring** (lower is better unless noted):
- banned-token count: `TBD|TODO|implement later|fill in details|appropriate error handling|handle edge cases|Similar to Task|Write tests for the above`
- steps lacking a fenced code block where the step changes code
- references to types/functions not defined anywhere in the plan output
- (higher is better) runnable commands with expected output per task
- **Two-stage scoring for V2:** also test the Self-Review half — feed each
generated plan back with the variant's Self-Review section and measure
whether the scan actually catches seeded placeholders (insert 2 known
placeholders into a fixture plan; detection rate is the metric).
- **Acceptance:** adopt a variant only if it beats V0 on banned-token count
without losing code-block coverage or self-review detection rate.
Expected cost: ~$6-10 total.
### PR scoping
Separate PR (writing-plans is a different skill; its "No Placeholders"
list is tuned content where the contributor guidelines demand eval
evidence). The PR must include: the micro-test harness + results table,
before/after text, and the V2 relocation rationale.
## The micro-test harness (method, so it isn't lost)
`/tmp/sdd-exp/micro/run-micro.py` and `/tmp/sdd-exp/micro2/run-micro2.py`
(2026-06-10; to be committed to superpowers-evals as
`docs/superpowers/skills/micro-testing-prompt-guidance.md` + scripts):
- One API call per sample: system prompt = the skill-guidance variant in
realistic surrounding context; user = a realistic mid-workflow scenario;
output = the composed artifact (dispatch prompt, plan, report).
- Programmatic scoring with greps for unambiguous markers; **manually
inspect every match before trusting a verdict** — one of tonight's
"violations" was the controller correctly quoting the prohibition, and
automated negation detection mislabeled another.
- ~$0.15-0.30/sample, seconds per iteration vs $12/50-min full eval runs.
Iterate phrasings here; confirm winners in full runs only when the
change is structural.
- Always include a no-guidance control — tonight it revealed both a
backfire (restating: prohibition worse than nothing) and a working
prohibition (test-reruns: 3/5 control failures vs 0/5 with either
phrasing).
## Result: writing-plans micro-test (run 2026-06-10, after this spec was written)
**Resolved — no change needed.** Stage 1 (3-task spec, no pressure): 0
placeholders in all 20 plans across all four variants including the
no-guidance control. Stage 1b (10-task spec, five near-identical commands
tempting "Similar to Task N", explicit ~2,500-word economy target): 40/40
clean — the single regex hit was a V2 self-review *attesting* "no
TBD/TODO ✓". Current-generation opus does not produce plan placeholders
even under deliberate pressure, with or without the banned-patterns list.
Disposition: leave the No Placeholders section exactly as it is (it costs
little and the counterfactual is unmeasurable); do NOT open the follow-up
PR. The V2 relocation design remains on file here should a future model
generation regress.
## Also explicitly not-dropped (tested-and-declined, with data)
Recorded so nobody re-proposes them without new evidence — full numbers in
the 2026-06-09 SDD design spec's Cost-iterations section:
- **Controller turn batching / parallel tool calls in one message:** the
controller emits exactly one tool call per message (0 multi-tool
messages across every measured run, with and without guidance). 46% of
controller turns are thinking/narration with no tool call — a
prompt-immune floor.
- **Pipelined reviews via parallel calls:** dead for the same reason.
- **Pipelined reviews via `run_in_background`:** mechanism adopted when
offered (7/28 dispatches) but benefit below the run-to-run noise floor
on 45-min scenarios (reviews are only ~30-60s each); adds dual
result-stream coordination. Worth revisiting only for plans whose
reviews are individually long.
- **Nuance clauses appended to winning recipes:** measurably degrade them
(C2: 3.8 noisy vs C: 3.0 consistent). Iterate by re-deriving the recipe,
not by appending caveats.
@@ -0,0 +1,265 @@
# Strict-Cost SDD — Design Spec
**Status:** Proposed experiment ladder (not implementation). Each rung ships
only with its gate evidence; abort any rung whose gates fail.
**Objective:** minimize dollars per plan-execution. Wall-clock is
unconstrained; token count matters only as a cost driver.
**Hard invariant:** quality. Concretely: `sdd-quality-reviewer-catches-
planted-defect` pass rate over **N=5 runs** (not 1 — single-run gates were
this campaign's weakest methodology), `sdd-rejects-extra-features` pass,
all end-to-end scenarios pass, blind A/B deliverable parity with the
current config. Any quality regression kills the rung, full stop.
## Where the dollars are (final 2026-06-10 config, go-fractals, ~$13/run)
| Component | $ | Driver |
|---|---|---|
| Controller (session model, opus) | ~6-7 | ~150 turns × resident context; prompt-immune turn floor (46% thinking/narration) |
| Implementers (sonnet, 10-13 dispatches) | ~5-6 | the actual work; ~25 turns each; ~13 pre-edit exploration calls each |
| Task reviewers (sonnet, 10) | ~1-1.5 | 3-9 turns each with package |
| Final review + fixes | ~1 | 6 turns with branch package |
Review-loop count (2-4 per run) is the biggest run-to-run cost variance;
loops are mostly caused by plan ambiguity the implementer resolved wrongly.
## Judgment guardrail (co-invariant with quality)
**Cheapen mechanics, never judgment.** Every rung must enumerate which
decisions it moves to a cheaper model and show each is *mechanical*
deterministic, scriptable, or cheaply verifiable after the fact. Judgment
stays at the highest tier or with the human. The judgment points in SDD,
explicitly:
- **BLOCKED / NEEDS_CONTEXT handling** — diagnosing why a subagent is stuck
and choosing the remedy
- **⚠️ "cannot verify from diff" resolution** — the controller adjudicating
with cross-task context
- **Dispatch curation** — ambiguity resolution and task-boundary drawing
(measured load-bearing: the Task 5 gradient-direction note prevented a
wrong implementation)
- **Review verdicts and severity calibration** — what is Important vs Minor
- **Review-loop adjudication** — deciding a finding is a false positive
- **Escalate-to-human recognition** — knowing the plan itself is wrong
A rung that would move any of these to a cheaper model must either (a)
restructure so the decision is made once by the expensive model at plan
time, (b) add an explicit escalation rule routing it back up at execution
time, or (c) die. "The cheap model usually gets it right" is not
acceptance evidence — judgment failures are rare-event, high-blast-radius,
and largely invisible to pass/fail gates, which is why every tier change
below carries a judgment audit (session-resume interrogation of each
judgment point in the gate runs, compared against the expensive-controller
baseline) in addition to the N=5 scenario gates.
## Thesis guardrail
SDD's thesis: **a fresh subagent per task with precisely curated context,
gated per task.** Rungs below must preserve it. Dispatch-time task batching
(one implementer dispatch handling several plan tasks) is **counter-thesis**
— it pollutes the fresh-context property and coarsens the gates — and is
deliberately NOT on the ladder. The thesis-compatible route to the same
dispatch economics is plan-time task right-sizing (L1): if the plan defines
fewer, better-sized tasks, SDD still runs one fresh subagent per task.
## The ladder (in expected $/leverage order)
### L1 — Plan-side crispness (writing-plans changes; est. $1.5-3/run, plus variance reduction)
**Status 2026-06-11 (final): elicitation tested end-to-end; claims
re-attributed.** Micro-tests: constraints header and Interfaces blocks
elicit deterministically (0→5/5, 0→100% of tasks, exact values);
right-sizing is modest and scale-dependent (9.4→8.4 tasks at svelte
scale, nothing to move at fractals scale). Full runs: an elicited plan
executed at $6.34/$8.49 — but the no-guidance control (opus plan,
complete code) hit $7.59/$7.73, inside that range. **The cost win
belongs to opus-written complete-code plans; the hand-written prose
fixture plans all prior numbers used are unrepresentative and ~2×
costlier to execute.** The guidance owns fidelity and variance instead:
deterministic constraints propagation (the one elicited-run fix was a
version-floor catch), exact cross-task interfaces, fix waves 1 vs 2-4
(the control plan shipped a real Sierpinski bug both runs had to fix).
The writing-plans PR claims those grounds, not dollars. Draft at
/tmp/sdd-exp/writing-plans-l1 (branch writing-plans-crisp).
The plan is upstream of every cost: task count sets dispatch count; plan
ambiguity sets review-loop count; plan completeness sets implementer
exploration. Current writing-plans optimizes for implementer success, not
execution economics. Changes to test:
1. **Task right-sizing guidance.** Today's plans produce tasks as small as
"create .gitignore" — each costing a full dispatch + review cycle
(~$0.60-1.00 fixed overhead). Add: "A task is the smallest unit that
carries its own test cycle and is worth a fresh reviewer's gate. Merge
setup/config steps into the task that needs them; split only at
boundaries where a reviewer could meaningfully reject." Fractals' plan
would drop from 10 tasks to ~7. Validate: dispatch count falls, gates
hold, review granularity still catches the planted defect.
2. **Structured `## Global Constraints` section** in the plan header
(version floors, naming/copy rules, platform requirements). Today these
live in design.md prose and reach reviewers only if the controller
remembers to paste them (a `go 1.26.1` floor violation shipped because
none did). A fixed heading makes them mechanically extractable —
`task-brief` can append them to every brief automatically (small script
change), removing a controller responsibility entirely.
3. **Per-task `Interfaces:` line** (consumes/produces, exact signatures).
The controller currently re-derives cross-task interfaces per dispatch
(its main legitimate "restating"), and implementers spend ~13 tool calls
re-discovering context. The planner already knows the interfaces; one
line per task moves the work to where it is done once.
4. **Per-task model-tier recommendation** from the planner ("mechanical /
standard / judgment"). The planner has the best information for the
Model Selection decision the controller currently re-makes per dispatch;
the controller keeps override authority.
Validation: micro-test the planner output shape (recipe-style, per the
instruction-design doctrine), then full runs. Note the 2026-06-10 result:
plan *placeholders* cannot be elicited from current opus — these changes
target economics and ambiguity, not placeholder hygiene.
### L2 — Controller tier (est. $4-5/run; the biggest single lever, gated hardest)
**Status 2026-06-11 (final): DIED AT THE GATES, as pre-registered — with
useful anatomy.** Recon was positive ($6.68/$8.05, n=2, mechanics clean).
The full battery split the judgment surface: the new
`sdd-escalates-broken-plan` scenario (explicit plan self-contradiction;
the human never volunteers it) passed **5/5 at sonnet** ($1.02-1.37/run;
opus baseline 2/2) — explicit conflicts get escalated. But the
planted-defect battery failed decisively: under a sonnet controller the
per-task quality gate collapsed into plan-compliance advocacy ("no
assertion, as required" listed under Strengths), the defect shipped in
4/5 runs (deterministic check), and only the tier-pinned opus final
reviewer ever caught it — while the same sonnet-tier reviewers under an
opus controller flagged it 5/5. Cheap controllers handle explicit
escalation; they absorb implicit authority-vs-quality adjudication.
A possible L2b (discrete rule: "a reviewer finding that conflicts with
the plan's text is the human's decision — escalate it") would route the
failing judgment through the escalation behavior that held.
**L2b tested 2026-06-11 (E35/E36, evals
`docs/experiments/2026-06-11-build-loop-autoresearch.md`): improves the
opus stack, does NOT rescue the sonnet rung.** Two rules: a reviewer
tripwire (a plan-mandated defect IS a finding — Important, labeled
plan-mandated; the human decides) and a controller escalation rule
(plan-mandated findings go to the human like any plan contradiction).
Micro on frozen sonnet-composed inputs: 0/6 → 6/6 labeled findings.
Full battery: opus controllers 2/2 internalized the rule, caught their
reviewer's miss as self-described backstop, and escalated for a
sanctioned fix (the 4241 ad-hoc behavior made structural); escalation
sanity 2/2 unbroken. Sonnet controllers: 1/5 full pass — paraphrase
drops the tripwire from dispatches (2/5 transmitted), transmission
alone doesn't fire it live (read-once dilution across the reviewer's
tool reads; placement within the dispatch refuted as the variable),
and no sonnet controller showed backstop behavior; 1/5 shipped the
defect. The L2b rules are a candidate commit for the opus stack.
A future L2c for the sonnet rung would pair the SKILL.md
constraints-recipe (the one channel sonnet transmits verbatim) with a
mandatory output-format slot for plan-mandated findings (the skeleton
survives every observed paraphrase and is consulted at composition
time); untested. Original recon notes follow.
**Recon (superseded):**
Sonnet-controller runs (claude-sonnet coding-agent): all gates green at
**$6.68 and $8.05** / 31-41 min (combo band $11.67-14.84), tokens inside
the combo band — no cheap-controller turn inflation. 26/26 and 31/31
dispatches model-explicit, with heavier (and sane) haiku tiering than
opus controllers showed; review loops, per-task Important→fix→re-review,
and omnibus-fixer rules followed in both runs; the run-1 controller
caught a fixer side-effect (`go mod tidy` removed cobra) before
re-review — real adjudication, not silent absorption. But neither run
surfaced a BLOCKED/⚠️ event (the escalation points were never stressed)
and final reviews ran on sonnet rather than the most capable tier. The
N=5 quality gates + full judgment audit below remain mandatory before
any skill change.
The controller is half the dollars solely because it inherits the session
model. Its turn floor is prompt-immune, so the lever is the rate per turn —
but the controller is also where most judgment points live, so this rung is
designed judgment-first:
1. **Primary form — judgment moved up front, mechanics cheapened:** the
expensive model does the judgment-dense work at plan time (L1's
Interfaces lines, ambiguity resolutions, per-task constraints — i.e.
the dispatch curation is pre-written into the plan). The mid-tier
execution session then runs a loop that is genuinely mechanical:
extract brief, dispatch, run script, route verdicts. Explicit
escalation rules in the skill: on BLOCKED, on any ⚠️ item, on a
suspected false positive, or on anything the plan does not already
answer, the cheap controller STOPS and escalates (to the human, or to
a fresh expensive-model consultation dispatch) — it never resolves
judgment alone.
2. **Gates beyond the standard N=5:** a judgment audit — every
BLOCKED/⚠️/adjudication event in the gate runs interrogated via
session-resume and scored against how the opus-controller baseline
handled the same class of event; any silently-absorbed judgment call
(cheap controller resolving what it should have escalated) fails the
rung regardless of scenario verdicts.
3. **User authority preserved:** the skill recommends, never enforces, the
execution-session tier.
Caveat from this campaign: cheap-model turn inflation was measured on
multi-step *work*, not dispatch loops; whether a mid-tier controller holds
~150 turns is part of what the experiment determines.
### L3 — Reviewer tier (est. $0.7-1/run; most likely rung to die on the judgment guardrail)
**Status 2026-06-11: DEAD, as pre-registered.** Planted-defect ×5 with
forced-haiku task reviewers: 2 pass / 1 indeterminate / 2 fail (baseline
5/5); per-task haiku cleanly flagged 0 of 10 planted defects at correct
severity — 1 found-but-downgraded with the exact prohibited rationale,
9 missed or rationalized (DRY praised as YAGNI; assert-nothing test
called plan-compliant). Cheap reviewers fail by *advocating* for
defects; passing runs survived only on controller redundancy or the
final review. Recorded in the experiments log, Batch A-E. Do not
re-propose without a structurally different design.
The package reviewer is near-single-step mechanically (3 turns / 1 Read
when calm), which invalidates the original turn-inflation rationale for the
mid-tier floor — but reviewing is judgment through and through: severity
calibration, spec verdicts, knowing what not to flag. Mechanical cheapness
does not make the decisions mechanical. Test haiku-with-package only with
the full judgment battery: planted-defect ×5, a severity-calibration check
(seeded Minor-vs-Important pairs; miscalibration fails the rung), and the
escape-hatch variance re-measured at that tier. Prior expectation: this
rung dies, and that is a fine outcome — it converts "we suspect cheap
reviewers are bad" into recorded evidence.
### L4 — Resident-context diet (est. $0.5-1/run)
- `task-brief --list` mode: controller reads task headings + Global
Constraints, never the full plan (the plan body is already delivered via
briefs).
- Reports trim 15 → 8 lines.
- SKILL.md minification pass (every section added this week re-justified
at composition-recipe density; Codex pays ~10k chars × ~500 re-reads per
long session).
### L5 — Re-litigations (explicitly flagged, maintainer-vetoed or counter-thesis)
Recorded for completeness; each requires Jesse's explicit reversal before
any experiment:
- **Scoped re-reviews** (verify fix + regression scan instead of full
re-review): vetoed 2026-06-09; worth ~$0.50/run at most.
- **Dispatch-time task batching**: counter-thesis (see guardrail). L1.1
is the sanctioned form.
## Budget and sequencing
L1 and L2.1 are independent — run both first (~$80: micro-tests + 2×5-run
gates + A/B). L3 after L2 settles the controller (reviewer behavior depends
on dispatch quality; ~$25 — planted-defect runs are $2-3 each). L4 last
(cheap, but re-gate once after the stack; ~$30). Total ≲ $150 for the full
ladder with honest N=5 gates. Expected end state if every rung survives its gates: **$5-7/run on
fractals (from $12-15)**; if the judgment-sensitive rungs (L2 beyond its
primary form, L3) die as expected, **$8-10/run** — the honest target, since
the guardrail prices judgment above dollars by construction.
## Relationship to existing work
Builds on the 2026-06-09 task-scoped review dispatch design (PR #1717) and
the 2026-06-10 experiment campaign (evals
`docs/experiments/2026-06-10-sdd-cost-experiments.md` — consult the
negative-results section before adding rungs; turn-discipline and
parallel-call mechanisms are dead). Instruction wording for any new prose
follows the positive-instruction doctrine spec and gets micro-tested before
full runs. L1 is a writing-plans change → its own PR with eval evidence;
L2-L4 are SDD changes → separate PR(s).
@@ -0,0 +1,225 @@
# Visual Companion Auth Hardening Design
**Date:** 2026-06-10
**Status:** Draft for Drew review
## Goal
Fix the security and reliability gaps found in PR #1720's brainstorming visual
companion without changing the companion's core workflow or adding runtime
dependencies.
The fixes must be test-first and must leave clear automated evidence for:
- cross-origin browser tabs cannot inject companion events by riding cookies
- restart reconnect works without depending only on browser cookie behavior
- bearer keys do not remain in the visible URL after bootstrap
- `/files/*` cannot serve files outside the content directory
- future same-origin vendored UI libraries still work
## Threat Model
The companion serves agent-generated local UI for a single brainstorming
session. The important assets are:
- screen content served from the companion
- the session key
- `state/events`, which the agent reads as user feedback
- local files under the companion session directory
In scope attackers:
- a malicious browser tab on another `localhost` port
- a browser page that can make requests to the companion but should not be able
to authenticate as the companion UI
- a direct remote client when the server is bound to a non-loopback interface
- accidental leakage through URL history, referrers, or committed local state
- content-directory symlinks or path tricks that escape `/files/*`
Out of scope for this fix:
- malicious agent-authored screen HTML
- malicious same-origin vendored JavaScript loaded by a companion screen
This out-of-scope boundary is intentional. Companion screens are part of the
agent UI surface. They may use inline scripts today and may someday use
same-origin vendored libraries such as Alpine or Three.js. Protecting against
malicious screen HTML would require a larger sandboxed-iframe architecture with
a narrow message bridge; that is not the scope of this PR hardening pass.
## Current Failures
Automated and headed-browser testing found these failures in the PR branch:
1. A cross-origin localhost page can open a cookie-authenticated WebSocket and
write attacker-controlled choices to `state/events` after the real companion
page sets the cookie.
2. `/files/*` serves symlinks that point outside `content/`, including a symlink
to `state/server-info` containing the keyed URL.
3. The session key remains in the URL of the actual screen page, so same-origin
screen JavaScript and accidental referrers/history can see it.
4. The helper reconnects with a keyless `ws://host` URL. In headed Chrome, after
a same-port/same-token restart, the browser stopped presenting the cookie to
the restarted server, so the open tab stayed stuck on the tombstone until a
manual reload.
5. Shell lint and the lifecycle test need cleanup so the test pass is stable in
Codex.
## Design
### 1. Bootstrap Keyed Loads
`GET /?key=<token>` becomes a bootstrap response, not the screen response.
When the key is valid, the server:
1. sets the HttpOnly session cookie as it does today
2. returns a small HTML bootstrap page
3. the bootstrap page stores the key in tab-scoped `sessionStorage`
4. the bootstrap page navigates to `/` using `location.replace('/')`
After this, the visible screen URL is bare `/`, not `/?key=...`.
`GET /` with a valid cookie serves the current screen. `GET /` without a valid
cookie still returns the friendly 403 page. `GET /?key=<wrong>` returns 403.
Why `sessionStorage`: the helper needs a reconnect credential that survives
same-port restarts and does not depend only on cookie behavior. Because screen
HTML is trusted same-origin UI, storing the key in tab-scoped storage is
acceptable for this threat model. It is materially better than leaving the key
in the address bar, history, and referrer surface.
### 2. WebSocket Same-Origin Enforcement
WebSocket upgrades must pass both checks:
1. valid session auth by query key or cookie
2. if an `Origin` header is present, it must match the request target origin
The origin check should compare:
```text
Origin === "http://" + req.headers.host
```
Browser attacker page example:
```text
Origin: http://localhost:9999
Host: localhost:58088
```
This must be rejected even if the browser sends the companion cookie.
Legitimate companion page example:
```text
Origin: http://localhost:58088
Host: localhost:58088
```
This should be accepted when the key or cookie is valid.
Direct non-browser clients may omit `Origin`; they still need the session key.
### 3. Helper Reconnect Credential
`helper.js` should read the tab-scoped key from `sessionStorage` and append it
to the WebSocket URL:
```text
ws://<host>/?key=<stored-key>
```
If no stored key exists, the helper falls back to the current cookie-only
`ws://<host>` behavior. This preserves compatibility for already-loaded pages
that do have a valid cookie but no storage entry.
### 4. `/files/*` Containment
The file server should continue to reject empty names and dotfiles. It must also
ensure the file is a real regular file inside `CONTENT_DIR`.
Use realpath containment as the boundary:
- compute `realContentDir = fs.realpathSync(CONTENT_DIR)`
- compute `realFilePath = fs.realpathSync(filePath)`
- serve only when `realFilePath` equals a descendant of `realContentDir`
- reject symlinks and anything outside the content directory with 404
The server should keep using `path.basename` so nested paths remain unsupported.
### 5. Leak-Reduction Headers
Add conservative headers that do not block inline scripts or future same-origin
vendored libraries:
```text
Referrer-Policy: no-referrer
Cache-Control: no-store
X-Frame-Options: DENY
Content-Security-Policy: frame-ancestors 'none'
Cross-Origin-Resource-Policy: same-origin
```
Do not add a restrictive `script-src` CSP in this pass. The companion currently
injects inline helper JavaScript and future screens may load same-origin
vendored libraries.
### 6. Gitignore Durable Session State
Add `.superpowers/` to the repo root `.gitignore` so persisted companion state
and `.last-token` are not accidentally committed when using `--project-dir`.
### 7. Test Stability And Lint
Clean up shell lint warnings in the touched start/stop scripts.
Update the lifecycle test that invokes `start-server.sh --idle-timeout-minutes`
so it cannot hang under Codex's `CODEX_CI` foreground auto-detection. The test
should force background mode with `--background` when it expects the script to
return startup JSON.
## Testing Strategy
All behavior changes should be TDD:
1. write the failing focused test
2. run it and confirm it fails for the expected reason
3. implement the minimum fix
4. rerun the focused test
5. rerun the full brainstorm-server suite
Required focused regressions:
- valid keyed `/` returns bootstrap, not screen content
- bootstrap stores key in `sessionStorage` and strips the URL
- cookie-only `/` still serves screen content
- helper uses `sessionStorage` key for WebSocket URL
- same-origin cookie WebSocket opens
- cross-origin cookie WebSocket is rejected and writes no events
- direct key WebSocket still opens without `Origin`
- symlink under `content/` pointing to `state/server-info` returns 404
- security headers are present on normal HTML, bootstrap, 403, and file responses
- restart same port/token can authenticate reconnect with the stored key
- shell lint passes for touched shell scripts
- lifecycle suite does not hang under Codex
## Acceptance Criteria
- `cd tests/brainstorm-server && npm test` passes repeatedly without hanging.
- The security probe that previously wrote `attacker-injected` from another
localhost origin now fails to open the WebSocket and leaves `state/events`
unchanged.
- The symlink-to-`server-info` probe returns 404.
- A headed or headless browser keyed load ends on a bare `/` URL and the status
pill reaches Connected.
- A same-port/same-token restart reconnects automatically without manual reload.
- `scripts/lint-shell.sh` passes for the touched shell scripts.
## Deferred Work
If the project later needs to treat screen HTML as untrusted, design a separate
sandboxed iframe architecture. That should isolate generated screens on a
separate origin or sandboxed frame and expose only a narrow `postMessage` bridge
for user choices. Do not bundle that into this fix.
@@ -0,0 +1,254 @@
# Visual Companion Final Hardening Fixup Design
**Date:** 2026-06-11
**Status:** Draft for Drew review
## Goal
Finish the PR #1720 visual companion hardening pass so the branch is ready for
Jesse review with clean security behavior, deterministic tests, and a PR diff
that contains only the companion work.
This is a fixup on top of the existing auth hardening design. It should not
redesign the companion or expand the feature surface.
## Background
The previous hardening pass added keyed sessions, same-origin WebSocket checks,
URL key stripping, `/files/*` containment, leak-reduction headers, IPv6 URL
formatting, Windows lifecycle coverage, and PR evidence updates.
The final review pass found five remaining issues:
1. The root `GET /` screen-selection path can still serve symlinks or hardlinks
under `content/` that point outside the content directory.
2. When the preferred port is occupied, fallback servers can reuse a persisted
`.last-token`, creating two live same-project companion servers with the same
bearer key.
3. `stop-server.sh` can signal an unrelated `node server.cjs` process when
strong ownership proof is unavailable.
4. Some tests can pass against the wrong fallback process, leak background
processes on failure, or assume symlink support on Windows-like hosts.
5. The PR is currently conflicted because the branch contains an older `evals`
submodule bump that was handled separately.
## Non-Goals
- Do not add HTTPS tunnel or `wss://` origin semantics in this pass.
- Do not implement opt-out, free-text, or contrast-helper companion features.
- Do not vendor Alpine, Three.js, or any other JavaScript library.
- Do not attempt to sandbox malicious agent-authored screen HTML.
- Do not add backward compatibility for stale stop-server PID files unless Drew
explicitly approves that tradeoff.
## Inherited Security Invariants
This fixup preserves the auth hardening already designed and implemented:
- `.last-token` and `state/server-info` remain sensitive owner-only state.
- Fallback tokens may appear in startup JSON and `state/server-info`, but must
not be written to `.last-token`.
- Cookies remain port-named, `HttpOnly`, `SameSite=Strict`, and scoped to `/`.
- WebSocket upgrades still require a valid key or cookie.
- WebSocket `Origin` checks remain enforced when the browser supplies an
`Origin` header.
- Direct no-`Origin` clients remain allowed only when they carry the session key.
- Generated same-origin screen JavaScript and future same-origin vendored
libraries are trusted. Sandboxing malicious screen HTML remains deferred.
## Design
### 1. Rebase Onto Current `dev`
Rebase `brainstorming-companion` onto current `origin/dev` before implementation
work. Resolve the `evals` submodule conflict by taking `dev`.
After the rebase:
- `evals` must not appear in the PR diff.
- PR #1720 can still mention eval evidence that was run elsewhere, but it must
include exact external evidence: eval repo commit, scenario path, command,
result artifact path or id, and RED/GREEN outcome.
- The PR body must not imply the evals submodule bump is part of this PR.
- Any earlier PR-body text or comment implying the submodule bump is included
must be superseded by the final PR-body evidence.
### 2. Root Screen Containment
The root screen route must use the same containment boundary as `/files/*`.
`getNewestScreen()` should ignore any `.html` candidate that does not pass the
regular-file-inside-content-dir guard. That guard must resolve real paths and
ensure the served file is inside `CONTENT_DIR`. It must also preserve the
existing hardlink protection by rejecting files whose link count is not exactly
one when the platform reports link counts.
Expected behavior:
- A symlink under `content/` pointing outside `content/` is ignored.
- A hardlink under `content/` to `state/server-info` is ignored when
`fs.linkSync` succeeds and `lstat.nlink > 1`.
- If no safe screen file remains, the waiting page is served.
- Existing `/files/*` containment behavior remains unchanged: empty names,
dotfiles, symlinks, hardlinks, and directories still return 404.
### 3. Fallback Token Isolation
Port fallback must not reuse a token loaded from persisted `.last-token`.
Token source should be explicit in code:
- `BRAINSTORM_TOKEN` from the environment is an intentional operator/test
override. If the preferred port is occupied while an explicit environment
token is set, the server must fail closed instead of falling back, because the
occupied server may be using the same explicit token.
- `.last-token` is persisted state for same-port reconnect convenience. If the
server falls back because the preferred port is occupied, discard that loaded
token and generate a fresh unpersisted token for the fallback process.
- A newly generated token that was not loaded from `.last-token` can be reused
within the same process because no other live process is known to have it.
The fallback server must continue to avoid overwriting `.last-port` and
`.last-token`.
### 4. Stop-Server Ownership Proof
`start-server.sh` should create a per-start server instance id and pass it to
Node as an inert command-line argument, for example:
```text
node server.cjs --brainstorm-server-id=<id>
```
The id is not an auth credential. It is only process-ownership evidence for the
local lifecycle scripts. `server.cjs` can ignore the argument.
The id must use a shell/MSYS-safe alphabet, such as
`^[A-Za-z0-9_-]{32,64}$`. Store it in `state/server-instance-id` with
owner-only permissions.
`stop-server.sh` should read the expected id from state and only signal the PID
when the target process argv contains the exact argument
`--brainstorm-server-id=<id>` as a full argv token, not as a loose substring.
Prefer `/proc/<pid>/cmdline` when available, then fall back to wide `ps` output.
A matching instance id is sufficient proof even when `server-info` is missing
or `lsof` is unavailable. Existing port-to-PID checks may remain as additional
evidence.
Fail closed when ownership cannot be proven:
- missing PID file
- missing or malformed server id
- target command line unavailable
- target command line does not include the expected id
- old/stale session metadata without the new id
This intentionally prefers leaving a stale process running over killing an
unrelated process.
Operator-visible outcomes should be explicit:
- missing PID file returns `not_running`
- missing or malformed server id returns `stale_pid`
- unavailable command line returns `stale_pid`
- wrong or absent argv id returns `stale_pid`
- successful stop returns `stopped`
On `stale_pid` and `stopped` outcomes, remove `server.pid` and
`server-instance-id` so future stop attempts do not keep targeting the same
ambiguous process. Do not remove persistent session content.
### 5. Test Hardening
The test pass should be deterministic across macOS and the Windows Git Bash host
used for validation.
Required changes:
- Fixed-port suites must either fail fast if the server reports a fallback port
or drive all clients from the reported startup port.
- `stop-server.test.sh` needs a top-level cleanup trap before any background
process is started.
- Symlink-specific assertions should probe symlink capability and skip only that
assertion when the host cannot create usable test symlinks.
- Tests that create impostor processes must assert that the impostor survives
when lifecycle metadata is missing or insufficient.
- Windows/MSYS start-server tests must assert that Windows-like detection still
clears `BRAINSTORM_OWNER_PID`, still auto-foregrounds when appropriate, and
still passes the instance-id argv exactly.
### 6. Docs And PR Consistency
Before Jesse reviews, reconcile reviewer-visible docs and PR metadata:
- Update the issue catalog so dispositions match what this PR actually ships.
- Keep auto-open docs consistent with the implemented `--open` behavior.
- Keep the documented default idle timeout at 4 hours everywhere.
- Review the PR body against the template after the rebase.
- Record macOS, Windows, browser/manual, and external eval evidence in the PR
body with concrete commands and results.
## Testing Strategy
Use TDD for each behavior change:
1. Add or tighten a focused regression test.
2. Run it and confirm it fails for the expected reason.
3. Implement the smallest fix.
4. Rerun the focused test.
5. Rerun the full brainstorm-server suite.
Required focused regressions:
| Behavior | Test File | Focused Command | Expected RED | Expected GREEN |
| --- | --- | --- | --- | --- |
| Root route ignores symlink escape | `tests/brainstorm-server/server.test.js` | `node tests/brainstorm-server/server.test.js` | authenticated `GET /` serves linked outside content | response serves waiting page or safe screen |
| Root route ignores supported hardlink escape | `tests/brainstorm-server/server.test.js` | `node tests/brainstorm-server/server.test.js` | authenticated `GET /` serves hardlinked `server-info` | hardlink candidate is ignored when `nlink > 1` |
| `/files/*` containment stays unchanged | `tests/brainstorm-server/server.test.js` | `node tests/brainstorm-server/server.test.js` | existing containment test regresses | empty, dotfile, directory, symlink, hardlink cases remain 404 |
| Persisted-token fallback rotates token | `tests/brainstorm-server/lifecycle.test.js` | `node tests/brainstorm-server/lifecycle.test.js` | fallback URL key equals persisted preferred-port key | fallback URL key differs and is not written to `.last-token` |
| Explicit-token fallback fails closed | `tests/brainstorm-server/lifecycle.test.js` | `node tests/brainstorm-server/lifecycle.test.js` | server falls back while `BRAINSTORM_TOKEN` is set | process exits non-zero and does not start fallback |
| Fallback key cannot authenticate to original server | `tests/brainstorm-server/lifecycle.test.js` | `node tests/brainstorm-server/lifecycle.test.js` | fallback key receives 200 from original port | original port rejects fallback key |
| Correct instance id permits stop | `tests/brainstorm-server/stop-server.test.sh` | `bash tests/brainstorm-server/stop-server.test.sh` | real start-server-launched server survives | stop returns `stopped` and process exits |
| Wrong, missing, malformed, or stale id is safe | `tests/brainstorm-server/stop-server.test.sh` | `bash tests/brainstorm-server/stop-server.test.sh` | impostor is signaled | stop returns `stale_pid` and impostor survives |
| Fixed-port suites cannot pass through fallback | `tests/brainstorm-server/server.test.js`, `tests/brainstorm-server/auth.test.js` | respective `node` commands | test silently talks to fallback port | test fails clearly or uses reported port intentionally |
| Shell cleanup traps run on failures | `tests/brainstorm-server/stop-server.test.sh` | `bash tests/brainstorm-server/stop-server.test.sh` | failure leaves child processes | trap reaps background children |
| Windows/MSYS start behavior keeps lifecycle invariants | `tests/brainstorm-server/start-server.test.sh`, `tests/brainstorm-server/windows-lifecycle.test.sh` | `bash` test commands on macOS and `ballmer` | owner PID or argv handling regresses | owner PID is cleared, foreground detection holds, id argv is present |
Each RED/GREEN cycle should leave a short evidence note for the PR body: focused
command, failing assertion before the fix, passing assertion after the fix, and
whether the evidence was gathered on macOS or Windows.
## Verification
Before calling the fixup complete, run:
- `git fetch origin dev && git rebase origin/dev`
- `git diff --quiet origin/dev...HEAD -- evals`
- `gh pr view 1720 --json mergeStateStatus,statusCheckRollup,headRefOid`
- `cd tests/brainstorm-server && npm test`
- relevant focused test commands used during TDD
- `git diff --check`
- Node syntax checks for touched JavaScript files
- shell lint for touched shell files
- Windows validation on `ballmer`: full runnable brainstorm-server suite plus
the standalone Windows lifecycle probe
Manual/browser testing comes only after the automated pass is green.
## Acceptance Criteria
- PR #1720 rebases cleanly onto current `dev`.
- `evals` is absent from the PR diff.
- Root screen serving cannot read outside `content/` through symlink or
supported hardlink escapes.
- `/files/*` containment protections remain unchanged.
- No fallback server runs with a token that may be shared with the occupied
preferred-port server.
- `stop-server.sh` does not signal unrelated processes when ownership proof is
missing or ambiguous.
- `stop-server.sh` can still stop a legitimate server with a matching instance
id when `server-info` or `lsof` is unavailable.
- Focused RED/GREEN evidence is recorded for each regression.
- macOS and Windows validation evidence is recorded in the PR body.
- The PR body accurately describes what is in the branch and what evidence was
gathered externally.