Six phase commands leave git_sha stale on every review-state card they write #493

New issue

Closed

opened 2026-09-01 15:32:18 +00:00 by mph · 1 comment

mph commented

2026-09-01 15:32:18 +00:00

Owner

Found triaging IW-436 (LESSONS.md entry 3). Accepted as a residual by IW-436 RESOLVED 3.

IW-436 fixed the stale git_sha in review-state update only. The same defect stays live at six other entry points, because none of them passes a gitSha to ReviewStateUpdater.UpdateInput:

PhaseStart.scala:116
PhaseAdvance.scala:214
PhasePr.scala:275,327,407,427
PhaseMerge.scala:268,325,379
CiGate.scala:243,266,401
CiGateCommand.scala:172,195,224

Why it is not a one-line fix

They write through the ReviewStateOps port (env.reviewState.update, some via ReviewStateWrite.tryUpdate at core/commands/ReviewStateWrite.scala:21), not through ReviewStateAdapter directly. A refresh placed in the adapter alone would be invisible to every harness test, and FakeReviewStateOps (core/test/fixtures/FakeCommandEnv.scala:754) would silently diverge from live behaviour.

Two options:

Change the port contract and the fake together.
Have each of the six commands fill gitSha from env.git before it calls the port.

The second is the smaller change and is testable through the existing harnesses.

Consequence while it stays unfixed

git_sha on any card touched by a phase command names an older commit than the card describes, so the wf-implement fast-path resume check ("state SHA is HEAD or an ancestor") passes vacuously there too.

Found triaging IW-436 (LESSONS.md entry 3). Accepted as a residual by IW-436 RESOLVED 3. IW-436 fixed the stale `git_sha` in `review-state update` only. The same defect stays live at six other entry points, because none of them passes a `gitSha` to `ReviewStateUpdater.UpdateInput`: - `PhaseStart.scala:116` - `PhaseAdvance.scala:214` - `PhasePr.scala:275,327,407,427` - `PhaseMerge.scala:268,325,379` - `CiGate.scala:243,266,401` - `CiGateCommand.scala:172,195,224` ## Why it is not a one-line fix They write through the `ReviewStateOps` port (`env.reviewState.update`, some via `ReviewStateWrite.tryUpdate` at `core/commands/ReviewStateWrite.scala:21`), not through `ReviewStateAdapter` directly. A refresh placed in the adapter alone would be invisible to every harness test, and `FakeReviewStateOps` (`core/test/fixtures/FakeCommandEnv.scala:754`) would silently diverge from live behaviour. Two options: 1. Change the port contract and the fake together. 2. Have each of the six commands fill `gitSha` from `env.git` before it calls the port. The second is the smaller change and is testable through the existing harnesses. ## Consequence while it stays unfixed `git_sha` on any card touched by a phase command names an older commit than the card describes, so the `wf-implement` fast-path resume check ("state SHA is HEAD or an ancestor") passes vacuously there too.

mph referenced this issue from a commit

2026-09-01 15:36:36 +00:00

workflow: Triage lessons for IW-436

mph referenced this issue

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

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

claude commented

2026-10-15 22:32:29 +00:00

Member

Intake: fixed by 40b3921996

Commit 40b39219 ("fix(review-state): record the commit a card was written at on every path") is on main. It puts the git_sha refresh in ReviewStateWrite.atHead (core/commands/ReviewStateWrite.scala): every update carries HEAD unless the caller names a commit. ReviewStateWrite.write is now the only production call to env.reviewState.update, and PhaseStart, PhaseAdvance, PhasePr, PhaseMerge, CiGate and CiGateCommand all write through it. This is option 2 of this issue: the commands fill gitSha before the port, so FakeReviewStateOps sees the same input. core/test/ReviewStateWriteTest.scala covers it.

Reopen this issue if a phase command still writes a stale git_sha.

Intake: fixed by 40b3921996c49495270100511ce085b7409a427d Commit 40b39219 ("fix(review-state): record the commit a card was written at on every path") is on `main`. It puts the `git_sha` refresh in `ReviewStateWrite.atHead` (`core/commands/ReviewStateWrite.scala`): every update carries HEAD unless the caller names a commit. `ReviewStateWrite.write` is now the only production call to `env.reviewState.update`, and PhaseStart, PhaseAdvance, PhasePr, PhaseMerge, CiGate and CiGateCommand all write through it. This is option 2 of this issue: the commands fill `gitSha` before the port, so `FakeReviewStateOps` sees the same input. `core/test/ReviewStateWriteTest.scala` covers it. Reopen this issue if a phase command still writes a stale `git_sha`.

claude closed this issue

2026-10-15 22:32:30 +00:00