Skip to content

feat: prepare DynamicGroup row identity reader - #281

Closed
jaeone94 wants to merge 1 commit into
jaeone94/dynamic-group-compatfrom
jaeone94/dynamic-group-row-identity
Closed

jaeone94 wants to merge 1 commit into
jaeone94/dynamic-group-compatfrom
jaeone94/dynamic-group-row-identity

Conversation

@jaeone94

@jaeone94 jaeone94 commented Oct 11, 2026 •

Copy link
Copy Markdown

ELI5

A row needs an identity that survives changes to its position. This prepares the shared-document reader to preserve and validate those identities, and prevents an ordinary row-count edit from leaving them attached to the wrong positions. Row insertion and deletion are not enabled by this PR.

Motivation

Existing DynamicGroup support addresses fields by numeric position. Atomic middle-row insertion/deletion needs stable identity before producers can safely send late edits or preserve surviving connections. This reader preparation is separated from the structural operations so the persisted manifest, schema boundary and validation can be reviewed first.

Summary

  • Accept opt-in properties.dynamicGroupRows, mapping group names to ordered, distinct, nonempty producer-owned ID strings. Scope is node lifetime plus group. Do not assign IDs to untagged workflows.
  • Require catalog group metadata and matching stored counts, including on declared forms. Preserve saved rows above a changed advertised maximum; retain allocation bounds.
  • Validate mint/add/insert/definition ingress before attaching maps. Recheck counts during projection and snapshot reads; expose only deeply frozen IDs in NodeSnapshot.dynamicGroupRows.
  • Refuse tagged scalar count changes on top-level, interior and promoted targets, including catalog-free declared-form writes. Existing field edits and untagged behavior remain unchanged.
  • Preserve the closed manifest during workflow insertion while continuing to scrub unrelated private keys.
  • Use shared-document schema 6. Old readers could resize a tagged group without updating IDs. Older documents are refused and must be re-minted from source; no relabelling or compatibility reader is introduced.

Review and adoption

Stacked on #276 — fix: preserve DynamicGroup widget identities in documents, at 7c39f5bd8323b88bcf69d013bd13023e261a40b3.

This is an unfinished row-operation feature and remains a draft. The manifest and schema boundary need maintainer/FE sign-off before adoption. No producer, agent tool or package release is enabled. Core object_info, numbered prompts and node execution contracts are unchanged.

The next stage must add operation-derived IDs, stable-ID field targets, atomic insertion/deletion, row-aware links/promotions, and admission distinct from accepted-op replay. Shared-definition writes retain the existing KA-1 guard.

Verification

  • Final new suite against parent production: 24 failures / 6 control passes. Restored implementation: 30 passed.
  • npm test -- --maxWorkers=1: 1,844 passed, 2 existing skips. npm run test:exhaustive -- --maxWorkers=1: 2 passed, 2 other-tier skips.
  • npm ci, build, typecheck, lint, purity, imports, pins, profile claims, CodeRabbit configuration, corpus verification and stateless checks: passed, serialized locally. Stateless probe: 18 passed; import graph: 27 modules, 99 dependencies.
  • CI on 04a473c: build/purity/test and exhaustive jobs passed. CI separates the 18 stateless probes from the remaining 1,826 tests; both passed.
  • The parent v5 reader refuses the encoded v6 snapshot through projection and snapshot reads without writes. With the version fence deliberately removed, it accepts count=1 while retaining two row IDs. This is a local compatibility probe, not deployed consumer QA.
Seven individual guard mutations, each caught by its targeted test

Each edit was applied separately and restored before the next run. The indicated failures are actual Vitest assertion output, not setup failures.

Mutation Targeted test Observed failure
src/dynamic-group-rows.ts: remove new Set(ids).size !== ids.length Duplicate-ID ingress expected [Function] to throw an error
Same module: remove count !== ids.length Stored-count mismatch expected [Function] to throw an error
src/applier.ts: disable the tagged scalar-resize guard Top-level resize rejection oracle code: undefined instead of code: 'malformed_op'
src/read.ts: replace frozen snapshot(rows, 1) with structuredClone(rows) Array round trip and frozen IDs expected false to be true
src/types.ts: change schema 6 back to 5 Schema boundary expected 5 to be 6
src/applier.ts: remove manifest preservation during scrubbing Private-name group insertion Manifest has one group instead of two
src/doc.ts: remove declared-controller membership check Serializer count residue expected [Function] to throw an error

Invariants

KA-1/KA-4: semantic mutations and pre-write validation; rejection/retry/abort byte identity. KA-3/KA-13 and FC-3: portable, stateless, Yjs-only runtime. KA-6/FC-5: frozen snapshot data, no new mutation exports. KA-11: schema-6 fence and explicit old-document disposition. KA-12: supplied group metadata validates IDs/counts without replacing declared widget mapping. FC-8: reader does not invent row IDs or rewrite values from defaults. FC-10/KA-12's provisional manifest/admission extension is registered in docs/decisions/EXCEPTIONS.md; no approved deviation is claimed.

Provenance

  • Authored by: interactive session.
  • Verified: local red/green, seven individual mutations, parent-reader probe and serialized gates listed above. Existing composite fixture and rejection oracle reused; corpus expectations were not generated from this applier.
  • Deviations: atomic row writers, initial identity assignment, row-aware topology, CLI/FE/doc-host adoption, performance measurement and full Agent/MCP execution remain follow-ups. This PR does not claim release readiness.

@coderabbitai

coderabbitai Bot commented Oct 11, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@jaeone94

jaeone94 commented Oct 13, 2026 •

Copy link
Copy Markdown
Author

On hold (2026-10-13), pending review. Edited 2026-10-14: first posted as "Deferred". I've proposed that Agent row changes use whole-group replacement expanded by the CLI into existing ops (comfy-cli #1004). If that is agreed, this row-level protocol is not a first-release prerequisite, and it stays a draft for a possible later fine-grained merge model. The proposal has not been agreed yet; see the architecture review.

@jaeone94

Copy link
Copy Markdown
Author

Closing. Agent row changes are moving to whole-group replacement (comfy-cli #1004), which the producer expands into existing ops, so the shared document doesn't need a row identity reader. If row-level operations come back, this can be reopened.

@jaeone94 jaeone94 closed this Oct 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk:R3 PR risk grade (advisory shadow check; grader-owned)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant