test(aws): add S3 contract tests

[mbevc1]

runS3ContractTests asserts the S3 behaviours this codebase depends on:
object listing, continuation-token pagination, object download, and the
error responses for unknown buckets and missing keys.

The suite runs against both FakeS3Client (always) and real AWS (gated on
credentials), so the fake cannot drift from the real API unnoticed.

Related #758

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]

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

---

PR Review: S3 contract tests

• Static analysis (go build, go vet, go mod tidy — all clean)
• Read changed files
• Review for quality, edge cases, bugs
• Post review

---

This is a well-constructed PR. The dual-target contract suite (fake + gated real-AWS) is exactly the right shape to stop the fake from drifting, and the seam design is clean.

Static analysis

• go build ./... ✅ go vet ./... ✅ go mod tidy → no diff ✅
• I could not run the test suite in this sandbox (go test needs an approval that isn't granted to this job), so the pass/fail claims below are from reading, not execution. CI covers the run.

Strengths

• The seam split is genuinely thoughtful. Keeping ListObjectsV2 at SDK level means s3.NewListObjectsV2Paginator still runs against your interface, so the continuation-token contract is verified against real S3 rather than a hand-rolled paginator — while DownloadObject sits at operation level to avoid reimplementing the transfer manager's ranged GetObject/HeadObject. The ADR update explains this trade-off crisply.
• TestGetS3DataFromClientFilterEquivalence is a nice technique — asserting "filter the full bucket == a bucket that only ever held the subset" pins down filter semantics without hardcoding directory digests that would be brittle.
• The fake's contract-vs-fake-only test separation is disciplined — error injection and the MaxKeys/negative-token rejections are correctly kept out of runS3ContractTests (with comments explaining why), since real S3 wouldn't behave that way.
• The prior review's hardening suggestions are fully addressed — MaxKeys < 1 and negative continuation tokens are now rejected (fake_s3.go:89-107), each with a dedicated fake-only test. No infinite-loop / panic footgun remains.
• WriterAt-nil validation and the strings.HasSuffix(key, "/") folder-marker skip line up correctly with the fixtures.

Minor / optional (nothing blocking)

1. Timestamp coupling — fakeS3LastModified (fake_s3.go:18, 2024-01-15 10:30) and the earlier value in aws_test.go are the same instant by hand. The "newest timestamp" case sets lastModified explicitly so there's no hidden dependency today, but if someone edits one constant the coincidence is easy to miss. A one-line comment noting they're intentionally equal (or deriving one from the other) would future-proof it.
2. Global factory mutation in snapshotS3_test.go — SetupTest writes the package global aws.NewS3ClientFunc and TearDownTest resets it. As you noted in the thread, this is safe here (no t.Parallel(), -p=8 is process-level, and it mirrors the existing NewLambdaClientFunc seam), so I agree it's fine and consistent. Worth keeping in mind only if S3 command tests ever gain t.Parallel().
3. FakeS3Client map reads under concurrency — agree with your assessment: the fake never writes to its receiver and concurrent map reads are safe, so there's no race. No change needed.

Verdict

Looks good to merge. The observations above are all optional polish — I found no correctness bugs, and the previously-raised hardening items are resolved.
· 20260731_s3_contract_testing

[mbevc1]

Comments on the observations:

• Global factory / t.Parallel() - no t.Parallel() in either package, and -p=8 is package-level parallelism (separate processes), so the two packages can't race on the global at all. Same point as the Claude's first review; NewLambdaClientFunc and the GitHub seam share the property, so fixing only S3 would be inconsistent.
• Unsynchronised map access - I think the review is wrong and FakeS3Client never writes to its receiver (no f.X = … in the file), and concurrent reads of a Go map are safe. Similar approach is with Lambda.
• sortedKeys() per page - not needed now