diff --git a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java index e6a5308..c6064fe 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java +++ b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java @@ -476,20 +476,14 @@ public final class Fleetd { log.warn("credential '{}' quarantined for {}s (profile '{}'): {}", credentialId, cfg.quarantineCooldownSeconds(), profile.profile(), reason); // fleetd #446 criterion 2: name the fix, not just the fact — an operator reading this - // should not have to work out which of several configured models to touch. + // should not have to work out which of several configured models to touch. Built by + // usageLimitFixWarning/usageLimitFixWarningNoModel (extracted fleetd #446 follow-up) + // so a test can pin the exact text without capturing real log output — see those + // methods' javadoc and FleetdUsageLimitFixWarningTest. String model = profile.model(); - if (model != null && !model.isBlank()) { - log.warn("usage-limit fix: profile '{}' runs model '{}' — set `enabled: false` on " - + "that model's entry under models.allow in fleetd.yaml to stop new " - + "spawns landing on it (models: is hot, no restart needed); remove " - + "the line again once the subscription window resets", - profile.profile(), model); - } else { - log.warn("usage-limit fix: profile '{}' has no model: configured, so models.allow " - + "cannot gate it by name — the {}s quarantine above is the only " - + "thing keeping new spawns off it for now", - profile.profile(), cfg.quarantineCooldownSeconds()); - } + log.warn(model != null && !model.isBlank() + ? usageLimitFixWarning(profile.profile(), model) + : usageLimitFixWarningNoModel(profile.profile(), cfg.quarantineCooldownSeconds())); }; // fleetd #175: point the forwarding sink handed to OpenCodeLauncher above at the real one, // now that `sessions` exists to resolve target -> session -> profile. @@ -883,6 +877,40 @@ public final class Fleetd { * with 0 errors and left all 1506 existing tests green before {@code * FleetdPatternCoverageLineTest} was added to catch exactly that swap. */ + /** + * fleetd #446 follow-up: the criterion-2 WARNING text — "name the fix, not just the fact" — + * for a profile whose {@code model:} is configured. Extracted out of the {@code + * exhaustionSink} lambda the same way {@link #exhaustedPatternCoverageLine} was extracted out + * of {@code main}: the inline version compiled, ran, and read correctly, but nothing pinned + * its text, so a later edit could silently stop naming the fix and every test would stay + * green (measured: renaming the leading {@code "usage-limit fix:"} tag left all 1586 + * pre-follow-up tests passing). {@link FleetdUsageLimitFixWarningTest} asserts on the return + * value of this method directly, which is exactly what the caller logs — {@code log.warn} is + * called with this method's result as a single, already-formatted argument, so the string a + * test sees here is byte-identical to what {@code fleetd.out} receives. + * + *

Deliberately asserts on three substrings, not the whole sentence (the profile name, the + * model name, and the literal {@code enabled: false} / {@code models.allow} action) — see that + * test's class doc for why a whole-sentence pin is the wrong granularity here. + */ + static String usageLimitFixWarning(String profile, String model) { + return "usage-limit fix: profile '" + profile + "' runs model '" + model + "' — set " + + "`enabled: false` on that model's entry under models.allow in fleetd.yaml to " + + "stop new spawns landing on it (models: is hot, no restart needed); remove the " + + "line again once the subscription window resets"; + } + + /** + * fleetd #446 follow-up: the {@link #usageLimitFixWarning} counterpart for a profile with no + * {@code model:} configured — {@code models.allow} gates by model name, so there is nothing + * for an operator to flip, and this says so instead of naming a fix that does not exist. + */ + static String usageLimitFixWarningNoModel(String profile, Integer quarantineCooldownSeconds) { + return "usage-limit fix: profile '" + profile + "' has no model: configured, so " + + "models.allow cannot gate it by name — the " + quarantineCooldownSeconds + + "s quarantine above is the only thing keeping new spawns off it for now"; + } + static String exhaustedPatternCoverageLine(Set allProfiles, Set configuredProfiles) { return CompletionResolver.coverage("exhaustedPattern", CompletionResolver.UnsetMeaning.OFF, allProfiles, configuredProfiles); diff --git a/fleetd/src/test/java/dev/ltms/fleet/FleetdUsageLimitFixWarningTest.java b/fleetd/src/test/java/dev/ltms/fleet/FleetdUsageLimitFixWarningTest.java new file mode 100644 index 0000000..c496a45 --- /dev/null +++ b/fleetd/src/test/java/dev/ltms/fleet/FleetdUsageLimitFixWarningTest.java @@ -0,0 +1,75 @@ +package dev.ltms.fleet; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * fleetd #446 follow-up (criterion 2): {@code exhaustionSink} used to build the "name the fix" + * WARNING inline with two SLF4J {@code {}}-placeholder {@code log.warn} calls. Nothing pinned + * that text — a mutation battery run against the merged PR #457 proved it: renaming the leading + * {@code "usage-limit fix:"} tag to {@code "MUTANT no fix named:"} at both call sites left all + * 1586 existing tests green. This class is what makes that mutation red. + * + *

{@link Fleetd#usageLimitFixWarning} and {@link Fleetd#usageLimitFixWarningNoModel} are the + * extracted call sites {@code exhaustionSink} actually invokes — {@code log.warn} is called with + * each method's return value as a single, already-formatted argument, so the string these tests + * assert on is byte-identical to what {@code fleetd.out} receives. Same extracted-static-method + + * dedicated-test idiom as {@link Fleetd#exhaustedPatternCoverageLine} / + * {@link FleetdPatternCoverageLineTest}, chosen over the {@link ch.qos.logback.core.read.ListAppender} + * idiom {@code ExhaustedPatternGapReportTest} uses — this warning is built once, at one call site, + * from a single method with no branching inside the message itself, so there is no real log + * plumbing left to prove; capturing an appender would only add setup/teardown around the same + * assertion. + * + *

Each assertion below checks for a specific substring — the leading {@code "usage-limit fix:"} + * tag an operator would grep the log for, the profile name, the model name, and the literal + * action ({@code enabled: false} under {@code models.allow}) — rather than the whole sentence. + * Criterion 2 promised those facts, not a particular wording of the prose that carries them; a + * whole-sentence {@code assertEquals} breaks on every copy-edit of that surrounding prose and + * teaches the next person to delete the test rather than read why it failed. The leading tag is + * pinned on purpose, unlike the rest of the sentence: it is the identifying label the fleetd #446 + * follow-up's own mutation battery renamed ({@code "usage-limit fix:"} → {@code "MUTANT no fix + * named:"}) to prove this class did not yet catch a tag rename — only the three-fact substrings + * above would have missed it, since none of them mention the tag itself. + */ +class FleetdUsageLimitFixWarningTest { + + @Test + @DisplayName("usageLimitFixWarning names the profile, the model, and the models.allow fix") + void usageLimitFixWarningNamesProfileModelAndFix() { + String warning = Fleetd.usageLimitFixWarning("terra", "claude-opus-5"); + + assertTrue(warning.startsWith("usage-limit fix:"), + "must carry the grep-able identifying tag: " + warning); + assertTrue(warning.contains("terra"), "must name the profile: " + warning); + assertTrue(warning.contains("claude-opus-5"), "must name the model: " + warning); + assertTrue(warning.contains("enabled: false"), "must name the action: " + warning); + assertTrue(warning.contains("models.allow"), "must name where the action goes: " + warning); + } + + @Test + @DisplayName("usageLimitFixWarning does not cross-name a different profile or model") + void usageLimitFixWarningDoesNotCrossName() { + String warning = Fleetd.usageLimitFixWarning("terra", "claude-opus-5"); + + // Guards against the mutation of swapping the two profile/model arguments at the call + // site — a test that only checks "some name appears" cannot catch that. + assertTrue(!warning.contains("sonnet"), "must not name an unrelated model: " + warning); + } + + @Test + @DisplayName("usageLimitFixWarningNoModel names the profile but no fix, since none exists") + void usageLimitFixWarningNoModelNamesProfileNotAFix() { + String warning = Fleetd.usageLimitFixWarningNoModel("gx", 1800); + + assertTrue(warning.startsWith("usage-limit fix:"), + "must carry the grep-able identifying tag: " + warning); + assertTrue(warning.contains("gx"), "must name the profile: " + warning); + assertTrue(warning.contains("1800"), "must name the quarantine duration it falls back to: " + warning); + assertTrue(!warning.contains("enabled: false"), + "a profile with no model: configured has no models.allow entry to flip — the " + + "fallback message must not claim one exists: " + warning); + } +} diff --git a/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetProfilesQuarantineModelReasonFieldsTest.java b/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetProfilesQuarantineModelReasonFieldsTest.java new file mode 100644 index 0000000..d9ba222 --- /dev/null +++ b/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetProfilesQuarantineModelReasonFieldsTest.java @@ -0,0 +1,111 @@ +package dev.ltms.fleet.mcp; + +import dev.ltms.fleet.config.FleetConfig; +import dev.ltms.fleet.guard.SubscriptionGuard; +import dev.ltms.fleet.herdr.AgentControl; +import dev.ltms.fleet.herdr.FakeHerdr; +import dev.ltms.fleet.herdr.WorkspaceControl; +import dev.ltms.fleet.member.ClaudeCodeLauncher; +import dev.ltms.fleet.peer.PeerLauncher; +import dev.ltms.fleet.placement.BackendQuarantine; +import io.modelcontextprotocol.spec.McpSchema; +import org.junit.jupiter.api.Test; + +import java.util.Map; +import java.util.Set; +import java.util.concurrent.TimeUnit; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * fleetd #446 follow-up (criterion 3): a mutation battery run against the merged PR #457 proved + * that deleting either {@code row.put("model", model);} or {@code row.put("reason", reason);} + * from {@link FleetMcp#profilesView} left all 1586 existing tests green — nothing exercised a + * {@link FleetMcp.QuarantineSource} whose {@code modelFor}/{@code reasonFor} actually return a + * value. This class is what makes both deletions red, and also pins the two "must be omitted, not + * emitted as null/blank" branches those two {@code if} guards exist for — {@link + * FleetMcpTest#profilesReportsAQuarantinedCredential} covers the credential/quarantine-duration + * shape but supplies neither function, so it cannot distinguish "the field is missing" from "the + * field was never asked for". + */ +class FleetProfilesQuarantineModelReasonFieldsTest { + + private static final String PROFILE = "terra"; + private static final String CREDENTIAL = "cred-terra"; + + private static PeerLauncher launcher(FakeHerdr h) { + FleetConfig.Profile profile = new FleetConfig.Profile( + PROFILE, "http://gx00.gw:8000", "claude-opus-5", null, "FLEETD_WORKER_TOKEN", null, + "tab", "fleetd-workers", "worker: {profile} #{n}", null, null, null); + return new ClaudeCodeLauncher(new AgentControl(h), new WorkspaceControl(h), + new SubscriptionGuard(Set.of("gx00.gw")), Map.of(PROFILE, profile), PROFILE, _ -> "tok"); + } + + private static BackendQuarantine quarantined(String credentialId) { + BackendQuarantine quarantine = new BackendQuarantine(() -> 0L, TimeUnit.MINUTES.toNanos(30)); + quarantine.quarantine(credentialId); + return quarantine; + } + + private static String textOf(McpSchema.CallToolResult r) { + return ((McpSchema.TextContent) r.content().getFirst()).text(); + } + + @Test + void quarantinedRowNamesTheModelAndTheReason() { + FakeHerdr h = new FakeHerdr(); + FleetMcp.QuarantineSource source = new FleetMcp.QuarantineSource( + _ -> CREDENTIAL, quarantined(CREDENTIAL), _ -> true, + _ -> "claude-opus-5", _ -> "The usage limit has been reached"); + + McpSchema.CallToolResult res = FleetMcp.profiles(launcher(h), source); + assertNotEquals(Boolean.TRUE, res.isError()); + String out = textOf(res); + + assertTrue(out.contains("\"quarantined\""), out); + assertTrue(out.contains("\"model\":\"claude-opus-5\""), + "the quarantined row must name the model the fix applies to: " + out); + assertTrue(out.contains("\"reason\":\"The usage limit has been reached\""), + "the quarantined row must name why it was quarantined: " + out); + } + + @Test + void quarantinedRowOmitsModelWhenNoneIsConfigured() { + FakeHerdr h = new FakeHerdr(); + // null and "" (blank) both exercise the same production guard (`!model.isBlank()`) — + // covered together since both must produce the identical outcome: the key absent. + for (String noModel : new String[] {null, "", " "}) { + FleetMcp.QuarantineSource source = new FleetMcp.QuarantineSource( + _ -> CREDENTIAL, quarantined(CREDENTIAL), _ -> true, + _ -> noModel, _ -> "some reason"); + + McpSchema.CallToolResult res = FleetMcp.profiles(launcher(h), source); + String out = textOf(res); + + assertTrue(out.contains("\"quarantined\""), out); + assertFalse(out.contains("\"model\""), + "modelFor returned " + (noModel == null ? "null" : "'" + noModel + "'") + + " — the key must be ABSENT, not emitted as null or an empty string: " + out); + } + } + + @Test + void quarantinedRowOmitsReasonWhenNoneIsKnown() { + FakeHerdr h = new FakeHerdr(); + for (String noReason : new String[] {null, "", " "}) { + FleetMcp.QuarantineSource source = new FleetMcp.QuarantineSource( + _ -> CREDENTIAL, quarantined(CREDENTIAL), _ -> true, + _ -> "claude-opus-5", _ -> noReason); + + McpSchema.CallToolResult res = FleetMcp.profiles(launcher(h), source); + String out = textOf(res); + + assertTrue(out.contains("\"quarantined\""), out); + assertFalse(out.contains("\"reason\""), + "reasonFor returned " + (noReason == null ? "null" : "'" + noReason + "'") + + " — the key must be ABSENT, not emitted as null or an empty string: " + out); + } + } +}