From 57f74457ff24560d87b9b220de91c38e61f29b19 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Mon, 20 Apr 2026 21:37:36 +0000 Subject: [PATCH 1/2] fix(service): bind binary services to pre-linked projects on add When `pv service:add mail` (or s3) runs, projects that were linked before the service existed have no Services.Mail flag set, so ProjectsUsingService returns nothing and their .env is never updated. Fix by calling bindBinaryServiceToAllProjects before updateLinkedProjectsEnvBinary in addBinary, which marks all linked Laravel/laravel-octane projects as using the service so the .env update step can find and update them. Co-authored-by: Clovis --- internal/commands/service/add.go | 7 ++++ internal/commands/service/hooks.go | 18 ++++++++ internal/commands/service/hooks_test.go | 56 +++++++++++++++++++++++++ 3 files changed, 81 insertions(+) diff --git a/internal/commands/service/add.go b/internal/commands/service/add.go index 7b99b68..a94f2cd 100644 --- a/internal/commands/service/add.go +++ b/internal/commands/service/add.go @@ -244,6 +244,13 @@ func addBinary(ctx context.Context, reg *registry.Registry, svc services.BinaryS return fmt.Errorf("cannot save registry: %w", err) } + // Projects linked before this service was added won't be in ProjectsUsingService; + // bind them now so updateLinkedProjectsEnvBinary can find them. + bindBinaryServiceToAllProjects(reg, name) + if err := reg.Save(); err != nil { + return fmt.Errorf("cannot save registry after binding service: %w", err) + } + // Update .env for linked Laravel projects — parity with the docker path // (updateLinkedProjectsEnv at the end of addDocker). Without this the // user adds s3 but linked projects never get AWS_* keys written. diff --git a/internal/commands/service/hooks.go b/internal/commands/service/hooks.go index dc6e53c..6883cc2 100644 --- a/internal/commands/service/hooks.go +++ b/internal/commands/service/hooks.go @@ -127,6 +127,24 @@ func updateLinkedProjectsEnvBinary(reg *registry.Registry, svcName string, svc s } } +func bindBinaryServiceToAllProjects(reg *registry.Registry, svcName string) { + for i := range reg.Projects { + p := ®.Projects[i] + if p.Type != "laravel" && p.Type != "laravel-octane" { + continue + } + if p.Services == nil { + p.Services = ®istry.ProjectServices{} + } + switch svcName { + case "mail": + p.Services.Mail = true + case "s3": + p.Services.S3 = true + } + } +} + // applyStopAllFallbacks applies env fallbacks for every Docker service in the // registry. Binary services are skipped because the stop-all command does not // stop them (they're managed by the daemon). Called from the no-args diff --git a/internal/commands/service/hooks_test.go b/internal/commands/service/hooks_test.go index 4a28955..c7e8351 100644 --- a/internal/commands/service/hooks_test.go +++ b/internal/commands/service/hooks_test.go @@ -163,6 +163,62 @@ func TestUpdateLinkedProjectsEnv_OnlyUpdatesLinkedProject(t *testing.T) { } } +func TestBindBinaryServiceToAllProjects_Mail(t *testing.T) { + reg := ®istry.Registry{ + Projects: []registry.Project{ + {Name: "app-no-services", Path: "/tmp/a", Type: "laravel"}, + {Name: "app-services-unset", Path: "/tmp/b", Type: "laravel", + Services: ®istry.ProjectServices{Redis: true}}, + {Name: "app-octane", Path: "/tmp/c", Type: "laravel-octane"}, + {Name: "app-other", Path: "/tmp/d", Type: "static"}, + {Name: "app-already", Path: "/tmp/e", Type: "laravel", + Services: ®istry.ProjectServices{Mail: true}}, + }, + } + + bindBinaryServiceToAllProjects(reg, "mail") + + for _, tc := range []struct { + name string + wantMail bool + }{ + {"app-no-services", true}, + {"app-services-unset", true}, + {"app-octane", true}, + {"app-other", false}, + {"app-already", true}, + } { + p := reg.Find(tc.name) + if p == nil { + t.Fatalf("project %q not found", tc.name) + } + gotMail := p.Services != nil && p.Services.Mail + if gotMail != tc.wantMail { + t.Errorf("project %q: Mail = %v, want %v", tc.name, gotMail, tc.wantMail) + } + } +} + +func TestBindBinaryServiceToAllProjects_S3(t *testing.T) { + reg := ®istry.Registry{ + Projects: []registry.Project{ + {Name: "app-laravel", Path: "/tmp/a", Type: "laravel"}, + {Name: "app-static", Path: "/tmp/b", Type: "static"}, + }, + } + + bindBinaryServiceToAllProjects(reg, "s3") + + laravelApp := reg.Find("app-laravel") + if laravelApp == nil || !laravelApp.Services.S3 { + t.Error("app-laravel: S3 should be true after bindBinaryServiceToAllProjects") + } + staticApp := reg.Find("app-static") + if staticApp != nil && staticApp.Services != nil && staticApp.Services.S3 { + t.Error("app-static: S3 should not be set for non-Laravel projects") + } +} + func TestUnbindService_ClearsMailBinding(t *testing.T) { reg := ®istry.Registry{ Projects: []registry.Project{ From d7aac57ece364394a4153c2d7ebe780c95f67337 Mon Sep 17 00:00:00 2001 From: Clovis Muneza Date: Mon, 20 Apr 2026 23:35:18 -0400 Subject: [PATCH 2/2] fix(service): guard bindBinaryServiceToAllProjects against unknown names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback on #70: - Return an error from bindBinaryServiceToAllProjects for unknown svcName so a future binary service added without updating the switch fails loudly at call time instead of silently skipping the retroactive-bind step. The ProjectServices fields and the cases here are a synchronization pair with no compile-time enforcement; the default case is the only guard. - Add unit coverage for the unknown-service error path and assert no project Services mutation occurs when the service is unknown. - Add an integration-level unit test chaining bind → ProjectsUsingService to lock the contract that motivates the fix for #69. - Mirror mail-binary.sh's pre-link assertion in s3-binary.sh so the S3 path of the retroactive bind is covered end-to-end. --- internal/commands/service/add.go | 4 +- internal/commands/service/hooks.go | 13 +++++- internal/commands/service/hooks_test.go | 60 +++++++++++++++++++++++-- scripts/e2e/s3-binary.sh | 21 +++++++++ 4 files changed, 93 insertions(+), 5 deletions(-) diff --git a/internal/commands/service/add.go b/internal/commands/service/add.go index a94f2cd..7e6674a 100644 --- a/internal/commands/service/add.go +++ b/internal/commands/service/add.go @@ -246,7 +246,9 @@ func addBinary(ctx context.Context, reg *registry.Registry, svc services.BinaryS // Projects linked before this service was added won't be in ProjectsUsingService; // bind them now so updateLinkedProjectsEnvBinary can find them. - bindBinaryServiceToAllProjects(reg, name) + if err := bindBinaryServiceToAllProjects(reg, name); err != nil { + return fmt.Errorf("cannot bind service to projects: %w", err) + } if err := reg.Save(); err != nil { return fmt.Errorf("cannot save registry after binding service: %w", err) } diff --git a/internal/commands/service/hooks.go b/internal/commands/service/hooks.go index 6883cc2..44fcf5d 100644 --- a/internal/commands/service/hooks.go +++ b/internal/commands/service/hooks.go @@ -127,7 +127,17 @@ func updateLinkedProjectsEnvBinary(reg *registry.Registry, svcName string, svc s } } -func bindBinaryServiceToAllProjects(reg *registry.Registry, svcName string) { +// bindBinaryServiceToAllProjects sets the per-project Services flag for svcName +// on every Laravel project so updateLinkedProjectsEnvBinary can find projects +// that were linked before the service existed. Returns an error for unknown +// svcName so new binary services can't silently skip this step — the set of +// cases here must stay in lockstep with registry.ProjectServices fields. +func bindBinaryServiceToAllProjects(reg *registry.Registry, svcName string) error { + switch svcName { + case "mail", "s3": + default: + return fmt.Errorf("bindBinaryServiceToAllProjects: unknown binary service %q (add a case here and a field on ProjectServices)", svcName) + } for i := range reg.Projects { p := ®.Projects[i] if p.Type != "laravel" && p.Type != "laravel-octane" { @@ -143,6 +153,7 @@ func bindBinaryServiceToAllProjects(reg *registry.Registry, svcName string) { p.Services.S3 = true } } + return nil } // applyStopAllFallbacks applies env fallbacks for every Docker service in the diff --git a/internal/commands/service/hooks_test.go b/internal/commands/service/hooks_test.go index c7e8351..5f00104 100644 --- a/internal/commands/service/hooks_test.go +++ b/internal/commands/service/hooks_test.go @@ -176,10 +176,12 @@ func TestBindBinaryServiceToAllProjects_Mail(t *testing.T) { }, } - bindBinaryServiceToAllProjects(reg, "mail") + if err := bindBinaryServiceToAllProjects(reg, "mail"); err != nil { + t.Fatalf("bindBinaryServiceToAllProjects returned error: %v", err) + } for _, tc := range []struct { - name string + name string wantMail bool }{ {"app-no-services", true}, @@ -207,7 +209,9 @@ func TestBindBinaryServiceToAllProjects_S3(t *testing.T) { }, } - bindBinaryServiceToAllProjects(reg, "s3") + if err := bindBinaryServiceToAllProjects(reg, "s3"); err != nil { + t.Fatalf("bindBinaryServiceToAllProjects returned error: %v", err) + } laravelApp := reg.Find("app-laravel") if laravelApp == nil || !laravelApp.Services.S3 { @@ -219,6 +223,56 @@ func TestBindBinaryServiceToAllProjects_S3(t *testing.T) { } } +func TestBindBinaryServiceToAllProjects_UnknownServiceReturnsError(t *testing.T) { + reg := ®istry.Registry{ + Projects: []registry.Project{ + {Name: "app", Path: "/tmp/a", Type: "laravel"}, + }, + } + + err := bindBinaryServiceToAllProjects(reg, "bogus") + if err == nil { + t.Fatal("expected error for unknown service name, got nil") + } + + p := reg.Find("app") + if p != nil && p.Services != nil { + t.Error("unknown service: must not mutate project Services (guards against silent skips when new binary services are added)") + } +} + +// TestBindBinaryServiceToAllProjects_EnablesProjectsUsingServiceLookup locks +// the contract with registry.ProjectsUsingService — the reason this function +// exists. Regression here would silently break the #69 fix. +func TestBindBinaryServiceToAllProjects_EnablesProjectsUsingServiceLookup(t *testing.T) { + reg := ®istry.Registry{ + Projects: []registry.Project{ + {Name: "pre-linked", Path: "/tmp/a", Type: "laravel"}, + {Name: "pre-linked-octane", Path: "/tmp/b", Type: "laravel-octane"}, + {Name: "static-site", Path: "/tmp/c", Type: "static"}, + }, + } + + if before := reg.ProjectsUsingService("mail"); len(before) != 0 { + t.Fatalf("precondition: ProjectsUsingService(mail) should be empty before bind, got %d", len(before)) + } + + if err := bindBinaryServiceToAllProjects(reg, "mail"); err != nil { + t.Fatalf("bindBinaryServiceToAllProjects returned error: %v", err) + } + + names := map[string]bool{} + for _, n := range reg.ProjectsUsingService("mail") { + names[n] = true + } + if !names["pre-linked"] || !names["pre-linked-octane"] { + t.Errorf("ProjectsUsingService(mail) missing laravel projects after bind: got %v", names) + } + if names["static-site"] { + t.Error("ProjectsUsingService(mail) should not include non-Laravel projects") + } +} + func TestUnbindService_ClearsMailBinding(t *testing.T) { reg := ®istry.Registry{ Projects: []registry.Project{ diff --git a/scripts/e2e/s3-binary.sh b/scripts/e2e/s3-binary.sh index 1f79166..32b290b 100755 --- a/scripts/e2e/s3-binary.sh +++ b/scripts/e2e/s3-binary.sh @@ -11,10 +11,22 @@ START_PID=$! sleep 8 cleanup() { + sudo -E pv unlink e2e-s3-env >/dev/null 2>&1 || true sudo -E pv stop >/dev/null 2>&1 || true + rm -rf "${ENVTEST_DIR:-}" 2>/dev/null || true } trap cleanup EXIT +# Create a minimal linked Laravel project BEFORE service:add so we exercise +# the retroactive-bind path (issue #69): projects linked before a binary +# service existed must still get their .env keys written. +ENVTEST_DIR=$(mktemp -d) +echo '{"require":{"php":"^8.2","laravel/framework":"^11.0"}}' > "$ENVTEST_DIR/composer.json" +mkdir -p "$ENVTEST_DIR/public" +echo ' "$ENVTEST_DIR/public/index.php" +echo "FILESYSTEM_DISK=local" > "$ENVTEST_DIR/.env" +sudo -E pv link "$ENVTEST_DIR" --name e2e-s3-env >/dev/null 2>&1 || { echo "FAIL: pv link for env test"; exit 1; } + echo "==> service:add s3" sudo -E pv service:add s3 || { echo "FAIL: pv service:add s3 failed"; exit 1; } @@ -42,6 +54,15 @@ done nc -z 127.0.0.1 9000 || { echo "FAIL: port 9000 not reachable after service:add"; exit 1; } echo "OK: port 9000 reachable" +echo "==> Verify linked project .env got AWS_ENDPOINT" +grep -q "AWS_ENDPOINT=http://127.0.0.1:9000" "$ENVTEST_DIR/.env" || { + echo "FAIL: linked project .env should have AWS_ENDPOINT after service:add s3"; + echo " actual .env contents:"; + cat "$ENVTEST_DIR/.env"; + exit 1; +} +echo "OK: linked project .env has AWS_ENDPOINT" + echo "==> service:stop s3" sudo -E pv service:stop s3 sleep 2