AkurAI Build
Menu

AkurAI-Build / Pull requests / #4

fix(runner): run branch-filtered jobs on merge-queue refs, and say why a job skipped

Merged · fix/merge-queue-branch-filters → main · stdio

Base: 3b1a0cfc9b74fbf61cdf4bb2b9e69802eca3158d Head: 84f8fa363294b9e3de2bbfa92d5902d73a86db4f

Two defects found by using the merge queue to deliver PR #3, not by reading the code.

1. branches: filters skipped merge-queue refs

Filters were matched against the ref's literal name, so a job gated on [main] skipped on refs/merge-queue/N.

Observed live on run 2002: PR #3 was gated by verify-pr alone; verify, package and deploy-production all skipped, the merge landed on main, and no run fired on main afterwards. So a queue merge shipped code that was never packaged, never deployed, and never saw cargo fmt, clippy -D warnings, the full test suite or test_pr_pipeline.py. The queue's CI was strictly weaker than the branch's, which inverts the point of the gate. Deploying #3 required a manual akurai_run_queue on the merged SHA (run 2003) to make up the difference.

branches is now matched against the branch the ref is going to become. The existing queue_targets_default computation — an independently approved merge commit the queue is about to land on the default branch — is hoisted above the filter and yields an effective_branch. This mirrors exactly the native-executor trust exception that already sat directly below it, on the same ref, for the same reason. Only merge-queue refs whose entry's PR targets the default branch qualify; every other ref matches on its own name, unchanged.

2. Skip reasons named neither cause nor culprit

A job filtered out by branches: reported Git ref is not allowed for this job, and anything downstream reported dependency did not succeed — which reads as a test failure. On run 2001 that sent me looking for a break that did not exist: verify-pr had passed exit 0 and the real cause was the branch filter.

Now: job is limited to branches ["main"] and this run is on "feature", and dependency "verify" is skipped.

Tests

  • A branches: [main] native job succeeds on a real refs/merge-queue/N whose approved PR targets main.
  • An unrelated ref is still skipped, with the reason naming both the filter and the actual ref.

335 passing; clippy --all-targets and fmt --check clean.

Note on this PR's own delivery

The running controller is 3b1a0cfc, which predates fix 1 — so this PR's own merge-queue run will again skip verify/package/deploy-production, and will again need a manual run on the merged SHA to deploy. From the next merge onward the queue does the full pipeline by itself.

🤖 Generated with Claude Code

https://claude.ai/code/session_01FRtUmfFA7yWCFpX9dCaXvz

Changes

