feat: orchestration improvements

.compozy/tasks/orch-improvs/reviews-001/issue_001.md

@@ -0,0 +1,65 @@
+---
+provider: coderabbit
+pr: "106"
+round: 1
+round_created_at: 2026-05-06T04:12:39.763475Z
+status: resolved
+file: internal/api/core/bridges.go
+line: 403
+author: coderabbitai[bot]
+provider_ref: thread:PRRT_kwDOR5y4QM5_2Lrj,comment:PRRC_kwDOR5y4QM6-UJdl
+---
+
+# Issue 001: _⚠️ Potential issue_ | _🔴 Critical_ | _⚡ Quick win_
+## Review Comment
+
+_⚠️ Potential issue_ | _🔴 Critical_ | _⚡ Quick win_
+
+**Reject foreign subscription IDs and bridge targets on create.**
+
+This path authorizes only the task, then accepts arbitrary `subscription_id`/scope fields and persists through an upsert keyed on `subscription_id`. A caller who knows another subscription ID can overwrite that record onto a different task, and there is also no check that the chosen bridge instance belongs to the same workspace/scope as the task.
+
+Generate IDs server-side for create, reject existing IDs that are already bound to another task, and verify the bridge instance’s scope/workspace matches the task before calling the store.
+
+
+
+Also applies to: 661-694
+
+<details>
+<summary>🤖 Prompt for AI Agents</summary>
+
+```
+Verify each finding against current code. Fix only still-valid issues, skip the
+rest with a brief reason, keep changes minimal, and validate.
+
+In `@internal/api/core/bridges.go` around lines 352 - 403, The
+CreateTaskBridgeNotificationSubscription handler currently trusts
+client-supplied subscription IDs and bridge instance selection, allowing ID
+collision/overwrite and cross-workspace binding; fix it by (1) generating a new
+subscription ID server-side instead of accepting req.SubscriptionID (update
+taskBridgeNotificationSubscriptionFromRequest to allow nil/empty incoming ID and
+return/accept a generated ID), (2) before PutBridgeTaskSubscription, check that
+the existing subscription (if any) with that ID does not belong to a different
+task and reject the request if it does, and (3) validate that the bridge
+instance returned by bridges.GetInstance belongs to the same workspace/scope as
+the authorized task (use the task/actor or manager scope from
+authorizeTaskBridgeNotification) and reject mismatched scope with an error;
+apply the same changes to the equivalent create path referenced in the second
+location.
+```
+
+</details>
+
+<!-- fingerprinting:phantom:medusa:grasshopper -->
+
+<!-- This is an auto-generated comment by CodeRabbit -->
+
+## Triage
+
+- Decision: `valid`
+- Notes:
+  - `CreateTaskBridgeNotificationSubscription` still accepts client-controlled `subscription_id` and persists the normalized subscription through an upsert keyed by that ID.
+  - The handler authorizes only task existence, then checks bridge existence without verifying the bridge instance scope/workspace against the authorized task.
+  - Root cause: create-time ownership and scope invariants are enforced too late or not at all; the API trusts transport fields that must remain daemon-owned.
+  - Planned fix: make create path generate daemon-owned subscription IDs, reject/ignore client-owned IDs on create, and validate the selected bridge instance against the authorized task scope/workspace before persistence.
+  - Resolved: create now generates server-owned subscription IDs, persists authoritative task scope/workspace, validates the bridge instance against the authorized task, and regression tests cover the create/list/get/delete flow.

.compozy/tasks/orch-improvs/reviews-001/issue_002.md

