Conversation
WalkthroughReorients the Makefile toward local development, externalizes production build/deploy and Caddy TLS logic into new makefile modules, updates Caddy to mTLS with a relay path rewrite, and propagates host URL and CA paths via docker-compose and Dockerfile build args. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev
participant Make as Makefile
participant DC as Docker Compose
participant NPM as Node/npm
Dev->>Make: make env-fresh
Make->>NPM: rm -rf node_modules, package-lock.json<br/>npm install
Dev->>Make: make local-fresh
Make->>DC: docker compose down -v
Make->>Make: invoke local-up
Make->>DC: docker compose build --no-cache
Make->>DC: docker compose up -d
sequenceDiagram
autonumber
participant Client
participant Caddy as Caddy (web)
participant API as Upstream API
Client->>Caddy: GET /relay/generate-signature...
Note over Caddy: Match path /relay/generate-signature*
Caddy->>Caddy: rewrite -> /api/generate-signature
Note over Caddy,API: mTLS: client.pem + client.key<br/>Trust: ca.pem
Caddy->>API: Proxy request (TLS client auth)
API-->>Caddy: Response
Caddy-->>Client: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches🧪 Generate unit tests
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
25-30: env-fresh fails if package-lock.json is absent.Use -f to avoid brittle deletes.
Apply:
- rm $(ROOT_PATH)/package-lock.json + rm -f $(ROOT_PATH)/package-lock.jsonOptionally prefer deterministic installs with npm ci (keeping the lockfile) if that better fits your workflow.
🧹 Nitpick comments (12)
build/makefiles/build.mk (3)
7-11: Pull latest bases in CI builds.Add
--pull alwaysso CI picks up patched base layers (Node/Caddy), improving supply-chain hygiene.- @docker compose --profile prod build + @docker compose --profile prod build --pull always
12-17: Release flow lacks registry auth and multi-arch context.If GHCR auth isn’t already handled,
docker pushmay fail. Also consider multi-arch if needed.
- Precede with
echo "$GHCR_TOKEN" | docker login ghcr.io -u "$GHCR_USER" --password-stdin.- Optionally switch to
buildx bakewith--platform linux/amd64,linux/arm64to publish a manifest list.
19-21: Ensure .env presence and prod profile coverage.
--env-file ./.envis hardcoded; fail fast if missing and consider--waitif your compose version supports it for better deploy feedback.build-deploy: - docker compose --env-file ./.env --profile prod up -d --no-build + test -f ./.env + docker compose --env-file ./.env --profile prod up -d --no-buildcaddy/WebCaddyfile.internal (2)
12-17: mTLS upstream config looks right; consider setting SNI and avoiding deprecated CA knobs.
- Good:
tls_client_authfor presenting a client cert, andtls_trust_pool file …to trust the upstream CA.tls_trust_poolis the modern replacement for deprecatedtls_trusted_ca_certs. (scwy.net)- Optional: If the upstream cert’s SAN doesn’t include
oullin_proxy_prod, settls_server_nameaccordingly to avoid SNI/cert mismatch. (scwy.net)transport http { tls # use HTTPS to upstream tls_client_auth /etc/caddy/mtls/client.pem /etc/caddy/mtls/client.key tls_trust_pool file /etc/caddy/mtls/ca.pem # trust the API's CA + # tls_server_name api.internal.example # set if SAN != oullin_proxy_prod }
3-6: Nit: group static directives together for readability.Minor readability tweak: keep
root,try_files, andfile_serveradjacent within the fallbackhandleto convey intent.No functional change; see the refactor diff above.
docker-compose.yml (2)
9-16: Build args need guardrails; confirm intended use at build vs. runtime.
- VITE_HOST_URL and ENV_API_LOCAL_DIR must be set in .env; otherwise they resolve to empty and builds may behave unexpectedly.
- Build args aren’t available at runtime. If these are needed in the container at runtime, mirror them under environment/env_file instead of only build.args.
Would you like a follow-up diff to promote these to runtime envs where needed?
22-35: If WebCaddyfile.internal references /etc/caddy/mtls, caddy-local will fail without the cert mounts.caddy-local does not mount client.pem, client.key, or ca.pem. If the local Caddyfile requires them, add the same mounts used by prod.
Apply this diff if mTLS is required in local too:
caddy-local: @@ volumes: - ./dist:/usr/share/caddy - ./caddy/WebCaddyfile.internal:/etc/caddy/Caddyfile + - ./caddy/mtls/client.pem:/etc/caddy/mtls/client.pem:ro + - ./caddy/mtls/client.key:/etc/caddy/mtls/client.key:ro + - ${ENV_API_LOCAL_DIR}/ca.pem:/etc/caddy/mtls/ca.pem:robuild/makefiles/caddy.mk (3)
6-6: Harden shell: add pipefail and restrictive umask before key generation.Prevents silent failures in pipelines and ensures private key perms from the start.
Apply:
- @set -eu; \ + @set -euo pipefail; \ + umask 077; \
18-23: Client cert lacks common key usage constraints.Add basicConstraints and keyUsage alongside extendedKeyUsage for better TLS client cert interoperability.
Apply:
-out "$$WEB_MTLS/client.pem" -days 1095 -sha256 \ - -extfile <(printf "extendedKeyUsage=clientAuth") >/dev/null 2>&1; \ + -extfile <(printf "basicConstraints=CA:FALSE\nkeyUsage=Digital Signature,Key Encipherment\nextendedKeyUsage=clientAuth") >/dev/null 2>&1; \
33-36: Align Caddy image tag with docker-compose (alpine vs non-alpine).Use a consistent tag to avoid version drift during validation vs runtime.
Apply:
- caddy:2.10.0 caddy validate --config /etc/caddy/Caddyfile + caddy:2.10.0-alpine caddy validate --config /etc/caddy/CaddyfileMakefile (2)
1-1: Add conventional phony targets (all/clean/test) to satisfy static analysis and common tooling.Keeps checkmake happy and standardizes entry points.
Apply:
-.PHONY: format env-fresh lint-fix local-fresh local-up +.PHONY: format env-fresh lint-fix local-fresh local-up all clean testAnd append these convenience targets:
+all: local-up +clean: local-fresh +test: lint-fix
39-41: Avoid unconditional --no-cache on local builds.It slows inner-loop dev. Make it opt-in (e.g., NO_CACHE=1) or at least add --pull.
Apply one of:
- docker compose --profile local build --no-cache + docker compose --profile local build --pullor
- docker compose --profile local build --no-cache + @if [ "$(NO_CACHE)" = "1" ]; then docker compose --profile local build --no-cache; else docker compose --profile local build; fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
-
Makefile(2 hunks) -
build/makefiles/build.mk(1 hunks) -
build/makefiles/caddy.mk(1 hunks) -
caddy/WebCaddyfile.internal(1 hunks) -
docker-compose.yml(2 hunks) -
docker/Dockerfile(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🔇 Additional comments (2)
build/makefiles/build.mk (1)
3-5: WEB_TAG aligns with docker-compose image — no change required.docker-compose.yml defines image: web-prod-builder:${TAG:-latest} (docker-compose.yml:43) and build/makefiles/build.mk sets WEB_TAG=web-prod-builder — alignment confirmed. Ensure docker tag commands include the tag (e.g.
$(WEB_TAG):$ (BUILD_VERSION)).docker-compose.yml (1)
68-72: External network dependency.caddy_net is marked external; ensure it exists in the target environment to avoid “network not found” at deploy time.
You can verify with: docker network inspect caddy_net
| caddy-del-certs: | ||
| @set -eu; \ | ||
| rm -f "$(CADDY_MTLS_DIR)/ca.key" "$(CADDY_MTLS_DIR)/ca.pem"; \ | ||
| printf "$(BLUE)✅ files removed from [$(NC)$(CADDY_MTLS_DIR)$(BLUE)]$(NC)\n" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
caddy-del-certs deletes CA files in the web dir, not the generated client certs.
caddy-gen-certs writes client.key/pem under $(CADDY_MTLS_DIR); removing ca.* here is a no-op and misleading. Switch to deleting client.* and provide a separate target if you need to purge CA in ENV_API_LOCAL_DIR.
Apply:
caddy-del-certs:
@set -eu; \
- rm -f "$(CADDY_MTLS_DIR)/ca.key" "$(CADDY_MTLS_DIR)/ca.pem"; \
- printf "$(BLUE)✅ files removed from [$(NC)$(CADDY_MTLS_DIR)$(BLUE)]$(NC)\n"
+ rm -f "$(CADDY_MTLS_DIR)/client.key" "$(CADDY_MTLS_DIR)/client.pem"; \
+ printf "$(BLUE)✅ client certs removed from [$(NC)$(CADDY_MTLS_DIR)$(BLUE)]$(NC)\n"
+
+# Optional: purge CA artifacts in the API MTLS dir (dangerous; keep separate)
+caddy-del-ca-certs:
+ @set -eu; \
+ API_MTLS="$(ENV_API_LOCAL_DIR)"; \
+ if [ -z "$$API_MTLS" ]; then printf "$(RED)✘ ERROR:$(NC) ENV_API_LOCAL_DIR is empty.\n"; exit 1; fi; \
+ rm -f "$$API_MTLS/ca.key" "$$API_MTLS/ca.pem" "$$API_MTLS/ca.srl"; \
+ printf "$(BLUE)✅ CA files removed from [$(NC)$$API_MTLS$(BLUE)]$(NC)\n"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| caddy-del-certs: | |
| @set -eu; \ | |
| rm -f "$(CADDY_MTLS_DIR)/ca.key" "$(CADDY_MTLS_DIR)/ca.pem"; \ | |
| printf "$(BLUE)✅ files removed from [$(NC)$(CADDY_MTLS_DIR)$(BLUE)]$(NC)\n" | |
| caddy-del-certs: | |
| @set -eu; \ | |
| rm -f "$(CADDY_MTLS_DIR)/client.key" "$(CADDY_MTLS_DIR)/client.pem"; \ | |
| printf "$(BLUE)✅ client certs removed from [$(NC)$(CADDY_MTLS_DIR)$(BLUE)]$(NC)\n" | |
| # Optional: purge CA artifacts in the API MTLS dir (dangerous; keep separate) | |
| caddy-del-ca-certs: | |
| @set -eu; \ | |
| API_MTLS="$(ENV_API_LOCAL_DIR)"; \ | |
| if [ -z "$$API_MTLS" ]; then printf "$(RED)✘ ERROR:$(NC) ENV_API_LOCAL_DIR is empty.\n"; exit 1; fi; \ | |
| rm -f "$$API_MTLS/ca.key" "$$API_MTLS/ca.pem" "$$API_MTLS/ca.srl"; \ | |
| printf "$(BLUE)✅ CA files removed from [$(NC)$$API_MTLS$(BLUE)]$(NC)\n" |
🤖 Prompt for AI Agents
In build/makefiles/caddy.mk around lines 27 to 31, the caddy-del-certs target
currently removes ca.key/ca.pem from $(CADDY_MTLS_DIR) which is incorrect and
misleading because client certs (client.key/client.pem) are generated there;
update the target to remove client.key and client.pem from $(CADDY_MTLS_DIR)
instead, and add a new make target (e.g., caddy-del-ca) that removes ca.key and
ca.pem from $(ENV_API_LOCAL_DIR) so CA purges are explicit and use the correct
directory.
| :80 { | ||
| root * /usr/share/caddy | ||
| file_server | ||
| try_files {path} /index.html | ||
|
|
||
| # Relay: browser calls this | ||
| @relay handle_path /relay/generate-signature* | ||
| root * /usr/share/caddy | ||
| file_server | ||
| try_files {path} /index.html |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Directive order will route SPA first and likely bypass the relay proxy.
try_files executes before handle blocks in Caddy’s directive order; unmatched files will be rewritten to /index.html before your @relay matcher runs, so /relay/generate-signature* may never hit reverse_proxy. Prefer an explicit fallback handle for static, with the relay handler preceding it. (notes.plc.one)
Reorder as fallback:
:80 {
- root * /usr/share/caddy
- file_server
- try_files {path} /index.html
-
- @relay path /relay/generate-signature*
- handle @relay {
- # rewrite relay path -> real API path
- uri replace ^/relay/generate-signature /api/generate-signature
-
- reverse_proxy https://oullin_proxy_prod:8443 {
- transport http {
- tls # use HTTPS to upstream
- tls_client_auth /etc/caddy/mtls/client.pem /etc/caddy/mtls/client.key
- tls_trust_pool file /etc/caddy/mtls/ca.pem # trust the API's CA
- }
- }
- }
+ @relay path /relay/generate-signature*
+ handle @relay {
+ # rewrite relay path -> real API path
+ uri replace /relay/generate-signature /api/generate-signature
+ reverse_proxy https://oullin_proxy_prod:8443 {
+ transport http {
+ tls
+ tls_client_auth /etc/caddy/mtls/client.pem /etc/caddy/mtls/client.key
+ tls_trust_pool file /etc/caddy/mtls/ca.pem
+ }
+ }
+ }
+
+ # Fallback: serve SPA
+ handle {
+ root * /usr/share/caddy
+ try_files {path} /index.html
+ file_server
+ }
}Also applies to: 7-20
🤖 Prompt for AI Agents
In caddy/WebCaddyfile.internal around lines 2-5 (also applies to lines 7-20):
the current directive order uses try_files before your relay matcher so SPA
fallback rewrites to /index.html and bypasses the /relay reverse_proxy; reorder
by placing the relay handler(s) (explicit matcher for /relay/generate-signature*
with reverse_proxy) before any static-file fallback and replace try_files with a
fallback handle for static content so unmatched requests are handled after the
relay rules; ensure the relay matcher uses a precise path matcher and that a
final handle or try_files-like fallback only rewrites to /index.html when no
earlier handles matched.
| @relay path /relay/generate-signature* | ||
| handle @relay { | ||
| reverse_proxy https://oullin_proxy_prod:8443 { | ||
| # Map relay path -> real API path | ||
| uri replace ^/relay/generate-signature /api/generate-signature | ||
| # rewrite relay path -> real API path | ||
| uri replace ^/relay/generate-signature /api/generate-signature | ||
|
|
There was a problem hiding this comment.
uri replace is not regex; the leading ^ prevents the rewrite from ever matching.
uri replace does substring replacement; use it without ^, or use uri path_regexp if you intend a regex. As written, /relay/generate-signature* won’t be rewritten to /api/generate-signature*. See Caddy docs on uri and its replace vs path_regexp modes. (scwy.net)
Fix with one of these:
Option A (simple substring):
- uri replace ^/relay/generate-signature /api/generate-signature
+ uri replace /relay/generate-signature /api/generate-signatureOption B (explicit regex):
- uri replace ^/relay/generate-signature /api/generate-signature
+ uri path_regexp ^/relay/generate-signature /api/generate-signature📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @relay path /relay/generate-signature* | |
| handle @relay { | |
| reverse_proxy https://oullin_proxy_prod:8443 { | |
| # Map relay path -> real API path | |
| uri replace ^/relay/generate-signature /api/generate-signature | |
| # rewrite relay path -> real API path | |
| uri replace ^/relay/generate-signature /api/generate-signature | |
| @relay path /relay/generate-signature* | |
| handle @relay { | |
| # rewrite relay path -> real API path | |
| uri replace /relay/generate-signature /api/generate-signature |
🤖 Prompt for AI Agents
In caddy/WebCaddyfile.internal around lines 7-11, the directive uses "uri
replace" with a regex anchor (leading ^) which prevents matching because replace
is substring-based; fix by either (A) using uri replace without the caret and
replacing "/relay/generate-signature" with "/api/generate-signature" so any
trailing characters are preserved, or (B) switch to uri path_regexp with a
proper regex like ^/relay/generate-signature(.*) and use the capture in the
replacement (e.g., /api/generate-signature{1}) to preserve the suffix.
| volumes: | ||
| - oullin_web_data:/data | ||
| - oullin_web_config:/config | ||
| - ./caddy/mtls/client.pem:/etc/caddy/mtls/client.pem:ro | ||
| - ./caddy/mtls/client.key:/etc/caddy/mtls/client.key:ro | ||
| - ${ENV_API_LOCAL_DIR}/ca.pem:/etc/caddy/mtls/ca.pem:ro | ||
| depends_on: |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Mounting ${ENV_API_LOCAL_DIR}/ca.pem is brittle without a preflight check.
This path must be absolute and the file must exist; otherwise docker compose up will error before building. Consider a preflight Make target or docs note.
Run this quick check locally:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
set -a; [ -f .env ] && source .env; set +a
test -n "${ENV_API_LOCAL_DIR:-}" && test -f "${ENV_API_LOCAL_DIR}/ca.pem" \
|| { echo "ENV_API_LOCAL_DIR or ca.pem missing"; exit 1; }Length of output: 35
Add a fail-fast preflight for ENV_API_LOCAL_DIR and ca.pem
Mounting ${ENV_API_LOCAL_DIR}/ca.pem in docker-compose.yml (lines 56–62) will make docker-compose up fail if ENV_API_LOCAL_DIR is unset or ca.pem is missing — local check returned: "ENV_API_LOCAL_DIR or ca.pem missing". Add a Make/CI preflight that verifies ENV_API_LOCAL_DIR is set to an absolute path and ${ENV_API_LOCAL_DIR}/ca.pem exists, or make the bind mount conditional; document the requirement.
🤖 Prompt for AI Agents
In docker-compose.yml around lines 56–62, the compose file blindly mounts
${ENV_API_LOCAL_DIR}/ca.pem which causes docker-compose up to fail if
ENV_API_LOCAL_DIR is unset or ca.pem is missing; add a fail-fast preflight:
create a Makefile/CI target (e.g., preflight or check-env-api-ca) that verifies
ENV_API_LOCAL_DIR is defined and absolute and that ${ENV_API_LOCAL_DIR}/ca.pem
exists, return non-zero and print a clear error if checks fail; alternatively
make the bind mount conditional (only include the ca.pem volume when
ENV_API_LOCAL_DIR is set and file exists) and update README/docs to state the
requirement for ENV_API_LOCAL_DIR to be an absolute path and for ca.pem to be
present.
| ARG VITE_HOST_URL | ||
| ARG VITE_ACCOUNT_NAME | ||
| ARG VITE_PUBLIC_KEY | ||
| ARG ENV_API_LOCAL_DIR |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Build args are declared but never used; export to ENV before build or drop them.
VITE_HOST_URL and ENV_API_LOCAL_DIR won’t affect npm run build unless you surface them as environment variables (Vite only reads envs, not Docker ARGs). Either map them to ENV before the build step or remove them to avoid confusion/cache busting with no effect.
Apply this minimal change to make them available during the build:
ARG VITE_HOST_URL
+ENV VITE_HOST_URL=${VITE_HOST_URL}
@@
ARG ENV_API_LOCAL_DIR
+ENV ENV_API_LOCAL_DIR=${ENV_API_LOCAL_DIR}If these are intentionally unused, consider deleting the ARGs.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ARG VITE_HOST_URL | |
| ARG VITE_ACCOUNT_NAME | |
| ARG VITE_PUBLIC_KEY | |
| ARG ENV_API_LOCAL_DIR | |
| ARG VITE_HOST_URL | |
| ENV VITE_HOST_URL=${VITE_HOST_URL} | |
| ARG VITE_ACCOUNT_NAME | |
| ARG VITE_PUBLIC_KEY | |
| ARG ENV_API_LOCAL_DIR | |
| ENV ENV_API_LOCAL_DIR=${ENV_API_LOCAL_DIR} |
🤖 Prompt for AI Agents
In docker/Dockerfile around lines 8 to 11, ARGs VITE_HOST_URL,
VITE_ACCOUNT_NAME, VITE_PUBLIC_KEY and ENV_API_LOCAL_DIR are declared but not
exposed as environment variables so Vite/npm build won't see them; either export
the needed ARGs to ENV before the build step (e.g., add ENV
VITE_HOST_URL=$VITE_HOST_URL and ENV ENV_API_LOCAL_DIR=$ENV_API_LOCAL_DIR — and
do the same for VITE_ACCOUNT_NAME and VITE_PUBLIC_KEY if they are required) so
they are available during RUN npm run build, or if any of these are
intentionally unused remove the corresponding ARG lines to avoid confusion and
cache effects.
1 participant
Summary by CodeRabbit
New Features
Chores