fix(combos,pi): keep unknown-ladder models selectable and Pi loopback models visible - #1151
Conversation
📝 WalkthroughWalkthroughThe PR changes combo effort intersections so unknown ladders act as wildcards, while runtime injection remains fail-closed. It also standardizes Pi and Kimi loopback placeholders and removes Pi secret environment export metadata. ChangesCombo effort wildcard handling
Loopback client export credentials
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 595b0e4a34
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const member: string[] = listed === undefined | ||
| ? [] | ||
| : listed.filter((effort) => effortSet.has(effort)); | ||
| if (listed === undefined) continue; |
There was a problem hiding this comment.
When a selected target has no effort metadata, this branch now preserves the other targets' efforts—or offers every effort when all ladders are unknown—but gui/src/i18n/en.ts still tells users that targets without metadata “offer none.” The visible explanation therefore contradicts the picker; revise the hint consistently across the locale files to explain the wildcard behavior.
AGENTS.md reference: gui/AGENTS.md:L14-L18
Useful? React with 👍 / 👎.
| destination: () => join(homedir(), ".pi", "agent", "models.json"), | ||
| apiKeyEnv: PI_API_KEY_ENV, | ||
| exportHint: `export ${PI_API_KEY_ENV}=<your key>`, | ||
| apiKeyEnv: "", |
There was a problem hiding this comment.
When the dashboard has no generated API keys, ClientConfigDialog unconditionally renders api.clientConfig.noKeyYet and interpolates envelope.apiKeyEnv; returning an empty string here makes the Pi dialog display a blank key name and instruct users to generate a key for off-loopback use, even though Pi is marked loopback-only and now needs no key. Gate that warning on a nonempty credential requirement, or represent placeholder-only clients explicitly.
Useful? React with 👍 / 👎.
| baseUrl: ctx.baseUrl, | ||
| api: PI_API_DIALECT, | ||
| apiKey: PI_API_KEY_ENV_REF, | ||
| apiKey: LOOPBACK_API_KEY_PLACEHOLDER, |
There was a problem hiding this comment.
After Pi starts serializing this literal placeholder, the shipped guidance still documents the removed environment-reference contract: src/cli/help.ts:181 says only Kimi uses a placeholder, while docs-site/src/content/docs/reference/cli/agents.md:176-182 and docs-site/src/content/docs/guides/integrations.md:12,85-87 still direct Pi users to OPENCODEX_API_KEY. Update those user-facing instructions and translated documentation so exported behavior and setup guidance agree.
AGENTS.md reference: AGENTS.md:L231-L232
Useful? React with 👍 / 👎.
Both remaining phases executed: #1150 (Yuxin Qiao, test isolation + Desktop allowlist docs) and #1151 (Eachann + n3wr1ch, effort picker + Pi loopback export). 16 contributor PRs landed across 12 stack PRs. Also records the one in-scope deviation: phase 140 marked tests/cli-export-command.test.ts as DROP, but the Pi fix made two of its assertions stale - they expected the exact output #1085 reports as the bug. Updating them beat shipping a red suite; the no-secret-in-stdout property is unchanged.
595b0e4 to
20a517b
Compare
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
|
Adapted from PR #1092 while preserving fail-closed runtime injection. Co-authored-by: 关俊江 <each1024@qq.com> Co-authored-by: Eachann <43348055+eachann1024@users.noreply.github.com>
Adapted from PR #1085 without direct-mode filtering or cross-client contract changes. Co-authored-by: n3wr1ch <40690535+n3wr1ch@users.noreply.github.com>
The two failing assertions encoded the behavior the fix deliberately changes: Pi no longer emits an OPENCODEX_API_KEY env reference, because Pi resolves apiKey before building its model list and hides the provider when the reference is unset - which is the defect #1085 reported. The security property under test is unchanged and still asserted: no ocx_ token ever reaches stdout. Only the expected reference form moved from $OPENCODEX_API_KEY to the non-secret opencodex-loopback placeholder. Co-authored-by: n3wr1ch <40690535+n3wr1ch@users.noreply.github.com>
Live GUI evidence for the combo effort-picker fix, captured from two Vite dev servers against the same running proxy: dev (b.png, left) and this branch (a.png, right), both with the same metadata-poor target selected (Zenmux / deepseek-v4-flash-free). Before: 'Default reasoning' offers 1 option - None (target default). After: 7 options - None plus low/medium/high/xhigh/max/ultra. The options are rendered into the page as a visible list because headless Chromium does not paint a native select popup.
20a517b to
033db63
Compare
github-actions
Bot
changed the title
github-actions
Bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/clients/config-export.ts`:
- Around line 946-954: Update buildClientConfigText to enforce the loopbackOnly
flag for the Pi provider: when the pi export’s ctx.baseUrl resolves to a
non-loopback target, reject the export instead of generating configuration.
Preserve valid loopback exports, including pi-loopback, and add coverage in the
existing client-config export and management route tests.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 851373c9-29a8-479d-834b-c6daf7b9fd3e
⛔ Files ignored due to path filters (1)
devlog/_plan/260806_stacked_bug_campaign/evidence/1151-effort-picker-before-after.pngis excluded by!**/*.png
📒 Files selected for processing (10)
gui/src/combo-workspace-data.tssrc/clients/config-export.tssrc/combos/request.tstests/cli-export-command.test.tstests/client-config-export-new-clients.test.tstests/client-config-export.test.tstests/client-config-new-clients.test.tstests/combo-workspace-data.test.tstests/combos.test.tstests/management-client-config-route.test.ts
| apiKeyEnv: "", | ||
| exportHint: "Pi reads a non-secret placeholder from models.json; loopback needs no key.", | ||
| build: buildPiClientConfig, | ||
| format: "json", | ||
| summarize: summarizePi, | ||
| buildContribution: buildPiContribution, | ||
| // No header field in Pi's provider block (and the schema is unverified | ||
| // against a real install), so there is nowhere to put the dedicated | ||
| // admission header a remote bind requires. | ||
| // No header field in Pi's provider block, so there is nowhere to put the | ||
| // dedicated admission header a remote bind requires. | ||
| loopbackOnly: true, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
ast-grep outline src/clients/config-export.ts --items all
rg -n -C 10 --glob '*.ts' \
'buildClientConfig|buildPiClientConfig|EXPORT_CLIENTS|apiKeyEnv|isLoopback|loopback|admission|baseUrl' \
src testsRepository: lidge-jun/opencodex
Length of output: 50376
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect the export spec definitions and management route usage without exceeding output limits.
wc -l src/clients/config-export.ts src/server/management/model-routes.ts src/routes.ts src/server/routes.ts src/server/.*
printf '\n--- config-export outline relevant symbols ---\n'
ast-grep outline src/clients/config-export.ts | sed -n '1,220p'
printf '\n--- relevant config-export sections ---\n'
sed -n '700,750p' src/clients/config-export.ts
sed -n '920,970p' src/clients/config-export.ts
rg -n --glob '*.ts' -C 3 'export type |api-spec|EXPORT_CLIENTS|buildClientConfig|exportClient|spec\|specs|client:\s*\{' src/clients/config-export.ts
printf '\n--- management route relevant sections ---\n'
sed -n '150,210p' src/server/management/model-routes.ts
rg -n --glob '*.ts' -C 4 'buildClientConfigText|model-routes|specs|client' src/server/management/model-routes.tsRepository: lidge-jun/opencodex
Length of output: 392
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '--- relevant config-export sections ---\n'
sed -n '1,180p' src/clients/config-export.ts
sed -n '680,760p' src/clients/config-export.ts
sed -n '910,990p' src/clients/config-export.ts
printf '\n--- targeted config-export refs ---\n'
rg -n --glob '*.ts' -C 3 'buildClientConfig|buildClientConfigText|buildPiClientConfig|EXPORT_CLIENTS|exportClient|specs|client:' src tests 2>/dev/null | sed -n '1,260p'
printf '\n--- management route relevant sections ---\n'
sed -n '1,230p' src/server/management/model-routes.ts
printf '\n--- routing files ---\n'
git ls-files src | rg 'route|management|config' | sed -n '1,200p'
printf '\n--- targeted route/management refs ---\n'
rg -n --glob '*.ts' -C 4 'config-export|model-routes|model-config|export|client' src/server src/routes tests 2>/dev/null | sed -n '1,340p'Repository: lidge-jun/opencodex
Length of output: 248
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- relevant config-export sections ---'
sed -n '1,180p' src/clients/config-export.ts
sed -n '680,760p' src/clients/config-export.ts
sed -n '910,990p' src/clients/config-export.ts
printf '%s\n' '--- targeted config-export refs ---'
rg -n --glob '*.ts' -C 3 'buildClientConfig|buildClientConfigText|buildPiClientConfig|EXPORT_CLIENTS|exportClient|specs|client:' src tests 2>/dev/null | sed -n '1,260p'
printf '%s\n' '--- management route relevant sections ---'
sed -n '1,230p' src/server/management/model-routes.ts
printf '%s\n' '--- routing files ---'
git ls-files src | rg 'route|management|config' | sed -n '1,200p'
printf '%s\n' '--- targeted route/management refs ---'
rg -n --glob '*.ts' -C 4 'config-export|model-routes|model-config|export|client' src/server src/routes tests 2>/dev/null | sed -n '1,340p'Repository: lidge-jun/opencodex
Length of output: 50375
Block non-loopback Pi exports from the shared exporter.
In src/server/management/model-routes.ts:183-186, /api/client-config builds pi with the live proxy opencodeProxyBaseUrl(...), but pi-loopback is not a loopback host unless the API port is 10100. src/clients/config-export.ts:952-954 already marks pi.loopbackOnly: true, with no field to carry x-opencodex-api-key. Reject buildClientConfigText("pi", ...) whenever src/clients/config-export.ts:701 (ctx.baseUrl) resolves to a remote target, then cover the rejection in the existing export/management route tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/clients/config-export.ts` around lines 946 - 954, Update
buildClientConfigText to enforce the loopbackOnly flag for the Pi provider: when
the pi export’s ctx.baseUrl resolves to a non-loopback target, reject the export
instead of generating configuration. Preserve valid loopback exports, including
pi-loopback, and add coverage in the existing client-config export and
management route tests.
Source: Path instructions
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 033db63074
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| baseUrl: ctx.baseUrl, | ||
| api: PI_API_DIALECT, | ||
| apiKey: PI_API_KEY_ENV_REF, | ||
| apiKey: LOOPBACK_API_KEY_PLACEHOLDER, |
There was a problem hiding this comment.
When Pi uses one of the exported native bare OpenAI rows while the canonical openai provider is configured with codexAccountMode: "direct", Pi sends this literal apiKey as its Authorization bearer; the chat-completions bridge forwards Authorization on direct native routes instead of replacing it with the main account token, so the ChatGPT upstream receives Bearer opencodex-loopback and those native Pi calls fail even on loopback. Keep the placeholder from becoming an upstream bearer for native/direct routes, or omit native/direct rows from Pi exports.
AGENTS.md reference: src/AGENTS.md:L20-L20
Useful? React with 👍 / 👎.
1 participant
Summary
Two independent visibility fixes extracted from two much larger PRs.
Effort picker. A model whose capability ladder is unknown disappeared from the combo effort picker entirely, instead of being offered with no forced default. An unknown ladder is now a wildcard for picker availability only — an explicitly empty ladder still restricts, and the runtime stays fail-closed:
concreteComboRequestBody()continues to omit the default unless the concrete target advertises support. Both halves are pinned by tests, because the useful behavior here is precisely that the picker becomes permissive while the wire does not.Pi loopback export. Pi's exported config referenced an unresolved environment variable, and Pi resolves
apiKeybefore building its model list — so it hid the provider entirely for a user who had never been told to export anything. It now ships the same non-secretopencodex-loopbackplaceholder the Kimi and Grok blocks use, with no required environment variable, since a loopback bind needs no admission key.UI change
Captured live from two Vite dev servers against the same running proxy, with the same metadata-poor target selected in both (
Zenmux/deepseek/deepseek-v4-flash-free). Left is currentdev, right is this branch:Before: 1 option —
None (target default). After: 7 options —Nonepluslow,medium,high,xhigh,max,ultra.The options are drawn into the page as a visible list because headless Chromium does not paint a native
<select>popup; the list is read from the live<option>elements, not hand-written.Attribution
Co-authored-by.Co-authored-by.Both source PRs were large — 32 and 31 files. Only the defect fix was taken from each. Dropped from #1092: catalog fallback synthesis, the public-name copy redesign,
imageInput, and locale churn (itssrc/combos/index.tsandtypes.tshunks wereimageInput, so those files are untouched). Dropped from #1085: combo/direct-mode filtering, cross-client contract changes, and generalized export-policy churn.One follow-up I did make, deliberately. The Pi change broke two assertions in
tests/cli-export-command.test.ts— a file the plan marked out of scope. They asserted the oldexport OPENCODEX_API_KEY=output, which is exactly the behavior #1085 reports as the bug. Leaving them red to respect a scope boundary would have shipped a red suite, so they were updated to the new contract. The security property they exist for is unchanged and still asserted: noocx_token ever reaches stdout.#1092 and #1085 are left open for their authors. Planning unit:
devlog/_plan/260806_stacked_bug_campaign/140_phase15_effort_picker_and_pi_export.md.Final PR of the 260806 attribution campaign; the other 12 are merged.
Verification
bun test tests/cli-export-command.test.ts tests/combo-workspace-data.test.ts tests/combos.test.ts— 71 pass, 0 fail (re-run after rebase onto currentdev)bun run test(full) — 9,556 pass, 8 skip, 0 fail across 596 filesbun run typecheck— exit 0bun run lint:gui— exit 0bun run build:gui— 214 modules transformed, builtbun run privacy:scan— passedgit diff --check— cleanChecklist
Summary by CodeRabbit
New Features
Documentation
Bug Fixes