Skip to content

Commit b8ec127

Browse files
os-elon-muskclaude
andauthored
fix(spec): resolve permission-set and seed object references against the release artifact, not the single stack (#18212)
Fixes #18202 Part of #14122 — the epic tracking the one-artifact/N-packages family. That tracker stays open. ## Rework after a contract-review FAIL The isolated contract review on head `b0072379` returned **FAIL** and the bolded compatibility claim this body used to carry was false. Three corrections landed on this branch; nothing else moved, and the changeset level stays `minor` (the review judged the level correct and the body wrong). | review finding | correction | |---|---| | the two new collectors carried **0** `Array.isArray` guards and **0** malformed warnings, while step 3 of the same `composeStacks` body carries 3 and 1 — so a malformed `permissions` / `data` on an unparsed input raised a bare `TypeError` (no `code`, no `status`, outside the ADR-0112 envelope) where `main` composes with a warning | both collectors now guard their own shapes: a non-array collection is announced through `warnMalformedCollectionKey` and skipped, and a non-object entry — or a non-string `object` — carries no reference for the rule to resolve and is skipped | | the artifact pass runs `for (const stack of stacks)` over **every** input, while `defineStack` returns at `if (!strict)` before cross-reference validation ever runs — so `strict: false` and hand-built inputs **are** newly refused | the claim is replaced by the invariant that holds, in the docstring, the changeset and this body; the narrowing is pinned in the fixture as declared behaviour | | `if (stacks.length === 1) return stacks[0];` is unchanged, so a one-input composition never reaches the pass, and the docs bullet and option docstring overstated the guarantee | both now say **"in a composition of two or more packages"**, and a fixture block fences the qualifier | ### Round 2 of the rework — one clause, two surfaces The second contract review re-measured all three findings above and recorded them resolved (36 probes; the guards hold with no `TypeError`, the `strict: false` refusals are pinned by five positive fixtures, the one-input qualifier is fenced, and it confirmed the false-green fixture correction). It failed on one sentence. The changeset and this body described the newly-refused input's prior fate with the phrase `previously composed with only a warning` — quoted here as the text that was REMOVED, and appearing nowhere else in this PR. That was false. On `main` this class of input produces **no diagnostic at all** — `composeStacks` calls `validateCrossReferences` zero times and `console.warn` zero times, and its one `warnMalformedCollectionKey` site fires on `declared.length !== arrays.length`, i.e. a collection key that is not an array. The newly-refused inputs carry well-formed arrays holding a dangling reference, so that condition never holds. The clause conflated two populations and handed an upgrading reader a false self-test — *"we never saw a warning, so this is not us"* — in text that ships verbatim into `CHANGELOG.md`. Corrected in the changeset and here. Taken in the same stroke, the precision the review recommended: the no-op half of the invariant holds for an input that passed the strict parse **and did not opt in**, which is now said in the changeset, in the `collectArtifactCrossReferenceErrors` docstring and in the fixture file's header, which carried the same sentence. Text only — no schema, no rule, no fixture and no docs page moved, and the level stays `minor`. ## What changed `defineStack` validated cross-references **per stack**, so "defined in `objects`" and "defined in the artifact" were the same question. In an ADR-0130 artifact they are two questions, and the two accepted records could not both hold once the `type: app` package owns objects of its own: - ADR-0130 D1 — one artifact, N packages co-owning one namespace. - ADR-0130's 2026-09-02 addendum (#14487) — **permission sets stay whole in the `type: app` package**, because a set is authored per ROLE and ADR-0086 D3 gives it exactly one owning package. So the app package's sets necessarily grant on its modules' objects, and every such grant was refused. Measured downstream on `objectstack-ai/hotcrm` `claude/issue-1907-sales-app-service-module` (`be11c07`) as 18 grants across 7 sets; `data[].object` was refused the same way, which is why that branch had to push its seed rows into the module. This PR gives the two reference classes that legitimately cross a **resolution scope of their own**, and moves their refusal to the pass that owns that scope. ### `DefineStackOptions.artifactObjects` ```ts const service = defineStack(serviceConfig); // owns crm_case const app = defineStack(appConfig, { // owns crm_account, grants on crm_case artifactObjects: service.objects?.map((o) => o.name), }); export default composeStacks([service, app], { manifest: 'preserve' }); ``` `permissions[].objects` and `data[].object` resolve against the stack's own objects **plus** these names. Nothing else in the function moves. ## The measurement that chose the mechanism The dispatch proposed gating the widening on the stack declaring non-empty `manifest.dependencies`. **Measured against the reporter's own repro, that gate does nothing for this defect**, and the finding is worth recording because the card's title carries the same framing: | package | id | declares | refuses what | |---|---|---|---| | `src/sales` | `app.objectstack.hotcrm` | `type: app`, **no `dependencies`** | its sets grant on `crm_case` — **this is the failing stack** | | `src/service` | `app.objectstack.hotcrm.service` | `type: module`, `dependencies: { 'app.objectstack.hotcrm': '^3.1.0' }` | nothing — it grants on its own objects | The dependency edge runs **module to app**, which is the only direction ADR-0116's topological order admits: the module registers after the package it extends. The app therefore cannot name its modules as dependencies without inverting that order. Resolving against the referencing package's **declared dependency closure** would refuse exactly the shape the addendum makes mandatory. ⇒ The resolution scope is the **artifact**, which is what ADR-0130 D1 says it is — not the dependency closure, and not "anything, checked later". ## The two classes that cross, and the ones that must not Every object-name reference class the check covers, with the per-class ruling: | class | crosses? | why | |---|---|---| | `permissions[].objects` | **yes** | the 2026-09-02 addendum forces it | | `data[].object` (seed) | **yes** | rows placed into a co-owned object, not a claim on its definition; the same shape, and the card measures it | | `hooks[].object` | **no** | ADR-0130 §1.5 measured this refusal and recorded it as the seam: *"the split must follow hook ownership"* | | app `navigation` `objectName` | **no** | same paragraph: *"navigation crosses only through contributions"* (`requiresObject` remains the declared per-item escape) | | view `data.object`, `mappings[].targetObject`, `actions[].objectName` (both positions) | **not widened here** | no record rules on them and the repro does not need them; leaving them refused is the conservative direction under Prime Directive #12. Named under *Follow-ups* below. | The first two blocks of the new fixture assert that `artifactObjects` does **not** widen the hook and navigation refusals even when the name is listed. ## The refusal MOVED — `composeStacks` is the named pass `composeStacks` did not re-run any cross-reference check at all (it only checked composed action-key collisions), so deferring without adding one would have opened a hole. It now re-checks the two artifact-scoped rules over the composed object set: - same `code` — `STACK_CROSS_REFERENCE_INVALID` — and the same `status: 422`; - the same per-finding message, byte-identical, so one grep serves both passes; - a distinct **header** — `composeStacks artifact cross-reference validation failed (N issues):` — because only the pass differs. A name `artifactObjects` claims and no package in the artifact defines is therefore still refused, loudly, at a named pass — **in a composition of two or more packages**. That fixture is an acceptance criterion of the card, and it is `ArtifactPass` in the new test file. ### What it can and cannot newly refuse An earlier revision of this body claimed the pass "cannot newly refuse a composition that passes today". That is false, and the contract review measured it twice. The invariant that actually holds is two statements, not one: - **An input that passed the strict `defineStack` parse AND did not opt in cannot newly fail here.** It does not read the option (recording the option on the returned stack would put a composition-time concern on `ObjectStackDefinitionSchema`, i.e. on every artifact already built). It re-runs the two rules for every input, which is a no-op for such an input — its references already resolved against its own objects, and those are a subset of the composed set. The qualifier is load-bearing: an input that **did** opt in also passed the strict parse, but it resolved against its own objects *plus the names it listed*, and checking a listed name against the real artifact is precisely what this pass is for — so an opted-in input can fail here by design. That is the `ArtifactPass` fixture, not a gap. - **An input that BYPASSED the strict parse is checked for these two rules here for the first time.** `defineStack(config, { strict: false })` returns before `validateCrossReferences` runs at all, and a hand-built stack object never enters it. Such an input carrying a dangling `permissions[].objects` key or `data[].object` is now refused at composition where it previously composed **with no diagnostic at all** — the existing non-array warning covers a malformed collection key, not a dangling reference. The second is a real narrowing of what `composeStacks` accepts. It is the Prime Directive #12 direction — reject off-spec input loudly — and it is now **declared**: the changeset states it, `collectArtifactCrossReferenceErrors`'s docstring states it, and the fixture pins it rather than leaving it to be rediscovered as a regression. What the narrowing is not is a licence to crash. Both collectors guard their own shapes, so an unparsed input carrying a non-array `permissions` / `data`, a `null` entry, or a non-string `object` is warned about and skipped the way step 3's concat pass already treats it — never a bare `TypeError` outside the ADR-0112 envelope. Inputs declaring **no** objects are still skipped, inheriting `validateCrossReferences`'s own `objectNames.size === 0` leniency verbatim; that leniency is why hotcrm#1449's measurement never saw the defect, and it is pinned as untouched in both directions. **Known boundary — this pass runs only for two or more packages.** `composeStacks` returns `stacks[0]` untouched for a single input, so a one-package composition never reaches the pass, and neither does a stack that is never composed. That is the population whose `artifactObjects` claim stands unverified — the same shape as `strict: false`, for the same reason. Every reader-facing statement of the guarantee now carries the qualifier. `os build` composes. ## Compatibility — #14122 §6 > 存量单 `manifest` 产物经新装载路径后,注册结果逐位相同 Nothing in this PR touches the load path, the artifact schema or any authored key. `defineStack` is unchanged for a stack that does not pass `artifactObjects`: the widened scope is literally the same `Set` object as the narrow one when the option is absent. The three existing cross-reference pin suites pass unchanged, and `check:authorable-surface` / `check:api-surface` are both green without regeneration. `composeStacks` is where the acceptance set does move, for exactly one population — inputs that bypassed the strict parse — and that is stated above, in the changeset and in the docstring rather than claimed away. §6's own subject (a stored single-`manifest` artifact replayed through the load path) is untouched by it: the artifact pass runs at composition time, on the inputs, and never at load. ## Verification Re-run in a worktree off `origin/main` `d4554d4`, at **`0c21631c`** — the rework head. Every exit code below was redirected to a file and captured before any pipe. | what | command | result | |---|---|---| | package suite | `pnpm --filter @objectstack/spec test` | exit 0 — **478 files / 13632 tests passed** | | package typecheck | `pnpm --filter @objectstack/spec typecheck` | exit 0 (incl. `check:test-typecheck`) | | the reworked fixture file | `vitest run src/stack-artifact-crossref.test.ts` | exit 0 — **30 tests passed** (16 before the rework, 14 added) | | generated artifacts | `pnpm --filter @objectstack/spec check:generated` | exit 0 — **all 15 up to date**, nothing to regenerate, tree clean after the build | | gate families | `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` derived **106**; each run singly with its exit code captured before any pipe, then `--ran` reconciled | **106 derived, 106 run, 0 NOT-MEASURED, 0 UNRUN**, every one exit 0 | | closure build | `pnpm exec turbo run build --filter='./packages/*' --filter='./packages/*/*' --concurrency=2` | **72/72 successful** | | repo-wide lint | `pnpm lint` (`eslint . --no-inline-config`) — the whole union, not a narrowing | exit 0, at `0c21631c` | Seven of the 106 first returned a PREREQUISITE reading — six `exit 3` (`PREREQUISITE NOT MET`) and `check:skill-examples` `exit 1` whose body says *"packages/client-react/dist holds no .d.ts declarations — build first, then re-run"*. Those are NOT-MEASURED, not reds: all seven were re-run after the closure build and all seven exit 0. They are `check:dual-build-cjs-loads`, `check:lean-entry-closure`, `check:type-check-debt`, `check:docs-transcript-drift`, `@objectstack/lint check:doc-formula-expressions`, `@objectstack/lint check:doc-security-posture` and `@objectstack/spec check:skill-examples`. **Carried from the pre-rework head `b0072379`, and why it is still valid:** the consumer sweep (`turbo run typecheck --filter='...@objectstack/spec'`, the DOWNSTREAM direction, 139/139) was not re-run. The rework changes two internal function bodies, docstrings, a fixture, a changeset and a docs page — no exported type, no published `exports` entry. `check:api-surface` is green with no regeneration owed and `check:generated` reports all 15 artifacts up to date, so the published declaration surface is byte-unchanged and no import site can have moved. **Known staleness, declared:** this branch is 12 commits behind `origin/main` and `dispatch-gates.mjs` says so in its own words — six of the files its derivation reads changed across that range. The 106 is therefore this tree's derivation, not `origin/main`'s. The branch is deliberately not merged forward: it is a targeted rework of an open PR that a reviewer is about to re-read against its recorded FAIL, and CI runs the full farm on the merge result anyway. ### Reverse verification (one-off, not committed tests) Two legs, each committed first, then mutated, then restored under a `trap` whose restore path is absolute and spelled `git checkout HEAD --`. Both restores are proven by blob identity and an empty `git diff HEAD`, never by an exit code. No build step is involved: these tests import `./stack.zod` by relative path, not through the package `exports`. **Leg 1 — the shape guards.** Prediction, written before the run: removing the two guards turns the malformed-input block red with bare `TypeError`s. Mutation proven on disk — `Array.isArray` inside the two collectors 2 to 0, `warnMalformedCollectionKey` call sites 3 to 1, blob `8debc710` vs HEAD `690c80dd`. Observed: **5 failed / 25 passed**, every failure a bare `TypeError` (`permissions is not iterable`, `datasets is not iterable`, `Cannot read properties of null`, `name.startsWith is not a function`). The sixth case in that block stays green under the mutation and is honestly a control, not a guard pin: `objects: null` on a permission entry was already covered by the pre-existing `!grants || typeof grants !== 'object'` line. One correction worth recording, because the first attempt of this leg measured a false green: the non-array `permissions` fixture originally used a **string**, and a string is iterable, so removing the guard changed nothing for it. It now uses map format — not iterable — and fails without the guard. The fixture was fixed and the leg re-run from the committed state. **Leg 2 — the one-input boundary.** Prediction: deleting `if (stacks.length === 1) return stacks[0];` turns the two "accepts a one-input composition" fixtures red and leaves the rest green, proving those fixtures actually observe the early return rather than merely asserting an accept. Mutation proven on disk (occurrences 1 to 0, blob `e7c7b384` vs HEAD `690c80dd`). Observed: **exactly 2 failed / 28 passed**, both in the one-input block. Restored, `git status --porcelain` empty. ## `docs-drift-check` advisory — decided, not deferred The advisory flagged five pages. Decisions, so a reviewer does not re-derive them: - **`content/docs/getting-started/glossary.mdx`** — names `composeStacks()` once, in passing ("the authoring form … or composes one with `composeStacks()`"). Nothing it states is falsified. **No edit.** - **`content/docs/getting-started/examples.mdx`** — its *"rules worth knowing before you split a product this way"* list is the page a reader lands on for exactly this shape, and nothing already there becomes untrue. But the list would have been silently missing the rule this PR creates, so it gains **one bullet** naming the two artifact-scoped classes, the `artifactObjects` spelling, and the hook exception. That is the documentation half of the option, not scope creep. - **`content/docs/releases/v17/*` (three pages)** — release-owned under AGENTS.md's Documentation Guardrails, and read-only in a code PR. **Not edited here**, and none of them was found to be wrong: they describe shipped behaviour this PR does not change. Note the advisory was computed on the merge commit `318def6e`, not on this PR's head, so a local re-derivation can legitimately differ. ## Acceptance notes - `check-plugin-teardown-shape.mjs --self-test` exits 1 on a shallow checkout — it pins its positive control to a commit the clone cannot reach and says so rather than passing. Fetching that one commit clears it (47 cases pass). Not a finding, and not this PR's to change. - Four gates exit **3** (`PREREQUISITE NOT MET`) until the package closure is built: `check:dual-build-cjs-loads`, `check:lean-entry-closure`, `check:type-check-debt`, `@objectstack/lint check:doc-formula-expressions`. All four are green after the closure build. Working as designed; recorded so the exit-3 readings in this PR's history are not read as reds. - The docs bullet first said the ADR-0090 D3 reserved word for a position; `pnpm check:role-word` caught it in the same local run and it now says **position**. Recorded because the word is ratcheted repo-wide and the gate is easy to meet only if you know it exists. - The `StackCrossReferenceError` docstring said "there is exactly ONE raise site". With the artifact pass there are two, so the paragraph now names both and says why they share one `code`: the code names the rule family (what a machine matches), the header names the pass (what a reader needs). - **`composeStacks` step 2 raises a bare `TypeError` for a non-array `objects` on an unparsed input, and always has.** `mergeObjects` iterates `stack.objects` with no guard; probed against the built `dist`, `objects` as a map and `objects` as a number both give `TypeError: stack.objects is not iterable`, `code` undefined, `status` undefined. `mergeObjects` is **not touched by this diff** (`git diff d4554d4..HEAD -- packages/spec/src/stack.zod.ts` has no hit on it), so this is a pre-existing `main` defect of the same family as the one this rework fixes, not a regression. It is reported for filing rather than fixed here: the rework dispatch is explicitly scoped to the three review findings, and widening it would be the second time this PR's scope moved. Dedupe words: `composeStacks`, `mergeObjects`, non-array `objects`, bare `TypeError`, ADR-0112 envelope. - `isPlatformObjectName` calls `name.startsWith` with no string guard. No live call site can reach it with a non-string now that the seed-data collector filters on `typeof === 'string'` (the other caller passes `Object.keys` output), so this is noted, not filed. - The `warnMalformedCollectionKey` call added inside each collector can never be the FIRST to warn for its key in today's code: step 3 of `composeStacks` runs before step 3b and warns for the same key, and the reporter deduplicates per key for the module's lifetime. It is kept anyway so the rule cannot become the silent one if a future caller reaches it first, and the fixture asserts the observable consequence — exactly ONE warning per key, not two. ## Follow-ups this PR does not take - **The remaining object-name classes.** View `data.object`, `mappings[].targetObject` and `actions[].objectName` stay stack-scoped. Whether a co-owning package may carry a view over, an import mapping into, or an action bound to a sibling's object is a contract question no record answers, and the repro does not need it. Flagged for the `domain:spec` seat rather than decided here. - **The bare-`Error` envelope gap on these refusals** is #14552's, deliberately untouched — and the class this PR moves **keeps its envelope shape**: both passes raise `StackCrossReferenceError`, `code` `STACK_CROSS_REFERENCE_INVALID`, `status` 422, `issues` one entry per finding. - **Record the seed-data reasoning on the card.** The contract review's Flag 1 accepted the widening but asked for one thing to be written down: ADR-0130's 2026-09-02 addendum says *"Modules group Data, Automation and Interface; they do not group Access."* A permission set is Access, so it crosses by that sentence. Seed rows are **Data** by the same sentence, and the addendum does not rule on them — they cross here because the card asked for it and because a seed row places data into a co-owned object rather than claiming its definition. The next author will read that sentence and ask why seed data crossed and views did not; the answer is this paragraph, and it belongs on #18202 rather than only in a review thread. - **The downstream end-to-end.** hotcrm's repro branch would need `artifactObjects` threaded into `createHotCrmAppStack` and a local `@objectstack/spec` link; the sister checkout is read-only here and that branch belongs to another card, so this PR relies on the fixture, which reproduces the two-package shape at its smallest. - `packages/cli`, `packages/lint` and `examples/**` are untouched — the sibling dispatch on #18204 owns them this round. --- _Generated by [Claude Code](https://claude.ai/code)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent cb648cb commit b8ec127

4 files changed

Lines changed: 865 additions & 48 deletions

File tree

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
---
2+
'@objectstack/spec': minor
3+
---
4+
5+
`defineStack`: a package of a multi-package release artifact can now grant permissions on, and seed data into, an object one of its SIBLING packages owns.
6+
7+
**FROM** — every `permissions[].objects` key and every `data[].object` had to name an object the same stack declares. In an ADR-0130 artifact this made two accepted records contradict each other: the 2026-09-02 addendum keeps every permission set whole in the `type: app` package, so as soon as that package also owns objects of its own, its sets were refused for granting on its modules' objects (`Permission 'sales_rep' grants on object 'crm_case' which is not defined in objects.`). The only escapes were `strict: false` for the whole package or splitting the sets per package, which contradicts the addendum.
8+
9+
**TO** — pass the artifact's other object names to `defineStack` and those two reference classes resolve against the artifact instead of the one stack:
10+
11+
```ts
12+
const service = defineStack(serviceConfig); // owns crm_case
13+
const app = defineStack(appConfig, { // owns crm_account, grants on crm_case
14+
artifactObjects: service.objects?.map((o) => o.name),
15+
});
16+
export default composeStacks([service, app], { manifest: 'preserve' });
17+
```
18+
19+
Nothing else widens. `hooks[].object` and an app's own `navigation` `objectName` stay refused against the stack's own objects even when the name is listed, because ADR-0130 §1.5 records both refusals as the shape of the package seam.
20+
21+
The refusal moved rather than disappearing: in a composition of **two or more** packages, `composeStacks` now re-checks those two classes over the composed artifact, so a name `artifactObjects` claims and no package in the artifact defines is refused there, with the same `STACK_CROSS_REFERENCE_INVALID` code, the same `422`, and the same per-finding message. Only the header differs, naming the pass that refused it. `composeStacks` returns a single input untouched, so a one-package composition does not re-check the claim.
22+
23+
**What that changes about which inputs `composeStacks` accepts.** `defineStack` itself is unchanged for a stack that does not pass `artifactObjects` — every single-package app validates exactly as before. `composeStacks` is not: it applies the two artifact-scoped rules to **every** input carrying objects, not only the ones that opted in. For an input that passed the strict `defineStack` parse **and did not opt in**, that is a no-op, so such an input cannot newly fail — its references were already resolved against its own objects, which are a subset of the composed set. (An input that *did* opt in also passed the strict parse, but it resolved against its own objects plus the names it listed; checking a listed name against the real artifact is what this pass is for, so it can fail here by design.) For an input that **bypassed** the strict parse the no-op argument does not apply at all: `defineStack(config, { strict: false })` returns before cross-reference validation runs, and a hand-built stack object never enters it, so these two rules have never been applied to it. Such an input carrying a dangling `permissions[].objects` key or `data[].object` is now refused at composition where it previously composed with no diagnostic at all — the existing non-array warning covers a malformed collection key, not a dangling reference. If you compose unparsed stacks, that is the one behavioural change to expect, and there is no earlier warning to have noticed it by; a malformed `permissions` / `data` on such an input is still skipped with that non-array warning rather than raising.

‎content/docs/getting-started/examples.mdx‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -432,6 +432,18 @@ The rules worth knowing before you split a product this way:
432432
may reference an object another package owns. An app's own `navigation` may
433433
not point outside its package — inject into another package's app with
434434
`navigationContributions` instead.
435+
- **Permission sets and seed data cross the boundary; declare what they reach.**
436+
A permission set is authored for a **position**, not for a module, so the
437+
`type: 'app'` package keeps every set whole and grants across the whole
438+
artifact — and seed rows are the same shape. Both resolve against the artifact rather than the one
439+
stack, but only once you hand `defineStack` the sibling object names:
440+
`defineStack(appConfig, { artifactObjects: ordersStack.objects?.map((o) => o.name) })`.
441+
**In a composition of two or more packages**, `composeStacks` then refuses any
442+
name no package in the artifact defines, so a typo is still caught — one pass
443+
later, and by name. The qualifier is real: `composeStacks` returns a single
444+
stack untouched, so composing one package on its own does not check the claim.
445+
Hooks are the deliberate exception: a hook still belongs to the package that
446+
owns its object.
435447
- **One artifact, one version.** Everything inside ships, installs and upgrades
436448
together; you cannot hot-fix one module on its own. A module that needs its
437449
own release cadence belongs in its own artifact.

0 commit comments

Comments
 (0)