Skip to content

feat(projects): ship bind-mounted files on a remote deploy - #80

Merged
malickyeu merged 7 commits into
mainfrom
feat/remote-project-binds
Jul 30, 2026
Merged

feat(projects): ship bind-mounted files on a remote deploy#80
malickyeu merged 7 commits into
mainfrom
feat/remote-project-binds

Conversation

@malickyeu

Copy link
Copy Markdown
Contributor

Summary

Lifts the "named-volumes-only" limit that Remote Projects shipped with in v1.5.
A remote daemon can't see files in Docker Commander's data dir, so a compose file
with a host-path bind mount was refused outright. Now each bind whose source
lives inside the project folder is copied into a dcseed-<project>-<hash>
volume on the target host and the mount is repointed at it, so sidecar configs
and scripts work remotely — including single-file mounts like
./nginx.conf:/etc/nginx/nginx.conf.

Mechanism: the existing volume-browser helper container + Docker copy API (works
over TCP and SSH alike, no CLI on the remote needed), plus a generated JSON
compose override — JSON is valid YAML, so this adds no dependency.

Binds pointing outside the project folder are still refused, now naming the
offending mounts: they address paths on the remote host and won't be mounted
blind.

A correction to the roadmap

NEXT.md framed this as "deploy via compose-go so bind mounts work on remote
hosts". Two things there were wrong and NEXT.md is updated:

  • compose-go cannot deploy — it is strictly a parser/loader. The only
    library that deploys is github.com/docker/compose (now v5), and it's
    heavy: 114 → 409 modules, and a hello-world that merely constructs the
    service is 24.9 MB vs. the current whole-app binary's 26.8 MB.
  • Bind mounts were never a CLI-vs-library problem — a bind resolves on the
    daemon's filesystem whatever the client is. Shipping the files is the fix,
    which is what this PR does. The in-process-engine idea is now a separate,
    cost-flagged roadmap item.

Type of change

  • Bug fix
  • New feature
  • Docs only
  • Refactor / chore

Checklist

  • go test -short ./... and go vet ./... pass
  • gofmt gate is clean (gofmt -l $(git ls-files '*.go') after staging)
  • Frontend type-checks (cd web && npx tsc --noEmit)
  • Rebuilt and committed web/dist
  • Added/updated tests for the change
  • Updated docs/ and added a CHANGELOG.md entry for user-facing changes

Notes for reviewers

31 new tests, 7 of them pentests. Worth reviewing in this order:

  • compose_binds_smoke_test.go drives the real compose CLI to pin the three
    behaviours the rewrite depends on: volumes merge by container target (so
    the override replaces the bind instead of adding a mount), external: true
    keeps the name unprefixed (a prefixed name would point at an unseeded
    volume), and volume.subpath mounts a single file. If compose ever changes
    those semantics this fails loudly rather than deploying binds still pointing at
    local paths. It also asserts nothing is left as type: bind.
  • compose_binds_integration_test.go drives a real daemon: directory
    contents land at the volume root (not nested), nesting is preserved, a single
    file is stored under its base name, the seed labels are set, and a re-seed
    (redeploy) is idempotent and ships current contents. It creates and removes
    only its own volumes — no host-global prune (per the NEXT.md gotcha); I
    verified no dcseed-* volumes or dc.volfs helpers were left behind.
  • Pentests cover the containment check, which is the security-critical part since
    a compose file is user input that decides which paths we read and ship:
    relative traversal, absolute paths (/etc/shadow, /var/run/docker.sock),
    symlink escape, a symlinked parent component, sibling dirs sharing a name
    prefix (…/projects/7-evil vs …/projects/7), and an empty source. Tar
    entries store symlinks as links rather than following them, and drop
    uid/gid/uname/gname since those mean something else on the target host.

Trade-offs / deliberate choices:

  • Outside-the-project binds are refused, not passed through. Keeps v1.5's
    fail-closed posture; letting a remote deploy mount an explicitly confirmed
    remote path is filed in NEXT.md instead.
  • Snapshot, not live sync. Surfaced in the deploy output, the Projects page
    blurb and docs/projects.md. A local deploy still mounts the folder directly.
  • Seeded volumes survive a down, like any named volume, and aren't reclaimed
    when a project is deleted yet — noted in NEXT.md.
  • ComposeBindMounts is removed (superseded by ClassifyProjectBinds), along
    with its two tests.
  • Also fixed an adjacent staleness: the Projects page still claimed projects
    deploy to the local host only, untrue since v1.5.

Not covered by automated tests: the full remote round-trip needs a second
daemon, so seeding is exercised against the local daemon and the override
against the real CLI, but no test deploys to an actual TCP/SSH host.

A remote daemon can't see Docker Commander's data dir, so v1.5 refused any
compose file with a host-path bind mount. Each bind whose source lives
inside the project folder is now copied into a dcseed-<project>-<hash>
volume on the target host — reusing the volume-browser helper container and
the Docker copy API, so it works over TCP and SSH alike — and repointed by
a generated JSON compose override (JSON is valid YAML, so no new dep).

This rests on three compose behaviours, each covered by a smoke test that
drives the real CLI: service volumes merge by container target (so the
override replaces the bind rather than adding a mount), external:true keeps
the volume name unprefixed, and volume.subpath handles single-file mounts.

Binds pointing outside the project folder stay refused — they name paths on
the remote host. The containment check canonicalises symlinks, including in
parent components; pentests cover relative traversal, symlink escape,
symlinked parents and sibling name prefixes.

The copy is a snapshot, not a live mount, which the deploy output now says.
Drops ComposeBindMounts, superseded by ClassifyProjectBinds.
Copilot AI review requested due to automatic review settings July 30, 2026 08:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Not ready to approve

Two verified issues remain: remote deploy “note” is not shown for deploys triggered from the project editor UI, and missing-but-inside bind sources currently fail during TAR creation instead of seeding an empty volume as documented/tested.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Adds support for Remote Projects that use bind mounts by seeding bind-mounted content (when the bind source is inside the project folder) into deterministic named volumes on the remote host, then deploying with a generated JSON compose override that repoints mounts at those volumes. This extends the existing Remote Projects feature beyond named volumes while keeping a fail-closed posture for binds that would reference remote-host paths.

Changes:

  • Introduces bind classification, deterministic dcseed-<project>-<hash> volume naming, TAR-based seeding via the existing volume-helper container, and JSON compose overrides for remote deploys.
  • Updates the deploy handler to seed internal binds, refuse external binds with a detailed message, and pass -f overrides to docker compose.
  • Updates UI/docs/changelog to explain snapshot semantics and surface an optional deploy note in output.
File summaries
File Description
web/src/pages/Projects.tsx Prepends remote-deploy “note” to output and updates Projects page help text (but one deploy path still misses the note).
web/src/lib/api.ts Extends deployProject response type to include optional note.
web/dist/index.html Rebuilt frontend bundle references.
web/dist/assets/index-chcsqF7b.css New built CSS asset from rebuild.
web/dist/assets/index-Cg7DAdd6.css Removes old built CSS asset.
web/dist/assets/CodeEditor-BOBghuRJ.js Rebuilt frontend JS asset referencing new main bundle hash.
README.md Updates feature list to mention remote bind-mount seeding behavior.
NEXT.md Updates roadmap to reflect seeding-based fix vs. “compose-go deploy” framing.
internal/docker/compose.go Adds ComposeUpFiles and runComposeFiles to support explicit -f layering.
internal/docker/compose_host.go Removes the old bind-mount detector helper (superseded).
internal/docker/compose_host_test.go Removes tests for the deleted bind-mount detector.
internal/docker/compose_binds.go New core implementation for classifying binds, seeding volumes, generating overrides, and TAR creation.
internal/docker/compose_binds_test.go Unit tests + pentests for containment/symlink escape and TAR behavior.
internal/docker/compose_binds_smoke_test.go Smoke test driving the real docker compose CLI to pin required merge/subpath/external semantics.
internal/docker/compose_binds_integration_test.go Integration test driving a real daemon to verify seeding layout/labels/idempotent updates.
internal/api/project_handlers.go Remote deploy path now seeds internal binds, refuses external binds, and returns a note to the UI.
internal/api/project_binds_test.go Tests for bind message formatting, note content, and project deploy env behavior.
internal/api/mcp_projects.go Updates MCP project operations to the new compose env helper signature.
docs/projects.md Rewrites Remote Projects bind-mount docs to explain seeding + snapshot semantics and refusal of external binds.
CHANGELOG.md Adds an Unreleased entry describing remote bind mount support and constraints.
Review details
  • Files reviewed: 16/21 changed files
  • Comments generated: 2
  • Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Comment thread web/src/pages/Projects.tsx Outdated
Comment on lines +195 to +199
const body = r.output || r.error || "(no output)";
// A remote deploy copies bind-mounted paths instead of mounting them live,
// which the user has to know about — lead with it, then the compose output.
const note = "note" in r && r.note ? `${r.note}\n\n` : "";
setOutput({ title: `${p.name} — ${kind}`, text: note + body, ok: r.ok });

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Confirmed and fixed in f0d0855. There were indeed two runCompose handlers building the output independently, and the editor one dropped the note.

Rather than patching the second copy, the formatting moved to web/src/lib/composeOutput.ts (composeOutputText), now used by both call sites and covered by unit tests — so a third caller cannot silently diverge the same way.

Comment on lines +293 to +296
return filepath.Walk(src, func(p string, fi os.FileInfo, err error) error {
if err != nil {
return err
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Confirmed as a real bug and fixed in f0d0855. Reproduced first: SeedProjectBinds failed with lstat …/notyet: no such file or directory, so a compose file naming a not-yet-created bind source (a database's ./data, for instance) would have failed the entire remote deploy.

writeTar now Lstats the source and emits an empty archive when it does not exist, matching what a local bind mount gets (Docker materialises the path on demand). The gap was exactly as you describe: the classification of that case was tested, the seeding of it was not. Both are now covered — TestTarPath_MissingSourceYieldsEmptyArchive for the archive and TestSeedProjectBinds_MissingSourceSeedsEmptyVolume against a real daemon.

Covers the round trip the earlier tests couldn't: a project deployed to a
daemon that genuinely can't see the project folder. Gated on
DC_REMOTE_DOCKER (a docker:dind sidecar works), asserting no mount is left
as a bind, the container actually runs, and both files are readable inside
it on the remote host.

Verified the failure it prevents: without seeding the remote daemon
materialises each missing bind source as a *directory*, so a single-file
config mount makes nginx exit with "Is a directory" — the container doesn't
start at all.

Also fixes teardown in both integration tests, which leaked volumes onto the
daemon: `defer st.Close()` runs before t.Cleanup callbacks, so the teardown
had no store left to resolve a Docker client from. Registering the store and
env teardown via t.Cleanup fixes the order, and volume removal now retries
and reports instead of discarding the error.
Copilot AI review requested due to automatic review settings July 30, 2026 09:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Not ready to approve

The deploy API currently omits the computed note on the error response path, leading to inconsistent user-facing messaging for remote deploy failures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Comments suppressed due to low confidence (2)

internal/docker/compose_binds.go:266

  • SeedProjectBinds may redundantly re-seed the same source path multiple times when the compose config mounts the same project path into multiple services/targets. Since the seeded volume name is derived from (slug, Rel), you can deduplicate by seeded volume name to avoid repeated TAR uploads and volume-helper churn.
		if _, err := cli.VolumeCreate(ctx, volume.CreateOptions{
			Name: name,
			Labels: map[string]string{
				seedVolLabel: slug,
				seedRelLabel: filepath.ToSlash(b.Rel),

internal/api/project_handlers.go:642

  • The deploy response drops note on the error path, so a remote deploy that already seeded bind-mounted files but then fails in docker compose up won’t show the snapshot/copy warning in the UI. Since note is computed before ComposeUpFiles, it should be returned consistently on both success and failure.
	out, err := docker.ComposeUpFiles(r.Context(), dir, p.Slug, body.Profiles, env, files)
	if err != nil {
		writeJSON(w, http.StatusOK, map[string]any{"ok": false, "error": err.Error(), "output": out})
		return
  • Files reviewed: 17/22 changed files
  • Comments generated: 0 new
  • Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

The remote e2e test now takes its host kind from the DC_REMOTE_DOCKER
scheme, so the same test drives both remote transports; for ssh it pins the
live host key through the real trust flow (probeSSHHostKey + SetHostKey).
scripts/remote-test-daemon.sh provisions a dind sidecar serving TCP and SSH
and prints the invocations. It never reads or writes ~/.ssh.

Documents two real SSH-host requirements found while getting this to pass:

- The remote sshd must allow forwarding: the SDK tunnels the daemon socket
  over a direct-streamlocal channel, which sshd gates on AllowTcpForwarding.
  Alpine ships "no", and sshd honours the FIRST occurrence of a keyword, so
  appending to sshd_config is silently ignored. Confirmed by flipping it and
  re-running. This yields a half-working host, since `docker compose` uses
  dial-stdio and needs no forwarding — deploys succeed while monitoring
  fails.
- An agent holding several keys can exhaust sshd's MaxAuthTries before the
  right key is offered.

Harness notes recorded in NEXT.md: `go test` caches results and the env var
doesn't invalidate the cache (hence -count=1), and OpenSSH takes ~ from the
passwd database rather than $HOME, so scratch credentials need an ssh shim.
Copilot AI review requested due to automatic review settings July 30, 2026 09:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Not ready to approve

The remote deploy path introduces correctness risks (compose config resolved with discovery vs deployed with explicit -f list, and reseeding doesn’t remove deleted files from seeded volumes) that should be addressed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Comments suppressed due to low confidence (3)

internal/api/project_handlers.go:1056

  • Remote deploy resolves the compose config via ComposeConfigJSON (no explicit -f, so it uses the CLI’s file discovery), but then deploys with an explicit file list (p.ComposeFile + generated override). If the discovered config differs from that explicit list (e.g. additional compose files / overrides present), the bind classification/override can drift from what actually gets deployed.
	// Fail closed: without a resolved config we can't prove which paths this
	// project would mount on the remote host, so don't deploy at all.
	cfgJSON, err := docker.ComposeConfigJSON(ctx, dir, p.Slug)
	if err != nil {
		return nil, nil, "", noop, fmt.Errorf("cannot validate the compose file for remote deploy: %v", err)
	}
	internal, external, err := docker.ClassifyProjectBinds(cfgJSON, dir)
	if err != nil {

internal/docker/compose.go:58

  • Grammar: “Empty files keeps …” should be “Empty files keep …” (plural subject).
// Empty files keeps the CLI's own file discovery.

internal/docker/compose_binds.go:260

  • SeedProjectBinds claims reseeding “ships current files”, but it only overlays a tar extract onto the existing volume. There’s no pre-clean step, so files deleted from the project won’t be removed from the seeded volume and can linger as stale config/scripts on redeploy.
// SeedProjectBinds creates and fills the volume behind each internal bind on the
// target host: the local files are streamed in as a TAR through a helper
// container. Existing content is overwritten so a redeploy ships current files.
func (m *Manager) SeedProjectBinds(ctx context.Context, hostID int64, projectDir, slug string, binds []ProjectBind) error {
	cli, err := m.Client(ctx, hostID)
	if err != nil {
		return err
	}
	for _, b := range binds {
		name := SeedVolumeName(slug, b.Rel)
		if _, err := cli.VolumeCreate(ctx, volume.CreateOptions{
			Name: name,
			Labels: map[string]string{
				seedVolLabel: slug,
				seedRelLabel: filepath.ToSlash(b.Rel),
			},
		}); err != nil {
			return fmt.Errorf("create seed volume for %s: %w", b.Rel, err)
		}
		src := filepath.Join(projectDir, b.Rel)
		tarball, err := tarPath(src, b.IsFile)
		if err != nil {
			return fmt.Errorf("archive %s: %w", b.Rel, err)
		}
		if err := m.VolumeCopyTo(ctx, hostID, name, "/", tarball); err != nil {
			return fmt.Errorf("seed volume for %s: %w", b.Rel, err)
		}
  • Files reviewed: 19/24 changed files
  • Comments generated: 0 new
  • Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Hosts never talk to each other here — each is reached independently — so
what's worth proving is that per-host state doesn't bleed:

- retarget: deploy to A, repoint at B, redeploy. B serves the edited files
  from its own seeds, A is undisturbed.
- concurrent mixed transports: two projects to two hosts at once, one over
  SSH and one over TCP, neither picking up the other's content.
- two projects, same relative bind path, one host: distinct volumes, each
  serving its own file.

The retarget test also pins down a pre-existing gap it confirmed: changing a
deployed project's host leaves the stack running on the old host along with
its seed volumes, so there are two live copies while the UI shows only the
new host. Asserted so it can't change unnoticed, and written up in
docs/projects.md and NEXT.md with the two possible fixes.

remote-test-daemon.sh now provisions up to three sidecars (`up [count]`),
exporting host 1 over SSH and host 2 over TCP so the fleet is mixed by
default. All of these skip cleanly when the fleet env isn't set.
Copilot AI review requested due to automatic review settings July 30, 2026 09:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Not ready to approve

Remote seeding currently doesn’t clear previously-seeded volume contents, so deleted project files can persist remotely across redeploys (stale snapshot behavior).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Comments suppressed due to low confidence (2)

internal/docker/compose_binds.go:261

  • SeedProjectBinds currently re-seeds by calling VolumeCopyTo, which overwrites files but does not remove files that were deleted from the project directory. That means a redeploy can leave stale files in the seeded volume (and thus in containers) that no longer exist locally. Consider clearing the seed volume root before copying so the volume is a true snapshot of current contents.
	for _, b := range binds {
		name := SeedVolumeName(slug, b.Rel)
		if _, err := cli.VolumeCreate(ctx, volume.CreateOptions{
			Name: name,
			Labels: map[string]string{
				seedVolLabel: slug,
				seedRelLabel: filepath.ToSlash(b.Rel),
			},
		}); err != nil {
			return fmt.Errorf("create seed volume for %s: %w", b.Rel, err)
		}
		src := filepath.Join(projectDir, b.Rel)
		tarball, err := tarPath(src, b.IsFile)
		if err != nil {
			return fmt.Errorf("archive %s: %w", b.Rel, err)
		}
		if err := m.VolumeCopyTo(ctx, hostID, name, "/", tarball); err != nil {
			return fmt.Errorf("seed volume for %s: %w", b.Rel, err)
		}

internal/docker/compose_binds_multihost_test.go:103

  • The bindProject helper comment says it returns the classified internal binds, but the function currently returns nil for that value. Either compute and return the binds or update the comment so it matches what the helper actually does.
// bindProject writes a minimal project with one directory bind and returns its
// dir plus the classified internal binds.
func bindProject(t *testing.T, marker string) (string, []ProjectBind, string) {
  • Files reviewed: 20/25 changed files
  • Comments generated: 0 new
  • Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Adds docs/testing.md describing what "tested" actually means here, since the
app is pointed at real (often production) Docker daemons. Five tiers, each
with what it proves and how to run it: unit, adversarial pentests,
runtime smoke, integration against a real daemon, and multi-daemon
end-to-end over TCP and SSH.

Every figure was taken from the tree rather than estimated (293 Go tests /
17 packages, 31 pentest cases in 4 files, 10 integration files, 13 frontend
tests), and the documented commands were run as written.

It also states what is NOT covered, which is the part that makes the rest
credible: CI runs only the deterministic tiers, so a green badge does not
mean "verified against real daemons"; there is no browser/UI suite; no HTTP
round trip for remote projects; Windows is cross-compiled but not
integration-tested; and coverage (~40%, unit-only) is framed as the weak
proxy it is.

Linked from the README, docs index and CONTRIBUTING.

Separately: the Go Report Card badge now renders "go report: retired" (the
service was shut down — it returns 200, so it fails as a misleading grade
rather than a broken image), so it's removed.
Copilot AI review requested due to automatic review settings July 30, 2026 10:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Not ready to approve

There are correctness issues around seeded-volume re-deploy behavior and validation/deploy consistency that can lead to stale files and mismatched bind classification versus what is actually deployed.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Comments suppressed due to low confidence (2)

internal/docker/compose_binds.go:260

  • SeedProjectBinds currently streams a TAR into an existing seed volume without clearing it first. CopyToContainer will overwrite matching paths but will not remove files that were deleted from the project since the last deploy, so a re-seed can leave stale files behind in the seeded volume (contradicting the “ships current files” behavior implied here). Consider clearing the volume root before copying so the seeded volume matches the project snapshot on every deploy.
		tarball, err := tarPath(src, b.IsFile)
		if err != nil {
			return fmt.Errorf("archive %s: %w", b.Rel, err)
		}
		if err := m.VolumeCopyTo(ctx, hostID, name, "/", tarball); err != nil {

internal/api/project_handlers.go:1055

  • projectDeployEnv validates bind mounts using docker.ComposeConfigJSON(ctx, dir, p.Slug), which relies on Compose’s default file discovery, but the actual deploy path later forces an explicit -f list ([]string{p.ComposeFile, <temp override>}). If a project folder contains implicit override files (e.g. compose.override.yml) or uses a non-default compose filename, the validation/classification result can diverge from what gets deployed (and from what gets seeded). It would be safer to resolve the config using the exact same file list you pass to ComposeUpFiles (and ideally the same profiles) so “validated == deployed”.
	// Fail closed: without a resolved config we can't prove which paths this
	// project would mount on the remote host, so don't deploy at all.
	cfgJSON, err := docker.ComposeConfigJSON(ctx, dir, p.Slug)
	if err != nil {
		return nil, nil, "", noop, fmt.Errorf("cannot validate the compose file for remote deploy: %v", err)
	}
	internal, external, err := docker.ClassifyProjectBinds(cfgJSON, dir)
  • Files reviewed: 23/28 changed files
  • Comments generated: 0 new
  • Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Two issues from review, both reproduced before fixing:

A compose file may name a bind source the project doesn't contain yet (a
database's ./data, say). ClassifyProjectBinds correctly calls that internal,
but writeTar returned filepath.Walk's lstat error, so the whole remote
deploy failed instead of seeding an empty volume. Locally Docker creates
such a path on demand, so an empty volume is the right remote equivalent.
The classification case was tested; the seeding of it wasn't — now both are,
including against a real daemon.

The deploy output note ("files were copied, not mounted live") was only
prepended in the project list's handler, so a deploy started from the
project editor silently dropped it. The formatting moved to
lib/composeOutput.ts, used by both call sites and covered by tests, so a
third caller can't diverge again.
Copilot AI review requested due to automatic review settings July 30, 2026 10:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Not ready to approve

The deploy handler currently drops the note on compose failure and one new “pentest” doesn’t actually exercise special-file skipping, so user-facing behavior and test intent don’t fully match the PR’s stated guarantees.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Comments suppressed due to low confidence (3)

internal/api/project_handlers.go:642

  • The remote-deploy note is only returned on the success path. If docker compose up fails after bind seeding, the UI won’t show the snapshot/copy warning even though it’s still relevant (and composeOutputText has explicit test coverage for note+error). Include note in the error response too so users understand the semantics even on a failed deploy.
	if err != nil {
		writeJSON(w, http.StatusOK, map[string]any{"ok": false, "error": err.Error(), "output": out})
		return
	}

internal/docker/compose_binds_test.go:587

  • This “PENTEST” doesn’t currently create any device/socket/fifo entry, so it can’t actually prove that tarPath skips special files (it only asserts a regular file is present). Either create a special file on Unix (e.g. syscall.Mkfifo + net.Listen("unix", …)) and assert it is absent from the archive, or rename/reword the test so it doesn’t claim coverage it doesn’t provide.
// PENTEST: a compose file naming a device/socket in the project dir must not
// break the deploy or ship the special file.
func TestPentestTarPath_SkipsSpecialFiles(t *testing.T) {
	root := t.TempDir()
	if err := os.WriteFile(filepath.Join(root, "ok.txt"), []byte("fine"), 0o644); err != nil {
		t.Fatal(err)
	}
	r, err := tarPath(root, false)
	if err != nil {
		t.Fatal(err)
	}
	names := tarNames(t, r)
	if _, ok := names["ok.txt"]; !ok {
		t.Errorf("regular files should still be archived: %v", names)
	}
}

internal/docker/compose_binds_multihost_test.go:280

  • deployTo calls t.Fatal/t.Fatalf and registers t.Cleanup but is invoked from a goroutine in TestMultiHostConcurrentMixedTransports. testing.T’s Fatal/FailNow semantics aren’t safe from non-test goroutines (it only exits that goroutine), and cleanup registration from goroutines can lead to hard-to-debug flakiness. Prefer returning an error/cleanup from the goroutine and failing/cleaning up from the main test goroutine.
	var wg sync.WaitGroup
	for _, s := range specs {
		wg.Add(1)
		go func(s spec) {
			defer wg.Done()
			deployTo(t, m, hosts[s.idx], ids[s.idx], s.dir, s.slug)
		}(s)
  • Files reviewed: 25/30 changed files
  • Comments generated: 0 new
  • Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Copilot AI review requested due to automatic review settings July 30, 2026 10:23
@malickyeu
malickyeu merged commit 1fa192e into main Jul 30, 2026
4 checks passed
@malickyeu
malickyeu deleted the feat/remote-project-binds branch July 30, 2026 10:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Not ready to approve

Seeded bind volumes are updated via tar copy without clearing existing contents first, so removed project files can linger across redeploys and cause stale remote configs.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Files not reviewed (1)

  • web/package-lock.json: Generated file

Comments suppressed due to low confidence (3)

internal/docker/compose_binds.go:260

  • Seed volumes are reseeded by copying a TAR into the helper container, but nothing clears the volume first. That means files removed from the project directory can linger in the seeded volume across redeploys (e.g. stale configs under conf.d), so the remote deployment may not reflect the current project state.
		if err := m.VolumeCopyTo(ctx, hostID, name, "/", tarball); err != nil {
			return fmt.Errorf("seed volume for %s: %w", b.Rel, err)

scripts/remote-test-daemon.sh:124

  • The generated ssh shim script expands $real_ssh and $WORKDIR unquoted. If DC_REMOTE_WORKDIR contains spaces (or if the ssh path ever does), the shim can break due to word-splitting. Quoting these paths makes the script robust.
#!/bin/sh
exec $real_ssh \\
  -o UserKnownHostsFile=$WORKDIR/home/.ssh/known_hosts \\
  -o IdentityFile=$WORKDIR/home/.ssh/id_ed25519 \\
  -o IdentitiesOnly=yes \\
  -o StrictHostKeyChecking=yes \\
  "\$@"

internal/docker/compose_binds_multihost_test.go:103

  • The helper's comment says it returns the classified internal binds, but the function currently returns nil for that value. This makes the helper misleading for future edits (it looks like callers could use the binds but they can't).
// bindProject writes a minimal project with one directory bind and returns its
// dir plus the classified internal binds.
func bindProject(t *testing.T, marker string) (string, []ProjectBind, string) {
  • Files reviewed: 25/32 changed files
  • Comments generated: 0 new
  • Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

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