feat: add profile-based integration routing#163
Open
i-am-thor[bot] wants to merge 37 commits into
Open
Conversation
Co-authored-by: Son Dao <son.dao@katalon.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR migrates Thor’s Slack gating and MCP integration routing from a single global allowlist/credential model to profile-based routing, where Slack channels map to profiles and integrations resolve profile-scoped env vars first with global fallback. This aligns Slack admission policy, MCP availability/advertisement, and approval resolution with per-channel credential targeting.
Changes:
- Replace
slack.private_channel_allowlistwithprofiles.<name>.channels[]in workspace config + add helpers for channel/profile resolution. - Add profile-scoped MCP proxy resolution (
resolveProxyConfig,getAvailableProxyNames) and routeremote-cliMCP calls + approval snapshots by resolved credential target. - Update runner tool instructions, docker compose/env/docs, and targeted unit tests to reflect profile-aware integration availability.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates deployment/config docs to describe routing profiles and profile-scoped env var behavior. |
| packages/common/src/workspace-config.ts | Adds profiles schema + channel/profile lookup helpers; enforces cross-profile channel uniqueness. |
| packages/common/src/workspace-config.test.ts | Updates tests to validate profiles + new helper behavior and duplicate-channel rejection. |
| packages/common/src/proxies.ts | Introduces profile-aware env resolution (resolveProxyConfig) and availability filtering (getAvailableProxyNames). |
| packages/common/src/proxies.test.ts | Adds unit coverage for profile suffix normalization and profile/global fallback behavior. |
| packages/common/src/index.ts | Re-exports new profile/proxy routing helpers from @thor/common. |
| packages/gateway/src/slack-channel-gate.ts | Switches gated-channel admission checks from allowlist to profile membership. |
| packages/gateway/src/slack-api.ts | Updates comments to reflect profile-based gating semantics. |
| packages/gateway/src/service.test.ts | Updates tests to use profiles in workspace config fixtures. |
| packages/gateway/src/app.test.ts | Renames/updates tests and fixtures for “profiled vs unprofiled” gated-channel behavior. |
| packages/remote-cli/src/approval-store.ts | Adds optional routing snapshot (targetKey/profile) to approval actions. |
| packages/remote-cli/src/mcp-handler.ts | Routes MCP connections by resolved credential target key; snapshots routing for approvals; profile-aware listing/tool visibility. |
| packages/remote-cli/src/mcp-handler.test.ts | Adds tests for profile-scoped credential routing and approval routing snapshots. |
| packages/runner/src/tool-instructions.ts | Advertises only MCP upstreams available for the current thread/profile. |
| packages/runner/src/index.ts | Passes correlationKey + configLoader to tool-instructions for profile-aware advertisement. |
| packages/runner/src/tool-instructions.test.ts | Adds compose/env alignment tests for MCP advertisement assumptions. |
| docker-compose.yml | Adds .env loading to runner; loosens Grafana MCP globals to allow “profile-only” routing setups. |
| .env.example | Documents profile-scoped credential env var conventions (suffix normalization, bundles). |
| docs/slack.md | Documents profiles as the gated-channel admission mechanism and profile-driven credential selection. |
| docs/examples/thor.json | Replaces allowlist example with profiles example. |
| docs/feat/security-model.md | Updates security model docs to reference routing profiles. |
| docs/feat/event-flow.md | Updates event flow docs to reference routing profiles for gated Slack channels. |
| docs/plan/2026052701_profile-based-integration-routing.md | Adds an implementation plan/spec for the profile-based routing model. |
Comments suppressed due to low confidence (1)
packages/remote-cli/src/mcp-handler.ts:305
- Reconnect logs (
upstream_reconnecting/upstream_reconnect_failed/upstream_reconnect_exhausted) only includename, but instances are now keyed byproxyDef.target.key. AddtargetKey(and ideallyprofile/envScope) to these log fields so multi-profile reconnection/debugging is unambiguous.
const delay = Math.min(BASE_DELAY_MS * 2 ** (attempt - 1), MAX_DELAY_MS);
logInfo(log, "upstream_reconnecting", { name, attempt, delayMs: delay });
setTimeout(() => {
connectUpstreamFn(name, upstreamConfig, () => scheduleReconnect(1))
.then((newUpstream) => {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+21
to
+38
| it("keeps runner deployment env aligned with env-based MCP advertisement", () => { | ||
| const compose = readFileSync("docker-compose.yml", "utf-8"); | ||
| const runnerBlock = compose.match(/\n runner:\n[\s\S]*?\n gateway:/)?.[0] ?? ""; | ||
|
|
||
| expect(runnerBlock).toContain("env_file:"); | ||
| expect(runnerBlock).toContain("- .env"); | ||
| }); | ||
|
|
||
| it("allows profile-only Grafana deployments in compose", () => { | ||
| const compose = readFileSync("docker-compose.yml", "utf-8"); | ||
| const grafanaBlock = compose.match(/\n grafana-mcp:\n[\s\S]*?\n mitmproxy:/)?.[0] ?? ""; | ||
|
|
||
| expect(grafanaBlock).toContain("GRAFANA_URL=${GRAFANA_URL:-}"); | ||
| expect(grafanaBlock).toContain( | ||
| "GRAFANA_SERVICE_ACCOUNT_TOKEN=${GRAFANA_SERVICE_ACCOUNT_TOKEN:-}", | ||
| ); | ||
| expect(grafanaBlock).not.toContain("GRAFANA_URL:?set GRAFANA_URL"); | ||
| }); |
Contributor
Author
There was a problem hiding this comment.
Fixed in c1d3f5b. The test now resolves docker-compose.yml relative to the test file via import.meta.url, so it no longer depends on the process working directory.
AI-generated — verify before acting. View Thor context
Comment on lines
207
to
+211
| - SLACK_TEAM_ID=${SLACK_TEAM_ID:-} | ||
| - SLACK_BOT_TOKEN=${SLACK_BOT_TOKEN:?set SLACK_BOT_TOKEN} | ||
| - SLACK_API_BASE_URL | ||
| env_file: | ||
| - .env |
Contributor
Author
There was a problem hiding this comment.
Good call. I am not changing that in this PR because the current prompt-side availability check needs access to arbitrary profile-suffixed integration env vars, and passing the existing compose env through the runner keeps runner/remote-cli advertisement in sync without adding a new internal API during this routing change. I agree the blast radius is broader than ideal; a follow-up to derive availability from remote-cli (or a narrower metadata surface) would be a better long-term fix.
AI-generated — verify before acting. View Thor context
Comment on lines
+395
to
+396
| const proxyDef = resolveProxyConfig(name, routing.profile); | ||
| if (!proxyDef || proxyDef.target.key !== routing.targetKey) return undefined; |
Contributor
Author
There was a problem hiding this comment.
Fixed in d11ccf3. Approval routing now stores envScope and re-resolves against that exact scope during approval resolution, so a pending action that originally used global fallback in a profiled channel stays on the global target even if profile-specific creds are added before approval.
AI-generated — verify before acting. View Thor context
Comment on lines
917
to
921
| getHealth(): Record<string, unknown> { | ||
| return { | ||
| configured: PROXY_NAMES.length, | ||
| connected: PROXY_NAMES.filter((name) => instances.has(name)).length, | ||
| configured: getAvailableProxyNames().length, | ||
| connected: instances.size, | ||
| instances: Object.fromEntries( |
Contributor
Author
There was a problem hiding this comment.
Fixed in d11ccf3. /health now reports the union of globally available upstreams plus profile-scoped upstreams discovered across configured profiles, with a best-effort fallback to global-only if workspace config cannot be loaded.
AI-generated — verify before acting. View Thor context
Comment on lines
+110
to
+114
| | `ATLASSIAN_AUTH` | No | `remote-cli`, `mitmproxy` | Global Atlassian MCP auth header and mitmproxy default injection; profile variants use `_PROFILE` suffixes | | ||
| | `POSTHOG_API_KEY` | No | `remote-cli` | Global PostHog MCP auth; profile variants use `_PROFILE` suffixes | | ||
| | `GRAFANA_URL` | No | `grafana-mcp`, `remote-cli` | Global Grafana instance URL; profile variants use `_PROFILE` suffixes | | ||
| | `GRAFANA_SERVICE_ACCOUNT_TOKEN` | No | `grafana-mcp`, `remote-cli` | Global Grafana service account token; profile variants use `_PROFILE` suffixes | | ||
| | `GRAFANA_ORG_ID` | No | `grafana-mcp` | Grafana org ID (defaults to `1`); profile variants use `_PROFILE` suffixes | |
Contributor
Author
There was a problem hiding this comment.
Fixed in d11ccf3. I updated the env-var table wording to say _<NORMALIZED_PROFILE_NAME> so the suffix contract is explicit instead of reading like a literal _PROFILE suffix.
AI-generated — verify before acting. View Thor context
Comment on lines
930
to
943
| getHealth(): Record<string, unknown> { | ||
| const configured = new Set(getAvailableProxyNames()); | ||
| try { | ||
| const config = getConfig(); | ||
| for (const profileName of Object.keys(config.profiles ?? {})) { | ||
| for (const name of getAvailableProxyNames(profileName)) configured.add(name); | ||
| } | ||
| } catch { | ||
| // Best-effort only; health falls back to global-only visibility when config cannot be loaded. | ||
| } | ||
| return { | ||
| configured: PROXY_NAMES.length, | ||
| connected: PROXY_NAMES.filter((name) => instances.has(name)).length, | ||
| configured: configured.size, | ||
| connected: instances.size, | ||
| instances: Object.fromEntries( |
Contributor
Author
There was a problem hiding this comment.
Fixed in 0c377cc. /health now keeps configured and connected on the same upstream-name basis, and also reports a separate connectedTargets count for credential-target fanout so operators can still see how many distinct profile/global targets are live.
AI-generated — verify before acting. View Thor context
Comment on lines
+56
to
+63
| function normalizeProfileKey(profileName: string): string { | ||
| const normalized = profileName.trim().toUpperCase().replace(/[^A-Z0-9]+/g, "_"); | ||
| let start = 0; | ||
| let end = normalized.length; | ||
| while (start < end && normalized[start] === "_") start += 1; | ||
| while (end > start && normalized[end - 1] === "_") end -= 1; | ||
| return normalized.slice(start, end); | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Fixed in 0c377cc. I extracted the normalization logic into a shared packages/common/src/profile-normalization.ts helper and reused it from both workspace-config validation and proxy/env resolution so the suffix rules stay in sync.
AI-generated — verify before acting. View Thor context
Comment on lines
+266
to
+270
| function resolveProfileForContext(context: McpCommandContext): string | undefined { | ||
| if (!context.sessionId) return undefined; | ||
| try { | ||
| return getProfileForSlackCorrelationKey(getConfig(), findSlackTriggerCorrelationKey(context.sessionId)); | ||
| } catch (err) { |
Contributor
Author
There was a problem hiding this comment.
Fixed in 57b6ef6. Profile routing now derives from the current trigger correlation key, so non-Slack sessions no longer inherit an older Slack thread profile from the same anchor and therefore fall back to unsuffixed global credentials as documented.
AI-generated — verify before acting. View Thor context
Comment on lines
+24
to
+30
| it("keeps runner deployment env aligned with env-based MCP advertisement", () => { | ||
| const compose = readFileSync(composePath, "utf-8"); | ||
| const runnerBlock = compose.match(/\n runner:\n[\s\S]*?\n gateway:/)?.[0] ?? ""; | ||
|
|
||
| expect(runnerBlock).toContain("env_file:"); | ||
| expect(runnerBlock).toContain("- .env"); | ||
| }); |
Contributor
Author
There was a problem hiding this comment.
Fixed in 57b6ef6. The runner compose assertions no longer depend on neighboring service order; the test now extracts the named service block directly before asserting on env_file.
AI-generated — verify before acting. View Thor context
Comment on lines
+32
to
+41
| it("allows profile-only Grafana deployments in compose", () => { | ||
| const compose = readFileSync(composePath, "utf-8"); | ||
| const grafanaBlock = compose.match(/\n grafana-mcp:\n[\s\S]*?\n mitmproxy:/)?.[0] ?? ""; | ||
|
|
||
| expect(grafanaBlock).toContain("GRAFANA_URL=${GRAFANA_URL:-}"); | ||
| expect(grafanaBlock).toContain( | ||
| "GRAFANA_SERVICE_ACCOUNT_TOKEN=${GRAFANA_SERVICE_ACCOUNT_TOKEN:-}", | ||
| ); | ||
| expect(grafanaBlock).not.toContain("GRAFANA_URL:?set GRAFANA_URL"); | ||
| }); |
Contributor
Author
There was a problem hiding this comment.
Fixed in 57b6ef6. The Grafana compose assertion now extracts the grafana-mcp service block directly instead of relying on the following service name, so unrelated service reordering no longer breaks the test.
AI-generated — verify before acting. View Thor context
Atlassian's MCP path honors profile-suffixed credentials, but mitmproxy's
`${ATLASSIAN_AUTH}` interpolation is global and session-agnostic, so direct
HTTP egress (curl, node fetch) bypasses profile routing. PostHog is MCP-only
and has no such gap, making it the cleaner canonical example. Also notes the
cron-trigger profile gap as a follow-up.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Resolves profile by enumerating every slack.thread alias on the session's anchor and fails fast when channels map to more than one profile, so a session bound to threads in different profiles can never silently flip credentials mid-call. Drops the legacy slack.thread_id alias type and the no-channel correlation key form. Moves approval routing from creation-time snapshot to click-time re-resolve, marking the action rejected with a system reason when the fresh resolution is ambiguous or the integration is unavailable. Grafana with a partial profile-suffixed bundle now throws instead of silently falling back to the unsuffixed bundle. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
buildSlackCorrelationKeys took channel as optional and returned [] when missing, which threaded the "maybe no channel" case through every caller. Under the strict channel/profile model a slack thread without a channel cannot be bound to an anchor at all, so callers should decide whether they have a channel before calling, not after. Renamed to buildSlackCorrelationKey, made channel required, and pushed the guard into the one caller (slack-post-message) that might not have one. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Drops the legacy row from docs/feat/event-flow.md (the live reference) and adds forward-pointing post-implementation notes to the two plan docs whose non-goals or schema definitions are now incompatible with Phase 6 of profile-based routing. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…tegration-routing
Constrain profile names to /^[A-Z_]+$/ at schema validation so the profile name itself is the env-suffix. Drops the normalize-and-collision- check path and the separate profile-normalization module. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Delete `getSlackCorrelationKeys` (the vestigial array-returning helper from the legacy/new key migration) and have `getSlackCorrelationKey` delegate to `buildSlackCorrelationKey` so the `slack:thread:` prefix lives in one place. Drop the dead `correlationKey !== rawKeys[0]` log branches in app.ts — `resolveCorrelationKeys([k])` always returns `k`, so they never fired. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The registry mixed static policy with templated upstream headers that were never interpolated — resolveProxyConfig spread the policy then overwrote upstream entirely. Inline each proxy's url/allow/approve into its branch and migrate the two getProxyConfig callers to resolveProxyConfig with the profile already in scope. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Move MCP affordances to the static build.md agent doc plus the live `mcp` CLI discovery flow, which hits the real profile resolver instead of a session-start snapshot. The injected block mostly duplicated build.md, and its only unique value — per-profile filtering — is delivered more accurately by `mcp` discovery. The proxy already fails closed server-side, so the prompt-side suppression was belt-and-suspenders. Deletes tool-instructions.ts + its test + the runner injection path, and prunes code orphaned along the way: the templated-header interpolation helpers (interpolateEnv/interpolateHeaders) left over from the PROXY_REGISTRY removal, the unused resolveStrictProfileForAnchor export, findActiveTriggerOrThrow + ActiveTriggerSnapshot (zero references), and the Phase 6 leftovers findTriggerCorrelationKey / getProfileForSlackCorrelationKey / getSlackChannelFromCorrelationKey / findUserByEmail (no production consumers). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tegration-routing
…tegration-routing # Conflicts: # package.json # packages/common/src/correlation.ts # packages/common/src/disclaimer.ts # packages/common/src/index.ts # packages/common/src/proxies.test.ts # packages/common/src/proxies.ts # packages/common/src/workspace-config.test.ts # packages/common/src/workspace-config.ts # packages/remote-cli/src/sandbox.test.ts # packages/runner/src/index.ts # packages/runner/src/tool-instructions.test.ts # scripts/test-mcp.ts
The merge in 3f66a02 resolved disclaimer.ts, workspace-config.ts, and proxies.ts toward origin/main, silently reverting cleanup this branch had done. The restored symbols compiled (dead code), so CI stayed green. Re-apply the branch's intended removals: - disclaimer.ts: findActiveTriggerOrThrow + ActiveTriggerSnapshot (no callers) - workspace-config.ts: interpolateEnv/interpolateHeaders/findUserByEmail/ getSlackPrivateChannelAllowlist + the private_channel_allowlist schema (superseded by profiles per plan) + dead ProxyConfig/ProxyUpstream - proxies.ts: PROXY_REGISTRY + getProxyConfig (plan replaces the registry with resolveProxyConfig; production already uses only the resolver) - index.ts: the matching dead barrel exports - workspace-config.test.ts: dangling getSlackPrivateChannelAllowlist import Preserve main's approval-inventory safety invariant (and its test), but re-source it from the per-upstream approve consts instead of PROXY_REGISTRY. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
envScope existed only to decorate one connecting_upstream log line. The profile-vs-global distinction is still computed where it matters: scopedEnv's scope feeds targetKey (the instance cache key), and the resolved header values carry the actual credentials. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add repos as a second profile selector alongside Slack channels so non-Slack/cron sessions and unlisted-channel sessions can resolve a profile from the repo they operate in. Channels stay authoritative; the repo fills in when the channel maps to no profile, and a channel/repo profile conflict fails closed. Repos play no part in Slack channel admission. - Schema: profiles.<name>.repos[], require >=1 of channels/repos, dedupe repos within and across profiles. - Resolver: precedence chain (channelProfile ?? repoProfile) with per-dimension ambiguity and cross-dimension conflict failures. - Live MCP path resolves the repo from the trusted OpenCode session directory; the approval-click path falls back to a new immutable `repo` anchor alias stamped once at trigger time. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move sessionId into a required StrictProfileOptions object and make liveRepo a required `string | undefined` field, so every call site declares whether it has a live OpenCode directory. The live MCP path passes the resolved repo; the approval-click path passes undefined and falls back to the anchor's `repo` alias. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
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.
Summary
Testing
AI-generated — verify before acting. View Thor context