Skip to content

fix: prevent conversation loss when switching Claude accounts#1549

Open
mvanhorn wants to merge 1 commit into
asheshgoplani:mainfrom
mvanhorn:fix/1533-account-switch-conversation-loss
Open

fix: prevent conversation loss when switching Claude accounts#1549
mvanhorn wants to merge 1 commit into
asheshgoplani:mainfrom
mvanhorn:fix/1533-account-switch-conversation-loss

Conversation

@mvanhorn

@mvanhorn mvanhorn commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Summary

Switching a Claude session between two named account slots (session set <id> account <slot> / session switch-account <id> <slot>) could rename the live conversation <id>.jsonl to <id>.jsonl.bak-<epoch> while writing nothing into the target account. The next start then ran claude --resume with no file, so the conversation was unrecoverable except for the orphaned .bak-<epoch>, which was never restored.

Why this matters

This is silent data loss on a supported workflow: a user who switches accounts on an active session loses their whole conversation history, and the set account path even prints account set, but conversation not migrated: ... no such file or directory after the live file has already been renamed. The only surviving copy is a .bak-<epoch> the tool never reads back, so the loss looks permanent from the user's side. The trigger is common in practice because two account config_dirs frequently resolve to the same real directory through a symlink or a shared mount.

Root cause

MigrateConversationFrom guarded against a same-directory migration with filepath.Clean(src) == filepath.Clean(dst), but ExpandPath does not resolve symlinks. When two account config_dirs resolve (via a symlink or cross-mount) to the same real directory, the guard was bypassed: dstFile named the same real file as the live source, the "backup existing destination" step renamed the only live file to a .bak-<epoch>, and the follow-up copy then failed opening the now-gone source, leaving only the orphan.

Changes

  • Compare source and target after filepath.EvalSymlinks (falling back to filepath.Clean when a path does not yet exist) so a same-real-dir switch is a true no-op that never touches the live file.
  • Make the backup/copy sequence rollback-safe: if the copy fails after a destination was backed up, restore the backup so a failed migration always leaves the original live .jsonl intact.
  • Add RestoreOrphanedConversationBackup, which restores <id>.jsonl from the newest orphaned <id>.jsonl.bak-<epoch> when the live file is missing, and wire it into the Start-path prelude and the known-ID restart resume path so a conversation orphaned by an earlier switch is healed on the next start or restart.

Testing

go test ./internal/session/... for the migration and restore suites:

  • same-real-dir switch via a symlinked config dir is a no-op with the live file untouched and no .bak- created
  • distinct-dir happy path preserved
  • backup restored and no orphan left when a copy fails
  • orphan auto-restore recreates <id>.jsonl from the newest .bak-<epoch>, prefers the newest by mtime, and is a no-op when a live file is already present

Fixes #1533

Summary by CodeRabbit

  • Bug Fixes
    • Improved session resume behavior by restoring missing conversation backups when possible.
    • Made conversation migration safer by preserving existing data if a copy fails.
    • Treated symlinked project directories as the same location to avoid unnecessary migration.
  • New Features
    • Added automatic recovery for orphaned conversation backups.
  • Tests
    • Expanded coverage for backup restoration, symlink handling, and failure recovery cases.

Switching a Claude session between two named account slots renamed the
live conversation <id>.jsonl to <id>.jsonl.bak-<epoch> and wrote nothing
into the target account, so the next start ran claude --resume with no
file and the conversation was unrecoverable.

Root cause: MigrateConversationFrom guarded against a same-directory
migration with filepath.Clean(src)==filepath.Clean(dst), but ExpandPath
does not resolve symlinks. When two account config_dirs resolve (via a
symlink or cross-mount) to the same real directory, the guard was
bypassed, dstFile named the same real file as the live source, the
'backup existing destination' step renamed the only live file to a
.bak-<epoch>, and the follow-up copy failed opening the now-gone source.

Fixes:
- Compare src/dst after filepath.EvalSymlinks so a same-real-dir switch
  is a true no-op that never touches the live file.
- Make the backup/copy sequence rollback-safe: if the copy fails after a
  destination was backed up, restore the backup so a failed migration
  always leaves the original live .jsonl intact.
- Add RestoreOrphanedConversationBackup, which restores <id>.jsonl from
  the newest orphaned <id>.jsonl.bak-<epoch> when the live file is
  missing, and wire it into the Start-path prelude and the known-ID
  restart resume path so an already-orphaned conversation is auto-healed
  on next start/restart.

Fixes asheshgoplani#1533
@mvanhorn mvanhorn requested a review from asheshgoplani as a code owner July 1, 2026 08:09
@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds orphaned Claude conversation backup restoration: a new RestoreOrphanedConversationBackup function and newestConversationBackup helper in migrate.go, hardens MigrateConversationFrom's overwrite/restore-on-failure path and real-path equivalence check, and wires restoration into Claude session start/restart resume flows in instance.go, with accompanying tests.

Changes

Orphaned Conversation Backup Restore

Layer / File(s) Summary
Migration equivalence and backup/restore hardening
internal/session/migrate.go
MigrateConversationFrom now treats source/destination as equivalent via resolved real paths, and restores the destination backup if copyFileVerified fails during overwrite.
RestoreOrphanedConversationBackup and newest-backup selection
internal/session/migrate.go
New exported RestoreOrphanedConversationBackup restores missing live .jsonl files from the newest .jsonl.bak-* orphan via newestConversationBackup, resolving effective working directory and project name.
Wiring restore into Start/Restart resume flows
internal/session/instance.go
ensureClaudeSessionIDFromDisk and Restart() invoke RestoreOrphanedConversationBackup for Claude tools before resuming, logging orphan_bak_restore/orphan_bak_restore_restart outcomes.
Migration and restore test coverage
internal/session/migrate_test.go
New helpers and tests cover symlinked-directory no-op migration, destination restore on copy failure, and RestoreOrphanedConversationBackup scenarios (basic restore, multi-repo path resolution, newest-by-mtime selection, no-op when live present).

Estimated code review effort: 4 (Complex) | ~55 minutes

Sequence Diagram(s)

sequenceDiagram
    participant Session as Session Instance
    participant Resume as ensureClaudeSessionIDFromDisk/Restart
    participant Migrate as migrate.go
    participant FS as Filesystem

    Session->>Resume: start/restart with ClaudeSessionID
    Resume->>Resume: adoptExplicitClaudeSessionID / detect existing tmux
    alt Tool is Claude
        Resume->>Migrate: RestoreOrphanedConversationBackup(inst, configDir)
        Migrate->>FS: check live <id>.jsonl exists
        alt live file missing
            Migrate->>FS: newestConversationBackup(<id>.jsonl.bak-*)
            Migrate->>FS: rename or copyFileVerified backup to live path
            FS-->>Migrate: restored path
            Migrate-->>Resume: restored path, nil
            Resume->>Resume: log resume reason=orphan_bak_restore(_restart)
        else live file present
            Migrate-->>Resume: "", nil (no-op)
        end
    end
    Resume->>Session: buildClaudeResumeCommand / prepareCommand
Loading

Possibly related PRs

  • asheshgoplani/agent-deck#1148: Both PRs modify explicit --session-id handling in internal/session/instance.go, composing at the same resume code paths.
  • asheshgoplani/agent-deck#1377: Main PR builds directly on this PR's MigrateConversationFrom in internal/session/migrate.go, extending its backup/restore behavior.
  • asheshgoplani/agent-deck#1473: Both PRs modify Claude Start/Restart session-id handling in internal/session/instance.go around the resume flow.

Suggested reviewers: asheshgoplani

🚥 Pre-merge checks | ✅ 6 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Test_coverage_per_surface ⚠️ Warning Only internal helper tests were added; the CLI entrypoints and Start/Restart restore hooks aren’t covered by surface-level regressions. Add command-level tests for session set account and session switch-account, plus Start/Restart regressions proving orphaned backup restoration on resume.
✅ Passed checks (6 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title uses Conventional Commits format and accurately describes the account-switch conversation-loss fix.
Linked Issues check ✅ Passed The changes address the lossless migration, rollback, symlink, and orphan-backup restore requirements from issue #1533.
Out of Scope Changes check ✅ Passed The added code and tests stay focused on migration safety and backup restoration, with no clear unrelated changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Remote_parity ✅ Passed No TUI files were changed; the PR only updates internal/session migration/resume logic, so RemoteSession parity is not applicable.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

if bak == "" {
return "", nil
}
if err := os.Rename(bak, live); err == nil {
if bak == "" {
return "", nil
}
if err := os.Rename(bak, live); err == nil {
var newest string
var newestMod time.Time
for _, file := range files {
info, err := os.Stat(file)

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/session/instance.go`:
- Around line 2959-2963: Keep the session log message constant in the restore
path and avoid concatenating i.ClaudeSessionID into the text, since it is
already emitted via the structured claude_session_id field. Update the
sessionLog.Info call in the orphan backup restore flow to use a fixed message
and rely on the existing slog fields for instance_id, claude_session_id, path,
and reason. Apply the same cleanup to the other referenced restore log site as
well so both calls follow the same structured-logging pattern.
- Line 2958: RestoreOrphanedConversationBackup failures are being ignored before
resume, which can lead to an empty-history claude --resume with no visibility.
Update the restore-and-resume path in the instance session flow around
RestoreOrphanedConversationBackup to log any returned error when the live
conversation JSONL is missing, and in Restart() consider stopping the
respawn/resume path early if restore fails so we don’t proceed blindly. Use the
existing resume logic and backup restore helpers to surface the failure clearly.
- Around line 2957-2958: The orphaned conversation backup recovery is hard-gated
to only the exact “claude” tool, so Claude-compatible wrappers can skip
.jsonl.bak-* restoration and resume empty. Update the resume/recovery checks
around RestoreOrphanedConversationBackup and the related Claude session restore
path to use IsClaudeCompatible(i.Tool) (or otherwise allow compatible tools)
while preserving the existing ClaudeSessionID and backup-dir behavior. Make sure
the logic in instance.go consistently treats compatible tools the same as claude
for backup recovery.

In `@internal/session/migrate.go`:
- Around line 93-100: The failed copy path in migrate.go leaves a partial
destination file behind, especially in the copyFileVerified and restore-fallback
flow. Update the error handling in the migration routine that uses
copyFileVerified, os.Remove, and os.Rename so any partially written dstFile/live
file is deleted on all failure paths, including when no backup exists and when
restoring bak fails. Use the migration helper(s) around copyFileVerified and the
backup restore logic to ensure resume/restore checks won’t see a truncated
.jsonl as a valid file.
- Line 55: The current alias guard in migrate.go only compares cleaned paths and
resolveRealPath(), which misses bind-mount aliases and can let migration target
the live conversation file. Update the migration path check around the existing
filepath.Clean and resolveRealPath logic to detect same-file/directory identity
first for existing paths, then fall back to the current string-based comparison
when either path is missing. Use the migrate flow and resolveRealPath helper as
the anchor points for the fix.
- Around line 176-194: Update newestConversationBackup so it no longer selects
the backup by os.Stat ModTime; instead, parse the numeric epoch from each
sessionID+".jsonl.bak-<epoch>" filename returned by filepath.Glob and choose the
highest suffix value. Keep the existing globbing and regular-file checks, but
use the suffix as the comparison key so the newest recovery point is returned
even when mtimes are preserved by rename.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: f4f1321c-92f5-4db6-97ae-9056c246f083

📥 Commits

Reviewing files that changed from the base of the PR and between 0e11487 and f3d23b8.

📒 Files selected for processing (3)
  • internal/session/instance.go
  • internal/session/migrate.go
  • internal/session/migrate_test.go

Comment on lines +2957 to +2958
if i.Tool == "claude" && i.ClaudeSessionID != "" {
if restored, err := RestoreOrphanedConversationBackup(i, GetClaudeConfigDirForInstance(i)); err == nil && restored != "" {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Inspect Claude-compatible tool values and account-switch migration call sites.
rg -n -C3 'func IsClaudeCompatible|IsClaudeCompatible\(|Tool == "claude"|RestoreOrphanedConversationBackup|MigrateConversationFrom' internal/session

Repository: asheshgoplani/agent-deck

Length of output: 50380


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== migrate.go (restore helper) ==\n'
sed -n '1,180p' internal/session/migrate.go

printf '\n== instance.go (Start/restart restore call sites) ==\n'
sed -n '2948,2990p' internal/session/instance.go
sed -n '5968,5990p' internal/session/instance.go

printf '\n== userconfig.go (IsClaudeCompatible) ==\n'
sed -n '3028,3058p' internal/session/userconfig.go

printf '\n== config dir resolver references ==\n'
rg -n -C2 'func GetClaudeConfigDirForInstance|GetClaudeConfigDir\(' internal/session

Repository: asheshgoplani/agent-deck

Length of output: 27954


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== call sites for MigrateConversation / RestoreOrphanedConversationBackup ==\n'
rg -n -C2 'MigrateConversation\(|MigrateConversationFrom\(|RestoreOrphanedConversationBackup\(' internal/session

printf '\n== exact claude-gates near resume/migration ==\n'
rg -n -C2 'Tool == "claude".*(RestoreOrphanedConversationBackup|MigrateConversation)|RestoreOrphanedConversationBackup\(.*Tool != "claude"|MigrateConversationFrom\(.*Tool != "claude"' internal/session

printf '\n== custom Claude-compatible semantics around sessions ==\n'
rg -n -C2 'IsClaudeCompatible\(.*\).*session|custom tool.*Claude-compatible|full Claude functionality' internal/session/userconfig.go internal/session/instance.go internal/session/*.go

Repository: asheshgoplani/agent-deck

Length of output: 8133


Extend orphan backup recovery to Claude-compatible tools (internal/session/instance.go:2957-2958, 5977-5978). Both resume paths are hard-gated to i.Tool == "claude", so custom Claude-compatible wrappers can still miss .jsonl.bak-* recovery and resume empty after the account-switch loss path. Use IsClaudeCompatible(i.Tool) here or broaden RestoreOrphanedConversationBackup to accept compatible tools.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/session/instance.go` around lines 2957 - 2958, The orphaned
conversation backup recovery is hard-gated to only the exact “claude” tool, so
Claude-compatible wrappers can skip .jsonl.bak-* restoration and resume empty.
Update the resume/recovery checks around RestoreOrphanedConversationBackup and
the related Claude session restore path to use IsClaudeCompatible(i.Tool) (or
otherwise allow compatible tools) while preserving the existing ClaudeSessionID
and backup-dir behavior. Make sure the logic in instance.go consistently treats
compatible tools the same as claude for backup recovery.

Source: Path instructions

if i.adoptExplicitClaudeSessionID("session_id_flag_explicit") {
explicitSessionID := i.adoptExplicitClaudeSessionID("session_id_flag_explicit")
if i.Tool == "claude" && i.ClaudeSessionID != "" {
if restored, err := RestoreOrphanedConversationBackup(i, GetClaudeConfigDirForInstance(i)); err == nil && restored != "" {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Don’t silently drop restore failures before resume.

If RestoreOrphanedConversationBackup fails while the live <id>.jsonl is missing, the code proceeds into claude --resume with no warning. Log the error at minimum; on Restart(), consider returning before respawn to avoid an empty-history resume.

Suggested error handling
-		if restored, err := RestoreOrphanedConversationBackup(i, GetClaudeConfigDirForInstance(i)); err == nil && restored != "" {
+		if restored, err := RestoreOrphanedConversationBackup(i, GetClaudeConfigDirForInstance(i)); err != nil {
+			sessionLog.Warn("resume: orphaned conversation backup restore failed",
+				slog.String("instance_id", i.ID),
+				slog.String("claude_session_id", i.ClaudeSessionID),
+				slog.String("reason", "orphan_bak_restore"),
+				slog.String("error", err.Error()))
+		} else if restored != "" {
-			if restored, rErr := RestoreOrphanedConversationBackup(i, GetClaudeConfigDirForInstance(i)); rErr == nil && restored != "" {
+			if restored, rErr := RestoreOrphanedConversationBackup(i, GetClaudeConfigDirForInstance(i)); rErr != nil {
+				sessionLog.Warn("resume: orphaned conversation backup restore failed",
+					slog.String("instance_id", i.ID),
+					slog.String("claude_session_id", i.ClaudeSessionID),
+					slog.String("reason", "orphan_bak_restore_restart"),
+					slog.String("error", rErr.Error()))
+			} else if restored != "" {

Also applies to: 5978-5978

🧰 Tools
🪛 ast-grep (0.44.0)

[warning] 2958-2962: A log/format call (log.Print/Printf/Println, the Fatal/Panic variants, fmt.Sprintf, or a structured logger's Info/Warn/Error/Debug method) is given a message built by concatenating a string literal with a non-literal value such as request data. Unsanitized, attacker-controlled input written to logs enables log forging / CRLF injection: an attacker can inject newlines to spoof log entries or break log parsers. Do not concatenate raw input into the log message; pass it as a separate structured field/argument (e.g. 'log.Printf("user: %s", user)' or 'logger.Info("login", "user", user)') and strip or escape newline characters first.
Context: sessionLog.Info("resume: restored orphaned conversation backup id="+i.ClaudeSessionID+" reason=orphan_bak_restore",
slog.String("instance_id", i.ID),
slog.String("claude_session_id", i.ClaudeSessionID),
slog.String("path", restored),
slog.String("reason", "orphan_bak_restore"))
Note: [CWE-117] Improper Output Neutralization for Logs.

(log-injection-request-data-concat-go)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/session/instance.go` at line 2958, RestoreOrphanedConversationBackup
failures are being ignored before resume, which can lead to an empty-history
claude --resume with no visibility. Update the restore-and-resume path in the
instance session flow around RestoreOrphanedConversationBackup to log any
returned error when the live conversation JSONL is missing, and in Restart()
consider stopping the respawn/resume path early if restore fails so we don’t
proceed blindly. Use the existing resume logic and backup restore helpers to
surface the failure clearly.

Comment on lines +2959 to +2963
sessionLog.Info("resume: restored orphaned conversation backup id="+i.ClaudeSessionID+" reason=orphan_bak_restore",
slog.String("instance_id", i.ID),
slog.String("claude_session_id", i.ClaudeSessionID),
slog.String("path", restored),
slog.String("reason", "orphan_bak_restore"))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Keep structured log messages constant.

i.ClaudeSessionID is already emitted as claude_session_id; concatenating it into the message reintroduces the log-forging pattern flagged by static analysis.

Suggested log cleanup
-			sessionLog.Info("resume: restored orphaned conversation backup id="+i.ClaudeSessionID+" reason=orphan_bak_restore",
+			sessionLog.Info("resume: restored orphaned conversation backup",
-				sessionLog.Info("resume: restored orphaned conversation backup id="+i.ClaudeSessionID+" reason=orphan_bak_restore_restart",
+				sessionLog.Info("resume: restored orphaned conversation backup",

Also applies to: 5979-5983

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/session/instance.go` around lines 2959 - 2963, Keep the session log
message constant in the restore path and avoid concatenating i.ClaudeSessionID
into the text, since it is already emitted via the structured claude_session_id
field. Update the sessionLog.Info call in the orphan backup restore flow to use
a fixed message and rely on the existing slog fields for instance_id,
claude_session_id, path, and reason. Apply the same cleanup to the other
referenced restore log site as well so both calls follow the same
structured-logging pattern.

Source: Linters/SAST tools

return "", fmt.Errorf("source and target config dirs must be non-empty")
}
if filepath.Clean(src) == filepath.Clean(dst) {
if filepath.Clean(src) == filepath.Clean(dst) || resolveRealPath(src) == resolveRealPath(dst) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '== migrate.go outline ==\n'
ast-grep outline internal/session/migrate.go --view expanded || true

printf '\n== search for resolveRealPath ==\n'
rg -n "resolveRealPath|filepath.Clean\\(src\\)|filepath.Clean\\(dst\\)" internal/session -S || true

printf '\n== read migrate.go relevant slices ==\n'
sed -n '1,260p' internal/session/migrate.go | cat -n

printf '\n== read any helper definitions around resolveRealPath ==\n'
rg -n "func resolveRealPath|os\\.Lstat|EvalSymlinks|RealPath" internal/session -S || true

Repository: asheshgoplani/agent-deck

Length of output: 20665


Detect bind-mount aliases before migrating. resolveRealPath() only follows symlinks, so two paths to the same mounted config dir can still pass this guard. That lets migration run against the live conversation file and can clobber it when the target file doesn’t already exist; compare existing dirs by file identity first, then fall back to the current string check for missing paths.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/session/migrate.go` at line 55, The current alias guard in
migrate.go only compares cleaned paths and resolveRealPath(), which misses
bind-mount aliases and can let migration target the live conversation file.
Update the migration path check around the existing filepath.Clean and
resolveRealPath logic to detect same-file/directory identity first for existing
paths, then fall back to the current string-based comparison when either path is
missing. Use the migrate flow and resolveRealPath helper as the anchor points
for the fix.

Source: Path instructions

Comment on lines 93 to 100
if err := copyFileVerified(srcFile, dstFile); err != nil {
if bak != "" {
_ = os.Remove(dstFile)
if restoreErr := os.Rename(bak, dstFile); restoreErr != nil {
return "", fmt.Errorf("%w (restore backup failed: %v)", err, restoreErr)
}
}
return "", err

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Remove partial live files after failed copies.

copyFileVerified can create/truncate dstFile/live before returning an error. On failure without a backup, or during restore fallback, the partial .jsonl remains and later resume/restore paths will treat it as valid because fileIsRegular succeeds.

Proposed fix
 if err := copyFileVerified(srcFile, dstFile); err != nil {
+	_ = os.Remove(dstFile)
 	if bak != "" {
-		_ = os.Remove(dstFile)
 		if restoreErr := os.Rename(bak, dstFile); restoreErr != nil {
 			return "", fmt.Errorf("%w (restore backup failed: %v)", err, restoreErr)
 		}
 	}
 	return "", err
 }
 if err := copyFileVerified(bak, live); err != nil {
+	_ = os.Remove(live)
 	return "", err
 }

Also applies to: 142-144

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/session/migrate.go` around lines 93 - 100, The failed copy path in
migrate.go leaves a partial destination file behind, especially in the
copyFileVerified and restore-fallback flow. Update the error handling in the
migration routine that uses copyFileVerified, os.Remove, and os.Rename so any
partially written dstFile/live file is deleted on all failure paths, including
when no backup exists and when restoring bak fails. Use the migration helper(s)
around copyFileVerified and the backup restore logic to ensure resume/restore
checks won’t see a truncated .jsonl as a valid file.

Comment on lines +176 to +194
func newestConversationBackup(projDir, sessionID string) (string, error) {
files, err := filepath.Glob(filepath.Join(projDir, sessionID+".jsonl.bak-*"))
if err != nil {
return "", fmt.Errorf("glob orphaned conversation backups: %w", err)
}
var newest string
var newestMod time.Time
for _, file := range files {
info, err := os.Stat(file)
if err != nil || !info.Mode().IsRegular() {
continue
}
if newest == "" || info.ModTime().After(newestMod) {
newest = file
newestMod = info.ModTime()
}
}
return newest, nil
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

sed -n '1,260p' internal/session/migrate.go

Repository: asheshgoplani/agent-deck

Length of output: 7853


🏁 Script executed:

rg -n "parseConversationBackupEpoch|newestConversationBackup|jsonl\.bak-|backup" internal/session -S

Repository: asheshgoplani/agent-deck

Length of output: 6821


🏁 Script executed:

ast-grep outline internal/session/migrate.go --view expanded

Repository: asheshgoplani/agent-deck

Length of output: 798


🏁 Script executed:

sed -n '1,380p' internal/session/migrate_test.go

Repository: asheshgoplani/agent-deck

Length of output: 13903


🏁 Script executed:

sed -n '250,360p' internal/session/instance.go

Repository: asheshgoplani/agent-deck

Length of output: 6363


🏁 Script executed:

sed -n '250,360p' internal/session/migrate_test.go

Repository: asheshgoplani/agent-deck

Length of output: 4416


🏁 Script executed:

sed -n '5930,6005p' internal/session/instance.go

Repository: asheshgoplani/agent-deck

Length of output: 3816


🏁 Script executed:

python3 - <<'PY'
import os, tempfile, time, pathlib, re

with tempfile.TemporaryDirectory() as d:
    sid = "abc123"
    older = pathlib.Path(d) / f"{sid}.jsonl.bak-100"
    newer = pathlib.Path(d) / f"{sid}.jsonl.bak-200"
    older.write_text("older suffix, newer mtime\n")
    newer.write_text("newer suffix, older mtime\n")

    now = time.time()
    os.utime(older, (now, now))          # make suffix-100 look newer on disk
    os.utime(newer, (now - 3600, now - 3600))

    files = [str(older), str(newer)]

    # current helper behavior: pick newest ModTime
    newest = ""
    newest_mtime = -1
    for f in files:
        st = os.stat(f)
        if newest == "" or st.st_mtime > newest_mtime:
            newest = f
            newest_mtime = st.st_mtime

    # suffix-based behavior: pick largest epoch
    def epoch(path):
        m = re.search(r"\.bak-(\d+)$", path)
        return int(m.group(1)) if m else -1
    suffix_newest = max(files, key=epoch)

    print("mtime चयन:", pathlib.Path(newest).name)
    print("suffix चयन:", pathlib.Path(suffix_newest).name)
PY

Repository: asheshgoplani/agent-deck

Length of output: 227


Select the newest backup by its .bak-<epoch> suffix

newestConversationBackup currently picks by file mtime, but these backups are renamed files and can keep the source conversation’s mtime. With multiple .bak-* files, that can restore an older recovery point; parse the suffix epoch and choose the highest one instead.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/session/migrate.go` around lines 176 - 194, Update
newestConversationBackup so it no longer selects the backup by os.Stat ModTime;
instead, parse the numeric epoch from each sessionID+".jsonl.bak-<epoch>"
filename returned by filepath.Glob and choose the highest suffix value. Keep the
existing globbing and regular-file checks, but use the suffix as the comparison
key so the newest recovery point is returned even when mtimes are preserved by
rename.

Copy link
Copy Markdown
Owner

🤖 autopilot: CI partially red — CodeQL failed (completed in ~5s, likely a GitHub service outage rather than a real code finding). All other checks passed (Full test suite, golangci, govulncheck, eval_smoke, TestPersistence, harness unit tests, diffscope). Cannot auto-merge until CodeQL is green — please re-run or wait for the service to recover.


Generated by Claude Code

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.

Account switch (session set account / switch-account) renames live conversation .jsonl to .bak and breaks --resume

3 participants