Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new S3-compatible API: config, SigV4 auth, SQLite-backed object store and migration, S3 service that submits Celestia commitments, HTTP server wiring into apex startup/shutdown, and comprehensive unit/integration/e2e tests. (50 words) ChangesS3 API Implementation
Sequence DiagramsequenceDiagram
participant Client
participant S3Server as S3 HTTP Server
participant Service as S3 Service
participant ObjectStore as ObjectStore (SQLite)
participant Submitter as Submitter (Celestia)
Client->>S3Server: PUT /bucket/key (SigV4, body)
activate S3Server
S3Server->>S3Server: Authenticate SigV4, validate headers
S3Server->>Service: PutObject(bucket,key,data,contentType)
deactivate S3Server
activate Service
Service->>Service: Compute SHA256, MD5(ETag), build CommitmentEnvelope
Service->>Submitter: Submit(blob)
activate Submitter
Submitter-->>Service: success
deactivate Submitter
Service->>ObjectStore: PutObject(...) (persist metadata + data)
activate ObjectStore
ObjectStore-->>Service: persisted
deactivate ObjectStore
Service-->>S3Server: ETag / 200 OK
deactivate Service
S3Server-->>Client: 200 OK (ETag)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
pthmas
changed the title
Write operations (CreateBucket, DeleteBucket, PutObject, DeleteObject) now return ErrReadOnly immediately when no submitter is wired in. Previously PutObject would silently write to SQLite with no Celestia anchor (height=0, empty commitments), producing orphaned data. ErrReadOnly maps to 405 MethodNotAllowed in the HTTP error handler. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- object.go: escape LIKE wildcards (%, _, \) in ListObjects prefix to prevent unintended pattern matching on user-supplied keys - object.go: populate Object.Namespace from o.ns on read instead of scanning per-row DB value, eliminating stale-config drift - server.go: remove dead handleBucket wrapper, route GET bucket directly to handleListObjects - server.go: clamp max-keys to 1000 per S3 spec - server.go: apply http.MaxBytesReader before reading PUT body so oversized requests are rejected at the network layer, not after buffering the full payload in memory - service.go: detect http.MaxBytesError from MaxBytesReader and map to ErrObjectTooLarge - auth.go: validate X-Amz-Date is within ±15 min to prevent replay attacks with captured signed requests - tests: update to use mockSubmitter for write ops, fix stale hardcoded SigV4 timestamp, add TestService_ReadOnly coverage Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Empty PUTs skip Celestia submission and store locally with Height=0 and no commitments. This is intentional to preserve S3 tool compatibility (e.g. folder placeholder keys like "prefix/"). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Critical fixes: - server.go: parsePath now uses r.URL.RawPath to avoid double-decoding; percent-encoded characters in keys (e.g. %2F) are preserved through path splitting and decoded per-segment - server.go: remove query-param priority over HTTP method in bucket router; DELETE/PUT/HEAD on a bucket with query params now routes correctly instead of falling through to handleListObjects - object.go: wrap DeleteBucket count check and delete in a single transaction to close TOCTOU race where a concurrent write could sneak in between the two separate queries Medium fixes: - object.go: remove redundant GetBucket call from PutObject; the SQLite FK constraint enforces bucket existence and is detected via isSQLiteFKConstraint → ErrBucketNotFound - migrations/005: drop idx_s3_objects_bucket; the composite index on (bucket, key) already covers all bucket-only lookups - service.go: validate bucket names (3-63 chars, lowercase alphanum + hyphen, no leading/trailing hyphen, not an IP address) and key length (max 1024 bytes); new ErrInvalidBucketName and ErrKeyTooLong errors map to 400 in the HTTP layer Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- auth.go: refactor authenticateRequest into focused helpers (parseAuthorizationHeader, validateCredentialScope, validateAmzDate, payloadHashFromRequest) for readability and testability - server.go: additional hardening from review - service.go: minor fix - integration_test.go: expand S3 integration coverage - object.go: additional store hardening - object_test.go: expand ObjectStore test coverage Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Celestia now serves as a verification layer, not storage. PutObject computes SHA256 of the object, builds a ~200-byte CommitmentEnvelope (bucket, key, size, sha256, etag), and submits that JSON to Celestia. Raw object data remains cached in SQLite and served from there. - Add CommitmentEnvelope struct and SHA256 field to Object - Update ObjectStore.PutObject interface to carry sha256 param - Squash migrations 004+005 into single final schema with sha256 column - Update store and all tests for new interface and envelope assertions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Verify X-Amz-Content-Sha256 header matches actual body on PutObject (SigV4 compliance; prevents body substitution after signing) - Handle content-length as canonical header for SigV4 compatibility - Check bucket existence before submitting to Celestia to avoid orphaned on-chain commitments for non-existent buckets - Add readRequestBody helper to buffer+restore body for hash check Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- ListObjects: add LIMIT maxKeys+1 to flat (no-delimiter) queries so SQLite stops scanning after the page boundary instead of reading all remaining rows - ListObjects: remove dead `key <= marker` guard (SQL already filters `key > marker`); the existing `commonPrefix <= marker` guard is correct and retained - Config: move S3 SigV4 credentials out of YAML into env vars only (APEX_S3_ACCESS_KEY_ID / APEX_S3_SECRET_ACCESS_KEY), matching the APEX_AUTH_TOKEN convention; yaml:"-" prevents yaml decode - Simplify isLoopbackBindAddr from 33 lines to 15 by removing scheme-stripping logic that doesn't apply to bind addresses - Update e2e tests to pass credentials via env vars on the subprocess - Update config load test to use t.Setenv for partial-credential check Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
S3 PutObject submits a CommitmentEnvelope JSON to Celestia, not the raw object bytes. The previous test computed the commitment of raw data and passed it to waitForIndexedBlob — that commitment would never exist on-chain so the test would always timeout. Fix: compute the same CommitmentEnvelope (version, bucket, key, content_type, size, sha256, etag) that service.go builds, marshal it, derive its blob commitment, and verify that envelope lands on-chain. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Drop Height, Namespace, Commitments from Object struct and PutObject interface; these were write-only and never surfaced in HTTP responses or used downstream - Pass pre-computed etag through PutObject to avoid double MD5 computation - Remove ns field from ObjectStore; namespace tracking was write-only - Remove ensureBucketExists one-liner wrapper; inline GetBucket at call sites - Remove dead method guards in handleCreateBucket/DeleteBucket/HeadBucket; ServeHTTP already routes by method - Extract xmlContents/xmlCommonPrefix to package level (shared by V1 and V2) - Move readRequestBody from auth.go to server.go where it is used Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
height, namespace, commitments were added speculatively but are never written or read by the Go code. Remove from schema while still in PR. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…a commitment Empty objects bypassed Celestia submission and were stored locally with no SHA256 or ETag, diverging from S3 semantics. Reject them with ErrEmptyObject (400 InvalidRequest) so every stored object has a verifiable on-chain commitment. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Importing pkg/s3 pulled in pkg/submit → Cosmos SDK gRPC code, which registered coin.proto twice alongside the e2e module's own Tastora deps. Duplicate proto registration panics at init. Fix by duplicating the CommitmentEnvelope struct locally in the test file. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- config/config.go: remove field alignment (gofumpt) - pkg/s3/types.go: remove trailing spaces in struct tag (gofumpt) - pkg/s3/auth.go: replace fmt.Sprintf with strconv.FormatInt (perfsprint) - pkg/s3/server_test.go: hardcode auth creds in test helper (unparam) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/submission_test.go (1)
299-333:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
proc.logsagainst concurrent access.
cmd.Stdout/cmd.Stderrwrite intologsfrom background goroutines whilewaitForApexHTTP,waitForS3HTTP,waitForIndexedBlob,doRPC, andStopreadproc.logs.String()before the child exits. A plainbytes.Bufferis not thread-safe for this concurrent access pattern, causing data races flagged bygo test -race.Suggested fix
+type safeBuffer struct { + mu sync.Mutex + bytes.Buffer +} + +func (b *safeBuffer) Write(p []byte) (int, error) { + b.mu.Lock() + defer b.mu.Unlock() + return b.Buffer.Write(p) +} + +func (b *safeBuffer) String() string { + b.mu.Lock() + defer b.mu.Unlock() + return b.Buffer.String() +} + type apexProcess struct { cmd *exec.Cmd done chan struct{} waitErr error - logs *bytes.Buffer + logs *safeBuffer } func startApexProcess(t *testing.T, binaryPath string, configPath string, envVars ...string) *apexProcess { t.Helper() @@ - var logs bytes.Buffer + var logs safeBuffer cmd.Stdout = &logs cmd.Stderr = &logs🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/submission_test.go` around lines 299 - 333, The bytes.Buffer in apexProcess is written concurrently by cmd.Stdout/Stderr while other helpers (waitForApexHTTP, waitForS3HTTP, waitForIndexedBlob, doRPC, Stop) read proc.logs, causing data races; fix by making access to logs thread-safe: add a sync.Mutex to apexProcess and either (a) create a small lockedWriter type whose Write method locks the mutex and forwards to the underlying bytes.Buffer and set cmd.Stdout/cmd.Stderr to that writer in startApexProcess, and expose a readLogs method on apexProcess that locks the mutex and returns logs.String(), or (b) add a read/write wrapper methods that lock around reads and writes and ensure all code (startApexProcess, Stop, waitFor..., doRPC) uses those wrappers instead of accessing proc.logs directly.
🧹 Nitpick comments (3)
config/load_test.go (1)
355-505: ⚡ Quick winConsider collapsing these S3 validation cases into one table-driven test.
These cases all follow the same write/load/assert flow with only the YAML and expected error changing. One table would keep the file shorter and make the next S3 validation case cheaper to add.
As per coding guidelines, "Use table-driven tests in Go test files."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/load_test.go` around lines 355 - 505, The four near-duplicate tests (TestLoadRejectsS3EnabledWithNonSQLiteStorage, TestLoadRejectsS3SubmissionWithoutNamespace, TestLoadRejectsInvalidS3Namespace, TestLoadRejectsPartialS3Credentials) should be collapsed into a single table-driven test (e.g., TestLoadRejectsS3Validations) that iterates cases containing: name, YAML content, optional env setup function (for the partial-credentials case), and expected error substring; for each case create a temp config file, apply env setup, call Load, and assert the error contains the expected substring (use t.Run for subtests and only call t.Parallel() where safe, avoid parallelizing the case that sets env vars). Reference the existing Load function and the four test names when locating the code to replace.pkg/store/object.go (1)
286-291: ⚡ Quick winUse typed SQLite constraint codes here instead of matching error strings.
These helpers are coupled to the exact
modernc.org/sqliteerror text. If that wording changes, bucket/object constraint violations will start leaking as generic internal errors instead ofErrBucketAlreadyExists/ErrBucketNotFound. Prefererrors.Ason the driver's error type and branch on the constraint code/subcode.As per coding guidelines,
**/*.go: Usemodernc.org/sqlitefor SQLite database operations (CGo-free implementation).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/store/object.go` around lines 286 - 291, The helpers isSQLiteUniqueConstraint and isSQLiteFKConstraint currently match error strings; change them to use errors.As to detect the modernc.org/sqlite error type (the driver's error struct) and inspect its constraint/extended code (or Code/SubCode fields) to determine UNIQUE vs FOREIGN KEY violations so that callers can reliably map to ErrBucketAlreadyExists and ErrBucketNotFound; update imports to include modernc.org/sqlite and errors, replace string.Contains checks with typed checks against the sqlite error code constants, and preserve nil-safety and behavior otherwise.pkg/s3/server_test.go (1)
34-378: 🏗️ Heavy liftCollapse the repeated HTTP cases into table-driven tests.
The coverage is solid, but most of this file repeats the same server setup, request construction, and status/body assertions in one-off tests. Converting the CRUD/auth permutations into table-driven cases will make this suite much easier to extend and keeps it aligned with the repo’s test conventions.
As per coding guidelines,
**/*_test.go: Use table-driven tests pattern for test cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/s3/server_test.go` around lines 34 - 378, Many tests in this file repeat the same setup, request construction, and assertions; convert them to table-driven tests to reduce duplication. Create one or more test tables (structs with fields like name, useAuth(bool), setupActions (e.g., call setupHTTPTestServer or setupHTTPTestServerWithAuth and run PutBucket/PutObject via store), method, path, headers, body, callHandler (ServeHTTP vs handlePutObject), expectedStatus, expectedBodySubstrings) and iterate with t.Run; reuse helpers setupHTTPTestServer, setupHTTPTestServerWithAuth, signRequest, and server.handlePutObject to perform setup and execution, and assert expectedStatus and substring assertions for responses (e.g., check "<Code>...", "ListBucketResult", ETag, Location, Content-Type" etc.) so each original test case (TestServer_ListBuckets, TestServer_CreateBucket, TestServer_CreateBucket_AlreadyExists, TestServer_DeleteBucket, TestServer_DeleteBucket_NotEmpty, TestServer_PutGetObject, TestServer_PutObject_EmptyRejected, TestServer_GetObject_NotFound, TestServer_HeadObject, TestServer_DeleteObject, TestServer_ListObjects, TestServer_HeadBucket, TestServer_HeadBucket_NotFound, TestServer_ErrorFormat, TestServer_URLDecoding, TestServer_AuthRejectsMissingAuthorization, TestServer_AuthAcceptsSignedRequest, TestServer_AuthRejectsWrongSecret, TestServer_HandlePutObjectRejectsPayloadHashMismatch) becomes an entry in one or more concise table-driven tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/apex/main.go`:
- Around line 251-262: The code starts httpSrv.ListenAndServe in a goroutine
which delays socket bind errors until after setupS3Server returns; change it to
bind the socket synchronously first by creating a net.Listener for
cfg.S3.ListenAddr, return any bind error immediately, then start
httpSrv.Serve(listener) in the goroutine and log the actual bound address
(listener.Addr().String()); reference httpSrv, cfg.S3.ListenAddr and replace the
current httpSrv.ListenAndServe usage with a net.Listen + httpSrv.Serve flow and
ensure the listener is closed on shutdown.
In `@config/load.go`:
- Around line 416-420: The pair-validation of S3 credentials runs before
checking if S3 is enabled, causing startup to fail when stray
APEX_S3_ACCESS_KEY_ID or APEX_S3_SECRET_ACCESS_KEY are present even if
s3cfg.Enabled is false; update the logic in the load/config initialization so
the check for (s3cfg.AccessKeyID == "") != (s3cfg.SecretAccessKey == "") is
performed only when s3cfg.Enabled is true (i.e., move or guard the
pair-validation after the if !s3cfg.Enabled early return or wrap it with if
s3cfg.Enabled { ... }), keeping the existing error message and using the s3cfg
symbol to locate the code to change.
In `@e2e/go.mod`:
- Line 26: Update the vulnerable gRPC module version in go.mod by changing the
dependency declaration for google.golang.org/grpc from v1.79.1 to v1.79.3 (or
any later patched release) and then run the Go module update commands (e.g., go
get google.golang.org/grpc@v1.79.3 and go mod tidy) to ensure the new version is
pulled and the lockfile is updated.
In `@e2e/s3_test.go`:
- Around line 87-197: The test currently calls the AWS SDK with
context.Background() (e.g., client.ListBuckets, CreateBucket, PutObject,
HeadObject, GetObject, ListObjectsV2, DeleteObject, DeleteBucket) which ignores
the test timeout; update each call to use the test's timeout-aware ctx (or
derive per-call contexts with a short deadline) instead of context.Background()
so each SDK operation is bounded by the test timeout; ensure you pass ctx to
every AWS call and for longer operations derive ctxWithTimeout :=
context.WithTimeout(ctx, X) and defer cancel() where appropriate.
In `@pkg/s3/auth.go`:
- Around line 197-253: The current canonicalQueryString uses r.URL.Query() which
form-decodes '+' to space; change it to parse r.URL.RawQuery directly (call
sites: where canonicalQueryString(r.URL.Query()) is invoked, pass r.URL.RawQuery
instead) and update canonicalQueryString to split the raw query on '&' and each
pair on the first '=' to obtain raw key and raw value (allow empty values),
without using url.Query() or url.Values; keep building the pairs slice, sort by
key then value, and call awsEncode on the raw key and raw value when
constructing parts so '+' remains '%2B' and SigV4 canonicalization is
RFC3986-compliant.
In `@pkg/store/object_test.go`:
- Around line 71-75: The test fixture setup currently ignores errors from
PutBucket and PutObject (calls to PutBucket(ctx, "bucket") and PutObject(ctx,
"bucket", "greeting.txt", ...)), which can mask setup failures; update each
setup to capture the returned error and fail the test immediately (use t.Fatal
or t.Fatalf) when PutBucket or PutObject returns a non-nil error so the test
stops with the real setup error; apply the same fix to the other occurrences
noted (the PutBucket/PutObject calls around lines referenced) so all fixture
writes fail fast instead of discarding errors.
---
Outside diff comments:
In `@e2e/submission_test.go`:
- Around line 299-333: The bytes.Buffer in apexProcess is written concurrently
by cmd.Stdout/Stderr while other helpers (waitForApexHTTP, waitForS3HTTP,
waitForIndexedBlob, doRPC, Stop) read proc.logs, causing data races; fix by
making access to logs thread-safe: add a sync.Mutex to apexProcess and either
(a) create a small lockedWriter type whose Write method locks the mutex and
forwards to the underlying bytes.Buffer and set cmd.Stdout/cmd.Stderr to that
writer in startApexProcess, and expose a readLogs method on apexProcess that
locks the mutex and returns logs.String(), or (b) add a read/write wrapper
methods that lock around reads and writes and ensure all code (startApexProcess,
Stop, waitFor..., doRPC) uses those wrappers instead of accessing proc.logs
directly.
---
Nitpick comments:
In `@config/load_test.go`:
- Around line 355-505: The four near-duplicate tests
(TestLoadRejectsS3EnabledWithNonSQLiteStorage,
TestLoadRejectsS3SubmissionWithoutNamespace, TestLoadRejectsInvalidS3Namespace,
TestLoadRejectsPartialS3Credentials) should be collapsed into a single
table-driven test (e.g., TestLoadRejectsS3Validations) that iterates cases
containing: name, YAML content, optional env setup function (for the
partial-credentials case), and expected error substring; for each case create a
temp config file, apply env setup, call Load, and assert the error contains the
expected substring (use t.Run for subtests and only call t.Parallel() where
safe, avoid parallelizing the case that sets env vars). Reference the existing
Load function and the four test names when locating the code to replace.
In `@pkg/s3/server_test.go`:
- Around line 34-378: Many tests in this file repeat the same setup, request
construction, and assertions; convert them to table-driven tests to reduce
duplication. Create one or more test tables (structs with fields like name,
useAuth(bool), setupActions (e.g., call setupHTTPTestServer or
setupHTTPTestServerWithAuth and run PutBucket/PutObject via store), method,
path, headers, body, callHandler (ServeHTTP vs handlePutObject), expectedStatus,
expectedBodySubstrings) and iterate with t.Run; reuse helpers
setupHTTPTestServer, setupHTTPTestServerWithAuth, signRequest, and
server.handlePutObject to perform setup and execution, and assert expectedStatus
and substring assertions for responses (e.g., check "<Code>...",
"ListBucketResult", ETag, Location, Content-Type" etc.) so each original test
case (TestServer_ListBuckets, TestServer_CreateBucket,
TestServer_CreateBucket_AlreadyExists, TestServer_DeleteBucket,
TestServer_DeleteBucket_NotEmpty, TestServer_PutGetObject,
TestServer_PutObject_EmptyRejected, TestServer_GetObject_NotFound,
TestServer_HeadObject, TestServer_DeleteObject, TestServer_ListObjects,
TestServer_HeadBucket, TestServer_HeadBucket_NotFound, TestServer_ErrorFormat,
TestServer_URLDecoding, TestServer_AuthRejectsMissingAuthorization,
TestServer_AuthAcceptsSignedRequest, TestServer_AuthRejectsWrongSecret,
TestServer_HandlePutObjectRejectsPayloadHashMismatch) becomes an entry in one or
more concise table-driven tests.
In `@pkg/store/object.go`:
- Around line 286-291: The helpers isSQLiteUniqueConstraint and
isSQLiteFKConstraint currently match error strings; change them to use errors.As
to detect the modernc.org/sqlite error type (the driver's error struct) and
inspect its constraint/extended code (or Code/SubCode fields) to determine
UNIQUE vs FOREIGN KEY violations so that callers can reliably map to
ErrBucketAlreadyExists and ErrBucketNotFound; update imports to include
modernc.org/sqlite and errors, replace string.Contains checks with typed checks
against the sqlite error code constants, and preserve nil-safety and behavior
otherwise.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cdf414fd-3ddb-4736-a4fc-4414b8032b06
⛔ Files ignored due to path filters (2)
e2e/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
cmd/apex/main.gocmd/apex/main_test.goconfig/config.goconfig/load.goconfig/load_test.goe2e/go.mode2e/s3_test.goe2e/submission_test.gogo.modpkg/s3/auth.gopkg/s3/integration_test.gopkg/s3/server.gopkg/s3/server_test.gopkg/s3/service.gopkg/s3/service_test.gopkg/s3/types.gopkg/store/migrations/004_s3_objects.sqlpkg/store/object.gopkg/store/object_test.gopkg/store/sqlite.gopkg/submit/celestia_blob.gopkg/submit/submit_test.go
- Pre-bind S3 listener socket so startup failures surface synchronously - Move S3 credential validation after the !Enabled early return - Fix SigV4 canonical query string to use RawQuery + PathUnescape, avoiding form-decoding of '+' as space (RFC3986 compliance) - Pass test context to all AWS SDK calls in e2e test - Fail fast on fixture errors in object store tests - Bump e2e grpc dependency v1.79.1 → v1.79.3
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/apex/main.go`:
- Around line 373-376: After setupS3Server returns successfully, the code must
close the running s3Srv if subsequent startup steps fail; add a deferred cleanup
that closes s3Srv on error paths (e.g., defer func() { if err != nil { _ =
s3Srv.Close() } }() or use a boolean startedOK flag and set it true only after
startup completes) immediately after calling setupS3Server so that failures in
buildCoordinatorOptions, gRPC listener bind, or in runIndexer will close the S3
server; refer to setupS3Server and the local s3Srv variable, and ensure the
defer runs before any early returns from the function.
- Around line 251-253: The listener is created with net.Listen which violates
the noctx rule; change setupS3Server to accept a context parameter (ctx) and
replace net.Listen with a context-aware call using
net.ListenConfig{}.ListenContext(ctx, "tcp", cfg.S3.ListenAddr), then update the
caller runIndexer to pass its ctx into setupS3Server (mirroring the gRPC
listener pattern used elsewhere) so the original context is threaded through.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d43f828-e459-45b7-bb56-8de608a7ccdf
⛔ Files ignored due to path filters (1)
e2e/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
cmd/apex/main.goconfig/load.goe2e/go.mode2e/s3_test.gopkg/s3/auth.gopkg/store/object_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/store/object_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- e2e/s3_test.go
- config/load.go
- pkg/s3/auth.go
- e2e/go.mod
Use ListenConfig.Listen for noctx compliance. Close s3Srv if buildCoordinatorOptions or gRPC listen fails after S3 is already serving.
runIndexer was at complexity 17 (limit 15). Extract HTTP mux + gRPC listener startup into startRPCServers so the complexity count drops to 13.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/apex/main.go (1)
219-219: 💤 Low valueRename the
logparameter to avoid shadowing the importedlogpackage.The
log zerolog.Loggerparameter shadowsgithub.com/rs/zerolog/logfor the entire function body, solog.Info()/log.Warn()/log.Error()insidesetupS3Serverand the goroutine resolve to the parameter rather than the package. It works (both exposeInfo/Warn/Error), but it's a readability footgun and inconsistent with the rest of the file which uses the packagelogdirectly. Rename tologger(matching the convention used inmaybeBackfillSourceOption).♻️ Proposed rename
-func setupS3Server(ctx context.Context, cfg *config.Config, db store.Store, blobSubmitter submit.Submitter, log zerolog.Logger) (*http.Server, error) { +func setupS3Server(ctx context.Context, cfg *config.Config, db store.Store, blobSubmitter submit.Submitter, logger zerolog.Logger) (*http.Server, error) { if !cfg.S3.Enabled { return nil, nil } if cfg.S3.AccessKeyID == "" && cfg.S3.SecretAccessKey == "" { - warnLog := log.Warn().Str("addr", cfg.S3.ListenAddr) + warnLog := logger.Warn().Str("addr", cfg.S3.ListenAddr) if isLoopbackBindAddr(cfg.S3.ListenAddr) { @@ - s3Srv := apexs3.NewServer(s3Svc, cfg.S3.Region, cfg.S3.AccessKeyID, cfg.S3.SecretAccessKey, log) + s3Srv := apexs3.NewServer(s3Svc, cfg.S3.Region, cfg.S3.AccessKeyID, cfg.S3.SecretAccessKey, logger) @@ go func() { - log.Info().Str("addr", cfg.S3.ListenAddr).Msg("S3 API server listening") + logger.Info().Str("addr", cfg.S3.ListenAddr).Msg("S3 API server listening") if err := httpSrv.Serve(lis); err != nil && !errors.Is(err, http.ErrServerClosed) { - log.Error().Err(err).Msg("S3 API server error") + logger.Error().Err(err).Msg("S3 API server error") } }()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/apex/main.go` at line 219, The parameter name log in the function setupS3Server shadows the imported package github.com/rs/zerolog/log; rename the parameter to logger to match existing conventions (e.g., maybeBackfillSourceOption) and update all usages inside setupS3Server (including any goroutines) to use logger instead of log so package-level log remains accessible and readability is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/apex/main.go`:
- Around line 364-378: openBlobSubmitter currently returns a
*submit.DirectSubmitter which can be a nil pointer that becomes a typed-nil when
boxed into submit.Submitter, causing downstream nil checks in setupS3Server and
setupAPIService to fail; fix by ensuring you never pass a typed-nil interface:
either change openBlobSubmitter to return the submit.Submitter interface (so a
disabled case returns a plain nil interface) or, if keeping the pointer return,
normalize before boxing (e.g. create a local var of type submit.Submitter and
only assign the *submit.DirectSubmitter to it when blobSubmitter != nil) and
pass that submit.Submitter to setupS3Server and setupAPIService so their nil
guards (and ErrReadOnly/ErrDisabled flows) work correctly.
---
Nitpick comments:
In `@cmd/apex/main.go`:
- Line 219: The parameter name log in the function setupS3Server shadows the
imported package github.com/rs/zerolog/log; rename the parameter to logger to
match existing conventions (e.g., maybeBackfillSourceOption) and update all
usages inside setupS3Server (including any goroutines) to use logger instead of
log so package-level log remains accessible and readability is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PutObjectsubmits a small JSONCommitmentEnvelope(~200 bytes) containing SHA-256 + ETag + object metadata to Celestia, not raw dataGetObjectserves from local cache; clients can verify authenticity by hashing the downloaded body and comparing against the on-chain SHA-256CreateBucket,DeleteBucket,PutObject,DeleteObject) returnErrReadOnlywhen no Celestia submitter is configuredparsePathusesr.URL.RawPathto avoid double URL-decoding of%2Fin object keyscanonicalQueryStringusesr.URL.RawQuery+url.PathUnescapeto treat+as literal+(RFC3986), not space — critical for SigV4 correctnessDeleteBucketwrapped in a transaction to prevent TOCTOU race between empty-check and deletemaxObjectSize= 2 MB, independent of Celestia blob size since only the envelope is submittednet.ListenConfig.Listen(ctx, ...)so startup failures surface synchronously;s3Srvclosed on all subsequent startup error pathsAPEX_S3_ACCESS_KEY_ID,APEX_S3_SECRET_ACCESS_KEY), never read from YAMLArchitecture
CommitmentEnvelope (submitted to Celestia)
{ "version": 1, "bucket": "my-bucket", "key": "path/to/file.txt", "content_type": "text/plain", "size": 1234, "sha256": "e3b0c44298fc1c149afbf4c8996fb924...", "etag": "d41d8cd98f00b204e9800998ecf8427e" }New files
pkg/s3/types.goObject,Bucket,CommitmentEnvelope,ListObjectsResultpkg/s3/service.goObjectStoreinterface,Service,validateBucketNamepkg/s3/auth.gopkg/s3/server.gopkg/store/object.goObjectStoreimplementation: upsert, list with prefix/delimiter/paginationpkg/store/migrations/004_s3_objects.sqls3_buckets+s3_objectstables, composite unique indexTest plan
just testpasses with-raceTestS3ObjectLifecyclee2e: full bucket+object lifecycle against real chain, verifiesCommitmentEnvelopeappears indexed on CelestiaTestService_PutObject_WithSubmitterverifies submitted blob is aCommitmentEnvelope, not raw dataTestObjectStore_ObjectCRUDverifies SHA-256 and ETag round-trip through SQLiteErrReadOnly413 Request Entity Too Large411 Length Required400 Bad RequestSummary by CodeRabbit
New Features
Migrations
Tests