Skip to content

fix: exclude draft script cache files from presets#138

Merged
dngrtech merged 2 commits into
mainfrom
fix/preset-draft-script-cruft
Jul 7, 2026
Merged

fix: exclude draft script cache files from presets#138
dngrtech merged 2 commits into
mainfrom
fix/preset-draft-script-cruft

Conversation

@dngrtech

@dngrtech dngrtech commented Jul 7, 2026

Copy link
Copy Markdown
Owner

Summary

  • Exclude generated/editor cruft when copying draft scripts into saved presets
  • Cover both create-preset and update-preset draft save paths
  • Bump release metadata to v1.13.9

Test Plan

  • RED: python -m pytest tests/test_preset_routes.py::test_create_preset_from_draft_excludes_python_cache_cruft tests/test_preset_routes.py::test_update_preset_from_draft_excludes_python_cache_cruft -q failed before the fix because __pycache__ was copied
  • GREEN: python -m pytest tests/test_preset_routes.py::test_create_preset_from_draft_excludes_python_cache_cruft tests/test_preset_routes.py::test_update_preset_from_draft_excludes_python_cache_cruft -q
  • python -m pytest tests/test_preset_routes.py tests/test_preset_download_routes.py -q

@github-actions github-actions Bot 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.

Strengths

  • Clean reuse of existing exclusion logic (preset_api_routes.py:66-73): _ignore_generated_script_cruft delegates to _should_skip_export_path, which already handles __pycache__, .pyc, .pyo, .swp, and other junk. No duplication of the exclusion list.
  • DRY application: The same ignore= argument is applied symmetrically to both the create (preset_api_routes.py:875) and update (preset_api_routes.py:1097) copy paths, so the fix is complete — not patched in one place and missed in the other.
  • Well-targeted tests: Both test_create_preset_from_draft_excludes_python_cache_cruft and test_update_preset_from_draft_excludes_python_cache_cruft (added in tests/test_preset_routes.py:435-489) exercise nested __pycache__/ directories, loose .pyc files at the root, and confirm that legitimate .py files are still copied. Good coverage of the real edge cases.
  • Minimal footprint: The PR touches exactly what's needed — no unrelated refactors or scope creep.

Issues

Critical (Must Fix)

None.


Important (Should Fix)

_ignore_generated_script_cruft calls os.path.isdir on a live filesystem mid-copy (preset_api_routes.py:70-71)

shutil.copytree calls the ignore callback before it begins descending into each directory, passing the directory path and the list of names inside it. At that moment the names are directory entries from the source tree, but the function here calls os.path.join(directory, name) and then os.path.isdir(path) — which is fine for the source path. However, _should_skip_export_path already detects __pycache__ and other excluded dirs by name alone (line 60: return name in EXPORT_EXCLUDED_DIRS), so the os.path.isdir call is only relevant for the pattern-match branch and for future additions. The current implementation is correct, but it could race if the source directory changes while copying (unlikely in practice for a web app, but worth a note). More importantly, a name that appears both as a file and a directory in different subdirectories will always get the is_dir flag wrong half the time — again, not a real problem for the current exclusion set, but fragile as exclusions grow.

Recommendation: Pass is_dir=False always for pattern matching (the patterns are filename-only), or if is_dir matters for future dir-pattern exclusions, document this assumption. The simpler fix:

def _ignore_generated_script_cruft(directory, names):
    ignored = []
    for name in names:
        full = os.path.join(directory, name)
        ignored_entry = _should_skip_export_path(name, is_dir=os.path.isdir(full))
        if ignored_entry:
            ignored.append(name)
    return ignored

This is what the code already does — the point is just that os.path.isdir is called on the source, which is correct, but it's worth a comment clarifying this is intentional and not accidentally checking the destination.

Test paths use relative configs/ path (tests/test_preset_routes.py:457-459)

preset_scripts = os.path.join('configs', 'presets', 'draft-clean-create', 'scripts')
assert os.path.exists(os.path.join(preset_scripts, 'keeper.py'))

The assertion uses a relative path (configs/presets/...) and relies on the CWD at test runtime matching the project root. The update test (test_update_preset_from_draft_excludes_python_cache_cruft) avoids this by using existing_scripts (derived from preset_path returned by _create_preset_folder). The create test should do the same — derive the expected path from app.config['PRESETS_BASE'] or from the response body, not from a hardcoded relative string. If a test runner changes CWD or the config changes, this assertion silently passes even when the copy didn't happen (because os.path.exists returns False for a non-existent path and the assert ... exists would correctly fail, but assert not os.path.exists(...) for the excluded files would also pass trivially — meaning the test could give a false green if the scripts dir was never created at all).

Fix: Build the expected path from app.config:

presets_base = app.config['PRESETS_BASE']
preset_scripts = os.path.join(presets_base, 'draft-clean-create', 'scripts')

Minor (Nice to Have)

Missing PR link in releases table (docs/user/releases.md:7)

The v1.13.9 row has in the PR column while every other row has a linked PR number. Once this PR is merged, the PR number should be filled in for consistency and traceability.

Docstring wording on _ignore_generated_script_cruft (preset_api_routes.py:67)

The docstring says "Return generated/editor junk to skip" — the function actually returns the list of names to ignore, not the junk items themselves. "Return names of generated/editor artifacts to exclude from shutil.copytree copies" would be clearer and matches the shutil.copytree ignore callback contract.


Assessment

Ready to merge? Yes, with minor fixes recommended.

Reasoning: The core change is correct — _ignore_generated_script_cruft properly reuses the existing exclusion logic and is applied consistently in both create and update paths. The one test path reliability issue is low-risk (the assertion would still fail if the bug were re-introduced), but should be tightened before the pattern spreads to other tests.

@dngrtech dngrtech merged commit 2f48720 into main Jul 7, 2026
@dngrtech dngrtech deleted the fix/preset-draft-script-cruft branch July 7, 2026 16:47
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