Skip to content

Report silent build command failures#238

Open
ChiragAgg5k wants to merge 1 commit into
mainfrom
fix-silent-build-command-failure
Open

Report silent build command failures#238
ChiragAgg5k wants to merge 1 commit into
mainfrom
fix-silent-build-command-failure

Conversation

@ChiragAgg5k

@ChiragAgg5k ChiragAgg5k commented Jun 30, 2026

Copy link
Copy Markdown
Member

What does this PR do?

Makes build command failures explicit in executor responses by introducing a dedicated build_failed error type.

Previously, a build command that exited non-zero could be surfaced as a generic runtime failure. With the Docker API orchestration adapter, non-zero command exits are raised as orchestration exceptions, so executor never reached its existing $status === false handling. That caused user build failures to lose their intended classification.

This PR keeps the fix local to executor:

  • Adds Exception::BUILD_FAILED = 'build_failed'.
  • Adds build_failed to the error config with HTTP 400 and publish => false.
  • Maps orchestration exceptions from the build-command execution step to BUILD_FAILED.
  • Keeps generic runtime/container errors mapped to runtime_failed.
  • Preserves existing ExecutorException types through the outer cleanup path instead of wrapping them as generic exceptions.
  • Keeps the build shell wrapper fallback for silent non-zero exits: Build command exited with code N.
  • Adds focused unit coverage for the build command wrapper fallback.
  • Adds e2e coverage asserting build command failures return type: build_failed.

Why

Downstream services need to distinguish user build failures from executor/runtime infrastructure failures. build_failed is safe to present as a user build error, while generic runtime failures can remain internal.

Test Plan

  • composer test:unit -- --filter DockerTest

Related PRs and Issues

@ChiragAgg5k ChiragAgg5k marked this pull request as ready for review June 30, 2026 05:51
@greptile-apps

greptile-apps Bot commented Jun 30, 2026

Copy link
Copy Markdown

Greptile Summary

This PR adds a dedicated build_failed error type so that build command failures are clearly distinguished from infrastructure/runtime failures in executor responses, enabling downstream services to handle user build errors separately.

  • Adds Exception::BUILD_FAILED constant and its errors.php config entry (HTTP 400, publish => false), then maps both the $status === false path and the new catch (OrchestrationException) path inside the build-command execution step to BUILD_FAILED.
  • Adds an inner catch (ExecutorException $err) { throw $err; } guard and an outer if ($throwable instanceof ExecutorException) guard so the BUILD_FAILED type is preserved through the cleanup/log-collection path instead of being re-wrapped as RUNTIME_FAILED.
  • Updates the v2 and non-v2 shell wrapper scripts to append Build command exited with code $status. on non-zero exit and propagate that exit code, with focused unit and e2e test coverage.

Confidence Score: 5/5

Safe to merge; the fix is tightly scoped to the build-command execution path and correctly preserves the new error type through both the inner and outer exception layers.

The previously flagged re-wrapping of BUILD_FAILED as RUNTIME_FAILED has been addressed with two complementary guards. The remaining observation about dropping the OrchestrationException message is a diagnostic nicety, not a correctness issue.

src/Executor/Runner/Docker.php — the catch (OrchestrationException) block hardcodes the message; worth reviewing whether the exception carries a more useful message from the Docker API.

Important Files Changed

Filename Overview
src/Executor/Runner/Docker.php Introduces inner catch (ExecutorException) re-throw and outer ExecutorException type-preservation guard to correctly propagate BUILD_FAILED. Also adds catch (OrchestrationException) to map Docker API non-zero exits to BUILD_FAILED, and appends exit-code info to the shell wrapper scripts. Logic is sound; minor: OrchestrationException message is dropped.
src/Executor/Exception.php Adds BUILD_FAILED = 'build_failed' constant; no logic changes.
app/config/errors.php Registers build_failed error type with HTTP 400 and publish => false; straightforward config addition.
tests/e2e/ExecutorTest.php Adds assertion that a failing build command returns type: build_failed in the response body.
tests/unit/Executor/Runner/DockerTest.php Adds unit test verifying that both v2 and v5 shell wrapper commands include the exit-code echo and exit $status statements.

Reviews (4): Last reviewed commit: "Report silent build command failures" | Re-trigger Greptile

Comment thread src/Executor/Runner/Docker.php Outdated
@ChiragAgg5k ChiragAgg5k force-pushed the fix-silent-build-command-failure branch from f2d527f to 73ca836 Compare June 30, 2026 05:56
@ChiragAgg5k ChiragAgg5k force-pushed the fix-silent-build-command-failure branch 4 times, most recently from 13fe422 to 7b214f8 Compare June 30, 2026 06:49
@ChiragAgg5k ChiragAgg5k force-pushed the fix-silent-build-command-failure branch from 7b214f8 to 3ea3e0c Compare June 30, 2026 06:53
Comment on lines +51 to +57
foreach (['v2', 'v5'] as $version) {
$commands = $this->invokeGetBuildCommands($docker, 'exit 1', $version);
$command = \implode(' ', $commands);

$this->assertStringContainsString('Build command exited with code $status.', $command);
$this->assertStringContainsString('exit $status', $command);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something feels weird here, $status doesnt exist.

Also, would be great to have test over HTTP too - trying to create build runtime with exit 1 command, and making sure logs include this message (and deployment is failed).
Maybe even assertion successfull build doesnt have this copy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants