fix(gateway): plumb DISCORD_PRIVILEGED_INTENTS through compose#652
Conversation
fro-bot
left a comment
There was a problem hiding this comment.
Verdict: CONDITIONAL
The compose wiring is correct and the pattern is consistent with the existing optional secrets. One issue needs to be addressed before merge.
Blocking issues
create_host_path: false breaks cold-start for an optional secret.
The new volume mount uses create_host_path: false, which means docker compose up will fail with a Compose error if deploy/secrets/discord-privileged-intents does not exist on the host. This is the stated and correct behavior for required secrets (where a missing file is always a bug). But this is an optional secret — operators who do not need privileged intents should not have to create a placeholder file just to bring the stack up.
Every other optional secret in this compose file (aws-access-key-id, aws-secret-access-key, aws-session-token, discord-guild-id) carries the same create_host_path: false constraint, but the README's setup section tells operators to touch each of those files unconditionally — so the missing-file error is effectively neutralized by the setup steps. The same pattern applies here: the README correctly adds touch deploy/secrets/discord-privileged-intents to the setup block, so operators following the README will never hit the error.
However, operators who followed the README for #651 (and for any prior setup before #652 landed) will not have this file. They will get a hard Compose failure on the next docker compose up upgrade unless they know to touch the new file. This is a silent breaking change for existing deployments.
Options:
- Document the migration step explicitly in the PR description (and ideally in the README's upgrade notes). A one-line "If upgrading from before this PR, run
touch deploy/secrets/discord-privileged-intents" under a## Upgradingsection in the README is sufficient. - Alternatively, change to
create_host_path: truefor this optional secret only, accepting that a missing file materializes as an empty directory (Docker default), which the gateway will silently ignore (matching the "empty = not set" contract). Less ideal for diagnostics, but zero friction for existing deployments.
Option 1 is preferred — it preserves the fail-fast diagnostic value while making the migration path explicit.
Non-blocking concerns
-
The
DISCORD_PRIVILEGED_INTENTS_FILEenv var comment in theenvironment:block (# Optional — opt into Discord privileged intents (MessageContent, GuildMembers)) is slightly inconsistent with thediscord-guild-idcomment style, which reads# Optional — guild-scoped slash command registration (fast dev propagation). Minor, but if you want full consistency, add a parenthetical explaining the opt-in shape (the comment already does this, so this is truly a nit). -
The README comment block above the
touchcommand is detailed and helpful, but it duplicates some of what's inAGENTS.md. Not a problem — README is operator-facing, AGENTS.md is agent-facing.
Missing tests
None — this PR touches only deploy infrastructure (compose.yaml and README.md). The gateway config parsing and client wiring for DISCORD_PRIVILEGED_INTENTS were tested in #651.
Risk assessment (MED): likelihood of regression, security exposure, or blast radius
- Regression risk: MED. Existing operators who upgrade without running
touch deploy/secrets/discord-privileged-intentswill get a harddocker compose upfailure. The blast radius is limited to the deploy path (not the gateway runtime), and the fix is a singletouchcommand, but the failure mode is silent from the PR description's "None" migration note. - Security exposure: LOW. The privileged intents opt-in remains gated behind a file on the host. An empty or absent file correctly produces the non-privileged baseline. No new attack surface introduced.
- Blast radius: LOW if the migration note is added; MED without it.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | fro-bot/agent |
| Run ID | 26146865667 |
| Cache | hit |
| Session | ses_1bbcdf1aeffe63WUujGeHPEsEl |
The gateway already reads the privileged-intents env var from `$DISCORD_PRIVILEGED_INTENTS_FILE` (PR #651), but compose.yaml didn't pass it through. Add the matching `_FILE` env line and a long-syntax bind mount with `create_host_path: false` so missing secret files fail at `docker compose up` time instead of silently becoming directories. README now lists the `touch` step alongside the other optional Discord secrets.
1d904b7 to
944505d
Compare
fro-bot
left a comment
There was a problem hiding this comment.
Verdict: PASS
The blocking issue from the previous review is resolved. The new ## Upgrading existing deployments section in the README addresses it cleanly — and goes further by establishing a documented pattern for all future optional secrets (idempotent touch block, inventory table, explicit guidance to add a row when new optional secrets land). The migration step is also documented in the PR description.
Blocking issues
None.
Non-blocking concerns
None beyond the comment-style nit from the previous review, which was already accepted as minor.
Missing tests
None — deploy infrastructure only; no source code touched.
Risk assessment (LOW): likelihood of regression, security exposure, or blast radius
- Regression risk: LOW. The upgrade path is now explicit and idempotent. Operators who follow the README (new or existing) will not hit the missing-file error.
- Security exposure: LOW. No change from the previous assessment.
- Blast radius: LOW. The
## Upgrading existing deploymentssection also retroactively documents the other optional secrets (aws-*,s3-endpoint,discord-guild-id), which is a net improvement for existing operators regardless of this PR.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | fro-bot/agent |
| Run ID | 26147113767 |
| Cache | hit |
| Session | ses_1bbcdf1aeffe63WUujGeHPEsEl |
Completes the deploy-contract surface for the privileged-intents opt-in that landed in #651.
What changes
The gateway has been reading
DISCORD_PRIVILEGED_INTENTS_FILEsince PR #651, butdeploy/compose.yamldidn't pass it through. Operators using infra-as-code (the canonical consumer ismarcusrbrown/infra) had to patchcompose.yamlthemselves or set the var on the host shell that runsdocker compose up. Both worked but broke the "compose.yaml is the contract surface" expectation that the rest of the deploy contract holds.This PR adds:
DISCORD_PRIVILEGED_INTENTS_FILE: /run/secrets/discord_privileged_intentsin the gateway serviceenvironment:block.create_host_path: false(matching the pattern from fix(gateway): harden deploy contract for infra-as-code consumers #649), so missing source files cause a clear Compose error atdocker compose uprather than silently materializing as directories.touch deploy/secrets/discord-privileged-intentsstep indeploy/README.md, alongside the other optional Discord secrets.## Upgrading existing deploymentssection in the README that documents the touch-on-upgrade requirement for new optional secrets, with a current inventory of every optional secret in the stack and when each was introduced.Why
Consistency with the rest of the deploy contract. Operators following the README's setup steps now get the env var threaded into the container without extra work.
Verification
docker compose -f deploy/compose.yaml configvalidates.Migration
Existing deployments must
touch deploy/secrets/discord-privileged-intentsbefore their nextdocker compose up. Without the file, Compose fails fast with a clear missing-source error. The new README section documents this explicitly so future optional secrets follow the same upgrade pattern.Operators who never opt into privileged intents leave the file empty; an empty file is the same as the file being absent.