fix(model-prices): protect locally-uploaded prices from cloud auto-sync overwrite (local-first)#1241
Conversation
Previously, cloud auto-sync could silently replace user-uploaded model prices because all entries were written with the litellm source. Manual models stored by the user were indistinguishable from cloud entries, causing local-first semantics to break. Add a source parameter to track whether a price entry originates from the user (manual) or from litellm auto-sync. When auto-sync runs, manual models are now protected and skipped unless explicitly force-overwritten. Additionally: - Model names are normalized with trim before comparison, so cloud tables with leading/trailing whitespace cannot bypass the manual protection check. - When an existing price changes or the source differs, the old row is deleted before the new one is inserted, preventing orphan rows. User-visible impact: existing manually uploaded prices will no longer be overwritten by automatic cloud sync; old orphan rows from prior syncs will be cleaned up on the next updated price entry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthrough为价格导入与持久化引入 ModelPriceSource 参数（默认 "litellm"），在导入前对模型名 trim，按 source 实施冲突/保护逻辑（local/manual 不被 litellm 自动覆盖），写入时将 source 传递给 create/upsert，并为 uploadPriceTable 明确传入 "manual"；相应单元测试已扩展覆盖这些行为。 Changes模型价格来源权威性和本地优先保护
🎯 3 (Moderate) | ⏱️ ~25 minutes 可能相关的 PR：
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a local-first mechanism for model pricing, distinguishing between cloud auto-sync ('litellm') and manual user uploads ('manual') to prevent auto-sync from overwriting manually maintained prices. It also normalizes model names by trimming whitespace and deletes stale rows before updates to prevent duplicates. A critical issue was identified in the local-first protection logic, where manual uploads are incorrectly skipped instead of being treated as authoritative updates; adding a check to ensure this protection only applies when source is 'litellm' was suggested.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![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: 6c19ad9f24
ℹ️ 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".
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
src/actions/model-prices.ts (1)
183-193:⚠️ Potential issue | 🟠 Major | ⚡ Quick win本地优先跳过逻辑缺少
source检查注释（183-185 行）说明只有在
source === "litellm"时才跳过手动模型，但实际条件（187 行）并未检查source。这意味着即使source === "manual"（用户显式上传），也会跳过已存在的手动模型，导致上传无法更新用户自己之前创建的手动价格。修复建议：添加 source 检查
// 本地优先：当本次写入来自云端/自动同步（source='litellm'）时， // 跳过用户手动维护的模型，除非显式列入覆盖列表。 // 用户显式上传（source='manual'）属于权威导入，不受此保护跳过。 const isManualPrice = manualPrices.has(modelName); - if (isManualPrice && !overwriteSet.has(modelName)) { + if (source === "litellm" && isManualPrice && !overwriteSet.has(modelName)) { // 跳过手动添加的模型，记录到 skippedConflicts result.skippedConflicts?.push(modelName); result.unchanged.push(modelName); logger.debug(`跳过手动添加的模型: ${modelName}`); continue; }🤖 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/actions/model-prices.ts` around lines 183 - 193, The skip logic currently only checks manualPrices.has(modelName) and overwriteSet, but must also verify the incoming source; update the condition around manualPrices/overwriteSet (the block that pushes into result.skippedConflicts/unchanged and logs via logger.debug for modelName) to only skip when source === 'litellm' (i.e., change the if to require the source check) so that source === 'manual' can still update user-uploaded prices unless explicitly excluded by overwriteSet.
🧹 Nitpick comments (1)
tests/unit/actions/model-prices.test.ts (1)
631-650: ⚡ Quick win测试未能验证
source='manual'时跳过逻辑被禁用此测试验证了
source='litellm'（默认值）时跳过手动模型的行为，但未测试当source='manual'时不应触发跳过逻辑的场景。建议增加一个测试用例：当以source='manual'上传与已有手动模型同名的条目时，应正常更新而非跳过。这与我在
src/actions/model-prices.ts中发现的 bug 相关——当前实现缺少source检查。建议新增测试用例
it("should NOT skip manual prices when source='manual' (user re-upload)", async () => { const manualPrice = makeMockPrice("my-custom-model", { mode: "chat", input_cost_per_token: 0.123, }); findAllManualPricesMock.mockResolvedValue(new Map([["my-custom-model", manualPrice]])); findAllLatestPricesMock.mockResolvedValue([manualPrice]); deleteModelPriceByNameMock.mockResolvedValue(undefined); createModelPriceMock.mockResolvedValue( makeMockPrice("my-custom-model", { mode: "chat", input_cost_per_token: 0.456 }, "manual") ); const { processPriceTableInternal } = await import("`@/actions/model-prices`"); // User explicitly uploads with source='manual' - should update, not skip const result = await processPriceTableInternal( JSON.stringify({ "my-custom-model": { mode: "chat", input_cost_per_token: 0.456 } }), undefined, "manual" ); expect(result.ok).toBe(true); expect(result.data?.skippedConflicts).not.toContain("my-custom-model"); expect(result.data?.updated).toContain("my-custom-model"); expect(deleteModelPriceByNameMock).toHaveBeenCalledWith("my-custom-model"); expect(createModelPriceMock).toHaveBeenCalledWith("my-custom-model", expect.any(Object), "manual"); });🤖 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 `@tests/unit/actions/model-prices.test.ts` around lines 631 - 650, Add a test that verifies processPriceTableInternal allows updates when called with source='manual' (so a manual upload of an existing manual model is NOT skipped) and assert deleteModelPriceByName and createModelPrice are invoked and the model appears in result.data.updated; then fix the implementation in processPriceTableInternal to check the incoming source parameter (e.g., if source === 'manual' bypass the "skip if existing manual price" branch) so that the existing manual-price protection only applies when source is not 'manual' (preserve identifiers processPriceTableInternal, skippedConflicts, createModelPrice, deleteModelPriceByName, and findAllManualPrices).
🤖 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 `@src/actions/model-prices.ts`:
- Around line 183-193: The skip logic currently only checks
manualPrices.has(modelName) and overwriteSet, but must also verify the incoming
source; update the condition around manualPrices/overwriteSet (the block that
pushes into result.skippedConflicts/unchanged and logs via logger.debug for
modelName) to only skip when source === 'litellm' (i.e., change the if to
require the source check) so that source === 'manual' can still update
user-uploaded prices unless explicitly excluded by overwriteSet.
---
Nitpick comments:
In `@tests/unit/actions/model-prices.test.ts`:
- Around line 631-650: Add a test that verifies processPriceTableInternal allows
updates when called with source='manual' (so a manual upload of an existing
manual model is NOT skipped) and assert deleteModelPriceByName and
createModelPrice are invoked and the model appears in result.data.updated; then
fix the implementation in processPriceTableInternal to check the incoming source
parameter (e.g., if source === 'manual' bypass the "skip if existing manual
price" branch) so that the existing manual-price protection only applies when
source is not 'manual' (preserve identifiers processPriceTableInternal,
skippedConflicts, createModelPrice, deleteModelPriceByName, and
findAllManualPrices).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4778a8b-e426-40d7-aade-bd190dfc8acd
📒 Files selected for processing (2)
src/actions/model-prices.tstests/unit/actions/model-prices.test.ts
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
This PR correctly fixes a data-integrity bug where locally-uploaded model prices were silently overwritten by cloud auto-sync. The core change — adding a source parameter to processPriceTableInternal with a 'manual' default for the upload path — is sound. The delete-then-insert refactor prevents orphan rows, and the 7 new tests provide strong regression coverage. One comment-level inaccuracy was identified where the inline documentation over-promises on what the source parameter controls in the protection check.
PR Size: S
- Lines changed: 185
- Files changed: 2
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 1 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Medium Priority Issues
- [COMMENT-INACCURATE]
src/actions/model-prices.ts:185— Inline comment claimssource='manual'bypasses the protection check, but the check at lines 186-187 does not referencesourceat all. The actual bypass is throughoverwriteManual. This could mislead future developers about code behavior.
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Claude AI
Previously, the "local-first" protection skipped any model already present in manual_prices, regardless of upload source. This prevented users from re-uploading a price list to revise their own manually-maintained models (source='manual'), incorrectly treating them as conflicts. Now the skip guard only applies when the incoming source is 'litellm' (cloud sync). Manual uploads are authoritative and always allowed to overwrite. Additionally, the update path now uses upsertModelPrice for an atomic delete+insert within a transaction. This closes the crash window between delete and insert that could leave no price row, and prevents orphan rows from accumulating. The upsertModelPrice function accepts an optional source parameter (defaulting to 'manual') so it can serve both manual maintenance and batch sync scenarios. Refs #1241 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🧪 测试结果
总体结果: ✅ 所有测试通过 |
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
1 participant
问题
用户反馈：在模型价格表中创建/导入的本地模型，在后续自动同步时会被云端价格静默覆盖(无任何警告),无论该模型 ID 是否与云端已有模型相同。期望遵循「本地优先」原则——自动同步不得覆盖用户手动维护的模型。
根因
价格表合并核心
processPriceTableInternal把它写入的每一行都硬编码为source='litellm'。两个入口都经过它:upsertSingleModelPrice→upsertModelPrice,写入source='manual',受保护(合并时被跳过)。uploadPriceTable→processPriceTableInternal,写入source='litellm',不受保护。因此用户上传的本地模型对
findAllManualPrices()不可见,合并时第 177 行的本地优先跳过保护永不命中;下次自动同步(启动定时器每 30 分钟 /missing-model触发)价格不同就插入新的 litellm 行覆盖用户价格,且残留孤儿行。对话框路径本身是正确受保护的。真实 Postgres 端到端复现确认:上传模型(litellelm)价格
0.123被自动同步覆盖为0.999并残留两行;对话框模型(manual)保持不变。修复(均在
processPriceTableInternal这一处)source参数(默认'litellm',后台/云端同步行为不变)。uploadPriceTable传'manual'→ 用户显式上传的本地模型自此受本地优先保护,不再被自动同步覆盖。trim归一化后再做保护比对,防止带空白的同名键绕过保护(与 manual 入库时的trim对齐)。行为变化:管理员显式上传的价格此后不会被自动同步覆盖;旧的孤儿行会在下次该模型价格更新时清理。仍可通过「同步」按钮(带冲突确认弹窗)或重新上传刷新。
测试
tests/unit/actions/model-prices.test.ts新增 7 个用例(其中 5 个在修复前会失败):source='manual'时按 manual 入库;上传入口写入 manual;uploadPriceTable非管理员拒绝。本地全绿(worktree 内并行执行):
bun run build、bun run lint、bun run typecheck、bun run test(687 文件 / 6181 passed)。Related
source(litellm|manual) field and local-first protection for the dialog path, but missed the upload path (processPriceTableInternalalways hardcodedsource='litellm'). This PR closes that gap.🤖 Generated with Claude Code
Greptile Summary
This PR closes a local-first data-integrity gap: models uploaded via the bulk-upload UI were written with
source='litellm', making them invisible to the manual-price protection check and vulnerable to silent cloud auto-sync overwrites. The fix threads asourceparameter throughprocessPriceTableInternal, sets it to'manual'on the upload path, and replaces the fragmented delete+create update branch with a transactionalupsertModelPricecall.source === \"litellm\", so cloud auto-sync skips uploaded models while the upload path itself is fully authoritative.upsertModelPrice(delete + insert in a single DB transaction), eliminating the orphan-row accumulation and the data-loss window that existed in the prior non-atomic path.overwriteManualis now dead code on the upload path: sincesource='manual'bypasses the skip guard entirely, any list passed by the conflict-resolution UI has no effect — worth documenting or removing from the upload signature to avoid confusion.Confidence Score: 5/5
Safe to merge — the fix is well-scoped, the update path is now transactional, and 7 targeted tests cover the key scenarios including the regression guard.
The core logic change is small and correctness-preserving: the skip-guard addition is additive, the
upsertModelPricecall replaces a fragile non-atomic pattern with a transactional one, and no existing callers of the repository function are broken by the new optional parameter. The only observations are a dead-code parameter and an unnecessary type-guard, neither of which affects runtime correctness.src/actions/model-prices.ts — the
overwriteManualparameter inuploadPriceTableis now effectively ignored and worth a clarifying comment or signature cleanup.Important Files Changed
sourceparameter toprocessPriceTableInternal(defaults to 'litellm'); upload path now passes 'manual'; local-first skip guard now gated onsource === "litellm"; update branch replaced with transactionalupsertModelPrice. TheoverwriteManualparam is accepted but becomes a no-op on the upload path.upsertModelPriceto accept an optionalsourceparameter (defaults to 'manual'), enabling reuse from the bulk-sync path with 'litellm'. Existing single-model dialog callers are unaffected.upsertModelPriceMock.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[processPriceTableInternal] --> B{source param} B -->|'litellm' default auto-sync / init| C{isManualPrice?} B -->|'manual' uploadPriceTable| E{existingPrice?} C -->|yes + NOT in overwriteSet| D[skip → skippedConflicts] C -->|no, OR in overwriteSet| E E -->|no| F[createModelPrice source] E -->|yes, source differs OR price differs| G[upsertModelPrice transaction: delete + insert source] E -->|yes, same source AND same price| H[unchanged] G --> I[(DB: modelPrices)] F --> IPrompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix(model-prices): allow manual re-uploa..." | Re-trigger Greptile