Skip to content

feat(tasks): surface comment mentions in the activity feed - #75910

Closed
puemos wants to merge 2 commits into
masterfrom
posthog-code/task-comment-mentions
Closed

feat(tasks): surface comment mentions in the activity feed#75910
puemos wants to merge 2 commits into
masterfrom
posthog-code/task-comment-mentions

Conversation

@puemos

@puemos puemos commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Problem

PostHog Code writes comments on a task's artifacts and canvases to the shared comments table, but the desktop app reads its own task activity feed, not the notifications inbox. So an @mention on one of those comments fanned out to email and the web inbox and appeared nowhere in the app the comment was written in — the one surface where the comment is actually readable.

Changes

Mentions reach the Code activity feed. record_comment_mention_activity projects mentions on task-scoped comments into the mentioned users' TaskActivity rows. Resolving a comment to its task needs help: an artifact id lives in a run's JSON rather than a table we can join against, so the client names the task in item_context (PostHog/code#3970) and the backend checks it against the team before writing. Visibility is deliberately left to the read path, which already re-checks it — that's what keeps a row honest when a task's visibility changes after the fact.

TaskActivity gains a nullable comment reference so these rows carry an author and a preview the way thread-message rows do. It's unindexed on purpose: the table is upserted on every thread message, and nothing reads it by comment. Migration 0077 is a metadata-only ADD COLUMN NULL.

Forwarded content is fenced. The existing "send a thread message to the agent" path concatenated the message behind a [Thread comment from X] prefix, which the message text can trivially escape. It's now wrapped in a delimited block carrying the author, matching how channel context and custom instructions are already framed, with the closing delimiter stripped from the body so the content can't close its block early and have the rest read as instructions.

What changed since the first draft

This started out roughly twice the size. It also carried a server-side endpoint to forward a comment into the agent's run — a new TaskCommentForward table, a migration, a facade function, an endpoint, and two serializers. That's dropped: the security rationale for doing it server-side doesn't hold (the task author can already put arbitrary text in front of their own agent via the thread, so forwarding a comment by id grants no new capability), so it belongs on the client, reusing the thread_messages + send_to_agent endpoints that already exist. The framing hardening stayed because it protects that existing forward path for everyone. Net result: ~660 → ~350 lines, two migrations → one.

How did you test this code?

What I actually ran:

  • products/tasks/backend/tests/test_forwarded_content.py — 8 pass. Covers the delimiter-stripping (four escape shapes, including spaced and uppercased closing tags) and author-attribute escaping.
  • ruff check / ruff format --check clean, manage.py check clean, manage.py makemigrations --check reports no pending changes.
  • manage.py sqlmigrate on 0077: metadata-only ADD COLUMN NULL (the auto FK index is dropped deliberately — nothing reads the table by comment).

What I could not run: test_comment_mention_activity.py. This sandbox has Postgres and Redis but the harness also needs a ClickHouse with the posthog_migrations cluster configured, which needs the real dev stack. The tests error in fixture setup, not on assertions — I have not seen them pass, and CI is the first place they'll actually run. Worth a reviewer's eye on that file specifically.

No manual testing — no running app here.

Those tests cover regressions nothing else did: a mention on an artifact comment reaching the feed at all; the feed rendering a comment's author/snippet (previously hardcoded to the thread message); and a forged or malformed taskId in client-supplied item_context, or a comment from another product, writing nothing.

Not in this PR

  • The web notification for these scopes is still unclickable (source_type=None, no source_url) and the email still links to the site root. Fixing that means registering the scopes plus a new SourceType, and SOURCE_TYPE_TO_PATH is a deliberate full Record on the frontend, so it's a wider cross-surface change — deferred on purpose.
  • Forwarding a comment to the agent moves to the client (compose a thread message from the comment, call the existing send_to_agent). Small follow-up, not here.

Separately, and worth someone's judgement: mention fan-out filters recipients to org members but never checks task visibility, and create_notification skips access-control filtering for resource_type=comment. So mentioning an org member who can't see a task still sends them the comment body by email and notification. That predates this PR and isn't changed here. The new activity rows aren't affected — the feed re-checks visibility on read.

Automatic notifications

  • Publish to changelog?
  • Alert Sales and Marketing teams?

Docs update

No user-facing docs affected.

🤖 Agent context

Human-driven (agent-assisted)

Asked to look at how Code activity notifications work for mentions, then asked to slim the first draft down. The forwarding half was cut on that pass — the by-id server-side design was over-built, and the reviewer's instinct that the PR was too big for the job was right.

…e author forward them

PostHog Code writes comments on a task's artifacts and canvases to the shared comments
table, but the desktop app reads its own activity feed rather than the notifications
inbox. A mention on one of those comments therefore fanned out to email and the web
inbox and appeared nowhere in the app the comment was written in.

Mentions on those comments now project into the mentioned users' task activity feeds, so
they land where the comment is readable. Resolving a comment to its task needs help:
artifact and canvas ids live in a run's JSON rather than a table, so the client names the
task in item_context, and the backend checks it against the team before writing anything.
Visibility is left to the read path, which already re-checks it, so a row stays honest
when a task's visibility changes later. TaskActivity gains a nullable comment reference so
these rows carry an author and a preview like thread-message rows do.

The task's author can also now send one of those comments into the task's live run, the
same shape as forwarding a thread message: author-only, live run required, and at most
once per comment. Forwarding names a stored comment instead of carrying its text, so the
wording and the author are read from the database and a caller cannot dress arbitrary text
up as a teammate's comment.

Forwarded text is now fenced in a labelled block rather than concatenated behind a
"[Thread comment from X]" prefix, matching how channel context and custom instructions are
already framed, with the closing delimiter stripped from the body so a comment cannot close
the block early and have the rest read as instructions. This covers thread messages too.


Generated-By: PostHog Code
Task-Id: ec8afa68-0d3b-417a-b9aa-4804b7429d76
@trunk-io

trunk-io Bot commented Jul 31, 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

…n feed

The first cut of this PR carried two features: projecting comment mentions into the task
activity feed, and a server-side endpoint to forward a comment into the agent's run. The
forwarding half was far more code than it earned — a new table, a migration, a facade
function, an endpoint, and two serializers — and the security argument for doing it
server-side does not hold up: the task author can already put arbitrary text in front of
their own agent through the thread, so forwarding a comment by id rather than by text
grants no capability they lack. That work belongs on the client, reusing the thread-message
and send_to_agent endpoints that already exist, so it is dropped here.

What stays is the mention-to-feed fix and the framing hardening. Forwarded content is still
fenced in a labelled block on the existing thread-message forward path, so whenever a
comment is forwarded that way its text cannot break out of its block and read as
instructions.

Removed: TaskCommentForward model + its migration, the forward_comment facade, endpoint,
and serializers, and their tests.


Generated-By: PostHog Code
Task-Id: ec8afa68-0d3b-417a-b9aa-4804b7429d76
@puemos puemos changed the title feat(tasks): surface comment mentions in the activity feed and let the author forward them feat(tasks): surface comment mentions in the activity feed Jul 31, 2026
@github-actions

github-actions Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

🤖 CI report

⚠️ Django migration SQL — 1 new migration to review

We've detected new migrations on this PR. Review the SQL output for each migration:

products/tasks/backend/migrations/0077_taskactivity_comment.py

BEGIN;
--
-- Add field comment to taskactivity
--
ALTER TABLE "posthog_task_activity" ADD COLUMN "comment_id" uuid NULL;
COMMIT;

Last updated: 2026-07-31 10:48 UTC (27591b9)

Django migration risk — migration analysis complete

We've analyzed your migrations for potential risks.

Summary: 1 Safe | 0 Needs Review | 0 Blocked

✅ Safe

Brief or no lock, backwards compatible

tasks.0077_taskactivity_comment
  └─ #1 ✅ AddField
     Adding nullable field requires brief lock
     model: taskactivity, field: comment

📚 How to Deploy These Changes Safely

AddField:

This operation acquires a brief lock but doesn't rewrite the table.

Deployment uses lock timeouts with automatic retries, so lock contention will cause retries rather than connection pile-up.

Last updated: 2026-07-31 10:48 UTC (27591b9)

puemos commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #76407, which combines the desktop and backend changes in one PR.

@puemos puemos closed this Aug 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant