From ba07db72c6c45ab98a1aa0b75fe4fc786eb7d087 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sat, 10 Oct 2026 23:55:49 -0400 Subject: [PATCH 1/2] fix(wizard): parse generated wizard stages whole so a mid-run edit cannot re-run steps bash reads a script from a file offset as it runs it, so an in-place edit that shifts bytes made an unwrapped wizard run finished stages again. The template now wraps its stages in run_wizard, which ends in exit, and its last line calls it. Ports mattpocock/skills#1142 (upstream 49dd158), with exit moved inside the function so a formatter splitting `run_wizard "$@"; exit` cannot undo the fix. The test rewrites the running script in place and asserts step-1 runs once, with an unwrapped control that runs it twice. Closes #6938 Co-Authored-By: Claude Opus 5.5 --- ...938-wizard-run-wizard-function-c7cd8da9.md | 7 +++ docs/upstream/mattpocock-skills.md | 2 +- plugins/wizard/skills/generate/SKILL.md | 3 +- plugins/wizard/skills/generate/template.sh | 52 +++++++++++-------- .../wizard/skills/generate/template.test.sh | 49 ++++++++++++++++- 5 files changed, 88 insertions(+), 25 deletions(-) create mode 100644 .changes/wizard/fix-6938-wizard-run-wizard-function-c7cd8da9.md diff --git a/.changes/wizard/fix-6938-wizard-run-wizard-function-c7cd8da9.md b/.changes/wizard/fix-6938-wizard-run-wizard-function-c7cd8da9.md new file mode 100644 index 0000000000..0cc2ed0695 --- /dev/null +++ b/.changes/wizard/fix-6938-wizard-run-wizard-function-c7cd8da9.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- A generated wizard no longer re-runs earlier steps when its script is edited while it runs. bash reads a script from a file offset as it goes, so an in-place edit that shifted the bytes made it run already-finished stages again. The stages now sit inside a `run_wizard` function that ends in `exit`, and the script's last line calls it, so bash parses the whole file before the first prompt. diff --git a/docs/upstream/mattpocock-skills.md b/docs/upstream/mattpocock-skills.md index 2af04aaaee..6ae5ee17d2 100644 --- a/docs/upstream/mattpocock-skills.md +++ b/docs/upstream/mattpocock-skills.md @@ -48,7 +48,7 @@ adding, removing or renaming an upstream skill. Re-audit every affected row. | `improve-codebase-architecture` YAGNI scoping filter (v1.2 #533) | `architecture:improve` deepening Phase 1 | Partial | ADOPTED scope-before-scanning: user-named direction scopes the scan, else recent-commit hot spots pull attention first (precomputed context widened to 20 commits). REJECTED his glossary file reference (`CONTEXT.md` at v1.2.3, renamed `GLOSSARY.md` in v1.3; our glossary-discovery ladder instead). Corrected 2026-10-04: the HTML report was ADOPTED (self-contained file in a temp dir, three recommendation badges; `architecture/skills/improve/actions/deepening.md:66-85`, since `30cf0343f`); only the CDN Tailwind and Mermaid loading is rejected. Upstream v1.3 removed the inbound `diagnosing-bugs` post-mortem hand-off to this skill (`1dab982`) | | `diagnosing-bugs` (v1.2.3 Redact + tagged logs) | `debugging:debug`, `testing:diagnose` | Partial | ADOPTED the redaction guard in both skills (secrets `` before any shown command/output/artifact; env-var credentials; signal-lines-only quoting) and the `[DEBUG-a4f2]` tagged-log convention in `testing:diagnose` (already present in `debugging:debug`). Re-evaluated at `d81f3a1` (2026-10-04): the ranked construction strategies and 3-5 ranked hypotheses are ALREADY-PRESENT (`debugging/skills/debug/SKILL.md:61-72`, `:112-122`). Real gaps, under evaluation: upstream's minimize step (`diagnosing-bugs/SKILL.md:78-86`; our `:152` and `:159` refer to a minimized repro no phase produces) and its Phase 1 completion criterion (one named command, already run, red output shown; `:59`, present since v1.2.3). Phase 6: upstream removed the post-mortem hand-off in `1dab982` (2026-08-15, upstream PR #880, fixing #453) because it was a Skill-tool call to a user-invoked skill from an often-unattended flow; graded KEEP ours (REJECT the removal), since our Phase 6 (`SKILL.md:163-180`) files the finding and calls no skill. Upstream PR #1121 (merged 2026-10-04, released in v1.3.1, closing issue #1117) now sends the "what would have prevented this?" question to `retro` after the fix | | `wait-what` (Productivity, NEW in v1.2 #751) | `discipline:wait-what` | Derived | Ported near-verbatim (one-sentence re-pitch body: back up, add missing context, ASD-STE100 register + inline gloss, ubiquitous language) as a declared non-corrector species in `discipline` beside `tighten-your-output`/`mind-your-maxims` — home chosen on the blame axis (the drift is the model's output, not the user's comprehension). Name KEPT with an explicit PLUGIN-PHILOSOPHY naming-exception entry (utterance-is-mechanism + upstream muscle-memory parity; a 5-generator/3-judge naming tournament's grammar-clean winner `re-pitch` was declined by the user). REJECTED his fixed glossary filenames: `CONTEXT.md` at v1.2.3, renamed `GLOSSARY.md` / `GLOSSARY-MAP.md` in v1.3 (`006a52b`) with no fallback for the old names (open upstream issue [#1153](https://github.com/mattpocock/skills/issues/1153), no migration path). Ours keeps format-externalized discovery: the nearest glossary or context map per the consumer's convention, plain technical English when none exists (`discipline/skills/wait-what/SKILL.md:15-17`). Re-checked 2026-10-04: his v1.3 map-following (`GLOSSARY-MAP.md` to the right `GLOSSARY.md`, upstream PR #904) now matches ours, so the rejection covers only the fixed names. Shape evidence: his X thread (status 2084753070437609606 → 2084941367659168064 → 2085681281795232026) — the same instruction failed as passive global CLAUDE.md AND as an output style; only the on-demand skill works, so the register text lives in the body, invoked at the moment of loss | -| `wizard` (Engineering; graduated from in-progress in v1.2) | `wizard:generate` | Derived | PORTED (lane 4) as a new single-capability plugin `wizard` 0.1.0, hardened. Kept: the 4-step scope/map/author/verify process, the fixed never-hand-edited library above the `STAGES` marker, model-invoked posture with the explicit non-trigger fence, gh-absence graceful degradation, ephemeral-by-default doctrine, agent-authors-never-runs doctrine. Hardened beyond upstream (deltas enumerated in `plugins/wizard/CHANGELOG.md` 0.1.0): mandatory human read-and-approve of the full STAGES block before `chmod +x`; https-only `open_url` (also closes a Windows UNC/NTLM leak via explorer.exe); `/dev/tty` fail-closed prompts (retires a verified multi-line-paste confirm bypass and `pause`'s fail-open at EOF); quoted `0600` `.env` writes + gitignore assert + trap-cleaned atomic temp; repo-resolved/confirmed `--repo`-explicit gh writes with stderr surfaced and empty values refused; key-name validation; readline on non-secret asks (fixes upstream #741 where safe); names-only live-`.env` scoping with the secrets-and-context property stated honestly. REJECTED: Codex `agents/openai.yaml` sidecar (no Codex target — standing precedent). Upstream status 2026-10-04: the template dropped "time remaining" (v1.2.3), but the docs page still advertises it (`docs/engineering/wizard.md:44`), so a later sync must not bring it back. Open upstream bugs: [#1041](https://github.com/mattpocock/skills/issues/1041) (`write_env` never assigns the shell variable) and [#811](https://github.com/mattpocock/skills/issues/811) (a symlinked `.env` is replaced, not written through) both reproduce in our `template.sh` and are not fixed here yet; [#1142](https://github.com/mattpocock/skills/issues/1142) (editing the script mid-run kills it) is unassessed | +| `wizard` (Engineering; graduated from in-progress in v1.2) | `wizard:generate` | Derived | PORTED (lane 4) as a new single-capability plugin `wizard` 0.1.0, hardened. Kept: the 4-step scope/map/author/verify process, the fixed never-hand-edited library above the `STAGES` marker, model-invoked posture with the explicit non-trigger fence, gh-absence graceful degradation, ephemeral-by-default doctrine, agent-authors-never-runs doctrine. Hardened beyond upstream (deltas enumerated in `plugins/wizard/CHANGELOG.md` 0.1.0): mandatory human read-and-approve of the full STAGES block before `chmod +x`; https-only `open_url` (also closes a Windows UNC/NTLM leak via explorer.exe); `/dev/tty` fail-closed prompts (retires a verified multi-line-paste confirm bypass and `pause`'s fail-open at EOF); quoted `0600` `.env` writes + gitignore assert + trap-cleaned atomic temp; repo-resolved/confirmed `--repo`-explicit gh writes with stderr surfaced and empty values refused; key-name validation; readline on non-secret asks (fixes upstream #741 where safe); names-only live-`.env` scoping with the secrets-and-context property stated honestly. REJECTED: Codex `agents/openai.yaml` sidecar (no Codex target — standing precedent). Upstream status 2026-10-04: the template dropped "time remaining" (v1.2.3), but the docs page still advertises it (`docs/engineering/wizard.md:44`), so a later sync must not bring it back. Upstream bugs ported as fixes: [#1041](https://github.com/mattpocock/skills/issues/1041) (`write_env` never assigns the shell variable) and [#811](https://github.com/mattpocock/skills/issues/811) (a symlinked `.env` is replaced, not written through) in 0.6.4; [#1142](https://github.com/mattpocock/skills/issues/1142) (editing the script mid-run re-runs or kills it), fixed upstream in `49dd158` by wrapping the stages in `run_wizard`, ported with `exit` moved inside the function so a formatter that splits `run_wizard "$@"; exit` cannot undo it | | `prototype` `LOGIC.md` shareable-HTML demo (Engineering, v1.2) | `prototype:pressure-test` | Partial | ADOPTED (lane 5) the audience-routed HTML demo shell: TUI stays default; when the driver is a non-developer (designer, PM, domain expert) or no terminal fits, the disposable shell over the same portable pure logic module is one self-contained `file://` page — domain-language labels, labeled state panel re-rendered per click, free-play buttons, guided-walkthrough scenarios resetting to a known initial state — under explore-directions' existing HTML-substrate constraint set reused verbatim-in-spirit (restrictive CSP meta tag, ephemeral `mktemp -d` / `%LOCALAPPDATA%\Temp` placement, synthetic data only, discard after the markdown capture). prototype 0.5.0. REJECTED the other half of upstream's step 5: the throwaway-branch "primary source" capture that keeps the prototype re-runnable on a branch — a two-lane branch-naming posture violation that also contradicts the plugin's delete-when-done discipline (`plugins/prototype/context/discipline.md`, "Delete or absorb when done") | | ask-matt `PHASE-BOUNDARIES.md` (v1.2) | `session-flow:workflow` continuation router + `context-guard` zones | Convergent / rejected | Re-read at `84fdeff`. At parity: the ordered first-yes-wins router and compaction last with a focus; ours adds clean-stop, user-gated background, instrumented zones, worker relay. ADOPTED one zone-gated criterion: prefer continue when the next stage consumes this stage's reasoning verbatim. REJECTED the boundary-only trigger (`/session-flow:workflow` routes by task, mid-stage included), the "handoff only for what travels" narrowing (`/session-flow:workflow` hands off in more cases than his travel list), and the ~150k smart-zone figure (self-declared-debated folklore; no official numeric threshold exists; our baseline is instrumented zone readings plus context-guard's declared judgment-default bands with named provenance (corrected 2026-08-18 per audit amendment A1: the bands are declared defaults, not measurements; only zone readings are measured), his dictionary entry noted as one more folklore anchor). Docs topic: [when your context fills up](https://code.claude.com/docs/en/context-window#when-your-context-fills-up). As-of 2026-10-02 | | `teach` (Productivity) | `education:teach` | Derived | Corrected by the `teach-skill-comparison` topic audit (PR #2958) — this row previously sat under "Not adopted", which is provably false: the original port took his workspace vocabulary (MISSION / GLOSSARY / RESOURCES / NOTES + learning records as "teaching ADRs"), near-verbatim FORMAT-spec content, and the K-S-W / ZPD / community-delegation pedagogy with learning-record doctrine. REJECTED: cwd-as-workspace (dedicated per-project workspace roots instead), Codex `agents/openai.yaml` sidecar (standing precedent), HTML references (the durable trio — reference, records, glossary — stays markdown). ADDED house-built: codebase mode, primer action, assess, staleness doctrine, evals, slug-collision guards, workspace-root resolution ladder. RE-ADOPTED in the same audit (education 0.7.0): storage-strength pedagogy (fluency-vs-storage, desirable-difficulty triad, knowledge/skills asymmetry, equal-length quiz answers) and HTML-first interactive lessons with a shared `assets/` library (answer-shuffling quiz component — fixes his #335 class of always-option-C bug); his acknowledged no-review-scheduling gap is out-executed via spaced review surfaced at resume/status from learning-record age × domain velocity | diff --git a/plugins/wizard/skills/generate/SKILL.md b/plugins/wizard/skills/generate/SKILL.md index 59ac304351..a27a7fa494 100644 --- a/plugins/wizard/skills/generate/SKILL.md +++ b/plugins/wizard/skills/generate/SKILL.md @@ -74,7 +74,8 @@ Never invent steps that may not exist. ### 3. Author the wizard Copy [template.sh](template.sh) to the target path. Replace the example stage -with one `stage` per step, in dependency order. Use the library helpers: +inside `run_wizard` with one `stage` per step, in dependency order, and keep +its closing `finish` and `exit` and the final `run_wizard` call. Use the library helpers: `stage`, `say`/`step`/`note`/`warn`, `open_url`, `ask`/`ask_secret`, `write_env`, `set_secret`/`set_var`, `pause`/`confirm`. Then set `TOTAL_STAGES` to the number of stages you wrote. diff --git a/plugins/wizard/skills/generate/template.sh b/plugins/wizard/skills/generate/template.sh index b7432557ae..164bca7f8b 100755 --- a/plugins/wizard/skills/generate/template.sh +++ b/plugins/wizard/skills/generate/template.sh @@ -66,6 +66,7 @@ SKIPPED=() # things we couldn't do (e.g. gh missing, gh errors) _WIZARD_TMP="" # if-form, not `[[ ]] &&`: a false condition must not leave a nonzero status # for the EXIT trap under set -e, which would turn a successful run into exit 1. +# shellcheck disable=SC2329 # invoked by the EXIT trap below _cleanup() { if [[ -n "$_WIZARD_TMP" && -f "$_WIZARD_TMP" ]]; then rm -f -- "$_WIZARD_TMP"; fi; } trap _cleanup EXIT @@ -456,6 +457,7 @@ set_secret() { _gh_set secret "$1" "$2"; } # set_var NAME VALUE — set a GitHub Actions repo variable (non-secret), value # piped over stdin (gh reads standard input when --body is omitted), never # argv. Same refusals as set_secret. +# shellcheck disable=SC2329 # library API; the example stage does not call it set_var() { _gh_set variable "$1" "$2"; } # finish — clear, then a closing summary of everything configured. Names only @@ -478,26 +480,34 @@ finish() { # STAGES — author this section. One stage() per step the human takes. # REPLACE the example below with your real stages, in dependency order, and # set TOTAL_STAGES to match the stages you write. +# Keep every stage inside run_wizard, ahead of its closing `finish` and `exit`, +# and keep the last line as is: bash then parses the whole file before the +# first prompt, so editing the script while it runs cannot re-run a step. # ────────────────────────────────────────────────────────────────────────── -TOTAL_STAGES=1 - -banner "Stripe setup" - -# ── EXAMPLE STAGE — replace me ──────────────────────────────────────────── -# shellcheck disable=SC2154 # ask/ask_secret assign the named variable via printf -v -stage "Stripe — API keys" -say "We'll grab your Stripe test keys and store them for local dev + CI." -open_url "https://dashboard.stripe.com/test/apikeys" -step "On the API keys page, copy the Publishable key (starts pk_test_)." -ask STRIPE_PUBLISHABLE_KEY "Paste the publishable key:" -step "Click 'Reveal test key' on the Secret key row, then copy it." -ask_secret STRIPE_SECRET_KEY "Paste the secret key:" -# shellcheck disable=SC2154 # assigned by ask via printf -v -write_env STRIPE_PUBLISHABLE_KEY "$STRIPE_PUBLISHABLE_KEY" -# shellcheck disable=SC2154 # assigned by ask_secret via printf -v -write_env STRIPE_SECRET_KEY "$STRIPE_SECRET_KEY" -set_secret STRIPE_SECRET_KEY "$STRIPE_SECRET_KEY" # CI needs this one -# ── END EXAMPLE STAGE ───────────────────────────────────────────────────── - -finish +run_wizard() { + TOTAL_STAGES=1 + + banner "Stripe setup" + + # ── EXAMPLE STAGE — replace me ────────────────────────────────────────── + # shellcheck disable=SC2154 # ask/ask_secret assign the named variable via printf -v + stage "Stripe — API keys" + say "We'll grab your Stripe test keys and store them for local dev + CI." + open_url "https://dashboard.stripe.com/test/apikeys" + step "On the API keys page, copy the Publishable key (starts pk_test_)." + ask STRIPE_PUBLISHABLE_KEY "Paste the publishable key:" + step "Click 'Reveal test key' on the Secret key row, then copy it." + ask_secret STRIPE_SECRET_KEY "Paste the secret key:" + # shellcheck disable=SC2154 # assigned by ask via printf -v + write_env STRIPE_PUBLISHABLE_KEY "$STRIPE_PUBLISHABLE_KEY" + # shellcheck disable=SC2154 # assigned by ask_secret via printf -v + write_env STRIPE_SECRET_KEY "$STRIPE_SECRET_KEY" + set_secret STRIPE_SECRET_KEY "$STRIPE_SECRET_KEY" # CI needs this one + # ── END EXAMPLE STAGE ─────────────────────────────────────────────────── + + finish + exit +} + +run_wizard "$@" diff --git a/plugins/wizard/skills/generate/template.test.sh b/plugins/wizard/skills/generate/template.test.sh index 7cb924fd7f..af06788811 100755 --- a/plugins/wizard/skills/generate/template.test.sh +++ b/plugins/wizard/skills/generate/template.test.sh @@ -205,8 +205,8 @@ assert_eq "library defines the full helper API a generated wizard calls" "" "$mi STAGES_SECTION="$TEST_TMPDIR/stages.sh" awk 'found { print } /^# STAGES/ { found = 1 }' "$TEMPLATE" >"$STAGES_SECTION" assert_eq "example TOTAL_STAGES matches the stage() calls below the marker" \ - "$(grep -E '^TOTAL_STAGES=' "$STAGES_SECTION" | tail -n1 | cut -d= -f2)" \ - "$(grep -cE '^stage "' "$STAGES_SECTION")" + "$(grep -E '^[[:space:]]*TOTAL_STAGES=' "$STAGES_SECTION" | tail -n1 | cut -d= -f2)" \ + "$(grep -cE '^[[:space:]]*stage "' "$STAGES_SECTION")" # --- 2. Fail-closed without a terminal -------------------------------------- @@ -1199,6 +1199,51 @@ else skip_case "TERM=dumb case: util-linux script (pty) unavailable" fi +# --- 12. Editing the script while it runs ----------------------------------- + +# bash reads a script as it runs it, from a file offset. An in-place edit that +# lengthens the file shifts earlier bytes under that offset, so an unwrapped +# script runs them again. The shipped wrapper (stages inside run_wizard, which +# ends in `exit`) is parsed whole before the first stage runs. The case swaps +# the example stages for a body that rewrites its own script in place, with a +# pad line exactly as long as the two lines already run, then counts step-1. +# The unwrapped control proves the edit does re-run a step when nothing guards it. +EDIT_BODY="$TEST_TMPDIR/edit-body.sh" +cat >"$EDIT_BODY" <<'BODY' +echo "ran:step-1" +[[ -e "$0.done" ]] || { : >"$0.done"; cat "$0.edited" >"$0"; } +echo "ran:step-2" +BODY +edit_case() { + local mode="$1" script pad + script="$(mktemp "$TEST_TMPDIR/edit.XXXXXX")" + sed 's|exec 3 0) print line + skip = 1; next + } + skip && /^ finish$/ { skip = 0; after = 1 } + skip { next } + after && mode == "unwrapped" && (/^}$/ || /^run_wizard "\$@"$/) { next } + { print }' >"$script" + pad=$(($(head -n2 "$EDIT_BODY" | wc -c) - 2)) + { printf '#%*s\n' "$pad" '' | tr ' ' x; cat "$script"; } >"$script.edited" + case_exec "$script" "$TTY_EOF" +} +assert_eq "the shipped template ends by calling run_wizard (stages are parsed whole)" \ + 'run_wizard "$@"' "$(tail -n1 "$TEMPLATE")" +out="$(edit_case wrapped)" +rc=$? +assert_eq "an in-place edit mid-run does not re-run an earlier step" 1 \ + "$(grep -c '^ran:step-1$' <<<"$out")" +assert_contains "... and the wizard still runs to its finish" "$out" "Setup complete" +assert_exit "... exiting 0, reading nothing past the run_wizard call" 0 "$rc" +out="$(edit_case unwrapped)" +assert_eq "control: the same edit re-runs step-1 when the stages are not wrapped" 2 \ + "$(grep -c '^ran:step-1$' <<<"$out")" + # --- Tally ------------------------------------------------------------------ printf '\n%d case(s), %d failure(s), %d skip(s)\n' "$CASE_NUM" "$FAILED" "$SKIPPED_N" From 021352c6824c95e1542ac4ca631cd1a9f5eab477 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sun, 11 Oct 2026 00:15:07 -0400 Subject: [PATCH 2/2] fix(wizard): have the trace check the run_wizard wrapper and name the layout pin Co-Authored-By: Claude Opus 5.5 --- plugins/wizard/skills/generate/SKILL.md | 4 +++- plugins/wizard/skills/generate/template.test.sh | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/plugins/wizard/skills/generate/SKILL.md b/plugins/wizard/skills/generate/SKILL.md index a27a7fa494..b423a54ca0 100644 --- a/plugins/wizard/skills/generate/SKILL.md +++ b/plugins/wizard/skills/generate/SKILL.md @@ -97,7 +97,9 @@ away. Don't touch the library above the marker. verify that every value from step 1 is captured and lands where step 1 said, that every `set_secret`/`set_var` name exactly matches a `secrets.*`/`vars.*` - reference in CI, and that nothing above the `STAGES` marker was edited. + reference in CI, that nothing above the `STAGES` marker was edited, and + that every stage sits inside `run_wizard`, which ends with `finish` then + `exit`, and the script's last line is `run_wizard "$@"`. 3. **Stop the line. Human approval gate.** Print the full `STAGES` block (everything below the marker) to the user and get their explicit approval. Do NOT `chmod +x` the script, and do NOT tell the user to run it, until they diff --git a/plugins/wizard/skills/generate/template.test.sh b/plugins/wizard/skills/generate/template.test.sh index af06788811..5fb839e4f6 100755 --- a/plugins/wizard/skills/generate/template.test.sh +++ b/plugins/wizard/skills/generate/template.test.sh @@ -1232,7 +1232,7 @@ edit_case() { { printf '#%*s\n' "$pad" '' | tr ' ' x; cat "$script"; } >"$script.edited" case_exec "$script" "$TTY_EOF" } -assert_eq "the shipped template ends by calling run_wizard (stages are parsed whole)" \ +assert_eq "layout pin: the shipped template's last line is the run_wizard call (exit stays inside it)" \ 'run_wizard "$@"' "$(tail -n1 "$TEMPLATE")" out="$(edit_case wrapped)" rc=$?