Skip to content

fix(tasks): stop slack agent-design replies from repeating the answer - #109894

Open
VojtechBartos wants to merge 7 commits into
masterfrom
posthog/slack-agent-design-final-text
Open

VojtechBartos wants to merge 7 commits into
masterfrom
posthog/slack-agent-design-final-text

Conversation

@VojtechBartos

@VojtechBartos VojtechBartos commented Oct 1, 2026 •

Copy link
Copy Markdown
Member

Problem

  • Slack app users sometimes get a reply that starts with a fragment of the answer, then the whole answer, then the rest of it.
  • It happens only on runs with the agent-design reply, and only when two signals race.
  • The relay builds the answer from streamed text deltas, which the event relay sends in batches.
  • The agent server also posts the whole answer at end_turn through relay_message, and under the flag that text went in as one more agent_text_delta.
  • When the HTTP path won the race, the whole answer landed between two batches of deltas.

Raised in https://posthog.slack.com/archives/C09G8Q32R6F/p1790854129609959

Changes

  • Users get the answer one time, even when the two paths race.
  • relay_message now sends a new agent_final_text signal with the text and the turn's trace id.
  • The relay keeps that text in its own slot and uses it as the answer at close. It never joins it to the deltas.
  • The deltas stay the fallback answer when the final text arrives after turn_completed or not at all.
  • A final text whose trace id differs from the turn's is ignored, because a late answer of an earlier turn must not replace the current one.
  • A final text that arrives before the turn has any activity is ignored. The agent server sends the final text after the turn ends, so a late one can reach the next turn's relay. Without this, a follow-up reply repeated the previous answer.
  • Trace ids are compared without hyphens. The relay endpoint sends a hyphenated UUID, and the turn-complete event sends the W3C form.
  • The plan block streams as before. The answer text never streamed live, so nothing visible changes in timing.

Before:

flowchart LR
  A{{Agent server}} -->|chunk deltas| R[Event relay, batched]
  A -->|end_turn: relay_message| F[Facade]
  R -->|agent_text_delta| N[(narrative)]
  F -->|agent_text_delta, whole answer| N
  N --> S[Slack reply]
  classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff;
  classDef phRed fill:#f54e00,stroke:#f54e00,color:#fff;
  classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000;
  classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000;
  class A phBlue; class R,F phRed; class N phGray; class S phYellow;
Loading

After:

flowchart LR
  A{{Agent server}} -->|chunk deltas| R[Event relay, batched]
  A -->|end_turn: relay_message| F[Facade]
  R -->|agent_text_delta| N[(narrative)]
  F -->|agent_final_text| T[(final text)]
  T -->|first choice| P[Pick at close]
  N -->|fallback| P
  P --> S[Slack reply]
  classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff;
  classDef phRed fill:#f54e00,stroke:#f54e00,color:#fff;
  classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000;
  classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000;
  class A phBlue; class R,F phRed; class N,T phGray; class P,S phYellow;
Loading

Note

The parent forwards agent_final_text only when the run recorded the tasks-slack-agent-final-text patch marker. A run that started on an older worker never forwards it, so a replay emits no command that its history lacks. Those runs use the deltas, as before.

How did you test this code?

  • Ran test_slack_agent_design_relay.py, test_slack_agent_design.py, test_replay.py, test_workflow.py and the relay_message tests in test_api.py locally.
  • Not run: an end-to-end Slack reply, repo-wide mypy, and a replay of a history where an old worker recorded agent_final_text. That history needs a full task run on the old code.

Test rationale: three cases extend test_last_prose_burst_is_the_answer. One sends the final text, with a hyphenated trace id, before the last delta, and fails if the relay joins it to the deltas or misreads the trace id. One sends a final text with another turn's trace id. One sends the previous turn's final text before the turn starts. Both fail if the relay uses that text.

Release status

  • No feature flag controls this change
  • This change is behind a feature flag and is not available to users
  • This change makes a previously flagged feature available to everyone

The fix applies only to runs with the Slack agent-design flag on.

Automatic notifications

  • Publish to changelog?

Docs update

None.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: PostHog Desktop (Claude Code), Claude Opus 5.5

  • Skills invoked: /writing-tests, /writing-code-comments, /writing-pr-descriptions.
  • The duplicate search found #109793. It touches the same relay files for the reply mention, but it does not fix this race. Expect a small rebase conflict in the relay test file.
  • No customer material is in the diff. The test text is invented.

