Squash the batch-implement blockers: nine fixes for the phase gate, the CI gate and the review-state card #501

Merged

mph merged 9 commits from batch-implement-blockers into main 2026-09-02 20:54:34 +00:00

Conversation 0 Commits 9 Files changed 32 +1023 -129

mph commented 2026-09-02 06:22:57 +00:00

Owner

Copy link

Bug-squashing batch over the batch-implement blockers found while auditing the last three weeks of issues. Nine issues, one commit each, TDD throughout, landed sequentially so every change was made against a final tree rather than in parallel.

Closes #467, #473, #472, #487, #474, #400, #465, #466, #337.

What changed, worst first

Issue	Commit	Behaviour now
#467	0441415	phase-pr asks the forge for the recorded pull request's head commit and refuses a url that is not on this branch. Previously a Duplicate create returned whatever url the issue-keyed card held — including an earlier, already-merged phase's — which the gate then waited on and --batch merged.
#472	e0c2d42	A re-triggered CI run supersedes the one it replaced. Supersession keys on (workflowLabel, event) and keeps the newest, after collapsing attempts per run id. Runs tied at the newest instant are all kept. ci_gated_sha is now cleared on every non-green exit and stamped with the commit actually gated.
#400	bd391d7	ci_pending/ci_fixing map to a new PhaseOutcome.AwaitCI instead of Recover, so waiting for CI no longer spends the --max-retries budget. Breaking — see below.
#473	a22104e	A ci_wait_started that is unreadable or ahead of the clock no longer disables the total timeout silently. It is clamped, the wait stays bounded, and the gate says so on stderr.
#474	dc707d3	Every field the schema declares format: date-time is parsed, not merely type-checked. This is what stops a bad value reaching startOfWait through an ordinary write.
#465	9c158d0	A blocked push carries the hook's report. git push puts only "failed to push some refs" on stderr; the hook writes its reasons to stdout, and both are now forwarded.
#466	138b099	./iw ./test lint runs the pre-push hook's exact scalafix command, and is part of the default run. Green now means pushable.
#487	f3cf1a4	review-state update can record phase, step and branch, with matching --clear-* flags.
#337	004ecd2	--items is repeatable and a comma is just a comma. A bullet containing a comma is no longer split into fragments.

Verification

• ./mill core.test — 186/186
• ./mill dashboard.test — 241/241
• scalafix --check --scala-version 3.3.7 --files core/ --files commands/ — clean
• ./iw ./test lint — passes, and its failure branch was proven by planting a var and observing the block

Every fix took a genuine red step first. The two places a red could not be taken cleanly are stated in the commit trail: #473's "unreadable value must warn" assertion could not exist until the return type carried a warning, and #337 needed PhaseArgs.repeatedArg to exist before the test file would compile (the production path was left on the old split for the red run, so the behaviour red is real).

Three things that must happen outside this branch

1. #400 breaks the kanon plugin. iw-plugin/commands/batch-implement.scala:348-366 matches all four PhaseOutcome cases with no wildcard. Nothing there sets -Xfatal-warnings, so it still compiles and then throws scala.MatchError: AwaitCI the first time a batch observes ci_pending — strictly worse than the "Exhausted N recovery attempts" message it replaces. It needs case PhaseOutcome.AwaitCI => <poll> plus a REQUIRES: iw-cli >= bump. This must not cross a release boundary unpatched. No shim was added, by decision.
2. #337 needs the skills regenerated. The kanon workflow skills and the published iw-cli-ops skill comma-join --items today. Until they are rebuilt from commands/phase-commit.scala (whose USAGE line this PR updates), agents keep emitting one long bullet.
3. #487 stays open on the workflow side. The CLI can record phase/step/branch; nothing here makes wf-implement pass or read them.

Review

/iterative-works:review-code across 8 skills, every finding then adversarially verified: 0 critical, 4 warnings, 18 suggestions. Nothing blocks merge. The four warnings:

