fix(data-objectstack): discriminate a refused mapping read from a served zero, on a channel - #8152
Conversation
…ved zero `listImportMappings` degrades every failure to an empty list and the import wizard hides its saved-mapping selector on an empty list, so "the server served zero mappings" and "the server refused, or broke" rendered identically on every deployment, with a `console.warn` as the only discriminator. The empty-list return is unchanged on every arm, including the loud ones. What is added is a channel alongside it: - `classifyImportMappingsFailure(err)` reads the error's ADR-0112 `code` first and the status only where no code was declared -- never "is the result an empty array", which is what both conditions produce. - `ObjectStackAdapter.onMetadataReadWarning(cb)` is a subscribe/unsubscribe channel, sibling in shape to `onWriteWarning` and `onSaveAdvisory`, carrying which read failed, on which object, whether the server refused this caller or the answer was unreadable, and the server's own code, status and message. - A deployment that does not serve the `mapping` kind stays quiet: same empty list, same hidden selector, no event. - `AdapterProvider` renders the channel as a warning toast, so a user without devtools can tell "there are none" from "we could not find out". Three new locale keys in all ten packs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
|
| new export | face |
|---|---|
ImportMappingsFailureKind |
@object-ui/data-objectstack (published) |
classifyImportMappingsFailure |
@object-ui/data-objectstack (published) |
MetadataReadWarningEvent |
@object-ui/data-objectstack (published) |
MetadataReadWarningListener |
@object-ui/data-objectstack (published) |
ObjectStackAdapter.onMetadataReadWarning(cb) |
a new public method on the published adapter |
TranslateFn, MetadataReadWarningSink, emitMetadataReadWarning |
@object-ui/app-shell |
⇒ needs:contract-review is now attached and this PR stays DRAFT. ⛔ It does not enqueue on that carrier alone.
⭐ The dev followed my fence precisely and reasoned well inside it, and its determination is correct about what it actually claims: the return-or-throw contract genuinely does not move, on every arm including the loud ones. Its structural guarantee is better than the claim needed — every loud test re-asserts the [] return alongside the emitted event, so a later contract change cannot ride in disguised as a channel change. That is a real safeguard, not a formality. It simply is not the whole of Clause-②, and the sentence that told it so was mine. ⛔ Recording this rather than quietly attaching a label, because the next seat inherits the fence, not the outcome.
Contract review — PASS
① Derived judgments
Direction: WIDENS, additively. No existing member changes; listImportMappings answers Promise<any[]> and never throws, so a consumer subscribing to nothing sees behaviour byte-identical to 17.1.0. ⭐ operation and kind are single-member unions on purpose — one emitter exists, and a second arrives as an additive reviewed widening rather than as something a consumer's exhaustive switch meets at runtime. That is the right shape for a union that will grow.
⭐ The discrimination is read from err, in the required order, and never from emptiness. ADR-0112 code first; HTTP status only in the code-less residual. That was the dispatch's binding requirement and the one this repo keeps failing — 「never from "is the result an empty array", which is what both conditions produce and so can never fail for the condition it is about」 is the dev's own phrasing, and it is exactly right. It also follows the neighbouring classifyAnalyticsFailure ladder (#5663 / #5721) rather than inventing one.
⭐⭐ The subtlest thing in this PR, and the one a naive implementation would have got wrong: INVALID_REQUEST with a 400 is classified quiet, because that is the metadata LIST door's own refusal for a kind the deployment cannot serve (framework#9488 refuseUnknownMetaListType) — the modern spelling of "an older server without the mapping kind". ⇒ an implementation that classified every 4xx as loud would have turned a real, supported deployment shape into a visible fault, which is precisely what the adjudication forbade preserving-by-accident. The quiet set (ROUTE_NOT_FOUND / NOT_IMPLEMENTED / code-less 404 / 501 / that 400) is justified member by member.
unreadable is deliberately not a silent bucket — a 5xx, a dropped connection, or an unrecognised code is not evidence that no mapping is registered. Correct: a catch-all that defaults to quiet would have rebuilt the original defect one level down.
⛔ The console.warn is kept verbatim on both arms, not re-levelled and not made conditional. The dispatch's ⛔ was against merely re-levelling it; keeping it and adding the channel beside it is the right reading — it was never the problem, being the only discriminator was.
The UI half actually satisfies the requirement. The dispatch and triage both insisted a log-level change is insufficient because a user without devtools sees a feature that is simply absent. AdapterProvider subscribes and toasts, so 「there are none」 and 「we could not find out」 are now distinguishable on screen. ⭐ The remedy is chosen by an exhaustive switch with a never check, not a ternary — a ternary would silently render the wrong fix for a third reason, which is the same defect class one layer up. And the server's code / status / message ride along verbatim and untranslated, with the reason stated: they are the evidence, and translating them would need a key per producer wording.
② semver
@object-ui/data-objectstack: minor · @object-ui/app-shell: minor · @object-ui/i18n: minor. ✅ Correct for an additive widening; major is forbidden by repo convention and Changeset Bump Policy is green.
③ Boundary flags
- ⭐ Three ablations, and the third is the one that matters. Cutting the
AdapterProvidersubscription reddens only the middle-link test while the producer suite stays green (2 failed | 21 passed) — reproducing the blind spotAdapterProvider.advisorySink.test.tsxmeasured for its own channel (objectui#7116) before it could be inherited here. A channel with a green producer suite and no wiring test is a channel that can be silently unplugged; this one cannot. The other two legs are non-vacuity proofs for the quiet and loud arms respectively. Restores proven byte-identical (git diff HEADempty +git hash-objectequal to the HEAD blob), under a trap. - ⭐
check:readme-exportsdeclared PREREQUISITE NOT MET, not red — needingdist/forplugin-timeline/plugin-gantt, packages outside this diff. CI has now returned it green. ✅ The declaration was honest. - ⭐ Two gate names that do not exist (
check:lint-coverage,check:type-check-coverage→ERR_PNPM_NO_SCRIPT) were read as NOT MEASURED and re-run under their real names (lint:coverage46/46,type-check:coverage45/46), ⛔ rather than reported as passes. That is the correct handling of a command that did not run. - Lint narrowing carries all three legs — population from eslint's own config, count from
--format json(16 files, 0 errors, and exactly 1 of 127 warnings on an added line), and invariance (tseslint.configs.recommended, ⛔ notrecommendedTypeChecked; noparserOptions.project/projectService⇒ no rule reads the type graph ⇒ untouched files cannot move). ✅ Valid. - The deviation is stated in the PR body, ⛔ not papered over: triage requires batching with objectui#7740,
createIdentityImportDataSourcespreads a class instance, solistImportMappings(a prototype method) is dropped and the saved-mapping selector is hidden onsys_userby accident rather than by the design the file states #7740 isneeds-user-decisionand unanswered, so the batch is unsatisfiable; onsys_userthis PR changes nothing becauselistImportMappingsis not on that wrapper at all, and the symptom is not fully closed.identityImport.tsis untouched (0 hits) andcreateIdentityImportDataSourcespreads a class instance, solistImportMappings(a prototype method) is dropped and the saved-mapping selector is hidden onsys_userby accident rather than by the design the file states #7740's design question is not decided. ✅ Exactly as dispatched. - objectui#8151 filed, ⛔ not folded in:
listViewscarries the identical swallow. The dev applied a four-condition test for the in-place exemption and it failed the fourth (it would widen the published event union and add a verification surface this card's gate set does not cover). ⭐ Declining to widen your own PR on a stated test is the behaviour this lane wants. open_questions: none.
Verdict
PASS. Same stroke: needs:contract-review is cleared, with this comment as its provenance — it was attached above to record that this PR required a contract review, and it is cleared because it has had one.
⛔ Not landing yet. CI unconverged: 32 enumerated against total_count: 32, 7 still in_progress (Lint, Test shards 1–4, Type Check, Live E2E), zero failures. Lint is not yet readable and this seat will ⛔ not flip ready without reading that job. Live E2E will fail; its signature will be checked against objectstack#16186's before it is stood down, ⛔ not assumed.
Generated by Claude Code
⛔
|
…s authority
`metadataReadWarningToast.ts` was modelled on its siblings `writeWarningToast`
and `saveAdvisoryToast`, and copied their local `export type TranslateFn`
declaration along with the pattern. That name already had three declarations,
so this made it four and reddened
`scripts/__tests__/one-authority-per-exported-name-6273.test.ts` (the
objectui#6172 ruling: every exported name has exactly one authority).
Re-exported instead, which the gate does not count as a declaration:
export type { TranslateFn } from './writeWarningToast.js';
`writeWarningToast` is the one pointed at on evidence already in the tree, not
a judgement made here: `AdapterProvider` -- this module's only caller -- already
imports `TranslateFn` from `./writeWarningToast.js` and passes that same value
into all three emitters, and `file-size-guard.ts`'s own declaration names
`app-shell/src/providers/writeWarningToast` as the established pattern it was
copied from.
No `KNOWN_COLLISIONS` entry was added; that baseline is shrink-only. The
pre-existing three-way collision is untouched -- this only stops adding to it.
The change is type-only and runtime-inert: `ts.transpileModule` with
`isolatedModules: true` emits byte-identical JS for the file before and after
(3586 bytes, 0 diagnostics, both legs).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S
|
Patch round pushed: Re-pointed, not baselined. ⛔ No Re-export, not rename — the three existing declarations are character-for-character the same shape ( Which one it points at, from evidence already in the tree rather than a judgement made here (Zone 1 ③'s stop-and-report condition therefore did not fire):
VerificationThe gate, and its whole neighbourhood.
Runtime inertness shown, not assumed. Also green on the new head: ⛔ The scoping lesson, recordedThe first round's test narrowing was Generated by Claude Code |
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
Patch-round delta review — PASS. Head
|
| requirement | verdict |
|---|---|
⛔ no KNOWN_COLLISIONS baseline entry |
✅ git diff origin/main..head -- scripts/__tests__/one-authority-per-exported-name-6273.test.ts is empty — the gate file is untouched |
| ✅ re-point, which the gate does not count as a declaration | ✅ import + export type { … } from |
| ⛔ do not repair the pre-existing three-way collision | ✅ writeWarningToast / saveAdvisoryToast / file-size-guard untouched |
✅ include scripts/ in the re-run |
✅ below |
⭐ The stop-and-report condition correctly did NOT fire, and the reason is the good part
My Zone 1 point 3 said: 「if re-pointing turns out to require first deciding which of the three is the authority, ⛔ stop and report rather than widen.」 It did not require deciding, because the tree had already decided — and the dev showed that rather than asserting it. I re-measured both legs:
AdapterProvider.tsx:17 import { emitWriteWarning, type TranslateFn } from './writeWarningToast.js';
AdapterProvider.tsx:81 … emitWriteWarning(ev, tRef.current as TranslateFn, …)
AdapterProvider.tsx:94 … emitSaveAdvisories(ev, tRef.current as TranslateFn, …)
AdapterProvider.tsx:107 … emitMetadataReadWarning(ev, tRef.current as TranslateFn, …)
file-size-guard.ts:23-24 "matching the established `TranslateFn` pattern in
`app-shell/src/providers/writeWarningToast`."
⇒ this module's only caller already imports TranslateFn from writeWarningToast and feeds that same value into all three emitters, and the third declaration site names writeWarningToast in its own docblock as the pattern it copied. ⭐ There was no judgement to make, so making none was correct — and saying why there was none is what separates that from having skipped the check.
Re-export rather than rename, because all three declarations are character-for-character identical: ⛔ nothing to rename apart.
⭐⭐ Runtime inertness shown, not assumed — and a bad instrument caught before it was quoted
I asked for this explicitly: 「a type-only re-export should be inert at runtime; show that rather than assume it」. Delivered via ts.transpileModule with isolatedModules: true (the repo's own setting) over the file's before and after text: 3586 bytes each, 0 diagnostics each, byte-identical output.
⭐ And the first attempt is reported NOT MEASURED: it used esbuild, which is absent in this container, so pnpm exec wrote its own error text into both output files — which then compared equal for the wrong reason. The dev caught that before quoting it as evidence.
⇒ that is exactly this session's recurring defect class — two readings agreeing because both are the same failure — caught by the dev, on its own instrument, unprompted. It is the same shape as my own "unfiltered props spread" grep on #8154 (a marker that survives the fix), and it is the better outcome of the two because it was caught before the number was used.
⭐ The scoping gap is closed and generalised
The re-run is pnpm exec vitest run scripts/ — 115 files / 3408 tests passed — and inclusion was counted rather than assumed: find scripts -name '*.test.*' returns 115, so the run is the closed set and the failing gate is provably inside it. ⛔ Not "I ran a broader scope and it passed".
The generalisation is right and worth keeping: scripts/ is 115 suites that read the TREE rather than a package — exported-name authority, i18n key parity, changeset presence, dist completeness, turbo inputs, workflow wiring, dependency edges. Any diff adding an exported name, a source file, a locale key, a dependency edge or a workflow line is inside their reach no matter which package it lives in. And it costs 91 seconds against the 15 minutes app-shell alone takes. ⇒ scripts/ belongs in every narrowed local run.
app-shell re-run: 637 files / 6120 passed | 1 skipped — identical to the pre-patch counts, with both new test files among them. ✅ The re-export moved no outcome.
TS2307 Cannot find module @object-ui/* — a fresh worktree with no dependency dist. Reported as PREREQUISITE NOT MET and re-run behind a closure build, ⛔ not reported as a red.
⭐ It corrected its own Clause-② determination
Reading back its own comment, the dev found my correction (5562132650) and superseded its own claim rather than leaving a stale one standing: Clause-② is YES (four new exports on @object-ui/data-objectstack plus the new public onMetadataReadWarning, three more on app-shell), needs:contract-review was attached and cleared in-seat. It also states the carry-forward rule correctly — ⛔ not 「a channel is not Clause-②」 but 「does the diff add or move anything on a published face」 — and flags that its own patch-round comment still carries the pre-correction wording, naming the report as the correction of record. That is the right handling of a claim that went stale under it.
Verdict
PASS on the delta. Everything in the first review (comment 5562131348) stands unchanged.
⛔ Not landing yet. CI restarted on the new head at 21:50Z: 32 enumerated against total_count: 32, 10 in_progress, zero failures. Test (shard 4/4) is among the ten and is the one that matters — it is the shard that caught this defect. ⛔ No ready-flip until it and Lint are read green.
One item routed rather than filed by the dev
TranslateFn is declared three times over, all character-for-character identical, against objectui#6172's one-authority ruling. The dev left it un-filed and handed the routing decision to this seat, reasoning that the KNOWN_COLLISIONS baseline already tracks it. ⇒ I am filing it as a shrink card: a baseline entry is a ledger row, not a work item, the gate itself is SHRINK-ONLY (so shrinking is the declared direction), and this is the cheapest possible shrink — the de facto authority is already named by the other two files' own code and comments.
Generated by Claude Code
1 participant
Fixes #7741
ObjectStackAdapter.listImportMappingsdegrades every failure to an empty list, and the import wizard hides its saved-mapping selector on an empty list. So "the server served zero mappings" and "the server refused, or broke" produced the identical UI on every deployment — the feature simply absent — with aconsole.warnas the only discriminator, in the browser console, with nothing pointing at it. That silence did not merely hide a fault: it produced a confident wrong diagnosis in a careful reporter (objectstack#14026 was filed, routed and worked by two seats against a wizard that had been correct since@object-ui/data-objectstack@17.1.0).This applies framework #13906 decision 1 option A — a thing that could not be READ is not a thing that is ABSENT — at this seam. An already-adopted discrimination, not a new principle.
Clause-② determination: the published contract does NOT move
A channel was added; the return-or-throw contract was not touched.
listImportMappingsstill answersPromise<any[]>and still never throws — on every arm, including the loud ones. A consumer that subscribes to nothing sees byte-identical behaviour to17.1.0. Every loud test in the pin re-asserts the[]return alongside the event, so a future change to the contract cannot slip through as a change to this card's channel.What settled it: the dispatch's fence names the signature as the line, and triage said the same — 「建议优先考虑加通道…而不是改返回契约」. Nothing in the requirement needed the signature. The distinction the card asks for (「the server does not serve this kind」 vs 「the server refused or errored」) is information about the failure, and information can be published on a side channel without changing what the caller is handed. Throwing would additionally have broken the one behaviour the card mandates preserving — the older-server case must stay quiet, and a quiet arm and a throwing arm cannot be the same method's contract without every existing consumer growing a try/catch.
Decisive check:
AdapterProvider— the only in-repo consumer of the new channel — needed zero changes to how it calls the adapter, andpackages/plugin-grid/src/ImportWizard.tsx(the feature-detecting caller) is not in this diff at all.What landed
packages/data-objectstack/src/index.tsclassifyImportMappingsFailure(err)(exported) andImportMappingsFailureKind. It reads the error — the ADR-0112codefirst, the HTTP status only in the code-less residual — and never "is the result an empty array", which is what both conditions produce and so can never fail for the condition it is about. Same ladder and same order as the neighbouringclassifyAnalyticsFailure(objectui#5663 / objectui#5721).not-served(quiet):ROUTE_NOT_FOUND/NOT_IMPLEMENTED; orINVALID_REQUESTwith a 400 — the metadata LIST door's own refusal for a kind this deployment cannot serve (framework#9488refuseUnknownMetaListType), which is the modern spelling of "an older server without themappingkind"; or a code-less 404/501.refused(loud):UNAUTHENTICATED/PERMISSION_DENIED, or 401/403/405.unreadable(loud): everything else — a 5xx, a dropped connection, a code this consumer cannot name. Deliberately not a silent bucket: none of those is evidence that no mapping is registered.onMetadataReadWarning(cb)andMetadataReadWarningEvent— a subscribe/unsubscribe channel, sibling in shape toonWriteWarningandonSaveAdvisory(one long-lived instance,AdapterProviderwires it once), and deliberately not a payload pushed down either of those: both announce a write that succeeded, so carrying a failed read on one would make the event lie.operationandkindare single-member unions on purpose — one emitter exists, and a second is an additive reviewed widening rather than something a consumer's exhaustive switch meets at runtime.console.warnis kept verbatim, on both arms. It was never the problem; being the only discriminator was. It is not re-levelled and not made conditional.packages/app-shell—AdapterProvidersubscribes and renders through a newmetadataReadWarningToast.ts, so a user without devtools open can tell "there are none" from "we could not find out". The remedy is chosen by an exhaustive switch with anevercheck (a ternary would silently render the wrong fix for a third reason). The server'scode/ status /messageride along verbatim, untranslated — they are the evidence, and translating them would need a key per producer wording.packages/i18n— threeconsole.importMappings*keys in all ten locale packs.Triage requires this card be done together with objectui#7740 — two independent causes of one user-visible symptom ("the selector doesn't appear"). #7740 carries
needs-user-decisionand is sitting in the decision box unanswered, so the batch is unsatisfiable today and the PM dispatched this card alone rather than hold a ruled, unblocked p2 behind an unruled sibling.⇒ On
sys_userthis PR changes nothing.listImportMappingsis not on that wrapper at all —createIdentityImportDataSourcespreads a class instance and drops the prototype method (that is #7740's bug, not addressed here, and its design question is not decided here under any framing).packages/app-shell/src/views/identityImport.tsis untouched by this diff.It is nonetheless fully load-bearing for every other object — including
crm_plant_cost, which is where the objectstack#14026 misdiagnosis actually happened. ⛔ The symptom is not fully closed by this PR.Verification
Everything below on
f7d57b728.Tests —
pnpm exec vitest runfrom the repo root:packages/data-objectstack/+packages/i18n/Test Files 118 passed (118)·Tests 1802 passed (1802)packages/app-shell/Test Files 637 passed (637)·Tests 6120 passed | 1 skipped (6121)Type-check —
pnpm --filter @object-ui/data-objectstack --filter @object-ui/app-shell --filter @object-ui/i18n type-check, all threeDone(script names echoed, so nothing was silently skipped; each package's tsconfig includes its tests).Reverse verification — three ablations, each committed-then-mutated, each restore proven byte-identical (
git diff HEADempty andgit hash-objectequal to the HEAD blob). No rebuild leg is needed or claimed:vitest.config.mtsaliases@object-ui/data-objectstacktopackages/data-objectstack/src, so every suite below resolves source, notdist.classifyImportMappingsFailurestops readingerrand answers one arm always10 failed | 9 passed, including all threeQUIETcasesemitMetadataReadWarning(...)call is cut9 failed | 14 passedacross both suitesAdapterProvidersubscription is cut2 failed | 21 passed; the producer suite stayed greenThat third row is the reason the wiring test exists: it is the blind spot
AdapterProvider.advisorySink.test.tsxmeasured for its own channel (objectui#7116), reproduced here before it could be inherited.Gates run locally, green:
check:i18n-keys·check:i18n-drift·check:i18n-dead-keys·check:control-bytes·check:unreferenced-sources·check:vi-mock-inherit·check:vi-mock-specifiers·check-changeset-presence·check-changeset-no-major·check-changeset-overwrite·check-changeset-fixed·check:phantom-deps·check:self-import·check:entry-guard·check:esm-specifiers·check:handler-key-reads·check:side-effects-array·lint:coverage(46/46) ·type-check:coverage(45/46, 42/42 test projects) ·check-governed-queue-guard --test(NOT GOVERNED, 17 paths, none matched).check:readme-exportsreports PREREQUISITE NOT MET, not red: it needs every package'sdist/on disk and namesplugin-timeline/plugin-gantt— packages outside this diff. Left to CI.Lint — a declared, measured narrowing.
eslintwas run on the 16 changed.ts/.tsxfiles rather than repo-wide, and the narrowing excludes nothing:eslint.config.jsmatchesfiles: ['**/*.{ts,tsx}']with top-levelignoresof**/dist,**/.next,**/node_modules,**/public,**/.source. All 16 changed files are inside that population.--format json:files judged: 16,errors: 0,warnings: 127— and exactly 1 of those 127 sits on a line this branch added (no-explicit-anyin the pin, matching the idiom already on line 107 of the same file)..github/workflows/lint.ymldeliberately sets no--max-warnings.tseslint.configs.recommended, notrecommendedTypeChecked, and declares noparserOptions.project/projectService— no rule reads the type graph, so this diff cannot move any untouched file's verdict.The repo-wide
turbo run lint,pnpm testand the full gate farm are CI's.Out-of-scope finding
Filed as objectui#8151 (unassigned, unlabelled, for triage):
listViewsin this same file carries the identical swallow, and its user-visible cost is arguably larger — an empty view switcher on a lapsed token reads as "this object has no saved views". ⛔ Deliberately not fixed here: it needs its own reading of which failures deserve to stay quiet (viewis a long-standing declared kind, so "an older server without this kind" is not its story), and folding it in would have widened the published event union and added a verification surface this card's gate set does not cover.🤖 Generated with Claude Code
https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S
Generated by Claude Code