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

New issue

Closed

opened 2026-08-25 11:58:25 +00:00 by mph · 2 comments

mph commented

2026-08-25 11:58:25 +00:00

Owner

iw review-state update --checkpoint "<phase>:<key>=<value>" stores the key inside the value: the <key>= prefix is not stripped.

Observed

./iw review-state update ... --checkpoint "4:context_sha=81a9203fd85e5545e7c71e5d125105aac3f36598"

writes

"phase_checkpoints": {
  "4": { "context_sha": "context_sha=81a9203fd85e5545e7c71e5d125105aac3f36598" }
}

The stored value is not a usable SHA. This is reproducible and long-standing — the same repository's phase 1 entry has the identical shape (phase_checkpoints.1.context_sha = "context_sha=9271863af..."), so it is not a recent regression.

ag-revert presumably tolerates or re-parses it, but anything reading the field as a bare SHA — a script, a dashboard link, a git invocation — gets a string that no git command accepts.

Expected

phase_checkpoints.4.context_sha == "81a9203fd85e5545e7c71e5d125105aac3f36598"

Found during MEDECA-430 (medeca-modul-poptavky); recorded in that issue's LESSONS.md entry 15 and promoted at triage.

`iw review-state update --checkpoint "<phase>:<key>=<value>"` stores the key **inside** the value: the `<key>=` prefix is not stripped. ## Observed ``` ./iw review-state update ... --checkpoint "4:context_sha=81a9203fd85e5545e7c71e5d125105aac3f36598" ``` writes ```json "phase_checkpoints": { "4": { "context_sha": "context_sha=81a9203fd85e5545e7c71e5d125105aac3f36598" } } ``` The stored value is not a usable SHA. This is reproducible and long-standing — the same repository's phase 1 entry has the identical shape (`phase_checkpoints.1.context_sha = "context_sha=9271863af..."`), so it is not a recent regression. `ag-revert` presumably tolerates or re-parses it, but anything reading the field as a bare SHA — a script, a dashboard link, a `git` invocation — gets a string that no `git` command accepts. ## Expected `phase_checkpoints.4.context_sha == "81a9203fd85e5545e7c71e5d125105aac3f36598"` --- Found during MEDECA-430 (medeca-modul-poptavky); recorded in that issue's `LESSONS.md` entry 15 and promoted at triage.

mph commented

2026-08-27 11:24:51 +00:00

Author

Owner

Re-examined while triaging IW-437, which hit the same corruption three times.

The premise here needs correcting. <phase>:<key>=<value> is not the intended syntax:

ReviewStateCliParser.parseCheckpoint (core/model/ReviewStateCliParser.scala:75-79) splits on the first colon and its own error message reads expected phase:sha.
The key is not caller-supplied. context_sha is applied by the updater (core/model/ReviewStateUpdater.scala:198,207).

So the correct invocation is --checkpoint "<phase>:<sha>", and stripping a context_sha= prefix would make the malformed caller silently work — entrenching the very snippet that produces these cards.

The real defects, split by cause:

The caller. The dx-fix Step 6a snippet says --checkpoint "${PHASE_NUM}:context_sha=${CONTEXT_SHA}". Filed against kanon as dev-docs#187, together with a second defect in the same five lines: --checkpoint merges with ArrayMergeMode.Replace and therefore destroys every earlier phase entry, where --append-checkpoint accumulates. IW-437 lost two checkpoints that way before anyone looked.
The silence. ReviewStateValidator (core/model/ReviewStateValidator.scala:368-378) checks only that context_sha is a string, so any value passes and the corruption is invisible until something tries to resolve it. Filed as #464.

Superseded by #464. Closing.

Re-examined while triaging IW-437, which hit the same corruption three times. The premise here needs correcting. `<phase>:<key>=<value>` is not the intended syntax: - `ReviewStateCliParser.parseCheckpoint` (core/model/ReviewStateCliParser.scala:75-79) splits on the first colon and its own error message reads `expected phase:sha`. - The key is not caller-supplied. `context_sha` is applied by the updater (core/model/ReviewStateUpdater.scala:198,207). So the correct invocation is `--checkpoint "<phase>:<sha>"`, and stripping a `context_sha=` prefix would make the malformed caller silently work — entrenching the very snippet that produces these cards. The real defects, split by cause: 1. **The caller.** The `dx-fix` Step 6a snippet says `--checkpoint "${PHASE_NUM}:context_sha=${CONTEXT_SHA}"`. Filed against kanon as dev-docs#187, together with a second defect in the same five lines: `--checkpoint` merges with `ArrayMergeMode.Replace` and therefore **destroys** every earlier phase entry, where `--append-checkpoint` accumulates. IW-437 lost two checkpoints that way before anyone looked. 2. **The silence.** `ReviewStateValidator` (core/model/ReviewStateValidator.scala:368-378) checks only that `context_sha` is a string, so any value passes and the corruption is invisible until something tries to resolve it. Filed as #464. Superseded by #464. Closing.

mph closed this issue

2026-08-27 11:24:53 +00:00

mph commented

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

Author

Owner

Correction to the previous comment: the superseding issue is #436, not #464.

#436 (filed from IW-423) already covers this as its defect 3, and covers the companion defect 4 — --checkpoint replacing the whole checkpoint map and wiping earlier phases. #464 was filed from IW-437 without checking the tracker first and has been closed as a duplicate of #436.

The substance of the earlier comment stands: <phase>:<key>=<value> is not the intended syntax. parseCheckpoint's own error reads expected phase:sha (core/model/ReviewStateCliParser.scala:75-79) and context_sha is applied by the updater (core/model/ReviewStateUpdater.scala:198,207), so stripping the prefix would make the malformed caller silently work.

Superseded by #436.

Correction to the previous comment: the superseding issue is **#436**, not #464. #436 (filed from IW-423) already covers this as its defect 3, and covers the companion defect 4 — `--checkpoint` replacing the whole checkpoint map and wiping earlier phases. #464 was filed from IW-437 without checking the tracker first and has been closed as a duplicate of #436. The substance of the earlier comment stands: `<phase>:<key>=<value>` is not the intended syntax. `parseCheckpoint`'s own error reads `expected phase:sha` (core/model/ReviewStateCliParser.scala:75-79) and `context_sha` is applied by the updater (core/model/ReviewStateUpdater.scala:198,207), so stripping the prefix would make the malformed caller silently work. Superseded by #436.

mph referenced this issue

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

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

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-09-01 15:32:10 +00:00

review-state: the phase key of a checkpoint is unconstrained, so two spellings make two entries #492