Skip to content

Commit 454423a

Browse files
garrytanclaude
andauthored
v1.21.1.0 test: tighten plan-ceo-review smoke (Step 0 must fire) (#1255)
* test: extract classifyVisible() + permission-dialog filter in PTY runner Pure classifier extracted from runPlanSkillObservation's polling loop so unit tests can exercise the actual branch order with synthetic input strings. Runner gains: - env? passthrough on runPlanSkillObservation (forwarded to launchClaudePty). gstack-config does not yet honor env overrides; plumbing is in place for a future change to make tests hermetic. - TAIL_SCAN_BYTES = 1500 exported constant. Replaces a duplicated magic number in test/skill-e2e-plan-ceo-mode-routing.test.ts so tuning stays in sync. - isPermissionDialogVisible: the bare phrase "Do you want to proceed?" now requires a file-edit context co-trigger. Other clauses unchanged. Skill questions that contain the bare phrase are no longer mis-classified. - classifyVisible(visible): pure function. Branch order silent_write → plan_ready → asked → null. Permission dialogs filtered out of the 'asked' classification so a permission prompt cannot pose as a Step 0 skill question. Adds 24 unit tests covering all classifier branches, edge cases, and the co-trigger contract. * test: tighten plan-ceo-review smoke to require Step 0 fires first Assertion narrows from ['asked', 'plan_ready'] to 'asked' only. Reaching plan_ready first means the agent skipped Step 0 entirely and went straight to ExitPlanMode — the regression we want to catch. Why plan-ceo is special: unlike plan-eng / plan-design / plan-devex (whose smokes legitimately reach plan_ready on certain branches without asking), plan-ceo-review's template mandates Step 0A premise challenge plus Step 0F mode selection BEFORE any plan write. There is no legitimate path to plan_ready that does not first emit a skill-question numbered prompt. Failure message now branches on outcome (plan_ready vs timeout vs silent_write) with a tailored diagnosis line per case. References the skill template by section name ("Step 0 STOP rules", "One issue = one AskUserQuestion call") instead of line numbers, so it survives template edits. Passes env: { QUESTION_TUNING: 'false', EXPLAIN_LEVEL: 'default' } through the runner. Today this is advisory — gstack-config reads only ~/.gstack/config.yaml, not env vars — but the wiring is in place for a future change. Documented honestly in the docstring. Verified across 4 PTY runs: 3 pre-refactor + 1 post-refactor, all PASS. * chore: capture v1.21.1.0 follow-ups in TODOS.md - P2: per-finding AskUserQuestion count assertion (V2) - P3: honor env vars in gstack-config so test isolation env actually works - P3: path-confusion hardening on SANCTIONED_WRITE_SUBSTRINGS All three surfaced during the v1.21.1.0 plan-eng-review and adversarial review passes. Captured here so the design intent persists. * chore: bump version and changelog (v1.21.1.0) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: extract MODE_RE + optionsSignature into PTY runner exports Refactor prep for the upcoming per-finding AskUserQuestion count test across plan-{ceo,eng,design,devex}-review. Both new tests and the existing mode-routing test need the same mode regex and the same option-list fingerprint dedupe — pulling them into one source of truth in test/helpers/claude-pty-runner.ts so a fifth mode (or a tweak to the fingerprint shape) updates everywhere instead of drifting per-test. Mechanical: no behavior change in the mode-routing test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: add per-finding count primitives + unit tests Pure helpers landing ahead of runPlanSkillCounting: - parseQuestionPrompt(visible) — extract the 1-3 line prompt above the latest "❯ 1." cursor, normalize to a 240-char snippet - auqFingerprint(prompt, opts) — Bun.hash of normalized prompt + sorted options signature; distinct prompts with shared option labels (the generic A/B/C TODO menu) get distinct fingerprints - COMPLETION_SUMMARY_RE — terminal-signal regex matching all four plan-review skills' completion / verdict markers - assertReviewReportAtBottom(content) — checks "## GSTACK REVIEW REPORT" is present and is the last "## " heading in a plan file - Step0BoundaryPredicate type + four per-skill predicates (ceo / eng / design / devex) — fire on the answered AUQ's fingerprint, marking the end of Step 0 deterministically (event-based, not content-based, per Codex F7) Plus 37 deterministic unit tests covering option-label collision regression, prompt extraction edge cases, predicate positive AND negative cases, and review-report-at-bottom triple-check (missing / mid-file / multiple trailing). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: add runPlanSkillCounting PTY helper Drives a plan-* skill end-to-end and counts distinct review-phase AskUserQuestions. Composes the primitives from the previous commit: - Boot + auto-trust handler (existing launchClaudePty) - Send slash command alone, sleep 3s, send plan content as follow-up message (proven pattern from skill-e2e-plan-design-with-ui) - Poll loop with permission-dialog auto-grant, same-redraw skip, empty-prompt re-poll - Event-based Step-0 boundary via isLastStep0AUQ predicate fired on the answered AUQ's fingerprint (Codex F7 — boundary is observed event, not later rendered content) - Multi-signal terminals: hard ceiling, COMPLETION_SUMMARY_RE, plan_ready, silent_write, exited, timeout Empty-prompt fingerprints are skipped per the contract documented in auqFingerprint's unit tests — fingerprinting them would re-introduce the option-label collision regression Codex F1 caught. No E2E tests yet — those land in commit 5 with the four skill fixtures. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: register four finding-count tests in touchfiles + tier map Each new test depends on its skill template, the runner, and three preamble resolvers (preamble.ts, generate-ask-user-format.ts, generate-completion-status.ts) — those affect question cadence and completion rendering, which is exactly what the test asserts on. All four classified periodic. Sequential execution during calibration; opt-in to concurrent only after measured comparison agrees (plan §D15). Updated touchfiles.test.ts: plan-ceo-review/** now selects 19 tests (was 18) because plan-ceo-finding-count joins the family. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: add four per-finding count E2E tests (plan-ceo + eng + design + devex) Each test drives its plan-* skill through Step 0 then asserts the review-phase AskUserQuestion count falls in [N-1, N+2] for an N=5 seeded plan, plus D19: produced plan file ends with "## GSTACK REVIEW REPORT" as its last "## " heading. plan-ceo also runs a paired-finding positive control: 2 deliberately related findings should still produce 2 distinct AUQs, not 1 batched. Periodic-tier (gate-skipped without EVALS=1, EVALS_TIER=periodic). Sequential execution by plan §D15. Each fixture is inline TypeScript content delivered as a follow-up message after the slash command, per the proven pattern at skill-e2e-plan-design-with-ui.test.ts. Calibration loop (5 runs per skill) and the manual pre-merge negative check (D7 + D12) are required before merge per plan §Verification. NOT yet run. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: fix parseNumberedOptions for inline-cursor box-layout AUQs Calibration run 1 timed out with step0=0 review=0 because the parser could not find the cursor in /plan-ceo-review's scope-selection AUQ. The TTY's box-layout rendering inlines divider + header + prompt + "1." onto one logical line — cursor escapes get stripped, leaving text crushed onto a single line. Cursor anchor regex changed from anchored to unanchored so it matches mid-line. Cursor-line option extraction uses a non-anchored regex; subsequent options stay with the original start-of-line parser. parseQuestionPrompt picks up the inline prompt text BEFORE the cursor on the cursor line (after stripping box-drawing chars + sigil) and appends it after any walked-up multi-line prompt above. Three new unit tests: clean-cursor still works, inline-cursor extracts all 7 options, prompt extraction strips box chars. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: add firstAUQPick + plan-ceo skip-interview routing Calibration run 1 surfaced a second issue beyond the parser bug: the default pick of 1 on /plan-ceo-review's scope-selection AUQ routes the agent to "branch diff vs main" — so it reviews the gstack PR itself (recursive!) instead of the seeded fixture plan we sent. Added firstAUQPick callback to runPlanSkillCounting. Override applies only to the FIRST AUQ; subsequent presses keep using defaultPick. ceoStep0Boundary now fires on either the mode-pick AUQ (existing path) or any AUQ containing "Skip interview and plan immediately" — which is the scope-selection AUQ. Picking that option bypasses Step 0 and routes straight to review-phase using the chat-paste plan as context. Plan-ceo test wires firstAUQPick = pickSkipInterview which finds the "Skip interview" option by label. Falls back to "describe inline" if the option labels change. Two new unit tests: ceoStep0Boundary fires on the scope-selection fixture; existing mode-pick fixture still fires. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent e8893a1 commit 454423a

14 files changed

Lines changed: 2271 additions & 78 deletions

CHANGELOG.md

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,51 @@
11
# Changelog
22

3+
## [1.21.1.0] - 2026-04-28
4+
5+
## **plan-ceo-review smoke tightens. The "agent skips Step 0 and ships a plan" regression now fails the gate.**
6+
7+
The v1.15.0.0 real-PTY harness shipped with a smoke that accepted either `'asked'` or `'plan_ready'` as success. That OR was too lax for `/plan-ceo-review` specifically: the skill template mandates Step 0A premise challenge plus Step 0F mode selection BEFORE any plan write, so reaching `plan_ready` first IS the regression. This release tightens the assertion to `'asked'` only for that smoke, and refactors the runner so the contract is testable in <1s instead of $0.50 of stochastic PTY.
8+
9+
### The numbers that matter
10+
11+
Numbers come from `git diff --shortstat origin/main..HEAD` and `bun test test/helpers/claude-pty-runner.unit.test.ts` on a clean tree.
12+
13+
| Metric | Δ |
14+
|---|---|
15+
| Net branch size vs main | +162 / −65 lines (3 files) |
16+
| New unit tests added | **+24** (claude-pty-runner.unit.test.ts) |
17+
| Unit suite runtime | **14ms** (deterministic, free-tier) |
18+
| Real-PTY gate runs verified | **4 clean PTY runs** (3 lock-in + 1 post-refactor) |
19+
| Outcome assertions covered | **5/5** (was 3/5; `plan_ready` is now FAIL for plan-ceo) |
20+
| Reviewers run on this PR | plan-eng-review (CLEARED) + codex consult + 2 specialists + adversarial |
21+
22+
### What this means for builders
23+
24+
Three new classes of harness regression are now caught deterministically in the free tier instead of waiting on a $0.50 stochastic PTY run. The classifier is extracted into a pure `classifyVisible()` function so reordering branches in the polling loop fails the unit tests instead of silently shipping. Permission dialogs (which render numbered lists) are filtered out of the `'asked'` classification so a permission prompt cannot pose as a Step 0 skill question. The bare phrase `Do you want to proceed?` no longer triggers permission detection on its own — it now requires a file-edit context co-trigger, so a skill question that contains the phrase isn't mis-classified.
25+
26+
For `/plan-ceo-review` specifically: any future preamble slim-down or template edit that lets the agent skip Step 0 and write a plan will fail the gate before the PR ships. Pull, run `bun test`, and the harness layer is provably tighter without you having to spend a token.
27+
28+
### Itemized changes
29+
30+
#### Added
31+
32+
- `test/helpers/claude-pty-runner.unit.test.ts`: 24 deterministic tests covering `isPermissionDialogVisible` (with the new co-trigger contract), `isNumberedOptionListVisible`, `parseNumberedOptions`, and the new `classifyVisible()` runtime path. Free-tier, runs on every `bun test`.
33+
- `classifyVisible(visible)` in `claude-pty-runner.ts`: pure classifier extracted from the polling loop. Returns `{ outcome, summary } | null`. Branch order: silent_write → plan_ready → asked → null (with permission-dialog filter). Live-state branches (process exited, "Unknown command") stay in the runner.
34+
- `TAIL_SCAN_BYTES = 1500` exported constant. Shared between `runPlanSkillObservation` and the routing test's nav loop so tuning stays in sync.
35+
- `env?: Record<string, string>` option on `runPlanSkillObservation`, threaded to `launchClaudePty`. Plumbing for future env-driven test isolation (gstack-config does not yet honor env overrides; tracked as post-merge follow-up).
36+
37+
#### Changed
38+
39+
- `test/skill-e2e-plan-ceo-plan-mode.test.ts`: assertion narrowed from `['asked', 'plan_ready']` to `'asked'` only. Failure message now branches on `outcome` (plan_ready vs timeout vs silent_write) with a tailored diagnosis line, and references skill-template section names instead of line numbers (durable to template edits).
40+
- `isPermissionDialogVisible`: bare `Do you want to proceed?` now requires a file-edit context co-trigger (`Edit to <path>` or `Write to <path>`). Other clauses (`requested permissions to`, `allow all edits`, `always allow access to`, `Bash command requires permission`) remain unconditional.
41+
- `test/skill-e2e-plan-ceo-mode-routing.test.ts`: replaces the local `1500` magic number with the shared `TAIL_SCAN_BYTES` constant.
42+
43+
#### For contributors
44+
45+
- The runner change is additive and the existing sibling smokes (`plan-eng`, `plan-design`, `plan-devex`, `plan-mode-no-op`) keep their loose `['asked', 'plan_ready']` assertion. Their behavior is unchanged.
46+
- Post-merge follow-ups captured in `TODOS.md`: per-finding AskUserQuestion count assertion (V2), env-driven gstack-config overrides (so `QUESTION_TUNING=false` actually isolates the test), path-confusion hardening on `SANCTIONED_WRITE_SUBSTRINGS`.
47+
48+
349
## [1.20.0.0] - 2026-04-28
450

551
## **Browser-skills land. `/scrape <intent>` first call drives the page; second call runs the codified script in 200ms.**

TODOS.md

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -213,6 +213,56 @@ scope of that PR; deliberately deferred to keep PTY-import small.
213213

214214
## Testing
215215

216+
## P2: Per-finding AskUserQuestion count assertion for /plan-ceo-review
217+
218+
**What:** PTY E2E test that drives /plan-ceo-review through Step 0 with a stable fixture diff containing N known findings, asserts that exactly N distinct AskUserQuestions fire (one per finding) before plan_ready.
219+
220+
**Why:** The skill template repeats "One issue = one AskUserQuestion call. Never combine multiple issues into one question." at every review checkpoint. No test enforces it. The current `skill-e2e-plan-ceo-plan-mode.test.ts` smoke (post-v1.21.1.0) only catches "agent skipped Step 0 entirely." Batching findings into one question slips through silently.
221+
222+
**Pros:** Locks in the strongest contract the skill mandates. Catches a real failure mode (the original attachment showed 2 findings batched as 0 questions).
223+
**Cons:** Needs a stable fixture diff to keep finding count deterministic (~1 day human / ~30 min CC). Opus may reasonably consolidate two related findings, so the assertion needs a forgiving lower bound (e.g., `>= ceil(N * 0.6)`) rather than strict equality.
224+
225+
**Context:** The PTY harness (`runPlanSkillObservation`) returns at first terminal outcome — for V2 we need a streaming variant that counts AskUserQuestions across the whole session up to `plan_ready`. Probably a new helper alongside `runPlanSkillObservation`.
226+
227+
**Depends on:** Stable fixture diff (`test/fixtures/plans/multi-finding.diff` or similar) with a small known set of issues that triggers all 4 review sections.
228+
229+
**Priority:** P2.
230+
**Effort:** S (CC: ~30 min once fixture exists). Captured from v1.21.1.0 plan-eng-review D2.
231+
232+
---
233+
234+
## P3: Honor env vars in gstack-config (so QUESTION_TUNING/EXPLAIN_LEVEL actually isolate tests)
235+
236+
**What:** `gstack-config get <key>` reads `~/.gstack/config.yaml`. `runPlanSkillObservation` plumbs `env: { QUESTION_TUNING: 'false', EXPLAIN_LEVEL: 'default' }` through to the spawned `claude` process — but the skill preamble bash uses `gstack-config get question_tuning`, which never looks at env. The env passthrough is theater on current code.
237+
238+
**Why:** Without env honoring, the v1.21.1.0 plan-ceo-review smoke is still flaky on machines with `question_tuning: true` set in YAML. AUTO_DECIDE preferences would skip the rendered AskUserQuestion list, masking the regression we want to catch.
239+
240+
**Pros:** Makes the gate test hermetic across machines. The env wiring is already in place — only `gstack-config` needs to read env first, fall back to YAML.
241+
**Cons:** Touches the gstack-config binary across all 3 platforms (linux/darwin/windows). Cross-binary refactor.
242+
243+
**Context:** Captured from v1.21.1.0 adversarial review. Documented honestly in the test docstring as a known limitation.
244+
245+
**Priority:** P3.
246+
**Effort:** S. Single-file edit to `bin/gstack-config` (~10 LOC for env-first lookup).
247+
248+
---
249+
250+
## P3: Path-confusion hardening on SANCTIONED_WRITE_SUBSTRINGS
251+
252+
**What:** `runPlanSkillObservation`'s silent-write detector uses substring matching on a few sanctioned paths (`.gstack/`, `CHANGELOG.md`, `TODOS.md`, etc). A write to `node_modules/some-pkg/CHANGELOG.md` or `src/foo/.gstack/leak.ts` is currently sanctioned because the substring matches anywhere in the path.
253+
254+
**Why:** Defensive — no current bug exploits this, but a malicious skill or fixture could write to a path that happens to contain `.gstack/` or `CHANGELOG.md` and slip past silent-write detection.
255+
256+
**Pros:** Hardens the harness against future skill misbehavior. Aligns substring rules with their intent.
257+
**Cons:** Need to anchor against absolute prefixes (`os.homedir() + '/.gstack/'`, worktree root) which makes the test less portable across machines.
258+
259+
**Context:** Captured from v1.21.1.0 adversarial review (HIGH/FIXABLE finding, pre-existing). Refactored into a `SANCTIONED_WRITE_SUBSTRINGS` constant in v1.21.1.0 but the substring-includes logic is unchanged from before.
260+
261+
**Priority:** P3.
262+
**Effort:** S.
263+
264+
---
265+
216266
## P1: Structural STOP-Ask forcing function across all skills
217267

218268
**What:** Design and implement a structural forcing function that catches when a skill mandates per-issue AskUserQuestion but the model silently substitutes batch-synthesis. Candidate mechanisms: question-count assertion (skill declares expected question count in frontmatter; post-run audit logs if model fired <N), typed question templates (skill hands the model pre-built AskUserQuestion payloads rather than prose instructions), or a canUseTool-based post-run audit that compares declared-gates-fired vs expected.

VERSION

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
1.20.0.0
1+
1.21.1.0

package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "gstack",
3-
"version": "1.20.0.0",
3+
"version": "1.21.1.0",
44
"description": "Garry's Stack — Claude Code skills + fast headless browser. One repo, one install, entire AI engineering workflow.",
55
"license": "MIT",
66
"type": "module",

0 commit comments

Comments
 (0)