diff-aware incremental grant expansion

[manojacs]

Expand only the subgraph affected by an incremental change instead of rebuilding and walking the whole entitlement graph. Seeds from both new edges and changed-membership entitlements, so a new member on an existing group propagates; new edges that close a cycle fall back to full expansion. Source reads stream and writes flush in chunks to bound memory. Additions only — callers use full expansion for change sets with revocations.

• expand: IncrementalExpander.ExpandChanges (streaming, chunked flush)
• sync: WithPreserveEntitlementGraph, GraphFromToken, NewExpanderStore
• synccompactor: WithIncrementalExpansion (diff-aware, cycle fallback)
• tests: expander + Pebble-c1z compactor differentials (incremental == full)

[github-actions]

General PR Review: [DO NOT REVIEW YET] feat(expand): diff-aware incremental grant expansion

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base b8a6b86df21f.
Review mode: full
View review run

Review Summary

Scanned the full PR diff for security and correctness. This adds diff-aware incremental grant expansion (expand.IncrementalExpander), a c1z entitlement-graph sidecar (Pebble-only; SQLite degrades to full expansion), and compactor wiring (WithIncrementalExpansion), all as additive exported APIs. The change is well-covered: end-to-end differential tests assert incremental output is byte-identical to a full rebuild (sources/provenance included) on real Pebble c1z files, plus cycle and revocation-decline fallbacks, chunked-flush, dangling-ref, and SQLite-degrade cases. No security issues and no confident correctness bugs found. The prior low-confidence finding on the State interface (new PeekEntitlementGraph / ClearEntitlementGraphTransientState methods) still applies — only the internal *state implements it, so impact is limited; not re-flagged here.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• pkg/sdk/version.go:3 — the SDK version stays v0.18.4 despite additive-but-breaking changes to the exported State interface (pkg/sync/state.go:26,28) and several new public APIs. Per the repo-local criteria, a 0.x minor bump is the compatibility signal for signature/interface/default changes; consider bumping before merge.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In pkg/sdk/version.go:
- Around line 3: The exported State interface in pkg/sync/state.go (lines 26 and 28)
  gained two methods (PeekEntitlementGraph, ClearEntitlementGraphTransientState),
  which breaks any external implementer, and the PR adds several new public APIs
  (WithPreserveEntitlementGraph, GraphFromToken, GraphFromStore, EntitlementGraphStore,
  NewExpanderStore, WithIncrementalExpansion, expand.IncrementalExpander). The version
  constant is still "v0.18.4". Per the repo-local SDK criteria, reflect this
  compatibility-relevant change with at least a 0.x minor version bump so downstream
  connectors get the signal.

[github-actions]

No blocking issues found.

[kans]

@manojacs

Review feedback from both me, and two Fable passes over the PR considering it in an of itself, and in regard to the big sync replay effort. Replays (eg delta queries in entra) may as well be a new flavor of online/live compaction with certified merge semantics around connector defined scopes (an etag for a page...) instead of the SDK's row level scoping, and they already solve the negative case of tomb stones. I plan on finishing off both sides after both change sets land - incremental expansion for replays, and tombstones for compaction.

Review: incremental grant expansion

The design direction is right. But there are two blockers that mean the incremental path never actually executes against a real store today, two correctness divergences from full expansion that are masked by those blockers, and several soundness/lifecycle gaps. Details below, then the requested changes.

Blockers

B1. The incremental expander cannot read grants through the real store adapter — it always errors and silently falls back to full expansion.
The compactor wires the expander through sync.NewExpanderStore, whose read methods enforce requireEntitlementRefs (entitlement must carry structured resource refs, never a bare id — pkg/sync/syncer.go). incremental.go builds bare-id entitlements (v2.Entitlement_builder{Id: entitlementID}.Build()) for every grant read in both forEachGrant and principalKeySet. The first recomputeDestination fails with "has no resource refs", and expandGrants warn-logs and runs full expansion. Both compactor parity tests pass through the fallback — "fell back to full" and "incremental worked" produce identical output, and the tests assert only output parity. Fix is mechanical (the destination entitlement is already fetched — pass it through; fetch source entitlements too), but the tests must also assert the incremental path actually ran (assert on IncrementalResult or a fallback counter), or this regresses invisibly.

B2. SQLite lifecycle refuses the resume the incremental path depends on.
The merge leaves the new sync ended; expandGrantsIncremental reopens it with StartOrResumeSync. C1File.ResumeSync hard-rejects ended syncs with FailedPrecondition ("cannot resume sync that has already ended"), which StartOrResumeSync propagates — so incremental always declines on SQLite outputs. It only works on Pebble because that adapter's ResumeSync happens not to check ended_at — an accident, not a contract (and both compactor tests use Pebble). Either reopen via a supported path (SetCurrentSync) with tests on both engines, or make WithIncrementalExpansion loudly Pebble-only.

