Fix get_files(directory) on the S3 and local storage drivers

[tmgbedu]

Fixes #218

get_files(directory) was broken on both real drivers, in two different ways, and the drivers disagreed about what a listing means — making it unusable from disk-agnostic code.

The two bugs

S3 — any non-empty prefix returned []. The nested-file filter tested the whole key with "/" not in key instead of the part below the prefix. Listing backups matched backups/a.sql, which contains a slash, so every file was rejected; meanwhile root-level keys (root.txt) passed the test and leaked into prefixed listings. Now the prefix is stripped and the remainder is what gets tested. A trailing slash is tolerated (backups == backups/) and the directory placeholder object is skipped.

Local — silent data loss. os.listdir yields bare names, but the loop called self.get(name), which resolves against the disk root rather than root/<directory>. get() swallows FileNotFoundError and returns None, so callers got File objects with None content and no error. The read is now joined to the directory. A missing directory returns [] instead of raising, and entries are sorted so the order matches what S3 returns.

Shared contract

All three drivers now document and implement the same thing: a non-recursive listing of the files directly under the given directory, with no directory entries. File.name() is the bare filename on every driver (S3 previously returned the full key). FakeDriver.get_files delegates to LocalDriver rather than reimplementing the listing, so the test double cannot drift from what it stands in for.

get_files was also missing from the StorageManager / Storage passthroughs, so it was unreachable via the facade — added.

File.content stays lazy on S3 (the boto3 ObjectSummary) rather than eagerly downloading every object body on a listing; local keeps its eager read. This difference is intentional and was confirmed with the PM.

Verification

Real MinIO (127.0.0.1:9002), bucket seeded with root.txt, backups/a.sql, backups/b.sql, backups/2024/deep.sql, audio/x/y.mp3:

get_files('backups')  -> ['a.sql', 'b.sql']
get_files('backups/') -> ['a.sql', 'b.sql']
get_files('audio')    -> []
get_files()           -> ['root.txt']
get_files('nope')     -> []

Before the fix, get_files('backups') returned [].

New tests: 9 real MinIO integration tests (test_s3_minio_integration.py, auto-skipped when no endpoint is reachable — overridable via S3_TEST_ENDPOINT/S3_TEST_KEY/S3_TEST_SECRET), 16 mocked S3 unit tests for the prefix logic, 8 local-driver tests asserting real byte content, and FakeDriver↔LocalDriver parity tests over an identical layout. boto3 added to the dev dependency group so the S3 path is testable at all.

2222 passed, 7 skipped in 101.61s
Required test coverage of 80.0% reached. Total coverage: 84.49%

🤖 Generated with Claude Code

https://claude.ai/code/session_01FyozUDQzC8en24yx1Je1ct

[tmgbedu]

Review — PR #220 (fixes #218)

The core prefix fix is right and I verified it against live MinIO. Two real defects in the local
driver block approval, plus some smaller notes.

Numbers I observed myself on 1a71f58:

• uv run pytest --ignore=tests/masoniteorm/postgres --cov → 2222 passed, 7 skipped, 84.49% coverage (matches the PR description)
• uv run pytest tests/storage/test_s3_minio_integration.py -v against the live MinIO on 127.0.0.1:9002 → 9 passed
• Same file with S3_TEST_ENDPOINT=http://127.0.0.1:9999 → 9 skipped, not passed. The guard is honest; CI will not go green on nothing. ✅

---

🔴 Blocking

1. local.py:138 — get_files() now raises UnicodeDecodeError on any non-UTF-8 file.

File(self.get(join(directory, name)), name) eagerly reads every file, and LocalDriver.get()
opens in text mode (open(..., "r")). Before this PR the path was broken differently — get(f)
missed the file and the swallowed FileNotFoundError produced File(None, ...). Now the read
succeeds and blows up on the first byte.

Reproduced on this branch:

audio/clip.mp3  (b"\xff\xfb\x90\x64...")
audio/ok.txt

>>> driver.get_files("audio")
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in position 0: invalid start byte

This is worse than the bug being fixed: silent None content becomes a hard 500, and it takes the
whole listing down, not just the one file. Issue #218's motivating example is literally an audio/
directory — mp3s, images and PDFs are the common case for a storage disk.

