Skip to content

Deferred review backlog rollup (PRs #14, #15, #17, #18, #22) — Medium + Low #26

@Anarchid

Description

@Anarchid

Rollup of deferred /roast items across recent merged PRs that haven't been picked up in follow-up commits. Critical and High items from the same audit are tracked as their own issues — see #23 (executeMerge branch-race), #24 (registerStates bare-catch), #25 (speculativeProduction default).

This issue exists so the long tail of Medium and Low items has some tracked surface. Pick any subset for a bulk-cleanup PR.

Medium

PR #14 — persistent and branch-aware strategy state

  • setMergedInto uses object-identity indexOf with silent miss. src/strategies/autobiographical.ts:787 does this.summaries.indexOf(entry) and bails on index < 0 without logging. Masks the symptom of switchBranch/fork race against in-flight executeMerge corrupts cross-branch state #23 (stale captured references) and is O(n). Suggest summariesById.get(id) after building the index once, or at minimum console.warn on miss.
  • No end-to-end persistence test through compressChunkHierarchical/executeMerge. test/strategy-persistence.test.ts exercises the persistence boundary only via TestableStrategy seed helpers — the production code paths aren't covered. Add a test that drives a real chunk-compress + merge through the persistence layer.

PR #15 — close 5 spec gaps

(PR #16 description explicitly punted findings 4–11 as "MEH follow-ups". None landed.)

  • No tests for getRenderStats, enforceBudget: false, or driveSpeculativeDrain termination. test/speculation-cap.test.ts exists but doesn't probe drain-on-no-progress termination; no enforce-budget* or render-stats* test file exists.
  • pinRange/markDocument accept any string as message ID without validation. Orphan pins become permanently inert with no error returned to the caller. Validate against messageStore.has(id) and throw / return failure.
  • Hardcoded 'claude-sonnet-4-20250514' in 6 sites. src/strategies/autobiographical.ts:1422, 1722, 1789, 2179, 2250, 3385. No DEFAULT_COMPRESSION_MODEL constant; default is two model generations behind current (Sonnet 4.6). Extract a constant; consider promoting to Sonnet 4.6 as the new default in the same PR.

PR #18 — adaptive-resolution design doc

(Doc-level deferrals; PR #19 implementing from this doc baked them in.)

  • §3.6 promises byte-faithful concatenation without noting the all-L0 precondition. Doc text at docs/adaptive-resolution-design.md:213, 227 reads as a universal guarantee; §3.6.1's rendering algorithm shows it only holds when all shards are L0. Add the precondition explicitly.
  • §4.2 claims fold makes "No LLM call" (docs/adaptive-resolution-design.md:399) — contradicts §3.8 where raiseGroup can trigger a background produce. Clarify or rephrase.
  • §3.8 doesn't describe locked-chunk-hole rendering in mixed groups. This is the V2 agent-driven-unfold path's primary case; design should spec it before V2 implementation starts.
  • §8 open questions never resolved in-doc. Tokenizer choice was effectively settled by PR Adaptive resolution: V1 scaffold (picker + chunker + strategies) #19 picking MessageStoreView.estimateTokens (chars/4); pins-interaction was implicitly settled but never written down. Either close the open questions or note the resolution.

PR #22 — chunk-boundary tool cycles fix

  • summariesById rebuilt O(n) at three sites. src/strategies/autobiographical.ts:1649, 1963, 2339. Same triple-rebuild pattern; consider a memoized accessor or pass the map into the helpers that consume it.
  • L1 lookup is linear scan over this.summaries at src/strategies/autobiographical.ts:1652, 2058. Add a summariesByLevel: Map<SummaryLevel, SummaryEntry[]> index, kept in sync in pushSummary/setMergedInto.

Low

PR #14

  • initialize() doesn't reset _cachedHeadStartIndex on branch switch. Only resetHeadWindow resets it (src/strategies/autobiographical.ts:3331). Cache-key luck currently masks this; would manifest as wrong head-window slice on a branch with a different message count.
  • ContextManager.switchBranch(branchId: string) param name remains branchId. The API is now name-keyed (per the PR's stated point that any caller passing the prior return value was broken); param should be branchName. src/context-manager.ts:376.
  • Docstring references nonexistent forkAt method. src/context-manager.ts:346.

PR #15

  • Dead defensive idx += needle.length || 1 in searchSummaries empty-text path. src/strategies/autobiographical.ts:677 — the outer if (query.text) already excludes the empty case.
  • buildRecallHeader uses String.replace with raw summary IDs as the replacement string. src/strategies/autobiographical.ts:3312-3319. $&, $1, $$ would expand. Latent footgun for any future ID scheme using $. Switch to String.replaceAll or a .split().join() pattern.
  • Dead isPositionPinned helper. src/strategies/autobiographical.ts:711.
  • Defensive-but-unreachable if (orig.type !== 'tool_result') continue in pruneToolEntries, around line 3754.
  • PR-body claim about FrontdeskStrategy not overriding select is imprecise. No docstring clarifies the "subclasses overriding select MUST call super" contract.

PR #17 — MessageStore.get

  • Vestigial _index: number parameter on internalToStored. src/message-store.ts:556. Two callers ignore the value.
  • Backwards-compat caveat for pre-fix chronicles never written. Pre-fix chronicles have internal.sequence === append-sequence rather than edit-sequence; reading old data still triggers the chained-branching bug. PR Drop append-then-edit in MessageStore; use chronicle's appendToStateJsonWithIdentity #21's MessageStore rewrite makes the +1 invariant moot but the caveat for users with existing chronicles was never documented.

PR #18

  • FoldOp.lower interface shipped even though non-monotonic strategies are V1 non-goal. Add explicit "reserved for V2" note at docs/adaptive-resolution-design.md:442.
  • §3.9 cache-cost framing understates the group-fold case. One fold collapsing N L1 entries invalidates more prefix than one boundary shift.

PR #22

  • Fixed-path test stores at project root. test/chunker-tool-boundary.test.ts and test/compression-shape-invariants.test.ts use hardcoded './test-...' paths. Switch to os.tmpdir() + randomUUID().
  • as any / as unknown as type laundering in test files. Introduce a __testHooks accessor on the strategy and a TestMembrane interface to drop the casts.

Origin

Audit run 2026-05-21 against the GitHub-visible review surface. Items here all flagged in /roast reviews by @Anarchid and never picked up in follow-up commits.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions