Write the bundled format blueprints in TypeScript - #466
Conversation
A blueprint's files/ may now be TypeScript: client.ts and server.ts are bundled with their lib/**/*.ts imports into the client.js / server.js the archive ships, readable rather than minified, so the installed gadget and the agent that edits it still see one JavaScript file per side. Three tsconfigs type-check the result under the globals each side really has: the client under the DOM lib, the server under the Workers types as part of this package's own program, and the blueprints' own tests under Node's. A lib/ module is checked under whichever side imports it. The build rejects what would otherwise ship wrong: an entry present as both .ts and .js, a .ts file outside the entry/lib layout, .tsx/.mts/.cts, a lib/ module no entry imports, an import that reaches outside files/, and an import the entry's runtime does not supply (esbuild leaves a URL import external without a word). The Docs, Sheets and Slides blueprints are rewritten this way, each with a lib/protocol.ts holding the types its two sides share, and the DOM helpers and collaboration plumbing the three have in common factored into typed modules under lib/ui/ and lib/sync/, one copy per blueprint. The Sheets formula engine moves to lib/formula.ts with unit tests beside it, and the latent mismatches the types exposed are fixed. The blueprints' Node-side tests run under vitest.blueprints.config.ts, since the workerd suite can neither spawn esbuild nor provide jsdom. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Preview:
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
|
Posted 3 actionable inline findings. |
The DOM helpers and collaboration plumbing the Docs, Sheets and Slides blueprints have in common move from three copies under each blueprint's lib/ to one source in packages/gadget-libraries, with the libraries' own tests. A blueprint imports them as `gadgets:<name>/client` and `gadgets:<name>/server`; the blueprint build resolves each to the library's entry and inlines what the entry uses into the shipped client.js / server.js, the way it inlines a lib/ module. The archive stays self-contained and nothing resolves the specifier at runtime, so the kernel is untouched; a gadget created from a blueprint carries its copy of the library as of its creation, as it does today. The build rejects a client importing a library's server side, a library that does not exist, a malformed specifier, and any input from node_modules, and the libraries directory is the one place an import may reach outside files/. The blueprint tsconfigs map the specifier to the same entry, and the workerd suite aliases it for the tests that import a blueprint's server directly. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Posted 4 actionable inline findings. |
…nto the checks Review of #466 found seven holes in the TypeScript blueprint build, all real, plus one README claim that was not. The build script: - A relative import written in a blueprint may not leave files/. The previous check ran on the metafile after esbuild had resolved everything, and allowed any input under packages/gadget-libraries -- so `../../../../gadget-libraries/ui/src/el.ts` from client.ts was accepted, bypassing the `gadgets:` specifier's side check and reaching into a library's private modules. A plugin now refuses the import at resolve time whenever the importer is inside files/ and the target is not, so the specifier is the libraries' only door. The metafile check stays as defense in depth (an input outside files/ must be under the libraries and never node_modules). - Containment treats an absolute `relative()` result (a different drive on Windows) as outside, in one `contains` helper both checks use. - A dynamic `import()` of a computed path is rejected. esbuild inlines a literal one like a static import, but leaves `import(p)` in the bundle as written, where it would resolve inside the sandbox against nothing the build checked. Comments cannot trip the scan: esbuild drops them. - A `lib/x.ts` beside a `lib/x.js` is rejected like an entry twin was: tsc would type the .ts while the bundle shipped the .js. - Roots are realpath'd before bundling: esbuild reports importers with symlinks resolved, so on macOS a blueprint under /var/folders was compared against a /private/var path and the plugin never fired. The type-check and test wiring: - `moduleDetection: force` in the three blueprint tsconfigs. Each entry is bundled alone, but an entry without an import or export was a global script to tsc, sharing declarations with another blueprint's. It is a no-op for the backend's own `src/`, which shares the server program and whose modules all import or export. - `tsconfig.blueprints-tests.json` gets the `gadgets:*` path mapping and the `blueprint-lib` vitest project the matching alias, so a blueprint test can import a module that reaches a library. - `packages/gadget-libraries` is a Worker input for the integration suite's watch mode: the build inlines it into the archives the Worker ships, so a library edit is a Worker change too. The gadget-libraries README said the doc comments in `src/` reach the agent through the inlined code. They do not: esbuild drops ordinary comments whatever the minify settings. The bundle is unminified, so a library's names and structure survive; its comments are for this repository's readers. The README also states the relative-path rule. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@Maximo-Guk Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
Posted 6 actionable inline findings. |
…emaining gaps Sheets kept its structure snapshot and whole-sheet replacements queued across a rejected applyOperation, for the scheduler's retry. A rejection is ambiguous -- Cap'n Web rejects every in-flight call when the socket drops, after the server may have committed -- and the server applies both payloads wholesale, so the retry could overwrite a collaborator's edit made in the gap. The next attempt after a failure now fetches the document first; if its revision is not the one the payloads were built against, they are dropped, the server's copy adopted, and only the versioned cell ops go out. The client still subscribes once at init and does not re-subscribe after a broken session; that is a follow-up. Formula: ROW and COLUMN look through the paren node the parser now keeps for the serializer, so ROW((A5)) is 5 again rather than #REF!. Subscribers: a newcomer dropped for failing a seed has its leave announced. It was a member while it seeded, so a subscriber added in that window was seeded with it and would otherwise show a phantom until its roster expired it. Blueprint build: a require() that survives bundling is rejected -- esbuild rewrites it to a __require shim that throws when reached, without a warning or, for a computed path, a metafile import. The specifier scan allows comments between a keyword and its operand, and a lib/ module the metafile shows inlined counts as imported, so the scan only has to witness type-only imports. The blueprint server tsconfig is a standalone program under @cloudflare/workers-types/experimental, as the libraries' is, so that a bare DurableObject or WorkerEntrypoint sees an empty Cloudflare.Env rather than the backend's bindings, which a gadget never receives. The package's own tsc runs beside it again. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
…w nits Sheets keys its pending cell ops by coordinate and nothing remapped them when a structural change (insert or delete rows or columns, sort) moved the sheet's cells and queued a whole-sheet replacement. The server applies cell ops after replacements, so an edit queued in the 180 ms before the change landed on whatever cell now held its coordinate. That was so before the resync. Queuing a replacement now drops the sheet's queued cell ops: the replacement carries the sheet's whole cell map, local edits included, so they were redundant as well as misplaced. The resync added by 2edb64f left a second copy of the problem: after a failed save it discarded the replacement but kept cell ops queued after it, keyed by the layout the replacement built, which the server may not have. If our commit had landed they were mostly redundant; if a peer's had, they would write into other cells. The reload now drops the cell ops of every sheet whose replacement it discards -- an edit typed after a structural change and before a failed save is lost, as every edit was on a failed save before the resync -- and clears the undo history, whose coordinates presume that layout too. Cell ops on sheets with no pending replacement stay queued as before. Blueprint build: the residual-require check matched only a call of the __require shim, so require.resolve(...), typeof require and a bare require passed along got through and would throw when reached. Any occurrence of the identifier is now the witness: esbuild emits the shim only when a reference to require survives, and renames a source identifier of that name away from it. Images: the library kept a GIF animated only within maxDataUrlLength (about 1 MB of file), where Docs kept one under 2 MB; a GIF between the two was flattened without notice. A GIF gets a budget of its own, maxGifDataUrlLength, defaulting to Docs' threshold. workshop-backend gains test:watch:blueprints for the Node-side blueprint tests, which test:watch could not include: one vitest process takes one config. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
All four addressed in fb2304e.
|
| name: "own-file-imports", | ||
| setup(pluginBuild) { | ||
| // A Go regular expression, so no `u` flag: esbuild compiles the filter itself. | ||
| pluginBuild.onResolve({ filter: /^\.\.?\// }, args => { |
There was a problem hiding this comment.
[P2] Reject absolute imports from blueprint sources
This hook intercepts only ./ and ../. Esbuild resolves an absolute filesystem import by default, and the metafile check later accepts any target under packages/gadget-libraries. A client.ts can therefore import the absolute path to sync/server.ts or any private src module, bypassing both the side check and the documented gadgets: "only door". Match absolute paths from blueprint importers here as well, or reject them before the containment allowlist.
There was a problem hiding this comment.
Fixed in e4e3301, structurally rather than with a third guard. Discussing this led to dropping the gadgets: specifier altogether: it only existed so the build had one door it could check, and every other resolver then needed its own mapping for it. Blueprints now import a library by the package's own name, @gadgets/bundled-blueprints/libraries/<name>/<side>, declared in exports the way @gadgets/scripts already shares its sources, so tsc, vitest and editors resolve it with no paths and no alias. The build gives esbuild the package name as an alias for the package root (so a tree with no node_modules above it, like the test fixtures or a BUNDLED_BLUEPRINTS_DIR, still builds) and replaces the two resolve plugins and the metafile loop with one audit that walks metafile.inputs[*].imports from the entry: an import written in a blueprint file that lands outside files/ must be the exported subpath, of the entry's side, resolved to exactly that library's <side>.ts. So a relative path, an absolute path, a bare specifier some node_modules satisfies, the package root and a src/ module all fail the same way, naming the importer and the specifier as written (original in the metafile). Tests cover absolute paths from client.ts and a lib/ module to both a library entry and a src/ module, a node_modules beside files/, and the extension-spelled and src/ package subpaths. Bundles are byte-identical; the three files/README.md changed text.
| invalid(label, `${entry.name}.ts contains a dynamic import whose path is not a string ` + | ||
| `literal; the bundler cannot check it`); | ||
| } | ||
| if (RESIDUAL_REQUIRE_PATTERN.test(text)) { |
There was a problem hiding this comment.
[P2] Parse residual syntax instead of matching string contents
Because text is unminified output, ordinary string literals survive. For example, document.body.textContent = "__require" matches this regex and rejects a valid blueprint despite containing no require reference; "import(foo)" similarly trips the computed-dynamic-import check at line 500. Parse or lex the output, or otherwise exclude literals rather than scanning raw text.
There was a problem hiding this comment.
Declined, for now. Real, but confined to a gadget whose output contains the literal text __require or import( followed by a non-quote, it fails the build with a message that names the check, and the fix is a JavaScript parser in the build path (typescript6 is in the workspace, but nothing under src/ depends on it today and the backend's build scripts run it directly under node). The comments on both patterns already say they are scans and which direction they err. Will revisit if a real blueprint hits it.
| this.#inFlight = false; | ||
| if (this.#saveAgain || (outcome !== "pending" && this.#options.isDirty())) { | ||
| this.#saveAgain = false; | ||
| this.schedule(this.#failures ? retryDelay(this.#failures) : RETRY_BASE_MS); |
There was a problem hiding this comment.
[P2] Keep the failure status visible during backoff
The catch sets Save failed — retrying, but this call immediately invokes schedule(), which synchronously replaces it with Saving… for the entire retry delay (eventually up to 10 seconds). Base Sheets left Save failed visible; after this refactor users see a false active-save state and no failure indication. Arm the retry timer without changing the offline status until the attempt actually starts.
There was a problem hiding this comment.
Fixed in d5f2214: the retry no longer goes through schedule(). A private #arm arms the timer and says nothing; the save announces Saving… when the timer fires, unless the line already says so from the keystroke that scheduled it, so typing, conflicts and pending outcomes report exactly the sequences they did and a failure reports offline until the retry starts. One correction to the premise: base Sheets left Save failed visible because it never retried at all -- it consumed pendingCellOps before sending and dropped them when the send failed -- so the library was already an improvement there; the message just came too early. Test asserts ["saving", "offline"] through the whole backoff and "saving", "saved" once the retry lands.
|
Posted 3 actionable inline findings. |
The bundled format blueprints came to main as data the backend embeds into src/generated/format-blueprints.ts. Once they were TypeScript with tests, the backend package carried three tsconfig programs, a second vitest config, an esbuild bundler under scripts/, and a sibling package (packages/gadget-libraries) that five places reached by relative path, all for code that never runs in its Worker; and `test:watch` could not cover the blueprint tests. packages/format-blueprints (@gadgets/format-blueprints, private) now holds all of it: - blueprints/: the three blueprints, content unchanged. - libraries/: what was packages/gadget-libraries, as a plain directory. The `gadgets:<name>/<side>` specifier stays. - src/: the archive codec, source reader and esbuild bundler (files.ts, from scripts/format-blueprint-files.ts), the manifest parser (manifest.ts), and the library half of the generator (generate.ts: generateFormatBlueprintsModule and BUNDLED_BLUEPRINTS_DIR). One vitest config with two projects (gadgets under jsdom, the build under node) and five tsc programs: client, server and tests as before, `server-tests` for a test of a server side (named server.test.ts or <topic>.server.test.ts), and `node` for src/. The server-side tests get their own program because they import a Durable Object: it needs the Workers types, and judging it under the DOM lib flags lib.dom's own Crypto and CompressionStream typings in blueprint source that is fine under workerd's. The two CLIs stay in packages/workshop-backend/scripts as thin entry points, since the generated module is the backend's and FORMAT_BLUEPRINTS_DIR keeps resolving against the backend package root. Unset, the source is the new package's blueprints/. The backend drops the blueprint tsconfigs, vitest.blueprints.config.ts, the third vitest run and `test:watch:blueprints`, the `gadgets:` alias in its vitest config, and the jsdom and @cloudflare/workers-types dev dependencies; it gains the workspace dependency. Trade-off: the sheets and slides server tests moved with their blueprints and now run under node against a stub of cloudflare:workers, so blueprint server code is no longer exercised inside workerd. The backend's format-blueprints.test.ts still installs and inspects the built archives in workerd. The integration-tests watch table now lists packages/format-blueprints always and the FORMAT_BLUEPRINTS_DIR tree only when the variable is set. PR #465's branch maximo/gadget-libraries stays frozen at 9794a1a, to be re-ported onto this layout. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
"Format" is a runtime curation state: a promoted blueprint, which an admin can take out of the formats menu (a bundled one) or put into it (an unbundled one). "Bundled" is what the package actually holds, so the package, the backend module, the generated module, the two CLIs, the vp task, the package.json script, the identifiers and the env var are now named for that. - packages/format-blueprints -> packages/bundled-blueprints (@gadgets/bundled-blueprints); FormatBlueprintManifest, FormatBlueprintPresentation and their parsers -> BundledBlueprint*; generateFormatBlueprintsModule -> generateBundledBlueprintsModule. - Backend: src/bundled-blueprints.ts (installBundledBlueprints, bundledBlueprintsManifestVersion), the generated src/generated/bundled-blueprints.ts (BUNDLED_BLUEPRINTS, BundledBlueprint), scripts/build-bundled-blueprints.ts, scripts/import-bundled-blueprint.ts, the `import:bundled-blueprint` script and the `build:bundled-blueprints` task. - FORMAT_BLUEPRINTS_DIR -> BUNDLED_BLUEPRINTS_DIR, renamed outright with no fallback: a deployment still setting the old name builds the bundled set. The README says so. Kept, deliberately: the blueprint IDs format.document, format.spreadsheet and format.slides, which are install and promotion keys on live deployments (the README carries a TODO: renaming them needs an install-time migration keyed on the old id); the AdminSettings storage keys installedFormatBlueprints and promotedFormatBlueprints, persisted state for the same reason; and everything genuinely about promotion: AdminConfig.formats, the Formats panel, FormatCuration, BlueprintMetadata.output, OUTPUT_ICONS, the shared API's `bundled` flag and the `formats.*` log events. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
| */ | ||
| const ENTRY_POINTS = [ | ||
| { name: "client", platform: "browser", external: [] }, | ||
| { name: "server", platform: "neutral", external: ["cloudflare:*"] }, |
There was a problem hiding this comment.
[P2] Allowlist production Cloudflare modules
This wildcard also accepts test-only and nonexistent specifiers such as cloudflare:test. That is hidden for this repository’s blueprints by tsconfig.server.json, but a BUNDLED_BLUEPRINTS_DIR tree is explicitly not type-checked: esbuild preserves the import as external, this check accepts it, and the generated blueprint then fails when production WorkerLoader instantiates its server. Restrict this to the cloudflare:* modules that gadget workers actually receive (at least cloudflare:workers) rather than treating the namespace as available.
There was a problem hiding this comment.
Fixed in d5f2214: the server's externals are ["cloudflare:workers"] and matchesExternal is an exact match, the wildcard branch gone. That is the one cloudflare: module loadGadgetWorker gives a gadget: globalOutbound: null, so cloudflare:sockets is moot, and none of the flags behind the others. Test imports cloudflare:test from server.ts and gets esbuild's own Could not resolve, since it is no longer external.
|
Posted 1 actionable inline finding. |
…tead of guarding resolution A blueprint imported a gadget library as `gadgets:<name>/<side>`, a specifier of the build's own. It existed because esbuild inlines whatever it resolves, and the build wanted one door into `libraries/` it could check: a resolve plugin mapped the specifier and refused the wrong side, a second plugin refused relative imports that left `files/`, and a metafile loop re-checked what was inlined. Every other resolver -- tsc, vitest, an editor -- then needed its own mapping (`paths` in four tsconfigs, an alias in vitest), and an absolute path into `libraries/` walked past all three guards, as review noticed. The specifier is gone. A blueprint imports a library by the package's own name, `@gadgets/bundled-blueprints/libraries/<name>/<side>`, which package.json now exports the way `@gadgets/scripts` already exports its TypeScript sources by name. In-repo blueprints refer to their own package, which tsc, Vite and esbuild resolve through `exports` with nothing to configure, so the `paths` blocks and the vitest alias go; a `BUNDLED_BLUEPRINTS_DIR` tree elsewhere gets types by linking the package into its workspace, as it must already do for `@gadgets/scripts`. `src/` and `blueprints/` stay unreachable by name because `exports` does not list them. The build still needs no `node_modules` above a blueprint -- the tests' temporary fixtures and an external tree have none -- so it gives esbuild the package name as an alias for the package root, read from package.json so it cannot drift from `exports`, and resolves the import to the libraries it ships with. The three guards collapse into one audit of the bundle graph: starting at the entry and following `metafile.inputs[*].imports`, every input is met as the edge that brought it in, and an import written in a blueprint file that lands outside `files/` has to be the exported subpath, of the entry's own side, resolved to exactly that library's `<side>.ts`. A relative or absolute path into `libraries/`, a `src/` module, the package root and a bare specifier some `node_modules` happens to satisfy all fail the same way, naming the importer and the specifier as written; a library's own imports may reach anything under `libraries/` but never `node_modules`. The bundles are byte-identical: the libraries inline the same files. The three blueprints' `files/README.md` name the new import, and a README is shipped in the archive, so the archives change by that text alone.
…'s status up while it waits The server bundle's externals were `cloudflare:*`, so any module under that prefix passed the build. A gadget's Durable Object is loaded by `loadGadgetWorker` with no outbound network and none of the flags behind the other `cloudflare:` modules, so `cloudflare:workers` is the only one it can use; the in-repo blueprints are type-checked against that, but a `BUNDLED_BLUEPRINTS_DIR` tree is not, and its `cloudflare:test` would build and fail when the object was instantiated. The externals now name `cloudflare:workers` alone, `matchesExternal` is an exact match with the wildcard branch gone, and a test imports `cloudflare:test` from a server. After a failed save the scheduler said `Save failed — retrying` and then, in the same tick, `Saving…`, because the retry went through `schedule()`, which announces the save when it arms the timer. Nothing was in flight for the whole backoff, up to ten seconds of it. Base Sheets never showed this: it did not retry at all -- it consumed its pending cell ops before sending and dropped them when the send failed -- so the failure simply stayed up. The timer is now armed by `#arm`, which says nothing itself; the save announces itself when the timer fires, unless the line already says `Saving…` from the keystroke that scheduled it. Typing, conflicts and pending outcomes report exactly what they did; a failure reports `offline` until the retry starts.
|
…mporting a dropped lib/*.ts A dynamic `import()` of a template literal with substitutions is expanded by esbuild into a glob helper: the metafile records an external edge whose path is the wildcard, every file the pattern matched -- anywhere it reaches, including outside files/ -- is inlined as an input no edge points at, and the output holds no `import(`. None of the three existing checks saw it. `auditInputs` now rejects the wildcard edge and then requires that the walk met every input the metafile lists, so a bundle that inlines something no import brought in is refused whatever produced it. In a mixed tree, a module the archive ships as written (an un-migrated entry, or a lib/*.js) could import a lib/*.ts, which is compiled into the entries and not shipped; the gadget then failed to load with no build error. Such an import is rejected before the bundle runs, naming the importer and the specifier. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
| `.mts or .cts`); | ||
| } | ||
| if (!path.endsWith(".ts")) { | ||
| output.set(path, source); |
There was a problem hiding this comment.
[P2] Audit package-library imports in JavaScript entries
Every non-TypeScript file is copied here before the only esbuild/audit pass, and a JavaScript-only tree returns at line 400. Thus a supported server.js containing import { MutationQueue } from "@gadgets/bundled-blueprints/libraries/sync/server" builds with that bare import unchanged, but WorkerLoader provides only archive modules and cloudflare:workers, so the instantiated gadget fails to load. Bundle JavaScript entries as well, or reject library imports from them instead of documenting libraries as available to all blueprints.
| source: string, | ||
| dropped: ReadonlySet<string>, | ||
| ): {specifier: string; module: string} | undefined { | ||
| for (const [, doubleQuoted, singleQuoted] of source.matchAll(SPECIFIER_PATTERN)) { |
There was a problem hiding this comment.
[P2] Decode escapes before resolving passthrough imports
SPECIFIER_PATTERN returns the source spelling, not the JavaScript string value. In a mixed tree, a retained server.js import of "./lib/sh\u0061red.js" is valid and resolves to ./lib/shared.js at runtime, but this loop checks a path containing the literal escape, misses lib/shared.ts, and lets the build drop that module after bundling the client. The generated server then has an unresolved import. Parse/cook the specifier before passing it to resolveWithinFiles.
|
Posted 2 actionable inline findings. |
… in a shipped module A JavaScript module is copied into the archive as written, so a gadget library it imports by package name is never inlined and the runtime has nothing to resolve the name against. The shipped-module scan now runs for a JavaScript-only tree too, ahead of the early return, and rejects a package-name import from any module that ships as written. The scan reads a specifier as spelled, so one written with an escape could name a dropped lib module, or the package, past both checks. A specifier containing a backslash is rejected outright rather than decoded: nothing legitimate spells an archive path that way. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
|
||
| console.log(""); | ||
| await import("./build-format-blueprints.ts"); | ||
| await import("./build-bundled-blueprints.ts"); |
There was a problem hiding this comment.
[P2] Keep the previous source until the generated build succeeds
This build is now the first point that applies the TypeScript/bundler policy to the imported files, but the extracted backup was deleted at line 249. If an archive contains a layout the new bundler rejects (for example client.ts beside client.js), the command fails here after permanently replacing the prior valid source; --new similarly leaves the invalid tree behind. Validate the staged tree before the rename, or retain and restore the backup through generation.
|
|
||
| await visit(filesDir, ""); | ||
| validateFilePaths(files.keys(), label); | ||
| return await bundleTypeScriptSources(filesDir, files, label); |
There was a problem hiding this comment.
[P2] Let the importer read broken source it is replacing
import-bundled-blueprint.ts calls readSourceFiles() on the current extracted tree only to compute its change summary. Since this helper now bundles, any syntax error, unresolved import, or new policy violation in the current TypeScript aborts the import before a valid archive can replace and repair it. Keep a raw-reader path for that caller rather than compiling the outgoing source.
|
|
||
| /** A `baseVersion` as sent over RPC, read as the non-negative integer it must be; anything else is 0. */ | ||
| export function normalizeBaseVersion(value: unknown): number { | ||
| return Math.max(0, Math.floor(Number(value) || 0)); |
There was a problem hiding this comment.
[P2] Do not coerce invalid versions into valid preconditions
Flooring changes optimistic-concurrency semantics: against stored version 2, baseVersion: 2.9 becomes 2 and overwrites the item, while a negative version for a missing item becomes 0 and creates it. The previous Docs implementation preserved these numeric values, so both cases failed their version checks. Reject/preserve non-integer and negative values instead of mapping them onto meaningful versions.
| async subscribe(callback: SubscriberCallbacks, client: Partial<Collaborator> = {}): Promise<DocumentSnapshot> { | ||
| // The registry keeps the stub, seeds the newcomer with everyone already | ||
| // connected, announces it to them, and drops it when its connection breaks. | ||
| this.subscribers.add(callback, normalizeCollaborator(client)); |
There was a problem hiding this comment.
[P2] Normalize the same identity on cursor updates
The subscribed collaborator is normalized here, but updatePresence() and leavePresence() broadcast the raw ID. Reusing an overlong ID therefore emits the join under its truncated key and cursor updates under a second key; disconnect removes only the truncated entry, leaving a ghost cursor until expiry. Name/color normalization is also undone by the first cursor update. Apply the same collaborator normalization to all presence events.
|
Posted 4 actionable inline findings. |
A tree holding any .ts (other than a .d.ts) may hold no module the archive would ship as written: a .js/.mjs/.cjs beside bundles it cannot share code with is rejected, naming the file. JavaScript-only trees still pass through unchanged, so the importer and external BUNDLED_BLUEPRINTS_DIR trees keep working; non-module files (JSON, CSS, README, assets) are unaffected either way. This one rule supersedes three special cases from 5f11a2a and 9b7229e: the entry twin check, the lib/ twin check, and the shipped-.js-imports-a-dropped-lib/*.ts check, which can no longer arise since a shipped module only ever sits in a JavaScript tree, where everything it can name ships beside it. checkShippedImports keeps the escape and library-import checks, which a JavaScript-only tree still needs, and now runs only for such a tree. The incremental-migration path the mixed-tree support served is no longer needed: the three in-repo blueprints are all TypeScript. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… anything readSourceFiles now applies the bundler policy, so an export can be rejected at build time on things extractFiles accepts (a .js beside a .ts, a .tsx, a library import from a shipped module). The importer built only after the rename, by which point the prior source was replaced and the backup deleted; --new left the rejected tree in place. The staged files/ is now run through readSourceFiles before the rename, so a rejected export is refused with the target untouched, and the post-rename build cannot fail on the files. The outgoing tree is also read through readSourceFiles, for the change summary only. If the current TypeScript does not build, the import now warns and reports the summary as unavailable instead of aborting: the archive being imported may be what repairs it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ounds cursor and leave like the join normalizeBaseVersion floored and clamped, so against stored version 2 a baseVersion of 2.9 was accepted and -1 for a missing item re-created it. A precondition that cannot be read should fail: a value that is not a non-negative integer now reads as -1, which matches no stored version and is not 0, so the upsert or deletion is rejected as stale or missing, as main's `Number(v || 0)` did. Absent stays 0. The sender is the gadget's own client, which can already send the honest value, so this is semantics rather than authority. Docs normalized the collaborator on subscribe but kept ad-hoc String() bounds on cursor and leave, so an overlong id joined under its truncated key, sent cursors under the full one, and the registry's disconnect leave removed only the truncated entry, leaving a ghost until STALE_MS. Cursor and leave now go through the same normalizeCollaborator as the join. Sheets keeps its own self-consistent bounds as on main. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
| let metafile: Metafile; | ||
| let text: string; | ||
| try { | ||
| const result = await build({ |
There was a problem hiding this comment.
[P2] Reject glob imports before invoking esbuild
The audit runs only after build() has fully resolved and bundled every input. A tiny imported archive can therefore use e.g. import(../../../../../../${name}.js) to make esbuild recursively enumerate and bundle JavaScript outside files/; the importer calls this on its staged, attacker-supplied TypeScript before the wildcard edge is rejected at line 532. The 32 MiB source limit does not bound that external tree, so this can exhaust CPU/memory or read a large part of the host filesystem before validation fails. Reject template/glob imports in a pre-build parse or an esbuild resolver hook, before expansion occurs.
| invalid(label, `${lib} is not imported by any entry point`); | ||
| } | ||
| } | ||
| return new Map([...output].toSorted(([a], [b]) => compareNames(a, b))); |
There was a problem hiding this comment.
[P2] Validate paths introduced by TypeScript output
Only the on-disk input paths are validated. For example, client.ts plus client.js/assets.txt is a valid source tree, but compilation adds a client.js file and this returns a map where that file conflicts with the existing client.js/ directory. buildContent() eventually rejects it, but the importer's staged check calls only readSourceFiles(), so it accepts the stage, replaces the old tree and deletes its backup before the final generator fails. Re-run validateFilePaths(output.keys(), label) after adding generated entries (and validate the complete staged archive before swapping).
|
…d runs; validate the paths the bundle adds
esbuild expands `import(`../../${name}.js`)`, or a concatenation that begins with a string, into a
glob over every file the pattern matches and bundles each of them before auditInputs can reject the
wildcard edge, so the walk was unbounded work on the machine running the build. The blueprint's
sources are now scanned for a dynamic import() or require() of anything but a plain string literal
and refused ahead of the build; the post-build checks stay as the backstop for library code.
The bundle also adds client.js / server.js to a tree validateFilePaths saw without them, so a
client.ts beside a client.js/ directory of non-modules survived readSourceFiles and failed only in
buildContent -- after the importer's staged check had accepted it and replaced the target. The
returned map's paths are validated before it is returned, so every caller gets the check.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…is queued, not when it is sent The send re-read the model's version for each pending op, on the claim that this made a stale op fail. It does not once the model has moved: the resync that follows a failed save installs the server's cells and versions, and a peer's upsert during the debounce does the same, so a surviving local op re-read the collaborator's version as its base and the server accepted the stale value as current. The same rule also sent every plain deletion with baseVersion 0, since a deletion removes the cell from the model before the op is queued, and the server rejected the delete as a conflict and the cell came back. queueCellOp now takes the version the caller saw before it wrote the model, and the send uses it. An edit typed while a save was in flight replaces its entry and stays pending, so the ack loop rebases it onto the version it just adopted: it was made on top of what the save carried, and only our write could have produced that version. Conflicts rebase nothing; the server's cell stands. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Findings
|
…; ignore tree-shaking annotations
The pre-build scan ended a `//` comment at LF only, so `import //x<CR>(`...${n}`)` read as one comment up to the next LF and passed, and esbuild enumerated the pattern before the wildcard edge could reject it. A line comment now ends at CR, U+2028 and U+2029 as well, as it does for esbuild; the terminator stays required so the alternative cannot stop partway through a comment and read a string inside it as the operand. The rejection quotes only the keyword, not whatever comment sat in the gap.
esbuild applies a `"sideEffects": false` in an enclosing package.json to the package's own relative imports, dropping `import "./lib/setup.ts"` with a warning the silent log level swallows, and the audit cannot see it: the source names the module, and a dropped input is absent from the metafile. The build now sets `ignoreAnnotations`; blueprints and libraries write no `@__PURE__` annotations, so nothing else changes.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…he revision it was built on; renders after a reload or conflict The resync compared the server's revision against `model.revision`, which a collaborator's broadcast advances while the structure snapshot or sheet replacement sits in the queue: a replacement queued at R0 whose send failed, followed by a peer's edit landing at R1, passed the check at R1 and was sent wholesale, erasing the peer's cell. The payload now records the revision the model held when its first item was queued and is compared against that; if they differ it is dropped and the server's copy adopted, as before. With nothing wholesale pending there is no base and no reload, since cell ops carry their own versions. A wholesale edit queued while a save is in flight is built at the pre-ack revision, so a failure of the next save drops it: the loss every failure had before the resync, confined to that timing. After a reload the grid showed the server's copy, and the acknowledged retry wrote the model without rendering; a conflict likewise adopted the server's cell into the model while the grid kept the optimistic value. The ack loop now renders in either case. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
| // Rejecting here leaves the flag set; the scheduler counts a failure and tries again. | ||
| const doc = await gadget.getDocument(); | ||
| resyncBeforeSave = false; | ||
| if (doc.revision !== wholesaleBaseRevision) { |
There was a problem hiding this comment.
[P1] Make the replay precondition atomic with the write
getDocument() releases the server mutation queue before this comparison, so the revision can change before the following applyOperation(). If the failed request did not commit, this can read base revision R, then a collaborator commits a cell edit at R+1, and the retry applies its stale whole-sheet replacement unconditionally at R+2, erasing that edit. Carry and validate a base revision in the same queued server operation (or deduplicate the original operation) instead of relying on this client-side check.
| // save is what every failure did before the resync; writing the wrong cell is not. The | ||
| // undo history presumes that layout too. Cell ops on sheets with no replacement stay queued. | ||
| for (const sheetId of pendingReplacements.keys()) dropCellOpsFor(sheetId); | ||
| pendingReplacements.clear(); |
There was a problem hiding this comment.
[P1] Preserve wholesale edits queued after the failed payload
This clears the live replacement/structure queues, not just the generation whose result was ambiguous. If save A commits but its reply is lost and the user queues structural edit B while A or this getDocument() is in flight, the revision mismatch caused by A clears B even though B was never sent; applySnapshot() then removes it locally too. Track the sent generation and discard only its entries, preserving replacements and structure queued afterward.
| tsconfigRaw: {}, | ||
| }); | ||
| metafile = result.metafile; | ||
| text = result.outputFiles[0]!.text; |
There was a problem hiding this comment.
[P2] Reject or handle additional esbuild outputs
A client.ts side-effect import of a .css or .module.css file makes esbuild emit both JavaScript and a sibling CSS output. This keeps only outputFiles[0], while the Workshop loads only the generated client.js; esbuild does not inject the discarded stylesheet, so the blueprint builds successfully but loses its styles at runtime. Require exactly one JavaScript output, or explicitly preserve and load every generated output.
|
Posted 3 actionable inline findings. |
…ning the text
The build decided three things by running regexes over module text: which
files a module imports (the "lib/ module no entry imports" check), whether a
shipped JavaScript module imports a gadget library, and whether a source holds
a dynamic import() or require() of anything but a string literal. All three
shared a hand-written comment grammar (GAP), and the last two review rounds
were both bypasses of it: a CR-terminated line comment, and before that a
comment between keyword and operand. A regex over JavaScript always has
another one.
The new src/scan.ts reads the imports from the syntax tree instead, with the
TypeScript 6 compiler API (the `typescript6` alias the repo already uses for
the configurator build; tsgo ships no JS API). GAP, SPECIFIER_PATTERN,
NON_LITERAL_DYNAMIC_IMPORT_PATTERN and COMPUTED_DYNAMIC_IMPORT_PATTERN are
gone; RESIDUAL_REQUIRE_PATTERN stays, since a single identifier in
comment-free output is not a grammar problem. Each TypeScript module is parsed
once and the result shared between the pre-build dynamic-import check and the
reachability walk; the output backstop parses the bundle as JavaScript.
Two behaviour changes, both toward exactness: a lib/ module named only in a
comment is no longer counted as imported, and an escaped specifier is compared
decoded, so the separate "spelled with an escape" rejection is dropped -- the
library check catches it under the name it spells. A type-position
`import("...")` is now seen as well as `import type`. Declaration files are
still read for what they import, as the text scan incidentally did.
The three shipped blueprints bundle byte-identically before and after (the
scan gates, it does not transform). Loading the compiler adds ~140ms to the
build script's import. `typescript6` is added as a devDependency of the
package beside esbuild.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Findings
|
1 participant
A blueprint's
files/may be TypeScript:client.tsandserver.tsare bundled with theirlib/**/*.tsimports into the shippedclient.js/server.js, and type-checked under each side's own globals. The Docs, Sheets and Slides blueprints are rewritten this way, and the code they share lives once as gadget libraries, imported asgadgets:<name>/clientandgadgets:<name>/serverand inlined by the build, so nothing changes at runtime.The blueprints, the libraries and the build now live in their own package,
packages/bundled-blueprints(@gadgets/bundled-blueprints):blueprints/,libraries/,src/. The Workshop backend keeps two thin CLIs (scripts/build-bundled-blueprints.ts,scripts/import-bundled-blueprint.ts) and installs the generated module. The mechanism is renamed from "format" to "bundled" throughout,FORMAT_BLUEPRINTS_DIRincluded (nowBUNDLED_BLUEPRINTS_DIR, no fallback); the blueprint IDsformat.*are unchanged. Making thegadgets:imports resolve at load time (PR #465) is re-ported onto this layout separately.🤖 Generated with Claude Code