rebase: run module agents on the abstracted provider backend - #170
Merged
Merged
Conversation
The rebase module agents were hardwired to the API transport
(`rebase_v3._tier_client` -> `Anthropic`, driven by `agent_loop`), so the
provider registry reached only STRICT_BACKEND and the review path. This
wires rebase onto the same abstraction: `ModuleRunConfig.backend` (default
"api", which leaves the in-process loop byte-for-byte unchanged) delegates
the whole module step to that provider's `run_session` when set to a
harness id.
Two things the in-process loop owns had to move into the bridge, or a
harness backend would silently run without them:
1. The 20-tool adapter surface. The shared registry has 6 tools; the rebase
agents run with 20. A spec carrying a `rebase` section makes the bridge
rebuild that pack in its own process via the same `build_rebase_tools`,
dispatched through `dispatch(..., extra=)` so scope guards, out-of-scope
recording and result bounds stay identical. `_build_backends` is split
into `build_backends` taking explicit serializable inputs (the bridge
cannot carry a StepContext) plus a thin in-process adapter.
2. The plan gate. `PlanGate` refuses edit_file/run_pytest/run_precommit at
DISPATCH and confines write_file to the plan directory while closed,
opening on the same event as the parent: a successful `.decision.md`
write. Enforcing at dispatch is stronger than withholding tools from the
advertised list, since a harness cannot bypass it by calling anyway. The
gate records `plan_gate_opened` so the parent can read `plan_done` back
out of the bridge trace.
The spec carries paths and model identity only -- no api_key and no child
env -- so nothing secret lands under `run_dir/bridge/`. Spec version goes to
2 so an older reader fails loudly instead of serving a 6-tool surface.
POLICY REVERSAL, called out: the bridge previously declined to carry
knowledge/memory retrieval ("a harness session may read this repo's
knowledge, never add to it"). Full parity was requested, so
search_debug_memory/record_debug_memory/skill_manage/search_skills are now
bridged. That reverses a recorded decision rather than merely implementing
one.
Disclosed gaps: `_PATH_ARGS` only knows the shared tools, so the read
containment pre-check is a no-op for adapter tools taking paths (writes stay
guarded by ToolScope inside dispatch); and cursor/codex have no native turn
cap, so `max_iters` binds only on claude-code (--max-turns) while the others
rely on `harness_timeout_s` plus prompt budget discipline.
Also fixes `_target_test_env`, which took a whole StepContext to read one
`settings.expansion_env()`; it now takes settings, which is what let the
backends builder work outside a step.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Generating MCP tool functions from the adapter's JSON schemas broke on
three shapes the unit tests did not reach, all found by running one real
cursor-agent session against the bridge:
1. Interleaved required/optional properties. `record_debug_memory`
declares an optional BEFORE a required one, which as positional
parameters is a SyntaxError. Parameters are now keyword-only, which
also preserves the schema's own property order; MCP calls by name.
2. Zero-property schemas. `git_diff_tests_upstream` emitted `def _tool(*, )`
-- "named arguments must follow bare *". The star is only emitted when
there is at least one parameter.
3. Unset optionals forwarded as None. A schema optional with no default
was passed through as None, so handlers never saw their own Python
default: `read_file` did `offset + int` on it and every call returned
{"error": "unsupported operand type(s) for +: 'NoneType' and 'int'"}.
Unset optionals are now dropped from the dispatch args; ones carrying a
schema default still forward it.
Smoke evidence (cursor-grok-4.6-high-fast, scratch root, not the live
campaign checkout): the harness discovers the bridge, `read_file` returns
real content, and the plan gate refuses through the harness --
"edit_file is locked until the plan-review decision file (.decision.md) is
written" -- with both events in bridge_trace.jsonl.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`_harness_attempt` passed `config.model` straight into the session request. That is the TIER model -- a raw-API id like `deepseek-flash` -- and every harness transport does `selected = model or settings.strict_backend_model`, so a non-empty tier model silently overrode the harness's own selection with an id it does not have. Running the rebase on cursor would have asked cursor-agent for `deepseek-flash`. Adds `rebase_backend_model` (the model INSIDE the harness, e.g. `cursor-grok-4.6-high-fast`), carried through ModuleRunConfig. When it is unset the request sends "", which lets the transport fall back to `strict_backend_model` and then the harness default -- never the API tier name. Pinned by a test that asserts the forwarded model is "" and not `deepseek-flash`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`build_backends(repo=...)` is a filesystem path -- it becomes
TestRunner(repo_root=Path(repo)). The bridge passed the spec's "repo",
which is the repo NAME, so every TestRunner-backed tool died before doing
anything:
FileNotFoundError: [Errno 2] No such file or directory:
PosixPath('vllm-omni')
That is run_pytest, run_precommit and reproduce -- i.e. exactly the gated
verification tools. A harness module agent could therefore edit code and
"finish" without a single test or pre-commit run, and the failure surfaced
only as an empty precommit log. The repo root now comes from `scope.root`,
which IS the repo root for a module scope (see `_module_scope`), with no
repo-specific fallback so a missing root fails loudly.
Second fault, same shape: the bridge sent no run `state`, so
`repo_name = state.task_spec.repo` was empty and the knowledge stores
resolved under an empty directory name -- record_debug_memory failed on
every call in the live run. ModuleRunConfig now carries a `state_slice`
(task_spec, run_id, upstream_commit) that the step populates and the
bridge forwards.
Both pinned by a test asserting build_backends receives the ROOT path
while the NAME lands in state.task_spec.repo.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`dispatch` traces a failed call with ok=False but not the error, so
bridge_trace.jsonl could not distinguish "the agent read a path that does
not exist" from "this tool is broken". Both look like
`{"tool": "read_file", "ok": false}`, and telling them apart meant
reproducing the call by hand -- which is how the repo-root bug was found,
twice over.
Failed calls now emit a `tool_error` event carrying the message (capped at
500 chars). Pinned by a test asserting the reason reaches the trace.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A harness that keeps its own built-in file tools edits the checkout without touching the bridge, so those writes miss BOTH guarantees the bridge provides: the scope guard's out-of-scope recording and the plan gate. This is not hypothetical -- one live module session made 9 native writes against 8 bridged product writes for the whole run, and the plan gate, enforced at dispatch, never saw them. Prevention needs a sandbox, which needs user namespaces (/proc/sys/user/max_user_namespaces is 0 on this host, the same condition that leaves sandboxed codex without a shell). Of the shipped harnesses only claude-code declares `builtin_tools_off`. Where neither is available, detection is the honest ceiling. After each harness session the module step now diffs the checkout against the bridged writes recorded in bridge_trace.jsonl: - every file changed without a matching bridged call is a NATIVE write and is recorded as `harness_native_writes`; - a native write whose mtime precedes `plan_gate_opened` changed product code before a decision existed -- the gate's whole contract -- and FAILS the module, so the wave gate cannot accept work the contract never covered. Files already dirty before the session (unchanged mtime) are not attributed to the harness. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`_write_mcp_config` refused to touch any existing `.cursor/mcp.json` so a repo-committed config is never clobbered. That guard cannot distinguish a committed config from OUR OWN orphan: the transport removes the file in a `finally`, which a killed session skips, so the file survives pointing at the spec of a run that is over. The next session then honours it and is silently bound to a DEAD run: - the ToolScope is the previous MODULE's, so the scope guard protects the wrong paths; - the plan-gate prefix is the old module's plan dir, so the decision file the agent writes can never open the gate; - tool events append to the OLD run's bridge_trace.jsonl, so the live run looks like it is making no bridged calls at all. Observed on 2026-09-18: a `worker_runner` session ran 27 minutes under `online_serving`'s spec from a run killed the previous day, while the dead run's trace kept growing. An existing config is now replaced only when it launches our own tool bridge; anything else is still left untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
check_spec_freshness --strict flagged 5 pages STALE: config, rebase_engine, engine/steps/rebase_v3, providers/cursor and tool_bridge all changed after they were last verified. Records the contracts rather than only bumping dates, including the three faults that only a live harness run exposed: build_backends(repo=) takes a repo ROOT PATH and not a name (a name makes the test tools fail closed while the module still reports done); the tier model must never be forwarded to a harness CLI; and generated MCP tool functions need keyword-only params with unset optionals dropped. Also records that cursor's registry capabilities lack builtin_tools_off, so its native tools bypass the bridge and are only RECORDED, not contained. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The rebase module agents ran only on the in-process Anthropic tool-use loop. Everything else in the copilot already goes through the provider abstraction — cursor-agent, claude-code, codex. The rebase pipeline was the exception, so it could not use a harness model at all.
This makes the rebase use the abstracted backend like the rest of the copilot.
Change
rebase_backend(defaultapi) selects who runs the module agents:apifor the in-process loop, or a harness provider id —cursor,claude-code,codex.rebase_backend_modelis the model id inside the harness, e.g.cursor-grok-4.6-high-fast.apileaves the existing in-process path untouched.Faults this found by being run, not by being tested
These are in the commit history because a live harness run exposed them and unit tests did not:
build_backends(repo=)takes a repo ROOT PATH, not a repo name. Passing the name maderun_pytest/run_precommit/reproducefail withFileNotFoundError('vllm-omni')— so modules "completed" with no test ever running, visible only as an empty pre-commit log. This is the dangerous one: silent false-green.model or strict_backend_model, so a raw-API tier name likedeepseek-flashwould override cursor's own selection.*for zero-property tools, and must drop unset optionals — passingNonebroke everyread_file.stateslice orrepo_nameis empty andrecord_debug_memoryfails every call.Known limitation, documented not fixed
Cursor's registry capabilities are
{mcp_tools, usage_reporting}— nobuiltin_tools_off(claude-code has it). So cursor's built-in tools bypass the bridge: bridged calls are contained by scope, native ones are only RECORDED. A live run logged 10 out-of-root reads with 0 bridge refusals. Recorded inproviders/cursorspec.Verification
test_rebase_harness_backend.pythis branch adds.check_spec_freshness --strict: 0 stale.Authorship
The 7 implementation commits are prior work from another session on this account; I added the spec re-verification and opened the PR. Commit authorship is unchanged.
🤖 Generated with Claude Code