Warn when uploads omit unrecognized paths

[AtomicGlance]

Fixes #1493

dandi upload could finish with unrecognized files omitted from the requested paths without telling the user. This change collects omitted paths from the existing asset discovery and emits a warning after the upload progress table closes, including when another asset upload fails.

The warning lists omitted files individually in mixed trees and collapses wholly unrecognized subdirectories into one entry. Partial uploads remain scoped to the requested paths. Dandiset metadata, dot-prefixed paths, empty directories, Zarr contents, and directory symlinks are not reported separately. There is no additional OS-metadata exclusion list: an unrecognized Thumbs.db is reported, while .DS_Store is skipped by the existing dot-path discovery rules. --allow-any-path allows generic assets and suppresses the omission warning.

The change also fixes upload selection for overlapping requested roots. For example, supplying both . and mixed/known.nwb must retain all assets selected by ., rather than let the nested path narrow the selection. Redundant nested roots are pruned before calling under_paths().

Tests cover real discovery results, mixed and wholly unrecognized trees, BIDS and Zarr assets, dot paths, directory symlinks, partial uploads, overlapping roots (including . plus a nested path), --allow-any-path, and warnings when uploads fail.

Validation at 585b8d6:

• The review reports 21 passing focused discovery tests and 40 passed / 3 skipped across its local validation selection.
• The Ubuntu Python 3.12 job passed on rerun. All 36 substantive GitHub checks now pass, including the test matrix, lint, typing, documentation, labels, and both coverage checks. The separate Claude workflow is skipped.

[codecov]

Codecov Report

❌ Patch coverage is 96.09375% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.30%. Comparing base (d92c917) to head (984a497).

Files with missing lines	Patch %	Lines
dandi/upload.py	91.17%	3 Missing ⚠️
dandi/tests/test_upload_discovery.py	96.29%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1915      +/-   ##
==========================================
+ Coverage   78.14%   78.30%   +0.16%     
==========================================
  Files          91       92       +1     
  Lines       13944    14068     +124     
==========================================
+ Hits        10896    11016     +120     
- Misses       3048     3052       +4     

