Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughAdds CLI Docker build/prewarm targets and orchestration, mounts a persistent Go build cache and API log path in docker-compose, introduces a /health handler and route, adds structured request and DB-ping logging, updates base image pins/checksums, and adds infra tests plus docs/help text changes. ChangesCLI Docker Build Cache & Prewarm Infrastructure
Caddy & Docker-Compose Integration Tests
Application Health, Routing & Request Logging
Base Image Pinning & Checksums
Sequence DiagramsequenceDiagram
participant User
participant Make as Make (run-cli)
participant Compose as Docker Compose
participant DB as api-db
participant Runner as api-runner
participant Cache as go_build_cache
User->>Make: make run-cli
Make->>Make: detect compose command
Make->>Compose: ensure base images
Make->>Compose: start api-db (if needed)
Make->>DB: poll /health (bounded retries)
DB-->>Make: healthy / timeout
Make->>Compose: run build-cli-docker / prewarm-cli-docker
Compose->>Runner: run build step, mount Cache (/tmp/go-build)
Runner->>Cache: read/write Go cache
Runner-->>Compose: produce /app/bin/metal-cli
Make->>Compose: compose run --rm api-runner /app/bin/metal-cli
Runner-->>User: CLI output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 2/5 reviews remaining, refill in 29 minutes and 56 seconds. Comment |
![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 optimizes the local development environment by introducing Docker-based CLI binary building, Go build caching, and automated health checks for the database during the run-cli process. It also adds new tests to verify Caddy configurations and Docker Compose structure. Feedback focuses on improving the robustness of the new tests—specifically by adding bounds checks for YAML parsing and using more efficient string matching—and ensuring the Makefile build triggers correctly by including untracked files in the dependency list.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
infra/makefile/app.mk (1)
112-128:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAccept plain relative secret paths.
Lines 112-128 reject valid overrides such as
DB_SECRET_USERNAME=database/infra/secrets/pg_usernamebecause only absolute paths and.//../prefixes are treated as files. That makesmake run-clifail even when the value is a legitimate path. Treat any non-empty override as a path and validate it with-f.Suggested simplification
- case "$$secret_value" in \ - /*|./*|../*) \ - if [ ! -f "$$secret_value" ]; then \ - if [ -z "$$missing_files" ]; then \ - missing_files=" - $$secret_name ($$secret_value)"; \ - else \ - missing_files="$$missing_files\n - $$secret_name ($$secret_value)"; \ - fi; \ - fi; \ - ;; \ - *) \ - if [ -z "$$missing_files" ]; then \ - missing_files=" - $$secret_name (literal value — must be a file path)"; \ - else \ - missing_files="$$missing_files\n - $$secret_name (literal value — must be a file path)"; \ - fi; \ - ;; \ - esac; \ + if [ ! -f "$$secret_value" ]; then \ + if [ -z "$$missing_files" ]; then \ + missing_files=" - $$secret_name ($$secret_value)"; \ + else \ + missing_files="$$missing_files\n - $$secret_name ($$secret_value)"; \ + fi; \ + fi; \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/makefile/app.mk` around lines 112 - 128, The current case block rejects valid relative secret paths by only accepting values that start with /, ./, or ../; update the logic around secret_value/secret_name/missing_files so any non-empty override is treated as a file path and validated with [ -f "$$secret_value" ] (instead of the case pattern). Replace the case branches so that if secret_value is non-empty you check file existence and append to missing_files if the file is missing; otherwise treat empty values as literal/missing as before—use the existing variables secret_value, secret_name and missing_files to build the same messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@infra/caddy/caddyfile_prod_test.go`:
- Around line 35-49: The test currently checks that tokens in the required slice
exist anywhere in caddyfile, so scope the assertions to the :8443 site block:
locate the start of the ":8443 {" block in caddyfile (use strings.Index on the
literal ":8443 {"), find its matching closing brace (scan forward counting
braces to get the block substring), and then run the regexp.QuoteMeta /
regexp.MustCompile checks against that block substring instead of the whole
caddyfile; keep using the same required slice and t.Fatalf for failures so the
checks still validate presence/order within the MTLS listener block.
In `@infra/makefile/build.mk`:
- Line 25: The CLI_DOCKER_BUILD_INPUTS variable currently uses "git ls-files" so
untracked/new .go files are ignored causing bin/metal-cli to be reused
incorrectly; update CLI_DOCKER_BUILD_INPUTS to derive prerequisites from the
working tree (e.g. a recursive filesystem scan like find or Make's
wildcard/$(shell find . -name '*.go')) or add a rebuild sentinel (e.g. a FORCE
or phony dependency) so the make target that builds bin/metal-cli (and the make
run-cli path) always considers new/untracked source files; modify the variable
and the related build targets that consume CLI_DOCKER_BUILD_INPUTS (and any
cached-binary logic around bin/metal-cli) accordingly to ensure rebuilds when
local sources change.
---
Outside diff comments:
In `@infra/makefile/app.mk`:
- Around line 112-128: The current case block rejects valid relative secret
paths by only accepting values that start with /, ./, or ../; update the logic
around secret_value/secret_name/missing_files so any non-empty override is
treated as a file path and validated with [ -f "$$secret_value" ] (instead of
the case pattern). Replace the case branches so that if secret_value is
non-empty you check file existence and append to missing_files if the file is
missing; otherwise treat empty values as literal/missing as before—use the
existing variables secret_value, secret_name and missing_files to build the same
messages.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7b69d325-46ac-4c11-bbf5-7d0c2b5a6c42
📒 Files selected for processing (8)
Makefiledocker-compose.ymldocs/SETUP.mdinfra/caddy/caddyfile_prod_test.goinfra/caddy/docker_compose_test.goinfra/makefile/app.mkinfra/makefile/build.mkinfra/makefile/db.mk
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@handler/keep_alive_db.go`:
- Around line 37-47: The DB ping failure is being logged twice: once with
slog.Error and again inside endpoint.LogInternalError; remove the explicit
slog.Error call in the keep-alive handler and rely on endpoint.LogInternalError
to perform the single error logging (ensure endpoint.LogInternalError is passed
the same error and context such as "database ping failed", request_id and other
metadata if needed so no diagnostic info is lost).
In `@pkg/endpoint/server.go`:
- Around line 155-157: The handler is currently appending the raw query string
(r.URL.RawQuery) into attrs which can leak sensitive data; replace this by
parsing r.URL.Query() and either whitelist allowed parameter names or redact
values before appending to attrs (e.g., only append specific keys or append keys
with masked values like "<redacted>"); update the code around the spots
referencing r.URL.RawQuery and attrs to use the sanitized map/values instead of
the raw query string.
- Around line 107-137: statusRecorder currently embeds http.ResponseWriter but
does not forward optional interfaces, causing downstream asserts (e.g.,
http.Hijacker, http.Flusher, http.Pusher) to fail; update the statusRecorder
type (and its creation in ServeHTTP) to either (a) add wrapper methods that
implement these optional interfaces by type-asserting the embedded
ResponseWriter and forwarding calls (e.g., func (r *statusRecorder) Hijack()
(net.Conn, *bufio.ReadWriter, error) { if h, ok :=
r.ResponseWriter.(http.Hijacker); ok { return h.Hijack() } return nil, nil,
fmt.Errorf("hijack not supported") }), and similarly implement Flush(), Push(),
etc., or (b) add an Unwrap() ResponseWriter method that returns the underlying
ResponseWriter so callers can perform their own assertions; implement whichever
approach you prefer and ensure ServeHTTP still constructs rec :=
&statusRecorder{ResponseWriter: w} so the forwarding/unwrapping works for
downstream handlers like h.next.ServeHTTP(rec, r).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 076d15ae-6377-439a-b8dd-2e0d52e88b69
📒 Files selected for processing (12)
.env.exampledocker-compose.ymldocs/SETUP.mdhandler/health.gohandler/health_test.gohandler/keep_alive_db.goinfra/caddy/docker_compose_test.gometal/kernel/app.gometal/router/router.gometal/router/router_health_test.gopkg/endpoint/server.gopkg/endpoint/server_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@infra/docker/base-images/Dockerfile.builder`:
- Line 40: The Dockerfile.builder currently installs openssl directly
(openssl=3.5.6-r0), breaking reproducible builds; update generate-checksums.sh
to add "openssl-3.5.6-r0.apk" to the BUILDER_PACKAGES array, include its
checksum in the verified artifacts list, ensure the script downloads that .apk
with the other packages and runs the same checksum verification flow, and then
update Dockerfile.builder/bootstrap steps to consume the verified openssl .apk
(rather than pulling from the live index) so the bootstrap
key-generation/signing steps use the checksummed, verified package.
In `@infra/makefile/build.mk`:
- Line 25: The bin/metal-cli target only watches Go sources, so add
docker-compose.yml, infra/docker/dockerfile-api and the BASE_IMAGE_VERSION into
the CLI_DOCKER_BUILD_INPUTS and make the build invocation force image rebuild;
update the CLI_DOCKER_BUILD_INPUTS shell find to also include
"docker-compose*.yml" and "infra/docker/dockerfile-api" (or the exact Dockerfile
path) and append a token that reflects BASE_IMAGE_VERSION (e.g. include
$(BASE_IMAGE_VERSION) in the variable expansion) so changes to the base image
version change the prerequisite checksum; then change the recipe that runs
docker-compose up for the api-runner (the rule that builds the image) to call
docker-compose build or docker-compose up --build so the api-runner image is
rebuilt when those files or BASE_IMAGE_VERSION change.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 60a51cf0-5e25-48af-8cef-bd5d6435f274
📒 Files selected for processing (12)
docker-compose.ymlinfra/caddy/caddyfile_prod_test.goinfra/caddy/docker_compose_test.goinfra/docker/base-images/Dockerfile.builderinfra/docker/base-images/Dockerfile.runtimeinfra/docker/base-images/checksums/builder-aarch64.sha256infra/docker/base-images/checksums/builder-x86_64.sha256infra/docker/base-images/checksums/runtime-aarch64.sha256infra/docker/base-images/checksums/runtime-x86_64.sha256infra/docker/base-images/generate-checksums.shinfra/docker/dockerfile-apiinfra/makefile/build.mk
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 by CodeRabbit
New Features
Documentation
Tests
Chores