feat(tree): add BranchCheckout for viewless, branch-bound checkouts

[jzaffiro]

Description

Adds BranchCheckout, a TreeCheckout subclass that is permanently bound to the SharedTreeBranch it was created over. Companion helpers forkAsBranchCheckout(parent) (fork any checkout into a viewless branch checkout) and getBranchCheckout(branch) (look up the canonical BranchCheckout for a given branch via a module-level WeakMap) round out the surface. All three are package-internal (@internal).

To make this work cleanly, TreeCheckout.fork is refactored into a generic forkWith<T>(ctor), which subclasses (and forkAsBranchCheckout) call directly. The @throwIfBroken guard now sits on forkWith rather than on fork, so every fork entry point — including subclass overrides — runs the broken-state check.

BranchCheckout overrides [disposeSymbol] (not dispose) so that both explicit disposal and the merge auto-dispose path (TreeCheckout.merge calls checkout[disposeSymbol]() directly) clear the registry entry. switchBranch is overridden to throw a UsageError, with the base parameter preserved to keep the override signature-compatible with TreeCheckout.

Reviewer Guidance

The review process is outlined on this wiki page.

• The new symbols are intentionally @internal — no public API surface change, no *.api.md diffs.
• A point worth gut-checking: should the WeakMap registry live on the SharedTree instance instead of the module? Module-global is simpler and works because branch identities are unique across trees, but happy to refactor if you prefer per-tree isolation.
• There's a pre-existing TODO at treeCheckout.ts:322-323 (createTreeCheckout always sets isSharedBranch=true) that affects how forkAsBranchCheckout interacts with views from independentView — out of scope here, but worth a follow-up.
• 18 tests in branchCheckout.spec.ts cover construction, viewlessness, permanence, edits/merges, dispose semantics (including the merge auto-dispose registry cleanup), broken-state propagation, chained forking, and disposeForksAfterTransaction propagation.

[github-actions]

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

Based on the diff (422 lines, 4 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

[github-actions]

On it! Starting review with: Correctness, Security, API Compatibility, Testing

[github-actions]

🔭 PR Review Fleet Report

Note

This report is generated by an experimental AI review fleet and is provided as a beta feature. Findings are a starting point for discussion, not a gate. Use your own judgement.

Verdict: ❌ Request Changes

0 Alert, 1 Stop, 4 Caution

Findings

Sev	#	Area	File	What	Fix
🛑 Stop	H1	API Compatibility	packages/dds/tree/src/shared-tree/branchCheckout.ts:48-54	getBranchCheckout, BranchCheckout, and forkAsBranchCheckout all carry both @internal and @alpha release tags — a combination that is not used anywhere else in shared-tree/ (and appears to be unique in the package). API Extractor allows only one release tag per declaration; when two are present the tool emits a warning and the outcome is implementation-defined. Because @internal takes precedence, the @alpha tag is silently ignored: the symbols are excluded from the alpha API report and from the alpha entrypoint, but any internal consumer reading the doc-comments will be misled into believing these are alpha-stability public APIs accessible via @fluidframework/tree/alpha.	Pick exactly one release tag. If the intent is that these APIs remain private to the package and to other Fluid Framework packages (imported via /internal paths), use @internal alone and drop @alpha. If the intent is to expose them as alpha-stability APIs to external consumers, add them to the alpha entrypoint (src/entrypoints/alpha.ts), regenerate the alpha API report, and use @alpha alone.
🚧 Caution	M1	Correctness	packages/dds/tree/src/shared-tree/branchCheckout.ts:131-138	The branchCheckoutMap.delete(this.mainBranch) cleanup is skipped whenever super[disposeSymbol]() throws after having already set this.disposed = true. This can happen concretely if any 'dispose' event listener throws (emitted at treeCheckout.ts:1305, after disposed = true at line 1298) or if view.dispose() throws inside the views loop. The result is a permanently stale map entry: getBranchCheckout(branch) returns the BranchCheckout with disposed === true, violating the documented invariant that 'getBranchCheckout never returns a disposed instance'. Code that calls getBranchCheckout and skips the disposed check before use would then hit 'already disposed' errors from subsequent operations.	Store the branch reference in a local variable before calling super[disposeSymbol](), then wrap the super call in a try/finally so the map delete always runs: const branch = this.mainBranch; try { super[disposeSymbol](); } finally { branchCheckoutMap.delete(branch); }. This also avoids accessing this.mainBranch (which reads through the disposed transaction stack) after the super call completes.
🚧 Caution	M2	Correctness	packages/dds/tree/src/shared-tree/branchCheckout.ts:131-133	There is a window during super[disposeSymbol]() where getBranchCheckout(branch) returns a BranchCheckout whose disposed flag is already true. The base-class [disposeSymbol]() sets this.disposed = true (line 1298) and then emits the 'dispose' event (line 1305), but the branchCheckoutMap.delete() in BranchCheckout only runs after the super returns. Any 'dispose' event listener that calls getBranchCheckout(branch) during this window receives the instance while it is half-torn-down. If that listener assumes the return value is either a live checkout or undefined, it will be surprised to find a disposed instance.	Move the branchCheckoutMap.delete call to happen before the super, or use a try/finally (see above). Alternatively, delete from the map as the very first action in the override, before delegating to super, so no 'already in map but disposed' window exists.
🚧 Caution	M3	Testing	packages/dds/tree/src/shared-tree/treeCheckout.ts:1194	TreeCheckout.fork() no longer carries @throwIfBroken — the decorator was intentionally moved to forkWith(). The spec has no test that calls checkout.fork() directly on a broken TreeCheckout. The only broken-state fork test added in this PR is for forkAsBranchCheckout, which exercises forkWith but not the fork() entry-point. If @throwIfBroken were accidentally removed from forkWith, or a future refactor split the call path, calling fork() on a broken checkout would silently succeed and create a corrupted child checkout — with no test to catch the regression.	Add a test in treeCheckout.spec.ts: break view.checkout with view.checkout.breaker.break(new Error('broken')), then assert that view.checkout.fork() throws a UsageError matching the broken-state message. This pins the invariant at the fork() call-site, independent of forkWith.
🚧 Caution	M4	Testing	packages/dds/tree/src/shared-tree/branchCheckout.ts:113	BranchCheckout.fork() is not tested in a broken state. The broken-state test in branchCheckout.spec.ts (line 307) only covers breaking the parent TreeCheckout before forkAsBranchCheckout is called. There is no test that breaks the BranchCheckout itself after creation and then calls .fork() on it. If @throwIfBroken on forkWith were ever bypassed for BranchCheckout, forking a corrupted BranchCheckout would register a new stale entry in branchCheckoutMap via its constructor, causing getBranchCheckout to return an invalid checkout that silently appears live.	Add a test: create a BranchCheckout via forkAsBranchCheckout, break it (e.g., branchCheckout.breaker.break(new Error('broken bc'))), then assert that branchCheckout.fork() throws a UsageError matching the broken-state message. Optionally, also assert that no new entry appears in branchCheckoutMap after the failed fork.

---

View workflow run