Skip to content

fix: verify source-message availability before committing decompression (#446) - #449

Merged
ranxianglei merged 1 commit into
masterfrom
2026-09-20_decompress-source-availability
Sep 21, 2026
Merged

ranxianglei merged 1 commit into
masterfrom
2026-09-20_decompress-source-availability

Conversation

@ranxianglei

Copy link
Copy Markdown
Owner

Problem (#446)

Block/range decompression could report Restored N message(s) and terminally deactivate the summary block even when the indexed original messages were absent from the history returned by the host (e.g. removed by native compaction or external deletion). The commit path treated a membership transition (byMessageId entry's activeBlockIds going empty) as proof of content restoration — computeRestoredMessages never even receives the fetched messages — so stats were decremented for phantom tokens and the only remaining coverage (the summary) was discarded irreversibly.

Repro from the issue confirmed against source: active block covering msg-gone, history lacking it → {"restoredMessageCount": 1, "restoredTokens": 100, "actualOriginalMessagesFound": 0, "restoredContentPreview": "", "blockDeactivated": true}.

Fix

Fail-closed pre-commit gate in createDecompressTool.execute (lib/compress/decompress.ts), placed after the toFile branch and before any state mutation, backed by two pure functions in lib/compress/decompress-logic.ts:

  • collectRequiredDecompressMessageIds(messagesState, targets, {full?}) — closed-form set of message IDs whose visibility this decompression would restore: entries exclusively covered by the target blocks (activeBlockIds.length > 0 ∧ ⊆ targets), minus the one-tier shield (union of effectiveMessageIds over the transitive consumed closure that syncCompressionBlocks would reactivate — non-user-deactivated consumed tiers). Full mode has no shield.
  • checkDecompressSourceAvailability(...) — splits that set against fetched-history IDs into available/missing.

Behavior:

Tests

  • 13 unit tests in tests/decompress-logic.test.ts (R computation: single/multi-target, one-tier shield, full nested, transitive closure, user-deactivated exclusion, empty membership → ∅).
  • 9 E2E regressions in new tests/decompress-source-availability.test.ts through the real createDecompressTool with controllable host history: all-missing abort (issue repro) + zero state change, ref-display in error, retry-after-abort, partial (lists only missing IDs), complete (count/stats/preview), nested one-tier success with consumed-tier raws absent (shielded), nested full abort + success, transient-fetch rejection.
  • Bug-detection proof (§5.7.3): with the gate temporarily removed, the all-missing E2E fails with the issue's exact phantom output Decompressed block b5. Restored 1 message(s) (~100 tokens); restored → green.
  • npm run typecheck clean; full suite 1312/1314 — the 2 failures are pre-existing sandbox issues (read-only /tmp: soft-block.test.ts import crash, inactive-block-decompress.test.ts hardcoded /tmp toFile path), reproduced identically on clean master.

Review

Dual-agent independent review (§5.3 code + §5.6 tests): both approve-with-nits. Applied: transient-fetch pinning test, ref-display test, nested stats assertions, stricter deep-flag assert. Documented as known limitation (DESIGN.md): one-tier nested commit does not verify the reactivated tier's summary-carrier compress message (carriers are hard-protected, never in byMessageId) — rare trigger, deferred beyond #446 scope.

No version bump (release-branch-only rule). No persisted-state-format changes; purely additive API surface.

Closes #446

…on (#446)

Block/range decompression could report 'Restored N message(s)' and terminally
deactivate the summary block even when the indexed originals were absent from
host history (removed by native compaction or external deletion): the commit
path treated a membership transition as proof of restored content.

Add a fail-closed pre-commit gate in createDecompressTool.execute (after the
toFile branch, before any state mutation) backed by two pure functions in
decompress-logic.ts:

- collectRequiredDecompressMessageIds: closed-form set of message IDs whose
  visibility this decompression would restore — exclusively covered by the
  target blocks, minus one-tier shielded IDs (transitive consumed closure that
  sync reactivates; full mode requires everything).
- checkDecompressSourceAvailability: splits that set against the fetched
  history IDs into available/missing.

Missing sources => warn log + explicit error naming targets, counts, and
missing refs; nothing is mutated, summary coverage preserved. Transient host
fetch failures still reject out of prepareDecompressSession before any
mutation, so the two failure classes stay structurally distinct. Because every
committed ID is verified present, restored counts/stats now reflect real
recovered content.

Tests: 13 unit + 9 E2E regressions (all-missing / ref-display / retry-after-
abort / partial / complete / nested one-tier / nested full abort+success /
transient-fetch rejection). Bug-detection proof: with the gate removed the
all-missing E2E fails with the issue's exact phantom output 'Decompressed
block b5. Restored 1 message(s) (~100 tokens)'.

Closes #446
@github-actions

Copy link
Copy Markdown
Contributor

📦 Built Plugin Artifact

Branch: 2026-09-20_decompress-source-availability (091e342)

Option A — Install from npm PR tag (recommended)

opencode plugin opencode-acp@pr-449 --global

Each push to this PR publishes a new version under the pr-449 npm tag.

Option B — Install from GitHub

opencode plugin "github:ranxianglei/opencode-acp#2026-09-20_decompress-source-availability" --global

Option C — Download artifact

  1. Download the artifact from the Actions run
  2. Extract the tarball and install:
tar xzf opencode-acp-pr449.tgz
cp -r package/dist ~/.cache/opencode/packages/opencode-acp@latest/node_modules/opencode-acp/dist
  1. Restart opencode to pick up changes.

This comment is automatically updated on each push.

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 收到。这是已完成的工作(PR #449 对应 bug #446),我不会重做,现在拉取分支核对代码 + 确认 CI 状态,稍后给出结论。

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 核对完成:修复了 decompress 在源消息已被宿主历史删除时仍"幻影恢复"并不可逆丢弃摘要的 #446(commit 路径把成员关系转移误当内容恢复),fail-closed 前置门设计正确、测试真实可捕获该 bug,无冲突,可以合并。

Verification report (PR #449 → closes #446)

Per the [PR] handling rule I did not redo the work — pulled branch 2026-09-20_decompress-source-availability (single commit 091e342a on master 026dbe73) and independently verified code + CI + tests.

1. CI — all green on 091e342a: pr-validation, test (22), test (24), build, e2e, build-artifact all completed/success.

2. Diff cleanliness: 7 files, +1007/−0, purely additive — lib/compress/decompress-logic.ts (+122), lib/compress/decompress.ts (+37), two new test files (+248/+426), devlog REQ/WORKLOG/DESIGN. No unrelated files, no version bump, no persisted-state-format change. Devlog folder matches branch name; DESIGN.md present (new module boundary — required).

3. Code review (independent trace):

  • Gate placement verified: after the toFile return, before any state mutation (snapshotActiveMessages comes after). Transient fetch failure still throws earlier in prepareDecompressSession, so transient vs missing-source stay structurally distinct — the error text says exactly that.
  • Cross-checked collectRequiredDecompressMessageIds against syncCompressionBlocks reactivation semantics (creation-order processing, user-deactivated stays inactive, each non-user-deactivated block deactivates its currently-active consumed blocks before activating itself). Traced the edge cases: transitive closure via BFS over consumedBlockIds; a consumed tier C that won't be reactivated because a later non-user-deactivated D consumes it is safe either way (D transitively covers C's messages, so they retain non-target active coverage and are excluded); user-deactivated consumed tiers correctly NOT shielded; full mode deep-deactivates the whole closure so no shield applies. Shield can only over-shield (safe-direction abort), never under-require.
  • No partial commit by design; known limitation (reactivated tier's summary-carrier compress message not verified — carriers are hard-protected, never in byMessageId) documented in DESIGN.md and deferred beyond [Bug]: decompress reports restored originals without checking host history availability #446 scope. Reasonable scoping.
  • Zero as any / @ts-ignore in new lib code.

4. Local run (fresh worktree of the PR head):

  • npm run typecheck clean.
  • Full suite: 1314 tests, 1312 pass, 2 fail — exactly the claimed numbers. Both failures reproduced and confirmed as pre-existing sandbox issues, not PR-related: inactive-block-decompress.test.ts hardcodes /tmp/test-inactive-block-decompress.txt (rejected by this env's toFile path validation) and soft-block.test.ts does mkdir /tmp/opencode-dcp-dangerous-* (read-only /tmp mount → EACCES). Neither file is touched by this PR or by recent master commits.

5. Bug-detection proof (§5.7.3) — done myself, not taken on faith: temporarily disabled the gate (if (false && ...)) in the worktree → 5 of 9 E2E tests failed, with the all-missing case producing the issue's exact phantom output: Decompressed block b5. Restored 1 message(s) (~100 tokens). Restored the gate → green. The regression tests genuinely catch the original bug.

6. Test quality (§5.6 spot-check): both test files import from real source (createDecompressTool, decompress-logic functions, lib/state/types); E2E drives the real tool with controllable host history; 62 assertions in the E2E file; as any only in test fixture literals (consistent with existing test conventions).

One note (not blocking): master has moved since the PR base (v1.18.2 release, #437/#441/#443 merged). GitHub reports mergeable: true, mergeable_state: behind — zero file overlap between this PR and those merges, so it merges cleanly as-is. If you want CI green against the current master before clicking merge, a quick rebase + push will refresh it; otherwise nothing blocks the merge.

Merging is your call — per project rules I don't merge PRs: #449

@ranxianglei
ranxianglei merged commit 967eec3 into master Sep 21, 2026
6 checks passed
ranxianglei added a commit that referenced this pull request Sep 23, 2026
Master's new E2E suites (PR #448/#449) build tool factories with a minimal
{reload(){}} prompts mock; since promptPack landed, factories also call
getRuntimePrompts() at creation time. Give both files the same
makeDefaultPromptsMock() helper used by the four pre-existing factory test
files so they run against the shipped default-pack descriptions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: decompress reports restored originals without checking host history availability

1 participant