feat(agent): replace the shell tool with the myshell process EDSL - #931
Conversation
1dc2747 to
f185287
Compare
tonyfettes
changed the title
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
457bc16 to
1d125d8
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
68a6290 to
1c493e8
Compare
28a00a8 to
cca9a1d
Compare
The agent no longer has a shell. Commands are spawned from `run_moonbit`
programs through `bobzhang/myshell`, a shell-free process EDSL where the
executable and the argument vector stay separate: `Cmd("moon", ["check"])`
passes every argument literally, so `|`, `>`, `&&`, `$()` and `*` have no
meaning and there is no quoting to get wrong. Pipes and control flow are
ordinary MoonBit — capture `out.stdout` and filter it in code — which is
also what replaces grep/sed/awk. What this deletes is a parsing layer and
the string heuristics that defend it.
Everything the shell tool actually delivered comes along:
- Background jobs move to `run_moonbit`'s `run_in_background`, watched by
`job_output` / `job_stop` (renamed from `shell_output` / `shell_stop`,
since a registry with no shell must not advertise shell names).
- The read-only subagents' scratch lab: explore, review, and audit already
registered `run_moonbit` beside shell, so those are mostly deletions —
except the lab, which `run_moonbit` now takes, building the snippet
inside it so the profile's single writable subtree covers both.
- The subtask worker keeps its kernel worker profile, which in that mode
REPLACES the workspace source-write profile: a worker is supposed to
write source, just only its own.
- The foreground bound rises 60s to 300s (a snippet runs whole `moon test`
cycles now), and a run that outlives it is DETACHED as a job rather than
killed, as the shell tool's deadline behaved.
Two hazards specific to compiling before running are handled explicitly. A
detached run is built in the foreground first (`moon run --build-only`), so
a syntax or type error answers inline with a `source:LINE:COL` diagnostic
instead of becoming a job to go read; `moon check` would be the better
preflight but does not accept a `.mbtx` script, hence a TODO(upstream). And
a spawned child no longer inherits the engine's fd 0 — under `serve` that
is the JSONL command channel — so `ShellExecution::start` and the job
runtime take an explicit stdin and run_moonbit passes an empty file.
The source-write sandbox still wraps every run. Note the consequence the
system prompt now states outright: the shell tool could statically
recognize trusted source-writing moon commands (`moon fmt`, `moon info`,
`moon add`, `moon test --update`) and run them exempt, but an arbitrary
program cannot be classified that way, so those commands are denied and
source changes go through the file tools.
`ShellExecution::spill_path` becomes public so the denial scan keeps its
coverage on the retained path: a snippet can flood output before it trips a
denial, and reading the spill file streams that where `read_all` would pull
up to 20MB into memory.
No production path registers the shell tool any more; `eval/tool_harness`
and the TUI's `!` command (a user typing a command) still use the package.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ream edits Replaying the branch onto current main by copying whole files silently reverted two upstream changes, which is the risk that approach carries: `README.mbt.md` went back to the lepus submodule that #941 removed, and `agent_explore/tool.mbt` went back to the deprecated `.to_json()` that #941 had fixed. Both are restored. The rest are bugs in the new background path: - Binary output ended the retained wait EARLY, with the child still running. Reporting there left the process alive while `defer cleanup()` deleted its build directory, and reported an invented exit code. It now waits out the real exit and says the rendering was lossy, as the shell tool did. - The `--build-only` preflight had no timeout, so a first build fetching dependencies could hold the turn open forever — the very thing asking for a background job avoids. - A foreground run detached at its deadline leaked its directory: `dir` only landed under the session's job dir when `run_in_background` was set, and nothing else ever reclaims a system temp dir. Any run that a job might take over now builds under the job dir. - Output past the inline cap KILLS the retained execution, and that was reported as a plain non-zero exit with a silently clipped head. It now says the program was stopped and points at `stdout=ToFile(...)`. - `writable_subtree` could disagree with the directory the snippet actually builds in when a caller passed both a job dir and a lab; it now follows `dir`. Two texts also promised the wrong thing: the description and the prompt said a foreground run is CANCELLED at its deadline, which is exactly the case where it is now detached instead. And the prompt claimed source-writing moon commands "are DENIED by the snippet sandbox" unconditionally, though the sandbox only enforces where `sandbox-exec` exists — it now states the rule and says plainly that off macOS keeping it is the agent's job, not the kernel's. `job_owns_dir` is a plain `let mut`: it lives in one call, so the `Ref` was only habit from the cross-call counter above it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The `target` description claimed native was "required for the async @fs/@stdio/@process IO batteries". That is not true: a snippet compiled to wasm reads and writes files, prints, and spawns commands through myshell just as well — the host performs those on the program's behalf, and `sandbox-exec` still binds the run because it wraps whatever `moon run` spawns. All four were checked by hand before changing the text. What does hold is the split the description now states: native and wasm can do IO and run commands, the other backends cannot (on js `@stdio` is not even defined). The default stays native — switching it is a behaviour change worth measuring rather than asserting. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…pace boundary Snippets previously ran under one rule: sandbox-exec denying writes to source files under the workspace. That rule could not be reconciled with the commands whose job is to rewrite source, so `moon fmt`, `moon info` and `git checkout` were unreachable, and off macOS nothing constrained a run at all. Replace it with two layers that cover different things. moonrun's `--wasm-policy` (every platform) decides which programs may start and bounds the snippet's own file access. The spawn allowlist is per subcommand, so `moon check` can be admitted while `moon run` is considered separately, and git's reconfiguring global options (`-c`, `-C`, `--git-dir`) fall out for free: they precede the subcommand, so no prefix rule matches them. That recovers, without a parser, what the shell tool spent `global_option_reconfigures` on. Subcommands whose dangerous form is selected by a flag rather than a verb (`clean -n`, `apply --check`, `merge -s`) cannot be split by a prefix and are left out whole; where the hazard is a verb, the reads are admitted and the mutations are not (`stash list` yes, `stash pop` no). sandbox-exec (macOS) now draws the line at the workspace instead of at source files: deny every write, re-allow the workspace, this run's build directory, and the toolchain's own directories. `moon fmt` and `git checkout` therefore work, and the reason the old rule existed — an arbitrary program rewriting files behind the model's back — is covered instead by an allowlist that admits no shell, no interpreter and no ad-hoc rewriter. Read-only subruns keep the source-write profile: they hold no editing tool, so "may not write source" is the guarantee that defines them. Three grants are not obvious and each was found by a failure rather than by reasoning: `/dev`, without which `git` dies on `/dev/null`; `/tmp`, because `@fs.tmpdir()` hardcodes it on macOS and Linux so every `moon test` fails without it; and the toolchain root, which moon resolves from its own executable rather than from `MOON_HOME` and so diverges from it whenever `MOON_HOME` is unset, as it is under the desktop app. The snippet's own `TMPDIR` is pointed at a directory the run already owns, so scratch space needs no grant at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
All three came from a live run rather than from review, and each cost the model steps it should not have spent. `git merge-base` and `range-diff` were missing from the spawn allowlist. The list was written from what a turn is usually seen doing, and comparing two branches is not in that picture until someone does it. The FFI refusal matched the bare word `extern`, so a snippet whose only offence was a commit message mentioning extern bindings was rejected as native FFI. It now matches the syntax it is actually looking for (`extern "`), which no longer fires on prose or string literals. The scan remains best-effort, as documented. `gh run view --log-failed` spools through gh's cache directory, so a read-only command failed on a write nobody asked for. The cache is granted the way MOON_HOME already is — `$XDG_CACHE_HOME/gh`, else `~/.cache/gh`, and only the cache: `~/.config/gh` holds the auth token and stays denied. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The boundary profile denied every write and re-allowed an enumerated set of roots. Every root it missed — `/dev` for git's `/dev/null`, `/tmp` for `@fs.tmpdir`, the toolchain root, gh's cache — surfaced as a bare EPERM in a live turn rather than in a test, and the mechanism only ever existed on macOS. What it protected against is also narrower than it looked. `git -C /other reset --hard` is already refused by the spawn allowlist, on every platform: git's redirecting global options precede the subcommand, so they match no prefix rule. And a profile here would have been the only path confinement in the tool set — `write`, `edit` and `remove` take an absolute path anywhere on disk for this role, since `WriteScope` is wired for worker subagents only. Confining the weaker channel while the direct one stays open is not a boundary, so what remains is the allowlist: a bound on which commands exist, not on where they may write. Refusal guidance now keys on the policy's own wording, captured from a run under one rather than paraphrased: `Sandbox policy blocked ` for a file or network access, and `@process.spawn(): Permission denied` for a refused spawn, which prints no line of moonrun's own. That removes the last thing coupled to the granted-roots list — the old detector called any "Permission denied" outside a re-allowed root its own — and it means a refusal is explained on every platform, where the text previously existed only where sandbox-exec did. A background job's refusal is explained too. Read-only subruns and workers keep their profiles unchanged. Holding no editing tool is what defines the first, so "may not write source" stays enforced in the kernel for it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The policy `run_moonbit` writes states its spawn allowlist with `process.allow`, and a moonrun without that field does not ignore it — it refuses the whole policy (`unknown field `allow`, expected `spawn``), taking every run down with it. That field reached nightly first, so nightly is a requirement rather than a preference: CI, the Copilot agent environment, the release build, and the seed the desktop app ships all move to it. Two things follow from having ONE channel again. `moon fmt --check` loses its stable-only condition — it was restricted because the repo cannot satisfy two formatters at once, and there is now only one. And `--deny-warn` goes, because a nightly compiler adds warning classes on its own schedule (`implicit_impl_as_method` already fires across the desktop protocol codecs), so denying warnings would red-line CI for untouched code on a day nobody chose. The seed is pinned exactly (`0.10.9+717497b44-nightly`) because packaging verifies the download against that string and the bundle's signing list is written by hand. That list needed one addition: the nightly archive ships `lib/libLLVM.dylib`, which the pinned stable build did not — the install script bundles core for the llvm backend on nightly. A Mach-O left off the list keeps its ad-hoc signature and fails notarization. Note the size: that dylib is 100MB, and the staged toolchain grows from 232MB to 333MB. CI installs the floating `nightly` rather than the pinned version, because the install script's llvm core bundle is conditional on the literal word. A ten-trial run then showed the allowlist costing the model steps in two ways. `moon --version` matched no rule, because only the `version` SUBCOMMAND was listed and a prefix gates whole tokens. One trial spent three attempts on it — plain, then with a rebuilt `PATH`, then by absolute path — reading each refusal as a broken toolchain. `git --version` was already listed; the asymmetry was an oversight. Both spellings are listed now, with `--help` beside them. The other eight refusals were `ls`, `find`, `which`, `printenv` and `pwd`: the POSIX utilities left out on purpose, discovered one command at a time. The system prompt now names what a snippet may start — the moon/git/gh subcommands in full — and gives the MoonBit replacement for each utility it will reach for, so the list is read once instead of probed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`~/.moon` is where a snippet looks for a package interface, because the API
lookup a turn is taught is `${MOON_HOME:-$HOME/.moon}` — and a machine that
relocated `MOON_HOME` usually still has a registry at the default path too.
Granting only the resolved `MOON_HOME` therefore refused a read that was
perfectly reasonable, and a ten-trial run spent a step on it: the snippet read
`~/.moon` for an `.mbti`, got a policy refusal rather than a plain "no such
file", and had to work out which of the two it was looking at.
Reads only, and beside roots this list already carries.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e prompt The allowlist lived in the main agent's system prompt, and the main agent is not the only role that runs snippets: explore, review, audit and the subtask worker all register `run_moonbit`, and each carries its own prompt. A read-only scout in a live run therefore learned the list the hard way — two refusals before it reasoned "`pwd`/`find`/`ls` aren't in the spawn allowlist, only `git`/`moon` seem usable" and switched to the read tool. Copying the list into four prompts is the drift this repository keeps paying for, and it is the wrong home anyway: which programs may start is a property of the tool, not of the role. It moves into `run_moonbit`'s description, which travels with the tool into every registry, and the system prompt keeps a pointer to it. The description carries the full list plus the MoonBit replacement for each utility a turn reaches for — `ls`, `find`, `cat`, `which`, `pwd`, `printenv`, `sh -c` — so the answer arrives before the refusal does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A read root list bought no confidentiality and cost steps to enforce. The `read` tool takes any absolute path with no scope of its own, so every file on the machine is already one tool call away, and `env.from_host: ["*"]` hands the snippet every environment variable the agent holds, API keys included. Against that, bounding a snippet's reads only turned a wrong guess at where the toolchain lives into a refusal that reads like a broken environment — live runs spent steps guessing `~/.moon`, `~/.mooncakes` and an opam-installed `moon`, none of which the list happened to name. Writes keep their bound for the reason reads do not: a snippet writing outside the tree is invisible in the transcript, where a `write` tool call naming the same path is right there to read. The asymmetry is legibility, not access. What this deletes is the last enumeration on the treadmill. `read: ["*"]` needs no answer to "where is the toolchain", so `moon_home()`, the `MOON_TOOLCHAIN_ROOT` / `PATH` walk behind `resolve_toolchain_root()`, its `Lazy` cache and `toolchain_roots()` all go — about sixty lines whose only job was to stay in step with how three different installers lay out a MoonBit toolchain, and which had already been patched twice for missing exactly that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t can fail The description had grown into one paragraph carrying four unrelated subjects — the snippet envelope, the command EDSL, the spawn allowlist, and the isolation rules — and was read as a wall. It gets headings, a runnable example at the top, and the utility replacements as an aligned mapping instead of an inline run-on. Two `.mbtx` traps are now answered before the compiler answers them. Across two ten-trial runs they produced a third of every diagnostic a snippet raised: 47 calls that raise from a plain `fn main` (the fix is `async fn main`, or `fn main raise` for errors without async) and 29 uses of `@fs`/`@stdio` whose sub-package was never in the import block. Both are properties of the envelope, not of the model's MoonBit, so no amount of fluency avoids them. The description tests that asserted it contains its own words are gone. They pinned wording rather than behaviour — restructuring the text broke them without any gap existing, which is the whole of what they measured. In their place is one derived from `spawnable_commands`: every program and prefix the allowlist admits must be named in the description, so the two cannot drift apart in the direction that costs a turn steps (a subcommand admitted, never documented). It failed on its first run, and correctly: the description claimed `moon` took "any subcommand" while the allowlist enumerates nineteen and withholds `publish`, `login` and `register`. The text now says what is actually true. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`openseek review` called `@run_moonbit.definition(workspace_root~)` with neither `scratch_dir` nor `worker_sandbox`, so profile selection fell past the read-only lab branch to the main-agent one, which carries no kernel profile at all. Its own doc comment said the opposite — "`run_moonbit` is sandboxed against source writes on macOS". The subrun kinds were fine: explore and goal-audit both create a lab and pass it; only the standalone CLI, which does not go through `subrun`, missed. `run_review` now takes `scratch_dir` and forwards it to both the tool and the task text, so the reviewer is told where it may legitimately write — `review_task` had no lab paragraph at all, while `audit_task` has had one. The CLI creates and removes the lab the way `subrun` does. Also drops two stale doc comments describing a `shell(read_only=true)` tool in the child toolsets. No subagent has registered a shell tool since the myshell replacement; `@shell.definition` survives only in its own test and the tool harness. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`defer` accepts async calls now — it did not when this code was written, and two workarounds were left behind saying so. `subrun` removed each child's scratch lab with a plain call after the child returned, under a comment admitting the leak that shape has: a child cancelled mid-run re-raises before the cleanup and its lab is left to OS temp-cleaning. A `defer` covers that path. `build_tools` is the other shape, and a plain `defer` is NOT the fix there: the function returns immediately while the session temp dir must outlive it, which is why the cleanup was parked on a never-fed queue waiting for group teardown to cancel it. `TaskGroup::add_defer` says that directly. It also closes a race the parked task had — being a child of the group itself, it could run while a background job was still spilling output, whereas group defers run only after every child has terminated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An A/B measured the two homes against each other. The tool description was given the correct rule for a snippet's import block, and the compiler error it targeted did not move: 60 trials still hit `Package "fs" not found` at the same rate, because the system prompt sat alongside saying the opposite — that importing `"moonbitlang/async"` brings `@fs` and `@stdio` in. The prompt won. So the envelope, the `Cmd` semantics, the spawn allowlist with its MoonBit replacements and the isolation rules move into the prompts, and the contradiction disappears with them since there is now one text rather than two. Five prompts carry it: the main agent's plus explore, audit, review and worker, each of which registers `run_moonbit` and carries its own prompt. Two copies are deliberately not verbatim. The worker's narrows `git` to the working-tree subcommands and refuses `gh`, because the harness owns commits and a worker never touches a remote — the allowlist permits more than that role may use. The read-only roles drop the bullet about editing source with `edit`/`multi_edit`, which they do not have. The description keeps only what cannot move: `## Long-running work` is conditional on a job runtime being wired, and a static prompt cannot know whether one is. Two facts join the prompt on the same evidence. `var x = 0` reached the compiler in 11 of 60 trials, and nothing said MoonBit has no `var`. `String::compare` is shortlex — length first — so `["port","debug"].sort()` returns the array unchanged; 14 of 60 trials spent between 1 and 23 steps rediscovering that, two of them concluding the compiler was broken before accepting it. The policy-refusal guidance shrinks from 600 characters to 239. It no longer lists evasion techniques it tells the reader not to use, and it drops two sentences the same 60 trials contradicted: coaching on reaching a just-built executable, which no trial ever attempted, and "stay inside the workspace", which says nothing to a refused `ls`. Every access refusal on record was a read, and reads are allowed outright now. What is left points at the MoonBit replacement, which is what every measured refusal actually needed. Tests that pinned the moved prose are deleted rather than reproduced five times: asserting a text contains its own words breaks on rewrites and catches no gap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The allowlist carried `rebase --continue|--abort|--skip` and withheld the verb, because `--exec` follows the subcommand and a prefix rule cannot separate `rebase origin/main` from `rebase --exec <anything>`. That reason does not survive this list's own opening paragraph: it is the agent's command vocabulary, not a security perimeter, and `moon run` — already admitted — compiles and runs an arbitrary program. Withholding the verb bought nothing and cost a common operation; a live turn hit the refusal on a plain `git rebase origin/main`. The four prompts that carry the allowlist in prose are updated with it. The worker's is not: its git line is narrowed to working-tree subcommands, since the harness owns commits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rationale had grown to 23 lines of comment over 4 lines of text. Same reasons, said once. Also drops the evasion list from `source_write_denial_feedback`, which still spelled out renames, copies and temp-then-move as things not to try — the same wording already removed from `policy_refusal_feedback`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`rev-list`, `show-ref`, `for-each-ref`, `cat-file` and `check-ignore` are each a subset of something already on the list — `rev-list` of `log`, `cat-file` of `show` — and none can write. They were missing because the list is written a subcommand at a time, so the gaps are the ones nobody happened to reach for yet. `rev-list` is the one with no substitute here: `--count` has no pipe-free equivalent. All five prompts get them, the worker's included — its git line is narrowed to read-only and working-tree commands, which is exactly what these are. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ls-files` reads the index, so a turn that wanted to list a tree at another commit had no spelling short of `cat-file -p` on a hash it must first resolve itself. `ls-tree` is a read like the rest of that group. Also corrects why `difftool`/`mergetool` are absent. The comment claimed that running an external program is their whole job — but `rebase` is admitted and `rebase --exec` runs whatever it is handed, so that is not the line this list draws. They are out because both drive an INTERACTIVE program, which a headless turn cannot answer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ive prompts The EDSL guidance, the spawn allowlist and the isolation rules moved into the five system prompts one revision ago. That copy had already drifted apart inside the single commit that created it — three different amounts of `Cmd` label documentation across five files — and the A/B that motivated the move showed no effect either way. Every role that registers this tool receives its description and nothing else does, so one copy that can go stale beats five that go stale separately. What stays in a prompt is what varies by role: the main agent's and the worker's "edit with the file tools, not by having a snippet rewrite files"; the worker's narrowing of the git set and its "the harness commits your work"; the scout's scratch lab. Everything else is deleted (-899 lines). The description is also shorter than the version that moved out (5262 -> ~4700 chars) even though it now carries more: the example imports `@fs` so the one-import-per-package rule is DEMONSTRATED rather than explained, which retires four bullets, and the paragraph re-listing refused git subcommands is gone — it restated "anything else is REFUSED" and only its point about global options before the subcommand was new, which now sits on the `git` line itself. Separately, `read_only` becomes its own parameter instead of being read off `scratch_dir is Some(_)`. Those are two facts — "you are a read-only role" and "your lab is here" — and conflating them meant a caller that forgot the lab silently lost the kernel profile too. That already happened once: the standalone review CLI shipped a revision unconfined and needed a follow-up fix. The new test pins the case the split buys, `read_only=true` with no lab wired. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Main moved CI from nightly to pre-release, which now carries the `process.allow` policy field this branch needs. The Copilot agent environment and the release build still installed nightly to match the CI this branch had set; both follow. The seed pin came back to main's `0.10.10+f8a486b6f` in the rebase, so the signing list drops `lib/libLLVM.dylib` again — that entry existed because the nightly archive shipped a 100MB LLVM library the pinned build did not. Checked against the installed toolchain, which IS that version: the other 37 listed paths are present and only this one is absent, so it would have named a file that does not exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cca9a1d to
2b12c2e
Compare
`denial_feedback` was a method that ignored `self` and returned the source-write text, so a worker that hit its own boundary — a sibling worktree, the parent checkout, the shared git state — was told "snippets run source-write-readonly by design ... make source changes with the line-anchored `edit` tool". A worker is supposed to write source; its own tree is writable. The advice pointed it at a tool it did not need for a rule that had not fired. The path is reachable: `output_reports_denial` matches on `denial_subjects`, and a worker's subjects are exactly those denied roots, so the classification succeeds and the wrong explanation follows it. The prepared command now carries its own text, set by whichever constructor built it, which is what the method's contract already claimed. Pinned where the worker test already asserts the denial is recognized. Also restores the field order in `SandboxedCommand` and at both construction sites. Moving `denial_subjects` below `plain_source_readonly` changed nothing and made two call sites churn for a reader to scan past. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88dd36fa79
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let saw_binary = exec.had_invalid_utf8() | ||
| if saw_binary { | ||
| exec.wait() |
There was a problem hiding this comment.
When a foreground program emits invalid UTF-8 and then keeps running, wait_or_invalid() returns as soon as the binary data is observed, but this branch then calls exec.wait() without any timeout. This execution has not been adopted into the background runtime, so neither the 300-second foreground limit nor the background reaper can terminate it, allowing one such command to wedge the agent session indefinitely.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a bug, but already in the shell tool.
There was a problem hiding this comment.
Confirmed and fixed in acb5f24, then simplified in c2e8714.
Two things the report did not have. First, as @tonyfettes says, the shell tool has the same gap — and its comment claimed the opposite, that the command "is allowed to finish normally (subject to the existing timeout/output limits)". The timeout wraps only wait_or_invalid, which by then has already returned, so the belief that this was covered is older than this PR.
Second, the deeper cause: wait_or_invalid() followed by if had_invalid_utf8() { wait() } is exactly wait(). Both are side-effect-free polls of the same terminal status; the early return exists for a caller that fails fast on binary output, and this one cannot (reporting with the child alive leaves it running while the deferred cleanup removes its build directory, and the exit code would be invented). So the code asked for a wake it then discarded, and that is what put the rest of the wait outside the deadline. It is now a single exec.wait() under the existing timeout.
On expiry it detaches into a background job rather than being killed, like any other long run — nothing is lost. Pinned by a new test in agent/tool_definition.mbt that injects run_timeout_ms=1500 and runs a snippet emitting b"\xff\xfe" then sleeping 60s; against the old code that test hangs for the full 60s.
| extra_roots=[ | ||
| ..run_cwd.map(c => [c]).unwrap_or([]), | ||
| ..scratch_dir.map(l => [l]).unwrap_or([]), |
There was a problem hiding this comment.
In worker mode on platforms without sandbox-exec (such as Linux), an absolute cwd or a relative path containing .. can resolve outside the worker worktree, and adding that directory to extra_roots explicitly grants the snippet write access there through the wasm policy. A worker can therefore modify a sibling worktree or another repository location without those changes appearing in its validated worker diff; reject worker cwd values whose canonical path is outside worker_root instead of granting them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this, and the framing overstates the delta.
The shell tool did not check this either. Both go through @workspace_path.resolve_cwd, which applies no confinement — its own test pins that an absolute cwd passes through unchanged (resolve("/tmp/work", "/var/log/file") == "/var/log/file") and that .. is only normalized lexically. run_moonbit adds the same two checks shell had (exists, is a directory) and no more.
On the platforms in question the comparison runs the other way. Without sandbox-exec the shell tool gave a worker no confinement at all, so writing a sibling worktree needed no cwd at all. The wasm policy at least binds the snippet, and cwd widens that by one root — a narrowing of a larger pre-existing gap, not a new grant.
It also would not close the hole it names: the policy binds the SNIPPET and stops at the process boundary (wasm_policy.mbt:199-204), so a worker that spawns moon or git is outside it regardless of cwd. The real gap is worker confinement on platforms with no kernel profile, and rejecting a cwd would only look like it addressed that.
Finally the exposure: the desktop app does not currently run on Linux at all, so no worker runs there today.
| // Admitting one of those would make every other entry decorative, since a | ||
| // snippet could reach anything through it — and myshell exists precisely so | ||
| // that no shell sits between the agent and a process. | ||
| ] |
There was a problem hiding this comment.
just integration gates
This closed process allowlist omits just, so after the shell tool is removed an agent cannot run this repository's required just check, just test, or just build gates (and cannot run the editor-specific just gates either). Reimplementing recipes manually is not equivalent because it bypasses the repository's wrappers and any future recipe changes; admit just or retain another way to invoke repository-defined integration gates.
AGENTS.md reference: AGENTS.md:L48-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Admitted in abba289.
The counter-argument was that a recipe is shell and the justfile sits in a workspace the agent may edit, so this admits arbitrary code. It does — but so does rebase --exec, which is already here and which a prefix cannot exclude, since a prefix gates whole tokens and the flag follows the subcommand. Refusing just on that ground would have stated a rule this list does not actually apply. Recipes are not enumerated one by one, for the reason you give: that goes stale the moment a recipe changes.
| "source": { "type": "string" }, | ||
| "target": { | ||
| "type": "string", | ||
| "enum": ["native", "wasm", "wasm-gc", "js", "llvm"], |
There was a problem hiding this comment.
The model-facing schema advertises native as a valid target, but @decode.decode deliberately rejects it and reports that only wasm, wasm-gc, js, and llvm are accepted. A schema-driven model can therefore make a valid-looking call that always fails; remove native from this enum so the advertised contract matches the decoder.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in abba289 — native is out of the enum.
Two related things found while checking it. The decoder docstring still said "native is the default because the async IO batteries require it", which contradicted both its own next paragraph and the actual default of wasm; that is gone.
And llvm, which the decoder does accept, does not work on the toolchain this repo pins (0.10.10+f8a486b6f): moon run probe.mbtx --target llvm fails with an internal compiler error, Sys_error(".../lib/core/_build/llvm/release/bundle/v128/v128.mi: No such file or directory") — the llvm core bundle is not part of that install. wasm-gc and js both run fine. So the schema and the decoder now agree, but llvm remains advertised and broken on the default toolchain; leaving that call to the repo owner, since it works on an install that does carry the llvm core.
`just` is admitted because a repository's check/test/build gates live in its justfile, and reimplementing those recipes by hand runs something else and goes stale the moment a recipe changes. A recipe is shell, so this does admit arbitrary code — but `rebase --exec` already does, and a prefix gates whole tokens, so refusing `just` on that ground would state a rule the list does not apply. `git config` is dropped for the opposite reason: it writes what LATER commands do (`core.pager`, `alias.*`, `protocol.ext.allow`, which turns `ls-remote 'ext::sh -c ...'` into a process launcher), so it undoes the `git -c` refusal one command later — and that refusal is the property this file reasons about and a test pins. Two statements were wrong. The description said "there is no `Pipeline`"; myshell exports it (`Pipeline::Pipeline`, `output`, `each_line`), and the original wording was a prohibition, not a claim about the API. Since the description is now the single contract for the command surface, a false statement in it is the one thing a model cannot check cheaply. And the schema advertised `target: "native"`, which the decoder rejects outright — the value a model reaches for first, since it was the documented default before this series. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abba2893fa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| as its own `Cmd`. `add`, non-force `remove`, and `prune` are allowed; | ||
| `remove --force` is not — commit or discard the worktree's changes with |
There was a problem hiding this comment.
When the agent follows the preceding recommendation and creates a worktree, both advertised cleanup commands are refused: spawnable_commands only admits git worktree list|add, not remove or prune. I checked git worktree -h, whose usage includes git worktree prune ... and git worktree remove [-f] <worktree>, so these are distinct prefixes that the current policy cannot match; the agent is therefore left with the added worktree despite being explicitly told cleanup is allowed. Either admit the safe forms with appropriate enforcement or remove this instruction.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — the allowlist has worktree list and worktree add only, so both cleanup commands the prompt advertises are refused.
Worth naming why the text reads that way. It says "add, non-force remove, and prune are allowed; remove --force is not", which was true under the shell tool: it parsed the command, so it could distinguish remove from remove --force. A policy prefix cannot express that distinction at all — the flag follows the subcommand, and a prefix gates whole tokens, so ("git", ["worktree", "remove"]) would admit the force form with it. The prompt is a leftover from a mechanism that no longer exists, not a policy oversight.
So it is one of: admit worktree remove/prune and drop the non-force claim from the prompt, or leave them out and tell the agent cleanup is not available from a snippet. Pending a decision from the repo owner; not fixed yet.
There was a problem hiding this comment.
Resolved across 616dde8 and 170bade, splitting the two commands rather than treating them alike.
remove stays out. A prefix cannot express the non-force form the prompt claimed, and its target is any worktree of the repository — including one with someone else's uncommitted work — not only the one the agent made.
prune is admitted. It takes no argument naming a worktree (git worktree prune [-n] [-v] [--expire <expire>]) and acts only on records whose directory is already gone, so it cannot reach a live tree. That also gives the agent a cleanup path that does not need remove: delete its own worktree's directory with @fs.rmdir — inside the workspace, so the policy allows it — then prune the stale record. The prompt describes that path now.
| @run_moonbit.definition(workspace_root~, bg_runtime~, job_dir?=spill_dir), | ||
| @job_output.definition(bg_runtime), | ||
| @job_stop.definition(bg_runtime), |
There was a problem hiding this comment.
After exposing job_output in place of shell_output, eval/bgjobs_capability/main.mbt still asks for forbidden sh -c commands at lines 41 and 119 and recognizes results only when tool_name is shell/shell_output at lines 64, 77, and 90. A correct run using the new run_moonbit/job_output surface can therefore never satisfy the retrieval/overlap matcher, while a foreground run_moonbit call is not recognized as blocking, so this experiment now reports misleading scores instead of measuring background-job behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, not fixed yet. eval/bgjobs_capability/main.mbt still asks for sh -c 'sleep 20; ...' (lines 41 and 119), which the allowlist now refuses outright, and matches on tool_name shell/shell_output (lines 64, 77, 89-90), which no longer exist — so the experiment cannot score a correct run either way. It needs the prompts rewritten around a run_moonbit snippet and the matchers pointed at run_moonbit/job_output.
…ct the output cap
`wait_or_invalid` returns as soon as non-UTF-8 bytes appear, with the child still
running, so the code waited out the real exit before reporting. That second wait
had no deadline of its own: a program that emitted one invalid byte and then kept
going — a watcher, a test printing raw bytes — held the turn for its entire
lifetime, and the detach branch was skipped for exactly this case. The shell tool
had the same gap, and its comment claimed the opposite ("subject to the existing
timeout"), so the belief that it was covered is older than this series.
Both waits now share ONE deadline. Giving the second its own budget would have
doubled the worst case instead of fixing it, and expiry needs no special handling
here — it detaches like any other long run, so nothing is lost. That also retires
the `!saw_binary` guard on the detach branch, which was only ever reachable if a
byte landed exactly on the deadline.
Separately, `OutputCapBytes` documented itself as a truncation limit. The
foreground path spawns with `kill_when_full`, so passing 48KB STOPS the program;
the reporting code already says so and only the docstring disagreed. The tool
description said to redirect "past a few hundred KB", which is well past the
point of being killed — it now names the real number.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…iscards The retained path called `wait_or_invalid` and then, on binary output, `wait`. Neither has side effects and both poll the same terminal status, so the pair was exactly `wait()` — the early return exists for a caller that FAILS FAST on non-UTF-8 bytes, and this one cannot: reporting with the child still running leaves it going while the deferred cleanup removes its build directory, and the exit code would be invented. Asking for a wake it then discards is what put the rest of the wait outside the deadline in the first place. `agent_tool/shell` still calls it the same redundant way; that package is no longer registered as an agent tool but is live for the TUI, auto_check and the eval harness, so it is left alone here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2e8714beb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #| mkdir -p → @fs.mkdir(d, recursive=true) | ||
| #| test -f → @fs.exists(p); test -d → @fs.kind(p) is Directory | ||
| #| echo/printf → println | ||
| #| rm/mv/cp → the `remove` tool for files you made; @fs otherwise |
There was a problem hiding this comment.
When the agent needs to remove or move a pre-existing file, this guidance explicitly sends it to @fs; the new policy grants snippets write access to the workspace, so @fs.remove_file/rename operations can bypass remove's created-this-session provenance check and irreversibly discard user work that the guarded tool intentionally refuses to delete. Do not advertise @fs as the fallback for destructive operations; keep deletions and moves behind provenance-aware file tools.
Useful? React with 👍 / 👎.
… eval The main prompt told the agent that `git worktree remove` and `prune` were available, and that only `remove --force` was not. That was true under the shell tool, which parsed the command and could tell the two apart. A policy prefix cannot: the flag follows the subcommand and a prefix gates whole tokens, so admitting `remove` would admit the force form with it. And the target of either is any worktree of the repository, not only the one the agent made — including a worktree with someone else's uncommitted work in it. So they stay out, and the prompt now says so and asks the agent to name what it left behind. `eval/bgjobs_capability` had stopped measuring anything. Its prompts asked for `sh -c 'sleep 20; ...'`, which the allowlist refuses outright, and its matchers keyed on `tool_name` `shell`/`shell_output`, neither of which exists any more — so a correct run could not satisfy the retrieval or overlap checks, and a blocking foreground run was not recognized as blocking. The scenarios now describe the slow check and let the agent stand it in with a snippet, which is what the tool surface makes natural, and the matchers point at `run_moonbit` and `job_output`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit refused `prune` alongside `remove`, on the grounds that their target is any worktree of the repository. That is true of `remove` and not of `prune`: `prune` takes no argument naming a worktree, and acts only on records whose DIRECTORY is already gone (`git worktree prune [-n] [-v] [--expire]`), so it cannot reach a live tree or the work in one. That also completes a cleanup path the agent can actually follow without `remove`: delete its own worktree's directory with `@fs.rmdir` — inside the workspace, so the policy allows it — then prune the record that `git worktree list` would otherwise keep reporting. The prompt describes that path now instead of telling the agent to leave the worktree behind. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 170bade142
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub fn output_reports_policy_refusal(output : String) -> Bool { | ||
| output.contains(PolicyBlockedAccess) || output.contains(PolicyRefusedSpawn) |
There was a problem hiding this comment.
When a successful snippet prints either marker literally, this substring-only check fabricates a sandbox denial and causes run_moonbit or job_output to return an error. This occurs during ordinary repository inspection here: an allowed rg "Sandbox policy blocked" prints the constant in this file, after which refusal_guidance classifies the exit-0 run as refused and gives the model misleading recovery guidance. Match the runtime's diagnostic line structure rather than any occurrence in command output.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Will be improved once moonrun have better refusals for process.spawn.
| is_error=exit_code != 0 || | ||
| sandbox_denied is Some(_) || | ||
| output_limit_reached, |
There was a problem hiding this comment.
When a foreground snippet emits invalid UTF-8 but exits 0 without hitting another limit, this branch appends the lossy-rendering warning but still returns is_error=false because saw_binary is omitted from the condition. The same execution moved to the background is explicitly treated as an error by job_output, so foreground and background runs disagree and callers may accept corrupted command output as a successful result; include saw_binary in the error predicate.
Useful? React with 👍 / 👎.
The policy granted writes to the workspace root, the run's build directory and whatever `cwd` the model named. The workspace grant was the one that mattered: moonrun gates `remove_path`, `rename_path` and `rmdir_path` on `fs.write` just like `open`, so a snippet could delete or move any file in the tree — including the files `remove` refuses to touch because the agent did not create them. The description sent it there in as many words: "rm/mv/cp → ... @fs otherwise". Write roots are now the snippet's own temp directory and, for a read-only role, its scratch lab. Nothing else: - The workspace is gone because a snippet has no business writing it. The file tools make edits, and the commands whose job is to rewrite source — `moon fmt`, `moon info`, `moon test --update`, `git checkout` — are CHILD processes, which no `fs` rule of this policy reaches. That asymmetry is why this can be strict where the `sandbox-exec` profile could not: the kernel profile covers the whole process tree and would have taken those commands with it. - The build directory narrows to the `tmp` subdirectory inside it. `moon` builds there as a child, so the snippet only needs the part `TMPDIR` points at. - `cwd` is gone: naming where a program runs should not also grant writing there. Reads stay open, so a snippet still works in a directory it may not modify. - The lab stays. explore/review/audit hold `read`, `run_moonbit` and a submit tool and no editing tool at all, so the lab is the only place they can build the experiment their prompts tell them to run. This also holds on every platform, unlike the macOS-only profile, and it is what answers the worker `cwd` question: a worker's snippet now writes nothing, its own worktree included, so where it points `cwd` grants nothing either. The cost is real and narrower than it looks: a snippet can no longer park large command output in a workspace file across calls. It can redirect into its own temp directory and read back what it needs, filter before printing, or hand the result to `write` — and a background job's spill file is written by the agent process, which is outside this policy. Two tests pinned the old contract and now pin the new one, including the deletion case that motivated the change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Draft: this removes the agent's shell. Worth deciding as a direction before
reviewing line by line.
What changes
Commands are spawned from
run_moonbitprograms throughbobzhang/myshell, a shell-freeprocess EDSL.
Cmd("moon", ["check", "--diagnostic-limit", "5"])keeps theexecutable and the argument vector separate and passes every argument
literally, so
|,>,&&,$()and*have no meaning and there is noquoting to get wrong. Pipes and control flow become ordinary MoonBit: capture
out.stdoutand filter it in code, which is also what replaces grep/sed/awk.What this deletes is a parsing layer and the string heuristics that defend it.
What it keeps is everything the shell tool actually delivered:
run_moonbit'srun_in_background, watched byjob_output/job_stop— renamed fromshell_output/shell_stop,since a registry with no shell should not advertise shell names. The job
runtime, spill dir, and completion-notice steer are wired exactly as before,
and a detached snippet keeps its directory until the session scope ends.
registered
run_moonbitbeside shell and never named shell in their prompts,so the swap is mostly a deletion — except for the lab, the one writable
directory a read-only child may fabricate source in to verify a claim.
run_moonbitnow takes it, building the snippet inside the lab so theprofile's single writable subtree covers both.
moon testcycles, not just compute.run_moonbitstill refuses native FFI. It no longer refuses spawningprocesses — that is now the point of the tool.
Every agent, including the worker
explore, review, and audit already registered
run_moonbitbeside shell andnever named shell in their prompts, so those were mostly deletions — except for
the scratch lab, the one writable directory a read-only child may fabricate
source in to verify a claim.
run_moonbitnow takes it, building the snippetinside the lab so the profile's single writable subtree covers both.
The subtask worker moves too, under the same kernel worker profile the
shell tool used: in worker mode that profile REPLACES the workspace
source-write one rather than adding to it, because a worker is supposed to
write source — just only its own. What is lost on platforms without
sandbox-execis the shell-text static floor (cd/git -C/--work-treeoperands parsed out of a command string), which has no analogue for an
arbitrary program. It caught accidents rather than adversaries — its own docs
admit variables and substitutions pass through — and the platform-independent
layers are unchanged: the file tools'
write_scopechecks their paths againstthe worker's allowed paths, and the controller validates the result from git
evidence before merging. Non-macOS was already documented as best-effort for
the whole sandbox stack.
Tests pin both profiles end to end: a lab write succeeds while the workspace's
own source is denied; a worker writing its own
.mbtsucceeds while a writeinto a sibling worktree is denied and that sibling's file is unchanged on disk.
No production path registers the shell tool any more. What still uses the
agent_tool/shellpackage iseval/tool_harness(which exercises every tooldefinition) and the TUI's
!command throughcollect_shell_output— a usertyping a command, not an agent tool. Pruning the now-unreachable tool and its
command-policy guards is a follow-up, kept out of this diff on purpose.
The one real regression
The shell tool could statically recognize trusted source-writing moon commands
(
moon fmt,moon info,moon add,moon test --update) and run them exemptfrom the sandbox. An arbitrary snippet cannot be classified that way, so under
this PR those commands are denied and source changes go through the file
tools. The system prompt says so and tells the agent to report what it could
not run.
Worth weighing against how well that exemption works today: across 10
sandboxed baseline trials the shell arm hit 33 sandbox denials, because the
model writes
moon add x; echo ===; moon add yandmoon fmt 2>&1 | head -5rather than bare commands, and those fall off the trusted list. The exemption
fires far less often than its design implies.
Evidence
Flash, cold-start TOML-parser-CLI task, harness-validated scoring
(
moon check+moon test+ three CLI probes), 10 trials per arm. Both armssandboxed — the posture this PR ships:
The step and request gap is the mechanism: one snippet runs several commands
and post-processes their output in MoonBit, where the shell arm spends a
round-trip per command — and, sandboxed, extra round-trips recovering from
denials.
A second pair of runs with the sandbox off on both sides (also n=10) came
out near parity: 10/10 both, steps 68.9 vs 58.1, wall 15.6 vs 18.2 min. So the
shell arm is the one that suffers under the sandbox, not this one.
Output quality is indistinguishable. Every parser these 20 sandboxed trials
produced was scored against Python
tomllibon a differential suite — 17in-scope valid documents (semantic JSON equality), the 7 invalid inputs the
task mandates, and 6 robustness inputs (100KB line, 19-level nesting, control
bytes, 2000 keys):
No parser in either arm panicked or hung. Both arms miss exactly one thing —
multi-line arrays with a trailing comma (7 parsers each), which the task
statement never names; one myshell trial also accepted a redefined table.
Caveats
is a joint change, not an isolated tool swap.
run_moonbitcalls — real friction thatnever became a runaway loop here, but it is the cost to watch.
@myshell.Pipelineis unusable with slow producers (upstreammoonbitlang/async#553), so
the prompt tells the agent to capture and filter in MoonBit instead. That is
also the more interesting direction.
!command still runs through@shell.collect_shell_output— thatis a user typing a command, not an agent tool, and is untouched.
Full native suite: 3446 tests pass.
🤖 Generated with Claude Code