Add Terraform for the API host, block volume and Vercel wiring - #4
Merged
Conversation
Both factories advertised multi-provider support behind real ABCs but only ever constructed public-OpenAI clients. This adds azure_openai to each. Approach: target Azure's v1 OpenAI-compatible surface (<endpoint>/openai/v1) and reuse the standard AsyncOpenAI client rather than AsyncAzureOpenAI, whose static types the openai SDK's own README warns "can be incorrect". So the Azure branch is a different base_url and a deployment name, not a second client implementation. - config.py: azure_openai_endpoint / _api_key / _deployment / _embedding_deployment / _tokenizer_model, plus azure_openai_base_url() which normalises the endpoint to /openai/v1 idempotently (accepts bare host, trailing slash, or an already-complete URL). - llm/factory.py + embeddings/factory.py: azure_openai branch, failing fast with a named variable when endpoint / key / deployment are missing. - openai_llm.py + openai_embeddings.py: api_key widened to str | Callable[[], str] so an Entra token provider can be passed without either class knowing how the credential is obtained. Nothing here supplies one yet -- auth is API-key only. - requirements.txt: openai>=1.106.0, the floor Microsoft documents for the v1 surface and callable token providers, and the first version exporting the error classes the health check now discriminates on. Was >=1.12.0. Two Azure divergences that would otherwise be silent: - health_check no longer treats a missing /models route as unhealthy. On Azure that route enumerates *deployments* and some configurations omit it entirely; a 404 means the endpoint answered, so credentials and networking are fine. 401/403 and unexpected statuses still fail. Previously /api/health would have reported a working Azure deployment as degraded. - openai_embeddings.py takes tokenizer_model separately, because tiktoken resolves an encoding from a model id and on Azure `model` is a deployment name. Note honestly that the old bare `except KeyError: cl100k_base` was *accidentally* correct: every current OpenAI embedding model resolves to cl100k_base anyway. So this is a latent correctness fix plus a warning where there was silence, not a live bug fix. It would have mattered on an o200k_base embedding model or a non-OpenAI base_url. Also, because Azure makes them reachable: - openai_embedding_dimensions is now configurable and threaded through both factories. It was hardcoded to 1536 while the model was configurable, so a text-embedding-3-large deployment (3072) only failed when Chroma rejected the insert. - embeddings/factory.py's unknown-provider branch now raises instead of falling back to OpenAI whenever a key happened to be set. That fallback dropped all seven rate-limit, batching and pacing arguments, so a typo in EMBEDDING_PROVIDER silently produced a differently-behaving client with no error. llm/factory.py already raised. docker-compose.yml forwards the six new variables (55 total), and .env.example plus docker/README.md document deployment-names-not-model-ids, the tokenizer requirement, the dimensions match, and that Entra is not wired up. Verified: 22 new unit tests covering URL normalisation, factory wiring, missing-config errors, tokenizer resolution, dimensions and all five health-check branches; 111 tests pass (was 89); ruff clean; both providers construct correctly from environment alone and the default OpenAI path is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The project had no deployed backend. apps/web has been live on Vercel, but
apps/web/src/lib/api-client.ts:1 reads NEXT_PUBLIC_API_URL and falls back to
http://localhost:8000, and nothing ever set that variable -- so the deployed frontend
could not reach an API. docker/docker-compose.yml was the only deployment artifact and
it is a local-development file.
Creates: droplet running docker/docker-compose.yml via cloud-init, a block volume for
the SQLite DB + Chroma + clones, volume attachment, firewall, project grouping, and the
Vercel environment variable set from the droplet's address -- one stack's output feeding
the other's input. ~$7/month at the defaults.
Host choice, since it differs from the obvious answer: Fly.io was the plan, but its
Terraform provider is abandoned -- fly-apps/fly is still 0.0.23 published 2023-06-22,
and the community fork last shipped 2024-10-28. Managing Fly through that would defeat
the purpose of using Terraform. digitalocean/digitalocean is a partner provider with
13.3M downloads updated within the last week, and a Droplet provides a real block
device, which is what SQLite and Chroma require. Azure Container Apps was rejected for
the opposite reason: no block-device volume type, only Azure Files over SMB/NFS, which
is the configuration sqlite.org/howtocorrupt.html section 2.1 warns against.
Security decisions, all deliberate:
- .gitignore covers *.tfstate, *.tfvars and *.tfplan, and is committed in the same
change so it cannot be forgotten before a first apply. State holds every resolved
secret in plaintext; `sensitive = true` only redacts CLI output.
- .terraform.lock.hcl IS committed, pinning provider checksums.
- Both providers pinned (~> 2.99, ~> 5.10). Both ship several releases a month.
- No inbound rule for 6379. Compose publishes Redis for local dev, which on a public
droplet is an unauthenticated Redis facing the internet; the DO firewall omits it and
ufw on the host allows only 22 and 8000, so the exposure does not rest on one control.
- ssh_allowed_cidrs defaults to [] rather than 0.0.0.0/0, so SSH access is an explicit
choice.
- cloud-init writes /etc/codebaseqa.env at 0600 and never formats a volume that already
has a filesystem.
Verified: terraform validate passes against the real digitalocean 2.99.1 and vercel
5.10.0 schemas, terraform fmt -check is clean, and nothing secret is staged. Validation
caught two genuine errors that reading alone would not have: ${VAR} inside a comment in
the cloud-init template was parsed as a template reference and had to be escaped, and
vercel_project_environment_variable requires an explicit `sensitive` argument in v5.
NOT applied. No plan or apply has run -- that needs live credentials and creates
billable resources. README states this, plus the limitations that matter: it serves
plain HTTP so a browser on the https Vercel page will block it as mixed content until
TLS is added; single instance only, because main.py runs migrations unguarded on every
startup; and local state, single operator, no remote backend or locking.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
The project had no deployed backend.
apps/webhas been live on Vercel, butapps/web/src/lib/api-client.ts:1readsNEXT_PUBLIC_API_URLand falls back tohttp://localhost:8000— and nothing ever set that variable, so the deployed frontend could not reach an API.docker/docker-compose.ymlwas the only deployment artifact and it is a local-development file.What it creates
Droplet running
docker/docker-compose.ymlvia cloud-init, a block volume for the SQLite DB + Chroma + clones, volume attachment, firewall, project grouping, and the Vercel env var set from the droplet's address — one stack's output feeding the other's input. ~$7/month at the defaults.Host choice differs from the obvious answer, on evidence
Fly.io was the plan. Its Terraform provider is abandoned:
fly-apps/flyandrewbaxter/fly(fork)digitalocean/digitaloceanvercel/vercelManaging Fly through a provider stuck on 0.0.23 for three years would defeat the purpose of using Terraform. A Droplet also gives a plain block device, which SQLite and Chroma both require — Azure Container Apps was rejected for the same reason (no block-device volume type; only Azure Files over SMB/NFS, the configuration
sqlite.org/howtocorrupt.html§2.1 warns against).Security decisions, all deliberate
.gitignorecovers*.tfstate,*.tfvars,*.tfplan, and ships in this same commit so it cannot be forgotten before a firstapply. State holds every resolved secret in plaintext —sensitive = trueonly redacts CLI output, never state..terraform.lock.hclIS committed, pinning provider checksums.~> 2.99,~> 5.10). Both ship several releases a month.ufwallows only 22/8000, so the exposure doesn't rest on a single control.ssh_allowed_cidrsdefaults to[], not0.0.0.0/0— SSH access is an explicit choice./etc/codebaseqa.envat0600and never formats a volume that already has a filesystem.Verification
terraform validatepasses against the real digitalocean 2.99.1 and vercel 5.10.0 schemas;terraform fmt -checkclean; nothing secret staged.Validation earned its keep — it caught two errors that reading alone would not have:
${VAR}inside a comment in the cloud-init template was parsed as a template reference and failed the build. Escaped to$${VAR}.vercel_project_environment_variablerequires an explicitsensitiveargument in v5.Not applied
No
planorapplyhas run. That needs live credentials and creates billable resources, so it's yours to run. Limitations the README states up front rather than leaving to be discovered:https://Vercel page will blockhttp://IP:8000as mixed content —curlwill work while the site doesn't. Finishing means a domain + TLS, thenNEXT_PUBLIC_API_URLon thehttps://name.main.pyrunsinit_db+run_pending_migrationsunguarded on every startup and Chroma holds a process-local client, so two replicas would race theALTER TABLEs.NEXT_PUBLIC_API_URLis inlined at build time, so setting the Vercel variable requires a frontend redeploy to take effect. Called out interraform output next_steps.🤖 Generated with Claude Code