Skip to content

fix(runtime): guard the legacy manifest.data seed read so a flat bundle collects each dataset once - #15605

Merged
os-litant merged 4 commits into
mainfrom
claude/issue-15262-app-plugin-seed-dataset-double-collect
Sep 5, 2026
Merged

fix(runtime): guard the legacy manifest.data seed read so a flat bundle collects each dataset once#15605
os-litant merged 4 commits into
mainfrom
claude/issue-15262-app-plugin-seed-dataset-double-collect

Conversation

@os-litant

Copy link
Copy Markdown
Collaborator

Fixes #15262

AppPlugin.start() collects seed data from two locations — the top-level data field, then the legacy manifest.data. The legacy read resolves its base as this.bundle.manifest || this.bundle, so on a flat bundle (manifest fields written directly on the bundle, no manifest: key — a shape AppPlugin supports by design, see the constructor's bundle?.manifest || bundle) it re-read the very array the top-level read had just contributed. Every seed dataset was collected twice.

The sibling two-location collector for translations has always carried the reference guard that makes the same read safe. That asymmetry was the whole defect, so the repair is the sibling's guard, not a third spelling:

-        if (manifest && Array.isArray(manifest.data)) {
+        if (manifest && Array.isArray(manifest.data) && manifest.data !== this.collections.data) {

Driven, not inferred — before/after

Two claims the card reasoned and the dispatch asked to be driven. Both are now driven, on the rig the card names (packages/runtime/src/app-plugin.seed.test.ts), against a flat bundle carrying one mode: 'insert' dataset with one record:

measurement before after
datasets reaching readSeedDatasets(ctx) 2 1
ql.insert calls for that one record (rows) 2 1

Verbatim from the red run:

× collects a flat bundle seed dataset ONCE, not once per read location
  AssertionError: expected [ { object: 'sys_user', ...(2) }, ...(1) ] to have a length of 1 but got 2
× applies a mode:insert record ONCE per boot on a flat bundle
  AssertionError: expected "vi.fn()" to be called 1 times, but got 2 times

So the distance between "collected twice" and "applied twice" is closed by measurement: it is doubled rows, not doubled work. mergeSeedDatasets (packages/runtime/src/seed-datasets.ts) was verified at source to be a plain list.push(...datasets) with no de-duplication of any kind — that is what carries the doubling past collection into the shared registry, the inline loader and every later per-org replay.

The third pin is the anti-vacuity control

A "fix" that simply deleted the legacy read would satisfy both rows above. The third pin refuses it: a bundle whose manifest.data is a genuinely different array from its top-level data must still contribute both (2 datasets, 2 inserts). It passes before and after — the legacy fallback is guarded, not removed.

Two-location collector enumeration (the card's open item)

The card asked whether any other two-location read in the same pass is missing the guard, and triage recorded that nobody had measured it. The dispatch narrowed it by one spelling (Array.isArray(manifest. matches exactly two sites) and explicitly asked for the real class instead of a repeat of that grep.

Class as measured: a site in app-plugin.ts that reads one logical collection from two locations (top level and manifest-nested) and accumulates both into one output.

Method: intersect two independently derived enumerations rather than grep one spelling —

  1. every accumulator in the file: grep -nE "\.push\(\.\.\." (4 hits) plus the record-merge and object-spread accumulators found by reading every this.collections. site;
  2. every read off a manifest-fallback alias: grep -oE "\b(sys|manifest)\.[A-Za-z0-9_]+" | sort | uniq -c (16 reads over 14 distinct keys), plus every manifest || / ?.manifest || base (8 sites).

Positive control: the histogram independently surfaced manifest.data and manifest.translations — the two known members — before any of them was looked for, and seedDatasets scores 6 hits in the same file.

Result — six members, and the repaired site is the only unguarded one:

site second location accumulates guard
data, in start() this.bundle.manifest || this.bundle push(...) NONE — this PR
translations, in loadTranslations() this.bundle.manifest || this.bundle push(...) reference check (pre-existing)
collectBundleHooks stack?.manifest?.hooks push per item per-item seen reference Set, and no || bundle fallback, so a flat bundle never re-reads
collectBundleActions stack?.manifest?.actions, stack?.manifest?.objects[*].actions push per item same seen Set, same absent fallback
collectBundleFunctionEntries stack?.manifest?.functions out[name] = fn idempotent by construction — a keyed record cannot double
security bundle, start() this.collections.manifest object spread key-level override, not accumulation

Ruled out of the class as either/or reads rather than accumulations: jobs (a ternary, this.collections.jobs or (this.bundle.manifest || {}).jobs, never both), i18nConfig (||), and the scalar sys.id / sys.name envelope reads.

⇒ A third two-location collector does exist — three of them — but every one is already guarded or structurally immune. No widening is needed and none is taken.

Upgrade condition: measured, and NOT met

Triage graded p2 explicitly because the trigger shape had been demonstrated only by this repo's own test, with no shipped application measured, and set: if any shipped app uses a flat bundle with a top-level data, re-grade p1.

Measured, so triage's stated unknown becomes a reading. Instrument: the TypeScript compiler API over every defineStack(...) / composeStacks(...) call in the repo, listing the argument literal's own top-level property names and flagging manifest absent + data present. Population: 128 candidate files (every non-test source under packages/, examples/, apps/, scripts/, skills/ mentioning either call), of which 19 are real object literals. Positive control, run FIRST: a synthetic flat bundle { id, name, data: [...] } — flagged FLAT+DATA, so the instrument sees the shape.

Result: 0 shipped apps in the flat + top-level-data shape. All five shipped stacks (app-showcase, app-crm, app-todo, and both app-multi-package package bodies) declare manifest: { ... }; the project-level composeStacks([...], { manifest: 'preserve' }) is additive and still selects a singular manifest. The three that carry seed data all nest their manifest, so none of them was ever double-collecting.

The upgrade condition is NOT met and the p2 grade stands. Re-grading is triage's call either way; this is a reading handed over, not a re-grade.

Verification

Resolution path, stated as asked: relative source, not dist/. The suite imports AppPlugin from ./app-plugin, and packages/runtime/vitest.config.ts aliases every @objectstack/* dependency it names to that package's src/. So a source-only mutation is measured directly and ablation-dist-preflight does not apply here — which the ablation then demonstrates rather than assumes, since an unrebuilt source mutation moved the result.

Ablation (guard removed, implementation already committed so HEAD carries it; restore under an EXIT INT TERM trap on an absolute path):

  • direction predicted before the run: the two new pins go RED, the anti-vacuity pin stays GREEN;
  • mutation proven on disk in both directions — removed text manifest.data !== this.collections.data count 1 to 0, injected marker count 0 to 1, blob 7dfa5287 to 9e2db7cf;
  • observed: Tests 2 failed | 12 passed with exactly the two predicted assertions, anti-vacuity pin green — prediction held;
  • restore proven: blob back to 7dfa5287 = the HEAD blob (non-empty, compared as such), git diff HEAD empty, injected marker absent, guard text back at 1.

Runs, all on the tree at 10a71f32f7e (this PR's head):

  • pnpm --filter @objectstack/runtime exec vitest run226 files / 3246 tests passed
  • pnpm --filter @objectstack/runtime typecheck — green on both legs; the second matters, because packages/runtime/tsconfig.json excludes **/*.test.ts, so tsc --noEmit alone would have said nothing about the edited test file. check:test-typecheck compiles the test layer under tsconfig.test.json: "@objectstack/runtime's test layer compiles under packages/runtime/tsconfig.test.json; 27 file(s) / 191 error(s) / 69 pinned signature(s) held"
  • downstream readers of the shared seed-datasets registry, enumerated by grep rather than assumed: @objectstack/cloud-connection marketplace seed-replayer (9 passed) and @objectstack/cli option-b-reader-acceptance.pin.test.ts (7 passed)
  • pnpm lint — the whole-repo run, eslint . --no-inline-config, exit 0. No narrowing claimed, so none needs justifying.
  • gate union: 54/54 families green, harvested with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands on a tree the tool does not call stale (origin/main was merged in first precisely so it would not), exit codes captured before any pipe. Two of them — check:dual-build-cjs-loads and check:type-check-debt — first returned PREREQUISITE NOT MET (exit 3), which reads NOT MEASURED rather than green; the workspace closure was built (turbo run build --filter=./packages/* --filter=./packages/*/*, 71/71) and both were re-run to a real verdict: "103 published require entry point(s) across 66 package(s) load" and "13 ledger entr(ies) re-measured in 133.5s, 143 raw tsc error(s) total, none above its recorded number".

Heavy runs were serialized through scripts/pm/os-verify-lock.sh; every verdict above is read from the gate's own printed line, never from a bare $? after a pipe.

Bump

patch. The rule is the Check Changeset step's WHICH LEVEL prose in .github/workflows/pr-automation.yml (cited from scripts/check-changeset-no-major.mjs): "A purely additive widening of a published package's public surface (a new exported symbol on an index, a new accepted key or value) takes at least minor … a fix( that changes no public surface stays patch." This adds no exported symbol, no accepted key and no accepted value — it stops one internal collector from reading the same array twice. Nothing widens, so the level stays where the commit type puts it. Not skip-changeset: the change does release from @objectstack/runtime and is user-visible, so it owes the changeset it carries.


Generated by Claude Code

RED: a bundle with no `manifest` key and a top-level `data` array has every
seed dataset collected twice, and a `mode: 'insert'` dataset applied twice
per boot. The third case is the anti-vacuity control -- a genuinely distinct
`manifest.data` must still be collected.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N
…ding the top level

On a flat bundle -- manifest fields written directly on the bundle, no
`manifest:` key -- `this.bundle.manifest || this.bundle` resolves to the bundle
itself, so the legacy `manifest.data` read re-read the very array the top-level
`data` read had just contributed and every seed dataset was collected twice.
`mergeSeedDatasets` does not de-duplicate, so both copies reached the shared
`seed-datasets` registry, the inline loader and every later per-org replay: a
`mode: 'insert'` dataset was applied twice per boot.

The guard is the sibling collector's, not a new spelling: `loadTranslations()`
has always carried `manifest.translations !== this.collections.translations` on
the identical two-location read.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N
… double-collect fix

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N
…ble-collect

Brings the gate-derivation scripts current so the family union is derived from
a tree the tool does not call stale.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N
@github-actions github-actions Bot added the size/m label Sep 4, 2026
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

Nothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 1 changed package(s)), so this run has no opinion about the docs.

What this run could not see
  • 1 name(s) were too generic to anchor anything (single lowercase words)
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 24 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json f1d787294f052a5ee79332b4c0459e24c9ff9f3fpackageMentionDocs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

2 participants