fix(review): rescope normal sandbox runtime #67
No reviewers
Labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Slipstream/auto_review!67
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "issue-46-sandbox-rescope"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
sandbox: "not-used", ignores legacyAR_SANDBOX_IMAGE, and documents remaining workspace access paths.Verification
nix develop -c cargo fmt --all -- --checknix develop -c cargo clippy --workspace --all-targets -- -D warningsnix develop -c cargo nextest run -p ar-review git_helper_ignores hermetic_git_command_removesnix develop -c cargo nextest run -p ar-gateway normal_review_runtimenix develop -c env -u FORGEJO_BASE_URL -u AR_FORGEJO_TOKEN -u FORGEJO_TOKEN -u LLM_REASONING_MODEL -u LLM_API_KEY -u LLM_BASE_URL cargo nextest run --workspace --no-tests=passCloses #46.
This PR addresses issue #46 by rescoping the normal sandbox runtime, removing the need for a sandbox image in the normal review runtime, and hardening Git operations. The changes appear well-documented and tested, making the PR safe to merge.
Walkthrough
I don't think we need the leftover sandbox stuff in here at all. We can add it back easily enough if that changes. We really shouldn't need to run any potentially unsafe things in the review aside from git; anything else should be executed in a CI runner.
And also, in deployed usage we should be running the entire review process in some type of sandbox like a docker container or other isolation that wraps the entire ar-gateway. Maybe we should set that up as part of this ticket? I think we already basically have a Docker image defined, right? But we should use Nix to create the docker image so that it is as lightweight and secure as we can possibly get it. Our CI build could create the image via nix and publish the image in our git.johnwilger.com package repo.
@ -249,1 +249,4 @@No sandbox image env var is required for the normal gateway/orchestrator reviewruntime. Deterministic linters, tests, and builds run in CI before the semanticreview trigger; `GET /info` reports `sandbox: "not-used"`.Why not just remove the concept of "sandbox" entirely here? What is the benefit of keeping it and just saying it isn't used?
Reflection: I kept the
sandboxfield as a compatibility placeholder instead of following the rescope all the way through the public surface. Classification: one-off YAGNI cleanup, not a new guardrail gap. Remediation: removed the normal-runtime sandbox field from gateway/infoand CLI status, removed stale chat sandbox wording, and removed the retired linter/sandbox crates from the active workspace. Verification includes focused gateway/CLI tests, workspace nextest with ambient env cleared, clippy, deny, and Nix image build.@ -82,2 +82,2 @@| `ar-tools` | Legacy static-analysis runners retained while runtime linter execution is retired || `ar-sandbox` | Sandbox abstraction retained pending issue #46 rescope || `ar-tools` | Legacy static-analysis runners retained outside normal review runtime || `ar-sandbox` | Sandbox abstraction retained for future execution features and tests |YAGNI
Reflection: I documented a future-facing sandbox abstraction instead of deleting the unused surface. Classification: one-off YAGNI cleanup. Remediation: removed
ar-sandboxand the legacyar-toolsrunners from the workspace, updated crate tables/docs to stop advertising them, and rewrote ADR-0002 as historical context only.@ -5,3 +3,1 @@linters and no longer wire this crate through the gateway. The originallinter-execution use case is preserved as historical context inADR-0002.Sandbox abstraction retained for future features that explicitly need anYAGNI
Reflection: I preserved the crate as speculative future work instead of forcing any future execution feature to reintroduce only what it needs. Classification: one-off YAGNI cleanup. Remediation: deleted the
ar-sandboxcrate and updated the threat model/ADR/runbooks so no live sandbox crate or escape harness is advertised for normal review runtime.Addressed the feedback locally:
sandboxpublic surface from gateway/info, CLI status, and chat docs/metadata.ar-toolsandar-sandboxcrates from the active workspace instead of retaining speculative execution abstractions..#ar-gateway-imageandnix run .#dev-gateway-containerwatcher that rebuilds, loads, and relaunches the deploy-shaped container locally.Verification run locally: fmt, clippy, nextest with ambient env cleared, cargo deny licenses/bans/sources, Nix flake eval, and
nix build .#ar-gateway-image.New commits pushed, approval review dismissed automatically according to repository settings
This PR removes the
ar-toolsandar-sandboxcomponents, rescoping the normal sandbox runtime and updating documentation to reflect these changes. It introduces a hermetic Git command to enhance security and reliability. The changes appear well-structured, but ensure thorough testing and documentation updates.Walkthrough
nix run .#dev-gateway-containerfor development, removing references toar-toolsandar-sandbox.ar-toolsandar-sandbox, indicating a shift in project architecture.ar-toolsandar-sandboxremoved from the project, reflecting their deprecation.ar-toolsandar-sandbox, ensuring consistency in documentation.sandboxfield fromGatewayInfo, indicating a change in sandbox handling.hermetic_git_commandto isolate Git command execution from the environment, with new tests to ensure reliability.New commits pushed, approval review dismissed automatically according to repository settings
This PR removes the
ar-toolsandar-sandboxcrates, rescoping the sandbox runtime to exclude legacy linter execution. It also hardens Git operations by using a hermetic command wrapper. The changes appear safe to merge, but ensure all dependencies on the removed crates are updated.Walkthrough
ar-toolsandar-sandboxfrom the workspace, reflecting the removal of these crates.