fix(combos,pi): keep unknown-ladder models selectable and Pi loopback models visible - #1151
Conversation
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>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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
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 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.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 as 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.Stack 12, the final one, stacked on #1150.
Verification
bun teston the combo, config-export and CLI export suites — 165 pass then 74 pass after rebase, 0 failbun 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, built successfullybun run privacy:scan— passedgit diff --check— cleanChecklist