test(coding-agent): removed tests using source-grep patterns

- Removed multiple test files and cases that relied on brittle source string matching for validation.
- Updated project architecture documentation to explicitly prohibit source-grep style testing patterns.
- Eliminated legacy reproduction tests for issues that reached project maturity.
This commit is contained in:
can1357
2026-06-26 15:48:09 +02:00
parent c39b655ae7
commit 44368e3459
10 changed files with 3 additions and 335 deletions
+2 -1
View File
@@ -47,7 +47,7 @@ Unless user tells you exactly what to write:
: new Worker(new URL("./<worker>.ts", import.meta.url).href, { type: "module" });
```
When the process was started from the omp CLI — source `cli.ts`, npm-bundle `dist/cli.js`, or compiled binary — `workerHostEntry()` is `Bun.main` and the worker re-enters the single entry module, so no per-worker `--compile` entrypoints or bundle entries exist. Outside a CLI host (`bun test`, SDK embedding, standalone `omp-stats`) it returns `null` and the direct-module fallback loads the worker source. New worker kinds MUST add their selector to the dispatch table in `cli.ts` and keep the fallback branch.
History: `with { type: "file" }` only copied the entry as a raw asset (workers crashed silently in compiled binaries — issues #1011, #1027), and the later literal-path + extra-entrypoint pattern required keeping spawn literals and two build scripts in sync (issue #1150). The repro tests for those issues now pin the worker-host contract instead.
History: `with { type: "file" }` only copied the entry as a raw asset (workers crashed silently in compiled binaries — issues #1011, #1027), and the later literal-path + extra-entrypoint pattern required keeping spawn literals and two build scripts in sync (issue #1150). The smoke probe below is the live validation of this contract.
Validate any new worker with the dedicated smoke probe: `omp --smoke-test` spawns the stats sync worker and the tiny-model subprocess, pings them, and exits — it's wired into `ci:test:smoke` and `scripts/install-tests/run-ci.sh` so binary, source-link, and tarball installs all exercise it. Add a sibling smoke if the new worker is on a different module graph.
## Bun Over Node
@@ -226,6 +226,7 @@ Test the contract the system exposes — not the easiest internal detail to asse
- Smoke tests are acceptable only when they catch a failure mode narrower tests would miss. "Package boots" or "command starts" alone is not enough.
- Assert exact strings, ordering, and formatting only when downstream code parses or depends on the exact bytes. Otherwise assert semantic content.
- Compile-time guarantees → type checks/type tests, not runtime placeholders.
- **Never source-grep.** A test that reads an implementation file (`.ts`/`.rs`/build script) and asserts on its *text* — `expect(src).toContain("someCall()")`, `.toMatch(/import .../)`, `.not.toContain("oldName")`, or "comment must say X" — is banned. It tests how code *looks*, not what it *does*: it breaks on harmless refactors (comment reflow, rename, import reorder) and passes while the behavior is broken. Assert the observable contract instead (run the code, check output/state/error), use the runtime smoke probe for wiring you cannot exercise in-process, and enforce structural invariants (no value-import of X, no self-import) with a type test or a lint/biome rule — never a string scan of the source. (Reading a file your code *wrote* — apply-patch result, generated bundle, temp fixture — and asserting on that output is fine; that is behavior, not a source grep.)
- Don't add tests for tiny low-risk changes unless they protect a real contract or fix a regression-prone edge case.
- Prefer focused package-local verification for the changed area.
@@ -113,22 +113,4 @@ describe("extension loader host runtime binding", () => {
expect(hookResult.hooks).toHaveLength(1);
expect(hookResult.hooks[0].handlers.has("identity:event")).toBe(true);
});
it("keeps runtime loaders free of bare package self-imports", async () => {
// Normal workspace resolution can make a bare self-import resolve to the same module,
// so keep a narrow static tripwire for the global-install mixed-version layout.
const loaderPaths = [
path.join(import.meta.dir, "..", "src", "extensibility", "extensions", "loader.ts"),
path.join(import.meta.dir, "..", "src", "extensibility", "custom-tools", "loader.ts"),
path.join(import.meta.dir, "..", "src", "extensibility", "custom-commands", "loader.ts"),
path.join(import.meta.dir, "..", "src", "extensibility", "hooks", "loader.ts"),
];
for (const loaderPath of loaderPaths) {
const source = await Bun.file(loaderPath).text();
expect(source).not.toMatch(/from\s+["']@oh-my-pi\/pi-coding-agent["']/);
expect(source).not.toMatch(/import\(\s*["']@oh-my-pi\/pi-coding-agent["']\s*\)/);
}
});
});
@@ -1,50 +0,0 @@
import { describe, expect, it } from "bun:test";
import * as path from "node:path";
/**
* Regression for https://github.com/can1357/oh-my-pi/issues/1011
*
* In v14.5.13 `spawnTabWorker` (in `src/tools/browser/tab-supervisor.ts`)
* resolved the worker entry as `new URL("./tab-worker-entry.ts", import.meta.url)`
* and passed `.href` to `new Worker(...)`. Bun's `--compile` static analyzer
* cannot discover that pattern, so the entry was never embedded in the
* single-file binary and the runtime symptom was "Timed out initializing
* browser tab worker".
*
* The original fix used an `import "./worker.ts" with { type: "file" }`
* trick. That copied the entry as a raw asset but could not resolve its
* relative imports inside the compiled binary, so the worker still failed
* to load (issue #1027 was the same root cause, retriggered).
*
* The current contract is simpler: when the process was started from the omp
* CLI (source, npm bundle, or compiled binary), spawn sites re-enter the
* declared worker-host entry — `new Worker(workerHostEntry(), { argv })` — and
* the CLI dispatches the hidden argv selector. Outside a CLI host (bun test,
* SDK embedding) they load the worker module directly. No separate worker
* module is ever bundled or listed as a `--compile` entrypoint.
*/
describe("issue #1011 — tab worker must re-enter the CLI entrypoint", () => {
const packageDir = path.resolve(import.meta.dir, "..");
const supervisorPath = path.join(packageDir, "src/tools/browser/tab-supervisor.ts");
const buildBinaryPath = path.join(packageDir, "scripts/build-binary.ts");
const workerArg = "__omp_worker_tab";
it("tab-supervisor re-enters the worker-host entry with the argv selector", async () => {
const source = await Bun.file(supervisorPath).text();
expect(
source.includes("workerHostEntry()"),
"tab-supervisor.ts must spawn via the declared worker-host entry",
).toBe(true);
expect(source).toContain(`argv: ["${workerArg}"]`);
expect(
source.includes('new URL("./tab-worker-entry.ts", import.meta.url)'),
"tab-supervisor.ts must keep the direct-module fallback for non-CLI hosts",
).toBe(true);
});
it("build-binary.ts no longer lists tab-worker-entry as a separate --compile entrypoint", async () => {
const source = await Bun.file(buildBinaryPath).text();
expect(source).not.toContain("./src/tools/browser/tab-worker-entry.ts");
});
});
@@ -1,51 +0,0 @@
import { describe, expect, it } from "bun:test";
import * as path from "node:path";
/**
* Regression for https://github.com/can1357/oh-my-pi/issues/1150
*
* In v15.1.3 `omp stats` crashed in the published Linux/macOS/Windows
* binaries with `BuildMessage: ModuleNotFound resolving
* "./packages/stats/src/sync-worker.ts" (entry point)`. The dev-mode build
* script `packages/coding-agent/scripts/build-binary.ts` listed the three
* worker entrypoints required by AGENTS.md, but the release script
* `scripts/ci-release-build-binaries.ts` — the one that actually builds the
* shipped artifacts — did not. The `new Worker("./packages/<pkg>/src/...")`
* literal at the spawn site fooled Bun's `--compile` static analyzer into
* keeping the call site, but without the matching `--compile` entrypoint
* the worker module was never emitted into bunfs and the runtime tried to
* bundle it on the fly, which fails in `$bunfs`.
*
* The current contract is simpler: every Worker re-enters the CLI entrypoint
* and selects its worker body via `WorkerOptions.argv`, so release builds no
* longer need to list the worker modules as extra `--compile` entrypoints.
* Runtime coverage lives in `omp --smoke-test`.
*/
describe("issue #1150 — release/dev builds route workers through the CLI entrypoint", () => {
const repoRoot = path.resolve(import.meta.dir, "../../..");
const ciScriptPath = path.join(repoRoot, "scripts/ci-release-build-binaries.ts");
const devScriptPath = path.join(repoRoot, "packages/coding-agent/scripts/build-binary.ts");
// Repo-root-relative CLI literal — every runtime worker spawn site uses this
// same entry plus a hidden argv selector.
const workerEntrypoints = [
"./packages/stats/src/sync-worker.ts",
"./packages/coding-agent/src/tools/browser/tab-worker-entry.ts",
"./packages/coding-agent/src/eval/js/worker-entry.ts",
];
it("release/dev build scripts do not list worker modules as explicit --compile entrypoints", async () => {
const releaseSource = await Bun.file(ciScriptPath).text();
const devSource = await Bun.file(devScriptPath).text();
for (const entry of workerEntrypoints) {
expect(releaseSource).not.toContain(`"${entry}"`);
}
for (const entry of [
"../stats/src/sync-worker.ts",
"./src/tools/browser/tab-worker-entry.ts",
"./src/eval/js/worker-entry.ts",
]) {
expect(devSource).not.toContain(`"${entry}"`);
}
});
});
@@ -16,7 +16,7 @@
*/
import { describe, expect, it } from "bun:test";
import * as path from "node:path";
import { createTinyTitleSubprocess, TINY_WORKER_ARG } from "@oh-my-pi/pi-coding-agent/tiny/title-client";
import { createTinyTitleSubprocess } from "@oh-my-pi/pi-coding-agent/tiny/title-client";
describe("issue #1606 — tiny model lives in an isolated subprocess", () => {
it("ping/pongs through the spawned worker subprocess and tears it down cleanly", async () => {
@@ -41,16 +41,6 @@ describe("issue #1606 — tiny model lives in an isolated subprocess", () => {
expect(exitCode).toBe(0);
}, 30_000);
it("CLI dispatches the flag that `title-client.ts` passes to the spawned child", async () => {
// `tinyWorkerSpawnCmd()` and the cli switch must agree on the exact
// flag, character-for-character — the spawned `bun`/binary sees only
// `argv` and there is no fallback path that "re-routes" the worker
// on misnamed flags. Pin the spelling on both ends.
const cliSource = await Bun.file(new URL("../src/cli.ts", import.meta.url)).text();
expect(cliSource).toContain(`"${TINY_WORKER_ARG}"`);
expect(cliSource).toContain("runTinyWorker");
});
it("surfaces unexpected signal exits so in-flight callers don't await forever", async () => {
// If the child dies from a signal we did NOT request — SIGSEGV from a
// native crash (the original Windows shutdown bug, now relocated to
@@ -1,5 +1,4 @@
import { describe, expect, it } from "bun:test";
import * as path from "node:path";
import { EnhancedPasteController } from "@oh-my-pi/pi-coding-agent/utils/enhanced-paste";
/**
@@ -103,23 +102,3 @@ describe("issue #2127 — enhanced-paste text must follow focus", () => {
expect(editor.pasted).toEqual(["fallback body"]);
});
});
describe("issue #2127 — InputController wires enhanced-paste through focus", () => {
const packageDir = path.resolve(import.meta.dir, "..");
const controllerPath = path.join(packageDir, "src/modes/controllers/input-controller.ts");
it("input-controller routes the enhanced-paste text callback through ui.getFocused", async () => {
const source = await Bun.file(controllerPath).text();
// Anchor the assertion on the EnhancedPasteController construction block:
// the `pasteText` callback must consult the focused component, not stash
// every payload into `this.ctx.editor` unconditionally. A future refactor
// that drops `getFocused()` from this callback re-introduces the bug.
const constructionStart = source.indexOf("new EnhancedPasteController(");
expect(constructionStart, "InputController must still construct EnhancedPasteController").toBeGreaterThan(-1);
const constructionSlice = source.slice(constructionStart, constructionStart + 2_000);
expect(
constructionSlice.includes("getFocused()"),
"The enhanced-paste callback must consult ui.getFocused() so modal Input prompts (OAuth API-key entry, OTPs, redirect URLs) receive the pasted text instead of the detached main editor (#2127).",
).toBe(true);
});
});
@@ -19,7 +19,6 @@ import { describe, expect, it } from "bun:test";
import * as path from "node:path";
import {
createMnemopiEmbedSubprocess,
MNEMOPI_EMBED_WORKER_ARG,
MnemopiEmbedClient,
type MnemopiEmbedWorkerHandle,
} from "@oh-my-pi/pi-coding-agent/mnemopi/embed-client";
@@ -51,16 +50,6 @@ describe("issue #3031 — mnemopi embeddings live in an isolated subprocess", ()
expect(exitCode).toBe(0);
}, 30_000);
it("CLI dispatches the flag that `embed-client.ts` passes to the spawned child", async () => {
// `mnemopiEmbedWorkerSpawnCmd()` and the cli switch must agree on the
// exact flag, character-for-character — the spawned `bun`/binary sees
// only `argv` and there is no fallback path that "re-routes" the
// worker on misnamed flags. Pin the spelling on both ends.
const cliSource = await Bun.file(new URL("../src/cli.ts", import.meta.url)).text();
expect(cliSource).toContain(`"${MNEMOPI_EMBED_WORKER_ARG}"`);
expect(cliSource).toContain("startMnemopiEmbedWorker");
});
it("surfaces unexpected signal exits so in-flight callers don't await forever", async () => {
// If the child dies from a signal we did NOT request — SIGSEGV from
// onnxruntime's NAPI fault (the original Windows shutdown bug, now
@@ -106,27 +95,6 @@ describe("issue #3031 — mnemopi embeddings live in an isolated subprocess", ()
expect(errored).toBe(false);
}, 10_000);
it("does not import fastembed-runtime from the main agent module graph", async () => {
// Issue #3031 caused: `mnemopi/state.ts` (or anything it transitively
// loaded) statically importing `core/fastembed-runtime`, which loads
// `onnxruntime-node` natively. Only the dedicated worker module is
// allowed to reach into that runtime. Scan the agent's mnemopi shim
// surface and the session entrypoint to lock the rule in.
const candidates = [
"../src/mnemopi/state.ts",
"../src/mnemopi/backend.ts",
"../src/mnemopi/embed-client.ts",
"../src/mnemopi/embed-protocol.ts",
"../src/session/agent-session.ts",
"../src/cli.ts",
];
for (const rel of candidates) {
const source = await Bun.file(new URL(rel, import.meta.url)).text();
expect(source).not.toContain("fastembed-runtime");
expect(source).not.toContain('"fastembed"');
expect(source).not.toContain('"onnxruntime-node"');
}
});
it("carries (model, cacheDir) on every embed so a respawned worker can self-init", async () => {
// Without this contract: after `shutdownMnemopiEmbedClient()` runs on
// session dispose, mnemopi still holds the cached `LocalEmbeddingModel`
@@ -1,67 +0,0 @@
import { describe, expect, it } from "bun:test";
import * as path from "node:path";
/**
* Regression for https://github.com/can1357/oh-my-pi/issues/3461
*
* Ctrl+Z stopped working after any tool call: the TUI tore down but the
* process kept running (`Sl+`, not `T`), wedging the terminal until
* `kill -9`. Root cause: brush-core's `Process::wait` calls
* `tokio::signal::unix::signal(SIGTSTP)` to detect when its children get
* stopped. Per tokio's documented contract the first call for a SignalKind
* permanently replaces the kernel-default handler — so after the first
* `Shell::run` the parent's SIGTSTP no longer triggers the kernel STOP
* action, and `process.kill(0, "SIGTSTP")` from the Ctrl+Z handler became
* a no-op.
*
* The fix has two halves:
*
* - `handleCtrlZ` sends SIGSTOP (uncatchable) to the foreground process
* group, so the kernel parks omp regardless of installed handlers and the
* parent shell sees the whole job stop even when omp runs behind a wrapper
* (`npx`, `pnpm exec`, `bunx`, …) or as one stage of a pipeline.
* - MCP stdio servers spawn detached, so terminal job-control signals cannot
* stop their process trees and leave the JSONL read loop blocked on silent
* pipes — and so the pgid=0 suspend above doesn't reach them either.
* The unit test in `input-controller-suspend.test.ts` covers the JS handler's
* call shape; this file pins the runtime contract on the brush/MCP side so
* refactors force a deliberate revisit instead of silently regressing behavior.
*/
describe("issue #3461 — Ctrl+Z hangs after a command has been run", () => {
const packageDir = path.resolve(import.meta.dir, "..");
const brushUnixSignal = path.resolve(packageDir, "../../crates/vendor/brush-core/src/sys/unix/signal.rs");
const brushProcesses = path.resolve(packageDir, "../../crates/vendor/brush-core/src/processes.rs");
const inputController = path.resolve(packageDir, "src/modes/controllers/input-controller.ts");
const mcpStdioTransport = path.resolve(packageDir, "src/mcp/transports/stdio.ts");
it("brush-core installs a tokio SIGTSTP listener on every Process::wait", async () => {
const signalSrc = await Bun.file(brushUnixSignal).text();
expect(signalSrc).toContain("tstp_signal_listener");
expect(signalSrc).toContain("tokio::signal::unix::signal");
// Pin the SIGTSTP constant specifically. A move to a non-job-control
// signal would invalidate the assumption this fix is built on.
expect(signalSrc).toMatch(/nix::libc::SIGTSTP/);
const processesSrc = await Bun.file(brushProcesses).text();
expect(processesSrc).toContain("tstp_signal_listener");
});
it("handleCtrlZ sends SIGSTOP to the foreground process group, defeating the brush hijack and covering wrappers/pipelines", async () => {
const src = await Bun.file(inputController).text();
// The original broken shape must not return.
expect(src).not.toMatch(/process\.kill\(\s*0\s*,\s*["']SIGTSTP["']\s*\)/);
// Self-only SIGSTOP was the v1 of this fix; it leaves wrapper/pipeline
// peers in the same process group running and the shell never sees the
// job stop. The handler now targets pgid=0.
expect(src).not.toMatch(/process\.kill\(\s*process\.pid\s*,\s*["']SIGSTOP["']\s*\)/);
// pgid=0 SIGSTOP is the only correct shape: uncatchable, and reaches
// every process the shell considers part of the foreground job.
expect(src).toMatch(/process\.kill\(\s*0\s*,\s*["']SIGSTOP["']\s*\)/);
});
it("MCP stdio servers spawn detached so terminal job-control signals cannot stop them", async () => {
const src = await Bun.file(mcpStdioTransport).text();
expect(src).toMatch(/detached:\s*true/);
expect(src).toContain("no controlling terminal");
});
});
@@ -1,54 +0,0 @@
import { describe, expect, it } from "bun:test";
import * as path from "node:path";
const sourceRoot = path.join(import.meta.dir, "..", "src");
describe("startup import graph", () => {
it("keeps normal startup off the aggregate modes barrel", async () => {
const mainSource = await Bun.file(path.join(sourceRoot, "main.ts")).text();
expect(mainSource).toContain('import { InteractiveMode } from "./modes/interactive-mode";');
expect(mainSource).not.toContain('from "./modes"');
});
it("keeps branch-only mode runners out of the modes barrel", async () => {
const modesBarrelSource = await Bun.file(path.join(sourceRoot, "modes/index.ts")).text();
expect(modesBarrelSource).toContain('from "./interactive-mode"');
expect(modesBarrelSource).not.toContain("runAcpMode");
expect(modesBarrelSource).not.toContain("runPrintMode");
expect(modesBarrelSource).not.toContain("runRpcMode");
expect(modesBarrelSource).not.toContain("./rpc/rpc-mode");
});
it("keeps marketplace implementation behind the lightweight auto-update starter", async () => {
const mainSource = await Bun.file(path.join(sourceRoot, "main.ts")).text();
const starterSource = await Bun.file(
path.join(sourceRoot, "extensibility/plugins/marketplace-auto-update.ts"),
).text();
expect(mainSource).toContain('from "./extensibility/plugins/marketplace-auto-update"');
expect(mainSource).not.toContain('from "./extensibility/plugins/marketplace"');
expect(starterSource).toContain('await import("./marketplace")');
});
it("keeps puppeteer-core/@puppeteer/browsers off the eager startup graph", async () => {
// The builtin tool registry (tools/index.ts) statically imports BrowserTool, which
// reaches attach.ts -> registry.ts -> launch.ts -> tab-supervisor.ts. A *value*
// import from puppeteer-core (e.g. the TargetType enum) executes puppeteer-core's
// barrel at boot, which re-exports the node launchers + BrowserConnector and pulls in
// @puppeteer/browsers (the Chromium downloader). A packaging quirk in that subtree then
// becomes a hard startup crash (e.g. `Cannot find module './browser-data/browser-data.js'`)
// instead of a recoverable ToolError on first browser use. These modules must therefore
// import puppeteer packages as `import type` only. tab-worker.ts is excluded: it runs
// solely inside the spawned browser worker, off the main startup graph.
const browserDir = path.join(sourceRoot, "tools", "browser");
const eagerFiles = ["attach.ts", "registry.ts", "launch.ts", "tab-supervisor.ts"];
const valueImportRe = /^import\s+(?!type\b)[^;]*?from\s+["'](?:puppeteer-core|@puppeteer\/browsers)["']/gm;
for (const file of eagerFiles) {
const src = await Bun.file(path.join(browserDir, file)).text();
const offenders = src.match(valueImportRe) ?? [];
expect(offenders, `${file} value-imports puppeteer; use \`import type\``).toEqual([]);
}
});
});
@@ -1,30 +0,0 @@
import { describe, expect, it } from "bun:test";
import * as path from "node:path";
describe("stats dashboard assets in distributed CLI builds", () => {
const repoRoot = path.resolve(import.meta.dir, "../../..");
const bundleScriptPath = path.join(repoRoot, "packages/coding-agent/scripts/bundle-dist.ts");
const cliPath = path.join(repoRoot, "packages/coding-agent/src/cli.ts");
const statsServerPath = path.join(repoRoot, "packages/stats/src/server.ts");
it("embeds the stats client archive while building the npm CLI bundle", async () => {
const bundleScript = await Bun.file(bundleScriptPath).text();
expect(bundleScript).toContain(`"scripts/generate-client-bundle.ts", "--generate"`);
expect(bundleScript).toContain(`"scripts/generate-client-bundle.ts", "--reset"`);
expect(bundleScript).toContain(`process.env.PI_BUNDLED="true"`);
});
it("uses embedded stats assets for prebuilt CLI distributions", async () => {
const statsServer = await Bun.file(statsServerPath).text();
expect(statsServer).toContain("process.env.PI_BUNDLED");
expect(statsServer).toContain("USE_EMBEDDED_CLIENT");
expect(statsServer).toContain("Embedded stats client bundle missing");
});
it("probes dashboard static assets in the install smoke test path", async () => {
const cliSource = await Bun.file(cliPath).text();
expect(cliSource).toContain("startServer(0)");
expect(cliSource).toContain("127.0.0.1");
expect(cliSource).toContain("dashboard HTML was not served");
});
});