From e2cfdcfaaeca7e129b112917e3c35a5b506d51b0 Mon Sep 17 00:00:00 2001 From: SunnyRad Date: Mon, 9 Nov 2026 22:07:19 +0800 Subject: [PATCH 1/4] fix(tools): bound numeric arguments before the core sees them `limit` and `hours` are plain numbers the model supplies, and the plugin forwarded them unchecked. The host does not cover this gap: `defineTool` compiles the property spec to JSON Schema, so it rejects a *type* mistake (`"hours": "yesterday"` -> `invalid arguments: "hours" must be a number`) but `-2`, `0` and `3.7` are all well-typed numbers and all reached the core. The core does not reject them either: * `int(limit or 20)` treats 0 as absent, so `search {limit: 0}` asked for nothing and got 20. * SQLite reads `LIMIT -N` as "no cap", so `search {limit: -2}` returned the whole index: measured 1241 rows / 480,826 bytes of JSON on a real index (94 sessions / 526,991 events), all of it into the caller's context. `recent {limit: -3}` uncapped the overview the same way (91 rows). * `search {limit: 3.7}` failed later inside SQLite with `slice indices must be integers or None`, an error carrying no trace of the argument that caused it. * `overview` computes `cutoff = now - hours*3600`, so `hours: -5` puts the cutoff in the future, filters out everything, and returns an empty list as though there were no history. This adds `intArg` / `numArg` to `src/tools/common.ts` and calls them in `search.ts` and `recent.ts` before the op is built. Both decide in the tool process, so a refused argument costs a message rather than a child process and a database open. Above the ceiling the value is clamped, not refused: `limit: 500` is an intelligible "give me a lot" and clamping to 200 honours that intent while keeping the bound. Below the floor it is refused, because `-2` is not a larger request, it is a category error. `hours` is allowed to be fractional (half an hour is a real window) and 0 is legitimate ("right now"). The refusal is thrown outside `guarded` on purpose, so it stays a `ParameterError` naming the argument and the accepted range instead of being flattened into the generic message every core failure produces: `limit` must be a whole number of hits 1..200 (got -2; too small). Verified: `npm test` -> 64 tests, 44 pass, 0 fail, 20 skipped (the skips are the core-dependent integration tests; no voyager core on this machine). The 10 new cases in `tests/numeric-args.test.ts` assert on an echo core's call record being *empty*, so "refused" means nothing was spawned rather than "an error came back". Mutation-checked each way: removing the `recent` bound turns 1 test red, removing the `search` bound turns 3 red, restoring both returns the suite to green. End to end against a live core, `search {limit: -2}` went from 480 KB to a 6 ms refusal. --- src/tools/common.ts | 62 +++++++++++++ src/tools/recent.ts | 20 ++++- src/tools/search.ts | 12 ++- tests/numeric-args.test.ts | 179 +++++++++++++++++++++++++++++++++++++ 4 files changed, 268 insertions(+), 5 deletions(-) create mode 100644 tests/numeric-args.test.ts diff --git a/src/tools/common.ts b/src/tools/common.ts index 56784c7..253545b 100644 --- a/src/tools/common.ts +++ b/src/tools/common.ts @@ -29,6 +29,68 @@ 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: `3.7` is not a "smaller 4" to the core, it is a value + * SQLite rejects with `slice indices must be integers`, which surfaces as an + * opaque error far from the caller's mistake. + */ +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); +} + /** 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 diff --git a/src/tools/recent.ts b/src/tools/recent.ts index 928bddc..0e3ea0a 100644 --- a/src/tools/recent.ts +++ b/src/tools/recent.ts @@ -1,6 +1,6 @@ import { defineTool } from '@deepseek-ai/dsh-tools'; import { callOp, type BridgeOptions } from '../bridge.js'; -import { guarded, jsonOutput } from './common.js'; +import { guarded, intArg, jsonOutput, numArg } from './common.js'; interface RecentSession { id: string; @@ -37,8 +37,22 @@ export function recentTool(bridge: BridgeOptions) { }, output: jsonOutput(), async execute(args) { + // Validated OUTSIDE `guarded` on purpose: a refusal must stay a + // ParameterError rather than being flattened into the same generic + // message every core failure produces. Nothing has been spawned yet. + // Bounded before the op is built. `limit: 0` used to reach the core, + // where `int(limit or 20)` turned it into 20 — a call that asked for + // nothing and got a full page. A negative value was worse: SQLite + // treated `LIMIT -3` as "no cap" and returned 91 rows. + const limit = intArg('limit', args.limit, + { min: 1, max: 200, fallback: 12, what: 'a whole number of rows' }); + // A negative window is not "since forever": the core computes + // `cutoff = now - hours*3600`, so -5 asks for a cutoff in the future + // and every session is filtered out. The caller gets an empty list and + // no hint that the window was the problem. + const hours = numArg('hours', args.hours, + { min: 0, max: 24 * 365 * 10, fallback: 48, what: 'a non-negative number of hours' }); return await guarded(async () => { - const limit = args.limit ?? 12; // The core's `overview` op has no provider filter, so a provider // narrowing has to happen here — but applying it *after* the core's // `limit` would silently under-return. Ask for a wider window first so @@ -46,7 +60,7 @@ export function recentTool(bridge: BridgeOptions) { const want = args.provider ? Math.min(limit * 4, 200) : limit; const ov = await callOp('overview', { repo: args.repo ?? null, - hours: args.hours ?? 48, + hours, limit: want, }, bridge); const sessions = args.provider diff --git a/src/tools/search.ts b/src/tools/search.ts index ee1391e..98cdb9b 100644 --- a/src/tools/search.ts +++ b/src/tools/search.ts @@ -1,6 +1,6 @@ import { defineTool } from '@deepseek-ai/dsh-tools'; import { callOp, type BridgeOptions } from '../bridge.js'; -import { guarded, jsonOutput } from './common.js'; +import { guarded, intArg, jsonOutput } from './common.js'; interface SearchHit { id: string; @@ -48,13 +48,21 @@ export function searchTool(bridge: BridgeOptions) { }, output: jsonOutput(), async execute(args) { + // Validated OUTSIDE `guarded` on purpose: a refusal must stay a + // ParameterError rather than being flattened into the same generic + // message every core failure produces. Nothing has been spawned yet. + // The core passes `limit` straight to SQLite, where a negative value + // means "no cap" rather than "an error", and an unbounded result lands + // in the model's context (measured: 1241 rows, ~480 KB of JSON). + const limit = intArg('limit', args.limit, + { min: 1, max: 200, fallback: 20, what: 'a whole number of hits' }); return await guarded(() => callOp('search', { query: args.query, provider: args.provider ?? null, repo: args.repo ?? null, since: args.since ?? null, until: args.until ?? null, - limit: args.limit ?? 20, + limit, }, bridge)); }, }); diff --git a/tests/numeric-args.test.ts b/tests/numeric-args.test.ts new file mode 100644 index 0000000..20d6261 --- /dev/null +++ b/tests/numeric-args.test.ts @@ -0,0 +1,179 @@ +/** + * Guard: a numeric argument that the core would misinterpret must never reach + * the core. + * + * Three distinct failures motivate this file, all measured on voyager 1.3.0 + * against a real index (94 sessions / 526,991 events): + * + * 1. `search {limit: -2}` -> count 1241, 480,826 bytes of JSON. The core does + * `int(limit or 20)` and hands the value to SQLite as `LIMIT -2`, which + * removes the cap rather than applying one. A model call with a stray + * minus sign is therefore an unbounded read into its own context. + * + * 2. `search {limit: 3.7}` -> the request itself is fine; SQLite rejects it + * later with `slice indices must be integers or None`, which surfaces as a + * message with no trace of the argument that caused it. + * + * 3. `overview {limit: 0}` -> 0 rows, not 12. `int(limit or 20)` treats 0 as + * absent, so "ask for nothing" silently means "ask for the default", while + * `overview {limit: -3}` means "ask for everything" (91 rows). + * + * The tools must refuse or clamp BEFORE spawning a child, and the refusal must + * be identifiable as a parameter problem rather than folded into the generic + * error every core failure produces. + */ + +import './isolate.ts'; + +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { dirname } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +import { recentTool, searchTool } from '../lib/index.js'; +import { ParameterError, intArg, numArg } from '../lib/tools/common.js'; + +const HERE = dirname(fileURLToPath(import.meta.url)); + +/** + * An echo core whose calls are recorded to a file. + * + * "Refused" has to mean *nothing was spawned*, not merely "an error came back": + * a core that was reached and then rejected the value would still have spent a + * child process and a database open on a call the tool could have refused. The + * assertion below is therefore on the RECORD being empty. + */ +async function callsDuring( + run: () => Promise, +): Promise> { + const dir = mkdtempSync(join(tmpdir(), 'dsh-sessionflow-num-')); + const record = join(dir, 'calls.txt'); + writeFileSync(record, ''); + const previous = process.env['SESSIONFLOW_ECHO_FILE']; + process.env['SESSIONFLOW_ECHO_FILE'] = record; + try { + try { + await run(); + } catch { + // the refusal is the subject; what it SENT is what we assert on + } + const text = readFileSync(record, 'utf8'); + return text.split('\n').filter((l) => l.trim().length > 0).map((l) => { + const idx = l.indexOf('|'); + return [l.slice(0, idx), JSON.parse(l.slice(idx + 1))] as [string, unknown]; + }); + } finally { + if (previous === undefined) delete process.env['SESSIONFLOW_ECHO_FILE']; + else process.env['SESSIONFLOW_ECHO_FILE'] = previous; + rmSync(dir, { recursive: true, force: true }); + } +} + +function echoBridge() { + return { + voyagerBin: process.execPath, + voyagerArgs: [join(HERE, 'fixtures', 'stub-echo.mjs')], + }; +} + +/** Run a tool that must refuse, and prove the core was never asked. */ +async function expectRefused( + run: (bridge: never) => Promise, + pattern: RegExp, +): Promise { + let error: Error | undefined; + const calls = await callsDuring(async () => { + try { + await run(echoBridge() as never); + } catch (e) { + error = e as Error; + throw e; + } + }); + assert.ok(error, 'the call should have been refused'); + assert.match(error.message, pattern); + assert.deepEqual(calls, [], 'a refused argument must not reach the core'); +} + +test('search refuses a negative limit instead of reading the whole index', async () => { + await expectRefused( + (b) => searchTool(b).execute({ query: 'x', limit: -2 }, {} as never), + /`limit`.*whole number of hits/); +}); + +test('search refuses a fractional limit rather than letting SQLite complain', async () => { + await expectRefused( + (b) => searchTool(b).execute({ query: 'x', limit: 3.7 }, {} as never), + /whole number/); +}); + +test('search refuses limit 0 rather than silently asking for the default', async () => { + await expectRefused( + (b) => searchTool(b).execute({ query: 'x', limit: 0 }, {} as never), + /`limit`/); +}); + +test('recent refuses a negative limit rather than uncapping the overview', async () => { + await expectRefused( + (b) => recentTool(b).execute({ limit: -3 }, {} as never), + /`limit`.*whole number of rows/); +}); + +test('recent refuses a negative hours window, which silently empties the result', async () => { + await expectRefused( + (b) => recentTool(b).execute({ hours: -5 }, {} as never), + /`hours`/); +}); + +test('recent refuses a non-numeric hours value', async () => { + // This one is decided by the HOST, not by the tool: `defineTool` compiles the + // property spec into JSON Schema, so `"hours": "yesterday"` is rejected with + // `invalid arguments: "hours" must be a number` before `execute` runs — and + // no child is spawned either way. + // + // That asymmetry is the whole reason this file exists: the host validates + // the *type*, and a type check cannot see `-2`. A negative limit, a zero + // limit and a fractional limit are all well-typed numbers, so every one of + // them used to reach SQLite intact. Range is the plugin's job. + await expectRefused( + (b) => recentTool(b).execute({ hours: 'yesterday' as never }, {} as never), + /"hours" must be a number/); +}); + +// --- the boundaries themselves: clamping must not become rejecting ---------- + +test('intArg clamps above the ceiling instead of failing the call', () => { + const b = { min: 1, max: 200, fallback: 20, what: 'rows' }; + assert.equal(intArg('limit', 500, b), 200, 'over the ceiling clamps to it'); + assert.equal(intArg('limit', 200, b), 200, 'the ceiling itself is accepted'); + assert.equal(intArg('limit', 1, b), 1, 'the floor itself is accepted'); + assert.equal(intArg('limit', undefined, b), 20, 'absent takes the fallback'); +}); + +test('intArg rejects rather than clamps below the floor', () => { + const b = { min: 1, max: 200, fallback: 20, what: 'rows' }; + for (const bad of [0, -1, -2, 0.5, 2.5, Number.NaN, Infinity]) { + assert.throws(() => intArg('limit', bad, b), ParameterError, + `${String(bad)} must be refused`); + } +}); + +test('numArg admits a fractional window, which is meaningful for hours', () => { + const b = { min: 0, max: 1000, fallback: 48, what: 'hours' }; + assert.equal(numArg('hours', 12.5, b), 12.5, 'half an hour is a real window'); + assert.equal(numArg('hours', 0, b), 0, 'zero is a legitimate "right now"'); + assert.equal(numArg('hours', 5000, b), 1000, 'over the ceiling clamps'); + assert.throws(() => numArg('hours', -1, b), ParameterError); +}); + +test('a refusal names the argument and the accepted range', () => { + assert.throws( + () => intArg('limit', -2, { min: 1, max: 200, fallback: 20, what: 'a whole number of hits' }), + (e: Error) => e instanceof ParameterError + && /`limit`/.test(e.message) + && /1\.\.200/.test(e.message) + && /-2/.test(e.message)); +}); From 8bd58f712b7ffcbad5798d8a82ada3aaca888201 Mon Sep 17 00:00:00 2001 From: SunnyRad Date: Mon, 9 Nov 2026 22:21:25 +0800 Subject: [PATCH 2/4] fix(tools): argv the core can parse, and bounds before the spawn Two more ways a model-supplied argument could make a tool fail badly, both measured against the real `merge` subparser rather than a mock. **The argv was shaped so argparse could not read it.** Options were emitted in the space form and the ids appended bare: merge a:1 b:2 --goal g * A goal beginning with `-` reached argparse as a flag. `--verbose` and `---` both produced `argument --goal: expected one argument` -> SystemExit -> `CLI_FAILED`, an error naming no argument. * A session id beginning with `-` was parsed as a flag, not an id. * Nothing bounded the values at all. The core parses `--goal=VALUE` unambiguously, so options now use that form and the ids follow a `--` separator. Interior whitespace, CJK and an embedded `=` all survive in that form (measured). 17 shape assertions in `tests/merge-argv.test.ts` pin it. **One trap lives inside that fix,** and it is the reason `--json` moved: `--json` used to be appended at the END of argv, which after the separator means argparse stops option parsing and reads it as a positional: merge a --goal=g -- a:1 --json -> sessions=['a:1','--json'], json=False The core would then look for a session called `--json` and answer without JSON. So `merge` places `--json` before the separator itself, and `runJson` gained `jsonAlreadySet` so the bridge does not append a second one. **An over-long argument crashed the spawn without naming itself.** A command line has a hard ceiling - 32767 chars on Windows, where Node raises `spawn ENAMETOOLONG` synchronously; `E2BIG` on POSIX. The bridge surfaced it as could not start '': Error: spawn ENAMETOOLONG which names the executable and reads like a broken install. Measured here: 32500 chars spawns, 33000 throws. `flagArg` and `assertArgvFits` now bound each value (8000) and the whole argv (24000) before the spawn, and the refusal names the field: `goal` is too long: 40000 characters, but at most 8000 can be carried on a command line. A 40000-character goal went from a 4 ms failure naming the executable to a 0 ms refusal naming `goal`. **`sessionflow_recent` could silently under-return when filtering.** The core's `overview` takes no provider argument, so the filter runs in the plugin, and it asked for `limit * 4` rows and hoped the wanted provider was at least a quarter of them. With 190 claude rows ahead of 10 codex rows, `{provider:'codex', limit:5}` returned an empty list - indistinguishable from "no codex sessions". It now asks for the widest window the core honours, and marks a window-limited answer with `truncated: true` so an incomplete list cannot read as complete. **The echo stub was lying about the response shape.** Its subcommand branch wrapped the reply in an `id`/`result` envelope; a real subcommand with `--json` prints a bare payload (`_handoff_via_engine` prints `json.dumps(payload)` directly - only the `api` op wraps). No assertion depended on it, which is exactly why it went unnoticed, and a fixture that mismeasures its subject is worth fixing before it hides the next mistake. `stub-overview.mjs` is new and answers with a caller-supplied row set, so the provider filter is testable without an index. Verified: `npm test` -> 87 tests, 67 pass, 0 fail, 20 skipped (the skips are the core-dependent integration tests; no voyager core on this machine). Mutation-checked, each mutation restored afterwards: | mutation | tests turning red | |---|---| | baseline | 0 | | `--goal` back to the space form | 6 | | `--json` moved after the separator | 3 | | argv length guard removed | 1 | | `recent` back to the `limit * 4` heuristic | 1 | | `truncated` marker removed | 1 | | restored | 0 | `tests/plugin.test.ts` had one assertion that pinned the exact old array `['a:1','b:2','--goal','g']` - the buggy shape. It now asserts what it was written to protect (ids arrive as whole separate arguments, nothing is a joined command line), because pinning the layout would have re-frozen the defect in the one test that exists to catch it. --- CHANGELOG.md | 42 ++++++ src/bridge.ts | 20 ++- src/tools/common.ts | 96 ++++++++++++++ src/tools/merge.ts | 64 +++++++-- src/tools/recent.ts | 29 ++++- tests/fixtures/stub-echo.mjs | 21 ++- tests/fixtures/stub-overview.mjs | 44 +++++++ tests/merge-argv.test.ts | 215 +++++++++++++++++++++++++++++++ tests/plugin.test.ts | 17 ++- tests/recent-provider.test.ts | 126 ++++++++++++++++++ 10 files changed, 651 insertions(+), 23 deletions(-) create mode 100644 tests/fixtures/stub-overview.mjs create mode 100644 tests/merge-argv.test.ts create mode 100644 tests/recent-provider.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index e2915a0..74261ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,48 @@ 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 `) 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 '': 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. + +- **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. + - **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. diff --git a/src/bridge.ts b/src/bridge.ts index bb87a6e..3d9b32c 100644 --- a/src/bridge.ts +++ b/src/bridge.ts @@ -448,10 +448,26 @@ export async function callOp(op: string, params: Record, 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(argv: string[], opts: BridgeOptions = {}): Promise { +export async function runJson(argv: string[], opts: BridgeOptions = {}, + runOpts: RunJsonOptions = {}): Promise { 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}`, diff --git a/src/tools/common.ts b/src/tools/common.ts index 253545b..a0d002c 100644 --- a/src/tools/common.ts +++ b/src/tools/common.ts @@ -91,6 +91,102 @@ export function numArg(name: string, value: unknown, b: Bounds): number { 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 '': 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, +): void { + const total = argvLength(argv); + if (total <= MAX_ARGV_CHARS) return; + const largest = contributions.reduce( + (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 diff --git a/src/tools/merge.ts b/src/tools/merge.ts index fcf11e4..b80bd18 100644 --- a/src/tools/merge.ts +++ b/src/tools/merge.ts @@ -1,6 +1,8 @@ import { defineTool } from '@deepseek-ai/dsh-tools'; import { runJson, type BridgeOptions } from '../bridge.js'; -import { guarded, jsonOutput } from './common.js'; +import { + MAX_ARG_CHARS, assertArgvFits, flagArg, guarded, jsonOutput, ParameterError, +} from './common.js'; interface MergeResult { action: string; @@ -37,18 +39,56 @@ export function mergeTool(bridge: BridgeOptions) { }, output: jsonOutput(), async execute(args) { + // Built and bounded OUTSIDE `guarded`, so a refusal stays a + // ParameterError naming the field instead of being flattened into the + // generic message a core failure produces. Nothing is spawned yet. + const ids = Array.isArray(args.session_ids) + ? args.session_ids.filter((s): s is string => typeof s === 'string' && s.length > 0) + : []; + if (ids.length === 0) { + throw new ParameterError('sessionflow_merge needs at least one session id.'); + } + // A session id becomes a POSITIONAL argument, so it must survive the same + // length ceiling as a value would. + const oversized = ids.find((id) => id.length > MAX_ARG_CHARS); + if (oversized !== undefined) { + throw new ParameterError( + `\`session_ids\` contains one id of ${oversized.length} characters, ` + + `over the ${MAX_ARG_CHARS} one argument may carry.`); + } + + // Options in `=` form: a goal of "--verbose" is data, not a flag. + const options: string[] = []; + const contributions: Array = [['session_ids', 0]]; + let idsChars = 0; + for (const id of ids) idsChars += id.length; + contributions[0] = ['session_ids', idsChars]; + + if (args.goal !== undefined) { + options.push(flagArg('goal', args.goal, { maxLength: MAX_ARG_CHARS })); + contributions.push(['goal', args.goal.length]); + } + if (args.thread_title !== undefined) { + options.push(flagArg('thread_title', args.thread_title, { maxLength: MAX_ARG_CHARS })); + contributions.push(['thread_title', args.thread_title.length]); + } + if (args.budget !== undefined) { + // A budget is a short token or an integer; the same ceiling applies. + options.push(flagArg('budget', args.budget, { maxLength: MAX_ARG_CHARS })); + contributions.push(['budget', args.budget.length]); + } + + // Order matters and is load-bearing: `--json` goes BEFORE the separator. + // After it, argparse stops option parsing and reads the flag as just + // another session id — measured as `sessions=['a:1','--json'], json=false`. + const argv = [ + 'merge', ...options, '--json', '--', ...ids, + ]; + assertArgvFits(argv, contributions); + return await guarded(async () => { - const ids = Array.isArray(args.session_ids) - ? args.session_ids.filter((s): s is string => typeof s === 'string' && s.length > 0) - : []; - if (ids.length === 0) { - throw new Error('sessionflow_merge needs at least one session id.'); - } - const argv = ['merge', ...ids]; - if (args.goal) argv.push('--goal', args.goal); - if (args.thread_title) argv.push('--thread-title', args.thread_title); - if (args.budget) argv.push('--budget', args.budget); - return await runJson(argv, bridge); + // `--json` is already in place, so the bridge must not append another. + return await runJson(argv, bridge, { jsonAlreadySet: true }); }); }, }); diff --git a/src/tools/recent.ts b/src/tools/recent.ts index 0e3ea0a..2dbe1b0 100644 --- a/src/tools/recent.ts +++ b/src/tools/recent.ts @@ -21,6 +21,15 @@ interface Overview { recent_sessions: RecentSession[]; } +/** + * How many rows to ask the core for when a provider filter will be applied here. + * + * Matches the ceiling `intArg` allows for `limit`, so the filtered answer can + * never be short because the request was: the window is the widest the core + * will honour, and `limit` then decides how much of it to show. + */ +const PROVIDER_FILTER_WINDOW = 200; + export function recentTool(bridge: BridgeOptions) { return defineTool({ name: 'sessionflow_recent', @@ -55,21 +64,31 @@ export function recentTool(bridge: BridgeOptions) { return await guarded(async () => { // The core's `overview` op has no provider filter, so a provider // narrowing has to happen here — but applying it *after* the core's - // `limit` would silently under-return. Ask for a wider window first so - // the filtered list can still reach `limit`. - const want = args.provider ? Math.min(limit * 4, 200) : limit; + // `limit` would silently under-return. The previous fix asked for + // `limit * 4` rows and hoped; that is a guess, and when the wanted + // provider is rarer than one row in four it still under-returns, with + // no signal that it did. Ask for the ceiling up front instead: the + // extra rows are cheap (the core filters and sorts before it reads any + // events) and the answer is then exact rather than probable. + const want = args.provider ? PROVIDER_FILTER_WINDOW : limit; const ov = await callOp('overview', { repo: args.repo ?? null, hours, limit: want, }, bridge); - const sessions = args.provider + const matched = args.provider ? ov.recent_sessions.filter((s) => s.provider === args.provider) : ov.recent_sessions; + // Say when the window, not the filter, is what limited the answer: a + // caller that got `limit` rows of a filtered list cannot otherwise tell + // a complete answer from a truncated one. + const truncated = matched.length > limit && want === ov.recent_sessions.length + && ov.recent_sessions.length >= PROVIDER_FILTER_WINDOW; return { stats: ov.stats, threads: ov.threads, - recent_sessions: sessions.slice(0, limit), + recent_sessions: matched.slice(0, limit), + ...(truncated ? { truncated: true } : {}), }; }); }, diff --git a/tests/fixtures/stub-echo.mjs b/tests/fixtures/stub-echo.mjs index e919fbc..50dbf41 100644 --- a/tests/fixtures/stub-echo.mjs +++ b/tests/fixtures/stub-echo.mjs @@ -43,7 +43,22 @@ if (argv[0] === 'integration-info') { }); }); } else { - // a subcommand call such as `merge --json` - record(`${argv[0]}|${JSON.stringify(argv.slice(1, -1))}`); - reply({ action: 'bundle', context: '', argv: argv.slice(0, -1) }); + // a subcommand call such as `merge --json -- ` + // + // `--json` is now placed BEFORE the `--` separator rather than at the end of + // argv (after the separator argparse would read it as a positional session + // id). Recording or slicing by POSITION would therefore drop a session id, + // so the identity flag is filtered out by value and the `--` separator is + // kept, which is what makes the recorded shape legible in a failure message. + record(`${argv[0]}|${JSON.stringify(argv.filter((a) => a !== '--json'))}`); + // A subcommand's `--json` output is a BARE payload, not an id/result + // envelope: `_handoff_via_engine` does `print(json.dumps(payload))` directly + // (cli.py). Only the `api` op wraps its answer. This stub used to wrap + // both, which is why nothing here ever noticed that the subcommand branch + // answered with a shape the real core cannot produce. + process.stdout.write(JSON.stringify({ + action: 'bundle', context: '', thread_id: 'stub-thread', + scope: null, style: 'continuation', context_chars: 0, member_count: 1, + warnings: [], error: null, bundle_path: null, argv, + })); } diff --git a/tests/fixtures/stub-overview.mjs b/tests/fixtures/stub-overview.mjs new file mode 100644 index 0000000..d2dd124 --- /dev/null +++ b/tests/fixtures/stub-overview.mjs @@ -0,0 +1,44 @@ +// Answers `overview` with a fixed JSON payload read from a file, so a test can +// control exactly which rows the tool's own filtering has to sift. Records the +// params it was asked for, like stub-echo does. +// +// Invocation is `node stub-overview.mjs ` with the bridge's +// arguments appended, so the payload path sits in argv BEFORE the subcommand +// (`integration-info`, `api`). Locating the subcommand by membership rather +// than by index is therefore load-bearing: indexing argv[0] would look at the +// payload path and every probe would be answered as an unknown subcommand. +import { appendFileSync, readFileSync } from 'node:fs'; + +const argv = process.argv.slice(2); +const payloadPath = argv[0]; + +function record(line) { + const file = process.env['SESSIONFLOW_ECHO_FILE']; + if (!file) return; + try { + appendFileSync(file, line + '\n'); + } catch { + // scratch dir may already be gone + } +} + +if (argv.includes('integration-info')) { + process.stdout.write(JSON.stringify({ + name: 'sessionFlow', package: 'voyager', version: '0.0.0-stub', + schema_version: 1, ops: [], capabilities: [], + })); +} else if (argv.includes('api')) { + let buf = ''; + process.stdin.setEncoding('utf8'); + process.stdin.on('data', (c) => { buf += c; }); + process.stdin.on('end', () => { + let req = {}; + try { req = JSON.parse(buf.trim() || '{}'); } catch { /* ignore */ } + record(`${req.op}|${JSON.stringify(req.params ?? {})}`); + let result = {}; + try { result = JSON.parse(readFileSync(payloadPath, 'utf8')); } catch { /* ignore */ } + process.stdout.write(JSON.stringify({ id: 1, result })); + }); +} else { + process.stdout.write('{}'); +} diff --git a/tests/merge-argv.test.ts b/tests/merge-argv.test.ts new file mode 100644 index 0000000..2d6f612 --- /dev/null +++ b/tests/merge-argv.test.ts @@ -0,0 +1,215 @@ +/** + * Guard: `sessionflow_merge` must build an argv the core's argparse can + * actually parse, and must refuse an argv the OS cannot carry. + * + * Two defects motivate this file, both measured against the real `merge` + * subparser (cli.py: `sessions` nargs="+", `--goal`, `--thread-title`, + * `--budget`, `--json`): + * + * 1. **A value beginning with `-` was parsed as a flag.** The tool emitted + * `--goal `, and argparse reads `--goal` plus a following + * dash-prefixed token as "expected one argument" -> SystemExit. So a goal + * of `--verbose` or `---` made the whole call fail with CLI_FAILED, an + * error that says nothing about which argument was at fault. The `=` form + * (`--goal=--verbose`) has no such ambiguity. + * + * 2. **An over-long argument crashed the spawn, unnamed.** Windows caps a + * command line at 32767 characters; Node raises `spawn ENAMETOOLONG` + * synchronously and the bridge reported it as + * `could not start '': Error: spawn ENAMETOOLONG` — the argument that + * caused it is nowhere in the message, and on POSIX the analogous limit is + * `E2BIG`. Measured in this repository: 33000 characters throws, 32500 + * succeeds. + * + * There is also a trap in the fix itself, encoded in the ordering test below: + * the shell-style `--` separator that makes a dash-leading *positional* safe + * also swallows every later flag, so the `--json` the bridge appends has to go + * BEFORE the separator. Emitting it after was measured to yield + * `sessions=['a:1', '--json'], json=False` — the core would then treat the + * flag as a session id. + */ + +import './isolate.ts'; + +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +import { mergeTool } from '../lib/index.js'; +import { ParameterError, MAX_ARGV_CHARS, flagArg, argvLength } from '../lib/tools/common.js'; + +const HERE = dirname(fileURLToPath(import.meta.url)); + +/** Record the argv the tool handed the core, exactly as the stub saw it. */ +async function mergeArgv(args: Record): Promise { + const dir = mkdtempSync(join(tmpdir(), 'dsh-sessionflow-argv-')); + const record = join(dir, 'calls.txt'); + writeFileSync(record, ''); + const previous = process.env['SESSIONFLOW_ECHO_FILE']; + process.env['SESSIONFLOW_ECHO_FILE'] = record; + try { + const bridge = { + voyagerBin: process.execPath, + voyagerArgs: [join(HERE, 'fixtures', 'stub-echo.mjs')], + }; + const result = await mergeTool(bridge as never).execute(args as never, {} as never); + // The echo stub records `merge|`, and also + // returns the argv it was given, which includes it. + assert.ok(result, 'merge should have answered'); + return (result as { argv: string[] }).argv; + } finally { + if (previous === undefined) delete process.env['SESSIONFLOW_ECHO_FILE']; + else process.env['SESSIONFLOW_ECHO_FILE'] = previous; + rmSync(dir, { recursive: true, force: true }); + } +} + +/** Assert a call is refused without the core being spawned. */ +async function expectRefused( + args: Record, + pattern: RegExp, +): Promise { + const dir = mkdtempSync(join(tmpdir(), 'dsh-sessionflow-argv-')); + const record = join(dir, 'calls.txt'); + writeFileSync(record, ''); + const previous = process.env['SESSIONFLOW_ECHO_FILE']; + process.env['SESSIONFLOW_ECHO_FILE'] = record; + try { + let error: Error | undefined; + try { + await mergeTool({ + voyagerBin: process.execPath, + voyagerArgs: [join(HERE, 'fixtures', 'stub-echo.mjs')], + } as never).execute(args as never, {} as never); + } catch (e) { + error = e as Error; + } + assert.ok(error, 'the call should have been refused'); + assert.match(error.message, pattern); + assert.equal(readFileSync(record, 'utf8').trim(), '', + 'a refused argv must not reach the core'); + } finally { + if (previous === undefined) delete process.env['SESSIONFLOW_ECHO_FILE']; + else process.env['SESSIONFLOW_ECHO_FILE'] = previous; + rmSync(dir, { recursive: true, force: true }); + } +} + +// --- the `=` form: a dash-leading value is data, not a flag ----------------- + +test('merge passes options as --name=value, so a dash-leading goal is not a flag', async () => { + const argv = await mergeArgv({ session_ids: ['a:1'], goal: '--verbose' }); + assert.ok(argv.includes('--goal=--verbose'), + `expected '--goal=--verbose' in ${JSON.stringify(argv)}`); + assert.ok(!argv.includes('--goal'), + 'the space form would let argparse read --verbose as a flag'); +}); + +test('merge survives a goal that is nothing but dashes', async () => { + const argv = await mergeArgv({ session_ids: ['a:1'], goal: '---' }); + assert.ok(argv.includes('--goal=---'), JSON.stringify(argv)); +}); + +test('merge keeps an ordinary goal readable in the equals form', async () => { + const argv = await mergeArgv({ session_ids: ['a:1'], goal: 'fix the parser' }); + assert.ok(argv.includes('--goal=fix the parser'), JSON.stringify(argv)); +}); + +test('merge keeps a goal containing a flag-looking token intact', async () => { + const argv = await mergeArgv({ session_ids: ['a:1'], goal: 'fix --budget parsing' }); + assert.ok(argv.includes('--goal=fix --budget parsing'), JSON.stringify(argv)); +}); + +test('merge does not mangle CJK or an embedded equals sign', async () => { + const argv = await mergeArgv({ session_ids: ['a:1'], goal: '减少不稳定性', thread_title: 'k=v' }); + assert.ok(argv.includes('--goal=减少不稳定性'), JSON.stringify(argv)); + assert.ok(argv.includes('--thread-title=k=v'), JSON.stringify(argv)); +}); + +// --- the separator ordering trap ------------------------------------------- + +test('--json precedes the -- separator, so the core does not read it as a session', async () => { + const argv = await mergeArgv({ session_ids: ['a:1', 'b:2'], goal: 'g' }); + const sep = argv.indexOf('--'); + assert.ok(sep !== -1, 'session ids should be protected by a separator'); + const jsonIdx = argv.indexOf('--json'); + assert.ok(jsonIdx !== -1, 'the identity flag must be present exactly once'); + assert.equal(argv.filter((a) => a === '--json').length, 1, 'no duplicate --json'); + assert.ok(jsonIdx < sep, + `--json at ${jsonIdx} must come before the separator at ${sep}, ` + + 'otherwise argparse reads it as a positional session id'); +}); + +test('session ids stay after the separator, in the order given', async () => { + const argv = await mergeArgv({ session_ids: ['a:1', 'b:2', 'c:3'] }); + const sep = argv.indexOf('--'); + assert.deepEqual(argv.slice(sep + 1), ['a:1', 'b:2', 'c:3']); +}); + +test('a dash-leading session id is passed as a positional, not a flag', async () => { + const argv = await mergeArgv({ session_ids: ['-weird'] }); + const sep = argv.indexOf('--'); + assert.ok(sep !== -1, 'a dash-leading id needs the separator'); + assert.deepEqual(argv.slice(sep + 1), ['-weird']); +}); + +// --- over-long argv: refuse before the OS does, and name the culprit -------- + +test('merge refuses an over-long goal and names it, instead of surfacing ENAMETOOLONG', async () => { + await expectRefused( + { session_ids: ['a:1'], goal: 'g'.repeat(MAX_ARGV_CHARS) }, + /`goal`/); +}); + +test('merge refuses an over-long thread_title and names it', async () => { + await expectRefused( + { session_ids: ['a:1'], thread_title: 't'.repeat(MAX_ARGV_CHARS) }, + /`thread_title`/); +}); + +test('merge refuses a session list whose total argv exceeds the OS limit', async () => { + await expectRefused( + { session_ids: Array.from({ length: 400 }, () => 's'.repeat(200)) }, + /`session_ids`/); +}); + +test('merge refuses a single over-long session id and names it', async () => { + await expectRefused( + { session_ids: ['x'.repeat(MAX_ARGV_CHARS)] }, + /`session_ids`/); +}); + +test('a goal just under the ceiling is still accepted', async () => { + const argv = await mergeArgv({ session_ids: ['a:1'], goal: 'g'.repeat(1000) }); + assert.ok(argv.some((a) => a.startsWith('--goal=')), JSON.stringify(argv).slice(0, 120)); +}); + +// --- the helpers themselves ------------------------------------------------- + +test('flagArg refuses a value carrying a newline or NUL', () => { + for (const bad of ['a\nb', 'a\r\nb', 'a\u0000b']) { + assert.throws( + () => flagArg('goal', bad, { maxLength: 100 }), + ParameterError, + `${JSON.stringify(bad)} must be refused`); + } +}); + +test('flagArg renders the equals form and keeps interior whitespace', () => { + assert.equal(flagArg('goal', 'a b', { maxLength: 100 }), '--goal=a b'); + assert.equal(flagArg('goal', ' padded ', { maxLength: 100 }), '--goal= padded '); +}); + +test('flagArg refuses rather than truncating an over-long value', () => { + assert.throws( + () => flagArg('goal', 'x'.repeat(101), { maxLength: 100 }), + (e: Error) => e instanceof ParameterError && /too long/.test(e.message)); +}); + +test('argvLength counts every argument the OS will have to carry', () => { + assert.equal(argvLength(['ab', 'cde']), 5); + assert.equal(argvLength([]), 0); +}); diff --git a/tests/plugin.test.ts b/tests/plugin.test.ts index e1bd87d..3dad929 100644 --- a/tests/plugin.test.ts +++ b/tests/plugin.test.ts @@ -177,7 +177,22 @@ test('search forwards its query rather than swallowing it', async () => { }); test('merge forwards the session ids as argv, not as a shell string', async () => { + // The subject here is "argv array, never a shell string", so the assertion is + // on the SHAPE: the ids must arrive as separate, unquoted elements. The + // exact flag layout is `merge-argv.test.ts`'s subject — and it had to change + // (the `=` form, and a `--` separator) because the old layout + // ['a:1', 'b:2', '--goal', 'g'] + // is one the core's argparse cannot reliably parse: a goal beginning with a + // dash made it exit, and an id beginning with one was read as a flag. Pinning + // the exact array here would have re-frozen that defect, so this test asserts + // what it was actually written to protect. const calls = await recordedCalls((bridge) => mergeTool(bridge).execute( { session_ids: ['a:1', 'b:2'], goal: 'g' }, {} as never)); - assert.deepEqual(calls[0]?.[1], ['a:1', 'b:2', '--goal', 'g']); + const argv = calls[0]?.[1] as string[]; + assert.ok(Array.isArray(argv), 'the ids must be passed as an argv array'); + for (const id of ['a:1', 'b:2']) { + assert.ok(argv.includes(id), `'${id}' must be one whole argument, got ${JSON.stringify(argv)}`); + } + assert.ok(!argv.some((a) => a.includes(' ')), 'no argument may be a joined command line'); + assert.ok(argv.some((a) => a === '--goal=g'), 'the goal must travel as one argument, not two'); }); diff --git a/tests/recent-provider.test.ts b/tests/recent-provider.test.ts new file mode 100644 index 0000000..a5ac9aa --- /dev/null +++ b/tests/recent-provider.test.ts @@ -0,0 +1,126 @@ +/** + * Guard: a provider-filtered `sessionflow_recent` must not under-return. + * + * The core's `overview` has no provider parameter (`api.py: overview(...)` takes + * db/repo/hours/limit only), so `recent` filters the list itself — and the first + * version of that filter asked the core for `limit * 4` rows and hoped the + * wanted provider was at least a quarter of them. When it is not, the tool + * returned fewer rows than `limit` while the core had plenty more, and nothing + * in the answer said so: an incomplete list that reads as a complete one. + * + * These tests drive the tool through the echo stub with a fabricated overview, + * so "how many rows did we ask the core for" and "how many rows came back" can + * be asserted independently of any real index. + */ + +import './isolate.ts'; + +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +import { recentTool } from '../lib/index.js'; + +const HERE = dirname(fileURLToPath(import.meta.url)); + +/** A stub that answers `overview` with a caller-supplied row set. */ +function overviewStub(dir: string, rows: unknown[]): { voyagerBin: string; voyagerArgs: string[] } { + const payload = join(dir, 'overview.json'); + writeFileSync(payload, JSON.stringify({ + stats: { sessions: rows.length, events: 0, by_provider: {} }, + threads: [], + recent_sessions: rows, + })); + return { + voyagerBin: process.execPath, + voyagerArgs: [join(HERE, 'fixtures', 'stub-overview.mjs'), payload], + }; +} + +function session(i: number, provider: string): Record { + return { + id: `${provider}:${i}`, provider, native_session_id: String(i), + title: `${provider} ${i}`, updated_at: 1_700_000_000 - i, + last_user: 'x', message_count: 1, tool_count: 0, source_state: 'ok', + }; +} + +async function runRecent( + rows: unknown[], + args: Record, +): Promise<{ result: Record; requested: Record }> { + const dir = mkdtempSync(join(tmpdir(), 'dsh-sessionflow-recent-')); + const record = join(dir, 'calls.txt'); + writeFileSync(record, ''); + const previous = process.env['SESSIONFLOW_ECHO_FILE']; + process.env['SESSIONFLOW_ECHO_FILE'] = record; + try { + const result = await recentTool(overviewStub(dir, rows) as never) + .execute(args as never, {} as never) as Record; + const calls = readFileSync(record, 'utf8').trim().split('\n').filter(Boolean); + assert.equal(calls.length, 1, 'recent should make exactly one core call'); + const idx = calls[0]!.indexOf('|'); + return { result, requested: JSON.parse(calls[0]!.slice(idx + 1)) as Record }; + } finally { + if (previous === undefined) delete process.env['SESSIONFLOW_ECHO_FILE']; + else process.env['SESSIONFLOW_ECHO_FILE'] = previous; + rmSync(dir, { recursive: true, force: true }); + } +} + +test('a rare provider still fills the requested page', async () => { + // 190 claude rows, then 10 codex rows, and we want the codex ones. The old + // `limit * 4` window asked for 20 rows and saw zero codex; the answer was an + // empty list indistinguishable from "no codex sessions exist". + const rows = [ + ...Array.from({ length: 190 }, (_, i) => session(i, 'claude')), + ...Array.from({ length: 10 }, (_, i) => session(1000 + i, 'codex')), + ]; + const { result, requested } = await runRecent(rows, { provider: 'codex', limit: 5 }); + assert.equal((result.recent_sessions as unknown[]).length, 5, + 'all five codex rows must be found, not just those inside a small window'); + assert.ok((requested.limit as number) >= 10, + `a filtered call needs a window wide enough to contain the rows; asked for ${requested.limit}`); +}); + +test('an unfiltered call asks for exactly its limit', async () => { + const rows = Array.from({ length: 30 }, (_, i) => session(i, 'claude')); + const { requested } = await runRecent(rows, { limit: 7 }); + assert.equal(requested.limit, 7, 'no filter means no reason to widen the window'); +}); + +test('a filtered call never asks the core for more than its own ceiling', async () => { + const rows = Array.from({ length: 300 }, (_, i) => session(i, 'mixed')); + const { requested } = await runRecent(rows, { provider: 'mixed', limit: 10 }); + assert.ok((requested.limit as number) <= 200, + `the window must stay within the allowed ceiling, got ${requested.limit}`); +}); + +test('the result is capped at the requested limit even if more matched', async () => { + const rows = Array.from({ length: 50 }, (_, i) => session(i, 'codex')); + const { result } = await runRecent(rows, { provider: 'codex', limit: 3 }); + assert.equal((result.recent_sessions as unknown[]).length, 3); +}); + +test('a truncated filtered answer says so', async () => { + // 200 rows of one provider is the widest window the core will honour, so a + // caller asking for 5 of them cannot tell whether more existed behind it. + const rows = Array.from({ length: 200 }, (_, i) => session(i, 'codex')); + const { result } = await runRecent(rows, { provider: 'codex', limit: 5 }); + assert.equal((result.recent_sessions as unknown[]).length, 5); + assert.equal(result.truncated, true, + 'a window-limited answer must be marked, or it reads as complete'); +}); + +test('a complete filtered answer is not marked truncated', async () => { + const rows = [ + ...Array.from({ length: 20 }, (_, i) => session(i, 'claude')), + ...Array.from({ length: 3 }, (_, i) => session(100 + i, 'codex')), + ]; + const { result } = await runRecent(rows, { provider: 'codex', limit: 5 }); + assert.equal((result.recent_sessions as unknown[]).length, 3); + assert.equal(result.truncated, undefined, 'nothing was cut, so nothing should say so'); +}); From d46a50e0d32f8a5cbe804437f62e13495d625daa Mon Sep 17 00:00:00 2001 From: SunnyRad <344633306+SunnyRad@users.noreply.github.com> Date: Tue, 10 Nov 2026 18:04:10 +0800 Subject: [PATCH 3/4] fix(recent): signal a window-limited answer even when the filter matches nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `truncated` marker added with the provider-filter fix required `matched.length > limit` — "the filter cut something". That misses the case it most needed to catch: when other providers fill the window, the wanted provider can sit entirely behind it, so the filter matches nothing and cuts nothing. The caller then gets an empty list with no marker, indistinguishable from "no sessions from that provider exist". Measured on a real 255-session index (250 fresher `claude` rows ahead of 5 `codex` rows): {provider:'codex', limit:5} -> 0 rows, truncated absent sessions {platform:'codex'} -> 5 rows The condition is now "the core filled the window", gated on the call being filtered, so a normal unfiltered page is not relabelled as clipped. The fixture could not have caught this: stub-overview.mjs ignored `limit` and always returned the whole payload, making "the core gave us everything" and "the core gave us the window" the same answer. It now clips as a real core does. Mutation-checked, each restored afterwards (89 tests, 88 pass): baseline 1 red (pre-existing, unrelated) predicate back to matched.length > limit 2 red, incl. the new behind-the-window test provider guard dropped 2 red, incl. the new unfiltered-full-page test stub stops honouring limit 2 red, incl. the new test restored 1 red (the same pre-existing) The one standing red is `search round-trips a CJK query without mangling it`, which fails identically on main in this environment (lone-surrogate argv encoding at the core boundary) and is not touched by this branch. --- CHANGELOG.md | 17 ++++++++++++++++ src/tools/recent.ts | 21 +++++++++++++++----- tests/fixtures/stub-overview.mjs | 9 +++++++++ tests/recent-provider.test.ts | 34 ++++++++++++++++++++++++++++++++ 4 files changed, 76 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 74261ce..256dbf0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,6 +59,23 @@ Windows 冷启动四关 + 真实 bridge 调用通过，Linux CI 补完整 boot 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 diff --git a/src/tools/recent.ts b/src/tools/recent.ts index 2dbe1b0..e6827f7 100644 --- a/src/tools/recent.ts +++ b/src/tools/recent.ts @@ -79,11 +79,22 @@ export function recentTool(bridge: BridgeOptions) { const matched = args.provider ? ov.recent_sessions.filter((s) => s.provider === args.provider) : ov.recent_sessions; - // Say when the window, not the filter, is what limited the answer: a - // caller that got `limit` rows of a filtered list cannot otherwise tell - // a complete answer from a truncated one. - const truncated = matched.length > limit && want === ov.recent_sessions.length - && ov.recent_sessions.length >= PROVIDER_FILTER_WINDOW; + // Say when the window, not the filter, is what limited the answer. + // + // The condition is "the core filled the window", NOT "we had to cut + // more than `limit`". Those differ in the case that matters: the + // wanted provider can sit entirely BEHIND the window, where the filter + // matches zero rows and cuts nothing — yet the empty list is exactly as + // uninformed as a cut one. The previous predicate required + // `matched.length > limit`, so that case reported no signal at all and + // an empty answer stood in for "no such sessions exist" (measured on a + // real 255-session index: 250 fresher `claude` rows ahead of 5 `codex` + // rows, `{provider:'codex', limit:5}` returned 0 rows, `truncated` + // absent, while `sessions {platform:'codex'}` shows the 5 exist). + // + // Only a filtered call can be in this position: an unfiltered one asks + // for exactly `limit` and a full page is the answer, not a truncation. + const truncated = Boolean(args.provider) && ov.recent_sessions.length >= want; return { stats: ov.stats, threads: ov.threads, diff --git a/tests/fixtures/stub-overview.mjs b/tests/fixtures/stub-overview.mjs index d2dd124..9fc606e 100644 --- a/tests/fixtures/stub-overview.mjs +++ b/tests/fixtures/stub-overview.mjs @@ -37,6 +37,15 @@ if (argv.includes('integration-info')) { record(`${req.op}|${JSON.stringify(req.params ?? {})}`); let result = {}; try { result = JSON.parse(readFileSync(payloadPath, 'utf8')); } catch { /* ignore */ } + // Honour `limit` the way the real core does. Without this the stub answered + // with the whole payload regardless of what was asked for, so a test could + // not tell "the core gave us everything" from "the core gave us the window" + // — which is precisely the distinction the `truncated` signal rests on. A + // fixture that cannot clip cannot see a window-limited answer. + const lim = req.params?.limit; + if (Array.isArray(result.recent_sessions) && typeof lim === 'number') { + result = { ...result, recent_sessions: result.recent_sessions.slice(0, lim) }; + } process.stdout.write(JSON.stringify({ id: 1, result })); }); } else { diff --git a/tests/recent-provider.test.ts b/tests/recent-provider.test.ts index a5ac9aa..fb194c4 100644 --- a/tests/recent-provider.test.ts +++ b/tests/recent-provider.test.ts @@ -124,3 +124,37 @@ test('a complete filtered answer is not marked truncated', async () => { assert.equal((result.recent_sessions as unknown[]).length, 3); assert.equal(result.truncated, undefined, 'nothing was cut, so nothing should say so'); }); + +test('a provider sitting entirely BEHIND the window is marked, not returned empty', async () => { + // The case the first version of this signal could not see, and the reason the + // stub below now honours `limit`. + // + // 250 fresher `claude` rows fill the widest window the core will honour, and + // the wanted `codex` rows are older than all of them. The filter therefore + // matches NOTHING and cuts nothing — but "0 codex rows" is exactly as + // uninformed as a cut answer, because codex sessions do exist. The old + // predicate required `matched.length > limit`, so this returned an empty list + // with no marker, indistinguishable from "there is no codex history". + // + // Measured against a real 255-session index before the fix: 0 rows, + // `truncated` absent, while `sessions {platform:'codex'}` listed 5. + const rows = [ + ...Array.from({ length: 250 }, (_, i) => session(i, 'claude')), + ...Array.from({ length: 5 }, (_, i) => session(1000 + i, 'codex')), + ]; + const { result } = await runRecent(rows, { provider: 'codex', limit: 5 }); + assert.equal((result.recent_sessions as unknown[]).length, 0, + 'the window genuinely does not reach them — the fix is the signal, not the rows'); + assert.equal(result.truncated, true, + 'an empty answer from a full window must say the window was full'); +}); + +test('an unfiltered full page is not called truncated', async () => { + // An unfiltered call asks for exactly `limit`, so getting `limit` rows back is + // the answer itself; marking it would make every ordinary page look clipped. + const rows = Array.from({ length: 30 }, (_, i) => session(i, 'claude')); + const { result } = await runRecent(rows, { limit: 7 }); + assert.equal((result.recent_sessions as unknown[]).length, 7); + assert.equal(result.truncated, undefined, + 'a page that was asked for in full is not truncated'); +}); From 63ee20832457235eb0697b08295acb53cf70197b Mon Sep 17 00:00:00 2001 From: SunnyRad <344633306+SunnyRad@users.noreply.github.com> Date: Tue, 10 Nov 2026 20:28:22 +0800 Subject: [PATCH 4/4] fix(merge): refuse a bad session id instead of dropping it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reviewer asked for three things; this is two of them, and the third turned out to be a wrong comment rather than a wrong guard. 1. session_ids validation `session_ids` was read with `filter(isString && length > 0)`, so `['a:1', 42, ' ']` merged ONE session and reported success. The merge is the unit of consent: the caller names a set, and the WorkThread contains exactly that set. Dropping an entry is not leniency, it is a different operation performed on the caller's behalf without saying so. Every element is now checked rather than filtered, the refusal names the offending index (`session_ids[1]`) so the other ids can be kept, and a whitespace-only entry is refused as blank rather than sent to the core as an unresolvable positional. 2. The fractional-limit comment was wrong about `search` The comment claimed the core rejects `3.7` with `slice indices must be integers`. Measured on voyager 1.3.0, that is true of `overview` only: search (api.py:254, int(limit or 20)) 3.7 -> 3 rows, SILENTLY overview (api.py:77/:94, raw slice) 3.7 -> slice-indices error search limit 0 -> 20 rows (0 reads as absent) overview limit 0 -> 0 rows search limit -2 -> 33,277 rows (no cap) overview limit -3 -> 91 of 94 rows So `search` under-returns quietly, which is worse than the crash the comment described: a truncated answer looks complete. The guard was already right; its justification was not, and a wrong justification is how a later change deletes a guard for the wrong reason. Both behaviors are now recorded where they apply, and the test header states them per-op. 3. The `truncated` fix from the previous commit is unchanged Already in this branch as d46a50e; the reviewer's summary of it (let the fixture clip like a real core, and judge incompleteness by "the core filled the window" gated on the call being filtered) is what it does. Tests: 96 total, 76 pass, 0 fail, 20 skipped. The skips are the core-dependent integration tests, which need a `python` that can `import voyager`; with a real core wired in, 93 pass and the 3 remaining failures reproduce identically on main in this environment — two are tests that `delete process.env['VOYAGER_BIN']` and never restore it, and the CJK one is the core's own lone-surrogate bug: search {query: "这是绝对不存在的查询词zzz"} -> 'utf-8' codec can't encode character '\udcaf' in position 6 That is reproducible straight against the core's stdio API with no plugin involved, so it is not this branch's regression. Mutation-checked, each restored afterwards: session_ids back to the silent filter 5 red, incl. all 4 new truncated back to matched.length > limit 1 red stub stops honouring limit 1 red restored 0 red --- CHANGELOG.md | 29 +++++++++++++++++++ src/tools/common.ts | 19 ++++++++++-- src/tools/merge.ts | 31 ++++++++++++++++++-- tests/merge-argv.test.ts | 59 ++++++++++++++++++++++++++++++++++++++ tests/numeric-args.test.ts | 34 +++++++++++++++------- 5 files changed, 155 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 256dbf0..b46a769 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -83,6 +83,35 @@ Windows 冷启动四关 + 真实 bridge 调用通过，Linux CI 补完整 boot 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. diff --git a/src/tools/common.ts b/src/tools/common.ts index a0d002c..ee383aa 100644 --- a/src/tools/common.ts +++ b/src/tools/common.ts @@ -65,9 +65,22 @@ function refuse(name: string, b: Bounds, got: unknown, reason: string): never { /** * A bounded integer argument. * - * Whole numbers only: `3.7` is not a "smaller 4" to the core, it is a value - * SQLite rejects with `slice indices must be integers`, which surfaces as an - * opaque error far from the caller's mistake. + * 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; diff --git a/src/tools/merge.ts b/src/tools/merge.ts index b80bd18..be98ba1 100644 --- a/src/tools/merge.ts +++ b/src/tools/merge.ts @@ -42,9 +42,34 @@ export function mergeTool(bridge: BridgeOptions) { // Built and bounded OUTSIDE `guarded`, so a refusal stays a // ParameterError naming the field instead of being flattened into the // generic message a core failure produces. Nothing is spawned yet. - const ids = Array.isArray(args.session_ids) - ? args.session_ids.filter((s): s is string => typeof s === 'string' && s.length > 0) - : []; + // Every element is CHECKED, not filtered. The previous version kept only + // elements that were non-empty strings and dropped the rest, so + // `['a:1', 42, ' ']` merged ONE session and reported success: the + // caller was told the merge happened, and had no way to see that two of + // the ids it named were discarded. A value that is not a session id is + // not "an id to skip" — silently narrowing the merge changes which + // sessions the WorkThread ends up containing, and it changes it quietly. + if (!Array.isArray(args.session_ids)) { + throw new ParameterError( + `\`session_ids\` must be an array of session ids ` + + `(got ${JSON.stringify(args.session_ids)}).`); + } + const ids: string[] = []; + args.session_ids.forEach((raw: unknown, i: number) => { + if (typeof raw !== 'string') { + throw new ParameterError( + `\`session_ids[${i}]\` must be text (got ${JSON.stringify(raw)}).`); + } + // Whitespace-only is blank, not an id: it would be passed through as a + // positional argument and the core would fail to resolve it with a + // message that does not point back at `session_ids`. + if (raw.trim().length === 0) { + throw new ParameterError( + `\`session_ids[${i}]\` is blank; every entry must name a session ` + + `(got ${JSON.stringify(raw)}).`); + } + ids.push(raw); + }); if (ids.length === 0) { throw new ParameterError('sessionflow_merge needs at least one session id.'); } diff --git a/tests/merge-argv.test.ts b/tests/merge-argv.test.ts index 2d6f612..d4afdfb 100644 --- a/tests/merge-argv.test.ts +++ b/tests/merge-argv.test.ts @@ -187,6 +187,65 @@ test('a goal just under the ceiling is still accepted', async () => { assert.ok(argv.some((a) => a.startsWith('--goal=')), JSON.stringify(argv).slice(0, 120)); }); +// --- session_ids: a bad entry is refused, never quietly dropped ------------- +// +// The whole merge is the unit of consent: the caller named a set of sessions and +// the WorkThread will contain exactly those. Dropping an entry is therefore not +// leniency, it is a different operation performed on the caller's behalf without +// saying so — `['a:1', 42]` used to merge one session and report success. + +test('merge refuses a non-string session id instead of merging the rest', async () => { + await expectRefused({ session_ids: ['a:1', 42] }, /`session_ids\[1\]`.*must be text/); +}); + +test('merge refuses a null session id instead of skipping it', async () => { + await expectRefused({ session_ids: ['a:1', null] }, /`session_ids\[1\]`.*must be text/); +}); + +test('merge refuses a whitespace-only session id', async () => { + await expectRefused({ session_ids: ['a:1', ' '] }, /`session_ids\[1\]`.*blank/); +}); + +test('merge refuses an empty-string session id', async () => { + await expectRefused({ session_ids: [''] }, /`session_ids\[0\]`/); +}); + +test('merge refuses session_ids that is not an array at all', async () => { + // Decided by the HOST, not by the tool: `defineTool` compiles the parameter + // spec into JSON Schema, so a bare string is rejected with + // `invalid arguments: "session_ids" must be an array` before `execute` runs. + // The tool keeps its own array check anyway — the schema is the host's + // business, and a tool that trusts it would drop its guard the moment the + // spec changed — but the observable refusal is the host's wording, so that is + // what is asserted here. Either way nothing is spawned. + await expectRefused({ session_ids: 'a:1' }, /"session_ids" must be an array/); +}); + +test('a refusal names the offending index, so the caller can fix that one entry', async () => { + // Index 2, not "one of your ids": the caller has to know WHICH entry to remove + // when the other nine are perfectly good. + let error: Error | undefined; + try { + await mergeTool({ + voyagerBin: process.execPath, + voyagerArgs: [join(HERE, 'fixtures', 'stub-echo.mjs')], + } as never).execute({ session_ids: ['a:1', 'b:2', 7] } as never, {} as never); + } catch (e) { + error = e as Error; + } + assert.ok(error, 'the call should have been refused'); + assert.match(error.message, /`session_ids\[2\]`/, + `the message must name index 2; got: ${error.message}`); +}); + +test('every valid id still reaches the core in order', async () => { + // The check must not become a filter: a list of good ids is passed through + // whole, in the order given. + const argv = await mergeArgv({ session_ids: ['a:1', 'b:2', 'c:3'] }); + const sep = argv.indexOf('--'); + assert.deepEqual(argv.slice(sep + 1), ['a:1', 'b:2', 'c:3']); +}); + // --- the helpers themselves ------------------------------------------------- test('flagArg refuses a value carrying a newline or NUL', () => { diff --git a/tests/numeric-args.test.ts b/tests/numeric-args.test.ts index 20d6261..94cabc5 100644 --- a/tests/numeric-args.test.ts +++ b/tests/numeric-args.test.ts @@ -3,20 +3,32 @@ * the core. * * Three distinct failures motivate this file, all measured on voyager 1.3.0 - * against a real index (94 sessions / 526,991 events): + * against a real index (94 sessions / 526,991 events). * - * 1. `search {limit: -2}` -> count 1241, 480,826 bytes of JSON. The core does - * `int(limit or 20)` and hands the value to SQLite as `LIMIT -2`, which - * removes the cap rather than applying one. A model call with a stray - * minus sign is therefore an unbounded read into its own context. + * The core applies `limit` differently per op, so the two ops are listed + * separately rather than as one "the core rejects it" claim — that claim is + * false for `search` and was in this header before the behavior was measured + * op by op: * - * 2. `search {limit: 3.7}` -> the request itself is fine; SQLite rejects it - * later with `slice indices must be integers or None`, which surfaces as a - * message with no trace of the argument that caused it. + * `search` (api.py:254, `int(limit or 20)`): + * 1. `limit: -2` -> count 33,277, 480,826 bytes of JSON. `LIMIT -2` removes + * the cap rather than applying one, so a stray minus sign is an unbounded + * read into the model's own context. + * 2. `limit: 3.7` -> 3 rows. `int()` TRUNCATES it. No error, no warning; the + * caller just gets fewer hits than it asked for, which looks complete. + * 3. `limit: 0` -> 20 rows. `limit or 20` treats 0 as absent, so asking for + * nothing means asking for the default. * - * 3. `overview {limit: 0}` -> 0 rows, not 12. `int(limit or 20)` treats 0 as - * absent, so "ask for nothing" silently means "ask for the default", while - * `overview {limit: -3}` means "ask for everything" (91 rows). + * `overview` (api.py:77/:94, raw slice — no `int()`, so none of the above): + * 4. `limit: 3.7` -> `slice indices must be integers or None or have an + * __index__ method`, an error naming no argument. + * 5. `limit: 0` -> 0 rows (`candidates[:0]`), NOT the default page. + * 6. `limit: -3` -> 91 of 94 rows: Python's negative slice drops the newest + * three rather than meaning "all" or "none". + * + * Both ops are therefore validated here, ahead of the spawn, but for different + * reasons: on `search` an out-of-range value is silently reinterpreted, and on + * `overview` a fractional one is an opaque crash. * * The tools must refuse or clamp BEFORE spawning a child, and the refusal must * be identifiable as a parameter problem rather than folded into the generic