Correctness bugs (currently masked by B1)

C1. Wrong directness predicate for shallow edges.
incremental.go gates shallow edges with isDirectGrant (len(sources) == 0). The full expander uses isGrantDirectOnEntitlement (len(sources) == 0 || sources[entitlementID] != nil). These disagree for a principal who is both a direct member of B and expanded into B — full expansion merges that into one grant whose sources map marks B direct. Add a new shallow edge B→Y in an increment: full propagates the principal to Y; incremental sees a non-empty sources map, calls it indirect, and silently drops the grant. The same predicate also sets IsDirect on newly written grants, so provenance is recorded wrong even on deep edges.

C2. Sources/provenance are never merged, and the drift compounds.
recomputeDestination skips any principal already present on the destination (no sources-map update, unlike mergeContributionIntoExistingGrant on the full path), and records only the first contributing edge for new principals. Even when the (entitlement, principal) row set matches full expansion, the sources maps don't — and directness for future shallow-edge evaluation is read from those sources maps, so each incremental round consumes the previous round's drifted provenance. Errors compound across compaction cycles. Parity tests must compare full grant rows including sources, not just entitlement|principal keys.

C3. Edge-spec changes on an existing edge are invisible.
baseGraphHasEdge checks only that (src, dst) exists. An increment can overwrite a rule grant with a widened annotation — shallow→deep, broader/removed resource-type filter — same endpoints, different semantics. Incremental skips it, seeds nothing, and produces a silently under-expanded artifact. (AddEdge already has deep-wins/unfiltered-wins merge semantics for duplicate edges; the compactor path never lets it see the new spec.) A narrowed spec (deep→shallow, filter shrunk, entitlement id removed from the annotation) is a revocation-shaped change and must auto-decline to full expansion. Note true row deletions can't happen in today's compactor (diff/deletion sync types are rejected as inputs), but annotation rewrites absolutely can — this is the concrete, checkable form of "detect revocations and auto-decline".

Unsoundness

U1. The caller's base graph is mutated in place, poisoning retries.
ExpandChanges adds new edges to the caller-owned incrementalBaseGraph before the cycle check. If a Compact run fails after that mutation and the caller retries with the same options object, baseGraphHasEdge now reports the never-expanded edges as present, newEdges comes out empty, and the retry finishes "successfully" with an unexpanded artifact. Clone the graph, or stage edges until a commit point.

U2. WithPreserveEntitlementGraph breaks expansion replay.
PrepareExpansionReplayToken relies on a finished token carrying no graph. A preserved graph has Loaded=true and every edge IsExpanded=true, so a replayed sync skips graph loading and the expander reports done immediately — replay silently no-ops. On main this can't happen because final tokens never carry a graph; this branch changes that invariant. PrepareExpansionReplayToken must clear/reset a preserved graph, plus a replay test pinning the behavior. (This is the direct collision point with the sync-replay work.)

Requested changes

1. Fix B1 — pass full entitlement records to all grant reads; make the compactor tests fail if the incremental path falls back (assert on IncrementalResult / a fallback counter).
2. Fix or scope B2 — supported reopen on SQLite (SetCurrentSync) with both-engine tests, or explicit Pebble-only.
3. Fix C1 — use isGrantDirectOnEntitlement(grant, sourceEntitlementID) for shallow filtering and IsDirect recording. Add a differential: principal who is both direct and expanded member of the source, new shallow edge in the increment.
4. Fix C2 — merge sources into existing grants and record all contributing edges on new grants; parity tests compare full rows including sources maps.
5. Derive changes inside the compactor — the merge already touches every applied record: collect changed entitlement ids and new/changed expandable annotations there, and drop the caller-supplied changedEntitlementIDs parameter (WithIncrementalExpansion(baseGraph) only). SQLite's attached merge can SELECT DISTINCT entitlement_id of applied-sync grants for pennies; the Pebble overlay/fold observes every applied record during the merge it already does. Compare edge specs, not just endpoints (fixes C3): widened specs seed re-expansion; narrowed specs / access-removing overwrites auto-decline to full. Trust the data, not the call site.
6. Make the revocation auto-decline a distinct, named branch — it is the future tombstone hook. Implement the decline from change 5 as its own explicit code path (e.g. a named sentinel like ErrIncrementalRevocationDecline, sibling to ErrIncrementalFallback), not folded into a generic error path. Today that branch means "decline to full." A later stage flips that same detection point from "decline" to "apply deletions in the merge, seed the affected entitlements, reconcile" — the flip must be a local change at one named site, not archaeology through error handling.
7. Document the seed contract as direction-neutral. Name and document changedEntitlementIDs (or its compactor-derived successor) as "entitlements whose membership changed in either direction." The additions-only restriction belongs to the write behavior (the current decline in change 6), not to the seed semantics. Costs a comment now; prevents a v2 interface later when removals are seeded through the same parameter.
8. Fix U1 — never mutate the caller's graph on a path that can fail or decline.
9. Converge the finish path with the other compaction paths — call Cleanup before EndSync/Close; treat post-EndSync Close failure as fatal (currently it triggers full-expansion fallback against a closed store); EndSync on the generic-error path like the cycle-fallback path does; bound the walk with remaining runDuration and poll ctx.Err(); skip-with-warn on dangling entitlement refs like the full evaluator (fetchEntitlement treats NotFound as skip) instead of erroring into fallback; log IncrementalResult and whether incremental ran or fell back.
10. Fix U2 — reset the preserved graph in PrepareExpansionReplayToken; add the replay test.
11. Cheap insurance tests —
    (a) Dangling-ref differential: increment adds a grant referencing an entitlement absent from the merged set; assert identical final rows between incremental and full.
    (b) Sealed-artifact lifecycle: after incremental's end→resume→write→end sequence, reopen the artifact and verify the sync record is sealed and the Pebble by_principal index covers the incrementally written grants (today this works via deferredIdxPending re-arming, but nothing pins it).
    (c) Revocation parity differential, asserting today's behavior: an increment carrying a revocation-shaped change (narrowed annotation / access-removing overwrite) hits the named decline branch from change 6, and incremental-with-fallback == full. This pins the decline contract, and it is the red test the future apply-deletions stage turns green.
12. Measure the preserved-graph token on a large nested-groups graph, and strip Actions/ExpansionPlan/ExpansionMetrics from the graph before the final checkpoint regardless (nothing strips them today). If still large, persist the graph in the c1z instead of the token.

Items 1–4 are correctness blockers; 5–8 and 10 are unsoundness/API-shape (6 and 7 are cheap now and structural later); 9 is convergence hygiene; 11–12 are cheap tests and a measurement.

Notes (no action required)

• Mid-sync checkpoint/resume machinery is untouched and remains correct; the only behavioral change is skipping ClearEntitlementGraph on the final checkpoint when preserve is set.
• A crash mid-incremental-expansion is safe: the compaction tmp dir is discarded and re-run, and expanded-grant writes are idempotent by deterministic grant id. Worth stating in the PR as a deliberate choice.
• The cycle → full-expansion fallback is the right call: a cycle has no topological order, its correct semantics are a fixpoint (every member of the cycle gets the union of all members), and the full path's SCC-collapse machinery (FixCycles) already handles that. Re-implementing it incrementally would be re-implementing full expansion in the riskiest corner of the problem space.
• The Pebble reopen-after-seal lifecycle (resume unseals, grant writes re-arm the deferred index, second EndSync rebuilds by_principal) looks deliberate and correct — test 11(b) exists to pin it, since it works by a chain of properties nobody asserts together.

Other stuff

• After the code is stable, do a profiling pass over the incremental expansion path for both cpu and memory, and then give it to Fable (historically, everything else will fail). Lets make sure allocations don't explode, etc.

[github-actions]

🟡 Suggestion: This adds a method to the exported State interface, which is a breaking change for any downstream that implements it. In practice State is only implemented by the unexported *state and can't be injected into the syncer (no public constructor/setter accepts a custom State), so the real-world impact is low. Flagging only for SDK-compat awareness — no change required if State isn't intended as a downstream extension point. (low confidence)

[github-actions]

No blocking issues found.

[github-actions]

🟡 Suggestion: If StartOrResumeSync fails here, the store may be left partially resumed, but this returns a plain error that the caller treats as a safe fallback to full expansion (case err != nil → warn + full). Elsewhere the code deliberately marks state-ambiguous failures (restoreEndedSync, finishIncrementalExpansion) as errIncrementalFatal precisely because running full expansion against an unknown store state is unsafe. Consider whether a resume failure should also be fatal (or otherwise guaranteed to leave the store in the ended state the full path expects). Low confidence — depends on StartOrResumeSync leaving the store untouched on error.

[github-actions]

No blocking issues found.

