feat(sequence): record propertyKeys on interval Add/Change local metadata at submit

[anthony-murphy]

Description

Adds an optional propertyKeys: ReadonlySet<string> field to both IntervalAddLocalMetadata and IntervalChangeLocalMetadata in intervalCollectionMapInterfaces.ts, and populates it at the two inline submit sites in IntervalCollection.add and IntervalCollection.change (props === undefined ? undefined : new Set(Object.keys(props))).

Why

Records which user-defined property keys an interval add/change op carried at submit time. The metadata is small, isolated, and read-only at the consumer; landing it independently keeps the data-layout change separate from the squash semantics that will eventually consume it.

Notes

Prep for upcoming staging-mode squash work for SharedString intervals. This PR does not add the squash filter helpers, the squash branch in resubmitMessage, or any new walker logic.

[github-actions]

Hi! Thank you for opening this PR. Want me to review it?

Based on the diff (194 lines, 3 files), I've queued these reviewers:

• Correctness — logic errors, race conditions, lifecycle issues
• Security — vulnerabilities, secret exposure, injection
• API Compatibility — breaking changes, release tags, type design
• Performance — algorithmic regressions, memory leaks
• Testing — coverage gaps, hollow tests

How this works

• Adjust the reviewer set by ticking/unticking boxes above. Reviewer toggles alone don't trigger anything.

• Tick Start review below to dispatch the review fleet.

• After review finishes, tick Start review again to request another run — it auto-resets after each dispatch.

• This comment updates as new commits land; your reviewer selections are preserved.

• Start review

[anthony-murphy]

Deep Review: Contract mismatch between the new JSDoc and this populate predicate.

The JSDoc added to IntervalAddLocalMetadata.propertyKeys (and the matching one on IntervalChangeLocalMetadata at the line 1357 site) states the field carries "user-defined property keys". But props here is the raw caller-supplied PropertySet, and new Set(Object.keys(props)) snapshots every key including reserved ones — reservedIntervalIdKey and reservedRangeLabelsKey.

The rest of the module is careful to segregate those reserved keys from user properties: getSerializedProperties (intervals/sequenceInterval.ts:85-102) destructures both reserved keys out before exposing properties, and createSequenceInterval strips them from stored interval properties (intervals/sequenceInterval.ts:995-997). On the change path at line 1357, reservedRangeLabelsKey is rejected upstream (intervalCollection.ts:1322-1328), but reservedIntervalIdKey can still flow through here on both paths, and label keys can still slip through the add path.

The PR intentionally lands propertyKeys as a stable data-layout contract for a future squash consumer. Any consumer that reads the JSDoc at face value will mis-handle ops that touched reserved metadata — label changes that look like user-property deltas, or vice versa. The fix is two lines while the field has zero readers; deferring locks the wrong semantic into a permanent contract.

Two valid resolutions:

1. Filter reserved keys at both submit sites — either inline (Object.keys(props).filter(k => k !== reservedIntervalIdKey && k !== reservedRangeLabelsKey)) or by sourcing from the already-computed serialized bag: Object.keys(getSerializedProperties(serializedInterval).properties), which performs the same stripping the rest of the module relies on.
2. Keep raw caller keys and reword the JSDoc on both interfaces to say "property keys present on this op's props bag at submit, including any reserved keys" — so the consumer knows it must filter.

Either resolution closes the finding, but the choice is a contract decision worth a sign-off (Abe27342 or pleath).

[anthony-murphy]

Deep Review: Resolved in eae55e7. Both populate sites now source from new Set(Object.keys(getSerializedProperties(serializedInterval).properties)), which strips reservedIntervalIdKey and reservedRangeLabelsKey centrally — matches the "user-defined property keys" JSDoc. New tests in sharedString.spec.ts:1084-1108 and :1140-1163 assert reserved-key exclusion on both add and change paths.

[anthony-murphy]

Deep Review: Same contract mismatch as the inline at line 1228 — new Set(Object.keys(props)) snapshots reserved keys despite the "user-defined" JSDoc on IntervalChangeLocalMetadata.propertyKeys.

On this change path, reservedRangeLabelsKey is rejected upstream at intervalCollection.ts:1322-1328, but reservedIntervalIdKey is not, so the leak path is real here too. Fix at both sites together (filter the predicate, or reword the JSDoc on both interfaces) — see the line 1228 thread for the two resolution options and the rationale.

[anthony-murphy]

Deep Review: Resolved in eae55e7 alongside the line 1228 site — same getSerializedProperties(...).properties sourcing closes the reservedIntervalIdKey leak on the change path too.

[anthony-murphy]

Deep Review

Reviewed commit eae55e7 on 2026-05-27.

Readiness: 8/10 — ALMOST READY

The prior contract mismatch is resolved: both populate sites now source from getSerializedProperties(serializedInterval).properties, which strips reservedIntervalIdKey and reservedRangeLabelsKey, so propertyKeys matches the "user-defined" JSDoc. New tests assert reserved-key exclusion on both add and change paths. Three polish items remain — see Path to Ready.

Path to Ready

• Resolve inline threads
• Update the PR description so the propertyKeys snippet matches the committed new Set(Object.keys(getSerializedProperties(serializedInterval).properties)) expression, and call out that reserved-key stripping is centralized in getSerializedProperties
• Either add a resubmit-preservation test (disconnect → add({…, props: { a: 1 }}) → reconnect → assert propertyKeys on resubmitted metadata equals new Set(["a"])) using the existing harness, or note in the PR description that this regression test is intentionally deferred to the squash consumer PR
• Link the follow-up squash consumer issue/PR in the description before un-drafting

Context for Reviewers

• PR Interval Collections: Refactor rebase to be based on local references #24867 deliberately slimmed IntervalAddLocalMetadata / IntervalChangeLocalMetadata from serialized op data (original) down to a live LocalReferencePosition (interval); this PR reintroduces a small property-derived slice (keys only, not values) onto the same shape — direction-of-travel context for future field accretion on these interfaces.
• Resubmit/rebase/stashed-op paths route through this.add(...) / this.change(...) and the unified pending-FIFO from Interval Collections: Unify pending state managment #24829, so the two new populate sites are the only ones needed — propertyKeys flows through existing replay paths automatically. IntervalDeleteLocalMetadata is intentionally omitted: delete ops carry no props bag in the post-IntervalCollection: Remove op emitter, and related interfaces #24186 / post-Interval Collections: Refactor rebase to be based on local references #24867 design.
• Reserved-key exclusion via getSerializedProperties aligns with the original interval-ID design from Add a unique ID to Interval properties, allow multiple intervals with the same (start, end) #6407 — framework owns intervalId / range-label lifecycle, and getSerializedProperties is the canonical filter point.
• intervalCollectionMapInterfaces.ts is unusually low-churn (1 edit / 6mo); the type-shape additions here are purely additive optional fields with no rename or removal.

For human reviewer

• Test-placement convention call (inline thread on sharedString.spec.ts) — the relocation to intervalCollection.spec.ts (or a new intervalCollection.localMetadata.spec.ts) is mechanical, but if area owners prefer the current location (harness closer to the integration boundary), that overrides the convention argument. Suggested sign-off: jzaffiro (authored SharedString test PR merge-tree: Add regression tests for bugs with zero length obliterate  #22787).
• Defer-vs-pin resubmit regression test — judgment call on whether to pin the resubmit contract here or in the consumer PR. Suggested sign-off: Abe27342 (previously flagged tolerant pending-change bookkeeping on Interval Collection: local rollback #24307 — exact pending-metadata surface this PR extends).
• ReadonlySet<string> vs readonly string[] — the O(1) has() justification is defensible for the future squash walker's expected access pattern, but only the consumer-PR drafter can confirm. Worth a glance during consumer-PR review, not blocking here.
• Field name propertyKeys vs propertyDeltaKeys — taste call; JSDoc supplies disambiguation. Revisit if consumer-PR call sites read better with the more specific name.
• Draft-PR landing discipline — confirm the squash consumer is at least drafted before merging so propertyKeys doesn't become orphaned dead data on the submit path. Tag jzaffiro on the public interval-collection API surface.

Suggested description

## Description

Adds an optional `propertyKeys: ReadonlySet<string>` field to both `IntervalAddLocalMetadata` and `IntervalChangeLocalMetadata` in `intervalCollectionMapInterfaces.ts`, and populates it at the two inline submit sites in `IntervalCollection.add` and `IntervalCollection.change`:

```ts
propertyKeys:
    props === undefined
        ? undefined
        : new Set(Object.keys(getSerializedProperties(serializedInterval).properties)),

Sourcing from getSerializedProperties(serializedInterval).properties (rather than raw Object.keys(props)) centralizes reserved-key stripping in getSerializedProperties, which already destructures reservedIntervalIdKey and reservedRangeLabelsKey out of the returned properties. The resulting propertyKeys contains only user-defined keys — verified by the new test cases for both add and change paths.

undefined means the op was submitted without any props bag at all; an empty Set means an explicit empty {} was passed. The distinction is preserved because the future squash consumer needs to tell "this op did not touch user properties" from "the user explicitly cleared/no-op'd the bag."

Why

Records which user-defined property keys an interval add/change op carried at submit time. The metadata is small, isolated, and read-only at the consumer; landing it independently keeps the data-layout change separate from the squash semantics that will eventually consume it. This reintroduces a small property-derived slice onto interfaces that #24867 had deliberately slimmed — a knowingly additive step.

Notes

Prep for upcoming staging-mode squash work for SharedString intervals. This PR does not add the squash filter helpers, the squash branch in resubmitMessage, or any new walker logic. The resubmit path preserves propertyKeys for free because resubmitMessage re-forwards the same localOpMetadata object via submitDelta (no recompute) — pinning a regression test for that round-trip is deferred to the consumer PR.

Follow-up: link the squash consumer issue/PR here before un-drafting.

<details><summary>Review history (1 prior review)</summary>

- `2f02e9f` 2026-05-27 · **5/10** — contract mismatch flagged inline at both submit sites: JSDoc promised "user-defined property keys" but populate predicate snapshotted raw `Object.keys(props)`

</details>

[anthony-murphy]

Deep Review: This new 174-line describe("IntervalCollection local op metadata propertyKeys", …) block exercises getIntervalCollection().add/change exclusively and asserts nothing SharedString-specific — the SharedString is only used to instantiate the harness. The established home for IntervalCollection behavior tests is the sibling packages/dds/sequence/src/test/intervalCollection.spec.ts (alongside intervalCollection.rollback.spec.ts, intervalCollection.events.spec.ts, intervalCollection.detached.spec.ts), and intervalCollection.spec.ts:87-98 already instantiates SharedStringClass for interval-collection tests, so the harness is portable.

sharedString.spec.ts's historical scope (PRs #22787, #23011) is SharedString text/obliterate regressions, not interval-collection metadata. Move the new describe block into packages/dds/sequence/src/test/intervalCollection.spec.ts, or create intervalCollection.localMetadata.spec.ts following the intervalCollection.rollback.spec.ts pattern. The relocation is mechanical — the harness only depends on SharedStringClass and MockContainerRuntimeFactory, both already in scope of the other interval specs.

If the area owners prefer the current location (harness closer to the SharedString integration boundary), that overrides the convention argument — worth a sign-off either way.