refactor(agentic): machine-runner is a remote control; Claude/Codex adapters move above it into agent-cli - #1861
Conversation
…dapters move above it into agent-cli Mirrors constructive-io/constructive-db#3855. The runner enrolls, runs allow-listed commands in a pty or pipes, streams bytes, resizes, signals, detaches/reattaches — and knows nothing about agents. Claude/Codex adapters, the agent stdio contract and approval decisions live in the new @constructive-db/agent-cli (constructive-agent-cli). The protocol gains the bounded LineSplitter and approval-decision line parser. Runner/agent-cli end-to-end proofs need the relay and stay in constructive-db; unit tests ship here.
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
Review complete. 🟡 3 medium 💬 Inline comments (3)
🧹 Nitpicks (1) — 🟢 1 low
The change moves agent-adapter and session logic from
Reviewed commit: aa456c1 |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR extracts the coding-agent CLI into a new agentic/agent-cli package and refactors machine-runner to delegate to it via pipeProcess, tightening approval policy and cwd confinement along the way.
Key findings
- 🟡 Symlink escape bypasses
resolveCwdpolicy root — policy.ts:53 - 🟡 Guard prompt path against spawn after session settled — session.ts:209
- 🟡 Treat spawn
erroras terminal inpipeProcess— process.ts:76
| const resolved = path.resolve(root, requested); | ||
| if (resolved !== root && !resolved.startsWith(root + path.sep)) { | ||
| throw new PolicyViolationError(`cwd '${requested}' is outside the policy root`); |
There was a problem hiding this comment.
🟡 security · medium
Symlink escape bypasses resolveCwd policy root
The cwd containment check in resolveCwd (agentic/machine-runner/src/policy.ts:50-58) is purely lexical: it compares path.resolve(root, requested) against a startsWith(root + path.sep) prefix and never resolves symlinks. A relay-supplied frame.cwd reaches it via resolveSpawn in openSession (agentic/machine-runner/src/runner.ts:367). A session running an allowed command inside the root can plant a symlink such as inside-link -> /etc and then request cwd: 'inside-link'; the lexical check passes while the spawned process works outside the policy root, defeating the machine owner's confinement.
📋 Prompt for AI Agents
In agentic/machine-runner/src/policy.ts around lines 50-58, make resolveCwd symlink-proof: after path.resolve(root, requested), canonicalize both the policy root and the resolved path with fs.realpath (async or realpathSync.native), falling back to the realpath of the deepest existing ancestor directory when the target does not exist yet, and only then apply the startsWith(root + path.sep) containment check. Add tests in agentic/machine-runner/tests/policy.test.ts covering a symlink inside the root that points outside being rejected.
| if (!child) { | ||
| start(line); | ||
| return; |
There was a problem hiding this comment.
🟡 bug · medium
Guard prompt path against spawn after session settled
onInputLine spawns the CLI via start(line) whenever child is null, but never checks exited (agentic/agent-cli/src/session.ts:209). When the abort signal fires before any prompt arrived, the listener resolves the session with { exitCode: -1, signal: 'SIGTERM' } (agentic/agent-cli/src/session.ts:220-228) while the stdin data/end listeners stay attached, so a prompt line buffered or flushed afterwards still spawns claude/codex. The caller has already been told the session ended, so the freshly spawned CLI runs as an unsupervised orphan; the same applies after fail() rejects.
📋 Prompt for AI Agents
In agentic/agent-cli/src/session.ts, guard the prompt path so no child is spawned after the session has concluded: in onInputLine (around line 200) return early when exited is true before start(line) and before any child.stdin.write; additionally, in the abort-without-child branch (lines 220-228) and in fail() (lines 81-88), remove the io.stdin 'data'/'end'/'error' listeners or call io.stdin.destroy(), so stdin bytes arriving after the promise settles cannot spawn or feed a CLI child.
| child.on('error', err => { | ||
| for (const listener of errorListeners) listener(err); | ||
| }); |
There was a problem hiding this comment.
🟡 bug · medium
Treat spawn error as terminal in pipeProcess
When spawnChild fails to start the program (ENOENT, EACCES), Node emits child.on('error') and never exit, so exited stays false in pipeProcess (agentic/machine-runner/src/process.ts:76-78) and no onExit listener ever runs. The runner's onError handler deletes the session and sends an error frame but no exit frame, and it never kills the process (agentic/machine-runner/src/runner.ts:404-408), so the relay sees a session that failed but formally never ended. Because exited stays false, later write() calls still hit child.stdin and re-emit EPIPE into errorListeners, producing duplicate error frames for an already-deleted session, and the child (if it did start) is left running as an orphan.
📋 Prompt for AI Agents
In agentic/machine-runner/src/process.ts, in the child.on('error', ...) handler (around line 76), set exited = true, invoke errorListeners once, then synthesize an exit event for exitListeners (e.g. { exitCode: -1 }) unless one was already delivered, so a spawn failure is terminal, write()/kill() become no-ops, and the runner receives one terminal report. In agentic/machine-runner/src/runner.ts in the proc.onError callback in openSession (lines 404-408), call proc.kill() before this.sessions.delete(sessionId) so an asynchronously failed pipe child is not left running as an orphan.
… no CLI spawn after the session concludes, complete lines survive an oversized tail
1 participant
Summary
Replaces what #1860 moved here — an agent-aware runner — with the decoupled packages merged in constructive-io/constructive-db#3855, so what gets published from this repo is the corrected layering:
Removed from
machine-runner:agent-cli.ts,cli-session.ts,headless-session.ts, agent bindings/approvals inrunner.ts/policy.ts, and theconfig.tsagent knobs. The relay (still in constructive-db) composes the final command, e.g. cli binding →constructive-agent-cli claude --resume <id> -- <args>.Tests: unit suites ship here (protocol codec/LineSplitter 20, runner cli/config/policy/spawn-helper 22, agent-cli adapters 5 incl. the codex
--prompt-injection regression). The runner and agent-cli end-to-end proofs need@constructive-db/machine-relay, which stays in constructive-db, so they remain there.agentic/agent-cliis added to theagenticCI batch.Versions are left for
lerna version/publishfrommain. Note: main already carries achore(release): publishbumping machine-protocol/runner to 0.2.0, but npm still only has 0.1.0 of each and noagent-cli— the previous publish did not land.Link to Devin session: https://app.devin.ai/sessions/3c993d055ffb47f6be27862718a9cd42
Open in Devin Desktop: https://app.devin.ai/desktop/session/3c993d055ffb47f6be27862718a9cd42?variant=devin
Requested by: @pyramation