fleet/fleetd
3
0
Fork 0
Code Issues 81 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

Remove LeadContextGauge.read's 3-arg overload — the last back-compat form that silently restores the fixed 200000 threshold #663

New Issue
Closed
opened 2026-10-03 19:15:01 +02:00 by ltms · 2 comments
ltms commented 2026-10-03 19:15:01 +02:00
Owner
Copy Link
Copy Source

Follow-up to #659, which asked for this decision and did not get one. PR #662 removed the three
forms named in #659's table but left LeadContextGauge.read's 3-argument overload untouched, and
said nothing about it either way. This ticket records the measurement and closes the gap.

The measurement

I ran all of this on main at 9417de1.

LeadContextGauge has two read forms:

176:    public Reading read(String configDir, String sessionId, String agentType) {
177:        return read(configDir, sessionId, agentType, null);
187:    public Reading read(String configDir, String sessionId, String agentType, Long effectiveWindowTokens) {

Production callers of the 3-arg form: zero. Both production callers use the 4-arg form:

$ grep -rn "\.read(" --include='*.java' fleetd/src/main/java | grep -iE "gauge|contextGauge"
fleetd/src/main/java/dev/ltms/fleet/Fleetd.java:1149:   gauge.read(configDir, live.sessionId(), live.agentType(), effectiveWindow);
fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java:2180:   contextGauge.read(configDir, sessionId, agentType, effectiveWindowTokens);

Test callers of the 3-arg form: 11, split as 10 in lead/LeadContextGaugeTest.java and 1 in
lead/LeadContextGaugeHighThresholdTest.java. There are 7 further 4-arg test call sites that need
no change.

Why it is worth removing

Same reason as #659, and it is the last instance of that shape. The 3-arg form passes null for
the window, so the gauge falls back to the fixed HIGH_THRESHOLD_TOKENS = 200_000. Any future
caller that reaches for the short form gets #637's defect back, silently and with a green build.
A back-compat form whose fallback is the old bug is a trap, not a convenience.

#659 also noted that this overload is what makes #637's cache-key defect reachable: with only the
4-arg form, every caller for a given lead resolves the same window, and the stale-cache failure
needs two different windows.

The ask

Remove LeadContextGauge.read(String, String, String) and migrate the 11 test call sites to pass
the window explicitly.

The trap — read this before touching the tests

Do not mechanically pass null everywhere. For most of the 10 sites in LeadContextGaugeTest
the window is genuinely irrelevant and an explicit null is right. But at least one site is
different:

lead/LeadContextGaugeHighThresholdTest.java calls read(legacyConfigDir, SESSION_ID, "claude")
and that test's whole purpose is the fixed-200000 fallback — noEffectiveWindowFallsBackToTheFixed200000Default
is the test #637 added, and it is the control for this behaviour. There null is the
meaningful value under test, not filler. Pass it explicitly and leave the assertion alone.
Read each test's name and assertion before choosing the argument.

Acceptance

  • grep -n "read(String configDir, String sessionId, String agentType)" fleetd/src/main/java/dev/ltms/fleet/lead/LeadContextGauge.java
    returns nothing. Pair that zero with a positive control — a broken pattern returns a clean 0
    that reads exactly like success, so also grep for the 4-arg form and show it still matches.
  • cd fleetd && mvn -o clean install passes, and you report the real Tests run: line.
  • The test count does not drop below 1923. If it moves, say which test went and why.
  • LeadContextGaugeHighThresholdTest.noEffectiveWindowFallsBackToTheFixed200000Default still
    exists and still asserts the 200000 fallback. Prove it still works: mutate
    HIGH_THRESHOLD_TOKENS to * 2 and show that test goes red, then revert and show an empty
    diffstat.

One correction to #659's record, while this is open

#659's body says there were "8 old-form call sites across 4 files", and says a reviewer's count of
7 across 3 files "misses FleetMcpLeadContextGaugeWiringTest.java". That correction was itself
wrong. The real count was 7 across 3 files, which is what PR #662 changed.

The supposed 8th site is in FleetdLeadConfigDirSourceWiringTest.java, and it is not a call site.
Every LeadConfigDirSource mention in that file is either inside a javadoc comment (lines 15-42)
or a call to the Fleetd.leadConfigDirSource(..) factory (lines 69, 85, 94). It never used the
removed 1-argument constructor, which is why PR #662 did not need to touch it and why the build
stayed green without it.

Recording this because #659 tells a future reader to "use the 8/4 figure", and that instruction
would send them hunting for a call site that does not exist.

Follow-up to #659, which asked for this decision and did not get one. PR #662 removed the three forms named in #659's table but left `LeadContextGauge.read`'s 3-argument overload untouched, and said nothing about it either way. This ticket records the measurement and closes the gap. ## The measurement I ran all of this on `main` at `9417de1`. `LeadContextGauge` has two `read` forms: ``` 176: public Reading read(String configDir, String sessionId, String agentType) { 177: return read(configDir, sessionId, agentType, null); 187: public Reading read(String configDir, String sessionId, String agentType, Long effectiveWindowTokens) { ``` **Production callers of the 3-arg form: zero.** Both production callers use the 4-arg form: ``` $ grep -rn "\.read(" --include='*.java' fleetd/src/main/java | grep -iE "gauge|contextGauge" fleetd/src/main/java/dev/ltms/fleet/Fleetd.java:1149: gauge.read(configDir, live.sessionId(), live.agentType(), effectiveWindow); fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java:2180: contextGauge.read(configDir, sessionId, agentType, effectiveWindowTokens); ``` **Test callers of the 3-arg form: 11**, split as 10 in `lead/LeadContextGaugeTest.java` and 1 in `lead/LeadContextGaugeHighThresholdTest.java`. There are 7 further 4-arg test call sites that need no change. ## Why it is worth removing Same reason as #659, and it is the last instance of that shape. The 3-arg form passes `null` for the window, so the gauge falls back to the fixed `HIGH_THRESHOLD_TOKENS = 200_000`. Any future caller that reaches for the short form gets #637's defect back, silently and with a green build. A back-compat form whose fallback is the old bug is a trap, not a convenience. #659 also noted that this overload is what makes #637's cache-key defect reachable: with only the 4-arg form, every caller for a given lead resolves the same window, and the stale-cache failure needs two different windows. ## The ask Remove `LeadContextGauge.read(String, String, String)` and migrate the 11 test call sites to pass the window explicitly. ## The trap — read this before touching the tests Do **not** mechanically pass `null` everywhere. For most of the 10 sites in `LeadContextGaugeTest` the window is genuinely irrelevant and an explicit `null` is right. But at least one site is different: `lead/LeadContextGaugeHighThresholdTest.java` calls `read(legacyConfigDir, SESSION_ID, "claude")` and that test's whole purpose is the fixed-200000 fallback — `noEffectiveWindowFallsBackToTheFixed200000Default` is the test #637 added, and it is the control for this behaviour. There `null` is the **meaningful value under test**, not filler. Pass it explicitly and leave the assertion alone. Read each test's name and assertion before choosing the argument. ## Acceptance - `grep -n "read(String configDir, String sessionId, String agentType)" fleetd/src/main/java/dev/ltms/fleet/lead/LeadContextGauge.java` returns nothing. Pair that zero with a positive control — a broken pattern returns a clean `0` that reads exactly like success, so also grep for the 4-arg form and show it still matches. - `cd fleetd && mvn -o clean install` passes, and you report the real `Tests run:` line. - **The test count does not drop below 1923.** If it moves, say which test went and why. - `LeadContextGaugeHighThresholdTest.noEffectiveWindowFallsBackToTheFixed200000Default` still exists and still asserts the 200000 fallback. Prove it still works: mutate `HIGH_THRESHOLD_TOKENS` to `* 2` and show that test goes red, then revert and show an empty diffstat. ## One correction to #659's record, while this is open #659's body says there were "8 old-form call sites across 4 files", and says a reviewer's count of 7 across 3 files "misses `FleetMcpLeadContextGaugeWiringTest.java`". That correction was itself wrong. The real count was **7 across 3 files**, which is what PR #662 changed. The supposed 8th site is in `FleetdLeadConfigDirSourceWiringTest.java`, and it is not a call site. Every `LeadConfigDirSource` mention in that file is either inside a javadoc comment (lines 15-42) or a call to the `Fleetd.leadConfigDirSource(..)` factory (lines 69, 85, 94). It never used the removed 1-argument constructor, which is why PR #662 did not need to touch it and why the build stayed green without it. Recording this because #659 tells a future reader to "use the 8/4 figure", and that instruction would send them hunting for a call site that does not exist.
ltms commented 2026-10-03 19:31:06 +02:00
Author
Owner
Copy Link
Copy Source

Lead review of PR #665 — one change needed before merge

I read the diff myself and verified the counts. The migration is correct. One leftover must go.

Verified good

  • Zero production callers of the 3-arg form, before and after. Both production sites were already 4-arg (Fleetd.java:1149, mcp/FleetMcp.java:2180).
  • 15 test call sites migrated, 15 removed and 15 added in the diff, and zero 3-arg calls remain in either test file. The worker's corrected count of 15 is right; the "11" in my brief and in this issue's body was wrong.
  • The javadoc of the removed overload is not lost. The 3-arg method sat between the first javadoc's @param tags and the second javadoc block, so deleting its closing */, the method, and the opening /** fuses the two blocks into one. The surviving 4-arg read now documents all four of its params. That is why the production file shows 6 deletions and 0 insertions.
  • The LeadContextGaugeTest changes are all mechanical. Those are generic property tests — token totals, compaction counts, UNKNOWN cases, torn final line, TTL cache — where the window is genuinely irrelevant, so null is filler there.

The finding — a dead assertion with a false message

fleetd/src/test/java/dev/ltms/fleet/lead/LeadContextGaugeHighThresholdTest.java:84-89, the third assertion inside noEffectiveWindowFallsBackToTheFixed200000Default.

Before this change it pinned a real property: the 3-arg read() behaves exactly like passing a null window. That property needed two ways to call read. Now there is one, and the assertion reads:

String legacyConfigDir = writeTranscript(tmp.resolve("legacy"), SESSION_ID, 200_000);
LeadContextGauge legacyGauge = new LeadContextGauge();
assertEquals(LeadContextGauge.State.HIGH,
        legacyGauge.read(legacyConfigDir, SESSION_ID, "claude", null).state(),
        "the 3-arg read() (no window argument at all) must behave exactly like passing a null window");

That is the same call, the same input (200_000) and the same expectation as the atGauge assertion four lines above it. Only the temp dir and the gauge instance differ, and the assertion is about neither.

Two things are wrong with it:

  1. It cannot fail unless atGauge already failed. It adds no coverage.
  2. Its message names a method that no longer exists. A reader who goes looking for "the 3-arg read()" will not find it. Project rule: a comment describes the code as it is now, and a test comment names the behaviour it protects.

This is the trap in this issue, in a different shape than expected. The worker did read the assertions and did keep the meaningful ones — the first two still pin the fallback. What it missed is that the third assertion's subject was the thing being deleted.

What to do

Delete those three lines — the legacyConfigDir setup, the legacyGauge, and their assertEquals. The property they pinned cannot go wrong any more, because there is no second way to call read.

Do not weaken the first two assertions. 199_999 → OK and 200_000 → HIGH are the test's stated purpose and the behaviour #637 fixed. They stay exactly as they are.

Then re-run the mutation: HIGH_THRESHOLD_TOKENS = 200_000 → 200_000 * 2 must make noEffectiveWindowFallsBackToTheFixed200000Default go red. The earlier run failing at the atGauge assertion before reaching the legacy one was fine, and after this deletion atGauge is the last assertion in the method anyway.

Tests run: must stay 1923. Deleting an assertion inside a @Test method does not change the test count. If it moves, say so rather than explaining it away.

## Lead review of PR #665 — one change needed before merge I read the diff myself and verified the counts. The migration is correct. One leftover must go. ### Verified good - **Zero** production callers of the 3-arg form, before and after. Both production sites were already 4-arg (`Fleetd.java:1149`, `mcp/FleetMcp.java:2180`). - **15** test call sites migrated, 15 removed and 15 added in the diff, and **zero** 3-arg calls remain in either test file. The worker's corrected count of 15 is right; the "11" in my brief and in this issue's body was wrong. - The javadoc of the removed overload is not lost. The 3-arg method sat between the first javadoc's `@param` tags and the second javadoc block, so deleting its closing `*/`, the method, and the opening `/**` fuses the two blocks into one. The surviving 4-arg `read` now documents all four of its params. That is why the production file shows 6 deletions and 0 insertions. - The `LeadContextGaugeTest` changes are all mechanical. Those are generic property tests — token totals, compaction counts, UNKNOWN cases, torn final line, TTL cache — where the window is genuinely irrelevant, so `null` is filler there. ### The finding — a dead assertion with a false message `fleetd/src/test/java/dev/ltms/fleet/lead/LeadContextGaugeHighThresholdTest.java:84-89`, the third assertion inside `noEffectiveWindowFallsBackToTheFixed200000Default`. Before this change it pinned a real property: the 3-arg `read()` behaves exactly like passing a null window. That property needed two ways to call `read`. Now there is one, and the assertion reads: ```java String legacyConfigDir = writeTranscript(tmp.resolve("legacy"), SESSION_ID, 200_000); LeadContextGauge legacyGauge = new LeadContextGauge(); assertEquals(LeadContextGauge.State.HIGH, legacyGauge.read(legacyConfigDir, SESSION_ID, "claude", null).state(), "the 3-arg read() (no window argument at all) must behave exactly like passing a null window"); ``` That is the same call, the same input (200_000) and the same expectation as the `atGauge` assertion four lines above it. Only the temp dir and the gauge instance differ, and the assertion is about neither. Two things are wrong with it: 1. **It cannot fail unless `atGauge` already failed.** It adds no coverage. 2. **Its message names a method that no longer exists.** A reader who goes looking for "the 3-arg `read()`" will not find it. Project rule: a comment describes the code as it is now, and a test comment names the behaviour it protects. This is the trap in this issue, in a different shape than expected. The worker did read the assertions and did keep the meaningful ones — the first two still pin the fallback. What it missed is that the third assertion's *subject* was the thing being deleted. ### What to do Delete those three lines — the `legacyConfigDir` setup, the `legacyGauge`, and their `assertEquals`. The property they pinned cannot go wrong any more, because there is no second way to call `read`. **Do not weaken the first two assertions.** `199_999 → OK` and `200_000 → HIGH` are the test's stated purpose and the behaviour #637 fixed. They stay exactly as they are. Then re-run the mutation: `HIGH_THRESHOLD_TOKENS = 200_000` → `200_000 * 2` must make `noEffectiveWindowFallsBackToTheFixed200000Default` go red. The earlier run failing at the `atGauge` assertion before reaching the legacy one was fine, and after this deletion `atGauge` is the last assertion in the method anyway. `Tests run:` must stay **1923**. Deleting an assertion inside a `@Test` method does not change the test count. If it moves, say so rather than explaining it away.
ltms commented 2026-10-03 19:38:48 +02:00
Author
Owner
Copy Link
Copy Source

Merged as 7e48d4b, pushed to main

PR #665 was merged locally and pushed. We merge locally, so the forge does not close the PR by itself — I am closing both by hand.

What I verified myself, not taken from the worker

Built the PR head (60fa86a) in a throwaway git worktree, never the main clone — see #664 for why that matters.

  • mvn clean install, unpiped, exit 0: Tests run: 1923, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS, 170 surefire report files (target/surefire-reports removed first, so no deleted class could be re-counted).
  • Mutation. HIGH_THRESHOLD_TOKENS = 200_000 → 200_000 * 2 turns the test red at the right place: 
    LeadContextGaugeHighThresholdTest.noEffectiveWindowFallsBackToTheFixed200000Default:81
  the fixed default must still be 200,000 when no window is resolvable ==> expected: <HIGH> but was: <OK>
    Reverted, touched the source so maven could not re-run the mutated .class, confirmed the file was byte-identical to the backup and the worktree clean against HEAD, then re-ran green: Tests run: 4, Failures: 0.
  • The merge needed no second build. The merged tree 9032841433a4d54e61d7a5b1346a1f335ad7b3cc is byte-identical to the PR head tree I had just built, and main had not moved (git rev-list --count 9417de1..origin/main = 0).

Corrections to this issue's record

  • The test call-site count was 15, not 11. My brief and this issue's body both said 11. The worker re-measured and was right. The old pattern \.read\([^()]*\) silently drops any call whose arguments contain nested parens — tmp.toString() — and happens to return a clean "10". My own "claude") check independently landed on 13, missing the two sites that pass null and "opencode" as the agentType. Listing the removed lines settles it at 15, and the diff shows 15 removed and 15 added.
  • tabPlacement() is at FleetConfig.java:845, not 846 as my brief said. Unrelated to this issue, but the same brief carried it.
  • The removed overload's javadoc was not lost. The 3-arg method sat between the first javadoc's @param tags and the second javadoc block, so deleting it fuses the two into one block that documents all four params of the surviving method. That is why the production file shows 6 deletions and 0 insertions.

The review finding, and what it says about the trap

The first round left a third assertion in noEffectiveWindowFallsBackToTheFixed200000Default whose subject was the deleted overload: same call, same input and same expectation as the atGauge assertion above it, under a message naming "the 3-arg read()". It could not fail unless atGauge already had, and it described a method that no longer existed. Removed in 60fa86a; the two meaningful assertions (199_999 → OK, 200_000 → HIGH) are untouched.

Worth recording for the next person: the trap was predicted as "the worker pattern-matches null everywhere without reading the assertions". It arrived in a different shape. The worker did read them and did keep the meaningful ones. What is easy to miss is that one assertion's subject is the thing being deleted, so it survives as a passing duplicate rather than as a failure.

## Merged as `7e48d4b`, pushed to `main` PR #665 was merged locally and pushed. We merge locally, so the forge does not close the PR by itself — I am closing both by hand. ### What I verified myself, not taken from the worker Built the PR head (`60fa86a`) in a **throwaway git worktree**, never the main clone — see #664 for why that matters. - `mvn clean install`, unpiped, exit **0**: `Tests run: 1923, Failures: 0, Errors: 0, Skipped: 0`, `BUILD SUCCESS`, **170** surefire report files (`target/surefire-reports` removed first, so no deleted class could be re-counted). - **Mutation.** `HIGH_THRESHOLD_TOKENS = 200_000` → `200_000 * 2` turns the test red at the right place: ``` LeadContextGaugeHighThresholdTest.noEffectiveWindowFallsBackToTheFixed200000Default:81 the fixed default must still be 200,000 when no window is resolvable ==> expected: <HIGH> but was: <OK> ``` Reverted, `touch`ed the source so maven could not re-run the mutated `.class`, confirmed the file was byte-identical to the backup and the worktree clean against `HEAD`, then re-ran green: `Tests run: 4, Failures: 0`. - **The merge needed no second build.** The merged tree `9032841433a4d54e61d7a5b1346a1f335ad7b3cc` is byte-identical to the PR head tree I had just built, and `main` had not moved (`git rev-list --count 9417de1..origin/main` = 0). ### Corrections to this issue's record - **The test call-site count was 15, not 11.** My brief and this issue's body both said 11. The worker re-measured and was right. The old pattern `\.read\([^()]*\)` silently drops any call whose arguments contain nested parens — `tmp.toString()` — and happens to return a clean "10". My own `"claude")` check independently landed on 13, missing the two sites that pass `null` and `"opencode"` as the agentType. Listing the removed lines settles it at 15, and the diff shows 15 removed and 15 added. - **`tabPlacement()` is at `FleetConfig.java:845`**, not 846 as my brief said. Unrelated to this issue, but the same brief carried it. - The removed overload's javadoc was not lost. The 3-arg method sat between the first javadoc's `@param` tags and the second javadoc block, so deleting it fuses the two into one block that documents all four params of the surviving method. That is why the production file shows 6 deletions and 0 insertions. ### The review finding, and what it says about the trap The first round left a third assertion in `noEffectiveWindowFallsBackToTheFixed200000Default` whose subject was the deleted overload: same call, same input and same expectation as the `atGauge` assertion above it, under a message naming "the 3-arg `read()`". It could not fail unless `atGauge` already had, and it described a method that no longer existed. Removed in `60fa86a`; the two meaningful assertions (`199_999 → OK`, `200_000 → HIGH`) are untouched. Worth recording for the next person: the trap was predicted as "the worker pattern-matches `null` everywhere without reading the assertions". It arrived in a different shape. The worker **did** read them and **did** keep the meaningful ones. What is easy to miss is that one assertion's *subject* is the thing being deleted, so it survives as a passing duplicate rather than as a failure.
ltms closed this issue 2026-10-03 19:38:58 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#663
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?