fix(runtime): guard the legacy manifest.data seed read so a flat bundle collects each dataset once - #15605
Merged
os-litant merged 4 commits intoSep 5, 2026
Conversation
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
Contributor
📓 Docs Drift CheckNothing 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
Coarse fallback — 24 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): |
This was referenced Sep 4, 2026
os-litant
marked this pull request as ready for review
September 5, 2026 09:06
os-litant
enabled auto-merge
September 5, 2026 09:06
os-litant
deleted the
claude/issue-15262-app-plugin-seed-dataset-double-collect
branch
September 5, 2026 10:04
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 #15262
AppPlugin.start()collects seed data from two locations — the top-leveldatafield, then the legacymanifest.data. The legacy read resolves its base asthis.bundle.manifest || this.bundle, so on a flat bundle (manifest fields written directly on the bundle, nomanifest:key — a shapeAppPluginsupports by design, see the constructor'sbundle?.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
translationshas 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: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 onemode: 'insert'dataset with one record:readSeedDatasets(ctx)ql.insertcalls for that one record (rows)Verbatim from the red run:
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 plainlist.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.datais a genuinely different array from its top-leveldatamust 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.tsthat 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 —
grep -nE "\.push\(\.\.\."(4 hits) plus the record-merge and object-spread accumulators found by reading everythis.collections.site;grep -oE "\b(sys|manifest)\.[A-Za-z0-9_]+" | sort | uniq -c(16 reads over 14 distinct keys), plus everymanifest ||/?.manifest ||base (8 sites).Positive control: the histogram independently surfaced
manifest.dataandmanifest.translations— the two known members — before any of them was looked for, andseedDatasetsscores 6 hits in the same file.Result — six members, and the repaired site is the only unguarded one:
data, instart()this.bundle.manifest || this.bundlepush(...)translations, inloadTranslations()this.bundle.manifest || this.bundlepush(...)collectBundleHooksstack?.manifest?.hookspushper itemseenreference Set, and no|| bundlefallback, so a flat bundle never re-readscollectBundleActionsstack?.manifest?.actions,stack?.manifest?.objects[*].actionspushper itemseenSet, same absent fallbackcollectBundleFunctionEntriesstack?.manifest?.functionsout[name] = fnstart()this.collections.manifestRuled out of the class as either/or reads rather than accumulations:
jobs(a ternary,this.collections.jobsor(this.bundle.manifest || {}).jobs, never both),i18nConfig(||), and the scalarsys.id/sys.nameenvelope 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
p2explicitly 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-leveldata, re-gradep1.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 flaggingmanifestabsent +datapresent. Population: 128 candidate files (every non-test source underpackages/,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: [...] }— flaggedFLAT+DATA, so the instrument sees the shape.Result: 0 shipped apps in the flat + top-level-
datashape. All five shipped stacks (app-showcase,app-crm,app-todo, and bothapp-multi-packagepackage bodies) declaremanifest: { ... }; the project-levelcomposeStacks([...], { manifest: 'preserve' })is additive and still selects a singularmanifest. The three that carry seeddataall nest their manifest, so none of them was ever double-collecting.⇒ The upgrade condition is NOT met and the
p2grade 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 importsAppPluginfrom./app-plugin, andpackages/runtime/vitest.config.tsaliases every@objectstack/*dependency it names to that package'ssrc/. So a source-only mutation is measured directly andablation-dist-preflightdoes 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
HEADcarries it; restore under anEXIT INT TERMtrap on an absolute path):manifest.data !== this.collections.datacount 1 to 0, injected marker count 0 to 1, blob7dfa5287to9e2db7cf;Tests 2 failed | 12 passedwith exactly the two predicted assertions, anti-vacuity pin green — prediction held;7dfa5287= theHEADblob (non-empty, compared as such),git diff HEADempty, 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 run— 226 files / 3246 tests passedpnpm --filter @objectstack/runtime typecheck— green on both legs; the second matters, becausepackages/runtime/tsconfig.jsonexcludes**/*.test.ts, sotsc --noEmitalone would have said nothing about the edited test file.check:test-typecheckcompiles the test layer undertsconfig.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"seed-datasetsregistry, enumerated by grep rather than assumed:@objectstack/cloud-connectionmarketplace seed-replayer (9 passed) and@objectstack/clioption-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.node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commandson a tree the tool does not call stale (origin/mainwas merged in first precisely so it would not), exit codes captured before any pipe. Two of them —check:dual-build-cjs-loadsandcheck:type-check-debt— first returnedPREREQUISITE 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 theCheck Changesetstep's WHICH LEVEL prose in.github/workflows/pr-automation.yml(cited fromscripts/check-changeset-no-major.mjs): "A purely additive widening of a published package's public surface (a new exported symbol on anindex, a new accepted key or value) takes at leastminor… afix(that changes no public surface stayspatch." 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. Notskip-changeset: the change does release from@objectstack/runtimeand is user-visible, so it owes the changeset it carries.Generated by Claude Code