fix: network optimizations - #263

web/src/routes/_app/settings/network.tsx-383-397 (1)

383-397: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Add inline validation for Digest flush interval web/src/routes/_app/settings/network.tsx:383-397

This is the only new delivery field that skips the validation/error pattern used by the sibling inputs. It accepts any string until save, so invalid durations surface only as a generic save failure instead of inline feedback.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web/src/routes/_app/settings/network.tsx` around lines 383 - 397, The Digest
flush interval input in the network settings form is missing the same inline
validation/error handling used by the sibling delivery fields. Update the Input
and its surrounding state logic in the settings page component to validate the
value as the user types, surface the field-specific error inline, and keep the
existing save flow unchanged; use the same pattern already implemented for the
other delivery interval inputs in this form.

Source: Coding guidelines

internal/tools/builtin/tasks.go-539-551 (1)

539-551: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Reject empty fan-out designation payloads at the tool schema boundary.

designations: [] and {"brief": ""} currently pass this schema, so tool callers can make avoidable mutating requests that only fail deeper in the task service.

Proposed schema tightening

 		"designations":{
 			"type":"array",
+			"minItems":1,
 			"items":{
 				"type":"object",
 				"required":["brief"],
 				"properties":{
-					"brief":{"type":"string"},
+					"brief":{"type":"string","minLength":1},
 					"metadata":{},
 					"idempotency_key":{"type":"string"}
 				},

🤖 Prompt for AI Agents

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/tools/builtin/tasks.go` around lines 539 - 551, The task tool schema
for designations is too permissive because the `designations` array in the
schema built in `internal/tools/builtin/tasks.go` allows empty fan-out payloads
and empty `brief` values. Tighten the `designations` item schema used by the
task tool so callers cannot submit `designations: []` or a designation with an
empty `brief`, and keep the change localized to the schema definition in the
task tool setup around the `designations` property.

web/src/systems/network/components/shell/channel-header.tsx-206-220 (1)

206-220: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Unhandled rejection propagates from updateChannel.mutateAsync.

mutateAsync rejects on failure; this onSubmit doesn't catch it, so the rejection bubbles into ChannelPolicyDialog's handleSubmit. See the companion comment in channel-policy-dialog.tsx for the root-cause fix.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web/src/systems/network/components/shell/channel-header.tsx` around lines 206
- 220, The ChannelPolicyDialog onSubmit handler is letting the rejection from
updateChannel.mutateAsync escape, which bubbles into the dialog submit flow.
Update the onSubmit callback in channel-header.tsx to handle the async failure
explicitly by catching the mutation error (or otherwise swallowing/forwarding it
safely) around updateChannel.mutateAsync, so ChannelPolicyDialog’s handleSubmit
does not receive an unhandled rejection.

web/src/systems/network/components/shell/channel-policy-dialog.tsx-111-120 (1)

111-120: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Wrap onSubmit in try/catch to avoid an unhandled rejection on failure.

onSubmit ultimately calls mutateAsync, which rejects on error. Since handleSubmit isn't awaited by React's form onSubmit, a failed submission becomes an unhandled promise rejection, and onOpenChange(false) is skipped by the throw rather than by an explicit decision.

🛡️ Proposed fix

   const handleSubmit = async (event: FormEvent<HTMLFormElement>) => {
     event.preventDefault();
     const payload: NetworkChannelUpdateRequest = {
       purpose: purpose.trim(),
       fanout_policy: policy,
       coordinator_peer_id: coordinatorPeerId.trim(),
     };
-    await onSubmit(payload);
-    onOpenChange(false);
+    try {
+      await onSubmit(payload);
+      onOpenChange(false);
+    } catch {
+      // Failure is surfaced via toast in the mutation's onError handler;
+      // keep the dialog open so the user can retry.
+    }
   };

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web/src/systems/network/components/shell/channel-policy-dialog.tsx` around
lines 111 - 120, Wrap the ChannelPolicyDialog submit flow in a try/catch so a
failed async submit does not become an unhandled rejection. Update handleSubmit
to call onSubmit(payload) inside try/catch, and only call onOpenChange(false)
after a successful submit; in the catch, handle or surface the error explicitly
and avoid closing the dialog on failure. Use the handleSubmit and onSubmit
symbols in channel-policy-dialog.tsx to locate the change.

web/src/systems/network/components/thread-overlay/thread-subscription-control.tsx-56-70 (1)

56-70: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Error state not surfaced — dropdown falls back to "Default" on fetch failure.

When subscriptions query fails, current becomes null and the label shows "Default" even if a subscription exists, while the trigger stays interactive. A user could then unintentionally reset/overwrite an existing subscription during a transient fetch error.

🛠️ Suggested fix

-  const isBusy = upsert.isPending || remove.isPending || subscriptions.isFetching;
+  const isBusy = upsert.isPending || remove.isPending || subscriptions.isFetching;
+  const hasError = subscriptions.isError;
   const label = modeLabel(current?.mode);

and disable the trigger / show an indicator when hasError is true instead of silently defaulting to "Default".

As per path instructions, "Handle all states — loading, error, and empty (never assume data exists)".

Also applies to: 93-114

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@web/src/systems/network/components/thread-overlay/thread-subscription-control.tsx`
around lines 56 - 70, The thread subscription dropdown is treating a failed
`useQuery(networkSubscriptionsOptions(...))` the same as an empty result, so
`current` becomes null and the control silently falls back to “Default”. Update
`thread-subscription-control.tsx` in the `subscriptions`/`current`/`label` flow
to explicitly detect the query error state from `useQuery`, surface an error
indicator or message, and disable the trigger while `hasError` is true so the
user cannot overwrite an existing subscription during a transient failure. Also
handle loading and empty states separately without assuming `subscriptions.data`
exists, and apply the same state handling to the related trigger/overlay logic
around the referenced subscription controls.

Source: Path instructions

internal/api/spec/spec.go-1630-1649 (1)

1630-1649: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Add the limit query parameter to listNetworkSubscriptions.
parsePositiveIntQuery reads limit, so this endpoint’s spec is missing a supported pagination/filter parameter and generated clients won’t expose it.

Suggested fix

 		Parameters: []ParameterSpec{
 			pathParam("workspace_id", "Workspace id"),
 			pathParam("channel", "Network channel"),
 			queryParam("peer_id", "Filter subscriptions by peer id", false),
 			queryParam("thread_id", "Filter subscriptions by thread id", false),
+			intQueryParam("limit", "Maximum number of subscriptions to return"),
 		},

🤖 Prompt for AI Agents

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/spec/spec.go` around lines 1630 - 1649, The
listNetworkSubscriptions endpoint spec is missing the supported limit query
parameter, so add it alongside peer_id and thread_id in the Parameters list for
the network subscriptions operation. Use the existing queryParam helper in
spec.go so generated clients expose the same pagination/filter input that
parsePositiveIntQuery already accepts, keeping the spec aligned with the handler
and contract.

internal/api/core/network_conversations.go-891-899 (1)

891-899: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Omit empty coordination-cost payloads.

CoordinationCost is optional, but this converter always returns a non-nil pointer; zero-cost threads serialize as coordination_cost: {} instead of omitting the field.

Proposed fix

 func NetworkCoordinationCostPayloadFromStore(
 	thread store.NetworkThreadSummary,
 ) *contract.NetworkCoordinationCostPayload {
+	if thread.DeliveredCount == 0 &&
+		thread.PromptSizeBytes == 0 &&
+		thread.EstimatedPromptTokens == 0 {
+		return nil
+	}
 	return &contract.NetworkCoordinationCostPayload{
 		DeliveredCount:        thread.DeliveredCount,
 		PromptSizeBytes:       thread.PromptSizeBytes,
 		EstimatedPromptTokens: thread.EstimatedPromptTokens,
 	}
 }