[github-actions]

🟡 Suggestion: These two additions (PeekEntitlementGraph, ClearEntitlementGraphTransientState) expand the exported State interface. Any external type that implements sync.State would fail to compile until it defines them. In practice only the internal *state implements this, so the risk is likely nil — but per the SDK compatibility criteria, adding methods to an exported interface is a breaking change. If State is intended to be internal-only, consider documenting that. Low confidence.

[github-actions]

No blocking issues found.

[github-actions]

No blocking issues found.

[kans]

@manojacs - review by me, Fable, and a bunch of sub-agents. Since you started work on this, we landed a major refactor to the SDK that centralizes the declaration and handling of SDK invariants (referential & business consistency) for the new write paths that I'm working towards - ie, partial sync replays.

Task: Rebase manoj/incremental-grant-expansion onto main, integrate with ingestion

invariants, fix verified bugs (test-first), add a differential fuzz harness, harden,

then benchmark

Repo: baton-sdk (Go). Branch: manoj/incremental-grant-expansion (exists on origin).

Background

The branch implements diff-aware incremental grant expansion:

• The base sync's entitlement graph is persisted in the c1z as a sidecar blob
  (pkg/sync/expand/graph_blob.go, pkg/dotc1z/engine/pebble/entitlement_graph_sidecar.go),
  stamped with a sync ID that UnmarshalGraphBlob/GraphFromStore use to reject stale blobs.
• The Pebble compaction fold collects changed-entitlement IDs during its merge
  (pkg/synccompactor/pebble/merge.go).
• Compactor.expandGrantsIncremental (pkg/synccompactor/compactor.go) diffs current
  PendingExpansion edges against the base graph and runs
  IncrementalExpander.ExpandChanges (pkg/sync/expand/incremental.go) over only the
  affected subgraph. Cycles and revocation-shaped changes decline to full expansion.
• WithPreserveEntitlementGraph (pkg/sync/syncer.go) writes the graph to the sidecar
  at sync end (Pebble) or keeps it in the token (SQLite).

A four-way review verified the bugs below against the code. Work in stages, in order.
Stages 0 and 1 are required; Stages 2-4 in order as capacity allows — stop cleanly at a
stage boundary rather than half-finishing one. For EACH bug in Stage 1: first write a
test that FAILS and demonstrates the behavior, run it to confirm it fails for the right
reason, then fix, then confirm it passes. One commit per bug (test + fix together).

NOTE: file/line references below were taken against the pre-rebase branch; expect them
to shift after Stage 0. Verify each cited behavior in the rebased code before acting.

Stage 0: rebase onto main + ingestion-invariant integration (do this FIRST)

The branch is based ~11 commits behind main, before "Phase 3 - ingestion invariants"
(#1017) landed. The invariant machinery (pkg/sync/ingest_invariants.go,
runIngestionInvariants in syncer.Sync, the verification marker persisted after
EndSync) does not exist on the branch. Fixing bugs against the stale base would just
create a second merge problem, and the branch has an unexamined semantic conflict with
the invariant design.

0a. Rebase

• Rebase the branch onto current main (or merge main in, if the team prefers merge
  commits — check recent history for convention). Resolve conflicts.
• Danger zone: conflicts will likely cluster in pkg/sync/syncer.go (end-of-sync
  sequence now includes runIngestionInvariants at ~line 936 and
  persistIngestInvariantVerification after EndSync at ~line 982) and
  pkg/synccompactor/compactor_pebble.go (the fold now clears the inherited invariant
  marker at ~lines 684-688: SetIngestInvariantGeneration("") etc.).
• After rebase, before any other work: run
  go test ./pkg/sync/... ./pkg/synccompactor/... ./pkg/dotc1z/... and confirm green.
  (Slow: budget 20+ minutes.)

0b. Invariant integration for the incremental path (test-first)

Semantic conflict to resolve: main's design assumes grant expansion goes through the
syncer, which evaluates ingestion invariants (I3-I9) over the finished store and stamps
a verification marker after EndSync. compactPebbleFold clears the inherited marker
with the comment "expansion/invariant pass will write a fresh marker when one runs."
But expandGrantsIncremental bypasses the syncer entirely (resumes the ended sync,
writes grants directly, EndSync/Close), so on the rebased branch:

• Incremental compactions produce artifacts with NO invariant evaluation and NO
  verification marker (fail-closed: reads as unverified — a silent verification
  downgrade on every incremental compaction).
• The full-expansion path gets the compaction-merge invariant checks; the incremental
  path — the riskier, newer write path — gets none. In particular I8 (grant→entitlement
  referential integrity) is the exact class of damage an incremental-expansion bug
  produces.

Fix:

• Failing test first: run an incremental compaction (reuse the harness in
  pkg/synccompactor/incremental_expansion_test.go), open the output artifact, and
  assert the sync-run record carries a current-generation ingest-invariant verification
  marker (see c1zstore.IngestInvariantVerification, mode
  IngestInvariantVerificationModeCompactionMerge). Currently fails (no marker).
• Fix: after a successful incremental expansion, before/around finalization in
  expandGrantsIncremental / finishIncrementalExpansion, run the exported
  sync.RunIngestInvariants (see pkg/sync/ingest_invariants.go, ~line 408) over the
  compacted store with compaction-merge policy (mirror how the syncer builds
  IngestInvariantsPolicy for CompactionMerge mode, including FailFast wiring), then
  persist the verification marker AFTER EndSync — the marker must only be readable on a
  sealed sync (see persistIngestInvariantVerification for the sealed-write pattern).
• If an invariant hard-fails under fail-fast during an incremental run, treat it like
  other pre-finalization errors: restore the ended sync and fall back to full expansion
  (which will re-evaluate invariants through the syncer). Do NOT wrap invariant
  failures in the fatal sentinel unless the store teardown itself failed.
• Commit Stage 0 as its own commit(s): rebase separate from invariant integration.

Stage 1: verified bugs

Bug 1 (correctness, highest priority): dropped expansion sources are never detected

expandGrantsIncremental iterates current PendingExpansion and classifies each edge
against the base graph (new / widened / narrowed / unchanged), but never performs the
reverse check: a base-graph edge that is ABSENT from the current pending set. Nothing in
compactor.go iterates base.Edges.

Because the fold merge is newer-wins, an increment can replace a grant carrying
GrantExpandable{entitlement_ids:[A,B]} with one carrying [A]. The dead B→C edge
survives in the cloned base graph, is persisted to the sidecar via persistGraphSidecar,
and recomputeDestination walks ALL incoming graph edges — so a later increment that adds
a member to B expands grants onto C through an edge that no longer exists. Full expansion
(which rebuilds the graph from pending) would not produce those grants: incremental ≠ full.

Note: the doc comment on ErrIncrementalRevocationDecline
(pkg/sync/expand/incremental.go) already claims "a source dropped" is covered. It is not.

• Failing test: in pkg/synccompactor/incremental_expansion_test.go, base sync has an
  expandable grant with entitlement_ids:[A,B] targeting C; increment replaces it with
  [A] and adds a member to B; assert the compaction output's grants on C match a full
  expansion of the same inputs (they currently won't — the incremental path writes extra
  grants via the ghost edge). Also assert the incremental path declined
  (incrementalExpansionRan == false) once fixed. If parity assertion is awkward in the
  harness, acceptable fallback: assert the decline sentinel fires and no grants on C are
  sourced via B.
• Fix: after collecting the pending (src,dst) edge set, iterate base.Edges (skip
  same-node/collapsed pairs); if any base edge is missing from pending, return
  expand.ErrIncrementalRevocationDecline (restore the ended sync first, matching the
  other decline paths).

Bug 2 (soundness): persisted graph violates its own invariants after incremental success

AddEdge (pkg/sync/expand/graph.go) creates edges with IsExpanded: false and sets
g.HasNoCycles = false. ExpandChanges never marks edges expanded and never restores
HasNoCycles after its cycle check passes. So a successful incremental run persists a
sidecar graph that is semantically expanded but reports IsExpanded() == false — and
Expander.IsDone() (pkg/sync/expand/expander.go) delegates directly to
graph.IsExpanded(). The documented precondition on IncrementalExpander says the graph
is "a prior completed expansion's graph (edges already expanded)"; the artifact this code
writes breaks that precondition for the NEXT run.

• Failing test: run an incremental compaction that adds a new edge, reload the sidecar
  graph from the output artifact (sync.GraphFromStore), assert graph.IsExpanded() is
  true and HasNoCycles is true. Currently fails.
• Fix: in ExpandChanges after the walk succeeds (or in the compactor just before
  persistGraphSidecar), mark all edges expanded and set HasNoCycles = true (the cycle
  check already passed). Mirror what markExpansionComplete does for the full expander.

Bug 3 (soundness): fold orphans the graph sidecar under the new sync ID

compactPebbleFold (pkg/synccompactor/compactor_pebble.go) mints newSyncID, rewrites
the sync-run record AND recomputes the stats sidecar under the new ID (and, post-rebase,
clears the inherited invariant marker) — but never touches the entitlement-graph sidecar,
which remains stamped with the base sync's ID. The delete-inherited-sidecar logic exists
only inside expandGrants; the skip-expansion path in doOneCompaction
(c.skipGrantExpansion or partial sync type) closes and publishes without ever reaching
it. Result: the published artifact carries a stale blob; GraphFromStore(ctx, store, newSyncID) returns nil and the incremental chain silently breaks (falls back to full
forever).

• Failing test: fold-compact a base that has a graph sidecar, with skip-expansion enabled;
  open the output and assert either (a) GraphFromStore with the new sync ID returns a
  usable graph, or (b) the raw sidecar blob is absent — i.e. never a blob stamped with the
  old ID. Currently the stale base-ID blob is present.
• Fix: in compactPebbleFold, after minting newSyncID, handle the graph sidecar the
  same way the stats sidecar and (post-rebase) the invariant marker are handled: either
  rewrite the envelope's sync ID (keeping the base graph available as the incremental
  base) or delete the blob. Deleting is the simpler, safe choice; rewriting is better for
  chain continuity — prefer rewriting only if you can do it without unmarshaling in the
  engine layer (the blob format is owned by pkg/sync/expand, the engine treats it as
  opaque — respect that layering).

Bug 4 (minor, no test needed): fix the WithPreserveEntitlementGraph godoc

pkg/sync/syncer.go: the godoc says the option "keeps the entitlement graph in the final
sync token instead of clearing it". On Pebble the actual behavior writes the graph to the
c1z sidecar and CLEARS it from the token; only SQLite keeps it in the token. Also update
WithIncrementalExpansion's doc in pkg/synccompactor/compactor.go, which directs
callers to sync.GraphFromToken — the primary loader is sync.GraphFromStore;
GraphFromToken is the SQLite/legacy fallback.

Stage 2: differential fuzz harness (incremental vs. full expansion parity)

The ghost-edge bug shows the scenario space (edge specs × membership deltas × annotation
replacements × cycles) is too combinatorial for hand-written cases. Build a differential
property test. The oracle is built in: for any base state + delta sequence, incremental
expansion must produce exactly the same grants as full expansion, OR decline (declining
IS full expansion, so parity holds trivially).

Two levels, weighted toward the cheap one:

2a. Fast loop in pkg/sync/expand (the main harness)

• Use pgregory.net/rapid (preferred, has shrinking; add to go.mod) or a seeded
  math/rand scenario generator with the seed logged on failure.
• Generate: a universe of ~5-30 entitlements and ~10-100 principals; a base state of
  direct grants and expandable edges (random shallow/deep, random resource-type filters,
  some multi-source annotations, occasional cycles); run FULL expansion over an in-memory
  ExpanderStore to establish the base (grants + graph). Follow the existing in-memory
  store patterns in pkg/sync/expand/incremental_test.go.
• Generate a delta script of 1-10 random operations: add member, remove member, replace
  an expandable annotation with a subset/superset of entitlement_ids, flip shallow/deep,
  tighten/widen a resource-type filter, add a brand-new edge, add a cycle-closing edge.
• Apply deltas to a copy of the store; compute newEdges/changedEntitlementIDs the same way
  the compactor does (including the Bug-1 reverse check once fixed); run
  IncrementalExpander.ExpandChanges against the base graph clone.
• Reference: run full expansion from scratch over the post-delta store.
• Assert, when the incremental path did NOT decline:
  1. Grant-set parity: same (entitlement, principal) pairs, same sources maps, same
     directness flags. Sort and diff; print the seed and the minimal delta script on
     failure.
  2. The post-run graph satisfies IsExpanded() and HasNoCycles, and survives a
     MarshalGraphBlob/UnmarshalGraphBlob round trip.
• Track the decline rate across the run and assert a healthy fraction (e.g. >30%) of
  cases actually exercised the incremental path (incrementalExpansionRan-equivalent).
  Without this, a regression that makes everything decline passes the fuzzer while
  silently killing the feature.
• Wire it as a standard go test with a bounded case count (e.g. 200 cases) so it runs
  in CI in seconds, plus an opt-in env var / flag for a long run (e.g. 50k cases).

2b. Thin end-to-end differential test at the compactor level

• One test in pkg/synccompactor that generates a handful (~10, seeded) of random
  base+increment c1z pairs, compacts each twice — once with WithIncrementalExpansion,
  once without — and asserts grant-set parity between the two outputs plus a valid
  sidecar graph under the output sync ID, plus (post-Stage-0) a current-generation
  invariant verification marker. This covers what 2a can't see: the fold's changed-ID
  collection, sidecar lifecycle, invariant marker, and decline plumbing. Keep the case
  count low; Pebble compactions are seconds each.

Stage 3: hardening and design follow-ups

Items 3.1-3.4 are implementation work. Items 3.5-3.6 are design decisions: produce a
short written proposal (in the PR description or a doc comment), do NOT implement
unilaterally.

3.1. Version the sidecar blob format (small, do it now)

graphBlobEnvelope (pkg/sync/expand/graph_blob.go) is JSON with a sync ID but no
format version. Go's JSON silently zero-fills missing or renamed fields, so the first
refactor of EntitlementGraph's serialized shape will successfully parse old blobs into
wrong graphs — no error, just corrupt incremental bases. Add a format_version (or a
generation string mirroring IngestInvariantGeneration) to the envelope; on mismatch,
UnmarshalGraphBlob returns (nil, nil) — same treatment as a sync-ID mismatch, so the
chain falls back to full expansion and heals. Test: a blob with a stale version is
rejected; a current one round-trips.

3.2. Artifact fsck: one validator for sealed-artifact invariants

Several Stage 1 bugs are one bug: a sealed c1z can violate its own self-description.
Write a single validation helper (suggested: a new file in pkg/synccompactor or a
shared testutil package) that, given a sealed store, asserts:

• If a graph sidecar exists, its envelope sync ID matches the store's sync-run record,
  its format version is current, and the graph passes IsExpanded(), HasNoCycles,
  and a marshal/unmarshal round trip.
• The ingest-invariant verification marker is present with the current generation
  (for artifacts produced by paths that promise verification).
• Sanity: sync-run record is sealed/ended.
  Call this validator at the end of every compactor test that produces an artifact (both
  incremental and full paths) and from the Stage 2b harness. This retires the category
  instead of pinning one regression test per lifecycle bug.

3.3. Instrument the fallback funnel

The design degrades to full expansion silently: an orphaned sidecar, chronic decline, or
marshal error produces zero errors and zero incremental runs, forever — the feature can
be dead in production with no signal. The only current observability is the test-only
incrementalExpansionRan bool. Add structured logging (and metrics if the compactor has
a metrics handle — check pkg/metrics conventions) recording per compaction: incremental
attempted / succeeded / declined (with reason: cycle, revocation-shaped, dropped-edge) /
fell back (with error class) / not attempted (no base graph, engine, skip), plus
grants written and entitlements walked on success. Keep field names stable — these will
be dashboarded. Replace test usage of the bool with these outcomes if straightforward;
otherwise leave the bool.

3.4. Crash/resume schedules in the fuzz harness

The checkpoint-focused review found ordering windows by hand (sidecar durable before
seal, Sync-vs-NoSync writes, fold rename). Extend the Stage 2 harness with interrupt
schedules: cancel the context / simulate failure at randomized seams (after sidecar
write, mid-ExpandChanges, before EndSync), then retry/resume the operation from its
inputs, and run the 3.2 fsck plus the parity oracle on the final artifact. Follow the
existing interrupt-test patterns (pkg/sync/expand/topological_merge_layer_interrupt_test.go,
pkg/dotc1z/engine/pebble/endsync_concurrent_writers_test.go). Assert the retried run
converges: no duplicate grants, no stale sidecar, no artifact published from a failed
attempt.

3.5. PROPOSAL ONLY: make the invariant seam structural

