CB-175: quarantine opencode model mismatches #203

2026-08-31T09:05:04+02:00

agent commented

2026-08-31 09:05:04 +02:00

Reads opencode's resolved model after spawn readiness and permanently quarantines a mismatch through the existing credential quarantine path. Adds match, mismatch, and unknown-storage tests.

Tests: mvn clean install (1040 tests, success).

Reads opencode's resolved model after spawn readiness and permanently quarantines a mismatch through the existing credential quarantine path. Adds match, mismatch, and unknown-storage tests. Tests: mvn clean install (1040 tests, success).

agent added 1 commit 2026-08-31 09:05:04 +02:00

CB-175: quarantine opencode model mismatches

CI / contract (pull_request) Successful in 58s

Details

CI / build (pull_request) Successful in 2m7s

Details

fc2e26c0c5

ltms referenced this pull request

2026-08-31 09:26:10 +02:00

opencode session discovery never finds a member's record, so agentSessionId is always null and resume silently does nothing #206

ltms commented

2026-08-31 09:26:36 +02:00

Holding this one — not merging yet. The code is well built, but I believe the check can never fire in production. That is not a criticism of the work: you flagged that you could not test end to end, and this is exactly the gap that opens.

The problem

verifyResolvedModel is called from spawn(), and returns silently when the session record is absent:

OpenCodeSessionDiscovery.SessionRecord record = discovery.sessionForDirectory(cwd);
if (record == null || record.providerId() == null || record.modelId() == null) {
    return;
}

The record is not there at spawn time. The existing code in the same file says so:

Lazy + retried, never a spawn-time blocker: opencode writes the session record only when the session is first persisted, so null here is the correct interim answer and the caller re-calls later

agentSessionId() is deliberately lazy and retried for that reason. verifyResolvedModel reads once, at the earliest possible moment, and treats absence as "unknown" — which your design correctly makes a silent no-op. Together those give a check that never runs.

Then I checked whether the record ever appears, and it does not

defaultDiscoveryRoot() is ~/.local/share/opencode. Four terra members (kind: opencode) ran today between about 08:20 and 09:40. Newest record anywhere under that root:

$ find ~/.local/share/opencode/storage -name 'ses_*.json' | xargs stat -f '%Sm %N' -t '%m-%d %H:%M' | sort -r | head -1
01-22 14:39  .../storage/session/global/ses_41b79fc90ffeI9E8uZv6VprUn2.json

22 January. Corroborated by fleet_list, which never reported agentSessionId for any opencode member today.

So the guard is not merely early — the evidence it waits for never arrives. I have filed that as #206, because it is a pre-existing defect in its own right: it also means resumeSessionId silently does nothing for every opencode profile.

What is genuinely good here, and which I want to keep

Reusing ExhaustionSink and BackendQuarantine rather than inventing a second tracker.
Choosing a permanent quarantine. Your reasoning is right: a withdrawn model selector cannot heal on a retry, so a cooldown would just reopen a paid fallback.
Spotting that toSecondsRoundedUp's (nanos + 999_999_999L) / 1_000_000_000L overflows to a negative at Long.MAX_VALUE. That is a real bug your change had to fix to work at all, and it was well caught.
Treating a missing or unreadable record as unknown rather than as a mismatch. Correct instinct — a false quarantine takes working capacity offline. It is only the combination with a single early read that makes the check dead.

What I want changed

#206 first. Until discovery works, no read-back strategy can work.
Do not read once at spawn. Verify at the point where evidence actually exists — the same lazy, retried place agentSessionId() uses, or on first turn completion.
Make "still unknown" visible. A check that silently does nothing is the same failure class this ticket is about: something that looks like it is protecting you and is not. If the record has not appeared by the time the member has done a turn, log it once, at the profile level.

I have left #175 open. When #206 lands, this becomes straightforward.

One thing I did not verify: whether an xf-style withdrawn-model spawn would even reach the readiness gate. Worth knowing, but it does not change the above.

Holding this one — **not** merging yet. The code is well built, but I believe the check can never fire in production. That is not a criticism of the work: you flagged that you could not test end to end, and this is exactly the gap that opens. ## The problem `verifyResolvedModel` is called from `spawn()`, and returns silently when the session record is absent: ```java OpenCodeSessionDiscovery.SessionRecord record = discovery.sessionForDirectory(cwd); if (record == null || record.providerId() == null || record.modelId() == null) { return; } ``` The record is not there at spawn time. The existing code in the same file says so: > Lazy + retried, never a spawn-time blocker: opencode writes the session record **only when the session is first persisted**, so null here is the correct interim answer and the caller re-calls later `agentSessionId()` is deliberately lazy and retried for that reason. `verifyResolvedModel` reads once, at the earliest possible moment, and treats absence as "unknown" — which your design correctly makes a silent no-op. Together those give a check that never runs. ## Then I checked whether the record ever appears, and it does not `defaultDiscoveryRoot()` is `~/.local/share/opencode`. Four `terra` members (`kind: opencode`) ran today between about 08:20 and 09:40. Newest record anywhere under that root: ``` $ find ~/.local/share/opencode/storage -name 'ses_*.json' | xargs stat -f '%Sm %N' -t '%m-%d %H:%M' | sort -r | head -1 01-22 14:39 .../storage/session/global/ses_41b79fc90ffeI9E8uZv6VprUn2.json ``` **22 January.** Corroborated by `fleet_list`, which never reported `agentSessionId` for any opencode member today. So the guard is not merely early — the evidence it waits for never arrives. I have filed that as **#206**, because it is a pre-existing defect in its own right: it also means `resumeSessionId` silently does nothing for every opencode profile. ## What is genuinely good here, and which I want to keep - Reusing `ExhaustionSink` and `BackendQuarantine` rather than inventing a second tracker. - **Choosing a permanent quarantine.** Your reasoning is right: a withdrawn model selector cannot heal on a retry, so a cooldown would just reopen a paid fallback. - Spotting that `toSecondsRoundedUp`'s `(nanos + 999_999_999L) / 1_000_000_000L` **overflows to a negative** at `Long.MAX_VALUE`. That is a real bug your change had to fix to work at all, and it was well caught. - Treating a missing or unreadable record as unknown rather than as a mismatch. Correct instinct — a false quarantine takes working capacity offline. It is only the *combination* with a single early read that makes the check dead. ## What I want changed 1. **#206 first.** Until discovery works, no read-back strategy can work. 2. **Do not read once at spawn.** Verify at the point where evidence actually exists — the same lazy, retried place `agentSessionId()` uses, or on first turn completion. 3. **Make "still unknown" visible.** A check that silently does nothing is the same failure class this ticket is about: something that looks like it is protecting you and is not. If the record has not appeared by the time the member has done a turn, log it once, at the profile level. I have left #175 open. When #206 lands, this becomes straightforward. One thing I did not verify: whether an `xf`-style withdrawn-model spawn would even reach the readiness gate. Worth knowing, but it does not change the above.

