Uh oh!
There was an error while loading. Please reload this page.
fix(core): cloneAsOverride returns a mutable type, so an override draft type-checks - #5320
Merged
Merged
Conversation
…ft types as mutable
`cloneAsOverride<T>(view: T): T` handed the input type straight back. Cloning a
`SystemView<S>` — `DeepReadonly<S>` plus the marker symbol — therefore returned
something still typed deep-readonly, while the implementation had always
produced a plain mutable object (`structuredClone`, or a JSON round-trip
fallback) and deliberately dropped the marker. The declaration was wrong about
its own value, and the documented override flow was what broke:
`packages/core/README.md`'s `draft.columns.push({ name: 'name' })` failed with
TS2339 against the built types.
Adds `DeepMutable<T>`, the inverse of the existing `DeepReadonly<T>`, and
returns it. Per the maintainer's 2026-08-19 ruling (option A), the two
alternatives were rejected by name: teaching a cast in the README is the
lenient-consumer pattern the contract rules out, and declaring the block a
documentation fragment hides a real signature defect behind the fragment marker.
The return type relaxes toward what the runtime already does, so no caller loses
a permission — a repo-wide sweep found no call site outside the README, and both
assignment directions are pinned as type-level cases.
Also refreshes the doc-snippet gate's ledger entry for `packages/core/README.md`,
which this change makes stale: TS2339x2 -> TS2339x1, and the survivor is now
triaged rather than un-triaged — it is the `❌ TypeError (strict mode)`
immutability demonstration, where a readonly rejection is the documentation
working as written.
Part of objectui#5257
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RV6yuVCxymHYE16PL9vQkEContributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
os-support-ai
marked this pull request as ready for review
August 19, 2026 14:35
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes#5257
Per the maintainer's 2026-08-19 ruling (verbatim 「全部接受」), option A:
cloneAsOverridereturns a type that tells the truth about the value it produces.The defect
cloneAsOverride< T >(view: T): Thanded the input type straight back. Cloning aSystemView< S >— which isDeepReadonly< S >plus the marker symbol — returned something still typed deep-readonly, even though the implementation has always produced a plain mutable object (structuredClone, or a JSON round-trip fallback) and deliberately drops the marker. The declaration was simply wrong about its own value.The acceptance criterion, measured
The criterion was concrete: the documented override flow in
packages/core/README.mdmust type-check, measured by the snippet compile gate from #5138.That document sits in the gate's
UNGATED_DOCSledger, so a plainpnpm check:doc-snippetsrun does not compile it. The measurement therefore re-runs the gate's own exported harness (analyze/compileSnippets— same compiler options, same dist-resolution control, same planted sentinel) with anungatedmap covering every document except core's README. Nothing about the measurement is re-implemented.Controls green on every run: resolution landed on
packages/types/dist/index.d.ts(a built artifact, not source), 0 source-file leaks, the planted sentinel produced its TS2305 (so the program is not resolving everything toany), positive control clean.README.md:124draft.columns.push(...)README.md:119userListView.columns.push(...)The surviving TS2339 is required. Line 119 is the
userListView.columns.push({ name: 'name' }) // ❌ TypeError (strict mode)demonstration — the System-View immutability example, where a readonly rejection is the documentation working as written. Relaxing the clone must not relax the source, and three type-level cases pin that it does not.Note on line numbers: the card cites
README.md:121and:116. Those shifted to:124and:119between filing and now; the substance is identical and the drift is only the numbering.The change
Adds
DeepMutable< T >, the inverse of the existingDeepReadonly< T >, and returns it.Naming — a deliberate deviation from the ruling's literal text, flagged for review. The ruling writes the signature as
Mutable< T >while also calling for "aMutable/DeepMutableinverse". This ships one type, namedDeepMutable, for the same reason the existing type is not calledReadonly: TS' built-inReadonly< T >is shallow, so a bareMutablewould understate its reach by exactly the depth gap that madecloneAsOverridewrong in the first place. Shipping a shallow-sounding name on a deep type would repeat this card's own bug one level up. Two exported names for one type was also rejected — it gives an author two spellings for one thing.Two limits, stated rather than left to be discovered.
DeepMutableis symmetric withDeepReadonlyarm for arm and inherits its tuple behaviour: a tuple widens to an array, exactly asDeepReadonlywidens it in the other direction (an inverse that preserved tuples would not be an inverse). And it does not strip theSYSTEM_VIEW_MARKERkey — the clone never carries the symbol at runtime, but the key is declared optional, so carrying it states "may be absent", which is true. Excluding it needsExclude< keyof T, typeof SYSTEM_VIEW_MARKER >, a non-homomorphic mapped type that drops the?modifier from every other property and turns optional keys required: a strictly worse type traded for removing a key that already reads as optional. One type-level case pins that optionality survives, so that regression cannot land quietly.Consumer sweep — the answer is zero, and it is counter-probed
cloneAsOverridehas no call site anywhere in the repo outsidepackages/core/README.md.DeepReadonlyandSystemViewhave no consumer outsidepackages/coreeither. The siblingobjectstackcheckout has zero hits.A zero is only worth as much as the probe that produced it, so the same grep was run over neighbouring core exports:
normalizeListViewandexpandFieldsreturn hits acrossplugin-detail,plugin-view,plugin-list,app-shellandtypes. The sweep spans packages; the zero is real.So the ruling's load-bearing premise — the return type relaxes toward runtime, existing callers keep compiling — holds trivially here, and is additionally pinned rather than assumed: two type-level cases assert a draft is still assignable to the
SystemView< S >and theDeepReadonly< S >it came from. Nobody loses a permission; a caller gains one. Hencepatch.Reverse verification — predicted before running, then observed
Reverted
freeze-schema.tsalone, keeping the new tests and the ledger edit.The rebuild question, per leg — and it mattered. The two legs resolve differently on purpose. The vitest pin file imports
../freeze-schemaby relative path, so it reads source and needs no build. The snippet gate resolves@object-ui/corethrough packageexportstodist/*.d.ts, so the edit only reaches it through a build. Measured: with the source reverted butdiststale, the gate reported 6 diagnostics — the fixed state. A false green. After rebuilding core it returned to 7, with TS2339 at both:119and:124. Every leg here, mutation and restoration alike, was rebuilt and the marker's presence/absence indist/utils/freeze-schema.d.tsverified before any result was read.type-checkDeepMutableimport:119and:124Both misses are recorded rather than smoothed over:
flippedcollapsed to[], and{ i: 0, now: true, before: true }). The overcount was cases 8 and 9: an unresolved type name reports at the import line, not at the annotation site, where it degrades toanyand yields no diagnostic. Only case 11 failed at its own line, via an implicit-anyparameter understrict.type-check.DeepMutableis named only inside virtual-module source strings in that file, never in a real import, sotscnever sees it. What went red instead was the runtime test's owndraft.columns.push(...)line.Green on both legs, stated plainly: the two runtime tests. They pass reverted and unreverted, because the runtime was never the defect — the type was. They are in the suite so a future edit cannot "fix" the mismatch by making the runtime readonly instead. Note the split they expose: vitest strips types, so those tests execute green on the reverted leg, while
type-checkreads their source and goes red. The two legs measure different things.Declared file surface
packages/core/src/utils/freeze-schema.ts—DeepMutable+ the signaturepackages/core/src/utils/__tests__/freeze-schema.types.test.ts— new; 12 type-level cases, a revert-proof discrimination control, 2 runtime companions.changeset/clone-as-override-returns-mutable-5257.md— new,patchscripts/check-doc-snippet-types.mjs— beyond the claimed surface, named here with evidence. One ledger string, no behaviour change. The entry forpackages/core/README.mdreadTS2339x2 — candidate real defects, un-triaged; this PR is what makes that count false. It now readsTS2339x1, and the survivor is triaged rather than un-triaged: it is the deliberate immutability demonstration, not a defect. Leaving it would have left the debt list asserting a measurement this PR falsified. The gate's own self-test (20 cases) and a full gate run were both re-run because of this edit.Verification
All at head
c548048f3, run from the repository root (core owns a standalonevitest.config.ts, one of the 11 packages objectui#5313 measured the invocation guard as not covering).vitest run packages/core scripts/__tests__/check-doc-snippet-types.test.ts— 92 files, 1937 tests, all passpnpm --filter @object-ui/core type-check— clean (tsc --noEmit && tsc -p tsconfig.test.json, both echoed)pnpm --filter @object-ui/core lint— 0 errorsnode scripts/check-doc-snippet-types.mjs— 68/68 blocks judged, 0 failed, all controls greennode scripts/check-control-bytes.mjs— 4716 files scanned, OKcheck-changeset-presence/no-major/fixed,check-type-check-coverage,check-lint-coverage— all greenGenerated by Claude Code