stack/agents/filbert/work/queue-156/r1/body.md

Filbert review, row 156 round 1: changes

Candidate manifest afea1df3e042ccf41f5fa750b2841793605110bb45f55bc8018167d23c90fe1f (14 paths), build.patch 78684bd1...3e2d. I applied the patch with --index to d88613bac. All 14 manifest lines matched, git diff --cached --check is clean, and the diff is +803 -89. I read it against brief blob aeddbc43224f and the approved DESIGN.md.

Most of it holds. Three findings block: one in the code and two brief tests that are missing.

F1 (code): landRelease can stop a run in the middle of a landing turn

Brief step 2 says the host "stops only between turns: a run with a turn in progress keeps going to a later tick". landRelease takes "a turn in progress" to mean "a message delivered and unread" (packages/bus/src/broker.mjs:1011-1017). A landing turn has no delivered message, because the runner takes it before its first message.receive (packages/harness/src/runner.mjs:378, :401). So the check can't see a landing turn.

How it happens:

1. Run R of merge-gate starts with landing: [d1]. It claims the role and starts its d1 landing turn.
2. During that turn, d2 resolves for the same instance.
3. On the next tick, landPass finds R running, and landingOf(R) doesn't have d2 (packages/cli/src/launcher.mjs:502). It calls landRelease for d2 (:505).
4. d2 is held, R holds the claim, and nothing is delivered to R, so unread is 0 and the broker releases the role.
5. land-relaunch is logged (:515), and the SIGTERM ends R's d1 turn while it runs.

Nothing lands wrongly. If land_work for d1 hadn't run yet, d1 is still held, so the next run gets [d1, d2]. If it had, the second land refuses decision-consumed. But the brief's rule doesn't hold, the harness can be killed in the middle of a tool call, and two docs promise the opposite: packages/cli/README.md:486 ("releases the role only between turns") and docs/TOOLS.md:283 ("stopped between turns first"). DESIGN.md's release section calls the unread count "a turn in progress" and doesn't consider landing turns. So the gap is in the approved design, not only in the build.

The fix is Dewey's to choose. Two possible routes:

• The broker could see a landing turn in progress, for example from the usage.begin/usage.end bracket the runner already sends with the decision id. That record exists only when usage is configured, though.
• The runner could tell the broker when its landing turns are done, and landRelease could refuse until then.

Whichever route you take, a test should hold a landing turn open, list a second decision for the instance, tick, and assert there is no land-relaunch until the turn ends. The mutant that drops the new check should fail that test.

F2 (test): "A thread run has no landing" has no test

The brief's test list names it, and DESIGN.md's test table puts it in launcher.test.mjs. BUILD.md gap 2 says it isn't asserted. The reason given there and under equivalent mutants (BUILD.md:56: "A thread launch reads landHeld and never uses holding") isn't accurate for the mutant. If the thread ? null : guard at launcher.mjs:307 goes, a thread launch's holding is no longer null, and :347 writes landing from it with no thread check. :380 also fills landingOf and landStarted from it. The mutant survives only because the real broker never lists a thread instance. That's land.test's job, and it is pinned there, but it isn't the launcher's.

thread-launch.test.mjs is already in the candidate, and its start(..., { ops }) can replace landHolders. One test there could answer landHolders with an entry for the thread instance, launch a thread run, and assert that session.json has no landing key. That kills the guard mutant.

F3 (test): no test shows a landing turn landing

The brief says a run launched with a held decision "takes one turn on it before any message arrives, and lands. This holds with the raising run's poll active". The "one turn first" part is covered:

• runner.test has both entries before a waiting message.
• launcher.test :444 has the landing turn with no message at all.
• launcher.test :463 has the old run in its receive loop when the decision resolves.

No test has a landing turn's land_work reach the broker with a held decision:

• launcher.test answers landHolders from state.holders, while the real broker holds no decision.
• runner.test's decisions aren't held (both usage calls refuse not-recipient).
• The fake adapter calls no tool.

Neither BUILD.md's gaps nor its test map lists "lands" as uncovered. Either add a test where the landing turn's land_work passes checkLand for a held decision, or ask Sage to rule that "lands" is covered by acceptance 314 step 7 after the re-pin, and record that ruling in BUILD.md.

Checked and fine

• Broker.
  • landHeld and landHolders share #held (broker.mjs:1024). A null role skips #threaded raisers, and the two agree at every step of land.test's landHolders test, including spent and superseded.
  • landRelease refuses in DESIGN.md's order. Its unread query is the same as #agentState's (:2343). The release writes only the claim row, with by='land-relaunch'.
  • #requestUsage accepts a decision-id begin only for a listed holder, and an end only after a begin row for the same run and turn.
• process.mjs. All 17 LAUNCH_OPS names have their own launchOp line. The fallthrough now throws launch-op-refused, which is caught at :88-89 and returns the same code. An op outside LAUNCH_OPS is unchanged. The fork test asserts the direct answers for landHolders, landRelease (unknown-run) and credentialStatus, and exit 0. I accept the behavior change; Sage took it in sage-0933.
• Launcher.
  • A non-thread launch makes one landHolders read (:307) that sets both land and landing.
  • A start is keyed on the decision through landStarted, which is filled in start() for every launch path.
  • A refusal is logged once per code per decision and isn't a start.
  • A run whose landing names the decision is left alone. The SIGTERM comes only after the release and the start-time check.
  • Nothing is persisted, so a restarted host's first tick starts the holder.
• Runner.
  • landingRequest is a fixed template with no message-body content.
  • landingOk refuses before the claim, exiting 2: an extra or missing key, a non-id decision or one over 160 characters, a task ref that isn't one (including a trailing newline), and a bad entry after a good one.
  • Landing turns send nothing and mark nothing read (runner.test asserts the store rows).
• Roles. I applied the 16 match/doesNotMatch regexes in workflow-roles.test.mjs:125-150 to the candidate role files with node, after the test's whitespace collapse: 16 hold, 0 fail. This was a regex check, not a test run.
• Hash pins. No 64-hex pin in packages/*/tests or scripts/test-*.sh names a non-test manifest path. thread-migration.test.mjs:46 pins a DDL string, not broker.mjs.
• Base drift. From d88613bac to HEAD 70c9131f0, only docs/TOOLS.md changed among the manifest paths (row 154, hunk at :438). build.patch passes git apply --cached --check on HEAD in a temporary index. Sage's landing gate should still run on the new base.

Notes (not blocking)

• If a landing turn's state read fails, the runner skips the turn (runner.mjs, the could not read the current state branch), which is row 109's rule. landStarted then keeps the host from starting the decision again until a host restart, and row 149's fallback is the only path. That follows the brief's "one start per decision", but the runner log line is the only trace. It may be worth a sentence in the README section.
• packages/cli/README.md:470 points to packages/harness/README.md for the landing turn order, which doesn't mention landing turns yet (BUILD.md gap 4). Doc-only, follow-up.

Ran by me

Nothing that needed the slot. I applied the patch, ran sha256sum -c, ran git apply --check on HEAD, ran the role-regex check above, and ran greps.

Not mine

Dewey's run 3 (22:14:13Z to 22:18:52Z): 22 singles, 248/248, with P01, P02 and RU01 killed. Run 2 killed the other 30 mutants on 887c47e7. I did not rerun them.