feat: probe host ports before starting the dev environment - #1323
Draft
Soner (shyim) wants to merge 3 commits into
Draft
feat: probe host ports before starting the dev environment#1323Soner (shyim) wants to merge 3 commits into
Soner (shyim) wants to merge 3 commits into
Conversation
Before `project dev` runs `docker compose up -d`, probe every host port
the generated compose file will publish (skipping ports held by the
project's own containers). On conflict, the TUI offers to switch the
busy ports to random free ones; headless starts get a new
--on-port-conflict=fail|random flag. Chosen ports are persisted
per-key into .shopware-project.local.yml and reused on later runs.
All published host ports are now configurable via docker.ports in the
project config; a value of false disables publishing a port entirely.
Persistence uses a surgical yaml.Node read-modify-write that preserves
comments, ${VAR} references and unrelated keys — the old full-rewrite
WriteLocalConfig is removed and the profiler-secret save migrated onto
the new helper. WriteConfig strips docker.ports so machine-local ports
never leak into the committed config.
Also fixes two pre-existing config bugs this feature would trigger:
ReadConfig ignored the local override file when the base config is
missing, and the local-override merge corrupted unquoted
compatibility_date values into RFC3339 timestamps.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # internal/tui/dev/tab_overview.go
…t handling - Enforce 0600 on the local override file after every write: os.WriteFile only applies the mode to newly created files, so pre-existing files kept their (possibly world-readable) permissions while storing profiler secrets. - UpdateLocalDockerPHP now removes empty credential keys instead of only merging non-empty ones, and treats a nil config as clearing all known secrets, so rotated or disabled Blackfire/Tideways credentials no longer survive in the local override to be re-merged by ReadConfig. - fixPortConflicts no longer mutates the shared model config from the tea.Cmd goroutine (data race with UI reads): overrides are applied to a detached config copy with its own Ports map, compose.yaml is rewritten first, the local override is persisted only after that succeeds, and the overrides reach m.config in updateLifecycle on the success message only. The headless path gets the same write order. - The overview watcher URLs fall back to the platform dev server port (AdminDevServerPort, 5173/8080) and the fixed storefront proxy port in non-docker environments, where docker.ports does not apply. Tests cover the mode enforcement on existing files, secret removal on rotation/disable, the deferred override application, and both watcher URL modes.
Contributor
📝 WalkthroughWalkthroughThe change adds Docker host-port configuration, local override persistence, conflict detection, random port allocation, Compose regeneration, and CLI/TUI handling before development and migration startup. ChangesDocker port management
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Developer
participant DevelopmentCLI
participant DockerPortScanner
participant PortAllocator
participant ComposeGenerator
participant LocalConfig
Developer->>DevelopmentCLI: start development environment
DevelopmentCLI->>DockerPortScanner: find port conflicts
DockerPortScanner-->>DevelopmentCLI: return conflicts
DevelopmentCLI->>PortAllocator: allocate random replacements
PortAllocator-->>DevelopmentCLI: return port map
DevelopmentCLI->>ComposeGenerator: regenerate compose.yaml
DevelopmentCLI->>LocalConfig: persist Docker port overrides
DevelopmentCLI-->>Developer: start containers or report conflicts
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
internal/shop/config_local_test.go (1)
79-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
t.Context()instead ofcontext.Background().The coding guidelines require
t.Context()for test contexts. The sibling fileinternal/shop/config_test.goalready follows this. This file usescontext.Background()at lines 86, 98, 115, 202, and 234.After replacing all five call sites, remove the now-unused
contextimport at line 4.♻️ Proposed change for the two call sites in this range
- cfg, err := ReadConfig(context.Background(), configPath, false) + cfg, err := ReadConfig(t.Context(), configPath, false) require.NoError(t, err) require.NotNil(t, cfg.Docker) assert.Equal(t, ConfigDockerPort(52341), cfg.Docker.Ports[DockerPortWeb])- cfg, err := ReadConfig(context.Background(), configPath, true) + cfg, err := ReadConfig(t.Context(), configPath, true)As per coding guidelines: "use
t.Context()for test contexts".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/shop/config_local_test.go` around lines 79 - 118, Replace every context.Background() call in the tests of this file, including the calls in TestUpdateLocalDockerPortsRoundTripsThroughReadConfig and TestReadConfigMergesLocalFileWithoutBaseConfig, with the corresponding test’s t.Context(). Apply the same change to the other three call sites, then remove the unused context import.Source: Coding guidelines
internal/docker/compose_test.go (1)
164-169: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider parsing the YAML instead of slicing the compose text.
strings.Cut(compose, "adminer:")matches the first occurrence of the literaladminer:. If a future compose template emitsadminer:earlier, for example as a mapping key inside adepends_onblock on thewebservice, the slice would cover the wrong region and the assertion would pass without testing the adminer service.Unmarshaling the generated file and reading
services.adminer.portskeeps the assertion tied to the intended node.♻️ Proposed change
- // A service whose ports are all disabled loses its ports key entirely. - _, adminerSection, found := strings.Cut(compose, "adminer:") - assert.True(t, found) - adminerSection, _, found = strings.Cut(adminerSection, "mailer:") - assert.True(t, found) - assert.NotContains(t, adminerSection, "ports:") + // A service whose ports are all disabled loses its ports key entirely. + var parsed struct { + Services map[string]struct { + Ports []string `yaml:"ports"` + } `yaml:"services"` + } + assert.NoError(t, yaml.Unmarshal(result, &parsed)) + assert.Empty(t, parsed.Services["adminer"].Ports)This also removes the need for the
stringsimport if no other assertion uses it.Run the following script to confirm the current compose layout and whether
adminer:can appear before the service definition:#!/bin/bash # Description: Inspect the compose generation template for depends_on usage and service ordering. set -euo pipefail fd -t f 'compose' internal/docker # Map the compose generator. ast-grep outline internal/docker/compose.go --items all # Look for depends_on rendering and the adminer service block. rg -n -C 6 'depends_on|adminer' internal/docker --type=go -g '!*_test.go' # Check for an embedded compose template file. fd -e tmpl -e yaml -e yml . internal/docker --exec rg -n -C 4 'adminer|depends_on' {}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/docker/compose_test.go` around lines 164 - 169, Replace the text slicing around the generated compose output in the relevant test with YAML unmarshaling, then inspect the parsed services.adminer node and assert that its ports field is absent. Remove the strings import if it is no longer used, while preserving the existing validation that the adminer service exists.cmd/project/project_dev.go (1)
222-228: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
e.configPathfor consistency.Line 227 reads the package-level
projectConfigPath, while lines 243 and 250 in the same method reade.configPath. Both hold the same value today, because line 194 assignsprojectConfigPathto the field. Reading one source in the whole method prevents the error message from pointing at a different file than the one the method writes.♻️ Proposed change
- return fmt.Errorf("cannot start the development environment, host ports are already in use:\n%s\nrerun with --on-port-conflict=random to switch them to free ports, or set docker.ports in %s", strings.Join(lines, "\n"), projectConfigPath) + return fmt.Errorf("cannot start the development environment, host ports are already in use:\n%s\nrerun with --on-port-conflict=random to switch them to free ports, or set docker.ports in %s", strings.Join(lines, "\n"), e.configPath)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/project/project_dev.go` around lines 222 - 228, Update the error message in the port-conflict handling branch of the enclosing method to use the method’s existing e.configPath field instead of the package-level projectConfigPath variable, matching the references used by the later branches.internal/tui/dev/lifecycle_test.go (1)
358-363: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
require.NoErrorin the setup helper.
writeMinimalComposeProjectestablishes a precondition. If thecomposer.lockwrite fails,assert.NoErrorrecords the failure but lets the test continue.fixPortConflictsthen fails for a second, unrelated reason, which hides the root cause.♻️ Proposed change
func writeMinimalComposeProject(t *testing.T, dir string) { t.Helper() lock := `{"packages": [{"name": "shopware/core", "version": "6.6.0.0"}], "packages-dev": []}` - assert.NoError(t, os.WriteFile(filepath.Join(dir, "composer.lock"), []byte(lock), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "composer.lock"), []byte(lock), 0o644)) }Add
"github.com/stretchr/testify/require"to the import block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/dev/lifecycle_test.go` around lines 358 - 363, Update writeMinimalComposeProject to use require.NoError for the os.WriteFile result, and add the testify/require import so setup failures stop the test immediately before dependent logic runs.internal/shop/config_local.go (1)
109-142: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the credential key list from one source.
The credential keys appear twice: in the
secretsmap literal and in the loop slice. A future key must be added in both places, otherwise it is written but never cleared.♻️ Proposed dedupe
- secrets := map[string]string{ - "blackfire_server_id": "", - "blackfire_server_token": "", - "tideways_api_key": "", - } - if php != nil { - secrets["blackfire_server_id"] = php.BlackfireServerID - secrets["blackfire_server_token"] = php.BlackfireServerToken - secrets["tideways_api_key"] = php.TidewaysAPIKey - } + type credential struct { + key string + value string + } + credentials := []credential{ + {key: "blackfire_server_id"}, + {key: "blackfire_server_token"}, + {key: "tideways_api_key"}, + } + if php != nil { + credentials[0].value = php.BlackfireServerID + credentials[1].value = php.BlackfireServerToken + credentials[2].value = php.TidewaysAPIKey + }Then iterate
credentialsin the mutate function.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/shop/config_local.go` around lines 109 - 142, Define a single credential key list, such as credentials, and use it to initialize or populate the secrets map and to drive the mutation loop in updateLocalConfig. Remove the duplicated inline key slice while preserving the existing set-or-delete behavior for each credential.internal/docker/ports_test.go (1)
63-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
busyto match what it returns.
busyreturns anisFreepredicate. The callbusy(8000)reads as "8000 is busy", but the returned function reportsfalsefor 8000. The inversion inside the closure makes the helper harder to follow.♻️ Proposed rename
- busy := func(ports ...int) func(int) bool { + // freeExcept returns an isFree predicate that reports the given ports as busy. + freeExcept := func(ports ...int) func(int) bool {Update the call sites accordingly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/docker/ports_test.go` around lines 63 - 72, Rename the test helper busy to isFree to reflect that its returned predicate reports whether a port is available, and update every call site accordingly. Preserve the existing predicate behavior and port-set logic while removing the misleading busy naming.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/docker/ports.go`:
- Around line 156-163: Update ownPublishedPorts to determine the Compose project
name from the inherited COMPOSE_PROJECT_NAME or
shop.ReadComposeProjectName(projectFolder) and pass it to docker compose ps via
-p. Also update the full FindPortConflicts probe, including ownPublishedPorts
and port checks, to run under a bounded timeout even when called with
context.Background().
In `@internal/shop/config_local.go`:
- Around line 53-63: Update the local configuration write flow around
os.WriteFile to write the secret content to a same-directory temporary file with
mode 0600, then atomically rename it over localFile. Add the required filepath
handling, ensure temporary files are cleaned up on failure, and preserve the
existing wrapped errors for write and rename failures.
In `@internal/shop/config_override.go`:
- Around line 220-238: Update normalizeTimestampValue for time.Time values to
determine date-only status from the value’s local wall-clock fields, replacing
the v.Truncate comparison. Only format as time.DateOnly when the local hour,
minute, second, and nanosecond are all zero; otherwise preserve the RFC3339
formatting.
In `@internal/tui/dev/lifecycle_test.go`:
- Around line 341-344: Update the persistence assertion near the local override
file read in the lifecycle test to verify the allocated web port value from
result.overrides[shop.DockerPortWeb], rather than checking only for the
ambiguous "web:" key. Keep the existing file-read and error assertion, and
ensure the persisted content must contain the expected port value.
In `@internal/tui/dev/lifecycle.go`:
- Around line 128-131: Guard m.config before calling SetDockerPortOverrides in
the update path around startContainers: ensure a nil config is initialized or
handled safely before applying non-empty msg.overrides, matching the nil-config
behavior permitted by fixPortConflicts. Preserve the existing override
application and container-start flow when m.config is available.
---
Nitpick comments:
In `@cmd/project/project_dev.go`:
- Around line 222-228: Update the error message in the port-conflict handling
branch of the enclosing method to use the method’s existing e.configPath field
instead of the package-level projectConfigPath variable, matching the references
used by the later branches.
In `@internal/docker/compose_test.go`:
- Around line 164-169: Replace the text slicing around the generated compose
output in the relevant test with YAML unmarshaling, then inspect the parsed
services.adminer node and assert that its ports field is absent. Remove the
strings import if it is no longer used, while preserving the existing validation
that the adminer service exists.
In `@internal/docker/ports_test.go`:
- Around line 63-72: Rename the test helper busy to isFree to reflect that its
returned predicate reports whether a port is available, and update every call
site accordingly. Preserve the existing predicate behavior and port-set logic
while removing the misleading busy naming.
In `@internal/shop/config_local_test.go`:
- Around line 79-118: Replace every context.Background() call in the tests of
this file, including the calls in
TestUpdateLocalDockerPortsRoundTripsThroughReadConfig and
TestReadConfigMergesLocalFileWithoutBaseConfig, with the corresponding test’s
t.Context(). Apply the same change to the other three call sites, then remove
the unused context import.
In `@internal/shop/config_local.go`:
- Around line 109-142: Define a single credential key list, such as credentials,
and use it to initialize or populate the secrets map and to drive the mutation
loop in updateLocalConfig. Remove the duplicated inline key slice while
preserving the existing set-or-delete behavior for each credential.
In `@internal/tui/dev/lifecycle_test.go`:
- Around line 358-363: Update writeMinimalComposeProject to use require.NoError
for the os.WriteFile result, and add the testify/require import so setup
failures stop the test immediately before dependent logic runs.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 58406773-933a-4217-8b67-08e4eaf65d94
📒 Files selected for processing (21)
cmd/project/project_dev.gointernal/docker/compose.gointernal/docker/compose_test.gointernal/docker/ports.gointernal/docker/ports_test.gointernal/shop/config.gointernal/shop/config_docker_ports.gointernal/shop/config_local.gointernal/shop/config_local_test.gointernal/shop/config_override.gointernal/shop/config_schema.jsoninternal/tui/dev/lifecycle.gointernal/tui/dev/lifecycle_test.gointernal/tui/dev/model.gointernal/tui/dev/model_commands.gointernal/tui/dev/model_test.gointernal/tui/dev/model_update.gointernal/tui/dev/model_view.gointernal/tui/dev/overlay_port_conflict.gointernal/tui/dev/tab_overview.gointernal/tui/dev/tab_overview_test.go
Comment on lines
+156
to
+163
| func ownPublishedPorts(ctx context.Context, projectFolder string) map[int]struct{} { | ||
| cmd := exec.CommandContext(ctx, "docker", "compose", "ps", "--format", "json") | ||
| cmd.Dir = projectFolder | ||
|
|
||
| output, err := cmd.Output() | ||
| if err != nil { | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate the compose project-name resolution used by the executor and check whether ports.go can reuse it.
set -euo pipefail
rg -n -C5 'composeProjectName|ComposeProjectName|COMPOSE_PROJECT_NAME' --type=go
# Callers of FindPortConflicts and the contexts they pass.
rg -n -C4 'FindPortConflicts\s*\(' --type=goRepository: shopware/shopware-cli
Length of output: 159
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(ports\.go|docker\.go)$'
printf '%s\n' '--- ports.go symbols and context ---'
rg -n -C6 'ownPublishedPorts|FindPortConflicts|needStartMsg|docker.*compose|compose.*ps' internal/docker internal --type=go
printf '%s\n' '--- executor Docker project-name handling ---'
rg -n -C8 'COMPOSE_PROJECT_NAME|--project-name|-p[ ,]|project.name|ProjectName|compose' internal --type=goRepository: shopware/shopware-cli
Length of output: 50378
🏁 Script executed:
set -euo pipefail
log=/tmp/coderabbit-shell-logs/shell-output-0o7Jdf
printf '%s\n' '--- file list ---'
sed -n '1,30p' "$log"
printf '%s\n' '--- internal/docker/ports.go ---'
wc -l internal/docker/ports.go
cat -n internal/docker/ports.go | sed -n '1,230p'
printf '%s\n' '--- internal/executor/docker.go matches ---'
rg -n -C6 -- 'COMPOSE_PROJECT_NAME|--project-name|composeProjectName|ComposeProjectName' internal/executor/docker.go
printf '%s\n' '--- exact FindPortConflicts call sites ---'
rg -n -C8 --glob '*.go' 'FindPortConflicts[[:space:]]*\(' .Repository: shopware/shopware-cli
Length of output: 16600
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- Docker executor construction and project-name resolution ---'
cat -n internal/executor/docker.go | sed -n '1,180p'
printf '%s\n' '--- all composeArgs and DockerExecutor construction references ---'
rg -n -C5 --glob '*.go' 'composeArgs\(|DockerExecutor\{|NewDocker|composeProjectName' internal cmd
printf '%s\n' '--- project .env and Compose-name helpers ---'
rg -n -C5 --glob '*.go' 'dotenv|godotenv|Read.*Env|Load.*Env|filepath\.Join\(.*\.env|COMPOSE_PROJECT_NAME|project-name|ProjectName' internal cmdRepository: shopware/shopware-cli
Length of output: 50378
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- shared Compose project-name helper ---'
rg -n -C12 --glob '*.go' 'func ReadComposeProjectName|ReadComposeProjectName|ComposeProjectNameEnvKey' internal
printf '%s\n' '--- executor factory imports and constructor ---'
cat -n internal/executor/factory.go | sed -n '1,75p'
printf '%s\n' '--- startup probe timing and context usage ---'
cat -n internal/tui/dev/model_commands.go | sed -n '1,90p'
cat -n cmd/project/project_dev.go | sed -n '190,240p'
printf '%s\n' '--- existing bounded contexts in related code ---'
rg -n -C4 --glob '*.go' 'context\.WithTimeout|WithCancel|time\.Second|time\.Millisecond' internal/docker internal/tui/dev cmd/projectRepository: shopware/shopware-cli
Length of output: 49664
Pin the Compose project name and bound the full port probe.
When COMPOSE_PROJECT_NAME is not set in the process environment, resolve the name with shop.ReadComposeProjectName(projectFolder) and pass it to docker compose ps with -p. Otherwise, preserve the inherited environment value. Apply a timeout to the full FindPortConflicts probe, including docker compose ps and port checks, because needStartMsg passes context.Background() and a hung Docker CLI can block startup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/docker/ports.go` around lines 156 - 163, Update ownPublishedPorts to
determine the Compose project name from the inherited COMPOSE_PROJECT_NAME or
shop.ReadComposeProjectName(projectFolder) and pass it to docker compose ps via
-p. Also update the full FindPortConflicts probe, including ownPublishedPorts
and port checks, to run under a bounded timeout even when called with
context.Background().
Comment on lines
+53
to
+63
| if err := os.WriteFile(localFile, out, 0o600); err != nil { | ||
| return fmt.Errorf("writing local config %s: %w", localFile, err) | ||
| } | ||
|
|
||
| // WriteFile only applies the mode to newly created files (and umask can | ||
| // still strip bits), so enforce it explicitly: the local override holds | ||
| // profiler secrets that must not stay readable by other users on a | ||
| // pre-existing file. | ||
| if err := os.Chmod(localFile, 0o600); err != nil { | ||
| return fmt.Errorf("setting permissions on local config %s: %w", localFile, err) | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Write the local override atomically with restrictive permissions.
os.WriteFile writes the secret content before os.Chmod tightens the mode. If the file already exists with a permissive mode, the profiler credentials are readable by other users during that window. A truncated write also destroys the previous content when the process stops mid-write.
Write to a temporary file in the same directory with mode 0600, then rename it over the target.
♻️ Proposed atomic write
- if err := os.WriteFile(localFile, out, 0o600); err != nil {
- return fmt.Errorf("writing local config %s: %w", localFile, err)
- }
-
- // WriteFile only applies the mode to newly created files (and umask can
- // still strip bits), so enforce it explicitly: the local override holds
- // profiler secrets that must not stay readable by other users on a
- // pre-existing file.
- if err := os.Chmod(localFile, 0o600); err != nil {
- return fmt.Errorf("setting permissions on local config %s: %w", localFile, err)
- }
+ // The local override holds profiler secrets, so the content must never be
+ // visible under a pre-existing permissive mode. Create the temporary file
+ // with 0600 (and chmod it, because umask can strip bits) before the
+ // rename publishes it.
+ tmp, err := os.CreateTemp(filepath.Dir(localFile), ".shopware-local-*.yml")
+ if err != nil {
+ return fmt.Errorf("writing local config %s: %w", localFile, err)
+ }
+ tmpName := tmp.Name()
+ defer func() {
+ _ = tmp.Close()
+ _ = os.Remove(tmpName)
+ }()
+
+ if err := tmp.Chmod(0o600); err != nil {
+ return fmt.Errorf("setting permissions on local config %s: %w", localFile, err)
+ }
+ if _, err := tmp.Write(out); err != nil {
+ return fmt.Errorf("writing local config %s: %w", localFile, err)
+ }
+ if err := tmp.Close(); err != nil {
+ return fmt.Errorf("writing local config %s: %w", localFile, err)
+ }
+ if err := os.Rename(tmpName, localFile); err != nil {
+ return fmt.Errorf("writing local config %s: %w", localFile, err)
+ }Add "path/filepath" to the imports.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if err := os.WriteFile(localFile, out, 0o600); err != nil { | |
| return fmt.Errorf("writing local config %s: %w", localFile, err) | |
| } | |
| // WriteFile only applies the mode to newly created files (and umask can | |
| // still strip bits), so enforce it explicitly: the local override holds | |
| // profiler secrets that must not stay readable by other users on a | |
| // pre-existing file. | |
| if err := os.Chmod(localFile, 0o600); err != nil { | |
| return fmt.Errorf("setting permissions on local config %s: %w", localFile, err) | |
| } | |
| // The local override holds profiler secrets, so the content must never be | |
| // visible under a pre-existing permissive mode. Create the temporary file | |
| // with 0600 (and chmod it, because umask can strip bits) before the | |
| // rename publishes it. | |
| tmp, err := os.CreateTemp(filepath.Dir(localFile), ".shopware-local-*.yml") | |
| if err != nil { | |
| return fmt.Errorf("writing local config %s: %w", localFile, err) | |
| } | |
| tmpName := tmp.Name() | |
| defer func() { | |
| _ = tmp.Close() | |
| _ = os.Remove(tmpName) | |
| }() | |
| if err := tmp.Chmod(0o600); err != nil { | |
| return fmt.Errorf("setting permissions on local config %s: %w", localFile, err) | |
| } | |
| if _, err := tmp.Write(out); err != nil { | |
| return fmt.Errorf("writing local config %s: %w", localFile, err) | |
| } | |
| if err := tmp.Close(); err != nil { | |
| return fmt.Errorf("writing local config %s: %w", localFile, err) | |
| } | |
| if err := os.Rename(tmpName, localFile); err != nil { | |
| return fmt.Errorf("writing local config %s: %w", localFile, err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/shop/config_local.go` around lines 53 - 63, Update the local
configuration write flow around os.WriteFile to write the secret content to a
same-directory temporary file with mode 0600, then atomically rename it over
localFile. Add the required filepath handling, ensure temporary files are
cleaned up on failure, and preserve the existing wrapped errors for write and
rename failures.
Comment on lines
+220
to
+238
| func normalizeTimestampValue(value any) any { | ||
| switch v := value.(type) { | ||
| case time.Time: | ||
| if v.Equal(v.Truncate(24 * time.Hour)) { | ||
| return v.Format(time.DateOnly) | ||
| } | ||
| return v.Format(time.RFC3339) | ||
| case map[string]any: | ||
| normalizeTimestamps(v) | ||
| return v | ||
| case []any: | ||
| for i, item := range v { | ||
| v[i] = normalizeTimestampValue(item) | ||
| } | ||
| return v | ||
| } | ||
|
|
||
| return value | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Compare the wall-clock fields instead of Truncate.
Truncate rounds against the zero time in UTC, not against the value's own location. A timestamp such as 2026-08-01T02:00:00+02:00 is midnight in UTC, so v.Equal(v.Truncate(24 * time.Hour)) is true and the value is re-marshalled as the date-only string 2026-08-01. The time-of-day is lost.
Check the clock fields in the value's own location.
🐛 Proposed fix
case time.Time:
- if v.Equal(v.Truncate(24 * time.Hour)) {
+ if v.Hour() == 0 && v.Minute() == 0 && v.Second() == 0 && v.Nanosecond() == 0 {
return v.Format(time.DateOnly)
}
return v.Format(time.RFC3339)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func normalizeTimestampValue(value any) any { | |
| switch v := value.(type) { | |
| case time.Time: | |
| if v.Equal(v.Truncate(24 * time.Hour)) { | |
| return v.Format(time.DateOnly) | |
| } | |
| return v.Format(time.RFC3339) | |
| case map[string]any: | |
| normalizeTimestamps(v) | |
| return v | |
| case []any: | |
| for i, item := range v { | |
| v[i] = normalizeTimestampValue(item) | |
| } | |
| return v | |
| } | |
| return value | |
| } | |
| func normalizeTimestampValue(value any) any { | |
| switch v := value.(type) { | |
| case time.Time: | |
| if v.Hour() == 0 && v.Minute() == 0 && v.Second() == 0 && v.Nanosecond() == 0 { | |
| return v.Format(time.DateOnly) | |
| } | |
| return v.Format(time.RFC3339) | |
| case map[string]any: | |
| normalizeTimestamps(v) | |
| return v | |
| case []any: | |
| for i, item := range v { | |
| v[i] = normalizeTimestampValue(item) | |
| } | |
| return v | |
| } | |
| return value | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/shop/config_override.go` around lines 220 - 238, Update
normalizeTimestampValue for time.Time values to determine date-only status from
the value’s local wall-clock fields, replacing the v.Truncate comparison. Only
format as time.DateOnly when the local hour, minute, second, and nanosecond are
all zero; otherwise preserve the RFC3339 formatting.
Comment on lines
+341
to
+344
| // ...but the override is persisted to the local override file... | ||
| localContent, err := os.ReadFile(filepath.Join(dir, ".shopware-project.local.yml")) | ||
| assert.NoError(t, err) | ||
| assert.Contains(t, string(localContent), "web:") |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the persisted port value, not just the key.
Line 344 checks only for the substring web:. That substring also matches web_alt: and mailer_web:, and it does not prove the allocated port reached the local override file. The test already holds the expected value in result.overrides[shop.DockerPortWeb], and line 349 uses it for the compose assertion. Use it here too, so a wrong persisted value fails the test.
♻️ Proposed change
// ...but the override is persisted to the local override file...
localContent, err := os.ReadFile(filepath.Join(dir, ".shopware-project.local.yml"))
assert.NoError(t, err)
- assert.Contains(t, string(localContent), "web:")
+ assert.Contains(t, string(localContent), fmt.Sprintf("web: %d", result.overrides[shop.DockerPortWeb]))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // ...but the override is persisted to the local override file... | |
| localContent, err := os.ReadFile(filepath.Join(dir, ".shopware-project.local.yml")) | |
| assert.NoError(t, err) | |
| assert.Contains(t, string(localContent), "web:") | |
| // ...but the override is persisted to the local override file... | |
| localContent, err := os.ReadFile(filepath.Join(dir, ".shopware-project.local.yml")) | |
| assert.NoError(t, err) | |
| assert.Contains(t, string(localContent), fmt.Sprintf("web: %d", result.overrides[shop.DockerPortWeb])) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui/dev/lifecycle_test.go` around lines 341 - 344, Update the
persistence assertion near the local override file read in the lifecycle test to
verify the allocated web port value from result.overrides[shop.DockerPortWeb],
rather than checking only for the ambiguous "web:" key. Keep the existing
file-read and error assertion, and ensure the persisted content must contain the
expected port value.
Comment on lines
+128
to
+131
| // Apply the overrides to the shared config here on the update thread; | ||
| // the command goroutine only touched detached copies. | ||
| m.config.SetDockerPortOverrides(msg.overrides) | ||
| return m, m.startContainers() |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard m.config before applying the overrides.
SetDockerPortOverrides dereferences its receiver after the len(ports) == 0 check. On the success path msg.overrides is never empty, so a nil m.config panics here. fixPortConflicts in internal/tui/dev/model_commands.go treats a nil config as reachable, so the two paths disagree.
Either build the config in New so it is never nil, or guard here.
🛡️ Proposed guard
+ if m.config == nil {
+ m.config = &shop.Config{}
+ }
m.config.SetDockerPortOverrides(msg.overrides)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Apply the overrides to the shared config here on the update thread; | |
| // the command goroutine only touched detached copies. | |
| m.config.SetDockerPortOverrides(msg.overrides) | |
| return m, m.startContainers() | |
| // Apply the overrides to the shared config here on the update thread; | |
| // the command goroutine only touched detached copies. | |
| if m.config == nil { | |
| m.config = &shop.Config{} | |
| } | |
| m.config.SetDockerPortOverrides(msg.overrides) | |
| return m, m.startContainers() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui/dev/lifecycle.go` around lines 128 - 131, Guard m.config before
calling SetDockerPortOverrides in the update path around startContainers: ensure
a nil config is initialized or handled safely before applying non-empty
msg.overrides, matching the nil-config behavior permitted by fixPortConflicts.
Preserve the existing override application and container-start flow when
m.config is available.
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
2 participants
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.
What changed?
project devnow checks whether the host ports the generatedcompose.yamlwill publish are actually free before runningdocker compose up -d(ports already held by the project's own containers are excluded, so restarts never false-alarm).On conflict:
project dev): a modal lists the busy ports and offers Use random free ports / Quit.project dev start, orproject devwithout a TTY): new--on-port-conflict=fail|randomflag.fail(default) aborts with a descriptive error,randomremaps only the conflicting ports.Chosen ports are persisted per-key into
.shopware-project.local.yml(or.local.yaml) and reused on every later run. All published host ports are also directly configurable now, andfalsedisables publishing a port entirely:CLI output with port 8000 occupied (default
failmode):With
--on-port-conflict=random:Implementation notes:
internal/docker/ports.goholds the single source of truth for all 12 published ports (web 8000/8080/9999/9998/5173/5773, adminer, mailpit, conditional lavinmq/opensearch), the availability probe, and the random allocator (all listeners held open until every port is picked, so no duplicates).internal/shop/config_local.go) that preserves comments,${VAR}references, and unrelated keys. The TUI profiler-secret save was migrated onto it; the old full-file-rewriteWriteLocalConfig(which would have clobbered the ports) is removed.WriteConfigstripsdocker.portsso machine-local ports never leak into the committed config.ReadConfigignored.shopware-project.local.ymlentirely when the base config file is missing, and the local-override merge corrupted unquotedcompatibility_datevalues into RFC3339 timestamps (yaml.v3!!timestampresolution during themap[string]anyround-trip).Why?
The dev environment binds a lot of host ports and anything already listening (a second project, a stray
php -S, another mailpit) madedocker compose upfail with a raw error. Now conflicts are detected up front and can be resolved automatically, with the resolution remembered per machine without touching the committed config.How was this tested?
go test ./...andgolangci-lint run ./...are green;internal/shop/config_schema.jsonregenerated.ports:key dropped when a service has none left), local-overlay persistence (creates 0600 file, preserves comments/${VAR}/secrets,.yamlnaming,ReadConfiground-trip, unquoted-date regression), TUI lifecycle (conflict → prompt, quit, fix → start, fix error).failmode error above,randommode remap + persisted overlay + regenerated compose (60636:8000), rerun with 8000 still busy correctly finds no conflict (override is probed instead), andadminer: falseremoves theports:key from the service.Related issue or discussion
—
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
failandrandomconflict-handling modes.Bug Fixes