iterative-works/iw-cli

Watch

Fork 0

Record the card's position on write, tell a killed process from a failing one, and identify a phase's pull request by its branch #506

Merged

mph merged 3 commits from phase-gate-followups into main

2026-09-03 05:55:29 +00:00

mph commented

2026-09-03 05:43:45 +00:00

Owner

Three defects a consumer found in 0.11.0, and the two review warnings that
shipped with it.

`review-state write` records phase, step and branch (#505)

#487 put --phase, --step and --branch on update only. Every
workflow's first transition creates the card with write, so a card born at
analysis_ready carried none of the three and the resume fast path could not
pass its sanity check at the one transition every workflow starts from.

write now takes the same three flags with the same semantics. The value is
stored verbatim as a JSON string — --phase 3 writes "3" — which is what
update already did and is now stated in the write documentation as well.
A consumer comparing state.phase against a phase number must compare strings.

A killed child is distinguishable from a failing one (#468, fault (c))

ProcessAdapter.run computed timedOut by comparing the exit code against
143 alone. os-lib sends SIGTERM and escalates to SIGKILL when the child does
not go, so a child killed the hard way exits 137 and run — the function that
was supposed to get this right — answered timedOut = false.

runStreaming returned the signal code with nothing to read it by, and the
constant was private. A consumer that wanted to tell "our guard fired" from
"the command said no" had to hardcode the two numbers; the kanon plugin did,
and until it did, a CI wait that outlived its guard was reported to the user
as a failing build.

killedBySignal names both codes and is public.
run uses it.
runStreaming keeps returning the code — that is what a caller already
matches on — and now says on stderr what it killed and how long it waited.
The unit-test harness gives each Mill suite the ten minutes the E2E and
contract suites already get, names the suite that did not pass, and reports
a killed suite as killed rather than failed.
The pre-push hook runs its two steps separately, so the banner can name the
step that blocked the push instead of leaving the reader with the tail of
whichever suite ran last.

phase-pr identifies a pull request by its branch (#467 review follow-ups)

The guard added in 0.11.0 asked whether the pull request the card records
points at the commit just pushed. That is the wrong question twice over.

A forge publishes a merge request's head commit after it takes the push, so
the right pull request could report the commit before it and be refused — a
false refusal that stops --batch and needs a human. And the refusal told the
reader to clear pr_url, destroying a good url to work around a lag.

The branch a pull request is open on says which work it is for, and no lag
moves it. fetchPrHeadBranch reads it from Forgejo, GitHub and GitLab, the
guard compares it against the branch this phase pushed, and the refusal names
both branches and asks for the right url instead of the loss of the recorded
one. A forge that will not answer is still a refusal: guessing is the failure
this guard exists to stop.

API surface

TrackerOps gains fetchPrHeadBranch — a break for anything that implements
the trait, and only for that. ProcessAdapter.SigtermExitCode changes from
private to public and gains SigkillExitCode and killedBySignal; nothing
that compiled before stops compiling.

Not in this change

Faults (a) and (b) of #468 — the test-server port hijack and the BATS
isolation leaks — are a separate body of work and stay on that issue. One new
sighting of the flake class was recorded during this work.

Closes #505

🤖 Generated with Claude Code

https://claude.ai/code/session_013UpRzj7oiV1UDArn4hfu7U

Three defects a consumer found in 0.11.0, and the two review warnings that shipped with it. ## `review-state write` records phase, step and branch (#505) `#487` put `--phase`, `--step` and `--branch` on `update` only. Every workflow's *first* transition creates the card with `write`, so a card born at `analysis_ready` carried none of the three and the resume fast path could not pass its sanity check at the one transition every workflow starts from. `write` now takes the same three flags with the same semantics. The value is stored verbatim as a JSON string — `--phase 3` writes `"3"` — which is what `update` already did and is now stated in the write documentation as well. A consumer comparing `state.phase` against a phase number must compare strings. ## A killed child is distinguishable from a failing one (#468, fault (c)) `ProcessAdapter.run` computed `timedOut` by comparing the exit code against 143 alone. os-lib sends SIGTERM and escalates to SIGKILL when the child does not go, so a child killed the hard way exits 137 and `run` — the function that was supposed to get this right — answered `timedOut = false`. `runStreaming` returned the signal code with nothing to read it by, and the constant was `private`. A consumer that wanted to tell "our guard fired" from "the command said no" had to hardcode the two numbers; the kanon plugin did, and until it did, a CI wait that outlived its guard was reported to the user as a failing build. - `killedBySignal` names both codes and is public. - `run` uses it. - `runStreaming` keeps returning the code — that is what a caller already matches on — and now says on stderr what it killed and how long it waited. - The unit-test harness gives each Mill suite the ten minutes the E2E and contract suites already get, names the suite that did not pass, and reports a killed suite as killed rather than failed. - The pre-push hook runs its two steps separately, so the banner can name the step that blocked the push instead of leaving the reader with the tail of whichever suite ran last. ## phase-pr identifies a pull request by its branch (#467 review follow-ups) The guard added in 0.11.0 asked whether the pull request the card records points at the commit just pushed. That is the wrong question twice over. A forge publishes a merge request's head commit *after* it takes the push, so the right pull request could report the commit before it and be refused — a false refusal that stops `--batch` and needs a human. And the refusal told the reader to clear `pr_url`, destroying a good url to work around a lag. The branch a pull request is open on says which work it is for, and no lag moves it. `fetchPrHeadBranch` reads it from Forgejo, GitHub and GitLab, the guard compares it against the branch this phase pushed, and the refusal names both branches and asks for the right url instead of the loss of the recorded one. A forge that will not answer is still a refusal: guessing is the failure this guard exists to stop. ## API surface `TrackerOps` gains `fetchPrHeadBranch` — a break for anything that implements the trait, and only for that. `ProcessAdapter.SigtermExitCode` changes from private to public and gains `SigkillExitCode` and `killedBySignal`; nothing that compiled before stops compiling. ## Not in this change Faults (a) and (b) of #468 — the test-server port hijack and the BATS isolation leaks — are a separate body of work and stay on that issue. One new sighting of the flake class was recorded during this work. Closes #505 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_013UpRzj7oiV1UDArn4hfu7U

mph added 3 commits

2026-09-03 05:43:45 +00:00

feat(review-state): record phase, step and branch on write 1a91ae5c15

Every workflow's first transition creates the card with `write`, not
`update`, so a card born at `analysis_ready` carried no position and the
resume fast path could not pass its sanity check there.

`write` now takes the same `--phase`, `--step` and `--branch` that
`update` takes, with the same verbatim string storage.

Closes #505

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013UpRzj7oiV1UDArn4hfu7U

fix(process): make a killed child distinguishable from a failing one 8b689768e3

A command the timeout killed reports 143, or 137 when it ignored SIGTERM
and had to be forced. `run` compared against 143 alone, so it answered
`timedOut = false` for a child it killed the hard way, and `runStreaming`
returned the signal code with nothing to read it by — a consumer had no
constant to compare against and reported a healthy build as a failure.

`killedBySignal` names both codes and is public, `run` uses it, and
`runStreaming` says on stderr what it killed and how long it waited.

The unit-test harness gives each Mill suite the same ten minutes the E2E
and contract suites already get, names the suite that did not pass, and
says when a suite was killed rather than calling it failed. The pre-push
hook runs its two steps separately so the banner can name the one that
blocked the push.

Refs #468

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013UpRzj7oiV1UDArn4hfu7U

fix(phase-pr): identify a recorded pull request by its branch, not its head

CI / contract (pull_request) Has been skipped

Details

CI / format (pull_request) Successful in 7s

Details

CI / lint (pull_request) Successful in 8s

Details

CI / compile (pull_request) Successful in 1m28s

Details

CI / dashboard-build (pull_request) Successful in 1m3s

Details

CI / test (pull_request) Successful in 10m9s

Details

cfe9876317

The guard on the duplicate path asked whether the pull request the card
records points at the commit just pushed. Two things follow from that
question being the wrong one.

A forge publishes a merge request's head commit after it takes the push,
so a request that is the right one could report the commit before it and
be refused. And the refusal told the reader to clear pr_url — destroying
a good url to work around a lag.

The branch a pull request is open on is what says which work it is for,
and no lag moves it. `fetchPrHeadBranch` reads it from all three forges,
the guard compares it against the branch this phase pushed, and the
refusal names both branches and asks for the right url rather than for
the loss of the recorded one.

Refs #467

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013UpRzj7oiV1UDArn4hfu7U

mph referenced this pull request

2026-09-03 05:44:19 +00:00

phase-pr can ask the forge which pull request is on this branch instead of trusting the card #507

mph merged commit f5545c205b into main

2026-09-03 05:55:29 +00:00

mph referenced this pull request from a commit

2026-09-03 05:55:30 +00:00

Merge pull request 'Record the card's position on write, tell a killed process from a failing one, and identify a phase's pull request by its branch' (#506) from phase-gate-followups into main

mph referenced this pull request

2026-09-03 05:55:44 +00:00

tmux-dependent BATS tests fail non-deterministically under a full ./iw ./test run #468

mph referenced this pull request