Repository navigation
fix(import): scope dedup by source kind, error on missing codex session - #28
Merged
Merged
Conversation
Two refinements to the import adapters surfaced by an adversarial review of the codex adapter (#27): - Scope re-import dedup to the session source kind. loadExistingByExternalId now indexes only sessions whose source.kind matches the adapter being imported, so a Codex import can no longer dedup against (or, under --force, delete) a Claude-derived session that happens to share an id string. The id namespaces stay separate as more adapters are added. - `basou import codex --session <id>` now errors when no rollout in the project matches the id, instead of reporting a silent success. This mirrors the Claude path, which fails on a missing --session transcript. Adds tests for both: cross-kind isolation under --force, and the missing --session error.
6 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Two refinements to the import adapters, surfaced by an adversarial review of the codex adapter (#27). Both are pre-release hardening of the just-landed
basou import codex; neither changes the import data format.1. Dedup scoped by source kind
loadExistingByExternalIdpreviously indexed every existing session'ssource.external_idregardless ofsource.kind. With two adapters now sharing the re-import dedup, a Codex rollout whose id collided with a Claude transcript id could be skipped as "already imported" and, under--force, the unrelated Claude session would be deleted.The dedup map is now scoped to the
source.kindbeing imported (passed into the sharedimportDerivedSessions). Each adapter's id namespace stays separate, which matters more as further adapters are added. The collision is astronomically unlikely in practice (Claude UUIDv4 vs Codex UUIDv7), but the fix makes the isolation correct by construction rather than probabilistic.2.
import codex --session <id>errors on no matchThe Codex
--sessionpath filtered the discovered rollouts and, on no match, reported a silent success ("No transcripts found to import"). The Claude path errors when a named--sessiontranscript is absent.discoverCodexRolloutsnow throwsCodex rollout not found for session id in projectwhen--sessionmatches nothing, so a mistyped id fails loudly and consistently.Tests
--force: a Claude session sharing an external id survives a Codex import; the rollout still imports as a new session. (Fails before the fix.)--sessionwith a non-existent id rejects with the new error.Verification
pnpm -r build && pnpm typecheck && pnpm test && pnpm lintall green. Tests: cli 388 (+2); core unchanged. Lint clean (only the 4 pre-existing unrelated warnings).Not included (reviewed, intentionally deferred)
The same review raised two further items, left out by decision: event-level
cwdinevents.jsonlis not sanitized, butevents.jsonlis gitignored (confirmed viagit ls-files) and this matches the existing claude path, so it is not a tracked-artifact leak; and yielded/interactiveexec_commandexit codes delivered via laterwrite_stdinrecords are recorded asnull, which respects the agreed MVP scope (exec_command only).🤖 Generated with Claude Code