Skip to content

fix: protect checksum workers and GUI garbage collection - #15

Merged
lukisch merged 9 commits into
masterfrom
fix/codex-checksum-close-lifecycle-20261001
Oct 4, 2026
Merged

lukisch merged 9 commits into
masterfrom
fix/codex-checksum-close-lifecycle-20261001

Conversation

@lukisch

@lukisch lukisch commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Keep the checksum dialog and its native worker alive until the worker has actually stopped.
  • Run cyclic Qt-object cleanup on the GUI thread; initialize the timer before disabling automatic collection and restore the original GC state even when shutdown cleanup raises.
  • Close the GUI collector if any application setup after installation fails.
  • Preserve the current master changelog entry alongside the checksum and GUI-GC entry.
  • Prevent command injection when opening a terminal from a path containing shell or Windows Terminal metacharacters; set the target through Popen(cwd=...).

Validation

  • Focused terminal tests: 22 passed.
  • Full local suite: 421 passed, 2 skipped. The skipped release smoke needs a locally built EXE; screenshot smoke needs a native Qt platform.
  • Translation check: all 356 strings complete in six languages.
  • GitHub CI run 37167846716: 9/9 Python 3.10–3.12 jobs passed across Ubuntu, macOS, and Windows, including Ruff and source compilation.
  • CodeQL run 37167844745: success, no new alerts in changed code.
  • Diff check passed.

@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown

Welcome! Thanks for your first pull request in this repository.

A maintainer will review it soon. Please make sure:

  • Your changes are tested
  • Documentation is updated if needed
  • The PR description explains what changed and why

Thanks for contributing.

@lukisch lukisch changed the title fix: close checksum dialog after native worker has joined fix: protect checksum workers and GUI garbage collection Oct 1, 2026
lukisch and others added 3 commits October 1, 2026 20:00

@lukisch lukisch left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Zweitmodell-Review: CI grün (12/12), derselbe Sicherheitsbefund wie in #14.

1. Befehlsinjektion beim Terminal-Start (src/core/platform_utils.py, get_terminal_command)
Der Verzeichnisname wird in powershell -Command "Set-Location -LiteralPath '{directory}'" und cmd /K "cd /d {directory}" interpoliert. Ein Ordnername mit ' oder & (unter Windows zulässig) führt zu Codeausführung. Popen(..., cwd=target_dir) setzt das Arbeitsverzeichnis bereits, die Interpolation kann entfallen:

if shutil.which("powershell"):
    return ["powershell", "-NoExit"]
return ["cmd", "/K"]

Dazu ein Test mit einem Ordner a'b&c.

2. Überlappung
Der Inhalt ist (bis auf die Laufwerkskapazität) in #14 enthalten, batch_rename_service, checksum_dialog, gui_gc und properties_dialog sind in beiden PRs identisch oder fast identisch. Es sollte nur eine der beiden Varianten gemergt werden, sonst Konflikte.

Sonst: Tests für Batch-Rename-Rollback, Properties-Dialog und Terminal vorhanden. Keine Credentials oder Nutzerpfade im Diff.


Generated by Claude Code

lukisch commented Oct 3, 2026

Copy link
Copy Markdown
Collaborator Author

Korrektur zu meinem Review vom 3.10.2026

Frühere Aussage: Der Review nennt „derselbe Sicherheitsbefund wie in #14" als Befund dieses PR.

Korrektur: Die betroffene Funktion in src/core/platform_utils.py steht bereits auf dem Hauptzweig (master, zum Prüfzeitpunkt 8a3989c) und wird von diesem PR nicht geändert: Die Datei steht nicht in der Dateiliste des PR. Geprüft gegen den Stand 2d8120c306b2b3c733be0a0a3a5f1abf69402d99. Es ist kein Befund dieses PR. Siehe auch die Korrektur auf #14.

Bestätigt bleibt: #15 hat gegenüber #14 keinen eigenen Inhalt (#15 ist Git-Vorfahre von #14).

Hinweis zu Details: Weitere technische Einzelheiten bitte über einen dafür geeigneten privaten Kanal klären, nicht in diesem Thread. Entscheidungen bleiben offen.


Generated by Claude Code

@lukisch

lukisch commented Oct 4, 2026

Copy link
Copy Markdown
Collaborator Author

Review (Claude Sonnet 5.5, Zweitmodell nach D-20260902-002) — Empfehlung: merge-ready

Geprüft: kompletter Diff (src, CI, pyproject), Branch im isolierten Worktree.

  • ruff check sauber, compileall ok, pytest -n 2 (mit pytest-qt wie in der CI): 421 passed, 2 skipped, 0 failed. GitHub-CI auf allen 12 Matrix-Jobs grün.
  • ChecksumDialog.done/closeEvent: Dialog wird nie zerstört, solange der Worker-QThread läuft (Polling über wait(0) + Timer, Ergebnis wird genau einmal nachgereicht). Kein Deadlock-Pfad gefunden.
  • GuiGarbageCollector: gc auf den GUI-Thread beschränkt, close() stellt die vorherige GC-Policy wieder her; main() ruft es im finally. Korrekt.
  • Terminal-Injection (get_terminal_command): Pfad wird auf Windows nicht mehr in einen Kommandostring interpoliert (wt -d ., powershell -NoExit, cmd /K + Popen(cwd=...)); macOS/Linux übergeben den Pfad als einzelnes argv-Element (absolut, kein führendes -), shell=True kommt im Code nirgends vor. Dicht.
  • Keine Secrets, keine lokalen Pfade, kein Mojibake.

Befunde: keine blockierenden. Hinweis: pytest-qt ist neu in CI/pyproject (nötig für die qtbot-Tests).

Merge-Reihenfolge: #15 → #14 → #17 (gestapelt: #14 enthält #15, #17 enthält #14+#15). Bitte als Merge-Commit (nicht Squash) mergen, dann bleiben #14/#17 ohne Rebase konfliktfrei. Danach #12, #16, #13 (brauchen nach dem Stapel einen kurzen Abgleich mit master — melden, ich erledige das).

@lukisch
lukisch merged commit 68c2ed4 into master Oct 4, 2026
12 checks passed
@lukisch
lukisch deleted the fix/codex-checksum-close-lifecycle-20261001 branch October 4, 2026 12:16
lukisch pushed a commit that referenced this pull request Oct 4, 2026
lukisch pushed a commit that referenced this pull request Oct 4, 2026
lukisch pushed a commit that referenced this pull request Oct 4, 2026
conftest.py: master-Fixture (GUI-GC) beibehalten, Windows-Symlink-Teardown-Schutz aus diesem PR ergaenzt; test_checksum_dialog.py auf master-Stand (qtbot, waitUntil).
lukisch pushed a commit that referenced this pull request Oct 4, 2026
lukisch pushed a commit that referenced this pull request Oct 4, 2026
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.

1 participant