refactor: improve e2e tests

[pedronauck]

Summary by CodeRabbit

• New Features

  • Create and revert knowledge memory entries via new UI dialog and API flow.
  • Create network channels from the UI with workspace-aware workflow.
  • Permission rejection notices and improved permission prompt lifecycle in sessions.

• Bug Fixes

  • Corrected tasks inbox empty-state rendering.
  • Network settings accept auto-assigned port (-1).
  • Permission prompt hides after successful decision.

• Improvements

  • Enhanced prompt cancellation and event tracking; various reliability/timeouts tightened in tests.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
agh-site	Error		May 9, 2026 10:32pm

[coderabbitai]

Walkthrough

Implements per-prompt cancellation and cancellation classification in ACP/session paths, routes memory decision reverts to workspace-aware stores, stabilizes permission data-part IDs and UI part upserts, adds knowledge create/revert UI and hooks, updates permission prompt UI, refactors network route view and various UI hooks, strengthens SDK/stdio integration tests, and adds many Playwright E2E parity/hardening suites, scripts, and timeout/test fixes.

Changes

Core runtime, contracts, UI, and E2E

Layer / File(s)	Summary
Data / Protocol / Transcript internal/api/core/prompt_stream.go, internal/transcript/ui_messages.go, internal/api/core/prompt_stream_test.go, internal/transcript/transcript_test.go	Adds SSE envelope id, computes stable permission data-part identifiers, changes UI message builder to upsert parts by (Type, ID), and adds tests validating pending→final permission frame id stability and UI part merging.
ACP & Session Cancellation internal/acp/*, internal/session/*, internal/testutil/acpmock/*, internal/acp/failure.go	Derives per-prompt contexts/cancel functions; driver begin/run refactored to accept and teardown per-prompt cancels; Driver.Cancel prefers local cancel of active prompt before sending session-cancel notification; failure classification detects request errors that indicate cancellation.
Memory Backend Routing internal/api/core/memory.go, internal/api/core/memory_workspace_test.go	RevertMemoryDecision now selects the appropriate memory store based on decision scope (global/workspace/agent-tier=workspace) with workspace resolution helpers; tests verify revert restores prior content for workspace-scoped and agent@workspace-scoped decisions.
Manager / Session Supervision internal/session/manager_prompt.go, internal/session/session.go, internal/session/prompt_activity.go	Manager wires prompt execution ctx earlier and stores session cancel; session tracks currentPromptCancel with set/clear/cancel helpers; supervision timeout/grace logic adjusted; tests/timeouts updated.
SDK Integration Tests (stdio) sdk/typescript/src/__tests__/integration.test.ts	Replaces simple line reader with BufferedMessageReader plus stderr waiting and robust process termination; increases timeouts and improves diagnostics.
Permission UI / Toolkit web/src/systems/session/components/permission-prompt.tsx, web/src/systems/session/lib/session-toolkit.tsx, tests	Adds PermissionDataPart and PermissionRejectedNotice; PermissionPrompt hides itself after successful decision; toolkit now renders PermissionDataPart; tests cover pending/resolved/rejected flows and DOM behavior.
Knowledge Create / Revert UI & Hooks web/src/systems/knowledge/..., web/src/routes/_app/knowledge.tsx, tests	Adds KnowledgeCreateDialog component, write/revert adapter and hooks, types and fixtures, decisions UI revert button/state/error, and page wiring (Create button). Tests assert write/revert adapter calls and UI interactions; contract test added for revert request shape.
Network Route View & Create Action web/src/systems/network/hooks/use-network-route-view.ts, web/src/systems/network/hooks/use-network-create-channel-action.tsx, web/src/routes/_app/network.tsx	Adds useNetworkRouteView to compose sub-views, introduces useNetworkCreateChannelAction hook to manage create-channel dialog and submit flow, refactors NetworkRouteShell to use the view and forward create dialog as meta.
Settings & Small UI tweaks web/src/systems/settings/components/*, web/src/routes/_app/settings/network.tsx, packages/ui/.../page-header.*, web/src/hooks/routes/use-tasks-page.ts, web/src/routes/_app/tasks.tsx, web/src/systems/tasks/components/tasks-inbox-view.tsx	Integer input now allows negative values (min -1) and pattern handling; restart banner shows active-session label; PageHeader responsive layout updated; tasks hook supports dismissed selection; inbox empty-state refined; Storybook/test timeouts increased.
Tests / E2E / Scripts / Infra web/e2e/**/*, web/playwright.config.ts, web/e2e/scripts/check-nightly-coverage.ts, internal/cli/cli_integration_test.go, internal/testutil/*	Large set of new/expanded Playwright E2E specs for bridges, dashboard, extensibility, harness, jobs, knowledge, network, sandbox, sessions, settings, tasks, triggers; helpers for parity snapshots, CLI shims, sensitive-leak scanning, viewport matrices, and artifact persistence. Playwright expect timeout set; nightly coverage script added; integration bridge service exposes optional task subscription ops; many test timeout increases and stability fixes.

Estimated code review effort
🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

• compozy/agh#27: Overlaps E2E harness, acpmock driver, and Playwright configs with similar parity/hardening additions.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

[coderabbitai]

Actionable comments posted: 10

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

web/src/systems/knowledge/mocks/index.ts (1)

2-12: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Re-export the legacy memoryMutationFixture alias from the public barrel.

fixtures.ts explicitly keeps memoryMutationFixture for backward compatibility, but this barrel drops it. Any existing caller importing the legacy name from @/systems/knowledge/mocks will break even though the alias still exists.

♻️ Proposed fix

 export {
+  memoryMutationFixture,
   memoryDecisionRevertFixture,
   memoryDecisionsFixture,
   memoryDeleteFixture,
   memoryDreamTriggerFixture,

🤖 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/knowledge/mocks/index.ts` around lines 2 - 12, The barrel
export in index.ts currently omits the legacy alias memoryMutationFixture;
update the export list to re-export memoryMutationFixture from "./fixtures" so
callers importing memoryMutationFixture from the public module (e.g.,
"@/systems/knowledge/mocks") continue to work; locate the export block in
index.ts and add memoryMutationFixture alongside the other fixtures to preserve
backward compatibility.

🧹 Nitpick comments (14)

web/e2e/__tests__/tasks-coordinator-handoff.spec.ts (1)

125-126: ⚡ Quick win

Add a URL assertion after goto("/tasks") for consistency and clearer diagnostics.

The first test already asserts the route after navigation; mirroring that here makes failures easier to triage and reduces ambiguity.

Suggested diff

   await appPage.goto(runtime.url("/tasks"), { waitUntil: "domcontentloaded" });
+  await expect(appPage).toHaveURL(/\/tasks$/);
   await tasksUI.openCreate.click();

🤖 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/e2e/__tests__/tasks-coordinator-handoff.spec.ts` around lines 125 - 126,
Add a URL assertion immediately after calling
appPage.goto(runtime.url("/tasks"), { waitUntil: "domcontentloaded" }) and
before invoking tasksUI.openCreate.click() so the test verifies the browser is
actually at runtime.url("/tasks"); locate the navigation call to appPage.goto
and insert an assertion that the current URL matches runtime.url("/tasks")
(using the project's existing test assertion helper or expect-to-have-URL
utility) to improve diagnostics.

web/src/routes/_app/settings/__tests__/-network.test.tsx (1)

183-199: ⚡ Quick win

Make this sentinel-port test interaction-based to avoid false positives.

Right now it preloads port: -1, so it doesn’t verify the actual user-edit path that triggers validity updates. Start from a normal port and type -1 in the input before asserting save state.

Proposed test adjustment

   it("accepts the auto-assigned listener port sentinel without blocking other network saves", () => {
-    pageState.envelope = {
-      ...envelope,
-      config: {
-        ...envelope.config,
-        port: -1,
-      },
-    };
-    pageState.draft = pageState.envelope.config;
     pageState.isDirty = true;
 
     render(<NetworkSettingsPage />);
 
-    expect(screen.getByTestId("settings-page-network-port-input")).toHaveValue("-1");
+    const portInput = screen.getByTestId("settings-page-network-port-input");
+    fireEvent.change(portInput, { target: { value: "-1" } });
+
+    expect(portInput).toHaveValue("-1");
     expect(screen.queryByTestId("settings-page-network-save-invalid")).not.toBeInTheDocument();
     expect(screen.getByTestId("settings-page-network-save")).not.toBeDisabled();
   });

🤖 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/__tests__/-network.test.tsx` around lines 183 -
199, The test should simulate the user typing the sentinel port instead of
preloading it; change the setup so pageState.envelope.config.port starts with a
normal port (e.g., 3000), render NetworkSettingsPage, then use userEvent to
clear and type "-1" into the input with test id
"settings-page-network-port-input", and finally assert that
"settings-page-network-save-invalid" is not in the document and
"settings-page-network-save" is enabled; this ensures the component's validity
logic (the user interaction path in NetworkSettingsPage) is exercised rather
than relying on a preloaded pageState value.

web/src/systems/network/hooks/use-network-create-channel-action.tsx (1)

39-55: ⚡ Quick win

Replace the promise chain with async/await.

This path is small enough that try/catch is clearer, and it matches the repo rule for TS promise handling.

Suggested refactor

-  const handleCreateChannel = () => {
+  const handleCreateChannel = async () => {
     if (!activeWorkspaceId || !canCreateChannel) {
       return;
     }

-    void createChannel
-      .mutateAsync({
+    try {
+      const response = await createChannel.mutateAsync({
         agent_names: createDraft.selectedAgentNames,
         channel: createDraft.channelName.trim(),
         purpose: createDraft.purpose.trim(),
         workspace_id: activeWorkspaceId,
-      })
-      .then(response => {
-        const channel = response.channel.channel;
-        setCreateDraft(createNetworkChannelDraft());
-        setCreateOpen(false);
-        void navigate({ params: { channel }, to: "/network/$channel/threads" });
-      })
-      .catch(error => {
-        const message = error instanceof Error ? error.message : "Failed to create network channel";
-        toast.error(message);
-      });
+      });
+      const channel = response.channel.channel;
+      setCreateDraft(createNetworkChannelDraft());
+      setCreateOpen(false);
+      await navigate({ params: { channel }, to: "/network/$channel/threads" });
+    } catch (error) {
+      const message = error instanceof Error ? error.message : "Failed to create network channel";
+      toast.error(message);
+    }
   };

As per coding guidelines, "Always use async/await for promises in JavaScript/TypeScript; avoid .then() chains."

🤖 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/hooks/use-network-create-channel-action.tsx` around
lines 39 - 55, The current createChannel.mutateAsync(...) call uses a
.then/.catch chain; replace it with an async function using try/catch and await
the createChannel.mutateAsync call. Inside try await the response, extract
response.channel.channel, call setCreateDraft(createNetworkChannelDraft()),
setCreateOpen(false), and call navigate({ params: { channel }, to:
"/network/$channel/threads" }); in the catch block capture the error (Error
type-check), and call toast.error(...) with the message; ensure this change is
applied where use-network-create-channel-action.tsx defines the handler so
createChannel.mutateAsync, setCreateDraft, setCreateOpen, navigate and
toast.error are used the same but with async/await.

internal/testutil/testutil.go (1)

15-15: ⚡ Quick win

Avoid hardcoding shared test timeout; allow config override.

A fixed 45s constant makes CI tuning harder across environments. Keep 45s as fallback, but resolve from config/env first.

🛠️ Example refactor

-const defaultTimeout = 45 * time.Second
+var defaultTimeout = resolveDefaultTimeout()
+
+func resolveDefaultTimeout() time.Duration {
+	if raw := os.Getenv("AGH_TEST_TIMEOUT"); raw != "" {
+		if d, err := time.ParseDuration(raw); err == nil && d > 0 {
+			return d
+		}
+	}
+	return 45 * time.Second
+}

As per coding guidelines, "Never hardcode configuration values in Go code; use config structs, environment variables, or config files."

🤖 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/testutil/testutil.go` at line 15, Replace the hardcoded constant
defaultTimeout with a configurable value: change const defaultTimeout = 45 *
time.Second to code that first reads an override (e.g., from an environment
variable like TEST_TIMEOUT_SECONDS or from a testutil config struct) and parses
it into a time.Duration, falling back to 45*time.Second when the env/struct
value is absent or invalid; update any callers that referenced defaultTimeout to
use the new exported function or variable (e.g., TestTimeout() or
DefaultTimeout) so tests can be tuned via env/config while retaining the 45s
default.

web/src/systems/settings/components/settings-restart-banner.tsx (1)

88-88: ⚡ Quick win

Use design-token classes instead of arbitrary Tailwind values.

Line 88 introduces ad-hoc typography/color tokens. Reuse the existing tokenized badge classes to keep the banner aligned with the design system.

🎨 Tokenized class example

- className="font-mono text-[10px] font-semibold tracking-[0.08em] text-[color:var(--color-text-label)]"
+ className="font-mono text-badge font-semibold tracking-badge text-(--color-text-label)"

As per coding guidelines, "Pull every color, font, radius, spacing step, and motion value from DESIGN.md — never invent tokens."

🤖 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/settings/components/settings-restart-banner.tsx` at line 88,
The component SettingsRestartBanner uses an ad-hoc class string ("font-mono
text-[10px] font-semibold tracking-[0.08em]
text-[color:var(--color-text-label)]"); replace this arbitrary Tailwind
typography and color with the design-system tokenized badge classes used
elsewhere (use the project’s tokenized badge/label utility classes instead of
raw font-, tracking-, size- and color- values) so the banner aligns with the
DESIGN.md tokens; update the className on the element in
settings-restart-banner.tsx to use those existing badge token classes.

internal/session/interfaces.go (1)

431-431: ⚡ Quick win

Make the Cancel doc comment intent-focused.

This comment restates the method name and drops the rationale behind cooperative prompt cancellation semantics. Prefer a why-oriented comment.

✍️ Suggested wording

-// Cancel cancels the active ACP prompt.
+// Cancel requests cooperative termination of the in-flight prompt while
+// keeping process/session lifecycle control with the manager.

As per coding guidelines, "Write comments that explain why the code exists, not what it does; avoid stating the obvious."

🤖 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/session/interfaces.go` at line 431, The doc comment for the Cancel
method is stating the obvious; replace it with an intent-focused comment that
explains why cooperative prompt cancellation exists and what guarantees callers
can expect (e.g., that Cancel requests cooperative termination of an active ACP
prompt to allow cleanup, avoid dangling work, and notify waiting parties),
updating the comment above the Cancel method in the session interface (symbol:
Cancel) to describe those semantics and rationale rather than repeating the
method name.

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

472-485: 💤 Low value

Note: Duplicated logic with uiPermissionDataPartID.

This helper has identical logic to uiPermissionDataPartID in internal/transcript/ui_messages.go. The duplication is acceptable given the packages have different responsibilities, but consider extracting to a shared location if more consumers emerge.

🤖 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/prompt_stream.go` around lines 472 - 485, Duplicate logic
found between promptPermissionDataPartID and uiPermissionDataPartID; extract the
shared logic into a single helper (e.g., permissionDataPartID(requestID, turnID,
toolCallID string) string) in a common/shared package and update both
promptPermissionDataPartID and uiPermissionDataPartID to call that helper,
preserving the existing return behavior and trimming rules for RequestID,
TurnID, and ToolCallID.

web/e2e/__tests__/session-hardening.spec.ts (1)

213-220: ⚡ Quick win

Persist and leak-scan the cancel/clear/delete flow as well.

This is the only hardening scenario in the file that stops after captureJSON()/captureScreenshot(). Without browserArtifacts.persist(appPage) and assertNoSensitiveLeak(...), this path never verifies the stored browser/network/daemon artifacts for secret leakage.

♻️ Suggested follow-up

   await runtime.artifactCollector.captureJSON("browser_api_snapshots", {
     after_clear: afterClear,
     before_clear: beforeClear,
     cli_sessions_after_delete: cliSessions,
     deleted_session_id: deletableSession.id,
   });
   await browserArtifacts.captureScreenshot("session-cancel-clear-delete", appPage);
+  await browserArtifacts.persist(appPage);
+  await assertNoSensitiveLeak(appPage, runtime, {
+    afterClear,
+    beforeClear,
+    cliSessions,
+    deletedSessionID: deletableSession.id,
+  });

🤖 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/e2e/__tests__/session-hardening.spec.ts` around lines 213 - 220, The test
stops after calling runtime.artifactCollector.captureJSON(...) and
browserArtifacts.captureScreenshot(...), so add calls to persist the collected
artifacts and run the leak scanner: call browserArtifacts.persist(appPage) after
captureScreenshot and then invoke assertNoSensitiveLeak(...) with the persisted
artifact identifiers (or the same appPage/context) to validate no secrets
leaked; update the session-hardening.spec.ts flow around the existing
runtime.artifactCollector.captureJSON and browserArtifacts.captureScreenshot
lines to include these calls so the cancel/clear/delete scenario is persisted
and scanned.

web/e2e/__tests__/jobs-hardening.spec.ts (1)

680-694: ⚡ Quick win

Leak-scan the console and network artifacts here too.

assertNoJobSensitiveLeak() currently skips browser_console and browser_network, so a secret that only leaks through those artifacts would still pass this hardening suite.

🔍 Suggested expansion

 async function assertNoJobSensitiveLeak(
   appPage: Page,
   runtime: BrowserRuntime,
   snapshot: unknown
 ): Promise<void> {
   expect(JSON.stringify(snapshot)).not.toMatch(sensitivePattern);
   expect((await appPage.textContent("body")) ?? "").not.toMatch(sensitivePattern);
+  const consolePath = runtime.artifactCollector.artifactPath("browser_console");
+  const networkPath = runtime.artifactCollector.artifactPath("browser_network");
   const routeStatePath = runtime.artifactCollector.artifactPath("browser_route_state");
   const apiSnapshotPath = runtime.artifactCollector.artifactPath("browser_api_snapshots");
+  await expect(readFileIfExists(consolePath)).resolves.not.toMatch(sensitivePattern);
+  await expect(readFileIfExists(networkPath)).resolves.not.toMatch(sensitivePattern);
   await expect(readFileIfExists(routeStatePath)).resolves.not.toMatch(sensitivePattern);
   await expect(readFileIfExists(apiSnapshotPath)).resolves.not.toMatch(sensitivePattern);
   if (runtime.paths?.daemonLog) {
     await expect(readFileIfExists(runtime.paths.daemonLog)).resolves.not.toMatch(sensitivePattern);
   }
 }

🤖 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/e2e/__tests__/jobs-hardening.spec.ts` around lines 680 - 694,
assertNoJobSensitiveLeak currently only checks route_state, api_snapshots, page
body and daemonLog; add checks for the browser_console and browser_network
artifacts as well by using
runtime.artifactCollector.artifactPath("browser_console") and
("browser_network") and then await
expect(readFileIfExists(<path>)).resolves.not.toMatch(sensitivePattern) for
each; also include the snapshot parameter check already present
(JSON.stringify(snapshot)) and keep existing checks like appPage.textContent and
daemonLog intact so all console/network artifacts are scanned for sensitive
leaks.

web/e2e/__tests__/tasks-hardening.spec.ts (1)

798-801: ⚡ Quick win

Replace the .then() chain in captureTaskRunParity.

This helper is already async, so the inline .then() just makes the control flow inconsistent with the rest of the suite.

♻️ Suggested cleanup

 async function captureTaskRunParity(
   runtime: BrowserRuntime,
   taskID: string,
   runID: string
 ): Promise<TaskRunParitySnapshot> {
+  const udsPayload = await requestOperatorJSONOrThrow<TaskRunDetailEnvelope>(
+    runtime,
+    `/api/task-runs/${encodeURIComponent(runID)}`
+  );
   return {
     cliRuns: await taskCLI<TaskRun[]>(runtime, ["task", "run", "list", taskID, "--last", "10"]),
     http: await getTaskRun(runtime, runID),
-    uds: await requestOperatorJSONOrThrow<TaskRunDetailEnvelope>(
-      runtime,
-      `/api/task-runs/${encodeURIComponent(runID)}`
-    ).then(payload => payload.run),
+    uds: udsPayload.run,
   };
 }

As per coding guidelines, "Always use async/await for promises in JavaScript/TypeScript; avoid .then() chains."

🤖 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/e2e/__tests__/tasks-hardening.spec.ts` around lines 798 - 801, The inline
.then() on the awaited promise makes control flow inconsistent; inside
captureTaskRunParity replace the chained .then on
requestOperatorJSONOrThrow<TaskRunDetailEnvelope>(...) with an await that
assigns the response to a local variable (e.g., payload) and then set uds to
payload.run; update the uds assignment that currently references the chained
call so it uses the awaited payload.run instead, keeping identifiers
requestOperatorJSONOrThrow, TaskRunDetailEnvelope and uds unchanged.

web/e2e/__tests__/bridges.spec.ts (1)

445-479: ⚡ Quick win

Keep polling through transient bridge API failures.

Both wait helpers abort on the first runtime.requestJSON() error. That makes restart/create flows flaky if the bridge API briefly returns a transport error or non-2xx response before settling into the expected state.

🤖 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/e2e/__tests__/bridges.spec.ts` around lines 445 - 479, The two helpers
waitForBridgeByName and waitForBridgeStatus currently abort on the first
runtime.requestJSON() error; change them to catch requestJSON failures inside
their polling loops, ignore transient errors (log or store the last error),
continue retrying until the deadline, and only throw after the deadline with a
message that includes either the last observed status (for waitForBridgeStatus)
or the last request error (for waitForBridgeByName) so test flakiness from
transient transport/non-2xx responses is avoided. Ensure the try/catch wraps the
await runtime.requestJSON(...) call in both functions, preserve the existing
delay(250) backoff, and include unique identifiers bridgeId/displayName and the
last error/status in the final thrown Error.

web/e2e/__tests__/dashboard.spec.ts (1)

348-355: 💤 Low value

Consider adding a brief comment explaining expected failure during restart polling.

The empty catch block is intentional since the daemon may be unavailable during restart. Adding a short comment would document this expected behavior and satisfy the "every error must be handled or have a written justification" guideline.

📝 Suggested documentation

 async function pollRestartStatus(runtime: BrowserRuntime, statusURL: string): Promise<string> {
   try {
     const payload = await runtime.requestJSON<SettingsRestartStatus>(statusURL);
     return payload.status;
-  } catch {
+  } catch {
+    // Expected: daemon may be unreachable during restart window
     return "restarting";
   }
 }

🤖 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/e2e/__tests__/dashboard.spec.ts` around lines 348 - 355, The catch in
pollRestartStatus is intentionally empty because runtime.requestJSON can fail
while the daemon is restarting; add a short inline comment above the catch (or
inside it) explaining that returning "restarting" on any error is expected
behavior during daemon restart and references SettingsRestartStatus,
pollRestartStatus, runtime.requestJSON and statusURL so reviewers understand
this is an intentional, documented suppression of errors rather than an
accidental omission.

web/e2e/__tests__/extensibility.spec.ts (1)

649-660: 💤 Low value

Local workspace helper duplicates fixture function.

This useGlobalWorkspaceIfPrompted is a duplicate of the one in web/e2e/fixtures/workspace.ts. Consider importing from the shared fixture to reduce duplication, though the current approach provides test isolation.

🤖 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/e2e/__tests__/extensibility.spec.ts` around lines 649 - 660, The function
useGlobalWorkspaceIfPrompted duplicates an existing helper in the shared fixture
(fixtures/workspace.ts); replace the local implementation by importing and using
the shared helper instead: remove the local async function
useGlobalWorkspaceIfPrompted from extensibility.spec.ts and add an import of the
helper (the exported function from workspace.ts) at the top, then call that
imported function where useGlobalWorkspaceIfPrompted was previously used; if the
fixture export has a different name, map it appropriately to the local usage so
test behavior remains identical.

web/e2e/__tests__/settings-hardening.spec.ts (1)

395-404: 💤 Low value

extractJSON may fail if CLI outputs trailing non-JSON content.

The function finds the start of JSON but returns everything to the end of the trimmed string. If the CLI ever outputs status messages or logs after the JSON payload, JSON.parse in the caller will fail with a misleading error.

Consider finding the matching closing bracket to extract only the JSON portion:

♻️ Suggested improvement for robustness

 function extractJSON(stdout: string): string {
   const trimmed = stdout.trim();
   const objectIndex = trimmed.indexOf("{");
   const arrayIndex = trimmed.indexOf("[");
   const start = [objectIndex, arrayIndex].filter(index => index >= 0).sort((a, b) => a - b)[0];
   if (start === undefined) {
     throw new Error(`CLI output did not contain JSON: ${stdout}`);
   }
-  return trimmed.slice(start);
+  const isObject = trimmed[start] === "{";
+  const closeChar = isObject ? "}" : "]";
+  const end = trimmed.lastIndexOf(closeChar);
+  if (end < start) {
+    throw new Error(`CLI output contained unclosed JSON: ${stdout}`);
+  }
+  return trimmed.slice(start, end + 1);
 }

🤖 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/e2e/__tests__/settings-hardening.spec.ts` around lines 395 - 404, The
extractJSON function currently returns from the first JSON opening char to the
end of stdout which breaks if non-JSON text follows; update extractJSON to
detect whether the JSON starts with "{" or "[" (use the existing
objectIndex/arrayIndex logic), then scan forward from that start to find the
matching closing brace/bracket while properly accounting for nested structures
and string/escape characters, return only the substring between start and the
matching end, and throw an error if no balanced closing bracket is found;
reference the extractJSON function and ensure callers that call JSON.parse
receive only the isolated JSON payload.

🤖 Prompt for all review comments with 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.

Inline comments:
In `@internal/acp/client.go`:
- Around line 945-960: The notifier can miss an already-triggered cancel if ctx
is canceled before the goroutine runs; update startPromptCancellationNotifier to
check ctx.Err() (or select on ctx.Done() non-blockingly) before spinning up the
goroutine and, if ctx is already canceled, call
d.sendPromptCancellationNotification(ctx, proc, active) immediately instead of
returning the done-closer. Keep the existing goroutine branch for the normal
case so it still watches ctx.Done(); reference startPromptCancellationNotifier
and sendPromptCancellationNotification to locate the fix and ensure
Driver.Cancel()/cancelCurrentPrompt() results always yield a notification.

In `@internal/session/manager_test.go`:
- Around line 1534-1538: The cleanup currently ignores the error from
h.manager.Stop; change it to capture and handle the error (e.g., if err :=
h.manager.Stop(testutil.Context(t), session.ID); err != nil {
t.Errorf("manager.Stop failed: %v", err) }) so failures are reported instead of
being discarded — keep the existing session.clearCurrentTurnSource() and
session.clearCurrentPromptCancel() calls and place the error check on
h.manager.Stop to surface cleanup failures in the test output.

In `@internal/testutil/acpmock/cmd/acpmock-driver/main.go`:
- Around line 451-485: The bug is that beginPromptRegistration clears
session.pendingPromptCancel (session.pendingPromptCancel = false) which wipes a
cancel recorded by an earlier Cancel; to fix, stop resetting pendingPromptCancel
in beginPromptRegistration (remove or omit the assignment) so a pre-existing
pendingPromptCancel is preserved until registerPromptCancel consumes it; keep
session.promptStarting = true as is and let registerPromptCancel (which checks
session.pendingPromptCancel and calls cancel) handle clearing/consuming the
flag.

In `@web/e2e/__tests__/bridges.spec.ts`:
- Around line 613-620: The PATH construction in cliEnv (use the function name
cliEnv and parameter paths.cliShim) uses a hardcoded ':' which breaks on
Windows; change the PATH concatenation to use path.delimiter instead of ':'
(i.e., PATH: `${path.dirname(paths.cliShim)}${path.delimiter}${process.env.PATH
?? ""}`) and apply the same replacement in the other test files where the same
pattern occurs.

In `@web/e2e/__tests__/knowledge.spec.ts`:
- Around line 524-530: In promptDiagnostics(), avoid crashing on a
partially-written last JSONL line by making JSON.parse resilient: when mapping
lines from diagnosticsPath into DiagnosticsRecord, either wrap JSON.parse in a
try/catch and skip lines that fail to parse (ignoring only trailing/incomplete
lines), or implement a short retry/poll loop that re-reads the file until all
non-empty lines parse cleanly; ensure you still trim/filter empty lines and keep
the final filter by !record.lifecycle_event && record.prompt_index > 0 so
callers receive only valid DiagnosticsRecord entries.

In `@web/e2e/__tests__/network.spec.ts`:
- Around line 585-592: The networkCLI helper uses execFileAsync without limits
which can hang tests; modify the execFileAsync call inside networkCLI to include
a local timeout (e.g., a few seconds) and a larger maxBuffer to avoid
truncation, passing these in the options object alongside env:
cliEnv(runtime.paths); reference the networkCLI function and the execFileAsync
call (using runtime.paths.cliShim and cliEnv) so the subprocess fails fast on
timeout and returns an error instead of stalling the test runner.

In `@web/e2e/__tests__/session-hardening.spec.ts`:
- Around line 205-209: The test calls runtime.requestOperatorJSON?.(...) which
may be undefined and causes expect(undefined).rejects to fail; update the test
to first ensure the UDS helper exists or branch accordingly: check that
runtime.requestOperatorJSON is defined (e.g.,
expect(runtime.requestOperatorJSON).toBeDefined or throw if absent) before
invoking runtime.requestOperatorJSON with deletableSession.id, or explicitly
branch to skip/assert a different outcome when requestOperatorJSON is undefined
so the rejects-toThrow("404") only runs when the function is present.

In `@web/src/hooks/routes/use-knowledge-page.ts`:
- Around line 322-331: handleRevertDecision currently sets revertingDecisionId
before calling revertMemoryDecisionMutate and only clears it on success; change
it to early-return if isRevertPending is true to serialize reverts, move the
cleanup setRevertingDecisionId(...) into a finally block so the pending state is
always released even on rejection, and keep resetRevertMutation() and
setSelectedMemoryKey(...) behavior but ensure they run in the correct order
(reset before mutate, setSelectedMemoryKey on success). If you still need to
attach errors to a specific decision, introduce a separate errorTargetId (do not
reuse revertingDecisionId) so errors don’t clear/overwrite the pending id.
Target symbols: handleRevertDecision, isRevertPending, revertingDecisionId,
revertMemoryDecisionMutate, resetRevertMutation, setSelectedMemoryKey.

In `@web/src/systems/knowledge/components/knowledge-create-dialog.tsx`:
- Around line 52-59: The effect currently resets fields whenever defaultType
changes while the dialog is open; change the effect so it only runs when the
dialog opens by removing defaultType from the dependency array and depending
solely on open. In the useEffect that calls setType(defaultType), setName(""),
setDescription(""), and setContent(""), update the dependency array to [open] so
the form is reset only when open goes true; keep setType(defaultType) inside
that block so the type is initialized on open but not re-applied during open
when defaultType changes.

In `@web/src/systems/network/hooks/use-network-create-channel-action.tsx`:
- Around line 24-37: canCreateChannel and handleCreateChannel currently only
validate workspace/form state and can still submit when the network is not
enabled; update the logic to also require the network `enabled` flag.
Specifically, include `enabled` in the canCreateChannel boolean (e.g., require
enabled === true along with activeWorkspaceId, channelName, purpose, and
selectedAgentNames) and add an early return in handleCreateChannel that checks
`if (!enabled) return;` (or incorporate enabled into the existing guard with
activeWorkspaceId and canCreateChannel) so submissions are blocked when the
network is disabled/loading/error.

---

Outside diff comments:
In `@web/src/systems/knowledge/mocks/index.ts`:
- Around line 2-12: The barrel export in index.ts currently omits the legacy
alias memoryMutationFixture; update the export list to re-export
memoryMutationFixture from "./fixtures" so callers importing
memoryMutationFixture from the public module (e.g., "@/systems/knowledge/mocks")
continue to work; locate the export block in index.ts and add
memoryMutationFixture alongside the other fixtures to preserve backward
compatibility.

---

Nitpick comments:
In `@internal/api/core/prompt_stream.go`:
- Around line 472-485: Duplicate logic found between promptPermissionDataPartID
and uiPermissionDataPartID; extract the shared logic into a single helper (e.g.,
permissionDataPartID(requestID, turnID, toolCallID string) string) in a
common/shared package and update both promptPermissionDataPartID and
uiPermissionDataPartID to call that helper, preserving the existing return
behavior and trimming rules for RequestID, TurnID, and ToolCallID.

In `@internal/session/interfaces.go`:
- Line 431: The doc comment for the Cancel method is stating the obvious;
replace it with an intent-focused comment that explains why cooperative prompt
cancellation exists and what guarantees callers can expect (e.g., that Cancel
requests cooperative termination of an active ACP prompt to allow cleanup, avoid
dangling work, and notify waiting parties), updating the comment above the
Cancel method in the session interface (symbol: Cancel) to describe those
semantics and rationale rather than repeating the method name.

In `@internal/testutil/testutil.go`:
- Line 15: Replace the hardcoded constant defaultTimeout with a configurable
value: change const defaultTimeout = 45 * time.Second to code that first reads
an override (e.g., from an environment variable like TEST_TIMEOUT_SECONDS or
from a testutil config struct) and parses it into a time.Duration, falling back
to 45*time.Second when the env/struct value is absent or invalid; update any
callers that referenced defaultTimeout to use the new exported function or
variable (e.g., TestTimeout() or DefaultTimeout) so tests can be tuned via
env/config while retaining the 45s default.

In `@web/e2e/__tests__/bridges.spec.ts`:
- Around line 445-479: The two helpers waitForBridgeByName and
waitForBridgeStatus currently abort on the first runtime.requestJSON() error;
change them to catch requestJSON failures inside their polling loops, ignore
transient errors (log or store the last error), continue retrying until the
deadline, and only throw after the deadline with a message that includes either
the last observed status (for waitForBridgeStatus) or the last request error
(for waitForBridgeByName) so test flakiness from transient transport/non-2xx
responses is avoided. Ensure the try/catch wraps the await
runtime.requestJSON(...) call in both functions, preserve the existing
delay(250) backoff, and include unique identifiers bridgeId/displayName and the
last error/status in the final thrown Error.

In `@web/e2e/__tests__/dashboard.spec.ts`:
- Around line 348-355: The catch in pollRestartStatus is intentionally empty
because runtime.requestJSON can fail while the daemon is restarting; add a short
inline comment above the catch (or inside it) explaining that returning
"restarting" on any error is expected behavior during daemon restart and
references SettingsRestartStatus, pollRestartStatus, runtime.requestJSON and
statusURL so reviewers understand this is an intentional, documented suppression
of errors rather than an accidental omission.

In `@web/e2e/__tests__/extensibility.spec.ts`:
- Around line 649-660: The function useGlobalWorkspaceIfPrompted duplicates an
existing helper in the shared fixture (fixtures/workspace.ts); replace the local
implementation by importing and using the shared helper instead: remove the
local async function useGlobalWorkspaceIfPrompted from extensibility.spec.ts and
add an import of the helper (the exported function from workspace.ts) at the
top, then call that imported function where useGlobalWorkspaceIfPrompted was
previously used; if the fixture export has a different name, map it
appropriately to the local usage so test behavior remains identical.

In `@web/e2e/__tests__/jobs-hardening.spec.ts`:
- Around line 680-694: assertNoJobSensitiveLeak currently only checks
route_state, api_snapshots, page body and daemonLog; add checks for the
browser_console and browser_network artifacts as well by using
runtime.artifactCollector.artifactPath("browser_console") and
("browser_network") and then await
expect(readFileIfExists(<path>)).resolves.not.toMatch(sensitivePattern) for
each; also include the snapshot parameter check already present
(JSON.stringify(snapshot)) and keep existing checks like appPage.textContent and
daemonLog intact so all console/network artifacts are scanned for sensitive
leaks.

In `@web/e2e/__tests__/session-hardening.spec.ts`:
- Around line 213-220: The test stops after calling
runtime.artifactCollector.captureJSON(...) and
browserArtifacts.captureScreenshot(...), so add calls to persist the collected
artifacts and run the leak scanner: call browserArtifacts.persist(appPage) after
captureScreenshot and then invoke assertNoSensitiveLeak(...) with the persisted
artifact identifiers (or the same appPage/context) to validate no secrets
leaked; update the session-hardening.spec.ts flow around the existing
runtime.artifactCollector.captureJSON and browserArtifacts.captureScreenshot
lines to include these calls so the cancel/clear/delete scenario is persisted
and scanned.

In `@web/e2e/__tests__/settings-hardening.spec.ts`:
- Around line 395-404: The extractJSON function currently returns from the first
JSON opening char to the end of stdout which breaks if non-JSON text follows;
update extractJSON to detect whether the JSON starts with "{" or "[" (use the
existing objectIndex/arrayIndex logic), then scan forward from that start to
find the matching closing brace/bracket while properly accounting for nested
structures and string/escape characters, return only the substring between start
and the matching end, and throw an error if no balanced closing bracket is
found; reference the extractJSON function and ensure callers that call
JSON.parse receive only the isolated JSON payload.

In `@web/e2e/__tests__/tasks-coordinator-handoff.spec.ts`:
- Around line 125-126: Add a URL assertion immediately after calling
appPage.goto(runtime.url("/tasks"), { waitUntil: "domcontentloaded" }) and
before invoking tasksUI.openCreate.click() so the test verifies the browser is
actually at runtime.url("/tasks"); locate the navigation call to appPage.goto
and insert an assertion that the current URL matches runtime.url("/tasks")
(using the project's existing test assertion helper or expect-to-have-URL
utility) to improve diagnostics.

In `@web/e2e/__tests__/tasks-hardening.spec.ts`:
- Around line 798-801: The inline .then() on the awaited promise makes control
flow inconsistent; inside captureTaskRunParity replace the chained .then on
requestOperatorJSONOrThrow<TaskRunDetailEnvelope>(...) with an await that
assigns the response to a local variable (e.g., payload) and then set uds to
payload.run; update the uds assignment that currently references the chained
call so it uses the awaited payload.run instead, keeping identifiers
requestOperatorJSONOrThrow, TaskRunDetailEnvelope and uds unchanged.

In `@web/src/routes/_app/settings/__tests__/-network.test.tsx`:
- Around line 183-199: The test should simulate the user typing the sentinel
port instead of preloading it; change the setup so
pageState.envelope.config.port starts with a normal port (e.g., 3000), render
NetworkSettingsPage, then use userEvent to clear and type "-1" into the input
with test id "settings-page-network-port-input", and finally assert that
"settings-page-network-save-invalid" is not in the document and
"settings-page-network-save" is enabled; this ensures the component's validity
logic (the user interaction path in NetworkSettingsPage) is exercised rather
than relying on a preloaded pageState value.

In `@web/src/systems/network/hooks/use-network-create-channel-action.tsx`:
- Around line 39-55: The current createChannel.mutateAsync(...) call uses a
.then/.catch chain; replace it with an async function using try/catch and await
the createChannel.mutateAsync call. Inside try await the response, extract
response.channel.channel, call setCreateDraft(createNetworkChannelDraft()),
setCreateOpen(false), and call navigate({ params: { channel }, to:
"/network/$channel/threads" }); in the catch block capture the error (Error
type-check), and call toast.error(...) with the message; ensure this change is
applied where use-network-create-channel-action.tsx defines the handler so
createChannel.mutateAsync, setCreateDraft, setCreateOpen, navigate and
toast.error are used the same but with async/await.

In `@web/src/systems/settings/components/settings-restart-banner.tsx`:
- Line 88: The component SettingsRestartBanner uses an ad-hoc class string
("font-mono text-[10px] font-semibold tracking-[0.08em]
text-[color:var(--color-text-label)]"); replace this arbitrary Tailwind
typography and color with the design-system tokenized badge classes used
elsewhere (use the project’s tokenized badge/label utility classes instead of
raw font-, tracking-, size- and color- values) so the banner aligns with the
DESIGN.md tokens; update the className on the element in
settings-restart-banner.tsx to use those existing badge token classes.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

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

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ac1db026-25e7-49d3-9cb9-4c51ecc48390

📥 Commits

Reviewing files that changed from the base of the PR and between 82bdede and 5f5552d.

⛔ Files ignored due to path filters (21)

• internal/testutil/acpmock/testdata/browser_sandbox_fixture.json is excluded by !**/*.json
• internal/testutil/acpmock/testdata/browser_session_hardening_fixture.json is excluded by !**/*.json
• internal/testutil/acpmock/testdata/browser_skills_context_fixture.json is excluded by !**/*.json
• packages/ui/README.md is excluded by !**/*.md
• sdk/examples/telegram-reference/extension.toml is excluded by !**/*.toml
• web/e2e/fixtures/__tests__/artifacts.test.ts is excluded by !**/fixtures/**
• web/e2e/fixtures/__tests__/browser-artifact-session.test.ts is excluded by !**/fixtures/**
• web/e2e/fixtures/__tests__/runtime-seed.test.ts is excluded by !**/fixtures/**
• web/e2e/fixtures/__tests__/runtime.test.ts is excluded by !**/fixtures/**
• web/e2e/fixtures/__tests__/scenario-contracts.test.ts is excluded by !**/fixtures/**
• web/e2e/fixtures/__tests__/selectors.test.ts is excluded by !**/fixtures/**
• web/e2e/fixtures/artifacts.ts is excluded by !**/fixtures/**
• web/e2e/fixtures/browser-artifact-session.ts is excluded by !**/fixtures/**
• web/e2e/fixtures/runtime-helpers.ts is excluded by !**/fixtures/**
• web/e2e/fixtures/runtime-seed.ts is excluded by !**/fixtures/**
• web/e2e/fixtures/runtime.ts is excluded by !**/fixtures/**
• web/e2e/fixtures/scenario-contracts.ts is excluded by !**/fixtures/**
• web/e2e/fixtures/selectors.ts is excluded by !**/fixtures/**
• web/e2e/fixtures/test.ts is excluded by !**/fixtures/**
• web/e2e/fixtures/workspace.ts is excluded by !**/fixtures/**
• web/package.json is excluded by !**/*.json

📒 Files selected for processing (81)

• internal/acp/client.go
• internal/acp/failure.go
• internal/acp/failure_test.go
• internal/acp/types.go
• internal/api/core/memory.go
• internal/api/core/memory_workspace_test.go
• internal/api/core/prompt_stream.go
• internal/api/core/prompt_stream_test.go
• internal/cli/cli_integration_test.go
• internal/e2elane/command_wiring_test.go
• internal/session/interfaces.go
• internal/session/manager_hooks_test.go
• internal/session/manager_prompt.go
• internal/session/manager_test.go
• internal/session/prompt_activity.go
• internal/session/prompt_activity_test.go
• internal/session/session.go
• internal/task/interfaces_integration_test.go
• internal/testutil/acpmock/cmd/acpmock-driver/main.go
• internal/testutil/acpmock/driver_test.go
• internal/testutil/testutil.go
• internal/transcript/transcript_test.go
• internal/transcript/ui_messages.go
• packages/site/lib/__tests__/static-route-metadata.test.ts
• packages/ui/src/components/custom/__tests__/page-header.test.tsx
• packages/ui/src/components/custom/page-header.tsx
• sdk/typescript/src/__tests__/integration.test.ts
• web/e2e/__tests__/automation.spec.ts
• web/e2e/__tests__/bridges.spec.ts
• web/e2e/__tests__/dashboard.spec.ts
• web/e2e/__tests__/extensibility.spec.ts
• web/e2e/__tests__/harness-smoke.spec.ts
• web/e2e/__tests__/jobs-hardening.spec.ts
• web/e2e/__tests__/knowledge.spec.ts
• web/e2e/__tests__/network.spec.ts
• web/e2e/__tests__/sandbox.spec.ts
• web/e2e/__tests__/session-hardening.spec.ts
• web/e2e/__tests__/settings-hardening.spec.ts
• web/e2e/__tests__/settings-transport.spec.ts
• web/e2e/__tests__/settings.spec.ts
• web/e2e/__tests__/skills.spec.ts
• web/e2e/__tests__/storybook-bootstrap.spec.ts
• web/e2e/__tests__/tasks-coordinator-handoff.spec.ts
• web/e2e/__tests__/tasks-hardening.spec.ts
• web/e2e/__tests__/triggers-hardening.spec.ts
• web/e2e/scripts/check-nightly-coverage.ts
• web/playwright.config.ts
• web/src/hooks/routes/__tests__/use-knowledge-page.test.tsx
• web/src/hooks/routes/use-knowledge-page.ts
• web/src/hooks/routes/use-tasks-page.ts
• web/src/lib/__tests__/memory-api-contract.test.ts
• web/src/routes/_app/__tests__/-knowledge.test.tsx
• web/src/routes/_app/knowledge.tsx
• web/src/routes/_app/network.tsx
• web/src/routes/_app/settings/__tests__/-network.test.tsx
• web/src/routes/_app/settings/network.tsx
• web/src/routes/_app/tasks.tsx
• web/src/storybook/__tests__/web-storybook-stories-and-fixtures.test.tsx
• web/src/systems/knowledge/adapters/__tests__/knowledge-api.test.ts
• web/src/systems/knowledge/adapters/knowledge-api.ts
• web/src/systems/knowledge/components/__tests__/knowledge-create-dialog.test.tsx
• web/src/systems/knowledge/components/knowledge-create-dialog.tsx
• web/src/systems/knowledge/components/knowledge-decisions-section.tsx
• web/src/systems/knowledge/components/knowledge-detail-panel.tsx
• web/src/systems/knowledge/hooks/__tests__/use-knowledge-actions.test.tsx
• web/src/systems/knowledge/hooks/use-knowledge-actions.ts
• web/src/systems/knowledge/index.ts
• web/src/systems/knowledge/mocks/fixtures.ts
• web/src/systems/knowledge/mocks/index.ts
• web/src/systems/knowledge/types.ts
• web/src/systems/network/hooks/use-network-create-channel-action.tsx
• web/src/systems/network/hooks/use-network-route-view.ts
• web/src/systems/network/index.ts
• web/src/systems/session/components/__tests__/permission-prompt.test.tsx
• web/src/systems/session/components/__tests__/session-chat-runtime-provider.test.tsx
• web/src/systems/session/components/permission-prompt.tsx
• web/src/systems/session/lib/session-toolkit.tsx
• web/src/systems/settings/components/__tests__/settings-restart-banner.test.tsx
• web/src/systems/settings/components/settings-number-input.tsx
• web/src/systems/settings/components/settings-restart-banner.tsx
• web/src/systems/tasks/components/tasks-inbox-view.tsx

[coderabbitai]

🧹 Nitpick comments (1)

web/e2e/__tests__/extensibility.spec.ts (1)

466-479: 💤 Low value

Consider adding a brief comment explaining why errors are swallowed during crash recovery polling.

The empty catch block silently discards exceptions during the recovery poll. While this is reasonable for transient failures expected during crash recovery, a brief inline comment would clarify the intent and satisfy the guideline that every error should be handled or have written justification.

💡 Suggested clarification

       async () => {
         try {
           const recovered = await invokeToolHTTP(
             runtime,
             { query: "recovered-after-crash" },
             "tc-ext-001-crash-recovery"
           );
           return recovered.result.preview ?? "";
-        } catch {
+        } catch {
+          // Transient failures expected while extension process recovers
           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 `@web/e2e/__tests__/extensibility.spec.ts` around lines 466 - 479, Add an
inline comment inside the empty catch block in the crash-recovery polling code
(the try/catch around invokeToolHTTP call using runtime and test id
"tc-ext-001-crash-recovery") explaining that exceptions are intentionally
swallowed because transient errors are expected during crash recovery polling
and that the poll will retry until timeout; reference invokeToolHTTP and the
surrounding polling logic so future readers know this is deliberate rather than
a missing error handler.

🤖 Prompt for all review comments with 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.

Nitpick comments:
In `@web/e2e/__tests__/extensibility.spec.ts`:
- Around line 466-479: Add an inline comment inside the empty catch block in the
crash-recovery polling code (the try/catch around invokeToolHTTP call using
runtime and test id "tc-ext-001-crash-recovery") explaining that exceptions are
intentionally swallowed because transient errors are expected during crash
recovery polling and that the poll will retry until timeout; reference
invokeToolHTTP and the surrounding polling logic so future readers know this is
deliberate rather than a missing error handler.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: c235f16b-d193-4a82-8747-4aff5de0b8d0

📥 Commits

Reviewing files that changed from the base of the PR and between 5f5552d and 0fc54db.

📒 Files selected for processing (22)

• internal/acp/client.go
• internal/session/interfaces.go
• internal/session/manager_test.go
• internal/testutil/acpmock/cmd/acpmock-driver/main.go
• web/e2e/__tests__/bridges.spec.ts
• web/e2e/__tests__/dashboard.spec.ts
• web/e2e/__tests__/extensibility.spec.ts
• web/e2e/__tests__/jobs-hardening.spec.ts
• web/e2e/__tests__/knowledge.spec.ts
• web/e2e/__tests__/network.spec.ts
• web/e2e/__tests__/session-hardening.spec.ts
• web/e2e/__tests__/settings-hardening.spec.ts
• web/e2e/__tests__/tasks-coordinator-handoff.spec.ts
• web/e2e/__tests__/tasks-hardening.spec.ts
• web/src/hooks/routes/use-knowledge-page.ts
• web/src/routes/_app/settings/__tests__/-network.test.tsx
• web/src/systems/knowledge/components/__tests__/knowledge-create-dialog.test.tsx
• web/src/systems/knowledge/components/knowledge-create-dialog.tsx
• web/src/systems/network/hooks/__tests__/use-network-create-channel-action.test.tsx
• web/src/systems/network/hooks/use-network-create-channel-action.tsx
• web/src/systems/settings/components/__tests__/settings-restart-banner.test.tsx
• web/src/systems/settings/components/settings-restart-banner.tsx

✅ Files skipped from review due to trivial changes (3)

• web/src/systems/network/hooks/tests/use-network-create-channel-action.test.tsx
• internal/session/interfaces.go
• web/src/systems/settings/components/tests/settings-restart-banner.test.tsx

🚧 Files skipped from review as they are similar to previous changes (17)

• web/src/systems/knowledge/components/tests/knowledge-create-dialog.test.tsx
• web/src/routes/_app/settings/tests/-network.test.tsx
• web/e2e/tests/tasks-coordinator-handoff.spec.ts
• web/src/systems/network/hooks/use-network-create-channel-action.tsx
• internal/testutil/acpmock/cmd/acpmock-driver/main.go
• web/src/systems/settings/components/settings-restart-banner.tsx
• internal/session/manager_test.go
• web/src/systems/knowledge/components/knowledge-create-dialog.tsx
• internal/acp/client.go
• web/e2e/tests/session-hardening.spec.ts
• web/src/hooks/routes/use-knowledge-page.ts
• web/e2e/tests/knowledge.spec.ts
• web/e2e/tests/settings-hardening.spec.ts
• web/e2e/tests/jobs-hardening.spec.ts
• web/e2e/tests/network.spec.ts
• web/e2e/tests/tasks-hardening.spec.ts
• web/e2e/tests/dashboard.spec.ts