Add NGINX reverse proxy with SSL, preview routing, and Grafana integration#1515
Add NGINX reverse proxy with SSL, preview routing, and Grafana integration#1515youngchingjui merged 11 commits intomainfrom
Conversation
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on March 19. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds NGINX reverse-proxy support with Docker Compose integration, new NGINX configs (global plus site and preview rules including SSE/widget injection), and accompanying deployment and developer documentation updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (browser)
participant Nginx as NGINX proxy
participant App as App container (or preview service)
participant Host as host.docker.internal (local backend)
Client->>Nginx: HTTP request to <sub>.issuetopr.dev
alt preview subdomain
Nginx->>App: proxy_pass http://<subdomain>:3000 (Docker network)
App-->>Nginx: HTTP response (HTML)
Nginx->>Nginx: sub_filter inject widget (uncompressed)
Nginx-->>Client: proxied response with widget
else main domain / SSE
Client->>Nginx: request to /api/sse
Nginx->>Host: proxy_pass http://host.docker.internal:3000 (SSE, no buffering)
Host-->>Nginx: streaming SSE events
Nginx-->>Client: forwarded SSE stream
end
Suggested labels
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25b94c3744
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Use the wildcard certificate that covers *.issuetopr.dev | ||
| ssl_certificate /etc/letsencrypt/live/issuetopr.dev-0001/fullchain.pem; | ||
| ssl_certificate_key /etc/letsencrypt/live/issuetopr.dev-0001/privkey.pem; |
There was a problem hiding this comment.
This preview vhost hardcodes the wildcard cert at /etc/letsencrypt/live/issuetopr.dev-0001/.... On a fresh install, certbot certonly -d issuetopr.dev -d '*.issuetopr.dev' typically creates /etc/letsencrypt/live/issuetopr.dev/ (no -0001 suffix). In that common case NGINX fails to start because the referenced cert files don’t exist, so all preview TLS routes are down until you manually rename or reissue. Consider pointing at the canonical issuetopr.dev path or making the path configurable.
Useful? React with 👍 / 👎.
| if docker run --rm -v "$(pwd)/docker/nginx:/etc/nginx:ro" nginx:stable-alpine nginx -t >/dev/null 2>&1; then | ||
| echo -e "${GREEN}✅ NGINX configuration syntax is valid${NC}" | ||
| else | ||
| echo -e "${RED}❌ ERROR: NGINX configuration syntax is invalid${NC}" | ||
| echo " Run this to see details:" |
There was a problem hiding this comment.
The preflight test runs nginx -t in a container that only mounts /etc/nginx. Because the config references /etc/letsencrypt/..., nginx -t will fail even when the config is valid and certs exist on the host, since those files aren’t mounted into the test container. In deploy-production.sh this means the preflight always fails and will block or prompt in non‑interactive runs. Mount /etc/letsencrypt (or skip cert‑dependent checks) to make this test meaningful.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@docker/compose/nginx.yml`:
- Around line 12-14: Add a clear production prerequisite note to the README
explaining that the docker/compose/nginx.yml bind-mount (-
/etc/letsencrypt:/etc/letsencrypt:ro) used by the prod profile requires a
populated /etc/letsencrypt on the host (or Docker will create an empty
root-owned directory causing NGINX to fail to start because configs in
docker/nginx/conf.d/ like issuetopr.dev.conf, preview.issuetopr.dev.conf and
grafana.issuetopr.dev.conf reference /etc/letsencrypt/live/* ssl_certificate
paths); update the README’s deployment/production section to state this
requirement and include a suggested remediation (create/populate
/etc/letsencrypt or omit the prod profile until certificates are present).
In `@docker/nginx/conf.d/preview.issuetopr.dev.conf`:
- Line 3: The server_name regex uses the named capture (?<preview>.+) which is
too permissive; change it to a constrained character class (e.g. use
(?<preview>[a-z0-9-]+) or similar) so $preview only contains allowed subdomain
characters (lowercase letters, digits, hyphens) before the .issuetopr.dev, and
update any other occurrences that rely on the same capture (notably the
proxy_pass usage referencing $preview) to match this stricter pattern.
In `@docker/scripts/check-nginx-prereqs.sh`:
- Around line 85-93: The docker run check in check-nginx-prereqs.sh assumes
$(pwd) is repo root; derive the repository root from the script location instead
and use that path in the volume mount. Add logic near the top of
check-nginx-prereqs.sh to compute REPO_ROOT from the script’s directory (e.g.,
using "${BASH_SOURCE[0]}" resolution) and replace "$(pwd)/docker/nginx" in the
nginx test command with "$REPO_ROOT/docker/nginx" so the config check works
regardless of the current working directory.
In `@docker/scripts/check-secrets.sh`:
- Around line 38-40: The echo call that includes "\n" is not portable in /bin/sh
(BusyBox ash) and can print a literal backslash-n; update the certbot usage
output in the script to use printf with explicit newlines (e.g., printf "%s\n"
...) or break into multiple echo calls without escape sequences so the certbot
arguments render correctly; change the echo invocation found in
docker/scripts/check-secrets.sh that prints the multi-line certbot example to
use printf (or separate echo lines) to guarantee POSIX-compatible newlines.
In `@docker/scripts/renew-certs.sh`:
- Around line 10-39: The script uses set -e which causes the certbot renew
command to abort the script on failure so the subsequent if [ $? -eq 0 ] check
is never reached; change the flow to run the certbot renew command directly in
an if conditional (e.g., if docker compose -f docker/docker-compose.yml
--profile prod run --rm certbot renew; then ...) so failures are handled by the
if/else block and the NGINX reload logic (referencing the docker compose certbot
renew invocation and the nginx reload exec command) executes the appropriate
success or failure branch instead of being short-circuited by set -e.
In `@docs/deployment/production-checklist.md`:
- Around line 360-372: The "3. Clean up old images and logs" step currently runs
the dangerous command "docker system prune -a"; change this to recommend the
safer "docker system prune" (without -a) or add an explicit warning directly
after the "docker system prune -a" line explaining it will remove all unused
images and may force a full re-pull/rebuild for subsequent docker compose up,
and suggest alternatives (e.g., use "docker image prune" or targeted image
removal) so editors can either replace the command or annotate it with the
impact and safer options.
In `@docs/deployment/README.md`:
- Around line 60-70: Update the certbot invocation shown (the docker compose
certbot certonly command) to use the correct authenticator flag and explicitly
pass the credentials file: replace the incorrect --dns-porkbun flag with
--authenticator dns-porkbun and add --dns-porkbun-credentials
/etc/letsencrypt/secrets/porkbun.ini so certbot uses the Porkbun credentials
created earlier when obtaining certificates.
In `@scripts/deploy-production.sh`:
- Around line 121-132: The health-wait loop currently breaks on any "healthy"
match and always prints success; change it to explicitly verify that both neo4j
and redis are healthy before breaking and to fail if the timeout is reached.
Update the for-loop that runs `docker compose -f docker/docker-compose.yml ps |
grep -q "healthy"` to instead check the status lines for the specific service
names "neo4j" and "redis" (e.g., confirm both have "healthy" in their status),
and after the loop add a conditional: if either service is not healthy after 30
attempts print a descriptive error and exit non‑zero (instead of printing the
success message), otherwise print the existing success message.
- Around line 84-93: The read-based prompt after running check-nginx-prereqs.sh
uses `read -r response` which fails silently in non-interactive contexts; modify
the block to detect a TTY (e.g., test with `[ -t 0 ]`) and only prompt when
interactive, otherwise print a clear message and abort (exit 1) or respect an
explicit override (e.g., a --yes / FORCE_CONTINUE env var) so the script does
not silently cancel in CI; update the logic around the `read -r response` and
the `response` check to implement this TTY check and optional override.
🧹 Nitpick comments (11)
docker/certbot-porkbun/Dockerfile (1)
1-3: Pin the base image to a specific version for reproducible builds.Using
certbot/certbot:latestmeans builds are non-deterministic and could break without warning when a new certbot version is published. Consider pinning to a specific version (e.g.,certbot/certbot:v3.x.x) and updating it intentionally.docs/deployment/CODE_REVIEW_FIXES.md (1)
1-233: This document appears to be AI-generated internal tracking — consider whether it belongs in the repo.This file documents feedback from "Sam (DevOps persona via Task agent)" — an AI persona. Shipping this as permanent documentation could confuse future contributors who may assume "Sam" is a real reviewer. Consider either:
- Moving this to a PR comment or issue instead of a committed file.
- Clearly labeling it as AI-assisted analysis at the top if you want to keep it.
Also, the "Testing Performed" section (lines 215–218) has all items unchecked (⬜), which contradicts the "Fixed Immediately" claims above.
MIGRATION_COMPLETE.md (1)
1-232: This migration-tracking file will quickly become stale — consider removing it or moving to a PR description.
MIGRATION_COMPLETE.mdat the repo root is a one-time migration artifact (references "Task#14", the oldyoung-and-ai-ec2-nginxrepo, etc.). The useful content is already covered bydocs/deployment/README.mdanddocker/nginx/README.md. Leaving this in the repo root adds clutter and will confuse future contributors when it inevitably becomes outdated.Consider capturing this in the PR description instead and removing the file.
docker/nginx/conf.d/issuetopr.dev.conf (2)
20-39: Inconsistent indentation (mixed tabs and spaces) in the SSE location block.The main
location /block uses 8-space indentation, whilelocation /api/sseuses tabs. This makes the config harder to maintain and diff.Also, lines 24–25 set
Connection 'upgrade'and then line 35 overrides it toConnection ''. The override is correct for SSE, but having the dead'upgrade'value above is misleading. Consider removing the redundant WebSocket headers from the SSE block.Proposed cleanup for the SSE block
# Location block for SSE connections location /api/sse { - proxy_pass http://host.docker.internal:3000; - proxy_http_version 1.1; - proxy_set_header Upgrade $http_upgrade; - proxy_set_header Connection 'upgrade'; - proxy_set_header Host $host; - proxy_set_header X-Real-IP $remote_addr; - proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; - proxy_set_header X-Forwarded-Proto $scheme; - proxy_cache_bypass $http_upgrade; - - # SSE specific settings - proxy_buffering off; - proxy_cache off; - proxy_set_header Connection ''; - chunked_transfer_encoding off; - proxy_read_timeout 86400s; - proxy_send_timeout 86400s; + proxy_pass http://host.docker.internal:3000; + proxy_http_version 1.1; + proxy_set_header Host $host; + proxy_set_header X-Real-IP $remote_addr; + proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; + proxy_set_header X-Forwarded-Proto $scheme; + + # SSE specific settings + proxy_set_header Connection ''; + proxy_buffering off; + proxy_cache off; + chunked_transfer_encoding off; + proxy_read_timeout 86400s; + proxy_send_timeout 86400s; }
1-49: Nohttp2and no security headers on the HTTPS server block.The
listen 443 ssldirective on line 42 does not enable HTTP/2. For a production proxy, consider addinghttp2 on;(NGINX ≥1.25.1) orlisten 443 ssl http2;(older versions). Security headers (HSTS, X-Content-Type-Options, X-Frame-Options) are also absent — I see these are tracked as future items, so just flagging for awareness.docker/nginx/conf.d/preview.issuetopr.dev.conf (1)
47-66: Same SSE block issue as inissuetopr.dev.conf— redundantConnection 'upgrade'overridden byConnection ''.Lines 52 and 62 set contradictory
Connectionheader values. Same cleanup as suggested for the other config applies here.docker/nginx/nginx.conf (1)
36-39: Inconsistent indentation: tabs vs. spaces in themapblock.Lines 37–38 use tab indentation while the rest of the file uses spaces. This won't break NGINX but is a style inconsistency.
docs/deployment/production-checklist.md (1)
14-15: Markdown lint: fenced code blocks need surrounding blank lines.Several code blocks are missing blank lines before or after them (MD031). This can cause rendering issues in some Markdown parsers. For example, add a blank line between the text and the opening
```fence.Also applies to: 23-24, 44-45, 55-56, 91-92, 117-118, 147-148, 152-153
docker/scripts/check-nginx-prereqs.sh (1)
95-110: Container name filter"name=nginx"is a substring match — may produce false positives.
docker ps --filter "name=nginx"matches any container whose name contains "nginx" (e.g.,my-nginx-proxy,nginxtest). This could flag unrelated containers. Consider using an anchored regex or exact match if possible.docker/compose/nginx.yml (1)
21-26: Healthcheck runsnginx -tevery 10 seconds — unnecessarily expensive.
nginx -tparses and validates the entire configuration on each invocation. Running it every 10 seconds as part of the healthcheck is wasteful. The HTTP check (wget ... http://localhost:80/) alone is sufficient to verify NGINX is healthy.Suggested fix
healthcheck: - test: ["CMD", "sh", "-c", "wget --quiet --tries=1 --spider http://localhost:80/ && nginx -t"] + test: ["CMD", "sh", "-c", "wget --quiet --tries=1 --spider http://localhost:80/"] interval: 10s timeout: 5s retries: 3 start_period: 10sscripts/deploy-production.sh (1)
123-123: Unused loop variablei(SC2034).Replace
iwith_to signal it's intentionally unused.Fix
-for i in {1..30}; do +for _ in {1..30}; do
| if docker run --rm -v "$(pwd)/docker/nginx:/etc/nginx:ro" nginx:stable-alpine nginx -t >/dev/null 2>&1; then | ||
| echo -e "${GREEN}✅ NGINX configuration syntax is valid${NC}" | ||
| else | ||
| echo -e "${RED}❌ ERROR: NGINX configuration syntax is invalid${NC}" | ||
| echo " Run this to see details:" | ||
| echo " docker run --rm -v \$(pwd)/docker/nginx:/etc/nginx:ro nginx:stable-alpine nginx -t" | ||
| echo "" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
NGINX config test assumes $(pwd) is the repository root.
If this script is invoked from a different working directory (e.g., via an absolute path), the volume mount $(pwd)/docker/nginx will point to the wrong location and the config test will fail with a misleading error. Consider deriving the repo root from the script's own location.
Suggested fix
+# Derive the repo root from this script's location
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+REPO_ROOT="$(cd "$SCRIPT_DIR/../.." && pwd)"
+
# 4. Check NGINX config syntax (if docker is available)
echo ""
echo "4️⃣ Testing NGINX configuration syntax..."
-if docker run --rm -v "$(pwd)/docker/nginx:/etc/nginx:ro" nginx:stable-alpine nginx -t >/dev/null 2>&1; then
+if docker run --rm -v "$REPO_ROOT/docker/nginx:/etc/nginx:ro" nginx:stable-alpine nginx -t >/dev/null 2>&1; then🤖 Prompt for AI Agents
In `@docker/scripts/check-nginx-prereqs.sh` around lines 85 - 93, The docker run
check in check-nginx-prereqs.sh assumes $(pwd) is repo root; derive the
repository root from the script location instead and use that path in the volume
mount. Add logic near the top of check-nginx-prereqs.sh to compute REPO_ROOT
from the script’s directory (e.g., using "${BASH_SOURCE[0]}" resolution) and
replace "$(pwd)/docker/nginx" in the nginx test command with
"$REPO_ROOT/docker/nginx" so the config check works regardless of the current
working directory.
| echo "Provide certbot args, e.g.:" | ||
| echo " docker compose run --rm certbot certonly --agree-tos -m support@youngandai.com --no-eff-email \\\n --dns-porkbun --dns-porkbun-credentials /etc/letsencrypt/secrets/porkbun.ini -d youngandai.com -d '*.youngandai.com'" | ||
| fi |
There was a problem hiding this comment.
\n in echo won't produce a newline in POSIX sh.
The certbot base image uses Alpine, where /bin/sh is BusyBox ash. The \n escape in echo has implementation-defined behavior under POSIX and may render as a literal \n rather than a newline. Use printf for portable formatting.
Proposed fix
- echo " docker compose run --rm certbot certonly --agree-tos -m support@youngandai.com --no-eff-email \\\n --dns-porkbun --dns-porkbun-credentials /etc/letsencrypt/secrets/porkbun.ini -d youngandai.com -d '*.youngandai.com'"
+ printf " docker compose run --rm certbot certonly --agree-tos -m support@youngandai.com --no-eff-email \\\\\n --dns-porkbun --dns-porkbun-credentials /etc/letsencrypt/secrets/porkbun.ini -d youngandai.com -d '*.youngandai.com'\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.
| echo "Provide certbot args, e.g.:" | |
| echo " docker compose run --rm certbot certonly --agree-tos -m support@youngandai.com --no-eff-email \\\n --dns-porkbun --dns-porkbun-credentials /etc/letsencrypt/secrets/porkbun.ini -d youngandai.com -d '*.youngandai.com'" | |
| fi | |
| echo "Provide certbot args, e.g.:" | |
| printf " docker compose run --rm certbot certonly --agree-tos -m support@youngandai.com --no-eff-email \\\\\n --dns-porkbun --dns-porkbun-credentials /etc/letsencrypt/secrets/porkbun.ini -d youngandai.com -d '*.youngandai.com'\n" | |
| fi |
🤖 Prompt for AI Agents
In `@docker/scripts/check-secrets.sh` around lines 38 - 40, The echo call that
includes "\n" is not portable in /bin/sh (BusyBox ash) and can print a literal
backslash-n; update the certbot usage output in the script to use printf with
explicit newlines (e.g., printf "%s\n" ...) or break into multiple echo calls
without escape sequences so the certbot arguments render correctly; change the
echo invocation found in docker/scripts/check-secrets.sh that prints the
multi-line certbot example to use printf (or separate echo lines) to guarantee
POSIX-compatible newlines.
There was a problem hiding this comment.
OK, I take your word for it that this is true. maybe just do a double check online if this is true, and if so, then make the fix as suggested.
There was a problem hiding this comment.
@youngchingjui, confirmed — the concern is well-founded. Per the official POSIX specification, if any argument to echo contains a backslash character, the results are implementation-defined. In short, echo is only portable if you omit flags and escape sequences; printf should be used instead when you need more than plain text.
Interestingly, ash/dash/busybox/ksh have a feature where echo interprets \n as a newline — but other shells don't do that, so behaviour is unpredictable across environments. printf is fully standardized in POSIX and behaves consistently across all systems.
Here is the fix to apply:
- echo " docker compose run --rm certbot certonly --agree-tos -m support@youngandai.com --no-eff-email \\\n --dns-porkbun --dns-porkbun-credentials /etc/letsencrypt/secrets/porkbun.ini -d youngandai.com -d '*.youngandai.com'"
+ printf " docker compose run --rm certbot certonly --agree-tos -m support@youngandai.com --no-eff-email \\\\\n --dns-porkbun --dns-porkbun-credentials /etc/letsencrypt/secrets/porkbun.ini -d youngandai.com -d '*.youngandai.com'\n"The \\\\ in the printf format string produces a literal \\ in the output (which then displays as the shell line-continuation \ in the example command), and the \n produces the actual newline.
🧠 Learnings used
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1282
File: shared/src/ports/events/publisher.ts:16-19
Timestamp: 2025-09-22T09:24:26.840Z
Learning: youngchingjui prefers to manage PR scope tightly and defer technical debt improvements to future PRs rather than expanding the current PR's scope, even for related consistency issues.
| set -e | ||
|
|
||
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| PROJECT_ROOT="$(cd "$SCRIPT_DIR/../.." && pwd)" | ||
|
|
||
| echo "=== Certificate Renewal: $(date) ===" | ||
|
|
||
| # Change to project directory | ||
| cd "$PROJECT_ROOT" | ||
|
|
||
| # Attempt to renew certificates | ||
| echo "Attempting certificate renewal..." | ||
| docker compose -f docker/docker-compose.yml --profile prod run --rm certbot renew | ||
|
|
||
| # Check if renewal was successful by looking at exit code | ||
| if [ $? -eq 0 ]; then | ||
| echo "Certificate renewal successful (or not needed yet)" | ||
|
|
||
| # Reload NGINX to pick up any new certificates | ||
| echo "Reloading NGINX configuration..." | ||
| if docker compose -f docker/docker-compose.yml exec nginx nginx -s reload; then | ||
| echo "✅ NGINX reloaded successfully" | ||
| else | ||
| echo "⚠️ Warning: Failed to reload NGINX" | ||
| exit 1 | ||
| fi | ||
| else | ||
| echo "❌ Certificate renewal failed" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
set -e makes the renewal failure branch unreachable.
With set -e on line 10, if docker compose ... certbot renew on line 22 exits non-zero, the script terminates immediately — the $? check on line 25 is never reached, and the else branch (lines 36-38) is dead code. Use the command directly in the if condition, which suppresses set -e for that command:
Proposed fix
# Attempt to renew certificates
echo "Attempting certificate renewal..."
-docker compose -f docker/docker-compose.yml --profile prod run --rm certbot renew
-
-# Check if renewal was successful by looking at exit code
-if [ $? -eq 0 ]; then
+if docker compose -f docker/docker-compose.yml --profile prod run --rm certbot renew; then
echo "Certificate renewal successful (or not needed yet)"📝 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.
| set -e | |
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | |
| PROJECT_ROOT="$(cd "$SCRIPT_DIR/../.." && pwd)" | |
| echo "=== Certificate Renewal: $(date) ===" | |
| # Change to project directory | |
| cd "$PROJECT_ROOT" | |
| # Attempt to renew certificates | |
| echo "Attempting certificate renewal..." | |
| docker compose -f docker/docker-compose.yml --profile prod run --rm certbot renew | |
| # Check if renewal was successful by looking at exit code | |
| if [ $? -eq 0 ]; then | |
| echo "Certificate renewal successful (or not needed yet)" | |
| # Reload NGINX to pick up any new certificates | |
| echo "Reloading NGINX configuration..." | |
| if docker compose -f docker/docker-compose.yml exec nginx nginx -s reload; then | |
| echo "✅ NGINX reloaded successfully" | |
| else | |
| echo "⚠️ Warning: Failed to reload NGINX" | |
| exit 1 | |
| fi | |
| else | |
| echo "❌ Certificate renewal failed" | |
| exit 1 | |
| fi | |
| set -e | |
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | |
| PROJECT_ROOT="$(cd "$SCRIPT_DIR/../.." && pwd)" | |
| echo "=== Certificate Renewal: $(date) ===" | |
| # Change to project directory | |
| cd "$PROJECT_ROOT" | |
| # Attempt to renew certificates | |
| echo "Attempting certificate renewal..." | |
| if docker compose -f docker/docker-compose.yml --profile prod run --rm certbot renew; then | |
| echo "Certificate renewal successful (or not needed yet)" | |
| # Reload NGINX to pick up any new certificates | |
| echo "Reloading NGINX configuration..." | |
| if docker compose -f docker/docker-compose.yml exec nginx nginx -s reload; then | |
| echo "✅ NGINX reloaded successfully" | |
| else | |
| echo "⚠️ Warning: Failed to reload NGINX" | |
| exit 1 | |
| fi | |
| else | |
| echo "❌ Certificate renewal failed" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
In `@docker/scripts/renew-certs.sh` around lines 10 - 39, The script uses set -e
which causes the certbot renew command to abort the script on failure so the
subsequent if [ $? -eq 0 ] check is never reached; change the flow to run the
certbot renew command directly in an if conditional (e.g., if docker compose -f
docker/docker-compose.yml --profile prod run --rm certbot renew; then ...) so
failures are handled by the if/else block and the NGINX reload logic
(referencing the docker compose certbot renew invocation and the nginx reload
exec command) executes the appropriate success or failure branch instead of
being short-circuited by set -e.
There was a problem hiding this comment.
What does set -e do?
There was a problem hiding this comment.
@youngchingjui, set -e (also written as set -o errexit) is a Bash option that tells the shell to exit immediately if any command returns a non-zero (failure) exit code, rather than continuing to the next line.
In your script, this creates the bug flagged in the review:
set -e # <-- exit on any failure
# If this command fails (non-zero exit), the script exits HERE...
docker compose -f docker/docker-compose.yml --profile prod run --rm certbot renew
# ...so this line is NEVER reached on failure
if [ $? -eq 0 ]; then
...
else
echo "❌ Certificate renewal failed" # <-- dead code, never executes
exit 1
fiWhy is this a problem?
- If
certbot renewfails,set -ekills the script right away on line 22. - The
$?check on line 25 only ever sees0(success), because the script would have already terminated if it were anything else. - Your
elsebranch (the "❌ Certificate renewal failed" message) is therefore unreachable dead code.
The fix is to move the command directly into the if condition. When a command is used as an if condition, Bash suspends set -e for that command and lets the if/else logic handle the result normally:
if docker compose -f docker/docker-compose.yml --profile prod run --rm certbot renew; then
echo "Certificate renewal successful (or not needed yet)"
...
else
echo "❌ Certificate renewal failed" # now reachable!
exit 1
fi🧠 Learnings used
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1282
File: shared/src/ports/events/publisher.ts:16-19
Timestamp: 2025-09-22T09:24:26.840Z
Learning: youngchingjui prefers to manage PR scope tightly and defer technical debt improvements to future PRs rather than expanding the current PR's scope, even for related consistency issues.
There was a problem hiding this comment.
why don't we just remove set -e?
There was a problem hiding this comment.
@youngchingjui, you can — but there's a trade-off to be aware of.
If you remove set -e, the if [ $? -eq 0 ] check works as intended, and the else branch becomes reachable again. That's valid.
However, set -e is also silently guarding two other critical commands earlier in the script:
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
PROJECT_ROOT="$(cd "$SCRIPT_DIR/../.." && pwd)"
...
cd "$PROJECT_ROOT" # if this silently fails, docker compose runs from the wrong directoryWithout set -e, if cd "$PROJECT_ROOT" fails (e.g., bad path), the script continues running in the wrong directory. The docker compose -f docker/docker-compose.yml command would then fail to find the compose file — still an error, but with a less helpful message.
Summary of options:
| Approach | Pro | Con |
|---|---|---|
Remove set -e |
Simple, $? check works |
Loses safety net on cd and path resolution |
Keep set -e, move certbot into if |
Keeps implicit safety on other commands | Slightly more verbose |
Remove set -e + add explicit cd check |
Best of both | More verbose |
The cleanest minimal change is still moving the certbot command into the if condition (as suggested), but removing set -e is a perfectly acceptable simplification for a script this short — the failure modes are easy to reason about.
🧠 Learnings used
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1282
File: shared/src/ports/events/publisher.ts:16-19
Timestamp: 2025-09-22T09:24:26.840Z
Learning: youngchingjui prefers to manage PR scope tightly and defer technical debt improvements to future PRs rather than expanding the current PR's scope, even for related consistency issues.
There was a problem hiding this comment.
I'd prefer more articulate, clearer logic paths that a more junior developer can understand, rather than something more nifty. Code is writing, after all, and clearer writing wins. So I'd prefer to remove set -e, and be more explicit about checks and failure modes.
There was a problem hiding this comment.
@youngchingjui, that's a great principle. Here's the script rewritten with set -e removed and every failure mode handled explicitly:
#!/bin/bash
# Certificate Renewal Script
#
# This script should be run periodically (via cron) to renew SSL certificates
# and reload NGINX to pick up the new certificates.
#
# Add to crontab (runs daily at 2am):
# 0 2 * * * /path/to/issue-to-pr/docker/scripts/renew-certs.sh >> /var/log/certbot-renewal.log 2>&1
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
PROJECT_ROOT="$(cd "$SCRIPT_DIR/../.." && pwd)"
echo "=== Certificate Renewal: $(date) ==="
# Change to project directory
cd "$PROJECT_ROOT"
if [ $? -ne 0 ]; then
echo "❌ Failed to change to project root: $PROJECT_ROOT"
exit 1
fi
# Attempt to renew certificates
echo "Attempting certificate renewal..."
docker compose -f docker/docker-compose.yml --profile prod run --rm certbot renew
if [ $? -ne 0 ]; then
echo "❌ Certificate renewal failed"
exit 1
fi
echo "Certificate renewal successful (or not needed yet)"
# Reload NGINX to pick up any new certificates
echo "Reloading NGINX configuration..."
docker compose -f docker/docker-compose.yml exec nginx nginx -s reload
if [ $? -ne 0 ]; then
echo "⚠️ Warning: Failed to reload NGINX"
exit 1
fi
echo "✅ NGINX reloaded successfully"Each command is run on its own line, followed by an explicit $? check — so a junior developer can read straight down the file and understand exactly what's being tested and what happens when it fails.
✏️ Learnings added
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1515
File: docker/scripts/renew-certs.sh:10-39
Timestamp: 2026-03-13T09:14:43.223Z
Learning: youngchingjui prefers explicit, readable shell scripting over concise idioms (e.g., prefers running a command then checking $? on the next line over embedding the command in an if condition). Values clarity for junior developers over brevity. Avoids set -e in favour of explicit per-command failure checks.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1282
File: shared/src/ports/events/publisher.ts:16-19
Timestamp: 2025-09-22T09:24:26.840Z
Learning: youngchingjui prefers to manage PR scope tightly and defer technical debt improvements to future PRs rather than expanding the current PR's scope, even for related consistency issues.
| bash docker/scripts/check-nginx-prereqs.sh || { | ||
| echo "" | ||
| echo -e "${YELLOW}⚠️ Pre-flight checks found potential issues.${NC}" | ||
| echo "Continue anyway? (y/N) " | ||
| read -r response | ||
| if [[ ! "$response" =~ ^[Yy]$ ]]; then | ||
| echo "Deployment cancelled." | ||
| exit 1 | ||
| fi | ||
| } |
There was a problem hiding this comment.
read will fail silently in non-interactive contexts (e.g., CI/CD).
When stdin is not a terminal (piped input, CI runner), read -r response gets EOF, response is empty, and the deployment is silently cancelled. If this script is intended for CI/CD use, consider checking for a TTY or defaulting to abort with a clear message.
Suggested improvement
bash docker/scripts/check-nginx-prereqs.sh || {
echo ""
echo -e "${YELLOW}⚠️ Pre-flight checks found potential issues.${NC}"
- echo "Continue anyway? (y/N) "
- read -r response
- if [[ ! "$response" =~ ^[Yy]$ ]]; then
- echo "Deployment cancelled."
- exit 1
+ if [ -t 0 ]; then
+ echo "Continue anyway? (y/N) "
+ read -r response
+ if [[ ! "$response" =~ ^[Yy]$ ]]; then
+ echo "Deployment cancelled."
+ exit 1
+ fi
+ else
+ echo "Non-interactive mode detected. Use --skip-checks to bypass."
+ exit 1
fi
}📝 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.
| bash docker/scripts/check-nginx-prereqs.sh || { | |
| echo "" | |
| echo -e "${YELLOW}⚠️ Pre-flight checks found potential issues.${NC}" | |
| echo "Continue anyway? (y/N) " | |
| read -r response | |
| if [[ ! "$response" =~ ^[Yy]$ ]]; then | |
| echo "Deployment cancelled." | |
| exit 1 | |
| fi | |
| } | |
| bash docker/scripts/check-nginx-prereqs.sh || { | |
| echo "" | |
| echo -e "${YELLOW}⚠️ Pre-flight checks found potential issues.${NC}" | |
| if [ -t 0 ]; then | |
| echo "Continue anyway? (y/N) " | |
| read -r response | |
| if [[ ! "$response" =~ ^[Yy]$ ]]; then | |
| echo "Deployment cancelled." | |
| exit 1 | |
| fi | |
| else | |
| echo "Non-interactive mode detected. Use --skip-checks to bypass." | |
| exit 1 | |
| fi | |
| } |
🤖 Prompt for AI Agents
In `@scripts/deploy-production.sh` around lines 84 - 93, The read-based prompt
after running check-nginx-prereqs.sh uses `read -r response` which fails
silently in non-interactive contexts; modify the block to detect a TTY (e.g.,
test with `[ -t 0 ]`) and only prompt when interactive, otherwise print a clear
message and abort (exit 1) or respect an explicit override (e.g., a --yes /
FORCE_CONTINUE env var) so the script does not silently cancel in CI; update the
logic around the `read -r response` and the `response` check to implement this
TTY check and optional override.
There was a problem hiding this comment.
Thanks for the consideration, but I don't think we intend to use this script for those purposes, but we should clearly state that at the top of the file or in a readme or both.
There was a problem hiding this comment.
@youngchingjui, makes sense! Here are concrete suggestions for both locations:
1. Add a note to the script header (scripts/deploy-production.sh, after line 13):
# Options:
# --with-nginx Deploy NGINX reverse proxy (requires ports 80/443)
# --skip-checks Skip pre-flight validation checks
# --help Show this help message
+#
+# NOTE: This script is intended for interactive, manual deployments only.
+# It is NOT designed for use in CI/CD pipelines or non-interactive environments.2. Add a note to the deployment README (docs/deployment/README.md), in whichever section covers the production deployment script:
> **Note:** `scripts/deploy-production.sh` is designed for **interactive, manual use** only (e.g., deploying from an SSH session on the server). It is not intended for CI/CD pipelines or automated non-interactive environments.Would you like me to plan these documentation changes, or would you prefer to add them manually?
🧠 Learnings used
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1282
File: shared/src/ports/events/publisher.ts:16-19
Timestamp: 2025-09-22T09:24:26.840Z
Learning: youngchingjui prefers to manage PR scope tightly and defer technical debt improvements to future PRs rather than expanding the current PR's scope, even for related consistency issues.
| # Wait for services to be ready | ||
| echo "Waiting for services to initialize..." | ||
| for i in {1..30}; do | ||
| if docker compose -f docker/docker-compose.yml ps | grep -q "healthy"; then | ||
| break | ||
| fi | ||
| sleep 1 | ||
| echo -n "." | ||
| done | ||
| echo "" | ||
| echo -e "${GREEN}✅ Core services ready${NC}" | ||
| echo "" |
There was a problem hiding this comment.
Health-wait loop is unreliable: breaks on any "healthy" service and reports success on timeout.
Two issues:
grep -q "healthy"matches if any service is healthy, not necessarily neo4j and redis specifically.- If the loop exhausts 30 iterations without a healthy match, the script still prints "Core services ready" — there's no timeout/failure path.
Suggested fix
# Wait for services to be ready
echo "Waiting for services to initialize..."
-for i in {1..30}; do
- if docker compose -f docker/docker-compose.yml ps | grep -q "healthy"; then
+READY=0
+for _ in {1..30}; do
+ # Check that both neo4j and redis are healthy
+ UNHEALTHY=$(docker compose -f docker/docker-compose.yml ps neo4j redis | grep -cv "healthy" || true)
+ if [ "$UNHEALTHY" -le 1 ]; then # header line always present
+ READY=1
break
fi
sleep 1
echo -n "."
done
echo ""
-echo -e "${GREEN}✅ Core services ready${NC}"
+if [ "$READY" -eq 1 ]; then
+ echo -e "${GREEN}✅ Core services ready${NC}"
+else
+ echo -e "${RED}❌ Core services did not become healthy within 30 seconds${NC}"
+ docker compose -f docker/docker-compose.yml ps neo4j redis
+ exit 1
+fi
echo ""🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 123-123: i appears unused. Verify use (or export if used externally).
(SC2034)
🤖 Prompt for AI Agents
In `@scripts/deploy-production.sh` around lines 121 - 132, The health-wait loop
currently breaks on any "healthy" match and always prints success; change it to
explicitly verify that both neo4j and redis are healthy before breaking and to
fail if the timeout is reached. Update the for-loop that runs `docker compose -f
docker/docker-compose.yml ps | grep -q "healthy"` to instead check the status
lines for the specific service names "neo4j" and "redis" (e.g., confirm both
have "healthy" in their status), and after the loop add a conditional: if either
service is not healthy after 30 attempts print a descriptive error and exit
non‑zero (instead of printing the success message), otherwise print the existing
success message.
Testing Plan for Nginx Migration PRThis PR is a config migration (moving nginx from a standalone repo into the monorepo), not new application code. Testing verifies the configs work identically in the new setup, with zero downtime. Key config changeThe only meaningful behavioral difference: Phase 1: Local/Offline Validation (safe, do anytime)
Phase 2: Parallel Test on EC2 (non-destructive)Run the new nginx alongside the old one on different ports to verify it works before cutting over.
Phase 3: Cutover (~30 seconds of risk)Only after Phase 2 passes:
Phase 4: Cleanup (after cutover confirmed)
|
PR #1515 Code Review — NGINX MigrationPhase 1: Local Validation Results1. NGINX Config Syntax Check (
|
| # | Severity | File | Issue |
|---|---|---|---|
| 1 | 🔴 Critical | docker/compose/nginx.yml |
Full /etc/nginx mount overwrites mime.types — NGINX won't start |
| 2 | 🔴 Critical | docker/compose/nginx.yml |
Mount of ../nginx/default.d references non-existent directory |
| 3 | 🟡 Medium | docker/scripts/renew-certs.sh |
set -e makes error handling branch unreachable |
| 4 | 🟡 Medium | scripts/deploy-production.sh |
source .env.nginx is unsafe — executes as shell code |
| 5 | 🟡 Medium | docker/nginx/conf.d/issuetopr.dev.conf |
SSE block sets Connection 'upgrade' then overrides with Connection '' — the first set is misleading dead config |
Security Observations
| # | Severity | Issue |
|---|---|---|
| 1 | 🟡 Medium | No rate limiting configured (documented in checklist but should be included in initial config) |
| 2 | 🟡 Medium | No security headers (X-Frame-Options, X-Content-Type-Options, etc.) |
| 3 | 🟢 Low | sub_filter in preview config injects a <script> tag — benign (console.log only) but worth noting |
| 4 | 🟢 Low | Certbot Dockerfile uses certbot/certbot:latest — should pin to a specific version for reproducibility |
Documentation Concerns
| # | Issue |
|---|---|
| 1 | MIGRATION_COMPLETE.md (232 lines) is a one-time status report, not ongoing documentation. It references files that don't exist in this PR (staging.youngandai.com.conf, financial-suitability.poc.youngandai.com.conf, .env.nginx.example). Should be removed or converted to a PR description. |
| 2 | docs/deployment/CODE_REVIEW_FIXES.md (233 lines) is internal process documentation ("Sam's review"). Should not be committed to the repo. |
| 3 | .codex/environments/environment.toml is unrelated to the nginx migration. |
| 4 | CLAUDE.md changes add a generic "Workflow for Making Changes" section unrelated to nginx. |
| 5 | Excessive documentation volume (5 new markdown files, ~1300+ lines) for what is essentially an nginx config migration. |
Positive Aspects ✅
- Clean separation of nginx configs into
conf.d/per-domain files - Proper SSE support with
proxy_buffering offand long timeouts - WebSocket upgrade map in nginx.conf
- Preview URL routing with dynamic DNS resolution is well-designed
- Healthcheck includes
nginx -t(good catch from earlier review) - Deploy script has smoke tests
- Profile-based gating (
prod) keeps dev environment clean extra_hostsfor Linuxhost.docker.internalsupport
Summary
This PR should not be merged as-is. The critical volume mount issue means NGINX will fail to start. Minimum required fixes before merge:
- Fix the
/etc/nginxvolume mount to not overwritemime.types - Remove or create the
default.d/directory mount - Fix
set -e+$?dead code inrenew-certs.sh - Remove
MIGRATION_COMPLETE.mdandCODE_REVIEW_FIXES.mdfrom the PR - Remove unrelated changes (
.codex/environments/environment.toml, generic CLAUDE.md additions)
After those fixes, re-run nginx -t to confirm the config is valid before merging.
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 `@docker/nginx/README.md`:
- Around line 15-23: The README.md contains a fenced code block without a
language tag (triggering markdownlint MD040); update the opening fence in the
docker/nginx README's directory-listing block to include a language (e.g., add
"text" after the triple backticks) so the block becomes a labeled code fence and
satisfies the linter.
- Around line 113-118: Add a one-line prerequisite that ensures the external
Docker network named "preview" exists before the "Start NGINX (Production
Profile)" docker compose command; update the section containing the docker
compose command (the block with "docker compose -f docker/docker-compose.yml
--profile prod up -d nginx") to include a preceding line that runs a safe create
for the "preview" network (e.g., docker network create preview 2>/dev/null ||
true) so the compose up won't fail on fresh environments.
- Line 22: Remove the stale "default.d/" entry from the README directory tree:
edit the docker/nginx/README.md to delete the "└── default.d/ #
Default server configurations" line (and any mention of default.d) so docs match
the repo; if you prefer to keep the include in nginx.conf (the "include
/etc/nginx/default.d/*.conf" at line 53), alternatively create the local
default.d directory and add it to docker-compose mounts, but the immediate fix
is to remove the nonexistent default.d reference from the README so the
documentation reflects the actual configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8c25a85e-a032-437b-9968-2ded761e8bf5
📒 Files selected for processing (2)
docker/compose/nginx.ymldocker/nginx/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docker/compose/nginx.yml
Follow-up: Simplification RecommendationsAfter reviewing all the other reviewer comments (CodeRabbit, Codex), here's a consolidated take. The core nginx migration is solid — the problems are in the wrapping scripts and documentation. Less code = fewer bugs. Challenge: Do the scripts need to exist?
|
| Reviewer suggestion | Their fix | Simpler alternative |
|---|---|---|
renew-certs.sh set-e bug |
Run certbot in if conditional |
Delete the script, use 2-line crontab |
deploy-production.sh TTY check |
Add --yes flag + TTY detection |
Remove the interactive prompt (or the script) |
check-nginx-prereqs.sh $(pwd) |
Derive root from SCRIPT_DIR |
Delete the script |
Healthcheck nginx -t every 10s |
HTTP-only check | Agree — just wget --spider http://localhost:80/ suffices |
source .env.nginx unsafe |
env $(cat file) pattern |
Use env_file: in compose YAML instead |
Agreed-upon fixes from all reviewers
These are straightforward and should be applied:
-
Volume mount — Mount individual paths, not all of
/etc/nginx:volumes: - ../nginx/nginx.conf:/etc/nginx/nginx.conf:ro - ../nginx/conf.d:/etc/nginx/conf.d:ro - /etc/letsencrypt:/etc/letsencrypt:ro
Remove the
default.dmount entirely (directory doesn't exist). -
SSE location block — Remove the misleading WebSocket headers. Clean version:
location /api/sse { proxy_pass http://host.docker.internal:3000; proxy_http_version 1.1; proxy_set_header Host $host; proxy_set_header X-Real-IP $remote_addr; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; proxy_set_header X-Forwarded-Proto $scheme; proxy_set_header Connection ''; proxy_buffering off; proxy_cache off; chunked_transfer_encoding off; proxy_read_timeout 86400s; proxy_send_timeout 86400s; }
-
Preview regex — Tighten from
(?<preview>.+)to(?<preview>[a-z0-9-]+)for safety. -
Pin certbot Dockerfile — Use a specific version instead of
:latest. -
Drop
nginx -tfrom healthcheck — Running config parse every 10s is wasteful; HTTP check alone is sufficient.
Documentation: what to keep vs. delete
Keep:
docker/nginx/README.md— useful, close to the codedocker/README.mdupdates — good additions
Delete from PR:
MIGRATION_COMPLETE.md— one-time status report, references files not in this PR (staging.youngandai.com.conf,.env.nginx.example)docs/deployment/CODE_REVIEW_FIXES.md— internal AI review session log, not repo documentationdocs/deployment/production-checklist.md(388 lines) — the useful bits (cert renewal cron, log rotation config) are ~10 lines that belong in the nginx READMEdocs/deployment/README.md(299 lines) — largely duplicatesdocker/README.md
Also remove unrelated changes:
.codex/environments/environment.toml- Generic "Workflow for Making Changes" addition to
CLAUDE.md
Proposed minimal PR
The simplest version of this migration:
docker/nginx/nginx.conf+conf.d/*.conf(with fixes above)docker/compose/nginx.yml(fixed volumes, simpler healthcheck)docker/certbot-porkbun/Dockerfile(pinned version)docker/docker-compose.yml(one include line)docker/README.mdupdates (nginx section + cert renewal crontab)docker/nginx/README.mddocker/scripts/check-secrets.sh(certbot entrypoint — this one earns its keep)
That's ~200 lines of actual config, zero script bugs, zero doc duplication. Everything else can be added incrementally if/when it's actually needed.
- Deleted `MIGRATION_COMPLETE.md`, `environment.toml`, and `CODE_REVIEW_FIXES.md` as they are no longer needed. - Removed the `production-checklist.md` to streamline deployment documentation. - Updated `CLAUDE.md` to reflect the removal of the production checklist and adjusted the documentation index accordingly. - Enhanced `docker/nginx/README.md` with clearer instructions for SSL certificate management and deployment processes. - Improved NGINX configuration files for better readability and organization. These changes aim to simplify the documentation and improve the clarity of deployment instructions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
docker/nginx/conf.d/issuetopr.dev.conf (2)
1-44: Consider adding security headers for the HTTPS server block.The configuration lacks common security headers that harden the deployment. These are particularly important for a production site handling user authentication.
📋 Recommended headers
server { server_name issuetopr.dev www.issuetopr.dev; # Security headers add_header Strict-Transport-Security "max-age=31536000; includeSubDomains" always; add_header X-Content-Type-Options "nosniff" always; add_header X-Frame-Options "SAMEORIGIN" always; add_header Referrer-Policy "strict-origin-when-cross-origin" always; # ... rest of config }This can be deferred to a follow-up PR if desired, given the scope of this migration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/nginx/conf.d/issuetopr.dev.conf` around lines 1 - 44, Add common security headers to the HTTPS server block shown (the server { ... } that contains listen 443 ssl and the location blocks) by inserting add_header directives for Strict-Transport-Security "max-age=31536000; includeSubDomains" always;, X-Content-Type-Options "nosniff" always;, X-Frame-Options "SAMEORIGIN" always;, and Referrer-Policy "strict-origin-when-cross-origin" always; so they are sent on all responses; place these lines near the top of the server block (above the location blocks) so they apply to both / and /api/sse responses.
37-41: Consider enabling HTTP/2 for improved performance.HTTP/2 provides multiplexing, header compression, and other benefits for modern browsers. Adding
http2to the listen directive is straightforward.♻️ Proposed fix
- listen 443 ssl; # managed by Certbot + listen 443 ssl http2; # managed by Certbot🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/nginx/conf.d/issuetopr.dev.conf` around lines 37 - 41, The nginx SSL server block currently uses the directive "listen 443 ssl;" without HTTP/2; update the listen directive to "listen 443 ssl http2;" (i.e., add http2 to the existing listen 443 ssl line) so the server block that includes ssl_certificate, ssl_certificate_key, include /etc/letsencrypt/options-ssl-nginx.conf, and ssl_dhparam uses HTTP/2 for improved performance; after change, reload/restart nginx to apply.docs/deployment/README.md (1)
85-90: Consider using a renewal script instead of inline cron command.The crontab entry is duplicated (here and line 276) and spans a single long line, making it error-prone. A wrapper script in
docker/scripts/would be more maintainable.📝 Alternative approach
Create
docker/scripts/renew-certs.sh(if not already present) and reference it:# Add to crontab (runs daily at 2am): 0 2 * * * /path/to/issue-to-pr/docker/scripts/renew-certs.sh >> /var/log/certbot-renewal.log 2>&1This centralizes the renewal logic and avoids duplication in documentation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment/README.md` around lines 85 - 90, The crontab example in the README is long, duplicated, and error-prone; create a wrapper script named docker/scripts/renew-certs.sh that encapsulates the renew and reload commands (the docker compose certbot renew invocation and the nginx reload), make the script executable and handle logging/exit codes, then update the README examples (the crontab line shown and the duplicate later) to reference the script as "0 2 * * * /path/to/issue-to-pr/docker/scripts/renew-certs.sh >> /var/log/certbot-renewal.log 2>&1" so the renewal logic is centralized and the docs no longer contain the long inline command.docker/certbot-porkbun/Dockerfile (1)
1-3: Pin thecertbot-dns-porkbunversion for reproducibility.The base image is pinned to
v3.3.0, butcertbot-dns-porkbunis unpinned. This could lead to unexpected behavior if a breaking change is released. The latest version available is0.11.0.Note: The Trivy warning about running as root is expected—certbot requires root privileges to write certificates to
/etc/letsencrypt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/certbot-porkbun/Dockerfile` around lines 1 - 3, Pin the certbot-dns-porkbun dependency in the Dockerfile by changing the RUN pip install step (the line that calls pip install certbot-dns-porkbun) to install a specific version — e.g., certbot-dns-porkbun==0.11.0 — while keeping the existing flags like --no-cache-dir to ensure reproducible builds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/deployment/README.md`:
- Around line 295-301: Rename the duplicate "Monitoring" subsection heading at
the second occurrence to a unique title (for example "Uptime & Health
Monitoring" or "Service Monitoring") to avoid conflict with the earlier
"Monitoring" heading; update the heading text in the docs where the second
"Monitoring" appears and ensure any internal links or TOC entries that pointed
to that subsection are updated to the new heading name so links remain correct.
---
Nitpick comments:
In `@docker/certbot-porkbun/Dockerfile`:
- Around line 1-3: Pin the certbot-dns-porkbun dependency in the Dockerfile by
changing the RUN pip install step (the line that calls pip install
certbot-dns-porkbun) to install a specific version — e.g.,
certbot-dns-porkbun==0.11.0 — while keeping the existing flags like
--no-cache-dir to ensure reproducible builds.
In `@docker/nginx/conf.d/issuetopr.dev.conf`:
- Around line 1-44: Add common security headers to the HTTPS server block shown
(the server { ... } that contains listen 443 ssl and the location blocks) by
inserting add_header directives for Strict-Transport-Security "max-age=31536000;
includeSubDomains" always;, X-Content-Type-Options "nosniff" always;,
X-Frame-Options "SAMEORIGIN" always;, and Referrer-Policy
"strict-origin-when-cross-origin" always; so they are sent on all responses;
place these lines near the top of the server block (above the location blocks)
so they apply to both / and /api/sse responses.
- Around line 37-41: The nginx SSL server block currently uses the directive
"listen 443 ssl;" without HTTP/2; update the listen directive to "listen 443 ssl
http2;" (i.e., add http2 to the existing listen 443 ssl line) so the server
block that includes ssl_certificate, ssl_certificate_key, include
/etc/letsencrypt/options-ssl-nginx.conf, and ssl_dhparam uses HTTP/2 for
improved performance; after change, reload/restart nginx to apply.
In `@docs/deployment/README.md`:
- Around line 85-90: The crontab example in the README is long, duplicated, and
error-prone; create a wrapper script named docker/scripts/renew-certs.sh that
encapsulates the renew and reload commands (the docker compose certbot renew
invocation and the nginx reload), make the script executable and handle
logging/exit codes, then update the README examples (the crontab line shown and
the duplicate later) to reference the script as "0 2 * * *
/path/to/issue-to-pr/docker/scripts/renew-certs.sh >>
/var/log/certbot-renewal.log 2>&1" so the renewal logic is centralized and the
docs no longer contain the long inline command.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db1ed645-909c-4e13-9b88-a88b53c626ea
📒 Files selected for processing (8)
CLAUDE.mddocker/certbot-porkbun/Dockerfiledocker/compose/nginx.ymldocker/nginx/README.mddocker/nginx/conf.d/issuetopr.dev.confdocker/nginx/conf.d/preview.issuetopr.dev.confdocker/nginx/nginx.confdocs/deployment/README.md
✅ Files skipped from review due to trivial changes (1)
- docker/nginx/README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- docker/nginx/conf.d/preview.issuetopr.dev.conf
- docker/compose/nginx.yml
- CLAUDE.md
- docker/nginx/nginx.conf
| ### Monitoring | ||
|
|
||
| Set up uptime monitoring (e.g. UptimeRobot free tier) for your domain. Monitor: | ||
|
|
||
| - HTTPS response on your domain | ||
| - SSL certificate expiration | ||
| - Disk space on the server |
There was a problem hiding this comment.
Rename to avoid duplicate heading.
The static analysis tool flagged a duplicate heading—"Monitoring" appears at both line 244 (section heading) and line 295 (subsection). This can confuse document linking and navigation.
📝 Proposed fix
-### Monitoring
+### Uptime Monitoring
Set up uptime monitoring (e.g. UptimeRobot free tier) for your domain. Monitor:🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 295-295: Multiple headings with the same content
(MD024, no-duplicate-heading)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/deployment/README.md` around lines 295 - 301, Rename the duplicate
"Monitoring" subsection heading at the second occurrence to a unique title (for
example "Uptime & Health Monitoring" or "Service Monitoring") to avoid conflict
with the earlier "Monitoring" heading; update the heading text in the docs where
the second "Monitoring" appears and ensure any internal links or TOC entries
that pointed to that subsection are updated to the new heading name so links
remain correct.
Code Review: NGINX Reverse Proxy ConfigurationOverall this is a solid setup — well-documented, clean compose structure, and good use of profiles. Below are findings organized by severity. Security Issues1. Missing security headers on all server blocks (High) None of the NGINX server blocks set security headers. For a production deployment, you should add at minimum: add_header Strict-Transport-Security "max-age=63072000; includeSubDomains; preload" always;
add_header X-Content-Type-Options "nosniff" always;
add_header X-Frame-Options "DENY" always;
add_header Referrer-Policy "strict-origin-when-cross-origin" always;Consider adding these in 2. No rate limiting (Medium) There's no limit_req_zone $binary_remote_addr zone=general:10m rate=10r/s;3. In if ($host = www.issuetopr.dev) {
return 301 https://issuetopr.dev$request_uri;
}Correctness Issues4. WebSocket
proxy_set_header Connection 'upgrade';This means non-WebSocket requests also get proxy_set_header Connection $connection_upgrade;5. Grafana uses a separate cert path, inconsistent with wildcard setup (Medium)
6. Preview proxy_set_header Accept-Encoding "";This forces upstream to send uncompressed responses so
Best Practice Issues7. No
gzip on;
gzip_vary on;
gzip_proxied any;
gzip_comp_level 6;
gzip_types text/plain text/css application/json application/javascript text/xml application/xml text/javascript;8. Log rotation not configured in compose (Low) The deployment guide mentions adding log rotation as a TODO, but the actual logging:
driver: "json-file"
options:
max-size: "10m"
max-file: "3"9. Healthcheck may fail on HTTPS-only setups (Low) The healthcheck hits 10. The script contains 11. Trailing if ($host = www.issuetopr.dev) {
return 301 https://$host$request_uri;
}
if ($host = issuetopr.dev) {
return 301 https://$host$request_uri;
}
listen 80;
server_name issuetopr.dev www.issuetopr.dev;
return 404;The DocumentationThe docs are comprehensive and well-organized. The One suggestion: the deployment guide's "Production Checklist" section recommends things (log rotation, monitoring) that could/should be included in the actual configs rather than left as manual follow-ups. SummaryThe architecture is sound — compose profiles, external preview network, certbot with Porkbun DNS-01, and the wildcard subdomain routing pattern are all well-designed. The main gaps are security hardening (headers, rate limiting) and compression. The WebSocket |
youngchingjui
changed the title
- Removed the `certbot-porkbun` directory and its associated Dockerfile as SSL management is now handled differently. - Updated `docker/nginx` configuration files to simplify SSL handling and improve readability, including changes to server blocks and proxy settings. - Enhanced the `docker/nginx/README.md` to clarify the routing and SSL setup processes, emphasizing the use of HTTP for local development. - Streamlined the deployment documentation in `docs/deployment/README.md`, focusing on the updated SSL setup and removing unnecessary steps. These changes aim to improve the clarity and maintainability of the Docker setup and related documentation.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docker/nginx/conf.d/preview.issuetopr.dev.conf (1)
25-28: Consider using the defaultsub_filter_once onfor single-replacement scenarios.Valid HTML documents have exactly one
</body>tag, so the defaultsub_filter_once onwould suffice and avoids scanning the entire response body after the first replacement. If set tooffintentionally to handle malformed HTML edge cases, this is fine as-is.♻️ Optional simplification
sub_filter '</body>' '<script>try{console.log("[IssueToPR Preview] This site is securely hosted and managed by Issue To PR.");}catch(e){}</script></body>'; - sub_filter_once off; + sub_filter_once on;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/nginx/conf.d/preview.issuetopr.dev.conf` around lines 25 - 28, The nginx config currently forces sub_filter_once off which causes NGINX to scan the whole response for multiple '</body>' matches; change sub_filter_once to the default "on" so the single replacement of '</body>' (handled by the existing sub_filter) stops after the first match—update the sub_filter_once directive (and keep the sub_filter line unchanged) unless you intentionally need off to handle malformed HTML.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docker/nginx/conf.d/preview.issuetopr.dev.conf`:
- Around line 25-28: The nginx config currently forces sub_filter_once off which
causes NGINX to scan the whole response for multiple '</body>' matches; change
sub_filter_once to the default "on" so the single replacement of '</body>'
(handled by the existing sub_filter) stops after the first match—update the
sub_filter_once directive (and keep the sub_filter line unchanged) unless you
intentionally need off to handle malformed HTML.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 93ee47dd-c84c-493d-bcde-f7a2b906c32d
📒 Files selected for processing (8)
docker/README.mddocker/compose/nginx.ymldocker/nginx/README.mddocker/nginx/conf.d/grafana.issuetopr.dev.confdocker/nginx/conf.d/issuetopr.dev.confdocker/nginx/conf.d/preview.issuetopr.dev.confdocker/nginx/nginx.confdocs/deployment/README.md
🚧 Files skipped from review as they are similar to previous changes (7)
- docker/README.md
- docker/nginx/README.md
- docker/nginx/conf.d/issuetopr.dev.conf
- docker/compose/nginx.yml
- docs/deployment/README.md
- docker/nginx/conf.d/grafana.issuetopr.dev.conf
- docker/nginx/nginx.conf
|
You know I feel these changes all need to be grounded in some sort of user experience or developer experience outline or goal, so we can assess if this is the correct implementation. I think from the users perspective, they won't see much of a difference yet, except to avoid non renewed SSL certificates. So this is more of a developer devops change, and so we should also describe those goals clearly in our documentation somewhere in /docs. My thinking is that we need some level of single-command line deployment on a server to get everything up and running for issue to PR. Or at least a single command for just the basic level of issue to pr. And have additional modules available (like HTTPS, preview domains, etc) also setup with additional (single-line) commands. So then we'd need to think about fencing off modules, and having a clear diagram and commands about what does what. I'm not sure if we have clear docs about this yet, but that should be our guiding star. Any implementation detail in here should be tied to those goals (UX or DX). Do we have that mapping here? @codex |
Summary
Testing
|
|
|
||
| ```bash | ||
| docker network create preview | ||
| docker compose -f docker/docker-compose.yml --profile prod up -d |
There was a problem hiding this comment.
I think this doesn't fully run the issue to pr NextJS app. That's for another issue, so we'll just need a placeholder of some sort, either in docker compose or we add to documentation here for now that you have to launch NextJS app manually or setup systemd
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/deployment/README.md (1)
9-14:⚠️ Potential issue | 🟠 MajorLocal Development command is incomplete/misleading.
pnpm devdoes not provision infra; the text on Line 13 says it starts Neo4j/Redis, which conflicts with the compose-based setup shown elsewhere. This can block first-time setup.Suggested doc fix
### Local Development ```bash -pnpm dev +docker compose -f docker/docker-compose.yml up -d +pnpm dev-Starts Neo4j and Redis. Run Next.js and workers locally with hot reload.
+Starts Neo4j and Redis via Docker, then runs the app locally with hot reload.
+If workers are needed locally, start them separately (or document the exact command here).</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/deployment/README.mdaround lines 9 - 14, Replace the misleading single
command "pnpm dev" with an explicit two-step instruction: first run the Docker
compose command that provisions Neo4j and Redis (reference the compose file
shown elsewhere, e.g., docker compose -f docker/docker-compose.yml up -d) and
then run the local dev server (pnpm dev); update the surrounding prose that
currently says "Starts Neo4j and Redis. Run Next.js and workers locally with hot
reload." to something like "Starts Neo4j and Redis via Docker, then runs the app
locally with hot reload." and add a short note about starting workers separately
(or include the exact worker start command) so readers aren’t misled into
thinking pnpm dev provisions infra.</details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (1)</summary><blockquote> <details> <summary>docs/dev/README.md (1)</summary><blockquote> `26-39`: **Consider moving the migration backlog out of this index page.** The “TODO: Migrate existing docs here” section turns this file into both a docs index and a migration tracker. Prefer a separate migration-tracking doc/issue and keep this page as the stable navigation entry point. As per coding guidelines, “Each documentation file should cover a single topic or component” and docs should be “broken down into logical, focused files.” <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/dev/README.mdaround lines 26 - 39, The "TODO: Migrate existing docs
here" migration backlog in docs/dev/README.md should be moved out of the index
page: remove the large migration bullet list under the "TODO: Migrate existing
docs here" header from docs/dev/README.md and create a separate
migration-tracking doc (e.g., docs/dev/migration-backlog.md) that contains the
listed mappings (docs/deployment/README.md → docs/dev/deployment.md, etc.);
leave docs/dev/README.md as a stable navigation entry that links to the new
migration-tracking file and to the finalized dev docs (architecture,
authentication, ai-integration, etc.) so the index only covers navigation and
not migration state.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In@docs/deployment/README.md:
- Around line 9-14: Replace the misleading single command "pnpm dev" with an
explicit two-step instruction: first run the Docker compose command that
provisions Neo4j and Redis (reference the compose file shown elsewhere, e.g.,
docker compose -f docker/docker-compose.yml up -d) and then run the local dev
server (pnpm dev); update the surrounding prose that currently says "Starts
Neo4j and Redis. Run Next.js and workers locally with hot reload." to something
like "Starts Neo4j and Redis via Docker, then runs the app locally with hot
reload." and add a short note about starting workers separately (or include the
exact worker start command) so readers aren’t misled into thinking pnpm dev
provisions infra.
Nitpick comments:
In@docs/dev/README.md:
- Around line 26-39: The "TODO: Migrate existing docs here" migration backlog in
docs/dev/README.md should be moved out of the index page: remove the large
migration bullet list under the "TODO: Migrate existing docs here" header from
docs/dev/README.md and create a separate migration-tracking doc (e.g.,
docs/dev/migration-backlog.md) that contains the listed mappings
(docs/deployment/README.md → docs/dev/deployment.md, etc.); leave
docs/dev/README.md as a stable navigation entry that links to the new
migration-tracking file and to the finalized dev docs (architecture,
authentication, ai-integration, etc.) so the index only covers navigation and
not migration state.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `2cbfe277-cd21-48d6-a928-87af38ecbb95` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between f657066406f64d219d7d1daa9cba45e22dc92e54 and 98b538b3a255d9d4bcf6e48483714ad05eadc2e9. </details> <details> <summary>📒 Files selected for processing (5)</summary> * `CLAUDE.md` * `docs/deployment/README.md` * `docs/dev/README.md` * `docs/dev/infrastructure.md` * `docs/user/README.md` </details> <details> <summary>✅ Files skipped from review due to trivial changes (2)</summary> * docs/dev/infrastructure.md * docs/user/README.md </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
1 participant
Summary
*.issuetopr.dev) for ephemeral preview deployments, with dynamic Docker container resolutiongrafana.issuetopr.devCLAUDE.mdwith workflow guidelines and README index entriesTest plan
Local (no SSL certs required)
These can be verified on any dev machine:
porkbun.iniand confirm clear error message:docker compose -f docker/docker-compose.yml --profile prod up certbot # Should exit with: [ERROR] Missing secret: /etc/letsencrypt/secrets/porkbun.inidocker compose -f docker/docker-compose.yml --profile prod config --services # Should list: neo4j, redis, workflow-workers, nginx, certbotdocker/nginx/README.mdanddocs/deployment/README.mdare accurate and internally consistentProduction server (requires SSL certs + DNS)
These must be tested on the production server where Let's Encrypt certs exist:
docker compose -f docker/docker-compose.yml --profile prod up -d nginx— container stays healthydocker compose -f docker/docker-compose.yml exec nginx nginx -tcurl -I http://issuetopr.devreturns 301 to httpshttps://issuetopr.devserves the Next.js apphttps://grafana.issuetopr.devserves the Grafana dashboardhttps://issuetopr.dev/api/ssestreams events without buffering issuesConnectionheader handling)previewnetwork with an alias, confirmhttps://<alias>.issuetopr.devresolves to itdocker compose -f docker/docker-compose.yml --profile prod run --rm certbot renew --dry-rundocker inspect issue-to-pr-nginx-1 --format='{{.State.Health.Status}}'showshealthyFollow-up items (out of scope for this PR)
limit_req_zone)nginx.conf$connection_upgrademap variable instead of hardcoded'upgrade'for WebSocket headersnginx.yml🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation