diff --git a/CHANGELOG.md b/CHANGELOG.md index b2eff53..4726c31 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,42 @@ Notable changes to the Swarm plugin. Format follows ## [Unreleased] +### Added +- Per-repo port ranges: `git config swarm.portStart` / `swarm.portSpan`. Two + repos swarming at once both started at 4100, and the only override was + environment-only, which an action-invoked fan-out pane never inherits. An + exported `HERDR_SWARM_PORT_START`/`_SPAN` still wins. + +### Fixed +- Merge winner could skip — and, on a clean worktree, archive and so kill — an + agent that was working again. Which slots were "finished" came from the + compare table, and a `marker` finish is never cleared. It now runs `settle` + after the merge and re-reads compare: a slot is skipped only if it is still + finished and its tip and dirty count are what you saw. Anything else is left + running and named. +- Merge winner checked base drift against the base it had just re-read, so a + base that moved after you confirmed merged without the drift warning. It now + checks against the base current at confirm, and refuses if it moved. +- Archive, discard and abort no longer act on a slot whose `validate` hook is + still running. The hook runs with the repo lock released and could write + ignored files after the cleanup inventory was approved, or after the + discard snapshot. +- A timeout that is not whole seconds (`HERDR_SWARM_CLONE_TIMEOUT=2m`, `0900`, + `08`) was found only after the timed command had started: the arithmetic + error ended the caller, leaving `cp` (or the validate hook) running with no + deadline and, for validate, its per-slot lock dropped. Timeouts are now + parsed as decimal before anything starts, and fan-out refuses a bad clone or + setup timeout before creating any worktree. +- `validate` exited 30 (base drift) when the *slot* moved while its hook ran. + It now exits 36 (refused); 30 still means only that the base moved. +- Compare could rank a slot `passed` on a validation record that the strict + candidate handoff rejects (one without the hook's own `validate` check): + the two had separate copies of the identity rule. Both now use + `isSwarmValidationRecord` from `pr-handoff.mjs`. +- A `validate` that times out in the harvest pane now names + `HERDR_SWARM_VALIDATE_TIMEOUT`, the setting that sets its budget, not the + generic step timeout. + ## [0.5.0] — 2026-10-04 The "pick the winner" release: Swarm now checks each slot, notices when a @@ -70,7 +106,11 @@ merging. Exercised live end to end on Herdr 0.8.2 (see `docs/readiness.md`). gets `HERDR_SWARM_RUN_ID`, `_SLOT`, `_PORT_BASE`, `_PORT_SPAN`: in `setup.sh` always, in the agent via `pane split --env` on Herdr 0.7.5+, and as a port-range line in the task file. Ports default to 10 per slot from - 4100 and are recorded on the manifest row. + 4100 and are recorded on the manifest row. Settings: + `HERDR_SWARM_PORT_START` / `_SPAN`, `HERDR_SWARM_CLONE_MODE=copy` (full copy + where copy-on-write is unavailable), `HERDR_SWARM_CLONE_TIMEOUT` (default + 120s), and a `clone-paths` file in the plugin config dir. Clone temps + (`.swarm-clone.*`) are added to `info/exclude`. - Compare view (`w` in the harvest pane; scriptable as the read-only `compare` verb). It ranks running slots by validation of their current tip, then commits, then finished, never by diff size, and shows commits, @@ -78,8 +118,11 @@ merging. Exercised live end to end on Herdr 0.8.2 (see `docs/readiness.md`). `d` shows the full diff between two slots' tips in git's pager. `m` merges a chosen winner through the normal merge, and only if it lands, skips and archives the other running slots (branches kept; anything that won't - archive cleanly is left and named). -- Finish detection: the status pane runs a new `settle` verb every 10s. It + archive cleanly is left and named). The merge verb takes an optional third + argument, `merge [expected-slot-tip]`: it refuses unless + the slot is still at that SHA, then merges the SHA, not the branch name. +- Finish detection: the status pane runs a new `settle` verb every 10s + (`HERDR_SWARM_SETTLE_INTERVAL_MS`). It records a slot as finished when its agent creates `.swarm-done` (now asked for in the task file's standing instructions, and excluded from `git status` and from ignored-file cleanup approval like `.swarm-task.md`). It also does @@ -87,7 +130,8 @@ merging. Exercised live end to end on Herdr 0.8.2 (see `docs/readiness.md`). Finished slots show `finished`, flip their plugin-reported state to `idle`, and get one Herdr notification per run when all are done. Opt-in auto-validate (`HERDR_SWARM_AUTO_VALIDATE=1` or a config-dir - `auto-validate` file) runs `validate` on each settled slot. + `auto-validate` file) runs `validate` on each finished slot. An empty + `.swarm-done` is exempt from that approval; one with content is not. - `validate ` harvest verb (`v`, then slot in the pane) runs the operator's `validate.sh` hook against a slot's clean HEAD and records a SHA-bound checks file. The strict candidate handoff now produces its own diff --git a/CONCEPTS.md b/CONCEPTS.md index be14d10..6db8db1 100644 --- a/CONCEPTS.md +++ b/CONCEPTS.md @@ -42,6 +42,22 @@ The Manifest is what makes a crashed Run recoverable rather than orphaned. It is archived rather than deleted when a Run ends, because it remains the record that lets cleanup resolve what a Run left behind. +### Finished +A running Slot whose agent has stopped working on its task, recorded as +`finished: {at, reason}` on the Slot while its status stays `running`. It is +evidence for the operator, not a lifecycle state: it never merges, skips or +archives anything by itself. Two reasons, and they differ in strength: +`marker` (the agent created the Finish marker) is final; `exited` (the pane is +back at its bare shell) is reversible, and is cleared if the agent is seen +busy again. Anything that acts on Finished re-reads it at the moment it acts, +because the operator may have given the agent more work since. + +### Finish marker +The empty `.swarm-done` file an agent creates in its worktree root to declare +it is done, as the task file asks. Excluded from git status like the task +file. Only an empty marker is exempt from the ignored-file approval at archive; +one with content is agent output like any other file. + ### Preset A named agent configuration a Slot runs. Presets are what make fan-out agent-agnostic: any command can be a Slot's agent, not a fixed list. @@ -99,6 +115,20 @@ The base branch moving between the moment a merge was previewed and the moment it would land. Detected drift sends the Slot back to re-preview rather than merging against a base the user never reviewed. +### Compared tip +The exact commit a Slot's branch pointed at when the compare view read it. +Picking a winner pins this SHA: the merge refuses if the branch has moved and +merges the SHA itself, not the branch name, so what lands is what was +compared. + +### Validation result +The checks file the `validate` verb writes for one Slot, bound to the HEAD it +ran against. Because it sits at a predictable path, a file there counts only if +it proves it is Swarm's own record for exactly this Run and Slot, with the +hook's own `validate` check present. One rule decides that, shared by the +compare ranking and the strict candidate handoff. A result for an older commit +is stale, never a pass. + ### Snapshot A copy of a Slot's uncommitted work — tracked and untracked — written to a plugin-owned ref before any discard. Discard is the only operation that @@ -127,3 +157,9 @@ since the leftovers may hold committed work that never landed. - "Archive" applies to two different things and both are intentional: a Slot is archived (worktree removed, branch kept), and a Manifest is archived (kept as a recovery record rather than deleted). Neither means "delete." +- "Settle" has an older and a newer meaning. In the Slot lifecycle, a settled + Slot is one whose agent has stopped (merged, skipped and failed come after + it). The `settle` verb and its `settledslotreason` output line are + Finish detection: they record Finished on a Slot that stays `running`. The + verb name is a scripting contract and is kept; in prose, say Finished for + the verb's result. diff --git a/README.md b/README.md index a2fce59..18df007 100644 --- a/README.md +++ b/README.md @@ -453,7 +453,8 @@ Where copy-on-write isn't available, behaviour depends on the platform: - **macOS:** `cp -c` silently falls back to a full copy when it can't clone, for example on a non-APFS volume or with worktrees on another volume. -`HERDR_SWARM_CLONE_TIMEOUT` (default 120 s) bounds each clone. A timeout stops +`HERDR_SWARM_CLONE_TIMEOUT` (default 120 s, whole seconds — `2m` refuses the +fan-out before any worktree is created) bounds each clone. A timeout stops `cp` and everything it started, and removes the partial copy. A clone failure never fails the slot. Cloning happens before `setup.sh`, so the hook can build on it. @@ -487,7 +488,23 @@ Each slot gets `HERDR_SWARM_RUN_ID`, `HERDR_SWARM_SLOT`, Ports start at `HERDR_SWARM_PORT_START` (default 4100), with `HERDR_SWARM_PORT_SPAN` (default 10) per slot: slot 1 gets 4100–4109, slot 2 gets 4110–4119. This is a convention the agents are told, not an OS -reservation. A range that doesn't fit in 1024–65535 refuses the fan-out. The same applies to repo hooks during harvest: a hook that +reservation. A range that doesn't fit in 1024–65535 refuses the fan-out. + +Ranges are not coordinated across repos: two repos swarming at the same time +both start at 4100. Give each repo its own range with git config, which a +fan-out pane reads even though it never inherits your shell's environment: + +```sh +git config swarm.portStart 5100 # in the second repo +git config swarm.portSpan 10 # optional +``` + +An exported `HERDR_SWARM_PORT_START`/`_SPAN` still overrides git config. + +### Fresh-worktree hooks + +Fresh worktrees lack gitignored files that the clone list doesn't cover +(`.env`, build output). The same applies to repo hooks during harvest: a hook that shells into `node_modules/.bin` fails in the plugin-owned merge worktree — `HERDR_SWARM_HARVEST_WT_NO_HOOKS=1` disables hooks in that worktree *only* (never in your tree). To automate worktree prep, drop a `setup.sh` in the @@ -627,8 +644,9 @@ In the harvest pane, `w` opens the compare view, which replaces the slot list with one ranked row per slot. The same data is scriptable as `bash scripts/harvest-step.sh compare`, which emits one `compare_slot{json}` per slot and one -`compare_overlapabn[files]` per candidate pair. It is -read-only. +`compare_overlapabn[files]` per candidate pair, where +`n` is `-1` when either slot's diff could not be read (overlap unknown, not +zero) and `[files]` lists at most 10 of the shared files. It is read-only. | Column | Source | | --- | --- | @@ -640,8 +658,9 @@ read-only. **Ranking** is an order, not a score: checks (passed > not run > stale > failed), then having commits, then finished, then slot number. Diff size is -shown but never ranked, because a smaller change isn't a better one. Settled -slots (merged, skipped) are listed without a rank. +shown but never ranked, because a smaller change isn't a better one. Slots +that are no longer running (merged, skipped, failed) are listed without a +rank. In the compare view: @@ -650,21 +669,25 @@ In the compare view: merge-tree shell. - `m`, then a slot digit, then `y`: **merge the winner and skip the rest**. The winner is merged **at the exact tip the table showed**: if its agent - committed since, the merge refuses and you re-open compare. Otherwise it - goes through the normal merge: re-previewed and drift-checked. If the merge - would now land in your checked-out branch and the confirm didn't say so, - you are asked again. **Only if that merge lands** are the other running - slots whose agent has *finished* marked skipped and archived. Slots still + committed since, the merge refuses and you re-open compare. If the base + moved after you confirmed, it refuses too. Otherwise it goes through the + normal merge: re-previewed and drift-checked. If the merge would now land + in your checked-out branch and the confirm didn't say so, you are asked + again. **Only if that merge lands** are the other running slots whose agent + has *finished* marked skipped and archived. "Finished" is re-checked at + that moment, not taken from the table: finish detection runs again, and a + slot whose tip or uncommitted changes differ from what the table showed is + treated as working (you may have given its agent a follow-up). Slots still working are left running and named. Branches are always kept. A worktree that won't archive cleanly (dirty, ignored files, a working agent) is left in place and named in the banner. If that's the winner's, its approval prompt stays open. A winner that isn't clean, or a merge that conflicts, skips nothing. +- `r` refreshes; `b`/Esc goes back. Scripted, the same pin is `harvest-step.sh merge `: it refuses unless the slot is still at ``, then merges that SHA rather than the branch name. -- `r` refreshes; `b`/Esc goes back. ## Finish detection @@ -696,7 +719,7 @@ yourself. A marker with content in it is treated as agent data, so archive needs the usual ignored-file approval to remove it. **Auto-validate (opt-in).** With `HERDR_SWARM_AUTO_VALIDATE=1`, or an empty -`auto-validate` file in the plugin config dir, a slot that settles also starts +`auto-validate` file in the plugin config dir, a slot that finishes also starts a detached `validate` for that slot, if `validate.sh` exists (log: `auto-validate--s.log` in the state dir). Use the file when the status pane is opened through the plugin action, because action-launched panes @@ -725,7 +748,7 @@ npm test >/dev/null 2>&1 && echo "unit-tests passed" >>"$HERDR_SWARM_CHECKS_FILE reserved line records the whole result as failed rather than trusting part of it. - A dirty slot refuses before the hook runs. If the slot is dirty or its HEAD - moved when the hook finishes, nothing is recorded (exit 36 or 30). A hook + moved when the hook finishes, nothing is recorded (exit 36). A hook that leaves build output behind needs that output gitignored. - Passing and failing hooks both exit 0 with a `validatedslotshapassed|failedpath` record. @@ -738,6 +761,9 @@ npm test >/dev/null 2>&1 && echo "unit-tests passed" >>"$HERDR_SWARM_CHECKS_FILE sent TERM and then KILL. A dev server or test worker it started therefore can't outlive it. A timeout is recorded as failed. A process that calls `setsid` to leave its group is beyond reach. +- While a slot's hook runs, archive, discard and abort refuse to touch that + slot's worktree (abort keeps it and says so): the hook may still be writing + files there that no cleanup approval or snapshot has seen. - Only one validate runs per slot at a time; a second is refused (exit 36). - `candidate-status` and `publish-candidate-pr` use this result when `HERDR_SWARM_CANDIDATE_VALIDATION_FILE` is unset. In that case the file must diff --git a/bin/renderer-harvest.mjs b/bin/renderer-harvest.mjs index ef72581..8e3029d 100644 --- a/bin/renderer-harvest.mjs +++ b/bin/renderer-harvest.mjs @@ -83,8 +83,9 @@ export function summarizeIgnored(files, limit = 20) { } // The pane's env minus every variable that reroutes or reconfigures git — -// the same list scripts/lib.sh clear_git_routing_env removes. -const GIT_ROUTING = new Set(["GIT_DIR", "GIT_WORK_TREE", "GIT_COMMON_DIR", "GIT_INDEX_FILE", "GIT_OBJECT_DIRECTORY", +// the same list scripts/lib.sh clear_git_routing_env removes (a test holds +// the two in lockstep). +export const GIT_ROUTING = new Set(["GIT_DIR", "GIT_WORK_TREE", "GIT_COMMON_DIR", "GIT_INDEX_FILE", "GIT_OBJECT_DIRECTORY", "GIT_ALTERNATE_OBJECT_DIRECTORIES", "GIT_NAMESPACE", "GIT_CEILING_DIRECTORIES", "GIT_DISCOVERY_ACROSS_FILESYSTEM", "GIT_PREFIX", "GIT_CONFIG_COUNT", "GIT_CONFIG_PARAMETERS"]); export function gitDisplayEnv(env) { @@ -414,7 +415,9 @@ export class HarvestRenderer { } timeoutBanner(verb, timeoutMs = this.stepTimeoutMs) { - return `harvest step '${verb}' timed out after ${timeoutMs}ms and was killed — check for a hung git hook or lock, then retry (HERDR_SWARM_STEP_TIMEOUT_MS raises the limit)`; + // validate's budget follows the hook timeout, so that is the knob. + const knob = verb === "validate" ? "HERDR_SWARM_VALIDATE_TIMEOUT" : "HERDR_SWARM_STEP_TIMEOUT_MS"; + return `harvest step '${verb}' timed out after ${timeoutMs}ms and was killed — check for a hung git hook or lock, then retry (${knob} raises the limit)`; } lastErrLine(res) { @@ -583,13 +586,21 @@ export class HarvestRenderer { // Compare view: one read-only `compare` verb call, parsed into the phase. async openCompare() { - const r = await this.step("compare"); - if (r.code !== 0) { - this.banner = this.lastErrLine(r); + const c = await this.readCompare(); + if (!c.ok) { + this.banner = c.error; this.phase = { name: "list" }; this.paint(); return; } + this.phase = { name: "compare", slots: c.slots, overlaps: c.overlaps, pick: null }; + this.paint(); + } + + // The `compare` verb, parsed: { ok, slots, overlaps } or { ok: false, error }. + async readCompare() { + const r = await this.step("compare"); + if (r.code !== 0) return { ok: false, error: this.lastErrLine(r) }; const slots = []; for (const [json] of r.out.compare_slot ?? []) { try { @@ -607,8 +618,7 @@ export class HarvestRenderer { } return { a: Number(a), b: Number(b), count: Number(n), shared: files }; }); - this.phase = { name: "compare", slots, overlaps, pick: null }; - this.paint(); + return { ok: true, slots, overlaps }; } // Two slots' tips, full diff, in git's own pager: the same terminal @@ -640,15 +650,21 @@ export class HarvestRenderer { }); } - // Merge the chosen slot at the exact tip the compare view showed, then — - // only if that merge LANDED — skip every other running slot whose agent - // has FINISHED and archive whichever archive cleanly. Unfinished slots are - // left running (skipping one would let its agent keep committing to a - // settled slot nobody looks at again). Branches are always kept (skip is - // bookkeeping; archive never deletes a branch). Anything that refuses to - // archive — the winner included — is left in place and named, never forced. + // Merge the chosen slot at the exact tip the compare view showed, onto the + // base that was current when it was confirmed, then — only if that merge + // LANDED — skip every other running slot whose agent has FINISHED and + // archive whichever archive cleanly. Unfinished slots are left running + // (skipping one would let its agent keep committing to a slot nobody looks + // at again). "Finished" is re-read after the merge, not trusted from the + // compare table: settle runs first (an exited agent restarted by hand + // flips back to working), and a slot whose tip or dirty count changed + // since compare is treated as resumed — a `marker` finish is never + // cleared by settle, so changed work is the only sign an agent was given a + // follow-up. Branches are always kept (skip is bookkeeping; archive never + // deletes a branch). Anything that refuses to archive — the winner + // included — is left in place and named, never forced. async mergeWinner(confirm) { - const { slot, tip, others, unfinished, locus } = confirm; + const { slot, tip, others, unfinished, locus, base, seen } = confirm; await this.refresh(); const row = this.rows.find((r) => r.slot === slot); if (row?.preview?.state !== "clean") { @@ -665,14 +681,32 @@ export class HarvestRenderer { this.paint(); return; } + // The merge verb's drift guard compares against the base it is handed; + // handing it the base refresh() just read would compare new to new. + if (row.preview.baseSha !== base) { + this.banner = `base moved since you confirmed (${String(base ?? "").slice(0, 7)} -> ${String(row.preview.baseSha ?? "").slice(0, 7)}) — compare again; nothing was merged or skipped`; + this.phase = { name: "list" }; + this.paint(); + return; + } if (!(await this.doMerge(slot, tip))) return; // doMerge already tried to archive the winner; an ignored-files approval // or dirty prompt it raised must survive the summary below. const winnerPrompt = ["ignored", "dirty"].includes(this.phase.name) ? this.phase : null; const skipped = []; const kept = []; + const resumed = [...unfinished]; + const settled = await this.step("settle"); + const now = settled.code === 0 ? await this.readCompare() : { ok: false }; for (const s of others) { if (this.rows.find((r) => r.slot === s)?.status !== "running") continue; + const cur = now.ok ? now.slots.find((c) => c.slot === s) : null; + const was = seen?.[s]; + // Unconfirmable is unfinished: no fresh reading, no skip. + if (!cur?.finished || !was || cur.tip !== was.tip || cur.dirty !== was.dirty) { + resumed.push(s); + continue; + } const sk = await this.step("skip", [s]); if (sk.code !== 0) { kept.push(`${s} (skip failed)`); @@ -690,7 +724,7 @@ export class HarvestRenderer { if (winnerRow && winnerRow.status !== "archived") kept.unshift(`${slot} (winner)`); this.phase = winnerPrompt ?? { name: "list" }; this.banner = `slot ${slot} merged${skipped.length ? `; skipped ${skipped.join(", ")}` : ""}${ - unfinished.length ? `; still running (agent not finished): ${unfinished.join(", ")}` : "" + resumed.length ? `; still running (agent not finished): ${resumed.join(", ")}` : "" }${kept.length ? `; worktrees kept for ${kept.join(", ")} — archive them from the list` : ""}`; this.paint(); } @@ -1017,7 +1051,10 @@ export class HarvestRenderer { tip: chosen.tip, others: rest.filter((s) => s.finished).map((s) => s.slot), unfinished: rest.filter((s) => !s.finished).map((s) => s.slot), + // What the operator is agreeing to skip, as they saw it. + seen: Object.fromEntries(rest.map((s) => [s.slot, { tip: s.tip, dirty: s.dirty }])), locus: this.rows.find((r) => r.slot === n)?.preview?.locus ?? null, + base: this.rows.find((r) => r.slot === n)?.preview?.baseSha ?? null, back: ph, }; } diff --git a/docs/residual-review-findings/finish-detection-2026-10-03.md b/docs/residual-review-findings/finish-detection-2026-10-03.md index eca3095..48856fd 100644 --- a/docs/residual-review-findings/finish-detection-2026-10-03.md +++ b/docs/residual-review-findings/finish-detection-2026-10-03.md @@ -54,3 +54,12 @@ labelled finished rather than lost, which is misleading but not destructive. That includes `HERDR_SOCKET_PATH` / `HERDR_PANE_ID`. A `validate.sh` that calls bare `herdr` acts as the Status pane. The same exposure already exists when validate runs from the Harvest pane. + +## Addendum 2026-10-06 + +"Roadmap item 3" above is the compare view, which shipped in 0.5.0. Finding 2 +still stands: a refused auto-validate writes no result file, so compare shows +the slot as `not run`, the same as one never validated; the refusal is still +only in the log. Separately, merge winner no longer trusts the compare +table's `finished` column at the moment it skips slots. It re-runs settle and +re-reads compare after the merge (fixed on worktree-swarm-review-fixes). diff --git a/docs/residual-review-findings/validate-verb-2026-10-03.md b/docs/residual-review-findings/validate-verb-2026-10-03.md index ac11820..9633805 100644 --- a/docs/residual-review-findings/validate-verb-2026-10-03.md +++ b/docs/residual-review-findings/validate-verb-2026-10-03.md @@ -48,3 +48,13 @@ recovers it. `validation--s.{json,log}` and `validation-checks.*` left by a SIGKILL accumulate in the `0700` state dir. Run IDs are unique, so nothing collides; this is disk use only. Fold into prune when it next changes. + +## Addendum 2026-10-06 + +"Roadmap item 2" above is finish detection, which shipped in 0.5.0. Finding 2 +still stands: manual `validate` does not wait for a finished agent. Only +auto-validate is tied to finishing. Two related gaps from the post-0.5.0 +review are now fixed: archive, discard and abort refuse while a slot's hook is +running, and the validate timeout is parsed as decimal before the hook starts +(`0900` used to be octal, and `08` aborted mid-run with the per-slot lock +dropped). diff --git a/scripts/abort.sh b/scripts/abort.sh index 8d67139..1565ce9 100755 --- a/scripts/abort.sh +++ b/scripts/abort.sh @@ -253,6 +253,11 @@ reap_slot_worktree() { note_kept "slot $slot worktree $wt (ownership check failed: $why)" return 0 fi + if validate_in_flight "$RUN_ID" "$slot"; then + echo "herdr-swarm: slot $slot KEPT — a validate hook is still running in $wt; abort again once it finishes." >&2 + note_kept "slot $slot worktree $wt (validate in flight)" + return 0 + fi if [ -n "$(git -C "$wt" status --porcelain 2>/dev/null)" ]; then # Dirty is KEPT, never prompted (no TTY) and never forced (R11); the # commit/skip/discard flow belongs to harvest (R9), not abort. diff --git a/scripts/compare.mjs b/scripts/compare.mjs index d103d6d..cff99e6 100644 --- a/scripts/compare.mjs +++ b/scripts/compare.mjs @@ -1,6 +1,7 @@ // compare.mjs — read-only slot comparison behind `harvest-step.sh compare`. -// The verb verifies ownership of every row in bash, then feeds one TSV line -// per slot on stdin: slot, label, branch, path, status, finished_reason. This +// The verb verifies ownership of every row in bash, then feeds one +// \x1f-separated line per slot on stdin: slot, label, branch, path, status, +// finished_reason, started_at, finished_at. This // script only READS git (rev-parse, rev-list, diff, status) and the // Swarm-produced validation files, and prints: // @@ -15,6 +16,7 @@ import fs from "node:fs"; import path from "node:path"; import { spawnSync } from "node:child_process"; import { fileURLToPath } from "node:url"; +import { isSwarmValidationRecord } from "./pr-handoff.mjs"; const VALIDATION_ORDER = { passed: 0, none: 1, stale: 2, failed: 3 }; const SHARED_LIMIT = 10; @@ -34,9 +36,9 @@ function validationFor(stateDir, runId, slot, tip) { } catch { return { status: "none" }; } - // Same identity rule the strict candidate gate applies to the default file. - if (value?.kind !== "herdr-swarm-validation" || value.run_id !== runId || value.slot !== slot || - !["passed", "failed"].includes(value.status)) return { status: "none" }; + // The strict candidate gate's own rule, so compare never ranks "passed" a + // record that gate would reject. + if (!isSwarmValidationRecord(value, runId, slot) || !["passed", "failed"].includes(value.status)) return { status: "none" }; if (!tip || value.head_sha !== tip) return { status: "stale", head_sha: value.head_sha ?? null }; const failed = (Array.isArray(value.checks) ? value.checks : []) .filter((c) => c?.status !== "passed").map((c) => String(c?.name ?? "?")).slice(0, 5); diff --git a/scripts/fanout-pane.sh b/scripts/fanout-pane.sh index 453d78d..6ea4694 100755 --- a/scripts/fanout-pane.sh +++ b/scripts/fanout-pane.sh @@ -605,8 +605,12 @@ preflight_check_argv "${slot_argvs[@]}" || fatal $? # Same rule for ports: the HIGHEST slot's range is checked here, so a range # that only overflows for slot N never strands slots 1..N-1 half-built. -slot_port_base "$n" >/dev/null || fatal 1 "herdr-swarm: HERDR_SWARM_PORT_START/_SPAN give $n slots no valid port range (need 1024-65535, decimal)." +slot_port_base "$n" >/dev/null || fatal 1 "herdr-swarm: HERDR_SWARM_PORT_START/_SPAN (or git config swarm.portStart/portSpan) give $n slots no valid port range (need 1024-65535, decimal)." port_span="$(slot_port_span)" +# And for the timeouts fan-out applies per slot: a bad value found mid-run +# would only surface as every clone or setup silently "failing". +positive_seconds "${HERDR_SWARM_CLONE_TIMEOUT:-120}" >/dev/null || fatal 1 "herdr-swarm: HERDR_SWARM_CLONE_TIMEOUT must be a positive whole number of seconds." +positive_seconds "${HERDR_SWARM_SETUP_TIMEOUT:-300}" >/dev/null || fatal 1 "herdr-swarm: HERDR_SWARM_SETUP_TIMEOUT must be a positive whole number of seconds." # --- Run identity and manifest ---------------------------------------------- @@ -728,8 +732,10 @@ for ((i = 1; i <= n; i++)); do slot_env=(HERDR_SWARM_RUN_ID="$run_id" HERDR_SWARM_SLOT="$i" HERDR_SWARM_PORT_BASE="$port_base" HERDR_SWARM_PORT_SPAN="$port_span") - # Optional per-repo setup hook: fresh worktrees lack gitignored deps - # (.env, node_modules), the most likely "all my agents failed" cause. + # Optional per-repo setup hook, run after the dependency clones above: + # it covers whatever they did not (.env, build output, a dep tree the + # clone list skipped) — missing deps being the likeliest "all my agents + # failed" cause. # Failure warns but does not fail the slot — the agent may not need # what setup provides, and the warning names the log for diagnosis. setup_hook="${HERDR_PLUGIN_CONFIG_DIR:-}/setup.sh" diff --git a/scripts/harvest-step-settle.sh b/scripts/harvest-step-settle.sh index 71ddbe6..1b19d09 100644 --- a/scripts/harvest-step-settle.sh +++ b/scripts/harvest-step-settle.sh @@ -24,8 +24,8 @@ auto_validate_enabled() { # AND a bare shell on two consecutive settles, so a reading taken # mid-restart does not count. # A finished slot gets `finished: {at, reason}` (status stays `running`; the -# harvest state machine is untouched), with auto-validate on a detached -# `validate` is started for it, and its plugin-reported agent state flips to +# harvest state machine is untouched); with auto-validate on, a detached +# `validate` is started for it; and its plugin-reported agent state flips to # idle. `exited` is reversible: a later busy reading (Ctrl-Z then fg, an # agent restarted by hand) clears it and reports working again. `marker` is # the agent's own declaration and is final. When every running slot has diff --git a/scripts/harvest-step-slot.sh b/scripts/harvest-step-slot.sh index 8150c10..f0cf3ab 100644 --- a/scripts/harvest-step-slot.sh +++ b/scripts/harvest-step-slot.sh @@ -141,6 +141,10 @@ do_discard() { echo "herdr-swarm: slot $1 worktree is gone ($SLOT_PATH)" >&2 return "$HS_EC_REFUSED" } + if validate_in_flight "$RUN_ID" "$1"; then + echo "herdr-swarm: slot $1 is being validated — its hook is still writing in the worktree; discard refused until it finishes." >&2 + return "$HS_EC_REFUSED" + fi # reset --hard, not `checkout -- .`: checkout restores the working tree FROM # the index, so STAGED changes survive it. The verb then reports "discarded" # while the slot is still dirty, and every later archive fails HS_EC_DIRTY @@ -204,6 +208,10 @@ do_archive() { finalize_run "$REPO_ROOT" "$RUN_ID" || return $? return 0 fi + if validate_in_flight "$RUN_ID" "$1"; then + echo "herdr-swarm: slot $1 is being validated — its hook is still writing in the worktree; archive refused until it finishes." >&2 + return "$HS_EC_REFUSED" + fi # Cleanup is preview/apply, not a process-global boolean. The recursive # inventory is canonicalized as NUL-delimited path bytes and its digest is # bound to repo/run/slot/physical worktree/operation. Apply re-verifies diff --git a/scripts/harvest-step-validate.sh b/scripts/harvest-step-validate.sh index 0dbedf2..c0052c6 100644 --- a/scripts/harvest-step-validate.sh +++ b/scripts/harvest-step-validate.sh @@ -91,13 +91,10 @@ do_validate() { echo "herdr-swarm: no validate.sh in the plugin config dir — add one to run checks per slot (see README: Validating slots)." >&2 return "$HS_EC_REFUSED" fi - case "${HERDR_SWARM_VALIDATE_TIMEOUT:-900}" in - '' | *[!0-9]* | 0) + secs="$(positive_seconds "${HERDR_SWARM_VALIDATE_TIMEOUT:-900}")" || { echo "herdr-swarm: HERDR_SWARM_VALIDATE_TIMEOUT must be a positive number of seconds." >&2 return "$HS_EC_REFUSED" - ;; - *) secs="${HERDR_SWARM_VALIDATE_TIMEOUT:-900}" ;; - esac + } head="$(slot_clean_head "$1")" || return $? VALIDATE_LOCK="$(state_dir)/validating-$RUN_ID-s$1" trap 'release_lock "$MUTATION_LOCK"; validating_lock_drop "${VALIDATE_LOCK:-}"' EXIT @@ -155,7 +152,7 @@ validate_record() { } if [ "$after" != "$head" ]; then echo "herdr-swarm: slot $slot moved from $head to $after while validate.sh ran — result not recorded; re-run validate." >&2 - return "$HS_EC_DRIFT" + return "$HS_EC_REFUSED" fi recorded="$(node -e ' const fs = require("fs"); diff --git a/scripts/lib.sh b/scripts/lib.sh index 3a1d6e9..e24918c 100644 --- a/scripts/lib.sh +++ b/scripts/lib.sh @@ -10,6 +10,18 @@ HERDR="${HERDR_BIN_PATH:-herdr}" # shellcheck disable=SC2034 # consumed by sourcing scripts, not here PLUGIN_ID="structupath.swarm" +# validate_in_flight : a `validate` hook is running in this +# slot's worktree right now (its per-slot lock is held by a live pid). The +# hook runs with the repo lock RELEASED and may still be writing — coverage/, +# .next/ — so anything that resets or removes that worktree checks this +# under the repo lock first; otherwise files written after an inventory or +# snapshot would go with it, unapproved. +validate_in_flight() { + local holder + holder="$(cat "$(state_dir)/validating-$1-s$2/pid" 2>/dev/null)" || return 1 + [ -n "$holder" ] && kill -0 "$holder" 2>/dev/null +} + state_dir() { local d="${HERDR_PLUGIN_STATE_DIR:-$HOME/.local/state/herdr-swarm}" # A literal leading '~' inside a variable value is never expanded by bash; @@ -143,11 +155,28 @@ clear_git_routing_env() { done < <(compgen -e) } +# positive_seconds : digits only, read as DECIMAL (10#) — bash reads +# "0900" as octal and rejects "08" outright, mid-arithmetic — capped in +# length so it cannot overflow, and > 0. Prints the normalized integer. +positive_seconds() { + case "${1-}" in + '' | *[!0-9]*) return 1 ;; + esac + [ "${#1}" -le 7 ] && [ "$((10#$1))" -gt 0 ] || return 1 + printf '%s\n' "$((10#$1))" +} + # Portable timeout (macOS lacks GNU timeout): poll the child and SIGKILL it # after SECONDS (exit 137). Done in-shell (no background watchdog) so a dying # script can never orphan a sleep that holds the caller's stdout pipe open. +# A bad SECS refuses (exit 2) BEFORE anything starts: found after the fork, +# the arithmetic error would abort the caller and orphan the child. with_timeout() { - local secs="$1" + local secs + secs="$(positive_seconds "${1-}")" || { + echo "herdr-swarm: internal error: timeout '${1-}' is not a positive whole number of seconds" >&2 + return 2 + } shift "$@" & local pid=$! @@ -175,7 +204,13 @@ with_timeout() { # shell, not a subshell, or neither variable reaches the caller. # shellcheck disable=SC2034 # SWARM_GROUP_TIMED_OUT is read by callers with_group_timeout() { - local secs="$1" deadline rc + local secs deadline rc + # Checked before the fork, as in with_timeout: a child with no deadline + # is exactly what this function exists to prevent. + secs="$(positive_seconds "${1-}")" || { + echo "herdr-swarm: internal error: timeout '${1-}' is not a positive whole number of seconds" >&2 + return 2 + } shift SWARM_GROUP_TIMED_OUT=0 set -m diff --git a/scripts/pr-handoff.mjs b/scripts/pr-handoff.mjs index 115a982..0ceb0c8 100644 --- a/scripts/pr-handoff.mjs +++ b/scripts/pr-handoff.mjs @@ -108,6 +108,17 @@ function candidateRefuse(issue, message) { throw new CandidateEvidenceError(issue, message); } +// A file nobody named (the slot's default validate result) sits at a +// predictable path, so it must also prove it is Swarm's own record for exactly +// this run and slot, with the hook's own check present. The one definition of +// that rule: the compare view ranks by it too, and a copy here and there had +// already drifted apart. +export function isSwarmValidationRecord(value, runId, slot) { + return value?.kind === "herdr-swarm-validation" && value.schema_version === 1 && + value.run_id === runId && value.slot === slot && Array.isArray(value.checks) && + value.checks.some(check => check?.name === "validate"); +} + function candidateValidation(filename, plan) { const sha = plan.sha; if (!filename) candidateRefuse("validation_missing", "Candidate handoff requires a validation evidence file."); @@ -118,12 +129,7 @@ function candidateValidation(filename, plan) { candidateRefuse("validation_invalid", `Candidate validation evidence is invalid: ${error.message}`); } if (value?.schema_version !== 1 || !Array.isArray(value.checks)) candidateRefuse("validation_invalid", "Candidate validation evidence must use the typed checks schema."); - // A file nobody named (the slot's default validate result) sits at a - // predictable path, so it must also prove it is Swarm's own record for - // exactly this run and slot, with the hook's own check present. - if (process.env.HERDR_SWARM_CANDIDATE_VALIDATION_PRODUCED === "1" && - (value.kind !== "herdr-swarm-validation" || value.run_id !== plan.run || value.slot !== plan.slot || - !value.checks.some(check => check?.name === "validate"))) { + if (process.env.HERDR_SWARM_CANDIDATE_VALIDATION_PRODUCED === "1" && !isSwarmValidationRecord(value, plan.run, plan.slot)) { candidateRefuse("validation_invalid", "The slot's validate result is not a Swarm validation record for this run and slot."); } if (value.head_sha !== sha) candidateRefuse("validation_stale", "Candidate validation evidence does not match the selected slot HEAD."); diff --git a/scripts/preflight.sh b/scripts/preflight.sh index a2fe9e0..50f7854 100644 --- a/scripts/preflight.sh +++ b/scripts/preflight.sh @@ -415,10 +415,12 @@ provision_slot_paths() { # slot_port_base : the first of HERDR_SWARM_PORT_SPAN (default 10) # ports reserved for this slot, from HERDR_SWARM_PORT_START (default 4100): # slot 1 → 4100–4109, slot 2 → 4110–4119. A convention the agents are told -# about, not a reservation the OS enforces. +# about, not a reservation the OS enforces. The ranges know nothing about +# other repos: two repos swarming at once both start at 4100 unless one of +# them sets swarm.portStart. slot_port_base() { local start span base - start="$(_port_decimal "${HERDR_SWARM_PORT_START:-4100}")" || return 1 + start="$(_port_decimal "$(_port_setting "${HERDR_SWARM_PORT_START:-}" swarm.portStart 4100)")" || return 1 span="$(slot_port_span)" || return 1 [ "$span" -ge 1 ] || return 1 base=$((start + ($1 - 1) * span)) @@ -427,7 +429,18 @@ slot_port_base() { } slot_port_span() { - _port_decimal "${HERDR_SWARM_PORT_SPAN:-10}" + _port_decimal "$(_port_setting "${HERDR_SWARM_PORT_SPAN:-}" swarm.portSpan 10)" +} + +# _port_setting : the exported variable +# wins; else the repo's own git config, which is per repo and — unlike the +# environment — reaches an action-invoked fan-out pane; else the default. +_port_setting() { + if [ -n "${1-}" ]; then + printf '%s\n' "$1" + return 0 + fi + git -C "${SWARM_REPO:-.}" config --get "$2" 2>/dev/null || printf '%s\n' "$3" } # _port_decimal : digits only, read as DECIMAL (10#) — bash would read diff --git a/tests/compare.test.mjs b/tests/compare.test.mjs index c428fa8..31c9d33 100644 --- a/tests/compare.test.mjs +++ b/tests/compare.test.mjs @@ -88,14 +88,47 @@ test("settled slots are listed unranked and kept out of overlaps; archived slots test("a validation file that is not Swarm's record for this run and slot counts as not run", () => { const run = makeFannedOutRun(h, { prefix: "cm" }); const tip = commitIn(run.wt(1), "x.js"); - for (const forged of [{ run_id: "other" }, { slot: 2 }, { kind: "something-else" }, { status: "maybe" }]) { - fs.writeFileSync(path.join(run.sdir, `validation-${run.runId}-s1.json`), JSON.stringify({ - schema_version: 1, kind: "herdr-swarm-validation", run_id: run.runId, slot: 1, head_sha: tip, status: "passed", checks: [], ...forged, - })); + const write = (forged) => fs.writeFileSync(path.join(run.sdir, `validation-${run.runId}-s1.json`), JSON.stringify({ + schema_version: 1, kind: "herdr-swarm-validation", run_id: run.runId, slot: 1, head_sha: tip, status: "passed", + checks: [{ name: "validate", status: "passed" }], ...forged, + })); + write({}); + assert.equal(compare(run).slots[0].validation.status, "passed", "the genuine record ranks — each case below differs by one field"); + for (const forged of [ + { run_id: "other" }, { slot: 2 }, { kind: "something-else" }, { status: "maybe" }, { schema_version: 2 }, + // The strict candidate gate rejects a record without the hook's own + // check; compare once ranked it "passed" anyway. + { checks: [{ name: "tests", status: "passed" }] }, + ]) { + write(forged); assert.equal(compare(run).slots[0].validation.status, "none", JSON.stringify(forged)); } }); +test("compare and the strict candidate gate share one identity rule", async () => { + const { isSwarmValidationRecord } = await import("../scripts/pr-handoff.mjs"); + const src = fs.readFileSync(new URL("../scripts/compare.mjs", import.meta.url), "utf8"); + assert.match(src, /import \{ isSwarmValidationRecord \} from "\.\/pr-handoff\.mjs"/); + assert.doesNotMatch(src, /"herdr-swarm-validation"/, "no second copy of the rule in compare"); + const ok = { schema_version: 1, kind: "herdr-swarm-validation", run_id: "r", slot: 1, checks: [{ name: "validate" }] }; + assert.equal(isSwarmValidationRecord(ok, "r", 1), true); + assert.equal(isSwarmValidationRecord(ok, "r", "1"), false, "slot is compared as a number"); +}); + +test("with checks equal, commits outrank finished, finished outranks neither, and diff size never decides", () => { + const run = makeFannedOutRun(h, { prefix: "cm", slots: 5 }); + commitIn(run.wt(2), "two.js"); + commitIn(run.wt(3), "big.js", "x\n".repeat(500)); // larger than slot 5's + commitIn(run.wt(5), "small.js", "y\n"); + setManifest(run, (d) => { + for (const i of [3, 4, 5]) d.slots[i - 1].finished = { at: "x", reason: "marker" }; + }); + // No validation anywhere: every slot ties on the first key. + const { slots } = compare(run); + assert.deepEqual(slots.map((s) => s.slot), [3, 5, 2, 4, 1], + "commits+finished (3 before 5 by slot, not by size), commits, finished, neither"); +}); + test("a row failing its ownership check is left out of compare, loudly", () => { const run = makeFannedOutRun(h, { prefix: "cm", slots: 2 }); setManifest(run, (d) => { d.slots[0].path = mkdtemp("hs-foreign-"); }); @@ -200,6 +233,61 @@ test("merge verb: an expected slot tip pins the exact SHA merged and refuses jun assert.equal(spawnSync("git", ["-C", run.repo, "merge-base", "--is-ancestor", tip, "main"]).status, 0); }); +test("merge winner refuses when the base moved after it was confirmed", async () => { + const run = makeFannedOutRun(h, { prefix: "cm", slots: 2 }); + commitIn(run.wt(1), "one.js"); + const winner = commitIn(run.wt(2), "two.js"); + finishAll(run, [1, 2]); + const r = pane(run); + await r.refresh(); + await keys(r, "w", "m", "2"); + const confirmedBase = r.phase.base; + const moved = commitIn(run.repo, "elsewhere.js"); // someone lands on main meanwhile + await keys(r, "y"); + assert.match(r.banner, new RegExp(`base moved since you confirmed \\(${confirmedBase.slice(0, 7)} -> ${moved.slice(0, 7)}\\)`)); + assert.equal(spawnSync("git", ["-C", run.repo, "merge-base", "--is-ancestor", winner, "main"]).status, 1, "nothing merged"); + assert.deepEqual(run.manifest().slots.map((s) => s.status), ["running", "running"], "nothing skipped"); +}); + +test("merge winner leaves a finished slot running when its work changed after compare", async () => { + const run = makeFannedOutRun(h, { prefix: "cm", slots: 4 }); + for (const i of [1, 2, 3, 4]) commitIn(run.wt(i), `f${i}.js`); + finishAll(run, [1, 2, 3, 4]); + const r = pane(run); + await r.refresh(); + await keys(r, "w", "m", "1"); + assert.deepEqual(r.phase.others, [2, 3, 4]); + // A `marker` finish is never cleared by settle: the operator gave slots 3 + // and 4 a follow-up, and changed work is the only trace of it. + commitIn(run.wt(3), "followup.js"); + fs.writeFileSync(path.join(run.wt(4), "f4.js"), "editing\n"); + await keys(r, "y"); + assert.equal(run.slotRow(2).status, "archived", "unchanged finished slot is still skipped"); + assert.deepEqual([3, 4].map((i) => run.slotRow(i).status), ["running", "running"]); + assert.ok(fs.existsSync(run.wt(3)) && fs.existsSync(run.wt(4)), "their worktrees were not touched"); + assert.match(r.banner, /slot 1 merged; skipped 2; still running \(agent not finished\): 3, 4/); +}); + +test("merge winner re-settles first: an exited agent restarted by hand is not skipped", async () => { + const run = makeFannedOutRun(h, { prefix: "cm", slots: 3 }); + for (const i of [1, 2, 3]) commitIn(run.wt(i), `f${i}.js`); + setManifest(run, (d) => { + d.slots[1].finished = { at: "x", reason: "marker" }; + d.slots[2].finished = { at: "x", reason: "exited" }; + d.slots[2].seen_running = true; + }); + Object.assign(run.env, { STUB_HERDR_VERSION: "0.8.2", STUB_PANE_FG: "busy" }); // slot 3's agent is back + const r = pane(run); + await r.refresh(); + await keys(r, "w", "m", "1"); + assert.deepEqual(r.phase.others, [2, 3], "compare still showed slot 3 finished"); + await keys(r, "y"); + assert.equal(run.slotRow(2).status, "archived"); + assert.equal(run.slotRow(3).status, "running"); + assert.equal(run.slotRow(3).finished ?? null, null, "settle cleared the reversible finish"); + assert.match(r.banner, /skipped 2; still running \(agent not finished\): 3/); +}); + test("merge winner leaves unfinished slots running and says so", async () => { const run = makeFannedOutRun(h, { prefix: "cm", slots: 3 }); commitIn(run.wt(1), "one.js"); @@ -252,6 +340,22 @@ test("the diff handoff drops inherited git routing", async () => { assert.deepEqual(env, { PATH: "/bin", GIT_PAGER: "less" }); }); +test("the renderer's git routing list is lib.sh's list, exactly", async () => { + const { GIT_ROUTING } = await import("../bin/renderer-harvest.mjs"); + const lib = fs.readFileSync(new URL("../scripts/lib.sh", import.meta.url), "utf8"); + const body = lib.match(/clear_git_routing_env\(\) \{[\s\S]*?for key in ([\s\S]*?); do/); + assert.ok(body, "clear_git_routing_env's variable list not found in lib.sh"); + const names = body[1].replace(/\\\n/g, " ").split(/\s+/).filter(Boolean); + assert.ok(names.length >= 10, `parsed ${names.length} names`); + assert.deepEqual([...GIT_ROUTING].sort(), names.sort()); +}); + +test("a validate timeout names the variable that sets validate's budget", async () => { + const r = new HarvestRenderer(h.freshEnv()); + assert.match(r.timeoutBanner("validate", 930_000), /HERDR_SWARM_VALIDATE_TIMEOUT raises the limit/); + assert.match(r.timeoutBanner("merge"), /HERDR_SWARM_STEP_TIMEOUT_MS raises the limit/); +}); + test("merge winner never skips anything when the merge does not land", async () => { const run = makeFannedOutRun(h, { prefix: "cm", slots: 2 }); commitIn(run.wt(1), "one.js"); diff --git a/tests/fanout.test.mjs b/tests/fanout.test.mjs index d5aafa2..6593dac 100644 --- a/tests/fanout.test.mjs +++ b/tests/fanout.test.mjs @@ -1424,6 +1424,57 @@ test("port range is configurable and refused when it does not fit", () => { } }); +test("a repo's git config sets its own port range; an exported variable still wins", () => { + { + const { repo, env } = setup(); + git(repo, "config", "swarm.portStart", "5200"); + git(repo, "config", "swarm.portSpan", "5"); + const r = runPane(lines(["2", "", "", "Task", ".", "", ""]), env, repo); + assert.equal(r.status, 0, `${r.stdout}\n${r.stderr}`); + assert.deepEqual(readManifest().slots.map((s) => [s.port_base, s.port_span]), [[5200, 5], [5205, 5]]); + } + { + const { repo, env } = setup({ HERDR_SWARM_PORT_START: "6000" }); + git(repo, "config", "swarm.portStart", "5200"); + const r = runPane(lines(["1", "", "Task", ".", ""]), env, repo); + assert.equal(r.status, 0, `${r.stdout}\n${r.stderr}`); + assert.equal(readManifest().slots[0].port_base, 6000); + } + { + const { repo, env } = setup(); + git(repo, "config", "swarm.portStart", "80"); + const r = runPane(lines(["1", "", "Task", ".", ""]), env, repo); + assert.notEqual(r.status, 0); + assert.match(r.stderr + r.stdout, /no valid port range/); + } +}); + +test("a timeout that is not whole seconds refuses fan-out before any worktree is created", () => { + for (const bad of [{ HERDR_SWARM_CLONE_TIMEOUT: "2m" }, { HERDR_SWARM_CLONE_TIMEOUT: "0" }, { HERDR_SWARM_SETUP_TIMEOUT: "abc" }]) { + const { repo, env } = setup(bad); + const r = runPane(lines(["1", "", "Task", ".", ""]), env, repo); + assert.notEqual(r.status, 0, JSON.stringify(bad)); + assert.match(r.stderr + r.stdout, new RegExp(`${Object.keys(bad)[0]} must be a positive whole number`)); + assert.doesNotMatch(log(), /worktree create/, `${JSON.stringify(bad)}: nothing was created`); + } +}); + +test("a timeout helper given a bad duration starts nothing", () => { + const dir = mkdtemp("hs-tmo-"); + for (const fn of ["with_timeout", "with_group_timeout"]) { + for (const bad of ["2m", "08x", "0", ""]) { + const flag = path.join(dir, `${fn}-${bad || "empty"}`); + const r = runLib(`${fn} "${bad}" touch "${flag}"`); + assert.equal(r.status, 2, `${fn} ${bad}`); + assert.match(r.stderr, /not a positive whole number of seconds/); + assert.equal(fs.existsSync(flag), false, `${fn} ${bad}: command never started`); + } + const ok = path.join(dir, `${fn}-ok`); + assert.equal(runLib(`${fn} 08 touch "${ok}"`).status, 0, `${fn}: 08 is eight seconds, not an octal error`); + assert.ok(fs.existsSync(ok)); + } +}); + test("a port range that only overflows for a later slot refuses before any worktree is created", () => { const { repo, env } = setup({ HERDR_SWARM_PORT_START: "65500", HERDR_SWARM_PORT_SPAN: "10" }); const r = runPane(lines(["4", "", "", "", "", "Task", ".", "", "", "", ""]), env, repo); diff --git a/tests/harvest.test.mjs b/tests/harvest.test.mjs index bbe7e1c..815cc04 100644 --- a/tests/harvest.test.mjs +++ b/tests/harvest.test.mjs @@ -1392,14 +1392,19 @@ test("no git-mutation strings in bin/ (destructive-git-in-scripts-only KTD)", () for (const re of forbidden) { assert.doesNotMatch(code, re, `bin/${f} contains mutation string ${re}`); } - // Every git invocation the renderer makes goes through this.git([...]) — - // its first argument must be a read-only subcommand. + // Every git invocation the renderer makes — through this.git([...]) or + // a direct spawn/exec of "git" — must name a read-only subcommand + // first, and a spawn of git whose argv this cannot read is refused. for (const m of code.matchAll(/\.git\(\s*\[\s*"([a-z-]+)"/g)) { assert.ok( readOnlyGit.has(m[1]), `bin/${f}: this.git(["${m[1]}", …]) is not read-only`, ); } + for (const m of code.matchAll(/(?:spawn|spawnSync|execFile|execFileSync|pExecFile)\(\s*"git"\s*,\s*(\[\s*"([a-z-]+)")?/g)) { + assert.ok(m[1], `bin/${f}: a direct git spawn must pass a literal argv whose first element is the subcommand`); + assert.ok(readOnlyGit.has(m[2]), `bin/${f}: direct git spawn of "${m[2]}" is not read-only`); + } } }); diff --git a/tests/validate.test.mjs b/tests/validate.test.mjs index c548c24..ca8f078 100644 --- a/tests/validate.test.mjs +++ b/tests/validate.test.mjs @@ -114,10 +114,10 @@ test("a hook that leaves untracked files is not recorded", () => { assert.equal(fs.existsSync(run.resultFile), false); }); -test("a commit landing during validation is drift, not a stale pass", () => { +test("a commit landing during validation is refused, not a stale pass — and not base drift", () => { const run = fixture("echo more > more.txt && git add more.txt && git commit -q -m more\n"); const r = run.step(); - assert.equal(r.status, 30); + assert.equal(r.status, 36, "30 means the BASE moved; the slot moving is a refusal"); assert.match(r.stderr, /moved from/); assert.equal(fs.existsSync(run.resultFile), false); }); @@ -212,9 +212,74 @@ test("a planted default-path file that is not Swarm's record for this run and sl assert.match(r.stdout, /"validation_status":"passed"/); }); -test("invalid timeout refuses", () => { - const run = fixture("true\n", { HERDR_SWARM_VALIDATE_TIMEOUT: "soon" }); - assert.equal(run.step().status, 36); +test("invalid timeout refuses before the hook runs", () => { + for (const bad of ["soon", "0", "000", "2m", "-5", "99999999"]) { + const run = fixture("true\n", { HERDR_SWARM_VALIDATE_TIMEOUT: bad }); + const r = run.step(); + assert.equal(r.status, 36, bad); + assert.match(r.stderr, /HERDR_SWARM_VALIDATE_TIMEOUT must be a positive/, bad); + assert.equal(fs.existsSync(path.join(run.sdir, `validation-${run.runId}-s1.log`)), false, `${bad}: hook never started`); + } +}); + +test("a timeout with leading zeros is decimal, not octal", () => { + for (const [given, secs] of [["0900", 900], ["08", 8]]) { + const run = fixture("true\n", { HERDR_SWARM_VALIDATE_TIMEOUT: given }); + assert.equal(validated(run.step()).status, "passed", given); + assert.equal(run.result().timeout_seconds, secs, given); + } +}); + +test("a run replaced while the hook runs discards the result", () => { + // The hook stands in for an abort-and-refan landing mid-validate. + const run = fixture(`node -e ' +const fs = require("fs"), f = process.env.HERDR_PLUGIN_STATE_DIR + "/run-w9.json"; +const d = JSON.parse(fs.readFileSync(f, "utf8")); d.run_id = "replaced"; d.slots = []; fs.writeFileSync(f, JSON.stringify(d));' +`); + const r = run.step(); + assert.equal(r.status, 36, r.stderr); + assert.match(r.stderr, /is no longer the live run — validation result discarded/); + assert.equal(fs.existsSync(run.resultFile), false, "no evidence written against a dead run"); +}); + +test("archive and discard refuse while a validate hook is still writing in the slot", async () => { + const marker = path.join(mkdtemp("hs-vbusy-"), "child.pid"); + const run = fixture(`sleep 30 &\necho $! > "${marker}"\nwait\n`); + const v = await startValidate(run, marker); + const mf = path.join(run.sdir, "run-w9.json"); + const d = JSON.parse(fs.readFileSync(mf, "utf8")); + d.slots[0].status = "skipped"; + fs.writeFileSync(mf, JSON.stringify(d)); + const ar = run.step("archive"); + assert.equal(ar.status, 36); + assert.match(ar.stderr, /being validated.*archive refused/); + assert.equal(run.step("snapshot").status, 0); + const dc = run.step("discard", { HERDR_SWARM_CONFIRM: run.branch(1) }); + assert.equal(dc.status, 36); + assert.match(dc.stderr, /being validated.*discard refused/); + assert.ok(fs.existsSync(run.wt(1)), "worktree untouched"); + v.child.kill("SIGTERM"); + await v.done; + // The inverse: a lock left by a dead validate never blocks archive. + const stale = path.join(run.sdir, `validating-${run.runId}-s1`); + fs.mkdirSync(stale); + fs.writeFileSync(path.join(stale, "pid"), "999999\n"); + const ok = run.step("archive"); + assert.equal(ok.status, 0, ok.stderr); + assert.equal(fs.existsSync(run.wt(1)), false); +}); + +test("abort keeps a slot whose validate hook is still running, and removes it once that holder is gone", () => { + const run = fixture("true\n"); + const lock = path.join(run.sdir, `validating-${run.runId}-s1`); + fs.mkdirSync(lock); + fs.writeFileSync(path.join(lock, "pid"), `${process.pid}\n`); // a live holder + const kept = h.runScript("abort.sh", [], run.env); + assert.match(kept.stderr, /slot 1 KEPT — a validate hook is still running/); + assert.ok(fs.existsSync(run.wt(1)), "worktree untouched"); + fs.writeFileSync(path.join(lock, "pid"), "999999\n"); // holder died + h.runScript("abort.sh", [], run.env); + assert.equal(fs.existsSync(run.wt(1)), false, "a dead holder blocks nothing"); }); test("harvest pane drives the real verb end to end", async () => {