fix: batch fixes for #60, #38, #59, #67, #68

[StuMason]

Summary

Five low-effort, high-impact fixes from open issues:

• Suppress /up health check from nginx access logs #68 — Suppress /up health check from nginx access logs (access_log off; log_not_found off)
• Scheduler supervisor uses schedule:run loop instead of schedule:work #60 — Scheduler supervisor now uses schedule:work instead of a while/sleep/schedule:run loop
• ssh-keygen command fails with 'Too many arguments' #38 — Fix ssh-keygen "Too many arguments" by using escapeshellarg() for all parameters
• Bug: Deployment sets specific commit SHA instead of HEAD #59 — Clear git_commit_sha after deploying a specific commit so future deploys use HEAD
• Add stderr + JSON logging to install/onboarding #67 — Container logging: generate docker/php-fpm.conf with catch_workers_output = yes, use errorlog channel (not stderr — which doesn't work with PHP-FPM), add logging guidance to coolify:install

Also closed #51 and #26 as out of scope.

Closes #60, closes #38, closes #59, closes #67, closes #68

Test plan

• All 268 existing tests pass
• PHPStan clean
• Verify coolify:install shows logging recommendation
• Verify generated Dockerfile includes php-fpm.conf COPY line
• Verify coolify:provision sets LOG_STACK=daily,errorlog

[claude]

PR Review: fix: batch fixes for #60, #38, #59, #67, #68

Good batch of targeted fixes. The changes are generally well-reasoned and the PR description clearly explains the motivation for each. A few concerns worth addressing before merge.

---

Bug: Missing try/finally in deploy() — git_commit_sha can get stuck

File: src/Repositories/CoolifyApplicationRepository.php

If $this->client->get('deploy', ...) throws an exception (network error, 5xx, timeout), the cleanup call that clears git_commit_sha never runs. The application ends up permanently pinned to that specific commit until someone manually clears it.

The current code skips cleanup on exception. Wrapping in try/finally guarantees cleanup regardless of the deploy outcome:

$response = null;
try {
    $response = $this->client->get('deploy', $params);
} finally {
    if ($commit !== null) {
        $this->update($uuid, ['git_commit_sha' => '']);
    }
}

---

Missing test coverage

Several new code paths have no tests:

1. generatePhpFpmConf() has no unit test. The existing "generates php.ini with correct settings" test is a straightforward template:

it('generates php-fpm.conf with catch_workers_output enabled', function () {
    $generator = new DockerGenerator;
    $content = $generator->generatePhpFpmConf();

    expect($content)->toContain('[www]')
        ->and($content)->toContain('catch_workers_output = yes')
        ->and($content)->toContain('decorate_workers_output = no');
});

2. "writes all Docker files to disk" test is now stale — it does not assert docker/php-fpm.conf exists, so the new file could be silently removed in a future refactor without any test failing.

3. "deploys a specific commit" test does not verify the cleanup. ApplicationRepositoryTest.php:85 verifies the PATCH to set the commit SHA, but not the second PATCH to clear it afterward. The regression from #59 could reappear undetected.

4. No test for schedule:work in the supervisor config. Worth a simple assertion that the generated supervisord.conf contains schedule:work and not the old shell loop.

---

Nginx /up suppression is hardcoded

File: src/Docker/DockerGenerator.php

The PR hardcodes /up for access log suppression. The codebase already has a coolify.docker.health_check_path config key used for the Docker HEALTHCHECK command. If a user has set a custom health check path (e.g. /health), their endpoint still gets logged. Using the configured value would keep things consistent:

$healthPath = config('coolify.docker.health_check_path', '/up');
// then interpolate $healthPath into the nginx location block

---

Minor: style tags in bulletList may render literally

File: src/Console/InstallCommand.php

The bulletList call wraps the value in comment style tags. Symfony Console's bulletList helper does not process style tags the same way as $this->line(). In non-TTY environments (CI pipelines, log capture) this may render the tags literally. Worth a quick manual smoke-test.

---

Positive notes

• escapeshellarg() fix (ssh-keygen command fails with 'Too many arguments' #38) is exactly right — a straightforward security improvement with no downsides.
• schedule:work (Scheduler supervisor uses schedule:run loop instead of schedule:work #60) is the correct Laravel-idiomatic approach; the old while/sleep loop had poor signal handling and no way for supervisord to cleanly manage the child process.
• PHP-FPM catch_workers_output (Add stderr + JSON logging to install/onboarding #67) — the decorate_workers_output = no pairing is the right call to suppress timestamp/pool-name noise. Using the errorlog channel rather than stderr is a subtle but important distinction, and it is well documented in the comment.
• The zz-laravel.conf filename ensures the config sorts last in pool.d and overrides defaults cleanly.

[StuMason]

@claude rereview please

[claude]

Claude finished @StuMason's task in 2m 8s —— View job

---

Re-review: PR #70

• Fetch latest changes and review current state
• Check if previous review concerns have been addressed
• Provide updated review feedback

---

Review Summary: ✅ All Previous Concerns Addressed

I've reviewed the latest commit (f42f395 fix: address PR review feedback) and all the concerns from my previous review have been properly addressed.

Previous Issues — Now Fixed

Issue	Status	Implementation
Missing try/finally in deploy()	✅ Fixed	CoolifyApplicationRepository.php:115-122 — deploy wrapped in try/finally, cleanup runs regardless of exceptions
No test for generatePhpFpmConf()	✅ Fixed	DockerGeneratorTest.php:233-240 — tests [www], catch_workers_output, decorate_workers_output
"writes all Docker files" test incomplete	✅ Fixed	DockerGeneratorTest.php:251,258 — asserts php-fpm.conf exists and is in files array
Commit SHA cleanup not tested	✅ Fixed	ApplicationRepositoryTest.php:102-114 — verifies 2 PATCH requests, first sets SHA, second clears it
No test for schedule:work	✅ Fixed	DockerGeneratorTest.php:192-206 — asserts schedule:work present and schedule:run absent
Nginx /up suppression hardcoded	✅ Fixed	DockerGenerator.php:446,512 — uses config('coolify.docker.health_check_path')

Test Results

Tests:    270 passed (537 assertions)
Duration: 7.77s

PHPStan:  [OK] No errors

Code Quality Notes

The implementation is clean and follows good practices:

• try/finally pattern ensures cleanup even on network failures — important for production reliability
• Test coverage now properly validates all new behaviors
• Config-driven health check path keeps nginx suppression consistent with Docker HEALTHCHECK
• schedule:work is the correct Laravel-idiomatic approach (vs while/sleep loop)

---

Verdict: This PR is ready to merge. All five issue fixes are well-implemented with appropriate test coverage.
|