fix(ci): actually skip CI on draft PRs

[spomichter]

Problem

PR #1398 claimed to skip CI on draft PRs but only added ready_for_review to the trigger types. The synchronize event still fires on every push regardless of draft status, wasting self-hosted runner time.

Fix

Added if: github.event.pull_request.draft == false to:

• check-changes job in docker.yml
• pre-commit job in code-cleanup.yml

These are the entry-point jobs — all downstream jobs depend on them via needs, so everything skips when a PR is draft.

2-line change

# docker.yml
check-changes:
  if: github.event.pull_request.draft == false  # ← added

# code-cleanup.yml
pre-commit:
  if: github.event.pull_request.draft == false  # ← added

Contributor License Agreement

• I have read and approved the CLA

[greptile-apps]

Greptile Summary

This PR adds if: github.event_name != 'pull_request' || github.event.pull_request.draft == false to the entry-point jobs of both docker.yml and code-cleanup.yml, correctly preventing CI from running when a synchronize push fires on a draft PR.

Key points:

• docker.yml: The condition is correct and necessary — the compound guard properly handles both push (no pull_request context) and pull_request (draft check) trigger types.
• code-cleanup.yml: The github.event_name != 'pull_request' portion is a dead branch because this workflow has no push trigger; the effective condition is simply github.event.pull_request.draft == false. Functionally correct, but the extra guard adds minor confusion.
• Partial skip, not full skip: Several jobs in docker.yml use if: always() (ros-python, dev, ros-dev, inspect-needs, ci-complete), so they are still dispatched to self-hosted runners on draft PRs even when check-changes is skipped. The expensive build operations inside those reusable workflows are correctly bypassed via should-run: false, but the jobs themselves are not zero-cost.
• PR description vs. implementation: The description shows the simpler draft == false guard, but the actual implementation is the more defensive compound form — this is an improvement over what was described.

Confidence Score: 4/5

• This PR is safe to merge — the core draft-skip logic is correct for both workflows, with only minor clarity and completeness concerns.
• The two-line changes accomplish their primary goal: draft PRs no longer run pre-commit or trigger the full docker build pipeline on synchronize events. The compound condition correctly handles push events in docker.yml. The redundant guard in code-cleanup.yml is harmless. The residual runner usage from if: always() downstream jobs is pre-existing behavior, not a regression from this PR. Score is 4 rather than 5 because the PR description overstates the savings ("everything skips") when several jobs will still touch self-hosted runners.
• .github/workflows/docker.yml — review the if: always() downstream jobs to confirm acceptable partial-skip behavior for draft PRs.

Important Files Changed

Filename	Overview
.github/workflows/code-cleanup.yml	Adds if guard to skip pre-commit job on draft PRs; the event_name != 'pull_request' portion is a dead branch since this workflow has no push trigger, but it is otherwise functionally correct.
.github/workflows/docker.yml	Adds correct if guard to check-changes; handles both push and pull_request triggers properly. However, several downstream jobs with if: always() still dispatch to self-hosted runners on draft PRs, so runner savings are partial rather than complete.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Workflow Triggered] --> B{Event Type?}
    B -- "push to main/dev" --> C["event_name != 'pull_request' → TRUE\ncheck-changes runs"]
    B -- "pull_request event" --> D{PR is draft?}
    D -- "draft == true" --> E["Condition FALSE\ncheck-changes SKIPPED"]
    D -- "draft == false" --> F["Condition TRUE\ncheck-changes RUNS"]

    E --> G["ros, python, navigation\nresult = skipped"]
    E --> H["ros-python, dev, ros-dev\nif: always() → dispatched\nshould-run = false inside template"]
    E --> I["inspect-needs\nif: always() → runs on runner"]
    E --> J["run-tests, run-mypy\nneeds.check-changes.result == 'success' → FALSE\nresult = skipped"]
    E --> K["ci-complete\nif: always() → runs on runner\nNo failures → ✅ passes"]

    F --> L["Normal CI pipeline\nAll jobs evaluate normally"]
    C --> L

Loading

Comments Outside Diff (1)

1. General comment

   if: always() downstream jobs still dispatch to self-hosted runners on draft PRs

   The PR description states "all downstream jobs depend on them via needs, so everything skips when a PR is draft." This is not entirely accurate. Several downstream jobs use if: always(), which bypasses the normal "skip if dependency skipped" behavior:

   • inspect-needs (line 83): pure if: always() — will always run on self-hosted runner
   • ros-python (line 100): if: always() — will dispatch to the reusable workflow with should-run: false because needs.check-changes.outputs.python resolves to '' (empty) when skipped
   • dev (line 125): same pattern
   • ros-dev (line 191): same pattern
   • ci-complete (line 239): if: always() — will always run

   While the expensive build operations inside those reusable workflows won't execute (since their should-run evaluates to false or needs.check-changes.result == 'success' fails), the jobs themselves are still dispatched and picked up by self-hosted runners. This represents non-zero runner overhead for draft PRs, contrary to the stated goal.

   This is pre-existing behavior rather than a regression introduced by this PR, but it is worth documenting so the intent of the draft-skip guard is clearly understood.

_{Last reviewed commit: b317bfb}

[greptile-apps]

if condition skips check-changes on push events to main/dev

docker.yml is also triggered by push events on main and dev branches (lines 3–8). For those push-triggered runs, github.event.pull_request does not exist in the event payload, so github.event.pull_request.draft resolves to '' (empty string) in GitHub Actions expression context.

GitHub Actions coerces '' == false as 0 == 0 → true, so the condition does evaluate to true for push events and the job runs correctly.

However, this coercion behavior is non-obvious and relies on undocumented type-casting semantics. A more robust and explicit form would be:

Suggested change

	if: github.event.pull_request.draft == false
	if: github.event_name == 'push' || github.event.pull_request.draft == false

This makes the intent unambiguous and guards against any future changes in GitHub Actions expression evaluation.

[spomichter]

@greptile

[greptile-apps]

Dead condition in event_name guard

code-cleanup.yml only has a pull_request trigger (line 3) — there is no push trigger in this file. This means github.event_name will always be 'pull_request' when this workflow runs, so the github.event_name != 'pull_request' branch is always false and never short-circuits the condition. The effective logic here is simply github.event.pull_request.draft == false.

This guard is necessary and correct in docker.yml (which has both push and pull_request triggers), but it is redundant in this file. Using the same pattern in both files provides consistency, which is a reasonable motivation — but it's worth noting the asymmetry so future readers aren't confused about why the extra guard is needed.

Suggested change

	if: github.event_name != 'pull_request' || github.event.pull_request.draft == false
	if: github.event.pull_request.draft == false