Repository navigation
feat(FG-233): step runners as WASM modules sealed into the workflow, with granted host imports - #269
Conversation
…t imports Spec first: host.md §3 `component`, the runner world (contracts/wit/runner/runner.wit), the grant list in settings.json and the new codes. The build seals a component action's module as an asset and refuses a runner outside the world or a grant spore does not offer; the host runs start, probe and stop through Wasmtime in the kernel's sandbox, enforcing the action's grants on every host import.
…rt the runner world H41-H43: a runner's detached process outlives its worker and another worker probes and stops it through the runner; every ungranted import fails the step naming the grant; start and validate refuse a runner they could not run. spore capabilities lists componentWorlds.
…new failpoint, validate H41 through the binary: the runner's process outlives a SIGKILLed worker, and a second spore probes it and its cancel stops it through the runner. H44: a crash at host.component.after-spawn and after-start leaves the command step done once and the run completes. spore validate refuses a runner outside the world and a grant spore does not offer. Glossary, layout and the known limit of handle checks.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: d5182292a1
ℹ️ 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 timeout = crate::external::millis(action, "timeoutMs", "timeoutMs") | ||
| .map_err(invalid)? | ||
| .map_or(START_TIMEOUT, Duration::from_millis); |
There was a problem hiding this comment.
When a component action configures "async": {"timeoutMs": ...}, read looks for timeoutMs on the action itself, so every documented configuration silently uses the 60-second default. Consequently short safety limits are ignored and starts intended to run longer than a minute are cut off early; the timeout needs to be read from the action's async object.
Useful? React with 👍 / 👎.
| .ctx | ||
| .files | ||
| .as_ref() | ||
| .and_then(|f| f.program(argv0)) | ||
| .ok_or_else(|| format!("`{argv0}` is not a file bundled with the action")); |
There was a problem hiding this comment.
For a valid grant such as ./tools/../bin/job, the builder normalizes and seals the asset as bin/job, but this lookup passes the original string to StepFiles::program, which performs an exact membership check for tools/../bin/job. The workflow therefore validates and builds successfully but start-process reports that the granted program was not bundled; normalize the path consistently with the build before consulting the asset set.
Useful? React with 👍 / 👎.
| if let Some(end) = reply.iter().position(|b| *b == b'\n') { | ||
| reply.truncate(end); | ||
| return Ok(reply); | ||
| } | ||
| if reply.len() > MAX_REPLY { |
There was a problem hiding this comment.
If a socket sends more than 1 MiB and places its first newline after that boundary, this branch truncates at the newline and returns the oversized reply before the subsequent MAX_REPLY check runs. This violates the host import's 1 MiB contract and allows replies up to the excess contained in the final read buffer; check the newline position or accumulated size before returning.
Useful? React with 👍 / 👎.
| if !exports.iter().any(|e| e == STEP_EXPORT) { | ||
| return Err(Problem::Invalid(format!( | ||
| "it does not export `{STEP_EXPORT}`" | ||
| ))); |
There was a problem hiding this comment.
A component can export an interface named runspore:runner/step@0.1.0 whose functions have incompatible parameters or results, and this name-only test still accepts it because wasmparser::Validator checks internal well-formedness rather than conformance to the runner WIT world. The build therefore accepts and seals a runner that RunnerPre::new later rejects during artifact validation or start; compare the exported interface type with the expected world rather than checking only its name.
Useful? React with 👍 / 👎.
| input: Value, | ||
| start_key: &str, | ||
| ) -> Result<StartOutcome, HostError> { | ||
| component::check_runners(workflow_json, None)?; |
There was a problem hiding this comment.
When an Engine is constructed with a registry that does not provide the component kind, starting a package containing a component action returns component.no-artifact here before check_workflow can report the actual action.kind-unregistered shortfall. The message incorrectly tells callers to supply an artifact even though that engine still cannot run the action; start_artifact has the analogous ordering for invalid modules, so workflow capability validation should precede the hard-coded runner checks.
Useful? React with 👍 / 👎.
Closes #233
Adds a
componentaction kind. Its start, probe and stop are a step runner: a WebAssembly component of the new worldrunspore:runner/runner@0.1.0(contracts/wit/runner/runner.wit), sealed into the workflow artifact as the action'smoduleand run through Wasmtime in the kernel's sandbox. The runner reaches the machine only through host imports (start a program detached, check whether a process is alive, signal it, one request over a Unix socket, sleep, read-only file and folder reads), and each import is checked against the action'sgrants(programs,sockets,folders); an ungranted call fails the step withcomponent.not-granted, naming the grant and the value. The spec comes first in the same PR (host.md §3component, settings fields in dot.md, artifact.md, cli.md, new codes and catalogue examples).spore validaterefuses a runner that imports outside the world or is not a runner, and a grant spore does not offer.startrefuses a runner it could not load.spore capabilitiesnow listscomponentandcomponentWorlds. Recovery reuses the async-command path: the handle is stored withhold_attempt, and any later worker probes and stops through the runner. Two new failpoints,host.component.after-spawnandhost.component.after-start, are crash-tested (H44), and a SIGKILLed-worker test (H41) shows a second spore probing and stopping the detached process. The full workspace suite passes; the three tests that failed during the run did so because of commits made mid-run (two version checks) and a missing release-please entry for the new crate, and all three pass on a re-run. Choices made where the ticket left room: a new action kind rather than the reservedcomponentform of probe/watch/stop (that form is still refused); acomponentaction is always async, and acommand-form probe, watch or stop given inasyncreplaces the runner's own export; known limit, written into the spec: the host checks a handle's program and start time but cannot prove the runner created it. Signing handles is left for later. The linked design doc (Max-Levitskiy/skills claude-step-runner-design.md) was not reachable, so the design follows the ticket body and the repo's specs.Built by the
build-0-2runspore run; ticket runrun_0caab19069ccc1d821e986e2ed76893f.