Skip to content

fix: delete codex sessions with backup#227

Draft
kkLullaby wants to merge 4 commits into
vakovalskii:mainfrom
kkLullaby:feat/codex-memory-isolation
Draft

fix: delete codex sessions with backup#227
kkLullaby wants to merge 4 commits into
vakovalskii:mainfrom
kkLullaby:feat/codex-memory-isolation

Conversation

@kkLullaby

Copy link
Copy Markdown

No description provided.

Copilot AI review requested due to automatic review settings May 26, 2026 08:50

Copilot AI 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.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds support for deleting Codex sessions by backing up and removing Codex session artifacts (session file, history/index entries, and optional sqlite thread row), along with tests for key deletion scenarios.

Changes:

  • Add Codex-specific delete path in deleteSession() with backup creation and artifact cleanup.
  • Implement helpers to collect/remove JSONL session references and prune empty session directories.
  • Add node:test coverage for Codex deletion (session file + history/index; history-only).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File Description
test/codex-delete.test.js Adds tests validating Codex deletion behavior and backup creation.
src/data.js Implements Codex-aware deletion, backups, JSONL cleanup helpers, and cache invalidation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread test/codex-delete.test.js
Comment on lines +7 to +9
function tmpDir() {
return fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'codbash-codex-delete-')));
}
Comment thread test/codex-delete.test.js
}

test('deleteSession removes Codex artifacts after creating a backup', () => {
const home = tmpDir();
Comment thread src/data.js
Comment on lines +2581 to +2590
function getCodexDeleteBackupRoot() {
if (process.env.CODEBASH_DELETE_BACKUP_DIR) {
return path.resolve(process.env.CODEBASH_DELETE_BACKUP_DIR);
}

const userBackup = path.join(ALL_HOMES[0], 'backup', 'codex');
if (fs.existsSync(userBackup)) return path.join(userBackup, 'codbash-deleted');

return path.join(CODEX_DIR, 'backups', 'codbash-deleted');
}
Comment thread src/data.js
Comment on lines +2610 to +2619
function removeJsonlLinesForSession(filePath, sessionId) {
if (!fs.existsSync(filePath)) return 0;
const lines = readLines(filePath);
const filtered = lines.filter(line => codexLineSessionId(line) !== sessionId);
const removed = lines.length - filtered.length;
if (removed > 0) {
fs.writeFileSync(filePath, filtered.length ? filtered.join('\n') + '\n' : '');
}
return removed;
}
Comment thread src/data.js
Comment on lines +2739 to +2750
const safeId = safeSqlString(sessionId);
const stateDb = path.join(CODEX_DIR, 'state_5.sqlite');
if (safeId && fs.existsSync(stateDb)) {
try {
execFileSync('sqlite3', [stateDb, `DELETE FROM threads WHERE id = '${safeId}';`], {
timeout: 5000,
windowsHide: true,
stdio: ['pipe', 'pipe', 'pipe'],
});
deleted.push('codex state thread');
} catch {}
}
@kkLullaby
kkLullaby marked this pull request as draft May 27, 2026 01:14
@vakovalskii

Copy link
Copy Markdown
Owner

Thanks for this — the Codex delete-with-backup logic is genuinely valuable; today deleting a Codex session on main is essentially a no-op, so backing up the rollout JSONL, history/index lines, and the state_5 thread row before removal is a real improvement, and validating the id before the sqlite3 calls is exactly right.

Two things before it can come out of draft:

  1. It now conflicts with maindeleteSession recently gained input-validation guards, so a rebase is needed.
  2. The branch bundles the larger "codex memory isolation" scaffold (plan docs + codex-memory.js + /api/codex-memory/*) with the delete fix. Per our contributing guidelines we keep PRs small and single-purpose — could you split the codex delete-with-backup into its own focused PR rebased on main? That part I'd review right away. The memory-isolation design (OpenAI summaries/embeddings/clustering) is a bigger discussion, especially re: the core's zero-dependency constraint — let's track it separately.

Minor: the ~/backup/codex default backup path is a bit specific; consider defaulting under ~/.codex/backups with the env override you already have.

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.

3 participants