Skip to content

fix(deployment): respect pinned commit SHA for deployments #9292

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lucaspluchon wants to merge 1 commit into
base: next
Choose a base branch
from
+83 −1
Open

fix(deployment): respect pinned commit SHA for deployments #9292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion app/Jobs/ApplicationDeploymentJob.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2189,7 +2189,17 @@ private function check_git_if_build_needed()
],
);
}
if ($this->saved_outputs->get('git_commit_sha') && ! $this->rollback) {
// Skip overwriting the commit when the application has a pinned SHA
$pinnedCommit = str(data_get($this->application, 'git_commit_sha'))->trim()->value();
$hasPinnedCommit = $this->pull_request_id === 0 && ! $this->rollback && $pinnedCommit !== '' && $pinnedCommit !== 'HEAD';

if ($hasPinnedCommit) {
$this->commit = $pinnedCommit;
if ($this->application_deployment_queue->commit !== $pinnedCommit) {
$this->application_deployment_queue->commit = $pinnedCommit;
$this->application_deployment_queue->save();
}
} elseif ($this->saved_outputs->get('git_commit_sha') && ! $this->rollback) {
Comment on lines +2192 to +2202
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 29, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Job-level pinned commit logic also overrides explicit queued commits.

Line 2194 currently applies pinned SHA without checking whether $this->commit was intentionally set upstream. This can still deploy the wrong revision even after queue-time fixes. Skynet behavior detected. 😄

💡 Suggested fix 
         $pinnedCommit = str(data_get($this->application, 'git_commit_sha'))->trim()->value();
-        $hasPinnedCommit = $this->pull_request_id === 0 && ! $this->rollback && $pinnedCommit !== '' && $pinnedCommit !== 'HEAD';
+        $hasPinnedCommit = $this->pull_request_id === 0
+            && ! $this->rollback
+            && ($this->commit === 'HEAD' || blank($this->commit))
+            && $pinnedCommit !== ''
+            && strcasecmp($pinnedCommit, 'HEAD') !== 0;
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@app/Jobs/ApplicationDeploymentJob.php` around lines 2192 - 2202, The
pinned-commit branch unconditionally overwrites any commit already set earlier;
change the logic so the pinned SHA is only applied when no explicit commit was
provided upstream. Concretely, update the hasPinnedCommit check or add a guard
before assigning $this->commit and updating
$this->application_deployment_queue->commit so it only sets them if
$this->commit is empty/null (or equals a default like ''/ 'HEAD'), leaving
existing explicit $this->commit untouched.

// Extract commit SHA from git ls-remote output, handling multi-line output (e.g., redirect warnings)
// Expected format: "commit_sha\trefs/heads/branch" possibly preceded by warning lines
// Note: Git warnings can be on the same line as the result (no newline)
Expand Down
9 changes: 9 additions & 0 deletions bootstrap/helpers/applications.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,15 @@ function queue_application_deployment(Application $application, string $deployme
$destination_id = $destination->id;
}

// Use the application's pinned commit SHA when no specific commit was requested
// and this is not a PR or rollback deployment
if ($pull_request_id === 0 && ! $rollback) {
$pinnedCommit = str($application->git_commit_sha ?? '')->trim()->value();
if ($pinnedCommit !== '' && $pinnedCommit !== 'HEAD') {
$commit = $pinnedCommit;
}
Comment on lines +34 to +38
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 29, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Pinned SHA override is terminating explicit commit inputs (wrong revision risk).

At Line 34, pinned SHA is applied unconditionally for non-PR/non-rollback flows, even when a real commit was explicitly passed (webhook/API/manual). That makes deployments ignore the requested commit and can queue the wrong target commit.

💡 Suggested fix 
-    if ($pull_request_id === 0 && ! $rollback) {
+    if ($pull_request_id === 0 && ! $rollback && ($commit === 'HEAD' || blank($commit))) {
         $pinnedCommit = str($application->git_commit_sha ?? '')->trim()->value();
-        if ($pinnedCommit !== '' && $pinnedCommit !== 'HEAD') {
+        if ($pinnedCommit !== '' && strcasecmp($pinnedCommit, 'HEAD') !== 0) {
             $commit = $pinnedCommit;
         }
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if ($pull_request_id === 0 && ! $rollback) {
$pinnedCommit = str($application->git_commit_sha ?? '')->trim()->value();
if ($pinnedCommit !== '' && $pinnedCommit !== 'HEAD') {
$commit = $pinnedCommit;
}
if ($pull_request_id === 0 && ! $rollback && ($commit === 'HEAD' || blank($commit))) {
$pinnedCommit = str($application->git_commit_sha ?? '')->trim()->value();
if ($pinnedCommit !== '' && strcasecmp($pinnedCommit, 'HEAD') !== 0) {
$commit = $pinnedCommit;
}
}
🧰 Tools
🪛 PHPMD (2.15.0)

[error] 34-34: The variable $pull_request_id is not named in camelCase. (undefined)

(CamelCaseVariableName)

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@bootstrap/helpers/applications.php` around lines 34 - 38, The current logic
unconditionally overrides $commit with $pinnedCommit when not a PR/rollback,
which discards any explicitly provided commit; change it so $pinnedCommit is
applied only when no explicit commit was supplied (e.g., $commit is empty or
null) — keep the checks around $pull_request_id and $rollback, and update the
block using the variables $pinnedCommit, $application->git_commit_sha and
$commit so that you assign $commit = $pinnedCommit only if $commit is not
already set or is an empty string.

}

// Check if the deployment queue is full for this server
$serverForQueueCheck = $server ?? Server::find($server_id);
$queue_limit = $serverForQueueCheck->settings->deployment_queue_limit ?? 25;
Expand Down
63 changes: 63 additions & 0 deletions tests/Unit/PinnedCommitDeploymentTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
<?php

use App\Models\Application;
use App\Models\ApplicationSetting;

describe('Pinned commit propagation', function () {
beforeEach(function () {
$this->application = new Application;
$this->application->forceFill([
'uuid' => 'test-app-uuid',
'git_commit_sha' => 'HEAD',
]);

$settings = new ApplicationSetting;
$settings->is_git_shallow_clone_enabled = false;
$settings->is_git_submodules_enabled = false;
$settings->is_git_lfs_enabled = false;
$this->application->setRelation('settings', $settings);
});

test('setGitImportSettings uses pinned commit when no explicit commit is passed', function () {
$pinnedSha = str_repeat('a', 40);
$this->application->git_commit_sha = $pinnedSha;

$result = $this->application->setGitImportSettings(
deployment_uuid: 'test-uuid',
git_clone_command: 'git clone',
public: true,
);

expect($result)->toContain($pinnedSha);
});

test('setGitImportSettings prefers explicit commit over pinned commit', function () {
$pinnedSha = str_repeat('a', 40);
$explicitSha = str_repeat('b', 40);
$this->application->git_commit_sha = $pinnedSha;

$result = $this->application->setGitImportSettings(
deployment_uuid: 'test-uuid',
git_clone_command: 'git clone',
public: true,
commit: $explicitSha,
);

expect($result)
->toContain($explicitSha)
->not->toContain($pinnedSha);
});

test('setGitImportSettings does not checkout when git_commit_sha is HEAD', function () {
$this->application->git_commit_sha = 'HEAD';

$result = $this->application->setGitImportSettings(
deployment_uuid: 'test-uuid',
git_clone_command: 'git clone',
public: true,
);

expect($result)->not->toContain('advice.detachedHead=false checkout');
});
Comment on lines +21 to +61
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 29, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Coverage gap: changed queue/job commit precedence paths are not asserted.

These tests validate Application::setGitImportSettings(), but the regression risk in this PR is in queue_application_deployment() and ApplicationDeploymentJob::check_git_if_build_needed() precedence rules. Please add tests that verify explicit non-PR commit input is not overridden by pinned SHA.

I can draft two focused tests:

  1. queue helper keeps explicit commit when pinned SHA exists,
  2. job keeps queued explicit commit when pinned SHA exists.

Based on learnings: "Every code change must be programmatically tested; write a new test or update an existing test, then run affected tests to ensure they pass."

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tests/Unit/PinnedCommitDeploymentTest.php` around lines 21 - 61, Add tests
asserting that an explicit non-PR commit passed to the deployment flow is not
overridden by an application's pinned SHA: 1) add a unit test for the queue
helper (queue_application_deployment) that sets
$this->application->git_commit_sha to a pinned SHA, calls
queue_application_deployment(...) with a commit => $explicitSha, then inspects
the queued job payload to assert it contains $explicitSha (and not the pinned
SHA); 2) add a unit test for ApplicationDeploymentJob::check_git_if_build_needed
(or invoke the job run path used in tests) that constructs a job payload
containing commit => $explicitSha while the Application model has git_commit_sha
set to the pinned SHA, then runs the job method and assert the job uses
$explicitSha for checkout/build decisions (i.e., the checked-out commit in
results/commands contains $explicitSha and not the pinned SHA). Reference
functions: queue_application_deployment(),
ApplicationDeploymentJob::check_git_if_build_needed(), and
Application::setGitImportSettings() when locating where to add/verify the
behavior.


});