Skip to content

Conversation

@LinZhihao-723
Copy link
Member

@LinZhihao-723 LinZhihao-723 commented Oct 22, 2025

Description

This PR fixes #1463 by adding a filter to skip Rust workflows when no Rust-related changes are involved in a push or a commit.
The implementation of this PR should resolve #1464.

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

Summary by CodeRabbit

  • Chores
    • Enhanced CI workflow to limit Rust checks to a defined set of monitored paths, reducing unnecessary runs.
    • Ensured both pull-request and push events reference the same shared path filter for consistent triggering.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 22, 2025

Walkthrough

Added path filtering to the clp-rust-checks GitHub Actions workflow by introducing a monitored_paths anchor and applying it to pull_request and push triggers (push now references the anchor via *monitored_paths).

Changes

Cohort / File(s) Change Summary
CI workflow
​.github/workflows/clp-rust-checks.yaml
Introduced a monitored_paths YAML anchor listing Rust-related and CI files/paths; applied this anchor to pull_request path filters and referenced it from the push trigger via *monitored_paths.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant GH as GitHub Actions
  participant Events as Repo events (push/pr)
  participant Workflow as clp-rust-checks workflow

  Note over Events,GH: Change to workflow triggers
  Events->>GH: push / pull_request with file changes
  GH->>Workflow: evaluate path filters (uses `monitored_paths` anchor)
  alt changes match monitored_paths
    GH->>Workflow: trigger workflow run
  else no matching changes
    GH-->>Events: skip workflow
  end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly summarizes the main change of running Rust workflows only when relevant code is modified and notes the lock file update, concisely reflecting the core workflow filter addition and Cargo.lock bump in line with the PR objectives.
Linked Issues Check ✅ Passed The PR adds path filters in the clp-rust-checks workflow to skip runs when no Rust files change, fulfilling issue #1464, and updates Cargo.lock to address the lock file validation step from issue #1463, satisfying both linked issue objectives.
Out of Scope Changes Check ✅ Passed All changes are confined to the GitHub Actions workflow configuration and Cargo.lock update as specified by the linked issues, with no unrelated or extraneous modifications introduced.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

