fix(slides): normalize themeVersion in setDeck - #23
Open
darjss wants to merge 18 commits into
Open
Conversation
…#344) ensureObserver's pre-call snapshot of already-registered gatekeepers was mutated when a pre-configured binding failed a verification pass. If the user then repaired that binding on the re-prompt, its re-registration was misclassified as newly added, and a terminal failure of any other binding rolled it back -- silently deleting a registration the persisted observer record still asserts exists, so the gatekeeper could no longer name the observer in excludeObservers. Stop mutating the snapshot (rename it registeredBeforeCall to state the invariant). A binding repaired under an unpersisted account choice now survives the rollback; the next open re-verifies the stale persisted choice, fails, and re-prompts, so it self-corrects. Regression test drives the exact sequence -- repair one binding on the re-prompt while another fails terminally -- against per-resource verify outcomes and an observer add/remove event log added to the test gatekeeper fixture.
Last week's changes to transition to git storage had some obscure issues. When an agent message contained a write/edit tool call, the resulting edits would be persisted to `chatChanges` (which contains the OT change log) at the time the tool executed, but the overall agent message was not persisted to the actual chat history until the end of the step. A poorly-timed crash could leave the edit in `chatChanges` while the agent message was never persisted. When the DO started back up, the agent would run again, and very likely try to make the same changes again, resulting in the changes either being double-applied or confusingly failing. We now instead buffer changes to an in-memory buffer, which gets persisted as part of the same transaction that persists the chat log message. Either everything gets persisted together, or nothing does. Moreover, this means that the `chatChanges` contains ONLY user-authored edits, which is closer to how things worked back in the Yjs days. Agent-authored changes go into the in-memory buffer, and then are persisted directly to a "changes" message in the chat log. These changes fell out of my upcoming work to enable agents to create git worktrees. In that work, the agent will gain the ability to edit files programmatically within executeCode. Such edits will also need to land in the in-memory buffer so that they can be persisted transactionally. `plans/step-transactionality.md` contains a more complete description of this change.
`SUGGESTED_MODELS` lists `@cf/deepseek-ai/deepseek-v4-pro-0813`, but pi
0.83.0's Workers AI catalog didn't have it, so `catalogModel()` returned
undefined and `gatewayNativeModel()`/`getModelDirect()` synthesized the
descriptor from defaults. Unknown models are meant to degrade gracefully,
and mostly do -- `SUGGESTED_MODELS` is authoritative for the token window
-- but two of the defaults were wrong for a DeepSeek reasoner:
* `reasoning` fell back to false, and every `thinkingFormat` branch in
pi's openai-completions request builder is gated on it, so no thinking
config was sent and `thinkingLevelMap` was absent. (Thinking still
streamed back: pi parses `reasoning_content` unconditionally.)
* `compat.requiresReasoningContentOnAssistantMessages` and
`thinkingFormat` were left to pi's auto-detection, which keys on
`provider === "deepseek" || baseUrl.includes("deepseek.com")`. A
Workers-AI-hosted DeepSeek is neither, so it detected `false` and
`"openai"` where 0.84.3's catalog entry declares `true` and
`"deepseek"` -- the flag that makes pi echo `reasoning_content` back on
assistant messages across tool steps.
`cost` also fell back to zero, which the AI Gateway path already covers
(`#getCostFromAiGateway` reconciles from the gateway log and only falls
back to the estimate), but which a BYOK-direct Workers AI config would
have reported as $0.
pi-agent-core moves with it: it depends on `@earendil-works/pi-ai:
^0.83.0`, which 0.84.3 does not satisfy, so bumping pi-ai alone would
have pulled a second nested copy. 0.84.3 also drops the bundled
`@mistralai/mistralai` dependency, hence the net shrink in the lockfile.
The catalog additionally gained `@cf/deepseek-ai/deepseek-v4-flash-0731`,
which this commit deliberately does not add to `SUGGESTED_MODELS`.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Add Google Drive search API types * Add Google Drive metadata search * Update release manifest for Google Drive * Address review of Google Drive metadata search A code review of the Drive expansion found one blocker class and several correctness gaps. This is the remediation pass. Nothing here changes what the feature does; it changes what it is allowed to reach and what it admits, and it removes a duplicated admission path. Grants are now recorded rather than inferred. `grantedResourcesFromScopes` reported a resource as granted when every one of its OAuth scopes was held, so any account that had ever connected a Doc or a Sheet reported the whole-account Drive pattern as already granted: `ensureResources` would then skip consent entirely and `hasDriveResourceGrant` would pass. The account now persists the resource set the user actually consented to, and the scope-derived list is frozen to the resources that predate recording. Drive's batch `files.get` no longer reads a stale token as a denial. The batch POST returns 200 when a subrequest 401s, so `fetchWithAuthRetry`'s one-shot refresh never saw it and every file came back inaccessible, permanently. The batch now forces the same cache invalidation the helper uses and replays once. Parts are placed by their echoed Content-ID rather than by arrival order, since these booleans gate observer admission and a swapped pair admits the wrong collaborator. `corpora`/`driveId` became one `DriveCorpus` value: a `driveId` without `corpora: "drive"` silently falls back to the user corpus, and a shared-drive binding's whole boundary is those two travelling together. Observer verification is capped again. `maxTrackedSets: null` was justified as safe for bulk verifiers because their per-open RPC count stays bounded, but the work behind that RPC is not: a bulk Drive check issues ceil(N/100) sequential subrequests, per observer, on every open. The staged-observer rollback also never fired, because it compared a deserialized KV value against the in-memory stub by reference; it now turns on a nonce. Drive observer admission is one path for all three scopes. A file binding forked in four places and hand-rolled its own verify, which duplicated the tracker's semantics and did not get the rollback fix above. A shared-drive or single-file binding is now seeded with the set its scope already names, so the tracker handles every scope and `addObserver`/`removeObserver` are one-liners. The factory takes `verifyBatch` rather than a verifier type, which keeps `drive-session.ts` independent of the worker entrypoint and makes the seeding invariant testable for the first time. `getScope()` refuses a provider id that disagrees with the binding, rather than labelling the binding with another drive's or file's name. `search()` refuses a single-file binding outright: Drive `q` has no `id =` clause, so it would have scanned the whole account and post-filtered. Documentation is matched to the code on the surfaces that describe authority. Account scope is not limited to My Drive, and reads by ID are not scope-checked, so the README and `DriveScope` say so. The model-facing type now records that `fullTextContains` reaches body text, description and OCR - the README already said it, but the agent never reads the README. A parity test pins all six `*-types.txt` files to their `.d.ts`, following the `mcp-shared/base-types` precedent; only `drive-types` had been checked, and by nothing. Verified: `pnpm build`, `pnpm lint:check` (0 errors), `pnpm test` across the workspace, and the release manifest golden test. * Fix Google Drive review remediation * Clarify Google Drive name prefix search * Fix Google Drive security boundaries * Document Google OAuth nonce comparison * Fix Google Drive search edge cases * fix(google): preserve concurrent OAuth grants
* Bound test tasks so a wedged run fails instead of hanging. Every command in the shared `test` task now runs under `scripts/with-timeout.ts`, which kills the whole process tree and exits 124 after 60s with no output or 600s total, printing which threshold fired and what was still alive. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Document the test-task watchdog in AGENTS.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Doc-comment the newly exported collectTree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Pass test-child values as argv instead of building code strings. CodeQL's js/bad-code-sanitization: JSON.stringify does not escape U+2028/U+2029, so it is not a valid escape for embedding a value in JavaScript source. The `node -e` bodies are now fixed source that reads what varies from process.argv. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Run the with-timeout cases concurrently. Every case is a timer running out rather than work being done, so in sequence the file cost the sum of its thresholds. 4.7s -> 1.6s. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Run the kill-process-tree cases concurrently too. Same shape as with-timeout: the time is spent waiting on signalled processes to go away. 0.96s -> 0.70s. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Remove with-timeout.test.ts due to flakiness on repeated runs, and test slowdowns * Drive-by: Fix kill-process-tree.test.ts when run on terminal. When run from a color-capable terminal, Node would colorize console output. But the test tried to read back the console output in order to parse the pid. It ended up reading something like `\x1b[33m2167339\x1b[39m`, from which the regex would select `33` as the PID. PID 33 just happened to be some long-lived PID on my system, so the test would consistently conclude that the script had failed to kill the PID it was supposed to kill. (cherry picked from commit 09cb7431a55c9d5f39ace87d9a979fe37219a4ca) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Kenton Varda <kenton@cloudflare.com>
Also bumps OpenCode from 1.18.18 to 1.18.24. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Preserve checkbox-list offsets across selection rerenders while allowing filter changes to reset the list. Add generated-runtime DOM coverage for both behaviors.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR prevents slides written without a theme version from being rejected and reset on the next read by making File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
I have read the CLA Document and I hereby sign the CLA 0 out of 7 committers have signed the CLA. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="packages/workshop-backend/__tests__/format-blueprints.test.ts" line_range="99-104" />
<code_context>
expect(client).toContain("app.replaceChildren(canvas)");
});
+ it("normalizes themeVersion in format.slides setDeck", async () => {
+ let entry = FORMAT_BLUEPRINTS.find(blueprint => blueprint.blueprintId === "format.slides")!;
+ let serverCode = await readBlueprintFile(entry, "server.js");
+ expect(serverCode).toContain('themeVersion: "workspace.1"');
+ expect(serverCode).toContain("async setDeck(deck)");
+ });
+
it("declares the intended export formats for every standard output format", async () => {
</code_context>
<issue_to_address>
**issue (testing):** The regression test only searches the bundled `server.js` text for `themeVersion: "workspace.1"` and `async setDeck(deck)` independently, so it passes when the constant appears in an unrelated method or when `setDeck` does not actually write it. An implementation that still returns an invalid deck can therefore pass the test.
**Triggers:** When the bundled server contains those strings outside the intended `setDeck` implementation, or `setDeck` contains the method signature but does not execute the normalization.
**Suggested fix:** Extract or execute the `setDeck` implementation and assert that calling it with `{slides}` persists a deck whose `themeVersion` is exactly `"workspace.1"`.
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: packages/workshop-backend/__tests__/format-blueprints.test.ts:104
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+99
to
+104
| it("normalizes themeVersion in format.slides setDeck", async () => { | ||
| let entry = FORMAT_BLUEPRINTS.find(blueprint => blueprint.blueprintId === "format.slides")!; | ||
| let serverCode = await readBlueprintFile(entry, "server.js"); | ||
| expect(serverCode).toContain('themeVersion: "workspace.1"'); | ||
| expect(serverCode).toContain("async setDeck(deck)"); | ||
| }); |
There was a problem hiding this comment.
issue (testing): The regression test only searches the bundled server.js text for themeVersion: "workspace.1" and async setDeck(deck) independently, so it passes when the constant appears in an unrelated method or when setDeck does not actually write it. An implementation that still returns an invalid deck can therefore pass the test.
Triggers: When the bundled server contains those strings outside the intended setDeck implementation, or setDeck contains the method signature but does not execute the normalization.
Suggested fix: Extract or execute the setDeck implementation and assert that calling it with {slides} persists a deck whose themeVersion is exactly "workspace.1".
Keep erxes executor-tools + AdminWorkspaceDump while taking upstream CHAT_CHANGE_MESSAGE_BUDGET, codeChangeSerializedSize, and Google Drive work.
Upstream manifest-lib refactor dropped this export; erxes self-host deploy still reads wrangler.jsonc per package when generating instance configs.
darjss
force-pushed
the
fix/slides-setdeck-themeversion
branch
from
September 1, 2026 08:14
ded9f2b to
c14a43b
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
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
7 participants
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.
Why
Buyanaa's agent called
setDeck({ slides })withoutthemeVersion. The nextgetDeck()treated the deck as invalid and re-seeded the 4-slide default, so slides looked broken after a successful write.Scope
packages/workshop-backend/format-blueprints/workspace-slides.gadget:setDecknow always writesthemeVersion: "workspace.1".packages/workshop-backend/format-blueprints/workspace-slides.json: revision 7 → 8.packages/workshop-backend/__tests__/format-blueprints.test.ts: regression test on bundledserver.js.Blast Radius
Deployments reinstall the slides format blueprint when revision changes. Existing user gadgets are unchanged until they are rebuilt from the updated blueprint.
Verification
bunx vitest run __tests__/format-blueprints.test.tsinpackages/workshop-backend(7 passed).Summary by Sourcery
Preserve user-authored slides by normalizing the deck theme version when saving it.
Bug Fixes:
Deployment:
Tests: