scaffold: project skeleton for namespace indexer#26
Conversation
Set up the full project structure so Phase 1 implementation can start: - go.mod (go 1.23) with cobra, yaml.v3, zerolog - Domain types: Namespace, Blob, Header, SyncState (pkg/types/) - Store interface (pkg/store/) - DataFetcher interface (pkg/fetch/) - Sync coordinator with functional options (pkg/sync/) - Config loading with strict YAML parsing and validation (config/) - Cobra CLI with version and start subcommands (cmd/apex/) - justfile with build/test/lint/fmt/check recipes - golangci-lint v2 config with gofumpt formatter - GitHub Actions CI (lint + test + build) - CLAUDE.md project instructions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello @tac0turtle, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request lays the foundational groundwork for the Apex Celestia namespace indexer. It establishes the core project structure, defines key data models and service interfaces, and sets up essential tooling for development, configuration, and continuous integration. The changes provide a ready-to-use framework for subsequent implementation phases, ensuring a consistent and maintainable codebase from the outset. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR establishes the foundational architecture for Apex, a Celestia namespace indexer, introducing CI/CD infrastructure, configuration management, CLI framework via Cobra, core data type definitions, and public interfaces for data fetching and storage persistence. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Remove the static config.example.yaml and add a Generate() function to the config package plus an `init` CLI subcommand that writes a commented default config to the --config path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request sets up a comprehensive project skeleton for the namespace indexer. The structure is well-organized with clear separation of concerns into packages for config, types, storage, fetching, and synchronization. The use of cobra for the CLI, zerolog for logging, and a justfile for build automation are all solid choices. My review includes a few suggestions to improve code clarity, reduce duplication, and enhance user feedback for configuration errors. Overall, this is an excellent foundation for the project.
| format: "json" | ||
| ` | ||
|
|
||
| // Load reads and validates a YAML config file at the given path. | ||
| // Environment variable APEX_AUTH_TOKEN overrides data_source.auth_token. |
There was a problem hiding this comment.
The validate function uses a local parseNamespaceHex function, which duplicates logic for hex decoding. The types.NamespaceFromHex function in pkg/types/types.go already provides this functionality using the standard library. Using it here will remove code duplication and make the code more maintainable.
After this change, the parseNamespaceHex and hexDigit functions (lines 63-93) can be removed from this file. You will also need to add "github.com/evstack/apex/pkg/types" to the imports.
| format: "json" | |
| ` | |
| // Load reads and validates a YAML config file at the given path. | |
| // Environment variable APEX_AUTH_TOKEN overrides data_source.auth_token. | |
| for _, nsStr := range cfg.DataSource.Namespaces { | |
| if _, err := types.NamespaceFromHex(nsStr); err != nil { | |
| return fmt.Errorf("invalid namespace %q: %w", nsStr, err) | |
| } | |
| } |
| return nil | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| func versionCmd() *cobra.Command { | ||
| return &cobra.Command{ | ||
| Use: "version", | ||
| Short: "Print the version", | ||
| Run: func(_ *cobra.Command, _ []string) { | ||
| fmt.Println(version) | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| func startCmd(cfgPath *string) *cobra.Command { | ||
| return &cobra.Command{ | ||
| Use: "start", | ||
| Short: "Start the Apex indexer", | ||
| RunE: func(_ *cobra.Command, _ []string) error { | ||
| cfg, err := config.Load(*cfgPath) | ||
| if err != nil { | ||
| return fmt.Errorf("loading config: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
The startCmd function currently depends on a pointer to a local variable from rootCmd. This creates a tight coupling and can make the code harder to reason about. A more robust and self-contained approach is to retrieve the flag value directly from the cobra.Command object within the RunE function.
This suggestion refactors startCmd to be self-contained. Note that this also requires changing the call in rootCmd (line 35) from root.AddCommand(startCmd(&cfgPath)) to root.AddCommand(startCmd()).
func startCmd() *cobra.Command {
return &cobra.Command{
Use: "start",
Short: "Start the Apex indexer",
RunE: func(cmd *cobra.Command, _ []string) error {
cfgPath, err := cmd.Flags().GetString("config")
if err != nil {
return fmt.Errorf("getting config flag: %w", err)
}
cfg, err := config.Load(cfgPath)
if err != nil {
return fmt.Errorf("loading config: %w", err)
}
setupLogger(cfg.Log)
log.Info().
Str("version", version).
Str("node_url", cfg.DataSource.CelestiaNodeURL).
Int("namespaces", len(cfg.DataSource.Namespaces)).
Msg("starting apex indexer")
// TODO(phase1): wire store, fetcher, and sync coordinator.
log.Info().Msg("apex indexer is not yet implemented — scaffolding only")
return nil
},
}
}| Str("version", version). | ||
| Str("node_url", cfg.DataSource.CelestiaNodeURL). | ||
| Int("namespaces", len(cfg.DataSource.Namespaces)). |
There was a problem hiding this comment.
When an invalid log level is provided in the configuration, it's silently ignored and defaults to InfoLevel. This can be confusing for the user. It would be better to log a warning message indicating that the provided level was invalid and that a default is being used.
| Str("version", version). | |
| Str("node_url", cfg.DataSource.CelestiaNodeURL). | |
| Int("namespaces", len(cfg.DataSource.Namespaces)). | |
| if err != nil { | |
| level = zerolog.InfoLevel | |
| log.Warn().Err(err).Str("provided_level", cfg.Level).Msg("invalid log level specified, defaulting to 'info'") | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (13)
justfile (3)
7-8: Consider adding-trimpathfor reproducible builds.Without
-trimpath, the local filesystem paths of the build machine are embedded in stack traces and the binary itself. This is a minor security/reproducibility gap.♻️ Proposed fix
- go build -ldflags '{{ldflags}}' -o bin/apex ./cmd/apex + go build -trimpath -ldflags '{{ldflags}}' -o bin/apex ./cmd/apex🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@justfile` around lines 7 - 8, The build recipe currently runs "go build -ldflags '{{ldflags}}' -o bin/apex ./cmd/apex" and should include the Go -trimpath flag to remove local filesystem paths for reproducible builds; update the build target in the justfile to invoke go build with -trimpath (e.g., add -trimpath before or after -ldflags) so the compiled binary and stack traces no longer embed local paths.
34-35:checksilently repairs go.mod/go.sum drift instead of detecting it.When used as a CI gate (
just check), runningtidyfirst means a stale go.mod/go.sum will be silently fixed and the check will still pass, masking dependency drift. CI should verify no changes rather than apply them.♻️ Proposed fix — add a drift-detection step in CI or as a dedicated recipe
Add a separate
tidy-checkrecipe that can be used in CI:# Tidy dependencies tidy: go mod tidy +# Verify go.mod/go.sum are tidy (for CI) +tidy-check: + go mod tidy + git diff --exit-code go.mod go.sum # Run all checks (CI equivalent) -check: tidy lint test build +check: tidy-check lint test build🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@justfile` around lines 34 - 35, The current just recipe named check runs tidy before tests which silently fixes go.mod/go.sum drift instead of failing CI; update the task definitions so that check no longer auto-fixes drift but uses a detection-only step: add a new tidy-check recipe (and have check call tidy-check instead of tidy) that detects drift (for example by running go mod tidy in a temporary copy or running go mod tidy && git diff --exit-code or comparing output) and exits non-zero if any changes would be made; reference the existing "check" recipe and the "tidy" action so you replace the auto-fixing invocation with a detection-only "tidy-check" invocation.
10-12: Consider adding an explicit-timeoutto the test target.Go's default timeout is 10 minutes, which may silently stall CI on a deadlocked test. An explicit value makes the intent clear and keeps CI pipelines bounded.
♻️ Proposed fix
- go test -race -count=1 ./... + go test -race -count=1 -timeout 5m ./...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@justfile` around lines 10 - 12, The test target currently runs "go test -race -count=1 ./..." without an explicit timeout; update the "test" justfile target to add a -timeout flag (for example -timeout=5m or your preferred bound) so the command becomes "go test -race -count=1 -timeout=5m ./..." to ensure CI won't hang indefinitely..github/workflows/ci.yml (1)
20-20: Prefergo-version-file: go.modover the hardcoded Go version.The version
"1.23"is repeated across all three jobs. Ifgo.modis updated to a new Go version, this must be changed in three places. Usinggo-version-file: go.modkeeps CI and the module in sync automatically.♻️ Proposed refactor (same change applies to all three jobs)
- uses: actions/setup-go@v5 with: - go-version: "1.23" + go-version-file: go.modAlso applies to: 32-32, 42-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml at line 20, Replace the hardcoded go-version entries with go-version-file: go.mod in each workflow job so the Go toolchain used by Actions is read from your module file rather than duplicated; specifically, update the go-version key occurrences (the runner setup step that currently uses go-version: "1.23") to use go-version-file: go.mod across all three jobs so CI automatically follows the version declared in go.mod.config/load.go (2)
63-93:parseNamespaceHex/hexDigitduplicateNamespaceFromHexfrompkg/types/types.goand re-implement stdlibencoding/hex.Since
validateonly needs the error (line 55:_, err := parseNamespaceHex(ns)), the simplest fix is to calltypes.NamespaceFromHexdirectly. If aconfig→typesimport is undesirable, at minimum replace the custom hex digit loop withencoding/hex.DecodeString.♻️ Option A — reuse types.NamespaceFromHex (preferred)
import ( "bytes" "fmt" "os" + + "github.com/evstack/apex/pkg/types" "gopkg.in/yaml.v3" )for _, ns := range cfg.DataSource.Namespaces { - if _, err := parseNamespaceHex(ns); err != nil { + if _, err := types.NamespaceFromHex(ns); err != nil { return fmt.Errorf("invalid namespace %q: %w", ns, err) } }Then delete
parseNamespaceHexandhexDigitentirely.♻️ Option B — use encoding/hex (if types import is unwanted)
+import "encoding/hex" -func parseNamespaceHex(s string) ([29]byte, error) { - var ns [29]byte - if len(s) != 58 { - return ns, fmt.Errorf("expected 58 hex chars (29 bytes), got %d", len(s)) - } - for i := range 29 { - hi, err := hexDigit(s[2*i]) - ... - } - return ns, nil -} - -func hexDigit(c byte) (byte, error) { ... } +func parseNamespaceHex(s string) ([29]byte, error) { + var ns [29]byte + b, err := hex.DecodeString(s) + if err != nil { + return ns, err + } + if len(b) != 29 { + return ns, fmt.Errorf("expected 29 bytes, got %d", len(b)) + } + copy(ns[:], b) + return ns, nil +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/load.go` around lines 63 - 93, Replace the custom parseNamespaceHex and hexDigit implementations with the existing canonical implementation: call types.NamespaceFromHex(...) from where parseNamespaceHex is used (the validate path that currently does "_, err := parseNamespaceHex(ns)"), and remove parseNamespaceHex and hexDigit entirely; if you must avoid importing pkg/types, instead replace the manual loop with encoding/hex.DecodeString(s) to decode the hex and then copy/convert to the [29]byte form before returning the same error behavior. Ensure references to parseNamespaceHex and hexDigit are updated to the chosen replacement (types.NamespaceFromHex or encoding/hex.DecodeString).
39-61: Consider validatinglog.levelandlog.formatagainst their known values.An invalid value (e.g.,
level: verbose) passes validation but will likely cause a zerolog parse error at startup. Adding an allowlist check here produces a clearer error message than a downstream panic.🛡️ Suggested addition
+ validLevels := map[string]bool{ + "trace": true, "debug": true, "info": true, + "warn": true, "error": true, "fatal": true, "panic": true, + } + if !validLevels[cfg.Log.Level] { + return fmt.Errorf("log.level %q is invalid; must be one of trace/debug/info/warn/error/fatal/panic", cfg.Log.Level) + } + if cfg.Log.Format != "json" && cfg.Log.Format != "console" { + return fmt.Errorf("log.format %q is invalid; must be json or console", cfg.Log.Format) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/load.go` around lines 39 - 61, The validate(cfg *Config) function currently misses checks for logging configuration; add allowlist validation for cfg.Log.Level and cfg.Log.Format inside validate so invalid values produce clear errors. Check cfg.Log.Level against accepted levels (e.g., debug, info, warn, error) and cfg.Log.Format against allowed formats (e.g., json, console) and return fmt.Errorf with descriptive messages (e.g., "log.level must be one of ...", "log.format must be one of ...") if they are invalid; place these checks near the other cfg.Sync/... validations in validate to surface bad log settings early.go.mod (1)
16-16:golang.org/x/sys v0.12.0is significantly outdated.
v0.12.0dates back to 2023. Runninggo get golang.org/x/sys@latestandgo mod tidywill pull in a much newer version, which may include security patches and compatibility fixes for Go 1.23.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 16, The go.mod entry for the module golang.org/x/sys is pinned to an old version (v0.12.0); update it to the latest compatible release by running `go get golang.org/x/sys@latest` and then `go mod tidy`, which will update the golang.org/x/sys version in go.mod and clean up dependencies; ensure the resulting go.mod shows the new version for golang.org/x/sys and commit the updated go.mod and go.sum.pkg/sync/coordinator.go (2)
23-35: Option setters accept invalid values without validation.
WithBatchSize(0)orWithConcurrency(-1)silently override the safe defaults. Add guard rails consistent with thevalidate()checks inconfig/load.go.🛡️ Suggested guards
func WithBatchSize(n int) Option { - return func(c *Coordinator) { c.batchSize = n } + return func(c *Coordinator) { + if n > 0 { + c.batchSize = n + } + } } func WithConcurrency(n int) Option { - return func(c *Coordinator) { c.concurrency = n } + return func(c *Coordinator) { + if n > 0 { + c.concurrency = n + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/sync/coordinator.go` around lines 23 - 35, The option setters currently allow invalid values; update WithBatchSize, WithConcurrency, and WithStartHeight to validate inputs before mutating Coordinator: in WithBatchSize only set c.batchSize when n > 0 (otherwise leave the existing/default value), in WithConcurrency only set c.concurrency when n > 0, and in WithStartHeight only set c.startHeight when h > 0 (or follow the same non-zero semantics used by validate() in config/load.go); mirror the guard logic used in config/load.go so these Option funcs become no-ops on invalid input rather than silently overriding safe defaults on Coordinator.
1-1: Consider renaming packagesyncto avoid potential name shadowing in Phase 1 implementation.The package
syncinpkg/sync/coordinator.gowill conflict with stdlibsynconce the coordinator uses concurrency primitives (Mutex, WaitGroup, etc.). Although no files currently import this package or use stdlibsyncalongside it, proactive renaming tocoordinator,syncer, orsyncpkgeliminates friction later.If renaming is deferred, aliasing the stdlib import (
import s "sync") is a simple workaround when collisions arise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/sync/coordinator.go` at line 1, The package is named "sync" which will collide with the standard library "sync" once you add concurrency primitives; rename the package declaration in coordinator.go from "sync" to a non-conflicting name such as "coordinator" (or "syncer"/"syncpkg") and update any files that import this package to use the new package name, ensuring go.mod/go build is updated; if you prefer not to rename now, instead document and use an alias for the stdlib (e.g., import s "sync") in files that need both, but the recommended fix is to change the package declaration and adjust all import paths/usages accordingly.pkg/store/store.go (1)
21-22:GetSyncStatereturns a value type; "store is empty" is ambiguous.
GetBlobandGetHeaderreturn pointers so callers can checknilfor "not found".GetSyncStatereturnstypes.SyncStatusby value, so a fresh store with no persisted state cannot be distinguished from a store that genuinely hasState == Initializing. Implementors need guidance on whether to return anerrors.Is-able sentinel error (e.g.,ErrNotFound) or to always return a valid default status.A consistent approach would be to either:
- return
(*types.SyncStatus, error)(pointer, nil if absent), or- document that a zero-value
SyncStatus{}means "not yet persisted."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/store/store.go` around lines 21 - 22, Change GetSyncState to return a pointer so callers can distinguish "not found" from a valid zero-value: update the interface signature GetSyncState(ctx context.Context) (*types.SyncStatus, error) (leave SetSyncState as-is), add or reuse a sentinel error (e.g., ErrNotFound) and ensure implementations return (nil, ErrNotFound) when no persisted state exists and (*status, nil) when present; update all callers of GetSyncState and concrete implementations to handle the pointer return and the ErrNotFound case accordingly.cmd/apex/main.go (1)
76-85: Consider making the log output destination explicit for both formats.
setupLoggerwrites console logs toos.Stderrbut leaves JSON logs at zerolog's default (os.Stdout). This is likely intentional (structured logs to stdout for aggregation, human-readable to stderr), but the asymmetry is implicit. Explicitly setting the output for JSON mode makes the intent clear and avoids surprises if the zerolog default ever changes.♻️ Proposed refactor
func setupLogger(cfg config.LogConfig) { level, err := zerolog.ParseLevel(cfg.Level) if err != nil { level = zerolog.InfoLevel } zerolog.SetGlobalLevel(level) if cfg.Format == "console" { log.Logger = log.Output(zerolog.ConsoleWriter{Out: os.Stderr}) + } else { + log.Logger = log.Output(os.Stdout) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/apex/main.go` around lines 76 - 85, The setupLogger function currently only sets console output to os.Stderr (via zerolog.ConsoleWriter) while leaving JSON output implicit; update setupLogger (inspect cfg.Format and the log.Logger/zerolog.ConsoleWriter usage) to explicitly set log output for both formats—configure console format to write to os.Stderr and configure JSON/default format to write to os.Stdout—so intent is explicit and not dependent on zerolog defaults.pkg/fetch/fetcher.go (1)
14-14: DocumentSubscribeHeaderschannel lifecycle contract.The returned channel's closure semantics are unspecified: implementors need to know whether the channel is closed on context cancellation, on
Close(), or both. Without this contract, callers can't safely range over the channel (risk of goroutine leak or panic on closed-channel send).Suggested doc addition:
- SubscribeHeaders(ctx context.Context) (<-chan *types.Header, error) + // SubscribeHeaders returns a channel of new block headers. The channel is + // closed when ctx is cancelled or Close is called. Callers should drain + // the channel after cancellation to avoid blocking senders. + SubscribeHeaders(ctx context.Context) (<-chan *types.Header, error)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fetch/fetcher.go` at line 14, Add a clear doc comment for SubscribeHeaders in pkg/fetch/fetcher.go that specifies the channel lifecycle: state that SubscribeHeaders(ctx context.Context) returns a receive-only channel of *types.Header which will be closed by the implementor when the provided context is canceled or when the fetcher’s Close() is invoked (whichever happens first), and that implementors MUST stop sending on the channel after closing it and callers may safely range over the channel until it is closed; reference the SubscribeHeaders method name, the context.Context parameter, the *types.Header element type, and the Close() lifecycle trigger in the comment so implementors and callers have an explicit contract.config/config.go (1)
37-39: Type-level safety improvement: consider usinguintforBatchSizeandConcurrency.While validation in
config/load.go(lines 92-96) already guards against zero and negative values, usinguintwould enforce this constraint at the type level, eliminating the need for runtime validation checks. This is a type-safety refactor rather than a bug fix.type SyncConfig struct { StartHeight uint64 `yaml:"start_height"` - BatchSize int `yaml:"batch_size"` - Concurrency int `yaml:"concurrency"` + BatchSize uint `yaml:"batch_size"` + Concurrency uint `yaml:"concurrency"` }
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.golangci.yml:
- Around line 17-20: Update the .golangci.yml config to remove the deprecated
issues.exclude-dirs key and replace it with the v2 format by adding
linters.exclusions.paths (or the equivalent top-level v2 exclusions key) for
path-based linter exclusions; also remove the non-existent "submit" entry from
the exclusion list and keep only valid directories such as "vendor" under the
new key so golangci-lint v2 picks up the exclusions correctly.
- Around line 22-24: The top-level key linters-settings is for golangci-lint v1
and is ignored in v2, so move the gocyclo configuration under the new
linters.settings path: remove the top-level linters-settings block and add the
gocyclo setting as linters.settings.gocyclo.min-complexity: 15 (keeping the
gocyclo and min-complexity symbols) so golangci-lint v2 picks up the intended
threshold.
In `@CLAUDE.md`:
- Around line 20-28: The fenced code block containing the architecture directory
listing (the block starting with the line "cmd/apex/ CLI entrypoint
(cobra)") is missing a language specifier causing a markdownlint MD040 warning;
edit that fenced block and add the language tag "text" after the opening
backticks (i.e., change ``` to ```text) so the directory listing is treated as
plain text and the warning is silenced.
In `@config/config.go`:
- Line 21: Remove the YAML tag from the AuthToken field so it cannot be
populated from disk-based configs: edit the AuthToken field on the config struct
(e.g., AuthToken on config.Config in config/config.go) to drop
`yaml:"auth_token"`, and keep/ensure the existing environment-based loader in
load.go continues to populate AuthToken from APEX_AUTH_TOKEN; also update any
tests or callers that assumed YAML unmarshalling of auth_token to use the
env-var loader instead.
In `@pkg/store/store.go`:
- Line 13: GetBlobs lacks documented range semantics for endHeight which can
lead to inconsistent implementations; update the GetBlobs declaration comment to
state explicitly whether endHeight is inclusive or exclusive (pick one and be
explicit — e.g., "endHeight is inclusive: return blobs with height h where
startHeight <= h <= endHeight") and require all implementations of GetBlobs and
callers to follow that convention; search for the GetBlobs method, implementing
types (store implementations), and any callers to ensure their SQL/loop logic
matches the documented inclusive/exclusive choice and adjust comparisons/queries
accordingly.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 20: Replace the hardcoded go-version entries with go-version-file: go.mod
in each workflow job so the Go toolchain used by Actions is read from your
module file rather than duplicated; specifically, update the go-version key
occurrences (the runner setup step that currently uses go-version: "1.23") to
use go-version-file: go.mod across all three jobs so CI automatically follows
the version declared in go.mod.
In `@cmd/apex/main.go`:
- Around line 76-85: The setupLogger function currently only sets console output
to os.Stderr (via zerolog.ConsoleWriter) while leaving JSON output implicit;
update setupLogger (inspect cfg.Format and the log.Logger/zerolog.ConsoleWriter
usage) to explicitly set log output for both formats—configure console format to
write to os.Stderr and configure JSON/default format to write to os.Stdout—so
intent is explicit and not dependent on zerolog defaults.
In `@config/load.go`:
- Around line 63-93: Replace the custom parseNamespaceHex and hexDigit
implementations with the existing canonical implementation: call
types.NamespaceFromHex(...) from where parseNamespaceHex is used (the validate
path that currently does "_, err := parseNamespaceHex(ns)"), and remove
parseNamespaceHex and hexDigit entirely; if you must avoid importing pkg/types,
instead replace the manual loop with encoding/hex.DecodeString(s) to decode the
hex and then copy/convert to the [29]byte form before returning the same error
behavior. Ensure references to parseNamespaceHex and hexDigit are updated to the
chosen replacement (types.NamespaceFromHex or encoding/hex.DecodeString).
- Around line 39-61: The validate(cfg *Config) function currently misses checks
for logging configuration; add allowlist validation for cfg.Log.Level and
cfg.Log.Format inside validate so invalid values produce clear errors. Check
cfg.Log.Level against accepted levels (e.g., debug, info, warn, error) and
cfg.Log.Format against allowed formats (e.g., json, console) and return
fmt.Errorf with descriptive messages (e.g., "log.level must be one of ...",
"log.format must be one of ...") if they are invalid; place these checks near
the other cfg.Sync/... validations in validate to surface bad log settings
early.
In `@go.mod`:
- Line 16: The go.mod entry for the module golang.org/x/sys is pinned to an old
version (v0.12.0); update it to the latest compatible release by running `go get
golang.org/x/sys@latest` and then `go mod tidy`, which will update the
golang.org/x/sys version in go.mod and clean up dependencies; ensure the
resulting go.mod shows the new version for golang.org/x/sys and commit the
updated go.mod and go.sum.
In `@justfile`:
- Around line 7-8: The build recipe currently runs "go build -ldflags
'{{ldflags}}' -o bin/apex ./cmd/apex" and should include the Go -trimpath flag
to remove local filesystem paths for reproducible builds; update the build
target in the justfile to invoke go build with -trimpath (e.g., add -trimpath
before or after -ldflags) so the compiled binary and stack traces no longer
embed local paths.
- Around line 34-35: The current just recipe named check runs tidy before tests
which silently fixes go.mod/go.sum drift instead of failing CI; update the task
definitions so that check no longer auto-fixes drift but uses a detection-only
step: add a new tidy-check recipe (and have check call tidy-check instead of
tidy) that detects drift (for example by running go mod tidy in a temporary copy
or running go mod tidy && git diff --exit-code or comparing output) and exits
non-zero if any changes would be made; reference the existing "check" recipe and
the "tidy" action so you replace the auto-fixing invocation with a
detection-only "tidy-check" invocation.
- Around line 10-12: The test target currently runs "go test -race -count=1
./..." without an explicit timeout; update the "test" justfile target to add a
-timeout flag (for example -timeout=5m or your preferred bound) so the command
becomes "go test -race -count=1 -timeout=5m ./..." to ensure CI won't hang
indefinitely.
In `@pkg/fetch/fetcher.go`:
- Line 14: Add a clear doc comment for SubscribeHeaders in pkg/fetch/fetcher.go
that specifies the channel lifecycle: state that SubscribeHeaders(ctx
context.Context) returns a receive-only channel of *types.Header which will be
closed by the implementor when the provided context is canceled or when the
fetcher’s Close() is invoked (whichever happens first), and that implementors
MUST stop sending on the channel after closing it and callers may safely range
over the channel until it is closed; reference the SubscribeHeaders method name,
the context.Context parameter, the *types.Header element type, and the Close()
lifecycle trigger in the comment so implementors and callers have an explicit
contract.
In `@pkg/store/store.go`:
- Around line 21-22: Change GetSyncState to return a pointer so callers can
distinguish "not found" from a valid zero-value: update the interface signature
GetSyncState(ctx context.Context) (*types.SyncStatus, error) (leave SetSyncState
as-is), add or reuse a sentinel error (e.g., ErrNotFound) and ensure
implementations return (nil, ErrNotFound) when no persisted state exists and
(*status, nil) when present; update all callers of GetSyncState and concrete
implementations to handle the pointer return and the ErrNotFound case
accordingly.
In `@pkg/sync/coordinator.go`:
- Around line 23-35: The option setters currently allow invalid values; update
WithBatchSize, WithConcurrency, and WithStartHeight to validate inputs before
mutating Coordinator: in WithBatchSize only set c.batchSize when n > 0
(otherwise leave the existing/default value), in WithConcurrency only set
c.concurrency when n > 0, and in WithStartHeight only set c.startHeight when h >
0 (or follow the same non-zero semantics used by validate() in config/load.go);
mirror the guard logic used in config/load.go so these Option funcs become
no-ops on invalid input rather than silently overriding safe defaults on
Coordinator.
- Line 1: The package is named "sync" which will collide with the standard
library "sync" once you add concurrency primitives; rename the package
declaration in coordinator.go from "sync" to a non-conflicting name such as
"coordinator" (or "syncer"/"syncpkg") and update any files that import this
package to use the new package name, ensuring go.mod/go build is updated; if you
prefer not to rename now, instead document and use an alias for the stdlib
(e.g., import s "sync") in files that need both, but the recommended fix is to
change the package declaration and adjust all import paths/usages accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cmd/apex/main.go (1)
91-101: Consider making theelsebranch (JSON) explicit for clarity.Currently, when
cfg.Formatis neither"console"nor"json"(e.g., a typo like"consol"), the logger silently falls back to JSON. This is safe but invisible to the operator. Alog.Warn()for unrecognized formats would help catch configuration mistakes early.♻️ Optional: warn on unrecognized format
func setupLogger(cfg config.LogConfig) { level, err := zerolog.ParseLevel(cfg.Level) if err != nil { level = zerolog.InfoLevel } zerolog.SetGlobalLevel(level) - if cfg.Format == "console" { + switch cfg.Format { + case "json": + // zerolog defaults to JSON; nothing to do. + case "console": log.Logger = log.Output(zerolog.ConsoleWriter{Out: os.Stderr}) + default: + log.Logger = log.Output(zerolog.ConsoleWriter{Out: os.Stderr}) + log.Warn().Str("format", cfg.Format).Msg("unrecognized log format, falling back to console") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/apex/main.go` around lines 91 - 101, In setupLogger, make the JSON branch explicit and warn on unrecognized formats: check cfg.Format with an if/else if/else (e.g., if cfg.Format == "console" { ... } else if cfg.Format == "json" { set JSON output explicitly } else { log.Warn() about unrecognized format and fall back to JSON }), referencing the existing setupLogger function and cfg.Format to locate where to add the else warning and explicit JSON handling.config/load.go (2)
20-55: Two sources of default values — consider a doc comment noting the invariant.
DefaultConfig()in config.go anddefaultConfigYAMLhere both encode defaults. SinceLoad()decodes on top ofDefaultConfig(), divergence between the two would meanapex initwrites values that don't match the actual fallback defaults. A brief comment linking the two (or a test asserting round-trip consistency) would prevent silent drift.💡 Example: round-trip consistency test
// config/load_test.go func TestDefaultConfigYAMLMatchesDefaultConfig(t *testing.T) { cfg := DefaultConfig() dec := yaml.NewDecoder(bytes.NewReader([]byte(defaultConfigYAML))) if err := dec.Decode(&cfg); err != nil { t.Fatalf("decoding defaultConfigYAML: %v", err) } want := DefaultConfig() // Compare fields that matter: if cfg.DataSource.CelestiaNodeURL != want.DataSource.CelestiaNodeURL { t.Errorf("CelestiaNodeURL: got %q, want %q", cfg.DataSource.CelestiaNodeURL, want.DataSource.CelestiaNodeURL) } // ... etc for other fields }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/load.go` around lines 20 - 55, DefaultConfig() and the defaultConfigYAML constant are two sources of truth for defaults and can drift; update the code by adding a brief doc comment near defaultConfigYAML linking it to DefaultConfig() and explaining that Load() decodes YAML on top of DefaultConfig(), and add a unit test (e.g., TestDefaultConfigYAMLMatchesDefaultConfig) that decodes defaultConfigYAML into a config struct and compares key fields against DefaultConfig() to assert round‑trip consistency; reference symbols: DefaultConfig(), defaultConfigYAML, and Load() so reviewers can find where to add the comment and test.
109-139: RemoveparseNamespaceHexandhexDigit; usetypes.NamespaceFromHexinvalidate()instead.
NamespaceFromHexalready exists inpkg/types/types.goand handles hex parsing usingencoding/hex.DecodeString. The hand-rolledparseNamespaceHexandhexDigitfunctions duplicate this logic. Update thevalidate()function at line 87–88 to calltypes.NamespaceFromHex(ns)directly, then delete both unused functions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/load.go` around lines 109 - 139, Replace the custom hex parsing with the existing helper: in validate() call types.NamespaceFromHex(ns) instead of using parseNamespaceHex, and remove the now-unused parseNamespaceHex and hexDigit functions; ensure the types package is imported and handle the returned namespace and error from types.NamespaceFromHex(ns) where parseNamespaceHex was previously used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/load.go`:
- Around line 13-18: In Generate, don't ignore non-existence errors from
os.Stat: call os.Stat(path) and if err == nil return the "already exists" error;
if err != nil use errors.Is(err, os.ErrNotExist) to detect the absent-file case
and only then proceed to call os.WriteFile; for any other err (e.g., permission
denied) return that err to the caller so the failure is clear. Ensure you import
and use errors.Is and reference the Generate function and the
os.Stat/os.WriteFile calls when making the change.
---
Nitpick comments:
In `@cmd/apex/main.go`:
- Around line 91-101: In setupLogger, make the JSON branch explicit and warn on
unrecognized formats: check cfg.Format with an if/else if/else (e.g., if
cfg.Format == "console" { ... } else if cfg.Format == "json" { set JSON output
explicitly } else { log.Warn() about unrecognized format and fall back to JSON
}), referencing the existing setupLogger function and cfg.Format to locate where
to add the else warning and explicit JSON handling.
In `@config/load.go`:
- Around line 20-55: DefaultConfig() and the defaultConfigYAML constant are two
sources of truth for defaults and can drift; update the code by adding a brief
doc comment near defaultConfigYAML linking it to DefaultConfig() and explaining
that Load() decodes YAML on top of DefaultConfig(), and add a unit test (e.g.,
TestDefaultConfigYAMLMatchesDefaultConfig) that decodes defaultConfigYAML into a
config struct and compares key fields against DefaultConfig() to assert
round‑trip consistency; reference symbols: DefaultConfig(), defaultConfigYAML,
and Load() so reviewers can find where to add the comment and test.
- Around line 109-139: Replace the custom hex parsing with the existing helper:
in validate() call types.NamespaceFromHex(ns) instead of using
parseNamespaceHex, and remove the now-unused parseNamespaceHex and hexDigit
functions; ensure the types package is imported and handle the returned
namespace and error from types.NamespaceFromHex(ns) where parseNamespaceHex was
previously used.
justfile: - add -trimpath for reproducible builds - add -timeout 5m to test recipe - add tidy-check recipe (drift detection for CI) .golangci.yml: - migrate to v2 format (linters.settings, linters.exclusions.paths) ci.yml: - use go-version-file: go.mod instead of hardcoded version config/config.go: - remove yaml tag from AuthToken (env-var only via APEX_AUTH_TOKEN) config/load.go: - remove duplicate parseNamespaceHex/hexDigit, use types.NamespaceFromHex - add log.level and log.format validation - fix Generate() to properly handle os.Stat errors (errors.Is) - add doc comment linking defaultConfigYAML to DefaultConfig() pkg/store/store.go: - GetSyncState returns *SyncStatus (pointer, nil when absent) - add ErrNotFound sentinel error - document GetBlobs range as inclusive pkg/fetch/fetcher.go: - document SubscribeHeaders channel lifecycle contract pkg/sync/coordinator.go: - rename package from sync to syncer (avoid stdlib collision) - guard WithBatchSize/WithConcurrency against non-positive values cmd/apex/main.go: - make setupLogger output explicit for both json and console go.mod: - upgrade golang.org/x/sys v0.12.0 -> v0.41.0 - go directive bumped to 1.24.0 (required by x/sys) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Resolve config flag via cmd.Flags().GetString() inside each RunE instead of passing a *string from rootCmd. Extracted shared configPath() helper. Addresses Gemini review comment on cmd/apex/main.go. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
go 1.23) with cobra, yaml.v3, zerolog dependenciesNamespace,Blob,Header,SyncState) inpkg/types/Store(pkg/store/) andDataFetcher(pkg/fetch/)Coordinatorwith functional options (pkg/sync/)config/)--config,version, andstartsubcommands (cmd/apex/)justfilewithbuild,test,lint,fmt,check,run,clean,tidyrecipesCLAUDE.mdwith build commands, architecture overview, and conventionsTest plan
just checkpasses (tidy + lint + test + build)./bin/apex versionprints commit hash./bin/apex start --config config.example.yamllogs startup message🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
version,init, andstartcommands.Chores