From d13981f29d07b46f9f5bf733330380025798b989 Mon Sep 17 00:00:00 2001 From: Danil Pismenny Date: Mon, 2 Mar 2026 10:52:49 +0000 Subject: [PATCH 01/12] fix: make TestLockPortSameDirectory_NoError resilient to busy ports Instead of hardcoding port 3003, dynamically find a free port within the configured range (3000-4000). This prevents flakiness when the hardcoded port is in use by other services (e.g. docker-proxy). Co-Authored-By: Claude Opus 4.6 --- cmd/port-selector/main_test.go | 38 +++++++++++++++++++++++++--------- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/cmd/port-selector/main_test.go b/cmd/port-selector/main_test.go index fe4fb49..adbdd5f 100644 --- a/cmd/port-selector/main_test.go +++ b/cmd/port-selector/main_test.go @@ -429,6 +429,23 @@ func TestLockPortFromAnotherDirectory_WithForce(t *testing.T) { func TestLockPortSameDirectory_NoError(t *testing.T) { binary := buildBinary(t) + // Find a free port within the default configured range (3000-4000) + // to avoid flakiness when hardcoded ports are in use by other services + var freePort int + for p := 3000; p <= 4000; p++ { + ln, err := net.Listen("tcp", fmt.Sprintf(":%d", p)) + if err != nil { + continue + } + ln.Close() + freePort = p + break + } + if freePort == 0 { + t.Fatal("could not find any free port in range 3000-4000") + } + portStr := fmt.Sprintf("%d", freePort) + tmpDir := t.TempDir() configDir := filepath.Join(tmpDir, ".config", "port-selector") if err := os.MkdirAll(configDir, 0755); err != nil { @@ -442,24 +459,25 @@ func TestLockPortSameDirectory_NoError(t *testing.T) { env := append(os.Environ(), "XDG_CONFIG_HOME="+filepath.Join(tmpDir, ".config")) - // Step 1: Allocate port 3003 for project - cmd := exec.Command(binary, "--lock", "3003") + // Step 1: Allocate the port for project + cmd := exec.Command(binary, "--lock", portStr) cmd.Dir = workDir cmd.Env = env if output, err := cmd.CombinedOutput(); err != nil { - t.Fatalf("failed to lock port 3003: %v, output: %s", err, output) + t.Fatalf("failed to lock port %s: %v, output: %s", portStr, err, output) } - // Step 2: Lock port 3003 again from same directory (should succeed without --force) - cmd = exec.Command(binary, "--lock", "3003") + // Step 2: Lock same port again from same directory (should succeed without --force) + cmd = exec.Command(binary, "--lock", portStr) cmd.Dir = workDir cmd.Env = env - output, err := cmd.CombinedOutput() - if err != nil { - t.Fatalf("expected success, got error: %v, output: %s", err, output) + output, err2 := cmd.CombinedOutput() + if err2 != nil { + t.Fatalf("expected success, got error: %v, output: %s", err2, output) } - if !strings.Contains(string(output), "Locked port 3003") { - t.Errorf("expected 'Locked port 3003' message, got: %s", output) + expectedMsg := fmt.Sprintf("Locked port %d", freePort) + if !strings.Contains(string(output), expectedMsg) { + t.Errorf("expected %q message, got: %s", expectedMsg, output) } } From 590757aedb174bc6435bf6cd54200d73b6fc783f Mon Sep 17 00:00:00 2001 From: Danil Pismenny Date: Mon, 2 Mar 2026 12:43:33 +0000 Subject: [PATCH 02/12] fix: address review feedback - Revert err2 back to idiomatic err (no shadowing conflict) - Fix "Step 1" comment: allocate -> allocate and lock - Add allocation state verification after re-lock (matches sibling test pattern) Co-Authored-By: Claude Opus 4.6 --- cmd/port-selector/main_test.go | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/cmd/port-selector/main_test.go b/cmd/port-selector/main_test.go index adbdd5f..bbf93b9 100644 --- a/cmd/port-selector/main_test.go +++ b/cmd/port-selector/main_test.go @@ -459,7 +459,7 @@ func TestLockPortSameDirectory_NoError(t *testing.T) { env := append(os.Environ(), "XDG_CONFIG_HOME="+filepath.Join(tmpDir, ".config")) - // Step 1: Allocate the port for project + // Step 1: Allocate and lock the port for project cmd := exec.Command(binary, "--lock", portStr) cmd.Dir = workDir cmd.Env = env @@ -471,14 +471,30 @@ func TestLockPortSameDirectory_NoError(t *testing.T) { cmd = exec.Command(binary, "--lock", portStr) cmd.Dir = workDir cmd.Env = env - output, err2 := cmd.CombinedOutput() - if err2 != nil { - t.Fatalf("expected success, got error: %v, output: %s", err2, output) + output, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("expected success, got error: %v, output: %s", err, output) } expectedMsg := fmt.Sprintf("Locked port %d", freePort) if !strings.Contains(string(output), expectedMsg) { t.Errorf("expected %q message, got: %s", expectedMsg, output) } + + // Verify allocation state is correct after re-lock + store, err := allocations.Load(configDir) + if err != nil { + t.Fatalf("failed to load allocations: %v", err) + } + alloc := store.FindByPort(freePort) + if alloc == nil { + t.Fatalf("expected allocation for port %d", freePort) + } + if alloc.Directory != workDir { + t.Errorf("expected port to belong to %s, got %s", workDir, alloc.Directory) + } + if !alloc.Locked { + t.Error("expected port to remain locked after re-lock") + } } func TestScan_RecordsBusyPorts(t *testing.T) { From 6811747b5b4f5381a4630bfa8992d0f785cab4d7 Mon Sep 17 00:00:00 2001 From: Danil Pismenny Date: Mon, 2 Mar 2026 13:21:15 +0000 Subject: [PATCH 03/12] fix: address PR review issues (iteration 1) - Move port discovery after setup to minimize TOCTOU window - Use t.Skipf instead of t.Fatalf for first --lock call (matches project pattern for port-unavailable scenarios) - Add .gitignore entries for pr-review-fix-loop artifacts Co-Authored-By: Claude Opus 4.6 --- .gitignore | 14 ++++++++++++++ cmd/port-selector/main_test.go | 31 ++++++++++++++++--------------- 2 files changed, 30 insertions(+), 15 deletions(-) diff --git a/.gitignore b/.gitignore index 6ffd459..92bc81e 100644 --- a/.gitignore +++ b/.gitignore @@ -34,3 +34,17 @@ dist/ # Claude Code ctags index .claude/tags + +# Claude Code worktrees +.claude/worktrees/ + +# PR Review Fix Loop +.claude/pr-review-loop-report.local.md +.claude/pr-review-loop-stats.local.json +.claude/pr-review-loop-debug.local.log +.claude/pr-review-fix-loop.local.md +.codex-review.md +.codex-review.stderr + +# pr-review-fix-loop local artifacts +.claude/*.local.md diff --git a/cmd/port-selector/main_test.go b/cmd/port-selector/main_test.go index bbf93b9..faaefd5 100644 --- a/cmd/port-selector/main_test.go +++ b/cmd/port-selector/main_test.go @@ -429,8 +429,22 @@ func TestLockPortFromAnotherDirectory_WithForce(t *testing.T) { func TestLockPortSameDirectory_NoError(t *testing.T) { binary := buildBinary(t) + tmpDir := t.TempDir() + configDir := filepath.Join(tmpDir, ".config", "port-selector") + if err := os.MkdirAll(configDir, 0755); err != nil { + t.Fatal(err) + } + + workDir := filepath.Join(tmpDir, "project") + if err := os.MkdirAll(workDir, 0755); err != nil { + t.Fatal(err) + } + + env := append(os.Environ(), "XDG_CONFIG_HOME="+filepath.Join(tmpDir, ".config")) + // Find a free port within the default configured range (3000-4000) - // to avoid flakiness when hardcoded ports are in use by other services + // to avoid flakiness when hardcoded ports are in use by other services. + // Done after setup to minimize TOCTOU window before first --lock call. var freePort int for p := 3000; p <= 4000; p++ { ln, err := net.Listen("tcp", fmt.Sprintf(":%d", p)) @@ -446,25 +460,12 @@ func TestLockPortSameDirectory_NoError(t *testing.T) { } portStr := fmt.Sprintf("%d", freePort) - tmpDir := t.TempDir() - configDir := filepath.Join(tmpDir, ".config", "port-selector") - if err := os.MkdirAll(configDir, 0755); err != nil { - t.Fatal(err) - } - - workDir := filepath.Join(tmpDir, "project") - if err := os.MkdirAll(workDir, 0755); err != nil { - t.Fatal(err) - } - - env := append(os.Environ(), "XDG_CONFIG_HOME="+filepath.Join(tmpDir, ".config")) - // Step 1: Allocate and lock the port for project cmd := exec.Command(binary, "--lock", portStr) cmd.Dir = workDir cmd.Env = env if output, err := cmd.CombinedOutput(); err != nil { - t.Fatalf("failed to lock port %s: %v, output: %s", portStr, err, output) + t.Skipf("port %s became unavailable between discovery and lock: %v, output: %s", portStr, err, output) } // Step 2: Lock same port again from same directory (should succeed without --force) From 1fa7826863c374b958294050aae6885c712075f6 Mon Sep 17 00:00:00 2001 From: Danil Pismenny Date: Mon, 2 Mar 2026 13:24:31 +0000 Subject: [PATCH 04/12] fix: address PR review issues (iteration 2) Narrow t.Skipf guard on first --lock call to only skip for TOCTOU-related failures ("in use", "busy"). All other failures now use t.Fatalf so real regressions in the --lock code path are not silently skipped. Co-Authored-By: Claude Opus 4.6 --- cmd/port-selector/main_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cmd/port-selector/main_test.go b/cmd/port-selector/main_test.go index faaefd5..42f7f5f 100644 --- a/cmd/port-selector/main_test.go +++ b/cmd/port-selector/main_test.go @@ -465,7 +465,11 @@ func TestLockPortSameDirectory_NoError(t *testing.T) { cmd.Dir = workDir cmd.Env = env if output, err := cmd.CombinedOutput(); err != nil { - t.Skipf("port %s became unavailable between discovery and lock: %v, output: %s", portStr, err, output) + outStr := string(output) + if strings.Contains(outStr, "in use") || strings.Contains(outStr, "busy") { + t.Skipf("port %s became unavailable between discovery and lock (TOCTOU): %v, output: %s", portStr, err, outStr) + } + t.Fatalf("failed to lock port %s: %v, output: %s", portStr, err, outStr) } // Step 2: Lock same port again from same directory (should succeed without --force) From 304d6d0c7cef5e4b77445eb59d0e414c289c45a6 Mon Sep 17 00:00:00 2001 From: Danil Pismenny Date: Mon, 2 Mar 2026 13:30:29 +0000 Subject: [PATCH 05/12] fix: address PR review issues (iteration 3) Check for external allocation path in Step 1 output after success. If port was claimed by external process between discovery and lock, skip the test instead of continuing with misleading state. Co-Authored-By: Claude Opus 4.6 --- cmd/port-selector/main_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/cmd/port-selector/main_test.go b/cmd/port-selector/main_test.go index 42f7f5f..8c39b21 100644 --- a/cmd/port-selector/main_test.go +++ b/cmd/port-selector/main_test.go @@ -464,13 +464,17 @@ func TestLockPortSameDirectory_NoError(t *testing.T) { cmd := exec.Command(binary, "--lock", portStr) cmd.Dir = workDir cmd.Env = env - if output, err := cmd.CombinedOutput(); err != nil { - outStr := string(output) + step1Output, err := cmd.CombinedOutput() + if err != nil { + outStr := string(step1Output) if strings.Contains(outStr, "in use") || strings.Contains(outStr, "busy") { t.Skipf("port %s became unavailable between discovery and lock (TOCTOU): %v, output: %s", portStr, err, outStr) } t.Fatalf("failed to lock port %s: %v, output: %s", portStr, err, outStr) } + if strings.Contains(string(step1Output), "externally used") { + t.Skipf("port %s was claimed by external process between discovery and lock (TOCTOU): output: %s", portStr, step1Output) + } // Step 2: Lock same port again from same directory (should succeed without --force) cmd = exec.Command(binary, "--lock", portStr) From fb683b5596d9c422a0124f44c724cdc1e9d59843 Mon Sep 17 00:00:00 2001 From: Danil Pismenny Date: Mon, 2 Mar 2026 13:43:23 +0000 Subject: [PATCH 06/12] fix: address PR review issues (final) Change t.Fatal to t.Skipf when no free port is found in range, matching project convention for port-unavailability scenarios. Co-Authored-By: Claude Opus 4.6 --- cmd/port-selector/main_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/port-selector/main_test.go b/cmd/port-selector/main_test.go index 8c39b21..35012ee 100644 --- a/cmd/port-selector/main_test.go +++ b/cmd/port-selector/main_test.go @@ -456,7 +456,7 @@ func TestLockPortSameDirectory_NoError(t *testing.T) { break } if freePort == 0 { - t.Fatal("could not find any free port in range 3000-4000") + t.Skipf("could not find any free port in range 3000-4000") } portStr := fmt.Sprintf("%d", freePort) From 6cc4f868e60ca5cc3129eeca81c7f9ed4453ce79 Mon Sep 17 00:00:00 2001 From: Danil Pismenny Date: Mon, 2 Mar 2026 14:00:47 +0000 Subject: [PATCH 07/12] fix: resolve golangci-lint warnings in test file - Check fmt.Sscanf error return value (errcheck) - Add unreachable return after t.Fatal to satisfy staticcheck SA5011 (staticcheck doesn't recognize t.Fatal as terminating) Co-Authored-By: Claude Opus 4.6 --- cmd/port-selector/main_test.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/cmd/port-selector/main_test.go b/cmd/port-selector/main_test.go index 35012ee..6e59458 100644 --- a/cmd/port-selector/main_test.go +++ b/cmd/port-selector/main_test.go @@ -177,6 +177,7 @@ func TestLockAllocatesAndLocksFreePort(t *testing.T) { alloc := allocs.FindByPort(3500) if alloc == nil { t.Fatal("allocation for port 3500 was not created") + return // unreachable, but satisfies staticcheck SA5011 } if alloc.Directory != workDir { t.Errorf("expected directory %s, got %s", workDir, alloc.Directory) @@ -255,6 +256,7 @@ func TestLockPortWhenDirectoryAlreadyHasAllocation(t *testing.T) { alloc := allocs2.FindByPort(3500) if alloc == nil { t.Fatal("allocation for port 3500 was not created") + return // unreachable, but satisfies staticcheck SA5011 } if alloc.Directory != workDir { t.Errorf("expected directory %s, got %s", workDir, alloc.Directory) @@ -417,6 +419,7 @@ func TestLockPortFromAnotherDirectory_WithForce(t *testing.T) { alloc := store.FindByPort(3002) if alloc == nil { t.Fatal("expected allocation for port 3002") + return // unreachable, but satisfies staticcheck SA5011 } if alloc.Directory != workDir2 { t.Errorf("expected port to belong to %s, got %s", workDir2, alloc.Directory) @@ -497,6 +500,7 @@ func TestLockPortSameDirectory_NoError(t *testing.T) { alloc := store.FindByPort(freePort) if alloc == nil { t.Fatalf("expected allocation for port %d", freePort) + return // unreachable, but satisfies staticcheck SA5011 } if alloc.Directory != workDir { t.Errorf("expected port to belong to %s, got %s", workDir, alloc.Directory) @@ -603,6 +607,7 @@ func TestScan_SkipsAlreadyAllocatedPorts(t *testing.T) { alloc := loaded.FindByPort(3501) if alloc == nil { t.Fatal("allocation for port 3501 disappeared") + return // unreachable, but satisfies staticcheck SA5011 } if alloc.Directory != existingDir { t.Errorf("expected directory %s to be preserved, got %s", existingDir, alloc.Directory) @@ -757,6 +762,7 @@ func TestLockPort_FreeUnlockedFromOtherDir_NoForceNeeded(t *testing.T) { alloc := loaded.FindByPort(3010) if alloc == nil { t.Fatal("expected allocation for port 3010") + return // unreachable, but satisfies staticcheck SA5011 } if alloc.Directory != workDir2 { t.Errorf("expected port to belong to %s, got %s", workDir2, alloc.Directory) @@ -860,6 +866,7 @@ func TestLockPort_BusyNotAllocated_RegistersAsExternal(t *testing.T) { alloc := loaded.FindByPort(3012) if alloc == nil { t.Fatal("expected allocation for port 3012") + return // unreachable, but satisfies staticcheck SA5011 } if alloc.Status != "external" { t.Errorf("expected status 'external', got %q", alloc.Status) @@ -905,6 +912,7 @@ func TestLockPort_UnlocksOldLockedPort(t *testing.T) { alloc3013 := loaded.FindByPort(3013) if alloc3013 == nil { t.Fatal("expected allocation for port 3013 to still exist") + return // unreachable, but satisfies staticcheck SA5011 } if alloc3013.Locked { t.Error("old port 3013 should be unlocked after locking new port") @@ -913,6 +921,7 @@ func TestLockPort_UnlocksOldLockedPort(t *testing.T) { alloc3014 := loaded.FindByPort(3014) if alloc3014 == nil { t.Fatal("expected allocation for port 3014") + return // unreachable, but satisfies staticcheck SA5011 } if !alloc3014.Locked { t.Error("new port 3014 should be locked") @@ -1044,6 +1053,7 @@ func TestLockPort_SameDirectoryDifferentName(t *testing.T) { alloc := loaded.FindByPort(3020) if alloc == nil { t.Fatal("expected allocation for port 3020") + return // unreachable, but satisfies staticcheck SA5011 } // Name should be preserved as "web" since we're locking an existing port if alloc.Name != "web" { @@ -1102,6 +1112,7 @@ func TestLockPort_SameDirectorySamePortIdempotent(t *testing.T) { alloc := loaded.FindByPort(3021) if alloc == nil { t.Fatal("expected allocation for port 3021") + return // unreachable, but satisfies staticcheck SA5011 } if !alloc.Locked { t.Error("expected port to remain locked") @@ -1269,6 +1280,10 @@ func TestPortSelector_ReturnsSamePortEvenWhenBusy(t *testing.T) { t.Logf("Initial port: %s", initialPort) // Step 2: Simulate user's service running on that port + portNum := 0 + if _, err := fmt.Sscanf(initialPort, "%d", &portNum); err != nil { + t.Fatalf("failed to parse port %q: %v", initialPort, err) + } ln, err := net.Listen("tcp", ":"+initialPort) if err != nil { t.Skipf("could not occupy port %s for test: %v", initialPort, err) From 5b40f71d92a892309281e9f48180fe6565bd3084 Mon Sep 17 00:00:00 2001 From: Danil Pismenny Date: Mon, 2 Mar 2026 14:05:19 +0000 Subject: [PATCH 08/12] ci: trigger CI run Co-Authored-By: Claude Opus 4.6 From 432296b6c89c446b57f056ef897587ede0751378 Mon Sep 17 00:00:00 2001 From: Danil Pismenny Date: Wed, 4 Mar 2026 07:49:41 +0000 Subject: [PATCH 09/12] fix: address PR review findings - Use port-specific TOCTOU string matching instead of generic "in use"/"busy" - Verify external allocation state instead of blindly skipping - Add comment explaining why Step 2 is immune to TOCTOU - Log last error when no free port found in scan - Validate parsed port range (1-65535) in Sscanf test - Verify Name field preserved after re-lock Co-Authored-By: Claude Opus 4.6 --- cmd/port-selector/main_test.go | 35 ++++++++++++++++++++++++++++++---- 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/cmd/port-selector/main_test.go b/cmd/port-selector/main_test.go index 6e59458..c60878f 100644 --- a/cmd/port-selector/main_test.go +++ b/cmd/port-selector/main_test.go @@ -449,9 +449,11 @@ func TestLockPortSameDirectory_NoError(t *testing.T) { // to avoid flakiness when hardcoded ports are in use by other services. // Done after setup to minimize TOCTOU window before first --lock call. var freePort int + var lastErr error for p := 3000; p <= 4000; p++ { ln, err := net.Listen("tcp", fmt.Sprintf(":%d", p)) if err != nil { + lastErr = err continue } ln.Close() @@ -459,7 +461,7 @@ func TestLockPortSameDirectory_NoError(t *testing.T) { break } if freePort == 0 { - t.Skipf("could not find any free port in range 3000-4000") + t.Skipf("could not find any free port in range 3000-4000 (last error: %v)", lastErr) } portStr := fmt.Sprintf("%d", freePort) @@ -470,16 +472,35 @@ func TestLockPortSameDirectory_NoError(t *testing.T) { step1Output, err := cmd.CombinedOutput() if err != nil { outStr := string(step1Output) - if strings.Contains(outStr, "in use") || strings.Contains(outStr, "busy") { + portInUse := fmt.Sprintf("port %s is in use", portStr) + portBusy := fmt.Sprintf("port %d", freePort) + if strings.Contains(outStr, portInUse) || (strings.Contains(outStr, portBusy) && strings.Contains(outStr, "busy")) { t.Skipf("port %s became unavailable between discovery and lock (TOCTOU): %v, output: %s", portStr, err, outStr) } t.Fatalf("failed to lock port %s: %v, output: %s", portStr, err, outStr) } if strings.Contains(string(step1Output), "externally used") { - t.Skipf("port %s was claimed by external process between discovery and lock (TOCTOU): output: %s", portStr, step1Output) + // Port was grabbed by external process between discovery and lock (TOCTOU). + // Verify allocation was registered as external rather than skipping entirely. + store, loadErr := allocations.Load(configDir) + if loadErr != nil { + t.Fatalf("failed to load allocations after external registration: %v", loadErr) + } + alloc := store.FindByPort(freePort) + if alloc == nil { + t.Fatalf("expected external allocation for port %d, got none", freePort) + return // unreachable, but satisfies staticcheck SA5011 + } + if alloc.Status != "external" { + t.Errorf("expected status 'external', got %q", alloc.Status) + } + t.Skipf("port %s was claimed by external process (TOCTOU); verified external allocation", portStr) } - // Step 2: Lock same port again from same directory (should succeed without --force) + // Step 2: Lock same port again from same directory (should succeed without --force). + // No TOCTOU skip needed here: once Step 1 allocates the port to this directory, + // lockSpecificPort takes the alloc.Directory==cwd fast path (main.go:645-651) + // which updates lock status without checking port busyness. cmd = exec.Command(binary, "--lock", portStr) cmd.Dir = workDir cmd.Env = env @@ -505,6 +526,9 @@ func TestLockPortSameDirectory_NoError(t *testing.T) { if alloc.Directory != workDir { t.Errorf("expected port to belong to %s, got %s", workDir, alloc.Directory) } + if alloc.Name != "main" { + t.Errorf("expected name 'main' preserved after re-lock, got %q", alloc.Name) + } if !alloc.Locked { t.Error("expected port to remain locked after re-lock") } @@ -1284,6 +1308,9 @@ func TestPortSelector_ReturnsSamePortEvenWhenBusy(t *testing.T) { if _, err := fmt.Sscanf(initialPort, "%d", &portNum); err != nil { t.Fatalf("failed to parse port %q: %v", initialPort, err) } + if portNum < 1 || portNum > 65535 { + t.Fatalf("port-selector returned invalid port number: %d (raw output: %q)", portNum, initialPort) + } ln, err := net.Listen("tcp", ":"+initialPort) if err != nil { t.Skipf("could not occupy port %s for test: %v", initialPort, err) From 6bea085c67393d2e302d58c0b3cd0af6bee9fe35 Mon Sep 17 00:00:00 2001 From: Danil Pismenny Date: Fri, 27 Mar 2026 12:33:20 +0000 Subject: [PATCH 10/12] docs: update CLAUDE.md spec + fix help text and panic in RefreshExternalAllocations Deep spec review revealed CLAUDE.md covered only ~30% of actual functionality. CLAUDE.md updates: - Add all 15 undocumented CLI commands (--list, --forget, --scan, --lock, etc.) - Update Project Structure with debug/, docker/, pathutil/, procinfo.go - Document full AllocationInfo model (13 fields vs 5 in old spec) - Add sections: Named Allocations, External Allocations, Docker Integration, Process Discovery, Lock Decision Matrix - Document allocationTTL config, freezePeriod vs TTL semantics - Update logging events (AllocExternal, AllocRefresh) - Clarify STDOUT rules for different command modes Code fixes: - Fix help text: default.yaml -> config.yaml (was referencing wrong filename) - Replace panic with error return in RefreshExternalAllocations (CLI tools should not panic on nil arguments) Co-Authored-By: Claude Opus 4.6 (1M context) --- CLAUDE.md | 280 +++++++++++++++++++---- cmd/port-selector/main.go | 7 +- internal/allocations/allocations.go | 9 +- internal/allocations/allocations_test.go | 28 ++- 4 files changed, 265 insertions(+), 59 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 7d95ecd..f73eea2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -2,7 +2,7 @@ ## About the Project -**port-selector** — CLI utility in Go for automatic free port selection from a given range. Designed for AI-driven development with multiple parallel agents. +**port-selector** — CLI utility in Go for automatic free port selection from a given range. Designed for AI-driven development with multiple parallel agents. Supports named allocations, port locking, Docker container detection, and process discovery. ## Tech Stack @@ -15,16 +15,23 @@ ``` port-selector/ -├── cmd/port-selector/main.go # Entry point, argument parsing +├── cmd/port-selector/main.go # Entry point, argument parsing, CLI commands ├── internal/ │ ├── allocations/ # Port allocations with flock-based locking -│ │ ├── allocations.go # Store, Load, Save, WithStore +│ │ ├── allocations.go # Store, Load, Save, WithStore, CRUD operations │ │ ├── lock_unix.go # Unix flock implementation │ │ └── lock_windows.go # Windows stub (no locking) -│ ├── config/config.go # Read/create YAML config +│ ├── config/config.go # Read/create YAML config, duration parsing +│ ├── debug/debug.go # Debug logging (--verbose flag) +│ ├── docker/docker.go # Docker container detection and project directory resolution │ ├── logger/logger.go # Structured logging for state changes -│ └── port/checker.go # Port availability checking -├── .github/workflows/release.yml +│ ├── pathutil/pathutil.go # Path utilities (~ shortening) +│ └── port/ +│ ├── checker.go # Port availability checking, free port search +│ └── procinfo.go # Process discovery via /proc (Linux only) +├── .github/workflows/ +│ ├── ci.yml # Tests and linting on PRs +│ └── release.yml # Build and release on tags ├── .mise.toml ├── go.mod └── go.sum @@ -34,35 +41,186 @@ port-selector/ ### Functional -1. **No arguments** → outputs free port to STDOUT -2. **-h, --help** → help message -3. **-v, --version** → version (embedded at build via `-ldflags`) -4. **Config** in `~/.config/port-selector/config.yaml`: - ```yaml - portStart: 3000 - portEnd: 4000 - freezePeriod: 24h - # log: ~/.config/port-selector/port-selector.log - ``` -5. **Allocations** in `~/.config/port-selector/allocations.yaml`: - ```yaml - last_issued_port: 3005 - allocations: - 3000: - directory: /path/to/project - assigned_at: 2024-01-15T10:30:00Z - last_used_at: 2024-01-15T12:00:00Z - locked: false - ``` -6. **Wrap-around** — after reaching portEnd, start from portStart -7. **Error** to STDERR with exit code 1 if all ports are busy +#### Port Allocation (primary command) +1. **No arguments** → outputs free port to STDOUT for current directory +2. **`--name NAME`** → allocate a named port (default: "main"), allowing multiple ports per directory +3. Port is **stable per (directory, name)** — same directory+name always returns the same port +4. **Wrap-around** — after reaching portEnd, start from portStart +5. **Error** to STDERR with exit code 1 if all ports are busy or frozen + +#### Information Commands +6. **`-h, --help`** → help message +7. **`-v, --version`** → version (embedded at build via `-ldflags`) +8. **`-l, --list`** → show all allocations in table format (PORT, DIRECTORY, NAME, SOURCE, STATUS columns) +9. **`--verbose`** → enable debug output to STDERR (combinable with any command) + +#### Allocation Management +10. **`--forget`** → remove all allocations for current directory +11. **`--forget --name NAME`** → remove only the named allocation +12. **`--forget-all`** → remove all allocations globally +13. **`--scan`** → scan port range, detect busy ports, identify owning processes/containers +14. **`--refresh`** → remove stale external allocations (ports no longer in use) + +#### Port Locking +15. **`-c, --lock [PORT]`** → lock port for current directory and name (prevent reuse by others) +16. **`-u, --unlock [PORT]`** → unlock port +17. **`--force, -f`** → force lock a busy port or reassign a locked port from another directory + +#### Lock Decision Matrix (for `--lock PORT`) + +| Port State | Allocated To | Action | `--force` needed? | +|------------|-------------|--------|-------------------| +| Free | Current dir | Update lock status | No | +| Free | Other dir (unlocked) | Reassign to current dir | No | +| Free | Other dir (locked) | Reassign to current dir | **Yes** | +| Busy | Current dir | Lock (same dir owns it) | No | +| Busy | Other dir | **Block** (even with --force) | N/A | +| Busy | Not allocated (known process, same CWD) | Register + lock | No | +| Busy | Not allocated (known process, other CWD) | Register as external | No | +| Busy | Not allocated (unknown process) | Error | Yes (user takes responsibility) | +| Free | Not allocated | Allocate + lock | No | ### Non-functional -- Minimal dependencies (only Go stdlib + yaml parser) -- Fast startup (< 100ms) +- **Go module dependencies:** only `gopkg.in/yaml.v3` +- **Optional runtime dependency:** Docker CLI (for container detection in `--scan`) +- Fast startup (< 100ms for port allocation, `--scan` may be slower) - Flock-based file locking (to prevent race conditions on Unix) -- Platform support: Linux, macOS (Windows builds but without file locking) +- Platform support: Linux (full), macOS (port allocation works, process discovery limited), Windows (builds but no file locking) + +## Config + +**Location:** `~/.config/port-selector/config.yaml` + +```yaml +portStart: 3000 +portEnd: 4000 +freezePeriod: 24h +# allocationTTL: 30d +log: ~/.config/port-selector/port-selector.log +``` + +| Field | Default | Description | +|-------|---------|-------------| +| `portStart` | 3000 | Start of port range (1-65535) | +| `portEnd` | 4000 | End of port range (must be > portStart) | +| `freezePeriod` | 24h | Time to avoid reusing recently allocated ports (supports d/h/m/s) | +| `allocationTTL` | disabled | Auto-expire allocations after this duration (e.g., 30d, 720h) | +| `log` | ~/.config/port-selector/port-selector.log | Path to log file (empty to disable) | + +**Duration format:** supports `30d` (days), `720h` (hours), `30m` (minutes), standard Go duration. + +**freezePeriod vs allocationTTL:** +- `freezePeriod` — prevents reusing a port within this window (port stays "frozen" for other directories) +- `allocationTTL` — deletes the allocation record entirely after this period of inactivity +- Locked allocations are **never** expired by TTL + +**Legacy:** `freezePeriodMinutes` (integer) is supported for backward compatibility; `freezePeriod` takes precedence. + +## Allocations + +**Location:** `~/.config/port-selector/allocations.yaml` + +```yaml +last_issued_port: 3005 +allocations: + 3000: + directory: /home/user/project-a + name: main + assigned_at: 2024-01-15T10:30:00Z + last_used_at: 2024-01-15T12:00:00Z + locked: false + process_name: node + 3001: + directory: /home/user/project-a + name: api + assigned_at: 2024-01-15T11:00:00Z + last_used_at: 2024-01-15T12:00:00Z + locked: true + locked_at: 2024-01-15T11:00:00Z + 3002: + directory: /home/user/project-b + name: main + assigned_at: 2024-01-16T09:00:00Z + last_used_at: 2024-01-16T09:00:00Z + status: external + external_pid: 1234 + external_user: root + external_process_name: docker-proxy + container_id: abc123def456 +``` + +### AllocationInfo Fields + +| Field | Type | Description | +|-------|------|-------------| +| `directory` | string | Working directory of the project | +| `name` | string | Allocation name (default: "main") | +| `assigned_at` | time | When the allocation was created | +| `last_used_at` | time | Last time port was issued or updated | +| `locked` | bool | Whether port is locked (prevent reuse) | +| `locked_at` | time | When the port was locked | +| `process_name` | string | Name of the process using the port | +| `container_id` | string | Docker container ID (if applicable) | +| `status` | string | "" (normal) or "external" | +| `external_pid` | int | PID of external process | +| `external_user` | string | Username of external process owner | +| `external_process_name` | string | Name of external process | + +**Primary key:** port number (unique in the map). +**Lookup keys:** (directory), (directory, name), (port). +**Invariant:** at most one locked port per (directory, name) combination. + +## Named Allocations + +One directory can have multiple port allocations with different names: + +```bash +$ cd ~/project +$ port-selector --name web # → 3000 +$ port-selector --name api # → 3001 +$ port-selector --name db # → 3002 +$ port-selector --name web # → 3000 (stable) +``` + +- Default name is `"main"` (empty name normalizes to "main") +- `--forget` without `--name` removes **all** allocations for the directory +- `--forget --name api` removes only the "api" allocation +- Freeze period applies per-port, not per-name + +## External Allocations + +Ports used by processes not managed by port-selector: + +- **Created by:** `--scan` (discovers busy ports) or `--lock PORT` (when port is busy on another directory) +- **Status:** `status: external` in allocations.yaml +- **Stored info:** PID, username, process name, container ID +- **Cleanup:** `--refresh` removes stale external allocations (ports now free) +- **Display:** shown as `SOURCE=external` in `--list` + +## Docker Integration + +When a port is owned by `docker-proxy`, port-selector resolves the actual project directory: + +1. Find container by port: `docker ps --filter publish=PORT` +2. Try compose label: `com.docker.compose.project.working_dir` +3. Fallback to first bind mount source + +**Requires:** Docker CLI (`docker` command). Gracefully degrades if unavailable. + +## Process Discovery + +On Linux, `--scan` and `--lock` identify port owners via: + +1. Parse `/proc/net/tcp` and `/proc/net/tcp6` for listening sockets +2. Match socket inode to process via `/proc/*/fd/` +3. Read process name from `/proc/[pid]/comm`, CWD from `/proc/[pid]/cwd` +4. Resolve UID to username + +**Platform limitations:** +- **Linux:** full support (may need sudo for other users' processes) +- **macOS:** process discovery not available (`/proc` doesn't exist) +- **Windows:** process discovery not available ## Development Commands @@ -98,12 +256,20 @@ func IsPortFree(port int) bool { } ``` +**Known limitation (TOCTOU):** Port is checked as free, then released. Another process could take it between check and actual use. Callers should handle bind failures gracefully. + ### Config Handling - Use `os.UserConfigDir()` for cross-platform compatibility - Create directories with `os.MkdirAll(..., 0755)` - Use `gopkg.in/yaml.v3` for YAML +### Atomic Operations + +- `WithStore(configDir, fn)` — read-modify-write with flock (use for all mutations) +- `Load(configDir)` — read-only without lock (use for `--list`) +- `Save(configDir, store)` — atomic write via temp file + rename (no lock) + ### Error Handling ```go @@ -111,10 +277,17 @@ func IsPortFree(port int) bool { fmt.Fprintln(os.Stderr, "error: all ports in range are busy") os.Exit(1) -// Successful output to STDOUT (port only!) +// Successful port output to STDOUT (port only!) fmt.Println(port) + +// Informational output (--list, --forget, --lock, etc.) to STDOUT +fmt.Printf("Locked port %d for '%s' in %s\n", port, name, dir) ``` +**STDOUT rules:** +- Port allocation (no args, `--name`) → port number only +- Other commands (`--list`, `--forget`, `--lock`, etc.) → informational messages + ## Testing ### Unit Tests @@ -139,6 +312,12 @@ func TestFindFreePort(t *testing.T) { } ``` +### Platform-specific Tests + +- Process discovery tests (`procinfo_test.go`) skip on non-Linux with `runtime.GOOS` +- Docker tests gracefully handle missing Docker CLI +- Concurrent tests verify flock correctness with multiple goroutines + ## GitHub Actions ### Release Workflow @@ -169,6 +348,8 @@ logger.AllocLock // When allocation lock status changes logger.AllocDelete // When a single allocation is removed logger.AllocDeleteAll // When all allocations are cleared logger.AllocExpire // When allocation expires due to TTL +logger.AllocExternal // When registering an external port (from --scan or --lock) +logger.AllocRefresh // When refreshing external allocations (--refresh) ``` ### Usage Pattern @@ -177,7 +358,8 @@ logger.AllocExpire // When allocation expires due to TTL // Log state changes with relevant fields logger.Log(logger.AllocAdd, logger.Field("port", port), - logger.Field("dir", directory)) + logger.Field("dir", directory), + logger.Field("name", name)) // Field() auto-quotes values with spaces logger.Field("dir", "/path with spaces") // -> dir="/path with spaces" @@ -186,27 +368,45 @@ logger.Field("dir", "/path with spaces") // -> dir="/path with spaces" ### Requirements for New Code 1. **Any function that modifies allocations** must call `logger.Log()` with appropriate event type -2. **Include relevant context** using `logger.Field()` (port, directory, count, etc.) +2. **Include relevant context** using `logger.Field()` (port, directory, name, count, reason, etc.) 3. **Logger is safe when nil** — if logging is disabled, calls are no-op 4. **No need to check if logger is enabled** — just call `logger.Log()` ## Important Details -1. **STDOUT for port only** — no additional text -2. **File locking** — flock-based locking on Unix for concurrent access safety -3. **Graceful handling** — if no permissions for config, continue with defaults +1. **STDOUT for port only** (in port allocation mode) — no additional text. Other commands output informational messages. +2. **File locking** — flock-based locking on Unix for concurrent access safety. `WithStore` uses blocking `LOCK_EX`. Lock auto-releases on process exit. +3. **Graceful handling** — if no permissions for config, continue with defaults with warning to STDERR 4. **Don't block port** — only check and immediately close listener +5. **Directory-based persistence** — port is allocated per working directory. Same directory always returns the same port. +6. **Name normalization** — empty name normalizes to "main" for backward compatibility ## Usage Example ```bash -# Agent runs +# Basic: get a port for current directory $ port-selector 3000 -# Next agent -$ port-selector +# Named allocations for multiple services +$ port-selector --name web 3001 +$ port-selector --name api +3002 + +# Lock a port (prevent reuse) +$ port-selector --lock +Locked port 3000 for 'main' in ~/project + +# List all allocations +$ port-selector --list +PORT DIRECTORY NAME SOURCE STATUS +3000 ~/project-a main free locked +3001 ~/project-a api free +3002 ~/project-b main free + +# Scan for busy ports in range +$ port-selector --scan # Usage in script $ npm run dev -- --port $(port-selector) diff --git a/cmd/port-selector/main.go b/cmd/port-selector/main.go index 73dd6b8..29183a5 100644 --- a/cmd/port-selector/main.go +++ b/cmd/port-selector/main.go @@ -968,7 +968,7 @@ Port Locking: registered as an external allocation instead of failing. Configuration: - ~/.config/port-selector/default.yaml + ~/.config/port-selector/config.yaml Available options: portStart: 3000 # Start of port range @@ -1108,8 +1108,9 @@ func runRefresh() error { } fmt.Printf("Refreshing %d external allocation(s)...\n", totalCount) - removedCount = store.RefreshExternalAllocations(port.IsPortFree) - return nil + var refreshErr error + removedCount, refreshErr = store.RefreshExternalAllocations(port.IsPortFree) + return refreshErr }) if err != nil { diff --git a/internal/allocations/allocations.go b/internal/allocations/allocations.go index 7ba7a36..b86e590 100644 --- a/internal/allocations/allocations.go +++ b/internal/allocations/allocations.go @@ -941,11 +941,10 @@ func (s *Store) SetExternalAllocation(port int, pid int, user, processName, cwd // RefreshExternalAllocations removes stale external allocations (ports that are now free). // Updates LastUsedAt for allocations that are still active. -// Returns the count of removed allocations. -// Panics if isPortFree is nil (programming error). -func (s *Store) RefreshExternalAllocations(isPortFree PortChecker) int { +// Returns the count of removed allocations and an error if isPortFree is nil. +func (s *Store) RefreshExternalAllocations(isPortFree PortChecker) (int, error) { if isPortFree == nil { - panic("RefreshExternalAllocations: isPortFree function cannot be nil") + return 0, fmt.Errorf("RefreshExternalAllocations: isPortFree function cannot be nil") } var removedPorts []int @@ -991,5 +990,5 @@ func (s *Store) RefreshExternalAllocations(isPortFree PortChecker) int { logger.Field("updated", len(updatedPorts))) } - return len(removedPorts) + return len(removedPorts), nil } diff --git a/internal/allocations/allocations_test.go b/internal/allocations/allocations_test.go index 96f0f86..b115c13 100644 --- a/internal/allocations/allocations_test.go +++ b/internal/allocations/allocations_test.go @@ -2341,7 +2341,10 @@ func TestRefreshExternalAllocations_RemovesStale(t *testing.T) { return port == 3000 // 3000 is free, 3001 is busy } - removed := store.RefreshExternalAllocations(portChecker) + removed, err := store.RefreshExternalAllocations(portChecker) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } if removed != 1 { t.Errorf("expected 1 removed, got %d", removed) @@ -2644,7 +2647,10 @@ func TestRefreshExternalAllocations_KeepsActive(t *testing.T) { // Port is still busy portChecker := func(port int) bool { return false } - removed := store.RefreshExternalAllocations(portChecker) + removed, err := store.RefreshExternalAllocations(portChecker) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } if removed != 0 { t.Errorf("expected 0 removed, got %d", removed) @@ -2668,7 +2674,10 @@ func TestRefreshExternalAllocations_SkipsNonExternal(t *testing.T) { portChecker := func(port int) bool { return true } - removed := store.RefreshExternalAllocations(portChecker) + removed, err := store.RefreshExternalAllocations(portChecker) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } if removed != 0 { t.Errorf("expected 0 removed (non-external should be skipped), got %d", removed) @@ -2680,16 +2689,13 @@ func TestRefreshExternalAllocations_SkipsNonExternal(t *testing.T) { } } -func TestRefreshExternalAllocations_NilPortChecker_Panics(t *testing.T) { +func TestRefreshExternalAllocations_NilPortChecker_ReturnsError(t *testing.T) { store := NewStore() - defer func() { - if r := recover(); r == nil { - t.Error("expected panic with nil PortChecker, but did not panic") - } - }() - - store.RefreshExternalAllocations(nil) + _, err := store.RefreshExternalAllocations(nil) + if err == nil { + t.Error("expected error with nil PortChecker, but got nil") + } } func TestFindByPort_IncludesExternalFields(t *testing.T) { From 782f221c8246e0bbb6991a620ab8b064ba08755e Mon Sep 17 00:00:00 2001 From: Danil Pismenny Date: Fri, 27 Mar 2026 12:37:33 +0000 Subject: [PATCH 11/12] fix: lowercase error message in RefreshExternalAllocations per Go convention Co-Authored-By: Claude Opus 4.6 (1M context) --- internal/allocations/allocations.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/allocations/allocations.go b/internal/allocations/allocations.go index b86e590..d8dea4d 100644 --- a/internal/allocations/allocations.go +++ b/internal/allocations/allocations.go @@ -944,7 +944,7 @@ func (s *Store) SetExternalAllocation(port int, pid int, user, processName, cwd // Returns the count of removed allocations and an error if isPortFree is nil. func (s *Store) RefreshExternalAllocations(isPortFree PortChecker) (int, error) { if isPortFree == nil { - return 0, fmt.Errorf("RefreshExternalAllocations: isPortFree function cannot be nil") + return 0, fmt.Errorf("refresh external allocations: isPortFree function cannot be nil") } var removedPorts []int From f6f8bdc68801138e832d4a93c352a4f806d4abae Mon Sep 17 00:00:00 2001 From: Danil Pismenny Date: Fri, 27 Mar 2026 13:18:25 +0000 Subject: [PATCH 12/12] fix: add TOCTOU retry to TestLockPortSameDirectory_NoError Instead of skipping the test on first TOCTOU, retry with the next free port. This reduces flaky skips in busy environments (CI, parallel runs). Co-Authored-By: Claude Opus 4.6 (1M context) --- cmd/port-selector/main_test.go | 61 ++++++++++++---------------------- 1 file changed, 22 insertions(+), 39 deletions(-) diff --git a/cmd/port-selector/main_test.go b/cmd/port-selector/main_test.go index c60878f..3a85414 100644 --- a/cmd/port-selector/main_test.go +++ b/cmd/port-selector/main_test.go @@ -2,6 +2,7 @@ package main import ( "bytes" + "fmt" "net" "os" "os/exec" @@ -445,63 +446,45 @@ func TestLockPortSameDirectory_NoError(t *testing.T) { env := append(os.Environ(), "XDG_CONFIG_HOME="+filepath.Join(tmpDir, ".config")) - // Find a free port within the default configured range (3000-4000) - // to avoid flakiness when hardcoded ports are in use by other services. - // Done after setup to minimize TOCTOU window before first --lock call. + // Find a free port and lock it in one loop, retrying on TOCTOU. + // This avoids skipping the entire test when a single port becomes busy + // between discovery and lock — instead we try the next free port. var freePort int - var lastErr error for p := 3000; p <= 4000; p++ { ln, err := net.Listen("tcp", fmt.Sprintf(":%d", p)) if err != nil { - lastErr = err continue } ln.Close() + + portStr := fmt.Sprintf("%d", p) + cmd := exec.Command(binary, "--lock", portStr) + cmd.Dir = workDir + cmd.Env = env + output, err := cmd.CombinedOutput() + if err != nil { + outStr := string(output) + if strings.Contains(outStr, "is in use") || strings.Contains(outStr, "busy") { + continue // TOCTOU: port became busy, try next + } + t.Fatalf("failed to lock port %s: %v, output: %s", portStr, err, outStr) + } + if strings.Contains(string(output), "externally used") { + continue // TOCTOU: external process grabbed it, try next + } freePort = p break } if freePort == 0 { - t.Skipf("could not find any free port in range 3000-4000 (last error: %v)", lastErr) + t.Skipf("could not find and lock any free port in range 3000-4000") } portStr := fmt.Sprintf("%d", freePort) - // Step 1: Allocate and lock the port for project - cmd := exec.Command(binary, "--lock", portStr) - cmd.Dir = workDir - cmd.Env = env - step1Output, err := cmd.CombinedOutput() - if err != nil { - outStr := string(step1Output) - portInUse := fmt.Sprintf("port %s is in use", portStr) - portBusy := fmt.Sprintf("port %d", freePort) - if strings.Contains(outStr, portInUse) || (strings.Contains(outStr, portBusy) && strings.Contains(outStr, "busy")) { - t.Skipf("port %s became unavailable between discovery and lock (TOCTOU): %v, output: %s", portStr, err, outStr) - } - t.Fatalf("failed to lock port %s: %v, output: %s", portStr, err, outStr) - } - if strings.Contains(string(step1Output), "externally used") { - // Port was grabbed by external process between discovery and lock (TOCTOU). - // Verify allocation was registered as external rather than skipping entirely. - store, loadErr := allocations.Load(configDir) - if loadErr != nil { - t.Fatalf("failed to load allocations after external registration: %v", loadErr) - } - alloc := store.FindByPort(freePort) - if alloc == nil { - t.Fatalf("expected external allocation for port %d, got none", freePort) - return // unreachable, but satisfies staticcheck SA5011 - } - if alloc.Status != "external" { - t.Errorf("expected status 'external', got %q", alloc.Status) - } - t.Skipf("port %s was claimed by external process (TOCTOU); verified external allocation", portStr) - } - // Step 2: Lock same port again from same directory (should succeed without --force). // No TOCTOU skip needed here: once Step 1 allocates the port to this directory, // lockSpecificPort takes the alloc.Directory==cwd fast path (main.go:645-651) // which updates lock status without checking port busyness. - cmd = exec.Command(binary, "--lock", portStr) + cmd := exec.Command(binary, "--lock", portStr) cmd.Dir = workDir cmd.Env = env output, err := cmd.CombinedOutput()