feat(tasks): route slack follow-ups through the workflow follow-up queue - #70806
Conversation
1135f96 to
e36c236
Compare
e36c236 to
07d817a
Compare
07d817a to
a1f105c
Compare
![hex-security-app[bot]](https://avatars.githubusercontent.com/in/2665233?s=60&v=4){kind=small}
| task_run.team_id, | ||
| content=user_text, | ||
| artifact_ids=_uploaded_attachment_ids(uploaded_attachments), | ||
| message_id=_slack_followup_message_id(channel, user_message_ts, thread_ts), |
There was a problem hiding this comment.
The queued signal carries content, artifacts, and an idempotency key, but not actor_user. The delivery activity instead reads the mutable slack_actor_user_id from run state to mint the sandbox JWT and refresh the MCP OAuth token. Since the next Slack follow-up updates that state before its queued predecessor is dispatched, a low-privilege teammate can send a prompt immediately before a privileged participant's follow-up; their queued prompt is then executed with the privileged participant's sandbox and OAuth credentials. The prior direct path minted the token from this invocation's actor_user, so it did not have this cross-message credential race.
Prompt To Fix With AI
Carry the authenticated Slack actor identity (at minimum the validated PostHog user ID, and preferably the Slack user ID for audit attribution) as part of signal_task_run_user_message -> signal_task_followup_message -> PendingFollowup -> SendFollowupToSandboxInput. At dispatch, resolve and validate that specific actor, and mint both the sandbox connection token and refreshed MCP OAuth credentials from it rather than from mutable TaskRun.state. Do not update shared actor state as the source of credentials for queued messages; add a regression test with two queued follow-ups from different users proving each delivery uses its own actor.Severity: high | Confidence: 96% | React with 👍 if useful or 👎 if not
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| signal_result = tasks_facade.signal_task_run_user_message( | ||
| task_run.id, | ||
| mapping.task_id, | ||
| task_run.team_id, | ||
| content=user_text, | ||
| artifact_ids=_uploaded_attachment_ids(uploaded_attachments), | ||
| message_id=_slack_followup_message_id(channel, user_message_ts, thread_ts), | ||
| ) |
There was a problem hiding this comment.
The signal carries the message but not its resolved actor_user, so delivery later derives credentials from the run-wide slack_actor_user_id. If two authorized Slack users reply quickly, the second forward can overwrite that state before the first message is delivered, causing the first user's message to run with the second user's sandbox credentials.
Rule Used: When implementing new features, ensure that owners... (source)
Learned From
PostHog/posthog#31236
| client = sync_connect() | ||
| handle = client.get_workflow_handle(workflow_id) | ||
| asyncio.run(handle.signal("send_followup_message", args=[message, artifact_ids])) | ||
| asyncio.run(handle.signal("send_followup_message", args=[message, artifact_ids, message_id])) |
There was a problem hiding this comment.
This always sends three positional arguments, while workers running the previous workflow code accept only message and artifact_ids. During a rolling deployment, a new caller can signal an in-flight workflow still assigned to an old worker, where the extra argument makes signal handling fail and the Slack follow-up is never queued.
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
| # Queue on the workflow so delivery is ordered with the web path and the | ||
| # turn runs under this message's actor. The deterministic message id | ||
| # keeps redelivery idempotent. | ||
| signal_result = tasks_facade.signal_task_run_user_message( |
There was a problem hiding this comment.
High: Follow-up identity is not bound to the queued message
This signal does not carry actor_user.id, while the delivery activity later mints credentials from the run's mutable slack_actor_user_id. If two authorized participants submit overlapping follow-ups, the later state update can make the earlier participant's command execute with the other participant's sandbox and MCP credentials. Include the actor identity in PendingFollowup and SendFollowupToSandboxInput, validate that actor still has team access, and mint the token from that immutable per-message identity.
PR overviewThis PR changes Slack task follow-ups so they are routed through the workflow follow-up queue rather than being handled directly. The touched task creation flow queues pending Slack follow-up messages for later delivery to the sandbox/workflow path. There is one open security issue, and none have been addressed yet. The remaining issue is significant: queued follow-up messages are not bound to the original Slack actor, so overlapping submissions by authorized participants could cause one user’s follow-up to run with another user’s sandbox and MCP credentials. The PR should bind the actor identity to each queued follow-up and validate access at delivery time before credentials are minted. Open issues (1)
Fixed/addressed: 0 · PR risk: 7/10 |
🤖 CI report
|
| File | Patch | Uncovered changed lines |
|---|---|---|
products/tasks/backend/facade/api.py |
42.9% | 2575–2578 |
products/tasks/backend/temporal/client.py |
50.0% | 476 |
products/tasks/backend/temporal/process_task/workflow.py |
66.7% | 1839 |
🤖 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 29725779251 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
platform_features |
██░░░░░░░░░░░░░░░░░░ 12.1% |
7 / 58 |
batch_exports |
████████░░░░░░░░░░░░ 39.6% |
8,414 / 21,225 |
demo |
███████████░░░░░░░░░ 56.2% |
1,497 / 2,663 |
warehouse_sources_queue |
████████████░░░░░░░░ 59.2% |
148 / 250 |
tasks |
█████████████░░░░░░░ 67.0% |
25,936 / 38,683 |
data_tools |
██████████████░░░░░░ 70.0% |
63 / 90 |
ai_gateway |
███████████████░░░░░ 75.0% |
9 / 12 |
signals |
████████████████░░░░ 79.4% |
19,503 / 24,561 |
data_modeling |
████████████████░░░░ 80.0% |
4,834 / 6,045 |
cdp |
████████████████░░░░ 80.7% |
3,118 / 3,864 |
wizard |
████████████████░░░░ 82.5% |
772 / 936 |
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 |
business_knowledge |
██████████████████░░ 88.5% |
4,400 / 4,969 |
conversations |
██████████████████░░ 89.0% |
16,183 / 18,186 |
mcp_analytics |
██████████████████░░ 89.2% |
2,514 / 2,819 |
dashboards |
██████████████████░░ 89.4% |
5,912 / 6,611 |
visual_review |
██████████████████░░ 89.4% |
5,818 / 6,505 |
engineering_analytics |
██████████████████░░ 89.5% |
5,507 / 6,154 |
alerts |
██████████████████░░ 89.9% |
4,054 / 4,508 |
early_access_features |
██████████████████░░ 90.1% |
1,031 / 1,144 |
error_tracking |
██████████████████░░ 90.1% |
9,885 / 10,966 |
streamlit_apps |
██████████████████░░ 90.4% |
2,501 / 2,767 |
slack_app |
██████████████████░░ 90.6% |
8,985 / 9,922 |
links |
██████████████████░░ 90.6% |
183 / 202 |
marketing_analytics |
██████████████████░░ 90.8% |
11,514 / 12,684 |
stamphog |
██████████████████░░ 91.0% |
3,993 / 4,387 |
product_analytics |
██████████████████░░ 91.4% |
5,781 / 6,328 |
mcp_store |
██████████████████░░ 91.8% |
3,685 / 4,012 |
data_warehouse |
██████████████████░░ 92.4% |
18,771 / 20,309 |
notifications |
███████████████████░ 92.7% |
1,031 / 1,112 |
ai_observability |
███████████████████░ 92.8% |
14,916 / 16,077 |
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 |
tracing |
███████████████████░ 93.4% |
2,546 / 2,725 |
managed_migrations |
███████████████████░ 93.8% |
1,220 / 1,300 |
legal_documents |
███████████████████░ 94.1% |
1,568 / 1,667 |
endpoints |
███████████████████░ 94.1% |
8,606 / 9,143 |
messaging |
███████████████████░ 94.2% |
2,647 / 2,810 |
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,776 / 14,383 |
growth |
███████████████████░ 95.8% |
2,837 / 2,960 |
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% |
287,939 / 298,535 |
access_control |
███████████████████░ 96.8% |
849 / 877 |
customer_analytics |
███████████████████░ 97.2% |
7,482 / 7,700 |
data_catalog |
███████████████████░ 97.4% |
2,303 / 2,365 |
analytics_platform |
████████████████████ 98.0% |
2,102 / 2,145 |
metrics |
████████████████████ 98.2% |
2,491 / 2,536 |
pulse |
████████████████████ 98.4% |
2,017 / 2,049 |
live_debugger |
████████████████████ 99.2% |
613 / 618 |
field_notes |
████████████████████ 99.4% |
158 / 159 |
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.
|
|
||
|
|
||
| def signal_task_followup_message(workflow_id: str, message: str | None, artifact_ids: list[str]) -> None: | ||
| def signal_task_followup_message( |
There was a problem hiding this comment.
here we need patching or in-roll workflows will fail, similarly in #70762
There was a problem hiding this comment.
yeah, you are right, interestingly my agent were talling me extra params will be dropped, but looks like TypeError will follow aka await handler(*input.args)
There was a problem hiding this comment.
if that agent was Fable, Anthropic needs to send us some money back 😆
There was a problem hiding this comment.
hahaha, opus 4.8, but still should refund us right? :D
There was a problem hiding this comment.
i have created new base PR0 which just adds defaults to signal handler, we should be good to go #71562
a1f105c to
f917c10
Compare
cvolzer3
left a comment
cvolzer3
left a comment
There was a problem hiding this comment.
Fable comments incoming
| # Queue on the workflow so delivery is ordered with the web path and the | ||
| # turn runs under this message's actor. The deterministic message id | ||
| # keeps redelivery idempotent. | ||
| signal_result = tasks_facade.signal_task_run_user_message( | ||
| task_run.id, | ||
| mapping.task_id, | ||
| task_run.team_id, | ||
| content=user_text, | ||
| artifact_ids=_uploaded_attachment_ids(uploaded_attachments), | ||
| message_id=_slack_followup_message_id(channel, user_message_ts, thread_ts), | ||
| ) |
There was a problem hiding this comment.
The message now travels in the ordered queue, but the actor still travels out-of-band through run state: this activity writes slack_actor_user_id (via update_task_run_state above) before signaling, and send_followup_to_sandbox re-reads it at delivery time via get_task_run_credential_user. Under the exact scenario this PR enables (two users queuing rapid messages), Alice's queued message can be delivered after Bob's forward has already overwritten the state, so her turn runs with Bob's connection token, Bob's distinct_id, and Bob's user-scoped MCP servers mounted by the credential refresh. The old path minted the token inline with the correct actor, so this is a new gap, and this comment's claim that "the turn runs under this message's actor" is no longer guaranteed.
Suggestion: carry actor_user_id in the signal payload (send_permission_response already does exactly this) and thread it through PendingFollowup into SendFollowupToSandboxInput, falling back to run state when absent. If that's deferred, this comment should at least be reworded so it doesn't document a guarantee the code no longer provides.
| @@ -1212,68 +1190,27 @@ def test_forwarding_failure_posts_error(self, mock_slack_cls, mock_send, mock_to | |||
| call_kwargs = mock_slack_instance.client.chat_postMessage.call_args.kwargs | |||
| assert "couldn't deliver" in call_kwargs["text"] | |||
There was a problem hiding this comment.
test_forwarding_failure_posts_error is stale now: both patches target functions the activity no longer calls, so the mocked failure return is inert. What actually happens is the real signal_task_run_user_message runs, sync_connect() attempts a live Temporal connection inside a unit test, the signal fails, and the activity posts the error reply, so the assertion passes by accident while doing real network I/O on every run. The scenario is properly covered by test_signal_failure_posts_error_reply below; this one can be deleted.
| artifact_ids=_uploaded_attachment_ids(uploaded_attachments), | ||
| message_id=_slack_followup_message_id(channel, user_message_ts, thread_ts), | ||
| ) | ||
| if signal_result is not True: |
There was a problem hiding this comment.
signal_task_run_user_message collapses every exception to False, so a transient Temporal connectivity blip now lands the user on an ❌ and "The sandbox may have stopped. Please try starting a new task." with no retry, where the old path retried retryable errors once. Since this activity is already designed to be safely re-run under Temporal retries (deterministic message_id, upsert-safe attachment upload), consider letting transient signal errors propagate so the activity retries, and reserving this terminal reply for definitive outcomes (run not found, workflow already completed).
| return True | ||
|
|
||
| # Message delivered; the agent is now working on it, so leave the :eyes: reaction | ||
| # Message queued; the agent picks it up next, so leave the :eyes: reaction |
There was a problem hiding this comment.
Behavior change worth stating in the PR description: once the message is queued, a delivery failure inside the workflow marks the whole run failed (_send_followup_to_sandbox's except path sets followup_delivery_failed), whereas the old direct path posted an inline error and left the run alive for an in-thread retry. And in that async failure path nothing swaps this 👀 reaction to ❌ on the user's message; the failure only surfaces through the generic run-failure Slack update. This is consistent with the web path so it's likely intentional, but it deserves an explicit sign-off.
f917c10 to
947292e
Compare
129a602 to
da75016
Compare
947292e to
844215b
Compare
Slack thread follow-ups delivered straight to the sandbox via the facade, bypassing the task workflow's follow-up queue: no ordering with the web path, and a busy agent needed a 504 special case. The forward now signals send_followup_message like the web UI, so the workflow delivers one message at a time and each delivery blocks until the turn completes. The deterministic slack message id travels with the signal, keeping redelivery idempotent across retries. Delivery failures surface through the run error path; the forward still replies inline when a message cannot be queued. The facade send_user_message wrapper loses its last caller and is removed.
844215b to
4febdf9
Compare
3 participants
Problem
Slack thread follow-ups were delivered straight to the sandbox, bypassing the task workflow's follow-up queue — so Slack and web messages had no ordering against each other, and a busy agent needed a 504 special case.
Changes
The Slack forward signals
send_followup_messagelike the web UI does. The workflow delivers one message at a time and each delivery blocks until the turn completes, so messages execute strictly in order. The deterministic Slackmessage_idrides in the signal, keeping redelivery idempotent. Delivery failures surface through the run's error path; the forward still replies inline when a message can't be queued. The facade's directsend_user_messagewrapper loses its last caller and is removed.How did you test this code?
All tested locally in a two-user Slack thread (rapid alternating commands execute strictly in order), plus the rewritten forwarding tests, delivery tests, and web API call-shape tests — ~360 green.
Automatic notifications
Docs update
Internal behavior only.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Claude Code (Claude Fable 5).