fix(gateway): testing ergonomics for the deploy stack#644
Conversation
fro-bot
left a comment
There was a problem hiding this comment.
Verdict: CONDITIONAL
The core logic is correct and well-tested. One blocking issue: the secret file bind-mount was already present before this PR, but it has no corresponding instruction in the README setup for the optional case — and the new setup docs don't note that it must exist even when unused. This is a pre-existing gap that this PR partially surfaces (by adding a parallel optional secret with explicit guidance), so worth fixing here.
Blocking issues
compose.yaml: s3_endpoint bind-mount has no fallback guidance for operators who don't use a custom endpoint
The new discord-guild-id secret is documented with explicit touch instructions and a comment explaining that an empty file is treated as unset. The s3_endpoint bind-mount at ./secrets/s3-endpoint:/run/secrets/s3_endpoint:ro has existed since before this PR, but the README setup section — which this PR now substantially owns — doesn't mention that touch deploy/secrets/s3-endpoint is required for operators using standard AWS S3 (where no custom endpoint is needed). A missing bind-mount source causes Docker to silently create a directory at the mount target, breaking secret resolution at runtime.
The PR's new README language sets the right pattern ("For optional secrets that you don't want to set… create the file empty"). It should apply that pattern consistently to s3-endpoint in the mkdir/echo/touch/chmod block.
The fix is a one-liner addition to the secret creation block, e.g.:
# Optional — omit for standard AWS S3; set for R2 or other compatible stores:
touch deploy/secrets/s3-endpoint
# echo -n 'https://your-endpoint.r2.dev' > deploy/secrets/s3-endpointNon-blocking concerns
-
readOptionalSecretenv-var path doesn't normalize empty strings. Ifprocess.env.DISCORD_GUILD_IDis set to''(empty string — possible in some container runtimes if the env var is declared but not assigned), the current code returns''rather thannull. The file-backed path now returnsnullfor empty files. This asymmetry won't affect the compose deploy (the env var isn't set there; the file path is), but it's a latent inconsistency. Not blocking for this PR since the stated goal is the file-path fix. -
The
discord_guild_idsecret environment-variable name uses underscores while the file name uses hyphens. This is consistent with all other secrets in the file (Docker normalizes secret names), so it's fine — just noting it isn't an oversight. -
deploy/.env.examplecomment says "Required only if your deployment actually writes to S3" but the current mitmproxy allowlist enforces S3 blocking at the proxy level, not at the secret level. The comment is accurate for v1 but could mislead operators who set a real bucket and forgetOBJECT_STORE_HOSTS. The README cross-reference to the allowlist section mitigates this.
Missing tests
The env-var empty-string case (non-blocking concern #1 above) has no test. No other gaps — the new test suite covers: file path, env var path, empty file → null, whitespace-only file → null, leading whitespace preserved, file-takes-precedence, missing-file-fallthrough, and all loadGatewayConfig branches including the new DISCORD_GUILD_ID path.
Risk assessment: LOW
The blast radius is limited to deploy ergonomics and one config helper function. The readOptionalSecret fix is strictly safer than the previous behavior (empty string → null rather than empty string flowing downstream). The compose change adds a missing optional secret mount that was already described in config.ts; the container will restart-loop without it only if the secrets file is absent, and the README now documents that touch is required. No runtime behavior changes to the slash command or mention handler.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | fro-bot/agent |
| Run ID | 26003693434 |
| Cache | hit |
| Session | ses_1c813d1a1ffebYiOt16N5nMah7 |
fro-bot
left a comment
There was a problem hiding this comment.
Verdict: CONDITIONAL
The core logic is correct and well-tested. One blocking issue: the s3_endpoint secret file bind-mount was already present before this PR, but the new README setup section — which this PR substantially owns — doesn't mention that touch deploy/secrets/s3-endpoint is required for operators using standard AWS S3 (no custom endpoint). The PR introduces the right pattern for discord-guild-id; it should apply that pattern consistently to s3-endpoint.
Blocking issues
README setup block is missing a touch for s3-endpoint
The s3_endpoint bind-mount at ./secrets/s3-endpoint:/run/secrets/s3_endpoint:ro has existed since before this PR. The new README Important note correctly explains that "all files under deploy/secrets/ must exist before running docker compose config" and that optional secrets should be created empty. But the secret-creation code block only shows a commented-out echo for s3-endpoint without a touch fallback — if the operator follows the block literally and skips the commented line, they'll hit a Docker bind-mount error on first compose up.
The discord-guild-id secret is handled correctly with an explicit touch before the commented-out echo. Apply the same pattern to s3-endpoint:
# Optional — omit for standard AWS S3; set for R2 or other S3-compatible stores:
touch deploy/secrets/s3-endpoint
# echo -n 'https://your-endpoint.r2.dev' > deploy/secrets/s3-endpointNon-blocking concerns
-
readOptionalSecretenv-var path doesn't normalize empty strings. Ifprocess.env.DISCORD_GUILD_IDis set to''(possible in some runtimes when a variable is declared but not assigned), the function returns''rather thannull. The file-backed path now correctly returnsnullfor empty files. This asymmetry won't affect the compose deploy (the file path is used exclusively there), but it's a latent inconsistency worth a follow-up. -
deploy/.env.examplecomment saysOBJECT_STORE_HOSTSis "Required only if your deployment actually writes to S3" — accurate for v1, but an operator who sets a real bucket and forgets this variable will see opaque proxy 403s rather than a clear error. The README cross-reference to the allowlist section mitigates this adequately for now.
Missing tests
The env-var empty-string edge case (concern #1) has no test. No other gaps — the new suite thoroughly covers the file-backed path: empty file → null, whitespace-only → null, leading whitespace preserved, file-over-env precedence, missing-file fallthrough, and all loadGatewayConfig branches including the new DISCORD_GUILD_ID path.
Risk assessment (LOW)
Blast radius is limited to deploy ergonomics and one config helper. The readOptionalSecret fix is strictly safer than before. The compose change adds a missing optional-secret mount that config.ts already expected; the only regression risk is operators missing the touch step, which the README Important callout addresses — except for s3-endpoint (the blocking issue above).
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | fro-bot/agent |
| Run ID | 26003693434 |
| Cache | hit |
| Session | ses_1c813d1a1ffebYiOt16N5nMah7 |
secret read, document testing-only config Three real friction points for someone bringing up the deploy stack for the first time on their own Discord server: 1. compose.yaml didn't wire DISCORD_GUILD_ID through to the gateway container. config.ts already read it via readOptionalSecret, but the secret file mount and the *_FILE env-var pointer were missing. Without this, slash commands register globally and propagate over up to an hour — instead of guild-scoped registration which propagates in ~5 seconds. Critical for dev iteration. 2. readOptionalSecret returned the empty string '' for an empty file instead of null. The path was that an operator would `touch deploy/secrets/discord-guild-id` to satisfy the bind-mount without actually setting a guild, intending to register globally — but the empty string flowed into Discord's slash-command registration API as an explicit guild ID. .trim() + explicit empty-string→null guard fixes it; two regression tests cover empty and whitespace-only files. 3. deploy/README.md implied that S3 setup is required for everything. The v1 gateway daemon validates S3 credentials at startup but does not actually write to S3 — that arrives in Units 5-7 with the workspace agent. New "Testing-Only Configuration" subsection documents the minimum viable secrets for verifying just the Discord plumbing, and the bucket-scoping subsection now cross-references it. Also adds deploy/.env.example (gitignored copy target) documenting OBJECT_STORE_HOSTS with v1 context, and deploy/.gitignore as belt-and-suspenders for .env exclusion at the directory level. Tests: packages/gateway 82 -> 84 (+2). docker compose config validates end-to-end with all six secret files present (including the new discord-guild-id, which may be empty).
1813662 to
bcd96cf
Compare
fro-bot
left a comment
There was a problem hiding this comment.
Verdict: PASS
Both issues raised in the previous review have been fully addressed:
-
README
s3-endpointtouch — the setup block now includestouch deploy/secrets/s3-endpointbefore the commented-out echo, matching the pattern established fordiscord-guild-id. Blocking issue resolved. -
Env-var empty-string asymmetry —
readOptionalSecretnow appliesvalue.trim() !== ''on the env-var path, making it consistent with the file-backed path. The non-blocking concern is fixed, not deferred. -
Tests for env-var normalization — two new cases cover the empty-string and whitespace-only env-var paths, closing the test gap noted in the previous review.
Blocking issues
None.
Non-blocking concerns
None.
Missing tests
None. The suite now covers all paths: file empty → null, file whitespace-only → null, file leading-whitespace preserved, env-var empty → null, env-var whitespace-only → null, file-over-env precedence, missing-file fallthrough, and all loadGatewayConfig branches.
Risk assessment (LOW)
All changes are deploy-ergonomics and config-helper fixes. The readOptionalSecret env-var normalization is strictly safer (fewer downstream surprises). No runtime behavior changes to the gateway, slash commands, or mention handler.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | fro-bot/agent |
| Run ID | 26003823377 |
| Cache | hit |
| Session | ses_1c813d1a1ffebYiOt16N5nMah7 |
Three friction points for someone bringing up the deploy stack for the first time on their own Discord server.
1.
DISCORD_GUILD_IDwasn't plumbed through composeconfig.tsalready read it viareadOptionalSecret, but the secret-file mount and the*_FILEenv-var pointer were missing fromcompose.yaml. Without this, slash commands register globally and take up to an hour to propagate. With it, guild-scoped registration propagates in ~5 seconds — critical for dev iteration.2.
readOptionalSecretmishandled empty filesThe previous behavior was
readFileSync(...).trimEnd()— strips trailing whitespace, returns the remainder. For an empty file, it returned''(empty string), notnull. That meant if youtouch deploy/secrets/discord-guild-idto satisfy the bind-mount but didn't want a guild override, the empty string flowed into Discord's slash-command registration API as an explicit guild ID, which would fail oddly.Fix is a two-step:
trimEnd()for the value we return (preserves leading whitespace, strips the trailing newline fromecho), then.trim() === ''as the emptiness check (whitespace-only and empty files →null). The env-var path was already correct; this just brings the file-backed path in line with it.Regression tests:
nullnull3. README implied S3 setup is required for everything
The v1 gateway daemon validates S3 credentials at startup but does not actually write to S3 — that arrives with the workspace agent in the next units. New "Testing-Only Configuration" subsection documents the minimum viable secrets for verifying just the Discord plumbing, and the bucket-scoping subsection cross-references it.
Also adds
deploy/.env.exampledocumentingOBJECT_STORE_HOSTSwith v1 context, and a deploy-directory.gitignoreas belt-and-suspenders for.env.What this PR does NOT change
Workspace agent stays a
sleep infinityplaceholder. Slash command surface stays/fro-bot pingplus the unknown-command ack path. Mention handler stays the v1 "create thread, post pong" behavior. Those are upcoming-unit work.Verification
pnpm --filter @fro-bot/gateway test: 85/85 (was 84, +1 leading-whitespace regression guard)pnpm --filter @fro-bot/gateway lint: 0 errorspnpm --filter @fro-bot/gateway check-types: 0 errorsdocker compose -f deploy/compose.yaml config --quiet: validates end-to-end with all six secret files present