[devin-ai-integration]

Why

Task: HSF-1393. Sibling app PR (draft, waits on this release): https://github.com/flowcircuits/hvakr/pull/8191.

@hvakr/firestate creates a document from any partial collection diff whose id is unknown: updateState applies the diff to localState, and sync() calls batch.set for every id not in syncState. The hvakr app guards space writes in ProjectProvider (HSF-1391, flowcircuits/hvakr#8000), but firestate's own undo/redo closures call updateState directly. So undoing a partial edit to a space that a collaborator (or a server function) deleted writes a fragment of that space back to Firestore.

Summary

A collection write can now create a document only as a whole document. In updateState, every id that the merged diff adds to the collection must pass validateCreate; failing ids are dropped (the rest of the diff still applies) and reported to onRejectedCreate.

// CollectionDefinition / col() option
validateCreate?(data: TData): boolean
// default: schema ? schema.safeParse(doc) passes, ignoring issues at FieldValue sentinel fields : false

// FirestateConfig / FirestateProvider prop
onRejectedCreate?: (ctx: { path; id; data; source: 'update' | 'undo' | 'redo' }) => void
// default: console.warn

• add() is unchanged (already schema.parses the full doc).
• Undo/redo closures pass their source and restoreIds: the ids the original write deleted whole (for the undo) or created whole (for the redo), from add(), remove(), or update() (including update({ X: deleteField() })). Those ids skip validateCreate, so undoing a delete restores the document even when it fails the current schema.
• The default check ignores Zod issues whose path runs through a FieldValue sentinel (serverTimestamp(), increment(), arrayUnion(), …), so a create with sentinels still passes. A custom validateCreate receives the sentinels as-is.
• An undo/redo step that applies nothing because every create in it was rejected returns false. The undo manager then drops it (no move to the redo/undo stack, no onUndo/onRedo). UndoAction.undo/redo may now return false for this. onNavigate still runs, because it runs before the action.
• The check runs in updateState, before the no-op collapse and undo entry, so a rejected id never reaches localState, sync()/batch.set, or the undo stack.
• Public export: RejectedCreateContext. Docs: README, docs/architecture.md, docs/api-recipes.md, AGENTS.md contract.
• Version 0.2.1 → 0.3.0 (behavior change for update()-creates). Not published, not tagged.

Tests

New src/__tests__/partial-create.test.ts: undo and redo of a partial edit to a remotely deleted doc, unknown id mixed into a valid diff, schema-valid update() create, custom validateCreate, no schema/no validator, add() without schema, undo/redo of add() and remove(), default warning.

• Before the fix: npx vitest run src/__tests__/partial-create.test.ts → 7 failed | 3 passed (undo/redo recreated X with batch.set).
• Review follow-up (11e8756), before the fix: npx vitest run src/__tests__/partial-create.test.ts → 4 failed | 12 passed (undo of an update() whole delete of a schema-failing doc, redo of an update() create, create with serverTimestamp()/arrayUnion(), an all-rejected undo still moved to redo). npx vitest run src/utils/undo.test.ts → 3 failed | 35 passed. The grouped undo with mixed results already passed (coverage). After: both files → 54 passed.
• After: 10 passed.
• pnpm typecheck (tsc --noEmit) → clean. vitest run → 18 files, 317 passed | 3 skipped. tsdown build → OK.
• No lint script in this repo. Prettier is applied to the new test; touched existing files were already not Prettier-clean on master, so I did not reformat them.

Migration needed: no. Only client write behavior changes; no stored data changes shape.

Decisions to confirm

• Taken from the task: reject, not accept the gap. Default validator is the collection schema (registry collections always have one); collections without a schema reject every update()-create unless they pass validateCreate. Alternative: allow schemaless creates by default. This is the breaking part for low-level defineCollection users who create via update().
• Rejected creates are dropped silently to the caller (no throw) and reported through onRejectedCreate / console.warn.

Release: a human must publish @hvakr/firestate@0.3.0 after merge; the hvakr PR bumps to it.

Opened by Devin.

• Open (Tryston): every hvakr project collection creates documents through update(), and only spaces were schema-checked before. Whether 0.3.0 enforces the schema for all collections at once, or hvakr starts in report-only mode for non-space collections, is in the review thread on collection.ts (finding 3). This PR keeps the strict default.

Link to Devin session: https://app.devin.ai/sessions/ff96b825ae42445bb8a11515d74ef30c
Open in Devin Desktop: https://app.devin.ai/desktop/session/ff96b825ae42445bb8a11515d74ef30c?variant=devin

---

^{Open in [code]smith · Need help on this PR? Tag @codesmith-bot with what you need.}

[devin-ai-integration]

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

• Disable automatic comment, CI, and merge conflict monitoring

---

Original prompt from Grok Bot — Billy (service account)

Task: HSF-1393 Firestate: stop partial updates from creating documents (undo/redo bypasses the app guard)
Task page: https://app.notion.com/p/407c597105c9817e8bc8d0f127ead722
Repos: flowcircuits/firestate (base main/default branch; the library published as @hvakr/firestate, currently 0.2.1) and flowcircuits/hvakr (base master)
Status: In progress
Requested by Tryston Perry (task owner).

Note: Do NOT try to write to the Notion task page. Just handle the task.

WHY THIS IS A RELAUNCH: two earlier Claude Code sessions for this task stopped without pushing anything, both while digging through node_modules in the hvakr checkout looking for the firestate source. The core fix belongs in the separate flowcircuits/firestate repo; clone and work there directly instead of reading node_modules.

CONTEXT (task body from Notion, fetched just before launch):
<<<TASK BODY
#``# Problem Statement

<code>@hvakr/firestate</code> 0.2.0 creates a document from any partial diff whose ID is not in the collection: <code>updateState</code> applies the diff to local state, and <code>sync</code> calls <code>batch.set</code> for any ID that is not in <code>syncState</code>. HSF-1391 guards space writes in <code>ProjectProvider.updateProjectCollection</code>, but these paths bypass that guard and can still write a partial space:

• Firestate's own undo/redo closures call its internal <code>updateState</code> directly. A partial undo or redo diff for a space that was deleted outside this client's undo stack (by a collaborator, a server function, or a non-undoable write) recreates a fragment.
• Study-apply undo calls the spaces handle's <code>update</code> directly (<code>recordStudyApplyUndo</code>).

#``# Context Links

• HSF-1391 (app guard)
• Incident: <mention url="https://app.notion.com/p/407c597105c98135961bc3079c7ccc98" />

#``# Goal

Decide whether firestate should refuse to create a document from a partial update in general (for example, create only through <code>add</code>, or validate again... (5464 chars truncated...)

[devin-ai-integration]

/review on 9920ae3:

Summary. The guard does what the task asks. The check runs in updateState before the no-op collapse and before the undo entry. A partially rejected diff keeps the ids that pass. restoreId makes the undo of remove() and the redo of add() recreate the whole document. The new export (RejectedCreateContext), the col() and FirestateProvider plumbing, the 0.3.0 bump and the docs all match the code. I found three Medium problems. Two are in the default rules: the undo of a whole-document delete made through update() is not treated like the undo of remove(), and an update() create that contains a Firestore sentinel is always rejected. The third is a regression risk for hvakr, where every project collection creates documents through update().

Local checks (head 9920ae3, pnpm, Node 22):

• pnpm typecheck (tsc --noEmit): pass.
• pnpm test (vitest): 18 files, 307 passed, 3 skipped.
• pnpm build (tsdown): pass.
• Lint: the package has no lint script.

To confirm findings 1 and 2, I ran scratch vitest cases in a copy of the repo outside the checkout (not committed):

• Finding 1: update({ X: deleteField(), Y: {v:2} }) → sync → undo. Y was restored. X was dropped with source: 'undo' because its stored data fails the schema.
• Finding 2: update({ X: { name, v, t: serverTimestamp() } }) on an empty collection was rejected.

Findings:

1. Medium. src/core/collection.ts:417: the undo of a whole-document delete made through update({id: deleteField()}) is validated as a create. remove() undo skips the check through restoreId.
2. Medium. src/core/collection.ts:188: the default validateCreate runs schema.safeParse on the merged diff. A create that contains a serverTimestamp(), increment() or other FieldValue fails for any typed field, and the write is dropped without an error.
3. Medium. src/core/collection.ts:350: regression risk for hvakr. Every project collection creates documents through handle.update(diff) in ProjectProvider.updateProjectCollection. Only spaces had a schema guard before this change. Any non-space create that fails its Zod schema is now dropped silently. hvakr#8191 only adds telemetry for these drops.
4. Low. AGENTS.md:97 still says partial update(diff) calls are never Zod-validated. That is no longer true for creates.
5. Low. An undo or redo whose entries are all rejected still pops the stack and runs onUndo/onRedo and getUndoPath navigation, but nothing changes. You could report this through onRejectedCreate only, or leave it as is.
6. Low. There is no test for findings 1 and 2, and no test for a grouped undo (undoGroupId) that mixes a rejected id with an accepted one.

[devin-ai-integration]

Medium. The undo of a whole-document delete made through update() is validated as a create, so it can lose part of the undo.

When update({ X: deleteField() }) removes a whole document, its undo diff is the complete stored document. This closure passes { source: 'undo' } with no restoreId, so validateCreate runs on it. If the stored document fails the current schema (for example a legacy document with drifted fields, which hvakr audits for), the undo drops X. The rest of the group still applies, so the user gets a partial undo with no error. I reproduced this with a scratch test: update({ X: deleteField(), Y: {v:2} }) → sync → undo restored Y and rejected X with source: 'undo'.

remove() skips the check for the same case through restoreId. hvakr deletes most documents with update + deleteField() (it has only a few .remove( calls), so this is the common delete path there.

Suggested fix: Generalize restoreId to restoreIds: Set<string>. In updateState, compute the ids in rawBase that are absent from newLocalState (whole deletes) and pass them to the undo closure. Compute the ids this write created whole (absent from rawBase, present after the write) and pass them to the redo closure. Then use the set in addDocument and removeDocument too. Add a test for undo of an update() delete where the stored document fails the schema.

[devin-ai-integration]

Fixed in 11e8756. Undo/redo closures now carry restoreIds, not one restoreId. In updateState, the undo gets the ids the write deleted whole (in rawBase, absent from the new state) and the redo gets the ids it created whole. add() and remove() use the same set. Restoring those ids skips validateCreate.

Test (failed before the fix, passes now): undo of an update() whole delete restores a doc that fails the current schema. It sets up X with v: 'legacy' (fails ItemSchema), runs update({ X: deleteField(), Y: { v: 2 } }), syncs, then undoes. X comes back whole with a batch.set and nothing is rejected. A redo plus a second undo round-trips too. The new test redo of an update() create restores it whole after a remote delete covers the redo side.

[devin-ai-integration]

Medium. The default validateCreate rejects every update() create that contains a Firestore sentinel, and the caller gets no error.

The default is schema.safeParse(mergedDoc). A merged document such as { name, createdAt: serverTimestamp() }, or one with increment() or arrayUnion(), fails any typed Zod field (z.number(), z.date(), a Timestamp schema). I reproduced this with a scratch test: update({ X: { name: 'X', v: 1, t: serverTimestamp() } }) against t: z.number().optional() was rejected. AGENTS.md and the README say diffs are not Zod-validated because they can contain sentinels, and update() was the only create path that accepted sentinels (add() already runs schema.parse). After this change, these creates are dropped with only a console.warn by default.

Suggested fix: Run the default validation with FieldValue sentinels removed. For example, strip every FieldValue leaf before safeParse, or skip the paths that hold sentinels. Also document the limit next to validateCreate, and add a test for a create that includes serverTimestamp().

[devin-ai-integration]

Fixed in 11e8756. The default check still runs schema.safeParse(doc), but now ignores every issue whose path runs through or ends at a FieldValue sentinel (pathHitsSentinel). Firestore resolves sentinels on write, so a typed field does not have to match the sentinel. Other issues still reject: a missing required field still fails when a sentinel sits next to it. A custom validateCreate receives the sentinels as-is (now documented in types.ts, the README, and the docs).

Tests: accepts a create whose sentinel fields stand in for typed values (createdAt: serverTimestamp() against z.number(), plus tags: arrayUnion('a')) failed before the fix and passes now. still rejects a create that fails the schema outside its sentinels passes.

[devin-ai-integration]

Medium. Regression risk for existing consumers: every update() create that fails the schema is now dropped without an error. hvakr creates documents in all project collections this way.

In hvakr, ProjectProvider.updateProjectCollection creates documents in every project collection (systems, zones, equipment, wall/roof/door types, comments, and others) with handle.update(diff). Before this PR, only spaces had a schema check, through getInvalidSpaceCreates. Other collections wrote whatever the caller sent. With 0.3.0, any create in those collections that fails its Zod schema disappears from local state. Examples are AI-tool output, imports, template copies, or an optional field set to null instead of being left out. The user sees nothing; the only signals are console.warn and the PostHog capture in hvakr#8191. The PR description calls this breaking only for schemaless defineCollection users, and it says registry collections are safe because they always have a schema. That does not cover schema-invalid creates in registry collections.

I did not check how often hvakr create paths produce schema-invalid records.

Suggested fix: Pick one before you publish 0.3.0:

• (a) Audit the hvakr create call sites, or run a local check of safeParse on each create path in tests.
• (b) Release in stages: let hvakr pass a validateCreate that logs and allows non-space collections, and enforce after the telemetry is clean.

Also state the user-visible effect ("create silently ignored") in the release notes and the hvakr PR.

[devin-ai-integration]

Default policy not changed (waiting for Tryston). Audit of flowcircuits/hvakr at fb0d98df1:

How creates reach firestate. All 24 project collections are col() entries with a Zod schema (packages/app/src/contexts/Project/collections/*.ts, for example systems.ts:21). Every app write goes through ProjectProvider.updateProjectCollection → handle.update(diff) (ProjectProvider.tsx:515). There are no add() calls on project collections, so every create is an update() create, and before 0.3.0 only spaces were checked (getInvalidSpaceCreates, ProjectProvider.tsx:177). With a study open, base-collection creates become a write to the existing study document (ProjectProvider.tsx:494-513), so 0.3.0 does not check them at that point.

Create paths with real risk of failing the schema:

1. Applying a study (overwriteProjectFromStudy, ProjectProvider.tsx:614 onward, writes at about line 697). The study post records become new base documents through handle.update. These records were stored when they were written into the study, without a schema check for non-space collections, and they may predate later schema changes. Highest risk: an apply could drop part of a study, and the user would only see that afterwards.
2. AI applied-diff undo (AIProjectProvider.tsx:296-326). It restores the stored originals through updateProjectCollection(key, diff). A document the AI deleted comes back as an update() create of the stored record, so a legacy record that fails the current schema would not be restored. This is not an undo-manager replay, so restoreIds (finding 1) does not cover it.
3. Study-apply undo (recordStudyApplyUndo.ts:21-89). It restores deleted documents the same way, through a direct update().
4. Stored data already fails schemas. collections/dashboards.ts:35-55 parses reads and skips dashboards that fail DashboardSchema. Any flow that copies such a record into a create would now drop it.

Low risk: UI creates that build a full typed record, such as ChatView.tsx:393 and ChatView.tsx:426 (ProjectCommentData built from the Zod-inferred type). With 11e8756, serverTimestamp() fields no longer reject.

Not verified: I did not audit all ~40 caller files line by line, and I did not measure how often production records fail their schemas. There was no production data access.

Options:

• (a) Keep the strict default, and before publishing, add hvakr tests that run each create path above (study apply, AI undo, study-apply undo) against safeParse.
• (b) Staged rollout (my recommendation): hvakr passes a validateCreate for non-space collections that reports to PostHog (INVALID_DOCUMENT_CREATE, already wired in hvakr#8191) and returns true, and keeps strict checking for spaces. Enforce per collection once the telemetry is clean. firestate needs no change for this.
• (c) Exempt known restore paths in hvakr: wrap study apply and AI undo so they write through add()-like full restores. That is more code, and it still leaves UI creates strict.

[devin-ai-integration]

Lows from the review, addressed in 11e8756:

• AGENTS.md:97: now says only edits to existing documents skip Zod. A collection update() that creates a new id is checked, and the default ignores issues at sentinel fields. The partial-create contract in AGENTS.md also covers restoreIds and dropped undo steps.
• All-rejected undo/redo step: fixed. When every create in a replay is rejected, updateState returns false. The undo manager then drops the action (or a group whose members all return false): it does not move to the other stack, and onUndo/onRedo do not fire. UndoAction.undo/redo may return false (typed and documented). One limit remains: onNavigate still runs, because the manager navigates before it runs the action and cannot know in advance that the step is dead. Moving navigation after the action would change the order for every undo, so I left it.
• Tests: grouped undo with mixed results (a grouped undo applies the members it can and rejects the rest: Y restored, X rejected, one onUndo, and redo works). an undo whose every create is rejected is dropped, not moved to redo. 4 new undo.test.ts cases for actions that return false (single, redo, a partly dead group that is kept, a fully dead group that is dropped). The tests for findings 1 and 2 are in their threads.

Before the fix: partial-create 4 failed | 12 passed, undo 3 failed | 35 passed. After: 54 passed. Full suite 317 passed | 3 skipped, tsc --noEmit clean, tsdown build OK.