It also breaks the "agree across drivers" claim in the commit message: S3Driver.get_files never
reads object bodies at all, so the same layout works on S3 and raises on local/fake.

What to do: don't read content during a listing. Populate File lazily (or with the path /
None) so get_files stays a listing operation, and let the caller call get()/stream() on the
entries it actually wants — which is also what the S3 side does. If eager content really is wanted,
it must at minimum be a binary-safe read. Add a test with a non-UTF-8 byte in the directory.

2. local.py:132 — a read-only listing creates directories on disk.

self.get_path(directory) calls make_file_path_if_not_exists(), which os.makedirs() the parent.
So listing a directory that does not exist mutates the disk:

>>> driver.get_files("reports/2024")
[]
>>> os.path.exists(root/"storage"/"reports")
True        # created by a get_files() call

test_missing_directory_returns_empty_list uses the single-segment "nope", whose parent is the
storage root (already present), so it passes without catching this. Resolve the path without
get_path()'s side effect (join root directly, or add a create=False flag), and extend the test to
a nested missing path.

---

🟡 Non-blocking notes

3. The S3 key is recoverable but undiscoverable. #218's use case ("most recent file under this
prefix") needs the full key to re-fetch. It is reachable via file.stream().key because content is
the ObjectSummary — the PR's own test asserts this — so no regression. But stream() returning an
ObjectSummary on S3 and a content str on local is a sharp edge, and nothing documents it.
File.path() currently just passes; that is the natural place to expose the key. Please at least
document it in the get_files docstring.

4. fake.py:27 — the get_files override is dead code. The body is return super().get_files(directory)
and FakeDriver already extends LocalDriver, so behaviour is identical with or without it. The
docstring is useful; the method is not. Delete it (move the note to the class docstring) or leave it
— but it should not read as if it adds behaviour. The LocalDriver delegation itself is sound: the
fake is a real temp-dir local disk, so it can't drift. It does mean the fake will inherit blocker #1.

5. assert_count fix is correct. f.name → f.name() — the old message printed bound-method
reprs. Good catch.

---

✅ Verified as claimed

• S3 prefix slice has no off-by-one. normalize_directory always yields "" or "dir/", and
  summary.key[len(prefix):] is exact for both. "backups", "backups/", "/backups/" all agree;
  the parametrised test covers it and MinIO confirms it.
• File.name() full-key → bare-name is non-breaking, claim holds. Checked the old code rather
  than taking it on faith: at root, prefix == "", the "/" not in key filter is identical, and
  name == key — byte-for-byte the same result. For any non-empty directory the old filter tested
  the whole key, so every nested key was dropped and the call returned []; there were no working
  callers to break. One nuance worth knowing: the old code treated the directory as a raw substring
  prefix, so get_files("back") used to return the root key "backups.txt" and now returns [].
  That old behaviour was incoherent (a prefix is not a directory) and I would not call it a
  regression, but it is the one input whose result changed from non-empty to empty.
• uv.lock churn is clean. +53 lines: boto3, botocore, jmespath, s3transfer and the dev
  group entries. Nothing removed, no unrelated version moves. The only other change is the
  fastapi-startkit self-version 0.51.0 → 0.56.0, which is the lock catching up to pyproject,
  not a dependency change.
• Facade .pyi: I checked this explicitly because CLAUDE.md says every facade has a stub.
  Storage is not in src/fastapi_startkit/facades/ — it is a plain class in storage/storage.py,
  and there is no Storage.pyi anywhere in the repo, before or after this PR. Every existing method
  there is *args, **kwargs with no annotations, so get_files is exactly as typed as its
  neighbours. Not a gap this PR introduced, and not blocking — but the facade being unstubbed at all
  is worth a follow-up issue.

[codecov]

Codecov Report

❌ Patch coverage is 92.30769% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...i_startkit/src/fastapi_startkit/storage/storage.py	60.00%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

[tmgbedu]

Both blockers fixed in ddb17cb. Reproduced each first, wrote the tests, watched them fail against the previous head, then fixed.

Blocker 1 — eager read crashes on binary files

