fix(manual-import): authorize companion files against the root folder, not the book folder by m4bard · Pull Request #832 · Listenarrs/Listenarr

m4bard · 2026-08-19T22:50:18Z

Summary

A manual import with includeCompanionFiles: true currently drops every companion file and still reports success. The companion pass hands the ownership store the book folder as its managed boundary, and the authorizer requires the boundary to be a configured root folder rather than to be inside one, so every companion is refused. This selects the boundary the same way the primary audio file's own import already does.

Full write-up, measurements and the alternative I did not take are in #831.

Changes

Fixed

ManualImportCompanionImporter.ImportAsync selects the managed boundary with LibraryDirectoryOwnershipPlanning.SelectMostSpecificBoundary over the configured root folders, falling back to destinationResolution.BoundaryPath, instead of passing destinationRoot. destinationRoot is DetermineScanPath over the batch's destination paths, so on a single-book import it is the book folder, which is never a root.
A companion whose destination yields no managed boundary is skipped with a warning rather than throwing, so a companion can still be lost but it cannot take the import with it.
ImportAsync takes a rootFolders argument, supplied at its single call site in ManualImportController where rootFolders is already in scope.

The authorizer is untouched and keeps its current strictness. The companion is now authorized against the same root folder that the audio file it sits beside is already authorized against.

Testing

ManualImportCompanionImporterTests gains ImportAsync_ManagedBoundaryIsTheConfiguredRootFolderRatherThanTheBookFolder, which captures the boundary handed to EnsureCreatedHierarchyAsync and asserts it is the configured root. With destinationRoot restored it fails on the boundary (library/Author/Book against the expected library); with the change it passes.

It asserts the argument rather than the outcome deliberately. The four existing companion tests all mock ILibraryDirectoryOwnershipStore and match the boundary with It.IsAny<string>(), so the real authorizer never runs and none of them can tell the book folder from the root. All four pass unchanged either way, which is a fair explanation for how this shipped. They gain the new argument and nothing else.

Full suite on this branch: 3,010 passed, 0 failed, 125 skipped. The skips are environment-gated and sit in areas this does not touch.

Reproduced end to end before and after against ghcr.io/listenarrs/listenarr:canary. On canary the audio arrives alone, with four boundary is not a configured root folder refusals and completed with 0 imported companion file(s). On a build of this branch the same run reports four imported, zero refusals, and the blacklisted decoy still filtered. The check is public, in the test-data repo linked from the issue.

Notes

The alternative was to relax LibraryDirectoryOwnershipBoundaryAuthorizer so a boundary inside a root is accepted. I left it out because it widens an authorization check, and because it would change the rename and download-import paths too, neither of which is failing. If you would rather converge that way, say so and I will redo it.

One thing the reproduction ruled out, since it is the natural first guess: where the source folder sits makes no difference. The same import with the source outside every configured root folder and again inside one gives an identical result, before and after.

… the book folder ManualImportCompanionImporter passed destinationRoot to EnsureCreatedHierarchyAsync as the managed boundary. destinationRoot is DetermineScanPath over the batch's destination paths, so for a single-book import it is the book folder. LibraryDirectoryOwnershipBoundaryAuthorizer matches the boundary against configured root folders by equivalence rather than containment, so it refused every companion with 'The requested directory boundary is not a configured root folder'. The exception was caught and logged as a warning, so the import reported success and silently dropped the companions. Select the boundary the way the primary audio file's import already does, via LibraryDirectoryOwnershipPlanning.SelectMostSpecificBoundary over the configured roots, falling back to the destination resolution's boundary path. The authorizer is unchanged and keeps its existing strictness; the companion is now authorized against the same root as the audio file it sits beside.

…p store The four existing companion tests all mock ILibraryDirectoryOwnershipStore with It.IsAny<string>() for the boundary, so the real authorizer never runs and nothing in the suite could tell the book folder from the root folder. All five pass unchanged either way, which is why this shipped. ImportAsync_ManagedBoundaryIsTheConfiguredRootFolderRatherThanTheBookFolder captures the boundary argument instead of the outcome and asserts it is the configured root. It fails with destinationRoot restored ("library/Author/Book" against the expected "library") and passes with the fix. The four existing call sites gain the new rootFolders argument.

…introduced Listenarrs#864 reshaped the companion pass. It now chooses between EnsureCreatedHierarchyAsync and EnsureAdditiveHierarchyAsync on the publication plan, and with no capability resolver injected a non-durable source takes the additive branch, which this test never stubbed. Capture the boundary from both so the assertion holds whichever runs. It also routes publication through IFileMover.PrepareActionForRegistrationDetailedAsync and a registration lease. Standing that up would mean asserting the mock graph rather than the boundary, which is the one thing this test exists to pin and which is captured before publication is attempted. The count assertion goes, with the reason recorded in place; ManualImportCompanionOwnershipTests still covers the end-to-end path. Verified by reintroducing only the buggy argument: the store then receives the book folder rather than the root folder, and this test fails on exactly that difference.

The companion pass takes the configured root folders so it can pick the same managed boundary the audio import picks. The test that covers a selected source outside the requested root predates that parameter and would not compile without it. It asserts the same thing as before: a companion whose source sits on another boundary is still mapped beside the audio destination that succeeded.

… missing Every other skip in the companion loop sets succeeded = false before it continues. The missing-boundary guard did not, so a skip there would have reported the pass as successful. Succeeded feeds companionPassSucceeded and then batchSucceeded, which is what decides whether the compatibility source cleanup may retire the sources, so reporting success on a skip is the wrong default. The guard is unreachable today, because PlanUniqueAsync runs earlier in the same iteration and throws when the resolution boundary is null or whitespace. That is why there is no test with it: a branch nothing can reach cannot be discriminated. The line is here so the guard is correct if it ever becomes reachable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…boundary The suite had the refusal case and the argument-pinning case, but nothing that watched a companion file arrive. This drives the importer with the real ownership store and a persisted root folder, so EnsureCreatedHierarchyAsync runs the boundary authorizer against the RootFolders table rather than a mock that accepts anything. That is what makes it discriminate. Put the old book-folder boundary back and AuthorizeAsync refuses it, the companion is skipped, and the assertions on ImportedCount and Succeeded both fail. Checked by reverting the selection to destinationRoot and running the filter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Archive extraction is on by default. It puts the contents of each archive under a fresh temporary directory and leaves the files the release shipped loose where the download client put them, then appends both to one batch. DownloadImportService relativized every companion against FileUtils.GetCommonDirectory of that batch, which is a directory neither half came from. On the stock container layout, with extraction under /tmp and downloads under /data, the only thing the two have in common is the filesystem root, and GetCommonDirectory returns "/". Path.GetRelativePath is a total function: it cannot report that a file has no meaningful position under the given root, and with "/" as the base it returns the candidate's own absolute path with the leading separator removed. That carries no "..", so the containment guard on the next line has nothing to reject, and the companion is written to audiobook.BasePath plus a de-rooted copy of wherever it came from. One import of one release leaves two directory trees inside the book folder, and nothing ever reclaims them. On a host where the temp directory and the download directory do share a parent the same thing happens one level down, so the bug is not confined to the layout that makes the common directory a bare root. The audio is unaffected throughout, because it is renamed by pattern and never reaches that line. The batch never had one source structure to reproduce, so stop looking for one. Each extraction directory is a root in its own right: the files under it came out of a single archive and their position relative to it is the structure that archive shipped. The files that stayed behind share the download directory. A companion is now mirrored under whichever of those it actually belongs to, so a release with an archive and loose sidecars lands exactly as the same release without an archive does. ArchiveImportExtractor already tracked the directories it extracted into, for cleanup. It now exposes them. When a companion belongs to no root, the fallback is the one ManualImportCompanionImporter.TryResolveCompanionDestination already uses on the manual import path: place it, by name, in the destination directory of an audio file imported out of its own directory. If there is no such file it is refused. Note that the prior art has no root check of its own, so it has the same hole on the manual path; this borrows its shape and adds the guard it lacks. Two deliberate choices worth stating. Refusing rather than flattening everything: a companion that genuinely sits one directory below its audio keeps that relationship, because its source root is a real directory whose structure means something. Flattening unconditionally would throw that away, and inventing a home for a file that arrived from an unrelated tree is how the stray trees appeared. Skipped rather than failed: DownloadProcessingJobProcessor fails the whole job on any unsuccessful ImportResult, which strands audio already copied into the library with no history row, no client-side import mark and no library scan. A companion with nowhere to go must not do that to an otherwise good import, so it is recorded as skipped. A companion whose resolved path is then rejected by the destination guard still fails, as it did before. FileUtils.IsFilesystemRoot is the bare-root backstop for a batch that spans disjoint trees with no archive to account for it. It decomposes the path exactly as GetCommonPathForDirectories does, so it agrees with the function whose output it classifies. Tests. Failing without this change: a release with an archive beside loose sidecars, which is the reported defect and needs no particular host layout; and a batch whose only common ancestor is the filesystem root, skipped where the machine cannot provide two such trees. Passing before and after, so they are controls rather than evidence: a batch under one source directory, which proves nothing was simply refused, and a companion one directory below the audio, which proves nothing was simply flattened. The destination guard is pinned separately as still rejecting a traversing path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The roots of a batch do not depend on which companion is being placed, so working them out inside the loop meant re-normalising every extraction directory and recomputing the common directory of the whole batch for each companion. On Windows the normalisation reaches TryResolveLongWindowsPath, which touches the filesystem. ImportCompanionDestinationResolver.ResolveRoots now returns them as one value, computed where the batch-wide common directory used to be computed. That assignment is the line this replaces: nothing read sourceRootPath after it once the companion branch stopped using it, so the count of lines in the file is unchanged and one redundant GetCommonDirectory call goes away with it. ResolveRoots takes the source semantics as nullable and, when they are unresolved, declines to ask whether a file sits under an extraction root rather than reaching for FileSystemPathSemantics.CurrentHostDefault, which is what BackendArchitectureTests.LegacyHostPathIdentity_StaysOnExplicitAllowList exists to prevent. A batch whose semantics are unresolved has no files to place. Two things the previous commit message should have said. Recording an unplaceable companion as Skipped rather than a failure also changes DownloadImportService.Coordination.cs:40-42, where batchSucceeded requires every result to have either succeeded or carry no source path. A failure with a source path held back the compatibility cleanup for the whole batch, so the sources of files that did import were retained; a skip lets the batch complete normally. The skipped companion's own source is untouched either way, because that cleanup is driven by the publication journal and a companion that was never published has no journal row. And the bare-root backstop is keyed to the symptom rather than the cause, so it only covers a batch that spans disjoint trees with no archive to account for it. If the un-extracted files themselves span two trees that do share a real ancestor, the shallow mirror survives. Reaching that needs one download item resolving to two local roots, which remote path mapping could do; the ordinary path supplies one download directory. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

