review-state: Artifacts accumulate with duplicates and lack relevance ordering #200

Open
opened 2026-02-13 23:07:27 +00:00 by mprihoda · 4 comments
mprihoda commented 2026-02-13 23:07:27 +00:00 (Migrated from github.com)

Problem

The review-state update --append-artifact mechanism has two issues that degrade the dashboard experience during multi-phase workflows:

1. Duplicate artifacts from blind append

ReviewStateUpdater.mergeArray with ArrayMergeMode.Append blindly concatenates new items — no deduplication by path. The ag-implement workflow appends the same artifact from multiple transition points:

  • "Implementation Log" gets appended in refactoring completion (Step 5.4) AND final completion (Step 10)
  • "Phase N Context" gets appended in ag-implement Step 6a AND ag-implementation-workflow Step 5.5
  • Actions like "implement:Start Implementation:ag-implement" get appended every phase in Step 6b

After a few phases, the artifacts array has multiple identical entries.

2. No relevance signal — current phase artifacts buried at bottom

Even without duplicates, a 5-phase issue accumulates ~15+ artifacts in chronological order. The artifacts the reviewer needs NOW (current phase context, review packet) are at the bottom, requiring scrolling past all historical artifacts.

Proposed Solution

Part 1: Upsert semantics for append

Change --append-artifact (and --append-badge, --append-action) to deduplicate by natural key:

  • Artifacts: deduplicate by path
  • Actions: deduplicate by id
  • Badges: deduplicate by label

When an item with the same key already exists, replace it in-place rather than creating a duplicate.

Part 2: Attention artifacts

Add a separate top-level attention_artifacts array (2-3 items) to review-state.json alongside the full artifacts array. This separates "what you should look at now" from "full history":

  • Dashboard renders attention_artifacts prominently at the top
  • Full artifacts list available for browsing (collapsed or secondary)
  • Workflow replaces attention_artifacts at each phase transition with only the current phase's key documents

New CLI flags: --attention-artifact "label:path" (replace mode) to set the current attention set.

Affected Code

  • .iw/core/model/ReviewStateUpdater.scala — mergeArray function (line 166-186)
  • .iw/commands/review-state/update.scala — CLI flag parsing
  • schemas/review-state.schema.json — add attention_artifacts field
  • Dashboard rendering (artifact display logic)
  • Plugin workflow skills (ag-implement, ag-implementation-workflow) — update to set attention_artifacts at transition points

Notes

  • The upsert change (Part 1) is backward-compatible — existing --append-artifact calls just stop producing duplicates
  • Part 2 requires schema version bump and dashboard changes
  • The full artifacts array accumulation is intentional for reviewer context — we're adding a relevance layer on top, not removing the history
## Problem The `review-state update --append-artifact` mechanism has two issues that degrade the dashboard experience during multi-phase workflows: ### 1. Duplicate artifacts from blind append `ReviewStateUpdater.mergeArray` with `ArrayMergeMode.Append` blindly concatenates new items — no deduplication by path. The ag-implement workflow appends the same artifact from multiple transition points: - **"Implementation Log"** gets appended in refactoring completion (Step 5.4) AND final completion (Step 10) - **"Phase N Context"** gets appended in ag-implement Step 6a AND ag-implementation-workflow Step 5.5 - **Actions** like `"implement:Start Implementation:ag-implement"` get appended every phase in Step 6b After a few phases, the artifacts array has multiple identical entries. ### 2. No relevance signal — current phase artifacts buried at bottom Even without duplicates, a 5-phase issue accumulates ~15+ artifacts in chronological order. The artifacts the reviewer needs NOW (current phase context, review packet) are at the bottom, requiring scrolling past all historical artifacts. ## Proposed Solution ### Part 1: Upsert semantics for append Change `--append-artifact` (and `--append-badge`, `--append-action`) to deduplicate by natural key: - **Artifacts**: deduplicate by `path` - **Actions**: deduplicate by `id` - **Badges**: deduplicate by `label` When an item with the same key already exists, replace it in-place rather than creating a duplicate. ### Part 2: Attention artifacts Add a separate top-level `attention_artifacts` array (2-3 items) to `review-state.json` alongside the full `artifacts` array. This separates "what you should look at now" from "full history": - Dashboard renders `attention_artifacts` prominently at the top - Full `artifacts` list available for browsing (collapsed or secondary) - Workflow replaces `attention_artifacts` at each phase transition with only the current phase's key documents New CLI flags: `--attention-artifact "label:path"` (replace mode) to set the current attention set. ## Affected Code - `.iw/core/model/ReviewStateUpdater.scala` — `mergeArray` function (line 166-186) - `.iw/commands/review-state/update.scala` — CLI flag parsing - `schemas/review-state.schema.json` — add `attention_artifacts` field - Dashboard rendering (artifact display logic) - Plugin workflow skills (ag-implement, ag-implementation-workflow) — update to set attention_artifacts at transition points ## Notes - The upsert change (Part 1) is backward-compatible — existing `--append-artifact` calls just stop producing duplicates - Part 2 requires schema version bump and dashboard changes - The full `artifacts` array accumulation is intentional for reviewer context — we're adding a relevance layer on top, not removing the history
Owner

