Skip to content

refactor(server): move DLC path resolution to lib/dlc_paths.py (R3 substrate) - #843

Merged
byrongamatos merged 1 commit into
mainfrom
refactor/r3-dlc-paths
Jul 10, 2026
Merged

refactor(server): move DLC path resolution to lib/dlc_paths.py (R3 substrate)#843
byrongamatos merged 1 commit into
mainfrom
refactor/r3-dlc-paths

Conversation

@byrongamatos

@byrongamatos byrongamatos commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

The keystone for the path-dependent routers (ws_highway @ 902 lines, song, audio-local-path, sloppak): the "where do the song files live + safe containment" layer leaves server.py so a router can reach it.

What moves

  • _resolve_dlc_path (pure — takes dlc + filename, no globals) → verbatim.
  • _get_dlc_dir reads the env-derived paths through the appstate seam (appstate.dlc_dir / dlc_dir_env / config_dir) instead of module globals, so lib/dlc_paths.py does no import-time IO.
  • appstate gains dlc_dir / dlc_dir_env slots (config_dir already there); server.py configures them. All three are env-derived, so a setenv+reimport fixture reconfigures them for free — zero setattr retargeting.

Zero test churn, via re-export

server.py re-exports both (from dlc_paths import _get_dlc_dir, _resolve_dlc_path), so its 24+16 call sites and the tests that reach server._get_dlc_dir() / server._resolve_dlc_path() directly (test_dlc_junction, test_highway_ws_notation, test_highway_ws_authors) resolve unchanged. No test edits.

server.py: 9,085 → 9,008.

Verification

  • _resolve_dlc_path byte-identical to origin/main; _get_dlc_dir's only change is the three path identifiers → appstate.*.
  • Route table identical (143); pyflakes clean; packaging guard 52 (picked up dlc_paths); pytest 2407 passed (the three direct-caller suites green through the re-export); npm run lint 0.
  • Boot smoke: library scans (8 songs — exercises _get_dlc_dir), a real song resolves and serves art (_resolve_dlc_path), and the highway WS reaches ready with notes.
  • Codex: 0 findings.

Next: ws_highway (902 lines) now needs only STATIC_DIR/SLOPPAK_CACHE_DIR in the seam (+ retargeting their ~5 setattr sites) and its few remaining helpers, then it can move.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved DLC library path handling across configuration and environment settings.
    • Added stronger validation to prevent invalid or unsafe DLC file paths.
    • Preserved support for valid library locations, including linked or mounted directories.
  • Documentation

    • Updated release notes with the latest router extraction and DLC path handling details.
    • Updated size-exemption documentation with the current server line count.

…bstrate)

The keystone for the path-dependent routers (ws_highway, song, audio-local-path,
sloppak): the "where do the song files live + safe containment" layer leaves
server.py so a router can reach it.

- `_resolve_dlc_path` (pure — args only) moves verbatim.
- `_get_dlc_dir` reads the env-derived paths through the appstate seam
  (appstate.dlc_dir/dlc_dir_env/config_dir) instead of module globals, so
  lib/dlc_paths.py does no import-time IO.
- appstate gains `dlc_dir`/`dlc_dir_env` slots (config_dir already there);
  server.py configures them. All three are env-derived, so a setenv+reimport
  fixture reconfigures them for free — ZERO setattr retargeting.
- server.py RE-EXPORTS both (`from dlc_paths import _get_dlc_dir,
  _resolve_dlc_path`), so its 24+16 call sites AND the tests that reach
  `server._get_dlc_dir()` / `server._resolve_dlc_path()` directly
  (test_dlc_junction, test_highway_ws_*) resolve unchanged — no test edits.

server.py: 9,085 -> 9,008.

Verified: _resolve_dlc_path byte-identical to origin/main; _get_dlc_dir's only
change is the three path identifiers -> appstate.*; pyflakes clean; route table
identical (143); pytest 2407 passed (test_dlc_junction + both highway_ws suites
green via re-export); packaging guard 52; eslint 0. Boot smoke: library scans
(8 songs, _get_dlc_dir), a real song resolves + serves art (_resolve_dlc_path),
and the highway WS reaches `ready` with notes.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

DLC directory selection and path containment validation moved from server.py into lib/dlc_paths.py. appstate now carries DLC configuration values, and server.py wires the extracted functions through that seam. Changelog and size-exemption documentation were updated.

Changes

DLC path resolution

Layer / File(s) Summary
DLC configuration and path validation
lib/appstate.py, lib/dlc_paths.py
Adds dlc_dir and dlc_dir_env appstate slots, resolves DLC directories from environment or configuration, and validates filenames against the DLC root.
Server integration and release metadata
server.py, CHANGELOG.md, docs/size-exemptions.md
Routes DLC values through appstate, imports the extracted resolvers, removes their duplicate server implementations, and updates related documentation.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Accurately describes the main change: moving DLC path resolution into lib/dlc_paths.py from server.py.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/r3-dlc-paths

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot 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.

🧹 Nitpick comments (1)
lib/dlc_paths.py (1)

26-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider logging the swallowed config-parse error.

The blind except Exception: pass (ruff S110/BLE001) silently discards a malformed config.json, so a user misconfiguration surfaces only as a mysterious "DLC folder not configured". A debug/warning log would make this diagnosable without changing the fallback behavior.

♻️ Log instead of silently passing
         if config_file.exists():
             try:
                 cfg = json.loads(config_file.read_text(encoding="utf-8"))
-            except Exception:
-                pass
+            except Exception:
+                logging.warning("Ignoring unreadable/invalid %s", config_file, exc_info=True)
🤖 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 `@lib/dlc_paths.py` around lines 26 - 39, Replace the silent `except Exception:
pass` in the config-loading logic with a warning or debug log that includes the
parsing exception and identifies `config.json`, while preserving the existing
fallback behavior of continuing with `cfg` unset.

Source: Linters/SAST tools

🤖 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.

Nitpick comments:
In `@lib/dlc_paths.py`:
- Around line 26-39: Replace the silent `except Exception: pass` in the
config-loading logic with a warning or debug log that includes the parsing
exception and identifies `config.json`, while preserving the existing fallback
behavior of continuing with `cfg` unset.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: e1593759-5ee7-43b9-a199-a61e9cfdb601

📥 Commits

Reviewing files that changed from the base of the PR and between 6da01c5 and 72c01c3.

📒 Files selected for processing (5)
  • CHANGELOG.md
  • docs/size-exemptions.md
  • lib/appstate.py
  • lib/dlc_paths.py
  • server.py

@byrongamatos
byrongamatos merged commit 0dcc913 into main Jul 10, 2026
5 checks passed
byrongamatos added a commit that referenced this pull request Jul 10, 2026
…#846)

Unblocked by the DLC-path substrate (#843): chart's only server-module deps are
now app + meta_db (both seam); _get_dlc_dir/_resolve_dlc_path come from dlc_paths,
sloppak/loose detection from the shared lib modules. 4 routes (split/unsplit/
work/fileinfo), meta_db-only otherwise. 0 setattr targets, 0 helpers to relocate.

Bodies verbatim; @app -> @router, meta_db -> appstate.meta_db. include_router at
the original site; 143-route table identical to origin/main. No test retargeting.

server.py: 8,003 -> 7,909.

Verified: pyflakes clean on the router; no new undefined/dead in server.py; route
table identical; pytest 2401 passed (74 across work_charts/context_menu/
group_filter/packaging); eslint 0. Boot smoke: chart/work 200, chart/fileinfo
resolves the real pack path through _resolve_dlc_path.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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