feat(loop-swarm): implement multi-agent consensus sandboxing - #398
Conversation
cobusgreyling
left a comment
cobusgreyling
left a comment
There was a problem hiding this comment.
Summary
loop-swarm is a clean first cut of multi-agent consensus on top of loop-sandbox (parallel sandboxes → SHA-256 patch equality → majority gate), and exposing loop-sandbox via main/types is the right packaging step. However, the consensus logic treats total agent failure as success (exit 0), ignores exitCode entirely, and the sole test is fragile and too narrow to validate L3 safety claims. Dependency wiring also does not match the monorepo’s file: + lockfile pattern used by sibling tools, so test:loop-swarm is unlikely to work cleanly in a fresh checkout.
Issue counts by severity
- bugs: 5
- suggestions: 2
- nits: 1
Decision: request changes. Do not merge until: (1) all-failed / non-zero exit agents cannot count as consensus success, (2) monorepo deps use file:../loop-sandbox + lockfile, (3) test setup mirrors loop-sandbox git identity/default branch, (4) concurrent in-process runInSandbox signal/manifest races are addressed or serialized.
| // Group by hash | ||
| const patches = results.filter(r => r.patchFile !== null && r.hasChanges).map(r => r.patchFile as string); | ||
|
|
||
| if (patches.length === 0) { |
There was a problem hiding this comment.
[bug] When every agent produces no patch (patches.length === 0), runSwarm returns reached: true and the CLI exits 0. That path does not inspect SandboxResult.exitCode, so a swarm of crashed agents (non-zero exits, failed spawns, or failed patch extraction) is reported as “consensus on no changes” with majorityCount: count. For a tool marketed as an L3 safety net, total failure must not look like success.
Suggestion: Only treat the empty-patch case as consensus when every result has exitCode === 0 (and optionally !hasChanges). If any agent failed, return reached: false (or a distinct failure mode) and exit non-zero. Surface failed run ids/exit codes in the log and ConsensusResult.
| console.log(`\n🧠 Analyzing swarm results...`); | ||
|
|
||
| // Group by hash | ||
| const patches = results.filter(r => r.patchFile !== null && r.hasChanges).map(r => r.patchFile as string); |
There was a problem hiding this comment.
[bug] Consensus hashing only considers runs with patchFile !== null && hasChanges. Failed or no-op runs are dropped from the vote set, but the majority threshold is still Math.floor(count / 2) + 1 of launched agents. That is fine when documenting “majority of N”, but combined with Issue 1 it means failures are silent non-votes rather than hard errors. More importantly, patches from agents with non-zero exitCode still count as full votes, so a failing agent that still dirtied the tree can drive a winning consensus.
Suggestion: Define and document the voting set explicitly (e.g. only exitCode === 0 results vote; failures abort or count against consensus). Reject or quarantine patches from failed runs unless an opt-in flag allows them.
| const root = await mkdtemp(join(tmpdir(), 'loop-swarm-test-')); | ||
|
|
||
| try { | ||
| spawnSync('git', ['init'], { cwd: root }); |
There was a problem hiding this comment.
[bug] The test runs git init + git commit --allow-empty without setting user.name / user.email, and without git init -b main. Sibling loop-sandbox tests correctly configure identity and the default branch. On a clean CI/agent environment without global git identity, the empty commit fails, the sandbox has no valid base, and the test fails for setup reasons rather than product behavior.
Suggestion: Mirror tools/loop-sandbox/test/sandbox.test.mjs setup: git init -b main, git config user.name/email, then commit. Assert on result.status/result.stderr with a helpful message when non-zero.
| "publishConfig": { | ||
| "access": "public" | ||
| }, | ||
| "dependencies": { |
There was a problem hiding this comment.
[bug] Dependencies are declared as registry ranges (@cobusgreyling/loop-sandbox: ^1.0.0, and an unused @cobusgreyling/loop-worktree: ^1.2.0) with no package-lock.json. Sibling packages (e.g. loop-sandbox) lock local deps via file:../… so monorepo tests use in-tree builds. Without that, npm run test:loop-swarm / test:tools cannot resolve the newly exposed library entry from this PR until a published release exists, and will not exercise the local main/types change.
Suggestion: Depend on "@cobusgreyling/loop-sandbox": "file:../loop-sandbox", drop the unused direct loop-worktree dependency, run npm install in tools/loop-swarm, and commit package-lock.json consistent with other tools. Keep registry versions only for publish-time if needed via a release workflow.
| console.log(`\n🐝 Launching loop-swarm with ${count} concurrent agents...`); | ||
|
|
||
| const promises: Promise<SandboxResult>[] = []; | ||
| for (let i = 0; i < count; i++) { |
There was a problem hiding this comment.
[bug] runSwarm fans out N concurrent runInSandbox calls. Each runInSandbox registers process-global SIGINT/SIGTERM handlers that call cleanup() then process.exit(1). Concurrent registration means N handlers fire on one signal, race on cleanup/exit, and the API was clearly designed for single-flight CLI use—not in-process parallelism. Concurrent createWorktree also does unlocked read-modify-write of .loop-worktrees/manifest.json, so parallel swarm runs can drop sibling entries from the manifest (cleanup still targets absolute paths, but shared process state is unsafe).
Suggestion: Prefer spawning N loop-sandbox child processes (process isolation for signals) or extend loop-sandbox with a concurrency-safe mode (shared signal multiplexer, no process.exit from library code, serialized manifest updates). At minimum document that in-process concurrent runInSandbox is unsupported and serialize sandbox execution until fixed.
| try { | ||
| spawnSync('git', ['init'], { cwd: root }); | ||
| spawnSync('git', ['commit', '--allow-empty', '-m', 'init'], { cwd: root }); | ||
|
|
There was a problem hiding this comment.
[suggestion] Coverage only checks the happy path (deterministic identical patches, count=2). There are no tests for: consensus failure on divergent patches, empty/no-change success vs all-failed agents, majority threshold (e.g. 2-of-3), presence of consensus.patch, or CLI exit codes on failure. Given the L3 safety positioning, these are the behaviors that matter most.
Suggestion: Add unit tests that mock runInSandbox (or inject a seam) for hash majority/minority and exit-code handling, plus one integration test that asserts .loop-sandbox/patches/consensus.patch exists and applies cleanly.
|
|
||
| `loop-swarm` runs an agent command multiple times concurrently in separate, isolated `loop-sandbox` worktrees. It then extracts the resulting `.patch` files from each run, hashes them, and automatically determines if a majority consensus was reached. | ||
|
|
||
| If an agent produces non-deterministic results, `loop-swarm` acts as an L3 safety net by ensuring that only changes verified by multiple parallel agent runs are proposed. |
There was a problem hiding this comment.
[suggestion] README claims an “L3 safety net” but does not link docs/safety.md, document failure modes, exit codes, or the exact majority rule (including how failed/no-change agents count). The PR checklist asserts safety docs are referenced; the shipped README does not.
Suggestion: Link docs/safety.md, document exit codes (0 = consensus including intentional no-op; non-zero = no consensus or infra failure), majority formula, and limitations (byte-identical patches only; worktree isolation is not OS sandboxing; stdio is shared across concurrent agents).
| allowPositionals: true | ||
| }); | ||
|
|
||
| if (values.help || positionals.length === 0 || positionals[0] !== 'run') { |
There was a problem hiding this comment.
[nit] Missing subcommand / wrong first positional (positionals[0] !== 'run') prints help and process.exit(0), same as intentional --help. That makes misuse look successful in scripts.
Suggestion: Exit 0 only for --help; exit 1 (or 2) for missing/invalid subcommand after printing usage.
|
Please review the changes. |
cobusgreyling
left a comment
cobusgreyling
left a comment
There was a problem hiding this comment.
Re-review (post-fix 7798656)
Good progress on the earlier blocking bugs. Several items are fixed; a few remain before merge.
Fixed (thank you)
- All-failed / empty consensus as success — failed runs (
exitCode !== 0) are excluded; all-failed falls through toreached: falsewith a non-zero CLI exit - Serialization — sequential
runInSandboxavoids process-global SIGINT races and concurrent worktree create - Git test setup —
git init -b main+user.name/user.email - Deps —
file:../loop-sandbox(no unusedloop-worktree);main/typesexposed onloop-sandbox - CLI misuse exit — missing/
runmismatch exits1;--helpexits0 - Safety docs — README links
docs/safety.md, documents exit codes and majority rule - Tests — happy path, divergent patches, all-failed agents
Remaining (must fix)
-
[bug] Signal handlers still call
process.exit(1)inside eachrunInSandbox
Sequential calls remove listeners infinally, so this is better than N concurrent handlers — but a SIGINT during agent k still exits the entireloop-swarmprocess via sandbox's handler rather than a swarm-owned cleanup path. More importantly: ifrunInSandboxever leaves a listener attached on an early throw before itstry(e.g. lock acquire failures in a future integration with #399), swarm inherits that. Acceptable for v1 only if documented; otherwise swarm should drive isolation via CLI subprocesses (loop-sandbox runas child) so signal ownership stays per-agent. -
[bug] Help / product copy still says “parallel”
CLI help:Number of parallel agents to spawnwhile implementation is intentionally sequential. README title path still markets “parallel agent runs” in places. Align all user-facing strings with sequential execution (or truly parallelize only after worktree/signal isolation is solved). -
[suggestion → required for L3 claim] Missing majority / mixed-outcome tests
Still no coverage for: 2-of-3 identical patches (majority with one divergent), exit-code-0 no-op majority, and CLI exit codes on consensus failure vs success. The all-failed + happy + divergent trio is a good start; L3 safety claims need the majority threshold asserted.
Nits (non-blocking)
- Commit/package-lock: confirm
npm testpasses from a clean monorepo root viatest:tools(file: dep + lockfile) - Consider naming: “swarm” implies concurrent; “sequential consensus” in the one-liner reduces surprise
Decision: request changes. Fix (2) and add at least the 2-of-3 majority test from (3). (1) can be a documented limitation in README for this PR if you prefer not to subprocess yet — but call it out under Limitations explicitly.
Triage status (2026-07-27)Maintainer triage cycle complete for this PR. Status: changes requested (see formal review above). Once the parallel→sequential copy is fixed and at least a 2-of-3 majority test is added (plus README limitation for sandbox signal ownership if you keep in-process Thanks for the solid follow-up on the earlier blocking bugs. |
|
Please review the changes. |
cobusgreyling
left a comment
cobusgreyling
left a comment
There was a problem hiding this comment.
Re-review (post-fix 4751d82)
Remaining items from the previous re-review are addressed:
- Parallel → sequential copy — CLI help, README narrative, and how-it-works all say sequential; L3 claim no longer markets parallel runs.
- Majority / mixed-outcome tests —
2-of-3 majority with 1 divergentandexit-code-0 no-op majorityadded; CLI exit codes asserted. - SIGINT limitation — documented under README Limitations as an accepted v1 constraint (in-process sandbox handler).
Also good: failed agents disqualified, monorepo file: dep + root test:loop-swarm, safety.md link, loop-sandbox main/types export for library use.
Merge. Thanks @THRISHAL12345 — solid iteration on the feedback.
2 participants
Summary
Introduces
loop-swarm, a new CLI tool that runs agents concurrently in isolated sandboxes and mathematically determines a majority consensus patch to provide L3 safety guarantees.Changes
templates/pattern-template.md+ updatedregistry.yaml)loop-swarmcreated,loop-sandboxAPI exposed)Checklist (from CONTRIBUTING)
STATE.md*examples use.examplesuffix (N/A)docs/safety.mdnode tools/loop-audit/dist/cli.js .(or on the starter) and addressed findingsTesting / Dogfood
loop-auditpasses on affected starters or this repotest/swarm.test.mjsverifying deterministic consensus generationloop-swarminto the monorepo roottest:toolsflow.Screenshots / Examples (if UI or command output)
$ npx @cobusgreyling/loop-swarm run --count 3 -- npx my-agent run 🐝 Launching loop-swarm with 3 concurrent agents... 📦 Creating ephemeral worktree isolation: sandbox-1a2b3c 📦 Creating ephemeral worktree isolation: sandbox-4d5e6f 📦 Creating ephemeral worktree isolation: sandbox-7g8h9i 🧠 Analyzing swarm results... ✅ Consensus reached! 3/3 agents produced the exact same patch. 🎉 Consensus patch saved to: .loop-sandbox/patches/consensus.patch