iterative-works/iw-cli

Watch

Fork 0

review-state CLI: flag parsing and update semantics defects #436

New issue

Open

opened 2026-08-23 19:28:13 +00:00 by mph · 1 comment

mph commented

2026-08-23 19:28:13 +00:00

Owner

Five defects in the review-state CLI surfaced while running the waterfall workflow for IW-423. They share a component and a root cause shape: a flag whose parsing does not match what every caller writes, with no validation to make a wrong invocation fail loudly.

1. `--needs-attention false` sets the flag to `true`

--needs-attention is a presence flag, not a value flag:

// core/model/ReviewStateCliParser.scala:153-154 (and :201-202)
needsAttention =
  if hasFlag(args, "--needs-attention") then Some(true) else None,

So ./iw review-state update --needs-attention false sets needs_attention to true and leaves false as an ignored stray argument — the exact opposite of the caller's intent, silently. Clearing needs --clear-needs-attention (:223).

The wf-create-tasks command's Step 8 script tells the caller to run --needs-attention false. Following it verbatim leaves every waterfall issue flagged as needing attention at tasks_ready, polluting the dashboard's pending-act count.

Fix: reject --needs-attention <value> rather than ignoring the value.

2. `--append-action` does not de-duplicate by id

wf-create-analysis Step 9 writes --action "implement:Start Implementation:wf-implement". wf-create-tasks Step 8 then runs --append-action with the same action, so available_actions ends up holding two identical implement entries. Nothing validates against duplicate action ids.

Fix: de-duplicate on append by id.

3. `--checkpoint` takes `phase:sha`, not `phase:context_sha=sha`

ReviewStateCliParser.parseCheckpoint (core/model/ReviewStateCliParser.scala:75-79) splits on the first : and returns (phase, sha). ReviewStateUpdater (:198, :207) then writes the context_sha key itself:

checkpoints(key) = ujson.Obj("context_sha" -> ujson.Str(sha))

The wf-implement Step 6a script says --checkpoint "${PHASE_NUM}:context_sha=${CONTEXT_SHA}". Following it verbatim writes "context_sha": "context_sha=ed7d788…". ReviewStateValidator only checks that context_sha is present and is a string, so it passes, and the resume path compares a non-SHA against git history.

Fix: validate the checkpoint value as a SHA in parseCheckpoint.

4. `--checkpoint` replaces the whole checkpoint map, wiping earlier phases

--checkpoint defaults to replace mode. ReviewStateUpdater (core/model/ReviewStateUpdater.scala:194-201) builds a fresh ujson.Obj and assigns it over phase_checkpoints:

case ArrayMergeMode.Replace =>
  if values.nonEmpty then
    val checkpoints = ujson.Obj()
    values.toList.sortBy(_._1).foreach { case (key, sha) => ... }
    existing("phase_checkpoints") = checkpoints

So --checkpoint "2:${SHA}" dropped phase 1's checkpoint. Nothing in the state file records what was lost. Passing both --checkpoint and --append-checkpoint in one call does not help either: replace mode wins and the appended value is dropped silently.

Fix: make append the default for --checkpoint. A checkpoint map is an accumulating record of phases; no caller wants phase N's checkpoint to delete phase N-1's. Replace would then need an explicit --clear-checkpoints.

5. `review-state update` does not refresh `git_sha`

ReviewStateUpdater documents git_sha as "preserved unless explicitly provided in update" (core/model/ReviewStateUpdater.scala:78,130). Only the write path fills it from env.git.getHeadSha (core/commands/ReviewState.scala:135).

Every wf-* command block claims the CLI "auto-populates required fields (version, issue_id, git_sha, last_updated)". That is true of write and false of update. For IW-423 git_sha stayed at the analysis commit through two later commits, and confirmed again on the Phase 2 transition. Step 1 of wf-implement uses the state SHA as a resume sanity check ("state SHA is HEAD or ancestor"), which a stale value passes vacuously.

Fix: refresh git_sha on update from HEAD. It is a record of the state, not user input.

