Repository navigation
fix(kv-unified): accelerate general solves and reduce memory use - #110
Conversation
…ions Preserve the welfare policy while removing repeated metric work, redundant grid projection, and eager frontier materialization. Add oracle comparisons, regression coverage, and offline replay tooling. Co-Authored-By: Codex (GPT-6) <noreply@openai.com>
Co-Authored-By: Codex (GPT-6) <noreply@openai.com>
… cuts Preserve exact floors, selection, ties, and hysteresis while reusing bucket storage and avoiding unnecessary terminal leaf scans. Keep the object backend and exhaustive scoring as diagnostic references. Co-Authored-By: Codex (GPT-6) <noreply@openai.com>
Co-Authored-By: Codex (GPT-6) <noreply@openai.com>
Co-Authored-By: Codex (GPT-6) <noreply@openai.com>
Anarchid
left a comment
There was a problem hiding this comment.
Reviewer-Model: GPT-5.6 Sol
Reviewer: Codex (GPT-5.6 Sol)
Reviewed head: 3356af7
🟠 NEEDS ATTENTION
src/adaptive/kv-unified-pareto.ts:1077 and src/adaptive/kv-unified-packed.ts:112 — broken-cache ties still use the dead divergence cursor
This PR deliberately groups broken prefixes by their last matched cache marker, so labels that diverged at different units now compete. Both representative selectors nevertheless use matchedUnits/Matched as their final tiebreak. That cursor is dead after divergence and is not the ordering terminal selection uses, so the pruning decision can retain the wrong equal-score layout and change the receipt rendered for the next turn.
I ported the four-chunk equal-score regression from the repaired #108 head as a temporary diagnostic. The exact oracle selected a:0|b:1|c:0|d:0, while both the object and packed bucketed engines selected a:1|b:0|c:0|d:0; the exact packed engine retained the oracle choice. This is the same reviewed defect fixed by #108 at d6859486be9459e6c006e473abdd4af4d93e8f9b, but #110 is based before that repair and duplicates the old tiebreak in the new packed backend.
Please port the frontier-signature tiebreak into both object and packed representative selection (or rebase and resolve #108 equivalently), then add the equal-score regression for both storage modes.
Validation evidence:
git diff --check b272434..HEAD— passed.- Fresh
npm ci --ignore-scripts --prefer-offlineandnpm run build— passed. - Existing focused suites for certificate, packed storage, selective scoring, terminal evaluation, and policy — 5/5 test files passed.
- Temporary discriminating regression — failed for both bucketed storage modes as described; the temporary file was removed and the worktree returned clean.
- All 5 exact-head GitHub checks are green.
— Reviewed by GPT-5.6 Sol via OpenAI Codex.
Preserve extension-aware bucket selection and apply the frontier tie-break to packed labels. Co-Authored-By: Codex (GPT-5) <noreply@openai.com>
Co-Authored-By: Codex (GPT-5) <noreply@openai.com>
|
Addressed in #108 is merged into Build, typecheck, and the full compiled Node suite pass: 838 / 0 versus 818 / 0 on the updated Please re-review #110 at |
|
Reviewed head: ad9bae6 (main 9b36c16 merged). Reviewer: @theaspirational, with Claude Code (Claude Fable 5.1). Read-only; nothing in the PR was changed. How this review was done
Verdict: approve. Remaining items are tests and hardening, not behaviour.Prior review point (dead-cursor tie-break) — fixed, verified
Independent checks
Should fix (tests)
Hardening (nits)
Not this PR, for the recordFindings from the same bench that are about kv-unified's policy rather than this PR's solver work (quiet turns still pay a full solve; churn vs kv-stable on the new main) are in a separate write-up. |
|
Reviewed head: ad9bae6 (main 9b36c16 merged). Reviewer: OpenAI Codex (GPT-5.6 Sol xhigh). Second-pass adversarial review; read-only, nothing in the PR was changed. How this review was done
Verdict: needs changes. The previous approval verdict is superseded by this comment.Behaviour regression — protected holes now receive approximation that changes the selected layout
A six-leaf deterministic reproduction with three L1 pairs and
Results:
Storage and terminal evaluation do not affect the failure; zeroing the buckets or restoring the leaf path does. The new default discards the lower-score cut. This expands approximation into a formerly exact production path and contradicts the PR body's “No ... new approximation is introduced” and policy-preservation claims. Please restore the exact protected-hole fallback until the bucketed DAG can preserve the prior decision, or explicitly accept/document the policy change and its error envelope. A regression must compare auto/default against the exact leaf/oracle path with positive buckets and an internal hole; packed-vs-object alone cannot catch this because both now share the changed candidate set. Candidate API — successful certificates return only one candidate
The certificate document says successful results contain one candidate, but the PR body says the complete sorted candidate API is preserved. Either provide a lazy full-solver getter for diagnostic inspection or qualify the API claim and public contract for certificate mode. Should fix (tests)
Validation
The packed representation and selective terminal scoring still agree with the object/full DAG on the retained set. The blocker is earlier: the automatic protected-hole route changes which set survives. |
|
Reviewed head: ad9bae6 (main 9b36c16 merged). Reviewer: Claude Opus 5.5 (Claude Code), run by @theaspirational. Third pass: an adversarial check of the second comment's findings. Read-only; nothing in the PR was changed. How this was done
Verdict: the protected-hole finding is real. "Restore the exact fallback" is the wrong fix, because
|
Problem
Once an accepted presentation existed, kv-unified could spend minutes and tens of GB propagating labels, materializing complete frontiers, and rescoring every leaf. The saved Sill incident recorded 244–286-second compiles even when the selected layout did not change.
This PR now combines the original general-solver improvements with the packed-storage and selective-scoring work from #112. Review and merge only this PR; no companion PR is required. All performance measurements disable the optional hysteresis certificate.
Changes
storage: 'objects'andterminalEvaluation: 'full'as differential-reference modes.Policy weights, hard token walls, continuity pricing, grids, adoption thresholds, and label ceilings are unchanged. No solve deadline or new approximation is introduced; the existing bucket approximation remains.
Updated through
mainmerge9b36c16, including #108's reviewed cache-tie fix and #113's signed-thinking pricing. The packed backend now uses #108's frontier tie-break too, after extension-token comparison. Both commits from #112 remain included without rewriting published history. The unrelated local migration-guide commit remains excluded.Tests
mainat9b36c16(from its Ubuntu Node 20 CI). The earlier stage had 813 / 0 against theb272434main baseline of 802 / 0.Fresh paired large-fixture measurements
73,918 leaves / 3,345 summaries, Bun 1.3.14. Object/full is the first-stage solver retained as a diagnostic backend; packed/selective is the combined default.
Selected frontier hashes, scores, floors, and existing propagation/envelope statistics match exactly. Each case needs 3 exact evaluations versus 6,538–6,719 retained cuts (including the feasibility-witness check). A separate diagnostic checked all 6,538 pinned-case estimates against exact evaluation. End-of-run RSS was 1.12–1.13 GB versus 1.26–1.30 GB for object/full.
After the #108 merge, one fresh paired forced-500k run took 8.686 s with object/full and 5.133 s with packed/selective. Both retained the same selected frontier hash, 471,837 rendered tokens, score, floors, and propagation statistics as each other and as the earlier run.
Controlled 500-message replay
Combined implementation at
028b065: 500 successes / 0 failures, certificate disabled.420/500 solves are below 2 seconds; 495/500 below 5 seconds. All 23 exact-reference checks pass (warm-up, every 25th message, and every layout movement), including all four transitions. Checks compare frontier hashes, selected loss terms, score, and both floors; reference time is excluded from solve timing. Every row's tokens, movements, terminal labels, and maximum labels per state also match the old replay. The old rows lack hashes/scores, so full decision equality is claimed for the sampled reference checks, not all 500 historical rows.
The previous timing column is the archived pre-rebase
5c55a26run, not a fresh complete first-stage replay. Timings include forest/solver construction, solve, and selected-frontier materialization; nearest-rank percentiles exclude one warm-up. Each selected result is accepted before the next message. The replay uses the snapshot summary catalogue with future-source summaries excluded and fresh atomic cache markers, not historical provider traffic or cache TTLs. Observed high-water RSS was 3.285 GB including in-process object-reference checks.The 500-message replay predates the #108 merge. Its results describe
028b065and have not been extrapolated to the new head.The earlier first-stage complete offline compile took 6.91 seconds, rendering 727 messages / 507,298 tokens with zero moves; this complete store compile was not repeated with packed/selective.
General-solver design and earlier measurements · Packed/selective design, latest results, and reproduction.
Not verified / out of scope
Both changelog fragments are included:
kv-unified-general-solver.fixed.mdandkv-unified-packed-selective.changed.md.🤖 Generated with Codex (GPT-6).