Skip to content

fix(importers): discover Pi auth-only model providers - #1761

Merged
vastsa merged 4 commits into
vastsa:mainfrom
IswordSun:fix/pi-auth-model-import
Oct 20, 2026
Merged

vastsa merged 4 commits into
vastsa:mainfrom
IswordSun:fix/pi-auth-model-import

Conversation

@IswordSun

@IswordSun IswordSun commented Oct 19, 2026 •

Copy link
Copy Markdown

按 @vastsa 在 #1750 的意见拆分:本 PR 只包含模型导入修复,不修改任何 apps/desktop/src 或 packages/i18n 文件,符合“本轮只合入无界面改动 fix”。

修复

模型导入扫描过去只读 ~/.pi/agent/models.json,因此通过 pi /login 连接的 provider(例如 DeepSeek、Z.AI Coding CN)不会出现在已有的 Pi 分组里。

  • 新增 parsePiAuthModelConfig():读取 ~/.pi/agent/auth.json 的 API key,并与 Pi 缓存的 ~/.pi/agent/models-store.json 目录配对。
  • models.json 仍然是权威来源;只有它没有描述的 provider 才使用 auth+catalog 草稿。
  • models.json 的 env:/$VAR 在 Desktop 进程里解析不到时,用同一 provider 的 auth.json key 补齐。
  • OAuth/订阅凭据跳过;!command 不执行、不作为字面量密钥保存。
  • 同步 ADR 0179、D342、provider-model spec 和 E2E-209。

MCP 扫描的修改不在本 PR 内:那部分需要新增可见的导入来源/标签、导入面板和 9 个语言包,按照本轮范围留到允许界面改动的轮次。

验证(base 589e580,head f0d4216)

  • pnpm --filter @pi-desktop/shared exec vitest run src/model-config-import.test.ts → 24 passed
  • node --test apps/desktop/test/model-config-import.test.mjs → 7 passed
  • pnpm --filter @pi-desktop/desktop typecheck → passed
  • pnpm lint:biome → 125 files checked, no fixes
  • pnpm test:e2e:settings-inline-import → passed
  • pnpm check:pr-base → passed(origin/main 589e580 是 HEAD 的祖先)

兼容性与安全

  • 不改变 IPC/RPC、数据库 schema 或已有导入优先级。
  • 仍然只在用户显式 Scan 时读取,不自动导入(D007 不变)。
  • 密钥只进入 Electron main 的 scan cache,不进入 renderer candidate。

Related

The explicit model-config scan only read ~/.pi/agent/models.json, so
providers connected through `pi /login` (for example DeepSeek or a Z.AI
coding plan) never appeared in the Pi group. Their keys live in auth.json
and their endpoint/model metadata lives in Pi's cached models-store.json.

Read both files and pair each API-key provider with its largest coherent
endpoint group. OAuth/subscription grants stay in Pi, `!command` keys are
never executed or stored, and models.json remains authoritative when both
files describe a provider. A key in auth.json also fills a models.json
provider whose `env:`/`$VAR` reference cannot resolve in Desktop.

Tests cover the cached-catalog pairing, OAuth/command exclusion, endpoint
selection, and the scanner merge.
@IswordSun

Copy link
Copy Markdown
Author

@vastsa 按你的范围意见拆出了这个 model-only PR:不修改 apps/desktop/src 或 packages/i18n,head 已基于最新 main 589e580 并重跑了检查。原 #1750 已关闭,MCP 扫描部分留到允许界面改动的轮次再提。

@muzimu217

Copy link
Copy Markdown
Contributor

Reviewed the split in an isolated worktree. This is the right shape for the batch condition, and the core logic is the same code I reviewed in #1750.

Split is clean. git diff --name-only against main shows zero files under apps/desktop/src or packages/i18n — only the importer, the shared parser, tests, and docs. That satisfies "no UI change this round" without dropping the fix.

Tests: 30/30. apps/desktop/test/model-config-import.test.mjs 6/6, packages/shared/src/model-config-import.test.ts 24/24 (run through vitest, which is how this repo resolves the workspace imports).

The safety properties I checked on #1750 still hold here:

  • models.json stays authoritative; auth+catalog only fills providers it does not describe.
  • auth.json handling stays conservative — PI_API_KEY_AUTH_TYPES skips OAuth/subscription grants, isShellCommandSecret keeps !command values from being executed or stored literally.
  • The secret never leaves the main-process scan result: publicModelConfigCandidate still strips it, so no i18n/UI work is needed for this half.

One note for the description: this PR is the model-import half only — the MCP scan half (scanPi for ~/.pi/agent/mcp.json) that was in #1750 is not here. If it is coming as a separate PR, worth linking them so a reader does not assume Pi's MCP servers are covered by this one.

@IswordSun

Copy link
Copy Markdown
Author

Thanks. Added a Related section: the MCP half is #1750, closed for this round with the branch retained for the next round when UI changes are in scope. This PR is explicitly model-import only.

@muzimu217

Copy link
Copy Markdown
Contributor

Heads-up on why this shows no checks: all three workflows for f0d4216 are in action_required (CI, Docs check, PR base — created 10:25 UTC), which is GitHub holding a first-time contributor's runs until a maintainer approves them. Nothing is wrong with the PR; the jobs simply have not been allowed to start yet.

