feat(proxy): run multiple local shops in parallel behind a shared proxy - #1208
feat(proxy): run multiple local shops in parallel behind a shared proxy#1208Tomasz Turkowski (tturkowski) wants to merge 26 commits into
Conversation
4b3b422 to
ab10413
Compare
|
Whats definitively missing here is:
|
Dev watchers through the shared proxyA quick summary of how the admin/storefront watchers can work behind the shared reverse proxy. Admin watcher — works as-is, no code changesThe admin is Vite-only. Vite works out its own HMR connection from the page it's loaded on, so all that was needed:
You open Storefront watcher (now) — webpack + a small runtime patchThe storefront's classic watcher (HMR + webpack, Rather than patching vendor file, we inject a tiny preload script when launching the watcher (Node Result: the storefront watcher runs fully through the proxy - multiple shops in parallel, clean port-free hostnames, no exposed ports, and no change to Shopware or the shop. You browse Tradeoff: it's a runtime patch — clever but hidden, and it leans on the internals of Storefront watcher (future) — Vite, the clean pathFrom 6.7.11 the storefront also ships a Vite dev server. To make that work behind a reverse proxy we need a small, fully backward-compatible contribution to
With that, the storefront watcher - once enabled, works at the shop's own URL ( Plan proposal:
|
|
btw because of excactly those REASONS I DONT WANT TO have those watchers directly inside Shopware. we're like now screwed |
54c9305 to
cae4216
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a new shopware-cli project proxy command group and supporting infrastructure to run multiple local Shopware projects in parallel behind a shared Traefik reverse proxy, using stable per-project hostnames with local DNS resolution and trusted HTTPS.
Changes:
- Adds a new
internal/proxysubsystem (DNS daemon, resolver configuration, Traefik management, verification, trust store integration, registry/settings state). - Implements proxy-mode Docker Compose overrides (marker-guarded
compose.override.yaml) to remove fixed host ports and route by hostname via Traefik. - Integrates proxy awareness into
project create,project dev, the dev TUI overview, and storefront watcher routing.
Reviewed changes
Copilot reviewed 69 out of 70 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/tui/dev/tab_overview_health.go | Adds proxy-related setup health checks to the TUI overview health panel. |
| internal/tui/dev/tab_overview_format_test.go | Adds tests for overview formatting helpers used by the TUI. |
| internal/tui/dev/model.go | Adds proxy fallback handling and an interactive proxy setup execution path in the TUI. |
| internal/system/dockername.go | Introduces Docker Compose project-name validation helper. |
| internal/proxy/wsl_resolver_test.go | Adds unit tests for WSL DNS resolver guidance text. |
| internal/proxy/windows_access.go | Adds WSL/Windows browser access guidance and hostname list generation. |
| internal/proxy/windows_access_test.go | Adds tests for Windows access guidance and hostname generation. |
| internal/proxy/verify.go | Implements proxy verify bottom-up checks (Docker → DNS → OS resolver → Traefik → trusted HTTPS). |
| internal/proxy/verify_windows.go | Windows stub for OS-level resolution check. |
| internal/proxy/verify_test.go | Adds tests for verification hints and probe-hostname generation. |
| internal/proxy/verify_linux.go | Linux OS-resolution check via getent hosts. |
| internal/proxy/verify_darwin.go | macOS OS-resolution check via dscacheutil. |
| internal/proxy/trust.go | Implements CA trust installation flow and user guidance (mkcert/truststore). |
| internal/proxy/trust_test.go | Adds tests for trust-blocked guidance content. |
| internal/proxy/traefik.go | Adds Traefik container/network lifecycle management and hostname alias reconciliation. |
| internal/proxy/traefik_test.go | Adds tests for Traefik dynamic config writing and alias helpers. |
| internal/proxy/stats.go | Adds proxy instance stats collection for the TUI (shop count + memory sum). |
| internal/proxy/stats_test.go | Adds tests for parsing Docker memory usage strings. |
| internal/proxy/statedir.go | Adds a shared state directory helper for proxy state files. |
| internal/proxy/settings.go | Adds machine-wide proxy settings (base domain) with validation and persistence. |
| internal/proxy/settings_test.go | Adds tests for settings validation and persistence round-trips. |
| internal/proxy/resolver.go | Adds resolver status types and Linux-without-systemd-resolved guidance text. |
| internal/proxy/resolver_windows.go | Windows resolver stubs reporting unsupported behavior. |
| internal/proxy/resolver_linux.go | Linux systemd-resolved split-DNS configuration (configure/unconfigure + guidance). |
| internal/proxy/resolver_linux_test.go | Adds tests for Linux resolver blocked guidance content. |
| internal/proxy/resolver_darwin.go | macOS /etc/resolver configuration (configure/unconfigure + guidance). |
| internal/proxy/resolver_darwin_test.go | Adds tests for macOS resolver blocked guidance content. |
| internal/proxy/registry.go | Adds registry state for registered projects and restore metadata. |
| internal/proxy/registry_test.go | Adds registry behavior tests (upsert/remove/find/round-trip). |
| internal/proxy/projectconfig.go | Adds comment-preserving YAML URL rewrite/restore logic for .shopware-project.yml. |
| internal/proxy/projectconfig_test.go | Adds tests for URL rewrite/restore semantics and missing-file behavior. |
| internal/proxy/hostname.go | Adds proxy hostname derivation (from config URL or directory name). |
| internal/proxy/hostname_test.go | Adds tests for hostname derivation edge cases. |
| internal/proxy/docker.go | Adds Docker Compose version check for !reset support and docker runner helper. |
| internal/proxy/dns.go | Adds embedded DNS server implementation and direct-query helper for verification/tests. |
| internal/proxy/dns_test.go | Adds tests for DNS zone behavior and garbage packet handling. |
| internal/proxy/dns_daemon.go | Adds non-Windows DNS daemon spawning/management via self re-exec and PID/state files. |
| internal/proxy/dns_daemon_windows.go | Adds Windows stubs and shared “not supported” error for DNS daemon operations. |
| internal/proxy/cert.go | Adds mkcert-compatible CA/cert management and SAN host list generation. |
| internal/proxy/cert_test.go | Adds tests for certificate creation, idempotency, and regeneration triggers. |
| internal/proxy/canonical.go | Adds canonical project-root resolution (symlink normalization). |
| internal/mkcert/mkcert.go | Adds BSD-licensed mkcert-derived CA/certificate implementation as an internal library. |
| internal/mkcert/mkcert_test.go | Adds tests for CAROOT behavior, CA creation/reuse, cert issuance, and keyless mode. |
| internal/mkcert/LICENSE | Adds the mkcert BSD license text for the adapted code. |
| internal/extension/storefront_watch.go | Adds proxy-mode support for the deprecated storefront hot-proxy watcher via env + Node preload. |
| internal/extension/storefront_hmr_patch.cjs | Adds a managed Node preload patch to rewrite webpack-dev-server websocket target behind the proxy. |
| internal/executor/docker.go | Improves watcher shutdown by SIGINTing the full in-container process tree (not just the wrapper process). |
| internal/envfile/upsert.go | Adds an env-file “upsert var” helper for surgical .env updates. |
| internal/envfile/upsert_test.go | Adds tests for env var upsert and read behavior. |
| internal/docker/compose.go | Adds a YAML boolean-node helper used by proxy compose override generation. |
| internal/docker/compose_test.go | Adds a regression test ensuring base compose output remains non-proxy (ports/labels absent). |
| internal/docker/compose_override.go | Adds generation + write/remove for marker-guarded proxy compose overrides with Traefik routes. |
| internal/docker/compose_override_test.go | Adds extensive tests for override content and safety checks (refuse user override files). |
| go.mod | Adds github.com/smallstep/truststore and an indirect plist dependency for trust installation. |
| go.sum | Adds checksums for new module dependencies. |
| docs/proxy.md | Adds end-to-end architecture/design documentation for the shared proxy feature set. |
| cmd/root.go | Treats ErrProxyNotRegistered as a user-facing error (exit 1 without extra logging). |
| cmd/project/project_storefront_watch.go | Routes storefront watcher through proxy hostname when the project is proxied. |
| cmd/project/project_proxy_verify.go | Adds project proxy verify command and shared output printer for verification steps. |
| cmd/project/project_proxy_test.go | Adds tests for local-domain choice resolution and Shopware command availability detection. |
| cmd/project/project_proxy_setup.go | Adds project proxy setup and teardown, including DNS/trust installation and verification. |
| cmd/project/project_proxy_list.go | Adds project proxy list and status commands with running-instance detection and links. |
| cmd/project/project_proxy_dns_serve.go | Adds hidden internal subcommand used as the DNS daemon re-exec target. |
| cmd/project/project_dev.go | Bootstraps proxy infra for proxy-mode projects with a non-blocking fallback to port mode. |
| cmd/project/project_dev_test.go | Adds tests for proxy-project detection and local-domain hostname normalization. |
| cmd/project/project_create.go | Adds --local-domain support, inline (prompted) one-time setup option, and base-domain lookup. |
| cmd/project/project_create_install.go | Writes proxy hostname URLs into the created project config and updates create summary output. |
| cmd/project/project_create_form.go | Extends interactive create form to prompt for local domains and optional one-time machine setup. |
Suppressed comments (1)
internal/proxy/hostname.go:33
- ProjectHostname can produce invalid DNS hostnames when the project directory contains underscores (Docker Compose allows them, DNS labels do not). Sanitizing underscores to dashes here keeps hostnames valid and matches the behavior in project create (localDomainHostname).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Works same way as it works on mac or windows, we do serve dns and in wsl we redirect domain traffic to our dns that resolves to 127.0.0.1 and then traefik takes care about the rest. WSL users who want to access WSL-running shops from Windows browser would need to one time setup Windows's hosts file.
Made them work, but it was a hard part. See comment above with details.
Made that work, containers will resolve from domain to the container, no need of container names usage. |
|
Follow-up from our chat:
|
232eb49 to
7c64ea1
Compare
Adds `shopware-cli project proxy` (setup/up/down/list/status/verify/ teardown): a shared Traefik container routes stable hostnames like https://shop1.shopware.local to local projects, so shops publish no host ports and any number can run at once. - embedded wildcard DNS server (x/net/dns) on 127.0.0.1:53535, wired via /etc/resolver (macOS) or systemd-resolved (Linux) by a one-time `proxy setup` with a single sudo ceremony (--domain, --skip-trust) - trusted HTTPS out of the box: mkcert-compatible local CA, per-project wildcard SANs, trust-store install via smallstep/truststore - proxy mode is a marker-guarded compose.override.yaml (ports cleared with !reset, requires Compose >= 2.24); the base compose.yaml stays untouched, so `project dev` and manual docker compose keep working in both modes - `up` points APP_URL, the sales channel domain and the url keys in .shopware-project.yml at the proxy; `down` restores everything exactly - `verify` checks the whole chain bottom-up with actionable hints, including guidance when sudo is blocked or systemd-resolved is missing - docs/proxy.md explains the architecture, decisions and trade-offs Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…cuit verify on Windows
…her port Drop the version-gated adminWatchLocalPort in favor of the shared extension.AdminDevServerPort (from #1288), which reads the ADMIN_VITE flag instead of guessing from the Shopware version — correct for a 6.6 shop that opted into Vite.
next tightened .golangci.yml (perfsprint): replace fmt.Sprintf/Errorf with strconv/errors/string concatenation in the proxy, docker-compose, and mkcert code.
7c64ea1 to
c26ca18
Compare
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
Hi Tomasz Turkowski (@tturkowski), I tested this end-to-end — it works well overall. Here's my QA report with a few issues I ran into along the way. 🙏 SummaryIt works. I ran three shops at the same time ( Both ways of using it work:
The setup step, the on/off commands, the status/list views and the teardown all behaved. The new-shop flow is smooth. The rough edges are almost all in the "turn the proxy on for an existing Issues I ran into (worst first)1. Turning the proxy on for an existing shop can quietly point it at an empty database 🔴 HighWhat I did: ran What I expected: the shop keeps using its own data. What happened: the shop opened fine, but later — while clicking around — it broke with Why it matters: it looks fine at first and only breaks later, in use — so it's easy to miss and 2. Turning the proxy on can fail with a confusing message and leave things half-started 🔴 HighWhat I did: ran What I expected: either it works, or it tells me clearly what's missing. What happened: it failed with Why it matters: the message gives the user nothing to act on, and the leftover half-started state is 3. Turning the proxy on is blocked on almost every real existing shop 🟠 MediumWhat I did: ran What I expected: it just turns the proxy on. What happened: it stopped with an error because the shop already has a Why it matters: this is the headline "turn it on for an existing project" feature, and it needs a 4. A running shop shows as "stopped" in the list 🟠 MediumWhat I did: created a new shop with What I expected: the list shows it as running (it opens fine in the browser and returns pages). What happened: the list showed it as stopped, even though it was clearly running and serving the Why it matters: the status display can't be trusted for shops started this way — it says "stopped" 5. On/off/teardown sometimes dumps a huge error page but keeps working 🟠 MediumWhat I did: ran What I expected: a short success message. What happened: it printed roughly 100 lines of red error/stack trace about a "duplicate entry" for Why it matters: it's alarming and looks broken, so a user is likely to think the command failed when 6. Health check looks like a failure when the proxy simply isn't running yet 🟡 LowWhat I did: ran What I expected: something like "proxy isn't running — start it first." What happened: it reported a red Why it matters: a first-time user could read this as a real fault when nothing is actually wrong. One note on my testingIssues 2, 3, 4 I reproduced cleanly from a normal starting point. Issues 1 and 5 appeared |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (22)
docs/proxy.md-20-21 (1)
20-21: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winQualify the in-container HTTPS claim.
The TL;DR states that shops can reach each other over HTTPS from inside their containers. Line 234-239 and Line 350-353 state that PHP/curl does not trust the proxy CA by default. Restrict this claim to clients with the mounted trust configuration, or state the PHP/curl prerequisite here.
🤖 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 `@docs/proxy.md` around lines 20 - 21, Update the in-container HTTPS claim in the TL;DR to qualify that it applies only to clients with the proxy CA trust configuration mounted, including the PHP/curl trust prerequisite stated elsewhere in the document.docs/proxy.md-233-233 (1)
233-233: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the reported Markdown lint errors.
- Keep
>on the blank line at Line 233, or remove the blank line, so the blockquote remains valid.- Add a language identifier to the fence at Line 319, such as
powershell.- Add a language identifier to the fence at Line 330, such as
text.Also applies to: 319-319, 330-330
🤖 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 `@docs/proxy.md` at line 233, Fix the Markdown lint issues in docs/proxy.md: preserve the blockquote by keeping the `>` marker on the blank line near line 233 or removing that blank line, and add appropriate language identifiers to the code fences near lines 319 and 330, using powershell and text respectively.Source: Linters/SAST tools
docs/proxy.md-51-55 (1)
51-55: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the absolute “no half-state” claim.
proxy setup --skip-trustexplicitly permits DNS setup without CA trust, as documented at Line 177-179. Change this text to state that the default setup avoids the half-state, with--skip-trustas the documented exception.🤖 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 `@docs/proxy.md` around lines 51 - 55, Update the proxy setup documentation paragraph to qualify the claim: state that the default setup configures trusted HTTPS together, while explicitly identifying proxy setup --skip-trust as the supported exception that permits DNS without CA trust.internal/tui/dev/tab_overview.go-416-423 (1)
416-423: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winShow a load failure instead of "No projects registered yet."
loadInstancesdiscards the error fromproxy.InstanceStats. If Docker is unavailable or the registry read fails,instancesis empty and the section states that no projects are registered. Keep the error ininstancesLoadedMsgand render it, so the user can tell a failure from an empty registry.Also applies to: 685-692
🤖 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/tab_overview.go` around lines 416 - 423, Update loadInstances to retain the error returned by proxy.InstanceStats and include it in instancesLoadedMsg. Extend the message handling and overview rendering to display that error instead of the empty-registry text when loading fails, while preserving “No projects registered yet.” for successful empty results.cmd/project/project_dev.go-22-52 (1)
22-52: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLowercase the host before the domain comparison.
url.URL.Hostname()preserves the case of the configured URL. A config value such ashttps://My-Shop.Shopware.localis a valid proxy URL for DNS, but the comparison againstbaseDomainfails, soproject devskips the proxy bootstrap. Compare lowercased values.🐛 Proposed fix
- host := parsed.Hostname() - return host == baseDomain || strings.HasSuffix(host, "."+baseDomain) + host := strings.ToLower(parsed.Hostname()) + baseDomain = strings.ToLower(baseDomain) + return host == baseDomain || strings.HasSuffix(host, "."+baseDomain)🤖 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 22 - 52, Update isProxyProjectForDomain to lowercase the value returned by parsed.Hostname() before comparing it with baseDomain, ensuring both exact and subdomain checks handle mixed-case proxy URLs correctly.internal/tui/dev/tab_overview.go-1272-1288 (1)
1272-1288: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAdd a Compose route for
rabbitmqor remove it fromknownServices. In proxy mode, the dashboard buildshttps://rabbitmq.<host>, but the override routeslavinmq, notrabbitmq; that URL has no router. Theadminer,mailer, andlavinmqlabels match.🤖 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/tab_overview.go` around lines 1272 - 1288, Update the proxy-mode URL construction in the discovered-services flow to use the Compose service name that is actually routed for RabbitMQ, such as the existing lavinmq identifier, while preserving the adminer and mailer routes; alternatively remove the RabbitMQ entry from knownServices so no unroutable rabbitmq.<host> URL is generated.cmd/project/project_proxy_setup.go-328-346 (1)
328-346: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winTell the user what to do after a failed deregistration.
The loop prints each failure and continues. Teardown then stops Traefik and the DNS server. A project that failed to deregister keeps its proxy URL and its
compose.override.yaml, so it becomes unreachable with no further hint. Count the failures. If any occurred, print the recovery step, for example "run "shopware-cli project proxy down" in ", and finish with a non-zero exit.🤖 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_proxy_setup.go` around lines 328 - 346, Track the number of deregistration failures in the loop over reg.Projects, incrementing it whenever newProxyEnvironmentForRoot or env.down fails. After stopping Traefik and the DNS server, print a recovery instruction for each failed entry using its project path, then return a non-zero error when any failures occurred instead of reporting successful teardown.internal/proxy/trust.go-16-26 (1)
16-26: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
%qcorrupts Windows paths in the printed command.
%qproduces a Go-quoted string and escapes every backslash. On WindowscaPathcontains backslashes, so the printedcertutilcommand shows a doubled-backslash path that fails when a user copies it. Print the path with%sinside plain double quotes.🐛 Proposed fix
case "windows": - return fmt.Sprintf("certutil -addstore -f ROOT %q", caPath) + return fmt.Sprintf("certutil -addstore -f ROOT \"%s\"", caPath)🤖 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/proxy/trust.go` around lines 16 - 26, Update the Windows branch of TrustInstructions to format caPath with %s inside plain double quotes instead of using %q, so copied certutil commands preserve Windows backslashes. Leave the macOS and Unix command formatting unchanged.cmd/project/project_proxy_setup.go-21-24 (1)
21-24: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd
SilenceErrorsso the sentinel error stays silent.
RunEreturnsErrProxyVerificationFailedat line 134. The declaration of that error incmd/project/project_proxy_verify.gostates that it exits non-zero without an extra message, andprojectProxyVerifyCmdsetsSilenceErrors: true.projectProxySetupCmddoes not, so Cobra prints "Error: proxy verification failed" after the printed check results.🐛 Proposed fix
var projectProxySetupCmd = &cobra.Command{ Use: "setup", SilenceUsage: true, + SilenceErrors: true, Short: "One-time machine setup for the shared proxy: DNS and HTTPS trust (needs sudo)",Note: with
SilenceErrorsevery error from this command becomes silent, so print the other failures explicitly, or return the sentinel through a wrapper that the root command recognizes.🤖 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_proxy_setup.go` around lines 21 - 24, Set SilenceErrors: true on projectProxySetupCmd so ErrProxyVerificationFailed does not produce Cobra’s duplicate error output. Audit the command’s RunE error paths and explicitly print any non-sentinel failures that must remain visible, preserving the existing verification-result output.cmd/project/project_proxy_setup.go-54-75 (1)
54-75: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStop
proxy setupearly on native Windows with the WSL2 guidance.On Windows,
ConfigureResolverreturnserrNotSupportedOnWindows, so setup exits throughResolverBlockedGuidancebeforeEnsureDNSServerRunningand shows no WSL2 pointer. Add a platform check before resolver work and print the hint used byproxy.Verify:Run shopware-cli inside WSL2 to use the proxy (see docs/proxy.md).🤖 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_proxy_setup.go` around lines 54 - 75, Add a native-Windows platform check before the resolver configuration block in the proxy setup flow, and print the WSL2 guidance used by proxy.Verify: “Run shopware-cli inside WSL2 to use the proxy (see docs/proxy.md).” Return immediately after displaying the hint so ConfigureResolver and EnsureDNSServerRunning are not invoked on Windows.internal/proxy/traefik.go-61-68 (1)
61-68: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRepresent
entryPointsas a YAML sequence. The file provider expects a list of strings. Use- websecureso Traefik loadsproxy-ping.🤖 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/proxy/traefik.go` around lines 61 - 68, Update the proxy-ping router YAML in the Traefik configuration template so entryPoints is represented as a sequence containing websecure, using the YAML list form required by the file provider. Preserve the existing router rule, TLS settings, and ping@internal service.internal/envfile/upsert.go-43-56 (1)
43-56: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStrip surrounding quotes from the returned value.
Symfony dotenv files allow quoted values, for example
APP_URL="http://127.0.0.1:8000".ReadEnvVarreturns the quotes as part of the value.cmd/project/project_proxy.gouses the result aspreviousAppURLand compares it againstproxyURL(lines 185-198), so a quoted value produces a wrong restore target and a wrongurlChangeddecision.🐛 Proposed fix
for _, l := range strings.Split(string(content), "\n") { if strings.HasPrefix(strings.TrimSpace(l), key+"=") { - return strings.TrimSpace(strings.TrimPrefix(strings.TrimSpace(l), key+"=")) + value := strings.TrimSpace(strings.TrimPrefix(strings.TrimSpace(l), key+"=")) + if len(value) >= 2 && (value[0] == '"' || value[0] == '\'') && value[len(value)-1] == value[0] { + value = value[1 : len(value)-1] + } + + 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/envfile/upsert.go` around lines 43 - 56, Update ReadEnvVar to remove one matching pair of surrounding single or double quotes from the extracted environment value before returning it, while preserving unquoted values and internal quote characters.cmd/project/project_proxy.go-605-620 (1)
605-620: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winEscape backslashes as well before interpolating into the SQL literal.
MariaDB treats
\as an escape character inside string literals by default. A URL that ends with a backslash turns the doubled quote into an escaped quote and changes the statement. Escape\in addition to'.🔒️ Proposed fix
- esc := func(s string) string { return strings.ReplaceAll(s, "'", "''") } + esc := func(s string) string { + s = strings.ReplaceAll(s, `\`, `\\`) + return strings.ReplaceAll(s, "'", "''") + }🤖 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_proxy.go` around lines 605 - 620, Update the local esc function in repointSalesChannelViaSQL to escape backslashes as well as single quotes before interpolating fromURL and toURL into the SQL string literals, preserving the existing query construction and command execution.internal/proxy/windows_access.go-36-41 (1)
36-41: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winPrefer a user-owned path over
C:\Users\Publicfor the CA copy.
C:\Users\Publicis writable by every local account. Another local user can replaceshopware-cli-rootCA.pembetween step 1 and step 2, and the administrator then trusts a foreign CA root. The file holds only the public certificate, so no key leaks, but the trust step is the sensitive part. Point the copy at a path inside the user profile, for example%USERPROFILE%\shopware-cli-rootCA.pem, and keep the WSL mount path in sync.🤖 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/proxy/windows_access.go` around lines 36 - 41, Update the windowsCACopyPath constant to use a user-owned location under %USERPROFILE% instead of C:\Users\Public, and update wslWindowsCACopyMount to reference the corresponding WSL-mounted user-profile path. Keep both constants synchronized so the copy and trust commands target the same per-user certificate file.internal/proxy/hostname.go-32-38 (1)
32-38: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate the derived label as a DNS label, not only as a Compose name.
system.ValidateDockerComposeNameaccepts names that are invalid DNS labels. A directory namedmy-shop-ormy_shop_maps tomy-shop-, which ends with a hyphen. A directory name longer than 63 characters also passes. Both produce a malformed hostname that Traefik routing and certificate matching reject later. The linked issue requires validation of generated domains.Add explicit label checks after the mapping.
🛡️ Proposed additional validation
name := strings.ReplaceAll(filepath.Base(projectRoot), "_", "-") if err := system.ValidateDockerComposeName(name); err != nil { return "", fmt.Errorf("cannot derive a hostname from directory name %q: %w", filepath.Base(projectRoot), err) } + if len(name) > 63 || strings.HasSuffix(name, "-") { + return "", fmt.Errorf("cannot derive a hostname from directory name %q: %q is not a valid DNS label", filepath.Base(projectRoot), name) + }🤖 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/proxy/hostname.go` around lines 32 - 38, Update the derived hostname label in the proxy hostname construction flow after the underscore-to-dash mapping and Compose-name validation. Add explicit DNS-label validation for an ASCII label: enforce the 63-character maximum, require alphanumeric start and end characters, and allow only alphanumeric characters or hyphens internally; return the existing hostname-derivation error for invalid labels before using the name.internal/proxy/registry_test.go-11-17 (1)
11-17: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRedirect the state directory on Windows too.
StateDirusesos.UserConfigDir. On Windows that function reads%AppData%and ignoresHOMEandXDG_CONFIG_HOME. A test run on Windows then reads and overwrites the real user registry at%AppData%\shopware-cli\proxy\registry.json.Set
AppDataas well.🛡️ Proposed fix
dir := t.TempDir() t.Setenv("HOME", dir) t.Setenv("XDG_CONFIG_HOME", dir) + t.Setenv("AppData", dir)🤖 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/proxy/registry_test.go` around lines 11 - 17, Update the test helper withTempStateDir to also set the AppData environment variable to the temporary directory, ensuring os.UserConfigDir resolves the isolated state directory on Windows while preserving the existing HOME and XDG_CONFIG_HOME setup.internal/mkcert/mkcert.go-50-69 (1)
50-69: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle an empty
LocalAppDataon Windows.If
LocalAppDatais empty,CAROOT()returns the relative pathmkcert.LoadOrCreateCA()then creates the CA in the current working directory instead of returning an error.🛡️ Proposed fix
case runtime.GOOS == "windows": dir = os.Getenv("LocalAppData") + if dir == "" { + return "" + }🤖 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/mkcert/mkcert.go` around lines 50 - 69, Update the Windows branch in CAROOT to validate that LocalAppData is non-empty before constructing the mkcert path; return the empty result used for missing base directories so LoadOrCreateCA does not create the CA in the current working directory.internal/proxy/hostname.go-21-30 (1)
21-30: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReject configured hosts outside
baseDomain.
EnsureCertificateadds explicit SANs, but the embedded DNS server only answers names underbaseDomain. Without a manual/etc/hostsentry, an override such asshop.example.comdoes not resolve to Traefik.🤖 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/proxy/hostname.go` around lines 21 - 30, Update the configured-host handling in the hostname resolution function around cfg.URL and parsed.Hostname() to accept only hostnames within the configured baseDomain, including the base domain itself, while preserving the existing exclusions for localhost and IP addresses. Reject or ignore hosts outside baseDomain so the returned hostname always resolves through the embedded DNS server.internal/proxy/projectconfig.go-139-146 (1)
139-146: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse two-space indentation when rewriting the project config.
yaml.Marshaldefaults to four-space indentation, so rewriting.shopware-project.ymlcan reindent nested blocks. Useyaml.EncoderwithSetIndent(2).🤖 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/proxy/projectconfig.go` around lines 139 - 146, Update writeConfigDoc to serialize the YAML document through a yaml.Encoder configured with SetIndent(2) instead of yaml.Marshal, then write the encoded output to path while preserving existing error propagation and file permissions.internal/proxy/cert_test.go-92-105 (1)
92-105: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse
requireinreadTestCertificateto avoid a nil dereference panic.
assert.NoErrorandassert.NotNilmark the test failed but continue. Ifos.ReadFilefails orpem.Decodereturns no block, line 101 dereferences a nilblockand the test binary panics. The panic hides the real assertion message.🛠️ Proposed fix
content, err := os.ReadFile(path) - assert.NoError(t, err) + require.NoError(t, err) block, _ := pem.Decode(content) - assert.NotNil(t, block) + require.NotNil(t, block) cert, err := x509.ParseCertificate(block.Bytes) - assert.NoError(t, err) + require.NoError(t, err)Add the
github.com/stretchr/testify/requireimport.🤖 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/proxy/cert_test.go` around lines 92 - 105, Update readTestCertificate to use testify/require for the os.ReadFile error and pem.Decode result checks, replacing the corresponding assert calls so the test exits before dereferencing a nil block; add the require import.internal/proxy/dns.go-33-63 (1)
33-63: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClose the UDP socket on every return path.
The watchdog goroutine closes
connonly whenctxis done. IfReadFromUDPfails for another reason,RunDNSServerreturns the error and leaves the socket open plus the goroutine blocked on<-ctx.Done(). A caller that restarts the server then fails to bind the port.🛠️ Proposed fix
conn, err := net.ListenUDP("udp", &net.UDPAddr{IP: net.IPv4(127, 0, 0, 1), Port: dnsPort}) if err != nil { return err } + defer func() { _ = conn.Close() }()🤖 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/proxy/dns.go` around lines 33 - 63, Update RunDNSServer to defer closing conn immediately after a successful net.ListenUDP call, ensuring the socket is released on both normal shutdown and read errors. Retain the existing context-watcher behavior, while ensuring its repeated close is harmless and the goroutine does not prevent cleanup.internal/proxy/cert.go-113-132 (1)
113-132: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle IP literals in proxy certificate coverage
ValidateDomainaccepts127.0.0.1, soCertHostscan pass an IP literal toEnsureCertificate.mkcertstores it inIPAddresses, butcertCoverschecks onlyDNSNames. Each call then regenerates the certificate and restarts Traefik. Reject IP literals inValidateDomain, or compare them withcert.IPAddresses.🤖 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/proxy/cert.go` around lines 113 - 132, Update certCovers to handle IP literal hosts by parsing each host and comparing valid IPs against cert.IPAddresses while continuing to compare domain names against cert.DNSNames; preserve the existing expiry and certificate-read checks so covered certificates are not regenerated unnecessarily.
🧹 Nitpick comments (18)
internal/tui/dev/model.go (1)
306-311: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMove the setup-state refresh off the update loop.
overviewSetupDonereadsregistry.json, loads proxy settings, and callsproxy.CheckResolverConfigured, which inspects the OS resolver. This runs synchronously insideUpdate, so the TUI freezes for the duration of that check. Return it as atea.Cmdand apply the result through a message, asloadSetupHealthalready does.🤖 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/model.go` around lines 306 - 311, The proxySetupDoneMsg branch in Update currently calls overviewSetupDone synchronously, blocking the TUI. Move that refresh into a tea.Cmd that returns a dedicated result message, then update m.overview.domainsSetupDone when handling that message; preserve the existing healthLoading and loadSetupHealth behavior.internal/tui/dev/tab_overview_health.go (1)
94-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated base-domain fallback logic. Both sites repeat the same
proxy.DefaultDomainplusLoadSettings().BaseDomain()fallback, whichcmd/project/project_create.goalso implements asproxyBaseDomain. Add one exported helper ininternal/proxyand call it from all three places.
internal/tui/dev/tab_overview_health.go#L94-L97: replace the local fallback block with the shared helper.internal/tui/dev/tab_overview.go#L342-L347: replace the identical block inoverviewSetupDonewith the same helper.🤖 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/tab_overview_health.go` around lines 94 - 97, Introduce one exported helper in internal/proxy that returns proxy.DefaultDomain unless proxy.LoadSettings succeeds, then returns settings.BaseDomain(). Replace the fallback blocks in internal/tui/dev/tab_overview_health.go:94-97 and internal/tui/dev/tab_overview.go:342-347 with calls to this helper, and update cmd/project/project_create.go’s proxyBaseDomain logic to use it as well.internal/tui/dev/tab_overview.go (1)
314-332: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueResolve the proxy hostname once in the constructor.
proxyHostname(projectRoot)runs three times here: at line 326, insideoverviewSetupDoneat line 327, and at line 328. Each call loadsregistry.jsonand resolves symlinks. Compute it once and pass it tooverviewSetupDone.♻️ Proposed refactor
func NewOverviewModel(envType, shopURL, username, password, projectRoot string, exec executor.Executor, shopCfg *shop.Config) OverviewModel { + proxyHost := proxyHostname(projectRoot) return OverviewModel{ ... - proxyHost: proxyHostname(projectRoot), - domainsSetupDone: overviewSetupDone(projectRoot), - instancesLoading: proxyHostname(projectRoot) != "", + proxyHost: proxyHost, + domainsSetupDone: proxyHost != "" && resolverConfigured(), + instancesLoading: proxyHost != "",🤖 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/tab_overview.go` around lines 314 - 332, Update NewOverviewModel to compute proxyHostname(projectRoot) once in a local variable, pass that value to overviewSetupDone, and reuse it for proxyHost and instancesLoading instead of invoking proxyHostname repeatedly.internal/proxy/dns_test.go (1)
20-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReport the server error when startup fails.
The goroutine discards the
RunDNSServererror. The reserved port can be taken by another process betweenconn.Close()and the bind. The test then fails with "DNS server did not start" and hides the bind error. Capture the error and include it in the failure message.♻️ Proposed change
ctx, cancel := context.WithCancel(t.Context()) t.Cleanup(cancel) + errCh := make(chan error, 1) go func() { - _ = RunDNSServer(ctx, port, "shopware.local") + errCh <- RunDNSServer(ctx, port, "shopware.local") }() addr := fmt.Sprintf("127.0.0.1:%d", port) // Wait until the server answers. for range 50 { + select { + case err := <-errCh: + require.NoError(t, err, "DNS server stopped") + default: + } + if _, err := queryDNS(ctx, addr, "probe.shopware.local", dnsmessage.TypeA, 200*time.Millisecond); err == nil { return addr } time.Sleep(20 * time.Millisecond) }🤖 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/proxy/dns_test.go` around lines 20 - 45, Update the RunDNSServer startup goroutine to capture its returned error through a test-safe channel or shared state, then include that error in the t.Fatal message when the startup probe loop fails. Preserve the existing successful startup flow and cleanup behavior.internal/proxy/trust.go (1)
70-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the discarded
mkcert -installfailure.The
mkcerterror is dropped, and the fallback runs silently. If both paths fail, the returned message only describes thetruststorefailure, so the root cause stays hidden. Log themkcerterror through the context logger.♻️ Proposed change
if err := cmd.Run(); err == nil { return "The mkcert root CA is installed, certificates issued by it are trusted.", nil - } + } else { + logging.FromContext(ctx).Debugf("mkcert -install failed, falling back to the truststore library: %s", err) + } // mkcert failed (often: sudo blocked, or a broken mkcert install); // fall through to the library path, which explains itself on failure.As per coding guidelines: "Use structured logging through
go.uber.org/zap, obtain context-based loggers withlogging.FromContext(ctx), and report errors gracefully to users."🤖 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/proxy/trust.go` around lines 70 - 85, Capture the error returned by cmd.Run in the mkcert installation path and log it through the context logger obtained with logging.FromContext(ctx), using structured zap error logging before falling back to truststore.InstallFile. Preserve the existing fallback and returned error behavior.Source: Coding guidelines
cmd/project/project_proxy_setup.go (2)
307-313: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the teardown command into its own file.
projectProxyTeardownCmdis a separate subcommand but lives inproject_proxy_setup.go. Move it,confirmTeardown, and its flag registration intocmd/project/project_proxy_teardown.go.As per coding guidelines: "Organize Cobra commands with the main command in
cmd/[group]/[group].goand subcommands incmd/[group]/[group]_[subcommand].go".🤖 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_proxy_setup.go` around lines 307 - 313, Move the projectProxyTeardownCmd declaration, confirmTeardown, and teardown-specific flag registration from project_proxy_setup.go into cmd/project/project_proxy_teardown.go, preserving their behavior and command wiring. Keep setup-related symbols in the original file and follow the project_proxy_<subcommand>.go organization convention.Source: Coding guidelines
176-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared resolver and DNS step.
Lines 176-193 repeat lines 54-75 almost exactly: the same
CheckResolverConfiguredbranch, the sameErrNoSystemdResolvedhandling, the same success messages, and the sameEnsureDNSServerRunningcall. Two copies drift apart when the guidance text or the error handling changes. Extract one helper that both call, and keep only the domain-change reporting inRunE.🤖 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_proxy_setup.go` around lines 176 - 193, The resolver and DNS startup logic is duplicated between RunE and the earlier setup flow. Extract the shared CheckResolverConfigured, ConfigureResolver, ErrNoSystemdResolved handling, success output, and EnsureDNSServerRunning sequence into one helper, have both callers invoke it, and leave only domain-change reporting in RunE.internal/proxy/traefik.go (1)
315-331: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueTrim each output line before comparison.
docker psoutput can carry\rline endings on Windows hosts. The comparisonname == ContainerNamethen fails, and the proxy container appears in the instance list. Trim each line.♻️ Proposed change
var instances []Instance for _, name := range strings.Split(strings.TrimSpace(out), "\n") { + name = strings.TrimSpace(name) if name == "" || name == ContainerName { continue }🤖 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/proxy/traefik.go` around lines 315 - 331, Update RunningInstances to trim whitespace from each name yielded by the docker output before checking for empty values or comparing it with ContainerName, so carriage returns and other line-ending whitespace cannot include the proxy container as an instance.internal/envfile/upsert.go (1)
38-38: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCreate env files with 0600 instead of 0644.
UpsertEnvVartargets.env.local, which commonly holdsAPP_SECRET, database credentials and API keys. When the file does not exist yet, this call creates it world-readable. Use0o600for a newly created secrets file.🔒️ Proposed change
- return os.WriteFile(path, []byte(strings.Join(lines, "\n")+"\n"), 0o644) + return os.WriteFile(path, []byte(strings.Join(lines, "\n")+"\n"), 0o600)🤖 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/envfile/upsert.go` at line 38, Update the file mode passed to os.WriteFile in UpsertEnvVar from 0o644 to 0o600 so newly created .env.local files are accessible only by their owner.internal/extension/storefront_watch.go (1)
98-106: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
maps.Copyfor the merge.The manual loop can be replaced by the standard-library helper.
♻️ Proposed refactor
if opts.ProxyHostname != "" { proxyEnv, err := storefrontProxyEnv(projectRoot, cmdExecutor, opts.ProxyHostname) if err != nil { return nil, err } - for k, v := range proxyEnv { - env[k] = v - } + maps.Copy(env, proxyEnv) }Add
"maps"to the import block.As per coding guidelines: "Prefer Go 1.24 standard-library packages, such as
slices, when appropriate."🤖 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/extension/storefront_watch.go` around lines 98 - 106, In the environment merge within the storefront proxy handling, replace the manual k/v iteration over proxyEnv with the Go standard-library maps.Copy helper, and add the maps import. Preserve the existing error return and merge behavior in the surrounding function.Source: Coding guidelines
internal/proxy/canonical.go (1)
8-15: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake the path absolute before resolving symlinks.
filepath.EvalSymlinkskeeps a relative input relative.ProjectEntry.ProjectRootis documented as the canonical absolute path and is used as the registry key (seeinternal/proxy/registry.go:11-23). If any caller passes a relative root, the registry gets a non-absolute key and lookups from another working directory miss. Addfilepath.Absfirst.♻️ Proposed hardening
func CanonicalProjectRoot(projectRoot string) string { - resolved, err := filepath.EvalSymlinks(projectRoot) + abs, err := filepath.Abs(projectRoot) + if err != nil { + abs = projectRoot + } + + resolved, err := filepath.EvalSymlinks(abs) if err != nil { - return projectRoot + return abs } return resolved }🤖 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/proxy/canonical.go` around lines 8 - 15, Update CanonicalProjectRoot to call filepath.Abs on projectRoot before filepath.EvalSymlinks, ensuring the returned canonical path is absolute. Preserve the existing fallback behavior on resolution errors, returning the absolute path when available.internal/proxy/projectconfig.go (1)
67-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument that these two functions need an existing config file.
SetProjectConfigURLsandRestoreProjectConfigURLsreturn theos.ReadFileerror when the file is absent.ReadProjectConfigURLsinstead maps that case toHasFile=false. A caller that skips theHasFilecheck receives a raw "no such file or directory" error. State the precondition in both doc comments, or return early when the file is missing.🤖 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/proxy/projectconfig.go` around lines 67 - 104, Update the doc comments for SetProjectConfigURLs and RestoreProjectConfigURLs to state that configPath must reference an existing configuration file; preserve their current missing-file error behavior rather than adding new handling.internal/proxy/registry.go (1)
57-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace manual slice loops and
sortwith theslicespackage. Both sites use pre-generics slice handling where asliceshelper is clearer and shorter. The coding guidelines ask for Go 1.24 standard-library packages such asslices.
internal/proxy/registry.go#L57-L78: useslices.IndexFuncinUpsertandRemove, andslices.Deletefor the removal.internal/proxy/stats.go#L81-L84: replacesort.SliceStablewithslices.SortStableFuncand drop thesortimport.
As per coding guidelines: "Prefer Go 1.24 standard-library packages, such asslices, when appropriate."🤖 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/proxy/registry.go` around lines 57 - 78, Replace the manual searches in internal/proxy/registry.go:57-78 with slices.IndexFunc in both Registry.Upsert and Registry.Remove, preserving the existing update and boolean-return behavior; use slices.Delete when removing the matched project. In internal/proxy/stats.go:81-84, replace sort.SliceStable with slices.SortStableFunc and remove the now-unused sort import.Source: Coding guidelines
internal/proxy/stats.go (2)
41-44: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueReport the
docker psfailure gracefully.
InstanceStatsreturns the rawdocker pserror. The development dashboard calls this function on refresh, so a stopped Docker daemon surfaces as a raw command error in the overview. Wrap the error with context, and log it throughlogging.FromContext(ctx)so the dashboard can present a readable state.🤖 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/proxy/stats.go` around lines 41 - 44, Update InstanceStats to wrap the error returned by runDocker for the "ps" command with descriptive context, then log the contextual error through logging.FromContext(ctx) before returning. Preserve the existing nil, zero, error return behavior.Source: Coding guidelines
105-111: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winLimit
docker statsto the relevant containers.
docker stats --no-streamsamples every running container on the host, including containers unrelated to registered projects. Each sample takes hundreds of milliseconds per container, and the development dashboard callsInstanceStatson refresh. Pass the known container IDs so the cost scales with the registered projects.♻️ Proposed change
-func memoryByProject(ctx context.Context, projectOfContainer map[string]string) map[string]int64 { +func memoryByProject(ctx context.Context, projectOfContainer map[string]string, containerIDs []string) map[string]int64 { byProject := map[string]int64{} + if len(containerIDs) == 0 { + return byProject + } - out, err := runDocker(ctx, "stats", "--no-stream", "--format", "{{.Name}}\t{{.MemUsage}}") + args := append([]string{"stats", "--no-stream", "--format", "{{.Name}}\t{{.MemUsage}}"}, containerIDs...) + out, err := runDocker(ctx, args...) if err != nil { return byProject }Update the call site at Line 60 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/proxy/stats.go` around lines 105 - 111, Update memoryByProject and its call site in InstanceStats to pass the known container IDs to runDocker’s docker stats invocation. Build the container arguments from projectOfContainer keys and preserve the existing no-stream and format options, so stats samples only registered project containers.internal/proxy/verify_darwin.go (1)
21-23: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueMatch the resolved address per line, not as a substring.
strings.Containsalso matchesip_address: 127.0.0.10orip_address: 127.0.0.1x. Compare the trimmed value of eachip_address:line instead.♻️ Proposed fix
- if !strings.Contains(string(out), "ip_address: 127.0.0.1") { - return fmt.Errorf("%s does not resolve to 127.0.0.1 via the system resolver", hostname) - } - - return nil + for _, line := range strings.Split(string(out), "\n") { + value, ok := strings.CutPrefix(strings.TrimSpace(line), "ip_address:") + if ok && strings.TrimSpace(value) == "127.0.0.1" { + return nil + } + } + + return fmt.Errorf("%s does not resolve to 127.0.0.1 via the system resolver", hostname)🤖 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/proxy/verify_darwin.go` around lines 21 - 23, Update the resolver validation around the ip_address check to inspect each output line, identify lines beginning with "ip_address:", and compare the trimmed value after the delimiter exactly to 127.0.0.1. Replace the substring-based strings.Contains check while preserving the existing error return when no exact match is found.internal/proxy/windows_access.go (1)
12-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse one source of truth for routed subdomains.
internal/proxy/windows_access.goduplicates the subdomains defined ininternal/docker/compose_override.go. When a new subdomain route is added, update both lists or derive the Windows hosts list from the shared route definitions.🤖 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/proxy/windows_access.go` around lines 12 - 19, Update ProxyHostnames to use the shared routed-subdomain definitions from compose_override.go instead of maintaining its own hardcoded subdomains list. Preserve the existing conditional inclusion of AMQP and Elasticsearch routes while ensuring future route additions automatically apply to the Windows hosts list.internal/proxy/projectconfig_test.go (1)
97-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
countOccurrenceswithstrings.Count.The standard library provides this exact behavior. The helper adds a hand-written loop with no added value.
♻️ Proposed refactor
-func countOccurrences(s, sub string) int { - count := 0 - for i := 0; i+len(sub) <= len(s); i++ { - if s[i:i+len(sub)] == sub { - count++ - } - } - return count -}Then use
strings.Countat the call sites (lines 53, 63) and add thestringsimport.As per coding guidelines: "Prefer Go 1.24 standard-library packages, such as
slices, when appropriate."🤖 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/proxy/projectconfig_test.go` around lines 97 - 105, Remove the custom countOccurrences helper and replace its call sites in the test with strings.Count, passing the same string and substring arguments. Add the strings import and preserve the existing assertions and counting behavior.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a6695d98-b1fe-4668-bfc2-071186a4a0c1
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (70)
cmd/project/project_create.gocmd/project/project_create_form.gocmd/project/project_create_install.gocmd/project/project_dev.gocmd/project/project_dev_test.gocmd/project/project_proxy.gocmd/project/project_proxy_dns_serve.gocmd/project/project_proxy_list.gocmd/project/project_proxy_setup.gocmd/project/project_proxy_test.gocmd/project/project_proxy_verify.gocmd/project/project_storefront_watch.gocmd/root.godocs/proxy.mdgo.modinternal/docker/compose.gointernal/docker/compose_override.gointernal/docker/compose_override_test.gointernal/docker/compose_test.gointernal/envfile/upsert.gointernal/envfile/upsert_test.gointernal/executor/docker.gointernal/extension/storefront_hmr_patch.cjsinternal/extension/storefront_watch.gointernal/mkcert/LICENSEinternal/mkcert/mkcert.gointernal/mkcert/mkcert_test.gointernal/proxy/canonical.gointernal/proxy/cert.gointernal/proxy/cert_test.gointernal/proxy/dns.gointernal/proxy/dns_daemon.gointernal/proxy/dns_daemon_windows.gointernal/proxy/dns_test.gointernal/proxy/docker.gointernal/proxy/hostname.gointernal/proxy/hostname_test.gointernal/proxy/projectconfig.gointernal/proxy/projectconfig_test.gointernal/proxy/registry.gointernal/proxy/registry_test.gointernal/proxy/resolver.gointernal/proxy/resolver_darwin.gointernal/proxy/resolver_darwin_test.gointernal/proxy/resolver_linux.gointernal/proxy/resolver_linux_test.gointernal/proxy/resolver_windows.gointernal/proxy/settings.gointernal/proxy/settings_test.gointernal/proxy/statedir.gointernal/proxy/stats.gointernal/proxy/stats_test.gointernal/proxy/traefik.gointernal/proxy/traefik_test.gointernal/proxy/trust.gointernal/proxy/trust_test.gointernal/proxy/verify.gointernal/proxy/verify_darwin.gointernal/proxy/verify_linux.gointernal/proxy/verify_test.gointernal/proxy/verify_windows.gointernal/proxy/windows_access.gointernal/proxy/windows_access_test.gointernal/proxy/wsl_resolver_test.gointernal/system/dockername.gointernal/tui/dev/model.gointernal/tui/dev/model_view.gointernal/tui/dev/tab_overview.gointernal/tui/dev/tab_overview_format_test.gointernal/tui/dev/tab_overview_health.go
| // localDomainHostname returns the stable proxy hostname for a project name, | ||
| // e.g. "my-shop.shopware.local". Underscores (valid in a project name but not | ||
| // in a hostname) become dashes. | ||
| func localDomainHostname(name, baseDomain string) string { | ||
| label := strings.ReplaceAll(filepath.Base(name), "_", "-") | ||
| return label + "." + baseDomain | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Malformed hostname when the project folder is .. localDomainHostname maps filepath.Base(".") to the label ".", so the derived hostname is "..shopware.local". This value is written to .shopware-project.yml and printed as a shop URL.
cmd/project/project_create.go#L82-L88: resolve the working directory name whennameis empty or".", and lowercase the label.cmd/project/project_dev_test.go#L11-L21: add aTestLocalDomainHostnamecase for the"."input that asserts the resolved directory name is used.
📍 Affects 2 files
cmd/project/project_create.go#L82-L88(this comment)cmd/project/project_dev_test.go#L11-L21
🤖 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_create.go` around lines 82 - 88, Update
localDomainHostname in cmd/project/project_create.go (lines 82-88) to resolve
the current working directory’s base name when name is empty or "." and
lowercase the resulting label before appending baseDomain. Add a
TestLocalDomainHostname case in cmd/project/project_dev_test.go (lines 11-21)
verifying "." uses the resolved directory name.
| if err != nil { | ||
| // Never block dev: drop back to fixed-port mode and tell the user how | ||
| // to diagnose the proxy. | ||
| _ = dockerpkg.RemoveComposeOverride(projectRoot) | ||
| fmt.Println(tui.RedText.Render(" Shared proxy unavailable: " + err.Error())) | ||
| fmt.Println(tui.DimText.Render(" Serving on a local port instead — run ") + tui.BoldText.Render("shopware-cli project proxy verify") + tui.DimText.Render(" to diagnose.")) | ||
| return true | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Report the failure of RemoveComposeOverride instead of discarding it.
RemoveComposeOverride returns an error when compose.override.yaml exists but has no shopware-cli marker (see internal/docker/compose_override.go lines 358-361). The error is discarded, so the override stays in place. The project then still routes through the (unavailable) proxy while the message states that the shop is served on a local port, and docker compose up keeps the proxy labels and no published web port. The shop is then reachable at neither address.
Print the removal failure so the user can resolve the conflicting file.
🐛 Proposed fix
_ = dockerpkg.RemoveComposeOverride(projectRoot)
fmt.Println(tui.RedText.Render(" Shared proxy unavailable: " + err.Error()))
+ if rmErr := dockerpkg.RemoveComposeOverride(projectRoot); rmErr != nil {
+ fmt.Println(tui.RedText.Render(" Could not remove the proxy override: " + rmErr.Error()))
+ fmt.Println(tui.DimText.Render(" Remove or rename compose.override.yaml, then start again."))
+ return true
+ }
fmt.Println(tui.DimText.Render(" Serving on a local port instead — run ") + tui.BoldText.Render("shopware-cli project proxy verify") + tui.DimText.Render(" to diagnose."))
return trueApply the removal once; delete the discarded call above.
As per coding guidelines: "report errors gracefully to users".
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 81-81: A log/format call (log.Print/Printf/Println, the Fatal/Panic variants, fmt.Sprintf, or a structured logger's Info/Warn/Error/Debug method) is given a message built by concatenating a string literal with a non-literal value such as request data. Unsanitized, attacker-controlled input written to logs enables log forging / CRLF injection: an attacker can inject newlines to spoof log entries or break log parsers. Do not concatenate raw input into the log message; pass it as a separate structured field/argument (e.g. 'log.Printf("user: %s", user)' or 'logger.Info("login", "user", user)') and strip or escape newline characters first.
Context: fmt.Println(tui.RedText.Render(" Shared proxy unavailable: " + err.Error()))
Note: [CWE-117] Improper Output Neutralization for Logs.
(log-injection-request-data-concat-go)
[warning] 82-82: A log/format call (log.Print/Printf/Println, the Fatal/Panic variants, fmt.Sprintf, or a structured logger's Info/Warn/Error/Debug method) is given a message built by concatenating a string literal with a non-literal value such as request data. Unsanitized, attacker-controlled input written to logs enables log forging / CRLF injection: an attacker can inject newlines to spoof log entries or break log parsers. Do not concatenate raw input into the log message; pass it as a separate structured field/argument (e.g. 'log.Printf("user: %s", user)' or 'logger.Info("login", "user", user)') and strip or escape newline characters first.
Context: fmt.Println(tui.DimText.Render(" Serving on a local port instead — run ") + tui.BoldText.Render("shopware-cli project proxy verify") + tui.DimText.Render(" to diagnose."))
Note: [CWE-117] Improper Output Neutralization for Logs.
(log-injection-request-data-concat-go)
🤖 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 78 - 85, In the proxy-unavailable
error branch, update the existing RemoveComposeOverride call to capture and
report its error instead of discarding it. Print a clear user-facing message
when removal fails, while preserving the fallback to fixed-port mode and the
existing diagnostic guidance.
Source: Coding guidelines
| func runningServices(entry proxy.ProjectEntry, instances []proxy.Instance) []string { | ||
| prefix := filepath.Base(entry.ProjectRoot) + "-" | ||
|
|
||
| var services []string | ||
| for _, inst := range instances { | ||
| name, found := strings.CutPrefix(inst.Container, prefix) | ||
| if !found { | ||
| continue | ||
| } | ||
|
|
||
| if idx := strings.LastIndex(name, "-"); idx > 0 { | ||
| services = append(services, name[:idx]) | ||
| } | ||
| } | ||
|
|
||
| slices.Sort(services) | ||
|
|
||
| return services | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Unnormalized Compose project name used as the container prefix. Both helpers derive the container prefix from filepath.Base(entry.ProjectRoot). Docker Compose normalizes the project name (lowercase, invalid characters removed), so any directory name that is not already valid produces a prefix that never matches. The result is a registered project shown as stopped with no service links. This PR adds internal/system/dockername.go; route both sites through that helper.
cmd/project/project_proxy_list.go#L142-L160: buildprefixinrunningServicesfrom the normalized Compose project name.cmd/project/project_proxy_list.go#L165-L174: buildprefixinprojectIsRunningfrom the same normalized name, ideally through a single shared helper.
📍 Affects 1 file
cmd/project/project_proxy_list.go#L142-L160(this comment)cmd/project/project_proxy_list.go#L165-L174
🤖 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_proxy_list.go` around lines 142 - 160, Update
runningServices and projectIsRunning to derive their container prefixes through
the shared normalization helper in internal/system/dockername.go instead of
filepath.Base(entry.ProjectRoot). Ensure both helpers use the same normalized
Compose project name and retain the existing matching and service-detection
behavior.
| if err := dockerpkg.WriteComposeFile(e.projectRoot, dockerpkg.ComposeOptionsFromConfig(e.cfg)); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| proxyURL := "https://" + e.hostname | ||
|
|
||
| start := time.Now() | ||
| err = runStep(ctx, "Starting development environment...", func(ctx context.Context) error { | ||
| return e.executor.StartEnvironment(ctx) | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("starting environment: %w", err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Remove the proxy override when the environment fails to start.
prepareProxyInfra writes compose.override.yaml and starts the shared infrastructure. If StartEnvironment fails, up returns early. The project keeps a proxy override but is never registered, so proxy down only removes the override and reports "nothing to deregister". The PR objectives list this unclear partial-failure state as an open QA finding. Remove the override on the start failure path, or register the entry before starting so down can revert everything.
🐛 Proposed cleanup
err = runStep(ctx, "Starting development environment...", func(ctx context.Context) error {
return e.executor.StartEnvironment(ctx)
})
if err != nil {
+ if rmErr := dockerpkg.RemoveComposeOverride(e.projectRoot); rmErr != nil {
+ fmt.Println(tui.RedText.Render(" Could not remove the proxy compose override: " + rmErr.Error()))
+ }
+
return fmt.Errorf("starting environment: %w", err)
}📝 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.
| if err := dockerpkg.WriteComposeFile(e.projectRoot, dockerpkg.ComposeOptionsFromConfig(e.cfg)); err != nil { | |
| return err | |
| } | |
| proxyURL := "https://" + e.hostname | |
| start := time.Now() | |
| err = runStep(ctx, "Starting development environment...", func(ctx context.Context) error { | |
| return e.executor.StartEnvironment(ctx) | |
| }) | |
| if err != nil { | |
| return fmt.Errorf("starting environment: %w", err) | |
| } | |
| if err := dockerpkg.WriteComposeFile(e.projectRoot, dockerpkg.ComposeOptionsFromConfig(e.cfg)); err != nil { | |
| return err | |
| } | |
| proxyURL := "https://" + e.hostname | |
| start := time.Now() | |
| err = runStep(ctx, "Starting development environment...", func(ctx context.Context) error { | |
| return e.executor.StartEnvironment(ctx) | |
| }) | |
| if err != nil { | |
| if rmErr := dockerpkg.RemoveComposeOverride(e.projectRoot); rmErr != nil { | |
| fmt.Println(tui.RedText.Render(" Could not remove the proxy compose override: " + rmErr.Error())) | |
| } | |
| return fmt.Errorf("starting environment: %w", 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 `@cmd/project/project_proxy.go` around lines 164 - 176, Update the
StartEnvironment failure path in prepareProxyInfra to remove the
compose.override.yaml created by WriteComposeFile before returning the error,
ensuring partial startup cleanup leaves no unregistered proxy override.
| sudo sed -i -E '/^hosts:/d' /etc/nsswitch.conf | ||
| echo 'hosts: files dns resolve' | sudo tee -a /etc/nsswitch.conf |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Preserve the existing hosts: resolver configuration.
This command deletes every hosts: line and replaces it with hosts: files dns resolve. It can remove existing mdns, myhostname, wins, or other resolver modules and break name resolution. Modify the existing entry by adding resolve after dns instead of deleting the complete line.
Proposed documentation change
- sudo sed -i -E '/^hosts:/d' /etc/nsswitch.conf
- echo 'hosts: files dns resolve' | sudo tee -a /etc/nsswitch.conf
+ # Edit the existing `hosts:` entry and add `resolve` immediately after `dns`.
+ # Preserve all other resolver modules and ordering.🤖 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 `@docs/proxy.md` around lines 296 - 297, Update the documented
/etc/nsswitch.conf command to preserve the existing hosts: resolver modules,
inserting resolve immediately after dns on the existing entry instead of
deleting and replacing the entire hosts: line.
| func (r Registry) Save() error { | ||
| dir, err := StateDir() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| data, err := json.MarshalIndent(r, "", " ") | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return os.WriteFile(filepath.Join(dir, registryFileName), data, 0o600) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Write the registry atomically and guard concurrent read-modify-write.
Save overwrites registry.json in place. Two concurrent commands (for example proxy up in two shells, or proxy up while the dev TUI reads stats) each call LoadRegistry, mutate their copy, and then call Save. The last writer erases the other registration. An interrupted write also leaves a truncated file, and LoadRegistry then fails for every later command.
Write to a temporary file in the same directory and rename it. Consider a lock file around load-modify-save in the callers.
🛡️ Proposed atomic write
data, err := json.MarshalIndent(r, "", " ")
if err != nil {
return err
}
- return os.WriteFile(filepath.Join(dir, registryFileName), data, 0o600)
+ target := filepath.Join(dir, registryFileName)
+ tmp, err := os.CreateTemp(dir, registryFileName+".tmp*")
+ if err != nil {
+ return err
+ }
+ defer func() {
+ _ = tmp.Close()
+ _ = os.Remove(tmp.Name())
+ }()
+
+ if err := tmp.Chmod(0o600); err != nil {
+ return err
+ }
+ if _, err := tmp.Write(data); err != nil {
+ return err
+ }
+ if err := tmp.Close(); err != nil {
+ return err
+ }
+
+ return os.Rename(tmp.Name(), target)📝 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.
| func (r Registry) Save() error { | |
| dir, err := StateDir() | |
| if err != nil { | |
| return err | |
| } | |
| data, err := json.MarshalIndent(r, "", " ") | |
| if err != nil { | |
| return err | |
| } | |
| return os.WriteFile(filepath.Join(dir, registryFileName), data, 0o600) | |
| } | |
| func (r Registry) Save() error { | |
| dir, err := StateDir() | |
| if err != nil { | |
| return err | |
| } | |
| data, err := json.MarshalIndent(r, "", " ") | |
| if err != nil { | |
| return err | |
| } | |
| target := filepath.Join(dir, registryFileName) | |
| tmp, err := os.CreateTemp(dir, registryFileName+".tmp*") | |
| if err != nil { | |
| return err | |
| } | |
| defer func() { | |
| _ = tmp.Close() | |
| _ = os.Remove(tmp.Name()) | |
| }() | |
| if err := tmp.Chmod(0o600); err != nil { | |
| return err | |
| } | |
| if _, err := tmp.Write(data); err != nil { | |
| return err | |
| } | |
| if err := tmp.Close(); err != nil { | |
| return err | |
| } | |
| return os.Rename(tmp.Name(), target) | |
| } |
🤖 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/proxy/registry.go` around lines 115 - 127, Update Registry.Save to
persist registryFileName atomically: create a temporary file in the same
directory, write the marshaled data with the intended 0600 permissions, flush
and close it, then rename it over the target and clean up on failure. Also add
locking around the LoadRegistry–modify–Save sequences in their callers so
concurrent commands cannot overwrite each other’s registrations.
| func resolverFilePath(baseDomain string) string { | ||
| return "/etc/resolver/" + baseDomain | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Unvalidated baseDomain reaches privileged file operations on both platforms. The proxy base domain flows from user input into root-privileged writes without any DNS-name validation. One shared validation step fixes both sites.
internal/proxy/resolver_darwin.go#L22-L24: validatebaseDomainbeforeresolverFilePathconcatenates it into/etc/resolver/, sosudo teeandsudo rm -fcannot target a path outside that directory.internal/proxy/resolver_linux.go#L52-L72: validatebaseDomainbefore line 57 formats it into the resolved drop-in content, so a newline cannot inject extra[Resolve]directives.
Add one exported validation helper in the proxy package and call it at the proxy setup boundary as well.
📍 Affects 2 files
internal/proxy/resolver_darwin.go#L22-L24(this comment)internal/proxy/resolver_linux.go#L52-L72
🤖 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/proxy/resolver_darwin.go` around lines 22 - 24, Add one exported
validation helper in the proxy package and invoke it at the proxy setup boundary
before any resolver operations. Apply the validated value in
internal/proxy/resolver_darwin.go lines 22-24 before resolverFilePath builds
privileged paths, and in internal/proxy/resolver_linux.go lines 52-72 before
formatting resolved drop-in content; reject invalid DNS names, including path
traversal or newline characters, so both platforms use only safe base domains.
| func composeProjectName(projectRoot string) string { | ||
| if content, readErr := os.ReadFile(filepath.Join(projectRoot, ".env")); readErr == nil { | ||
| if name := shop.ExtractComposeProjectName(content); name != "" { | ||
| return name | ||
| } | ||
| } | ||
|
|
||
| return strings.ToLower(filepath.Base(projectRoot)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
docker compose default project name sanitization rules directory name
💡 Result:
When Docker Compose uses the base name of the directory as the default project name, it applies specific sanitization rules to ensure the resulting project name is compatible with platform resource naming requirements [1][2][3]. Project Name Requirements According to the Compose Specification, project names must [2][3]: - Contain only lowercase letters, decimal digits, dashes, and underscores. - Begin with a lowercase letter or decimal digit. Sanitization Mechanism When Compose determines the project name from the directory path, it effectively normalizes the string to adhere to these constraints [1]. In implementations of Docker Compose (such as the legacy Python-based Compose), this is typically achieved by converting the string to lowercase and removing any characters that do not match the permitted set (i.e., characters other than a-z, 0-9, -, or _) [4]. Important Considerations - Behavior Changes: Users should be aware that historical changes in how Compose handles these names (such as the truncation of characters like dots) have occurred, which can cause naming conflicts or force unexpected redeployments [5][6]. - Overriding the Default: Because automatic sanitization can lead to unexpected project names (e.g., if a directory name contains unsupported special characters), it is recommended to explicitly set the project name if the directory name might violate these rules [1]. This can be done via the -p/--project-name flag, the COMPOSE_PROJECT_NAME environment variable, or the top-level name attribute in the Compose file [1][7][2]. - Precedence: The precedence for determining the project name, from highest to lowest, is: the -p command line flag, the COMPOSE_PROJECT_NAME environment variable, the top-level name attribute in the Compose file, and finally the base name of the project directory [1].
Citations:
- 1: https://docs.docker.com/compose/how-tos/project-name/
- 2: https://compose-spec.github.io/compose-spec/spec.html
- 3: https://github.com/compose-spec/compose-spec/blob/main/02-model.md
- 4: https://github.com/docker/compose/blob/master/compose/cli/command.py
- 5: Project name derived from parent directory is truncated docker/compose#8822
- 6: [question] detecting resources using "legacy" naming convetion docker/compose#5933
- 7: https://docs.docker.com/reference/compose-file/version-and-name/
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(stats\.go|.*compose.*|.*proxy.*test.*)$' | head -80
printf '%s\n' '--- stats.go outline ---'
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline internal/proxy/stats.go
else
true
fi
printf '%s\n' '--- stats.go focused sections ---'
cat -n internal/proxy/stats.go | sed -n '1,220p'
printf '%s\n' '--- project-name symbols and callers ---'
rg -n -S 'composeProjectName|runningProjects|InstanceStats|ExtractComposeProjectName' . \
-g '!vendor' -g '!node_modules' -g '!dist' -g '!build' | head -240Repository: shopware/shopware-cli
Length of output: 11616
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(stats\.go|.*compose.*|.*proxy.*test.*)$' | head -80
printf '%s\n' '--- stats.go outline ---'
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline internal/proxy/stats.go
fi
printf '%s\n' '--- stats.go focused sections ---'
cat -n internal/proxy/stats.go | sed -n '1,220p'
printf '%s\n' '--- project-name symbols and callers ---'
rg -n -S 'composeProjectName|runningProjects|InstanceStats|ExtractComposeProjectName' . \
-g '!vendor' -g '!node_modules' -g '!dist' -g '!build' | head -240Repository: shopware/shopware-cli
Length of output: 11616
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- stats tests ---'
cat -n internal/proxy/stats_test.go | sed -n '1,260p'
printf '%s\n' '--- compose project-name helper ---'
cat -n internal/shop/compose_project_name.go | sed -n '1,150p'
printf '%s\n' '--- compose-name references and sanitization terms ---'
rg -n -i 'project name|project_name|sanitize|sanitiz|COMPOSE_PROJECT_NAME' internal cmd \
-g '*.go' -g '*.md' | head -240
printf '%s\n' '--- current Compose implementation candidates ---'
for url in \
'https://raw.githubusercontent.com/compose-spec/compose-go/main/cli/options.go' \
'https://raw.githubusercontent.com/docker/compose/main/compose.go' \
'https://raw.githubusercontent.com/docker/compose/main/cmd/compose/compose.go'
do
printf '\nURL: %s\n' "$url"
curl -LfsS --max-time 15 "$url" 2>/dev/null | rg -n -i -C 5 'sanitize|project.?name|working.?dir' | head -100 || true
doneRepository: shopware/shopware-cli
Length of output: 18234
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- stats tests ---'
cat -n internal/proxy/stats_test.go | sed -n '1,260p'
printf '%s\n' '--- compose project-name helper ---'
cat -n internal/shop/compose_project_name.go | sed -n '1,150p'
printf '%s\n' '--- compose-name references and sanitization terms ---'
rg -n -i 'project name|project_name|sanitize|sanitiz|COMPOSE_PROJECT_NAME' internal cmd \
-g '*.go' -g '*.md' | head -240
printf '%s\n' '--- current Compose implementation candidates ---'
for url in \
'https://raw.githubusercontent.com/compose-spec/compose-go/main/cli/options.go' \
'https://raw.githubusercontent.com/docker/compose/main/compose.go' \
'https://raw.githubusercontent.com/docker/compose/main/cmd/compose/compose.go'
do
printf '\nURL: %s\n' "$url"
curl -LfsS --max-time 15 "$url" 2>/dev/null |
rg -n -i -C 5 'sanitize|project.?name|working.?dir' | head -100 || true
doneRepository: shopware/shopware-cli
Length of output: 18234
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- repository naming rules ---'
cat -n internal/system/dockername.go | sed -n '1,100p'
cat -n internal/shop/project_creation.go | sed -n '1,80p'
cat -n cmd/project/project_proxy_list.go | sed -n '145,185p'
printf '%s\n' '--- Compose normalization source candidates ---'
for path in \
'loader/loader.go' \
'loader/loader_test.go' \
'loader/normalize.go' \
'loader/normalize_test.go' \
'loader/types.go' \
'cli/options.go'
do
url="https://raw.githubusercontent.com/compose-spec/compose-go/main/$path"
body="$(curl -LfsS --max-time 15 "$url" 2>/dev/null || true)"
if printf '%s' "$body" | rg -q 'NormalizeProjectName|ProjectNameRule'; then
printf '\nPATH: %s\n' "$path"
printf '%s' "$body" | rg -n -C 12 'NormalizeProjectName|ProjectNameRule'
fi
done
printf '%s\n' '--- standalone comparison of fallback policies ---'
python3 - <<'PY'
import re
names = ["My.Shop", "my shop", "--My.Shop--", "_My.Shop_", "...", "My__Shop", "My@Shop"]
for name in names:
lower = name.lower()
proposed = re.sub(r"[^a-z0-9_-]", "", lower).lstrip("_-")
repository = re.sub(r"[^a-z0-9_-]+", "-", lower).strip("-_")
print(f"{name!r}: proposed={proposed!r}, repository_generator={repository!r}")
PYRepository: shopware/shopware-cli
Length of output: 9176
Sanitize the fallback Compose project name before matching it.
Docker Compose removes unsupported characters and trims leading - and _. The current fallback does not. For example, My.Shop becomes myshop in Docker's project label but my.shop here, so a running project is reported as stopped. Use the same normalization and add coverage for these names.
🤖 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/proxy/stats.go` around lines 93 - 101, Update composeProjectName to
normalize the filepath.Base(projectRoot) fallback using the same Docker Compose
project-name sanitization used for matching, including lowercasing, removing
unsupported characters, and trimming leading hyphens or underscores. Preserve
.env-derived names and add coverage for names such as My.Shop.
| _, err = runDocker(ctx, "run", "-d", | ||
| "--name", ContainerName, | ||
| "--network", NetworkName, | ||
| "--restart", "unless-stopped", | ||
| "--label", configVersionLabel+"="+configVersion, | ||
| "-p", "80:80", | ||
| "-p", "443:443", | ||
| "-p", fmt.Sprintf("%d:%d", StorefrontAssetsPort, StorefrontAssetsPort), | ||
| "-v", "/var/run/docker.sock:/var/run/docker.sock:ro", | ||
| "-v", filepath.Join(dir, "traefik")+":"+containerConfigDir+":ro", | ||
| TraefikImage, | ||
| // Enable the ping service without its default route (which would | ||
| // need the "traefik" entrypoint); the dynamic config routes | ||
| // ping@internal via https://proxy.<domain>/ping instead. | ||
| "--ping.manualrouting=true", | ||
| "--providers.docker.exposedbydefault=false", | ||
| "--providers.docker.network="+NetworkName, | ||
| "--providers.file.directory="+containerConfigDir+"/dynamic", | ||
| "--providers.file.watch=true", | ||
| "--entrypoints.web.address=:80", | ||
| "--entrypoints.web.http.redirections.entrypoint.to=websecure", | ||
| "--entrypoints.web.http.redirections.entrypoint.scheme=https", | ||
| "--entrypoints.websecure.address=:443", | ||
| fmt.Sprintf("--entrypoints.sfassets.address=:%d", StorefrontAssetsPort), | ||
| ) | ||
| return err | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Bind the published proxy ports to the loopback interface.
docker run -p 80:80 publishes on all host interfaces. Every registered shop, including its admin panel, then becomes reachable from the whole local network under a hostname that carries a certificate signed by the locally trusted CA. The shared Docker network handles container-to-container routing, so loopback publication keeps self-reachability and browser access intact.
If access from other devices is a wanted feature, keep the current behavior and document it in docs/proxy.md.
🔒 Proposed change to publish on loopback only
- "-p", "80:80",
- "-p", "443:443",
- "-p", fmt.Sprintf("%d:%d", StorefrontAssetsPort, StorefrontAssetsPort),
+ "-p", "127.0.0.1:80:80",
+ "-p", "127.0.0.1:443:443",
+ "-p", fmt.Sprintf("127.0.0.1:%d:%d", StorefrontAssetsPort, StorefrontAssetsPort),📝 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.
| _, err = runDocker(ctx, "run", "-d", | |
| "--name", ContainerName, | |
| "--network", NetworkName, | |
| "--restart", "unless-stopped", | |
| "--label", configVersionLabel+"="+configVersion, | |
| "-p", "80:80", | |
| "-p", "443:443", | |
| "-p", fmt.Sprintf("%d:%d", StorefrontAssetsPort, StorefrontAssetsPort), | |
| "-v", "/var/run/docker.sock:/var/run/docker.sock:ro", | |
| "-v", filepath.Join(dir, "traefik")+":"+containerConfigDir+":ro", | |
| TraefikImage, | |
| // Enable the ping service without its default route (which would | |
| // need the "traefik" entrypoint); the dynamic config routes | |
| // ping@internal via https://proxy.<domain>/ping instead. | |
| "--ping.manualrouting=true", | |
| "--providers.docker.exposedbydefault=false", | |
| "--providers.docker.network="+NetworkName, | |
| "--providers.file.directory="+containerConfigDir+"/dynamic", | |
| "--providers.file.watch=true", | |
| "--entrypoints.web.address=:80", | |
| "--entrypoints.web.http.redirections.entrypoint.to=websecure", | |
| "--entrypoints.web.http.redirections.entrypoint.scheme=https", | |
| "--entrypoints.websecure.address=:443", | |
| fmt.Sprintf("--entrypoints.sfassets.address=:%d", StorefrontAssetsPort), | |
| ) | |
| return err | |
| } | |
| _, err = runDocker(ctx, "run", "-d", | |
| "--name", ContainerName, | |
| "--network", NetworkName, | |
| "--restart", "unless-stopped", | |
| "--label", configVersionLabel+"="+configVersion, | |
| "-p", "127.0.0.1:80:80", | |
| "-p", "127.0.0.1:443:443", | |
| "-p", fmt.Sprintf("127.0.0.1:%d:%d", StorefrontAssetsPort, StorefrontAssetsPort), | |
| "-v", "/var/run/docker.sock:/var/run/docker.sock:ro", | |
| "-v", filepath.Join(dir, "traefik")+":"+containerConfigDir+":ro", | |
| TraefikImage, | |
| // Enable the ping service without its default route (which would | |
| // need the "traefik" entrypoint); the dynamic config routes | |
| // ping@internal via https://proxy.<domain>/ping instead. | |
| "--ping.manualrouting=true", | |
| "--providers.docker.exposedbydefault=false", | |
| "--providers.docker.network="+NetworkName, | |
| "--providers.file.directory="+containerConfigDir+"/dynamic", | |
| "--providers.file.watch=true", | |
| "--entrypoints.web.address=:80", | |
| "--entrypoints.web.http.redirections.entrypoint.to=websecure", | |
| "--entrypoints.web.http.redirections.entrypoint.scheme=https", | |
| "--entrypoints.websecure.address=:443", | |
| fmt.Sprintf("--entrypoints.sfassets.address=:%d", StorefrontAssetsPort), | |
| ) | |
| return 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/proxy/traefik.go` around lines 160 - 186, Update the Docker port
mappings in the runDocker invocation to bind ports 80, 443, and
StorefrontAssetsPort explicitly to the loopback interface while preserving their
host-to-container port assignments. Keep the shared Docker network configuration
unchanged; only retain all-interface publication if the behavior is documented
in docs/proxy.md.
| if _, err := runDocker(ctx, "network", "disconnect", NetworkName, ContainerName); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| args := []string{"network", "connect"} | ||
| for _, host := range hostnames { | ||
| args = append(args, "--alias", host) | ||
| } | ||
| args = append(args, NetworkName, ContainerName) | ||
|
|
||
| _, err = runDocker(ctx, args...) | ||
| return err | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Restore the proxy connection if network connect fails.
The code disconnects Traefik from the shared network first. If the following network connect fails, Traefik stays detached. Every proxied shop then becomes unreachable, and no later command repairs it until a reconcile succeeds. Reconnect without aliases on failure, and report the original error.
🛡️ Proposed fallback
args = append(args, NetworkName, ContainerName)
- _, err = runDocker(ctx, args...)
- return err
+ if _, err := runDocker(ctx, args...); err != nil {
+ // Leaving the proxy detached would break every proxied shop, so
+ // re-attach it without aliases before reporting the failure.
+ if _, reconnectErr := runDocker(ctx, "network", "connect", NetworkName, ContainerName); reconnectErr != nil {
+ return fmt.Errorf("registering proxy hostnames failed (%w) and the proxy stayed detached from %s: %w", err, NetworkName, reconnectErr)
+ }
+
+ return fmt.Errorf("registering proxy hostnames: %w", err)
+ }
+
+ return nil📝 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.
| if _, err := runDocker(ctx, "network", "disconnect", NetworkName, ContainerName); err != nil { | |
| return err | |
| } | |
| args := []string{"network", "connect"} | |
| for _, host := range hostnames { | |
| args = append(args, "--alias", host) | |
| } | |
| args = append(args, NetworkName, ContainerName) | |
| _, err = runDocker(ctx, args...) | |
| return err | |
| } | |
| if _, err := runDocker(ctx, "network", "disconnect", NetworkName, ContainerName); err != nil { | |
| return err | |
| } | |
| args := []string{"network", "connect"} | |
| for _, host := range hostnames { | |
| args = append(args, "--alias", host) | |
| } | |
| args = append(args, NetworkName, ContainerName) | |
| if _, err := runDocker(ctx, args...); err != nil { | |
| // Leaving the proxy detached would break every proxied shop, so | |
| // re-attach it without aliases before reporting the failure. | |
| if _, reconnectErr := runDocker(ctx, "network", "connect", NetworkName, ContainerName); reconnectErr != nil { | |
| return fmt.Errorf("registering proxy hostnames failed (%w) and the proxy stayed detached from %s: %w", err, NetworkName, reconnectErr) | |
| } | |
| return fmt.Errorf("registering proxy hostnames: %w", err) | |
| } | |
| return nil |
🤖 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/proxy/traefik.go` around lines 235 - 247, Update the network
reconnection flow after the initial disconnect in the relevant proxy method: if
the aliased network connect via runDocker fails, attempt a second network
connect using only NetworkName and ContainerName to restore Traefik’s
attachment, then return the original connect error regardless of fallback
outcome.
What changed?
New command group
shopware-cli project proxy— run any number of local shops in parallel under stable hostnames, instead of everyone fighting over127.0.0.1:8000.proxy setup--domain,--skip-trust)proxy up/downproxy list/statusproxy verifyproxy teardownUnder the hood: one shared Traefik container routes by hostname (shops publish no host ports at all), a tiny DNS server embedded in the binary answers
*.shopware.local → 127.0.0.1, and an mkcert-compatible local CA provides trusted HTTPS. Proxy mode is a marker-guardedcompose.override.yaml— the basecompose.yamlstays untouched, soproject devand manualdocker composekeep working.uppointsAPP_URL, the sales-channel domain and the project config at the proxy;downrestores every value exactly.➡️ Architecture, design decisions and trade-offs:
docs/proxy.mdWhy?
The dev environment publishes fixed host ports, so a second shop can't start — anyone working on multiple projects juggles ports or stops shops. Routing by hostname removes the conflict by construction, and trusted HTTPS matters for testing payment providers locally.
How was this tested?
go test ./...green,golangci-lint run ./...— 0 issuesup/down/teardowncycles with byte-identical restore of.shopware-project.yml,.env.localand the sales-channel domain;verifyladder validated against a real corporate sudo-block scenarioRelated issue or discussion
Closes #1094, related: #939
Summary by CodeRabbit