Skip to content

fix: make strict replay sandbox survive docker-compose service starts#228

Merged
jy-tan merged 5 commits intomainfrom
fix-docker
Apr 24, 2026
Merged

fix: make strict replay sandbox survive docker-compose service starts#228
jy-tan merged 5 commits intomainfrom
fix-docker

Conversation

@jy-tan
Copy link
Copy Markdown
Contributor

@jy-tan jy-tan commented Apr 24, 2026

Summary

Consume fence's new SetService / ExposeHostPath API to unblock strict-sandbox runs whose service start command is docker compose up (or any daemon-delegated command).

Companion to Use-Tusk/fence#139.

Changes

  • internal/runner/service.go: swap SetExposedPorts -> SetService(ServiceOptions{..., ExecutionModel: ...}); set ServiceBindsOnHost when the start command is docker/podman-shaped, so fence skips the reverse socat bridge that would otherwise collide with dockerd's host-port bind. Register the replay compose env-override file via ExposeHostPath, making it visible inside the sandbox despite fence's --tmpfs /tmp.
  • internal/runner/server.go + server_test.go: rename isDockerCommand -> serviceDelegatesToHostDaemon with a docstring naming its two call sites (communication-type auto-detection + fence execution model). No behavior change; removes the duplication implied by two unnamed usages of the same predicate.
  • internal/runner/compose_replay.go: keep os.CreateTemp("", ...) (i.e. /tmp) and add a comment pointing at the ExposeHostPath call site - callers no longer need to route around fence's mount plan.

Copy link
Copy Markdown
Contributor

@tusk-dev tusk-dev Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 2 issues

Comment thread go.mod Outdated
Comment thread internal/runner/service.go Outdated
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 6 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="go.mod">

<violation number="1" location="go.mod:128">
P1: Committed local `replace` directive points to `../fence`, which will break builds outside your local workspace.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.

Comment thread go.mod Outdated
@tusk-dev
Copy link
Copy Markdown
Contributor

tusk-dev Bot commented Apr 24, 2026

Tip

New to Tusk? Learn more here.

Code Review

Tusk Review: No issues found


Unit Tests

Generated 5 tests - 5 passed

Commit tests View tests

Test Summary

  • serviceDelegatesToHostDaemon - 1 ✓
  • StartService - 4 ✓

Results

Tusk's tests all pass. The test suite validates the core changes: serviceDelegatesToHostDaemon correctly identifies docker/podman commands with edge case handling (case insensitivity, whitespace, env vars), and StartService properly handles the new ExposeHostPath logic across multiple scenarios—non-sandboxed overrides, platform unsupport errors, graceful degradation when fence is unavailable, and docker commands without sandboxing. These tests cover the critical paths: the refactored daemon detection predicate that gates both communication-type auto-detection and fence execution model configuration, plus the service startup flow with the new sandbox-aware override file exposure. The changes integrate fence's new API cleanly without breaking existing behavior.

Avg +48% line coverage gain across 2 files
Source file Line Branch
internal/runner/server.go 31% (+31%) 0%
internal/runner/service.go 65% (+65%) 0%

Coverage is calculated by running tests directly associated with each source file, learn more here.

Last run on 60efd38. Comment @use-tusk generate to create a test suite on the latest commit. Configure here.

View check history

Commit Unit Tests Created (UTC)
f8fd9f6 No tests generated · Output Apr 24, 2026 9:10AM
60efd38 5 ✓, 0 ✗ · Tests Apr 24, 2026 8:32PM

Was Tusk helpful? Give feedback by reacting with 👍 or 👎

@jy-tan jy-tan requested a review from sohil-kshirsagar April 24, 2026 20:57
Copy link
Copy Markdown
Contributor

@tusk-dev tusk-dev Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 1 issue

Comment thread internal/runner/service.go
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 8 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="internal/runner/sandbox_unix.go">

<violation number="1" location="internal/runner/sandbox_unix.go:41">
P1: The docstring promises error-path cleanup but none of the error returns after `fence.NewManager()` call `mgr.Cleanup()`. If `ExposeHostPath` or `Initialize` fails, any resources already allocated by the manager will leak. Add a deferred cleanup that is cancelled on success.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.

Comment thread internal/runner/sandbox_unix.go
Copy link
Copy Markdown
Contributor

@tusk-dev tusk-dev Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 1 issue

Comment thread internal/runner/service.go
@jy-tan jy-tan merged commit aa6257e into main Apr 24, 2026
14 checks passed
@jy-tan jy-tan deleted the fix-docker branch April 24, 2026 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants