From 737ab3edb1c864447079bd4939dad5c0382d08f7 Mon Sep 17 00:00:00 2001 From: israel Date: Wed, 2 Sep 2026 16:31:16 +0100 Subject: [PATCH] fix(platform): gate the task-comment automation trigger on the author MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two 0.4 behaviours the 0.5 port dropped from the task-comment path. An agent- or workflow-authored comment could start the task's owning automation. 0.4 called the trigger from one place — `applyUserTaskComment`, the single write path for a USER comment — while the agent door hard-coded `actorType: 'agent'` and never reached it. 0.5 merged both lanes into one `addTaskComment`, and `maybeTriggerOwningAutomation` ran unconditionally with no author type to check. The result is a self-sustaining loop: the automation comments, the comment restarts the automation, and every iteration is a metered agent turn. `startWorkflowForTask`'s duplicate guard does not catch it — one live run per (automation, task) blocks a concurrent second start, not a sequential loop. The trigger now takes the author type and returns early unless it is `user`, mirroring the agent lane's `dispatchMentionedProjectAgent` gate right below it in the same file. Editing a comment never re-resolved its mentions: `editTaskComment` wrote only `edited_at_ms`, so editing a comment to add `@someone` notified nobody and the stored mention set went stale. It now re-resolves through `resolveSurfaceMentions` and diffs with `addedMentions` — which had survived the port with no production caller — patching the full set onto the meta row and fanning out the ADDED mentions only, as the bell plus `comment.mentioned` with no subscriber re-alert. Rewording prose around an existing `@handle` still notifies nobody. Editing starts no engine. Both are probed in the real-Postgres integration check, each seen red against deliberately broken code: dropping the author gate enqueues a `task.start_workflow` job for the agent-authored comment; reverting the edit path notifies nobody and leaves the stored set stale; fanning out the full set instead of the added ones gives the already-mentioned teammate a second bell. Task DESCRIPTION mentions (`updateTask`) stay PENDING — a separate decision. --- services/platform/backend/MIGRATION.md | 2 +- .../backend/core/tasks/mentions.test.ts | 5 +- .../backend/domains/tasks/comments.ts | 86 +++++++++++++++++- .../platform/backend/integration-check.ts | 91 ++++++++++++++++++- 4 files changed, 175 insertions(+), 9 deletions(-) diff --git a/services/platform/backend/MIGRATION.md b/services/platform/backend/MIGRATION.md index 5257b63574..544c902ee7 100644 --- a/services/platform/backend/MIGRATION.md +++ b/services/platform/backend/MIGRATION.md @@ -111,7 +111,7 @@ increment. | chat | done | inc 16: `executeTurn` REUSED verbatim (model resolution from org providers, attachment gate, budgeted history, context assembly, guardrail seams, tool rounds, streaming decode) with its store/usage ports swapped for PG (`app.generations` per-thread streaming row, throttled writes + NOTIFY, `app.usage_events`) and every ctx.run* dispatched via `chatShimHandlers`; the 0.4 three-tool executor (rag_search/rag_fetch/web_fetch) runs unchanged on the same shim (entity legs answered by SQL over ported domains; knowledge_entries/websites/conversations/mail/video legs started as honest empties; knowledge_entries and conversations/mail now answer from their ported domains — inc 56 wired the conversations leg (`domains/conversations/search-chat.ts`); websites/video stay empty until those domains land); routes: threads create/list, history, send (caller awaits the turn — the 0.4 action contract, at-most-once), per-thread SSE progress lane (poll at the 250ms write throttle), mid-stream cancel. Governance seams allow-all/no-op until governance ports (checkModelAccess, context cap, recordConnectorUsage, per-subject read matrix). inc 30: the THREAD SURFACE — migration 0023 (surface columns on the `thread_metadata` sidecar: archive/pin/read-watermark/share/capabilities/effort/branch lineage; partial-unique share_token) + `domains/chat/threads.ts` porting `threads.ts`/`thread_lifecycle.ts`/`project_threads.ts`/`search.ts`: pinned-float active list + keyset-paged archived list, owned/project-shared reads (real `checkProjectAccess` gate — the create route's existence-only check upgraded too), capability/effort/rename/pin/read/archive/file-to-project metadata edits (recency preserved; archive audited), org-internal share links (256-bit token IS the URL, `sharedAt` snapshot cut, token survives unshare), branch-at-a-message (history ≤ fork copied, lineage stamped), trash/restore with the generating guard + `branch_root_id` cascade (audited; legal holds ride the retention port; the purge sweep rides retention too), the project Chats tab (mine + shared-with-project), and the 0.4 bounded palette search (40×30 recency scan, AND-token match) over the derived `text` column. TITLE GENERATION reused whole: `generateThreadTitle` hoisted onto the chat shim (+`getChatModelInternal`/`setThreadTitleInternal` handlers) behind the `chat.generate_title` job, auto-enqueued by the TurnStore's first-user-message append on an untitled thread (the same seam also stamps `last_reply_at` for assistant rows and bumps the branch ROOT's recency); the fill-only write never clobbers a rename. NOTE: sandbox execution mode is DEAD in 0.4 (#2877 made chat plain-conversation-only) — dropped from the map. inc 31: EDIT/REGENERATE LINEAGE — migration 0024 (branch_parent_id / branch_fork_sequence / branch_selections) + the branch half of `threads.ts`: `branchForEdit` copies strictly BEFORE the edited user message, `branchForRegenerate` copies THROUGH the prompt it re-answers (chat appends are flat, so `order` IS the 0.4 sequence and copies stay gap-free from zero), both as HIDDEN siblings inheriting agent/capabilities/project on the root's lineage; `listThreadBranches` + the bounded root-side selection map; the chat shim's placeholder `getThreadLineageIds` replaced with the REAL lineage walk, so a turn's retrieval scope (attachment binds included) widens across every sibling. inc 32: AUTO ROUTING live — `modelSelection: 'auto'` was already inside the reused `executeTurn` (`resolveChatModel` resolves a concrete pair before anything binds); what landed is its two missing shim reads (`listActiveCredentialFactsInternal` over `app.provider_credentials`, `resolveModelGovernanceInternal` → the new `resolveModelGovernanceForUser`: the `default_models` pin dropped when `model_access` would refuse it + the accessible catalog subset, over the hoisted pure `filterAccessibleModels`/`findApplicableModelRule`) and the send lane accepting Auto (modelId optional, the engine enforces the XOR). MEMORIES — migration 0025 `app.memories` + `domains/chat/memories.ts`: approval-gated (pending until the OWNER approves; retrieval sees `approved` only, (org,user)-scoped), proposing audited (`memory.save`, category `ai`); surface routes (list/save/review/search); the `memory.save`/`memory.search` TOOL lane stays unwired exactly like 0.4's three-tool executor. DEFERRED SENDS — 0025 `app.deferred_sends` + `domains/chat/deferred-sends.ts`: park-on-Send while media index (readiness matrix over `file_metadata.rag_status`/`transcription_status`; images never gate; video-link legs read absent rows as "erased — proceed" until that domain lands), the 0.4 scheduler chain replaced by the self-chaining `chat.deferred_send_poll` job (aged 3s→15s backoff), claim → the turn runs under the stored identity → the row settles in a `finally` (the 0.4 mop-up posture); tray list + waiting-only cancel + XOR/cap guards. FIXED EN ROUTE (platform-wide): the worker's `notifyPollingIntervalSeconds` sat at 30s — NOTIFY fires on INSERT, never when a delayed job's `startAfter` passes, so EVERY delayed self-chain (deferred sends, automation polls) crawled at up-to-30s per hop; now 2s to match the polling interval. inc 33: the COMPOSER SURFACE — `domains/chat/composer.ts`: the model picker's listing over the SAME connector walk a turn resolves (`walkChatCatalog` on the chat shim; the per-hit projection hoisted as the pure `collectComposerOptions` and REUSED — voice availability riding the same walk), governance-filtered server-side (`getAccessibleModelsForUser` — the picker never even sees a hidden model; the turn re-checks at send), the managed-harness roster (reused loaders + inlined icons), and the capability menus (project/automation skill listings through the reused file-layer viewer with the project's OWN team scope; connectors return the honest empty until the connector-credentials domain lands). Routes: `/composer/models`, `/composer/project/:id/capabilities`, `/composer/automation-capabilities` (developer-gated). PENDING: arena, voice actions (tts/dictation), queue steering, questions surface, trash purge sweep (retention), memory tool lane (with the capability executor, if 0.4 rewires it); inc 97: `getOrgChatHealth` twin (`domains/chat/health.ts`) — assistant-turn fold over one bounded `app.messages` page (errors classified via the shared chat-error decoder, blocked/tokens/series, model+agent breakdowns, agent attribution joined from `thread_metadata` NOT `threads`) behind admin/developer `GET /chat/health`. inc 98: the chat feature's LAST websocket surfaces — QUESTIONS (`domains/chat/questions.ts`: the pending set on `app.approvals` `human_input_request`, one per thread with supersede-on-ask; "still outstanding" DERIVED from the thread — any user message newer than the ask settles it, so no close-write can strand the panel; resolve stamps the transcript's `human-input` part best-effort) and ARENA (`domains/chat/arena.ts`: migration 0054 puts the 0.4 `arena` marker on `thread_metadata`; ensure copies A's bounded history into a hidden B stamped `branch_root_id` but never `branch_parent_id`, idempotent + half-open heal; settle CHOOSES the surviving thread — loser hidden+archived, a winning B graduates — and a verdict inserts the analytics-contract feedback row; the fanned turn busy-gates BOTH columns then runs two `runChatTurn`s concurrently with per-side error rows). Migration 0054 also narrows `message_feedback`'s (message_id, user_id) uniqueness to a PARTIAL index `WHERE metadata IS NULL`: votes keep upserting, arena verdicts stack per run (the 0.4 insert-per-settle contract). FRONTEND: `chat.ts` grew the question/arena/share-status rows, `arena-actions.ts` left the Convex client entirely, and the seam's watch-lane invariant tests re-pointed at a still-unmigrated ref — **every chat read is HTTP now**. BROWSER: Arena Mode toggled in a live thread → pair created over pg → one prompt fanned to two fake-provider models → both columns streamed their own answer → 'B is better' settled (URL swapped to the winner, loser hidden+archived) → the verdict appeared on the feedback metrics page's matchup table. inc 111 ledger truth pass — the row's old PENDING list is stale or empty: ARENA and the QUESTIONS surface landed in inc 98, VOICE actions (synthesize/chunks/dictation/overrides) in inc 78 with the dictation door on `POST /files/dictation`, the TRASH PURGE sweep belongs to retention (`purgeThreadLineage`, wired into its daily cleanup), and QUEUE STEERING has no server lane to port — the composer's send queue is client-side and the mid-turn steer that does exist is the task-agent lane (inc 110). The MEMORY TOOL lane was conditional on 0.4 rewiring its capability executor, which never happened: the chat tool wire is exactly `rag_search`/`rag_fetch`/`web_fetch` (with `ask_question` deliberately off it), so there is no memory tool to port. Every chat read is HTTP; the row's remaining seam is the `@automation` run trigger, tracked on the automations row. inc 112: row CLOSED — every chat read is HTTP, arena/questions/voice/purge all shipped, and the remaining `@automation` trigger is tracked on the automations row | | chat_filter_events | done | inc 91: table (migration 0051) + recent-events list for the guardrails settings page; inc 97: `getGuardrailStats` fold (by kind/filter/direction/category + daily series, bounded newest-first walk) behind `GET /governance/chat-filter-events/stats`; PRODUCER pending — the 0.5 chat sanitize pipeline does not emit rows yet (0.4 wrote them from the filter runner; wire when the guardrail runner lands) | | cloud_import | done | inc 64: migration 0042 (`user_cloud_authorizations` — one secret-box-sealed OAuth2 payload per (org, user, provider), intentional Documents-import grants, never org connector credentials and never agent-resolvable; `cloud_import_oauth_states` — hashed one-shot PKCE states with a TTL + lazy expired sweep) + `domains/cloud_import/{service,routes}.ts` twinning `convex/cloud_import` with the PURE pieces REUSED (deployment_config env resolvers, provider endpoint catalog, `token_refresh.ts` — the Microsoft/Google refresh fetches HOISTED out of the 'use node' action, 0.4 suites 9/9 —, and the whole http_connectors kit: mintStateToken/hashStateToken, generatePkcePair, buildAuthorizeUrl, exchangeAuthorizationCode, the connector error pages). The OAuth doors keep the 0.4 wire path (`/api/cloud-import/oauth2/{start,callback}` + the `/http_api` proxy alias — vendor app registrations carry the callback): start is session + knowledgeWrite gated with the Entra-tenant fallback off the org's SSO issuer; the callback consumes the one-shot state, exchanges server-side, fetches the account label best-effort, and seals the grant. `resolveCloudAccessToken` = the resolve twin (decrypt → refresh at the 5-min buffer → re-seal; refresh-less expiry and failed refresh mark `needs-reauth`). Surface `/api/app/cloud-import` (list own grants metadata-only, revoke drops the sealed payload). Integration: the 302 authorize shape (host/client/PKCE/state), 401/403/unknown-provider gates, vendor-declined consuming the one-shot state + replay refused, store → fresh resolve, expiry-no-refresh → needs-reauth row, revoke → refused, no secret in the listing. The LIVE vendor exchange/refresh (hardcoded Google/Microsoft hosts) stays out of the harness by design | -| collab | done | inc 37: the CORE — migration 0028 (`user_notifications` per-recipient rows + `task_subscriptions` + tri-state `notification_preferences`) + `domains/collab/service.ts` with the COALESCE discipline reused (`coalesceKeyFor` verbatim: an unread twin on the same dimension is rewritten in place, an `undoes` event drops both; 100-row unread scan bound), the pref gate (`isNotificationAllowed`, review group locked always-on), list/unread/mark-read, subscriptions (auto-subscribe, manual subscribe/mute), preferences get/set, and the REVIEW BELL WRITERS wired into `tasks/reviews.ts` (mint → reviewer bell + reviewer auto-subscribed as 'reviewer' + superseded bells dismissed; respond → dismissed + pref-gated resolved bells to watchers minus the decider; withdraw → dismissed). Realtime hints on every write. inc 38: the TASK EMITTERS — `notifyTaskAssigned` (the previous human carrier told with `undoes` so assign-then-unassign leaves nothing; the new human assignee subscribed + told, never for self-assignment; agents/apps have no inbox), `notifyTaskStatusChanged` (subscribers minus the actor, wired into BOTH status writers — the human route and the trusted agent door), `notifyTaskComment` (commenter follows, mentioned humans get the precedence 'mention' row, other subscribers `task_commented`; wired into `addTaskComment` — mention EXTRACTION still rides the mention-directory port), and the creator auto-follow at `createTask`. inc 39: AGENT-ASK ESCALATIONS — `notifyAgentQuestionAsked` fans `agent_escalation` rows to everyone who can SEE the run's project (admins ∪ team members; org-wide project = all members; no project = org admins; 500 cap), wired into the tool door's `createAskForExec` (create AND fold — with a task subject the `question` dimension rewrites the unread row; a no-task ask has no collapse subject, the 0.4 posture) and dismissed transactionally by `answerAsk`/`closeAsk` (ONE SQL by `params->>'askId'` — tighter than 0.4's capped per-member walk). Plus the PENDING-REVIEWS FACET: `collectPendingReviewsForProjects` (bounded org-level read over `metadata->>'projectId'`) behind `GET /tasks/pending-reviews` for the board chips (registered BEFORE the `/:taskId` wildcard). inc 54: the debounced EMAIL sink — every actionable write in `writeCoalescedNotification` bumps the row's `email_epoch` (migration 0038) and enqueues `notification.email` (retryLimit 0) one debounce window out (60s; env-tunable `NOTIFICATION_EMAIL_DEBOUNCE_MS`); the job (`domains/collab/email-sink.ts`, the 0.4 `deliverActionableEmailAction` twin) re-reads the row and sends only when it is still unread AND the payload epoch is current — the epoch fence REPLACES the 0.4 cancel+reschedule (a rewrite's older job no-ops, its newer job carries the final state; an undo deletes the row; a read row mails nothing), safe under at-least-once delivery. Delivery: recipient email from the auth `user` row, the tri-state `actionable_email` pref (absent = ON), mailbox picked by the REUSED `pickSendableMailbox` over active mail credentials (the pure pick/input helpers HOISTED out of `send_actionable_email.ts` into `actionable_email_input.ts`; `buildPersonalNotificationUrl` hoisted to `personal_notification_url.ts` — both stacks import the seams, 0.4 suites stay green), org default locale from `organization.metadata`, `renderActionableEmailContent` REUSED, and the send through the connector door as the system caller (imap-smtp `notificationSender` From rewrite). Integration: burst → ONE email with the final state + deep link + `notification@` From, read/undo/pref-off all silent; the harness now installs a run-wide default fake mail transport (no itest job ever opens a real IMAP/SMTP connection) and drains pending notification emails before every SMTP-counting check. PENDING: mention directory + description mentions, automation alerts, attention summary, deadline crons. inc 106: the MENTION lane + the return loop. `domains/collab/mention-directory.ts` builds the project-scoped directory on pg with the 0.4 rules intact — only members who can ACCESS the project are mentionable (through the SHARED `hasProjectAccess`, so the mentionable set can never disagree with who can open the task), handle precedence is insertion order (listed slugs → deployed automations → the project's agent INSTANCES last, so an instance shadows a same-named retired slug and the mention reaches the live lane), a non-`restricted` project is PERMISSIVE (an unclaimed token reads as an agent handle), and every leg degrades on its own. The scanning itself is REUSED from `tasks/mentions.ts` — one grammar for `@handle`. Task comments now resolve mentions for real (the TODO is gone): the resolved list drives the existing notify fan-out (bell + auto-subscribe with reason `mention`) and rides the meta row, while tokens that matched nobody go back to the composer so the author is told instead of silently ignored. `getMyAttentionSummary` + `GET /collab/attention` complete the return loop — unread split actionable/total by the shared predicate, task reviews waiting on THIS person, and their own open assignments merged into one id set so a task that is both counts once; `projectId` scopes it to one board. PENDING: the @automation RUN TRIGGER and steering a mention into a live agent run (they belong to the automations/agents lanes). inc 113 CLOSES the row: the `@automation` RUN TRIGGER ships — @-ing the automation that OWNS a task starts its task workflow (which re-reads the timeline, this comment included), while a plain comment or a mention of any OTHER automation starts nothing. Ownership is the 0.4 three-shape rule (app-assigned names it, app-created names its creator, externally-mirrored matches through the deployed version's task contract); the gate is WRITE access (commenting is read-level, running a workflow is an edit) and one engine per task across BOTH lanes. The start is ENQUEUED, not inline: the comment must commit first, and 0.4 scheduled it for the same reason. Steering a mention into a live agent run landed in inc 110 | +| collab | done | inc 37: the CORE — migration 0028 (`user_notifications` per-recipient rows + `task_subscriptions` + tri-state `notification_preferences`) + `domains/collab/service.ts` with the COALESCE discipline reused (`coalesceKeyFor` verbatim: an unread twin on the same dimension is rewritten in place, an `undoes` event drops both; 100-row unread scan bound), the pref gate (`isNotificationAllowed`, review group locked always-on), list/unread/mark-read, subscriptions (auto-subscribe, manual subscribe/mute), preferences get/set, and the REVIEW BELL WRITERS wired into `tasks/reviews.ts` (mint → reviewer bell + reviewer auto-subscribed as 'reviewer' + superseded bells dismissed; respond → dismissed + pref-gated resolved bells to watchers minus the decider; withdraw → dismissed). Realtime hints on every write. inc 38: the TASK EMITTERS — `notifyTaskAssigned` (the previous human carrier told with `undoes` so assign-then-unassign leaves nothing; the new human assignee subscribed + told, never for self-assignment; agents/apps have no inbox), `notifyTaskStatusChanged` (subscribers minus the actor, wired into BOTH status writers — the human route and the trusted agent door), `notifyTaskComment` (commenter follows, mentioned humans get the precedence 'mention' row, other subscribers `task_commented`; wired into `addTaskComment` — mention EXTRACTION still rides the mention-directory port), and the creator auto-follow at `createTask`. inc 39: AGENT-ASK ESCALATIONS — `notifyAgentQuestionAsked` fans `agent_escalation` rows to everyone who can SEE the run's project (admins ∪ team members; org-wide project = all members; no project = org admins; 500 cap), wired into the tool door's `createAskForExec` (create AND fold — with a task subject the `question` dimension rewrites the unread row; a no-task ask has no collapse subject, the 0.4 posture) and dismissed transactionally by `answerAsk`/`closeAsk` (ONE SQL by `params->>'askId'` — tighter than 0.4's capped per-member walk). Plus the PENDING-REVIEWS FACET: `collectPendingReviewsForProjects` (bounded org-level read over `metadata->>'projectId'`) behind `GET /tasks/pending-reviews` for the board chips (registered BEFORE the `/:taskId` wildcard). inc 54: the debounced EMAIL sink — every actionable write in `writeCoalescedNotification` bumps the row's `email_epoch` (migration 0038) and enqueues `notification.email` (retryLimit 0) one debounce window out (60s; env-tunable `NOTIFICATION_EMAIL_DEBOUNCE_MS`); the job (`domains/collab/email-sink.ts`, the 0.4 `deliverActionableEmailAction` twin) re-reads the row and sends only when it is still unread AND the payload epoch is current — the epoch fence REPLACES the 0.4 cancel+reschedule (a rewrite's older job no-ops, its newer job carries the final state; an undo deletes the row; a read row mails nothing), safe under at-least-once delivery. Delivery: recipient email from the auth `user` row, the tri-state `actionable_email` pref (absent = ON), mailbox picked by the REUSED `pickSendableMailbox` over active mail credentials (the pure pick/input helpers HOISTED out of `send_actionable_email.ts` into `actionable_email_input.ts`; `buildPersonalNotificationUrl` hoisted to `personal_notification_url.ts` — both stacks import the seams, 0.4 suites stay green), org default locale from `organization.metadata`, `renderActionableEmailContent` REUSED, and the send through the connector door as the system caller (imap-smtp `notificationSender` From rewrite). Integration: burst → ONE email with the final state + deep link + `notification@` From, read/undo/pref-off all silent; the harness now installs a run-wide default fake mail transport (no itest job ever opens a real IMAP/SMTP connection) and drains pending notification emails before every SMTP-counting check. PENDING: mention directory + description mentions, automation alerts, attention summary, deadline crons. inc 106: the MENTION lane + the return loop. `domains/collab/mention-directory.ts` builds the project-scoped directory on pg with the 0.4 rules intact — only members who can ACCESS the project are mentionable (through the SHARED `hasProjectAccess`, so the mentionable set can never disagree with who can open the task), handle precedence is insertion order (listed slugs → deployed automations → the project's agent INSTANCES last, so an instance shadows a same-named retired slug and the mention reaches the live lane), a non-`restricted` project is PERMISSIVE (an unclaimed token reads as an agent handle), and every leg degrades on its own. The scanning itself is REUSED from `tasks/mentions.ts` — one grammar for `@handle`. Task comments now resolve mentions for real (the TODO is gone): the resolved list drives the existing notify fan-out (bell + auto-subscribe with reason `mention`) and rides the meta row, while tokens that matched nobody go back to the composer so the author is told instead of silently ignored. `getMyAttentionSummary` + `GET /collab/attention` complete the return loop — unread split actionable/total by the shared predicate, task reviews waiting on THIS person, and their own open assignments merged into one id set so a task that is both counts once; `projectId` scopes it to one board. PENDING: the @automation RUN TRIGGER and steering a mention into a live agent run (they belong to the automations/agents lanes). inc 113 CLOSES the row: the `@automation` RUN TRIGGER ships — @-ing the automation that OWNS a task starts its task workflow (which re-reads the timeline, this comment included), while a plain comment or a mention of any OTHER automation starts nothing. Ownership is the 0.4 three-shape rule (app-assigned names it, app-created names its creator, externally-mirrored matches through the deployed version's task contract); the gate is WRITE access (commenting is read-level, running a workflow is an edit) and one engine per task across BOTH lanes. The start is ENQUEUED, not inline: the comment must commit first, and 0.4 scheduled it for the same reason. Steering a mention into a live agent run landed in inc 110. TWO 0.4 behaviours the port had dropped are back: the run trigger gates on the comment's AUTHOR TYPE (0.4 reached it only from `applyUserTaskComment`; 0.5 merged the user and agent doors into one `addTaskComment`, so an agent- or workflow-authored comment naming the owning automation restarted the engine that wrote it — a sequential loop of metered agent turns the one-live-run guard cannot see, because it blocks a CONCURRENT second start, not a later one), and `editTaskComment` RE-RESOLVES mentions (it wrote only `edited_at_ms`, so editing a comment to add `@someone` notified nobody and the stored set went stale; the fan-out is `addedMentions` only, never the full set). Description mentions (`updateTask`) stay PENDING | | connector_credentials | done | inc 50: migration 0035 (`app.connector_credentials` — one AES-256-GCM envelope per row via the REUSED `lib/secret_box`, case-insensitive name uniqueness + the at-most-one-default invariant as UNIQUE INDEXES, per-credential mail-sync watermarks) + `domains/connector_credentials/service.ts` twinning `mutations/actions/queries/resolve_credential` with the 0.4 PURE modules reused verbatim (`auth_injection` payload parse/Basic-bearer header build/secret bindings, `masking`, `normalizeEndpointOrigin`, `withImapFromAddress` From-mirroring, the shipped `lib/connectors/catalog` reader — auth-method + `configFields` validation incl. number/enum coercion and defaults). Plaintext reachable ONLY through `resolveConnectorCredential` (id-or-name ref, else the default; coded refusals for disabled / needs-reauth / key-rotated / shape-invalid); default juggling: first-is-default, promote-demotes, delete promotes the OLDEST remaining ACTIVE row. `listActiveCredentials` + `patchMailSyncWatermarks` ready for the mailbox-sync lane. Routes `/api/app/connector-credentials` (reads = member; writes = developer capability, the 0.4 gate). Integration: create against the real shipped imap-smtp connector (defaults + From-mirroring asserted), masked listing with a secret-leak sweep, name clash 409 / unknown connector 404 / wrong method 400, the resolve seam (secrets + config + computed Basic header, name ref), promote → delete-promotes-oldest, disabled refusal. PENDING: oauth2 grant flows (`storeOauth2Credential` + refresh — ride the per-connector OAuth routes), 0.4-parity NO audit rows (none in 0.4 either) | | connectors | done | inc 52: THE DOOR — `domains/connectors/service.ts` twins `execute_action.ts`: the REUSED dispatcher (`lib/connectors` catalog/registry/validation/caller policy/idempotency) assembled per call with the node-vm code runner, the PG credential resolver, the PG approvals gate, and the audit sink (`connector.` rows, category `connector`; system callers must state a reason). Native backends: imap-smtp (REUSED whole; `setMailTransportForTesting` injection seam), platform task/document stores over the 0.5 domains, conversations = the REUSED sync/ingest modules (see conversations); the WebDAV store is REAL (inc 62 — the shared tree via `domains/webdav/connector-store.ts`); sandbox scripts fail loud until that lane lands. The automation stepper's connector nodes and approvals gate now dispatch through this door (both fail-loud shim placeholders replaced). Integration: the mailbox lane end-to-end + a user-caller live write parking `approval-required`, human approve (executing), the retried operation running and consuming the record. inc 124: the in-sandbox CONNECTORS BRIDGE lands, and with it out-of-process live execution. CORRECTION to inc 118's note: live yaml-js is NOT refused in 0.4 — the dispatcher refuses only when the runner is the in-process `node-vm`; the bridge hands it a SESSION-BOUND sandbox-exec runner, so a 0.4 external turn does run live bodies. That plumb-through is now here: `runConnectorAction` takes an `execSessionId`, mints the one-run HMAC capability the in-sandbox façade phones home with, and passes the per-invocation runner (never the process-global slot, which every concurrent org shares); with no HMAC root it falls back to the in-process refusal rather than running a body whose `ctx.http` could not be mediated. `domains/connectors/bridge-routes.ts` serves the three doors: `execute`/`status` authenticate the session VK and take the org, the user and the GRANT SET from the token row (a container names a connector, never an org or a grant), and `hostcall` authenticates the capability and re-resolves the credential ITSELF, so a leaked token cannot become one. Both decision bodies are REUSED — hoisted out of the 0.4 bridge into `runBridgeConnectorImpl`/`bridgeConnectorStatusImpl` — so every refusal reads identically to the model that relays it, including V1's read-only rule. Cutover: compose dual-homes `backend-api` on the sandbox network and derives `SANDBOX_HTTP_API_BASE_URL` from the switch, and the proxy template gains the bridge, host-call, connector-OAuth and Slack-events lanes (the last two were a GAP from incs 102/103 — their doors existed but no cutover route reached them). Blob sink (`ctx.files`) stays absent in BOTH lanes: the dispatcher takes a `blobs` sink and neither host supplies one, so it is a shared-lane capability, not a migration debt | | contacts | done | inc 12: CRUD/list-filter/find-or-create (email normalized), soft trash, contact.* events; REST lanes with the machine door (inc 22) + inc 46 bulk import (per-item duplicate email/externalId accounting, 0.4 un-audited-rows parity). inc 99: the org-level legal-hold gate refuses `deleteContact` (the 0.4 posture: contacts carry no per-row hold). inc 112 audit — no open seam: CRUD, the keyset listing, the org-hold delete guard and the conversation linkage ship and are probed | diff --git a/services/platform/backend/core/tasks/mentions.test.ts b/services/platform/backend/core/tasks/mentions.test.ts index b3db5ac28c..3ddbab34e9 100644 --- a/services/platform/backend/core/tasks/mentions.test.ts +++ b/services/platform/backend/core/tasks/mentions.test.ts @@ -181,8 +181,9 @@ describe('addedMentions', () => { expect(addedMentions([], next)).toEqual(next); }); - // Comment edits reuse the same helper as description edits - // (`editTaskDiscussionMessage` → `addedMentions` → `comment.mentioned`). + // The comment-edit wire: `editTaskComment` re-resolves the body, diffs it + // through this helper, and fans out `comment.mentioned` for the added ones + // alone (`domains/tasks/comments.ts`). it('flags an @agent added when editing a comment that had none', () => { const previous = extractMentions('already fixed', directory); const next = extractMentions( diff --git a/services/platform/backend/domains/tasks/comments.ts b/services/platform/backend/domains/tasks/comments.ts index b6e0b0fe7a..42de098d86 100644 --- a/services/platform/backend/domains/tasks/comments.ts +++ b/services/platform/backend/domains/tasks/comments.ts @@ -3,6 +3,10 @@ import { z } from 'zod'; import { parseTaskSubjectContract } from '../../../lib/shared/schemas/task_contract.ts'; import { TASK_AUDIT_ACTIONS } from '../../core/tasks/audit_actions.ts'; +import { + addedMentions, + type ResolvedMention, +} from '../../core/tasks/mentions.ts'; import type { CommentEventComment } from '../../core/tasks/types.ts'; import { toJson } from '../../db/sql.ts'; import { addJobInTx } from '../../jobs/enqueue.ts'; @@ -147,6 +151,7 @@ export async function addTaskComment( auth, task, mentions, + authorType: author.actorType, }); // A comment that @-mentions one of the project's agent INSTANCES puts it // to work: steering its RUNNING turn, or — when the task is idle — @@ -345,12 +350,22 @@ export async function listTaskComments( async function loadCommentMeta( tx: TransactionSql | Sql, messageId: string, -): Promise<{ taskId: string; authorType: string; authorId: string }> { +): Promise<{ + taskId: string; + authorType: string; + authorId: string; + mentions: ResolvedMention[] | null; +}> { const rows = await tx< - { taskId: string; authorType: string; authorId: string }[] + { + taskId: string; + authorType: string; + authorId: string; + mentions: ResolvedMention[] | null; + }[] >` SELECT task_id AS "taskId", author_type AS "authorType", - author_id AS "authorId" + author_id AS "authorId", mentions FROM app.task_discussion_message_meta WHERE message_id = ${messageId} `; const meta = rows[0]; @@ -375,6 +390,23 @@ function assertCommentOwnerOrAdmin( } } +/** + * Edit one comment's body — and RE-RESOLVE what it names. + * + * An edit is a second chance to mention someone: adding `@handle` to a + * comment has to reach them, and the stored mention set has to stay the + * truth of who the comment names (it is what the feed renders and what the + * next edit diffs against). Only the NEWLY added mentions fan out — + * rewording prose around an existing `@handle` must not re-notify, and + * re-notifying the whole set on every edit is exactly what the 0.4 + * `addedMentions` diff existed to prevent. + * + * The fan-out is the MENTION half of {@link addTaskComment} only: the bell + * and auto-subscribe for the newly named, no subscriber re-alert (this is + * not a fresh comment), and `comment.mentioned` rather than + * `comment.created`. Editing never starts an engine — the automation + * trigger and the agent dispatch belong to a posted comment. + */ export async function editTaskComment( tx: TransactionSql, auth: ProjectAuthContext, @@ -389,12 +421,47 @@ export async function editTaskComment( if (body.length === 0 || body.length > TASK_COMMENT_MAX) { throw new TaskError('TASK_COMMENT_INVALID', 'Invalid comment body'); } + const resolved = await resolveSurfaceMentions(tx, { + organizationId: auth.organizationId, + body, + projectId: task.projectId, + }); + const mentions = resolved.mentions; + const added = addedMentions(meta.mentions ?? [], mentions); await updateMessageText(tx, args.messageId, body); await tx` UPDATE app.task_discussion_message_meta - SET edited_at_ms = ${Date.now()} + SET edited_at_ms = ${Date.now()}, + mentions = ${mentions.length > 0 ? tx.json(toJson(mentions)) : null} WHERE message_id = ${args.messageId} `; + if (added.length > 0) { + await notifyTaskComment(tx, { + task, + commentId: args.messageId, + mentions: added, + actorType: 'user', + actorId: auth.userId, + notifySubscribers: false, + }); + const comment: CommentEventComment = { + body, + projectId: task.projectId, + taskId: meta.taskId, + mentions: added, + }; + await emitEvent(tx, { + organizationId: auth.organizationId, + eventType: 'comment.mentioned', + eventData: { + comment, + taskId: meta.taskId, + mentions: added, + actorType: 'user', + actorId: auth.userId, + }, + }); + } await createAuditLog(tx, { organizationId: auth.organizationId, actorId: auth.userId, @@ -405,7 +472,7 @@ export async function editTaskComment( resourceType: 'task_comment', resourceId: args.messageId, resourceName: task.title, - metadata: { taskId: meta.taskId }, + metadata: { taskId: meta.taskId, addedMentionCount: added.length }, status: 'success', }); await emitHintInTx(tx, { @@ -683,6 +750,13 @@ async function dispatchMentionedProjectAgent( * run keeps it; `startWorkflowForTask`'s own duplicate guard backstops the * pre-check. * + * Only a HUMAN's comment starts anything — the same rule the agent lane's + * dispatcher keeps. An agent- or workflow-authored comment naming the owning + * automation would restart the very engine that wrote it, and each iteration + * is a metered agent turn; `startWorkflowForTask`'s one-live-run-per-task + * guard blocks a concurrent second start, not a sequential loop, so the + * author type is the only thing standing between a comment and that loop. + * * Returns whether a start was scheduled, so the caller can skip the steer * lane for the same comment. */ @@ -692,8 +766,10 @@ async function maybeTriggerOwningAutomation( auth: ProjectAuthContext; task: TaskRow; mentions: { type: string; id: string }[]; + authorType: string; }, ): Promise { + if (args.authorType !== 'user') return false; const mentioned = args.mentions.find( (mention) => mention.type === 'automation', ); diff --git a/services/platform/backend/integration-check.ts b/services/platform/backend/integration-check.ts index 3c80b6494f..62276ab4fd 100644 --- a/services/platform/backend/integration-check.ts +++ b/services/platform/backend/integration-check.ts @@ -12149,6 +12149,30 @@ async function checkRunProvenance( SELECT count(*)::text AS count FROM app.automation_runs WHERE org_id = ${orgId} AND name = ${automationName} `; + // An AGENT- or WORKFLOW-authored comment naming the OWNER starts nothing. + // Otherwise the automation restarts the engine that wrote the comment and + // every iteration is a metered agent turn; `startWorkflowForTask`'s + // one-live-run guard blocks a concurrent second start, not a sequential + // loop. 0.4 kept the trigger on the USER comment path alone. + const startJobsBeforeAgent = await sql<{ count: string }[]>` + SELECT count(*)::text AS count FROM pgboss.job + WHERE name = 'task.start_workflow' + `; + await sql.begin((tx) => + addComment(tx, commentAuth, { + taskId: ownedTaskId, + body: `@${automationName} continuing my own work`, + author: { actorType: 'agent', actorId: 'workflow' }, + }), + ); + const startJobsAfterAgent = await sql<{ count: string }[]>` + SELECT count(*)::text AS count FROM pgboss.job + WHERE name = 'task.start_workflow' + `; + const runsAfterAgentAuthor = await sql<{ count: string }[]>` + SELECT count(*)::text AS count FROM app.automation_runs + WHERE org_id = ${orgId} AND name = ${automationName} + `; // @-ing the OWNER starts its task workflow. await sql.begin((tx) => addComment(tx, commentAuth, { @@ -12156,6 +12180,20 @@ async function checkRunProvenance( body: `@${automationName} please pick this up`, }), ); + // The same seam, the human lane: the start job IS enqueued. Read before + // the poll below so the count cannot be confused with a worker's doing. + const startJobsAfterUser = await sql<{ count: string }[]>` + SELECT count(*)::text AS count FROM pgboss.job + WHERE name = 'task.start_workflow' + `; + record( + 'automations: an agent-authored @owner comment never starts the workflow', + startJobsAfterAgent[0]?.count === startJobsBeforeAgent[0]?.count && + runsAfterAgentAuthor[0]?.count === runsBefore[0]?.count && + Number(startJobsAfterUser[0]?.count ?? '0') === + Number(startJobsBeforeAgent[0]?.count ?? '0') + 1, + `agentAuthored: startJobs=${startJobsAfterAgent[0]?.count} (unchanged from ${startJobsBeforeAgent[0]?.count}), runs=${runsAfterAgentAuthor[0]?.count} (want ${runsBefore[0]?.count}); humanAuthored: startJobs=${startJobsAfterUser[0]?.count} (want ${Number(startJobsBeforeAgent[0]?.count ?? '0') + 1})`, + ); // The start is ENQUEUED (the comment commits first); give the worker a // moment, then assert the run itself. let startedRuns: { id: string; input: unknown }[] = []; @@ -13166,7 +13204,10 @@ async function checkCollabMentions( `; const taskId = taskRows[0]?.id ?? ''; const commented = z - .object({ unresolvedMentionTokens: z.array(z.string()) }) + .object({ + messageId: z.string(), + unresolvedMentionTokens: z.array(z.string()), + }) .loose() .safeParse( await ( @@ -13193,6 +13234,54 @@ async function checkCollabMentions( `unresolved=${commented.success ? commented.data.unresolvedMentionTokens.join(',') : 'ERR'}, mentionBells=${bell[0]?.count}, autoSubscribed=${subscription[0]?.count}`, ); + // ---- editing a comment RE-RESOLVES its mentions ----------------------- + // An edit is a second chance to name someone: the newly added mention has + // to reach them and the stored set has to catch up, while a mention that + // was already there must NOT fire again — mention rows never coalesce, so + // a re-notification would show up as a second bell. + const editedMessageId = commented.success ? commented.data.messageId : ''; + const editStatus = ( + await api(`/api/app/tasks/comments/${editedMessageId}`, { + body: { + body: '@mention-teammate-1 can you review? cc @mention-outsider-1', + }, + }) + ).status; + const teammateBellsAfterEdit = await sql<{ count: string }[]>` + SELECT count(*)::text AS count FROM app.user_notifications + WHERE org_id = ${orgId} AND user_id = ${teammate} AND type = 'mention' + `; + const outsiderBellsAfterEdit = await sql<{ count: string }[]>` + SELECT count(*)::text AS count FROM app.user_notifications + WHERE org_id = ${orgId} AND user_id = ${outsider} AND type = 'mention' + AND resource_id = ${editedMessageId} + `; + const editedMeta = await sql< + { + mentions: { type: string; id: string }[] | null; + editedAt: number | null; + }[] + >` + SELECT mentions, edited_at_ms::float8 AS "editedAt" + FROM app.task_discussion_message_meta + WHERE message_id = ${editedMessageId} + `; + const storedMentionKeys = (editedMeta[0]?.mentions ?? []).map( + (mention) => `${mention.type}:${mention.id}`, + ); + record( + 'mentions: editing a comment notifies the ADDED mention only', + editStatus === 200 && + Number(outsiderBellsAfterEdit[0]?.count ?? '0') === 1 && + // The teammate was already named before the edit — no second bell. + teammateBellsAfterEdit[0]?.count === bell[0]?.count && + // The stored set is the FULL truth of who the comment now names. + storedMentionKeys.includes(`user:${teammate}`) && + storedMentionKeys.includes(`user:${outsider}`) && + editedMeta[0]?.editedAt !== null, + `edit=${editStatus}, addedBell=${outsiderBellsAfterEdit[0]?.count} (want 1), alreadyMentionedBells=${teammateBellsAfterEdit[0]?.count} (unchanged from ${bell[0]?.count}), stored=${storedMentionKeys.join(',') || 'none'}, editedAt=${editedMeta[0]?.editedAt !== null}`, + ); + // ---- attention summary ------------------------------------------------ await sql` UPDATE app.tasks SET assignee_type = 'user', assignee_id = ${userId}