diff --git a/src/runner.rs b/src/runner.rs
index 37cc03c..6daaf08 100644
--- a/src/runner.rs
+++ b/src/runner.rs
@@ -466,7 +466,11 @@ impl Runner {
                     .iter()
                     .filter(|candidate| job.needs.contains(&candidate.base_name))
                     .collect::<Vec<_>>();
-                if dependencies.iter().any(|dependency| {
+                // Name the dependency and its real status. The bare
+                // "dependency did not succeed" read as a test failure when the
+                // dependency had merely been filtered out by `branches:`, which
+                // sent a reader hunting for a break that did not exist.
+                if let Some(blocker) = dependencies.iter().find(|dependency| {
                     matches!(
                         dependency.status.as_str(),
                         "failed" | "skipped" | "canceled" | "interrupted"
@@ -475,7 +479,10 @@ impl Runner {
                     self.database.set_job_status(
                         job.id,
                         "skipped",
-                        Some("dependency did not succeed"),
+                        Some(&format!(
+                            "dependency {:?} is {}",
+                            blocker.base_name, blocker.status
+                        )),
                         None,
                     )?;
                     progressed = true;
@@ -535,15 +542,6 @@ impl Runner {
             .git_ref
             .strip_prefix("refs/heads/")
             .unwrap_or(&run.git_ref);
-        if !spec.branches.is_empty() && !spec.branches.iter().any(|allowed| allowed == branch) {
-            self.database.set_job_status(
-                job_id,
-                "skipped",
-                Some("Git ref is not allowed for this job"),
-                None,
-            )?;
-            return Ok(());
-        }
         let repository_trusted = self.database.repository_trusted(run.repository_id)?;
         let repository_default_branch = self.database.repository(&run.repository)?.default_branch;
         // A trusted *repository* only earns unrestricted execution on its
@@ -565,6 +563,36 @@ impl Runner {
                     .ok()
             })
             .is_some_and(|detail| detail.pull_request.target_ref == repository_default_branch);
+        // `branches:` filters are matched against the branch this ref is going
+        // to BECOME, not its literal name. A merge-queue staging ref is the
+        // default branch's own pre-merge commit, so a job gated on `[main]`
+        // must run there -- otherwise the queue's CI is strictly WEAKER than
+        // the branch's, which inverts the gate: verify/package/deploy-production
+        // all skipped on refs/merge-queue/N and the merge landed code that was
+        // never packaged, never deployed, and never saw fmt/clippy/full tests.
+        // This mirrors the native-executor trust exception directly above.
+        let effective_branch = if queue_targets_default {
+            repository_default_branch.as_str()
+        } else {
+            branch
+        };
+        if !spec.branches.is_empty()
+            && !spec
+                .branches
+                .iter()
+                .any(|allowed| allowed == effective_branch)
+        {
+            self.database.set_job_status(
+                job_id,
+                "skipped",
+                Some(&format!(
+                    "job is limited to branches {:?} and this run is on {effective_branch:?}",
+                    spec.branches
+                )),
+                None,
+            )?;
+            return Ok(());
+        }
         let ref_trusted =
             repository_trusted && (branch == repository_default_branch || queue_targets_default);
         if spec.executor == "native" && (!self.allow_native || !ref_trusted) {
@@ -2016,6 +2044,72 @@ mod tests {
         }
     }
 
+    #[test]
+    fn branch_filter_matches_merge_queue_refs_targeting_the_default_branch() -> Result<()> {
+        // A `branches: [main]` job MUST run on refs/merge-queue/N when that
+        // entry's PR targets main. Before this, verify/package/deploy-production
+        // all skipped on the queue ref, so the queue's CI was strictly weaker
+        // than the branch's and a queue merge landed code that was never
+        // packaged, never deployed, and never saw fmt/clippy/full tests.
+        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 spec = make_job_spec("build", "native", None);
+        spec.branches = vec!["main".into()];
+        database.insert_jobs(run_id, &[spec])?;
+        let runner = Runner::new(database, root.path().to_owned(), true)?;
+        let workspace = root.path().join("workspaces").join(run_id.to_string());
+        fs::create_dir_all(&workspace)?;
+        let job = runner.database.jobs(run_id)?[0].id;
+        runner.execute_job(run_id, job, &workspace, &RunCancellation::new())?;
+        let job = &runner.database.jobs(run_id)?[0];
+        assert_eq!(job.status, "succeeded", "{:?}", job.logs);
+        Ok(())
+    }
+
+    #[test]
+    fn branch_filter_still_skips_an_unrelated_ref_and_says_why() -> Result<()> {
+        let root = tempfile::tempdir()?;
+        let database = Database::memory(KEY)?;
+        let repo = database.add_repository("app", "https://example.com/app.git", "main")?;
+        let run_id = database.create_run(repo.id, "refs/heads/feature", None, "manual")?;
+        let mut spec = make_job_spec("build", "native", None);
+        spec.branches = vec!["main".into()];
+        database.insert_jobs(run_id, &[spec])?;
+        let runner = Runner::new(database, root.path().to_owned(), true)?;
+        let workspace = root.path().join("workspaces").join(run_id.to_string());
+        fs::create_dir_all(&workspace)?;
+        let job = runner.database.jobs(run_id)?[0].id;
+        runner.execute_job(run_id, job, &workspace, &RunCancellation::new())?;
+        let job = &runner.database.jobs(run_id)?[0];
+        assert_eq!(job.status, "skipped");
+        assert!(
+            job.logs.contains("limited to branches") && job.logs.contains("feature"),
+            "skip reason must name the filter and the actual ref: {:?}",
+            job.logs
+        );
+        Ok(())
+    }
+
     #[test]
     fn native_job_rejected_on_non_default_branch_of_trusted_repository() -> Result<()> {
         let root = tempfile::tempdir()?;

Reviews

Current changes approved.

reviewer · Approved

Reviewed at head 84f8fa363294b9e3de2bbfa92d5902d73a86db4f, base 3b1a0cfc9b74fbf61cdf4bb2b9e69802eca3158d. CI: run 2004, verify-pr (docker, rust@sha256:4277ce86..., untrusted-branch path) -> exit 0, "335 passed; 0 failed; 4 ignored". Checked the security boundary first, because this change moves code near it: - `ref_trusted` is byte-identical to before: `repository_trusted && (branch == repository_default_branch || queue_targets_default)`. `queue_targets_default` was hoisted, not rewritten -- same DB lookups, same repository_id filter, same target_ref == default_branch predicate. No trust decision changed. - `effective_branch` substitutes the default branch ONLY when `queue_targets_default` already holds, which requires a real merge_queue_entry in this repository whose PR targets the default branch. A caller cannot reach it by naming a ref: `refs/merge-queue/<n>` with no matching entry, an entry from another repository, or a PR targeting a non-default ref all fall through to the literal branch name. - Consequently this widens `branches:` matching for exactly the refs that already earned native-executor trust one check below, and for nothing else. An attacker-controlled feature branch gains nothing: it was already excluded by both the filter and the trust gate, and still is. - `spec.branches.is_empty()` short-circuit is unchanged, so unfiltered jobs behave exactly as before. Behaviour verified by the two new tests rather than by inspection alone: a `branches: [main]` NATIVE job succeeds on a genuine merge-queue ref built from an enqueued, independently approved PR targeting main; an unrelated ref is still skipped and the reason names both the filter and the actual ref. The skip-message change is diagnostic only -- it alters no control flow. `find` replacing `any` preserves the same predicate and the same skip decision; it only captures which dependency triggered it. The defect being fixed is real and was observed, not theorised: run 2002 merged PR #3 with verify/package/deploy-production skipped and produced no deployment, and no run fired on main afterwards. Deploying #3 needed a manual run (2003) to compensate. Accepted limitation, stated in the PR body: the running controller predates this fix, so THIS PR's merge-queue run will also skip the main-only jobs and will need a manual run on the merged SHA. Self-consistent and correctly disclosed. Disclosure: this approval is not independent. The same session authored the change; the reviewer identity was used at the repository owner's explicit instruction. The gate is satisfied procedurally, not by a second pair of eyes.

Current changes

Merge queue

No changes waiting to merge.