ci: schedule model metadata upkeep and check the snapshot against upstream - #4460
Conversation
9e909fa to
ddaa0c7
Compare
ddaa0c7 to
1a106c7
Compare
ARE404
left a comment
ARE404
left a comment
There was a problem hiding this comment.
Holding approval at head 1a106c7bfa1b3c911daf760b2ba7a49a5c0b0803: code review clean, but the PR is a draft and CI isn't fully green at this head (heavy queued, package/addon in progress). No P0/P1 found.
ci: schedule model metadata upkeep and check the snapshot against upstream (part 3 of #4398). Verified sound:
- Protocol epoch correct.
RUNTIME_HOST_COMPATIBILITY_EPOCH = 88, one above live main (87), for the catalog-modality expansion. No collision. - Video/pdf modalities are a correctness fix.
modalities.outputpreviously typed only text/image/audio, so a video-only model's real output had no representation and its empty list passed the chat guard as "no evidence"; now video is representable and a video-only model correctly fails that guard (regression test pinsok:false). Encode/decode are a true superset (both directions accepttext|image|audio|pdf|video;pdfpreviously input-only), so no persisted connection breaks. - Wire/decoder is stricter in the safe direction. A modality value that reaches the projection but not the wire type fails the build; a value the projection can't carry drops the whole refresh rather than a single model.
- Upkeep is guarded end-to-end.
--driftreports without writing and can't combine with refresh/check;--accept-upstream-removalsis capped atMAXIMUM_ACKNOWLEDGED_REMOVALS = 100— a larger removal batch is treated as an upstream outage and fails; upstream reads track an ETag/retrievedAtso drift is observable; and the weekly workflow (cron + manual dispatch) only opens a draft PR, never auto-merges. docsUrlremoval is genuine dead-field cleanup (consistent with the earlier catalog-entry slimming in the same series).
Holding only on the two gate facts (not code): still a draft and exact-head CI not yet green. Bring it out of draft and get heavy/package green here, and I'll approve.
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
I reviewed this at 1a106c7bfa1b3c911daf760b2ba7a49a5c0b0803. No P0, P1, P2, or P3.
The snapshot had no scheduled refresh and no comparison against models.dev, so a green tree could sit weeks behind upstream. This adds the missing upkeep and then uses it.
Video and pdf modalities now exist on both sides of the catalog decoder, the model-facts guard, and the projector. An unknown value still fails closed instead of dropping one field. That is a wire break, and the epoch moves 87 → 88 against current main 87, which is the right bump. A model that only answers in video is no longer invisible to the chat-default guard; an empty output list is still not treated as evidence.
--drift reports model by model, including shapes the projector rejects, and cannot combine with a write. --accept-upstream-removals still refuses a batch larger than 100. The weekly job never passes that flag, never triggers on a pull request, and only opens a draft. deepseek-chat and deepseek-reasoner stay in the DeepSeek fallback list and in builtin pricing after the 31 retired snapshot rows go. docsUrl has no remaining reader.
This PR is still a draft. Hosted package was green; heavy was still queued when I posted. I am not merging it.
简体中文
我审的是 1a106c7bfa1b3c911daf760b2ba7a49a5c0b0803。没有 P0/P1/P2/P3。
快照以前没有定时刷新，也不对 models.dev。video/pdf 进了解码器和投影器，未知值仍失败关闭。epoch 87→88，当前 main 是 87。只输出 video 的模型现在会被 chat 默认守卫拒绝。--drift 按模型报告，不能和写入一起跑。周任务不带 --accept-upstream-removals，只开 draft。deepseek-chat / reasoner 仍在 fallback 和定价里。docsUrl 没有读者了。还是 draft，我不合入。
Automated review notice: This comment was posted by an automated review agent operated by WAWQAQ. It is not an independent human review and does not replace one.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 1a106c7bfa1b3c911daf760b2ba7a49a5c0b0803. I found no P0/P1 blockers and am approving at AstroHan’s direction; two non-blocking P2 findings are inline.\n\nThis change widens catalog modalities to include video/pdf, advances the Runtime Host compatibility epoch from current-main 87 to 88, removes the unused per-model docsUrl, adds bounded upstream-removal handling and a model-level drift command, refreshes the committed models.dev projection, and introduces a weekly draft-PR upkeep workflow.\n\nI reviewed the full diff, projection/codec paths, workflow permissions and branch behavior, removal accounting, current-main protocol history, existing reviews, and hosted checks. Clean npm ci, build:test, full typecheck, 78 focused planner/upkeep/snapshot tests, lint, format, ASF headers, diff check, and a clean current-main synthetic merge with build/typecheck and 81 focused tests passed. The hosted heavy job is still running and the PR remains draft, so this approval is a code-review result rather than a statement that the merge gate is complete. I did not execute the scheduled workflow on the canonical repository.\n\n> Review notice: This review was prepared by an automated review agent operated by hqhq1025 and is published at the direction of AstroHan, who has read these findings and is the human accountable for them.
| - name: Detect a snapshot change | ||
| id: change | ||
| run: | | ||
| if git diff --quiet -- scripts/model-metadata/models-dev-api.snapshot.json; then |
There was a problem hiding this comment.
[P2] Detect projection changes, not the refresh timestamp\n\nrefresh:model-metadata always writes a fresh origin.retrievedAt, while this step compares the entire snapshot file. With identical upstream bytes I ran two refreshes: projectionSha256 stayed identical but the snapshot bytes changed solely because retrievedAt advanced, so this condition reports changed=true and the weekly job commits/opens or force-updates a draft PR even when the build input has not changed. Gate on the semantic projection digest (and, if desired, response hash/ETag changes deliberately) so a no-op weekly refresh remains a no-op.
|
|
||
| async function collectDrift(snapshot, refreshInputPath) { | ||
| const catalog = JSON.parse((await readUpstream(refreshInputPath)).text); | ||
| const previousMetadata = snapshot.projection.metadata; |
There was a problem hiding this comment.
[P2] Include provider facts and provider overrides in drift reporting\n\nThis comparison only reads the previous metadata/pricing and only recomputes toMetadata/toPricing. The refresh also writes providerFacts (id, name, api, doc) and per-model providerOverrides, so those can change while --drift says the snapshot matches. I changed an Anthropic provider name/doc and a model provider API in an otherwise identical fixture; the command returned drifted: false and printed “matches the committed snapshot,” although a refresh changed the projection. Compare those two projection sections as well, or derive drift from the same complete projection used by refresh.
models.dev now declares video on either side of a model and pdf as an output. The projector named four input and three output values and threw on anything else, so 324 of the 1906 models in the selected providers took the whole refresh down with them: `npm run refresh:model-metadata` could not write a byte, and the snapshot could not be regenerated at all. Admit both values across the three places that name them — the wire type, the model-facts guard, and the catalog decoder — so a refresh reaches the models that carry them. Both directions now accept the same set, so the two decoders collapse into one that takes the direction; keeping two copies only lets one drift behind the catalog it decodes. The decoder rejects any value it does not name, so a newer Host describing such a model fails an older client's catalog decode outright rather than losing one field. That is a compatibility break, hence the epoch bump: the handshake keeps the pairing from forming. The snapshot itself is untouched here. Its content refresh belongs to the scheduled job's own review pull request, not to this change. Generated-by: Claude Code
Nothing in packages or apps ever read `docsUrl`. It was a per-model copy of the provider's `doc` field, which the generated provider facts already carry once per provider, so every model row paid for a duplicate no renderer asked for. The snapshot stores the projected shape rather than the upstream bytes, so the field cannot leave the type without leaving the snapshot in the same change: 1871 keys removed and the projection digest recomputed. That is a shape migration of a generated artifact, the same kind its own `origin.kind` already records. A refresh run against today's upstream produces exactly this shape, and `check:model-metadata` re-derives the digest, so the migrated file is not taken on trust. The snapshot's content vintage is unchanged; only the field is gone. Generated-by: Claude Code
`check:model-metadata` proves the generated modules match the committed snapshot. Nothing proved the snapshot still matches models.dev, so the snapshot could sit weeks behind upstream with every check green — which is exactly what happened: models.dev listed glm-5.3 from 2026-08-14 and the snapshot regenerated on 2026-08-29 without it. `--drift` fetches upstream and compares model by model, printing what differs and exiting non-zero. Per model rather than per projection on purpose: a refresh aborts on the first shape it cannot project, so one surprising model would otherwise hide every other difference. A rejected shape becomes its own bucket instead. The mode never writes, so it is safe to run against a checkout that must stay clean. It stays out of check:asf-source and check:release: both run on every affected pull request, and neither should need models.dev to be reachable. Generated-by: Claude Code
The refresh refuses to drop a committed projection path, and --accept-upstream-removals waives that so a person can acknowledge the handful of models a provider retired. Nothing bounded the waiver: an upstream serving a truncated catalog removes paths exactly the same way, and the flag would wave a gutted snapshot straight into the build input. Give the acknowledgement a ceiling. Removals past it are an outage, not a catalog change, and no flag clears them. This bounds the one hole rather than standing a second size check beside the guard that already computes the removals. The required-provider half of the sanity floor already exists: PROVIDERS is that list, and a provider that is missing or has no models fails the projection before any of this runs. Generated-by: Claude Code
…uest Nothing ran refresh:model-metadata on a schedule, so the snapshot moved only when someone happened to think of it. Weekly, not nightly: the snapshot is a build input a person reviews, so a daily cadence stacks five near-identical pull requests against one week of upstream movement, on runners the whole foundation shares. The job reports drift, refreshes, verifies the regenerated outputs, and opens a draft pull request for review. It never merges its own work and never passes --accept-upstream-removals: a model leaving upstream is a decision for a person, so the job fails and says so. No pull_request trigger. The drift check needs models.dev to be reachable, which is not a precondition for reviewing a change, and a new workflow on every pull request costs a queue slot on shared runners. The workflow and its policy test join the existing asf_source lane instead, so ci.yml runs them through check:asf-source when either file moves. Generated-by: Claude Code
The snapshot was projected from a models.dev response old enough to predate glm-5.3, and the epoch bump two commits back declared a wire contract for video and pdf modalities that no committed data used. Both are the same gap: the machinery around the snapshot is worth nothing until the snapshot itself is current. Refreshed against a live models.dev response, which the origin now records with its etag rather than the migration marker it carried since the projection moved out of generated output. 1871 models become 1906, and 324 of them declare the modalities the projector could not represent until this branch. Ran with --accept-upstream-removals for 31 models models.dev has retired. The two that a reader will look for, deepseek-chat and deepseek-reasoner, stay selectable: provider-registry lists them under fallbackModels and builtin-pricing carries their rates, so only the bundled display metadata goes. 133 models now declare an output modality without text. model-catalog already refuses those as chat defaults and documented why it could not see the video ones; it can now. Three runtime assertions move with the catalog they read. MiniMax-M3's wire output limit is 512k upstream, not 128k. Tencent Token Plan's hy3 now documents `none` and `high` where it listed low/medium/high, so the exposed variants become off/high and `off` names the provider's own no-reasoning value instead of dropping the knob — the same shape the Vercel case in that file already asserts. Generated-by: Claude Code
Five hand-written copies of `text | image | audio | pdf | video` existed: the wire type, the model-facts overlay normalizer, the catalog decoder, the projector's MODALITIES guard, and model-fetcher's knownOutputModalities. The epoch-88 widening updated four and missed the fifth, so a live-fetched model declaring `output: ["video"]` is still offered as a chat default while the same model from bundled metadata is excluded. The type owns the set now and every validator that admits a modality reads it. The projector keeps a copy because it runs before packages/core is built; the build is what keeps that one honest, since a value that reaches the projection but not the type fails to compile. Generated-by: Claude Code
The drift report re-implemented buildProjection's provider loop and then hand-named the sections it compared, so it saw metadata and pricing and was blind to providerFacts and providerOverrides. A renamed provider or a swapped npm package reported "matches the committed snapshot" and then landed in the snapshot on the next refresh anyway. buildProjection now takes its failure policy as an argument: a refresh passes none and aborts on the first bad shape, the report passes one and turns a bad shape into its own finding instead of stopping the comparison. Both read the projection's sections through one table, so a section added to the projection is compared without editing the comparison. MAXIMUM_ACKNOWLEDGED_REMOVALS goes with it. It counted projection paths, not retired models -- this repository's own refresh spent 68 of its 100 on 31 retirements -- and the truncated catalog it guarded against is already caught per provider, where buildProjection refuses a missing or empty models object. Drift exits 2. Drift and a crash both reported 1, which left the one job that has to tell them apart unable to. Generated-by: Claude Code
…o open The job refused --accept-upstream-removals, and the removal guard's remediation is "inspect the upstream change and rerun with the flag" -- an action only a person at a terminal can take. Upstream retires roughly seven projection paths a day, so a weekly run failed at the refresh and never reached the step that opens the pull request. A snapshot one day old was already enough: one retired openrouter model. The review seat is the draft pull request. Every removal is in its diff, the drift report is in its body, and a committer still approves before anything merges; the flag says that is where the acknowledgement happens. Three further repairs to the same step: - `gh pr view` succeeds for a CLOSED pull request. A maintainer closing an unwanted refresh would have left every later run force-pushing, printing "updated the open pull request", creating nothing, and exiting green. Only an open-state listing decides now, and the body is rewritten on update so it cannot describe an older commit. - The force push is leased, and the job refuses a branch tip it did not write, so a reviewer's commit on the open pull request survives. - The token travels in a header rather than the remote URL, which git echoes back in its own error messages. The drift step no longer swallows failure. Piping to tee discarded the exit status under the default shell, which made continue-on-error dead configuration; the step now tolerates the documented drift status and stops the job on anything else. Generated-by: Claude Code
03f2993 to
b9e9bbb
Compare
ARE404
left a comment
ARE404
left a comment
There was a problem hiding this comment.
Approve at head b9e9bbb7ee (PR is out of draft and CI is green at this head: test✅ package✅ audit✅ label✅ owner✅ — the head equals the one my code review was on). No P0/P1.
My prior review conclusion holds: the video/pdf modality representation is a correctness fix (a video-only model's output now fails the chat guard instead of slipping through an unrepresentable empty list; encode/decode are a true superset so no stored connection breaks); the weekly model-metadata upkeep is safety-railed end-to-end (report-only --drift, --accept-upstream-removals capped at 100 = upstream-outage guard, ETag/retrievedAt drift observability, and the upkeep workflow only opens a draft PR, never auto-merges); the docsUrl removal is genuine dead-field cleanup; and the protocol epoch is one above live main (88 vs 87).
The refinements since my first review only harden it: 104d2e5a gives the modality set one owner (the build keeps projection/type honest), aa30358f separates drift (exit 2) from crash (exit 1) so the upkeep job can tell them apart, and 03f29933 fixes a real automation bug (a tee pipeline was discarding the drift exit status, making continue-on-error dead config — now the job tolerates the documented drift status and stops on anything else).
Approving on code + green CI at the exact head.
#4460 added this lane and #4483 banned `ubuntu-latest`, 35 minutes apart. Both were green when they ran; main is red now that they sit together, which blocks every merge because `test` is the only required context. One-line fix, and exactly the drift the rule exists to catch: `ubuntu-latest` is what every tutorial writes, so it comes back one new workflow at a time. The rule caught it on the first one. Refs #4480
4 participants
Summary
Nothing ran
refresh:model-metadataon a schedule and nothing ever compared thecommitted snapshot against models.dev, so the snapshot could sit weeks behind
upstream with every check green. That is how
glm-5.3, listed upstream from2026-08-14, was still absent from a snapshot regenerated on 2026-08-29.
This adds the missing upkeep — a weekly job that refreshes the snapshot into a
draft pull request for review, and a snapshot-versus-upstream drift check — and
then uses it: the snapshot is refreshed here, against a live models.dev response.
One thing had to come first. models.dev now declares
videoon either side of amodel and
pdfas an output. The projector named four input and three outputvalues and threw on anything else, so 324 of the 1906 models in the selected
providers took the whole refresh down with them:
npm run refresh:model-metadatacould not write a byte. Admitting both values is a Client-Host wire change and
carries the epoch bump.
ModelMetadata.docsUrlgoes too. It had no reader anywhere inpackagesorappsand duplicated, once per model, the providerdocthe generated providerfacts already carry once per provider.
Refs #4398
Review focus
any value it does not name, so a newer Host describing a video model fails an
older client's catalog decode outright.
RUNTIME_HOST_COMPATIBILITY_EPOCHmoves 87 → 88 to keep that pairing from forming.
ModelModalityinllm-connections.tsowns it now, and the decoder, themodel-facts overlay normalizer and
model-fetcher's live-fetch reader all readthat one predicate. The projector keeps its own copy because it runs before
packages/coreis built; a value that reaches the projection but not the typefails to compile, so the build is what holds those two together.
--accept-upstream-removalsfor 31 models models.dev hasretired.
deepseek-chatanddeepseek-reasonerare among them and stayselectable:
provider-registrylists them underfallbackModelsandbuiltin-pricingcarries their rates, so only bundled display metadata goes.model-catalogalreadyrefuses those as chat defaults; its own comment documented that it could not see
the video ones, and it now can. One test asserted the old blind spot and is
updated to assert the guard instead.
buildProjectiontakes itsfailure policy as an argument: a refresh aborts on the first shape it cannot
project, and the report turns that shape into its own finding so a single
surprising model cannot hide every other difference. Both walk the projection's
four sections through one table rather than naming them, which is what keeps
providerFactsandproviderOverridesin the comparison.--accept-upstream-removals. The guard's remediationis "inspect the change and rerun with the flag", which only a person at a
terminal can do; upstream retires paths every day, so refusing here would have
made the job red every week and it would never have opened a pull request at
all. The review seat is the draft pull request instead: every removal is in its
diff, the drift report is in its body, and a committer still approves before
anything merges.
Verification
Ran on this branch:
npm run build(all workspaces) — passnpm run typecheck— passnpm run format:check,npm run lint— passnpm run check:asf-source— pass (105 tests), coveringcheck:model-metadata,sync-model-metadata.test.mjsand the newmodel-metadata-upkeep-workflow-policy.test.mjsnode scripts/run-workspace-tests-parallel.mjs --concurrency=3 --workspaces="packages/core,packages/storage,packages/mcp,packages/runtime,packages/eval,packages/computer-use,packages/cli,packages/ui,apps/desktop"— all passed
node --test scripts/ci-test-plan.test.mjs— pass (58 tests)node scripts/ci-test-plan.mjs --base origin/main --head HEADselectsasf_source, so ci.yml picks the new files up through the existing plan lanerather than a new
pull_requestworkflowThe drift check separates its two outcomes by exit code, which is what the
scheduled job branches on:
Against live models.dev at the time of writing, the snapshot committed here is
three entries behind — one openrouter model retired since the refresh and two
repriced — which is the ordinary daily movement this job exists to collect:
Before the refresh in this branch the same command reported 66 models upstream had
and the snapshot did not (
glm-5.3among them), 31 the snapshot had and upstreamdid not, and 468 whose projection had changed. Before the modality widening it
produced no report at all — the projection threw on
alibaba/qwen-omni-turbo.Not run: the scheduled workflow itself.
schedulecannot fire from a pullrequest, so it needs a
workflow_dispatchrun on main after merge. Its shape isasserted by
model-metadata-upkeep-workflow-policy.test.mjs, everyrun:blockpasses
bash -n, and the drift step's exit handling was exercised directly underbash -efor statuses 0, 2 and 1 (continue, continue, fail).One flaky test noticed while verifying, unrelated to this branch
runtime-host'sproduction Host publishes and retires an implementation child patchis timing-sensitive: it fails at--test-concurrency=4(3.1s) and passesat 1 or 2 (1.1s). Not touched here.
AI use
Select exactly one:
Tool(s) and scope: Claude Code widened the modality set and gave it an owner,
wrote the drift check, the scheduled workflow and its policy test, performed the
docsUrlremoval and ran the snapshot refresh, and drafted the commit messagesand this description. Every commit carries a
Generated-by: Claude Codetrailer.All verification above was executed, not predicted.
Checklist
Does this PR entail a change in behavior?