🤖 Prompt for AI Agents

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/network_conversations.go` around lines 891 - 899,
NetworkCoordinationCostPayloadFromStore always returns a non-nil payload, which
causes empty coordination_cost values to be serialized instead of omitted.
Update NetworkCoordinationCostPayloadFromStore to detect an all-zero
NetworkThreadSummary cost and return nil in that case, and only build a
contract.NetworkCoordinationCostPayload when at least one of DeliveredCount,
PromptSizeBytes, or EstimatedPromptTokens is set.

internal/api/core/network_details.go-130-133 (1)

130-133: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Align fanout validation between create and update.

• createNetworkChannelFanoutFields routes store.ValidateNetworkFanoutPolicy failures through StatusForNetworkError, but that error is not mapped there, so invalid fanout policy can return 500 on create while update returns 400.
• NetworkChannelEntry.Validate() only checks that CoordinatorPeerID is a valid peer ID; add the policy-specific coordinator requirement/forbidden rule in one place if that coupling is intended.

🤖 Prompt for AI Agents

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/network_details.go` around lines 130 - 133, The fanout
validation path in create is inconsistent with update because
`createNetworkChannelFanoutFields` and `NetworkChannelEntry.Validate` do not
enforce the same policy-specific rules. Update the fanout handling around
`createNetworkChannelFanoutFields` so invalid
`store.ValidateNetworkFanoutPolicy` input is mapped to the same client-facing
error behavior as update, and add the coordinator requirement/forbidden check in
`NetworkChannelEntry.Validate` if that policy coupling belongs there. Keep the
logic aligned with the existing `createNetworkChannelFanoutFields`,
`NetworkChannelEntry.Validate`, and `StatusForNetworkError` flow so create and
update reject the same invalid fanout requests.

internal/store/globaldb/global_db_network_conversation_repository_test.go-243-337 (1)

243-337: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Add the required test-placement metadata.

Before this new case, name the invariant, owning layer, and canonical suite so the test’s placement remains intentional. As per path instructions, **/*_test.go: “Test placement is mandatory before test creation. Before adding, moving, or expanding any test, name the invariant, owning layer, and canonical suite.”

Proposed addition

+	// Invariant: delivered prompt-cost updates aggregate by thread peer.
+	// Owning layer: globaldb network conversation repository.
+	// Canonical suite: TestGlobalDBWriteConversationMessageThreadSummaries.
 	t.Run("Should aggregate delivered prompt cost by thread peer", func(t *testing.T) {

🤖 Prompt for AI Agents

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/store/globaldb/global_db_network_conversation_repository_test.go`
around lines 243 - 337, The new test case in globalDB network conversation
repository needs the required test-placement metadata before the t.Run block.
Add the invariant, owning layer, and canonical suite near the existing test
around ListNetworkThreadPeerTokenStats and GetThread so the placement is
intentional and traceable. Use the identifiers openTestGlobalDB,
UpdateNetworkThreadPeerTokenStats, ListNetworkThreadPeerTokenStats, and
GetThread to anchor the metadata to this test scope.

Source: Path instructions

internal/config/config.go (1)

459-472: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider upper bounds for the new byte-size fields.

MaxPayload validates both a lower and upper bound (<= 0 and > 1<<31-1), but ResponseGuidanceMaxBytes and DeliveryStructuredBodyMaxBytes only check positivity. Not blocking, but for consistency with the existing pattern you may want to cap these too.

Also applies to: 1742-1762

🤖 Prompt for AI Agents

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/config/config.go` around lines 459 - 472, Add upper-bound validation
for the new byte-size settings in NetworkConfig alongside the existing
MaxPayload checks. Update the validation logic that handles
ResponseGuidanceMaxBytes and DeliveryStructuredBodyMaxBytes so they also reject
values above the same practical maximum used for other size fields, matching the
pattern in the config validation code and any related defaults/tests around
those symbols.

web/src/hooks/routes/use-task-detail-orchestration-tab.ts (1)

117-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Toast message doesn't pluralize correctly for a single run.

${result.runs.length} designated runs queued. always uses plural "runs", producing "1 designated runs queued." when only one designation is fanned out.

✏️ Proposed fix

-        toast.success(`${result.runs.length} designated runs queued.`);
+        const count = result.runs.length;
+        toast.success(`${count} designated run${count === 1 ? "" : "s"} queued.`);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web/src/hooks/routes/use-task-detail-orchestration-tab.ts` around lines 117 -
133, The success toast in handleFanOutRuns always uses the plural “runs,” so
update the message to handle singular vs plural based on result.runs.length.
Keep the logic in use-task-detail-orchestration-tab’s handleFanOutRuns callback,
and build the toast text from the count so it says “1 designated run queued.”
for one item and “N designated runs queued.” otherwise.

internal/api/core/tasks.go (1)

2119-2134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Reusing tool-invoke-status constants for an unrelated rollup schema.

toolInvokeStatusCompleted/toolInvokeStatusCanceled (defined in internal/api/core/tools.go for tool invocation status) are used as JSON map keys for the task designation rollup. It works today because the string values happen to coincide with "completed"/"canceled", but it couples two unrelated concerns and will silently drift if either constant's value ever changes for its original purpose. Define dedicated constants (or use literal strings, consistent with "failed", "running", etc. in the same map) for the rollup schema.

🤖 Prompt for AI Agents

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/tasks.go` around lines 2119 - 2134, The task designation
rollup in the JSON map is reusing tool-invoke status constants for unrelated
keys, which tightly couples two separate schemas. Update the map construction in
the rollup logic to use dedicated rollup key constants or plain literal strings
for the completed and canceled fields, matching the other schema keys already
present in this payload. Keep the change localized around the JSON encoding
block so it no longer depends on toolInvokeStatusCompleted or
toolInvokeStatusCanceled from tools.go.

internal/observe/tasks.go (1)

1249-1268: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicate designation-population logic across observe/tasks.go and task/live.go.

taskInboxRunSummary here and runSummaryFromRun in internal/task/live.go both build the RunSummary and then separately overwrite DesignationGroupID/Designation via taskpkg.DesignationFromRun. Consider adding a small helper on taskpkg.RunSummary (or a package-level function) that both call sites can share, so the derivation logic lives in one place.

♻️ Proposed refactor (sketch)

+// in internal/task/designation.go
+func ApplyDesignation(summary *RunSummary, run Run) {
+	if designation, ok := DesignationFromRun(run); ok {
+		summary.DesignationGroupID = designation.GroupID
+		summary.Designation = designation.Summary()
+	}
+}

Then both taskInboxRunSummary and runSummaryFromRun call taskpkg.ApplyDesignation(summary, run) instead of repeating the conditional inline.

🤖 Prompt for AI Agents

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/observe/tasks.go` around lines 1249 - 1268, The
designation-population logic is duplicated between taskInboxRunSummary and
runSummaryFromRun, so extract the conditional taskpkg.DesignationFromRun
handling into a shared helper on taskpkg.RunSummary or a package-level function.
Update both call sites to build the base RunSummary and then invoke the shared
helper (for example, ApplyDesignation) to set DesignationGroupID and Designation
consistently in one place.

internal/config/task_orchestration.go (1)

90-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the 5 fan-out cap into a shared constant.

The designated-run ceiling (5) appears as the default, the validation upper bound here, and again as a hardcoded fallback in internal/api/core/tasks.go and internal/daemon/native_tools.go (per the provided downstream context). Centralizing it avoids drift if the cap ever changes.

♻️ Proposed refactor

+const DefaultDesignatedRunMax = 5
+
 func DefaultTaskConfig() TaskConfig {
 	return TaskConfig{
 		Orchestration: TaskOrchestrationConfig{
 			...
-			DesignatedRunMax:          5,
+			DesignatedRunMax:          DefaultDesignatedRunMax,
 			...
 
 func (c TaskOrchestrationConfig) Validate(path string) error {
 	...
-	if c.DesignatedRunMax <= 0 || c.DesignatedRunMax > 5 {
+	if c.DesignatedRunMax <= 0 || c.DesignatedRunMax > DefaultDesignatedRunMax {
 		return fmt.Errorf("%s.designated_run_max must be between 1 and 5: %d", path, c.DesignatedRunMax)
 	}

Also applies to: 173-175

🤖 Prompt for AI Agents

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/config/task_orchestration.go` at line 90, The designated-run fan-out
cap is hardcoded as 5 in multiple places, so extract it into a shared constant
and use it for the default, validation upper bound, and fallback paths. Update
the configuration logic in task orchestration (including the `DesignatedRunMax`
default/validation) and replace the duplicated fallback values in
`internal/api/core/tasks.go` and `internal/daemon/native_tools.go` with the
shared symbol so the cap stays consistent.

internal/daemon/task_role_runtime.go (1)

590-618: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Extra blank line when a designation is present.

taskRoleDesignationOverlay returns a string with a trailing "\n", and the enclosing template also places %s on its own template line ending in \n, so a present designation renders with a blank line after it in the prompt (Coordination channel: block). Not functionally wrong, but a minor prompt-formatting inconsistency.

✏️ Optional cleanup

-	return "Designated fan-out assignment: " + strings.Join(parts, ", ") + ".\n"
+	return "Designated fan-out assignment: " + strings.Join(parts, ", ") + "."

🤖 Prompt for AI Agents

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/daemon/task_role_runtime.go` around lines 590 - 618, The prompt
formatting in taskRoleDesignationOverlay is adding an extra blank line when a
designation exists because the helper returns a trailing newline and the
surrounding fmt.Sprintf template already places the designation on its own line.
Update taskRoleDesignationOverlay (and, if needed, the fmt.Sprintf block that
uses designation) so the designation text is emitted without a double line break
while preserving the rest of the taskRoleRuntime prompt formatting.

internal/api/core/network_test.go (1)

1424-1509: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

New subtests skip t.Parallel().

Both new subtests ("Should update network channel policy", "Should upsert and list network subscriptions...") omit t.Parallel() inside the t.Run closure.

As per coding guidelines, **/*_test.go: "Use t.Run("Should ...") subtests in Go tests, with t.Parallel by default (opt-out with t.Setenv)".

