docs(onboarding): make .env.example the canonical dev template (closes #260) - #264
docs(onboarding): make .env.example the canonical dev template (closes #260)#264dchaudhari7177 wants to merge 3 commits into
Conversation
cofade
left a comment
There was a problem hiding this comment.
Code review
Found 1 issue:
- The template's
DUCKDB_SERVICE_URLcomment is wrong about the backend. It says the variable is optional for "the backend and image-service" because "it defaults to this exact value, which is the Docker service name". That holds for image-service (image-service/app.py'sos.getenv("DUCKDB_SERVICE_URL", "http://duckdb-service:8000")), but the backend's code default isDEFAULT_DUCKDB_URL = 'http://127.0.0.1:8002'— deliberately the host-port mapping for a backend running on the host vianpm run dev. The compose stack works regardless becausedocker-compose.yml's backendenvironment:block pinsDUCKDB_SERVICE_URL: http://duckdb-service:8000. Since this file now declares itself the one description of the dev env, it should not restate the backend's default incorrectly — that is the PR's own thesis.
Lines 18 to 23 in ef43e1a
highfive/backend/src/duckdbClient.ts
Lines 1 to 8 in 205bcb7
Lines 10 to 23 in 205bcb7
Suggested wording:
# Where image-service reaches the DuckDB service. Optional — image-service
# defaults to this exact value (the Docker service name), and in compose the
# backend gets it from docker-compose.yml's `environment:` block. (The
# backend's own code default is the host-side http://127.0.0.1:8002, for
# `npm run dev` outside compose.) Only change it if you run the services
# outside compose.
Nits, non-blocking:
DISCORD_WEBHOOK_URLis also read from the root.envin dev, via compose interpolation (${DISCORD_WEBHOOK_URL:-}indocker-compose.yml), and is not in the template. A commented# DISCORD_WEBHOOK_URL=line would make the "one description" claim fully true.- The old "VITE_API_KEY was removed (#142)" tripwire comment was dropped. ADR-019 covers it, but it is free to keep.
- Yes, please fold in
.claude/skills/esp32-onboarding/SKILL.md:32— it is the fifth restatement and drifts the same way. README.mdhas no production pointer at all; a one-liner under the quick start ("Deploying? See.env.production.exampleanddocs/07-deployment-view/production-deployment.md") would fit here. That is the one useful idea in the duplicate #266, which points at the PM2 runbook by mistake.
Everything else verified against the tree: the env_file wiring (backend / image-service / duckdb-service; homepage has none), the os.getenv defaults, the cd hivehive correction, and the troubleshooting.md correction. Test-merges cleanly with #263 and #265; scripts/check-doc-citations.sh reports 7 OK on the merged tree.
🤖 Generated with Claude Code
|
Good catch on the backend default — that was the one thing in the file that was actually wrong, and wrong in exactly the way the file exists to prevent. All five points are in. The blocker. Nits, all taken:
|
cofade
left a comment
There was a problem hiding this comment.
senior-reviewer — good change, one P1 wording correction before merge
You flagged that you couldn't run the senior-reviewer gate in your environment. Consider this that gate. MERGEABLE / CLEAN, docs + one template only.
Claims verified against the tree, not the PR body
| claim | verdict |
|---|---|
image-service defaults DUCKDB_SERVICE_URL to the compose service name |
✅ image-service/app.py's module-level DUCKDB_SERVICE_URL |
backend's code default is http://127.0.0.1:8002 |
✅ backend/src/duckdbClient.ts's DEFAULT_DUCKDB_URL; pinned in compose by the backend service's environment: block |
DEBUG optional in both Flask services |
✅ os.getenv("DEBUG", "false") in both image-service/app.py and duckdb-service/app.py |
env_file: .env on backend, image-service, duckdb-service — not homepage |
✅ three env_file: keys in docker-compose.yml; the homepage service has none |
DISCORD_WEBHOOK_URL passes through as ${DISCORD_WEBHOOK_URL:-} |
✅ both Python services |
dropping VITE_API_URL from .env.example is safe |
✅ production supplies it as a build arg in docker-compose.prod.yml, never from .env — so it genuinely never reached homepage |
Your second commit's correction of the backend's DuckDB default is the right call, and the distinction it draws (code default vs. what compose pins) is the part that would otherwise have drifted again.
Removing HIGHFIVE_API_KEY=your_secure_production_key_here from a file the README tells you to cp is a real security improvement, not tidying — that placeholder was becoming the admin password of every fresh checkout. Same category as the incident in ADR-019.
P1 — the rewritten troubleshooting.md entry can now mislead
The entry says "The most common cause is a missing or malformed .env" and then "no variable in it is actually required." Read together, that reads as though the file is optional. It is not. With no .env at the repo root:
$ docker compose config -q
env file C:\...\highfive\.env not found: GetFileAttributesEx C:\...\highfive\.env: The system cannot find the file specified.That hard failure is exactly why scripts/check-duckdb-bind-claims.sh manufactures a throwaway .env and removes it again in its EXIT trap. Please state the string compose actually prints — it is the symptom the section exists to resolve, and it is the one thing a stuck contributor can grep for. The same gap is in the CLAUDE.md hunk, which drops the word "Required" from the heading while the file remains mandatory.
P2 — unrelated table churn in the skill file
Nine lines of column-padding realignment in the gotchas table of .claude/skills/esp32-onboarding/SKILL.md, unrelated to the .env change. Cosmetic, inflates the diff; not blocking.
P2 — a tradeoff worth naming, not an objection
CLAUDE.md is auto-loaded into every agent session; .env.example is not. Trading the inline block for a pointer is right for drift, but costs a file read to learn the same three variables. Your wording ("every variable in it is optional; the stack boots on the defaults") mostly compensates, so I'd keep it as-is — flagging the tradeoff so it's a decision rather than an accident.
Chapter 11 entry
In the required format, and the abstraction is the right one: a file that ships as a template is the canonical description of itself — docs link to it, they do not restate it. Good.
Verdict
Approve once the P1 wording is fixed. Please also rebase onto main first — #272 landed new ts-quality, python-lint and repo-guards CI jobs after this branch's last run, so the current green checks don't cover them.
To answer the open question in your description: yes, folding in .claude/skills/esp32-onboarding/SKILL.md was the right call — a fifth restatement in agent tooling drifts the same way, and agents are exactly the readers who will not notice.
|
For context: my review above was written against |
README's quick start says cp .env.example .env, but that file was headed "HighFive Production Environment Variables" and carried NODE_ENV=production, PORT=3001, a production VITE_API_URL and a placeholder admin key. Compose loads it into backend, image-service and duckdb-service, and the environment: block overrides NODE_ENV and PORT, so the dev stack booted while the placeholder silently became the admin password of every fresh checkout. Rewrite it as the dev template: DEBUG, DUCKDB_SERVICE_URL and a commented HIGHFIVE_API_KEY, each annotated with what reads it and the fact that all three are optional. Production wording now lives only in .env.production.example, which is what docker-compose.prod.yml consumes. Collapse the four restatements into pointers. CLAUDE.md, CONTRIBUTING.md, docs/07-deployment-view/docker-compose.md and docs/troubleshooting.md each described a different dev .env; they now link to the template instead. troubleshooting.md also claimed DEBUG and DUCKDB_SERVICE_URL were required "at minimum", which the getenv defaults in both Flask services contradict. Also fixes the wrong directory in docker-compose.md: cd hivehive after cloning highfive.git. Lesson recorded in docs/11-risks-and-technical-debt per CLAUDE.md. Refs schutera#260
The template said DUCKDB_SERVICE_URL defaults to the compose service name for both services. That is only true of image-service; the backend's code default is DEFAULT_DUCKDB_URL = 'http://127.0.0.1:8002' (the host-port mapping, for a backend run outside compose), and in compose it is pinned by the environment: block rather than read from this file. Restating a default incorrectly is the exact drift this file exists to stop. Also: add the commented DISCORD_WEBHOOK_URL line, since compose interpolates it from the root .env; restore the VITE_ tripwire note; point the ESP32 onboarding skill at the template instead of restating the values a fifth time; and give the README a production pointer, which it had none of.
Review P1: the troubleshooting entry paired "the most common cause is a
missing .env" with "no variable in it is actually required", which reads as
though the file is optional. It is not -- docker-compose.yml declares
env_file: .env on backend, image-service and duckdb-service, so compose
refuses to render the stack at all when it is absent.
State the string compose actually prints, since that is the symptom a stuck
contributor can grep for:
env file /path/to/highfive/.env not found: ...
The tail after "not found:" is the OS stat error and differs between Linux
and Windows, so only the stable prefix is quoted. Reproduced locally on
Docker 27.5.1 (CreateFile ...: The system cannot find the file specified).
Also note that an *empty* .env boots the stack -- that is precisely why
scripts/check-duckdb-bind-claims.sh manufactures a throwaway one in CI --
so a service exiting with a .env present is failing on a syntax error, not
a missing value.
CLAUDE.md keeps the word "Required" on the heading for the same reason, and
now draws the file-vs-variables distinction explicitly.
Reverts the nine lines of unrelated column-padding realignment in the
esp32-onboarding gotchas table (review P2); only the .env line changes there.
67e0304 to
badae82
Compare
|
Thanks — P1 addressed and rebased onto P1 — $ docker compose config -q
env file /path/to/highfive/.env not found: ...I reproduced it here on Docker 27.5.1 and got The entry now also notes that an empty
P2 — table churn. Reverted. The esp32-onboarding skill file is back to a one-line diff; the nine lines of column realignment were prettier reflowing the table after my edit, and they add nothing. P2 —
|
Senior-reviewer re-review -- needs changes before mergeThe earlier P1 about the production-shaped P1 findings
Architectural smellThe PR declares Requested next stepAddress the three P1 findings, then rerun the documentation-link and Compose validation checks and request another review. The dev-only |
Addresses #260 (auto-close keyword kept in the title only, per CLAUDE.md).
Documentation and one template file. No code, no compose change.
.env.examplebecomes the dev templateIt now carries
DEBUG,DUCKDB_SERVICE_URLand a commentedHIGHFIVE_API_KEY, each annotated with which service reads it and the fact that all three are optional. Gone: the "Production Environment Variables" heading,NODE_ENV=production,PORT=3001, the productionVITE_API_URL, andHIGHFIVE_API_KEY=your_secure_production_key_here— the placeholder that was becoming the admin password of every fresh dev box.Production wording now points exclusively at
.env.production.example, which is whatdocker-compose.prod.ymlactually consumes, and the header says so explicitly so nobody copies the wrong one.Four restatements become four pointers
CLAUDE.md.env" blockcp .env.example .env+ pointerCONTRIBUTING.mddocs/07-deployment-view/docker-compose.mddocs/troubleshooting.mdAlso fixed:
docker-compose.mdsaidcd hivehiveafter cloninghighfive.git.Two things I found beyond the issue
troubleshooting.mdwas actively wrong. It said the.env"must contain at minimum"DEBUGandDUCKDB_SERVICE_URL. Both are optional —image-service/app.py:512andduckdb-service/app.py:123areos.getenv("DEBUG", "false"), andimage-service/app.py:159defaultsDUCKDB_SERVICE_URLto the service name. So the one doc a stuck contributor reaches for was sending them to fix a non-problem. It now says to restore from the template, and that a service dying on a malformed.envis failing on a parse error (docker compose configshows it), not a missing value.A fifth restatement exists that I did not touch:
.claude/skills/esp32-onboarding/SKILL.md:32tells the agent to confirm.envcontainsDEBUG=trueandDUCKDB_SERVICE_URL=.... Same drift, but it is agent tooling rather than contributor docs, so I left the call to you — happy to fold it in.Claims verified against the tree
env_file: - .envappears atdocker-compose.yml:10(backend),:78(image-service),:131(duckdb-service); thehomepageservice has noenv_file, soVITE_API_URLnever reached it. The docs now say exactly this — the old CONTRIBUTING wording ("used byimage-serviceandduckdb-service") omittedbackend.os.getenvdefaults cited above, not assumed.Docs gate
docs/11-risks-and-technical-debt/README.mdgains a Lessons-learned entry in the required format. The general lesson: a file that ships as a template is the canonical description of itself — docs link to it, they do not restate it. Five copies meant no single one was wrong enough to notice, and the composeenvironment:overrides hid the mismatch, which is structurally the same failure as the inert-security-control incident already recorded in that chapter.make check-citations→ 7 OK, 0 problems.Note: my other open PR (#263) also appends to the Lessons-learned section. I inserted at a different anchor so the two should not textually conflict, but if they do it is a one-line rebase — say the word.
One deviation to flag
CLAUDE.md's end-of-implementation gate asks for the
senior-reviewersubagent. I could not run it in my environment, so this has not been through that gate. Everything else —make check-citations, the docs update, branch and commit conventions, the no-auto-close-keyword-in-bodies rule — has been followed.