fix(tasks): rebind sandbox MCP session on actor transitions - #70454
Conversation
|
🤖 CI report✅ Bundle size — no changeUncompressed size of every built Total: 64.72 MiB · no change No file changed by more than 1000 B. Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report ✅ Eager graph — within budgetHow much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy
🟢 Largest files eagerly shipped from
|
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 762 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 280.3 KiB | ../node_modules/.pnpm/posthog-js@1.400.0/node_modules/posthog-js/dist/rrweb.js |
| 267.7 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 234.9 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 222.8 KiB | ../node_modules/.pnpm/posthog-js@1.400.0/node_modules/posthog-js/dist/module.js |
| 164.0 KiB | src/queries/validators.js |
| 154.3 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 105.9 KiB | src/lib/api.ts |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Dist folder size — no change
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1284.95 MiB · no change
ℹ️ MCP UI apps size — 31 app(s), 16561.5 KB JS
Built size of each MCP UI app (main.js + styles.css).
| App | JS | CSS |
|---|---|---|
| debug | 598.2 KB | 179.2 KB |
| action | 456.5 KB | 179.2 KB |
| action-list | 563.0 KB | 179.2 KB |
| cohort | 455.4 KB | 179.2 KB |
| cohort-list | 562.0 KB | 179.2 KB |
| email-template | 455.3 KB | 179.2 KB |
| error-details | 471.1 KB | 179.2 KB |
| error-issue | 456.1 KB | 179.2 KB |
| error-issue-list | 562.9 KB | 179.2 KB |
| experiment | 560.1 KB | 179.2 KB |
| experiment-list | 563.8 KB | 179.2 KB |
| experiment-results | 561.8 KB | 179.2 KB |
| feature-flag | 565.8 KB | 179.2 KB |
| feature-flag-list | 569.5 KB | 179.2 KB |
| feature-flag-testing | 459.4 KB | 179.2 KB |
| insight-actors | 560.8 KB | 179.2 KB |
| invite-email-preview | 454.7 KB | 179.2 KB |
| llm-costs | 558.1 KB | 179.2 KB |
| session-recording | 457.2 KB | 179.2 KB |
| session-summary | 462.5 KB | 179.2 KB |
| survey | 457.0 KB | 179.2 KB |
| survey-global-stats | 560.9 KB | 179.2 KB |
| survey-list | 563.7 KB | 179.2 KB |
| survey-stats | 560.8 KB | 179.2 KB |
| trace-span | 455.8 KB | 179.2 KB |
| trace-span-list | 562.9 KB | 179.2 KB |
| workflow | 455.8 KB | 179.2 KB |
| workflow-list | 562.4 KB | 179.2 KB |
| query-results | 743.7 KB | 179.2 KB |
| render-ui | 824.0 KB | 179.2 KB |
| visual-review-snapshots | 460.3 KB | 179.2 KB |
⚠️ Backend coverage — 4.0% of changed backend lines covered — 119 uncovered
🧪 Backend test coverage
Patch coverage — changed backend lines (products + core): █░░░░░░░░░░░░░░░░░░░ 4.0% (5 / 124)
| File | Patch | Uncovered changed lines |
|---|---|---|
products/tasks/backend/temporal/process_task/activities/send_followup_to_sandbox.py |
0.0% | 166–168, 279, 281–288, 299, 324, 329, 332, 337, 339, 350, 352, 368, 370, 378 |
products/tasks/backend/temporal/process_task/tests/test_send_followup_to_sandbox.py |
0.0% | 27, 30, 32, 69–73, 76–80, 84–85, 139, 150, 152, 155, 163, 179, 186, 188, 199, 208, 214–218, 220–221, 223–224, 226, 233–236, 238–239, 241, 244–247, 250, 255, 258, 260, 265, 268, 270, 273, 275–276, 278–279, 282, 284, 286–287, 289, 292, 294, 296–297, 299, 302, 305, 307, 309–311, 313, 320, 322, 324–325, 327–329, 331, 338, 341–342, 344–346, 394 |
products/tasks/backend/temporal/process_task/activities/start_agent_server.py |
33.3% | 349–350 |
products/tasks/backend/temporal/process_task/utils.py |
50.0% | 371, 375, 384, 390 |
🤖 Agents: add a test covering the lines above, or note why under "How did you test this code?". Machine-readable gap list: the patch-coverage artifact on this run (gh run download 29739868576 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
demo |
███████████░░░░░░░░░ 56.2% |
1,497 / 2,663 |
tasks |
█████████████░░░░░░░ 67.0% |
25,953 / 38,752 |
signals |
████████████████░░░░ 79.4% |
19,501 / 24,561 |
data_modeling |
████████████████░░░░ 80.0% |
4,834 / 6,045 |
cdp |
████████████████░░░░ 80.7% |
3,118 / 3,864 |
notebooks |
█████████████████░░░ 85.1% |
7,096 / 8,338 |
cohorts |
█████████████████░░░ 86.2% |
4,065 / 4,717 |
agent_platform |
█████████████████░░░ 86.4% |
3,807 / 4,405 |
actions |
█████████████████░░░ 86.6% |
717 / 828 |
product_tours |
█████████████████░░░ 87.5% |
1,266 / 1,447 |
exports |
██████████████████░░ 88.4% |
6,943 / 7,853 |
conversations |
██████████████████░░ 89.1% |
16,361 / 18,360 |
mcp_analytics |
██████████████████░░ 89.2% |
2,514 / 2,819 |
dashboards |
██████████████████░░ 89.4% |
5,917 / 6,616 |
engineering_analytics |
██████████████████░░ 89.5% |
5,507 / 6,154 |
alerts |
██████████████████░░ 89.9% |
4,054 / 4,508 |
error_tracking |
██████████████████░░ 90.1% |
9,757 / 10,828 |
early_access_features |
██████████████████░░ 90.1% |
1,031 / 1,144 |
streamlit_apps |
██████████████████░░ 90.4% |
2,501 / 2,767 |
slack_app |
██████████████████░░ 90.6% |
8,989 / 9,926 |
marketing_analytics |
██████████████████░░ 90.8% |
11,514 / 12,684 |
stamphog |
██████████████████░░ 91.0% |
3,993 / 4,387 |
product_analytics |
██████████████████░░ 91.4% |
5,781 / 6,328 |
data_warehouse |
██████████████████░░ 92.5% |
18,819 / 20,348 |
ai_observability |
███████████████████░ 92.7% |
14,921 / 16,093 |
workflows |
███████████████████░ 92.8% |
5,520 / 5,949 |
web_analytics |
███████████████████░ 92.9% |
13,853 / 14,913 |
surveys |
███████████████████░ 93.0% |
5,724 / 6,157 |
posthog_ai |
███████████████████░ 93.2% |
1,325 / 1,421 |
approvals |
███████████████████░ 93.3% |
3,395 / 3,640 |
reminders |
███████████████████░ 93.4% |
468 / 501 |
endpoints |
███████████████████░ 94.1% |
8,606 / 9,143 |
revenue_analytics |
███████████████████░ 94.5% |
3,598 / 3,809 |
skills |
███████████████████░ 94.5% |
2,881 / 3,049 |
review_hog |
███████████████████░ 94.6% |
6,806 / 7,191 |
logs |
███████████████████░ 95.3% |
9,844 / 10,325 |
experiments |
███████████████████░ 95.7% |
24,417 / 25,527 |
replay_vision |
███████████████████░ 95.8% |
13,850 / 14,459 |
annotations |
███████████████████░ 96.2% |
732 / 761 |
feature_flags |
███████████████████░ 96.3% |
16,233 / 16,859 |
user_interviews |
███████████████████░ 96.4% |
2,242 / 2,325 |
warehouse_sources |
███████████████████░ 96.5% |
289,025 / 299,653 |
customer_analytics |
███████████████████░ 97.2% |
7,482 / 7,700 |
data_catalog |
███████████████████░ 97.4% |
2,303 / 2,365 |
pulse |
████████████████████ 98.4% |
2,017 / 2,049 |
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
4969cdc to
8da27f6
Compare
ab8d961 to
5df4389
Compare
7e00cc6 to
8d0d872
Compare
e307e5b to
6558def
Compare
8d0d872 to
f2b815c
Compare
540c261 to
1bd42a4
Compare
|
Non-blocking — one finding from reviewing the transition-with-no-configs path. The comment at Consequence: on an actor transition where the new actor resolves zero configs, the empty-list send is a silent no-op that reports success, so Why non-blocking: the branch is unreachable on Cloud — Suggested follow-up:
|
f7baa33 to
a8a59c9
Compare
8bbd3c9 to
5b41a75
Compare
a8a59c9 to
d9caad1
Compare
230c58f to
4a56c2a
Compare
d9caad1 to
0c1ff7e
Compare
8b6e210 to
92badb0
Compare
0c1ff7e to
632c21f
Compare
92badb0 to
feec41d
Compare
| # without a resolvable MCP URL (get_sandbox_ph_mcp_configs is | ||
| # otherwise never empty), so best-effort delivery is acceptable. | ||
| logger.info("refresh_mcp_no_configs_on_transition", run_id=run_id, previous_user_id=bound_user_id) | ||
| return True |
There was a problem hiding this comment.
Fail closed when a transition has no MCP configs
This path leaves the live session bound to previous_user_id but reports it safe, so _deliver_followup sends the new participant's prompt anyway. This is reachable when the PostHog MCP URL is unavailable and the new actor has no installations while the prior actor has a personal MCP installation: the agent can invoke that retained server with the prior actor's bearer token. Retaining the old cache binding prevents a later skip, but does not protect the current turn.
Prompt To Fix With AI
On an actor transition with an empty resolved MCP-config list, return False so _deliver_followup does not send a prompt against the retained prior-actor session. Preserve the previous binding and add a regression test that verifies the follow-up is rejected rather than delivered in this case.Severity: medium | Confidence: 94% | React with 👍 if useful or 👎 if not
feec41d to
54a3240
Compare
The follow-up MCP refresh skipped whenever any token had been issued
for the run within the freshness window, so when a different user spoke
next (multiplayer Slack threads), the live session kept the previous
actor's OAuth token for up to the rest of the window.
One cache entry per sandbox (tasks:sandbox-mcp-session:{scope}) records
which user the live session was last bound to and expires with the
freshness window: same-actor repeats skip, while a speaker change, an
expired or unknown entry, and a replacement sandbox (fresh scope) all
refresh. The gate receives the delivery's already-resolved, payload-
pinned actor, so it follows the message's sender; runs with no
credential user skip the doomed mint instead of warning per message.
Boot records the initial session binding under the sandbox id.
…fig transition An empty mcpServers list is a notification-only no-op on the agent-server (it returns without rebuilding the session), so sending it on an actor transition that resolves no configs neither clears the previous actor's session nor rebinds — yet it reported success and falsely marked the new actor. Stop sending it and leave the binding on the previous actor, who still holds the live session. Only reachable on deployments without a resolvable MCP URL.
The session marker self-expires at half the OAuth token lifetime, so an absent marker can mean the previous actor's session is still live, not that the sandbox is fresh. Treating that as a best-effort first bind delivered a new actor's follow-up under the previous actor's credentials when the mint or refresh failed in the marker-expired window. Fail closed whenever a rebind can't be confirmed, not only on a known transition.
54a3240 to
02268db
Compare
| def test_skips_send_when_no_mcp_configs_resolved( | ||
| self, mock_oauth, mock_ph_configs, mock_user_configs, mock_send_refresh | ||
| self, mock_oauth, mock_ph_configs, mock_user_configs, mock_send_refresh, _sleep | ||
| ): | ||
| mock_oauth.return_value = "fresh-token" | ||
| _arm_success(mock_oauth, mock_ph_configs, mock_user_configs, mock_send_refresh) | ||
| mock_ph_configs.return_value = [] | ||
| mock_user_configs.return_value = [] | ||
|
|
||
| _refresh(_make_task_run_mock()) | ||
|
|
||
| mock_send_refresh.assert_not_called() | ||
| # Marked anyway: with no session to rebind, don't re-mint per message. | ||
| assert get_sandbox_mcp_session_user("run-1") == 42 | ||
|
|
There was a problem hiding this comment.
Test assertion doesn't match production code behavior. When mcp_configs is empty, the production code at line 324-334 of send_followup_to_sandbox.py returns False without calling mark_sandbox_mcp_session(). Therefore, the cache will remain empty and the assertion assert get_sandbox_mcp_session_user("run-1") == 42 will fail because it expects the value to be 42 but will get None.
# The assertion should be:
assert get_sandbox_mcp_session_user("run-1") is NoneOr if the intent is to mark even with no configs, the production code needs to call mark_sandbox_mcp_session(scope, actor_user.id) before returning at line 334.
| def test_skips_send_when_no_mcp_configs_resolved( | |
| self, mock_oauth, mock_ph_configs, mock_user_configs, mock_send_refresh | |
| self, mock_oauth, mock_ph_configs, mock_user_configs, mock_send_refresh, _sleep | |
| ): | |
| mock_oauth.return_value = "fresh-token" | |
| _arm_success(mock_oauth, mock_ph_configs, mock_user_configs, mock_send_refresh) | |
| mock_ph_configs.return_value = [] | |
| mock_user_configs.return_value = [] | |
| _refresh(_make_task_run_mock()) | |
| mock_send_refresh.assert_not_called() | |
| # Marked anyway: with no session to rebind, don't re-mint per message. | |
| assert get_sandbox_mcp_session_user("run-1") == 42 | |
| def test_skips_send_when_no_mcp_configs_resolved( | |
| self, mock_oauth, mock_ph_configs, mock_user_configs, mock_send_refresh, _sleep | |
| ): | |
| _arm_success(mock_oauth, mock_ph_configs, mock_user_configs, mock_send_refresh) | |
| mock_ph_configs.return_value = [] | |
| _refresh(_make_task_run_mock()) | |
| mock_send_refresh.assert_not_called() | |
| # Not marked: with no configs resolved, mark_sandbox_mcp_session is not called. | |
| assert get_sandbox_mcp_session_user("run-1") is None | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
On an actor transition where the new actor resolves no MCP configs, an empty-list refresh_session is a no-op on the agent-server: it can neither rebind the sandbox MCP session to the new actor nor tear down the previous actor's. Returning True let _deliver_followup run the new actor's turn against a session that may still hold the prior actor's credentials, so fail closed. A first/unknown binding with no configs has no recorded prior actor and nothing to leak, so it still runs the turn — the agent is not blocked just because MCP is unavailable (only self-hosted deployments without a resolvable MCP URL reach an empty config list at all).
454d497 to
0294158
Compare
| # there is nothing to leak, so let the turn run rather than block the | ||
| # agent just because MCP is unavailable. Record the binding so a later | ||
| # actor transition is still detected. | ||
| mark_sandbox_mcp_session(scope, actor_user.id) |
There was a problem hiding this comment.
Fail closed for unknown bindings with no configs
None is not evidence that this sandbox has no live MCP session: this marker expires after three hours while its OAuth token is valid for six. After actor A's marker expires, actor B on a deployment without a resolvable PostHog MCP URL can resolve no configs, take this branch, be marked as the session owner without a refresh_session, and have their prompt delivered using A's retained personal MCP bearer token. Subsequent B turns then skip rebinding for another cache window.
Prompt To Fix With AI
Treat an empty resolved MCP-config list as unsafe when the session binding is unknown as well as when it belongs to another actor. Do not update the session-binding cache and return False so `_deliver_followup` rejects the turn, unless durable sandbox state proves no MCP session was ever initialized. Add a regression test for an expired/evicted marker with a retained prior-actor session and no configs for the new actor.Severity: medium | Confidence: 98% | React with 👍 if useful or 👎 if not
_refresh_sandbox_mcp is now a bool-returning gate: _deliver_followup rejects the turn when it returns falsy. The refresh-ordering test stubbed it with a side effect that returned None, which trips the gate. Return True so the stub honors the contract and the test exercises ordering, not the reject path.

Problem
Even with every message carrying its sender (#70806, #70762), the sandbox's live MCP session doesn't follow speaker changes. Token refresh is gated by a per-run time window, so when user B speaks inside user A's window, B's turn runs on A's token and PostHog writes are attributed to (and authorized as) the wrong person. The window mark is also keyed by run id, so a replacement sandbox (retry, snapshot restore) inherits a stale mark.
Changes
One cache entry per sandbox —
tasks:sandbox-mcp-session:{scope}— tracks which user the live session is bound to:refresh_session, rebind. Unknown fails safe by refreshing.The gate reads the pinned actor from #70762's delivery, which the workflow serializes — so check-then-bind has a single executor.
Showcase
How did you test this code?
Tested manually: a two-user Slack thread where alternating speakers trigger a session refresh and same-speaker repeats skip it, with flag changes attributed to the correct user in PostHog. Automated gate tests cover same-actor skip, transition bypass, switch-back refresh, unknown-binding refresh, replacement-sandbox rebind, and binding only on successful delivery.
Automatic notifications
Docs update
Internal behavior only.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Claude Code (Claude Fable 5).