@@ -0,0 +1,31 @@
+---
+provider: coderabbit
+pr: "106"
+round: 1
+round_created_at: 2026-05-06T04:12:39.763475Z
+status: resolved
+file: internal/api/core/tasks.go
+line: 337
+severity: major
+author: coderabbitai[bot]
+provider_ref: review:4233115469,nitpick_hash:04fe79bf2745
+review_hash: 04fe79bf2745
+source_review_id: "4233115469"
+source_review_submitted_at: "2026-05-06T04:12:03Z"
+---
+
+# Issue 002: Strip server-managed fields from execution-profile writes.
+## Review Comment
+
+This endpoint binds directly into `taskpkg.ExecutionProfile`, and the helper only normalizes `TaskID`. The manager later preserves a non-zero `CreatedAt`, so callers can backdate or otherwise forge immutable profile metadata through the public API. Please switch to a dedicated request DTO, or at minimum clear the server-owned fields here before handing the object off.
+
+Also applies to: 1537-1555
+
+## Triage
+
+- Decision: `valid`
+- Notes:
+  - `SetTaskExecutionProfileRequest` is still an alias of `task.ExecutionProfile`, so the request shape includes `created_at` and `updated_at`.
+  - `taskExecutionProfileFromRequest` only normalizes `task_id` and returns the request object directly, leaving server-managed timestamps writable from the public API surface.
+  - Planned fix: scrub server-managed profile fields at the handler boundary before calling task service authority, and cover the behavior with an API regression test.
+  - Resolved: the write path now copies request data into a fresh profile, overwrites `TaskID` from the URL path, clears `CreatedAt`/`UpdatedAt`, and API tests assert the sanitized payload.

.compozy/tasks/orch-improvs/reviews-001/issue_003.md

@@ -0,0 +1,33 @@
+---
+provider: coderabbit
+pr: "106"
+round: 1
+round_created_at: 2026-05-06T04:12:39.763475Z
+status: resolved
+file: internal/api/core/tasks_test.go
+line: 247
+severity: minor
+author: coderabbitai[bot]
+provider_ref: review:4233115469,nitpick_hash:2768ae3a6256
+review_hash: 2768ae3a6256
+source_review_id: "4233115469"
+source_review_submitted_at: "2026-05-06T04:12:03Z"
+---
+
+# Issue 003: Assert response bodies on the new negative and 204 cases.
+## Review Comment
+
+These additions introduce several status-only checks. That leaves the response contract untested on the paths most likely to drift — especially the 400/404 JSON payloads and the 204 empty-body cases. Decode `contract.ErrorPayload` on the error branches and assert an empty body for the `204` responses.
+
+As per coding guidelines "Always assert both HTTP status code AND response body in tests; status-code-only assertions are insufficient".
+
+Also applies to: 447-512, 763-780
+
+## Triage
+
+- Decision: `valid`
+- Notes:
+  - The new handler tests still contain status-only assertions on error and `204 No Content` paths.
+  - That leaves JSON error payloads and empty-body guarantees unverified, which violates the repo’s HTTP test contract.
+  - Planned fix: decode `contract.ErrorPayload` on the negative branches and assert an empty body on the `204` responses.
+  - Resolved: `tasks_test.go` now asserts empty `204` bodies, concrete bad-request and not-found payloads, and the dynamic subscription IDs emitted by the server-owned create flow.

.compozy/tasks/orch-improvs/reviews-001/issue_004.md

@@ -0,0 +1,29 @@
+---
+provider: coderabbit
+pr: "106"
+round: 1
+round_created_at: 2026-05-06T04:12:39.763475Z
+status: resolved
+file: internal/api/spec/spec.go
+line: 4469
+severity: major
+author: coderabbitai[bot]
+provider_ref: review:4233115469,nitpick_hash:24191b22804b
+review_hash: 24191b22804b
+source_review_id: "4233115469"
+source_review_submitted_at: "2026-05-06T04:12:03Z"
+---
+
+# Issue 004: Document the default review-policy enum value too.
+## Review Comment
+
+The schema migration in this PR defaults `tasks.review_policy` to `"none"`, but this enum only advertises the routed policies. Clients generated from the OpenAPI document will reject the persisted default, and the spec no longer round-trips existing task data correctly.
+
+## Triage
+
+- Decision: `valid`
+- Notes:
+  - `taskReviewPolicyValues()` still omits `task.ReviewPolicyNone` even though the task model and config defaults support `"none"`.
+  - That makes the OpenAPI enum stricter than persisted/runtime data and can break generated clients on round-trips.
+  - Planned fix: include `"none"` in the spec enum values so the contract matches runtime truth.
+  - Resolved: the spec helper now includes `taskpkg.ReviewPolicyNone`, and generated OpenAPI/TypeScript artifacts were refreshed via codegen.

.compozy/tasks/orch-improvs/reviews-001/issue_005.md

@@ -0,0 +1,29 @@
+---
+provider: coderabbit
+pr: "106"
+round: 1
+round_created_at: 2026-05-06T04:12:39.763475Z
+status: resolved
+file: internal/bridges/task_notifier.go
+line: 183
+severity: major
+author: coderabbitai[bot]
+provider_ref: review:4233115469,nitpick_hash:2cdee8d9dce2
+review_hash: 2cdee8d9dce2
+source_review_id: "4233115469"
+source_review_submitted_at: "2026-05-06T04:12:03Z"
+---
+
+# Issue 005: Page past ignored records or subscriptions can get stuck forever.
+## Review Comment
+
+`listTaskNotificationRecords` is capped by `eventLimit`, but the cursor only advances after a delivery. If the first page contains only non-terminal events, or only superseded mismatches before the real terminal event, every sweep re-reads the same prefix and never reaches the later final record. This needs paging/progress semantics that can move past records which are no longer candidates.
+
+## Triage
+
+- Decision: `valid`
+- Notes:
+  - `deliverSubscription` only advances the cursor after a delivery. If a full page contains only ignored/non-terminal records or only superseded mismatches, the cursor never moves and later terminal records stay unreachable.
+  - Existing tests cover later valid events inside the same page, but not pagination starvation across `eventLimit` boundaries.
+  - Planned fix: add safe progress semantics for scanned records that can never become deliverable and add a regression test for page starvation.
+  - Resolved: the notifier now advances the durable cursor to the highest safely scanned sequence, including ignored and mismatched records, and regression tests cover the starvation case across pages.

.compozy/tasks/orch-improvs/reviews-001/issue_006.md

@@ -0,0 +1,31 @@
+---
+provider: coderabbit
+pr: "106"
+round: 1
+round_created_at: 2026-05-06T04:12:39.763475Z
+status: resolved
+file: internal/bridges/task_notifier.go
+line: 349
+severity: major
+author: coderabbitai[bot]
+provider_ref: review:4233115469,nitpick_hash:f5af1cc8e237
+review_hash: f5af1cc8e237
+source_review_id: "4233115469"
+source_review_submitted_at: "2026-05-06T04:12:03Z"
+---
+
+# Issue 006: Redact task errors before building the bridge notification.
+## Review Comment
+
+`run.Error` is copied verbatim into `TerminalTaskNotification.Error`, and that field is then sent in both `ProviderMetadata` and the rendered message text. Any raw claim token in the task error can leak over the bridge transport from this path.
+
+As per coding guidelines, "Raw `claim_token` (`agh_claim_*`) ... MUST NEVER appear in logs, status APIs, settings views, error payloads, channel messages, SSE, web UI, or memory — use hash forms (`claim_token_hash`) over the wire".
+
+## Triage
+
+- Decision: `valid`
+- Notes:
+  - `resolveTerminalTaskNotification` still copies `run.Error` verbatim into `TerminalTaskNotification.Error`.
+  - That value is later sent over bridge delivery metadata/rendered text, so raw claim tokens can escape through bridge transports.
+  - Planned fix: redact claim tokens at notification construction time and cover the replay path with a notifier regression test.
+  - Resolved: terminal bridge notifications now redact claim tokens with `taskpkg.RedactClaimTokens`, and a regression test asserts the delivered payload no longer leaks the raw claim token.