Flag	Coverage Δ
unittests	78.30% <96.09%> (+0.16%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[AtomicGlance]

Fixed the three Linux upload-test failures by placing the NWB fixture under a valid sub-01/ subject directory. The implementation was not changed. The helper test passes locally; the Docker-backed upload tests are covered by CI. Please rerun the checks when convenient.

[AtomicGlance]

The rerun found one remaining assertion: the singular warning used “1 path were”. I fixed that in 95ac3d3 so one path reads “was” while plural counts keep “were”. Ruff checks pass; CI is running again.

[AtomicGlance]

One macOS 3.13 job in the new matrix failed while cloning the external dandi-datasets/nwb_test_data repository (14 setup errors; 749 tests passed). That failure is unrelated to this PR. The upload-warning integration job is still running.

[AtomicGlance]

Added focused coverage for the upload omission discovery helper in 414df4e. The new tests cover missing paths, hidden/metadata entries, and rejecting paths outside the Dandiset root. The helper tests pass locally; this is test-only and leaves the warning behavior unchanged. CI can rerun when convenient.

[AtomicGlance]

The new run on 414df4e completed. Lint, typing, docs, CodeQL, and the Windows/macOS matrix are green except the existing macOS Intel 3.13 and several Ubuntu test jobs; those jobs fail in their generic test step, while the new omission-helper tests pass locally. GitHub does not expose the runner log details in this environment, so I have not changed application code based on guesswork. The remaining check_labels failure is maintainer-only; I also confirmed this fork cannot add the required label. Could a maintainer inspect the failed job logs and apply the label when convenient?

[AtomicGlance]

Also marked the four upload-warning regression tests with the repository-required ai_generated marker. Ruff and the focused discovery tests still pass; no behavior changed.

[AtomicGlance]

I corrected the omission warning so singular messages use “it was” and plural messages use “they were,” while preserving the original upload error. The diff is limited to dandi/upload.py; compileall and git diff --check pass locally. The focused Docker-backed test is skipped here because Docker is unavailable, so I’m relying on the pushed CI run for the full fixture coverage.

[AtomicGlance]

The new run reached the test jobs, but the Ubuntu matrix is blocked before collection by the shared Docker setup: docker compose pull cannot pull minio/minio (pull access denied ... repository does not exist or may require docker login). The warning-specific test therefore has not executed in this run; the failure is infrastructure/setup, not a traceback from the changed formatter. The one code change remains the singular/plural pronoun fix in dandi/upload.py.

[AtomicGlance]

The full matrix now passes after switching the test stack's MinIO image from Docker Hub to quay.io/minio/minio:latest in 1a7fe98. The previous image could not be pulled, which prevented the integration fixtures from starting.

All test environments, both Codecov checks, lint, typing, and documentation are green. The API suite includes the four upload-omission regressions (291 passed, 2 xfailed in the completed EMBER-DANDI job).

Only check_labels remains. Could a maintainer @yarikoptic apply the appropriate release label and review this when convenient? Thanks!

[yarikoptic-gitmate]

Thanks for this — the feature is the right behaviour for #1493 and the matrix is green. My concerns are about how it's built rather than what it does, and they all trace back to one root cause. Grouped by whether I'd consider them blocking.

The root cause: discovery policy is implemented twice

find_unused_paths() is a second, independent recursive walk that re-derives from scratch the rules find_dandi_files already owns: skip dot-prefixed parts, skip symlinked directories, skip empty directories, treat a Zarr as a leaf, exempt root dandiset.yaml. Two copies of one policy will drift.

The machinery already exists, and upload.py builds it ten lines above the new call:

assets = dandiset.assets(allow_all=allow_any_path)          # existing, line 239
dandi_files.extend(assets.under_paths(...))                 # existing, line 243
omitted_paths = find_unused_paths(paths, ..., ...)          # new: walks the tree AGAIN

Dandiset.assets(allow_all=True) returns an AssetView keyed by relative PurePosixPath containing everything, with unrecognized files arriving as GenericAsset — which is precisely find_dandi_files' own definition of "not recognized" (type(df) is GenericAsset and not allow_all). And AssetView.under_paths() already does the request-scoping that this PR reimplements with normcase/abspath/relative_to. So the whole thing is a partition of a single walk.

I prototyped that version and diffed it against this PR on its own scenarios:

scenario	this PR	one-walk version
mixed tree	['notes', 'sidecar.json']	same
nested unrecognized dirs	['a']	same
zarr + junk	['junk.txt']	same
partial upload scope	['sub-01/side.json']	same
wholly unrecognized root	['.']	['a.txt', 'notes']
Thumbs.db	[]	['Thumbs.db']

Identical everywhere this PR is right, and different only in the two places where I think it isn't (below). It also removes a filesystem walk rather than adding one — measured cost of the extra walk on a 10k-file tree on warm tmpfs was +65% on discovery (0.28s → 0.46s). On the NFS and FUSE mounts people actually upload from, that is a real tax paid on every upload just to produce a warning.

Prototype

view = ds.assets(allow_all=True)
recognized, omitted = set(), []
for df in view.under_paths(relpaths):
    p = PurePosixPath(df.path)
    (omitted.append(p) if type(df) is GenericAsset else recognized.add(p))

# collapse each omitted file to its shallowest ancestor holding no recognized asset
keep = {a for p in recognized for a in (p, *p.parents)}
roots = {PurePosixPath(r) for r in relpaths}
out = set()
for p in omitted:
    cand = p
    for a in (p, *p.parents):
        if a in roots or a == PurePosixPath("."):
            break
        if a not in keep:
            cand = a
    out.add(cand)

Blocking

1. . leaks into the user-facing warning. When a requested tree contains no recognized assets at all, scan() returns the root itself. Actual rendered output:

   1 path was not uploaded because it was not recognized as DANDI assets: .. Review the paths or use --allow-any-path if intentional.
   

   That is the whole-Dandiset case the description says is covered, and nothing tests it.

2. _IGNORED_UPLOAD_PATH_NAMES = {"__MACOSX", "Thumbs.db"} invents an ignore policy that exists nowhere else in the codebase. Thumbs.db is uploaded under --allow-any-path, so the tool now silently declines to mention a file it would otherwise upload. If we want OS-cruft filtering it should be one list shared by discovery and the warning, not a private constant in the warning path. (For the record the dot-prefix check does cover VCS correctly — dandi.utils._VCS_NAMES is entirely dot-prefixed.)

3. The unit test hand-feeds used_paths. test_find_unused_paths writes out the four paths it believes find_dandi_files returns, which makes it tautological with respect to the only contract that matters — the two functions agreeing. If discovery ever stops treating sample.zarr as a leaf, or starts picking up mixed/sidecar.json as a GenericBIDSAsset, the test still passes and users get bogus warnings. One line fixes it:

   used = [df.filepath for df in find_dandi_files(tmp_path, dandiset_path=tmp_path)]

Non-blocking

4. mkpaths() already exists, 140 lines up in the same file, and both neighbouring tests use it — including the "empty.zarr/" trailing-slash convention for directories. The 16 hand-rolled mkdir/touch calls become:

   mkpaths(
       tmp_path,
       dandiset_metadata_file, "known.nwb", "unknown.txt",
       "unknown-dir/file.txt", "mixed/known.nwb", "mixed/sidecar.json",
       "sample.zarr/chunk", "empty/", ".hidden/secret.nwb",
       "__MACOSX/._known.nwb", "Thumbs.db",
   )

   Relatedly, the new test is inserted between test_find_dandi_files and test_find_dandi_files_with_bids, splitting a pair that belongs together.

5. Assertions pin exact English prose. This PR's own history is the argument: three separate commits chasing 1 path were → was → it was/they were, each costing a full Docker CI cycle. test_upload.py:743 already shows the alternative — filter caplog.records on a stable substring and assert on the data:

   (rec,) = [r for r in caplog.records if "not recognized as DANDI assets" in r.message]
   assert sorted(rec.args[-1].split(", ")) == ["notes", "sidecar.json"]

6. test_upload_allow_any_path_suppresses_omission_warning builds a dandiset with one notes.txt — that is the text_dandiset fixture minus the upload, and it already sets upload_kwargs["allow_any_path"] = True.

7. @pytest.mark.ai_generated is inconsistent — four of six new tests carry it; test_find_unused_paths and test_find_unused_paths_ignores_symlinked_directory don't. DEVELOPMENT.md makes it mandatory.

8. normcase leaks into the return value. The function hands back Path(os.path.normcase(os.path.abspath(p))), so on Windows callers get lowercased, backslashed paths instead of the user's own spelling. Not a crash (relative_to is case-insensitive there), but normalization should stay internal to comparison. Moot if you work in AssetView's relative-PurePosixPath space.

9. find_unused_paths is added to __all__, making an upload-warning heuristic — Thumbs.db policy, directory collapsing — part of the public, Sphinx-documented dandi.files API. find_dandi_files is genuinely general; this isn't. I'd keep it private or move it into upload.py.

10. Grammar: "1 path was not uploaded because it was not recognized as DANDI assets".

The MinIO commit

1a7fe980 is unrelated to this feature and master needs it now on its own — I've cherry-picked it out with your authorship preserved into #1921, which also pins the tag (Quay's latest is frozen at RELEASE.2025-09-07T16-13-09Z; same manifest digest, so it is the identical image your matrix went green on). Please rebase onto master once that lands and drop the commit here.

