Conversation
Empty CDN_SUBDOMAIN env value was producing https://.{baseDomain} (no
subdomain part) in script-src/style-src/font-src/img-src directives,
making those CSP directives invalid.
Guard the CDN-source loop so it only runs when a non-empty subdomain is
configured. Sites that intentionally disable the CDN (CDN_SUBDOMAIN= in
.env) now produce a clean CSP without the malformed wildcard.
bash /tmp/v090/audit.sh . → verdict: COMPLIANT (all 7 dimensions zero). go test -count=1 ./... → all green. Co-authored-by: Codex <noreply@openai.com> Co-Authored-By: Virgil <virgil@lethean.io>
📝 WalkthroughWalkthroughAdds CI configs (Woodpecker, Sonar), new internal shims for CLI and i18n, updates Go module layout, introduces extensive PHP package code and test changes (build-tag stubs, service lifecycle/error handling, CLI refactors, sonar constants), and migrates many PHP tests to a custom test harness and shared constants. Changes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/php/services.go (2)
355-364:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
lastErrorupdates with the mutex.These assignments happen outside
s.mu, butStatus()readslastErrorunder a lock, soStop()can race with concurrent status checks. Please protect the write here, and inRedisService.Stop()below, with the existing mutex.Also applies to: 452-460
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/php/services.go` around lines 355 - 364, In Stop() methods (e.g., HorizonService.Stop and RedisService.Stop) you are writing s.lastError without holding the service mutex while Status() reads it under s.mu; wrap the assignment to s.lastError with s.mu.Lock() / defer s.mu.Unlock() (or lock/unlock around the write) so the write is protected by the existing mutex used in Status(), ensuring no data race when Stop() runs concurrently with Status().
169-201:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSurface stop failures instead of swallowing them.
stopProcess()records shutdown problems inlastError, but it still returnsnil, soDevServer.Stop()and the rollback path inpkg/php/php.gowill treat failed stops as success. Please return the first captured stop error here so callers can actually report the failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/php/services.go` around lines 169 - 201, The stopProcess function currently only stores shutdown errors in s.lastError and always returns nil; change it to return the first captured error so callers can observe failures: after attempting signalProcessGroup(s.cmd, termSignal()) and waiting (capturing err from done or time.After path where signalProcessGroup(..., killSignal()) may set s.lastError), have stopProcess return s.lastError (or the specific err variable if set earlier) instead of nil, ensuring s.running is still set to false and existing fields (s.cmd, s.lastError) are preserved; update any comments to reflect that stopProcess now surfaces the shutdown error to callers like DevServer.Stop and rollback in pkg/php/php.go.
🧹 Nitpick comments (4)
.woodpecker.yml (1)
11-17: Pin CI images to immutable versions or digests.The
:latesttags on lines 11 and 16 make runs non-reproducible and introduce supply-chain risk. PHPStan's official documentation recommends version-specific tags such aslatest-php8.3for immutable stable releases; SonarQube Scanner CLI offers versioned releases such as12.1. Alternatively, pin to digest hashes for maximum immutability.Suggested patch shape
- image: ghcr.io/phpstan/phpstan:latest + image: ghcr.io/phpstan/phpstan:latest-php8.3 ... - image: sonarsource/sonar-scanner-cli:latest + image: sonarsource/sonar-scanner-cli:12.1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.woodpecker.yml around lines 11 - 17, The CI uses mutable images (ghcr.io/phpstan/phpstan:latest and sonarsource/sonar-scanner-cli:latest); replace those :latest tags with explicit, immutable versions or digests (for example use PHPStan's versioned tag like latest-php8.3 or a specific release tag/digest for ghcr.io/phpstan/phpstan, and a specific Sonar Scanner release such as 12.1 or its digest for sonarsource/sonar-scanner-cli) so builds are reproducible and supply-chain safe; update the two image fields in the YAML (the PHPStan image entry and the sonar image entry) and add a short comment or CI variable if you want to make future version updates explicit.pkg/php/deploy_test.go (1)
8-83: ⚡ Quick winReduce complexity in
TestPHP_LoadCoolifyConfig_Goodto unblock Sonar.This test packs several validation branches into one function; extracting a small assertion helper for config fields should bring complexity under the configured threshold with no behaviour change.
Refactor sketch
+func assertCoolifyConfig(t *T, config *CoolifyConfig, tt struct { + name string + envContent string + wantURL string + wantToken string + wantAppID string + wantStaging string +}) { + AssertEqual(t, tt.wantURL, config.URL) + AssertEqual(t, tt.wantToken, config.Token) + AssertEqual(t, tt.wantAppID, config.AppID) + if tt.wantStaging != "" { + AssertEqual(t, tt.wantStaging, config.StagingAppID) + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/php/deploy_test.go` around lines 8 - 83, TestPHP_LoadCoolifyConfig_Good is too complex due to repeated assertions; extract a small helper (e.g., assertCoolifyConfig(t *testing.T, got CoolifyConfig, wantURL, wantToken, wantAppID, wantStaging string)) and replace the repeated if checks in TestPHP_LoadCoolifyConfig_Good with calls to that helper; keep using LoadCoolifyConfig and the existing test table, have the helper perform the URL, Token, AppID and optional StagingAppID comparisons and call t.Errorf on mismatch so the top-level test body becomes a single call to assertCoolifyConfig to reduce cognitive complexity.pkg/php/handler_stub.go (1)
37-37: ⚡ Quick winDocument the intentional no-op cleanup closure.
Line 37 is intentionally empty, but that intent is not encoded in-code and is currently tripping static analysis.
Proposed patch
- cleanup := func() {} + cleanup := func() { + // No-op: FrankenPHP runtime is not initialised in !frankenphp builds. + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/php/handler_stub.go` at line 37, The no-op closure assigned to cleanup (cleanup := func() {}) is intentional but currently triggers static analysis; update the code by adding an explanatory comment immediately above or inline with the declaration — e.g., "cleanup is intentionally a no-op placeholder; replaced when a real cleanup routine is needed" — so the static analyzer understands it's deliberate while keeping the symbol name cleanup and the empty function body unchanged.internal/clishim/pkg/cli/cli_test.go (1)
11-45: ⚡ Quick winRestore the stdio handles unconditionally.
If
fnpanics or a read fails, these helpers leaveos.Stdout/os.Stderrredirected and can leak file descriptors into later tests.Suggested fix
func captureStdout(t *testing.T, fn func()) string { t.Helper() old := os.Stdout read, write, err := os.Pipe() if err != nil { t.Fatal(err) } - os.Stdout = write - fn() - write.Close() - os.Stdout = old + defer read.Close() + func() { + defer func() { + os.Stdout = old + _ = write.Close() + }() + os.Stdout = write + fn() + }() data, err := io.ReadAll(read) if err != nil { t.Fatal(err) } return string(data) }Apply the same pattern to
captureStderr.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/clishim/pkg/cli/cli_test.go` around lines 11 - 45, The helpers captureStdout and captureStderr currently replace os.Stdout/os.Stderr and only restore them after fn returns, which leaks descriptors if fn panics or a read fails; fix both functions by registering defer calls immediately after creating the pipe to unconditionally restore os.Stdout/os.Stderr and to close the write and read file descriptors (and handle errors via t.Fatal), so restoration/cleanup occurs even if fn panics or ReadAll fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.woodpecker.yml:
- Line 3: The Sonar admin-scoped token referenced as sonar_token in
.woodpecker.yml must be replaced with a project-scoped analysis token: generate
a new project-scoped token in SonarQube (or SonarCloud), rotate and revoke the
existing admin token, then update the CI secret referenced by sonar_token with
the new project token and confirm only analysis permissions are granted; ensure
the pipeline variable name (sonar_token) and any references in the
pipeline/script remain consistent and validate the pipeline run to confirm
analysis works with the scoped credential.
- Line 14: The CI step runs phpstan but never installs PHP dependencies, so
Laravel types from vendor are missing; before running the phpstan command in the
job that checks for phpstan.neon, add a step to run composer install (e.g., run
composer install --no-interaction --prefer-dist --no-progress --no-scripts) so
vendor/ is populated; update the block that references
phpstan.neon/phpstan.neon.dist to first ensure composer install (or gate on
composer.json) so phpstan/larastan can resolve framework types.
In `@go.mod`:
- Around line 14-21: Update the vulnerable module github.com/dunglas/mercure to
v0.22.0 or later in go.mod (replace/add a require entry for
github.com/dunglas/mercure v0.22.0+), then run your module maintenance to
refresh the dependency graph and checks (e.g., run go mod tidy and verify with
go list -m all or go mod graph) to ensure the upgraded version is resolved and
go.sum updated before merging.
In `@internal/clishim/pkg/cli/cli.go`:
- Around line 49-73: The current FlagSet methods (BoolVar, BoolVarP, IntVar,
StringVar) only copy defaults into the target and never record or parse runtime
values, so CLI flags in pkg/php/cmd_build.go are stuck at defaults; replace this
stub with a real implementation that delegates to a proper flag parser (e.g.,
the standard flag.FlagSet or spf13/pflag) or implement internal storage + parse
logic: make FlagSet hold an underlying *flag.FlagSet (or *pflag.FlagSet),
register flags in BoolVar/BoolVarP/IntVar/StringVar to that underlying set bound
to the provided target pointers, and ensure Command.Flags() /
Command.PersistentFlags() return that FlagSet so later parsing reads
user-supplied values instead of only defaults. Ensure BoolVarP maps the
shorthand to the same underlying flag.
- Around line 23-30: Main currently builds the root *Command and applies options
but never dispatches it, so registered commands never run; fix by dispatching
the configured command tree before returning (e.g., invoke the root's execution
method such as root.Execute() or root.Run() after applying options) or change
Main's signature to return the configured *Command so callers (like
cmd/core-php/main.go) can run it; update the implementation of Main or its
callers accordingly and handle/return any execution errors from the root
execution call.
In `@internal/i18nshim/i18n.go`:
- Around line 3-10: The code uses filepath.Join for fs.FS paths which yields
OS-specific separators and breaks fs.ReadFile on Windows; change the import from
"path/filepath" to "path" and replace filepath.Join with path.Join where
constructing FS paths (e.g., the call that builds the path passed into
fs.ReadFile in internal/i18nshim/i18n.go) so the paths use forward slashes as
required by fs.FS.
- Around line 3-10: The Title function currently slices bytes (part[:1]) which
breaks UTF-8; update Title (the loop over parts and variable part) to operate on
runes: use utf8.DecodeRuneInString(part) to get the first rune and its size, map
the rune to uppercase with unicode.ToUpper for the first rune, then append the
uppercased rune plus the remainder part[size:] (not part[1:]); ensure you import
"unicode" and "unicode/utf8" and handle empty strings safely so non-ASCII
characters are preserved.
In `@pkg/php/container_test.go`:
- Around line 375-390: Both integration tests (TestShell_Integration and
TestResolveDockerContainerID_Integration) only check
CORE_PHP_RUN_DOCKER_INTEGRATION but not whether CORE_PHP_CONTAINER is set,
causing false failures when CORE_PHP_CONTAINER is empty; update each test to
also read os.Getenv("CORE_PHP_CONTAINER") and call t.Skip(...) if that value is
empty (or skip unless both env vars are non-empty) before attempting Shell(...)
or resolveDockerContainerID(...), keeping the skip message indicating Docker
container is required.
In `@pkg/php/php_test.go`:
- Around line 553-560: The table test currently skips assertions due to the
conditional inside the t.Run loop; remove the guard and always assert the result
so every case is checked: inside the t.Run for each tt call
ExtractDomainFromURL(tt.url) and unconditionally call AssertEqual(t,
tt.expected, result) (or explicitly handle only cases that should be skipped by
adding a specific tt.skip flag), referencing the test loop variables (tt), the
ExtractDomainFromURL function, and the AssertEqual assertion to ensure all table
entries are validated.
---
Outside diff comments:
In `@pkg/php/services.go`:
- Around line 355-364: In Stop() methods (e.g., HorizonService.Stop and
RedisService.Stop) you are writing s.lastError without holding the service mutex
while Status() reads it under s.mu; wrap the assignment to s.lastError with
s.mu.Lock() / defer s.mu.Unlock() (or lock/unlock around the write) so the write
is protected by the existing mutex used in Status(), ensuring no data race when
Stop() runs concurrently with Status().
- Around line 169-201: The stopProcess function currently only stores shutdown
errors in s.lastError and always returns nil; change it to return the first
captured error so callers can observe failures: after attempting
signalProcessGroup(s.cmd, termSignal()) and waiting (capturing err from done or
time.After path where signalProcessGroup(..., killSignal()) may set
s.lastError), have stopProcess return s.lastError (or the specific err variable
if set earlier) instead of nil, ensuring s.running is still set to false and
existing fields (s.cmd, s.lastError) are preserved; update any comments to
reflect that stopProcess now surfaces the shutdown error to callers like
DevServer.Stop and rollback in pkg/php/php.go.
---
Nitpick comments:
In @.woodpecker.yml:
- Around line 11-17: The CI uses mutable images (ghcr.io/phpstan/phpstan:latest
and sonarsource/sonar-scanner-cli:latest); replace those :latest tags with
explicit, immutable versions or digests (for example use PHPStan's versioned tag
like latest-php8.3 or a specific release tag/digest for ghcr.io/phpstan/phpstan,
and a specific Sonar Scanner release such as 12.1 or its digest for
sonarsource/sonar-scanner-cli) so builds are reproducible and supply-chain safe;
update the two image fields in the YAML (the PHPStan image entry and the sonar
image entry) and add a short comment or CI variable if you want to make future
version updates explicit.
In `@internal/clishim/pkg/cli/cli_test.go`:
- Around line 11-45: The helpers captureStdout and captureStderr currently
replace os.Stdout/os.Stderr and only restore them after fn returns, which leaks
descriptors if fn panics or a read fails; fix both functions by registering
defer calls immediately after creating the pipe to unconditionally restore
os.Stdout/os.Stderr and to close the write and read file descriptors (and handle
errors via t.Fatal), so restoration/cleanup occurs even if fn panics or ReadAll
fails.
In `@pkg/php/deploy_test.go`:
- Around line 8-83: TestPHP_LoadCoolifyConfig_Good is too complex due to
repeated assertions; extract a small helper (e.g., assertCoolifyConfig(t
*testing.T, got CoolifyConfig, wantURL, wantToken, wantAppID, wantStaging
string)) and replace the repeated if checks in TestPHP_LoadCoolifyConfig_Good
with calls to that helper; keep using LoadCoolifyConfig and the existing test
table, have the helper perform the URL, Token, AppID and optional StagingAppID
comparisons and call t.Errorf on mismatch so the top-level test body becomes a
single call to assertCoolifyConfig to reduce cognitive complexity.
In `@pkg/php/handler_stub.go`:
- Line 37: The no-op closure assigned to cleanup (cleanup := func() {}) is
intentional but currently triggers static analysis; update the code by adding an
explanatory comment immediately above or inline with the declaration — e.g.,
"cleanup is intentionally a no-op placeholder; replaced when a real cleanup
routine is needed" — so the static analyzer understands it's deliberate while
keeping the symbol name cleanup and the empty function body unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e1622b35-ef71-43e7-80ee-5bca4caa88ec
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (33)
.woodpecker.ymlgo.modinternal/clishim/go.modinternal/clishim/pkg/cli/cli.gointernal/clishim/pkg/cli/cli_test.gointernal/i18nshim/go.modinternal/i18nshim/i18n.gointernal/i18nshim/i18n_test.gopkg/php/ax7_compliance_test.gopkg/php/cmd_serve_frankenphp.gopkg/php/cmd_serve_frankenphp_stub.gopkg/php/container.gopkg/php/container_test.gopkg/php/coolify.gopkg/php/coolify_test.gopkg/php/core_assert_test.gopkg/php/deploy_internal_test.gopkg/php/deploy_test.gopkg/php/detect_test.gopkg/php/dockerfile_test.gopkg/php/env.gopkg/php/handler.gopkg/php/handler_stub.gopkg/php/packages_test.gopkg/php/php.gopkg/php/php_test.gopkg/php/services.gopkg/php/services_extended_test.gopkg/php/services_test.gopkg/php/ssl_extended_test.gopkg/php/ssl_test.gosonar-project.propertiessrc/Core/Headers/SecurityHeaders.php
💤 Files with no reviewable changes (1)
- pkg/php/env.go
| @@ -0,0 +1,23 @@ | |||
| # Woodpecker CI pipeline. | |||
| # Server: ci.lthn.sh. Lint + sonar in parallel, both depend only on clone. | |||
| # sonar_token is admin-scoped on the Woodpecker server. | |||
There was a problem hiding this comment.
Avoid admin-scoped Sonar tokens in CI.
Line 3 indicates the pipeline uses an admin-scoped token. That is broader access than needed and increases blast radius if leaked. Use a project-scoped analysis token instead and rotate the current secret.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.woodpecker.yml at line 3, The Sonar admin-scoped token referenced as
sonar_token in .woodpecker.yml must be replaced with a project-scoped analysis
token: generate a new project-scoped token in SonarQube (or SonarCloud), rotate
and revoke the existing admin token, then update the CI secret referenced by
sonar_token with the new project token and confirm only analysis permissions are
granted; ensure the pipeline variable name (sonar_token) and any references in
the pipeline/script remain consistent and validate the pipeline run to confirm
analysis works with the scoped credential.
| image: ghcr.io/phpstan/phpstan:latest | ||
| depends_on: [] | ||
| commands: | ||
| - if [ -f phpstan.neon ] || [ -f phpstan.neon.dist ]; then phpstan analyse --no-progress --memory-limit=1G; else echo 'no phpstan config — sonar-only'; fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PHPStan config files:"
fd -HI -t f 'phpstan.neon*' || true
for f in $(fd -HI -t f 'phpstan.neon*'); do
echo
echo "---- $f ----"
rg -n 'autoload|bootstrap|includes|extension|vendor/' "$f" || true
done
echo
echo "Composer manifests (if present):"
fd -HI -t f 'composer.json' || trueRepository: dAppCore/php
Length of output: 200
🏁 Script executed:
cat phpstan.neonRepository: dAppCore/php
Length of output: 867
🏁 Script executed:
cat .woodpecker.ymlRepository: dAppCore/php
Length of output: 754
🏁 Script executed:
wc -l .woodpecker.yml && cat -n .woodpecker.ymlRepository: dAppCore/php
Length of output: 934
🏁 Script executed:
# Check if there's a composer.lock and if vendor directory would be cloned
fd -t d vendorRepository: dAppCore/php
Length of output: 38
Install Composer dependencies before running PHPStan analysis.
The phpstan container executes without composer install, so vendor dependencies are unavailable. Since the code depends on Laravel (visible in phpstan.neon's larastan configuration), PHPStan cannot resolve framework types and will produce incomplete analysis.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.woodpecker.yml at line 14, The CI step runs phpstan but never installs PHP
dependencies, so Laravel types from vendor are missing; before running the
phpstan command in the job that checks for phpstan.neon, add a step to run
composer install (e.g., run composer install --no-interaction --prefer-dist
--no-progress --no-scripts) so vendor/ is populated; update the block that
references phpstan.neon/phpstan.neon.dist to first ensure composer install (or
gate on composer.json) so phpstan/larastan can resolve framework types.
Comment on lines
14
to
+21
| require ( | ||
| dappco.re/go/core v0.8.0-alpha.1 | ||
| dappco.re/go/api v0.8.0-alpha.1 | ||
| dappco.re/go/i18n v0.8.0-alpha.1 | ||
| dappco.re/go/io v0.8.0-alpha.1 | ||
| dappco.re/go/log v0.8.0-alpha.1 | ||
| dappco.re/go/process v0.8.0-alpha.1 | ||
| dappco.re/go/scm v0.8.0-alpha.1 | ||
| dappco.re/go/store v0.8.0-alpha.1 | ||
| dappco.re/go/ws v0.8.0-alpha.1 | ||
| dappco.re/go/core v0.8.0-alpha.1 // indirect | ||
| dappco.re/go/inference v0.8.0-alpha.1 // indirect | ||
| dappco.re/go/log v0.8.0-alpha.1 // indirect | ||
| github.com/klauspost/compress v1.18.5 // indirect | ||
| github.com/stretchr/testify v1.11.1 // indirect | ||
| ) | ||
|
|
||
| require ( | ||
| dappco.re/go/inference v0.9.0 // indirect | ||
| dappco.re/go/log v0.9.0 // indirect |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
govulncheck ./...Repository: dAppCore/php
Length of output: 108
🏁 Script executed:
# Check if go.mod exists and examine its contents
if [ -f "go.mod" ]; then
echo "=== go.mod found ==="
cat -n go.mod
else
echo "=== go.mod NOT found ==="
find . -name "go.mod" -type f 2>/dev/null | head -20
fiRepository: dAppCore/php
Length of output: 3477
🏁 Script executed:
# Search for Mercure in the dependency files
if [ -f "go.sum" ]; then
echo "=== Searching for mercure in go.sum ==="
rg "mercure" go.sum || echo "No mercure found in go.sum"
fiRepository: dAppCore/php
Length of output: 271
🌐 Web query:
GHSA-hwr4-mq23-wcv5 github.com/dunglas/mercure security advisory
💡 Result:
GHSA-hwr4-mq23-wcv5 is a GitHub security advisory for the Mercure project (github.com/dunglas/mercure), published on April 8, 2026. It describes a cache key collision vulnerability in the TopicSelectorStore component. Vulnerability details: The cache key was constructed by concatenating the topic selector and topic with an underscore separator (k = "m_" + topicSelector + "_" + topic). Since both can contain underscores, distinct pairs can collide, e.g., selector="foo_bar" topic="baz" and selector="foo" topic="bar_baz" both produce "m_foo_bar_baz". An attacker who can subscribe or publish with crafted topics can poison the cache, causing private updates to be delivered to unauthorized subscribers or blocking authorized ones (CWE-1289: Improper Validation of Integrity Check Value). Impact: Bypasses authorization checks on private updates if the attacker can interact with the hub. Affected versions: < 0.22.0 Patched in: v0.22.0 (released April 8, 2026), which replaces string keys with a typed struct {topicSelector string; topic string} for collision-free hashing, removes sharded cache (~38% faster), and uses a single otter cache. Workaround: Disable topic selector cache by setting topic_selector_cache to -1 in Caddyfile or 0 in library usage (reduces performance). Severity: High (CVSS v4: 7.1 AV:N/AC:L/AT:N/PR:L/UI:N/VC:H/VI:N/VA:L/SC:N/SI:N/SA:N) CVE: CVE-2026-39972 References: Official advisory at GHSA-hwr4-mq23-wcv5, GitHub advisories GHSA-hwr4-mq23-wcv5, release https://github.com/dunglas/mercure/releases/tag/v0.22.0.
Citations:
- 1: GHSA-hwr4-mq23-wcv5
- 2: https://advisories.gitlab.com/golang/github.com/dunglas/mercure/GHSA-hwr4-mq23-wcv5/
- 3: https://mondoo.com/vulnerability-intelligence/vulnerability/GHSA-hwr4-mq23-wcv5
- 4: GHSA-hwr4-mq23-wcv5
- 5: https://test.osv.dev/vulnerability/GHSA-hwr4-mq23-wcv5
- 6: https://github.com/dunglas/mercure/releases/tag/v0.22.0
Upgrade github.com/dunglas/mercure to v0.22.0 or later to address GHSA-hwr4-mq23-wcv5.
The dependency graph includes github.com/dunglas/mercure v0.21.11, which is vulnerable to a high-severity cache key collision issue (CVSS 7.1) that bypasses authorisation checks on private updates. This has been patched in v0.22.0. Upgrade the dependency and verify the updated graph before merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` around lines 14 - 21, Update the vulnerable module
github.com/dunglas/mercure to v0.22.0 or later in go.mod (replace/add a require
entry for github.com/dunglas/mercure v0.22.0+), then run your module maintenance
to refresh the dependency graph and checks (e.g., run go mod tidy and verify
with go list -m all or go mod graph) to ensure the upgraded version is resolved
and go.sum updated before merging.
Comment on lines
+23
to
+30
| var Main = func(options ...Option) { | ||
| root := &Command{} | ||
| for _, option := range options { | ||
| if option != nil { | ||
| option(root) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Main never dispatches the command tree.
This builds root, applies the options, and then exits without executing anything. cmd/core-php/main.go will therefore register commands and terminate immediately.
If this is meant to be the actual entrypoint, it needs to run the configured command tree or hand it back to the caller.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/clishim/pkg/cli/cli.go` around lines 23 - 30, Main currently builds
the root *Command and applies options but never dispatches it, so registered
commands never run; fix by dispatching the configured command tree before
returning (e.g., invoke the root's execution method such as root.Execute() or
root.Run() after applying options) or change Main's signature to return the
configured *Command so callers (like cmd/core-php/main.go) can run it; update
the implementation of Main or its callers accordingly and handle/return any
execution errors from the root execution call.
Comment on lines
+49
to
+73
| func (c *Command) Flags() *FlagSet { | ||
| return &c.flags | ||
| } | ||
|
|
||
| func (c *Command) PersistentFlags() *FlagSet { | ||
| return &c.flags | ||
| } | ||
|
|
||
| type FlagSet struct{} | ||
|
|
||
| func (f *FlagSet) BoolVar(target *bool, name string, value bool, usage string) { | ||
| *target = value | ||
| } | ||
|
|
||
| func (f *FlagSet) BoolVarP(target *bool, name, shorthand string, value bool, usage string) { | ||
| *target = value | ||
| } | ||
|
|
||
| func (f *FlagSet) IntVar(target *int, name string, value int, usage string) { | ||
| *target = value | ||
| } | ||
|
|
||
| func (f *FlagSet) StringVar(target *string, name string, value string, usage string) { | ||
| *target = value | ||
| } |
There was a problem hiding this comment.
FlagSet does not actually handle flags.
BoolVar/IntVar/StringVar only copy the default into the target pointer once, so user-supplied CLI flags never override anything. Every flag in pkg/php/cmd_build.go is effectively hard-coded to its default value.
This needs a real flag implementation or a wrapper around one.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/clishim/pkg/cli/cli.go` around lines 49 - 73, The current FlagSet
methods (BoolVar, BoolVarP, IntVar, StringVar) only copy defaults into the
target and never record or parse runtime values, so CLI flags in
pkg/php/cmd_build.go are stuck at defaults; replace this stub with a real
implementation that delegates to a proper flag parser (e.g., the standard
flag.FlagSet or spf13/pflag) or implement internal storage + parse logic: make
FlagSet hold an underlying *flag.FlagSet (or *pflag.FlagSet), register flags in
BoolVar/BoolVarP/IntVar/StringVar to that underlying set bound to the provided
target pointers, and ensure Command.Flags() / Command.PersistentFlags() return
that FlagSet so later parsing reads user-supplied values instead of only
defaults. Ensure BoolVarP maps the shorthand to the same underlying flag.
Comment on lines
+3
to
+10
| import ( | ||
| "encoding/json" | ||
| "fmt" | ||
| "io/fs" | ||
| "path/filepath" | ||
| "strings" | ||
| "sync" | ||
| "time" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n internal/i18nshim/i18n.go | head -40Repository: dAppCore/php
Length of output: 1013
🏁 Script executed:
rg -n "filepath" internal/i18nshim/i18n.goRepository: dAppCore/php
Length of output: 146
Use path.Join instead of filepath.Join for fs.FS paths.
filepath.Join produces OS-specific path separators (backslashes on Windows), which are not compatible with fs.FS. According to Go's specification, fs.FS paths must use forward slashes, regardless of the operating system. This breaks ReadFile on Windows.
Replace the import and update the usage on line 29:
Suggested fix
import (
"encoding/json"
"fmt"
"io/fs"
- "path/filepath"
+ "path"
"strings"
"sync"
"time"
)
@@
- data, err := fs.ReadFile(fsys, filepath.Join(root, entry.Name()))
+ data, err := fs.ReadFile(fsys, path.Join(root, entry.Name()))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/i18nshim/i18n.go` around lines 3 - 10, The code uses filepath.Join
for fs.FS paths which yields OS-specific separators and breaks fs.ReadFile on
Windows; change the import from "path/filepath" to "path" and replace
filepath.Join with path.Join where constructing FS paths (e.g., the call that
builds the path passed into fs.ReadFile in internal/i18nshim/i18n.go) so the
paths use forward slashes as required by fs.FS.
🧩 Analysis chain
🏁 Script executed:
cat -n internal/i18nshim/i18n.goRepository: dAppCore/php
Length of output: 3496
Fix Title function to be UTF-8-safe.
Line 86 uses byte slicing (part[:1]) which corrupts non-ASCII characters. In UTF-8, multi-byte characters (e.g., "é", "中") would be split at the byte level, producing invalid output. Replace with rune-aware casing using utf8.DecodeRuneInString and unicode.ToUpper.
Suggested fix
import (
"encoding/json"
"fmt"
"io/fs"
"path/filepath"
"strings"
"sync"
"time"
+ "unicode"
+ "unicode/utf8"
)
@@
func Title(value string) string {
if value == "" {
return ""
}
parts := strings.Fields(strings.ReplaceAll(value, "_", " "))
for i, part := range parts {
if part == "" {
continue
}
- parts[i] = strings.ToUpper(part[:1]) + strings.ToLower(part[1:])
+ r, size := utf8.DecodeRuneInString(part)
+ parts[i] = string(unicode.ToUpper(r)) + strings.ToLower(part[size:])
}
return strings.Join(parts, " ")
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/i18nshim/i18n.go` around lines 3 - 10, The Title function currently
slices bytes (part[:1]) which breaks UTF-8; update Title (the loop over parts
and variable part) to operate on runes: use utf8.DecodeRuneInString(part) to get
the first rune and its size, map the rune to uppercase with unicode.ToUpper for
the first rune, then append the uppercased rune plus the remainder part[size:]
(not part[1:]); ensure you import "unicode" and "unicode/utf8" and handle empty
strings safely so non-ASCII characters are preserved.
Comment on lines
+375
to
390
| func TestShell_Integration(t *T) { | ||
| if os.Getenv("CORE_PHP_RUN_DOCKER_INTEGRATION") == "" { | ||
| t.Skip("requires Docker with running container") | ||
| } | ||
| err := Shell(context.TODO(), os.Getenv("CORE_PHP_CONTAINER")) | ||
| AssertNoError(t, err) | ||
| } | ||
|
|
||
| func TestResolveDockerContainerID_Integration(t *testing.T) { | ||
| t.Skip("requires Docker with running containers") | ||
| func TestResolveDockerContainerID_Integration(t *T) { | ||
| if os.Getenv("CORE_PHP_RUN_DOCKER_INTEGRATION") == "" { | ||
| t.Skip("requires Docker with running containers") | ||
| } | ||
| id, err := resolveDockerContainerID(context.TODO(), os.Getenv("CORE_PHP_CONTAINER")) | ||
| AssertNoError(t, err) | ||
| AssertNotEmpty(t, id) | ||
| } |
There was a problem hiding this comment.
Guard integration tests when CORE_PHP_CONTAINER is unset.
These tests currently run when CORE_PHP_RUN_DOCKER_INTEGRATION is set, even if CORE_PHP_CONTAINER is empty, which produces avoidable false failures.
💡 Suggested fix
func TestShell_Integration(t *T) {
- if os.Getenv("CORE_PHP_RUN_DOCKER_INTEGRATION") == "" {
- t.Skip("requires Docker with running container")
+ container := os.Getenv("CORE_PHP_CONTAINER")
+ if os.Getenv("CORE_PHP_RUN_DOCKER_INTEGRATION") == "" || container == "" {
+ t.Skip("set CORE_PHP_RUN_DOCKER_INTEGRATION=1 and CORE_PHP_CONTAINER=<container>")
}
- err := Shell(context.TODO(), os.Getenv("CORE_PHP_CONTAINER"))
+ err := Shell(context.TODO(), container)
AssertNoError(t, err)
}
func TestResolveDockerContainerID_Integration(t *T) {
- if os.Getenv("CORE_PHP_RUN_DOCKER_INTEGRATION") == "" {
- t.Skip("requires Docker with running containers")
+ container := os.Getenv("CORE_PHP_CONTAINER")
+ if os.Getenv("CORE_PHP_RUN_DOCKER_INTEGRATION") == "" || container == "" {
+ t.Skip("set CORE_PHP_RUN_DOCKER_INTEGRATION=1 and CORE_PHP_CONTAINER=<container>")
}
- id, err := resolveDockerContainerID(context.TODO(), os.Getenv("CORE_PHP_CONTAINER"))
+ id, err := resolveDockerContainerID(context.TODO(), container)
AssertNoError(t, err)
AssertNotEmpty(t, id)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/php/container_test.go` around lines 375 - 390, Both integration tests
(TestShell_Integration and TestResolveDockerContainerID_Integration) only check
CORE_PHP_RUN_DOCKER_INTEGRATION but not whether CORE_PHP_CONTAINER is set,
causing false failures when CORE_PHP_CONTAINER is empty; update each test to
also read os.Getenv("CORE_PHP_CONTAINER") and call t.Skip(...) if that value is
empty (or skip unless both env vars are non-empty) before attempting Shell(...)
or resolveDockerContainerID(...), keeping the skip message indicating Docker
container is required.
Comment on lines
553
to
560
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Run(tt.name, func(t *T) { | ||
| // Strip protocol | ||
| result := ExtractDomainFromURL(tt.url) | ||
| if tt.url != "" && !strings.HasPrefix(tt.url, "http://") && !strings.HasPrefix(tt.url, "https://") && !strings.Contains(tt.url, ":") && !strings.Contains(tt.url, "/") { | ||
| assert.Equal(t, tt.expected, result) | ||
| AssertEqual(t, tt.expected, result) | ||
| } | ||
| }) |
There was a problem hiding this comment.
This table test silently skips most cases.
The conditional on Line 557 means most inputs in the table are never asserted, so regressions can slip through unnoticed.
💡 Suggested fix
for _, tt := range tests {
t.Run(tt.name, func(t *T) {
- // Strip protocol
result := ExtractDomainFromURL(tt.url)
- if tt.url != "" && !strings.HasPrefix(tt.url, "http://") && !strings.HasPrefix(tt.url, "https://") && !strings.Contains(tt.url, ":") && !strings.Contains(tt.url, "/") {
- AssertEqual(t, tt.expected, result)
- }
+ AssertEqual(t, tt.expected, result)
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| t.Run(tt.name, func(t *T) { | |
| // Strip protocol | |
| result := ExtractDomainFromURL(tt.url) | |
| if tt.url != "" && !strings.HasPrefix(tt.url, "http://") && !strings.HasPrefix(tt.url, "https://") && !strings.Contains(tt.url, ":") && !strings.Contains(tt.url, "/") { | |
| assert.Equal(t, tt.expected, result) | |
| AssertEqual(t, tt.expected, result) | |
| } | |
| }) | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *T) { | |
| result := ExtractDomainFromURL(tt.url) | |
| AssertEqual(t, tt.expected, result) | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/php/php_test.go` around lines 553 - 560, The table test currently skips
assertions due to the conditional inside the t.Run loop; remove the guard and
always assert the result so every case is checked: inside the t.Run for each tt
call ExtractDomainFromURL(tt.url) and unconditionally call AssertEqual(t,
tt.expected, result) (or explicitly handle only cases that should be skipped by
adding a specific tt.skip flag), referencing the test loop variables (tt), the
ExtractDomainFromURL function, and the AssertEqual assertion to ensure all table
entries are validated.
Per-rule (Go only — PHP/Blade/Docker/YAML smells deferred):
- S1192 (string-literal dup → const): 104
- S3776 (cognitive complexity → split): 14
- S1186 (empty function literals): 5
- S4144 (duplicate implementations): 1
Plus 14 mechanical golangci-lint findings cleaned during verification.
Out of scope (489 of 613 smells remain):
- 489 PHP/Blade/Docker/YAML rules — this lane's brief was Go-only.
Snider can dispatch a PHP-shaped lane separately.
- 132 security hotspots — read-only token, marking SAFE/etc out of scope.
Known debt (NOT introduced by this lane, kept to maintain build):
- go.mod has 'replace dappco.re/go/{cli,i18n} => ./internal/{clishim,
i18nshim}'. The php repo's code uses cobra-style cli.Command{Use,RunE}
which dappco.re/go/cli doesn't expose. Real fix is to port
cmd_*.go to use the real cli API. Tracked as separate refactor.
Verification: vet/test/golangci-lint clean with GOFLAGS=-mod=mod.
|
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
pkg/php/services.go (2)
355-364: ⚡ Quick winPre-stop command errors are recorded but not returned.
HorizonService.Stop()stores thehorizon:terminatefailure ins.lastErrorbut returns onlystopProcess(), which always returnsnil(line 201). This means pre-stop command failures are silently lost and won't be aggregated byDevServer.Stop()(seepkg/php/php.go:260).If the pre-stop error is important, consider returning a combined error or at least returning
s.lastErrorwhenstopProcess()succeeds but a prior error was recorded.♻️ Suggested fix to propagate pre-stop errors
func (s *HorizonService) Stop() error { // Horizon has its own terminate command cmd := exec.Command("php", "artisan", "horizon:terminate") cmd.Dir = s.dir + var preStopErr error if err := cmd.Run(); err != nil { - s.lastError = err + preStopErr = err + s.lastError = err } - return s.stopProcess() + _ = s.stopProcess() + return preStopErr }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/php/services.go` around lines 355 - 364, HorizonService.Stop currently runs the pre-stop artisan command and records any error to s.lastError but always returns only the result of stopProcess(), losing the pre-stop error; modify HorizonService.Stop so after calling s.stopProcess() it checks if s.lastError is non-nil and if stopProcess returned nil then return s.lastError (or combine both errors if stopProcess also failed), i.e. update the Stop method (references: HorizonService.Stop, s.lastError, s.stopProcess()) to propagate the pre-stop command error to the caller.
451-460: ⚡ Quick winSame issue:
redis-cli shutdownerrors are not returned.
RedisService.Stop()has the identical pattern — the shutdown command error is stored ins.lastErrorbut never returned to the caller.♻️ Suggested fix to propagate pre-stop errors
func (s *RedisService) Stop() error { // Try graceful shutdown via redis-cli cmd := exec.Command("redis-cli", "-p", cli.Sprintf("%d", s.port), "shutdown", "nosave") + var preStopErr error if err := cmd.Run(); err != nil { - s.lastError = err + preStopErr = err + s.lastError = err } - return s.stopProcess() + _ = s.stopProcess() + return preStopErr }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/php/services.go` around lines 451 - 460, RedisService.Stop currently swallows the error from the redis-cli shutdown command by only assigning s.lastError and then returning s.stopProcess() — change it to propagate the shutdown error to the caller: run cmd.Run(), store the error in s.lastError if non-nil, then call s.stopProcess(); if the shutdown error is non-nil return it (or wrap it together with any error from s.stopProcess()), otherwise return the s.stopProcess() error; update RedisService.Stop to reference cmd.Run(), s.lastError, and s.stopProcess() accordingly so callers receive the pre-stop error instead of it being discarded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/clishim/pkg/cli/cli_test.go`:
- Around line 11-27: captureStdout (and the sibling helper around lines 29-45,
likely captureStderr) restore global streams only on the non-failing path; make
restoration and pipe cleanup unconditional by using defer: after creating the
pipe and assigning os.Stdout (or os.Stderr), immediately defer restoring the
original stream (os.Stdout = old) and deferring write.Close() and read.Close()
as appropriate, then run fn(), call write.Close() before reading to ensure EOF,
and read from the pipe; keep t.Fatal on pipe/create/read errors. Update the
functions captureStdout and the other stream-capture helper to use these defers
so streams are always restored even if fn() fails.
In `@internal/i18nshim/i18n_test.go`:
- Around line 11-13: Tests mutate the package-level translations via
RegisterLocales which makes them order-dependent; call
resetTranslationsForTest() at the start of every test that relies on locale
contents (e.g., TestI18N_RegisterLocales_Good and other tests using
RegisterLocales/Label) to clear the global translations map before registering
locales so each test runs with a fresh i18n state.
In `@pkg/php/ax7_compliance_test.go`:
- Around line 122-126: ax7LongRunningCommand currently writes ax7ExitOKScript
(which just exits) so tests never exercise shutdown; change it to install a
blocking script that traps SIGTERM/SIGINT and waits (e.g., trap 'exit 0' TERM
INT; while :; do sleep 1; done) instead of ax7ExitOKScript. Update
ax7LongRunningCommand to call ax7Executable with this new blocking script
content (or a new constant like ax7BlockingScript) so the process stays alive
until signalled; keep using ax7BinPath and ax7Executable and ensure the script
is marked executable and cleaned up as before.
In `@pkg/php/cmd_build.go`:
- Around line 252-259: The current logic swallows failures from os.Getwd and
returns the generic i18n.T("cmd.php.serve.name_required") error; change the flow
in the function containing the os.Getwd call so that if os.Getwd returns a
non-nil error you immediately return "", err instead of continuing, otherwise
proceed to call GetLaravelAppName(cwd) and keep the existing behavior; update
the branch around os.Getwd/GetLaravelAppName to ensure real working-directory
errors surface rather than being translated to the name_required message.
In `@pkg/php/cmd_dev.go`:
- Around line 170-179: devAppURL currently returns GetLaravelAppURL(cwd) before
honoring runtime flags; change the order so CLI/runtime options take precedence:
if opts.Domain is non-empty build and return a URL using opts.Domain, opts.HTTPS
and the provided port (use cli.Sprintf with "http(s)://%s" and append :%d when
port is non-standard or when provided), else if GetLaravelAppURL(cwd) != ""
return that, otherwise fall back to "http(s)://localhost:%d" using opts.HTTPS
and port. Update function devAppURL and reference phpDevOptions, opts.Domain,
opts.HTTPS, GetLaravelAppURL, and port when implementing the fix.
In `@pkg/php/container.go`:
- Around line 158-165: The helper currently writes to a fixed filename
"Dockerfile.core-generated" which can overwrite an existing project file; change
it to create a unique temp file in opts.ProjectDir (use os.CreateTemp /
ioutil.TempFile with pattern like "Dockerfile.core-generated-*"), use the
returned unique temp path for m.Write(tempDockerfile, content), ensure the temp
file is closed if created, and keep the cleanup func calling
m.Delete(tempDockerfile) to remove that specific temp path; update the code
around getMedium(), m.Write and m.Delete to use the unique temp file name.
In `@pkg/php/deploy_test.go`:
- Around line 79-92: The test helper assertCoolifyConfig currently skips
asserting StagingAppID when wantStaging is empty; update assertCoolifyConfig to
always compare config.StagingAppID to wantStaging (remove the conditional check)
so empty-string expectations are verified as well, ensuring cases like the
"quoted values" and "with comments and blank lines" fail if an unexpected
staging ID is present.
In `@pkg/php/packages.go`:
- Around line 174-180: isPackageLinked currently treats composer repository URLs
as absolute; change it (and shouldUnlinkRepository) to normalise repo.URL
relative to the composer root directory before comparing or inspecting: add a
dir parameter to isPackageLinked and shouldUnlinkRepository (and update callers
LinkPackages and UnlinkPackages to pass the root dir), then for each repo if
repo.URL is not an absolute path, join it with dir and run filepath.Clean/Abs to
produce a canonical absolute path and use that for comparisons (same
normalisation logic should be applied in the other helper referenced around the
230-253 region).
In `@pkg/php/ssl_test.go`:
- Around line 110-113: The test case for CertsExist uses a hard-coded absolute
path; change it to derive a guaranteed-missing path from the test TempDir by
calling t.TempDir() and joining a non-existent subdirectory (e.g.,
filepath.Join(t.TempDir(), "missing")) so the path is portable and
deterministic; update the t.Run block that calls CertsExist(testDomain,
SSLOptions{Dir: ...}) to use that joined path and ensure filepath is imported if
not already.
- Around line 117-122: The subtest "returns error when mkcert not installed" is
non-deterministic because it conditionally skips using IsMkcertInstalled(); to
fix it, make the test force the error path deterministically by mocking or
overriding IsMkcertInstalled() (or the underlying check it calls) within the
test to return false, then assert the expected failure; locate the test in
ssl_test.go and change the conditional skip to stub IsMkcertInstalled (or inject
a test-only hook) so the code path under test always executes and CI validates
the mkcert error handling.
---
Nitpick comments:
In `@pkg/php/services.go`:
- Around line 355-364: HorizonService.Stop currently runs the pre-stop artisan
command and records any error to s.lastError but always returns only the result
of stopProcess(), losing the pre-stop error; modify HorizonService.Stop so after
calling s.stopProcess() it checks if s.lastError is non-nil and if stopProcess
returned nil then return s.lastError (or combine both errors if stopProcess also
failed), i.e. update the Stop method (references: HorizonService.Stop,
s.lastError, s.stopProcess()) to propagate the pre-stop command error to the
caller.
- Around line 451-460: RedisService.Stop currently swallows the error from the
redis-cli shutdown command by only assigning s.lastError and then returning
s.stopProcess() — change it to propagate the shutdown error to the caller: run
cmd.Run(), store the error in s.lastError if non-nil, then call s.stopProcess();
if the shutdown error is non-nil return it (or wrap it together with any error
from s.stopProcess()), otherwise return the s.stopProcess() error; update
RedisService.Stop to reference cmd.Run(), s.lastError, and s.stopProcess()
accordingly so callers receive the pre-stop error instead of it being discarded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 589ba4ca-9136-42d7-81a5-f896f6897d26
📒 Files selected for processing (40)
go.modinternal/clishim/pkg/cli/cli_test.gointernal/i18nshim/i18n_test.gopkg/php/ax7_compliance_test.gopkg/php/bridge.gopkg/php/cmd.gopkg/php/cmd_build.gopkg/php/cmd_ci.gopkg/php/cmd_deploy.gopkg/php/cmd_dev.gopkg/php/cmd_packages.gopkg/php/cmd_serve_frankenphp.gopkg/php/cmd_serve_frankenphp_stub.gopkg/php/container.gopkg/php/container_test.gopkg/php/coolify.gopkg/php/coolify_test.gopkg/php/deploy.gopkg/php/deploy_internal_test.gopkg/php/deploy_test.gopkg/php/detect.gopkg/php/detect_test.gopkg/php/dockerfile.gopkg/php/dockerfile_test.gopkg/php/env.gopkg/php/extract.gopkg/php/handler_stub.gopkg/php/packages.gopkg/php/packages_test.gopkg/php/php.gopkg/php/php_test.gopkg/php/quality.gopkg/php/services.gopkg/php/services_extended_test.gopkg/php/services_test.gopkg/php/sonar_constants.gopkg/php/sonar_test_constants_test.gopkg/php/ssl_extended_test.gopkg/php/ssl_test.gopkg/php/testing.go
✅ Files skipped from review due to trivial changes (4)
- pkg/php/bridge.go
- pkg/php/sonar_constants.go
- pkg/php/testing.go
- pkg/php/sonar_test_constants_test.go
🚧 Files skipped from review as they are similar to previous changes (12)
- pkg/php/env.go
- pkg/php/handler_stub.go
- pkg/php/coolify.go
- pkg/php/deploy_internal_test.go
- pkg/php/ssl_extended_test.go
- pkg/php/services_extended_test.go
- pkg/php/dockerfile_test.go
- pkg/php/cmd_serve_frankenphp_stub.go
- pkg/php/packages_test.go
- pkg/php/php_test.go
- pkg/php/detect_test.go
- pkg/php/container_test.go
Comment on lines
+11
to
+27
| func captureStdout(t *testing.T, fn func()) string { | ||
| t.Helper() | ||
| old := os.Stdout | ||
| read, write, err := os.Pipe() | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| os.Stdout = write | ||
| fn() | ||
| write.Close() | ||
| os.Stdout = old | ||
| data, err := io.ReadAll(read) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| return string(data) | ||
| } |
There was a problem hiding this comment.
Harden stream-capture helpers with guaranteed cleanup
Lines [18-22] and [36-40] restore global streams only on the non-failing path. If fn() exits abnormally, later tests can inherit redirected stdio.
Suggested fix
func captureStdout(t *testing.T, fn func()) string {
t.Helper()
old := os.Stdout
read, write, err := os.Pipe()
if err != nil {
t.Fatal(err)
}
os.Stdout = write
+ t.Cleanup(func() {
+ os.Stdout = old
+ _ = write.Close()
+ _ = read.Close()
+ })
fn()
- write.Close()
- os.Stdout = old
+ _ = write.Close()
data, err := io.ReadAll(read)
if err != nil {
t.Fatal(err)
}
return string(data)
}
func captureStderr(t *testing.T, fn func()) string {
t.Helper()
old := os.Stderr
read, write, err := os.Pipe()
if err != nil {
t.Fatal(err)
}
os.Stderr = write
+ t.Cleanup(func() {
+ os.Stderr = old
+ _ = write.Close()
+ _ = read.Close()
+ })
fn()
- write.Close()
- os.Stderr = old
+ _ = write.Close()
data, err := io.ReadAll(read)
if err != nil {
t.Fatal(err)
}
return string(data)
}Also applies to: 29-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/clishim/pkg/cli/cli_test.go` around lines 11 - 27, captureStdout
(and the sibling helper around lines 29-45, likely captureStderr) restore global
streams only on the non-failing path; make restoration and pipe cleanup
unconditional by using defer: after creating the pipe and assigning os.Stdout
(or os.Stderr), immediately defer restoring the original stream (os.Stdout =
old) and deferring write.Close() and read.Close() as appropriate, then run fn(),
call write.Close() before reading to ensure EOF, and read from the pipe; keep
t.Fatal on pipe/create/read errors. Update the functions captureStdout and the
other stream-capture helper to use these defers so streams are always restored
even if fn() fails.
Comment on lines
+11
to
+13
| func TestI18N_RegisterLocales_Good(t *testing.T) { | ||
| RegisterLocales(fstest.MapFS{testEnglishLocaleFile: {Data: []byte(`{"common":{"label":{"done":"Done"}}}`)}}, "locales") | ||
| got := Label("done") |
There was a problem hiding this comment.
Reset global i18n state between tests
Line [12] (and other RegisterLocales calls) mutates shared package state. Without clearing translations, tests can become order-dependent as the suite grows.
Suggested fix
+func resetTranslationsForTest() {
+ mu.Lock()
+ clear(translations)
+ mu.Unlock()
+}
+
func TestI18N_RegisterLocales_Good(t *testing.T) {
+ resetTranslationsForTest()
RegisterLocales(fstest.MapFS{testEnglishLocaleFile: {Data: []byte(`{"common":{"label":{"done":"Done"}}}`)}}, "locales")
got := Label("done")Apply resetTranslationsForTest() at the start of each test that relies on locale map contents.
Also applies to: 19-21, 27-29, 35-37, 58-60, 73-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/i18nshim/i18n_test.go` around lines 11 - 13, Tests mutate the
package-level translations via RegisterLocales which makes them order-dependent;
call resetTranslationsForTest() at the start of every test that relies on locale
contents (e.g., TestI18N_RegisterLocales_Good and other tests using
RegisterLocales/Label) to clear the global translations map before registering
locales so each test runs with a fresh i18n state.
Comment on lines
+122
to
+126
| func ax7LongRunningCommand(t *T, name string) { | ||
| t.Helper() | ||
| bin := ax7BinPath(t) | ||
| ax7Executable(t, bin, name, ax7ExitOKScript) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Make ax7LongRunningCommand genuinely long-running.
This helper currently writes ax7ExitOKScript, which is just exit 0, so the service lifecycle tests below are not exercising running-process or shutdown behaviour at all. A blocking stub with a TERM/INT trap will make those assertions meaningful.
Suggested fix
func ax7LongRunningCommand(t *T, name string) {
t.Helper()
bin := ax7BinPath(t)
- ax7Executable(t, bin, name, ax7ExitOKScript)
+ ax7Executable(t, bin, name, `trap 'exit 0' TERM INT
+while :; do
+ sleep 3600
+done
+`)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func ax7LongRunningCommand(t *T, name string) { | |
| t.Helper() | |
| bin := ax7BinPath(t) | |
| ax7Executable(t, bin, name, ax7ExitOKScript) | |
| } | |
| func ax7LongRunningCommand(t *T, name string) { | |
| t.Helper() | |
| bin := ax7BinPath(t) | |
| ax7Executable(t, bin, name, `trap 'exit 0' TERM INT | |
| while :; do | |
| sleep 3600 | |
| done | |
| `) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/php/ax7_compliance_test.go` around lines 122 - 126, ax7LongRunningCommand
currently writes ax7ExitOKScript (which just exits) so tests never exercise
shutdown; change it to install a blocking script that traps SIGTERM/SIGINT and
waits (e.g., trap 'exit 0' TERM INT; while :; do sleep 1; done) instead of
ax7ExitOKScript. Update ax7LongRunningCommand to call ax7Executable with this
new blocking script content (or a new constant like ax7BlockingScript) so the
process stays alive until signalled; keep using ax7BinPath and ax7Executable and
ensure the script is marked executable and cleaned up as before.
Comment on lines
+252
to
+259
| cwd, err := os.Getwd() | ||
| if err == nil { | ||
| if appName := GetLaravelAppName(cwd); appName != "" { | ||
| return strings.ToLower(strings.ReplaceAll(appName, " ", "-")), nil | ||
| } | ||
| } | ||
|
|
||
| return "", errors.New(i18n.T("cmd.php.serve.name_required")) |
There was a problem hiding this comment.
Do not swallow Getwd failures here.
If Line 252 fails, this helper currently degrades to cmd.php.serve.name_required instead of returning the real working-directory error. That makes an environment problem look like a missing flag.
Suggested fix
cwd, err := os.Getwd()
- if err == nil {
- if appName := GetLaravelAppName(cwd); appName != "" {
- return strings.ToLower(strings.ReplaceAll(appName, " ", "-")), nil
- }
+ if err != nil {
+ return "", cli.Err(cliWrapErrorFormat, i18n.T(i18nFailGetKey, workingDirectorySubject), err)
+ }
+ if appName := GetLaravelAppName(cwd); appName != "" {
+ return strings.ToLower(strings.ReplaceAll(appName, " ", "-")), nil
}
return "", errors.New(i18n.T("cmd.php.serve.name_required"))
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/php/cmd_build.go` around lines 252 - 259, The current logic swallows
failures from os.Getwd and returns the generic
i18n.T("cmd.php.serve.name_required") error; change the flow in the function
containing the os.Getwd call so that if os.Getwd returns a non-nil error you
immediately return "", err instead of continuing, otherwise proceed to call
GetLaravelAppName(cwd) and keep the existing behavior; update the branch around
os.Getwd/GetLaravelAppName to ensure real working-directory errors surface
rather than being translated to the name_required message.
Comment on lines
+170
to
+179
| func devAppURL(cwd string, opts phpDevOptions, port int) string { | ||
| appURL := GetLaravelAppURL(cwd) | ||
| if appURL != "" { | ||
| return appURL | ||
| } | ||
| if opts.HTTPS { | ||
| return cli.Sprintf("https://localhost:%d", port) | ||
| } | ||
| return cli.Sprintf("http://localhost:%d", port) | ||
| } |
There was a problem hiding this comment.
Respect CLI overrides when printing the app URL.
Line 172 returns APP_URL before checking any runtime flags, and Lines 175-178 never use opts.Domain at all. That means php dev --domain …, --port …, or --https can start the server on one address whilst the ready banner points at another.
Suggested fix
func devAppURL(cwd string, opts phpDevOptions, port int) string {
- appURL := GetLaravelAppURL(cwd)
- if appURL != "" {
- return appURL
- }
- if opts.HTTPS {
- return cli.Sprintf("https://localhost:%d", port)
- }
- return cli.Sprintf("http://localhost:%d", port)
+ scheme := "http"
+ if opts.HTTPS {
+ scheme = "https"
+ }
+
+ if opts.Domain != "" {
+ return cli.Sprintf("%s://%s:%d", scheme, opts.Domain, port)
+ }
+ if opts.Port != 0 || opts.HTTPS {
+ return cli.Sprintf("%s://localhost:%d", scheme, port)
+ }
+
+ appURL := GetLaravelAppURL(cwd)
+ if appURL != "" {
+ return appURL
+ }
+
+ return cli.Sprintf("%s://localhost:%d", scheme, port)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func devAppURL(cwd string, opts phpDevOptions, port int) string { | |
| appURL := GetLaravelAppURL(cwd) | |
| if appURL != "" { | |
| return appURL | |
| } | |
| if opts.HTTPS { | |
| return cli.Sprintf("https://localhost:%d", port) | |
| } | |
| return cli.Sprintf("http://localhost:%d", port) | |
| } | |
| func devAppURL(cwd string, opts phpDevOptions, port int) string { | |
| scheme := "http" | |
| if opts.HTTPS { | |
| scheme = "https" | |
| } | |
| if opts.Domain != "" { | |
| return cli.Sprintf("%s://%s:%d", scheme, opts.Domain, port) | |
| } | |
| if opts.Port != 0 || opts.HTTPS { | |
| return cli.Sprintf("%s://localhost:%d", scheme, port) | |
| } | |
| appURL := GetLaravelAppURL(cwd) | |
| if appURL != "" { | |
| return appURL | |
| } | |
| return cli.Sprintf("%s://localhost:%d", scheme, port) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/php/cmd_dev.go` around lines 170 - 179, devAppURL currently returns
GetLaravelAppURL(cwd) before honoring runtime flags; change the order so
CLI/runtime options take precedence: if opts.Domain is non-empty build and
return a URL using opts.Domain, opts.HTTPS and the provided port (use
cli.Sprintf with "http(s)://%s" and append :%d when port is non-standard or when
provided), else if GetLaravelAppURL(cwd) != "" return that, otherwise fall back
to "http(s)://localhost:%d" using opts.HTTPS and port. Update function devAppURL
and reference phpDevOptions, opts.Domain, opts.HTTPS, GetLaravelAppURL, and port
when implementing the fix.
Comment on lines
+158
to
+165
| m := getMedium() | ||
| tempDockerfile := filepath.Join(opts.ProjectDir, "Dockerfile.core-generated") | ||
| if err := m.Write(tempDockerfile, content); err != nil { | ||
| return "", nil, cli.WrapVerb(err, "write", "Dockerfile") | ||
| } | ||
|
|
||
| // Build Docker image | ||
| return tempDockerfile, func() { _ = m.Delete(tempDockerfile) }, nil | ||
| } |
There was a problem hiding this comment.
Use a unique temp file instead of a fixed Dockerfile name.
This helper always writes to Dockerfile.core-generated and always deletes it afterwards. If that file already exists in the project, this build path will overwrite it and then remove it.
Suggested fix
- m := getMedium()
- tempDockerfile := filepath.Join(opts.ProjectDir, "Dockerfile.core-generated")
- if err := m.Write(tempDockerfile, content); err != nil {
+ tempFile, err := os.CreateTemp(opts.ProjectDir, "Dockerfile.core-generated-*.Dockerfile")
+ if err != nil {
+ return "", nil, cli.WrapVerb(err, "create", "temporary Dockerfile")
+ }
+ tempDockerfile := tempFile.Name()
+ if _, err := tempFile.WriteString(content); err != nil {
+ _ = tempFile.Close()
+ _ = os.Remove(tempDockerfile)
+ return "", nil, cli.WrapVerb(err, "write", "Dockerfile")
+ }
+ if err := tempFile.Close(); err != nil {
+ _ = os.Remove(tempDockerfile)
+ return "", nil, cli.WrapVerb(err, "close", "temporary Dockerfile")
+ }
- return "", nil, cli.WrapVerb(err, "write", "Dockerfile")
- }
-
- return tempDockerfile, func() { _ = m.Delete(tempDockerfile) }, nil
+
+ return tempDockerfile, func() { _ = os.Remove(tempDockerfile) }, 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.
Suggested change
| m := getMedium() | |
| tempDockerfile := filepath.Join(opts.ProjectDir, "Dockerfile.core-generated") | |
| if err := m.Write(tempDockerfile, content); err != nil { | |
| return "", nil, cli.WrapVerb(err, "write", "Dockerfile") | |
| } | |
| // Build Docker image | |
| return tempDockerfile, func() { _ = m.Delete(tempDockerfile) }, nil | |
| } | |
| tempFile, err := os.CreateTemp(opts.ProjectDir, "Dockerfile.core-generated-*.Dockerfile") | |
| if err != nil { | |
| return "", nil, cli.WrapVerb(err, "create", "temporary Dockerfile") | |
| } | |
| tempDockerfile := tempFile.Name() | |
| if _, err := tempFile.WriteString(content); err != nil { | |
| _ = tempFile.Close() | |
| _ = os.Remove(tempDockerfile) | |
| return "", nil, cli.WrapVerb(err, "write", "Dockerfile") | |
| } | |
| if err := tempFile.Close(); err != nil { | |
| _ = os.Remove(tempDockerfile) | |
| return "", nil, cli.WrapVerb(err, "close", "temporary Dockerfile") | |
| } | |
| return tempDockerfile, func() { _ = os.Remove(tempDockerfile) }, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/php/container.go` around lines 158 - 165, The helper currently writes to
a fixed filename "Dockerfile.core-generated" which can overwrite an existing
project file; change it to create a unique temp file in opts.ProjectDir (use
os.CreateTemp / ioutil.TempFile with pattern like
"Dockerfile.core-generated-*"), use the returned unique temp path for
m.Write(tempDockerfile, content), ensure the temp file is closed if created, and
keep the cleanup func calling m.Delete(tempDockerfile) to remove that specific
temp path; update the code around getMedium(), m.Write and m.Delete to use the
unique temp file name.
Comment on lines
+79
to
92
| func assertCoolifyConfig(t *T, config *CoolifyConfig, wantURL, wantToken, wantAppID, wantStaging string) { | ||
| t.Helper() | ||
| if config.URL != wantURL { | ||
| t.Errorf("URL = %q, want %q", config.URL, wantURL) | ||
| } | ||
| if config.Token != wantToken { | ||
| t.Errorf("Token = %q, want %q", config.Token, wantToken) | ||
| } | ||
| if config.AppID != wantAppID { | ||
| t.Errorf("AppID = %q, want %q", config.AppID, wantAppID) | ||
| } | ||
| if wantStaging != "" && config.StagingAppID != wantStaging { | ||
| t.Errorf("StagingAppID = %q, want %q", config.StagingAppID, wantStaging) | ||
| } |
There was a problem hiding this comment.
Always assert StagingAppID in this helper.
When wantStaging is empty, the helper skips the check entirely, so the "quoted values" and "with comments and blank lines" cases will still pass if LoadCoolifyConfig populates an unexpected staging ID. Comparing the field unconditionally keeps the empty-string expectation covered as well.
Suggested fix
func assertCoolifyConfig(t *T, config *CoolifyConfig, wantURL, wantToken, wantAppID, wantStaging string) {
t.Helper()
if config.URL != wantURL {
t.Errorf("URL = %q, want %q", config.URL, wantURL)
}
if config.Token != wantToken {
t.Errorf("Token = %q, want %q", config.Token, wantToken)
}
if config.AppID != wantAppID {
t.Errorf("AppID = %q, want %q", config.AppID, wantAppID)
}
- if wantStaging != "" && config.StagingAppID != wantStaging {
+ if config.StagingAppID != wantStaging {
t.Errorf("StagingAppID = %q, want %q", config.StagingAppID, wantStaging)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func assertCoolifyConfig(t *T, config *CoolifyConfig, wantURL, wantToken, wantAppID, wantStaging string) { | |
| t.Helper() | |
| if config.URL != wantURL { | |
| t.Errorf("URL = %q, want %q", config.URL, wantURL) | |
| } | |
| if config.Token != wantToken { | |
| t.Errorf("Token = %q, want %q", config.Token, wantToken) | |
| } | |
| if config.AppID != wantAppID { | |
| t.Errorf("AppID = %q, want %q", config.AppID, wantAppID) | |
| } | |
| if wantStaging != "" && config.StagingAppID != wantStaging { | |
| t.Errorf("StagingAppID = %q, want %q", config.StagingAppID, wantStaging) | |
| } | |
| func assertCoolifyConfig(t *T, config *CoolifyConfig, wantURL, wantToken, wantAppID, wantStaging string) { | |
| t.Helper() | |
| if config.URL != wantURL { | |
| t.Errorf("URL = %q, want %q", config.URL, wantURL) | |
| } | |
| if config.Token != wantToken { | |
| t.Errorf("Token = %q, want %q", config.Token, wantToken) | |
| } | |
| if config.AppID != wantAppID { | |
| t.Errorf("AppID = %q, want %q", config.AppID, wantAppID) | |
| } | |
| if config.StagingAppID != wantStaging { | |
| t.Errorf("StagingAppID = %q, want %q", config.StagingAppID, wantStaging) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/php/deploy_test.go` around lines 79 - 92, The test helper
assertCoolifyConfig currently skips asserting StagingAppID when wantStaging is
empty; update assertCoolifyConfig to always compare config.StagingAppID to
wantStaging (remove the conditional check) so empty-string expectations are
verified as well, ensuring cases like the "quoted values" and "with comments and
blank lines" fail if an unexpected staging ID is present.
Comment on lines
+174
to
+180
| func isPackageLinked(repos []composerRepository, absPath string) bool { | ||
| for _, repo := range repos { | ||
| if repo.Type == "path" && repo.URL == absPath { | ||
| return true | ||
| } | ||
| } | ||
| return false |
There was a problem hiding this comment.
Normalise path repositories before comparing or inspecting them.
These helpers treat repo.URL as if it were always absolute. In practice Composer path repositories are often relative to the root composer.json, so isPackageLinked can add duplicates and shouldUnlinkRepository can fail to resolve package metadata for unlink-by-name.
Suggested fix
-func isPackageLinked(repos []composerRepository, absPath string) bool {
+func isPackageLinked(projectDir string, repos []composerRepository, absPath string) bool {
for _, repo := range repos {
- if repo.Type == "path" && repo.URL == absPath {
+ repoPath := repo.URL
+ if repo.Type != "path" {
+ continue
+ }
+ if !filepath.IsAbs(repoPath) {
+ repoPath = filepath.Join(projectDir, repoPath)
+ }
+ if repoPath == absPath {
return true
}
}
return false
}-func shouldUnlinkRepository(repo composerRepository, toUnlink map[string]bool) bool {
+func shouldUnlinkRepository(projectDir string, repo composerRepository, toUnlink map[string]bool) bool {
if repo.Type != "path" {
return false
}
+ repoPath := repo.URL
+ if !filepath.IsAbs(repoPath) {
+ repoPath = filepath.Join(projectDir, repoPath)
+ }
+
shouldUnlink := false
- if IsPHPProject(repo.URL) {
- pkgName, _, err := getPackageInfo(repo.URL)
+ if IsPHPProject(repoPath) {
+ pkgName, _, err := getPackageInfo(repoPath)
if err == nil && toUnlink[pkgName] {
shouldUnlink = true
cli.Print("Unlinked: %s\n", pkgName)
}
}You'd also need to pass dir into both helper calls from LinkPackages and UnlinkPackages.
Also applies to: 230-253
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/php/packages.go` around lines 174 - 180, isPackageLinked currently treats
composer repository URLs as absolute; change it (and shouldUnlinkRepository) to
normalise repo.URL relative to the composer root directory before comparing or
inspecting: add a dir parameter to isPackageLinked and shouldUnlinkRepository
(and update callers LinkPackages and UnlinkPackages to pass the root dir), then
for each repo if repo.URL is not an absolute path, join it with dir and run
filepath.Clean/Abs to produce a canonical absolute path and use that for
comparisons (same normalisation logic should be applied in the other helper
referenced around the 230-253 region).
Comment on lines
+110
to
113
| t.Run("returns false for invalid directory", func(t *T) { | ||
| // Use invalid directory path | ||
| assert.False(t, CertsExist("domain.test", SSLOptions{Dir: "/nonexistent/path/that/does/not/exist"})) | ||
| AssertFalse(t, CertsExist(testDomain, SSLOptions{Dir: "/nonexistent/path/that/does/not/exist"})) | ||
| }) |
There was a problem hiding this comment.
Prefer a guaranteed-missing path derived from TempDir
Using a fixed absolute path can be brittle across environments. Build a non-existent path from t.TempDir() to keep this test portable and deterministic.
Proposed change
t.Run("returns false for invalid directory", func(t *T) {
- // Use invalid directory path
- AssertFalse(t, CertsExist(testDomain, SSLOptions{Dir: "/nonexistent/path/that/does/not/exist"}))
+ missingDir := filepath.Join(t.TempDir(), "missing", "ssl")
+ AssertFalse(t, CertsExist(testDomain, SSLOptions{Dir: missingDir}))
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| t.Run("returns false for invalid directory", func(t *T) { | |
| // Use invalid directory path | |
| assert.False(t, CertsExist("domain.test", SSLOptions{Dir: "/nonexistent/path/that/does/not/exist"})) | |
| AssertFalse(t, CertsExist(testDomain, SSLOptions{Dir: "/nonexistent/path/that/does/not/exist"})) | |
| }) | |
| t.Run("returns false for invalid directory", func(t *T) { | |
| missingDir := filepath.Join(t.TempDir(), "missing", "ssl") | |
| AssertFalse(t, CertsExist(testDomain, SSLOptions{Dir: missingDir})) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/php/ssl_test.go` around lines 110 - 113, The test case for CertsExist
uses a hard-coded absolute path; change it to derive a guaranteed-missing path
from the test TempDir by calling t.TempDir() and joining a non-existent
subdirectory (e.g., filepath.Join(t.TempDir(), "missing")) so the path is
portable and deterministic; update the t.Run block that calls
CertsExist(testDomain, SSLOptions{Dir: ...}) to use that joined path and ensure
filepath is imported if not already.
Comment on lines
+117
to
122
| t.Run("returns error when mkcert not installed", func(t *T) { | ||
| // This test assumes mkcert might not be installed | ||
| // If it is installed, we skip this test | ||
| if IsMkcertInstalled() { | ||
| t.Skip("mkcert is installed, skipping error test") | ||
| t.Skip(testMkcertInstalledSkip) | ||
| } |
There was a problem hiding this comment.
Make the mkcert error-path test deterministic
This subtest currently skips based on host state (Line 120), so CI/dev machines with mkcert installed won’t validate the failure path at all.
Proposed change
func TestPHP_SetupSSL_Bad(t *T) {
t.Run("returns error when mkcert not installed", func(t *T) {
- // This test assumes mkcert might not be installed
- // If it is installed, we skip this test
- if IsMkcertInstalled() {
- t.Skip(testMkcertInstalledSkip)
- }
+ // Force a deterministic "mkcert missing" environment.
+ t.Setenv("PATH", "")
err := SetupSSL("example.test", SSLOptions{})
AssertError(t, err)
AssertContains(t, err.Error(), testMkcertNotInstalled)
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| t.Run("returns error when mkcert not installed", func(t *T) { | |
| // This test assumes mkcert might not be installed | |
| // If it is installed, we skip this test | |
| if IsMkcertInstalled() { | |
| t.Skip("mkcert is installed, skipping error test") | |
| t.Skip(testMkcertInstalledSkip) | |
| } | |
| func TestPHP_SetupSSL_Bad(t *T) { | |
| t.Run("returns error when mkcert not installed", func(t *T) { | |
| // Force a deterministic "mkcert missing" environment. | |
| t.Setenv("PATH", "") | |
| err := SetupSSL("example.test", SSLOptions{}) | |
| AssertError(t, err) | |
| AssertContains(t, err.Error(), testMkcertNotInstalled) | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/php/ssl_test.go` around lines 117 - 122, The subtest "returns error when
mkcert not installed" is non-deterministic because it conditionally skips using
IsMkcertInstalled(); to fix it, make the test force the error path
deterministically by mocking or overriding IsMkcertInstalled() (or the
underlying check it calls) within the test to return false, then assert the
expected failure; locate the test in ssl_test.go and change the conditional skip
to stub IsMkcertInstalled (or inject a test-only hook) so the code path under
test always executes and CI validates the mkcert error handling.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Brings this repo to verdict: COMPLIANT against the v0.9.0 audit.
🤖 Generated with Claude Code + Codex
Co-Authored-By: Codex noreply@openai.com
Co-Authored-By: Virgil virgil@lethean.io
Summary by CodeRabbit
Bug Fixes
Chores
Tests
New Features