feat(snapshot s3): fingerprint S3 buckets from a virtual tree by mbevc1 · Pull Request #1180 · kosli-dev/cli

mbevc1 · 2026-09-14T20:03:58Z

Change so kosli snapshot s3 no longer lays bucket objects out under their keys on the operator's filesystem. Each object downloads to an anonymous temp file, is hashed and removed, and digest.VirtualDirSha256 computes the fingerprint from the (key, sha256) pairs, reproducing what digest.DirSha256 gives the same tree on disk. Every existing fingerprint is preserved (TestPinnedFingerprints, TestMatchesAttestedDirectory), a root .kosli_ignore is honoured exactly as on disk (TestMatchesDirSha256 materialises the tree and compares), and no object is ever dropped silently.

Object keys are checked by one platform-independent rule: a .. segment, two keys folding onto one path, an object that is also a prefix, and malformed listings are errors that name every key involved. Keys that only an operating system would reject, such as CON, colons, backslashes or over-long components, snapshot again. The #1155 filesystem safeguards are deleted with the layout they protected.

The ADR in docs/adr/ records the decision, the compatibility contract, the rejected CRC64 alternative and the consequences. Parallel downloads with a byte budget follow in #1167, which stacks on this branch.

Replaces #1178, whose review rounds are folded into the fix(snapshot s3): harden… commit here. Closes #1155's follow-up; related #1069.

Checklist

Docs are autogenerated from CLI help — make sure the help text reflects your changes
Helm chart (charts/k8s-reporter/) updated, if needed. Note: these changes live in a separate PR
Terraform provider and related changes (terraform-provider-kosli, terraform-aws-evidence-reporter, terraform-aws-kosli-reporter) updated, if needed

claude · 2026-09-14T20:04:53Z

Claude finished @mbevc1's task in 2m 54s —— View job

Follow-up review complete

Reviewed the diff introduced by the latest commits (bd8ddc65^..7ab9a92b: bd8ddc65, 404b1985, fea1ff8e, 7c966db5, 7ab9a92b) against the five previous findings.

Fetch existing review comments/findings
Diff the fix/docs commits (BEFORE was unreachable — branch was rebased — so scoped to the 5 newest commits per the fallback rule)
Assess each previous finding
Check for new bugs/regressions in the changed hunks
Post summary

Finding status:

internal/aws/aws.go — seen duplicate-key map scaling with the whole bucket instead of the filtered result → RESOLVED in fea1ff8e. The check now runs after the folder-marker and shouldExcludePath skips, bounded by the filtered set; comment and TestAListingThatRepeatsAKeyIsAnError updated to match.
internal/aws/aws.go:637 — FilesNeedingContent/VirtualDirSha256 each rebuild the tree and re-resolve ignore rules → NOT ADDRESSED (deliberately deferred by the author to a follow-up PR).
internal/digest/virtualglob.go — **/x comment describing the wrong mechanism → RESOLVED in 7c966db5. The comment (and the matching ADR §4 line) now correctly describes the spelling mismatch (tree//x vs. cleaned tree/x) and the directory-survives/contents-excluded case; a corresponding TestMatchesDirSha256 row ("a rule that leaves the tree and names the root") was added.
docs/adr/...md — memory trade-off missing from Consequences → RESOLVED in 7ab9a92b. A consequence line now quantifies the per-object memory cost and the million-object figure as the bound for snapshot s3: parallel downloads with a bytes-in-flight budget and --download-* flags #1167.
internal/aws/s3_keys.go:66 — unbounded accumulation of rejected-key problem strings → NOT ADDRESSED (deliberate; author's rationale is that the run already holds objects/keys for every listed key on this path, so it's the same order of memory and not worth the extra code).

New issues in the changed hunks: none found. The rework (repeated-key check ordering, newest.IsZero() guard for listings with no timestamps, escapesVirtualRoot for rules that leave the tree and re-enter via the literal tree name, protectedVirtualPath now checking the ignore-file entry isn't a directory, bounded collision reporting in s3_keys.go) is each covered by a new, focused test (TestAListingWithoutModificationTimesDoesNotPanic, TestAListingEntryWithoutAKeyIsAnError, TestADirectoryNamedLikeTheIgnoreFileIsNotProtected, TestVirtualPathsForS3KeysCapsTheKeysNamedPerCollision, the new TestMatchesDirSha256/TestMalformedRuleIsAnErrorOnBothSides rows), and I couldn't find a case these miss.

All 5 review threads are already marked resolved on GitHub, consistent with the above.

Record the decision to stop laying bucket objects out by key on the operator's filesystem. Objects download to anonymous temp files, are hashed and deleted, and digest.VirtualDirSha256 computes the fingerprint from (key, sha256) pairs. Captures the compatibility contract, the shared key rule for content and metadata mode, the rejected CRC64 alternative, and the delivery slices.

path.Clean folds a/../b onto b silently, so the rejection has to run on the raw key. Also spell out that the rule exists for fingerprint compatibility, not safety: DirSha256 can never produce a tree holding ".", ".." or an empty name.

Name the three properties the virtual-tree change must hold, snapshot equals attestation, snapshot equals its own history, and no object is dropped silently, together with the test that holds each one.

…a filesystem VirtualDirSha256 reproduces DirSha256 from (path, sha256) pairs alone: it builds the tree, walks it in filepath.WalkDir order and hashes each entry's base name plus each file's content digest. SingleVirtualFile mirrors containsSingleFile so a one-file tree can take the FileSha256 branch. Every equivalence test materialises the tree on disk and requires DirSha256 to agree, so the two cannot drift silently. No callers yet. This is the first slice of moving kosli snapshot s3 off key-named local files (see docs/adr/20260911-s3-fingerprint-from-virtual-tree.md).

…colliding key virtualPathForS3Key turns an object key into the path it occupies in the virtual tree that will be fingerprinted. A ".." segment is rejected on the raw key, before path.Clean can fold it onto a sibling; a leading slash, "." segments and doubled slashes fold exactly as filepath.Join did, so existing fingerprints are unchanged. Nothing is created under the path, so reserved names, colons, backslashes and overlong components are ordinary names and there is no per-OS branch. virtualPathsForS3Keys validates a whole key set at once and reports every problem together: rejected keys, keys folding onto one path, and an object whose path is also a directory holding other objects, capped at ten. No callers yet; content mode switches to it in a later slice.

VirtualDirSha256 takes the rules of the tree's root .kosli_ignore and excludes what DirSha256 would exclude on disk. Rather than reimplement what the globs appear to mean, virtualFS reproduces filepathx.Glob, filepath.Glob and filepath.Walk step for step over the virtual tree, so their quirks come out identical: a literal "**/x" never matches at the root because the pieces concatenate to a double slash, "**/*.log" does, and excluding "logs/*" leaves an empty directory whose name is still hashed. Exclusion therefore happens inside the tree walk, not by filtering the file list. The root ignore file is never excluded by its own rules, as on disk. ParseIgnoreRules is extracted from excludePathsFromFile so callers that hold the file's bytes get the same reading DirSha256 gives the file. Every rule set in the equivalence test is materialised on disk and fingerprinted with DirSha256, and the virtual digest must match; rows that should change the digest also assert that they do.

…s local paths Each object now downloads to an anonymous temp file that is hashed and removed; the fingerprint comes from digest.VirtualDirSha256 over the (key, sha256) pairs, which reproduces what DirSha256 gave the same tree on disk. No key ever names a file, so traversal, overwrites, reserved names, case folding and component length limits stop being properties of this code, and the fingerprint is the same on every operating system. A root .kosli_ignore is downloaded first and its rules applied; objects the rules exclude are not downloaded at all. digest.FilesNeedingContent decides which files need a digest, and VirtualDirSha256 refuses a tree that needs a digest it was not given, so a skipped download can never leak into a fingerprint. Keys that cannot form a directory tree (a ".." segment, two keys folding onto one path, an object that is also a prefix) fail the snapshot and name every key involved. Fingerprints recorded against main are pinned before the switch and unchanged after it; a bucket built from a directory fingerprints as DirSha256 fingerprints the directory. localPathForS3Key, the O_EXCL and ENOTDIR handling and containsSingleFile are gone with their tests. The cmd/kosli TestSnapshotS3 suite needs the local Kosli server and is left to CI.

Drop references that only made sense during development: the throwaway experiment and the gitignored TODO.md. Name the tests that hold each guarantee, describe the .kosli_ignore resolution as the filepathx simulation that was actually built, mark parallel downloads as delivered separately in #1167, and correct the claim that the codebase gets smaller: the platform-dependent code is gone, but the faithful glob simulation costs more lines than the fenced layout it replaces.

… rules and malformed listings Folds the fixes found in review of the virtual-tree fingerprint into one change, so the ignore-rule mirror and the S3 listing hold up against input the bucket's writers control. Ignore-rule mirror (internal/digest): - validate a rule up front as filepath.Glob does, on the first "**" piece only, so a malformed rule fails on both sides and "nonexistent/**/a[" stays a no-op on both - bound glob recursion and virtual path depth at filepath's separator limit - skip a rule that leaves the tree even when it names the root ("../tree/x"), which the cleaned pattern alone let back in - do not protect a directory named .kosli_ignore from the rules, as ignoreFilePathInTree does not on disk - keep one IgnoreFileName constant; drop references to deleted functions S3 listing and fingerprint (internal/aws): - tolerate a listing entry without LastModified, and error rather than panic or send a zero timestamp when no matching object carries one - reject a listing entry with no key or a key listed twice, before the filters, rather than drop an object silently - bound the keys one collision line names, as the problem list already was, and name the count - name the object key when hashing a downloaded object fails - decide the single-object case with SingleVirtualFile on the manifest; name the bucket's ignore file only for a bad rule (ignoreRuleError) Each fix carries a test that was red before it: the DirSha256 equivalence table gains the re-entering, wildcard-dipping and directory-named-ignore-file rows, and the listing faults are pinned against the fake S3 client.

Backslash-in-rule semantics on Windows, the malformed-listing rejections, the path depth bound, and the shared cap constant.

…ach the fingerprint Checking before the filters kept one string per listed key, so a large bucket narrowed by --include-regex still held the whole listing in memory. Checked after the filters, memory is bounded by the filtered set, which is what the rest of the pipeline already holds.

The glob does find a root x, spelled with a double slash the walk's cleaned paths never equal; a root directory of that name keeps its name and loses its contents. The comment and the ADR said it never matched.

Fingerprinting a tree means holding it, so peak memory rises from one listing page to every object that survives the filters.

mbevc1 mentioned this pull request Sep 14, 2026

feat: fingerprint S3 buckets from a virtual tree #1178

Closed

3 tasks

github-actions Bot added documentation Improvements or additions to documentation go Pull requests that update go code feat labels Sep 14, 2026