Skip to content

fix(plugin): reject SQLite snapshot self-copy - #513

Open
sylvesterkaczmarek wants to merge 3 commits into
openai:mainfrom
sylvesterkaczmarek:fix/sqlite-snapshot-self-copy
Open

fix(plugin): reject SQLite snapshot self-copy#513
sylvesterkaczmarek wants to merge 3 commits into
openai:mainfrom
sylvesterkaczmarek:fix/sqlite-snapshot-self-copy

Conversation

@sylvesterkaczmarek

@sylvesterkaczmarek sylvesterkaczmarek commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Summary

Fail fast when snapshot_sqlite.py is asked to back up a SQLite database onto the same filesystem object.

Fixes #512.

Problem

The helper opens the source database read-only, opens the destination separately, and calls sqlite3.Connection.backup(). When both connections refer to the same database file, the backup does not make progress and the command can remain stuck indefinitely.

A string comparison is insufficient because another path, including a hard-link alias, can identify the same database.

Changes

  • Rebased onto current main and moved the fix to the canonical plugin source under plugins/codex-security.
  • If the destination already exists, use Path.samefile() to compare filesystem identity with the resolved source.
  • Reject a self-copy through argparse before either SQLite connection is opened.
  • Add Python regression coverage for both the exact source path and a hard-link alias, with a subprocess timeout so a recurrence fails instead of hanging.

Validation

The regression covers:

  • exact source path as destination: exits 2 with the new diagnostic;
  • hard-link alias as destination: exits 2 with the new diagnostic;
  • the normal snapshot path remains unchanged.

Pushed-head CI is required for the full repository validation.

Risk

Low. The normal snapshot path is unchanged. The new check runs only when the destination already exists and rejects only destinations that the operating system reports as the same file as the source.

@github-actions github-actions Bot added the bug Something isn't working label Aug 17, 2026
@Hughhhhcoder

Copy link
Copy Markdown
Contributor

I reproduced this against current main; the filesystem-identity fix and the same-path/hard-link regression still apply. One repository-layout update may help unblock this PR: the canonical plugin source is now plugins/codex-security, while sdk/typescript/_bundled_plugin is generated and current AGENTS.md says not to edit it directly. The equivalent current-main patch belongs in plugins/codex-security/scripts/snapshot_sqlite.py, with regression coverage in plugins/codex-security/tests/test_workbench_setup_and_migrations.py. I verified that placement locally with the required Ruff 0.16.1 lint/format checks, plugin source compatibility check, the 65-test workbench setup/migrations file, and the complete plugin suite (1110 passed, 5 skipped, 110 subtests passed). I will not open a competing PR; sharing this only to make the rebase/port easier.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 5, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
🔒 Security Review Completed 2026-09-06T18:50:00.981149Z 606ee03 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Copy link
Copy Markdown
Contributor Author

Thanks for the pointer. I rebased onto current main and moved the fix into the canonical plugins/codex-security source with Python regression coverage for the same-path and hard-link cases. The PR is mergeable again; the fork CI runs are currently waiting at action_required for workflow approval.

@Hughhhhcoder Hughhhhcoder left a comment

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.

I checked the rebased head 3655b37a locally. The focused regression passes (1 passed) and Ruff lint is clean, but the repository-pinned formatter (ruff==0.16.1) still reports plugins/codex-security/tests/test_snapshot_sqlite.py as unformatted: the multiline assertion at the end is collapsed to one line. Running uv run --project plugins/codex-security --extra test ruff format plugins/codex-security/tests/test_snapshot_sqlite.py (then the corresponding ruff format --check) should clear the gate before fork CI is approved.

@sylvesterkaczmarek

Copy link
Copy Markdown
Contributor Author

Thanks for catching the formatter gate. I ran the repository-pinned Ruff formatter on the regression, verified ruff format --check, reran the focused snapshot test (1 passed), and pushed 606ee03 to the same branch.

@Hughhhhcoder Hughhhhcoder left a comment

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.

Rechecked the updated head 606ee037. The focused self-copy/hard-link regression passes, Ruff lint and the repository-pinned format check are clean, and git diff --check passes. The formatter issue is resolved; I do not see a remaining correctness blocker in this scoped patch.

@sylvesterkaczmarek

Copy link
Copy Markdown
Contributor Author

Thanks for rechecking the updated head and the formatter gate. Appreciate the approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

snapshot_sqlite.py hangs when source and destination are the same database

2 participants