From 86070620c719a4f0cabe7b07b599d331802ac35e Mon Sep 17 00:00:00 2001 From: Artjoms Stukans Date: Fri, 25 Sep 2026 23:06:49 +0200 Subject: [PATCH 1/8] Let a build pay with a Codex subscription seat MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A repository's build setup now says how a build pays: an API key, as before, or a signed-in Codex subscription. The choice is part of what an approval binds (binding version 4), so an approval for one is not an approval for the other. - A subscription run leases one seat, fenced by its run id. The run result releases it; a lease that is never released expires at wall clock + 10 minutes. The API-key selector never reaches a seat. - The agent gets the stored sign-in with every refresh token emptied. Codex accepts such a file and runs on the access token; the refresh token never leaves the orchestrator. The adapter writes the file as $HOME/.codex/auth.json (umask 077) and unsets the variable. - Charging follows how the run paid, not the model: a subscription run records UNMETERED lines with its real token counts. - A subscription needs no model prices, but saving one needs a seat. - Failure details scrub each token in the file, not only the file. - Run detail names a seat as a subscription, never as an API key billed per token; Settings shows a leased seat as "In use". Part of M3.5 part F. The design records the measurement behind the emptied refresh token (§5.3) and where the lease differs from §5.5. --- docs/UNVERIFIED.md | 3 +- ...actory-m35-one-ticket-to-a-build-design.md | 36 ++++- .../contract/command/RunCommand.java | 37 +++++- .../dev/codespire/contract/work/PayWith.java | 29 +++++ .../contract/work/WorkPreparation.java | 27 +++- .../command/ExecuteRunBranchModeTest.java | 9 ++ .../WorkPreparationBindingVersionTest.java | 28 ++++ .../src/test/resources/contract-schema.txt | 2 +- .../codespire/harness/codex/CodexAdapter.java | 36 ++++- .../harness/codex/CodexAdapterTest.java | 25 ++++ .../codespire/harness/HarnessInvocation.java | 8 ++ .../orchestrator/factory/BuildDefaults.java | 57 +++++--- .../factory/FactoryRunProjection.java | 9 +- .../factory/HarnessCredentialPool.java | 107 +++++++++++++-- .../orchestrator/factory/RunCharges.java | 16 ++- .../orchestrator/factory/RunResultSaga.java | 8 ++ .../orchestrator/factory/SignInFiles.java | 51 ++++++++ .../orchestrator/factory/WorkRunAssembly.java | 44 +++++-- .../orchestrator/llm/LlmModelPricer.java | 15 +++ .../work/WorkPreparationSweep.java | 9 +- .../work/WorkRunAssemblyRefusals.java | 3 +- .../migration/V83__pay_with_subscription.sql | 14 ++ .../factory/BuildDefaultsTest.java | 58 +++++++++ .../factory/FactoryRunProjectionTest.java | 21 +++ .../factory/HarnessSubscriptionLeaseTest.java | 123 ++++++++++++++++++ .../orchestrator/factory/RunChargesTest.java | 43 ++++++ .../factory/RunResultSagaTest.java | 26 ++++ .../orchestrator/factory/SignInFilesTest.java | 43 ++++++ .../work/WorkPreparationSweepTest.java | 29 ++++- .../work/WorkRunDispatchTest.java | 30 +++++ .../dev/codespire/runworker/Credentials.java | 39 ++++++ .../dev/codespire/runworker/RunFailures.java | 8 +- .../codespire/runworker/RunUnitBuilder.java | 3 +- .../codespire/runworker/RunLauncherTest.java | 25 ++++ .../runworker/RunUnitBuilderTest.java | 9 ++ .../runworker/SignInSecretsTest.java | 40 ++++++ spire-ui/src/api.ts | 9 +- spire-ui/src/components/RunDefinitionCard.tsx | 13 +- spire-ui/src/components/RunDetail.test.tsx | 7 + .../SettingsHarnessCredentials.test.tsx | 12 ++ .../components/SettingsHarnessCredentials.tsx | 15 +-- .../repositories/factory/BuildModelFields.tsx | 10 +- .../repositories/factory/BuildStep.tsx | 19 ++- .../factory/RepositoryFactory.test.tsx | 24 +++- .../repositories/factory/buildDefaultsApi.ts | 6 +- .../src/components/work-items/workReasons.ts | 4 + 46 files changed, 1089 insertions(+), 100 deletions(-) create mode 100644 spire-contract/src/main/java/dev/codespire/contract/work/PayWith.java create mode 100644 spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/SignInFiles.java create mode 100644 spire-orchestrator/src/main/resources/db/migration/V83__pay_with_subscription.sql create mode 100644 spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessSubscriptionLeaseTest.java create mode 100644 spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/SignInFilesTest.java create mode 100644 spire-run-worker/src/test/java/dev/codespire/runworker/SignInSecretsTest.java diff --git a/docs/UNVERIFIED.md b/docs/UNVERIFIED.md index 5d858da29..31f800f07 100644 --- a/docs/UNVERIFIED.md +++ b/docs/UNVERIFIED.md @@ -471,8 +471,9 @@ 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 | -| **Codex CLI 0.156.1 in the agent image** (2026-09-23) | none yet | Raised from 0.146.0 so the model list includes the gpt-6 models. Re-checked on 0.156.1: every flag the adapter passes, the API-key login, and the shape of the file it writes (`auth_mode=apikey`). NOT re-checked: the `--json` event stream the usage parser reads, and the device sign-in output. Both need a paid run or a real sign-in, and the first of each proves or breaks them | +| **Codex CLI 0.156.1 in the agent image** (2026-09-23) | none yet | Raised from 0.146.0 so the model list includes the gpt-6 models. Re-checked on 0.156.1: every flag the adapter passes, the API-key login, and the shape of the file it writes (`auth_mode=apikey`). The device sign-in output was then proved by a real sign-in (2026-09-25), and one live `codex exec` on a subscription produced the same `--json` event types and the same five usage buckets. Still NOT re-checked: a multi-turn run with tool calls, which exercises the rest of the parser | | **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 | +| **A build paid by a Codex subscription** (M3.5 part F, 2026-09-25) | none yet | Tests prove the lease, the emptied refresh token, the unmetered charge lines and the file the adapter writes. One live `codex exec` accepted a file with its refresh token emptied, three hours after sign-in with the id token expired. No item build has run on a seat yet. NOT observed: a run that outlives the access token (10 days — the seat then needs a new sign-in, nothing refreshes it), two builds contending for one seat on a real worker, and whether the vendor's CLI ever tries to refresh mid-run with an empty refresh token | | **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 6f79786dd..21b09a01a 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 @@ -216,6 +216,19 @@ EXECUTION-LAYER §3.3 with their date and the CLI version: 5. What a usage-limit refusal looks like on the NDJSON stream and in the exit code, so the pool can tell `rate_limited` from `rejected`, and which usage buckets a subscription run reports. +**Measured on 2026-09-25** from the first real sign-in made through 5.2's screen, with +`@openai/codex@0.156.1`. Field names, types and token lifetimes only; no value was printed: + +| Asked | Answer | +|---|---| +| 1. What a ChatGPT-mode `auth.json` holds | `auth_mode` = `chatgpt`; `OPENAI_API_KEY` null; `tokens.id_token` (JWT, **1 hour**); `tokens.access_token` (JWT, **10 days**); `tokens.refresh_token` (opaque); `tokens.account_id`; `last_refresh`. | +| Does `codex login --with-access-token` take that access token? | **No.** It expects an agent-identity JWT and refuses: "agent identity JWT payload is not valid JSON". 5.6's first plan does not work. | +| Does a file with an EMPTY refresh token work? | **Yes.** A missing `refresh_token` field is refused as malformed; an empty one is accepted ("Logged in using ChatGPT"). A live `codex exec` three hours after sign-in — the id token already expired — answered and exited 0. | +| 2. Does a run rewrite the file? | **Not that run:** the file was byte-identical afterwards. A run near the access token's expiry is not yet measured. | +| Usage on a subscription run | The same five buckets as an API-key run: input, cached input, cache write, output, reasoning. | + +Questions 3–5 (refresh-token rotation, a quota-free renewal command, the usage-limit refusal) remain open. + **Nothing is built on an unmeasured answer.** Where one is missing, the design takes the option that is safe when the guess is wrong: no automatic refresh, and a sign-in that is used until the vendor refuses it. @@ -258,12 +271,23 @@ takes no sign-in and never replays the agent (`WorkRunWorker.java:91`). Cases th duplicate command delivery, worker death before release, a failed stop, a late release from an old lease, dispatch failure before the container exists, and two uploads of the same sign-in. +- **Built 2026-09-25, and where it differs.** The fence is the **run id**, not a separate + `lease_version`: a run id names one attempt, so a late release from an older run cannot match a newer + run's lease. `RunResultSaga` releases on every run result except `RunStarted` — `RunWorkReady` + included, so a held build frees its seat when the agent stops, not when the item ends. A lease whose + release never arrives expires at wall clock + 10 minutes. **Known gap:** the watchdog can report a + failure before its stop completes (above), and that report releases the seat, so a next run can + briefly share it with a dying agent. Tested: two runs contending, the fence, expiry, and a release on + each ending result. Not tested: duplicate delivery and a failed stop on a real worker. + ### 5.6 Injection and refresh — the agent never hands a credential back -- The worker passes the credential kind beside `HarnessInvocation.CREDENTIAL`. `CodexAdapter` pipes an - **access token** into `codex login --with-access-token` on stdin, exactly as it pipes an API key into - `--with-api-key` today (F0 measured both). Then it starts `codex exec`. No credential file is written - into the agent container, and nothing reaches argv or the environment. +- **Revised 2026-09-25 (5.3):** `--with-access-token` refuses a ChatGPT access token, so the first plan — + piping the access token into it — cannot work. Instead the worker hands the adapter the stored sign-in + file **with its refresh token emptied**, and `CodexAdapter` writes that file as the agent's + `auth.json` before `codex exec`. It arrives the same way an API key does today: in the environment + under the neutral credential name, never on argv. The access token and id token go in; the refresh token + does not. - **The refresh token never leaves the orchestrator.** An access token expires on its own; a refresh token does not, and an agent that reads one holds the sign-in until a person revokes it. Handing the agent the short-lived half is therefore not a detail of the plumbing — it is the whole difference @@ -315,6 +339,10 @@ translation, a retry-time field on the refusal, durable handling of that result, credential **version**, and the matching change to the arch guard `spire-arch/src/test/java/dev/codespire/arch/CredentialRefusalHasNoProducerTest.java`. +**Not built in the first part F change (2026-09-25).** A seat whose quota runs out is reported the way +any provider failure is today (collapsed as described above); nothing marks the seat resting. The first live runs show the operator the real quota +message, which is what this path must then classify. + ### 5.9 Risks, stated plainly - **The sign-in file is a person's account credential,** placed in a container that runs ticket text. A 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 3de03c590..fc33997d5 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 @@ -81,7 +81,7 @@ record ExecuteRun(String runId, RepoRef repo, String remoteUri, List protectedPaths, long maxWallClockSeconds, String scmCredential, String harnessCredential, boolean existingBranch, String protectedBranch, - String reasoningEffort) implements RunCommand { + String reasoningEffort, boolean harnessSignIn) 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 @@ -93,7 +93,7 @@ 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, "", null); + harnessCredential, false, "", null, false); } /** @@ -108,7 +108,22 @@ public ExecuteRun(String runId, RepoRef repo, String remoteUri, boolean existingBranch, String protectedBranch) { this(runId, repo, remoteUri, baseBranch, baseCommit, branch, prompt, harness, model, agentImage, protectedPaths, maxWallClockSeconds, scmCredential, - harnessCredential, existingBranch, protectedBranch, null); + harnessCredential, existingBranch, protectedBranch, null, false); + } + + /** + * Every caller written before subscriptions existed passes an API key (M3.5 part F). A run already + * on the bus decodes with false, which is what every such run carried. + */ + 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, String reasoningEffort) { + this(runId, repo, remoteUri, baseBranch, baseCommit, branch, prompt, harness, model, + agentImage, protectedPaths, maxWallClockSeconds, scmCredential, + harnessCredential, existingBranch, protectedBranch, reasoningEffort, false); } public ExecuteRun { @@ -170,7 +185,7 @@ 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, reasoningEffort); + harnessCredential, true, destination, reasoningEffort, harnessSignIn); } /** @@ -181,7 +196,18 @@ public ExecuteRun onExistingBranch(String destination) { 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); + harnessCredential, existingBranch, protectedBranch, level, harnessSignIn); + } + + /** + * The same run, where {@code harnessCredential} is a sealed sign-in file rather than an API key + * (M3.5 part F). The worker needs to know which, because the two are handed to the harness + * differently — and guessing from the bytes is how a key ends up written as a file. + */ + public ExecuteRun paidBySignIn() { + return new ExecuteRun(runId, repo, remoteUri, baseBranch, baseCommit, branch, prompt, + harness, model, agentImage, protectedPaths, maxWallClockSeconds, scmCredential, + harnessCredential, existingBranch, protectedBranch, reasoningEffort, true); } @Override @@ -198,6 +224,7 @@ public String toString() { + ", protectedPaths=" + protectedPaths + ", maxWallClockSeconds=" + maxWallClockSeconds + ", existingBranch=" + existingBranch + + ", harnessSignIn=" + harnessSignIn + ", protectedBranch=" + protectedBranch + ", reasoningEffort=" + reasoningEffort + ", promptChars=" + prompt.length() diff --git a/spire-contract/src/main/java/dev/codespire/contract/work/PayWith.java b/spire-contract/src/main/java/dev/codespire/contract/work/PayWith.java new file mode 100644 index 000000000..a15fdab1a --- /dev/null +++ b/spire-contract/src/main/java/dev/codespire/contract/work/PayWith.java @@ -0,0 +1,29 @@ +package dev.codespire.contract.work; + +/** + * How a build pays for its model calls (M3.5 part F). + * + *

