-
Notifications
You must be signed in to change notification settings - Fork 3k
feat(daemon): add explicit channel delivery #7388
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
b4d2d4b
8a28e41
47f8887
e1ec034
9f0881e
e811053
a0d2795
106592a
e61320c
0fcd38d
c39fb56
4ed0b01
7e8fb43
c7c17d7
1f1c0c8
9eb6a81
2d2569e
72fa93a
7394ca7
4781fe6
06b35e8
265f2aa
8d157ec
baf3c9e
85fc094
43a5e76
6ed4459
e5e9348
21e7f68
103587c
62e706a
af2ee80
bd5ab63
7b54191
076be35
2320748
47629ed
464baba
8b3f5f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| # Channel Delivery V1 | ||
|
|
||
| ## Goal | ||
|
|
||
| Allow scheduled tasks, daemon prompts, and a direct Notify API to send text to an explicit IM target through the Channel Worker that owns the selected workspace. Delivery is immediate and best effort: there is no durable outbox, replay, retry, or global final-answer hook. | ||
|
|
||
| ## Public contract | ||
|
|
||
| ```ts | ||
| interface ChannelDelivery { | ||
| kind: 'channel'; | ||
| target: { | ||
| channelName: string; | ||
| type: 'user' | 'chat'; | ||
| id: string; | ||
| }; | ||
| } | ||
| ``` | ||
|
|
||
| Scheduled task creation and `POST /session/:id/prompt` accept an optional top-level `delivery`. Direct notification uses: | ||
|
|
||
| ```http | ||
| POST /workspace/notify | ||
| POST /workspaces/:workspace/notify | ||
|
|
||
| { | ||
| "text": "alert text", | ||
| "delivery": { | ||
| "kind": "channel", | ||
| "target": { | ||
| "channelName": "dingtalk", | ||
| "type": "user", | ||
| "id": "platform-user-id" | ||
| } | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| The daemon normalizes the public target at its trust boundary to the internal worker request `{ deliveryId, channelName, target: { type, id }, text }`. Text sent to a worker must be non-empty and is bounded to 100,000 UTF-16 code units before IPC. Prompt and scheduled reverse control may carry an empty string only to report a successful turn with no deliverable final answer as `skipped`; that path never reaches worker IPC. | ||
|
|
||
| ## Execution boundaries | ||
|
|
||
| Scheduled tasks and Prompt own their final-answer semantics. A Session captures text only when the current invocation carries delivery metadata. Each model send owns one response block: non-thought stream chunks are joined within that block, non-continuation retry or model fallback discards superseded chunks, and any block that requests a tool is intermediate and cannot become the delivery payload. A later automatic continuation replaces the earlier terminal candidate. After the complete turn reaches a successful `end_turn`, the Session submits exactly one reverse control request containing only the last tool-free assistant response block. Inter-tool narration and all earlier response blocks are excluded. | ||
|
|
||
| Successful `end_turn` always submits the reverse control request, including when the final block is empty or whitespace-only. The daemon consumes the pinned authorization first, returns `skipped` without resolving a worker, and publishes a `channel_delivery_result` event. Cancellation, Agent failure, and token-limit termination submit nothing. Empty output is therefore distinguishable from a turn that was never eligible for delivery. | ||
|
|
||
| Prompt admission remains `202`; Agent completion remains `turn_complete` or `turn_error`. Channel completion is a later `channel_delivery_result` event and never converts Agent success into `turn_error`. | ||
|
|
||
| Notify bypasses Session and Agent. It waits for one worker delivery attempt and maps invalid input to 400, unavailable or full workers to 503, timeout to 504, and adapter failure to 502. A timeout has an unknown delivery outcome and is not retried. | ||
|
|
||
| Webhook remains an independent asynchronous path with its own secret and `202` worker-admission contract. It may reuse `ChannelBase` sending primitives and error classification, but not Prompt/Notify control flow. Background notification prompts remain local Agent work and do not automatically send to IM. | ||
|
|
||
| ## Workspace ownership | ||
|
|
||
| The daemon binds the workspace when constructing each ACP bridge. Prompt admission records the daemon-issued delivery ID and pinned target, while scheduled delivery is authorized from the persisted task. The child callback must match that authorization and cannot choose `workspaceCwd` or replace the target. The host callback consumes the authorization before deciding between `skipped` and worker delivery, so empty finals cannot forge events or leave one-shot/monotonic authorization state unchanged. Non-empty text routes only to the canonical workspace's worker group. Missing, bootstrapping, draining, stopped, or removed owners return `channel_worker_unavailable`; there is no fallback to the primary runtime and no lazy worker startup. | ||
|
|
||
| ## Reliability and privacy | ||
|
|
||
| Authorization is consumed before worker availability is checked, so a transient worker blip after consume drops that single delivery permanently; this is consistent with the immediate, best-effort, no-retry contract. | ||
|
|
||
| This V1 has no persistence, startup replay, historical scan, retry, or idempotency guarantee. Existing tasks without delivery never send. Existing scheduler catch-up behavior is unchanged. Normal executions carry delivery only when the task already contains it; the synthetic historical missed-one-shot batch explicitly clears delivery so enabling Channel later cannot create a burst of old alerts. | ||
|
|
||
| V1 observes the Channel send Promise only. A rejection is sanitized and mapped to `channel_delivery_failed`, except adapters that already provide a typed permanent disposition map to `channel_delivery_rejected`. Provider-specific response parsing and consistent error-reason semantics across IM adapters are follow-up work; the daemon and worker contain no platform-specific error handling. | ||
|
|
||
| Delivery result events and logs include correlation identifiers, source, status, and sanitized error data. They never include message text, target IDs, credentials, or webhook secrets. `delivered` means the adapter send Promise resolved; it does not assert that the provider accepted the message or that a user received or read it. | ||
|
|
||
| ## Capability | ||
|
|
||
| The daemon advertises `channel_delivery` when it supports the contracts and routes. This is protocol support, not a live-health assertion for any worker or adapter. |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -187,7 +187,7 @@ registry. Clients **must** gate UI off `features`, not off `mode` (per design | |||||
| 'permission_mediation', 'prompt_absolute_deadline', 'writer_idle_timeout', | ||||||
| 'non_blocking_prompt', 'session_language', 'session_rewind', | ||||||
| 'workspace_hooks', 'session_hooks', 'workspace_extensions', | ||||||
| 'session_branch', 'rate_limit', 'workspace_reload', | ||||||
| 'session_branch', 'rate_limit', 'workspace_reload', 'channel_delivery', | ||||||
| 'multi_workspace_sessions', 'multi_workspace_session_rewind', | ||||||
| 'multi_workspace_session_shell', 'persistent_workspace_registration', | ||||||
| 'workspace_display_name', | ||||||
|
|
@@ -748,6 +748,89 @@ readers; single-workspace daemons keep the original single-worker shape. Worker | |||||
| stdout/stderr are forwarded into the daemon log with bearer tokens, sensitive | ||||||
| worker environment values, and proxy URL credentials redacted. | ||||||
|
|
||||||
| ### Channel delivery and Notify | ||||||
|
|
||||||
| `channel_delivery` advertises immediate, best-effort delivery support. It is a | ||||||
| protocol capability, not a worker health signal. Delivery never starts a | ||||||
| missing worker, falls back to another workspace, retries, persists an outbox, | ||||||
| or replays historical notifications. | ||||||
|
|
||||||
| Direct Notify bypasses Agent and Session and waits for one send attempt: | ||||||
|
|
||||||
| ```http | ||||||
| POST /workspace/notify | ||||||
| POST /workspaces/:workspace/notify | ||||||
| Authorization: Bearer <token> | ||||||
| Content-Type: application/json | ||||||
|
|
||||||
| { | ||||||
| "text": "service unavailable", | ||||||
| "delivery": { | ||||||
| "kind": "channel", | ||||||
| "target": { | ||||||
| "channelName": "dingtalk", | ||||||
| "type": "user", | ||||||
| "id": "platform-user-id" | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| ``` | ||||||
|
|
||||||
| Both routes use the strict mutation gate. The qualified route resolves only a | ||||||
| registered, trusted workspace. Success is `200 {delivered:true,deliveryId}`. | ||||||
| `delivered:true` means the Channel send Promise resolved; it does not prove | ||||||
| provider acceptance, user receipt, or a read receipt. Provider-specific | ||||||
| response validation and consistent error-reason semantics across IM adapters | ||||||
| are outside this V1 contract. | ||||||
| Errors are `400 channel_delivery_invalid`, `503 channel_worker_unavailable` or | ||||||
| `channel_delivery_queue_full`, `504 channel_delivery_timeout`, and `502 | ||||||
| channel_delivery_rejected` or `channel_delivery_failed`. A timeout has an | ||||||
| unknown outcome and is not retried. | ||||||
| There is intentionally no separate connectivity-test endpoint: a normal | ||||||
| Notify call is the end-to-end test. | ||||||
|
|
||||||
| The replayable result event contains only correlation and sanitized status: | ||||||
|
|
||||||
| ```json | ||||||
| { | ||||||
| "type": "channel_delivery_result", | ||||||
| "promptId": "prompt-1", | ||||||
| "data": { | ||||||
| "sessionId": "session-1", | ||||||
| "deliveryId": "prompt-1", | ||||||
| "source": "prompt", | ||||||
| "status": "failed", | ||||||
| "promptId": "prompt-1", | ||||||
| "code": "channel_worker_unavailable", | ||||||
| "error": "Channel worker is not running." | ||||||
| } | ||||||
| } | ||||||
| ``` | ||||||
|
|
||||||
| An empty successful Prompt final omits error fields: | ||||||
|
|
||||||
| ```json | ||||||
| { | ||||||
| "type": "channel_delivery_result", | ||||||
| "promptId": "prompt-1", | ||||||
| "data": { | ||||||
| "sessionId": "session-1", | ||||||
| "deliveryId": "prompt-1", | ||||||
| "source": "prompt", | ||||||
| "status": "skipped", | ||||||
| "promptId": "prompt-1" | ||||||
| } | ||||||
| } | ||||||
| ``` | ||||||
|
|
||||||
| `source` is `prompt` or `scheduled`; `status` is `delivered`, `failed`, or | ||||||
| `skipped`. `skipped` means the eligible turn completed successfully but its | ||||||
| last tool-free assistant response block was empty or whitespace-only. The | ||||||
| daemon consumes the delivery authorization and publishes the event without | ||||||
| resolving a Channel Worker. Scheduled correlation uses `taskId` and `firedAt`. | ||||||
| The event never contains target IDs, message text, credentials, or webhook | ||||||
| secrets. | ||||||
|
|
||||||
| Security: the response never includes bearer tokens, client ids, full ACP | ||||||
| connection ids, device-flow user codes, or verification URLs. Both detail | ||||||
| levels may include additive `daemon.runId`, `daemon.logMode`, and | ||||||
|
|
@@ -2118,33 +2201,49 @@ Request: | |||||
|
|
||||||
| ```json | ||||||
| { | ||||||
| "prompt": [{ "type": "text", "text": "What does src/main.ts do?" }] | ||||||
| "prompt": [{ "type": "text", "text": "What does src/main.ts do?" }], | ||||||
| "delivery": { | ||||||
| "kind": "channel", | ||||||
| "target": { | ||||||
| "channelName": "dingtalk", | ||||||
| "type": "user", | ||||||
| "id": "platform-user-id" | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| ``` | ||||||
|
|
||||||
| `delivery` is optional and requires the `channel_delivery` capability. The | ||||||
| daemon still returns `202 {promptId,lastEventId}` when the prompt is admitted. | ||||||
| After a successful `end_turn`, the session submits the visible final text to | ||||||
| the exact workspace's already-running Channel Worker. The payload is only the | ||||||
| last tool-free assistant response block; tool-call preambles, inter-tool | ||||||
| narration, superseded retries, and earlier automatic-continuation blocks are | ||||||
| excluded. An empty or whitespace-only final still produces a correlated | ||||||
| `channel_delivery_result` with `status: "skipped"` after authorization is | ||||||
| consumed, but it does not contact a worker. Delivery success or failure arrives | ||||||
| later through the same replayable event and never changes `turn_complete` into | ||||||
| `turn_error`. Cancellation, Agent failure, and token-limit termination do not | ||||||
| send or publish a delivery result. | ||||||
|
|
||||||
| Validation: `prompt` must be a non-empty array of objects. Other failures return `400` before reaching the bridge. | ||||||
|
|
||||||
| Response: | ||||||
|
|
||||||
| ```json | ||||||
| { "stopReason": "end_turn" } | ||||||
| { "promptId": "session-id########1", "lastEventId": 42 } | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The 202 response example omits the The implementation at res.status(202).json({ promptId, lastEventId, eventEpoch });but this example and the prose description (
Suggested change
— qwen3.7-max via Qwen Code /review |
||||||
| ``` | ||||||
|
|
||||||
| Other stop reasons: `cancelled`, `max_tokens`, `error`, `length` (per ACP spec). | ||||||
| The `202` response acknowledges admission, not Agent completion. Observe the | ||||||
| session SSE stream after `lastEventId` and correlate `turn_complete` or | ||||||
| `turn_error` by `promptId`. `turn_complete.data.stopReason` may be `end_turn`, | ||||||
| `cancelled`, `max_tokens`, `error`, or `length`. | ||||||
|
|
||||||
| If the HTTP client disconnects mid-prompt, the daemon sends an ACP `cancel` notification to the agent, which winds the prompt down with `stopReason: "cancelled"`. | ||||||
|
|
||||||
| > **Stage 1 limitation — no server-side prompt timeout.** The bridge | ||||||
| > only races the agent's `prompt()` against `transportClosedReject` | ||||||
| > (the agent child crashing) and the caller's HTTP-disconnect | ||||||
| > AbortSignal. A wedged-but-alive agent (e.g. a model call that | ||||||
| > hangs) blocks the per-session FIFO until the HTTP client times out | ||||||
| > on its end and disconnects. Long-running prompts are legitimate | ||||||
| > (deep research, large-codebase analysis) so a default deadline is | ||||||
| > deliberately not set; Stage 2 will expose a configurable | ||||||
| > `promptTimeoutMs` opt-in. Until then, callers should set their own | ||||||
| > client-side timeout and disconnect (or call | ||||||
| > `POST /session/:id/cancel`) on expiry. | ||||||
| When `prompt_absolute_deadline` is advertised, `deadlineMs` may shorten the | ||||||
| configured server deadline. Expiry emits a correlated `turn_error` with | ||||||
| `errorKind: "prompt_deadline_exceeded"`. | ||||||
|
|
||||||
| ### `POST /session/:id/cancel` | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,132 @@ | ||
| # Channel Delivery Review Fixes Implementation Plan | ||
|
|
||
| > **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. | ||
|
|
||
| **Goal:** Close the actionable review findings on PR #7388 without adding durable delivery retries or allowing delivery diagnostics to affect Agent execution. | ||
|
|
||
| **Architecture:** Keep immediate delivery semantics. Add short-lived daemon-owned authorization for prompt and scheduled targets, preserve producer-specific execution, and keep logging as a best-effort observation side effect. Reuse the existing Channel worker IPC and public result event. | ||
|
|
||
| **Tech Stack:** TypeScript, ACP bridge, Express daemon routes, Channel worker IPC, Vitest. | ||
|
|
||
| ## Global Constraints | ||
|
|
||
| - Delivery remains immediate and best-effort; no persistent outbox or retry queue. | ||
| - A delivery failure or a logging failure must not reject or delay the completed Prompt or scheduled Agent turn. | ||
| - Logs and events must not contain message text, target IDs, or credentials. | ||
| - Existing calls without `delivery` remain unchanged. | ||
|
|
||
| --- | ||
|
|
||
| ### Task 1: Restore CI capability coverage | ||
|
|
||
| **Files:** | ||
|
|
||
| - Modify: `integration-tests/cli/qwen-serve-routes.test.ts` | ||
|
|
||
| **Interfaces:** | ||
|
|
||
| - Consumes: `GET /capabilities` feature list. | ||
| - Produces: exact integration expectation including `channel_delivery`. | ||
|
|
||
| - [x] Add `channel_delivery` to the existing exact capability expectation. | ||
| - [x] Run the affected bundled-daemon integration file and verify all 35 cases pass. | ||
|
|
||
| ### Task 2: Pin daemon-authorized delivery targets | ||
|
|
||
| **Files:** | ||
|
|
||
| - Create: `packages/cli/src/serve/channel-delivery-authorization.ts` | ||
| - Create: `packages/cli/src/serve/channel-delivery-authorization.test.ts` | ||
| - Modify: `packages/cli/src/serve/routes/session.ts` | ||
| - Modify: `packages/cli/src/serve/routes/scheduled-tasks.ts` | ||
| - Modify: `packages/cli/src/serve/scheduled-task-keepalive.ts` | ||
| - Modify: `packages/cli/src/serve/run-qwen-serve.ts` | ||
| - Modify: `packages/cli/src/serve/server.ts` | ||
| - Modify affected route, keepalive, and server tests. | ||
|
|
||
| **Interfaces:** | ||
|
|
||
| - Consumes: daemon-admitted Prompt `{sessionId, promptId, target}` and persisted scheduled task `{id, sessionId, recurring, lastFiredAt, delivery.target}`. | ||
| - Produces: one-shot `authorizePrompt`, scheduled `registerTask`, `revokeTask`, and callback `consume` checks that deep-compare the target before worker IPC. | ||
|
|
||
| - [x] Write failing tests proving an unregistered callback, changed target, repeated Prompt callback, and invalid/replayed scheduled fire are rejected before worker IPC. | ||
| - [x] Run the focused tests and confirm the expected authorization failures. | ||
| - [x] Implement an in-memory authorization store with Prompt consume-once behavior and scheduled monotonic-fire/one-shot behavior. | ||
| - [x] Wire Prompt admission, scheduled CRUD/rehydration, and the bound delivery handler to the store. | ||
| - [x] Run the focused tests and verify authorized Prompt, recurring, and one-shot deliveries still pass. | ||
|
|
||
| ### Task 3: Preserve final-answer semantics across retries and failures | ||
|
|
||
| **Files:** | ||
|
|
||
| - Modify: `packages/cli/src/acp-integration/session/Session.ts` | ||
| - Modify: `packages/cli/src/acp-integration/session/Session.test.ts` | ||
| - Modify: `packages/cli/src/serve/run-qwen-serve.ts` | ||
| - Modify: `packages/cli/src/serve/run-qwen-serve.test.ts` | ||
|
|
||
| **Interfaces:** | ||
|
|
||
| - Consumes: Prompt and cron stream events plus resolved `channel_delivery_result` values. | ||
| - Produces: retry-safe final text, no delivery for non-`end_turn`/aborted turns, and best-effort sanitized daemon warning logs for non-delivered results. | ||
|
|
||
| - [x] Add failing Prompt and cron tests for retry collector rollback and cancelled/error non-delivery. | ||
| - [x] Add tests where delivery resolves `failed` or daemon logger output throws while the Agent turn remains normal. | ||
| - [x] Run focused Session tests and confirm each regression test fails for the intended reason. | ||
| - [x] Trim the collector at retry boundaries without awaiting delivery from the Agent turn. | ||
| - [x] Route failure diagnostics through a non-throwing sanitized daemon helper. | ||
| - [x] Run focused Session and daemon-handler tests and verify all cases pass. | ||
|
|
||
| ### Task 4: Make worker shutdown and errors deterministic | ||
|
|
||
| **Files:** | ||
|
|
||
| - Modify: `packages/cli/src/commands/channel/daemon-worker.ts` | ||
| - Modify: `packages/cli/src/commands/channel/daemon-worker.test.ts` | ||
| - Modify: `packages/cli/src/serve/channel-delivery-ipc.ts` | ||
| - Modify: `packages/cli/src/serve/routes/channel-notify.ts` | ||
| - Modify corresponding ACP bridge and SDK event types/tests. | ||
|
|
||
| **Interfaces:** | ||
|
|
||
| - Consumes: active webhook/delivery promises and Channel adapter failures. | ||
| - Produces: one shared 10-second drain budget, typed unavailable errors, and a distinct provider-rejection code. | ||
|
|
||
| - [x] Add failing tests for concurrent drain, typed unavailable classification, and provider rejection not mapping to HTTP 400. | ||
| - [x] Run focused worker/route tests and confirm the expected failures. | ||
| - [x] Drain both active maps concurrently, throw/classify typed errors, and add `channel_delivery_rejected` across the wire contract. | ||
| - [x] Run focused worker, route, bridge, and SDK tests. | ||
|
|
||
| ### Task 5: Remove local contract drift and repair Unicode coverage | ||
|
|
||
| **Files:** | ||
|
|
||
| - Modify: `packages/cli/src/serve/channel-delivery.ts` | ||
| - Modify: `packages/cli/src/serve/channel-delivery.test.ts` | ||
| - Modify: `packages/cli/src/acp-integration/session/Session.ts` | ||
|
|
||
| **Interfaces:** | ||
|
|
||
| - Consumes: arbitrary delivery text. | ||
| - Produces: one CLI truncation helper with surrogate-safe 100,000-code-unit output. | ||
|
|
||
| - [x] Correct the surrogate-boundary test so deleting the protection makes it fail. | ||
| - [x] Export and reuse the CLI text normalizer from Session. | ||
| - [x] Run both focused test files. | ||
|
|
||
| ### Task 6: Full verification and PR update | ||
|
|
||
| **Files:** | ||
|
|
||
| - Inspect every changed and untracked file. | ||
|
|
||
| **Interfaces:** | ||
|
|
||
| - Consumes: completed implementation. | ||
| - Produces: verified branch suitable for maintainer rereview. | ||
|
|
||
| - [x] Run focused tests for every touched package. | ||
| - [x] Run `npm run build`, `npm run typecheck`, and `npm run lint`. | ||
| - [x] Run the bundled-daemon route integration test that failed in CI. | ||
| - [ ] Run real IM E2E for notify, Prompt final, scheduled final, and provider rejection using redacted credentials. | ||
| - [x] Perform two clean diff self-audit passes. | ||
| - [ ] Commit and deliver the fixes to the maintainer-selected branch without resolving or replying to review threads. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion]
promptIdappears at both the top-level SSE envelope and insidedatain both channel_delivery_result event examples, with no explanation of whether the two values are always identical or can differ. — Concrete cost: a client implementer cannot determine which to use for correlation. If they assume the two can differ and build separate handling, they add unnecessary complexity.Consider adding a one-line note explaining the duplication (e.g., "The top-level
promptIdis the SSE correlation key;data.promptIdis always identical for prompt-sourced events") or removing the innerpromptIdfromdataif redundant.— qwen3.7-max via Qwen Code /review