docs(agents): wave coordination structure design#1
Conversation
…her-parsed flags The Command contract (core) gains declared flags/positionals specs; the dispatcher parses them strictly (parseCommandArgs), generates per-command help, usage errors, and flag-name completion, and hands run() ParsedArgs instead of raw argv. All 26 commands across cli/manager/delivery migrate; per-command parseArgs is gone. - workspace: the shared target flag bundle (--space/--server/--creds) — commands spread it so the target vocabulary can't drift; provenance reporter (→ using / → wrote lines) wired into spawn (persona resolution + creds write). - completion: a '-' prefixed word now completes against the command's declared flags, generated from the specs. - deliberate tightenings (documented in the flag-inventory smoke): commands whose positionals were accepted-but-ignored now reject them; manager commands accept exactly their own flags instead of the union (dead --drive dropped); feedback keeps its own dual-mode parsing (rawArgs) until the intake server splits out. - smokes: command-kernel (parse/usage/flag-completion semantics, pure) and flag-inventory (golden per-command flag parity at the composition root), both wired into smoke:ci.
… intercept feedback --help previously fell through to its internal parse and printed a red unknown-option line above the help (review finding). The dispatcher's help intercept now keys on the __ prefix, not rawArgs — __complete keeps its verbatim argv (a --help there is a word being completed), feedback gets real help.
…rbs start; control clients move into the CLI Stage 2a of the CLI rework (plan: .internal/plans/cli-rework.md). - workspace: the shared launch bundle (launchFlags) + individual spaceFlag/ serverFlag/credsFlag exports; core: FlagValues<> derives a command's values type from its as-const specs, so the cast can't disagree with the parser. - spawn: --detach/-d launches via the manager on the SAME flag set as the foreground path (one spawnFlags list — the old spawn/start ability drift is structurally impossible). Foreground gains --model and --cwd; --creds is --detach-only (control-caller cred) and fails loud otherwise. - manager: the start op accepts the full grammar — prompt, subscribe/allow* overrides (flags > persona file, foreground precedence), shareTools narrowing; a manifest launch (resolved) rejects imperative overrides. cotal start is a tombstone naming the replacement — never a silent alias. - stop/ps/attach move into @cotal-ai/cli as thin control clients (manager keeps only supervise); resolveManagerTarget's duplicated plumbing collapses onto lib/connect.ts; attach-client.ts moves with them and the ws attach contract is pinned in docs/architecture.md. - smokes: start-overrides (manager threading), launch-parity (grammar ⊆ start-op ⊆ MCP cotal_spawn, tier-safe by-test enforcement), and spawn-detach-live — a real-broker/real-manager e2e of the merged grammar, attach ws contract, tombstone, and fail-loud guards; all wired into the gates. Docs updated (architecture, claude-code-integration).
… — never silently dropped Review finding 1 (stage 2a), fixed the full way (option i): the start op gains `identity` — the presence-name override, separate from the persona REF — and the manager applies it with foreground precedence (identity ?? file name:), minted into the creds and threaded through COTAL_NAME so presence and credential can't diverge. A manifest launch (resolved) rejects it like the other imperative overrides. spawnDetached mirrors foreground ref precedence exactly (--config > positional > --name > default) and sends identity separately; the launch-parity golden maps --name → identity. Also per review: e2e extended to assert allow-subscribe/allow-publish/model end-to-end plus --cwd proven by the real child reporting its working directory (20 checks); start-overrides covers identity override, auto-number series, and resolved+identity rejection; splitFlag hoisted; the transcript tri-state now flows down instead of being re-derived.
…olver smoke:server-resolution:live (CI-only — not in local `pnpm check`, which is why it escaped) imported resolveManagerTarget from the manager's deleted commands surface. The resolver moved to the CLI in this PR; the smoke moves with it (tier rule: a manager smoke can't import @cotal-ai/cli) and drives resolveControlTarget with the explicit privileged profile. 15/15 locally.
…Ctrl-] Carries Cotal-AI#172 (COTAL_DETACH_KEY) through the stage-2a move of attach-client into the CLI: the 'attached to <name>' hint now reads detachKey().label so an overridden key is what the operator is told.
The rebase onto main brought presence-based launch readiness (Cotal-AI#159 Part B): startAgent now resolves 'started' only when the assigned id joins the roster. The fake ep gains on/off + getRoster reporting every managed agent joined — the same fake manifest-launch.smoke.ts uses — so the three spawn-succeeds checks pass again.
…al-AI#159 B1) The rebase onto main brought presence-based launch readiness: the manager replies to a start only on a REAL outcome — the agent joins, the process exits, or the ~30s backstop. The CLI's one-shot control request used the 5s op default, so every detached spawn of a real (slow-booting) agent died client-side with 'no manager reachable (timeout)' while the launch proceeded. askManager gains a timeout parameter; the start op passes 40s and prints a dim waiting line. stop/ps/attach keep the 5s default. The e2e's fixture child becomes a REAL mesh endpoint (joins presence under the manager-assigned id via the connector env) instead of a bare keepalive — under readiness a non-joining child is honestly 'uncertain', not started. NOTE: main's own start client (pre-rework commands.ts) and the MCP cotal_spawn tool share this 5s hole against a #159B manager — flagged for a follow-up outside this train.
Cotal-AI#172 (detach key) added imports of ../src/attach-client.js to the attach smokes after stage 2a was cut; the module moved to cli/src/lib in 2a, so the rebased imports dangled (ERR_MODULE_NOT_FOUND in the tmux runtime CI step — outside pnpm check, which is why the local gate stayed green). Dev-only relative smoke imports; the smokes still drive the manager's real ws server.
…ake splits out Stage 2b of the CLI rework (plan: .internal/plans/cli-rework.md). - setup: configure-only and state-independent — checks Node + locates nats-server, installs the Claude plugin, seeds personas, and LAUNCHES NOTHING (no mesh/web/manager/delivery/cmux/tmux/demo side boots). Every write is announced with a provenance line; repeat runs print a read-only status card naming the exact command for anything that's down. The --auth/--open flags moved to their real home (cotal up). - go: deleted (pure alias); errors as an unknown command. - up: owns the full local stack — broker, then the control plane (delivery daemon → detached manager via ensureControlPlane), so spawn --detach / cotal_spawn work right after up with no setup side effects. down already tears all of it down. - feedback: client-only with declared flags (real --help); the --keys intake HTTP server is the new `feedback-intake` command in @cotal-ai/delivery (tier-clean, byte-identical move). - help: regrouped by user intent (Setup · Mesh · Messaging · Agents · Observe), summaries slimmed now that generated help carries the detail. - pruned the orphaned session/launch machinery (ensureWeb, startWebDetached, cmux/tmux process detectors). - smokes: setup-pure-live — a 16-check subprocess e2e pinning state-independence (exit 0 with nothing running, launches nothing, writes + provenance, status card, removed surface fails loud); golden inventory updated (go out, feedback-intake in; still 26 commands). - docs: setup-internals, getting-started, architecture, claude-code-integration, manifest, README swept to the new split.
…ne degradation is announced Review findings (stage 2b): 1. Foreground `cotal up` orphaned the detached manager on Ctrl-C — the SIGINT/exit handlers stopped only delivery, leaving the manager reconnect-looping against a dead broker (the documented orphan-supervisor failure mode). Both handlers now call stopManager() alongside stopDelivery(), symmetric pidfile kills. 2. ensureControlPlane's non-fatal catch is no longer silent: one dim line names what degraded and the manual recovery (cotal supervise) — a swallowed manager start otherwise surfaces later as an unexplained 'no manager reachable' on spawn --detach. 3. Status card no longer claims spawns start the manager (they don't): 'start: cotal up, or: cotal supervise'. 4. Comment nits: dead cmuxManagerRunning reference dropped; the --auth/--open note no longer overstates a 'move' (--open already lived on up; --auth died with the launch behavior).
Since up brings the manager up with the broker (setup is configure-only), the manifest path started a SECOND supervise for the launch spec. Exactly one manager serves a space (the singleton lease), so the two raced: if the launch manager lost, the agents never booted while the CLI printed '✓ launching N agent(s)'; if the plain one lost, its pid file had already been overwritten and 'cotal down' could no longer stop it (the orphan-supervisor mode). The resolved launch spec is now written first and rides through ensureControlPlane to the one manager startMeshDetached starts (DetachOpts runtime/launch → ensureManager). upManifest also stops any leftover detached manager first — a stale one would win the fresh mesh's lease and the launch manager would refuse. Live e2e: up-manifest-live now asserts exactly one supervise (launch-carrying, alive, gone after down) — red on the old code, green now; new up-stack-live proves the documented 'up --detach starts mesh + delivery + manager' claim as real usage (real binary, JWT-authed broker, real 'cotal ps', clean down). Both wired into pnpm check + CI.
The SIGTERM'd manager/daemon shut down gracefully; on slow CI that exceeds the fixed 1s settle, flaking the no-orphan assertions. Poll up to 12s instead.
startMeshDetached now reports whether the control plane came up; a degraded control plane (already announced) turns the '✓ launching N agent(s)' line into an explicit '✗ NOT launched' — the two lines can no longer contradict.
…d/remove/list
Extensions install into a cotal-owned npm prefix ($XDG_CONFIG_HOME/cotal/extensions),
never the user's project. `ext add` imports the package once, verifies its commands
actually landed in THIS CLI's registry (core must be a peerDependency; the prefix's
core is linked to the running binary's copy so there is one registry singleton),
pins name@version, and caches the command surface into extensions.json.
From then on help/<TAB> read the cache (no import); dispatch verifies the version
pin and imports lazily, parsing with LIVE specs — the cache is display-only.
Failures are loud and adds roll back: builtin-name collision (builtin wins),
core as a regular dependency, missing peerDep, zero registrations, version skew,
broken import at run time (names the package, prescribes re-add).
Only the published binary opts in (runCli { extensions: true }); library
composition roots keep the explicit-import model.
e2e: bin/smoke/ext-live.smoke.ts drives the real binary as subprocesses in a
sandboxed prefix through the full lifecycle (20 checks), wired into smoke:ci.
…c→name; corrupt manifest is one red line Review findings on Cotal-AI#175: - An installed sibling extension is invisible to the registry during add (it is never imported), so contributed names are now also checked against the manifest cache — a duplicate fails the add naming BOTH extensions, and the overlay warning distinguishes builtin-vs-ext from ext-vs-ext (first wins). - The installed package NAME is resolved from the spec itself (path spec reads its package.json; registry spec carries the name; git/tarball URLs refused) instead of diffing prefix dependencies after the fact — the heuristic could bind to the wrong key on a drifted prefix, and made re-adding an installed extension impossible. npm's --json gives only counts, so the reviewer's suggested added[].name does not exist on npm 11. - A corrupt extensions.json still fails every invocation (a manifest error must never silently shrink the surface) but renders as the CLI's one red line instead of an unhandled-rejection stack dump. ext-live grows to 27 checks: re-add refresh, corrupt-manifest rendering, ext-vs-ext collision + rollback. One docs line on binary moves landing loud.
…ut as extension packages The dashboard and the demo trace generator leave the built-in surface (25 commands) and become the first REAL extensions: implementations/web (npm cotal-web, superseding the reserved placeholder) and implementations/demo (@cotal-ai/demo, private dev aid). Each self-registers its command on import; installed via cotal ext add, by path or (web, once published) registry name. Dogfooding immediately forced one contract generalization (reviewer notified): a real extension can't live on core alone — web needs @cotal-ai/workspace — and npm can't install workspace:* deps from a path spec, while peers are ignored at install. So: every shared @cotal-ai/* package must be a peerDependency, and ext add links EACH declared @cotal-ai/* peer to the running binary's copy (the existing core link, generalized; core stays mandatory). @cotal-ai/* as a regular dependency fails the add; a peer the binary doesn't carry fails loud. The shared workstation helpers the new surfaces need — connectOrExit and friends, resolveSpace, the ANSI palette — move from the CLI into @cotal-ai/workspace's documented charter, with re-export shims so existing importers and smokes are untouched. Setup's ready-card is extension-aware: 'web not installed — add: cotal ext add cotal-web'. e2e: bin/smoke/dogfood-live.smoke.ts (22 checks) drives the FULL operator journey with the real packages — unknown command → ext add (both peer links proven by provenance) → up --detach (JWT) → the dashboard serving /, /app.js and /api/meta over HTTP → demo trace on an open mesh verified in real channel history → remove → unknown again. ext-live grows to 32 checks (multi-peer link + both new refusals). Docs swept; reserved/cotal-web removed.
…rder Modules self-register on import and tsx's entry interop doesn't guarantee evaluation order across the composition root's imports — the smaller daemon graphs could finish first and put 'Manager' above 'Setup' in --help (dev-only; the built ESM binary ordered correctly, i.e. dev and prod help disagreed). help() now ranks its groups explicitly (Setup → Mesh → Messaging → Agents → Observe → Extensions → Manager); unknown groups follow in first-seen order.
feat: declarative command kernel — stage 1 of the CLI rework
feat: one launch grammar — spawn --detach absorbs start (stage 2a)
feat: setup is configure-only; go is gone; feedback intake splits out (stage 2b)
feat(cli,workspace): operator-installed CLI extensions — cotal ext (stage 3)
feat(cli,workspace,web,demo): dogfood cotal ext — web and demo move out as extensions (stage 4)
… personas Design record for Matt's multi-agent wave: a two-channel layout (#general announcements, #coordination lateral detail), hub-and-spoke control plane where all task-routing + spawns funnel through a generic supervisor persona, mapped onto Cotal auth mode (enforced) and open mode (convention). Frozen on merge. Co-Authored-By: seal <noreply@sealedsecurity.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds typed command parsing, extension loading, detached agent control commands, web/demo extraction, configure-only setup/up orchestration, feedback intake splitting, and a larger set of smoke tests and docs/script updates. ChangesCLI kernel, extensions, and lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant ExtensionLoader
participant Registry
participant Manager
User->>CLI: cotal ext add cotal-web
CLI->>ExtensionLoader: overlayExtensions(registry)
ExtensionLoader->>Registry: import & cache commands
User->>CLI: cotal spawn --detach
CLI->>Manager: askManager("start", overrides)
Manager-->>CLI: ControlReply { ws }
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR bundles a frozen design record for the multi-agent wave coordination structure (two channels, hub-and-spoke control plane, persona/capability matrix, auth-enforcement split) together with a substantial source refactoring across 90 files. The code changes restructure CLI flag parsing around a shared
Confidence Score: 5/5Safe to merge; all issues found are non-blocking style and minor security hardening opportunities in the new feedback daemon. The refactoring is well-structured: flag parsing is centralised, identity/ACL overrides are correctly guarded against manifest launches, the orphan-manager teardown is properly wired into both the signal and exit handlers, and the extension loader verifies version pins before importing. The only code-level findings are in the new feedback-intake.ts: renderFeedback is called twice redundantly, and safeEqual uses an early length-mismatch return before timingSafeEqual, which leaks a minor timing signal. Neither affects correctness or represents a blocking concern for merge. implementations/delivery/src/feedback-intake.ts — the redundant renderFeedback call and the safeEqual length-leak before constant-time comparison. Important Files Changed
Reviews (2): Last reviewed commit: "docs(agents): harden replay + bring-up c..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| `cotal supervise`; cmux/tmux are opt-in; a zellij runtime is a **separate** design record). | ||
| - **Deliverables of this plan:** config files + persona files + a runbook README. No Cotal | ||
| source changes; every mechanism used below exists in this repo today and is cited as such. |
There was a problem hiding this comment.
The document header and the PR description both state "No Cotal source changes; every mechanism cited below exists in this repo today and is cited as such." However, this PR includes 4 300+ lines of source insertions across manager.ts, commands.ts, manager-proc.ts, feedback-intake.ts, ext-loader.ts, several new packages (implementations/demo, implementations/web), and more.
Because the design document is meant to be "frozen on merge" and the scope line implies it is purely a design record with no implementation changes, the heading is misleading. At minimum, the header's deliverables statement should reflect that significant code refactoring is bundled with this record.
Cite the recall/replay guard pair (agent.ts:434-443 — the "recall must not become a history bypass" doc-comment + the focus-gate return) so the no-backfill claim carries both contract and enforcing code; add the parseDuration ref for "24h" (channels.ts:59-60) and the exact nix activation anchors for the copy step. Co-Authored-By: seal <noreply@sealedsecurity.com>
|
Note for review + merge: the supervisor operator (running the wave now) flagged that once this record is frozen, it's the routing contract they execute against — so if the persona/capability cred scopes shift during review (especially who holds |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="implementations/cli/src/ext-loader.ts">
<violation number="1" location="implementations/cli/src/ext-loader.ts:113">
P2: Entrypoint resolution logic for extension packages is duplicated in `commands/ext.ts` (add-time `importExtension`) and `ext-loader.ts` (runtime `materializeExtensionCommand`). Both blocks perform the same manual `package.json` `main` → `exports["."]` string → `exports["."]` object → `exports` string fallback sequence. This creates drift risk: a future fix for a new `exports` shape that only updates one path will cause add-time and run-time behavior to diverge in ways that are hard to diagnose. Extracting a shared `resolvePackageEntry(dir)` helper would remove this risk and make future `exports` changes a single-point update.</violation>
</file>
<file name="implementations/manager/src/manager.ts">
<violation number="1" location="implementations/manager/src/manager.ts:577">
P2: Empty-string `identity` and `prompt` are silently dropped in `opStart` due to truthy ternary checks, which silently falls back to the persona file's `name:` (for identity) or no prompt (for prompt). This is inconsistent with the explicit "no fallbacks" validation style used for `resume` and the ACL override arrays. An explicitly provided empty string for `identity` would be caught downstream by `nameError`, so dropping it upstream hides a caller-side bug. Consider treating these like `resume` — either validating explicitly or preserving the value so downstream validation can reject it.</violation>
</file>
<file name="implementations/web/package.json">
<violation number="1" location="implementations/web/package.json:22">
P2: The `build` script relies on POSIX-only commands (`rm -rf`, `cp -R`) and will fail on Windows environments that lack WSL, Git Bash, or GNU coreutils. Since this package is meant to be installed as a cross-platform extension, using non-portable shell utilities in `package.json` scripts blocks Windows contributors and CI runners from building or publishing it. Consider replacing them with a cross-platform alternative—either Node.js built-in `fs.rmSync`/`fs.cpSync` (available in the Node v20 runtime this repo targets), a devDependency like `rimraf`, or a small Node build script.</violation>
</file>
<file name="implementations/manager/smoke/attach.smoke.ts">
<violation number="1" location="implementations/manager/smoke/attach.smoke.ts:12">
P2: The `detachKey` import reaches across package boundaries into `@cotal-ai/cli` internals (`src/lib/attach-client.js`) rather than using a stable public export. This bypasses the CLI's `exports` contract and is fragile if the CLI reorganizes its source tree. It also won't work in package-isolated workflows because the manager package does not declare `@cotal-ai/cli` as a dependency. Consider exporting `detachKey` from `@cotal-ai/cli`'s public entry point, or adding `@cotal-ai/cli` as a devDependency and importing through its declared export map.</violation>
</file>
<file name="implementations/cli/src/commands/spawn.ts">
<violation number="1" location="implementations/cli/src/commands/spawn.ts:176">
P2: Detached mode forwards `cwd` as raw user input while foreground mode normalizes it with `resolvePath(values.cwd)`. This breaks the documented launch-grammar parity — a relative `--cwd` will resolve against the manager process's working directory in detached mode, but against the invoking shell's cwd in foreground mode, causing inconsistent file access between the two modes. Normalize `cwd` with `resolvePath` before sending it in the `askManager` call to preserve parity.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| ); | ||
| } | ||
| const dir = extensionPackageDir(pkg); | ||
| const meta = JSON.parse(readFileSync(join(dir, "package.json"), "utf8")) as { main?: string; exports?: unknown }; |
There was a problem hiding this comment.
P2: Entrypoint resolution logic for extension packages is duplicated in commands/ext.ts (add-time importExtension) and ext-loader.ts (runtime materializeExtensionCommand). Both blocks perform the same manual package.json main → exports["."] string → exports["."] object → exports string fallback sequence. This creates drift risk: a future fix for a new exports shape that only updates one path will cause add-time and run-time behavior to diverge in ways that are hard to diagnose. Extracting a shared resolvePackageEntry(dir) helper would remove this risk and make future exports changes a single-point update.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At implementations/cli/src/ext-loader.ts, line 113:
<comment>Entrypoint resolution logic for extension packages is duplicated in `commands/ext.ts` (add-time `importExtension`) and `ext-loader.ts` (runtime `materializeExtensionCommand`). Both blocks perform the same manual `package.json` `main` → `exports["."]` string → `exports["."]` object → `exports` string fallback sequence. This creates drift risk: a future fix for a new `exports` shape that only updates one path will cause add-time and run-time behavior to diverge in ways that are hard to diagnose. Extracting a shared `resolvePackageEntry(dir)` helper would remove this risk and make future `exports` changes a single-point update.</comment>
<file context>
@@ -0,0 +1,131 @@
+ );
+ }
+ const dir = extensionPackageDir(pkg);
+ const meta = JSON.parse(readFileSync(join(dir, "package.json"), "utf8")) as { main?: string; exports?: unknown };
+ let entry = meta.main ?? "index.js";
+ const dot = (meta.exports as Record<string, unknown> | undefined)?.["."];
</file context>
| agent: args.agent ? String(args.agent) : undefined, | ||
| role: args.role ? String(args.role) : undefined, | ||
| config: args.config ? String(args.config) : undefined, | ||
| identity: args.identity ? String(args.identity) : undefined, |
There was a problem hiding this comment.
P2: Empty-string identity and prompt are silently dropped in opStart due to truthy ternary checks, which silently falls back to the persona file's name: (for identity) or no prompt (for prompt). This is inconsistent with the explicit "no fallbacks" validation style used for resume and the ACL override arrays. An explicitly provided empty string for identity would be caught downstream by nameError, so dropping it upstream hides a caller-side bug. Consider treating these like resume — either validating explicitly or preserving the value so downstream validation can reject it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At implementations/manager/src/manager.ts, line 577:
<comment>Empty-string `identity` and `prompt` are silently dropped in `opStart` due to truthy ternary checks, which silently falls back to the persona file's `name:` (for identity) or no prompt (for prompt). This is inconsistent with the explicit "no fallbacks" validation style used for `resume` and the ACL override arrays. An explicitly provided empty string for `identity` would be caught downstream by `nameError`, so dropping it upstream hides a caller-side bug. Consider treating these like `resume` — either validating explicitly or preserving the value so downstream validation can reject it.</comment>
<file context>
@@ -534,16 +552,38 @@ export class Manager {
agent: args.agent ? String(args.agent) : undefined,
role: args.role ? String(args.role) : undefined,
config: args.config ? String(args.config) : undefined,
+ identity: args.identity ? String(args.identity) : undefined,
model: args.model ? String(args.model) : undefined,
resume: args.resume ? String(args.resume) : undefined,
</file context>
| }, | ||
| "scripts": { | ||
| "typecheck": "tsc -p tsconfig.json --noEmit", | ||
| "build": "tsc -p tsconfig.json && rm -rf dist/web && cp -R src/web dist/web", |
There was a problem hiding this comment.
P2: The build script relies on POSIX-only commands (rm -rf, cp -R) and will fail on Windows environments that lack WSL, Git Bash, or GNU coreutils. Since this package is meant to be installed as a cross-platform extension, using non-portable shell utilities in package.json scripts blocks Windows contributors and CI runners from building or publishing it. Consider replacing them with a cross-platform alternative—either Node.js built-in fs.rmSync/fs.cpSync (available in the Node v20 runtime this repo targets), a devDependency like rimraf, or a small Node build script.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At implementations/web/package.json, line 22:
<comment>The `build` script relies on POSIX-only commands (`rm -rf`, `cp -R`) and will fail on Windows environments that lack WSL, Git Bash, or GNU coreutils. Since this package is meant to be installed as a cross-platform extension, using non-portable shell utilities in `package.json` scripts blocks Windows contributors and CI runners from building or publishing it. Consider replacing them with a cross-platform alternative—either Node.js built-in `fs.rmSync`/`fs.cpSync` (available in the Node v20 runtime this repo targets), a devDependency like `rimraf`, or a small Node build script.</comment>
<file context>
@@ -0,0 +1,39 @@
+ },
+ "scripts": {
+ "typecheck": "tsc -p tsconfig.json --noEmit",
+ "build": "tsc -p tsconfig.json && rm -rf dist/web && cp -R src/web dist/web",
+ "prepublishOnly": "pnpm run build"
+ },
</file context>
| "build": "tsc -p tsconfig.json && rm -rf dist/web && cp -R src/web dist/web", | |
| "build": "tsc -p tsconfig.json && node --input-type=module --eval \"import { rmSync, cpSync } from 'node:fs'; rmSync('dist/web', { recursive: true, force: true }); cpSync('src/web', 'dist/web', { recursive: true });\"", |
| import { execFileSync } from "node:child_process"; | ||
| import { createRuntime } from "../src/index.js"; | ||
| import { detachKey } from "../src/attach-client.js"; | ||
| import { detachKey } from "../../cli/src/lib/attach-client.js"; // the operator ws client moved into @cotal-ai/cli (stage 2a); dev-only smoke import |
There was a problem hiding this comment.
P2: The detachKey import reaches across package boundaries into @cotal-ai/cli internals (src/lib/attach-client.js) rather than using a stable public export. This bypasses the CLI's exports contract and is fragile if the CLI reorganizes its source tree. It also won't work in package-isolated workflows because the manager package does not declare @cotal-ai/cli as a dependency. Consider exporting detachKey from @cotal-ai/cli's public entry point, or adding @cotal-ai/cli as a devDependency and importing through its declared export map.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At implementations/manager/smoke/attach.smoke.ts, line 12:
<comment>The `detachKey` import reaches across package boundaries into `@cotal-ai/cli` internals (`src/lib/attach-client.js`) rather than using a stable public export. This bypasses the CLI's `exports` contract and is fragile if the CLI reorganizes its source tree. It also won't work in package-isolated workflows because the manager package does not declare `@cotal-ai/cli` as a dependency. Consider exporting `detachKey` from `@cotal-ai/cli`'s public entry point, or adding `@cotal-ai/cli` as a devDependency and importing through its declared export map.</comment>
<file context>
@@ -9,7 +9,7 @@
import { execFileSync } from "node:child_process";
import { createRuntime } from "../src/index.js";
-import { detachKey } from "../src/attach-client.js";
+import { detachKey } from "../../cli/src/lib/attach-client.js"; // the operator ws client moved into @cotal-ai/cli (stage 2a); dev-only smoke import
import "@cotal-ai/cmux"; // registers the `cmux` runtime provider
import "@cotal-ai/tmux"; // registers the `tmux` runtime provider
</file context>
| agent: values.agent, | ||
| config: values.config, | ||
| model: values.model, | ||
| cwd: values.cwd, |
There was a problem hiding this comment.
P2: Detached mode forwards cwd as raw user input while foreground mode normalizes it with resolvePath(values.cwd). This breaks the documented launch-grammar parity — a relative --cwd will resolve against the manager process's working directory in detached mode, but against the invoking shell's cwd in foreground mode, causing inconsistent file access between the two modes. Normalize cwd with resolvePath before sending it in the askManager call to preserve parity.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At implementations/cli/src/commands/spawn.ts, line 176:
<comment>Detached mode forwards `cwd` as raw user input while foreground mode normalizes it with `resolvePath(values.cwd)`. This breaks the documented launch-grammar parity — a relative `--cwd` will resolve against the manager process's working directory in detached mode, but against the invoking shell's cwd in foreground mode, causing inconsistent file access between the two modes. Normalize `cwd` with `resolvePath` before sending it in the `askManager` call to preserve parity.</comment>
<file context>
@@ -114,32 +128,74 @@ async function uniqueMeshName(
+ agent: values.agent,
+ config: values.config,
+ model: values.model,
+ cwd: values.cwd,
+ resume: values.resume, // host-local session id; the manager preflights connector resume support
+ prompt: values.prompt,
</file context>
| cwd: values.cwd, | |
| cwd: values.cwd ? resolvePath(values.cwd) : undefined, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
implementations/manager/src/manager.ts (1)
700-712: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReject
resumewhenresolvedis set atimplementations/manager/src/manager.ts:700-704.
startAgent({ resolved, resume })can currently mix a manifest-authoritative launch with an imperative session fork; addresumeto the same rejection list for consistency with the documented contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@implementations/manager/src/manager.ts` around lines 700 - 712, The manifest-authoritative branch in startAgent currently rejects several imperative overrides when opts.resolved is present, but it still allows opts.resume to slip through. Update the resolved check in manager.ts so the same rejection path also includes resume, returning the same contract-error message from the startAgent flow and keeping the resolved launch behavior consistent with the existing override guard.
🧹 Nitpick comments (10)
implementations/cli/src/lib/status.ts (1)
20-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
WEB_PORT/WEB_URLare duplicated withimplementations/web/src/web.ts.Both files independently declare the same literals (
7799, thecotal.localhostURL template). The comment explains this is intentional to avoid importing the extension package into the core CLI, but it's still a single fact declared twice — a future port change in one place silently desyncs the other (e.g., the ready-card would report the wrong URL/port).Consider hoisting the constant into a tiny shared location both sides can depend on without coupling to the extension's full module (e.g.
@cotal-ai/coreor@cotal-ai/workspace, both of which the CLI and extension already carry).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@implementations/cli/src/lib/status.ts` around lines 20 - 25, The port/URL literals are duplicated between status.ts and the web module, so keep a single source of truth for the dashboard address. Move WEB_PORT and WEB_URL into a small shared location that both the CLI status code and the web entrypoint can import without pulling in the full extension, and update the existing references in WEB_PORT/WEB_URL consumers to use that shared constant.docs/OVERVIEW.md (1)
65-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAwkward sentence break in the Observability bullet.
The new install clause is spliced into the middle of the "presence, channels, and a live feed" list, breaking the parallel structure and readability.
✏️ Suggested rewording
- **Observability.** Traces and presence live on the mesh, so any observer can render them: `cotal console` (terminal) or `cotal web` (browser dashboard with presence, - installed once via `cotal ext add cotal-web` — - channels, and a live feed; see [web.md](web.md)). + channels, and a live feed; installed once via `cotal ext add cotal-web` — see + [web.md](web.md)).As per coding guidelines, "Keep docs short and human, and keep them updated in the same change as behavior changes."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/OVERVIEW.md` around lines 65 - 68, The Observability bullet has a sentence break that interrupts the parallel list and makes the install clause read awkwardly. Reword the text around the “cotal web” description so the “installed once via cotal ext add cotal-web” note sits cleanly outside the “presence, channels, and a live feed” list, preserving a single readable sentence in docs/OVERVIEW.md.Source: Coding guidelines
implementations/cli/src/command.ts (1)
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLocal
cparameter shadows the importedc(ui colors).
visible.map((c) => c.name.length)shadows thecimport used elsewhere in this same function (c.bold,c.dim), which is easy to misread on a future edit.♻️ Suggested rename
- const pad = Math.max(...visible.map((c) => c.name.length)); + const pad = Math.max(...visible.map((cmd) => cmd.name.length));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@implementations/cli/src/command.ts` at line 25, The local `c` callback parameter in `command.ts` is shadowing the imported `c` colors helper used in the same function, which makes the code easy to misread. Rename the `visible.map((c) => c.name.length)` parameter in the command rendering logic to something distinct, and keep the imported `c.bold`/`c.dim` references unchanged so the intent stays clear.implementations/cli/smoke/server-resolution-live.smoke.ts (1)
31-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the
--creds-driven space-fallback branch.
control.ts's doc comment calls outresolveControlTarget's "ONE control-specific delta": whenflags.credsis set andflags.spaceis absent, space falls back to the folder's.cotal/authspace instead ofDEFAULT_SPACE. None of the scenarios here passcredswithoutspace, so this deliberate behavioral difference is untested.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@implementations/cli/smoke/server-resolution-live.smoke.ts` around lines 31 - 112, Add a smoke test in server-resolution-live.smoke.ts that exercises resolveControlTarget with flags.creds present and flags.space omitted, so the fallback uses the folder’s .cotal/auth space instead of DEFAULT_SPACE. Reuse resolveControlTarget and a control-caller-privileged call, set up a creds-backed scenario, and assert the resolved space matches the auth space while keeping the existing registry/live-broker coverage intact..github/workflows/ci.yml (1)
54-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComment doesn't cover all newly added smoke targets.
The step comment explains
core-boundary,preflight,server-resolution:live,spawn-from-anywhere:live,up-stack/up-manifest:live, andext:live, but the run line also addssmoke:spawn-detach:live,smoke:setup-pure:live, andsmoke:dogfood:livewith no corresponding rationale.📝 Suggested comment addition
# smoke:up-stack:live / smoke:up-manifest:live e2e the full `up --detach` stack (broker + # delivery + manager, real `cotal ps`) and the one-manager-carries-the-launch invariant of # `up -f` (the singleton-lease race regression). smoke:ext:live e2es the operator-installed # extension lifecycle (cotal ext) through the real binary. nats-server installed above. + # smoke:spawn-detach:live, smoke:setup-pure:live, and smoke:dogfood:live cover the detach flow, + # the pure/no-daemon setup path, and the end-to-end dogfood scenario respectively. run: pnpm smoke:core-boundary && pnpm smoke:preflight && pnpm smoke:server-resolution:live && pnpm smoke:spawn-from-anywhere:live && pnpm smoke:spawn-detach:live && pnpm smoke:setup-pure:live && pnpm smoke:up-stack:live && pnpm smoke:up-manifest:live && pnpm smoke:ext:live && pnpm smoke:dogfood:live🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 54 - 65, The step comment for the smoke-test job is missing rationale for the newly added targets in the run command. Update the comment above the job’s run line so it also explains smoke:spawn-detach:live, smoke:setup-pure:live, and smoke:dogfood:live, keeping the existing style alongside the other named smoke targets in this workflow step.docs/protocol-view.md (1)
19-19: 📐 Maintainability & Code Quality | 🔵 TrivialConsider pointing to the extension's own README instead of embedding install commands.
Per the guideline that
docs/describes the protocol only and each example documents itself in its ownexamples/*/README.md, embedding the exactcotal ext add cotal-web/cotal ext add ./implementations/demoinstall commands here duplicates content that belongs with the web/demo packages themselves, and will drift if package names/paths change again.As per coding guidelines, "docs/ describes the protocol only" and "each example documents itself in its own examples/*/README.md."
Also applies to: 32-34
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/protocol-view.md` at line 19, The protocol view is duplicating extension install commands instead of keeping docs/protocol-view.md protocol-only. Update the web and demo entries to refer readers to the extension’s own README or example README, and remove the embedded cotal ext add installation strings from this table so package names/paths stay documented alongside the implementation. Use the existing web/demo row content as the anchor when editing this section.Source: Coding guidelines
implementations/manager/src/commands.ts (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType-unsafe narrowing of
ParsedArgs.valuesto string-onlyValues.
args.valuesis typedRecord<string, string | boolean | undefined>, but the cast toValues(Record<string, string | undefined>) silently assumes no boolean flags. Currently true forsupervise(all flags arestring), but the cast won't warn if a boolean flag is ever added here.Also applies to: 36-37
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@implementations/manager/src/commands.ts` at line 16, The `Values` alias and the `args.values as Values` narrowing in `commands.ts` are too permissive because they drop the `boolean` branch from `ParsedArgs.values` without protection. Update the `supervise` parsing flow around `Values` and the related casts so the types reflect only the actual allowed keys or explicitly validate/convert each expected value before use, instead of assuming all entries are strings. This should prevent future boolean flags from being silently accepted in the `supervise` command handling and the same fix should be applied to the other `args.values as Values` usage in this module.implementations/cli/src/commands/channels.ts (1)
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winManual
valuescast risks silent drift from the flags array inindex.ts.
valuesis hand-typed viaas {...}rather than derived from the command'sFlagSpec[]the wayagents.ts'sstop/ps/attachdo (FlagValues<typeof stopFlags>). Ifchannels's flags array inindex.tschanges without updating this cast, TypeScript won't catch the mismatch. Currently consistent, but this same pattern also appears inpersonas.ts,down.ts, andfeedback.ts.Consider exporting a local
channelsFlagsconst (asagents.tsdoes) and typingvalueswithFlagValues<typeof channelsFlags>for compile-time safety.Also applies to: 24-26
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@implementations/cli/src/commands/channels.ts` at line 8, The manual type cast for values in channels.ts can drift from the actual flags defined in index.ts, so make values derive from the command’s FlagSpec[] instead of a handwritten shape. Export a local channelsFlags const and use FlagValues<typeof channelsFlags> the same way agents.ts does for stop/ps/attach, then update the ParsedArgs handling so the flag types stay synchronized at compile time. Apply the same pattern here wherever the channels command’s flags are defined and consumed.docs/designs/agents/coordination-structure.md (1)
1-529: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffPlacement/scope conflicts with the
docs/guideline.This is a design record for one operator's external wave-coordination workflow (nix-config paths, a private wave tracker, Linear backlog conventions) — not a description of the Cotal protocol, and it isn't scoped to an
examples/*/README.mdeither. It also runs to 529 lines with dense inline citations, which is far from "short."As per coding guidelines: "
docs/describes the protocol only" and "Keep docs short and human." Consider whether this record belongs outsidedocs/(e.g. under.internal/plans/alongside itsSTATUS.mdrow, consistent with the plan-tracking convention) rather than in the protocol-documentation tree.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/designs/agents/coordination-structure.md` around lines 1 - 529, This design record is out of scope for the docs tree because it documents one operator’s external wave workflow, private tracker, and nix-config-specific implementation details rather than the Cotal protocol itself. Move the content to the internal plan-tracking area (for example alongside its STATUS.md entry) and keep docs/ limited to short, human protocol docs; update the existing record path or split the material so only protocol-facing guidance remains in docs, with the rest referenced from the plan artifact.Source: Coding guidelines
bin/smoke/ext-live.smoke.ts (1)
104-113: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCorrupted fixture state isn't restored if an assertion throws mid-block.
ok()throws on failure, so a failing assertion between a corruption write and its restore (sections B, C3, D) leaves the on-disk fixture broken for every later block in the same run, turning one real failure into a cascade of confusing follow-on failures.♻️ Proposed fix — restore in `finally`
{ const good = readFileSync(installedIndex, "utf8"); writeFileSync(installedIndex, 'throw new Error("BOOM — cache must not import me");\n'); - const help = cotal(["--help"]); - ok("--help lists the extension command WITHOUT importing it", help.status === 0 && /hello-ext/.test(help.stdout), help.stdout.slice(-400)); - const comp = cotal(["__complete", "hello-ext", "--"]); - ok("<TAB> offers cached flags WITHOUT importing", comp.status === 0 && /--shout/.test(comp.stdout), comp.stdout); - const run = cotal(["hello-ext"]); - ok("running the broken extension fails loud, naming it", run.status === 1 && /cotal-ext-fixture/.test(run.stderr) && /BOOM/.test(run.stderr), run.stderr.slice(0, 300)); - writeFileSync(installedIndex, good); + try { + const help = cotal(["--help"]); + ok("--help lists the extension command WITHOUT importing it", help.status === 0 && /hello-ext/.test(help.stdout), help.stdout.slice(-400)); + const comp = cotal(["__complete", "hello-ext", "--"]); + ok("<TAB> offers cached flags WITHOUT importing", comp.status === 0 && /--shout/.test(comp.stdout), comp.stdout); + const run = cotal(["hello-ext"]); + ok("running the broken extension fails loud, naming it", run.status === 1 && /cotal-ext-fixture/.test(run.stderr) && /BOOM/.test(run.stderr), run.stderr.slice(0, 300)); + } finally { + writeFileSync(installedIndex, good); + } }(Same pattern for sections C3 and D.)
Also applies to: 134-141, 144-150
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/smoke/ext-live.smoke.ts` around lines 104 - 113, The smoke test in cotal’s fixture-corruption block leaves the fixture mutated if any ok() assertion throws before the restore write runs. Wrap each corruption/restore sequence in try/finally so writeFileSync(installedIndex, good) always executes, and apply the same pattern in the other affected blocks around the cotal helper assertions (including the sections that exercise --help, __complete, and hello-ext).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/smoke/setup-pure-live.smoke.ts`:
- Line 34: The smoke test setup in setup-pure-live.smoke.ts is using
import.meta.dirname, which is not available on all Node 20.x versions supported
by the package. Update the path resolution near tsxCli to use a
compatibility-safe approach such as fileURLToPath(import.meta.url), or raise the
Node engine requirement if this API is intentionally required.
In `@docs/getting-started.md`:
- Around line 65-72: Add a language hint to the fenced status-card block in the
getting-started markdown to satisfy markdownlint MD040. Update the fenced block
around the cotal status output to use a text language identifier (for example,
the block containing “cotal · status” and the NATS/plugin/mesh lines) so the
docs lint cleanly.
In `@docs/setup-internals.md`:
- Around line 69-80: The setup docs currently claim that foreground up routes
through startMeshDetached, but plain up starts nats-server inline and only up
--detach uses that helper. Update the wording in the section around cotal up,
ensureControlPlane, and startMeshDetached so it distinguishes foreground up from
detached mode and only attributes background mesh startup to startMeshDetached.
In `@implementations/cli/src/commands/ext.ts`:
- Around line 44-60: The issue is that ourPackageDir() relies on
import.meta.resolve, which is unavailable by default on Node 20.0–20.5 while the
CLI/runtime checks still allow those versions. Update the ext command path in
ext.ts by either raising the supported Node floor to 20.6+ wherever the runtime
is validated, or adding a compatibility fallback in ourPackageDir() that avoids
import.meta.resolve on older 20.x versions. Make sure the behavior remains
correct for resolving `@cotal-ai/core` and other shared packages.
In `@implementations/cli/src/commands/up.ts`:
- Around line 212-231: The up command currently calls stopManager() and
immediately starts startMeshDetached(), which can race the old manager still
holding the singleton lease. Update the up flow in the command that writes the
launch spec and invokes startMeshDetached so it waits for the previous manager
process to actually exit after stopManager() before launching the replacement,
and keep the launch-spec-first ordering intact.
In `@implementations/delivery/src/feedback-intake.ts`:
- Around line 153-156: The catch block in feedback-intake’s request handler is
returning raw non-HttpError exception messages to the caller, which can leak
internal details. Update the error handling around the try/catch in the feedback
intake flow so that unexpected exceptions are mapped to a generic sanitized 500
response, while preserving the original message only for internal logging. Keep
the existing HttpError path intact, but avoid using e.message directly in the
json response for non-HttpError cases.
- Around line 128-150: The multicast payload in feedback-intake is leaking
tester IPs because ep.multicast is sending the full FeedbackRecord data,
including remoteAddress, to the mesh channel. Keep remoteAddress in the locally
stored record for appendFileSync, but build a separate sanitized payload for
renderFeedback and the multicast data part in feedback-intake.ts so only non-PII
fields are broadcast. Use the FeedbackRecord construction and the ep.multicast
call site to locate the change.
In `@implementations/web/tsconfig.json`:
- Around line 7-8: The tsconfig exclusion is targeting the src/web directory,
but the intent is to exclude the src/web.ts entry file. Update the exclude
setting in the web tsconfig so it references src/web.ts instead of src/web,
keeping the include/exclude behavior aligned with the actual file matched by the
config.
In `@README.md`:
- Around line 88-93: The README setup description still mentions a Codex
connector, but the supported connector list should only reference Claude and
OpenCode. Update the setup blurb around the guided first-run flow so it says
Claude installs a plugin and OpenCode auto-wires at spawn, and remove any Codex
mention from that sentence while keeping the rest of the onboarding text
unchanged.
---
Outside diff comments:
In `@implementations/manager/src/manager.ts`:
- Around line 700-712: The manifest-authoritative branch in startAgent currently
rejects several imperative overrides when opts.resolved is present, but it still
allows opts.resume to slip through. Update the resolved check in manager.ts so
the same rejection path also includes resume, returning the same contract-error
message from the startAgent flow and keeping the resolved launch behavior
consistent with the existing override guard.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 54-65: The step comment for the smoke-test job is missing
rationale for the newly added targets in the run command. Update the comment
above the job’s run line so it also explains smoke:spawn-detach:live,
smoke:setup-pure:live, and smoke:dogfood:live, keeping the existing style
alongside the other named smoke targets in this workflow step.
In `@bin/smoke/ext-live.smoke.ts`:
- Around line 104-113: The smoke test in cotal’s fixture-corruption block leaves
the fixture mutated if any ok() assertion throws before the restore write runs.
Wrap each corruption/restore sequence in try/finally so
writeFileSync(installedIndex, good) always executes, and apply the same pattern
in the other affected blocks around the cotal helper assertions (including the
sections that exercise --help, __complete, and hello-ext).
In `@docs/designs/agents/coordination-structure.md`:
- Around line 1-529: This design record is out of scope for the docs tree
because it documents one operator’s external wave workflow, private tracker, and
nix-config-specific implementation details rather than the Cotal protocol
itself. Move the content to the internal plan-tracking area (for example
alongside its STATUS.md entry) and keep docs/ limited to short, human protocol
docs; update the existing record path or split the material so only
protocol-facing guidance remains in docs, with the rest referenced from the plan
artifact.
In `@docs/OVERVIEW.md`:
- Around line 65-68: The Observability bullet has a sentence break that
interrupts the parallel list and makes the install clause read awkwardly. Reword
the text around the “cotal web” description so the “installed once via cotal ext
add cotal-web” note sits cleanly outside the “presence, channels, and a live
feed” list, preserving a single readable sentence in docs/OVERVIEW.md.
In `@docs/protocol-view.md`:
- Line 19: The protocol view is duplicating extension install commands instead
of keeping docs/protocol-view.md protocol-only. Update the web and demo entries
to refer readers to the extension’s own README or example README, and remove the
embedded cotal ext add installation strings from this table so package
names/paths stay documented alongside the implementation. Use the existing
web/demo row content as the anchor when editing this section.
In `@implementations/cli/smoke/server-resolution-live.smoke.ts`:
- Around line 31-112: Add a smoke test in server-resolution-live.smoke.ts that
exercises resolveControlTarget with flags.creds present and flags.space omitted,
so the fallback uses the folder’s .cotal/auth space instead of DEFAULT_SPACE.
Reuse resolveControlTarget and a control-caller-privileged call, set up a
creds-backed scenario, and assert the resolved space matches the auth space
while keeping the existing registry/live-broker coverage intact.
In `@implementations/cli/src/command.ts`:
- Line 25: The local `c` callback parameter in `command.ts` is shadowing the
imported `c` colors helper used in the same function, which makes the code easy
to misread. Rename the `visible.map((c) => c.name.length)` parameter in the
command rendering logic to something distinct, and keep the imported
`c.bold`/`c.dim` references unchanged so the intent stays clear.
In `@implementations/cli/src/commands/channels.ts`:
- Line 8: The manual type cast for values in channels.ts can drift from the
actual flags defined in index.ts, so make values derive from the command’s
FlagSpec[] instead of a handwritten shape. Export a local channelsFlags const
and use FlagValues<typeof channelsFlags> the same way agents.ts does for
stop/ps/attach, then update the ParsedArgs handling so the flag types stay
synchronized at compile time. Apply the same pattern here wherever the channels
command’s flags are defined and consumed.
In `@implementations/cli/src/lib/status.ts`:
- Around line 20-25: The port/URL literals are duplicated between status.ts and
the web module, so keep a single source of truth for the dashboard address. Move
WEB_PORT and WEB_URL into a small shared location that both the CLI status code
and the web entrypoint can import without pulling in the full extension, and
update the existing references in WEB_PORT/WEB_URL consumers to use that shared
constant.
In `@implementations/manager/src/commands.ts`:
- Line 16: The `Values` alias and the `args.values as Values` narrowing in
`commands.ts` are too permissive because they drop the `boolean` branch from
`ParsedArgs.values` without protection. Update the `supervise` parsing flow
around `Values` and the related casts so the types reflect only the actual
allowed keys or explicitly validate/convert each expected value before use,
instead of assuming all entries are strings. This should prevent future boolean
flags from being silently accepted in the `supervise` command handling and the
same fix should be applied to the other `args.values as Values` usage in this
module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d649707e-3079-4321-8fb0-31c0f99c1966
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (90)
.github/workflows/ci.ymlAGENTS.mdREADME.mdbin/cotal.tsbin/package.jsonbin/smoke/dogfood-live.smoke.tsbin/smoke/ext-live.smoke.tsbin/smoke/flag-inventory.smoke.tsbin/smoke/launch-parity.smoke.tsbin/smoke/setup-pure-live.smoke.tsbin/smoke/spawn-detach-live.smoke.tsbin/smoke/up-stack-live.smoke.tsdocs/OVERVIEW.mddocs/architecture.mddocs/claude-code-integration.mddocs/designs/agents/coordination-structure.mddocs/getting-started.mddocs/manifest.mddocs/protocol-view.mddocs/setup-internals.mddocs/web.mdextensions/connector-core/smoke/spawn-args.smoke.tsextensions/connector-core/src/agent.tsextensions/connector-core/src/tool-specs.tsimplementations/cli/package.jsonimplementations/cli/smoke/command-kernel.smoke.tsimplementations/cli/smoke/server-resolution-live.smoke.tsimplementations/cli/smoke/up-manifest-live.smoke.tsimplementations/cli/src/command.tsimplementations/cli/src/commands/agents.tsimplementations/cli/src/commands/channels.tsimplementations/cli/src/commands/completion.tsimplementations/cli/src/commands/console.tsimplementations/cli/src/commands/down.tsimplementations/cli/src/commands/ext.tsimplementations/cli/src/commands/feedback.tsimplementations/cli/src/commands/history.tsimplementations/cli/src/commands/join.tsimplementations/cli/src/commands/meshes.tsimplementations/cli/src/commands/mint.tsimplementations/cli/src/commands/personas.tsimplementations/cli/src/commands/send.tsimplementations/cli/src/commands/setup.tsimplementations/cli/src/commands/spawn.tsimplementations/cli/src/commands/topology.tsimplementations/cli/src/commands/up.tsimplementations/cli/src/commands/use.tsimplementations/cli/src/ext-loader.tsimplementations/cli/src/index.tsimplementations/cli/src/lib/attach-client.tsimplementations/cli/src/lib/connect.tsimplementations/cli/src/lib/control.tsimplementations/cli/src/lib/delivery-proc.tsimplementations/cli/src/lib/manager-proc.tsimplementations/cli/src/lib/status.tsimplementations/cli/src/ui.tsimplementations/delivery/src/delivery.tsimplementations/delivery/src/feedback-intake.tsimplementations/delivery/src/index.tsimplementations/demo/package.jsonimplementations/demo/src/demo.tsimplementations/demo/src/index.tsimplementations/demo/tsconfig.jsonimplementations/manager/package.jsonimplementations/manager/smoke/attach-repaint.smoke.tsimplementations/manager/smoke/attach.smoke.tsimplementations/manager/smoke/start-overrides.smoke.tsimplementations/manager/src/commands.tsimplementations/manager/src/manager.tsimplementations/web/package.jsonimplementations/web/src/index.tsimplementations/web/src/web.tsimplementations/web/src/web/app.jsimplementations/web/src/web/graph.htmlimplementations/web/src/web/graph.jsimplementations/web/src/web/index.htmlimplementations/web/tsconfig.jsonpackage.jsonpackages/core/src/command.tspackages/core/src/connector-config.tspackages/workspace/src/colors.tspackages/workspace/src/connect.tspackages/workspace/src/extensions.tspackages/workspace/src/flags.tspackages/workspace/src/index.tspackages/workspace/src/provenance.tspackages/workspace/src/space.tsreserved/cotal-web/LICENSEreserved/cotal-web/README.mdreserved/cotal-web/package.json
💤 Files with no reviewable changes (3)
- reserved/cotal-web/README.md
- reserved/cotal-web/package.json
- reserved/cotal-web/LICENSE
| const realNode = spawnSync("which", ["node"], { encoding: "utf8" }).stdout.trim(); | ||
| symlinkSync(realNode, join(binDir, "node")); | ||
| const env = { ...process.env, COTAL_HOME: home, PATH: binDir, COTAL_SKIP_ASSIST: "1" }; | ||
| const tsxCli = resolve(import.meta.dirname, "..", "..", "node_modules", "tsx", "dist", "cli.mjs"); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the declared Node engine constraint across the monorepo
rg -n '"node"' --type=json -g 'package.json' -A1Repository: sealedsecurity/Cotal
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate package.json files and inspect any Node engine constraints
git ls-files '**/package.json' | sed 's#^`#FILE`: #'
echo
for f in $(git ls-files '**/package.json'); do
echo "===== $f ====="
python3 - <<'PY' "$f"
import json, sys
path = sys.argv[1]
with open(path, 'r', encoding='utf-8') as fh:
data = json.load(fh)
engines = data.get("engines", {})
print("name:", data.get("name"))
print("engines.node:", engines.get("node"))
PY
echo
doneRepository: sealedsecurity/Cotal
Length of output: 2856
Avoid import.meta.dirname here
bin/package.json only declares engines.node >=20, but import.meta.dirname requires Node 20.11+/21.2+, so this smoke test will fail on older supported 20.x releases. Either raise the engine floor or switch to fileURLToPath(import.meta.url).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/smoke/setup-pure-live.smoke.ts` at line 34, The smoke test setup in
setup-pure-live.smoke.ts is using import.meta.dirname, which is not available on
all Node 20.x versions supported by the package. Update the path resolution near
tsxCli to use a compatibility-safe approach such as
fileURLToPath(import.meta.url), or raise the Node engine requirement if this API
is intentionally required.
| ``` | ||
| cotal · ready | ||
| ✓ NATS ✓ plugin ✓ mesh nats://127.0.0.1:4222 · space main | ||
| ✓ web http://cotal.localhost:7799 | ||
| ✓ manager running | ||
| cotal · status | ||
| ✓ NATS nats://127.0.0.1:4222 | ||
| ✓ plugin installed | ||
| ○ mesh down — start: cotal up --detach | ||
| ○ web not installed — add: cotal ext add cotal-web | ||
| ○ manager not running — spawns start it, or: cotal supervise | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language hint to the fenced status-card block.
markdownlint flags this fence for missing a language (MD040).
📝 Proposed fix
-```
+```text
cotal · status
✓ NATS nats://127.0.0.1:4222
✓ plugin installed
○ mesh down — start: cotal up --detach
○ web not installed — add: cotal ext add cotal-web
○ manager not running — spawns start it, or: cotal supervise</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 65-65: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/getting-started.md` around lines 65 - 72, Add a language hint to the
fenced status-card block in the getting-started markdown to satisfy markdownlint
MD040. Update the fenced block around the cotal status output to use a text
language identifier (for example, the block containing “cotal · status” and the
NATS/plugin/mesh lines) so the docs lint cleanly.
Source: Linters/SAST tools
| `cotal up` brings up the whole local stack in one place — since setup became configure-only | ||
| (stage 2b), this is where the mesh and control plane start, so `cotal spawn --detach` / | ||
| `cotal_spawn` find a manager right after `up`. The control plane comes up in cutover order — | ||
| old-manager preflight → **delivery daemon** (auth mode only) → **manager** — via | ||
| `ensureControlPlane` | ||
| ([`lib/delivery-proc.ts`](../implementations/cli/src/lib/delivery-proc.ts)). The detached | ||
| processes, all stopped by `cotal down`: | ||
|
|
||
| - **Mesh:** `startMeshDetached` | ||
| ([`commands/up.ts`](../implementations/cli/src/commands/up.ts)) is the one place that boots a | ||
| background nats-server (also used by `up --detach`). Writes `.cotal/nats.pid` and tails | ||
| `.cotal/nats.log` for the live pane. | ||
| - **Web dashboard:** `startWebDetached` / `ensureWeb` | ||
| ([`commands/web.ts`](../implementations/cli/src/commands/web.ts)) re-execs `cotal web` | ||
| detached. Writes `.cotal/web.pid` and `.cotal/web.log`. `webUp()` probes the port for the | ||
| status card. | ||
| background nats-server (foreground `up` and `up --detach` both route through it). Writes | ||
| `.cotal/nats.pid` and tails `.cotal/nats.log`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm which up() paths call startMeshDetached, and where ensureControlPlane lives.
rg -n 'startMeshDetached|ensureControlPlane' implementations/cli/src/commands/up.ts implementations/cli/src/lib/delivery-proc.ts implementations/cli/src/lib/manager-proc.tsRepository: sealedsecurity/Cotal
Length of output: 1028
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '70,120p' implementations/cli/src/commands/up.ts
printf '\n---\n'
sed -n '200,340p' implementations/cli/src/commands/up.ts
printf '\n---\n'
sed -n '130,220p' implementations/cli/src/lib/delivery-proc.tsRepository: sealedsecurity/Cotal
Length of output: 10611
Drop the foreground up claim — plain up spawns nats-server inline; only up --detach uses startMeshDetached here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/setup-internals.md` around lines 69 - 80, The setup docs currently claim
that foreground up routes through startMeshDetached, but plain up starts
nats-server inline and only up --detach uses that helper. Update the wording in
the section around cotal up, ensureControlPlane, and startMeshDetached so it
distinguishes foreground up from detached mode and only attributes background
mesh startup to startMeshDetached.
Source: Coding guidelines
| /** Resolve the running binary's @cotal-ai/core package dir — the ONE core instance every | ||
| * extension must share. Resolved from this module's own import graph (so dev workspace links | ||
| * and installed node_modules both work) by walking up from the package's resolved ENTRY to the | ||
| * directory holding its package.json — the `exports` maps don't expose "./package.json", | ||
| * so the subpath can't be resolved directly. NOTE: resolution runs from @cotal-ai/cli's graph, | ||
| * so only shared packages the CLI itself carries (core, workspace) are linkable today; an | ||
| * extension peering any other @cotal-ai/* package fails its add loudly. */ | ||
| function ourPackageDir(name: string): string { | ||
| let dir = dirname(fileURLToPath(import.meta.resolve(name))); | ||
| for (;;) { | ||
| const pj = join(dir, "package.json"); | ||
| if (existsSync(pj) && (JSON.parse(readFileSync(pj, "utf8")) as { name?: string }).name === name) return dir; | ||
| const parent = dirname(dir); | ||
| if (parent === dir) throw new Error(`couldn't locate ${name}'s package root from its resolved entry`); | ||
| dir = parent; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check declared Node engine floor across the repo
rg -n '"engines"' -A3 package.json bin/package.json implementations/cli/package.json 2>/dev/null
cat .nvmrc 2>/dev/null
rg -n 'node-version' .github/workflows/ci.yml 2>/dev/nullRepository: sealedsecurity/Cotal
Length of output: 441
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the ext command and the Node-version gate it relies on.
ast-grep outline implementations/cli/src/commands/ext.ts --view expanded || true
rg -n "major < 20|node-version|engines" implementations/cli/src package.json bin/package.json .github/workflows -S
# Read the relevant slices with line numbers.
cat -n implementations/cli/src/commands/ext.ts | sed -n '1,140p'Repository: sealedsecurity/Cotal
Length of output: 10181
🌐 Web query:
Node.js import.meta.resolve Node 20.6.0 unflagged 18.19.0 experimental-import-meta-resolve documentation
💡 Result:
In Node.js, the import.meta.resolve API was unflagged (made available by default without requiring the --experimental-import-meta-resolve CLI flag) starting in Node.js version 20.6.0 and version 18.19.0 [1][2][3]. While the core functionality of import.meta.resolve(specifier) is now stable and unflagged, the non-standard second argument (parentURL) remains experimental and continues to require the --experimental-import-meta-resolve flag to function [4][5][6]. Key takeaways regarding this API: - Default availability: The basic usage, import.meta.resolve(specifier), is available by default in all supported versions equal to or newer than 20.6.0 and 18.19.0 [3][7]. - Non-standard second argument: If you need to provide a parentURL (e.g., import.meta.resolve(specifier, parentURL)), you must still use the --experimental-import-meta-resolve flag [4][5]. - Synchronous behavior: The API returns a string synchronously [8][9]. - Limitations: It is not available within custom loaders, as it would cause a deadlock [8][4]. For authoritative documentation, refer to the "Modules: ECMAScript modules" section of the official Node.js documentation for your specific version [9][4][10].
Citations:
- 1: https://nodejs.org/en/blog/release/v20.6.0
- 2: https://nodejs.org/en/blog/release/v18.19.0
- 3: nodejs/node@1235426
- 4: https://r2.nodejs.org/docs/v22.17.1/api/esm.html
- 5: nodejs/node@65ffb2cae3
- 6: nodejs/node@c20fdb4e52
- 7: v18.19.0 release proposal nodejs/node#50953
- 8: https://nodejs.org/download/release/v18.19.0/docs/api/esm.html
- 9: https://r2.nodejs.org/dist/v20.19.2/docs/api/esm.html
- 10: https://r2.nodejs.org/docs/v18.20.2/api/esm.html
Bump the runtime floor or avoid import.meta.resolve on Node 20.0–20.5. import.meta.resolve() is only available by default from Node 20.6.0, but this repo still declares >=20 and setup.ts only rejects <20, so cotal ext add breaks on 20.0–20.5 unless the floor is raised or a fallback is added.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@implementations/cli/src/commands/ext.ts` around lines 44 - 60, The issue is
that ourPackageDir() relies on import.meta.resolve, which is unavailable by
default on Node 20.0–20.5 while the CLI/runtime checks still allow those
versions. Update the ext command path in ext.ts by either raising the supported
Node floor to 20.6+ wherever the runtime is validated, or adding a compatibility
fallback in ourPackageDir() that avoids import.meta.resolve on older 20.x
versions. Make sure the behavior remains correct for resolving `@cotal-ai/core`
and other shared packages.
| // The resolved launch spec is written FIRST so the ONE control-plane manager that comes up with | ||
| // the broker (inside startMeshDetached) carries it. Exactly one manager serves a space (the | ||
| // singleton lease): a second `supervise` started here for the launch would race the plain one for | ||
| // the lease — the loser refuses, so either the agents never boot or the incumbent is orphaned | ||
| // behind an overwritten pid file. The manager materializes each transient persona and mints creds | ||
| // from the resolved policy — never re-reading a file for authority. | ||
| const specPath = writeLaunchSpec(cotalRoot(), buildLaunchSpec(eff, genRunId())); | ||
| // A leftover detached manager (its broker is gone — the reachability check above proved nothing | ||
| // lives at this address) would win the fresh mesh's lease and the launch manager would refuse. | ||
| // Stop it, so the manager started below WITH the launch spec is THE manager. | ||
| stopManager(); | ||
| let pid: number; | ||
| let controlPlane = false; | ||
| try { | ||
| ({ pid } = await startMeshDetached({ server, space: m.space, open, host, seed: manifestToChannels(eff) })); | ||
| ({ pid, controlPlane } = await startMeshDetached({ server, space: m.space, open, host, seed: manifestToChannels(eff), runtime, launch: specPath })); | ||
| } catch (e) { | ||
| console.error(c.red(`✗ ${(e as Error).message}`)); | ||
| process.exit(1); | ||
| } | ||
| console.log(c.green(`✓ mesh "${m.space}" up at ${server}`) + c.dim(` (broker pid ${pid})`)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the singleton-lease acquisition logic referenced in comments.
rg -n -B3 -A15 'singleton lease' implementations/manager/src/manager.tsRepository: sealedsecurity/Cotal
Length of output: 3305
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== up.ts around the changed block =="
sed -n '190,250p' implementations/cli/src/commands/up.ts
echo
echo "== stopManager definition =="
rg -n -A20 -B8 'function stopManager|const stopManager|export .*stopManager' implementations -g '*.ts'
echo
echo "== manager lease handling around renew/acquire =="
sed -n '200,340p' implementations/manager/src/manager.tsRepository: sealedsecurity/Cotal
Length of output: 14366
Wait for the old manager to exit before starting the replacement. stopManager() only sends SIGTERM and removes the pid file, so startMeshDetached() can still race the previous manager for the singleton lease and turn up -f into an intermittent retry.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@implementations/cli/src/commands/up.ts` around lines 212 - 231, The up
command currently calls stopManager() and immediately starts
startMeshDetached(), which can race the old manager still holding the singleton
lease. Update the up flow in the command that writes the launch spec and invokes
startMeshDetached so it waits for the previous manager process to actually exit
after stopManager() before launching the replacement, and keep the
launch-spec-first ordering intact.
| const record: FeedbackRecord = { | ||
| id: randomUUID(), | ||
| receivedAt: new Date().toISOString(), | ||
| tester: { tester: tester.tester, name: tester.name }, | ||
| remoteAddress: req.socket.remoteAddress, | ||
| feedback: payload, | ||
| }; | ||
|
|
||
| appendFileSync(store, JSON.stringify(record) + "\n", { encoding: "utf8", mode: 0o600 }); | ||
|
|
||
| let published = true; | ||
| try { | ||
| await ep.multicast(renderFeedback(record), { | ||
| channel, | ||
| parts: [ | ||
| { kind: "text", text: renderFeedback(record) }, | ||
| { kind: "data", data: record }, | ||
| ], | ||
| }); | ||
| } catch (e) { | ||
| published = false; | ||
| console.error(c.red(`! stored feedback ${record.id}, but couldn't publish to Cotal: ${(e as Error).message}`)); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
PII leak: tester's IP address broadcast to the whole mesh channel.
The FeedbackRecord interface includes remoteAddress?: string, populated from req.socket.remoteAddress when building each record. That full record — including remoteAddress — is passed as the data part to ep.multicast, so every peer subscribed to the #feedback channel receives the tester's raw IP address, even though the human-readable text rendering (renderFeedback) deliberately omits it. Persisting the IP locally in store for abuse triage is reasonable, but broadcasting it mesh-wide looks unintended.
🔒 Proposed fix: strip remoteAddress before broadcasting
let published = true;
try {
+ const { remoteAddress: _remoteAddress, ...broadcastRecord } = record;
await ep.multicast(renderFeedback(record), {
channel,
parts: [
{ kind: "text", text: renderFeedback(record) },
- { kind: "data", data: record },
+ { kind: "data", data: broadcastRecord },
],
});
} catch (e) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const record: FeedbackRecord = { | |
| id: randomUUID(), | |
| receivedAt: new Date().toISOString(), | |
| tester: { tester: tester.tester, name: tester.name }, | |
| remoteAddress: req.socket.remoteAddress, | |
| feedback: payload, | |
| }; | |
| appendFileSync(store, JSON.stringify(record) + "\n", { encoding: "utf8", mode: 0o600 }); | |
| let published = true; | |
| try { | |
| await ep.multicast(renderFeedback(record), { | |
| channel, | |
| parts: [ | |
| { kind: "text", text: renderFeedback(record) }, | |
| { kind: "data", data: record }, | |
| ], | |
| }); | |
| } catch (e) { | |
| published = false; | |
| console.error(c.red(`! stored feedback ${record.id}, but couldn't publish to Cotal: ${(e as Error).message}`)); | |
| } | |
| const record: FeedbackRecord = { | |
| id: randomUUID(), | |
| receivedAt: new Date().toISOString(), | |
| tester: { tester: tester.tester, name: tester.name }, | |
| remoteAddress: req.socket.remoteAddress, | |
| feedback: payload, | |
| }; | |
| appendFileSync(store, JSON.stringify(record) + "\n", { encoding: "utf8", mode: 0o600 }); | |
| let published = true; | |
| try { | |
| const { remoteAddress: _remoteAddress, ...broadcastRecord } = record; | |
| await ep.multicast(renderFeedback(record), { | |
| channel, | |
| parts: [ | |
| { kind: "text", text: renderFeedback(record) }, | |
| { kind: "data", data: broadcastRecord }, | |
| ], | |
| }); | |
| } catch (e) { | |
| published = false; | |
| console.error(c.red(`! stored feedback ${record.id}, but couldn't publish to Cotal: ${(e as Error).message}`)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@implementations/delivery/src/feedback-intake.ts` around lines 128 - 150, The
multicast payload in feedback-intake is leaking tester IPs because ep.multicast
is sending the full FeedbackRecord data, including remoteAddress, to the mesh
channel. Keep remoteAddress in the locally stored record for appendFileSync, but
build a separate sanitized payload for renderFeedback and the multicast data
part in feedback-intake.ts so only non-PII fields are broadcast. Use the
FeedbackRecord construction and the ep.multicast call site to locate the change.
| } catch (e) { | ||
| const err = e instanceof HttpError ? e : new HttpError(500, (e as Error).message); | ||
| return json(res, err.status, { error: err.message }); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Internal error messages leak to the HTTP caller.
Any non-HttpError exception is wrapped as new HttpError(500, (e as Error).message) and the raw message is returned verbatim in the response body — this can expose internal details (e.g. filesystem paths from appendFileSync/readFileSync failures) to the remote caller, who is an external beta tester, not a trusted operator.
🔒 Proposed fix: sanitize unexpected errors before responding
} catch (e) {
- const err = e instanceof HttpError ? e : new HttpError(500, (e as Error).message);
- return json(res, err.status, { error: err.message });
+ if (e instanceof HttpError) return json(res, e.status, { error: e.message });
+ console.error(c.red(`! feedback intake internal error: ${(e as Error).message}`));
+ return json(res, 500, { error: "internal server error" });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (e) { | |
| const err = e instanceof HttpError ? e : new HttpError(500, (e as Error).message); | |
| return json(res, err.status, { error: err.message }); | |
| } | |
| } catch (e) { | |
| if (e instanceof HttpError) return json(res, e.status, { error: e.message }); | |
| console.error(c.red(`! feedback intake internal error: ${(e as Error).message}`)); | |
| return json(res, 500, { error: "internal server error" }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@implementations/delivery/src/feedback-intake.ts` around lines 153 - 156, The
catch block in feedback-intake’s request handler is returning raw non-HttpError
exception messages to the caller, which can leak internal details. Update the
error handling around the try/catch in the feedback intake flow so that
unexpected exceptions are mapped to a generic sanitized 500 response, while
preserving the original message only for internal logging. Keep the existing
HttpError path intact, but avoid using e.message directly in the json response
for non-HttpError cases.
| "include": ["src"], | ||
| "exclude": ["src/web"] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd . implementations/web/srcRepository: sealedsecurity/Cotal
Length of output: 401
🌐 Web query:
TypeScript tsconfig exclude non-glob path exact directory file match include exclude src/web.ts directory web.ts semantics
💡 Result:
In TypeScript, the exclude property in tsconfig.json does not prevent files from being included in the codebase; it only filters the set of files that would otherwise be included by the include property [1][2]. Key semantics and behaviors: 1. Scope of exclude: The exclude property specifically limits the files resolved by include [1][2]. It has no effect on files added via the files property, files imported in your source code, files included via /// <reference directives, or files included via type declarations [3][1][4]. If a file is referenced by another file that is already included, it will be part of the program regardless of whether it is in the exclude list [4][5]. 2. Exact vs. Glob Patterns: You can use both exact file paths and glob patterns in the exclude array [6]. For example, "exclude": ["src/web.ts"] will successfully prevent that specific file from being picked up if it was matched by an include glob like "src//*" [1][6]. 3. Directory Semantics: If you provide a directory path in exclude (e.g., "dist"), it excludes all files within that directory [7]. However, as noted, this only stops the compiler from "finding" those files through its directory-walking include process; it does not block the compiler from including them if they are referenced elsewhere in your project [1][2]. 4. Defaults: If exclude is not specified, TypeScript defaults to excluding node_modules, bower_components, jspm_packages, and the outDir directory [4]. In summary, to effectively exclude a file like web.ts, you must ensure it is not imported anywhere in your project and not present in your files list [1][4]. The exclude property simply acts as a filter for the automated include process [2].
Citations:
- 1: https://aka.ms/tsconfig/
- 2: https://www.typescriptlang.org/tsconfig/extends.html
- 3: https://www.typescriptlang.org/tsconfig/
- 4: https://github.com/microsoft/TypeScript-Handbook/blob/master/pages/tutorials/tsconfig.json.md
- 5: tsconfig.json exclude doesn't support specific file path microsoft/TypeScript#33526
- 6: Adds glob-style pattern matching for file specifications using new "include" property in tsconfig.json microsoft/TypeScript#3232
- 7: "exclude" with glob does not exclude folders microsoft/TypeScript#12750
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' implementations/web/tsconfig.json
printf '\n--- file list ---\n'
find implementations/web/src -maxdepth 2 -type f | sort
printf '\n--- imports ---\n'
grep -RIn --include='*.ts' --include='*.js' -E 'from .*web|require\\(.*web|import .*web' implementations/web/src || trueRepository: sealedsecurity/Cotal
Length of output: 599
exclude: ["src/web"] does not skip src/web.ts.
exclude only filters the include scan; this path matches the src/web/ directory, not the src/web.ts entry file. If the file should stay out of this config, change it to src/web.ts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@implementations/web/tsconfig.json` around lines 7 - 8, The tsconfig exclusion
is targeting the src/web directory, but the intent is to exclude the src/web.ts
entry file. Update the exclude setting in the web tsconfig so it references
src/web.ts instead of src/web, keeping the include/exclude behavior aligned with
the actual file matched by the config.
| Guided setup, **configure-only**. The **first run** checks prerequisites (locates | ||
| `nats-server` — bundled, or your own on PATH), lets you pick connectors (Claude installs a | ||
| plugin; Codex/OpenCode auto-wire at spawn), and adds two experts plus your session: **david** | ||
| the engineer, **sven** the guide, and **me**, the one you drive. It **launches nothing** and | ||
| prints the commands to start things. If a step fails, it hands you to an interactive Claude | ||
| with the failure context, then retries. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether a "codex" connector exists anywhere in the repo.
rg -ni 'codex' --type=ts --type=md -g '!**/node_modules/**'Repository: sealedsecurity/Cotal
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## README excerpt\n'
sed -n '80,100p' README.md
printf '\n## Connector/setup docs references\n'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' 'Claude|OpenCode|Codex|connector|picker|found = \{ claude, opencode \}' docs README.md .
printf '\n## Likely setup implementation files\n'
fd -i 'setup-internals.md|runFirstRun|first run|setup' .Repository: sealedsecurity/Cotal
Length of output: 50379
Remove the stray “Codex” connector mention
README.md:89-90 only lists Claude and OpenCode as connectors; this should say “OpenCode auto-wires at spawn.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 88 - 93, The README setup description still mentions
a Codex connector, but the supported connector list should only reference Claude
and OpenCode. Update the setup blurb around the guided first-run flow so it says
Claude installs a plugin and OpenCode auto-wires at spawn, and remove any Codex
mention from that sentence while keeping the rest of the onboarding text
unchanged.
Source: Coding guidelines
|
Closing — this record was misfiled. The wave coordination structure is Matt's workflow, not a Cotal-tool feature (the record itself makes no Cotal source changes and states the config artifacts live in Moved to its correct home, with the fork-relative wording corrected and the markdownlint issues cleaned for the zireael CI gate: mattwilkinsonn/zireael#235 ( |
4 participants
Design record (frozen-on-merge) for how Matt's multi-agent wave coordinates over Cotal — reviewed as pure design, separate from any implementation.
What it defines
#general(supervisor-authored announcements, 24h replay) +#coordination(lateral worker↔worker detail, replay off). Dropped the initially-proposed#tasks/#status/#requests— the wave tracker is the single assignment authority (a channel board would be a second authority that drifts), status is ambient presence, and requests are unicast DM/anycast (no channel grant needed).supervisorpersona (the only one withcapabilities: [spawn]); workers request work/agents viaanycast(role: supervisor), coordinate implementation details laterally. Resolves the orchestrator-hub tension by a control-plane / detail-plane split.#generalpost gate + read/post scoping in-mesh; the one-authority / no-self-assign invariants are enforced by tracker ownership + convention outside Cotal (a tracker write never crosses the broker). Open mode is documented as a convention-only quick-start with a verbatim non-enforcement list.Grounded in the repo throughout (file+line + quoted snippets per our planning-evidence bar). Every mechanism cited exists today; the plan authors config + persona + runbook files (in nix-config, not this fork — they're our operational config, not an upstream contribution), no Cotal source changes.
The four operating invariants and the two-channel split were live-validated by the supervisor operator during design. All eight open questions were resolved by Matt (see Resolved Decisions). Runtime-agnostic (pty default; a zellij runtime is a separate follow-up record).
Merge freezes it as the contract executing agents build against.
Co-Authored-By: seal noreply@sealedsecurity.com