fix(deepseek): review-driven hardening across the harness integration

Fifteen review findings on the dsh mode, the serious ones first: - Multi-user: DEEPSEEK_BASE_URL joins the owner-clamped env keys. _configureDeepSeek() forwards the SERVER's own DEEPSEEK_API_KEY into every dsh pane and applyEnvOverrides() lands after it, so a non-granted owner who could redirect the base URL would have the operator's key sent as a bearer credential to a host of their choosing. - Wait registry: until=stop/blocked is refused on docker and remote-SSH dsh sessions (new deepSeekBridgeUnreachable fact in sessionHookOptions). The HERDR triple is set via LOCAL tmux setenv, which crosses neither docker exec nor ssh, so such a session can never post a hook event and the wait burned its whole timeout on every turn. - Approvals: a dsh item is an ALERT, not an answerable card. The answer route refuses (the '1'/Esc keystrokes are Claude-dialog-shaped and the option parser cannot read a third-party TUI's frames, so an answer was a blind keystroke into a foreign composer), and the push notification carries no Approve/Deny actions for dsh sessions. - Status shim (v3): --seq is forwarded and the server drops stale retried reports inside a 60s window (the TUI retries with backoff, so a retried 'working' could land after 'blocked' and resolve an approval whose dialog was still on screen); 4xx responses exit 0 instead of retrying, so one misconfigured session cannot feed the auth rate-limit bucket until the hook endpoint 429s for the whole instance. - Web-UI server: concurrent starts are serialized through a lock (two racing POSTs used to pick the same port and orphan the winner), and the readiness poll / timeout paths only clear or stop the singleton while it is still theirs. First click actually opens the tab now (refreshWebviews, not the nonexistent loadWebviews). DELETE /api/deepseek/web requires the privileged grant in multi-user mode. - Cron: deepseek jobs run the same two-part launch gate as the HTTP create paths (impl moved into the resolver so all three share it) and no longer stamp a Claude default model on the session. - Parity sweeps: quick-start's docker branch rejects deepSeekConfig like the remote branch; the Ralph auto-enable list gained deepseek; HookEventType gained agent_working; the phone overview run menu filters managed webview records like the desktop menu. - install.sh: the dsh identity probe closes stdin (under curl|bash a child that reads stdin eats the rest of the script), bounds the exec with timeout where available, and is memoized to one scan per install. - Welcome screen: .welcome-btn-deepseek styled in the #4d6bfe brand identity (it rendered as an unstyled UA-grey button); stale markup comment about the web shortcut rewritten; clamp docs updated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-10-04 14:39:42 +02:00 · 2026-08-25 19:01:29 +02:00
parent c30dfaf0e7
commit a628737d1f
18 changed files with 311 additions and 74 deletions
@@ -657,7 +657,8 @@
              </button>
              <!-- The DeepSeek Harness browser UI is the vendor's OWN interactive
                   surface (the terminal one is third-party), so it gets a shortcut:
-                   this starts `dsh web` in a shell session and opens it as a tab.
+                   POST /api/deepseek/web starts a background `dsh web` fenced to
+                   this origin, and the URL opens as a managed web tab.
                   Shown only when dsh is installed. -->
              <button class="run-mode-option run-mode-option--web" id="runModeDeepSeekWeb" style="display: none;" onclick="app.runDeepSeekWeb()">
                <span class="run-mode-dot deepseek"></span>DeepSeek web UI&hellip;
@@ -581,6 +581,11 @@ Object.assign(CodemanApp.prototype, {
    menu.appendChild(header);

    for (const webview of this.webviews ? this.webviews.values() : []) {
+      // Managed records are Codeman-owned shortcut state (the DeepSeek web UI
+      // writes one), not saved dashboards: same filter as the desktop run menu,
+      // or the phone picker lists a stale 127.0.0.1:<port> row that dies on the
+      // next server restart with no affordance here to restart it.
+      if (webview.managed) continue;
      const option = document.createElement('button');
      option.type = 'button';
      option.className = 'mobile-overview-run-option';
@@ -577,7 +577,11 @@ Object.assign(CodemanApp.prototype, {
        if (!wvData.success) throw new Error(wvData.error || 'Failed to save the web tab');
        webview = wvData.data.webview || wvData.data;
      }
-      await this.loadWebviews?.();
+      // refreshWebviews, not a hopeful optional-chain: openWebview() reads
+      // this.webviews and silently no-ops on an id it has not loaded, so
+      // skipping the refresh made the FIRST click create the record but open
+      // nothing (the SSE round-trip had not landed yet).
+      await this.refreshWebviews?.();

      this._appendSessionLaunchStatus(ownsLaunchTerminal, `Serving on ${url} - opening it as a tab.`);
      if (webview?.id) await this.openWebview(webview.id);
@@ -3892,6 +3892,24 @@ body.touch-device .terminal-container .xterm .xterm-helper-textarea {
  transform: translateY(-1px);
 }

+/* DeepSeek Harness: the #4d6bfe blue identity, matching
+   .btn-toolbar.btn-run.mode-deepseek and .run-mode-dot.deepseek so the welcome
+   action reads as the same backend. */
+.welcome-btn-deepseek {
+  background: linear-gradient(135deg, #101a4d 0%, #2740c4 55%, #4d6bfe 100%);
+  border-color: rgba(124, 147, 255, 0.4);
+  color: #eef2ff;
+  box-shadow: 0 2px 8px rgba(77, 107, 254, 0.16), inset 0 1px 0 rgba(255, 255, 255, 0.06);
+}
+
+.welcome-btn-deepseek:hover {
+  background: linear-gradient(135deg, #16225f 0%, #3350e6 55%, #6b83ff 100%);
+  box-shadow: 0 4px 20px rgba(77, 107, 254, 0.3), 0 0 40px rgba(39, 64, 196, 0.12), inset 0 1px 0 rgba(255, 255, 255, 0.08);
+  border-color: rgba(150, 170, 255, 0.5);
+  color: #f8faff;
+  transform: translateY(-1px);
+}
+
 .welcome-btn-gemini {
  background: linear-gradient(135deg, #10243f 0%, #174ea6 55%, #4f46e5 100%);
  border-color: rgba(96, 165, 250, 0.4);
@@ -102,6 +102,19 @@ export function registerApprovalRoutes(app: FastifyInstance, ctx: SessionPort):
    if (!hooksAvailableForMode(session.mode, sessionHookOptions(session))) {
      return createErrorResponse(ApiErrorCode.CONFLICT, 'Session mode cannot have pending approvals');
    }
+    // A dsh approval is an ALERT, not an answerable card: the dialog belongs to
+    // a third-party TUI whose keystroke contract Codeman has not measured, the
+    // Claude-shaped option parser never reads options off its frames, and
+    // verifyStillAnswerable() can therefore never be conclusive for it — so the
+    // '1'/Esc below would be a blind keystroke into a foreign composer. The item
+    // still raises the red alert and clears on the harness's own working/stop
+    // reports; answering happens in the terminal.
+    if (session.mode === 'deepseek') {
+      return createErrorResponse(
+        ApiErrorCode.INVALID_INPUT,
+        'DeepSeek Harness approvals must be answered in the terminal: the dialog belongs to a third-party TUI whose keystrokes Codeman cannot verify.'
+      );
+    }

    // Re-capture the pane before aiming keystrokes at it: if the dialog was
    // answered in the terminal moments ago, the digit would land in whatever
@@ -36,6 +36,23 @@ const APPROVAL_KIND_BY_EVENT: Record<string, ApprovalKind> = {
 */
 const APPROVAL_RESOLVING_EVENTS = new Set(['stop', 'elicitation_complete', 'elicitation_response', 'agent_working']);

+/**
+ * Last DeepSeek status-bridge sequence number seen per session.
+ *
+ * The Herdr contract the dsh TUI speaks stamps every report with `--seq <n>`
+ * and RETRIES failed deliveries with backoff — so a stale report can land
+ * AFTER a newer one, and applying it in arrival order resolves an approval
+ * with a retried `working` while the harness sits blocked, or releases a wait
+ * with a retried `idle` mid-turn. A report whose seq is not newer than the
+ * last accepted one is dropped, but only inside a short window: the TUI's
+ * retry backoff is seconds, so a LOWER seq arriving after the window is a
+ * restarted TUI's fresh numbering (same pane, new generation), not a stale
+ * retry, and must be accepted. Insertion-order eviction bounds the map.
+ */
+const dshSeqBySession = new Map<string, { seq: number; at: number }>();
+const DSH_SEQ_STALE_WINDOW_MS = 60_000;
+const DSH_SEQ_MAX_SESSIONS = 500;
+
 export function registerHookEventRoutes(
  app: FastifyInstance,
  ctx: SessionPort & EventPort & RespawnPort & ConfigPort & InfraPort
@@ -46,6 +63,22 @@ export function registerHookEventRoutes(
      return createErrorResponse(ApiErrorCode.NOT_FOUND, 'Session not found');
    }

+    // DeepSeek status-bridge ordering: drop a stale retried report (see
+    // dshSeqBySession above). Success rather than an error, so the shim exits 0
+    // and the TUI does not keep retrying a report that will stay stale.
+    if (data && data.source === 'dsh-status-shim' && typeof data.seq === 'number') {
+      const last = dshSeqBySession.get(sessionId);
+      const now = Date.now();
+      if (last && data.seq <= last.seq && now - last.at < DSH_SEQ_STALE_WINDOW_MS) {
+        return {};
+      }
+      if (!dshSeqBySession.has(sessionId) && dshSeqBySession.size >= DSH_SEQ_MAX_SESSIONS) {
+        const oldest = dshSeqBySession.keys().next().value;
+        if (oldest !== undefined) dshSeqBySession.delete(oldest);
+      }
+      dshSeqBySession.set(sessionId, { seq: data.seq, at: now });
+    }
+
    // Wake anything blocked on `GET /api/sessions/:id/wait`. Hooks are the only
    // DEFINITIVE signals Codeman gets (`idle` is inferred from output stabilization
    // and can flap mid-turn), so these two are what an orchestrating agent should
@@ -163,12 +196,15 @@ export function registerHookEventRoutes(
    // the browser loaded with. Debounced, so a hook burst costs one broadcast.
    ctx.broadcastSessionStateDebounced(sessionId);

-    // Send push notifications for hook events
+    // Send push notifications for hook events. Push Approve/Deny actions ride
+    // on approvalId, and the answer route refuses keystrokes for dsh dialogs
+    // (third-party TUI, unmeasured contract) — so a dsh push stays a plain
+    // notification instead of offering buttons whose answer would be refused.
    ctx.sendPushNotifications(`hook:${event}`, {
      sessionId,
      sessionName,
      ...safeData,
-      ...(approvalId && { approvalId }),
+      ...(approvalId && session?.mode !== 'deepseek' && { approvalId }),
    });

    // Track in run summary
@@ -394,11 +394,12 @@ export const _clampExternalCliBypassForOwner = clampExternalCliBypassForOwner;

 /**
 * Env-var keys a non-granted owner must not be able to set, because each one
- * hands back privilege the config clamp above just removed.
+ * hands back privilege the config clamp above just removed — or, for the last,
+ * redirects a credential the server injects.
 *
- * Both are DeepSeek's, and both are reachable because `DSH_*` is an allowlisted
- * `envOverrides` prefix (schemas.ts) — which it has to be, since that is also how
- * a user configures the harness's non-privileged knobs.
+ * All are DeepSeek's, and all are reachable because `DSH_*` and `DEEPSEEK_*` are
+ * allowlisted `envOverrides` prefixes (schemas.ts) — which they have to be, since
+ * that is also how a user configures the harness's non-privileged knobs.
 *
 * - `DSH_PERMISSION_MODE` IS the harness's permission switch. Every other CLI's
 *   bypass is a command-line FLAG, reachable only through the per-CLI config the
@@ -407,8 +408,14 @@ export const _clampExternalCliBypassForOwner = clampExternalCliBypassForOwner;
 * - `DSH_HOME` points the launcher at a profile tree, and a profile's plugin code
 *   executes at BOOT, before any approval row can apply. A user who can write a
 *   workspace can put a profile in it, so this is the wider of the two.
+ * - `DEEPSEEK_BASE_URL` aims the provider endpoint, and `_configureDeepSeek()`
+ *   forwards the SERVER's own `DEEPSEEK_API_KEY` into every dsh pane before
+ *   `applyEnvOverrides()` runs — so a non-granted owner who could set the base
+ *   URL would have the operator's API key sent as a bearer credential to a host
+ *   of their choosing. (`DEEPSEEK_API_KEY` itself stays overridable: supplying
+ *   your OWN key removes privilege rather than granting it.)
 */
-const OWNER_CLAMPED_ENV_KEYS = ['DSH_PERMISSION_MODE', 'DSH_HOME'] as const;
+const OWNER_CLAMPED_ENV_KEYS = ['DSH_PERMISSION_MODE', 'DSH_HOME', 'DEEPSEEK_BASE_URL'] as const;

 /**
 * Env-var half of the multi-user bypass clamp.
@@ -456,30 +463,11 @@ export const _clampEnvOverridesForOwner = clampEnvOverridesForOwner;
 * and exits, so both would present as "the tab immediately died".
 */
 async function resolveDeepSeekLaunchError(requestedProfile?: string): Promise<string | null> {
-  const { isDeepSeekAvailable, getDeepSeekNotFoundMessage, listDeepSeekProfiles, resolveDefaultDeepSeekProfile } =
-    await import('../../utils/deepseek-cli-resolver.js');
-  if (!isDeepSeekAvailable()) return getDeepSeekNotFoundMessage();
-
-  const profiles = listDeepSeekProfiles();
-  if (requestedProfile) {
-    const match = profiles.find((p) => p.name === requestedProfile);
-    if (!match) {
-      return `DeepSeek Harness profile "${requestedProfile}" does not exist. Create it with: dsh plugin --profile ${requestedProfile} add <package>`;
-    }
-    if (match.kind === 'web' || match.kind === 'headless') {
-      return `DeepSeek Harness profile "${requestedProfile}" is a ${match.kind} profile and cannot run in a terminal session. Pick an interactive profile, or open the web profile as a Codeman web tab.`;
-    }
-    return null;
-  }
-
-  if (!resolveDefaultDeepSeekProfile()) {
-    return (
-      'No interactive DeepSeek Harness profile is installed. DeepSeek ships only the web and headless ' +
-      'profiles, so the terminal agent comes from a plugin — install one with: ' +
-      'dsh plugin --profile dsh-tui add @deepseek-harness-tui/dsh-tui'
-    );
-  }
-  return null;
+  // Thin async wrapper: the implementation moved into the resolver module so
+  // CRON fires can ask the same question before constructing a Session; the
+  // dynamic import keeps this file's startup free of the probe machinery.
+  const { resolveDeepSeekLaunchError: impl } = await import('../../utils/deepseek-cli-resolver.js');
+  return impl(requestedProfile);
 }

 // ═══════════════════════════════════════════════════════════════
@@ -1299,6 +1287,7 @@ export function registerSessionRoutes(
        session.mode !== 'antigravity' &&
        session.mode !== 'pi' &&
        session.mode !== 'grok' &&
+        session.mode !== 'deepseek' &&
        ctx.store.getConfig().ralphEnabled &&
        !session.ralphTracker.autoEnableDisabled
      ) {
@@ -2921,6 +2910,7 @@ export function registerSessionRoutes(
        antigravityConfig ||
        piConfig ||
        grokConfig ||
+        deepSeekConfig ||
        openCodeConfig
      ) {
        return createErrorResponse(
@@ -547,7 +547,16 @@ export function registerSystemRoutes(
    return { success: true, data: getDeepSeekWebStatus() };
  });

-  app.delete('/api/deepseek/web', async () => {
+  app.delete('/api/deepseek/web', async (req) => {
+    // Same bar as POST: the server is a single shared instance, so in
+    // multi-user mode stopping it out from under other users' tabs is a
+    // privileged act (single-user and granted owners are unaffected).
+    if (isMultiUserMode() && !(await canUsernameRunPrivilegedCommands(getAuthUser(req).username))) {
+      return createErrorResponse(
+        ApiErrorCode.FORBIDDEN,
+        'Stopping the DeepSeek web UI requires the can-bypass-permissions grant'
+      );
+    }
    const { stopDeepSeekWeb } = await import('../../deepseek-web-server.js');
    await stopDeepSeekWeb();
    return { success: true, data: { stopped: true } };
@@ -179,6 +179,16 @@ export interface HookCapabilityOptions {
   * session emit hook events at all.
   */
  deepSeekStatusReporting?: boolean;
+  /**
+   * True when the pane's harness runs somewhere the status bridge cannot reach:
+   * a docker case (`docker exec` does not carry the local tmux env into the
+   * container, and the loopback-bound API is unreachable from it) or a
+   * remote-SSH case (the `HERDR_*` triple is set on the LOCAL ssh process, not
+   * the remote shell). Such a session never posts a hook event however the
+   * statusReporting flag is set, so `until=stop` on it would burn its whole
+   * timeout on every turn.
+   */
+  deepSeekBridgeUnreachable?: boolean;
 }

 /**
@@ -221,7 +231,9 @@ export function hooksAvailableForMode(mode: SessionMode, options: HookCapability
  // deliver `stop` and `blocked` — unless the user turned the bridge off, in
  // which case nothing on the box will ever post one. Every other mode is
  // output-stabilization guesswork and must keep failing the ask.
-  if (mode === 'deepseek') return options.deepSeekStatusReporting !== false;
+  if (mode === 'deepseek') {
+    return options.deepSeekStatusReporting !== false && options.deepSeekBridgeUnreachable !== true;
+  }
  return false;
 }

@@ -234,8 +246,15 @@ export function hooksAvailableForMode(mode: SessionMode, options: HookCapability
 * call sites, so a future per-session fact is added in one place instead of
 * being forgotten at three of them.
 */
-export function sessionHookOptions(session: { deepSeekStatusReporting?: boolean }): HookCapabilityOptions {
-  return { deepSeekStatusReporting: session.deepSeekStatusReporting };
+export function sessionHookOptions(session: {
+  deepSeekStatusReporting?: boolean;
+  docker?: unknown;
+  remote?: unknown;
+}): HookCapabilityOptions {
+  return {
+    deepSeekStatusReporting: session.deepSeekStatusReporting,
+    deepSeekBridgeUnreachable: Boolean(session.docker || session.remote),
+  };
 }

 /** Outcome of resolving a caller-supplied wait target against a session's mode. */
@@ -291,8 +310,11 @@ export function resolveWaitSignals(
      // caller looking for a bug that is really a setting they chose.
      error:
        options.mode === 'deepseek'
-          ? `Signal(s) ${rejected.join(', ')} never fire for this deepseek session: its status bridge is off ` +
-            `(deepSeekConfig.statusReporting: false), so nothing posts hook events. Use idle or exit.`
+          ? options.deepSeekBridgeUnreachable
+            ? `Signal(s) ${rejected.join(', ')} never fire for this deepseek session: it runs in a container or on ` +
+              `a remote host, where the local status bridge cannot reach the harness. Use idle or exit.`
+            : `Signal(s) ${rejected.join(', ')} never fire for this deepseek session: its status bridge is off ` +
+              `(deepSeekConfig.statusReporting: false), so nothing posts hook events. Use idle or exit.`
          : `Signal(s) ${rejected.join(', ')} never fire for ${options.mode} sessions (no Claude Code hooks). Use idle or exit.`,
    };
  }