Recorded during IW-423; see project-management/issues/IW-423/LESSONS.md entries 1, 2, 4, 5, 13, 14.
The paired command-text corrections in dev-docs are filed separately as a kanon note.

Five defects in the `review-state` CLI surfaced while running the waterfall workflow for IW-423. They share a component and a root cause shape: a flag whose parsing does not match what every caller writes, with no validation to make a wrong invocation fail loudly. ## 1. `--needs-attention false` sets the flag to `true` `--needs-attention` is a presence flag, not a value flag: ```scala // core/model/ReviewStateCliParser.scala:153-154 (and :201-202) needsAttention = if hasFlag(args, "--needs-attention") then Some(true) else None, ``` So `./iw review-state update --needs-attention false` sets `needs_attention` to `true` and leaves `false` as an ignored stray argument — the exact opposite of the caller's intent, silently. Clearing needs `--clear-needs-attention` (`:223`). The `wf-create-tasks` command's Step 8 script tells the caller to run `--needs-attention false`. Following it verbatim leaves every waterfall issue flagged as needing attention at `tasks_ready`, polluting the dashboard's pending-act count. **Fix:** reject `--needs-attention <value>` rather than ignoring the value. ## 2. `--append-action` does not de-duplicate by id `wf-create-analysis` Step 9 writes `--action "implement:Start Implementation:wf-implement"`. `wf-create-tasks` Step 8 then runs `--append-action` with the same action, so `available_actions` ends up holding two identical `implement` entries. Nothing validates against duplicate action ids. **Fix:** de-duplicate on append by `id`. ## 3. `--checkpoint` takes `phase:sha`, not `phase:context_sha=sha` `ReviewStateCliParser.parseCheckpoint` (`core/model/ReviewStateCliParser.scala:75-79`) splits on the first `:` and returns `(phase, sha)`. `ReviewStateUpdater` (`:198`, `:207`) then writes the `context_sha` key itself: ```scala checkpoints(key) = ujson.Obj("context_sha" -> ujson.Str(sha)) ``` The `wf-implement` Step 6a script says `--checkpoint "${PHASE_NUM}:context_sha=${CONTEXT_SHA}"`. Following it verbatim writes `"context_sha": "context_sha=ed7d788…"`. `ReviewStateValidator` only checks that `context_sha` is present and is a string, so it passes, and the resume path compares a non-SHA against git history. **Fix:** validate the checkpoint value as a SHA in `parseCheckpoint`. ## 4. `--checkpoint` replaces the whole checkpoint map, wiping earlier phases `--checkpoint` defaults to replace mode. `ReviewStateUpdater` (`core/model/ReviewStateUpdater.scala:194-201`) builds a fresh `ujson.Obj` and assigns it over `phase_checkpoints`: ```scala case ArrayMergeMode.Replace => if values.nonEmpty then val checkpoints = ujson.Obj() values.toList.sortBy(_._1).foreach { case (key, sha) => ... } existing("phase_checkpoints") = checkpoints ``` So `--checkpoint "2:${SHA}"` dropped phase 1's checkpoint. Nothing in the state file records what was lost. Passing both `--checkpoint` and `--append-checkpoint` in one call does not help either: replace mode wins and the appended value is dropped silently. **Fix:** make append the default for `--checkpoint`. A checkpoint map is an accumulating record of phases; no caller wants phase N's checkpoint to delete phase N-1's. Replace would then need an explicit `--clear-checkpoints`. ## 5. `review-state update` does not refresh `git_sha` `ReviewStateUpdater` documents `git_sha` as "preserved unless explicitly provided in update" (`core/model/ReviewStateUpdater.scala:78,130`). Only the `write` path fills it from `env.git.getHeadSha` (`core/commands/ReviewState.scala:135`). Every `wf-*` command block claims the CLI "auto-populates required fields (`version`, `issue_id`, `git_sha`, `last_updated`)". That is true of `write` and false of `update`. For IW-423 `git_sha` stayed at the analysis commit through two later commits, and confirmed again on the Phase 2 transition. Step 1 of `wf-implement` uses the state SHA as a resume sanity check ("state SHA is HEAD or ancestor"), which a stale value passes vacuously. **Fix:** refresh `git_sha` on `update` from HEAD. It is a record of the state, not user input. --- Recorded during IW-423; see `project-management/issues/IW-423/LESSONS.md` entries 1, 2, 4, 5, 13, 14. The paired command-text corrections in dev-docs are filed separately as a kanon note.

mph referenced this issue from a commit

2026-08-23 20:00:59 +00:00

workflow: Triage lessons for IW-423

mph referenced this issue

2026-08-27 19:14:10 +00:00

review-state: a context_sha that is not a git SHA is accepted silently #464

mph referenced this issue

2026-08-27 19:14:17 +00:00

review-state update --checkpoint stores the key inside the value #446

mph commented

2026-08-27 19:14:59 +00:00

Author

Owner

Defects 3 and 4 hit again in IW-437, at all three phases, from the same unchanged caller snippet. Evidence that the loss in defect 4 is real rather than theoretical:

The card's own history shows it: {1: ...} at phase 1, then {2: ...} alone at phase 2 (f294ed1), then {3: ...} alone at phase 3 (349a04a). Two checkpoints were destroyed before anyone looked.
Recovery was possible only from git history: phase 1 (0f64de40) and phase 2 (b66e166e) were read back out of the card's own history, verified to resolve to their "Phase N: context ready" commits, and re-appended alongside phase 3 (ceb86105).
Nothing reported either defect at any point. The write succeeds and the validator is satisfied — the map is well-formed, it is just short. The loss is visible only by diffing the card against its own history.

This supports defect 4's proposed fix (append as the default, explicit --clear-checkpoints for replace) over any caller-side-only correction: the caller snippet was already recorded as wrong after IW-423 (dev-docs note #183) and was still producing corrupt cards three phases later in IW-437.

Two duplicates filed on this ground have been closed in favour of this issue: #464 (from IW-437) and #446 (from MEDECA-430, which proposed the inverse fix — stripping a context_sha= prefix — on a mistaken reading of the intended syntax).

Caller-side fix tracked at dev-docs#187.

Defects 3 and 4 hit again in IW-437, at all three phases, from the same unchanged caller snippet. Evidence that the loss in defect 4 is real rather than theoretical: - The card's own history shows it: `{1: ...}` at phase 1, then `{2: ...}` alone at phase 2 (f294ed1), then `{3: ...}` alone at phase 3 (349a04a). Two checkpoints were destroyed before anyone looked. - Recovery was possible only from git history: phase 1 (0f64de40) and phase 2 (b66e166e) were read back out of the card's own history, verified to resolve to their "Phase N: context ready" commits, and re-appended alongside phase 3 (ceb86105). - Nothing reported either defect at any point. The write succeeds and the validator is satisfied — the map is well-formed, it is just short. The loss is visible only by diffing the card against its own history. This supports defect 4's proposed fix (append as the default, explicit `--clear-checkpoints` for replace) over any caller-side-only correction: the caller snippet was already recorded as wrong after IW-423 (dev-docs note #183) and was still producing corrupt cards three phases later in IW-437. Two duplicates filed on this ground have been closed in favour of this issue: #464 (from IW-437) and #446 (from MEDECA-430, which proposed the inverse fix — stripping a `context_sha=` prefix — on a mistaken reading of the intended syntax). Caller-side fix tracked at dev-docs#187.

mph referenced this issue from a commit

2026-08-28 07:21:43 +00:00

workflow: Triage lessons for IW-437

mph referenced this issue

2026-08-31 08:46:16 +00:00

review-state update cannot write phase or step, so wf-implement's fast-path resume never fires #487

mph referenced this issue

2026-09-02 06:22:57 +00:00

Squash the batch-implement blockers: nine fixes for the phase gate, the CI gate and the review-state card #501