feat(models): enforce per-account upstream cloud catalogs - #232
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements per-upstream-account cloud model catalogs by syncing GetCascadeModelConfigs for each active account, exposing the union of active catalogs in pool-wide listings (/v1/models + Dashboard), and enforcing per-account catalog checks during routing.
Changes:
- Added per-account cloud catalog storage + filtering logic in
src/models.js, including a pool-union view and a compatibility opt-out viaWINDSURFAPI_IGNORE_CLOUD_FILTER=1. - Updated account lifecycle + routing gates in
src/auth.jsto sync catalogs per active account, invalidate stale catalogs on key/account changes, and enforce catalog checks before entitlements/tier overrides. - Updated Dashboard endpoints and added a dedicated test suite to ensure union listings + per-account routing behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/cloud-catalog-filter.test.js | Adds coverage for union listings, per-account routing enforcement, fail-open behavior, and lifecycle invalidation. |
| src/models.js | Introduces per-account cloud catalog filtering, updates tier/model listing behavior, and stores catalog snapshots per account. |
| src/dashboard/api.js | Ensures Dashboard model/tier-access endpoints expose the filtered union instead of the full static catalog. |
| src/auth.js | Implements per-account catalog synchronization, catalog invalidation on lifecycle changes, and per-account routing checks. |
| README.md | Documents WINDSURFAPI_IGNORE_CLOUD_FILTER and clarifies listing vs routing behavior (CN). |
| README.en.md | Documents WINDSURFAPI_IGNORE_CLOUD_FILTER and clarifies listing vs routing behavior (EN). |
| .env.example | Adds example documentation for WINDSURFAPI_IGNORE_CLOUD_FILTER. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
评审结论:方向正确,但有 2 个阻塞问题 + 1 条声明不实,暂不合并感谢这个 PR。它修的问题(#231)是真的:静态模型表被 以下结论都经过实测,不是读描述得出的。 已验证做对的部分
阻塞问题1)(blocker)干旱模式会被打成全站中断
一旦干旱模式激活(credits 低于阈值),空白名单意味着 建议:池级调用点( 2)(major)上游"部分响应"没有守卫
上游少回一批模型 == 给用户断供,而唯一的出路是 声明不实,需要你确认PR 描述写:
这条在 master 上不复现。 我在干净 master 上实跑该文件:9 tests / 9 pass / 0 fail;在你的分支上也是 0 fail。请确认是不是本地环境残留(比如 结论修掉上面第 1 条(必须)和第 2 条(强烈建议),并更正测试声明后,我会重新评审。设计本身我认可,不需要重做。 |
复审
|
dwgx
left a comment
dwgx
left a comment
There was a problem hiding this comment.
Request changes — 设计认可,还剩 2 个问题 + 1 条声明待更正
前面两条 comment 里已给出实测细节(评审、复审 b8de001),这里是正式结论,方便你对着清单收尾。
设计层面我认可,不需要重做。 "发现用并集 + 路由用账号维度"这个分层是对的,b8de001 的非数组 fail-open 也是对的,请保留。connect 路径走独立 selector 命名空间门禁、没被这个 MODELS 命名空间过滤影响 —— 这点你处理对了,本仓库历史上"修复只覆盖部分协议路由"已经犯过两次。
待改清单
- (blocker) 干旱白名单不能被账号 catalog 过滤。
src/auth.js:201的getTierModels('free')与:229的freeTierModels都是池级语义。实测某账号 catalog 只含 sonnet 时白名单变成[],而空白名单会让isModelBlockedByDrought对每个模型返回 true —— 干旱一激活即全站中断。空白名单绝不能被解释成"什么都不许用"。 - (major) 给采纳加一道合理性门槛。 目前只在响应不可解析时 fail-open;响应是合法数组但内容不全时会被当权威目录(实测 pro 账号 130 → 6)。建议:新目录相比该账号上次目录、或相比静态表显著缩小时,拒绝采纳并
log.warn。上游少回一批模型等于给用户断供,而WINDSURFAPI_IGNORE_CLOUD_FILTER=1是用户报障之后才会想到的东西。 - 更正测试声明。 描述里写
test/account-add-proxy-ordering.test.js:320那条失败"also reproduces on master"。我在干净 master 上实跑是 9 tests / 9 pass / 0 fail,你分支上也是 0 fail。请确认是否本地环境残留(.env/ 代理设置)并更正描述。
第三条单独说一句,不是针对那条测试本身:本仓库合并前会逐条核实提交者的诊断链,所以测试结论的准确性比它是否通过更重要 —— 一条不实的声明会让整份验证结果都需要重新核对。你其余的诊断我复核后都成立,这条更正掉就没有信任成本了。
改完 ping 我,我重新跑全量 + 对抗验证。
|
感谢作者答复，b8de001 是修复 copilot 自动审阅提出的小问题。 您提出的问题清单修改如下：
对抗场景复测：首次同步即返回 Sonnet-only catalog 时，一次请求后直接采纳 1 个模型；已有 130 个模型的 last-known-good 突然缩小到 Sonnet-only 时，模型数为 补充同步范围：本 PR 没有新增定时 catalog refresh。 当前同步仍由事件触发：进程启动时执行，并在账号新增、重新激活或 API key 更换等现有生命周期事件后执行；长期保持 active 且凭证不变的账号不会在运行过程中定时重新拉取上游目录。因此，无账号事件时的上游 catalog 变更检测，以及相应的调度、退避和请求频率策略，明确属于本 PR 的 out of scope，适合后续单独处理。 验证结果：本地全部自动化测试通过，包括 |
答复质量很好,但代码还没推上来先说要紧的:PR head 仍是 也就是空白名单拦一切的问题原样还在。是漏 push 了,还是推到了别的分支?推上来我就重新审。 对四条答复本身的评价方案层面我基本认可,尤其第 1、3、4 条:
第 2 条有一处内部矛盾,需要你重新想一下你的缩小检查依赖"后续独立同步轮次返回完全相同的 UID 集合才采纳"。但你在同一条答复的补充说明里写:
这两句放在一起会得到:一个长期 active、凭证不变的账号永远不会有"后续轮次"。 于是上游真的缩减了目录(比如运维方在 Devin 侧调整了 allowlist)时,新目录被 quarantine 后永久卡在旧的 last-known-good 上 —— 直到进程重启或账号事件发生。 这个方向的错误比原来那条轻(宁可多暴露模型,也不要少),所以不是 blocker。但它意味着第 2 条的保护在最常见的稳态部署下永远不会解除。两个出路:
我倾向 1,因为它诚实且不引入新机制。但请在代码注释里写明这个限制,否则下一个读者会以为 quarantine 会自动解除。 另外:分支需要 rebase你的分支基于 推上来后我会跑全量 + 对抗验证,并按台账惯例记录。 |
|
先说清楚:你的方案我认可,不用重做。 干旱那条你想的办法(先算 catalog 和 free-tier 的交集,非空就照旧限制、空了才单独 fail-open)比我原来建议的更好,保住了干旱本来的用途。Dashboard 那个 不过有个情况得先确认一下:代码好像没推上来。 是推到别的分支了,还是本地还没 push?你说的四项改动我在分支上都没看到,所以没法审。方便的话推一下,或者告诉我在哪个分支,我去看。 剩下要做的,就这几条
这三条就是全部,我不会再往上加。除非你的新代码本身带出新问题 —— 那种情况我会明确说是新代码引起的,不会当成一直都有的要求。 另外分支需要 rebase 一下:master 现在到 v3.9.4 了(这几天连着修了几个流处理的 bug)。你动的是 推上来我就跑全量测试和对抗验证,通过就合并,台账也会补上。 |
b8de001 to
cf6ece1
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
The new pool-wide filtering path repeatedly rebuilds the active-account union per model key, which can add avoidable per-request overhead and should be optimized before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (1)
src/models.js:718
- filterModelKeysByCloudCatalog() currently calls isModelAllowedByCloudCatalog() once per key. When accountId is omitted and there are active accounts, isModelAllowedByCloudCatalog() rebuilds the active-account union set on every call via applicableCloudCatalogUids(), making pool listings O(models × union) unnecessarily expensive (notably listModels(), MODEL_TIER_ACCESS getters, and dashboard endpoints).
export function filterModelKeysByCloudCatalog(
keys = Object.keys(MODELS),
env = process.env,
accountId = null,
) {
const input = Array.from(keys || []);
return input.filter((key) => isModelAllowedByCloudCatalog(key, env, accountId));
}
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
昨晚测试中发现其他问题 #234 ，b8de001 是修复 Copilot 第一次提出的问题。 PR 正文也已按最新行为和验证结果更正。 本次修复更新如下：
验证结果：
请作者基于新的 PR head 复测。 |
There was a problem hiding this comment.
🟡 Not ready to approve
listModels({ env }) introduces env injection for filtering, but still reads a related flag from process.env, which can produce inconsistent behavior when callers supply an effective environment.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (1)
src/models.js:772
listModels()now acceptsopts.envand uses it for cloud-catalog filtering, butincludeDisabledSpecialAgentstill reads fromprocess.env. WhenlistModels({ env: effectiveEnv })is used (e.g. inhandleModels()), this makes env injection partial and can produce inconsistent behavior vs the provided env overrides.
const env = opts.env ?? process.env;
const specialAgentEnabled = opts.specialAgentEnabled ?? isSpecialAgentCatalogEnabled();
const includeDisabledSpecialAgent = opts.includeDisabledSpecialAgent
?? process.env.WINDSURFAPI_SHOW_DISABLED_SPECIAL_AGENT_MODELS === '1';
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
Copilot 最新的评论不在本 PR 修改范围内，考虑为过度防御，或者后续再单独处理。 |
合并 #232(按账号隔离上游模型目录)与 #233(CI x64 runner),外加检索端点的 5 条契约修复 —— 其中 2 条是 v3.9.4 自己引入的,含一条把 PII 写进 URL。 两条教训记入交接文档: - 读过某条约束不等于会遵守它。那条 PII 缺陷里,我两小时前才引用过 caller-key.js 的 PII 注释,然后亲手违反了它。 - 注释解释的是设计意图、不是当前事实。macOS x64 连续四个版本终态是 cancelled 而非排队,我每次照 workflow 注释解释就放过了,是外部贡献者发现的。 #234 已验证成立,进未修清单。测试 3072 → 3108,全量绿。
已合并 ✅ — v3.9.5复测通过,已用 验证记录(在 head
对抗验证(找新代码激活了什么):重点查了新增的 quarantine 定时器 —— 台账: 给 A 而不是 S 的理由我说明白:方案取舍(并集发现 + 账号维度路由)是这个 PR 最值钱的部分,你在 #231 里把三种替代方案逐一论证否决,这个论证本身就是贡献;两轮修复都精准命中;Dashboard 的 干旱那条你给的办法比我建议的好 —— 我原本想的是"池级调用点直接不过滤",你先算 #234 我已验证成立,打了 感谢,首次贡献质量很高。 |
改了什么 / What changed
GetCascadeModelConfigsper active account, expose the active-account union through Cascade model listings, and route only through accounts whose own catalog contains the requested model.DEVIN_CONNECTor special-agent routing, start Connect synchronization without waiting for Cascade confirmation, and preserveWINDSURFAPI_IGNORE_CLOUD_FILTER=1as a compatibility escape hatch.为什么 / Why
The static model table was exposed independently by API, Dashboard, and routing paths. Cascade deployments with a restricted upstream catalog could therefore advertise models that their accounts could not use.
The initial filtering implementation also kept one process-wide snapshot from the first active account. In a mixed account pool, that could hide usable models or apply one account's catalog to unrelated accounts.
This change treats the active pool listing as the union of its account catalogs, while keeping routing decisions isolated to the selected account.
Relates to #231.
行为 / Behavior
DEVIN_CONNECTor special-agent routing. Existing Connect discovery still starts from the shared model registry and its own selector catalog, as described under Out of scope.DEVIN_CONNECTselector-catalog synchronization.WINDSURFAPI_IGNORE_CLOUD_FILTER=1restores the complete static catalog.Out of scope
This PR does not add regular time-based catalog refresh for an unchanged active account. Normal synchronization remains event-driven: it runs at startup and after existing account lifecycle events such as account creation, reactivation, or API key replacement. The only timer added here is a single 30-second confirmation retry after an anomalously small non-empty response; it does not continue polling after the candidate is accepted or replaced. General refresh scheduling, adaptive backoff, request-rate policy, and persisted last-known-good state remain out of scope.
DEVIN_CONNECTuses a different selector namespace, but its discovery paths still start from the sharedMODELSregistry before applying the Connect selector snapshot/live catalog. This PR does not make/v1/modelsor Dashboard model views account-entitlement-aware, does not isolate that shared registry, does not move the drought gate into the Connect request path, and does not silently rewrite paid-model requests toswe-1-6-slow. With Connect enabled, the displayed model set therefore remains broader than the account-entitled selector set, and a confirmed Cascade catalog can still change that display when it contributes a previously unknown row to the shared registry. Connect-specific discovery, registry isolation, and enforcement are left for a separate follow-up.测试 / Testing
Automated verification:
masterv3.9.4 before verification.npm test: 3096/3096 passed.npm run test:release: all 233 test files completed with exit code 0.test/cloud-catalog-filter.test.jsandtest/cloud-catalog-backend-boundary.test.js.DEVIN_CONNECT/v1/modelsor Dashboard views, Connect mode does not trigger the Cascade-only drought fail-open warning, and Connect selector-catalog synchronization is not delayed.test/rate-limit.test.js: expected1632000, received1631999. The exact file passes locally 13/13 and the exact shard command passes locally; this branch does not modify that test or its rate-limit implementation. The failed shard is pending a CI rerun.npm run secret-scan: passed.git diff --check: passed.Manual smoke testing with a live restricted account confirmed the expected transition: the first below-threshold snapshot emitted a warning and preserved the fail-open view, then an exact confirmation about 30 seconds later applied the restricted listing. Two live accounts governed by the same policy produced identical sorted model-ID sets, and enabling both produced the same deduplicated pool union. Toggling Connect mode confirmed the existing broader Connect discovery view described above; its displayed count can change after Cascade confirmation because both paths still share the mutable
MODELSregistry. Entitlement-aware Connect discovery and registry isolation remain in the follow-up.Not run with heterogeneous live account catalogs: unique-model union, per-account routing, and removal of one account's unique models. These paths are covered by automated mixed-pool regression tests.
Checklist
CONTRIBUTING.md([Feature] Filter model catalogs per upstream account #231)