Make the forge the authority on a branch's pull request, and stop the test suites lying about themselves #511

Merged

mph merged 8 commits from forge-truth-and-test-isolation into main 2026-09-03 09:21:31 +00:00

Conversation 0 Commits 8 Files changed 34 +1441 -653

mph commented 2026-09-03 07:22:41 +00:00

Owner

Copy link

Four defects: one that let --batch stop for a human it did not need, and three
that made the test suites lie about themselves.

phase-pr asks the forge which pull request is on this branch (#507)

A create that comes back Duplicate means the forge already holds a pull
request for the branch. Reading the url off the review-state card to find out
which one made a local record the input to a merge decision. The card is keyed
on the issue, so it can name an earlier phase's pull request; a repository that
keeps no card could not recover from a duplicate at all; and the guard that
0.12.0 added for the first case turned a stale card into a refusal that needed a
person.

TrackerOps.fetchPullRequestForBranch asks the forge instead. It reports the
state rather than filtering on it — merged, closed and open are three different
answers to a caller, and a lookup that returned only open ones would be silent
exactly when a caller asks what became of the work. Forgejo already had this
lookup and it was promoted rather than rewritten; GitHub and GitLab gained one.
A state a forge is not known to send is a Left, not a guess: this reading
decides whether work gets merged.

Only an open request can carry the push. A merged or closed one is refused, and
so is a forge that will not answer. Every refusal names the branch, what the
forge said, and that nothing was gated, so a caller can tell it from a CI
failure by the message alone. It stays exit 1 — the 0/1/3 contract is documented
across six generated skills and does not gain a fourth code for this.

fetchPrHeadBranch shipped one release ago to check the card's url against this
branch. Nothing consults the card here now, and all three lookups filter on the
branch at the source, so the check and the port method it needed are gone.

A slept fake clock passes time (#508)

A wait reads its budget off the clock and sleeps between polls. The fake
recorded the sleep and left the clock where it was, so a harness test that
scripted a state the wait could not leave spun in the poll loop instead of
failing — no assertion, no output, twenty minutes of a test JVM at 190% CPU
before it was killed by hand. Sleeping now moves the clock. Scripted readings
still win over it, so tests that drive a wait by hand are unchanged, and the
whole suite passed the change without an edit.

A test server proves it owns its port (#468, fault (a))

A port is chosen by asking the OS for a free one and letting it go again, so two
JVMs can be handed the same number. The loser's bind failed on a bare thread
where nothing caught it, its readiness probe was answered 200 by the winner, and
it spent the rest of the test writing into another test's server and reading
that server's state back as its own. Nothing in that produced an error — only a
worktree count that made no sense.

/health now reports which server answered. Both harnesses generate an identity
per server, accept readiness only on their own, and move to a new port rather
than losing to whoever holds this one. A startup failure that is not a lost race
is reported with its own stack trace instead of being retried four more times.
Two tests picked their port by guessing a number in a range with no availability
check at all; the integration suite's two hand-rolled start loops are one
recursive helper.

The E2E suites stop deleting each other's worktrees (#468, fault (b))

Six suites created worktrees as siblings of a directory under the shared temp
directory and cleaned up by globbing that shared directory for anything named
after the project. Two runs on one machine deleted each other's worktrees
mid-test. Each suite now gives its project a parent of its own; every assertion
is relative to the project directory and is unaffected.

The E2E runner also pointed IW_CORE_JAR at a release path a development
checkout never has. iw-run discards an override it cannot read, so the override
did not save a resolution — it destroyed the good value iw-run had already
exported, and each of ~190 tests asked the build tool for the jar again. It now
names the jar the bootstrap actually left, and refuses the run if there is none.

API surface

TrackerOps gains fetchPullRequestForBranch and loses fetchPrHeadBranch; the
three clients lose their head-branch readers. Breaking only for something that
implements the trait or called those members. PRState gains fromGitHub and
fromGitLab. CaskServer gains an instanceId with a generated default, and
/health gains that field.

What the review changed

Eight review skills ran over this range. Three findings were real defects, not
polish.

The Forgejo lookup was a window, not an answer. No endpoint filters a
pull-request listing by head branch, so it scanned the 50 most recently updated
requests and filtered client-side. Right(None) meant "not among those", and
phase-pr turned that into a refusal — on the forge this project actually uses.
The base branch is now an input and every forge narrows on it. Verified against
live data on all three: a merged request with a deleted head resolves, and a
pair with none answers cleanly.

A branch name reached the GitLab query string unescaped. Git allows & and
# in a ref, so a branch could add a parameter to the request or truncate it.

A merged request whose branch was deleted was invisible to the branch-only
listing. Forgejo rewrites head.ref to refs/pull/<n>/head once the branch
is gone, and this tool's own merge deletes the branch by default — so the
dashboard read "no pull request" for every merged phase. head.label keeps the
branch it was opened from, and the listing matches either now.

The rest: runStreaming no longer prints its own diagnostic — it printed a
caller's whole argv where a credential could sit, on a channel Console owns,
duplicating what its one attentive caller already reported; killedBySignal is
the channel, and the tmux and herdr adapters now say a kill was a kill. One
selection rule instead of one per forge. ForgeConfig.credentials instead of
the same two lines in six places. CoreJar beside DashboardJar over one
InstalledJar search, so the path convention has one home rather than three.
The E2E runner tells "could not start" from "ran and failed", and a suite killed
before its limit is no longer reported as having reached it. Both test-server
helpers are provably tail-recursive. FakeClock refuses to be slept a thousand
times, so a loop that cannot end fails instead of hanging.

CI

Two runs of this branch went red with the tree green locally. Neither was the
code: every job began with actions/checkout@v4, which the runner fetches from
github.com without credentials, and GitHub rate-limited it. The job died before
running a step, so a run read as a random subset of the suite failing. Both
actions now come from code.forgejo.org, which the workflow already used for
upload-artifact, and CONTRIBUTING.md records the rule.

A third run hit an unrelated intermittent crash in the formatter itself, filed
as #512; it did not reproduce.

Verification

Full suite green locally: unit, lint, command compile, 192 E2E, dashboard unit
and integration. All six CI jobs green. The consumer compiled all six of its
commands and 23 hook tests against this branch, tested the deleted-head case
that its own path depends on, and confirmed the removals reach nothing of
theirs.

Closes #507
Closes #508

🤖 Generated with Claude Code

https://claude.ai/code/session_013UpRzj7oiV1UDArn4hfu7U

Four defects: one that let `--batch` stop for a human it did not need, and three that made the test suites lie about themselves. ## phase-pr asks the forge which pull request is on this branch (#507) A create that comes back `Duplicate` means the forge already holds a pull request for the branch. Reading the url off the review-state card to find out *which* one made a local record the input to a merge decision. The card is keyed on the issue, so it can name an earlier phase's pull request; a repository that keeps no card could not recover from a duplicate at all; and the guard that 0.12.0 added for the first case turned a stale card into a refusal that needed a person. `TrackerOps.fetchPullRequestForBranch` asks the forge instead. It reports the state rather than filtering on it — merged, closed and open are three different answers to a caller, and a lookup that returned only open ones would be silent exactly when a caller asks what became of the work. Forgejo already had this lookup and it was promoted rather than rewritten; GitHub and GitLab gained one. A state a forge is not known to send is a `Left`, not a guess: this reading decides whether work gets merged. Only an open request can carry the push. A merged or closed one is refused, and so is a forge that will not answer. Every refusal names the branch, what the forge said, and that nothing was gated, so a caller can tell it from a CI failure by the message alone. It stays exit 1 — the 0/1/3 contract is documented across six generated skills and does not gain a fourth code for this. `fetchPrHeadBranch` shipped one release ago to check the card's url against this branch. Nothing consults the card here now, and all three lookups filter on the branch at the source, so the check and the port method it needed are gone. ## A slept fake clock passes time (#508) A wait reads its budget off the clock and sleeps between polls. The fake recorded the sleep and left the clock where it was, so a harness test that scripted a state the wait could not leave spun in the poll loop instead of failing — no assertion, no output, twenty minutes of a test JVM at 190% CPU before it was killed by hand. Sleeping now moves the clock. Scripted readings still win over it, so tests that drive a wait by hand are unchanged, and the whole suite passed the change without an edit. ## A test server proves it owns its port (#468, fault (a)) A port is chosen by asking the OS for a free one and letting it go again, so two JVMs can be handed the same number. The loser's bind failed on a bare thread where nothing caught it, its readiness probe was answered 200 by the winner, and it spent the rest of the test writing into another test's server and reading that server's state back as its own. Nothing in that produced an error — only a worktree count that made no sense. `/health` now reports which server answered. Both harnesses generate an identity per server, accept readiness only on their own, and move to a new port rather than losing to whoever holds this one. A startup failure that is not a lost race is reported with its own stack trace instead of being retried four more times. Two tests picked their port by guessing a number in a range with no availability check at all; the integration suite's two hand-rolled start loops are one recursive helper. ## The E2E suites stop deleting each other's worktrees (#468, fault (b)) Six suites created worktrees as siblings of a directory under the shared temp directory and cleaned up by globbing that shared directory for anything named after the project. Two runs on one machine deleted each other's worktrees mid-test. Each suite now gives its project a parent of its own; every assertion is relative to the project directory and is unaffected. The E2E runner also pointed `IW_CORE_JAR` at a release path a development checkout never has. `iw-run` discards an override it cannot read, so the override did not save a resolution — it destroyed the good value `iw-run` had already exported, and each of ~190 tests asked the build tool for the jar again. It now names the jar the bootstrap actually left, and refuses the run if there is none. ## API surface `TrackerOps` gains `fetchPullRequestForBranch` and loses `fetchPrHeadBranch`; the three clients lose their head-branch readers. Breaking only for something that implements the trait or called those members. `PRState` gains `fromGitHub` and `fromGitLab`. `CaskServer` gains an `instanceId` with a generated default, and `/health` gains that field. ## What the review changed Eight review skills ran over this range. Three findings were real defects, not polish. **The Forgejo lookup was a window, not an answer.** No endpoint filters a pull-request listing by head branch, so it scanned the 50 most recently updated requests and filtered client-side. `Right(None)` meant "not among those", and `phase-pr` turned that into a refusal — on the forge this project actually uses. The base branch is now an input and every forge narrows on it. Verified against live data on all three: a merged request with a deleted head resolves, and a pair with none answers cleanly. **A branch name reached the GitLab query string unescaped.** Git allows `&` and `#` in a ref, so a branch could add a parameter to the request or truncate it. **A merged request whose branch was deleted was invisible to the branch-only listing.** Forgejo rewrites `head.ref` to `refs/pull/<n>/head` once the branch is gone, and this tool's own merge deletes the branch by default — so the dashboard read "no pull request" for every merged phase. `head.label` keeps the branch it was opened from, and the listing matches either now. The rest: `runStreaming` no longer prints its own diagnostic — it printed a caller's whole argv where a credential could sit, on a channel `Console` owns, duplicating what its one attentive caller already reported; `killedBySignal` is the channel, and the tmux and herdr adapters now say a kill was a kill. One selection rule instead of one per forge. `ForgeConfig.credentials` instead of the same two lines in six places. `CoreJar` beside `DashboardJar` over one `InstalledJar` search, so the path convention has one home rather than three. The E2E runner tells "could not start" from "ran and failed", and a suite killed before its limit is no longer reported as having reached it. Both test-server helpers are provably tail-recursive. `FakeClock` refuses to be slept a thousand times, so a loop that cannot end fails instead of hanging. ## CI Two runs of this branch went red with the tree green locally. Neither was the code: every job began with `actions/checkout@v4`, which the runner fetches from github.com without credentials, and GitHub rate-limited it. The job died before running a step, so a run read as a random subset of the suite failing. Both actions now come from `code.forgejo.org`, which the workflow already used for upload-artifact, and `CONTRIBUTING.md` records the rule. A third run hit an unrelated intermittent crash in the formatter itself, filed as #512; it did not reproduce. ## Verification Full suite green locally: unit, lint, command compile, 192 E2E, dashboard unit and integration. All six CI jobs green. The consumer compiled all six of its commands and 23 hook tests against this branch, tested the deleted-head case that its own path depends on, and confirmed the removals reach nothing of theirs. Closes #507 Closes #508 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_013UpRzj7oiV1UDArn4hfu7U

mph added 4 commits 2026-09-03 07:22:41 +00:00

fix(test): let a slept fake clock pass time, so a harness wait can end ... 835b6cdf39

A wait reads its budget off the clock and sleeps between polls. The fake
recorded the sleep and left the clock where it was, so a harness test that
scripted a state the wait could not leave spun in the poll loop instead of
failing — heartbeating the card, indefinitely, with no assertion and no
output to say what was wrong.

Sleeping now moves the clock by what was slept. Scripted readings still win
over it, so tests that drive a wait by hand are unaffected.

Closes #508

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

fix(test): make a test server prove it owns its port before a test uses it ... 07d35a9f92

A port is chosen by asking the OS for a free one and letting it go again,
so two JVMs can be handed the same number. The loser's bind failed on a
bare thread where nothing caught it, its readiness probe was answered 200
by the winner, and it spent the rest of the test writing into another
test's server and reading that server's state back as its own. Nothing in
that sequence produced an error — only a count that made no sense.

`/health` now reports which server answered. The unit and integration
harnesses generate an identity per server, accept readiness only on their
own, and move to a new port rather than losing to whoever holds this one.
A startup failure that is not a lost race is reported with its own stack
trace instead of being retried four more times.

Two tests picked a port by guessing a number in a range with no
availability check at all; they use the same helper now. The integration
suite's two hand-rolled start loops are one recursive helper.

Refs #468

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

fix(test): stop the E2E suites deleting each other's worktrees ... 25e977b39f

Six suites created their worktrees as siblings of a directory under the
shared temp directory, and cleaned up by globbing that shared directory
for anything named after the project. Two runs on one machine deleted
each other's worktrees mid-test, which is most of why a suite that passes
alone fails in a full run.

Each suite now gives its project a parent of its own. The worktrees land
inside it and the teardown removes that parent, so a run reaches nothing
it did not create. Every assertion is relative to the project directory
and is unaffected.

The E2E runner also pointed IW_CORE_JAR at a release path a development
checkout never has. `iw-run` discards an override it cannot read, so the
override did not save a jar resolution — it destroyed the good value
`iw-run` had already exported, and every one of ~190 tests asked the
build tool for the jar again. It now names the jar the bootstrap actually
left, and refuses the run if the bootstrap left none.

Refs #468

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

feat(phase-pr): ask the forge which pull request is on this branch ... 120bc4ae63

A create that comes back a duplicate means the forge already holds a pull
request for the branch. Reading the url off the review-state card to find
out which one made a record the input to a merge decision: the card is
keyed on the issue, so it can name an earlier phase's pull request, and a
repository that keeps no card could not recover from a duplicate at all.

`fetchPullRequestForBranch` asks the forge instead. It reports the state
rather than filtering on it, because merged, closed and open are three
different answers to a caller — a lookup that returned only open ones
would be silent exactly when a caller asks what became of the work.

Only an open request can carry this push. A merged or closed one is
refused, and so is a forge that will not answer: a verdict on a resolved
request says nothing about the commit just pushed, and under `--batch`
the merge that follows would act on work nobody offered. Each refusal
names the branch, what the forge said, and that nothing was gated, so a
caller can tell it from a CI failure by the message. It stays exit 1.

`fetchPrHeadBranch` was added one release ago to check the card's url
against this branch. Nothing consults the card here now, so the check and
the port method it needed are gone.

Closes #507

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

mph added 1 commit 2026-09-03 07:46:32 +00:00

fix: act on the code review of this batch ... aa2b37a84c

Eight review skills over the range since the last review. The findings
worth acting on, and what changed.

The Forgejo branch lookup was a window, not an answer. It scanned the 50
most recently updated pull requests and filtered client-side, so
`Right(None)` meant "not among those" and phase-pr turned that into a
refusal. The forge resolves a base/head pair directly, so the port now
asks for both branches and every forge answers exactly. The branch-only
listing stays for the dashboard, which does not know a base, and now says
in its own documentation what its `None` is worth.

A branch name reached the GitLab query string unescaped. Git allows `&`
and `#` in a ref, so a branch could add a parameter to the request or
truncate it. Query values are encoded, and Forgejo's path segments are
encoded one at a time so a branch keeps its slashes.

`runStreaming` no longer prints. An adapter writing to stderr is a
channel `Console` owns, it printed a caller's whole argv where a
credential could sit, and it duplicated the line its one attentive caller
already wrote. `killedBySignal` is the channel; the tmux and herdr
adapters now say a kill was a kill instead of reporting it as a failure
to attach.

The rest: one selection rule for a branch's pull request instead of one
per forge; `ForgeConfig.credentials` instead of the same two lines in six
places; `CoreJar` beside `DashboardJar` over one `InstalledJar` search,
so the path convention has one home rather than three; the E2E runner is
expression-shaped and tells "could not start" from "ran and failed"; a
suite killed before its limit is no longer reported as having reached it;
both test-server helpers are provably tail-recursive and fail fast on a
startup bug rather than retrying it; and `FakeClock` refuses to be slept
a thousand times, so a loop that cannot end fails instead of hanging.

Refs #468, #507, #508

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

mph added 1 commit 2026-09-03 07:53:15 +00:00

fix(forgejo): find a pull request whose branch a merge deleted ... ea491cdacd

Forgejo rewrites a pull request's head.ref to refs/pull/<number>/head
once the branch is gone, and a merge that deletes the branch is this
tool's own default. The branch-only listing matched on head.ref alone, so
every merged pull request whose branch was cleaned up was invisible to
it — the dashboard read "no pull request" for work that had merged.

head.label keeps the branch the request was opened from, whether or not
the branch still exists, and the listing now matches either.

Verified against live data on all three forges: the base-and-head lookup
resolves a merged pull request with a deleted head on Forgejo, and the
head-and-base listing does the same through gh and glab.

Refs #507

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

mph added 1 commit 2026-09-03 07:59:09 +00:00

ci: fetch the checkout action from Forgejo, not from GitHub ... 142566df78

Every job began with `actions/checkout@v4`, which the runner downloads
from github.com without credentials. GitHub rate-limits unauthenticated
downloads, and when it does, the job dies before it runs a step with
"could not fetch remote 'origin'". Whichever jobs were unlucky went red,
so a run looked like a random subset of the suite failing rather than an
infrastructure limit — two runs of this branch failed that way, on
different jobs each time, with the tree green locally.

The workflow already takes upload-artifact from code.forgejo.org, which
mirrors checkout at the same v4. Taking both from there removes GitHub
from the path of a Forgejo-hosted build.

Also records what a fork's head label looks like on Forgejo, checked
against a public instance: a plain branch name, not the owner:branch form
Gitea uses, so the branch listing finds a fork's pull request too.

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

mph added 1 commit 2026-09-03 08:12:16 +00:00

docs: describe the CI this project actually runs ... 9559b0f930

CONTRIBUTING said pull requests trigger a GitHub Actions pipeline. They
run Forgejo Actions on a self-hosted runner; the GitHub remote is an
archived mirror that runs nothing. The list also named four jobs where
there are six, and described a job graph that no longer matches.

It now also states the rule the last two red runs came from: an action
has to come from code.forgejo.org, because the runner fetches one from
github.com without credentials and a rate limit there kills the job
before it runs a step.

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 08:12:31 +00:00

format job crashes the formatter instead of checking formatting #512

mph referenced this pull request 2026-09-03 08:37:03 +00:00

phase-pr cannot tell a forge that is down from a forge that refused, so both stop a batch #513

mph merged commit 6d9b48898e into main 2026-09-03 09:21:31 +00:00

mph referenced this pull request from a commit 2026-09-03 09:21:31 +00:00

Merge pull request 'Make the forge the authority on a branch's pull request, and stop the test suites lying about themselves' (#511) from forge-truth-and-test-isolation into main

mph deleted branch forge-truth-and-test-isolation 2026-09-03 09:21:31 +00:00

mph referenced this pull request 2026-09-03 09:32:13 +00:00

docs: index the jar locator and stop promising exit 1 means CI #514

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

No description provided.