feat(core,storage): add user-overridable model facts - #3129
Conversation
📝 WalkthroughSummaryThis PR adds a versioned, schema-validated It solves these problems:
Source of truthThis PR extends the existing model-fact sources. It does not replace provider inventories or generated built-in facts. The effective order is:
The Complexity deltaThe PR adds:
It removes no existing authority. Generated facts remain read-only, and provider inventories remain available. The added complexity supports custom models, field-level precedence, atomic replacement, restart recovery, unsafe-input rejection, and connection-test invalidation. No safe deletion or simplification is apparent without weakening behavior or regression coverage. Total maintenance complexity increases, but the increase is justified by the required persisted override behavior. Optional follow-up concerns include surfaced diagnostics, external-edit generation handling, dead catalog-pipeline code, precedence documentation, repeated projections, diagnostic classification, late byte-budget validation, and enabled-ID trimming. Risks and validationEffective model facts now affect model visibility, context budgeting, connection tests, execution, and behavior across restarts. Persisted replacements and external file changes can affect verification invalidation and in-flight test results. The new The tests cover provider-specific keys, field merging, capability preservation, custom models, validation, bounded persistence, malformed files, prototype-key rejection, restart recovery, cleanup, rollback, external edits, and pending-ticket supersession. The repository defines build, typecheck, lint, formatting, and Core/Storage test commands. The supplied context reports these checks as passing, but the shell output provides no direct execution results. Final required-check status therefore remains unverified. Review-relevant risksThe diff has apparent user-visible behavior and public-contract effects through model selection, catalog projection, connection testing, execution, the The diff has an apparent persistence and operational-integrity effect through atomic The diff has an apparent security effect through schema validation and prototype-key rejection. Material changes in this area require independent human review under repository policy. No licensing, release, or governance effect was identified in the current diff. The person performing the merge reviews the final diff. A maintainer makes the final determination. WalkthroughThis change adds validated model-fact overrides for metadata and capabilities. It persists overrides in ChangesModel facts override flow
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to Pricing overrides are currently rejected, so the advertised user-defined pricing behavior will not work. Merge should wait until the supported pricing fields are accepted and their merge behavior is verified. Sequence Diagram(s)sequenceDiagram
participant RuntimePolicyStoresWriter
participant RuntimePolicyCoordinator
participant ModelFactsDocumentOwner
participant CatalogSnapshot
RuntimePolicyStoresWriter->>RuntimePolicyCoordinator: replaceModelFacts(overrides)
RuntimePolicyCoordinator->>ModelFactsDocumentOwner: replace(root, overrides)
ModelFactsDocumentOwner-->>RuntimePolicyCoordinator: validated ModelFactsDocument
RuntimePolicyCoordinator->>CatalogSnapshot: apply model-fact overrides
CatalogSnapshot-->>RuntimePolicyStoresWriter: projected runtime-policy result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 21de45db-4119-444a-a30e-50161c8d0fb3
📒 Files selected for processing (15)
packages/core/package.jsonpackages/core/src/__tests__/model-catalog.test.tspackages/core/src/__tests__/model-facts.test.tspackages/core/src/__tests__/runtime-policy-codec.test.tspackages/core/src/model-catalog.tspackages/core/src/model-facts.tspackages/core/src/runtime-policy/connection-catalog-codec.tspackages/storage/src/__tests__/model-facts-store.test.tspackages/storage/src/__tests__/runtime-policy-model-facts.test.tspackages/storage/src/__tests__/runtime-policy-stores.test.tspackages/storage/src/index.tspackages/storage/src/model-facts-store.tspackages/storage/src/runtime-policy-stores.tspackages/storage/src/runtime-policy/coordinator.tspackages/storage/src/runtime-policy/document-io.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.
|
Final revision is ready for maintainer review. Addressed both CodeRabbit findings in one follow-up commit: prototype-key rejection now fails closed, and model-fact replacement now invalidates verification before persistence while superseding in-flight connection-test tickets. Verification is complete: all GitHub checks pass, CodeRabbit reports no actionable comments, and the PR is mergeable. Local verification also passed Core 540/540 and the focused model-facts suites; Storage passed 783 tests with 14 platform skips, with the one unrelated bundled-Git smoke failure documented in the PR description. |
|
Thanks for the rework — I verified the architecture is right: read-time projection (overrides never enter the persisted catalog, so session history stays clean — sessions store slug+modelId and re-resolve at execution), user > provider > metadata precedence with field-level capability merge, protected id, schema-validated fail-closed file, atomic Conclusion: PASS with two P2s that need handling or an explicit deferral note. P2-1 — the only production entrypoint is hand-editing the JSON file, and a bad file silently disables all overrides with zero diagnostics. P2-2 — the invalidation semantics only hold for the (uncalled) API path, not for the file-edit path that is the real entrypoint. P3 (optional): the AI-assisted review disclosure: this review was produced with AI assistance (pi review subagent on 中文摘要（AI 辅助审查）结论：PASS（2 个 P2 需处理或显式延后）。架构正确：读时投影（覆盖不进持久化 catalog、Session 历史干净——session 只存 slug+modelId 执行时重解析）、优先级 user>provider>metadata+字段级 capability merge、id 受保护、schema 校验 fail-closed、replaceModelFacts 原子写，执行路径真打通（resolveExecutionConnection 投影后 resolveSelectedModelContextWindow 取覆盖值，headline 场景"覆盖 window 而非 16K 回退"成立）；第二 commit 的 fail-closed clear-then-write 竞态修复与原型污染防御正确；优先级顺序符合 issue 跨工具调研共识。CI 14/14 绿。P2-1：唯一生产入口是手改 JSON，坏文件静默失效全部覆盖零诊断——readWithDiagnostics 返回 diagnostic 但所有消费者只取 .document.overrides，且 grep 确认 runtime-host/cli/ui 零引用 modelFacts/replaceModelFacts（API 无生产调用者）；打错一个字符所有覆盖静默消失，#2330 的"坏文件→error surfaced、内置照常"只完成一半。至少：投影读路径 warn 日志 + "坏文件经 coordinator 投影后覆盖禁用"集成测试（现只测到 store 层）。P2-2：作废语义只对（无人调用的）API 路径成立——modelFactsGeneration 仅 replaceModelFacts 递增，文件外部编辑不改变；sameConnectionTestModelBasis 只比较 id+apiProtocol，仅改 contextWindow 的编辑保留过期 lastTest: verified 且不拦截飞行中 connection test（后者以旧事实测试结果提交到新事实下）。建议：读路径用文件 mtime/stat 派生 generation，或显式声明"文件编辑为 advisory，作废仅经 replaceModelFacts"；另 replaceModelFacts 对所有连接（含无关 provider）清 lastTest 属过度作废，建议按受影响 provider/model 收窄。P3（可选）：model-catalog.ts 的 modelFactOverrides 管线在生产是死代码（无生产调用者，桌面模型菜单走 storage 投影）——若后续 UI PR 不消费应删除（~240 行）；webSearch 能力透传对未覆盖的 provider 模型也是行为变更（此前被丢弃现在出现在 catalog）未声明；与 relayModelProfiles 双权威（relay 连接上 relay profile 静默压过全局文件的重叠字段）优先级未文档化；每次 catalog 快照/resolve/test-start 都重读+refreeze 全量投影（可加 stat/mtime 缓存）；错误分类依赖 error.message.includes('exceeds') 脆；512 覆盖×2048 字符可能超 256KB 字节上限且发生在 prepareReplacement 清 lastTests 之后→commit_outcome_unknown（验证已丢覆盖未写入），字节预算应在 prepare 阶段；未 trim 的 enabledModelIds 原样 push 而 lookup trim——空白 id 产生脏条目。 |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The field-level precedence (user > provider > metadata), immutable persisted authority, atomic replacement, generation tickets, and execution projection are coherent. Existing public threads already cover malformed-file diagnostics and external-edit invalidation, so I am not duplicating them.
One additional transaction-ordering issue remains below. The first-principles rule is that the complete candidate document—including serialized byte budget—must be validated before any dependent state is invalidated. More broadly, a content fingerprint can be the single authority for both external-edit generation and test invalidation, instead of maintaining parallel API-only generation semantics.
Review performed with three Codex reviewer agents and DeepSeek V4 Flash as advisory tools; I reproduced the finding on the latest head. The branch also needs rebasing because GitHub currently reports conflicts with main.
中文评论
字段级 precedence（user > provider > metadata）、不可变持久权威、原子替换、generation ticket 和执行投影整体闭环。现有公开线程已覆盖 malformed-file diagnostic 与外部编辑失效，我不重复。
以下仍有一个额外的事务顺序问题。第一性原理是：完整候选文档（包括序列化字节预算）必须在任何依赖状态失效之前完成验证。更进一步，可用内容 fingerprint 作为外部编辑 generation 与测试失效的单一权威，避免维护只对 API 写入生效的并行 generation 语义。
本次审查使用了三位 Codex reviewer agents 与 DeepSeek V4 Flash 作为辅助工具；我已在最新 head 上复现该问题。当前 GitHub 还显示分支与 main 冲突，需要 rebase。
|
/agentic_review |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
6fc9c1d to
c5c2fa1
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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ba670128-8656-4efd-a2c1-b5feb82a2d33
📒 Files selected for processing (15)
packages/core/package.jsonpackages/core/src/__tests__/model-catalog.test.tspackages/core/src/__tests__/model-facts.test.tspackages/core/src/__tests__/runtime-policy-codec.test.tspackages/core/src/model-catalog.tspackages/core/src/model-facts.tspackages/core/src/runtime-policy/connection-catalog-codec.tspackages/storage/src/__tests__/model-facts-store.test.tspackages/storage/src/__tests__/runtime-policy-model-facts.test.tspackages/storage/src/__tests__/runtime-policy-stores.test.tspackages/storage/src/index.tspackages/storage/src/model-facts-store.tspackages/storage/src/runtime-policy-stores.tspackages/storage/src/runtime-policy/coordinator.tspackages/storage/src/runtime-policy/document-io.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/storage/src/index.ts
- packages/storage/src/tests/runtime-policy-stores.test.ts
- packages/core/src/tests/runtime-policy-codec.test.ts
- packages/core/package.json
- packages/core/src/tests/model-facts.test.ts
- packages/storage/src/model-facts-store.ts
- packages/storage/src/runtime-policy-stores.ts
- packages/core/src/tests/model-catalog.test.ts
- packages/core/src/runtime-policy/connection-catalog-codec.ts
- packages/core/src/model-catalog.ts
- packages/storage/src/runtime-policy/coordinator.ts
- packages/storage/src/tests/model-facts-store.test.ts
Included review availability: Your plan provides up to 3 included reviews per hour; 2 remain after this review.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The immutable facts document, field-level projection, generation tickets, and execution resolver are coherent on this head; I also verified that the execution resolver applies overrides before returning, so an initially suspected execution bypass was discarded. One normal refresh path still removes the headline override-only model state.
The inline P2 is the smallest remaining product correction: model fetch reconciliation needs to preserve currently enabled IDs backed by model facts without writing them into provider inventory. The current live typecheck is also failing, so this head is not merge-ready independently of the review finding.
AI-assisted review by Codex with three independent reviewer passes and OpenCode Go DeepSeek V4 Flash (high) as an advisory pass; I verified the current head, refresh reconciliation, projection semantics, existing threads, and live CI.
中文
当前 head 的不可变 facts document、字段级 projection、generation ticket 与 execution resolver 整体闭环；我也确认 execution resolver 返回前确实应用 overrides，因此剔除了一个初步误报。仍有一个正常刷新路径会删除核心的 override-only model 状态。
行内 P2 是最小剩余修复：model fetch reconciliation 应保留当前已启用且由 model facts 支撑的 ID，但不要把它写进 provider inventory。当前实时 typecheck 也失败，因此即使不考虑 finding，这个 head 也尚不可合并。
本次由 Codex、三个独立 reviewer 与 OpenCode Go DeepSeek V4 Flash high 辅助；已核对当前 head、刷新 reconciliation、projection semantics、已有线程和实时 CI。
| return deepFreeze({ kind: 'committed' as const, snapshot }); | ||
| return deepFreeze({ | ||
| kind: 'committed' as const, | ||
| snapshot: await this.projectCatalogSnapshot(root), |
There was a problem hiding this comment.
[P2] Preserve enabled override-only models across provider refresh
By the time this projected snapshot is returned, writeModelFetchResult has already reconciled enabledModelIds strictly against the fetched inventory. An enabled custom model backed by model-facts.json is therefore removed (and may lose the default) whenever the provider returns only its live inventory; the projection here cannot add it back because it is no longer enabled. Please pass the current facts-backed IDs into model-fetch reconciliation, preserve only those already enabled/defaulted, and add a refresh regression proving the custom model remains selectable and executable.
中文
返回这个 projected snapshot 时，writeModelFetchResult 已经只按 provider inventory 重算 enabledModelIds。由 model-facts.json 支撑的已启用 custom model 会在刷新时被删除，默认项也可能被切换；此时 projection 因该 ID 已不再 enabled，无法把它加回来。请把当前 facts-backed IDs 传入 reconciliation，只保留已经 enabled/default 的部分，并补刷新后仍可选择与执行 custom model 的回归。
c5c2fa1 to
8c592fd
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The current head is semantically equivalent to the previously reviewed implementation plus rebase/formatting. I verified that the oversized-input validation, sparse-modality validation, and post-publication generation invalidation are now covered; I also resolved the pricing thread because #2330 explicitly keeps pricing in the existing pricing editor rather than duplicating it in model-facts.json.
The existing provider-refresh P2 remains current and unresolved: refresh reconciliation still removes an enabled/default override-only model before the facts projection runs. No duplicate inline is needed. This also changes user-visible model selection by making override-only models selectable, but the PR has no visual evidence; please add an actual TUI or Desktop model-selector screenshot showing a facts-backed custom model. Exact-head CI has not run yet because the Apache Actions lanes are unavailable/queued.
AI-assisted review by OpenAI Codex. I verified the exact-head range-diff, current implementation and tests, issue #2330’s ownership decision, review-thread state, screenshot gate, provenance, and live checks.
中文
当前 head 与上一轮相比主要是 rebase/format；oversized input、稀疏 modalities 和发布后 generation 失效均已有修复。pricing 线程不成立，因为 #2330 明确要求 pricing 继续由现有 pricing editor 负责，不应重复进入 model-facts.json。
已有的 provider refresh P2 仍然有效：刷新会在 facts projection 前删除已启用/默认的 override-only model，因此不重复发 inline。该改动会让自定义模型出现在用户选择器中，也需要补一张真实 TUI 或 Desktop 模型选择器截图。当前 exact-head CI 尚未实际运行。
本次由 OpenAI Codex 辅助，已核对 exact-head range-diff、实现与测试、#2330 权威边界、线程状态、截图门禁、来源披露和实时检查。
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
8c592fd to
f321119
Compare
|
Implemented and reviewed the provider-refresh correction in one follow-up commit: f321119.\n\n- Refresh reconciliation now preserves only facts-backed model IDs that are already enabled or are the connection's current default.\n- Raw provider inventories remain limited to live discovery results; custom facts are projected read-time.\n- The default target remains valid and the execution resolver still resolves the selected custom model.\n- Added Core and Storage regressions covering refresh, default retention, execution, inventory purity, and exclusion of unselected facts-backed models.\n- Rebased onto current main.\n\nVerification on the rebased head:\n- Core typecheck passed.\n- Storage typecheck passed after rebuilding Core.\n- Biome check passed for all changed files.\n- git diff --check passed.\n- Core reconciliation tests: 9 passed.\n- Storage model-facts tests: 6 passed.\n- Storage runtime-policy-stores tests: 44 passed.\n- Independent adversarial review found no actionable defect.\n\nPricing remains intentionally outside model-facts.json, per issue #2330's authority decision; the existing pricing editor remains the pricing source of truth. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — the storage half of this is genuinely careful, and I want to name that before the findings. Reviewed exact head f321119a7f2ebf3576e1a9bc9decfa2ea8326249.
The store gets the hard mechanics right: atomicity and temp-file recovery, commit_outcome_unknown on both the pre-clear and post-publication failure paths, oversize rejection that preserves verification, external-edit ticket supersession, and __proto__ rejection — with the key grammar handling colon-bearing model ids like ollama-cloud:gpt-oss:120b correctly, because only the provider segment is colon-free. The numeric validator is tighter than the codec's and rejects zero, negatives, non-integers and infinities. Most importantly, no write path reads a projected snapshot and writes it back, so overrides never contaminate connection-catalog.json — that is what keeps a stale pin recoverable, and there is an explicit assertion for it. Pricing is not reachable from this PR at all, since the override allowlist excludes price fields and pricing has its own authority; the wrong-number risk here is truncation, not cost.
The problem is the precedence chain. Context window now has three authorities — relayModelProfiles, this new fact table, and bundled metadata — and resolveSelectedModelContextWindow consults the first one first and returns early, so on an openai-compatible connection a model fact silently loses to a setting the user may have configured months earlier, with no diagnostic. RelayModelProfile is the pre-existing per-model user-declaration seam with a documented gate; the project rule is to extend the closest existing seam rather than run a parallel one beside it, and that is what happened here. Separately, the PR threads a second, entirely dead projection path through the catalog builders, which is why the one provenance signal it adds can never fire.
Two P1s, six P2s and three P3s inline. Not approving while P1/P2 findings are open.
On tests: the storage suite is strong on mechanics and empty on precedence. Nothing asserts a fact override against a competing authority, nothing drives capabilitySource through the call shape production actually uses — which is precisely why the dead path went unnoticed — nothing covers a contextWindow override against a row carrying the inputLimit this same PR starts persisting, and nothing covers a non-1 schemaVersion, a concurrent replace, or an override for a model that no longer exists. Each of those inherits its severity from the finding it belongs to rather than standing on its own.
Review disclosure: this review was prepared with Claude Code, which read the diff at this head, enumerated the reader sites and the precedence chain by search rather than by assumption, and executed several of the pure functions against verbatim copies to check the merge semantics. Evidence grade is stated per finding — three were reproduced by execution, the rest are code reading or labelled inference. The human contributor reviewed this before posting.
| : metadata.capabilities | ||
| ? 'static_catalog' | ||
| : 'unknown', | ||
| capabilitySource: lookupModelFactOverride( |
There was a problem hiding this comment.
[P1] This projection path has no production caller, so capabilitySource: 'user_override' can never fire. The ~94 lines threading modelFactOverrides through the catalog builders are reached only from model-catalog.test.ts; a search over the whole tree at this head finds no other caller. Production instead arrives here with a connection whose models[] was already merged by the storage-side projection in coordinator.ts, so input.modelFactOverrides is always undefined, lookupModelFactOverride returns undefined, and the ternary falls through to 'provider_api'. Concretely: a user writes {"openai:gpt-5":{"contextWindow":32000,"capabilities":{"vision":false}}}, and Settings shows 32 000 attributed to the provider API. The user cannot distinguish their own pin from a provider report — which is exactly the signal they need months later when the provider has corrected the value and the pin is now the stale one. Either delete this path and derive provenance inside the storage projection (project a factOverriddenFields set alongside models[]), or wire the real callers. The regression test has to go through the same call shape production uses; the two existing tests pass modelFactOverrides directly, which is why this was invisible.
| ); | ||
| } | ||
|
|
||
| private async readModelFacts(root: string) { |
There was a problem hiding this comment.
[P1] Make this pure — a read must not perform a durable catalog write. readModelFacts calls clearAllConnectionLastTests on a fingerprint change, which is a real write that bumps every connection's revision, and failures surface as commit_outcome_unknown. It is reached from projectCatalogSnapshot, getModelFacts, and resolveExecutionConnection — that last one being on the send path. Concretely: a user with six verified connections hand-edits model-facts.json to fix one Ollama model's window; the next resolveExecutionConnection for an unrelated Anthropic connection wipes verification on all six, which then render as not verified, and if the catalog write fails the user's send fails with a commit-unknown error rather than degrading to a stale read. Two problems compounded: the write happens on a read, and the invalidation is global when the change is per-model. Move the generation bump and the invalidation into an explicit write-path reconcile, and scope it to connections whose projected model set actually changed. Note also that openInteractiveRuntimePolicyStoresForRead builds this coordinator on a read-access lease and runWithStorageRootLease only checks identity rather than blocking writes — that variant has no production caller today, so the read-lease angle is inference, but it is the kind of assumption worth not leaving lying around.
| throw new Error('Invalid apiProtocol'); | ||
| result.apiProtocol = value.apiProtocol; | ||
| } | ||
| for (const key of ['contextWindow', 'inputLimit', 'maxOutputTokens'] as const) { |
There was a problem hiding this comment.
[P2] An override that raises contextWindow is silently discarded by the min against inputLimit. This PR newly persists inputLimit on stored model rows and newly lets a user override contextWindow, but resolveSelectedModelContextWindow takes narrowestPositiveLimit(model.contextWindow, model.inputLimit), and applyModelFactOverride sets only contextWindow and leaves the stored inputLimit alone. Reproduced by execution against verbatim copies: row {contextWindow: 8192, inputLimit: 8192} plus override {contextWindow: 200000} yields an applied row of {contextWindow: 200000, inputLimit: 8192} and a truncation budget of 8192 — while the catalog entry reports 200 000. So the number the user sees and the number that governs truncation disagree, which is the single worst outcome for a feature whose purpose is to correct that number. When an override sets contextWindow, clear or raise the row's inputLimit in applyModelFactOverride. Regression test: exactly the pair above. That a discovery adapter emits inputLimit today is inference — only the generated metadata sets it now — but the codec is asserted to pass it through, so the row shape is reachable.
| return deepFreeze({ | ||
| kind: 'ready' as const, | ||
| connection: structuredClone(connection), | ||
| connection: applyModelFactOverridesToConnection( |
There was a problem hiding this comment.
[P2] Pick one authority for a user-declared context window. Merging the fact override into models[] here does not make it win: resolveSelectedModelContextWindow consults relayModelProfile(connection, modelId)?.contextWindow first and returns early, so on an openai-compatible connection the older setting silently beats the newer fact. Concretely: a user sets relayModelProfiles['m'].contextWindow = 8192 in settings, later writes {"openai-compatible:m":{"contextWindow":200000}} in model-facts.json, and truncation stays at 8192 while the model picker shows 200 000 — no warning, no precedence documented anywhere. RelayModelProfile is the pre-existing per-model user-declaration seam and it already has a documented gate, so generalising it was the closest existing seam; running a second override table beside it is the thing the project's own rule asks us not to do. At minimum make resolveSelectedModelContextWindow the single resolver for both and state the order in one place — but the better answer is one table, not two.
| .map((key) => key.slice(prefix.length)); | ||
| } | ||
|
|
||
| export function decodeModelFactsDocument(value: unknown): ModelFactsDocument { |
There was a problem hiding this comment.
[P2] Distinguish an unsupported future version from a malformed file, and refuse to write over one. decodeModelFactsDocument requires schemaVersion === 1 exactly; readWithDiagnostics swallows the throw into diagnostic: 'malformed' with an empty document; and replaceModelFacts then overwrites the whole file with a v1 document. So a user who runs a newer build that writes schemaVersion: 2 and then reverts to this one loses every override silently — the only signal is a process.emitWarning — has all their connection verification cleared on the next read, and has the v2 data destroyed by the first write. Version downgrade is a normal thing for users on a release channel to do, and this is a one-way door. Treat an unknown-but-parseable version as a refusal to write rather than as corruption, and add a regression test that a schemaVersion: 2 document survives a replace.
| } | ||
| } | ||
|
|
||
| async replace(root: string, overrides: ModelFactOverrides): Promise<ModelFactsDocument> { |
There was a problem hiding this comment.
[P2] Adopt the revision-checked shape the sibling override store in this same package already uses. replace(root, overrides) swaps the whole table with no expected revision, so two windows that each read the table, each add one override, and each call replace silently lose the first one's entry — and getModelFacts returns no revision, so no caller can even detect the loss. packages/storage/src/pricing-store.ts is the existing user-override authority right next door and it does this correctly: upsert(expectedRevision, ...) / delete(expectedRevision, ...) with a PricingRevisionConflictError. Following it removes the lost-update entirely, and per-key writes also make the 256 KiB whole-document byte check unnecessary.
| const PROVIDER_MODEL_KEY_PATTERN = /^([^:\s]{1,128}):([^\s]{1,256})$/; | ||
| const MAX_FACT_NUMBER = 10_000_000_000; | ||
|
|
||
| export function modelFactKey(providerType: ProviderType | string, modelId: string): string { |
There was a problem hiding this comment.
[P2] Validate the provider segment against the registered providers, or report keys that matched nothing. modelFactKey types the provider as ProviderType | string and checks only that it has no colon, no whitespace and is at most 128 characters — so "anthropic:claude-opus-5" is happily stored for a connection whose providerType is anthropic-api, and the override then never applies anywhere. The document validates, getModelFacts reads it back intact, and the only symptom the user gets is that nothing changed — for a hand-edited JSON file, which is the primary way this feature is used, that is a bad failure mode. Either reject unknown provider segments on write, or have getModelFacts report unmatched keys so the UI can say which overrides are inert.
| ...(override.capabilities === undefined | ||
| ? {} | ||
| : { capabilities: { ...model.capabilities, ...override.capabilities } }), | ||
| ...(override.modalities === undefined ? {} : { modalities: override.modalities }), |
There was a problem hiding this comment.
[P3] Merge modalities per direction, or document that they replace wholesale. capabilities merge field by field but modalities replace the whole object, and nothing in the type says so. Reproduced by execution: a row with {input: ['text','image','pdf'], output: ['text']} plus an override of {input: ['text'], output: ['text']} drops image and pdf — permanently, including any input modality the provider adds later. A user who only wanted to remove pdf has silently also removed image, and since the whole point of the pin is that it survives provider updates, they will not find out from a later correction. Per-direction merge matches how capabilities already behave and is what a reader would expect.
| return { schemaVersion: MODEL_FACTS_SCHEMA_VERSION, overrides }; | ||
| } | ||
|
|
||
| export function normalizeModelFactOverride(value: unknown): ModelFactOverride { |
There was a problem hiding this comment.
[P3] Give the user a way to say "this fact is unknown", not just "this fact is that". normalizeModelFactOverride accepts only present-and-valid values, with no null tombstone, so when a provider reports a wrong contextWindow the user's only option is to substitute another number they would have to guess — they cannot express "ignore the provider here and fall back to bundled metadata", which for a correction feature is the more honest of the two things they might want. Reproduced by execution. A null sentinel in the allowlist would cover it.
| reasoning?: true; | ||
| functionCalling?: true; | ||
| imageGeneration?: true; | ||
| webSearch?: true; |
There was a problem hiding this comment.
[P3] webSearch here is a separate concern with no production reader. The only thing that reads entry.capabilities.webSearch is model-catalog.test.ts; model-web-search.ts reads ModelInfo.capabilities.webSearch off the model row, not the catalog entry. So this adds a field to a public shape for a consumer that does not exist, inside a PR about user-overridable facts. The repository asks that product changes stay separate unless they must ship together — either wire the reader or move it to its own change.
Generated-by: Codex
Generated-by: Codex
2 participants
Summary
Adds a versioned, schema-validated
model-facts.jsonauthority keyed byprovider:model. User overrides are projected over provider inventory and generated metadata for catalog, connection-test, and execution paths; enabled override-only models remain selectable without exposing unrelated models. Replacing facts atomically persists validated data and invalidates prior connection verification.Fixes #2330
Verification
npm --workspace @maka/core run buildnpm --workspace @maka/storage run buildnpm --workspace @maka/core run typechecknpm --workspace @maka/storage run typechecknode_modules/dugite/git/bin/gitforbundled-git-workspace-smoke.testbiome checkon all 15 changed filesgit diff --checkAI use
Tool(s) and scope: Codex implemented the change and tests, and performed local static and test review. The final commit contains
Generated-by: Codex.Checklist
Does this PR entail a change in behavior?