fix(env): scope docker compose with hop project name; review fixes#16
Merged
fix(env): scope docker compose with hop project name; review fixes#16
Conversation
Compose was invoked without -p, so the project name (and therefore
container, network, and volume identifiers) defaulted to the basename
of the worktree directory. Two hops with the same branch name across
different hubs collided on container names like staging-redis-1, and
stale containers from a crashed prior run blocked restart.
This was the third leg of hop isolation. Ports and volumes were
already namespaced via the generated override file, but
container/network identity was not.
Changes:
- Add ComposeProjectName(org, repo, branch) producing a stable,
compose-safe slug (lowercase [a-z0-9_-], hyphens between segments).
- Refactor EnvironmentManager.buildCommandWithOverride to
buildComposeCommand which always injects -p <project> for the
docker-compose manager (with or without an override file).
- Thread org/repo/branch through Start/Stop in env_managers.
- Extend Docker.ComposeUp/Stop/Down/Ps in the wrapper to take a
project name and inject -p when set; update status.go callers to
pass the right name (resolving the enclosing hub for the current
worktree case). Test helper CleanupContainers gains a project arg;
call sites pass "" to keep prior behavior.
- Tests:
* TestComposeProjectName covers slugification edge cases.
* TestBuildComposeCommand_InjectsProjectName covers both override
and no-override paths plus non-docker-compose passthrough.
Known limitations (out of scope for this fix):
- Compose files that hardcode container_name still collide; -p only
affects auto-generated names.
- Stale containers with the OLD (basename-derived) project name will
remain on disk until manually cleaned. The new project name avoids
collision with them but they leak. A follow-up should extend env gc
to sweep orphaned containers labeled with project names that no
longer correspond to a known hop.
Closes #12
Addresses all 12 Copilot review findings on PR #14: 1. composeSlugify allowed leading '_' or '-' to survive into the compose project name, violating the "must start with alphanumeric" rule. Trim leading separators from both ends of the slug, and drop any residual non-alphanumeric leading byte defensively. Adds 4 new test cases (leading underscore, leading hyphen, all-underscore segment, all-unsafe input). 2. CleanupContainers previously took (t, dir, project string) and all e2e call sites passed "" — which meant the cleanup ran against compose's default project (cwd basename) instead of the hop-scoped project name hop env start now injects via -p. Result: containers started by the tests would leak, and cleanup-on-failure would miss them entirely. Change signature to (t, dir, branch string) and derive the project name inside the helper via hopComposeProject, which loads the enclosing hub config and calls ComposeProjectName. All 13 call sites updated with the correct branch. 3. CleanupContainers inline comment claimed "remove containers and volumes" but ComposeDown runs without --volumes, so named volumes actually survived cleanup. Update the comment to match behavior and note the workaround for tests that need volume teardown. 4. WaitForServiceHealthy and WaitForContainerReady ran raw `docker compose` commands without -p, so they checked the WRONG project (cwd basename) once hop started namespacing. Extend both helpers to take a branch argument, derive the project name via hopComposeProject, and inject -p through a new composeArgs helper. Also plumb through waitForLaravelServices. All call sites updated. 5. status.go's `len(ps) > 0` check was true for "[]" (the empty JSON array that `docker compose ps --format json` emits when nothing is running), overcounting running services. Pre-existing bug widened by PR #14's surface. Add composePsHasRunning which parses JSON arrays, handles JSON Lines output (newer compose versions), and treats whitespace-only / "[]" output as empty. New unit test TestComposePsHasRunning covers 10 cases including both formats and defensive fallback for malformed non-empty input. Test status: - go test ./cmd/... ./internal/... ./test/integration/... — green - go vet ./... — clean - go build ./... — clean - e2e/docker compiles with updated signatures Refs #12
There was a problem hiding this comment.
Pull request overview
This PR fixes Docker Compose environment isolation by introducing a stable, compose-safe project name and ensuring docker compose commands are consistently scoped with -p <project>, preventing cross-hub/container name collisions and improving status/cleanup correctness.
Changes:
- Add
services.ComposeProjectName(org, repo, branch)and inject the derived-p <project>into docker-compose environment manager commands. - Extend the Docker wrapper and
statuscommand to query compose state within the correct project scope and correctly detect “no running containers”. - Update e2e Docker helpers/tests to derive and pass the hop-scoped compose project name so readiness checks and cleanup target the correct containers.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/docker/docker_realworld_nextjs_test.go | Passes branch to cleanup/readiness helpers so they scope compose by hop project. |
| test/e2e/docker/docker_realworld_laravel_test.go | Threads branch through Laravel helper chain to scope compose health checks/cleanup. |
| test/e2e/docker/docker_realworld_django_test.go | Updates service health checks/cleanup to include branch-derived compose project. |
| test/e2e/docker/docker_override_test.go | Updates health checks/cleanup to include branch for compose project scoping. |
| test/e2e/docker/docker_isolation_test.go | Updates health checks/cleanup to include branch for compose project scoping. |
| test/e2e/docker/docker_integration_test.go | Updates health checks/cleanup to include branch for compose project scoping. |
| test/e2e/docker/docker_helpers.go | Adds hopComposeProject + composeArgs; updates helpers to use -p <project>. |
| internal/services/env_managers.go | Introduces ComposeProjectName/slugify and injects -p in compose command building. |
| internal/services/env_managers_test.go | Adds tests for project-name slugification and -p injection behavior. |
| internal/docker/wrapper.go | Extends compose wrapper methods to accept a project and prepend compose -p <project>. |
| cmd/status.go | Scopes docker compose ps by hop project and fixes “running services” detection for [] output. |
| cmd/status_test.go | Adds unit tests for composePsHasRunning edge cases and malformed output behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/services/env_managers.go
Outdated
Comment on lines
+68
to
+71
| // Trim separators from both ends. Compose requires the result to start | ||
| // with [a-z0-9]; underscores are allowed internally but not as the | ||
| // first character. | ||
| out := strings.Trim(b.String(), "-_") |
There was a problem hiding this comment.
composeSlugify() trims both leading and trailing "-" via strings.Trim(..., "-"). Compose project-name regex allows trailing '-'/'_' (only the first char must be [a-z0-9]), so trimming the trailing separators is unnecessary and can create collisions (e.g., branch "foo" vs "foo-" both slugify to "foo"). Consider trimming only the leading separators (e.g., TrimLeft/explicit loop) and leaving trailing characters intact unless you’ve confirmed compose rejects them.
internal/services/env_managers.go
Outdated
| composeFile := docker.FindComposeFile(worktreePath) | ||
| if composeFile == "" { | ||
| // buildComposeCommand assembles the docker compose invocation, injecting: | ||
| // - -p <project> always (so containers/networks/volumes are hop-scoped) |
There was a problem hiding this comment.
The buildComposeCommand doc comment says it injects -p <project> "always", but the implementation only appends -p when projectName != "". Please align the comment with the actual behavior (e.g., "when non-empty") or ensure ComposeProjectName always returns a non-empty, compose-safe value for real branches.
Suggested change
| // - -p <project> always (so containers/networks/volumes are hop-scoped) | |
| // - -p <project> when ComposeProjectName(org, repo, branch) is non-empty | |
| // (so containers/networks/volumes are hop-scoped) |
Addresses 2 Copilot review findings on PR #16: 1. composeSlugify used strings.Trim(out, "-_") which stripped both leading AND trailing separators. Compose project-name regex ^[a-z0-9][a-z0-9_-]*$ only requires the FIRST character to be alphanumeric — trailing "-" and "_" are legal. Trimming them collapsed distinct inputs (branch "foo" vs "foo-") into the same project name, creating spurious isolation collisions. Use TrimLeft only, and drop the now-redundant defensive leading-trim loop. Adds 4 new test cases covering trailing hyphen, trailing underscore, trailing mixed, and the foo-vs-foo- distinction. 2. buildComposeCommand doc comment said it injects -p "always" but the implementation gates on projectName != "". Align the comment with the actual behavior and explain the fallback (compose's default project = cwd basename) when slugify yields "". Refs #12
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
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.
Replaces #14 with review fixes applied. Addresses all 12 Copilot findings on that PR.
Summary
`git hop env start` invoked `docker compose` without `-p`, so the project name (and therefore container, network, and volume identifiers) defaulted to the basename of the worktree directory. Two hops with the same branch name across different hubs collided on container names like `staging-redis-1`; stale containers from crashed prior runs blocked restart. This was the third leg of hop isolation — ports and volumes were already namespaced via the override file, but container/network identity was not.
Closes #12
Review fixes applied (vs #14)
Slugify leading-underscore bug (review finding A). `composeSlugify` allowed `foo` to survive as a project name, which compose rejects (project names must start with alphanumeric). Now trims leading ``/`-` plus a defensive final alphanumeric check. 4 new test cases added.
Cleanup helpers passed empty project (review findings B1-B7). All 13 `CleanupContainers(t, path, "")` call sites were leaking containers because compose defaulted to cwd-basename instead of the hop project name. Changed the helper to take `branch` and derive the project internally via a new `hopComposeProject(dir, branch)` helper that loads the enclosing hub config.
Comment/code drift (review finding C). `CleanupContainers` comment claimed "removes containers and volumes" but `ComposeDown` ran without `--volumes`. Updated the comment to match actual behavior.
Other e2e helpers ran raw compose without -p (review finding D). `WaitForServiceHealthy`, `WaitForContainerReady`, and `waitForLaravelServices` all ran `docker compose ...` directly without `-p`, so after PR landed they'd check the wrong project. All now take `branch` and inject `-p` via a `composeArgs` helper.
`len(ps) > 0` overcounts on empty JSON array (review findings E). `docker compose ps --format json` returns `"[]"` (2 chars) when nothing is running, so the check was always true. Added `composePsHasRunning` that handles both JSON-array and JSON-Lines compose output formats. New `TestComposePsHasRunning` covers 10 cases.
Test plan
Known limitations (unchanged from #14)
Stacking
Independent of #15 (deps cache fix). Can land in either order.