Skip to content

fix(kv-unified): stale presentation receipt made the label count superlinear (#97) - #98

Merged
Anarchid merged 5 commits into
mainfrom
fix/kv-unified-stale-receipt
Sep 21, 2026
Merged

Anarchid merged 5 commits into
mainfrom
fix/kv-unified-stale-receipt

Conversation

@Anarchid

@Anarchid Anarchid commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #97. Companion context: #95, #96. Follow-ups that this PR does not address: #105, #107.

What breaks

A kvunified:presentation-receipt survives folding-strategy switches. After kv-stable has presented from a store for a while, every leaf folded since is "extension" for kv-unified, and extensionTokens was keyed exactly in the Pareto label state while dominates() never looked at it, so labels differing only in their extension total never collapsed. On the Lynx Knowledge Resident (241 chunks / 318 summaries, receipt six days old) the first compile after switching back threw exact label propagation exceeded ceiling 100000 at 125796, and was still climbing at 1.64M labels / 5.3 GB under a 1M ceiling. Same store with the receipt removed: 133 ms.

Fix (as of round 5, bd94014)

  1. kv-unified-pareto.ts: extension tokens stay on the label but leave the state key. While a provider cache is relevant they are a fourth dominance dimension (the cache term prices them: more extension = less avoidable recompute); per-bucket representatives keep the max-extension label, and whatever extension a covered label had beyond its representative goes into a cache component of the approximation envelope, reported as approximationCacheErrorBound and included in the score bound. With no relevant cache, extension is inert.
  2. autobiographical.ts: the first successful, non-dry-run, adaptive presentation by a non-kv-unified folding strategy nulls the persisted receipt, once, with one warning line, at the same commit point as the carried frontier (after every refusal-capable stage). Loading only notes that a receipt exists; it writes and registers nothing. A later switch back starts from an empty chain: one cold-cache turn, which the migration runbook already expects, instead of a permanent hard-down. If the nulling write throws, the pending flag stays raised and the next compile retries.

Limits

Evidence

Synthetic 28-chunk chain forest (buildChronicleWithChain, buckets 100/100/100), no relevant cache, labels created:

presentation before after
fresh (covers all leaves) 23,630 23,630
half-stale (covers older half) 178,797 37,460
fully stale (covers none) 78,031 3,642

The residual 1.6× for half-stale is continuity-loss bucketing, not extension keys.

DAG engine with a relevant cache, half-stale ÷ fresh at 24/28/32 chunks: 1.44 / 1.56 / 1.50 here (flat) versus 3.1 / 5.2 / 8.3 on main (growing). Bucketed vs all-zero-bucket solves agree (score gap 0, no bound violation) across 48 cache-relevant configurations.

Production store (16:11Z snapshot of the incident session, stale receipt left in place), 100k ceiling, measured with no relevant provider cache (the harness passed no kvUnifiedImmutablePrefixHash): COMPILE OK in 332 ms / 436 MB. With the stale receipt's own prefix hash supplied, the same solve is ~15 s / 8–9.5 GB because of #105.

Tests

  • kv-unified-policy.test.ts:
    • no-cache growth bound (half-stale ≤ 2× fresh, fully stale ≤ fresh);
    • the reviewer's 3-chunk case against the recursive oracle ([1,1,0], bucketed and unbucketed), which fails if extension is dropped from dominance;
    • relevant-cache growth bound: half-stale ≤ 2× fresh, both under the cache (fails on main's solver, 91,356 vs 29,620), plus the bucketed selection's regret against an all-zero-bucket solve ≤ the reported score bound.
  • kv-unified-strategy-integration.test.ts:
    • a kv-stable load and a dryRun compile leave the receipt intact with zero warnings; the first real kv-stable compile supersedes with exactly one warning; a second does not warn again; switching back finds an empty chain;
    • a kv-stable compile that throws OverBudgetError keeps the receipt;
    • an injected failure of the nulling write is retried by the next compile (fails with the old flag order).
  • Full suite on a clean build: 802/802.

🤖 Generated with Claude Code

…rlinear

A `kvunified:presentation-receipt` persists across folding-strategy
switches. After kv-stable has presented from the store for a while,
every leaf folded since is "extension" for kv-unified, and
`extensionTokens` was keyed exactly in the Pareto label state while
`dominates()` never looks at it, so labels differing only in their
extension total never collapsed. On the Lynx Knowledge Resident's store
(241 chunks, 318 summaries, receipt from six days earlier) the first
compile after switching back threw "exact label propagation exceeded
ceiling 100000 at 125796" and was still climbing at 1.64M labels / 5.3 GB
under a 1M ceiling; with the receipt removed the same store solved in
133 ms. Half-covering receipts are the worst case: on a 28-chunk synthetic
forest, 178,797 labels vs 23,630 fresh.

Two changes:

- `stateKey` keys extension tokens at token-bucket resolution, and omits
  them when no provider cache is relevant — they feed only
  `cacheChurn()`'s avoidable-recompute term, which is unpriced then.
  Exact mode (`tokenBucketSize: 0`) keeps them exact. Same synthetic
  forest after: 37,460 half-stale (1.6× fresh, the rest is continuity
  bucketing), 3,642 fully stale.
- `loadPersistedState` supersedes a persisted receipt when the folding
  strategy is not kv-unified (the state is now registered for every
  strategy), with one warning line. A later switch back starts from an
  empty chain: one cold-cache turn, which the migration runbook already
  expects, instead of a hard-down.

The production store with its stale receipt now compiles in 332 ms /
423 MB under the original 100k ceiling. Tests: bounded-growth assertion
on the synthetic forest, exact keys retained under a relevant cache, and
an integration test that a kv-stable restart clears the receipt.

Fixes #97.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

@Anarchid Anarchid left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 CLEAR

Reviewer: Codex (GPT-5.6 Sol)

Reviewed head: c2691d56982a40fa37553098c0557da5fecb8ddc

Findings

No material findings.

The two halves of the fix line up with the failure mode: extension totals no longer create exact-key cardinality when they are unpriced, while a relevant provider cache preserves that dimension at the configured token-bucket resolution; separately, loading any non-kv-unified strategy clears the old accepted-presentation chain before a later kv-unified restart can mistake it for the preceding turn. The receipt slot is registered for every folding strategy, the reset is durable, and the integration path proves that switching back observes an empty chain.

Tooling results

  • git diff --check origin/main...HEAD — passed.
  • npm ci --offline --ignore-scripts — passed from the exact lockfile; 6 packages installed, 0 audit findings.
  • npx tsc --noEmit — passed.
  • npm run build — passed.
  • Direct execution of dist/test/adaptive/kv-unified-policy.test.js — 20/20 passed, including the stale-presentation label bound and cache-relevant extension path.
  • Direct execution of dist/test/kv-unified-strategy-integration.test.js — 8/8 passed, including the durable strategy-switch reset.
  • A deterministic exact-oracle comparison over 1,164 feasible small stale-prefix/cache-relevant cases found no a-posteriori score-bound violation.
  • npm test — 94/96 test files passed. The two failures are unchanged subprocess-output suites (release-changelog.test.js and repair-pyramid-keepers.test.js) whose nested child processes expose no stdout/stderr in this sandbox; neither touches this diff.
  • GitHub checks at the final refresh — all five green across Ubuntu/macOS and Node 20/24, plus changelog validation.

Verdict

No blocker or non-blocking follow-up surfaced in the changed policy, persistence lifecycle, or immediate receipt/cache dependencies. Local confidence is limited only by the two known sandbox-blocked subprocess suites; exact-head CI supplies the missing platform evidence.

— Reviewed by GPT-5.6 Sol via OpenAI Codex.

@Tengro Tengro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Severity: 🟠 CONCERNING

Reviewer: Claude Fable 5.1 (Claude Code), reviewed head c2691d56, base 0e2302ff. Merges cleanly onto current main (#101 landed since).

The diagnosis in #97 is excellent and the fix stops the bleeding. But one half of it is a half-measure guarded by a test that tests nothing, and the other half is a destructive write on a read path that already breaks a sibling PR sitting in the same queue.

Findings

🟠 src/adaptive/kv-unified-pareto.ts:789 — extension is kept as a state dimension that nothing prices

const extensionKey = !cacheRelevant
  ? 'e*'
  : tokenBucketSize > 0
    ? Math.ceil(label.extensionTokens / tokenBucketSize)
    : label.extensionTokens;

The comment above it says extension tokens "distinguish labels only while a provider cache is relevant". I grepped. label.extensionTokens is written in emit() and flushPendingEmissions() and read in exactly one place: this key. dominates() does not look at it. Nothing in the solver's welfare, final pick, or tiebreak (line 907 sorts on matchedUnits) reads it. The cache-churn term the comment refers to lives in kv-unified-policy.ts:305, and it recomputes extension from the layout via this.extensionTokens(layout, presentation), not from the label.

So when the cache is relevant, the state key still splits labels on a quantity that neither dominance nor pricing ever consults. That is the same defect as before, just divided by the bucket size. A resident with a relevant cache and a half-stale receipt gets the "half-stale" row's blowup again, scaled by 1/100. The issue's own table shows the blowup is multiplicative with forest size, so a bucket is a delay, not a bound.

Fix: pick one. Either extension is a Pareto dimension, in which case dominates() must consider it and the key needs it at every relevance; or it isn't, in which case delete it from the key entirely and the cacheRelevant parameter goes away. Given nothing reads it, the honest version is:

return [
  label.remaining.toString(16),
  tokenKey,
  label.cache.intact ? 1 : 0,
  label.cache.matchedUnits,
  label.cache.cachedTokens,
  ...

and keep extensionTokens on the label only if something is going to consume it for diagnostics. Otherwise remove the field too.

🟠 src/strategies/autobiographical.ts:2074 — the receipt is destroyed on open, not on present

const receiptState = this.store.getStateJson(this.kvUnifiedReceiptStateId);
if (receiptState && typeof receiptState === 'object') {
  this.store.setStateJson(this.kvUnifiedReceiptStateId, null);

The comment justifies this with "once another folding strategy presents from this store". The code does it in loadPersistedState, which runs on every ContextManager.open and on the coalesced-compression reload at line 5662. Opening a store with a non-kv-unified strategy to inspect it now erases the only record of the last accepted kv-unified presentation, with a console.warn as the sole trace.

This is not hypothetical. The folding migrator in draft #94 opens the store with a default adaptive strategy in both validate and to-unified to read the flavor slot. With this PR's source, its test to-unified refuses an existing chain unless overwrite is set fails:

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
true !== false
[autobiographical] superseded a persisted kv-unified presentation receipt: folding strategy is now flat-profile; ...

The refuse-without---overwrite guard never fires because the chain was nulled during open(), before readFlavor ran. A documented dry-run became a write. Every other tool that opens a store read-only with a recipe-default strategy (conhost's web UI, a preview, a misconfigured recipe) gets the same behavior.

Fix: supersede where the comment says: at the first commit by a non-kv-unified presenter. persistResolutions() at line 2100 is the shared commit path both strategies use, already guarded by requireBranchMutation, and it runs exactly when another strategy has actually presented. Move the null-and-warn there, keyed on this.config.foldingStrategy !== 'kv-unified'. Alternatively stamp instead of delete ({ supersededBy, supersededAt }) so the switch-back can decide. Either way, loadPersistedState should stay a load.

Related: the new write also skips requireBranchMutation, which every other persist* in this class calls. There is precedent for that in the merge-queue repair a few lines up, so I will only sigh about it.

🟡 test/adaptive/kv-unified-policy.test.ts:733 — a test named for a property it does not check

test('kv-unified keeps exact extension keys when a relevant cache is priced', () => {
  ...
  assert.equal(result.feasible, true);
  assert.ok(result.selected.renderedTokens <= 250);
  assert.ok((result.propagation?.labelsCreated ?? 0) > 0);
});

Feasible, under budget, made at least one label. This passes with the extension key deleted, bucketed, exact, or replaced with Math.random(). It is the only test covering the cacheRelevant === true branch and it constrains nothing. Either assert a label-count difference between two extension-distinct configurations or, per the first finding, delete the branch and the test together.

🟡 test/kv-unified-strategy-integration.test.ts:259 — the PR body claims more than the test asserts

The description says the integration test proves "the warning fires once". The test asserts head === null and leaves.size === 0. No console.warn capture, no once-ness. The claim should match the test, or the test should match the claim.

🟡 src/strategies/autobiographical.ts:1841 — registration side effect on every store

Registering the receipt slot for every folding strategy is required by the chosen design, but it means the first open of every existing non-kv-unified store after this release appends a state registration record. Benign, and it goes away if the supersede moves to the commit path and stays conditional on the slot already existing. Noting it so nobody is surprised by a new slot in stores that never ran kv-unified.

Tooling results

Check Result
npx tsc --noEmit clean, exit 0
npm run build clean, exit 0
npm test (clean dist/) 762 tests, 762 pass, 0 fail, 14.9 s
git diff --check clean
Merge onto current upstream/main clean

One honest caveat about the numbers: my first run reported 772 tests with 1 failure. The extra 10 were stale dist/test/folding-migration.test.js from the #94 branch checked out earlier in the same tree, which tsc never deletes. That is an artifact of my environment, not the PR. It is also how I found the second finding, so I am not complaining.

Verdict

The investigation behind this PR is the best part of it: the label-cardinality argument in #97 is precise, reproduced synthetically, and confirmed on the production snapshot. The pareto change does kill the incident. But it keeps an unpriced dimension in the key with a comment that misdescribes the pricing, guarded by a test that would pass for any implementation. And the receipt supersede is wired to the wrong event: a load, not a presentation, which already breaks the dry-run guarantee of the migrator in #94. Both fixes are small. Drop extension from the key outright, move the supersede to the resolutions commit path, and replace the vacuous test. Then this is a 🟢.

@Tengro

Tengro commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Clarification on the review above, so #94 does not read as a blocker for this PR.

Having re-checked the migrator (#94) against this fix: #98 makes most of it redundant, not the other way round. to-stable existed only to clear the receipt slot before a flip back, and this PR does that automatically; it will be dropped. to-unified goes from required to an optional warm start (it only saves the one cold-cache turn and presentation jump the runbook already accepts). validate is orthogonal (strict-forest go/no-go per treeification policy) and unaffected either way. So #94 will be rebased on top of whatever shape #98 lands in, and its remaining read paths can simply open with foldingStrategy: 'kv-unified' if the load-time supersede stays.

The second 🟠 finding therefore stands on its own merits rather than on #94: loadPersistedState is a read path that now performs a durable, warning-only delete on every open by a non-kv-unified strategy (inspection tools, previews, a misconfigured recipe). The migrator test was just the first thing to trip over it. Moving the supersede to the resolutions commit path (or stamping instead of deleting) is still my recommendation, but I would not hold the PR on #94's account. The first 🟠 (extension kept as an unpriced key dimension) and the vacuous cache-relevant test are unchanged.

…esent, not on load

Review follow-up on #98 (Tengro):

- Extension tokens leave the Pareto label entirely. The previous revision
  kept them in the state key at bucket resolution while a cache was
  relevant, but nothing reads them from the label: dominates() ignores
  them and the cache-churn term recomputes extension from the rendered
  layout. A bucket was a delay, not a bound. emit()/assignRawRun() lose
  the extension argument, isExtension() goes with it, and stateKey() no
  longer takes a relevance flag.
- The receipt is superseded by a presentation, not by a load.
  loadPersistedState() only notes (read-only, via listStates + a null
  check, no registration side effect) that a receipt exists under a
  non-kv-unified strategy; the first non-dry-run select by that strategy
  nulls it, under requireBranchMutation, with one warning. Opening a
  store to inspect, preview or dry-run it leaves the receipt intact.
  Hooked on the presentation commit rather than persistResolutions()
  because that only runs when a resolution changed, and a no-op compile
  is still a presentation.
- The vacuous cache-relevant test is gone with the branch it covered.
  The integration test now captures console.warn and asserts: a kv-stable
  load does not supersede and the receipt survives it; the first kv-stable
  compile supersedes exactly once; a second compile does not warn again;
  switching back finds an empty chain.

Full suite 761/761. Real store with its stale receipt: 309 ms / 434 MB
under the original 100k ceiling.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@Anarchid

Copy link
Copy Markdown
Contributor Author

Round 2 at 45f9b21, one finding at a time:

  • Extension as an unpriced key dimension (🟠): agreed, and you were right that a bucket only divides the blowup. Extension tokens are now gone from the label and the key entirely; emit()/assignRawRun() lose the argument and isExtension() is deleted. The comment now says what is true: the cache-churn term recomputes extension from the rendered layout, so the label never needed it.
  • Supersede on open (🟠): moved to the presentation. loadPersistedState() only notes, read-only (listStates() + null check, no registration), that a receipt exists under a non-kv-unified strategy; the first non-dry-run select by that strategy nulls it under requireBranchMutation, once. I hooked it on the presentation commit in selectAdaptive rather than inside persistResolutions(), because that only runs when a resolution actually changed, and a no-op compile is still a presentation (the integration test's first kv-stable compile is exactly that case). Your feat: folding-strategy migration utility (kv-stable ⇄ kv-unified) #94 validate/to-unified read paths should now be safe with any strategy; I did not test feat: folding-strategy migration utility (kv-stable ⇄ kv-unified) #94 itself.
  • Vacuous cache-relevant test (🟡): deleted with the branch it covered.
  • "Warning fires once" (🟡): the integration test now captures console.warn and asserts 0 after a kv-stable load (receipt verified intact by a kv-unified reopen), 1 after the first kv-stable compile, still 1 after a second, and an empty chain after switching back.
  • Registration on every store (🟡): gone; registration is kv-unified-only again, and the supersede writes to a slot that already exists.

Full suite 761/761 (one fewer: the deleted test). Real store with the stale receipt: 309 ms / 434 MB under the 100k ceiling, cgroup-capped this time.

@Anarchid
Anarchid requested a review from Tengro September 17, 2026 13:06
@antra-tess

Copy link
Copy Markdown
Contributor

Reviewed head 45f9b21140bc0eeec9e7068a7491a562a9fff3db. I recommend changes for two issues:

  1. [P1] Removing extension tokens makes pruning discard optimal layouts — kv-unified-pareto.ts:771–776.

    Cache pricing still subtracts extension tokens from recomputation cost. Labels with identical rendered-token totals and cache state can therefore have different final cache costs. Recomputing that cost from the layout afterward cannot recover a candidate already discarded by dominance.

    I reproduced this with three chronological chunks a, b, c, each 100 raw tokens with its own 20-token L1 summary; saliences are 0.2, 0.4, 0.2. The accepted presentation and relevant cache cover raw a and b, with a marker after b; c is extension. Constrain a to L1 and use a 140-token budget, buckets 100/100/100, and policy overrides alpha: 0, budgetUnderLambda: 0, budgetOverLambda: 0, continuityLambda: 0, cacheLambda: 10000, cacheScale: 100.

    The base commit and exhaustive oracle select levels [1, 1, 0], score 60. This revision selects [1, 1, 1], score 80, unnecessarily folding the new chunk while reporting zero approximation error. The optimal candidate is pruned in favor of [1, 0, 1], which has lower fidelity/continuity losses but higher cache churn (138 versus 46). A separate reproduction with all bucket sizes zero also disagrees with the exhaustive oracle and reports zero approximation error.

    Preserve cache-cost information in dominance/pruning and account for any approximation of that information in the error bound. The new label-growth test has no relevant cache, so it cannot detect this regression.

  2. [P2] Failed compiles erase the accepted receipt — autobiographical.ts:7806–7807.

    Superseding runs before the hard-budget check and rendering, both of which can throw. I reproduced this by accepting a kv-unified presentation, reopening with kv-stable, adding unsummarized messages, and compiling with an insufficient budget. The code logs that kv-stable "has presented," clears the receipt, then throws OverBudgetError (1,426 planned tokens versus a 306-token hard budget). Reopening with kv-unified finds an empty chain even though no replacement presentation succeeded.

    Move cleanup after successful selection/rendering and add a regression test asserting that a failed compile preserves the receipt.

Validation: npm run build, git diff --check, and all 761 existing tests pass. Both findings were reproduced separately against the reviewed head; the solver reproduction was also compared with the base implementation and exhaustive oracle.

— Reviewed by Codex.

…; supersede only after a successful present

Review follow-up on #98 (antra-tess):

- P1: dropping extension from the label lost information the final cache
  term still prices (avoidable recompute = recomputed - extension), so
  dominance could discard the optimal cut under a relevant cache. The
  three-chunk reproduction from the review ([1,1,0] score 60 vs the
  round-2 pick [1,1,1] score 80, with a reported zero error) is now a
  test against the exhaustive oracle, bucketed and exact. Extension is
  back on the label; while a cache is relevant it is a fourth dominance
  dimension (more extension = less avoidable recompute), the bucket-group
  representatives keep the max-extension label, and any extension a
  covered label had beyond its representative goes into a new cache
  component of the approximation envelope, which the score error bound
  prices at the cache slope and reports as approximationCacheErrorBound.
  It stays out of the state key, which is what made the count superlinear.
- P2: the supersede ran before the hard-budget check and emission, so a
  failed kv-stable compile erased the receipt without presenting. Moved
  to the end of the successful select path, after rsEnd(); a regression
  test accepts a kv-unified presentation, over-budgets a kv-stable
  compile (OverBudgetError), and finds the receipt intact on switching
  back.
- A cache-relevant variant of the label-growth bound, so the regression
  the review pointed at (the growth test had no relevant cache) is
  covered.

All three new tests fail on the round-2 source and pass here. Full suite
764/764. Real store with its stale receipt: 332 ms / 436 MB under the
original 100k ceiling.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@Anarchid

Copy link
Copy Markdown
Contributor Author

Round 3 at 32f8a25. Both findings reproduced and fixed, thank you for the exact case.

  • P1 (extension dropped from dominance): you were right, and it also shows Tengro's round-2 reading and mine were wrong in the same way: nothing read label.extensionTokens, but the quantity is priced at the end. Extension is back on the label, out of the key. While a cache is relevant it is a fourth dominance dimension (more extension = less avoidable recompute), the per-bucket representatives keep the max-extension label, and any extension a covered label had beyond its representative goes into a new cache component of the approximation envelope, priced at the cache slope in the score bound and reported as approximationCacheErrorBound. Your 3-chunk case is now a test against the recursive oracle: [1,1,0], same score, bucketed 100/100/100 and all-zero buckets, with the selected-minus-oracle regret ≤ the reported bound. It fails on the round-2 source. There is also a cache-relevant variant of the label-growth bound, since as you noted the existing one could not see this.
  • P2 (supersede before the throws): moved to the end of the successful select path, after the hard-budget check, emission, structural repair and rsEnd(). Regression test: accept a kv-unified presentation, reopen with kv-stable, add unsummarized messages, compile at 300 tokens → OverBudgetError; switching back finds head.sequence === 1. Also fails on the round-2 source.

Full suite 764/764. Real store with its stale receipt still 332 ms / 436 MB under the 100k ceiling. The 28-chunk circle numbers are unchanged (it has no relevant cache).

@Anarchid
Anarchid requested a review from antra-tess September 17, 2026 14:56
Resolves the one conflict in autobiographical.ts: main (17d9569, "persist
frontier only after accepted compile") deleted the early resolutions
commit that this branch had touched the lines below. Main's placement is
the same commit point this branch chose for superseding a foreign
kv-unified receipt, so the supersede now sits directly after that
frontier commit, before rsEnd(): both writes happen only once every
refusal-capable stage has succeeded, and a rejected compile changes
neither the carried frontier nor the receipt.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@Anarchid

Anarchid commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor Author

Merged origin/main into the branch at 8172c8f (main had moved on to 0.10.0). One conflict, in autobiographical.ts: 17d9569 deleted the early persistResolutions() block this branch had edited around. Its new placement — commit the carried frontier only after every refusal-capable stage — is the same point I had chosen for superseding a foreign kv-unified receipt, so the supersede now sits directly after that frontier commit, before rsEnd(). A rejected compile changes neither.

Suite on the merged head, with the locked dependency tree (npm ci; main now needs chronicle 0.4.0): 801/801 pass, 0 fail, including the new overbudget-transaction and history-index suites. (An earlier version of this comment said "all green" before I had looked at the full run; it was 776/801 against a stale chronicle 0.3.0 binding — the 25 were the history-index suites throwing Chronicle history index unsupported, environmental, gone after the reinstall.)

@Anarchid Anarchid left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review of round 4 (8172c8f, main aae303f merged in) — two-reviewer pass (Codex Sol + Claude, every finding re-verified)

Verdict: the fix is correct on the path that caused the incident; merge after three small changes (items 1–3). The round-3 shape (extension on the label, out of the key, a dominance dimension only while a cache is relevant, a cache envelope component) survives an exact-solve comparison under a relevant cache, which is the path rounds 1 and 2 never exercised. What does not survive is some of the prose: the PR claims more than it delivers for the leaf engine, for previews through the real consumer, and for its own benchmark number. Several solver weaknesses turned up along the way that predate this PR; they are listed separately and should not hold it.

Change before merge

  1. The relevant-cache growth test does not test the fix (test/adaptive/kv-unified-policy.test.ts, "label count stays bounded under a stale presentation with a relevant cache"). It asserts withCache ≤ 3 × noCache at the same staleness. On main's solver that ratio is 91,356 / 60,791 = 1.50, so the assertion passes before the fix; it only fails on main because approximationCacheErrorBound is undefined there. Its "error bound must be honest" comment is backed by two >= 0 assertions. Compare half-stale against fresh, both with the relevant cache: at 24 chunks that is 42,679 / 29,620 = 1.44 at this head versus 91,356 / 29,620 = 3.1 on main, so ≤ 2× discriminates. For honesty, compare selected.score against an all-zero-bucket solve of the same forest and assert the gap ≤ the reported bound (it is; see "Verified fine").
  2. supersedeKvUnifiedReceipt() clears the retry flag before the write (autobiographical.ts:2116-2117): kvUnifiedReceiptSupersedePending = false precedes setStateJson(..., null). If that write throws, the compile fails after the frontier was already persisted (:8305-8307) and no later compile on the instance retries, so the stale receipt stays. Swap the two lines.
  3. Scope three claims to what is true.
    • Changelog, "332 ms / 436 MB": measured through a harness that passes no kvUnifiedImmutablePrefixHash, so cacheRelevant was false. Reproduced at this head (351–390 ms, 522–540 MB). With the stale receipt's own hash supplied the same solve is 15.3 s / 8–9.5 GB, for a reason that predates this PR (#105). Say "with no relevant provider cache".
    • Changelog and :2068-2078 comment, "previewing … leaves it untouched": true for CM's own dryRun preview, false for the consumer. agent-framework previewActivation (src/framework.ts:3306-3324 at 32ef790) documents "no Chronicle writes" but reaches compileWithInjections, whose options carry only the prefix hash (src/agent.ts:536-545), so it is a non-dry-run compile and now also nulls the receipt. The defect is AF's (it already persisted the frontier from previews), and the practical harm is small, since a host previewing under kv-stable will present under kv-stable anyway. But the integration test's "inspection, preview, dry run" block only opens and closes a manager. Drop "previewing" from the claim or name the dryRun requirement; the AF side wants its own issue.
    • "No longer superlinear": holds for the DAG engine. In the leaf engine (auto-selected when a pin or lock leaves a protected hole inside a summary, kv-unified-pareto.ts:124-138) with a relevant cache, the fix barely registers, because insert-time dominance keeps every extension-incomparable label and there is no representative cap there. Forced-leaf, half-stale ÷ fresh, relevant cache, 20/24/28 chunks: 1.05 / 1.26 / 1.57 at this head, 1.08 / 1.27 / 1.75 on main (171,694 vs 190,914 labels at 28). Not a regression, and that engine is exponential in any case, but a pinned store with a stale relevant receipt can still reach the ceiling. Either give the leaf engine the same cover, or state the limit and open a follow-up. Without a relevant cache the leaf engine is fixed (47,466 vs 135,585).

Minor

  1. Stale round-2 claim in the first new test's comment (kv-unified-policy.test.ts:667-670): "Extension tokens are no longer part of the label state at all: nothing in dominance or pricing reads them from the label". Round 3 exists because that is false (kv-unified-pareto.ts:35-39, :892, :900). The test is fine (no cache, extension is inert there); the comment should say that.
  2. Non-adaptive configurations never supersede. select() sends adaptiveResolution: false to selectHierarchical (autobiographical.ts:4372-4377), which by design writes nothing, while load still raises the pending flag. A kv-unified → hierarchical → kv-unified store keeps its old receipt. No longer fatal after this PR; either hook the common select() wrapper (honouring dryRun) or say "adaptive" in the comment and changelog.
  3. My round-3 PR comment says the supersede sits after rsEnd(); since the merge of main it sits before it (:8314-8317). The code comment is right.

Verified fine

  • Dominance is a valid pruning rule for the cache term. Labels compare only inside one stateKey, so they share cache state and suffix; the final term is max(0, recomputed − extension) × price (kv-unified-policy.ts:300-310), so a.tokens ≤ b.tokens with a.extension ≥ b.extension gives a.avoidable ≤ b.avoidable for every completion. Label-side isExtension() (kv-unified-pareto.ts:715) matches the scorer's extensionTokens() (kv-unified-policy.ts:312-328). Both reviewers reached this independently.
  • The cache envelope is conservative and its slope conversion is dimensionally consistent (kv-unified-pareto.ts:545-575, :844-870).
  • Exact-solve comparison under a relevant cache: 48 configurations (12–24 chunks, 3 token layouts, budgets 0.5/0.75, cacheLambda 1 and 10000, half-stale presentation, marker mid-prefix). Bucketed 100/100/100 vs all-zero buckets: score gap 0 in all 48, no bound violation.
  • DAG growth is bounded in both cache modes. Half-stale ÷ fresh with a relevant cache: 1.44 / 1.56 / 1.50 at 24/28/32 chunks (flat); on main 3.1 / 5.2 / 8.3 (growing). Production config requires positive buckets (strategies/kv-unified.ts:315), so the DAG path always caps representatives per key; representative choice is deterministic.
  • The other new tests discriminate. No-cache growth test: main 14,256 fresh vs 60,791 half-stale, head 14,256 vs 19,448. Priced-dominance test fails on the round-2 solver (score 80 vs oracle 60), passes on main and head. Supersede test fails with main's autobiographical.ts. 30/30 at head across the two touched files.
  • Supersede lifecycle inside CM. Load only reads (:2079-2081). The single write is at the end of a successful non-dry-run selectAdaptive, after every OverBudgetError site, assertMiddleCoverage() and the frontier commit from 17d9569. kv-unified never runs it; absent or null receipts do nothing; once-only. A kv-unified load of the nulled slot yields an empty chain (:2060-2063).
  • CI green on all four matrix cells; merge state clean; git diff --check clean.

Not from this PR (lines untouched by the diff)

  • #105: cache.matchedUnits stays in the state key after the cache breaks, so broken-cache labels never compete. ~17 s / 11 GB per compile on the resident-sized store from the second kv-unified turn on, with a fresh receipt, identically on main and here. This, not #98, gates switching that store back.
  • Approximation debt is lost when its representative later exceeds the budget (Sol; prune() drops over-budget labels at kv-unified-pareto.ts:333-335, the final bound reads only survivors at :497). Sol reports an oracle disagreement (640.40 exact vs 714.60 selected) with every reported bound at zero. I checked the mechanism in the code but did not re-run that fixture. It applies equally to the new cache component.
  • Leaf-engine dominance treats fewer tokens as always better while the budget term penalises under-filling, and reports a zero bound across a token bucket (Sol; :189-200, :282-289, kv-unified-policy.ts:440-452). Same status: mechanism checked, fixture not re-run.
  • The a-posteriori bounds are honest but uninformative at resident scale (token envelope 456k on a 262k render; score bound ~150k on a score of ~7.4k).

Rejected

  • Sol's same-namespace concurrency finding (two managers with different folding strategies presenting from one namespace at once): no deployment does this and nothing else in the strategy is safe under it either.

Method

Sol: gpt-5.6-sol xhigh, read-only, 184 commands, ~31 min, 13.1M input tokens (12.6M cached); no network and no node_modules, so it could not run the integration suite or see the production store. Claude: build + mutation runs against main and the round-2 solver, the 48-configuration probe, and memory-capped runs on a copy of the production store. Found by both: dominance soundness, stale test comment, vacuous bound assertions, hierarchical gap. Sol only: leaf-engine residual, AF preview path, flag ordering, the growth test passing on main (I had wrongly counted it as discriminating), the two pre-existing solver weaknesses. Claude only: the benchmark's missing prefix hash and #105.

…afe supersede, scoped claims

- The relevant-cache label-growth test compared with-cache against no-cache
  at the same staleness, a ratio that was already 1.5 before the fix. It now
  compares half-stale against fresh with the cache relevant in both (fails on
  main's solver: 91,356 vs 29,620) and checks the reported score bound
  against an all-zero-bucket solve of the same forest.
- supersedeKvUnifiedReceipt() cleared its pending flag before the write, so
  a throwing write was never retried. Flag now clears after the write;
  regression test injects one write failure and asserts the next compile
  supersedes.
- Integration test now runs a real dryRun compile in the "must not
  supersede" block instead of only opening and closing a manager.
- Changelog and comments scoped: the benchmark number was measured with no
  relevant provider cache (#105 dominates otherwise); only a dryRun compile
  is a non-presenting preview; adaptiveResolution:false never supersedes; the
  leaf engine under a relevant cache is not covered (#107).
- Stale round-2 claim removed from the no-cache growth test's comment.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@Anarchid

Copy link
Copy Markdown
Contributor Author

Round 5 at bd94014, against the three items of the self-review above.

  1. Growth test now tests the fix. It compares half-stale against fresh with the cache relevant in both, ≤ 2×. Swapping main's kv-unified-pareto.ts/kv-unified-policy.ts into the tree makes it fail on that assertion (half-stale 91356 vs fresh 29620); at head it passes. The two >= 0 assertions are joined by a real one: the bucketed selection's regret against an all-zero-bucket solve of the same half-stale forest must be within the reported score bound.
  2. Flag order swapped in supersedeKvUnifiedReceipt(): the pending flag clears only after setStateJson returns. New integration test injects one failure of the nulling write, expects that compile to reject, the next compile on the same instance to supersede, and a kv-unified reopen to find an empty chain. With the old order it fails at that last assertion.
  3. Claims scoped.

Minors: (4) the stale round-2 sentence in the no-cache test's comment is replaced; (5) changelog and comment now say "adaptive" and state that adaptiveResolution: false never supersedes, rather than hooking select(); (6) needs no code change. The PR description was still describing round 1 (supersede on load, slot registered for every strategy) and has been rewritten to the current shape, with a Limits section.

Full suite on a clean dist/: 802/802. One run in between reported 801/802; that was my own mutation check, which had patched dist/ directly, and the incremental tsc did not rewrite the file. Gone after rm -rf dist.

@Anarchid
Anarchid merged commit 3f6a93c into main Sep 21, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants