fix(headless): keep --json stderr free of commander's error print

Address the ds-review-bot findings on 8724223621: - The JSON error override throws commander's control-flow error instead of delegating to `program.error`, whose own printer wrote an unprefixed `error: …` line to stderr and broke the documented `--json` contract. - Cover the post-idle adoptability re-check with a focused case: an overlay that appends a preset while the runner awaits idle is still rejected, and the stale "live Session" comment now names that residual writer. - Scope the Agent Note change sentence to the product change so it no longer claims the whole patch is confined to one package.
2026-09-10 16:25:19 +08:00
1 parent 9098a58f94
commit 31be030ccc
@@ -2,5 +2,5 @@
 # side as of the last confirmed-consistent state. Both languages carry equal authority;
 # after editing either side, bring the other along and re-record with:
 #   pnpm run verify-translation-pairing --write .agents/notes/implemented/feature/2026-09-09-headless-machine-readable-run-surface.md
-2026-09-09-headless-machine-readable-run-surface.md: c436e57c4c3461a1d3700ce873041e3b8ab9a603
-2026-09-09-headless-machine-readable-run-surface.zh.md: b55ec5f7df5c86720d9aa611594bbadd335a1b31
+2026-09-09-headless-machine-readable-run-surface.md: e49efb3761a8c43d06fff3f3d10de68eef16dcc3
+2026-09-09-headless-machine-readable-run-surface.zh.md: 465d07b966576b08ed0085da752ec8da647eec58
@@ -22,7 +22,7 @@ Three additions extend the app-owned command line that [Apps own their command l

 A per-run `--model` override is deliberately out of scope; the composition default stays authoritative.

-The change is confined to `packages/bundle/headless`: `src/startup.ts`, `src/index.ts`, the new `src/json-stream.ts`, the package manifest and `tsconfig.json`, and its tests. The product-profile expectation test in `apps/cli/tests/profiles/headless/tests/headless.expected.e2e.ts` covers both output modes end to end, which adds one optional caller-owned `cwd` to the `packages/test-support/loader-smoke` harness so two wakes can share a world. No core session, persistence, session-controller, base composition, or launcher file changes.
+The product change is confined to `packages/bundle/headless`: `src/startup.ts`, `src/index.ts`, the new `src/json-stream.ts`, the package manifest and `tsconfig.json`, and its tests. Around it, the product-profile expectation test in `apps/cli/tests/profiles/headless/tests/headless.expected.e2e.ts` covers both output modes end to end and adds one optional caller-owned `cwd` to the `packages/test-support/loader-smoke` harness so two wakes can share a world, while `scripts/check-workspace-constraints.ts`, `pnpm-lock.yaml`, and the package manifest publish the shared `lib/json-stream-*.js` chunk both entries import. No core session, persistence, session-controller, base composition, or launcher file changes.

 ### Command-line contract

@@ -22,7 +22,7 @@ Status: implemented

 每次运行的 `--model` 覆盖被明确排除在范围之外；组合默认模型仍然权威。

-改动范围限于 `packages/bundle/headless`：`src/startup.ts`、`src/index.ts`、新增的 `src/json-stream.ts`、包清单与 `tsconfig.json`，以及测试。产品 profile 的期望测试位于 `apps/cli/tests/profiles/headless/tests/headless.expected.e2e.ts`，端到端覆盖两种输出模式；为此给 `packages/test-support/loader-smoke` harness 增加了一个可选的调用方自有 `cwd`，让两次唤醒共享同一个世界。不修改任何 core session、持久化、session-controller、base 组合或 launcher 文件。
+产品改动限于 `packages/bundle/headless`：`src/startup.ts`、`src/index.ts`、新增的 `src/json-stream.ts`、包清单与 `tsconfig.json`，以及测试。围绕它，产品 profile 的期望测试位于 `apps/cli/tests/profiles/headless/tests/headless.expected.e2e.ts`，端到端覆盖两种输出模式，并给 `packages/test-support/loader-smoke` harness 增加了一个可选的调用方自有 `cwd`，让两次唤醒共享同一个世界；`scripts/check-workspace-constraints.ts`、`pnpm-lock.yaml` 与包清单负责发布两个入口共同引用的共享 chunk `lib/json-stream-*.js`。不修改任何 core session、持久化、session-controller、base 组合或 launcher 文件。

 ### 命令行契约

@@ -359,8 +359,9 @@ async function run(ctx: Context, config: Config, io: HeadlessIo): Promise<void>
    : await resolveAgent(ctx, agents, sessionId, agentOptions, setup)
  await agent.whenIdle()
  if (config.sessionId !== undefined) {
-    // A live Session can select an agent preset while the runner awaits idle;
-    // re-read its log so the rejection cannot be outrun by that timing.
+    // The resume-time check read a snapshot; an overlay can still append a
+    // preset selection between it and the interval this run now owns, so
+    // re-read the log the runner holds before submitting the task.
    assertAdoptable(agent.session.header, liveEvents(agent.session), sessionId)
  }
  const firstSeq = agent.session.seq
@@ -6,7 +6,7 @@
 * @module @deepseek-ai/dsh-headless/startup
 */

-import { Command } from 'commander'
+import { Command, CommanderError } from 'commander'
 import type { Context } from '@deepseek-ai/cordis'
 import { parseCmdline } from '@deepseek-ai/dsh-cmdline'
 import { boundJsonLine } from './json-stream.ts'
@@ -89,13 +89,15 @@ export function apply(ctx: Context): void {
  // error (an unknown option, a missing option value) before the action runs,
  // and such a rejection still owes a --json caller the error event.
  if (jsonRequested(ctx.get('cmdlineArgs')?.get() ?? [])) {
-    const originalError = program.error.bind(program)
-    program.error = (message: string, errorOptions?: Parameters<typeof originalError>[1]): never => {
+    program.error = (message: string, errorOptions?: Parameters<Command['error']>[1]): never => {
      // The event message matches the runner's runtime errors, which carry no
-      // commander `error: ` prefix; the stderr line keeps commander's text.
+      // commander `error: ` prefix.
      const payload = boundJsonLine({ type: 'error', message: message.replace(/^error: /, '') })
      internals.stdout.write(`${payload}\n`)
-      return originalError(message, errorOptions)
+      // The JSON contract keeps stderr to `dsh:` diagnostics, so commander's
+      // own print of this message must not run; throwing the same control-flow
+      // error still leaves through the launcher's exit path.
+      throw new CommanderError(1, errorOptions?.code ?? 'commander.error', message)
    }
  }
  program.action(() => {
@@ -52,6 +52,8 @@ interface BenchOptions {
  prelive?: boolean
  /** Header facts for that pre-registered live Agent. */
  preliveMeta?: { cwd?: string; origin?: 'subagent'; agentPreset?: string }
+  /** Run when the runner awaits idle, e.g. to append to the attached log. */
+  onWhenIdle?: (agent: Agent) => void
 }

 const frameStates = new WeakMap<Agent, { attemptId: ReturnType<typeof LlmAttemptId>; revision: number; index: number }>()
@@ -147,7 +149,10 @@ async function bench(script: Script, options: BenchOptions = {}): Promise<{
      },
      steer: () => {},
      inject: () => {},
-      whenIdle: () => idle,
+      whenIdle: () => {
+        options.onWhenIdle?.(agent)
+        return idle
+      },
    }
    await createOptions.setup?.(ownerCtx, agent)
    ctx.agents.register(agent)
@@ -772,6 +777,24 @@ describe('headless runner', () => {
    await test.ctx.fiber.dispose()
  })

+  it('rejects a preset an overlay appends while the runner awaits idle', async () => {
+    const test = await bench({ afterPrompt: () => {} }, {
+      sessionId: 'session-exact',
+      observe: () => Promise.resolve({
+        header: { cwd: process.cwd() },
+        events: [],
+        [Symbol.dispose]() {},
+      }),
+      onWhenIdle: (agent) => { selectPreset(agent.session, 'minimal') },
+    })
+    test.ctx.sessions.create(brandString<SessionId>('session-exact'), { meta: { cwd: process.cwd() } })
+    const result = await test.run()
+    expect(result.code).toBe(1)
+    expect(result.err).toContain('runs under agent preset "minimal"')
+    expect(result.out).toBe('')
+    await test.ctx.fiber.dispose()
+  })
+
  it('fails when a live event below the captured Session length cannot be read', async () => {
    let capturedLength = 0
    const test = await bench({
@@ -24,6 +24,7 @@ import {
 interface Observed {
  exits: number[]
  out: string
+  err: string
  runnerConfig?: unknown
 }

@@ -56,7 +57,7 @@ async function bootStartup(
 ): Promise<{ task: HeadlessStartupValues | undefined; observed: Observed }> {
  const dir = mkdtempSync(join(tmpdir(), 'dsh-headless-startup-'))
  tempDirs.push(dir)
-  const observed: Observed = { exits: [], out: '' }
+  const observed: Observed = { exits: [], out: '', err: '' }
  writeFileSync(join(dir, 'row.mjs'), 'export function apply(_ctx, config) { globalThis.__headlessStartupObserved.runnerConfig = config }\n')
  // Loader imports through Node's resolver, so this fixture delegates to the
  // source-plane plugin already imported by the test.
@@ -79,8 +80,17 @@ export const apply = ctx => globalThis.__headlessStartupApply(ctx)
    '',
  ].join('\n'))
  const observing = { write: (chunk: string) => { observed.out += chunk; return true } }
+  // Commander's own output keeps landing in `out` so existing assertions see
+  // the full transcript, while `err` isolates what stderr actually carried.
+  const observingErr = {
+    write: (chunk: string) => {
+      observed.out += chunk
+      observed.err += chunk
+      return true
+    },
+  }
  cmdlineInternals.stdout = observing
-  cmdlineInternals.stderr = observing
+  cmdlineInternals.stderr = observingErr
  startupInternals.stdinIsTty = () => options.stdinIsTty === true
  startupInternals.stdout = observing
  const globals = globalThis as unknown as {
@@ -163,6 +173,7 @@ describe('headless command-line provider', () => {
      message: 'a task is required, for example: dsh --profile headless "run the tests"',
    })
    expect(task).toBeUndefined()
+    expect(observed.err).toBe('')
    expect(observed.exits).toEqual([1])
  })

@@ -171,6 +182,7 @@ describe('headless command-line provider', () => {
    const first = JSON.parse(observed.out.trim().split('\n')[0] ?? '{}') as { type: string; message: string }
    expect(first.type).toBe('error')
    expect(first.message).toContain('--session-id requires a non-empty session id')
+    expect(observed.err).toBe('')
    expect(observed.exits).toEqual([1])
  })

@@ -179,6 +191,7 @@ describe('headless command-line provider', () => {
    const first = JSON.parse(observed.out.trim().split('\n')[0] ?? '{}') as { type: string; message: string }
    expect(first).toEqual({ type: 'error', message: "unknown option '--bogus'" })
    expect(task).toBeUndefined()
+    expect(observed.err).toBe('')
    expect(observed.exits).toEqual([1])
  })