fleetd #489 follow-up: fix stale class javadoc, off-by-one nudge count, weak test
Three review corrections on top of the previous commit:
1. The class javadoc's four-step continuation list (lines 47-58) was stale.
Step 1 said "report an injectable state", but waitUntilAtTurnBoundary's
own javadoc excludes BLOCKED - fixed to say IDLE or DONE. Step 3 still
described the old plain re-check ("the original, pre-correction wait...
still here") - fixed to describe what waitForClearPickupAndSettle
actually does: nudge while unpicked-up, then wait for a real WORKING ->
IDLE/DONE boundary, releasing rather than wedging if WORKING never shows.
2. PICKUP_GRACE_POLLS=8 bounds the number of consecutive not-yet-picked-up
polls, not the number of nudges - the 8th poll releases instead of
nudging again, so 8 polls produce 7 nudges. The log.info in the release
branch and two javadoc spots said "8 nudges"; fixed all three to state
the poll count and the nudge count separately and correctly. Behavior
and the constant are unchanged.
3. pickupSeenStopsNudgingAndBootstrapTextIsSent asserted only promptCallCount
and sendKeysCallCount, both of which a return-true stub also satisfies.
Added an assertion on the already-tracked postClearGetCalls counter
(>= 2), which only a real post-/clear poll loop can produce - this is
what makes the test fail against a return-true mutant.
This commit is contained in:
@@ -44,15 +44,18 @@ import java.util.function.Supplier;
|
||||
* {@code continuationRunner} before returning. That continuation is what actually touches the pane,
|
||||
* once the calling turn has ended, in this order:
|
||||
* <ol>
|
||||
* <li>wait for the lead's own pane to report an injectable state — i.e. wait for the very
|
||||
* {@code confirm()} call that approved this roll to finish its turn — bounded by
|
||||
* {@code turnSettleSeconds}. <strong>If this never happens, nothing else in this list runs:
|
||||
* no {@code /clear} is ever sent.</strong> A lead that never goes idle is a lead still doing
|
||||
* real work, and clearing it would throw away live context — exactly the failure this
|
||||
* correction exists to prevent.</li>
|
||||
* <li>wait for the lead's own pane to report a real turn boundary — {@code IDLE} or {@code
|
||||
* DONE}, never merely {@code BLOCKED} — i.e. wait for the very {@code confirm()} call that
|
||||
* approved this roll to finish its turn — bounded by {@code turnSettleSeconds}. <strong>If
|
||||
* this never happens, nothing else in this list runs: no {@code /clear} is ever sent.</strong>
|
||||
* A lead that never goes idle is a lead still doing real work, and clearing it would throw
|
||||
* away live context — exactly the failure this correction exists to prevent.</li>
|
||||
* <li>{@code agents.send(lead, "/clear")}</li>
|
||||
* <li>wait again for the pane to report injectable, bounded by {@code clearSettleSeconds} (this
|
||||
* is the original, pre-correction wait — still here, just no longer the only one)</li>
|
||||
* <li>wait for {@code /clear} to be picked up and settle, bounded by {@code clearSettleSeconds}
|
||||
* (fleetd #489: no longer a plain re-check of the same boundary — {@code /clear} starts no
|
||||
* turn of its own, so this instead nudges the submit keystroke while no pickup has been seen,
|
||||
* then waits for a real {@code WORKING} → {@code IDLE}/{@code DONE} boundary once one has;
|
||||
* see {@link #waitForClearPickupAndSettle})</li>
|
||||
* <li>{@code agents.send(lead, cfg.bootstrapTextFor(p.handoverPath()))}</li>
|
||||
* </ol>
|
||||
* A {@link #confirm} that returns {@link RollDecision#approved()} therefore means <em>"every gate
|
||||
@@ -112,10 +115,13 @@ public final class LeadRollover {
|
||||
static final long SETTLE_POLL_MS = 250;
|
||||
|
||||
/**
|
||||
* How many consecutive not-yet-picked-up polls {@link #waitForClearPickupAndSettle} nudges the
|
||||
* submit keystroke before releasing rather than wedging the roll — the same constant and the
|
||||
* same release-not-wedge choice {@link dev.ltms.fleet.inject.Injector} already makes for its own
|
||||
* post-turn {@code /clear} housekeeping (fleetd #306).
|
||||
* How many consecutive not-yet-picked-up polls {@link #waitForClearPickupAndSettle} allows
|
||||
* before releasing rather than wedging the roll — the same constant and the same
|
||||
* release-not-wedge choice {@link dev.ltms.fleet.inject.Injector} already makes for its own
|
||||
* post-turn {@code /clear} housekeeping (fleetd #306). <strong>This bounds the number of
|
||||
* consecutive polls, not the number of nudges:</strong> the first {@code PICKUP_GRACE_POLLS - 1}
|
||||
* of those polls each send a nudge, and the {@code PICKUP_GRACE_POLLS}th releases instead of
|
||||
* nudging again — so 8 polls produce 7 nudges, not 8.
|
||||
*/
|
||||
static final int PICKUP_GRACE_POLLS = 8;
|
||||
|
||||
@@ -510,13 +516,16 @@ public final class LeadRollover {
|
||||
* <ul>
|
||||
* <li>an {@link AgentStatus#WORKING} sample means {@code /clear} was picked up as a real
|
||||
* turn;</li>
|
||||
* <li>until that happens, every poll that still reports {@link AgentStatus#IDLE} or {@link
|
||||
* <li>until that happens, each poll that still reports {@link AgentStatus#IDLE} or {@link
|
||||
* AgentStatus#DONE} re-sends the submit keystroke ({@link AgentControl#submit}) to nudge
|
||||
* the raced Enter — up to {@link #PICKUP_GRACE_POLLS} times. A second Enter on an empty
|
||||
* the raced Enter — for the first {@code PICKUP_GRACE_POLLS - 1} of {@link
|
||||
* #PICKUP_GRACE_POLLS} consecutive such polls (i.e. {@code PICKUP_GRACE_POLLS - 1}
|
||||
* nudges: 7, not 8, given {@code PICKUP_GRACE_POLLS = 8}). A second Enter on an empty
|
||||
* Claude Code prompt is a no-op, so repeating it is safe;</li>
|
||||
* <li>if the nudge budget runs out with {@code WORKING} never observed, this releases rather
|
||||
* than wedges the roll — the same choice {@code Injector} makes — and returns {@code true}
|
||||
* anyway, logged at {@code info} so an operator can see which path ran;</li>
|
||||
* <li>the {@code PICKUP_GRACE_POLLS}th consecutive such poll, with {@code WORKING} still never
|
||||
* observed, releases rather than wedges the roll instead of nudging again — the same
|
||||
* choice {@code Injector} makes — and returns {@code true} anyway, logged at {@code info}
|
||||
* so an operator can see which path ran;</li>
|
||||
* <li>once {@code WORKING} has been observed, nudging stops and this instead waits for a real
|
||||
* {@code working → IDLE/DONE} completion boundary before returning {@code true}.</li>
|
||||
* </ul>
|
||||
@@ -558,8 +567,9 @@ public final class LeadRollover {
|
||||
}
|
||||
if (++idlePollsAwaitingPickup >= PICKUP_GRACE_POLLS) {
|
||||
log.info("lead-rollover: /clear on {} was never observed as WORKING after {} "
|
||||
+ "nudges — releasing rather than wedging the roll",
|
||||
target, PICKUP_GRACE_POLLS);
|
||||
+ "consecutive IDLE/DONE polls ({} of those were nudged) — "
|
||||
+ "releasing rather than wedging the roll",
|
||||
target, PICKUP_GRACE_POLLS, PICKUP_GRACE_POLLS - 1);
|
||||
return true;
|
||||
}
|
||||
try {
|
||||
|
||||
@@ -548,6 +548,12 @@ class LeadRolloverTest {
|
||||
+ "still complete the full roll — /clear then bootstrapText");
|
||||
assertEquals(0, sendKeysCallCount(fake), "once WORKING was observed, nudging must stop "
|
||||
+ "immediately — no agent.send_keys call should ever have been needed or sent");
|
||||
assertTrue(postClearGetCalls.get() >= 2, "the pane's status must have been polled AGAIN "
|
||||
+ "after the WORKING sample, before bootstrapText was sent — this is what proves "
|
||||
+ "the method actually waited for the WORKING -> IDLE completion boundary instead "
|
||||
+ "of returning as soon as pickup was seen (or worse, without polling at all, as a "
|
||||
+ "stub that just returns true would); got " + postClearGetCalls.get()
|
||||
+ " agent.get call(s) after /clear");
|
||||
}
|
||||
|
||||
@Test
|
||||
|
||||
Reference in New Issue
Block a user