Skip to content

docs: narrow README claims; fail fast on missing S3 bucket#104

Merged
0lut merged 6 commits into
mainfrom
codex/readme-fact-check-audit
Jun 11, 2026
Merged

docs: narrow README claims; fail fast on missing S3 bucket#104
0lut merged 6 commits into
mainfrom
codex/readme-fact-check-audit

Conversation

@0lut

@0lut 0lut commented Jun 11, 2026

Copy link
Copy Markdown
Owner

Summary

Started as the README fact-check follow-up; now also makes S3 misconfiguration fail fast at startup. The audit artifact (docs/readme-fact-check-audit.md) is removed so this is directly mergeable.

README P1 fixes — two claims were overbroad relative to the implementation:

  • "GitHub, GitLab, Bitbucket, or any Smart HTTP Git server" → now "allowlisted HTTPS upstreams addressable as host/owner/repo (GitHub-style remotes, including top-level GitLab and Bitbucket repos)". Materializer::upstream_url only constructs https://{host}/{owner}/{repo}.git, and RepoKey::parse requires exactly three segments — no custom ports, path prefixes, or GitLab subgroups.
  • "Everything after that is served locally" → now notes pack data is served locally once warm, while ref advertisements and branch/default-branch selectors still verify refs upstream (direct-Git GET /info/refs calls upstream_refs by design).

Region docs — the Helm install examples no longer pass aws.region=us-west-2; both READMEs now explain the resolution chain (GIT_CACHE_S3_REGIONAWS_REGIONAWS_DEFAULT_REGION → SDK default chain, with IRSA injecting AWS_REGION on EKS) and that aws.region is only a fallback.

Fail-fast missing-bucket check — previously a typo'd/missing bucket let the server start and then fail every S3 call with NoSuchBucket. Now S3ObjectStore::verify_bucket_access() runs a HeadBucket during AppState::try_new_async, so the pod crashloops immediately with:

s3 bucket `<name>` does not exist; create it (the server never creates buckets) or fix GIT_CACHE_S3_BUCKET

(404 → Validation; other failures → UpstreamUnavailable mentioning credentials/endpoint.) The server deliberately does not auto-create buckets — that would need s3:CreateBucket IAM and would silently mask typos with a fresh empty cache. The sync test-only AppState::try_new path is unchanged. Covered by a new MinIO integration test minio_verify_bucket_access_round_trips (missing bucket → clear Validation error naming the bucket; existing bucket → Ok), which runs in the existing MinIO Rust CI shard.

The lower-priority audit findings (P2 "like any other HTTP remote", P3 Basic-only per-request auth, stale v2 comments in example configs) are intentionally not addressed here.

Link to Devin session: https://app.devin.ai/sessions/73b059cc0b864478861a2bda02dc1246
Requested by: @0lut

0lut and others added 4 commits June 11, 2026 14:48
@devin-ai-integration devin-ai-integration Bot changed the title docs: add README fact-check audit docs: narrow README upstream-scope and cache-locality claims Jun 11, 2026
devin-ai-integration Bot and others added 2 commits June 11, 2026 22:04
…is needed

Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
@devin-ai-integration devin-ai-integration Bot changed the title docs: narrow README upstream-scope and cache-locality claims docs: narrow README claims; fail fast on missing S3 bucket Jun 11, 2026
@0lut
0lut marked this pull request as ready for review June 11, 2026 22:32
Arc::new(
s3_store_async(bucket, &v2_s3_prefix(prefix), endpoint.as_deref()).await?,
)
let store =

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Todo - make it interface based to accomadate multiple providers here, right now it mentions S3, but it does not need to be

@0lut
0lut merged commit b5c07bf into main Jun 11, 2026
21 of 22 checks passed

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant