-
Notifications
You must be signed in to change notification settings - Fork 70
fix: prevent conversation loss when switching Claude accounts #1549
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2953,7 +2953,17 @@ func (i *Instance) ensureClaudeSessionIDFromDisk() { | |
| // transcripts (e.g. a removed-then-recreated review session) cannot hijack | ||
| // a sibling's conversation. The Restart prelude already does this for | ||
| // #1147; this closes the same gap on the Start path. | ||
| 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 != "" { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. (log-injection-request-data-concat-go) 🤖 Prompt for AI Agents |
||
| 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")) | ||
|
Comment on lines
+2959
to
+2963
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win Keep structured log messages constant.
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 AgentsSource: Linters/SAST tools |
||
| } | ||
| } | ||
| if explicitSessionID { | ||
| return | ||
| } | ||
| if i.ClaudeSessionID != "" { | ||
|
|
@@ -5959,6 +5969,20 @@ func (i *Instance) Restart() error { | |
|
|
||
| // If Claude session with known ID AND tmux session exists, use respawn-pane. | ||
| if IsClaudeCompatible(i.Tool) && i.ClaudeSessionID != "" && i.tmuxSession != nil && i.tmuxSession.Exists() { | ||
| // A known-ID restart resumes straight to `claude --resume <uuid>` | ||
| // without passing through the Start-path prelude. If the live | ||
| // <id>.jsonl was lost to the #1533 account-switch bug but an | ||
| // orphaned <id>.jsonl.bak-<epoch> remains, restore it here so the | ||
| // resume finds its history instead of starting empty. | ||
| if i.Tool == "claude" { | ||
| if restored, rErr := RestoreOrphanedConversationBackup(i, GetClaudeConfigDirForInstance(i)); rErr == nil && restored != "" { | ||
| sessionLog.Info("resume: restored orphaned conversation backup id="+i.ClaudeSessionID+" reason=orphan_bak_restore_restart", | ||
| slog.String("instance_id", i.ID), | ||
| slog.String("claude_session_id", i.ClaudeSessionID), | ||
| slog.String("path", restored), | ||
| slog.String("reason", "orphan_bak_restore_restart")) | ||
| } | ||
| } | ||
| resumeCmd, containerName, err := i.prepareCommand(i.buildClaudeResumeCommand()) | ||
| if err != nil { | ||
| return err | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,7 +52,7 @@ | |
| if src == "" || dst == "" { | ||
| 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) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 || trueRepository: asheshgoplani/agent-deck Length of output: 20665 Detect bind-mount aliases before migrating. 🤖 Prompt for AI AgentsSource: Path instructions |
||
| return "", nil | ||
| } | ||
|
|
||
|
|
@@ -82,19 +82,69 @@ | |
| return "", fmt.Errorf("create target project dir: %w", err) | ||
| } | ||
| dstFile := filepath.Join(dstProjDir, sid+".jsonl") | ||
| bak := "" | ||
| if fileIsRegular(dstFile) { | ||
| // Backup before any destructive write (2026-06-04 incident, S2). | ||
| bak := fmt.Sprintf("%s.bak-%d", dstFile, time.Now().Unix()) | ||
| bak = fmt.Sprintf("%s.bak-%d", dstFile, time.Now().Unix()) | ||
| if err := os.Rename(dstFile, bak); err != nil { | ||
| return "", fmt.Errorf("backup existing conversation: %w", err) | ||
| } | ||
| } | ||
| 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 | ||
|
Comment on lines
93
to
100
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
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 |
||
| } | ||
| return dstFile, nil | ||
| } | ||
|
|
||
| // RestoreOrphanedConversationBackup restores a conversation whose live | ||
| // <id>.jsonl went missing but whose most recent <id>.jsonl.bak-<epoch> | ||
| // orphan still exists in the project dir (the #1533 data-loss residue). | ||
| // It is a no-op when a live <id>.jsonl is already present, when inst has | ||
| // no ClaudeSessionID, or when no matching .bak- orphan exists. Returns the | ||
| // restored path (or "" for no-op) and any error. | ||
| func RestoreOrphanedConversationBackup(inst *Instance, configDir string) (string, error) { | ||
| if inst == nil || inst.Tool != "claude" || inst.ClaudeSessionID == "" || strings.TrimSpace(configDir) == "" { | ||
| return "", nil | ||
| } | ||
|
|
||
| cfgDir := ExpandPath(strings.TrimSpace(configDir)) | ||
| projectPath := inst.EffectiveWorkingDir() | ||
| resolvedPath := projectPath | ||
| if resolved, err := filepath.EvalSymlinks(projectPath); err == nil { | ||
| resolvedPath = resolved | ||
| } | ||
| encodedPath := ConvertToClaudeDirName(resolvedPath) | ||
| if encodedPath == "" { | ||
| encodedPath = "-" | ||
| } | ||
| projDir := filepath.Join(cfgDir, "projects", encodedPath) | ||
| live := filepath.Join(projDir, inst.ClaudeSessionID+".jsonl") | ||
| if fileIsRegular(live) { | ||
| return "", nil | ||
| } | ||
|
|
||
| bak, err := newestConversationBackup(projDir, inst.ClaudeSessionID) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| if bak == "" { | ||
| return "", nil | ||
| } | ||
| if err := os.Rename(bak, live); err == nil { | ||
Check failureCode scanning / CodeQL Uncontrolled data used in path expression High
This path depends on a
user-provided value Error loading related location Loading Check failureCode scanning / CodeQL Uncontrolled data used in path expression High
This path depends on a
user-provided value Error loading related location Loading |
||
|
|
||
| return live, nil | ||
| } | ||
| if err := copyFileVerified(bak, live); err != nil { | ||
| return "", err | ||
| } | ||
| return live, nil | ||
| } | ||
|
|
||
| // newestConversationFile returns the most recently modified UUID-named | ||
| // conversation file in projDir (and its session id), skipping agent-*.jsonl. | ||
| // Unlike findActiveSessionIDExcluding it has no recency cutoff: a conversation | ||
|
|
@@ -123,6 +173,26 @@ | |
| return path, sessionID | ||
| } | ||
|
|
||
| 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) | ||
Check failureCode scanning / CodeQL Uncontrolled data used in path expression High
This path depends on a
user-provided value Error loading related location Loading |
||
|
|
||
| if err != nil || !info.Mode().IsRegular() { | ||
| continue | ||
| } | ||
| if newest == "" || info.ModTime().After(newestMod) { | ||
| newest = file | ||
| newestMod = info.ModTime() | ||
| } | ||
| } | ||
| return newest, nil | ||
| } | ||
|
Comment on lines
+176
to
+194
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.goRepository: asheshgoplani/agent-deck Length of output: 7853 🏁 Script executed: rg -n "parseConversationBackupEpoch|newestConversationBackup|jsonl\.bak-|backup" internal/session -SRepository: asheshgoplani/agent-deck Length of output: 6821 🏁 Script executed: ast-grep outline internal/session/migrate.go --view expandedRepository: asheshgoplani/agent-deck Length of output: 798 🏁 Script executed: sed -n '1,380p' internal/session/migrate_test.goRepository: asheshgoplani/agent-deck Length of output: 13903 🏁 Script executed: sed -n '250,360p' internal/session/instance.goRepository: asheshgoplani/agent-deck Length of output: 6363 🏁 Script executed: sed -n '250,360p' internal/session/migrate_test.goRepository: asheshgoplani/agent-deck Length of output: 4416 🏁 Script executed: sed -n '5930,6005p' internal/session/instance.goRepository: 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)
PYRepository: asheshgoplani/agent-deck Length of output: 227 Select the newest backup by its
🤖 Prompt for AI Agents |
||
|
|
||
| // copyFileVerified copies src to dst (0600, matching Claude's conversation | ||
| // files) and verifies the written size matches the source. | ||
| func copyFileVerified(src, dst string) error { | ||
|
|
||
There was a problem hiding this comment.
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:
Repository: asheshgoplani/agent-deck
Length of output: 50380
🏁 Script executed:
Repository: asheshgoplani/agent-deck
Length of output: 27954
🏁 Script executed:
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. UseIsClaudeCompatible(i.Tool)here or broadenRestoreOrphanedConversationBackupto accept compatible tools.🤖 Prompt for AI Agents
Source: Path instructions