From cef5573fba68aad4f7691ceddf3b6aec9a674a76 Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Sun, 30 Aug 2026 14:30:50 +0000 Subject: [PATCH] feat(fleet): save a command with its settings, and manage them from the app MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps, both real. A saved command stored the script, the interpreter, sudo, the working directory and the timeout — but not **how many servers at a time** or **whether a failure stops the rest**. Those are the two settings that differ most between commands: "reload nginx" and "upgrade the database tier" want opposite answers, and re-choosing them on every run is how a saved command still gets run wrong. They are part of the command now, stored and restored, with a flag still winning at the point of use. And the desktop could not save a command **at all**. The recipe strip was read-only; only the CLI could write one. Editing anything now offers "Save these settings", and a saved command carries an x to delete it. A shipped recipe has no x — it is copied, not edited, so an upgrade of DiskPush never silently changes a command someone relies on, and the main process refuses a delete aimed at one rather than trusting the window to have hidden the button. Migration 004 adds the two columns with the schema's own defaults, so every existing row reads back exactly as it behaved before. Caught by the type checker on the way through: `copyRecipe` did not carry the new fields, so `fleet commands copy` would have produced a copy that ran the same script at a different speed — the one thing a copy must not do. Caught by running it: the built-in recipes are plain object literals typed as FleetCommand, and nothing parses them through the schema, so a field the schema merely defaults came back `undefined`. That reached the desktop as `setConcurrency(undefined)`. They state every field now, with a test asserting it, because the next field added will have the same problem. Verified end to end against a real store: saved with `--concurrency 2 --stop-on-error --timeout 45`, read back intact, honoured on a run with no flags, overridden by a flag, and a pre-existing row migrating to the defaults. The desktop side was screenshotted: picking a saved command restores sudo, "At once 2", "Timeout 45" and "Stop after a failure" together, not just the text. 508 tests. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UeSWg1Czsb2Lwxj8vHUnA4 --- apps/cli/src/commands/fleet.ts | 14 ++- apps/desktop/electron/main/ipc.ts | 9 ++ apps/desktop/electron/main/services/fleet.ts | 30 +++++ apps/desktop/electron/preload/index.ts | 2 + apps/desktop/electron/shared/contract.ts | 25 ++++ apps/desktop/src/components/fleet-view.tsx | 116 +++++++++++++++++-- apps/desktop/src/lib/api.ts | 18 +++ docs/desktop.md | 7 ++ docs/fleet.md | 24 +++- packages/database/src/migrations.ts | 16 +++ packages/database/src/store.ts | 9 +- packages/fleet-core/src/recipes.test.ts | 17 +++ packages/fleet-core/src/recipes.ts | 14 ++- packages/schemas/src/fleet.ts | 7 ++ 14 files changed, 293 insertions(+), 15 deletions(-) diff --git a/apps/cli/src/commands/fleet.ts b/apps/cli/src/commands/fleet.ts index c290b49..830881e 100644 --- a/apps/cli/src/commands/fleet.ts +++ b/apps/cli/src/commands/fleet.ts @@ -296,6 +296,9 @@ type FleetInvocation = { timeoutSeconds: number commandId: string | null targetFallback: readonly string[] + /** From a saved command when it came from one; a flag always wins. */ + concurrency?: number + onFailure?: 'continue' | 'stop' /** * The caller already asked about this script's hazards in terms specific to * what it does. `fleet upgrade --reboot` names the servers it will restart, @@ -339,8 +342,8 @@ async function execute( output: Output, ): Promise { const targets = await resolveTargets(parsed, store, invocation.targetFallback) - const concurrency = numberFlag(parsed, '--concurrency') ?? FLEET_DEFAULT_CONCURRENCY - const onFailure = hasFlag(parsed, '--stop-on-error') ? 'stop' : 'continue' + const concurrency = numberFlag(parsed, '--concurrency') ?? invocation.concurrency ?? FLEET_DEFAULT_CONCURRENCY + const onFailure = hasFlag(parsed, '--stop-on-error') ? 'stop' : (invocation.onFailure ?? 'continue') const env = envFromFlags(parsed) const assumeYes = hasFlag(parsed, '--yes') @@ -550,6 +553,8 @@ async function fleetRun(parsed: ParsedArgv, store: DiskPushStore, output: Output sudo: command.sudo || hasFlag(parsed, '--sudo'), workingDirectory: flagValue(parsed, '--cwd') ?? command.workingDirectory, timeoutSeconds: numberFlag(parsed, '--timeout') ?? command.timeoutSeconds, + concurrency: numberFlag(parsed, '--concurrency') ?? command.concurrency, + onFailure: hasFlag(parsed, '--stop-on-error') ? 'stop' : command.onFailure, commandId: command.builtin ? null : command.id, targetFallback: command.targets, }, @@ -841,6 +846,11 @@ async function fleetCommands(parsed: ParsedArgv, store: DiskPushStore, output: O sudo: hasFlag(parsed, '--sudo'), workingDirectory: flags.workingDirectory, timeoutSeconds: flags.timeoutSeconds, + // Pacing is part of the command, not of the invocation: a saved command + // that forgets it was meant to run two at a time is a saved command that + // still gets run wrong. + concurrency: numberFlag(parsed, '--concurrency') ?? FLEET_DEFAULT_CONCURRENCY, + onFailure: hasFlag(parsed, '--stop-on-error') ? 'stop' : 'continue', targets: flagValues(parsed, '--on'), tags: flagValues(parsed, '--tag'), }) diff --git a/apps/desktop/electron/main/ipc.ts b/apps/desktop/electron/main/ipc.ts index 71b7530..373f84f 100644 --- a/apps/desktop/electron/main/ipc.ts +++ b/apps/desktop/electron/main/ipc.ts @@ -10,6 +10,7 @@ import { DeleteEntryRequestSchema, ExternalUrlSchema, FleetCheckRequestSchema, + FleetCommandSaveSchema, FleetListRenameSchema, FleetListSaveSchema, FleetRequestSchema, @@ -29,6 +30,8 @@ import { fleetCommands, fleetLists, fleetRunDetail, + removeFleetCommand, + saveFleetCommand, fleetServers, removeFleetList, renameFleetList, @@ -331,6 +334,12 @@ export function registerIpc(): void { handle(IPC.fleetRunDetail, z.object({ runId: FleetRunIdSchema }), async ({ runId }) => fleetRunDetail(runId)) + handle(IPC.fleetCommandSave, FleetCommandSaveSchema, async (input) => saveFleetCommand(input)) + + handle(IPC.fleetCommandRemove, z.object({ name: z.string().min(1).max(128) }), async ({ name }) => + removeFleetCommand(name), + ) + handle(IPC.fleetLists, z.undefined(), async () => fleetLists()) handle(IPC.fleetListSave, FleetListSaveSchema, async (input) => saveFleetList(input)) diff --git a/apps/desktop/electron/main/services/fleet.ts b/apps/desktop/electron/main/services/fleet.ts index b3045a2..6f006c2 100644 --- a/apps/desktop/electron/main/services/fleet.ts +++ b/apps/desktop/electron/main/services/fleet.ts @@ -173,6 +173,36 @@ export async function startFleet(request: FleetRequest, sender: WebContents): Pr } } +// --- saved commands --------------------------------------------------------- + +/** + * Saves a command from the Fleet view. + * + * `builtin` is not settable here: the store forces it false, so a saved + * command can shadow a shipped recipe by name but can never claim to be one. + */ +export async function saveFleetCommand(input: { + name: string + description: string + script: string + interpreter: 'sh' | 'bash' | 'raw' + sudo: boolean + workingDirectory: string | null + timeoutSeconds: number + concurrency: number + onFailure: 'continue' | 'stop' + targets: string[] +}): Promise { + return (await store()).saveFleetCommand({ ...input, tags: [] }) +} + +export async function removeFleetCommand(name: string): Promise { + if (BUILTIN_RECIPES.some((recipe) => recipe.name === name)) { + throw new Error(`${name} is a recipe DiskPush ships and cannot be deleted. Save a copy under another name instead.`) + } + return (await store()).deleteFleetCommand(name) +} + // --- saved lists ------------------------------------------------------------ export async function fleetLists(): Promise { diff --git a/apps/desktop/electron/preload/index.ts b/apps/desktop/electron/preload/index.ts index fcfae96..70e3b4c 100644 --- a/apps/desktop/electron/preload/index.ts +++ b/apps/desktop/electron/preload/index.ts @@ -58,6 +58,8 @@ const api = { check: (connectionIds: string[], concurrency = 4, timeoutSeconds = 180) => call(IPC.fleetCheck, { connectionIds, concurrency, timeoutSeconds }), runs: (limit = 25) => call(IPC.fleetRuns, { limit }), + saveCommand: (input: unknown) => call(IPC.fleetCommandSave, input), + removeCommand: (name: string) => call(IPC.fleetCommandRemove, { name }), lists: () => call(IPC.fleetLists), saveList: (name: string, connectionIds: string[], description = '') => call(IPC.fleetListSave, { name, connectionIds, description }), diff --git a/apps/desktop/electron/shared/contract.ts b/apps/desktop/electron/shared/contract.ts index 9ae8706..afae0c9 100644 --- a/apps/desktop/electron/shared/contract.ts +++ b/apps/desktop/electron/shared/contract.ts @@ -45,6 +45,8 @@ export const IPC = { fleetCancel: 'fleet:cancel', fleetCheck: 'fleet:check', fleetRuns: 'fleet:runs', + fleetCommandSave: 'fleet:command-save', + fleetCommandRemove: 'fleet:command-remove', fleetLists: 'fleet:lists', fleetListSave: 'fleet:list-save', fleetListRename: 'fleet:list-rename', @@ -244,6 +246,29 @@ export const FleetListNameSchema = z.string().min(1).max(128).refine((name) => n message: 'A name cannot begin or end with a space.', }) +/** + * Saving a command from the Fleet view. + * + * The same shape the run request takes, minus the servers and the password: + * a saved command is the *settings*, and which servers to point them at is a + * separate choice made at run time (or remembered as `targets`). + */ +export const FleetCommandSaveSchema = z.object({ + name: z.string().min(1).max(128).refine((value) => value.trim() === value, { + message: 'A name cannot begin or end with a space.', + }), + description: z.string().max(500).default(''), + script: z.string().min(1).max(256 * 1024), + interpreter: z.enum(['sh', 'bash', 'raw']).default('sh'), + sudo: z.boolean().default(false), + workingDirectory: PathSchema.nullable().default(null), + timeoutSeconds: z.number().int().min(1).max(86400).default(900), + concurrency: z.number().int().min(1).max(64).default(4), + onFailure: z.enum(['continue', 'stop']).default('continue'), + /** Remembered so a saved command can carry the servers it is usually for. */ + targets: z.array(z.string().min(1).max(128)).max(500).default([]), +}) + export const FleetListSaveSchema = z.object({ name: FleetListNameSchema, description: z.string().max(500).default(''), diff --git a/apps/desktop/src/components/fleet-view.tsx b/apps/desktop/src/components/fleet-view.tsx index 9a0c986..c32a4a4 100644 --- a/apps/desktop/src/components/fleet-view.tsx +++ b/apps/desktop/src/components/fleet-view.tsx @@ -71,6 +71,8 @@ export function FleetView({ onAddServer }: { onAddServer: () => void }) { const [lists, setLists] = useState([]) const [savingList, setSavingList] = useState(false) const [newListName, setNewListName] = useState('') + const [savingCommand, setSavingCommand] = useState(false) + const [newCommandName, setNewCommandName] = useState('') const [selected, setSelected] = useState>(new Set()) const [tagFilter, setTagFilter] = useState(null) @@ -221,9 +223,51 @@ export function FleetView({ onAddServer }: { onAddServer: () => void }) { setInterpreter(command.interpreter) setSudo(command.sudo) setTimeoutSeconds(command.timeoutSeconds) + // The pacing is part of the command: "reload nginx" and "upgrade the + // database tier" want very different answers, and leaving whatever was + // last on screen is how a saved command still gets run wrong. + setConcurrency(command.concurrency) + setStopOnError(command.onFailure === 'stop') setHazards([]) } + /** Saves everything on screen except the servers, which are chosen per run. */ + const saveCommand = useCallback(async () => { + const name = newCommandName.trim() + if (!name || !script.trim()) return + setError(null) + try { + await unwrap( + api()?.fleet.saveCommand({ + name, + script, + interpreter, + sudo, + workingDirectory: null, + timeoutSeconds, + concurrency, + onFailure: stopOnError ? 'stop' : 'continue', + }), + ) + setNewCommandName('') + setSavingCommand(false) + setLabel(name) + setCommands(await unwrap(api()?.fleet.commands())) + } catch (caught) { + setError(caught instanceof Error ? caught.message : String(caught)) + } + }, [newCommandName, script, interpreter, sudo, timeoutSeconds, concurrency, stopOnError]) + + const removeCommand = useCallback(async (name: string) => { + setError(null) + try { + await unwrap(api()?.fleet.removeCommand(name)) + setCommands(await unwrap(api()?.fleet.commands())) + } catch (caught) { + setError(caught instanceof Error ? caught.message : String(caught)) + } + }, []) + const requestBody = useCallback( (hazardsConfirmed: boolean) => ({ connectionIds: [...selected], @@ -457,19 +501,77 @@ export function FleetView({ onAddServer }: { onAddServer: () => void }) {
- Recipes + Commands {commands.map((command) => ( - + {/* + Only what someone saved can be deleted. A shipped recipe is + copied, not edited, so upgrading DiskPush never silently + changes a command anyone relies on. + */} + {command.builtin ? null : ( + + )} + + ))} + + {script.trim() && !savingCommand ? ( + - ))} + ) : null} + + {savingCommand ? ( + + setNewCommandName(event.target.value)} + onKeyDown={(event) => { + if (event.key === 'Enter') void saveCommand() + if (event.key === 'Escape') { + setSavingCommand(false) + setNewCommandName('') + } + }} + placeholder="command name" + className="h-6 w-[150px] text-[11.5px]" + /> + + + ) : null}
{/* diff --git a/apps/desktop/src/lib/api.ts b/apps/desktop/src/lib/api.ts index c3b2092..c0740cd 100644 --- a/apps/desktop/src/lib/api.ts +++ b/apps/desktop/src/lib/api.ts @@ -104,11 +104,27 @@ export type FleetCommand = { sudo: boolean workingDirectory: string | null timeoutSeconds: number + concurrency: number + onFailure: 'continue' | 'stop' targets: string[] tags: string[] builtin: boolean } +/** Everything a saved command remembers. The servers are chosen at run time. */ +export type FleetCommandSave = { + name: string + description?: string + script: string + interpreter: 'sh' | 'bash' | 'raw' + sudo: boolean + workingDirectory: string | null + timeoutSeconds: number + concurrency: number + onFailure: 'continue' | 'stop' + targets?: string[] +} + export type Hazard = { kind: string; explanation: string; line: string; lineNumber: number } export type FleetListMember = { connectionId: string; connectionName: string } @@ -209,6 +225,8 @@ type Api = { check(connectionIds: string[], concurrency?: number, timeoutSeconds?: number): Promise> runs(limit?: number): Promise> runDetail(runId: string): Promise> + saveCommand(input: FleetCommandSave): Promise> + removeCommand(name: string): Promise> lists(): Promise> saveList(name: string, connectionIds: string[], description?: string): Promise> renameList(from: string, to: string): Promise> diff --git a/docs/desktop.md b/docs/desktop.md index dd9920c..daa845c 100644 --- a/docs/desktop.md +++ b/docs/desktop.md @@ -95,6 +95,13 @@ script editor, and the results — and **the action bar is pinned outside all of them**. Run is on screen at the 960×600 minimum window size just as it is maximised. +The command strip above the editor holds the shipped recipes and anything you +have saved. Editing the script or the settings offers **Save these settings**, +which stores the script, the interpreter, sudo, the timeout, how many servers +at a time, and whether a failure stops the rest — picking it again restores all +of it. A saved command carries an × to delete it; a shipped recipe does not, +because it is copied rather than edited. + Saved **lists** sit at the top of the sidebar, above the tag chips. Clicking one ticks exactly its members; ticking servers by hand offers **Save these 3**; the × on a chip deletes the list and leaves the servers alone. A member whose diff --git a/docs/fleet.md b/docs/fleet.md index 5f94b2c..3109e72 100644 --- a/docs/fleet.md +++ b/docs/fleet.md @@ -173,9 +173,27 @@ diskpush fleet commands copy upgrade my-upgrade # built-ins are copied, not e diskpush fleet run --command reload-nginx # --on comes from the saved default ``` -A built-in cannot be edited or deleted, only copied, so upgrading DiskPush -never silently changes a command you rely on. A saved command with the same -name as a built-in shadows it. +A saved command remembers **the settings, not just the script**: the +interpreter, whether it runs through sudo, the timeout, how many servers at a +time, and whether a failure stops the rest. Picking one restores all of it. +Those last two matter — "reload nginx" and "upgrade the database tier" want +very different answers, and re-choosing them on every run is how a saved +command still gets run wrong. + +```bash +diskpush fleet commands save careful "systemctl reload nginx" \ + --sudo --concurrency 2 --stop-on-error --timeout 45 --on tag:web + +diskpush fleet run --command careful # 2 at a time, stops on failure +diskpush fleet run --command careful --concurrency 8 # a flag still wins +``` + +In the desktop app the same commands are the strip above the editor. Editing +anything offers **Save these settings**; a saved command carries an × to +delete it. A shipped recipe has no × — it is copied, not edited, so upgrading +DiskPush never silently changes a command you rely on. + +A saved command with the same name as a built-in shadows it. ### `fleet runs` and `fleet show` diff --git a/packages/database/src/migrations.ts b/packages/database/src/migrations.ts index 722e9a3..85251bc 100644 --- a/packages/database/src/migrations.ts +++ b/packages/database/src/migrations.ts @@ -173,4 +173,20 @@ export const MIGRATIONS: Migration[] = [ )`, ], }, + { + name: '004-fleet-command-pacing', + statements: [ + /* + * A saved command captured what to run and where, but not how fast. + * "reload nginx" and "upgrade the database tier" want very different + * answers to those two, and re-choosing them on every run is how a + * saved command still gets run wrong. + * + * Defaults match the schema's, so every existing row reads back exactly + * as it behaved before. + */ + `ALTER TABLE fleet_commands ADD COLUMN concurrency INTEGER NOT NULL DEFAULT 4`, + `ALTER TABLE fleet_commands ADD COLUMN on_failure TEXT NOT NULL DEFAULT 'continue'`, + ], + }, ] diff --git a/packages/database/src/store.ts b/packages/database/src/store.ts index b92da69..59fbb65 100644 --- a/packages/database/src/store.ts +++ b/packages/database/src/store.ts @@ -333,12 +333,13 @@ export class DiskPushStore { await this.client.execute({ sql: `INSERT INTO fleet_commands ( id, name, description, script, interpreter, sudo, working_directory, - timeout_seconds, targets, tags, created_at, updated_at - ) VALUES (?,?,?,?,?,?,?,?,?,?,?,?) + timeout_seconds, concurrency, on_failure, targets, tags, created_at, updated_at + ) VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?) ON CONFLICT(id) DO UPDATE SET name=excluded.name, description=excluded.description, script=excluded.script, interpreter=excluded.interpreter, sudo=excluded.sudo, working_directory=excluded.working_directory, timeout_seconds=excluded.timeout_seconds, + concurrency=excluded.concurrency, on_failure=excluded.on_failure, targets=excluded.targets, tags=excluded.tags, updated_at=excluded.updated_at`, args: [ command.id, @@ -349,6 +350,8 @@ export class DiskPushStore { command.sudo ? 1 : 0, command.workingDirectory, command.timeoutSeconds, + command.concurrency, + command.onFailure, JSON.stringify(command.targets), JSON.stringify(command.tags), command.createdAt, @@ -588,6 +591,8 @@ function rowToFleetCommand(row: Row): FleetCommand { sudo: Number(row.sudo) === 1, workingDirectory: row.working_directory === null ? null : String(row.working_directory), timeoutSeconds: Number(row.timeout_seconds), + concurrency: Number(row.concurrency), + onFailure: String(row.on_failure), targets: JSON.parse(String(row.targets)), tags: JSON.parse(String(row.tags)), builtin: false, diff --git a/packages/fleet-core/src/recipes.test.ts b/packages/fleet-core/src/recipes.test.ts index aa4deec..4d82620 100644 --- a/packages/fleet-core/src/recipes.test.ts +++ b/packages/fleet-core/src/recipes.test.ts @@ -40,6 +40,19 @@ describe('the recipes DiskPush ships', () => { } }) + it('state every field, since nothing parses them through the schema', () => { + // These are plain literals typed as FleetCommand. A field the schema + // merely defaults is `undefined` here, and reached the desktop as + // `setConcurrency(undefined)`. + for (const entry of BUILTIN_RECIPES) { + expect(entry.concurrency, entry.name).toBeGreaterThan(0) + expect(['continue', 'stop'], entry.name).toContain(entry.onFailure) + expect(entry.timeoutSeconds, entry.name).toBeGreaterThan(0) + expect(entry.workingDirectory, entry.name).toBeNull() + expect(Array.isArray(entry.targets), entry.name).toBe(true) + } + }) + it('have unique names, so one cannot shadow another', () => { const names = BUILTIN_RECIPES.map((recipe) => recipe.name) expect(new Set(names).size).toBe(names.length) @@ -68,6 +81,10 @@ describe('copyRecipe', () => { expect(copy.builtin).toBe(false) expect(copy.tags).not.toContain('builtin') expect(copy.script).toBe(findRecipe('upgrade')!.script) + // A copy that lost the pacing would run the same script at a different + // speed, which is the one thing a copy must not do. + expect(copy.concurrency).toBe(findRecipe('upgrade')!.concurrency) + expect(copy.onFailure).toBe(findRecipe('upgrade')!.onFailure) }) }) diff --git a/packages/fleet-core/src/recipes.ts b/packages/fleet-core/src/recipes.ts index f190194..ece9ec6 100644 --- a/packages/fleet-core/src/recipes.ts +++ b/packages/fleet-core/src/recipes.ts @@ -1,4 +1,4 @@ -import { FLEET_DEFAULT_TIMEOUT_SECONDS, type FleetCommand } from '@diskpush/schemas' +import { FLEET_DEFAULT_CONCURRENCY, FLEET_DEFAULT_TIMEOUT_SECONDS, type FleetCommand } from '@diskpush/schemas' import { buildUpgradeScript, CHECK_SCRIPT } from './upgrade.js' /** @@ -25,6 +25,14 @@ function recipe( sudo: false, workingDirectory: null, timeoutSeconds: FLEET_DEFAULT_TIMEOUT_SECONDS, + /* + * Stated, not inherited. These objects are plain literals typed as + * FleetCommand — nothing parses them through the schema, so a field the + * schema defaults is simply `undefined` here. That reached the desktop as + * `setConcurrency(undefined)` and broke the input. + */ + concurrency: FLEET_DEFAULT_CONCURRENCY, + onFailure: 'continue', targets: [], tags: ['builtin'], builtin: true, @@ -134,6 +142,10 @@ export function copyRecipe(recipeToCopy: FleetCommand, name: string): Omit tag !== 'builtin'), builtin: false, diff --git a/packages/schemas/src/fleet.ts b/packages/schemas/src/fleet.ts index dbbb5e4..3d2da31 100644 --- a/packages/schemas/src/fleet.ts +++ b/packages/schemas/src/fleet.ts @@ -61,6 +61,13 @@ export const FleetCommandSchema = z.object({ * Default target selector, in the same syntax `--on` takes. Saved with the * command so `diskpush fleet run deploy-reload` needs no `--on` at all. */ + /** + * How the run is paced. Stored with the command because "reload nginx" and + * "upgrade the database tier" want very different answers, and re-choosing + * them every time is how a saved command still gets run wrong. + */ + concurrency: z.number().int().positive().default(FLEET_DEFAULT_CONCURRENCY), + onFailure: FleetFailureModeSchema.default('continue'), targets: z.array(z.string().min(1)).default([]), tags: z.array(z.string()).default([]), /**