From 7cf9a58de5bb7187484faf2f48fda929bb88400f Mon Sep 17 00:00:00 2001 From: Hatton Date: Thu, 3 Sep 2026 11:09:27 -0600 Subject: [PATCH] Make the bloom-automation scripts safe to ask for help `node killBloomProcess.mjs --help` killed the running Bloom. Unknown flags were ignored, so the destructive default ran, which made "read the usage first" the dangerous move. The sibling scripts shared the shape. Every script in .github/skills/bloom-automation now recognizes --help and -h, prints its usage, and exits without killing anything. An unknown option is rejected with the usage rather than ignored, and a --pid or --watch-pid that is not a positive integer is rejected instead of being passed to the kill code, where a NaN would have meant "every Bloom this worktree owns". A help request is answered wherever it sits on the command line, which took two more changes. `requireOptionValue` rejects any value starting with "-", because `killBloomProcess.mjs --repo-root -h` otherwise stored "-h" as the repository path, named no target, and reached that destructive default; every value these scripts take is a path, a TCP port or a process id, so nothing legitimate is lost. And `asksForHelp(args)` runs before each parse loop can consume the request as some option's value. `switchWorkspaceTab.mjs`, `dismissProblemDialog.mjs`, `webview2Targets.mjs` and `driveAiImageEditor.mjs` needed the help and the unknown-option check as well: two had no help at all, and two took `--help` only. So the claim above holds for all eight scripts. Retires the AUTOMATION-DEBT.md entry "Automation helper scripts run destructive defaults on unknown flags". Folding these helpers into the launch fixture, the other half of that entry's fix direction, is not part of this. Verified by running each script with `--nonsense`, with `-h`, and with `-h` in the second position, and by checking that a real option still parses. Co-Authored-By: Claude Opus 5 (1M context) --- .../bloom-automation/bloomProcessCommon.mjs | 33 ++++++++- .../bloom-automation/bloomProcessStatus.mjs | 32 ++++++++- .../bloom-automation/dismissProblemDialog.mjs | 20 ++++-- .../bloom-automation/driveAiImageEditor.mjs | 35 ++++++++- .../bloom-automation/killBloomProcess.mjs | 71 +++++++++++++++++-- .../bloom-automation/launcherControl.mjs | 25 +++++++ .../bloom-automation/switchWorkspaceTab.mjs | 22 ++++-- .../bloom-automation/webview2Targets.mjs | 27 +++++++ src/BloomE2E/AUTOMATION-DEBT.md | 13 ---- 9 files changed, 242 insertions(+), 36 deletions(-) diff --git a/.github/skills/bloom-automation/bloomProcessCommon.mjs b/.github/skills/bloom-automation/bloomProcessCommon.mjs index 21d2784acc66..f0fde3c2e00d 100644 --- a/.github/skills/bloom-automation/bloomProcessCommon.mjs +++ b/.github/skills/bloom-automation/bloomProcessCommon.mjs @@ -42,9 +42,40 @@ export const requireTcpPortOption = (optionName, value) => { return port; }; +/** + * A process id given on the command line, as a positive integer. + * + * This throws rather than returning undefined, because the caller that wants a process id wants + * to act on exactly that process. A killer script that read a malformed id as "no id given" would + * fall through to whatever its no-target default is, and killBloomProcess.mjs's default is to kill + * every Bloom the worktree owns. + */ +export const requireProcessIdOption = (optionName, value) => { + const normalized = value === undefined ? "" : String(value).trim(); + const processId = /^\d+$/.test(normalized) + ? toPositiveInteger(normalized) + : undefined; + if (!processId) { + throw new Error( + `${optionName} must be a positive integer process id. Received: ${value}`, + ); + } + + return processId; +}; + +// Whether the caller asked for the usage, wherever the request sits on the command line. Every +// script checks this before it parses anything else: `--repo-root -h` used to store "-h" as +// the path, name no target, and reach the default that kills every Bloom on the machine. +export const asksForHelp = (args) => + args.some((arg) => arg === "--help" || arg === "-h"); + export const requireOptionValue = (args, index, optionName) => { const value = args[index + 1]; - if (!value || value.startsWith("--")) { + // A leading "-" of any length means the next flag, not this option's value. Every value + // these scripts take is a path, a TCP port or a process id, and none of those starts + // with "-", so there is nothing legitimate to reject here. + if (!value || value.startsWith("-")) { throw new Error(`${optionName} requires a value.`); } diff --git a/.github/skills/bloom-automation/bloomProcessStatus.mjs b/.github/skills/bloom-automation/bloomProcessStatus.mjs index cc3df6e285e3..0a072df964ba 100644 --- a/.github/skills/bloom-automation/bloomProcessStatus.mjs +++ b/.github/skills/bloom-automation/bloomProcessStatus.mjs @@ -1,4 +1,5 @@ import { + asksForHelp, buildProcessChain, classifyProcesses, fetchBloomInstanceInfo, @@ -13,8 +14,24 @@ import { toWorkspaceTabsEndpoint, } from "./bloomProcessCommon.mjs"; +const usage = `Report the Bloom and launcher processes this machine is running. + + node bloomProcessStatus.mjs [options] + + --help, -h Print this and exit. + --json Report as JSON. + --running-bloom Also probe each running Bloom's own HTTP server. + --repo-root The worktree to judge instances against (default: this checkout). + --http-port Report the instance whose server answers on this port. + +This script only reads; it never kills anything.`; + const parseArgs = () => { const args = process.argv.slice(2); + if (asksForHelp(args)) { + console.log(usage); + process.exit(0); + } const options = { json: false, runningBloom: false, @@ -25,6 +42,11 @@ const parseArgs = () => { for (let i = 0; i < args.length; i++) { const arg = args[i]; + if (arg === "--help" || arg === "-h") { + console.log(usage); + process.exit(0); + } + if (arg === "--json") { options.json = true; continue; @@ -36,7 +58,11 @@ const parseArgs = () => { } if (arg === "--repo-root") { - options.repoRoot = args[i + 1] || options.repoRoot; + // A required value, checked the same way as every other option's. Taking + // `args[i + 1]` and falling back to the default would swallow the next flag as + // this option's value. killBloomProcess.mjs has the same parser, where that + // mistake is destructive. + options.repoRoot = requireOptionValue(args, i, "--repo-root"); i++; continue; } @@ -57,6 +83,10 @@ const parseArgs = () => { ); continue; } + + // An unknown flag is a mistake, and silently ignoring it hides it. + console.error(`Unknown option ${arg}.\n\n${usage}`); + process.exit(2); } return options; diff --git a/.github/skills/bloom-automation/dismissProblemDialog.mjs b/.github/skills/bloom-automation/dismissProblemDialog.mjs index 4e37b73febc0..fbb46ff52dc9 100644 --- a/.github/skills/bloom-automation/dismissProblemDialog.mjs +++ b/.github/skills/bloom-automation/dismissProblemDialog.mjs @@ -23,6 +23,7 @@ import { createRequire } from "node:module"; import path from "node:path"; import { + asksForHelp, fetchBloomInstanceInfo, getDefaultRepoRoot, normalizeBloomInstanceInfo, @@ -32,8 +33,15 @@ import { toLocalOrigin, } from "./bloomProcessCommon.mjs"; +const usage = + "Usage: node .github/skills/bloom-automation/dismissProblemDialog.mjs --http-port [--wait] [--timeout-ms ] [--json]"; + const parseArgs = () => { const args = process.argv.slice(2); + if (asksForHelp(args)) { + console.log(usage); + process.exit(0); + } const options = { httpPort: undefined, wait: false, @@ -79,12 +87,12 @@ const parseArgs = () => { continue; } - if (arg === "--help") { - console.log( - "Usage: node .github/skills/bloom-automation/dismissProblemDialog.mjs --http-port [--wait] [--timeout-ms ] [--json]", - ); - process.exit(0); - } + // A typo must not be ignored: an option this script does not know is a request it + // cannot carry out, so say so rather than do something else. + console.error(`Unknown option ${arg}. + +${usage}`); + process.exit(2); } if (!options.httpPort) { diff --git a/.github/skills/bloom-automation/driveAiImageEditor.mjs b/.github/skills/bloom-automation/driveAiImageEditor.mjs index 4743bd4de0e6..a9695c4d4cba 100644 --- a/.github/skills/bloom-automation/driveAiImageEditor.mjs +++ b/.github/skills/bloom-automation/driveAiImageEditor.mjs @@ -35,6 +35,29 @@ const { chromium } = createRequire(path.join(componentTester, "package.json"))( ); const args = process.argv.slice(2); + +const usage = `Drive the "Edit with AI…" image editor of a running Bloom over CDP. + + node driveAiImageEditor.mjs [options] [command] + + frames List every frame of the Edit tab (the default command). + images Report the images of the current page. + credits Report each book image's credits, read from the file metadata. + dummy-edit Open the editor, edit with the Local Dummy model, and commit. + + --help, -h Print this and exit. + --http-port The Bloom whose server answers on this port (default: 8092). + --cdp-port The debugging port to attach to (default: --http-port plus 2). + --match Part of the src of the image to edit (default: ai-image). + --shot Where to write the screenshot of a dummy-edit run.`; + +// The usage counts wherever the request sits, and this script attaches to a running Bloom, so +// answer it before anything reaches that Bloom. +if (args.some((arg) => arg === "--help" || arg === "-h")) { + console.log(usage); + process.exit(0); +} + const opt = (name, def) => { const i = args.indexOf(name); return i >= 0 && args[i + 1] ? args[i + 1] : def; @@ -44,8 +67,16 @@ const valueFlags = new Set(["--http-port", "--cdp-port", "--match", "--shot"]); const positional = []; for (let i = 0; i < args.length; i++) { if (args[i].startsWith("--")) { - if (valueFlags.has(args[i])) i++; // skip its value - continue; + if (valueFlags.has(args[i])) { + i++; // skip its value + continue; + } + // A typo must not be ignored: this script attaches to a running Bloom and does things + // to the book being edited, so an option it does not know stops the run. + console.error(`Unknown option ${args[i]}. + +${usage}`); + process.exit(2); } positional.push(args[i]); } diff --git a/.github/skills/bloom-automation/killBloomProcess.mjs b/.github/skills/bloom-automation/killBloomProcess.mjs index 7c97dde452bc..c48b40a9897a 100644 --- a/.github/skills/bloom-automation/killBloomProcess.mjs +++ b/.github/skills/bloom-automation/killBloomProcess.mjs @@ -6,12 +6,42 @@ import { getWindowsProcessSnapshot, killProcessIds, normalizeBloomInstanceInfo, + asksForHelp, requireOptionValue, + requireProcessIdOption, requireTcpPortOption, } from "./bloomProcessCommon.mjs"; +const usage = `Kill the Bloom.exe (and dotnet.exe BloomExe.csproj) processes of this worktree. + + node killBloomProcess.mjs [options] + + --help, -h Print this and exit without killing anything. + --json Report what was killed as JSON. + --only-mismatched Kill only instances whose repo root is not this worktree. + --repo-root The worktree to judge instances against (default: this checkout). + --http-port Kill the instance whose server answers on this port. + --pid Kill this process and the Bloom processes in its chain. + --watch-pid Kill this launcher/watch process and its Bloom processes. + +With no --http-port, --pid or --watch-pid, this kills EVERY Bloom this worktree owns.`; + +// Print the usage and exit 0 without killing anything, or reject an unknown flag with a non-zero +// exit. Reading the usage first must never be the dangerous move: this script used to ignore +// --help and go straight to its destructive default (AUTOMATION-DEBT.md, "Automation helper +// scripts run destructive defaults on unknown flags"). +const exitWithUsage = (unknownArgument) => { + if (unknownArgument) { + console.error(`Unknown option ${unknownArgument}.\n\n${usage}`); + process.exit(2); + } + console.log(usage); + process.exit(0); +}; + const parseArgs = () => { const args = process.argv.slice(2); + if (asksForHelp(args)) exitWithUsage(); const options = { json: false, onlyMismatched: false, @@ -24,6 +54,10 @@ const parseArgs = () => { for (let i = 0; i < args.length; i++) { const arg = args[i]; + if (arg === "--help" || arg === "-h") { + exitWithUsage(); + } + if (arg === "--json") { options.json = true; continue; @@ -35,7 +69,11 @@ const parseArgs = () => { } if (arg === "--repo-root") { - options.repoRoot = args[i + 1] || options.repoRoot; + // A required value, checked the same way as every other option's. Taking + // `args[i + 1]` and falling back to the default would swallow the NEXT FLAG as + // this option's value, so `--repo-root --pid 123` would name no target at all and + // reach the default that kills every Bloom this worktree owns. + options.repoRoot = requireOptionValue(args, i, "--repo-root"); i++; continue; } @@ -58,25 +96,40 @@ const parseArgs = () => { } if (arg === "--pid") { - options.pid = Number(args[i + 1]); + options.pid = requireProcessIdOption( + "--pid", + requireOptionValue(args, i, "--pid"), + ); i++; continue; } if (arg.startsWith("--pid=")) { - options.pid = Number(arg.slice("--pid=".length)); + options.pid = requireProcessIdOption( + "--pid", + arg.slice("--pid=".length), + ); continue; } if (arg === "--watch-pid") { - options.watchPid = Number(args[i + 1]); + options.watchPid = requireProcessIdOption( + "--watch-pid", + requireOptionValue(args, i, "--watch-pid"), + ); i++; continue; } if (arg.startsWith("--watch-pid=")) { - options.watchPid = Number(arg.slice("--watch-pid=".length)); + options.watchPid = requireProcessIdOption( + "--watch-pid", + arg.slice("--watch-pid=".length), + ); + continue; } + + exitWithUsage(arg); } return options; @@ -85,8 +138,14 @@ const parseArgs = () => { const options = parseArgs(); const processState = classifyProcesses(options.repoRoot); const processIds = new Set(); +// Whether the caller named a target, not whether the value we parsed from it is usable. The +// two are the same now that every target option is validated at parse time, and this says the +// intended thing: a caller who asked for one process must never reach the default that kills +// every Bloom this worktree owns. const exactTargetRequested = - !!options.httpPort || !!options.pid || !!options.watchPid; + options.httpPort !== undefined || + options.pid !== undefined || + options.watchPid !== undefined; let targetedInstance; let exactTargetResolutionError; diff --git a/.github/skills/bloom-automation/launcherControl.mjs b/.github/skills/bloom-automation/launcherControl.mjs index c614f481079a..a2d17ac9baee 100644 --- a/.github/skills/bloom-automation/launcherControl.mjs +++ b/.github/skills/bloom-automation/launcherControl.mjs @@ -26,6 +26,7 @@ import { } from "node:fs"; import path from "node:path"; import { + asksForHelp, getDefaultRepoRoot, requireOptionValue, } from "./bloomProcessCommon.mjs"; @@ -43,8 +44,25 @@ const actionNames = [ "--ensure-running", ]; +const usage = `Command the go.sh launcher for this worktree. + + node launcherControl.mjs [options] + + actions: ${actionNames.join(", ")} + options: --json, --wait-ready, --repo-root , --timeout-ms + --help, -h Print this and exit without commanding anything. + + --restart rebuilds and relaunches; --quit-bloom stops Bloom and leaves the launcher; + --shutdown stops Bloom, the launcher, and Vite.`; + const parseArgs = () => { const args = process.argv.slice(2); + // Asking a destructive script how to use it must never be the destructive move, and the + // request counts wherever it sits: see asksForHelp. + if (asksForHelp(args)) { + console.log(usage); + process.exit(0); + } const options = { action: undefined, json: false, @@ -56,6 +74,13 @@ const parseArgs = () => { for (let i = 0; i < args.length; i++) { const arg = args[i]; + // Print the usage and stop, before any action can run. Asking a destructive script how to + // use it must never be the destructive move. + if (arg === "--help" || arg === "-h") { + console.log(usage); + process.exit(0); + } + if (actionNames.includes(arg)) { if (options.action) { throw new Error( diff --git a/.github/skills/bloom-automation/switchWorkspaceTab.mjs b/.github/skills/bloom-automation/switchWorkspaceTab.mjs index 5ef4a7d92b94..0e8c97244bc1 100644 --- a/.github/skills/bloom-automation/switchWorkspaceTab.mjs +++ b/.github/skills/bloom-automation/switchWorkspaceTab.mjs @@ -1,6 +1,7 @@ import { createRequire } from "node:module"; import path from "node:path"; import { + asksForHelp, fetchBloomInstanceInfo, findRunningStandardBloomInstance, getDefaultRepoRoot, @@ -11,8 +12,15 @@ import { toWorkspaceTabsEndpoint, } from "./bloomProcessCommon.mjs"; +const usage = + "Usage: node .github/skills/bloom-automation/switchWorkspaceTab.mjs (--running-bloom | --http-port ) --tab [--json] [--timeout-ms ]"; + const parseArgs = () => { const args = process.argv.slice(2); + if (asksForHelp(args)) { + printHelp(); + process.exit(0); + } const options = { runningBloom: false, httpPort: undefined, @@ -68,19 +76,19 @@ const parseArgs = () => { continue; } - if (arg === "--help") { - printHelp(); - process.exit(0); - } + // A typo must not be ignored: an option this script does not know is a request it + // cannot carry out, so say so rather than do something else. + console.error(`Unknown option ${arg}. + +${usage}`); + process.exit(2); } return options; }; const printHelp = () => { - console.log( - "Usage: node .github/skills/bloom-automation/switchWorkspaceTab.mjs (--running-bloom | --http-port ) --tab [--json] [--timeout-ms ]", - ); + console.log(usage); }; const normalizeTab = (tab) => { diff --git a/.github/skills/bloom-automation/webview2Targets.mjs b/.github/skills/bloom-automation/webview2Targets.mjs index ee6b62fbe3fe..40a8acc49bd9 100644 --- a/.github/skills/bloom-automation/webview2Targets.mjs +++ b/.github/skills/bloom-automation/webview2Targets.mjs @@ -1,4 +1,5 @@ import { + asksForHelp, fetchBloomInstanceInfo, findRunningStandardBloomInstance, normalizeBloomInstanceInfo, @@ -7,8 +8,26 @@ import { toLocalOrigin, } from "./bloomProcessCommon.mjs"; +const usage = `List the WebView2 debugging targets of a running Bloom. + + node webview2Targets.mjs [options] + + --help, -h Print this and exit. + --json Report the targets as JSON. + --all List every target, not only Bloom's own documents. + --running-bloom Find the Bloom that is running, rather than naming a port. + --http-port The Bloom whose server answers on this port. + --host The host to ask for targets (default: localhost). + --port The CDP port to ask directly. + --wait Wait for a target to appear. + --timeout-ms How long to wait (default: 15000).`; + const parseArgs = () => { const args = process.argv.slice(2); + if (asksForHelp(args)) { + console.log(usage); + process.exit(0); + } const options = { host: "localhost", port: undefined, @@ -85,7 +104,15 @@ const parseArgs = () => { if (arg === "--timeout-ms") { options.timeoutMs = Number(args[i + 1] || options.timeoutMs); i++; + continue; } + + // A typo must not be ignored: an option this script does not know is a request it + // cannot carry out, so say so rather than do something else. + console.error(`Unknown option ${arg}. + +${usage}`); + process.exit(2); } return options; diff --git a/src/BloomE2E/AUTOMATION-DEBT.md b/src/BloomE2E/AUTOMATION-DEBT.md index e9075221197a..21f533106693 100644 --- a/src/BloomE2E/AUTOMATION-DEBT.md +++ b/src/BloomE2E/AUTOMATION-DEBT.md @@ -21,7 +21,6 @@ the identity here. Before you start on a marked entry, ask the owner of its bran | Branch | What it pays down | | --- | --- | -| `BL-16799-automation-scripts` | The `bloom-automation` scripts answer `--help` without killing anything, and reject an unknown flag or a malformed process id. | | `BL-16799-vr-collect-failures` | A visual-regression case collects every failed comparison and fails once at the end. | | `BL-16799-component-tests-in-ci` | The component-tester Playwright suites get a nightly job. | | `BL-16799-vite-port` | `BLOOM_E2E_VITE_PORT` makes a run test the working tree's front end. Adds a new entry for what remains. | @@ -217,18 +216,6 @@ authoring painful. Fix direction: a small per-comparison pixel tolerance, or machine-profile baselines, or render fonts only from Bloom's own WOFF2 set in --e2e mode. (Found 2026-09-01 while verifying the bloom-testing-inputs rewire.) -## Automation helper scripts run destructive defaults on unknown flags - -`node .github/skills/bloom-automation/killBloomProcess.mjs --help` killed the running -Bloom: unknown flags are ignored and the destructive default runs, so "read the usage -first" is itself the dangerous move; sibling scripts may share the shape. Fix -direction: recognize `--help`/`-h` and reject unknown flags in every script that kills -processes — and fold these helpers' jobs into the library's audited launch/teardown -fixture over time. (Promoted from PAPERCUTS 2026-07-24.) - -being fixed on `BL-16799-automation-scripts`, for the `--help`, unknown-flag and -malformed-process-id part. Folding the helpers into the fixture is not part of it. - ## Adding a page needs the Add Page dialog, which offers nothing to automate against A book made from a template starts with front and back matter only, because every page