fix(tools): argv the core can parse, and bounds before the spawn

CHANGELOG.md

@@ -24,6 +24,94 @@ Windows 冷启动四关 + 真实 bridge 调用通过，Linux CI 补完整 boot 
   and the Linux cold-start CI rows run the full boot gate on both channels.
 
 ### Fixed
+- **`sessionflow_merge` built an argv the core's argparse could not parse.**  The
+  tool emitted options in the space form (`--goal <value>`) and appended the ids
+  bare, so:
+  * a `goal` beginning with `-` (`--verbose`, `---`) made argparse read it as a
+    flag and exit with `expected one argument`, surfacing as a `CLI_FAILED` that
+    named no argument;
+  * a session id beginning with `-` was parsed as a flag rather than an id.
+
+  Options now travel in the `=` form (`--goal=--verbose`), the ids follow a `--`
+  separator, and `--json` is placed **before** that separator — after it,
+  argparse stops option parsing and reads the flag as another session id
+  (measured: `sessions=['a:1','--json'], json=false`, so the core would have
+  looked for a session called `--json`).  All three shapes are proven against
+  the real `merge` subparser, not against a mock.
+
+- **An over-long argument crashed the spawn without naming the culprit.**  A
+  command line has a hard ceiling (32767 characters on Windows, where Node
+  raises `ENAMETOOLONG` synchronously; `E2BIG` on POSIX), and the bridge reported
+  it as `could not start '<node>': Error: spawn ENAMETOOLONG` — which reads like
+  a broken install and points at the executable.  Values are now bounded before
+  the spawn (`MAX_ARG_CHARS` per value, `MAX_ARGV_CHARS` in total) and the
+  refusal names the field: `` `goal` is too long: 40000 characters, but at most
+  8000 can be carried on a command line. ``  Measured transition for a
+  40000-character goal: a 4 ms failure naming the executable → a 0 ms refusal
+  naming `goal`.
+
+- **A provider-filtered `sessionflow_recent` could silently under-return.**  The
+  core's `overview` op has no provider parameter, so the filter runs in the
+  plugin — and it asked the core for `limit * 4` rows and hoped the wanted
+  provider was at least a quarter of them.  When it was not, the tool returned
+  fewer rows than asked while the core had plenty more, and the answer was
+  indistinguishable from "there is nothing else".  It now requests the widest
+  window the core honours and marks a window-limited answer with `truncated` so
+  an incomplete list cannot read as a complete one.
+
+- **…and the first version of that `truncated` signal could not see the worst
+  case: the wanted provider sitting entirely *behind* the window.**  The
+  predicate required `matched.length > limit`, i.e. "the filter cut something",
+  but when 250 fresher rows of another provider fill the window the filter
+  matches **nothing** and cuts nothing — so an empty list came back with no
+  marker while the sessions did exist.  Measured on a real 255-session index
+  (250 `claude` ahead of 5 `codex`): `{provider:'codex', limit:5}` returned
+  **0 rows, `truncated` absent**, while `sessions {platform:'codex'}` listed all
+  5.  The condition is now "the core filled the window" rather than "we cut
+  more than `limit`", and it is gated on the call being filtered at all, so an
+  ordinary full page is not relabelled as clipped.
+
+  The fixture could not have caught this: `stub-overview.mjs` ignored `limit`
+  and always answered with the whole payload, so "the core gave us everything"
+  and "the core gave us the window" were the same answer.  It now clips the way
+  a real core does.
+
+- **The echo stub answered with a shape the real core cannot produce.**  The
+  subcommand branch wrapped its reply in an `id`/`result` envelope; a real
+  subcommand with `--json` prints a bare payload (`_handoff_via_engine` does
+  `print(json.dumps(payload))` directly — only the `api` op wraps).  No
+  assertion depended on the envelope, which is why nothing noticed; it is fixed
+  so the fixture cannot hide a future parsing mistake.
+
+- **`sessionflow_merge` silently discarded session ids it did not like.**  The
+  argument was read with `filter(isString && length > 0)`, so `['a:1', 42, '  ']`
+  merged **one** session and reported success: the caller was told the merge
+  happened, with no way to see that two of the ids it named were dropped.  The
+  merge is the unit of consent — the caller names a set and the WorkThread
+  contains exactly that set — so quietly narrowing it is a different operation
+  performed on the caller's behalf.  Every element is now checked, not filtered
+  (`session_ids[i]` names the offending index so the other nine ids can be
+  kept), whitespace-only entries are refused as blank, and a non-array argument
+  is refused too.  Measured: `['a:1', 42]` went from "merged a:1, exit 0" to a
+  refusal naming `session_ids[1]`.
+
+- **A fractional `limit` did not behave the way the code comment claimed.**  The
+  comment said the core rejects `3.7` with `slice indices must be integers`.
+  That is true of `overview` but **not** of `search`, which does
+  `int(limit or 20)` (`api.py:254`) and so truncates `3.7` to `3` — no error, no
+  warning, just fewer hits than were asked for, which reads as a complete
+  answer.  Both behaviors are now documented where they belong and measured:
+
+  | input | `search` (`int()`) | `overview` (raw slice) |
+  |---|---|---|
+  | `limit: 3.7` | 3 rows, silently truncated | `slice indices must be integers` |
+  | `limit: 0` | 20 rows (0 reads as absent) | 0 rows |
+  | `limit: -2` | 33,277 rows (no cap) | 91 of 94 rows |
+
+  The guard itself was already right; only its justification was wrong, and a
+  wrong justification is how a later change removes the guard for the wrong
+  reason.
+
 - **The plugin could not be installed on any current harness.**  Both peer
   ranges excluded the versions users actually run, so `dsh plugin add` ended in
   an `ERESOLVE` that had to be worked around by hand.

src/bridge.ts

@@ -448,10 +448,26 @@ export async function callOp<T>(op: string, params: Record<string, unknown>,
   return payload.result as T;
 }
 
+/** Options for {@link runJson}. */
+export interface RunJsonOptions {
+  /**
+   * The caller already placed `--json` itself, so it must not be appended.
+   *
+   * Needed because the append happens at the END of argv, and a caller that
+   * uses the `--` separator to protect positional values loses every flag
+   * after it: `merge a --goal=g -- a:1 --json` parses as
+   * `sessions=['a:1','--json'], json=False`, silently.  A caller in that shape
+   * therefore puts `--json` before the separator and says so here.
+   */
+  jsonAlreadySet?: boolean;
+}
+
 /** Run a core subcommand that emits JSON on stdout (`--json` is appended). */
-export async function runJson<T>(argv: string[], opts: BridgeOptions = {}): Promise<T> {
+export async function runJson<T>(argv: string[], opts: BridgeOptions = {},
+                                 runOpts: RunJsonOptions = {}): Promise<T> {
   const cmd = await resolveVoyager(opts);
-  const { stdout, stderr, code } = await run(cmd, [...argv, '--json'], opts);
+  const full = runOpts.jsonAlreadySet ? [...argv] : [...argv, '--json'];
+  const { stdout, stderr, code } = await run(cmd, full, opts);
   if (code !== 0) {
     throw new VoyagerError('CLI_FAILED',
       `\`voyager ${argv.join(' ')}\` failed with exit code ${code}`,

src/tools/common.ts

@@ -29,6 +29,177 @@ export function describeError(e: unknown): string {
   return e instanceof Error ? e.message : String(e);
 }
 
+/**
+ * A parameter the tool refuses to forward.
+ *
+ * Thrown before any child process is spawned, so a bad number costs a message
+ * rather than a round trip — and never reaches the core, where a negative
+ * `limit` is not an error at all: `search` does `int(limit or 20)` and hands the
+ * value to SQLite as `LIMIT -N`, which removes the cap instead of applying one.
+ * On a real index that is 1241 rows and ~480 KB of JSON into the context.
+ */
+export class ParameterError extends Error {
+  constructor(message: string) {
+    super(message);
+    this.name = 'ParameterError';
+  }
+}
+
+export interface Bounds {
+  /** Smallest accepted value, inclusive. */
+  min: number;
+  /** Largest accepted value, inclusive. Clamped, not rejected. */
+  max?: number;
+  /** Value used when the argument is absent. */
+  fallback: number;
+  /** What the number means, for the refusal message. */
+  what: string;
+}
+
+function refuse(name: string, b: Bounds, got: unknown, reason: string): never {
+  const range = b.max === undefined ? `>= ${b.min}` : `${b.min}..${b.max}`;
+  throw new ParameterError(
+    `\`${name}\` must be ${b.what} ${range} (got ${JSON.stringify(got)}${reason ? `; ${reason}` : ''}).`);
+}
+
+/**
+ * A bounded integer argument.
+ *
+ * Whole numbers only.  What a fractional value does depends on the op, and the
+ * two behaviors are different enough that neither can be described as "the
+ * core rejects it" — both measured on voyager 1.3.0 against a real index:
+ *
+ *  * `search` does `int(limit or 20)` (api.py:254), so `3.7` is silently
+ *    truncated to `3`.  Nothing fails and nothing says so; the caller simply
+ *    gets fewer hits than it asked for.  A silent under-read is the worst of
+ *    the three outcomes, because it looks like a complete answer.
+ *  * `overview` slices with the raw value (api.py:77 and :94), so the same
+ *    `3.7` raises `slice indices must be integers or None or have an
+ *    __index__ method` — an error that names no argument and arrives far from
+ *    the caller's mistake.
+ *
+ * A fractional count is therefore never "a smaller integer" to be rounded down
+ * on the caller's behalf: it is either a quiet under-read or an opaque failure.
+ * Refusing it here costs one message and names the field.
+ */
+export function intArg(name: string, value: unknown, b: Bounds): number {
+  if (value === undefined || value === null) return b.fallback;
+  if (typeof value !== 'number' || !Number.isFinite(value)) {
+    refuse(name, b, value, 'not a finite number');
+  }
+  if (!Number.isInteger(value)) {
+    refuse(name, b, value, 'must be a whole number');
+  }
+  if (value < b.min) refuse(name, b, value, 'too small');
+  return b.max === undefined ? value : Math.min(value, b.max);
+}
+
+/** A bounded finite number, for windows measured in hours. */
+export function numArg(name: string, value: unknown, b: Bounds): number {
+  if (value === undefined || value === null) return b.fallback;
+  if (typeof value !== 'number' || !Number.isFinite(value)) {
+    refuse(name, b, value, 'not a finite number');
+  }
+  if (value < b.min) refuse(name, b, value, 'too small');
+  return b.max === undefined ? value : Math.min(value, b.max);
+}
+
+/**
+ * Ceiling for one argument value, in characters.
+ *
+ * A single absurd value is almost always a client-side mistake rather than a
+ * large request, so it is refused on its own terms before the total below is
+ * even consulted — which lets the message name that one field.
+ */
+export const MAX_ARG_CHARS = 8_000;
+
+/**
+ * Total ceiling for the arguments this plugin builds, in characters.
+ *
+ * Windows caps a command line at 32767 characters, and Node raises
+ * `spawn ENAMETOOLONG` **synchronously** when an argument pushes past it;
+ * POSIX raises `E2BIG` for the same class of failure.  Measured in this
+ * repository: a 32,500-character argument still spawned, 33,000 did not.
+ *
+ * The budget is deliberately well under the OS limit so the resolved
+ * executable, the bridge's prefix arguments and the flags the bridge adds all
+ * fit inside the same command line.  Exceeding it used to surface as
+ * `could not start '<node>': Error: spawn ENAMETOOLONG` — an error naming the
+ * executable and never the argument, which reads like a broken install.
+ */
+export const MAX_ARGV_CHARS = 24_000;
+
+/** Total characters across every argument the OS will have to carry. */
+export function argvLength(argv: readonly string[]): number {
+  let total = 0;
+  for (const arg of argv) total += arg.length;
+  return total;
+}
+
+export interface FlagBounds {
+  /** Largest accepted value, in characters. Refused, never truncated. */
+  maxLength: number;
+}
+
+/**
+ * One option as `--name=value`, with the value proven safe to carry.
+ *
+ * The `=` form is not a style choice: argparse reads `--goal --verbose` as a
+ * missing value and exits, so a goal whose text happens to begin with a dash
+ * turned the whole call into a `CLI_FAILED` that named nothing.  With `=` the
+ * value is data no matter what it starts with, and interior whitespace, CJK and
+ * embedded `=` all survive (all measured against the real `merge` subparser).
+ *
+ * Underscores in the parameter name become dashes, because every core flag is
+ * kebab-case (`--thread-title`).  The name is only ever a literal from this
+ * repository, never caller input.
+ */
+export function flagArg(name: string, value: unknown, b: FlagBounds): string {
+  if (typeof value !== 'string') {
+    throw new ParameterError(
+      `\`${name}\` must be text (got ${JSON.stringify(value)}).`);
+  }
+  if (value.length > b.maxLength) {
+    throw new ParameterError(
+      `\`${name}\` is too long: ${value.length} characters, but at most ` +
+      `${b.maxLength} can be carried on a command line.`);
+  }
+  // A line break or NUL cannot survive argv intact; the OS truncates at the
+  // NUL and a newline splits the value, so both are refused rather than sent
+  // as something subtly different from what the caller asked for.
+  if (/[\u0000\r\n]/.test(value)) {
+    throw new ParameterError(
+      `\`${name}\` must be a single line without NUL bytes ` +
+      `(got ${JSON.stringify(value.slice(0, 40))}).`);
+  }
+  return `--${name.replace(/_/g, '-')}=${value}`;
+}
+
+/**
+ * Refuse an argv the operating system cannot deliver, naming the field that
+ * made it too big.
+ *
+ * `contributions` is every caller-supplied part with the name a model can act
+ * on, so the message points at `session_ids` rather than at "the command line".
+ */
+export function assertArgvFits(
+  argv: readonly string[],
+  contributions: ReadonlyArray<readonly [string, number]>,
+): void {
+  const total = argvLength(argv);
+  if (total <= MAX_ARGV_CHARS) return;
+  const largest = contributions.reduce<readonly [string, number] | undefined>(
+    (best, entry) => (best === undefined || entry[1] > best[1] ? entry : best),
+    undefined,
+  );
+  const who = largest === undefined ? 'this call' : `\`${largest[0]}\``;
+  throw new ParameterError(
+    `${who} would make this call ${total} characters, over the ` +
+    `${MAX_ARGV_CHARS} that fit on one command line (the OS limit is lower ` +
+    `still once the executable path is counted). Send fewer ids, or shorter ` +
+    `values.`);
+}
+
 /** Run a tool body, turning any failure into a clean, actionable error.
  *
  * The return type is deliberately `any`: the registry types a tool's canonical