Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .ai/agent-practices.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,12 @@ execution environment. Rules of thumb:
visible in the Work Files sidebar tab for debugging.
- **No `sleep N && command` to wait for background work** — if a task runs in the background,
react to its completion signal; don't poll with a fixed sleep.
- **`test/trigger-watcher.test.js` flaking under load is a known, tolerated pattern, not a
regression to chase** — it uses real timers and real `fs.watch`; `npm test` already runs it
serially and after everything else (see `.ai/contexts/trigger-watcher.md`, "timing tests and
host load"). If it still fails under a genuinely loaded machine, rerun with
`SWITCHBOARD_TEST_TIME_SCALE=<n> node --test test/trigger-watcher.test.js` (env, default 1)
before assuming the code broke.

## 5. Memory / notes hygiene

Expand Down
56 changes: 56 additions & 0 deletions .ai/contexts/trigger-watcher.md
Original file line number Diff line number Diff line change
Expand Up @@ -1337,6 +1337,62 @@ Verified both mutations turn the test red: negating the comparison
(`midBusy === true`) and deleting the clause entirely — both fail on the new
step-level assertion with `actual: 'confirmed'`.

## Timing tests and host load

`test/trigger-watcher.test.js` uses real timers and real `fs.watch` against
fixed wall-clock budgets (no fake-timer injection yet). It measures the host,
not only the code under test, so it is far more sensitive to CPU contention
than the rest of the suite. `npm test` / `npm run coverage` (`scripts/run-tests.js`)
run it alone, serially, after every other test file, specifically to remove
the self-inflicted contention of ~150 sibling test-file processes running
concurrently — that alone fixed most of what issue #260 measured. What's left
after serializing is contention from outside the suite (other agents,
antivirus, a second `node --test` run): under a synthetic load of that kind
(measured 2026-09-11 with 8 CPU-saturating processes on an 8-core machine),
several timing-sensitive tests still failed:

- `waitForFile`'s ceiling (`maxMs`) is a ceiling, not a measurement — it is
scaled by `SWITCHBOARD_TEST_TIME_SCALE` (env, default 1) inside the helper
itself, so every call site benefits without being touched individually.
- A handful of assertions put a tight UPPER bound on elapsed time or
`waited_ms` (e.g. the chain instant-reply test's verify-window checks).
Those upper bounds are wrapped in the same `scaleUp()` helper. Lower bounds
that prove an ordering or a minimum wait happened are never scaled by this
factor — a mutation that breaks that ordering must still turn the test red
regardless of `SWITCHBOARD_TEST_TIME_SCALE`.
- Two tests (`W7 dies during wait`, `waitForBusyFall settle window still
applies once a rise is observed`) and the shared `makeChainCtx` auto-turn
schedule had margins so tight that ordinary scheduling jitter under load
could produce a *smaller* measured value than the nominal minimum (e.g.
`waited_ms=128` against a `>=200` floor) — not because the code was slower,
but because the test's own local `setTimeout` schedule and the poll
detecting it are two independent clocks that can drift apart under
contention. These were widened **unconditionally** (not via
`SWITCHBOARD_TEST_TIME_SCALE`, since the pre-commit hook runs at the
default scale of 1) by multiplying every constant in the affected
schedule by the same factor — which is time-linear and so preserves the
exact margin between "the code is right" and "the code is wrong",
preserving mutation power.
- `SWITCHBOARD_TEST_TIME_SCALE` is opt-in, for exceptionally loaded machines:
`SWITCHBOARD_TEST_TIME_SCALE=3 node --test test/trigger-watcher.test.js`.

**What a real clock injection would take** (out of scope for issue #260;
`trigger-watcher.js` has 29 `setTimeout`/`Date.now()` call sites across
~1384 lines): thread a clock object (`{ now(), setTimeout(), clearTimeout() }`,
the pattern already used in `remote-watch.js`, `remote-index.js`, and
`remote-activity.js`) through `pollLoop`, `waitForIdle`, `waitForComposerFree`,
`submitWithVerify`, and `waitForBusyFall` — the five functions that own a
wall-clock wait — plus the `fs.watch` debounce in `start()`. Every test that
asserts a specific `waited_ms`/`elapsed` value (roughly 20 of the ~120 tests
in the file, concentrated in the "submit-verify", "chain", and "waitForBusyFall
settle window" sections) would then drive a fake clock instead of real
`setTimeout`, making the pass/fail boundary exact instead of a tolerance
band, and removing sensitivity to host load entirely. The tests that exercise
real `fs.watch` (the startup-scan and live-watcher dispatch tests) would stay
on real timers as smoke tests, per the issue's own suggested direction, since
`fs.watch` itself cannot be faked without swapping the whole file-system
observation layer.

## Change-also checklist

- If you rename `_cliBusy` on `session` in `main.js`, update `isSessionBusy` in `trigger-context.js`.
Expand Down
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@
"start": "npm run bundle:codemirror && electron .",
"lint": "eslint .",
"pretest": "npm run lint",
"test": "node --test",
"coverage": "c8 node --test",
"test": "node scripts/run-tests.js",
"coverage": "c8 node scripts/run-tests.js",
"electron": "electron .",
"electron-dev": "SWITCHBOARD_DATA_DIR=\"$HOME/.switchboard-dev\" electron .",
"bundle:codemirror": "esbuild public/codemirror-setup.js --bundle --outfile=public/codemirror-bundle.js --format=iife --platform=browser --minify",
Expand Down
42 changes: 42 additions & 0 deletions scripts/run-tests.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
#!/usr/bin/env node
// Runs the node:test suite in two stages, cross-platform (no shell-specific
// syntax, so this works the same under cmd.exe and under a POSIX shell).
//
// Stage 1: every test file except trigger-watcher.test.js, node's own default
// concurrency.
// Stage 2: trigger-watcher.test.js alone, serially, with a generous timeout.
// It uses real timers + real fs.watch against wall-clock budgets (no fake-timer
// injection yet -- see .ai/contexts/trigger-watcher.md, "timing tests and host
// load"), so it is far more sensitive to CPU contention from sibling test
// processes than the rest of the suite. Running it alone, after everything
// else, removes that self-inflicted contention; see issue #260.
'use strict';

const { spawnSync } = require('child_process');
const fs = require('fs');
const path = require('path');

const TEST_DIR = path.join(__dirname, '..', 'test');
const ISOLATED_FILE = 'trigger-watcher.test.js';

const mainFiles = fs.readdirSync(TEST_DIR)
.filter((name) => name.endsWith('.js') && name !== ISOLATED_FILE)
.map((name) => path.join('test', name));

function run(args) {
const result = spawnSync(process.execPath, args, { stdio: 'inherit' });
if (result.error) throw result.error;
return result.status === null ? 1 : result.status;
}

const mainStatus = run(['--test', ...mainFiles]);

// No --test-timeout: with an explicit file operand node applies it to the
// file-level entry too, and this file legitimately runs for minutes on CI.
const isolatedStatus = run([
'--test',
'--test-concurrency=1',
path.join('test', ISOLATED_FILE),
]);

process.exit(mainStatus !== 0 ? mainStatus : isolatedStatus);
Loading
Loading