IW-436: review-state CLI — fix five flag-parsing and update-semantics defects #499

Merged

mph merged 54 commits from IW-436 into main 2026-09-01 19:57:52 +00:00

Conversation 0 Commits 54 Files changed 28 +8971 -61

mph commented 2026-09-01 15:36:57 +00:00

Owner

Copy link

Fixes the five defects reported in IW-436, each in its own phase with its own reviewed PR.

Defects fixed

Phase	Defect	Where
1	--checkpoint selected a merge mode that wiped the whole checkpoint map, so a new phase destroyed every earlier phase's checkpoint	ReviewStateCliParser.parseArrayMode
1	a checkpoint value was accepted as any string, so a non-SHA landed on the card silently	ReviewStateCliParser.parseCheckpoint
2	review-state update never refreshed git_sha, so the card's SHA named an older commit than the card described	ReviewState.applyUpdate
3	--needs-attention false set the flag to true — the exact opposite of the intent — and exited 0	ReviewStateCliParser presence flags
4	--append-action / --append-badge concatenated blindly, so repeating one produced a duplicate button or badge	ReviewStateUpdater.mergeArray

Phases 1 and 3 change a published CLI contract: a wrong invocation that was previously accepted now exits 1 with a message naming the remedy. That was settled in the analysis as a straight semantic flip — no deprecation period, no warning mode.

Deliberately not done

• Existing cards are not repaired. The 26 cards holding a malformed context_sha stay as they are, and destroyed checkpoints are not recovered. The SHA check lives in parseCheckpoint only — putting it in ReviewStateValidator or the schema would fail the next update of every still-corrupt card.
• Artifacts are not de-duplicated. mergeArray's new keyField is given no key for artifacts: IW-206 holds two Review Packet entries with the same label and different paths, so a label key would destroy one. Evidence and the 9-of-55 measurement are on #200, where the key decision belongs.
• git_sha is refreshed on the update path only. Six phase commands still write a stale SHA through the ReviewStateOps port. Accepted residual, now filed as #493.

Testing

Unit tier throughout, TDD, every phase writing the failing test first and confirming it failed for the stated reason. Phase 1 also closed a standing coverage gap: ReviewStateUpdaterTest.scala had no checkpoint tests at all, which is why the map-wiping defect shipped and then destroyed checkpoints across three phases of IW-437. Replace, Append and Clear cases for phase_checkpoints are now covered.

Phase 3's rejection tests assert the full error message rather than isLeft, after mutation testing showed four separate mistakes stayed green under isLeft-only assertions. That rule is now written down in docs/testing.md.

Lessons

All 13 entries in LESSONS.md are triaged and decided. Most already had a home on the tracker, so the usual outcome was evidence attached to an open issue rather than a new one.

• New: #492, #493, #494, #495, #496
• Evidence added to: #463, #468, #443, #200
• Rejected as already filed: phase-commit --items comma splitting (#337)
• Landed in this branch: the docs/testing.md rule, and a correction to the phase-04 documents that had mismarked one reproduction as a guard

Release notes

project-management/issues/IW-436/release-notes.md (Czech).

Fixes the five defects reported in IW-436, each in its own phase with its own reviewed PR. ## Defects fixed | Phase | Defect | Where | |---|---|---| | 1 | `--checkpoint` selected a merge mode that **wiped the whole checkpoint map**, so a new phase destroyed every earlier phase's checkpoint | `ReviewStateCliParser.parseArrayMode` | | 1 | a checkpoint value was accepted as any string, so a non-SHA landed on the card silently | `ReviewStateCliParser.parseCheckpoint` | | 2 | `review-state update` never refreshed `git_sha`, so the card's SHA named an older commit than the card described | `ReviewState.applyUpdate` | | 3 | `--needs-attention false` set the flag to **`true`** — the exact opposite of the intent — and exited 0 | `ReviewStateCliParser` presence flags | | 4 | `--append-action` / `--append-badge` concatenated blindly, so repeating one produced a duplicate button or badge | `ReviewStateUpdater.mergeArray` | Phases 1 and 3 change a published CLI contract: a wrong invocation that was previously accepted now exits 1 with a message naming the remedy. That was settled in the analysis as a straight semantic flip — no deprecation period, no warning mode. ## Deliberately not done - **Existing cards are not repaired.** The 26 cards holding a malformed `context_sha` stay as they are, and destroyed checkpoints are not recovered. The SHA check lives in `parseCheckpoint` only — putting it in `ReviewStateValidator` or the schema would fail the next `update` of every still-corrupt card. - **Artifacts are not de-duplicated.** `mergeArray`'s new `keyField` is given no key for artifacts: IW-206 holds two `Review Packet` entries with the same label and *different* paths, so a `label` key would destroy one. Evidence and the 9-of-55 measurement are on #200, where the key decision belongs. - **`git_sha` is refreshed on the `update` path only.** Six phase commands still write a stale SHA through the `ReviewStateOps` port. Accepted residual, now filed as #493. ## Testing Unit tier throughout, TDD, every phase writing the failing test first and confirming it failed for the stated reason. Phase 1 also closed a standing coverage gap: `ReviewStateUpdaterTest.scala` had **no checkpoint tests at all**, which is why the map-wiping defect shipped and then destroyed checkpoints across three phases of IW-437. Replace, Append and Clear cases for `phase_checkpoints` are now covered. Phase 3's rejection tests assert the **full** error message rather than `isLeft`, after mutation testing showed four separate mistakes stayed green under `isLeft`-only assertions. That rule is now written down in `docs/testing.md`. ## Lessons All 13 entries in `LESSONS.md` are triaged and decided. Most already had a home on the tracker, so the usual outcome was evidence attached to an open issue rather than a new one. - New: #492, #493, #494, #495, #496 - Evidence added to: #463, #468, #443, #200 - Rejected as already filed: `phase-commit --items` comma splitting (#337) - Landed in this branch: the `docs/testing.md` rule, and a correction to the phase-04 documents that had mismarked one reproduction as a guard ## Release notes `project-management/issues/IW-436/release-notes.md` (Czech).

mph added 52 commits 2026-09-01 15:36:57 +00:00

workflow(dx): Create diagnostic analysis for IW-436 ... 4498215a65

review-state CLI: flag parsing and update semantics defects

Artifacts:
- analysis.md
- LESSONS.md (two findings recorded out of scope)

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fpcp15SVY87KSVrCcgWy2H

chore(IW-436): update review-state to analysis_ready ... d5219ed8f5

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fpcp15SVY87KSVrCcgWy2H

workflow: Resolve clarifications for IW-436 ... 08412acf7a

Resolved:
- analysis.md: Repair path for the 26 already-corrupt cards -> no repair; cards
  are ephemeral, existing corruption stays, validator/schema not tightened
- analysis.md: Deprecation and release coordination -> straight semantic flip,
  hard failure for defects 1 and 3, no version floor (no consumer pins a release)
- analysis.md: Reach of the git_sha refresh -> CLI update path only, port untouched

Also corrected the wrong premise that consumer projects pin their own iw-cli
release, which had leaked into the breaking-change surface and the external
blockers.

LESSONS.md entry 3 records the six phase commands that still leave git_sha
stale, as an accepted residual with a proposed tracker issue.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fpcp15SVY87KSVrCcgWy2H

chore(IW-436): clear needs-attention after clarifications resolved ... de59a7ba02

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fpcp15SVY87KSVrCcgWy2H

workflow(dx): Create task index for IW-436 ... 237e4355c8

review-state CLI: flag parsing and update semantics defects

Artifacts:
- tasks.md (4 phases, 7.5-13.5h)
- review-state.json (tasks_ready)

Defects 3 and 4 are grouped into Phase 1: they edit the same checkpoint path
and the same test file, and defect 3 must rewrite the very values defect 4's
contract change also touches.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fpcp15SVY87KSVrCcgWy2H

Phase 1: context ready ... 130ac13a76

Issue: IW-436
Phase: 1 - `--checkpoint` corrupts and destroys the checkpoint map (defects 3 + 4)

This commit marks the checkpoint for phase 1 context generation.
Use dx-fix IW-436 to return to this point.

🤖 Generated with Claude Code

chore(IW-436): update review-state to context_ready ef1e897dc5

workflow(dx): Generate Phase 1 tasks for IW-436 ... 1e03340bda

`--checkpoint` corrupts and destroys the checkpoint map (defects 3 + 4)

Artifacts:
- phase-01-tasks.md

🤖 Generated with Claude Code

chore(IW-436): update review-state to tasks_ready cbee1c879e

chore(IW-436): update review-state for phase 01 520277a59b

Fix --checkpoint SHA validation and append semantics (defects 3 + 4) ... a16b79fd1e

- CONTRACT CHANGE: --checkpoint now accumulates instead of replacing phase_checkpoints (defect 4) — replace silently destroyed earlier phases across two issues and six phases; --clear-checkpoints remains the only way to drop entries and clear-and-set now needs two invocations
- parseCheckpoint rejects a value that is not 7-64 hex characters (defect 3) — the doubled-key form produced 26 malformed context_sha entries in this repository
- parseArrayMode gained unionOnAppend: Boolean; only the checkpoint call site passes it and the other four array fields keep the exclusive clear > replace > append precedence
- Rewrote the test that pinned the defective Replace default — a deliberate contract change and not a test bent to make the fix pass
- Closed the checkpoint coverage gap with 5 phase_checkpoints merge cases that pass against an unmodified ReviewStateUpdater — which is what localises the defect to mode selection; the two Replace cases pin a branch no production path reaches after this fix and are kept on purpose
- ReviewStateValidator and review-state.schema.json deliberately gain no SHA pattern — the validator runs on the merged whole file and would fail the next update of every still-corrupt card

chore(IW-436): update review-state for phase 01 965fc7fb84

Merge pull request 'Phase 1: Fix --checkpoint SHA validation and append semantics (defects 3 + 4)' (#482) from IW-436-phase-01 into IW-436 ... c3e00360b7

Reviewed-on: #482

chore(IW-436): update review-state for phase 01 95202e0eef

chore(IW-436): mark phase 01 complete in tasks.md ee5de5fdcb

Phase 2: context ready ... ce4abc8c60

Issue: IW-436
Phase: 2 - `review-state update` does not refresh `git_sha` (defect 5)

This commit marks the checkpoint for phase 2 context generation.
Use dx-fix IW-436 to return to this point.

🤖 Generated with Claude Code

chore(IW-436): update review-state to context_ready 367c83999e

docs(IW-436): record JDK_JAVA_OPTIONS breaking Mill daemon RPC ... 146ac9dd41

LESSONS.md entry 5. A set JDK_JAVA_OPTIONS makes every JVM Mill spawns
print a NOTE line onto the daemon's RPC pipe, so the daemon fails to
parse it as JSON and the launcher reports a crashed worker. Environment
fault, not an iw-cli defect; workaround is `env -u JDK_JAVA_OPTIONS`.

🤖 Generated with Claude Code

workflow(dx): Generate Phase 2 tasks for IW-436 ... e6663417bc

`review-state update` does not refresh `git_sha` (defect 5)

Artifacts:
- phase-02-tasks.md

🤖 Generated with Claude Code

chore(IW-436): update review-state to tasks_ready 86a1746b46

chore(IW-436): update review-state for phase 02 b2658db7c7

review-state update refreshes git_sha (defect 5) ... d8e45b9c97

- applyUpdate fills git_sha from HEAD via a shared currentGitSha helper; an explicit --git-sha still wins, and a git failure preserves the card's existing value
- 6 harness cases and 1 BATS case covering refresh, override, --commit ordering and the preserve-on-failure branch
- FakeGit gained a getHeadSha failure hook, closing a fixture gap that left a non-repository unsimulatable at every tier
- docs/commands/review-state.md now states the real git_sha contract on update
- LESSONS entries 5-7 record three findings left deliberately unfixed under RESOLVED 3

Phase 2 of IW-436. The pure core is untouched: core/model/, the ReviewStateOps
port, ReviewStateAdapter, FakeReviewStateOps and the schema all show zero diff.

🤖 Generated with Claude Code

chore(IW-436): update review-state for phase 02 e7e6a68c7c

Merge pull request 'Phase 2: review-state update refreshes git_sha (defect 5)' (#484) from IW-436-phase-02 into IW-436 ... 6e0242abd3

Reviewed-on: #484

chore(IW-436): update review-state for phase 02 700bf82fc3

chore(IW-436): mark phase 02 complete in tasks.md 671c5ba5fa

Phase 3: context ready ... 11603380c7

Issue: IW-436
Phase: 3 - `--needs-attention false` sets the flag to `true` (defect 1)

This commit marks the checkpoint for phase 3 context generation.
Use dx-fix IW-436 to return to this point.

Also records LESSONS entry 8: the pre-push hook reports a Bloop BSP
timeout as a compile failure.

🤖 Generated with Claude Code

chore(IW-436): update review-state to context_ready 35e4d8286e

workflow(dx): Generate Phase 3 tasks for IW-436 ... fbb741d8b5

`--needs-attention false` sets the flag to `true` (defect 1)

Artifacts:
- phase-03-tasks.md

🤖 Generated with Claude Code

chore(IW-436): update review-state to tasks_ready 6c168c9d5c

chore(IW-436): update review-state for phase 03 0b5c6a354e

Presence flags refuse a following value (defect 1) ... 2f3fc26e39

CLI CONTRACT CHANGE. `--needs-attention` and the 13 clear flags are
set-only. A value after one of them was previously read by nobody, so
`--needs-attention false` wrote `needs_attention: true` at exit 0. Such
an invocation now exits 1 and names the flag and the stray token. Per
RESOLVED 2 this is a straight semantic flip: no warning mode, no
deprecation period, no version floor.

- `parsePresenceFlag` scans every occurrence, delegating to
  `extractRepeatedFlag` — an `indexOf` shape would pass
  `--needs-attention --status x --needs-attention false`. It covers all
  14 flag names across 15 call sites, so the 13 sibling instances of the
  same defect close with it.
- `--needs-attention` carries the remedy a user cannot guess:
  `--clear-needs-attention` removes the field.
- 146 test lines added, none removed. Every rejection asserts the exact
  message, not `isLeft` — the message is the deliverable.
- Docs: both clear-flag tables state the rule, and the table gains the
  two rows it was missing, `--clear-activity` and `--clear-workflow-type`.

🤖 Generated with Claude Code

chore(IW-436): update review-state for phase 03 3fc36b5107

Merge pull request 'Phase 3: --needs-attention false sets the flag to true (defect 1)' (#488) from IW-436-phase-03 into IW-436 ... 48ebe754aa

Reviewed-on: #488

chore(IW-436): update review-state for phase 03 c298a8aa16

chore(IW-436): mark phase 03 complete in tasks.md d6d95a78e1

Phase 4: context ready ... 68267da282

Issue: IW-436
Phase: 4 - `--append-action` does not de-duplicate by id (defect 2)

This commit marks the checkpoint for phase 4 context generation.
Use dx-fix IW-436 to return to this point.

🤖 Generated with Claude Code

chore(IW-436): update review-state to context_ready f574137395

Phase 4: widen scope to badges ... 9de3b64271

Issue: IW-436
Phase: 4 - `--append-action` and `--append-badge` do not de-duplicate (defect 2)

The keyField parameter that fixes actions fixes badges at the same call
site cost: Some("id") for available_actions, Some("label") for badges.

Badges duplicate on 22 of 28 cards that carry them, no card holds one
label with two type values, and badges render one-to-one in the card
view - so a duplicate is visible noise and a label key loses nothing.

Artifacts stay keyed None. IW-206 holds two artifacts both labelled
"Review Packet" pointing at different files, so a label key would
destroy one. Task lists stay None; no card uses the field.

This widens the fix strategy's "actions only" recommendation, which is
not one of RESOLVED 1-3. Its premise was that badge duplication is not
reported; measurement supersedes that.

Estimate raised 1-2h to 2-3h - the test count roughly doubles.

🤖 Generated with Claude Code

chore(IW-436): update review-state to context_ready 8b7a536eab

workflow(dx): Generate Phase 4 tasks for IW-436 ... bb410e565a

`--append-action` and `--append-badge` do not de-duplicate (defect 2)

Artifacts:
- phase-04-tasks.md

🤖 Generated with Claude Code

chore(IW-436): update review-state to tasks_ready 11f7e02af9

chore(IW-436): update review-state for phase 04 9bfe1549c9

--append-action and --append-badge de-duplicate (defect 2) ... 9651a2e62e

`mergeArray`'s Append arm concatenated with no key, so appending an action
whose id the card already offered stored a second entry instead of replacing
the first. 29 cards in this repository hold a duplicate action id and 22 hold
a duplicate badge label.

- `mergeArray` gains an optional `keyField` and a private `elementKey` that
  reads the key off a `ujson.Value`. The key is read after `toJson`, on both
  sides: the existing array holds `ujson.Value` while the incoming values are
  `List[T]` with `T` differing per call site, so a `T => Option[String]` could
  not compare the two.
- The Append arm overwrites the first match in place — preserving its index,
  because button order is the caller's decision — and removes any further
  match in reverse index order. A forward loop shifts the indices still to be
  visited; it was written that way first and threw `IndexOutOfBoundsException`
  on the three-duplicate case, which is what that case exists to catch.
- Two of four call sites opt in: `available_actions` on `id`, `badges` on
  `label`. `keyField = None` is byte-for-byte the previous behaviour.
- 16 tests added. `ReviewStateUpdaterTest` had no case for `available_actions`,
  `badges` or `task_lists` across 479 lines — `mergeArray` was covered through
  artifacts alone, the one field of the four whose duplicate tolerance is
  correct. That gap is why the defect shipped.

CLI contract change: `--append-action` with an existing `id`, and
`--append-badge` with an existing `label`, now collapse to exactly one entry at
the index the first match occupied, last-wins across every field. Every match
is collapsed, not just the first.

Not covered, deliberately: `--append-artifact` and `--append-task-list` are
unchanged. Artifacts are excluded for data loss rather than YAGNI — IW-206
holds two artifacts labelled `Review Packet` pointing at different files, so a
`label` key would destroy one of them; a regression test pins that shape.
`--action` and `--badge` (Replace) still admit a duplicate within one
invocation, `review-state write` does not de-duplicate, and existing cards get
no repair pass (RESOLVED 1) — a card is healed only for the keys its own
workflow re-appends.

Scope: `analysis.md` recommended actions only, on the premise that duplicate
badges are not reported. That premise was false as measured — 22 of 28 cards
carrying badges hold a duplicate label and every badge renders one-to-one — so
Michal widened the scope to badges. RESOLVED 1, 2 and 3 are untouched by that.

Docs: `docs/commands/review-state.md` `:96` and `:101` name the key field of a
badge and of an action and state that `write` does not de-duplicate; `:165`
lifts badges and actions out of the "same pattern as artifacts" sentence and
records why artifacts stay excluded.

No existing test was edited to make the fix pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fpcp15SVY87KSVrCcgWy2H

docs(IW-436): record CaskServerTest port race blocking the push ... c536e74888

A pre-push run failed one dashboard test that the change cannot reach; a
standalone re-run and the following push were both green. The test picks a
port with findAvailablePort() and then binds Undertow to it, and the full
suite starts dozens of servers within seconds.

Third instance of a pre-push gate failing for reasons unrelated to the change
being pushed (entries 4 and 8 are the others).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fpcp15SVY87KSVrCcgWy2H

chore(IW-436): update review-state for phase 04 1ff7868c54

chore(IW-436): update review-state for phase 04 6c0a3c484c

chore(IW-436): update review-state for phase 04 36fb7b6960

Merge pull request 'Phase 4: --append-action and --append-badge do not de-duplicate (defect 2)' (#490) from IW-436-phase-04 into IW-436 ... 6773f60dba

Reviewed-on: #490

chore(IW-436): update review-state for phase 04 81530eb881

chore(IW-436): mark phase 04 complete in tasks.md 56007507ba

workflow: Triage lessons for IW-436 ... 88f99e406c

All 13 entries decided. Most already had a home on the tracker, so the
common outcome was evidence attached to an open issue, not a new one.

Promoted to a new tracker issue:
- checkpoint phase key is unconstrained -> #492
- six phase commands leave git_sha stale -> #493
- JDK_JAVA_OPTIONS breaks Mill's daemon RPC -> #494
- --git-sha documented on `write`, ignored there -> #495
- needs_attention is a tri-state on the dashboard, two-state on the CLI -> #496

Promoted as evidence on an existing issue:
- extractFlag takes a flag as a value -> #463
- project-commands-execute.bats flake, CaskServerTest port race -> #468
- pre-push reports a Bloop BSP timeout as a compile failure -> #443
- artifact labels are not identities -> #200

Promoted in this branch:
- assert the message, not isLeft, when the message is the contract
  -> docs/testing.md
- phase-04 case 9 was mismarked [guard]; it fails today
  -> phase-04-context.md, phase-04-tasks.md

Rejected:
- phase-commit --items splits on commas: already filed as #337 (and #471,
  closed as its duplicate).

Also adds the Czech release notes for the issue.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fpcp15SVY87KSVrCcgWy2H

mph added 2 commits 2026-09-01 19:43:53 +00:00

chore(IW-436): update review-state to all_complete f77d8d5797

docs(IW-436): record the kanon caller breakage from the Phase 3 flip ... db08d4ebbe

Running dx-fix Step 10 for this issue failed on the workflow's own snippet:
--needs-attention true now exits 1, as Phase 3 intends. 12 occurrences across
11 kanon files are affected — every workflow's completion and refactoring flow.

Filed as dev-docs#200. Recorded as LESSONS.md entry 14, already triaged:
analysis.md RESOLVED 2 settled "no release-order constraint" without ever
running the one rg command that would have shown the callers.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fpcp15SVY87KSVrCcgWy2H

mph merged commit 995a11b0b4 into main 2026-09-01 19:57:52 +00:00

mph referenced this pull request from a commit 2026-09-01 19:57:53 +00:00

Merge pull request 'IW-436: review-state CLI — fix five flag-parsing and update-semantics defects' (#499) from IW-436 into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

agent-ready

  

bug

Something isn't working

  

contract

Run the tool-contract suite on this PR

  

documentation

Improvements or additions to documentation

  

duplicate

This issue or pull request already exists

  

enhancement

New feature or request

  

good first issue

Good for newcomers

  

help wanted

Extra attention is needed

  

invalid

This doesn't seem right

  

needs-human

  

needs-input

Intake asked the human a question on the issue; waiting for the answer

  

question

Further information is requested

  

wontfix

This will not be worked on

No labels

agent-ready

bug

contract

documentation

duplicate

enhancement

good first issue

help wanted

invalid

needs-human

needs-input

question

wontfix

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

iterative-works/iw-cli!499

No description provided.