Two values, as names rather than a boolean: a third way to pay is easy to imagine, and a flag called + * "subscription" would have to be rewritten to admit one. Every value written before this existed is an + * API key, because until then that was the only way a run could pay. + */ +public final class PayWith { + + /** Per token, with a key from the credential pool. */ + public static final String API_KEY = "API_KEY"; + + /** A signed-in seat: no per-token price, the real token counts still recorded. */ + public static final String SUBSCRIPTION = "SUBSCRIPTION"; + + private PayWith() { + } + + /** The value, with blank read as {@link #API_KEY}; anything else is refused rather than guessed. */ + public static String normalise(String value) { + if (value == null || value.isBlank()) return API_KEY; + String stripped = value.strip(); + if (!stripped.equals(API_KEY) && !stripped.equals(SUBSCRIPTION)) + throw new IllegalArgumentException("A build pays with " + API_KEY + " or " + SUBSCRIPTION + ", was: " + value); + 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 e60efbcd7..70dd7bb6c 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 @@ -11,7 +11,7 @@ /** 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 effort) { + String effort, String payWith) { /** Where an artifact's approved bytes live. Absent in stored history means {@link Origin#TRACKER}. */ public enum Origin { @@ -68,15 +68,28 @@ public record Artifact(WorkIssueLocation location, String sha256, Origin origin, */ public static final int EFFORT_BINDING = 3; + /** + * Adds how the build pays (M3.5 part F). Part of the binding because it decides what a build costs: + * a plan approved to run on a subscription must not start billing an API key per token because the + * build setup changed after the decision. Its own version for the reason the others have one. + */ + public static final int PAY_WITH_BINDING = 4; + + /** Every preparation written before payment modes existed pays with an API key. */ + public WorkPreparation(Artifact specification, Artifact plan, String baseBranch, String baseCommit, + String harness, String model, String registeredBy, int bindingVersion, String effort) { + this(specification, plan, baseBranch, baseCommit, harness, model, registeredBy, bindingVersion, effort, null); + } + 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, null); + this(specification, plan, baseBranch, baseCommit, harness, model, registeredBy, TRACKER_BINDING, null, 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); + this(specification, plan, baseBranch, baseCommit, harness, model, registeredBy, bindingVersion, null, null); } public WorkPreparation { @@ -90,13 +103,18 @@ 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 && bindingVersion != EFFORT_BINDING) + if (bindingVersion < TRACKER_BINDING || bindingVersion > PAY_WITH_BINDING) throw new IllegalArgumentException("Unknown preparation binding version " + bindingVersion); 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); + payWith = PayWith.normalise(payWith); + // A subscription under a version that does not hash it would reach the build without being part + // of what was approved. + if (!payWith.equals(PayWith.API_KEY) && bindingVersion < PAY_WITH_BINDING) + throw new IllegalArgumentException("Paying with " + payWith + " needs binding version " + PAY_WITH_BINDING); if (bindingVersion == TRACKER_BINDING && (specification.origin() != Origin.TRACKER || plan.origin() != Origin.TRACKER)) throw new IllegalArgumentException("A version 1 binding describes tracker artifacts only"); @@ -123,6 +141,7 @@ public String binding() { String level = effort == null ? "" : effort; value.append(level.length()).append(':').append(level); } + if (bindingVersion >= PAY_WITH_BINDING) value.append(payWith.length()).append(':').append(payWith); 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 3ff27797d..d8c443a03 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 @@ -47,6 +47,15 @@ void theThinkingLevelSurvivesEveryRebuildAndTheOthersSurviveIt() { assertThrows(IllegalArgumentException.class, () -> run().atEffort("high'; rm")); } + /** A sign-in is written as a file and a key is piped into a login; losing the flag swaps the two. */ + @Test + void aRunPaidBySignInSaysSoThroughEveryRebuild() { + RunCommand.ExecuteRun run = run().paidBySignIn().atEffort("high").onExistingBranch("develop"); + + assertTrue(run.harnessSignIn()); + assertFalse(run().harnessSignIn(), "every run before part F paid with a key"); + } + @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 2871bb1ef..a6d9b341c 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 @@ -137,6 +137,34 @@ void aLevelThatIsNotAPlainWordIsRefused() { assertEquals(null, atLevel(" ").effort(), "blank is the model's own default, not a level called blank"); } + private static WorkPreparation paying(String payWith) { + 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.PAY_WITH_BINDING, null, payWith); + } + + /** How a build pays decides what it costs, so an approval for one is not an approval for the other. */ + @Test + void howABuildPaysIsPartOfWhatIsApproved() { + assertNotEquals(paying(PayWith.API_KEY).binding(), paying(PayWith.SUBSCRIPTION).binding()); + assertEquals(PayWith.API_KEY, paying(null).payWith(), "no choice is the API key every build used before"); + assertNotEquals(atLevel(null).binding(), paying(PayWith.API_KEY).binding(), "version 4 is not version 3"); + } + + /** Versions 1 to 3 do not hash the payment, so they may not carry a subscription to the build. */ + @Test + void aSubscriptionUnderAVersionThatDoesNotHashItIsRefused() { + 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.EFFORT_BINDING, null, PayWith.SUBSCRIPTION)); + assertThrows(IllegalArgumentException.class, () -> paying("TEST-free")); + } + @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 5cc33bca1..aa50994c1 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, reasoningEffort: 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, harnessSignIn: boolean) 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 d5f230ae4..3e5a0943b 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 @@ -52,6 +52,12 @@ public final class CodexAdapter implements HarnessAdapter { /** What the Codex process reads its key from. Vendor knowledge, and this arm's alone. */ private static final String API_KEY_VARIABLE = "OPENAI_API_KEY"; + /** + * Where the sign-in file waits for the login step to write it (M3.5 part F). This arm's own name: the + * script below reads it once, writes the file Codex reads, and unsets it before Codex starts. + */ + private static final String SIGN_IN_VARIABLE = "CODEX_SIGN_IN_FILE"; + /** * The exit code the login step uses when it cannot authenticate, so a run that never reached * the model is not reported as one the model failed to answer. @@ -122,8 +128,7 @@ public List command(HarnessInvocation invocation) { // nothing lingers to interpret anything, and the two interpolations are quoted and refused // if they could close the quote. The prompt still arrives on the harness's stdin, which the // entrypoint redirects into the whole command; the login reads its own stdin from the pipe. - String script = "printenv " + API_KEY_VARIABLE + " | codex login --with-api-key >/dev/null" - + " || exit " + LOGIN_FAILED + "; " + String script = login(invocation) + "exec codex exec --json --sandbox danger-full-access --skip-git-repo-check" + " --model " + quoted(invocation.model(), "model") + effort(invocation.reasoningEffort()) @@ -132,6 +137,27 @@ public List command(HarnessInvocation invocation) { return List.of("sh", "-c", script); } + /** + * How this run signs Codex in: a key piped into {@code --with-api-key}, or a sign-in file written + * where Codex reads one. + * + *

The file is written because no login flag takes it. {@code --with-access-token} was the plan and + * refuses a ChatGPT access token (measured 2026-09-25, codex-cli 0.156.1: design §5.3); the file, + * with its refresh token emptied by the orchestrator, is accepted and runs. Written owner-only + * ({@code umask 077}), and the variable that carried it is unset before Codex starts, so the process + * that runs ticket text does not also inherit it. The agent can still read the file, as it can read + * a key today — what it cannot hold is a refresh token, because none was ever sent. + */ + private static String login(HarnessInvocation invocation) { + if (invocation.credentials().containsKey(HarnessInvocation.SIGN_IN)) { + return "mkdir -p \"$HOME/.codex\" && (umask 077 && printenv " + SIGN_IN_VARIABLE + + " > \"$HOME/.codex/auth.json\") || exit " + LOGIN_FAILED + "; " + + "unset " + SIGN_IN_VARIABLE + "; "; + } + return "printenv " + API_KEY_VARIABLE + " | codex login --with-api-key >/dev/null" + + " || exit " + LOGIN_FAILED + "; "; + } + /** * The thinking level as a config override, or nothing, so the model's own default applies. * @@ -177,6 +203,10 @@ public Map environment(HarnessInvocation invocation) { if (apiKey != null) { credentials.put(API_KEY_VARIABLE, apiKey); } + String signIn = credentials.remove(HarnessInvocation.SIGN_IN); + if (signIn != null) { + credentials.put(SIGN_IN_VARIABLE, signIn); + } return EnvironmentPolicy.merge(credentials, OWN_SETTINGS); } @@ -341,7 +371,7 @@ public TerminalOutcome classify(int exitCode, RunEventSummary seen) { // never started, so nothing about the model or the work item is implicated. The first // live dispatch spent its diagnosis on the model because this case had no name. return TerminalOutcome.failure(FailureCause.HARNESS_EXIT_NONZERO, - "codex login did not accept the API key, so the harness never ran"); + "codex login failed with the run's credential (a key or a sign-in file), so the harness never ran"); } if (!seen.sawAnyOutput()) { // Distinct and nameable: the model spent its whole budget and said nothing. Reported as 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 85698291f..1f28cbfe9 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 @@ -55,6 +55,31 @@ void theChosenThinkingLevelReachesCodexAndNoLevelMeansNoOverride() { assertFalse(script(adapter.command(invocation())).contains("model_reasoning_effort")); } + /** + * A sign-in is WRITTEN where Codex reads one, owner-only, and the variable that carried it is unset + * before Codex starts; no key is piped into a login (M3.5 part F — design §5.3 measured that no login + * flag takes a ChatGPT sign-in). + */ + @Test + void aSignInIsWrittenAsTheFileCodexReadsAndNotPipedIntoALogin() { + HarnessInvocation signedIn = new HarnessInvocation("run_abc", "fix the bug", "/workspace", "gpt-5.6", + Map.of(HarnessInvocation.SIGN_IN, "{\"auth_mode\":\"TEST\"}"), Duration.ofMinutes(30)); + + String script = script(adapter.command(signedIn)); + Map env = adapter.environment(signedIn); + + assertTrue(script.contains("umask 077 && printenv CODEX_SIGN_IN_FILE > \"$HOME/.codex/auth.json\""), script); + assertTrue(script.contains("unset CODEX_SIGN_IN_FILE; exec codex exec"), script); + assertFalse(script.contains("--with-api-key"), script); + assertEquals("{\"auth_mode\":\"TEST\"}", env.get("CODEX_SIGN_IN_FILE")); + assertFalse(env.containsKey("OPENAI_API_KEY")); + } + + @Test + void anApiKeyIsStillPipedIntoTheLogin() { + assertTrue(script(adapter.command(invocation())).startsWith("printenv OPENAI_API_KEY | codex login --with-api-key")); + } + @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 9138dd339..c87560e8a 100644 --- a/spire-harness/src/main/java/dev/codespire/harness/HarnessInvocation.java +++ b/spire-harness/src/main/java/dev/codespire/harness/HarnessInvocation.java @@ -31,6 +31,14 @@ public HarnessInvocation(String runId, String prompt, String workspacePath, */ public static final String CREDENTIAL = "HARNESS_CREDENTIAL"; + /** + * The key under which the worker supplies a SIGN-IN FILE instead of an API key (M3.5 part F): the + * vendor CLI's own sign-in record, with its refresh token already emptied by the orchestrator. A + * separate key rather than a flag beside {@link #CREDENTIAL}, so an arm that knows only API keys + * never mistakes a file for a key and pipes it into a login that would echo it. + */ + public static final String SIGN_IN = "HARNESS_SIGN_IN"; + public HarnessInvocation { Objects.requireNonNull(runId, "runId"); Objects.requireNonNull(prompt, "prompt"); 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 e13021d16..f6e68ed00 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 @@ -24,6 +24,7 @@ public class BuildDefaults { @Inject LlmModelRegistry models; @Inject dev.codespire.orchestrator.llm.LlmModelPricer pricer; @Inject HarnessCatalogues catalogues; + @Inject HarnessCredentialPool pool; /** * @param revision 0 when the repository has none yet, with null coordinates — "not set" is a state @@ -31,18 +32,28 @@ public class BuildDefaults { */ /** * @param effort the thinking level, or null for the model's own default — a real choice, not a gap + * @param payWith {@code API_KEY} or {@code SUBSCRIPTION} (M3.5 part F) */ 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, null); } + String payWith, String updatedBy, Instant updatedAt) { + public static Defaults none() { return new Defaults(0, null, null, null, null, null, null, null); } public boolean set() { return revision > 0; } } - /** @param effort null for the model's own default */ - public record Input(long expectedRevision, String baseBranch, String harness, String model, String effort) { + /** + * @param effort null for the model's own default + * @param payWith null or blank for an API key, which is what every setup paid with before part F + */ + public record Input(long expectedRevision, String baseBranch, String harness, String model, String effort, + String payWith) { /** 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); + this(expectedRevision, baseBranch, harness, model, null, null); + } + + /** Every caller written before payment modes existed pays with an API key. */ + public Input(long expectedRevision, String baseBranch, String harness, String model, String effort) { + this(expectedRevision, baseBranch, harness, model, effort, null); } } @@ -63,14 +74,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,effort,updated_by,updated_at" + String sql = "SELECT revision,base_branch,harness,model,effort,pay_with,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.getString(6), rs.getTimestamp(7).toInstant()); + rs.getString(5), rs.getString(6), rs.getString(7), rs.getTimestamp(8).toInstant()); } } } @@ -97,14 +108,23 @@ public Defaults save(UUID repository, Input input, String actor) { 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. - if (models.list().stream().noneMatch(known -> known.enabled() && known.name().equals(model))) - throw new Refused("model_unknown"); - var unpriced = pricer.unpricedTypes(model, harness); - if (!unpriced.isEmpty()) throw new Refused("model_pricing_incomplete:" - + unpriced.stream().map(Enum::name).collect(java.util.stream.Collectors.joining(","))); + String payWith; + try { payWith = dev.codespire.contract.work.PayWith.normalise(input.payWith()); } + catch (IllegalArgumentException unknown) { throw new Refused("pay_with_invalid"); } + if (payWith.equals(dev.codespire.contract.work.PayWith.SUBSCRIPTION)) { + // A subscription is not priced per token, so the model needs no rates and no catalogue entry + // — the harness's own list above is what says it can run. What it does need is a seat. + if (!pool.hasSubscription(harness)) throw new Refused("subscription_not_signed_in"); + } else { + // 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. + if (models.list().stream().noneMatch(known -> known.enabled() && known.name().equals(model))) + throw new Refused("model_unknown"); + var unpriced = pricer.unpricedTypes(model, harness); + if (!unpriced.isEmpty()) throw new Refused("model_pricing_incomplete:" + + unpriced.stream().map(Enum::name).collect(java.util.stream.Collectors.joining(","))); + } try (Connection c = dataSource.getConnection()) { // Same lock order as the policy save: the repository row first, so a save cannot interleave // with a repository or account edit that decides whether these coordinates can run at all. @@ -115,14 +135,15 @@ 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,effort,updated_by) - VALUES (?,?,?,?,?,?) + INSERT INTO repository_build_defaults(repository_id,base_branch,harness,model,effort,updated_by,pay_with) + VALUES (?,?,?,?,?,?,?) ON CONFLICT (repository_id) DO UPDATE SET base_branch=excluded.base_branch,harness=excluded.harness, model=excluded.model,effort=excluded.effort,updated_by=excluded.updated_by,updated_at=now(), + pay_with=excluded.pay_with, 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, effort); ps.setString(6, actor); + ps.setString(4, model); ps.setString(5, effort); ps.setString(6, actor); ps.setString(7, payWith); ps.executeUpdate(); } // Saving a setup is the repair for "this repository has no build setup", so the items that diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/FactoryRunProjection.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/FactoryRunProjection.java index f1ba7f732..4bf499029 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/FactoryRunProjection.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/FactoryRunProjection.java @@ -174,7 +174,8 @@ public record RunView(String runId, String status, String pushedRef, List blocked, String failureCause, String failureDetail, String unitId, String prUrl, String prError, Instant agentStartedAt, - String kind, String harness, String model, String credentialLabel, String credentialType, String providerType, + String kind, String harness, String model, String credentialLabel, String credentialType, + String credentialAuthMode, String providerType, String workspace, String slug, String subject, int attempt, String baseBranch, String baseCommit, String branch, String pushedAs, String reviewId, String findingRef, String taskSummary, @@ -1015,14 +1016,14 @@ public boolean exists(String runId) { public Optional find(String runId) { // The credential names WHO paid for this run and how it is billed. Its secret is never - // selected here; the label and type are the operator metadata that name the key. + // selected here; the label, type and auth mode are the operator metadata that name the key. String sql = """ SELECT r.status, r.pushed_ref, r.blocked_changes, r.failure_cause, r.failure_detail, r.unit_id, r.pr_url, r.pr_error, r.agent_started_at, r.kind, r.harness, r.model, r.provider_type, r.workspace, r.slug, r.subject, r.attempt, r.base_branch, r.base_commit, r.branch, r.pushed_as, r.review_id, r.finding_ref, r.task_summary, r.started_at, r.ended_at, r.work_item_id, r.checkpoint_head, r.work_ready_at, r.active_wall_seconds, - h.label AS credential_label, h.type AS credential_type + h.label AS credential_label, h.type AS credential_type, h.auth_mode AS credential_auth_mode FROM factory_run r LEFT JOIN harness_credential h ON h.id = r.harness_credential_id WHERE r.run_id = ? @@ -1051,7 +1052,7 @@ private static RunView readView(String runId, ResultSet rs, RunSpend spend) thro rs.getString("unit_id"), rs.getString("pr_url"), rs.getString("pr_error"), instant(rs, "agent_started_at"), rs.getString("kind"), rs.getString("harness"), rs.getString("model"), rs.getString("credential_label"), rs.getString("credential_type"), - rs.getString("provider_type"), rs.getString("workspace"), + rs.getString("credential_auth_mode"), rs.getString("provider_type"), rs.getString("workspace"), rs.getString("slug"), rs.getString("subject"), rs.getInt("attempt"), rs.getString("base_branch"), rs.getString("base_commit"), rs.getString("branch"), rs.getString("pushed_as"), rs.getString("review_id"), rs.getString("finding_ref"), diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCredentialPool.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCredentialPool.java index 50dfd33c4..754a2eb9d 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCredentialPool.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCredentialPool.java @@ -15,6 +15,7 @@ import java.time.Instant; import java.util.ArrayList; import java.util.List; +import java.util.Optional; import java.util.UUID; /** @@ -163,12 +164,11 @@ public Selection select() { WHERE id = ( SELECT id FROM harness_credential -- API keys only, and this is a CONTAINMENT boundary rather than a filter. A - -- subscription row holds the whole sign-in file; the harness arm would feed it - -- to --with-api-key, and the agent container -- which runs untrusted ticket - -- text at full shell access -- could then read it, refresh credential and all. - -- Until selection, injection and charging exist for a subscription, no run may - -- reach one. The rule lives here rather than in a caller's discipline because - -- three dispatch paths call this and each would have to remember. + -- subscription row holds the whole sign-in file, refresh token included; it is + -- reached only through selectSubscription, which leases it to one run and hands + -- out a copy with the refresh token removed. The rule lives here rather than in + -- a caller's discipline because three dispatch paths call this and each would + -- have to remember. WHERE enabled AND auth_mode = 'API_KEY' AND rejected_at IS NULL @@ -205,6 +205,87 @@ public Selection select() { return whyNothingIsAvailable(); } + /** + * Leases a signed-in seat of this harness to one run, and hands out its sign-in file (M3.5 part F). + * + *

A lease, where an API key is shared: two agents on one sign-in can each invalidate the other's + * session. The pick and the lease are ONE statement with {@code SKIP LOCKED}, so two dispatches cannot + * take the same seat. A seat whose lease has run out is free again — that bound is what frees a seat + * whose release never arrived. + * + * @return the member with its sign-in file as {@code apiKey}, or empty when every seat is in use, + * rejected, switched off, or none is signed in. The caller reports that as one refusal. + */ + public Optional selectSubscription(String harness, String runId, Instant leasedUntil) { + String sql = """ + UPDATE harness_credential + SET last_used_at = now(), updated_at = now(), leased_by_run = ?, leased_until = ? + WHERE id = ( + SELECT id FROM harness_credential + WHERE enabled + AND auth_mode = 'SUBSCRIPTION' + AND type = ? + AND rejected_at IS NULL + AND (rate_limited_until IS NULL OR rate_limited_until <= now()) + AND (leased_until IS NULL OR leased_until <= now()) + ORDER BY exhausted_at NULLS FIRST, last_used_at NULLS FIRST + LIMIT 1 + FOR UPDATE SKIP LOCKED) + RETURNING id, label, type, base_url, api_key + """; + try (Connection c = dataSource.getConnection(); PreparedStatement ps = c.prepareStatement(sql)) { + ps.setString(1, runId); + ps.setTimestamp(2, java.sql.Timestamp.from(leasedUntil)); + ps.setString(3, harness); + try (ResultSet rs = ps.executeQuery()) { + return rs.next() ? Optional.of(decrypt(rs.getObject("id", UUID.class), rs)) : Optional.empty(); + } + } catch (SQLException e) { + throw new IllegalStateException("The harness credential pool could not be read", e); + } + } + + /** + * Ends the lease this run holds, if it still holds one. Fenced by the run id, which is unique per + * attempt: a late release from an earlier run matches nothing and so cannot free a seat another run + * is using. + */ + public void releaseLease(String runId) { + try (Connection c = dataSource.getConnection(); PreparedStatement ps = c.prepareStatement( + "UPDATE harness_credential SET leased_by_run = NULL, leased_until = NULL WHERE leased_by_run = ?")) { + ps.setString(1, runId); + ps.executeUpdate(); + } catch (SQLException e) { + throw new IllegalStateException("The lease of run " + runId + " could not be released", e); + } + } + + /** How a member pays: {@code API_KEY} or {@code SUBSCRIPTION}. Empty for an unknown id. */ + public Optional authModeOf(UUID id) { + try (Connection c = dataSource.getConnection(); PreparedStatement ps = c.prepareStatement( + "SELECT auth_mode FROM harness_credential WHERE id = ?")) { + ps.setObject(1, id); + try (ResultSet rs = ps.executeQuery()) { + return rs.next() ? Optional.of(rs.getString(1)) : Optional.empty(); + } + } catch (SQLException e) { + throw new IllegalStateException("The harness credential pool could not be read", e); + } + } + + /** Whether any enabled, unrejected seat is signed in for this harness — the question a build setup asks. */ + public boolean hasSubscription(String harness) { + try (Connection c = dataSource.getConnection(); PreparedStatement ps = c.prepareStatement(""" + SELECT 1 FROM harness_credential + WHERE enabled AND auth_mode = 'SUBSCRIPTION' AND type = ? AND rejected_at IS NULL LIMIT 1 + """)) { + ps.setString(1, harness); + try (ResultSet rs = ps.executeQuery()) { return rs.next(); } + } catch (SQLException e) { + throw new IllegalStateException("The harness credential pool could not be read", e); + } + } + private PoolMember decrypt(UUID id, ResultSet rs) throws SQLException { return new PoolMember(id, rs.getString("label"), rs.getString("type"), rs.getString("base_url"), encryption.decryptString(rs.getString("api_key"), aad(id))); @@ -301,19 +382,19 @@ public boolean clearRejection(UUID id) { /** * What the settings surface shows. Never carries the key. * - * @param authMode {@code API_KEY} or {@code SUBSCRIPTION}. On the view because the screen must be - * able to say that a subscription cannot pay for a run yet: it is deliberately unreachable by - * the selector, and a row rendered as plain "Available" told the operator the opposite. + * @param authMode {@code API_KEY} or {@code SUBSCRIPTION}, so the screen can say which kind a row is. + * @param leasedUntil when a subscription's current lease runs out, or null when no run holds it — + * "in use" is the one state an operator sees for a seat and not for a shared key. */ public record MemberView(UUID id, String label, String type, String baseUrl, boolean enabled, Instant rateLimitedUntil, Instant rejectedAt, Instant lastUsedAt, - String authMode) { + String authMode, Instant leasedUntil) { } public List list() { String sql = """ SELECT id, label, type, base_url, enabled, rate_limited_until, rejected_at, last_used_at, - auth_mode + auth_mode, CASE WHEN leased_until > now() THEN leased_until END AS leased_until FROM harness_credential ORDER BY label """; List members = new ArrayList<>(); @@ -323,7 +404,7 @@ public List list() { members.add(new MemberView(rs.getObject("id", UUID.class), rs.getString("label"), rs.getString("type"), rs.getString("base_url"), rs.getBoolean("enabled"), instant(rs, "rate_limited_until"), instant(rs, "rejected_at"), - instant(rs, "last_used_at"), rs.getString("auth_mode"))); + instant(rs, "last_used_at"), rs.getString("auth_mode"), instant(rs, "leased_until"))); } return members; } catch (SQLException e) { @@ -358,7 +439,7 @@ INSERT INTO harness_credential (id, label, type, base_url, api_key) ps.setString(4, baseUrl); ps.setString(5, encryption.encryptString(apiKey, aad(id))); ps.executeUpdate(); - return new MemberView(id, label, type, baseUrl, true, null, null, null, "API_KEY"); + return new MemberView(id, label, type, baseUrl, true, null, null, null, "API_KEY", null); } catch (SQLException e) { if ("23505".equals(e.getSQLState())) { throw new DuplicateLabelException(label, e); diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/RunCharges.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/RunCharges.java index 35e8d398d..7043a2e37 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/RunCharges.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/RunCharges.java @@ -14,6 +14,7 @@ import org.jboss.logging.Logger; import java.util.List; +import java.util.Optional; import java.util.UUID; /** @@ -65,6 +66,10 @@ public class RunCharges { @Inject LlmModelPricer pricer; + /** How the run's credential paid: the pricing rule, where the model used to be (M3.5 part F). */ + @Inject + HarnessCredentialPool pool; + /** * The largest self-reported usage this deployment will PRICE for one run. * @@ -99,10 +104,17 @@ public void record(RunResult result) { // IllegalArgumentException out of valueOf — which would reach the messaging layer and // produce exactly the redelivery loop the comment below says this catch prevents. ModelUsage usage = RunTokenUsage.of(result, model, maxReportedTokens); - List lines = pricer.priceCall(model, usage); // Which key paid, read from the run's own row like the model beside it. Empty for a run // dispatched before the pool existed; the column is nullable for exactly that. - String credentialRef = runs.harnessCredentialOf(runId).map(UUID::toString).orElse(null); + Optional credential = runs.harnessCredentialOf(runId); + // Priced by how the run PAID, not by its model: a subscription run's tokens cost nothing per + // token, and the same model's API-key run must still be priced (M3.5 part F, design §5.7). + // A lambda, not pool::authModeOf: a bound method reference dereferences pool at once, so a run + // with no credential at all would fail on a pool it never needed. + boolean subscription = credential.flatMap(id -> pool.authModeOf(id)) + .filter(dev.codespire.contract.work.PayWith.SUBSCRIPTION::equals).isPresent(); + List lines = subscription ? pricer.priceUnmetered(usage) : pricer.priceCall(model, usage); + String credentialRef = credential.map(UUID::toString).orElse(null); ledger.recordCharges(ChargeCall.forRun(runId, CallRefs.forRun(runId, AGENT_CALL), model, lines, credentialRef)); // The terminal-status push precedes charging. FIX runs and runs that delivered nothing diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/RunResultSaga.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/RunResultSaga.java index c0136192d..f32097e73 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/RunResultSaga.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/RunResultSaga.java @@ -32,6 +32,10 @@ public class RunResultSaga { @Inject FactoryPullRequests pullRequests; + /** Frees the signed-in seat a finished agent held (M3.5 part F). */ + @Inject + HarnessCredentialPool pool; + @Inject dev.codespire.orchestrator.work.WorkItemRunBridge workItems; @@ -48,6 +52,10 @@ public void on(RunResult result) { MDC.put(MDC_RUN_ID, result.runId()); try { LOG.infof("run result %s", result.getClass().getSimpleName()); + // FIRST, before any check that may drop the result: every one of these says the agent has + // stopped, so the signed-in seat it held is free for the next run whatever else happens to + // this result. Fenced by run id, so a late result from an older run frees nothing. + if (!(result instanceof RunResult.RunStarted)) pool.releaseLease(result.runId()); if(!workItems.acceptsBinding(result))return; projection.apply(result); // AFTER the projection, deliberately. The run's outcome is the fact an operator is diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/SignInFiles.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/SignInFiles.java new file mode 100644 index 000000000..50d9c9467 --- /dev/null +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/SignInFiles.java @@ -0,0 +1,51 @@ +package dev.codespire.orchestrator.factory; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.node.ObjectNode; + +import java.util.Iterator; +import java.util.Map; + +/** + * The copy of a stored sign-in that an agent may hold (M3.5 part F). + * + *

The agent runs untrusted ticket text at full shell access, so whatever it is given, it can read. An + * access token expires on its own; a refresh token does not, and an agent that reads one holds the seat + * until a person revokes it. So every refresh token in the file is EMPTIED before the file leaves the + * orchestrator — emptied rather than removed, because the vendor's CLI refuses a file whose + * {@code refresh_token} field is missing but accepts an empty one and runs on the access token (measured + * 2026-09-25, codex-cli 0.156.1: design §5.3). + */ +final class SignInFiles { + + private static final ObjectMapper JSON = new ObjectMapper(); + private static final String REFRESH_TOKEN = "refresh_token"; + + private SignInFiles() { + } + + /** + * @throws IllegalStateException named {@code subscription_unreadable} when the stored bytes are not a + * JSON object — never with the bytes in the message, which are a credential + */ + static String forAgent(String storedFile) { + try { + JsonNode file = JSON.readTree(storedFile); + if (!(file instanceof ObjectNode object)) throw new IllegalStateException("subscription_unreadable"); + emptyRefreshTokens(object); + return JSON.writeValueAsString(object); + } catch (JsonProcessingException unreadable) { + throw new IllegalStateException("subscription_unreadable"); + } + } + + private static void emptyRefreshTokens(ObjectNode node) { + for (Iterator> fields = node.properties().iterator(); fields.hasNext(); ) { + Map.Entry field = fields.next(); + if (field.getKey().equals(REFRESH_TOKEN)) field.setValue(JSON.getNodeFactory().textNode("")); + else if (field.getValue() instanceof ObjectNode child) emptyRefreshTokens(child); + } + } +} 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 14ab16605..0d6fc638f 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 @@ -45,32 +45,58 @@ public Prepared assemble(Connection c,WorkSourceRegistry.Source source,WorkItemE ps.setObject(1,source.repositoryId());try(ResultSet rs=ps.executeQuery()) { if(!rs.next())throw new IllegalStateException("factory_account_unavailable"); } } var account=accounts.resolve(source.repositoryId()).orElseThrow(()->new IllegalStateException("factory_account_unavailable")); + // How the APPROVED preparation pays — not the build setup now, which may have changed since. + boolean subscription=dev.codespire.contract.work.PayWith.SUBSCRIPTION.equals(item.preparation().payWith()); // Every type the chosen harness can report must have a rate or a not-billed assertion. Asking // only about INPUT and OUTPUT is what let item 36 start, spend, report CACHED_INPUT and - // REASONING, and stop with a cost nobody could account for. - try { if(models.isDisabled(in.model()))throw new IllegalStateException("model_disabled"); } + // REASONING, and stop with a cost nobody could account for. A subscription is not priced per + // token, so the price list neither enables nor blocks it. + try { if(!subscription && models.isDisabled(in.model()))throw new IllegalStateException("model_disabled"); } 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 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(","))); + if(!subscription) { + 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(","))); + } + // Still consulted for a subscription run: it adds no money, but the deployment's own gate is the + // operator's policy, and part F does not quietly change it (design §5.7). if(spend.decide().refused())throw new IllegalStateException("deployment_spend_cap_reached"); - if(!(pool.select() instanceof HarnessCredentialPool.Selection.Chosen chosen))throw new IllegalStateException("harness_credential_unavailable"); 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()), + HarnessCredentialPool.PoolMember member=subscription ? leaseSeat(in.harness(),id,wall) : pickKey(); + // The agent gets the sign-in with its refresh token emptied; the whole file stays here, encrypted. + String handedOver=subscription ? SignInFiles.forAgent(member.apiKey()) : member.apiKey(); + RunCommand.ExecuteRun command=new RunCommand.ExecuteRun(id,source.repository(),FactoryCloneUrls.cloneUrl(source.scm(),account.baseUrl(),source.repository()), // 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())) + credentials.packScm(id,account.botUsername(),account.secret()),credentials.packHarness(id,handedOver)) // The level the approved binding hashed, so the build runs at what was approved (M3.5 part M). .atEffort(item.preparation().effort()); + if(subscription)command=command.paidBySignIn(); 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())); + return new Prepared(held,new FactoryRunProjection.QueuedRun(id,in.harness(),in.model(),in.baseBranch(),in.baseCommit(),branch,account.botUsername(),member.id())); + } + + /** + * How long past the wall clock a lease outlives the run it was taken for. The run's own result + * releases it; this only bounds a lease whose release never arrives, so the seat is not held for ever. + */ + private static final long LEASE_MARGIN_SECONDS=600; + + private HarnessCredentialPool.PoolMember leaseSeat(String harness,String runId,long wallSeconds) { + return pool.selectSubscription(harness,runId,java.time.Instant.now().plusSeconds(wallSeconds+LEASE_MARGIN_SECONDS)) + .orElseThrow(()->new IllegalStateException("subscription_unavailable")); + } + + private HarnessCredentialPool.PoolMember pickKey() { + if(!(pool.select() instanceof HarnessCredentialPool.Selection.Chosen chosen))throw new IllegalStateException("harness_credential_unavailable"); + return chosen.member(); } } diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/llm/LlmModelPricer.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/llm/LlmModelPricer.java index 25b162f52..fc403c24c 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/llm/LlmModelPricer.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/llm/LlmModelPricer.java @@ -75,6 +75,21 @@ public List priceCall(String model, ModelUsage usage) { return counts.stream().map(count -> line(pricing, count)).toList(); } + /** + * A call paid for by a subscription rather than per token (M3.5 part F): rate 0 and cost 0, with the + * real token counts, whatever the model's own pricing says. Priced by how the call PAID rather than by + * model, so one model can serve an API-key run and a subscription run in the same deployment. + * + *

Missing usage stays UNKNOWN, exactly as in {@link #priceCall}: a subscription makes a reported + * call cost zero; it does not make an unreported one free. + */ + public List priceUnmetered(ModelUsage usage) { + List counts = usage == null ? List.of() : usage.counts(); + if (counts.isEmpty()) return List.of(ChargeLine.unknown(TokenType.TOTAL, 0)); + if (!usage.reconciled()) return List.of(ChargeLine.unmetered(TokenType.TOTAL, usage.reportedTotal())); + return counts.stream().map(count -> ChargeLine.unmetered(count.type(), count.tokens())).toList(); + } + /** Whether a review may be started against this model: priceable, or explicitly unbilled. */ public boolean isPriceable(String model) { Pricing pricing = pricingFor(model); 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 082198fd4..69c5e8493 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 @@ -203,9 +203,12 @@ private Result attempt(String id, boolean again, String actor, if (!setup.set()) return refuse(id, generation, expectedRevision, "build_defaults_missing"); // What the dispatch will ask, asked before an approval is opened on it. Without this, disabling // a model after the setup was saved still produced a decision whose build was already refused. + boolean subscription = dev.codespire.contract.work.PayWith.SUBSCRIPTION.equals(setup.payWith()); try { - if (models.isDisabled(setup.model())) return refuse(id, generation, expectedRevision, "model_disabled"); - var unpriced = pricer.unpricedTypes(setup.model(), setup.harness()); + // A subscription is not priced per token, so the price list neither enables nor blocks it. + if (!subscription && models.isDisabled(setup.model())) return refuse(id, generation, expectedRevision, "model_disabled"); + var unpriced = subscription ? java.util.List.of() + : 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. @@ -247,7 +250,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.EFFORT_BINDING, setup.effort()); + WorkPreparation.PAY_WITH_BINDING, setup.effort(), setup.payWith()); // 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/main/java/dev/codespire/orchestrator/work/WorkRunAssemblyRefusals.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/work/WorkRunAssemblyRefusals.java index a39911517..72d77018d 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 @@ -21,7 +21,8 @@ final class WorkRunAssemblyRefusals { 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_not_run_by_harness", "effort_not_offered", "effort_unverifiable"); + "model_not_run_by_harness", "effort_not_offered", "effort_unverifiable", + "subscription_unavailable", "subscription_unreadable"); /** 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/main/resources/db/migration/V83__pay_with_subscription.sql b/spire-orchestrator/src/main/resources/db/migration/V83__pay_with_subscription.sql new file mode 100644 index 000000000..ce4b92216 --- /dev/null +++ b/spire-orchestrator/src/main/resources/db/migration/V83__pay_with_subscription.sql @@ -0,0 +1,14 @@ +-- Paying for a build with a Codex subscription (M3.5 part F). + +-- A signed-in seat serves ONE agent at a time: two agents sharing a sign-in can each invalidate the +-- other's session. So a subscription member carries a lease: which run holds it, and until when. The +-- run id is the fence -- it is unique per attempt, so a late release from an old run matches nothing -- +-- and the time bounds a lease whose release never arrives. NULL for every API key, which is shared. +ALTER TABLE harness_credential ADD COLUMN leased_by_run TEXT; +ALTER TABLE harness_credential ADD COLUMN leased_until TIMESTAMPTZ; + +-- How a repository's builds pay. Every existing setup pays with an API key, because until now that was +-- the only way a build could. +ALTER TABLE repository_build_defaults ADD COLUMN pay_with VARCHAR(16) NOT NULL DEFAULT 'API_KEY'; +ALTER TABLE repository_build_defaults ADD CONSTRAINT repository_build_defaults_known_pay_with + CHECK (pay_with IN ('API_KEY', 'SUBSCRIPTION')); 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 b3e9629e5..aa78cae2a 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 @@ -156,6 +156,64 @@ void noThinkingLevelMeansTheModelsOwnDefault() { assertNull(defaults.save(repository, input("main", "codex", model, 0), "TEST-operator").effort()); } + @Inject HarnessCredentialPool pool; + + /** A signed-in codex seat, switched off by the caller when done: a seat a run points at cannot be deleted. */ + private UUID seat() throws java.sql.SQLException { + try (var c = dataSource.getConnection()) { + return pool.addSubscription(c, "TEST-build-seat-" + UUID.randomUUID(), "codex", "{\"auth_mode\":\"TEST\"}"); + } + } + + /** + * A subscription is not priced per token, so a model with no price at all may be saved — the harness's + * own list is what says it can run (M3.5 part F). + */ + @Test + void aSubscriptionSetupNeedsASeatButNoPrice() throws java.sql.SQLException { + String unpriced = "TEST-unpriced-" + UUID.randomUUID().toString().substring(0, 8); + UUID seat = seat(); + try { + var saved = defaults.save(repository, new BuildDefaults.Input(0, "main", "codex", unpriced, null, "SUBSCRIPTION"), + "TEST-operator"); + + assertEquals("SUBSCRIPTION", saved.payWith()); + assertEquals("SUBSCRIPTION", defaults.get(repository).payWith()); + } finally { + pool.remove(seat); + } + } + + /** Paying with a subscription nobody has signed in would refuse every build it prepares. */ + @Test + void aSubscriptionSetupIsRefusedWhileNoSeatIsSignedIn() { + assumeNoCodexSeat(); + assertEquals("subscription_not_signed_in", refusal(new BuildDefaults.Input(0, "main", "codex", model, null, "SUBSCRIPTION"))); + } + + /** Other suites switch their seats off; one left on would make the refusal above untestable, not wrong. */ + private void assumeNoCodexSeat() { + org.junit.jupiter.api.Assumptions.assumeFalse(pool.hasSubscription("codex"), "a codex seat is signed in on this database"); + } + + /** The same model paid with an API key is still refused without its prices. */ + @Test + void theSameModelPaidWithAKeyStillNeedsItsPrices() { + String unpriced = "TEST-unpriced-" + UUID.randomUUID().toString().substring(0, 8); + + assertEquals("model_unknown", refusal(new BuildDefaults.Input(0, "main", "codex", unpriced, null, "API_KEY"))); + } + + @Test + void anUnknownWayToPayIsRefused() { + assertEquals("pay_with_invalid", refusal(new BuildDefaults.Input(0, "main", "codex", model, null, "TEST-free"))); + } + + @Test + void aSetupThatSaysNothingPaysWithAKey() { + assertEquals("API_KEY", defaults.save(repository, input("main", "codex", model, 0), "TEST-operator").payWith()); + } + private String refusal(BuildDefaults.Input input) { return assertThrows(BuildDefaults.Refused.class, () -> defaults.save(repository, input, "TEST-operator")).reason(); } diff --git a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/FactoryRunProjectionTest.java b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/FactoryRunProjectionTest.java index 3d220cb37..d4d01b60e 100644 --- a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/FactoryRunProjectionTest.java +++ b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/FactoryRunProjectionTest.java @@ -373,6 +373,26 @@ void aRunNamesTheCredentialThatPaidForIt() { FactoryRunProjection.RunView view = projection.find(runId).orElseThrow(); assertEquals(member.label(), view.credentialLabel()); assertEquals("openai", view.credentialType()); + assertEquals("API_KEY", view.credentialAuthMode()); + } + + /** A run a signed-in seat paid for says so; the screen must not call it an API key billed per token. */ + @Test + void aRunPaidByASubscriptionSaysSo() throws SQLException { + UUID seat; + try (Connection c = dataSource.getConnection()) { + seat = pool.addSubscription(c, "TEST-frp-seat-" + UUID.randomUUID(), "codex", "{\"auth_mode\":\"TEST\"}"); + } + try { + String runId = "run::github:TEST-acme/app:subject-" + UUID.randomUUID() + ":1"; + assertTrue(queueWith(runId, seat)); + + FactoryRunProjection.RunView view = projection.find(runId).orElseThrow(); + assertEquals("SUBSCRIPTION", view.credentialAuthMode()); + assertEquals("codex", view.credentialType()); + } finally { + pool.remove(seat); + } } /** A run dispatched with no pool member names none, rather than inventing one. */ @@ -381,6 +401,7 @@ void aRunWithNoCredentialNamesNone() { FactoryRunProjection.RunView view = projection.find(queuedRun()).orElseThrow(); assertNull(view.credentialLabel()); assertNull(view.credentialType()); + assertNull(view.credentialAuthMode()); } private String queuedRun() { diff --git a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessSubscriptionLeaseTest.java b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessSubscriptionLeaseTest.java new file mode 100644 index 000000000..2cbdaecfa --- /dev/null +++ b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessSubscriptionLeaseTest.java @@ -0,0 +1,123 @@ +package dev.codespire.orchestrator.factory; + +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.PreparedStatement; +import java.sql.SQLException; +import java.time.Instant; +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; +import java.util.UUID; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * A signed-in seat serves one build at a time (M3.5 part F, design §5.5). + * + *

Uses its own harness name so no other suite's seats are in range, and switches every seat it made + * off afterwards: a seat a finished run points at cannot be deleted, by design. + */ +@QuarkusTest +class HarnessSubscriptionLeaseTest { + + @Inject HarnessCredentialPool pool; + @Inject DataSource dataSource; + + private static final String HARNESS = "TEST-lease-harness"; + private static final String FILE = "{\"auth_mode\":\"TEST\",\"tokens\":{\"refresh_token\":\"TEST-refresh\"}}"; + private final List seats = new ArrayList<>(); + + @AfterEach + void switchSeatsOff() { + seats.forEach(pool::remove); + } + + private UUID seat(String label) throws SQLException { + try (Connection c = dataSource.getConnection()) { + UUID id = pool.addSubscription(c, label + "-" + UUID.randomUUID(), HARNESS, FILE); + seats.add(id); + return id; + } + } + + private static Instant inAnHour() { + return Instant.now().plusSeconds(3600); + } + + @Test + void aLeasedSeatIsNotHandedToASecondRun() throws SQLException { + UUID seat = seat("TEST-lease-one"); + + Optional first = pool.selectSubscription(HARNESS, "TEST-run-1", inAnHour()); + Optional second = pool.selectSubscription(HARNESS, "TEST-run-2", inAnHour()); + + assertEquals(seat, first.orElseThrow().id()); + assertEquals(FILE, first.orElseThrow().apiKey(), "the whole stored file; the dispatch empties its refresh token"); + assertTrue(second.isEmpty(), "one sign-in, one agent"); + } + + /** Fenced by run id: a late release from an earlier run cannot free a seat another run is using. */ + @Test + void onlyTheRunHoldingTheLeaseCanRelease() throws SQLException { + seat("TEST-lease-fence"); + pool.selectSubscription(HARNESS, "TEST-run-holder", inAnHour()).orElseThrow(); + + pool.releaseLease("TEST-run-someone-else"); + assertTrue(pool.selectSubscription(HARNESS, "TEST-run-next", inAnHour()).isEmpty()); + + pool.releaseLease("TEST-run-holder"); + assertTrue(pool.selectSubscription(HARNESS, "TEST-run-next", inAnHour()).isPresent()); + } + + /** A lease whose release never arrives ends by itself, so a seat is never held for ever. */ + @Test + void aLeaseThatRanOutFreesTheSeat() throws SQLException { + UUID seat = seat("TEST-lease-expired"); + pool.selectSubscription(HARNESS, "TEST-run-lost", inAnHour()).orElseThrow(); + try (Connection c = dataSource.getConnection(); + PreparedStatement ps = c.prepareStatement("UPDATE harness_credential SET leased_until=now()-interval '1 minute' WHERE id=?")) { + ps.setObject(1, seat); + ps.executeUpdate(); + } + + assertTrue(pool.selectSubscription(HARNESS, "TEST-run-after", inAnHour()).isPresent()); + } + + /** The screen says "in use" only while a lease is live. */ + @Test + void theListSaysWhenASeatIsInUse() throws SQLException { + UUID seat = seat("TEST-lease-view"); + pool.selectSubscription(HARNESS, "TEST-run-view", inAnHour()).orElseThrow(); + + var view = pool.list().stream().filter(member -> member.id().equals(seat)).findFirst().orElseThrow(); + assertNotNull(view.leasedUntil()); + + pool.releaseLease("TEST-run-view"); + assertNull(pool.list().stream().filter(member -> member.id().equals(seat)).findFirst().orElseThrow().leasedUntil()); + } + + /** API-key selection never reaches a seat: that path hands its credential out whole, to anyone. */ + @Test + void theKeySelectorNeverHandsOutASeat() throws SQLException { + UUID seat = seat("TEST-lease-containment"); + + HarnessCredentialPool.Selection selection = pool.select(); + + assertFalse(selection instanceof HarnessCredentialPool.Selection.Chosen chosen && chosen.member().id().equals(seat)); + } + + @Test + void aSeatOfAnotherHarnessIsNotOffered() throws SQLException { + seat("TEST-lease-other"); + + assertTrue(pool.selectSubscription("TEST-no-such-harness", "TEST-run-x", inAnHour()).isEmpty()); + assertTrue(pool.hasSubscription(HARNESS)); + assertFalse(pool.hasSubscription("TEST-no-such-harness")); + } +} diff --git a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/RunChargesTest.java b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/RunChargesTest.java index c1940071a..0e49faff1 100644 --- a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/RunChargesTest.java +++ b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/RunChargesTest.java @@ -93,6 +93,20 @@ public List priceCall(String model, ModelUsage usage) { private final RecordingLedger ledger = new RecordingLedger(); private final StubRuns runs = new StubRuns(); private final StubPricer pricer = new StubPricer(); + /** + * How the run's credential paid. Overridden on purpose, for the reason StubRuns gives: the real one + * reads the database, and RunCharges would swallow the failure. + */ + private static final class StubPool extends HarnessCredentialPool { + String mode = "API_KEY"; + + @Override + public Optional authModeOf(java.util.UUID id) { + return Optional.of(mode); + } + } + + private final StubPool pool = new StubPool(); private final RunCharges charges = charges(); private RunCharges charges() { @@ -100,6 +114,7 @@ private RunCharges charges() { c.ledger = ledger; c.runs = runs; c.pricer = pricer; + c.pool = pool; // Stated rather than left at the field default. Outside CDI a long field is 0, and these // tests are about what gets charged -- not about a ceiling nobody set. c.maxReportedTokens = RunTokenUsage.UNBOUNDED; @@ -110,6 +125,34 @@ private static RunResult.RunFinished finished(String runId, Map us return new RunResult.RunFinished(runId, "refs/heads/spire/s", List.of(), List.of(), usage, false); } + /** + * A run paid for by a subscription costs nothing per token, whatever its model's rates — and keeps its + * real token counts, so what a subscription run used is still visible (M3.5 part F, design §5.7). + */ + @Test + void aSubscriptionRunIsChargedNothingPerTokenWithItsRealCounts() { + runs.credential = java.util.UUID.fromString("00000000-0000-4000-8000-00000000c0de"); + pool.mode = "SUBSCRIPTION"; + + charges.record(finished(RUN_ID, Map.of("INPUT", 1200L, "OUTPUT", 340L))); + + var lines = ledger.calls.getFirst().lines(); + assertEquals(2, lines.size()); + assertTrue(lines.stream().allMatch(line -> line.mode() == dev.codespire.orchestrator.llm.PricingMode.UNMETERED && Long.valueOf(0L).equals(line.costMillicents())), lines.toString()); + assertEquals(1540, lines.stream().mapToInt(ChargeLine::tokens).sum()); + } + + /** The same model paid with an API key is still priced: pricing follows the payment, not the model. */ + @Test + void anApiKeyRunOfTheSameModelIsStillPriced() { + runs.credential = java.util.UUID.fromString("00000000-0000-4000-8000-00000000c0de"); + pool.mode = "API_KEY"; + + charges.record(finished(RUN_ID, Map.of("INPUT", 1200L))); + + assertEquals(dev.codespire.orchestrator.llm.PricingMode.METERED, ledger.calls.getFirst().lines().getFirst().mode()); + } + @Test void aFinishedRunWritesOneChargeLinePerTokenType() { charges.record(finished(RUN_ID, Map.of("INPUT", 1200L, "OUTPUT", 340L))); diff --git a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/RunResultSagaTest.java b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/RunResultSagaTest.java index 31ff48b80..009a7ad20 100644 --- a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/RunResultSagaTest.java +++ b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/RunResultSagaTest.java @@ -39,6 +39,25 @@ public void record(RunResult result) { } } + /** Which runs' seats the saga released. Static because the saga factories below are. */ + private static final List released = new ArrayList<>(); + + /** + * Every result that says the agent has stopped frees the seat it held; a start does not (M3.5 part F). + * Released even when the work-item bridge would drop the result, because the agent is still done. + */ + @Test + void aResultThatEndsTheAgentFreesItsSeatAndAStartDoesNot() { + released.clear(); + RunResultSaga saga = saga(new RecordingProjection()); + + saga.on(new RunResult.RunStarted("run::github:TEST-acme/app:started:1", "TEST-unit")); + saga.on(new RunResult.RunFinished("run::github:TEST-acme/app:done:1", "refs/heads/spire/s", + List.of(), List.of(), Map.of("input", 10L), false)); + + assertEquals(List.of("run::github:TEST-acme/app:done:1"), released); + } + private static RunResultSaga saga(RecordingProjection projection) { return saga(projection, new RecordingCharges()); } @@ -57,6 +76,13 @@ private static RunResultSaga saga(RecordingProjection projection, RecordingCharg // Same reason, and the same trap arriving again with a new collaborator: RunCredentialFeedback // reads the run's row to find which pool member to mark, so leaving it null is an NPE and // leaving it real is a database call from a unit test. + // The same trap once more: releasing a seat's lease writes to the pool's table. Recorded instead. + saga.pool = new HarnessCredentialPool() { + @Override + public void releaseLease(String runId) { + released.add(runId); + } + }; saga.credentials = new RunCredentialFeedback() { @Override public void reactTo(RunResult result) { diff --git a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/SignInFilesTest.java b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/SignInFilesTest.java new file mode 100644 index 000000000..abbad0276 --- /dev/null +++ b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/SignInFilesTest.java @@ -0,0 +1,43 @@ +package dev.codespire.orchestrator.factory; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * The agent may hold the short-lived half of a sign-in, never the refresh token (M3.5 part F, design §5.6). + * Placeholder values: this is about what reaches the agent. + */ +class SignInFilesTest { + + private static final ObjectMapper JSON = new ObjectMapper(); + private static final String STORED = "{\"auth_mode\":\"chatgpt\",\"tokens\":{\"access_token\":\"TEST-access\"," + + "\"id_token\":\"TEST-id\",\"refresh_token\":\"TEST-refresh-must-not-leave\",\"account_id\":\"TEST-account\"}," + + "\"last_refresh\":\"1970-01-01T00:00:00Z\"}"; + + @Test + void theRefreshTokenIsEmptiedAndEverythingElseKept() throws Exception { + String forAgent = SignInFiles.forAgent(STORED); + JsonNode file = JSON.readTree(forAgent); + + assertFalse(forAgent.contains("TEST-refresh-must-not-leave")); + // Emptied, not removed: the vendor's CLI refuses a file whose refresh_token field is missing. + assertTrue(file.path("tokens").has("refresh_token")); + assertEquals("", file.path("tokens").path("refresh_token").asText()); + assertEquals("TEST-access", file.path("tokens").path("access_token").asText()); + assertEquals("TEST-id", file.path("tokens").path("id_token").asText()); + assertEquals("chatgpt", file.path("auth_mode").asText()); + } + + @Test + void storedBytesThatAreNotAJsonObjectAreRefusedWithoutQuotingThem() { + IllegalStateException refused = assertThrows(IllegalStateException.class, + () -> SignInFiles.forAgent("TEST-not-a-sign-in")); + assertEquals("subscription_unreadable", refused.getMessage()); + } +} 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 d0e52bc72..2cea3f84e 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 @@ -105,6 +105,30 @@ void aSetupTheHarnessNoLongerRunsIsNotPrepared() throws Exception { } } + @Inject dev.codespire.orchestrator.factory.HarnessCredentialPool pool; + + /** How the setup pays is what the prepared task binds, so the approval covers it (M3.5 part F). */ + @Test + void theSavedWayToPayIsCopiedIntoThePreparedTask() throws Exception { + java.util.UUID seat; + try (Connection c = dataSource.getConnection()) { + seat = pool.addSubscription(c, "TEST-sweep-seat-" + java.util.UUID.randomUUID(), "codex", "{\"auth_mode\":\"TEST\"}"); + } + try { + defaults.save(repository, new BuildDefaults.Input(defaults.get(repository).revision(), "main", "codex", model, + null, "SUBSCRIPTION"), "TEST-prepared-admin"); + String id = admit("assisted", 83); + + sweep.sweep(); + + var prepared = store.load(id).preparation(); + assertNotNull(prepared); + assertEquals(dev.codespire.contract.work.PayWith.SUBSCRIPTION, prepared.payWith()); + } finally { + pool.remove(seat); + } + } + /** The level saved with the build setup is the level the prepared task binds (M3.5 part M). */ @Test void theSavedThinkingLevelIsCopiedIntoThePreparedTask() throws Exception { @@ -119,7 +143,7 @@ void theSavedThinkingLevelIsCopiedIntoThePreparedTask() throws Exception { var prepared = store.load(id).preparation(); assertNotNull(prepared); assertEquals("high", prepared.effort()); - assertEquals(WorkPreparation.EFFORT_BINDING, prepared.bindingVersion()); + assertEquals(WorkPreparation.PAY_WITH_BINDING, prepared.bindingVersion()); } finally { executeWith("DELETE FROM harness_catalogue"); } @@ -134,8 +158,9 @@ void oneTicketBecomesAPreparedTaskWithNoTypingAtAll() throws Exception { var prepared = store.load(id).preparation(); assertNotNull(prepared, "the ticket alone must be enough"); - assertEquals(WorkPreparation.EFFORT_BINDING, prepared.bindingVersion()); + assertEquals(WorkPreparation.PAY_WITH_BINDING, prepared.bindingVersion()); assertNull(prepared.effort(), "no level was saved, so the model's own default applies"); + assertEquals(dev.codespire.contract.work.PayWith.API_KEY, prepared.payWith(), "a setup that says nothing pays with a key"); 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 31d65e5ab..3671927c3 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 @@ -62,6 +62,36 @@ private void codexRuns(String slug,String... levels) { "TEST-registry.invalid/agent@sha256:0000000000000000000000000000000000000000000000000000000000000000")); } @org.junit.jupiter.api.AfterEach void forgetTheCatalogue() throws Exception {executeWith("DELETE FROM harness_catalogue");} + @Inject dev.codespire.encryption.EncryptionService encryption; + + /** + * A build approved to pay with a subscription leases a seat and hands the agent the sign-in with its + * refresh token emptied — never the whole stored file (M3.5 part F, design §5.6). + */ + @Test void aSubscriptionBuildLeasesASeatAndHandsOverNoRefreshToken() throws Exception { + UUID seat; + try(var c=dataSource.getConnection()) { + seat=pool.addSubscription(c,"TEST-dispatch-seat-"+UUID.randomUUID(),"codex", + "{\"auth_mode\":\"chatgpt\",\"tokens\":{\"access_token\":\"TEST-access\",\"refresh_token\":\"TEST-refresh-must-not-leave\"}}"); + } + try { + String id=admit("autonomous",58);var plain=preparation("TEST-prepared-admin"); + var paying=new WorkPreparation(plain.specification(),plain.plan(),plain.baseBranch(),plain.baseCommit(),plain.harness(), + plain.model(),plain.registeredBy(),WorkPreparation.PAY_WITH_BINDING,null,PayWith.SUBSCRIPTION); + var outcome=transitions.prepare(id,store.history(id).size(),paying);assertEquals(200,outcome.status(),outcome.reason()); + + dispatcher.drain(); + + var command=heldCommands.getLast().execution(); + assertTrue(command.harnessSignIn(),"the worker must write a file, not pipe a key"); + String handedOver=encryption.decryptString(command.harnessCredential(),dev.codespire.contract.command.RunCommand.harnessCredentialAad(command.runId())); + assertFalse(handedOver.contains("TEST-refresh-must-not-leave"),"the refresh token never leaves the orchestrator"); + assertTrue(handedOver.contains("TEST-access")); + assertEquals(1,count("SELECT count(*) FROM factory_run WHERE run_id=? AND harness_credential_id=?",command.runId(),seat), + "the run names the seat that paid"); + assertTrue(pool.list().stream().anyMatch(member->member.id().equals(seat)&&member.leasedUntil()!=null),"the seat is leased"); + } finally { pool.remove(seat); } + } @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/Credentials.java b/spire-run-worker/src/main/java/dev/codespire/runworker/Credentials.java index 7192bedbf..6d1a351ef 100644 --- a/spire-run-worker/src/main/java/dev/codespire/runworker/Credentials.java +++ b/spire-run-worker/src/main/java/dev/codespire/runworker/Credentials.java @@ -91,4 +91,43 @@ public Map harnessEnv(String runId, String packed) { return Map.of(HarnessInvocation.CREDENTIAL, encryption.decryptString(packed, RunCommand.harnessCredentialAad(runId))); } + + /** + * The same credential, handed over under {@link HarnessInvocation#SIGN_IN} when it is a sign-in file. + * It goes through {@link #harnessEnv(String, String)} rather than decrypting again, so that method + * stays the one a test fake overrides: a second decrypting overload let the scrub path bypass every + * fake and leave the key unscrubbed. + */ + public Map harnessEnv(String runId, String packed, boolean signIn) { + Map env = harnessEnv(runId, packed); + if (!signIn || env.isEmpty()) return env; + return Map.of(HarnessInvocation.SIGN_IN, env.values().iterator().next()); + } + + /** + * Everything in a sign-in file that must never reach a log: the file itself, and every text value in + * it long enough to be a token or an account id. A tool that echoes one token prints that token, not + * the whole file, so scrubbing the file as one string would miss exactly what leaks. + */ + public static java.util.List signInSecrets(String file) { + java.util.List secrets = new java.util.ArrayList<>(); + secrets.add(file); + try { + collect(JSON.readTree(file), secrets); + } catch (JsonProcessingException unreadable) { + // The whole file is still scrubbed; nothing inside it can be named. + } + return secrets; + } + + private static void collect(com.fasterxml.jackson.databind.JsonNode node, java.util.List secrets) { + if (node.isTextual() && node.asText().length() >= SHORTEST_SCRUBBED_VALUE) secrets.add(node.asText()); + node.forEach(child -> collect(child, secrets)); + } + + /** Short enough to cover an account id, long enough to leave a word like "chatgpt" alone. */ + private static final int SHORTEST_SCRUBBED_VALUE = 16; + + /** For the static helper above; it reads a document and never serialises one. */ + private static final ObjectMapper JSON = new ObjectMapper(); } diff --git a/spire-run-worker/src/main/java/dev/codespire/runworker/RunFailures.java b/spire-run-worker/src/main/java/dev/codespire/runworker/RunFailures.java index e75027993..f74b5462f 100644 --- a/spire-run-worker/src/main/java/dev/codespire/runworker/RunFailures.java +++ b/spire-run-worker/src/main/java/dev/codespire/runworker/RunFailures.java @@ -113,8 +113,12 @@ SecretScrub scrubFor(RunCommand.ExecuteRun command) { try { // No username: an API key rides a Bearer header, not a Basic pair, so a base64 form // built for it would match nothing. - credentials.harnessEnv(command.runId(), command.harnessCredential()).values() - .forEach(secret -> forms.add(new SecretScrub.Credential(null, secret))); + for (String secret : credentials.harnessEnv(command.runId(), command.harnessCredential(), + command.harnessSignIn()).values()) { + // A sign-in file is scrubbed token by token as well as whole (M3.5 part F). + for (String form : command.harnessSignIn() ? Credentials.signInSecrets(secret) : java.util.List.of(secret)) + forms.add(new SecretScrub.Credential(null, form)); + } } catch (RuntimeException undecryptable) { LOG.warnf("run %s: the harness credential could not be decrypted to redact it; this " + "run's failure details are unscrubbed for it", command.runId()); 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 84dfbb404..7328e5bdb 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 @@ -126,7 +126,8 @@ private RunUnitSpec build(RunCommand.ExecuteRun command, HarnessAdapter adapter, + "); the channel's ack budget is sized to the latter"); } Credentials.Scm scm = credentials.scm(command.runId(), command.scmCredential()); - Map harnessEnv = credentials.harnessEnv(command.runId(), command.harnessCredential()); + Map harnessEnv = credentials.harnessEnv(command.runId(), command.harnessCredential(), + command.harnessSignIn()); ContainerSpec init = new ContainerSpec( publisherImage, diff --git a/spire-run-worker/src/test/java/dev/codespire/runworker/RunLauncherTest.java b/spire-run-worker/src/test/java/dev/codespire/runworker/RunLauncherTest.java index c6fc6fc65..03e1d8d2c 100644 --- a/spire-run-worker/src/test/java/dev/codespire/runworker/RunLauncherTest.java +++ b/spire-run-worker/src/test/java/dev/codespire/runworker/RunLauncherTest.java @@ -670,6 +670,31 @@ void aFailureDetailCarriesNoCredential() { assertTrue(failed.detail().contains("create failed"), "the diagnosis itself must survive"); } + /** A sign-in is scrubbed token by token: an echoed access token is not the whole file (M3.5 part F). */ + @Test + void aSignInRunScrubsEachTokenOfTheFile() { + String accessToken = "TEST-access-token-0123456789"; + launcher.failures = failuresWith(new Credentials() { + @Override + public Scm scm(String runId, String packed) { + return new Scm(SCM_USERNAME, READ_SECRET, SCM_USERNAME, WRITE_SECRET); + } + + @Override + public Map harnessEnv(String runId, String packed) { + return Map.of(dev.codespire.harness.HarnessInvocation.CREDENTIAL, + "{\"tokens\":{\"access_token\":\"" + accessToken + "\"}}"); + } + }); + runtime.salvageFails = new IllegalStateException("codex said: bearer " + accessToken); + + RunResult.RunFailed failed = assertInstanceOf(RunResult.RunFailed.class, + launcher.launch(COMMAND.paidBySignIn(), RunObserver.IGNORING)); + + assertFalse(failed.detail().contains(accessToken), "one echoed token is still a leaked token"); + assertTrue(failed.detail().contains("codex said"), "the diagnosis itself must survive"); + } + @Test void aFailureIsRetryableOnlyWhenItsCauseIs() { // Every publisher failure used to be reported retryable. A push the forge rejected refuses 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 88265690f..55a2f646f 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 @@ -192,6 +192,15 @@ void aProtectedBranchIsNotDroppedJustBecauseTheModeIsTheDefault() { assertFalse(env.containsKey("SPIRE_BRANCH_MODE"), env.keySet().toString()); } + /** A run paid by a sign-in hands the agent the file, never as a key (M3.5 part F). */ + @Test + void aRunPaidBySignInHandsTheAgentTheFileNotAKey() { + Map env = builder.build(command().paidBySignIn(), new CodexAdapter()).agent().environment(); + + assertEquals(HARNESS_KEY, env.get("CODEX_SIGN_IN_FILE")); + assertFalse(env.containsKey("OPENAI_API_KEY")); + } + /** The level on the command is the level in the agent's command line, not lost on the way. */ @Test void theThinkingLevelReachesTheAgentsCommand() { diff --git a/spire-run-worker/src/test/java/dev/codespire/runworker/SignInSecretsTest.java b/spire-run-worker/src/test/java/dev/codespire/runworker/SignInSecretsTest.java new file mode 100644 index 000000000..c2545e40c --- /dev/null +++ b/spire-run-worker/src/test/java/dev/codespire/runworker/SignInSecretsTest.java @@ -0,0 +1,40 @@ +package dev.codespire.runworker; + +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * A tool that echoes one token prints that token, not the whole file, so a sign-in is scrubbed token by + * token as well as whole (M3.5 part F). Placeholder values: this is about which strings are listed. + */ +class SignInSecretsTest { + + private static final String FILE = "{\"auth_mode\":\"chatgpt\",\"tokens\":{\"access_token\":\"TEST-access-token-0123456789\"," + + "\"id_token\":\"TEST-id-token-0123456789\",\"refresh_token\":\"\",\"account_id\":\"TEST-account-0123456789\"}}"; + + @Test + void everyTokenInTheFileIsScrubbedAsWellAsTheWholeFile() { + var secrets = Credentials.signInSecrets(FILE); + + assertTrue(secrets.contains(FILE)); + assertTrue(secrets.contains("TEST-access-token-0123456789")); + assertTrue(secrets.contains("TEST-id-token-0123456789")); + assertTrue(secrets.contains("TEST-account-0123456789")); + } + + /** A short word is not a secret, and scrubbing it would mangle every log line that says it. */ + @Test + void shortWordsAreLeftAlone() { + var secrets = Credentials.signInSecrets(FILE); + + assertFalse(secrets.contains("chatgpt")); + assertFalse(secrets.contains("")); + } + + @Test + void anUnreadableFileIsStillScrubbedWhole() { + assertTrue(Credentials.signInSecrets("not json").contains("not json")); + } +} diff --git a/spire-ui/src/api.ts b/spire-ui/src/api.ts index 4ea1e14a2..46942ab93 100644 --- a/spire-ui/src/api.ts +++ b/spire-ui/src/api.ts @@ -1103,6 +1103,8 @@ export interface RunView extends RunListEntry { // Which pool member paid for this run. Null on a run dispatched before a credential was recorded. credentialLabel: string | null; credentialType: string | null; + // API_KEY or SUBSCRIPTION; absent from a server older than M3.5 part F, which only held API keys. + credentialAuthMode?: string | null; providerType: string; workspace: string; slug: string; @@ -1696,11 +1698,10 @@ export interface HarnessCredentialView { /** When the vendor refused the key. Only an operator clears this. */ rejectedAt: string | null; lastUsedAt: string | null; - /** - * How this member pays. A SUBSCRIPTION is deliberately unreachable by any run until selection, - * injection and zero-cost charging exist, so the screen must not render it as ready to use. - */ + /** How this member pays: a shared API key, or a signed-in seat that serves one build at a time. */ authMode?: 'API_KEY' | 'SUBSCRIPTION'; + /** When a seat's current lease runs out; null while no build holds it. Always null for a key. */ + leasedUntil?: string | null; } export interface NewHarnessCredential { label: string; type: string; baseUrl: string; apiKey: string } diff --git a/spire-ui/src/components/RunDefinitionCard.tsx b/spire-ui/src/components/RunDefinitionCard.tsx index 5349c3c6b..5cc0deaaa 100644 --- a/spire-ui/src/components/RunDefinitionCard.tsx +++ b/spire-ui/src/components/RunDefinitionCard.tsx @@ -5,6 +5,15 @@ import { formatEventTime } from '../format'; import RunCard, { RunFields } from './RunCard'; import { isRunUnfinished, reviewPath, runDuration } from './Runs'; +// Which credential paid, in the operator's words. A subscription seat is not billed per token, so it +// must never read as an API key: that is the one question this row exists to answer (M3.5 part F). +function billedTo(run: RunView): string | null { + if (!run.credentialLabel) return null; + const type = run.credentialType ? ` · ${run.credentialType}` : ''; + if (run.credentialAuthMode === 'SUBSCRIPTION') return `Subscription ${run.credentialLabel}${type}, no per-token price`; + return `API key ${run.credentialLabel}${type ? `${type}, billed per token` : ''}`; +} + export default function RunDefinitionCard({ run }: { run: RunView }) { const [now, setNow] = useState(Date.now); const ticking = isRunUnfinished(run.status) && !run.endedAt; @@ -15,9 +24,7 @@ export default function RunDefinitionCard({ run }: { run: RunView }) { }, [ticking]); const link = run.reviewId ? reviewPath(run.reviewId) : null; const elapsed = runDuration({ ...run, endedAt: run.endedAt ?? (ticking ? new Date(now).toISOString() : null) }); - // Which key paid, in the operator's words. Every harness credential here is an API key, billed per - // token — a subscription is not a credential this deployment holds, so the card never implies one. - const billing = run.credentialLabel ? `API key ${run.credentialLabel}${run.credentialType ? ` · ${run.credentialType}, billed per token` : ''}` : null; + const billing = billedTo(run); return {run.reviewId} : run.reviewId], diff --git a/spire-ui/src/components/RunDetail.test.tsx b/spire-ui/src/components/RunDetail.test.tsx index 1e107c8b8..7efc1e741 100644 --- a/spire-ui/src/components/RunDetail.test.tsx +++ b/spire-ui/src/components/RunDetail.test.tsx @@ -196,6 +196,13 @@ it('names the key a run was billed to', async () => { expect(screen.getByText('API key TEST-factory-key · openai, billed per token')).toBeInTheDocument(); }); +// A seat's run is not billed per token; calling it an API key would answer the question wrongly. +it('names a subscription seat as a subscription, not as a key billed per token', async () => { + show(runView({ credentialLabel: 'TEST-factory-seat', credentialType: 'codex', credentialAuthMode: 'SUBSCRIPTION' })); + expect(await screen.findByText('Subscription TEST-factory-seat · codex, no per-token price')).toBeInTheDocument(); + expect(screen.queryByText(/API key/)).toBeNull(); +}); + // An unrecorded key reads as unknown, never as a run nobody paid for. it('shows no key rather than inventing one when a run recorded none', async () => { show(runView({ credentialLabel: null, credentialType: null })); diff --git a/spire-ui/src/components/SettingsHarnessCredentials.test.tsx b/spire-ui/src/components/SettingsHarnessCredentials.test.tsx index a6537bd30..b6805041f 100644 --- a/spire-ui/src/components/SettingsHarnessCredentials.test.tsx +++ b/spire-ui/src/components/SettingsHarnessCredentials.test.tsx @@ -50,6 +50,18 @@ it('tells a resting key apart from a refused one, and offers the action each nee expect(within(rejected).queryByRole('button', { name: 'Rest longer' })).toBeNull(); }); +// A signed-in seat pays for one build at a time: "In use" is the state a shared key never has (M3.5 part F). +it('shows a subscription as ready, and as in use while a build holds it', async () => { + vi.mocked(api.fetchHarnessCredentials).mockResolvedValue([ + member({ id: 'TEST-seat-free', label: 'TEST-seat-free', type: 'codex', authMode: 'SUBSCRIPTION', leasedUntil: null }), + member({ id: 'TEST-seat-busy', label: 'TEST-seat-busy', type: 'codex', authMode: 'SUBSCRIPTION', leasedUntil: '2099-01-01T00:00:00Z' }), + ]); + render(); + + expect(within(await row('TEST-seat-free')).getByText('Ready · subscription')).toBeInTheDocument(); + expect(within(await row('TEST-seat-busy')).getByText('In use')).toBeInTheDocument(); +}); + it('switches a member off and back on, and says which one changed', async () => { // The list is read again after the change, so the second answer is the switched-off row. vi.mocked(api.fetchHarnessCredentials).mockResolvedValueOnce([member()]).mockResolvedValue([member({ enabled: false })]); diff --git a/spire-ui/src/components/SettingsHarnessCredentials.tsx b/spire-ui/src/components/SettingsHarnessCredentials.tsx index 23ada2a22..b64d61575 100644 --- a/spire-ui/src/components/SettingsHarnessCredentials.tsx +++ b/spire-ui/src/components/SettingsHarnessCredentials.tsx @@ -21,10 +21,10 @@ function state(member: HarnessCredentialView): { label: string; tone: string } { if (member.rejectedAt) return { label: 'Rejected', tone: 'chip danger' }; if (member.rateLimitedUntil && new Date(member.rateLimitedUntil) > new Date()) return { label: 'Resting', tone: 'chip warn' }; if (!member.enabled) return { label: 'Switched off', tone: 'chip' }; - // Before "Available", because it is not. A subscription is stored and safe, and no run can use it - // until the factory can select, inject and bill one. Rendering it as ready told the operator the - // factory was finished and left them to discover otherwise at the first build. - if (member.authMode === 'SUBSCRIPTION') return { label: 'Not usable yet', tone: 'chip warn' }; + // A signed-in seat serves one build at a time; "In use" is the state a shared key never has. + if (member.authMode === 'SUBSCRIPTION' && member.leasedUntil && new Date(member.leasedUntil) > new Date()) + return { label: 'In use', tone: 'chip warn' }; + if (member.authMode === 'SUBSCRIPTION') return { label: 'Ready · subscription', tone: 'chip ok' }; return { label: 'Available', tone: 'chip ok' }; } @@ -98,8 +98,8 @@ export default function SettingsHarnessCredentials() { load rather than burning one window.

- A Codex subscription can be signed in and is kept safely, but no run can use one yet: choosing - it, handing it to a run and recording its cost as zero are not built. Every run uses an API key. + A signed-in Codex subscription pays for builds whose setup says Pay with: subscription, one build + at a time. A build on it costs nothing per token; its token counts are still recorded.

{signingIn && ( @@ -110,8 +110,7 @@ export default function SettingsHarnessCredentials() { // Says what actually happened. "Runs can now be paid by the subscription" was false: // nothing selects, injects or bills one yet, and the credential is deliberately // unreachable by every run until all three exist. - reload(`Saved ${label}. It is kept safely, and no run can use it yet — paying with a` - + ' subscription is not built. Runs keep using an API key.'); + reload(`Saved ${label}. Builds whose setup pays with a subscription can use it now.`); }} /> )} diff --git a/spire-ui/src/components/repositories/factory/BuildModelFields.tsx b/spire-ui/src/components/repositories/factory/BuildModelFields.tsx index d556d2e06..bac89f8b4 100644 --- a/spire-ui/src/components/repositories/factory/BuildModelFields.tsx +++ b/spire-ui/src/components/repositories/factory/BuildModelFields.tsx @@ -1,7 +1,7 @@ import type { LlmModelView } from '../../../api'; import { TOKEN_TYPE_LABEL, unpricedTypesFor } from '../../../llmPricing'; import SettingField from '../../SettingField'; -import type { HarnessModels } from './buildDefaultsApi'; +import type { HarnessModels, PayWith } 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 = { @@ -24,8 +24,10 @@ export interface ModelChoice { value: string; label: string; blocked: string | n * 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]) : []); + reported: Record, payWith: PayWith = 'API_KEY'): ModelChoice[] { + // A subscription is not priced per token (M3.5 part F): no model is held back for a missing rate. + const subscription = payWith === 'SUBSCRIPTION'; + const unpriced = (model: LlmModelView) => (harness && !subscription ? unpricedTypesFor(model, reported[harness]) : []); if (known?.status === 'OK') { return known.offered.map(model => { const price = priced.find(entry => entry.name === model.slug); @@ -35,7 +37,7 @@ export function modelChoices(harness: string, known: HarnessModels | undefined, 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' + blocked: subscription ? null : !price ? 'no price yet — add it in Settings → LLM' : missing.length ? `no price for ${missingLabel(missing)}` : null, }; }); diff --git a/spire-ui/src/components/repositories/factory/BuildStep.tsx b/spire-ui/src/components/repositories/factory/BuildStep.tsx index 7ec90ea41..b7f7698fc 100644 --- a/spire-ui/src/components/repositories/factory/BuildStep.tsx +++ b/spire-ui/src/components/repositories/factory/BuildStep.tsx @@ -3,7 +3,7 @@ import { fetchLlmModels, type LlmModelView } from '../../../api'; import SettingField from '../../SettingField'; import BuildModelFields, { modelChoices } from './BuildModelFields'; import FactoryStep from './FactoryStep'; -import { buildOptions, repositoryBranchHead, saveBuildDefaults, type BuildDefaults, type HarnessModels } from './buildDefaultsApi'; +import { buildOptions, repositoryBranchHead, saveBuildDefaults, type BuildDefaults, type HarnessModels, type PayWith } from './buildDefaultsApi'; interface Props { repositoryId: string; @@ -23,12 +23,15 @@ interface Props { * here once, they are offered as what this deployment can actually run, and the same refusals arrive * where they can be fixed. */ +/** How a build pays, in the words the setup shows. */ +const PAY_WITH_LABEL: Record = { API_KEY: 'an API key', SUBSCRIPTION: 'a Codex subscription' }; + export default function BuildStep({ repositoryId, defaults, open, setOpen, changed, reload }: Props) { const live = useRef(true); useEffect(() => { live.current = true; return () => { live.current = false; }; }, []); const [form, setForm] = useState({ baseBranch: defaults.baseBranch ?? '', harness: defaults.harness ?? '', model: defaults.model ?? '', - effort: defaults.effort ?? '', + effort: defaults.effort ?? '', payWith: (defaults.payWith ?? 'API_KEY') as PayWith, }); const [choices, setChoices] = useState<{ harnesses: string[]; models: LlmModelView[]; reportedTypes: Record; harnessModels: Record }>({ harnesses: [], models: [], reportedTypes: {}, harnessModels: {} }); @@ -62,7 +65,7 @@ export default function BuildStep({ repositoryId, defaults, open, setOpen, chang try { // 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}.`); + if (live.current) changed(`Build setup saved: ${form.harness} on ${form.model}${form.effort ? ` (${form.effort})` : ''}, paid with ${PAY_WITH_LABEL[form.payWith]}, starting from ${form.baseBranch}.`); } catch (failure) { if (live.current) setError(String(failure instanceof Error ? failure.message : failure)); } finally { if (live.current) setBusy(null); } } @@ -70,7 +73,7 @@ export default function BuildStep({ repositoryId, defaults, open, setOpen, chang // 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 known = choices.harnessModels[form.harness]; - const offered = modelChoices(form.harness, known, choices.models, choices.reportedTypes); + const offered = modelChoices(form.harness, known, choices.models, choices.reportedTypes, form.payWith); 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. @@ -83,7 +86,7 @@ export default function BuildStep({ repositoryId, defaults, open, setOpen, chang actions={!editing && }> {defaults.revision > 0 - ?
{defaults.harness} · {defaults.model}{defaults.effort ? ` · ${defaults.effort}` : ''}starts from {defaults.baseBranch}
+ ?
{defaults.harness} · {defaults.model}{defaults.effort ? ` · ${defaults.effort}` : ''}starts from {defaults.baseBranch} · paid with {PAY_WITH_LABEL[(defaults.payWith ?? 'API_KEY') as PayWith]}
:

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 &&
@@ -102,6 +105,12 @@ export default function BuildStep({ repositoryId, defaults, open, setOpen, chang {choices.harnesses.map(harness => )} {form.harness && !choices.harnesses.includes(form.harness) && } + + setForm(previous => ({ ...previous, model }))} setEffort={effort => setForm(previous => ({ ...previous, effort }))} /> diff --git a/spire-ui/src/components/repositories/factory/RepositoryFactory.test.tsx b/spire-ui/src/components/repositories/factory/RepositoryFactory.test.tsx index a19a134f9..b7cf3c215 100644 --- a/spire-ui/src/components/repositories/factory/RepositoryFactory.test.tsx +++ b/spire-ui/src/components/repositories/factory/RepositoryFactory.test.tsx @@ -238,11 +238,31 @@ describe('build setup', () => { fireEvent.click(screen.getByRole('button', { name: 'Save build setup' })); await waitFor(() => expect(build.saveBuildDefaults).toHaveBeenCalledWith(repository.id, // 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 })); + { expectedRevision: 0, baseBranch: 'main', harness: 'codex', model: 'TEST-model', effort: null, payWith: 'API_KEY' })); expect(await screen.findByText(/Build setup saved: codex on TEST-model/)).toBeInTheDocument(); }); /** A model with no output price is refused at dispatch, so it cannot be chosen here either. */ + // A subscription is not priced per token (M3.5 part F): an unpriced model can be picked and saved. + it('pays with a subscription without needing a price for the model', async () => { + const types = ['INPUT', 'CACHED_INPUT', 'CACHE_WRITE', 'OUTPUT', 'REASONING']; + vi.mocked(build.buildOptions).mockResolvedValue({ harnesses: ['codex'], reportedTypes: { codex: types }, + models: { codex: { status: 'OK', offered: [{ slug: 'TEST-unpriced-only', displayName: 'TEST unpriced only', + defaultEffort: 'medium', efforts: ['medium'], visible: true, priority: 1 }] } } }); + renderFactory(); + await open(); + fireEvent.change(await screen.findByLabelText('Base branch', field), { target: { value: 'main' } }); + fireEvent.change(await screen.findByLabelText('Harness', field), { target: { value: 'codex' } }); + fireEvent.change(screen.getByRole('combobox', { name: 'Pay with' }), { target: { value: 'SUBSCRIPTION' } }); + + expect(await screen.findByRole('option', { name: 'TEST unpriced only (TEST-unpriced-only)' })).toBeEnabled(); + fireEvent.change(screen.getByRole('combobox', { name: 'Model' }), { target: { value: 'TEST-unpriced-only' } }); + fireEvent.click(screen.getByRole('button', { name: 'Save build setup' })); + + await waitFor(() => expect(build.saveBuildDefaults).toHaveBeenCalledWith(repository.id, expect.objectContaining({ + model: 'TEST-unpriced-only', payWith: 'SUBSCRIPTION' }))); + }); + // Every option disabled: the select ignores clicks and keys, which the operator read as broken. it('says why no model can be picked when every one lacks a price', async () => { const types = ['INPUT', 'CACHED_INPUT', 'CACHE_WRITE', 'OUTPUT', 'REASONING']; @@ -356,7 +376,7 @@ describe('build setup', () => { 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 })); + { expectedRevision: 2, baseBranch: 'main', harness: 'codex', model: 'TEST-model', effort: null, payWith: 'API_KEY' })); }); // A level belongs to one harness's list: switching away and back must not bring it back unseen. diff --git a/spire-ui/src/components/repositories/factory/buildDefaultsApi.ts b/spire-ui/src/components/repositories/factory/buildDefaultsApi.ts index 624fe2608..965067d26 100644 --- a/spire-ui/src/components/repositories/factory/buildDefaultsApi.ts +++ b/spire-ui/src/components/repositories/factory/buildDefaultsApi.ts @@ -9,9 +9,13 @@ export interface BuildDefaults { model: string | null; /** The thinking level, or null for the model's own default. Absent from an older server. */ effort?: string | null; + /** How builds pay: per token with an API key, or on a signed-in subscription. Absent from an older server. */ + payWith?: PayWith | null; updatedBy: string | null; updatedAt: string | null; } +export type PayWith = 'API_KEY' | 'SUBSCRIPTION'; + /** One model a harness can run, as its agent image declares it. */ export interface HarnessModel { slug: string; @@ -61,7 +65,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; effort: string | null }) => +export const saveBuildDefaults = (repository: string, input: { expectedRevision: number; baseBranch: string; harness: string; model: string; effort: string | null; payWith: PayWith }) => 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)}`); diff --git a/spire-ui/src/components/work-items/workReasons.ts b/spire-ui/src/components/work-items/workReasons.ts index a5281ffaa..8ed70d910 100644 --- a/spire-ui/src/components/work-items/workReasons.ts +++ b/spire-ui/src/components/work-items/workReasons.ts @@ -22,6 +22,10 @@ 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.', + subscription_not_signed_in: 'No Codex subscription is signed in for this harness. Sign one in under Settings → Harness credentials, or pay with an API key.', + subscription_unavailable: 'Every signed-in subscription is busy with another build, switched off or refused. The build was not started; it can start once one is free.', + subscription_unreadable: 'The stored subscription sign-in could not be read. Sign in again under Settings → Harness credentials.', + pay_with_invalid: 'Choose how builds pay: an API key or a subscription.', 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.', From 04917068cd3146402047954794cbf7ad3843956b Mon Sep 17 00:00:00 2001 From: Artjoms Stukans Date: Fri, 25 Sep 2026 23:39:25 +0200 Subject: [PATCH 2/8] Keep how a run paid through a retry, and free a missed seat - factory_run.paid_by (V84) records how a run pays. A re-armed dispatch clears the credential on purpose, so charging no longer reads the payment through it; a subscription retry was priced as an API-key run. A retry that pays another way is refused like any other differing component. - A dispatch the broker definitely missed is assembled again under the same run id; that run now gets its own seat back instead of being refused by the lease it never used. - Refresh tokens inside arrays are emptied too. --- .../factory/FactoryRunProjection.java | 60 ++++++++++++++++--- .../factory/HarnessCredentialPool.java | 6 +- .../orchestrator/factory/RunCharges.java | 9 +-- .../orchestrator/factory/SignInFiles.java | 13 +++- .../orchestrator/factory/WorkRunAssembly.java | 3 +- .../db/migration/V84__run_paid_by.sql | 8 +++ .../factory/FactoryRunProjectionTest.java | 30 ++++++++++ .../factory/HarnessSubscriptionLeaseTest.java | 13 ++++ .../orchestrator/factory/RunChargesTest.java | 30 ++++------ .../orchestrator/factory/SignInFilesTest.java | 12 ++++ .../factory/WorkRunChargeProof.java | 2 + .../work/WorkRunDispatchTest.java | 2 + 12 files changed, 151 insertions(+), 37 deletions(-) create mode 100644 spire-orchestrator/src/main/resources/db/migration/V84__run_paid_by.sql diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/FactoryRunProjection.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/FactoryRunProjection.java index 4bf499029..0b2b64b93 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/FactoryRunProjection.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/FactoryRunProjection.java @@ -148,6 +148,25 @@ public Optional modelOf(String runId) { * The caller must treat both the same way and mark nothing: guessing a member here would take * a working key out of rotation on the strength of a run that never used it. */ + /** + * How a run paid: {@code API_KEY} or {@code SUBSCRIPTION}, empty for a run this deployment never + * queued. Unlike the credential, a re-arm cannot clear it. + * + * @throws IllegalStateException when the row cannot be read. Not "empty": an unreadable row read as + * an API key would price a subscription run at a key's rates. + */ + public Optional paidByOf(String runId) { + try (Connection c = dataSource.getConnection(); + PreparedStatement ps = c.prepareStatement("SELECT paid_by FROM factory_run WHERE run_id = ?")) { + ps.setString(1, runId); + try (ResultSet rs = ps.executeQuery()) { + return rs.next() ? Optional.of(rs.getString("paid_by")) : Optional.empty(); + } + } catch (SQLException e) { + throw new IllegalStateException("How run " + runId + " paid could not be read", e); + } + } + public Optional harnessCredentialOf(String runId) { String sql = "SELECT harness_credential_id FROM factory_run WHERE run_id = ?"; try (Connection c = dataSource.getConnection(); PreparedStatement ps = c.prepareStatement(sql)) { @@ -371,11 +390,34 @@ private static Instant instant(ResultSet rs, String column) throws SQLException * Deliberately NOT part of the re-arm comparison: a retry may * legitimately draw a different member, which is the rotation working * rather than a different request. + * @param paidBy {@code API_KEY} or {@code SUBSCRIPTION}: how the run pays, which decides + * how it is charged. Kept apart from the credential because a re-arm + * clears that and must not change this (M3.5 part F). */ public record QueuedRun(String runId, String harness, String model, String baseBranch, String baseCommit, String branch, String pushedAs, UUID harnessCredentialId, String kind, String reviewId, - String findingRef, String commentId) { + String findingRef, String commentId, String paidBy) { + + public QueuedRun { + paidBy = dev.codespire.contract.work.PayWith.normalise(paidBy); + } + + /** Every run before M3.5 part F paid with an API key. */ + public QueuedRun(String runId, String harness, String model, String baseBranch, + String baseCommit, String branch, String pushedAs, + UUID harnessCredentialId, String kind, String reviewId, + String findingRef, String commentId) { + this(runId, harness, model, baseBranch, baseCommit, branch, pushedAs, + harnessCredentialId, kind, reviewId, findingRef, commentId, null); + } + + /** The same row, paid by a signed-in subscription seat. */ + public QueuedRun paidBySubscription() { + return new QueuedRun(runId, harness, model, baseBranch, baseCommit, branch, pushedAs, + harnessCredentialId, kind, reviewId, findingRef, commentId, + dev.codespire.contract.work.PayWith.SUBSCRIPTION); + } /** A build run: what every dispatch was before M2, and what the REST endpoint still sends. */ public QueuedRun(String runId, String harness, String model, String baseBranch, @@ -416,7 +458,7 @@ public QueuedRun asFixFor(String reviewId, String findingRef, String commentId) + "it, or a redelivery of that comment buys a second run with no symptom"); } return new QueuedRun(runId, harness, model, baseBranch, baseCommit, branch, pushedAs, - harnessCredentialId, RunKind.FIX.name(), reviewId, findingRef, commentId); + harnessCredentialId, RunKind.FIX.name(), reviewId, findingRef, commentId, paidBy); } } @@ -497,8 +539,8 @@ public boolean queued(QueuedRun row, String taskSummary, UUID repositoryId) { INSERT INTO factory_run (run_id, provider_type, workspace, slug, subject, attempt, status, harness, model, base_branch, base_commit, branch, pushed_as, harness_credential_id, kind, review_id, finding_ref, - comment_id, task_summary, repository_id) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + comment_id, task_summary, repository_id, paid_by) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) ON CONFLICT (run_id) DO UPDATE -- The credential is NULLED on a re-arm, not carried and not overwritten, and this -- is a correctness rule rather than tidiness. The re-arm exists because the FIRST @@ -523,7 +565,10 @@ -- the feedback class states as its own rule. SET status = EXCLUDED.status, failure_cause = NULL, failure_detail = NULL, ended_at = NULL, harness_credential_id = NULL, task_summary = EXCLUDED.task_summary - WHERE factory_run.status = ? AND factory_run.failure_cause = ? + WHERE factory_run.status = ? + -- How the run pays is part of what it IS: a retry that pays another way is a + -- different request, refused like any other differing component. + AND factory_run.paid_by = EXCLUDED.paid_by AND factory_run.failure_cause = ? AND factory_run.harness = EXCLUDED.harness AND factory_run.model = EXCLUDED.model AND factory_run.base_branch = EXCLUDED.base_branch AND factory_run.base_commit = EXCLUDED.base_commit AND factory_run.branch = EXCLUDED.branch @@ -568,8 +613,9 @@ -- the feedback class states as its own rule. // column, and a finished run knows only a branch name and a list of paths. ps.setString(19, taskSummary); ps.setObject(20, repositoryId); - ps.setString(21, FAILED); - ps.setString(22, DISPATCH_FAILED); + ps.setString(21, row.paidBy()); + ps.setString(22, FAILED); + ps.setString(23, DISPATCH_FAILED); // 1 on insert and on a re-arm; 0 when ON CONFLICT matched a row the WHERE declined to // touch. That 0 used to be discarded, and the dispatch went ahead anyway. changed = ps.executeUpdate() == 1; diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCredentialPool.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCredentialPool.java index 754a2eb9d..4ea0319f0 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCredentialPool.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCredentialPool.java @@ -227,7 +227,10 @@ public Optional selectSubscription(String harness, String runId, Ins AND type = ? AND rejected_at IS NULL AND (rate_limited_until IS NULL OR rate_limited_until <= now()) - AND (leased_until IS NULL OR leased_until <= now()) + -- A lease this same run already holds is its own: a dispatch the broker + -- definitely missed is assembled again under the same run id, and must not + -- be locked out by the seat it never used. + AND (leased_until IS NULL OR leased_until <= now() OR leased_by_run = ?) ORDER BY exhausted_at NULLS FIRST, last_used_at NULLS FIRST LIMIT 1 FOR UPDATE SKIP LOCKED) @@ -237,6 +240,7 @@ public Optional selectSubscription(String harness, String runId, Ins ps.setString(1, runId); ps.setTimestamp(2, java.sql.Timestamp.from(leasedUntil)); ps.setString(3, harness); + ps.setString(4, runId); try (ResultSet rs = ps.executeQuery()) { return rs.next() ? Optional.of(decrypt(rs.getObject("id", UUID.class), rs)) : Optional.empty(); } diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/RunCharges.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/RunCharges.java index 7043a2e37..f0a74e114 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/RunCharges.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/RunCharges.java @@ -66,10 +66,6 @@ public class RunCharges { @Inject LlmModelPricer pricer; - /** How the run's credential paid: the pricing rule, where the model used to be (M3.5 part F). */ - @Inject - HarnessCredentialPool pool; - /** * The largest self-reported usage this deployment will PRICE for one run. * @@ -109,9 +105,8 @@ public void record(RunResult result) { Optional credential = runs.harnessCredentialOf(runId); // Priced by how the run PAID, not by its model: a subscription run's tokens cost nothing per // token, and the same model's API-key run must still be priced (M3.5 part F, design §5.7). - // A lambda, not pool::authModeOf: a bound method reference dereferences pool at once, so a run - // with no credential at all would fail on a pool it never needed. - boolean subscription = credential.flatMap(id -> pool.authModeOf(id)) + // Read from the run, not through its credential: a re-armed dispatch clears the credential. + boolean subscription = runs.paidByOf(runId) .filter(dev.codespire.contract.work.PayWith.SUBSCRIPTION::equals).isPresent(); List lines = subscription ? pricer.priceUnmetered(usage) : pricer.priceCall(model, usage); String credentialRef = credential.map(UUID::toString).orElse(null); diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/SignInFiles.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/SignInFiles.java index 50d9c9467..2f28054c8 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/SignInFiles.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/SignInFiles.java @@ -3,6 +3,7 @@ import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.node.ArrayNode; import com.fasterxml.jackson.databind.node.ObjectNode; import java.util.Iterator; @@ -41,11 +42,17 @@ static String forAgent(String storedFile) { } } - private static void emptyRefreshTokens(ObjectNode node) { - for (Iterator> fields = node.properties().iterator(); fields.hasNext(); ) { + /** Every level, arrays included: the vendor may add a list of sessions, each with its own token. */ + private static void emptyRefreshTokens(JsonNode node) { + if (node instanceof ArrayNode array) { + array.forEach(SignInFiles::emptyRefreshTokens); + return; + } + if (!(node instanceof ObjectNode object)) return; + for (Iterator> fields = object.properties().iterator(); fields.hasNext(); ) { Map.Entry field = fields.next(); if (field.getKey().equals(REFRESH_TOKEN)) field.setValue(JSON.getNodeFactory().textNode("")); - else if (field.getValue() instanceof ObjectNode child) emptyRefreshTokens(child); + else emptyRefreshTokens(field.getValue()); } } } 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 0d6fc638f..86682e620 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 @@ -81,7 +81,8 @@ public Prepared assemble(Connection c,WorkSourceRegistry.Source source,WorkItemE if(subscription)command=command.paidBySignIn(); 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(),member.id())); + var row=new FactoryRunProjection.QueuedRun(id,in.harness(),in.model(),in.baseBranch(),in.baseCommit(),branch,account.botUsername(),member.id()); + return new Prepared(held,subscription?row.paidBySubscription():row); } /** diff --git a/spire-orchestrator/src/main/resources/db/migration/V84__run_paid_by.sql b/spire-orchestrator/src/main/resources/db/migration/V84__run_paid_by.sql new file mode 100644 index 000000000..3b33b3a07 --- /dev/null +++ b/spire-orchestrator/src/main/resources/db/migration/V84__run_paid_by.sql @@ -0,0 +1,8 @@ +-- How a run paid, on the run itself (M3.5 part F, design section 5.7). +-- +-- Charging used to read it through harness_credential_id, but a re-armed dispatch NULLS that column on +-- purpose (FactoryRunProjection.queued explains why), so a subscription retry was priced as if an API +-- key had paid. How a run pays is fixed by its approval and does not change on a retry, so it lives +-- here, where a re-arm cannot erase it. Every existing run paid with an API key. +ALTER TABLE factory_run ADD COLUMN paid_by VARCHAR(16) NOT NULL DEFAULT 'API_KEY'; +ALTER TABLE factory_run ADD CONSTRAINT factory_run_paid_by_known CHECK (paid_by IN ('API_KEY', 'SUBSCRIPTION')); diff --git a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/FactoryRunProjectionTest.java b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/FactoryRunProjectionTest.java index d4d01b60e..162dd4f4f 100644 --- a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/FactoryRunProjectionTest.java +++ b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/FactoryRunProjectionTest.java @@ -376,6 +376,36 @@ void aRunNamesTheCredentialThatPaidForIt() { assertEquals("API_KEY", view.credentialAuthMode()); } + /** + * A re-arm clears the credential on purpose, but not how the run paid: charging reads that, so a + * subscription retry is never priced as an API-key run (review of PR #178). + */ + @Test + void aReArmKeepsHowTheRunPaid() { + String runId = "run::github:TEST-acme/app:subject-" + UUID.randomUUID() + ":1"; + var row = new FactoryRunProjection.QueuedRun(runId, "codex", "gpt-5.6", "main", "abc1234", + "spire/TEST-paid", null, null).paidBySubscription(); + assertTrue(projection.queued(row, "TEST summary", null)); + projection.dispatchFailed(runId, "TEST: the broker did not answer"); + + assertTrue(projection.queued(row, "TEST summary", null), "the same request re-arms"); + + assertEquals(Optional.of("SUBSCRIPTION"), projection.paidByOf(runId)); + } + + /** A retry that pays another way is a different request, refused like any other differing component. */ + @Test + void aReArmThatPaysAnotherWayIsRefused() { + String runId = "run::github:TEST-acme/app:subject-" + UUID.randomUUID() + ":1"; + var row = new FactoryRunProjection.QueuedRun(runId, "codex", "gpt-5.6", "main", "abc1234", + "spire/TEST-paid", null, null); + assertTrue(projection.queued(row, "TEST summary", null)); + projection.dispatchFailed(runId, "TEST: the broker did not answer"); + + assertFalse(projection.queued(row.paidBySubscription(), "TEST summary", null)); + assertEquals(Optional.of("API_KEY"), projection.paidByOf(runId)); + } + /** A run a signed-in seat paid for says so; the screen must not call it an API key billed per token. */ @Test void aRunPaidByASubscriptionSaysSo() throws SQLException { diff --git a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessSubscriptionLeaseTest.java b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessSubscriptionLeaseTest.java index 2cbdaecfa..a14b1280c 100644 --- a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessSubscriptionLeaseTest.java +++ b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessSubscriptionLeaseTest.java @@ -62,6 +62,19 @@ void aLeasedSeatIsNotHandedToASecondRun() throws SQLException { assertTrue(second.isEmpty(), "one sign-in, one agent"); } + /** + * A dispatch the broker definitely missed is assembled again under the same run id; the seat it + * leased and never used is still its own (review of PR #178). + */ + @Test + void theSameRunGetsItsOwnSeatBackAndNoOtherRunDoes() throws SQLException { + UUID seat = seat("TEST-lease-reentrant"); + pool.selectSubscription(HARNESS, "TEST-run-missed", inAnHour()).orElseThrow(); + + assertTrue(pool.selectSubscription(HARNESS, "TEST-run-other", inAnHour()).isEmpty()); + assertEquals(seat, pool.selectSubscription(HARNESS, "TEST-run-missed", inAnHour()).orElseThrow().id()); + } + /** Fenced by run id: a late release from an earlier run cannot free a seat another run is using. */ @Test void onlyTheRunHoldingTheLeaseCanRelease() throws SQLException { diff --git a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/RunChargesTest.java b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/RunChargesTest.java index 0e49faff1..5cb5b31d2 100644 --- a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/RunChargesTest.java +++ b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/RunChargesTest.java @@ -75,6 +75,14 @@ public Optional modelOf(String runId) { public Optional harnessCredentialOf(String runId) { return Optional.ofNullable(credential); } + + String paidBy = "API_KEY"; + + /** How the run paid, answered here for the same reason as the credential above. */ + @Override + public Optional paidByOf(String runId) { + return Optional.ofNullable(paidBy); + } } /** Prices everything at a flat metered rate, so a line's presence is the thing under test. */ @@ -93,20 +101,6 @@ public List priceCall(String model, ModelUsage usage) { private final RecordingLedger ledger = new RecordingLedger(); private final StubRuns runs = new StubRuns(); private final StubPricer pricer = new StubPricer(); - /** - * How the run's credential paid. Overridden on purpose, for the reason StubRuns gives: the real one - * reads the database, and RunCharges would swallow the failure. - */ - private static final class StubPool extends HarnessCredentialPool { - String mode = "API_KEY"; - - @Override - public Optional authModeOf(java.util.UUID id) { - return Optional.of(mode); - } - } - - private final StubPool pool = new StubPool(); private final RunCharges charges = charges(); private RunCharges charges() { @@ -114,7 +108,6 @@ private RunCharges charges() { c.ledger = ledger; c.runs = runs; c.pricer = pricer; - c.pool = pool; // Stated rather than left at the field default. Outside CDI a long field is 0, and these // tests are about what gets charged -- not about a ceiling nobody set. c.maxReportedTokens = RunTokenUsage.UNBOUNDED; @@ -131,8 +124,9 @@ private static RunResult.RunFinished finished(String runId, Map us */ @Test void aSubscriptionRunIsChargedNothingPerTokenWithItsRealCounts() { - runs.credential = java.util.UUID.fromString("00000000-0000-4000-8000-00000000c0de"); - pool.mode = "SUBSCRIPTION"; + // No credential at all: a re-armed dispatch clears it, and the run must still be charged as paid. + runs.credential = null; + runs.paidBy = "SUBSCRIPTION"; charges.record(finished(RUN_ID, Map.of("INPUT", 1200L, "OUTPUT", 340L))); @@ -146,7 +140,7 @@ void aSubscriptionRunIsChargedNothingPerTokenWithItsRealCounts() { @Test void anApiKeyRunOfTheSameModelIsStillPriced() { runs.credential = java.util.UUID.fromString("00000000-0000-4000-8000-00000000c0de"); - pool.mode = "API_KEY"; + runs.paidBy = "API_KEY"; charges.record(finished(RUN_ID, Map.of("INPUT", 1200L))); diff --git a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/SignInFilesTest.java b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/SignInFilesTest.java index abbad0276..b557328c6 100644 --- a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/SignInFilesTest.java +++ b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/SignInFilesTest.java @@ -34,6 +34,18 @@ void theRefreshTokenIsEmptiedAndEverythingElseKept() throws Exception { assertEquals("chatgpt", file.path("auth_mode").asText()); } + /** A token inside a list is still a refresh token (review of PR #178). */ + @Test + void aRefreshTokenInsideAListIsEmptiedToo() throws Exception { + String stored = "{\"sessions\":[{\"refresh_token\":\"TEST-listed-refresh\",\"access_token\":\"TEST-listed-access\"}]}"; + + String forAgent = SignInFiles.forAgent(stored); + + assertFalse(forAgent.contains("TEST-listed-refresh")); + assertEquals("", JSON.readTree(forAgent).path("sessions").path(0).path("refresh_token").asText()); + assertTrue(forAgent.contains("TEST-listed-access")); + } + @Test void storedBytesThatAreNotAJsonObjectAreRefusedWithoutQuotingThem() { IllegalStateException refused = assertThrows(IllegalStateException.class, diff --git a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/WorkRunChargeProof.java b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/WorkRunChargeProof.java index dfd380491..d8c511df6 100644 --- a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/WorkRunChargeProof.java +++ b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/WorkRunChargeProof.java @@ -42,6 +42,8 @@ public static void main(String[] args) throws Exception { charges.runs=new FactoryRunProjection(){ @Override public Optional modelOf(String id){return Optional.of("TEST-metered-model");} @Override public Optional harnessCredentialOf(String id){return Optional.empty();} + // Overridden for the trap RunChargesTest names: the real one reads a row this proof never wrote. + @Override public Optional paidByOf(String id){return Optional.of("API_KEY");} @Override protected void push(String id){ /* TEST: no dashboard websocket */ } }; charges.pricer=new LlmModelPricer(){@Override public List priceCall(String model,ModelUsage usage){ 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 3671927c3..22f7ebc8b 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 @@ -89,6 +89,8 @@ private void codexRuns(String slug,String... levels) { assertTrue(handedOver.contains("TEST-access")); assertEquals(1,count("SELECT count(*) FROM factory_run WHERE run_id=? AND harness_credential_id=?",command.runId(),seat), "the run names the seat that paid"); + assertEquals(1,count("SELECT count(*) FROM factory_run WHERE run_id=? AND paid_by='SUBSCRIPTION'",command.runId()), + "the run records how it paid, where a re-arm cannot erase it"); assertTrue(pool.list().stream().anyMatch(member->member.id().equals(seat)&&member.leasedUntil()!=null),"the seat is leased"); } finally { pool.remove(seat); } } From b1525f7c0cc1f6ba89ee58bf75c288e6e1895cda Mon Sep 17 00:00:00 2001 From: Artjoms Stukans Date: Sat, 26 Sep 2026 01:01:50 +0200 Subject: [PATCH 3/8] Free a seat only when its agent is confirmed stopped A failed run can leave its agent running when a stop does not take, and a queued command could start after its seat went to another build, so two agents could share one sign-in. - The lease has two phases. Until the agent starts it is time-bound, and the command carries signInStartBy: the worker refuses a sign-in build it picks up later. Once RunStarted arrives the time bound is removed. - RunAgentStopped is a new run result, sent only when the runtime (RunRuntime.agentRunning; Docker reads the agent container's state) confirms no agent process runs: after the build, on recovery, on a takeover hold, and once per unit from the watchdog. It is the only thing that frees a seat. A runtime that cannot tell keeps it held. - An assembly that fails after leasing frees the seat at once. Settings gains "Free seat" for a worker that died for good. - One seat per account: account_ref is filled from tokens.account_id, an enabled seat's account is unique (V85), and signing in again to an account replaces that seat's file. Older seats are identified at startup; duplicates are switched off. - Every run-worker log record passes a filter that scrubs the secrets of every run and sign-in the worker holds, in the message and in each exception of the chain. - The standalone dispatch path refuses a sign-in command. --- docs/UNVERIFIED.md | 3 +- ...actory-m35-one-ticket-to-a-build-design.md | 30 +++- .../contract/command/RunCommand.java | 39 +++-- .../codespire/contract/event/RunResult.java | 20 ++- .../command/ExecuteRunBranchModeTest.java | 3 +- .../src/test/resources/contract-schema.txt | 3 +- .../factory/FactoryRunProjection.java | 4 + .../factory/HarnessCredentialPool.java | 162 ++++++++++++++++-- .../factory/HarnessCredentialResource.java | 24 ++- .../orchestrator/factory/HarnessSignIns.java | 18 ++ .../orchestrator/factory/RunResultSaga.java | 15 +- .../orchestrator/factory/RunTokenUsage.java | 2 + .../orchestrator/factory/SignInFiles.java | 13 ++ .../orchestrator/factory/WorkRunAssembly.java | 35 +++- .../migration/V85__one_seat_per_account.sql | 13 ++ .../factory/BuildDefaultsTest.java | 2 +- .../factory/FactoryRunProjectionTest.java | 2 +- .../factory/HarnessSignInsTest.java | 47 ++++- .../factory/HarnessSubscriptionLeaseTest.java | 106 +++++++++++- .../factory/RunResultSagaTest.java | 30 +++- .../work/WorkPreparationSweepTest.java | 2 +- .../work/WorkRunDispatchTest.java | 35 +++- .../runworker/HarnessSignInWorker.java | 8 + .../dev/codespire/runworker/LiveSecrets.java | 64 +++++++ .../codespire/runworker/OrphanWatchdog.java | 21 +++ .../codespire/runworker/RunDispatcher.java | 19 +- .../runworker/RunResultReporter.java | 19 ++ .../codespire/runworker/SecretLogFilter.java | 72 ++++++++ .../codespire/runworker/WorkRunWorker.java | 19 ++ .../src/main/resources/application.yml | 3 + .../runworker/OrphanWatchdogTest.java | 40 +++++ .../runworker/RunDispatcherTest.java | 19 ++ .../codespire/runworker/RunLauncherTest.java | 2 +- .../runworker/RunUnitBuilderTest.java | 2 +- .../runworker/SecretLogFilterTest.java | 81 +++++++++ .../runworker/WorkRunWorkerTest.java | 55 +++++- .../runtime/docker/DockerRunRuntime.java | 19 ++ .../runtime/docker/DockerAgentStateTest.java | 29 ++++ .../dev/codespire/runtime/RunRuntime.java | 17 ++ spire-ui/src/api.ts | 7 +- .../components/HarnessSubscriptionSignIn.tsx | 1 + .../SettingsHarnessCredentials.test.tsx | 42 ++++- .../components/SettingsHarnessCredentials.tsx | 37 +++- 43 files changed, 1093 insertions(+), 91 deletions(-) create mode 100644 spire-orchestrator/src/main/resources/db/migration/V85__one_seat_per_account.sql create mode 100644 spire-run-worker/src/main/java/dev/codespire/runworker/LiveSecrets.java create mode 100644 spire-run-worker/src/main/java/dev/codespire/runworker/SecretLogFilter.java create mode 100644 spire-run-worker/src/test/java/dev/codespire/runworker/SecretLogFilterTest.java create mode 100644 spire-runtime-docker/src/test/java/dev/codespire/runtime/docker/DockerAgentStateTest.java diff --git a/docs/UNVERIFIED.md b/docs/UNVERIFIED.md index 31f800f07..fb56ad524 100644 --- a/docs/UNVERIFIED.md +++ b/docs/UNVERIFIED.md @@ -473,7 +473,8 @@ Each has a runbook mode. None has been run by an operator. | 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 | | **Codex CLI 0.156.1 in the agent image** (2026-09-23) | none yet | Raised from 0.146.0 so the model list includes the gpt-6 models. Re-checked on 0.156.1: every flag the adapter passes, the API-key login, and the shape of the file it writes (`auth_mode=apikey`). The device sign-in output was then proved by a real sign-in (2026-09-25), and one live `codex exec` on a subscription produced the same `--json` event types and the same five usage buckets. Still NOT re-checked: a multi-turn run with tool calls, which exercises the rest of the parser | | **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 | -| **A build paid by a Codex subscription** (M3.5 part F, 2026-09-25) | none yet | Tests prove the lease, the emptied refresh token, the unmetered charge lines and the file the adapter writes. One live `codex exec` accepted a file with its refresh token emptied, three hours after sign-in with the id token expired. No item build has run on a seat yet. NOT observed: a run that outlives the access token (10 days — the seat then needs a new sign-in, nothing refreshes it), two builds contending for one seat on a real worker, and whether the vendor's CLI ever tries to refresh mid-run with an empty refresh token | +| **A build paid by a Codex subscription** (M3.5 part F, 2026-09-25) | none yet | Tests prove the two-phase lease, the start deadline, the emptied refresh token, one seat per account, the unmetered charge lines and the file the adapter writes. One live `codex exec` accepted a file with its refresh token emptied, three hours after sign-in with the id token expired. No item build has run on a seat yet. NOT observed: Docker's agent-container state read by `agentRunning` on a real held build, a run that outlives the access token (10 days — the seat then needs a new sign-in), two builds contending for one seat on a real worker, whether the vendor's CLI ever tries to refresh mid-run with an empty refresh token, and whether `tokens.account_id` is stable across sign-ins of one account | +| **Run-worker log lines are scrubbed of run secrets** (review of PR #178, 2026-09-25) | none yet | `SecretLogFilter` is unit-tested on a JBoss log record and its console configuration is asserted. Not observed: the filter on the JSON console handler of a running worker. And by design it cannot scrub a unit this process did not start: after a worker restart, the watchdog's log lines about an older unit carry that unit's secrets unscrubbed, as `RunFailures` already states for failure details | | **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 21b09a01a..3c64cafa4 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 @@ -271,14 +271,28 @@ takes no sign-in and never replays the agent (`WorkRunWorker.java:91`). Cases th duplicate command delivery, worker death before release, a failed stop, a late release from an old lease, dispatch failure before the container exists, and two uploads of the same sign-in. -- **Built 2026-09-25, and where it differs.** The fence is the **run id**, not a separate - `lease_version`: a run id names one attempt, so a late release from an older run cannot match a newer - run's lease. `RunResultSaga` releases on every run result except `RunStarted` — `RunWorkReady` - included, so a held build frees its seat when the agent stops, not when the item ends. A lease whose - release never arrives expires at wall clock + 10 minutes. **Known gap:** the watchdog can report a - failure before its stop completes (above), and that report releases the seat, so a next run can - briefly share it with a dying agent. Tested: two runs contending, the fence, expiry, and a release on - each ending result. Not tested: duplicate delivery and a failed stop on a real worker. +- **Built 2026-09-25, as the review of PR #178 reshaped it.** The fence is the **run id**, not a + separate `lease_version`: a run id names one attempt, so a message from an older run cannot touch a + newer run's lease. The lease has two phases: + - **Before the agent starts** it is time-bound: the command carries `signInStartBy` (assembly + 10 + minutes) and the lease lasts 5 minutes longer. The worker refuses a sign-in build it picks up after + that time (`BAD_COMMAND`), so a command that waited in the queue never starts on a seat that has + meanwhile gone to another build. A build that never starts frees its seat when the time runs out; an + assembly that fails after leasing frees it at once; a dispatch the broker definitely missed gets the + same seat back under the same run id. + - **Once the agent starts** (`RunStarted`) the time bound is removed. Only `RunAgentStopped` frees + the seat — a result the worker sends when the runtime (`RunRuntime.agentRunning`, Docker: the agent + container's state) confirms no agent process runs. An outcome never frees it: a failed run can leave + its agent running when a stop does not take. The worker checks after the build, on recovery, on a + takeover hold, and the watchdog checks every reaped or held unit once. A runtime that cannot tell, + or an agent that may still run, keeps the seat held. + - **An operator's "Free seat"** is the recovery for a worker that died for good, since nothing else + will ever report. It asks the operator to confirm no build still uses the seat. +- **One seat per account.** `account_ref` is filled from the measured `tokens.account_id`, an enabled + seat's account is unique per harness (V85), and a seat with no known account is never leased. Signing + in again to an account that has a seat replaces that seat's file — which is also how a seat whose access + token expired is renewed. Seats stored before this are identified at startup; older duplicates are + switched off and cannot be switched back on beside the newest. ### 5.6 Injection and refresh — the agent never hands a credential back 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 fc33997d5..c365fb73d 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 @@ -81,7 +81,7 @@ record ExecuteRun(String runId, RepoRef repo, String remoteUri, List protectedPaths, long maxWallClockSeconds, String scmCredential, String harnessCredential, boolean existingBranch, String protectedBranch, - String reasoningEffort, boolean harnessSignIn) implements RunCommand { + String reasoningEffort, java.time.Instant signInStartBy) 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 @@ -93,7 +93,7 @@ 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, "", null, false); + harnessCredential, false, "", null, null); } /** @@ -108,12 +108,12 @@ public ExecuteRun(String runId, RepoRef repo, String remoteUri, boolean existingBranch, String protectedBranch) { this(runId, repo, remoteUri, baseBranch, baseCommit, branch, prompt, harness, model, agentImage, protectedPaths, maxWallClockSeconds, scmCredential, - harnessCredential, existingBranch, protectedBranch, null, false); + harnessCredential, existingBranch, protectedBranch, null, null); } /** * Every caller written before subscriptions existed passes an API key (M3.5 part F). A run already - * on the bus decodes with false, which is what every such run carried. + * on the bus decodes with no start-by time, which is what every such run carried. */ public ExecuteRun(String runId, RepoRef repo, String remoteUri, String baseBranch, String baseCommit, String branch, @@ -123,7 +123,7 @@ public ExecuteRun(String runId, RepoRef repo, String remoteUri, boolean existingBranch, String protectedBranch, String reasoningEffort) { this(runId, repo, remoteUri, baseBranch, baseCommit, branch, prompt, harness, model, agentImage, protectedPaths, maxWallClockSeconds, scmCredential, - harnessCredential, existingBranch, protectedBranch, reasoningEffort, false); + harnessCredential, existingBranch, protectedBranch, reasoningEffort, null); } public ExecuteRun { @@ -185,7 +185,7 @@ 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, reasoningEffort, harnessSignIn); + harnessCredential, true, destination, reasoningEffort, signInStartBy); } /** @@ -196,18 +196,31 @@ public ExecuteRun onExistingBranch(String destination) { 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, harnessSignIn); + harnessCredential, existingBranch, protectedBranch, level, signInStartBy); } /** - * The same run, where {@code harnessCredential} is a sealed sign-in file rather than an API key - * (M3.5 part F). The worker needs to know which, because the two are handed to the harness - * differently — and guessing from the bytes is how a key ends up written as a file. + * Whether {@code harnessCredential} is a sealed sign-in file rather than an API key (M3.5 part F). + * The worker needs to know which, because the two are handed to the harness differently — and + * guessing from the bytes is how a key ends up written as a file. */ - public ExecuteRun paidBySignIn() { + public boolean harnessSignIn() { + return signInStartBy != null; + } + + /** + * The same run, paid by a signed-in seat that it must start using by {@code startBy}. + * + *

The seat is leased when the build is assembled, and the lease is only held for a bounded + * time until the agent starts. A command that waits in the queue past that time could start + * after its seat was handed to another build, so the worker refuses it instead (review of + * PR #178). Once the agent starts, the lease holds until the agent is confirmed stopped. + */ + public ExecuteRun paidBySignIn(java.time.Instant startBy) { + Objects.requireNonNull(startBy, "startBy"); return new ExecuteRun(runId, repo, remoteUri, baseBranch, baseCommit, branch, prompt, harness, model, agentImage, protectedPaths, maxWallClockSeconds, scmCredential, - harnessCredential, existingBranch, protectedBranch, reasoningEffort, true); + harnessCredential, existingBranch, protectedBranch, reasoningEffort, startBy); } @Override @@ -224,7 +237,7 @@ public String toString() { + ", protectedPaths=" + protectedPaths + ", maxWallClockSeconds=" + maxWallClockSeconds + ", existingBranch=" + existingBranch - + ", harnessSignIn=" + harnessSignIn + + ", signInStartBy=" + signInStartBy + ", protectedBranch=" + protectedBranch + ", reasoningEffort=" + reasoningEffort + ", promptChars=" + prompt.length() diff --git a/spire-contract/src/main/java/dev/codespire/contract/event/RunResult.java b/spire-contract/src/main/java/dev/codespire/contract/event/RunResult.java index 108f947ff..31af8f5c4 100644 --- a/spire-contract/src/main/java/dev/codespire/contract/event/RunResult.java +++ b/spire-contract/src/main/java/dev/codespire/contract/event/RunResult.java @@ -13,7 +13,8 @@ @JsonSubTypes.Type(value = RunResult.RunStarted.class, name = "RunStarted"), @JsonSubTypes.Type(value = RunResult.RunWorkReady.class, name = "RunWorkReady"), @JsonSubTypes.Type(value = RunResult.RunFinished.class, name = "RunFinished"), - @JsonSubTypes.Type(value = RunResult.RunFailed.class, name = "RunFailed") + @JsonSubTypes.Type(value = RunResult.RunFailed.class, name = "RunFailed"), + @JsonSubTypes.Type(value = RunResult.RunAgentStopped.class, name = "RunAgentStopped") }) public sealed interface RunResult { @@ -26,6 +27,23 @@ record RunStarted(String runId, String providerRunId) implements RunResult { } } + /** + * The run's agent is confirmed not running: its exit was observed, it was stopped, or it never + * started (M3.5 part F). + * + *

Not an outcome — the terminal result says how the run went, and may arrive before or after + * this. It exists because a failure does not prove the agent stopped: a run can be reported failed + * while its unit is preserved and, if the stop did not take, still running. A signed-in seat is + * freed on THIS, never on the outcome, so it is never handed to a second agent while the first + * may still use it. The worker sends it only when it knows; silence keeps the seat held. + */ + record RunAgentStopped(String runId) implements RunResult { + + public RunAgentStopped { + Objects.requireNonNull(runId, "runId"); + } + } + /** * A completed build whose checkpoint remains unpublished. This is not a terminal publication * result. The same measured token map rides the eventual terminal result under the same charge 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 d8c443a03..ab1ce6f4a 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 @@ -50,9 +50,10 @@ void theThinkingLevelSurvivesEveryRebuildAndTheOthersSurviveIt() { /** A sign-in is written as a file and a key is piped into a login; losing the flag swaps the two. */ @Test void aRunPaidBySignInSaysSoThroughEveryRebuild() { - RunCommand.ExecuteRun run = run().paidBySignIn().atEffort("high").onExistingBranch("develop"); + RunCommand.ExecuteRun run = run().paidBySignIn(java.time.Instant.parse("2099-01-01T00:00:00Z")).atEffort("high").onExistingBranch("develop"); assertTrue(run.harnessSignIn()); + assertEquals(java.time.Instant.parse("2099-01-01T00:00:00Z"), run.signInStartBy(), "the deadline survives every rebuild"); assertFalse(run().harnessSignIn(), "every run before part F paid with a key"); } diff --git a/spire-contract/src/test/resources/contract-schema.txt b/spire-contract/src/test/resources/contract-schema.txt index aa50994c1..7953c5adc 100644 --- a/spire-contract/src/test/resources/contract-schema.txt +++ b/spire-contract/src/test/resources/contract-schema.txt @@ -43,13 +43,14 @@ 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, reasoningEffort: java.lang.String, harnessSignIn: boolean) +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, signInStartBy: java.time.Instant) 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) SteerRun(runId: java.lang.String, instruction: java.lang.String) # RunResult +RunAgentStopped(runId: java.lang.String) RunFailed(runId: java.lang.String, cause: java.lang.String, detail: java.lang.String, retryable: boolean, tokenUsage: java.util.Map) RunFinished(runId: java.lang.String, pushedRef: java.lang.String, changedPaths: java.util.List, blocked: java.util.List, tokenUsage: java.util.Map, agentUnobserved: boolean) RunStarted(runId: java.lang.String, providerRunId: java.lang.String) diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/FactoryRunProjection.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/FactoryRunProjection.java index 0b2b64b93..7534751dc 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/FactoryRunProjection.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/FactoryRunProjection.java @@ -786,6 +786,10 @@ public void apply(RunResult result) { case RunResult.RunWorkReady ready -> workReady(ready); case RunResult.RunFinished finished -> finished(finished); case RunResult.RunFailed failed -> failed(failed); + // Not an outcome: the saga frees the run's seat on it and hands it to nothing else. + case RunResult.RunAgentStopped ignored -> { + return; + } } push(result.runId()); } diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCredentialPool.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCredentialPool.java index 4ea0319f0..a7569da43 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCredentialPool.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCredentialPool.java @@ -208,10 +208,13 @@ public Selection select() { /** * Leases a signed-in seat of this harness to one run, and hands out its sign-in file (M3.5 part F). * - *

A lease, where an API key is shared: two agents on one sign-in can each invalidate the other's - * session. The pick and the lease are ONE statement with {@code SKIP LOCKED}, so two dispatches cannot - * take the same seat. A seat whose lease has run out is free again — that bound is what frees a seat - * whose release never arrived. + *

A lease, where an API key is shared: one sign-in serves one agent. The pick and the lease are ONE + * statement with {@code SKIP LOCKED}, so two dispatches cannot take the same seat. + * + *

The lease has two phases. Until the agent starts it runs out at {@code leasedUntil}, which frees a + * seat whose build never reached a worker. Once the agent starts, {@link #holdWhileRunning} removes + * the time bound, and only {@link #releaseLease} — sent when the agent is confirmed stopped — or an + * operator's {@link #freeSeat} ends it. A seat is never freed by a clock while an agent may use it. * * @return the member with its sign-in file as {@code apiKey}, or empty when every seat is in use, * rejected, switched off, or none is signed in. The caller reports that as one refusal. @@ -227,10 +230,13 @@ public Optional selectSubscription(String harness, String runId, Ins AND type = ? AND rejected_at IS NULL AND (rate_limited_until IS NULL OR rate_limited_until <= now()) - -- A lease this same run already holds is its own: a dispatch the broker - -- definitely missed is assembled again under the same run id, and must not - -- be locked out by the seat it never used. - AND (leased_until IS NULL OR leased_until <= now() OR leased_by_run = ?) + -- A seat whose account is unknown could be a second seat on one account. + AND account_ref IS NOT NULL + -- Free: nobody holds it, or a holder that never started ran out of time. A + -- lease with no end is a running agent's and is never taken. A lease this same + -- run already holds is its own: a dispatch the broker definitely missed is + -- assembled again under the same run id, and must not be locked out by it. + AND (leased_by_run IS NULL OR leased_until <= now() OR leased_by_run = ?) ORDER BY exhausted_at NULLS FIRST, last_used_at NULLS FIRST LIMIT 1 FOR UPDATE SKIP LOCKED) @@ -249,6 +255,31 @@ public Optional selectSubscription(String harness, String runId, Ins } } + /** + * The run's agent has started: its lease now lasts until the agent is confirmed stopped, however long + * that takes. Fenced by run id like the release. + */ + public void holdWhileRunning(String runId) { + try (Connection c = dataSource.getConnection(); PreparedStatement ps = c.prepareStatement( + "UPDATE harness_credential SET leased_until = NULL WHERE leased_by_run = ?")) { + ps.setString(1, runId); + ps.executeUpdate(); + } catch (SQLException e) { + throw new IllegalStateException("The lease of run " + runId + " could not be held", e); + } + } + + /** + * An operator frees a seat whose worker will never report — a worker that died for good. Only the + * operator can know no agent still uses it, so nothing calls this on its own. + * + * @return false when the member is not a leased seat + */ + public boolean freeSeat(UUID id) { + return update("UPDATE harness_credential SET leased_by_run = NULL, leased_until = NULL" + + " WHERE id = ? AND auth_mode = 'SUBSCRIPTION' AND leased_by_run IS NOT NULL", id) == 1; + } + /** * Ends the lease this run holds, if it still holds one. Fenced by the run id, which is unique per * attempt: a late release from an earlier run matches nothing and so cannot free a seat another run @@ -392,13 +423,15 @@ public boolean clearRejection(UUID id) { */ public record MemberView(UUID id, String label, String type, String baseUrl, boolean enabled, Instant rateLimitedUntil, Instant rejectedAt, Instant lastUsedAt, - String authMode, Instant leasedUntil) { + String authMode, boolean inUse, boolean identified) { } public List list() { String sql = """ SELECT id, label, type, base_url, enabled, rate_limited_until, rejected_at, last_used_at, - auth_mode, CASE WHEN leased_until > now() THEN leased_until END AS leased_until + auth_mode, + leased_by_run IS NOT NULL AND (leased_until IS NULL OR leased_until > now()) AS in_use, + auth_mode <> 'SUBSCRIPTION' OR account_ref IS NOT NULL AS identified FROM harness_credential ORDER BY label """; List members = new ArrayList<>(); @@ -408,7 +441,8 @@ auth_mode, CASE WHEN leased_until > now() THEN leased_until END AS leased_until members.add(new MemberView(rs.getObject("id", UUID.class), rs.getString("label"), rs.getString("type"), rs.getString("base_url"), rs.getBoolean("enabled"), instant(rs, "rate_limited_until"), instant(rs, "rejected_at"), - instant(rs, "last_used_at"), rs.getString("auth_mode"), instant(rs, "leased_until"))); + instant(rs, "last_used_at"), rs.getString("auth_mode"), rs.getBoolean("in_use"), + rs.getBoolean("identified"))); } return members; } catch (SQLException e) { @@ -443,7 +477,7 @@ INSERT INTO harness_credential (id, label, type, base_url, api_key) ps.setString(4, baseUrl); ps.setString(5, encryption.encryptString(apiKey, aad(id))); ps.executeUpdate(); - return new MemberView(id, label, type, baseUrl, true, null, null, null, "API_KEY", null); + return new MemberView(id, label, type, baseUrl, true, null, null, null, "API_KEY", false, true); } catch (SQLException e) { if ("23505".equals(e.getSQLState())) { throw new DuplicateLabelException(label, e); @@ -479,20 +513,109 @@ public boolean hasLabel(Connection c, String label) throws SQLException { * @return the new member's id */ public UUID addSubscription(Connection c, String label, String type, String body) throws SQLException { + String account = SignInFiles.accountOf(body) + .orElseThrow(() -> new IllegalArgumentException("subscription_unidentified")); UUID id = UUID.randomUUID(); try (PreparedStatement ps = c.prepareStatement(""" - INSERT INTO harness_credential (id, label, type, base_url, api_key, auth_mode) - VALUES (?, ?, ?, '', ?, 'SUBSCRIPTION') + INSERT INTO harness_credential (id, label, type, base_url, api_key, auth_mode, account_ref) + VALUES (?, ?, ?, '', ?, 'SUBSCRIPTION', ?) """)) { ps.setObject(1, id); ps.setString(2, label); ps.setString(3, type); ps.setString(4, encryption.encryptString(body, aad(id))); + ps.setString(5, account); ps.executeUpdate(); } return id; } + /** The enabled seat already signed in to this account, locked for the caller's transaction. */ + public Optional seatFor(Connection c, String type, String account) throws SQLException { + try (PreparedStatement ps = c.prepareStatement(""" + SELECT id FROM harness_credential + WHERE auth_mode = 'SUBSCRIPTION' AND enabled AND type = ? AND account_ref = ? + FOR UPDATE + """)) { + ps.setString(1, type); + ps.setString(2, account); + try (ResultSet rs = ps.executeQuery()) { + return rs.next() ? Optional.of(rs.getObject("id", UUID.class)) : Optional.empty(); + } + } + } + + /** + * A new sign-in to an account that already has a seat replaces that seat's file rather than adding a + * second seat — which is also how a seat whose access token expired is signed in again. A refusal the + * old file earned is cleared: the new file has not been refused. A lease is left alone: the agent + * holding it runs on its own copy. + */ + public void replaceSubscription(Connection c, UUID id, String body) throws SQLException { + try (PreparedStatement ps = c.prepareStatement(""" + UPDATE harness_credential SET api_key = ?, rejected_at = NULL, updated_at = now() WHERE id = ? + """)) { + ps.setString(1, encryption.encryptString(body, aad(id))); + ps.setObject(2, id); + ps.executeUpdate(); + } + } + + /** + * Fills the account of every seat stored before accounts were recorded, from its own file. The newest + * seat of an account keeps it; older seats of the same account are switched off, because two seats on + * one account are two leases on one sign-in. A file that names no account, or cannot be read, stays + * unidentified and is never leased. + * + * @return how many seats were switched off as duplicates + */ + public int identifySeats() { + int switchedOff = 0; + try (Connection c = dataSource.getConnection(); PreparedStatement ps = c.prepareStatement(""" + SELECT id, type, api_key FROM harness_credential + WHERE auth_mode = 'SUBSCRIPTION' AND account_ref IS NULL ORDER BY updated_at DESC + """); ResultSet rs = ps.executeQuery()) { + while (rs.next()) { + UUID id = rs.getObject("id", UUID.class); + Optional account; + try { + account = SignInFiles.accountOf(encryption.decryptString(rs.getString("api_key"), aad(id))); + } catch (RuntimeException unreadable) { + LOG.warnf("subscription seat %s could not be read to find its account; it stays unused", id); + continue; + } + if (account.isEmpty()) continue; + boolean duplicate = seatFor(c, rs.getString("type"), account.orElseThrow()).isPresent(); + try (PreparedStatement set = c.prepareStatement( + "UPDATE harness_credential SET account_ref = ?, enabled = enabled AND NOT ? WHERE id = ?")) { + set.setString(1, account.orElseThrow()); + set.setBoolean(2, duplicate); + set.setObject(3, id); + set.executeUpdate(); + } + if (duplicate) switchedOff++; + } + } catch (SQLException e) { + throw new IllegalStateException("Subscription seats could not be identified", e); + } + if (switchedOff > 0) { + LOG.warnf("%d subscription seat(s) were switched off: another seat is signed in to the same account", + switchedOff); + } + return switchedOff; + } + + void identifyOnStart(@jakarta.enterprise.event.Observes io.quarkus.runtime.StartupEvent started) { + identifySeats(); + } + + /** Raised when an enabled seat already uses this account, so the caller can answer 409. */ + public static class SeatTakenException extends IllegalStateException { + SeatTakenException(UUID id, Throwable cause) { + super("another enabled seat is signed in to the same account as " + id, cause); + } + } + /** * Take a member out of rotation, keeping the row. * @@ -510,8 +633,15 @@ public boolean remove(UUID id) { /** Bring a disabled member back, because disabling is not deletion and must not be one-way. */ public boolean enable(UUID id) { - return update("UPDATE harness_credential SET enabled = TRUE, updated_at = now()" - + " WHERE id = ? AND NOT enabled", id) == 1; + try (Connection c = dataSource.getConnection(); PreparedStatement ps = c.prepareStatement( + "UPDATE harness_credential SET enabled = TRUE, updated_at = now() WHERE id = ? AND NOT enabled")) { + ps.setObject(1, id); + return ps.executeUpdate() == 1; + } catch (SQLException e) { + // One seat per account: switching a second seat of an account back on is refused by name. + if ("23505".equals(e.getSQLState())) throw new SeatTakenException(id, e); + throw new IllegalStateException("The harness credential " + id + " could not be updated", e); + } } private static Instant instant(ResultSet rs, String column) throws SQLException { diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCredentialResource.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCredentialResource.java index 146e211d4..1d6aad2f7 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCredentialResource.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessCredentialResource.java @@ -118,12 +118,34 @@ public Response disable(@PathParam("id") String id) { @Path("/{id}/enable") @Consumes(MediaType.WILDCARD) public Response enable(@PathParam("id") String id) { - if (!pool.enable(uuid(id))) { + boolean enabled; + try { + enabled = pool.enable(uuid(id)); + } catch (HarnessCredentialPool.SeatTakenException taken) { + throw new ClientErrorException(Response.status(Response.Status.CONFLICT) + .entity("subscription_account_taken").type(MediaType.TEXT_PLAIN).build()); + } + if (!enabled) { throw new NotFoundException("no disabled harness credential with id: " + id); } return Response.noContent().build(); } + /** + * Free a seat whose build will never report its agent stopped — the worker running it is gone. + * An operator's call: only a person can know no agent still uses the sign-in. + */ + @POST + @Path("/{id}/free-seat") + @Consumes(MediaType.WILDCARD) + public Response freeSeat(@PathParam("id") String id) { + if (!pool.freeSeat(uuid(id))) { + throw new NotFoundException("no leased subscription seat with id: " + id); + } + LOG.warnf("subscription seat %s was freed by an operator", id); + return Response.noContent().build(); + } + /** * The operator says a refused key works again — a rotated secret, or restored credit. * diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessSignIns.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessSignIns.java index 9f1b31043..4a7b2a643 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessSignIns.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/HarnessSignIns.java @@ -279,6 +279,24 @@ public void completed(HarnessSignInResult.Completed result) { fail(c, id, HarnessSignInResult.Failed.WRONG_MODE); return; } + // One seat per account. Signing in again to an account that has a seat replaces that + // seat's file: it is how an expired seat is renewed, and a second seat would be a second + // lease on the same sign-in. + Optional account = SignInFiles.accountOf(body); + if (account.isEmpty()) { + fail(c, id, "subscription_unidentified"); + return; + } + Optional existing = pool.seatFor(c, pending.get().harness(), account.orElseThrow()); + if (existing.isPresent()) { + pool.replaceSubscription(c, existing.orElseThrow(), body); + try (PreparedStatement ps = c.prepareStatement(""" + UPDATE harness_sign_in SET state='COMPLETE', credential_id=?, updated_at=now() WHERE id=? + """)) { + ps.setObject(1, existing.orElseThrow()); ps.setObject(2, id); ps.executeUpdate(); + } + return; + } if (pool.hasLabel(c, pending.get().label())) { // Taken by an ordinary key while this person was approving. Refusing by name beats // letting the unique constraint throw, because that throw used to be swallowed: diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/RunResultSaga.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/RunResultSaga.java index f32097e73..50009c5bc 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/RunResultSaga.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/RunResultSaga.java @@ -32,7 +32,7 @@ public class RunResultSaga { @Inject FactoryPullRequests pullRequests; - /** Frees the signed-in seat a finished agent held (M3.5 part F). */ + /** Holds and frees the signed-in seat a run's agent uses (M3.5 part F). */ @Inject HarnessCredentialPool pool; @@ -52,10 +52,15 @@ public void on(RunResult result) { MDC.put(MDC_RUN_ID, result.runId()); try { LOG.infof("run result %s", result.getClass().getSimpleName()); - // FIRST, before any check that may drop the result: every one of these says the agent has - // stopped, so the signed-in seat it held is free for the next run whatever else happens to - // this result. Fenced by run id, so a late result from an older run frees nothing. - if (!(result instanceof RunResult.RunStarted)) pool.releaseLease(result.runId()); + // A seat is freed only when the worker confirms the agent stopped, never on an outcome: a + // failed run can leave its agent running (review of PR #178). Not an outcome, so nothing + // else reads it. Both writes are fenced by run id, so a late message frees nothing else. + if (result instanceof RunResult.RunAgentStopped) { + pool.releaseLease(result.runId()); + return; + } + // The agent is running: its seat is held until it is confirmed stopped, however long. + if (result instanceof RunResult.RunStarted) pool.holdWhileRunning(result.runId()); if(!workItems.acceptsBinding(result))return; projection.apply(result); // AFTER the projection, deliberately. The run's outcome is the fact an operator is diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/RunTokenUsage.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/RunTokenUsage.java index e16c8f24d..ec6ba40a7 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/RunTokenUsage.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/RunTokenUsage.java @@ -196,6 +196,8 @@ private static Map usageOf(RunResult result) { case RunResult.RunWorkReady ready -> ready.tokenUsage(); case RunResult.RunFailed failed -> failed.tokenUsage(); case RunResult.RunStarted ignored -> null; + // Not an outcome and never charged: the saga stops at it. + case RunResult.RunAgentStopped ignored -> null; }; } diff --git a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/SignInFiles.java b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/SignInFiles.java index 2f28054c8..e8a417f8f 100644 --- a/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/SignInFiles.java +++ b/spire-orchestrator/src/main/java/dev/codespire/orchestrator/factory/SignInFiles.java @@ -42,6 +42,19 @@ static String forAgent(String storedFile) { } } + /** + * The vendor account a sign-in belongs to: {@code tokens.account_id}, measured on codex-cli 0.156.1 + * (design §5.3). Empty when the file names none, which the caller refuses rather than guesses. + */ + static java.util.Optional accountOf(String storedFile) { + try { + String account = JSON.readTree(storedFile).path("tokens").path("account_id").asText(""); + return account.isBlank() ? java.util.Optional.empty() : java.util.Optional.of(account); + } catch (JsonProcessingException unreadable) { + return java.util.Optional.empty(); + } + } + /** Every level, arrays included: the vendor may add a list of sessions, each with its own token. */ private static void emptyRefreshTokens(JsonNode node) { if (node instanceof ArrayNode array) { 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 86682e620..e242e6319 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 @@ -68,7 +68,21 @@ public Prepared assemble(Connection c,WorkSourceRegistry.Source source,WorkItemE if(spend.decide().refused())throw new IllegalStateException("deployment_spend_cap_reached"); 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())); - HarnessCredentialPool.PoolMember member=subscription ? leaseSeat(in.harness(),id,wall) : pickKey(); + java.time.Instant startBy=java.time.Instant.now().plusSeconds(START_WITHIN_SECONDS); + HarnessCredentialPool.PoolMember member=subscription ? leaseSeat(in.harness(),id,startBy.plusSeconds(START_REPORT_SECONDS)) : pickKey(); + try { + return prepare(source,item,in,account,admission,id,branch,wall,member,subscription?startBy:null); + } catch(RuntimeException failure) { + // Nothing was dispatched, so no agent uses the seat: free it now rather than at its deadline. + if(subscription)pool.releaseLease(id); + throw failure; + } + } + + private Prepared prepare(WorkSourceRegistry.Source source,WorkItemEvent item,DispatchRequestParser.Parsed in, + dev.codespire.orchestrator.provider.ScmProvider account,HarnessCatalogues.Admission admission,String id,String branch, + long wall,HarnessCredentialPool.PoolMember member,java.time.Instant signInStartBy) { + boolean subscription=signInStartBy!=null; // The agent gets the sign-in with its refresh token emptied; the whole file stays here, encrypted. String handedOver=subscription ? SignInFiles.forAgent(member.apiKey()) : member.apiKey(); RunCommand.ExecuteRun command=new RunCommand.ExecuteRun(id,source.repository(),FactoryCloneUrls.cloneUrl(source.scm(),account.baseUrl(),source.repository()), @@ -78,7 +92,7 @@ public Prepared assemble(Connection c,WorkSourceRegistry.Source source,WorkItemE credentials.packScm(id,account.botUsername(),account.secret()),credentials.packHarness(id,handedOver)) // The level the approved binding hashed, so the build runs at what was approved (M3.5 part M). .atEffort(item.preparation().effort()); - if(subscription)command=command.paidBySignIn(); + if(subscription)command=command.paidBySignIn(signInStartBy); var held=new RunCommand.ExecuteWorkRun(command,new dev.codespire.contract.work.WorkRunBinding( item.workItemId(),item.generation(),item.progress().attemptId(),item.preparation().binding())); var row=new FactoryRunProjection.QueuedRun(id,in.harness(),in.model(),in.baseBranch(),in.baseCommit(),branch,account.botUsername(),member.id()); @@ -86,13 +100,20 @@ public Prepared assemble(Connection c,WorkSourceRegistry.Source source,WorkItemE } /** - * How long past the wall clock a lease outlives the run it was taken for. The run's own result - * releases it; this only bounds a lease whose release never arrives, so the seat is not held for ever. + * How long a worker has to start a subscription build. The worker refuses the build after this, so a + * command that waited in the queue cannot start on a seat that has meanwhile gone to another build. + */ + static final long START_WITHIN_SECONDS=600; + + /** + * How long after the start deadline the seat stays held for the build's start to be reported. The + * report removes the time bound; a lease that reaches this without one belonged to a build that + * never started, and frees itself. */ - private static final long LEASE_MARGIN_SECONDS=600; + static final long START_REPORT_SECONDS=300; - private HarnessCredentialPool.PoolMember leaseSeat(String harness,String runId,long wallSeconds) { - return pool.selectSubscription(harness,runId,java.time.Instant.now().plusSeconds(wallSeconds+LEASE_MARGIN_SECONDS)) + private HarnessCredentialPool.PoolMember leaseSeat(String harness,String runId,java.time.Instant leasedUntil) { + return pool.selectSubscription(harness,runId,leasedUntil) .orElseThrow(()->new IllegalStateException("subscription_unavailable")); } diff --git a/spire-orchestrator/src/main/resources/db/migration/V85__one_seat_per_account.sql b/spire-orchestrator/src/main/resources/db/migration/V85__one_seat_per_account.sql new file mode 100644 index 000000000..cfbc98b26 --- /dev/null +++ b/spire-orchestrator/src/main/resources/db/migration/V85__one_seat_per_account.sql @@ -0,0 +1,13 @@ +-- One signed-in seat per vendor account (M3.5 part F, review of PR #178). +-- +-- V77 left account_ref empty until a real sign-in had been measured. It has been (design section 5.3): +-- the file carries tokens.account_id. Two seats signed in to one account would be two leases on one +-- sign-in, so an enabled seat's account is unique per harness. Signing in to the same account again +-- replaces that seat's file instead of adding a second seat. +-- +-- Existing seats have no account_ref yet. The orchestrator fills it at startup from each stored file, +-- switching off all but the newest seat of an account; a seat whose file names no account is never +-- leased and shows "Sign in again". +CREATE UNIQUE INDEX harness_credential_one_seat_per_account + ON harness_credential (type, account_ref) + WHERE auth_mode = 'SUBSCRIPTION' AND enabled AND account_ref IS NOT NULL; 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 aa78cae2a..3c4f95d9e 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 @@ -161,7 +161,7 @@ void noThinkingLevelMeansTheModelsOwnDefault() { /** A signed-in codex seat, switched off by the caller when done: a seat a run points at cannot be deleted. */ private UUID seat() throws java.sql.SQLException { try (var c = dataSource.getConnection()) { - return pool.addSubscription(c, "TEST-build-seat-" + UUID.randomUUID(), "codex", "{\"auth_mode\":\"TEST\"}"); + return pool.addSubscription(c, "TEST-build-seat-" + UUID.randomUUID(), "codex", "{\"auth_mode\":\"TEST\",\"tokens\":{\"account_id\":\"TEST-account-" + UUID.randomUUID() + "\"}}"); } } diff --git a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/FactoryRunProjectionTest.java b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/FactoryRunProjectionTest.java index 162dd4f4f..132356282 100644 --- a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/FactoryRunProjectionTest.java +++ b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/FactoryRunProjectionTest.java @@ -411,7 +411,7 @@ void aReArmThatPaysAnotherWayIsRefused() { void aRunPaidByASubscriptionSaysSo() throws SQLException { UUID seat; try (Connection c = dataSource.getConnection()) { - seat = pool.addSubscription(c, "TEST-frp-seat-" + UUID.randomUUID(), "codex", "{\"auth_mode\":\"TEST\"}"); + seat = pool.addSubscription(c, "TEST-frp-seat-" + UUID.randomUUID(), "codex", "{\"auth_mode\":\"TEST\",\"tokens\":{\"account_id\":\"TEST-account-" + UUID.randomUUID() + "\"}}"); } try { String runId = "run::github:TEST-acme/app:subject-" + UUID.randomUUID() + ":1"; diff --git a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessSignInsTest.java b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessSignInsTest.java index e4cbedad6..882fb3e2f 100644 --- a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessSignInsTest.java +++ b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessSignInsTest.java @@ -37,8 +37,11 @@ class HarnessSignInsTest { private static final String HARNESS = "codex"; /** A subscription-shaped file. The mode is NOT the measured API-key one, which is all that matters. */ - private static final String SUBSCRIPTION_FILE = - "{\"auth_mode\":\"TEST-chatgpt\",\"tokens\":{\"access\":\"TEST-not-a-real-token\"}}"; + private static final String SUBSCRIPTION_FILE = subscriptionFile("TEST-account-" + UUID.randomUUID(), "TEST-not-a-real-token"); + + private static String subscriptionFile(String account, String token) { + return "{\"auth_mode\":\"TEST-chatgpt\",\"tokens\":{\"access\":\"" + token + "\",\"account_id\":\"" + account + "\"}}"; + } /** The measured API-key file, with an obviously fake key. */ private static final String API_KEY_FILE = "{\"auth_mode\":\"apikey\",\"OPENAI_API_KEY\":\"sk-TEST-not-real\"}"; @@ -126,6 +129,46 @@ void anApprovedSignInBecomesASubscriptionMemberUnderTheNameTheOperatorGaveIt() { * bill per token — the exact outcome this part exists to prevent, and one nothing downstream could * detect, because both look like a pool member afterwards. */ + /** + * Signing in again to an account that has a seat renews that seat's file instead of adding a second + * seat — the way an expired seat is renewed, and never a second lease on one sign-in (review of PR #178). + */ + @Test + void aSecondSignInToTheSameAccountRenewsItsSeat() throws SQLException { + String account = "TEST-account-" + UUID.randomUUID(); + HarnessSignIns.View first = start("TEST-seat-renew-first"); + signIns.completed(completion(first.id(), subscriptionFile(account, "TEST-old-token"))); + UUID seat = signIns.get(first.id()).orElseThrow().credentialId(); + + HarnessSignIns.View second = start("TEST-seat-renew-second"); + signIns.completed(completion(second.id(), subscriptionFile(account, "TEST-new-token"))); + + HarnessSignIns.View renewed = signIns.get(second.id()).orElseThrow(); + assertEquals("COMPLETE", renewed.state()); + assertEquals(seat, renewed.credentialId(), "the same seat, not a second one"); + assertTrue(pool.list().stream().noneMatch(member -> member.label().equals("TEST-seat-renew-second"))); + try (Connection c = dataSource.getConnection(); + java.sql.PreparedStatement ps = c.prepareStatement("SELECT api_key FROM harness_credential WHERE id = ?")) { + ps.setObject(1, seat); + try (java.sql.ResultSet rs = ps.executeQuery()) { + assertTrue(rs.next()); + assertTrue(encryption.decryptString(rs.getString(1), "harness-credential:" + seat).contains("TEST-new-token")); + } + } + } + + @Test + void aSignInThatNamesNoAccountIsRefused() { + HarnessSignIns.View view = start("TEST-seat-no-account"); + + signIns.completed(completion(view.id(), "{\"auth_mode\":\"TEST-chatgpt\",\"tokens\":{\"access\":\"TEST-token\"}}")); + + HarnessSignIns.View after = signIns.get(view.id()).orElseThrow(); + assertEquals("FAILED", after.state()); + assertEquals("subscription_unidentified", after.reason()); + assertTrue(pool.list().stream().noneMatch(member -> member.label().equals("TEST-seat-no-account"))); + } + @Test void anApiKeySignInIsRefusedRatherThanStoredAsASubscription() { HarnessSignIns.View view = start("TEST-seat-wrong-mode"); diff --git a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessSubscriptionLeaseTest.java b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessSubscriptionLeaseTest.java index a14b1280c..b37d3d827 100644 --- a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessSubscriptionLeaseTest.java +++ b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/HarnessSubscriptionLeaseTest.java @@ -30,22 +30,45 @@ class HarnessSubscriptionLeaseTest { @Inject DataSource dataSource; private static final String HARNESS = "TEST-lease-harness"; - private static final String FILE = "{\"auth_mode\":\"TEST\",\"tokens\":{\"refresh_token\":\"TEST-refresh\"}}"; private final List seats = new ArrayList<>(); + /** A sign-in file of its own account: one account has one enabled seat. */ + private static String file(String account) { + return "{\"auth_mode\":\"TEST\",\"tokens\":{\"refresh_token\":\"TEST-refresh\",\"account_id\":\"" + account + "\"}}"; + } + + private static String anAccount() { + return "TEST-account-" + UUID.randomUUID(); + } + @AfterEach void switchSeatsOff() { seats.forEach(pool::remove); } private UUID seat(String label) throws SQLException { + return seatOn(label, anAccount()); + } + + private UUID seatOn(String label, String account) throws SQLException { try (Connection c = dataSource.getConnection()) { - UUID id = pool.addSubscription(c, label + "-" + UUID.randomUUID(), HARNESS, FILE); + UUID id = pool.addSubscription(c, label + "-" + UUID.randomUUID(), HARNESS, file(account)); seats.add(id); return id; } } + private void sql(String statement, Object... params) throws SQLException { + try (Connection c = dataSource.getConnection(); PreparedStatement ps = c.prepareStatement(statement)) { + for (int i = 0; i < params.length; i++) ps.setObject(i + 1, params[i]); + ps.executeUpdate(); + } + } + + private HarnessCredentialPool.MemberView view(UUID seat) { + return pool.list().stream().filter(member -> member.id().equals(seat)).findFirst().orElseThrow(); + } + private static Instant inAnHour() { return Instant.now().plusSeconds(3600); } @@ -58,7 +81,7 @@ void aLeasedSeatIsNotHandedToASecondRun() throws SQLException { Optional second = pool.selectSubscription(HARNESS, "TEST-run-2", inAnHour()); assertEquals(seat, first.orElseThrow().id()); - assertEquals(FILE, first.orElseThrow().apiKey(), "the whole stored file; the dispatch empties its refresh token"); + assertTrue(first.orElseThrow().apiKey().contains("TEST-refresh"), "the whole stored file; the dispatch empties its refresh token"); assertTrue(second.isEmpty(), "one sign-in, one agent"); } @@ -107,12 +130,83 @@ void aLeaseThatRanOutFreesTheSeat() throws SQLException { void theListSaysWhenASeatIsInUse() throws SQLException { UUID seat = seat("TEST-lease-view"); pool.selectSubscription(HARNESS, "TEST-run-view", inAnHour()).orElseThrow(); + assertTrue(view(seat).inUse()); - var view = pool.list().stream().filter(member -> member.id().equals(seat)).findFirst().orElseThrow(); - assertNotNull(view.leasedUntil()); + pool.holdWhileRunning("TEST-run-view"); + assertTrue(view(seat).inUse(), "a running agent's seat is in use"); pool.releaseLease("TEST-run-view"); - assertNull(pool.list().stream().filter(member -> member.id().equals(seat)).findFirst().orElseThrow().leasedUntil()); + assertFalse(view(seat).inUse()); + } + + /** + * Once the agent starts, no clock frees its seat: only the worker's word that the agent stopped does + * (review of PR #178 — a failed stop can leave an agent running long past its wall clock). + */ + @Test + void aRunningAgentsSeatIsNeverFreedByTheClock() throws SQLException { + UUID seat = seat("TEST-lease-running"); + pool.selectSubscription(HARNESS, "TEST-run-running", Instant.now().minusSeconds(60)).orElseThrow(); + pool.holdWhileRunning("TEST-run-running"); + + assertTrue(pool.selectSubscription(HARNESS, "TEST-run-other", inAnHour()).isEmpty()); + + pool.releaseLease("TEST-run-running"); + assertTrue(pool.selectSubscription(HARNESS, "TEST-run-other", inAnHour()).isPresent()); + } + + /** A worker that died for good never reports; an operator frees the seat. */ + @Test + void anOperatorCanFreeASeatWhoseWorkerIsGone() throws SQLException { + UUID seat = seat("TEST-lease-freed"); + assertFalse(pool.freeSeat(seat), "a seat nobody holds has nothing to free"); + pool.selectSubscription(HARNESS, "TEST-run-lost-worker", inAnHour()).orElseThrow(); + pool.holdWhileRunning("TEST-run-lost-worker"); + + assertTrue(pool.freeSeat(seat)); + + assertFalse(view(seat).inUse()); + assertTrue(pool.selectSubscription(HARNESS, "TEST-run-after-freeing", inAnHour()).isPresent()); + } + + /** A seat whose account is unknown could be a second seat on one account, so it is never leased. */ + @Test + void aSeatWithNoKnownAccountIsNeverLeased() throws SQLException { + UUID seat = seat("TEST-lease-unidentified"); + sql("UPDATE harness_credential SET account_ref = NULL WHERE id = ?", seat); + + assertTrue(pool.selectSubscription(HARNESS, "TEST-run-unidentified", inAnHour()).isEmpty()); + assertFalse(view(seat).identified()); + } + + /** A sign-in that names no account is not stored as a seat at all. */ + @Test + void aSignInWithNoAccountIsRefused() { + IllegalArgumentException refused = assertThrows(IllegalArgumentException.class, () -> { + try (Connection c = dataSource.getConnection()) { + pool.addSubscription(c, "TEST-lease-no-account-" + UUID.randomUUID(), HARNESS, "{\"auth_mode\":\"TEST\"}"); + } + }); + assertEquals("subscription_unidentified", refused.getMessage()); + } + + /** + * Seats stored before accounts were recorded are identified from their own files. The newest seat of + * an account keeps it; an older one is switched off, and cannot be switched back on beside it. + */ + @Test + void identifyingOldSeatsKeepsTheNewestSeatOfAnAccount() throws SQLException { + String account = anAccount(); + UUID older = seatOn("TEST-lease-older", account); + sql("UPDATE harness_credential SET account_ref = NULL, updated_at = now() - interval '2 days' WHERE id = ?", older); + UUID newer = seatOn("TEST-lease-newer", account); + sql("UPDATE harness_credential SET account_ref = NULL, updated_at = now() - interval '1 day' WHERE id = ?", newer); + + assertTrue(pool.identifySeats() >= 1); + + assertTrue(view(newer).enabled() && view(newer).identified()); + assertFalse(view(older).enabled(), "a second seat on one account is a second lease on one sign-in"); + assertThrows(HarnessCredentialPool.SeatTakenException.class, () -> pool.enable(older)); } /** API-key selection never reaches a seat: that path hands its credential out whole, to anyone. */ diff --git a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/RunResultSagaTest.java b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/RunResultSagaTest.java index 009a7ad20..8bcf5b59d 100644 --- a/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/RunResultSagaTest.java +++ b/spire-orchestrator/src/test/java/dev/codespire/orchestrator/factory/RunResultSagaTest.java @@ -39,23 +39,30 @@ public void record(RunResult result) { } } - /** Which runs' seats the saga released. Static because the saga factories below are. */ + /** Which runs' seats the saga released and held. Static because the saga factories below are. */ private static final List released = new ArrayList<>(); + private static final List held = new ArrayList<>(); /** - * Every result that says the agent has stopped frees the seat it held; a start does not (M3.5 part F). - * Released even when the work-item bridge would drop the result, because the agent is still done. + * A seat is held while the agent runs and freed only when the worker confirms the agent stopped — + * never on an outcome, because a failed run can leave its agent running (review of PR #178). */ @Test - void aResultThatEndsTheAgentFreesItsSeatAndAStartDoesNot() { + void aSeatIsHeldFromTheStartAndFreedOnlyWhenTheAgentIsConfirmedStopped() { released.clear(); - RunResultSaga saga = saga(new RecordingProjection()); + held.clear(); + RecordingProjection projection = new RecordingProjection(); + RunResultSaga saga = saga(projection); + String run = "run::github:TEST-acme/app:seat:1"; - saga.on(new RunResult.RunStarted("run::github:TEST-acme/app:started:1", "TEST-unit")); - saga.on(new RunResult.RunFinished("run::github:TEST-acme/app:done:1", "refs/heads/spire/s", - List.of(), List.of(), Map.of("input", 10L), false)); + saga.on(new RunResult.RunStarted(run, "TEST-unit")); + saga.on(new RunResult.RunFailed(run, "AGENT_TIMEOUT", "TEST: overran", false, null)); + assertEquals(List.of(run), held); + assertEquals(List.of(), released, "a failure does not prove the agent stopped"); - assertEquals(List.of("run::github:TEST-acme/app:done:1"), released); + saga.on(new RunResult.RunAgentStopped(run)); + assertEquals(List.of(run), released); + assertEquals(2, projection.applied.size(), "not an outcome: nothing else reads it"); } private static RunResultSaga saga(RecordingProjection projection) { @@ -82,6 +89,11 @@ private static RunResultSaga saga(RecordingProjection projection, RecordingCharg public void releaseLease(String runId) { released.add(runId); } + + @Override + public void holdWhileRunning(String runId) { + held.add(runId); + } }; saga.credentials = new RunCredentialFeedback() { @Override 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 2cea3f84e..f1d454142 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 @@ -112,7 +112,7 @@ void aSetupTheHarnessNoLongerRunsIsNotPrepared() throws Exception { void theSavedWayToPayIsCopiedIntoThePreparedTask() throws Exception { java.util.UUID seat; try (Connection c = dataSource.getConnection()) { - seat = pool.addSubscription(c, "TEST-sweep-seat-" + java.util.UUID.randomUUID(), "codex", "{\"auth_mode\":\"TEST\"}"); + seat = pool.addSubscription(c, "TEST-sweep-seat-" + java.util.UUID.randomUUID(), "codex", "{\"auth_mode\":\"TEST\",\"tokens\":{\"account_id\":\"TEST-account-" + java.util.UUID.randomUUID() + "\"}}"); } try { defaults.save(repository, new BuildDefaults.Input(defaults.get(repository).revision(), "main", "codex", model, 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 22f7ebc8b..c6e20fdf1 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 @@ -72,7 +72,8 @@ private void codexRuns(String slug,String... levels) { UUID seat; try(var c=dataSource.getConnection()) { seat=pool.addSubscription(c,"TEST-dispatch-seat-"+UUID.randomUUID(),"codex", - "{\"auth_mode\":\"chatgpt\",\"tokens\":{\"access_token\":\"TEST-access\",\"refresh_token\":\"TEST-refresh-must-not-leave\"}}"); + "{\"auth_mode\":\"chatgpt\",\"tokens\":{\"access_token\":\"TEST-access\",\"refresh_token\":\"TEST-refresh-must-not-leave\"," + +"\"account_id\":\"TEST-account-"+UUID.randomUUID()+"\"}}"); } try { String id=admit("autonomous",58);var plain=preparation("TEST-prepared-admin"); @@ -91,7 +92,37 @@ private void codexRuns(String slug,String... levels) { "the run names the seat that paid"); assertEquals(1,count("SELECT count(*) FROM factory_run WHERE run_id=? AND paid_by='SUBSCRIPTION'",command.runId()), "the run records how it paid, where a re-arm cannot erase it"); - assertTrue(pool.list().stream().anyMatch(member->member.id().equals(seat)&&member.leasedUntil()!=null),"the seat is leased"); + assertTrue(pool.list().stream().anyMatch(member->member.id().equals(seat)&&member.inUse()),"the seat is leased"); + // The worker must start it soon, or refuse it: a late start could meet a seat given to another build. + java.time.Duration window=java.time.Duration.between(java.time.Instant.now(),command.signInStartBy()); + assertTrue(window.toSeconds()>500 && window.toSeconds()<=600,"start within ten minutes, was "+window); + } finally { pool.remove(seat); } + } + + /** An assembly that fails after the seat is leased dispatches nothing, so it frees the seat at once. */ + @Test void aSubscriptionBuildThatCannotBeAssembledFreesItsSeat() throws Exception { + UUID seat; + try(var c=dataSource.getConnection()) { + seat=pool.addSubscription(c,"TEST-dispatch-unreadable-"+UUID.randomUUID(),"codex", + "{\"auth_mode\":\"TEST\",\"tokens\":{\"account_id\":\"TEST-account-"+UUID.randomUUID()+"\"}}"); + // The stored file becomes unreadable after it was identified: the hand-over fails after the lease. + try(var ps=c.prepareStatement("UPDATE harness_credential SET api_key=? WHERE id=?")) { + ps.setString(1,encryption.encryptString("TEST-not-a-sign-in","harness-credential:"+seat)); + ps.setObject(2,seat);ps.executeUpdate(); + } + } + try { + String id=admit("autonomous",59);var plain=preparation("TEST-prepared-admin"); + var paying=new WorkPreparation(plain.specification(),plain.plan(),plain.baseBranch(),plain.baseCommit(),plain.harness(), + plain.model(),plain.registeredBy(),WorkPreparation.PAY_WITH_BINDING,null,PayWith.SUBSCRIPTION); + var outcome=transitions.prepare(id,store.history(id).size(),paying);assertEquals(200,outcome.status(),outcome.reason()); + int before=heldCommands.size(); + + dispatcher.drain(); + + assertEquals(before,heldCommands.size(),"nothing was dispatched"); + assertTrue(pool.list().stream().anyMatch(member->member.id().equals(seat)&&!member.inUse()), + "no agent uses the seat, so it is free now rather than at its deadline"); } finally { pool.remove(seat); } } @Inject RunResultSaga saga; diff --git a/spire-run-worker/src/main/java/dev/codespire/runworker/HarnessSignInWorker.java b/spire-run-worker/src/main/java/dev/codespire/runworker/HarnessSignInWorker.java index 4bbf4bf3b..b46ba8288 100644 --- a/spire-run-worker/src/main/java/dev/codespire/runworker/HarnessSignInWorker.java +++ b/spire-run-worker/src/main/java/dev/codespire/runworker/HarnessSignInWorker.java @@ -59,6 +59,9 @@ public class HarnessSignInWorker { @Inject SignInRuntime runtime; @Inject EncryptionService encryption; + + /** Keys a sign-in's entry in LiveSecrets apart from any run id. */ + private static final String SIGN_IN_SECRETS = "sign-in:"; @Inject HarnessRegistry harnesses; /** Keyed and AWAITABLE: a Record send answers a stage, which a bare Message send does not. */ @@ -243,6 +246,7 @@ private void start(HarnessSignInCommand.Start command) { } } finally { running.remove(command.signInId()); + LiveSecrets.forget(SIGN_IN_SECRETS + command.signInId()); // ALWAYS, including after a failure. What this removes is a credential the operator just // created; leaving it in a stopped container leaves it readable by anything that can reach // the daemon, for as long as nobody notices. @@ -270,6 +274,10 @@ private HarnessSignInResult collect(String signInId, SignInRuntime.Handle handle "the sign-in ended without a credential (exit " + exit + ")"); } String body = new String(written.get(), StandardCharsets.UTF_8); + // A person's account credential, in this process until the unit is destroyed; see LiveSecrets. + LiveSecrets.register(SIGN_IN_SECRETS + signInId, () -> dev.codespire.secrets.SecretScrub.of( + Credentials.signInSecrets(body).stream() + .map(secret -> new dev.codespire.secrets.SecretScrub.Credential(null, secret)).toList())); String mode = SignInAuthMode.of(body); if (mode == null) { return new HarnessSignInResult.Failed(signInId, HarnessSignInResult.Failed.UNIT_FAILED, diff --git a/spire-run-worker/src/main/java/dev/codespire/runworker/LiveSecrets.java b/spire-run-worker/src/main/java/dev/codespire/runworker/LiveSecrets.java new file mode 100644 index 000000000..491f4a039 --- /dev/null +++ b/spire-run-worker/src/main/java/dev/codespire/runworker/LiveSecrets.java @@ -0,0 +1,64 @@ +package dev.codespire.runworker; + +import dev.codespire.secrets.SecretScrub; +import org.jboss.logging.Logger; + +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Supplier; + +/** + * The secrets of every run and sign-in this worker holds right now, for {@link SecretLogFilter}. + * + *

A failure detail was always scrubbed before it left the worker, but a log line was not: about + * twenty-five places log an exception whose message can quote a container's create request, and with it + * a model key, a forge token or a sign-in (review of PR #178). Scrubbing each site would miss the next + * one written. So every log record passes one filter, and the filter scrubs against what is held here. + * + *

Static because Quarkus builds logging filters before CDI exists. An entry lives while its unit may + * still produce output: from the start of a run until its unit is gone. A unit that outlives this + * process is logged about by a later process that never decrypted its secrets, so that process cannot + * scrub them — the same limit {@code RunFailures} states for the watchdog. + */ +final class LiveSecrets { + + private static final Logger LOG = Logger.getLogger(LiveSecrets.class); + + private static final Map HELD = new ConcurrentHashMap<>(); + + private LiveSecrets() { + } + + /** + * Holds a run's secrets for the log filter. Never throws: building the scrub decrypts, and a run must + * not fail because its log lines could not be protected — it says so instead. + */ + static void register(String key, Supplier scrub) { + try { + HELD.put(key, scrub.get()); + } catch (RuntimeException unavailable) { + LOG.warnf("%s: its secrets could not be read for log scrubbing (%s); its log lines are not scrubbed", + key, unavailable.getClass().getSimpleName()); + } + } + + static void forget(String key) { + HELD.remove(key); + } + + static boolean isEmpty() { + return HELD.isEmpty(); + } + + static boolean holds(String key) { + return HELD.containsKey(key); + } + + /** The text with every held secret replaced. */ + static String clean(String text) { + if (text == null) return null; + String cleaned = text; + for (SecretScrub scrub : HELD.values()) cleaned = scrub.clean(cleaned); + return cleaned; + } +} diff --git a/spire-run-worker/src/main/java/dev/codespire/runworker/OrphanWatchdog.java b/spire-run-worker/src/main/java/dev/codespire/runworker/OrphanWatchdog.java index 38e79ba9b..edfda7739 100644 --- a/spire-run-worker/src/main/java/dev/codespire/runworker/OrphanWatchdog.java +++ b/spire-run-worker/src/main/java/dev/codespire/runworker/OrphanWatchdog.java @@ -66,6 +66,12 @@ public class OrphanWatchdog { */ static final String REAP_SLOT = "reap"; + /** + * Once-only, like the reap report: a held unit is stopped again on every sweep, and reporting its + * agent stopped each time would repeat one message for as long as the workspace is kept. + */ + static final String AGENT_STOPPED_SLOT = "agent-stopped"; + /** * How many consecutive missed heartbeats a live run must survive before it looks abandoned. * @@ -211,6 +217,7 @@ private void reapIfOrphaned(RunHandle unit, Instant staleBefore) { // ordinary salvage, terminal failure or deletion can stand in for a delivery decision. stop(unit); LOG.warn("publication is held; stopped the abandoned processes and retained their workspace"); + agentStopped(unit); return; } boolean preserved = lease.map(WorkspaceLeases.Lease::preserved).orElse(false); @@ -251,6 +258,7 @@ private void reap(RunHandle unit, boolean alreadyReported) { + finalization.detail() + "); the unit is preserved and was not destroyed"); } stop(unit); + agentStopped(unit); return; } if (!alreadyReported) { @@ -259,7 +267,20 @@ private void reap(RunHandle unit, boolean alreadyReported) { } if (destroy(unit)) { leases.release(unit.runId()); + LiveSecrets.forget(unit.runId()); } + agentStopped(unit); + } + + /** + * Frees a signed-in seat once the runtime confirms the agent stopped (M3.5 part F). Every unit, not + * only a seat's: the watchdog cannot tell which paid by sign-in, and freeing a seat nobody holds is a + * no-op on the other side. The claim is given back when nothing was sent — the agent may still run, + * or the broker refused — so the next sweep asks again. + */ + private void agentStopped(RunHandle unit) { + if (!claims.claim(unit.runId(), AGENT_STOPPED_SLOT)) return; + if (!results.agentStoppedIfKnown(runtime, unit.runId())) claims.release(unit.runId(), AGENT_STOPPED_SLOT); } private Finalization salvage(RunHandle unit) { diff --git a/spire-run-worker/src/main/java/dev/codespire/runworker/RunDispatcher.java b/spire-run-worker/src/main/java/dev/codespire/runworker/RunDispatcher.java index 95c904b67..d55d52341 100644 --- a/spire-run-worker/src/main/java/dev/codespire/runworker/RunDispatcher.java +++ b/spire-run-worker/src/main/java/dev/codespire/runworker/RunDispatcher.java @@ -164,6 +164,14 @@ private CompletionStage handle(Message message, RunCommand com ack(message); return DONE; } + if (execute.harnessSignIn()) { + // A seat pays for held item builds only, whose worker path reports the agent stopped and + // frees the seat. This path does not, so a sign-in here would hold a seat for ever. + ack(message); + emit(failures.of(execute, RunFailureCause.BAD_COMMAND.name(), + "a subscription seat pays for item builds only, and this run is not one")); + return DONE; + } if (!claims.claim(execute.runId(), EXECUTE_SLOT)) { // A redelivery. Not an error, and NOT a reason to re-run the agent: the first delivery // either finished or is finishing, and a second unit would spend money twice. @@ -214,6 +222,8 @@ private CompletionStage handle(Message message, RunCommand com return DONE; } + // From here the run's credentials are in a container that may log them back; see LiveSecrets. + LiveSecrets.register(execute.runId(), () -> failures.scrubFor(execute)); LeaseKeeper keeper = new LeaseKeeper(execute.runId(), execute.harness()); RunResult result; try { @@ -240,7 +250,8 @@ private CompletionStage handle(Message message, RunCommand com // reclaim it. emit is guarded, but asCancellationIfCancelled reaches SecretScrub, which // is not, so "nothing after the ack may throw" was a rule this line did not enforce. registry.forget(execute.runId()); - keeper.settle(); + // A preserved unit may still log; the watchdog forgets it when it reaps the unit. + if (keeper.settle()) LiveSecrets.forget(execute.runId()); } return DONE; } @@ -360,13 +371,15 @@ public void unitReleased() { * find — but it is STAMPED rather than merely left alone, which stops the heartbeat from * refreshing it forever and so lets it become findable at all. */ - private void settle() { + /** @return whether the unit is gone */ + private boolean settle() { if (!unitExists || unitGone) { leases.release(runId); - return; + return true; } LOG.infof("run %s: its unit was not destroyed, so the lease is kept for the watchdog", runId); leases.preserve(runId); + return false; } } diff --git a/spire-run-worker/src/main/java/dev/codespire/runworker/RunResultReporter.java b/spire-run-worker/src/main/java/dev/codespire/runworker/RunResultReporter.java index 556a1d11d..65798f3ed 100644 --- a/spire-run-worker/src/main/java/dev/codespire/runworker/RunResultReporter.java +++ b/spire-run-worker/src/main/java/dev/codespire/runworker/RunResultReporter.java @@ -56,6 +56,25 @@ void check(@Observes StartupEvent event) { } } + /** + * Report the run's agent as stopped when the runtime KNOWS it is not running — what frees a signed-in + * seat (M3.5 part F). Silent when the agent may still run or the runtime cannot tell: the seat then + * stays held, which is the safe way to be wrong, and a later sweep or an operator frees it. + * + * @return whether the report was sent and acknowledged + */ + public boolean agentStoppedIfKnown(dev.codespire.runtime.RunRuntime runtime, String runId) { + boolean running; + try { + running = runtime.agentRunning(new dev.codespire.runtime.RunHandle(runId, runId)); + } catch (RuntimeException unknown) { + LOG.warnf("run %s: whether its agent still runs is unknown (%s); a seat it holds stays held", + runId, unknown.getClass().getSimpleName()); + return false; + } + return !running && report(new RunResult.RunAgentStopped(runId)); + } + /** Publish, awaiting the broker's acknowledgement, and report rather than throw on a refusal. */ /** * @return true when the broker acknowledged the result; false when it did not. diff --git a/spire-run-worker/src/main/java/dev/codespire/runworker/SecretLogFilter.java b/spire-run-worker/src/main/java/dev/codespire/runworker/SecretLogFilter.java new file mode 100644 index 000000000..764cb1ae5 --- /dev/null +++ b/spire-run-worker/src/main/java/dev/codespire/runworker/SecretLogFilter.java @@ -0,0 +1,72 @@ +package dev.codespire.runworker; + +import io.quarkus.logging.LoggingFilter; +import org.jboss.logmanager.ExtLogRecord; + +import java.util.IdentityHashMap; +import java.util.Map; +import java.util.logging.Filter; +import java.util.logging.LogRecord; +import java.util.logging.SimpleFormatter; + +/** + * Scrubs every held run secret out of every log record: the message, and each message in the exception + * chain (review of PR #178). Configured on the console handler as {@code run-secrets}. + * + *

An exception is replaced, not edited — a {@link Throwable}'s message cannot be changed — by a copy + * that keeps the original class name in its message and the original stack frames, so the trace still + * points where it did. Suppressed exceptions are not copied. + */ +@LoggingFilter(name = "run-secrets") +public final class SecretLogFilter implements Filter { + + /** Deep enough for any real chain; a guard against a cycle the identity map somehow missed. */ + private static final int MAX_CAUSES = 32; + + @Override + public boolean isLoggable(LogRecord record) { + if (LiveSecrets.isEmpty()) return true; + String message = record instanceof ExtLogRecord ext ? ext.getFormattedMessage() : new SimpleFormatter().formatMessage(record); + String cleaned = LiveSecrets.clean(message); + if (cleaned != null && !cleaned.equals(message)) { + if (record instanceof ExtLogRecord ext) ext.setMessage(cleaned, ExtLogRecord.FormatStyle.NO_FORMAT); + else record.setMessage(cleaned); + record.setParameters(null); + } + if (record.getThrown() != null && quotesASecret(record.getThrown())) record.setThrown(scrubbed(record.getThrown())); + return true; + } + + private static boolean quotesASecret(Throwable thrown) { + int depth = 0; + for (Throwable at = thrown; at != null && depth < MAX_CAUSES; at = at.getCause(), depth++) { + String message = at.getMessage(); + if (message != null && !message.equals(LiveSecrets.clean(message))) return true; + } + return false; + } + + private static Throwable scrubbed(Throwable thrown) { + return copy(thrown, new IdentityHashMap<>(), 0); + } + + private static Throwable copy(Throwable original, Map seen, int depth) { + if (original == null || depth >= MAX_CAUSES || seen.put(original, Boolean.TRUE) != null) return null; + return new Scrubbed(original, copy(original.getCause(), seen, depth + 1)); + } + + /** A stand-in carrying the original's class name, scrubbed message and stack frames. */ + static final class Scrubbed extends RuntimeException { + Scrubbed(Throwable original, Throwable cause) { + super(original.getClass().getName() + ": " + LiveSecrets.clean(String.valueOf(original.getMessage())), + cause, false, true); + setStackTrace(original.getStackTrace()); + } + + /** Printed as the original would be — its class and message — not as this wrapper's name. */ + @Override + public String toString() { + return getMessage(); + } + } +} diff --git a/spire-run-worker/src/main/java/dev/codespire/runworker/WorkRunWorker.java b/spire-run-worker/src/main/java/dev/codespire/runworker/WorkRunWorker.java index ecaceda7d..baed64d9c 100644 --- a/spire-run-worker/src/main/java/dev/codespire/runworker/WorkRunWorker.java +++ b/spire-run-worker/src/main/java/dev/codespire/runworker/WorkRunWorker.java @@ -42,11 +42,18 @@ public CompletionStage execute(Message message,RunCommand.Exec message.ack().toCompletableFuture().join(); if(!claimed){flushResults();return CompletableFuture.completedFuture(null);} String id=command.runId(); + // Held until the retained workspace is released: a held unit's publisher runs later. + LiveSecrets.register(id,()->failures.scrubFor(command.execution())); active.add(id); try { RunResult result; if(claims.taken(id,RunDispatcher.CANCEL_SLOT) || store.revoked(command)) { result=failures.of(command.execution(),"CANCELLED","Cancelled before the held build started"); + } else if(startedTooLate(command)) { + // The seat was leased for a bounded time until the agent starts. Past it, the seat may + // already serve another build, and two agents must never share one sign-in. + result=failures.of(command.execution(),"BAD_COMMAND","The subscription seat's start deadline passed before" + +" a worker took the build; it may serve another build by now, so this one did not start"); } else if(!leases.take(id)) { result=failures.of(command.execution(),"WORKER_FAILED","No lease was taken; the held build was not started"); } else { @@ -62,10 +69,15 @@ public CompletionStage execute(Message message,RunCommand.Exec leases.preserve(id); active.remove(id); } + if(command.execution().harnessSignIn())results.agentStoppedIfKnown(runtime,id); flushResults(); return CompletableFuture.completedFuture(null); } + private static boolean startedTooLate(RunCommand.ExecuteWorkRun command) { + return command.execution().harnessSignIn() && Instant.now().isAfter(command.execution().signInStartBy()); + } + public void publish(RunCommand.PublishWorkRun request) { if(!(runtime instanceof PublicationRuntime publication))return; Optional unit=localUnit(request.runId()); @@ -83,6 +95,8 @@ public void publish(RunCommand.PublishWorkRun request) { private void publishClaimed(WorkRunStore.Held held,RunHandle handle,PublicationRuntime publication) { String id=held.execution().runId(); + // A publisher may run in a later process than the build did, so its secrets are held again here. + LiveSecrets.register(id,()->failures.scrubFor(held.execution().execution())); // Register the retained handle before publisher creation so control can find this run again. registry.register(id,held.execution().execution().harness(),handle,RunNotes.IGNORING); try { @@ -137,6 +151,7 @@ public void recover() { } else if("ready".equals(held.state()) && (cancelled(id) || store.revoked(held.execution()))) { unit.ifPresent(publication::cancel); store.terminal(cancelledResult(held.execution(),held.ready())); + if(held.execution().execution().harnessSignIn())results.agentStoppedIfKnown(runtime,id); } else if("building".equals(held.state()) && held.updatedAt().isBefore(horizon.orElseThrow())) { var lease=leases.find(id); if(lease.isPresent() && !lease.orElseThrow().preserved() @@ -146,6 +161,7 @@ public void recover() { unit.ifPresent(publication::cancel); store.abandonBuild(failures.of(held.execution().execution(),RunFailureCause.SALVAGE_FAILED.name(), "The worker stopped before recording build readiness; its unpublished workspace is retained")); + if(held.execution().execution().harnessSignIn())results.agentStoppedIfKnown(runtime,id); } } catch(RuntimeException failure) { LOG.warnf("run %s: retained work recovery deferred (%s)",id,failure.getClass().getSimpleName()); @@ -173,6 +189,7 @@ private void releasePublished(WorkRunStore.Held held,PublicationRuntime publicat else if(localUnit(id).isPresent())throw new IllegalStateException("Published resources no longer carry their expected hold"); store.released(id); leases.release(id); + LiveSecrets.forget(id); } private Optional localUnit(String id) { @@ -197,6 +214,7 @@ private RunResult cancelledResult(RunCommand.ExecuteWorkRun command,RunResult re case RunResult.RunFinished finished -> finished.tokenUsage(); case RunResult.RunFailed failed -> failed.tokenUsage(); case RunResult.RunStarted ignored -> null; + case RunResult.RunAgentStopped ignored -> null; }; return failures.of(command.execution(),"CANCELLED","The held run was cancelled; its unpublished workspace is retained").withUsage(usage); } @@ -229,5 +247,6 @@ public void hold(RunCommand.HoldWorkRun command) { registry.cancel(command.runId()); if(runtime instanceof PublicationRuntime publication) localUnit(command.runId()).filter(publication::publicationHeld).ifPresent(publication::cancel); + if(held.orElseThrow().execution().execution().harnessSignIn())results.agentStoppedIfKnown(runtime,command.runId()); } } diff --git a/spire-run-worker/src/main/resources/application.yml b/spire-run-worker/src/main/resources/application.yml index ab0cdc174..fe7eeb9d2 100644 --- a/spire-run-worker/src/main/resources/application.yml +++ b/spire-run-worker/src/main/resources/application.yml @@ -68,6 +68,9 @@ quarkus: create-schemas: true log: console: + # Scrubs every held run secret out of every record, exceptions included (SecretLogFilter). + # Some exceptions quote a container's create request, and with it the run's credentials. + filter: run-secrets json: enabled: true # The same two fields the other deployables carry, so one log pipeline can tell the diff --git a/spire-run-worker/src/test/java/dev/codespire/runworker/OrphanWatchdogTest.java b/spire-run-worker/src/test/java/dev/codespire/runworker/OrphanWatchdogTest.java index 57db6cc24..1be0ccb62 100644 --- a/spire-run-worker/src/test/java/dev/codespire/runworker/OrphanWatchdogTest.java +++ b/spire-run-worker/src/test/java/dev/codespire/runworker/OrphanWatchdogTest.java @@ -55,6 +55,14 @@ public Finalization publishHeld(RunHandle handle,dev.codespire.runtime.Publicati final List destroyed = new ArrayList<>(); Finalization finalization = Finalization.salvaged(0, "exited"); RuntimeException salvageFails; + /** What the runtime says about the agent; null is "cannot tell", which the SPI default throws. */ + Boolean agentRunning; + + @Override + public boolean agentRunning(RunHandle handle) { + if (agentRunning == null) throw new UnsupportedOperationException("TEST runtime cannot tell"); + return agentRunning; + } @Override public RuntimeType type() { @@ -268,6 +276,38 @@ void aSiblingsLiveRunIsNeverReaped() { assertEquals(List.of(), reported); } + /** + * A held unit is stopped again on every sweep; its agent is reported stopped once, when the runtime + * confirms it — which frees a signed-in seat (M3.5 part F, review of PR #178). + */ + @Test void aStoppedHeldAgentIsReportedOnceAcrossSweeps() { + runtime.held=true;runtime.agentRunning=false; + runtime.units.add(new RunHandle("TEST-held-stopped","TEST-unit")); + watchdog().sweep(); + watchdog().sweep(); + assertEquals(List.of(new RunResult.RunAgentStopped("TEST-held-stopped")),reported); + } + + /** An agent that may still run is not reported, and the next sweep asks again. */ + @Test void anAgentThatMayStillRunIsAskedAgainNextSweep() { + runtime.held=true;runtime.agentRunning=true; + runtime.units.add(new RunHandle("TEST-held-alive","TEST-unit")); + watchdog().sweep(); + assertTrue(reported.isEmpty(),"a seat is never freed under an agent that may still run"); + + runtime.agentRunning=false; + watchdog().sweep(); + assertEquals(List.of(new RunResult.RunAgentStopped("TEST-held-alive")),reported); + } + + /** A reaped unit's agent is reported stopped too, after the run's own reclamation report. */ + @Test void aReapedUnitsAgentIsReportedStopped() { + runtime.agentRunning=false; + runtime.units.add(new RunHandle("TEST-reaped","TEST-unit")); + watchdog().sweep(); + assertEquals(new RunResult.RunAgentStopped("TEST-reaped"),reported.getLast()); + } + @Test void aHeldWorkspaceWithNoLeaseIsStoppedAndRetainedWithoutInventingATerminalResult() { runtime.held=true; runtime.units.add(new RunHandle("TEST-held-no-lease","TEST-unit")); diff --git a/spire-run-worker/src/test/java/dev/codespire/runworker/RunDispatcherTest.java b/spire-run-worker/src/test/java/dev/codespire/runworker/RunDispatcherTest.java index 38e9405a9..a7ccffa0a 100644 --- a/spire-run-worker/src/test/java/dev/codespire/runworker/RunDispatcherTest.java +++ b/spire-run-worker/src/test/java/dev/codespire/runworker/RunDispatcherTest.java @@ -460,6 +460,21 @@ void aPoisonRecordIsNackedNotAckedAndNotRun() { assertTrue(results.sent.isEmpty()); } + /** + * A seat pays for held item builds only: this path never reports an agent stopped, so a sign-in run + * here would hold its seat for ever (review of PR #178). + */ + @Test + void aSignInRunIsRefusedOnTheStandalonePath() { + Delivery delivery = new Delivery(order); + dispatcher.onCommand(delivery.of(EXECUTE.paidBySignIn(java.time.Instant.now().plusSeconds(600)))) + .toCompletableFuture().join(); + + assertTrue(delivery.acked); + assertEquals(0, launcher.launches); + assertEquals("BAD_COMMAND", assertInstanceOf(RunResult.RunFailed.class, results.sent.getLast()).cause()); + } + @Test void aCancelIsAcknowledgedWithoutAClaimOrARun() { Delivery delivery = new Delivery(order); @@ -562,6 +577,7 @@ void anOrdinaryFailureReleasesItsLease() { dispatcher.onCommand(new Delivery(order).of(EXECUTE)).toCompletableFuture().join(); assertTrue(leases.released); + assertFalse(LiveSecrets.holds(EXECUTE.runId()), "a gone unit logs nothing more, so its secrets are let go"); } @Test @@ -577,6 +593,9 @@ void aFailureWhoseUnitSurvivedKeepsItsLeaseWhateverItsCause() { assertFalse(leases.released); assertTrue(leases.preserved); + // A surviving unit can still log; its secrets stay scrubbed until the watchdog reaps it. + assertTrue(LiveSecrets.holds(EXECUTE.runId())); + LiveSecrets.forget(EXECUTE.runId()); } @Test diff --git a/spire-run-worker/src/test/java/dev/codespire/runworker/RunLauncherTest.java b/spire-run-worker/src/test/java/dev/codespire/runworker/RunLauncherTest.java index 03e1d8d2c..c4ad670ee 100644 --- a/spire-run-worker/src/test/java/dev/codespire/runworker/RunLauncherTest.java +++ b/spire-run-worker/src/test/java/dev/codespire/runworker/RunLauncherTest.java @@ -689,7 +689,7 @@ public Map harnessEnv(String runId, String packed) { runtime.salvageFails = new IllegalStateException("codex said: bearer " + accessToken); RunResult.RunFailed failed = assertInstanceOf(RunResult.RunFailed.class, - launcher.launch(COMMAND.paidBySignIn(), RunObserver.IGNORING)); + launcher.launch(COMMAND.paidBySignIn(java.time.Instant.parse("2099-01-01T00:00:00Z")), RunObserver.IGNORING)); assertFalse(failed.detail().contains(accessToken), "one echoed token is still a leaked token"); assertTrue(failed.detail().contains("codex said"), "the diagnosis itself must survive"); 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 55a2f646f..180719bdc 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 @@ -195,7 +195,7 @@ void aProtectedBranchIsNotDroppedJustBecauseTheModeIsTheDefault() { /** A run paid by a sign-in hands the agent the file, never as a key (M3.5 part F). */ @Test void aRunPaidBySignInHandsTheAgentTheFileNotAKey() { - Map env = builder.build(command().paidBySignIn(), new CodexAdapter()).agent().environment(); + Map env = builder.build(command().paidBySignIn(java.time.Instant.parse("2099-01-01T00:00:00Z")), new CodexAdapter()).agent().environment(); assertEquals(HARNESS_KEY, env.get("CODEX_SIGN_IN_FILE")); assertFalse(env.containsKey("OPENAI_API_KEY")); diff --git a/spire-run-worker/src/test/java/dev/codespire/runworker/SecretLogFilterTest.java b/spire-run-worker/src/test/java/dev/codespire/runworker/SecretLogFilterTest.java new file mode 100644 index 000000000..d81d9c6d6 --- /dev/null +++ b/spire-run-worker/src/test/java/dev/codespire/runworker/SecretLogFilterTest.java @@ -0,0 +1,81 @@ +package dev.codespire.runworker; + +import dev.codespire.secrets.SecretScrub; +import org.jboss.logmanager.ExtLogRecord; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.logging.Level; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * No held run secret reaches a log line, in the message or anywhere in the exception chain (review of + * PR #178: raw exceptions quoting a container's create request were logged at about 25 places). + */ +class SecretLogFilterTest { + + private static final String RUN = "TEST-log-filter-run"; + private static final String SECRET = "TEST-secret-0123456789"; + + @AfterEach + void forget() { + LiveSecrets.forget(RUN); + } + + private static void hold() { + LiveSecrets.register(RUN, () -> SecretScrub.of(List.of(new SecretScrub.Credential(null, SECRET)))); + } + + private static ExtLogRecord record() { + ExtLogRecord record = new ExtLogRecord(Level.SEVERE, "run %s failed: %s", ExtLogRecord.FormatStyle.PRINTF, + SecretLogFilterTest.class.getName()); + record.setParameters(new Object[] {RUN, "env=[OPENAI_API_KEY=" + SECRET + "]"}); + record.setThrown(new IllegalStateException("create failed: " + SECRET, + new RuntimeException("caused by " + SECRET))); + return record; + } + + @Test + void aHeldSecretLeavesNeitherTheMessageNorTheExceptionChain() { + hold(); + ExtLogRecord record = record(); + StackTraceElement[] frames = record.getThrown().getStackTrace(); + + assertTrue(new SecretLogFilter().isLoggable(record), "scrubbed, never dropped"); + + assertFalse(record.getFormattedMessage().contains(SECRET), record.getFormattedMessage()); + assertTrue(record.getFormattedMessage().contains("run " + RUN + " failed"), "the diagnosis survives"); + for (Throwable at = record.getThrown(); at != null; at = at.getCause()) { + assertFalse(String.valueOf(at.getMessage()).contains(SECRET), at.getMessage()); + } + assertTrue(record.getThrown().toString().startsWith("java.lang.IllegalStateException: create failed"), + "the original class still names the failure"); + assertEquals(frames.length, record.getThrown().getStackTrace().length, "the trace still points where it did"); + } + + @Test + void aForgottenRunIsNoLongerScrubbed() { + hold(); + LiveSecrets.forget(RUN); + ExtLogRecord record = record(); + + new SecretLogFilter().isLoggable(record); + + assertTrue(record.getFormattedMessage().contains(SECRET), "nothing is held, so nothing is changed"); + } + + /** The filter does nothing unless the console handler names it; the name is the contract. */ + @Test + void theConsoleHandlerUsesTheFilter() throws Exception { + String config = Files.readString(Path.of("src/main/resources/application.yml")); + String name = SecretLogFilter.class.getAnnotation(io.quarkus.logging.LoggingFilter.class).name(); + + assertTrue(config.contains("filter: " + name), "quarkus.log.console.filter must name " + name); + } +} diff --git a/spire-run-worker/src/test/java/dev/codespire/runworker/WorkRunWorkerTest.java b/spire-run-worker/src/test/java/dev/codespire/runworker/WorkRunWorkerTest.java index b1d1f3e4e..34dc8bb7b 100644 --- a/spire-run-worker/src/test/java/dev/codespire/runworker/WorkRunWorkerTest.java +++ b/spire-run-worker/src/test/java/dev/codespire/runworker/WorkRunWorkerTest.java @@ -14,7 +14,9 @@ /** Discriminating worker decisions; persistence and containers have separate real integration proofs. */ class WorkRunWorkerTest { - final RunCommand.ExecuteWorkRun command=HeldRunLauncherTest.COMMAND; + RunCommand.ExecuteWorkRun command=HeldRunLauncherTest.COMMAND; + /** What the fake runtime says about the agent: null is "cannot tell". */ + Boolean agentRunning; final RunResult.RunWorkReady ready=new RunResult.RunWorkReady(command.runId(),command.work(),HeldRunLauncherTest.HEAD,List.of("TEST-file"),Map.of("INPUT",7L),9); final RunCommand.PublishWorkRun permit=new RunCommand.PublishWorkRun(command.runId(),new WorkPublicationPermit(command.work(),UUID.randomUUID(),ready.head(),Instant.now().minusSeconds(1),Instant.now().plusSeconds(60),List.of()),"TEST-current-scm"); final List events=new ArrayList<>(); @@ -60,6 +62,11 @@ final class Runtime extends RunLauncherTest.FakeRuntime implements PublicationRu if(lateCancel)WorkRunWorkerTest.this.cancelled=true; return WorkRunWorkerTest.this.finalization; } + @Override public boolean agentRunning(RunHandle run){ + assertEquals(command.runId(),run.runId()); + if(agentRunning==null)throw new UnsupportedOperationException("TEST runtime cannot tell"); + return agentRunning; + } @Override public void destroyHeld(RunHandle run,PublicationKey key){events.add("destroy");assertNotNull(terminal,"Record final evidence before deleting the workspace");deletions++;present=false;} } final Runtime runtime=new Runtime(); @@ -79,6 +86,52 @@ final class Runtime extends RunLauncherTest.FakeRuntime implements PublicationRu } @AfterEach void close(){worker.launcher.stopStreams();} void execute(){worker.execute(Message.of((RunCommand)command,()->{events.add("ack-command");return CompletableFuture.completedFuture(null);}),command).toCompletableFuture().join();} + /** The same held build, paid by a signed-in seat that must be in use by {@code startBy}. */ + void paidBySignIn(Instant startBy){command=new RunCommand.ExecuteWorkRun(command.execution().paidBySignIn(startBy),command.work());} + List agentStopped(){return reported.stream().filter(result->result instanceof RunResult.RunAgentStopped).toList();} + + /** + * A command that waited past its start deadline never starts: its seat may serve another build by now, + * and one sign-in serves one agent (review of PR #178). Nothing ran, so the seat is freed at once. + */ + @Test void aSignInBuildPastItsStartDeadlineNeverStarts(){ + paidBySignIn(Instant.now().minusSeconds(1));agentRunning=false; + execute(); + assertEquals(0,launches); + assertEquals("BAD_COMMAND",assertInstanceOf(RunResult.RunFailed.class,terminal).cause()); + assertEquals(List.of(new RunResult.RunAgentStopped(command.runId())),agentStopped()); + } + @Test void aSignInBuildInTimeStarts(){ + paidBySignIn(Instant.now().plusSeconds(600));agentRunning=false; + execute(); + assertEquals(1,launches); + } + /** A seat is freed only on the runtime's word that the agent is not running — never on an outcome. */ + @Test void aSignInBuildReportsItsAgentStoppedOnlyWhenTheRuntimeKnows(){ + paidBySignIn(Instant.now().plusSeconds(600)); + agentRunning=true;execute(); + assertTrue(agentStopped().isEmpty(),"an agent that may still run keeps its seat"); + } + @Test void aRuntimeThatCannotTellKeepsTheSeatHeld(){ + paidBySignIn(Instant.now().plusSeconds(600)); + agentRunning=null;execute(); + assertTrue(agentStopped().isEmpty()); + } + @Test void aStoppedAgentFreesItsSeat(){ + paidBySignIn(Instant.now().plusSeconds(600)); + agentRunning=false;execute(); + assertEquals(List.of(new RunResult.RunAgentStopped(command.runId())),agentStopped()); + } + @Test void anApiKeyBuildSaysNothingAboutSeats(){ + agentRunning=false;execute(); + assertTrue(agentStopped().isEmpty()); + } + /** An abandoned build is stopped by recovery, and its seat freed once the runtime confirms it. */ + @Test void recoveryFreesTheSeatOfAnAbandonedSignInBuild(){ + paidBySignIn(Instant.now().plusSeconds(600));state="building";agentRunning=false; + worker.recover(); + assertEquals(List.of(new RunResult.RunAgentStopped(command.runId())),agentStopped()); + } @Test void aFailedClaimCannotAcknowledgeTheCommand(){claimFails=true;assertThrows(IllegalStateException.class,this::execute);assertEquals(List.of("claim"),events);} @Test void anExecuteRedeliveryCannotRunTheHarnessAgain(){claim=false;execute();assertEquals(0,launches);assertTrue(reported.isEmpty());assertEquals(List.of("claim","ack-command"),events);} @Test void cancellationBeforeBuildBuysNoHarnessCall(){cancelled=true;execute();assertEquals(0,launches);assertEquals("CANCELLED",assertInstanceOf(RunResult.RunFailed.class,terminal).cause());} 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 1675b7e03..0c78cbaf3 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 @@ -754,6 +754,25 @@ public void cancel(RunHandle handle) { } } + /** Found by the run's label, so a unit that was never announced is still found. */ + @Override + public boolean agentRunning(RunHandle handle) { + return containerRecordOf(handle.runId(), AGENT).map(Container::getState).map(DockerRunRuntime::mayBeRunning) + .orElse(false); + } + + /** + * Docker's container states, read for "may a process still run here". A paused or restarting agent is + * alive; a created one has not started, and nothing in this arm starts it later. + */ + static boolean mayBeRunning(String state) { + if (state == null) return true; + return switch (state.toLowerCase(java.util.Locale.ROOT)) { + case "exited", "dead", "created" -> false; + default -> true; + }; + } + private void killAgent(RunHandle handle) { containerOf(handle.runId(), AGENT).ifPresent(this::killQuietly); } diff --git a/spire-runtime-docker/src/test/java/dev/codespire/runtime/docker/DockerAgentStateTest.java b/spire-runtime-docker/src/test/java/dev/codespire/runtime/docker/DockerAgentStateTest.java new file mode 100644 index 000000000..71d44acee --- /dev/null +++ b/spire-runtime-docker/src/test/java/dev/codespire/runtime/docker/DockerAgentStateTest.java @@ -0,0 +1,29 @@ +package dev.codespire.runtime.docker; + +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Which Docker states may still hold a running agent (M3.5 part F). A signed-in seat is freed only on + * "not running", so every state that is not certainly stopped must read as running. + */ +class DockerAgentStateTest { + + @Test + void onlyAStoppedOrNeverStartedContainerIsNotRunning() { + assertFalse(DockerRunRuntime.mayBeRunning("exited")); + assertFalse(DockerRunRuntime.mayBeRunning("dead")); + assertFalse(DockerRunRuntime.mayBeRunning("created")); + } + + @Test + void everythingElseMayStillRun() { + assertTrue(DockerRunRuntime.mayBeRunning("running")); + assertTrue(DockerRunRuntime.mayBeRunning("paused")); + assertTrue(DockerRunRuntime.mayBeRunning("restarting")); + assertTrue(DockerRunRuntime.mayBeRunning("removing")); + assertTrue(DockerRunRuntime.mayBeRunning(null), "an unreported state is not proof of a stop"); + } +} 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 602fe2187..2db0b8aa3 100644 --- a/spire-runtime/src/main/java/dev/codespire/runtime/RunRuntime.java +++ b/spire-runtime/src/main/java/dev/codespire/runtime/RunRuntime.java @@ -41,6 +41,23 @@ public interface RunRuntime { */ void steer(RunHandle handle, String instruction); + /** + * Whether the run's agent may still be running (M3.5 part F). + * + *

The one fact that frees a signed-in seat: a seat serves one agent, and a run reported failed can + * still have a live agent when a stop did not take. So an arm answers {@code false} only when it + * KNOWS no agent process runs — exited, never started, or gone — and {@code true} whenever it may. + * + *

A default that throws rather than answering: "not running" from an arm that did not look would + * free a seat under a live agent, and "running" would hold every seat for ever. The caller treats the + * throw as unknown and keeps the seat held. + * + * @throws UnsupportedOperationException from an arm that cannot tell + */ + default boolean agentRunning(RunHandle handle) { + throw new UnsupportedOperationException("this runtime cannot tell whether an agent is running"); + } + /** Takes everything worth keeping, BEFORE {@link #destroy}. Never destroys anything itself. */ Finalization salvage(RunHandle handle); diff --git a/spire-ui/src/api.ts b/spire-ui/src/api.ts index 46942ab93..73c246395 100644 --- a/spire-ui/src/api.ts +++ b/spire-ui/src/api.ts @@ -1700,8 +1700,10 @@ export interface HarnessCredentialView { lastUsedAt: string | null; /** How this member pays: a shared API key, or a signed-in seat that serves one build at a time. */ authMode?: 'API_KEY' | 'SUBSCRIPTION'; - /** When a seat's current lease runs out; null while no build holds it. Always null for a key. */ - leasedUntil?: string | null; + /** Whether a build's agent holds this seat now. Always false for a key, which is shared. */ + inUse?: boolean; + /** False for a seat whose account is unknown: it is never used until it is signed in again. */ + identified?: boolean; } export interface NewHarnessCredential { label: string; type: string; baseUrl: string; apiKey: string } @@ -1731,6 +1733,7 @@ export const disableHarnessCredential = (id: string) => credentialAction(id, '', export const enableHarnessCredential = (id: string) => credentialAction(id, '/enable', 'POST', 'Failed to switch the credential on'); export const clearHarnessCredentialRejection = (id: string) => credentialAction(id, '/clear-rejection', 'POST', 'Failed to clear the rejection'); export const restHarnessCredential = (id: string) => credentialAction(id, '/rest', 'POST', 'Failed to rest the credential'); +export const freeHarnessSeat = (id: string) => credentialAction(id, '/free-seat', 'POST', 'Failed to free the seat'); /** * A subscription sign-in in progress (M3.5 part F). diff --git a/spire-ui/src/components/HarnessSubscriptionSignIn.tsx b/spire-ui/src/components/HarnessSubscriptionSignIn.tsx index 8453d1433..13381b25b 100644 --- a/spire-ui/src/components/HarnessSubscriptionSignIn.tsx +++ b/spire-ui/src/components/HarnessSubscriptionSignIn.tsx @@ -15,6 +15,7 @@ const REASONS: Record = { sign_in_cancelled: 'The sign-in was cancelled.', sign_in_unit_failed: 'The sign-in tool did not start, or printed something this version cannot read. Nothing was stored.', sign_in_wrong_mode: 'That signed in as an API key, not a subscription. Add it as an API key instead.', + subscription_unidentified: 'The sign-in did not say which account it belongs to, so it was not stored. Start again.', sign_in_not_started: 'No run worker showed a code within six minutes. Check that the run worker is running and has the agent image, then start again.', }; diff --git a/spire-ui/src/components/SettingsHarnessCredentials.test.tsx b/spire-ui/src/components/SettingsHarnessCredentials.test.tsx index b6805041f..ece7bd7d3 100644 --- a/spire-ui/src/components/SettingsHarnessCredentials.test.tsx +++ b/spire-ui/src/components/SettingsHarnessCredentials.test.tsx @@ -53,8 +53,8 @@ it('tells a resting key apart from a refused one, and offers the action each nee // A signed-in seat pays for one build at a time: "In use" is the state a shared key never has (M3.5 part F). it('shows a subscription as ready, and as in use while a build holds it', async () => { vi.mocked(api.fetchHarnessCredentials).mockResolvedValue([ - member({ id: 'TEST-seat-free', label: 'TEST-seat-free', type: 'codex', authMode: 'SUBSCRIPTION', leasedUntil: null }), - member({ id: 'TEST-seat-busy', label: 'TEST-seat-busy', type: 'codex', authMode: 'SUBSCRIPTION', leasedUntil: '2099-01-01T00:00:00Z' }), + member({ id: 'TEST-seat-free', label: 'TEST-seat-free', type: 'codex', authMode: 'SUBSCRIPTION', inUse: false, identified: true }), + member({ id: 'TEST-seat-busy', label: 'TEST-seat-busy', type: 'codex', authMode: 'SUBSCRIPTION', inUse: true, identified: true }), ]); render(); @@ -111,3 +111,41 @@ it('shows the server refusal rather than a status line', async () => { render(); expect(await screen.findByRole('alert')).toHaveTextContent('Failed to load the harness credential pool'); }); + +// A worker that died for good never reports its agent stopped, so only a person can free the seat — +// and only after saying no build still uses it (review of PR #178). +it('frees a seat only after the operator confirms its build is gone', async () => { + const free = vi.spyOn(api, 'freeHarnessSeat').mockResolvedValue(undefined); + vi.mocked(api.fetchHarnessCredentials).mockResolvedValue([ + member({ id: 'TEST-seat-busy', label: 'TEST-seat-busy', type: 'codex', authMode: 'SUBSCRIPTION', inUse: true, identified: true }), + ]); + render(); + + fireEvent.click(within(await row('TEST-seat-busy')).getByRole('button', { name: 'Free seat' })); + expect(free).not.toHaveBeenCalled(); + fireEvent.click(within(await row('TEST-seat-busy')).getByRole('button', { name: 'Free it' })); + + await waitFor(() => expect(free).toHaveBeenCalledWith('TEST-seat-busy')); +}); + +// A seat whose account is unknown could be a second seat on one account; no build uses it. +it('asks for a new sign-in on a seat whose account is unknown', async () => { + vi.mocked(api.fetchHarnessCredentials).mockResolvedValue([ + member({ id: 'TEST-seat-old', label: 'TEST-seat-old', type: 'codex', authMode: 'SUBSCRIPTION', inUse: false, identified: false }), + ]); + render(); + + expect(within(await row('TEST-seat-old')).getByText('Sign in again')).toBeInTheDocument(); +}); + +it('says why a second seat of one account cannot be switched back on', async () => { + vi.mocked(api.enableHarnessCredential).mockRejectedValue(new Error('Failed to switch the credential on: subscription_account_taken')); + vi.mocked(api.fetchHarnessCredentials).mockResolvedValue([ + member({ id: 'TEST-seat-off', label: 'TEST-seat-off', type: 'codex', authMode: 'SUBSCRIPTION', enabled: false, identified: true }), + ]); + render(); + + fireEvent.click(within(await row('TEST-seat-off')).getByRole('button', { name: 'Switch on' })); + + expect(await screen.findByText('Another seat is already signed in to this account. Switch that one off first.')).toBeInTheDocument(); +}); diff --git a/spire-ui/src/components/SettingsHarnessCredentials.tsx b/spire-ui/src/components/SettingsHarnessCredentials.tsx index b64d61575..382b42f76 100644 --- a/spire-ui/src/components/SettingsHarnessCredentials.tsx +++ b/spire-ui/src/components/SettingsHarnessCredentials.tsx @@ -2,7 +2,7 @@ import { useEffect, useState } from 'react'; import { KeyRound } from 'lucide-react'; import { addHarnessCredential, clearHarnessCredentialRejection, disableHarnessCredential, - enableHarnessCredential, fetchHarnessCredentials, restHarnessCredential, + enableHarnessCredential, fetchHarnessCredentials, freeHarnessSeat, restHarnessCredential, type HarnessCredentialView, } from '../api'; import HarnessSubscriptionSignIn from './HarnessSubscriptionSignIn'; @@ -21,15 +21,23 @@ function state(member: HarnessCredentialView): { label: string; tone: string } { if (member.rejectedAt) return { label: 'Rejected', tone: 'chip danger' }; if (member.rateLimitedUntil && new Date(member.rateLimitedUntil) > new Date()) return { label: 'Resting', tone: 'chip warn' }; if (!member.enabled) return { label: 'Switched off', tone: 'chip' }; + // A seat whose account is unknown could be a second seat on one account, so no build uses it. + if (member.authMode === 'SUBSCRIPTION' && member.identified === false) return { label: 'Sign in again', tone: 'chip warn' }; // A signed-in seat serves one build at a time; "In use" is the state a shared key never has. - if (member.authMode === 'SUBSCRIPTION' && member.leasedUntil && new Date(member.leasedUntil) > new Date()) - return { label: 'In use', tone: 'chip warn' }; + if (member.authMode === 'SUBSCRIPTION' && member.inUse) return { label: 'In use', tone: 'chip warn' }; if (member.authMode === 'SUBSCRIPTION') return { label: 'Ready · subscription', tone: 'chip ok' }; return { label: 'Available', tone: 'chip ok' }; } const when = (value: string | null) => (value ? new Date(value).toLocaleString() : '—'); +/** Server refusals that have a sentence; anything else is shown as the server said it. */ +function sentence(message: string): string { + if (message.includes('subscription_account_taken')) + return 'Another seat is already signed in to this account. Switch that one off first.'; + return message; +} + /** * The keys a factory run may call the model with (FR-F12, ADR-031). * @@ -45,6 +53,8 @@ export default function SettingsHarnessCredentials() { const [error, setError] = useState(''), [notice, setNotice] = useState(''); const [adding, setAdding] = useState(false), [busy, setBusy] = useState(false); const [signingIn, setSigningIn] = useState(false); + // The seat an operator asked to free, waiting for them to confirm no build still uses it. + const [freeing, setFreeing] = useState(null); const [form, setForm] = useState({ label: '', type: 'openai', baseUrl: '', apiKey: '' }); const [refresh, setRefresh] = useState(0); @@ -61,8 +71,8 @@ export default function SettingsHarnessCredentials() { async function act(action: () => Promise, message: string) { setBusy(true); setError(''); try { await action(); reload(message); } - catch (failure) { setError(String(failure instanceof Error ? failure.message : failure)); } - finally { setBusy(false); } + catch (failure) { setError(sentence(String(failure instanceof Error ? failure.message : failure))); } + finally { setBusy(false); setFreeing(null); } } async function save() { @@ -148,6 +158,23 @@ export default function SettingsHarnessCredentials() { {when(member.lastUsedAt)} + {member.inUse && freeing !== member.id && ( + + )} + {member.inUse && freeing === member.id && ( + <> + Only if its build's worker is gone for good. + + + + )} {member.rejectedAt && ( - )} - {member.inUse && freeing === member.id && ( - <> - Only if its build's worker is gone for good. - - - - )} {member.rejectedAt && (