broker: accept org IDs in deprecated workspace alias schema#199
broker: accept org IDs in deprecated workspace alias schema#199benvinegar wants to merge 2 commits intomainfrom
Conversation
Greptile SummaryThis PR migrates the baudbot broker integration from Slack workspace IDs ( Key changes:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Env at startup] --> B{applyGatewayEnvAliases}
B --> C{GATEWAY_BROKER_ORG_ID set?}
C -- yes --> D[resolved.SLACK_BROKER_ORG_ID = GATEWAY value]
C -- no --> E{SLACK_BROKER_ORG_ID set?}
E -- yes --> F[resolved.SLACK_BROKER_ORG_ID = SLACK value]
E -- no --> G{GATEWAY_BROKER_WORKSPACE_ID set?}
G -- yes --> H[resolved.SLACK_BROKER_ORG_ID = GATEWAY workspace value\n⚠️ deprecated warning]
G -- no --> I{SLACK_BROKER_WORKSPACE_ID set?}
I -- yes --> J[resolved.SLACK_BROKER_ORG_ID = SLACK workspace value\n⚠️ deprecated warning\n⚠️ ALSO general alias warning → GATEWAY_BROKER_WORKSPACE_ID]
I -- no --> K[brokerOrgId = empty]
D & F & H & J --> L[resolved.SLACK_BROKER_WORKSPACE_ID = brokerOrgId]
L --> M[process.env mutated]
M --> N{Required key check:\nSLACK_BROKER_ORG_ID present?}
N -- yes --> O[broker-bridge.mjs starts]
N -- no --> P[process.exit 1]
Last reviewed commit: 71d2d1b |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } else if (hasGatewayWorkspaceId) { | ||
| brokerOrgId = String(gatewayWorkspaceId); | ||
| warnings.push("⚠️ GATEWAY_BROKER_WORKSPACE_ID is deprecated; use GATEWAY_BROKER_ORG_ID."); | ||
| } else if (hasSlackWorkspaceId) { | ||
| brokerOrgId = String(slackWorkspaceId); | ||
| warnings.push("⚠️ SLACK_BROKER_WORKSPACE_ID is deprecated; use GATEWAY_BROKER_ORG_ID/SLACK_BROKER_ORG_ID."); |
There was a problem hiding this comment.
Contradictory deprecation warnings for SLACK_BROKER_WORKSPACE_ID
When only SLACK_BROKER_WORKSPACE_ID is set (the common legacy case), users will receive two contradictory warnings:
-
From the general alias loop at the top of
resolveGatewayEnvAliases(since["GATEWAY_BROKER_WORKSPACE_ID", "SLACK_BROKER_WORKSPACE_ID"]is still inGATEWAY_ENV_ALIAS_PAIRS):"Using legacy SLACK_* env vars (SLACK_BROKER_WORKSPACE_ID→GATEWAY_BROKER_WORKSPACE_ID); set GATEWAY_* aliases instead"
This tells the user to migrate to
GATEWAY_BROKER_WORKSPACE_ID— but that key is also deprecated in this PR. -
From the new custom block here:
"SLACK_BROKER_WORKSPACE_ID is deprecated; use GATEWAY_BROKER_ORG_ID/SLACK_BROKER_ORG_ID."
This is the correct guidance.
A user following warning #1 would set another deprecated key (GATEWAY_BROKER_WORKSPACE_ID) and trigger the same situation again. The existing test ("falls back from deprecated workspace id when org id is absent") only asserts the second warning is present — it does not assert the absence of the first misleading one.
Consider filtering out workspace ID pairs from legacyFallbackKeys when the new org-ID resolution block has already emitted a more specific warning, or moving the workspace/org ID handling entirely out of GATEWAY_ENV_ALIAS_PAIRS into the custom block below.
Prompt To Fix With AI
This is a comment left during a code review.
Path: gateway-bridge/env-aliases.mjs
Line: 96-101
Comment:
**Contradictory deprecation warnings for `SLACK_BROKER_WORKSPACE_ID`**
When only `SLACK_BROKER_WORKSPACE_ID` is set (the common legacy case), users will receive two **contradictory** warnings:
1. From the general alias loop at the top of `resolveGatewayEnvAliases` (since `["GATEWAY_BROKER_WORKSPACE_ID", "SLACK_BROKER_WORKSPACE_ID"]` is still in `GATEWAY_ENV_ALIAS_PAIRS`):
> "Using legacy SLACK_* env vars (SLACK_BROKER_WORKSPACE_ID→GATEWAY_BROKER_WORKSPACE_ID); set GATEWAY_* aliases instead"
This tells the user to migrate to `GATEWAY_BROKER_WORKSPACE_ID` — but that key is **also deprecated** in this PR.
2. From the new custom block here:
> "SLACK_BROKER_WORKSPACE_ID is deprecated; use GATEWAY_BROKER_ORG_ID/SLACK_BROKER_ORG_ID."
This is the correct guidance.
A user following warning #1 would set another deprecated key (`GATEWAY_BROKER_WORKSPACE_ID`) and trigger the same situation again. The existing test (`"falls back from deprecated workspace id when org id is absent"`) only asserts the second warning is present — it does not assert the absence of the first misleading one.
Consider filtering out workspace ID pairs from `legacyFallbackKeys` when the new org-ID resolution block has already emitted a more specific warning, or moving the workspace/org ID handling entirely out of `GATEWAY_ENV_ALIAS_PAIRS` into the custom block below.
How can I resolve this? If you propose a fix, please make it concise.
|
Closed for #200 |
1 participant
Summary
Root cause
intentionally mirrors org ID into for backward compatibility. The schema still enforced on that deprecated key, causing false config failures.
Verification
✔ parseArgs accepts --workspace-id as backward-compatible alias for --org-id (0.09017ms)
✔ parseArgs sets verbose=true for -v and --verbose (0.064ms)
✔ isMainModule handles symlink argv path (0.337995ms)
✔ parseArgs accepts registration token (0.066274ms)
✔ parseArgs rejects unknown arguments (0.208462ms)
✔ parseArgs rejects legacy auth-code argument (0.058329ms)
✔ validation helpers normalize and enforce broker/org formats (0.18087ms)
✔ mapRegisterError returns actionable messages (0.17022ms)
✔ registerWithBroker fetches pubkeys then posts registration payload (1.861459ms)
✔ registerWithBroker sends registration_token when provided (0.342795ms)
✔ runRegistration integration path succeeds against live local HTTP server (18.70844ms)
✔ env schema accepts org IDs for deprecated workspace aliases (0.195688ms)
✅ Agent restarted.
✔ runRegistration does not write SLACK_BOT_TOKEN even when broker returns encrypted_bot_token (2.169107ms)
✔ runRegistration requires registration token (0.13865ms)
✔ upsertEnvContent updates existing values and appends new ones (0.184126ms)
✔ lookupUser parses passwd entries correctly (0.100188ms)
✔ resolveConfigTargets uses BAUDBOT_CONFIG_USER when set (0.178956ms)
✔ resolveConfigTargets throws for unknown BAUDBOT_CONFIG_USER (0.072226ms)
✔ parseArgs accepts --no-restart flag (0.027422ms)
✔ parseArgs defaults noRestart to false (0.020098ms)
✔ restartAgent returns false and warns when execFileSync fails (0.113533ms)
✔ restartAgent calls systemctl restart baudbot on success (0.073739ms)
ℹ tests 23
ℹ suites 0
ℹ pass 23
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 77.780798
Checked 43 files in 21ms. No fixes applied.
✅ ShellCheck passed (61 files)
RUN v4.0.18 /home/bentlegen/Projects/baudbot-beta
✓ test/shell-scripts.test.mjs (13 tests) 28955ms
✓ subagents cli 28450ms
✓ test/broker-bridge.integration.test.mjs (13 tests) 2587ms
✓ pi/extensions/agent-spawn.test.mjs (4 tests) 1034ms
✓ returns readiness timeout and does not issue cleanup commands 1003ms
✓ test/legacy-node-tests.test.mjs (5 tests) 794ms
✓ extension scanner 501ms
✓ test/security-audit.test.mjs (3 tests) 250ms
✓ test/github-events.test.mjs (1 test) 65ms
✓ test/integrity-status-check.test.mjs (3 tests) 48ms
✓ pi/extensions/subagent-manager.test.mjs (4 tests) 6ms
✓ pi/extensions/heartbeat.test.mjs (73 tests) 4ms
✓ pi/extensions/memory.test.mjs (49 tests) 4ms
✓ pi/extensions/subagent-util.test.mjs (2 tests) 4ms
Test Files 11 passed (11)
Tests 170 passed (170)
Start at 21:28:59
Duration 34.81s (transform 89ms, setup 0ms, import 450ms, tests 33.75s, environment 1ms)
DigitalOcean Arch droplet end-to-end check: