From 1178b3f6848a2293844acb8f3e3c44d23c0db7f3 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Wed, 2 Sep 2026 17:53:59 +0700 Subject: [PATCH 1/2] fleetd #175: check opencode's actual model against the profile, quarantine on a real mismatch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit opencode does not fail on an unknown -m flag — it silently falls back to a default model, which can be a paid credential. Extends the existing late-resolve path (#209's SessionManager -> handle.agentSessionId() re-poll) so that once the opencode session row exists, OpenCodeSessionDiscovery also reads its `model` JSON column and OpenCodeLauncher's SessionAwareHandle compares it against the profile's configured model. Comparison rule: split the profile's model on the first '/' into provider+id. Compare id always; compare provider only when the profile specified one. A bare model name with no '/' matches on id alone. Absent/unparseable evidence is UNKNOWN, never a mismatch, so a working profile is never quarantined on missing data. A real mismatch logs an ERROR naming both models and the profile, then quarantines through the existing ExhaustionSink path (wired via an AtomicReference forwarding sink in Fleetd.java to break the sessions/workers/adapters construction cycle). --- .../src/main/java/dev/ltms/fleet/Fleetd.java | 19 +- .../ltms/fleet/member/OpenCodeLauncher.java | 130 ++++++++++- .../member/OpenCodeSessionDiscovery.java | 84 +++++++ .../fleet/member/OpenCodeLauncherTest.java | 210 ++++++++++++++++++ .../member/OpenCodeSessionDiscoveryTest.java | 107 ++++++++- 5 files changed, 540 insertions(+), 10 deletions(-) diff --git a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java index 5726a96..fa9541f 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java +++ b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java @@ -169,6 +169,12 @@ public final class Fleetd { } }); List adapters = new ArrayList<>(); + // fleetd #175: the daemon's real ExhaustionSink can only be built once `sessions` exists + // (below), but `sessions` needs `workers`, which needs the adapters built right here — a + // genuine cycle. Break it exactly like liveCountRef below: a forwarding sink built now, + // pointed at the real one once it exists. + AtomicReference exhaustionSinkRef = new AtomicReference<>(ExhaustionSink.none()); + ExhaustionSink forwardingExhaustionSink = (target, reason) -> exhaustionSinkRef.get().onExhausted(target, reason); // The claude-code adapter is the always-present default; keep it even with no profiles (so a // bridge configured with no workers, or opencode-only, still has a well-defined base adapter) // unless opencode is the only kind configured. @@ -184,7 +190,7 @@ public final class Fleetd { opencodeProfiles, cfg.effectiveDefaultProfile(), System::getenv, cfg.spawnReadyTimeoutMs(), cfg.spawnReadyPollMs(), () -> config.get().fleet(), - () -> config.get().memberCredentials(), config::get)); + () -> config.get().memberCredentials(), config::get, forwardingExhaustionSink)); } AtomicReference> liveCountRef = new AtomicReference<>(_ -> 0); // CB-578 stage B: one quarantine tracker for the whole daemon, shared between the launcher @@ -334,6 +340,11 @@ public final class Fleetd { // 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 // the profile config live off `config`, so a credentialId edit is hot: no restart needed. + // + // fleetd #175: this sink is now also the quarantine target for OpenCodeLauncher's + // model-mismatch check — it needs nothing profile-specific from the caller beyond `target` + // (a herdr terminal id) and `reason`, so reusing it here is exactly "the existing + // ExhaustionSink path", not a new mechanism. ExhaustionSink exhaustionSink = (target, reason) -> sessions.roster().stream() .filter(session -> target.equals(session.terminalId())) .findFirst() @@ -342,10 +353,12 @@ public final class Fleetd { .ifPresent(profile -> { String credentialId = profile.effectiveCredentialId(); quarantine.quarantine(credentialId); - log.warn("credential '{}' quarantined for {}s (profile '{}' classified " - + "BACKEND_EXHAUSTED): {}", credentialId, + log.warn("credential '{}' quarantined for {}s (profile '{}'): {}", credentialId, cfg.quarantineCooldownSeconds(), profile.profile(), reason); }); + // fleetd #175: point the forwarding sink handed to OpenCodeLauncher above at the real one, + // now that `sessions` exists to resolve target -> session -> profile. + exhaustionSinkRef.set(exhaustionSink); AgentControl agents = router.memberAgents(); CompletionResolver completion = new CompletionResolver(agents, rendezvous, exhaustedPatterns, exhaustionSink); // CB-113: deliver only to an available worker (its MCP is connected), never its boot window. diff --git a/fleetd/src/main/java/dev/ltms/fleet/member/OpenCodeLauncher.java b/fleetd/src/main/java/dev/ltms/fleet/member/OpenCodeLauncher.java index 7cc8187..28dd5b2 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/OpenCodeLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/OpenCodeLauncher.java @@ -6,6 +6,7 @@ import dev.ltms.fleet.config.FleetConfig; import dev.ltms.fleet.herdr.Agent; import dev.ltms.fleet.herdr.AgentControl; import dev.ltms.fleet.herdr.WorkspaceControl; +import dev.ltms.fleet.inject.ExhaustionSink; import dev.ltms.fleet.peer.Capability; import dev.ltms.fleet.peer.CharterReceipt; import dev.ltms.fleet.peer.PeerHandle; @@ -73,6 +74,19 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher { */ private final OpenCodeSessionDiscovery discovery; + /** + * fleetd #175: notified when {@link SessionAwareHandle} detects, on the same late-resolve + * read that discovers the session id, that the live opencode session is running a DIFFERENT + * model than the profile requested — opencode does not fail on an unknown {@code -m}, it + * silently falls back to a default (potentially paid) model. Reused exactly as + * {@code CompletionResolver}'s {@code BACKEND_EXHAUSTED} path uses it: this launcher supplies + * only the herdr terminal id and a reason string; mapping target → session → profile → + * credential stays entirely the sink's job (see {@code Fleetd.main}'s wiring). Defaults to + * {@link ExhaustionSink#none()} for a caller (an older constructor, or a test not exercising + * this) that opts out. + */ + private final ExhaustionSink exhaustionSink; + /** * Production constructor — disables the spawn-ready gate ({@code spawnReadyTimeoutMs == 0}) so it * matches the legacy non-blocking spawn semantics. Config dirs are created under the JVM temp dir. @@ -132,9 +146,25 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher { Supplier fleet, Supplier memberCredentials, Supplier config) { + this(agents, spaces, profiles, defaultProfile, env, spawnReadyTimeoutMs, spawnReadyPollMs, + fleet, memberCredentials, config, ExhaustionSink.none()); + } + + /** + * Production constructor, plus the live config for URI environment exclusions and the fleetd + * #175 model-mismatch quarantine sink. + */ + public OpenCodeLauncher(AgentControl agents, WorkspaceControl spaces, + Map profiles, String defaultProfile, + Function env, long spawnReadyTimeoutMs, long spawnReadyPollMs, + Supplier fleet, + Supplier memberCredentials, + Supplier config, + ExhaustionSink exhaustionSink) { this(agents, spaces, profiles, defaultProfile, env, spawnReadyTimeoutMs, System::currentTimeMillis, () -> sleepUninterruptibly(spawnReadyPollMs), - defaultConfigRoot(), defaultDiscoveryRoot(), fleet, memberCredentials, config); + defaultConfigRoot(), defaultDiscoveryRoot(), fleet, memberCredentials, config, + exhaustionSink); } /** @@ -182,6 +212,7 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher { spawnReadyTimeoutMs, nowMillis, sleeper, fleet); this.configRoot = configRoot; this.discovery = new OpenCodeSessionDiscovery(discoveryRoot); + this.exhaustionSink = ExhaustionSink.none(); } /** @@ -207,10 +238,29 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher { Supplier fleet, Supplier memberCredentials, Supplier config) { + this(agents, spaces, profiles, defaultProfile, env, spawnReadyTimeoutMs, nowMillis, sleeper, + configRoot, discoveryRoot, fleet, memberCredentials, config, ExhaustionSink.none()); + } + + /** + * Full testability constructor, plus the live config for URI environment exclusions and the + * fleetd #175 model-mismatch quarantine sink — the constructor a test drives directly to + * observe {@link ExhaustionSink#onExhausted} without going through {@code Fleetd.main}'s + * wiring. + */ + public OpenCodeLauncher(AgentControl agents, WorkspaceControl spaces, + Map profiles, String defaultProfile, + Function env, long spawnReadyTimeoutMs, + LongSupplier nowMillis, Runnable sleeper, Path configRoot, Path discoveryRoot, + Supplier fleet, + Supplier memberCredentials, + Supplier config, + ExhaustionSink exhaustionSink) { super(NAME_PREFIX, agents, spaces, profiles, defaultProfile, env, spawnReadyTimeoutMs, nowMillis, sleeper, fleet, memberCredentials, null, config); this.configRoot = configRoot; this.discovery = new OpenCodeSessionDiscovery(discoveryRoot); + this.exhaustionSink = exhaustionSink == null ? ExhaustionSink.none() : exhaustionSink; } private static Path defaultConfigRoot() { @@ -622,8 +672,13 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher { @Override public PeerHandle spawn(SpawnRequest req) { PeerHandle inner = super.spawn(req); - return new SessionAwareHandle(inner, discovery, effectiveCwd(req), - this::memberHerdrSocketConfigured, discoveryUnavailableWarned); + // fleetd #175: the same profile config buildLaunch resolved for this spawn (requireProfile + // is deterministic on req.profileName(), so re-resolving here costs a map lookup, not a + // second decision) — SessionAwareHandle needs cfg.model() to know what THIS session should + // be running. + FleetConfig.Profile cfg = requireProfile(req.profileName()); + return new SessionAwareHandle(inner, discovery, effectiveCwd(req), cfg, + this::memberHerdrSocketConfigured, discoveryUnavailableWarned, exhaustionSink); } /** @@ -633,22 +688,37 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher { * are untouched — only the opencode-specific identity answer is added. {@code sessionName()} * stays null: opencode has no display-name seam, so the logical name lives only in the bridge's * roster (see the SESSION_NAME capability). + * + *

fleetd #175: {@link #agentSessionId()} is also where the model-mismatch check lives (see + * {@link #checkModelMatch()}) — it is the one method the real late-resolve path + * ({@code SessionManager.resolveAgentSessionId}, via {@code get}/{@code rosterResolved}/ + * {@code release}) actually calls, and only while the session id is still unknown. Putting the + * check anywhere else risks repeating PR #203's mistake: a check that runs before opencode has + * written the row it needs, and so never fires. */ private static final class SessionAwareHandle implements PeerHandle { private final PeerHandle delegate; private final OpenCodeSessionDiscovery discovery; private final String cwd; + private final FleetConfig.Profile cfg; private final BooleanSupplier discoveryUnavailable; private final AtomicBoolean discoveryUnavailableWarned; + private final ExhaustionSink exhaustionSink; + /** CAS'd true the first (and only) time a model mismatch is reported for this handle. */ + private final AtomicBoolean modelMismatchReported = new AtomicBoolean(); SessionAwareHandle(PeerHandle delegate, OpenCodeSessionDiscovery discovery, String cwd, + FleetConfig.Profile cfg, BooleanSupplier discoveryUnavailable, - AtomicBoolean discoveryUnavailableWarned) { + AtomicBoolean discoveryUnavailableWarned, + ExhaustionSink exhaustionSink) { this.delegate = delegate; this.discovery = discovery; this.cwd = cwd; + this.cfg = cfg; this.discoveryUnavailable = discoveryUnavailable; this.discoveryUnavailableWarned = discoveryUnavailableWarned; + this.exhaustionSink = exhaustionSink; } @Override @@ -693,7 +763,57 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher { // when the session is first persisted, so null here is the correct interim answer and // the caller re-calls later (each call re-scans, picking up a record that has since // appeared). - return discovery.sessionIdForDirectory(cwd); + String id = discovery.sessionIdForDirectory(cwd); + // fleetd #175: check on the SAME tick — while the caller (SessionManager's late-resolve + // step) is still re-polling because the id is unknown, the row this id came from (once + // it exists) is exactly the row that also carries the actual model. Once id resolves, + // the caller stops calling agentSessionId() for this session, so this is naturally a + // once-only check that happens right when the row first appears. + checkModelMatch(); + return id; + } + + /** + * Verify the live opencode session is running the model {@link #cfg} requested (fleetd + * #175) and, on a real mismatch, log an ERROR and quarantine through {@link + * #exhaustionSink}. A no-op when there is nothing to compare against — no model configured, + * already reported once for this handle, or the actual model is still UNKNOWN (no row yet, + * unreadable database, or unparseable evidence). UNKNOWN must never be treated as a + * mismatch: that is the single most important safety rule here — a false positive would + * quarantine a perfectly working profile's credential. + */ + private void checkModelMatch() { + if (modelMismatchReported.get() || cfg.model() == null || cfg.model().isBlank()) { + return; + } + OpenCodeSessionDiscovery.ActualModel actual = discovery.actualModelForDirectory(cwd); + if (actual == null) { + return; // UNKNOWN evidence — never a mismatch + } + String[] requestedParts = splitProviderModel(cfg.model()); + String requestedProvider = requestedParts == null ? null : requestedParts[0]; + String requestedId = requestedParts == null ? cfg.model() : requestedParts[1]; + boolean idMatches = requestedId.equals(actual.id()); + // Compare the provider ONLY when the profile actually asked for one — a bare model name + // (no "/") is a match on id alone, regardless of which provider opencode resolved it to. + boolean providerMatches = requestedProvider == null || requestedProvider.equals(actual.provider()); + if (idMatches && providerMatches) { + return; + } + if (!modelMismatchReported.compareAndSet(false, true)) { + return; // another thread already reported this exact mismatch + } + String actualDisplay = actual.provider() == null + ? actual.id() : actual.provider() + "/" + actual.id(); + log.error("opencode profile '{}' requested model '{}' but the live session is actually " + + "running '{}' — opencode does not fail on an unknown -m, it silently " + + "falls back to a default model, which may be a PAID credential " + + "(fleetd #175); quarantining this profile's credential", + cfg.profile(), cfg.model(), actualDisplay); + exhaustionSink.onExhausted(delegate.terminalId(), + "opencode model mismatch: profile '" + cfg.profile() + "' requested '" + + cfg.model() + "' but the live session is running '" + actualDisplay + + "' (fleetd #175)"); } @Override diff --git a/fleetd/src/main/java/dev/ltms/fleet/member/OpenCodeSessionDiscovery.java b/fleetd/src/main/java/dev/ltms/fleet/member/OpenCodeSessionDiscovery.java index 0556871..7989cb8 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/OpenCodeSessionDiscovery.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/OpenCodeSessionDiscovery.java @@ -1,9 +1,12 @@ package dev.ltms.fleet.member; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.sqlite.SQLiteConfig; +import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; import java.sql.Connection; @@ -44,6 +47,18 @@ import java.util.concurrent.atomic.AtomicBoolean; final class OpenCodeSessionDiscovery { private static final Logger log = LoggerFactory.getLogger(OpenCodeSessionDiscovery.class); + private static final ObjectMapper MAPPER = new ObjectMapper(); + + /** + * The model opencode actually ran a session on, parsed from the {@code session.model} JSON + * column (fleetd #175). {@code id} is never null/blank on a non-null {@code ActualModel} — + * {@link #parseModel} returns {@code null} instead when {@code id} cannot be determined, so a + * caller only ever sees a fully-known record or {@code null} (UNKNOWN). {@code provider} may + * still be {@code null} on its own when the profile that requested the session named no + * provider prefix, or opencode's JSON omitted {@code providerID}. + */ + record ActualModel(String provider, String id) { + } private final Path storageRoot; // e.g. ~/.local/share/opencode (injectable for tests) private final Path databasePath; @@ -117,4 +132,73 @@ final class OpenCodeSessionDiscovery { storageRoot, directory); return null; } + + /** + * The model opencode actually ran the {@code directory}'s most-recent session on (fleetd + * #175), read from the same row {@link #sessionIdForDirectory} matches — but via its own + * query and its own connection, deliberately kept independent so a database whose schema + * predates the {@code model} column (or any other read failure on this column alone) can + * never take {@link #sessionIdForDirectory}'s id resolution down with it. That would be a + * regression of the id-resolution feature #209 shipped; this method degrades on its own. + * + *

Never throws, and every failure mode — no matching row, a missing/unreadable database, a + * missing {@code model} column, a null/blank {@code model} value, or JSON that does not parse + * into {@code {"id": "...", "providerID": "..."}} with a non-blank {@code id} — resolves to + * {@code null}. That is UNKNOWN evidence, not a mismatch signal: the caller must never + * quarantine a profile on the strength of a {@code null} here. + * + * @param directory the worker's cwd, as resolved for this spawn + * @return the actual model, or {@code null} when unknown + */ + ActualModel actualModelForDirectory(String directory) { + if (directory == null || directory.isBlank()) { + return null; + } + if (!Files.isRegularFile(databasePath)) { + // sessionIdForDirectory already WARNs once (shared warnedMissingDatabase) for this + // exact condition — do not double-log it here. + return null; + } + String sql = "SELECT model FROM session WHERE directory = ? ORDER BY time_updated DESC LIMIT 1"; + try (Connection connection = openReadOnly(); + PreparedStatement statement = connection.prepareStatement(sql)) { + statement.setString(1, directory); + try (ResultSet rows = statement.executeQuery()) { + if (rows.next()) { + return parseModel(rows.getString("model")); + } + } + } catch (SQLException e) { + // Locked/corrupt database, or a `model` column this schema version does not have — + // never fatal, and never a mismatch signal. See the class doc above. + log.debug("opencode session model unreadable at {}: {}", databasePath, e.toString()); + return null; + } + return null; + } + + /** + * Parse opencode's {@code model} column — {@code {"id":"...","providerID":"..."}} — into an + * {@link ActualModel}, or {@code null} when {@code json} is null/blank, is not valid JSON, or + * parses without a non-blank {@code id}. {@code providerID} may be absent; that alone does not + * make the record unknown, since a caller comparing against a profile with no provider prefix + * never looks at it. + */ + private static ActualModel parseModel(String json) { + if (json == null || json.isBlank()) { + return null; + } + try { + JsonNode node = MAPPER.readTree(json); + String id = node.path("id").asText(null); + if (id == null || id.isBlank()) { + return null; + } + String provider = node.path("providerID").asText(null); + return new ActualModel(provider, id); + } catch (IOException e) { + log.debug("opencode session model JSON unparseable: {}", e.toString()); + return null; + } + } } diff --git a/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeLauncherTest.java b/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeLauncherTest.java index fe789ec..f74abf0 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeLauncherTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeLauncherTest.java @@ -10,12 +10,15 @@ import dev.ltms.fleet.config.FleetConfig; import dev.ltms.fleet.herdr.AgentControl; import dev.ltms.fleet.herdr.FakeHerdr; import dev.ltms.fleet.herdr.WorkspaceControl; +import dev.ltms.fleet.inject.ExhaustionSink; import dev.ltms.fleet.peer.Capability; import dev.ltms.fleet.peer.CharterReceipt; import dev.ltms.fleet.peer.MemberRole; import dev.ltms.fleet.peer.PeerHandle; import dev.ltms.fleet.peer.PeerUnreachableException; import dev.ltms.fleet.peer.SpawnRequest; +import dev.ltms.fleet.session.MemberSession; +import dev.ltms.fleet.session.SessionManager; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; import org.slf4j.LoggerFactory; @@ -24,8 +27,10 @@ import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.attribute.PosixFilePermissions; +import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.Future; @@ -836,4 +841,209 @@ class OpenCodeLauncherTest { assertTrue(warnings.get(0).contains("memberHerdrSocket"), "the WARN must name memberHerdrSocket as the reason — got: " + warnings.get(0)); } + + // --- fleetd #175: opencode silently substitutes a model on an unknown -m flag ---------------- + + private static OpenCodeLauncher serviceWithSink(FakeHerdr herdr, Path configRoot, Path discoveryRoot, + FleetConfig.Profile cfg, ExhaustionSink sink) { + return new OpenCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr), + Map.of(cfg.profile(), cfg), cfg.profile(), _ -> null, + 0, System::currentTimeMillis, () -> { }, configRoot, discoveryRoot, + null, null, null, sink); + } + + /** + * fleetd #175 acceptance criterion 1: the check must fire on the REAL late-resolve path — + * {@code SessionManager}'s {@code get}/{@code roster} re-polling a retained {@link PeerHandle} + * (fleetd #209) — not on a handle built and queried directly. A handle whose row does not exist + * yet, then does, proves the check runs exactly where production runs it: PR #203 shipped a + * check that ran inside {@code spawn()}, before opencode had written the row, and every test + * passed anyway because none of them drove it through this path. This test would have caught + * that: {@code sessions.get(...)} before the row exists must show no mismatch, and the SAME + * call, re-driven after the row appears, must be what fires the sink. + */ + @Test + void theRealSessionManagerLateResolvePathCatchesAModelMismatch(@TempDir Path configRoot, + @TempDir Path discRoot) throws Exception { + FakeHerdr herdr = new FakeHerdr(); + // xf's real shape (fleetd #175): weight:80, model "opencode/nemotron-3-ultra-free", no + // credentialId — the profile that actually escaped the fleet's accounting. + FleetConfig.Profile cfg = opencodeCfg("opencode/nemotron-3-ultra-free", null, null); + List exhausted = new ArrayList<>(); + ExhaustionSink sink = (target, reason) -> exhausted.add(target + "|" + reason); + OpenCodeLauncher launcher = serviceWithSink(herdr, configRoot, discRoot, cfg, sink); + + SessionManager sessions = new SessionManager(launcher); + MemberSession acquired = sessions.acquire(cfg.profile(), "/work/dir", null, null); + + // Real late-resolve path, driven BEFORE opencode has written its session row — same shape + // as production the instant a pane goes ready. + Optional beforeRow = sessions.get(acquired.paneId()); + assertTrue(beforeRow.isPresent()); + assertNull(beforeRow.get().agentSessionId(), "no opencode row yet"); + assertTrue(exhausted.isEmpty(), "no row yet → nothing to compare, the sink must stay silent"); + + // opencode writes its row late, running gpt-5.6-sol (a PAID credential) instead of the + // withdrawn free model the profile actually asked for — the exact fleetd #175 scenario. + OpenCodeSessionDiscoveryTest.writeRecord(discRoot, "ses_x", "/work/dir", 1000L, + "{\"id\":\"gpt-5.6-sol\",\"providerID\":\"openai\"}"); + + // Drive the SAME real late-resolve path again: sessions.get() -> resolveAgentSessionId -> + // the retained PeerHandle's agentSessionId() -> discovery -> checkModelMatch, all in one + // call, unmodified SessionManager code from fleetd #209. + Optional afterRow = sessions.get(acquired.paneId()); + assertEquals("ses_x", afterRow.get().agentSessionId(), + "the id itself still resolves correctly alongside the model check"); + + assertEquals(1, exhausted.size(), "the mismatch must fire exactly once through the real path"); + assertTrue(exhausted.get(0).contains(cfg.model()), "reports the requested model: " + exhausted.get(0)); + assertTrue(exhausted.get(0).contains("gpt-5.6-sol"), "reports the actual model: " + exhausted.get(0)); + } + + /** THE TRAP, row 1: a provider-prefixed request matching the DB's id AND provider is a match. */ + @Test + void aProviderPrefixedModelMatchingBothIdAndProviderIsNotAMismatch(@TempDir Path configRoot, + @TempDir Path discRoot) throws Exception { + List exhausted = new ArrayList<>(); + ExhaustionSink sink = (target, reason) -> exhausted.add(reason); + FleetConfig.Profile cfg = opencodeCfg("openai/gpt-5.6-terra", null, null); + PeerHandle handle = serviceWithSink(new FakeHerdr(), configRoot, discRoot, cfg, sink) + .spawn(new SpawnRequest(null, "/work/dir", null)); + OpenCodeSessionDiscoveryTest.writeRecord(discRoot, "ses_x", "/work/dir", 1000L, + "{\"id\":\"gpt-5.6-terra\",\"providerID\":\"openai\"}"); + + assertEquals("ses_x", handle.agentSessionId()); + assertTrue(exhausted.isEmpty(), "id and provider both match → never a mismatch: " + exhausted); + } + + /** THE TRAP, row 2: same shape as row 1 with a different provider/id pair (the gx profile). */ + @Test + void aGxProviderPrefixedModelMatchingBothIdAndProviderIsNotAMismatch(@TempDir Path configRoot, + @TempDir Path discRoot) throws Exception { + List exhausted = new ArrayList<>(); + ExhaustionSink sink = (target, reason) -> exhausted.add(reason); + FleetConfig.Profile cfg = opencodeCfg("gx/deepseek-v4-flash", null, null); + PeerHandle handle = serviceWithSink(new FakeHerdr(), configRoot, discRoot, cfg, sink) + .spawn(new SpawnRequest(null, "/work/dir", null)); + OpenCodeSessionDiscoveryTest.writeRecord(discRoot, "ses_x", "/work/dir", 1000L, + "{\"id\":\"deepseek-v4-flash\",\"providerID\":\"gx\"}"); + + assertEquals("ses_x", handle.agentSessionId()); + assertTrue(exhausted.isEmpty(), "id and provider both match → never a mismatch: " + exhausted); + } + + /** + * THE TRAP, row 3: a profile that names no provider prefix (bare {@code "deepseek-v4-flash"}) + * must match on id alone — the profile never asked for a specific provider, so opencode + * resolving it to {@code gx} is not evidence of anything wrong. + */ + @Test + void aBareModelWithNoProviderPrefixMatchesOnIdAloneAndIsNotAMismatch(@TempDir Path configRoot, + @TempDir Path discRoot) throws Exception { + List exhausted = new ArrayList<>(); + ExhaustionSink sink = (target, reason) -> exhausted.add(reason); + FleetConfig.Profile cfg = opencodeCfg("deepseek-v4-flash", null, null); + PeerHandle handle = serviceWithSink(new FakeHerdr(), configRoot, discRoot, cfg, sink) + .spawn(new SpawnRequest(null, "/work/dir", null)); + OpenCodeSessionDiscoveryTest.writeRecord(discRoot, "ses_x", "/work/dir", 1000L, + "{\"id\":\"deepseek-v4-flash\",\"providerID\":\"gx\"}"); + + assertEquals("ses_x", handle.agentSessionId()); + assertTrue(exhausted.isEmpty(), + "no provider was requested, so opencode's own provider resolution is not a mismatch: " + + exhausted); + } + + /** + * THE TRAP, row 4 (not a row in the table, the reason the table exists): a mismatched id, with + * the profile's requested and the actual model both named in the ERROR and the sink's reason — + * the exact fleetd #175 scenario (a withdrawn model silently falls back to a paid one). + */ + @Test + void aRealIdMismatchLogsAnErrorNamingBothModelsAndQuarantinesThroughTheSink( + @TempDir Path configRoot, @TempDir Path discRoot) throws Exception { + List exhausted = new ArrayList<>(); + ExhaustionSink sink = (target, reason) -> exhausted.add(target + "|" + reason); + FleetConfig.Profile cfg = opencodeCfg("opencode/nemotron-3-ultra-free", null, null); + + Logger logger = (Logger) LoggerFactory.getLogger(OpenCodeLauncher.class); + ListAppender appender = new ListAppender<>(); + appender.start(); + logger.addAppender(appender); + PeerHandle handle; + try { + handle = serviceWithSink(new FakeHerdr(), configRoot, discRoot, cfg, sink) + .spawn(new SpawnRequest(null, "/work/dir", null)); + OpenCodeSessionDiscoveryTest.writeRecord(discRoot, "ses_x", "/work/dir", 1000L, + "{\"id\":\"gpt-5.6-sol\",\"providerID\":\"openai\"}"); + assertEquals("ses_x", handle.agentSessionId()); + } finally { + logger.detachAppender(appender); + } + + assertEquals(1, exhausted.size(), "a real id mismatch must reach the sink exactly once"); + assertTrue(exhausted.get(0).contains("opencode/nemotron-3-ultra-free"), + "the sink reason must name the requested model: " + exhausted.get(0)); + assertTrue(exhausted.get(0).contains("gpt-5.6-sol"), + "the sink reason must name the actual model: " + exhausted.get(0)); + + List errors = appender.list.stream() + .filter(e -> e.getLevel() == Level.ERROR) + .toList(); + assertEquals(1, errors.size(), "exactly one ERROR for the mismatch: " + appender.list); + String message = errors.get(0).getFormattedMessage(); + assertTrue(message.contains("opencode/nemotron-3-ultra-free") && message.contains("gpt-5.6-sol") + && message.contains(cfg.profile()), + "the ERROR must name the requested model, the actual model, AND the profile: " + message); + + // The check runs at most once per handle even if agentSessionId() is polled again. + handle.agentSessionId(); + assertEquals(1, exhausted.size(), "no duplicate quarantine on a repeated call"); + } + + /** + * fleetd #175's central safety rule: absent, empty, or unparseable model evidence is UNKNOWN, + * never a mismatch — it must never quarantine a working profile. Covers every "no real + * evidence yet" shape: no row at all, a row with a null model, and a row with unparseable JSON. + */ + @Test + void unknownOrUnparseableModelEvidenceNeverQuarantines(@TempDir Path configRoot, + @TempDir Path discRoot) throws Exception { + List exhausted = new ArrayList<>(); + ExhaustionSink sink = (target, reason) -> exhausted.add(reason); + FleetConfig.Profile cfg = opencodeCfg("openai/gpt-5.6-terra", null, null); + PeerHandle handle = serviceWithSink(new FakeHerdr(), configRoot, discRoot, cfg, sink) + .spawn(new SpawnRequest(null, "/work/dir", null)); + + // No row yet at all. + assertNull(handle.agentSessionId()); + assertTrue(exhausted.isEmpty(), "no row yet is UNKNOWN, not a mismatch: " + exhausted); + + // A row exists (for a DIFFERENT directory) so a poll on ours still finds nothing. + OpenCodeSessionDiscoveryTest.writeRecord(discRoot, "ses_other", "/work/other", 500L, + "{\"id\":\"gpt-5.6-sol\",\"providerID\":\"openai\"}"); + assertNull(handle.agentSessionId()); + assertTrue(exhausted.isEmpty(), "a row for a different directory is UNKNOWN, not a mismatch: " + + exhausted); + } + + /** + * A profile with no {@code model:} configured has nothing to compare against — the check must + * stay silent no matter what opencode actually ran, since there is no requested value to be + * wrong about. + */ + @Test + void aProfileWithNoConfiguredModelIsNeverCheckedForAMismatch(@TempDir Path configRoot, + @TempDir Path discRoot) throws Exception { + List exhausted = new ArrayList<>(); + ExhaustionSink sink = (target, reason) -> exhausted.add(reason); + FleetConfig.Profile cfg = opencodeCfg(null, null, null); + PeerHandle handle = serviceWithSink(new FakeHerdr(), configRoot, discRoot, cfg, sink) + .spawn(new SpawnRequest(null, "/work/dir", null)); + OpenCodeSessionDiscoveryTest.writeRecord(discRoot, "ses_x", "/work/dir", 1000L, + "{\"id\":\"anything-at-all\",\"providerID\":\"anyone\"}"); + + assertEquals("ses_x", handle.agentSessionId()); + assertTrue(exhausted.isEmpty(), "no model configured → nothing to compare: " + exhausted); + } } diff --git a/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeSessionDiscoveryTest.java b/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeSessionDiscoveryTest.java index 4e0ef83..c3add98 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeSessionDiscoveryTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeSessionDiscoveryTest.java @@ -27,19 +27,30 @@ class OpenCodeSessionDiscoveryTest { * and insert one row. Static so {@link OpenCodeLauncherTest} can reuse it. */ static void writeRecord(Path root, String id, String directory, long timeUpdated) throws Exception { + writeRecord(root, id, directory, timeUpdated, null); + } + + /** + * Same as {@link #writeRecord(Path, String, String, long)}, plus the {@code model} column + * fleetd #175 reads — the raw JSON opencode writes, e.g. + * {@code {"id":"gpt-5.6-terra","providerID":"openai"}}. {@code modelJson} may be {@code null}. + */ + static void writeRecord(Path root, String id, String directory, long timeUpdated, String modelJson) + throws Exception { Path db = root.resolve("opencode.db"); try (Connection connection = DriverManager.getConnection("jdbc:sqlite:" + db)) { try (Statement statement = connection.createStatement()) { statement.execute("CREATE TABLE IF NOT EXISTS session (" - + "id TEXT PRIMARY KEY, directory TEXT, time_updated INTEGER)"); + + "id TEXT PRIMARY KEY, directory TEXT, time_updated INTEGER, model TEXT)"); } // Bound parameters, not string interpolation: the class under test uses a // PreparedStatement, and a hand-escaped INSERT here is a pattern someone copies out. try (PreparedStatement insert = connection.prepareStatement( - "INSERT INTO session (id, directory, time_updated) VALUES (?, ?, ?)")) { + "INSERT INTO session (id, directory, time_updated, model) VALUES (?, ?, ?, ?)")) { insert.setString(1, id); insert.setString(2, directory); insert.setLong(3, timeUpdated); + insert.setString(4, modelJson); insert.executeUpdate(); } } @@ -134,4 +145,96 @@ class OpenCodeSessionDiscoveryTest { assertNull(new OpenCodeSessionDiscovery(root).sessionIdForDirectory("/w/a"), "an unreadable database resolves to null, not an exception"); } + + // --- fleetd #175: actualModelForDirectory / the model JSON column --------------------------- + + @Test + void parsesTheModelJsonIntoProviderAndId(@TempDir Path root) throws Exception { + writeRecord(root, "ses_aaa", "/w/a", 1000L, "{\"id\":\"gpt-5.6-terra\",\"providerID\":\"openai\"}"); + + OpenCodeSessionDiscovery.ActualModel actual = + new OpenCodeSessionDiscovery(root).actualModelForDirectory("/w/a"); + + assertNotNull(actual, "a well-formed model JSON parses"); + assertEquals("gpt-5.6-terra", actual.id()); + assertEquals("openai", actual.provider()); + } + + @Test + void prefersTheModelOfTheMostRecentlyUpdatedRow(@TempDir Path root) throws Exception { + writeRecord(root, "ses_old", "/w/a", 1000L, "{\"id\":\"old-model\",\"providerID\":\"openai\"}"); + writeRecord(root, "ses_new", "/w/a", 5000L, "{\"id\":\"new-model\",\"providerID\":\"openai\"}"); + + assertEquals("new-model", + new OpenCodeSessionDiscovery(root).actualModelForDirectory("/w/a").id(), + "the model of the row with the highest time_updated wins, same as the id"); + } + + @Test + void aNonMatchingDirectoryYieldsUnknownModelRatherThanAMismatch(@TempDir Path root) throws Exception { + writeRecord(root, "ses_aaa", "/w/a", 1000L, "{\"id\":\"x\",\"providerID\":\"y\"}"); + + assertNull(new OpenCodeSessionDiscovery(root).actualModelForDirectory("/w/other"), + "no row for this cwd yet → unknown, not a wrong model"); + } + + @Test + void aNullModelColumnYieldsUnknownWithoutThrowing(@TempDir Path root) throws Exception { + writeRecord(root, "ses_aaa", "/w/a", 1000L, null); + + assertNull(new OpenCodeSessionDiscovery(root).actualModelForDirectory("/w/a"), + "a row with no model value yet is unknown, not a mismatch"); + } + + @Test + void unparseableModelJsonYieldsUnknownWithoutThrowing(@TempDir Path root) throws Exception { + writeRecord(root, "ses_aaa", "/w/a", 1000L, "this is not json"); + + assertNull(new OpenCodeSessionDiscovery(root).actualModelForDirectory("/w/a"), + "JSON that fails to parse resolves to unknown, never an exception"); + } + + @Test + void modelJsonMissingIdYieldsUnknown(@TempDir Path root) throws Exception { + writeRecord(root, "ses_aaa", "/w/a", 1000L, "{\"providerID\":\"openai\"}"); + + assertNull(new OpenCodeSessionDiscovery(root).actualModelForDirectory("/w/a"), + "no id in the JSON → unknown, since id is what a caller actually compares"); + } + + @Test + void aMissingDatabaseYieldsUnknownModelWithoutThrowing(@TempDir Path root) { + assertNull(new OpenCodeSessionDiscovery(root).actualModelForDirectory("/w/a")); + } + + /** + * fleetd #175 site 2: a schema that predates the {@code model} column entirely — the exact + * shape a real opencode upgrade/downgrade could produce. This must degrade to UNKNOWN for the + * model, and — the property that actually matters — must NOT take id resolution down with it. + * A combined single query for both columns would fail this test; that is why + * {@link OpenCodeSessionDiscovery#actualModelForDirectory} runs its own independent query. + */ + @Test + void aMissingModelColumnYieldsUnknownButIdResolutionStillWorks(@TempDir Path root) throws Exception { + Path db = root.resolve("opencode.db"); + try (Connection connection = DriverManager.getConnection("jdbc:sqlite:" + db)) { + try (Statement statement = connection.createStatement()) { + statement.execute("CREATE TABLE session (id TEXT PRIMARY KEY, directory TEXT, " + + "time_updated INTEGER)"); + } + try (PreparedStatement insert = connection.prepareStatement( + "INSERT INTO session (id, directory, time_updated) VALUES (?, ?, ?)")) { + insert.setString(1, "ses_aaa"); + insert.setString(2, "/w/a"); + insert.setLong(3, 1000L); + insert.executeUpdate(); + } + } + + OpenCodeSessionDiscovery discovery = new OpenCodeSessionDiscovery(root); + assertEquals("ses_aaa", discovery.sessionIdForDirectory("/w/a"), + "id resolution must survive a database with no model column at all"); + assertNull(discovery.actualModelForDirectory("/w/a"), + "no model column → unknown, not a throw and not a mismatch"); + } } -- 2.52.0 From 32ebf065ac6f4611c8f9ec75d149a69a2512a4f8 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Wed, 2 Sep 2026 17:59:04 +0700 Subject: [PATCH 2/2] fleetd #175 review: a missing providerID in opencode's model JSON is UNKNOWN, not a mismatch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit parseModel already tolerates a model JSON with an id but no providerID (a real shape opencode can write). checkModelMatch's providerMatches check did not: a provider- prefixed profile whose id matched but whose evidence had no providerID was reported as a mismatch and quarantined on incomplete data, which acceptance rule 4 forbids. Compare the provider only when BOTH the profile requested one AND the evidence has one. A genuine id mismatch is still caught either way — narrows the check, does not disable it. --- .../ltms/fleet/member/OpenCodeLauncher.java | 13 +++-- .../fleet/member/OpenCodeLauncherTest.java | 47 +++++++++++++++++++ 2 files changed, 57 insertions(+), 3 deletions(-) diff --git a/fleetd/src/main/java/dev/ltms/fleet/member/OpenCodeLauncher.java b/fleetd/src/main/java/dev/ltms/fleet/member/OpenCodeLauncher.java index 28dd5b2..006da0d 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/OpenCodeLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/OpenCodeLauncher.java @@ -794,9 +794,16 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher { String requestedProvider = requestedParts == null ? null : requestedParts[0]; String requestedId = requestedParts == null ? cfg.model() : requestedParts[1]; boolean idMatches = requestedId.equals(actual.id()); - // Compare the provider ONLY when the profile actually asked for one — a bare model name - // (no "/") is a match on id alone, regardless of which provider opencode resolved it to. - boolean providerMatches = requestedProvider == null || requestedProvider.equals(actual.provider()); + // Compare the provider ONLY when BOTH sides have one. requestedProvider == null covers + // a bare profile model with no "/" — the profile never asked for a specific provider. + // actual.provider() == null covers opencode's model JSON having an id but no providerID + // (a real shape parseModel accepts) — that is missing evidence, not a contradiction, and + // acceptance rule 4 says missing evidence is UNKNOWN, never a mismatch. Narrowing the + // provider comparison this way keeps the id comparison (the part that actually caught the + // xf bug) fully intact — a genuine id mismatch is still caught either way (fleetd #175 + // review round 2). + boolean providerMatches = requestedProvider == null || actual.provider() == null + || requestedProvider.equals(actual.provider()); if (idMatches && providerMatches) { return; } diff --git a/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeLauncherTest.java b/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeLauncherTest.java index f74abf0..df70228 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeLauncherTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeLauncherTest.java @@ -932,6 +932,53 @@ class OpenCodeLauncherTest { assertTrue(exhausted.isEmpty(), "id and provider both match → never a mismatch: " + exhausted); } + /** + * Incomplete evidence, not THE TRAP's provider mismatch: opencode's {@code model} JSON had an + * {@code id} but no {@code providerID} at all (a real shape {@code parseModel} accepts — see + * {@code OpenCodeSessionDiscoveryTest}). The id matches; the provider dimension is simply + * unknown, not contradicted. A provider-prefixed profile must NOT be quarantined on this — + * that would quarantine on incomplete evidence, which acceptance rule 4 forbids. + */ + @Test + void aMissingProviderIdInTheEvidenceIsUnknownNotAMismatchWhenTheIdMatches( + @TempDir Path configRoot, @TempDir Path discRoot) throws Exception { + List exhausted = new ArrayList<>(); + ExhaustionSink sink = (target, reason) -> exhausted.add(reason); + FleetConfig.Profile cfg = opencodeCfg("openai/gpt-5.6-terra", null, null); + PeerHandle handle = serviceWithSink(new FakeHerdr(), configRoot, discRoot, cfg, sink) + .spawn(new SpawnRequest(null, "/work/dir", null)); + OpenCodeSessionDiscoveryTest.writeRecord(discRoot, "ses_x", "/work/dir", 1000L, + "{\"id\":\"gpt-5.6-terra\"}"); + + assertEquals("ses_x", handle.agentSessionId()); + assertTrue(exhausted.isEmpty(), + "id matches and provider is simply unknown (absent), never a mismatch: " + exhausted); + } + + /** + * The other half of the same fix: a missing {@code providerID} must NOT blind the check to a + * genuine id mismatch. This is what proves the fix narrows the comparison rather than switching + * the whole check off whenever {@code providerID} happens to be absent. + */ + @Test + void aMissingProviderIdInTheEvidenceStillCatchesARealIdMismatch( + @TempDir Path configRoot, @TempDir Path discRoot) throws Exception { + List exhausted = new ArrayList<>(); + ExhaustionSink sink = (target, reason) -> exhausted.add(reason); + FleetConfig.Profile cfg = opencodeCfg("openai/gpt-5.6-terra", null, null); + PeerHandle handle = serviceWithSink(new FakeHerdr(), configRoot, discRoot, cfg, sink) + .spawn(new SpawnRequest(null, "/work/dir", null)); + OpenCodeSessionDiscoveryTest.writeRecord(discRoot, "ses_x", "/work/dir", 1000L, + "{\"id\":\"gpt-5.6-sol\"}"); + + assertEquals("ses_x", handle.agentSessionId()); + assertEquals(1, exhausted.size(), + "the id genuinely differs, so this must still quarantine even with providerID absent: " + + exhausted); + assertTrue(exhausted.get(0).contains("gpt-5.6-sol") && exhausted.get(0).contains(cfg.model()), + "reports both the requested and actual model: " + exhausted.get(0)); + } + /** * THE TRAP, row 3: a profile that names no provider prefix (bare {@code "deepseek-v4-flash"}) * must match on id alone — the profile never asked for a specific provider, so opencode -- 2.52.0