Skip to content

fix(claude): keep IDE-context envelopes out of first_message previews - #1265

Open
adammw wants to merge 1 commit into
kenn-io:mainfrom
adammw:fix/claude-ide-envelope-preview
Open

fix(claude): keep IDE-context envelopes out of first_message previews#1265
adammw wants to merge 1 commit into
kenn-io:mainfrom
adammw:fix/claude-ide-envelope-preview

Conversation

@adammw

@adammw adammw commented Jul 25, 2026

Copy link
Copy Markdown

Fixes #1238.

Claude IDE-context wrappers prepended to real prompts are now split into hidden
system metadata and a visible user prompt in both parser paths. The revealed
prompt goes through the normal user-text preprocessing, while the synthetic
envelope receives a distinct source identity. This keeps raw IDE markup out of
first_message and the visible transcript without aliasing the prompt's
identity. Data version 86 reparses existing sessions, and the Claude format
provenance records the observed mixed-envelope shape.

Because that reparse can rebuild message rows, SQLite and PostgreSQL pin
restoration now follow only unambiguous prior message identities. UUID-less and
duplicated-UUID rows may use a guarded ordinal, role, and content fallback;
vanished or ambiguous targets are dropped instead of moving or multiplying
pins. Explicit re-uploads preserve edited legacy pins by ordinal only while the
hidden-row layout through that ordinal is unchanged. In-place SQLite updates
fall back to guarded remapping when a pinned identity changes, orphan recovery
maps pins by the verbatim-copied ordinal, and PostgreSQL serializes pin
mutations with transcript replacement while keeping recorded and resolved
anchor ordinals distinct. Changed visible prompt digests also revoke stale
Recall provenance.

The parser intentionally handles only well-formed leading ide_opened_file and
ide_selection wrappers with a non-empty remainder; malformed or mid-message
markup remains untouched. The main review areas are the split logic in
internal/parser/claude.go, conservative pin remapping in
internal/db/messages.go, internal/db/orphaned.go, and
internal/postgres/push.go, and the explicit upload adapter in
internal/server/upload.go.

@roborev-ci

roborev-ci Bot commented Jul 25, 2026

Copy link
Copy Markdown

roborev: Combined Review (26eaed8)

Review identified one medium-severity issue affecting identity and provenance during Claude message splitting.

Medium

  • Duplicate SourceUUID across split messagesinternal/parser/claude.go:1110, internal/parser/claude.go:1136, internal/parser/claude.go:2156, internal/parser/claude.go:2180

    Both split messages receive the same SourceUUID. During the forced v75 reparse, source-anchored pins may move to the hidden envelope or duplicate across both rows, while Recall evidence treats duplicate source UUIDs as ambiguous and revokes provenance.

    Recommended fix: Preserve the original UUID only on the real prompt, and give the synthetic metadata row a distinct deterministic identity or no UUID. Add reparse tests covering pins and Recall evidence.


Reviewers: 2 done | Synthesis: codex, 10s | Total: 4m26s

@roborev-ci

roborev-ci Bot commented Aug 1, 2026

Copy link
Copy Markdown

roborev: Combined Review (15dbd40)

Code review found two medium-severity correctness issues; no security concerns were identified.

Medium

  • internal/db/recall_evidence_window_test.go:247 — The test inserts an envelope before otherwise unchanged messages, but the actual v74→v75 reparse changes the original row from <ide…> prompt to prompt. Because recall reconciliation recomputes the content digest, evidence spanning that row will still lose provenance_ok despite the distinct UUID.

    • Fix: Test the actual combined-to-split transformation and add narrowly scoped digest migration/normalization for the IDE-envelope rewrite, or explicitly treat revocation as expected.
  • internal/db/orphaned.go:578 — Ordinal fallback also applies when a non-empty source UUID is missing or ambiguous in the rebuilt database. Since reparsing can shift or remove rows, this may silently move a pin to an unrelated message at the former ordinal.

    • Fix: Use ordinal fallback only when the source row has an empty UUID; otherwise drop unresolved pins or validate them using stable content identity.

Reviewers: 2 done | Synthesis: codex, 9s | Total: 6m43s

@roborev-ci

roborev-ci Bot commented Aug 1, 2026

Copy link
Copy Markdown

roborev: Combined Review (58dabd8)

Code changes are generally sound, but one medium-severity pin-remapping issue should be fixed.

Medium

  • internal/db/orphaned.go:1220 — The ordinal fallback runs when a nonempty source_uuid resolves to zero or multiple destination rows. If the original source message disappeared and another message now occupies that ordinal, the pin can silently attach to unrelated content.

    Fix: Use ordinal fallback only for legacy NULL/empty UUIDs. For duplicate UUIDs, require the ordinal candidate to have the same UUID; otherwise drop the pin. Add coverage for a missing UUID with a reused ordinal.


Reviewers: 2 done | Synthesis: codex, 9s | Total: 5m34s

@mjacobs
mjacobs force-pushed the fix/claude-ide-envelope-preview branch from 58dabd8 to 5193f4a Compare August 1, 2026 03:32
@roborev-ci

roborev-ci Bot commented Aug 1, 2026

Copy link
Copy Markdown

roborev: Combined Review (5193f4a)

Medium-severity correctness issue found in UUID-based metadata remapping.

Medium

  • internal/db/orphaned.go:1208 — UUID remapping verifies uniqueness only in the destination. If the old session contains duplicate source_uuid values but only one survives re-parsing, a pin on the removed duplicate may be incorrectly transferred to the surviving message instead of being dropped. Require the UUID to be unique in both old_db.messages and main.messages; otherwise use the guarded ordinal fallback.

Reviewers: 2 done | Synthesis: codex, 9s | Total: 3m21s

@roborev-ci

roborev-ci Bot commented Aug 1, 2026

Copy link
Copy Markdown

roborev: Combined Review (dbe4eba)

Code changes are generally sound, but one medium-severity parser preprocessing issue remains.

Medium

  • internal/parser/claude.go:1117, internal/parser/claude.go:2172 — The IDE envelope is split after preprocessClaudeUserText runs, causing the revealed remainder to bypass command and system-reminder preprocessing. For example, <command-name>/clear</command-name> after an IDE envelope may be stored as raw XML and incorrectly become first_message instead of normalizing to /clear and being excluded from previews.

    Fix: Split the IDE envelope before preprocessing, or preprocess the remainder and honor its skip result in both extraction paths.


Reviewers: 2 done | Synthesis: codex, 8s | Total: 5m44s

@roborev-ci

roborev-ci Bot commented Aug 1, 2026

Copy link
Copy Markdown

roborev: Combined Review (26c1417)

Review identified two medium-severity correctness issues in migration reconciliation and pin remapping.

Medium

  • internal/db/recall_evidence_window_test.go:249 — The new Recall test inserts an envelope before an unchanged prompt, but the real migration changes the prompt from <ide_…>…</ide_…> prompt to prompt. Because Recall’s canonical digest includes message content, reconciliation detects a mismatch and revokes provenance for evidence anchored to the affected prompt despite its stable UUID.

    • Fix: Test the actual combined-to-split transformation and add migration-aware digest reconciliation for removal of a recognized leading IDE envelope.
  • internal/db/orphaned.go:1235 — The duplicate-UUID ordinal fallback can misattach a pin after ordinal shifts. If messages A and B share a UUID, A at ordinal 1 is pinned, and reparsing removes A so B shifts from ordinal 2 to 1, the guard incorrectly accepts B because its UUID and new ordinal match.

    • Fix: For duplicate UUIDs, require stronger identity—such as matching content and role with unchanged multiplicity—before using the ordinal fallback; otherwise drop the pin.

Reviewers: 2 done | Synthesis: codex, 12s | Total: 5m4s

@mjacobs
mjacobs force-pushed the fix/claude-ide-envelope-preview branch from 26c1417 to af011ba Compare August 1, 2026 17:40
@roborev-ci

roborev-ci Bot commented Aug 1, 2026

Copy link
Copy Markdown

roborev: Combined Review (af011ba)

Code is generally sound, but one medium-severity pin-mapping issue should be fixed before merging.

Medium

  • internal/db/orphaned.go:1238 — UUID-less pins still fall back unconditionally by ordinal. After an IDE-envelope split, the old combined prompt’s ordinal belongs to the hidden envelope, so the pin can silently attach to metadata instead of the prompt.
    • Fix: Require matching role/content identity for UUID-less ordinal fallback, explicitly map the split shape to the following prompt, or drop the pin when identity cannot be established.

Reviewers: 2 done | Synthesis: codex, 8s | Total: 4m23s

@roborev-ci

roborev-ci Bot commented Aug 1, 2026

Copy link
Copy Markdown

roborev: Combined Review (ef49dd6)

Pin restoration is mostly hardened, but one medium-severity duplication bug remains for orphaned sessions.

Medium

  • internal/db/orphaned.go:1194copyPinnedMessagesForIDs copies pins before hardened restoration and joins on source_uuid without requiring uniqueness. A pinned message with a duplicated UUID can therefore attach its pin to every matching orphaned message, and the later INSERT OR IGNORE does not remove the incorrect pins. Data-version 80’s full resync may duplicate pins in orphaned sessions.
    • Suggested fix: Apply the same uniqueness and guarded fallback rules in copyPinnedMessagesForIDs, or copy orphan pins only by unchanged ordinals. Add a regression test covering an orphaned session with duplicate UUIDs.

Reviewers: 2 done | Synthesis: codex, 10s | Total: 3m42s

@roborev-ci

roborev-ci Bot commented Aug 3, 2026

Copy link
Copy Markdown

roborev: Combined Review (645d8c5)

The changes need revision due to two medium-severity pin-restoration issues.

Medium

  • internal/db/messages.go:1173 — Identity-aware pin restoration is bypassed by the in-place diff path. Role/content changes are allowed whenever source_uuid remains equal, including empty or duplicate UUIDs, so an unrelated message can inherit a pin. Force full replacement when changed rows have empty or non-unique UUIDs, or apply ordinal/role/content identity checks during in-place updates. Add coverage for one changed pinned row among several unchanged rows.

  • internal/db/messages.go:1668 — A legacy UUID-less pin is lost when an otherwise identical replacement gains a source_uuid. Ordinal, role, and content already establish fallback identity, and the metadata-copy path permits UUID enrichment. Remove the requirement that the replacement UUID remain empty and test pin preservation when a UUID-less message gains a UUID.


Reviewers: 2 done | Synthesis: codex, 11s | Total: 9m17s

@mjacobs mjacobs closed this Aug 3, 2026
@mjacobs mjacobs reopened this Aug 3, 2026
@roborev-ci

roborev-ci Bot commented Aug 3, 2026

Copy link
Copy Markdown

roborev: Combined Review (f587872)

Medium-severity issue found in PostgreSQL pin reconciliation; no Critical or High findings.

Medium

  • PostgreSQL pin remapping can attach pins to unrelated messages
    Location: internal/db/messages.go:1605, internal/postgres/push.go:2908
    SQLite now requires an unambiguous UUID/role/content identity, but PostgreSQL reconciliation still selects any matching UUID—preferring the old ordinal—and unconditionally enriches UUID-less pins from that ordinal. After a reparse, duplicate UUIDs or an IDE-envelope split can therefore misassociate a pin.
    Fix: Snapshot each pin’s prior message identity before deleting PostgreSQL messages, then apply the same uniqueness checks and guarded ordinal fallback used by SQLite. Add PostgreSQL coverage matching the new SQLite ambiguity tests.

Reviewers: 2 done | Synthesis: codex, 9s | Total: 6m59s

@roborev-ci

roborev-ci Bot commented Aug 3, 2026

Copy link
Copy Markdown

roborev: Combined Review (47b7bb0)

No Medium, High, or Critical findings; the reviewed changes are clean at the requested severity threshold.


Reviewers: 2 done | Synthesis: codex, 6s | Total: 8m20s

@roborev-ci

roborev-ci Bot commented Aug 3, 2026

Copy link
Copy Markdown

roborev: Combined Review (9ed4a06)

Changes improve context separation and pin reconciliation, but introduce a High-severity CockroachDB compatibility regression.

High

  • internal/postgres/curation.go:18 — Message push and pin mutations now use pg_advisory_xact_lock(hashtextextended(...)), which CockroachDB does not support. Changed-session pushes and pin/unpin operations will fail on the documented CockroachDB backend. Use a compatible serialization mechanism such as a per-session row lock with SELECT ... FOR UPDATE, or detect CockroachDB and apply a supported fallback.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 7m36s

@roborev-ci

roborev-ci Bot commented Aug 3, 2026

Copy link
Copy Markdown

roborev: Combined Review (4ac1f70)

Code quality is generally solid, but upload conversion mishandles compact-summary boundaries.

Medium

  • internal/server/upload.go:217 — Upload conversion preserves IsSystem but drops IsCompactBoundary. Uploaded compact summaries are stored as ordinary system messages and hidden from both transcripts and focused exports rather than rendered as compact-boundary dividers. Copy m.IsCompactBoundary into db.Message and add a regression test covering compact boundaries during upload conversion.

Reviewers: 2 done | Synthesis: codex, 9s | Total: 8m40s

@roborev-ci

roborev-ci Bot commented Aug 3, 2026

Copy link
Copy Markdown

roborev: Combined Review (5a0e8c3)

The PR needs changes: two medium-severity correctness issues remain in Claude branch scoring and upload mapping.

Medium

  • internal/parser/claude.go:2077 — DAG branch scoring examines unsplit IDE-envelope text instead of applying the same splitting and remainder preprocessing used during message extraction. If preprocessing later discards the remainder, such as for an empty command envelope, extraction yields only hidden system metadata while isCountedClaudeUserTurn counts it as a real turn. Near forkThreshold, this can select the wrong fork strategy and produce incorrect session topology.

    • Fix: Apply identical splitting and remainder preprocessing during branch scoring, and add a fork test covering a discarded remainder.
  • internal/server/upload.go:208 — The upload mapper omits ParsedMessage.IsSidechain. Uploaded split-envelope messages marked as sidechain are stored as main-chain messages, altering content-unit boundaries and search behavior compared with locally synced transcripts.

    • Fix: Map IsSidechain: m.IsSidechain and cover it in the message-identity upload test.

Reviewers: 2 done | Synthesis: codex, 12s | Total: 14m20s

@roborev-ci

roborev-ci Bot commented Aug 3, 2026

Copy link
Copy Markdown

roborev: Combined Review (3a98e81)

No issues found.


Reviewers: 2 done | Synthesis: codex | Total: 7m48s

@mariusvniekerk

Copy link
Copy Markdown
Collaborator

The pin reattachment parts look a bit overwrought here?

@mjacobs

mjacobs commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Agreed... this has grown beyond the original bug. I’m going to keep the prompt as one message and remove the leading IDE wrapper in place. That avoids shifting message ordinals, so the pin reattachment changes can come out of this PR.

@wesm

wesm commented Aug 12, 2026

Copy link
Copy Markdown
Member

looking

Squash of follow-up work on this branch:

- fix(claude): give split IDE envelopes a distinct source identity
- fix(db): drop orphaned pins instead of ordinal-matching unrelated rows
- fix(db): require pin uuid uniqueness in the old DB as well
- fix(claude): preprocess the prompt revealed by an IDE-envelope split
- fix(db): harden duplicate UUID pin fallback
- test(recall): model IDE-envelope digest revocation
- fix(db): require role/content identity for uuid-less pin fallback
- fix(db): avoid duplicate orphaned-session pins
- fix(db): guard pins during in-place reparses
- fix(db): fall back when pin UUID becomes ambiguous
- fix(db): guard pins across diff updates
- fix(postgres): restore pins by prior message identity
- fix(postgres): preserve pins created during sync
- fix(postgres): serialize pin mutations with sync
- fix(postgres): follow shifted pin UUID anchors
- fix(db): avoid pinning hidden upload metadata
- fix(db): guard upload pins from hidden row shifts
- fix(postgres): snapshot resolved pin ordinals
- fix(postgres): separate pin anchor ordinals
- fix(db): require matching pin source UUIDs
- fix(postgres): use portable pin session locks
- fix(server): preserve uploaded message provenance
- fix(server): preserve uploaded compact boundaries
- fix(claude): align fork scoring with extraction
- fix(server): preserve uploaded sidechain state

Co-authored-by: Matthew Jacobs <mjacobs@apache.org>
@wesm
wesm force-pushed the fix/claude-ide-envelope-preview branch from 3a98e81 to 202295a Compare August 13, 2026 01:33
@roborev-ci

roborev-ci Bot commented Aug 13, 2026

Copy link
Copy Markdown

roborev: Combined Review (202295a)

Code changes are generally sound, but one medium-severity incremental parsing issue remains.

Medium

  • internal/parser/claude.go:1191 — When preprocessing discards the prompt remainder, the synthetic envelope becomes the sole stored message but retains the derived uuid:ide-context. This becomes last_entry_uuid, while the next real entry references the original UUID, falsely triggering incremental DAG fork detection and repeated expensive full reparses.

    Suggested fix: Assign the original entry UUID to the sole envelope message when no visible remainder survives, or track the raw entry UUID independently. Apply this to both extraction paths and add coverage for an appended UUID chain following a discarded remainder.


Reviewers: 2 done | Synthesis: codex, 11s | Total: 11m5s

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Strip out / parse <ide_opened_file>/<ide_selection> in Claude Code via VSCode extension

4 participants