Make a ChatGPT subscription a first-class text provider for PortOS (#5590) - #5632
Merged
Conversation
#5590) Phase 1 (#5589) taught PortOS to read the Codex account. This makes that subscription usable: the same `codex` provider record now advertises a second, opt-in text-inference capability backed by the app-server, so Brain, Identity, JIRA and the other API-only features can run on a ChatGPT plan instead of requiring an OpenAI API key. Two boundaries hold the feature together: - A text call is not a coding agent. Generic turns run in a fresh empty directory outside the checkout, under a read-only no-network sandbox with fail-closed approvals, no MCP servers and no web search, on an ephemeral thread. CoS coding tasks keep going through the existing CLI/TUI harness with the workspace they were given. - Partial output is never an answer. An interrupted or failed turn raises an error rather than handing back the text streamed so far, so a caller parsing JSON can't be given half an object. Billing source is never switched silently. The transport is off until the user enables it; the blind "first configured provider" step of the resolver stays API-only in both directions; and when quota or auth fails, only the TRANSPORT is benched — for the reset window Codex itself reported — after which the call retries on the provider's explicitly named fallback, or fails with the real reason. A caller who cancels benches nothing. Model discovery reads the app-server catalog and keeps the sentinels honest: `null` = never fetched, `[]` = a plan with genuinely no models, and a failed refresh returns the last-known-good list so one timeout can't empty the picker. Usage is attributed to the subscription with no invented dollar cost. Migration 331 stamps the capability onto an installed `codex` record without enabling it, and leaves a record repointed at another binary alone.
Two lifecycle defects found in self-review of #5590: - Two turns starting together each ran mkdtemp, and the loser's directory leaked for the life of the process because only one path can be remembered. Coalesced behind one promise, the way the connection already is. - A caller's onDelta hook shared a try/catch with the accumulator, so a throwing hook skipped the frame and the finished answer silently lost that chunk of text. The hook now has its own guard.
`providerDeclaresCodexTextTransport` restated the type + command-basename test that `isCodexSubscriptionProvider` already owns, so the account side and the inference side could disagree about which record is the ChatGPT subscription. It now composes that predicate instead.
Reviewer findings from codex and claude, most severe first. **The resolver rename was widening a coding agent's blast radius.** Aliasing `resolveAPIProvider` onto the new subscription-aware resolver changed what its existing callers receive: five of them hand the result to `promptRunner.runPromptThroughProvider`, which dispatches on `provider.type` and would have run a returned `codex` record through `executeCliRun` — the file-writing coding harness, in the PortOS checkout, with the network and the user's MCP servers. `resolveAPIProvider` is API-only again; the new `resolveTextProvider` is a separate export, used only by the one caller whose result goes to `callProviderAISimple`. **A readiness poll was unbenching a spent subscription.** `ready` is weak evidence: a per-session budget can be spent while the account's windows sit at 60%, and a quota read that merely FAILED lands there too. The Providers page polls every 15s, so a quota bench was being cleared almost immediately and the next call re-hit the same limit. Readiness no longer clears a bench at all; the bench expires on the reset time Codex reported, and an explicit sign-in or sign-out clears it early. Also fixed: - A `turn/completed` frame with no status was defaulted to success, returning whatever had streamed so far — a caller parsing JSON would get half a document as a plausible, wrong object. It now fails the turn. - The turn id is latched from the first frame that carries one, so a cancelled turn can still be interrupted when the `turn/start` response is delayed. - `model/list` pagination is followed. A truncated catalog also silently disabled the effort clamp for every model on a later page. - RPC error messages are scrubbed before they reach a log or an HTTP response — an upstream failure can quote the credential that failed. - Deltas arriving after the last completed item are no longer dropped; the progress hook honours the same turn filter the accumulator does; and the accumulator enforces the thread filter its own contract promises. - `teardown` clears request timers and only fails turns belonging to the connection that actually died. - A provider-reported reset is clamped to 24h, so a bad unit can't bench for days. - The migration stamps every Codex-command record, matching what its own comment claimed, and both seed catalogs now carry `codex-tui` to match. The read boundary is stated honestly rather than overclaimed: the envelope blocks writes, network, MCP and web search, but Codex's read-only sandbox still lets the model's shell tool read files by path. Narrowing that needs a permission profile verified against a live account — filed as #5628.
…ix more Round 2 of the local review. Both reviewers independently caught that round 1's `wasLive` scoping broke the case it was written for. **Shutdown left live turns hanging, then resurrected the child.** `stopCodexAppServer` dropped the live connection handle before terminating the child, so `teardown` no longer recognised it as live and never failed the turns running on it. An in-flight text call then waited out its full 5-minute deadline — and its cleanup, seeing a still-"running" turn, sent `turn/interrupt` through `call()`, which reconnects: a brand-new `codex app-server` spawned after shutdown to interrupt a thread that only ever existed in the dead one. The handle is now dropped after the child is stopped, and the interrupt goes to the live connection directly instead of through the reconnecting path. **The explicit fallback was the least reliable call in the system.** It posted `/chat/completions` itself, so it skipped the Ollama/MTPLX warm-ups and the LM Studio auto-load retry, and hardcoded `temperature: 0.3, max_tokens: 1000` — silently truncating a caller that had asked for 1500. It now hands the fallback back to `callProviderAISimple`, which re-enters its own HTTP path with the original prompt and options. Also fixed: - Token usage read `last` (the final model request) instead of `total`. PortOS's threads are ephemeral and carry one turn, so a turn that reasoned before answering was under-reporting by an order of magnitude. - A `turn/completed` carrying no id was accepted even with an id latched, so an anonymous frame could finish somebody else's turn and return partial text. - A failed turn's error message bypassed the redactor on its way to a log, a toast, and the caller. - The redactor missed `Authorization: Token …` (only Bearer/Basic were handled) and mangled benign prose — "The secret: sauce" came back redacted. It now requires a credential-shaped VALUE, and covers any auth scheme, JWTs, and bare vendor keys. - Hitting the model-list page cap cached a partial catalog and reported `error: null`, overwriting a complete last-known-good list with a truncated one presented as authoritative. It is an error now. - The migration's command match was case-sensitive and stripped `.cmd`/`.bat`, diverging from `commandBasename`: a `codex.cmd` record got a flag the runtime gate rejects forever, and a `Codex` record never got one at all.
Three hardening fixes from the round-3 self-review: - A turn resolved its connection per request through `call`, which reconnects. A mid-turn reconnect could not continue the thread anyway — it lives inside one app-server process — so the frames would have gone to a child that had never heard of that thread id. The whole turn now runs on one pinned connection, which also makes the cleanup interrupt unambiguous. - An anonymous `turn/completed` while an id is latched was ignored, leaving the call to wait out its full 5-minute deadline. It is malformed, so it fails the turn now. A frame naming a *different* turn is still ignored. - The message redactor clamps its input before the patterns run, so the scan is bounded by a constant whatever an upstream error quotes at us.
…action Both reviewers converged on these; the first two are user-visible. **The fallback leg orphaned its toast.** `startAIOp` mints a fresh id per call and the client tracks ops by id, so round 2's re-entry opened a second op while the first — already showing "Falling back to openai…" with an infinite duration — had nothing left to terminate it. A user who hit a quota limit was left with a spinner until reload. The handoff now carries the outer op down, so both legs are one op that ends when the fallback does. **An anonymous `turn/completed` was failing a good answer.** Round 2 tightened the id check so far that a server putting the id on the envelope rather than in the object had its completion ignored, hanging the call for the full 5 minutes and then benching the transport on a timeout. A frame that reached the accumulator by thread belongs to it — PortOS's threads are ephemeral and carry exactly one turn — so only a completion naming a *different* turn is rejected. Also fixed: - `turn/started` was not projected, so a turn that streams no deltas never latched its id and could not be interrupted when cancelled. - The redactor exempted all-letter values, which is precisely a passphrase; the 12-character floor already spares ordinary prose. Its value class also stopped at the first unusual character, leaving a tail of the secret behind. - Shutdown awaited the scratch-directory removal before stopping the child, so a handshake completing in that window published itself afterwards and the next call spawned a replacement app-server. The child now goes down first, a `stopped` flag makes a late handshake drop itself, and an in-flight `mkdtemp` is drained so its directory cannot outlive the process. - A completed sign-in — including one another Codex client started — now drops the cached model catalog, which belonged to the previous account. - `signal` reaches `postChatCompletion`, so Stop cancels the fallback leg instead of letting it run to its own timeout.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes a signed-in ChatGPT subscription usable for PortOS's text workflows. The existing
codexprovider record now advertises a second, opt-in text-inference capability backed by the Codex app-server, so Brain, Identity, JIRA and the other features that previously demanded an HTTPtype: apiprovider can run on a ChatGPT plan without an OpenAI API key. The record stays a file-writing CLI/TUI coding harness for CoS tasks — that path is untouched.Two boundaries hold the feature together:
codexTurn.jsrather than overclaimed, and narrowing it needs a permission profile verified against a live account: filed as Confine filesystem reads for Codex subscription text turns #5628.Billing source is never switched silently. The transport is off until the user enables it (
textTransportEnabled, which #5591's UI will set);resolveAPIProviderstays API-only for its existing callers, with the widerresolveTextProviderreserved for the one path that can run a subscription safely; and when quota or auth fails, only the transport is benched — for the reset window Codex reported — after which the call retries on the provider's explicitly named fallback, or fails with the real reason.Model discovery reads the app-server catalog and keeps the sentinels honest:
null= never fetched,[]= a plan with genuinely no models, and a failed refresh returns the last-known-good list. Usage is attributed to the Codex subscription family with no invented dollar cost.Migration 331 stamps the capability onto every installed Codex-harness record without enabling it.
Review
Three rounds with
codexandclaude, all findings applied. The two that mattered most were regressions the review itself caught: aliasing the resolver would have routed universe/mood-board prompts into the coding harness pointed at the PortOS checkout, and a later fix left shutdown hanging in-flight turns and respawning the app-server. Both are covered by regression tests.Test plan
cd server && npm test— 1811 files / 36886 tests greencd client && npm test— 847 files / 10628 tests greencd client && npm run lint— cleanthread/start, text and structured-output turns, cancellation and interrupt, malformed events, auth expiry, quota exhaustion and bench/fallback, catalog pagination and its sentinels, process loss and shutdown, credential redaction, the API-only resolver guard, and the migration's command matching. No real provider calls.Closes #5590