fix(ci): use path filter outputs for test should-run decisions

[spomichter]

Problem

PR #1279 removed paths-ignore: '**.md' from the pull_request trigger to fix branch protection blocking md-only PRs. But the test caller should-run expressions relied on dev.result == 'success' to decide whether to run tests.

Build jobs with if: always() report success even when should-run=false and all steps are skipped. So for md-only PRs:

1. check-changes correctly detects no code changes (all outputs false)
2. dev job runs with should-run=false, skips all steps, reports success
3. Test callers see dev.result == 'success' → should-run = true → full test suite runs

Fix

Replace upstream job result checks with direct path filter output checks:

• Non-ROS tests (run-tests, run-heavy-tests, run-lcm-tests, run-integration-tests): tests || python || dev
• ROS tests + mypy (run-ros-tests, run-mypy): tests || ros || python || dev

When only .md files change, all path filter outputs are false → should-run = false → tests skip.

Verification

After merging, open a PR that only changes .md files against dev. Test jobs should complete in seconds with all steps skipped, and ci-complete should pass.

[greptile-apps]

Greptile Summary

This PR fixes a regression introduced in #1279 where removing paths-ignore: '**.md' from the pull_request trigger caused test jobs to erroneously run on markdown-only PRs. The root cause was that dev and ros-dev build jobs with if: always() report success even when skipped, so the old needs.dev.result == 'success' condition was insufficient to suppress tests.

The fix replaces upstream job result checks with direct check-changes path filter outputs in the should-run expression for all six test caller jobs:

• run-ros-tests and run-mypy: now trigger on tests || ros || python || dev
• run-tests, run-heavy-tests, run-lcm-tests, run-integration-tests: now trigger on tests || python || dev

Key changes:

• The new logic correctly short-circuits when all path filter outputs are false (e.g. md-only PRs), preventing unnecessary test runs.
• The fix removes the dependency on upstream build job results (dev.result, ros-dev.result) from should-run decisions, which was the source of the bug.
• Behavioral difference: Under the old logic, a failed upstream build job would cause should-run=false (tests skipped). Under the new logic, if a build fails but a relevant path filter is true, tests will still run with should-run=true — using the fallback :dev image (controlled by the dev-image expression which already accounts for this). This is arguably more correct (run tests even if build fails, just use the last known-good image), but it's a subtle behavioral change worth being aware of.
• The commented-out run-module-tests job still uses the old pattern — not a problem since it's commented out, but should be updated if it gets re-enabled.

Confidence Score: 4/5

• This PR is safe to merge — it correctly fixes the md-only PR test over-triggering bug with no functional regressions for normal code PRs.
• The fix directly addresses the root cause described in the PR description. The new path-filter-output-based conditions are simpler and more correct than the upstream-job-result-based conditions for the should-run decision. The only caveat is a minor behavioral difference when an upstream build fails (tests now run against fallback image instead of being skipped), which is a pre-existing design choice already expressed in the dev-image fallback logic. No security concerns, no syntax errors, and the commented-out job's stale pattern is a low-risk style issue.
• .github/workflows/docker.yml — specifically the commented-out run-module-tests block which still uses the old pattern.

Important Files Changed

Filename	Overview
.github/workflows/docker.yml	Replaces upstream job result checks with direct path filter output checks for test should-run decisions. The fix correctly prevents spurious test runs on md-only PRs. One minor concern: test jobs can now run with should-run=true against the fallback :dev image even when an upstream build job explicitly fails, whereas the old logic would have short-circuited in that case.

Flowchart

flowchart TD
    A[PR Opened] --> B[check-changes job]
    B --> C{dorny/paths-filter outputs}
    C --> D[ros: true/false]
    C --> E[python: true/false]
    C --> F[dev: true/false]
    C --> G[tests: true/false]

    D --> H[ros build job]
    E --> I[python build job]
    F --> J[dev build job\nneeds: python]
    D --> K[ros-python build job]
    K --> L[ros-dev build job\nneeds: ros-python]

    H --> L

    G --> M{should-run decision\nnew logic}
    E --> M
    F --> M
    D --> N{should-run decision\nnew logic ROS}
    E --> N
    F --> N
    G --> N

    M -->|tests OR python OR dev| O[run-tests\nrun-heavy-tests\nrun-lcm-tests\nrun-integration-tests]
    N -->|tests OR ros OR python OR dev| P[run-ros-tests\nrun-mypy]

    J --> O
    L --> P

    O --> Q[ci-complete gate]
    P --> Q
    Q -->|all pass/skip| R[✅ Branch protection passes]
    Q -->|any failure/cancel| S[❌ CI fails]

Loading

_{Last reviewed commit: 9ac8df0}

[greptile-apps]

_{1 file reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Tests run even when upstream build fails

The new condition correctly fixes the md-only PR issue, but it removes the implicit guard that the old logic provided: under the previous condition, if ros-dev explicitly failed (result != 'success' and != 'skipped'), should-run would be false. Now, if ros-dev fails but tests == 'true', tests will run with should-run=true against the fallback :dev image.

This is not necessarily wrong — the dev-image expression already handles the fallback — but it does mean CI will report test results even when the freshly built image wasn't used. Consider whether you want to surface build failures as test skips or as tests running against stale images.

[greptile-apps]

Additional Comments (1)

.github/workflows/docker.yml
Stale pattern in commented-out job

The commented-out run-module-tests job (lines 239–271) still references the old needs.dev.result == 'success' pattern that this PR is replacing. If this job is re-enabled in the future without updating its condition, it will reintroduce the same bug.

#   if: ${{
#     always() &&
#     needs.check-changes.result == 'success' &&
#     ((needs.dev.result == 'success') ||
#     (needs.dev.result == 'skipped' &&
#     needs.check-changes.outputs.tests == 'true'))
#     }}

Consider updating it to match the new pattern (tests || python || dev) so any future uncomment picks up the correct logic, or add a comment noting it needs updating before re-enabling.

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}