.compozy/tasks/orch-improvs/reviews-001/issue_007.md

@@ -0,0 +1,29 @@
+---
+provider: coderabbit
+pr: "106"
+round: 1
+round_created_at: 2026-05-06T04:12:39.763475Z
+status: resolved
+file: internal/cli/client.go
+line: 2630
+severity: major
+author: coderabbitai[bot]
+provider_ref: review:4233115469,nitpick_hash:bf64d7db5ca4
+review_hash: bf64d7db5ca4
+source_review_id: "4233115469"
+source_review_submitted_at: "2026-05-06T04:12:03Z"
+---
+
+# Issue 007: Add nil checks for pointer request parameters before marshaling.
+## Review Comment
+
+Lines 2633, 2654, 2711, and 2751 accept `*...Request` pointer parameters and pass them through `doJSON`, which marshals using `interface{}`. In Go, a typed nil pointer stored in an interface{} compares non-nil, so `json.Marshal` will encode it as JSON `null` instead of rejecting it at the client. Add a nil check at the entry of each method to fail fast.
+
+## Triage
+
+- Decision: `valid`
+- Notes:
+  - The client methods still accept pointer request arguments and pass them straight into `doJSON`.
+  - A typed nil pointer inside `interface{}` marshals as JSON `null`, so the client currently converts obvious caller mistakes into backend requests instead of failing locally.
+  - Planned fix: add explicit nil checks to the affected client entry points and exercise them with focused client/CLI tests.
+  - Resolved: the affected client helpers now reject nil pointer requests with explicit errors, and client tests cover all guarded methods.

.compozy/tasks/orch-improvs/reviews-001/issue_008.md

@@ -0,0 +1,29 @@
+---
+provider: coderabbit
+pr: "106"
+round: 1
+round_created_at: 2026-05-06T04:12:39.763475Z
+status: resolved
+file: internal/cli/task.go
+line: 658
+severity: minor
+author: coderabbitai[bot]
+provider_ref: review:4233115469,nitpick_hash:9d8d8fcc1d42
+review_hash: 9d8d8fcc1d42
+source_review_id: "4233115469"
+source_review_submitted_at: "2026-05-06T04:12:03Z"
+---
+
+# Issue 008: Validate --last for notification listing.
+## Review Comment
+
+This builder forwards negative limits to the API, while the other list commands in this file reject them locally. `agh task notification list --last -1` should fail fast in the CLI instead of producing downstream behavior that depends on the server implementation.
+
+## Triage
+
+- Decision: `valid`
+- Notes:
+  - `buildTaskBridgeNotificationSubscriptionListQuery` still forwards `last` directly into the query without the negative-value guard used by sibling list builders.
+  - That pushes an obvious CLI validation error downstream into server behavior.
+  - Planned fix: reject negative `--last` values in the CLI builder and cover the contract with command tests.
+  - Resolved: the task notification list builder now rejects negative `--last`, and CLI tests assert the validation error.

.compozy/tasks/orch-improvs/reviews-001/issue_009.md

@@ -0,0 +1,29 @@
+---
+provider: coderabbit
+pr: "106"
+round: 1
+round_created_at: 2026-05-06T04:12:39.763475Z
+status: resolved
+file: internal/cli/task.go
+line: 824
+severity: minor
+author: coderabbitai[bot]
+provider_ref: review:4233115469,nitpick_hash:3535b363925f
+review_hash: 3535b363925f
+source_review_id: "4233115469"
+source_review_submitted_at: "2026-05-06T04:12:03Z"
+---
+
+# Issue 009: Reject negative review rounds and attempts in the CLI.
+## Review Comment
+
+`buildTaskRunReviewRequest` currently passes negative `--round` and `--attempt` values through unchanged. That turns obvious caller mistakes into server-side validation failures instead of immediate CLI feedback.
+
+## Triage
+
+- Decision: `valid`
+- Notes:
+  - `buildTaskRunReviewRequest` still forwards negative `round` and `attempt` values unchanged.
+  - That converts deterministic caller input errors into server-side validation failures.
+  - Planned fix: fail fast in the CLI builder for negative review round/attempt values and add regression coverage.
+  - Resolved: the review-request builder now rejects negative round and attempt values, and CLI tests cover both invalid flags.

.compozy/tasks/orch-improvs/reviews-001/issue_010.md

@@ -0,0 +1,31 @@
+---
+provider: coderabbit
+pr: "106"
+round: 1
+round_created_at: 2026-05-06T04:12:39.763475Z
+status: resolved
+file: internal/daemon/native_profile_tools.go
+line: 66
+severity: major
+author: coderabbitai[bot]
+provider_ref: review:4233115469,nitpick_hash:b5d79ae0935c
+review_hash: b5d79ae0935c
+source_review_id: "4233115469"
+source_review_submitted_at: "2026-05-06T04:12:03Z"
+---
+
+# Issue 010: Reject conflicting task_id values before persisting the profile.
+## Review Comment
+
+`taskExecutionProfileSet()` keys the write by the top-level `task_id`, but `profile()` will prefer `profile.task_id` when it is set. A caller can submit two different IDs here and produce a stored payload that no longer matches the record being updated.
+
+Also applies to: 111-123
+
+## Triage
+
+- Decision: `valid`
+- Notes:
+  - `taskExecutionProfileSet()` keys writes by top-level `task_id`, but `taskExecutionProfileInput.profile()` still prefers an embedded `profile.task_id` when present.
+  - That allows a mismatched payload to persist a profile whose body no longer matches the addressed task record.
+  - Planned fix: reject conflicting `task_id` values before materializing the profile payload and add a native-tool regression test.
+  - Resolved: the native profile tool now rejects conflicting top-level and nested task IDs before persistence, returns a schema-invalid tool error, and tests confirm no write occurs.

.compozy/tasks/orch-improvs/reviews-001/issue_011.md

@@ -0,0 +1,31 @@
+---
+provider: coderabbit
+pr: "106"
+round: 1
+round_created_at: 2026-05-06T04:12:39.763475Z
+status: resolved
+file: internal/daemon/native_review_tools.go
+line: 348
+severity: major
+author: coderabbitai[bot]
+provider_ref: review:4233115469,nitpick_hash:a57799fce42c
+review_hash: a57799fce42c
+source_review_id: "4233115469"
+source_review_submitted_at: "2026-05-06T04:12:03Z"
+---
+
+# Issue 011: Map unbound-review failures to denied, and keep the fallback redacted.
+## Review Comment
+
+`LookupRunReviewForSession()` misses satisfy `ErrRunReviewNotFound`, so this switch returns 404 before `isReviewBindingError()` can translate them to `ReasonSessionDenied`. The `default` branch then returns the original error unchanged, which bypasses the redaction already computed in `message`. Missing reviewer bindings should be denied, and unmapped task errors should still surface through a redacted tool error.
+
+As per coding guidelines, "Raw `claim_token` (`agh_claim_*`) ... MUST NEVER appear in ... error payloads".
+
+## Triage
+
+- Decision: `valid`
+- Notes:
+  - `nativeReviewToolError` still matches `ErrRunReviewNotFound` as `not_found` before the binding-denial check, so unbound reviewer lookups do not map to `ReasonSessionDenied`.
+  - The `default` branch returns the raw error, which bypasses the already-computed redacted message and can leak claim tokens.
+  - Planned fix: reorder the mapping so binding failures become denied and wrap unmapped review errors in a redacted tool error envelope instead of returning the raw error.
+  - Resolved: review binding errors now map to denied/session-denied before not-found handling, and the fallback path wraps backend failures in a redacted tool error instead of returning raw errors.