From 27c10efa9da8625b64b90f1fbdc3a499bc067f63 Mon Sep 17 00:00:00 2001 From: joctaTorres Date: Thu, 9 Jul 2026 19:19:48 -0300 Subject: [PATCH] feat(engine): thread per-step env through rex runtimes + gate/mark agent-cmd override Phase 1 of the engine-runtime-hardening batch. - Both rex runtimes export AgentSpawnRequest.env before launching the agent, via one shared spawn-command helper (overlay merge semantics). - RATCHET_BATCH/EVAL_AGENT_CMD overrides now print a notice, set agentOverride in --json, and stamp via:env-override provenance; one shared spawn-request helper dedups the override gate across engine/judge/mutation-harness. Closes #89 Closes #80 Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_014hATZnT1NQP1STMr6pVxtm --- .../.ratchet.yaml | 3 + .../override-notice.feature | 30 ++ .../override-provenance.feature | 24 ++ .../shared-spawn-helper.feature | 21 + .../gate-and-mark-agent-cmd-override/plan.md | 65 ++++ .../.ratchet.yaml | 3 + .../env-reaches-spawned-agent.feature | 25 ++ .../env-serialization-safety.feature | 22 ++ .../thread-env-through-rex-runtimes/plan.md | 52 +++ docs/engine/agent-runtime.md | 359 +++++++++++++++++- docs/eval-mutation-harness.md | 32 +- src/commands/batch/apply.ts | 86 ++++- src/commands/batch/report.ts | 28 +- src/commands/eval/run.ts | 26 +- src/core/batch/engine/agent.ts | 255 +++++++++++-- src/core/batch/engine/context.ts | 9 + src/core/batch/engine/contract.ts | 7 + src/core/batch/engine/engine.ts | 163 +++++--- src/core/batch/engine/index.ts | 7 + .../engine/runtime/rex-remote-runtime.ts | 46 ++- .../engine/runtime/rex-sidecar-runtime.ts | 107 +++++- src/core/batch/engine/runtime/sidecar.py | 181 ++++++++- .../batch/engine/runtime/spawn-command.ts | 52 +++ src/core/batch/journal.ts | 24 ++ src/core/eval/execute.ts | 18 + src/core/eval/judge.ts | 35 +- src/core/eval/mutation-harness.ts | 35 +- src/core/eval/run.ts | 8 + test/batch-engine/agent.test.ts | 160 +++++++- .../engine-agent-override.test.ts | 79 +++- test/batch-engine/rex-remote-runtime.test.ts | 211 +++++++++- test/batch-engine/rex-sidecar-runtime.test.ts | 348 ++++++++++++++++- test/batch-engine/spawn-command.test.ts | 116 ++++++ test/commands/apply.test.ts | 2 +- test/commands/batch/apply.test.ts | 63 +++ test/commands/batch/report.test.ts | 61 +++ test/commands/eval/run.test.ts | 68 ++++ 37 files changed, 2633 insertions(+), 198 deletions(-) create mode 100644 .ratchet/changes/gate-and-mark-agent-cmd-override/.ratchet.yaml create mode 100644 .ratchet/changes/gate-and-mark-agent-cmd-override/features/agent-cmd-override/override-notice.feature create mode 100644 .ratchet/changes/gate-and-mark-agent-cmd-override/features/agent-cmd-override/override-provenance.feature create mode 100644 .ratchet/changes/gate-and-mark-agent-cmd-override/features/agent-cmd-override/shared-spawn-helper.feature create mode 100644 .ratchet/changes/gate-and-mark-agent-cmd-override/plan.md create mode 100644 .ratchet/changes/thread-env-through-rex-runtimes/.ratchet.yaml create mode 100644 .ratchet/changes/thread-env-through-rex-runtimes/features/rex-env-threading/env-reaches-spawned-agent.feature create mode 100644 .ratchet/changes/thread-env-through-rex-runtimes/features/rex-env-threading/env-serialization-safety.feature create mode 100644 .ratchet/changes/thread-env-through-rex-runtimes/plan.md create mode 100644 src/core/batch/engine/runtime/spawn-command.ts create mode 100644 test/batch-engine/spawn-command.test.ts diff --git a/.ratchet/changes/gate-and-mark-agent-cmd-override/.ratchet.yaml b/.ratchet/changes/gate-and-mark-agent-cmd-override/.ratchet.yaml new file mode 100644 index 0000000..4901dcc --- /dev/null +++ b/.ratchet/changes/gate-and-mark-agent-cmd-override/.ratchet.yaml @@ -0,0 +1,3 @@ +schema: ratchet +created: 2026-07-09 +standards: [delegated-lifecycle, documentation, testing] diff --git a/.ratchet/changes/gate-and-mark-agent-cmd-override/features/agent-cmd-override/override-notice.feature b/.ratchet/changes/gate-and-mark-agent-cmd-override/features/agent-cmd-override/override-notice.feature new file mode 100644 index 0000000..83fffc6 --- /dev/null +++ b/.ratchet/changes/gate-and-mark-agent-cmd-override/features/agent-cmd-override/override-notice.feature @@ -0,0 +1,30 @@ +Feature: Agent-cmd override prints a one-line notice + As a ratchet operator + I want an active RATCHET_BATCH_AGENT_CMD / RATCHET_EVAL_AGENT_CMD to be loudly surfaced + So that a leftover test override can never silently replace the configured coding agent + + Scenario: batch apply text output carries the override notice + Given RATCHET_BATCH_AGENT_CMD is set to a stand-in command + When `ratchet batch apply` runs a step that spawns the agent + Then the rendered step result includes the one-line notice "⚠ agent overridden by RATCHET_BATCH_AGENT_CMD" + + Scenario: batch apply --json output carries agentOverride + Given RATCHET_BATCH_AGENT_CMD is set to a stand-in command + When `ratchet batch apply --json` runs a step that spawns the agent + Then the emitted step-result JSON has an "agentOverride" field set to true + + Scenario: eval run text output carries the override notice + Given RATCHET_EVAL_AGENT_CMD is set to a stand-in command + When `ratchet eval run` executes + Then the scorecard output includes the one-line notice "⚠ agent overridden by RATCHET_EVAL_AGENT_CMD" + + Scenario: eval run --json output carries agentOverride + Given RATCHET_EVAL_AGENT_CMD is set to a stand-in command + When `ratchet eval run --json` executes + Then the emitted run JSON has an "agentOverride" field set to true + + Scenario: no override means no notice and no flag + Given neither RATCHET_BATCH_AGENT_CMD nor RATCHET_EVAL_AGENT_CMD is set + When `ratchet batch apply` or `ratchet eval run` executes + Then no override notice line is printed + And the --json output carries no "agentOverride" field diff --git a/.ratchet/changes/gate-and-mark-agent-cmd-override/features/agent-cmd-override/override-provenance.feature b/.ratchet/changes/gate-and-mark-agent-cmd-override/features/agent-cmd-override/override-provenance.feature new file mode 100644 index 0000000..6771303 --- /dev/null +++ b/.ratchet/changes/gate-and-mark-agent-cmd-override/features/agent-cmd-override/override-provenance.feature @@ -0,0 +1,24 @@ +Feature: Override provenance is stamped on journal entries and run records + As a ratchet operator auditing a batch or eval run + I want every journal entry and run record produced under an agent-cmd override marked "via: env-override" + So that synthetic runs are distinguishable from real agent work after the fact + + Scenario: engine transition-outcome journal entry is stamped + Given RATCHET_BATCH_AGENT_CMD is set to a stand-in command + When the engine spawns a step and records its transition-outcome journal entry + Then the appended journal entry carries "via": "env-override" + + Scenario: agent-reported journal entries are stamped + Given a `ratchet batch report` invocation whose process environment carries an active RATCHET_BATCH_AGENT_CMD + When it appends a progress, blocker, needs-input, or completion entry + Then the appended journal entry carries "via": "env-override" + + Scenario: eval run record is stamped + Given RATCHET_EVAL_AGENT_CMD is set to a stand-in command + When `ratchet eval run` persists the run under .ratchet/evals/runs/ + Then the persisted run record carries "via": "env-override" + + Scenario: work produced without an override stays unstamped + Given neither RATCHET_BATCH_AGENT_CMD nor RATCHET_EVAL_AGENT_CMD is set + When journal entries and eval run records are produced + Then none of them carry a "via" field diff --git a/.ratchet/changes/gate-and-mark-agent-cmd-override/features/agent-cmd-override/shared-spawn-helper.feature b/.ratchet/changes/gate-and-mark-agent-cmd-override/features/agent-cmd-override/shared-spawn-helper.feature new file mode 100644 index 0000000..41b6e5d --- /dev/null +++ b/.ratchet/changes/gate-and-mark-agent-cmd-override/features/agent-cmd-override/shared-spawn-helper.feature @@ -0,0 +1,21 @@ +Feature: One shared helper owns override-aware spawn-request construction + As a ratchet maintainer + I want the env-override gate and spawn-request construction to exist in exactly one helper + So that the batch engine, the eval judge, and the mutation harness cannot drift apart (the #67 triplication) + + Scenario: the three spawn seams share one override gate + Given the batch engine, the eval judge, and the mutation harness each build an agent spawn request + When their override env var is set to a stand-in command + Then each produces the same `bash -c ` request shape through the shared helper + And each reports that the agent was overridden + + Scenario: a whitespace-only override is inactive + Given RATCHET_BATCH_AGENT_CMD is set to only whitespace + When a spawn request is built + Then the configured adapter path is used + And the agent is not reported as overridden + + Scenario: an override-built request threads env like any other request + Given a spawn request built under an active override with a per-step env var + When a rex runtime builds the launch command for it + Then the launch command exports the per-step env var before invoking the override command diff --git a/.ratchet/changes/gate-and-mark-agent-cmd-override/plan.md b/.ratchet/changes/gate-and-mark-agent-cmd-override/plan.md new file mode 100644 index 0000000..c6a3d85 --- /dev/null +++ b/.ratchet/changes/gate-and-mark-agent-cmd-override/plan.md @@ -0,0 +1,65 @@ +# gate-and-mark-agent-cmd-override + +## Why + +`RATCHET_BATCH_AGENT_CMD` / `RATCHET_EVAL_AGENT_CMD` are read unconditionally at spawn time in production code, in three drifting copies (`src/core/batch/engine/engine.ts:835`, `src/core/eval/judge.ts:274`, `src/core/eval/mutation-harness.ts:154`): any leftover value — from an eval session, CI, a `.envrc` — silently replaces the configured coding agent with `bash -c `, with no console signal and journal entries/run records indistinguishable from real agent work (issue #80; the audit hole behind #78's self-attested completions). This change makes an active override loud (one-line notice + `agentOverride: true` in `--json`), auditable (`via: "env-override"` provenance on journal entries and eval run records), and single-sourced (one shared override-aware spawn-request helper, coordinating with the #67 triplication). + +## What Changes + +- A shared override-aware spawn-request helper in `src/core/batch/engine/agent.ts` (exported through `src/core/batch/engine/index.ts`, which `judge.ts`/`mutation-harness.ts` already import from): `activeAgentCmdOverride(envVar, env)` (trimmed non-empty value or `undefined`), `buildAgentSpawnRequest({ overrideEnvVar, instructions, cwd, env, buildAdapterRequest })` returning `{ request, agentOverride }`, `agentOverrideNotice(envVar)` (the `⚠ agent overridden by ` line), and an `ENV_OVERRIDE_PROVENANCE = 'env-override'` constant. All three spawn seams — `engine.ts` `buildSpawnRequest`, `judge.ts` `buildVoteRequest`, `mutation-harness.ts` `buildSeedRequest` — delegate their override branch to it. Implements `features/agent-cmd-override/shared-spawn-helper.feature`. +- `JournalEntry` (`src/core/batch/journal.ts:55`) gains an optional `via?: 'env-override'` field; the engine stamps it on the transition-outcome entry it appends (`engine.ts:807`) when the step's spawn used the override, and `ratchet batch report` (`src/commands/batch/report.ts:84-106`) stamps every entry it appends when its own process environment carries an active `RATCHET_BATCH_AGENT_CMD` (the spawned stand-in inherits the var, so stub-reported completions become auditable — the #78 hole). Implements `features/agent-cmd-override/override-provenance.feature`. +- `StepResult` (`src/core/batch/engine/contract.ts:230`) and `EngineStepOutcome` gain an optional `agentOverride?: true`; `batch apply --json` therefore carries the field verbatim (`renderResult`, `src/commands/batch/apply.ts:1085`), and the text path prints the one-line notice when set. Implements `features/agent-cmd-override/override-notice.feature`. +- `EvalRun` (`src/core/eval/run.ts:63`) gains an optional `via?: 'env-override'` stamped at run persistence when `RATCHET_EVAL_AGENT_CMD` is active; `eval run --json` adds a top-level `agentOverride: true` and the text scorecard prints the notice line (`src/commands/eval/run.ts:76-97`). +- Both rex runtime test suites assert an override-built request (`bash -c ` + per-step env) threads env through the launch command like any adapter-built request, so the phase proof-of-work run (`npm test -- test/batch-engine/rex-sidecar-runtime.test.ts test/batch-engine/rex-remote-runtime.test.ts`) genuinely covers the override path's composition with #89's env threading. +- Reference docs updated in the same change: `docs/engine/agent-runtime.md` (override seam: notice, `agentOverride` JSON field, `via: env-override` journal provenance) and `docs/eval-mutation-harness.md` (its two existing `RATCHET_EVAL_AGENT_CMD` mentions gain the notice/provenance contract); `README.md` checked for affected surfaces. +- Field additions are optional and absent when no override is active — no journal/run-record migration, byte-identical output for override-free runs. Not breaking. +- Out of scope (kept thin per the vertical-slice strategy): the issue's "consider an explicit opt-in flag / NODE_ENV gate" (proposal item 3) — the definition of done requires notice + provenance + shared helper, and an opt-in gate would break every existing e2e/eval harness invocation; revisit if #78's corroboration work (phase 2) still needs it. Narrowing what env reaches the agent stays #86. + +## Design + +**One helper, three seams (`delegated-lifecycle`, #67 coordination).** The override gate moves into `agent.ts` — already the home of `AgentSpawnRequest`, the adapter registry, and `resolveAdapter` — as `buildAgentSpawnRequest`, which checks `activeAgentCmdOverride(overrideEnvVar, process.env)` and returns either the `bash -c ` request (`agentOverride: true`) or `buildAdapterRequest()`'s result (`agentOverride: false`). Site-specific adapter resolution (the engine's stage-map/spec/`emitsStreamJson` logic, the eval side's bare `resolveAdapter(agentName)`) stays at each call site inside the `buildAdapterRequest` closure — the helper owns only the shared override semantics (trim check, request shape, flag), so the gate exists in exactly one place without flattening genuinely different adapter paths. This satisfies the phase criterion "spawn-request construction lives in one shared helper" at the engine layer; `runtime/spawn-command.ts` (the sibling change's shell-serialization helper) stays a separate module because it operates at a different layer (rex launch-command text, consumed by both runtimes), and merging the two would couple engine-level request construction to rex-only shell details. Per the `delegated-lifecycle` standard the helper is purely mechanical — it carries no instruction text, no standards loading, and no done-semantics; lifecycle authorship stays in the shared skill/workflow layer untouched. + +**Notice rides the result, not a side-channel print.** The engine sets `agentOverride: true` on the step outcome; `toStepResult` carries it to `StepResult`. `renderResult` in `--json` mode then emits the field with zero extra plumbing (it already `JSON.stringify`s the whole result), and in text mode prints `⚠ agent overridden by RATCHET_BATCH_AGENT_CMD` as its first line for the step. This keeps `--json` output a single well-formed document (no notice line interleaved into JSON stdout) and the notice appears exactly when a spawn actually ran under the override. Pre-spawn parks (`notAdvanced`) print no notice — nothing was spawned. `eval run` follows the same shape: `executeRun` records the override state once for the run (`EvalRun.via`), and the command layer adds `agentOverride: true` to the JSON payload / a notice line atop the text scorecard. The eval stamp/notice keys on the var being ACTIVE for the run (deterministic, documented), not on whether a given case happened to spawn — a run executed with the seam armed is synthetic evidence regardless of which contributors fired. + +**Provenance at both producers.** Journal entries have two producers: the engine host loop (transition-outcome entry, `engine.ts:807`) and the `ratchet batch report` verb invoked *by the spawned agent*. The engine stamps from its in-hand `agentOverride` flag; `report.ts` stamps from its own process env via the same `activeAgentCmdOverride` helper — the spawned stand-in inherits `RATCHET_BATCH_AGENT_CMD` (the engine builds `env` from `process.env`, engine.ts:332, and #89's threading now delivers it), so a stub's `--complete` entry is stamped even though the engine never sees that append. `via` is optional and only ever `'env-override'` today (a string-literal union left open to widen later); readers ignore it, so `readJournal`/status/fold paths need no change. `ProofOfWorkRecord` is not stamped: the boundary proof is host-run (never through the agent seam), and stamping it is #82/phase-2 territory. + +**Ecosystem/agent neutrality.** Nothing ships into consuming repos (`generalizable-defaults`: the notice text and provenance constant are CLI/runtime output, not generated artifacts) and no skill/template changes (`instruction-fed-config` untouched). The helper treats every adapter uniformly and names no specific agent (`multi-agent-support`): there are no per-agent outputs to enumerate. + +**Testing (per the `testing` standard — right layer, pyramid-weighted).** Unit tests (no fs, no spawn) cover the helper: active/whitespace-only/unset var, `bash -c` request shape carrying instructions/cwd/env, fallback closure invoked exactly once when inactive, notice text per var name. Integration tests cover each producer/surface: `test/batch-engine/engine-agent-override.test.ts` extends to assert `StepResult.agentOverride` and the stamped outcome entry; `test/commands/batch/report.test.ts` asserts entries appended under an active var carry `via: 'env-override'` (and stay unstamped without it); `test/commands/batch/apply.test.ts` asserts the text notice line and the `--json` field; `test/commands/eval/run.test.ts` asserts the persisted run record stamp, the JSON `agentOverride`, and the text notice; existing judge/mutation-harness tests keep passing with the delegated override branch. The rex runtime suites gain one assertion each: an override-shaped request threads a per-step env var into the launch command (sidecar run-op / remote `/execute` body), tying this change into the phase proof-of-work run. Test headers name the `.feature` files they implement. Full suite and the coverage gate stay green at or above the enforced `COVERAGE_THRESHOLD`. + +**Documentation (per the `documentation` standard).** `docs/engine/agent-runtime.md` (existing Reference doc for this core flow, already touched by the sibling change) documents the override seam contract — the notice line, the `agentOverride` JSON field, and `via: env-override` journal provenance — and `docs/eval-mutation-harness.md`'s existing `RATCHET_EVAL_AGENT_CMD` entries (lines 126, 230) gain the same contract for eval runs and run records. Existing diagrams in both docs are re-verified against the changed flow (the override seam adds fields, not new components — no new diagram, per the standard's "use diagrams deliberately"). `README.md` is checked for any description of the override/journal surfaces and updated to match (expected no-op). + +## Tasks + +**1. Shared override-aware spawn-request helper** + +- [x] 1.1 Add `activeAgentCmdOverride`, `buildAgentSpawnRequest`, `agentOverrideNotice`, and `ENV_OVERRIDE_PROVENANCE` to `src/core/batch/engine/agent.ts`; export them via `src/core/batch/engine/index.ts` +- [x] 1.2 Unit tests (no fs, no spawn) in `test/batch-engine/agent.test.ts`: active/whitespace-only/unset override, `bash -c` request shape (instructions/cwd/env carried), fallback closure used exactly once when inactive, notice text per env-var name; header names `features/agent-cmd-override/shared-spawn-helper.feature` + +**2. Batch engine + apply surface** + +- [x] 2.1 `engine.ts` `buildSpawnRequest` delegates its override branch to `buildAgentSpawnRequest` and returns the `agentOverride` flag; the step path threads it onto `EngineStepOutcome`/`StepResult` (`agentOverride?: true`) and stamps `via: 'env-override'` on the transition-outcome journal entry (`engine.ts:807`) when set +- [x] 2.2 Add `via?: 'env-override'` to `JournalEntry` (`src/core/batch/journal.ts`); `src/commands/batch/report.ts` stamps every entry it appends when `activeAgentCmdOverride('RATCHET_BATCH_AGENT_CMD', process.env)` is active +- [x] 2.3 `renderResult` (`src/commands/batch/apply.ts`) prints the one-line `⚠ agent overridden by RATCHET_BATCH_AGENT_CMD` notice in text mode when `result.agentOverride` is set (`--json` carries the field via the existing stringify) +- [x] 2.4 Integration tests: `engine-agent-override.test.ts` asserts `StepResult.agentOverride` + stamped outcome entry; `test/commands/batch/report.test.ts` asserts stamped/unstamped entries by env; `test/commands/batch/apply.test.ts` asserts the text notice and `--json` field; headers name `features/agent-cmd-override/override-notice.feature` / `override-provenance.feature` + +**3. Eval surface** + +- [x] 3.1 `judge.ts` `buildVoteRequest` and `mutation-harness.ts` `buildSeedRequest` delegate their override branch to `buildAgentSpawnRequest` (adapter resolution stays in each `buildAdapterRequest` closure) +- [x] 3.2 `executeRun` stamps `via: 'env-override'` on the persisted `EvalRun` when `RATCHET_EVAL_AGENT_CMD` is active; `src/commands/eval/run.ts` adds top-level `agentOverride: true` to the `--json` payload and a notice line atop the text scorecard +- [x] 3.3 Integration tests in `test/commands/eval/run.test.ts`: persisted run record stamped under an active var (unstamped without), `--json` `agentOverride`, text notice; judge/mutation-harness suites stay green on the delegated branch; headers name the implemented `.feature` files + +**4. Rex runtime composition (phase proof-of-work)** + +- [x] 4.1 Add one assertion to each of `test/batch-engine/rex-sidecar-runtime.test.ts` and `test/batch-engine/rex-remote-runtime.test.ts`: an override-shaped request (`bash -c ` with a per-step env var) has that var exported in the built launch command — the override path composes with #89's env threading; headers reference `features/agent-cmd-override/shared-spawn-helper.feature` + +**5. Documentation (mandatory — `documentation` standard, Reference docs)** + +- [x] 5.1 Update `docs/engine/agent-runtime.md`: document the override seam contract — the one-line notice, `agentOverride: true` in `--json`, `via: env-override` journal provenance, and the single shared helper; re-verify the doc's existing diagram/tables still depict the code accurately +- [x] 5.2 Update `docs/eval-mutation-harness.md`: the `RATCHET_EVAL_AGENT_CMD` entries state the notice and the `via: env-override` run-record stamp +- [x] 5.3 Check `README.md` for any description of the override/journal/run-record surfaces this change alters and update it to match (expected no-op) + +**6. Verification** + +- [x] 6.1 Run `npm test -- test/batch-engine/rex-sidecar-runtime.test.ts test/batch-engine/rex-remote-runtime.test.ts` — exit code 0 with the override-composition assertions in place (phase proof-of-work) +- [x] 6.2 Run the full test suite and the coverage gate — green, coverage at or above the enforced `COVERAGE_THRESHOLD` (`testing` standard) diff --git a/.ratchet/changes/thread-env-through-rex-runtimes/.ratchet.yaml b/.ratchet/changes/thread-env-through-rex-runtimes/.ratchet.yaml new file mode 100644 index 0000000..ed6b712 --- /dev/null +++ b/.ratchet/changes/thread-env-through-rex-runtimes/.ratchet.yaml @@ -0,0 +1,3 @@ +schema: ratchet +created: 2026-07-09 +standards: [documentation, testing] diff --git a/.ratchet/changes/thread-env-through-rex-runtimes/features/rex-env-threading/env-reaches-spawned-agent.feature b/.ratchet/changes/thread-env-through-rex-runtimes/features/rex-env-threading/env-reaches-spawned-agent.feature new file mode 100644 index 0000000..ec5e2a4 --- /dev/null +++ b/.ratchet/changes/thread-env-through-rex-runtimes/features/rex-env-threading/env-reaches-spawned-agent.feature @@ -0,0 +1,25 @@ +Feature: Per-step env reaches the agent spawned by the rex runtimes + As a batch engine operator + I want the env the engine places on AgentSpawnRequest.env to be applied to the + agent command launched by both rex runtimes + So that per-step environment variables (and future env-based hardening) are + actually in effect inside the spawned agent, honoring the documented contract + + Scenario: Sidecar runtime applies request env to the launched agent command + Given the engine builds an AgentSpawnRequest with env entry "RATCHET_STEP_VAR=from-engine" + When the rex sidecar runtime constructs the run-op command for the agent + Then the run-op command exports "RATCHET_STEP_VAR" with value "from-engine" before invoking the agent + And executing that command in a shell makes "RATCHET_STEP_VAR=from-engine" observable to the agent process + + Scenario: Remote runtime applies request env to the launched agent command + Given the engine builds an AgentSpawnRequest with env entry "RATCHET_STEP_VAR=from-engine" + When the rex remote runtime constructs the launch command it executes on the server + Then the launch command exports "RATCHET_STEP_VAR" with value "from-engine" before invoking the agent + And executing that command in a shell makes "RATCHET_STEP_VAR=from-engine" observable to the agent process + + Scenario: Request env overlays the runtime session's base environment + Given a runtime session whose base environment already defines "SHARED_VAR=from-session" + And an AgentSpawnRequest whose env defines "SHARED_VAR=from-request" + When the runtime launches the agent command + Then the agent observes "SHARED_VAR=from-request" + And base-environment variables absent from the request env remain visible to the agent diff --git a/.ratchet/changes/thread-env-through-rex-runtimes/features/rex-env-threading/env-serialization-safety.feature b/.ratchet/changes/thread-env-through-rex-runtimes/features/rex-env-threading/env-serialization-safety.feature new file mode 100644 index 0000000..244cfa7 --- /dev/null +++ b/.ratchet/changes/thread-env-through-rex-runtimes/features/rex-env-threading/env-serialization-safety.feature @@ -0,0 +1,22 @@ +Feature: Request env is serialized into the launch command safely + As a batch engine maintainer + I want env serialization to be shell-safe and shared by both rex runtimes + So that arbitrary env values cannot break or inject into the launch command, + and the two runtimes cannot drift apart in how they apply env + + Scenario: Env values containing shell metacharacters are quoted safely + Given an AgentSpawnRequest env entry whose value contains single quotes, spaces, and "$" characters + When the env is serialized into the launch command + Then the launched agent observes the value byte-for-byte unchanged + And the metacharacters are not interpreted by the shell + + Scenario: Env entries with names that are not valid shell identifiers are skipped + Given an AgentSpawnRequest env containing an entry whose name is not a valid shell identifier + When the env is serialized into the launch command + Then that entry is omitted from the serialized exports + And all valid-identifier entries are still exported + + Scenario: Both runtimes serialize env through one shared helper + Given the rex sidecar runtime and the rex remote runtime + When each constructs its agent launch command from an AgentSpawnRequest + Then both delegate env serialization to the same shared helper function diff --git a/.ratchet/changes/thread-env-through-rex-runtimes/plan.md b/.ratchet/changes/thread-env-through-rex-runtimes/plan.md new file mode 100644 index 0000000..32d66a3 --- /dev/null +++ b/.ratchet/changes/thread-env-through-rex-runtimes/plan.md @@ -0,0 +1,52 @@ +# thread-env-through-rex-runtimes + +## Why + +The engine builds `AgentSpawnRequest.env` for every step (`src/core/batch/engine/engine.ts:332/470/643` → `buildSpawnRequest` → `agent.ts:67`), and `docs/engine/agent-runtime.md` presents `env` as part of the spawn contract — but both rex runtimes silently drop it: the sidecar runtime reads `req.env` only for `RATCHET_BATCH_NAME` dir naming (`rex-sidecar-runtime.ts:413`) and the remote runtime never references it at all. Only the legacy in-process `realSpawner` (`agent.ts:288`) honors it. This is a live doc/code contract violation (issue #89) and blocks every env-based hardening step that follows (e.g. the #86 allowlist). + +## What Changes + +- Both rex runtimes apply `AgentSpawnRequest.env` to the agent command they launch: the env is serialized as shell `export` statements prefixed to the launch command, on the sidecar path (`buildRunCommand`, `rex-sidecar-runtime.ts:144-152`) and the remote path (`buildRemoteRunCommand`, `rex-remote-runtime.ts:124-127`). +- Env serialization lives in ONE shared helper used by both runtimes (also consolidating the two identical `shquote` copies at `rex-sidecar-runtime.ts:126` and `rex-remote-runtime.ts:115` into that shared module). Implements `features/rex-env-threading/env-serialization-safety.feature`. +- Merge semantics are decided and documented: **request env overlays the runtime session's base environment** (exported on top of it; request value wins on collision, base vars absent from the request remain visible). Implements `features/rex-env-threading/env-reaches-spawned-agent.feature`. +- Tests assert a per-step env var set by the engine is visible to the spawned command on BOTH runtimes (`test/batch-engine/rex-sidecar-runtime.test.ts`, `test/batch-engine/rex-remote-runtime.test.ts`). +- `docs/engine/agent-runtime.md` is corrected: the env-threading contract (overlay semantics) is documented, the run-op command description reflects the env exports, and the `insecure` (settings key) vs `allowInsecure` (runtime option, `rex-remote-runtime.ts:89`, mapped at `engine.ts:218`) naming drift is clarified. +- No protocol change: `sidecar.py` and the Node→sidecar run-op shape (`{op, id, command}`) are untouched; env rides inside the command string. The swe-rex REST protocol is likewise untouched. + +## Design + +**Serialize env into the command string; no sidecar/remote protocol change.** Issue #89 offers two routes for the sidecar (an additive `env` field on the run op consumed by `sidecar.py`, or exports embedded in the launcher command). We embed exports in the command because it is the thinnest end-to-end slice: one serialization helper works identically for both runtimes, requires no Python change, no cross-language protocol contract, and is directly assertable by the existing test harnesses (the sidecar `FakeChild` captures the run-op `command`; the remote `fakeServer` captures the `/execute` body command). + +**One shared helper.** A new shared runtime module (e.g. `src/core/batch/engine/runtime/spawn-command.ts`) exports `shquote` and `buildEnvExports(env): string`. `buildEnvExports` emits `export NAME='value'; ` per entry with values shell-quoted via `shquote`, and **skips entries whose name is not a valid shell identifier** (`/^[A-Za-z_][A-Za-z0-9_]*$/` — such names are unreachable in shell anyway and would break `export`). Entries with `undefined` values are skipped. Both runtimes import from this module; the two local `shquote` copies are deleted. This is the seed of the phase-level "spawn-request construction lives in one shared helper" criterion (change `gate-and-mark-agent-cmd-override` extends it) and pre-dedups part of #91. + +**Command shape.** Sidecar: `cd ''; cat '' | '' ''` (exports inserted after the cwd prefix, before the pipeline). Remote: same insertion in `buildRemoteRunCommand`'s output, which is then wrapped by the existing nohup/log/exit-sentinel launcher unchanged. Quoting nests safely: the run-op command is JSON-encoded on the wire and `sidecar.py` re-wraps it via its own `_shquote`; single-quote escaping preserves newlines and metacharacters byte-for-byte. + +**Overlay, not replace.** The legacy `realSpawner` replaces the child env wholesale, but a shell-session runtime cannot sanely replace (`env -i` would strip the session `PATH` the agent needs for command resolution on docker/remote loci). Exports on top of the session base env give deterministic, documentable semantics: request wins on collision, base survives otherwise. Note `req.env` today is `{...process.env, RATCHET_BATCH_NAME}`; narrowing WHAT the engine puts in it (host-env leakage to docker/remote) is explicitly out of scope here — that is issue #86 (allowlist), which this change unblocks. + +**Testing (per the `testing` standard — right layer, pyramid-weighted).** Unit tests with no fs/spawn cover `buildEnvExports` (metacharacter quoting, identifier filtering, undefined skipping) and the string-level output of both command builders. One small execution test per builder runs the built command through the system shell (`sh -c`) with a controlled base env and asserts the spawned command observes the request value (collision → request wins; base var absent from request → still visible) — this proves actual visibility, not just string shape, satisfying the definition of done on both runtimes. Runtime-level tests assert via the existing fakes that the run-op / `/execute` command carries the exports. Test file headers name the `.feature` files they implement. Full suite and the coverage gate stay green. + +**Documentation (per the `documentation` standard).** `docs/engine/agent-runtime.md` is an existing Reference doc for a core flow; it is updated in this same change — env contract, run-op command description, `insecure`/`allowInsecure` clarification — and any existing diagram/table touched by the flow is re-verified for accuracy. `README.md` is checked for surfaces this change alters and updated if it describes agent env behavior. + +## Tasks + +**1. Shared env serialization helper** + +- [x] 1.1 Create the shared runtime module (e.g. `src/core/batch/engine/runtime/spawn-command.ts`) exporting `shquote` and `buildEnvExports(env)`: shell-quoted `export` statements, invalid-identifier names skipped, `undefined` values skipped; both rex runtimes import `shquote` from it (local copies removed) +- [x] 1.2 Add unit tests (no fs, no spawn) for `buildEnvExports`: metacharacter/quote/`$`/space/newline values survive quoting, invalid-identifier entries are omitted while valid ones remain, empty env yields empty prefix; header names `features/rex-env-threading/env-serialization-safety.feature` + +**2. Thread env through the sidecar runtime** + +- [x] 2.1 `buildRunCommand` (`rex-sidecar-runtime.ts`) prefixes `buildEnvExports(request.env)` to the agent pipeline (after the cwd prefix); run-op construction passes the request through unchanged +- [x] 2.2 Extend `test/batch-engine/rex-sidecar-runtime.test.ts`: run-op command carries the export of a per-step var set on the request env, and an execution test runs the built command via `sh -c` proving the spawned command observes the request value with overlay semantics (request wins over a colliding base var; base vars absent from the request stay visible); header names `features/rex-env-threading/env-reaches-spawned-agent.feature` + +**3. Thread env through the remote runtime** + +- [x] 3.1 `buildRemoteRunCommand` (`rex-remote-runtime.ts`) prefixes `buildEnvExports(request.env)` the same way; the nohup launcher wrapping stays unchanged +- [x] 3.2 Extend `test/batch-engine/rex-remote-runtime.test.ts`: the `/execute` nohup body command carries the export of a per-step var, plus the same `sh -c` execution/overlay assertion for the remote builder; header names `features/rex-env-threading/env-reaches-spawned-agent.feature` + +**4. Documentation (mandatory — `documentation` standard, Reference docs)** + +- [x] 4.1 Update `docs/engine/agent-runtime.md`: document that both rex runtimes export `AgentSpawnRequest.env` before launching the agent with overlay merge semantics (request overlays session base; legacy in-process spawner replaces instead), update the run-op command description to show the env exports, and fix the `insecure`/`allowInsecure` drift by stating the settings key `insecure` maps to the runtime option `allowInsecure`; re-verify the env table and any Mermaid diagram in the doc still depict the code accurately and update them if the flow change made them stale +- [x] 4.2 Check `README.md` for any description of agent env/runtime behavior affected by this change and update it to match (no-op if it describes none) +- [x] 5.1 Run `npm test -- test/batch-engine/rex-sidecar-runtime.test.ts test/batch-engine/rex-remote-runtime.test.ts` — exit code 0 with the new env-threading assertions in place (phase proof-of-work) +- [x] 5.2 Run the full test suite and the coverage gate — green, coverage at or above the enforced `COVERAGE_THRESHOLD` (`testing` standard) diff --git a/docs/engine/agent-runtime.md b/docs/engine/agent-runtime.md index 3c13a26..0a71aca 100644 --- a/docs/engine/agent-runtime.md +++ b/docs/engine/agent-runtime.md @@ -63,6 +63,199 @@ newline-joined, exitCode from the exit event). A bootstrap failure or sidecar no new outcome states — so the engine maps it to blocked/failed and the step remains resumable. +## Per-step environment (`AgentSpawnRequest.env`) + +The engine builds a per-step `AgentSpawnRequest.env` for every transition (see +`buildSpawnRequest` → `agent.ts`). Both rex runtimes **export that env before +launching the agent** so it is actually in effect inside the spawned command — +honoring the spawn contract this runtime layer presents. Env serialization +lives in one shared helper (`runtime/spawn-command.ts`: `shquote` + +`buildEnvExports`), consumed by both runtimes, so the two cannot drift apart in +how they apply env. + +**Merge semantics are overlay, not replace.** The runtimes prefix the agent +launch command with `export NAME='value'; ` statements (single-quoted via +`shquote`), which run on top of the runtime session's base environment: + +- A request value **wins on collision** with a session base variable. +- A base variable **absent from the request env remains visible** to the agent. + +A shell-session runtime cannot sanely replace the whole environment (`env -i` +would strip the session `PATH` the agent needs for command resolution on +docker/remote loci), so exports-on-top is the deterministic, documentable +contract. The legacy in-process `realSpawner` (`agent.ts`) replaces the child +env wholesale instead — that path spawns the agent binary directly as a Node +child, not through a shell session, so it controls env by passing it to the +spawn call rather than exporting it. + +**Serialization safety.** `buildEnvExports` single-quotes every value so +metacharacters (`$`, spaces, quotes, newlines, backticks) survive byte-for-byte +and are not interpreted by the shell. Entries whose name is not a valid shell +identifier (`/^[A-Za-z_][A-Za-z0-9_]*$/` — unreachable in shell and would break +`export`) and entries with `undefined` values are skipped; an empty/absent env +yields an empty prefix. + +**No protocol change.** Env rides inside the command string: + +- **Sidecar** — the run-op `command` becomes `cd ; cat | `. + The run-op shape (`{op, id, command}`) and `sidecar.py` are untouched; the + command is JSON-encoded on the wire and the sidecar re-wraps it via its own + `_shquote`, which preserves single-quote escaping byte-for-byte. +- **Remote** — `buildRemoteRunCommand` emits ` cat | `, + and the existing nohup/log/exit-sentinel launcher wraps that unchanged; the + exports sit inside its `( … )` subshell, so they apply to the agent pipeline + without touching the REST protocol. + +> **What the engine places in `AgentSpawnRequest.env` is scoped, not the raw +> host env.** Every spawn site builds the request env from +> `scopeAgentEnv(process.env)` (see [Agent environment allowlist](#agent-environment-allowlist)) +> overlaying `RATCHET_BATCH_NAME`, so non-allowlisted host secrets do not reach +> the agent session. The sidecar bootstrap independently scopes its own launch +> env the same way. + +## Agent environment allowlist + +Defined in `src/core/batch/engine/agent-env.ts` (`scopeAgentEnv`, +`buildAgentEnvAllowlist`, `adapterEnvPassthroughKeys`, `AGENT_ENV_ALLOW_VAR`). + +Every spawn site — the engine's change-transition, decompose, and PR spawns +(`engine.ts`) and the ReX sidecar bootstrap (`runtime/rex-bootstrap.ts`) — +builds the environment it hands to the agent from `scopeAgentEnv(hostEnv)` rather +than from the raw host environment. A non-allowlisted host variable is dropped +before it can reach an agent session, so a secret the operator holds in their +shell (`AWS_SECRET_ACCESS_KEY`, a personal `GITHUB_TOKEN`, etc.) cannot leak into +a spawned agent regardless of locus. + +```mermaid +flowchart TD + A["⚙️ host process.env"] --> B["🔐 scopeAgentEnv(hostEnv)"] + B --> C{"name in allowlist?"} + C -- "yes (exact or prefix match)" --> D["✅ kept"] + C -- "no" --> E["❌ dropped"] + D --> F["📨 AgentSpawnRequest.env"] + B --> G["➕ RATCHET_BATCH_NAME overlay"] + G --> F + + classDef input fill:#E6E6FA,stroke:#333,stroke-width:2px,color:darkblue + classDef proc fill:#90EE90,stroke:#333,stroke-width:2px,color:darkgreen + classDef keep fill:#90EE90,stroke:#333,stroke-width:2px,color:darkgreen + classDef drop fill:#FFB6C1,stroke:#DC143C,stroke-width:2px,color:black + classDef out fill:#FFEFD5,stroke:#333,stroke-width:2px,color:darkslategray + class A input + class B,G proc + class C proc + class D keep + class E drop + class F out +``` + +### Allowlist composition + +`buildAgentEnvAllowlist(hostEnv)` returns `{ exact: Set, prefixes: string[] }`. +A host variable passes through when its name is in `exact` OR starts with one of +the `prefixes` (a prefix entry ending in `_` matches any var beginning with it). + +| Source | Exact names | Prefix patterns | +| --- | --- | --- | +| Baseline process vars | `PATH`, `HOME`, `TMPDIR`, `LANG`, `TERM`, `SYSTEMROOT`, `COMSPEC`, `PATHEXT`, `USERPROFILE`, `TEMP`, `TMP`, `APPDATA`, `LOCALAPPDATA`, `PROGRAMDATA` | `LC_` | +| Proxy | `HTTP_PROXY`, `HTTPS_PROXY`, `NO_PROXY`, `http_proxy`, `https_proxy`, `no_proxy` | — | +| Forge auth | `GH_TOKEN`, `GITHUB_TOKEN` | — | +| Ratchet control | — | `RATCHET_` | +| Adapter passthrough | the union of every registered adapter's `envPassthrough` (see below) | adapter `PREFIX_*` entries | + +`RATCHET_*` control variables pass through as a prefix so engine/runtime knobs +(e.g. `RATCHET_BATCH_AGENT_CMD`, `RATCHET_BATCH_NAME`) ride through. The ReX +sidecar's own `REX_*` threading vars (`REX_LOCUS`, `REX_WORKDIR`, `REX_IMAGE`, +…) are NOT in the baseline allowlist; the bootstrap sets them explicitly AFTER +scoping, so they survive by construction rather than by allowlist entry. + +### Adapter `envPassthrough` + +Every `AgentAdapter` declares `readonly envPassthrough: readonly string[]` — the +provider/auth variables that adapter's agent needs. `scopeAgentEnv` takes the +union across all registered adapters (`adapterEnvPassthroughKeys`), so an +agent's own credentials reach it without any agent being special-cased in a +shared path. An entry may be an exact name (`ANTHROPIC_API_KEY`) or a `PREFIX_*` +glob (`CLAUDE_CODE_*` matches any var starting with `CLAUDE_CODE_`). + +The builtin adapter declarations: + +| Adapter | `envPassthrough` | +| --- | --- | +| claude | `ANTHROPIC_API_KEY`, `CLAUDE_CODE_*` | +| codex | `OPENAI_API_KEY`, `CODEX_HOME` | +| gemini | `GEMINI_API_KEY`, `GOOGLE_API_KEY` | +| cursor | `CURSOR_API_KEY`, `CURSOR_*` | +| opencode | `OPENCODE_*` | + +A drift guard in `test/core/batch/agent-init-link.test.ts` asserts every +spawnable adapter declares a non-empty `envPassthrough`, so a newly added agent +cannot silently widen or narrow the allowlist. + +### Operator escape hatch + +`RATCHET_AGENT_ENV_ALLOW` (exported as `AGENT_ENV_ALLOW_VAR`) is a +comma-separated list of extra host env names read from the host environment at +spawn time. Each named var is added to the `exact` allowlist for that spawn +only. It is intended for operator-supplied site credentials that are not a +known provider key; it is read from the host env and never committed to a +repo's batch manifest. + +### Where scoping is applied + +| Site | File | Overlay | +| --- | --- | --- | +| Change-transition spawn | `engine.ts` (`runStep`) | `RATCHET_BATCH_NAME` (when a batch is active) | +| Decompose spawn | `engine.ts` (`runDecompositionStep`) | `RATCHET_BATCH_NAME` | +| PR spawn | `engine.ts` (`runPrStep`) | `RATCHET_BATCH_NAME` | +| ReX sidecar bootstrap | `runtime/rex-bootstrap.ts` (`bootstrapRexRuntime`) | `PATH` (venv bin prepended), `VIRTUAL_ENV`, `REX_*` threading vars | + +The per-step request env is then serialized into the agent command by +`buildEnvExports` (see [Per-step environment](#per-step-environment-agentspawnrequestenv)) +as exports-on-top of the session base; a var dropped by scoping is absent from +the request env and so is never exported. + +## Agent-cmd override seam + +`RATCHET_BATCH_AGENT_CMD` (and `RATCHET_EVAL_AGENT_CMD` on the eval side) stand +in for the configured coding-agent binary when set: the engine launches +`bash -c ` (instructions on stdin) instead of resolving the adapter. +This is the deterministic spawn path the e2e/eval harnesses exercise the +orchestration through without a real agent. An active override is **loud and +auditable** — a leftover value (from an eval session, CI, a `.envrc`) can never +silently replace the configured agent: + +- **One-line notice (text output).** `ratchet batch apply` prints + `⚠ agent overridden by RATCHET_BATCH_AGENT_CMD` as the first line of the + rendered step result; `ratchet eval run` prints the matching + `⚠ agent overridden by RATCHET_EVAL_AGENT_CMD` atop the scorecard. The notice + rides the result, not a side-channel print — it appears exactly when a spawn + ran under the override. Pre-spawn parks print no notice (nothing was + spawned). +- **`agentOverride: true` (`--json`).** `batch apply --json` carries + `agentOverride: true` on the step-result JSON; `eval run --json` carries it + at the top level. Absent when no override is active — byte-identical output + for override-free runs. +- **`via: env-override` provenance.** Every journal entry and eval run record + produced under an active override is stamped `via: "env-override"`: + - the engine stamps the transition-outcome journal entry it appends; + - `batch report` stamps every entry it appends from its own process env (the + spawned stand-in inherits `RATCHET_BATCH_AGENT_CMD`, so a stub-reported + completion/blocker/progress/needs-input is auditable even though the engine + never sees that append); + - `eval run` stamps `via` on the persisted run record. + The field is optional and only ever `'env-override'` today; readers ignore + it, so override-free entries/runs carry no `via` and need no migration. + +The override gate and spawn-request construction live in **one shared helper** +(`buildAgentSpawnRequest` in `src/core/batch/engine/agent.ts`): the batch +engine's `buildSpawnRequest`, the eval judge's `buildVoteRequest`, and the +mutation harness's `buildSeedRequest` all delegate their override branch to it, +so the three spawn seams cannot drift apart. The helper owns only the shared +override semantics (the trim check, the `bash -c ` request shape, the +`agentOverride` flag); each call site's `buildAdapterRequest` closure owns its +own adapter resolution, so genuinely different adapter paths are not flattened. + ## SWE-ReX sidecar For the `local` and `docker` loci, ratchet bootstraps an isolated Python sidecar @@ -118,6 +311,11 @@ Environment variables threaded to the sidecar: | `REX_IMAGE` | docker only | configured image, or `DEFAULT_DOCKER_IMAGE` (`python:3.12`) | | `REX_MOUNT_HOST` | docker only | project root (host path bind-mounted into the container) | | `REX_MOUNT_CONTAINER` | docker only | `/workspace` (in-container mount point) | +| `REX_DOCKER_USER` | docker only, when `dockerUser` is set | host `uid:gid` for `docker run --user`; when unset the sidecar resolves the current host uid:gid | +| `REX_DOCKER_MEMORY` | docker only | configured `dockerMemory`, or `DEFAULT_DOCKER_MEMORY` (`2g`) | +| `REX_DOCKER_PIDS_LIMIT` | docker only | configured `dockerPidsLimit`, or `DEFAULT_DOCKER_PIDS_LIMIT` (`512`) | +| `REX_DOCKER_CPUS` | docker only, when `dockerCpus` is set | configured `dockerCpus` (e.g. `1.5`); no flag when unset | +| `REX_DOCKER_NETWORK` | docker only | configured `network`, or `DEFAULT_DOCKER_NETWORK` (`bridge`) | ### Sidecar process (`sidecar.py`) @@ -158,6 +356,29 @@ before emitting the `exit` event. The `locus` setting selects the runtime implementation. The default locus is `local`. +### Threat model per locus + +The resolved `locus` determines what **real isolation** backs a run. `ratchet +batch config` and `ratchet batch view` render this honestly (see +[isolation rendering](../commands/batch.md#isolation-and-enforcement-rendering)), +and `ratchet doctor` nudges toward the docker locus when a permissive posture +runs on local (see [doctor](../commands/doctor.md#batch-isolation-check)). + +| Locus | Real isolation | Threat model | +|---|---|---| +| `local` | **Advisory — none.** The agent runs as a direct child of the operator's shell session. No filesystem or network isolation; the argv denylist (`REPO_SANDBOX_DENY_PATTERNS`) is best-effort damage reduction, not containment. Env is scoped to the [allowlist](#agent-environment-allowlist) so non-allowlisted host secrets are dropped, but that is scoping, not isolation. | A full-autonomy (or even permissive) agent on `local` can do anything the launching user can — read any file the user can read, reach any network the user can reach, write anywhere the user can write. The posture flags are the only gate, and for cursor/opencode even those are a no-op (agent defaults apply). **This is the right locus for trusted, operator-supervised runs.** | +| `docker` | **Partial container isolation.** The agent runs in a Docker container with a read-write bind mount of the project root at `/workspace`. Resource bounds (memory, pids, optional cpus) and a `--user` mapping keep runaway and file-ownership in check. See the [honest isolation contract](#honest-isolation-contract) below for what is and is NOT bounded. | The container shares the host kernel (no VM boundary) and has outbound network by default (`network: bridge`). The repo bind mount is read-write by design so the agent can edit files and the engine can read the journal back. The docker locus bounds resource runaway and ensures file ownership — it is **not** a hardened sandbox for untrusted agents, but it is real containment where `local` has none. | +| `remote` | **Server's boundary.** The agent runs on an external `swerex-remote` server over REST; ratchet's process never touches the agent's filesystem or process tree. | The isolation boundary is whatever the server operator configures — ratchet does not own it. The transport is authenticated (`X-API-Key`) and scheme-selected (loopback → http, non-local → https unless `insecure`). This is the locus for shared/CI runners and hardened remote sandboxes. | + +> **The argv denylist is not containment.** `REPO_SANDBOX_DENY_PATTERNS` (the +> `deny` list merged for `repo-sandboxed-permissive` and `curated-allowlist`) +> blocks a handful of destructively-shaped shell commands at the agent's own +> permission layer. It is best-effort damage reduction — an agent that can run +> arbitrary shell can trivially evade a pattern denylist. **Real containment is +> the docker locus** (resource bounds, filesystem ownership, optional network +> isolation via `network: none`). The denylist exists to catch accidental +> foot-guns on the `local` locus, not to stop a determined agent. + ### `local` — `RexSidecarRuntime` (`rex-sidecar-runtime.ts`) The local runtime bootstraps the SWE-ReX sidecar, spawns it as a child process, @@ -165,14 +386,17 @@ and drives the JSON-lines protocol described above. Prompt delivery: the agent's instructions are written to a temporary prompt file at `.ratchet/batches//.run//prompt.txt` on the host. The run command -sent to the sidecar is `cd ; cat | `. The prompt -file is removed after the run (in a `finally` block). +sent to the sidecar is `cd ; export NAME='value'; …; cat | ` +— the per-step `AgentSpawnRequest.env` is exported before the pipeline (after the +`cd` prefix) via the shared `buildEnvExports` helper. The prompt file is removed +after the run (in a `finally` block). The overall run timeout defaults to `600000` ms (10 minutes) and is configurable via `batch.agentTimeoutMs` or the `RATCHET_AGENT_TIMEOUT_MS` environment variable (see [Per-agent timeout](#per-agent-timeout) below). On completion or timeout the -sidecar receives `SIGTERM` followed (after a 2 s grace) by `SIGKILL` if it has not -exited. +sidecar is torn down via the [process-group reaping sequence](#agent-process-group-reaping-on-teardown) +— a shutdown op first, then a group `SIGKILL` after a short grace — so the agent +process is never left alive. ### `docker` — `RexSidecarRuntime` with `DockerDeployment` @@ -194,6 +418,48 @@ Additional behavior specific to the docker locus: translated to the in-container equivalent before it is passed to the sidecar. 4. `REX_IMAGE` is set to the configured `image`, or `DEFAULT_DOCKER_IMAGE` (`python:3.12`) when none is configured. +5. **Docker hardening knobs** are threaded as `REX_DOCKER_*` env vars and + spliced into `docker run` argv by the sidecar: + + | Knob | Flag | Default when unset | Source of truth | + |---|---|---|---| + | `dockerUser` | `--user` | current host `uid:gid` (resolved by the sidecar) | `REX_DOCKER_USER` (Node); sidecar fallback resolves `os.getuid():os.getgid()` | + | `dockerMemory` | `--memory` | `2g` (`DEFAULT_DOCKER_MEMORY`) | `config.ts` / `sidecar.py` mirror | + | `dockerPidsLimit` | `--pids-limit` | `512` (`DEFAULT_DOCKER_PIDS_LIMIT`) | `config.ts` / `sidecar.py` mirror | + | `dockerCpus` | `--cpus` | *(no flag — opt-in only)* | `REX_DOCKER_CPUS` only when set | + | `network` | `--network` | `bridge` (`DEFAULT_DOCKER_NETWORK`) | `config.ts` / `sidecar.py` mirror | + + The `--user` default ensures container file writes land as the **host user**, + not root, so journal writes on the bind mount are owned by the operator. Each + knob is overridable via the nearest-wins settings cascade (project config ← + per-change manifest), exactly like `image`/`locus`. + +#### Honest isolation contract + +The docker locus is a **partial isolation** boundary, not a security sandbox. +Operators should understand what it does and does NOT contain: + +**What is bounded:** +- **File writes** — land on the host bind mount (the project root, mounted + read-write at `/workspace`) as the host `uid:gid` (via `--user`), not as root. +- **Memory** — capped at `dockerMemory` (default `2g`) via `--memory`. +- **Process count** — capped at `dockerPidsLimit` (default `512`) via + `--pids-limit`, bounding fork-bomb-style runaway. +- **CPU** — optionally capped via `dockerCpus` (`--cpus`); no cap by default. + +**What is NOT bounded (by default):** +- **Network** — the default `network: bridge` means the container **HAS outbound + network access**. To fully isolate, set `network: none`. A custom network name + is also accepted (passed verbatim to `docker run --network`). +- **Root filesystem** — the container's own rootfs is Docker's default (not + read-only); only the bind mount is shared with the host. +- **Kernel surface** — the container shares the host kernel (no VM boundary). + +The repo bind mount stays **read-write by design** so the agent can edit files +and the engine can read the journal back over the mount. This is the documented +contract: the docker locus bounds resource runaway and ensures file ownership, +but it is **not** a substitute for a hardened sandbox when running untrusted +agents. ### `remote` — `RexRemoteRuntime` (`rex-remote-runtime.ts`) @@ -209,8 +475,12 @@ Transport scheme selection: - A bare loopback host (`localhost`, `127.x.x.x`, `::1`) defaults to `http`. - A bare non-local host defaults to `https`. - An explicit `https://` prefix is honored. -- An explicit `http://` prefix to a non-local host is refused unless `insecure: - true` is set in the settings. +- An explicit `http://` prefix to a non-local host is refused unless + `allowInsecure` is set. The settings key is `insecure` (a boolean under the + resolved `batch:` / remote scope); the engine maps `insecure: true` to the + runtime option `allowInsecure` (`rex-remote-runtime.ts`, threaded at + `engine.ts`). The two names refer to the same opt-in — the settings schema + names it `insecure`, the runtime surface names it `allowInsecure`. The remote runtime reproduces the sidecar's tail-poll streaming over REST: @@ -222,6 +492,11 @@ The remote runtime reproduces the sidecar's tail-poll streaming over REST: advance a byte cursor; emit `stdout` events as complete lines arrive. 6. Read the exit sentinel. Drain final bytes, emit `exit`, then close the session and runtime (`POST /close_session`, `POST /close`). +7. **Teardown reaps the agent group before close.** After the run completes, the + runtime kills the agent's process group (TERM → 1 s grace → KILL on the + negative pgid recorded in `agent.pid`) BEFORE `rm -rf` of the run dir and the + `/close_session` + `/close` calls, so a detached agent is never left alive on + the server. See [Agent process-group reaping on teardown](#agent-process-group-reaping-on-teardown). The overall run timeout defaults to `600000` ms (10 minutes) and is configurable via `batch.agentTimeoutMs` or the `RATCHET_AGENT_TIMEOUT_MS` environment variable @@ -273,7 +548,60 @@ flowchart TD class resolved out; ``` -## Agent adapters +## Agent process-group reaping on teardown + +Every runtime now deterministically reaps the spawned agent's **process group** +on teardown, so a detached agent can never outlive the runtime that launched it. +This fixes the leak where `nohup`-style launchers (remote) and SWE-ReX's own +`execute()` (sidecar) left the agent alive after the runtime session closed. + +### Launch (job control) + +Each runtime launches the agent under **job control** (`set -m`) so the agent +pipeline becomes its own process-group leader, and records that group's pid to a +**pidfile** so teardown can target the whole group: + +- **Sidecar (`sidecar.py`)** — the run-op shell command is wrapped as + `set -m; bash -c ' >log 2>&1 & echo $! >pid; wait; echo $? >done`. The + backgrounded pipeline's pid is written to `/agent.pid`; the sidecar + tracks it via `self.run_pidfile`. The `run_dir` is threaded from the Node side + (host runDir for `local`, the in-container equivalent for `docker`), falling + back to the sidecar workdir when absent. +- **Remote (`rex-remote-runtime.ts`)** — `buildRemoteRunCommand` emits + `set -m; nohup … & echo $! >/agent.pid` inside its existing + `( … )` subshell, so the detached agent's pid is recorded on the server. +- **Legacy in-process (`agent.ts:makeRealSpawner`)** — spawns the agent with + `detached: true` (POSIX) so the child is its own group leader; `reapGroup` + targets the negative pid. + +### Teardown (group reap before close) + +Teardown never just closes the session and leaves — it reaps the group **first**: + +1. **Sidecar** — `teardownChild` sends `{op:"shutdown"}` so the sidecar stops its + SWE-ReX deployment and reaps the agent group (`_reap_agent_group`: TERM → + grace → KILL on the negative pgid from the pidfile) as part of shutdown. Only + after the grace does the Node side `killGroup(SIGKILL)` the sidecar's own + process group if it has not exited. No `SIGTERM` is sent to the sidecar + directly — that would kill it before it could reap the agent. +2. **Remote** — `teardown()` kills the group (`kill -TERM -- -$(cat agent.pid)` → + sleep 1 → `kill -KILL -- -$(cat agent.pid)`) BEFORE `rm -rf` of the run dir and + the `POST /close_session` + `POST /close` calls. +3. **Legacy in-process** — on overall timeout, `reapGroup` sends TERM → grace → + KILL to the negative pid. + +The sidecar's SIGTERM handler raises `SystemExit(0)` after shutdown, so a + SIGTERM sent to the sidecar process itself still reaps the agent group and + exits cleanly rather than dying mid-teardown. + +### Idempotency + +`_reap_agent_group` claims the pidfile (sets `self.run_pidfile = None`) on entry, +so a concurrent shutdown + SIGTERM is safe — only one call performs the kill; +the second is a no-op. Missing, empty, or non-numeric pidfiles skip the kill +(best-effort, never throws). + + An adapter knows how to build the spawn request for one coding agent. The engine resolves the adapter by name from the resolved settings before any spawn, and @@ -814,10 +1142,14 @@ The default posture is `repo-sandboxed-permissive`. in `allow` or any shell step will stall headless. - **`full-autonomy`**: all permission checks are bypassed. -### Baseline deny patterns (`repo-sandboxed-permissive`) +### Baseline deny patterns (best-effort damage reduction) The following patterns are merged into the effective denylist for the sandboxed -and curated postures (not for `full-autonomy`): +and curated postures (not for `full-autonomy`). They block a handful of +destructively-shaped shell commands at the agent's own permission layer — this is +**best-effort damage reduction, not containment** (see the +[threat model per locus](#threat-model-per-locus) above). Real containment is the +docker locus; the denylist catches accidental foot-guns on `local`: ``` Bash(rm -rf *) @@ -917,9 +1249,12 @@ built-in default ← user/global ← project config (.ratchet/config.yaml batch: Scalar settings (including `locus`, `agent`, `image`, `host`, `port`, `authToken`, `insecure`) are nearest-wins. Permissions use per-field merge -semantics: posture nearest-wins, `deny` is the union of all scopes, `allow` is -replaced by the nearest defining scope, and each agent's `raw` entry is -nearest-wins. +semantics: posture nearest-wins **across the operator-owned scopes** +(default/user/project); the **manifest scope is narrow-only** — it may only +LOWER the posture, never raise it above the operator-owned scopes' accumulated +value (a raise is clamped unless `batch apply --allow-manifest-escalation` is +passed); `deny` is the union of all scopes; `allow` is replaced by the nearest +defining scope, and each agent's `raw` entry is nearest-wins. Built-in defaults: diff --git a/docs/eval-mutation-harness.md b/docs/eval-mutation-harness.md index 6dc7096..781dc6b 100644 --- a/docs/eval-mutation-harness.md +++ b/docs/eval-mutation-harness.md @@ -122,14 +122,21 @@ export async function runMutationHarness( caught: it propagates to the caller as "could not run at all", distinct from "ran and was red". 3. **Seed** — for each of up to `invariant.budget` attempts, the harness - builds a spawn request the same way `judge.ts`'s `buildVoteRequest` does: - `RATCHET_EVAL_AGENT_CMD`, when set, stands in for the agent binary - (deterministic e2e testing); otherwise `resolveAdapter(deps.agentName)` - resolves the configured coding agent's adapter and `buildRequest` builds - the request, which `deps.spawner` (default `realSpawner`) runs. The - instructions (`buildSeedInstructions`) ask the agent to make exactly one - small, discrete edit to a non-test source file and to not run the test - suite itself. + builds a spawn request the same way `judge.ts`'s `buildVoteRequest` does: + `RATCHET_EVAL_AGENT_CMD`, when set, stands in for the agent binary + (deterministic e2e testing); otherwise `resolveAdapter(deps.agentName)` + resolves the configured coding agent's adapter and `buildRequest` builds + the request, which `deps.spawner` (default `realSpawner`) runs. The + override gate and spawn-request construction live in one shared helper + (`buildAgentSpawnRequest` in `src/core/batch/engine/agent.ts`), shared with + the batch engine and the judge, so the three spawn seams cannot drift + apart. An active `RATCHET_EVAL_AGENT_CMD` is loud and auditable: + `eval run` prints the one-line notice `⚠ agent overridden by + RATCHET_EVAL_AGENT_CMD` atop the scorecard, emits `agentOverride: true` in + `--json`, and stamps `via: "env-override"` on the persisted run record. The + instructions (`buildSeedInstructions`) ask the agent to make exactly one + small, discrete edit to a non-test source file and to not run the test + suite itself. 4. **Detect** — `git add -A` (stages tracked and untracked changes, so a new file the agent created is not silently invisible) followed by `git diff --cached` captures the fault as a unified diff. An **empty @@ -227,8 +234,11 @@ stays independently testable in isolation from that reduction. ## Agent-neutrality Every seed request is built through `resolveAdapter(deps.agentName).buildRequest(...)` -(or the `RATCHET_EVAL_AGENT_CMD` test override) — the same adapter registry -and spawn seam `judge.ts`'s `llm-judge` binding uses. There is no -agent-specific branch anywhere in this module: `runMutationHarness` never +(or the `RATCHET_EVAL_AGENT_CMD` test override, gated through the single shared +`buildAgentSpawnRequest` helper) — the same adapter registry and spawn seam +`judge.ts`'s `llm-judge` binding uses. An active override prints the one-line +`⚠ agent overridden by RATCHET_EVAL_AGENT_CMD` notice, emits `agentOverride: +true` in `--json`, and stamps `via: "env-override"` on the run record. There is +no agent-specific branch anywhere in this module: `runMutationHarness` never checks which coding agent is configured before seeding, satisfying the `multi-agent-support` standard by construction. diff --git a/src/commands/batch/apply.ts b/src/commands/batch/apply.ts index 5a7bda7..16be1a2 100644 --- a/src/commands/batch/apply.ts +++ b/src/commands/batch/apply.ts @@ -12,7 +12,7 @@ import { execFileSync } from 'child_process'; import { resolveCurrentPlanningHomeSync } from '../../core/planning-home.js'; import { loadBatchManifest, type Phase } from '../../core/batch/manifest.js'; import { computeBatchStatus } from '../../core/batch/status.js'; -import { resolveBatchSettings, type PrGrouping } from '../../core/batch/config.js'; +import { resolveBatchSettings, type PrGrouping, type SuppressedEscalation } from '../../core/batch/config.js'; import { RatchetBatchEngine, computeNextTransition, @@ -28,6 +28,8 @@ import { type StepResult, type ProofOfWorkResult, type RunProofOfWorkDeps, + agentOverrideNotice, + BATCH_AGENT_CMD_ENV, } from '../../core/batch/engine/index.js'; import type { BatchStatusInfo, ChangeStatus } from '../../core/batch/status.js'; // The two pure stacked-grouping policies are imported from their own modules — @@ -53,6 +55,15 @@ import { resolveBatchName } from './shared.js'; export interface BatchApplyOptions { json?: boolean; + /** + * Per-invocation opt-in letting a repo-committed manifest RAISE the posture + * above the operator-owned (default/user/project) scopes. Default `false`: + * the manifest may only NARROW posture (lower it); a raise is clamped and + * surfaced via a posture warning on `batch apply`. Pass `true` (the + * `--allow-manifest-escalation` flag) to let a manifest raise posture + * unchanged. See {@link resolveBatchSettings}. + */ + allowManifestEscalation?: boolean; } /** The git branch names the completion PR step opens between, resolved by the CLI. */ @@ -146,9 +157,31 @@ export async function batchApplyCommand( const projectRoot = deps.projectRoot ?? resolveCurrentPlanningHomeSync().root; const batch = resolveBatchName(projectRoot, name); const manifest = loadBatchManifest(projectRoot, batch); - const { settings, agentStageScopes } = resolveBatchSettings(projectRoot, manifest); + const { settings, sources, agentStageScopes, suppressedEscalation } = resolveBatchSettings( + projectRoot, + manifest, + { allowManifestEscalation: options.allowManifestEscalation } + ); const status = await computeBatchStatus(projectRoot, manifest); + // Surface the effective permission posture and its source scope at the start + // of every human-readable run so the permission story is visible up front + // instead of buried in config (`apply-posture-banner.feature`). `--json` + // suppresses the line (machine consumers read the resolved settings they + // build themselves); the banner is for interactive operators. + if (!options.json && settings.permissions) { + renderPostureBanner(settings.permissions.posture, sources.permissions); + } + + // Surface a manifest posture-raise refusal as a human-facing warning BEFORE + // any engine work, so the operator knows the effective posture was clamped to + // the operator-owned scopes' value and how to opt in. `--json` suppresses it + // (machine callers read the structured `suppressedEscalation` on the resolved + // settings they build themselves); the banner is for interactive operators. + if (suppressedEscalation && !options.json && settings.permissions) { + renderSuppressedEscalationWarning(suppressedEscalation, settings.permissions.posture); + } + // The engine is bundled into this package; construct it and run in-process. const engine = new RatchetBatchEngine(); @@ -1031,6 +1064,8 @@ async function runProofAtBoundary( policy: result.policy, reason: result.reason, detail: result.detail, + conditionKind: result.conditionKind, + matchedExcerpt: result.matchedExcerpt, }; recordProofOfWork(projectRoot, batch, phase.name, record); renderProofOutcome(phase.name, result, options); @@ -1086,6 +1121,17 @@ async function renderResult( return; } + // An override notice rides the result, not a side-channel print: the engine + // sets `agentOverride` on the step outcome exactly when its spawn ran under + // an active `RATCHET_BATCH_AGENT_CMD`. The notice appears before the result + // line so a leftover override is the first thing the operator sees; --json + // carries the field via the stringify above (no notice interleaved into the + // JSON document). Pre-spawn parks (`notAdvanced`) carry no `agentOverride` + // and print no notice — nothing was spawned. + if (result.agentOverride) { + console.log(chalk.yellow(agentOverrideNotice(BATCH_AGENT_CMD_ENV))); + } + console.log(chalk.bold(`\nRan: ${result.change} (${result.transition})`)); switch (result.state) { case 'advanced': @@ -1101,3 +1147,39 @@ async function renderResult( console.log(chalk.dim(result.message ?? result.state)); } } + +/** + * Render the one-line effective-posture banner that opens every human-readable + * `batch apply` run: `permissions: ( scope)`. Names the + * effective permission posture and which scope supplied it so the permission + * story is visible up front instead of buried in config. Suppressed under + * `--json` (machine consumers read the resolved settings they build themselves). + */ +function renderPostureBanner(posture: string, source: string): void { + console.log(chalk.dim(`permissions: ${posture} (${source} scope)`)); +} + +/** + * Render a human-facing posture banner when a repo-committed manifest tried to + * RAISE the posture above the operator-owned (default/user/project) scopes and + * the raise was clamped. Names the requested posture, the effective (clamped) + * posture, and the `--allow-manifest-escalation` opt-in so the operator knows + * both that the effective posture was held back and how to allow the raise. + * Printed once before any engine work; `--json` callers read the structured + * `suppressedEscalation` on the resolved settings they build themselves. + */ +function renderSuppressedEscalationWarning( + suppressed: SuppressedEscalation, + effectivePosture: string +): void { + console.log( + chalk.yellow( + `⚠ manifest requested posture '${suppressed.requested}' (higher than operator scopes); clamped to '${effectivePosture}'.` + ) + ); + console.log( + chalk.dim( + ` To allow the raise: rerun with --allow-manifest-escalation, or raise posture in your user/project config (operator-owned scopes).` + ) + ); +} diff --git a/src/commands/batch/report.ts b/src/commands/batch/report.ts index d1386f4..a9a213a 100644 --- a/src/commands/batch/report.ts +++ b/src/commands/batch/report.ts @@ -16,6 +16,11 @@ import { recordAnswer, recordReject, } from '../../core/batch/journal.js'; +import { + activeAgentCmdOverride, + BATCH_AGENT_CMD_ENV, + ENV_OVERRIDE_PROVENANCE, +} from '../../core/batch/engine/agent.js'; export interface BatchReportOptions { change?: string; @@ -71,6 +76,21 @@ export async function batchReportCommand( console.log(result.text); } +/** + * The `via` provenance stamped on every entry `batch report` appends when its + * own process environment carries an active `RATCHET_BATCH_AGENT_CMD`. The + * spawned stand-in inherits the var (the engine builds env from `process.env`), + * so a stub-reported completion/blocker/progress/needs-input entry is stamped + * even though the engine never sees that append — closing the #78 audit hole. + * Absent (no stamp) when the override is inactive, so override-free runs are + * byte-identical. + */ +function overrideProvenance(): { via?: 'env-override' } { + return activeAgentCmdOverride(BATCH_AGENT_CMD_ENV, process.env) !== undefined + ? { via: ENV_OVERRIDE_PROVENANCE } + : {}; +} + function applyReport( projectRoot: string, batch: string, @@ -81,11 +101,11 @@ function applyReport( ): { kind: string; change: string; text: string } { switch (kind) { case 'status': - appendJournal(projectRoot, batch, { change, kind: 'progress', message }); + appendJournal(projectRoot, batch, { change, kind: 'progress', message, ...overrideProvenance() }); return { kind, change, text: chalk.dim(`Recorded progress for ${change}: ${message}`) }; case 'blocker': - appendJournal(projectRoot, batch, { change, kind: 'blocker', message }); + appendJournal(projectRoot, batch, { change, kind: 'blocker', message, ...overrideProvenance() }); parkStep(projectRoot, batch, { change, kind: 'blocked', reason: message }); return { kind, @@ -94,7 +114,7 @@ function applyReport( }; case 'needs-input': - appendJournal(projectRoot, batch, { change, kind: 'needs-input', message }); + appendJournal(projectRoot, batch, { change, kind: 'needs-input', message, ...overrideProvenance() }); parkStep(projectRoot, batch, { change, kind: 'blocked', reason: message }); return { kind, @@ -103,7 +123,7 @@ function applyReport( }; case 'complete': - appendJournal(projectRoot, batch, { change, kind: 'completion', message }); + appendJournal(projectRoot, batch, { change, kind: 'completion', message, ...overrideProvenance() }); // Under an after-propose gate, a finished propose parks for approval. if (options.awaitingApproval) { parkStep(projectRoot, batch, { diff --git a/src/commands/eval/run.ts b/src/commands/eval/run.ts index 571a469..a47adfa 100644 --- a/src/commands/eval/run.ts +++ b/src/commands/eval/run.ts @@ -14,6 +14,10 @@ import chalk from 'chalk'; import { executeRun, evaluateRun, type EvalReport } from '../../core/eval/index.js'; +import { + activeAgentCmdOverride, + agentOverrideNotice, +} from '../../core/batch/engine/agent.js'; import { projectRoot, resolveScope, @@ -23,6 +27,14 @@ import { type ScopeFlags, } from './shared.js'; +/** + * The env var that arms the eval agent-cmd override. Declared locally so the + * run-command's notice/json stamp is self-documenting; the override GATE itself + * lives in the shared `buildAgentSpawnRequest` helper used by the judge and + * mutation harness. + */ +const EVAL_AGENT_CMD_ENV = 'RATCHET_EVAL_AGENT_CMD'; + export interface EvalRunOptions extends ScopeFlags { /** `--gate `: set the enabled contributor set outright. */ gate?: string; @@ -73,6 +85,10 @@ export async function evalRunCommand(options: EvalRunOptions = {}): Promise 0 ? trimmed : undefined; +} + +/** + * The one-line override notice printed (text output) when a spawn ran under an + * active agent-cmd override. Mirrors the `RATCHET_EVAL_AGENT_CMD` notice on the + * eval side. + */ +export function agentOverrideNotice(envVar: string): string { + return `⚠ agent overridden by ${envVar}`; +} + +/** + * Build an override-aware agent spawn request through the single shared gate. + * + * When `activeAgentCmdOverride(overrideEnvVar, env)` is active, the override + * command stands in for the coding-agent binary as `bash -c ` + * (instructions on stdin, NOT stream-json-capable) and `agentOverride` is + * `true`. Otherwise the supplied `buildAdapterRequest` closure builds the + * configured-adapter request and `agentOverride` is `false`. The closure owns + * site-specific adapter resolution (the engine's stage-map/spec logic, the eval + * side's bare `resolveAdapter`), so this helper owns ONLY the shared override + * semantics (trim check, `bash -c` request shape, flag) — the gate exists in + * exactly one place without flattening genuinely different adapter paths. + * + * Coordinates with the #67 triplication: the batch engine's `buildSpawnRequest`, + * the eval judge's `buildVoteRequest`, and the mutation harness's + * `buildSeedRequest` all delegate their override branch here. + */ +export function buildAgentSpawnRequest(args: { + overrideEnvVar: string; + instructions: string; + cwd: string; + env: NodeJS.ProcessEnv; + buildAdapterRequest: () => AgentSpawnRequest; +}): { request: AgentSpawnRequest; agentOverride: boolean } { + const override = activeAgentCmdOverride(args.overrideEnvVar, args.env); + if (override !== undefined) { + return { + request: { + command: 'bash', + args: ['-c', override], + instructions: args.instructions, + cwd: args.cwd, + env: args.env, + }, + agentOverride: true, + }; + } + return { request: args.buildAdapterRequest(), agentOverride: false }; +} + /** The injectable process-spawn seam. */ export type Spawner = (request: AgentSpawnRequest) => Promise; @@ -102,6 +184,16 @@ export interface AgentAdapter { cwd: string, env: NodeJS.ProcessEnv ): AgentSpawnRequest; + /** + * Environment variable names (or `PREFIX_*` glob patterns) this adapter needs + * from the host environment to function (API keys, config dirs, etc.). The + * engine's env allowlist passes these through alongside the baseline process + * vars and `RATCHET_*` control vars. A `PREFIX_*` entry matches any var whose + * name starts with `PREFIX_` (the trailing `_*` is the glob). Required on + * every built-in adapter so the registry drift guard can assert one exists + * per agent — a newly added agent cannot silently ship without a declaration. + */ + readonly envPassthrough: readonly string[]; } /** @@ -122,7 +214,13 @@ class CommandAgentAdapter implements AgentAdapter { * to the base argv the adapter already owns, not in a separate model-flag * registry, so `AI_TOOLS` stays about init/binaries. */ - readonly modelFlag: string + readonly modelFlag: string, + /** + * Environment variable names (or `PREFIX_*` glob patterns) this adapter + * needs from the host env. Threaded to the env allowlist so the agent's + * own secrets reach it without leaking everything else in `process.env`. + */ + readonly envPassthrough: readonly string[] ) {} buildRequest( @@ -190,26 +288,55 @@ const BUILTIN_ADAPTERS: Record = { () => ['-p', '--output-format', 'stream-json', '--verbose', '--include-partial-messages'], true, true, - '--model' + '--model', + ['ANTHROPIC_API_KEY', 'CLAUDE_CODE_*'] ), // codex uses `-m` to name a model. - codex: new CommandAgentAdapter('codex', agentBinaryFor('codex'), () => ['exec', '-'], true, false, '-m'), + codex: new CommandAgentAdapter( + 'codex', + agentBinaryFor('codex'), + () => ['exec', '-'], + true, + false, + '-m', + ['OPENAI_API_KEY', 'CODEX_HOME'] + ), // gemini uses `-m` to name a model. - gemini: new CommandAgentAdapter('gemini', agentBinaryFor('gemini'), () => ['-p'], true, false, '-m'), + gemini: new CommandAgentAdapter( + 'gemini', + agentBinaryFor('gemini'), + () => ['-p'], + true, + false, + '-m', + ['GEMINI_API_KEY', 'GOOGLE_API_KEY'] + ), // cursor uses `--model` to name a model. - cursor: new CommandAgentAdapter('cursor', agentBinaryFor('cursor'), () => ['-p'], true, false, '--model'), + cursor: new CommandAgentAdapter( + 'cursor', + agentBinaryFor('cursor'), + () => ['-p'], + true, + false, + '--model', + ['CURSOR_API_KEY', 'CURSOR_*'] + ), // opencode emits structured stream-json NDJSON (one event per line) with // `run --format json`, reading the prompt from stdin. Its event schema // (step_start/text/step_finish) differs from claude's, so the renderer parses // both — gated on `emitsStreamJson`, never the agent name. `--model` is - // opencode's model flag. + // opencode's model flag. opencode is multi-provider: it can drive any of the + // other agents' provider keys, but those are covered by the UNION of all + // adapter declarations (claude/codex/gemini/cursor), so opencode only + // declares its own namespaced config vars here. opencode: new CommandAgentAdapter( 'opencode', agentBinaryFor('opencode'), () => ['run', '--format', 'json'], true, true, - '--model' + '--model', + ['OPENCODE_*'] ), }; @@ -280,31 +407,99 @@ export function resolveAdapter( /** * The real spawner: runs the agent binary, feeds instructions on stdin when * present, and captures stdout/stderr and exit status. + * + * On POSIX the child is spawned `detached: true` so it leads its own process + * group (pgid == pid); a hung agent is reaped by escalating TERM → grace → KILL + * on the whole group (so grandchildren like `cat prompt | agent` are reaped + * too, not just the recorded pid), resolving with a timeout message in stderr + * instead of hanging forever. On Windows process groups aren't a thing — it + * falls back to a bare `child.kill(sig)`. + * + * Use {@link makeRealSpawner} to tune `timeoutMs`/`killGraceMs`; the exported + * `realSpawner` is built from the factory with defaults so the eval judge and + * mutation harness inherit the timeout/kill semantics unchanged. */ -export const realSpawner: Spawner = (request) => - new Promise((resolve, reject) => { - const child = spawn(request.command, request.args, { - cwd: request.cwd, - env: request.env, - stdio: ['pipe', 'pipe', 'pipe'], - }); +export function makeRealSpawner( + opts: { timeoutMs?: number; killGraceMs?: number } = {} +): Spawner { + const timeoutMs = opts.timeoutMs ?? 10 * 60 * 1000; + const killGraceMs = opts.killGraceMs ?? 2000; + const isPosix = process.platform !== 'win32'; - let stdout = ''; - let stderr = ''; - child.stdout?.on('data', (d: Buffer) => { - stdout += d.toString(); - }); - child.stderr?.on('data', (d: Buffer) => { - stderr += d.toString(); - }); + return (request: AgentSpawnRequest) => + new Promise((resolve, reject) => { + const child = spawn(request.command, request.args, { + cwd: request.cwd, + env: request.env, + stdio: ['pipe', 'pipe', 'pipe'], + detached: isPosix, // own process group on POSIX; harmless on Windows + }); + + let stdout = ''; + let stderr = ''; + let settled = false; + let killTimer: ReturnType | undefined; + + const reapGroup = (signal: NodeJS.Signals) => { + const pid = child.pid; + if (pid !== undefined && isPosix) { + try { + process.kill(-pid, signal); + return; + } catch { + /* group gone — fall through */ + } + } + try { + child.kill(signal); + } catch { + /* already gone */ + } + }; - child.on('error', (err) => reject(err)); - child.on('close', (exitCode, signal) => { - resolve({ exitCode, signal, stdout, stderr }); + child.stdout?.on('data', (d: Buffer) => { + stdout += d.toString(); + }); + child.stderr?.on('data', (d: Buffer) => { + stderr += d.toString(); + }); + + const overall = setTimeout(() => { + if (settled) return; + const msg = `Agent timed out after ${timeoutMs}ms`; + stderr += (stderr ? '\n' : '') + msg; + onTimeout(); + }, timeoutMs); + + const onTimeout = () => { + // TERM the whole group, short grace, then KILL; resolve with a timeout + // result (non-zero exit) instead of hanging. + reapGroup('SIGTERM'); + killTimer = setTimeout(() => reapGroup('SIGKILL'), killGraceMs); + }; + + const finish = (result: AgentSpawnResult) => { + if (settled) return; + settled = true; + clearTimeout(overall); + if (killTimer) clearTimeout(killTimer); + resolve(result); + }; + + child.on('error', (err) => { + if (settled) return; + clearTimeout(overall); + reject(err); + }); + child.on('close', (exitCode, signal) => { + finish({ exitCode, signal, stdout, stderr }); + }); + + if (request.instructions && child.stdin) { + child.stdin.write(request.instructions); + child.stdin.end(); + } }); +} - if (request.instructions && child.stdin) { - child.stdin.write(request.instructions); - child.stdin.end(); - } - }); +export const realSpawner: Spawner = makeRealSpawner(); diff --git a/src/core/batch/engine/context.ts b/src/core/batch/engine/context.ts index 8bbc2e3..3fd2e7b 100644 --- a/src/core/batch/engine/context.ts +++ b/src/core/batch/engine/context.ts @@ -30,6 +30,13 @@ export interface EngineStepOutcome { detail?: string; message?: string; journalRefs?: number[]; + /** + * Present (true) when this step's spawn ran under an active agent-cmd + * override. Carried to {@link StepResult.agentOverride} so `--json` emits the + * field and the text renderer prints the override notice. Absent when no + * override was active — byte-identical to override-free runs. + */ + agentOverride?: true; } /** @@ -53,6 +60,7 @@ export function toStepResult(outcome: EngineStepOutcome): StepResult { detail: outcome.detail, journalRefs: outcome.journalRefs, message: outcome.message ?? outcome.detail, + ...(outcome.agentOverride ? { agentOverride: true } : {}), }; } return { @@ -64,6 +72,7 @@ export function toStepResult(outcome: EngineStepOutcome): StepResult { detail: outcome.detail, journalRefs: outcome.journalRefs, message: outcome.message, + ...(outcome.agentOverride ? { agentOverride: true } : {}), }; } diff --git a/src/core/batch/engine/contract.ts b/src/core/batch/engine/contract.ts index 5ba1b55..08728ac 100644 --- a/src/core/batch/engine/contract.ts +++ b/src/core/batch/engine/contract.ts @@ -245,4 +245,11 @@ export interface StepResult { /** Pointer to journal entries this step produced (indices or ids). */ journalRefs?: number[]; message?: string; + /** + * Present (true) when this step's spawn ran under an active agent-cmd + * override (`RATCHET_BATCH_AGENT_CMD`). `--json` carries the field verbatim; + * the text renderer prints the one-line override notice. Absent when no + * override was active — byte-identical output for override-free runs. + */ + agentOverride?: true; } diff --git a/src/core/batch/engine/engine.ts b/src/core/batch/engine/engine.ts index 091f4bf..3498d6b 100644 --- a/src/core/batch/engine/engine.ts +++ b/src/core/batch/engine/engine.ts @@ -38,11 +38,15 @@ import type { AgentStage, AgentSpec } from '../agent-setting.js'; import { resolveAdapter, UnknownAgentError, + buildAgentSpawnRequest, + BATCH_AGENT_CMD_ENV, + ENV_OVERRIDE_PROVENANCE, type AgentAdapter, type AgentSpawnRequest, type AgentSpawnResult, type Spawner, } from './agent.js'; +import { scopeAgentEnv } from './agent-env.js'; import type { AgentEvent, AgentRuntime } from './runtime/contract.js'; import { makeRexSidecarRuntime } from './runtime/rex-sidecar-runtime.js'; import { makeRexRemoteRuntime } from './runtime/rex-remote-runtime.js'; @@ -64,6 +68,7 @@ import { type SkillLocusDeps, } from './skill-locus.js'; import { mapSessionToOutcome, type ModelAttribution } from './outcome.js'; +import { parksForApproval } from './approval-gate.js'; import { toStepResult, resolveProjectRoot, type EngineStepOutcome } from './context.js'; import { computeNextTransition, @@ -187,7 +192,9 @@ export class RatchetBatchEngine { * is the ONLY place that branches on locus: `local` drives the ReX sidecar * with `REX_LOCUS=local` and `REX_WORKDIR=projectRoot`; `docker` drives the * SAME sidecar runtime with `REX_LOCUS=docker` plus the resolved `image` - * (the project root is bind-mounted by the runtime/sidecar); `remote` drives + * and docker hardening knobs (`dockerUser`/`dockerMemory`/`dockerPidsLimit`/ + * `dockerCpus`/`network`, threaded as `REX_DOCKER_*` env vars by the + * bootstrap; the project root is bind-mounted by the runtime/sidecar); `remote` drives * the native-Node `RexRemoteRuntime` over the swerex-remote REST API with the * resolved host/port/authToken (no local Python). The engine, renderer, and * event channel are otherwise locus-agnostic — streaming and rendering are @@ -222,7 +229,16 @@ export class RatchetBatchEngine { return makeRexSidecarRuntime({ locus, projectRoot, - ...(locus === 'docker' ? { image: settings.image } : {}), + ...(locus === 'docker' + ? { + image: settings.image, + dockerUser: settings.dockerUser, + dockerMemory: settings.dockerMemory, + dockerPidsLimit: settings.dockerPidsLimit, + dockerCpus: settings.dockerCpus, + network: settings.network, + } + : {}), ...(timeoutMs !== undefined ? { timeoutMs } : {}), }); } @@ -330,16 +346,18 @@ export class RatchetBatchEngine { // prompt file under `.ratchet/batches//.run//`. With no batch the // runtime falls back to the change-local `.run/`, so the var is omitted. const env: NodeJS.ProcessEnv = batch - ? { ...process.env, RATCHET_BATCH_NAME: batch } - : { ...process.env }; + ? { ...scopeAgentEnv(process.env), RATCHET_BATCH_NAME: batch } + : { ...scopeAgentEnv(process.env) }; let request; let emitsStreamJson = false; let spec: AgentSpec | undefined; + let agentOverride = false; try { const built = this.buildSpawnRequest(ctx, instructions, projectRoot, env, transition); request = built.request; emitsStreamJson = built.emitsStreamJson; spec = built.spec; + agentOverride = built.agentOverride; } catch (err) { if (err instanceof UnknownAgentError) { return toStepResult({ @@ -383,6 +401,7 @@ export class RatchetBatchEngine { transition, parkForApproval: this.shouldParkForApproval(ctx, transition), modelAttribution, + agentOverride, before, diskBefore, diskAfter: () => { @@ -467,10 +486,11 @@ export class RatchetBatchEngine { } const instructions = buildDecompositionInstructions(context); - const env: NodeJS.ProcessEnv = { ...process.env, RATCHET_BATCH_NAME: batch }; + const env: NodeJS.ProcessEnv = { ...scopeAgentEnv(process.env), RATCHET_BATCH_NAME: batch }; let request; let emitsStreamJson = false; let spec: AgentSpec | undefined; + let agentOverride = false; try { const built = this.buildSpawnRequest( { batch, change: key, settings: context.settings }, @@ -482,6 +502,7 @@ export class RatchetBatchEngine { request = built.request; emitsStreamJson = built.emitsStreamJson; spec = built.spec; + agentOverride = built.agentOverride; } catch (err) { if (err instanceof UnknownAgentError) { return toStepResult({ @@ -519,7 +540,11 @@ export class RatchetBatchEngine { // Same spawn→stream→map→journal tail as the change core. The decomposition // artifact is the `batch.yaml` edit the agent makes — there is no change // directory to stamp or measure — so disk evidence is a no-op snapshot - // (before === after) and there is never an approval park. + // (before === after) and, per the gate matrix, a decomposition step never + // parks for approval under any gate (a PR is itself the checkpoint; a + // decomposition's authored intents are reviewed when each change's propose + // parks). Routed through the same `parksForApproval` matrix so the policy + // has one author. return this.spawnAndMap({ request, emitsStreamJson, @@ -528,8 +553,9 @@ export class RatchetBatchEngine { locus, change: key, transition: 'decompose', - parkForApproval: false, + parkForApproval: parksForApproval(context.settings.gate, 'decompose'), modelAttribution, + agentOverride, before, diskBefore: diskState, diskAfter: () => diskState, @@ -640,10 +666,11 @@ export class RatchetBatchEngine { } const instructions = buildPrInstructions(context); - const env: NodeJS.ProcessEnv = { ...process.env, RATCHET_BATCH_NAME: batch }; + const env: NodeJS.ProcessEnv = { ...scopeAgentEnv(process.env), RATCHET_BATCH_NAME: batch }; let request; let emitsStreamJson = false; let spec: AgentSpec | undefined; + let agentOverride = false; try { // Route the PR step through the `pr` STAGE of the agent map, exactly as a // change step routes propose/apply/verify: a stage-map spawns the mapped @@ -660,6 +687,7 @@ export class RatchetBatchEngine { request = built.request; emitsStreamJson = built.emitsStreamJson; spec = built.spec; + agentOverride = built.agentOverride; } catch (err) { if (err instanceof UnknownAgentError) { return toStepResult({ @@ -688,7 +716,9 @@ export class RatchetBatchEngine { // Snapshot the PR journal (keyed by batch) so we can isolate this session's // entries. There is no change directory for a PR step, so — like the // decomposition path — disk evidence is a no-op snapshot (before === after) - // and there is never an approval park. + // and, per the gate matrix, a PR-open step never parks for approval under + // any gate (the PR is itself the human checkpoint). Routed through the same + // `parksForApproval` matrix so the policy has one author. const locus: RunLocus = { batch }; const before = readChangeJournalTolerantForLocus(projectRoot, locus, key).length; const diskState = readChangeDiskState(projectRoot, key); @@ -701,8 +731,9 @@ export class RatchetBatchEngine { locus, change: key, transition: 'pr', - parkForApproval: false, + parkForApproval: parksForApproval(context.settings.gate, 'pr'), modelAttribution, + agentOverride, before, diskBefore: diskState, diskAfter: () => diskState, @@ -735,6 +766,7 @@ export class RatchetBatchEngine { transition: StepKind; parkForApproval: boolean; modelAttribution?: ModelAttribution; + agentOverride?: boolean; before: number; diskBefore: ChangeDiskState; diskAfter: () => ChangeDiskState; @@ -749,6 +781,7 @@ export class RatchetBatchEngine { transition, parkForApproval, modelAttribution, + agentOverride, before, diskBefore, diskAfter, @@ -801,6 +834,12 @@ export class RatchetBatchEngine { modelAttribution, }); + // An active agent-cmd override marks the outcome so the rendered result + // carries `agentOverride: true` (--json carries it verbatim; the text + // renderer prints the one-line notice), and stamps `via: env-override` + // provenance on the transition-outcome journal entry produced below. + if (agentOverride) outcome.agentOverride = true; + // Record a journal entry for the transition outcome (the agent may not have // reported one, e.g. on failure), so resume sees this step. Written at the // resolved locus — the batch run dir, or the change-local `.run/`. @@ -812,6 +851,7 @@ export class RatchetBatchEngine { outcome.blocker ?? `${transition} ${outcome.state}`, transition, + ...(agentOverride ? { via: ENV_OVERRIDE_PROVENANCE } : {}), }); return toStepResult(outcome); @@ -831,47 +871,55 @@ export class RatchetBatchEngine { projectRoot: string, env: NodeJS.ProcessEnv, stage?: AgentStage - ): { request: AgentSpawnRequest; emitsStreamJson: boolean; spec?: AgentSpec } { - const override = process.env.RATCHET_BATCH_AGENT_CMD; - if (override && override.trim().length > 0) { - // The `bash -c` override stands in for the agent binary and is NOT - // stream-json-capable (keeps e2e/eval deterministic) → raw streaming. It - // bypasses spec parsing, so no `AgentSpec` is returned — the override - // path carries no model attribution by construction. - return { - request: { command: 'bash', args: ['-c', override], instructions, cwd: projectRoot, env }, - emitsStreamJson: false, - }; - } - // Resolve the spawn agent for the running transition's STAGE when one is - // given (propose/apply/verify/decompose/pr) — a stage-map routes each stage - // independently; a scalar/unset agent resolves the same for every stage. - // Either way `resolveAdapter` maps an unmapped-stage/unset name to - // `DEFAULT_AGENT` and rejects an unknown name (`UnknownAgentError`) before - // any spawn. - const resolved = stage - ? resolveAgentForStage(context.settings.agent, stage) - : scalarAgent(context.settings.agent); - // Parse the resolved spec ONCE per transition: the stored value is a whole - // `agent[:model]` spec string (config load already rejected malformed specs, - // so this parse cannot throw on validated config). The adapter is resolved by - // the AGENT PART — an unknown agent part still throws `UnknownAgentError` - // before any spawn, naming `rex` not `rex:some-model`; the model part is - // threaded to the adapter via `AgentRequestContext.model` so the adapter - // emits its own flag. A bare agent name (no `:`) parses to `{ agent }` with - // no `model` key, so the adapter emits no flag and the agent uses its - // harness-configured default model — byte-for-byte today's argv. - const spec = resolved !== undefined ? parseAgentSpec(resolved) : undefined; - const adapter = resolveAdapter(spec?.agent, this.adapters); + ): { request: AgentSpawnRequest; emitsStreamJson: boolean; spec?: AgentSpec; agentOverride: boolean } { + // The override gate lives in the shared helper (`buildAgentSpawnRequest`) so + // the engine, the eval judge, and the mutation harness share one override + // seam (the #67 triplication). The adapter-resolution closure owns the + // site-specific stage-map/spec logic; the helper owns only the shared + // `bash -c ` shape and the `agentOverride` flag. + let spec: AgentSpec | undefined; + let emitsStreamJson = false; + const built = buildAgentSpawnRequest({ + overrideEnvVar: BATCH_AGENT_CMD_ENV, + instructions, + cwd: projectRoot, + env, + buildAdapterRequest: () => { + // Resolve the spawn agent for the running transition's STAGE when one + // is given (propose/apply/verify/decompose/pr) — a stage-map routes + // each stage independently; a scalar/unset agent resolves the same for + // every stage. Either way `resolveAdapter` maps an unmapped-stage/unset + // name to `DEFAULT_AGENT` and rejects an unknown name + // (`UnknownAgentError`) before any spawn. + const resolved = stage + ? resolveAgentForStage(context.settings.agent, stage) + : scalarAgent(context.settings.agent); + // Parse the resolved spec ONCE per transition: the stored value is a + // whole `agent[:model]` spec string (config load already rejected + // malformed specs, so this parse cannot throw on validated config). The + // adapter is resolved by the AGENT PART — an unknown agent part still + // throws `UnknownAgentError` before any spawn, naming `rex` not + // `rex:some-model`; the model part is threaded to the adapter via + // `AgentRequestContext.model` so the adapter emits its own flag. A bare + // agent name (no `:`) parses to `{ agent }` with no `model` key, so the + // adapter emits no flag and the agent uses its harness-configured + // default model — byte-for-byte today's argv. + spec = resolved !== undefined ? parseAgentSpec(resolved) : undefined; + const adapter = resolveAdapter(spec?.agent, this.adapters); + emitsStreamJson = adapter.emitsStreamJson === true; + return adapter.buildRequest( + { ...context, model: spec?.model }, + instructions, + projectRoot, + env + ); + }, + }); return { - request: adapter.buildRequest( - { ...context, model: spec?.model }, - instructions, - projectRoot, - env - ), - emitsStreamJson: adapter.emitsStreamJson === true, - spec, + request: built.request, + emitsStreamJson: built.agentOverride ? false : emitsStreamJson, + spec: built.agentOverride ? undefined : spec, + agentOverride: built.agentOverride, }; } @@ -912,19 +960,22 @@ export class RatchetBatchEngine { } /** - * Under `after-propose` (and `every-phase`) gates, a completed propose parks - * for approval before apply. `voluntary` and `autonomous` never park for - * approval (autonomous still parks on agent blockers, handled in mapping). + * Under the configured `gate`, which completed transitions park for approval? + * Delegates the gate×transition decision to the pure + * {@link parksForApproval} matrix (the single source of truth) so the policy + * has one author: `voluntary`/`autonomous` park nothing; `after-propose` + * parks `propose` only; `every-phase` parks every completed change transition + * (propose/apply/verify). A resume that already carries an answer/feedback + * means the user acted on THIS transition, so it does not re-park (an + * approved/rejected re-run of the SAME transition advances). */ private shouldParkForApproval( context: ChangeStepContext, transition: Transition ): boolean { - if (transition !== 'propose') return false; // A resume that already carries an answer/feedback means the user acted; do // not re-park for approval. if (context.resume?.answer || context.resume?.feedback) return false; - const gate = context.settings.gate; - return gate === 'after-propose' || gate === 'every-phase'; + return parksForApproval(context.settings.gate, transition); } } diff --git a/src/core/batch/engine/index.ts b/src/core/batch/engine/index.ts index c6a1a62..f00ff96 100644 --- a/src/core/batch/engine/index.ts +++ b/src/core/batch/engine/index.ts @@ -34,11 +34,17 @@ export { realSpawner, UnknownAgentError, DEFAULT_AGENT, + activeAgentCmdOverride, + buildAgentSpawnRequest, + agentOverrideNotice, + ENV_OVERRIDE_PROVENANCE, + BATCH_AGENT_CMD_ENV, type AgentAdapter, type AgentRequestContext, type Spawner, type AgentSpawnRequest, type AgentSpawnResult, + type EnvOverrideProvenance, } from './agent.js'; export { buildAgentInstructions, @@ -57,6 +63,7 @@ export { type SkillLocusDeps, } from './skill-locus.js'; export { mapSessionToOutcome } from './outcome.js'; +export { parksForApproval } from './approval-gate.js'; export { computeNextTransition, readChangeDiskState, diff --git a/src/core/batch/engine/runtime/rex-remote-runtime.ts b/src/core/batch/engine/runtime/rex-remote-runtime.ts index e0ff189..3823b55 100644 --- a/src/core/batch/engine/runtime/rex-remote-runtime.ts +++ b/src/core/batch/engine/runtime/rex-remote-runtime.ts @@ -42,6 +42,7 @@ import type { AgentSpawnRequest, AgentSpawnResult } from '../agent.js'; import type { AgentEvent, AgentRuntime } from './contract.js'; +import { shquote, buildEnvExports } from './spawn-command.js'; /** A minimal `fetch` surface (the Node global `fetch` is assignable to this). */ export type FetchLike = ( @@ -111,19 +112,22 @@ const defaultDeps: RemoteDeps = { now: () => Date.now(), }; -/** Single-quote a string for safe embedding in a `sh -c` argument. */ -function shquote(s: string): string { - return "'" + s.replace(/'/g, "'\\''") + "'"; -} - /** * Build the agent shell command the server will run: `cat | `. * Identical in spirit to the sidecar's `buildRunCommand`, but the prompt path is * a SERVER path (the prompt is written to the server via /write_file). + * + * `AgentSpawnRequest.env` is exported before the pipeline via the shared + * {@link buildEnvExports} helper, overlaying the server bash session's base + * environment (request wins on collision; base vars absent from the request + * remain visible). The caller wraps this in the existing nohup/log/exit-sentinel + * launcher unchanged — the exports sit inside that launcher's `( … )` subshell, + * so they apply to the agent pipeline without touching the REST protocol. */ export function buildRemoteRunCommand(serverPromptPath: string, request: AgentSpawnRequest): string { const argv = [request.command, ...request.args].map(shquote).join(' '); - return `cat ${shquote(serverPromptPath)} | ${argv}`; + const exports = buildEnvExports(request.env); + return `${exports}cat ${shquote(serverPromptPath)} | ${argv}`; } /** Raised internally to short-circuit to the error-result path with a clean message. */ @@ -304,6 +308,10 @@ export function makeRexRemoteRuntime(options: RexRemoteRuntimeOptions): AgentRun const promptPath = `${runDir}/prompt.txt`; const logPath = `${runDir}/agent.log`; const exitPath = `${runDir}/exit.code`; + // The agent's process-group id (== pid under `set -m`) recorded to a pidfile + // so teardown can reap the whole tree (agent + `cat`) before the session is + // torn down. Lives in the per-run dir, swept with it on `rm -rf`. + const pidPath = `${runDir}/agent.pid`; let stdout = ''; let stderr = ''; @@ -330,12 +338,18 @@ export function makeRexRemoteRuntime(options: RexRemoteRuntimeOptions): AgentRun await call('/write_file', { path: promptPath, content: req.instructions ?? '' }); // 4. Non-blocking launch: background the agent to a logfile + an exit - // sentinel so /execute returns immediately while the agent keeps running. + // sentinel so /execute returns immediately while the agent keeps + // running. `set -m` (POSIX job control) makes the backgrounded pipeline + // its OWN process-group leader (pgid == `$!`), so teardown can reap the + // whole tree with one `kill -- -`; macOS ships no `setsid` binary, + // so `set -m` is used (works in bash/POSIX sh on macOS + Linux). The + // pid is recorded to a pidfile so teardown finds the group even after + // the launch subshell exits. const agentCmd = buildRemoteRunCommand(promptPath, req); const cwd = req.cwd ? `cd ${shquote(req.cwd)}; ` : ''; const launch = - `${cwd}( ${agentCmd} ) >${shquote(logPath)} 2>&1; ` + - `echo $? >${shquote(exitPath)}`; + `set -m; ${cwd}( ${agentCmd} ) >${shquote(logPath)} 2>&1 & ` + + `echo $! >${shquote(pidPath)}; wait $!; echo $? >${shquote(exitPath)}`; // Detach so the foreground /execute returns at once; nohup keeps it alive. await execShell(`nohup sh -c ${shquote(launch)} >/dev/null 2>&1 &`); @@ -389,7 +403,21 @@ export function makeRexRemoteRuntime(options: RexRemoteRuntimeOptions): AgentRun }; // Best-effort session/runtime teardown — failures must not mask the result. + // ORDERING MATTERS: reap the launched agent's process group BEFORE removing + // the run dir / closing the session — the pidfile lives in runDir and the + // session must still be alive for the kill to run. TERM then KILL the whole + // group (negative pid), best-effort; a missing pidfile or already-dead group + // is a no-op. This is the remote-path fix for the orphan: the prior code + // closed the session leaving the nohup'd agent alive on the server. const teardown = async (): Promise => { + try { + await execShell( + `test -f ${shquote(pidPath)} && { kill -TERM -- -$(cat ${shquote(pidPath)}) 2>/dev/null; ` + + `sleep 1; kill -KILL -- -$(cat ${shquote(pidPath)}) 2>/dev/null; } || true` + ); + } catch { + /* best-effort */ + } try { await execShell(`rm -rf ${shquote(runDir)}`); } catch { diff --git a/src/core/batch/engine/runtime/rex-sidecar-runtime.ts b/src/core/batch/engine/runtime/rex-sidecar-runtime.ts index 12cd3b4..eaee1d4 100644 --- a/src/core/batch/engine/runtime/rex-sidecar-runtime.ts +++ b/src/core/batch/engine/runtime/rex-sidecar-runtime.ts @@ -40,6 +40,7 @@ import { type BootstrapOptions, type ResolvedLaunch, } from './rex-bootstrap.js'; +import { shquote, buildEnvExports } from './spawn-command.js'; /** The minimal child-process surface the runtime drives (a `ChildProcess` subset). */ export interface SidecarChild { @@ -58,6 +59,8 @@ export interface SidecarChild { on(event: 'error', listener: (err: Error) => void): void; on(event: 'exit', listener: (code: number | null, signal: NodeJS.Signals | null) => void): void; kill(signal?: NodeJS.Signals): void; + /** The child's OS pid (for `killGroup`); `null` if unavailable. */ + pid?: number | null; } /** Injectable side-effect seams (spawn / fs / clock) for testability. */ @@ -72,6 +75,16 @@ export interface SidecarDeps { /** Schedule a callback after `ms` (defaults to setTimeout); returns a handle. */ setTimer(fn: () => void, ms: number): ReturnType; clearTimer(handle: ReturnType): void; + /** + * Signal the sidecar child's whole PROCESS GROUP (`kill -- -`). The child + * is spawned `detached: true` so it leads its own group (pgid == pid); one + * `killGroup(pid, sig)` reaps the sidecar AND any agent it launched that + * survived a protocol teardown. Guarded to POSIX (the default implementation + * falls back to a bare `child.kill(sig)` on Windows, where process groups are + * not a thing). Injected so unit tests observe the escalation without real + * processes. + */ + killGroup(pid: number, signal: NodeJS.Signals): void; } /** @@ -92,6 +105,17 @@ export interface RexSidecarRuntimeOptions { * bootstrap applies `DEFAULT_DOCKER_IMAGE`. */ image?: string; + /** + * Docker hardening knobs (docker locus only; ignored for `local`). Each maps + * 1:1 to a `REX_DOCKER_*` env var threaded into the bootstrap. See + * {@link BootstrapOptions} for semantics; defaults are resolved in the + * bootstrap (the single source of truth) when these are unset. + */ + dockerUser?: string; + dockerMemory?: string; + dockerPidsLimit?: number; + dockerCpus?: number; + network?: string; /** Overall guard against a hung child (ms). Default 10 minutes. */ timeoutMs?: number; /** Grace before escalating SIGTERM → SIGKILL on teardown (ms). Default 2s. */ @@ -105,9 +129,14 @@ const DEFAULT_KILL_GRACE_MS = 2000; const defaultDeps: SidecarDeps = { spawn(launch) { + // `detached: true` puts the sidecar in its OWN process group (pgid == pid) + // so teardown can reap the whole tree (sidecar + agent it launched) with one + // `process.kill(-pid, sig)`. On Windows process groups aren't a thing; + // `detached` there just creates a new console, which is harmless. return spawn(launch.command, launch.args, { env: launch.env, stdio: ['pipe', 'pipe', 'pipe'], + detached: true, }) as unknown as SidecarChild; }, bootstrap: (options) => bootstrapRexRuntime(options), @@ -120,13 +149,23 @@ const defaultDeps: SidecarDeps = { }, setTimer: (fn, ms) => setTimeout(fn, ms), clearTimer: (handle) => clearTimeout(handle), + killGroup(pid, signal) { + if (process.platform !== 'win32') { + try { + process.kill(-pid, signal); + return; + } catch { + /* group already gone — fall through to bare kill */ + } + } + try { + process.kill(pid, signal); + } catch { + /* already gone */ + } + }, }; -/** Single-quote a string for safe embedding in a bash `-c` argument. */ -function shquote(s: string): string { - return "'" + s.replace(/'/g, "'\\''") + "'"; -} - /** * Build the agent shell command the sidecar will run: `cat | `. * The prompt file feeds the instructions to the agent's stdin uniformly for any @@ -140,6 +179,12 @@ function shquote(s: string): string { * IN-CONTAINER path (the host `req.cwd` may not exist inside the container — see * the docker caller, which translates it onto the bind mount). When omitted the * agent inherits the ReX session cwd (REX_WORKDIR), preserving prior behaviour. + * + * `AgentSpawnRequest.env` is exported before the pipeline (after the cwd prefix) + * via the shared {@link buildEnvExports} helper, overlaying the ReX session's + * base environment: request values win on collision, base vars absent from the + * request remain visible. The run-op shape (`{op, id, command}`) is unchanged — + * env rides inside the command string, so no `sidecar.py` protocol change. */ export function buildRunCommand( promptFile: string, @@ -148,7 +193,8 @@ export function buildRunCommand( ): string { const argv = [request.command, ...request.args].map(shquote).join(' '); const prefix = cwd ? `cd ${shquote(cwd)}; ` : ''; - return `${prefix}cat ${shquote(promptFile)} | ${argv}`; + const exports = buildEnvExports(request.env); + return `${prefix}${exports}cat ${shquote(promptFile)} | ${argv}`; } /** @@ -196,6 +242,15 @@ export function makeRexSidecarRuntime(options: RexSidecarRuntimeOptions): AgentR : req.cwd : undefined; + // The run directory is where the sidecar writes its per-run sentinels + // (ratchet-rex-.log/.done) and the new pidfile. For `local` it is the + // host runDir verbatim; for `docker` the sidecar runs IN the container, so + // translate the host runDir onto the bind mount (the host path does not + // exist inside the container) — same swap as the prompt file. + const runDirForSidecar = isDocker + ? hostToContainerPath(runDir, options.projectRoot, DOCKER_MOUNT_CONTAINER) + : runDir; + // Resolve the launch descriptor (lazy/cached bootstrap). A missing Python or // (for docker) a missing daemon throws RexBootstrapError → surface as a // failed result (non-zero exit + message in stderr) so the engine maps it to @@ -212,6 +267,11 @@ export function makeRexSidecarRuntime(options: RexSidecarRuntimeOptions): AgentR image: options.image, mountHost: options.projectRoot, mountContainer: DOCKER_MOUNT_CONTAINER, + dockerUser: options.dockerUser, + dockerMemory: options.dockerMemory, + dockerPidsLimit: options.dockerPidsLimit, + dockerCpus: options.dockerCpus, + network: options.network, } : { locus, @@ -263,23 +323,35 @@ export function makeRexSidecarRuntime(options: RexSidecarRuntimeOptions): AgentR }; const teardownChild = () => { - // End stdin and ask the child to stop; escalate to SIGKILL after a grace - // window so no sidecar is orphaned. + // Teardown ordering: ask the sidecar to shut down FIRST (it reaps the + // agent group + stops the docker container + emits `closed`, which + // settles the run cleanly). Only if it does NOT exit within the grace + // window do we SIGKILL the sidecar's whole process group — this is the + // fix for the orphan: the prior code SIGTERM'd the sidecar immediately, + // killing it before it could reap the agent it launched. Sending + // `shutdown` is idempotent: the clean path already sent it and received + // `closed` (so the child is gone and the kill timer is a harmless + // no-op); the timeout/error path sends it here for the first time. + send({ op: 'shutdown' }); try { child.stdin?.end(); } catch { /* already closed */ } - try { - child.kill('SIGTERM'); - } catch { - /* already gone */ - } + const pid = child.pid; killHandle = deps.setTimer(() => { - try { - child.kill('SIGKILL'); - } catch { - /* already gone */ + if (pid !== undefined && pid !== null) { + try { + deps.killGroup(pid, 'SIGKILL'); + } catch { + /* already gone */ + } + } else { + try { + child.kill('SIGKILL'); + } catch { + /* already gone */ + } } }, killGraceMs); }; @@ -315,6 +387,7 @@ export function makeRexSidecarRuntime(options: RexSidecarRuntimeOptions): AgentR op: 'run', id: 1, command: buildRunCommand(promptFileInContainer, req, cwdForSidecar), + run_dir: runDirForSidecar, }); return; case 'stdout': { diff --git a/src/core/batch/engine/runtime/sidecar.py b/src/core/batch/engine/runtime/sidecar.py index 7ed7d93..7583396 100644 --- a/src/core/batch/engine/runtime/sidecar.py +++ b/src/core/batch/engine/runtime/sidecar.py @@ -7,7 +7,7 @@ talks to it one JSON object per line: Node -> sidecar (stdin): - {"op":"run","id":N,"command":""} launch + stream a command + {"op":"run","id":N,"command":"","run_dir":""?} launch + stream {"op":"shutdown"} stop the deployment, exit 0 sidecar -> Node (stdout): @@ -37,6 +37,15 @@ exist inside the container), so logfile writes land on the writable bind mount and journal writes propagate back to the host. + REX_DOCKER_USER= `docker run --user` (host uid:gid; when unset + the sidecar resolves the current host uid:gid + so container writes land as the host user). + REX_DOCKER_MEMORY=<2g> `docker run --memory` (fallback: "2g"). + REX_DOCKER_PIDS_LIMIT=<512> `docker run --pids-limit` (fallback: 512). + REX_DOCKER_CPUS=<1.5> `docker run --cpus` (opt-in; no flag when unset). + REX_DOCKER_NETWORK= `docker run --network` (fallback: "bridge" — + the container HAS outbound network; set "none" + to fully isolate). The repo bind mount is expressed via ``DockerDeploymentConfig.docker_args`` (``["-v", f"{host}:{container}"]``) because swe-rex (1.4.0) has NO dedicated @@ -93,6 +102,15 @@ def _exception_detail(exc: BaseException) -> object: # the two in sync if the default image ever changes. DEFAULT_DOCKER_IMAGE = "python:3.12" +# Cross-language fallbacks for the docker hardening knobs. These MUST match the +# TS constants in config.ts (the single source of truth). They are PURE +# UNSET-FALLBACKS: the Node side always threads the resolved value via the +# matching `REX_DOCKER_*` env var, so these are only reached if that env is +# missing. Keep the two languages in sync if any default ever changes. +DEFAULT_DOCKER_MEMORY = "2g" +DEFAULT_DOCKER_PIDS_LIMIT = "512" +DEFAULT_DOCKER_NETWORK = "bridge" + def _make_deployment(locus: str): """Construct a deployment for the requested locus. Docker is imported lazily @@ -102,6 +120,11 @@ def _make_deployment(locus: str): ``python:3.12``) and the project root is bind-mounted via ``docker_args`` (``-v REX_MOUNT_HOST:REX_MOUNT_CONTAINER``) — swe-rex 1.4.0 has no dedicated ``volumes`` field; ``start()`` splices ``docker_args`` into the run argv. + + Docker hardening knobs (``REX_DOCKER_*``) are appended to ``docker_args`` + conditionally on env presence, so an unset knob never emits a flag (Docker's + own default then applies). The ``--user`` knob defaults to the current host + uid:gid so container writes land as the host user rather than root. """ if locus == "docker": from swerex.deployment.docker import DockerDeployment @@ -113,7 +136,53 @@ def _make_deployment(locus: str): ) docker_args: list = [] if mount_host: - docker_args = ["-v", f"{mount_host}:{mount_container}"] + docker_args += ["-v", f"{mount_host}:{mount_container}"] + + # `--user`: default to the current host uid:gid so container file writes + # land as the host user (not root). An explicit `REX_DOCKER_USER` wins. + docker_user = os.environ.get("REX_DOCKER_USER", "").strip() + if not docker_user: + docker_user = f"{os.getuid()}:{os.getgid()}" + docker_args += ["--user", docker_user] + + # `--memory`: always applied (sane default bounds the container). + docker_memory = os.environ.get("REX_DOCKER_MEMORY", "").strip() or DEFAULT_DOCKER_MEMORY + docker_args += ["--memory", docker_memory] + + # `--pids-limit`: always applied (bounds fork-bomb-style runaway). + docker_pids = os.environ.get("REX_DOCKER_PIDS_LIMIT", "").strip() or DEFAULT_DOCKER_PIDS_LIMIT + # Fail before spawn: a non-integer or non-positive pids limit would + # otherwise surface as a cryptic docker error. Node validates upstream, + # but the sidecar is the last gate before `docker run`. + try: + if int(docker_pids) <= 0: + raise ValueError + except ValueError: + raise RuntimeError( + f"REX_DOCKER_PIDS_LIMIT must be a positive integer (got '{docker_pids}')." + ) + docker_args += ["--pids-limit", docker_pids] + + # `--network`: always applied. `bridge` (default) means the container + # HAS outbound network; `none` fully isolates. The honest isolation + # contract documents this explicitly. + docker_network = os.environ.get("REX_DOCKER_NETWORK", "").strip() or DEFAULT_DOCKER_NETWORK + docker_args += ["--network", docker_network] + + # `--cpus`: opt-in (no default → no flag when unset, Docker's default). + docker_cpus = os.environ.get("REX_DOCKER_CPUS", "").strip() + if docker_cpus: + # Fail before spawn: a non-numeric or non-positive cpus value would + # otherwise surface as a cryptic docker error. + try: + if float(docker_cpus) <= 0: + raise ValueError + except ValueError: + raise RuntimeError( + f"REX_DOCKER_CPUS must be a positive number (got '{docker_cpus}')." + ) + docker_args += ["--cpus", docker_cpus] + return DockerDeployment(image=image, docker_args=docker_args) # Default / "local". from swerex.deployment.local import LocalDeployment @@ -128,6 +197,13 @@ def __init__(self) -> None: self.session = "ratchet-rex" self.deployment = None self.runtime = None + # pidfile of the in-flight run's process group (pgid == pid under + # `set -m`), so `shutdown` can reap the whole tree even when the Node + # parent SIGKILLs us instead of speaking the protocol. None when no run + # is in flight (or the run already reaped itself). + self.run_pidfile: str | None = None + # Grace (s) between SIGTERM and SIGKILL when reaping the agent group. + self._shutdown_grace_s = 1.0 async def start(self) -> None: from swerex.runtime.abstract import CreateBashSessionRequest @@ -166,21 +242,39 @@ async def _exec(self, command: str): Command(command=command, shell=True, check=False) ) - async def run(self, run_id, command: str) -> None: + async def run(self, run_id, command, run_dir: str | None = None) -> None: """Launch ``command`` detached to a logfile and stream its stdout lines, - then report the exit code exactly once.""" + then report the exit code exactly once. + + ``run_dir`` (optional, from the run op) is the directory the sidecar + writes its per-run sentinels (``ratchet-rex-.log/.done``) and the + new pidfile into — typically ``.ratchet/batches//.run//`` on + the host, or its in-container translation for docker. Absent → fall back + to the workdir (the prior behaviour, so the op protocol is a pure + extension).""" + sentinel_dir = run_dir if run_dir else self.workdir token = uuid.uuid4().hex - log = f"{self.workdir.rstrip('/')}/ratchet-rex-{token}.log" - done = f"{self.workdir.rstrip('/')}/ratchet-rex-{token}.done" - - # Clear any prior sentinels, then launch detached, recording the exit - # code to a sentinel file when the command finishes. - await self._exec(f"rm -f {log} {done}") - launcher = ( - f"nohup bash -c {_shquote(command)} > {log} 2>&1; " - f"echo $? > {done}" + log = f"{sentinel_dir.rstrip('/')}/ratchet-rex-{token}.log" + done = f"{sentinel_dir.rstrip('/')}/ratchet-rex-{token}.done" + pid = f"{sentinel_dir.rstrip('/')}/ratchet-rex-{token}.pid" + + # Clear any prior sentinels, then launch detached under job control so the + # backgrounded pipeline becomes its OWN process-group leader (pgid == + # pid). One `kill -- -` then reaps the whole tree (agent + `cat`), + # which a lone-pid kill would strand. The inner `bash -c ` runs the + # agent pipeline; `$!` is its pid (== pgid under `set -m`), recorded to + # the pidfile so `shutdown` can find the group; `wait` collects the exit + # code into the done sentinel. macOS ships no `setsid` binary, so `set + # -m` (POSIX job control) is used — works in bash and POSIX sh on both + # macOS (local) and Linux (docker/remote). + await self._exec(f"rm -f {log} {done} {pid}") + self.run_pidfile = pid + inner = ( + f"set -m; bash -c {_shquote(command)} > {log} 2>&1 & " + f"echo $! > {pid}; wait $!; echo $? > {done}" ) - await self._exec(f"nohup bash -c {_shquote(launcher)} >/dev/null 2>&1 &") + launcher = f"nohup bash -c {_shquote(inner)} >/dev/null 2>&1 &" + await self._exec(launcher) offset = 0 while True: @@ -231,12 +325,49 @@ async def run(self, run_id, command: str) -> None: except (ValueError, IndexError): exit_code = -1 emit({"event": "exit", "id": run_id, "exit_code": exit_code}) - await self._exec(f"rm -f {log} {done}") + await self._exec(f"rm -f {log} {done} {pid}") + if self.run_pidfile == pid: + self.run_pidfile = None return await asyncio.sleep(POLL_INTERVAL) + async def _reap_agent_group(self) -> None: + """Reap the in-flight run's process group: TERM → short grace → KILL, + then remove its log/done/pid sentinels. Best-effort — a missing pidfile + or an already-dead group is a no-op. Idempotent (safe to call from both + the `shutdown` op and the SIGTERM handler).""" + pidfile = self.run_pidfile + if not pidfile: + return + self.run_pidfile = None # claim it; idempotent across concurrent calls + try: + res = await self._exec(f"cat {pidfile} 2>/dev/null") + pgid = (res.stdout or "").strip() + if not pgid or not pgid.lstrip("-").isdigit(): + return + # TERM the whole group (negative pid = pgid), short grace, then KILL. + await self._exec(f"kill -TERM -- -{pgid} 2>/dev/null || true") + await asyncio.sleep(self._shutdown_grace_s) + await self._exec(f"kill -KILL -- -{pgid} 2>/dev/null || true") + except Exception: # noqa: BLE001 — reaping must never raise + pass + finally: + # Sweep the sentinels/pidfile so nothing litters the run dir. The log + # basename pattern is shared (same token) but we don't track the + # exact paths here — `rm -f` the pidfile and any ratchet-rex-* in the + # same dir is over-broad, so derive the log/done from the pidfile's + # token by best-effort globbing of the recorded names. + try: + await self._exec(f"rm -f {pidfile} {pidfile[:-4]}.log {pidfile[:-4]}.done 2>/dev/null || true") + except Exception: # noqa: BLE001 + pass + async def shutdown(self) -> None: + # Reap the agent process group BEFORE stopping the deployment so the + # agent (and its `cat` sibling) can't outlive the sidecar — the prior + # behaviour orphaned them on teardown. + await self._reap_agent_group() if self.deployment is not None: try: await self.deployment.stop() @@ -248,6 +379,21 @@ async def shutdown(self) -> None: async def serve(self) -> int: await self.start() loop = asyncio.get_event_loop() + # Register a SIGTERM handler that funnels into the same teardown as the + # `shutdown` op and stdin-EOF: the Node parent SIGKILLing us (instead of + # speaking the protocol) still reaps the agent group and stops the + # docker container before we die. `loop.add_signal_handler` is POSIX- + # only; on the unsupported platform (Windows) we skip — the op/EOF paths + # still tear down cleanly there. After teardown the process exits 0 + # (SystemExit from the coroutine propagates through asyncio.run). + async def _on_sigterm(): + await self.shutdown() + raise SystemExit(0) + + try: + loop.add_signal_handler(asyncio.SIGTERM, lambda: asyncio.ensure_future(_on_sigterm())) + except (NotImplementedError, RuntimeError): + pass while True: line = await loop.run_in_executor(None, sys.stdin.readline) if line == "": @@ -277,8 +423,11 @@ async def serve(self) -> int: if op == "run": run_id = msg.get("id") command = msg.get("command", "") + run_dir = msg.get("run_dir") + if not isinstance(run_dir, str) or not run_dir.strip(): + run_dir = None try: - await self.run(run_id, command) + await self.run(run_id, command, run_dir) except Exception as exc: # noqa: BLE001 — surface, don't crash emit( { diff --git a/src/core/batch/engine/runtime/spawn-command.ts b/src/core/batch/engine/runtime/spawn-command.ts new file mode 100644 index 0000000..76553f3 --- /dev/null +++ b/src/core/batch/engine/runtime/spawn-command.ts @@ -0,0 +1,52 @@ +/** + * Shared spawn-command helpers for the rex runtimes. + * + * Both `RexSidecarRuntime` and `RexRemoteRuntime` launch the agent as a shell + * command built from an `AgentSpawnRequest`. The two constructions share two + * primitives — single-quoting a token for safe shell embedding, and serializing + * `AgentSpawnRequest.env` into `export` statements that overlay the runtime + * session's base environment — so they live here once and the runtimes cannot + * drift apart (seed of the phase-level "spawn-request construction lives in one + * shared helper" criterion; pre-dedups part of #91). + * + * Env merge semantics are OVERLAY, not replace: the exports run on top of the + * session's base environment, so a request value wins on collision and base + * variables absent from the request remain visible to the agent. (The legacy + * in-process `realSpawner` replaces the child env wholesale; a shell-session + * runtime cannot sanely replace — `env -i` would strip the session `PATH` the + * agent needs for command resolution on docker/remote loci.) Narrowing WHAT the + * engine puts in `AgentSpawnRequest.env` (host-env leakage to docker/remote) is + * out of scope here — that is issue #86 (allowlist), which this unblocks. + * + * Implements `features/rex-env-threading/env-serialization-safety.feature`. + */ + +/** Single-quote a string for safe embedding in a `sh -c` / bash `-c` argument. */ +export function shquote(s: string): string { + return "'" + s.replace(/'/g, "'\\''") + "'"; +} + +const SHELL_IDENT = /^[A-Za-z_][A-Za-z0-9_]*$/; + +/** + * Serialize `env` into a sequence of shell `export` statements (each trailing + * `"; "`) that overlay the runtime session's base environment when prefixed to + * the agent launch command. + * + * - Values are single-quoted via {@link shquote} so metacharacters ($, spaces, + * quotes, newlines) survive byte-for-byte and are not interpreted by the shell. + * - Entries whose name is not a valid shell identifier are skipped: such names + * are unreachable in shell and would break `export`. + * - Entries with `undefined` values are skipped. + * - An empty/absent env yields an empty prefix. + */ +export function buildEnvExports(env: NodeJS.ProcessEnv | undefined): string { + if (!env) return ''; + let out = ''; + for (const [name, value] of Object.entries(env)) { + if (value === undefined) continue; + if (!SHELL_IDENT.test(name)) continue; + out += `export ${name}=${shquote(String(value))}; `; + } + return out; +} diff --git a/src/core/batch/journal.ts b/src/core/batch/journal.ts index 84252cb..d94dcac 100644 --- a/src/core/batch/journal.ts +++ b/src/core/batch/journal.ts @@ -16,6 +16,7 @@ import path from 'path'; import { getBatchDir } from './manifest.js'; import { RATCHET_DIR_NAME } from '../config.js'; import type { ProofOfWorkPolicy } from './config.js'; +import type { PassConditionKind } from './manifest.js'; export type JournalEntryKind = | 'progress' @@ -50,6 +51,20 @@ export interface ProofOfWorkRecord { reason: string; /** Human-readable explanation of the verdict. */ detail: string; + /** + * Which pass-condition kind was evaluated (`exit-zero` | `contains` | `regex` + * | `substring`). Absent for the not-yet-wired `llm-judge` kind and on older + * records written before this field existed — readers ignore absence, no + * migration. + */ + conditionKind?: PassConditionKind; + /** + * The matched excerpt on a pass: the needle for `contains`/`substring`, the + * actual matched text for `regex`. Absent on a fail, for exit-zero, and on + * older records — readers ignore absence. Persisted so gate evidence is + * reviewable instead of a bare pass/fail bit. + */ + matchedExcerpt?: string; } export interface JournalEntry { @@ -62,6 +77,15 @@ export interface JournalEntry { transition?: string; /** Present only on `proof-of-work` entries: the recorded verdict. */ proof?: ProofOfWorkRecord; + /** + * Provenance marker stamped on entries produced under an active agent-cmd + * override (`RATCHET_BATCH_AGENT_CMD`). The engine stamps the + * transition-outcome entry it appends; `batch report` stamps the entries it + * appends from its own process env (the spawned stand-in inherits the var). + * Absent on override-free work — readers ignore it, so no migration. Today + * the only value is `'env-override'`; the union is open to widen later. + */ + via?: 'env-override'; } export type ParkedKind = 'blocked' | 'awaiting-approval'; diff --git a/src/core/eval/execute.ts b/src/core/eval/execute.ts index a0f6006..0bbade6 100644 --- a/src/core/eval/execute.ts +++ b/src/core/eval/execute.ts @@ -31,6 +31,18 @@ import { type EvalRun, type CaseRecord, } from './run.js'; +import { + activeAgentCmdOverride, + ENV_OVERRIDE_PROVENANCE, +} from '../batch/engine/agent.js'; + +/** + * The env var that overrides the eval agent spawn. Declared here so the run + * record stamp is self-documenting; the override GATE itself lives in the + * shared `buildAgentSpawnRequest` helper (used by the judge and mutation + * harness). The run-level stamp keys on the var being ACTIVE for the run. + */ +const EVAL_AGENT_CMD_ENV = 'RATCHET_EVAL_AGENT_CMD'; export interface RunOptions { scope: EvalScope; @@ -135,6 +147,12 @@ export async function executeRun(projectRoot: string, options: RunOptions): Prom createdAt: (options.now ?? new Date()).toISOString(), scope: { kind: options.scope.kind, target: options.scope.target }, gate: ALL_CONTRIBUTOR_IDS.filter((id) => options.gate.has(id)), + // Stamp `via: env-override` when the eval agent-cmd override is active for + // the run — a run executed with the seam armed is synthetic evidence + // regardless of which contributors fired. Absent when inactive. + ...(activeAgentCmdOverride(EVAL_AGENT_CMD_ENV, process.env) !== undefined + ? { via: ENV_OVERRIDE_PROVENANCE } + : {}), cases: [], verdicts: {}, }; diff --git a/src/core/eval/judge.ts b/src/core/eval/judge.ts index a55e406..0e6e2ce 100644 --- a/src/core/eval/judge.ts +++ b/src/core/eval/judge.ts @@ -33,6 +33,7 @@ import { realBashRunner, realSpawner, resolveAdapter, + buildAgentSpawnRequest, type BashRunner, type Spawner, type AgentRequestContext, @@ -264,19 +265,33 @@ function judgeContext(c: EvalCase): AgentRequestContext { } /** - * Build the spawn request for one judge vote. When `RATCHET_EVAL_AGENT_CMD` is - * set, that command stands in for the coding-agent binary (used by e2e tests to - * exercise the agent path deterministically without a real agent). Otherwise the - * configured adapter is resolved as usual. + * The env var that overrides the eval judge's coding-agent spawn. Declared + * locally so the judge's override seam is self-documenting; the override GATE + * itself lives in the shared `buildAgentSpawnRequest` helper so the engine, the + * judge, and the mutation harness share one override seam (the #67 + * triplication). + */ +const EVAL_AGENT_CMD_ENV = 'RATCHET_EVAL_AGENT_CMD'; + +/** + * Build the spawn request for one judge vote through the shared override-aware + * helper. When `RATCHET_EVAL_AGENT_CMD` is active, that command stands in for + * the coding-agent binary (used by e2e tests to exercise the agent path + * deterministically without a real agent). Otherwise the configured adapter is + * resolved as usual. The override gate exists in exactly one place + * (`buildAgentSpawnRequest`); the closure here owns only the judge's + * site-specific adapter resolution. */ function buildVoteRequest(c: EvalCase, binding: LlmJudgeBinding, cwd: string, agentName?: string) { const instructions = buildJudgeInstructions(c, binding); - const override = process.env.RATCHET_EVAL_AGENT_CMD; - if (override && override.trim().length > 0) { - return { command: 'bash', args: ['-c', override], instructions, cwd, env: process.env }; - } - const adapter = resolveAdapter(agentName); - return adapter.buildRequest(judgeContext(c), instructions, cwd, process.env); + const { request } = buildAgentSpawnRequest({ + overrideEnvVar: EVAL_AGENT_CMD_ENV, + instructions, + cwd, + env: process.env, + buildAdapterRequest: () => resolveAdapter(agentName).buildRequest(judgeContext(c), instructions, cwd, process.env), + }); + return request; } async function castVote( diff --git a/src/core/eval/mutation-harness.ts b/src/core/eval/mutation-harness.ts index aba5389..f334589 100644 --- a/src/core/eval/mutation-harness.ts +++ b/src/core/eval/mutation-harness.ts @@ -48,6 +48,7 @@ import { realBashRunner, realSpawner, resolveAdapter, + buildAgentSpawnRequest, type BashRunner, type BashResult, type Spawner, @@ -143,20 +144,32 @@ function seedContext(invariant: MutationInvariant): AgentRequestContext { } /** - * Build the spawn request for one seed attempt. When `RATCHET_EVAL_AGENT_CMD` - * is set, that command stands in for the coding-agent binary (used by e2e - * tests to exercise the agent path deterministically without a real agent). - * Otherwise the configured adapter is resolved as usual — mirrors `judge.ts`'s - * `buildVoteRequest` exactly, so there is no agent-specific branch here. + * The env var that overrides the mutation seeder's coding-agent spawn. Declared + * locally so the seeder's override seam is self-documenting; the override GATE + * itself lives in the shared `buildAgentSpawnRequest` helper so the engine, the + * judge, and the mutation harness share one override seam (the #67 + * triplication), mirroring `judge.ts`'s `buildVoteRequest` exactly. + */ +const EVAL_AGENT_CMD_ENV = 'RATCHET_EVAL_AGENT_CMD'; + +/** + * Build the spawn request for one seed attempt through the shared override-aware + * helper. When `RATCHET_EVAL_AGENT_CMD` is active, that command stands in for + * the coding-agent binary (deterministic e2e testing); otherwise the configured + * adapter is resolved as usual. The override gate exists in exactly one place + * (`buildAgentSpawnRequest`); the closure here owns only the seeder's + * site-specific adapter resolution. */ function buildSeedRequest(invariant: MutationInvariant, cwd: string, agentName?: string): AgentSpawnRequest { const instructions = buildSeedInstructions(invariant); - const override = process.env.RATCHET_EVAL_AGENT_CMD; - if (override && override.trim().length > 0) { - return { command: 'bash', args: ['-c', override], instructions, cwd, env: process.env }; - } - const adapter = resolveAdapter(agentName); - return adapter.buildRequest(seedContext(invariant), instructions, cwd, process.env); + const { request } = buildAgentSpawnRequest({ + overrideEnvVar: EVAL_AGENT_CMD_ENV, + instructions, + cwd, + env: process.env, + buildAdapterRequest: () => resolveAdapter(agentName).buildRequest(seedContext(invariant), instructions, cwd, process.env), + }); + return request; } /** diff --git a/src/core/eval/run.ts b/src/core/eval/run.ts index 09f83e2..29955f2 100644 --- a/src/core/eval/run.ts +++ b/src/core/eval/run.ts @@ -82,6 +82,14 @@ export interface EvalRun { * "not evaluated". */ invariantGate?: InvariantGateResult; + /** + * Provenance marker stamped on the run record when `RATCHET_EVAL_AGENT_CMD` + * was active for the run. The eval stamp keys on the var being ACTIVE for + * the run (deterministic, documented), not on whether a given case happened + * to spawn — a run executed with the seam armed is synthetic evidence + * regardless of which contributors fired. Absent on override-free runs. + */ + via?: 'env-override'; cases: CaseSnapshot[]; verdicts: Record; } diff --git a/test/batch-engine/agent.test.ts b/test/batch-engine/agent.test.ts index f2bc8c8..100dfb2 100644 --- a/test/batch-engine/agent.test.ts +++ b/test/batch-engine/agent.test.ts @@ -1,5 +1,15 @@ import { describe, it, expect } from 'vitest'; -import { resolveAdapter, type AgentRequestContext } from '../../src/core/batch/engine/agent.js'; +import { + resolveAdapter, + activeAgentCmdOverride, + buildAgentSpawnRequest, + agentOverrideNotice, + ENV_OVERRIDE_PROVENANCE, + makeRealSpawner, + realSpawner, + type AgentRequestContext, + type AgentSpawnRequest, +} from '../../src/core/batch/engine/agent.js'; /** * Adapter capability + argv assertions for `capability-gating.feature` scenarios @@ -135,3 +145,151 @@ describe('CommandAgentAdapter — model flag argv (model-flag-argv.feature)', () expect(req.args.slice(base.length + 2)).toContain('--permission-mode'); }); }); + +/** + * Implements: features/agent-cmd-override/shared-spawn-helper.feature + * + * The shared override-aware spawn-request helper: `activeAgentCmdOverride` is the + * single gate (active / whitespace-only / unset), `buildAgentSpawnRequest` + * returns the `bash -c ` request with `agentOverride: true` when + * active (carrying instructions/cwd/env) or delegates to the adapter closure + * (invoked exactly once) with `agentOverride: false` when inactive, and + * `agentOverrideNotice` renders the per-var notice line. + */ +describe('buildAgentSpawnRequest — override gate (shared-spawn-helper.feature)', () => { + const ENV = 'RATCHET_BATCH_AGENT_CMD'; + const INSTRUCTIONS = 'do the thing'; + const CWD = '/proj'; + const ENV_OBJ: NodeJS.ProcessEnv = { [ENV]: 'echo stub-agent', OTHER: 'x' }; + + /** A buildAdapterRequest closure that records its invocations. */ + function adapterClosure(calls: number[]) { + return (): AgentSpawnRequest => { + calls[0] += 1; + return { command: 'fake-agent', args: ['-p'], instructions: INSTRUCTIONS, cwd: CWD, env: ENV_OBJ }; + }; + } + + // Scenario: an active override produces the `bash -c ` request and + // reports the agent was overridden. + it('an active override yields a `bash -c ` request with agentOverride=true', () => { + const calls = [0]; + const { request, agentOverride } = buildAgentSpawnRequest({ + overrideEnvVar: ENV, + instructions: INSTRUCTIONS, + cwd: CWD, + env: { [ENV]: 'echo stub-agent' }, + buildAdapterRequest: adapterClosure(calls), + }); + expect(agentOverride).toBe(true); + expect(request).toEqual({ + command: 'bash', + args: ['-c', 'echo stub-agent'], + instructions: INSTRUCTIONS, + cwd: CWD, + env: { [ENV]: 'echo stub-agent' }, + }); + // The adapter closure is NOT consulted when the override is active. + expect(calls[0]).toBe(0); + }); + + // Scenario: a whitespace-only override is inactive (configured adapter used). + it('a whitespace-only override is inactive and the adapter closure is used', () => { + const calls = [0]; + const { request, agentOverride } = buildAgentSpawnRequest({ + overrideEnvVar: ENV, + instructions: INSTRUCTIONS, + cwd: CWD, + env: { [ENV]: ' ' }, + buildAdapterRequest: adapterClosure(calls), + }); + expect(agentOverride).toBe(false); + expect(request.command).toBe('fake-agent'); + // The adapter closure is invoked EXACTLY ONCE. + expect(calls[0]).toBe(1); + }); + + // Scenario: an unset override is inactive (configured adapter used). + it('an unset override is inactive and the adapter closure is used exactly once', () => { + const calls = [0]; + const { request, agentOverride } = buildAgentSpawnRequest({ + overrideEnvVar: ENV, + instructions: INSTRUCTIONS, + cwd: CWD, + env: {}, + buildAdapterRequest: adapterClosure(calls), + }); + expect(agentOverride).toBe(false); + expect(request.command).toBe('fake-agent'); + expect(calls[0]).toBe(1); + }); + + // Scenario: the override request threads instructions/cwd/env like any request. + it('the override request carries instructions, cwd, and env verbatim', () => { + const { request } = buildAgentSpawnRequest({ + overrideEnvVar: ENV, + instructions: 'PROMPT', + cwd: '/the/cwd', + env: { [ENV]: 'cmd', RATCHET_STEP_VAR: 'from-engine' }, + buildAdapterRequest: adapterClosure([0]), + }); + expect(request.instructions).toBe('PROMPT'); + expect(request.cwd).toBe('/the/cwd'); + expect(request.env).toEqual({ [ENV]: 'cmd', RATCHET_STEP_VAR: 'from-engine' }); + }); + + it('activeAgentCmdOverride trims, treats whitespace-only as inactive, and undefined as inactive', () => { + expect(activeAgentCmdOverride(ENV, { [ENV]: ' echo x ' })).toBe('echo x'); + expect(activeAgentCmdOverride(ENV, { [ENV]: ' ' })).toBeUndefined(); + expect(activeAgentCmdOverride(ENV, { [ENV]: '' })).toBeUndefined(); + expect(activeAgentCmdOverride(ENV, {})).toBeUndefined(); + }); + + it('agentOverrideNotice names the env var and ENV_OVERRIDE_PROVENANCE is env-override', () => { + expect(agentOverrideNotice(ENV)).toBe('⚠ agent overridden by RATCHET_BATCH_AGENT_CMD'); + expect(agentOverrideNotice('RATCHET_EVAL_AGENT_CMD')).toBe( + '⚠ agent overridden by RATCHET_EVAL_AGENT_CMD' + ); + expect(ENV_OVERRIDE_PROVENANCE).toBe('env-override'); + }); +}); + +/** + * makeRealSpawner — the real process-spawn seam: detached process group on + * POSIX, overall timeout with TERM→grace→KILL on the whole group, and a + * resolved (non-hanging) result carrying stdout/stderr/exit. + * + * These run REAL subprocesses (sh -c …) — parity with the env-threading tests + * in rex-sidecar-runtime.test.ts — to prove the spawned command observes the + * timeout/reap behavior, not just the string shape. + */ +describe('makeRealSpawner — detached spawn + timeout reap', () => { + function req(command: string): AgentSpawnRequest { + return { + command: 'sh', + args: ['-c', command], + instructions: '', + cwd: process.cwd(), + env: { ...process.env, PATH: process.env.PATH ?? '' }, + }; + } + + it('runs a fast command and resolves with its stdout + exit code', async () => { + const spawner = makeRealSpawner({ timeoutMs: 5000 }); + const result = await spawner(req("echo 'hello world'; exit 3")); + expect(result.exitCode).toBe(3); + expect(result.stdout.trim()).toBe('hello world'); + }); + + it('times out a hung command: resolves with non-zero + a timeout message in stderr', async () => { + const spawner = makeRealSpawner({ timeoutMs: 50, killGraceMs: 20 }); + const result = await spawner(req('sleep 30')); + expect(result.exitCode).not.toBe(0); + expect(result.stderr).toMatch(/timed out/i); + }); + + it('realSpawner is a Spawner built from makeRealSpawner', () => { + expect(typeof realSpawner).toBe('function'); + expect(typeof makeRealSpawner).toBe('function'); + }); +}); diff --git a/test/batch-engine/engine-agent-override.test.ts b/test/batch-engine/engine-agent-override.test.ts index 3a4feef..a155e01 100644 --- a/test/batch-engine/engine-agent-override.test.ts +++ b/test/batch-engine/engine-agent-override.test.ts @@ -1,8 +1,9 @@ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { promises as fs } from 'fs'; +import * as fsSync from 'fs'; import path from 'path'; import os from 'os'; -import { appendJournal } from 'ratchet-ai'; +import { appendJournal, readJournalForChange } from 'ratchet-ai'; import type { ResolvedStepContext, BatchSettings, ProofOfWork } from 'ratchet-ai'; import { RatchetBatchEngine } from '../../src/core/batch/engine/engine.js'; import type { AgentAdapter, Spawner, AgentSpawnRequest } from '../../src/core/batch/engine/agent.js'; @@ -13,6 +14,12 @@ import type { AgentAdapter, Spawner, AgentSpawnRequest } from '../../src/core/ba * configured adapter. Unset → behavior is identical to today. The `Spawner` * (unit-test injection seam) stays untouched either way; here we use it to * capture the request the engine built. + * + * Also implements features/agent-cmd-override/override-notice.feature and + * features/agent-cmd-override/override-provenance.feature (engine side): a + * step spawned under an active override carries `agentOverride: true` on the + * `StepResult` and stamps `via: 'env-override'` on the transition-outcome + * journal entry the engine appends. */ let projectRoot: string; @@ -94,17 +101,27 @@ function engineWith(behavior: Parameters[0]) { return { engine, fake }; } +/** Corroborate a propose completion by writing the change dir + plan.md. */ +function corroboratePropose(root: string, change: string): void { + const dir = path.join(root, '.ratchet', 'changes', change); + fsSync.mkdirSync(dir, { recursive: true }); + fsSync.writeFileSync(path.join(dir, 'plan.md'), '## Tasks\n- [ ] do it\n'); +} + describe('RatchetBatchEngine.runStep — RATCHET_BATCH_AGENT_CMD override', () => { it('runs the override via `bash -c` with instructions on stdin, skipping the adapter', async () => { process.env[ENV] = 'echo stub-agent'; const { engine, fake } = engineWith({ - report: (root, batch, change) => - appendJournal(root, batch, { change, kind: 'completion', message: 'proposed', transition: 'propose' }), + report: (root, batch, change) => { + corroboratePropose(root, change); + appendJournal(root, batch, { change, kind: 'completion', message: 'proposed', transition: 'propose' }); + }, }); const result = await engine.runStep(context()); expect(result.state).toBe('advanced'); + expect(result.agentOverride).toBe(true); // override notice flag rides the result expect(fake.state.adapterCalls).toBe(0); // adapter was NOT resolved/used expect(fake.calls.length).toBe(1); const req = fake.calls[0]; @@ -117,8 +134,10 @@ describe('RatchetBatchEngine.runStep — RATCHET_BATCH_AGENT_CMD override', () = it('treats a blank/whitespace override as unset (configured adapter is used)', async () => { process.env[ENV] = ' '; const { engine, fake } = engineWith({ - report: (root, batch, change) => - appendJournal(root, batch, { change, kind: 'completion', message: 'proposed', transition: 'propose' }), + report: (root, batch, change) => { + corroboratePropose(root, change); + appendJournal(root, batch, { change, kind: 'completion', message: 'proposed', transition: 'propose' }); + }, }); const result = await engine.runStep(context()); @@ -130,13 +149,16 @@ describe('RatchetBatchEngine.runStep — RATCHET_BATCH_AGENT_CMD override', () = it('uses the configured adapter when the override is unset', async () => { const { engine, fake } = engineWith({ - report: (root, batch, change) => - appendJournal(root, batch, { change, kind: 'completion', message: 'proposed', transition: 'propose' }), + report: (root, batch, change) => { + corroboratePropose(root, change); + appendJournal(root, batch, { change, kind: 'completion', message: 'proposed', transition: 'propose' }); + }, }); const result = await engine.runStep(context()); expect(result.state).toBe('advanced'); + expect(result.agentOverride).toBeUndefined(); // no override → no flag expect(fake.state.adapterCalls).toBe(1); expect(fake.calls[0].command).toBe('fake-agent'); }); @@ -149,6 +171,7 @@ describe('RatchetBatchEngine.runStep — RATCHET_BATCH_AGENT_CMD override', () = expect(result.state).toBe('blocked'); // failed surfaces as a resumable blocked step expect(result.blocker).toMatch(/exited|completion/i); + expect(result.agentOverride).toBe(true); // override was active even on failure expect(fake.calls[0].command).toBe('bash'); // The batch run-state stays consistent: a later retry can run again. @@ -156,4 +179,46 @@ describe('RatchetBatchEngine.runStep — RATCHET_BATCH_AGENT_CMD override', () = expect(retry.state).toBe('blocked'); expect(fake.calls.length).toBe(2); }); + + // features/agent-cmd-override/override-provenance.feature — Scenario: engine + // transition-outcome journal entry is stamped. + it('stamps `via: env-override` on the transition-outcome journal entry under an override', async () => { + process.env[ENV] = 'echo stub-agent'; + const { engine } = engineWith({ + report: (root, batch, change) => { + corroboratePropose(root, change); + appendJournal(root, batch, { change, kind: 'completion', message: 'proposed', transition: 'propose' }); + }, + }); + + await engine.runStep(context()); + + // The engine appends a transition-outcome entry after the agent session. + // The agent's own report (via the fake spawner's callback) appended a + // completion entry WITHOUT `via` first; the engine's outcome entry is the + // stamped one, so assert the stamped entry exists rather than the first + // completion. + const entries = readJournalForChange(projectRoot, 'b', 'add-login-api'); + const stamped = entries.find( + (e) => e.transition === 'propose' && e.kind === 'completion' && e.via === 'env-override' + ); + expect(stamped).toBeDefined(); + }); + + // Override-free work stays unstamped (override-provenance.feature — Scenario: + // work produced without an override stays unstamped). + it('does not stamp `via` on the outcome entry when no override is active', async () => { + const { engine } = engineWith({ + report: (root, batch, change) => { + corroboratePropose(root, change); + appendJournal(root, batch, { change, kind: 'completion', message: 'proposed', transition: 'propose' }); + }, + }); + + await engine.runStep(context()); + + // No entry the engine appends carries `via` when the override is inactive. + const entries = readJournalForChange(projectRoot, 'b', 'add-login-api'); + expect(entries.every((e) => e.via === undefined)).toBe(true); + }); }); diff --git a/test/batch-engine/rex-remote-runtime.test.ts b/test/batch-engine/rex-remote-runtime.test.ts index 6391d74..4ca994d 100644 --- a/test/batch-engine/rex-remote-runtime.test.ts +++ b/test/batch-engine/rex-remote-runtime.test.ts @@ -1,4 +1,8 @@ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { mkdtempSync, writeFileSync, rmSync } from 'node:fs'; +import path from 'node:path'; +import os from 'node:os'; import { makeRexRemoteRuntime, buildRemoteRunCommand, @@ -336,6 +340,82 @@ describe('makeRexRemoteRuntime — error paths (actionable, no hang, no secret l }); }); +/** + * Job-control launch + group-kill teardown — the reap-agents-on-teardown change. + * + * The launch command runs under `set -m` and records the agent's pid to a + * pidfile (`agent.pid`) so teardown can reap the whole process group (negative + * pid) BEFORE removing the run dir and closing the session — the prior code + * closed the session leaving the nohup'd agent alive on the server. + */ +describe('makeRexRemoteRuntime — job-control launch + group-kill teardown', () => { + function executeCommands(server: { fetch: FetchLike; calls: Recorded[] }): string[] { + return server.calls + .filter((c) => c.path === '/execute') + .map((c) => (c.body as { command: string }).command); + } + + it('launches under `set -m` and writes the agent pid to a pidfile', async () => { + const server = fakeServer({ authToken: 'tok', logChunks: ['x\n'], exitCode: 0 }); + const runtime = makeRexRemoteRuntime({ + host: 'h', + port: 1, + authToken: 'tok', + pollIntervalMs: 0, + deps: noWaitDeps(server.fetch), + }); + await runtime(req, () => {}); + const cmds = executeCommands(server); + const launch = cmds.find((c) => /^nohup /.test(c)); + expect(launch).toBeDefined(); + // `set -m` makes the backgrounded pipeline its own process-group leader. + expect(launch).toContain('set -m'); + // The pid is recorded to a pidfile so teardown finds the group. + expect(launch).toContain('agent.pid'); + expect(launch).toMatch(/echo \$! >/); + }); + + it('teardown reaps the group (TERM→KILL) BEFORE rm -rf and close', async () => { + const server = fakeServer({ authToken: 'tok', logChunks: ['x\n'], exitCode: 0 }); + const runtime = makeRexRemoteRuntime({ + host: 'h', + port: 1, + authToken: 'tok', + pollIntervalMs: 0, + deps: noWaitDeps(server.fetch), + }); + await runtime(req, () => {}); + + const allCalls = server.calls; + const executeCalls = allCalls.filter((c) => c.path === '/execute'); + const teardownKill = executeCalls.find((c) => + /kill -TERM -- -\$\(cat/.test((c.body as { command: string }).command) + ); + expect(teardownKill).toBeDefined(); + const killCmd = (teardownKill!.body as { command: string }).command; + // TERM then KILL on the negative pgid, best-effort. + expect(killCmd).toContain('kill -TERM -- -$(cat'); + expect(killCmd).toContain('kill -KILL -- -$(cat'); + expect(killCmd).toContain('agent.pid'); + + // ORDERING: the group-kill /execute appears BEFORE the rm -rf /execute and + // the /close_session + /close calls (the pidfile lives in runDir and the + // session must be alive for the kill to run). + const killIdx = allCalls.indexOf(teardownKill!); + const rmrf = allCalls.find((c) => { + if (c.path !== '/execute') return false; + return /^rm -rf /.test((c.body as { command: string }).command); + }); + expect(rmrf).toBeDefined(); + const rmrfIdx = allCalls.indexOf(rmrf!); + const closeSessionIdx = allCalls.findIndex((c) => c.path === '/close_session'); + const closeIdx = allCalls.findIndex((c) => c.path === '/close'); + expect(killIdx).toBeLessThan(rmrfIdx); + expect(rmrfIdx).toBeLessThan(closeSessionIdx); + expect(closeSessionIdx).toBeLessThan(closeIdx); + }); +}); + describe('resolveTransport — scheme selection + plaintext guard', () => { it('defaults a loopback host to http (token never leaves the machine)', () => { expect(resolveTransport('localhost')).toEqual({ scheme: 'http', host: 'localhost' }); @@ -460,4 +540,133 @@ describe('buildRemoteRunCommand', () => { }); expect(cmd).toBe("cat '/tmp/run/prompt.txt' | 'agent' '-p' 'it'\\''s'"); }); + + it('omits exports when the request env is empty (parity with prior output)', () => { + const cmd = buildRemoteRunCommand('/tmp/run/prompt.txt', { + command: 'agent', + args: ['-p'], + instructions: '', + cwd: '/', + env: {}, + }); + expect(cmd.startsWith('cat ')).toBe(true); + expect(cmd).not.toContain('export '); + }); +}); + +/** + * Env threading — the per-step env the engine places on AgentSpawnRequest.env + * reaches the agent the remote runtime launches, with overlay merge semantics. + * + * Implements `features/rex-env-threading/env-reaches-spawned-agent.feature`. + * The `/execute` nohup body assertion uses the fake server (no spawn); the + * execution assertions run buildRemoteRunCommand's output through a real `sh -c` + * with a controlled base env so actual visibility is proven (per the testing + * standard: an execution test per builder proves the spawned command observes the + * request value). Temp dirs are isolated via mkdtemp and removed in afterEach. + */ +describe('makeRexRemoteRuntime — env threading (env-reaches-spawned-agent.feature)', () => { + let tmp: string; + + beforeEach(() => { + tmp = mkdtempSync(path.join(os.tmpdir(), 'rex-remote-env-')); + }); + + afterEach(() => { + rmSync(tmp, { recursive: true, force: true }); + }); + + /** + * Run buildRemoteRunCommand's output through a real `sh -c` with a controlled + * base env, returning the spawned command's stdout. Keeps PATH so `sh` and the + * agent binary resolve; controls only the vars under test. + */ + function runBuiltCommand(request: AgentSpawnRequest, baseEnv: NodeJS.ProcessEnv): string { + const promptPath = path.join(tmp, 'prompt.txt'); + writeFileSync(promptPath, request.instructions ?? ''); + const cmd = buildRemoteRunCommand(promptPath, request); + return execFileSync('sh', ['-c', cmd], { + env: { PATH: process.env.PATH ?? '', ...baseEnv }, + encoding: 'utf-8', + }); + } + + it('the /execute nohup body command exports a per-step env var set on the request env', async () => { + const server = fakeServer({ authToken: 'tok', logChunks: ['x\n'], exitCode: 0 }); + const runtime = makeRexRemoteRuntime({ + host: 'h', + port: 1, + authToken: 'tok', + pollIntervalMs: 0, + deps: noWaitDeps(server.fetch), + }); + await runtime({ ...req, env: { RATCHET_STEP_VAR: 'from-engine' } }, () => {}); + const launch = server.calls.find( + (c) => c.path === '/execute' && /nohup/.test((c.body as { command: string }).command) + ); + expect(launch).toBeDefined(); + const cmd = (launch!.body as { command: string }).command; + // The export rides inside the nohup launcher's `( … )` subshell, before `cat`. + // The outer `shquote(launch)` re-escapes the inner single quotes, so assert on + // the quote-free tokens that survive the wrap (`export NAME=` and the value); + // actual visibility is proven by the execution assertions below. + expect(cmd).toContain('export RATCHET_STEP_VAR='); + expect(cmd).toContain('from-engine'); + expect(cmd).toContain('cat '); + expect(cmd).toContain('exit.code'); // the nohup launcher wrapping is unchanged + }); + + it('the spawned agent observes the request env value (execution via sh -c)', () => { + const out = runBuiltCommand( + { + command: 'printenv', + args: ['RATCHET_STEP_VAR'], + instructions: '', + cwd: '/srv/project', + env: { RATCHET_STEP_VAR: 'from-engine' }, + }, + // A colliding SESSION base value — the request must win (overlay). + { RATCHET_STEP_VAR: 'from-session' } + ); + expect(out.trim()).toBe('from-engine'); + }); + + it('a base var absent from the request env stays visible (overlay, not replace)', () => { + const out = runBuiltCommand( + { + command: 'printenv', + args: ['BASE_ONLY'], + instructions: '', + cwd: '/srv/project', + env: { RATCHET_STEP_VAR: 'from-engine' }, + }, + // BASE_ONLY is in the session base but NOT in the request env. + { BASE_ONLY: 'base-val' } + ); + expect(out.trim()).toBe('base-val'); + }); + + /** + * Implements: features/agent-cmd-override/shared-spawn-helper.feature + * + * Scenario: an override-built request threads env like any other request. An + * override-shaped request (`bash -c ` with a per-step env var) has + * that var exported in the built launch command — the override path composes + * with #89's env threading on the remote runtime. + */ + it('an override-built (bash -c) request exports a per-step env var into the launch command', () => { + const promptPath = path.join(tmp, 'prompt.txt'); + writeFileSync(promptPath, 'PROMPT BODY'); + const cmd = buildRemoteRunCommand(promptPath, { + command: 'bash', + args: ['-c', 'echo stub-agent'], + instructions: 'PROMPT BODY', + cwd: '/srv/project', + env: { RATCHET_STEP_VAR: 'from-engine' }, + }); + expect(cmd).toContain('export RATCHET_STEP_VAR='); + expect(cmd).toContain('from-engine'); + expect(cmd).toContain("'bash' '-c' 'echo stub-agent'"); + expect(cmd).toContain('cat '); + }); }); diff --git a/test/batch-engine/rex-sidecar-runtime.test.ts b/test/batch-engine/rex-sidecar-runtime.test.ts index 59b0539..5e55636 100644 --- a/test/batch-engine/rex-sidecar-runtime.test.ts +++ b/test/batch-engine/rex-sidecar-runtime.test.ts @@ -1,5 +1,9 @@ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { EventEmitter } from 'node:events'; +import { execFileSync } from 'node:child_process'; +import { mkdtempSync, writeFileSync, rmSync } from 'node:fs'; +import path from 'node:path'; +import os from 'node:os'; import { makeRexSidecarRuntime, buildRunCommand, @@ -24,6 +28,8 @@ class FakeChild extends EventEmitter implements SidecarChild { /** Ops the runtime wrote to stdin (parsed JSON). */ ops: any[] = []; killed: NodeJS.Signals[] = []; + /** A fake OS pid so teardown's `killGroup(pid, sig)` seam is exercised. */ + pid: number | null = 4242; stdout = { setEncoding: () => {}, @@ -76,9 +82,11 @@ function fakeDeps(child: FakeChild, over: Partial = {}): { deps: SidecarDeps; files: Map; removed: string[]; + killGroups: { pid: number; signal: NodeJS.Signals }[]; } { const files = new Map(); const removed: string[] = []; + const killGroups: { pid: number; signal: NodeJS.Signals }[] = []; const deps: SidecarDeps = { spawn: () => child, bootstrap: () => LAUNCH, @@ -87,15 +95,16 @@ function fakeDeps(child: FakeChild, over: Partial = {}): { rmrf: (p) => removed.push(p), setTimer: (fn, ms) => setTimeout(fn, ms), clearTimer: (h) => clearTimeout(h), + killGroup: (pid, signal) => killGroups.push({ pid, signal }), ...over, }; - return { deps, files, removed }; + return { deps, files, removed, killGroups }; } describe('makeRexSidecarRuntime', () => { it('drives ready→run→stdout→exit→shutdown→closed and accumulates the transcript', async () => { const child = new FakeChild(); - const { deps } = fakeDeps(child); + const { deps, removed } = fakeDeps(child); const runtime = makeRexSidecarRuntime({ projectRoot: '/proj', deps }); const events: AgentEvent[] = []; @@ -136,8 +145,11 @@ describe('makeRexSidecarRuntime', () => { // Exit event carried the exit code; shutdown was sent after exit. expect(events.find((e) => e.kind === 'exit')?.exitCode).toBe(0); expect(child.ops.some((o) => o.op === 'shutdown')).toBe(true); - // Child torn down. - expect(child.killed.length).toBeGreaterThan(0); + // On the CLEAN path the sidecar exits itself (closed → exit), so teardown + // does NOT escalate to a kill — the run dir is swept instead (the "no + // leftover" guarantee). No kill, no killGroup. + expect(child.killed).toHaveLength(0); + expect(removed.length).toBeGreaterThan(0); }); it('reports a non-zero agent exit in the accumulated result', async () => { @@ -455,8 +467,13 @@ describe('makeRexSidecarRuntime', () => { it('tears down the child on a timeout and surfaces a timeout error', async () => { const child = new FakeChild(); - const { deps } = fakeDeps(child); - const runtime = makeRexSidecarRuntime({ projectRoot: '/proj', timeoutMs: 20, deps }); + const { deps, killGroups, removed } = fakeDeps(child); + const runtime = makeRexSidecarRuntime({ + projectRoot: '/proj', + timeoutMs: 20, + killGraceMs: 1, + deps, + }); const events: AgentEvent[] = []; // Never reply to ops → the timeout must fire. @@ -464,8 +481,323 @@ describe('makeRexSidecarRuntime', () => { child.emitLine({ event: 'ready', locus: 'local' }); const result = await runPromise; + // The run settles immediately on timeout, but the kill is scheduled for + // killGraceMs later — give the timer a tick to fire before asserting. + await new Promise((r) => setTimeout(r, 30)); expect(result.exitCode).not.toBe(0); expect(result.stderr).toMatch(/timed out/i); - expect(child.killed.length).toBeGreaterThan(0); + // On the timeout path the sidecar did not exit on its own, so teardown + // escalates: a shutdown op is sent first, then after the grace the agent + // process GROUP is SIGKILLed via killGroup (negative-pgid kill). The bare + // child.kill is NOT used (it cannot reach the agent's group). + expect(child.ops.some((o) => o.op === 'shutdown')).toBe(true); + expect(killGroups.some((k) => k.signal === 'SIGKILL' && k.pid === child.pid)).toBe(true); + expect(child.killed).toHaveLength(0); + // The run dir is still swept on the timeout path (no leftovers). + expect(removed.length).toBeGreaterThan(0); + }); + + /** + * The run op must carry `run_dir` so the sidecar writes its pidfile + sentinels + * there (the new job-control launcher needs it to reap the agent's process + * group on teardown). For `local` it is the host runDir verbatim. + */ + it('threads run_dir in the run op (local locus → host runDir)', async () => { + const child = new FakeChild(); + const { deps } = fakeDeps(child); + const runtime = makeRexSidecarRuntime({ projectRoot: '/proj', deps }); + + child.onOp = (op, self) => { + if (op.op === 'run') self.emitLine({ event: 'exit', id: op.id, exit_code: 0 }); + else if (op.op === 'shutdown') { + self.emitLine({ event: 'closed' }); + self.emit('exit', 0, null); + } + }; + + const runPromise = runtime(request(), () => {}); + child.emitLine({ event: 'ready', locus: 'local' }); + await runPromise; + + const runOp = child.ops.find((o) => o.op === 'run'); + expect(runOp.run_dir).toBeDefined(); + expect(runOp.run_dir).toContain('/proj/.ratchet/batches/b/.run/'); + }); + + /** + * For `docker` the sidecar runs IN the container, so run_dir must be the + * IN-CONTAINER path (the host path does not exist there) — same swap as the + * prompt file onto DOCKER_MOUNT_CONTAINER. + */ + // ------------------------------------------------------------------------- + // Docker-locus hardening (features/docker-locus-hardening): the five + // `REX_DOCKER_*` knobs thread from RexSidecarRuntimeOptions into the + // bootstrap call for docker ONLY — local never receives them. + // ------------------------------------------------------------------------- + it('threads the docker hardening knobs into the bootstrap call (docker only)', async () => { + const child = new FakeChild(); + let bootstrapArgs: BootstrapOptions | undefined; + const { deps } = fakeDeps(child, { + bootstrap: (opts) => { + bootstrapArgs = opts; + return LAUNCH; + }, + }); + const runtime = makeRexSidecarRuntime({ + projectRoot: '/host/project', + locus: 'docker', + image: 'my/image:tag', + dockerUser: '1000:1000', + dockerMemory: '4g', + dockerPidsLimit: 256, + dockerCpus: 1.5, + network: 'none', + deps, + }); + + child.onOp = (op, self) => { + if (op.op === 'run') self.emitLine({ event: 'exit', id: op.id, exit_code: 0 }); + else if (op.op === 'shutdown') { + self.emitLine({ event: 'closed' }); + self.emit('exit', 0, null); + } + }; + + const runPromise = runtime(request({ cwd: '/host/project' }), () => {}); + child.emitLine({ event: 'ready', locus: 'docker' }); + await runPromise; + + expect(bootstrapArgs?.dockerUser).toBe('1000:1000'); + expect(bootstrapArgs?.dockerMemory).toBe('4g'); + expect(bootstrapArgs?.dockerPidsLimit).toBe(256); + expect(bootstrapArgs?.dockerCpus).toBe(1.5); + expect(bootstrapArgs?.network).toBe('none'); + }); + + it('omits the docker hardening knobs from the bootstrap call for local', async () => { + const child = new FakeChild(); + let bootstrapArgs: BootstrapOptions | undefined; + const { deps } = fakeDeps(child, { + bootstrap: (opts) => { + bootstrapArgs = opts; + return LAUNCH; + }, + }); + const runtime = makeRexSidecarRuntime({ + projectRoot: '/proj', + locus: 'local', + // Even if set, they must NOT reach the local bootstrap call. + dockerUser: '1000:1000', + dockerMemory: '4g', + dockerPidsLimit: 256, + dockerCpus: 1.5, + network: 'none', + deps, + }); + + child.onOp = (op, self) => { + if (op.op === 'run') self.emitLine({ event: 'exit', id: op.id, exit_code: 0 }); + else if (op.op === 'shutdown') { + self.emitLine({ event: 'closed' }); + self.emit('exit', 0, null); + } + }; + + const runPromise = runtime(request(), () => {}); + child.emitLine({ event: 'ready', locus: 'local' }); + await runPromise; + + expect(bootstrapArgs?.dockerUser).toBeUndefined(); + expect(bootstrapArgs?.dockerMemory).toBeUndefined(); + expect(bootstrapArgs?.dockerPidsLimit).toBeUndefined(); + expect(bootstrapArgs?.dockerCpus).toBeUndefined(); + expect(bootstrapArgs?.network).toBeUndefined(); + }); + + it('threads run_dir translated to the in-container mount (docker locus)', async () => { + const child = new FakeChild(); + const { deps } = fakeDeps(child); + const runtime = makeRexSidecarRuntime({ + projectRoot: '/host/project', + locus: 'docker', + image: 'img', + deps, + }); + + child.onOp = (op, self) => { + if (op.op === 'run') self.emitLine({ event: 'exit', id: op.id, exit_code: 0 }); + else if (op.op === 'shutdown') { + self.emitLine({ event: 'closed' }); + self.emit('exit', 0, null); + } + }; + + const runPromise = runtime(request({ cwd: '/host/project' }), () => {}); + child.emitLine({ event: 'ready', locus: 'docker' }); + await runPromise; + + const runOp = child.ops.find((o) => o.op === 'run'); + expect(runOp.run_dir).toContain(`${DOCKER_MOUNT_CONTAINER}/.ratchet/batches/`); + expect(runOp.run_dir).not.toContain('/host/project'); + }); + + /** + * Teardown ordering: on the dirty (timeout/error) path the runtime sends a + * `shutdown` op FIRST (asking the sidecar to reap its agent group cleanly), + * and only SIGKILLs the group after the grace — it does NOT send SIGTERM to + * the sidecar directly. This proves the fix for the orphan: the prior code + * SIGTERM'd the sidecar immediately, killing it before it could reap the + * agent it launched. + */ + it('teardown sends shutdown BEFORE killGroup (not SIGTERM) on the dirty path', async () => { + const child = new FakeChild(); + const order: string[] = []; + const { deps, killGroups } = fakeDeps(child, { + killGroup: (pid, sig) => { + order.push(`killGroup:${sig}`); + killGroups.push({ pid, signal: sig }); + }, + }); + const runtime = makeRexSidecarRuntime({ + projectRoot: '/proj', + timeoutMs: 20, + killGraceMs: 1, + deps, + }); + + // Capture the shutdown op ordering relative to the kill. + child.onOp = (op) => { + if (op.op === 'shutdown') order.push('shutdown'); + }; + + const runPromise = runtime(request(), () => {}); + child.emitLine({ event: 'ready', locus: 'local' }); + await runPromise; + await new Promise((r) => setTimeout(r, 30)); + + expect(order).toEqual(['shutdown', 'killGroup:SIGKILL']); + }); +}); + +/** + * Env threading — the per-step env the engine places on AgentSpawnRequest.env + * reaches the agent the sidecar launches, with overlay merge semantics. + * + * Implements `features/rex-env-threading/env-reaches-spawned-agent.feature`. + * The run-op assertion uses the FakeChild (no spawn); the execution assertions + * run the built command through a real `sh -c` with a controlled base env so + * actual visibility — not just string shape — is proven (per the testing + * standard: an execution test per builder proves the spawned command observes the + * request value). Temp dirs are isolated via mkdtemp and removed in afterEach. + */ +describe('makeRexSidecarRuntime — env threading (env-reaches-spawned-agent.feature)', () => { + let tmp: string; + + beforeEach(() => { + tmp = mkdtempSync(path.join(os.tmpdir(), 'rex-sidecar-env-')); + }); + + afterEach(() => { + rmSync(tmp, { recursive: true, force: true }); + }); + + /** + * Build the sidecar run command for a request and run it through a real + * `sh -c` with a controlled base env, returning the spawned command's stdout. + * Keeps PATH so `sh` and the agent binary resolve; controls only the vars + * under test so overlay semantics are asserted deterministically. + */ + function runBuiltCommand(request: AgentSpawnRequest, baseEnv: NodeJS.ProcessEnv): string { + const promptPath = path.join(tmp, 'prompt.txt'); + writeFileSync(promptPath, request.instructions ?? ''); + const cmd = buildRunCommand(promptPath, request); + return execFileSync('sh', ['-c', cmd], { + env: { PATH: process.env.PATH ?? '', ...baseEnv }, + encoding: 'utf-8', + }); + } + + it('the run-op command exports a per-step env var set on the request env', async () => { + const child = new FakeChild(); + const { deps } = fakeDeps(child); + const runtime = makeRexSidecarRuntime({ projectRoot: '/proj', deps }); + + child.onOp = (op, self) => { + if (op.op === 'run') self.emitLine({ event: 'exit', id: op.id, exit_code: 0 }); + else if (op.op === 'shutdown') { + self.emitLine({ event: 'closed' }); + self.emit('exit', 0, null); + } + }; + + const runPromise = runtime( + request({ env: { RATCHET_BATCH_NAME: 'b', RATCHET_STEP_VAR: 'from-engine' } }), + () => {} + ); + child.emitLine({ event: 'ready', locus: 'local' }); + await runPromise; + + const runOp = child.ops.find((o) => o.op === 'run'); + // The export sits after the cwd prefix, before the `cat … | …` pipeline. + expect(runOp.command).toContain("export RATCHET_STEP_VAR='from-engine'; "); + expect(runOp.command).toContain('cat '); + expect(runOp.command).toContain('| '); + }); + + it('the spawned agent observes the request env value (execution via sh -c)', () => { + // The agent argv is `printenv RATCHET_STEP_VAR`, which prints the var it sees. + const out = runBuiltCommand( + { + command: 'printenv', + args: ['RATCHET_STEP_VAR'], + instructions: '', + cwd: '/proj', + env: { RATCHET_STEP_VAR: 'from-engine' }, + }, + // A colliding SESSION base value — the request must win (overlay). + { RATCHET_STEP_VAR: 'from-session' } + ); + expect(out.trim()).toBe('from-engine'); + }); + + it('a base var absent from the request env stays visible (overlay, not replace)', () => { + const out = runBuiltCommand( + { + command: 'printenv', + args: ['BASE_ONLY'], + instructions: '', + cwd: '/proj', + env: { RATCHET_STEP_VAR: 'from-engine' }, + }, + // BASE_ONLY is in the session base but NOT in the request env. + { BASE_ONLY: 'base-val' } + ); + expect(out.trim()).toBe('base-val'); + }); + + /** + * Implements: features/agent-cmd-override/shared-spawn-helper.feature + * + * Scenario: an override-built request threads env like any other request. An + * override-shaped request (`bash -c ` with a per-step env var) has + * that var exported in the built launch command — the override path composes + * with #89's env threading. + */ + it('an override-built (bash -c) request exports a per-step env var into the launch command', () => { + const promptPath = path.join(tmp, 'prompt.txt'); + writeFileSync(promptPath, 'PROMPT BODY'); + const cmd = buildRunCommand(promptPath, { + command: 'bash', + args: ['-c', 'echo stub-agent'], + instructions: 'PROMPT BODY', + cwd: '/proj', + env: { RATCHET_STEP_VAR: 'from-engine' }, + }); + // The per-step env var is exported before the `cat … | bash -c …` pipeline, + // so an override-built request threads env exactly like an adapter-built one. + expect(cmd).toContain("export RATCHET_STEP_VAR='from-engine'; "); + expect(cmd).toContain("'bash' '-c' 'echo stub-agent'"); + expect(cmd).toContain('cat '); + expect(cmd).toContain('| '); }); }); diff --git a/test/batch-engine/spawn-command.test.ts b/test/batch-engine/spawn-command.test.ts new file mode 100644 index 0000000..72151a1 --- /dev/null +++ b/test/batch-engine/spawn-command.test.ts @@ -0,0 +1,116 @@ +import { describe, it, expect } from 'vitest'; +import { shquote, buildEnvExports } from '../../src/core/batch/engine/runtime/spawn-command.js'; + +/** + * Unit tests for the shared env-serialization helper used by BOTH rex runtimes. + * + * Implements `features/rex-env-threading/env-serialization-safety.feature`. + * No filesystem, no spawn — pure serialization assertions over in-memory inputs + * (the testing standard's unit layer). Shell-execution proof that a built command + * actually observes the value lives in the runtime tests (`rex-sidecar-runtime` + * and `rex-remote-runtime`), which run the built command through `sh -c`. + */ + +describe('shquote', () => { + it('single-quotes a plain token', () => { + expect(shquote('plain')).toBe("'plain'"); + }); + + it('escapes embedded single quotes with the canonical close/escape/reopen idiom', () => { + // `it's` → 'it'\''s' : close quote, escaped quote (\''), reopen quote. + expect(shquote("it's")).toBe("'it'\\''s'"); + }); + + it('wraps an empty string in a single-quote pair (preserves emptiness)', () => { + expect(shquote('')).toBe("''"); + }); +}); + +describe('buildEnvExports', () => { + it('emits an export statement per valid-identifier entry, in insertion order', () => { + const out = buildEnvExports({ RATCHET_STEP_VAR: 'from-engine', FOO: 'bar' }); + expect(out).toBe("export RATCHET_STEP_VAR='from-engine'; export FOO='bar'; "); + }); + + it('yields an empty prefix for an empty env', () => { + expect(buildEnvExports({})).toBe(''); + }); + + it('yields an empty prefix for an absent env (undefined)', () => { + expect(buildEnvExports(undefined)).toBe(''); + }); + + it('skips entries whose value is undefined', () => { + const out = buildEnvExports({ KEEP: 'yes', DROP: undefined }); + expect(out).toBe("export KEEP='yes'; "); + }); + + it('skips entries whose name is not a valid shell identifier', () => { + // Invalid names are unreachable in shell and would break `export`. + const out = buildEnvExports({ + VALID_VAR: 'kept', + '1leading-digit': 'dropped', + 'has-dash': 'dropped', + 'has space': 'dropped', + 'has.dot': 'dropped', + 'has$dollar': 'dropped', + }); + expect(out).toBe("export VALID_VAR='kept'; "); + }); + + it('keeps entries whose name starts with an underscore or letter', () => { + const out = buildEnvExports({ _UNDER: 'u', A: 'a', Z9: 'z' }); + expect(out).toBe("export _UNDER='u'; export A='a'; export Z9='z'; "); + }); + + it('quotes a value with a single quote using shquote (no injection)', () => { + const val = "it's a 'test'"; + const out = buildEnvExports({ Q: val }); + // The serialized form is exactly one export statement quoting the whole value. + expect(out).toBe(`export Q=${shquote(val)}; `); + // And it is inert: there is no trailing command after the export that the + // embedded quote could splice into (the value is fully wrapped in single quotes). + expect(out).toMatch(/^export Q='.*'; $/); + expect(out.endsWith("'; ")).toBe(true); + }); + + it('quotes a value containing $ so the serialized form cannot be expanded', () => { + const val = '$HOME/${UNSET:-x} $(id)'; + const out = buildEnvExports({ DOLLAR: val }); + // Single-quoted: the $ never reaches the shell, so it is a literal. + expect(out).toBe(`export DOLLAR='${val}'; `); + }); + + it('quotes a value with spaces, newlines, and semicolons as one inert token', () => { + const val = 'a b\nc; rm -rf /; & | > <'; + const out = buildEnvExports({ METACHAR: val }); + // A newline inside single quotes is preserved byte-for-byte (no command split). + expect(out).toBe(`export METACHAR=${shquote(val)}; `); + expect(out).toContain("'a b\nc; rm -rf /; & | > <'"); + }); + + it('coerces non-string defined values to string before quoting', () => { + // NodeJS.ProcessEnv values are string|undefined, but be defensive: a numeric + // or boolean coerces so the export never emits an unquoted token. + const out = buildEnvExports({ NUM: 42 as unknown as string, FLAG: true as unknown as string }); + expect(out).toBe("export NUM='42'; export FLAG='true'; "); + }); + + it('produces no output for a request env whose every entry is dropped', () => { + expect(buildEnvExports({ 'bad-name': 'x', DROP: undefined })).toBe(''); + }); + + /** + * Implements: features/agent-env-scoping/sidecar-bootstrap-env.feature + * Scenario: The per-step request env overlays the scoped base in the agent + * command — exports RATCHET_BATCH_NAME and exports no var absent from the + * request env. + */ + it('exports RATCHET_BATCH_NAME from the request env and never a var absent from it', () => { + const out = buildEnvExports({ RATCHET_BATCH_NAME: 'demo' }); + expect(out).toBe("export RATCHET_BATCH_NAME='demo'; "); + // A host secret that was dropped by scoping is NOT in the request env, so + // it is never exported into the agent command. + expect(out).not.toContain('SUPER_SECRET_TOKEN'); + }); +}); diff --git a/test/commands/apply.test.ts b/test/commands/apply.test.ts index 9b17105..00457e9 100644 --- a/test/commands/apply.test.ts +++ b/test/commands/apply.test.ts @@ -63,7 +63,7 @@ describe('applyCommand', () => { it('advances a happy-path apply via the forced apply transition', async () => { await fixture.writeChangeWithTasks('ready', { done: 0, total: 2 }); - const { spawner, calls } = completingSpawner(fixture.root, 'ready'); + const { spawner, calls } = completingSpawner(fixture.root, 'ready', 'apply'); await applyCommand('ready', {}, { projectRoot: () => fixture.root, spawner }); diff --git a/test/commands/batch/apply.test.ts b/test/commands/batch/apply.test.ts index eae3fd5..00aa782 100644 --- a/test/commands/batch/apply.test.ts +++ b/test/commands/batch/apply.test.ts @@ -57,6 +57,10 @@ vi.mock('../../../src/core/batch/engine/index.js', () => ({ journal.some((e) => e.kind === 'completion' && e.transition === 'pr'), readJournalTolerant: readJournalTolerantMock, runProofOfWork: runProofOfWorkMock, + // Pure helpers `renderResult` imports for the override notice — passed + // through as the real functions so the notice text is asserted honestly. + agentOverrideNotice: (envVar: string) => `⚠ agent overridden by ${envVar}`, + BATCH_AGENT_CMD_ENV: 'RATCHET_BATCH_AGENT_CMD', })); vi.mock('../../../src/core/planning-home.js', () => ({ @@ -533,4 +537,63 @@ describe('batchApplyCommand', () => { expect(output()).toContain('Nothing to do — all changes are done.'); expect(runPrStepMock).not.toHaveBeenCalled(); }); + + /** + * Implements: features/agent-cmd-override/override-notice.feature + * + * Scenario: batch apply text output carries the override notice / --json + * carries agentOverride. The engine sets `StepResult.agentOverride` exactly + * when its spawn ran under an active `RATCHET_BATCH_AGENT_CMD`; `renderResult` + * prints the one-line notice (text) and emits the field verbatim (--json). + * No override → no notice and no field. + */ + describe('agent-cmd override notice (override-notice.feature)', () => { + it('prints the one-line override notice in text mode when the result carries agentOverride', async () => { + await fixture.writeBatch('b', { phases: [{ ...PHASE, changes: [{ name: 'c1' }] }] }); + runStepMock.mockResolvedValue({ + state: 'advanced', + change: 'c1', + transition: 'propose', + message: 'step complete', + agentOverride: true, + } satisfies StepResult); + + await batchApplyCommand('b', {}); + + expect(output()).toContain('⚠ agent overridden by RATCHET_BATCH_AGENT_CMD'); + // The notice precedes the Ran line. + expect(output().indexOf('⚠ agent overridden')).toBeLessThan(output().indexOf('Ran:')); + }); + + it('emits agentOverride: true in --json when the result carries agentOverride', async () => { + await fixture.writeBatch('b', { phases: [{ ...PHASE, changes: [{ name: 'c1' }] }] }); + runStepMock.mockResolvedValue({ + state: 'advanced', + change: 'c1', + transition: 'propose', + message: 'step complete', + agentOverride: true, + } satisfies StepResult); + + await batchApplyCommand('b', { json: true }); + + const parsed = JSON.parse(output()); + expect(parsed.agentOverride).toBe(true); + }); + + it('prints no notice and omits agentOverride when no override is active', async () => { + await fixture.writeBatch('b', { phases: [{ ...PHASE, changes: [{ name: 'c1' }] }] }); + runStepMock.mockResolvedValue({ + state: 'advanced', + change: 'c1', + transition: 'propose', + message: 'step complete', + } satisfies StepResult); + + await batchApplyCommand('b', { json: true }); + + const parsed = JSON.parse(output()); + expect(parsed.agentOverride).toBeUndefined(); + }); + }); }); diff --git a/test/commands/batch/report.test.ts b/test/commands/batch/report.test.ts index 6509935..a2a35a9 100644 --- a/test/commands/batch/report.test.ts +++ b/test/commands/batch/report.test.ts @@ -132,4 +132,65 @@ describe('batchReportCommand', () => { expect(getParkedStep(fixture.root, 'b', 'c1')?.feedback).toBe('wrong approach'); expect(output()).toMatch(/re-runs propose/); }); + + /** + * Implements: features/agent-cmd-override/override-provenance.feature + * + * Scenario: agent-reported journal entries are stamped. `batch report` runs + * INSIDE the spawned agent; when its own process env carries an active + * RATCHET_BATCH_AGENT_CMD, every entry it appends (progress, blocker, + * needs-input, completion) carries `via: 'env-override'` — the spawned + * stand-in inherits the var, so stub-reported completions are auditable. + * Override-free reports stay unstamped. + */ + describe('override provenance stamping (override-provenance.feature)', () => { + const ENV = 'RATCHET_BATCH_AGENT_CMD'; + let saved: string | undefined; + + beforeEach(() => { + saved = process.env[ENV]; + }); + afterEach(() => { + if (saved === undefined) delete process.env[ENV]; + else process.env[ENV] = saved; + }); + + it('stamps every appended entry with `via: env-override` under an active override', async () => { + process.env[ENV] = 'echo stub-agent'; + await batchReportCommand('b', { change: 'c1', status: 'making progress' }); + await batchReportCommand('b', { change: 'c1', complete: 'all done' }); + + const entries = readJournalForChange(fixture.root, 'b', 'c1'); + expect(entries).toHaveLength(2); + for (const e of entries) expect(e.via).toBe('env-override'); + }); + + it('stamps blocker and needs-input entries under an active override', async () => { + process.env[ENV] = 'echo stub-agent'; + await batchReportCommand('b', { change: 'c1', blocker: 'which db?' }); + // Answer is a USER action (recordAnswer appends its own 'answer' entry + // inside journal.ts, not report.ts), so it is intentionally NOT stamped — + // only the agent-reported kinds report.ts appends directly carry `via`. + await batchReportCommand('b', { change: 'c1', answer: 'use postgres' }); + await batchReportCommand('b', { change: 'c1', needsInput: 'need an API key' }); + + const entries = readJournalForChange(fixture.root, 'b', 'c1'); + const blocker = entries.find((e) => e.kind === 'blocker'); + const needsInput = entries.find((e) => e.kind === 'needs-input'); + expect(blocker?.via).toBe('env-override'); + expect(needsInput?.via).toBe('env-override'); + // The user-driven answer entry stays unstamped. + const answer = entries.find((e) => e.kind === 'answer'); + expect(answer?.via).toBeUndefined(); + }); + + it('leaves entries unstamped when no override is active', async () => { + delete process.env[ENV]; + await batchReportCommand('b', { change: 'c1', status: 'making progress' }); + + const entries = readJournalForChange(fixture.root, 'b', 'c1'); + expect(entries).toHaveLength(1); + expect(entries[0].via).toBeUndefined(); + }); + }); }); diff --git a/test/commands/eval/run.test.ts b/test/commands/eval/run.test.ts index f9e916a..aff2520 100644 --- a/test/commands/eval/run.test.ts +++ b/test/commands/eval/run.test.ts @@ -219,4 +219,72 @@ describe('evalRunCommand', () => { // The enabled set is persisted on the run, in display order. expect(run.gate).toEqual(['llm-judge']); }); + + /** + * Implements: features/agent-cmd-override/override-notice.feature and + * features/agent-cmd-override/override-provenance.feature (eval side). + * + * An active RATCHET_EVAL_AGENT_CMD prints the one-line notice (text), emits + * `agentOverride: true` (--json), and stamps `via: 'env-override'` on the + * persisted run record. The stamp keys on the var being ACTIVE for the run + * (deterministic), not on whether a case happened to spawn — exercised over + * an unbound case so no real agent is spawned. Override-free runs stay + * unstamped and carry no `agentOverride`. + */ + describe('agent-cmd override (override-notice.feature, override-provenance.feature)', () => { + const ENV = 'RATCHET_EVAL_AGENT_CMD'; + let saved: string | undefined; + + beforeEach(() => { + saved = process.env[ENV]; + }); + afterEach(() => { + if (saved === undefined) delete process.env[ENV]; + else process.env[ENV] = saved; + }); + + it('stamps `via: env-override` on the persisted run record under an active override', async () => { + process.env[ENV] = 'echo stub-judge'; + await evalRunCommand({ json: true }); + const parsed = JSON.parse(output()); + + const run = JSON.parse( + await fs.readFile( + path.join(fixture.root, '.ratchet', 'evals', 'runs', `${parsed.runId}.json`), + 'utf-8' + ) + ); + expect(run.via).toBe('env-override'); + }); + + it('emits agentOverride: true in --json under an active override', async () => { + process.env[ENV] = 'echo stub-judge'; + await evalRunCommand({ json: true }); + const parsed = JSON.parse(output()); + expect(parsed.agentOverride).toBe(true); + }); + + it('prints the one-line override notice atop the text scorecard under an active override', async () => { + process.env[ENV] = 'echo stub-judge'; + await evalRunCommand({}); + const text = output(); + expect(text).toContain('⚠ agent overridden by RATCHET_EVAL_AGENT_CMD'); + // The notice precedes the scorecard header. + expect(text.indexOf('⚠ agent overridden')).toBeLessThan(text.indexOf('Eval run')); + }); + + it('leaves the run record unstamped and omits agentOverride when no override is active', async () => { + delete process.env[ENV]; + await evalRunCommand({ json: true }); + const parsed = JSON.parse(output()); + expect(parsed.agentOverride).toBeUndefined(); + const run = JSON.parse( + await fs.readFile( + path.join(fixture.root, '.ratchet', 'evals', 'runs', `${parsed.runId}.json`), + 'utf-8' + ) + ); + expect(run.via).toBeUndefined(); + }); + }); });