feat(library): seed bundled starter content into the library on first run - #743
Conversation
… run Ship a public-domain Für Elise (keys) feedpak as starter content so a fresh install isn't an empty library. server._seed_builtin_starter_content() copies bundled packs into DLC_DIR/starter/ exactly once, guarded by a marker in CONFIG_DIR — unlike the always-reseeding diagnostic seed, a user who deletes the starter song does not get it back. `starter/` is deliberately outside the diagnostics/tutorials library carve-out so the song surfaces as a normal library entry. Extract the shared symlink-safe, mtime-aware copy loop into _copy_builtin_packs() and route both the diagnostic and starter seeds through it (diagnostic behavior unchanged; existing tests green). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughRefactors bundled diagnostics-pack seeding in server.py into shared symlink-safe, mtime-aware copy helpers (_copy_builtin_packs, _write_builtin_pack), then adds one-time starter-content seeding into DLC_DIR/starter/ guarded by a marker file, wired into background scan. A new test suite validates starter seeding behavior. ChangesBuiltin Pack Seeding Refactor
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant BackgroundScan
participant SeedStarter as _seed_builtin_starter_content
participant Marker as CONFIG_DIR marker
participant DLC as DLC_DIR/starter
participant WritePack as _write_builtin_pack
BackgroundScan->>SeedStarter: call with dlc dir
SeedStarter->>Marker: check marker exists
alt marker missing
SeedStarter->>DLC: check destination state (lstat)
alt destination safe and missing
SeedStarter->>WritePack: copy bundled source atomically
WritePack->>DLC: os.replace() temp file onto destination
SeedStarter->>Marker: write marker if all sources present
else destination unsafe or existing
SeedStarter->>SeedStarter: skip, leave marker unwritten
end
else marker present
SeedStarter->>SeedStarter: skip seeding
end
BackgroundScan->>BackgroundScan: continue library scan
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR seeds bundled starter content into the library on first run by adding a one-time “starter content” seeding path alongside the existing diagnostic seed, with an emphasis on symlink-safe, atomic writes and mtime-aware behavior.
Changes:
- Refactors bundled-pack seeding into a shared
_copy_builtin_packs()helper and introduces_write_builtin_pack()for atomic temp+replace writes (with a POSIXdir_fdhardening path). - Adds one-time starter content seeding (
_seed_builtin_starter_content) guarded by a marker file inCONFIG_DIR, and wires it into_background_scan(). - Adds a new pytest suite covering starter seed semantics (seed-once, deletion stays gone, no overwrite, symlink/non-regular refusal, mtime preservation, marker behavior).
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
server.py |
Adds shared bundled-pack copy/write helpers, introduces one-time starter seeding with marker guard, and calls it during background scan. |
tests/test_builtin_starter_seed.py |
Adds automated tests validating the starter content seeding contract and safety behaviors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if dest_dir.is_symlink(): | ||
| log.warning("%s: %s is a symlink, skipping all seeding", label, dest_dir.name) | ||
| return 0 | ||
| dest_dir.mkdir(parents=True, exist_ok=True) |
| # Preserve the bundle mtime (copyfileobj doesn't) so the mtime-based | ||
| # refresh check matches the shutil.copy2 fallback path. Best-effort. | ||
| try: | ||
| shutil.copy2(source, dest) | ||
| log.info( | ||
| "Builtin diagnostic seed: %s %s -> %s/%s", | ||
| action, | ||
| source.name, | ||
| _BUILTIN_DIAGNOSTIC_SUBDIR, | ||
| os.utime( | ||
| dest_name, | ||
| ns=(src_stat.st_atime_ns, src_stat.st_mtime_ns), | ||
| dir_fd=dir_fd, | ||
| follow_symlinks=False, | ||
| ) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_builtin_starter_seed.py (2)
66-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest doesn't exercise the actual carve-out predicate.
This test only asserts a hardcoded literal set (
{"diagnostics-builtin", "tutorials-builtin"}), never calling the real_is_excluded_from_libraryclosure from_background_scan(). If that predicate's set changes (e.g.,"starter"accidentally gets added, or a rename introduces a mismatch), this test won't catch it — it will still pass because it never touches production code.Since the predicate is a local closure, consider either extracting it to module scope for direct testability, or at minimum seeding a starter file and asserting via
_background_scan()'s library listing that it appears (integration-style check) rather than duplicating the excluded set as a literal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_builtin_starter_seed.py` around lines 66 - 69, The current test is only checking a hardcoded exclusion literal and never exercises the real library carve-out logic in _background_scan() or its _is_excluded_from_library predicate. Update the test to validate behavior through production code by either extracting _is_excluded_from_library to module scope for direct unit testing, or by seeding a starter file and asserting it appears in the library listing returned by _background_scan(), so the test fails if the actual excluded set changes.
112-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo coverage for a per-file destination symlink (only the seed-directory symlink case is tested).
_copy_builtin_packsalso refuses to write when the destination file itself is a symlink (dest_islinkbranch), which is a distinct code path from the symlinked seed-directory case already tested at Line 112. Consider adding a test that pre-createsdestas a symlink (e.g., pointing outsidedlc) and asserts it is left untouched and the marker stays unwritten.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_builtin_starter_seed.py` around lines 112 - 166, Add coverage for the per-file symlink path in _copy_builtin_packs, since only the symlinked seed-directory case is tested now. Create a test that pre-creates the destination returned by _dest(...) as a symlink pointing outside the DLC tree, then call _seed_builtin_starter_content(dlc) and assert the symlink target is left untouched and the _STARTER_SEED_MARKER is not written. Use the existing server_mod helpers and keep the new test aligned with the current symlink/refusal behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_builtin_starter_seed.py`:
- Around line 131-148: The test test_seed_never_overwrites_an_existing_user_file
is missing the same source existence guard used by the sibling starter-seed
tests. Add a source.is_file() check with pytest.skip(...) before calling
_seed_builtin_starter_content so this case skips cleanly when the bundled pack
is absent, and keep the existing assertions on dest and
CONFIG_DIR/_STARTER_SEED_MARKER only for the present-source path.
---
Nitpick comments:
In `@tests/test_builtin_starter_seed.py`:
- Around line 66-69: The current test is only checking a hardcoded exclusion
literal and never exercises the real library carve-out logic in
_background_scan() or its _is_excluded_from_library predicate. Update the test
to validate behavior through production code by either extracting
_is_excluded_from_library to module scope for direct unit testing, or by seeding
a starter file and asserting it appears in the library listing returned by
_background_scan(), so the test fails if the actual excluded set changes.
- Around line 112-166: Add coverage for the per-file symlink path in
_copy_builtin_packs, since only the symlinked seed-directory case is tested now.
Create a test that pre-creates the destination returned by _dest(...) as a
symlink pointing outside the DLC tree, then call
_seed_builtin_starter_content(dlc) and assert the symlink target is left
untouched and the _STARTER_SEED_MARKER is not written. Use the existing
server_mod helpers and keep the new test aligned with the current
symlink/refusal behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 03bd6b19-ded4-46ff-a322-56e15bcb4180
📒 Files selected for processing (3)
content/starter/beethoven-fur_elise.feedpakserver.pytests/test_builtin_starter_seed.py
| def test_seed_never_overwrites_an_existing_user_file(tmp_path, server_mod): | ||
| """One-time starter seeding must never replace a user's own file at the | ||
| destination, even if the bundled pack has a newer mtime.""" | ||
| import os as _os | ||
|
|
||
| dlc = tmp_path / "dlc" | ||
| dlc.mkdir() | ||
| dest = _dest(server_mod, dlc) | ||
| dest.parent.mkdir(parents=True, exist_ok=True) | ||
| dest.write_bytes(b"user's own edited pack") | ||
| _os.utime(dest, (1_000_000, 1_000_000)) # far older than the bundled source | ||
|
|
||
| server_mod._seed_builtin_starter_content(dlc) | ||
|
|
||
| assert dest.read_bytes() == b"user's own edited pack" # untouched | ||
| # counted as already-present, so the one-time seed considers itself done | ||
| assert (server_mod.CONFIG_DIR / server_mod._STARTER_SEED_MARKER).is_file() | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Missing skip guard for absent bundled source, unlike sibling tests.
Every other test in this file guards on source.is_file() and calls pytest.skip(...) when the bundled feedpak isn't present in the checkout (e.g. lines 41-42, 58-59, 78-79). This test omits that guard.
Per _copy_builtin_packs's contract (source snippet), the source.is_file() check runs before the dest_exists/update_existing branch — if the source is missing, the pack is never counted as present, so the marker won't be written here either, and the assertion at Line 147 would fail with a confusing message instead of skipping like its siblings.
🔧 Proposed fix
dlc = tmp_path / "dlc"
dlc.mkdir()
+ source = _source(server_mod)
+ if not source.is_file():
+ pytest.skip(f"starter source not present in checkout: {source}")
dest = _dest(server_mod, dlc)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_seed_never_overwrites_an_existing_user_file(tmp_path, server_mod): | |
| """One-time starter seeding must never replace a user's own file at the | |
| destination, even if the bundled pack has a newer mtime.""" | |
| import os as _os | |
| dlc = tmp_path / "dlc" | |
| dlc.mkdir() | |
| dest = _dest(server_mod, dlc) | |
| dest.parent.mkdir(parents=True, exist_ok=True) | |
| dest.write_bytes(b"user's own edited pack") | |
| _os.utime(dest, (1_000_000, 1_000_000)) # far older than the bundled source | |
| server_mod._seed_builtin_starter_content(dlc) | |
| assert dest.read_bytes() == b"user's own edited pack" # untouched | |
| # counted as already-present, so the one-time seed considers itself done | |
| assert (server_mod.CONFIG_DIR / server_mod._STARTER_SEED_MARKER).is_file() | |
| def test_seed_never_overwrites_an_existing_user_file(tmp_path, server_mod): | |
| """One-time starter seeding must never replace a user's own file at the | |
| destination, even if the bundled pack has a newer mtime.""" | |
| import os as _os | |
| dlc = tmp_path / "dlc" | |
| dlc.mkdir() | |
| source = _source(server_mod) | |
| if not source.is_file(): | |
| pytest.skip(f"starter source not present in checkout: {source}") | |
| dest = _dest(server_mod, dlc) | |
| dest.parent.mkdir(parents=True, exist_ok=True) | |
| dest.write_bytes(b"user's own edited pack") | |
| _os.utime(dest, (1_000_000, 1_000_000)) # far older than the bundled source | |
| server_mod._seed_builtin_starter_content(dlc) | |
| assert dest.read_bytes() == b"user's own edited pack" # untouched | |
| # counted as already-present, so the one-time seed considers itself done | |
| assert (server_mod.CONFIG_DIR / server_mod._STARTER_SEED_MARKER).is_file() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_builtin_starter_seed.py` around lines 131 - 148, The test
test_seed_never_overwrites_an_existing_user_file is missing the same source
existence guard used by the sibling starter-seed tests. Add a source.is_file()
check with pytest.skip(...) before calling _seed_builtin_starter_content so this
case skips cleanly when the bundled pack is absent, and keep the existing
assertions on dest and CONFIG_DIR/_STARTER_SEED_MARKER only for the
present-source path.
What
A fresh feedBack install lands on an empty library. This seeds a bundled, public-domain Für Elise (keys) feedpak as starter content so the library isn't empty on first run.
server._seed_builtin_starter_content()copies bundled packs intoDLC_DIR/starter/exactly once, guarded by a marker inCONFIG_DIR. Unlike the always-reseeding diagnostic seed, this is a one-time welcome — if the user deletes the starter song it stays gone.starter/is deliberately outside the diagnostics/tutorials library carve-out, so the song surfaces as an ordinary library entry.How
_copy_builtin_packs()and route both the diagnostic and starter seeds through it. Anupdate_existingflag keeps the diagnostic's refresh-on-newer-bundle behavior while starter content never overwrites a user's own file. Diagnostic behavior is unchanged (existing tests green)._write_builtin_pack()writes atomically (temp +os.replace). On POSIX the seed directory is pinned by anO_NOFOLLOWdir fd and every stat/create/replace goes through it, closing parent- and final-name symlink TOCTOUs; a path-based fallback covers platforms withoutdir_fd.O_CREAT|O_EXCL|O_NOFOLLOW(never written through a symlink) and read as an lstat sentinel.content/starter/beethoven-fur_elise.feedpak(public-domain, DMCA-clean; keys arrangement).tests/test_builtin_starter_seed.pycovers seed-once, delete-stays-gone, deferral until a DLC is configured, never-overwrite-user-file, non-regular/symlink refusal, mtime preservation, and marker-not-written-on-incomplete-seed.Desktop bundling of
content/starter/(and a diagnostic-name skew fix) ships in a companion feedBack-desktop PR.Codex preflight: clean (NO ISSUES).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes