Skip to content

Deferred review backlog rollup (PRs #32, #62, #65, #79, #81, #85, #88) — Medium + Low #92

Description

@Anarchid

Second harvest of review items that were raised on PRs that then merged without them. Window: 2026-05-22 → 2026-09-08; anchors verified against main 5dbbc47 on 2026-09-08. Companion rollups exist in agent-framework, connectome-host and membrane; three items from this window are filed separately as #89 (Bedrock family regex, from #87), #90 (cross-path pass-order drift, from #32) and #91 (the viewFilter × derived-summary contract, from #54).

Pricing: plan prices what it does not emit

From #32 (tool-pair integrity)

Findings N2 and N3 from the re-review, both left for a follow-up that was never filed. N1 is #90.

  • N2 — the Phase 3b comment misattributes the trigger, and the warning slanders healthy stores. :9107 says the excluded-with-partially-dropped-children state "only arises from a store damaged mid-merge", which is contradicted eight lines up by the function's own Phase 3b header and by the first test in test/anti-redundancy-budget.test.ts, which manufactures the state on an undamaged store using nothing but researchL1BudgetCap: 0.3. What is corruption-specific is many simultaneous candidates, not the state. The warning at :9159 therefore prints "(store likely corrupted mid-merge)" at a healthy KnowledgeStrategy store with a tight budget and a couple of large legitimate covering L2s. Correct the comment, and make the warn state the fact (repair allowance exceeded, N summaries skipped) with both possible causes.
  • N3 — [tool call omitted] is the wrong epitaph for a relocated result. :10580: when relocation empties the source entry it is replaced with the Rule-A drop placeholder, but nothing was omitted and it wasn't a call — a tool result was moved to its correct position. This string is rendered context the model reads. [tool result moved during context repair] costs one string literal.

From #62 (branch-scoped message index)

  • Key the id index by {name, generation}, not branch name. Noted at merge as the residual edge. src/message-store.ts:191/206/215 still compares this.store.currentBranch().name, so delete-and-recreate-same-name is missed. src/branch-generation.ts already provides observeStoreBranch/markStoreBranchSwitch and is used by context-manager.ts; the message store has not adopted it.

From #79 (mint request preimages)

Four "minor / no action required" notes from the re-review.

  • buildPreimageEnvelope stores media blobs before the splice verification, so a failed verification leaves those blobs behind. Content-addressed and almost always already present via MessageStore, so harmless — recorded.
  • getMintRequestByHash's doc says a hash naming a non-request blob "throws" — verify whether it throws the typed MintPreimageMaterializationError on all paths or still leaks a raw SyntaxError from JSON.parse; either wrap it or soften the doc.
  • Text-only growth number belongs in the fragment. A full compression context is ~400–800 KB of JSON per mint; at tens of mints a day that's tens of MB/day per resident with the option on. One sentence so operators can size it — the knob's whole job is store size.
  • The trueHash !== requestHash divergence path stores and indexes under trueHash, so the summary's own key reads null. Correct (never readable-but-wrong), but a stderr line is its only trace.

From #81 (carrierPolicy)

  • Stale comment at :9412 — "text blocks — signed thinking must ride along here too" was an invariant and is now a policy default. Reword to point at carrierPolicy so the next reader doesn't take it as a promise 'live-strip' breaks.

From #88 (split-stitch L1 fallback)

  • Field doc should say a stitched L1 reads as N consecutive summaries, not one — the pieces are joined with blank lines and a later L2 over it will see that seam. The stitched metadata carries ranges and hashes for auditing; the shape just isn't stated where an operator reads it.
  • Orphaned doc comment. The three new protected fields carry two stacked doc comments; the first one's field was renamed and its comment was left above the wrong line.

From #65 (source order across id widths)

  • Orphaned provenance sorts at MAX_SAFE_INTEGER with a lexical tie-break. Accepted at approval as a reasonable fail-honest fallback for summaries whose source start is absent from MessageStore — recorded so it's a decision rather than an accident.

Method

Harvested from every PR merged into this repo in the window, by pulling review bodies, review threads and non-bot PR comments and separating "raised and then addressed in a later commit" from "raised, acknowledged, merged anyway". Line anchors are current as of 5dbbc47.

Two items from this window were checked and found already repaid by later work, and are not listed above: #38's test-fixture damage (fixtures now set compressionModel; test/compression-model-required.test.ts pins the fatal record) and the raw NUL bytes noted on #54 (src/adaptive/kv-cache-sim.ts, src/strategies/autobiographical.ts — both clean).

Related: #26 (previous rollup), #24, #25.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions