From cfae8a0d68e115bcbca056b416b4843fbfe707a8 Mon Sep 17 00:00:00 2001 From: Artjoms Stukans Date: Tue, 22 Sep 2026 21:01:11 +0200 Subject: [PATCH 1/7] Bake the models a harness can run into the agent image Found by the operator testing part B: the build setup offered every enabled model in the LLM catalogue, filtered by nothing, so Codex was offered Claude and Gemini models. The backend agreed and was equally wrong -- BuildDefaults.save checks that a model is enabled and priced, and never asks whether the harness can run it. So codex with claude-opus-5 saves, and dies when the run starts: after an approval, mid-item, which is the failure this milestone exists to remove. Measured the same day in the pinned image. The catalogue is not a weaker version of the harness's list, it is a different list: the reference image's Codex knows eight models, the catalogue offers six for OpenAI, and they have TWO in common. Filtering the catalogue by vendor does not fix that -- it leaves a menu that is still mostly wrong and still missing everything that works. `codex debug models` renders the catalogue as JSON, works signed out, and carries the thinking levels each model allows with its own default. So the harness answers "which models and which levels", and the LLM catalogue goes back to what it is good at: what a token costs, which only an API-key run needs. The image carries that answer rather than being asked for it. Running the image to fill in a dropdown would, on Kubernetes, mean scheduling a pod -- a capability no arm has yet, and one that would then be written twice. Reading a label is the one thing every runtime must already do, since it cannot pull otherwise. Trimmed to six fields the value is 864 bytes; the raw document is 314 KB. The usual objection to a second copy is drift, and it does not apply: the value is generated FROM the binary in the image, during the build that installs it, and sealed into the same artifact. - A LABEL cannot read a RUN's output, so deploy/agent/build-codex.sh builds, asks the binary, then builds again passing the answer as a build argument. The second pass is cached but for the label layer. - Base64, because the value is JSON and has to survive a Dockerfile, a shell, `docker inspect` and a Kubernetes manifest without a quote being eaten. The verifier decodes it before printing. - If `codex debug models` ever moves or disappears, the IMAGE BUILD fails, naming the script. That is the whole reason it is read during a build rather than when somebody opens a page. `models` joins the declared clauses of the image contract. An image without it still conforms; the factory then has no model list for it and says so rather than guessing. The three documents that gave the old `docker build` line now give the script. --- CLAUDE.md | 2 +- deploy/README.md | 2 +- deploy/agent/build-codex.sh | 78 +++++++++++++++++ deploy/agent/codex/Dockerfile | 21 +++++ docs/SMOKE-TEST.md | 2 +- docs/factory/AGENT-IMAGE-CONTRACT.md | 39 +++++++++ ...actory-m35-one-ticket-to-a-build-design.md | 86 +++++++++++++++++++ .../agentimage/AgentImageVerifier.java | 22 ++++- .../dev/codespire/agentimage/Clauses.java | 25 +++++- .../agentimage/ReferenceImageIT.java | 22 ++++- 10 files changed, 293 insertions(+), 6 deletions(-) create mode 100644 deploy/agent/build-codex.sh diff --git a/CLAUDE.md b/CLAUDE.md index 8d29a7a4..70c2500a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -142,7 +142,7 @@ cd spire-ui && npm install && npm run dev # React dashboard :34000 (UI_PORT) **The factory's two images** are not on GHCR yet and are built locally (SMOKE-TEST Mode Q): ```bash -docker build -f deploy/agent/codex/Dockerfile -t spire-agent-codex:latest deploy/agent +./deploy/agent/build-codex.sh # builds, then bakes in the model catalogue ./gradlew :spire-publisher:installDist && docker build -t spire-publisher:latest spire-publisher ``` diff --git a/deploy/README.md b/deploy/README.md index 7918c45a..6fe1e6cb 100644 --- a/deploy/README.md +++ b/deploy/README.md @@ -140,7 +140,7 @@ The run worker — the service that executes agent runs — is behind a compose ```bash # Build it locally first; the two factory images are not on GHCR yet (see CLAUDE.md, Mode Q). -docker build -f deploy/agent/codex/Dockerfile -t spire-agent-codex:latest deploy/agent +./deploy/agent/build-codex.sh # builds, then bakes in the model catalogue ./gradlew :spire-publisher:installDist && docker build -t spire-publisher:latest spire-publisher docker compose -f deploy/compose.yml --env-file deploy/.env --profile factory up -d diff --git a/deploy/agent/build-codex.sh b/deploy/agent/build-codex.sh new file mode 100644 index 00000000..5b08d287 --- /dev/null +++ b/deploy/agent/build-codex.sh @@ -0,0 +1,78 @@ +#!/usr/bin/env bash +# +# Builds the reference Codex agent image WITH its model catalogue baked in. +# +# Why this script exists at all: a Dockerfile cannot set a LABEL from a RUN's output. The value has to +# exist before the build that carries it, and the value can only come from the binary that build +# installs. So the image is built twice — once to get a binary to ask, once to carry the answer. The +# second pass reuses the whole cache except the label layer, so it costs a second, not a rebuild. +# +# ./deploy/agent/build-codex.sh [tag] # default tag: spire-agent-codex:latest +# +# `docker build -f deploy/agent/codex/Dockerfile -t spire-agent-codex:latest deploy/agent` still works +# and still produces a runnable agent. What it does NOT produce is the model label, and the factory then +# has no model list for that image — which the settings screen says rather than guessing. +set -euo pipefail + +TAG="${1:-spire-agent-codex:latest}" +HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +DOCKERFILE="$HERE/codex/Dockerfile" + +echo "==> pass 1: building $TAG without a model catalogue" +docker build -f "$DOCKERFILE" -t "$TAG" "$HERE" + +# `codex debug models` renders the raw catalogue as JSON and needs no sign-in (measured 2026-09-18 on +# @openai/codex@0.146.0). The raw document is ~314 KB and almost all of it describes things a screen has +# no use for, so it is trimmed here to the six fields the factory shows or enforces: +# +# s slug what --model is given +# n display name what the operator reads +# d default reasoning level that model's OWN default, not a global one +# e supported levels the levels THIS model allows, which differ per model +# v visibility the vendor's own "show this one" flag +# p priority the vendor's own ordering +# +# Models the vendor marks as not usable through the API are dropped: an API-key run cannot call them, +# and a subscription run is not a reason to offer a model that half this deployment cannot use. +# +# Trimmed inside the container, with the node that is already there, so this script needs nothing on the +# host but docker. Node is present because the image is node-based and the CLI ships through npm. +echo "==> reading the model catalogue from the image" +MODELS="$(docker run --rm --entrypoint sh "$TAG" -c ' + codex debug models 2>/dev/null | node -e " + let raw = \"\"; + process.stdin.on(\"data\", chunk => raw += chunk); + process.stdin.on(\"end\", () => { + const parsed = JSON.parse(raw); + const trimmed = (parsed.models || []) + .filter(model => model.supported_in_api) + .map(model => ({ + s: model.slug, + n: model.display_name, + d: model.default_reasoning_level, + e: (model.supported_reasoning_levels || []).map(level => level.effort), + v: model.visibility, + p: model.priority, + })); + if (trimmed.length === 0) throw new Error(\"the catalogue named no API-usable model\"); + process.stdout.write(Buffer.from(JSON.stringify(trimmed)).toString(\"base64\")); + }); + " +')" + +if [ -z "$MODELS" ]; then + # Loudly, at BUILD time. `codex debug models` lives under `debug`, so the vendor may move or remove + # it — and the whole reason the catalogue is read during a build is that this failure lands on + # whoever built the image, rather than on an operator opening a settings page months later. + echo "FAILED: the image produced no model catalogue." >&2 + echo " \`codex debug models\` answered nothing usable. If the vendor changed that command, this" >&2 + echo " script is what has to change — not the screens that read the label." >&2 + exit 1 +fi + +echo "==> $(printf '%s' "$MODELS" | base64 -d | node -e 'let r="";process.stdin.on("data",c=>r+=c);process.stdin.on("end",()=>console.log(JSON.parse(r).map(m=>m.s).join(", ")))')" + +echo "==> pass 2: baking the catalogue into $TAG" +docker build -f "$DOCKERFILE" --build-arg AGENT_MODELS="$MODELS" -t "$TAG" "$HERE" + +echo "==> done. $TAG carries dev.codespire.agent.models ($(printf '%s' "$MODELS" | wc -c) bytes)" diff --git a/deploy/agent/codex/Dockerfile b/deploy/agent/codex/Dockerfile index eeb7e5c0..b9b2b5c9 100644 --- a/deploy/agent/codex/Dockerfile +++ b/deploy/agent/codex/Dockerfile @@ -36,6 +36,27 @@ COPY --chmod=755 spire-agent-entrypoint.sh /usr/local/bin/spire-agent-entrypoint LABEL dev.codespire.agent.toolchain=node LABEL dev.codespire.agent.harness=codex +# The models THIS image's harness can run, and the thinking levels each one allows, as base64 of a +# trimmed `codex debug models`. The build script deploy/agent/build-codex.sh produces it; building +# this Dockerfile by hand leaves it empty, and the factory then has no model list for the image. +# +# WHY A LABEL. The alternative is running the image to ask it, which on Kubernetes means scheduling a +# pod to fill in a dropdown -- a capability that does not exist yet and would be written twice. Image +# metadata is the one thing every runtime must already be able to read, since it cannot pull otherwise. +# +# WHY IT CANNOT DRIFT. The value is generated FROM the binary in this image, during this build, and +# sealed into the same artifact. A new CLI version yields a new image and a new label; they ship or +# fail together. +# +# WHY BASE64. The trimmed value is JSON, and a raw JSON string would have to survive a Dockerfile, a +# shell, `docker inspect` output and a Kubernetes manifest without a quote being eaten anywhere. +# Base64 is [A-Za-z0-9+/=] and survives all four. `spire-agent-image verify` prints it decoded. +# +# An ARG, because a LABEL cannot read a RUN's output: the value must exist before the build that +# carries it. Declared here, after the heavy layers, so the second pass reuses the whole cache. +ARG AGENT_MODELS="" +LABEL dev.codespire.agent.models=$AGENT_MODELS + USER 1001:1001 ENV HOME=/home/agent \ SPIRE_WORKSPACE=/workspace \ diff --git a/docs/SMOKE-TEST.md b/docs/SMOKE-TEST.md index e9829fa7..de3d1ba5 100644 --- a/docs/SMOKE-TEST.md +++ b/docs/SMOKE-TEST.md @@ -1740,7 +1740,7 @@ against a forge, authenticated as a machine account. 1. **Images.** Neither image is published yet; build both locally: ```bash - docker build -f deploy/agent/codex/Dockerfile -t spire-agent-codex:latest deploy/agent + ./deploy/agent/build-codex.sh ./gradlew :spire-publisher:installDist && docker build -t spire-publisher:latest spire-publisher ``` diff --git a/docs/factory/AGENT-IMAGE-CONTRACT.md b/docs/factory/AGENT-IMAGE-CONTRACT.md index ad1067cc..0128c90b 100644 --- a/docs/factory/AGENT-IMAGE-CONTRACT.md +++ b/docs/factory/AGENT-IMAGE-CONTRACT.md @@ -146,6 +146,45 @@ Which harness the image provides, matching a `HarnessAdapter` name (`codex`). behaves as that harness without a model credential and a paid call. Running one to find out would make a conformance check cost money. +### `models` — `dev.codespire.agent.models` + +Which models this image's harness can run, and which thinking levels each one allows. **Base64 of a +JSON array**, one object per model: + +| Key | Meaning | +|---|---| +| `s` | slug — what the harness is given as its model name | +| `n` | display name — what an operator reads | +| `d` | that model's own default thinking level | +| `e` | the thinking levels this model allows | +| `v` | `list` or `hide` — the vendor's own "show this one" flag | +| `p` | the vendor's own ordering | + +*Why base64:* the value is JSON, and it has to survive a Dockerfile, a shell, `docker inspect` output +and a Kubernetes manifest without a quote being eaten anywhere. `spire-agent-image verify` decodes it +before printing, so a report shows JSON rather than base64. + +*Why the factory needs it:* without it, a build setup can only offer every model somebody typed into +the LLM catalogue — and those are different lists. Measured on 2026-09-18, the reference image's Codex +and this deployment's catalogue had **two models in common**, so the screen offered models that could +not run and hid every one that could. + +*Why a label and not a question:* asking the image means running it, and on Kubernetes that means +scheduling a pod to fill in a dropdown. Reading an image's labels is the one thing every runtime must +already do, because it cannot pull otherwise. + +*Why it cannot drift:* the value is generated FROM the binary in the image, during the build that +installs it, and sealed into the same artifact. A new CLI version produces a new image and a new +label; they ship or fail together. + +*Why it cannot be verified:* proving a model runs means calling the vendor once per model, with a +credential, for money. + +**An image without it still conforms.** The factory then has no model list for that image and says so, +rather than guessing. `deploy/agent/build-codex.sh` is what produces it for the reference image — a +plain `docker build` of the same Dockerfile leaves it empty, because a `LABEL` cannot read a `RUN`'s +output and the value must exist before the build that carries it. + --- ## What conformance does not promise diff --git a/docs/superpowers/specs/2026-09-16-factory-m35-one-ticket-to-a-build-design.md b/docs/superpowers/specs/2026-09-16-factory-m35-one-ticket-to-a-build-design.md index 31acbf61..e6b4f586 100644 --- a/docs/superpowers/specs/2026-09-16-factory-m35-one-ticket-to-a-build-design.md +++ b/docs/superpowers/specs/2026-09-16-factory-m35-one-ticket-to-a-build-design.md @@ -481,6 +481,92 @@ create a second paid attempt. The stored form stays what the gate binds. The detail page says "The ticket changed after it was prepared" and offers "Prepare again", which supersedes an open gate and writes new artifact rows. +## 6A. Part M — the models a harness can actually run + +**Found by the operator on 2026-09-18, testing part B.** The model list offered for a build setup is every +enabled model in the LLM catalogue, filtered by nothing. Codex was offered Claude and Gemini models. The +backend agrees with the screen and is equally wrong: `BuildDefaults.save` checks that a model is enabled +and fully priced, and never asks whether the chosen harness can run it. So `codex` with `claude-opus-5` +saves, and dies when the run starts — after an approval, mid-item, which is the exact failure this +milestone exists to remove. + +Measured the same day, in the pinned image (`@openai/codex@0.146.0`): + +| Asked | Answer | +|---|---| +| What models does this deployment's Codex know? | `gpt-5.6-sol`, `gpt-5.6-terra`, `gpt-5.6-luna`, `gpt-5.5`, `gpt-5.4`, `gpt-5.4-mini`, `gpt-5.2`, `codex-auto-review` | +| What does the LLM catalogue offer for OpenAI? | `gpt-5.4`, `gpt-5.4-mini`, `gpt-5.4-nano`, `gpt-5.4-pro`, `gpt-5.5`, `gpt-5.5-pro` | +| Overlap | **two**. Three catalogue models Codex cannot run, and every `gpt-5.6` missing | + +So the catalogue is not a weaker version of the harness's list — it is a different list. Filtering it by +vendor does not fix this; it leaves a menu that is still mostly wrong and still missing what works. + +### 6A.1 Who knows the answer + +`codex debug models` renders the raw model catalogue as JSON, **works signed out**, and carries per model: +the slug, the display name, `default_reasoning_level`, `supported_reasoning_levels`, `visibility`, +`priority` and `supported_in_api`. It answers both open questions at once — which models, and which +thinking levels — from the vendor rather than from us. + +That settles the split: + +| Question | Answered by | +|---|---| +| Which models can this harness run, at which thinking levels? | the harness itself | +| What does a token cost? | the LLM catalogue, and only when we pay per token | + +The catalogue stops being a menu and becomes what it is good at. An API-key run still needs a priced +entry, so the refusal becomes "you chose `gpt-5.6-sol` and it has no rate yet". A subscription run needs +none: its cost is an asserted zero (5.7). + +### 6A.2 Where it is read, and why not by running the image + +The obvious implementation — run `codex debug models` when the screen needs it — was rejected for one +reason: **Kubernetes**. `RuntimeType` declares `DOCKER` and `KUBERNETES` and only Docker is built, so +running a container to fill in a dropdown would make a settings page depend on a capability that does not +exist yet, and would have to be written twice. + +**The image carries its own answer instead.** At build time the catalogue is read from the binary, +trimmed to what a screen needs, and baked in as `dev.codespire.agent.models` beside the two labels the +image contract already declares. Trimmed it is **864 bytes**; the raw JSON is 314 KB, which is why it is +trimmed rather than copied. + +The usual objection to a second copy — drift — does not apply. The copy is generated FROM the binary, in +the same build, and sealed into the same artifact. They cannot disagree, because they ship together and a +new CLI version produces a new image and a new label. + +**A Dockerfile cannot set a `LABEL` from a `RUN`'s output**, so this needs a build wrapper: build, ask +the binary, then build again passing the answer as a build argument. The second pass is cached except for +the label layer. + +### 6A.3 Reading it without a daemon in the orchestrator + +The run worker reads the label and reports it; the orchestrator stores it and serves every screen from +its own database. Two consequences, both wanted: + +- **No screen ever waits on a daemon or a cluster.** The dropdown is a database read. +- **A deployment whose arm is unfinished degrades to the last list it was told**, rather than to no + models at all. + +Reading an image's metadata is the one thing every arm must be able to do — it cannot pull an image +otherwise — so this asks for no capability the factory does not already require. + +### 6A.4 What the operator chooses + +Factory tab, step 5 becomes: base branch, harness, **pay with**, model, **thinking level**. The model list +comes from the harness; the levels are the ones THAT model declares, defaulting to its own default. An +API-key choice additionally requires a priced model and says so by name. + +### 6A.5 The cost, stated + +The image contract gains a clause, so an operator building their own agent image must produce that label +or the factory has no model list for it. `spire-agent-image verify` reports it, under the same heading +that already says which clauses are declared rather than proved. + +`codex debug models` lives under `debug`, so the vendor may change or remove it. If it does, the build +fails at image build time — not at run time, and not silently — which is the whole reason it is read +during a build rather than when somebody opens a page. + ## 7. Part P — live proof on `spire-test` 1. Write two TEST tickets with acceptance criteria. Build defaults: harness `codex`, a priced model. diff --git a/spire-agent-image/src/main/java/dev/codespire/agentimage/AgentImageVerifier.java b/spire-agent-image/src/main/java/dev/codespire/agentimage/AgentImageVerifier.java index de4f138b..a3685d33 100644 --- a/spire-agent-image/src/main/java/dev/codespire/agentimage/AgentImageVerifier.java +++ b/spire-agent-image/src/main/java/dev/codespire/agentimage/AgentImageVerifier.java @@ -345,7 +345,27 @@ private static List declarations(InspectImageResp new ConformanceReport.Declaration(Clauses.HARNESS, bounded(labels.get(Clauses.HARNESS_LABEL)), "verifying this needs a model credential and a paid call, so a " - + "conformance check would cost money to run")); + + "conformance check would cost money to run"), + new ConformanceReport.Declaration(Clauses.MODELS, + bounded(decoded(labels.get(Clauses.MODELS_LABEL))), + "verifying this needs the vendor to confirm each model, which costs a " + + "credential and a paid call for every one of them")); + } + + /** + * Base64 in, JSON out — or the value untouched when it is not base64. + * + *

An operator reading a conformance report wants to see which models the image claims, not a + * kilobyte of base64. A value that does not decode is shown AS IT IS rather than hidden: an image + * whose label was set by hand is exactly the case this line exists to make visible. + */ + private static String decoded(String value) { + if (value == null || value.isBlank()) return value; + try { + return new String(java.util.Base64.getDecoder().decode(value), java.nio.charset.StandardCharsets.UTF_8); + } catch (IllegalArgumentException notBase64) { + return value; + } } /** diff --git a/spire-agent-image/src/main/java/dev/codespire/agentimage/Clauses.java b/spire-agent-image/src/main/java/dev/codespire/agentimage/Clauses.java index 4b6d2838..8ad51ce0 100644 --- a/spire-agent-image/src/main/java/dev/codespire/agentimage/Clauses.java +++ b/spire-agent-image/src/main/java/dev/codespire/agentimage/Clauses.java @@ -51,6 +51,29 @@ public final class Clauses { /** The label a {@link #HARNESS} declaration is read from. */ public static final String HARNESS_LABEL = "dev.codespire.agent.harness"; + /** + * Which models this image's harness can run, and the thinking levels each one allows. + * + *

Unverifiable here for the same reason {@link #HARNESS} is: proving it means calling the + * vendor, which costs money and a credential. What a checker CAN do is read it back and show it, + * which is what turns "the factory has no models for this image" into an answerable question. + * + *

The factory reads this from image METADATA rather than by running the image. That is not an + * optimisation: on Kubernetes, running it means scheduling a pod to fill in a dropdown, which is a + * capability no arm has yet. Reading an image's labels is the one thing every arm must already be + * able to do, since it cannot pull otherwise. + */ + public static final String MODELS = "models"; + + /** + * The label a {@link #MODELS} declaration is read from, base64 of a trimmed model catalogue. + * + *

Base64 because the value is JSON and has to survive a Dockerfile, a shell, {@code docker + * inspect} output and a Kubernetes manifest without a quote being eaten anywhere. This checker + * decodes it before printing, so an operator reads JSON rather than base64. + */ + public static final String MODELS_LABEL = "dev.codespire.agent.models"; + /** Every clause this checker proves, in report order. */ public static final List VERIFIED = List.of( ENTRYPOINT, NON_ROOT, MOUNT_POINTS, GIT, CA_CERTIFICATES, @@ -63,7 +86,7 @@ public final class Clauses { * edit to both this file and the contract — which is exactly the change that should not happen * quietly. */ - public static final List DECLARED = List.of(TOOLCHAIN, HARNESS); + public static final List DECLARED = List.of(TOOLCHAIN, HARNESS, MODELS); private Clauses() { } diff --git a/spire-agent-image/src/test/java/dev/codespire/agentimage/ReferenceImageIT.java b/spire-agent-image/src/test/java/dev/codespire/agentimage/ReferenceImageIT.java index 657c66de..b10bdce7 100644 --- a/spire-agent-image/src/test/java/dev/codespire/agentimage/ReferenceImageIT.java +++ b/spire-agent-image/src/test/java/dev/codespire/agentimage/ReferenceImageIT.java @@ -17,6 +17,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; /** @@ -233,7 +234,7 @@ void anImageRunningAsRootFailsTheNonRootClause() throws IOException { void declaredClausesAreReportedSeparatelyFromVerifiedOnes() throws IOException { ConformanceReport report = verify(buildConforming()); - assertEquals(List.of(Clauses.TOOLCHAIN, Clauses.HARNESS), + assertEquals(List.of(Clauses.TOOLCHAIN, Clauses.HARNESS, Clauses.MODELS), report.declared().stream().map(ConformanceReport.Declaration::id).toList()); assertEquals("conformance-probe", report.declared().stream() .filter(declaration -> declaration.id().equals(Clauses.HARNESS)) @@ -241,4 +242,23 @@ void declaredClausesAreReportedSeparatelyFromVerifiedOnes() throws IOException { assertTrue(report.conforms(), "a harness that does not exist is a claim, and no verified clause checked it"); } + + /** + * An image with no model catalogue still CONFORMS, and says the clause is empty. + * + *

The clause is declared, not required: an operator's own image is a conforming agent whether + * or not it can tell the factory which models it runs. What must not happen is silence — a missing + * catalogue has to be visible here, because the only other place it shows up is a build-setup + * screen with an empty dropdown and no explanation. + */ + @Test + void anImageWithNoModelCatalogueSaysSoAndStillConforms() throws IOException { + ConformanceReport report = verify(buildConforming()); + + assertNull(report.declared().stream() + .filter(declaration -> declaration.id().equals(Clauses.MODELS)) + .findFirst().orElseThrow().claimed(), + "the conformance probe declares no models, and an absent claim reads as absent"); + assertTrue(report.conforms(), "a model catalogue is declared, never required"); + } } From 535543bf46080eaf762afe4b114c32f6f4301244 Mon Sep 17 00:00:00 2001 From: Artjoms Stukans Date: Tue, 22 Sep 2026 21:40:55 +0200 Subject: [PATCH 2/7] Learn each harness's models from its image, and cache the answer The orchestrator now knows which models each harness can run, and at which thinking levels, without ever reading an image itself. It has no container runtime, and on Kubernetes it never will. The run worker must reach every agent image or no run could start, so it is the one asked: it reads the image's catalogue label and reports, and the orchestrator keeps the answer in harness_catalogue (V79). Every screen reads that table, so no page waits on a runtime, and a deployment whose runtime arm is unfinished degrades to the last answer rather than to none. - RunRuntime gains imageLabels(image). On that port and not a new one because the registry credential lives there and must reach nothing but a pull; the Docker arm reuses the exact authenticated pull a run uses, so an image a run could start is an image that can be described. The default THROWS rather than answering empty: an empty map is a real answer ("declares nothing") and an arm that cannot look is a different fact. - An empty list means four different things -- no catalogue, an unreadable one, an unreachable image, or nothing heard yet -- so each is named and kept beside the list. A screen that cannot tell them apart can only show an empty dropdown. - One bad entry makes the whole label unreadable rather than yielding the entries that parsed: a list with a silent hole looks exactly like a complete one. - An answer describing an image the harness no longer runs is dropped, and a stored one is not served once the configuration moves on. A different tag may carry a different CLI and a different list. - The options endpoint returns each harness's offered models beside its name, in the vendor's order with its hidden ones left out, so the screen makes one call and cannot pair a harness with another's models. Proved against a real daemon: the reference image's models are read from metadata alone, and the test asserts no container was started. Also turns the part C preparation sweep OFF under test, where every other background loop already was. It had been running every 20 seconds against whatever items the current test created, writing artifacts and health rows into the middle of teardowns -- the actual cause of the foreign-key failures that were being patched one teardown at a time. --- .../contract/command/HarnessImageCommand.java | 37 +++++ .../contract/event/HarnessImageResult.java | 69 ++++++++ .../factory/HarnessCatalogues.java | 151 ++++++++++++++++++ .../HarnessImageCommandSerializer.java | 8 + .../HarnessImageResultDeserializer.java | 25 +++ .../factory/HarnessImageResultSerializer.java | 50 ++++++ .../factory/HarnessImageResults.java | 36 +++++ .../factory/RepositoryBuildResource.java | 29 +++- .../src/main/resources/application.yml | 25 +++ .../db/migration/V79__harness_catalogue.sql | 22 +++ .../factory/HarnessCataloguesTest.java | 123 ++++++++++++++ .../HarnessImageCommandDeserializer.java | 25 +++ .../HarnessImageCommandSerializer.java | 11 ++ .../HarnessImageResultSerializer.java | 8 + .../runworker/HarnessImageWorker.java | 81 ++++++++++ .../runworker/ModelCatalogueLabel.java | 67 ++++++++ .../src/main/resources/application.yml | 23 +++ .../runworker/ModelCatalogueLabelTest.java | 98 ++++++++++++ .../runtime/docker/DockerRunRuntime.java | 9 ++ .../runtime/docker/DockerImageLabelsIT.java | 57 +++++++ .../dev/codespire/runtime/RunRuntime.java | 18 +++ 21 files changed, 968 insertions(+), 4 deletions(-) create mode 100644 spire-contract/src/main/java/dev/codespire/contract/command/HarnessImageCommand.java create mode 100644 spire-contract/src/main/java/dev/codespire/contract/event/HarnessImageResult.java create mode 100644 spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCatalogues.java create mode 100644 spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessImageCommandSerializer.java create mode 100644 spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessImageResultDeserializer.java create mode 100644 spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessImageResultSerializer.java create mode 100644 spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessImageResults.java create mode 100644 spire-orchestrator/src/main/resources/db/migration/V79__harness_catalogue.sql create mode 100644 spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessCataloguesTest.java create mode 100644 spire-run-worker/src/main/java/dev/codespire/runworker/HarnessImageCommandDeserializer.java create mode 100644 spire-run-worker/src/main/java/dev/codespire/runworker/HarnessImageCommandSerializer.java create mode 100644 spire-run-worker/src/main/java/dev/codespire/runworker/HarnessImageResultSerializer.java create mode 100644 spire-run-worker/src/main/java/dev/codespire/runworker/HarnessImageWorker.java create mode 100644 spire-run-worker/src/main/java/dev/codespire/runworker/ModelCatalogueLabel.java create mode 100644 spire-run-worker/src/test/java/dev/codespire/runworker/ModelCatalogueLabelTest.java create mode 100644 spire-runtime-docker/src/test/java/dev/codespire/runtime/docker/DockerImageLabelsIT.java diff --git a/spire-contract/src/main/java/dev/codespire/contract/command/HarnessImageCommand.java b/spire-contract/src/main/java/dev/codespire/contract/command/HarnessImageCommand.java new file mode 100644 index 00000000..ee4aa576 --- /dev/null +++ b/spire-contract/src/main/java/dev/codespire/contract/command/HarnessImageCommand.java @@ -0,0 +1,37 @@ +package dev.codespire.contract.command; + +import com.fasterxml.jackson.annotation.JsonSubTypes; +import com.fasterxml.jackson.annotation.JsonTypeInfo; + +/** + * Asking the run worker what an agent image says about itself (M3.5 part M). + * + *

The orchestrator knows which image each harness dispatches to — it holds that configuration — but + * it cannot read an image: it has no container runtime, and on Kubernetes it never will. The run worker + * must be able to reach every agent image or no run could start, so it is the one asked. + * + *

Rides {@code cs.harness-image-commands}, keyed by {@code requestId}. + */ +@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type") +@JsonSubTypes({ + @JsonSubTypes.Type(value = HarnessImageCommand.Describe.class, name = "Describe") +}) +public sealed interface HarnessImageCommand { + + String requestId(); + + /** + * Read the model catalogue this image declares. + * + * @param harness the name the orchestrator dispatches under, echoed back so the answer files itself + * @param image the exact reference the orchestrator will run — the answer describes THIS image, and a + * different tag of the same repository may carry a different CLI and a different list + */ + record Describe(String requestId, String harness, String image) implements HarnessImageCommand { + public Describe { + if (requestId == null || requestId.isBlank()) throw new IllegalArgumentException("A request id is required"); + if (harness == null || harness.isBlank()) throw new IllegalArgumentException("A harness is required"); + if (image == null || image.isBlank()) throw new IllegalArgumentException("An image is required"); + } + } +} diff --git a/spire-contract/src/main/java/dev/codespire/contract/event/HarnessImageResult.java b/spire-contract/src/main/java/dev/codespire/contract/event/HarnessImageResult.java new file mode 100644 index 00000000..1885b24a --- /dev/null +++ b/spire-contract/src/main/java/dev/codespire/contract/event/HarnessImageResult.java @@ -0,0 +1,69 @@ +package dev.codespire.contract.event; + +import com.fasterxml.jackson.annotation.JsonSubTypes; +import com.fasterxml.jackson.annotation.JsonTypeInfo; + +import java.util.List; +import java.util.Objects; + +/** + * What an agent image declares about itself (M3.5 part M). Rides {@code cs.harness-image-results}. + */ +@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type") +@JsonSubTypes({ + @JsonSubTypes.Type(value = HarnessImageResult.Described.class, name = "Described") +}) +public sealed interface HarnessImageResult { + + String requestId(); + + /** + * One model the image's harness can run. + * + * @param slug what the harness is given as its model name + * @param displayName what an operator reads + * @param defaultEffort THAT model's own default thinking level — models differ, so there is no global one + * @param efforts the thinking levels this model allows, in the vendor's order + * @param visible the vendor's own "offer this one" flag; a hidden model still runs if named + * @param priority the vendor's own ordering, lower first + */ + record Model(String slug, String displayName, String defaultEffort, List efforts, + boolean visible, int priority) { + public Model { + if (slug == null || slug.isBlank()) throw new IllegalArgumentException("A model slug is required"); + efforts = efforts == null ? List.of() : List.copyOf(efforts); + } + } + + /** + * Why an image has no usable list. Named, so the screen can say which of these it is rather than + * showing an empty dropdown that could mean any of them. + */ + enum Status { + /** The label was there and read. */ + OK, + /** The image carries no catalogue label — built without deploy/agent/build-codex.sh, typically. */ + NO_CATALOGUE, + /** The label was there and could not be read as a catalogue. */ + UNREADABLE, + /** The image could not be reached: not held, and the pull failed. */ + IMAGE_UNAVAILABLE + } + + /** + * @param models empty unless {@code status} is {@link Status#OK} + */ + record Described(String requestId, String harness, String image, Status status, List models) + implements HarnessImageResult { + public Described { + if (requestId == null || requestId.isBlank()) throw new IllegalArgumentException("A request id is required"); + Objects.requireNonNull(harness, "harness"); + Objects.requireNonNull(image, "image"); + Objects.requireNonNull(status, "status"); + models = models == null ? List.of() : List.copyOf(models); + if (status != Status.OK && !models.isEmpty()) { + throw new IllegalArgumentException("Only a readable catalogue carries models, status was " + status); + } + } + } +} diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCatalogues.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCatalogues.java new file mode 100644 index 00000000..59a1cb3e --- /dev/null +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCatalogues.java @@ -0,0 +1,151 @@ +package dev.codespire.orchestrator.factory; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.core.type.TypeReference; +import com.fasterxml.jackson.databind.ObjectMapper; +import dev.codespire.contract.command.HarnessImageCommand; +import dev.codespire.contract.event.HarnessImageResult; +import dev.codespire.orchestrator.pipeline.KafkaSends; +import io.quarkus.runtime.StartupEvent; +import io.quarkus.scheduler.Scheduled; +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.enterprise.event.Observes; +import jakarta.inject.Inject; +import org.eclipse.microprofile.reactive.messaging.Channel; +import org.eclipse.microprofile.reactive.messaging.Emitter; +import org.jboss.logging.Logger; + +import javax.sql.DataSource; +import java.sql.Connection; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.time.Instant; +import java.util.Comparator; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.UUID; + +/** + * Which models each harness can run, as its agent image declares them (M3.5 part M). + * + *

The orchestrator asks and remembers; it never reads an image itself. It has no container runtime, + * and on Kubernetes it never will, so the run worker — which must reach every agent image or no run could + * start — reads the label and reports. What comes back is cached in {@code harness_catalogue}, and every + * screen reads the cache: no page waits on a runtime, and an unfinished runtime arm degrades to the last + * answer rather than to none. + */ +@ApplicationScoped +public class HarnessCatalogues { + + private static final Logger LOG = Logger.getLogger(HarnessCatalogues.class); + + @Inject DataSource dataSource; + @Inject ObjectMapper mapper; + @Inject FactoryConfig config; + + @Inject @Channel("harness-image-commands-out") + Emitter commands; + + /** What a screen shows for one harness. */ + public record Catalogue(String harness, String image, HarnessImageResult.Status status, + List models, Instant observedAt) { + + /** The models to OFFER: the ones the vendor wants shown, in the vendor's own order. */ + public List offered() { + return models.stream().filter(HarnessImageResult.Model::visible) + .sorted(Comparator.comparingInt(HarnessImageResult.Model::priority)).toList(); + } + + /** Whether this harness can run the model, whether or not the vendor offers it. */ + public Optional find(String slug) { + return models.stream().filter(model -> model.slug().equals(slug)).findFirst(); + } + } + + /** The refresh schedule, read so that turning it off also turns off the ask at startup. */ + @org.eclipse.microprofile.config.inject.ConfigProperty(name = "spire.harness-catalogue-interval", defaultValue = "10m") + String refreshEvery; + + void onStart(@Observes StartupEvent event) { + // "off" means off: a schedule switched off that still asked once per boot would put a broker + // send into every test class's startup, for an answer nothing in that test reads. + if (!"off".equalsIgnoreCase(refreshEvery)) refresh(); + } + + /** + * Asks again, for every configured harness. + * + *

On a schedule as well as at startup, because an image can be rebuilt under the same tag, and + * because a worker that was down at startup would otherwise leave the cache empty until the next + * orchestrator restart. The answer is cheap: a label read, and a pull only the first time. + */ + @Scheduled(every = "${spire.harness-catalogue-interval:10m}", + concurrentExecution = Scheduled.ConcurrentExecution.SKIP) + void refresh() { + for (Map.Entry harness : config.agentImage().entrySet()) { + try { + KafkaSends.sendAndAwait(commands, harness.getKey(), + new HarnessImageCommand.Describe(UUID.randomUUID().toString(), harness.getKey(), harness.getValue()), + "describe the image for " + harness.getKey()); + } catch (RuntimeException undelivered) { + // The next interval asks again; one lost question is not worth failing startup over. + LOG.warnf("could not ask for the model catalogue of %s (%s)", harness.getKey(), + undelivered.getClass().getSimpleName()); + } + } + } + + /** + * Stores an answer — unless it describes an image the harness no longer runs. + * + *

A different tag of the same repository may carry a different CLI and a different list, so an + * answer that arrives after the configuration moved on would describe models that will not run. + */ + public void record(HarnessImageResult.Described answer) { + String current = config.agentImage().get(answer.harness()); + if (current == null || !current.equals(answer.image())) { + LOG.infof("ignoring a model catalogue for %s that describes an image it no longer runs", answer.harness()); + return; + } + try (Connection c = dataSource.getConnection(); PreparedStatement ps = c.prepareStatement(""" + INSERT INTO harness_catalogue (harness, image, status, models, observed_at) + VALUES (?, ?, ?, ?::jsonb, now()) + ON CONFLICT (harness) DO UPDATE + SET image=excluded.image, status=excluded.status, models=excluded.models, observed_at=now() + """)) { + ps.setString(1, answer.harness()); + ps.setString(2, answer.image()); + ps.setString(3, answer.status().name()); + ps.setString(4, mapper.writeValueAsString(answer.models())); + ps.executeUpdate(); + } catch (SQLException | JsonProcessingException failure) { + throw new IllegalStateException("The model catalogue for " + answer.harness() + " could not be stored", failure); + } + } + + /** + * The catalogue for a harness, or empty when nothing has been heard yet. + * + *

Empty is kept distinct from a catalogue whose status says why it has no models: "we have not + * asked yet" and "the image declares nothing" send an operator to different places. + */ + public Optional get(String harness) { + try (Connection c = dataSource.getConnection(); PreparedStatement ps = c.prepareStatement( + "SELECT harness, image, status, models::text, observed_at FROM harness_catalogue WHERE harness=?")) { + ps.setString(1, harness); + try (ResultSet rs = ps.executeQuery()) { + if (!rs.next()) return Optional.empty(); + // An answer for an image the configuration has since left is not served. + if (!rs.getString("image").equals(config.agentImage().get(harness))) return Optional.empty(); + return Optional.of(new Catalogue(rs.getString("harness"), rs.getString("image"), + HarnessImageResult.Status.valueOf(rs.getString("status")), + mapper.readValue(rs.getString("models"), new TypeReference>() { }), + rs.getTimestamp("observed_at").toInstant())); + } + } catch (SQLException | JsonProcessingException failure) { + throw new IllegalStateException("The model catalogue for " + harness + " could not be read", failure); + } + } +} diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessImageCommandSerializer.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessImageCommandSerializer.java new file mode 100644 index 00000000..44b08969 --- /dev/null +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessImageCommandSerializer.java @@ -0,0 +1,8 @@ +package dev.codespire.orchestrator.factory; + +import dev.codespire.contract.command.HarnessImageCommand; +import io.quarkus.kafka.client.serialization.ObjectMapperSerializer; + +/** Polymorphic JSON on {@code cs.harness-image-commands}, keyed by harness. */ +public class HarnessImageCommandSerializer extends ObjectMapperSerializer { +} diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessImageResultDeserializer.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessImageResultDeserializer.java new file mode 100644 index 00000000..581fdbf9 --- /dev/null +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessImageResultDeserializer.java @@ -0,0 +1,25 @@ +package dev.codespire.orchestrator.factory; + +import dev.codespire.contract.event.HarnessImageResult; +import io.quarkus.kafka.client.serialization.ObjectMapperDeserializer; +import org.jboss.logging.Logger; + +/** Never throws on a poison record: a throwing deserializer kills the consumer and redelivers for ever. */ +public class HarnessImageResultDeserializer extends ObjectMapperDeserializer { + + private static final Logger LOG = Logger.getLogger(HarnessImageResultDeserializer.class); + + public HarnessImageResultDeserializer() { + super(HarnessImageResult.class); + } + + @Override + public HarnessImageResult deserialize(String topic, byte[] data) { + try { + return super.deserialize(topic, data); + } catch (RuntimeException undeserializable) { + LOG.errorf(undeserializable, "dropping an undeserializable record on %s", topic); + return null; + } + } +} diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessImageResultSerializer.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessImageResultSerializer.java new file mode 100644 index 00000000..d6ccfa07 --- /dev/null +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessImageResultSerializer.java @@ -0,0 +1,50 @@ +package dev.codespire.orchestrator.factory; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.ObjectWriter; +import dev.codespire.contract.event.HarnessImageResult; +import io.quarkus.arc.Arc; +import org.apache.kafka.common.serialization.Serializer; + +import java.io.UncheckedIOException; + +/** + * The orchestrator never publishes a {@link HarnessImageResult} — the worker does — but the + * dead-letter queue on {@code harness-image-results-in} re-serializes a record whose processing + * failed, and SmallRye resolves that serializer by rewriting the configured deserializer's class name + * ({@code HarnessImageResultDeserializer} → {@code HarnessImageResultSerializer}). Without this class + * the channel refuses to start at all, and the whole service with it. + * + *

The same trap {@code RunResultSerializer} documents, met again here for the same reason: the name + * is derived, so a channel with a dead-letter queue needs both halves in one package even when only + * one direction is ever used. + */ +public class HarnessImageResultSerializer implements Serializer { + + private final ObjectWriter writer = resolveMapper().writerFor(HarnessImageResult.class); + + private static ObjectMapper resolveMapper() { + var container = Arc.container(); + if (container != null && container.isRunning()) { + var instance = container.instance(ObjectMapper.class); + if (instance.isAvailable()) { + return instance.get(); + } + } + return new ObjectMapper().findAndRegisterModules(); + } + + @Override + public byte[] serialize(String topic, HarnessImageResult result) { + if (result == null) { + return null; + } + try { + // writerFor the interface, so the type discriminator survives a dead-letter round trip. + return writer.writeValueAsBytes(result); + } catch (JsonProcessingException e) { + throw new UncheckedIOException("Failed to serialize " + result.getClass().getSimpleName(), e); + } + } +} diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessImageResults.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessImageResults.java new file mode 100644 index 00000000..f91927b0 --- /dev/null +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessImageResults.java @@ -0,0 +1,36 @@ +package dev.codespire.orchestrator.factory; + +import dev.codespire.contract.event.HarnessImageResult; +import io.smallrye.reactive.messaging.annotations.Blocking; +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.inject.Inject; +import org.eclipse.microprofile.reactive.messaging.Incoming; +import org.eclipse.microprofile.reactive.messaging.Message; +import org.jboss.logging.Logger; + +import java.util.concurrent.CompletionStage; + +/** What the run worker reported an agent image declares, handed to {@link HarnessCatalogues}. */ +@ApplicationScoped +public class HarnessImageResults { + + private static final Logger LOG = Logger.getLogger(HarnessImageResults.class); + + @Inject HarnessCatalogues catalogues; + + @Incoming("harness-image-results-in") + @Blocking(ordered = false) + public CompletionStage onResult(Message message) { + if (message.getPayload() instanceof HarnessImageResult.Described described) { + try { + catalogues.record(described); + } catch (RuntimeException failure) { + // Nack, so the dead-letter queue sees it. The next scheduled ask would repair the cache + // anyway, but a failure nobody can see is how a stale list goes unnoticed for a week. + LOG.errorf(failure, "the model catalogue for %s could not be recorded", described.harness()); + return message.nack(failure); + } + } + return message.ack(); + } +} diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/RepositoryBuildResource.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/RepositoryBuildResource.java index 84ffdbf1..0f70038a 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/RepositoryBuildResource.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/RepositoryBuildResource.java @@ -41,7 +41,21 @@ public class RepositoryBuildResource { * without an agent image refuses at dispatch; a model that cannot price one of those types refuses * there too, so the screen is told both rather than guessing at either. */ - public record Options(List harnesses, Map> reportedTypes) {} + public record Options(List harnesses, Map> reportedTypes, + Map models) {} + + /** + * The models a harness can run, as its image declares them, and why there are none when there are none. + * + * @param status {@code UNKNOWN} when nothing has been heard from the run worker yet, otherwise the + * image's own answer. An empty list means four different things, and a screen that cannot tell them + * apart can only show an empty dropdown. + * @param offered the models to show, in the vendor's order. The vendor's hidden ones are left out: + * they still run if named, but an operator choosing from a list should see what the vendor offers. + */ + public record HarnessModels(String status, List offered) {} + + @Inject HarnessCatalogues catalogues; /** @param account the role whose account answered, so a reviewer-confirmed head is not read as factory push access */ public record Head(String branch, String commit, String account) {} @@ -54,9 +68,16 @@ public BuildDefaults.Defaults get(@PathParam("repository") UUID repository) { @GET @Path("/build/options") public Options options() { List harnesses = config.agentImage().keySet().stream().sorted().toList(); - return new Options(harnesses, harnesses.stream().collect(java.util.stream.Collectors.toMap( - harness -> harness, - harness -> HarnessTokenReport.reportedBy(harness).stream().map(Enum::name).sorted().toList()))); + return new Options(harnesses, + harnesses.stream().collect(java.util.stream.Collectors.toMap(harness -> harness, + harness -> HarnessTokenReport.reportedBy(harness).stream().map(Enum::name).sorted().toList())), + harnesses.stream().collect(java.util.stream.Collectors.toMap(harness -> harness, this::modelsOf))); + } + + private HarnessModels modelsOf(String harness) { + return catalogues.get(harness) + .map(catalogue -> new HarnessModels(catalogue.status().name(), catalogue.offered())) + .orElse(new HarnessModels("UNKNOWN", List.of())); } @PUT @Path("/build") diff --git a/spire-orchestrator/src/main/resources/application.yml b/spire-orchestrator/src/main/resources/application.yml index 182ecb07..6d0f7f49 100644 --- a/spire-orchestrator/src/main/resources/application.yml +++ b/spire-orchestrator/src/main/resources/application.yml @@ -139,6 +139,15 @@ mp: serializer: org.apache.kafka.common.serialization.StringSerializer value: serializer: io.quarkus.kafka.client.serialization.ObjectMapperSerializer + harness-image-commands-out: + connector: smallrye-kafka + topic: cs.harness-image-commands + acks: all + key: + serializer: org.apache.kafka.common.serialization.StringSerializer + value: + serializer: dev.codespire.orchestrator.factory.HarnessImageCommandSerializer + waitForWriteCompletion: true harness-sign-in-commands-out: connector: smallrye-kafka topic: cs.harness-sign-in-commands @@ -261,6 +270,20 @@ mp: topic: cs.dlq # What the trusted sign-in unit reported (M3.5 part F). Its own channel, because a sign-in is # not a run: no claim, no money, no transcript, and an operator waiting on a screen for it. + # What each agent image declares about itself (M3.5 part M). + harness-image-results-in: + connector: smallrye-kafka + topic: cs.harness-image-results + group: + id: spire-orchestrator-harness-image + value: + deserializer: dev.codespire.orchestrator.factory.HarnessImageResultDeserializer + auto: + offset: + reset: latest + failure-strategy: dead-letter-queue + dead-letter-queue: + topic: cs.dlq harness-sign-in-results-in: connector: smallrye-kafka topic: cs.harness-sign-in-results @@ -451,6 +474,8 @@ spire: "%test": spire: work-gate-expiry-interval: "off" + work-preparation-interval: "off" + harness-catalogue-interval: "off" work-run-interval: "off" work-delivery-interval: "off" work-activity-interval: "off" diff --git a/spire-orchestrator/src/main/resources/db/migration/V79__harness_catalogue.sql b/spire-orchestrator/src/main/resources/db/migration/V79__harness_catalogue.sql new file mode 100644 index 00000000..96c63e5c --- /dev/null +++ b/spire-orchestrator/src/main/resources/db/migration/V79__harness_catalogue.sql @@ -0,0 +1,22 @@ +-- What each agent image declares it can run (M3.5 part M). +-- +-- A CACHE, and the design depends on it being one. The run worker reads the image and reports; this row +-- is what every screen reads. So no page ever waits on a container runtime or a cluster, and a +-- deployment whose runtime arm is unfinished degrades to the last list it was told rather than to none. +-- +-- One row per harness, holding the whole list. The list is only ever read whole and replaced whole, so +-- a row per model would be rows nobody reads alone and a replacement that has to be made atomic by hand. +CREATE TABLE harness_catalogue ( + harness VARCHAR(64) PRIMARY KEY, + -- The exact image reference the answer describes. A different tag of the same repository may carry + -- a different CLI and a different list, so an answer for an image the harness no longer runs is + -- discarded rather than served. + image TEXT NOT NULL, + -- OK / NO_CATALOGUE / UNREADABLE / IMAGE_UNAVAILABLE. Kept beside the list because an empty list + -- means four different things, and a screen that cannot tell them apart shows an empty dropdown. + status VARCHAR(32) NOT NULL + CHECK (status IN ('OK','NO_CATALOGUE','UNREADABLE','IMAGE_UNAVAILABLE')), + -- [{slug, displayName, defaultEffort, efforts[], visible, priority}], empty unless status is OK. + models JSONB NOT NULL DEFAULT '[]'::jsonb, + observed_at TIMESTAMPTZ NOT NULL DEFAULT now() +); diff --git a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessCataloguesTest.java b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessCataloguesTest.java new file mode 100644 index 00000000..c1ce4a82 --- /dev/null +++ b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessCataloguesTest.java @@ -0,0 +1,123 @@ +package dev.codespire.orchestrator.factory; + +import dev.codespire.contract.event.HarnessImageResult; +import io.quarkus.test.junit.QuarkusTest; +import jakarta.inject.Inject; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +import javax.sql.DataSource; +import java.sql.Connection; +import java.sql.SQLException; +import java.sql.Statement; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * The orchestrator's cache of which models each harness can run (M3.5 part M). + * + *

Model names are placeholders on purpose: this is about what the cache keeps and serves, and a list of + * real names would go stale the day the vendor ships. + */ +@QuarkusTest +class HarnessCataloguesTest { + + @Inject HarnessCatalogues catalogues; + @Inject FactoryConfig config; + @Inject DataSource dataSource; + + private static final String HARNESS = "codex"; + + @AfterEach + void clean() throws SQLException { + try (Connection c = dataSource.getConnection(); Statement s = c.createStatement()) { + s.executeUpdate("DELETE FROM harness_catalogue"); + } + } + + private String image() { + return config.agentImage().get(HARNESS); + } + + private static HarnessImageResult.Model model(String slug, boolean visible, int priority) { + return new HarnessImageResult.Model(slug, slug, "medium", List.of("low", "medium", "high"), visible, priority); + } + + private HarnessImageResult.Described answer(String image, HarnessImageResult.Status status, + List models) { + return new HarnessImageResult.Described("TEST-request", HARNESS, image, status, models); + } + + /** "Not asked yet" and "the image declares nothing" send an operator to different places. */ + @Test + void nothingHeardYetIsAbsentRatherThanAnEmptyList() { + assertTrue(catalogues.get(HARNESS).isEmpty()); + } + + @Test + void anAnswerIsKeptAndServed() { + catalogues.record(answer(image(), HarnessImageResult.Status.OK, + List.of(model("TEST-a", true, 2), model("TEST-b", true, 1)))); + + var catalogue = catalogues.get(HARNESS).orElseThrow(); + assertEquals(HarnessImageResult.Status.OK, catalogue.status()); + assertEquals(2, catalogue.models().size()); + } + + /** Offered means the vendor's shown ones, in the vendor's own order — not insertion order. */ + @Test + void theOfferedListIsTheVendorsShownModelsInTheVendorsOrder() { + catalogues.record(answer(image(), HarnessImageResult.Status.OK, List.of( + model("TEST-third", true, 30), model("TEST-hidden", false, 1), model("TEST-first", true, 10)))); + + var offered = catalogues.get(HARNESS).orElseThrow().offered(); + + assertEquals(List.of("TEST-first", "TEST-third"), offered.stream().map(HarnessImageResult.Model::slug).toList()); + } + + /** A hidden model is not OFFERED, but it still runs if named — so it must still be findable. */ + @Test + void aHiddenModelIsNotOfferedButIsStillKnown() { + catalogues.record(answer(image(), HarnessImageResult.Status.OK, List.of(model("TEST-hidden", false, 1)))); + + var catalogue = catalogues.get(HARNESS).orElseThrow(); + assertTrue(catalogue.offered().isEmpty()); + assertTrue(catalogue.find("TEST-hidden").isPresent()); + } + + /** + * An answer about an image the harness no longer runs is dropped. + * + *

A different tag of the same repository may carry a different CLI and a different list. Keeping an + * answer that arrived after the configuration moved on would offer models that will not run. + */ + @Test + void anAnswerAboutAnotherImageIsNotKept() { + catalogues.record(answer("TEST-some-other-image:tag", HarnessImageResult.Status.OK, + List.of(model("TEST-stale", true, 1)))); + + assertTrue(catalogues.get(HARNESS).isEmpty()); + } + + /** And the empty cases keep their reason, which is the whole point of storing a status. */ + @Test + void anImageWithNoCatalogueSaysSoRatherThanLookingEmpty() { + catalogues.record(answer(image(), HarnessImageResult.Status.NO_CATALOGUE, List.of())); + + var catalogue = catalogues.get(HARNESS).orElseThrow(); + assertEquals(HarnessImageResult.Status.NO_CATALOGUE, catalogue.status()); + assertTrue(catalogue.models().isEmpty()); + } + + /** A newer answer replaces the older one whole, including a change of status. */ + @Test + void aNewerAnswerReplacesTheOlderOne() { + catalogues.record(answer(image(), HarnessImageResult.Status.OK, List.of(model("TEST-old", true, 1)))); + catalogues.record(answer(image(), HarnessImageResult.Status.UNREADABLE, List.of())); + + var catalogue = catalogues.get(HARNESS).orElseThrow(); + assertEquals(HarnessImageResult.Status.UNREADABLE, catalogue.status()); + assertTrue(catalogue.find("TEST-old").isEmpty(), "nothing of the old list may survive a replacement"); + } +} diff --git a/spire-run-worker/src/main/java/dev/codespire/runworker/HarnessImageCommandDeserializer.java b/spire-run-worker/src/main/java/dev/codespire/runworker/HarnessImageCommandDeserializer.java new file mode 100644 index 00000000..ecd3dd94 --- /dev/null +++ b/spire-run-worker/src/main/java/dev/codespire/runworker/HarnessImageCommandDeserializer.java @@ -0,0 +1,25 @@ +package dev.codespire.runworker; + +import dev.codespire.contract.command.HarnessImageCommand; +import io.quarkus.kafka.client.serialization.ObjectMapperDeserializer; +import org.jboss.logging.Logger; + +/** Never throws on a poison record, for the reason {@code RunCommandDeserializer} gives. */ +public class HarnessImageCommandDeserializer extends ObjectMapperDeserializer { + + private static final Logger LOG = Logger.getLogger(HarnessImageCommandDeserializer.class); + + public HarnessImageCommandDeserializer() { + super(HarnessImageCommand.class); + } + + @Override + public HarnessImageCommand deserialize(String topic, byte[] data) { + try { + return super.deserialize(topic, data); + } catch (RuntimeException undeserializable) { + LOG.errorf(undeserializable, "dropping an undeserializable record on %s", topic); + return null; + } + } +} diff --git a/spire-run-worker/src/main/java/dev/codespire/runworker/HarnessImageCommandSerializer.java b/spire-run-worker/src/main/java/dev/codespire/runworker/HarnessImageCommandSerializer.java new file mode 100644 index 00000000..d654e755 --- /dev/null +++ b/spire-run-worker/src/main/java/dev/codespire/runworker/HarnessImageCommandSerializer.java @@ -0,0 +1,11 @@ +package dev.codespire.runworker; + +import dev.codespire.contract.command.HarnessImageCommand; +import io.quarkus.kafka.client.serialization.ObjectMapperSerializer; + +/** + * Exists for the dead-letter queue, not for producing. The Kafka extension derives the serializer's name + * from the deserializer's, and without it the whole messaging layer refuses to start. + */ +public class HarnessImageCommandSerializer extends ObjectMapperSerializer { +} diff --git a/spire-run-worker/src/main/java/dev/codespire/runworker/HarnessImageResultSerializer.java b/spire-run-worker/src/main/java/dev/codespire/runworker/HarnessImageResultSerializer.java new file mode 100644 index 00000000..0848c0da --- /dev/null +++ b/spire-run-worker/src/main/java/dev/codespire/runworker/HarnessImageResultSerializer.java @@ -0,0 +1,8 @@ +package dev.codespire.runworker; + +import dev.codespire.contract.event.HarnessImageResult; +import io.quarkus.kafka.client.serialization.ObjectMapperSerializer; + +/** Polymorphic JSON on {@code cs.harness-image-results}, keyed by requestId. */ +public class HarnessImageResultSerializer extends ObjectMapperSerializer { +} diff --git a/spire-run-worker/src/main/java/dev/codespire/runworker/HarnessImageWorker.java b/spire-run-worker/src/main/java/dev/codespire/runworker/HarnessImageWorker.java new file mode 100644 index 00000000..4d075156 --- /dev/null +++ b/spire-run-worker/src/main/java/dev/codespire/runworker/HarnessImageWorker.java @@ -0,0 +1,81 @@ +package dev.codespire.runworker; + +import com.fasterxml.jackson.databind.ObjectMapper; +import dev.codespire.contract.command.HarnessImageCommand; +import dev.codespire.contract.event.HarnessImageResult; +import dev.codespire.runtime.RunRuntime; +import io.smallrye.reactive.messaging.annotations.Blocking; +import io.smallrye.reactive.messaging.kafka.Record; +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.inject.Inject; +import org.eclipse.microprofile.config.inject.ConfigProperty; +import org.eclipse.microprofile.reactive.messaging.Channel; +import org.eclipse.microprofile.reactive.messaging.Emitter; +import org.eclipse.microprofile.reactive.messaging.Incoming; +import org.eclipse.microprofile.reactive.messaging.Message; +import org.jboss.logging.Logger; + +import java.util.List; +import java.util.Map; +import java.util.concurrent.CompletionStage; +import java.util.concurrent.TimeUnit; + +/** + * Answers "what does this agent image declare about itself" (M3.5 part M). + * + *

The worker is asked because it is the only component that can reach an agent image at all: the + * orchestrator has no container runtime, and on Kubernetes never will. Reading labels uses the same + * authenticated pull a run uses, so an image a run could start is an image this can describe. + */ +@ApplicationScoped +public class HarnessImageWorker { + + private static final Logger LOG = Logger.getLogger(HarnessImageWorker.class); + + @Inject RunRuntime runtime; + @Inject ObjectMapper mapper; + + @Inject @Channel("harness-image-results-out") + Emitter> results; + + @ConfigProperty(name = "spire.run.result-ack-seconds") + long ackSeconds; + + @Incoming("harness-image-commands-in") + @Blocking(ordered = false) + public CompletionStage onCommand(Message message) { + if (message.getPayload() instanceof HarnessImageCommand.Describe describe) { + publish(describe(describe)); + } + return message.ack(); + } + + HarnessImageResult.Described describe(HarnessImageCommand.Describe command) { + Map labels; + try { + labels = runtime.imageLabels(command.image()); + } catch (RuntimeException unreachable) { + // Named, not rethrown: an image that cannot be pulled is an answer the screen has to give + // ("this image is not available"), and a thrown exception here would only dead-letter it. + LOG.warnf("the image for harness %s could not be read (%s)", command.harness(), + unreachable.getClass().getSimpleName()); + return new HarnessImageResult.Described(command.requestId(), command.harness(), command.image(), + HarnessImageResult.Status.IMAGE_UNAVAILABLE, List.of()); + } + ModelCatalogueLabel.Read read = ModelCatalogueLabel.of(labels, mapper); + return new HarnessImageResult.Described(command.requestId(), command.harness(), command.image(), + read.status(), read.models()); + } + + private void publish(HarnessImageResult result) { + try { + results.send(Record.of(result.requestId(), result)).toCompletableFuture().get(ackSeconds, TimeUnit.SECONDS); + } catch (InterruptedException interrupted) { + Thread.currentThread().interrupt(); + } catch (RuntimeException | java.util.concurrent.ExecutionException + | java.util.concurrent.TimeoutException undelivered) { + // The orchestrator asks again on its own schedule, so a lost answer costs one interval. + LOG.warnf(undelivered, "the description of image %s could not be published", result.requestId()); + } + } +} diff --git a/spire-run-worker/src/main/java/dev/codespire/runworker/ModelCatalogueLabel.java b/spire-run-worker/src/main/java/dev/codespire/runworker/ModelCatalogueLabel.java new file mode 100644 index 00000000..839e901d --- /dev/null +++ b/spire-run-worker/src/main/java/dev/codespire/runworker/ModelCatalogueLabel.java @@ -0,0 +1,67 @@ +package dev.codespire.runworker; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import dev.codespire.contract.event.HarnessImageResult; + +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.Base64; +import java.util.List; +import java.util.Map; + +/** + * Reads the model catalogue an agent image declares in its labels (M3.5 part M). + * + *

The label is written by {@code deploy/agent/build-codex.sh}: base64 of a JSON array whose objects + * use short keys — {@code s} slug, {@code n} display name, {@code d} default level, {@code e} levels, + * {@code v} visibility, {@code p} priority — because the whole value rides in image metadata and every + * byte of it is copied into every registry manifest. + * + *

Pure: no daemon, no broker, so every malformed case can be tested directly. + */ +final class ModelCatalogueLabel { + + /** The label the reference image carries. Declared in the image contract, clause {@code models}. */ + static final String LABEL = "dev.codespire.agent.models"; + + /** What the vendor marks a model it wants offered. Anything else is present but not offered. */ + private static final String SHOWN = "list"; + + private ModelCatalogueLabel() { + } + + /** A status and, only when it is OK, the models. */ + record Read(HarnessImageResult.Status status, List models) { + } + + static Read of(Map labels, ObjectMapper mapper) { + String value = labels.get(LABEL); + // Blank counts as absent: a plain `docker build` of the Dockerfile sets the label from an empty + // build argument, so the key exists with nothing in it. That is "no catalogue", not "broken". + if (value == null || value.isBlank()) return new Read(HarnessImageResult.Status.NO_CATALOGUE, List.of()); + try { + JsonNode array = mapper.readTree(new String(Base64.getDecoder().decode(value.trim()), StandardCharsets.UTF_8)); + if (!array.isArray() || array.isEmpty()) return unreadable(); + List models = new ArrayList<>(); + for (JsonNode entry : array) { + String slug = entry.path("s").asText(""); + if (slug.isBlank()) return unreadable(); + List efforts = new ArrayList<>(); + entry.path("e").forEach(level -> efforts.add(level.asText())); + models.add(new HarnessImageResult.Model(slug, entry.path("n").asText(slug), + entry.path("d").asText(""), efforts, SHOWN.equals(entry.path("v").asText(SHOWN)), + entry.path("p").asInt(Integer.MAX_VALUE))); + } + return new Read(HarnessImageResult.Status.OK, List.copyOf(models)); + } catch (IllegalArgumentException | java.io.IOException notACatalogue) { + // One bad entry makes the WHOLE label unreadable rather than yielding the entries that + // parsed: a list with a silent hole in it looks exactly like a complete list. + return unreadable(); + } + } + + private static Read unreadable() { + return new Read(HarnessImageResult.Status.UNREADABLE, List.of()); + } +} diff --git a/spire-run-worker/src/main/resources/application.yml b/spire-run-worker/src/main/resources/application.yml index bfa9fb34..dce2c9f6 100644 --- a/spire-run-worker/src/main/resources/application.yml +++ b/spire-run-worker/src/main/resources/application.yml @@ -233,7 +233,30 @@ mp: failure-strategy: dead-letter-queue dead-letter-queue: topic: cs.dlq + # What an agent image declares about itself (M3.5 part M). Its own channel: the answer is + # small, fast and idempotent, and must not queue behind a sign-in that is waiting on a person. + harness-image-commands-in: + connector: smallrye-kafka + topic: cs.harness-image-commands + group: + id: spire-run-worker-harness-image + value: + deserializer: dev.codespire.runworker.HarnessImageCommandDeserializer + auto: + offset: + reset: latest + failure-strategy: dead-letter-queue + dead-letter-queue: + topic: cs.dlq outgoing: + harness-image-results-out: + connector: smallrye-kafka + topic: cs.harness-image-results + key: + serializer: org.apache.kafka.common.serialization.StringSerializer + value: + serializer: dev.codespire.runworker.HarnessImageResultSerializer + waitForWriteCompletion: true harness-sign-in-results-out: connector: smallrye-kafka topic: cs.harness-sign-in-results diff --git a/spire-run-worker/src/test/java/dev/codespire/runworker/ModelCatalogueLabelTest.java b/spire-run-worker/src/test/java/dev/codespire/runworker/ModelCatalogueLabelTest.java new file mode 100644 index 00000000..4d9e76cc --- /dev/null +++ b/spire-run-worker/src/test/java/dev/codespire/runworker/ModelCatalogueLabelTest.java @@ -0,0 +1,98 @@ +package dev.codespire.runworker; + +import com.fasterxml.jackson.databind.ObjectMapper; +import dev.codespire.contract.event.HarnessImageResult; +import org.junit.jupiter.api.Test; + +import java.nio.charset.StandardCharsets; +import java.util.Base64; +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * Reading the model catalogue an agent image declares (M3.5 part M). + * + *

The shape below is what {@code deploy/agent/build-codex.sh} writes. The model names are placeholders + * on purpose: the point is the parsing, and a list of real names would go stale the day the vendor ships. + */ +class ModelCatalogueLabelTest { + + private static final ObjectMapper MAPPER = new ObjectMapper(); + + private static String label(String json) { + return Base64.getEncoder().encodeToString(json.getBytes(StandardCharsets.UTF_8)); + } + + private static ModelCatalogueLabel.Read read(String json) { + return ModelCatalogueLabel.of(Map.of(ModelCatalogueLabel.LABEL, label(json)), MAPPER); + } + + @Test + void aWellFormedCatalogueYieldsEveryModelWithItsOwnLevels() { + var read = read(""" + [{"s":"TEST-fast","n":"Test Fast","d":"low","e":["low","medium","high"],"v":"list","p":1}, + {"s":"TEST-deep","n":"Test Deep","d":"high","e":["medium","high","xhigh"],"v":"hide","p":9}] + """); + + assertEquals(HarnessImageResult.Status.OK, read.status()); + assertEquals(List.of("TEST-fast", "TEST-deep"), read.models().stream().map(HarnessImageResult.Model::slug).toList()); + var fast = read.models().getFirst(); + assertEquals("low", fast.defaultEffort(), "each model carries ITS OWN default, not a global one"); + assertEquals(List.of("low", "medium", "high"), fast.efforts()); + assertTrue(fast.visible()); + assertFalse(read.models().get(1).visible(), "hide is read as not offered, not as absent"); + } + + /** + * A plain `docker build` of the Dockerfile sets the label from an EMPTY build argument, so the key is + * present with nothing in it. That is "this image declares nothing", not "this image is broken". + */ + @Test + void aBlankLabelIsNoCatalogueRatherThanUnreadable() { + assertEquals(HarnessImageResult.Status.NO_CATALOGUE, + ModelCatalogueLabel.of(Map.of(ModelCatalogueLabel.LABEL, ""), MAPPER).status()); + assertEquals(HarnessImageResult.Status.NO_CATALOGUE, ModelCatalogueLabel.of(Map.of(), MAPPER).status()); + } + + @Test + void somethingThatIsNotBase64IsUnreadable() { + var read = ModelCatalogueLabel.of(Map.of(ModelCatalogueLabel.LABEL, "not base64 at all!"), MAPPER); + assertEquals(HarnessImageResult.Status.UNREADABLE, read.status()); + assertTrue(read.models().isEmpty()); + } + + @Test + void base64ThatIsNotACatalogueIsUnreadable() { + assertEquals(HarnessImageResult.Status.UNREADABLE, read("{\"not\":\"an array\"}").status()); + assertEquals(HarnessImageResult.Status.UNREADABLE, read("[]").status(), "an empty list declares nothing usable"); + } + + /** + * One broken entry spoils the whole label rather than yielding the entries that parsed. + * + *

A list with a silent hole in it looks exactly like a complete list, and the model an operator is + * looking for is always the one in the hole. + */ + @Test + void oneEntryWithoutASlugMakesTheWholeLabelUnreadable() { + var read = read(""" + [{"s":"TEST-fine","d":"low","e":["low"],"v":"list","p":1}, + {"n":"no slug here","d":"low","e":["low"]}] + """); + + assertEquals(HarnessImageResult.Status.UNREADABLE, read.status()); + assertTrue(read.models().isEmpty(), "partial answers are not answers"); + } + + /** Missing optional fields do not make an entry unusable; missing the slug does. */ + @Test + void aModelWithOnlyASlugIsStillAModel() { + var model = read("[{\"s\":\"TEST-bare\"}]").models().getFirst(); + + assertEquals("TEST-bare", model.displayName(), "the slug stands in for a missing display name"); + assertEquals(List.of(), model.efforts()); + assertTrue(model.visible(), "no visibility stated reads as offered"); + } +} diff --git a/spire-runtime-docker/src/main/java/dev/codespire/runtime/docker/DockerRunRuntime.java b/spire-runtime-docker/src/main/java/dev/codespire/runtime/docker/DockerRunRuntime.java index 6e058d80..2e1667e1 100644 --- a/spire-runtime-docker/src/main/java/dev/codespire/runtime/docker/DockerRunRuntime.java +++ b/spire-runtime-docker/src/main/java/dev/codespire/runtime/docker/DockerRunRuntime.java @@ -324,6 +324,15 @@ private static String digestOf(String runId) { } } + @Override + public java.util.Map imageLabels(String image) { + // The SAME authenticated pull a run uses, so a private registry needs nothing new: an image a + // run could start is an image whose labels can be read, and the reverse. + ensureImage(image); + var config = client.inspectImageCmd(image).exec().getConfig(); + return config == null || config.getLabels() == null ? java.util.Map.of() : java.util.Map.copyOf(config.getLabels()); + } + /** * Pulls the image when the daemon does not hold it. An operator's agent image lives in a * registry and a digest-pinned reference (FR-F13) is the normal case, not a local tag — the diff --git a/spire-runtime-docker/src/test/java/dev/codespire/runtime/docker/DockerImageLabelsIT.java b/spire-runtime-docker/src/test/java/dev/codespire/runtime/docker/DockerImageLabelsIT.java new file mode 100644 index 00000000..e72077a7 --- /dev/null +++ b/spire-runtime-docker/src/test/java/dev/codespire/runtime/docker/DockerImageLabelsIT.java @@ -0,0 +1,57 @@ +package dev.codespire.runtime.docker; + +import org.junit.jupiter.api.Test; + +import java.nio.charset.StandardCharsets; +import java.util.Base64; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.*; +import static org.junit.jupiter.api.Assumptions.assumeTrue; + +/** + * Reading an agent image's labels through the run arm, against a real daemon (M3.5 part M). + * + *

The claim under test is that the factory can learn which models an image runs WITHOUT running it — + * which is the whole reason the catalogue is a label and not a question. So this reads metadata only and + * starts no container. + */ +class DockerImageLabelsIT { + + private static final String IMAGE = "spire-agent-codex:latest"; + private static final String MODELS_LABEL = "dev.codespire.agent.models"; + + private final DockerRunRuntime runtime = new DockerRunRuntime(); + + private boolean imagePresent() { + try { runtime.client().inspectImageCmd(IMAGE).exec(); return true; } + catch (RuntimeException absent) { return false; } + } + + @Test + void theReferenceImageDeclaresItsModelsInItsLabels() { + assumeTrue(imagePresent(), IMAGE + " is not built on this machine"); + int before = runtime.client().listContainersCmd().withShowAll(true).exec().size(); + + Map labels = runtime.imageLabels(IMAGE); + + assertEquals("codex", labels.get("dev.codespire.agent.harness")); + String catalogue = labels.get(MODELS_LABEL); + // Skip rather than fail when the image was built with plain `docker build`: that is a supported + // way to build it, and it produces an agent that runs but declares no models. + assumeTrue(catalogue != null && !catalogue.isBlank(), + IMAGE + " was built without deploy/agent/build-codex.sh, so it declares no models"); + String decoded = new String(Base64.getDecoder().decode(catalogue), StandardCharsets.UTF_8); + assertTrue(decoded.startsWith("[") && decoded.contains("\"s\":"), "a JSON array of models: " + decoded); + + assertEquals(before, runtime.client().listContainersCmd().withShowAll(true).exec().size(), + "learning what an image runs must not run it"); + } + + /** An image that does not exist is a failure to REACH it, not an image that declares nothing. */ + @Test + void anImageThatCannotBeReachedIsAFailureNotAnEmptyAnswer() { + assertThrows(RuntimeException.class, + () -> runtime.imageLabels("spire-test-no-such-image-" + System.nanoTime() + ":never")); + } +} diff --git a/spire-runtime/src/main/java/dev/codespire/runtime/RunRuntime.java b/spire-runtime/src/main/java/dev/codespire/runtime/RunRuntime.java index a3b7df2e..4cf38de1 100644 --- a/spire-runtime/src/main/java/dev/codespire/runtime/RunRuntime.java +++ b/spire-runtime/src/main/java/dev/codespire/runtime/RunRuntime.java @@ -68,4 +68,22 @@ public interface RunRuntime { * the Docker arm's went 30s to 300s once and a guard that did not read it kept passing. */ Duration drainWindow(); + + /** + * The labels an image carries, fetching it first if this arm does not hold it (M3.5 part M). + * + *

On this port rather than a new one because the registry credential lives here and nowhere + * else: it authenticates a pull and must reach nothing but a pull. Reading an image's labels is the + * one thing every arm must already be able to do, since it cannot start a unit otherwise — on Docker + * by inspecting the image, on Kubernetes by reading the registry manifest. + * + *

A default that THROWS rather than answering empty. An empty map would read as "this image + * declares nothing", which is a real answer with a real screen; an arm that cannot look at all is a + * different fact and must not be mistaken for it. + * + * @throws UnsupportedOperationException when this arm cannot read image metadata + */ + default java.util.Map imageLabels(String image) { + throw new UnsupportedOperationException(type() + " cannot read image labels"); + } } From b338563783723f510c5ba42e6d9ff2dd0d68dac1 Mon Sep 17 00:00:00 2001 From: Artjoms Stukans Date: Tue, 22 Sep 2026 22:00:27 +0200 Subject: [PATCH 3/7] Offer the models a harness runs, and the thinking level with the model The build setup now offers the models the chosen harness's image says it runs, not every model on the price list. That is the defect the operator reported: codex was offered Claude and Gemini, and a save accepted one. Once the list is known, a model the harness cannot run is refused at save -- here, where it can be fixed -- rather than when the run starts, after an approval. The price list keeps its real job. It no longer decides WHICH models are offered, only whether each can be paid for: a model the harness runs but nobody has priced is shown, so the operator learns it exists, and blocked with "no price yet", because an API-key run needs a rate for every token type the harness reports. A thinking level is chosen with the model, because the levels belong to it. Codex itself offers "Model and Effort" together, and the levels differ per model -- one allows low to ultra, another only low to xhigh -- so the list is rebuilt from the chosen model and checked against that model's own list at save. No level chosen means the model's own default, a real choice the vendor publishes, stored as NULL (V80) rather than as a guessed name. A level chosen for one model is cleared when the model changes, since the next one may not offer it. When the harness's list is NOT known the save is not blocked, and that is a decision: a development stack with no run worker never hears the answer, and refusing every save would lock the operator out of the build setup over a reply that has not come. The screen says which of the four reasons it is and falls back to the price list, as before. A thinking level is refused in that case, since there is nothing to check it against. Recorded in the design as 6A.4a. Mutation-verified: removing the harness check fails three tests. --- ...actory-m35-one-ticket-to-a-build-design.md | 18 ++++ .../orchestrator/factory/BuildDefaults.java | 57 ++++++++++-- .../migration/V80__build_defaults_effort.sql | 9 ++ .../factory/BuildDefaultsTest.java | 83 +++++++++++++++++ .../factory/BuildModelFields.test.tsx | 90 +++++++++++++++++++ .../repositories/factory/BuildModelFields.tsx | 88 ++++++++++++++++++ .../repositories/factory/BuildStep.tsx | 58 +++++------- .../factory/RepositoryFactory.test.tsx | 3 +- .../repositories/factory/buildDefaultsApi.ts | 29 +++++- 9 files changed, 387 insertions(+), 48 deletions(-) create mode 100644 spire-orchestrator/src/main/resources/db/migration/V80__build_defaults_effort.sql create mode 100644 spire-ui/src/components/repositories/factory/BuildModelFields.test.tsx create mode 100644 spire-ui/src/components/repositories/factory/BuildModelFields.tsx diff --git a/docs/superpowers/specs/2026-09-16-factory-m35-one-ticket-to-a-build-design.md b/docs/superpowers/specs/2026-09-16-factory-m35-one-ticket-to-a-build-design.md index e6b4f586..3a167478 100644 --- a/docs/superpowers/specs/2026-09-16-factory-m35-one-ticket-to-a-build-design.md +++ b/docs/superpowers/specs/2026-09-16-factory-m35-one-ticket-to-a-build-design.md @@ -557,6 +557,24 @@ Factory tab, step 5 becomes: base branch, harness, **pay with**, model, **thinki comes from the harness; the levels are the ones THAT model declares, defaulting to its own default. An API-key choice additionally requires a priced model and says so by name. +### 6A.4a When the list is not known + +A save is refused for a model the harness cannot run **only when the harness's list is known.** When it +is not — the run worker has not answered, the image was built without its catalogue, the label could not +be read, or the image could not be reached — the save goes ahead, the screen says which of those four it +is, and the model select falls back to the price list, which is what it offered before. + +That is a decision, not a gap. The check exists to stop an avoidable wrong choice; when nothing can know +which choice is wrong, refusing every save would lock the operator out of the build setup over a +background answer that has not arrived. A development stack with no run worker would never hear the +answer at all, and could never save a build setup. The case this lets through is the one that existed +before part M: a model the harness cannot run is refused when the run starts. + +A **thinking level** is refused when the list is unknown. There is nothing to check it against, and a +level the model does not offer would reach the vendor as it stands. With no level chosen, the model's own +default applies, which is a real choice the vendor publishes per model — so it is stored as NULL rather +than as a guessed name. + ### 6A.5 The cost, stated The image contract gains a clause, so an operator building their own agent image must produce that label diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/BuildDefaults.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/BuildDefaults.java index 869d27a6..7bb0c904 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/BuildDefaults.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/BuildDefaults.java @@ -23,17 +23,28 @@ public class BuildDefaults { @Inject FactoryConfig config; @Inject LlmModelRegistry models; @Inject dev.codespire.orchestrator.llm.LlmModelPricer pricer; + @Inject HarnessCatalogues catalogues; /** * @param revision 0 when the repository has none yet, with null coordinates — "not set" is a state * the setup screen has to render, and a zero-revision row is the same answer a save rejects */ - public record Defaults(long revision, String baseBranch, String harness, String model, + /** + * @param effort the thinking level, or null for the model's own default — a real choice, not a gap + */ + public record Defaults(long revision, String baseBranch, String harness, String model, String effort, String updatedBy, Instant updatedAt) { - public static Defaults none() { return new Defaults(0, null, null, null, null, null); } + public static Defaults none() { return new Defaults(0, null, null, null, null, null, null); } public boolean set() { return revision > 0; } } - public record Input(long expectedRevision, String baseBranch, String harness, String model) {} + + /** @param effort null for the model's own default */ + public record Input(long expectedRevision, String baseBranch, String harness, String model, String effort) { + /** Every caller written before thinking levels existed keeps the model's own default. */ + public Input(long expectedRevision, String baseBranch, String harness, String model) { + this(expectedRevision, baseBranch, harness, model, null); + } + } /** A refusal that names its rule, so the screen can say what to change rather than "400". */ public static final class Refused extends RuntimeException { @@ -52,14 +63,14 @@ public Defaults get(UUID repository) { * against the saved one INSIDE the transaction that registers the result (M3.5 part C). */ public Defaults get(Connection c, UUID repository, boolean lock) throws SQLException { - String sql = "SELECT revision,base_branch,harness,model,updated_by,updated_at" + String sql = "SELECT revision,base_branch,harness,model,effort,updated_by,updated_at" + " FROM repository_build_defaults WHERE repository_id=?" + (lock ? " FOR UPDATE" : ""); try (PreparedStatement ps = c.prepareStatement(sql)) { ps.setObject(1, repository); try (ResultSet rs = ps.executeQuery()) { if (!rs.next()) return Defaults.none(); return new Defaults(rs.getLong(1), rs.getString(2), rs.getString(3), rs.getString(4), - rs.getString(5), rs.getTimestamp(6).toInstant()); + rs.getString(5), rs.getString(6), rs.getTimestamp(7).toInstant()); } } } @@ -75,6 +86,7 @@ public Defaults save(UUID repository, Input input, String actor) { if (input == null) throw new Refused("build_defaults_required"); if (actor == null || actor.isBlank()) throw new Refused("operator_identity_required"); String branch = strip(input.baseBranch()), harness = strip(input.harness()), model = strip(input.model()); + String effort = strip(input.effort()); if (branch == null) throw new Refused("base_branch_blank"); // The branch, the harness and the model are checked against the rules the DISPATCH applies, not // against a second opinion written here. A value that passes here and fails there would put the @@ -84,6 +96,7 @@ public Defaults save(UUID repository, Input input, String actor) { if (harness == null || !config.agentImage().containsKey(harness)) throw new Refused("harness_unconfigured"); if (model == null || !DispatchRequestParser.isModelName(model)) throw new Refused("model_name_invalid"); + checkAgainstTheHarness(harness, model, effort); // Exactly what the dispatch refuses, asked here: the model must be offered, and it must price // every token type this harness can report. A model disabled AFTER this save is refused at // dispatch too (WorkRunAssembly), so the two no longer disagree in either direction. @@ -102,14 +115,14 @@ public Defaults save(UUID repository, Input input, String actor) { if (get(c, repository, true).revision() != input.expectedRevision()) throw new Refused("build_defaults_changed"); try (PreparedStatement ps = c.prepareStatement(""" - INSERT INTO repository_build_defaults(repository_id,base_branch,harness,model,updated_by) - VALUES (?,?,?,?,?) + INSERT INTO repository_build_defaults(repository_id,base_branch,harness,model,effort,updated_by) + VALUES (?,?,?,?,?,?) ON CONFLICT (repository_id) DO UPDATE SET base_branch=excluded.base_branch,harness=excluded.harness, - model=excluded.model,updated_by=excluded.updated_by,updated_at=now(), + model=excluded.model,effort=excluded.effort,updated_by=excluded.updated_by,updated_at=now(), revision=repository_build_defaults.revision+1 """)) { ps.setObject(1, repository); ps.setString(2, branch); ps.setString(3, harness); - ps.setString(4, model); ps.setString(5, actor); + ps.setString(4, model); ps.setString(5, effort); ps.setString(6, actor); ps.executeUpdate(); } // Saving a setup is the repair for "this repository has no build setup", so the items that @@ -125,6 +138,32 @@ WHERE work_item_id IN (SELECT id FROM work_item WHERE repository_id=?) } catch (SQLException failure) { throw database(failure); } } + /** + * Whether the chosen harness can run this model, at this thinking level (M3.5 part M). + * + *

Only when the harness's own list is known. When it is not — no answer from the run worker + * yet, an image built without its catalogue, one that could not be read or reached — this does NOT + * refuse, and that is a decision rather than an oversight. The check exists to stop an avoidable + * wrong choice; when nothing can know which choice is wrong, refusing every save would lock the + * operator out of the build setup altogether over a background answer that has not arrived, and a + * development stack with no run worker would never be able to save one at all. The failure it lets + * through is the one that existed before this check — a model the harness cannot run is refused when + * the run starts — and the screen says the list could not be read, so the gap is visible. + * + *

A thinking level, though, IS refused when the list is unknown. There is nothing to check it + * against, and a level the model does not offer would be passed to the vendor as it stands. + */ + private void checkAgainstTheHarness(String harness, String model, String effort) { + var catalogue = catalogues.get(harness) + .filter(known -> known.status() == dev.codespire.contract.event.HarnessImageResult.Status.OK); + if (catalogue.isEmpty()) { + if (effort != null) throw new Refused("effort_unverifiable"); + return; + } + var runs = catalogue.get().find(model).orElseThrow(() -> new Refused("model_not_run_by_harness")); + if (effort != null && !runs.efforts().contains(effort)) throw new Refused("effort_not_offered"); + } + private static IllegalStateException database(SQLException failure) { return new IllegalStateException("The repository build defaults could not be read or saved", failure); } diff --git a/spire-orchestrator/src/main/resources/db/migration/V80__build_defaults_effort.sql b/spire-orchestrator/src/main/resources/db/migration/V80__build_defaults_effort.sql new file mode 100644 index 00000000..0b8a3514 --- /dev/null +++ b/spire-orchestrator/src/main/resources/db/migration/V80__build_defaults_effort.sql @@ -0,0 +1,9 @@ +-- How hard the model thinks, chosen with the model (M3.5 part M). +-- +-- Codex offers a thinking level per model, and the levels differ by model: measured 2026-09-18, one +-- allows low..ultra and another only low..xhigh. So it is stored beside the model it belongs to, and it +-- is checked against THAT model's own list when the harness's catalogue is known. +-- +-- NULL means "the model's own default", which is a real choice and not a missing value: the vendor +-- publishes a default per model, and an operator who has not picked one gets that one. +ALTER TABLE repository_build_defaults ADD COLUMN effort TEXT; diff --git a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/BuildDefaultsTest.java b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/BuildDefaultsTest.java index 1bac29dd..b3e9629e 100644 --- a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/BuildDefaultsTest.java +++ b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/BuildDefaultsTest.java @@ -73,6 +73,89 @@ private BuildDefaults.Input input(String branch, String harness, String model, l return new BuildDefaults.Input(revision, branch, harness, model); } + @Inject HarnessCatalogues catalogues; + @Inject FactoryConfig config; + + /** + * Tells the cache what the codex image declares, as the run worker would. + * + *

Two placeholder levels per model and different ones for each, because the rule under test is + * that a level is checked against THAT model's own list, not against a list shared by all. + */ + private void codexDeclares(String... slugs) { + List declared = new java.util.ArrayList<>(); + for (String slug : slugs) { + declared.add(new dev.codespire.contract.event.HarnessImageResult.Model(slug, slug, "medium", + List.of("medium", slug.equals(model) ? "high" : "low"), true, 1)); + } + catalogues.record(new dev.codespire.contract.event.HarnessImageResult.Described("TEST-request", "codex", + config.agentImage().get("codex"), dev.codespire.contract.event.HarnessImageResult.Status.OK, declared)); + } + + @org.junit.jupiter.api.AfterEach + void forgetTheCatalogue() throws java.sql.SQLException { + try (var c = dataSource.getConnection(); var s = c.createStatement()) { + s.executeUpdate("DELETE FROM harness_catalogue"); + } + } + + /** + * The defect the operator found: codex was offered Claude and Gemini models, and a save accepted one. + * Once the image says what it runs, a model outside that list is refused HERE — not when the run + * starts, after somebody approved it. + */ + @Test + void aModelTheHarnessCannotRunIsRefusedOnceItsListIsKnown() { + codexDeclares("TEST-some-other-model"); + + assertEquals("model_not_run_by_harness", refusal(input("main", "codex", model, 0))); + } + + @Test + void aModelTheHarnessCanRunIsAccepted() { + codexDeclares(model); + + assertEquals(model, defaults.save(repository, input("main", "codex", model, 0), "TEST-operator").model()); + } + + /** + * When the list is NOT known, the save is not blocked. + * + *

Deliberate. A development stack with no run worker never hears the answer, and refusing every + * save would lock the operator out of the build setup over a background reply that has not come. The + * wrong-model case this lets through is the one that existed before, and the screen says so. + */ + @Test + void anUnknownListDoesNotLockTheBuildSetup() { + assertEquals(model, defaults.save(repository, input("main", "codex", model, 0), "TEST-operator").model()); + } + + /** A thinking level is the model's OWN list — this model allows "high", the other only "low". */ + @Test + void aThinkingLevelIsCheckedAgainstThatModelsOwnList() { + codexDeclares(model, "TEST-other"); + + var saved = defaults.save(repository, new BuildDefaults.Input(0, "main", "codex", model, "high"), "TEST-operator"); + assertEquals("high", saved.effort()); + assertEquals("effort_not_offered", + refusal(new BuildDefaults.Input(1, "main", "codex", model, "low")), + "another model's level is not this model's"); + } + + /** With nothing to check it against, a level would reach the vendor unverified, so it is refused. */ + @Test + void aThinkingLevelIsRefusedWhenTheListIsUnknown() { + assertEquals("effort_unverifiable", refusal(new BuildDefaults.Input(0, "main", "codex", model, "high"))); + } + + /** No level chosen means the model's own default — a real choice, stored as such. */ + @Test + void noThinkingLevelMeansTheModelsOwnDefault() { + codexDeclares(model); + + assertNull(defaults.save(repository, input("main", "codex", model, 0), "TEST-operator").effort()); + } + private String refusal(BuildDefaults.Input input) { return assertThrows(BuildDefaults.Refused.class, () -> defaults.save(repository, input, "TEST-operator")).reason(); } diff --git a/spire-ui/src/components/repositories/factory/BuildModelFields.test.tsx b/spire-ui/src/components/repositories/factory/BuildModelFields.test.tsx new file mode 100644 index 00000000..443c9b3e --- /dev/null +++ b/spire-ui/src/components/repositories/factory/BuildModelFields.test.tsx @@ -0,0 +1,90 @@ +import { cleanup, fireEvent, render, screen } from '@testing-library/react'; +import { afterEach, expect, it } from 'vitest'; +import type { LlmModelView } from '../../../api'; +import BuildModelFields, { modelChoices } from './BuildModelFields'; +import type { HarnessModels } from './buildDefaultsApi'; + +afterEach(cleanup); + +/** A priced catalogue entry. Placeholder names: this is about the rules, not today's vendor list. */ +function priced(name: string, notBilled: string[] = []): LlmModelView { + return { + id: `TEST-${name}`, type: 'openai', name, label: name, pricingMode: 'METERED', + rates: { INPUT: 1, OUTPUT: 1 }, outputTokenParam: 'MAX_TOKENS', supportsTemperature: true, + reasoningEffort: null, extraParams: {}, enabled: true, createdAt: '1970-01-01T00:00:00Z', notBilled, + } as unknown as LlmModelView; +} + +const REPORTED = { codex: ['INPUT', 'OUTPUT'] }; + +const KNOWN: HarnessModels = { + status: 'OK', + offered: [ + { slug: 'TEST-fast', displayName: 'Test Fast', defaultEffort: 'low', efforts: ['low', 'medium'], visible: true, priority: 1 }, + { slug: 'TEST-deep', displayName: 'Test Deep', defaultEffort: 'high', efforts: ['medium', 'high', 'xhigh'], visible: true, priority: 2 }, + ], +}; + +// The operator's report: codex was offered Claude and Gemini. When the image says what it runs, the +// price list must not add models of its own — it only decides whether each one can be paid for. +it('offers the models the harness runs, not everything on the price list', () => { + const choices = modelChoices('codex', KNOWN, [priced('TEST-fast'), priced('TEST-claude'), priced('TEST-gemini')], REPORTED); + + expect(choices.map(choice => choice.value)).toEqual(['TEST-fast', 'TEST-deep']); +}); + +// A model the harness runs but nobody has priced yet is SHOWN — so the operator learns it exists — and +// blocked with the reason, because an API-key run cannot be paid for without a rate. +it('shows a model the harness runs but that has no price, and says what is missing', () => { + const choices = modelChoices('codex', KNOWN, [priced('TEST-fast')], REPORTED); + + const deep = choices.find(choice => choice.value === 'TEST-deep'); + expect(deep?.blocked).toMatch(/no price yet/); + expect(choices.find(choice => choice.value === 'TEST-fast')?.blocked).toBeNull(); +}); + +// When the image did not say, the price list is all there is — what this screen offered before — and it +// is not dressed up as the harness's own list. +it('falls back to the price list, and says why, when the harness list is unknown', () => { + const unknown: HarnessModels = { status: 'NO_CATALOGUE', offered: [] }; + const choices = modelChoices('codex', unknown, [priced('TEST-any')], REPORTED); + + expect(choices.map(choice => choice.value)).toEqual(['TEST-any']); + + render( { }} setEffort={() => { }} />); + expect(screen.getByRole('status')).toHaveTextContent(/built without deploy\/agent\/build-codex\.sh/); +}); + +// The levels belong to the model: Codex itself offers "Model and Effort" together, and they differ per +// model. Choosing a different model must rebuild the list from THAT model. +it('offers the thinking levels of the chosen model, with its own default named', () => { + const choices = modelChoices('codex', KNOWN, [priced('TEST-fast'), priced('TEST-deep')], REPORTED); + render( { }} setEffort={() => { }} />); + + const levels = Array.from(screen.getByRole('combobox', { name: 'Thinking level' }).querySelectorAll('option')).map(option => option.textContent); + expect(levels).toEqual(['Model default (high)', 'medium', 'high', 'xhigh']); +}); + +// Nothing honest to offer when the image did not say which levels exist, and the save refuses a level it +// cannot check — so no level control is shown at all. +it('offers no thinking level when the harness list is unknown', () => { + const unknown: HarnessModels = { status: 'UNKNOWN', offered: [] }; + render( { }} setEffort={() => { }} />); + + expect(screen.queryByRole('combobox', { name: 'Thinking level' })).not.toBeInTheDocument(); +}); + +// A level chosen for one model is not carried to the next: another model may not offer it at all. +it('clears the thinking level when the model changes', () => { + let cleared = 'not-called'; + const choices = modelChoices('codex', KNOWN, [priced('TEST-fast'), priced('TEST-deep')], REPORTED); + render( { }} setEffort={effort => { cleared = effort; }} />); + + fireEvent.change(screen.getByRole('combobox', { name: 'Model' }), { target: { value: 'TEST-fast' } }); + + expect(cleared).toBe(''); +}); diff --git a/spire-ui/src/components/repositories/factory/BuildModelFields.tsx b/spire-ui/src/components/repositories/factory/BuildModelFields.tsx new file mode 100644 index 00000000..d556d2e0 --- /dev/null +++ b/spire-ui/src/components/repositories/factory/BuildModelFields.tsx @@ -0,0 +1,88 @@ +import type { LlmModelView } from '../../../api'; +import { TOKEN_TYPE_LABEL, unpricedTypesFor } from '../../../llmPricing'; +import SettingField from '../../SettingField'; +import type { HarnessModels } from './buildDefaultsApi'; + +/** Why a harness has no model list, as a sentence. The empty dropdown alone could mean any of these. */ +const NO_LIST: Record = { + UNKNOWN: 'The run worker has not said which models this harness runs yet. Showing the price list instead.', + NO_CATALOGUE: 'This agent image does not say which models it runs — it was built without deploy/agent/build-codex.sh. Showing the price list instead.', + UNREADABLE: 'This agent image declares a model list that could not be read. Showing the price list instead.', + IMAGE_UNAVAILABLE: 'The agent image could not be reached, so its model list is unknown. Showing the price list instead.', +}; + +const missingLabel = (types: string[]) => types.map(type => TOKEN_TYPE_LABEL[type as keyof typeof TOKEN_TYPE_LABEL]).join(', '); + +/** One choosable model: what it is called, and why it cannot be chosen when it cannot. */ +export interface ModelChoice { value: string; label: string; blocked: string | null } + +/** + * The models to offer for this harness, and what blocks each (M3.5 part M). + * + *

When the image says what it runs, THAT is the list — the price list only decides whether each one + * can be paid for. When it does not, the price list is all there is, which is what this screen offered + * before, and the screen says so rather than presenting it as the harness's own list. + */ +export function modelChoices(harness: string, known: HarnessModels | undefined, priced: LlmModelView[], + reported: Record): ModelChoice[] { + const unpriced = (model: LlmModelView) => (harness ? unpricedTypesFor(model, reported[harness]) : []); + if (known?.status === 'OK') { + return known.offered.map(model => { + const price = priced.find(entry => entry.name === model.slug); + const missing = price ? unpriced(price) : []; + return { + value: model.slug, + label: model.displayName === model.slug ? model.slug : `${model.displayName} (${model.slug})`, + // Not offered as runnable until it can be paid for: an API-key run needs a rate for every type + // the harness reports, and the save would refuse it anyway. + blocked: !price ? 'no price yet — add it in Settings → LLM' + : missing.length ? `no price for ${missingLabel(missing)}` : null, + }; + }); + } + return priced.map(model => { + const missing = unpriced(model); + return { + value: model.name, + label: model.name === model.label ? model.label : `${model.label} (${model.name})`, + blocked: missing.length ? `no price for ${missingLabel(missing)}` : null, + }; + }); +} + +interface Props { + harness: string; + known: HarnessModels | undefined; + choices: ModelChoice[]; + model: string; + effort: string; + setModel: (model: string) => void; + setEffort: (effort: string) => void; +} + +/** + * The model and its thinking level, chosen together because the levels belong to the model. + * + *

Codex shows its models as "Select Model and Effort", and the levels differ per model — measured + * 2026-09-18, one allows low to ultra and another only low to xhigh. So the level list is rebuilt from the + * chosen model every time, and it is offered only when the image said which levels exist: there is nothing + * honest to offer otherwise, and the save refuses a level it cannot check. + */ +export default function BuildModelFields({ harness, known, choices, model, effort, setModel, setEffort }: Props) { + const runs = known?.status === 'OK' ? known.offered.find(entry => entry.slug === model) : undefined; + return <> + {harness && known && known.status !== 'OK' &&

{NO_LIST[known.status]}

} + + + {runs && + } + ; +} diff --git a/spire-ui/src/components/repositories/factory/BuildStep.tsx b/spire-ui/src/components/repositories/factory/BuildStep.tsx index 4de082a4..fe74890b 100644 --- a/spire-ui/src/components/repositories/factory/BuildStep.tsx +++ b/spire-ui/src/components/repositories/factory/BuildStep.tsx @@ -1,9 +1,9 @@ import { useEffect, useRef, useState } from 'react'; import { fetchLlmModels, type LlmModelView } from '../../../api'; -import { TOKEN_TYPE_LABEL, unpricedTypesFor } from '../../../llmPricing'; import SettingField from '../../SettingField'; +import BuildModelFields, { modelChoices } from './BuildModelFields'; import FactoryStep from './FactoryStep'; -import { buildOptions, repositoryBranchHead, saveBuildDefaults, type BuildDefaults } from './buildDefaultsApi'; +import { buildOptions, repositoryBranchHead, saveBuildDefaults, type BuildDefaults, type HarnessModels } from './buildDefaultsApi'; interface Props { repositoryId: string; @@ -15,18 +15,6 @@ interface Props { reload: () => void; } -/** - * What this model cannot price of what the chosen harness reports; empty means a run may start. - * - *

With NO harness chosen the question has no answer yet, so nothing is judged: a missing entry for a - * real harness name means "assume it reports everything", but an empty selection is not a harness. - */ -function unpriced(model: LlmModelView, harness: string, reported: Record) { - return harness ? unpricedTypesFor(model, reported[harness]) : []; -} - -const missingLabel = (types: string[]) => types.map(type => TOKEN_TYPE_LABEL[type as keyof typeof TOKEN_TYPE_LABEL]).join(', '); - /** * How this repository builds: the coordinates every prepared task copies (M3.5 part B). * @@ -40,9 +28,10 @@ export default function BuildStep({ repositoryId, defaults, open, setOpen, chang useEffect(() => { live.current = true; return () => { live.current = false; }; }, []); const [form, setForm] = useState({ baseBranch: defaults.baseBranch ?? '', harness: defaults.harness ?? '', model: defaults.model ?? '', + effort: defaults.effort ?? '', }); - const [choices, setChoices] = useState<{ harnesses: string[]; models: LlmModelView[]; reportedTypes: Record }>( - { harnesses: [], models: [], reportedTypes: {} }); + const [choices, setChoices] = useState<{ harnesses: string[]; models: LlmModelView[]; reportedTypes: Record; + harnessModels: Record }>({ harnesses: [], models: [], reportedTypes: {}, harnessModels: {} }); const [busy, setBusy] = useState<'saving' | 'head' | null>(null); const [error, setError] = useState(''), [head, setHead] = useState<{ commit: string; account: string } | null>(null); const editing = open === 'build'; @@ -53,7 +42,8 @@ export default function BuildStep({ repositoryId, defaults, open, setOpen, chang Promise.all([buildOptions(repositoryId), fetchLlmModels()]) // A wire answer of the wrong shape offers nothing rather than blanking the step. .then(([options, models]) => { if (active) setChoices({ harnesses: options.harnesses ?? [], - models: (models ?? []).filter(model => model.enabled), reportedTypes: options.reportedTypes ?? {} }); }) + models: (models ?? []).filter(model => model.enabled), reportedTypes: options.reportedTypes ?? {}, + harnessModels: options.models ?? {} }); }) .catch(() => { /* the selects fall back to what is already saved; the save still refuses an unrunnable pair */ }); return () => { active = false; }; }, [repositoryId, editing]); @@ -70,24 +60,26 @@ export default function BuildStep({ repositoryId, defaults, open, setOpen, chang async function submit() { setBusy('saving'); setError(''); try { - await saveBuildDefaults(repositoryId, { expectedRevision: defaults.revision, ...form }); - if (live.current) changed(`Build setup saved: ${form.harness} on ${form.model}, starting from ${form.baseBranch}.`); + // An empty level is the model's own default, sent as null rather than as a level called "". + await saveBuildDefaults(repositoryId, { expectedRevision: defaults.revision, ...form, effort: form.effort || null }); + if (live.current) changed(`Build setup saved: ${form.harness} on ${form.model}${form.effort ? ` (${form.effort})` : ''}, starting from ${form.baseBranch}.`); } catch (failure) { if (live.current) setError(String(failure instanceof Error ? failure.message : failure)); } finally { if (live.current) setBusy(null); } } // The chosen PAIR, not just three non-empty fields. Choosing a model and then changing the harness // can leave a selection the dispatch refuses; the option goes grey, and Save used to stay live. - const chosen = choices.models.find(model => model.name === form.model); - const missing = chosen ? unpriced(chosen, form.harness, choices.reportedTypes) : []; + const known = choices.harnessModels[form.harness]; + const offered = modelChoices(form.harness, known, choices.models, choices.reportedTypes); + const picked = offered.find(choice => choice.value === form.model); // An unresolved model is UNKNOWN, not complete: before the catalogue answers, and for a saved name the // catalogue no longer offers, there is nothing to judge — so Save waits rather than guessing. - const complete = !!form.baseBranch.trim() && !!form.harness && !!chosen && missing.length === 0; + const complete = !!form.baseBranch.trim() && !!form.harness && !!picked && picked.blocked === null; return 0 ? 'done' : 'missing'} actions={!editing && }> {defaults.revision > 0 - ?

{defaults.harness} · {defaults.model}starts from {defaults.baseBranch}
+ ?
{defaults.harness} · {defaults.model}{defaults.effort ? ` · ${defaults.effort}` : ''}starts from {defaults.baseBranch}
:

Not set, so every ticket has to be given a branch, a harness and a model by hand.

}

A prepared task copies these. Changing them here never changes a decision that is already open.

{editing &&
@@ -105,20 +97,12 @@ export default function BuildStep({ repositoryId, defaults, open, setOpen, chang {choices.harnesses.map(harness => )} {form.harness && !choices.harnesses.includes(form.harness) && } - - - {missing.length > 0 &&

- {chosen?.label ?? form.model} has no price for {missingLabel(missing)}, which {form.harness} reports. - Enter each rate in Settings → LLM, or mark the type as one this vendor does not bill.

} + setForm(previous => ({ ...previous, model }))} + setEffort={effort => setForm(previous => ({ ...previous, effort }))} /> + {picked?.blocked &&

+ {form.model} has {picked.blocked}. {form.harness} reports those token types, and an API-key run needs a rate + for each — or a mark in Settings → LLM that the vendor does not bill it.

} {error &&

{error}

} {/* A stale revision cannot be retried from this form: every attempt resends the number it loaded. */} {error.includes('Reload it') &&
diff --git a/spire-ui/src/components/repositories/factory/RepositoryFactory.test.tsx b/spire-ui/src/components/repositories/factory/RepositoryFactory.test.tsx index d987d430..b2f1668d 100644 --- a/spire-ui/src/components/repositories/factory/RepositoryFactory.test.tsx +++ b/spire-ui/src/components/repositories/factory/RepositoryFactory.test.tsx @@ -237,7 +237,8 @@ describe('build setup', () => { fireEvent.change(await screen.findByLabelText('Model', field), { target: { value: 'TEST-model' } }); fireEvent.click(screen.getByRole('button', { name: 'Save build setup' })); await waitFor(() => expect(build.saveBuildDefaults).toHaveBeenCalledWith(repository.id, - { expectedRevision: 0, baseBranch: 'main', harness: 'codex', model: 'TEST-model' })); + // No level chosen is the model's own default, sent as null rather than as a level called ''. + { expectedRevision: 0, baseBranch: 'main', harness: 'codex', model: 'TEST-model', effort: null })); expect(await screen.findByText(/Build setup saved: codex on TEST-model/)).toBeInTheDocument(); }); diff --git a/spire-ui/src/components/repositories/factory/buildDefaultsApi.ts b/spire-ui/src/components/repositories/factory/buildDefaultsApi.ts index 475905d1..624fe260 100644 --- a/spire-ui/src/components/repositories/factory/buildDefaultsApi.ts +++ b/spire-ui/src/components/repositories/factory/buildDefaultsApi.ts @@ -7,13 +7,40 @@ export interface BuildDefaults { baseBranch: string | null; harness: string | null; model: string | null; + /** The thinking level, or null for the model's own default. Absent from an older server. */ + effort?: string | null; updatedBy: string | null; updatedAt: string | null; } +/** One model a harness can run, as its agent image declares it. */ +export interface HarnessModel { + slug: string; + displayName: string; + /** THIS model's own default thinking level. Models differ, so there is no global one. */ + defaultEffort: string; + /** The thinking levels this model allows. */ + efforts: string[]; + visible: boolean; + priority: number; +} + +/** + * The models a harness can run, and why there are none when there are none. + * + *

An empty list means four different things, so the status travels with it: UNKNOWN (the run worker + * has not answered yet), NO_CATALOGUE (the image was built without one), UNREADABLE, IMAGE_UNAVAILABLE. + */ +export interface HarnessModels { + status: 'OK' | 'UNKNOWN' | 'NO_CATALOGUE' | 'UNREADABLE' | 'IMAGE_UNAVAILABLE'; + offered: HarnessModel[]; +} + export interface BuildOptions { harnesses: string[]; /** Per harness, the token types it can report. A model that cannot price one of them is refused. */ reportedTypes: Record; + /** Per harness, the models its image says it runs. Absent from an older server. */ + models?: Record; } /** `account` is the role whose account answered: a REVIEWER-read head is not proof the factory can push. */ export interface BranchHead { branch: string; commit: string; account: string } @@ -34,7 +61,7 @@ const base = (repository: string) => `/api/repositories/${encodeURIComponent(rep export const buildDefaults = (repository: string) => read(`${base(repository)}/build`); export const buildOptions = (repository: string) => read(`${base(repository)}/build/options`); -export const saveBuildDefaults = (repository: string, input: { expectedRevision: number; baseBranch: string; harness: string; model: string }) => +export const saveBuildDefaults = (repository: string, input: { expectedRevision: number; baseBranch: string; harness: string; model: string; effort: string | null }) => read(`${base(repository)}/build`, { method: 'PUT', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(input) }); export const repositoryBranchHead = (repository: string, branch: string) => read(`${base(repository)}/branch-head?branch=${encodeURIComponent(branch)}`); From a6c36ce417a3ad9026988b961a422f6aa0d08c9e Mon Sep 17 00:00:00 2001 From: Artjoms Stukans Date: Tue, 22 Sep 2026 22:38:35 +0200 Subject: [PATCH 4/7] Run the build at the thinking level the operator approved The build setup saved a thinking level, but nothing carried it to the run, so every build ran at the model's own default. The level now travels the whole way: - WorkPreparation binds it under a new binding version 3, so gates opened under versions 1 and 2 keep their exact hashes, and a level may not ride on a version that does not hash it. - ExecuteRun carries it as a nullable reasoningEffort, with an atEffort wither; WorkRunAssembly sets it from the approved preparation. - The run worker puts it on HarnessInvocation, and the Codex arm adds -c model_reasoning_effort="" only when a level was chosen. The Codex CLI does not validate the level, so every hop accepts only a short lower-case word. Eight mutants, one per hop and guard, each fail exactly the test written for them. --- ...actory-m35-one-ticket-to-a-build-design.md | 17 ++++++++ .../contract/command/RunCommand.java | 40 +++++++++++++++++-- .../contract/work/WorkPreparation.java | 37 +++++++++++++++-- .../command/ExecuteRunBranchModeTest.java | 11 +++++ .../WorkPreparationBindingVersionTest.java | 40 +++++++++++++++++++ .../src/test/resources/contract-schema.txt | 2 +- .../codespire/harness/codex/CodexAdapter.java | 12 ++++++ .../harness/codex/CodexAdapterTest.java | 14 +++++++ .../codespire/harness/HarnessInvocation.java | 16 +++++++- .../harness/HarnessInvocationTest.java | 7 ++++ .../orchestrator/factory/WorkRunAssembly.java | 4 +- .../work/WorkPreparationSweep.java | 2 +- .../work/WorkPreparationSweepTest.java | 28 ++++++++++++- .../work/WorkRunDispatchTest.java | 10 +++++ .../codespire/runworker/RunUnitBuilder.java | 2 +- .../runworker/RunUnitBuilderTest.java | 11 ++++- 16 files changed, 240 insertions(+), 13 deletions(-) diff --git a/docs/superpowers/specs/2026-09-16-factory-m35-one-ticket-to-a-build-design.md b/docs/superpowers/specs/2026-09-16-factory-m35-one-ticket-to-a-build-design.md index 3a167478..a78d1f6c 100644 --- a/docs/superpowers/specs/2026-09-16-factory-m35-one-ticket-to-a-build-design.md +++ b/docs/superpowers/specs/2026-09-16-factory-m35-one-ticket-to-a-build-design.md @@ -575,6 +575,23 @@ level the model does not offer would reach the vendor as it stands. With no leve default applies, which is a real choice the vendor publishes per model — so it is stored as NULL rather than as a guessed name. +### 6A.4b The level reaches the run, or it is not approved + +A level saved and then ignored would be the quiet lie this milestone keeps removing. So the level is +carried the whole way, and each hop has a test that fails if it drops it: + +1. The sweep copies the saved level into the preparation, which binds under a new **version 3** + (`WorkPreparation.EFFORT_BINDING`). Versions 1 and 2 keep their exact hashes, per 6.3, and may not + carry a level at all — a level they do not hash would reach the build without being approved. +2. The build command carries it (`ExecuteRun.reasoningEffort`, nullable, so a command already on the + bus decodes as "model default"), set from the preparation the gate approved. +3. The run worker puts it on the harness invocation, and the Codex arm adds + `-c 'model_reasoning_effort=""'` — nothing at all when no level was chosen. + +The vendor's CLI does not check the level (measured on 2026-09-22: a nonsense value is echoed back), so +every hop accepts only a short lower-case word. That keeps the value from closing a quote or naming a +second config key, and it is why the save already refuses a level the model does not declare. + ### 6A.5 The cost, stated The image contract gains a clause, so an operator building their own agent image must produce that label diff --git a/spire-contract/src/main/java/dev/codespire/contract/command/RunCommand.java b/spire-contract/src/main/java/dev/codespire/contract/command/RunCommand.java index 5fd61e5c..c18797a1 100644 --- a/spire-contract/src/main/java/dev/codespire/contract/command/RunCommand.java +++ b/spire-contract/src/main/java/dev/codespire/contract/command/RunCommand.java @@ -80,7 +80,8 @@ record ExecuteRun(String runId, RepoRef repo, String remoteUri, String prompt, String harness, String model, String agentImage, List protectedPaths, long maxWallClockSeconds, String scmCredential, String harnessCredential, - boolean existingBranch, String protectedBranch) implements RunCommand { + boolean existingBranch, String protectedBranch, + String reasoningEffort) implements RunCommand { // Every call site that predates ADR-040 keeps working and keeps the M0 rule — the // additive treatment the other wire records take. A run already on the bus reads as @@ -92,10 +93,31 @@ public ExecuteRun(String runId, RepoRef repo, String remoteUri, String scmCredential, String harnessCredential) { this(runId, repo, remoteUri, baseBranch, baseCommit, branch, prompt, harness, model, agentImage, protectedPaths, maxWallClockSeconds, scmCredential, - harnessCredential, false, ""); + harnessCredential, false, "", null); + } + + /** + * Every caller written before thinking levels existed runs at the model's own default (M3.5 part M). + * A run already on the bus decodes with a null level, which is what every such run was. + */ + public ExecuteRun(String runId, RepoRef repo, String remoteUri, + String baseBranch, String baseCommit, String branch, + String prompt, String harness, String model, String agentImage, + List protectedPaths, long maxWallClockSeconds, + String scmCredential, String harnessCredential, + boolean existingBranch, String protectedBranch) { + this(runId, repo, remoteUri, baseBranch, baseCommit, branch, prompt, harness, model, + agentImage, protectedPaths, maxWallClockSeconds, scmCredential, + harnessCredential, existingBranch, protectedBranch, null); } public ExecuteRun { + // The vendor CLI does not check this (measured 2026-09-22: a nonsense level is echoed back and + // sent on), and it reaches a shell line in the agent container. So its shape is fixed here, + // once, before anything downstream can quote it wrongly. + reasoningEffort = reasoningEffort == null || reasoningEffort.isBlank() ? null : reasoningEffort; + if (reasoningEffort != null && !reasoningEffort.matches("[a-z]{1,16}")) + throw new IllegalArgumentException("a thinking level is a short lower-case word, was: " + reasoningEffort); Objects.requireNonNull(runId, "runId"); Objects.requireNonNull(repo, "repo"); // The clone URL cannot be derived from RepoRef: that is (workspace, slug) with no host, @@ -150,7 +172,18 @@ public boolean pushesToAnExistingBranch() { public ExecuteRun onExistingBranch(String destination) { return new ExecuteRun(runId, repo, remoteUri, baseBranch, baseCommit, branch, prompt, harness, model, agentImage, protectedPaths, maxWallClockSeconds, scmCredential, - harnessCredential, true, destination); + harnessCredential, true, destination, reasoningEffort); + } + + /** + * The same run at a chosen thinking level. A wither, and the full component list spelled once + * here, for the reason {@link #onExistingBranch} gives: a shorter constructor would still + * compile at a rebuild site while quietly dropping the level. + */ + public ExecuteRun atEffort(String level) { + return new ExecuteRun(runId, repo, remoteUri, baseBranch, baseCommit, branch, prompt, + harness, model, agentImage, protectedPaths, maxWallClockSeconds, scmCredential, + harnessCredential, existingBranch, protectedBranch, level); } @Override @@ -168,6 +201,7 @@ public String toString() { + ", maxWallClockSeconds=" + maxWallClockSeconds + ", existingBranch=" + existingBranch + ", protectedBranch=" + protectedBranch + + ", reasoningEffort=" + reasoningEffort + ", promptChars=" + prompt.length() + ", scmCredential=" + (scmCredential == null ? "absent" : "***") + ", harnessCredential=" + (harnessCredential == null ? "absent" : "***") + "]"; diff --git a/spire-contract/src/main/java/dev/codespire/contract/work/WorkPreparation.java b/spire-contract/src/main/java/dev/codespire/contract/work/WorkPreparation.java index 6a7dd9ab..a503b286 100644 --- a/spire-contract/src/main/java/dev/codespire/contract/work/WorkPreparation.java +++ b/spire-contract/src/main/java/dev/codespire/contract/work/WorkPreparation.java @@ -10,7 +10,8 @@ /** Pinned references and execution coordinates only; tracker artifact text never enters aggregate history. */ public record WorkPreparation(Artifact specification, Artifact plan, String baseBranch, String baseCommit, - String harness, String model, String registeredBy, int bindingVersion) { + String harness, String model, String registeredBy, int bindingVersion, + String effort) { /** Where an artifact's approved bytes live. Absent in stored history means {@link Origin#TRACKER}. */ public enum Origin { @@ -55,9 +56,27 @@ public record Artifact(WorkIssueLocation location, String sha256, Origin origin, /** Adds the artifact origin and the stored identity, for preparations this deployment composes. */ public static final int STORED_BINDING = 2; + /** + * Adds the thinking level (M3.5 part M). + * + *

A version of its own, for the reason the others exist: a gate stores the binding and answering + * it compares against a RECOMPUTED one, so folding the level into version 2 would change the hash of + * every composed preparation already waiting on a decision. It belongs in the binding at all because + * it changes what a build costs and how hard the model works — the same reason the model does — and + * because the vendor's CLI does not check it: measured 2026-09-22, a nonsense level is accepted and + * echoed back. So the level an operator approved is the one the build runs, or nothing is. + */ + public static final int EFFORT_BINDING = 3; + public WorkPreparation(Artifact specification, Artifact plan, String baseBranch, String baseCommit, String harness, String model, String registeredBy) { - this(specification, plan, baseBranch, baseCommit, harness, model, registeredBy, TRACKER_BINDING); + this(specification, plan, baseBranch, baseCommit, harness, model, registeredBy, TRACKER_BINDING, null); + } + + /** Every preparation written before thinking levels existed carries the model's own default. */ + public WorkPreparation(Artifact specification, Artifact plan, String baseBranch, String baseCommit, + String harness, String model, String registeredBy, int bindingVersion) { + this(specification, plan, baseBranch, baseCommit, harness, model, registeredBy, bindingVersion, null); } public WorkPreparation { @@ -71,8 +90,15 @@ public WorkPreparation(Artifact specification, Artifact plan, String baseBranch, baseCommit = baseCommit.toLowerCase(java.util.Locale.ROOT); // Absent in older stored JSON, where every artifact was a tracker ticket. bindingVersion = bindingVersion == 0 ? TRACKER_BINDING : bindingVersion; - if (bindingVersion != TRACKER_BINDING && bindingVersion != STORED_BINDING) + if (bindingVersion != TRACKER_BINDING && bindingVersion != STORED_BINDING && bindingVersion != EFFORT_BINDING) throw new IllegalArgumentException("Unknown preparation binding version " + bindingVersion); + effort = effort == null || effort.isBlank() ? null : effort.strip(); + // A level under a version that does not hash it would be carried to the build without being part + // of what was approved -- exactly what the version exists to prevent. + if (effort != null && bindingVersion < EFFORT_BINDING) + throw new IllegalArgumentException("A thinking level needs binding version " + EFFORT_BINDING); + if (effort != null && !effort.matches("[a-z]{1,16}")) + throw new IllegalArgumentException("A thinking level is a short lower-case word, was: " + effort); if (bindingVersion == TRACKER_BINDING && (specification.origin() != Origin.TRACKER || plan.origin() != Origin.TRACKER)) throw new IllegalArgumentException("A version 1 binding describes tracker artifacts only"); @@ -94,6 +120,11 @@ public String binding() { } } for (String part : java.util.List.of(baseBranch, baseCommit, harness, model)) value.append(part.length()).append(':').append(part); + // Versions 1 and 2 must keep producing exactly the hashes their open gates hold. + if (bindingVersion >= EFFORT_BINDING) { + String level = effort == null ? "" : effort; + value.append(level.length()).append(':').append(level); + } return digest(value.toString()); } diff --git a/spire-contract/src/test/java/dev/codespire/contract/command/ExecuteRunBranchModeTest.java b/spire-contract/src/test/java/dev/codespire/contract/command/ExecuteRunBranchModeTest.java index fac26049..3ff27797 100644 --- a/spire-contract/src/test/java/dev/codespire/contract/command/ExecuteRunBranchModeTest.java +++ b/spire-contract/src/test/java/dev/codespire/contract/command/ExecuteRunBranchModeTest.java @@ -36,6 +36,17 @@ private static RunCommand.ExecuteRun run() { * every run dispatched before ADR-040 pushes into the factory's own namespace, and a default of * {@code existing} would lift that floor for all of them at once. */ + /** A wither that forgot one component would drop it at every rebuild site, and still compile. */ + @Test + void theThinkingLevelSurvivesEveryRebuildAndTheOthersSurviveIt() { + RunCommand.ExecuteRun run = run().atEffort("high").onExistingBranch("develop"); + + assertEquals("high", run.reasoningEffort()); + assertEquals("develop", run.protectedBranch()); + assertEquals(null, run().reasoningEffort(), "a run nobody chose a level for uses the model's default"); + assertThrows(IllegalArgumentException.class, () -> run().atEffort("high'; rm")); + } + @Test void aCommandThatSaysNothingUsesTheNamespaceMode() { assertFalse(run().pushesToAnExistingBranch()); diff --git a/spire-contract/src/test/java/dev/codespire/contract/work/WorkPreparationBindingVersionTest.java b/spire-contract/src/test/java/dev/codespire/contract/work/WorkPreparationBindingVersionTest.java index 0f05d579..2871bb1e 100644 --- a/spire-contract/src/test/java/dev/codespire/contract/work/WorkPreparationBindingVersionTest.java +++ b/spire-contract/src/test/java/dev/codespire/contract/work/WorkPreparationBindingVersionTest.java @@ -97,6 +97,46 @@ void aVersionOneBindingCannotDescribeStoredArtifacts() { "main", COMMIT, "codex", "TEST-model", "system", WorkPreparation.TRACKER_BINDING)); } + private static WorkPreparation atLevel(String level) { + UUID specId = UUID.fromString("00000000-0000-4000-8000-000000000071"); + UUID planId = UUID.fromString("00000000-0000-4000-8000-000000000072"); + return new WorkPreparation( + new WorkPreparation.Artifact(ticket("71"), SPEC_SHA, WorkPreparation.Origin.STORED, specId), + new WorkPreparation.Artifact(ticket("72"), PLAN_SHA, WorkPreparation.Origin.STORED, planId), + "main", COMMIT, "codex", "TEST-model", "system", WorkPreparation.EFFORT_BINDING, level); + } + + /** + * The level is part of what an operator approves, so two levels are two approvals. And version 3 + * with no level is still not version 2: a gate opened under 2 must not be answered by a recomputed 3. + */ + @Test + void theThinkingLevelIsPartOfWhatIsApproved() { + assertNotEquals(atLevel("high").binding(), atLevel("xhigh").binding()); + assertNotEquals(atLevel("high").binding(), atLevel(null).binding()); + UUID specId = UUID.fromString("00000000-0000-4000-8000-000000000071"); + UUID planId = UUID.fromString("00000000-0000-4000-8000-000000000072"); + assertNotEquals(stored(specId, planId).binding(), atLevel(null).binding()); + } + + /** Versions 1 and 2 do not hash a level, so they may not carry one to the build. */ + @Test + void aLevelUnderAVersionThatDoesNotHashItIsRefused() { + UUID specId = UUID.randomUUID(), planId = UUID.randomUUID(); + assertThrows(IllegalArgumentException.class, () -> new WorkPreparation( + new WorkPreparation.Artifact(ticket("71"), SPEC_SHA, WorkPreparation.Origin.STORED, specId), + new WorkPreparation.Artifact(ticket("72"), PLAN_SHA, WorkPreparation.Origin.STORED, planId), + "main", COMMIT, "codex", "TEST-model", "system", WorkPreparation.STORED_BINDING, "high")); + } + + /** The level ends up inside the harness's own config syntax; only a plain word may get there. */ + @Test + void aLevelThatIsNotAPlainWordIsRefused() { + assertThrows(IllegalArgumentException.class, () -> atLevel("high\" x=\"y")); + assertThrows(IllegalArgumentException.class, () -> atLevel("HIGH")); + assertEquals(null, atLevel(" ").effort(), "blank is the model's own default, not a level called blank"); + } + @Test void anUnknownVersionIsRefusedRatherThanHashedSomeOtherWay() { assertThrows(IllegalArgumentException.class, () -> new WorkPreparation( diff --git a/spire-contract/src/test/resources/contract-schema.txt b/spire-contract/src/test/resources/contract-schema.txt index 4b983745..5cc33bca 100644 --- a/spire-contract/src/test/resources/contract-schema.txt +++ b/spire-contract/src/test/resources/contract-schema.txt @@ -43,7 +43,7 @@ RefuseFinding(reviewId: java.lang.String, repo: dev.codespire.contract.scm.RepoR # RunCommand CancelRun(runId: java.lang.String, reason: java.lang.String) -ExecuteRun(runId: java.lang.String, repo: dev.codespire.contract.scm.RepoRef, remoteUri: java.lang.String, baseBranch: java.lang.String, baseCommit: java.lang.String, branch: java.lang.String, prompt: java.lang.String, harness: java.lang.String, model: java.lang.String, agentImage: java.lang.String, protectedPaths: java.util.List, maxWallClockSeconds: long, scmCredential: java.lang.String, harnessCredential: java.lang.String, existingBranch: boolean, protectedBranch: java.lang.String) +ExecuteRun(runId: java.lang.String, repo: dev.codespire.contract.scm.RepoRef, remoteUri: java.lang.String, baseBranch: java.lang.String, baseCommit: java.lang.String, branch: java.lang.String, prompt: java.lang.String, harness: java.lang.String, model: java.lang.String, agentImage: java.lang.String, protectedPaths: java.util.List, maxWallClockSeconds: long, scmCredential: java.lang.String, harnessCredential: java.lang.String, existingBranch: boolean, protectedBranch: java.lang.String, reasoningEffort: java.lang.String) ExecuteWorkRun(runId: java.lang.String, execution: dev.codespire.contract.command.RunCommand$ExecuteRun, work: dev.codespire.contract.work.WorkRunBinding) HoldWorkRun(runId: java.lang.String, work: dev.codespire.contract.work.WorkRunBinding) PublishWorkRun(runId: java.lang.String, permit: dev.codespire.contract.work.WorkPublicationPermit, scmCredential: java.lang.String) diff --git a/spire-harness-codex/src/main/java/dev/codespire/harness/codex/CodexAdapter.java b/spire-harness-codex/src/main/java/dev/codespire/harness/codex/CodexAdapter.java index f25aecca..d5f230ae 100644 --- a/spire-harness-codex/src/main/java/dev/codespire/harness/codex/CodexAdapter.java +++ b/spire-harness-codex/src/main/java/dev/codespire/harness/codex/CodexAdapter.java @@ -126,11 +126,23 @@ public List command(HarnessInvocation invocation) { + " || exit " + LOGIN_FAILED + "; " + "exec codex exec --json --sandbox danger-full-access --skip-git-repo-check" + " --model " + quoted(invocation.model(), "model") + + effort(invocation.reasoningEffort()) + " -C " + quoted(invocation.workspacePath(), "workspace path") + " -"; return List.of("sh", "-c", script); } + /** + * The thinking level as a config override, or nothing, so the model's own default applies. + * + *

Measured against codex-cli 0.146.0 on 2026-09-22: {@code -c model_reasoning_effort="high"} is + * echoed back as "reasoning effort: high". The value is TOML, hence the double quotes inside the + * single ones. {@link HarnessInvocation} has already held it to a lower-case word. + */ + private static String effort(String level) { + return level == null ? "" : " -c " + quoted("model_reasoning_effort=\"" + level + "\"", "thinking level"); + } + /** * A value for the one shell line above, in single quotes, refused rather than escaped if it * could close them. diff --git a/spire-harness-codex/src/test/java/dev/codespire/harness/codex/CodexAdapterTest.java b/spire-harness-codex/src/test/java/dev/codespire/harness/codex/CodexAdapterTest.java index 3a602713..85698291 100644 --- a/spire-harness-codex/src/test/java/dev/codespire/harness/codex/CodexAdapterTest.java +++ b/spire-harness-codex/src/test/java/dev/codespire/harness/codex/CodexAdapterTest.java @@ -41,6 +41,20 @@ private HarnessInvocation invocation() { // ---- invocation ------------------------------------------------------------------------- + /** + * The chosen level reaches the CLI as a config override, measured against codex-cli 0.146.0; with + * no level there is no override at all, so the model's own default applies. + */ + @Test + void theChosenThinkingLevelReachesCodexAndNoLevelMeansNoOverride() { + HarnessInvocation high = new HarnessInvocation("run_abc", "fix the bug", "/workspace", "gpt-5.6", + Map.of(HarnessInvocation.CREDENTIAL, "sk-secret"), Duration.ofMinutes(30), "high"); + + assertTrue(script(adapter.command(high)).contains(" -c 'model_reasoning_effort=\"high\"' "), + script(adapter.command(high))); + assertFalse(script(adapter.command(invocation())).contains("model_reasoning_effort")); + } + @Test void theTypeIsCodex() { assertEquals(HarnessType.CODEX, adapter.type()); diff --git a/spire-harness/src/main/java/dev/codespire/harness/HarnessInvocation.java b/spire-harness/src/main/java/dev/codespire/harness/HarnessInvocation.java index ab627a87..9138dd33 100644 --- a/spire-harness/src/main/java/dev/codespire/harness/HarnessInvocation.java +++ b/spire-harness/src/main/java/dev/codespire/harness/HarnessInvocation.java @@ -15,7 +15,13 @@ */ public record HarnessInvocation(String runId, String prompt, String workspacePath, String model, Map credentials, - Duration wallClock) { + Duration wallClock, String reasoningEffort) { + + /** A run at the model's own thinking level: every caller written before levels existed (M3.5 part M). */ + public HarnessInvocation(String runId, String prompt, String workspacePath, + String model, Map credentials, Duration wallClock) { + this(runId, prompt, workspacePath, model, credentials, wallClock, null); + } /** * The key under which the worker supplies the model credential in {@link #credentials()}. The @@ -46,6 +52,13 @@ public record HarnessInvocation(String runId, String prompt, String workspacePat // be; "/" would hand it everything mounted. requireArgumentSafe(model, "model"); requireArgumentSafe(workspacePath, "workspacePath"); + // An arm places the level inside its own configuration syntax, so the shape is fixed here: a + // short lower-case word can close no quote and name no second key. The vendor does not check + // it (measured 2026-09-22), so a wrong word would run rather than fail. + reasoningEffort = reasoningEffort == null || reasoningEffort.isBlank() ? null : reasoningEffort.strip(); + if (reasoningEffort != null && !reasoningEffort.matches("[a-z]{1,16}")) { + throw new IllegalArgumentException("reasoningEffort must be a short lower-case word, was: " + reasoningEffort); + } if (!workspacePath.startsWith("/")) { throw new IllegalArgumentException("workspacePath must be absolute, was: " + workspacePath); } @@ -67,6 +80,7 @@ private static void requireArgumentSafe(String value, String field) { public String toString() { return "HarnessInvocation[runId=" + runId + ", model=" + model + + ", reasoningEffort=" + reasoningEffort + ", workspacePath=" + workspacePath + ", wallClock=" + wallClock + ", credentials=" + credentials.keySet() + " (values redacted)" diff --git a/spire-harness/src/test/java/dev/codespire/harness/HarnessInvocationTest.java b/spire-harness/src/test/java/dev/codespire/harness/HarnessInvocationTest.java index a10239dd..50224062 100644 --- a/spire-harness/src/test/java/dev/codespire/harness/HarnessInvocationTest.java +++ b/spire-harness/src/test/java/dev/codespire/harness/HarnessInvocationTest.java @@ -18,6 +18,13 @@ private static HarnessInvocation invocation(Map credentials) { credentials, Duration.ofMinutes(30)); } + @Test + void aThinkingLevelThatIsNotAPlainWordIsRefused() { + assertThrows(IllegalArgumentException.class, () -> new HarnessInvocation("run-1", "p", "/workspace", + "gpt-5-codex", Map.of(), Duration.ofMinutes(1), "high\" -c x=\"y")); + assertEquals(null, invocation(Map.of()).reasoningEffort()); + } + @Test void toStringNeverPrintsACredential() { // A record's generated toString() prints every component, so `log.info("{}", invocation)` diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/WorkRunAssembly.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/WorkRunAssembly.java index 488bde8e..7ecfea21 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/WorkRunAssembly.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/WorkRunAssembly.java @@ -61,7 +61,9 @@ public Prepared assemble(Connection c,WorkSourceRegistry.Source source,WorkItemE var command=new RunCommand.ExecuteRun(id,source.repository(),FactoryCloneUrls.cloneUrl(source.scm(),account.baseUrl(),source.repository()), in.baseBranch(),in.baseCommit(),branch,in.prompt(),in.harness(),in.model(),in.agentImage(), item.policy().limits().protectedPaths().stream().sorted().toList(),wall, - credentials.packScm(id,account.botUsername(),account.secret()),credentials.packHarness(id,chosen.member().apiKey())); + credentials.packScm(id,account.botUsername(),account.secret()),credentials.packHarness(id,chosen.member().apiKey())) + // The level the approved binding hashed, so the build runs at what was approved (M3.5 part M). + .atEffort(item.preparation().effort()); var held=new RunCommand.ExecuteWorkRun(command,new dev.codespire.contract.work.WorkRunBinding( item.workItemId(),item.generation(),item.progress().attemptId(),item.preparation().binding())); return new Prepared(held,new FactoryRunProjection.QueuedRun(id,in.harness(),in.model(),in.baseBranch(),in.baseCommit(),branch,account.botUsername(),chosen.member().id())); diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/work/WorkPreparationSweep.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/work/WorkPreparationSweep.java index f0688bac..1c7b8e2c 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/work/WorkPreparationSweep.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/work/WorkPreparationSweep.java @@ -243,7 +243,7 @@ private Result attempt(String id, boolean again, String actor, WorkPreparation.Origin.STORED, planId), setup.baseBranch(), head, setup.harness(), setup.model(), actor == null ? "system:build-defaults@" + setup.revision() : actor, - WorkPreparation.STORED_BINDING); + WorkPreparation.EFFORT_BINDING, setup.effort()); // The saved setup is compared again INSIDE the registration's own transaction: it was read // before a forge call this waited on, and a repository whose branch, harness or model changed diff --git a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/work/WorkPreparationSweepTest.java b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/work/WorkPreparationSweepTest.java index 4e9b4075..a479a872 100644 --- a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/work/WorkPreparationSweepTest.java +++ b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/work/WorkPreparationSweepTest.java @@ -36,6 +36,8 @@ class WorkPreparationSweepTest extends WorkPreparedFixture { @Inject WorkPreparationSweep sweep; @Inject BuildDefaults defaults; @Inject WorkItemArtifacts stored; + @Inject dev.codespire.orchestrator.factory.HarnessCatalogues catalogues; + @Inject dev.codespire.orchestrator.factory.FactoryConfig config; @BeforeEach void buildSetup() { @@ -76,6 +78,29 @@ private void awaitHeadRequest() throws Exception { throw new AssertionError("the sweep never asked the forge for the branch head"); } + /** The level saved with the build setup is the level the prepared task binds (M3.5 part M). */ + @Test + void theSavedThinkingLevelIsCopiedIntoThePreparedTask() throws Exception { + try { + catalogues.record(new dev.codespire.contract.event.HarnessImageResult.Described("TEST-request", "codex", + config.agentImage().get("codex"), dev.codespire.contract.event.HarnessImageResult.Status.OK, + java.util.List.of(new dev.codespire.contract.event.HarnessImageResult.Model(model, model, "medium", + java.util.List.of("medium", "high"), true, 1)))); + defaults.save(repository, new BuildDefaults.Input(defaults.get(repository).revision(), "main", "codex", model, "high"), + "TEST-prepared-admin"); + String id = admit("assisted", 81); + + sweep.sweep(); + + var prepared = store.load(id).preparation(); + assertNotNull(prepared); + assertEquals("high", prepared.effort()); + assertEquals(WorkPreparation.EFFORT_BINDING, prepared.bindingVersion()); + } finally { + executeWith("DELETE FROM harness_catalogue"); + } + } + @Test void oneTicketBecomesAPreparedTaskWithNoTypingAtAll() throws Exception { String id = admit("assisted", 80); @@ -85,7 +110,8 @@ void oneTicketBecomesAPreparedTaskWithNoTypingAtAll() throws Exception { var prepared = store.load(id).preparation(); assertNotNull(prepared, "the ticket alone must be enough"); - assertEquals(WorkPreparation.STORED_BINDING, prepared.bindingVersion()); + assertEquals(WorkPreparation.EFFORT_BINDING, prepared.bindingVersion()); + assertNull(prepared.effort(), "no level was saved, so the model's own default applies"); assertEquals(WorkPreparation.Origin.STORED, prepared.specification().origin()); assertEquals(WorkPreparation.Origin.STORED, prepared.plan().origin()); // The build coordinates come from the repository's saved setup, and the commit from the forge. diff --git a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/work/WorkRunDispatchTest.java b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/work/WorkRunDispatchTest.java index 1f19b1d9..fd80a1bc 100644 --- a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/work/WorkRunDispatchTest.java +++ b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/work/WorkRunDispatchTest.java @@ -33,6 +33,16 @@ class WorkRunDispatchTest extends WorkPreparedFixture { execute("UPDATE scm_provider SET bot_account_id='900003',bot_username='TEST-renamed-again' WHERE id=?",account); assertEquals("900002",store.load(id).control().factoryActor()); } + /** The level the approved binding hashed is the level the build is sent with (M3.5 part M). */ + @Test void theApprovedThinkingLevelIsTheOneTheBuildRunsAt() throws Exception { + String id=admit("autonomous",57);var plain=preparation("TEST-prepared-admin"); + var atLevel=new WorkPreparation(plain.specification(),plain.plan(),plain.baseBranch(),plain.baseCommit(),plain.harness(), + plain.model(),plain.registeredBy(),WorkPreparation.EFFORT_BINDING,"high"); + var outcome=transitions.prepare(id,store.history(id).size(),atLevel);assertEquals(200,outcome.status(),outcome.reason()); + dispatcher.drain(); + assertEquals("high",heldCommands.getLast().execution().reasoningEffort()); + assertEquals(atLevel.binding(),heldCommands.getLast().work().preparationBinding()); + } @Inject RunResultSaga saga; @Inject dev.codespire.orchestrator.factory.WorkRunAssembly assembly; String ready() throws Exception {String id=admit("autonomous",56);register(id);return id;} diff --git a/spire-run-worker/src/main/java/dev/codespire/runworker/RunUnitBuilder.java b/spire-run-worker/src/main/java/dev/codespire/runworker/RunUnitBuilder.java index cee8814c..84dfbb40 100644 --- a/spire-run-worker/src/main/java/dev/codespire/runworker/RunUnitBuilder.java +++ b/spire-run-worker/src/main/java/dev/codespire/runworker/RunUnitBuilder.java @@ -141,7 +141,7 @@ private RunUnitSpec build(RunCommand.ExecuteRun command, HarnessAdapter adapter, HarnessInvocation invocation = new HarnessInvocation(command.runId(), withCommitInstruction(command.prompt()), "/workspace", command.model(), harnessEnv, - Duration.ofSeconds(command.maxWallClockSeconds())); + Duration.ofSeconds(command.maxWallClockSeconds()), command.reasoningEffort()); Map agentEnv = new LinkedHashMap<>(adapter.environment(invocation)); agentEnv.put("SPIRE_BASE_COMMIT", command.baseCommit()); diff --git a/spire-run-worker/src/test/java/dev/codespire/runworker/RunUnitBuilderTest.java b/spire-run-worker/src/test/java/dev/codespire/runworker/RunUnitBuilderTest.java index 4959a0e0..88265690 100644 --- a/spire-run-worker/src/test/java/dev/codespire/runworker/RunUnitBuilderTest.java +++ b/spire-run-worker/src/test/java/dev/codespire/runworker/RunUnitBuilderTest.java @@ -182,7 +182,8 @@ void aProtectedBranchIsNotDroppedJustBecauseTheModeIsTheDefault() { original.repo(), original.remoteUri(), original.baseBranch(), original.baseCommit(), original.branch(), original.prompt(), original.harness(), original.model(), original.agentImage(), original.protectedPaths(), original.maxWallClockSeconds(), - original.scmCredential(), original.harnessCredential(), false, "develop"); + original.scmCredential(), original.harnessCredential(), false, "develop", + original.reasoningEffort()); Map env = builder.build(withFloorOnly, new CodexAdapter()) .publisher().environment(); @@ -191,6 +192,14 @@ void aProtectedBranchIsNotDroppedJustBecauseTheModeIsTheDefault() { assertFalse(env.containsKey("SPIRE_BRANCH_MODE"), env.keySet().toString()); } + /** The level on the command is the level in the agent's command line, not lost on the way. */ + @Test + void theThinkingLevelReachesTheAgentsCommand() { + List argv = builder.build(command().atEffort("xhigh"), new CodexAdapter()).agent().argv(); + + assertTrue(String.join(" ", argv).contains("model_reasoning_effort=\"xhigh\""), argv.toString()); + } + /** A fix run carries both, because the publisher refuses the mode without the destination. */ @Test void aFixRunTellsThePublisherWhichBranchIsOffLimits() { From 04f6b3c2af9014348fd7d8b13c38398f1b134454 Mon Sep 17 00:00:00 2001 From: Artjoms Stukans Date: Wed, 23 Sep 2026 00:01:49 +0200 Subject: [PATCH 5/7] Check the harness's models at every step, not only at save Review of PR #167 found four gaps; this closes three and part of the fourth. - A setup was checked against the image only when saved. If the image changed afterwards, the task was still prepared and the build still dispatched. HarnessCatalogues.refusal now holds the one check, and the save, the preparation sweep and the dispatch all ask it. The three reasons are named on the item page. - An image could declare a level such as "x-high": offered, saved, then refused at preparation. ThinkingLevel is now the one shape rule, used by the declared model, the preparation and the run command, so such a label reads as UNREADABLE. - The build screen kept a level across a harness change, and a saved level with no known list had no control on screen yet was sent. The level is dropped on a harness change; an unusable one is named, with a way back to the model default, and Save waits. - Image answers were keyed by a random id and handled unordered, so an older answer could replace a newer one. They are now keyed by harness and handled in order. Still open: two workers holding different images under one mutable tag. --- .../contract/command/RunCommand.java | 4 +- .../contract/event/HarnessImageResult.java | 14 +++- .../contract/work/ThinkingLevel.java | 29 +++++++ .../contract/work/WorkPreparation.java | 4 +- .../event/HarnessImageResultModelTest.java | 28 +++++++ .../orchestrator/factory/BuildDefaults.java | 9 +- .../factory/HarnessCatalogues.java | 17 ++++ .../orchestrator/factory/WorkRunAssembly.java | 4 + .../work/WorkPreparationSweep.java | 4 + .../work/WorkRunAssemblyRefusals.java | 3 +- .../work/WorkPreparationSweepTest.java | 32 ++++++- .../work/WorkRunDispatchTest.java | 16 ++++ .../runworker/HarnessImageWorker.java | 16 +++- .../runworker/HarnessImageWorkerTest.java | 83 +++++++++++++++++++ .../runworker/ModelCatalogueLabelTest.java | 7 ++ .../repositories/factory/BuildStep.tsx | 12 ++- .../factory/RepositoryFactory.test.tsx | 33 ++++++++ .../src/components/work-items/workReasons.ts | 3 + 18 files changed, 294 insertions(+), 24 deletions(-) create mode 100644 spire-contract/src/main/java/dev/codespire/contract/work/ThinkingLevel.java create mode 100644 spire-contract/src/test/java/dev/codespire/contract/event/HarnessImageResultModelTest.java create mode 100644 spire-run-worker/src/test/java/dev/codespire/runworker/HarnessImageWorkerTest.java diff --git a/spire-contract/src/main/java/dev/codespire/contract/command/RunCommand.java b/spire-contract/src/main/java/dev/codespire/contract/command/RunCommand.java index c18797a1..3de03c59 100644 --- a/spire-contract/src/main/java/dev/codespire/contract/command/RunCommand.java +++ b/spire-contract/src/main/java/dev/codespire/contract/command/RunCommand.java @@ -115,9 +115,7 @@ public ExecuteRun(String runId, RepoRef repo, String remoteUri, // The vendor CLI does not check this (measured 2026-09-22: a nonsense level is echoed back and // sent on), and it reaches a shell line in the agent container. So its shape is fixed here, // once, before anything downstream can quote it wrongly. - reasoningEffort = reasoningEffort == null || reasoningEffort.isBlank() ? null : reasoningEffort; - if (reasoningEffort != null && !reasoningEffort.matches("[a-z]{1,16}")) - throw new IllegalArgumentException("a thinking level is a short lower-case word, was: " + reasoningEffort); + reasoningEffort = dev.codespire.contract.work.ThinkingLevel.normalise(reasoningEffort); Objects.requireNonNull(runId, "runId"); Objects.requireNonNull(repo, "repo"); // The clone URL cannot be derived from RepoRef: that is (workspace, slug) with no host, diff --git a/spire-contract/src/main/java/dev/codespire/contract/event/HarnessImageResult.java b/spire-contract/src/main/java/dev/codespire/contract/event/HarnessImageResult.java index 1885b24a..55004d52 100644 --- a/spire-contract/src/main/java/dev/codespire/contract/event/HarnessImageResult.java +++ b/spire-contract/src/main/java/dev/codespire/contract/event/HarnessImageResult.java @@ -3,6 +3,8 @@ import com.fasterxml.jackson.annotation.JsonSubTypes; import com.fasterxml.jackson.annotation.JsonTypeInfo; +import dev.codespire.contract.work.ThinkingLevel; + import java.util.List; import java.util.Objects; @@ -31,7 +33,17 @@ record Model(String slug, String displayName, String defaultEffort, List boolean visible, int priority) { public Model { if (slug == null || slug.isBlank()) throw new IllegalArgumentException("A model slug is required"); - efforts = efforts == null ? List.of() : List.copyOf(efforts); + // The same rule every later hop applies. A level only this record accepted could be offered + // and saved, then refused when the task is prepared -- after the operator walked away. + String ownDefault = ThinkingLevel.normalise(defaultEffort); + defaultEffort = ownDefault == null ? "" : ownDefault; + List levels = new java.util.ArrayList<>(); + for (String level : efforts == null ? List.of() : efforts) { + String checked = ThinkingLevel.normalise(level); + if (checked == null) throw new IllegalArgumentException("A declared thinking level is blank"); + levels.add(checked); + } + efforts = List.copyOf(levels); } } diff --git a/spire-contract/src/main/java/dev/codespire/contract/work/ThinkingLevel.java b/spire-contract/src/main/java/dev/codespire/contract/work/ThinkingLevel.java new file mode 100644 index 00000000..a44bcf37 --- /dev/null +++ b/spire-contract/src/main/java/dev/codespire/contract/work/ThinkingLevel.java @@ -0,0 +1,29 @@ +package dev.codespire.contract.work; + +/** + * The one rule for what a thinking level may look like (M3.5 part M). + * + *

A level travels from an image label, through a saved setup and an approved binding, into a + * config override on the harness's command line. The vendor's CLI does not check it (measured + * 2026-09-22), so the shape is checked here, and every hop uses this same check: a label that + * declared a level another hop refuses would let a setup be saved that could never prepare. + */ +public final class ThinkingLevel { + + private ThinkingLevel() { + } + + /** + * The level, stripped, or null for blank — which means the model's own default. + * + * @throws IllegalArgumentException when it is not a short lower-case word; such a word can close + * no quote and name no second config key + */ + public static String normalise(String level) { + if (level == null || level.isBlank()) return null; + String stripped = level.strip(); + if (!stripped.matches("[a-z]{1,16}")) + throw new IllegalArgumentException("A thinking level is a short lower-case word, was: " + level); + return stripped; + } +} diff --git a/spire-contract/src/main/java/dev/codespire/contract/work/WorkPreparation.java b/spire-contract/src/main/java/dev/codespire/contract/work/WorkPreparation.java index a503b286..e60efbcd 100644 --- a/spire-contract/src/main/java/dev/codespire/contract/work/WorkPreparation.java +++ b/spire-contract/src/main/java/dev/codespire/contract/work/WorkPreparation.java @@ -92,13 +92,11 @@ public WorkPreparation(Artifact specification, Artifact plan, String baseBranch, bindingVersion = bindingVersion == 0 ? TRACKER_BINDING : bindingVersion; if (bindingVersion != TRACKER_BINDING && bindingVersion != STORED_BINDING && bindingVersion != EFFORT_BINDING) throw new IllegalArgumentException("Unknown preparation binding version " + bindingVersion); - effort = effort == null || effort.isBlank() ? null : effort.strip(); + effort = ThinkingLevel.normalise(effort); // A level under a version that does not hash it would be carried to the build without being part // of what was approved -- exactly what the version exists to prevent. if (effort != null && bindingVersion < EFFORT_BINDING) throw new IllegalArgumentException("A thinking level needs binding version " + EFFORT_BINDING); - if (effort != null && !effort.matches("[a-z]{1,16}")) - throw new IllegalArgumentException("A thinking level is a short lower-case word, was: " + effort); if (bindingVersion == TRACKER_BINDING && (specification.origin() != Origin.TRACKER || plan.origin() != Origin.TRACKER)) throw new IllegalArgumentException("A version 1 binding describes tracker artifacts only"); diff --git a/spire-contract/src/test/java/dev/codespire/contract/event/HarnessImageResultModelTest.java b/spire-contract/src/test/java/dev/codespire/contract/event/HarnessImageResultModelTest.java new file mode 100644 index 00000000..35bc4d44 --- /dev/null +++ b/spire-contract/src/test/java/dev/codespire/contract/event/HarnessImageResultModelTest.java @@ -0,0 +1,28 @@ +package dev.codespire.contract.event; + +import org.junit.jupiter.api.Test; + +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +/** + * A declared level is held to the rule every later hop applies. The review of PR #167 found an image + * could declare "x-high": offered, saved, then refused when the task was prepared. + */ +class HarnessImageResultModelTest { + + @Test + void aDeclaredLevelEveryLaterHopWouldRefuseIsRefusedHere() { + assertThrows(IllegalArgumentException.class, () -> new HarnessImageResult.Model("TEST-model", "TEST", "medium", + List.of("medium", "x-high"), true, 1)); + assertThrows(IllegalArgumentException.class, () -> new HarnessImageResult.Model("TEST-model", "TEST", "x-high", + List.of("medium"), true, 1)); + } + + @Test + void noDeclaredDefaultIsKeptAsNoneRatherThanRefused() { + assertEquals("", new HarnessImageResult.Model("TEST-model", "TEST", "", List.of("medium"), true, 1).defaultEffort()); + } +} diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/BuildDefaults.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/BuildDefaults.java index 7bb0c904..e13021d1 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/BuildDefaults.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/BuildDefaults.java @@ -154,14 +154,7 @@ WHERE work_item_id IN (SELECT id FROM work_item WHERE repository_id=?) * against, and a level the model does not offer would be passed to the vendor as it stands. */ private void checkAgainstTheHarness(String harness, String model, String effort) { - var catalogue = catalogues.get(harness) - .filter(known -> known.status() == dev.codespire.contract.event.HarnessImageResult.Status.OK); - if (catalogue.isEmpty()) { - if (effort != null) throw new Refused("effort_unverifiable"); - return; - } - var runs = catalogue.get().find(model).orElseThrow(() -> new Refused("model_not_run_by_harness")); - if (effort != null && !runs.efforts().contains(effort)) throw new Refused("effort_not_offered"); + catalogues.refusal(harness, model, effort).ifPresent(reason -> { throw new Refused(reason); }); } private static IllegalStateException database(SQLException failure) { diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCatalogues.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCatalogues.java index 59a1cb3e..b940fd53 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCatalogues.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCatalogues.java @@ -125,6 +125,23 @@ ON CONFLICT (harness) DO UPDATE } } + /** + * Why this harness cannot run the model at this level, or empty when it can — or when nobody can + * know, which lets a model through and refuses a level (design §6A.4a). + * + *

Asked at save, at preparation and at dispatch, because the image can change under a saved + * setup between any two of them. Checking only the save let a setup saved against one image open + * an approval, and start a build, against another that does not run it. + */ + public Optional refusal(String harness, String model, String effort) { + var catalogue = get(harness).filter(known -> known.status() == HarnessImageResult.Status.OK); + if (catalogue.isEmpty()) return effort == null ? Optional.empty() : Optional.of("effort_unverifiable"); + var runs = catalogue.get().find(model); + if (runs.isEmpty()) return Optional.of("model_not_run_by_harness"); + if (effort != null && !runs.get().efforts().contains(effort)) return Optional.of("effort_not_offered"); + return Optional.empty(); + } + /** * The catalogue for a harness, or empty when nothing has been heard yet. * diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/WorkRunAssembly.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/WorkRunAssembly.java index 7ecfea21..7b72be02 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/WorkRunAssembly.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/WorkRunAssembly.java @@ -22,6 +22,7 @@ public class WorkRunAssembly { @Inject LlmModelPricer pricer; @Inject dev.codespire.orchestrator.llm.LlmModelRegistry models; @Inject RunCredentials credentials; + @Inject HarnessCatalogues catalogues; public record Prepared(RunCommand.ExecuteWorkRun command,FactoryRunProjection.QueuedRun row) {} public void validate(WorkSourceRegistry.Source source,dev.codespire.contract.work.WorkPreparation preparation,WorkArtifacts.Evidence evidence) { @@ -51,6 +52,9 @@ public Prepared assemble(Connection c,WorkSourceRegistry.Source source,WorkItemE catch(dev.codespire.orchestrator.llm.LlmModelRegistry.CatalogueUnavailable unreadable) { throw new IllegalStateException("catalogue_unavailable"); } + // Approved against the image the harness ran then; it may run another by now (§6A.4b). + var cannotRun=catalogues.refusal(in.harness(),in.model(),item.preparation().effort()); + if(cannotRun.isPresent())throw new IllegalStateException(cannotRun.get()); var unpriced=pricer.unpricedTypes(in.model(),in.harness()); if(!unpriced.isEmpty())throw new IllegalStateException("model_pricing_incomplete:" +unpriced.stream().map(Enum::name).collect(java.util.stream.Collectors.joining(","))); diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/work/WorkPreparationSweep.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/work/WorkPreparationSweep.java index 1c7b8e2c..082198fd 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/work/WorkPreparationSweep.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/work/WorkPreparationSweep.java @@ -53,6 +53,7 @@ public class WorkPreparationSweep { @Inject WorkSourceEffects effects; @Inject dev.codespire.orchestrator.llm.LlmModelRegistry models; @Inject dev.codespire.orchestrator.llm.LlmModelPricer pricer; + @Inject dev.codespire.orchestrator.factory.HarnessCatalogues catalogues; /** Read-then-write, so a slow forge cannot hold an item lock; the write re-checks under the lock. */ private static final int MAX_ITEMS_PER_SWEEP = 5; @@ -207,6 +208,9 @@ private Result attempt(String id, boolean again, String actor, var unpriced = pricer.unpricedTypes(setup.model(), setup.harness()); if (!unpriced.isEmpty()) return refuse(id, generation, expectedRevision, "model_pricing_incomplete:" + unpriced.stream().map(Enum::name).collect(java.util.stream.Collectors.joining(","))); + // The setup was checked against the image it was saved for; the harness may run another now. + var cannotRun = catalogues.refusal(setup.harness(), setup.model(), setup.effort()); + if (cannotRun.isPresent()) return refuse(id, generation, expectedRevision, cannotRun.get()); } catch (dev.codespire.orchestrator.llm.LlmModelRegistry.CatalogueUnavailable unavailable) { return refuse(id, generation, expectedRevision, "catalogue_unavailable"); } diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/work/WorkRunAssemblyRefusals.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/work/WorkRunAssemblyRefusals.java index c62d4e8a..a3991151 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/work/WorkRunAssemblyRefusals.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/work/WorkRunAssemblyRefusals.java @@ -20,7 +20,8 @@ final class WorkRunAssemblyRefusals { /** Reasons the dashboard has a sentence for, and which carry no payload at all. */ private static final Set NAMED = Set.of( "factory_account_unavailable", "harness_credential_unavailable", "deployment_spend_cap_reached", - "model_pricing_unavailable", "model_disabled", "catalogue_unavailable"); + "model_pricing_unavailable", "model_disabled", "catalogue_unavailable", + "model_not_run_by_harness", "effort_not_offered", "effort_unverifiable"); /** The one reason that carries a payload: "model_pricing_incomplete:CACHED_INPUT,REASONING". */ private static final String PRICING = "model_pricing_incomplete"; diff --git a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/work/WorkPreparationSweepTest.java b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/work/WorkPreparationSweepTest.java index a479a872..d0e52bc7 100644 --- a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/work/WorkPreparationSweepTest.java +++ b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/work/WorkPreparationSweepTest.java @@ -78,14 +78,38 @@ private void awaitHeadRequest() throws Exception { throw new AssertionError("the sweep never asked the forge for the branch head"); } + /** What the codex image says it runs: one model, with the levels given. */ + private void codexRuns(String slug, String... levels) { + catalogues.record(new dev.codespire.contract.event.HarnessImageResult.Described("TEST-request", "codex", + config.agentImage().get("codex"), dev.codespire.contract.event.HarnessImageResult.Status.OK, + java.util.List.of(new dev.codespire.contract.event.HarnessImageResult.Model(slug, slug, "medium", + java.util.List.of(levels), true, 1)))); + } + + /** + * The setup was saved when nothing said what codex runs; the image now says it does not run that + * model. The review of PR #167 found this still opened an approval for a build that cannot run. + */ + @Test + void aSetupTheHarnessNoLongerRunsIsNotPrepared() throws Exception { + try { + codexRuns("TEST-some-other-model", "medium"); + String id = admit("assisted", 82); + + sweep.sweep(); + + assertNull(store.load(id).preparation()); + assertEquals("model_not_run_by_harness", reason(id)); + } finally { + executeWith("DELETE FROM harness_catalogue"); + } + } + /** The level saved with the build setup is the level the prepared task binds (M3.5 part M). */ @Test void theSavedThinkingLevelIsCopiedIntoThePreparedTask() throws Exception { try { - catalogues.record(new dev.codespire.contract.event.HarnessImageResult.Described("TEST-request", "codex", - config.agentImage().get("codex"), dev.codespire.contract.event.HarnessImageResult.Status.OK, - java.util.List.of(new dev.codespire.contract.event.HarnessImageResult.Model(model, model, "medium", - java.util.List.of("medium", "high"), true, 1)))); + codexRuns(model, "medium", "high"); defaults.save(repository, new BuildDefaults.Input(defaults.get(repository).revision(), "main", "codex", model, "high"), "TEST-prepared-admin"); String id = admit("assisted", 81); diff --git a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/work/WorkRunDispatchTest.java b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/work/WorkRunDispatchTest.java index fd80a1bc..44b0ccdb 100644 --- a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/work/WorkRunDispatchTest.java +++ b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/work/WorkRunDispatchTest.java @@ -35,6 +35,7 @@ class WorkRunDispatchTest extends WorkPreparedFixture { } /** The level the approved binding hashed is the level the build is sent with (M3.5 part M). */ @Test void theApprovedThinkingLevelIsTheOneTheBuildRunsAt() throws Exception { + codexRuns(model,"medium","high"); String id=admit("autonomous",57);var plain=preparation("TEST-prepared-admin"); var atLevel=new WorkPreparation(plain.specification(),plain.plan(),plain.baseBranch(),plain.baseCommit(),plain.harness(), plain.model(),plain.registeredBy(),WorkPreparation.EFFORT_BINDING,"high"); @@ -43,6 +44,21 @@ class WorkRunDispatchTest extends WorkPreparedFixture { assertEquals("high",heldCommands.getLast().execution().reasoningEffort()); assertEquals(atLevel.binding(),heldCommands.getLast().work().preparationBinding()); } + /** Approved while codex ran this model; by dispatch its image no longer does (review of PR #167). */ + @Test void aModelTheHarnessNoLongerRunsCannotStartABuild() throws Exception { + String id=ready();codexRuns("TEST-some-other-model","medium"); + dispatcher.drain(); + assertEquals(0,runCount(id));assertTrue(dispatched.isEmpty()); + assertEquals("model_not_run_by_harness",store.load(id).reason()); + } + @Inject dev.codespire.orchestrator.factory.HarnessCatalogues catalogues; + @Inject dev.codespire.orchestrator.factory.FactoryConfig factoryConfig; + private void codexRuns(String slug,String... levels) { + catalogues.record(new dev.codespire.contract.event.HarnessImageResult.Described("TEST-request","codex", + factoryConfig.agentImage().get("codex"),dev.codespire.contract.event.HarnessImageResult.Status.OK, + List.of(new dev.codespire.contract.event.HarnessImageResult.Model(slug,slug,"medium",List.of(levels),true,1)))); + } + @org.junit.jupiter.api.AfterEach void forgetTheCatalogue() throws Exception {executeWith("DELETE FROM harness_catalogue");} @Inject RunResultSaga saga; @Inject dev.codespire.orchestrator.factory.WorkRunAssembly assembly; String ready() throws Exception {String id=admit("autonomous",56);register(id);return id;} diff --git a/spire-run-worker/src/main/java/dev/codespire/runworker/HarnessImageWorker.java b/spire-run-worker/src/main/java/dev/codespire/runworker/HarnessImageWorker.java index 4d075156..ca0d4165 100644 --- a/spire-run-worker/src/main/java/dev/codespire/runworker/HarnessImageWorker.java +++ b/spire-run-worker/src/main/java/dev/codespire/runworker/HarnessImageWorker.java @@ -41,8 +41,14 @@ public class HarnessImageWorker { @ConfigProperty(name = "spire.run.result-ack-seconds") long ackSeconds; + /* + * In order, and keyed by harness on the way out: the orchestrator keeps the LAST answer for a + * harness, so two answers for one harness must arrive in the order they were given. Unordered + * processing, or a random key spreading answers over partitions, let an older answer land after a + * newer one and replace it (review of PR #167). The questions are few, so order costs nothing. + */ @Incoming("harness-image-commands-in") - @Blocking(ordered = false) + @Blocking public CompletionStage onCommand(Message message) { if (message.getPayload() instanceof HarnessImageCommand.Describe describe) { publish(describe(describe)); @@ -67,9 +73,15 @@ HarnessImageResult.Described describe(HarnessImageCommand.Describe command) { read.status(), read.models()); } + private static String keyOf(HarnessImageResult result) { + return switch (result) { + case HarnessImageResult.Described described -> described.harness(); + }; + } + private void publish(HarnessImageResult result) { try { - results.send(Record.of(result.requestId(), result)).toCompletableFuture().get(ackSeconds, TimeUnit.SECONDS); + results.send(Record.of(keyOf(result), result)).toCompletableFuture().get(ackSeconds, TimeUnit.SECONDS); } catch (InterruptedException interrupted) { Thread.currentThread().interrupt(); } catch (RuntimeException | java.util.concurrent.ExecutionException diff --git a/spire-run-worker/src/test/java/dev/codespire/runworker/HarnessImageWorkerTest.java b/spire-run-worker/src/test/java/dev/codespire/runworker/HarnessImageWorkerTest.java new file mode 100644 index 00000000..410f0d10 --- /dev/null +++ b/spire-run-worker/src/test/java/dev/codespire/runworker/HarnessImageWorkerTest.java @@ -0,0 +1,83 @@ +package dev.codespire.runworker; + +import com.fasterxml.jackson.databind.ObjectMapper; +import dev.codespire.contract.command.HarnessImageCommand; +import dev.codespire.contract.event.HarnessImageResult; +import dev.codespire.runtime.RunRuntime; +import io.smallrye.reactive.messaging.annotations.Blocking; +import io.smallrye.reactive.messaging.kafka.Record; +import org.eclipse.microprofile.reactive.messaging.Emitter; +import org.eclipse.microprofile.reactive.messaging.Message; +import org.junit.jupiter.api.Test; + +import java.lang.reflect.Proxy; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionStage; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * The orchestrator keeps the LAST answer it hears for a harness, so the answers for one harness must + * reach it in the order they were given (review of PR #167). + */ +class HarnessImageWorkerTest { + + @Test + void anAnswerIsKeyedByItsHarnessSoOneHarnessKeepsOnePartition() { + List> sent = new ArrayList<>(); + HarnessImageWorker worker = new HarnessImageWorker(); + // Only the label read is reached; any other runtime call returns null and would fail loudly. + worker.runtime = (RunRuntime) Proxy.newProxyInstance(RunRuntime.class.getClassLoader(), new Class[] { RunRuntime.class }, + (proxy, method, args) -> method.getName().equals("imageLabels") ? Map.of() : null); + worker.mapper = new ObjectMapper(); + worker.ackSeconds = 5; + worker.results = new Capturing(sent); + + worker.onCommand(Message.of(new HarnessImageCommand.Describe("TEST-request-1", "codex", "TEST-image"))); + + assertEquals(1, sent.size()); + assertEquals("codex", sent.getFirst().key()); + } + + /** One partition keeps order only if the worker does not answer two of its messages at once. */ + @Test + void theQuestionsAreAnsweredOneAtATime() throws NoSuchMethodException { + Blocking blocking = HarnessImageWorker.class.getMethod("onCommand", Message.class).getAnnotation(Blocking.class); + assertTrue(blocking.ordered()); + } + + private record Capturing(List> sent) implements Emitter> { + @Override + public CompletionStage send(Record record) { + sent.add(record); + return CompletableFuture.completedFuture(null); + } + + @Override + public >> void send(M message) { + throw new UnsupportedOperationException("the worker sends records, not messages"); + } + + @Override + public void complete() { + } + + @Override + public void error(Exception e) { + } + + @Override + public boolean isCancelled() { + return false; + } + + @Override + public boolean hasRequests() { + return true; + } + } +} diff --git a/spire-run-worker/src/test/java/dev/codespire/runworker/ModelCatalogueLabelTest.java b/spire-run-worker/src/test/java/dev/codespire/runworker/ModelCatalogueLabelTest.java index 4d9e76cc..08b45032 100644 --- a/spire-run-worker/src/test/java/dev/codespire/runworker/ModelCatalogueLabelTest.java +++ b/spire-run-worker/src/test/java/dev/codespire/runworker/ModelCatalogueLabelTest.java @@ -29,6 +29,13 @@ private static ModelCatalogueLabel.Read read(String json) { return ModelCatalogueLabel.of(Map.of(ModelCatalogueLabel.LABEL, label(json)), MAPPER); } + /** A level no later hop would accept makes the list unreadable, not a list that fails a day later. */ + @Test + void aLevelOfTheWrongShapeMakesTheWholeLabelUnreadable() { + assertEquals(HarnessImageResult.Status.UNREADABLE, + read("[{\"s\":\"TEST-fast\",\"d\":\"low\",\"e\":[\"low\",\"x-high\"]}]").status()); + } + @Test void aWellFormedCatalogueYieldsEveryModelWithItsOwnLevels() { var read = read(""" diff --git a/spire-ui/src/components/repositories/factory/BuildStep.tsx b/spire-ui/src/components/repositories/factory/BuildStep.tsx index fe74890b..06b74314 100644 --- a/spire-ui/src/components/repositories/factory/BuildStep.tsx +++ b/spire-ui/src/components/repositories/factory/BuildStep.tsx @@ -74,7 +74,11 @@ export default function BuildStep({ repositoryId, defaults, open, setOpen, chang const picked = offered.find(choice => choice.value === form.model); // An unresolved model is UNKNOWN, not complete: before the catalogue answers, and for a saved name the // catalogue no longer offers, there is nothing to judge — so Save waits rather than guessing. - const complete = !!form.baseBranch.trim() && !!form.harness && !!picked && picked.blocked === null; + // A level is sent only when the chosen model visibly offers it. A saved level whose list is no longer + // known has no control on screen, so it is named below with a way to drop it, and Save waits. + const levels = known?.status === 'OK' ? known.offered.find(model => model.slug === form.model)?.efforts ?? [] : []; + const levelUnusable = !!form.effort && !levels.includes(form.effort); + const complete = !!form.baseBranch.trim() && !!form.harness && !!picked && picked.blocked === null && !levelUnusable; return 0 ? 'done' : 'missing'} actions={!editing && }> @@ -92,7 +96,8 @@ export default function BuildStep({ repositoryId, defaults, open, setOpen, chang {head && head {head.commit.slice(0, 7)}{head.account === 'REVIEWER' ? ' · read with the reviewer account; no usable factory account was available' : ''}}

- setForm(previous => ({ ...previous, harness: event.target.value, effort: '' }))}> {choices.harnesses.map(harness => )} {form.harness && !choices.harnesses.includes(form.harness) && } @@ -103,6 +108,9 @@ export default function BuildStep({ repositoryId, defaults, open, setOpen, chang {picked?.blocked &&

{form.model} has {picked.blocked}. {form.harness} reports those token types, and an API-key run needs a rate for each — or a mark in Settings → LLM that the vendor does not bill it.

} + {levelUnusable && picked &&

+ The thinking level {form.effort} cannot be checked or is not offered for {form.model} here.{' '} +

} {error &&

{error}

} {/* A stale revision cannot be retried from this form: every attempt resends the number it loaded. */} {error.includes('Reload it') &&
diff --git a/spire-ui/src/components/repositories/factory/RepositoryFactory.test.tsx b/spire-ui/src/components/repositories/factory/RepositoryFactory.test.tsx index b2f1668d..e7a0ce6c 100644 --- a/spire-ui/src/components/repositories/factory/RepositoryFactory.test.tsx +++ b/spire-ui/src/components/repositories/factory/RepositoryFactory.test.tsx @@ -331,6 +331,39 @@ describe('build setup', () => { expect(build.saveBuildDefaults).not.toHaveBeenCalled(); }); + // A saved level whose list is no longer known has no control on screen. Sending it anyway got a refusal + // naming a value the operator could not see (review of PR #167), so it is named, and can be dropped. + it('names a saved thinking level it cannot check, and offers the model default instead', async () => { + vi.mocked(build.buildDefaults).mockResolvedValue(buildSetup({ revision: 2, baseBranch: 'main', harness: 'codex', model: 'TEST-model', effort: 'high' })); + renderFactory(); + fireEvent.click(within(await step(5)).getByRole('button', { name: 'Change' })); + expect(await screen.findByRole('alert')).toHaveTextContent('The thinking level high cannot be checked'); + expect(screen.getByRole('button', { name: 'Save build setup' })).toBeDisabled(); + + fireEvent.click(screen.getByRole('button', { name: 'Use the model default' })); + fireEvent.click(screen.getByRole('button', { name: 'Save build setup' })); + await waitFor(() => expect(build.saveBuildDefaults).toHaveBeenCalledWith(repository.id, + { expectedRevision: 2, baseBranch: 'main', harness: 'codex', model: 'TEST-model', effort: null })); + }); + + // A level belongs to one harness's list: switching away and back must not bring it back unseen. + it('drops the thinking level when the harness changes', async () => { + const types = ['INPUT', 'CACHED_INPUT', 'CACHE_WRITE', 'OUTPUT', 'REASONING']; + const runs = { status: 'OK' as const, offered: [{ slug: 'TEST-model', displayName: 'TEST model', defaultEffort: 'medium', + efforts: ['medium', 'high'], visible: true, priority: 1 }] }; + vi.mocked(build.buildOptions).mockResolvedValue({ harnesses: ['codex', 'TEST-other'], reportedTypes: { codex: types, 'TEST-other': types }, + models: { codex: runs, 'TEST-other': runs } }); + vi.mocked(build.buildDefaults).mockResolvedValue(buildSetup({ revision: 2, baseBranch: 'main', harness: 'codex', model: 'TEST-model', effort: 'high' })); + renderFactory(); + fireEvent.click(within(await step(5)).getByRole('button', { name: 'Change' })); + expect(await screen.findByRole('combobox', { name: 'Thinking level' })).toHaveValue('high'); + + fireEvent.change(screen.getByRole('combobox', { name: 'Harness' }), { target: { value: 'TEST-other' } }); + fireEvent.change(screen.getByRole('combobox', { name: 'Harness' }), { target: { value: 'codex' } }); + + expect(screen.getByRole('combobox', { name: 'Thinking level' })).toHaveValue(''); + }); + // Before the catalogue answers there is no model to judge, so Save waits rather than guessing. it('waits for the catalogue before allowing a saved setup to be saved again', async () => { vi.mocked(build.buildDefaults).mockResolvedValue(buildSetup({ revision: 2, baseBranch: 'main', harness: 'codex', model: 'TEST-model' })); diff --git a/spire-ui/src/components/work-items/workReasons.ts b/spire-ui/src/components/work-items/workReasons.ts index 7ab72cd3..a5281ffa 100644 --- a/spire-ui/src/components/work-items/workReasons.ts +++ b/spire-ui/src/components/work-items/workReasons.ts @@ -22,6 +22,9 @@ const REASONS = new Map(Object.entries({ model_name_invalid: 'That model name has characters a run cannot pass to the agent.', catalogue_unavailable: 'The model catalogue could not be read, so whether that model may run is unknown. The build was not started.', model_disabled: 'That model is switched off in the catalogue, so a run cannot call it.', + model_not_run_by_harness: 'The agent image for that harness does not run this model. Choose one of the models it offers in the build setup.', + effort_not_offered: 'That model does not offer this thinking level. Choose one of its own levels in the build setup.', + effort_unverifiable: 'The models this harness runs are not known yet, so the thinking level cannot be checked. Use the model default, or wait for the list.', model_pricing_unavailable: 'That model has no price for input and output tokens, so a run with it would be refused.', harness_unconfigured: 'This deployment has no agent image for that harness. Choose one of the offered names.', model_unknown: 'That model is not in the catalogue, or it is switched off. Choose an enabled model.', From 8660fe5d0be37fbb042da4729f747dcc50ff9a36 Mon Sep 17 00:00:00 2001 From: Artjoms Stukans Date: Wed, 23 Sep 2026 01:26:00 +0200 Subject: [PATCH 6/7] Run each build in the exact image its model list was read from Two run workers can hold different images under one mutable tag, and the deployment default is spire-agent-codex:latest. The model list could be read from one worker's image while the build ran on another. The run worker now reads the labels and the exact image in one inspect: the registry digest when the image has one, else the daemon's image id. The answer carries the pin, the orchestrator stores it with the list (V81), and every run it sends - item builds, REST dispatches and /fix runs - uses the pin instead of the tag. An item build takes its model check and its pin from one read of the cache. With nothing read yet, runs use the tag as before. A local-only image pinned by id fails to pull on a worker that does not hold it, which is the honest outcome. RunRuntime.imageLabels becomes describeImage. Recorded in UNVERIFIED: no test pulls a registry image or runs two workers. --- docs/UNVERIFIED.md | 1 + ...actory-m35-one-ticket-to-a-build-design.md | 19 +++++++ .../contract/event/HarnessImageResult.java | 17 +++++- .../event/HarnessImageResultModelTest.java | 9 +++ .../factory/FixRunDispatcher.java | 6 +- .../factory/HarnessCatalogues.java | 56 +++++++++++++++---- .../orchestrator/factory/RunResource.java | 6 +- .../orchestrator/factory/WorkRunAssembly.java | 7 ++- .../migration/V81__harness_catalogue_pin.sql | 4 ++ .../factory/FixRunDispatcherTest.java | 19 +++++++ .../factory/HarnessCataloguesTest.java | 18 ++++++ .../orchestrator/factory/RunResourceTest.java | 30 ++++++++++ .../RepositoryResolverCutoverTest.java | 4 ++ .../work/WorkRunDispatchTest.java | 5 +- .../runworker/HarnessImageWorker.java | 8 +-- .../runworker/HarnessImageWorkerTest.java | 6 +- .../runtime/docker/DockerRunRuntime.java | 27 ++++++++- .../runtime/docker/DockerImageLabelsIT.java | 8 ++- .../runtime/docker/DockerImagePinTest.java | 44 +++++++++++++++ .../codespire/runtime/ImageDescription.java | 21 +++++++ .../dev/codespire/runtime/RunRuntime.java | 10 +++- 21 files changed, 294 insertions(+), 31 deletions(-) create mode 100644 spire-orchestrator/src/main/resources/db/migration/V81__harness_catalogue_pin.sql create mode 100644 spire-runtime-docker/src/test/java/dev/codespire/runtime/docker/DockerImagePinTest.java create mode 100644 spire-runtime/src/main/java/dev/codespire/runtime/ImageDescription.java diff --git a/docs/UNVERIFIED.md b/docs/UNVERIFIED.md index 84b90152..49d0459e 100644 --- a/docs/UNVERIFIED.md +++ b/docs/UNVERIFIED.md @@ -468,6 +468,7 @@ Each has a runbook mode. None has been run by an operator. | The whole M1 lifecycle against a real forge | **Mode Q** | Cancel, steer, the watchdog, the push gate and the charge ledger have only ever met a WireMock LLM and a local origin | | Corporate-only bundle → the failure it produces | Mode R §5 | The documented trap (internal forge works, model API fails) is asserted nowhere; it is the mistake an operator will actually make | | A private-registry pull | Mode S §4 | Nothing pulls from a private registry in any test. `authFor` and the attachment are unit-tested; the *pull* is not | +| **Runs pinned to the image their model list came from** (M3.5 part M, 2026-09-23) | none yet | Choosing the pin is unit-tested against given daemon answers, and one real-daemon test pins a LOCAL build by its image id. No test pulls a registry image and pins it by its registry digest, and none runs two workers. So "two workers holding different images under one tag run the same one" is argued from the code, not watched. A local-only image is pinned by an id that exists on one daemon only: on a second worker such a run fails to pull — by design, but unobserved | | **OIDC sessions actually renew instead of re-authenticating** | **Mode J check 11** (2026-09-10) | The bug it fixes needs a real browser, a real Keycloak and **fifteen elapsed minutes**. No suite here has any of the three: there are zero WebSocket client tests, and nothing observes a token reaching its `exp`. `OidcSessionsAreRenewedTest` asserts the four `application.yml` files *say* renewal is on — it cannot assert Quarkus *does* it | **Evidence needed.** An operator pass per mode. These are cheap and the runbooks are written. diff --git a/docs/superpowers/specs/2026-09-16-factory-m35-one-ticket-to-a-build-design.md b/docs/superpowers/specs/2026-09-16-factory-m35-one-ticket-to-a-build-design.md index a78d1f6c..6f79786d 100644 --- a/docs/superpowers/specs/2026-09-16-factory-m35-one-ticket-to-a-build-design.md +++ b/docs/superpowers/specs/2026-09-16-factory-m35-one-ticket-to-a-build-design.md @@ -592,6 +592,25 @@ The vendor's CLI does not check the level (measured on 2026-09-22: a nonsense va every hop accepts only a short lower-case word. That keeps the value from closing a quote or naming a second config key, and it is why the save already refuses a level the model does not declare. +### 6A.4c A run uses the image its list was read from + +A tag can move, and two run workers can hold different images under one tag. The deployment default is +`spire-agent-codex:latest`, and a worker runs its own copy without pulling a newer one. So the list could be +read from one worker's image while the build ran on another's (review of PR #167, operator's option B). + +The run worker now reads the labels **and** the exact image in one inspect, and answers with both: the +registry digest (`repo@sha256:…`) when the image has one, else the daemon's image id. The orchestrator +stores the pin with the list (V81). Every run it sends — an item build, a REST dispatch, a /fix — uses the +pin instead of the tag. An item build takes the check and the pin from one read of the cache, so it cannot +be checked against one answer and run in another's image. + +- **Nothing read yet:** the tag, as before part M. +- **A registry image:** every worker pulls the same digest. +- **A local-only image (dev):** the id runs on the daemon that built it. Elsewhere the pull fails and the + run fails. That is the honest answer: that worker does not hold the image the list describes. +- **Rebuilt under the same tag:** runs keep the old pin until the next refresh (`spire.harness-catalogue-interval`, + 10 minutes), and the old list goes with it, so the two still agree. + ### 6A.5 The cost, stated The image contract gains a clause, so an operator building their own agent image must produce that label diff --git a/spire-contract/src/main/java/dev/codespire/contract/event/HarnessImageResult.java b/spire-contract/src/main/java/dev/codespire/contract/event/HarnessImageResult.java index 55004d52..3ff54b70 100644 --- a/spire-contract/src/main/java/dev/codespire/contract/event/HarnessImageResult.java +++ b/spire-contract/src/main/java/dev/codespire/contract/event/HarnessImageResult.java @@ -64,10 +64,25 @@ enum Status { /** * @param models empty unless {@code status} is {@link Status#OK} + * @param pinnedImage the exact image that was read — a digest reference or an image id — or null + * when it could not be reached. A run of this harness uses it rather than the tag, + * so it runs the image these models were read from (review of PR #167). Null in an + * answer sent before pins existed. */ - record Described(String requestId, String harness, String image, Status status, List models) + record Described(String requestId, String harness, String image, Status status, List models, + String pinnedImage) implements HarnessImageResult { + + public Described(String requestId, String harness, String image, Status status, List models) { + this(requestId, harness, image, status, models, null); + } + public Described { + pinnedImage = pinnedImage == null || pinnedImage.isBlank() ? null : pinnedImage; + // It becomes the image a container is created from; a reference has no space or control in it. + if (pinnedImage != null && (pinnedImage.length() > 512 || !pinnedImage.matches("\\S+") + || pinnedImage.chars().anyMatch(Character::isISOControl))) + throw new IllegalArgumentException("A pinned image is one reference, was: " + pinnedImage); if (requestId == null || requestId.isBlank()) throw new IllegalArgumentException("A request id is required"); Objects.requireNonNull(harness, "harness"); Objects.requireNonNull(image, "image"); diff --git a/spire-contract/src/test/java/dev/codespire/contract/event/HarnessImageResultModelTest.java b/spire-contract/src/test/java/dev/codespire/contract/event/HarnessImageResultModelTest.java index 35bc4d44..c5fc3501 100644 --- a/spire-contract/src/test/java/dev/codespire/contract/event/HarnessImageResultModelTest.java +++ b/spire-contract/src/test/java/dev/codespire/contract/event/HarnessImageResultModelTest.java @@ -21,6 +21,15 @@ void aDeclaredLevelEveryLaterHopWouldRefuseIsRefusedHere() { List.of("medium"), true, 1)); } + /** The pin becomes the image a container is created from, so it is one reference and nothing more. */ + @Test + void aPinnedImageIsOneReference() { + assertThrows(IllegalArgumentException.class, () -> new HarnessImageResult.Described("TEST-request", "codex", + "TEST-image", HarnessImageResult.Status.OK, List.of(), "TEST-image --privileged")); + assertEquals(null, new HarnessImageResult.Described("TEST-request", "codex", "TEST-image", + HarnessImageResult.Status.OK, List.of()).pinnedImage(), "an answer from before pins has none"); + } + @Test void noDeclaredDefaultIsKeptAsNoneRatherThanRefused() { assertEquals("", new HarnessImageResult.Model("TEST-model", "TEST", "", List.of("medium"), true, 1).defaultEffort()); diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/FixRunDispatcher.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/FixRunDispatcher.java index b4928a42..4750ff89 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/FixRunDispatcher.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/FixRunDispatcher.java @@ -50,6 +50,9 @@ public class FixRunDispatcher { @Inject FixDispatch plans; + @Inject + HarnessCatalogues catalogues; + @Inject FactoryRunProjection runs; @@ -204,7 +207,8 @@ public Result dispatch(String reviewId, RepoRef repo, String threadRef, String c RunCommand.ExecuteRun command = new RunCommand.ExecuteRun(planned.runId(), repo, FactoryCloneUrls.cloneUrl(planned.scmType(), account.get().baseUrl(), repo), planned.baseBranch(), planned.baseCommit(), planned.branch(), - FixPrompt.of(spec.get()), harness, model, config.agentImage().get(harness), + // The exact image the run worker last read for this harness, so every worker runs the same one. + FixPrompt.of(spec.get()), harness, model, catalogues.imageFor(harness), NO_EXTRA_PROTECTED_PATHS, config.wallClockSeconds(), credentials.packScm(planned.runId(), account.get().botUsername(), account.get().secret()), credentials.packHarness(planned.runId(), credential.apiKey())) diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCatalogues.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCatalogues.java index b940fd53..5de18faa 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCatalogues.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCatalogues.java @@ -50,7 +50,7 @@ public class HarnessCatalogues { /** What a screen shows for one harness. */ public record Catalogue(String harness, String image, HarnessImageResult.Status status, - List models, Instant observedAt) { + List models, Instant observedAt, String pinnedImage) { /** The models to OFFER: the ones the vendor wants shown, in the vendor's own order. */ public List offered() { @@ -110,15 +110,17 @@ public void record(HarnessImageResult.Described answer) { return; } try (Connection c = dataSource.getConnection(); PreparedStatement ps = c.prepareStatement(""" - INSERT INTO harness_catalogue (harness, image, status, models, observed_at) - VALUES (?, ?, ?, ?::jsonb, now()) + INSERT INTO harness_catalogue (harness, image, status, models, observed_at, pinned_image) + VALUES (?, ?, ?, ?::jsonb, now(), ?) ON CONFLICT (harness) DO UPDATE - SET image=excluded.image, status=excluded.status, models=excluded.models, observed_at=now() + SET image=excluded.image, status=excluded.status, models=excluded.models, observed_at=now(), + pinned_image=excluded.pinned_image """)) { ps.setString(1, answer.harness()); ps.setString(2, answer.image()); ps.setString(3, answer.status().name()); ps.setString(4, mapper.writeValueAsString(answer.models())); + ps.setString(5, answer.pinnedImage()); ps.executeUpdate(); } catch (SQLException | JsonProcessingException failure) { throw new IllegalStateException("The model catalogue for " + answer.harness() + " could not be stored", failure); @@ -134,12 +136,44 @@ ON CONFLICT (harness) DO UPDATE * an approval, and start a build, against another that does not run it. */ public Optional refusal(String harness, String model, String effort) { - var catalogue = get(harness).filter(known -> known.status() == HarnessImageResult.Status.OK); - if (catalogue.isEmpty()) return effort == null ? Optional.empty() : Optional.of("effort_unverifiable"); + return Optional.ofNullable(admit(harness, model, effort).refusal()); + } + + /** + * What a build of this harness may run: a refusal, or the image to run it in. + * + * @param refusal why it may not run, or null + * @param image the image the checked list was read from, or the configured tag when nothing pinned one + */ + public record Admission(String refusal, String image) { } + + /** + * The check and the image from ONE read of the cache, so the list a build was checked against and + * the image it runs in cannot come from two different answers. + */ + public Admission admit(String harness, String model, String effort) { + Optional known = get(harness); + String image = imageOf(harness, known); + var catalogue = known.filter(read -> read.status() == HarnessImageResult.Status.OK); + if (catalogue.isEmpty()) return new Admission(effort == null ? null : "effort_unverifiable", image); var runs = catalogue.get().find(model); - if (runs.isEmpty()) return Optional.of("model_not_run_by_harness"); - if (effort != null && !runs.get().efforts().contains(effort)) return Optional.of("effort_not_offered"); - return Optional.empty(); + if (runs.isEmpty()) return new Admission("model_not_run_by_harness", image); + if (effort != null && !runs.get().efforts().contains(effort)) return new Admission("effort_not_offered", image); + return new Admission(null, image); + } + + /** + * The image a run of this harness uses: the exact one the run worker last read, else the tag. + * + *

A tag can move, and two workers can hold different images under it; the pin cannot. Where + * nothing has been read yet the tag is all there is, as before part M. + */ + public String imageFor(String harness) { + return imageOf(harness, get(harness)); + } + + private String imageOf(String harness, Optional known) { + return known.map(Catalogue::pinnedImage).orElseGet(() -> config.agentImage().get(harness)); } /** @@ -150,7 +184,7 @@ public Optional refusal(String harness, String model, String effort) { */ public Optional get(String harness) { try (Connection c = dataSource.getConnection(); PreparedStatement ps = c.prepareStatement( - "SELECT harness, image, status, models::text, observed_at FROM harness_catalogue WHERE harness=?")) { + "SELECT harness, image, status, models::text, observed_at, pinned_image FROM harness_catalogue WHERE harness=?")) { ps.setString(1, harness); try (ResultSet rs = ps.executeQuery()) { if (!rs.next()) return Optional.empty(); @@ -159,7 +193,7 @@ public Optional get(String harness) { return Optional.of(new Catalogue(rs.getString("harness"), rs.getString("image"), HarnessImageResult.Status.valueOf(rs.getString("status")), mapper.readValue(rs.getString("models"), new TypeReference>() { }), - rs.getTimestamp("observed_at").toInstant())); + rs.getTimestamp("observed_at").toInstant(), rs.getString("pinned_image"))); } } catch (SQLException | JsonProcessingException failure) { throw new IllegalStateException("The model catalogue for " + harness + " could not be read", failure); diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/RunResource.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/RunResource.java index ac6b4b15..81cb5b9e 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/RunResource.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/RunResource.java @@ -73,6 +73,9 @@ public class RunResource { @Inject MachineAccounts machineAccounts; + @Inject + HarnessCatalogues catalogues; + @Inject dev.codespire.orchestrator.repository.RepositoryRegistry repositories; @@ -161,7 +164,8 @@ public Response dispatch(DispatchRequest req) { // the review path already keeps); a dead-lettered command lands in dlq_entry.payload as sent. RunCommand.ExecuteRun command = new RunCommand.ExecuteRun(runId, repo, FactoryCloneUrls.cloneUrl(in.scmType(), account.baseUrl(), repo), - in.baseBranch(), in.baseCommit(), branch, in.prompt(), in.harness(), in.model(), in.agentImage(), + // The exact image the run worker last read for this harness, so every worker runs the same one. + in.baseBranch(), in.baseCommit(), branch, in.prompt(), in.harness(), in.model(), catalogues.imageFor(in.harness()), List.of(), config.wallClockSeconds(), runCredentials.packScm(runId, account.botUsername(), account.secret()), runCredentials.packHarness(runId, credential.apiKey())); diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/WorkRunAssembly.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/WorkRunAssembly.java index 7b72be02..14ab1660 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/WorkRunAssembly.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/WorkRunAssembly.java @@ -53,8 +53,8 @@ public Prepared assemble(Connection c,WorkSourceRegistry.Source source,WorkItemE throw new IllegalStateException("catalogue_unavailable"); } // Approved against the image the harness ran then; it may run another by now (§6A.4b). - var cannotRun=catalogues.refusal(in.harness(),in.model(),item.preparation().effort()); - if(cannotRun.isPresent())throw new IllegalStateException(cannotRun.get()); + var admission=catalogues.admit(in.harness(),in.model(),item.preparation().effort()); + if(admission.refusal()!=null)throw new IllegalStateException(admission.refusal()); var unpriced=pricer.unpricedTypes(in.model(),in.harness()); if(!unpriced.isEmpty())throw new IllegalStateException("model_pricing_incomplete:" +unpriced.stream().map(Enum::name).collect(java.util.stream.Collectors.joining(","))); @@ -63,7 +63,8 @@ public Prepared assemble(Connection c,WorkSourceRegistry.Source source,WorkItemE String id=RunIds.of(source.scm(),in.workspace(),in.slug(),subject,1),branch=DispatchRequestParser.RUN_BRANCH_PREFIX+subject; long wall=Math.min(config.wallClockSeconds(),Math.subtractExact(item.policy().limits().maxWallClockSeconds(),item.progress().wallSeconds())); var command=new RunCommand.ExecuteRun(id,source.repository(),FactoryCloneUrls.cloneUrl(source.scm(),account.baseUrl(),source.repository()), - in.baseBranch(),in.baseCommit(),branch,in.prompt(),in.harness(),in.model(),in.agentImage(), + // The image the checked list was read from, not the tag, which may name another by now. + in.baseBranch(),in.baseCommit(),branch,in.prompt(),in.harness(),in.model(),admission.image(), item.policy().limits().protectedPaths().stream().sorted().toList(),wall, credentials.packScm(id,account.botUsername(),account.secret()),credentials.packHarness(id,chosen.member().apiKey())) // The level the approved binding hashed, so the build runs at what was approved (M3.5 part M). diff --git a/spire-orchestrator/src/main/resources/db/migration/V81__harness_catalogue_pin.sql b/spire-orchestrator/src/main/resources/db/migration/V81__harness_catalogue_pin.sql new file mode 100644 index 00000000..4a88171c --- /dev/null +++ b/spire-orchestrator/src/main/resources/db/migration/V81__harness_catalogue_pin.sql @@ -0,0 +1,4 @@ +-- M3.5 part M, review of PR #167: the exact image a harness's model list was read from. A run uses it +-- instead of the tag, so two workers holding different images under one tag cannot run a model list +-- that was read from the other. NULL for a row written before pins, and for an image not reached. +ALTER TABLE harness_catalogue ADD COLUMN pinned_image TEXT; diff --git a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/FixRunDispatcherTest.java b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/FixRunDispatcherTest.java index 8b3e0c33..76992aac 100644 --- a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/FixRunDispatcherTest.java +++ b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/FixRunDispatcherTest.java @@ -73,6 +73,13 @@ class FixRunDispatcherTest { private FixRunDispatcher dispatcher() { FixRunDispatcher dispatcher = new FixRunDispatcher(); + dispatcher.catalogues = new HarnessCatalogues() { + // The real one reads the database; this plain unit test answers what the worker last read. + @Override + public String imageFor(String harness) { + return pinned != null ? pinned : "spire-agent-codex:1"; + } + }; dispatcher.plans = new FixDispatch() { @Override public Plan plan(String reviewId, String threadRef, RepoRef repo) { @@ -296,6 +303,18 @@ void theRowNamesTheReviewTheFindingAndTheCommentThatAsked() { *

FR-F27's premise is that the finding is a complete task specification; a command carrying a * location and no description would be a paid run on a line number. */ + /** What the run worker last read for the harness; null means nothing was, so the tag is used. */ + private String pinned; + + /** A fix run uses the exact image the worker read, so every worker runs the same one (review of PR #167). */ + @Test + void aFixRunUsesTheExactImageTheWorkerRead() { + pinned = "TEST-registry.invalid/agent@sha256:0000000000000000000000000000000000000000000000000000000000000000"; + dispatch(); + + assertEquals("TEST-registry.invalid/agent@sha256:0000000000000000000000000000000000000000000000000000000000000000", launched.getFirst().agentImage()); + } + @Test void theRunIsToldTheFindingAndTheDeploymentsHarness() { dispatch(); diff --git a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessCataloguesTest.java b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessCataloguesTest.java index c1ce4a82..8148bd7d 100644 --- a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessCataloguesTest.java +++ b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessCataloguesTest.java @@ -40,6 +40,24 @@ private String image() { return config.agentImage().get(HARNESS); } + /** + * A run uses the exact image the list was read from; with nothing read, the tag, as before part M. + * And an answer about an image the harness has since left pins nothing (review of PR #167). + */ + @Test + void aRunUsesTheImageTheListWasReadFromAndTheTagWhenNothingWasRead() { + assertEquals(image(), catalogues.imageFor(HARNESS), "nothing read yet: the tag"); + + catalogues.record(new HarnessImageResult.Described("TEST-request", HARNESS, image(), + HarnessImageResult.Status.OK, List.of(model("TEST-model", true, 1)), "TEST-registry.invalid/agent@sha256:0000000000000000000000000000000000000000000000000000000000000000")); + assertEquals("TEST-registry.invalid/agent@sha256:0000000000000000000000000000000000000000000000000000000000000000", catalogues.imageFor(HARNESS)); + assertEquals("TEST-registry.invalid/agent@sha256:0000000000000000000000000000000000000000000000000000000000000000", catalogues.admit(HARNESS, "TEST-model", null).image()); + + catalogues.record(new HarnessImageResult.Described("TEST-request-2", HARNESS, "TEST-another-image:1", + HarnessImageResult.Status.OK, List.of(model("TEST-model", true, 1)), "TEST-another@sha256:" + "1".repeat(64))); + assertEquals("TEST-registry.invalid/agent@sha256:0000000000000000000000000000000000000000000000000000000000000000", catalogues.imageFor(HARNESS), "an answer about another image changes nothing"); + } + private static HarnessImageResult.Model model(String slug, boolean visible, int priority) { return new HarnessImageResult.Model(slug, slug, "medium", List.of("low", "medium", "high"), visible, priority); } diff --git a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/RunResourceTest.java b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/RunResourceTest.java index 854b9698..1e049925 100644 --- a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/RunResourceTest.java +++ b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/RunResourceTest.java @@ -595,6 +595,36 @@ void aRequestCannotPinTheHarnessCredential() { .body(containsString("no longer accepted")); } + @Inject HarnessCatalogues catalogues; + @Inject FactoryConfig factoryConfig; + + /** A dispatched run uses the exact image the worker last read, not the tag (review of PR #167). */ + @Test + @TestSecurity(user = "op", roles = "spire-admin") + void aDispatchedRunUsesTheExactImageTheWorkerRead() { + String workspace = workspaceWithFactoryAccount(); + List sent = new java.util.ArrayList<>(); + QuarkusMock.installMockForType(new RunCommandEmitter() { + @Override + public void dispatch(RunCommand command) { + sent.add(command); + } + }, RunCommandEmitter.class); + try { + catalogues.record(new dev.codespire.contract.event.HarnessImageResult.Described("TEST-request", "codex", + factoryConfig.agentImage().get("codex"), dev.codespire.contract.event.HarnessImageResult.Status.NO_CATALOGUE, + List.of(), "TEST-registry.invalid/agent@sha256:0000000000000000000000000000000000000000000000000000000000000000")); + + given().contentType("application/json").body(body(workspace)) + .when().post("/api/runs") + .then().statusCode(201); + + org.junit.jupiter.api.Assertions.assertEquals("TEST-registry.invalid/agent@sha256:0000000000000000000000000000000000000000000000000000000000000000", ((RunCommand.ExecuteRun) sent.getFirst()).agentImage()); + } finally { + sql("DELETE FROM harness_catalogue"); + } + } + @Test @TestSecurity(user = "op", roles = "spire-admin") void dispatchingReturnsADerivedRunId() { diff --git a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/repository/RepositoryResolverCutoverTest.java b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/repository/RepositoryResolverCutoverTest.java index 19196e99..7aef7190 100644 --- a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/repository/RepositoryResolverCutoverTest.java +++ b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/repository/RepositoryResolverCutoverTest.java @@ -210,6 +210,10 @@ private FixRunDispatcher fixDispatcher() throws Exception { // The dispatcher asks this one now; the real method would reach a database from a unit test. @Override public java.util.List unpricedTypes(String model, String harness) { return java.util.List.of(); } }); + // Answers what the worker last read; the real one would reach a database from a unit test. + set(fix, "catalogues", new dev.codespire.orchestrator.factory.HarnessCatalogues() { + @Override public String imageFor(String harness) { return "TEST-image"; } + }); set(fix, "models", new dev.codespire.orchestrator.llm.LlmModelRegistry() { @Override public boolean isDisabled(String model) { return false; } }); diff --git a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/work/WorkRunDispatchTest.java b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/work/WorkRunDispatchTest.java index 44b0ccdb..31d65e5a 100644 --- a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/work/WorkRunDispatchTest.java +++ b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/work/WorkRunDispatchTest.java @@ -43,6 +43,8 @@ class WorkRunDispatchTest extends WorkPreparedFixture { dispatcher.drain(); assertEquals("high",heldCommands.getLast().execution().reasoningEffort()); assertEquals(atLevel.binding(),heldCommands.getLast().work().preparationBinding()); + // The exact image the checked list was read from, not the tag (review of PR #167). + assertEquals("TEST-registry.invalid/agent@sha256:0000000000000000000000000000000000000000000000000000000000000000",heldCommands.getLast().execution().agentImage()); } /** Approved while codex ran this model; by dispatch its image no longer does (review of PR #167). */ @Test void aModelTheHarnessNoLongerRunsCannotStartABuild() throws Exception { @@ -56,7 +58,8 @@ class WorkRunDispatchTest extends WorkPreparedFixture { private void codexRuns(String slug,String... levels) { catalogues.record(new dev.codespire.contract.event.HarnessImageResult.Described("TEST-request","codex", factoryConfig.agentImage().get("codex"),dev.codespire.contract.event.HarnessImageResult.Status.OK, - List.of(new dev.codespire.contract.event.HarnessImageResult.Model(slug,slug,"medium",List.of(levels),true,1)))); + List.of(new dev.codespire.contract.event.HarnessImageResult.Model(slug,slug,"medium",List.of(levels),true,1)), + "TEST-registry.invalid/agent@sha256:0000000000000000000000000000000000000000000000000000000000000000")); } @org.junit.jupiter.api.AfterEach void forgetTheCatalogue() throws Exception {executeWith("DELETE FROM harness_catalogue");} @Inject RunResultSaga saga; diff --git a/spire-run-worker/src/main/java/dev/codespire/runworker/HarnessImageWorker.java b/spire-run-worker/src/main/java/dev/codespire/runworker/HarnessImageWorker.java index ca0d4165..a1dc99d9 100644 --- a/spire-run-worker/src/main/java/dev/codespire/runworker/HarnessImageWorker.java +++ b/spire-run-worker/src/main/java/dev/codespire/runworker/HarnessImageWorker.java @@ -57,9 +57,9 @@ public CompletionStage onCommand(Message message) { } HarnessImageResult.Described describe(HarnessImageCommand.Describe command) { - Map labels; + dev.codespire.runtime.ImageDescription image; try { - labels = runtime.imageLabels(command.image()); + image = runtime.describeImage(command.image()); } catch (RuntimeException unreachable) { // Named, not rethrown: an image that cannot be pulled is an answer the screen has to give // ("this image is not available"), and a thrown exception here would only dead-letter it. @@ -68,9 +68,9 @@ HarnessImageResult.Described describe(HarnessImageCommand.Describe command) { return new HarnessImageResult.Described(command.requestId(), command.harness(), command.image(), HarnessImageResult.Status.IMAGE_UNAVAILABLE, List.of()); } - ModelCatalogueLabel.Read read = ModelCatalogueLabel.of(labels, mapper); + ModelCatalogueLabel.Read read = ModelCatalogueLabel.of(image.labels(), mapper); return new HarnessImageResult.Described(command.requestId(), command.harness(), command.image(), - read.status(), read.models()); + read.status(), read.models(), image.pinned()); } private static String keyOf(HarnessImageResult result) { diff --git a/spire-run-worker/src/test/java/dev/codespire/runworker/HarnessImageWorkerTest.java b/spire-run-worker/src/test/java/dev/codespire/runworker/HarnessImageWorkerTest.java index 410f0d10..54b42209 100644 --- a/spire-run-worker/src/test/java/dev/codespire/runworker/HarnessImageWorkerTest.java +++ b/spire-run-worker/src/test/java/dev/codespire/runworker/HarnessImageWorkerTest.java @@ -32,7 +32,8 @@ void anAnswerIsKeyedByItsHarnessSoOneHarnessKeepsOnePartition() { HarnessImageWorker worker = new HarnessImageWorker(); // Only the label read is reached; any other runtime call returns null and would fail loudly. worker.runtime = (RunRuntime) Proxy.newProxyInstance(RunRuntime.class.getClassLoader(), new Class[] { RunRuntime.class }, - (proxy, method, args) -> method.getName().equals("imageLabels") ? Map.of() : null); + (proxy, method, args) -> method.getName().equals("describeImage") + ? new dev.codespire.runtime.ImageDescription("TEST-registry.invalid/agent@sha256:0000000000000000000000000000000000000000000000000000000000000000", Map.of()) : null); worker.mapper = new ObjectMapper(); worker.ackSeconds = 5; worker.results = new Capturing(sent); @@ -41,6 +42,9 @@ void anAnswerIsKeyedByItsHarnessSoOneHarnessKeepsOnePartition() { assertEquals(1, sent.size()); assertEquals("codex", sent.getFirst().key()); + // The exact image read travels with the answer, so a run can use it instead of the tag. + assertEquals("TEST-registry.invalid/agent@sha256:0000000000000000000000000000000000000000000000000000000000000000", + ((HarnessImageResult.Described) sent.getFirst().value()).pinnedImage()); } /** One partition keeps order only if the worker does not answer two of its messages at once. */ diff --git a/spire-runtime-docker/src/main/java/dev/codespire/runtime/docker/DockerRunRuntime.java b/spire-runtime-docker/src/main/java/dev/codespire/runtime/docker/DockerRunRuntime.java index 2e1667e1..0a3de2c7 100644 --- a/spire-runtime-docker/src/main/java/dev/codespire/runtime/docker/DockerRunRuntime.java +++ b/spire-runtime-docker/src/main/java/dev/codespire/runtime/docker/DockerRunRuntime.java @@ -325,12 +325,33 @@ private static String digestOf(String runId) { } @Override - public java.util.Map imageLabels(String image) { + public dev.codespire.runtime.ImageDescription describeImage(String image) { // The SAME authenticated pull a run uses, so a private registry needs nothing new: an image a // run could start is an image whose labels can be read, and the reverse. ensureImage(image); - var config = client.inspectImageCmd(image).exec().getConfig(); - return config == null || config.getLabels() == null ? java.util.Map.of() : java.util.Map.copyOf(config.getLabels()); + var inspected = client.inspectImageCmd(image).exec(); + var config = inspected.getConfig(); + return new dev.codespire.runtime.ImageDescription(pinned(image, inspected.getRepoDigests(), inspected.getId()), + config == null || config.getLabels() == null ? java.util.Map.of() : config.getLabels()); + } + + /** + * The exact image behind a reference: the reference itself when it is already a digest, else the + * registry digest of the same repository, else the daemon's image id. + * + *

The image id is the honest last resort, not a fallback that pretends: a locally built image has + * no registry digest, and its id runs on this daemon and fails to pull anywhere else — which is the + * right answer on a worker that does not hold the image the models were read from. + */ + static String pinned(String image, java.util.List repoDigests, String imageId) { + if (image.contains("@sha256:")) return image; + int slash = image.lastIndexOf('/'), colon = image.lastIndexOf(':'); + // A colon before the last slash is a registry port ("localhost:5000/agent"), not a tag. + String repository = colon > slash ? image.substring(0, colon) : image; + for (String digest : repoDigests == null ? java.util.List.of() : repoDigests) { + if (digest.startsWith(repository + "@sha256:")) return digest; + } + return imageId; } /** diff --git a/spire-runtime-docker/src/test/java/dev/codespire/runtime/docker/DockerImageLabelsIT.java b/spire-runtime-docker/src/test/java/dev/codespire/runtime/docker/DockerImageLabelsIT.java index e72077a7..e049cb1a 100644 --- a/spire-runtime-docker/src/test/java/dev/codespire/runtime/docker/DockerImageLabelsIT.java +++ b/spire-runtime-docker/src/test/java/dev/codespire/runtime/docker/DockerImageLabelsIT.java @@ -33,7 +33,11 @@ void theReferenceImageDeclaresItsModelsInItsLabels() { assumeTrue(imagePresent(), IMAGE + " is not built on this machine"); int before = runtime.client().listContainersCmd().withShowAll(true).exec().size(); - Map labels = runtime.imageLabels(IMAGE); + var described = runtime.describeImage(IMAGE); + Map labels = described.labels(); + // A locally built image has no registry digest, so it is pinned by the daemon's own id. + var inspected = runtime.client().inspectImageCmd(IMAGE).exec(); + assertTrue(described.pinned().equals(inspected.getId()) || described.pinned().contains("@sha256:"), described.pinned()); assertEquals("codex", labels.get("dev.codespire.agent.harness")); String catalogue = labels.get(MODELS_LABEL); @@ -52,6 +56,6 @@ void theReferenceImageDeclaresItsModelsInItsLabels() { @Test void anImageThatCannotBeReachedIsAFailureNotAnEmptyAnswer() { assertThrows(RuntimeException.class, - () -> runtime.imageLabels("spire-test-no-such-image-" + System.nanoTime() + ":never")); + () -> runtime.describeImage("spire-test-no-such-image-" + System.nanoTime() + ":never")); } } diff --git a/spire-runtime-docker/src/test/java/dev/codespire/runtime/docker/DockerImagePinTest.java b/spire-runtime-docker/src/test/java/dev/codespire/runtime/docker/DockerImagePinTest.java new file mode 100644 index 00000000..ae8bdbfa --- /dev/null +++ b/spire-runtime-docker/src/test/java/dev/codespire/runtime/docker/DockerImagePinTest.java @@ -0,0 +1,44 @@ +package dev.codespire.runtime.docker; + +import org.junit.jupiter.api.Test; + +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +/** + * Which exact image a tag named when it was read (review of PR #167). Pure: the daemon's answers are + * given, so every branch is reached without one. + */ +class DockerImagePinTest { + + private static final String DIGEST = "sha256:" + "a".repeat(64); + private static final String OTHER = "sha256:" + "b".repeat(64); + private static final String ID = "sha256:" + "c".repeat(64); + + @Test + void aTagIsPinnedToTheRegistryDigestOfItsOwnRepository() { + assertEquals("ghcr.io/TEST-org/agent@" + DIGEST, DockerRunRuntime.pinned("ghcr.io/TEST-org/agent:1.2", + List.of("ghcr.io/TEST-org/other@" + OTHER, "ghcr.io/TEST-org/agent@" + DIGEST), ID)); + } + + /** "localhost:5000/agent" has a colon that is a port, not a tag. */ + @Test + void aRegistryPortIsNotMistakenForATag() { + assertEquals("localhost:5000/agent@" + DIGEST, + DockerRunRuntime.pinned("localhost:5000/agent", List.of("localhost:5000/agent@" + DIGEST), ID)); + } + + @Test + void aReferenceThatIsAlreadyADigestIsKept() { + assertEquals("ghcr.io/TEST-org/agent@" + DIGEST, + DockerRunRuntime.pinned("ghcr.io/TEST-org/agent@" + DIGEST, List.of(), ID)); + } + + /** A local build has no registry digest; its id runs here and fails to pull anywhere else. */ + @Test + void anImageWithNoDigestOfItsOwnRepositoryIsPinnedByItsId() { + assertEquals(ID, DockerRunRuntime.pinned("spire-agent-codex:latest", null, ID)); + assertEquals(ID, DockerRunRuntime.pinned("spire-agent-codex:latest", List.of("spire-agent-codex-old@" + OTHER), ID)); + } +} diff --git a/spire-runtime/src/main/java/dev/codespire/runtime/ImageDescription.java b/spire-runtime/src/main/java/dev/codespire/runtime/ImageDescription.java new file mode 100644 index 00000000..ee05abc4 --- /dev/null +++ b/spire-runtime/src/main/java/dev/codespire/runtime/ImageDescription.java @@ -0,0 +1,21 @@ +package dev.codespire.runtime; + +import java.util.Map; +import java.util.Objects; + +/** + * What one read of an image says about it (M3.5 part M). + * + * @param pinned the exact image that was read: its registry digest ({@code repo@sha256:…}) when it has + * one, otherwise the daemon's own image id. A tag can move and two workers can hold + * different images under one tag; this cannot, so a run started with it runs the image + * the labels came from — or fails, where the image is not there. + * @param labels the labels of THAT image, from the same read, so the two cannot describe different images + */ +public record ImageDescription(String pinned, Map labels) { + + public ImageDescription { + if (pinned == null || pinned.isBlank()) throw new IllegalArgumentException("A pinned image reference is required"); + labels = labels == null ? Map.of() : Map.copyOf(Objects.requireNonNull(labels)); + } +} diff --git a/spire-runtime/src/main/java/dev/codespire/runtime/RunRuntime.java b/spire-runtime/src/main/java/dev/codespire/runtime/RunRuntime.java index 4cf38de1..602fe218 100644 --- a/spire-runtime/src/main/java/dev/codespire/runtime/RunRuntime.java +++ b/spire-runtime/src/main/java/dev/codespire/runtime/RunRuntime.java @@ -70,7 +70,8 @@ public interface RunRuntime { Duration drainWindow(); /** - * The labels an image carries, fetching it first if this arm does not hold it (M3.5 part M). + * The labels an image carries, and the exact image they came from, fetching it first if this arm + * does not hold it (M3.5 part M). * *

On this port rather than a new one because the registry credential lives here and nowhere * else: it authenticates a pull and must reach nothing but a pull. Reading an image's labels is the @@ -81,9 +82,12 @@ public interface RunRuntime { * declares nothing", which is a real answer with a real screen; an arm that cannot look at all is a * different fact and must not be mistaken for it. * + *

Labels and pin come from ONE read. Two reads could straddle a rebuild under the same tag and + * describe the models of one image while pinning another. + * * @throws UnsupportedOperationException when this arm cannot read image metadata */ - default java.util.Map imageLabels(String image) { - throw new UnsupportedOperationException(type() + " cannot read image labels"); + default ImageDescription describeImage(String image) { + throw new UnsupportedOperationException(type() + " cannot read image metadata"); } } From cd01a2a9a278ea247e2471fd68d0f1a9c7f4c1b0 Mon Sep 17 00:00:00 2001 From: Artjoms Stukans Date: Wed, 23 Sep 2026 08:21:18 +0200 Subject: [PATCH 7/7] Record image answers in order, and match Docker Hub names Second review of PR #167 found two gaps in the image pin. - The orchestrator consumed image answers unordered, so an older answer queued during an outage could still finish last and overwrite the newer list and pin. It now records them one at a time, which keeps the per-harness order the worker's key gives. - Docker reports a Docker Hub image under its short name whatever spelling pulled it: docker.io/library/alpine:3.20 reports alpine@sha256:... The digest match now compares repositories in Docker's canonical form, so such an image is pinned by its portable digest instead of a local image id. --- .../factory/HarnessImageResults.java | 5 +++- .../factory/HarnessImageResultsOrderTest.java | 21 +++++++++++++++++ .../runtime/docker/DockerRunRuntime.java | 23 +++++++++++++++++-- .../runtime/docker/DockerImagePinTest.java | 21 +++++++++++++++++ 4 files changed, 67 insertions(+), 3 deletions(-) create mode 100644 spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessImageResultsOrderTest.java diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessImageResults.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessImageResults.java index f91927b0..6585f042 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessImageResults.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessImageResults.java @@ -18,8 +18,11 @@ public class HarnessImageResults { @Inject HarnessCatalogues catalogues; + // In order: the cache keeps the LAST answer per harness, and the worker keys answers by harness so + // one harness's answers share a partition. Handled unordered, an older answer queued in an outage + // could still finish last and overwrite the newer list and pin (second review of PR #167). @Incoming("harness-image-results-in") - @Blocking(ordered = false) + @Blocking public CompletionStage onResult(Message message) { if (message.getPayload() instanceof HarnessImageResult.Described described) { try { diff --git a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessImageResultsOrderTest.java b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessImageResultsOrderTest.java new file mode 100644 index 00000000..f121d399 --- /dev/null +++ b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessImageResultsOrderTest.java @@ -0,0 +1,21 @@ +package dev.codespire.orchestrator.factory; + +import io.smallrye.reactive.messaging.annotations.Blocking; +import org.eclipse.microprofile.reactive.messaging.Message; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * The cache keeps the LAST answer per harness, so the answers must be recorded in the order they arrive. + * The worker keys them by harness; that order survives only if they are also recorded one at a time + * (second review of PR #167). + */ +class HarnessImageResultsOrderTest { + + @Test + void theAnswersAreRecordedOneAtATime() throws NoSuchMethodException { + Blocking blocking = HarnessImageResults.class.getMethod("onResult", Message.class).getAnnotation(Blocking.class); + assertTrue(blocking.ordered()); + } +} diff --git a/spire-runtime-docker/src/main/java/dev/codespire/runtime/docker/DockerRunRuntime.java b/spire-runtime-docker/src/main/java/dev/codespire/runtime/docker/DockerRunRuntime.java index 0a3de2c7..1675b7e0 100644 --- a/spire-runtime-docker/src/main/java/dev/codespire/runtime/docker/DockerRunRuntime.java +++ b/spire-runtime-docker/src/main/java/dev/codespire/runtime/docker/DockerRunRuntime.java @@ -347,13 +347,32 @@ static String pinned(String image, java.util.List repoDigests, String im if (image.contains("@sha256:")) return image; int slash = image.lastIndexOf('/'), colon = image.lastIndexOf(':'); // A colon before the last slash is a registry port ("localhost:5000/agent"), not a tag. - String repository = colon > slash ? image.substring(0, colon) : image; + String repository = canonical(colon > slash ? image.substring(0, colon) : image); for (String digest : repoDigests == null ? java.util.List.of() : repoDigests) { - if (digest.startsWith(repository + "@sha256:")) return digest; + int at = digest.indexOf("@sha256:"); + // Compared in Docker's canonical spelling: "docker.io/library/alpine" is reported as + // "alpine@sha256:…", and missing that pinned a pullable image by an id nothing else holds. + if (at > 0 && canonical(digest.substring(0, at)).equals(repository)) return digest; } return imageId; } + /** + * Docker's own normalisation of a repository name: a first part with no dot, no colon and not + * "localhost" is not a registry, so the name is on Docker Hub; a Docker Hub name with no namespace is + * in "library"; "index.docker.io" is "docker.io". + */ + static String canonical(String repository) { + int slash = repository.indexOf('/'); + String first = slash < 0 ? "" : repository.substring(0, slash); + boolean registry = slash > 0 && (first.contains(".") || first.contains(":") || first.equals("localhost")); + String host = registry ? first : "docker.io"; + String path = registry ? repository.substring(slash + 1) : repository; + if (host.equals("index.docker.io")) host = "docker.io"; + if (host.equals("docker.io") && !path.contains("/")) path = "library/" + path; + return host + "/" + path; + } + /** * Pulls the image when the daemon does not hold it. An operator's agent image lives in a * registry and a digest-pinned reference (FR-F13) is the normal case, not a local tag — the diff --git a/spire-runtime-docker/src/test/java/dev/codespire/runtime/docker/DockerImagePinTest.java b/spire-runtime-docker/src/test/java/dev/codespire/runtime/docker/DockerImagePinTest.java index ae8bdbfa..925c46cb 100644 --- a/spire-runtime-docker/src/test/java/dev/codespire/runtime/docker/DockerImagePinTest.java +++ b/spire-runtime-docker/src/test/java/dev/codespire/runtime/docker/DockerImagePinTest.java @@ -29,6 +29,27 @@ void aRegistryPortIsNotMistakenForATag() { DockerRunRuntime.pinned("localhost:5000/agent", List.of("localhost:5000/agent@" + DIGEST), ID)); } + /** + * Docker reports a Docker Hub image under its short name whatever spelling pulled it. Measured + * read-only on 2026-09-23: "docker.io/library/alpine:3.20" reported "alpine@sha256:…" (second review + * of PR #167). Missing the match pinned a pullable image by an id no other worker holds. + */ + @Test + void anyDockerHubSpellingFindsItsDigest() { + List reported = List.of("alpine@" + DIGEST); + assertEquals("alpine@" + DIGEST, DockerRunRuntime.pinned("docker.io/library/alpine:3.20", reported, ID)); + assertEquals("alpine@" + DIGEST, DockerRunRuntime.pinned("index.docker.io/library/alpine:3.20", reported, ID)); + assertEquals("alpine@" + DIGEST, DockerRunRuntime.pinned("library/alpine", reported, ID)); + assertEquals("TEST-org/agent@" + DIGEST, + DockerRunRuntime.pinned("docker.io/TEST-org/agent:1", List.of("TEST-org/agent@" + DIGEST), ID)); + } + + /** A registry that merely shares a path is a different repository. */ + @Test + void theSamePathOnAnotherRegistryIsNotTheSameRepository() { + assertEquals(ID, DockerRunRuntime.pinned("ghcr.io/library/alpine:3.20", List.of("alpine@" + DIGEST), ID)); + } + @Test void aReferenceThatIsAlreadyADigestIsKept() { assertEquals("ghcr.io/TEST-org/agent@" + DIGEST,