feat(run): create the OpenAI run-scoped provider only when the runtime needs it - #6925
Conversation
PR Summary by QodoCreate OpenAI run-scoped providers only when required
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
|
🤖 Review · Commit: |
Site previewPreview: https://325a4555-site.fullsend-ai.workers.dev Commit: |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
2457ce9 to
0d1ba26
Compare
|
🤖 Review · Commit: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
0d1ba26 to
ae42a02
Compare
|
🤖 Finished Review · ✅ Success · Started 7:20 PM UTC · Completed 7:42 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $9.85 |
|
Risk Assessment: moderate (2/5) DetailsModerate risk preserved from prior assessment. Tier 1 signals unchanged: 13 files, 827 lines, large blast radius across CLI + runtime + docs + scaffold, no protected or security-sensitive paths, feature-branch base. Tier 2 shows run.go is very hot but 4 of 13 files are brand new with zero contention, averaging out to moderate. PR C in a well-scoped 5-PR stack for security-labeled issue #6920, mitigated by incremental delivery. Previous runRisk Assessment: moderate (2/5) DetailsModerate risk preserved from prior assessment. Tier 1 signals unchanged: 13 files, 827 lines, large blast radius across CLI + runtime + docs + scaffold, no protected or security-sensitive paths, feature-branch base. 5 of 13 files are brand new with zero contention. PR C in a well-scoped 5-PR stack for issue #6920. Previous run (2)Risk Assessment: moderate (2/5) DetailsModerate risk. The change touches 13 files with 760 lines across a large blast radius (CLI + runtime + docs + scaffold), and the area has high recent churn with many distinct authors. However, 5 of 13 files are brand new (so contention is overstated by raw counts), no protected paths or security-sensitive files are changed, and the PR targets a feature branch (codex-runtime-stream-parser) giving a safe rollback path. It is PR C in a well-scoped 5-PR stack for issue #6920, with a test file ratio of 0.23. Tier 1 signals are unchanged from the prior assessment. Previous run (3)Risk Assessment: moderate (2/5) DetailsModerate risk. The change touches 13 files with 738 lines across a large blast radius (CLI + runtime + docs + scaffold), and the area has high recent churn with many distinct authors. However, several files are brand new so contention is overstated by raw counts. The PR targets a feature branch, not main, giving a safe rollback path. It is PR C in a well-scoped 5-PR stack for issue #6920, with a test file ratio of 0.23. No protected paths, security-sensitive files, CI workflows, or dependency files are changed. |
ReviewFindingsMedium
Low
Next steps:
Previous runReviewFindingsLow
Next steps:
Previous run (2)ReviewFindingsLow
Next steps:
Previous run (3)ReviewFindingsLow
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Review · Commit: |
8ce6348 to
8be89bc
Compare
|
🤖 Review · Commit: |
|
🤖 Finished Review · ✅ Success · Started 11:20 PM UTC · Completed 11:42 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high |
There was a problem hiding this comment.
Note: The following review comments could not be posted on the diff (GitHub returned 422) and are included here instead:
internal/cli/run_openai_test.go(file-level): Line 1019 · [medium] test-integrity
The TestReseedOpenAIAuth_VerifiesAndRepeatsTheSeed test script was silently broken: the grep verification case always exited 1 regardless of the condition, masked by the old seed() closure returning nil. This PR correctly fixes both the test script (explicit if/then/fi) and the production code (return lastErr instead of nil), pinned by TestReseedOpenAIAuth_UnverifiedSeedIsAnError.
Outdated: every finding from this review is addressed in later commits and all threads are resolved; dismissed so the stack can enter the merge queue in order.
c9d8436 to
98d5cff
Compare
The profile's binaries: rule decides which executable the gateway lets open a connection to api.openai.com. pi issues its requests from node, but codex's npm launcher (bin/codex.js) spawns the per-platform native binary and that process makes the calls itself — so a codex run on this profile would have every request refused by the proxy, with nothing in the agent's log to explain it. Refs #6920 Assisted-by: Claude (implementation) Signed-off-by: Wayne Sun <gsun@redhat.com>
A harness that declares `providers: [openai]` made every run on it resolve an OpenAI credential, whatever runtime was selected — so a fleet harness could not carry the provider for the runs that need it without breaking the Vertex runs that do not. The runner now materializes it only when the selected backend will actually call OpenAI: codex, whose only provider it is, or pi on a model that resolves to pi's openai provider. Otherwise the entry is skipped with a note and nothing happens: no credential is resolved, the profile is not imported, no instance is created, and the name is not attached to the sandbox, so its egress rules never open. runtime.NeedsOpenAIProvider decides, next to the runtimes, off the same effective model the plan block prints. The re-seed the refresher performs is no longer pi's. A credential refresh has to reach the running agent, and the agent process cannot follow it through the environment: OpenShell pins a placeholder to the generation it was issued for. pi solves that by re-reading auth.json per request; codex will do the same with a token file its auth command prints. Both are now expressed as runtime.OpenAICredentialSeeder — the in-sandbox file, and the sh fragment that writes the current placeholder into it — and ensureOpenAIProvider asks the selected backend for them instead of hard-wiring pi's. A backend without a seeder (Claude Code) or with a stubbed one (codex until #6920 lands its Bootstrap) leaves both empty, which already means "provider refreshed, nothing re-seeded". buildPiRunCommand's provider gates and NeedsOpenAIProvider now share piModelProvider, so the two cannot disagree about which spec is an OpenAI one. Refs #6920 Assisted-by: Claude (implementation) Signed-off-by: Wayne Sun <gsun@redhat.com>
The skip rule read the harness model only, but that is not what pi launches on: buildPiRunCommand falls back to the agent definition's frontmatter `model:` when the runner resolved none, while the decision fell back to translatePiModel's own default. Two fallback chains, and both directions were wrong. An agent whose frontmatter pins an openai model, run with no override, had its provider skipped and then failed inside the sandbox with no credential and nothing pointing at why. The inverse — FULLSEND_PI_PROVIDER=openai with a frontmatter-pinned Vertex model — created a live OpenAI credential and opened its egress route for a run that never calls OpenAI. There is now one chain: EffectiveModel(runModel, agentModel), used by buildPiRunCommand to build --model and by NeedsOpenAIProvider to decide, so the launch and the decision cannot disagree. The runner reads the frontmatter with the parser it already has (AgentDefinitionModel over parsePiAgent); an unreadable or unparseable definition resolves to the runtime default here and fails later in Bootstrap, where the message is better. Also from review: the skip line is StepInfo, not StepWarn — declaring the provider on a harness several runtimes share is the documented way to write a portable harness, so it must not bury real warnings. The reserved-profile-id rejection keeps running before the skip, and now says why: skipping first would leave a repo-controlled profile with the reserved id live on the gateway. Docs: the `fullsend-openai` profile sentence names `**/codex` alongside `**/node` and the materialization rule, and the Grok-on-Vertex note no longer claims the gate uses EqualFold. Refs #6920 Assisted-by: Claude (implementation) Signed-off-by: Wayne Sun <gsun@redhat.com>
…tiveModel Run still inlined the params.Model-then-manifest fallback in two places (model validation and the spec it hands the renderer), so the single chain the provider decision relies on held only in buildPiRunCommand. Call EffectiveModel in all three, and the invariant is structural rather than a convention three call sites have to remember. Also re-flows three comment blocks in run_openai.go that the runtime-neutral rewording left overlong or broken mid-sentence. Refs #6920 Assisted-by: Claude (implementation) Signed-off-by: Wayne Sun <gsun@redhat.com>
After writing the credential file the refresher greps it for the new placeholder, twice. Both greps failing returned success anyway, so the refresh loop recorded the new generation while the file might still name the old one — and the next rotation's settle wait would then compare against a generation the agent never held. Return the last verification error instead: the loop keeps the placeholder the agent actually holds, retries under its existing policy, and gives up loudly rather than silently drifting. The existing "verifies and repeats the seed" test never exercised a passing second verification — its stub's grep branch fell through to the trailing `exit 1`, so every verification failed and the test only passed because of this bug. The branch now exits explicitly, and a new test covers both verifications failing, through reseedOpenAIAuth and through refreshOpenAIProvider (which must return the old placeholder). Also gives CodexRuntime's two seeder stubs their own doc comments instead of one free-floating paragraph above the pair. Refs #6920 Assisted-by: Claude (implementation) Signed-off-by: Wayne Sun <gsun@redhat.com>
98d5cff to
c00acd8
Compare
|
🤖 Finished Review · ✅ Success · Started 12:22 AM UTC · Completed 1:00 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $9.93 |
|
🤖 Finished Retro · ✅ Success · Started 12:48 AM UTC · Completed 1:04 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.98 |
|
Review skipped — this PR is already merged. The Posted by fullsend post-review check |
Retro: PR #6925 — OpenAI run-scoped providerA human-authored, 13-file (739+/88−) PR that is part C of a five-PR stack for Codex runtime support (#6920). The PR introduces Timeline
Review qualityThe review agent correctly identified stale-docs drift (ADR 0092, Evidence for existing issues
Proposals filed
|
2 participants
Summary
Makes the OpenAI credential path (ADR 0092) runtime-neutral and creates the run-scoped provider only when the run needs it.
runtime.OpenAICredentialSeeder: the runner asks the selected backend for the credential-file re-seed fragment and file instead of hard-coding pi'sauth.json. pi implements it with its existing fragments; codex gets the stub filled in PR D. Backends without a seeder degrade to provider-only refresh.runtime.NeedsOpenAIProvider(backend, runModel, agentModel): a harness-declaredopenaiprovider is materialised for codex, or for pi when the effective model resolves to pi's openai provider; otherwise it is skipped with an informational note and not attached (no credential, no egress route), so fleet harnesses can declare both providers without an overlay per adopter.runtime.EffectiveModel/AgentDefinitionModel: one chain for the provider decision and pi's launch — the agent-frontmatter fallback previously diverged (a frontmatteropenai/agent had its provider skipped; a Vertex agent underFULLSEND_PI_PROVIDER=openaigot one).fullsend-openaiprofilebinaries+=**/codex(the codex native binary makes the HTTPS calls).Review rounds
sol + Grok: HIGH (frontmatter fallback divergence) fixed with the shared chain and end-to-end tests in both directions; skip note is informational, not a warning; doc sentences corrected; test gaps closed. Grok confirmed
rejectReservedProfileIDmust stay ahead of the skip.Verification (live, macOS, OpenShell gateway)
openai/gpt-5.6-lunathrough the run-scoped provider: call succeeded, provider deleted — with the model on the harness, and again with the model only in the agent frontmatter.FULLSEND_PI_PROVIDER=openaiandproviders: [openai]: informational skip, no profile import, no provider instance.Part of a five-PR stack for #6920 (Codex as an agent runtime): A image pin → B stream parser → C OpenAI credential seeder → D runtime core (ADR 0099) → E enable + docs. Each PR is reviewable on its own diff; they merge bottom-up. Plan and verified Codex facts:
research/fullsend-codex-runtime-plan.mdin the ai-workspace-public research repo (to be linked once pushed).Refs #6920
Assisted-by: Claude (implementation and review orchestration), Codex gpt-5.6-sol (review), Grok 4.6 (review)