fix: keep backup files out of preset export archives

[dngrtech]

Problem

Importing a preset export failed with 400:

Unsupported script file: scripts/ranked.py.bak-pre-player-ip-connected-20260704-222233

Export and import disagreed about what belongs in a preset archive:

• Export (_build_preset_export_zip) walks the whole preset directory and includes anything not matching EXPORT_EXCLUDED_PATTERNS — only *.pyc, *.pyo, *.swp, *.tmp, *~. A backup file left beside a script didn't match, so it went into the ZIP.
• Import (_validate_script_path) allows only .py, .txt, .so and font extensions under scripts/, and hard-fails the entire archive on anything else.

So QLSM was writing archives QLSM itself refuses to import.

Fix

Add ARCHIVE_EXCLUDED_PATTERNS = ('*.bak', '*.bak-*', '*.bak.*', '*.orig', '*.rej') and apply it in _should_skip_export_path, which export and import both call. Archives already exported with a stray backup now import cleanly — no re-export needed.

Patterns are deliberately precise rather than *.bak*, so a legitimately named bakery.py isn't swallowed.

What is intentionally not changed

_ignore_generated_script_cruft stays on the narrower list. It feeds the copytree that repopulates a preset's scripts/ directory after an rmtree (preset_api_routes.py:1122, :1391), so filtering backups there would silently delete a user's own .bak files on the next preset save. The helper is split into _matches_excluded_path so the archive filter can be broad while the draft-save filter stays narrow.

Tests

• test_skips_backup_files_left_beside_scripts — the reported filename plus .bak, .bak.1, .orig, .rej, and a root-level server.cfg.bak
• test_keeps_scripts_whose_name_merely_contains_bak — guards against over-broad patterns
• test_download_preset_excludes_backup_files — export side
• test_update_preset_from_draft_keeps_user_backup_files — regression guard for the deletion side effect above

The first three fail without the fix. The fourth passes before and after — that's its purpose.

Known limitation

This addresses the backup-file class. The underlying export/import asymmetry remains: any other unexpected extension under scripts/ (a stray .md, .json) still 400s the whole import. Making the importer skip-with-warning instead of hard-failing is a behavior change with a security-boundary tradeoff, so it's left for a separate discussion.

[github-actions]

Code Review

Strengths

• Clear separation of concerns (preset_api_routes.py:68–108): Splitting _should_skip_export_path into a thin wrapper over _matches_excluded_path(patterns) is the right abstraction. The archive filter and the draft-save filter now differ only in the patterns argument, making the distinction explicit and auditable.
• Intentional asymmetry is documented (preset_api_routes.py:88–93): The docstring on _ignore_generated_script_cruft clearly explains why backup files are kept during draft saves (they live in the user's real scripts directory) but dropped from archives (the import validator rejects unknown extensions). This is a non-obvious invariant and the comment earns its place.
• Pattern set is correct and complete for the reported issue: *.bak, *.bak-*, and *.bak.* handle the three structural variants of timestamped or numbered backups; *.orig and *.rej cover merge-tool artefacts. Each pattern handles a structurally distinct case without overlapping.
• False-positive test is present (test_preset_import_validation.py:355–363): test_keeps_scripts_whose_name_merely_contains_bak explicitly guards against bakery.py being swallowed — exactly the kind of boundary case that catches substring-match bugs.
• Round-trip symmetry (test_preset_download_routes.py:263–293): test_download_preset_excludes_backup_files verifies the archive side; test_update_preset_from_draft_keeps_user_backup_files verifies the draft-save side preserves those same files. Together they pin the contract from both directions.
• No new dependencies, no auth surface changes, no logging of sensitive data: security footprint is unchanged.

---

Issues

Critical (Must Fix)

None.

Important (Should Fix)

None.

Minor (Nice to Have)

_matches_excluded_path has no docstring (preset_api_routes.py:68).
The patterns parameter is the load-bearing distinction between archive and draft-save behaviour. A one-line note — e.g. """Core path filter; callers supply the pattern set.""" — would make it immediately clear why two callers pass different values rather than sharing a constant.

*.bak-* could match any file with .bak- anywhere in its name (preset_api_routes.py:57).
fnmatch.fnmatch('foo.bak-bar.py', '*.bak-*') returns True, so a hypothetically legitimate file named foo.bak-bar.py would be excluded. This is almost certainly fine in practice (no real script would be named that way), but it is worth a comment alongside the pattern to flag the wideness.

---

Assessment

Ready to merge? Yes

Reasoning: This is a focused, well-scoped fix for a concrete bug (archive exports including backup files that QLSM's own import validator then rejects). The refactoring is clean, the asymmetry between archive and draft-save filtering is correctly identified and documented, and all three behavioural paths (archive exclusion, import exclusion, draft-save preservation) are covered by dedicated tests with boundary checks.

[github-actions]

PR Review: Filter backup files on every preset path

Summary

This PR extends EXPORT_EXCLUDED_PATTERNS to cover editor/tooling backup files (.bak, .bak-*, .bak.*, .orig, .rej) and ensures the single _should_skip_export_path function is the gatekeeper on all three preset paths: export ZIP, import ZIP parse, and draft-to-preset copy. Three corresponding test files each gain one or two focused tests.

---

Strengths

• Single point of truth (preset_api_routes.py:73–88): the pattern list lives in one place and is imported by preset_import_validation.py and used by _ignore_generated_script_cruft, so all three paths stay in sync automatically. This is the right design.
• False-positive guard (test_keeps_scripts_whose_name_merely_contains_bak): the test explicitly checks that bakery.py is not swallowed, which validates the fnmatch patterns don't over-match.
• Layered coverage: the new tests cover export (test_preset_download_routes.py), import (test_preset_import_validation.py), and draft save (test_preset_routes.py) independently — each path is exercised by a dedicated test rather than relying on a single end-to-end case.
• Comment explains the "why" (preset_api_routes.py:54–61): the inline rationale (import validator rejects these extensions → round-trip breaks) prevents someone from removing the patterns as "unnecessary" in the future.
• Path-traversal safety is unaffected: the existing _validate_entry_name / _resolve_export_root safeguards were not changed, and the new patterns only act as an additional skip layer after those checks pass.

---

Issues

Critical (Must Fix)

None.

Important (Should Fix)

1. *.rej pattern may be too broad for the configs/ tree
preset_api_routes.py:61

fnmatch matches on the bare filename regardless of directory. A hypothetical legitimate config file at configs/acl.rej (ACL reject list, for example) would be silently dropped. The .rej extension is rare outside of patch conflicts, but the filter is applied to all paths, not just scripts/. In practice this is a low-probability issue for the current config extension allowlist, but worth being explicit: a comment noting that .rej targets patch reject files, not ACL files, would help a future maintainer audit this confidently. Or tighten to scripts/*.rej only if config files will never legitimately end in .rej.

2. Draft user-hooks copy does not apply the backup filter
preset_api_routes.py:1137–1140

The draft-scripts copy at line 1132 passes ignore=_ignore_generated_script_cruft. The draft-user-hooks copy at line 1139 does not pass an ignore argument. If an editor backup lands in user-hooks/ (less likely but possible), it would be copied into the preset unchanged. This is inconsistent with the stated goal ("one filter for every path"). No test covers this case.

Fix:

shutil.copytree(draft_user_hooks, preset_user_hooks,
                dirs_exist_ok=True,
                ignore=_ignore_generated_script_cruft)

Minor (Nice to Have)

3. Test for *.bak.* (numbered backup) pattern is not represented in the download test
tests/test_preset_download_routes.py:260–293

The import test (test_skips_backup_files_left_beside_scripts) covers ranked.py.bak.1, but the download test does not include a *.bak.* file in its fixture. This is not a gap that risks a regression right now (the patterns are shared), but symmetry would make future refactors safer.

4. Minor: _ignore_generated_script_cruft is never directly unit-tested

The function is indirectly covered by test_update_preset_from_draft_drops_backup_files, but a small direct unit test of the function itself would make it easier to add new patterns in the future and verify the is_dir branch works as expected.

---

Assessment

Ready to merge? Yes

Reasoning: The fix is minimal, correctional, and well-reasoned. The user-hooks gap (issue #2) is a real inconsistency but affects a path that is currently unlikely to receive backup files, and does not introduce any new security risk. All three main preset paths are covered by the new filter and tested. The pattern design (single shared constant) is sound and prevents future drift.

[github-actions]

PR Review: Backup file filtering across export, import, and draft→preset copy

Strengths

• Single source of truth (ui/routes/preset_api_routes.py:73): _should_skip_export_path is the one filter used for export, import, and draft→preset copy. Adding the patterns once propagates everywhere automatically — the import path picks them up via the explicit import in preset_import_validation.py:14.
• False-positive test (tests/test_preset_import_validation.py:337): test_keeps_scripts_whose_name_merely_contains_bak and the bakery.py assertion in test_preset_routes.py prove the filter does not swallow legitimately named files.
• Symmetric round-trip fix: The bug that motivated this — exporting a preset with a stale .bak file and then having QLSM refuse to re-import that archive — is closed cleanly by applying the same filter on both ends.
• user-hooks gap closed (ui/routes/preset_api_routes.py:1140–1144, 1409–1413): The prior shutil.copytree for user-hooks lacked the ignore callback; this PR fills that gap with a dedicated test (test_preset_routes.py:1628–1652).

---

Issues

Critical (Must Fix)

None found.

---

Important (Should Fix)

1. create_preset_api user-hooks path has no coverage
ui/routes/preset_api_routes.py:1140–1144 (the create path) gets the same fix as the update path, but test_preset_routes.py only tests the update path (client.put). If someone accidentally reverts or mis-merges just that call-site, no test would catch it.

Fix: Add a parallel test that creates a preset from a draft containing a .bak file in user-hooks via client.post('/api/presets', ...).

2. _ignore_generated_script_cruft docstring is stale
ui/routes/preset_api_routes.py:92: the docstring still reads "Return generated/editor junk to skip when saving draft scripts." The function is now also called for user-hooks, so the docstring under-describes the scope.

Fix: Update to something like "Return generated/editor junk to skip when copying draft files into a preset (scripts and user-hooks)."

---

Minor (Nice to Have)

3. *.bak-* is subtly broad
ui/routes/preset_api_routes.py:61: *.bak-* matches any filename containing the substring .bak- (e.g. a hypothetical hook.bak-copy.so). In practice this matches the QLSM-generated foo.py.bak-pre-<event>-<stamp> pattern, so the risk is low, but the intent could be made explicit with a comment referencing the stamp format.

4. Comment count in EXPORT_EXCLUDED_PATTERNS comment
ui/routes/preset_api_routes.py:54: "The trailing five are backup copies" — currently five patterns are listed (*.bak, *.bak-*, *.bak.*, *.orig, *.rej). If a sixth is ever added, the count will silently lie. Consider dropping the count or saying "the trailing group."

---

Assessment

Ready to merge? Yes, with the optional addition of a create_preset_api user-hooks test.

Reasoning: The core logic is correct and consistent across all three code paths; the patterns are safe and the false-positive tests give confidence the filter is narrow enough. The only real gap is missing coverage for the create path's user-hooks branch, which is low-risk given it calls the same helper but worth adding before this code touches production.