Measured data on the de-duplication key, from IW-436 (2026-09-01).

IW-436 defect 2 gave ReviewStateUpdater.mergeArray a keyField and used it to de-duplicate available_actions (on id) and badges (on label). Artifacts were deliberately left out, and this issue is where that decision needs settling.

Why label cannot be the key

project-management/issues/IW-206/review-state.json holds label colliding two different ways at once:

$ jq -c '.artifacts[] | {label, path}' project-management/issues/IW-206/review-state.json
{"label":"Phase 1 Context","path":"project-management/issues/IW-206/phase-01-context.md"}
...
{"label":"Phase 1 Context","path":"project-management/issues/IW-206/phase-01-context.md"}
{"label":"Review Packet","path":"project-management/issues/IW-206/review-packet-phase-01.md"}
...
{"label":"Review Packet","path":"project-management/issues/IW-206/review-packet-phase-02.md"}
  • Two Phase 1 Context entries share both label and path — a genuine duplicate, the thing this issue asks to remove.
  • Two Review Packet entries share label but point at different files — a legitimate collision, not a duplicate.

Keying on label alone would collapse the second pair and destroy the phase-1 review packet's only reference.

Nothing in the schema, the validator or ReviewStateBuilder/ReviewStateUpdater distinguishes the two shapes; both are valid by every check the tool owns. 9 of the 55 cards under project-management/ hold a duplicate artifact label.

The question this leaves

Should --append-artifact key on path, on label+path, or stay as it is?

Implementation note for whichever answer is chosen: mergeArray's keyField as added by IW-436 keys on a single string field, so a label+path key needs a small generalisation.


Recorded during IW-436; see project-management/issues/IW-436/LESSONS.md entry 12.

Measured data on the de-duplication key, from IW-436 (2026-09-01). IW-436 defect 2 gave `ReviewStateUpdater.mergeArray` a `keyField` and used it to de-duplicate `available_actions` (on `id`) and `badges` (on `label`). **Artifacts were deliberately left out**, and this issue is where that decision needs settling. ## Why `label` cannot be the key `project-management/issues/IW-206/review-state.json` holds `label` colliding two different ways at once: ``` $ jq -c '.artifacts[] | {label, path}' project-management/issues/IW-206/review-state.json {"label":"Phase 1 Context","path":"project-management/issues/IW-206/phase-01-context.md"} ... {"label":"Phase 1 Context","path":"project-management/issues/IW-206/phase-01-context.md"} {"label":"Review Packet","path":"project-management/issues/IW-206/review-packet-phase-01.md"} ... {"label":"Review Packet","path":"project-management/issues/IW-206/review-packet-phase-02.md"} ``` - Two `Phase 1 Context` entries share **both** `label` and `path` — a genuine duplicate, the thing this issue asks to remove. - Two `Review Packet` entries share `label` but point at **different files** — a legitimate collision, not a duplicate. Keying on `label` alone would collapse the second pair and destroy the phase-1 review packet's only reference. Nothing in the schema, the validator or `ReviewStateBuilder`/`ReviewStateUpdater` distinguishes the two shapes; both are valid by every check the tool owns. **9 of the 55 cards under `project-management/` hold a duplicate artifact `label`.** ## The question this leaves Should `--append-artifact` key on `path`, on `label`+`path`, or stay as it is? **Implementation note for whichever answer is chosen:** `mergeArray`'s `keyField` as added by IW-436 keys on a *single* string field, so a `label`+`path` key needs a small generalisation. --- Recorded during IW-436; see `project-management/issues/IW-436/LESSONS.md` entry 12.
Member

Seen again on 2026-09-16 while working IW-556: dx-create-tasks --force runs review-state update --append-artifact "Tasks:…/tasks.md=output" on a card that already holds that exact artifact, and the card ended with two identical "Tasks" entries. It was repaired by hand with --artifact (replace list). Any --force regeneration step hits this, not only multi-phase transitions.

Seen again on 2026-09-16 while working IW-556: `dx-create-tasks --force` runs `review-state update --append-artifact "Tasks:…/tasks.md=output"` on a card that already holds that exact artifact, and the card ended with two identical "Tasks" entries. It was repaired by hand with `--artifact` (replace list). Any `--force` regeneration step hits this, not only multi-phase transitions.
Member

IW-715 (PR #717) closes the artifacts part of this issue: --append-artifact is now keyed by path, so a second append of the same path replaces the entry in place. The key is in the shared merge, so the phase commands (phase-start, phase-pr, phase-merge, phase-advance, ci-gate) dedupe the same way.

IW-715 (PR #717) closes the artifacts part of this issue: `--append-artifact` is now keyed by `path`, so a second append of the same path replaces the entry in place. The key is in the shared `merge`, so the phase commands (`phase-start`, `phase-pr`, `phase-merge`, `phase-advance`, `ci-gate`) dedupe the same way.
Member

Intake: needs-input

1. Do we still want Part 2, a separate attention_artifacts list that the workflow sets?

Part 1 is done. 771e37ba (IW-715) makes --append-artifact replace an entry that has the same path. 995a11b0 (IW-436) does the same for actions on id and badges on label. Issue #351 (open) "incorporates #200". Its question 2, option a, puts the current-phase artifacts first, from the phase field that the card already holds, with no schema change.

  • a) Close #200 and leave relevance ordering to #351. There is no schema change and no plugin change. The order comes from the phase.
  • b) Do Part 2 here as the issue writes it. This adds a schema field, a --attention-artifact flag and a dashboard block. #351 must then use the field or ignore it.
  • c) Do Part 2, but only after #351 decides its ordering. This issue then waits on #351.

Recommended: a): #351 gets the same result from data that the card already holds, and it changes less.

2. If Part 2 goes ahead, does this issue change the iw-cli side only?

The issue asks for changes to the ag-implement and ag-implementation-workflow skills. These skills are not in this repository. They are in the dev-docs plugin. The dashboard shows one flat list now (WorktreeCardRenderer.scala#L934-L949).

  • a) iw-cli only: the schema, the flag and the dashboard. Open a separate dev-docs issue for the skills. Until the skills change, the dashboard shows no attention list.
  • b) Do both here. This issue then has work in a second repository.

Recommended: a): each change stays in the repository that holds its code.

Answer in a comment on this issue, per number, for example "1: a", or in your own words; intake reads it on its next iteration.

Intake: needs-input ### 1. Do we still want Part 2, a separate `attention_artifacts` list that the workflow sets? Part 1 is done. 771e37ba (IW-715) makes `--append-artifact` replace an entry that has the same `path`. 995a11b0 (IW-436) does the same for actions on `id` and badges on `label`. Issue #351 (open) "incorporates #200". Its question 2, option a, puts the current-phase artifacts first, from the `phase` field that the card already holds, with no schema change. - **a) Close #200 and leave relevance ordering to #351.** There is no schema change and no plugin change. The order comes from the phase. - **b) Do Part 2 here as the issue writes it.** This adds a schema field, a `--attention-artifact` flag and a dashboard block. #351 must then use the field or ignore it. - **c) Do Part 2, but only after #351 decides its ordering.** This issue then waits on #351. **Recommended: a)**: #351 gets the same result from data that the card already holds, and it changes less. ### 2. If Part 2 goes ahead, does this issue change the iw-cli side only? The issue asks for changes to the ag-implement and ag-implementation-workflow skills. These skills are not in this repository. They are in the dev-docs plugin. The dashboard shows one flat list now ([WorktreeCardRenderer.scala#L934-L949](https://code.iterative.works/iterative-works/iw-cli/src/branch/main/dashboard/jvm/src/presentation/views/WorktreeCardRenderer.scala#L934-L949)). - **a) iw-cli only: the schema, the flag and the dashboard. Open a separate dev-docs issue for the skills.** Until the skills change, the dashboard shows no attention list. - **b) Do both here.** This issue then has work in a second repository. **Recommended: a)**: each change stays in the repository that holds its code. Answer in a comment on this issue, per number, for example "1: a", or in your own words; intake reads it on its next iteration.
Sign in to join this conversation.
No project
No assignees
3 participants
Notifications
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#200
No description provided.