Align S3FileSystem.find() with fsspec for the path itself and prefix levels

[laughingman7743]

WHAT

S3FileSystem.find() (and AioS3FileSystem._find(), which calls it) now handles the path itself and prefix the way fsspec's find() does, in both the maxdepth and the unlimited branch.

Call (keys d/direct, d/sub/nested, d/sub/deep/x)	Before	After
find("d", withdirs=True) / find("d", maxdepth=1, withdirs=True)	no d	includes d (#963)
glob("d/**")	no d	includes d, as fsspec (#963)
find("d", maxdepth=1, prefix="sub/deep/")	["d/sub/deep/x"]	[] (#964)
find("d", maxdepth=3, prefix="sub/deep/")	["d/sub/deep/x"]	["d/sub/deep/x"]
find("d", prefix="sub/deep/", withdirs=True)	includes d/sub, d/sub/deep	["d", "d/sub/deep/x"] (#964)
find("d/direct", maxdepth=1)	[]	["d/direct"] (#966)

• Path itself (S3FileSystem.find(withdirs=True) omits the root directory #963, S3FileSystem.find(object_path, maxdepth=n) returns an empty list #966). If something is listed below the path, the path is a directory, and with withdirs it is included, unless it is a bucket. This needs no extra request. If nothing is listed, both branches now fall back to info(path): an object is returned, and with withdirs a directory is returned too. Before, only the unlimited branch did this. As in fsspec and in the unlimited branch, the fallback ignores prefix.

• prefix levels (S3FileSystem.find(maxdepth=..., prefix=...) ignores the depth limit when prefix contains a slash #964). With maxdepth, each / in prefix counts as one level below the path. If the prefix is already deeper than maxdepth, nothing is listed. With withdirs, neither branch includes the directories above the prefix, such as sub for sub/deep/. The maxdepth listing never returned them; the unlimited branch derived them from the keys and now derives only the directories below the last slash of the prefix. A slash-free prefix, which is what fsspec's glob() passes, lists the same entries as before.

• The recursion of the maxdepth branch moves to _find_levels(), which returns files and directories. _find() then handles the path itself and the fallback once, at the top level, and drops the directories without withdirs. The find() docstring describes both rules.

• mv() conflict check. _move_paths(), which S3FileSystem.mv() and AioS3FileSystem._mv() share since Delete only the copied source objects in mv() #1013, no longer counts a directory moved onto another directory as a conflict. Here, a directory is a source with another source below it. With the root included, mv("s3://bucket/src/**", "s3://bucket/src/archive/", recursive=True) paired src with an existing src/archive and raised ValueError; master moves it. A source that would conflict and has other sources below it is now skipped only if HeadObject finds no object at its key. Such a directory is not copied, so it writes no destination. An object that also has keys below it is copied, so it is still rejected. This costs one HeadObject, and only for those sources.

• mv() data safety (pre-existing on master, found in review). _move_paths() now compares paths by what they name and counts the sources left in place:

  Call	Master	Now
  mv([a, b], [b, b])	copies a over b, which was to stay in place	ValueError, nothing copied
  mv([a, b?versionId=null], [b, out])	copies a over b, then the overwritten b to out	ValueError, nothing copied
  mv([b?versionId=null], [b])	copies b onto itself, then deletes b?versionId=null, which removes b in a bucket without versioning	left in place

  A write to a key replaces its null version, so key?versionId=null is compared as key. Other versions do not change, so they stay distinct; for example, mv([b?versionId=v1], [b]) still copies v1 onto b and then deletes v1. In a bucket with versioning enabled, the null version is an older version that a write does not replace, but PyAthena does not look up the versioning state. There, moving the null version onto its key is also left in place, and the second call above also raises.

Remaining differences from fsspec:

• A bucket path gets no root entry. fsspec's recursive copy() keeps the directories from glob() and passes them to cp_file(), which raises ValueError for a bucket, so including it would break copy("s3://bucket/**", ..., recursive=True).
• A path that is both an object and a key prefix (d and d/x) is reported as a directory when something is listed below it. This is how the subdirectories were already treated.

Request counts

Measured with a mocked client (keys as above):

Call	Before	After
glob("d/*") with matches	1	1
find("d", withdirs=True) / maxdepth=1	1	1
find("d", maxdepth=1, prefix="s") with only a subdirectory listed	1	1
expand_path("d", recursive=True) with or without maxdepth (used by rm(), copy())	3	1
glob("missing/*") (sync), or a stem glob with no matches through AioS3FileSystem	1	3
glob("missing/**")	3	3
copy("d/**", "out/", recursive=True) / mv(...), flat d (the root's info() in _copy_file())	7 / 8	9 / 10
copy("src", "dst", recursive=True) / mv(...) (no exists() in expand_path())	11 / 12	9 / 10
mv("src/**", "src/archive/", recursive=True) with an existing src/archive/x	13	14

expand_path() checks exists(path) only when find() did not return the path, so the included root saves the HeadObject and the ListObjectsV2 request. When a glob with a maxdepth listing finds nothing, it now sends the info() fallback (HeadObject and ListObjectsV2), as fsspec's find() (isfile()) and the unlimited branch already did. That fallback raises PermissionError where HeadObject is denied, as the unlimited branch already did.

Release note (4.0.0, behavior change)

• S3FileSystem.find(path, withdirs=True) includes path when it is a directory other than a bucket, as fsspec does, so glob("s3://bucket/d/**") matches bucket/d.
• S3FileSystem.find(path, maxdepth=n, prefix=...) counts each / in prefix as one level below path. With withdirs=True, find(..., prefix=...) omits the directories above the prefix.
• S3FileSystem.mv() and AioS3FileSystem._mv() raise ValueError before copying when a destination is a source left in place, or the key of a source's null version, and they leave a null version moved onto its key in place. Before, such moves overwrote or deleted the object. This also applies in a bucket with versioning enabled, where the null version is not replaced by a write.
• S3FileSystem.find(object_path, maxdepth=n) returns [object_path] instead of []. A maxdepth listing that finds nothing now sends HeadObject and ListObjectsV2, so a glob with no matches sends 3 requests instead of 1.

WHY

Fixes #963, fixes #964, fixes #966.

fsspec's find() adds the root directory when withdirs is true ("needed for posix glob compliance") and returns [path] when the path is a file. fsspec's glob() calls find(..., withdirs=True), so glob("d/**") missed d (#963). The maxdepth branch returned [] for an object path, unlike the unlimited branch (#966).
With maxdepth, the listing Prefix=f"{key}/{prefix}" with Delimiter="/" treated entries below a slash-containing prefix as the first level, so every level was shifted by the slashes in the prefix (#964).

The maintainer chose: count the prefix's slashes as levels (#964), make the fallback match fsspec's expected behavior regardless of prefix (#966), and accept the request counts above.

TEST

Tested at ace8248, on master a73c6db (after #990, #1013, #1017, and #1024).

• just lint: passed.
• New tests (no AWS) in TestS3FileSystem. They run against a mocked client (_serve_keys) that answers ListObjectsV2, HeadObject, CopyObject, and DeleteObjects from a key set:
  • test_find_matches_fsspec (11 cases) and test_glob_matches_fsspec (8 cases, including maxdepth) compare against fsspec's MemoryFileSystem with the same keys.
  • test_find_withdirs_omits_bucket, test_find_directory_without_extra_requests, test_find_maxdepth_listings_follow_invalidation, test_find_prefix_counts_levels_from_path, and test_find_object_path_ignores_prefix cover the other rules.
  • test_mv_glob_with_directories covers three moves, checked on the resulting keys: a flat d/** (the directory itself is not copied); src/** onto an existing src/archive/, which fails without the _move_paths() change; and a directory that shares its destination with an object.
  • test_mv_objects_with_keys_below_conflict (2 cases) checks that objects that also have keys below them still raise when one would overwrite another source, and that no CopyObject or DeleteObjects request is sent. Its second case fails on 931171b, where every source with keys below it was skipped.
  • Two new test_mv_conflicting_destinations cases (a source left in place; a null version) and test_mv_versions_onto_their_key (the null version stays in place, while v1 is copied and deleted) fail before 1444eda. test_mv_version_with_keys_below_conflicts (a version with keys below its key is not taken for a directory) fails on 1444eda.
  • With the find() source reverted to master, 13 of the 30 find/glob tests fail.
• Real S3, sync: test_find_maxdepth, test_find_withdirs, and test_glob have new assertions. Async: test_glob checks glob("nested/**").
• uv run --env-file .env pytest -n 4 tests/pyathena/filesystem/: 581 passed, including Delete only the copied source objects in mv() #1013's live test_move_recursive (sync and aio).
• The request counts and the copy/mv comparisons come from offline probes with the same mocked client, run on master and on this branch. They were not measured against AWS.

Not run locally: the other suites. CI runs them once the PR is Ready.

🤖 Generated with Claude Code

[laughingman7743]

Self-review round one (behavior and implementation; base aa0fc9146f4e683d6cdf96b894f963f9dd8f7abe, initial head 2371d9686d0be4391a25c4b73a39d97621393e8e, repairs through a54580b2eda18fc6c16d3f0f8cd7df665489ef04)

Scope: the full diff, covering _find(), _find_levels(), _extract_parent_directories(), the find() docstring, and the offline and live tests. The callers through fsspec are glob(), expand_path() (used by rm(), copy() and get()), du(), and AioS3FileSystem._find()/_glob(), which delegate to the sync _find(). Round one included a /code-review pass and a /simplify pass (reuse, simplification, efficiency, altitude).

FINDINGS (repaired):

1. _find_levels() recursed with f"s3://{bucket}/{item.key}". _ls_dirs() caches under the raw path, so the subdirectory listings went under ("s3://bucket/dir/sub", "/"), a key that invalidate_cache() never drops. The old recursion through _find() stripped the protocol. Repro: after find(maxdepth=2) and invalidate_cache("s3://bucket/dir/sub/nested"), the entry stayed. Repaired in a54580b: the recursion uses item.name. test_find_maxdepth_listings_follow_invalidation fails on 533d401 and passes now.
2. Without withdirs, _find_levels() dropped directories. A path with only subdirectories below it therefore looked empty and went to the info() fallback, 1→3 requests. Repaired in 533d401: _find_levels() returns all entries and _find() filters them at the end. The prefix="s" case of test_find_directory_without_extra_requests covers it.

[laughingman7743]

Self-review round one (behavior and implementation; base aa0fc9146f4e683d6cdf96b894f963f9dd8f7abe, initial head 2371d9686d0be4391a25c4b73a39d97621393e8e, repairs through a54580b2eda18fc6c16d3f0f8cd7df665489ef04)

Simplification (applied): instead of a new prefix filter inside _extract_parent_directories(), the unlimited branch passes the base key rebased below the last slash of the prefix. This gives the same set (the altitude pass compared 3,000 random key/prefix cases) and leaves the helper unchanged from master. Also applied: _find_levels() handles maxdepth < 1 itself, so the caller's ternary goes; the hand-written listing table in test_find_maxdepth_counts_levels_like_fsspec is replaced by the shared _serve_keys().

[laughingman7743]

Self-review round one (behavior and implementation; base aa0fc9146f4e683d6cdf96b894f963f9dd8f7abe, initial head 2371d9686d0be4391a25c4b73a39d97621393e8e, repairs through a54580b2eda18fc6c16d3f0f8cd7df665489ef04)

Deferred, with reasons:

• A key that is both an object dir and a prefix dir/: the root is added as a directory, while info() returns the file. Subdirectories in both branches were already treated this way, and checking would cost a HeadObject on every find().
• A dir//x key gives duplicate dir entries. This is the pre-existing double-slash normalization in _ls_dirs(); prefix boundaries are tracked in clear_multipart_uploads() and object_version_info() match sibling keys by string prefix #981.
• The root entry of a bucket path is a synthetic directory rather than the HeadBucket info, and an empty bucket gets no root. Matching fsspec exactly would cost a HeadBucket request. The docstring (s3.py:828) now states the condition: objects exist below the path.
• A prefix with a leading slash or empty segments is not normalized. That is malformed input.
• Sibling directories are listed one at a time. This is pre-existing design.
• The list is copied twice with withdirs in the unlimited branch. The cost is negligible next to the listing requests.

[laughingman7743]

Self-review round one (behavior and implementation; base aa0fc9146f4e683d6cdf96b894f963f9dd8f7abe, initial head 2371d9686d0be4391a25c4b73a39d97621393e8e, repairs through a54580b2eda18fc6c16d3f0f8cd7df665489ef04)

FINDING (repaired in 533d401): an exact-equality check on glob("nested/**") breaks when CI reruns the test in the same session (#811), because the test writes a new uuid file each run. It now checks only that the root is included. The async live test keeps only this glob check, because AioS3FileSystem._find() runs the sync _find() and the other assertions would repeat sync coverage with real S3 requests.

Validation at a54580b: just lint passed. 29 offline find/glob tests passed; on master 13 of them fail. Live: pytest -n 1 test_s3.py test_s3_async.py -k "find or glob or rm or expand or copy or cp" gave 65 passed. The full tests/pyathena/filesystem/ run (295 passed) was at 2371d96 and is rerun in round two.

[laughingman7743]

Self-review round two (claims, callers, and operational effects; base aa0fc9146f4e683d6cdf96b894f963f9dd8f7abe, head a54580b2a→d016e99ab96f0a014aa5a0e7457c99fdaf812a86)

Claims checked:

• Every row of the PR's behavior table, and every request count, was re-run on master and on a54580b with the mocked client. All match. The request counts are fake-provider counts, not AWS measurements, and the PR says so.
• "expand_path() checks exists(path) only when find() did not return the path": confirmed in fsspec 2026.9.0, spec.py expand_path (if p not in out and (recursive is False or self.exists(p))). copy() (spec.py:1209) and get() (spec.py:1040) go through expand_path(), and S3FileSystem.rm() calls it directly. The expanded set is unchanged (the root was added through exists() before); only the requests drop from 3 to 1.
• "The fallback raises PermissionError where HeadObject is denied, as the unlimited branch already did": info() → _head_object() catches only FileNotFoundError, and _call() maps 403 to PermissionError. That is the same code path the unlimited branch had on master.
• Existing callers: the signatures of _find(), find(), and _extract_parent_directories() match master. AioS3FileSystem._find() still passes kwargs through. The result order now starts with the root, but nothing depends on the order, and fsspec sorts. du(withdirs=True) adds a size-0 root, so the totals are unchanged. fsspec is unpinned in pyproject.toml. Older fsspec _glob() does not pass prefix, which only avoids the prefix rules.
• Docs: docs/filesystem.md:65 shows plain fs.find(path), which this PR does not change. No other docs mention withdirs, prefix, or find(maxdepth).

Corrections (d016e99): this docstring said that with withdirs only the directories starting with the prefix are included, which contradicts the included root. It now says that the directories above the prefix are not included. The PR body was updated the same way, and it now lists the two remaining differences from fsspec (empty bucket, object-and-prefix key).

[laughingman7743]

Self-review round two (claims, callers, and operational effects; base aa0fc9146f4e683d6cdf96b894f963f9dd8f7abe, head a54580b2a→d016e99ab96f0a014aa5a0e7457c99fdaf812a86)

Correction (d016e99): the comment said the fallback returns the path only if it is an object. info() also returns a directory, which is kept with withdirs, for example when a prefix filters out everything below an existing directory. The comment now covers both.

AWS operator / adversarial: retries are unchanged (_call → retry_api_call). The fallback goes through info(), which reads the existing listing cache first, so a cached parent listing can answer it with no request, and a stale cache entry is served the same way it was on master's unlimited branch. The subdirectory listings of maxdepth are now cached under stripped paths, so invalidate_cache() drops them (round one's repair).

Evidence: at a54580b, pytest -n 4 tests/pyathena/filesystem/ against real S3 gave 299 passed, and the targeted run gave 65 passed. d016e99 changes only a docstring and a comment (just lint passed). The other suites are left to CI.

Result: FINDINGS (wording only), repaired. No deferrals beyond those recorded in round one.

[laughingman7743]

Independent review (relayed): OpenAI Codex CLI 0.160.0, model gpt-6-astra, reasoning effort max, --sandbox read-only, session 01a100e3-fa7e-7202-a731-1fa30509bda3. This was a static review of a detached snapshot at d016e99ab96f0a014aa5a0e7457c99fdaf812a86, diffed against the merge-base aa0fc9146f4e683d6cdf96b894f963f9dd8f7abe. The prompt carried no PR number, description, commit messages, or prior findings, and asked for no edits, builds, tests, network, or GitHub access. Afterwards, the snapshot and the PR worktree were both unchanged.

Reviewer output, verbatim:

Surfaces covered:

• Sync/async find, depth and prefix handling, directory inclusion, fallback, and detailed results.
• fsspec glob, expand_path, rm, copy, and du; bucket, version-aware, and trailing-slash paths.
• Listing caches, refresh/invalidation, request costs, and regression-test assertions.

FINDINGS

1. P2 — Introduced regression: recursive bucket-glob copying fails.
   pyathena/filesystem/s3.py:779
   Given an existing src bucket containing only a.txt, fs.copy("s3://src/**", "s3://dst/out/", recursive=True) now expands to include src itself. fsspec retains directories for unlimited recursive copying, so cp_file receives the bucket and raises ValueError("Cannot copy buckets.") at line 1222. The default error handler only ignores FileNotFoundError. At the base revision, this glob expanded only to the object and copying succeeded. The async copy path has the same rejection. Preserve the required root inclusion while making copying tolerate these directory entries.

2. P2 — Pre-existing omission, still violates the intended behavior: empty bucket roots disappear.
   pyathena/filesystem/s3.py:780
   For an existing empty bucket, fs.find("s3://bucket", withdirs=True) returns [], as does the maxdepth=1 variant, instead of ["bucket"]. Because key is None, neither branch reaches info(path). Consequently, glob("s3://bucket/**") also omits the existing directory. This behavior predates the diff but remains an uncovered part of the requested fallback/root-inclusion fix.

The added assertions check observable paths and request counts and would catch the three original defect classes. They do not cover either scenario above. Integration additions reuse existing fixtures and add modest request costs.

Static review only; no files changed, tests/builds run, or network accessed.

[laughingman7743]

Repair of the independent-review findings (head 39e41ad54…; the maintainer chose the approach)

1. Recursive bucket-glob copy (introduced regression): verified and repaired in 39e41ad. fsspec 2026.9.0's copy() filters directories only when it is not recursive or has a maxdepth (spec.py:1212). cp_file() raises ValueError("Cannot copy buckets.") for a path without a key (s3.py:1222), and that error is not ignored the way FileNotFoundError is. _find() now adds the root only for key paths (if withdirs and key), so a bucket path is left out, as on master. The find() docstring and the PR body list this as a difference from fsspec. test_find_withdirs_omits_bucket checks find() with and without maxdepth, and expand_path("s3://bucket/**", recursive=True), which copy() uses; it fails before the repair. A non-bucket root goes through cp_file() → CopyObject NoSuchKey → FileNotFoundError, which recursive copy() ignores, the same as the subdirectory entries already did (tracked in Recursive get(), copy() and mv() mishandle directory entries #974).
2. Empty bucket root (pre-existing): not changed. The bucket root is now excluded in every case, so an empty bucket follows the same rule, and the PR body records it as a difference from fsspec.

Repair self-review: behavior: the aio path delegates to the same _find(), and rm() skips keyless entries, so it is unaffected. Claims: the commit message, docstring, and PR body were checked against spec.py:1212 and s3.py:1222. Validation at 39e41ad: just lint passed; 30 offline find/glob tests passed; the live targeted run (-k "find or glob or rm or expand or copy or cp") gave 66 passed. The full filesystem suite result is recorded in the PR body.

[laughingman7743]

Independent follow-up review (relayed): OpenAI Codex CLI 0.160.0, model gpt-6-astra, reasoning effort max, --sandbox read-only, session 01a100f1-b978-7873-8621-c2e783f89bfd. This was a static review of d016e99a..39e41ad505b058e17ab6ab3dd9ce8f3d00d5f4d4 (same base aa0fc914) on a detached snapshot, which remained clean, as did the PR worktree.

Reviewer output, verbatim:

Reviewed d016e99a..39e41ad5 at the requested HEAD. Covered surfaces:

• _find/find: bucket versus key paths, bounded/unbounded depth, directory inclusion, and list/detail results.
• fsspec glob → expand_path → copy/cp_file/get, including destination mapping and exception handling.
• rm: sync and async deletion already filter keyless entries.
• du: sync explicitly retains the zero-size bucket entry; async follows _find and now omits it. Byte totals remain unchanged.
• Aio delegation through s3_async.py:335, inherited async callers, and generated sync wrappers.
• The new test covers bucket omission and wildcard expansion, but does not check transfer destinations.

FINDINGS

P2 — Removing the bucket changes recursive transfer destinations. s3.py:779

With only dir/file in the bucket, expansion of s3://bucket/** changes from:

bucket, bucket/dir, bucket/dir/file

to:

bucket/dir, bucket/dir/file

fsspec’s other_paths at utils.py:414 consequently strips bucket/dir instead of bucket. Thus await aio._get("s3://bucket/**", "/tmp/out", recursive=True) now downloads to /tmp/out/file, whereas it previously downloaded to /tmp/out/dir/file. Sync get has the same regression when its required local directories already exist. Copy uses the same mapping. Bucket-relative destination mapping needs preserving while excluding bucket copy operations.

Remaining failures, distinguished from that regression:

• Original failure only partly resolved: with the same tree, copy("s3://bucket/**", "s3://destination/", recursive=True) maps bucket/dir to the bare destination bucket. s3.py:1223 still raises ValueError("Cannot copy buckets."). This operation already failed before the repair; the source bucket is removed, but a destination bucket still reaches cp_file.
• Pre-existing literal-bucket case: copy("s3://bucket", dst, recursive=True) still includes the bucket because spec.py:1285 adds the existing literal input back.

Static source review only; no edits, builds, tests, GitHub, or network access. Worktree remained clean.

Author verification. These are offline probes with the mocked client, run on master, d016e99, and 39e41ad.

• Bucket-glob destination mapping: the result on 39e41ad is identical to master. For s3://bucket/** with only dir/file, master and 39e41ad both expand to [bucket/dir, bucket/dir/file], so the destination is /tmp/out/file; d016e99 gave /tmp/out/dir/file. This is a remaining difference from fsspec, not a regression against master. For a non-bucket root the PR improves the mapping. With only d/sub/nested, master mapped get("s3://bucket/d/**", out) to out/nested and dropped sub; 39e41ad maps it to out/sub/nested, as fsspec does.
• Copy to a bare destination bucket and literal bucket copy: both fail the same way on master. They are pre-existing.
• While tracing, the author found a further regression against master. With a flat directory (d/a, d/b), mv("s3://bucket/d/**", "s3://bucket/out/", recursive=True) succeeds on master but raises FileNotFoundError('d') on 39e41ad, without copying anything. The root d now reaches cp_file(), CopyObject fails with NoSuchKey, and mv() uses on_error="raise". Recursive copy() still succeeds but sends one more failing CopyObject (2→3). A literal mv("s3://bucket/d", recursive=True) already fails this way on master, which is Recursive get(), copy() and mv() mishandle directory entries #974. Open PR Follow fsspec's get_file() contract for directories and file objects #990 makes cp_file()/_cp_file() skip directory entries, which resolves this; how to proceed is with the maintainer.

[laughingman7743]

Self-review after the rebase (rounds one and two of the repair). Old range aa0fc914..39e41ad5, new range de8cc52ac2a43cdba72bb4571c185883287741fc..931171b9. The rebase went through c3ddabf (#990, which landed only the get_file() part) and then de8cc52 (#1013 / #1008). The patch series is the same apart from an import conflict in test_s3.py. The new commits are 6c54bf9 and 931171b.

Round one (behavior). Upstream changes that touch this PR's contracts:

• Delete only the copied source objects in mv() #1013: cp_file()/_copy_file() skip directories, and mv() deletes only the copied sources. This resolves the flat-directory mv("d/**", recursive=True) regression; test_mv_glob_with_directories[0] covers it.
• Follow fsspec's get_file() contract for directories and file objects #990: get_file() runs makedirs(lpath) for a directory rpath, so the root entry now creates the destination.
• S3FileSystem cache reads and invalidations raise KeyError under concurrent invalidation #996: the dircache get() changes. clear_multipart_uploads() and object_version_info() match sibling keys by string prefix #981: prefix boundaries, which do not touch find().

FINDING (introduced by the root, repaired). With an existing src/archive/x, mv("s3://bucket/src/**", "s3://bucket/src/archive/", recursive=True) raised ValueError because the root src maps to src/archive, which is a source directory. Master moves it to src/archive/a and src/archive/archive/x. The maintainer chose to drop directories from the check.

• 6c54bf9 excluded every source that has another source below it.
• Round two found a hole in that. An object d that also has d/x below it counts as a directory, but _copy_file() copies it. With mv([d, d/x, e], [e, o/x, o/e]), d would overwrite e before e is copied, and master rejects this.
• 931171b skips only a directory moved onto another directory. test_mv_conflicting_destinations[path12-path22] fails on 6c54bf9 and passes now.
• The directory set is built from the parents of the stripped sources in O(n × depth) time, with no requests. The aio _mv() uses the same _move_paths().

[laughingman7743]

Self-review after the rebase (rounds one and two of the repair). Old range aa0fc914..39e41ad5, new range de8cc52ac2a43cdba72bb4571c185883287741fc..931171b9. The rebase went through c3ddabf (#990, which landed only the get_file() part) and then de8cc52 (#1013 / #1008). The patch series is the same apart from an import conflict in test_s3.py. The new commits are 6c54bf9 and 931171b.

Round two (claims and evidence).

• Commit messages and docstrings: the "directory moved onto another directory" wording matches the code. The mv()/_mv()/_move_paths() Raises sections were updated.
• PR body: the dependency note and the TEST section are updated to this head.
• _serve_keys fake: it now applies CopyObject and DeleteObjects to its key set, so these tests check the keys after the move rather than calls. In test_mv_glob_with_directories, case 1 passes on master (no root there) and case 2 fails on 6c54bf9's predecessor without the _move_paths() change.
• Deferred: the copy-time NoSuchKey/info() cost of directory entries is the same as on master for subdirectories.

Validation at 931171b:

• just lint passed.
• 42 offline find/glob/mv/cp_file tests passed.
• uv run --env-file .env pytest -n 4 tests/pyathena/filesystem/ against real S3: 545 passed. 6c54bf9 gave 544.

Independent full-scope review of de8cc52a..931171b9 is next.

[laughingman7743]

Independent full-scope review after the rebase (relayed): OpenAI Codex CLI 0.160.0, model gpt-6-astra, reasoning effort max, --sandbox read-only, session 01a10211-d0ab-7291-8301-321196094238. This was a static review of de8cc52ac2a43cdba72bb4571c185883287741fc..931171b9ff5b735bccfe27f9e0155eab03921f90 on a detached snapshot, which remained clean, as did the PR worktree. The prompt carried no PR number, description, commit messages, or prior findings.

Reviewer output, verbatim:

Covered: exact de8cc52a..931171b9 diff; sync/async find, prefix/maxdepth handling, glob, expand_path, rm, copy/move, get/get_file, du, caching/invalidation, bucket paths, trailing slashes, request counts, and regression-test effectiveness against the supplied fsspec source. Static review only; no builds, tests, or network access.

FINDINGS

1. P1 — Introduced: real objects can bypass move-conflict checks and lose data. pyathena/filesystem/s3.py:1481

   With an unversioned bucket containing distinct object payloads at a, a/b, and a/b/x, call:

   fs.mv(
       ["b/a", "b/a/b", "b/a/b/x"],
       ["b/a/b", "b/out", "b/out/x"],
       recursive=True,
   )

   Both b/a and b/a/b enter directories because they have descendants. Consequently, a → a/b is excluded from conflict checking, although _copy_file() identifies and copies both as actual objects. The first copy overwrites a/b; the second copies that overwritten payload to out; deletion then removes the sources. The original a/b payload is lost instead of the move raising ValueError before copying.

   The shared helper also exposes async moves. The added test at test_s3.py:1515 misses this because destination e has no descendants. Exemption must establish that the source is a directory that copy will skip; ancestry alone cannot establish that.

Author verification and repair (97232d3). Confirmed. Being an ancestor of another source does not prove that a source is a directory. A pair that would conflict is now skipped only if its source has other sources below it and _head_object() finds no object at its key. A pure directory is not copied, so it writes nothing. test_mv_objects_with_keys_below_conflict covers the reviewer's case (a, a/b, a/b/x → ValueError, nothing copied) and the case from 931171b; the reviewer's case fails on 931171b. Measured with the mocked client, the extra HeadObject is sent only for those sources: mv("src/**", "src/archive/") with an existing src/archive/x makes 13 requests on master and 14 here. A 403 from HeadObject raises PermissionError before anything is copied. Validation at 97232d3: just lint passed; 43 offline find/glob/mv tests passed; the live tests/pyathena/filesystem/ run gave 546 passed. An independent follow-up of 931171b9..97232d3c is running.

[laughingman7743]

Independent follow-up 1 (relayed): Codex CLI 0.160.0, gpt-6-astra, effort max, read-only, session 01a1021d-7422-7011-8ba2-bf102fdb514c. This was a static review of 931171b9..97232d3cc967e3185c553350827f135ab9325683.

Covered 931171b9..97232d3c: conflict detection, fsspec expansion/mapping, sync/async copy-and-delete ordering, HeadObject errors/cache/version handling, request costs, and new tests. Static inspection only; no edits, execution, or network access.

The reported object-with-descendants overwrite is resolved. HeadObject 403 aborts before copying; 404 permits exemption. Positive cache entries cannot grant exemption. The guard adds lookups only for potentially conflicting ancestor sources; misses are uncached and may be looked up again during copying. Async awaits the same guard before starting copies.

FINDINGS

Example paths below omit the common s3://bucket/ prefix.

1. P2 — Repair regression: exempt directories still contribute to duplicate destinations. s3.py:1474, s3.py:1489. With only objects d/x and e/y, mapping [d, d/x, e/y] → [e, e, out] previously succeeded: directory d writes nothing, and the two objects have distinct destinations. Now d receives HeadObject 404 and is exempted, but counts[e] remains two, so d/x raises ValueError. Exempt pairs need to be excluded from duplicate-writer accounting.

2. P1 — Pre-existing: mutable null-version sources evade overlap detection. s3.py:1472. In an unversioned or versioning-suspended bucket, mapping [a, b?versionId=null] → [b, out] passes because b differs textually from the version-qualified source. Sync copies a over b’s null version, then copies that overwritten content to out; original b is lost. Async permits the corresponding race. This predates the repair.

3. P1 — Pre-existing: removing self-moves hides protected sources. s3.py:1470. Mapping [a, b] → [b, b] removes b → b before conflict detection. The remaining copy overwrites b, despite the documented promise to leave self-destination files in place. This also predates the repair.

The new tests cover the reported overwrite layouts, but their key-set assertion cannot detect an overwrite of an existing key before an exception. Explicitly asserting that no copy/delete request occurred would verify that requirement.

Author disposition.

• 1 (regression from 97232d3): confirmed and repaired in d4e9342. Skipped directories are now dropped before both checks. The [d, d/x, e/y] → [e, e, out] case was added to test_mv_glob_with_directories; it fails on 97232d3. The conflict tests now also check that no CopyObject or DeleteObjects request was sent, as the reviewer suggested.
• 2 and 3 (pre-existing in Delete only the copied source objects in mv() #1013's _move_paths() on master): not changed here. One is a ?versionId=null source that is compared as text; the other is a self-move that is dropped before the checks. Both reproduce on master without this PR, so they are left out of this PR to keep its scope. Whether to file them as an issue is with the maintainer.

Independent follow-up 2 (relayed): same reviewer setup, session 01a10228-d4e8-73e0-9177-de1a3e6a6ab0. This was a static review of 97232d3c..d4e9342f91fb5a267d3076931177afaf672f85cb, with the two pre-existing issues named as out of scope.

Covered 97232d3c..d4e9342f:

• Conflict checks, s3.py:1484: exempted pairs are removed before recounting destinations. Remaining destinations are checked against the full source set. Both callers finish validation before copying.
• Request costs: validation HeadObject calls remain limited to potentially conflicting ancestor sources. The eager filtering pass can probe later eligible ancestors before rejecting an earlier conflict.
• Regression test, test_s3.py:1557: covers the exact [d, d/x, e/y] → [e, e, out] case and expected final keys. Safety assertions at line 1595 explicitly check that rejected object conflicts issue neither copy nor delete requests.

CLEAN — the previous duplicate-destination false positive is resolved. No correctness or data-safety regression found in this repair. The two identified pre-existing issues remain excluded from the verdict.

Static review only; no tests, builds, writes, or network access.

Validation at d4e9342: just lint passed; 44 offline find/glob/mv tests passed; the live tests/pyathena/filesystem/ run gave 547 passed. Both snapshots and the PR worktree stayed clean.

[laughingman7743]

mv() data-safety fixes folded in at the maintainer's request (the pre-existing findings 2 and 3 from the earlier follow-up, plus mv([b?versionId=null], [b]), which deleted b). The commits are 1444eda and a7306a1, on master fe21250.

Self-review.

• Round one (behavior): _move_target() compares bucket/key, keeping the version ID except for null. The sources left in place count for the "destination is another source" check. parse_path() accepts all four versionId spellings. An invalid path now raises at the check instead of later in cp_file(). The aio _mv() shares _move_paths(). No requests are added.
• Round two (claims and operations): in a bucket with versioning enabled, the null version is an older version that a write does not replace. PyAthena does not call GetBucketVersioning, so there a null version moved onto its key is left in place, and [a, b?versionId=null] → [b, out] raises. The PR body and the release note state this. The "deleted b" claim follows from the requests: CopyObject b(null) → b, then DeleteObjects b with VersionId=null, which in a bucket without versioning is the object itself.

Independent follow-up on 1444eda (relayed): Codex gpt-6-astra, effort max, read-only, session 01a10268-75a8-7290-823f-c0f3375816c2.

Covered 1444edad: path normalization, all four version-ID spellings, invalid paths, sources left in place, duplicate destinations, directory exemptions, copy/delete ordering, async parity, request costs, and new tests.

The stated examples are repaired. Normalization adds no requests; directory exemptions retain conditional HeadObject calls, with no bucket-versioning lookup. Async uses the same preflight checks. The new tests verify rejection/no-request behavior and version-specific request parameters.

FINDINGS

P1 — Regression: directory exemption probes the wrong version. s3.py:1475 supplies normalized identities to the HeadObject check at line 1494, removing versionId=null.

Concrete scenario: versioned bucket src contains a readable null version of d, hidden by a current delete marker, plus objects d/x and a. Bucket dst is unversioned:

fs.mv(
    ["src/d?versionId=null", "src/d/x", "src/a"],
    ["dst/out", "dst/x", "dst/out"],
)

d/x makes normalized src/d a directory candidate. Unqualified HeadObject returns missing, so d → out is excluded from duplicate-destination checks. However, the returned pairs retain the original version-qualified source: its copy succeeds, then a → out overwrites it. Deletion permanently removes the original null version, losing its contents.

The parent rejected this duplicate destination. Async admits the same conflicting copies. This exceeds the accepted conservative null-version trade-off. Preserve the original source version for the existence probe, or exclude explicit versions from directory exemptions.

The new version test misses this because _serve_keys models keys without versions or delete markers.

Pre-existing limitation — scalar version-qualified sources undergo glob expansion. At s3.py:1452, mv("bucket/b?versionId=null", "bucket/out") treats ? as a wildcard and raises FileNotFoundError when only b exists. Both-list inputs bypass this. This behavior is unchanged by the commit.

Static review only; no tests, builds, edits, or network access. HEAD remained unchanged and the worktree clean.

Disposition.

• P1: confirmed, repaired in a7306a1. A source whose original path has a version is never exempted as a directory. test_mv_version_with_keys_below_conflicts covers it; it fails on 1444eda.
• The scalar ?versionId= glob expansion: pre-existing and unchanged; it is related to Keys containing '?' are rejected, and pinned or listed versions are not addressable #979 (keys containing ?).

Independent follow-up on a7306a1 (relayed): same setup, session 01a1026f-9a5d-7d22-8c50-8c1fa01747ea.

Covered a7306a1 against its parent:

• Repair and collisions: s3.py:1495 checks the original path, so ?versionId=null cannot receive the directory exemption. A hidden null version with descendant keys now remains subject to both duplicate-destination and destination-is-source checks.
• Data safety: Traced normalization, protocol aliases, null/non-null versions, sources left in place, ordinary/multipart copies, and deletion. No remaining collision bypass found within the reviewed scope.
• Request costs: The added parsing makes no requests. Versioned candidates skip the exemption’s HeadObject; unversioned handling is unchanged.
• Async parity: s3_async.py:363 awaits the same validation before scheduling any copies.
• New test: test_s3.py:1606 exercises the null-version exemption hole through duplicate destinations and asserts no copy/delete requests. By inspection, it would fail against the parent. Its mock does not model stored versions/delete markers, so coverage is of preflight rejection.

CLEAN — the prior finding is resolved. No regression introduced by this commit or additional pre-existing defect identified within this scope.

Static review only; no files changed, builds/tests run, or GitHub/network access.

Validation at a7306a1: just lint passed; 48 offline find/glob/mv tests passed; the live tests/pyathena/filesystem/ run gave 564 passed. The snapshots and the PR worktree stayed clean.

[laughingman7743]

Rebase onto a73c6db (conflict resolution): fe21250a..a7306a10 → a73c6db4..ace8248c44e166e8dd22a07431863a0e37edc497.

• Conflict: only the import block of s3.py. Send the lookup parameters of a file with its lookups #1024 added import time next to this PR's from collections import Counter, and both are kept. git range-diff shows all 12 commits unchanged except for that context line in e37e046.
• Upstream contracts checked:
  • Send the lookup parameters of a file with its lookups #1024 gives _head_object() an optional lookup_kwargs and caches versions under one ?versionId= spelling. The call in _move_paths() keeps its meaning.
  • _move_paths() sends HeadObject without lookup parameters, as _copy_file()'s info(path1) and expand_path() on master do, so this adds no new divergence.
  • Keep system metadata in setxattr() and reject version paths #1016 (setxattr), Use the compression table properties that Athena applies on write #1025, and Keep NULL rows of single-column CSV results in ArrowCursor #1031 do not touch find() or mv().
• Validation at ace8248:
  • just lint passed.
  • 48 offline find/glob/mv tests passed.
  • uv run --env-file .env pytest -n 4 tests/pyathena/filesystem/ against real S3: 581 passed.