Skip to content

Commit 78c85e1

Browse files
dmealingclaude
andcommitted
fix: four defects a widened review found in unreviewed work
A code review of the @sortableDefaultOrder thread found six defects, two of them gates left red. That hit rate argued for widening to the commits nobody had looked at, scoped by blast radius rather than by date: the three marked breaking plus two codegen-correctness ones. It found a REGRESSION, not just latent debt. 1. outputLayout "package" — the fix was HALF-APPLIED and shipped a regression. The built-in barrel moved to effectivePackage(); the four reference/owned copies — the ones `meta init` actually scaffolds — kept the bare `.package` read and merely gained an unused import. Their emitted index.ts exported "./Widget" for a file at acme/commerce/Widget.ts: TS2307 on a project with nothing wrong with it. It is a regression rather than a leftover, because before the change the entity files landed flat too and the barrel agreed with them. The gate could not see it. It asserts on EMITTED PATHS, and a barrel's path is correct whatever it exports — a wrong import SPECIFIER is structurally out of its reach, so the earlier claim that "a new generator reaching for .package again fails without anyone maintaining a list" was not true of the barrel. It now also runs the barrel and requires every export specifier to name a file the same run actually wrote; proven by reverting, which names "Widget". The comment that caused the defect was still in place stating the opposite of the code — it told the reader to pass obj.package "for consistency with the emitter", which is how the bare read propagated. Rewritten to say why consistency with a wrong emitter is not a reason. Three leftover unused effectivePackage imports removed; nothing flagged them, as the workspace sets no noUnusedLocals. 2. attr("isArray") guarded ONE door of four. attr()/hasAttr() threw; ownAttr(), ownHasAttr() and ownMetaAttr() asked the identical forbidden question and answered it just as silently, and ownAttr is a documented public accessor an owned generator can reach for. One rule with four doors, three still open, is this repo's most-repeated defect shape. All four now guarded, `value` still exempt (an attr node reads its own payload through ownAttr(RESERVED_KEY_VALUE), so the exemption must be settled before the guard). The derived test covers all four, and its header now says what the derivation does NOT reach: STRUCTURAL_KEYWORD_ACCESSORS is hand-written, so a keyword added to RESERVED_KEYS and not to that map gets no runtime guard — the loop goes red and says so, which is the forcing function. Five parser tests used ownHasAttr as the negative probe for "this reserved key is not stored as an attr". They now read ownAttrs(), which is literally the attr map each assertion's own description names. 3. The useEntityFetcher -> useEntityPathFetcher rename left four shipped surfaces on the old name, including the metaobjects-runtime-ui agent-context skill `meta init` installs into every project. It described the hook as "reads the fetcher from context (generated hooks call this)" — both halves false, and the omitted half is exactly the path rewrite the rename exists to warn about. That is the third time a shipped skill has taught something the code had already changed, so the two agent-context-conformance fixtures move with it. 4. Three tier-1 adopter docs still presented `bigserial(...)` in blocks headed as generated output; `meta gen` has emitted bigint(...) + .generatedByDefaultAsIdentity() since the identity fix. No gate covers these blocks — the doc-examples gate loads fenced METADATA only. Also adds CHANGELOG entries for all five reviewed commits. Three were marked breaking and none had one: a throw added to a core public API, a public export rename, and a gate whose verdict logic changed. Verified: workspace typecheck clean; ci-local --quick green (incl. owned-template -copies and site-reference freshness); sdk 304, codegen-ts 1575, metadata 2616, cli 838, tanstack 49, runtime-web 55 — all passing. Not fixed here, reported for judgment: docs-drift's `checked` count is machine- dependent (it includes disk-derived orphans) and its own test cannot fail on the denominator; the Angular fetcher seam has the same path rewrite with neither the rename nor a warning; and an ignored orphan is dropped from the check but not from the "git-ignored pages not checked" count. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent e9447af commit 78c85e1

22 files changed

Lines changed: 251 additions & 32 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,96 @@ rather than returning null for an absent name, so a name-keyed lookup would turn
149149
divergence between the allowlist and the map into an aborted build instead of a skipped
150150
entry.
151151