Reproduced exactly as described:

get_files('audio') -> RAISED UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in position 0

Listings no longer carry a body at all — get_files returns the listing, content is fetched on demand with get(join(directory, file.name())). After:

get_files('audio') -> ['clip.mp3', 'notes.txt']

One correction to the justification, since I was asked to verify rather than take it. The claim was that local get_files() content was always None before, so nothing can depend on it. That is true for sub-directory listings but not for root listings — pre-PR, get_files("") called self.get(name), which resolves correctly at the root, so content was populated there:

OLD root listing content:   [('root.txt', 'root content')]
OLD subdir listing content: [('one.txt', None)]

The conclusion still holds, for two other reasons: get_files has zero callers anywhere in the repo (framework, application/, example/, docs — the only references are the passthroughs this branch added), and the root path already crashed on a binary file under the old code too, so there was no working binary behaviour to preserve:

OLD code, binary file at root -> RAISED UnicodeDecodeError

So this is safe, but not because content was always None.

Blocker 2 — read-only listing mutated disk

Reproduced: get_files('reports/2024') returned [] but left reports/ created on disk.

Path resolution is now split. resolve_path() resolves against the disk root and touches nothing; get_path() keeps its create-on-demand behaviour for the write paths that actually want it. get_files uses resolve_path. After:

before: reports/ exists? False
get_files('reports/2024') -> []
after:  reports/ exists? False

Tests, failing-first

The six new assertions all failed against the previous head:

FAILED ...DoesNotReadContent::test_binary_file_does_not_break_the_listing
FAILED ...DoesNotReadContent::test_binary_file_at_the_root_does_not_break_the_listing
FAILED ...DoesNotReadContent::test_content_is_fetched_on_demand_not_during_the_listing
FAILED ...DoesNotMutateDisk::test_missing_nested_directory_returns_empty_and_creates_nothing
FAILED ...DoesNotMutateDisk::test_listing_a_disk_whose_root_is_absent_creates_nothing
FAILED ...DoesNotMutateDisk::test_resolve_path_does_not_touch_the_filesystem
6 failed, 1 passed

The one that passed is test_missing_single_segment_directory_creates_nothing — kept precisely because it documents the gap that let blocker 2 through, now pinned alongside the multi-segment case.

Two existing tests asserted the old eager-content contract and were updated rather than deleted: test_file_content_is_populated_not_none became test_listed_names_resolve_back_to_their_content, and the Fake↔Local parity test now asserts both drivers carry no body and that the listed names resolve to identical content through each driver's own get().

Non-blocking items

• File now documents what content means per driver, including that file.stream().key recovers the full S3 key. Put on the class rather than File.path() — path() is an unimplemented stub returning None, so a docstring there would be easy to miss.
• FakeDriver.get_files removed. It was a pure super() passthrough, and as noted it inherited blocker 1, so fixing local fixed the fake; the parity tests hold it to the contract.

Verification

2229 passed, 7 skipped in 96.53s
Required test coverage of 80.0% reached. Total coverage: 84.49%
storage/drivers/local.py 99% | storage/drivers/fake.py 100% | storage/file.py 90%

MinIO re-verified after the change, unchanged:

get_files('backups')  -> ['a.sql', 'b.sql']
get_files('backups/') -> ['a.sql', 'b.sql']
get_files('audio')    -> []
get_files()           -> ['root.txt']
get_files('nope')     -> []

[tmgbedu]

Follow-up — correction on my part, and one thing the zero-callers claim misses

You were right to push back, and I got a detail wrong. Re-checked everything by running the old
implementation side by side with the new one.

Where I was wrong

The binary crash at the root is pre-existing, not introduced by this PR. I said the old code
produced File(None, ...); at the root self.get(f) resolves correctly, so content was populated
and a non-UTF-8 file blew up there too. Verified:

ROOT listing with a .mp3 present
  OLD: RAISED UnicodeDecodeError
  NEW: RAISED UnicodeDecodeError

So "this PR turns silent None into a hard 500" was wrong as stated. Withdrawn.

Where the old behaviour was worse than either of us said

It was not None in the sub-directory case — it was silently the wrong file's contents. The old
self.get(f) passed a bare name, so it resolved against the disk root regardless of which directory
was listed. With shadow.txt present at both levels with different contents:

Listing "audio/":
  OLD   shadow.txt -> 'I AM THE ROOT COPY'      # <- wrong file, silently
        unique.txt -> None                      # None only when nothing shadows it
  NEW   shadow.txt -> 'i am the audio copy'
        unique.txt -> 'i am unique to audio'

That is data corruption, not just data loss, and it makes the core fix more clearly worth having.

Where the zero-callers claim doesn't hold

I grepped independently — framework, example/, packages/, bin/, plus application/ and the
docs site (docs/, index.md) from the main checkout, which are not present in my worktree. No
external callers, agreed. But there is one internal caller: fake.py:64, FakeDriver.assert_count()
— and assert_directory_empty() through it. Those are public, user-facing test helpers, and they
run on the local/fake driver, which is the one with the eager text-mode read.

That caller demonstrably regresses:

FakeDriver.assert_count(1, "audio")  with one .mp3 in the directory
  OLD: OK
  NEW: RAISED UnicodeDecodeError

So the safety argument can't rest on zero callers — Storage.fake('s3').assert_count(3, 'uploads')
over an image or audio fixture passes today and raises on this branch. That is the thing I'd still
like fixed, and the fix is the same one-liner: don't read file contents during a listing. It also
removes the eager full-directory read, and brings local into line with S3, which never reads bodies.

Unchanged

Blocker 2 stands untouched by any of this — get_path() still makedirs the parent on a read-only
get_files("reports/2024"). Nothing above bears on it.

Everything in the "verified as claimed" section of my first comment also stands: prefix slice, the
bare-name change, key recoverability via stream().key, and the uv.lock churn.

[tmgbedu]

✅ Re-review of ddb17cb9 — both blockers fixed, approving

Re-ran everything against the new head. (Posting as a comment rather than a formal approval because
GitHub won't let me review my own account's PR.)

Blocker 1 — eager body read: fixed

get_files now appends File(None, name) and never opens a body. Verified directly:

FakeDriver.assert_count(1, "audio") with a .mp3 present   -> PASSES   (was: UnicodeDecodeError)
get_files("audio") over clip.mp3 + ok.txt                 -> ['clip.mp3', 'ok.txt'], contents [None, None]
get_files("") with a binary file at the root              -> lists cleanly
driver.get("audio/ok.txt")                                -> 'hello'   (on-demand read still works)

This also fixes the pre-existing root-level crash I conceded was not yours, and it drops the eager
full-directory read, so a listing is now O(1) syscalls per entry instead of a full read. Local and S3
now genuinely agree: neither carries a body.

Blocker 2 — listing mutated the disk: fixed

Splitting resolve_path() (pure) out of get_path() (creates) is the right cut — get_path keeps
its existing create-on-write behaviour for put/store/append, and only the listing opts out.

get_files("reports/2024") on a disk with no reports/ -> []  |  reports/ created: NO   (was: YES)

Non-blocking notes, also addressed

The File docstring now spells out that listing results carry no body and that stream().key
recovers the full S3 key — that was my note 3, and it's a better answer than the File.path() idea I
floated. The dead FakeDriver.get_files passthrough (note 4) is gone.

Test coverage of the fixes

Both regressions are now pinned by tests that would have caught them, which is what I wanted:
test_binary_file_does_not_break_the_listing, test_binary_file_at_the_root_...,
test_content_is_fetched_on_demand_not_during_the_listing,
test_missing_nested_directory_returns_empty_and_creates_nothing,
test_listing_a_disk_whose_root_is_absent_creates_nothing, and
test_resolve_path_does_not_touch_the_filesystem. The parity test was correctly reworked to compare
names plus on-demand get() rather than listing bodies.

Numbers I observed on ddb17cb9

• uv run pytest --ignore=tests/masoniteorm/postgres --cov → 2229 passed, 7 skipped, 84.49% (+7 tests, coverage held)
• MinIO integration against the live container on :9002 → 9 passed

Everything from my first comment's "verified as claimed" section is untouched by this commit and
still stands. Approving — no outstanding items from me. Merge is the PM's call.