fix(pi): keep loopback models visible - #1085
Conversation
|
✅ Deterministic PR hygiene checks passed. |
|
Important Review skippedNo new commits to review since the last review. ⚙️ 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:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPi and Kimi exports now use the literal ChangesLoopback credential handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
382f424 to
ad837c9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs-site/src/content/docs/ja/reference/cli/agents.md`:
- Around line 152-160: Update the localized output summaries to describe
client-specific launch guidance rather than implying every client uses
environment exports: in docs-site/src/content/docs/ja/reference/cli/agents.md
lines 152-160, docs-site/src/content/docs/ru/reference/cli/agents.md lines
185-190, and docs-site/src/content/docs/zh-cn/reference/cli/agents.md lines
153-161, revise the surrounding summary using consistent wording that reflects
Pi’s prose “Before launching:” hint while preserving environment-variable
guidance for clients that support it.
In `@docs-site/src/content/docs/ru/reference/cli/agents.md`:
- Around line 185-190: Update the credential paragraph in the localized agents
documentation to distinguish the clients: state that opencode stores an
environment-variable reference, while Pi stores the literal `opencodex-loopback`
placeholder. Remove wording that implies the loopback reference is unused for
both clients, matching the distinction in the English source.
🪄 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: 3deb26aa-f662-4b71-8af7-922671d20dfa
⛔ Files ignored due to path filters (1)
devlog/_fin/260806_pi_loopback_models/evidence/pi-config-preview.pngis excluded by!**/*.png
📒 Files selected for processing (19)
docs-site/src/content/docs/guides/integrations.mddocs-site/src/content/docs/guides/pi.mddocs-site/src/content/docs/ja/guides/pi.mddocs-site/src/content/docs/ja/reference/cli/agents.mddocs-site/src/content/docs/ko/guides/pi.mddocs-site/src/content/docs/ko/reference/cli/agents.mddocs-site/src/content/docs/reference/cli/agents.mddocs-site/src/content/docs/ru/guides/pi.mddocs-site/src/content/docs/ru/reference/cli/agents.mddocs-site/src/content/docs/zh-cn/guides/pi.mddocs-site/src/content/docs/zh-cn/reference/cli/agents.mdgui/src/components/apikeys-workspace/ClientConfigDialog.tsxgui/tests/client-config-panel.test.tsxsrc/clients/config-export.tstests/cli-export-command.test.tstests/client-config-export-new-clients.test.tstests/client-config-export.test.tstests/client-config-new-clients.test.tstests/management-client-config-route.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs-site/src/content/docs/ja/reference/cli/agents.md (1)
154-154: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winScope the no-environment-variable claim to the generated Pi export.
The localized paragraphs say that Pi never uses environment variables, then describe Pi resolving
apiKeyenvironment references. State that the generated Pi export uses the literalopencodex-loopbackplaceholder and does not require an environment variable. Keep the separate behavior for existing Pi configurations that contain environment references.
docs-site/src/content/docs/ja/reference/cli/agents.md#L154-L154: revise the absolute Pi claim.docs-site/src/content/docs/ru/reference/cli/agents.md#L187-L190: revise the absolute Pi claim.docs-site/src/content/docs/zh-cn/reference/cli/agents.md#L155-L155: revise the absolute Pi claim.🤖 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 `@docs-site/src/content/docs/ja/reference/cli/agents.md` at line 154, Revise the absolute Pi environment-variable claim in docs-site/src/content/docs/ja/reference/cli/agents.md lines 154-154, docs-site/src/content/docs/ru/reference/cli/agents.md lines 187-190, and docs-site/src/content/docs/zh-cn/reference/cli/agents.md lines 155-155: clarify that the generated Pi export uses the literal opencodex-loopback placeholder and requires no environment variable, while preserving the separate behavior for existing Pi configurations containing environment references.Source: Path instructions
🤖 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.
Outside diff comments:
In `@docs-site/src/content/docs/ja/reference/cli/agents.md`:
- Line 154: Revise the absolute Pi environment-variable claim in
docs-site/src/content/docs/ja/reference/cli/agents.md lines 154-154,
docs-site/src/content/docs/ru/reference/cli/agents.md lines 187-190, and
docs-site/src/content/docs/zh-cn/reference/cli/agents.md lines 155-155: clarify
that the generated Pi export uses the literal opencodex-loopback placeholder and
requires no environment variable, while preserving the separate behavior for
existing Pi configurations containing environment references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 032d7207-3014-47bd-b0bc-39f1826bbc9b
📒 Files selected for processing (3)
docs-site/src/content/docs/ja/reference/cli/agents.mddocs-site/src/content/docs/ru/reference/cli/agents.mddocs-site/src/content/docs/zh-cn/reference/cli/agents.md
|
Addressed the remaining CodeRabbit outside-diff finding in 58216c4: all five locales now scope the no-env claim to the generated Pi export while preserving Pi’s behavior for existing unset env references. Docs build and privacy scan pass. |
![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: 58216c4f4b
ℹ️ 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".
|
Maintainer triage (code-level, against
Since this touches credential-destination behavior in |
58216c4 to
1397890
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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 `@docs-site/src/content/docs/guides/pi.md`:
- Around line 72-78: Update the Pi guide sections in
docs-site/src/content/docs/guides/pi.md:72-78,
docs-site/src/content/docs/ja/guides/pi.md:63-65,
docs-site/src/content/docs/ko/guides/pi.md:73-81,
docs-site/src/content/docs/ru/guides/pi.md:74-80, and
docs-site/src/content/docs/zh-cn/guides/pi.md:63-65 to consistently describe
opencodex-loopback as a non-secret literal apiKey placeholder in Pi’s normal
request format, not an admission credential or upstream provider key. State that
Pi normally calls /chat/completions with Bearer authentication, while the
loopback proxy ignores this value and requires no credential.
🪄 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: 7cbdc7e5-3404-41b8-9f97-4a4c927df2ea
⛔ Files ignored due to path filters (1)
devlog/_fin/260806_pi_loopback_models/evidence/pi-config-preview.pngis excluded by!**/*.png
📒 Files selected for processing (20)
docs-site/src/content/docs/guides/integrations.mddocs-site/src/content/docs/guides/pi.mddocs-site/src/content/docs/ja/guides/pi.mddocs-site/src/content/docs/ja/reference/cli/agents.mddocs-site/src/content/docs/ko/guides/pi.mddocs-site/src/content/docs/ko/reference/cli/agents.mddocs-site/src/content/docs/reference/cli/agents.mddocs-site/src/content/docs/ru/guides/pi.mddocs-site/src/content/docs/ru/reference/cli/agents.mddocs-site/src/content/docs/zh-cn/guides/pi.mddocs-site/src/content/docs/zh-cn/reference/cli/agents.mdgui/src/components/apikeys-workspace/ClientConfigDialog.tsxgui/tests/client-config-panel.test.tsxsrc/clients/config-export.tssrc/server/management/model-rows.tstests/cli-export-command.test.tstests/client-config-export-new-clients.test.tstests/client-config-export.test.tstests/client-config-new-clients.test.tstests/management-client-config-route.test.ts
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 `@docs-site/src/content/docs/ru/guides/pi.md`:
- Around line 74-75: В русском тексте вокруг `Bearer` уберите перенос строки
после дефиса, чтобы в отрендеренном предложении не появлялось `Bearer-
авторизации`; используйте формулировку `авторизации Bearer` или разместите
составную фразу на одной строке.
🪄 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: 706b0d8d-f410-45e9-ada9-64119bbef8e6
📒 Files selected for processing (5)
docs-site/src/content/docs/guides/pi.mddocs-site/src/content/docs/ja/guides/pi.mddocs-site/src/content/docs/ko/guides/pi.mddocs-site/src/content/docs/ru/guides/pi.mddocs-site/src/content/docs/zh-cn/guides/pi.md
|
Verdict from triage: READY pending a credential-destination security pass, since the change affects which loopback destinations models stay visible for without an env export. No code defects found; the branch is current against |
|
Reviewed in a sweep of the The bug is real and the diagnosis is exact: It also stays inside the boundary that already exists: Your remaining blocker is the security review, which your description already calls for. Anything touching credential material needs an explicit second-maintainer pass under One practical note: the head is 85 commits behind Thanks for the locale coverage on the Pi guides — documenting the placeholder in every translation rather than only the English source is what keeps the next person from "fixing" it back. |
0c61a87 to
4994b12
Compare
|
I will review the current PR head. The review will verify the Pi loopback credential boundary, generated configuration serialization, Direct-mode model filtering, and regression coverage. ✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
expected_head='4994b128197089ae007e02a5d13369cb723e9eb8'
previous_reviewed='0c61a8731e89f35461988c386ed6c716bc78bfb4'
printf '%s\n' '== PR and checkout identity =='
gh pr view 1085 --json number,baseRefName,baseRefOid,headRefName,headRefOid,files \
--jq '{number, baseRefName, baseRefOid, headRefName, headRefOid, files: [.files[].path]}'
printf 'HEAD: '
git rev-parse HEAD
git status --short
printf '%s\n' '== Incremental changes since the last reviewed head =='
git diff --name-status "$previous_reviewed...HEAD"
git diff --stat "$previous_reviewed...HEAD"
git diff --find-renames --unified=45 "$previous_reviewed...HEAD" -- \
src/clients/config-export.ts \
src/integrations/writer.ts \
src/cli/opencode.ts \
src/cli/export-command.ts \
src/server/management/model-rows.ts \
gui/src/components/apikeys-workspace/ClientConfigDialog.tsx \
tests gui/tests docs-site/src/content/docs
printf '%s\n' '== Relevant source structure =='
ast-grep outline src/clients/config-export.ts --items all --view expanded
ast-grep outline src/integrations/writer.ts --items all --view expanded
ast-grep outline src/cli/opencode.ts --items all --view expanded
ast-grep outline src/server/management/model-rows.ts --items all --view expanded
printf '%s\n' '== Pi serialization and loopback enforcement =='
rg -n -C 10 \
'LOOPBACK_API_KEY_PLACEHOLDER|opencodex-loopback|clientId:\s*"pi"|loopbackOnly|apiKeyEnv|non_loopback|buildContribution|writeFile' \
src/clients/config-export.ts src/integrations/writer.ts src/cli/export-command.ts src/server \
--glob '*.ts'
printf '%s\n' '== Direct-mode filtering and model-route helper =='
rg -n -C 10 \
'modelRoutesThroughProvider|providerCodexAccountMode|codex.*direct|direct.*codex|canonical.*openai' \
src/cli/opencode.ts src/server/management/model-rows.ts src/combos \
--glob '*.ts'
printf '%s\n' '== Focused regression coverage =='
rg -n -C 8 \
'opencodex-loopback|LOOPBACK_API_KEY_PLACEHOLDER|loopbackOnly|non_loopback|apiKeyEnv|Codex Direct|codex direct|combo|context limits|Hermes' \
tests gui/tests \
--glob '*.{ts,tsx}'
printf '%s\n' '== Pi image artifact status =='
git ls-files --error-unmatch devlog/_fin/260806_pi_loopback_models/evidence/pi-config-preview.png
file devlog/_fin/260806_pi_loopback_models/evidence/pi-config-preview.pngLength of output: 194782 🏁 Script executed: #!/bin/bash
set -euo pipefail
previous_reviewed='0c61a8731e89f35461988c386ed6c716bc78bfb4'
printf '%s\n' '== Identity =='
printf 'HEAD: '; git rev-parse HEAD
gh pr view 1085 --json headRefOid,baseRefOid --jq '{headRefOid,baseRefOid}'
printf '%s\n' '== Incremental file list =='
git diff --name-status "$previous_reviewed...HEAD"
printf '%s\n' '== Incremental implementation and test patch =='
git diff --unified=24 "$previous_reviewed...HEAD" -- \
src/clients/config-export.ts \
src/integrations/writer.ts \
src/cli/opencode.ts \
src/cli/export-command.ts \
src/server/management/model-rows.ts \
src/combos/types.ts \
tests/client-config-export.test.ts \
tests/cli-export-command.test.ts \
tests/management-client-config-route.test.ts \
tests/integrations-writer.test.ts \
gui/tests/client-config-panel.test.tsx
printf '%s\n' '== Pi client specification and serializer =='
rg -n -A45 -B12 'clientId:\s*"pi"|LOOPBACK_API_KEY_PLACEHOLDER' src/clients/config-export.ts
printf '%s\n' '== Loopback write guard =='
rg -n -A28 -B16 'loopbackOnly|non_loopback' src/integrations/writer.ts
printf '%s\n' '== Direct-mode export filtering =='
rg -n -A22 -B12 'modelRoutesThroughProvider|providerCodexAccountMode' \
src/cli/opencode.ts src/server/management/model-rows.ts src/combos/types.ts
printf '%s\n' '== Focused test cases =='
rg -n -A38 -B5 \
'opencodex-loopback|non_loopback|Codex Direct|context limits are not represented' \
tests/client-config-export.test.ts \
tests/cli-export-command.test.ts \
tests/management-client-config-route.test.ts \
tests/integrations-writer.test.ts \
gui/tests/client-config-panel.test.tsx
printf '%s\n' '== Artifact PNG signature and dimensions =='
python3 - <<'PY'
from pathlib import Path
import struct
path = Path("devlog/_fin/260806_pi_loopback_models/evidence/pi-config-preview.png")
data = path.read_bytes()
assert data[:8] == b"\x89PNG\r\n\x1a\n"
assert data[12:16] == b"IHDR"
width, height = struct.unpack(">II", data[16:24])
print(f"{path}: {width}x{height}, {len(data)} bytes")
PYLength of output: 125550
I did not execute repository tests in this sandbox. The readiness gate reports green checks, and the inspected tests cover the changed behavior. [approve] You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes.
CodeRabbit reviewed this same head. The review found no blocking defects and approved the PR. You are interacting with an AI system. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4994b12819
ℹ️ 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".
| if (omitDirectOpenAi && ( | ||
| modelRoutesThroughProvider(config, { provider: row.provider ?? "", id: row.id ?? "" }, "openai") | ||
| )) continue; |
There was a problem hiding this comment.
When canonical OpenAI in Direct mode is the only enabled provider, this predicate removes every /api/models row, after which handleExportCommand still serializes and may write a provider with zero models; the shared loadExportModels path does the same for management and integration exports. This contradicts the export contract in src/cli/export-command.ts that an empty models block is never emitted and leaves users with an apparently successful but unusable config. After applying the Direct-mode filter, return a clear no-compatible-models error instead of building or applying the document, and cover the OpenAI-Direct-only configuration.
Useful? React with 👍 / 👎.
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 `@docs-site/src/content/docs/ja/reference/cli/agents.md`:
- Line 130: Update the Japanese documentation sentence around the opencodex
provider description by changing the link text from `円周率` to the product name
`Pi`, while preserving the existing `/guides/pi/` destination and surrounding
wording.
🪄 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: 99127b7e-ab00-4bb3-b93b-35bdca9d6444
⛔ Files ignored due to path filters (1)
devlog/_fin/260806_pi_loopback_models/evidence/pi-config-preview.pngis excluded by!**/*.png
📒 Files selected for processing (30)
docs-site/src/content/docs/guides/integrations.mddocs-site/src/content/docs/guides/pi.mddocs-site/src/content/docs/ja/guides/pi.mddocs-site/src/content/docs/ja/reference/cli/agents.mddocs-site/src/content/docs/ko/guides/pi.mddocs-site/src/content/docs/ko/reference/cli/agents.mddocs-site/src/content/docs/reference/cli/agents.mddocs-site/src/content/docs/ru/guides/pi.mddocs-site/src/content/docs/ru/reference/cli/agents.mddocs-site/src/content/docs/zh-cn/guides/pi.mddocs-site/src/content/docs/zh-cn/reference/cli/agents.mdgui/src/components/apikeys-workspace/ClientConfigDialog.tsxgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/tests/client-config-panel.test.tsxsrc/cli/export-command.tssrc/cli/opencode.tssrc/clients/config-export.tssrc/combos/index.tssrc/combos/types.tssrc/server/management/model-rows.tstests/cli-export-command.test.tstests/client-config-export-new-clients.test.tstests/client-config-export.test.tstests/client-config-new-clients.test.tstests/management-client-config-route.test.ts
|
✅ Action performedReview finished.
|
|
I will review the current PR head. The review will verify Pi loopback credential handling, configuration serialization, Direct-mode model filtering, and regression coverage. ✅ Action performedReview finished.
|
|
I will review the current PR head. The review will verify Pi loopback credential handling, configuration serialization, Direct-mode model filtering, and regression coverage.
|
|
Thank you — the Pi loopback fix is landing as #1151, credited to you via Your root cause was exactly right, and it is a subtle one: Pi resolves What I did not take: the combo/direct-mode filtering, the cross-client contract changes, and the generalized export-policy churn — 31 files where the defect fix is a handful of lines. Those are worth reviewing on their own terms rather than as part of a visibility fix. One follow-up worth knowing: your change makes two assertions in Verified: 74 pass / 0 fail after rebase; full suite 9,556 pass / 0 fail; typecheck, GUI lint/build and privacy scan clean. Leaving this PR open for you. |
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.
3 participants
Summary
opencodexprovider visible without requiring an unusedOPENCODEX_API_KEYenvironment variable on loopbackRoot cause
Pi resolves
apiKeywhile building its model list and hides the entire provider when the value is an unset environment-variable reference. The integration previously emitted$OPENCODEX_API_KEY, even though Pi is loopback-only and opencodex does not require admission authentication on loopback.The generated Pi block now carries the existing non-secret
opencodex-loopbackplaceholder shared by loopback-only clients. Non-loopback integration writes remain refused.Security review
This changes credential-destination behavior and needs explicit maintainer security review. The change does not serialize a user or provider secret: the literal placeholder is accepted only for the loopback-only Pi integration, where the data-plane admission key is not checked. Upstream provider credentials remain in opencodex's own configuration.
Verification
bun run typecheckbun run test— 9,418 pass, 8 skip, 0 failbun run privacy:scancd gui && bun test tests— 646 pass, 0 failcd gui && bun run lintcd gui && bun run lint:i18ncd gui && bun run buildcd docs-site && bun install --frozen-lockfile && bun run build— 221 pagespi --list-modelschanged from 0 to 25opencodexmodels with noOPENCODEX_API_KEY; an actualxai/grok-4.5request returnedPI_OKScreenshot
The Pi config preview shows the literal loopback placeholder and the routed model array; the empty environment-variable warning is no longer rendered.
Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
New Features
opencodex-loopbackplaceholder without API-key environment variables.Bug Fixes
Documentation