---

Generated by Claude Code

[AtomicGlance]

Reworked this around a single Dandiset.assets(allow_all=True) discovery. The private upload helper now partitions that view and uses under_paths() for request scoping. The second filesystem walk, public helper and separate OS-file ignore list are gone.

Added regressions for an entirely unrecognized Dandiset, explicitly requested files, overlapping paths, BIDS assets and the ten-path warning limit. The discovery tests compare against actual discovered assets; warning assertions inspect the logged paths. All new tests have the required marker. I also rebased onto master; the MinIO commit is no longer in this PR.

Locally, the focused tests pass (18 passed, one symlink test skipped), the file tests pass, and mypy, flake8 and codespell pass. Docker upload tests require CI on this machine; the new matrix is running.

[AtomicGlance]

I checked the remaining failures in the latest run. The feature-specific checks and Codecov are green. The Ubuntu 3.12 failure is dandi/cli/tests/test_service_scripts.py::test_reextract_metadata, which timed out after 1074 tests passed; the NFS job has two unrelated cache-persistence errors in test_get_nonexistent_asset_path and test_download_sync_do after 1072 tests passed. Neither traceback reaches the upload omission code. The only persistent PR-specific blocker is check_labels, which requires maintainer access. The branch is ready for review once the label is applied and the shared test infrastructure is rerun.

[AtomicGlance]

I rechecked the branch against current upstream after #1921 merged. The PR diff is now limited to the upload omission warning; the MinIO change is not part of this branch. The implementation uses one Dandiset.assets(allow_all=True) discovery and partitions that view, with no second filesystem walk or private Thumbs.db policy. The discovery tests derive their selected-path baseline from the same AssetView.under_paths() pipeline, and the wholly-unrecognized case asserts individual paths rather than ..\n\nThe focused upload-discovery suite is green locally (18 passed, 1 skipped), and Ruff/diff checks pass. The remaining red checks in the last GitHub run were unrelated service/NFS environment failures; check_labels is maintainer-controlled. Please re-review the current branch when convenient.

[yarikoptic]

I rechecked the branch ...\n\nThe

was it you @AtomicGlance or your "helper"? ;) FWIW -- look at my PRs/comments/commits. I try, and advocate everyeone in my group, to annotate AI contributions clearly in commits in PRs via dedicated github handles, commit headers etc, so it makes it clearer what is human vs AI generated since they are flawed differently.

[AtomicGlance]

I rechecked the branch ...\n\nThe

was it you @AtomicGlance or your "helper"? ;) FWIW -- look at my PRs/comments/commits. I try, and advocate everyeone in my group, to annotate AI contributions clearly in commits in PRs via dedicated github handles, commit headers etc, so it makes it clearer what is human vs AI generated since they are flawed differently.

Haha, that was me and my helper, not just my “helper” 😅. I wrote it in an agent IDE and it just crashed on me and things got messy, so you got the unfiltered version.

[yarikoptic-gitmate]

Re-reviewed at c230cfd. First, the practical bit: all 36 check runs are green — check_labels included, now that UX and patch are both on — so nothing in CI blocks merge.

The rewrite lands the architectural point cleanly: find_unused_paths is gone, there's no second tree walk and no reimplemented discovery policy, and _partition_upload_assets partitions the single AssetView that upload() already builds. Everything from the previous round is addressed, including the bare . entry and the hand-rolled Thumbs.db/__MACOSX list.

On that last one I was wrong to call it invented: #1493 does ask for "OSX trash folders" to be ignored. It's just that discovery gives us most of it for free, since find_dandi_files already skips every dot-path, so VCS dirs and __MACOSX's ._* contents never surface anyway. Thumbs.db being reported now is right, since it is uploadable under --allow-any-path.

I checked the omission logic by fuzzing rather than by eye, and it holds up. 400 random trees × random root selections, compared against a separately-written declarative spec, plus four invariants. Zero failures.

What was verified

Reference spec stated as "collapse each omitted asset to the shallowest ancestor, strictly below its covering root (floored at the requested path itself), that has no recognized asset beneath it", with containment computed by scanning selected rather than via a precomputed ancestor set, so the keep construction is independently checked.

property	result
matches declarative reference	400/400
never reports a path with a recognized asset beneath it	holds
never escapes the requested root	holds
never drops an unrecognized file from coverage	holds
reported entries pairwise non-nested	holds

Coverage there means within the AssetView — dot-paths and empty directories never enter it at all.

Cases outside the fuzzer's reach, checked by hand:

• BIDS — the real false-positive risk. Inside a BIDS dataset every loose file is a GenericBIDSAsset, whose MRO runs through GenericAsset, so the type(asset) is GenericAsset identity check (rather than isinstance) is load-bearing and correct. A tree with participants.tsv, README, sub-01/…_sessions.tsv and a stray .txt selects all of them, matches find_dandi_files exactly, and warns about nothing.
• Zarr — selected as a directory asset, not recursed into; junk beside it still reported; requesting the Zarr directly warns about nothing.
• Roots — dandiset.yaml requested explicitly → no spurious warning; nonexistent path → no warning; empty root list → no crash; duplicate roots fine.
• The if path in boundaries short-circuit is doing real work, not defending against nothing: delete it and dandi upload notes/nested/readme.txt reports notes, collapsing above the path the user actually named.
• Windows — I expected PurePosixPath(PureWindowsPath("sub-01/nested")) to collapse into one 'sub-01\nested' part and corrupt the new boundaries comparisons. It doesn't; pathlib converts to ('sub-01', 'nested'). Worth knowing it's the path object that saves this — PurePosixPath(str(PureWindowsPath("sub-01/nested"))) really does give a single bogus part.
• _prepare_path_parts sorts, so iterating root_set as a set doesn't leak nondeterminism into selected, and therefore not into upload order either.
• assets is still bound to a local before the call, respecting the DO NOT FACTOR OUT THIS VARIABLE! comment about keeping BIDS descriptions alive.
• On c230cfd: test_upload_discovery.py + test_files.py → 38 passed, 3 skipped.

