fix(commands): stop assuming shell state survives between blocks

Dogfooding `plan` + `implement` against a throwaway config surfaced one root
defect with many arms: the command templates treat consecutive fenced blocks as
one shell. They are not. Every ```bash fence runs as its own Bash call in its own
process, so a variable set in one block is empty in the next, and `$$` is a
different PID (measured: 21710 vs 22109).

The planner agent confirmed the sharpest arm at runtime, reporting that
`Mode: $RAW_FLAG` "arrived literally unsubstituted" — `--raw` was documented in
three command files while being functionally dead. A machine sweep found the same
root in 20 places across 9 files, well past the two the written fasit predicted:

  - `$RAW_FLAG` read from non-shell agent prompts (analyze, plan, implement)
  - `$TMPFILE` read across blocks (tokens, manifest, whats-active,
    plugin-health) — each command could not read the file it had just written
  - `$GLOBAL_FLAG` across blocks (fix)
  - `$TODAY` never assigned in any block (campaign), passing
    `--reference-date ""` to a write CLI in six places
  - three `$$` temp paths handed to the Read tool (fix), which expands neither

All now follow the hardened drift.md pattern: a fixed literal path, or a
re-derivation inside each block that needs it.

Also fixed, all confirmed against ground truth rather than inferred:

  - `implement` printed a rollback ID it never captured (the timestamp lived only
    inside a command substitution) — the one message a user reads after a bad run
  - `plan` reported "No analysis results found" for valid sessions, because Read
    was pointed at a glob it cannot expand; now uses Glob and verifies the
    analysis report exists before spawning the agent
  - five phase commands wrote state.yaml with two of four required fields; since
    the agent writes all four, a follow-up write silently deleted the rest
  - `implement` promised rollback deletes created files; rollback deliberately
    leaves them (M-BUG-26 still open) — the doc, not the engine, was wrong
  - `implement` claimed a score delta with no pre-change measurement
  - `verifier-agent` was told to write a report it has no tool to write
  - dead `Task` tool name in always-loaded rule context; planner-agent template
    demonstrated the inline file content its own line 110 forbids

The sweeps land as tests/commands/command-shell-state-shape.test.mjs, verified
red before the fix and proven able to fail by reintroducing the defect. Two
existing tests asserted the old bash-block mechanism rather than the intent and
were updated. Suite 1449/0; frozen v5.0.0 snapshots and all scanner code
untouched.

Not fixed, deliberately: neither command scope-gates its actions to the audit
target. The generated plan included an edit to a real file under ~/.claude,
outside the throwaway target, because the skill/agent scanners are machine-wide.
That is a design change, not a side fix.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0195udHgCcFegzm7ecKku2Yc
This commit is contained in:
Kjell Tore Guttormsen 2026-08-01 20:12:17 +02:00
commit 09f817977c
17 changed files with 413 additions and 71 deletions

View file

@ -8,6 +8,53 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## [Unreleased]
### Fixed
- **`M-BUG-40` — command templates assumed shell state survives between fenced blocks.** It does not:
every ```` ```bash ```` fence is executed as its own Bash call, in its own process. A variable
assigned in one block is empty in the next, and `$$` (the PID) differs between calls, so a
`/tmp/foo-$$.json` path created in one block can never be reconstructed in a later one. The defect
was surfaced by dogfooding `plan` + `implement`, and **confirmed at runtime by the planner agent
itself**, which reported that `Mode: $RAW_FLAG` "arrived literally unsubstituted" — `--raw` was
documented in both files while being functionally dead. A machine sweep found the same root in
**20 places across 9 files**, far past the two predicted: `$RAW_FLAG` referenced from non-shell
agent prompts (`analyze`, `plan`, `implement`); `$TMPFILE` referenced across blocks in `tokens`,
`manifest`, `whats-active` and `plugin-health`, so each command could not read the file it had just
written; `$GLOBAL_FLAG` in `fix`; `$TODAY` in `campaign`, which was **never assigned in any block**
and passed `--reference-date ""` to a write CLI; and three `$$` temp paths handed to the Read tool
in `fix`, which expands neither `$$` nor variables. All now follow the hardened `drift.md` pattern:
a fixed literal path, or a re-derivation inside each block that needs it.
- **`implement` handed out a rollback ID it never captured.** The backup directory was created with
`mkdir -p .../$(date +%Y%m%d_%H%M%S)/`, so the timestamp existed only inside a command
substitution, while step 6 promised `/config-audit rollback {timestamp}` — the one message a user
reads after a bad run. The step now prints `BACKUP_ID` and substitutes it literally.
- **`plan` reported "No analysis results found" for valid sessions.** Step 1 pointed the Read tool at
`~/.claude/config-audit/sessions/*/state.yaml`; Read takes one literal path and does not expand
`*`, so the lookup failed and the command reported the session as missing. It now uses Glob, and
additionally verifies `analysis-report.md` exists before spawning the planner agent — a session can
carry a valid `state.yaml` and still be missing its report.
- **Phase commands wrote `state.yaml` with two of the four required fields.** `.claude/rules/state-management.md`
mandates `current_phase`, `completed_phases`, `next_phase` and `updated_at`; `analyze`, `discover`,
`implement`, `interview` and `plan` named only a subset. Because the planner agent writes all four,
a follow-up full-file Write naming two **deletes** the other two — the fields that make an
interrupted run resumable.
- **`implement` documented a rollback semantics that does not exist.** Its "## Rollback" section
promised to "delete newly created files", while `rollback.md` deliberately leaves them in place and
lists them under "Left in place" (deletion is unimplemented; `M-BUG-26` remains open). The doc now
mirrors actual behaviour rather than describing a half-restore as clean.
- **`implement` claimed a score delta with no source**, since nothing captured the pre-change grade
before the edits ran, and its implied posture call omitted both `--output-file` and `2>/dev/null`
required by the output rules. It now reports a delta only when a pre-change grade was actually
measured.
- **`verifier-agent` was instructed to write a report it has no tool to write** (`tools: Read, Glob,
Grep`, and "Read-only validation" by design). It now returns findings as its final message and the
command appends them with Bash `>>`, preserving both the read-only design and the shared-log
append discipline.
- **Dead tool name in always-loaded context:** `.claude/rules/command-development.md` taught
`allowed-tools: ... Task` while every command uses `Agent`, and `interview.md` carried two more
`Task` references. `planner-agent.md` also contradicted itself — line 110 forbids inline file
content while its own output template demonstrated exactly that, pushing plans past the 200-line
budget the same file sets.
### Fixed (previously released work)
- **`M-BUG-39` — every scanner CLI could truncate its own output when piped.** `process.exit()`
terminates immediately, but Node writes stdout **asynchronously** when stdout is a pipe, so whatever
is still buffered is discarded. `scan-orchestrator.mjs` measured **246 854 bytes to a file vs