Skip approval-gated deployments on merge-queue refs too

Merged · fix/skip-approval-gated-deploys-on-queue-refs → main · stdio

Completes d8a4a3d, which was a half fix that looked complete.

What d8a4a3d missed

The merge-queue deployment guard went into execute_job, which only sees jobs that actually reach execution. execute_claimed parks an approval-gated job and moves on before calling it:

if job.approval_required && job.approved_at.is_none() {
    self.database.set_job_status(job.id, "waiting", None, None)?;
    progressed = true;
    continue;          // <- execute_job, and its guard, never run
}
self.execute_job(run_id, job.id, &workspace, cancellation)?;

So the fix covered exactly one of the two symptoms it was written for:

Repo	`deploy` job	Result under d8a4a3d
AkurAI-Build	`approval: false`	correctly skipped on `refs/merge-queue/N`
AkurAI-Framework	`approval: true`	still waiting — queue still blocked

Run 2011 reproduced it against the deployed fix: verify succeeded, package succeeded, deploy waiting on refs/merge-queue/7. I confirmed the deployed binary did contain d8a4a3d (service restarted 12:34:46, nine seconds after the merge, and the new skip string is present in it), so this was a coverage gap and not a stale deploy.

Why the first regression test did not catch it: make_job_spec defaults approval to false, and the test only set environment. It therefore exercised only the non-approval path and passed against an incomplete fix — the failure mode I should have anticipated, since the approval-gated case is precisely the one that was stalling the queue.

The fix

Decide before the approval gate, and share the merge-queue predicate between the two call sites via Runner::queue_targets_default rather than deriving it twice.

Verification

approval_gated_deployment_is_skipped_not_parked_on_merge_queue_refs drives the whole claimed-run path instead of execute_job in isolation, and asserts both that the job is skipped with a reason naming the merge queue and that the run does not end in waiting. I verified it fails without the new guard (left: "waiting", right: "skipped").

cargo fmt --all -- --check: clean
cargo clippy --all-targets --all-features --locked -- -D warnings: clean
cargo test --lib: 337 passed, 0 failed, 4 ignored

Unlike the previous PR, this one's own queue run will already skip its deployment job — d8a4a3d is deployed, and this job has no approval gate.

🤖 Generated with Claude Code

https://claude.ai/code/session_01FRtUmfFA7yWCFpX9dCaXvz

Changes

diff --git a/src/runner.rs b/src/runner.rs
index 1a689bc..798be17 100644
--- a/src/runner.rs
+++ b/src/runner.rs
@@ -494,6 +494,33 @@ impl Runner {
                 {
                     continue;
                 }
+                // Deployment jobs never run on a merge-queue ref. This has to
+                // be decided BEFORE the approval gate below: an approval-gated
+                // job is parked in `waiting` here and never reaches
+                // execute_job, so the equivalent guard there cannot see it.
+                // That gap left AkurAI-Framework's approval-gated deploy
+                // stalling the queue on a human production approval inside
+                // what is only a merge check, even after the execute_job
+                // guard shipped.
+                if job.environment.is_some()
+                    && self.queue_targets_default(
+                        &run.git_ref,
+                        run.repository_id,
+                        &repository.default_branch,
+                    )
+                {
+                    self.database.set_job_status(
+                        job.id,
+                        "skipped",
+                        Some(
+                            "deployment jobs do not run on merge-queue refs; the \
+                             post-merge default-branch run deploys",
+                        ),
+                        None,
+                    )?;
+                    progressed = true;
+                    continue;
+                }
                 if job.approval_required && job.approved_at.is_none() {
                     self.database
                         .set_job_status(job.id, "waiting", None, None)?;
@@ -524,6 +551,29 @@ impl Runner {
         }
     }
 
+    /// True when `git_ref` is a merge-queue staging ref whose entry's pull
+    /// request targets the repository's default branch -- i.e. a commit the
+    /// queue is about to land there, which is what makes running the default
+    /// branch's jobs on it correct.
+    fn queue_targets_default(
+        &self,
+        git_ref: &str,
+        repository_id: i64,
+        default_branch: &str,
+    ) -> bool {
+        git_ref
+            .strip_prefix("refs/merge-queue/")
+            .and_then(|id| id.parse::<i64>().ok())
+            .and_then(|id| self.database.merge_queue_entry(id).ok())
+            .filter(|entry| entry.repository_id == repository_id)
+            .and_then(|entry| {
+                self.database
+                    .pull_request(&entry.repository, entry.pull_request_number)
+                    .ok()
+            })
+            .is_some_and(|detail| detail.pull_request.target_ref == default_branch)
+    }
+
     fn execute_job(
         &self,
         run_id: i64,
@@ -551,18 +601,8 @@ impl Runner {
         // A merge-queue staging ref is the exception: it is an independently
         // approved merge commit that the queue is about to land on the
         // default branch, and CI on it is what gates that landing.
-        let queue_targets_default = run
-            .git_ref
-            .strip_prefix("refs/merge-queue/")
-            .and_then(|id| id.parse::<i64>().ok())
-            .and_then(|id| self.database.merge_queue_entry(id).ok())
-            .filter(|entry| entry.repository_id == run.repository_id)
-            .and_then(|entry| {
-                self.database
-                    .pull_request(&entry.repository, entry.pull_request_number)
-                    .ok()
-            })
-            .is_some_and(|detail| detail.pull_request.target_ref == repository_default_branch);
+        let queue_targets_default =
+            self.queue_targets_default(&run.git_ref, run.repository_id, &repository_default_branch);
         // Validating a merge is not the same as landing it. A merge-queue ref
         // is a commit that has not merged and may never merge, so a job that
         // deploys (`environment:` set) must not run there: it would put
@@ -2170,6 +2210,60 @@ mod tests {
         Ok(())
     }
 
+    #[test]
+    fn approval_gated_deployment_is_skipped_not_parked_on_merge_queue_refs() -> Result<()> {
+        // The execute_job guard alone was NOT enough: execute_claimed parks an
+        // approval-gated job in `waiting` and `continue`s, so such a job never
+        // reaches execute_job and its guard never fires. AkurAI-Framework's
+        // deploy is approval-gated and kept stalling the queue for exactly
+        // this reason, even after the first fix deployed. The decision has to
+        // happen before the approval gate, so assert the whole claimed-run
+        // path here rather than execute_job in isolation.
+        let root = tempfile::tempdir()?;
+        let database = Database::memory(KEY)?;
+        let repo = database.add_repository("app", "https://example.com/app.git", "main")?;
+        database.create_pull_request(
+            "app",
+            "Feature",
+            "",
+            "feature",
+            "main",
+            &"a".repeat(40),
+            &"b".repeat(40),
+            "alice",
+            "Alice",
+        )?;
+        database.add_pull_request_review("app", 1, "bob", "Bob", "approved", "ship it")?;
+        let entry = database.enqueue_merge("app", 1, "tester")?;
+        let run_id = database.create_run(
+            repo.id,
+            &format!("refs/merge-queue/{}", entry.id),
+            None,
+            "webhook",
+        )?;
+        let mut deploy = make_job_spec("deploy", "native", None);
+        deploy.branches = vec!["main".into()];
+        deploy.environment = Some("production".into());
+        deploy.approval = true;
+        database.insert_jobs(run_id, &[deploy])?;
+        assert!(database.claim_run(run_id)?);
+        let runner = Runner::new(database, root.path().to_owned(), true)?;
+        fs::create_dir_all(root.path().join("workspaces").join(run_id.to_string()))?;
+        let detail = runner.process_claimed_run(run_id)?;
+        let deploy = &detail.jobs[0];
+        assert_eq!(deploy.status, "skipped", "{:?}", deploy.logs);
+        assert!(
+            deploy.logs.contains("merge-queue"),
+            "skip reason must say why: {:?}",
+            deploy.logs
+        );
+        assert_ne!(
+            detail.run.status, "waiting",
+            "an approval-gated deployment must not park the queue run in waiting"
+        );
+        Ok(())
+    }
+
     #[test]
     fn branch_filter_still_skips_an_unrelated_ref_and_says_why() -> Result<()> {
         let root = tempfile::tempdir()?;

Reviews

Current changes approved.

reviewer · Approved

Approved. I reviewed d8a4a3d as complete and it was not, so I re-checked this one against the live failure rather than against the description. Confirmed the gap is real and not a stale deploy: - The running binary does contain d8a4a3d (akurai-build.service ExecMainStartTimestamp 12:34:46, nine seconds after the merge at 12:34:37, and `strings` on /home/olibuijr/.local/lib/akurai-build/akurai finds the skip string). - Run 2011 on refs/merge-queue/7 nevertheless shows verify=succeeded, package=succeeded, deploy=waiting with environment=production and approval_required=true, and no skip reason recorded. - Reading execute_claimed, the approval branch sets "waiting" and `continue`s before execute_job is ever called, so the guard shipped in d8a4a3d is unreachable for an approval-gated job. That is the whole defect. The fix is placed correctly: the environment check now precedes the approval gate in the execute_claimed loop, and both call sites share Runner::queue_targets_default instead of deriving the predicate twice, so they cannot drift apart. job.environment is populated on the Job row, so the loop does not need to deserialize spec_json. The new test is the important part. It drives process_claimed_run rather than execute_job in isolation -- the precise scoping error that let the first test pass against a half fix -- sets approval=true, and asserts both the skip reason and that the run does not end in `waiting`. I verified independently that removing the loop guard makes it fail (left: "waiting", right: "skipped"). The earlier execute_job-level test is retained, so both paths stay pinned. fmt clean, clippy -D warnings clean, cargo test --lib 337 passed / 0 failed. Scope is unchanged for ordinary branch and default-branch runs: queue_targets_default is false there, so deployments still run exactly as before, and the post-merge default-branch run remains the thing that deploys.

Current changes

Merge queue

No changes waiting to merge.