feat(redirect): migrate f3-redirect into the monorepo as apps/redirect#311
Draft
pstaylor-patrick wants to merge 4 commits into
Draft
feat(redirect): migrate f3-redirect into the monorepo as apps/redirect#311pstaylor-patrick wants to merge 4 commits into
pstaylor-patrick wants to merge 4 commits into
Conversation
pstaylor-patrick
requested review from
dnishiyama,
evanpetzoldt and
taterhead247
as code owners
📝 WalkthroughWalkthroughAdds a multi-tenant HTTPS redirect service: a Go redirect server and CLI with CertMagic+GCS cert storage, a Next.js admin web app (DB, auth, UI, API), Terraform GCP infra (Bucket, Artifact Registry, VM, Cloud Run mapping), Docker builds, CI/CD workflows, tests (unit, integration, parity, E2E), and tooling scripts/configs. ChangesF3 Redirect Service
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 34
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 46-49: Replace the moving tag uses: actions/setup-go@v5 with an
immutable pinned commit SHA in every CI job that currently uses that tag (search
for the literal "uses: actions/setup-go@v5"); update each occurrence to use the
corresponding full commit SHA for the actions/setup-go action (same major
version but pinned) so the workflow uses a fixed, verifiable reference instead
of the floating `@v5` tag.
In @.github/workflows/deploy-redirect-server.yml:
- Around line 64-72: The workflow currently pushes and boots the VM using the
mutable :latest tag (IMAGE:latest) which is non-deterministic; instead push only
the immutable build image (IMAGE:${GITHUB_SHA}) and obtain its content digest
(sha256) after push, then use that digest reference (IMAGE@sha256:<DIGEST>) or a
versioned tag (not :latest) when instructing the VM to pull/run; update the
steps that push IMAGE:latest and the VM reset logic so the VM startup pulls the
explicit IMAGE@sha256 digest (or the GITHUB_SHA tag) and remove
pushing/depending on :latest to ensure deterministic deploys (references: IMAGE,
GITHUB_SHA, INSTANCE, ZONE, PROJECT).
- Around line 43-51: Replace the mutable action references and image tag with
immutable SHAs and a deterministic image tag: pin actions currently referenced
as actions/checkout@v4, google-github-actions/auth@v2 and
google-github-actions/setup-gcloud@v2 to their corresponding commit SHAs (use
the commit SHA for each action release), and change any image push/pull using
IMAGE:latest to use the commit-specific tag (e.g. ${IMAGE}:${GITHUB_SHA} or
${IMAGE}:${{ github.sha }}), updating the workflow steps that build/push and the
VM pull step to use that same SHA-tagged image so deployments are deterministic.
In @.github/workflows/deploy-redirect-web.yml:
- Around line 45-52: Pin the GitHub Actions usages to immutable commit SHAs
instead of tags for the referenced actions (replace uses: actions/checkout@v4,
google-github-actions/auth@v2, google-github-actions/setup-gcloud@v2, and
google-github-actions/deploy-cloudrun@v2 with their corresponding commit-SHA
refs) and update the Docker build step that currently runs docker build --file
apps/redirect/web/Dockerfile . to use the app folder as the build context
(docker build --file apps/redirect/web/Dockerfile apps/redirect/web) or
alternatively add the necessary ignore patterns to the repository root
.dockerignore so apps/redirect/web/.dockerignore is honored.
- Around line 67-70: The workflow runs docker build with build context set to
repo root (the command using --file apps/redirect/web/Dockerfile and context
"."), so apps/redirect/web/.dockerignore is ignored and unwanted files get
included by the Dockerfile's COPY . .; fix by moving the ignore patterns from
apps/redirect/web/.dockerignore into the repository root .dockerignore or rename
apps/redirect/web/.dockerignore to apps/redirect/web/Dockerfile.dockerignore so
Docker will apply those patterns when building the Dockerfile at
apps/redirect/web/Dockerfile.
In `@apps/redirect/infra/terraform/main.tf`:
- Line 72: Replace the global default VPC binding (network = "default") with a
dedicated VPC and subnet and wire the VM and firewall resources to that network:
create/declare a google_compute_network (e.g., redirectd_network) and a
google_compute_subnetwork (e.g., redirectd_subnet) and update any
google_compute_instance and google_compute_firewall resources that currently use
network = "default" to reference the new network/subnet; tighten the firewall
rules to only allow the explicit ingress/egress ports required by redirectd
(remove broad SSH/project-global rules) and ensure the instance's
network_interface uses the new subnetwork.
- Around line 106-120: The instance metadata block needs explicit SSH hardening:
add metadata keys "block-project-ssh-keys" = "true" to prevent project-wide SSH
keys and, if SSH access is required, add "enable-oslogin" = "true" to enforce OS
Login; update the same metadata map where startup-script is set (the metadata
variable named metadata and the startup-script/templatefile usage) to include
these two keys so the instance enforces OS Login and blocks project keys.
- Around line 18-23: The google_storage_bucket resource "data" currently sets
uniform_bucket_level_access but not public access prevention; update the
resource block for google_storage_bucket.data to add public_access_prevention =
"enforced" so future IAM public grants are blocked—locate the resource named
google_storage_bucket "data" and insert the public_access_prevention attribute
with value "enforced" (then re-run terraform plan/apply).
In `@apps/redirect/infra/terraform/startup-script.sh.tftpl`:
- Line 5: Replace the shell options invocation that currently reads "set -uo
pipefail" with "set -euo pipefail" in the startup script template so the script
exits immediately on any command failure; update the invocation of the set
builtin (the "set -uo pipefail" line in
apps/redirect/infra/terraform/startup-script.sh.tftpl) accordingly and ensure no
other later code overrides the errexit behavior.
In `@apps/redirect/infra/terraform/variables.tf`:
- Around line 42-46: The variable "redirect_status" currently accepts any
string; add a Terraform validation block to the variable "redirect_status" that
enforces allowed values ("301" or "302") by using a condition like
contains(["301","302"], var.redirect_status) and provide a clear error_message
indicating the valid choices so invalid deploy-time values are rejected.
In `@apps/redirect/README.md`:
- Around line 100-104: Update the CI/CD subsection in README.md to match the
actual workflow files and triggers present in this PR: replace references to
`.github/workflows/ci.yaml` and `.github/workflows/deploy.yaml` and the
statement “every push to main” with the real workflow filenames and trigger
rules used in this PR (e.g., the actual CI workflow name, the actual deployment
workflow name, and their push/PR/branch/tag triggers), and correct any
deployment details (image push targets, VM rollout behavior, and auth method
such as Workload Identity Federation) so the documentation reflects the current
config.
- Around line 36-44: The fenced code block in the README is missing a language
tag; update the block around the listed commands so the opening fence is
"```text" (instead of just "```") to satisfy markdown linting and ensure
consistent rendering — modify the README.md fenced block that contains the lines
starting with "cmd/redirectd", "cmd/f3redirect", etc., to use ```text as the
opening fence.
In `@apps/redirect/server/cmd/redirectd/main.go`:
- Around line 66-68: Detect and fail fast when the admin proxy config is
partial: if either ADMIN_HOST (normalized into adminHost) or ADMIN_UPSTREAM
(adminUpstream) is set but the other is empty, log a clear error and exit
instead of silently disabling the proxy; implement this check in main (around
where adminHost/adminUpstream are read). Also stop logging raw adminUpstream
URLs — parse adminUpstream (e.g., with url.Parse) and log only the redacted host
or scheme+host (no userinfo, query, or path) or a masked value to avoid leaking
credentials/secrets wherever adminUpstream is printed.
In `@apps/redirect/server/Dockerfile`:
- Around line 11-12: The Dockerfile uses the mutable tag
"gcr.io/distroless/static-debian12" in the FROM instruction; update that FROM
line in apps/redirect/server/Dockerfile to pin the distroless runtime image by
its immutable digest (use the recommended sha256 value) so builds are
reproducible and resistant to upstream drift, leaving the existing COPY
--from=build /out/redirectd /redirectd line unchanged.
In `@apps/redirect/server/internal/certstore/gcs_test.go`:
- Around line 30-35: The TestMain currently treats emulator setup failure as a
skip and exits with 0, which can produce false-green CI runs; update TestMain so
that when withEmulator(m) returns an error you log the error (using fmt.Println
or t.Log equivalent) and exit with a non-zero status (os.Exit(1)) instead of
os.Exit(0), locating the change in the TestMain function that calls withEmulator
to ensure CI fails when the emulator cannot be set up.
In `@apps/redirect/server/internal/certstore/gcs.go`:
- Around line 223-229: Unlock currently deletes the lock object unconditionally
(in GCS.Unlock), which can release a lock owned by another process; change
Unlock to first fetch the lock object's attributes via
g.lockObj(name).Attrs(ctx), verify the stored owner identifier (e.g., metadata
key used when acquiring the lock, such as g.ownerID or g.lockToken) matches this
instance's owner value, and only then delete using a generation precondition to
avoid races (call g.lockObj(name).If(storage.Conditions{GenerationMatch:
attrs.Generation}).Delete(ctx)); if the owner does not match return a not-owner
error or nil as appropriate.
- Around line 207-210: Capture and check the error returned from
g.client.Bucket(g.bucket).Object(obj.ObjectName()).If(storage.Conditions{GenerationMatch:
attrs.Generation}).Delete(ctx) instead of discarding it; assign it to a variable
(e.g., err := ...Delete(ctx)), then handle non-nil errors appropriately (log and
return the error or propagate it) so real GCS delete failures aren’t masked
during lock-steal attempts—only ignore the specific benign "object not found"
case if applicable, otherwise treat errors from Delete(ctx) as actionable.
- Around line 183-184: The current lock key generation replaces "/" with "_"
(variable safe) which causes collisions (e.g. "a/b" vs "a_b"); change the
encoding to a collision-free form (for example URL-escape the full name or
compute a deterministic hash like sha256 of name and use its hex) and use that
encoded value when building the GCS object path (the expression that calls
g.client.Bucket(g.bucket).Object(g.prefix + "locks/" + safe + ".lock")). Ensure
you replace the current safe computation with the chosen collision-free encoding
so each distinct name maps to a unique lock key.
In `@apps/redirect/server/internal/mappings/dns.go`:
- Around line 39-47: When IsApex(host) is true, guard against an empty
opt.StaticIP before emitting a required A record: check if opt.StaticIP == ""
and instead of returning a required A with an empty Value, return a sentinel
DNSRecord (same Type "A" and Name host) whose Note explicitly states "StaticIP
not set — cannot create apex A record; configure StaticIP" and mark
Optional=true so callers don't treat it as a valid required instruction; update
the logic around IsApex and DNSRecord construction to use this guard (refer to
IsApex, DNSRecord, and opt.StaticIP).
In `@apps/redirect/server/internal/mappings/gcsstore.go`:
- Around line 59-67: The current write in Save unconditionally overwrites the
object (using s.client.Bucket(s.bucket).Object(s.object).NewWriter), which
allows concurrent writes to clobber changes; change Save to use generation-based
preconditions: first call s.client.Bucket(s.bucket).Object(s.object).Attrs(ctx)
to read the current Generation (use 0 if object missing), then create the writer
with
s.client.Bucket(s.bucket).Object(s.object).IfGenerationMatch(generation).NewWriter(ctx)
so the write will fail on concurrent modifications; propagate the precondition
failure error (HTTP 412 / storage precondition error) to callers so they can
reload and retry instead of silently overwriting.
In `@apps/redirect/server/internal/mappings/store.go`:
- Around line 31-39: Unmarshal currently returns any syntactically valid JSON as
Config even if mappings are semantically invalid; update Unmarshal to validate
the parsed Config before returning by invoking the same validation used at
save-time (e.g., call cfg.Validate() or the existing Save-time guard/validation
function) and return an error if validation fails so invalid hosts/targets
cannot be loaded into runtime; ensure you reference the Config type and reuse
the Save validation logic rather than duplicating ad-hoc checks.
In `@apps/redirect/server/internal/redirect/live.go`:
- Around line 52-54: The watcher currently passes interval directly to
time.NewTicker in Live.Watch which panics for zero or negative durations; add a
guard at the start of Live.Watch to check interval <= 0 and handle it (e.g., set
interval to a safe default like 30s or return early and call onErr), then create
the ticker with the validated interval; update any related comments and keep
references to Live.Watch and time.NewTicker so reviewers can locate the change.
In `@apps/redirect/server/internal/redirect/redirect.go`:
- Around line 54-55: The code calls h.Resolver.Resolve(host) without checking
h.Resolver for nil (NewHandler(nil, ...) is allowed) which can panic; before
invoking Resolve (in the request handling function that calls
h.Resolver.Resolve), add a nil check for h.Resolver and return a controlled
error response (or log and return an appropriate HTTP error) when it's nil
instead of dereferencing it; ensure the check preserves existing health-check
behavior and reference the h.Resolver field and the Resolve call so reviewers
can find and update that code path.
In `@apps/redirect/web/scripts/local-e2e.ts`:
- Around line 23-24: The script currently calls db.delete(domain) and
db.delete(user) which will perform full-table deletes; update the cleanup in
local-e2e.ts to avoid destructive blanket deletes by scoping deletions or gating
them: restrict deletes with explicit where clauses (e.g., only delete rows
created by this test via a test-run ID or specific keys), or check and assert a
safe test DATABASE_URL / NODE_ENV before allowing db.delete(domain) /
db.delete(user); ensure the modifications are applied at both occurrences (the
delete calls around lines 23-24 and 120-121) and fail fast if the environment
check does not pass.
- Around line 73-84: The catch block around db.insert(domain).values(...)
currently sets claimed = true for any exception; change it to only treat
uniqueness/duplicate-key errors as a successful "already claimed" case by
inspecting the thrown error (e.g., error.code or error.message) and setting
claimed = true only when it matches the DB unique constraint/duplicate-host
error, otherwise rethrow or fail the test; update the catch around the insert
(the block that sets claimed) so unrelated DB errors do not make assert(claimed,
...) pass.
In `@apps/redirect/web/src/app/api/domains/`[id]/route.ts:
- Around line 42-43: The DB commit should not be coupled to the synchronous
export; find the places calling exportConfigToGCS() in the route handler(s) and
stop awaiting it after a successful write: instead either enqueue the export
(push a message to your job/outbox queue) or kick it off in background (call
exportConfigToGCS() without await and attach .catch(...) to log failures), and
update the HTTP response to return success for the committed change with an
optional field like exportStatus: "pending" or "failed" so request semantics
remain consistent while export is retried asynchronously.
In `@apps/redirect/web/src/app/api/domains/route.test.ts`:
- Around line 40-46: The tests call db.delete/db.insert (see cleanup(),
beforeAll and the db.delete(domain)/db.delete(user) calls) before the schema
exists; fix by ensuring migrations/reset run before any DB ops: invoke your test
DB reset (e.g., resetTestDb or a drizzle-kit push --force / reset-test-db
script) in the global test setup or at the top of this test file before
cleanup()/beforeAll so the domain and user tables are created; update
src/test-setup.ts or add a globalSetup hook to run the migration/reset command,
or explicitly call the reset helper before calling cleanup()/db.* in this test.
In `@apps/redirect/web/src/auth.ts`:
- Around line 23-30: The socialProviders block enables Google auth when only
GOOGLE_CLIENT_ID exists and sets clientSecret to "" if GOOGLE_CLIENT_SECRET is
missing; change this to enable the google provider only when both
process.env.GOOGLE_CLIENT_ID and process.env.GOOGLE_CLIENT_SECRET are present
and non-empty, remove the fallback empty string for clientSecret, and set
socialProviders to undefined otherwise (update the object creation around
socialProviders -> google -> clientId/clientSecret to use a combined truthy
check for both env vars).
In `@apps/redirect/web/src/components/AuthForm.tsx`:
- Around line 16-37: The submit function currently uses try/finally but no
catch, so exceptions thrown by signIn.email or signUp.email bypass the res.error
handling; add a catch block around the await calls in submit to catch thrown
errors, call setError(err.message ?? "authentication failed") and ensure any
thrown error does not swallow setBusy(false) (leave the existing finally), and
keep existing handling of res.error for returned error objects; locate this
logic inside the submit function that calls signIn.email and signUp.email and
update accordingly.
In `@apps/redirect/web/src/components/Dashboard.tsx`:
- Around line 135-154: The save (and similarly add) mutation functions currently
assume fetch() succeeds and res.json() parses without error, so wrap the fetch +
res.json() steps in a try/catch that catches network/parsing errors, calls
setError(...) with a user-friendly message (include error.message when
available), and returns early so the success path (onChange, setEditing) isn't
executed; keep the existing finally block to call setBusy(false). Specifically,
update the async save and add functions to perform the fetch call and await
res.json() inside a try block, handle non-ok responses as before, and in the
catch block call setError(error.message ?? "request failed") and return.
In `@apps/redirect/web/src/db/index.ts`:
- Around line 17-25: The DSN regex in the connectionString parser is too greedy
(the regex used when assigning to `socket` and later destructured into [, user,
pass, database, host]) and captures everything after `host=` so query params
like `&sslmode=disable` get included; update the regex to limit the host capture
to stop at an ampersand or end-of-string (e.g. make the host group match up to
the next `&` or `$`) so `host` contains only the socket path/host, keep the same
destructuring (socket -> [, user, pass, database, host]) and continue to
decodeURIComponent(host) as before.
In `@apps/redirect/web/src/lib/gcs-export.ts`:
- Around line 56-60: Update the doc comment in gcs-export.ts to accurately
describe the return value: replace "Returns the number of objects deleted" with
a note that the function returns the number of objects targeted for deletion
(the value returned as targets.length) and that deletions are best-effort and
errors are swallowed (see where errors are caught and ignored), so the return
value does not guarantee the number of successfully deleted objects.
- Around line 65-68: The current code retrieves all objects under "certs/" then
filters with certObjectsForHost, which is inefficient; update the GCS query in
the storage.bucket(BUCKET).getFiles call to use a narrower prefix derived from
CertMagic's object layout (e.g., "certs/certificates/" or include CA and host
segments where certObjectsForHost expects them) so fewer objects are returned,
and if multiple CAs (staging/production) exist, loop over those CA prefixes and
merge results before applying certObjectsForHost to ensure correctness while
improving performance.
- Around line 20-43: The mutations that change the domain table must not leave
DB state changed if exporting the JSON fails: wrap the call to
exportConfigToGCS() (from exportConfigToGCS in gcs-export.ts) in explicit error
handling in the domain insert (route.ts) and update/delete ([id]/route.ts)
handlers; implement one of two fixes: (A) perform the DB mutation inside a
transaction and only commit after exportConfigToGCS() succeeds, or (B) if
transactions are not used, on export failure run a compensating DB operation to
revert the change (e.g. delete the newly inserted row after insert, restore the
prior row after update, re-insert a deleted row after delete), ensure the DELETE
route’s catch also covers the export step (including export errors that occur
after deleteCertsForHost), and log and rethrow the export error so callers see
failure. Ensure you reference and call exportConfigToGCS exactly and that
deleteCertsForHost remains invoked as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 63837591-76b6-4714-b13d-098365842946
⛔ Files ignored due to path filters (2)
apps/redirect/server/go.sumis excluded by!**/*.sumpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (74)
.github/workflows/ci.yml.github/workflows/deploy-redirect-server.yml.github/workflows/deploy-redirect-web.ymlapps/redirect/README.mdapps/redirect/infra/terraform/.terraform.lock.hclapps/redirect/infra/terraform/main.tfapps/redirect/infra/terraform/outputs.tfapps/redirect/infra/terraform/startup-script.sh.tftplapps/redirect/infra/terraform/terraform.tfvars.exampleapps/redirect/infra/terraform/variables.tfapps/redirect/infra/terraform/versions.tfapps/redirect/server/.dockerignoreapps/redirect/server/Dockerfileapps/redirect/server/cmd/f3redirect/main.goapps/redirect/server/cmd/redirectd/main.goapps/redirect/server/go.modapps/redirect/server/internal/certstore/gcs.goapps/redirect/server/internal/certstore/gcs_test.goapps/redirect/server/internal/mappings/dns.goapps/redirect/server/internal/mappings/dns_parity_test.goapps/redirect/server/internal/mappings/dns_test.goapps/redirect/server/internal/mappings/filestore.goapps/redirect/server/internal/mappings/gcsstore.goapps/redirect/server/internal/mappings/mappings.goapps/redirect/server/internal/mappings/mappings_test.goapps/redirect/server/internal/mappings/store.goapps/redirect/server/internal/mappings/store_test.goapps/redirect/server/internal/redirect/integration_test.goapps/redirect/server/internal/redirect/live.goapps/redirect/server/internal/redirect/proxy.goapps/redirect/server/internal/redirect/redirect.goapps/redirect/server/internal/redirect/redirect_test.goapps/redirect/server/internal/server/server.goapps/redirect/server/package.jsonapps/redirect/server/scripts/coverage.shapps/redirect/shared/dns-instructions.jsonapps/redirect/web/.dockerignoreapps/redirect/web/.gitignoreapps/redirect/web/Dockerfileapps/redirect/web/drizzle.config.tsapps/redirect/web/e2e/admin.spec.tsapps/redirect/web/e2e/passkey.spec.tsapps/redirect/web/eslint.config.jsapps/redirect/web/next.config.tsapps/redirect/web/package.jsonapps/redirect/web/playwright.config.tsapps/redirect/web/public/.gitkeepapps/redirect/web/scripts/local-e2e.tsapps/redirect/web/src/app/api/auth/[...all]/route.tsapps/redirect/web/src/app/api/domains/[id]/route.tsapps/redirect/web/src/app/api/domains/route.test.tsapps/redirect/web/src/app/api/domains/route.tsapps/redirect/web/src/app/dashboard/page.tsxapps/redirect/web/src/app/globals.cssapps/redirect/web/src/app/layout.tsxapps/redirect/web/src/app/page.tsxapps/redirect/web/src/auth.tsapps/redirect/web/src/components/AuthForm.test.tsxapps/redirect/web/src/components/AuthForm.tsxapps/redirect/web/src/components/Dashboard.test.tsxapps/redirect/web/src/components/Dashboard.tsxapps/redirect/web/src/db/index.tsapps/redirect/web/src/db/schema.tsapps/redirect/web/src/lib/auth-client.tsapps/redirect/web/src/lib/domains.parity.test.tsapps/redirect/web/src/lib/domains.test.tsapps/redirect/web/src/lib/domains.tsapps/redirect/web/src/lib/gcs-export.test.tsapps/redirect/web/src/lib/gcs-export.tsapps/redirect/web/src/test-setup.tsapps/redirect/web/src/vitest.d.tsapps/redirect/web/tsconfig.jsonapps/redirect/web/vitest.config.tspnpm-workspace.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/deploy-redirect-server.yml (1)
48-48: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueOptional: Set
persist-credentials: falseon checkout.This prevents Git credentials from persisting in
.git/configfor subsequent steps. While this workflow has no steps that could exploit leaked credentials, it's a low-effort hardening that aligns with the security improvements mentioned as follow-ups.- uses: actions/checkout@v4 + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/deploy-redirect-server.yml at line 48, Update the checkout step configuration to disable persisting credentials by adding persist-credentials: false to the actions/checkout@v4 step; locate the checkout usage (the line with "uses: actions/checkout@v4") and modify its step to include the persist-credentials: false key so Git credentials are not written to .git/config for later steps.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/redirect/web/.env.local.example`:
- Line 24: The PASSKEY_RP_NAME environment variable value contains a space and
should be quoted for deterministic .env parsing; update the PASSKEY_RP_NAME
entry (look for PASSKEY_RP_NAME in the file) to use a quoted string (e.g.,
surround the value with double quotes) so parsers treat "F3 Redirect" as a
single value.
---
Outside diff comments:
In @.github/workflows/deploy-redirect-server.yml:
- Line 48: Update the checkout step configuration to disable persisting
credentials by adding persist-credentials: false to the actions/checkout@v4
step; locate the checkout usage (the line with "uses: actions/checkout@v4") and
modify its step to include the persist-credentials: false key so Git credentials
are not written to .git/config for later steps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ce123e69-b803-473c-9fb6-d56af31a4c6a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (33)
.github/workflows/deploy-redirect-server.yml.github/workflows/deploy-redirect-web.ymlapps/redirect/README.mdapps/redirect/infra/terraform/.gitignoreapps/redirect/infra/terraform/main.tfapps/redirect/infra/terraform/variables.tfapps/redirect/server/.env.local.exampleapps/redirect/server/cmd/redirectd/main.goapps/redirect/server/internal/certstore/gcs_test.goapps/redirect/server/internal/mappings/dns.goapps/redirect/server/internal/redirect/live.goapps/redirect/server/internal/redirect/redirect.goapps/redirect/server/package.jsonapps/redirect/server/scripts/build.shapps/redirect/server/scripts/format-check.shapps/redirect/server/scripts/format.shapps/redirect/server/scripts/lint.shapps/redirect/server/scripts/test.shapps/redirect/shared/package.jsonapps/redirect/web/.env.cloud-run.exampleapps/redirect/web/.env.local.exampleapps/redirect/web/.gitignoreapps/redirect/web/drizzle.config.tsapps/redirect/web/package.jsonapps/redirect/web/scripts/local-e2e.tsapps/redirect/web/scripts/reset-test-db.mjsapps/redirect/web/src/auth.tsapps/redirect/web/src/components/AuthForm.tsxapps/redirect/web/src/components/Dashboard.tsxapps/redirect/web/src/db/index.tsapps/redirect/web/src/lib/gcs-export.tsapps/redirect/web/vitest.config.tspnpm-workspace.yaml
Collaborator
Author
✅ Deployment validated end-to-end against the live
|
| Tier | Build source | Deploy | Result |
|---|---|---|---|
Go redirect (redirectd) |
apps/redirect/server/Dockerfile |
rolled GCE VM redirect-vm |
https://f3muletown.com → 302 → regions.f3nation.com/muletown; www inherits; /healthz 200; fresh Let's Encrypt cert (on-demand TLS issuing) |
Admin web (f3redirect-web) |
apps/redirect/web/Dockerfile (turbo-prune) |
Cloud Run rev 00012 |
run.app 200 (sign-in page renders); /api/auth/ok 200 (better-auth + Cloud SQL connected — validates the DSN-parsing fix) |
| Chain | — | Go proxy → Cloud Run | https://admin.f3regions.com → 200 |
CI is green on c5455de (build, lint, test, test-coverage, typecheck, format-check). The deploy workflows now fire on push to dev (single prod env, path-filtered), so merging this PR will reproduce exactly the deploy that was just validated by hand.
Notes for reviewers
- v1 = personal-funded sandbox (
f3-redirects). Long-term pivot to an F3 Nation org project + F3PROD schema/service-principal (replacing the interim Cloud SQL) is tracked in the PR description. Neon is off the table. - All 34 CodeRabbit threads addressed (19 fixed, 14 deferred-with-reasoning, 1 n/a) and resolved.
🤖 Generated with Claude Code
Collaborator
Author
|
✅
Merge-ready, blocked only on a required human approval. Not merged. |
pstaylor-patrick
force-pushed
the
feat/redirect-app
branch
from
99d144d to
d9d644b
Compare
Collaborator
Author
|
✅
Rebased + CI-green; not merged per request. Blocked only on human review ( |
|
❌ redirect Terraform drift check: |
|
✅ redirect Terraform drift check: in sync — live infrastructure matches the committed config. |
Collaborator
Author
Terraform alignment + drift enforcement for
|
|
✅ redirect Terraform drift check: in sync — live infrastructure matches the committed config. |
1 similar comment
|
✅ redirect Terraform drift check: in sync — live infrastructure matches the committed config. |
Contributor
|
@pstaylor-patrick is this still true "v1 runs in Patrick's personal, self-funded GCP project"? What do we need to do to get this onto F3 letterhead. I reticent to merge code into the Nation monorepo that references private infrastructure. I see in \infra |
Bring the standalone f3-redirect service into the Turborepo monorepo as a
grouped multi-tier app under apps/redirect:
- apps/redirect/server: Go on-demand-TLS redirect service (Cloud Run/COS),
GCS-backed cert + mapping stores, DNS parity tests
- apps/redirect/web: Next.js admin dashboard (Better Auth passkey/email),
domains API, Drizzle schema, Playwright e2e + vitest
- apps/redirect/shared: shared DNS instruction config
- apps/redirect/infra/terraform: Terraform IaC + drift-detection CI
Repo wiring:
- register redirect web/server/shared in pnpm-workspace.yaml
- add Go toolchain + a Postgres-backed `test` job to ci.yml
- deploy-redirect-{server,web} and redirect-terraform-drift workflows
- .npmrc node-linker=hoisted; .dockerignore hardening
- normalize packages/db eslint.config.mjs -> .js to match siblings
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
pstaylor-patrick
force-pushed
the
feat/redirect-app
branch
from
fd1c732 to
a848edc
Compare
|
✅ redirect Terraform drift check: in sync — live infrastructure matches the committed config. |
There was a problem hiding this comment.
Actionable comments posted: 32
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/deploy-redirect-server.yml:
- Around line 18-25: The workflow currently allows manual runs via
workflow_dispatch which can target non-dev refs; add a guard to the deploy job
to block manual deploys from non-dev refs by adding a job-level condition (e.g.
if: github.ref == 'refs/heads/dev') to the main deploy job so only runs from the
dev branch proceed, or alternatively remove workflow_dispatch entirely; update
the job that performs the deploy (the job that consumes the on: push for
branches and runs the deployment steps) to include this conditional check
referencing github.ref or drop the workflow_dispatch trigger.
In @.github/workflows/redirect-terraform-drift.yml:
- Around line 89-105: The PR-comment step ("Comment drift result on PR") can 403
on forked PRs; add a guard at the top of the actions/github-script block to only
call github.rest.issues.createComment when the PR head repo equals the base repo
(e.g. compare context.payload.pull_request.head.repo.full_name to
context.payload.pull_request.base.repo.full_name or compare their owners), and
otherwise skip/comment a no-op; preserve use of steps.plan.outputs.exitcode, the
computed body, and the call to github.rest.issues.createComment so the comment
only runs for non-forked PRs.
In `@apps/redirect/infra/terraform/startup-script.sh.tftpl`:
- Line 1: Replace the non-portable shebang at the top of the startup-script
template: change the current /bin/bash shebang line to use the env-based shebang
so the script uses the repository standard (use /usr/bin/env bash) — update the
first line (the shebang) in the startup-script.sh.tftpl template accordingly.
In `@apps/redirect/infra/terraform/variables.tf`:
- Around line 53-56: The variable "image_tag" currently defaults to "latest",
which makes deployments non-reproducible; remove the default value from the
variable "image_tag" in variables.tf so Terraform requires an explicit immutable
tag or digest from CI, or replace the default with a fixed immutable tag/digest
if you really need a default; also update the variable description to mention
that an explicit immutable tag/digest must be provided and ensure the CI/CD
pipeline supplies the image_tag input when applying.
- Around line 1-4: The variables currently default to real environment
identifiers (variable "project" and variable "admin_domain"), which can cause
accidental applies against live resources; remove their defaults (or set them to
empty/null) so Terraform requires explicit values via tfvars/CLI, and add a
validation block on "project" and "admin_domain" to ensure non-empty values (and
optionally reject known production strings) so accidental unparameterized
applies are prevented.
In `@apps/redirect/infra/terraform/versions.tf`:
- Around line 14-16: The Terraform GCS backend is hardcoded to
"f3-redirects-tfstate" in the backend "gcs" block (bucket and prefix), which
pins state to a personal bucket; remove the bucket and related backend settings
from the committed backend "gcs" block and instead document/require supplying
them via terraform init -backend-config (e.g., backend bucket, prefix,
credentials) per environment so state is injected at init time rather than baked
into this file; update any CI/workflow docs or scripts to call terraform init
-backend-config for each environment and ensure backend "gcs" remains present
but empty in the repo.
In `@apps/redirect/README.md`:
- Around line 33-34: Update the README’s Admin CLI description to remove the
“TypeScript management UI is deferred” claim and note that a self-serve admin
app has been added at apps/redirect/web; specifically edit the paragraph
describing "Admin CLI (Go, `cmd/f3redirect`)" to either remove the deferred
sentence or replace it with a short note that a TypeScript/React management UI
now exists in apps/redirect/web and can be used for add/list/remove mappings and
DNS record guidance.
In `@apps/redirect/server/cmd/f3redirect/main.go`:
- Around line 245-248: The current logic silently ignores extra positional args
by only reading fs.Arg(0) and leaving only empty for NArg()>1; change it to
explicitly reject more than one positional argument: check fs.NArg(), if it
equals 1 set only = mappings.NormalizeHost(fs.Arg(0)), but if fs.NArg() > 1
print a clear usage/error (e.g. call fs.Usage() or log.Fatalf) and exit so the
CLI returns an error instead of proceeding with only == ""; update the block
around the only variable and the fs.NArg()/fs.Arg usage accordingly.
In `@apps/redirect/server/internal/certstore/gcs.go`:
- Around line 205-213: The current loop silently ignores any non-nil error
returned by obj.Attrs(ctx); update the logic in the lock-checking loop (the call
to obj.Attrs(ctx) and subsequent handling of attrs.Created and Generation) to
only treat a NotFound/ErrObjectNotExist as a benign case (continue), but
propagate or return any other unexpected error instead of swallowing it; ensure
the error is surfaced to the caller (instead of sleeping/retrying) so callers of
the lock path can observe real GCS failures. Use the existing symbols
obj.Attrs(ctx), lockTTL, attrs.Generation and the delete call via
g.client.Bucket(...).Object(...).If(...).Delete(ctx) to locate the code to
change.
In `@apps/redirect/server/internal/mappings/dns_test.go`:
- Around line 11-12: Add a guard that checks len(recs) before indexing recs[0]
in the failing test cases: verify len(recs) > 0 and call t.Fatalf with a clear
message (e.g., "expected at least 1 record, got %d", len(recs)) if not, then
proceed to assert recs[0].Type/Name/Value/Optional; apply the same pattern to
the other test that indexes recs[0] (the block around the second assertion) so
tests fail fast instead of panicking.
In `@apps/redirect/server/internal/mappings/dns.go`:
- Around line 76-81: The CNAME fallback note uses opt.StaticIP directly and may
render empty; update the DNSRecord Note (where ApexOf(host) produces apex and
the record is constructed) to include a fallback target when opt.StaticIP is
unset—e.g., use opt.StaticIP if non-empty otherwise use apex (or similar
actionable placeholder) so the message always reads "...must carry an A record
to <target>." Modify the Note formatting in the DNSRecord construction to
reference this computed target instead of opt.StaticIP raw.
In `@apps/redirect/server/internal/mappings/mappings.go`:
- Around line 47-53: The current host check in mappings.go (NormalizeHost and
the mapping loop) is too weak; replace the simple strings.Contains check with a
proper hostname validator: implement a function (e.g., isValidHostname or
ValidateHostname) that enforces RFC-like rules (no leading/trailing dots, no
empty labels, each label 1–63 chars, labels match `^[A-Za-z0-9-]+$` and do not
start or end with '-', total length ≤253, and require at least one dot), call it
where you now check strings.Contains(host, ".") and return a descriptive
fmt.Errorf including i and m.Host when validation fails; reuse NormalizeHost
output and keep the existing error messages style.
In `@apps/redirect/server/internal/redirect/integration_test.go`:
- Line 37: The test currently ignores the error returned by NewAdminProxy which
can mask setup failures; update the test to capture the error (e.g., proxy, err
:= redirect.NewAdminProxy(upstream.URL, "admin.example.com")) and fail fast if
err != nil (e.g., t.Fatalf or require.NoError) so the test stops immediately
when NewAdminProxy cannot construct the admin proxy for upstream.URL/host.
In `@apps/redirect/server/internal/redirect/live.go`:
- Around line 31-32: The Config() accessor currently returns *l.cfg.Load() which
exposes the internal slice backing array; change Live.Config to return a deep
copy of mappings.Config: call cfg := l.cfg.Load(), create a new mappings.Config
value, allocate and copy the Mappings slice (and any nested slice/map fields)
into the new struct, and return that copy so callers cannot mutate the live
snapshot; locate the method named Config on type Live and the l.cfg.Load() usage
to implement the defensive copy.
In `@apps/redirect/server/internal/redirect/proxy.go`:
- Around line 13-17: The code currently only checks url.Parse errors but allows
relative or scheme-less upstreams; after calling url.Parse(upstream) validate
that the parsed URL is absolute (e.g. u.Scheme != "" and u.Host != "" or use
u.IsAbs()) and return a descriptive error if not; do this check before calling
httputil.NewSingleHostReverseProxy(u) so that NewSingleHostReverseProxy only
receives a fully-qualified upstream (reference symbols: url.Parse, the upstream
variable, the parsed u, and httputil.NewSingleHostReverseProxy).
In `@apps/redirect/server/internal/redirect/redirect.go`:
- Around line 49-50: Normalize h.AdminHost the same way r.Host is normalized
before comparing so configured values with uppercase letters or a trailing dot
still match the admin proxy branch; e.g., compute a normalizedAdminHost by
lowercasing and trimming any trailing dot from h.AdminHost and then use if
normalizedAdminHost != "" && h.AdminProxy != nil && host == normalizedAdminHost
{ h.AdminProxy.ServeHTTP(w, r) } so the comparison uses the same normalized form
as the incoming host.
In `@apps/redirect/web/drizzle.config.ts`:
- Around line 8-10: The config currently reads process.env.DATABASE_URL directly
for the url property; replace that with the repo's with-env loader so the
Drizzle CLI uses the same env resolution as the app. Locate the url property in
drizzle.config.ts (the url key that currently uses process.env.DATABASE_URL) and
call the with-env helper (the project's "with-env" loader) to resolve
DATABASE_URL, preserving the existing fallback
("postgresql://f3local:f3local@localhost:5433/f3nation") if the helper returns
undefined.
In `@apps/redirect/web/scripts/local-e2e.ts`:
- Around line 22-31: The assertLocalTestDb function (and the other direct reads
around lines where EXPORT_LOCAL_PATH is used) currently reads DATABASE_URL and
EXPORT_LOCAL_PATH from process.env; replace those direct accesses with the
repo’s shared env loader by importing and using the with-env helper to load
configuration (e.g., call withEnv or withEnvSync to obtain DATABASE_URL and
EXPORT_LOCAL_PATH) and then use the returned values in assertLocalTestDb and the
other locations instead of process.env; ensure the import references the correct
helper name as exported by the repo and preserve the existing validation logic
that checks isLocal and looksTest.
In `@apps/redirect/web/scripts/reset-test-db.mjs`:
- Around line 26-45: The script currently only creates DEDICATED_DB if missing,
leaving existing rows intact; update the admin logic (the admin postgres client
and the DEDICATED_DB handling around admin.unsafe / admin.end) to forcibly reset
the DB by dropping and recreating it before pushing the schema: use admin.unsafe
to run a DROP DATABASE IF EXISTS ${DEDICATED_DB} followed by CREATE DATABASE
${DEDICATED_DB} (while still connected as admin), then call await admin.end();
keep the later dedicated variable and execSync("pnpm exec drizzle-kit push
--force", { ..., env: { ... , DATABASE_URL: dedicated } }) so the freshly
created database is populated from scratch.
In `@apps/redirect/web/src/app/api/domains/route.ts`:
- Around line 71-92: The code currently inserts the domain row before calling
exportConfigToGCS(), so if the export fails the DB still claims the domain; to
fix this, ensure the DB commit only persists when exportConfigToGCS() succeeds:
either perform the insert and export inside a DB transaction (use db.transaction
and replace db.insert(domain)... with tx.insert(domain)..., call
exportConfigToGCS() inside the transaction and only commit after it returns) or,
if calling external GCS inside a DB transaction is undesirable, immediately
delete/rollback the inserted row on export failure (capture the inserted record
from the insert using the existing inserted variable and call
db.delete(domain).where(domain.id.eq(inserted.id)) or delete by hostname in the
catch block before returning the 500), keeping the existing unique-index
handling for the initial insert.
In `@apps/redirect/web/src/app/globals.css`:
- Around line 153-155: Remove the deprecated CSS declaration by deleting the
`word-break: break-word;` line inside the `.domain-head .dest` rule and keep
only `overflow-wrap: anywhere;` so Stylelint stops flagging the rule; update the
`.domain-head .dest` block (the selector shown in the diff) accordingly.
In `@apps/redirect/web/src/auth.ts`:
- Around line 24-48: Import and use the repository's with-env helper instead of
direct process.env access: replace every process.env.* usage in the auth config
(references: socialProviders block, secret, baseURL, trustedOrigins, and
passkey({ rpID, rpName, origin }) options) with calls to the with-env helper
(e.g., withEnv('GOOGLE_CLIENT_ID') etc.), preserving the existing default
fallbacks and conditional logic; ensure you add the import for the with-env
helper at the top of the module and update the passkey origin/rpID/rpName
expressions to use the helper so env loading and normalization follow the
monorepo convention.
In `@apps/redirect/web/src/components/AuthForm.test.tsx`:
- Around line 29-41: The navigation assertion in AuthForm.test.tsx is racing the
async submit flow: change the two places (email+password test and passkey test)
to wait for the router.push call after the submit completes instead of asserting
immediately; e.g., after triggering userEvent.click in the "signs in with
email+password and routes to the dashboard" test (and the passkey test at lines
72-81), use an async wait helper (like waitFor or findBy... / await expect(() =>
h.push).toHaveBeenCalledWith("/dashboard") wrapped in waitFor) to assert
h.push("/dashboard") only after h.signInEmail (or the passkey submit) resolves,
ensuring the test waits for the async continuation in the component submit
handler.
In `@apps/redirect/web/src/components/AuthForm.tsx`:
- Line 86: The error paragraph in AuthForm.tsx ({error && <p
className="error">{error}</p>}) isn't announced by screen readers; update the
rendered error element to be accessible by adding an appropriate live region
(e.g., role="alert" or aria-live="assertive" plus aria-atomic="true") to the <p>
that renders the error so assistive tech will announce auth failures
immediately.
In `@apps/redirect/web/src/components/Dashboard.test.tsx`:
- Around line 40-44: The test suite currently assigns global.fetch directly in
beforeEach (global.fetch = vi.fn()), which leaks a mocked fetch into other
tests; replace that assignment by stubbing the global with
vi.stubGlobal("fetch", vi.fn()) inside beforeEach and add an afterEach that
calls vi.unstubAllGlobals() (or vi.unstubGlobal("fetch")) to restore the
original fetch; locate the beforeEach block (and related h.push / h.addPasskey
mocks) in Dashboard.test.tsx and update it to use vi.stubGlobal and an afterEach
cleanup to prevent cross-test leakage.
In `@apps/redirect/web/src/components/Dashboard.tsx`:
- Around line 160-220: The remove() handler currently ignores failed DELETE
responses and the existing error UI only appears when editing; update remove()
to treat non-ok responses and network exceptions as errors by catching
exceptions and/or checking !res.ok, reading the response message/body (text or
JSON) and calling setError(...) with a user-friendly message (while still
ensuring setBusy(false) in the finally block), and on success call
onRemove(domain.id); also surface the error state outside the editing branch by
moving or duplicating the {error && ...} display so errors are visible when
editing === false. Use the existing symbols remove, setBusy, setError, onRemove,
domain.id, error, and editing to locate and modify the code.
In `@apps/redirect/web/src/db/index.ts`:
- Around line 5-8: Replace the direct use of process.env for DATABASE_URL in the
DB entrypoint: import and use the repo's with-env helper to load the
DATABASE_URL (instead of accessing process.env.DATABASE_URL) and assign it to
the existing connectionString variable; update the top of the file to import the
helper (e.g., withEnv/getEnv) and call it to retrieve DATABASE_URL, then keep
the existing null-check/throw logic around connectionString so behavior remains
the same.
In `@apps/redirect/web/src/db/schema.ts`:
- Around line 97-107: Change the non-unique index on the passkey credential ID
to a UNIQUE constraint so duplicate WebAuthn credential IDs cannot be stored:
update the table definition that declares credentialID (column name
credentialID) by replacing the index call that creates passkey_credentialID_idx
(index("passkey_credentialID_idx").on(table.credentialID)) with a unique
index/constraint (e.g., uniqueIndex or unique(...) on table.credentialID) so the
schema enforces uniqueness; if you have existing data, run a migration that
deduplicates or fails safely before applying the unique constraint.
In `@apps/redirect/web/src/lib/domains.ts`:
- Around line 4-12: STATIC_IP and CANONICAL_HOST currently read process.env at
module import; replace that with the monorepo env helper (the "with-env" helper)
so the module does not depend on ambient process state. Use the repo helper to
load REDIRECT_STATIC_IP and REDIRECT_CANONICAL_HOST (with the same default
fallbacks) and either export values initialized via the helper at app bootstrap
or export accessor functions that call the helper lazily; update the exports
STATIC_IP and CANONICAL_HOST to obtain their values through the with-env helper
instead of process.env.
- Around line 49-58: The hostname validation currently lets bare public suffixes
(e.g. "co.uk") pass because after transform(normalizeHost) it only tests
HOSTNAME_RE; update registerSchema to add a second refine that calls
getDomain(normalizedHost) and rejects when it returns null (i.e. require
getDomain(h) !== null) so only registrable domains are accepted; reference the
existing normalizeHost transform, HOSTNAME_RE check, and use getDomain(...) in
the new refine to perform the registrable-domain validation.
In `@apps/redirect/web/src/lib/gcs-export.ts`:
- Around line 6-10: This file directly reads process.env for BUCKET, OBJECT, and
LOCAL_PATH (constants BUCKET, OBJECT, LOCAL_PATH) which bypasses the repo's env
validation; replace those direct reads by importing and using the repo's
with-env helper to load and validate each variable (preserve the existing
default values when the helper supports defaults, or apply the same fallback
logic after loading), update the constants in gcs-export.ts to be populated from
the with-env results, and remove direct process.env usage so all envs flow
through the shared with-env helper.
In `@apps/redirect/web/vitest.config.ts`:
- Around line 9-11: Replace direct process.env access by loading env via the
shared with-env helper: call the helper (e.g., withEnv() or the repo's exported
function from "with-env") to get the env object, then set const base =
env.TEST_DATABASE_URL ?? "postgresql://f3local:f3local@localhost:5433/f3nation";
and derive the redirect DB URL (e.g., f3redirect_test) from base by replacing
the database name portion with "f3redirect_test" so the rest of the connection
string (user/host/port) is preserved; update usages to reference this derived
f3redirect_test variable instead of process.env.TEST_DATABASE_URL.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6cc0f2a8-caeb-4afe-88a2-5a6bc358d5fb
⛔ Files ignored due to path filters (2)
apps/redirect/server/go.sumis excluded by!**/*.sumpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (89)
.dockerignore.github/workflows/ci.yml.github/workflows/deploy-redirect-server.yml.github/workflows/deploy-redirect-web.yml.github/workflows/redirect-terraform-drift.ymlapps/redirect/README.mdapps/redirect/infra/terraform/.gitignoreapps/redirect/infra/terraform/.terraform.lock.hclapps/redirect/infra/terraform/ci.tfapps/redirect/infra/terraform/domain-mapping.tfapps/redirect/infra/terraform/main.tfapps/redirect/infra/terraform/outputs.tfapps/redirect/infra/terraform/startup-script.sh.tftplapps/redirect/infra/terraform/terraform.tfvars.exampleapps/redirect/infra/terraform/variables.tfapps/redirect/infra/terraform/versions.tfapps/redirect/server/.dockerignoreapps/redirect/server/.env.local.exampleapps/redirect/server/Dockerfileapps/redirect/server/cmd/f3redirect/main.goapps/redirect/server/cmd/redirectd/main.goapps/redirect/server/go.modapps/redirect/server/internal/certstore/gcs.goapps/redirect/server/internal/certstore/gcs_test.goapps/redirect/server/internal/mappings/dns.goapps/redirect/server/internal/mappings/dns_parity_test.goapps/redirect/server/internal/mappings/dns_test.goapps/redirect/server/internal/mappings/filestore.goapps/redirect/server/internal/mappings/gcsstore.goapps/redirect/server/internal/mappings/mappings.goapps/redirect/server/internal/mappings/mappings_test.goapps/redirect/server/internal/mappings/store.goapps/redirect/server/internal/mappings/store_test.goapps/redirect/server/internal/redirect/integration_test.goapps/redirect/server/internal/redirect/live.goapps/redirect/server/internal/redirect/proxy.goapps/redirect/server/internal/redirect/redirect.goapps/redirect/server/internal/redirect/redirect_test.goapps/redirect/server/internal/server/server.goapps/redirect/server/package.jsonapps/redirect/server/scripts/build.shapps/redirect/server/scripts/coverage.shapps/redirect/server/scripts/format-check.shapps/redirect/server/scripts/format.shapps/redirect/server/scripts/lint.shapps/redirect/server/scripts/test.shapps/redirect/shared/dns-instructions.jsonapps/redirect/shared/package.jsonapps/redirect/web/.dockerignoreapps/redirect/web/.env.cloud-run.exampleapps/redirect/web/.env.local.exampleapps/redirect/web/.gitignoreapps/redirect/web/Dockerfileapps/redirect/web/drizzle.config.tsapps/redirect/web/e2e/admin.spec.tsapps/redirect/web/e2e/passkey.spec.tsapps/redirect/web/eslint.config.jsapps/redirect/web/next.config.tsapps/redirect/web/package.jsonapps/redirect/web/playwright.config.tsapps/redirect/web/public/.gitkeepapps/redirect/web/scripts/local-e2e.tsapps/redirect/web/scripts/reset-test-db.mjsapps/redirect/web/src/app/api/auth/[...all]/route.tsapps/redirect/web/src/app/api/domains/[id]/route.tsapps/redirect/web/src/app/api/domains/route.test.tsapps/redirect/web/src/app/api/domains/route.tsapps/redirect/web/src/app/dashboard/page.tsxapps/redirect/web/src/app/globals.cssapps/redirect/web/src/app/layout.tsxapps/redirect/web/src/app/page.tsxapps/redirect/web/src/auth.tsapps/redirect/web/src/components/AuthForm.test.tsxapps/redirect/web/src/components/AuthForm.tsxapps/redirect/web/src/components/Dashboard.test.tsxapps/redirect/web/src/components/Dashboard.tsxapps/redirect/web/src/db/index.tsapps/redirect/web/src/db/schema.tsapps/redirect/web/src/lib/auth-client.tsapps/redirect/web/src/lib/domains.parity.test.tsapps/redirect/web/src/lib/domains.test.tsapps/redirect/web/src/lib/domains.tsapps/redirect/web/src/lib/gcs-export.test.tsapps/redirect/web/src/lib/gcs-export.tsapps/redirect/web/src/test-setup.tsapps/redirect/web/src/vitest.d.tsapps/redirect/web/tsconfig.jsonapps/redirect/web/vitest.config.tspnpm-workspace.yaml
- turbo.json: add the redirect-web env vars (BETTER_AUTH_*, GOOGLE_CLIENT_*, PASSKEY_*, REDIRECT_*, CONFIG_*, EXPORT_LOCAL_PATH) to globalEnv so the turbo/no-undeclared-env-vars lint rule passes during `next build`. - db/index.ts: instantiate the postgres client lazily via a memoized Proxy instead of at module load. `next build` evaluates the route module graph during page-data collection where DATABASE_URL is intentionally absent; connecting eagerly threw "DATABASE_URL is not set" and failed CI build. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅ redirect Terraform drift check: in sync — live infrastructure matches the committed config. |
Server (Go):
- api/domains: roll back the DB claim and return 503 when GCS publish fails,
so a failed export can't leave a stale-but-claimed domain (Critical)
- mappings: strengthen hostname validation (reject a..com, .x.com, foo/bar.com)
- dns: render an actionable target when StaticIP is unset in the subdomain
fallback note instead of "A record to "
- redirect: normalize AdminHost before comparing to the request host
- proxy: reject non-absolute upstream URLs at construction
- live: return a defensive copy from Config() so callers can't mutate the
live snapshot / race readers
- certstore/gcs: propagate unexpected lock-inspection errors (treat only
ErrObjectNotExist as benign)
- cmd/f3redirect: reject extra positional args in `dns`
- tests: fail fast on NewAdminProxy error; guard len(recs) before indexing
Web (Next.js):
- domains: reject bare public suffixes (require a registrable eTLD+1)
- db/schema: make passkey.credentialID a UNIQUE index
- AuthForm: announce errors to assistive tech (role=alert/aria-live)
- Dashboard: surface delete failures instead of failing silently
- globals.css: drop deprecated word-break: break-word
- tests: waitFor async navigation; stubGlobal/unstubAllGlobals fetch
- scripts/reset-test-db: drop+recreate the dedicated DB (true reset)
Infra/CI:
- deploy-redirect-{server,web}: guard deploy on github.ref == refs/heads/dev
so workflow_dispatch can't ship unmerged code to prod
- terraform-drift: skip the PR comment on fork PRs (read-only token → 403)
- terraform: drop the live default for `project` (+validation); pass it via
TF_VAR in the drift workflow; clear the admin_domain live default
- startup-script: use /usr/bin/env bash per repo convention
- README: document the new self-serve admin web app
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅ redirect Terraform drift check: in sync — live infrastructure matches the committed config. |
PUT /api/domains/:id now returns 503 if exportConfigToGCS() throws after the destination update, mirroring the POST rollback fix. Without this the DB shows the new destination while the redirect tier keeps serving the old one with no signal to the caller; a retry of the same PUT re-exports cleanly. DELETE is intentionally left as-is: a failed export there is self-healing on the next successful write, and a post-delete 503 would only yield a confusing 404 on retry (the row is already gone). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅ redirect Terraform drift check: in sync — live infrastructure matches the committed config. |
Collaborator
Author
✅ Validation — head
|
| Gate | Result |
|---|---|
build (turbo: Next apps + Go go build ./...) |
✅ pass |
lint (eslint flat config + Go) |
✅ pass |
typecheck (tsc --noEmit + Go) |
✅ pass |
test (Go pkg tests + web vitest + Postgres-backed integration) |
✅ pass |
test-coverage |
✅ pass |
format-check |
✅ pass |
Terraform drift (plan vs live f3-redirects) |
✅ pass |
| CodeRabbit | ✅ pass |
Locally re-verified during the review pass: f3-redirect-web build/typecheck/lint clean; apps/redirect/server gofmt/go vet/go build/go test ./... clean; Go↔TS DNS-rule parity test passes after the dual-side hostname-validation change.
The "Validation" section in the description uses ✅ status markers (no - [ ] checkboxes), so there are no checkboxes to tick — all listed items are confirmed by the green CI matrix above.
Collaborator
Author
🤖 Merge-readiness attestationBrought this PR to merge-ready state (not merged):
Left for a human: final approval (branch protection requires it) and the merge itself. 🤖 Generated with Claude Code |
Contributor
Baguette. Per this comment, and the comment in Slack, and me refining the boards for standup this week. I'm going to set this one back to draft for now. Feel free to undraft it, or hit me up on Slack. |
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
2 participants
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.
What
Migrates the standalone
f3-redirectrepo into this monorepo as a single application with three tiers underapps/redirect/.Why this shape — gold standard for multilingual packages
This is the first Go code and the first paired frontend+backend in an otherwise-TypeScript monorepo. The pattern is intended as the template Python packages will follow:
package.jsonwhose scripts shell togo(build/test/lint/typecheck/format). Turbo orchestrates Go and TS tiers uniformly — no bespoke CI lane.ci.yml) gainsactions/setup-goin every job that runs a turbo task the Go package participates in, plus a Postgres-backedtestjob, sopnpm lint/typecheck/build/testtransparently cover both languages.shared/dir holds the single source-of-truth DNS-rule fixture that both the Go parity test and the TS parity test assert against.Monorepo alignment
apps/redirect/{web,server,shared}added explicitly to the pnpm workspace globs (soinfra/terraformisn't treated as a package).eslint.config.js(@acme/eslint-config) +@acme/tsconfig.github.com/F3-Nation/f3-nation/apps/redirect/server.turbo.jsonglobalEnv(Better Auth / Google OAuth / passkey / redirect / config / export) soturbo/no-undeclared-env-varspasses duringnext build.next buildpage-data collection doesn't connect (and fail) whenDATABASE_URLis absent.packages/db/eslint.config.mjs→.jsto match every sibling package.Deploy model
deploy-redirect-server.yml): on push todev(path-filtered to the Go tier) — build image, push to Artifact Registry, roll the GCE VM. Guarded withif: github.ref == 'refs/heads/dev'soworkflow_dispatchcan't ship unmerged code to prod.deploy-redirect-web.yml): on push todev(path-filtered to the web tier) — build + deploy to Cloud Run, same dev-ref guard.redirect-terraform-drift.yml):terraform planagainst the live infra; comments on same-repo PRs (skipped on fork PRs where the token is read-only).f3-redirects(WIF provider / deployer SA / Terraform state bucket are personal-sandbox identifiers). Terraformprojectno longer has a live default — it must be supplied per environment (the drift CI passes it viaTF_VAR_project).-backend-config, and the interim Cloud SQL database is replaced by an app-specific schema + service principal in the F3PROD data warehouse (the Codex / PaxVault pattern). Neon Postgres is off the table.:latestimage tag (the VM pulls it on boot/restart); immutable tags/digests are deferred to the org-project migration.Review hardening (this PR)
Resolved 32 CodeRabbit threads; applied the verified fixes, including:
POST /api/domainsnow rolls back the DB claim + returns 503 if the GCS publish fails (no stale-but-claimed domain that 409s on retry).PUTsurfaces the same publish failure as a 503.a..com,.x.com,foo/bar.com);Live.Config()returns a defensive copy;NewAdminProxyrejects non-absolute URLs;AdminHostnormalized before comparison; GCS lock inspection propagates unexpected errors;dnsCLI rejects extra args.credentialIDis a UNIQUE index; registration rejects bare public suffixes; auth errors announced to assistive tech (role="alert"); delete failures surfaced in the UI; test stability (waitFor,vi.stubGlobal/unstubAllGlobals);reset-test-dbis a true drop-and-recreate.The repo-wide
with-envenv-loader suggestions were declined: the redirect web app is intentionally self-contained (its own.envscheme) until it moves to the org-owned project.Validation
All run in-monorepo via turbo / CI:
typecheck,lint,format-check,build— green (TS + Go)test— Go:certstore/mappings/redirectpass; Web: vitest unit + Postgres-backed integration passtest-coverageand Terraformdrift— green🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores
Tests