… the caller's path The manual companion pass relativized each companion against the request's `path` and re-rooted that relative path under the destination. `path` is the folder the caller browsed to, and GET preview enumerates below it with SearchOption.AllDirectories, so it is routinely several directories above the files the user then selects. Every directory in between was recreated inside the book folder. Measured on a build carrying the companion pass: a request with `path` set to `/data` and the audio at `/data/src/Arthur Conan Doyle/2006 - The Valley of Fear/book.m4b` put all four companions at `<book>/src/Arthur Conan Doyle/2006 - The Valley of Fear/`, and created those three directories. The same request with `path` set to the file's own directory placed them beside the audio and created nothing. One field, opposite outcomes. The structure worth reproducing at the destination is the one the selected files have, not the one below an arbitrary browse location. There are no archives on this path, so the batch has exactly one source root and it is the selected files' common directory. That is at or below the request's path in every case, so this only ever narrows what gets mirrored, and it narrows it to nothing whenever the request's path was the files' own directory. The decision is `ImportCompanionDestinationResolver`'s, which the automatic download import already asks the same question. Widened to serve a second caller: the fallback that places a companion beside the file imported from its own directory now takes an `ImportedFilePlacement` pair rather than the automatic path's `ImportResult`, because the two paths carry their results in different types. The audio filter moved out to `ImportedAudioFrom`, which is the automatic path's projection, because that batch's results carry its companions too while the manual batch's carry only what the caller selected. Both callers reach one implementation on purpose: two copies of a containment rule is how a fix to one of them leaves the other broken. A bare filesystem root as the request path is the same defect in its worst form, and it is covered by the same change: the resolver refuses to mirror under a root that describes no structure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…nd nowhere else A review of the previous commit found two things, both right, and this replaces its rule rather than adjusting it. The first is a regression the previous commit introduced. Its argument rested on the selected files always being under the request's path, and that is not what the controller enforces: ManualImportController.ProcessItem.cs:48-64 accepts an item that is under the request path OR inside any configured root folder. So the selected files' common directory can sit above the request path or beside it, and using it as the source root could mirror a deeper tree than the code it replaced. A companion in a second, unrelated selected directory used to land flat via the fallback and would have gained two directories. The second is that the manual path has no source structure worth reproducing in the first place. ManualImportPathPlanner.GeneratePathAsync builds every audio destination from the naming pattern and the metadata; it never consults where the file sat in the source tree. Mirroring a companion's source position therefore hands the sidecar a shape the audio it accompanies has just lost, which is the same defect in a smaller font. Readarr and Sonarr both place extras in the imported file's own folder and reproduce nothing (src/NzbDrone.Core/Extras/Files/ExtraFileManager.cs, ImportFile, in both trees). So the manual path now declares no source roots at all. Every companion it sweeps up sits in the directory of a selected file, because that is where the sweep looks, and it is placed by name in the destination directory of the file imported out of that directory. If no such file was imported, it is refused, which is a change: a sidecar for a book file that never arrived used to be copied into the library anyway. The resolver still holds the decision, reached with CompanionSourceRoots.None. Two callers, one implementation, and the mirror branch is now exercised only by the automatic path, which does have archives and does have a structure to keep. Reachability, stated plainly because the previous commit message overstated it. The pass is off unless includeCompanionFiles is set, and that is set by exactly one caller, fe/src/stores/libraryImport.ts:541, which sends path = item.folderPath = the selected files' own directory (UnmatchedScanBackgroundService.cs:307-310, 328, 420), so old and new behaviour are identical there. The caller that does send a browse ancestor, fe/src/components/feedback/ManualImportModal.vue:731-735, never enables companions. The defect is reachable by an API client, and by any future UI that wires companions into that modal. Also from the review: the results projections are deferred so they run inside the resolver's own exception guard rather than in an argument list outside it, and only when the fallback is actually consulted. The doc comment claiming GetCommonPathForDirectories returns a bare root for disjoint trees is corrected; on Windows it returns null and GetCommonDirectory substitutes the first directory, so that check is a Unix backstop rather than a general one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…e rule the manual path uses A second review pass, and three corrections to the commit before this one. None change what the manual path does; two of them change what is claimed about it, which matters more than it sounds given the previous commit had to correct an overstatement of its own. The neighbour lookup compared directory strings. It now compares with FileSystemPathIdentity.AreEquivalent, which canonicalises both sides under the declared syntax. The two sides arrive from different places and nothing in the resolver makes them agree on spelling: a companion's directory descends from Path.GetFullPath, while an imported file's source path is whatever its caller put in the results, and ResolveNativeAbsolutePath hands back an already-qualified path untouched, so a `.` segment survives it. With a string comparison the resolver reported two spellings of one directory as different and refused the companion. Reachability, because the reviewer's measurement and mine disagreed and the difference is the whole point. Neither caller can reach it today: ManualImportItemDto.FullPath's setter rejects `..`, `./` and `.\` outright and then canonicalises what is left, so every manual source path in the results is already canonical before the pass runs, and the automatic path's come from its own file enumeration. The defect is in the resolver's public surface rather than in either caller, and the fix is kept on those terms. The test lives at the resolver, where the reachable API is, rather than at the importer, where the DTO would refuse the input. A malformed entry in the imported-file list is now skipped instead of abandoning the whole lookup; canonicalisation throws on a path that does not fit the declared syntax, and one bad entry should not cost the rest of the batch its companions. The manual path now calls TryResolveBesideImportedFile by name, and CompanionSourceRoots.None is gone. Asking for the rule you want reads better than handing the resolver an empty set of roots so that a branch you have just argued must never run declines to run. Same single implementation of the placement rule, reached by both callers. Corrections to the previous commit message, which a reader of this branch should have: - "never consults where the file sat in the source tree" was false. MultiFileImportPlanner.InferOrderHints reads the parent directory's name and folds a disc/disk/cd/part/volume/chapter match into the disk-number hint, which a {DiskNumber} pattern can turn into a destination subfolder. The argument survives, because what comes out is a pattern-shaped folder built from a number rather than a reproduction of the source tree, and a companion follows the audio into it. The absolute phrasing did not. - "old and new behaviour are identical there" needed the words "for a successful import". In the libraryImport.ts flow an orphan companion, one whose item failed, used to be copied into the library and is now refused. That change does reach that flow. One visible consequence worth stating rather than leaving to be discovered: two companions with the same name, from two source directories whose audio landed in the same destination directory, now collide where the old rule kept them apart in mirrored subdirectories. ManualImportDestinationTracker.PlanUniqueAsync gives the second one a uniquifying suffix rather than overwriting, so nothing is lost, but the names change. Readarr's extras collide the same way for the same reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…is actually true Two corrections from a final review pass, and one test made self-contained. No behaviour change: the four container cases come out byte-identical to the build before this one. The doc comment said the automatic path's source paths come from its own file enumeration. That is the fallback branch, not the main one. DownloadClientGateway.cs:254-264 takes the file list an adapter reported, passes each entry through RemotePathMappingService.TranslatePathAsync, and adds it; that method ends `return remotePath` when no mapping matches, which is the ordinary case on a single-host install. Only the no-file-list branch at :265-282 enumerates and calls NormalizeStoredPath. So those paths are client-reported strings. The conclusion is unchanged and the reason is better. Both callers are safe structurally rather than hygienically: each draws both sides of the comparison from one list of source paths, so whatever spelling that list uses appears on both sides and matches anyway. Divergence needs one batch to spell one directory two ways. That argument survives someone relaxing ManualImportItemDto's setter, which the previous message leaned on, and it is what the comment now says. Also corrected, from the same pass: the previous message put Listenarr's same-name collision handling beside Readarr's as though they matched. They collide for the same reason, and they resolve it differently. Readarr's ExtraFileManager.ImportFile passes overwrite: true to DiskTransferService.TransferFile, so the second extra replaces the first. Listenarr's ManualImportDestinationTracker.PlanUniqueAsync suffixes instead and keeps both. Listenarr is the safer of the two here. BesideImportedFile_MatchesTheNeighbourByPathIdentityNotSpelling asserted only the boolean, so it would have passed on a true with a wrong path. It now pins the relative path in both directions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…now requires The placement tests were written against the companion importer before this branch added the rootFolders argument, so the merge of the two lines of work compiles as a runtime image and does not compile as a test project. The image build only ever compiles production code, which is why every end-to-end measurement of the merged tree was made without this showing up. The root folder supplied is the book folder's parent, which is the shape all six cases already build, so the pass now selects its ownership boundary the same way it does in the controller rather than falling through to the resolution boundary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

m4bard requested a review from a team August 19, 2026 22:50

m4bard mentioned this pull request Aug 21, 2026

fix(qbittorrent): one unreadable torrent should not truncate the queue poll #830

Open

m4bard force-pushed the fix/bug12-companion-import-boundary branch from d225f38 to 2c60553 Compare August 24, 2026 16:36

m4bard mentioned this pull request Aug 24, 2026

Bug: manual import with includeCompanionFiles drops every companion file, and reports success #831

Open

krejko mentioned this pull request Aug 26, 2026

chore: sync with upstream canary and merge 25 open upstream fixes nexalapp/Listenarr#9

Merged

m4bard and others added 4 commits August 27, 2026 10:40

m4bard force-pushed the fix/bug12-companion-import-boundary branch from 2c60553 to e95ed5b Compare August 27, 2026 15:44

m4bard and others added 2 commits September 11, 2026 12:35

m4bard force-pushed the fix/bug12-companion-import-boundary branch from f87d8f1 to 9351af7 Compare September 15, 2026 18:35

m4bard and others added 7 commits September 21, 2026 10:17

Navigation Menu

Uh oh!

fix(manual-import): authorize companion files against the root folder, not the book folder - #832

fix(manual-import): authorize companion files against the root folder, not the book folder#832
m4bard wants to merge 13 commits into
Listenarrs:canaryfrom
m4bard:fix/bug12-companion-import-boundary

m4bard commented Aug 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

m4bard commented Aug 19, 2026

Summary

Changes

Fixed

Testing

Notes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant