🤖 refactor: auto-cleanup

src/browser/components/AutomationModal/AutomationModal.tsx

@@ -34,6 +34,12 @@ import {
   workflowScheduleIntervalMinutesToMs,
 } from "@/browser/utils/workflowScheduleIntervalMinutes";
 
+// Fallback messages shown when the save/remove RPC fails without a specific
+// error string. Deduplicated so the wording stays consistent across the
+// pre-check, RPC-result, and thrown-error paths below.
+const SAVE_AUTOMATION_ERROR_MESSAGE = "Failed to save automation.";
+const REMOVE_AUTOMATION_ERROR_MESSAGE = "Failed to remove automation.";
+
 interface AutomationModalProps {
   open: boolean;
   projectPath: string;
@@ -308,7 +314,7 @@ export function AutomationModal(props: AutomationModalProps) {
           schedule: workspaceSchedule,
         });
         if (!result.success) {
-          setSaveError(result.error ?? "Failed to save automation.");
+          setSaveError(result.error ?? SAVE_AUTOMATION_ERROR_MESSAGE);
           return false;
         }
       } else {
@@ -327,15 +333,15 @@ export function AutomationModal(props: AutomationModalProps) {
           },
         });
         if (!result.success) {
-          setSaveError(result.error ?? "Failed to save automation.");
+          setSaveError(result.error ?? SAVE_AUTOMATION_ERROR_MESSAGE);
           return false;
         }
       }
 
       await refreshProjects();
       return true;
     } catch (error) {
-      setSaveError(getErrorMessage(error) || "Failed to save automation.");
+      setSaveError(getErrorMessage(error) || SAVE_AUTOMATION_ERROR_MESSAGE);
       return false;
     } finally {
       setIsSaving(false);
@@ -388,21 +394,21 @@ export function AutomationModal(props: AutomationModalProps) {
           scheduleId: props.projectWorkflowSchedule.id,
         });
         if (!result.success) {
-          throw new Error(result.error ?? "Failed to remove automation.");
+          throw new Error(result.error ?? REMOVE_AUTOMATION_ERROR_MESSAGE);
         }
       } else if (isLegacyWorkspaceSchedule) {
         const result = await api.workspace.setWorkflowSchedule({
           workspaceId: props.workspaceId,
           schedule: null,
         });
         if (!result.success) {
-          throw new Error(result.error ?? "Failed to remove automation.");
+          throw new Error(result.error ?? REMOVE_AUTOMATION_ERROR_MESSAGE);
         }
       }
       await refreshProjects();
       props.onOpenChange(false);
     } catch (error) {
-      setSaveError(getErrorMessage(error) || "Failed to remove automation.");
+      setSaveError(getErrorMessage(error) || REMOVE_AUTOMATION_ERROR_MESSAGE);
     } finally {
       setIsSaving(false);
     }

src/browser/features/Tools/WorkflowActionListToolCall.tsx

@@ -20,6 +20,7 @@ import {
 } from "./Shared/ToolPrimitives";
 import { getStatusDisplay, isToolErrorResult, type ToolStatus } from "./Shared/toolUtils";
 import {
+  BlockedBadge,
   WorkflowBadge,
   WorkflowJsonBlock,
   WorkflowKindBadge,
@@ -126,7 +127,7 @@ function WorkflowActionListRow(props: { descriptor: WorkflowActionDescriptor })
               {action.metadata.effect}
             </WorkflowBadge>
           ) : (
-            <WorkflowBadge tone="danger">blocked</WorkflowBadge>
+            <BlockedBadge />
           )}
         </span>
         <span

src/browser/features/Tools/WorkflowDefinitionToolCall.tsx

@@ -76,6 +76,12 @@ export function WorkflowBadge(props: {
   );
 }
 
+// Blocked workflows/actions share one badge so the "blocked" label and danger
+// tone stay coupled wherever a non-executable entry is shown.
+export function BlockedBadge() {
+  return <WorkflowBadge tone="danger">blocked</WorkflowBadge>;
+}
+
 export function WorkflowSection(props: {
   title: string;
   children: React.ReactNode;
@@ -151,7 +157,7 @@ function WorkflowDefinitionListRow(props: { descriptor: WorkflowDefinitionDescri
       </span>
       <div className="flex items-center gap-1.5">
         <WorkflowBadge>{descriptor.scope}</WorkflowBadge>
-        {!descriptor.executable && <WorkflowBadge tone="danger">blocked</WorkflowBadge>}
+        {!descriptor.executable && <BlockedBadge />}
       </div>
       <div className="min-w-0 [@container(max-width:640px)]:col-span-2">
         <div className="text-muted truncate text-[11px]" title={descriptor.description}>
@@ -195,7 +201,7 @@ export function WorkflowDefinitionCard(props: {
       <div className="flex flex-wrap items-center gap-2">
         <span className="text-foreground font-mono text-[12px] font-medium">{descriptor.name}</span>
         <WorkflowBadge>{descriptor.scope}</WorkflowBadge>
-        {!descriptor.executable && <WorkflowBadge tone="danger">blocked</WorkflowBadge>}
+        {!descriptor.executable && <BlockedBadge />}
       </div>
       {!props.compact && (
         <div className="text-muted mt-1 text-[11px]">{descriptor.description}</div>

src/browser/features/Tools/WorkflowRunToolCall.tsx

@@ -1097,10 +1097,6 @@ export const WorkflowRunToolCall: React.FC<WorkflowRunToolCallProps> = ({
     resetKey: runId,
   });
 
-  const toggleWorkflowExpanded = () => {
-    toggleExpanded();
-  };
-
   const [actionError, setActionError] = useState<string | null>(null);
   const [promotedDefinition, setPromotedDefinition] = useState<WorkflowDefinitionDescriptor | null>(
     null
@@ -1461,7 +1457,7 @@ export const WorkflowRunToolCall: React.FC<WorkflowRunToolCallProps> = ({
 
   return (
     <ToolContainer expanded={expanded}>
-      <ToolHeader onClick={toggleWorkflowExpanded}>
+      <ToolHeader onClick={toggleExpanded}>
         <ExpandIcon expanded={expanded}>▶</ExpandIcon>
         <ToolIcon toolName={toolName} />
         <WorkflowKindBadge />

src/browser/hooks/useModelsFromSettings.test.ts

@@ -87,20 +87,28 @@ const actualRoutingModule = { ...RoutingModule };
 const actualAPIModule = { ...APIModule };
 const actualPolicyContextModule = { ...PolicyContextModule };
 
+// Specifiers mocked by this suite, shared by install/restore so the two helpers can never
+// drift: a path that gets mocked but not restored would silently re-introduce the
+// cross-suite leakage these helpers exist to prevent.
+const PROVIDERS_CONFIG_MODULE = "@/browser/hooks/useProvidersConfig";
+const ROUTING_MODULE = "@/browser/hooks/useRouting";
+const API_MODULE = "@/browser/contexts/API";
+const POLICY_CONTEXT_MODULE = "@/browser/contexts/PolicyContext";
+
 async function installUseModelsModuleMocks() {
-  await mock.module("@/browser/hooks/useProvidersConfig", () => ({
+  await mock.module(PROVIDERS_CONFIG_MODULE, () => ({
     ...actualProvidersConfigModule,
     useProvidersConfig: useProvidersConfigMock,
   }));
-  await mock.module("@/browser/hooks/useRouting", () => ({
+  await mock.module(ROUTING_MODULE, () => ({
     ...actualRoutingModule,
     useRouting: useRoutingMock,
   }));
-  await mock.module("@/browser/contexts/API", () => ({
+  await mock.module(API_MODULE, () => ({
     ...actualAPIModule,
     useAPI: () => ({ api: apiMock }),
   }));
-  await mock.module("@/browser/contexts/PolicyContext", () => ({
+  await mock.module(POLICY_CONTEXT_MODULE, () => ({
     ...actualPolicyContextModule,
     usePolicy: () => ({
       status: { state: "disabled" as const },
@@ -112,10 +120,10 @@ async function installUseModelsModuleMocks() {
 async function restoreUseModelsModuleMocks() {
   // Bun's mock.module() has no disposer, and mock.restore() does not undo module mocks.
   // Restore the real modules so this file cannot poison later tests in the same process.
-  await mock.module("@/browser/hooks/useProvidersConfig", () => actualProvidersConfigModule);
-  await mock.module("@/browser/hooks/useRouting", () => actualRoutingModule);
-  await mock.module("@/browser/contexts/API", () => actualAPIModule);
-  await mock.module("@/browser/contexts/PolicyContext", () => actualPolicyContextModule);
+  await mock.module(PROVIDERS_CONFIG_MODULE, () => actualProvidersConfigModule);
+  await mock.module(ROUTING_MODULE, () => actualRoutingModule);
+  await mock.module(API_MODULE, () => actualAPIModule);
+  await mock.module(POLICY_CONTEXT_MODULE, () => actualPolicyContextModule);
 }
 
 let cleanupDom: (() => void) | null = null;

src/node/services/analytics/etl.ts

@@ -9,9 +9,12 @@ import { getErrorMessage } from "@/common/utils/errors";
 import { createDisplayUsage } from "@/common/utils/tokens/displayUsage";
 import { log } from "@/node/services/log";
 import { toUtcDateString } from "@/node/services/analytics/dateUtils";
-import { CHAT_ARCHIVE_FILE_NAME } from "@/common/constants/paths";
+import { CHAT_FILE_NAME, CHAT_ARCHIVE_FILE_NAME } from "@/common/constants/paths";
 
-export const CHAT_FILE_NAME = "chat.jsonl";
+// Re-export the canonical chat history filename (defined in constants/paths.ts)
+// so existing analytics consumers (workspaceDiscovery, tests) can keep importing
+// it from this module without duplicating the "chat.jsonl" literal.
+export { CHAT_FILE_NAME };
 
 /**
  * Sealed pre-boundary history rotates from chat.jsonl into chat-archive.jsonl

src/node/services/memoryConsolidationService.ts

@@ -177,13 +177,22 @@ function findNewestWorkspaceRecord(
   );
 }
 
+/**
+ * Effective ordering timestamp for a harvest record: when it completed, or when
+ * it started if still pending. findNewestHarvestRecord and pruneHarvestRecords
+ * must rank records the same way, so both derive recency from this single key.
+ */
+function harvestRecordTime(record: MemoryHarvestRecord): number {
+  return record.completedAt ?? record.startedAt;
+}
+
 function findNewestHarvestRecord(
   records: Record<string, MemoryHarvestRecord> | undefined
 ): MemoryHarvestRecord | null {
   if (records === undefined) return null;
   return Object.values(records).reduce<MemoryHarvestRecord | null>((latest, record) => {
-    const recordTime = record.completedAt ?? record.startedAt;
-    const latestTime = latest === null ? -1 : (latest.completedAt ?? latest.startedAt);
+    const recordTime = harvestRecordTime(record);
+    const latestTime = latest === null ? -1 : harvestRecordTime(latest);
     return recordTime > latestTime ? record : latest;
   }, null);
 }
@@ -231,11 +240,9 @@ function parseHarvestRecords(value: unknown): Record<string, Record<string, Memo
 }
 
 function pruneHarvestRecords(records: Record<string, MemoryHarvestRecord>): void {
-  const ranked = Object.entries(records).sort(([, left], [, right]) => {
-    const leftTime = left.completedAt ?? left.startedAt;
-    const rightTime = right.completedAt ?? right.startedAt;
-    return rightTime - leftTime;
-  });
+  const ranked = Object.entries(records).sort(
+    ([, left], [, right]) => harvestRecordTime(right) - harvestRecordTime(left)
+  );
   for (const [boundaryKey] of ranked.slice(HARVEST_RECORD_RETENTION)) {
     delete records[boundaryKey];
   }

src/node/services/workflows/WorkflowRunner.ts

@@ -17,6 +17,7 @@ import { validateJsonSchemaSubset } from "@/common/utils/jsonSchemaSubset";
 import { removeCommonJsWorkflowMetadataDeclaration } from "./staticWorkflowMetadata";
 import { WORKFLOW_RUNTIME_STDLIB_SOURCE } from "./workflowRuntimeSources.generated";
 import type { IJSRuntime, IJSRuntimeFactory } from "@/node/services/ptc/runtime";
+import { isWorkflowRunTaskId } from "@/node/services/tools/taskId";
 import { AsyncMutex } from "@/node/utils/concurrency/asyncMutex";
 import { AsyncSemaphore } from "@/node/utils/concurrency/asyncSemaphore";
 import type { ResolvedWorkflowAction, WorkflowActionRegistry } from "./WorkflowActionRegistry";
@@ -1001,7 +1002,7 @@ export class WorkflowRunner {
     const run = await this.runStore.getRun(runId);
     const driftedStep = run.steps.find(
       (step) =>
-        step.stepId === spec.id && step.inputHash !== inputHash && step.taskId?.startsWith("wfr_")
+        step.stepId === spec.id && step.inputHash !== inputHash && isWorkflowRunTaskId(step.taskId)
     );
     if (driftedStep != null) {
       throw new Error(

src/node/services/workflows/WorkflowService.ts

@@ -2,6 +2,7 @@ import * as crypto from "node:crypto";
 
 import {
   isActiveWorkflowRunStatus,
+  isTerminalWorkflowRunStatus,
   type WorkflowActionDescriptor,
   type WorkflowDefinitionDescriptor,
   type WorkflowRunRecord,
@@ -708,12 +709,7 @@ export class WorkflowService {
     }
 
     const run = await this.getRun({ workspaceId, runId });
-    if (
-      run == null ||
-      run.status === "completed" ||
-      run.status === "failed" ||
-      run.status === "interrupted"
-    ) {
+    if (run == null || isTerminalWorkflowRunStatus(run.status)) {
       return;
     }
 

src/node/services/workflows/workflowArgs.ts

@@ -40,6 +40,11 @@ interface TokenizeResult {
 
 const RAW_INPUT_FIELD = "input";
 
+// Shared by assertObjectSchema and propertySchemas so the two `properties`
+// validators can't drift to differing messages for the same failure.
+const ARGS_SCHEMA_PROPERTIES_ERROR_MESSAGE =
+  "Workflow metadata.argsSchema.properties must be an object";
+
 export function normalizeWorkflowArgsForSource(
   definitionSource: string,
   rawArgs: unknown,
@@ -146,14 +151,14 @@ function assertObjectSchema(schema: Record<string, unknown>): void {
     throw validationError('Workflow metadata.argsSchema must have type "object"');
   }
   if (!isPlainObject(schema.properties)) {
-    throw validationError("Workflow metadata.argsSchema.properties must be an object");
+    throw validationError(ARGS_SCHEMA_PROPERTIES_ERROR_MESSAGE);
   }
 }
 
 function propertySchemas(schema: Record<string, unknown>): ArgsPropertySchema[] {
   const rawProperties = schema.properties;
   if (!isPlainObject(rawProperties)) {
-    throw validationError("Workflow metadata.argsSchema.properties must be an object");
+    throw validationError(ARGS_SCHEMA_PROPERTIES_ERROR_MESSAGE);
   }
 
   return Object.entries(rawProperties).map(([name, rawProperty]) => {

src/node/services/workflows/workspaceHostActions.ts

@@ -118,6 +118,21 @@ const WorkspaceIdInputSchema = z.object({
   workspaceId: z.string().min(1),
 });
 
+/**
+ * Metadata JSON Schema for the id-targeted host actions whose only input is a
+ * workspace id (getLatestAssistantMessage, archive, unarchive). Shared so a new
+ * id-only action can't drift from the rest — #3583's workspace.unarchive
+ * otherwise duplicated this block verbatim. Runtime parsing of these inputs
+ * uses the matching WorkspaceIdInputSchema above.
+ */
+const WORKSPACE_ID_ONLY_INPUT_SCHEMA = {
+  type: "object",
+  properties: {
+    workspaceId: { type: "string" },
+  },
+  required: ["workspaceId"],
+} as const;
+
 function listedWorkspace(metadata: {
   id: string;
   name: string;
@@ -524,13 +539,7 @@ const WORKSPACE_HOST_ACTION_DEFINITIONS: Record<string, WorkspaceHostActionDefin
       version: 1,
       description: "Read the most recent assistant message text from a workspace's chat history",
       effect: "read",
-      inputSchema: {
-        type: "object",
-        properties: {
-          workspaceId: { type: "string" },
-        },
-        required: ["workspaceId"],
-      },
+      inputSchema: WORKSPACE_ID_ONLY_INPUT_SCHEMA,
       outputSchema: { type: "object" },
       timeoutMs: 30_000,
     },
@@ -569,13 +578,7 @@ const WORKSPACE_HOST_ACTION_DEFINITIONS: Record<string, WorkspaceHostActionDefin
       version: 1,
       description: "Archive a workspace (idempotent; succeeds when already archived)",
       effect: "external",
-      inputSchema: {
-        type: "object",
-        properties: {
-          workspaceId: { type: "string" },
-        },
-        required: ["workspaceId"],
-      },
+      inputSchema: WORKSPACE_ID_ONLY_INPUT_SCHEMA,
       outputSchema: { type: "object" },
       timeoutMs: 120_000,
     },
@@ -599,13 +602,7 @@ const WORKSPACE_HOST_ACTION_DEFINITIONS: Record<string, WorkspaceHostActionDefin
       version: 1,
       description: "Unarchive a workspace (idempotent; succeeds when already unarchived)",
       effect: "external",
-      inputSchema: {
-        type: "object",
-        properties: {
-          workspaceId: { type: "string" },
-        },
-        required: ["workspaceId"],
-      },
+      inputSchema: WORKSPACE_ID_ONLY_INPUT_SCHEMA,
       outputSchema: { type: "object" },
       timeoutMs: 120_000,
     },