Repository navigation
fix(kv-unified): hold a level pin at the deepest level its chain supplies until the pinned level exists (#193) - #213
Conversation
…lies until the pinned one exists Under kv-unified, a level pin (`pinAtLevel`, or `pinRange`/`markDocument` with `level`) became an exact constraint, and a leaf whose ownership chain didn't supply that level yet had no allowed level, so every compile threw "kv-unified constraints are infeasible ... produce the missing levels." Nothing requested the level, so the refusal lasted until compression and merging reached it on their own (Anarchid's probe on anima-research#175's review: five unmerged L1s under mergeThreshold 6, a level-2 pin, every compile refused, twelve ticks queued nothing), or until the pin was released. kv-stable has always clamped the same pin to the deepest produced level (kv-stable.ts:132). This direction was chosen over refusing at pin time and over requesting the missing levels from the conflict, after probing all three. - The forest decides: where buildLeaf narrows a `pin-level` exact constraint, it uses pinLevelHeldAt(availableLevels, level), the pinned level when the chain supplies it and otherwise the deepest level the chain supplies below it (0 when none). Availability is the forest's own, the same list requestedMissingLevels is computed from, so the hold and the solver can't disagree. The constraint keeps the pin's own level, so derive's sameLeafInputs still matches it; when a merge changes a leaf's chain, extend rebuilds the leaf and the hold moves with it. Locks and every other exact constraint stay strict, so a level pin that conflicts with a lock is still infeasible. `PIN_LEVEL_SOURCE` names the source both places read. - heldLevelPin(leaf) reports a hold. After each non-dry kv-unified pick, reportHeldLevelPins reads the holds off the solved forest and attributes each held message to the pin whose level its bound took (overlapping pins: the lowest level wins, so an overridden pin isn't reported). It logs once when a pin starts being held, naming the pin, its level and the level(s) held at, and once when the hold ends or the pin is removed. The episode map is per branch (cleared with the branch mirrors), so a pin that exists only on another branch isn't reported removed. A dry run logs nothing: a preview isn't the live state. - Docs: unified-solve-design.md F4b names the hold as the one exception to "no constraint is weakened, clamped, or selected inside the solver"; ProtectedRange.level, PinLevelOptions.level and both pinAtLevel docs say what a level pin does before its level exists. - Changelog: a fixed fragment for anima-research#193. 0.15.0's notes describe the refusal this replaces, for a level pin no summary supplies yet and for a range whose summary the load repair dropped; the fragment names the second, since its 0.15.0 entry says it is refused whatever the budget. Tests: test/kv-unified-level-pin-hold.test.ts runs the five probe shapes through the strategy (placed over two unmerged L1s, inside one L1, two levels up, a stored pin reopened, a pin placed in the recent window that then leaves it), each asserting compiles go through, the held level, then the pinned level once it exists; and the log: once per episode, nothing from a dry run, removal, overlapping pins, branch switches. A forest test in kv-unified.test.ts covers the rule itself: held at L1 or raw, moved to L2 through derive's extension when the L2 arrives, held again when the L2 is dropped, and a lock against a held pin still a conflict. anima-research#116's picker test no longer tolerates the refusal, and the certificate test that used pinLevel 9 to make a conflict now uses a lock against a pin. anima-research#175's persistence test, which pinned the refusal for a range pinned at a level only a dropped summary supplied, now asserts the hold there: the range renders at its surviving L1s, and the hold is logged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ints allow the pinned level A review of e9bcfa2 found that the hold was decided before the leaf's other constraints were weighed. buildLeaf narrowed a level pin to its held level, so a lock at that level agreed with the hold and no conflict was recorded, though the lock and the pinned level contradict each other. Merges don't consult locks (checkMergeThreshold never reads this.locked), so once a merge produced the pinned level, every compile refused. The conflict's requestedMissingLevels also lost the pinned level, since the held target is always available. - levelPinHold(constraints, availableLevels) decides the hold once, for buildLeaf and heldLevelPin alike: a leaf's level pin is held only when its chain doesn't supply the pinned level and every other constraint on the leaf admits the pinned level itself (admitsLevel: raw admits 0, an exact level only itself, a cap levels at or under it, a floor levels at or over it). A pin that a lock at another level, or any other constraint, contradicts keeps its own level, so the pair conflicts at once, as before anima-research#193, and the conflict lists the pinned level as missing when it is. pinLevelHeldAt is folded into it and no longer exported. - Text the review found overstated or loose: - the changelog fragment: held "below" the pinned level; a range the budget can't hold at its held level is refused as over budget (a probe threw the same OverBudgetError with and without the pin); what the log reports, and that it starts over after an open or a branch switch; a level pin and a lock at another level are refused even while the pinned level doesn't exist. - reportHeldLevelPins' doc says when each line comes: a start line when a compile first finds a pin held, an end line when a later compile finds none of its messages held for it (its level arrived, another pin's bound took them, or they left the folded middle) or the pin gone; dry runs and refused compiles report nothing; the record starts over with each open and branch switch. - ProtectedRange.level and both pinAtLevel docs: held at the deepest level below the pinned one that a summary supplies; under kv-unified a lock at another level still conflicts. - docs/unified-solve-design.md: F4 names the hold as the one, logged, exception to honouring pins absolutely; F4b says produced means on the forest's own chain (a summary above a treeified one supplies no level there), and that the hold applies only while every other restriction allows p itself; its test line excepts a held pin. Tests: the forest test locks the pinned leaf at L0 and at the L1 the pin would be held at, and asserts the pin isn't held, the leaf has no level and the conflict names the missing L2; locked at the pinned level once it exists, the two agree. A new forest test runs one other constraint of each kind beside a pin at a missing L2 (a cap at 3 or 1, a floor at 1, 2 or 3, a raw constraint, an exact L1) and asserts the leaf's levels and the missing levels its conflict names. The certificate test's lock-against-pin step now asserts that its conflict is c0's, from the lock and the pin. Not changed, from the same review: a held pin beside a capped neighbour needs nothing, since the solver emits the neighbour raw as a protected hole beside the L2's recall, the same frontier as a pin at an existing L2 (probe at e9bcfa2). No API changes a pin's level in place. That the conflict suggestion always mentions missing levels predates this change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…llows the held level too Answers a second review, of e9bcfa2..2245c31. Code: levelPinHold weighed the leaf's other constraints only at the pinned level, and buildLeaf then narrowed the leaf to the held level. A floor that allows the pinned level but not the held one (pinned L2 or L3 beside "level >= 2", chain L1) emptied the leaf, and since the pin was "held" its own level never reached requestedMissingLevels: the conflict named only the floor's level, or nothing when a level above the pin met the floor. The hold now applies only where every other constraint allows both the pinned level and the level held at; otherwise the pin is strict and the conflict names the pinned level, which is what would clear it. The pin itself doesn't allow the held level, so `other === pin` now carries weight. The integer/negative guard on the pin's own level went: buildLeaf already empties the leaf for such a level, so the forest is infeasible and nothing reads its holds. Tests: the table now sets the pinned level per row and checks heldLevelPin on every row. New rows: a cap at the pinned level (held, which pins max's <=), a floor at the held level (held, which pins min's >=), and a floor between the held and the pinned level (pinned L3 beside min 2: not held, missing [2, 3], where the old rule gave [2]). The floor-at-L2 row now expects the pin not held. Text: - F4 says a pin over a head or tail message, the recent window included, adds no restriction while the message is there, as before anima-research#193. - F4b states the rule as both levels, with the floor case. - reportHeldLevelPins' doc says it reports what the solve found, before presentation: a compile the solver refuses reports nothing, and one refused after it, as over budget, has already reported its holds. - The fragment, the test header and the pin docs (ProtectedRange.level and both pinAtLevel docs) describe the hold as kv-unified's only, without "as kv-stable does": kv-stable clamps to min(pinned, deepest produced) and, on a chain with a level above the pin and none at it, fixes the chunk at the pinned level, so the two differ there. Not changed: a three-level start line reads "raw and L1 and L2", which isn't worth two lines; a pin contradicted by a lock names its missing level, which producing won't clear, which predates anima-research#193 and goes with the conflict-text item in the PR's out of scope; a non-exact constraint labelled 'pin-level' needs a label constraintsFor never gives one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…r budget The hold report runs after the solve and before the frontier is presented (autobiographical.ts, the kv-unified block right after picker.run), so a compile the budget then refuses has already said which ranges it holds finer than pinned, which can be why the range was too large. That placement was a choice, and the PR text describes it, but no test held it. The new test pins a range at L2 over two L1s and compiles at a 600-token budget: the compile is refused with OverBudgetError, and the hold's start line was logged first. Moving the report after the over-budget check, so it reports only from compiles whose solve wasn't exhausted, fails it. On main the same compile is refused as infeasible instead, so it fails there too. A compile the solver itself refuses logs nothing, since the report is never reached; that's incidental to where the report sits, so it stays described by reading rather than pinned. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…; the floor wording says which floors Answers a third review, of 2245c31..067ba0e. - The round before removed levelPinHold's check that the pinned level is a non-negative integer, on the reason that buildLeaf already empties such a leaf and nothing reads holds from a refused solve. That reason held for the hold report, but not for heldLevelPin, which is exported and documented as null unless a leaf is held: for a level of 1.5, -1 or NaN it returned a hold for a leaf buildLeaf had emptied. The check is back, so levelPinHold holds no pin buildLeaf can't place, and its doc names the case. The table test's three new rows (1.5, -1, NaN) assert no hold, an empty leaf and a conflict that names no level; taking the check out fails them. - The table now covers a leaf whose chain supplies no summary, held raw beside a cap at the pinned level and strict beside a floor that excludes raw, and two other constraints together, where only the second rules the hold out; weighing only the first other constraint fails it. The table's rows now take a leaf id and a list of constraints. - "A floor above the held level" in levelPinHold's comment and F4b also covered a floor above the pinned level, which contradicts the pin and which producing the pinned level doesn't clear. Both now say a floor above the held level and at or below the pinned one. Not changed: how a pin's level is validated (pinRange drops an invalid level, and pins read back from a store aren't checked) predates this change and goes in the PR's out of scope; that an invalid level's conflict names no level is the conflict-suggestion item already there; that a bound takes a classic pin's place where pins overlap is documented; the strategy test "two levels up" covers an L3 pin held below its level, asserting L2 once L1 and L2 exist; and the strategy's docs say only a lock can sit beside a level pin. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
[Medium impact] The PR appears safe to merge; the previous finding is fixed and no new actionable issue was found. SummaryUnder
Reviews (2) · Last reviewed commit: "fix(kv-unified): know the strategy's lev..." · Reviewed by Greptile |
…s label (anima-research#213 review) Greptile's review of context-manager anima-research#213 found that levelPinHold took any constraint labelled 'pin-level' as the level pin to hold. A constraint's source is a human-readable label (CanonicalLeafConstraint.source), and CanonicalForestOptions.constraints lets a library caller give any label: an exact constraint {level: 2, source: 'pin-level'} on a leaf whose chain supplies only L1, with no chunk pinLevel, was held at L1 instead of conflicting. The internal review hadn't raised it. constraintsFor now records the constraint it makes from a picker chunk's pinLevel in a module-private WeakSet, and levelPinHold and buildLeaf's narrowing find the pin there, so a caller's exact constraint stays strict whatever it's labelled. The label stays the source certificates show. buildLeaf's check is the same identity for consistency: with levelPinHold finding the pin by identity, a held pin and another raw or exact constraint can't coexist, so no test can tell the two forms of that check apart. sameLeafInputs still reads the pin's level by its label, as it reads every strategy constraint; derive declines whenever option constraints are given, so it only ever sees constraintsFor's own. Test: a table row with no chunk pinLevel and a caller's exact constraint labelled 'pin-level' at a missing L2 expects no hold, an empty leaf and a conflict naming L2. Removal R24 (the pin found by its label) fails it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s label (anima-research#213 review) Greptile's review of context-manager anima-research#213 found that levelPinHold took any constraint labelled 'pin-level' as the level pin to hold. A constraint's source is a human-readable label (CanonicalLeafConstraint.source), and CanonicalForestOptions.constraints lets a library caller give any label: an exact constraint {level: 2, source: 'pin-level'} on a leaf whose chain supplies only L1, with no chunk pinLevel, was held at L1 instead of conflicting. The internal review hadn't raised it. constraintsFor now records the constraint it makes from a picker chunk's pinLevel in a module-private WeakSet, and levelPinHold and buildLeaf's narrowing find the pin there, so a caller's exact constraint stays strict whatever it's labelled. The label stays the source certificates show. buildLeaf's check is the same identity for consistency: with levelPinHold finding the pin by identity, a held pin and another raw or exact constraint can't coexist, so no test can tell the two forms of that check apart. sameLeafInputs still reads the pin's level by its label, as it reads every strategy constraint; derive declines whenever option constraints are given, so it only ever sees constraintsFor's own. Test: a table row with no chunk pinLevel and a caller's exact constraint labelled 'pin-level' at a missing L2 expects no hold, an empty leaf and a conflict naming L2. Removal R24 (the pin found by its label) fails it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Anarchid
left a comment
There was a problem hiding this comment.
Nightly dual review · round R1 at 52bfcd3 · reviewers: Codex gpt-6-astra (xhigh) and Claude claude-fable-5-1, merged and every claim re-verified by the Claude leg · posted by the maintainer's campaign runner. Verdict: CHANGES_REQUESTED (the formal review event is left to the maintainer). Finding ids R1-n are stable; cite them in fix commits.
Reviewed at 52bfcd3 against main cc599f1
Verdict: changes requested, small ones. The hold itself is correct and minimal, it composes with the #45 set (#231's subtree memo and #229 on top of it), and the certificate cannot serve a stale level across a hold starting or ending. What stands between this and merge is mechanical: the branch conflicts with main (three files, all doc-comment and test-adjacency), and once rebased its three new log lines would be the only strategy lines on main that bypass the namespace logger #239 introduced. One minor hole in admitsLevel and one minor loss of test coverage round it out.
No earlier review by Anarchid exists on this PR; this is round 1.
Established facts checked
- Fixes #193: confirmed. On the merge-base (0.15.0) a
pinLevelthe chain doesn't supply became an exact constraint with no allowed level and every compile threw. The rewrittentest/l1-recall-raw-pins-frontier.test.ts:590no longer tolerates the throw, and the new strategy tests compile through 12 rounds and across the merge that produces the level. - Hold at the deepest supplied level below the pin, raw if none, via a WeakSet-identified pin: confirmed (
src/adaptive/kv-unified.ts:349-359,:966,:979,:1866-1869). Start/stop logging confirmed (src/strategies/autobiographical.ts:10651-10698). - Conflicts with current main: confirmed,
src/strategies/autobiographical.ts(pinAtLevel doc comment, where main now carries kv-stable's wording from a later PR),src/types/strategy.ts(PinLevelOptions.leveldoc),test/strategy-persistence.test.ts(main added a test directly above the one this PR rewrites; main also still has the old "refuses as infeasible" test body, which must be replaced by this PR's). Keeping both sides resolves each;src/adaptive/kv-unified.tshas not changed onmainsince the merge-base, so the forest patch applies as is. - Strategy-reset overlap with #231: both edits of the
clearBranchMirrorsblock auto-merge cleanly; #231'sdelete this.kvUnifiedReuse.subtreeand this PR'sthis.heldLevelPins.clear()both survive. Not a real conflict.
Blockers
None.
Major
R1-1. Rebase onto main, and route the three hold lines through the strategy's namespace logger. main now carries #239: every strategy stderr line goes through this.diag (src/strategies/autobiographical.ts:1778 on main, src/diagnostics.ts), which appends ns=<namespace> so several managers in one process can be told apart. main's autobiographical.ts has zero direct console.warn( calls left; this PR adds three (src/strategies/autobiographical.ts:10685, :10694-10697 at this head). After a plain merge they would be the only un-namespaced strategy lines, and a reader filtering one namespace's diagnostics would never see which agent's pin is being held.
Fix: on rebase, replace console.warn( with this.diag.warn( for the three lines. Note the default namespace is default, so the exact-equality assertions in test/kv-unified-level-pin-hold.test.ts:206-208, :213, :225, :238-240, :251-253 will then see ns=default appended and must expect it (or match a prefix); the includes-style checks in the persistence test and the regex-based ones survive as written. Resolve the three conflicts by keeping both sides, and replace main's old "refuses as infeasible" persistence test body with this PR's "holds" body. (The trial merges below did exactly this and pass.)
Minor
R1-2. admitsLevel trusts a malformed companion bound that buildLeaf rejects, so a leaf can be reported held while it conflicts, and its conflict then names no missing level. admitsLevel (src/adaptive/kv-unified.ts:324-330) compares min/max bounds numerically only; buildLeaf (:970-973) empties the leaf for any bound that isn't a non-negative integer. With pinLevel: 2, chain supplying [0, 1], and a companion { kind: 'min', level: -1 } (or 0.5, or { kind: 'max', level: 2.5 }): levelPinHold returns { level: 2, heldAt: 1 }, buildLeaf narrows the pin to L1 (available, so nothing is added to requestedMissingLevels), then the bad bound empties allowedLevels. Result: a conflict whose missingLevels is [] while heldLevelPin(leaf) says the pin is held, and the strategy's report would log a hold for a leaf that was refused. Reachable only through CanonicalForestOptions.constraints or a stored pin with an unvalidated maxLevel, and the compile still refuses, so this is a diagnostics defect, not a frontier one. Fix: in admitsLevel, return false for any non-raw constraint whose level isn't a non-negative integer, and add the three rows to the table test in test/adaptive/kv-unified.test.ts:226-299 (expect no hold, missingLevels [2]). Found by both reviewers independently.
R1-3. The rewritten persistence test dropped two cheap checks. test/strategy-persistence.test.ts:1476-1484 used to go on to unpin and compile, then re-pin at an existing L1 and compile. Both still hold under the new behaviour and cost two lines; keeping them preserves the "release clears it" path for this repaired-store shape, which nothing else exercises on exactly this fixture. Nit-level.
Verified fine
- Hold is minimal. It applies only when every other constraint on the leaf admits both the pinned and the held level (
kv-unified.ts:358-359). A lock at the carried level, a raw pin, a cap at or below the held level, or a floor above the held level and at or below the pin all keep the pin strict, so the conflict lists the pin's own source and names the pinned level inrequestedMissingLevels(table attest/adaptive/kv-unified.test.ts:226-299; lock rows at:197-224). Head and tail chunks never carrypinLevel(autobiographical.ts:9027builds only middle segments with bounds), so the F4 note about the recent window is accurate. - WeakSet identity.
constraintsFortags each pin it builds (:1866-1869); the leaf keeps the same constraint objects, and derived forests share leaf objects, soheldLevelPinreads the same identity the solve used. A caller's exact constraint labelledpin-levelstays strict (last table row).derivedeclines whenever either forest has option constraints (:1036), so no reuse path mixes the two. - Transition and churn. The hold is a pure function of the leaf's chain and constraints, recomputed whenever the leaf is rebuilt. A summary arriving or changing parent flips
ownershipChangedandextendrebuilds every leaf whose chain changed (:795-817,:895-909); a summary dropped or re-levelled forces a full build (:1056,:1068). So the leaf moves on the first compile after the level exists, and cannot flip-flop unless the store itself oscillates. The strategy tests pin this with exact line counts across growth and merging (one start line, one end line over dozens of compiles;test/kv-unified-level-pin-hold.test.ts:178-227), and the forest test covers arrive-via-derive and drop-then-held-again (test/adaptive/kv-unified.test.ts:197-224). - Hysteresis certificate. Fixed levels are reused only for the identical leaf object against the identical receipt entry (
src/adaptive/kv-unified-certificate.ts:437-443); any rebuilt leaf is re-checked against its currentallowedLevelsand representation hash (:446-447) and the certificate declines rather than fixing a level the hold no longer allows.reuseDaglikewise declines on any owned-leaf object change (:168-178). A hold starting or ending always rebuilds the leaf, so no stale fixed level can survive it. - #231's subtree memo. Its per-leaf signature hashes the full
allowedLevelslist andavailableLevels.length(src/adaptive/kv-unified-subtree.ts:289-300on #231/#229), and entries are read through the summary object, which is new whenever its leaves or children change. The hold's input is therefore part of the memo key. - Logging bounded and correct. The map holds at most one entry per level pin; entries are added once at hold start and deleted at hold end or pin removal (
autobiographical.ts:10683-10698). Dry runs report nothing (:9159). Branch switch and open clear it (:1885), so a pin on another branch is never reported removed and a still-held pin is re-announced, both tested (test/kv-unified-level-pin-hold.test.ts:257-276). Attribution follows the binding bound:pinLevelBoundsgives a position the lowest level among overlapping pins (:2751-2756) and the report counts a message toward a pin only whenhold.level === pin.level(:10673), tested with an L3 and an L2 pin over the same range. Two pins at the same level over the same messages are each reported; harmless. - Rewritten tests lose no real coverage except R1-3: the frontier test still asserts per-message bounds and now requires success; the certificate test still proves a lock-versus-pin conflict is declined and now asserts which constraints conflict.
- Docs and changelog match the code (F4/F4b wording,
ProtectedRange.level,PinLevelOptions.level, bothpinAtLeveldocs, the.fixed.mdfragment naming matcheschangelog.d/). The design claim "produced means on the chain the forest builds" matchesbuildLeafreadingavailableLevelsfromchainonly. - Trial merges. (a) This head merged onto
maincc599f1 with the three conflicts resolved as above: typecheck and build clean, 2215/2215 tests pass on Node 22. (b) This head merged ontomain+ #229 (which contains #231, so the subtree engine is the default): typecheck and build clean, 2246/2246 pass on a rerun. One run of (b) had a singleTypeError: Number.isFinite is not a functioninside the unrelatedkv-unified-certificate-holesexhaustive-cuts test; it did not reproduce in three isolated runs or a second full run and looks environmental, not this PR's.
Method. Two passes. Codex (gpt-6-astra, reasoning effort xhigh, read-only sandbox, no network) read the PR worktree against current main and the #231 ref; 58 commands, 1.63M input tokens (1.50M cached), 12.6k output, about 10 minutes; node_modules were absent in its tree so it ran no tests and used an in-memory source probe for R1-2. Claude Fable 5.1 did the forest-level reading, both trial merges with full test runs, and the main-side checks (#239 logger, default namespace, pin-bound construction). R1-1 and R1-3: Codex first, confirmed by Claude against main. R1-2: found independently by both. Everything under "Verified fine" was checked by at least one reviewer and re-read by Claude before adoption.
…search#213 review R1-1) Anarchid's review R1 of anima-research#213 at 224c33c (R1-1) asked for main to be merged in and the hold's three log lines sent through the strategy's namespace diagnostics, which main gained with anima-research#239 and anima-research#245. Conflicts, each resolved by keeping both sides: - src/strategies/autobiographical.ts, pinAtLevel's doc: main's wording, which drops "EXACTLY" and gives kv-stable's exceptions (a chunk whose summaries stop below the level, a summary that doesn't list it), with this branch's parenthesis on kv-unified's hold. - src/types/strategy.ts, PinLevelOptions.level's doc: main's kv-stable exceptions, and kv-unified's "once a summary supplies it". - test/strategy-persistence.test.ts: main added a test (a kv-unified submission kept in flight across a reload) directly above the test this branch rewrites; main's test stays, followed by this branch's "holds" test in place of main's unchanged "refuses as infeasible" one. Beyond the conflicts: the hold report's two console.warn calls, three lines, now go through this.diag.warn, so they end with ` ns=<namespace>` like every other strategy line on main, and the hold test's six exact assertions expect ` ns=default`. The prefix and `includes` checks match as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…the hold; restore the release checks (anima-research#213 review R1-2, R1-3) R1-2: admitsLevel compared a companion min or max bound numerically, while buildLeaf empties a leaf for any bound that isn't a non-negative integer. So with a pin at L2 over a chain supplying L1 and a floor of -1 (or 0.5, or a cap of 2.5), the hold applied, the pin narrowed to the available L1 and named no missing level, and the bound then emptied the leaf: a conflict with no missing level on a leaf heldLevelPin reported held, and a hold logged for a refused leaf. admitsLevel now allows no level for a constraint whose level isn't a non-negative integer, so the pin stays strict there and the conflict names L2. The table test gains the three rows Anarchid named (no hold, allowed levels [], missing [2]). R1-3: the persistence test, rewritten to assert the hold, had dropped the old test's closing checks. They're back: unpin, compile, and the hold's "was removed" line; then a pin at the L1 the surviving summaries supply, and compile. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…search#213 review R1-1) Anarchid's review R1 of anima-research#213 at 52bfcd3 (R1-1) asked for main to be merged in and the hold's three log lines sent through the strategy's namespace diagnostics, which main gained with anima-research#239 and anima-research#245. Conflicts, each resolved by keeping both sides: - src/strategies/autobiographical.ts, pinAtLevel's doc: main's wording, which drops "EXACTLY" and gives kv-stable's exceptions (a chunk whose summaries stop below the level, a summary that doesn't list it), with this branch's parenthesis on kv-unified's hold. - src/types/strategy.ts, PinLevelOptions.level's doc: main's kv-stable exceptions, and kv-unified's "once a summary supplies it". - test/strategy-persistence.test.ts: main added a test (a kv-unified submission kept in flight across a reload) directly above the test this branch rewrites; main's test stays, followed by this branch's "holds" test in place of main's unchanged "refuses as infeasible" one. Beyond the conflicts: the hold report's two console.warn calls, three lines, now go through this.diag.warn, so they end with ` ns=<namespace>` like every other strategy line on main, and the hold test's six exact assertions expect ` ns=default`. The prefix and `includes` checks match as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…the hold; restore the release checks (anima-research#213 review R1-2, R1-3) R1-2: admitsLevel compared a companion min or max bound numerically, while buildLeaf empties a leaf for any bound that isn't a non-negative integer. So with a pin at L2 over a chain supplying L1 and a floor of -1 (or 0.5, or a cap of 2.5), the hold applied, the pin narrowed to the available L1 and named no missing level, and the bound then emptied the leaf: a conflict with no missing level on a leaf heldLevelPin reported held, and a hold logged for a refused leaf. admitsLevel now allows no level for a constraint whose level isn't a non-negative integer, so the pin stays strict there and the conflict names L2. The table test gains the three rows Anarchid named (no hold, allowed levels [], missing [2]). R1-3: the persistence test, rewritten to assert the hold, had dropped the old test's closing checks. They're back: unpin, compile, and the hold's "was removed" line; then a pin at the L1 the surviving summaries supply, and compile. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Replying to this review. All three R1 findings are addressed in two new commits on this branch:
At The description now describes this head. That includes kv-stable since #234: it renders a pin whose level the chunk's lineage skips at the next level down, as the hold does (the test "a pin-at-level at a level its own lineage skips holds the chunk at the next level down" in Written by Hamish-2699. — Lottie-2661 |
Conflicts, each kept on both sides: - src/strategies/autobiographical.ts: main's import of ceilingMark beside this branch's import of heldLevelPin. - test/adaptive/kv-unified.test.ts: this branch's level-pin tests and main's "suggests producing the missing levels only where that clears every conflict", which landed at the same place. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…h constraintAdmits main's anima-research#259 (for anima-research#256) added constraintAdmits, the same rule as the hold's admitsLevel: whether a leaf constraint admits a level, with a constraint whose own level isn't a non-negative integer admitting nothing. levelPinHold now calls constraintAdmits, and admitsLevel is gone. constraintAdmits' doc adds that it reads the constraint alone, whatever the leaf's chain supplies. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
At Written by Hamish-2699 (Claude Opus 5.5). Desmond-2314 read the merge and the fold. — Daniel-2612 |
Anarchid
left a comment
There was a problem hiding this comment.
Nightly dual review · round R2 at 85b77ca · reviewers: Codex gpt-5.6-sol (xhigh) and Claude claude-opus-5-5, merged and every claim re-verified by the Claude leg · posted by the maintainer's campaign runner. Verdict: CHANGES_REQUESTED (the formal review event is left to the maintainer). Finding ids R2-n are stable; cite them in fix commits.
Reviewed at 85b77ca against main 4fbe6c9
Verdict: changes requested, one small change. All three R1 findings are fixed. The hold itself is still correct and minimal, and the branch merges onto today's main with no textual conflict and passes the full suite there. But main has moved 24 merges past the branch's last merge (17a7f6d6). Two of those merges, #327 and #330, changed how every pin reader reads a pin over a message stored in parts, and the hold report this PR adds is a pin reader that doesn't take that reading. So on main as it is today, the forest can hold a pin's parts below its level while the report undercounts them or logs nothing at all. That is the "silently violated" case F4 says the log exists to rule out. The fix is a few lines, which we tried on the trial merge (below).
R1 dispositions
- R1-1 (merge
main, route the three hold lines through the namespace logger): fixed.mainwas merged atcc599f19(47247e7) and again at17a7f6d6(b5cc62b). All three lines now go throughthis.diag.warn(src/strategies/autobiographical.ts:15290,:15299), which addsns=<namespace>. The exact assertions intest/kv-unified-level-pin-hold.test.tsexpectns=default. The branch is behind again (see R2-1), but it merges cleanly. - R1-2 (
admitsLevelaccepted malformed companion bounds): fixed. It was fixed first in 26a97ab. In 85b77ca,admitsLevelwas folded into #259'sconstraintAdmits(src/adaptive/kv-unified.ts:2061-2068). That function applies the same rule asbuildLeaf(:990-1011) for every kind:rawadmits only 0, andexact/max/mincompare numerically. A bound whose level isn't a non-negative integer admits nothing, so it can't let the hold apply to a leaf thatbuildLeafthen empties. The table test has the three R1 rows (test/adaptive/kv-unified.test.ts:284-286: floor -1, floor 0.5, cap 2.5 → no hold, conflict names L2), plus rows for pin levels 1.5, -1 and NaN (:293-295). - R1-3 (persistence test dropped two checks): fixed.
test/strategy-persistence.test.ts:1840-1849again unpins and compiles, asserts the "was removed" line, then re-pins at L1 and compiles.
Facts checked
- Fixes #193: confirmed on today's
main.main'sbuildLeafstill turns an unsuppliedpinLevelinto an emptyallowedLevels. With this PR merged onto4fbe6c9, the new strategy tests compile through every probe shape, and the rewritten #116, #175 and certificate tests pass. - Round 1 was at
52bfcd3, by a Codex Astra + Claude pass, with R1-1..R1-3 as summarised: confirmed. - The PR description's citations and attributions: every
file:linein it resolves as claimed at this head. The namespace logger is #245 (for issue #239).constraintAdmitscame with #259, and kv-stable'srenderableLevelwith #234. The connectome-hostlevelHonorednote is accurate at connectome-host's currentmain(nowsrc/web/panel-data.ts:620).
Blockers
None.
Major
R2-1. The hold report reads a pin by its ids, but since #327/#330 the forest reads it whole. A held pin over a message stored in parts is undercounted, or never logged.
reportHeldLevelPins decides which messages count toward a pin by walking from pin.firstMessageId to pin.lastMessageId (src/strategies/autobiographical.ts:15271-15276 at this head). On main, #327 added pinPositions (src/strategies/autobiographical.ts:3773 at 4fbe6c9). Under adaptive resolution, which kv-unified's level pins require, its span runs from the first part of the message the first id is in to the last part of the message the last id is in. #330 then made it the reading for every pin reader (doc at :3769: "Every reader of pins takes this"). pinLevelBounds and resolvedPinBounds read through it, and the picker chunks take pinLevel from those bounds (:13254-13263). Recent-window chunks carry no bound (:13294). So the forest puts the level, and the hold, on every part of the message, while the report looks only at the parts between the pin's own ids. Both #327 and #330 landed after 17a7f6d6, so this is a semantic conflict that no textual merge flags. The trial merge passes 2,915/2,915 because no test checks the hold report for a level pin on a message stored in parts.
Probe on the trial merge: kv-unified with adaptive resolution and 1,500-token chunks, one 6,000-word message stored in 9 parts, and a pin at L2 where no summary supplies L2:
| pin | parts the forest holds (heldLevelPin) |
what the log says |
|---|---|---|
markDocument(id, { level: 2 }), as connectome-host's panel calls it |
9 | … at L1 in 1 message(s) … |
pinRange(part, part, { level: 2 }) on the first or the last part, message in the middle |
9 | … in 1 message(s) … |
pinRange(lastPart, lastPart, { level: 2 }), newest parts still in the recent window (0 / 4 / 10 messages after it) |
5 / 6 / 8 | nothing |
The last row is a silent hold: the named part sits in the recent window, so it isn't held, while the earlier parts are held below the pinned level. By the same code, a "no longer holds" line can also fire while parts the pin's ids don't name are still held, if the named parts' level arrives first. We didn't probe that case. This path is reachable from connectome-host. Its pins panel lists stored message ids from getMessageWindow, parts included (src/web/panel-data.ts:648-686 at its main), and passes the operator's chosen ids straight to pinRange / markDocument (:690-703). The frontier is right in every case; only the diagnostics are wrong. But those diagnostics are the PR's stated answer to F4, and the changelog promises that a compile that finds a pin held logs it.
Fix: in reportHeldLevelPins, take the span from this.pinPositions(pin, positionOf, messages) (skip the pin when it returns undefined) instead of walking the ids. We applied exactly that to the trial merge. The probe then reports 9 / 9 / 5 / 6 / 8, matching the forest, and the hold, persistence, whole-body-pins, picker-frontier and forest test files pass 171/171. Add a strategy test with a level mark on a message stored in parts, including one case where the named part is in the recent window. Two smaller points:
- With the fix, one message in 9 parts reads
in 9 message(s). Either count messages (body runs) or call them parts. - Merge
mainagain so CI runs the tree that will ship.
Minor
R2-2. The changelog fragment dates the old refusal to one release. changelog.d/193-kv-unified-level-pin-hold.fixed.md says the repaired-store case is held "where 0.15.0 refused it as infeasible whatever the budget". Every release from 0.15.0 through 0.21.0 refuses it, and the fragment will ship after 0.21.0. A reader on 0.20 or 0.21 could take it to mean their version already behaves. Suggest "where releases from 0.15.0 refused it". Nit-level.
Verified fine
- The hold rule.
levelPinHold(src/adaptive/kv-unified.ts:368-379) picks the deepest available level below the pin, or 0, which is always available (:974). It applies only when every other constraint admits both the pinned and the held level. So a held leaf can never conflict: its allowed levels are exactly[heldAt]. A lock at the held level or at another level, a raw pin, or a floor between the two keeps the pin strict, and the conflict names the pinned level (table attest/adaptive/kv-unified.test.ts). - The pin is known by identity. Only
constraintsForbuilds leaf constraints (:1877-1903), and it tags the pin in theWeakSet.derivedeclines whenever either forest has option constraints (:1058), so a caller'sexactconstraint labelledpin-levelstays strict, and no reuse path mixes the two. - Reuse paths on today's
main. A summary arriving, dropping or re-levelling rebuilds the leaf, so the hold moves with it. The certificate re-checks allowed levels and representation hashes before reusing a fixed level. The subtree memo's signature includes the full allowed-level list. Each was checked by Sol on the trial merge and re-read by Claude, and the R1 analysis carries over unchanged. - Where the report runs. It runs after the solve and before the frontier is presented, so an over-budget refusal has already logged the hold. Dry runs skip it. The map is cleared with the branch mirrors on open and branch switch (
src/strategies/autobiographical.ts:2858), holds at most one entry per reported pin, and lives per strategy instance, so several managers' lines stay apart under the namespace logger. - The rest of what
maingained since17a7f6d6doesn't touch the hold: thinking binding, dry-compile stats, the load-repair report and the plan's over-budget naming. Head and tail chunks still carry no bound, so the F4 sentence about the recent window holds onmain. #327's "a mark's own part that another pin's whole reading holds at a level is summarized" keeps those parts feasible and composes with the hold. - Docs. F4/F4b,
ProtectedRange.level,PinLevelOptions.leveland bothpinAtLeveldocs read coherently after the auto-merge withmain's rewritten pin docs. No remaining text on the merged tree says a kv-unified level pin refuses. - Rewritten tests lose no coverage. The certificate test now asserts which constraints conflict (
lockedByAgent,pin-level). The #116 picker test now requires success. - Trial merge. This head merged onto
main4fbe6c9with no conflicts. Typecheck and build are clean, andnpm testpasses 2,915/2,915 on Node 22.22.1.
Outside this PR (as its description says)
- connectome-host's pins panel still tells operators that under any folding but kv-stable, "at L_k degrades to raw" (
src/web/panel-data.ts:620,web/src/Pins.tsx:166at itsmain). That is already false under kv-unified, and with this PR a held pin isn't raw either. It's a connectome-host change. - Pins read back from a store don't go through
normalizePinLevels(src/strategies/autobiographical.ts:3383-3386onmain), as the description says. Unchanged here.
Method. Two passes. Codex (gpt-5.6-sol, reasoning effort xhigh, read-only sandbox, no network) read the PR worktree against current main, a built trial merge of this head onto main, and connectome-host's main. It ran 118 commands in about 7 minutes, using 4.99M input tokens (4.80M cached) and 26.4k output. It ran the two filesystem-free adaptive test files in both trees; the sandbox's read-only /tmp stopped the store-backed files. Claude (Opus 5.5) did the freshness check (24 merges on main since the branch's last merge), the trial merge and full test run, the R2-1 probes and the trial fix, the connectome-host reachability check, and the changelog. R2-1 was found independently by both: Sol's scenario, a pin naming a part in the recent window, is the last row of Claude's probe. R2-2 was found by Claude only. Both reviewers checked the R1 dispositions. Everything under "Verified fine" was checked by at least one reviewer and re-read by Claude before adoption.
No textual conflicts. Since the branch's last merge, anima-research#327 and anima-research#330 made every reader of a pin read it whole through pinPositions: a pin naming one part of a message stored in parts covers the whole message. The hold report this branch adds still read pins by their ids; the next commit brings it to pinPositions (R2-1). Under kv-stable, anima-research#327's hold of each summary a level mark reaches and this branch's kv-unified hold never act in the same compile: each runs under its own foldingStrategy. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
reportHeldLevelPins walked a pin from its first id to its last, while the forest reads it through pinPositions, which under adaptive resolution covers every part of a message stored in parts. So a level pin naming one part was reported held in fewer messages than the forest held, or not at all when the part it names was still in the recent window or already had the pinned level; a "no longer holds" line could come while other parts were still held. The report now takes the span from pinPositions, and counts messages: a message's parts count once. Tests, on a 6,000-word message stored in nine parts: a pin from its fourth part to the next message is reported in 2 messages; a pin on its last part, still in the recent window, is reported held; and with mergeThreshold 2, where L2 reaches the named first part before the rest, the hold is reported until L2 covers every part. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e from 0.15.0 (R2-2) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… promised the level; pin more of the hold report - The README and PickerChunk.pinLevel said a level pin holds its messages at that level. Under kv-unified, until the level exists, the forest holds them below it; both now say so. - A lock at another level conflicts with a level pin on a message in the folded middle; head and tail messages carry no bound. ProtectedRange.level and the fragment now say where. - The fragment no longer says a held range is always larger: it can take more of the budget than the pin asked for. - The report's doc names one more way a hold ends for a pin: the store no longer holds an end it names. - Tests: a lineage that skips a level (L1, L2, then L4) holds an L3 pin at L2, not L4; two pins at one level over different spans each count their own messages; and the levels a message's parts are held at are each named. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Replying to this review. @Anarchid Addressed in
At By our reading of Written by Hamish-2699 (Claude Opus 5.5). — Fiona-2757 |
Anarchid
left a comment
There was a problem hiding this comment.
Nightly dual review · round R3 at cd779a3 · reviewers: Codex gpt-5.6-sol (xhigh) and Claude claude-opus-5-5, merged and every claim re-verified by the Claude leg · posted by the maintainer's campaign runner. Verdict: APPROVE (the formal review event is left to the maintainer). Finding ids R3-n are stable; cite them in fix commits.
Reviewed at cd779a3 against main 1fe572e
Nightly dual review · round R3 · reviewers: Codex gpt-5.6-sol (xhigh) and Claude claude-opus-5-5, merged, with every claim re-verified by the Claude pass · posted by the maintainer's campaign runner. Finding ids R3-n are stable; cite them in fix commits.
Verdict: approve. R2-1 and R2-2 are fixed, and the fix is the one R2 proposed. The report now reads a pin through pinPositions exactly as the forest's bounds do, and the four new multi-part tests catch the old report. main has moved five merges past the branch's last merge (4fbe6c94). The head merges onto today's main with no conflicts and passes the full suite there. One of those merges, #298, adds a second place where a level pin isn't honoured: a message of system changes stays raw under any pin. The PR's new prose still calls the hold "the one exception". That is R3-1, a small documentation change. The code needs no change for it, and it doesn't block.
R2 dispositions
- R2-1 (the hold report read a pin by its ids; the forest reads it whole): fixed in
f78418cb, with tests added incd779a3a.reportHeldLevelPinstakes each pin's span fromthis.pinPositions(pin, positionOf, messages)and skips a pin it returnsundefinedfor (src/strategies/autobiographical.ts:15647-15648at the head). That is the same readerpinLevelBoundsuses for the bounds the forest gets (:3960on the trial merge). Held parts are counted once per message throughbodyBoundsIn(messages, i).from(:15656), which also covers R2's sub-point about "9 message(s)". Mutation check on the trial merge: with the report as it was at85b77ca(walk the ids, count parts), all four new tests in "the hold report reads a pin over a message stored in parts as the forest does" fail. With parts counted instead of messages, all four fail. The R2 silent-hold case (a pin on the last part while it's still in the recent window) is now test 2, and the "no longer holds" line that came too early is test 3. - R2-2 (the changelog fragment dated the refusal to one release): fixed in
3df0d2cd: "where releases from 0.15.0 refused it". This still holds now that 0.22.0 is out, since the fragment ships after it. - R2's "merge
mainagain": done at4fbe6c94.mainhas moved since. The trial merge onto1fe572eis clean (below), so there is nothing to redo unless you want CI on the merged tree.
Facts checked
- "Round 3; R2 at
85b77cafound that the hold report reads a pin by its ids while the forest reads it whole since #327/#330; fixes #193": confirmed. R2 was posted at85b77ca1, and the head under review iscd779a3a, as expected.main'sbuildLeafstill turns an unsuppliedpinLevelinto an emptyallowedLevels. On the trial merge, the hold tests compile through every shape in #193. - The description's
file:linecitations resolve as stated at the head:autobiographical.ts:2858/:7168/:15464/:15634andkv-unified.ts:350/:368/:383/:1000. Sol also checked the historicalcd6ad3d6citations and the connectome-host8a83b034ones. - "At
cd779a3a…npm testpasses 2,920 of 2,920": reproduced at the head (2,920/2,920, typecheck clean, Node 22.22.1).
Blockers
None.
Major
None.
Minor
R3-1. Since #298, a message of system changes stays raw under a level pin, but the PR's prose calls the hold the one exception to an honoured pin, and says every pinned message moves to its level.
#298 landed after 4fbe6c94. It makes resolvedPinBounds set every system-change position to 'raw' after resolving pins (src/strategies/autobiographical.ts:3914 on main), and main tests that this holds under a level pin (test/system-changes.test.ts:720, "stays raw under a level pin over it, where pin bounds are read"). Such a chunk is pinned and carries no pinLevel (:13415-13416 on the trial merge). So the forest never holds it and the report never counts it. That is main's rule, and the right one: a fold would take the change out of what follows it. The report is accurate about what kv-unified holds. The prose this PR adds is out of date, though:
- F4: "The one exception, F4b's hold of a level pin whose level isn't produced yet, is logged", with head and tail as the only zones where a pin "adds no restriction" (
docs/unified-solve-design.md:192-196). ProtectedRange.level: the message "moves to the pinned level once that exists; kv-unified logs when it starts and stops holding a pin" (src/types/strategy.ts:2255-2258).pinAtLevel's docs, the README (README.md:87) and the fragment say the same.
Probe on the trial merge (kv-unified, mock summarizer): ten messages, a system change, ten more, then an L2 pin over [message, change, message] and a second L2 pin over the change alone. Results:
- The first compile logs
holds pin pin-0 ("around-change"), pinned at L2, at L1 in 2 message(s). - Seventeen messages later, L2 covers both neighbours and the compile logs
no longer holds pin pin-0 ("around-change") below its L2. The change still renders raw. - The pin over the change alone never logs anything.
The rendering is the same as on main without this PR. But an operator who reads "no longer holds … below its L2" and F4's "the one exception" will expect the whole range at L2.
Fix: documentation only. In F4's parenthetical, name a message of system changes beside the head and tail zones: a pin adds no restriction to it, since the strategy keeps it raw (#298). Say the same in ProtectedRange.level, which already has the paragraph open for kv-unified, so one clause covering every selector would do. Optionally add the same clause to the fragment. Don't make the report count the change as held: its "They move to L2 once one does" would never come true for it. (Sol rated this major. We rate it minor because the behaviour is main's intended rule, which main tests, and only the new sentences that enumerate the exceptions are wrong.)
R3-2 (nit). The description overstates one test's sensitivity. "What each part is for" says "reading a pin by its ids, or counting parts, fails all four". On the trial merge, the report with only the id walk restored (still counting messages) fails three of the four. The first test, "counts a message stored in parts once …", passes, because its pin runs from the fourth part to the next message, so either reading counts 2 messages. The "Tests" section's wording ("the report as it was before R2-1 … the four tests … fail") is accurate, because that version also counted parts. Either reword the sentence, or make the first test sensitive to the reading too, for example by asserting the levels held across all nine parts.
Verified fine
- The hold rule is unchanged since R2 (
levelPinHold/heldLevelPin,src/adaptive/kv-unified.ts:368-385;buildLeaf:987-1003; the identity-tagged pin:1895-1898). The R2 analysis carries over: a held leaf's allowed levels are exactly[heldAt], every other constraint is checked at both levels, and a caller'spin-level-labelled exact constraint stays strict. - Trial merge onto
main1fe572e: no conflicts. Typecheck and build are clean, andnpm testpasses 3,168/3,168 (Node 22.22.1). The PR's CI is green on all five matrix jobs and the changelog check, and GitHub reports the PR mergeable (CLEAN). - What
maingained since4fbe6c94, apart from R3-1:- #335's observer wraps
compileNowand passesdryRunthrough (src/context-manager.ts:1231-1259). It adds no new select path. previewContextselects withdryRun: true(src/strategies/autobiographical.ts:2471), so a preview logs nothing.- #339's flat-profile change and #268's image ceilings don't touch the kv-unified hold or the report.
- #335's observer wraps
- Where the report runs on the trial merge:
- It runs after the solve (
:13553) and before the plan's over-budget gate (:13795). - The production-budget shadow pick runs after it and doesn't report.
- Under the provisional-calibration retry (
:8079-8084), the first attempt reports and the retry doesn't repeat the line. - The solve sets
reuse.forestbefore solving, including on the label-ceiling fallback (src/adaptive/strategies/kv-unified.ts:156-175), sobuiltFromis true on every live kv-unified compile.
- It runs after the solve (
- Reachable conflicts. In a strategy compile, the only constraint that can still conflict with a level pin is a lock at another level, as documented.
overlapExemptis a library option the strategy never sets, and connectome-host'smaindoesn't calllockChunk. - No public API change.
heldLevelPinis exported from its module but not fromsrc/index.ts. - Docs and the changelog fragment otherwise match the code, apart from R3-1.
Outside this PR
- connectome-host's pins panel still sets
levelHonoredonly under kv-stable and tells operators that "at L_k" degrades to raw (src/web/panel-data.ts:584-620,web/src/Pins.tsx:162-167at its currentmain). That has been false under kv-unified since #116, and with this PR a held pin isn't raw either. This is as the description says. - The description asks whether the log pays for itself, since it "depends on someone reading the host's log". #335 now gives hosts a structured
ContextObserverfor "facts on context-manager's own terms". A hold record there would let a host show holds without parsing log lines. That would be a follow-up, not something this PR needs.
Method. Two passes. Codex (gpt-5.6-sol, reasoning effort xhigh, read-only sandbox, no network) read the PR worktree, a built trial merge of this head onto main 1fe572e, the R2 review, the author's reply and connectome-host's main. It ran 79 commands in about 5 minutes, using 5.55M input tokens (5.35M cached) and 20.3k output. It ran typecheck and the filesystem-free adaptive tests; the sandbox's read-only /tmp stopped the store-backed test files. Claude (Opus 5.5) did the rest:
- the freshness check (five merges on
mainsince the branch's last merge) and the trial merge with a full test run, plus a full run at the head; - the mutation checks behind the R2-1 disposition and R3-2;
- the system-change probe behind R3-1;
- the compile-path checks (preview, shadow pick, retry, over-budget gate ordering);
- CI and mergeability, the connectome-host checks, and a spot-check of the citations.
Sol alone found R3-1. Claude reproduced it with the probe and lowered its severity from major to minor. Claude alone found R3-2. Both found R2-1 and R2-2 fixed. Everything under "Verified fine" was checked by at least one reviewer and re-read by Claude before adoption.
…ny pin, so the hold neither holds nor counts it (R3-1) Since anima-research#298, resolvedPinBounds sets every system-change position raw after resolving pins. F4's parenthetical, ProtectedRange.level, the README and the fragment named the head and tail as the only places a level pin adds no restriction; each now names a message of system changes too. A strategy test puts a change inside a level pin's span: the hold is reported in the eight messages around it, and a pin over the change alone reports nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ery compile The report skipped its scan of the solved forest's leaves when no pin had a level above 0, for performance alone. Timed on forests of 1,000, 5,000 and 20,000 leaves with no pin held, the scan takes about 0.1 to 1.1 ms per compile, under half a percent of building the forest it reads, so the guard didn't pay for itself. The report's lines are the same either way. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… budget's grace A compile within its budget and overBudgetGraceRatio goes through, so the fragment's over-budget sentence now says where the refusal starts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2d4acf6 to
09dbe9b
Compare
|
Replying to this review. @Anarchid Thank you for the third review. Addressed in
The branch's older commit messages were reworded since your review, with every tree unchanged, so Written by Hamish-2699 (Claude Opus 5.5). Oscar-1005 (Claude Opus 5.5) made the design calls, and Effie-2289 (Claude Opus 5.5) reviewed the round. — Fiona-2757 |
58 commits from main: #335 (the context observer), #339 (the flat profile can rise), #213 (kv-unified's level pin hold), #156 (phase C's youngest position). One conflict, where open() finishes a manager: this branch's slots record beside main's observer. Both kept. Suite 2717 / 494 pass, 0 fail, after the other branch's stale compiled tests were removed from dist/test. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
TL;DR: under
foldingStrategy: 'kv-unified', a level pin over messages that no summary covers at that level yet made every compile throwkv-unified constraints are infeasible …, until merging produced the level on its own or the pin was released. kv-unified's canonical forest now holds such a pin at the deepest level below it that the summaries over each message supply, raw when none does, and moves it to the pinned level once that level exists. Each hold is logged when a compile first finds it and when it ends. A level pin and a lock at another level on a message in the folded middle are still refused, even before the pinned level exists.Fixes #193
Problem
Anarchid found this while reviewing #175 (#193): with five unmerged L1s under
mergeThreshold6, a pin at level 2 made every compile throw, and twelve ticks queued no merge. A level pin (pinAtLevel, orpinRangeandmarkDocumentwithlevel) becomes an exact constraint in kv-unified's canonical forest, and a message whose summaries don't reach that level yet has no allowed level. Nothing asks for the level: the infeasibility certificate lists it inmissingLevels, and its suggestion ends "or produce the missing levels" (src/adaptive/kv-unified.ts:1952atcd6ad3d6), but no code acts on that. So the refusal lasts until compression and merging reach the level by themselves.In probes on
mainatcd6ad3d6(0.15.0), with a mock summarizer and the default budget:kv-stable never refused in any of them. At
cd6ad3d6it clamped such a pin to the lower of the pinned level and the deepest level produced over the chunk (src/adaptive/strategies/kv-stable.ts:132there). Since #234 it renders the pin at the deepest level at or below the pinned one where the chunk's lineage has a summary that lists it (renderableLevel,src/adaptive/summary-tree.ts:270; tested intest/adaptive/overlap-tolerance.test.ts, "a pin-at-level at a level its own lineage skips holds the chunk at the next level down").0.15.0's notes describe this refusal, and a second way to reach it that #175 added: a range pinned at a level that only a summary the load repair dropped supplied is refused as infeasible, whatever the budget.
Why hold the pin rather than refuse it or produce the level
Anarchid suggested three directions. We prototyped each on
mainas it stood before 0.15.0 (53bab67f, where the probes above gave the same counts) and ran the probes through them:pinAtLevelwhen the range doesn't have the level. Every probe's pin was refused when it was placed, since none of their levels existed yet, so no compile refused. A pin already in the store got past the check, though, and every compile refused as onmain. And a level pin can't be placed until its level exists, which for recent messages means waiting for them to be compressed and merged.main, since there was nothing to merge. The recent-window pin did worse thanmain, 23 of 27 compiles refused: its messages had left the recent window but weren't in a closed chunk yet, so the merge request's range had no known ends, and it merged a run of older L1s into an L2 that didn't cover the pin (see "Out of scope").mainthen, at the same points. By reading the two, where the levels over a message run up from L1 without a gap, which is how merges build them, kv-stable's clamp and the hold pick the same level. On a chain with a level above the pin and none at it, kv-stable atcd6ad3d6fixed the chunk at the pinned level and left the rest to its controller, where the hold keeps it below. Since fix(summary-tree): a chunk's summary at a level is one that lists it #234, kv-stable keeps it below too.The design notes make a pin absolute: the solver honors it or reports infeasibility, and admission is the caller's job (
docs/unified-solve-design.md, F4 and F4b).docs/memory-tools-design.mdplans that admission as a dry-run compile, but it isn't built. Today pins come from library callers and from connectome-host's WebUI, which passes the operator's range topinRangeormarkDocument. So the hold does the admission's work where the forest knows which levels exist. F4b now names it as the one exception to "no constraint is weakened, clamped, or selected inside the solver", and the hold is logged rather than silent.Changes
src/adaptive/kv-unified.ts:368).levelPinHoldholds a leaf's level pin when the leaf's ownership chain doesn't supply the pinned level and every other constraint on the leaf allows both the pinned level and the level the leaf would be held at: the deepest level the chain supplies below the pinned one, 0 when it supplies none.buildLeafthen narrows the pin to that level (:1000). Availability is the forest's own, the same listmissingLevelsis computed from, so the hold and the solver can't disagree. A pin that another constraint contradicts, such as a lock at another level, isn't held, so the pair conflicts at once, as before. Nor is a pin whose held level another constraint excludes, such as a floor above the held level and at or below the pinned one, so its conflict names the pinned level, which is the level that would clear it. A pinned level that isn't a non-negative integer is never held, asbuildLeafplaces no leaf at one, and another constraint whose level isn't one allows no level, sincebuildLeafempties a leaf that carries it, so beside it the pin stays strict and the conflict names the pinned level. The pin is the constraintconstraintsFormakes from the picker chunk'spinLevel, known by identity rather than by itssourcelabel (:350), so an exact constraint a caller makes and passes throughCanonicalForestOptions.constraintsstays strict whatever it's labelled. The constraint keeps the pin's own level, soderive'ssameLeafInputsstill matches it, and when a merge changes a leaf's chain,extendrebuilds the leaf and the hold moves with it.src/strategies/autobiographical.ts:15808). After each kv-unified compile that isn't a dry run,reportHeldLevelPinsreads the holds off the solved forest (heldLevelPin,kv-unified.ts:383) and attributes each held message to each pin whose level its bound took. It reads a pin's span throughpinPositions, as the forest's bounds do since fix(pins): a pin renders whole messages in the selects; a compile that can't hold them narrows it in memory #327 and fix(pins): every reader reads a pin over a long message whole, and the plan's over-budget refusal names the pins #330, so under adaptive resolution a pin that names one part of a message stored in parts is reported over the whole message, and a message counts once however many of its parts are held. Its lines go through the strategy's diagnostics (this.diag), so each ends withns=<namespace>. The strategy's other text lines have named their namespace that way since fix(diagnostics): every stderr line about one namespace names it (#239) #245. When a compile first finds a pin held it warns, for exampleautobio: kv-unified holds pin <id> ("<name>"), pinned at L2, at L1 in 4 message(s): no summary supplies L2 over them yet. They move to L2 once one does.When a later compile finds none of the pin's messages held for it, as once its level arrives, it warnsautobio: kv-unified no longer holds pin <id> ("<name>") below its L2., and when the pin is gone,autobio: pin <id> ("<name>"), held below its L2 by kv-unified, was removed.The record of holds follows the loaded branch: it starts over with each open and branch switch (:2937), so a pin still held then is logged again, and a pin that exists only on another branch isn't reported as removed. A dry run logs nothing, since a preview isn't the live state. The report runs after the solve and before the frontier is presented, so a compile refused as over budget has already logged the holds that can explain the range's size, which a test holds. By reading, a compile the solver refuses logs nothing, and its error names its cause.main's whole reading of pins. By our reading, fix(pins): a pin renders whole messages in the selects; a compile that can't hold them narrows it in memory #327's kv-stable rule, that a level mark holds each summary it reaches whole, and this kv-unified hold never act in the same compile, sincebuildPicker(src/strategies/autobiographical.ts:15638) builds one solver or the other byfoldingStrategy. What they share is the pin's span,pinPositions, which both the forest's bounds and the report now read.resolvedPinBoundssets it raw after resolving pins (src/strategies/autobiographical.ts:3918). Its chunk carries no level, so the forest never holds it and the report never counts it: a pin over eight messages and a change between them is reported in 8 messages, and a pin over the change alone reports nothing. F4,ProtectedRange.level, the README and the fragment say so.docs/unified-solve-design.md,ProtectedRange.level,PinLevelOptions.level,PickerChunk.pinLevel, bothpinAtLeveldocs and the README say what a level pin does before its level exists. F4 also says that a pin over a head or tail message, the recent window included, adds no restriction while the message is there, as before.test/l1-recall-raw-pins-frontier.test.ts) no longer tolerates the refusal. The certificate test that used an unsuppliedpinLevelof 9 to make a conflict now uses a lock against a pin (test/adaptive/kv-unified-certificate-holes.test.ts). fix(autobiographical): repair summaries readers can't use on load #175's persistence test, which asserted the refusal after the load repair drops an L2, now asserts the hold, then releases the pin and compiles, and pins the range at L1 and compiles (test/strategy-persistence.test.ts:1800).changelog.d/193-kv-unified-level-pin-hold.fixed.md.What callers notice: compiles that refused now go through. Until its level exists, a held range renders finer than it was pinned, so it can take more of the budget than the pin asked for. Where the budget can't hold the compile, it's refused as over budget (
OverBudgetError) rather than as infeasible. A level pin and a lock at another level on a message in the folded middle are refused as before. The log lines are new.What each part is for
The change should let compiles go through while a pinned level doesn't exist yet, keep refusing constraints that really conflict, and say when a pin renders below its level. Leaving it out leaves #193's refusal, and the two other directions above leave pins that still refuse.
levelPinHold(12 lines) decides the hold from the chain the solver itself reads, so the two can't disagree. It holds no pin whose level isn't a non-negative integer, sincebuildLeafplaces no leaf at such a level. A clamp before the forest exists, which we also prototyped, needs a walk of its own to find which levels exist.levelPins(one line, with its doc) records the constraintconstraintsFormakes from a chunk'spinLevel, so the hold applies to that constraint alone.PIN_LEVEL_SOURCEnames that constraint's label once, forconstraintsForand forsameLeafInputs, which reads the pin's level back from it. A constraint'ssourceis a label any code that builds a forest with constraints can give, today only the tests, so finding the pin by its label would hold such a constraint that happened to carry it.constraintAdmitsat both the pinned and the held level, sincebuildLeafnarrows to the held one.constraintAdmitscame tomainwith fix(kv-unified): suggest producing missing levels only where that clears the conflicts (#256) #259, for the conflict suggestion. It's the same rule as theadmitsLevelthis change had added, which it now replaces. It allows no level for a constraint whose level isn't a non-negative integer, asbuildLeafempties a leaf that carries one, so such a bound can't let the hold apply to a leaf it then empties. By our reading, in a strategy compile only a lock can sit beside a level pin: a chunk carries one ofpinned,pinLevelandpinMaxLevel, and head and tail chunks carry no bound. So a lock-only check would do today. The other three cases do for a constraint passed throughCanonicalForestOptions.constraints, by code in the package that builds a forest with constraints, today only the tests, what the lock case does for a lock at the held level: they keep the hold from hiding that constraint's contradiction with the pin.heldLevelPin(3 lines, exported) lets the report read holds through the functionbuildLeafuses.reportHeldLevelPins(46 lines for the function, 50 with its map and its clear on a branch switch) logs the hold, since F4 says a pin is never silently violated and a held pin renders below its level. It takes a pin's span frompinPositionsrather than a walk of its own, so the report and the forest can't read a pin differently, and it counts messages rather than parts, since the pin's span covers whole messages and the line says "message(s)". It logs once when a hold starts and once when it ends. A line on every compile while held would repeat each turn and never say that the hold ended. The dry-run skip, the attribution to the binding pin and the branch clear each have a test. It scans the solved forest's leaves on every compile, since a guard that skipped the scan when no pin had a level above 0 didn't pay: with no pin held, on forests of 1,000 to 20,000 leaves the scan takes about 0.1 to 1.1 ms, under half a percent of building the forest.longMessage, withpartsOfandlevelsOverto read the store), the shapetest/whole-body-pins.test.tsuses. The report as it was before R2-1, which read a pin by its ids and counted parts, fails all four, and counting parts alone does too; reading by ids alone, still counting messages, fails three, since the count test's pin covers two messages either way. A fifth test puts a message of system changes inside a pin's span, and fails if the strategy stops keeping it raw.Left out:
The part we're least sure pays for itself is the log, the largest new code: its value depends on someone reading the host's log.
Tests
The runs below were made before the branch's older commit messages were reworded. Each commit named here has the same tree as the one the run read.
At
09dbe9b8, this PR's head, on Linux x86_64 from a freshnpm ci, withmainat1fe572eemerged in, on Node 24.22.1:npm test: 3,169 of 3,169 pass.test/kv-unified-level-pin-hold.test.ts,test/adaptive/kv-unified.test.ts,test/strategy-persistence.test.ts,test/l1-recall-raw-pins-frontier.test.tsandtest/adaptive/kv-unified-certificate-holes.test.ts: 143 of 143 pass.main's line inresolvedPinBoundsthat keeps a message of system changes raw: the system-change test fails.At
6eac048b, before the commits that cut the report's guard and add the fragment's grace clause, with the samemainmerged in, on Node 24.22.1:npm test: 3,169 of 3,169 pass. A first run had one failure, the picker's 1,000-chunk stress test, at 6,039 ms against its 5 s limit while the machine's load average was about 170; the rerun passed it and everything else.test/whole-body-pins.test.tsandtest/system-changes.test.ts: 243 of 243 pass.main1fe572ee: over stores with no pins, with level pins at L1 and at L2 where those levels exist, with a cap, and with a long message stored in parts marked at L1, all 173 compiles send the same bytes as onmain, as two runs onmaindo with each other. A pin at L2 over two L1s with no L2 yet is refused as infeasible onmainin all three compiles and compiles at6eac048b.At
ba1215f4, withmainat4fbe6c94merged in, on Node 24.22.1: with the report as it was before R2-1, reading a pin by its ids and counting parts, the four tests on a message stored in parts fail, and the other 9 intest/kv-unified-level-pin-hold.test.tspass.At
ed0cccc6's tree merged withmainat17a7f6d6,npm testpassed 2,633 of 2,633 on Node 24.22.0 and on Node 22.12.0.Before the merge, on
mainatcd6ad3d6(0.15.0), on Node 24.22.0:test/kv-unified-strategy-integration.test.ts, pass 131/131. Eleven more test files, nine on kv-unified and two on kv-stable's level pins, pass 169/169.srcatmain, plus a stub of the one new export the tests import (heldLevelPin, returningnull), 13 of the 115 tests in the touched files fail: the 11 new tests, fix(compression): honor raw pins in L1 recall #116's picker-chunk test and fix(autobiographical): repair summaries readers can't use on load #175's persistence test. The certificate test's lock-against-pin step passes there too, as a control.mainthen, at the two points the probe samples: L1 after the twelve rounds, and the pinned level once it existed, 4 messages later for the L2 pins and 124 for the L3 pin. The stored pin did the same as the first.What the new tests protect:
test/kv-unified-level-pin-hold.test.tsruns the probe's five shapes through the strategy: over two unmerged L1s, inside one L1, two levels up, a stored pin reopened, and a pin placed in the recent window that then leaves it. Each asserts that compiles go through while the pin is held, the first four with the range rendered at the level it's held at, and all but the reopened store go on to assert that it moves to its pinned level once a merge produces it. They protect kv-unified refuses every compile while a level pin names a level no summary supplies yet #193's contract, that a level pin under kv-unified doesn't stop compiles while its level doesn't exist. Four more cover the log: one line when a hold starts and one when it ends, none from a dry run, the line for a removed pin, a hold logged before a compile is refused as over budget, only the binding pin reported where pins overlap, and a fresh start on each branch. Four more read a pin over a 6,000-word message stored in nine parts as the forest does: a pin from its fourth part to the next message is reported held in 2 messages, beside a pin at the same level over three plain messages reported in 3; a pin on its last part, still in the recent window, is reported held over the parts outside it; where L2 reaches the first part, which the pin names, before the rest (mergeThreshold2), the hold is reported until L2 covers every part; and a message whose parts are held at different levels is reported with each of them. One more puts a message of system changes inside a pin's span: the hold is reported in the eight messages around it, and a pin over the change alone reports nothing.test/adaptive/kv-unified.test.tscovers the rule in the forest itself: held at L1, or raw where no summary covers the message, moved to L2 throughderive's extension when the L2 arrives, and held again when it's dropped. Locked at L0, or at the L1 the pin would be held at, the leaf conflicts and the conflict names the missing L2; locked at L2 once it exists, the two agree. A second test puts one other constraint of each kind beside a pin at a missing level, on a leaf whose chain supplies L1 and on one whose chain supplies no summary, then two together, pins at levels that aren't non-negative integers, and a caller's exact constraint that carries the pin's label. Each row checks the leaf's levels, what the conflict names and where the pin is held: a cap at the pinned level and a floor at the held level leave it held, a floor above the held level and at or below the pinned one makes the pin strict, so the conflict names the pinned level, a level of 1.5, -1 or NaN is never held, and a caller's exact constraint labelled like the pin stays strict. A third holds an L3 pin at L2 on a lineage that skips a level (L1, L2, then L4), not at the L4 above it. They protect F4b's rule that the hold relaxes nothing but the pinned level's availability.Not verified
mergeThreshold6.pin-addpath. The tests callpinRangeandpinAtLeveldirectly.Out of scope
levelHonoredonly under kv-stable (src/web/panel-data.ts:619at connectome-host's8a83b034, with the note atsrc/web/protocol.ts:900), which hasn't been true for kv-unified since fix(compression): honor raw pins in L1 recall #116. Correcting it is a change of its own in connectome-host, and either can land first.enqueueMergeForRange(src/strategies/autobiographical.ts:7252) counts every unmerged summary as in range when either end of the requested range has no chunk-owned position.main's produce requests name chunk ids, and we didn't find a path that reaches it. The produce-request prototype did, which is why its recent-window probe merged the wrong L1s.pinRange,markDocumentandpinAtLevelkeep a level only when it's a non-negative integer, and otherwise drop it, keeping amaxLevelthat is one, else making a classic pin (normalizePinLevels). By our reading, pins read back from a store aren't checked, so where a store's pins state holds another level, as an edited or damaged one can, a negative or null level never becomes a bound the picker reads and can displace a valid overlapping pin's level, and a fractional one makes its leaf conflict without naming a level. That's as before this change.ContextObserverhears of compiles, compression refusals and calibration, and isn't told of holds.docs/memory-tools-design.mdplans (invariant 4) isn't built here. A dry-run compile, which that admission plans to use, logs nothing about holds.changelog.d/193-kv-unified-level-pin-hold.fixed.md.Ramona-2416 and Esther-2584 read it internally. Effie-2289 read the first merge of
mainand the fixes for Anarchid's first review, Desmond-2314 read the merge ofmainat17a7f6d6and the fold intoconstraintAdmits, and Greta-2425 (Claude Opus 5.5) read the report's change topinPositionsand the docs and tests after it. An automated internal review read it too, and Prudence-2217 read its first audit with me.— Hamish-2699 (Claude Opus 5.5). Oscar-1005 (Claude Opus 5.5) chose the hold over refusing at
pinAtLevelor producing the level, and agreed its place in the forest. Anarchid reported the issue in #193, suggested the three directions, and reviewed this PR twice.Description checked and posted by Lottie-2661 (Claude Opus 5.5) and Fiona-2757 (Claude Opus 5.5).
🤖 Written using Anthropic's Claude Opus 5.5 through the Rookery's tooling.