diff --git a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java index eddc419..7181b7a 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java +++ b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java @@ -380,8 +380,7 @@ public final class Fleetd { .map(session -> exhaustedPatternsByProfile.get(session.profile())) .orElse(null); log.info("backend-exhausted classification (CB-578 stage A): {}", - CompletionResolver.coverage("exhaustedPattern", cfg.profiles().keySet(), - exhaustedPatternsByProfile.keySet())); + exhaustedPatternCoverageLine(cfg.profiles().keySet(), exhaustedPatternsByProfile.keySet())); // fleetd #201 Unit 5: classify a completion-fallback scrape that matches a profile's // configured backend-error refusal (a credential outage, a provider 5xx) as a backend error // rather than handing it back as a real answer. Compiled once at startup, keyed by profile @@ -401,8 +400,7 @@ public final class Fleetd { BackendErrorPatternLookup backendErrorPatterns = backendErrorPatternLookup(sessions::roster, errorPatternsByProfile); log.info("backend-error classification (fleetd #201 Unit 5): {}", - CompletionResolver.coverage("errorPattern", cfg.profiles().keySet(), - errorPatternsByProfile.keySet())); + errorPatternCoverageLine(cfg.profiles().keySet(), errorPatternsByProfile.keySet())); // CB-578 stage B: on a classification that actually wins, quarantine the exhausted profile's // CREDENTIAL — not the profile name — so a profile sharing that credential (e.g. two models // on one OpenAI account) is refused too, not just the one that happened to report it. Reads @@ -807,6 +805,41 @@ public final class Fleetd { }, quarantine, profile -> startupExhaustedPatterns.containsKey(profile)); } + /** + * fleetd #415 (review follow-up): package-private factory for the CB-578 stage A {@code + * exhaustedPattern} startup coverage line, paired explicitly with {@link + * CompletionResolver.UnsetMeaning#OFF} — {@code exhaustedPattern} has no fallback, so a + * profile with none configured really does have the classification off. + * + *

Extracted out of {@code main} for the same reason {@link #capacitySource} and {@link + * #worktreeBranchLookup} were: {@code coverage()}'s own tests ({@code CompletionResolverTest}) + * prove it words {@code OFF} and {@link CompletionResolver.UnsetMeaning#BUILT_IN_DEFAULT} + * correctly when a test supplies the meaning itself — they cannot prove {@code main} pairs the + * right meaning with the right key, which is the actual fleetd #415 defect. Measured: + * swapping the {@code UnsetMeaning} arguments between this method and {@link + * #errorPatternCoverageLine} — recreating #415's defect with the two keys exchanged — compiled + * with 0 errors and left all 1506 existing tests green before {@code + * FleetdPatternCoverageLineTest} was added to catch exactly that swap. + */ + static String exhaustedPatternCoverageLine(Set allProfiles, Set configuredProfiles) { + return CompletionResolver.coverage("exhaustedPattern", CompletionResolver.UnsetMeaning.OFF, + allProfiles, configuredProfiles); + } + + /** + * fleetd #415 (review follow-up): the {@code errorPattern} counterpart of {@link + * #exhaustedPatternCoverageLine}, paired explicitly with {@link + * CompletionResolver.UnsetMeaning#BUILT_IN_DEFAULT} — an unset {@code errorPattern} still runs + * backend-error classification against {@code CompletionResolver}'s built-in {@code + * BACKEND_ERROR} pattern, so the empty case is not "off". See {@link + * #exhaustedPatternCoverageLine}'s javadoc for the measured swap mutation this pairing guards + * against. + */ + static String errorPatternCoverageLine(Set allProfiles, Set configuredProfiles) { + return CompletionResolver.coverage("errorPattern", CompletionResolver.UnsetMeaning.BUILT_IN_DEFAULT, + allProfiles, configuredProfiles); + } + /** * fleetd #416: production source for {@code fleet_list}'s per-profile capacity facts. * diff --git a/fleetd/src/main/java/dev/ltms/fleet/inject/CompletionResolver.java b/fleetd/src/main/java/dev/ltms/fleet/inject/CompletionResolver.java index 7ad8406..d1faa05 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/inject/CompletionResolver.java +++ b/fleetd/src/main/java/dev/ltms/fleet/inject/CompletionResolver.java @@ -698,17 +698,57 @@ public final class CompletionResolver implements TurnListener { } /** - * Coverage summary for the CB-578 stage A exhausted-pattern classification, logged at startup + * What an unset pattern key means for the classification it configures (fleetd#415). + * {@code coverage()} cannot infer this from the key's name — the two keys it currently + * describes disagree on it, and a string comparison on the name would just move the same bug + * to a new spot — so every caller must state it explicitly. + * + *

This alone does not prove a caller passes the right one for its key. A + * test that calls {@code coverage()} directly and supplies the meaning itself only proves this + * enum is worded correctly, never that {@code Fleetd}'s two call sites pair each key with its + * true meaning — that pairing is #415's actual defect. Measured on review: swapping the two + * {@code UnsetMeaning} arguments at those call sites (giving {@code exhaustedPattern} the + * built-in-default wording and {@code errorPattern} the off wording — #415's exact defect with + * the keys exchanged) compiled with 0 errors and left all 1506 existing tests green. See + * {@code dev.ltms.fleet.Fleetd#exhaustedPatternCoverageLine}/{@code #errorPatternCoverageLine} + * and {@code FleetdPatternCoverageLineTest}, which exists specifically to catch that swap. + */ + public enum UnsetMeaning { + /** No fallback exists: a profile with no configured pattern truly has this classification off. */ + OFF, + /** A built-in pattern applies when unset: the classification still runs for that profile. */ + BUILT_IN_DEFAULT + } + + /** + * Coverage summary for a fleetd#201/CB-578-style pattern-key classification, logged at startup * the way {@link dev.ltms.fleet.health.FleetHealthMonitor#coverage} is — so an operator can * see whether the classification is on, and for which profiles, without reading every * profile's config by hand. * + *

fleetd#415: this method measures pattern coverage — how many profiles set the + * key — which is not the same thing as feature state for a key with a fallback. For + * {@code errorPattern}, an empty {@code configuredProfiles} still runs the classification + * against {@code CompletionResolver}'s built-in compatibility pattern ({@link #BACKEND_ERROR} + * at line ~84); for {@code exhaustedPattern} there is no fallback, so empty really does mean + * off. {@code unsetMeaning} is the single, required source of that fact — see + * {@link dev.ltms.fleet.config.FleetConfig#rejectMalformedProfilePatterns} lines ~2029-2032 for + * where it is documented for config authors. It is a required parameter, not a defaulted + * overload: a third pattern key added later must supply one to compile at all, rather than + * silently inheriting whichever wording this method happened to default to. + * * @param allProfiles every configured profile name - * @param configuredProfiles the subset of {@code allProfiles} that carry an exhausted pattern + * @param configuredProfiles the subset of {@code allProfiles} that carry the pattern */ - public static String coverage(String patternKey, Set allProfiles, Set configuredProfiles) { + public static String coverage(String patternKey, UnsetMeaning unsetMeaning, Set allProfiles, + Set configuredProfiles) { if (configuredProfiles.isEmpty()) { - return "off (no profile has an " + patternKey + " configured; profiles: " + sorted(allProfiles) + ")"; + return switch (unsetMeaning) { + case OFF -> "off (no profile has an " + patternKey + " configured; profiles: " + + sorted(allProfiles) + ")"; + case BUILT_IN_DEFAULT -> "built-in default for all profiles (no profile customises " + + patternKey + "; profiles: " + sorted(allProfiles) + ")"; + }; } Set unconfigured = new TreeSet<>(allProfiles); unconfigured.removeAll(configuredProfiles); diff --git a/fleetd/src/test/java/dev/ltms/fleet/FleetdPatternCoverageLineTest.java b/fleetd/src/test/java/dev/ltms/fleet/FleetdPatternCoverageLineTest.java new file mode 100644 index 0000000..fdc7edb --- /dev/null +++ b/fleetd/src/test/java/dev/ltms/fleet/FleetdPatternCoverageLineTest.java @@ -0,0 +1,67 @@ +package dev.ltms.fleet; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +import java.util.Set; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; + +/** + * fleetd #415 (review follow-up): {@code CompletionResolverTest} proves {@code coverage()} words + * {@code UnsetMeaning.OFF} and {@code UnsetMeaning.BUILT_IN_DEFAULT} correctly — but every one of + * those tests supplies the meaning itself. That proves the enum's wording, never that {@code + * Fleetd} pairs the right meaning with the right pattern key. That pairing is #415's actual + * defect: {@code coverage()} had no way to know what unset meant for its key, so the fix moved + * the fact to the caller — and nothing yet proved the caller states it correctly. + * + *

Measured or it didn't happen: swapping the two {@code UnsetMeaning} arguments at + * {@code Fleetd}'s two coverage call sites — giving {@code exhaustedPattern} the built-in-default + * wording and {@code errorPattern} the off wording, #415's exact defect with the keys exchanged — + * compiled with 0 errors and left all 1506 existing tests green. This class exists to turn that + * swap red. + * + *

It calls {@link Fleetd#exhaustedPatternCoverageLine} and {@link Fleetd#errorPatternCoverageLine} + * directly rather than reading {@code Fleetd.java} as source text (the shape {@code + * FleetdCompletionResolverWiringTest} uses for a different wiring gap): those two methods are the + * extracted call sites {@code main} actually invokes, following the same {@code static} factory + + * dedicated-test pattern as {@link Fleetd#capacitySource} and {@link Fleetd#worktreeBranchLookup}. + */ +class FleetdPatternCoverageLineTest { + + private static final Set PROFILES = Set.of("terra", "gx10"); + + @Test + @DisplayName("exhaustedPatternCoverageLine says off when no profile configures exhaustedPattern") + void exhaustedPatternCoverageLineSaysOffWhenNoProfileConfiguresIt() { + assertEquals("off (no profile has an exhaustedPattern configured; profiles: [gx10, terra])", + Fleetd.exhaustedPatternCoverageLine(PROFILES, Set.of())); + } + + @Test + @DisplayName("errorPatternCoverageLine says built-in default when no profile configures errorPattern") + void errorPatternCoverageLineSaysBuiltInDefaultWhenNoProfileConfiguresIt() { + assertEquals("built-in default for all profiles (no profile customises errorPattern; " + + "profiles: [gx10, terra])", + Fleetd.errorPatternCoverageLine(PROFILES, Set.of())); + } + + @Test + @DisplayName("the two keys produce different wording for the identical empty-coverage input") + void theTwoKeysProduceDifferentWordingForTheSameEmptyInput() { + String exhaustedLine = Fleetd.exhaustedPatternCoverageLine(PROFILES, Set.of()); + String errorLine = Fleetd.errorPatternCoverageLine(PROFILES, Set.of()); + + // Pinned individually above; restated here so this test alone still catches a swap even + // if one of the two tests above were ever deleted. + assertEquals("off (no profile has an exhaustedPattern configured; profiles: [gx10, terra])", + exhaustedLine); + assertEquals("built-in default for all profiles (no profile customises errorPattern; " + + "profiles: [gx10, terra])", errorLine); + assertNotEquals(exhaustedLine, errorLine, + "swapping which UnsetMeaning pairs with which pattern key at Fleetd's call sites " + + "must be caught here — that pairing, not coverage()'s own wording in isolation, " + + "is fleetd #415's actual defect"); + } +} diff --git a/fleetd/src/test/java/dev/ltms/fleet/inject/CompletionResolverTest.java b/fleetd/src/test/java/dev/ltms/fleet/inject/CompletionResolverTest.java index 1d17799..73e0b01 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/inject/CompletionResolverTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/inject/CompletionResolverTest.java @@ -20,6 +20,7 @@ import java.util.regex.Pattern; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertTrue; /** Unit behaviour of the CB-106 completion resolver in isolation from the injector. */ @@ -884,19 +885,22 @@ class CompletionResolverTest { @Test void coverageIsOffWhenNoProfileHasAPatternConfigured() { assertEquals("off (no profile has an exhaustedPattern configured; profiles: [terra])", - CompletionResolver.coverage("exhaustedPattern", Set.of("terra"), Set.of())); + CompletionResolver.coverage("exhaustedPattern", CompletionResolver.UnsetMeaning.OFF, + Set.of("terra"), Set.of())); } @Test void coverageIsFullWhenEveryProfileHasAPatternConfigured() { assertEquals("full (all profiles configured: [gx10, terra])", - CompletionResolver.coverage("exhaustedPattern", Set.of("terra", "gx10"), Set.of("terra", "gx10"))); + CompletionResolver.coverage("exhaustedPattern", CompletionResolver.UnsetMeaning.OFF, + Set.of("terra", "gx10"), Set.of("terra", "gx10"))); } @Test void coverageIsPartialAndNamesWhichProfilesAreConfigured() { assertEquals("partial (configured: [terra]; not configured: [gx10])", - CompletionResolver.coverage("exhaustedPattern", Set.of("terra", "gx10"), Set.of("terra"))); + CompletionResolver.coverage("exhaustedPattern", CompletionResolver.UnsetMeaning.OFF, + Set.of("terra", "gx10"), Set.of("terra"))); } /** @@ -910,11 +914,42 @@ class CompletionResolverTest { * *

Every earlier test here passed the exhaustion case only, so none of them could see it. This * one pins that the message names the key the caller actually meant. + * + *

fleetd#415: the expected wording changed here too. {@code errorPattern} has a built-in + * fallback ({@link CompletionResolver#BACKEND_ERROR}), so an empty {@code configuredProfiles} + * for it is not "off" — see {@link #coverageDistinguishesOffFromBuiltInDefaultForTheSameEmptyInput} + * for the test built specifically to pin that distinction. */ @Test void coverageNamesTheConfigKeyItsCallerMeansRatherThanAlwaysSayingExhaustedPattern() { - assertEquals("off (no profile has an errorPattern configured; profiles: [gx10, terra])", - CompletionResolver.coverage("errorPattern", Set.of("terra", "gx10"), Set.of())); + assertEquals("built-in default for all profiles (no profile customises errorPattern; " + + "profiles: [gx10, terra])", + CompletionResolver.coverage("errorPattern", CompletionResolver.UnsetMeaning.BUILT_IN_DEFAULT, + Set.of("terra", "gx10"), Set.of())); + } + + /** + * fleetd#415: {@code coverage()} measures pattern coverage (how many profiles set the key), but + * for {@code errorPattern} the empty case is not the feature-off state — a profile with no + * configured {@code errorPattern} still runs the classification against + * {@link CompletionResolver#BACKEND_ERROR}. For {@code exhaustedPattern} there is no fallback, + * so empty really is off. Same shape of input (empty {@code configuredProfiles}, one profile), + * different {@link CompletionResolver.UnsetMeaning} — the wording must differ, or this method is + * back to conflating pattern coverage with feature state for the one key where they disagree. + */ + @Test + void coverageDistinguishesOffFromBuiltInDefaultForTheSameEmptyInput() { + String exhaustedLine = CompletionResolver.coverage("exhaustedPattern", + CompletionResolver.UnsetMeaning.OFF, Set.of("gx10", "terra"), Set.of()); + String errorLine = CompletionResolver.coverage("errorPattern", + CompletionResolver.UnsetMeaning.BUILT_IN_DEFAULT, Set.of("gx10", "terra"), Set.of()); + + assertEquals("off (no profile has an exhaustedPattern configured; profiles: [gx10, terra])", + exhaustedLine); + assertEquals("built-in default for all profiles (no profile customises errorPattern; " + + "profiles: [gx10, terra])", errorLine); + assertNotEquals(exhaustedLine, errorLine, + "the same empty-coverage input must not read as the same feature state for both keys"); } // --- fleetd#201 Unit 1: target-keyed backend-error pattern + typed sink ----------------------