Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 76 additions & 0 deletions devlog/2026-09-20_decompress-source-availability/DESIGN.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
# DESIGN: source-availability gate for decompress

## Layer analysis (triage)

- **Symptom**: `Restored 1 message(s)` with empty preview; stats decremented for content that never returns to context.
- **Root cause**: the decompression commit path treats _membership transition_ (`activeBlockIds` → empty) as proof of _content restoration_. It never intersects the restored set with the IDs actually present in fetched host history. `computeRestoredMessages` does not even receive rawMessages.
- The issue's suggested direction (verify before commit, explicit missing reporting, fail closed) hits the root cause. Adopted.

## Key architectural facts that shape the design

1. A block's summary lives in its compress tool-call message in host history (`summary` param); originals are hidden from the window by `prune.filterCompressedRanges` while `entry.activeBlockIds.length > 0`.
2. Decompression = deactivate target blocks + `syncCompressionBlocks`. One-tier: consumed (non-user-deactivated) blocks are reactivated by sync, so their raw messages stay hidden and only the previous tier's compress-call messages become visible. Full: consumed blocks get `deactivatedByUserDeep` and stay dead, so ALL covered raw messages must return.
3. User-deactivated blocks are terminal (sync never reactivates them) — a blind deactivation with missing originals is irreversible.
4. `byMessageId` membership is bidirectional with `block.effectiveMessageIds` (built in `compress/state.ts`), and prune filters exactly on `entry.activeBlockIds`. So "which messages become visible" is a pure function of membership + sync reactivation semantics.

## Design

**Fail-closed pre-commit gate**, inserted between target resolution and the deactivation loop in `createDecompressTool.execute` (after the toFile branch, which makes no state changes).

New pure functions in `lib/compress/decompress-logic.ts`:

```ts
export interface DecompressAvailabilityResult {
requiredMessageIds: string[] // IDs whose visibility this decompression would restore
availableMessageIds: string[] // subset present in fetched history
missingMessageIds: string[] // subset absent — non-empty ⇒ abort
}

export function collectRequiredDecompressMessageIds(
messagesState: PruneMessagesState,
targets: CompressionTarget[],
options: { full?: boolean },
): string[]

export function checkDecompressSourceAvailability(
messagesState: PruneMessagesState,
targets: CompressionTarget[],
options: { full?: boolean },
availableMessageIds: Set<string>,
): DecompressAvailabilityResult
```

### Required set R (closed form, no state mutation)

For each `byMessageId` entry: include iff

1. `entry.activeBlockIds.length > 0`, AND
2. every ID in `entry.activeBlockIds` belongs to the target set T (exclusively covered by what we are about to kill — otherwise another live block still hides it, and its summary remains), AND
3. one-tier only: the ID is NOT in the union of `effectiveMessageIds` of the transitive consumed closure of T whose members would be reactivated by sync (not user-deactivated / not deep-deactivated). Those messages stay hidden under reactivated lower tiers, so they are not required sources.

Full mode skips condition 3 (everything under T must be present).

Why closed form instead of mutate→diff→rollback: zero crash window, no rollback code path, no partial-mutation risk. Cost: it mirrors sync's reactivation rule (non-user-deactivated consumed blocks revive). Soundness argument: over-inclusion in the shield set is safe (a shielded message either stays hidden under a revived block or under another live consumer — in both cases it is still covered, so excluding it from R can never let a false "restored" claim through). Over-requirement is possible only if a target-exclusive entry is also listed in a consumed block's `effectiveMessageIds` without that block reviving — which cannot happen: any consumed block of a target either revives (one-tier, non-user-flagged) or was already dead via user flags (then its messages were already exposed, i.e. their entries do not list the target exclusively… conservatively excluded anyway).

Note on empty `byMessageId`: when membership is unverified/empty, R = ∅ and the gate passes — consistent with existing behavior, because prune then filters nothing and `computeRestoredMessages` also reports nothing. The tool's restoration claim is exactly the membership-transition claim; verifying precisely those IDs against history closes the gap completely.

### Gate behavior

- `missingMessageIds.length > 0` → warn log + return an explicit error naming the target blocks, counts, and missing refs (via `state.messageIds.byRawId`, falling back to raw IDs). **No state mutation, no stat change.** Block stays compressed; summary remains available. Message also notes this is distinct from a transient fetch failure (that would have thrown in `prepareDecompressSession` before any mutation).
- Empty → proceed exactly as today. Because every ID in R is verified present, `computeRestoredMessages`' count/tokens are honest recovered-content numbers (acceptance #4 falls out for free).

### What we deliberately do NOT do

- No partial-commit mode (acceptance #3 default): restoring some originals while losing summary coverage semantics mid-block is worse than refusing; escape hatches (force flag / policy key) are future work, not requested here.
- No retry logic: fetch failure already surfaces as an exception before mutation; the two failure classes are structurally distinguished (acceptance #6 satisfied by construction + documented in the error text).
- toFile path untouched (#445 owns those field bugs).

## Known limitation (from dual-agent code review)

In one-tier nested decompress, what becomes visible after commit also includes the **reactivated consumed block's compress tool-call message** (its summary carrier), whose visibility is keyed on block liveness in `hideConsumedCompressCalls` — not on `byMessageId` membership. Compress calls are hard-protected, so carriers never have `byMessageId` entries and cannot enter R. If host history lost an intermediate tier's carrier while the topmost block is still resolvable, the gate passes, the commit proceeds, and the topmost summary is terminally discarded while nothing usable returns to context. Rare trigger (multi-tier nesting + selective deletion of exactly the intermediate carrier); pre-fix behavior was strictly worse (phantom claim + stats corruption on top). Fixing it properly means extending R with reactivating closure members' `compressMessageId`s — deliberately deferred: it changes R semantics beyond #446's stated scope (indexed originals) and needs its own review. Recorded here so the follow-up is findable; per project rules no separate issue is filed from this working session.

## Testing strategy

- Unit (extend `tests/decompress-logic.test.ts`): R computation for single-target, multi-target, one-tier nested (raws shielded, prev-tier compress call required), full nested (all required), unrelated overlapping active block (not required), empty membership (∅).
- Integration (new `tests/decompress-source-availability.test.ts`, reusing the `inactive-block-decompress.test.ts` harness with a controllable client history): all-missing aborts with exact message + zero state change; partial aborts listing only missing IDs; complete succeeds with correct restored count/tokens; nested one-tier succeeds with raws missing but prev-tier call present; nested full aborts; preview non-empty on success.
- Bug-detection proof: the integration all-missing test must fail against pre-fix code (it asserts the abort + no deactivation; pre-fix returns "Restored 1 message(s)" and deactivates). Verify by temporarily reverting the gate insertion.
48 changes: 48 additions & 0 deletions devlog/2026-09-20_decompress-source-availability/REQ.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# REQ: decompress must verify source-message availability before committing

- **Issue**: https://github.com/ranxianglei/opencode-acp/issues/446
- **Branch**: `2026-09-20_decompress-source-availability`
- **Date**: 2026-09-20
- **Type**: Bug fix

## Problem

When an indexed original message is absent from the history returned by the host
(native compaction, external deletion, V2 checkpoint divergence), block decompression
still reports `Restored N message(s)`, deactivates the summary block, and decrements
`totalPruneTokens` — restoring visibility metadata without verifying that any content
actually returns to context.

Verified trace on master (`f6c70a7e` era, v1.18.x):

1. `lib/compress/decompress.ts:340` — `snapshotActiveMessages` snapshots indexed IDs, not fetched originals.
2. `lib/compress/decompress.ts:343-347` — deactivates targets + `syncCompressionBlocks`.
3. `lib/messages/sync.ts:136-146` — membership update never intersects with available history IDs.
4. `lib/compress/decompress-logic.ts:162-177` — `computeRestoredMessages` counts membership transitions only; does not accept rawMessages.
5. `lib/compress/decompress.ts:358-361,382-384` — stats adjusted + success message emitted.
6. Preview (`decompress-logic.ts:191-231`) reads real history; empty when originals missing; `decompress.ts:395-399` silently omits it. No warning.

Repro fixture (issue #446): active b5 covers `msg-gone`;
`byMessageId["msg-gone"] = { tokenCount: 100, allBlockIds: [5], activeBlockIds: [5] }`;
history contains a different message. Observed pre-fix:
`restoredMessageCount: 1, restoredTokens: 100, actualOriginalMessagesFound: 0, restoredContentPreview: "", blockDeactivated: true`.

## Why it matters

- **False success**: model believes "restored" content is visible → hallucination risk about details that are not in context.
- **Stats corruption**: `totalPruneTokens` decremented by phantom tokens → `/acp context` lies.
- **Irreversible coverage loss**: user-deactivated blocks are terminal (sync keeps them dead); if the summary carrier (compress tool call) was also removed from history, the content is unrecoverable after a blind deactivation.

## Acceptance criteria (from issue)

1. Verify required source messages are present before committing a decompression that claims to restore originals, respecting one-tier vs full semantics.
2. Return explicit complete/partial/missing-source results with missing IDs/counts; do not label metadata-only restoration as recovered text.
3. Preserve usable summary coverage on unavailable originals (fail closed), unless an explicit policy deliberately permits partial restoration.
4. Count restored originals / adjust statistics based on verified recovered content, not only membership transitions.
5. Integration regressions: all originals missing, partial availability, complete availability, nested one-tier/full cases.
6. Distinguish transient host-fetch failures (error before any mutation) from missing/deleted originals (gate aborts, no state change).

## Scope

- In scope: normal non-toFile block/range decompression path (`createDecompressTool.execute`).
- Out of scope: toFile export field bugs (#445), V2 host-history projection (#425), retry logic for fetch failures (fetch failure already errors before any mutation — documented, no new mechanism).
50 changes: 50 additions & 0 deletions devlog/2026-09-20_decompress-source-availability/WORKLOG.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
# WORKLOG — decompress-source-availability (#446)

Branch: `2026-09-20_decompress-source-availability` · Base: master `026dbe73` · Date: 2026-09-20

## Summary

Fix for #446: block decompression reported `Restored N message(s)` and terminally
deactivated the summary block even when the indexed original messages were absent
from the host history. Root cause: the commit path treated a membership transition
(`activeBlockIds` going empty) as proof that content was restored, without ever
intersecting the required source IDs with the fetched history.

## Changes

| File | Change |
|------|--------|
| `lib/compress/decompress-logic.ts` | New `DecompressAvailabilityResult`, `collectRequiredDecompressMessageIds()`, `checkDecompressSourceAvailability()` |
| `lib/compress/decompress.ts` | Pre-commit availability gate in block/range decompress path (after toFile branch, before any state mutation); explicit missing-source error; warn log with refs |
| `tests/decompress-logic.test.ts` | 13 unit tests for the two new functions (tier semantics, closure, multi-target, empty membership) |
| `tests/decompress-source-availability.test.ts` | 9 E2E regressions through `createDecompressTool` with controllable host history: all-missing / ref-display / retry-after-abort / partial / complete / nested one-tier / nested full abort / nested full success / transient-fetch-failure rejection |
| `devlog/2026-09-20_decompress-source-availability/` | REQ.md + DESIGN.md + this file |

## Verification log

- [x] `npm run typecheck` — clean
- [x] `npm run test` — 1312/1314 pass (final state incl. post-review additions; one intermediate run showed a transient soft-block crash-count flake of `fail 3`, re-run stable at 2). The 2 failures are pre-existing environment issues, reproduced identically on clean master (verified by stashing this change):
- `tests/soft-block.test.ts` — crashes at import: `EACCES mkdir '/tmp/opencode-dcp-dangerous-*'` (read-only `/tmp` in sandbox)
- `E2E: toFile on inactive block writes block summary` — tool path validation rejects `/tmp` when `$TMPDIR` points elsewhere
- [x] `npm run build` — success (dist/index.js 488.57 KB)
- [x] Bug-detection proof (§5.7.3): gate temporarily removed → all-missing E2E test fails with `Decompressed block b5. Restored 1 message(s) (~100 tokens)` (the exact phantom-restoration behavior from the issue) → gate restored → 7/7 pass
- [x] Dual-agent review (§5.3 code + §5.6 tests) — two independent agents, both verdicts **approve-with-nits** (details below)

## Dual-agent review findings & disposition

| # | Finding | Severity | Disposition |
|---|---------|----------|-------------|
| C1 | One-tier nested commit does not verify the reactivated tier's summary-carrier compress message (carriers are hard-protected, never in `byMessageId`, so cannot enter R); if host history lost exactly the intermediate carrier, the topmost summary is terminally discarded with nothing usable restored | minor | **Documented as known limitation in DESIGN.md**; extending R with `compressMessageId`s changes R semantics beyond #446's stated scope and needs its own review |
| C2 | toFile path bypasses the gate by design but its success message under-counts silently when some indexed sources are absent | minor | Deferred — #445 owns the toFile field bugs; noted for that work |
| C3 | Untested branches: `deactivatedByUserDeep` exclusion from shielding; stale closure id; range-mode multi-target E2E | nit | deep-flag: covered indirectly by stricter `assert.equal(..., undefined)` on the abort path; stale-id + range-mode: unit-level coverage exists (closure tolerance, multi-target union), range E2E skipped as mode-agnostic (gate runs identically after target resolution) |
| C4 | BFS uses `queue.shift()` — O(k²) worst case | nit | No action: k = blocks under one target (small), runs once per tool call, never per LLM turn |
| T1 | No E2E pin of transient host-fetch failure (acceptance #6) | minor | **Fixed**: new test with rejecting client asserts `assert.rejects` out of the tool call + zero mutation |
| T2 | Ref-resolution display branch (`byRawId ?? raw`) untested — all fixtures had empty maps | minor | **Fixed**: new test seeds `byRawId`/`byRef` mapping, asserts `Missing: m00052.` (mappings verified to survive `assignMessageRefs`, lib/message-ids.ts:174-180) |
| T3 | Nested success tests didn't assert stats (acceptance #4) | nit | **Fixed**: one-tier success asserts `totalPruneTokens === 970` (3×10 restored), full success asserts `=== 950` (5×10 restored) |
| T4 | Weak deep-flag assertion (`!t1.deactivatedByUserDeep` passes for undefined and false) | nit | **Fixed**: `assert.equal(t1.deactivatedByUserDeep, undefined)` |
| T5 | `(+N more)` truncation untested (>10 missing IDs) | nit | Skipped: low value, string-format detail |

## Notes

- `npm run format` (Prettier) reformats ~350 unrelated repo files in this checkout (formatting drift vs. the committed tree). All unrelated rewrites were reverted; only the 4 PR-relevant source/test files carry changes. CI does not gate on `format:check`, so the drift is out of scope here.
- No version bump (release-branch-only rule). No state-format changes; purely additive API surface in `decompress-logic.ts`.
Loading
Loading