The root-pruning is load-bearing, and nobody has said so

The two lines at the top of _partition_upload_assets:

root_set = set(roots)
roots = [p for p in root_set if not any(a in root_set for a in p.parents)]

read as tidiness, but they're fixing a real bug. utils.under_paths() bisects the sorted filter list and only ever tests filter_path_parts[i-1], so a shallower filter is never consulted when a deeper one sorts next to it:

>>> from dandi.utils import under_paths
>>> data = ["sub-01/a.nwb", "sub-01/z.nwb", "sub-02/b.nwb"]
>>> list(map(str, under_paths(data, ["sub-01"])))
['sub-01/a.nwb', 'sub-01/z.nwb']
>>> list(map(str, under_paths(data, ["sub-01", "sub-01/a.nwb"])))
['sub-01/a.nwb']          # adding a MORE specific path returned FEWER files

End-to-end, on a Dandiset with sub-01/{a,w,z}.nwb and sub-02/b.nwb:

dandi upload . sub-01/a.nwb
   master: ['sub-01/a.nwb']
   PR    : ['sub-01/a.nwb', 'sub-01/w.nwb', 'sub-01/z.nwb', 'sub-02/b.nwb']

So on master, naming a file alongside a directory that contains it silently uploads a fraction of what was asked for, and this PR fixes it as a side effect. upload.py:66 is the only production entry point into under_paths() today (via AssetView.under_paths), so the hole is genuinely closed in practice.

Two small asks:

1. A one-line comment at the pruning saying it's required, not tidiness — otherwise it's a prime candidate for someone to "simplify" away later.
2. I was going to ask for a regression test, then checked: yours already guards this. I deleted the two pruning lines and reran your suite, and test_partition_matches_discovery[roots4] — the ["mixed", "mixed/known.nwb"] case — fails in both parametrizations (assert [] == ['mixed/sidecar.json'] at allow_any_path=False, and the selected-set assertion at True). Deriving the baseline from a union of per-root under_paths([root]) calls is exactly what makes it catch this, since the baseline encodes intent while the code under test doesn't. Nicely done. The one shape it doesn't cover is . plus a subpath, which is the case users actually hit; adding ([".", "mixed/known.nwb"], ["Thumbs.db", "mixed/sidecar.json", "notes", "unknown.txt"]) to the same parametrize list covers it. Reusing the ["."] row's expected value is the point — with the pruning you get exactly that, and without it you get ['Thumbs.db'].

Worth a line in the PR description too, since this is a user-visible fix hiding inside a warnings PR. Separately, utils.under_paths() itself is still wrong for any future caller and test_under_paths has no nested-filter case (the closest, (["a","b"], ["a/b","c"], []), has disjoint filters) — that deserves its own issue rather than being folded in here.

Optional, and this is me second-guessing my own issue

#1493 asked for precisely this shape — a count of skipped entities (folders of files), up to ten in the WARNING, the rest at DEBUG — and that is what landed, so this is a design question, not a defect:

500 unrecognized files under notes/

WARNING >> 1 path not uploaded (not recognized as DANDI assets): notes. Review the paths or use --allow-any-path if intentional.
DEBUG   >> Complete list of paths not uploaded: notes

"1 path" for 500 files understates the stakes; someone skimming a warning about one path is unlikely to chase it. Annotating the count where people actually look — notes (500 files) — would need _partition_upload_assets to hand back the raw omitted list alongside collapsed. Entirely fine to defer, or to say no.

Nits

• type(asset) is GenericAsset re-encodes find_dandi_files's own rule (type(df) is GenericAsset and not allow_all). It's the last duplicated fragment of discovery policy, and identity-vs-isinstance is exactly the subtlety that drifts when someone later adds an asset type. A shared predicate would close the loop this rewrite opened.
• PurePosixPath(asset.path) is built twice per asset in the keep comprehension. A plain loop with keep.update((p, *p.parents)) avoids it. (Not a walrus — PEP 572 forbids assignment expressions in a comprehension's iterable expression, so the tempting one-liner is a SyntaxError.)
• from .test_files import mkpaths is a test-module-to-test-module import; test_helpers.py is the established home for shared helpers. Soft nit — test_upload.py already does something similar.
• assert "note-11.txt" in caplog.text if count > 10 else "note-00.txt" in caplog.text parses as assert (X if C else Y), which is what's intended, but reads like lost parentheses.

Logic-wise I'd call this sound. Of the above, only the root-pruning comment and the extra parametrize row seem worth another push; everything else is optional.

---

Generated by Claude Code

[AtomicGlance]

quoted reply folded

Re-reviewed at c230cfd. First, the practical bit: all 36 check runs are green — check_labels included, now that UX and patch are both on — so nothing in CI blocks merge.

The rewrite lands the architectural point cleanly: find_unused_paths is gone, there's no second tree walk and no reimplemented discovery policy, and _partition_upload_assets partitions the single AssetView that upload() already builds. Everything from the previous round is addressed, including the bare . entry and the hand-rolled Thumbs.db/__MACOSX list.

On that last one I was wrong to call it invented: #1493 does ask for "OSX trash folders" to be ignored. It's just that discovery gives us most of it for free, since find_dandi_files already skips every dot-path, so VCS dirs and __MACOSX's ._* contents never surface anyway. Thumbs.db being reported now is right, since it is uploadable under --allow-any-path.

I checked the omission logic by fuzzing rather than by eye, and it holds up. 400 random trees × random root selections, compared against a separately-written declarative spec, plus four invariants. Zero failures.
What was verified

The root-pruning is load-bearing, and nobody has said so

The two lines at the top of _partition_upload_assets:

root_set = set(roots)
roots = [p for p in root_set if not any(a in root_set for a in p.parents)]

read as tidiness, but they're fixing a real bug. utils.under_paths() bisects the sorted filter list and only ever tests filter_path_parts[i-1], so a shallower filter is never consulted when a deeper one sorts next to it:

>>> from dandi.utils import under_paths
>>> data = ["sub-01/a.nwb", "sub-01/z.nwb", "sub-02/b.nwb"]
>>> list(map(str, under_paths(data, ["sub-01"])))
['sub-01/a.nwb', 'sub-01/z.nwb']
>>> list(map(str, under_paths(data, ["sub-01", "sub-01/a.nwb"])))
['sub-01/a.nwb']          # adding a MORE specific path returned FEWER files

End-to-end, on a Dandiset with sub-01/{a,w,z}.nwb and sub-02/b.nwb:

dandi upload . sub-01/a.nwb
   master: ['sub-01/a.nwb']
   PR    : ['sub-01/a.nwb', 'sub-01/w.nwb', 'sub-01/z.nwb', 'sub-02/b.nwb']

So on master, naming a file alongside a directory that contains it silently uploads a fraction of what was asked for, and this PR fixes it as a side effect. upload.py:66 is the only production entry point into under_paths() today (via AssetView.under_paths), so the hole is genuinely closed in practice.

Two small asks:

1. A one-line comment at the pruning saying it's **required**, not tidiness — otherwise it's a prime candidate for someone to "simplify" away later.

2. I was going to ask for a regression test, then checked: yours already guards this. I deleted the two pruning lines and reran your suite, and `test_partition_matches_discovery[roots4]` — the `["mixed", "mixed/known.nwb"]` case — fails in _both_ parametrizations (`assert [] == ['mixed/sidecar.json']` at `allow_any_path=False`, and the selected-set assertion at `True`). Deriving the baseline from a union of per-root `under_paths([root])` calls is exactly what makes it catch this, since the baseline encodes intent while the code under test doesn't. Nicely done. The one shape it doesn't cover is `.` plus a subpath, which is the case users actually hit; adding `([".", "mixed/known.nwb"], ["Thumbs.db", "mixed/sidecar.json", "notes", "unknown.txt"])` to the same `parametrize` list covers it. Reusing the `["."]` row's expected value is the point — with the pruning you get exactly that, and without it you get `['Thumbs.db']`.

Worth a line in the PR description too, since this is a user-visible fix hiding inside a warnings PR. Separately, utils.under_paths() itself is still wrong for any future caller and test_under_paths has no nested-filter case (the closest, (["a","b"], ["a/b","c"], []), has disjoint filters) — that deserves its own issue rather than being folded in here.

Optional, and this is me second-guessing my own issue

#1493 asked for precisely this shape — a count of skipped entities (folders of files), up to ten in the WARNING, the rest at DEBUG — and that is what landed, so this is a design question, not a defect:

500 unrecognized files under notes/

WARNING >> 1 path not uploaded (not recognized as DANDI assets): notes. Review the paths or use --allow-any-path if intentional.
DEBUG   >> Complete list of paths not uploaded: notes

"1 path" for 500 files understates the stakes; someone skimming a warning about one path is unlikely to chase it. Annotating the count where people actually look — notes (500 files) — would need _partition_upload_assets to hand back the raw omitted list alongside collapsed. Entirely fine to defer, or to say no.

Nits

* `type(asset) is GenericAsset` re-encodes `find_dandi_files`'s own rule (`type(df) is GenericAsset and not allow_all`). It's the last duplicated fragment of discovery policy, and identity-vs-`isinstance` is exactly the subtlety that drifts when someone later adds an asset type. A shared predicate would close the loop this rewrite opened.

* `PurePosixPath(asset.path)` is built twice per asset in the `keep` comprehension. A plain loop with `keep.update((p, *p.parents))` avoids it. (Not a walrus — PEP 572 forbids assignment expressions in a comprehension's iterable expression, so the tempting one-liner is a `SyntaxError`.)

* `from .test_files import mkpaths` is a test-module-to-test-module import; `test_helpers.py` is the established home for shared helpers. Soft nit — `test_upload.py` already does something similar.

* `assert "note-11.txt" in caplog.text if count > 10 else "note-00.txt" in caplog.text` parses as `assert (X if C else Y)`, which is what's intended, but reads like lost parentheses.

Logic-wise I'd call this sound. Of the above, only the root-pruning comment and the extra parametrize row seem worth another push; everything else is optional.

Generated by Claude Code

Thanks for the careful re-review. I added the requested comment explaining why parent/child upload roots must be pruned, and added the . plus nested-path regression case so that request scoping cannot narrow the upload unexpectedly. The focused upload-discovery tests pass locally (20 passed, 1 skipped); the broader paired run reached 38 passed and 4 skipped, with one unrelated flaky-plugin compatibility error. The new commit is 585b8d6. Please take another look when convenient.

[yarikoptic-gitmate]

Re-reviewed at 585b8d6. Both asks landed, and I verified each rather than taking them on sight.

The comment is accurate — under_paths() narrowing the selection when a nested root arrives alongside its parent is exactly the failure mode.

The new parametrize row earns its place. I mutation-tested it: deleting the two pruning lines takes the suite from 2 failing params to 4, with the new roots5 ([".", "mixed/known.nwb"]) failing in both allow_any_path variants. So it does cover the .-plus-subpath shape the existing ["mixed", "mixed/known.nwb"] case left open.

Re-verified on this head: 40 passed / 3 skipped locally (up from 38, the +2 being the new param), and my fuzz harness is still 400/400 with all four invariants holding. The omission logic itself is unchanged since the last pass, so nothing to re-litigate there.

CI: one red job, and I've re-run it

test (ubuntu-latest, 3.12, normal, default) failed on this head — 35 of 36 checks pass. Flagging it because the last two rounds of this PR have had genuine CI noise and I don't want it merged on an assumption.

I couldn't extract the pytest failure; the retrievable portion of that job's log is entirely the post-failure docker-compose dump. But the circumstantial case for a flake is strong:

• That same job passed at c230cfd, the previous head.
• The entire delta since is 4 lines of test parametrize and 2 lines of comment — nothing that could selectively break 3.12 while ubuntu 3.11, 3.13, 3.14, nfs, dev-deps, lowest-deps and dandi-api all pass.
• master is intermittently red on its own right now: at unchanged ff82dab its scheduled run failed on 2026-09-16 (on windows-2022, 3.13) and passed on 2026-09-17.

I've triggered a re-run of just the failed job, so that should settle it shortly. Nothing for you to do unless it comes back red a second time.

One thing in the PR description is now wrong

The description says the change keeps "Dandiset metadata, dot/VCS paths, common OS metadata, empty directories, Zarr contents, and directory symlinks out of the warning." That stopped being true when the Thumbs.db/__MACOSX list was dropped:

tree: sub-01/s.nwb, Thumbs.db, .DS_Store, __MACOSX/._s.nwb, .git/config
omitted (warned about): ['Thumbs.db']

Dot-prefixed cruft is still silent — that falls out of find_dandi_files skipping dot-paths — but Thumbs.db is reported now, which is the right call and worth saying plainly rather than leaving the old claim standing.

While you're in there: the Local validation block still says "focused discovery tests: 3 passed" (21 now), and the nested-roots upload fix still isn't mentioned. That last one is the user-visible part of this PR and it's currently invisible to anyone reading the description.

Still open, all optional

The notes (500 files) annotation, the type(asset) is GenericAsset duplication, the doubled PurePosixPath(asset.path), the mkpaths cross-module import, and the ternary assert. None of them block anything; leaving them is a fine answer.

Logic-wise this is done from my side. Once the re-run is green, the only thing I'd want before merge is the description correction.

---

Generated by Claude Code

[AtomicGlance]

Re-reviewed at 585b8d6. Both asks landed, and I verified each rather than taking them on sight.

The comment is accurate — under_paths() narrowing the selection when a nested root arrives alongside its parent is exactly the failure mode.

The new parametrize row earns its place. I mutation-tested it: deleting the two pruning lines takes the suite from 2 failing params to 4, with the new roots5 ([".", "mixed/known.nwb"]) failing in both allow_any_path variants. So it does cover the .-plus-subpath shape the existing ["mixed", "mixed/known.nwb"] case left open.

Re-verified on this head: 40 passed / 3 skipped locally (up from 38, the +2 being the new param), and my fuzz harness is still 400/400 with all four invariants holding. The omission logic itself is unchanged since the last pass, so nothing to re-litigate there.

CI: one red job, and I've re-run it

test (ubuntu-latest, 3.12, normal, default) failed on this head — 35 of 36 checks pass. Flagging it because the last two rounds of this PR have had genuine CI noise and I don't want it merged on an assumption.

I couldn't extract the pytest failure; the retrievable portion of that job's log is entirely the post-failure docker-compose dump. But the circumstantial case for a flake is strong:

* That same job **passed at `c230cfd`**, the previous head.

* The entire delta since is 4 lines of test parametrize and 2 lines of comment — nothing that could selectively break 3.12 while ubuntu 3.11, 3.13, 3.14, `nfs`, `dev-deps`, `lowest-deps` and `dandi-api` all pass.

* `master` is intermittently red on its own right now: at unchanged `ff82dab` its scheduled run failed on 2026-09-16 (on `windows-2022, 3.13`) and passed on 2026-09-17.

I've triggered a re-run of just the failed job, so that should settle it shortly. Nothing for you to do unless it comes back red a second time.

One thing in the PR description is now wrong

The description says the change keeps "Dandiset metadata, dot/VCS paths, common OS metadata, empty directories, Zarr contents, and directory symlinks out of the warning." That stopped being true when the Thumbs.db/__MACOSX list was dropped:

tree: sub-01/s.nwb, Thumbs.db, .DS_Store, __MACOSX/._s.nwb, .git/config
omitted (warned about): ['Thumbs.db']

Dot-prefixed cruft is still silent — that falls out of find_dandi_files skipping dot-paths — but Thumbs.db is reported now, which is the right call and worth saying plainly rather than leaving the old claim standing.

While you're in there: the Local validation block still says "focused discovery tests: 3 passed" (21 now), and the nested-roots upload fix still isn't mentioned. That last one is the user-visible part of this PR and it's currently invisible to anyone reading the description.

Still open, all optional

The notes (500 files) annotation, the type(asset) is GenericAsset duplication, the doubled PurePosixPath(asset.path), the mkpaths cross-module import, and the ternary assert. None of them block anything; leaving them is a fine answer.

Logic-wise this is done from my side. Once the re-run is green, the only thing I'd want before merge is the description correction.

Generated by Claude Code

Thanks for the careful re-review and for rerunning the failed job! It passed. I’ve updated the description to clarify the Thumbs.db behavior, explain the overlapping-roots fix, and reflect the current validation results. I appreciate the extra checks on the regression test.