Stage 0b wires invariants into the incremental path by convention — the next new write
path can forget again, which is the exact failure mode the Phase 3 design warns about
("any second ingestion path turns every stream-coupled side effect into a latent bug").
Propose a structural enforcement: e.g. a store that is resumed-and-written after seal
cannot re-seal without either a fresh verification pass or an explicit unverified stamp.
Sketch the API change (likely in c1zstore / the engine's EndSync path), its blast
radius, and migration. Do not implement.

3.6. PROPOSAL ONLY: decide the deletions contract

Additions-only is load-bearing everywhere: the decline sentinels, the fuzz oracle, the
seed semantics of changedEntitlementIDs, and the Bug 1 fix shape. The code carries a
named hook (ErrIncrementalRevocationDecline) for a future tombstone/deletion stage.
Write a short decision memo: (a) if deletions are coming, what the oracle, fsck
invariants, and Bug-1-style reverse diff need to look like to accommodate them; (b) if
additions-only is permanent, which aspirational comments to delete and where to document
the contract (package doc of pkg/sync/expand). Recommend one. Do not implement either.

Stage 4: CPU/memory benchmarks + easy performance wins

The feature exists for performance, but the branch has no benchmarks: nothing proves the
incremental win, locates the crossover point vs full expansion, or measures what a FAILED
attempt costs (a tenant with chronic declines pays the incremental overhead on every
compaction and then runs full expansion anyway).

4a. Benchmarks (write these first, before touching any code)

Go benchmarks with b.ReportAllocs(); capture peak heap via runtime.ReadMemStats
deltas (or -benchmem plus a peak-tracking allocator shim if the repo has one). Compare
runs with benchstat. Parameterize by scale; suggested grid: entitlements E in
{1k, 10k, 100k}, principals P in {10k, 100k}, edges ~2x E, delta size K in {1, 100, 10k}.

Happy path (fast, in pkg/sync/expand against the in-memory store from Stage 2a):

• BenchmarkIncrementalExpand/E=..,K=.. — ExpandChanges on a K-sized delta.
• BenchmarkFullExpand/E=.. — full expansion of the same post-delta state.
• Report the crossover: at what K/E ratio does incremental stop winning? Put the numbers
  in the PR description.
• BenchmarkGraphClone/E=.. and BenchmarkMarshalGraphBlob/E=.. — the per-attempt
  fixed costs.

Sad paths:

• BenchmarkIncrementalDecline/E=.. — an attempt that declines (e.g. a narrowed edge)
  measured END-TO-END including clone, pending scan, cycle check, and restore: this is
  pure overhead added to every full expansion for a declining tenant. This number is
  the feature's worst-case regression; it must be small relative to full expansion.
• BenchmarkIncrementalFallbackCompaction — one compactor-level (Pebble) benchmark:
  compaction with incremental-requested-but-declined vs compaction without incremental.
  Keep it coarse (single scale) — it's seconds per iteration.

One e2e happy-path compactor benchmark at a single realistic scale (base + small
increment) for wall-clock and RSS sanity; the fine-grained work stays at the expand level.

4b. Easily pickable fruit (only with benchmark evidence)

Rules: only local, semantics-preserving changes; each must show a measurable win in 4a
benchmarks; re-run the Stage 2 fuzz harness after each; one commit per change with the
benchstat delta in the commit message. Known candidates from review (verify against the
rebased code):

1. Defer/avoid the whale clone on decline paths. expandGrantsIncremental clones the
   entire base graph via JSON round-trip BEFORE the pending scan and spec classification,
   but those checks only read the graph. Restructure so the clone (or the mutation phase)
   happens only after the decline checks pass. For a chronically-declining tenant this
   removes the entire clone cost from every compaction.
2. Replace Clone-by-JSON with a structural deep copy (maps/slices copied directly).
   The quality review flagged it; justify with the BenchmarkGraphClone numbers. Keep
   reinitMaps for the unmarshal path.
3. Memoize getEntitlement within one ExpandChanges run. recomputeDestination
   re-fetches the same source entitlement for every destination that shares it; a
   per-run map[string]*v2.Entitlement caps lookups at one per distinct entitlement.
   (Bound it or accept the graph-sized worst case — it's one proto per entitlement.)
4. Skip topologicalNodeOrder over the WHOLE graph when the affected set is tiny:
   order only the affected subgraph, or early-out when len(affected) is small relative
   to the graph. Only do this if the benchmark shows the full-graph sort matters at
   E=100k with K=1.
5. IncrementalResult.EntitlementsWalked accumulates every walked entitlement ID but
   callers only log its length — replace with a count (check all callers first).

Do NOT chase anything beyond these without flagging it — deeper changes (e.g. altering
flush chunking, store read paths) belong in the planned quality pass.

Constraints

• Do not restructure the error protocol or refactor expandGrantsIncremental beyond what
  the fixes require — a separate quality pass is planned. (Stage 4b item 1 is the one
  sanctioned exception: moving the clone is allowed.)
• Follow existing test patterns in pkg/synccompactor/incremental_expansion_test.go and
  pkg/sync/expand/incremental_test.go.
• After all work: run go test ./pkg/sync/... ./pkg/synccompactor/... ./pkg/dotc1z/...
  (slow, 20+ min) and make lint. All must pass. If the fuzz harness finds NEW
  divergences beyond the bugs above, do not paper over them: minimize the repro, add it
  as a named regression test, and report it — fix it only if the fix is small and
  obvious.
• This is a public repo: no customer names or production identifiers in code, comments,
  or commit messages.

[kans]

Another random thought. We are accumulating fuzzers - it would be nice to have a make fuzzball target that runs all of them for a bit