Repository navigation
Swarm-review follow-ups for the #76 execution layer: security hardening, Windows shims, correctness nits, named test gaps #88
Copy link
Copy link
Closed
Description
Summary
Consolidated follow-ups from the six-leg swarm review of the #76 final head (6347d1a) — full report + per-finding dispositions on #76. Related: #67–#84 (delivery), PR #85 + #86 + #87 (already-remediated findings), #71 (the capability-registry direction several of these feed).
Security
- S1 (HIGH) — repo-weaponization threat model is undocumented and the opencode permission-abort boundary is event-driven, not policy-enforced. An owned
opencode serveruns withcwd= target repo + fullprocess.env; opencode merges the project'sopencode.json, so a malicious repo can ship{"permission":{"bash":"allow"}}(nopermission.updatedevent ever fires → the adapter's abort never trips) and hostilemcpentries, whileAGENTS.mdcontent flows into the worker prompt. claude/codex workers analogously inherit the repo's.claude/settings.jsonunder the user's trust posture. Options: pin a serve-time restrictive permission policy for owned servers (e.g.--permissionscoped to edit-only within the cwd), or document the "ak runonly in repos you trust with full user privileges" contract in ADR-0018 + run help. (src/lib/execution/opencode.mjs:158,258-262,285-287) - S2 (MED) — port-reservation TOCTOU can hand a rogue local process the per-run Basic credential, the full worker prompt, and the ability to fabricate terminal SSE evidence. Probe-bind-close-rebind (
defaultReservePort) races undermaxConcurrent. Harden: watchchild.exitCodeduringwaitForHealth, or serve--port 0and parse the bound port from child stdout. (src/lib/execution/opencode.mjs:21-30,122-133,255-258) - S3 (LOW) — SSE per-line buffer is unbounded (memory-DoS amplifier; owned-server context). Cap like the 256 KB subprocess cap. (
opencode.mjs:167-169)
Execution correctness
- Windows shim-awareness gap in the execution layer (two reviewers converged):
resolveShim(PATHEXT-aware) is used byhave()but never exported or applied at launch; barespawnFn('claude'|'codex'|'opencode')and'codex'missing fromCMD_SHIMS. Windows: readiness passes, launch ENOENTs. Fix: export + apply insubprocess/opencodelaunch paths, addcodextoCMD_SHIMS, add a win32 spawn assertion. (src/lib/exec.mjs:21-38,subprocess.mjs:99,opencode.mjs:258) maxTurnsis silently dropped byak run— templates carry per-node caps (6–15) thatclaude-flow-codex dualhonored; no execution adapter reads them. Translate to--max-turnsin claude/codexargumentsFor(opencode has no equivalent — document) or drop the field. (routing.mjs:398-424,452)$-pattern substitution corrupts the opencode worker prompt —template.replace('{{task}}', worker.prompt)treats$&$`$'as metachars. Use replacer functions. (opencode.mjs:72)--timeouthas no upper bound — values >2³¹−1 ms are clamped by Node to ~1 ms, turning a huge timeout into an instant timeout for every worker. Cap inpositiveInt. (run.mjs:37-41)- Persisted
dualRoutingis never validated at load — a hand-edited kit.json with{model:123}crashes plan printing far from the cause. Schema-check inbuildRunPlan. (run.mjs,routing.mjs) - cancel/cleanup HTTP is unbounded —
/abort+/instance/disposehave no timeout (a wedged server hangs the runner's timeout branch). Same class as court B4 (launch-phase fetches); fix together. (opencode.mjs:299,307) - No construction invariant tying
canRouteActivitieshosts toEXECUTION_ADAPTERSkeys — a routable host with no execution adapter loads, validates, and materializes, then every worker failscli_unavailableat runtime. Same failure class as court A3, one axis over; exactly Architecture: capability-driven host, provider, binding, and observability adapters #71's trap. (execution/adapters.mjs:7-11)
Test coverage gaps (verified — each names the missing assertion)
- Session-isolation: foreign
sessionIDSSE events must be ignored (currently untested — deleting the filter terminates on any session's idle). (opencode-execution.test.mjs) - Plan-validation guards: duplicate id / unknown dep / self-dep / bad maxConcurrent / dependency-cycle throw. (
execution-runner.test.mjs) - Materialization capability gate: "cannot materialize… requires canRouteActivities". (
routing.test.mjs) - Readiness-false →
cli_unavailablepath. (runner suites) - KILL-fallback signal assertion (
['SIGTERM','SIGKILL'], not just the orphan verdict). (opencode-execution.test.mjs:120-137) - Smaller:
auth_required/cancelledcategories,session.errormapping, SSE multi-chunk splits,run.mjsexit-1/2 paths, schema type rejections + frozen immutability.
Docs/migration notes (LOW)
- Dyadic projection layer (
HOST_PROVIDERduplicated in two places,swapHostModelis two-host-only) — fold into Architecture: capability-driven host, provider, binding, and observability adapters #71's redesign rather than patch. - Downgrade note landed via PR docs(swarm-review): three coherence nits from the #76 swarm documentation review #87 (opencode routes need the current release).
--parallelis silently dropped onak run(informational).
Acceptance
- S1 disposition: policy pinned for owned servers, or the trust contract documented (ADR-0018 + run help).
- S2/S3 hardening landed.
- Windows shim path fixed + asserted.
- maxTurns /
$-prompt / timeout-clamp / dualRouting-load-validation / cancel-cleanup bounds fixed. - Execution-adapter construction invariant added.
- Named test gaps filled.
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
No labels
Activity
robertelee78 commentedon Jul 29, 2026
Driven to PR #91 — every item landed or explicitly dispositioned:
ak run --help— workers run with the user's own CLI trust posture; a hostile repo'sopencode.json/.claude/settings.json/AGENTS.mdare inside it (pre-allowed permissions generate nopermission.updated, so the abort does not trip by design — it covers requests, it is not a sandbox).:0and reports its port on stdout (no probe-bind-release race at all); child death before port report / during health check fails honestly with exit code.resolveShimexported (+{windows, env}seam) and applied at both spawn sites;codexadded to `CMD_SHIMS; PATHEXT-order simulated test.--max-turns; codex/opencode have no surface (documented, not faked).pnpm run checkexit 0 (1083 kit + cjs),pnpm run test:surfaceexit 0 (25). Two pre-existing items from the original list stayed out by design: dyadic projection redesign (#71's closed architecture, not a patch) and the court-B4/B5 runner lifecycle bounds (larger lifecycle redesign — flag if you want it as its own issue; the teardown-HTTP half IS in this PR).fix(execution): harden supervised host workers (pacphi#88)
fix(execution): swarm-review follow-ups — security hardening, Windows…