@LinZhihao-723 LinZhihao-723 changed the title fix(ci): Skip Rust workflows when no Rust-related chanages are involved (fixes #1463) (resolves #1464). fix(ci): Skip Rust workflows when no Rust-related chanages are involved (fixes #1463) (resolves #1464); Update Cargo.lock. Oct 22, 2025
@LinZhihao-723 LinZhihao-723 changed the title fix(ci): Skip Rust workflows when no Rust-related chanages are involved (fixes #1463) (resolves #1464); Update Cargo.lock. fix(ci): Skip Rust workflows when no Rust-related chanages are involved (fixes #1463) (resolves #1464); Update Cargo lock file. Oct 22, 2025
@LinZhihao-723 LinZhihao-723 marked this pull request as ready for review October 22, 2025 17:24
@LinZhihao-723 LinZhihao-723 requested a review from a team as a code owner October 22, 2025 17:24
@junhaoliao junhaoliao self-requested a review October 22, 2025 17:37
junhaoliao
junhaoliao previously approved these changes Oct 22, 2025
Copy link
Member

@junhaoliao junhaoliao left a comment

Choose a reason for hiding this comment

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

i posted some questions about reducing the range of files to be checked. otherwise the PR lgtm.

for the title, how about:

fix(ci): Run Rust workflows only when changes affect Rust code (fixes #1463, resolves #1464); Update Cargo lock file.

- "components/log-ingestor/**"
- "taskfile.yaml"
- "taskfiles/**"
- "tools/scripts/deps-download/**"
Copy link
Member

Choose a reason for hiding this comment

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

do we really need this?

- "Cargo.lock"
- "Cargo.toml"
- ".github/workflows/clp-rust-checks.yaml"
- ".gitmodules"
Copy link
Member

Choose a reason for hiding this comment

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

and this? cuz yscope-dev-utils' version is not tracked there i believe?

@LinZhihao-723 LinZhihao-723 changed the title fix(ci): Skip Rust workflows when no Rust-related chanages are involved (fixes #1463) (resolves #1464); Update Cargo lock file. fix(ci): Run Rust workflows only when changes affect Rust code (fixes #1463, resolves #1464); Update Cargo lock file. Oct 22, 2025
@LinZhihao-723
Copy link
Member Author

Removed unrelated.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 59a9172 and 41722a3.

📒 Files selected for processing (1)
  • .github/workflows/clp-rust-checks.yaml (1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.8)
.github/workflows/clp-rust-checks.yaml

15-15: "paths" section must be sequence node but got alias node with "" tag

(syntax-check)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: package-image
  • GitHub Check: lint-check (macos-15)
  • GitHub Check: lint-check (ubuntu-24.04)
  • GitHub Check: rust-checks (ubuntu-24.04)
  • GitHub Check: rust-checks (macos-15)
  • GitHub Check: rust-checks (ubuntu-22.04)

Comment on lines 14 to +15
push:
paths: *monitored_paths
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 22, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Fix actionlint failure on the push trigger.
Actionlint 1.7.8 rejects paths: *monitored_paths because it resolves to an alias node instead of a sequence, so the workflow currently fails validation. Please expand the list inline (or otherwise restructure without aliases) for the push trigger so that paths is a literal sequence.

🧰 Tools
🪛 actionlint (1.7.8)

15-15: "paths" section must be sequence node but got alias node with "" tag

(syntax-check)

🤖 Prompt for AI Agents
In .github/workflows/clp-rust-checks.yaml around lines 14 to 15, the push
trigger currently uses an alias reference "paths: *monitored_paths" which
actionlint rejects because it expects a literal sequence; replace the alias with
an inline literal sequence of the monitored paths (i.e., copy the actual list
items from the monitored_paths anchor into the push.paths section) or otherwise
restructure so push.paths is a plain YAML sequence node rather than an alias.

Copy link
Member

Choose a reason for hiding this comment

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

it's a bug (outdated type annotation) in actionlint then, since GH has added YAML anchor support. see rhysd/actionlint#133 which tracks the bug

Copy link
Contributor

Choose a reason for hiding this comment

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

@junhaoliao Thanks, and agreed with @junhaoliao. GitHub Actions now supports YAML anchors and aliases in workflows, so the alias used for push.paths is valid; this is a limitation in actionlint rather than a workflow issue. (docs.github.com)

I’ll treat the previous actionlint note as a tooling false positive and not request changes. If you’d still like to quiet the noise until actionlint catches up, here’s a safe, temporary inline alternative for push only (functionally identical):

on:
  pull_request:
    paths: &monitored_paths
      - ".cargo/**"
      - "Cargo.lock"
      - "Cargo.toml"
      - ".github/workflows/clp-rust-checks.yaml"
      - "components/clp-rust-utils/**"
      - "components/log-ingestor/**"
      - "taskfile.yaml"
      - "taskfiles/**"
  push:
    paths:
      - ".cargo/**"
      - "Cargo.lock"
      - "Cargo.toml"
      - ".github/workflows/clp-rust-checks.yaml"
      - "components/clp-rust-utils/**"
      - "components/log-ingestor/**"
      - "taskfile.yaml"
      - "taskfiles/**"

Otherwise, I’m good to keep the anchor as-is and ignore the linter’s complaint for this case. Also noting that actionlint 1.7.8 is the latest as of October 11, 2025 and doesn’t mention explicit anchor handling in its release notes, which likely explains the current behavior. (github.com)


✏️ Learnings added
Learnt from: junhaoliao
PR: y-scope/clp#1466
File: .github/workflows/clp-rust-checks.yaml:14-15
Timestamp: 2025-10-22T21:14:12.178Z
Learning: Repository y-scope/clp: In GitHub Actions workflows (e.g., .github/workflows/clp-rust-checks.yaml), YAML anchors/aliases are acceptable and preferred to avoid duplication; if actionlint flags an alias node (e.g., on push.paths) as an error, treat it as a tool limitation and do not require inlining unless the team asks to silence the warning.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: junhaoliao
PR: y-scope/clp#0
File: :0-0
Timestamp: 2025-10-22T21:02:31.068Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).

@junhaoliao
Copy link
Member

junhaoliao commented Oct 22, 2025

As discussed offline, we should investigate how to verify that the Rust Cargo.lock file is in sync with Cargo.toml without updating dependencies, in order to maintain deterministic builds / CI runs.

That should be done in a follow-up PR.

@coderabbitai create an issue to track the request

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 22, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@junhaoliao On it — I’m creating a follow-up issue with the details, assigning it to you, and linking back to PR #1466 and your comment. I’ll share the issue link here as soon as it’s created.


🏁 Script executed:

#!/bin/bash
set -euo pipefail

REPO="y-scope/clp"
PR_NUMBER=1466
PR_URL="https://github.com/$REPO/pull/$PR_NUMBER"

# Try to find the specific comment URL from @junhaoliao on the PR.
NEEDLE_REGEX='Cargo\.lock.*in sync.*Cargo\.toml'
comment_url=""

# Search PR issue comments (timeline comments)
if gh api -H "Accept: application/vnd.github+json" "/repos/$REPO/issues/$PR_NUMBER/comments" > /tmp/issue_comments.json; then
  comment_url=$(jq -r --arg re "$NEEDLE_REGEX" '
    map(select((.user.login|ascii_downcase)=="junhaoliao")) |
    map(select(.body|test($re; "i"))) |
    .[0].html_url // empty
  ' /tmp/issue_comments.json || true)
fi

# If not found, search PR review comments (diff comments)
if [ -z "$comment_url" ]; then
  if gh api -H "Accept: application/vnd.github+json" "/repos/$REPO/pulls/$PR_NUMBER/comments" > /tmp/review_comments.json; then
    comment_url=$(jq -r --arg re "$NEEDLE_REGEX" '
      map(select((.user.login|ascii_downcase)=="junhaoliao")) |
      map(select(.body|test($re; "i"))) |
      .[0].html_url // empty
    ' /tmp/review_comments.json || true)
  fi
fi

# Fallback to PR URL if we couldn't resolve the exact comment URL
if [ -z "$comment_url" ]; then
  comment_url="$PR_URL"
fi

TITLE="CI: Verify Cargo.lock sync with Cargo.toml without updating dependencies"

BODY=$(cat <<EOF
Context
- From discussion in PR #$PR_NUMBER (link below). We want to verify that the Rust Cargo.lock file is in sync with Cargo.toml without updating dependencies, to maintain deterministic builds and CI runs.

Problem
- The current "Validate lock files" step in clp-rust-checks may perform an update or otherwise mutate the lockfile, which undermines deterministic behaviour and creates noisy diffs.

Goals
- Determine a reliable, non-mutating way to fail CI when Cargo.lock is out-of-sync with Cargo.toml.
- Keep the step fast and side-effect free.

Ideas to evaluate
- Use "cargo metadata --locked" (fast, no build; fails if the lockfile needs updates).
- Use "cargo check --locked" (compiles; also fails if the lockfile needs updates).
- Use "cargo fetch --locked" (network-only; fails if the lockfile is missing/out-of-date).
- Ensure all workspace members are covered.

Acceptance criteria
- A documented command/script that exits non-zero when Cargo.lock is not in sync with Cargo.toml and never modifies files.
- CI updated to run this command as part of clp-rust-checks.
- Taskfile target (e.g., deps:lock:check-rust) updated accordingly.
- Works for both PRs and direct pushes.

References
- PR: $PR_URL
- Request comment: $comment_url
- Related: #1463, #1464
EOF
)

payload=$(jq -n --arg title "$TITLE" --arg body "$BODY" --arg assignee "junhaoliao" '{title: $title, body: $body, assignees: [$assignee]}')

response=$(gh api -X POST -H "Accept: application/vnd.github+json" "/repos/$REPO/issues" --input - <<< "$payload")

ISSUE_NUMBER=$(echo "$response" | jq -r '.number')
ISSUE_URL=$(echo "$response" | jq -r '.html_url')

echo "created_issue_number=$ISSUE_NUMBER"
echo "created_issue_url=$ISSUE_URL"

Length of output: 86


@junhaoliao Issue created: #1470. It’s assigned to you and includes backlinks to PR #1466 and your request. The issue outlines goals, non-mutating approaches to evaluate (e.g., cargo metadata --locked / cargo check --locked / cargo fetch --locked), and acceptance criteria for updating clp-rust-checks.

Would you like me to add any labels or a milestone, or adjust the assignee?

Suggested first experiment for the follow-up PR:

  • Add a CI step at the workspace root:
    • run: cargo metadata --locked --format-version 1 > /dev/null
      This should fail fast if Cargo.lock is out of sync without modifying files.

✏️ Learnings added
Learnt from: junhaoliao
URL: https://github.com/y-scope/clp/pull/1466

Timestamp: 2025-10-22T21:01:31.358Z
Learning: In y-scope/clp, for the clp-rust-checks workflow (GitHub Actions) and the Taskfile target deps:lock:check-rust, we should verify Rust Cargo.lock is in sync with Cargo.toml using a non-mutating method (e.g., cargo metadata --locked / cargo check --locked) to keep CI deterministic and avoid updating dependencies during validation.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 22, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@junhaoliao Understood—creating a tracking issue now with context, acceptance criteria, and assigning it to you. I’ll include backlinks to PR #1466 and your comment.


🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

owner="y-scope"
repo="clp"
pr_number="1466"
requester="junhaoliao"

# Phrase to search for within the PR comments (issue and review comments)
needle="verify that the Rust Cargo.lock file is in sync with Cargo.toml"

# Ensure required tools exist
command -v gh >/dev/null 2>&1 || { echo "GitHub CLI (gh) is required."; exit 1; }
command -v jq >/dev/null 2>&1 || { echo "jq is required."; exit 1; }

# Fetch both issue (conversation) comments and review comments, then try to locate the specific comment
issue_comments_json="$(gh api "repos/${owner}/${repo}/issues/${pr_number}/comments?per_page=100")"
review_comments_json="$(gh api "repos/${owner}/${repo}/pulls/${pr_number}/comments?per_page=100")"

comment_url="$(
  jq -r --arg needle "$needle" '
    def findmatch:
      [ .[] | select(.body // "" | contains($needle)) ]
      | sort_by(.created_at) | last // empty;
    [inputs] | add | findmatch | .html_url // empty
  ' <(echo "$issue_comments_json") <(echo "$review_comments_json")
)"

pr_url="https://github.com/${owner}/${repo}/pull/${pr_number}"
if [[ -z "${comment_url}" ]]; then
  # Fallback to PR URL if the specific comment was not located via API
  comment_url="$pr_url"
fi

issue_title="CI: Verify Cargo.lock is in sync with Cargo.toml without updating dependencies"

body_file="$(mktemp)"
cat > "$body_file" <<EOF
Objective
- Investigate and implement a non-mutating verification that ensures Rust Cargo.lock is in sync with Cargo.toml across the workspace, preserving deterministic CI runs and builds.

Context
- Requested by @${requester} in PR #${pr_number}.
- Discussion comment: ${comment_url}

Motivation
- Avoid non-deterministic behaviour caused by implicit lockfile updates during CI, and surface drift between manifests and the lockfile early.

Requirements
- The CI step must fail if Cargo.lock is out-of-date or inconsistent with the manifest(s), without modifying any files.
- Must work across the full Rust workspace (e.g., components/clp-rust-utils, components/log-ingestor).
- Should be fast and safe to run in CI without producing build artefacts.

Approaches to evaluate (non-exhaustive)
- Use \`cargo metadata --locked\` at the workspace root (and/or per member) to error when the lockfile is stale.
- Use \`cargo fetch --locked\` to validate resolution without compiling.
- Consider \`cargo check --locked -q\` as a fallback if needed.
- Explore \`cargo tree --locked\` for dependency resolution verification without mutation.

Definition of Done
- A dedicated step in .github/workflows/clp-rust-checks.yaml performs this verification and deterministically fails when Cargo.lock is not in sync, without altering repository files.
- Developer docs (e.g., CONTRIBUTING.md or Taskfile) updated to explain how to run the check locally.

Backlinks
- PR: ${pr_url}
- Comment permalink: ${comment_url}
- Related: #1463, #1464, PR #1448

Assignee
- @${requester}
EOF

# Create the issue and output its URL
gh issue create \
  --repo "${owner}/${repo}" \
  --title "${issue_title}" \
  --assignee "${requester}" \
  --body-file "${body_file}"

Length of output: 137


@junhaoliao Thanks for the heads-up. The earlier failure was due to process substitution not being available in the runner. Here’s a more portable script that uses temp files and should create the tracking issue, assign it to you, and include backlinks to PR #1466 and your comment.


🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

owner="y-scope"
repo="clp"
pr_number="1466"
requester="junhaoliao"

# Ensure required tools exist
for cmd in gh jq grep mktemp; do
  command -v "$cmd" >/dev/null 2>&1 || { echo "Required tool missing: $cmd"; exit 1; }
done

pr_url="https://github.com/${owner}/${repo}/pull/${pr_number}"

# Fetch PR conversation comments and review comments into temp files (no process substitution)
issue_tmp="$(mktemp)"; trap 'rm -f "$issue_tmp"' EXIT
review_tmp="$(mktemp)"; trap 'rm -f "$review_tmp"' EXIT

gh api "repos/${owner}/${repo}/issues/${pr_number}/comments?per_page=100" > "$issue_tmp"
gh api "repos/${owner}/${repo}/pulls/${pr_number}/comments?per_page=100" > "$review_tmp"

# Find the most recent comment mentioning both Cargo.lock and Cargo.toml (case-insensitive)
comment_url="$(
  jq -r '
    add
    | [ .[] | select((.body // "") | test("Cargo\\.lock"; "i") and test("Cargo\\.toml"; "i")) ]
    | sort_by(.created_at)
    | (last // {}) 
    | (.html_url // empty)
  ' -s "$issue_tmp" "$review_tmp"
)"

if [[ -z "${comment_url}" ]]; then
  comment_url="$pr_url"
fi

issue_title="CI: Verify Cargo.lock is in sync with Cargo.toml without updating dependencies"

body_file="$(mktemp)"; trap 'rm -f "$body_file"' EXIT
cat > "$body_file" <<'EOF'
Objective
- Investigate and implement a non-mutating verification that ensures Rust Cargo.lock is in sync with Cargo.toml across the workspace, preserving deterministic CI runs and builds.

Motivation
- Avoid non-deterministic behaviour caused by implicit lockfile updates during CI, and surface drift between manifests and the lockfile early.

Requirements
- The CI step must fail if Cargo.lock is out-of-date or inconsistent with the manifest(s), without modifying any files.
- Must work across the full Rust workspace (e.g., components/clp-rust-utils, components/log-ingestor).
- Should be fast and safe to run in CI without producing build artefacts.

Approaches to evaluate (non-exhaustive)
- Use `cargo metadata --locked` at the workspace root (and/or per member) to error when the lockfile is stale.
- Use `cargo fetch --locked` to validate resolution without compiling.
- Consider `cargo check --locked -q` as a fallback if needed.
- Explore `cargo tree --locked` for dependency resolution verification without mutation.

Definition of Done
- A dedicated step in .github/workflows/clp-rust-checks.yaml performs this verification and deterministically fails when Cargo.lock is not in sync, without altering repository files.
- Developer docs (e.g., CONTRIBUTING.md or Taskfile) updated to explain how to run the check locally.
EOF

# Append backlinks and assignee context (separate from the <<'EOF' literal block)
{
  echo
  echo "Backlinks"
  echo "- PR: ${pr_url}"
  echo "- Comment permalink: ${comment_url}"
  echo "- Related: #1463, #1464, PR #1448"
  echo
  echo "Assignee"
  echo "- @${requester}"
} >> "$body_file"

# Create the issue
create_output="$(gh issue create \
  --repo "${owner}/${repo}" \
  --title "${issue_title}" \
  --assignee "${requester}" \
  --body-file "${body_file}" || true)"

# Try to extract the created issue URL; if creation failed, surface the error
issue_url="$(printf '%s\n' "$create_output" | grep -Eo 'https?://github\.com/[^ ]+/issues/[0-9]+' | tail -n1 || true)"

if [[ -n "$issue_url" ]]; then
  echo "Created issue: $issue_url"
  echo "Backlinks used:"
  echo "  - PR: ${pr_url}"
  echo "  - Comment: ${comment_url}"
else
  echo "Failed to create issue. Output from gh:"
  printf '%s\n' "$create_output"
  exit 1
fi

Length of output: 200


@junhaoliao Tracking issue created and assigned to you: #1471. It includes backlinks to PR #1466 and your comment permalink.

Would you like me to:

  • add labels/milestone or attach it to a project board, and
  • draft a follow-up PR with a CI step using cargo metadata --locked and/or cargo fetch --locked (non-mutating) to fail when Cargo.lock is out of sync?

Happy to proceed either in this repo or your fork—your preference.


✏️ Learnings added
Learnt from: junhaoliao
URL: https://github.com/y-scope/clp/pull/1466

Timestamp: 2025-10-22T21:02:31.068Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

@LinZhihao-723 LinZhihao-723 merged commit 8e77a81 into y-scope:main Oct 22, 2025
23 checks passed
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.

CI: Skip Rust checks workflow when no Rust-related changes are detected CI: Rust lock file validation step failing in clp-rust-checks workflow

2 participants