• PhasePr.scala:177 (flagged independently by architecture, api, error-handling and testing) — the guard answers "is this PR on my branch?" by comparing head commits, because the port carries no head branch. Two consequences: on GitLab the MR sha is recomputed asynchronously after a push, so a legitimately-matching MR can be refused within that window; and the refusal message says "clear pr_url from the card", which deletes correct state if the cause was a transport failure. The refuse-don't-trust posture was a deliberate call; the destructive advice and the GitLab race were not part of it.
• CiGateCommand.scala:188 — clearing a stale ci_gated_sha on the hand-over path is untested. Proven by mutation: setting the flag to false on both sites leaves all 186 tests green.
• ScalafixGateTest.scala:31 — the default-run assertion can silently stop testing the default run.
• schemas/review-state.schema.json:24 — the phase description contradicts what the CLI writes (verbatim string, never a number).

Convergent across three skills and confirmed independently: PhaseArgs.repeatedArg inherits !value.startsWith("--") from its sibling in ReviewStateCliParser, so --items "--dev flag now required" is silently dropped — a narrower instance of the same silent-loss class #337 is about. Left as-is for consistency with the sibling parser; it wants its own issue.

One verifier died on an API error, so a single PhaseArgs.scala:16 finding went unverified and is not counted above. Three other findings on the same function survived and say the same thing.

Not in this batch

• #443 (Bloop timeout in CI) — deliberately skipped; the fix needs the runner's RAM or a measured decision about dropping the build server, and neither was available.
• #489 — diagnosed only, by decision. The diagnosis is posted on the issue: three distinct faults, of which the important one is that runStreaming returns a killed child's signal code as if it were the child's own answer, so a core.test that exceeds the 5-minute default is SIGTERMed by our own harness and reported as a test failure. That is the "PUSH BLOCKED over passing output" case, and it is not flakiness.
• #436, #463, #476 — left alone; they have live branches whose work a fresh implementation would discard.

🤖 Generated with Claude Code

https://claude.ai/code/session_013UpRzj7oiV1UDArn4hfu7U

Bug-squashing batch over the `batch-implement` blockers found while auditing the last three weeks of issues. Nine issues, one commit each, TDD throughout, landed sequentially so every change was made against a final tree rather than in parallel. Closes #467, #473, #472, #487, #474, #400, #465, #466, #337. ## What changed, worst first | Issue | Commit | Behaviour now | |---|---|---| | #467 | `0441415` | `phase-pr` asks the forge for the recorded pull request's head commit and refuses a url that is not on this branch. Previously a `Duplicate` create returned whatever url the issue-keyed card held — including an earlier, already-merged phase's — which the gate then waited on and `--batch` merged. | | #472 | `e0c2d42` | A re-triggered CI run supersedes the one it replaced. Supersession keys on `(workflowLabel, event)` and keeps the newest, after collapsing attempts per run id. Runs tied at the newest instant are all kept. `ci_gated_sha` is now cleared on every non-green exit and stamped with the commit actually gated. | | #400 | `bd391d7` | `ci_pending`/`ci_fixing` map to a new `PhaseOutcome.AwaitCI` instead of `Recover`, so waiting for CI no longer spends the `--max-retries` budget. **Breaking — see below.** | | #473 | `a22104e` | A `ci_wait_started` that is unreadable or ahead of the clock no longer disables the total timeout silently. It is clamped, the wait stays bounded, and the gate says so on stderr. | | #474 | `dc707d3` | Every field the schema declares `format: date-time` is parsed, not merely type-checked. This is what stops a bad value reaching `startOfWait` through an ordinary write. | | #465 | `9c158d0` | A blocked push carries the hook's report. `git push` puts only "failed to push some refs" on stderr; the hook writes its reasons to stdout, and both are now forwarded. | | #466 | `138b099` | `./iw ./test lint` runs the pre-push hook's exact scalafix command, and is part of the default run. Green now means pushable. | | #487 | `f3cf1a4` | `review-state update` can record `phase`, `step` and `branch`, with matching `--clear-*` flags. | | #337 | `004ecd2` | `--items` is repeatable and a comma is just a comma. A bullet containing a comma is no longer split into fragments. | ## Verification - `./mill core.test` — 186/186 - `./mill dashboard.test` — 241/241 - `scalafix --check --scala-version 3.3.7 --files core/ --files commands/` — clean - `./iw ./test lint` — passes, and its failure branch was proven by planting a `var` and observing the block Every fix took a genuine red step first. The two places a red could not be taken cleanly are stated in the commit trail: #473's "unreadable value must warn" assertion could not exist until the return type carried a warning, and #337 needed `PhaseArgs.repeatedArg` to exist before the test file would compile (the production path was left on the old split for the red run, so the behaviour red is real). ## Three things that must happen outside this branch 1. **#400 breaks the kanon plugin.** `iw-plugin/commands/batch-implement.scala:348-366` matches all four `PhaseOutcome` cases with no wildcard. Nothing there sets `-Xfatal-warnings`, so it still *compiles* and then throws `scala.MatchError: AwaitCI` the first time a batch observes `ci_pending` — strictly worse than the "Exhausted N recovery attempts" message it replaces. It needs `case PhaseOutcome.AwaitCI => <poll>` plus a `REQUIRES: iw-cli >=` bump. **This must not cross a release boundary unpatched.** No shim was added, by decision. 2. **#337 needs the skills regenerated.** The kanon workflow skills and the published `iw-cli-ops` skill comma-join `--items` today. Until they are rebuilt from `commands/phase-commit.scala` (whose USAGE line this PR updates), agents keep emitting one long bullet. 3. **#487 stays open on the workflow side.** The CLI can record `phase`/`step`/`branch`; nothing here makes `wf-implement` pass or read them. ## Review `/iterative-works:review-code` across 8 skills, every finding then adversarially verified: **0 critical, 4 warnings, 18 suggestions.** Nothing blocks merge. The four warnings: - **`PhasePr.scala:177`** (flagged independently by architecture, api, error-handling and testing) — the guard answers "is this PR on my branch?" by comparing head *commits*, because the port carries no head branch. Two consequences: on GitLab the MR `sha` is recomputed asynchronously after a push, so a legitimately-matching MR can be refused within that window; and the refusal message says "clear pr_url from the card", which *deletes correct state* if the cause was a transport failure. The refuse-don't-trust posture was a deliberate call; the destructive advice and the GitLab race were not part of it. - **`CiGateCommand.scala:188`** — clearing a stale `ci_gated_sha` on the hand-over path is untested. Proven by mutation: setting the flag to `false` on both sites leaves all 186 tests green. - **`ScalafixGateTest.scala:31`** — the default-run assertion can silently stop testing the default run. - **`schemas/review-state.schema.json:24`** — the `phase` description contradicts what the CLI writes (verbatim string, never a number). Convergent across three skills and confirmed independently: `PhaseArgs.repeatedArg` inherits `!value.startsWith("--")` from its sibling in `ReviewStateCliParser`, so `--items "--dev flag now required"` is **silently dropped** — a narrower instance of the same silent-loss class #337 is about. Left as-is for consistency with the sibling parser; it wants its own issue. One verifier died on an API error, so a single `PhaseArgs.scala:16` finding went unverified and is not counted above. Three other findings on the same function survived and say the same thing. ## Not in this batch - **#443** (Bloop timeout in CI) — deliberately skipped; the fix needs the runner's RAM or a measured decision about dropping the build server, and neither was available. - **#489** — diagnosed only, by decision. The diagnosis is posted on the issue: three distinct faults, of which the important one is that `runStreaming` returns a killed child's signal code as if it were the child's own answer, so a `core.test` that exceeds the 5-minute default is SIGTERMed by our own harness and reported as a test failure. That is the "PUSH BLOCKED over passing output" case, and it is not flakiness. - **#436, #463, #476** — left alone; they have live branches whose work a fresh implementation would discard. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_013UpRzj7oiV1UDArn4hfu7U

mph added 9 commits 2026-09-02 06:22:57 +00:00

fix(phase-pr): refuse a recorded pull request that is not on this branch ... 04414152bc

The review-state card is keyed on the issue, so its pr_url can still be an
earlier phase's pull request. When the forge refuses a duplicate, phase-pr now
asks the forge for the recorded pull request's head commit and accepts the url
only when it is the commit the branch was just pushed at; anything else stops
the command instead of gating and, under --batch, merging other work.

Refs: #467

fix(ci-gate): bound the wait when the card's start cannot be trusted ... a22104e396

A ci_wait_started ahead of the clock made every elapsed time negative, so
the total-budget check could never fire and the gate resumed for as long
as CI kept running - re-persisting the same future instant on every poll.
An unreadable value was swallowed and replaced by now with no signal.

startOfWait now returns the start together with the reason the card could
not be used for it: a value still to come, or one nothing can parse, falls
back to now so the budget stays bounded, and phase-pr, phase-merge and
ci-gate print the reason before they wait.

Refs: #473

fix(ci-gate): let a re-triggered run supersede the one it replaced ... e0c2d42bb8

Forgejo answers a re-trigger with a whole new run id, not a new attempt of
the old one, so keying supersession on the run id left both the failed run
and its green replacement on the commit. Red wins over green, so the gate
called the commit failed for as long as the card lived.

Supersession now keys on the workflow and the event and keeps the newest
run of each, after the highest attempt of each run id. Runs of different
workflows, or of different events, are separate answers and all of them
gate. Runs tied at the newest instant are all kept: equal timestamps say
nothing about which came second, and the gate must not drop a failure on
a coin flip.

Every non-green exit - red, blocked, timed out, no run - and the hand-over
of a branch with no CI now clear ci_gated_sha, so a card cannot carry a
commit's green proof into a state that contradicts it. phase-merge reads
that field to decide whether a second wait can be skipped.

Refs: #472

fix(review-state): let update record the workflow's phase, step and branch ... f3cf1a4379

`review-state update` accepted --phase, --step and --branch silently and
wrote nothing: the flags reached no UpdateInput field, merge emitted no such
key, and the schema contract refused all three, so even a card that already
carried them could not be updated at all.

The three flags now land on the card, each with a matching clear flag, and
the schema declares them as workflow-owned scalars the dashboard never reads.
A phase is stored verbatim as a string; the validator accepts an integer too,
so a card written by hand with "phase": 3 still validates.

Refs: #487

fix(review-state): reject a timestamp nothing can read ... dc707d3569

The validator checked only that last_updated and ci_wait_started were
strings, so a card holding "not-a-date" passed as valid and every reader
of those fields then had to cope with a value it could not parse.

Both fields are now validated as a whole against java.time.Instant.parse,
the same grammar the readers accept, so the validator's yes is exactly the
reader's yes. A value that does not parse is an error naming the field. A
non-string value still reports the type error, once. Z-suffixed instants,
numeric offsets and nanosecond fractions all remain valid.

Refs: #474

fix(batch): give a CI wait its own phase outcome ... bd391d7172

decideOutcome maps ci_pending and ci_fixing to a distinct PhaseOutcome.AwaitCI
instead of Recover. Recover re-dispatches the implementer and costs one
--max-retries unit per observation, so with the default budget of 1 a phase that
sat in ci_pending twice ended the batch with 'Exhausted 1 recovery attempts'
while CI was progressing normally. AwaitCI says there is nothing to re-dispatch:
the orchestrator polls until CI resolves, bounded by the phase timeout rather
than the retry budget. It is a real enum case, so neither status falls through to
the unknown-status arm.

Refs: #400

fix(git): carry a blocked push's hook report in the failure ... 9c158d04be

A pre-push hook writes its report to its own stdout, and git keeps that on
git's stdout. git's stderr says only "error: failed to push some refs to
...". GitAdapter.push built the failure message from stderr alone, so
every file, line and column the hook named was thrown away and the caller
saw nothing but that one uninformative line.

The failure now carries both captured streams, stderr first, joined by a
newline; an exit code with neither stream is named instead. The "Failed to
push branch '<branch>': " prefix is unchanged, so the three call sites
that print the message whole (phase-pr's create, phase-pr's review-state
persist, phase-start) gain the hook's reasons on the lines that follow.

Refs: #465

test(lint): run the push gate's scalafix check from iw test ... 138b09957e

`./iw ./test` ran four gates and none of them was scalafix, while
`.git-hooks/pre-push` and the CI lint job both run
`scalafix --check --scala-version 3.3.7 --files core/ --files commands/`.
The pre-PR run could therefore be fully green on sources git push rejects -
`--files core/` is recursive, so a `var` in core/test/ compiles under Mill
and fails DisableSyntax at push time.

ScalafixGate holds that argv once in the functional core, and a new
`iw test lint` subcommand runs it from the project root, where scalafix
finds .scalafix.conf and the relative --files paths resolve. The gate is
part of the default all-run, first and without short-circuiting, so every
other gate still reports. A missing scalafix binary fails the check with an
install hint rather than skipping, since a skip restores the same false
green. ScalafixGateTest pins the command text against all three copies.

Refs: #466

fix(phase-commit): give each --items occurrence its own bullet ... 004ecd2f61

A commit bullet is prose, and prose carries commas. phase-commit read a
single --items value and split it on every comma, so one bullet became
several with exit code 0 and no warning; repeating the flag did not help
either, because PhaseArgs.namedArg keeps only the first occurrence.

--items is now repeatable: each occurrence contributes exactly one bullet
and a comma inside a value stays a comma. PhaseArgs.repeatedArg collects
the value after every occurrence, mirroring
ReviewStateCliParser.extractRepeatedFlag so the two CLIs read repeated
flags alike. That guard also means a bullet whose text starts with '--'
is dropped rather than swallowing the following flag - a known trade-off
taken for consistency with the sibling parser.

The usage line reads [--items ITEM]... in both the command script and the
missing-argument error.

Refs: #337

mph merged commit d1eba9c408 into main 2026-09-02 20:54:34 +00:00

mph referenced this pull request from a commit 2026-09-02 20:54:36 +00:00

Merge pull request 'Squash the batch-implement blockers: nine fixes for the phase gate, the CI gate and the review-state card' (#501) from batch-implement-blockers into main

mph referenced this pull request 2026-09-02 20:55:14 +00:00

A corrupt or future ci_wait_started silently disables the CI wait budget #473

mph referenced this pull request 2026-09-02 20:55:14 +00:00

iw ci-gate reads a commit's whole status history, so a re-run can never go green #472

mph referenced this pull request 2026-09-02 20:55:14 +00:00

ReviewStateValidator does not enforce the schema's date-time format on any timestamp field #474

mph referenced this pull request 2026-09-02 20:55:14 +00:00

A failed push reports only 'failed to push some refs'; the hook's stderr is dropped #465

mph referenced this pull request 2026-09-02 20:55:14 +00:00

./iw ./test is green on code the pre-push hook rejects #466

mph referenced this pull request 2026-09-02 20:55:14 +00:00

phase-commit: --items splits commas inside bullet strings, mangling commit body #337

mph referenced this pull request 2026-09-02 20:55:30 +00:00

batch-implement: ci_pending/ci_fixing should poll for CI, not consume the retry budget #400

mph referenced this pull request 2026-09-02 20:55:30 +00:00

review-state update cannot write phase or step, so wf-implement's fast-path resume never fires #487

mph referenced this pull request 2026-09-02 21:08:30 +00:00

Regenerate the core module index and document the modules the gate work touched #503

claude referenced this pull request 2026-10-15 22:46:05 +00:00

review-state update cannot write phase or step, so wf-implement's fast-path resume never fires #487

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

agent-ready

  

bug

Something isn't working

  

contract

Run the tool-contract suite on this PR

  

documentation

Improvements or additions to documentation

  

duplicate

This issue or pull request already exists

  

enhancement

New feature or request

  

good first issue

Good for newcomers

  

help wanted

Extra attention is needed

  

invalid

This doesn't seem right

  

needs-human

  

needs-input

Intake asked the human a question on the issue; waiting for the answer

  

question

Further information is requested

  

wontfix

This will not be worked on

No labels

agent-ready

bug

contract

documentation

duplicate

enhancement

good first issue

help wanted

invalid

needs-human

needs-input

question

wontfix

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

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!501

No description provided.