fix(gateway): honor platforms.signal.enabled=false over SIGNAL_* env vars - #11103
fix(gateway): honor platforms.signal.enabled=false over SIGNAL_* env vars#11103briandevans wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a config precedence bug in the messaging gateway where platforms.signal.enabled: false in config.yaml was being overridden by SIGNAL_HTTP_URL + SIGNAL_ACCOUNT environment variables.
Changes:
- Update
gateway/config.pyso Signal is only force-enabled from env vars when Signal is not already present inconfig.platforms. - Continue populating Signal
extrafields from env vars even when YAML disables Signal, so re-enabling later in YAML uses current env connection details. - Add regression tests covering YAML-disabled + env, YAML-enabled + env, and env-only configuration.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
gateway/config.py |
Adjust Signal env override logic to preserve YAML-declared enabled while still filling extra connection details. |
tests/gateway/test_config.py |
Add regression tests validating the updated Signal precedence behavior across common config/env combinations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Not declared in config.yaml — env vars alone enable Signal. | ||
| config.platforms[Platform.SIGNAL] = PlatformConfig(enabled=True) | ||
| # When platforms.signal is already declared in config.yaml, preserve | ||
| # its ``enabled`` value (including an explicit ``enabled: false``) so | ||
| # env vars only supply connection defaults, not override the YAML | ||
| # opt-out. Env vars still update ``extra`` so a re-enable in YAML | ||
| # picks up the current env-supplied url/account without another | ||
| # restart cycle. See issue #11096 (Bug 3). |
There was a problem hiding this comment.
The comment says this preserves enabled only when Signal is declared in config.yaml, but the condition is Platform.SIGNAL not in config.platforms, which also treats legacy gateway.json entries as “declared” and will therefore prevent SIGNAL_HTTP_URL/SIGNAL_ACCOUNT from auto-enabling Signal in that case. If that’s intended, please update the comment (and ideally add a regression test for the gateway.json path); if it’s not intended, consider distinguishing YAML-declared platforms from legacy defaults before deciding whether to force enabled=True.
| # Not declared in config.yaml — env vars alone enable Signal. | |
| config.platforms[Platform.SIGNAL] = PlatformConfig(enabled=True) | |
| # When platforms.signal is already declared in config.yaml, preserve | |
| # its ``enabled`` value (including an explicit ``enabled: false``) so | |
| # env vars only supply connection defaults, not override the YAML | |
| # opt-out. Env vars still update ``extra`` so a re-enable in YAML | |
| # picks up the current env-supplied url/account without another | |
| # restart cycle. See issue #11096 (Bug 3). | |
| # No existing Signal platform entry was loaded, so env vars alone | |
| # enable Signal. | |
| config.platforms[Platform.SIGNAL] = PlatformConfig(enabled=True) | |
| # When a Signal platform entry already exists in ``config.platforms`` | |
| # (for example from config.yaml or legacy gateway.json loading), | |
| # preserve its ``enabled`` value, including an explicit | |
| # ``enabled: false``. In that case env vars only supply connection | |
| # defaults and do not override the existing opt-out. Env vars still | |
| # update ``extra`` so a later re-enable picks up the current | |
| # env-supplied url/account without another restart cycle. See issue | |
| # #11096 (Bug 3). |
|
Addressed the Copilot review in
Precedence truth table is unchanged:
Validation: |
SIGNAL_HTTP_URL + SIGNAL_ACCOUNT env vars unconditionally overwrote config.platforms[Platform.SIGNAL].enabled to True inside _apply_env_overrides, clobbering an explicit platforms.signal.enabled=false set in config.yaml. Operators wiring Signal deliberately to a separate service saw it silently re-enabled; diagnosing required removing the env vars because logs surfaced no hint the YAML flag had been dropped. The env-var block now only forces enabled=True when platforms.signal is absent from config.yaml (fresh PlatformConfig). When the YAML already declared the platform — including with enabled=false — the declared flag is preserved. Env vars still populate extra.http_url / extra.account / extra.ignore_stories, so a later enabled=true flip reuses the current env-supplied connection details without needing to re-export. Narrow scope: Signal only. The same pattern exists across several other platform blocks in _apply_env_overrides and is intentionally left for a follow-up so this change is small and easy to review. Regression tests added to tests/gateway/test_config.py: - test_signal_yaml_disabled_beats_env_vars - test_signal_yaml_enabled_preserved_with_env_vars - test_signal_env_vars_alone_enable_platform Fixes part of NousResearch#11096 (Bug 3).
Addresses the Copilot review note on PR NousResearch#11103: the previous comment claimed the preservation applied only when Signal was declared in ``config.yaml``, but the actual condition (``Platform.SIGNAL not in config.platforms``) covers any existing persisted source — including legacy ``gateway.json`` — because JSON is merged into ``config.platforms`` before ``_apply_env_overrides`` runs. - Update the inline comment to reflect the real semantics. - Add ``test_signal_gateway_json_disabled_beats_env_vars`` regression test so the JSON-only opt-out path is covered. Validation: ``python -m pytest tests/gateway/test_config.py -q -k signal`` -> 4 passed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
909f8d2 to
9c9d0e3
Compare
|
Rebased onto current Focused tests still pass on the rebased base: The four signal-precedence tests added by this PR all pass:
The bug is still present on |
|
Closing to keep the queue clean — happy to reopen if this is still useful. |
What does this PR do?
Fixes the issue #11096 Bug 3 precedence bug where setting
platforms.signal.enabled: falseinconfig.yamlwas silently overwritten bySIGNAL_HTTP_URL+SIGNAL_ACCOUNTenv vars. The operator who filed the bug spent ~30 min diagnosing — the YAML flag was never honored, and the only workaround was to comment out the env vars.Root cause.
_apply_env_overridesingateway/config.pyunconditionally assignedconfig.platforms[Platform.SIGNAL].enabled = Truewhenever both env vars were set, regardless of whatconfig.yamlhad already loaded intoconfig.platforms[Platform.SIGNAL].enabledviaGatewayConfig.from_dict().Smallest safe fix.
enabled=Trueis now only applied when Signal is not already declared inconfig.platforms(freshPlatformConfig). Whenplatforms.signalis inconfig.yaml— including withenabled: false— the YAML-declared flag is preserved. Env vars continue to populateextra.http_url,extra.account, andextra.ignore_stories, so a later YAML flip back toenabled: truepicks up current env-supplied connection details without another export cycle.Related Issue
Fixes part of #11096 (Bug 3 —
config.yamlsignal.enabled: falseis silently ignored whenSIGNAL_*env vars are set).Type of Change
Changes Made
gateway/config.py— Signal block in_apply_env_overrides: only forceenabled=Trueon freshPlatformConfigcreation; preserve YAML-declared.enabledwhen the platform already exists. Still updatesextraso env-supplied connection details are retained.tests/gateway/test_config.py— three regression tests covering YAML-disabled + env vars (the bug), YAML-enabled + env vars (mixed), and env vars alone (preserve documented behavior for purely env-driven Signal configuration).Backward compatibility / precedence
SIGNAL_HTTP_URL+SIGNAL_ACCOUNTstill enable Signal end-to-end. Documented.env-only configuration path is unchanged.enabled: true+ env vars: Signal stays enabled; env vars continue to fill inextra.http_url/extra.account. Unchanged.enabled: false+ env vars (the bug): Signal now stays disabled as the operator intended.extrais still populated so a later YAML flip doesn't require re-exporting env vars.platforms.signal: {}(empty block) + env vars: Signal remains at its YAML-derived default (enabled=False). Users who declare a platform block in YAML opt into YAML-authoritative behavior; setenabled: trueexplicitly to combine with env-supplied details. This is the same precedence principle Three bugs in v0.9.0: context_length override, thinking-block sessions, config.yaml vs env precedence #11096 recommends.Narrow scope — why only Signal
The same
enabled = Truepattern exists across other blocks in_apply_env_overrides(Discord, Mattermost, Matrix, etc. — 19 total). I deliberately kept this PR narrow to the platform the issue flagged so the fix is small, easy to review, and easy to revert if the precedence semantics need tuning. Once the precedence model is agreed, the same treatment can extend to the rest in follow-up PRs.How to Test
Repro on
main(fails before this PR):Regression suite:
Validation
All commands run with
source venv/bin/activate:python -m pytest tests/gateway/test_config.py -q -k signal— 3 passed (new regression tests)python -m pytest tests/gateway/test_config.py tests/gateway/test_signal.py -q— 87 passedpython -m pytest tests/gateway/ -q --tb=short— 3016 passed, 1 skipped. The 2 residual failures (test_approve_deny_commands.py::TestBlockingApprovalE2E::test_blocking_approval_approve_onceand::test_blocking_approval_deny) also fail on cleanorigin/main— they are pre-existing and unrelated. A third flaky failure (test_non_internal_event_without_user_triggers_pairing) passes in isolation on this branch; it is a test-ordering issue unrelated to this change.git diff --check— cleanpython -m py_compile gateway/config.py tests/gateway/test_config.py— OKTested on macOS (Darwin 25.5.0), Python 3.11.15.
No standalone lint/typecheck command is defined in the contributor docs; the Tests workflow (
.github/workflows/tests.yml) runspython -m pytest tests/ -q --ignore=tests/integration --ignore=tests/e2e --tb=short -n auto, matching what CI will execute.Checklist
fix(gateway): ...)