1. src/gateway/server-reload-handlers.ts, line 167-178 (link)

   getInspectableTaskRegistrySummary performs disk I/O inside the deferral poll loop

   getInspectableTaskRegistrySummary() calls reconcileInspectableTasks(), which in turn calls hasBackingSession() for every active task that has a childSessionKey. For subagent/cli runtimes that means loading the session store from disk; for acp it reads ACP session metadata. This function is invoked on every tick of the deferGatewayRestartUntilIdle polling loop.

   Semantically, using the inspectable (reconciled) version is the right choice here — it lets the deferral end immediately once tasks whose backing sessions are already gone project to lost, rather than waiting up to the full 5-minute grace period. But it's worth being aware that at high active-task counts each poll cycle carries per-task file reads.

   No action required unless this proves problematic in practice; just worth documenting the trade-off or adding a note near this call site.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: src/gateway/server-reload-handlers.ts
   Line: 167-178
   
   Comment:
   **`getInspectableTaskRegistrySummary` performs disk I/O inside the deferral poll loop**
   
   `getInspectableTaskRegistrySummary()` calls `reconcileInspectableTasks()`, which in turn calls `hasBackingSession()` for every active task that has a `childSessionKey`. For `subagent`/`cli` runtimes that means loading the session store from disk; for `acp` it reads ACP session metadata. This function is invoked on every tick of the `deferGatewayRestartUntilIdle` polling loop.
   
   Semantically, using the inspectable (reconciled) version is the right choice here — it lets the deferral end immediately once tasks whose backing sessions are already gone project to `lost`, rather than waiting up to the full 5-minute grace period. But it's worth being aware that at high active-task counts each poll cycle carries per-task file reads.
   
   No action required unless this proves problematic in practice; just worth documenting the trade-off or adding a note near this call site.
   
   How can I resolve this? If you propose a fix, please make it concise.

1. 🟡 Task registry operational metadata exposed to non-admin clients via gateway status method

Description

The gateway status request returns getStatusSummary({ includeSensitive: scopes.includes("operator.admin") }), intending to hide sensitive fields from non-admin clients. However, getStatusSummary() now always includes a tasks summary and redactSensitiveStatusSummary() does not remove or coarsen it.

As a result, any client authorized for the status method (e.g., operator.read) can learn internal operational metadata such as:

• total number of tracked tasks
• counts of queued/running tasks
• counts of failures/timeouts/lost tasks
• runtime breakdown (acp/cron/subagent/cli)

This can leak service health and activity patterns to parties that are intentionally not granted admin visibility.

Vulnerable code paths:

• status handler only gates sensitive view by admin scope, but still returns tasks
• redactSensitiveStatusSummary() does not redact tasks

Vulnerable code:

​// src/gateway/server-methods/health.ts
const status = await getStatusSummary({
  includeSensitive: scopes.includes(ADMIN_SCOPE),
});
respond(true, status, undefined);

​// src/commands/status.summary.ts
const tasks = (await loadTaskRegistryMaintenanceModule()).getInspectableTaskRegistrySummary();
...
return includeSensitive ? summary : redactSensitiveStatusSummary(summary);

​// src/commands/status.summary.ts
export function redactSensitiveStatusSummary(summary: StatusSummary): StatusSummary {
  return {
    ...summary,
    sessions: { ... },
    ​// tasks is not removed/redacted
  };
}

Recommendation

Decide whether tasks is admin-only data.

If it should be admin-only, redact it when includeSensitive is false:

​// src/commands/status.summary.ts
import { createEmptyTaskRegistrySummary } from "../tasks/task-registry.summary.js";

export function redactSensitiveStatusSummary(summary: StatusSummary): StatusSummary {
  return {
    ...summary,
    tasks: createEmptyTaskRegistrySummary(), // or omit tasks entirely
    sessions: {
      ...summary.sessions,
      paths: [],
      defaults: { model: null, contextTokens: null },
      recent: [],
      byAgent: summary.sessions.byAgent.map((entry) => ({ ...entry, path: "[redacted]", recent: [] })),
    },
  };
}

Alternatively, gate inclusion at the source:

const tasks = includeSensitive
  ? (await loadTaskRegistryMaintenanceModule()).getInspectableTaskRegistrySummary()
  : createEmptyTaskRegistrySummary();

Add a test asserting tasks is redacted/empty for non-admin callers of the gateway status method.

2. 🟡 Prototype pollution / DoS via unvalidated task.status and task.runtime used as object keys in summarizeTaskRecords

Description

summarizeTaskRecords() aggregates counts by writing to plain JavaScript objects using dynamic keys derived from TaskRecord fields:

• summary.byStatus[task.status] += 1
• summary.byRuntime[task.runtime] += 1

Although TypeScript types restrict status/runtime, persisted task records are loaded from JSON without runtime validation. loadTaskRegistrySnapshotFromJson() accepts arbitrary objects from runs.json and restores them directly into the in-memory registry (restoreTaskRegistryOnce()), meaning a tampered/corrupted state file can inject unexpected strings like "__proto__", "constructor", or "toString" into task.status / task.runtime.

Impact:

• Availability (DoS): += 1 on inherited/accessor properties (e.g., __proto__) can yield unexpected coercions and may throw in some environments or break later logic by overwriting methods (e.g., toString).
• Prototype pollution class risk: assigning to __proto__ can invoke the special setter, potentially mutating the prototype chain (depending on the value type and engine semantics), which can cause unpredictable behavior.

Vulnerable code:

summary.byStatus[task.status] += 1;
summary.byRuntime[task.runtime] += 1;

Because task records are restored from disk with minimal checks (only taskId is validated), this is reachable when an attacker (or another local process) can modify the state directory (OPENCLAW_STATE_DIR/.../tasks/runs.json).

Recommendation

Defend in depth by (1) validating/normalizing restored records and/or (2) making the aggregation maps safe against special keys.

Option A (preferred): Validate on restore (or before summarizing)

• Accept only known TaskStatus/TaskRuntime values; otherwise coerce to a safe default or skip.

const STATUS: ReadonlySet<string> = new Set([
  "queued","running","succeeded","failed","timed_out","cancelled","lost",
]);
const RUNTIME: ReadonlySet<string> = new Set(["subagent","acp","cli","cron"]);

function safeStatus(value: unknown): TaskStatus {
  return typeof value === "string" && STATUS.has(value) ? (value as TaskStatus) : "queued";
}
function safeRuntime(value: unknown): TaskRuntime {
  return typeof value === "string" && RUNTIME.has(value) ? (value as TaskRuntime) : "cli";
}

​// when restoring or summarizing:
const status = safeStatus(task.status);
const runtime = safeRuntime(task.runtime);
summary.byStatus[status] += 1;
summary.byRuntime[runtime] += 1;

Option B: Use null-prototype objects or Map

const byStatus = Object.create(null) as Record<string, number>;
​// initialize known keys to 0, then increment

Also consider tightening loadTaskRegistrySnapshotFromJson() to validate required fields (including status and runtime) instead of restoring arbitrary objects.

3. 🟡 Potential DoS: restart deferral polling performs expensive full task reconciliation and disk I/O

Description

getInspectableTaskRegistrySummary() summarizes tasks by calling reconcileInspectableTasks(), which:

• Calls listTaskRecords() (builds an array of all tasks and sorts by createdAt => O(n log n) + allocations)
• Maps every task through reconcileTaskRecordForOperatorInspection()
• For some tasks, reconciliation can perform filesystem reads via loadSessionStore() / readAcpSessionEntry() when determining whether to mark tasks as lost

This expensive summary is now invoked inside restart deferral checks that can be polled every 500ms (see deferGatewayRestartUntilIdle default) via setPreRestartDeferralCheck / scheduleGatewaySigusr1Restart. If the task registry grows large, or if many tasks trigger hasBackingSession() checks, a restart deferral window can cause repeated heavy CPU + disk I/O, potentially allowing resource exhaustion (DoS) by repeatedly triggering restarts while many tasks exist.

Vulnerable code:

export function getInspectableTaskRegistrySummary(): TaskRegistrySummary {
  return summarizeTaskRecords(reconcileInspectableTasks());
}

and used in a hot polling path:

setPreRestartDeferralCheck(() => ... + getInspectableTaskRegistrySummary().active)

and deferGatewayRestartUntilIdle polls getPendingCount() every 500ms by default.

Recommendation

Make restart/health checks use a lightweight active-task counter that does not sort/reconcile every task and never performs disk I/O.

Suggested changes:

1. Add an O(1) / O(n) in-memory counter (no sorting) for active tasks, e.g. by iterating tasks.values() without listTaskRecords() sorting, and do not call hasBackingSession() in this path.
2. Cache/throttle getInspectableTaskRegistrySummary() (e.g., memoize for 1–5s) if it must be used in frequently-called paths.
3. Keep the expensive reconciliation (that may touch disk) for explicit operator/diagnostic commands only.

Example (lightweight active count):

​// task-registry.ts
export function countActiveTasks(): number {
  ensureTaskRegistryReady();
  let active = 0;
  for (const task of tasks.values()) {
    if (task.status === "queued" || task.status === "running") active++;
  }
  return active;
}

​// server.impl.ts
setPreRestartDeferralCheck(() =>
  getTotalQueueSize() +
  getTotalPendingReplies() +
  getActiveEmbeddedRunCount() +
  countActiveTasks(),
);