Phase 4: `--append-action` and `--append-badge` do not de-duplicate (defect 2) #490

Merged

mph merged 6 commits from IW-436-phase-04 into IW-436

2026-09-01 13:21:00 +00:00

mph commented

2026-09-01 12:41:38 +00:00

Owner

Phase 04 implementation for IW-436.

Files

Browse changed files: https://code.iterative.works/iterative-works/iw-cli/-/blob/IW-436-phase-04/

Phase 04 implementation for IW-436. ### Files Browse changed files: https://code.iterative.works/iterative-works/iw-cli/-/blob/IW-436-phase-04/

mph added 2 commits

2026-09-01 12:41:38 +00:00

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

--append-action and --append-badge de-duplicate (defect 2)

CI / contract (pull_request) Has been skipped

Details

CI / compile (pull_request) Successful in 1m16s

Details

CI / format (pull_request) Successful in 7s

Details

CI / lint (pull_request) Successful in 8s

Details

CI / dashboard-build (pull_request) Successful in 55s

Details

CI / test (pull_request) Successful in 10m5s

Details

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

mph added 2 commits

2026-09-01 12:53:47 +00:00

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

CI / contract (pull_request) Has been skipped

Details

CI / format (pull_request) Successful in 8s

Details

CI / lint (pull_request) Successful in 11s

Details

CI / test (pull_request) Has been cancelled

Details

CI / dashboard-build (pull_request) Has been cancelled

Details

CI / compile (pull_request) Has been cancelled