Since the code is unchanged from what I reviewed, I re-ran the affected suites locally on f0d4216: model-config-import 6/6 and the shared parser 24/24 — so when the runs are approved they should pass on the same code.

@vastsa — flagging in case the approval request is sitting in a queue; this one needs only the standard approval, not a re-push.

@vastsa vastsa left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

目前不建议合入:模型配置回退读取有一个会让 Pi 旧路径失效的实际问题,见行内说明。修复并补上回退路径用例后,我会继续复核。感谢把模型导入拆成无界面改动。

(await readJson(path.join(home, ".pi", "models.json")));
return parsePiModelConfig(models, env);
const agentDir = path.join(home, ".pi", "agent");
const fallbackDir = path.join(home, ".pi");

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里的 readJson(...) 返回 Promise,?? 在 Promise 被 await 之前就求值,因此左侧 Promise 永远非 null,~/.pi/models.json / auth.json / models-store.json 的回退路径实际上不会读取。旧实现先 await 再判断;当前新增测试只写了 ~/.pi/agent/,没覆盖 fallback。请把每个读取改成“await 主路径,再在结果为空时 await fallback”(可抽一个小 helper),并增加仅有 ~/.pi/ 文件的回归用例。

`readJson(...) ?? readJson(...)` compared the Promise itself, so the left
side was always truthy and ~/.pi/{models,auth,models-store}.json were
never read. Use a sequential helper that awaits each candidate before
moving on, preserving the ~/.pi/agent primary path.

Add a fallback-only regression test covering all three Pi files.
@IswordSun

Copy link
Copy Markdown
Author

确认,这确实是一个真 bug:readJson(...) ?? readJson(...) 比较的是 Promise 本身,左边永远非空,所以 ~/.pi/models.json、auth.json、models-store.json 的回退路径从未被读取。\n\n已在 76c766d 修复:新增 readFirstJson(),逐个 await 主路径后再读 fallback,保持 ~/.pi/agent 优先。并增加了仅存在 ~/.pi/ 文件的回归用例,同时覆盖 models.json、auth.json、models-store.json 三条回退。\n\n验证(head 76c766d,base 589e580):\n- model-config-import 7/7\n- shared parser 24/24\n- typecheck、lint:biome、test:e2e:settings-inline-import 均通过\n- check:pr-base 通过\n\n麻烦再复核一次。

vastsa commented Oct 19, 2026

Copy link
Copy Markdown
Owner

已在当前 head 94996ce 复核:此前指出的 Promise ?? Promise 回退缺陷已由逐条 await 修复,且新增用例覆盖仅存在于 ~/.pi/ 的 models、auth 和 model-store 文件。我在该 head 上复跑了 importer 7/7、shared parser 24/24、Desktop typecheck 和 electron-vite build,均通过;当前 main (87a48b9) 已包含在 PR base 中。

暂不合入的唯一阻塞是远端 CI、Docs check、PR-base 三个 workflow 在这个 head 上都处于 action_required(run 39266502994、39266503159、39266502950),没有实际运行结果。请维护者放行这些 workflow;通过后我会继续合入审核。感谢你及时补上真实回退路径 bug。

@muzimu217

Copy link
Copy Markdown
Contributor

Independently verified 94996ce rather than taking the summary on faith — your Promise ?? Promise fix is correct and the numbers match.

  • readFirstJson (apps/desktop/electron/main/importers/model-config.ts:38-44) awaits each candidate in order, which is exactly what the old readJson(a) ?? readJson(b) could not do: the first operand was a pending Promise (always truthy), so the fallback path was dead code.
  • Ran both suites on this head: importer 7/7, shared parser 24/24.

On the blocker: I confirmed the same thing independently — all three workflows for 94996ce are action_required (CI, Docs check, PR base), so there are no results to judge. Nothing else is outstanding from my side; the code, the scope split (no apps/desktop/src or packages/i18n files), and the tests are all clean.

@vastsa — the approval is the only step left on this one, and the local evidence above covers what the blocked workflows would have told you.

@vastsa vastsa left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

确认 #1750 拆分后的本 PR 只包含 model importer/shared parser/tests/docs,没有 apps/desktop/src 或 i18n 界面改动。

#1732 相关导入缺失是真问题:原扫描只读取 models.json,pi /login 写入 auth.json 的 provider 会被漏掉。现在通过 Pi 的缓存模型目录补齐 auth-only provider,models.json 仍优先;未解析的 env key 会回退到同 provider 的 auth key。OAuth 凭据和 !command 都不会被导入,secret 也不会进入 renderer candidate。

我在包含最新 main 的隔离候选上验证无冲突,并运行 importer 7/7、shared parser 24/24,以及 shared TypeScript build,全部通过。此前发现的 Promise fallback 问题已由顺序 await 修复,且 ~/.pi/ 三个旧路径均有回归覆盖。

@vastsa
vastsa merged commit 639f049 into vastsa:main Oct 20, 2026

vastsa commented Oct 20, 2026

Copy link
Copy Markdown
Owner

更正:我刚才正式 review 中提到的 #1732 是笔误,应为 #1750 这条 Pi 登录后 auth-only provider 导入问题;本 PR 与 #1732 的 sessionDir 问题无关。验证结果与合入结论不变,抱歉造成歧义。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants