feat(dual-solve): run agents in docker sandbox - #272
Merged
Conversation
each engine run is multi-minute and fully captured, so the operator watched a blank terminal with no signal of which engine was active. thread an optional on_progress callback through prepare() and wire the cli to echo it to stderr: a line per phase -- fetch, ground, and each engine's run with elapsed time and diff size. defaults to none so existing behaviour and tests are unchanged, and stderr keeps stdout and --json output clean.
a single-page app, shipped inside the review-ui, that takes a github issue link, runs dual-solve against the repo the server lives in, streams progress over the existing websocket, shows both engines' diffs side by side, and lets a human pick the winner -- keeping the branch and proposing the rationale into the kb through the same review gate. captures the decisions made during brainstorming: shipped feature in src/vouch/web, fixed target repo, buildless vue 3, full pick semantics, and an executing http surface that stays gated (off by default, edit-only, bearer-token guarded) and requires a vep before merge.
a bite-sized, tdd task plan turning the design spec into the shipped feature: vep, the gate + plumbing, the run/job/progress-bridge backend, the choose endpoint that finalizes through the review gate, the buildless vue spa, and the changelog. carries the dual-solve constraints forward (import-as-used, ascii-coerced kb text, .venv tooling, edit-only over http).
the vep documents the new HTTP routes (/dual-solve, /dual-solve/run,
/dual-solve/job/{id}, /dual-solve/choose), the --allow-dual-solve gate,
the edit-only autonomy constraint, and the security argument that the
review-gate invariant is preserved (web finalize only proposes, never
auto-approves).
mount the /dual-solve shell route only when the server is started with --allow-dual-solve. when the flag is absent, register() returns immediately and the route does not exist (404). this is the security gate: the dual-solve runner spawns external processes (claude+codex) so it must be an explicit opt-in. changes: - src/vouch/web/dual_solve_api.py: new register() function; calls ds.repo_root() at app-build time to fail fast if not in a git repo. - src/vouch/web/templates/dual_solve.html: spa shell template with the #dual-solve-app vue mount point. - src/vouch/web/server.py: build_app gains allow_dual_solve=False; _tmpl injects dual_solve_enabled; _register_dual_solve called before return app. - src/vouch/web/__init__.py: create_app gains allow_dual_solve=False and passes it to build_app. - src/vouch/web/templates/base.html: conditional nav link for dual-solve. - src/vouch/cli.py: review-ui gains --allow-dual-solve flag; passed to create_app. - tests/test_web_dual_solve.py: two tests — enabled renders 200 with the vue mount point; disabled returns 404.
add DualSolveJob dataclass, _serialize helper, POST /dual-solve/run (201),
GET /dual-solve/job/{id}, and the sync→async progress bridge.
the job is created synchronously in the route handler before the
asyncio.create_task fires, so the single-flight 409 check is reliable.
the on_progress callback runs on the worker thread and bridges to the hub
via run_coroutine_threadsafe, capturing the event loop in the handler (not
the worker). autonomy is hard-forced to "edit" regardless of what the caller
sends.
deviation from spec: the single-flight guard uses `status not in
("done", "error")` rather than `status in ("running", "finalizing")`.
this is required because in the starlette testclient's per-request portal
environment, the background task (run_in_threadpool → anyio thread) completes
during portal teardown before the second post arrives, leaving status as
"ready". the broader guard matches the semantic intent (reject while a job is
still active/awaiting-decision).
the previous guard rejected new runs whenever a prior job was in any
non-terminal state, including "ready". a ready job the operator abandons
(closes the tab, never chooses) would block every future run forever and
make the stale-worktree cleanup path unreachable — a permanent leak.
corrected to 409 only on ("running", "finalizing"). ready/done/error jobs
are replaced by a new run, which first runs ds.cleanup on their candidates.
test_run_is_single_flight is rewritten to set the precondition directly
(no timing dependency on when the background task completes). a new test,
test_run_replaces_abandoned_ready_job_and_cleans_up, locks in the
replace-and-cleanup behavior for the ready case.
adds POST /dual-solve/choose which takes {job_id, winner, reason},
looks up the candidate matching the winner engine on the in-memory job,
calls ds.finalize with record=True and proposed_by=reviewer() so the
rationale lands in the kb review queue, then marks the job done and
broadcasts a "done" ws frame.
winner=null skips finalize entirely and returns empty proposed_ids.
guards: 404 when job_id doesn't match the active job, 409 when the
job is not in "ready" status, 500 on ValueError/RuntimeError from
finalize. the review gate is preserved — finalize only ever calls
proposals.propose_claim; no approve/durable-write is added here.
…s, constrain winner rename test_choose_before_ready_is_conflict to test_choose_unknown_job_is_not_found to match what it actually tests (404 on bogus job_id, not the 409 guard). add test_choose_when_not_ready_is_conflict that exercises the real 409 path by pre-planting a job in status="finalizing" and asserting the guard fires. add an error broadcast in the choose handler's except block so clients see the error frame before the 500 is raised, matching _run_job's error path. tighten _ChooseReq.winner to Literal["claude","codex"]|None so an unrecognised engine name is rejected with 422 rather than silently treated as "keep neither". also enter testclient as a context manager in _client() so the blocking portal stays alive across all requests within a test; without this asyncio.create_task background jobs are cancelled when the per-request ephemeral portal closes, which caused test_choose_winner_finalizes_and_returns_ids and test_choose_neither to fail intermittently depending on test ordering.
vendor vue 3.4.38 (esm-browser.prod build, 150 kb) under
src/vouch/web/static/vendor/ with sha256 recorded in VENDOR.md.
add dual_solve.js — the full vue 3 spa: runform, websocket progress
log, side-by-side diff panes (with a minimal unified-diff parser), and
a choice bar that posts to /dual-solve/choose.
add dual_solve.css — two-column pane grid with diff line colouring.
extend base.html with a {% block head %} slot so per-page stylesheets
can be injected without touching the shared layout. dual_solve.html
uses it to pull in dual_solve.css.
add test_spa_assets_are_served to tests/test_web_dual_solve.py:
asserts the js, css, and vendor vue files all return 200, and that
the rendered page references both dual_solve.js and createApp.
no npm, no bundler, no build step — the esm-browser build includes the
template compiler so template: strings work at runtime.
note the new `review-ui --allow-dual-solve` browser surface under [unreleased]/added, pointing at vep-0006. the pick keeps the branch and proposes through the existing review gate; nothing auto-approves.
…recondition from the final whole-branch review: assert the executing run/choose routes 404 when --allow-dual-solve is off (not just the page), so a future refactor cannot silently expose them; and document why a prepare failure leaks no worktrees today.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
adds an opt-in docker sandbox path for dual-solve agent execution.
claude and codex can now run inside a docker image, defaulting to
amika/coder:latest, while git and gh remain on the host. the sandbox runner mounts only the candidate worktree and a temporary copied home containing known claude/codex credential files, so agent writes stay confined to the throwaway dual-solve branches and host auth files are not mutated.also wires the same sandbox option into the review-ui dual-solve surface with
--dual-solve-sandbox.verified:
.venv/bin/python -m pytest tests/test_sandbox.py tests/test_dual_solve.py tests/test_web_dual_solve.py -q.venv/bin/python -m ruff check src tests.venv/bin/python -m mypy src.venv/bin/python -m pytest tests/ -q --ignore=tests/embeddingsclaude --versionandcodex --versionthroughDockerAgentRunneragainstamika/coder:latest