Conversation
|
Review verdict: hold — conflicts with a design decision that landed on main after this branch was cut, and it retires only half the facade.
Suggested path: keep the direction, but make it a complete step — retire ALL backend free functions in one change, move each backend's spawn/log/summary body into its |
…ner (#1182) Completes the #832/#1169 direction for ALL backends (the #1169 PR did codex only, which left the facade half-migrated): - Each backend's spawn/log/summary body moves to a crate-private run_with_executable() in its own module; the BackendRunner impl is the only caller. The public run_* and run_*_with_executable wrappers are deleted, as are their facade re-exports. - runner_adapter_facade_preserves_public_call_paths now pins the BackendRunner abstraction (for_kind + the seven *Runner types) instead of the retired free functions. - Backend golden tests drive their core functions as before; NEW contract tests in backend_runner_contract_tests.rs drive all seven backends through the production path (for_kind(kind).run(&ctx)) with recording binaries, pinning each CLI's argv shape, stream handling, and result collection — the layer where runner wiring and CLI contracts meet. Per-backend golden tests below the runner path keep covering deeper behavior (stall kills, usage extraction, transcript binding, resource capture). Co-authored-by: Colton Spurgin <colton@coltonspurgin.tech>
#1183) * refactor(runner): retire all backend free functions behind BackendRunner; add runner-path contract tests Completes the #832/#1169 direction for ALL backends (the #1169 PR did codex only, which left the facade half-migrated): - Each backend's spawn/log/summary body moves to a crate-private run_with_executable() in its own module; the BackendRunner impl is the only caller. The public run_* and run_*_with_executable wrappers are deleted, as are their facade re-exports. - runner_adapter_facade_preserves_public_call_paths now pins the BackendRunner abstraction (for_kind + the seven *Runner types) instead of the retired free functions. - Backend golden tests drive their core functions as before; NEW contract tests in backend_runner_contract_tests.rs drive all seven backends through the production path (for_kind(kind).run(&ctx)) with recording binaries, pinning each CLI's argv shape, stream handling, and result collection — the layer where runner wiring and CLI contracts meet. Per-backend golden tests below the runner path keep covering deeper behavior (stall kills, usage extraction, transcript binding, resource capture). * feat(ledger): keep external-approval grants within the pending request (#1170 take-over) Ported from the codex branch (codex/external-approval-state), rebased onto today's main. External approval grants previously accepted a missing or broader pending request, reset consumption on replay, and could clear an unrelated human hold. Now: - Grants require a pending request; omitted bounds inherit from it. - Scope expansion (service label, env vars, request/dollar caps, expiry) is rejected under the ledger write lock. - Inspection and credential injection share one replay implementation. - Sparse lifecycle records retain bounds and purpose; consumption preserves granted credentials when configuration changes. Also splits the pure gate/alias/index analysis out of jsonl.rs into ledger/gates.rs — fixing the source-size-guard failure that made the original PR's CI red (jsonl.rs was 1518 lines; now 1290). Full docs in docs/EXTERNAL_APPROVALS.md (from the same branch). --------- Co-authored-by: Colton Spurgin <colton@coltonspurgin.tech>
1 participant
Codex dispatch already used
BackendRunner, but its implementation still passed through two public free functions. Move the existing spawn, log, summary, and transcript code intoCodexRunnerand retirerun_codex/run_codex_with_executable, completing the Codex cleanup step in #832. The remaining backend migrations stay on that issue.CLI arguments, environment handling, process supervision, and result collection are unchanged. The Rust library intentionally drops the two old Codex helper paths; callers use
CodexRunner.run(&RunContext)instead. No other backend or review code changes.Validation: preserve all seven existing Codex process/argv tests through the trait, and strengthen the registry test to check an explicitly selected executable and the full ordered argv.
cargo fmt --checkandgit diff --checkpass locally. Rust tests and Clippy run in CI to avoid recreating the Mac build cache.