Skip to content

refactor(recover): drop unreachable legacy fallback in collision check #25575

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.

Sign up for GitHub

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

Merged
clopen-set merged 1 commit into from
Apr 14, 2026
Merged

refactor(recover): drop unreachable legacy fallback in collision check #25575

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 2 additions & 8 deletions cli/src/commands/recover.ts
View file
Open in desktop

@devin-ai-integration devin-ai-integration Bot Apr 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚩 Pre-existing tar extraction target mismatch for multi-instance entries

The TODO at cli/src/commands/recover.ts:65-67 documents a pre-existing issue: the tar extraction at line 68 is hardcoded to homedir(), but the collision check now correctly uses entry.resources.instanceDir. For multi-instance entries where instanceDir differs from homedir(), the archive would extract to the wrong location. This PR doesn't introduce the mismatch — it was already present — but the collision check fix at line 55 makes the asymmetry more visible: the check guards the right path while the extraction writes to a potentially different path.

(Refers to lines 64-68)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Original file line number Diff line number Diff line change
Expand Up @@ -51,14 +51,8 @@ export async function recover(): Promise<void> {
);
}

// 3. Check that the recovering entry's own target directory is free. Only
// this one path matters — iterating all lockfile entries would block
// recovery whenever any unrelated local assistant is still installed.
// Fall back to the legacy `~/.vellum` path for entries without
// resources (pre env-data-layout installs).
const target = entry.resources?.instanceDir
? join(entry.resources.instanceDir, ".vellum")
: join(homedir(), ".vellum");
// 3. Check that the recovering entry's own target directory is free.
const target = join(entry.resources.instanceDir, ".vellum");

@chatgpt-codex-connector chatgpt-codex-connector Bot Apr 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Validate instanceDir before building recover target path

recover() only checks that entry.resources exists, then immediately uses entry.resources.instanceDir in join(...); if archived metadata has a partial resources object without instanceDir, this now throws or checks the wrong path (e.g., empty string) before extraction. That case is realistic because the lockfile migration code explicitly backfills missing resources.instanceDir (cli/src/lib/assistant-config.ts), but recover() reads archive metadata directly and skips that migration path. The previous ternary handled this by falling back to ~/.vellum; after this change, recovery can fail with an unhandled runtime error for legacy/partial archives.

Useful? React with 👍 / 👎.

if (existsSync(target)) {
console.error(
`Error: ${target} already exists (owned by ${entry.assistantId}). ` +
Expand Down