152+
### Changed — an attr lookup for a reserved structural keyword now THROWS
153+
154+
`attr("isArray")` could only ever answer `undefined`, and `undefined` is
155+
indistinguishable from "the author did not set that attribute". Reserved structural
156+
keywords are NATIVE PROPERTIES: `@isArray` is refused at the load with
157+
`ERR_RESERVED_ATTR`, so an attribute lookup for one asks a question the metamodel
158+
forbids anyone to answer.
159+
160+
Found in an adopter's own generator — `f.attr("isArray") === true ? base + "[]" : base`
161+
— whose generated reference document described **all 31 of that model's array fields as
162+
scalars**. Its sibling module had used the resolving accessor since the day it was
163+
written, with a comment naming the trap, and nothing compared the two. ADR-0039 is taught
164+
in three shipped skills and in that file's own header fourteen lines above the violation:
165+
prose is not a gate.
166+
167+
A throw rather than a warning, because there is no reading under which the call is
168+
correct and the fix is one line at the call site. The message names the native accessor
169+
to use (`.resolvedIsArray()`, `.name`, `.isAbstract`, `children()`, …).
170+
171+
**All four name-taking doors are guarded**, not just the resolving pair: `attr()`,
172+
`hasAttr()`, `ownAttr()`, `ownHasAttr()` and `ownMetaAttr()`. The own-only accessors ask
173+
the identical forbidden question and answer it just as silently, and `ownAttr` is a
174+
documented public accessor an owned generator can reach for — one rule with four doors,
175+
three left open, is this repo's most-repeated defect shape. `value` stays exempt: an
176+
`attr` node reads its own typed payload through `ownAttr(RESERVED_KEY_VALUE)`, which is
177+
the one legitimate reserved-key attr read in the model.
178+
179+
Potentially breaking for a consumer that made such a call — but every such call was
180+
already returning `undefined`, so the behaviour it depended on was a silent wrong answer.
181+
No metadata changes; `metamodelVersion` is unchanged. TypeScript only: Python's own-only
182+
`attr()` and the JVM/C# equivalents carry no guard yet, tracked as follow-up.
183+
184+
### Changed — `useEntityFetcher()` is renamed `useEntityPathFetcher()`
185+
186+
The hook did not merely read the fetcher from context — it returned one that **prepends
187+
the provider's `baseUrl` to every path**. The name described the read half only, so an
188+
application-absolute path handed to it produced `<baseUrl>` + that path and a silent 404.
189+
`useEntityFetcher` remains as a deprecated alias (literally the same function), so nothing
190+
breaks on upgrade; the rename is what makes the rewrite visible at the call site.
191+
192+
Generated hooks emit ENTITY-relative paths and call the new name. Because the reference
193+
templates compose `renderHooksFile` / `renderGridHookFile`, an ejected copy picks the
194+
rename up with no edit.
195+
196+
The shipped surfaces that still taught the old name are corrected in the same release —
197+
including the `metaobjects-runtime-ui` agent-context skill `meta init` installs into every
198+
project, which described it as "reads the fetcher from context" with no mention of the
199+
rewrite. That is the third time a shipped skill has taught something the code had already
200+
changed, so the two `agent-context-conformance` fixtures move with it.
201+
202+
### Fixed — `outputLayout: "package"` was inert for the form the docs teach
203+
204+
A file may declare its package once at the root (`{"metadata.root": {"package":
205+
"acme::blog", …}}`) or on each object; the loader treats these as equivalent. Codegen did
206+
not: every path decision read the object's OWN bare `.package`, which is `undefined` for
207+
the root-declared form, so `packageToPath(undefined)` returned `""` and every file landed
208+
flat. With `outputLayout: "package"` the emitted tree was **byte-identical to `flat`** —
209+
not a broken build, a config key that reads as honoured and does nothing.
210+
211+
It mattered because root-level is the form the product TEACHES: the TypeScript quickstart
212+
uses it, and across this repo's own metadata it is 614 files against 7 using object-level.
213+
It survived because the single fixture behind the package-layout golden is one of those 7
214+
— the gate was written on the rare form, so the common form had never been generated in
215+
package layout by any test.
216+
217+
**The first fix was itself half-applied**, and that half is fixed here too: the built-in
218+
`barrel` generator moved to `effectivePackage()` while the four reference/owned copies —
219+
the ones `meta init` actually scaffolds — kept the bare read and merely gained an unused
220+
import. Their emitted `index.ts` therefore exported `./Widget` for a file at
221+
`acme/commerce/Widget.ts`: `TS2307` on a project with nothing wrong with it, and a
222+
REGRESSION, since before the change the entity files landed flat too and the barrel
223+
agreed with them. The comment that caused the original defect — which instructed the
224+
reader to pass `obj.package` "for consistency with the emitter" — was still in place
225+
stating the opposite of the code, and is rewritten to say why consistency with a wrong
226+
emitter is not a reason.
227+
228+
The path-level gate could not see any of it: it asserts on EMITTED PATHS, and a barrel's
229+
path is correct whatever it exports. It now also runs the barrel and requires **every
230+
export specifier to name a file the same run actually wrote**.
231+
232+
### Fixed — an `increment` primary key was spelled `serial` while migrate wrote IDENTITY
233+
234+
Generated Drizzle emitted `serial`/`bigserial` for a `@generation: increment` key while
235+
`meta migrate` emitted a `GENERATED BY DEFAULT AS IDENTITY` column — two doors answering
236+
one declaration with two different DDL forms. Codegen now emits `integer`/`bigint` plus
237+
`.generatedByDefaultAsIdentity()`, matching migrate. Verified against both ends of
238+
`codegen-ts`'s declared drizzle-orm peer range, and the PK stays insert-optional (compiled
239+
with the real TypeScript compiler for `field.int` and `field.long`). The three adopter
240+
docs that showed `bigserial` as generated output are corrected.
241+
152242
### Changed — `meta eject` says how your copy compares to the reference
153243

154244
An owned generator is the one artifact ADR-0034 hands an adopter and then never speaks

‎agent-context/skills/metaobjects-runtime-ui/references/tanstack.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,8 @@ rejects and poisons every later install in the project with `ERESOLVE`.
3434
| Export | Purpose |
3535
|---|---|
3636
| `<EntityFetcherProvider fetcher={fetcher} baseUrl="/api">` | supplies the single `EntityFetcher` every generated hook reads, plus the base URL it prepends. `baseUrl` is optional (default `""` = same origin at the root); generated hooks emit entity-relative paths. |
37-
| `useEntityFetcher()` | reads the fetcher from context (generated hooks call this) |
37+
| `useEntityPathFetcher()` | reads the fetcher from context and returns one that PREPENDS the provider's `baseUrl` — generated hooks call this, and the path they pass is entity-relative. |
38+
| `useEntityFetcher()` | deprecated alias of the above. The name said "read the fetcher"; it also rewrites every path, so passing an application-absolute path yields `<baseUrl>` + that path and a silent 404. |
3839
| `<EntityGrid>` | opinionated TanStack Table component |
3940
| `<CellRendererProvider>` + `defaultCellRenderers` | renderer overrides keyed by the column's `meta.view` |
4041

‎client/web/packages/runtime-web/src/join-base-url.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
* Join a client base URL to a generated entity-relative path.
33
*
44
* ONE implementation, shared by both client tiers on purpose. `@metaobjectsdev/tanstack`
5-
* wraps `useEntityFetcher()` with it and `@metaobjectsdev/angular` wraps the
5+
* wraps `useEntityPathFetcher()` with it and `@metaobjectsdev/angular` wraps the
66
* `EntityFetcherToken`; implemented separately they would eventually disagree about a
77
* trailing slash, and the disagreement would surface as a 404 in one framework only.
88
*

‎docs/features/api-contract.md‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,10 @@ generated):
4747

4848
The fetcher is supplied once via `<EntityFetcherProvider fetcher={fetcher} baseUrl="/api">`
4949
at the React tree root; every generated hook reads it via
50-
`useEntityFetcher()` from `@metaobjectsdev/tanstack`.
50+
`useEntityPathFetcher()` from `@metaobjectsdev/tanstack`, which prepends `baseUrl`
51+
to the ENTITY-relative path the hook passes it. (`useEntityFetcher()` is a
52+
deprecated alias — it rewrites paths too, which its name did not say, so handing it
53+
an application-absolute path produces `<baseUrl>` + that path and a silent 404.)
5154

5255
## URL grammar
5356

‎docs/features/entities.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -148,12 +148,12 @@ export const AuthorNames = {
148148

149149
```ts
150150
// generated/acme/blog/Author.ts
151-
import { pgTable, bigserial, varchar } from "drizzle-orm/pg-core";
151+
import { pgTable, bigint, varchar } from "drizzle-orm/pg-core";
152152
import { z } from "zod";
153153
import { AuthorNames } from "./Author.names";
154154
155155
export const author = pgTable(AuthorNames.sources.primary.table, {
156-
id: bigserial(AuthorNames.fields.id.column, { mode: "number" }).primaryKey(),
156+
id: bigint(AuthorNames.fields.id.column, { mode: "number" }).primaryKey().generatedByDefaultAsIdentity(),
157157
name: varchar(AuthorNames.fields.name.column, { length: 200 }).notNull(),
158158
bio: varchar(AuthorNames.fields.bio.column, { length: 2000 }),
159159
});

‎docs/features/field-types.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,7 @@ below names no column directly — to read the resolved spelling, open the names
227227
import { AuthorNames } from "./Author.names";
228228

229229
export const author = pgTable(AuthorNames.sources.primary.table, {
230-
id: bigserial(AuthorNames.fields.id.column, { mode: "number" }).primaryKey(),
230+
id: bigint(AuthorNames.fields.id.column, { mode: "number" }).primaryKey().generatedByDefaultAsIdentity(),
231231
name: varchar(AuthorNames.fields.name.column, { length: 200 }).notNull(),
232232
bio: varchar(AuthorNames.fields.bio.column, { length: 2000 }),
233233
priceCents: bigint(AuthorNames.fields.priceCents.column, { mode: "number" }).notNull(), // currency: minor units

‎docs/features/relationships.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,7 @@ import { AnyPgColumn } from "drizzle-orm/pg-core";
176176
import { PostNames } from "./Post.names";
177177
178178
export const post = pgTable(PostNames.sources.primary.table, {
179-
id: bigserial(PostNames.fields.id.column, { mode: "number" }).primaryKey(),
179+
id: bigint(PostNames.fields.id.column, { mode: "number" }).primaryKey().generatedByDefaultAsIdentity(),
180180
title: varchar(PostNames.fields.title.column, { length: 255 }).notNull(),
181181
authorId: bigint(PostNames.fields.authorId.column, { mode: "number" })
182182
.notNull()

‎docs/ports/typescript-client.md‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ a roadmap item: eject the generator and retarget its emit (FR-040).
5656
|---|---|---|
5757
| `@metaobjectsdev/runtime-web` | Pure framework-agnostic browser core. Zero React, zero TanStack, zero Node-only deps. | `formatCurrency`, `parseCurrency`, `minorUnitsFor`, `buildFilterQs`, type `EntityFetcher`, type `GridConfig` |
5858
| `@metaobjectsdev/react` | React-specific runtime (peer-deps on `react`, `react-hook-form`, `@hookform/resolvers`, `zod`). | `useEntityForm`, `<CurrencyInput>`, types `EntityMeta`, `EntityFieldMeta`, `BoundInputProps` |
59-
| `@metaobjectsdev/tanstack` | TanStack runtime (peer-deps on `@tanstack/react-query`, `@tanstack/react-table`). | `<EntityFetcherProvider>`, `useEntityFetcher`, `<EntityGrid>`, `<CellRendererProvider>`, `defaultCellRenderers` |
59+
| `@metaobjectsdev/tanstack` | TanStack runtime (peer-deps on `@tanstack/react-query`, `@tanstack/react-table`). | `<EntityFetcherProvider>`, `useEntityPathFetcher`, `<EntityGrid>`, `<CellRendererProvider>`, `defaultCellRenderers` |
6060

6161
## Codegen packages
6262

@@ -111,8 +111,11 @@ below).
111111

112112
## The `<EntityFetcherProvider>` contract
113113

114-
Every generated hook (React Query) calls `useEntityFetcher()`, which reads
115-
a single `EntityFetcher` function from React context. The consumer's app
114+
Every generated hook (React Query) calls `useEntityPathFetcher()`, which reads
115+
a single `EntityFetcher` function from React context AND returns one that prepends
116+
the provider's `baseUrl`. The path a generated hook passes it is therefore
117+
ENTITY-relative, never application-absolute — `useEntityFetcher()` is a deprecated
118+
alias whose name suggested only the read half. The consumer's app
116119
supplies the concrete implementation — auth headers, base URL, error
117120
handling — and the same fetcher serves every entity.
118121

‎examples/advanced-modeling/codegen/generators/barrel.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ export const barrel = function barrel(opts?: BarrelOpts): Generator {
6464
path: "index.ts",
6565
content: await formatTs(
6666
renderBarrel(
67-
entities.map((e) => ({ name: rc.valueObjectEmittedName(e), package: e.package })),
67+
entities.map((e) => ({ name: rc.valueObjectEmittedName(e), package: effectivePackage(e) })),
6868
rc.extStyle,
6969
rc.selfTarget,
7070
rc.entityModuleTarget,

‎examples/showcase/codegen/generators/barrel.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ export const barrel = function barrel(opts?: BarrelOpts): Generator {
6464
path: "index.ts",
6565
content: await formatTs(
6666
renderBarrel(
67-
entities.map((e) => ({ name: rc.valueObjectEmittedName(e), package: e.package })),
67+
entities.map((e) => ({ name: rc.valueObjectEmittedName(e), package: effectivePackage(e) })),
6868
rc.extStyle,
6969
rc.selfTarget,
7070
rc.entityModuleTarget,

0 commit comments

Comments
 (0)