docs(architecture): audit Model access slice under #3522 - #4711
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed current head 953a003492cd92fa09ad8f1021cbef07eff125a2 (OPEN). One P2 below — the banner's "implemented, archivable" conclusion does not hold. Note: this is the same first-screen dependency chain flagged on PR #4794; the two should be handled together so the wording stays consistent.
P2 — two of the banner's three factual claims fail, so "implemented, archivable" fails
The move itself is clean (pure git rename, 96% similarity, history preserved, +2 lines). Claim (b) holds: onboarding-hero.tsx:27 is import type only, runtime uses the local FIRST_RUN_PROVIDER_TYPES constant. But (a) and (c) fail — a fully static chain still reaches provider-registry on the first screen: main.tsx:22 → app.tsx:23 → app-shell.tsx:170 → app-shell-overlays.tsx:30 → app-shell-command-actions.ts:36 → command-palette-commands.ts:50 → isRetiredProvider from @maka/core/provider-registry. All six hops are static imports (the dynamic import at app-shell-overlays.tsx:42 leads to settings/settings-modal, a different path, not this chain) — and past provider-registry the dependency is a value dependency, not shakeable side effects.
What I could not judge
Whether the team wants the doc archived with corrected claims vs kept live is a maintainer call.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
Two corrections to my review above. The P2 itself stands unchanged; both corrections concern how it was stated.
Scope of the evidence. The review omitted a limitation that materially bounds the finding: no build was run and no bundle output was measured. The dependency chain reported above is static import-graph analysis, not byte-level evidence that the first-screen chunk contains the models.dev metadata. Producing that evidence would require a build followed by inspection of the emitted chunk, which was not done. One supporting detail that was checked and does belong in the record: packages/core/package.json declares "sideEffects": false, so a bundler is permitted to drop unused module-scope code — but that does not rescue this chain, because isRetiredProvider has a genuine value dependency on PROVIDER_REGISTRY (provider-registry.ts:1685 → providerDefaultsOf at :1644-1648), which in turn is constructed from the generated models.dev tables imported at :20-24. Tree shaking removes what is unused; it cannot remove a constant that is used.
Merge posture. The Simplified Chinese block ended with wording that amounted to "fix before merging". That was not the reviewer's conclusion and it contradicts the English body of the same comment. This review does not gate the merge. The finding is that the banner's claims (a) and (c) do not hold, and the choice of remedy belongs to the maintainer. Two options were offered: withdraw the archive, keep the document under docs/, and record this surviving chain in its current-state section; or break the chain in code first — for example by extracting the small retirement predicate that isRetiredProvider needs out of provider-registry, so the first screen does not pull in the whole model-metadata table for one boolean — and archive afterwards.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
简体中文
两处更正,P2 结论本身不变,更正的是表述。
其一,上一条漏了一项限定:没有跑构建、没有测量产物体积,那条依赖链是静态引用图分析,不是「首屏 chunk 确实含 models.dev 元数据」的字节级证据。
其二,上一条中文块结尾的「修好再合」不是本次评审的结论,且与同一条评论的英文正文矛盾。本评审不设合并门禁。 结论是横幅的 (a)(c) 两条事实不成立,采取哪种补救由维护者决定;已给出的两个方案是撤回归档并在文档现状小节记录这条残留链路,或先在代码里断链后再归档。
Audit the two Model access documents against current main: - openai-responses-incremental-transport.md: verified accurate as written. Every cited symbol (ModelAdapter, endLane, previous_response_id, prompt_cache_key, the sdk.response.messages contrast) still exists with the semantics the document describes. No change. - model-metadata-firstscreen-optimization.md: partially implemented, not archivable. Verified done: OnboardingHero no longer imports RECOMMENDED_PROVIDER_TYPES at runtime. Not done: a static import chain still reaches provider-registry from the first screen (main.tsx -> app.tsx -> app-shell.tsx -> app-shell-overlays.tsx -> app-shell-command-actions.ts -> command-palette-commands.ts -> isRetiredProvider -> providerDefaultsOf -> PROVIDER_REGISTRY, built from the generated models.dev tables). Add an audit note recording this surviving chain so the document is not mistaken for an implemented proposal; keep it live under docs/ rather than archiving it. Address review on apache#4711: the original 'implemented, archivable' banner was wrong — two of its three factual claims fail. A static import chain still reaches provider-registry from the first screen via command-palette-commands' isRetiredProvider value import, which tree-shaking cannot drop because PROVIDER_REGISTRY is a used constant. Refs apache#3522 Generated-by: Claude (Claude Code)
953a003 to
e260c89
Compare
|
Thanks for the P2 — verified, and you're right. I traced the chain you flagged and it holds: Updated the PR: dropped the archive move, kept the document live under Left the code-break-the-chain option (extracting |
Summary
Audits the Model access subsystem slice under #3522 — the two documents listed there were read against current
mainand either corrected or confirmed accurate.docs/architecture/openai-responses-incremental-transport.md— verified accurate, no change. Every cited symbol (ModelAdapter,endLane,previous_response_id,prompt_cache_key, thesdk.response.messagescontrast) still exists with the semantics the document describes.docs/model-metadata-firstscreen-optimization.md— the optimization it proposes has been implemented. The renderer startup path no longer statically importsmodel-metadata;OnboardingHerono longer importsRECOMMENDED_PROVIDER_TYPESat runtime;PROVIDER_REGISTRYis now reached only from lazy Settings paths — matching the document's desired outcome. The document was being read as an open proposal when its acceptance criteria are met, so it is moved todocs/archive/with anArchived:banner so it is read as the historical design record.The move is a git rename (history preserved); the only content change is the two-line banner.
docs/archive/**is out of scope for #3522's audit, but moving a now-implemented proposal into the archive is the audit conclusion for this document, not an edit to an existing archive entry.Refs #3522
Verification
model-metadata-firstscreen-optimizationby path, so the move breaks no links (checkedscripts/,.github/, all*.md/*.ts/*.mjs).node --test scripts/source-legal-inventory.test.mjs(the test that readsdocs/): 5/5 pass.grepagainstpackages/runtime/src.R), so blame/history is preserved.Not run: the repo's full lint/typecheck (a full
npm cidoes not complete from this network). The change is docs-only and confined todocs/; no code or test is touched.AI use
Tool(s) and scope: Claude Code — audited both documents against current code, implemented the archive move and banner, and authored the commit. The commit carries a
Generated-by: Claude (Claude Code)trailer.Checklist
(Both unchecked: this is a docs-only change with no test surface; the relevant
source-legal-inventorytest passes as noted under Verification.)Does this PR entail a change in behavior?