fix: serve remote daemon request diagnostics to the caller

packages/kernel/src/errors.ts

@@ -44,11 +44,24 @@ export function toAppErrorCode(
   return fallback;
 }
 
+/**
+ * Locator for one request's diagnostics record on the daemon host, in the
+ * daemon's own vocabulary rather than as a filesystem path: `logPath` names
+ * that same record as a path, which only a caller on the daemon host can read.
+ * A remote caller fetches the record by this locator instead
+ * (`GET /sessions/<session>/requests/<requestId>/diagnostics`).
+ */
+export type DiagnosticsRecordRef = {
+  session: string;
+  requestId: string;
+};
+
 /**
  * Details bag for AppError. Free-form context is allowed, but these keys carry
  * meaning at normalize/render time and must keep their types:
  * - `hint` — overrides `defaultHintForCode`; re-wraps preserve an existing hint.
- * - `diagnosticId` / `logPath` — lifted onto the normalized error, stripped from details.
+ * - `diagnosticId` / `logPath` / `logPathUnavailable` / `diagnosticsRecord` —
+ *   lifted onto the normalized error, stripped from details.
  * - `processExitError` + `stdout`/`stderr`/`exitCode` — marks a wrap of a real
  *   process exit so normalizeError can surface the first meaningful stderr line;
  *   build these via `execFailureDetails`/`requireExecSuccess` in src/utils/exec.ts
@@ -60,6 +73,8 @@ export type AppErrorDetails = Record<string, unknown> & {
   hint?: string;
   diagnosticId?: string;
   logPath?: string;
+  logPathUnavailable?: string;
+  diagnosticsRecord?: DiagnosticsRecordRef;
   retriable?: boolean;
   supportedOn?: string;
   processExitError?: boolean;
@@ -75,7 +90,22 @@ export type NormalizedError = {
   message: string;
   hint?: string;
   diagnosticId?: string;
+  /**
+   * Diagnostics record path **the reader of this error can open**. A daemon
+   * renders its own host path here; a client talking to a REMOTE daemon
+   * replaces it with the caller-local copy it fetched, or drops it and sets
+   * `logPathUnavailable` (see `localizeRemoteDaemonError`). A path the reader
+   * cannot open never belongs in this field (#1801).
+   */
   logPath?: string;
+  /**
+   * Why no readable `logPath` could be produced, e.g.
+   * `remote daemon https://host, request 8f2c: 404`. Set only in place of
+   * `logPath`, and never carries a daemon-host path.
+   */
+  logPathUnavailable?: string;
+  /** Locator the record can be fetched by when it lives on a remote daemon. */
+  diagnosticsRecord?: DiagnosticsRecordRef;
   /**
    * Lifted from `details.retriable` when a throw site classified the failure as
    * clearly transient (or clearly not). Included only when set, so the default
@@ -96,7 +126,16 @@ export type DaemonError = {
   message: string;
   hint?: string;
   diagnosticId?: string;
+  /** Path on the DAEMON host. Meaningful to a local caller only (#1801). */
   logPath?: string;
+  /** Why no readable path is named; set by the client, never by the daemon. */
+  logPathUnavailable?: string;
+  /**
+   * Additive locator (#1801) for the request diagnostics record `logPath`
+   * names, so a remote caller can fetch it over the daemon API instead of
+   * being handed a path on a filesystem it cannot read.
+   */
+  diagnosticsRecord?: DiagnosticsRecordRef;
   details?: Record<string, unknown>;
   /** Additive retry and platform-support signals; absent when not derivable. */
   retriable?: boolean;
@@ -123,6 +162,8 @@ export function throwDaemonError(error: DaemonError): never {
     hint: error.hint,
     diagnosticId: error.diagnosticId,
     logPath: error.logPath,
+    logPathUnavailable: error.logPathUnavailable,
+    diagnosticsRecord: error.diagnosticsRecord,
     retriable: error.retriable,
     supportedOn: error.supportedOn,
   });
@@ -140,21 +181,27 @@ export function isAgentDeviceError(err: unknown): err is AppError {
   return err instanceof AppError;
 }
 
+export type NormalizeErrorContext = {
+  diagnosticId?: string;
+  logPath?: string;
+  diagnosticsRecord?: DiagnosticsRecordRef;
+};
+
 export function normalizeAgentDeviceError(
   err: unknown,
-  context: { diagnosticId?: string; logPath?: string } = {},
+  context: NormalizeErrorContext = {},
 ): NormalizedError {
   return normalizeError(err, context);
 }
 
-export function normalizeError(
-  err: unknown,
-  context: { diagnosticId?: string; logPath?: string } = {},
-): NormalizedError {
+export function normalizeError(err: unknown, context: NormalizeErrorContext = {}): NormalizedError {
   const appErr = asAppError(err);
   const details = appErr.details ? redactDiagnosticData(appErr.details) : undefined;
   const diagnosticId = stringDetail(details, 'diagnosticId') ?? context.diagnosticId;
   const logPath = stringDetail(details, 'logPath') ?? context.logPath;
+  const logPathUnavailable = stringDetail(details, 'logPathUnavailable');
+  const diagnosticsRecord =
+    readDiagnosticsRecordRef(details?.diagnosticsRecord) ?? context.diagnosticsRecord;
   const hint = stringDetail(details, 'hint') ?? defaultHintForCode(appErr.code);
   const retriable = booleanDetail(details, 'retriable') ?? retriableForErrorCode(appErr.code);
   const supportedOn = stringDetail(details, 'supportedOn');
@@ -167,6 +214,8 @@ export function normalizeError(
     hint,
     diagnosticId,
     logPath,
+    ...(logPathUnavailable !== undefined ? { logPathUnavailable } : {}),
+    ...(diagnosticsRecord !== undefined ? { diagnosticsRecord } : {}),
     // Typed-error signals stay absent unless confidently known (#939 wire shape).
     ...(retriable !== undefined ? { retriable } : {}),
     ...(supportedOn !== undefined ? { supportedOn } : {}),
@@ -226,6 +275,19 @@ function stringDetail(
   return typeof value === 'string' ? value : undefined;
 }
 
+/**
+ * Narrows an untrusted `diagnosticsRecord` — off the wire or out of a details
+ * bag — to the locator type, or `undefined`. One reader, so a daemon payload
+ * and a details bag can never be accepted on different terms.
+ */
+export function readDiagnosticsRecordRef(value: unknown): DiagnosticsRecordRef | undefined {
+  if (!value || typeof value !== 'object') return undefined;
+  const { session, requestId } = value as Partial<DiagnosticsRecordRef>;
+  if (typeof session !== 'string' || typeof requestId !== 'string') return undefined;
+  if (session.length === 0 || requestId.length === 0) return undefined;
+  return { session, requestId };
+}
+
 function booleanDetail(
   details: Record<string, unknown> | undefined,
   key: string,
@@ -242,6 +304,8 @@ function stripDiagnosticMeta(
   delete output.hint;
   delete output.diagnosticId;
   delete output.logPath;
+  delete output.logPathUnavailable;
+  delete output.diagnosticsRecord;
   delete output.retriable;
   delete output.supportedOn;
   return Object.keys(output).length > 0 ? output : undefined;

src/__tests__/cli-remote-diagnostics.test.ts

@@ -0,0 +1,180 @@
+/**
+ * #1801: a caller driving a REMOTE daemon must never be handed a diagnostics
+ * path on the daemon's filesystem. These run the real CLI against a real daemon
+ * HTTP server on loopback — the only place the whole chain (daemon error →
+ * locator → fetch → rendered line) is observable end to end.
+ */
+
+import { test } from 'vitest';
+import assert from 'node:assert/strict';
+import fs from 'node:fs';
+import path from 'node:path';
+import { normalizeError, AppError } from '@agent-device/kernel/errors';
+import { createDaemonHttpServer } from '../daemon/server/http-server.ts';
+import { resolveSessionRequestLogPath } from '../daemon/session-store.ts';
+import { safeSessionName } from '../daemon/session-paths.ts';
+import type { DaemonRequest, DaemonResponse } from '../daemon/types.ts';
+import { runCliCapture, type CapturedCliRun } from './cli-capture.ts';
+import {
+  closeLoopbackServer,
+  listenOnLoopback,
+  skipWhenLoopbackUnavailable,
+} from './test-utils/index.ts';
+import { mkdtempForTestSync } from './test-utils/tmp-dir.ts';
+
+const DAEMON_TOKEN = 'daemon-secret';
+const DAEMON_SESSION = 'cwd:abcdef0123456789:default';
+const RECORD_SENTINEL = 'REMOTE_RECORD_SENTINEL';
+
+type RemoteRun = {
+  run: CapturedCliRun;
+  clientStateDir: string;
+  daemonLogPath: string;
+  requestId: string;
+};
+
+/**
+ * Runs `argv` against a loopback daemon that always fails the command and
+ * writes the request's diagnostics record, exactly as the real router does.
+ * `serveRecord: false` models a daemon with no diagnostics route (an older
+ * release), so the fetch fails.
+ */
+async function runAgainstRemoteDaemon(
+  argv: string[],
+  options: { serveRecord: boolean },
+): Promise<RemoteRun> {
+  const clientStateDir = mkdtempForTestSync('agent-device-remote-client-');
+  const daemonStateDir = mkdtempForTestSync('agent-device-remote-daemon-');
+  const daemonSessionsDir = path.join(daemonStateDir, 'sessions');
+  const resolveRecordPath = (session: string, requestId: string | undefined): string =>
+    resolveSessionRequestLogPath(path.join(daemonSessionsDir, safeSessionName(session)), requestId);
+  let requestId = '';
+  let daemonLogPath = '';
+
+  const handleRequest = async (req: DaemonRequest): Promise<DaemonResponse> => {
+    requestId = req.meta?.requestId ?? '';
+    daemonLogPath = resolveRecordPath(DAEMON_SESSION, requestId);
+    fs.mkdirSync(path.dirname(daemonLogPath), { recursive: true });
+    fs.writeFileSync(daemonLogPath, `{"phase":"request_failed","data":"${RECORD_SENTINEL}"}\n`);
+    return {
+      ok: false,
+      error: normalizeError(new AppError('COMMAND_FAILED', 'wait timed out'), {
+        logPath: daemonLogPath,
+        diagnosticsRecord: { session: DAEMON_SESSION, requestId },
+      }),
+    };
+  };
+
+  const server = await createDaemonHttpServer({
+    token: DAEMON_TOKEN,
+    handleRequest,
+    ...(options.serveRecord
+      ? { resolveRequestDiagnosticsPath: (ref) => resolveRecordPath(ref.session, ref.requestId) }
+      : {}),
+  });
+
+  try {
+    const port = await listenOnLoopback(server);
+    const run = await runCliCapture(argv, {
+      useRealDaemonClient: true,
+      env: {
+        AGENT_DEVICE_STATE_DIR: clientStateDir,
+        AGENT_DEVICE_DAEMON_BASE_URL: `http://127.0.0.1:${port}`,
+        AGENT_DEVICE_DAEMON_AUTH_TOKEN: DAEMON_TOKEN,
+      },
+    });
+    return { run, clientStateDir, daemonLogPath, requestId };
+  } finally {
+    await closeLoopbackServer(server);
+    fs.rmSync(daemonStateDir, { recursive: true, force: true });
+  }
+}
+
+test('a remote failure names a record the caller can actually read', async (t) => {
+  if (await skipWhenLoopbackUnavailable(t)) return;
+
+  const { run, clientStateDir, daemonLogPath, requestId } = await runAgainstRemoteDaemon(
+    ['clipboard', 'write', 'hello'],
+    { serveRecord: true },
+  );
+  try {
+    const localPath = path.join(
+      clientStateDir,
+      'remote-diagnostics',
+      safeSessionName(DAEMON_SESSION),
+      'requests',
+      `${requestId}.ndjson`,
+    );
+    assert.equal(run.code, 1);
+    assert.match(run.stderr, new RegExp(`Diagnostics Log: ${escapeRegExp(localPath)}`));
+    assert.equal(run.stderr.includes(daemonLogPath), false, 'daemon-host path must not be printed');
+    assert.match(fs.readFileSync(localPath, 'utf8'), new RegExp(RECORD_SENTINEL));
+  } finally {
+    fs.rmSync(clientStateDir, { recursive: true, force: true });
+  }
+});
+
+test('--json carries the caller-local record path, never the daemon-host one', async (t) => {
+  if (await skipWhenLoopbackUnavailable(t)) return;
+
+  const { run, clientStateDir, daemonLogPath } = await runAgainstRemoteDaemon(
+    ['clipboard', 'write', 'hello', '--json'],
+    { serveRecord: true },
+  );
+  try {
+    const payload = JSON.parse(run.stdout) as { error: { logPath?: string } };
+    assert.equal(payload.error.logPath?.startsWith(clientStateDir), true);
+    assert.equal(fs.existsSync(payload.error.logPath ?? ''), true);
+    assert.equal(run.stdout.includes(daemonLogPath), false);
+  } finally {
+    fs.rmSync(clientStateDir, { recursive: true, force: true });
+  }
+});
+
+test('--debug prints the fetched record inline and not the local daemon log', async (t) => {
+  if (await skipWhenLoopbackUnavailable(t)) return;
+
+  const { run, clientStateDir } = await runAgainstRemoteDaemon(
+    ['clipboard', 'write', 'hello', '--debug'],
+    { serveRecord: true },
+  );
+  try {
+    // A local daemon log in the caller's state dir must stay unread: it belongs
+    // to a different daemon than the one that failed.
+    assert.equal(run.stderr.includes('[daemon log]'), false);
+    assert.match(run.stderr, /\[remote diagnostics\]/);
+    assert.match(run.stderr, new RegExp(RECORD_SENTINEL));
+  } finally {
+    fs.rmSync(clientStateDir, { recursive: true, force: true });
+  }
+});
+
+test('an unfetchable remote record says so instead of naming a path', async (t) => {
+  if (await skipWhenLoopbackUnavailable(t)) return;
+
+  const { run, clientStateDir, daemonLogPath, requestId } = await runAgainstRemoteDaemon(
+    ['clipboard', 'write', 'hello', '--json'],
+    { serveRecord: false },
+  );
+  try {
+    const payload = JSON.parse(run.stdout) as {
+      error: { logPath?: string; logPathUnavailable?: string };
+    };
+    // A path may still be named — the CLI's own client-side record, written on
+    // this machine — but never the daemon host's, and never a record the fetch
+    // failed to produce.
+    assert.notEqual(payload.error.logPath, daemonLogPath);
+    assert.equal(payload.error.logPath?.includes('remote-diagnostics') ?? false, false);
+    assert.match(
+      payload.error.logPathUnavailable ?? '',
+      new RegExp(`^remote daemon http://127\\.0\\.0\\.1:\\d+, request ${requestId}: HTTP 404$`),
+    );
+    assert.equal(run.stdout.includes(daemonLogPath), false);
+  } finally {
+    fs.rmSync(clientStateDir, { recursive: true, force: true });
+  }
+});
+
+function escapeRegExp(value: string): string {
+  return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+}