Make docker compose up the front door - #35
Merged
Merged
Conversation
Both Dockerfiles build with `COPY . .` and there was no ignore file, so every image carried the whole working tree -- including `.env`, which holds a real GitHub client secret and the session secret. A secret copied into a layer stays in that layer regardless of what the running container does with it. This lands first and alone because it is the one fault that is actively dangerous rather than merely broken: the other seven stop the stack from working, this one would have shipped credentials. Also keeps host `node_modules` out (wrong platform anyway -- the parser's tree-sitter bindings are compiled), stale `dist` out, and test files out, which incidentally closes the compiled-tests-in-dist gap CLAUDE.md records as "harmless locally, wrong in an image".
Four faults in one commit, because they only fail one at a time. The build context was `./apps/api` while the Dockerfile COPYs workspace manifests and packages/shared from above it, so the build died on its first COPY. It is the repo root now, with `dockerfile:` pointing here. The runner never copied packages/shared and ran `node dist/index.js`, but that package exports `./src/*.ts` and node cannot follow a `.ts` export. The image ships shared as source and runs under tsx, which leaves `pnpm dev`, Vite and Vitest resolving exactly as they did. `pnpm start` was `node dist/index.js` and could never have worked either; it is `tsx src/index.ts` now, closing a gap CLAUDE.md had recorded. There was no worker service at all. A webhook verified its HMAC, enqueued a job, answered 202, and nothing ever consumed it -- incremental refresh silently did nothing, which reads as a slow parse rather than as a missing process. It is the same image with a different command, since the two differ only in entrypoint. Three things found by building it rather than reading it: pnpm 11.15.1 cannot run on Node 20. It uses a builtin the runtime does not have and dies with ERR_UNKNOWN_BUILTIN_MODULE before installing anything, so both stages are node:24-alpine. The worker needs `git` in the image. `internal/clone/clone.go` shells out to `git clone`, and a bare Alpine has none -- the existing parser image has the same hole, which went unnoticed because the isolation harness only ever parses a local fixture. `.env` is written for host-native `make start`, where Postgres and Redis are on localhost. Inside a container localhost is the container, so DATABASE_URL and REDIS_URL are overridden with service names; `environment` beats `env_file`. WEB_APP_URL, CORS_ORIGIN and GITHUB_REDIRECT_URI stay host-facing because the browser is on the host. The API port is published on 127.0.0.1 rather than 0.0.0.0. Verified running: healthz answers 200, /api/repos answers 401, the worker attaches to the queue, the parser binary and git are both present in the image, and no .env was baked into a layer.
…ervice Three of the remaining faults. Nothing ran migrations in compose. `make start` uses the golang-migrate CLI, which someone running `docker compose up` has no reason to have installed, so the API and worker would have reached a database with no tables and failed in a way that reads as a code bug. A `migrate` service on the image CI already uses (R17), and both consumers gate on `service_completed_successfully` -- finished, not merely started. `apps/web` had no Dockerfile at all, so `docker compose build` failed here first. It is a Vite build served by nginx, with `try_files` doing the SPA fallback `vercel.json` does on Vercel -- without it a refresh on /app is a 404. Asset requests deliberately do not fall back: a missing bundle should 404 rather than quietly return the HTML shell, which is how a broken build disguises itself as a routing bug. VITE_API_URL is a build ARG because Vite inlines it; setting it on the container would do nothing. The parser service declared `network_mode: none` and a dependency on Postgres health, which cannot both hold -- with no interfaces it can never reach Postgres over TCP. The dependency was vestigial and nobody hit it because `make parser-isolated` runs it with `--no-deps`. It is gone; the three isolation settings stay, since those are the actual contract. A `tools` profile keeps `docker compose up` from starting a container the product never uses (R38). Two more found by building rather than reading. `tsconfig.json` extends `@funcatlas/typescript-config/base.json`, so omitting that workspace package made tsc fall back to its own defaults and report forty errors that all traced to one missing file. And `pnpm run` verifies the workspace before executing a script, so the build stage needs the lockfile and every manifest, not just the ones the bundle imports -- otherwise it aborts with ERR_PNPM_WORKSPACE_PKG_NOT_FOUND attributed to the bundler. Verified with the stack actually up: migrate applies and exits, / and /app both answer 200, a missing asset answers 404, healthz answers 200, and the parser container is not started.
Registering a GitHub OAuth app was the largest thing between a clone and a running stack. FUNCATLAS_SINGLE_USER removes it: the API registers no /auth/login, /auth/callback or /auth/logout and resolves every request to that user. The branch lives in `requireSession`, which is the only function that ever sets `req.session` -- a second hook would be a parallel auth path, and those drift until one forgets a check. The OAuth routes are not registered rather than registered-and-refusing: 404 says nothing, 401 tells a prober there is something here worth finding credentials for, which is the reasoning that shaped R30. This is not R30 in a new hat. That was a session-minting endpoint shipped in every deployment behind a NODE_ENV gate, so the gate had to be right exactly once. Here there is no endpoint and no gate: the instance is unauthenticated because a human wrote their own username into their own .env. What it is not is safe to expose, and I said otherwise when proposing it. The process binds 0.0.0.0 -- it must, inside a container, or Docker cannot reach it -- so an in-process loopback check is impossible. Compose publishes on 127.0.0.1, the server warns on every start, and R39 records the residual risk instead of implying it is closed. Three things this dragged in. The GitHub OAuth credentials had to become conditionally required, or the API still refused to start without an app it would never call; the check runs at startup, naming which are missing and both ways to fix it, rather than surfacing as a 500 on first sign-in. The arctic client is built lazily for the same reason. And a blank FUNCATLAS_SINGLE_USER= now means off, because answering "I tried to turn this off" with a Zod stack trace about string length is a bad first impression. `make setup` writes .env, generates both secrets, creates the test database and prints what it did -- including why the instance now has no authentication. Idempotent, and it never overwrites an existing .env. Two test faults fixed on the way. env.test classified any key without `.default(` as required, so it demanded the newly optional ones in CI. And a running compose worker consumes the jobs queue/parse.test.ts asserts on, which reads as a broken queue rather than as two things sharing one Redis; `make test` now says so and stops instead of failing obscurely.
Three faults, all found by cloning into /tmp and running the thing rather than by reading it. `.env.example` shipped `GITHUB_CLIENT_ID=<client-id>`, and the Makefile sources that file with `set -a && . ./.env`. A shell reads `<` as a redirection, so every env-using target died with "newline unexpected" the moment `make setup` had copied it. The placeholders are empty values now, which is also what the schema wants, and the guidance moved into comments. `make setup` sourced .env to create the test database, which it does not need: the credentials there are compose's own. The one that actually mattered: `.optional()` only accepts a *missing* variable, and an empty value is a present empty string, so `.min(1)` rejected it. `GITHUB_CLIENT_ID=` -- the state a fresh .env is in, and the state anyone leaves a key they are not using -- crashed the API on startup complaining about the length of a credential single-user mode was never going to use. I had already fixed exactly this for FUNCATLAS_SINGLE_USER and left the three siblings broken, which is the whole argument for fixing at the shared point: it is now one `blankAsUnset` helper wrapping all four. Postgres and Redis published on 0.0.0.0 while everything else in this file was bound to 127.0.0.1. On a stack whose API runs unauthenticated, an internet-facing Postgres with the password `funcatlas` is the worse hole. They are published at all only because `make start` runs the API natively. Exit test passes from a clean clone with `make start` never run: `make setup` then `docker compose up`, register sindresorhus/p-limit through the API with no cookie, and the worker parses it to ready -- 6 files, 36 functions, commit SHA recorded. The startup warning fires, /auth/me answers without a cookie, and /auth/login is a 404.
The README told contributors `docker compose up` did not work and listed four prerequisites. It is two commands now -- `make setup && docker compose up` -- with Docker, make and openssl the only things needed, and the native path kept for anyone who wants hot reload. The single-user warning is stated where it will be read: in the quickstart, in "what it does not do", and in CLAUDE.md's known gaps. A README that buries "this has no authentication" under a heading nobody scrolls to is not honest just because the sentence exists somewhere. R39 records the residual risk and says plainly that the mitigation is weaker than the plan claimed -- the process binds 0.0.0.0 and cannot check what is in front of it, so the protection is a loopback port map and a human writing the value themselves, not a guard in code. R11 said prod runs everything in compose. That was the intent from Phase 0 and had never been true, which is how eight faults accumulated in a file nobody ran; it is true now and says when it became so. PRD NFR-4 likewise. Two CLAUDE.md gaps close with it: `pnpm start` could never run the API, and compiled test files landed in `apps/api/dist`.
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (23)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds a loopback-only Docker Compose runtime with migrations, API and worker containers, and nginx web serving. It adds ChangesLocal runtime and authentication
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The current changes introduce no identified merge-blocking correctness, security, availability, deployment, or runtime risk, so the PR is merge-ready after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Developer
participant MakeSetup
participant DockerCompose
participant PostgreSQL
participant Migrate
participant API
participant Worker
participant Nginx
Developer->>MakeSetup: Run make setup
MakeSetup->>MakeSetup: Create .env and generate secrets
MakeSetup->>DockerCompose: Start infrastructure
DockerCompose->>PostgreSQL: Start and wait for health
DockerCompose->>Migrate: Apply migrations
Migrate-->>DockerCompose: Report migration success
DockerCompose->>API: Start API
DockerCompose->>Worker: Start worker
DockerCompose->>Nginx: Serve built web assets
Nginx-->>Developer: Expose local web application
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
apps/api/package.jsonESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. apps/api/src/auth/github.tsESLint skipped: the matched ESLint configuration already failed (missing-dependency). apps/api/src/auth/routes.test.tsESLint skipped: the matched ESLint configuration already failed (missing-dependency).
Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/settings/billing. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Documentation