agent referenced this pull request

2026-08-31 10:53:00 +02:00

fleetd #206: read opencode sessions from opencode.db instead of the frozen JSON tree #207

ltms commented

2026-08-31 17:00:34 +02:00

Closing this as superseded. Two independent reasons, neither of which is a criticism of the work — the code reads correctly and the tests are real.

1. The guard can never fire, by construction. verifyResolvedModel(...) is called from inside spawn(), on the line right after super.spawn(req):

public PeerHandle spawn(SpawnRequest req) {
    PeerHandle inner = super.spawn(req);
    verifyResolvedModel(requireProfile(req.profileName()), effectiveCwd(req));
    return new SessionAwareHandle(inner, discovery, effectiveCwd(req));
}

opencode has not written its session row at that point — that is the asynchrony #206 established and the same one the neighbouring SessionAwareHandle.agentSessionId() javadoc describes. So record is always null, and record == null → return is silent by design. The tests pass because they inject the record, so it always exists there.

This is the shape to watch for: when a guard's evidence is produced asynchronously, ask when the evidence exists, not just whether the code reads it correctly.

2. The branch no longer applies. OpenCodeSessionDiscovery was rewritten in #207 (merged in a55079a) to query opencode.db over SQLite. The JSON-tree implementation this PR extends with sessionForDirectory/SessionRecord is gone, so this is a rebase onto a data source that no longer exists rather than a merge conflict.

What a working version needs. #175 stays open. The read-back must happen after the row appears, not at spawn. #209 is adding exactly that machinery — a retained PeerHandle plus a resolve step that re-reads while the answer is still unknown, driven from roster()/find(). Once that lands, the model check has a real place to live: the same resolve step, reading provider/model out of the same row it is already opening. The ExhaustionSink quarantine wiring and the "unknown evidence must not quarantine a working profile" rule from this PR are both right and should be carried over.

Thanks — the quarantine path and the unknown-evidence handling are the parts worth keeping.

Closing this as superseded. Two independent reasons, neither of which is a criticism of the work — the code reads correctly and the tests are real. **1. The guard can never fire, by construction.** `verifyResolvedModel(...)` is called from inside `spawn()`, on the line right after `super.spawn(req)`: ```java public PeerHandle spawn(SpawnRequest req) { PeerHandle inner = super.spawn(req); verifyResolvedModel(requireProfile(req.profileName()), effectiveCwd(req)); return new SessionAwareHandle(inner, discovery, effectiveCwd(req)); } ``` opencode has not written its session row at that point — that is the asynchrony #206 established and the same one the neighbouring `SessionAwareHandle.agentSessionId()` javadoc describes. So `record` is always null, and `record == null → return` is silent by design. The tests pass because they inject the record, so it always exists there. This is the shape to watch for: **when a guard's evidence is produced asynchronously, ask *when* the evidence exists, not just whether the code reads it correctly.** **2. The branch no longer applies.** `OpenCodeSessionDiscovery` was rewritten in #207 (merged in `a55079a`) to query `opencode.db` over SQLite. The JSON-tree implementation this PR extends with `sessionForDirectory`/`SessionRecord` is gone, so this is a rebase onto a data source that no longer exists rather than a merge conflict. **What a working version needs.** #175 stays open. The read-back must happen *after* the row appears, not at spawn. #209 is adding exactly that machinery — a retained `PeerHandle` plus a resolve step that re-reads while the answer is still unknown, driven from `roster()`/`find()`. Once that lands, the model check has a real place to live: the same resolve step, reading provider/model out of the same row it is already opening. The `ExhaustionSink` quarantine wiring and the "unknown evidence must not quarantine a working profile" rule from this PR are both right and should be carried over. Thanks — the quarantine path and the unknown-evidence handling are the parts worth keeping.

ltms closed this pull request

2026-08-31 17:00:46 +02:00

ltms referenced this pull request

2026-08-31 17:01:07 +02:00

fleetd asks opencode for a model and never checks it got that model, so a withdrawn name silently bills a paid credential #175

agent referenced this pull request

2026-09-02 12:54:45 +02:00

fleetd #175: check opencode's actual model against the profile, quarantine on a real mismatch #231

ltms referenced this pull request

2026-09-02 13:00:53 +02:00

autoCompactWindow is set by flag and never read back — the #175 shape in a second place #232

ltms referenced this issue from a commit