Created with PostHog Desktop

🤖 Generated with Claude Code

@VojtechBartos VojtechBartos self-assigned this Oct 1, 2026
@trunk-io

trunk-io Bot commented Oct 1, 2026

Copy link
Copy Markdown

Merging to master in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

@github-actions

github-actions Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

🤖 CI report

⚠️ Trunk lane — backend Python lane

This PR is assigned to the backend Python lane. It runs backend Python tests and may merge in parallel with PRs in other lanes.

✅ Duplication (Python) — clean

New Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

✅ Duplication (TypeScript) — clean

New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

🚨 Comment density — 9% of added code lines are comments (21 of 232)

This section warns when comments are more than 3% of the code lines a PR adds, and alerts above 6%. Before agent-assisted PRs, the typical share was about 2%. Only full-line comments count. Docstrings, generated files, snapshots, migrations, and workflow files are left out.

Comments that restate the code, record how the change came about, or narrate the next line add noise for the next reader. Keep the comments that explain a reason the code cannot show, and remove the rest. See .agents/skills/writing-code-comments/SKILL.md for the house rules.

Files with the most added comment lines:

File Comment lines Added lines
products/tasks/backend/temporal/process_task/slack_agent_design_relay.py 10 49
products/tasks/backend/temporal/process_task/tests/test_slack_agent_design_relay.py 5 65
products/tasks/backend/temporal/process_task/workflow.py 4 27
products/slack_app/backend/slack_thread.py 1 32
products/tasks/backend/temporal/process_task/activities/slack_agent_design.py 1 8

This check does not block merging. It updates on every push and clears when the share drops.

⚠️ Backend coverage — 90.0% of changed backend lines covered — 12 uncovered

🧪 Backend test coverage

Patch coverage — changed backend lines (products + core): ██████████████████░░ 90.0% (114 / 126)

File Patch Uncovered changed lines
products/tasks/backend/facade/api.py 0.0% 5605
products/tasks/backend/temporal/client.py 50.0% 665
products/tasks/backend/temporal/process_task/activities/slack_agent_design.py 60.0% 141, 148
products/tasks/backend/temporal/process_task/workflow.py 66.7% 3585, 3596, 3602, 3608, 3610
products/slack_app/backend/slack_thread.py 88.0% 323, 518, 558

🤖 Agents: add a test only if an uncovered line exposes a realistic regression that existing tests miss. Otherwise explain why no new test is needed under "How did you test this code?". Gap list: the patch-coverage artifact on this run (gh run download 242556399827536 -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
demo ███████████░░░░░░░░░ 53.5% 1,447 / 2,707
data_tools ████████████░░░░░░░░ 61.2% 90 / 147
warehouse_sources_queue █████████████░░░░░░░ 65.9% 1,611 / 2,446
ai_gateway ███████████████░░░░░ 75.0% 9 / 12
aeo ███████████████░░░░░ 76.3% 617 / 809
batch_exports ████████████████░░░░ 81.3% 21,575 / 26,552
apm █████████████████░░░ 84.1% 1,306 / 1,553
cdp ██████████████████░░ 88.3% 4,559 / 5,164
ml_inference ██████████████████░░ 88.8% 539 / 607
mcp_analytics ██████████████████░░ 89.2% 5,038 / 5,651
product_tours ██████████████████░░ 89.3% 1,340 / 1,500
dashboards ██████████████████░░ 89.6% 6,924 / 7,727
notebooks ██████████████████░░ 90.2% 15,304 / 16,971
signals ██████████████████░░ 90.4% 59,702 / 66,064
cohorts ██████████████████░░ 90.5% 8,534 / 9,434
data_warehouse ██████████████████░░ 90.6% 14,375 / 15,860
streamlit_apps ██████████████████░░ 90.8% 2,684 / 2,956
managed_warehouse ██████████████████░░ 91.0% 10,252 / 11,263
data_modeling ██████████████████░░ 91.2% 10,562 / 11,584
today ██████████████████░░ 91.4% 896 / 980
exports ██████████████████░░ 91.7% 9,684 / 10,566
business_knowledge ██████████████████░░ 92.0% 8,447 / 9,181
tasks ██████████████████░░ 92.2% 79,870 / 86,670
engineering_analytics ██████████████████░░ 92.2% 11,497 / 12,475
ai_training ██████████████████░░ 92.2% 356 / 386
conversations ███████████████████░ 92.6% 29,137 / 31,467
early_access_features ███████████████████░ 92.6% 1,339 / 1,446
managed_migrations ███████████████████░ 92.7% 1,581 / 1,705
stamphog ███████████████████░ 92.8% 8,109 / 8,742
visual_review ███████████████████░ 92.9% 9,534 / 10,265
canvas ███████████████████░ 92.9% 7,155 / 7,703
approvals ███████████████████░ 93.0% 3,974 / 4,271
mcp_registry ███████████████████░ 93.1% 1,670 / 1,794
notifications ███████████████████░ 93.2% 1,144 / 1,228
error_tracking ███████████████████░ 93.2% 16,370 / 17,557
surveys ███████████████████░ 93.4% 6,644 / 7,113
autoresearch ███████████████████░ 93.6% 8,837 / 9,442
slack_app ███████████████████░ 93.7% 14,611 / 15,600
context_layer ███████████████████░ 93.8% 3,415 / 3,639
web_analytics ███████████████████░ 93.9% 23,680 / 25,229
billing_alerts ███████████████████░ 94.1% 2,094 / 2,226
mcp_store ███████████████████░ 94.3% 8,959 / 9,501
ai_observability ███████████████████░ 94.6% 24,974 / 26,409
wizard ███████████████████░ 94.7% 6,150 / 6,496
alerts ███████████████████░ 94.7% 9,321 / 9,845
workflows ███████████████████░ 94.7% 15,164 / 16,011
reminders ███████████████████░ 94.8% 760 / 802
review_hog ███████████████████░ 95.0% 11,750 / 12,362
annotations ███████████████████░ 95.1% 817 / 859
endpoints ███████████████████░ 95.1% 9,231 / 9,703
customer_analytics ███████████████████░ 95.2% 25,992 / 27,302
legal_documents ███████████████████░ 95.2% 2,311 / 2,427
marketing_analytics ███████████████████░ 95.3% 19,450 / 20,413
posthog_ai ███████████████████░ 95.3% 2,491 / 2,614
actions ███████████████████░ 95.5% 756 / 792
logs ███████████████████░ 95.5% 15,468 / 16,200
experiments ███████████████████░ 95.5% 32,953 / 34,503
data_catalog ███████████████████░ 95.6% 4,402 / 4,606
tracing ███████████████████░ 95.6% 3,536 / 3,699
replay_vision ███████████████████░ 95.6% 29,371 / 30,708
growth ███████████████████░ 95.7% 11,381 / 11,888
skills ███████████████████░ 95.8% 6,972 / 7,274
messaging ███████████████████░ 95.9% 3,824 / 3,989
product_analytics ███████████████████░ 96.0% 28,521 / 29,696
revenue_analytics ███████████████████░ 96.4% 1,889 / 1,959
user_interviews ███████████████████░ 96.5% 2,870 / 2,974
feature_flags ███████████████████░ 96.6% 26,913 / 27,856
access_control ███████████████████░ 96.7% 7,738 / 8,005
warehouse_sources ███████████████████░ 97.3% 468,811 / 481,716
data_quality ████████████████████ 97.5% 7,701 / 7,895
links ████████████████████ 97.9% 234 / 239
security ████████████████████ 98.0% 1,283 / 1,309
metrics ████████████████████ 98.0% 4,252 / 4,338
analytics_platform ████████████████████ 98.3% 2,784 / 2,833
pulse ████████████████████ 98.5% 2,046 / 2,078
live_debugger ████████████████████ 99.2% 626 / 631
field_notes ████████████████████ 99.4% 172 / 173

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.

@github-actions github-actions Bot added the feature/desktop Feature Tag: Desktop label Oct 1, 2026
@VojtechBartos
VojtechBartos requested a review from a team October 1, 2026 11:53
@VojtechBartos VojtechBartos added stamphog Request AI approval (no full review) reviewhog ($$$) Reviews pull requests before humans do labels Oct 1, 2026
@VojtechBartos
VojtechBartos marked this pull request as ready for review October 1, 2026 11:54
@posthog

posthog Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

🦔 PostHog Review reviewed this pull request

Found 0 must fix, 1 should fix, 0 consider.

Published 1 finding (view the review).

@pr-assigner-resolver-posthog
pr-assigner-resolver-posthog Bot requested review from a team October 1, 2026 11:54
stamphog[bot]

This comment was marked as outdated.

@stamphog
stamphog Bot dismissed their stale review October 1, 2026 11:57

A new stamphog review started for this PR — the fresh verdict replaces this approval.

stamphog[bot]

This comment was marked as outdated.

@trunk-io

trunk-io Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Static Badge   Static Badge   Static Badge

View Full Report ↗︎ ⋅ Docs

@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Important

Review skipped

We couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting @coderabbitai full review.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The facade now sends agent-design responses as final text with the turn’s trace ID. The Temporal client and process workflow route the new signal to the Slack relay. The relay stores nonblank final text and its optional trace ID, then prefers that text unless both trace IDs exist and differ. Tests cover answer selection when final text arrives between prose deltas and when it belongs to another turn.

Priority: ➖ Normal

Merge Risk: 🟡 Moderate · up to 4b5b0

A delayed answer from an earlier turn can cause Slack to post only a fragment of the current answer. Resolve or explicitly accept this ordering risk before merging, and extend the matching-answer test with a trailing delta.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 4b5b0

The change preserves existing access controls. Late replies can still displace the current answer, but the demonstrated impact is limited to an already authorized conversation.

Retained concerns

  • Low · reliability · inferred: Final-answer ownership is not preserved under cross-turn reordering. After any current-turn activity, every nonblank final payload overwrites the slot before identity validation. A late previous-turn payload with missing trace identity can become the current answer; a mismatched payload can overwrite an already valid current final and force narrative fallback. This affects answer integrity and alignment with the completed turn’s trace, within an already authorized task run. The supplied baseline appended whole-answer text instead, so stale delivery itself is not established as newly introduced; authoritative replacement is the changed behavior.
Security review details

Security Blast Radius

  • inferred — The demonstrated exposure is answer content in the mapped Slack thread of an authorized task run. The inspected path derives the workflow and destination from task ownership rather than accepting an independent workflow or Slack destination from the caller; it does not demonstrate cross-task or cross-tenant delivery.

Security Findings and Attack Paths

  • inferred — A caller already authorized to relay messages can supply nonblank text without a trace ID. After relay activity, that text can replace the selected answer and be closed with the current turn’s trace attribution. The previous path already accepted answer text from that caller, so this is an intra-run integrity concern, not demonstrated privilege escalation or a verified new security vulnerability.

Trust Boundaries and Controls

  • observed — The HTTP producer requires authentication and task:write scope, checks task control access and current run ownership, and passes task/team identity to the facade. The facade also rejects terminal runs, absent Slack mappings, and empty text. These controls bound the new answer authority.

Resilience and Maintainability Implications

  • observed — Existing answer tests cover populated trace mismatch and a no-trace previous answer arriving before activity. They do not establish preservation of a valid final answer when a stale payload arrives afterward, or rejection of a no-trace stale payload after current activity. Those are the ownership transitions relevant to the concern.

Hardening Proposals

  • proposed — Bind final-answer delivery to an immutable server-owned turn identity, preserve an accepted current-turn final against stale overwrites, and define an explicit fallback policy when identity is absent. This would make answer ownership independent of activity timing and caller-provided trace formatting.
🚥 Pre-merge checks | ✅ 1
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The description is complete and standalone. It explains the user impact, root cause, behavior change, compatibility gate, testing performed, known test gaps, release status, and agent context. Minor o…
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.

🟡 Other comments (1)
products/tasks/backend/temporal/process_task/slack_agent_design_relay.py-213-215 (1)

213-215: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Keep final text by trace ID until completion.

relay_task_run_message() sends final-text signals to the process workflow for the run. The process workflow forwards each signal to the current relay without checking its trace ID. A delayed signal from an earlier turn can overwrite the matching text in a later turn.

At completion, _final_answer() rejects the stale text and falls back to _narrative or _last_burst. _close_stream() then passes that fallback as final_markdown, so Slack can publish incomplete prose.

Store traced final text by trace ID until completion. Select the entry matching complete_turn(), and keep untraced text as a separate fallback. Add a regression test for matching text followed by stale text with incomplete deltas.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: PostHog/posthog/.coderabbit.yaml

Review profile: QUIET

Plan: Enterprise

Run ID: 7d7837f7-e87b-45c4-ac20-84a9005238e9

📥 Commits

Reviewing files that changed from the base of the PR and between 7a7ecdc and 729ff24.

📒 Files selected for processing (5)
  • products/tasks/backend/facade/api.py
  • products/tasks/backend/temporal/client.py
  • products/tasks/backend/temporal/process_task/slack_agent_design_relay.py
  • products/tasks/backend/temporal/process_task/tests/test_slack_agent_design_relay.py
  • products/tasks/backend/temporal/process_task/workflow.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

@posthog

posthog Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PostHog Review

Found 1 should fix.

Comment on lines +3551 to +3560
@temporalio.workflow.signal
async def agent_final_text(self, payload: dict[str, Any]) -> None:
if not self._is_agent_design_enabled or not self._current_slack_relay_workflow_id:
return
try:
handle = workflow.get_external_workflow_handle(self._current_slack_relay_workflow_id)
await handle.signal(SlackAgentDesignRelayWorkflow.agent_final_text, payload)
except Exception as e:
workflow.logger.debug(
"slack_final_text_forward_failed",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Version the forwarding of previously unknown signals

should_fix compatibility

Issue description

The API now emits agent_final_text while old workers can still serve the task queue. An old worker records this unknown signal without forwarding it. When a new worker replays that history with an active relay, this handler emits an external-signal command that the history lacks. A minimal replay test with temporalio 1.33.0 fails with TMPRL1100 for this signal sequence. This can stop the whole task workflow, not just its Slack reply.

Why we think it's a valid issue
  • Checked: The new handler at products/tasks/backend/temporal/process_task/workflow.py:3550-3553 and the helper _forward_to_slack_relay at workflow.py:3523-3534.
  • Found: When agent design is on and a relay is active, the helper calls workflow.get_external_workflow_handle(...).signal(...). This call emits a SignalExternalWorkflowExecution command. No workflow.patched(...) check controls it.
  • Checked: The sender in products/tasks/backend/facade/api.py:5561-5563 and products/tasks/backend/temporal/client.py:658-665.
  • Found: relay_message sends agent_final_text to every run whose state has AGENT_DESIGN_STATE_KEY. It does not check the worker version. The API pods and the worker pods roll out separately, so an old worker can receive this signal.
  • Checked: The installed SDK. pyproject.toml:145 pins temporalio==1.33.0. I read _apply_signal_workflow in .venv/.../temporalio/worker/_workflow_instance.py:1136-1144 and searched process_task/ for dynamic signal handlers.
  • Found: If a signal has no named handler and no dynamic handler, the SDK puts the job in _buffered_signals and emits no command. ProcessTaskWorkflow has no dynamic signal handler. So an old worker records agent_final_text in history and emits no command for it.
  • Found: On replay, the new code runs the handler at that same point. If _current_slack_relay_workflow_id is set at that point, the handler emits a command that the history does not have. The relay is set when the final text comes before turn_completed (workflow.py:3556-3560), and the PR says this is the usual race. The next command event in the history is often the complete_turn SignalExternal. Thus the replayed commands no longer match the recorded events.
  • Found: The PR NOTE says an old worker "buffers the unknown signal" and the deltas take over. That is correct for the old worker. But the NOTE does not cover the replay on a new worker after the old pod stops.
  • Found: The repo knows this failure class. workflow.py:356-363 records an earlier TMPRL1100 from a signal-handler change. .claude/rules/temporal-workflow-versioning.md requires a workflow.patched(...) gate for new commands on paths that in-flight histories already passed. process_task/tests/test_replay.py already gives a replay harness for this workflow.
  • Found: No code sets NondeterminismError as a workflow failure type for this worker. Only posthog/temporal/data_modeling/workflows/materialize_view.py:796 refers to it. So a nondeterminism error makes the workflow task retry again and again. Follow-ups, heartbeats, and completion handling stop for that task run.
  • Impact: A flagged Slack agent-design run can get stuck. This occurs when the run ends a turn on an old worker after the new API is live, and the run is still alive when a new worker replays it. The fix is small: capture a patch flag in the main path (as _PATCH_ID_SLACK_AGENT_DESIGN_STATUS does at workflow.py:1841) and forward only when the flag is true.
  • Priority: Lowered to should_fix. The trigger needs the agent-design flag, which the PR says is not available to users. The risk window is only the one-time overlap while this PR rolls out. The consequence is serious, but few runs can reach it.
Suggested fix

Capture a new workflow.patched(...) support marker in the main workflow path before the relay starts. Forward agent_final_text only when that captured value is true. Older histories must retain the delta fallback and emit no new forwarding command. Do not create the patch marker inside this signal handler. Add a replay test where an old worker already recorded an unknown agent_final_text signal.

Prompt to fix with AI (copy-paste)
## Context
@products/tasks/backend/temporal/process_task/workflow.py#L3551-3560

<issue_description>
The API now emits agent_final_text while old workers can still serve the task queue. An old worker records this unknown signal without forwarding it. When a new worker replays that history with an active relay, this handler emits an external-signal command that the history lacks. A minimal replay test with temporalio 1.33.0 fails with TMPRL1100 for this signal sequence. This can stop the whole task workflow, not just its Slack reply.
</issue_description>

<issue_validation>
- **Checked:** The new handler at `products/tasks/backend/temporal/process_task/workflow.py:3550-3553` and the helper `_forward_to_slack_relay` at `workflow.py:3523-3534`.
- **Found:** When agent design is on and a relay is active, the helper calls `workflow.get_external_workflow_handle(...).signal(...)`. This call emits a SignalExternalWorkflowExecution command. No `workflow.patched(...)` check controls it.
- **Checked:** The sender in `products/tasks/backend/facade/api.py:5561-5563` and `products/tasks/backend/temporal/client.py:658-665`.
- **Found:** `relay_message` sends `agent_final_text` to every run whose state has `AGENT_DESIGN_STATE_KEY`. It does not check the worker version. The API pods and the worker pods roll out separately, so an old worker can receive this signal.
- **Checked:** The installed SDK. `pyproject.toml:145` pins `temporalio==1.33.0`. I read `_apply_signal_workflow` in `.venv/.../temporalio/worker/_workflow_instance.py:1136-1144` and searched `process_task/` for dynamic signal handlers.
- **Found:** If a signal has no named handler and no dynamic handler, the SDK puts the job in `_buffered_signals` and emits no command. `ProcessTaskWorkflow` has no dynamic signal handler. So an old worker records `agent_final_text` in history and emits no command for it.
- **Found:** On replay, the new code runs the handler at that same point. If `_current_slack_relay_workflow_id` is set at that point, the handler emits a command that the history does not have. The relay is set when the final text comes before `turn_completed` (`workflow.py:3556-3560`), and the PR says this is the usual race. The next command event in the history is often the `complete_turn` SignalExternal. Thus the replayed commands no longer match the recorded events.
- **Found:** The PR NOTE says an old worker "buffers the unknown signal" and the deltas take over. That is correct for the old worker. But the NOTE does not cover the replay on a new worker after the old pod stops.
- **Found:** The repo knows this failure class. `workflow.py:356-363` records an earlier TMPRL1100 from a signal-handler change. `.claude/rules/temporal-workflow-versioning.md` requires a `workflow.patched(...)` gate for new commands on paths that in-flight histories already passed. `process_task/tests/test_replay.py` already gives a replay harness for this workflow.
- **Found:** No code sets `NondeterminismError` as a workflow failure type for this worker. Only `posthog/temporal/data_modeling/workflows/materialize_view.py:796` refers to it. So a nondeterminism error makes the workflow task retry again and again. Follow-ups, heartbeats, and completion handling stop for that task run.
- **Impact:** A flagged Slack agent-design run can get stuck. This occurs when the run ends a turn on an old worker after the new API is live, and the run is still alive when a new worker replays it. The fix is small: capture a patch flag in the main path (as `_PATCH_ID_SLACK_AGENT_DESIGN_STATUS` does at `workflow.py:1841`) and forward only when the flag is true.
- **Priority:** Lowered to should_fix. The trigger needs the agent-design flag, which the PR says is not available to users. The risk window is only the one-time overlap while this PR rolls out. The consequence is serious, but few runs can reach it.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Capture a new workflow.patched(...) support marker in the main workflow path before the relay starts. Forward agent_final_text only when that captured value is true. Older histories must retain the delta fallback and emit no new forwarding command. Do not create the patch marker inside this signal handler. Add a replay test where an old worker already recorded an unknown agent_final_text signal.
</potential_solution>

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

valid. Fixed in 8dc364c: the parent records a tasks-slack-agent-final-text patch marker in the main path, right before the event relay starts, and forwards agent_final_text only when that marker is present. A run that started on an older worker replays with the marker absent, so the handler emits no command and the relay uses the deltas. I did not add a replay fixture, because capturing a history with an unknown agent_final_text signal needs a full agent-design task run on the old code. The existing saved histories still replay.

@posthog posthog Bot removed the reviewhog ($$$) Reviews pull requests before humans do label Oct 1, 2026
@stamphog
stamphog Bot dismissed their stale review October 1, 2026 12:33

A new stamphog review started for this PR — the fresh verdict replaces this approval.

Copy link
Copy Markdown
Member Author

@coderabbitai re the minor finding on slack_agent_design_relay.py 213-215 (keep final text by trace id): not changed. The failure needs the final text of an earlier turn to arrive after the final text of the current turn. A new turn starts only after a new Slack message, and the agent server sends the final text right at end_turn, so that order is not realistic. If it did occur, the relay falls back to the streamed deltas. That text is the full answer in normal runs, not a duplicate. A per-trace-id map would add state for a case that does not occur.

stamphog[bot]

This comment was marked as outdated.

@coderabbitai

coderabbitai Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail
fd '^slack_agent_design_relay\.py$'
ast-grep outline products/tasks/backend/temporal/process_task/slack_agent_design_relay.py
sed -n '1,280p' products/tasks/backend/temporal/process_task/slack_agent_design_relay.py

Length of output: 14512


🏁 Script executed:

#!/bin/bash
set -euo pipefail
sed -n '280,450p' products/tasks/backend/temporal/process_task/slack_agent_design_relay.py

Length of output: 8401


@VojtechBartos I withdraw the minor finding. _final_answer() selects either final text or streamed prose; it does not concatenate them. A per-trace-ID map is not needed under the stated turn-ordering contract. No change is required.

Thanks for the clarification.


✏️ Learnings added
Learnt from: VojtechBartos
URL: https://github.com/PostHog/posthog/pull/109894

Timestamp: 2026-10-01T12:51:41.271Z
Learning: In PostHog's Slack agent-design flow, a new turn starts only after a new Slack message, and the agent server sends final text at end_turn. In products/tasks/backend/temporal/process_task/slack_agent_design_relay.py, SlackAgentDesignRelayWorkflow._final_answer rejects final text when both trace IDs exist and differ, then falls back to streamed prose. Streamed deltas contain the full answer in normal runs. Do not require a per-trace-ID final-text map solely for the hypothetical arrival of an earlier turn's final text after the current turn's final text under this ordering contract.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

You are interacting with an AI system.

@stamphog
stamphog Bot dismissed their stale review October 1, 2026 12:56

A new stamphog review started for this PR — the fresh verdict replaces this approval.

stamphog[bot]

This comment was marked as outdated.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
products/tasks/backend/temporal/process_task/tests/test_slack_agent_design_relay.py (1)

155-155: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Send the trailing delta after agent_final_text.

The matching-final-text case ends before a trailing delta. Add the buffered delta to detect duplication while keeping the expected answer unchanged.

Proposed change
                     ("agent_final_text", {"text": "The answer is 42.", "trace_id": str(uuid.UUID(TRACE_ID))}),
+                    ("agent_text_delta", "42."),

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: PostHog/posthog/.coderabbit.yaml

Review profile: QUIET

Plan: Enterprise

Run ID: bd2eb6ba-8e13-4c88-a531-821257fcbb47

📥 Commits

Reviewing files that changed from the base of the PR and between 8dc364c and 4b5b0d7.

📒 Files selected for processing (2)
  • products/tasks/backend/temporal/process_task/slack_agent_design_relay.py
  • products/tasks/backend/temporal/process_task/tests/test_slack_agent_design_relay.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 8 remain after this review.

…stream

Slack answers message_not_in_streaming_state when it has closed a stream. The handler now marks the stream ended, skips later appends and the stop call, and posts the final answer as a thread reply. The relay stops dispatching appends and opens a new message for the answer, gated with workflow.patched so recorded histories replay unchanged.

Generated-By: PostHog Desktop
Task-Id: a418769d-0f5e-4910-a08d-80124b52d572
…ches

Generated-By: PostHog Desktop
Task-Id: a418769d-0f5e-4910-a08d-80124b52d572
posthog Bot and others added 5 commits October 1, 2026 18:43
…ream

The closed-stream fallback posted the answer but skipped the chart cards. The stop activity now delivers the attachments that are still pending as their own thread message, after the file attach step, so no file uploads twice.

Generated-By: PostHog Desktop
Task-Id: a418769d-0f5e-4910-a08d-80124b52d572
The agent server posts the whole answer through relay_message, and under the
agent-design flag that text was added to the same buffer as the streamed text
deltas. When it arrived between two deltas, the reply showed a prefix, the
whole answer, then the rest of the answer.

The relay now keeps the agent server's text in its own slot and uses it as the
answer at close, falling back to the deltas when it does not arrive in time.
A trace id from another turn makes the relay ignore the text.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Generated-By: PostHog Desktop
Task-Id: 8d3f7087-abe2-49b2-9f6b-33b44e75dd95
The parent workflow forwarded agent_status_update, agent_text_delta and
agent_final_text to the relay child with three copies of the same guard and
try/except. They now call one helper. The relay also strips the final text
once, when it arrives.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Generated-By: PostHog Desktop
Task-Id: 8d3f7087-abe2-49b2-9f6b-33b44e75dd95
A worker without the agent_final_text handler records the signal and sends
nothing. A newer worker that replays that history and forwards the signal
emits a command the history does not have, which fails the replay with
TMPRL1100. The parent now forwards the signal only when the run recorded the
tasks-slack-agent-final-text patch marker, so in-flight runs keep the delta
fallback.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Generated-By: PostHog Desktop
Task-Id: 8d3f7087-abe2-49b2-9f6b-33b44e75dd95
The agent server sends a turn's final text after the turn ends, so the text
can reach the relay of the next turn. Without a trace id the relay could not
tell it was stale, and the next reply repeated the previous answer. The relay
now ignores a final text that arrives before its own turn has any activity.

The relay endpoint also sends the trace id as a hyphenated UUID, while the
turn-complete event carries the W3C form. The relay now compares them without
hyphens, so a matching final text is no longer taken for a stale one.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Generated-By: PostHog Desktop
Task-Id: 8d3f7087-abe2-49b2-9f6b-33b44e75dd95
@VojtechBartos
VojtechBartos force-pushed the posthog/slack-agent-design-final-text branch from 4b5b0d7 to 046bc93 Compare October 2, 2026 07:28
@stamphog
stamphog Bot dismissed their stale review October 2, 2026 07:28

A new stamphog review started for this PR — the fresh verdict replaces this approval.

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved.

Contained Slack agent-design reply fix behind a feature flag. The Temporal changes are version-gated by patch markers, reviewer concerns were addressed or withdrawn, and the author has STRONG familiarity with the code. The diff also adds Slack closed-stream fallback handling that the description doesn't mention. I checked it: it is coherent, tested, and outside risky territory.

  • Author wrote 96% of the modified lines and has 65 merged PRs in these paths (familiarity STRONG).
  • The description omits a bundled behavior: when Slack closes a reply stream early, the answer is now posted as a plain thread reply and pending file artifacts are delivered separately. It has tests, but the author should describe it in the PR.
Gate mechanics and policy version
Gate Result
prerequisites ✓ all clear
deny-list ✓ no deny categories matched
size ✓ 165L, 6F substantive, 298L/9F incl. docs/generated/snapshots — within ceiling
tier ✓ T1-agent / T1c-medium (298L, 9F, single-area, fix)
stamphog 2.3.1 .stamphog/policy.yml @ 046bc93 · reviewed head 046bc93

@parameterai

parameterai Bot commented Oct 2, 2026

Copy link
Copy Markdown

Risk: No findings

This increment adds a "contributors per channel" listing and pull-request-title lookup (both correctly scoped through channel/task visibility filters), hardens the reply-mention attribution so a reply tags the turn's actual sender, records Slack-closed streams so answers post as plain thread replies, defers sandbox TTL rotation while the agent is active, and protects agent_instructions from public exposure and PATCH. All new data paths are filtered by Channel.visible_to_q/task_visibility_q with team_id, and the message-actor lookup only resolves ids the server recorded itself. No new security vulnerabilities found.

Sentinel reviewed 046bc93 · Review settings

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature/desktop Feature Tag: Desktop stamphog Request AI approval (no full review)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant