Backend reorganization: api / services / database / tasks / utils (Phase 7) - #665
Open
nandyalu wants to merge 54 commits into
Open
Backend reorganization: api / services / database / tasks / utils (Phase 7)#665nandyalu wants to merge 54 commits into
nandyalu wants to merge 54 commits into
Conversation
Stage C applies the orwell-writing skill to backend log lines, comments and docstrings after the Stage A moves and the Stage B API thinning. Two constraints found while writing it: - 60 openapi operations take their description from the route handler docstring, so the sweep changes the spec. Invariant 1 becomes 'no structural change' with two permitted text-only diffs, each in its own commit. - Two tests match log substrings in test_download_attribution.py. The pre-flight audit records the baseline (1089 tests, openapi hash, 640 core imports, 263 patch targets) and clears two pitfalls: no alembic migration imports app modules, and the database package imports nothing outside itself. It also reconciles the move map with four packages it predated.
Stage A move 1 of the backend reorganization. The database package becomes a standalone top layer: core/base/database/models/ -> database/models/ core/base/database/manager/ -> database/manager/ core/base/database/utils/ -> database/ (engine, init_db, version_guard) Tests mirror the new tree under tests/database/. No behavior change. Two problems the move exposed, both fixed here: - version_guard anchored the alembic versions directory with parents[4], correct at the old depth and wrong at the new one. The guard reads that directory to learn which revisions exist, and it returns early when the set is empty, so a wrong path disables the downgrade protection without any error. Now parents[1]. Logged as hygiene H12 to make the empty case loud. - test_connection hardcoded connection ids 1 and 2. Every test shares one session database, so the ids depend on collection order, and the move changed that order. The tests now use the id that create() returns. Verified: 1089 tests pass, same as before the move. 'import main' clean. No core.base.database references remain.
Stage A move 2. Creates the services/ package. core/updates/ has one importer (tasks/schedules.py) and depends only on app_logger and settings, so this move is self-contained. Verified: 1089 tests pass.
Stage A move 3. Tests move to tests/services/notifications/. The move exposed a layering violation the pre-flight audit had missed: database/manager/event/create.py::_notify does a function-local import of the dispatcher. A wider re-audit found 13 imports from database/ into the business-logic layers, not the zero the first pass reported. The first pass used line-anchored patterns through the rtk grep proxy and undercounted; the plan now records the correct numbers, the audit method, and the Stage B work to make database/ actually standalone. Left in place with a TODO per the Stage A shim rule. Verified: 1089 tests pass.
Stage A move 4. The startup binary-path checks (yt-dlp, ffmpeg, ffprobe,
Deno) become a service. Tests move to tests/services/test_binaries.py.
This move rewrote 10 patch("core.binaries...") string targets. Import
rewrites now run as a Python pass over the tree rather than sed through the
grep proxy, so string targets inside test files are covered too.
Verified: 1089 tests pass.
Stage A move 5. Connection Doctor, the health checks, the cookies status and the report store become services. Tests move to tests/services/diagnostics/. This package arrived with the Onboarding track in v0.11.4, after the phase plan was written, so the move map did not cover it. The map now does. The rewrite also covered the PKG constants the tests patch through and the cross-test import in test_health.py. Verified: 1089 tests pass.
Stage A move 6a. These three helpers are used only by business logic, so they move straight to services/. path_utils is held back: the database layer imports it, so it needs a home both layers may use (6b). Verified: 1089 tests pass.
Stage A move 6b. backend/utils/ holds pure helpers that carry no dependencies on the other layers, so database/ and services/ may both import them. utils/ imports nothing from api, services, database or tasks. path_utils is the case that forced the decision: the database managers use is_subpath and normalize_trailing_slash, so sending it to services/ would have added three more database->services imports to fix later. Layering violations drop from 13 to 10. Moving tests/core/base/utils/test_filter_parity.py up two levels also broke its parents[3] anchor to tests/fixtures/filter-cases.json, which failed at collection and stopped the whole run. Now parents[1]. The plan records that tests carry these anchors too, and which ones are correct as they stand. Verified: 1089 tests pass.
…s/files Stage A move 7. core/files_handler.py and core/files/media_scanner.py join as services/files/. Tests move to tests/services/files/. Verified: 1089 tests pass.
Stage A move 8. Creates the services/connections/ package. Tests move to tests/services/connections/plex/. Verified: 1089 tests pass.
Stage A move 9. core/base/arr_manager/ becomes services/connections/arr/ (base.py, request_manager.py), and core/radarr/ and core/sonarr/ become subpackages under it. They keep their own packages because both define the same module names (api_manager, connection_manager, data_parser, database_manager, models). Tests move from tests/services/arr_manager/ to tests/services/connections/arr/. Verified: 1089 tests pass.
Stage A move 10. core/base/ is now empty and gone: its database package, utils and arr_manager all moved in earlier commits, and the shared BaseConnectionManager was the last file left. Verified: 1089 tests pass.
Stage A move 11, the largest one: core/download/ -> services/trailers/ core/download/image.py -> services/images/image.py core/download/error_classify -> utils/error_classify.py error_classify has no imports at all, and the database layer uses it, so it belongs with path_utils in utils/ rather than in services/. That removes the last of the shared-helper violations: database -> business-logic imports are now 9, and all nine are the connection-validation and notification calls that Stage B extracts. cli.py is vendored from yt-dlp and carries the 'allow direct execution' sys.path line, which resolves relative to its own location. Nothing runs it as a script (trailer_search imports it), and cli_to_api was called after the move to confirm it still works. Verified: 1089 tests pass.
Stage A move 12, the last one. core/tasks/ becomes the top-level tasks/ package and core/ is gone. The scheduler, the schedules and the task bodies all move together. The map sends the bodies of files_scan, download_attribution and image_refresh into services/, but that is an extraction, not a move, so Stage B does it. Stage A is complete. Backend layers are now api / services / database / tasks / utils. Verified: 1089 tests pass; no 'from core' or 'import core' remains anywhere; openapi.json is byte-identical to the pre-reorg spec (79 paths, 56 schemas), which is the proof these were pure moves.
CLAUDE.md now describes api/services/database/tasks/utils instead of core/, and states the layering rule with the nine remaining violations that Stage B removes, so future sessions do not add more. The phase plan gains a completion record with the evidence: byte-identical openapi, zero core imports, 27 migrations on a fresh database, and a real boot where both startup passes ran and every layer answered 200.
Stage B. The connection managers called Radarr, Sonarr and Plex over the
network from inside database/. That put network I/O behind a database call,
and update() made those calls inside an open write transaction, so a slow or
hanging server held a write lock for the length of an HTTP request.
Two new modules:
services/connections/probe.py validate_connection, get_rootfolders and
get_machine_identifier. Network only.
services/connections/service.py the create and update order of work:
probe first, then write the row.
The database managers now only persist. create() and update() are no longer
async, take the machine identifier the caller read, and never reach outward.
update() used to validate the merged entity inside the session. The service
now reads the stored row, merges the changed fields in memory, probes that,
and only then writes. A caller sees the same behavior — nothing is committed
when validation fails — but the transaction no longer spans a network call.
Tests follow the same split: persistence tests stay in
tests/database/manager/test_connection.py, validation moves to
tests/services/connections/test_probe.py, and new tests cover the ordering,
including that a failed probe writes no row and that update probes the NEW
url rather than the stored one.
database -> business-logic imports: 9 -> 1.
Verified: 1101 tests pass; openapi.json unchanged.
…mported Stage B, last layering violation. database/manager/event/create.py imported the notification dispatcher inside a function to dodge a cycle. The database layer now publishes to listeners, and the dispatcher registers one when it loads. The plan said to move the notify call up to the callers. That does not work here: the track_* helpers live in database/, and two database modules (connection/delete.py and media/create.py) call them, so "up" would have meant restructuring the media and connection managers. Inverting the dependency reaches the same goal and touches no call site. Registration happens when the dispatcher module loads, not in start(), which keeps the old behavior: events queue up before the loop runs and start() drains them. Two traps worth recording. Importing database.manager.event.create returns the create FUNCTION, because the package __init__ re-exports the name and shadows the submodule; subscribe is exported from the package instead. And wiring like this fails silently, so tests/services/notifications/ test_event_hook.py asserts the dispatcher is registered and that a listener which raises neither breaks event storage nor stops later listeners. tests/test_layering.py now guards all of it by parsing imports with ast: database/ imports nothing above it, utils/ imports no layer, and nothing imports the retired core/. Comments and function-local imports are counted correctly, which grep got wrong twice during this phase. database -> business-logic imports: 1 -> 0. Verified: 1109 tests pass; openapi.json unchanged.
Nine database -> business-logic imports are gone and a test guards the result. Notes the two traps the work turned up: the package __init__ that shadows the create submodule, and listener wiring that fails silently.
Stage B. api/v1/authentication.py had no route handlers at all: it was the
session store, bcrypt hashing and the credential checks, sitting in the api
layer. A service could not check a password without importing api/.
Split by what each part does:
services/auth.py sessions, hashing, set/verify username and
password, verify_api_key. No HTTP.
api/v1/authentication.py the FastAPI dependencies and verify_login,
which map those answers to 401s.
The names stay re-exported from api/v1/authentication.py, so existing
callers and their patch targets keep working.
Tests split the same way: tests/services/test_auth.py covers the logic and
patches services.auth, while tests/api/test_authentication.py keeps the
dependency tests. Both patch the module that actually resolves the name.
Verified: 1109 tests pass; openapi.json unchanged.
Stage B, router 1 of 7. The two update handlers held the whole decision tree: which keys are valid, which combination of new username and password was sent, and what message to send back. That moves to services/settings.py, and the handlers now just pass the request through. The logic had no tests, because reaching it meant going through HTTP. It has 13 now, including the case where 0 is a valid setting value: it is falsy, but only None and "" are refused. Both functions still answer with a message and a 200 rather than raising. That is the contract the frontend reads, so it stays until the frontend and the spec change together. Verified: 1122 tests pass; openapi.json unchanged.
Stage B, router 2 of 7. The log-file reading and the line parsing move to services/logs.py. The router keeps the FileResponse and the mapping to the Log response model. The service gives back plain dictionaries rather than Log objects. Log is a response model in api/v1/models.py, and a service that imported it to build one would invert the layering; the handler does Log(**record) instead. The parsing had no tests, because the only way in was an HTTP call to a deprecated endpoint. It has 10 now, covering the module prefix being lifted out of the message, job lines being filed under Tasks, and a line that matches nothing being kept whole instead of dropped. One test asserts every record carries exactly the keys the Log model needs, since the handler now unpacks them. Verified: 1132 tests pass; openapi.json unchanged.
Stage B, router 3 of 7. services/files/service.py takes the path and file-type guards, the byte-range read, and the rename and delete work that also updates the download rows. The handlers keep the checks that decide an HTTP status, which is what a handler is for, and now call a predicate instead of holding the rule. is_path_safe guards every path that arrives from a request, and it had no tests at all: the only tests that reached it patched it to True to get out of the way. It has them now — system folders, shallow paths, traversal with .., and real media paths. The rename and delete tests move to the service and no longer patch a guard the service does not call. One thing the new tests record rather than change: a relative path is resolved against the working directory, so the same string is safe or unsafe depending on where the process runs. Under Docker the working directory is /app, which is refused. That has always been true; Stage B only moved the function, so the test documents it instead of correcting it. notifications.py needed no work: every handler there is already a service call plus HTTP-error mapping, and it already follows the H1 pattern. Verified: 1170 tests pass; openapi.json unchanged.
Stage B, router 4 of 7. Three pieces of doctor orchestration move to
services/diagnostics/connection_doctor.py:
run_doctor_for_all the gather over every connection, and the
decision to log a failed check and leave that
connection out rather than fail the whole call
apply_mapping_and_recheck save a suggested mapping, then re-run
schedule_doctor the background run after a save, including the
strong reference set that stops the event loop
collecting a task while it runs
The CRUD handlers stay as they are. They were already the shape Stage B
asks for: a service call, a websocket broadcast, and the mapping to an HTTP
status.
_schedule_refresh stays in the api layer on purpose. It registers a job with
the scheduler, and a service reaching into tasks/ would invert the layering
the last two commits just established. The handler calls it after the
service returns a healthy report, which is what the old code did inside its
try block — the function swallows its own errors, so the move out of the try
changes nothing.
Verified: 1170 tests pass; openapi.json unchanged.
Stage B, router 5. services/media.py takes the work behind deleting a trailer, saving a YouTube id, and changing the monitor flag. Each function returns an ActionResult that says what happened and how to say it, so the handler still owns the websocket broadcast and the HTTP status. The three had no tests, because reaching them meant an HTTP call. They have seven now, covering the parts worth protecting: only files that still exist are deleted, one event is tracked per media item rather than per file, and re-saving the same YouTube id stores it without tracking a change. media.py is 739 -> 683 lines. Left for a later commit: batch_update_media still calls the delete_media_trailer handler in a loop, which is how it broadcasts one message per item. Routing that through the service would drop those messages, and zero behavior change comes first, so the handler-calling- handler stays until the batch path gets its own extraction. Verified: 1177 tests pass; openapi.json unchanged.
Five routers thinned, with a table of what moved where, the routers that needed no work, and the three rules the work turned up: a service must not build an API response model, validation stays in the handler, and measure statements-per-handler before rewriting anything.
Writing tests for the path guard during Stage B turned up more than the working-directory quirk noted earlier. The guard refuses every path on Windows. Its last check counts forward slashes, and on Windows os.path is ntpath, so normpath returns backslashes and the count is zero. Checked with ntpath: a real library path, a user profile path and even a forward-slash path with a drive letter all come back False. On the Windows direct install that shipped in v0.11.1 this means /files/video, /files/read, /files/video_info, /files/trim_video, /files/rename and /files/delete all answer 400. Browsing still works because /files/files_simple is not guarded at all, so the symptom is "I can see my folder but nothing works on it". Two smaller problems come with it. The prefix match is not path-aware, so a library under /variable/media or /usr-data/media is refused for starting with an unsafe string. And a relative path is judged by wherever the process is running; under Docker that is /app, which is refused by luck rather than by design. H13 records all of it with the suggested fix: an allowlist built from the connection root folders, using Path.resolve and Path.is_relative_to, which is path-aware and cross-platform, instead of a POSIX-only denylist and a slash-counting depth heuristic. The docstring says not to extend the list. Two test classes assert the behavior as it stands so the bug is visible in the suite rather than living only in a plan file. They must be inverted when H13 is fixed, not deleted. The Windows breakage is a live bug rather than a cleanup, so it wants a patch release instead of waiting for v0.12.0. Verified: 1184 tests pass.
Stage B. batch_update_media used to call the delete_media_trailer route handler in a loop, which is how a batch delete sends one message per item. That body is now a plain function, _delete_trailer_and_report, and both the endpoint and the batch action call it. A route handler calling another route handler hid where the work happened. The bulk monitor and unmonitor branches move to media_service.set_monitoring_bulk. Its docstring records why it tracks no event: the single-item path knows the value before the change, and a bulk update does not read each row first. A test holds that in place, so nobody "fixes" it into one query per item. The download branch keeps its profile-id check in the handler. That is request validation, which Stage B leaves with the handler. Behavior is unchanged, including the double message a failing batch delete sends — the inner "Error deleting trailer!" and then the outer "Error updating Media!". That reads like a wart, but it is what the code does today, and this stage does not change behavior. Verified: 1214 tests pass; openapi.json unchanged.
… (H1) Stage B, hygiene H1. Eighteen handlers caught every exception and answered 404 with str(e). That is the wrong status for a failure that is not a missing item, and it returned whatever the exception said — which can be a file path, an SQL statement with its bound parameters, or a URL with a token in it. api/v1/errors.py now holds the mapping. A missing item is a 404. An exception whose message is written for the user keeps it: ConnectionError, ConnectionTimeoutError, InvalidResponseError, ItemExistsError, FolderNotFoundError, FolderPathEmptyError and ValueError. Anything else is logged with its traceback and answered with a line that names only the action, such as "Read media failed". The user-facing exceptions matter for the connection endpoints in particular. "Connection refused" or "Invalid API key" is the answer the user needs from Test Connection and the Connection Doctor, not an internal detail, so those keep their message and their 400. An HTTPException a handler raised on purpose passes through untouched. `except Exception` catches HTTPException too, so without that check the 406 from update_yt_id would have become a 500. A test holds it. The frontend only prints the status and the detail, so a status that changes from 404 to 500 changes the text shown, not any behavior. The spec still documents 404 where a handler can now answer 500. That is the next commit, which is the permitted spec diff. Verified: 1227 tests pass; openapi.json unchanged by this commit.
The dedicated spec commit for hygiene H1, and one of the two text diffs the
phase plan permits.
The previous commit made eighteen handlers answer 500 for an unexpected
failure instead of 404 with the exception text. The spec still said 404, so
it now documents the 500 as well.
The diff is 20 entries, all additions:
18 a documented 500 on the handlers that changed
2 a documented 404 on /media/{id}/rescan_files and /files/files_simple
Those two routes carried no responses dict at all, yet both answered 404 for
a missing item long before this phase. Adding it makes the spec say what the
code has always done.
Nothing was removed and no existing entry changed: the path count and the
schema count are the same, and every diff is an ADDED response code.
Verified: 1227 tests pass; the spec diff contains only these 20 additions.
The H1 change passes logger=logger to errors.as_http_error, but api/v1/connections.py never had a module-level logger — it borrowed connection_doctor.logger for its two background helpers. Every failing connection request therefore raised NameError inside the except block, and the caller got a bare "Internal Server Error" with nothing in the log. The full suite passed with that in place. No test walks those error paths, so nothing caught it; driving the running app did. Testing a connection against a dead port now answers 400 with "Connection Refused while connecting to API." instead of a blank 500. tests/api/test_error_wiring.py stops it happening again. It parses each api/v1 module, finds the calls to as_http_error, and checks that every name they pass resolves — so it covers every handler rather than the few an integration test reaches. Confirmed it fails when the logger is removed again. Verified: 1234 tests pass; openapi.json unchanged by this commit.
Records what H1 changed, the two cases that must not be genericized (the connection probes' message is the answer, and a deliberate HTTPException must pass through), and the NameError the suite missed because no test walks a handler's error path.
Stage C groundwork. The logs page links a line to a media item when its
mediaid column is set, and that column had one source: the database handler
searched the message for the first [123] and used that number.
Two problems. The number had to be the media id and had to come first, so
Setting profile [7] on download [12] for media [42]
stored 7 and linked the line to whatever media has id 7. Three lines were
doing this. And it tied the wording of every log line to a parsing rule,
which is the opposite of what Stage C needs.
A caller could not pass the id instead, even though the handler looks for
one: ModuleLogger is a LoggerAdapter, and the default process() replaces
kwargs["extra"] with the adapter's own, which is None here. Passing
extra={"mediaid": 42} was thrown away without a word.
process() now keeps what the caller passed, and logger.media(id) builds it:
logger.info("Trailarr downloaded the trailer.", **logger.media(media.id))
The message needs no brackets at all. logger.media(None) adds nothing, so an
optional id needs no branch at the call site.
The message search stays as a fallback for lines that have not been
converted and for logs from libraries.
The three mis-linking lines now pass the id explicitly. Checked through the
database, not only in tests: a message with no brackets stored mediaid 4242,
while the old shape stored 7 for a line about media 4243.
Verified: 1243 tests pass.
413 log calls written over years disagree with each other: most have no end punctuation, 49 open with a gerund, 12 trail off in '...', 10 use '!'. Records the style Stage C applies, and the rule that matters most now that logger.media() exists: the media id goes in that call, never in the prose, because a bracketed number in the text is still read as the media id by the handler's fallback.
The first package of the Simplified Technical English sweep. Every log line
in tasks/ that a user reads is now a whole sentence, in the active voice,
that names Trailarr as the actor and ends with a period.
"Scheduling all background tasks!" -> "Trailarr schedules the background tasks."
"No connections found in the database" -> "There are no connections to refresh."
"API Refresh completed!" -> "Trailarr refreshed the data from every connection."
"Stop event set, terminating scan of media folders."
-> "Trailarr stopped the disk scan. A stop was requested."
Eleven lines that carried the media id in square brackets now pass it
through logger.media() and name the title instead:
"Found new trailer file: '{path}' for '{title}' [{id}]"
-> "Trailarr found a new trailer file for '{title}'. Path: '{path}'."
One of those needed care. Taking [{media.id}] out of the disk-unavailable
error would have dropped its link to the media item, because the bracket was
the only thing that set the mediaid column. A check across the package
caught it before the commit, and it now passes the id explicitly.
Read the result from a real boot rather than from the diff: the startup
sequence, the scheduler and the first refresh all read as plain sentences.
Verified: 1243 tests pass.
…e C)
The download flow, which is where a user looks when a trailer does not
appear. Every line at INFO or above is now a sentence that names the actor
and says what happened.
"Monitoring is disabled, skipping trailers download"
-> "Monitoring is off. Trailarr does not download any trailer."
"Media 'X' [12] skipped: storage backing the media folder is unreachable."
-> "Trailarr skips 'X'. It cannot reach the storage of the media folder."
"Video codec 'av1' not supported by NVIDIA hardware encoder, using CPU"
-> "The NVIDIA hardware encoder does not support the video codec 'av1'.
Trailarr uses the CPU."
Twenty lines moved the media id out of the text and into logger.media().
Two things in this package are load-bearing and were left exactly as they
are. "YT-DLP Output::" and "FFMPEG Output::" are markers: db_handler.py
matches them to move the tool output into the traceback column and replace
the message. Changing either would put a wall of ffmpeg output back on the
Logs page.
One rewrite was wrong before it was checked. The backoff line reads
next_eligible_at(attempt), which is a module function, and the rewrite had
turned it into attempt.next_eligible_at — an AttributeError that only fires
when a download is in backoff. A check across the package now compares every
{...} expression against the original and reports any that is new; it reads
zero for this package.
Verified: 1243 tests pass.
The rest of the packages: database/manager, api/v1, services/connections,
services/files, services/diagnostics, notifications, images, updates and the
loose service modules.
The 16 event helpers said "Failed to track trailer_downloaded event for
[42]: ..." and now say "Trailarr could not record the trailer downloaded
event: ...", with the id passed through logger.media().
Four more lines were linking to the wrong media item. A bracketed number is
read as the media id, and these bracketed a Plex section key, a connection
id and a channel id (twice). Seven such lines existed in total across the
codebase; all seven are now fixed.
Two mistakes this sweep made, both caught before the commit by comparing
every {...} expression against the original:
- ProbeStatus.FAIL, on the line that ends every Connection Doctor run.
ProbeStatus has OK, WARNING, ERROR and SKIPPED. It would have raised
AttributeError on every run.
- attempt.next_eligible_at instead of next_eligible_at(attempt), fixed in
the previous commit.
Both passed the full suite, because a log message only runs when its line
runs. tests/test_log_message_safety.py now reads the calls instead:
it checks that an enum member named in a log message exists, and that no
log message puts a number that is not a media id in square brackets. Both
were confirmed to fail when the bug is put back.
Verified: 1419 tests pass; a real boot logs no traceback.
The last of the log lines, plus the messages that are built into a variable
and then logged, broadcast to the web UI, or returned from an endpoint.
"Media 'X' [12] is now monitored" -> "Trailarr now monitors 'X'."
"3 Media are now monitored" -> "Trailarr set 3 media items to monitored."
"Failed to refresh item 4021: ..." -> "Trailarr could not refresh the Plex item 4021: ..."
Two sets of strings were deliberately left alone.
exceptions.py keeps "{model_name} with id {id} not found". It is the text
of ItemNotFoundError, which handlers return as the 404 detail and which
tests assert on, so it is part of the API rather than prose.
The yt-dlp and FFmpeg failure messages in video_v2.py keep their wording.
They are fed to classify_ytdlp_error, which matches text fragments such as
"please sign in" and "login required". Rewriting one could make it start
matching a signature it does not match today, and change which reason the
user is shown.
Verified: 1419 tests pass; openapi.json unchanged.
The second of the two text diffs the phase plan permits, and the last piece of Stage C. A route handler's docstring becomes the operation description in openapi.json, so this had to be its own commit with the spec regenerated in it. Most of the 60 descriptions already read well; the v0.11.4 work went through them. Three said what they did in a way that only made sense to someone who already knew: "Monitor media by ID." -> "Turn monitoring on or off for one media item." "Returns: str: Monitoring message." -> "A line that says what changed." "Returns: str: Updating YouTube ID message." -> "A line that says what changed." The Args and Returns blocks keep their shape. Stage C rewrites the prose inside a docstring, it does not restyle the format. The diff is 3 description strings. Nothing else moved: 79 paths and 56 schemas before and after, no additions, no removals. Verified: the structural comparison reports 0 non-description differences.
The media link was never decoupled from the wording, seven lines linked to the wrong title, passing the id explicitly did not work, and rewriting f-strings broke two lines that the suite could not catch. Also lists the strings that must not be reworded, with the consumer that reads each one.
.github/instructions/backend.instructions.md is what Copilot reads before it writes backend code, and every path in it still described core/. It would have sent an agent to core/base/database/models/ for a model and core/tasks/ for a task, neither of which exists. The folder trees, the import examples and the "how to add a resource, a model, a task" checklists now describe api / services / database / tasks / utils, and the test tree matches what is on disk. .github/planned_tasks.md gets the same treatment. Its hook-point paths now point at the real files, and the files it proposes for later work (services/tmdb/, services/connections/filesystem/) are named for the layout they will be born into rather than the one that is gone. docs/references/contributing.md needed nothing: it has no backend path references, which the plan had already recorded. No core/ or core. reference remains in either file.
The frontend light touch the plan asks for. helpers/ held every pipe and directive while shared/ held every shared component, so "where does shared code live" had two answers. Pipes are now shared/pipes/ and directives are shared/directives/, next to the shared components. media/pipes/ was already gone: hygiene H5 merged the duplicate displayTitle into helpers/ back in v0.11.0, so only half of the plan's consolidation was left to do. The move broke one import in the way a file move usually does. Going from src/app/helpers/ to src/app/shared/directives/ puts the file one level deeper, so copy-to-clipboard.directive.ts asking for '../services/ websocket.service' was suddenly looking inside shared/. It is ../../ now. Same shape as the parents[N] anchors that broke in Stage A. src/app/README.md is new: what each folder holds, what belongs in shared/, and the five rules that are easy to get wrong here — standalone components, Signals for state, components call services rather than the API, MD3 tokens for styling, and one folder per shared component. Verified: 135 frontend tests pass, the production build compiles, and the built bundle serves through scripts/launch.py with every page answering 200.
The graphify re-index surfaced these: docstrings and comments that still named core/ paths, mostly at the top of a test file saying which module it covers. "Tests for core/tasks/files_scan.py" now reads "Tests for tasks/files_scan.py", and the same for the test-tree paths that one test file uses to point at another. Thirteen files. No code changed. Verified: 1419 tests pass. No core/ path remains in any Python file.
…stake The remaining log lines that carried the media id in square brackets now pass it through logger.media(). That is every one of them: no log message in the backend puts an id in brackets any more. One of those rewrites was wrong, and it is the third of its kind. It read media.title inside process_image, where media is a MediaImage — a dataclass with id, is_poster, image_url, image_path and headers, and no title. Image refresh runs over the whole library every six hours, so every image would have raised AttributeError. The full suite passed with it in place, as it did for ProbeStatus.FAIL and attempt.next_eligible_at. Three times is a pattern, so the guard now covers it. tests/test_log_message_safety.py reads each log call, looks at which argument the message reaches into, and checks the annotated type really has that field. Confirmed it fails when media.title is put back. Verified: 1594 tests pass; the Docker image builds; a copy of the real 1,700-title library boots with no traceback and its startup passes run.
Every check the plan asks for, with its result, and the three bugs that a green suite could not catch: a missing module logger, two names that do not exist, and a field on the wrong type. Each has a test now.
130 of 153 backend modules had no docstring. After a reorg that moved almost every file, a reader landing in services/trailers/trailer_file.py or services/connections/plex/connection_manager.py had nothing telling them what they were looking at. This adds 42 of them, to tasks/ and services/ — the packages a reader reaches for first. Each says what the module does, and where the behavior is surprising, why: - files_handler: every function checks the storage answers first, because a disconnected drive looks like an empty folder and acting on that deletes rows for trailers that are still there. - connections/base: media that leaves an Arr server but is still in Plex is demoted, not deleted. - plex/connection_manager: a show folder is derived from its episode folders, and a folder at or above a library root is refused because it would claim every title under it. - schedules: the first-run delays are staggered so a restart does not run everything at once. - startup_passes: the download run waits for them, or it downloads trailers the attribution pass has not claimed yet. api/v1, database/ and config/ still have none. They are next. Verified: 1594 tests pass; openapi.json unchanged, since none of these modules holds a route.
31 more module docstrings, in the packages a reader reaches for after services/ and tasks/. Three of them carry a warning that is easy to trip over: - config/logs/db_handler.py explains the mediaid column: a caller sets it with logger.media(id), and the fallback reads the first bracketed number in the message. It says plainly what that means — a profile or channel id in brackets links the line to the wrong title. - database/engine.py describes what the session decorators do, including that a write session commits on return and rolls back on an exception. - config/settings.py records that a variable set for the container wins over the value stored in .env. A module docstring on a router does not reach openapi.json: the spec description comes from main.py, and the tags carry none. Checked by regenerating rather than assuming. Backend modules with a docstring: 23 before this phase, 96 now. Verified: 1594 tests pass; openapi.json unchanged.
The last 57. The CRUD modules under database/manager/ and the models get a line each; the eight loose modules get a paragraph where there is something worth knowing: - app_logger explains logger.media(), which is how a log line links to a title now that the message no longer carries the id. - exceptions says which exceptions carry a message meant for the user, and points at api/v1/errors.py for the rest. - frontend/router records that it finds frontend-build/ from its own location, so the file cannot move. - services/trailers/cli says it is copied from yt-dlp's devscripts and should only change to follow that file upstream. - export_openapi says to set APP_VERSION, or the spec records 0.0.0. Backend modules with a docstring: 23 before this phase, 153 of 153 now. Verified: 1594 tests pass; openapi.json unchanged.
Three stages complete, the verification protocol run, and the branch ready for the v0.12.0 PR.
Seven new items, and a sharper H4.
H4 said "update all tests to current codebase", which is not actionable.
What is actually wrong: the suite builds one database for the whole session,
so a test that assumes an autoincrement id depends on which test ran first.
Stage A reshuffled collection order and broke test_connection.py, which had
hardcoded id 1. That was one instance; Phases 8 to 10 all add tables and
tests, and the next instance will look like an unrelated failure.
H14 the bracketed-id fallback in the log handler, now that every line passes
the id explicitly. Kept for third-party logs; revisit in a release or two.
H15 a failing batch delete broadcasts two error messages. Stage B kept it
because that stage does not change behavior.
H16 api/v1/media.py is 691 lines. The read handlers still build their own
filters. Do it when a later phase touches the router anyway.
H17 a register of strings that are contract rather than prose: the
ItemNotFoundError message, the yt-dlp text that feeds the error
classifier, and the two output markers the log handler matches. Phase 7
had to rediscover all three; the next prose pass should not have to.
H18 commented-out manage_session in database/engine.py, under "Remove in
v0.8.0!". We are at v0.12.0 and nothing references it.
H19 a service cannot schedule a task. Fine while only a handler needs it;
invert it the way the event listeners were inverted when Phase 8 needs
a service to schedule the TMDB refresh.
H20 the 144 DEBUG log lines keep their old wording. Stage C spent its effort
on the lines a user reads.
The hygiene backlog is for work to schedule. These two are rules for work that has not started, so they go where someone looks first. CLAUDE.md gains the log message conventions: a whole sentence naming Trailarr, the id passed with logger.media() rather than written into the text, and the reason a number that is not a media id must never appear in square brackets — the handler's fallback reads the first one as the media id and links the line to the wrong title. It also states the thing that made Phase 7 expensive: a log message is only evaluated when its line runs, so an f-string naming something that does not exist passes every test and raises in production, on the error path where the message mattered most. plans/README.md gains a section before the cross-phase invariants, because that file is read before every phase. Three bugs went past a green suite in Phase 7, all with the same shape: a line that only runs when something goes wrong is invisible to a passing test run. It names the three, points at the tests that read the code rather than running it, and says to boot the app and drive the pages a change touches.
Rebasing this branch onto the new dev dropped its one merge commit, which is where the v0.11.4 path-guard fix had been ported into services/files/service.py. Git drops merge commits by default, and their conflict resolutions go with them. Without this commit the branch reverted a fix that has already shipped: services/files/service.py was back to counting '/' characters, so a Windows install could not play, read, rename or delete a trailer, and tests/services/files/test_path_safety.py was gone. Restored from what is on dev, unchanged: PurePath component comparison, Windows system folders matched by name under the drive, a component count instead of a slash count, and relative paths refused. Also removed again the two test classes that recorded the old broken behavior, and the copy of the path-safety tests at tests/api/ — the rebase brought all three back, and the file now lives with the code it covers. Checked rather than assumed: services/files/service.py and test_service_guards.py are byte-identical to their pre-rebase state, and a direct call confirms a Windows media path is allowed, a Windows system path is refused, /variable/media is allowed and /var/log is not. Verified: 1594 tests pass; openapi.json unchanged; module docstrings still 153 of 153.
Contributor
|
🚀 Docs preview for this PR: View Documentation |
Contributor
☂️ Code Coverage
Overall Coverage
New Files
Modified Files
|
Brings the v0.11.5 release into the reorg branch. Rename detection moved most of dev's work onto the new paths on its own; the eager load in database/manager/media/read.py merged with no help at all. Resolved by hand: - services/trailers/trailers/missing.py — dev rewrote this file for the sweep-based scan, and Phase 7 moved it and rewrote its log messages. Kept dev's implementation, which is what shipped, and applied Phase 7's conventions to it: the new imports, the STE100 wording, and logger.media(id) in place of the five bracketed ids that dev's new code introduced. The guard in test_log_message_safety.py allows a bracketed media id, so nothing would have failed; H14 states that every backend line uses logger.media(id), and that claim has to stay true. - The two test files, same split: dev's content, Phase 7's paths. - test_missing_trailer_sweeps.py is new on dev. Git placed it under tests/services/trailers/; its imports needed rewriting. - CLAUDE.md — dropped the note added on dev that says logger.media() does not exist. It exists here, which is the whole point of the branch. - plans/hygiene-backlog.md — kept both sides: Phase 7's H14 to H20 and dev's H21. Dropped H21's note about the numbering gap, and pointed its paths at the new layout. - The OpenAPI spec is generated, so it was regenerated rather than merged. Verified: 1622 backend tests pass, 135 frontend tests pass, the frontend builds, and no `core.` import survives anywhere.
Merge commit, not squash and not rebase: the 53 commits carry the Stage A/B/C structure, and nothing else in the repository records it. The rebase button refuses this PR because the v0.11.5 merge gave the branch a merge commit. That is expected. Releases v0.9.2, v0.9.3, v0.9.4 and v0.10.2 all landed as two-parent merges, so the recent linear history is a preference and not a rule. Records the two rejected alternatives with their reasons, so neither is argued again at merge time.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 7 of the roadmap. No feature, no fix, no schema change: the backend moves from
core/into layers, the API handlers hand their work to services, and the prose that describes it all gets rewritten. Ships as v0.12.0.Plan and verification:
plans/phase-07-backend-reorg.md.The layout
core/is gone.utils/is new:path_utilsanderror_classifyhave no dependencies and bothdatabase/andservices/use them, so sending either throughservices/would have inverted the layering.Stage A — the moves
13 commits, one per package, each a
git mvplus a mechanical import rewrite.openapi.jsonis byte-identical across the whole stage, which is the proof they were pure moves: the entire API surface regenerated to the same bytes from relocated code.Two bugs surfaced, both fixed in the commit that caused them:
version_guard.pyfound the migrations folder withparents[4], correct at the old depth. It reads that folder to learn which revisions exist and returns early when the set is empty, so the wrong path turned off the protection with no error. Only its unit test noticed.Stage B — thinning the API, and the layering
database/imported business logic in 9 places. The connection managers called Radarr, Sonarr and Plex over the network from inside the database layer, andupdate()made those calls inside an open write transaction — a hanging server held a write lock for the length of an HTTP request. Probing now happens before the write.The event manager imported the notification dispatcher. It publishes to registered listeners instead, and the dispatcher subscribes on import.
tests/test_layering.pyenforces the result by parsing imports withast, so it cannot regress.Five routers thinned;
api/v1went from 2946 to 2589 lines. Seven routers needed no work — measuring statements per handler first saved rewriting them.Hygiene H1: eighteen handlers caught every exception and answered 404 with
str(e), which is the wrong status and returns whatever the exception said — a path, an SQL statement with its parameters, a URL with a token.api/v1/errors.pymaps them now. The exceptions whose message is written for the user keep it: "Connection refused" is the answer Test Connection exists to give.Stage C — the prose
Every log message rewritten to one house style: a whole sentence, active voice, naming Trailarr, ending in a period.
The media link had to be fixed first. The Logs page links a line to a title from the
mediaidcolumn, and that column was filled by searching the message for the first[123]. Seven lines bracketed something else — a profile, a download, a Plex section key, a channel — and each linked to whatever media holds that number. Passing the id was impossible:ModuleLoggeris aLoggerAdapter, and the defaultprocess()discardedextra={"mediaid": ...}without a word.logger.media(id)works now, and a message needs no brackets at all.Every backend module has a docstring: 23 before, 153 of 153 now.
Verification
openapi.jsondatabase/intoapi,servicesortasksscripts/launch.pyThree bugs a green suite could not catch
Worth reading before the next phase, because the shape repeats: a line that only runs when something goes wrong is invisible to a passing test run.
connections.pyhad no module-level logger, so every failing connection request answered a bare 500 and logged nothing.ProbeStatus.FAIL, which does not exist, on the line that ends every Connection Doctor run.media.titleon aMediaImage, which has no title, in the image refresh that runs over the whole library every six hours.All three passed the full suite. Each was found by driving the running app or by diffing the rewrite against the original, and each now has a test that reads the code rather than running it.
Not in this PR
plans/hygiene-backlog.mdgained seven items for what Phase 7 found and deliberately left: the bracketed-id fallback that can now retire, a batch delete that reports twice, the strings that are contract rather than prose, and four more.Review notes