Repository navigation
feat: mocactl subagent commands - spawn, subagents, get - #496
Conversation
Adds `mocactl spawn | subagents | get` plus the shared leaf-turn transport and subagent ledger they build on. Three new user-facing commands: Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Evaline Ju <69598118+evaline-ju@users.noreply.github.com>
Signed-off-by: Evaline Ju <69598118+evaline-ju@users.noreply.github.com>
Signed-off-by: Evaline Ju <69598118+evaline-ju@users.noreply.github.com>
Signed-off-by: Evaline Ju <69598118+evaline-ju@users.noreply.github.com>
Signed-off-by: Evaline Ju <69598118+evaline-ju@users.noreply.github.com>
ingpaolodettori-dev
left a comment
There was a problem hiding this comment.
Thanks. The ledger/transport split is a good shape, and the security-sensitive parts hold up: the token goes over stdin (never argv or disk), get validates the sid, the ledger opens 0600 with O_NOFOLLOW, and remote strings are sanitized in the table.
However, two of the headline features don't work against the server on main: --bundle is never sent to /runs, so the run goes bare, and --kind converge discards the prompt (the converge path builds its prompt from the synthetic item). The detached worker also drops --control-plane-url/--harness-url and exits without updating the ledger. Details are inline.
Tests: nothing covers runSpawnWorker, defaultFork or the __spawn-worker path, which is where #1 and #3 live. The live smoke accepts failed as a passing state, so it can't catch them either. It also has a dead ...(credential ? [] : []), and its prompt asks for "UNFLAGGED" while the Verdict enum is FLAGGED/CLEAR.
5 commits, all signed off. lint is green again at this head after the 4dc516f prettier failure; check and k8s-kind-e2e were still running when this was drafted. The two CodeQL "file system race" alerts on subagent-ledger.ts are the check-then-open pattern. The O_NOFOLLOW open mitigates the symlink case, so they look acceptable to dismiss.
The new design spec (4dc516f/218c052) documents the current behaviour, including the /runs configRef inheritance and the converge prompt, so the spec comments below track the code must-fixes above. Also, packages/mocactl/README.md:187 still says "periodic compaction", which contradicts the spec's §5 ("Automatic ledger compaction … has no trigger").
| function buildEnvelope(sessionId: string, opts: SpawnOptions): LeafEnvelope { | ||
| // Converge requires `item`; prompt ignores it. One synthetic covers both. | ||
| const item = { item_id: 'spawn', file: '-', pattern: '-' }; | ||
| return { |
There was a problem hiding this comment.
must-fix — --bundle is never applied to the run. The envelope sent to POST /runs has no configRef; the bundle only goes to createSession and the ledger copy. On main, /runs does not pick up the session's bound configRef: handleRunLeafParsed calls runLeaf(body, buildConfig(auth)), buildConfig (knative-server server.ts:81-103) doesn't copy auth.configRef (only the /turn paths do, server.ts:293/475/600), and runPromptLeaf reads only env.configRef (run-leaf.ts:442). So spawn --bundle silently runs bare — the core of the demo flow. Fix: ...(opts.bundle ? { configRef: opts.bundle } : {}) in the envelope, and have subagent-spawn.test.ts assert on the submitted envelope, not just on createSession.
| sessionId, | ||
| kind: opts.kind, | ||
| ...(opts.kind === 'prompt' ? { prompt: opts.prompt } : {}), | ||
| ...(opts.kind === 'converge' |
There was a problem hiding this comment.
must-fix — --kind converge throws away the user's prompt. LeafEnvelope.prompt is only read by runPromptLeaf (run-leaf.ts:365/460); the converge path runs buildLeafPrompt(item, …), so the agent is told to review File: - with Pattern of interest: - for item spawn and the actual prompt never reaches it. The result will be no_verdict or a meaningless verdict. The README also promises a <sid>.patch "for a solve result", but spawn can't send kind: 'solve' (problemStatement + repoUrl + ref). Was converge-with-repo meant to be solve? Either map it to solve or drop converge until it has a real item.
| const logPath = join(stateDir, `spawn-${sessionId}.log`); | ||
| const logFd = openSync(logPath, 'a', 0o600); | ||
| try { | ||
| const child = spawnChild(process.execPath, [process.argv[1]!, '__spawn-worker'], { |
There was a problem hiding this comment.
must-fix — the worker loses the endpoint flags. It's spawned as [argv[1], '__spawn-worker'] with no CLI flags, and buildRuntime resolves endpoints flags → env → config. If the user passed --control-plane-url/--harness-url, the child hits if (!rt.harness) return 2; (line 205) without writing the ledger: nothing is submitted, spawn has already printed a sessionId, and the row drifts to unknown. Pass the resolved endpoints in the stdin payload (or forward the flags), and write a failed ledger update on every early return (line 211's JSON-parse failure has the same problem). Related: spawnChild reports ENOENT etc. through an async 'error' event the try/catch never sees, so child.pid! can be undefined and an unhandled 'error' can crash the parent. Add child.on('error', …) / child.stdin.on('error', …) and check pid.
| }); | ||
| return view.state === 'done' ? 0 : 1; | ||
| } catch (err) { | ||
| ledger.updateState(parsed.sessionId, 'failed', { |
There was a problem hiding this comment.
suggestion — a sync 503 {status:'failed',reason:'saturated'} lands here and gets written as a terminal failed. The spec says saturation is "retry, not a terminal failure" and the server deliberately doesn't persist it, but subagents then trusts the ledger's failed permanently (line 269). The worker should retry with Retry-After, or at least record a non-terminal state.
| continue; | ||
| } | ||
| const alive = r.childPid !== undefined && isAlive(r.childPid); | ||
| const observedAt = alive ? now : r.stateObservedAt; |
There was a problem hiding this comment.
suggestion — observedAt is the last ledger write, and a running row is never rewritten, so the 60s grace runs from spawn rather than from when the child died. Together with a bare kill(pid, 0) in isAlive (pid reuse), a dead worker whose run never landed can show running indefinitely. Consider recording the child's start time (or a harness-side deadline) and giving up after it.
| ``` | ||
|
|
||
| Fetches a terminal result and writes `<sid>.text` (prompt), `<sid>.verdict.json` | ||
| (converge), or `<sid>.patch` (solve) to `--out DIR`. Patches get a trailing newline |
There was a problem hiding this comment.
suggestion: <sid>.patch (solve) can never be produced, because §2.1 only allows --kind prompt|converge. Drop it, or add solve to spawn (see the line-36 comment).
| `approve`/`reject` can resend with a gate decision. | ||
| - `update` — written on every state change, carries just the delta. | ||
|
|
||
| Each append is a single `write(2)` under POSIX `PIPE_BUF` (4 KiB), which is what makes |
There was a problem hiding this comment.
suggestion: this is self-contradictory. A 64 KiB prompt can't fit in a 4 KiB PIPE_BUF line, and PIPE_BUF only applies to pipes/FIFOs anyway. What serializes the appends is O_APPEND on a local filesystem (and not on NFS). Restate the atomicity basis here and in §4 ("two-line fold under PIPE_BUF"), same as the subagent.ts:30 nit.
| | `responded`/`done`/`solved` | — | — | `done` | | ||
| | `paused` | — | — | `paused` | | ||
| | `failed` | — | — | `failed` | | ||
| | 404 / not_found | — | — | `unknown` | |
There was a problem hiding this comment.
nit: GET /runs/status never returns 404 on main. A missing record is 200 {status:'queued'}, so this row (and §3's "A 404 from /runs/status exits 1 with a hint") describes a dead branch. The table is also missing aborted, which the server persists. Map it to failed, or such rows stay unknown and are re-polled forever.
| ## 3. Error handling | ||
|
|
||
| - `createSession` failure: no ledger row written, exit `1`. | ||
| - Fork failure after `createSession`: the ledger row is marked `failed` with the fork |
There was a problem hiding this comment.
suggestion: as implemented, this only covers a synchronous throw from spawn. ENOENT and similar arrive as an async 'error' event, and the worker's early exits (missing harness endpoint because the --control-plane-url/--harness-url flags aren't forwarded, bad stdin JSON) write no ledger update. See the subagent.ts:187 comment. Also worth listing here: sync 503 saturated is retryable, not failed.
| shows the failed spawn.) | ||
| - Per-row `/runs/status` failure in `subagents`: that row is reported as `unknown` with | ||
| the error as its detail; other rows still render. | ||
| - `spawn` validation: bundle digest must match `sha256:<64 lowercase hex>`; `--kind |
There was a problem hiding this comment.
nit: the lint fix left --kind / converge split across a line break with the continuation at column 0, which ends the list item's indentation visually. Keep the inline code on one line (e.g. break before `--kind converge`) so prettier leaves it indented.
Signed-off-by: Evaline Ju <69598118+evaline-ju@users.noreply.github.com>
|
Thanks for the review. Addressed on 8a74e36: Must-fixes:
Suggestions:
Nits:
Deferred to follow-up: storing solve envelopes under |
ingpaolodettori-dev
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround. 8a74e36 fixes the big ones from the 218c052 review. configRef now goes on the envelope, converge became solve with problemStatement/repoUrl/ref (matches isRunEnvelope/runSolveLeaf on main), the CLI uses --config, and the worker gets its endpoints through the stdin payload. The fork now awaits 'spawn'/'error', sessionId is validated before join, get sanitizes and echoes reason/message, aborted is mapped, and there are worker tests. The spec's converge and configRef claims are fixed too.
One new must-fix, and one residual of the earlier endpoint must-fix:
GET /v1/runs/statusis not a route on main. The server serves/runs/statusand/run-leaf/statusonly (packages/knative-server/src/server.ts:983-988,routes.ts:114,122)./v1/runsexists for POST only. Everysubagentspoll and everygetwould get the empty 404, so rows gounknownandgetalways fails. The unit tests mock fetch, so they can't catch it. Revert the status URL to/runs/status(or add a/v1/runs/statusroute + ROUTES entry + openapi in a server PR first).- Forwarded endpoints are applied only when the child has no harness at all. If the user passes
--harness-url Xand their config has a control-plane URL, the child already has aDiscoveringHarness, ignoresX, and sends the session token to the discovered harness.
Details are inline. The rest are suggestions.
Author: evaline-ju (MEMBER — maintainer)
Areas reviewed: TypeScript (mocactl CLI, worker, transport, ledger), tests, docs (spec + README). Incremental 218c052..8a74e36.
Agent/IDE config (.claude/.vscode): none
Commits: 6, all signed-off: yes
CI status: passing (all 15 checks incl. k8s-kind-e2e)
FYI: #497's docs/reference/cli-mocactl.md says --config is rejected outside run. With this PR that's also allowed on spawn, so whichever lands second should update it.
| signal?: AbortSignal, | ||
| ): Promise<LeafResultBody | 'queued'> { | ||
| const base = trimTrailingSlashes(await this.harness.baseUrl()); | ||
| const url = `${base}/v1/runs/status?sessionId=${encodeURIComponent(sessionId)}`; |
There was a problem hiding this comment.
must-fix: GET /v1/runs/status doesn't exist on main. The status handler matches url.startsWith('/runs/status') || url.startsWith('/run-leaf/status') (packages/knative-server/src/server.ts:983-988), and routes.ts lists only /runs/status and the deprecated /run-leaf/status (lines 114/122). /v1/runs is POST only (server.ts:1024). So this request falls through to the empty 404:
subagentsturns every non-terminal row intounknownand persists it.getalways exits 1 (the 404 has nosession_not_foundcode).
transport-leaf-turn.test.ts was changed to expect the new URL, and the other tests mock fetch, so CI can't see it. Please go back to /runs/status (the canonical client route per routes.ts, surface: 'client'), or land a server-side /v1/runs/status route + ROUTES entry + harness-openapi.yaml first. The spec and README now say /v1/runs/status throughout, so they need the same fix.
| return 2; | ||
| } | ||
| // Rebuild runtime endpoints if the parent forwarded them (the child's env/flags are empty). | ||
| if (!rt.harness && (payload.harnessUrl || payload.controlPlaneUrl)) { |
There was a problem hiding this comment.
must-fix (residual of the earlier endpoint comment): the forwarded endpoints are applied only when !rt.harness. But the child runs buildRuntime with the same env and config.json, and wire() (runtime.ts:75-81) creates a DiscoveringHarness whenever a control-plane URL comes from env or config. So with mocactl spawn --harness-url X … and a configured control plane, the child ignores X and POSTs the run, with the session token, to whatever /v1/discovery returns. The same goes for an env SH_HARNESS_URL that differs from the flag.
The parent already resolved flags → env → config, so the payload is the single source of truth. Apply it whenever it's present (if (payload.harnessUrl || payload.controlPlaneUrl) applyEndpoints(rt, {...})). Also forward the discovered harness URL when the parent used discovery (rt.endpoints.harnessUrl is unset then), so the child doesn't re-discover. A test where the child's env/config points somewhere else would pin this.
| // Sync 503 saturated is a retry-later signal (`run-leaf.ts`: "a 503 is 'retry', not a | ||
| // terminal failure, and /v1/runs/status must not report it as one"). Keep the row running | ||
| // but bump stateObservedAt so the 60s grace restarts from exit instead of from spawn. | ||
| if (body.status === 'failed' && body.reason === 'saturated') { |
There was a problem hiding this comment.
suggestion: nothing retries a saturated run. The worker writes running / saturated — retry and exits, but the server stored nothing, so the next subagents gets queued with a dead child, and 60 s after this write mapHonestState returns unknown ("child exited and no result recorded"), with saturated gone from the detail. The spec (§3, line 158) says it surfaces "as a retry hint instead of ageing to unknown", which isn't what happens. Either retry in the worker, honouring Retry-After with a bounded number of attempts (it's already detached, so waiting is cheap), or record a distinct state/detail that subagents keeps. Then fix the spec sentence.
| ...(rt.endpoints.controlPlaneUrl ? { controlPlaneUrl: rt.endpoints.controlPlaneUrl } : {}), | ||
| ...(rt.endpoints.harnessUrl ? { harnessUrl: rt.endpoints.harnessUrl } : {}), | ||
| }; | ||
| child.stdin!.end(JSON.stringify(payload) + '\n'); |
There was a problem hiding this comment.
nit: child.stdin still has no 'error' listener. If the child exits before reading its payload (a bad argv[1], an early crash), the write gets EPIPE, and an unhandled 'error' on the stream crashes the parent after the ledger row is written. child.stdin!.on('error', () => {}) (or record it as a failed update) closes that.
| (`responded`/`done`/`solved`/`paused`/`failed`/`aborted`) the row becomes terminal with | ||
| the matching honest state. On a sync 503 `{status:'failed', reason:'saturated'}` the | ||
| server treats saturation as a retryable transient (`run-leaf.ts`: _"a 503 is 'retry', | ||
| not a terminal failure, and `/v1/runs/status` must not report it as one"_); the worker |
There was a problem hiding this comment.
nit: the quote is attributed to run-leaf.ts and says /v1/runs/status. The comment is in packages/knative-server/src/server.ts:885 and says /runs/status. This will be fixed along with the status-route comment on leaf-turn.ts.
Signed-off-by: Evaline Ju <69598118+evaline-ju@users.noreply.github.com>
Signed-off-by: Evaline Ju <69598118+evaline-ju@users.noreply.github.com>
|
Thanks, addressed on the latest commits along with a merge in from main: Must-fixes:
Suggestions / nits:
#497 FYI: |
ingpaolodettori-dev
left a comment
There was a problem hiding this comment.
Thanks. 45cb900 resolves everything from the 8a74e36 review. Status reads now go to /runs/status, and the forwarded endpoints are applied unconditionally from the parent's post-discovery URL. Saturation is a real bounded retry: reason is folded into code, and attempts and wall time are both capped, which matches the server's no-record 503. The stdin EPIPE is handled, and the spec and CLI reference are reconciled. The main merge is clean.
One doc nit inline. Approving.
Author: evaline-ju (MEMBER — maintainer)
Areas reviewed: TypeScript (mocactl worker, transport, errors, ledger), tests, docs (spec, README, cli-mocactl reference), merge commit. Incremental 8a74e36..45cb900.
Agent/IDE config (.claude/.vscode): none
Commits: 9, all non-merge commits signed-off (the merge commit 1ec71b9 has no sign-off; DCO passes)
CI status: passing (all 15 checks incl. k8s-kind-e2e)
| commands share one flag name for the same digest. | ||
| - Flow: create session → append a `record` line to the ledger (§2.5) → fork a detached | ||
| worker (§2.6) that holds the sync `POST /v1/runs`. The sessionId is printed on stdout | ||
| before the fork completes so a shell pipeline can capture it even if the worker exits |
There was a problem hiding this comment.
nit: "The sessionId is printed on stdout before the fork completes" doesn't match cmdSpawn. It awaits forkFn (which awaits 'spawn'/'error') and prints the sessionId only after that. On a fork failure it exits 1 with no sessionId on stdout. Suggested wording: "printed on stdout once the worker has started, so a shell pipeline can capture it even if the worker exits immediately afterwards".
Signed-off-by: Evaline Ju <69598118+evaline-ju@users.noreply.github.com>
ingpaolodettori-dev
left a comment
There was a problem hiding this comment.
Follow-up review at fae233e (incremental since our 45cb900 approval).
8925c4dmerges main (5a29af3, #494 workload context delivery).git show --remerge-diffis empty: no conflicts, no evil merge, and none of the PR's 19 files are touched by #494. The newinternalContextfield on the leaf envelope is optional and server-internal, so the mocactl spawn envelope is unaffected.fae233erewords the spec's spawn flow ("printed on stdout once the worker has started"), which resolves our earlier nit. It matchescmdSpawn, which awaitsforkFnbefore writing the sessionId (subagent.ts:154-170).
No new findings. Commits are DCO signed-off. At draft time CI on this head was still running (check, proto, deploy-scripts, k8s-kind-e2e, codeql pending; lint/docs/trivy/shellcheck passed).
Summary
This PR adds three new user-facing commands (spawn, subagents, get) in
mocactland supporting infrastructure to enable. Part of #381, not included: watch, approve/reject, follow-up, configurable concurrency cap, async-turn backendDetails
mocactl spawn "prompt" --bundle sha256:… --kind prompt|converge [--repo URL --ref REF] [--tag T] — creates a session(pinned to the bundle if given), writes a ledger record to $XDG_STATE_HOME/mocactl/subagents.jsonl, then forks a detached worker that holds the sync POST /runs call outside this process group. Prints the sessionId on stdout immediately. Quitting mocactl does not kill the worker. --kind converge enables a harness-side git clone of repoUrl+ref into the sandbox; --kind prompt does a plain prompt turn. --tag groups related spawns so one parent agent's children can be listed in isolation.mocactl subagents [--tag T] [--json]— reads the local ledger and joins each row with GET /runs/status, resolving the server's three-way queued ambiguity (still running / never recorded / aged out) using child-pid liveness and a 60-second grace. Stateunknownis surfaced where the ledger cannot tell status. Default output is an aligned table; --json emits a stable shape.mocactl get SESSIONID [--out DIR] [--json]— fetches a terminal result and writes files (.text, .verdict.json, .patch) plus a bounded summary. Normalizes a trailing newline on patches so git apply doesn't reject them.Infrastructure pieces to enable:
POST /runs+GET /runs/status. This is the one place /runs dispatch lives, so when an async-turn transport ships later, the three commands don't change.$XDG_STATE_HOME/mocactl/subagents.jsonl,mode 0600. Dual line type (record + update) means state transitions don't rewrite history. The record stores the full dispatch envelope so a future approve/reject command can resume a gated session without re-sending flags.Assisted-By: Claude (Anthropic AI) noreply@anthropic.com
Rationale
#468 made solve (implementation) leaves run with promoted config bundles, but there was no mocactl entry point to leverage it. A demo we want to show to showcase moca capabilities needs 3 operations that mocactl did not provide today: dispatch a leaf against a repo (subagent cloning a private repo), surface the parent/child tree (show how a parent agent is connected to spawned subagent), and read back the result of a detached session (parent reads a subagent’s result). The tradeoff would be that the demo would need a wrapper around POST /runs, since mocactl run goes through /v1/turn only (prompt focused, no detached dispatch). These additions allow us to leverage mocactl commands directly and communicate capabilities as “platform patterns”
Checklist
git commit -s).make lintand the tests for the touched packages pass.docs/page, with the right maturity label (seedocs/CONTRIBUTING-DOCS.md). -mocactl/READMEupdatedmake check-linkspasses if any Markdown changed.docs/specs/README.mdordocs/adrs/README.md.