♻️ Suggested fix

 	t.Run("Should update network channel policy", func(t *testing.T) {
+		t.Parallel()
 		updateResp := performRequest(

 	t.Run("Should upsert and list network subscriptions with default channel metadata", func(t *testing.T) {
+		t.Parallel()
 		upsertResp := performRequest(

🤖 Prompt for AI Agents

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/network_test.go` around lines 1424 - 1509, The new network
API subtests are missing the default parallelization used in Go tests. Add
t.Parallel() at the start of each t.Run closure for the new cases in
network_test.go, specifically the subtests around updating channel policy and
upserting/listing subscriptions, unless they must opt out because they use
t.Setenv or other shared state.

Source: Coding guidelines

web/src/systems/network/components/shell/__tests__/channel-policy-dialog.test.tsx (1)

39-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider covering fanout-policy/coordinator selection changes.

Only the purpose-edit path is exercised; the new fanout policy and coordinator-peer selectors (including reset to "No coordinator") aren't tested.

As per coding guidelines, web/**/*.{test,spec}.{js,ts,jsx,tsx}: "Write unit tests with high code coverage for critical paths".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@web/src/systems/network/components/shell/__tests__/channel-policy-dialog.test.tsx`
around lines 39 - 68, The ChannelPolicyDialog test only covers editing the
purpose field and misses the new fanout-policy and coordinator-peer selector
behavior. Add coverage in ChannelPolicyDialog for changing fanout_policy and
coordinator_peer_id, asserting onSubmit receives the updated values, and include
a case that resets the coordinator selection back to “No coordinator” to verify
the cleared state is submitted correctly. Use the existing render flow and
userEvent interactions in this test alongside the current onSubmit and
onOpenChange assertions.

Source: Coding guidelines

internal/network/delivery_test.go (1)

271-323: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

New test doesn't use t.Run("Should ...") subtests.

TestFormatNetworkMessageCapsStructuredBody is a brand-new test but is written flat (no subtests), unlike the other new test in this file (TestDeliveryCoordinatorCoalescesDigestDeliveries) which properly wraps scenarios in t.Run("Should ..."). As per path instructions, Go tests must use t.Run("Should...") subtests.

♻️ Suggested wrap

 func TestFormatNetworkMessageCapsStructuredBody(t *testing.T) {
 	t.Parallel()
-
-	envelope := Envelope{
-		...
-	}
-	...
+
+	t.Run("Should cap oversized structured trace body and mark truncation", func(t *testing.T) {
+		t.Parallel()
+
+		envelope := Envelope{
+			...
+		}
+		...
+	})
 }

🤖 Prompt for AI Agents

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/network/delivery_test.go` around lines 271 - 323,
`TestFormatNetworkMessageCapsStructuredBody` should be wrapped in a
`t.Run("Should ...")` subtest to match the test style used by
`TestDeliveryCoordinatorCoalescesDigestDeliveries` and the repo’s Go test
conventions. Keep the existing assertions and setup inside a single `t.Run`
block, using a descriptive “Should…” name so the scenario is explicit and
consistent with nearby tests.

Source: Coding guidelines

internal/api/core/network_details.go (1)

792-794: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated participant-recording loop across three functions.

applyNetworkChannelMessages, summarizeNetworkMessageHistory, and summarizeHistoricalParticipantCount each independently repeat the same "record PeerFrom, PeerTo, then each Mentions entry" pattern. Consider extracting a shared helper (e.g., recordMessageParticipants(recorder func(string), message store.NetworkMessageEntry)) to keep the three call sites in sync as the set of participant sources evolves.

Also applies to: 1429-1431, 1466-1468

🤖 Prompt for AI Agents

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/network_details.go` around lines 792 - 794, The
participant-recording logic is duplicated in applyNetworkChannelMessages,
summarizeNetworkMessageHistory, and summarizeHistoricalParticipantCount, so
extract a shared helper (for example, a recordMessageParticipants-style
function) that records PeerFrom, PeerTo, and each Mentions entry in one place.
Update those three call sites to use the helper and keep the behavior identical
so any future participant-source changes only need to be made once.

web/src/systems/network/components/composer/composer.tsx (1)

77-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider a semantic role for the mentions container.

The <div aria-label="Mention recipients"> has no ARIA role, so some screen readers may not announce the label. Adding role="group" (or role="list" with role="listitem" on each Pill) would make it reliably announced.

♿ Optional fix

       {state.mentions.length > 0 ? (
         <div
           aria-label="Mention recipients"
+          role="group"
           className="flex min-h-5 flex-wrap items-center gap-1.5"
           data-testid={`network-composer-mentions-${testIdSuffix}`}
         >

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web/src/systems/network/components/composer/composer.tsx` around lines 77 -
97, The mentions container in composer.tsx uses aria-label without a semantic
role, so screen readers may not announce it reliably. Update the container in
the mentions render block to use a proper role such as group, or switch to
list/listitem semantics if that better fits the Pills. Keep the existing
aria-label and ensure the role is applied where state.mentions is rendered so
the mention recipients section is announced consistently.

internal/task/designation_test.go (1)

8-50: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Missing test case for partial designation (group set, metadata unparsable).

Per DesignationFromRun's documented behavior, ok=true is returned whenever DesignationGroupID is non-empty — even when Metadata fails to parse as JSON, in which case Index/Brief fall back to zero values. This means ApplyRunDesignationSummary would silently overwrite a summary's existing Designation with an empty one in that scenario. Add a test case exercising a run with a non-empty DesignationGroupID but missing/invalid Metadata to lock in the intended fallback behavior.

🤖 Prompt for AI Agents

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/task/designation_test.go` around lines 8 - 50, Add a test in
TestApplyRunDesignationSummary that covers a run with a non-empty
DesignationGroupID but missing or invalid Metadata, since DesignationFromRun
returns ok=true in that case and ApplyRunDesignationSummary should preserve the
group while falling back to zero-value designation fields. Use the existing
ApplyRunDesignationSummary and RunSummary symbols to verify the summary gets the
run’s DesignationGroupID and an empty/default RunDesignationSummary rather than
remaining stale or failing.

internal/store/globaldb/global_db_test.go (1)

460-460: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Minor: extract magic number 25 into a named constant.

Unlike the sibling prefix test (line 409) which derives its cutoff from expectedGlobalMigrationPrefix(), this test hardcodes 25. A named constant (e.g., preNetworkActivationMigrationCount) would make the intent clearer and reduce the risk of silent drift if someone edits nearby test code without understanding what 25 represents.

🤖 Prompt for AI Agents

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/store/globaldb/global_db_test.go` at line 460, The test in
global_db_test.go uses a hardcoded migration slice cutoff of 25, which should be
replaced with a named constant to make the intent explicit and avoid silent
drift. Update the RunMigrations call site in the global DB test to use a
descriptive constant such as preNetworkActivationMigrationCount, and define it
near the sibling migration-prefix test logic so the cutoff is easy to understand
and maintain.

internal/network/router.go (1)

1237-1240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Log the swallowed routing-resolution errors.

Both fallback branches (thread-participant lookup failure and fanout-policy lookup failure) discard err entirely before defaulting to a degraded routing mode. This is the right fail-open behavior (fixes the earlier message-drop issue), but a persistent DB failure here will silently and permanently downgrade delivery mode (e.g., coordinator/all_members silently becomes capability_match) with no diagnostic trail. Consider a slog.Warn call before defaulting so operators can detect the underlying resolver failure.

Also applies to: 1285-1289

🤖 Prompt for AI Agents

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/network/router.go` around lines 1237 - 1240, The fallback in router
resolution is swallowing resolver errors, so add a warning log before defaulting
to degraded delivery. In router.go, update the thread-participant path in the
routing logic around threadParticipantSet, and the fanout-policy fallback path
in the same router flow, to emit a slog.Warn with the caught err and enough
context (such as envelope/target identifiers) before calling the empty/default
delivery selector. Keep the fail-open behavior, but ensure both fallback
branches preserve a diagnostic trail when the lookup fails.

internal/daemon/network_task_status_observer.go (1)

42-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider functional options instead of growing positional parameters.

newNetworkTaskStatusObserver now takes 6 positional args after this change (adding queueSize, timeout). Router in this same PR (internal/network/router.go) already uses the functional-options pattern for equivalent constructor knobs. Adopting the same pattern here (newNetworkTaskStatusObserver(networkRuntime, tasks, opts ...Option)) would improve extensibility and match repo convention.

Based on learnings and coding guidelines, "Use functional options for constructors — NewManager(opts ...Option)."

🤖 Prompt for AI Agents

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/daemon/network_task_status_observer.go` around lines 42 - 80,
newNetworkTaskStatusObserver is becoming harder to extend because it now relies
on many positional parameters; switch it to a functional-options constructor to
match the repo’s constructor pattern used by Router and NewManager. Update the
observer constructor to accept opts ...Option, move queueSize and timeout into
options with sensible defaults, and keep the existing nil/default handling
inside newNetworkTaskStatusObserver so call sites stay readable and future knobs
can be added without changing the signature.

Source: Coding guidelines

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

internal/store/globaldb/global_db_network_channels.go (1)

238-244: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Redundant branching in error handling — both paths are identical.

Both the errors.Is(err, sql.ErrNoRows) branch and the fallback return the exact same value. Simplify.

♻️ Simplify redundant branch

 	entry, err := scanNetworkChannel(row)
 	if err != nil {
-		if errors.Is(err, sql.ErrNoRows) {
-			return store.NetworkChannelEntry{}, err
-		}
-		return store.NetworkChannelEntry{}, err
+		return store.NetworkChannelEntry{}, err
 	}
 	return entry, nil

🤖 Prompt for AI Agents

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/store/globaldb/global_db_network_channels.go` around lines 238 -
244, The error handling in the scanNetworkChannel path has a redundant
sql.ErrNoRows branch because both branches return the same
store.NetworkChannelEntry{} and err. Simplify the logic in the function that
processes scanNetworkChannel results by removing the errors.Is(err,
sql.ErrNoRows) special case and just returning the empty entry plus the original
error from the single err check.