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

New issue

Closed

opened 2026-08-27 08:15:35 +00:00 by mph · 1 comment

mph commented

2026-08-27 08:15:35 +00:00

Owner

Found triaging IW-437.

ReviewStateValidator (core/model/ReviewStateValidator.scala:368-378) checks only that a phase checkpoint's context_sha is a string. Any string passes.

ReviewStateCliParser.parseCheckpoint (core/model/ReviewStateCliParser.scala:75-79) splits --checkpoint on the FIRST colon only and stores the remainder verbatim (core/model/ReviewStateUpdater.scala:198,207). So a wrong invocation such as

./iw review-state update --checkpoint "3:context_sha=0f64de40..."

writes context_sha = "context_sha=0f64de40...". The card validates. The corruption is visible only by reading the file back. Anything that later resolves a checkpoint to a commit (ag-revert, a dx-fix/wf-implement resume that reads phase_checkpoints) gets a reference git cannot resolve.

This happened twice in one issue (IW-437 phases 1 and 2) from an unchanged command snippet, and was caught both times only by reading the written card back.

PROPOSED FIX: reject a context_sha that is not 7-40 hexadecimal characters, so a wrong invocation fails loudly instead of writing a dead reference. Same "a typo must fail loudly" principle IW-423 raised for --needs-attention.

The snippet defect itself is filed against kanon as dev-docs#187.

Found triaging IW-437. ReviewStateValidator (core/model/ReviewStateValidator.scala:368-378) checks only that a phase checkpoint's context_sha is a string. Any string passes. ReviewStateCliParser.parseCheckpoint (core/model/ReviewStateCliParser.scala:75-79) splits --checkpoint on the FIRST colon only and stores the remainder verbatim (core/model/ReviewStateUpdater.scala:198,207). So a wrong invocation such as ./iw review-state update --checkpoint "3:context_sha=0f64de40..." writes context_sha = "context_sha=0f64de40...". The card validates. The corruption is visible only by reading the file back. Anything that later resolves a checkpoint to a commit (ag-revert, a dx-fix/wf-implement resume that reads phase_checkpoints) gets a reference git cannot resolve. This happened twice in one issue (IW-437 phases 1 and 2) from an unchanged command snippet, and was caught both times only by reading the written card back. PROPOSED FIX: reject a context_sha that is not 7-40 hexadecimal characters, so a wrong invocation fails loudly instead of writing a dead reference. Same "a typo must fail loudly" principle IW-423 raised for --needs-attention. The snippet defect itself is filed against kanon as dev-docs#187.

mph referenced this issue

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

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

mph commented

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

Author

Owner

Duplicate. #436 already covers this as its defect 3, and #436 places the fix better — in parseCheckpoint, where a bad value can be rejected at parse time, rather than in the validator after it is written.

#436 defect 4 also covers the second half of the same problem, which this issue did not mention: --checkpoint merges with ArrayMergeMode.Replace and therefore wipes every earlier phase entry. #436 proposes making append the default, with an explicit --clear-checkpoints for replace. That is the right shape.

Evidence to carry over from IW-437, which hit both defects at every one of its three phases:

The card's own history shows the loss: {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.
They were recovered by reading the card back out of git history: phase 1 (0f64de40) and phase 2 (b66e166e), each verified to resolve to its "Phase N: context ready" commit, then re-appended alongside phase 3 (ceb86105).
Nothing reported either defect. The write succeeds and the validator is satisfied — the map is well-formed, it is just short.

The caller-side snippet fix is tracked at dev-docs#187 (and note dev-docs#183).

Closing in favour of #436.

Duplicate. #436 already covers this as its defect 3, and #436 places the fix better — in `parseCheckpoint`, where a bad value can be rejected at parse time, rather than in the validator after it is written. #436 defect 4 also covers the second half of the same problem, which this issue did not mention: `--checkpoint` merges with `ArrayMergeMode.Replace` and therefore wipes every earlier phase entry. #436 proposes making append the default, with an explicit `--clear-checkpoints` for replace. That is the right shape. Evidence to carry over from IW-437, which hit both defects at every one of its three phases: - The card's own history shows the loss: `{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. - They were recovered by reading the card back out of git history: phase 1 (0f64de40) and phase 2 (b66e166e), each verified to resolve to its "Phase N: context ready" commit, then re-appended alongside phase 3 (ceb86105). - Nothing reported either defect. The write succeeds and the validator is satisfied — the map is well-formed, it is just short. The caller-side snippet fix is tracked at dev-docs#187 (and note dev-docs#183). Closing in favour of #436.

mph closed this issue

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

mph referenced this issue

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

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

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

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