Skip to content

Only run evals etc when relevant - #1589

Closed
aantn wants to merge 5 commits into
masterfrom
claude/skip-evals-docs-only-FuMiy
Closed

aantn wants to merge 5 commits into
masterfrom
claude/skip-evals-docs-only-FuMiy

Conversation

@aantn

@aantn aantn commented Feb 19, 2026 •

Copy link
Copy Markdown
Collaborator

Summary by CodeRabbit

  • Chores
    • CI adds a pre-check that detects docs-only or non-code PRs and exposes a code-change flag.
    • Builds, benchmarks, evaluations, and comparison steps are gated by that flag and will skip when only non-code files changed, reducing unnecessary runs and improving PR turnaround.
    • Benchmark artifacts are now surfaced when runs occur.

Add paths-ignore filters to eval-regression, cli-performance, and
docker-dev-images workflows so they don't run when a PR only changes
documentation files (docs/**, *.md, mkdocs.yml). Other triggers like
workflow_dispatch, issue_comment, push to master, and schedule are
unaffected since GitHub Actions paths filters only apply to push and
pull_request events.

https://claude.ai/code/session_01JGwBxWqzQyD3Wwd5K4HtV8
Signed-off-by: Claude <noreply@anthropic.com>
paths-ignore prevents the workflow from triggering entirely, which causes
required status checks to stay in "expected" state and block PR merges.

Instead, always trigger the workflow but add a lightweight check-changes
job using dorny/paths-filter that detects docs-only PRs. Expensive jobs
(evals, benchmarks, docker build) depend on it and get cleanly "skipped"
when only docs/md/mkdocs.yml files changed - which satisfies required
status checks.

For eval-regression.yaml, check-changes only runs on pull_request events.
Non-PR triggers (push, workflow_dispatch, issue_comment) are unaffected
because the output defaults to empty (not 'false'), so llm_evals still
runs.

https://claude.ai/code/session_01JGwBxWqzQyD3Wwd5K4HtV8
Signed-off-by: Claude <noreply@anthropic.com>
@netlify

netlify Bot commented Feb 19, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for holmes-docs ready!

Name Link
🔨 Latest commit b42d416
🔍 Latest deploy log https://app.netlify.com/projects/holmes-docs/deploys/699775f7f1c7aa00080ea459
😎 Deploy Preview https://deploy-preview-1589--holmes-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@github-actions

github-actions Bot commented Feb 19, 2026 •

Copy link
Copy Markdown
Contributor

✅ Docker image ready for 60c9ba3 (built in 44s)

⚠️ Warning: does not support ARM (ARM images are built on release only - not on every PR)

Use this tag to pull the image for testing.

📋 Copy commands

⚠️ Temporary images are deleted after 30 days. Copy to a permanent registry before using them:

gcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:60c9ba3
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:60c9ba3 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:60c9ba3
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:60c9ba3

Patch Helm values in one line (choose the chart you use):

HolmesGPT chart:

helm upgrade --install holmesgpt ./helm/holmes \
  --set registry=me-west1-docker.pkg.dev/robusta-development/development \
  --set image=holmes-dev:60c9ba3

Robusta wrapper chart:

helm upgrade --install robusta robusta/robusta \
  --reuse-values \
  --set holmes.registry=me-west1-docker.pkg.dev/robusta-development/development \
  --set holmes.image=holmes-dev:60c9ba3

@github-actions

github-actions Bot commented Feb 19, 2026 •

Copy link
Copy Markdown
Contributor

📂 Previous Runs

📜 Run @ 895f57f (#22172446458)

✅ Results of HolmesGPT evals

Automatically triggered by commit 895f57f on branch claude/skip-evals-docs-only-FuMiy

View workflow logs

⚠️ No eval report was generated.


✅ Results of HolmesGPT evals

Automatically triggered by commit c369d52 on branch claude/skip-evals-docs-only-FuMiy

View workflow logs

⚠️ No eval report was generated.

📖 Legend
Icon Meaning
✅ The test was successful
➖ The test was skipped
⚠️ The test failed but is known to be flaky or known to fail
🚧 The test had a setup failure (not a code regression)
🔧 The test failed due to mock data issues (not a code regression)
🚫 The test was throttled by API rate limits/overload
❌ The test failed and should be fixed before merging the PR
🔄 Re-run evals manually

⚠️ Warning: /eval comments always run using the workflow from master, not from this PR branch. If you modified the GitHub Action (e.g., added secrets or env vars), those changes won't take effect.

To test workflow changes, use the GitHub CLI or Actions UI instead:

gh workflow run eval-regression.yaml --repo HolmesGPT/holmesgpt --ref claude/skip-evals-docs-only-FuMiy -f markers=regression -f filter=

Option 1: Comment on this PR with /eval:

/eval
markers: regression

Or with more options (one per line):

/eval
model: gpt-4o
markers: regression
filter: 09_crashpod
iterations: 5

Run evals on a different branch (e.g., master) for comparison:

/eval
branch: master
markers: regression
Option Description
model Model(s) to test (default: same as automatic runs)
markers Pytest markers (no default - runs all tests!)
filter Pytest -k filter (use /list to see valid eval names)
iterations Number of runs, max 10
branch Run evals on a different branch (for cross-branch comparison)

Quick re-run: Use /rerun to re-run the most recent /eval on this PR with the same parameters.

Option 2: Trigger via GitHub Actions UI → "Run workflow"

🏷️ Valid markers

benchmark, chain-of-causation, compaction, confluence, context_window, coralogix, counting, database, datadog, datetime, easy, elasticsearch, embeds, fast, frontend, grafana-dashboard, hard, integration, kafka, kubernetes, leaked-information, logs, loki, medium, metrics, network, newrelic, no-cicd, numerical, one-test, port-forward, prometheus, question-answer, regression, runbooks, slackbot, storage, toolset-limitation, traces, transparency


Commands: /eval · /rerun · /list

CLI: gh workflow run eval-regression.yaml --repo HolmesGPT/holmesgpt --ref claude/skip-evals-docs-only-FuMiy -f markers=regression -f filter=

@coderabbitai

coderabbitai Bot commented Feb 19, 2026 •

Copy link
Copy Markdown
Contributor

Walkthrough

Adds a reusable GitHub Actions job check-changes to several workflows to detect docs-only PRs and gate downstream jobs (build, benchmarks, evals, compare) to run only when non-doc code changes exist via has_code_changes output.

Changes

Cohort / File(s) Summary
CLI Performance Workflow
​.github/workflows/cli-performance.yaml
Added check-changes job; benchmark-pr, benchmark-master, and compare now needs: check-changes and guarded with if: needs.check-changes.outputs.has_code_changes == 'true'; benchmark-pr exposes new outputs startup_json, llm_json.
Docker Dev Images Workflow
​.github/workflows/docker-dev-images.yaml
Added check-changes job that filters changed paths; build now needs: check-changes and runs only when has_code_changes is true, preventing builds for docs/config-only changes.
Evaluation Regression Workflow
​.github/workflows/eval-regression.yaml
Added check-changes job; llm_evals now needs: [check-changes] and its if condition includes needs.check-changes.outputs.has_code_changes gating alongside existing event-type/cancel checks.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant PR as "PR / Push"
participant Check as "check-changes job"
participant CI as "CI jobs (build / llm_evals / benchmark-pr / benchmark-master)"
participant Compare as "compare job"
PR->>Check: trigger and list changed files
Note over Check: set outputs.has_code_changes = true|false
alt has_code_changes == true
Check-->>CI: allow downstream jobs via needs/if
CI->>Compare: produce artifacts/outputs (e.g., startup_json, llm_json)
Compare->>Compare: run comparison
else has_code_changes == false
Check-->>CI: downstream jobs skipped by if guards
Compare-->>Compare: skipped
end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • Sheeproid
  • arikalon1
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Only run evals etc when relevant' directly relates to the main purpose of the PR: adding code-change detection gates to prevent unnecessary workflow runs when only documentation or configuration files are modified.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

github-actions Bot commented Feb 19, 2026 •

Copy link
Copy Markdown
Contributor

🔬 CLI Performance Benchmark

🟡 Startup Time (no LLM)

Measures holmes version execution time (imports + initialization)

Metric PR Master Change
Cold Start 10.46s 10.94s -4.4%
Warm Mean 4.69s 4.91s -4.4%
Warm Min 4.67s 4.85s
Warm Max 4.71s 4.98s

🟡 Full CLI with LLM

Measures holmes ask execution time (OpenRouter + Haiku 4.5)

Metric PR Master Change
Cold Start 21.31s 32.65s -34.7%
Warm Mean 7.31s 7.67s -4.7%
Warm Min 7.02s 6.89s
Warm Max 7.65s 8.77s

PR: 60c9ba34 | Master: b68d17fa | Iterations: 5

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (2)
.github/workflows/eval-regression.yaml (1)

140-150: !cancelled() + != 'false' pattern is correct; consider guarding against check-changes failures.

The design handles all cases correctly:

  • pull_request, code changes → check-changes outputs 'true'; 'true' != 'false' ✓ runs
  • pull_request, docs-only → check-changes outputs 'false'; 'false' != 'false' ✗ skips
  • push / workflow_dispatch / issue_comment → check-changes is skipped (output is ''); !cancelled() allows llm_evals to evaluate despite the skipped dependency; '' != 'false' ✓ runs

One minor gap: if check-changes fails (e.g., GitHub API error), !cancelled() is still true and the empty output satisfies != 'false', so llm_evals would run unconditionally for PR events — effectively ignoring the docs filter. A needs.check-changes.result != 'failure' guard closes this.

🛡️ Proposed defensive guard
     if: |
       !cancelled() &&
+      needs.check-changes.result != 'failure' &&
       needs.check-changes.outputs.has_code_changes != 'false' &&
       (
         github.event_name == 'pull_request' ||
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/eval-regression.yaml around lines 140 - 150, Add a
defensive guard to the job conditional to prevent running when the check-changes
job failed: update the if expression that currently uses !cancelled() and
needs.check-changes.outputs.has_code_changes != 'false' to also require
needs.check-changes.result != 'failure' so that when check-changes fails (not
just skipped) the job will not run; reference the existing symbols !cancelled(),
needs.check-changes.outputs.has_code_changes and add needs.check-changes.result
!= 'failure' into the combined boolean expression controlling the job.
.github/workflows/cli-performance.yaml (1)

18-18: Pin dorny/paths-filter to a commit SHA in three workflows.

Using a mutable tag (@v3) allows an upstream tag move to silently execute different code in a context that holds GITHUB_TOKEN. Pin to the exact commit SHA of the desired release for supply-chain safety. This applies to:

  • .github/workflows/cli-performance.yaml line 18
  • .github/workflows/docker-dev-images.yaml line 20
  • .github/workflows/eval-regression.yaml line 53

Use the format: dorny/paths-filter@<commit-sha> # v3 (GitHub's recommended pattern: see their security guide for actions).

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

In @.github/workflows/cli-performance.yaml at line 18, Replace the mutable tag
usage "dorny/paths-filter@v3" with a pinned commit SHA for each occurrence
(three places) so the action cannot change unexpectedly; update the "uses:
dorny/paths-filter@v3" lines to the form "dorny/paths-filter@<commit-sha> # v3"
(keep the “# v3” comment), ensuring you pick the commit SHA that corresponds to
the v3 release and apply the change where the exact string
"dorny/paths-filter@v3" appears.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In @.github/workflows/docker-dev-images.yaml:
- Line 20: The workflow currently references the third-party action with a loose
tag "dorny/paths-filter@v3"; replace this with a SHA-pinned reference to a
specific commit (e.g. "dorny/paths-filter@<commit-sha>") to avoid supply-chain
drift—update the usage in .github/workflows/docker-dev-images.yaml to the same
exact commit SHA you used to fix cli-performance.yaml (or a vetted commit SHA),
ensuring the action is locked to a fixed immutable version.

In @.github/workflows/eval-regression.yaml:
- Line 53: The workflow uses the floating tag dorny/paths-filter@v3 which is not
SHA-pinned; update the action reference to a fixed commit SHA (e.g.
dorny/paths-filter@<full-commit-sha>) to ensure reproducible builds and match
the change requested in cli-performance.yaml, replacing the tag usage of
dorny/paths-filter@v3 with the corresponding immutable SHA for the same release.

---

Nitpick comments:
In @.github/workflows/cli-performance.yaml:
- Line 18: Replace the mutable tag usage "dorny/paths-filter@v3" with a pinned
commit SHA for each occurrence (three places) so the action cannot change
unexpectedly; update the "uses: dorny/paths-filter@v3" lines to the form
"dorny/paths-filter@<commit-sha> # v3" (keep the “# v3” comment), ensuring you
pick the commit SHA that corresponds to the v3 release and apply the change
where the exact string "dorny/paths-filter@v3" appears.

In @.github/workflows/eval-regression.yaml:
- Around line 140-150: Add a defensive guard to the job conditional to prevent
running when the check-changes job failed: update the if expression that
currently uses !cancelled() and needs.check-changes.outputs.has_code_changes !=
'false' to also require needs.check-changes.result != 'failure' so that when
check-changes fails (not just skipped) the job will not run; reference the
existing symbols !cancelled(), needs.check-changes.outputs.has_code_changes and
add needs.check-changes.result != 'failure' into the combined boolean expression
controlling the job.

The workflow YAML files themselves were being detected as code changes,
causing evals/benchmarks/docker to run on PRs that only change CI config
or docs. CI config changes don't affect application behavior and don't
need eval/benchmark/docker validation.

https://claude.ai/code/session_01JGwBxWqzQyD3Wwd5K4HtV8
Signed-off-by: Claude <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (2)
.github/workflows/eval-regression.yaml (1)

56-62: Same .github/** exclusion note as in cli-performance.yaml.

Changes to this workflow file itself (or to .github/actions/ composite actions it depends on) won't be detected as code changes during PR runs. This is less critical here since workflow_dispatch provides an escape hatch, but worth being aware of — a PR that only modifies eval infrastructure will silently skip evals.

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

In @.github/workflows/eval-regression.yaml around lines 56 - 62, The branch
protection filter currently excludes changes under the ".github/**" pattern in
the workflow's "filters" block, causing edits to workflow files or composite
actions to be ignored by PR-triggered eval runs; remove the "'!.github/**'"
exclusion (or narrow it to only ignore unrelated files) so that modifications to
workflow YAMLs and composite actions are detected, ensuring the workflow (which
already has workflow_dispatch) runs on PRs that change the CI/eval
infrastructure.
.github/workflows/cli-performance.yaml (1)

11-27: The !.github/** exclusion will skip benchmarks even when the benchmark workflow itself changes.

If someone modifies this very workflow file (.github/workflows/cli-performance.yaml) or the benchmark script invocation, the filter will report no code changes and the benchmarks won't run — which is exactly when you'd want them to run.

Consider narrowing the exclusion or adding a carve-out:

Proposed filter adjustment
             code:
               - '**'
               - '!docs/**'
               - '!**/*.md'
               - '!mkdocs.yml'
-              - '!.github/**'
+              - '!.github/workflows/**'
+              - '.github/workflows/cli-performance.yaml'
+              - '.github/actions/**'

This way, changes to the benchmark workflow or composite actions still trigger a run, while unrelated workflow changes (eval-regression, docker, etc.) are excluded.

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

In @.github/workflows/cli-performance.yaml around lines 11 - 27, The
paths-filter in the check-changes job currently excludes the entire .github/**
pattern (see step id filter and the code filter block), which prevents changes
to this workflow file from being detected; update the code filter to stop
excluding all of .github/** and instead narrow the exclusion or add an explicit
include for this workflow (e.g., allow .github/workflows/cli-performance.yaml
and any benchmark scripts/composite actions you want to run) so edits to
.github/workflows/cli-performance.yaml (or related benchmark files) will trigger
the benchmarks; modify the filter block under id filter (the "code:" filter
rules) to remove or refine the '!.github/**' rule and add a positive include for
the specific workflow and benchmark paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In @.github/workflows/cli-performance.yaml:
- Around line 11-27: The paths-filter in the check-changes job currently
excludes the entire .github/** pattern (see step id filter and the code filter
block), which prevents changes to this workflow file from being detected; update
the code filter to stop excluding all of .github/** and instead narrow the
exclusion or add an explicit include for this workflow (e.g., allow
.github/workflows/cli-performance.yaml and any benchmark scripts/composite
actions you want to run) so edits to .github/workflows/cli-performance.yaml (or
related benchmark files) will trigger the benchmarks; modify the filter block
under id filter (the "code:" filter rules) to remove or refine the '!.github/**'
rule and add a positive include for the specific workflow and benchmark paths.

In @.github/workflows/eval-regression.yaml:
- Around line 56-62: The branch protection filter currently excludes changes
under the ".github/**" pattern in the workflow's "filters" block, causing edits
to workflow files or composite actions to be ignored by PR-triggered eval runs;
remove the "'!.github/**'" exclusion (or narrow it to only ignore unrelated
files) so that modifications to workflow YAMLs and composite actions are
detected, ensuring the workflow (which already has workflow_dispatch) runs on
PRs that change the CI/eval infrastructure.

Use first-party actions/github-script to check PR files via the GitHub
API instead of the third-party dorny/paths-filter action. No external
dependency needed - just lists PR files and checks if any are outside
docs/md/mkdocs.yml/.github paths.

https://claude.ai/code/session_01JGwBxWqzQyD3Wwd5K4HtV8
Signed-off-by: Claude <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
.github/workflows/cli-performance.yaml (1)

11-33: Consider extracting check-changes into a reusable workflow.

This exact job (lines 11–33) is duplicated verbatim in docker-dev-images.yaml (and likely eval-regression.yaml). A reusable workflow or a composite action would eliminate the duplication and ensure the filter logic stays consistent when the exclusion list evolves.

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

In @.github/workflows/cli-performance.yaml around lines 11 - 33, The duplicated
job "check-changes" (job id check-changes, step id check using
actions/github-script@v7) should be extracted into a reusable workflow or
composite action that outputs has_code_changes; create a new workflow/composite
(e.g., check-changes) that runs the github-script logic (including
core.setOutput('has_code_changes', ...)) and then replace the verbatim blocks in
this repo (e.g., cli-performance.yaml, docker-dev-images.yaml,
eval-regression.yaml) with a single "uses:
./.github/workflows/<new-workflow>.yaml" invocation passing the pull_request
context, ensuring the same output name has_code_changes is preserved so callers
remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/cli-performance.yaml:
- Around line 27-32: The current hasCodeChanges predicate excludes any file
under '.github/', so edits to this workflow itself will be treated as "no code
changes" and skip the benchmark; update the files.some condition (the line using
!f.filename.startsWith('.github/')) so that either (a) you remove that exclusion
entirely in this workflow so changes to workflows trigger the benchmark, or (b)
narrow it to only skip unrelated CI config by replacing the broad '.github/'
check with a more specific check (e.g., only skip other workflow files by
checking for the workflows subdirectory and excluding a specific filename
exception) so this workflow file is not ignored by hasCodeChanges.

---

Duplicate comments:
In @.github/workflows/docker-dev-images.yaml:
- Around line 13-35: This workflow duplicates the check-changes job/step logic
found elsewhere; extract the files.some(...) filtering logic currently used in
the check-changes job (step id "check", output "has_code_changes") into a single
reusable unit (either a reusable workflow or composite action) and call that
from both workflows instead of duplicating the block, and update the exclusion
predicate in the files.some(...) filter so it does not blanket-ignore the entire
".github/" directory (e.g., change !f.filename.startsWith('.github/') to a more
specific path like !f.filename.startsWith('.github/workflows/') or explicitly
allow Dockerfile/build-config paths).

---

Nitpick comments:
In @.github/workflows/cli-performance.yaml:
- Around line 11-33: The duplicated job "check-changes" (job id check-changes,
step id check using actions/github-script@v7) should be extracted into a
reusable workflow or composite action that outputs has_code_changes; create a
new workflow/composite (e.g., check-changes) that runs the github-script logic
(including core.setOutput('has_code_changes', ...)) and then replace the
verbatim blocks in this repo (e.g., cli-performance.yaml,
docker-dev-images.yaml, eval-regression.yaml) with a single "uses:
./.github/workflows/<new-workflow>.yaml" invocation passing the pull_request
context, ensuring the same output name has_code_changes is preserved so callers
remain unchanged.

Comment on lines +27 to +32
const hasCodeChanges = files.some(f =>
!f.filename.startsWith('docs/') &&
!f.filename.endsWith('.md') &&
f.filename !== 'mkdocs.yml' &&
!f.filename.startsWith('.github/')
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Excluding .github/ means changes to this workflow itself won't trigger the benchmark.

The filter on line 31 (!f.filename.startsWith('.github/')) causes modifications to this very workflow file to be treated as "no code changes," so the benchmark won't run. If someone refactors the benchmark script invocation or changes environment variables in this YAML, the workflow would silently skip itself.

If the intent is to only skip CI-config files unrelated to benchmarks, consider narrowing the exclusion (e.g., only skip .github/workflows/ files that aren't this one, or remove the .github/ exclusion from this workflow entirely).

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

In @.github/workflows/cli-performance.yaml around lines 27 - 32, The current
hasCodeChanges predicate excludes any file under '.github/', so edits to this
workflow itself will be treated as "no code changes" and skip the benchmark;
update the files.some condition (the line using
!f.filename.startsWith('.github/')) so that either (a) you remove that exclusion
entirely in this workflow so changes to workflows trigger the benchmark, or (b)
narrow it to only skip unrelated CI config by replacing the broad '.github/'
check with a more specific check (e.g., only skip other workflow files by
checking for the workflows subdirectory and excluding a specific filename
exception) so this workflow file is not ignored by hasCodeChanges.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (3)
.github/workflows/docker-dev-images.yaml (2)

15-37: The check-changes job is duplicated verbatim across all three workflow files.

Consider extracting this into a reusable workflow or a composite action to avoid maintaining identical logic in three places. A single check-code-changes reusable workflow returning has_code_changes would DRY this up.

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

In @.github/workflows/docker-dev-images.yaml around lines 15 - 37, Duplicate
check logic in the check-changes job (job name "check-changes", step id "check",
uses "actions/github-script@v7" producing output "has_code_changes" via
core.setOutput) should be moved into a single reusable workflow or composite
action named e.g. "check-code-changes"; create the reusable workflow that
accepts pull request context, runs the same github.rest.pulls.listFiles
pagination/filename filtering logic, sets and returns an output
"has_code_changes", then replace the duplicated job blocks in each workflow with
a call to the new reusable workflow (consuming its has_code_changes output).

6-7: paths-ignore and the JS filter overlap for .github/ — intentional belt-and-suspenders, but note the interaction.

paths-ignore: '.github/**' at the trigger level means the workflow won't fire at all for .github/-only PRs, so the JS .startsWith('.github/') exclusion on line 35 is only reached when non-.github/ files are also present. This is fine — just noting that the JS exclusion is effectively redundant for this workflow. No action needed unless you want to keep the JS filter strictly aligned with only the "docs" concern (removing the .github/ check from JS).

Also applies to: 29-36

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

In @.github/workflows/docker-dev-images.yaml around lines 6 - 7, The workflow
currently uses paths-ignore: '.github/**' at the trigger level while the JS
filter also checks filePath.startsWith('.github/') (the JS exclusion around the
`.startsWith('.github/')` check) — this makes the JS check redundant; to fix,
remove the `.startsWith('.github/')` branch from the JS filter (or alternatively
remove the top-level paths-ignore and keep the JS check) so the filtering logic
is consistent; update the JS predicate that uses `.startsWith('.github/')` (and
any related conditional around the "docs" concern) to reflect the chosen
single-source filtering approach.
.github/workflows/eval-regression.yaml (1)

148-159: The !cancelled() + != 'false' pattern is correct and necessary — but worth a brief inline comment for future maintainers.

The three-part condition works because:

  1. !cancelled() overrides GitHub Actions' default behavior of skipping jobs when a needs job is skipped (essential for non-PR triggers).
  2. != 'false' (rather than == 'true') allows the job to run when check-changes was skipped and output is empty.

This is non-obvious. A brief YAML comment explaining why != 'false' is used instead of == 'true' would help future maintainers.

💡 Suggested inline comment
     if: |
-      !cancelled() &&
-      needs.check-changes.outputs.has_code_changes != 'false' &&
+      !cancelled() &&
+      # != 'false' (not == 'true') so non-PR events pass through when check-changes is skipped
+      needs.check-changes.outputs.has_code_changes != 'false' &&
       (
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/eval-regression.yaml around lines 148 - 159, Add a short
inline YAML comment next to the llm_evals job's if condition explaining the two
non-obvious parts: note that !cancelled() is used to override GitHub Actions'
behavior of skipping downstream jobs when a needs job is skipped (important for
non-PR triggers), and that needs.check-changes.outputs.has_code_changes !=
'false' is intentionally used instead of == 'true' because the output can be
empty when check-changes was skipped—so != 'false' allows the job to run in that
case; place this comment adjacent to the existing if: expression referencing
!cancelled() and needs.check-changes.outputs.has_code_changes != 'false'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In @.github/workflows/cli-performance.yaml:
- Around line 29-34: The current files.some check (assigned to hasCodeChanges)
excludes any path starting with '.github/', which prevents changes to this
workflow file from triggering the benchmark; update the predicate so it does not
blanket-exclude the workflow itself—e.g., keep the existing exclusions but add
an exception for '.github/workflows/cli-performance.yaml' (or narrow the
exclusion to specific non-workflow subpaths) by adjusting the files.some
condition to allow that file through while still excluding other .github files.

---

Nitpick comments:
In @.github/workflows/docker-dev-images.yaml:
- Around line 15-37: Duplicate check logic in the check-changes job (job name
"check-changes", step id "check", uses "actions/github-script@v7" producing
output "has_code_changes" via core.setOutput) should be moved into a single
reusable workflow or composite action named e.g. "check-code-changes"; create
the reusable workflow that accepts pull request context, runs the same
github.rest.pulls.listFiles pagination/filename filtering logic, sets and
returns an output "has_code_changes", then replace the duplicated job blocks in
each workflow with a call to the new reusable workflow (consuming its
has_code_changes output).
- Around line 6-7: The workflow currently uses paths-ignore: '.github/**' at the
trigger level while the JS filter also checks filePath.startsWith('.github/')
(the JS exclusion around the `.startsWith('.github/')` check) — this makes the
JS check redundant; to fix, remove the `.startsWith('.github/')` branch from the
JS filter (or alternatively remove the top-level paths-ignore and keep the JS
check) so the filtering logic is consistent; update the JS predicate that uses
`.startsWith('.github/')` (and any related conditional around the "docs"
concern) to reflect the chosen single-source filtering approach.

In @.github/workflows/eval-regression.yaml:
- Around line 148-159: Add a short inline YAML comment next to the llm_evals
job's if condition explaining the two non-obvious parts: note that !cancelled()
is used to override GitHub Actions' behavior of skipping downstream jobs when a
needs job is skipped (important for non-PR triggers), and that
needs.check-changes.outputs.has_code_changes != 'false' is intentionally used
instead of == 'true' because the output can be empty when check-changes was
skipped—so != 'false' allows the job to run in that case; place this comment
adjacent to the existing if: expression referencing !cancelled() and
needs.check-changes.outputs.has_code_changes != 'false'.

@aantn aantn closed this Feb 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants