Uh oh!
There was an error while loading. Please reload this page.
ci: scope test workflows to the domains a change actually touches - #1271
Conversation
Replace the four copy-pasted 'sdk_swift_only' change-detection jobs with a shared reusable workflow (detect-changes.yml) that classifies changed files into rust / node / swift / inert domains and fails open (runs everything) when the scope is unknown. - rust-ci: cargo test/clippy/fmt/cross-compile now skip on TS-only, docs-only, and swift-only changes; the sdk-check TS job gates on node changes instead. - test, node-compat, package-validation: node jobs also skip docs-only changes (previously only swift-only). Rust changes still run the TS suite because pretest builds the broker from source. - security: npm-audit, CodeQL (js-ts), and license-check gate on node changes; secret scanning still runs on every PR. - e2e-tests now triggers on crates/** (the e2e script builds the broker); fleet-e2e and build-broker-binary now trigger on crates/relay-pty/** and Cargo.lock, which the broker binary depends on. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018xpYY8uoeGnxT3Fpt6XcNs
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCI workflows now share reusable changed-file classification, expose Rust/Node/Swift scope outputs, gate jobs accordingly, broaden Rust-related triggers, and build the root Swift manifest before Swift tests. ChangesCI scope detection and gating
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant GitHub
participant DetectChanges
participant CIJobs
GitHub->>DetectChanges: Invoke reusable workflow
DetectChanges->>DetectChanges: Fetch and classify changed files
DetectChanges-->>CIJobs: Return rust_changed and node_changed
CIJobs->>CIJobs: Run matching jobs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:0e323d0e75
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
All reported issues were addressed across 9 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…wift build - Fail open when the paginated pulls.listFiles result hits GitHub's 3000-file cap, matching the push-path compareCommits handling. - Classify the tracked .agents/ skills directory as inert (nothing in CI consumes it; the skills installer only writes such paths in target projects). - Build the root Package.swift in the Swift SDK job: git-URL consumers resolve the root manifest, which no job validated before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018xpYY8uoeGnxT3Fpt6XcNs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/detect-changes.yml:
- Around line 16-18: Add explicit read-only permissions to the reusable
workflow’s top-level on.workflow_call configuration: grant contents read for
compareCommits and pull-requests read for listFiles, ensuring the workflow token
does not inherit broader caller permissions.
- Around line 53-67: Update the changed-file collection in both the `listFiles`
and `compareCommits` branches to include each record’s `previous_filename`
alongside `filename`, so renamed files classify both old and new paths. Keep
`unknownScope` cap checks based on the number of API records, not the expanded
path list length.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f26f15c-4136-4505-83d5-7ec9370c0aa9
📒 Files selected for processing (9)
.github/workflows/build-broker-binary.yml.github/workflows/detect-changes.yml.github/workflows/e2e-tests.yml.github/workflows/fleet-e2e.yml.github/workflows/node-compat.yml.github/workflows/package-validation.yml.github/workflows/rust-ci.yml.github/workflows/security.yml.github/workflows/test.yml
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Include previous_filename when collecting changed paths so a rename out of a domain (e.g. crates/foo.rs -> docs/foo.md) still triggers that domain's jobs; keep truncation-cap checks on the raw API record count. Declare explicit read-only permissions on the reusable workflow instead of inheriting the caller's defaults. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018xpYY8uoeGnxT3Fpt6XcNs
There was a problem hiding this comment.
All reported issues were addressed across 9 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Uh oh!
There was an error while loading. Please reload this page.
tests/mcp_merge_e2e.rs lives outside crates/, so a change to it skipped the Rust suite; match .rs files anywhere plus .cargo/ and rust-toolchain files. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018xpYY8uoeGnxT3Fpt6XcNs
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Uh oh!
There was an error while loading. Please reload this page.
A .rs code example under docs/ or specs/ matched the new suffix rule and ran the full Rust CI; exclude inert paths from the Rust predicate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018xpYY8uoeGnxT3Fpt6XcNs
Summary
CI currently runs the full Rust suite (cargo test debug+release, clippy, cross-compile on Ubuntu + macOS) on TS-only PRs, and runs everything on docs-only PRs. The only existing filter was a copy-pasted "skip if swift-only" job in four workflows. This PR replaces that with a shared reusable workflow,
.github/workflows/detect-changes.yml, that classifies changed files into rust / node / swift / inert domains, and gates each workflow's jobs on the domain it actually depends on:rust-ci.yml— cargo test/clippy/fmt/cross-compile skip on TS-only, docs-only, and swift-only changes; thesdk-checkTypeScript job gates on node changes instead of rust.test.yml,node-compat.yml,package-validation.yml— node jobs now also skip docs-only changes (previously only swift-only). Rust changes still run the TS suite deliberately:npm test'spretesthook runsbuild:rust, so the TS tests exercise a broker built from the PR's Rust sources.security.yml— npm-audit, CodeQL (javascript-typescript), and license-check gate on node changes; secret scanning still runs on every PR since any file can leak a secret.swift build && swift test.Detection fails open: on API errors, truncated file lists (compareCommits' 300-file cap), or unexpected events, all domains report changed and everything runs — a skip is never the result of an error. Jobs skipped via
if:report "skipped", which satisfies required status checks (same mechanism as the existing swift-only gate).Also fixes three under-triggering gaps in the opposite direction:
e2e-tests.ymlbuilds the broker (npm run build→build:rust) but never triggered oncrates/**changes.fleet-e2e.ymlandbuild-broker-binary.ymlwatchedcrates/broker/**but notcrates/relay-pty/**(a workspace crate the broker depends on) orCargo.lock— a relay-pty change on main could ship a stale prebuilt binary.Note: the change-scope check now reports as "changes / Detect change scope" (reusable workflow naming). If branch protection required the old "Detect change scope" name, that entry needs updating; all test/lint job names are unchanged.
Test Plan
prettier --checkchangesjob output on this PR (workflow-file changes classify as all-domains, so every suite should run here)Screenshots
N/A
🤖 Generated with Claude Code
https://claude.ai/code/session_018xpYY8uoeGnxT3Fpt6XcNs
Generated by Claude Code