Conversation
Run tests/ref/ on every push/PR that modifies tops/cpu/ or tests/ref/, ensuring CPU reference kernels stay correct. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdded two GitHub Actions workflows (API coverage and CPU reference tests), a SkyPilot GPU job configuration, and a Python script that discovers public interfaces in Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Runner as Ubuntu Runner
participant GCP as GCP (Workload Identity)
participant Sky as SkyPilot
participant GPU as GPU Instance
participant Repo as Repository (code/tests)
GH->>Runner: trigger job (push/PR)
Runner->>Repo: checkout
Runner->>Runner: setup Python 3.12
alt API coverage job
Runner->>Repo: run `scripts/check_test_coverage.py`
Runner->>Runner: parse tops/ops, scan tests, report pass/miss
end
alt CPU ref tests job (path-filtered)
Runner->>GCP: authenticate using workload identity
Runner->>Sky: `sky launch .github/ci/cpu-ref-tests-gpu.sky.yaml`
Sky->>GCP: provision GPU instance
Sky->>GPU: provision resources & run setup (install deps)
GPU->>GPU: run `pytest tests/ref/ -v`
Sky->>Runner: return results
Runner->>Sky: `sky down` (always)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces a new script, scripts/check_test_coverage.py, which automates the verification of test coverage for public APIs in the tops/ops/ directory by parsing AST for exported symbols and searching for references in the tests/ directory. The review feedback suggests optimizing the search performance by combining individual regex patterns into a single combined pattern. Additionally, it is recommended to use the more idiomatic tree.body for AST traversal instead of ast.iter_child_nodes(tree) to improve code clarity and follow standard practices.
| patterns = { | ||
| sym: re.compile(r"\b" + re.escape(sym) + r"\b") for sym in symbols | ||
| } | ||
| references: dict[str, list[str]] = {sym: [] for sym in symbols} | ||
|
|
||
| for py_file in sorted(tests_dir.rglob("*.py")): | ||
| rel = py_file.relative_to(tests_dir) | ||
| if rel.parts and rel.parts[0] == "src": | ||
| continue | ||
|
|
||
| content = py_file.read_text(encoding="utf-8") | ||
| for sym, pattern in patterns.items(): | ||
| if pattern.search(content): | ||
| references[sym].append(str(py_file)) |
There was a problem hiding this comment.
当前 find_test_references 函数的实现效率较低。它为每个符号都编译一个正则表达式,然后遍历每个测试文件,并对文件内容逐一尝试匹配所有符号的正则表达式。当符号数量较多时,这会导致大量的重复操作和性能开销。
为了优化,建议将所有符号合并成一个单一的、更复杂的正则表达式。这样,对于每个文件,只需进行一次正则搜索即可找到所有被引用的符号。这种方法可以显著提高脚本的执行速度,尤其是在 CI 环境中。
if not symbols:
return {}
references: dict[str, list[str]] = {sym: [] for sym in symbols}
combined_pattern = re.compile("\\b(" + "|".join(map(re.escape, symbols)) + ")\\b")
for py_file in sorted(tests_dir.rglob("*.py")):
rel = py_file.relative_to(tests_dir)
if rel.parts and rel.parts[0] == "src":
continue
content = py_file.read_text(encoding="utf-8")
for sym in set(combined_pattern.findall(content)):
references[sym].append(str(py_file))|
|
||
| def _extract_dunder_all(tree: ast.Module) -> list[str]: | ||
| """从 AST 中提取 __all__ 列表中的字符串常量。""" | ||
| for node in ast.iter_child_nodes(tree): |
There was a problem hiding this comment.
| def _extract_reexports(tree: ast.Module) -> list[str]: | ||
| """提取 'from .mod import name as name' 风格的显式 re-export 符号。""" | ||
| names = [] | ||
| for node in ast.iter_child_nodes(tree): |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/check_test_coverage.py (1)
106-121: Consider: Regex matching may produce false positives.The word-boundary regex
\b{sym}\bwill match symbol occurrences in comments, docstrings, or string literals—not just actual imports or function calls. This could report coverage for a symbol that's merely mentioned in a comment.For a CI gate, false positives (incorrectly claiming coverage) are more problematic than false negatives. Consider strengthening the detection by also checking for import patterns:
# Example: also check for actual imports import_pattern = re.compile(rf"(?:from\s+\S+\s+import\s+.*\b{re.escape(sym)}\b|import\s+.*\b{re.escape(sym)}\b)")That said, for an initial implementation, the current approach is reasonable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/check_test_coverage.py` around lines 106 - 121, The current word-boundary regex in the loop (variables: patterns, pattern, sym, references) yields false positives from comments/strings; update the detection to also verify real code usage by either parsing the file with the ast module to look for Import, ImportFrom, Name, Attribute or Call nodes that reference sym, or (if you prefer a simpler change) add an additional import-specific regex (e.g., matching "from ... import sym" or "import ... sym") and require either the import regex or the original word-boundary match before appending to references[sym]; perform this check inside the same py_file loop where pattern.search(content) is done so references is only appended when a concrete import/AST usage is found.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/check_test_coverage.py`:
- Around line 106-121: The current word-boundary regex in the loop (variables:
patterns, pattern, sym, references) yields false positives from
comments/strings; update the detection to also verify real code usage by either
parsing the file with the ast module to look for Import, ImportFrom, Name,
Attribute or Call nodes that reference sym, or (if you prefer a simpler change)
add an additional import-specific regex (e.g., matching "from ... import sym" or
"import ... sym") and require either the import regex or the original
word-boundary match before appending to references[sym]; perform this check
inside the same py_file loop where pattern.search(content) is done so references
is only appended when a concrete import/AST usage is found.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2e1d996-a2fd-4a5b-9210-891a06ba42c1
📒 Files selected for processing (3)
.github/workflows/check_coverage.yml.github/workflows/cpu_ref_tests.ymlscripts/check_test_coverage.py
Replace ubuntu-latest with SkyPilot-launched GCP L4 GPU so that GPU comparison tests (CPU ref vs FLA Triton) actually run instead of being skipped. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/cpu_ref_tests.yml:
- Around line 4-13: The workflow trigger is missing the delegated CI file, so
changes to the delegated pipeline won't run this check; update the workflow
triggers in cpu_ref_tests.yml to include the delegated CI file(s) (at least the
file named cpu-ref-tests-gpu.sky.yaml or the .github/ci/** folder) so edits to
the delegated CI wiring cause the workflow to run; modify the push and
pull_request paths blocks to add those paths alongside "tops/cpu/**" and
"tests/ref/**".
- Around line 46-52: The step with id "cluster" currently interpolates
github.head_ref/github.ref_name directly into the run script, which lets
attacker-controlled branch names be executed; change the step to pass those
values via env: (e.g., set GITHUB_HEAD_REF: ${{ github.head_ref }} and
GITHUB_REF_NAME: ${{ github.ref_name }}) and then inside the run block read them
from the environment (e.g., BRANCH="${GITHUB_HEAD_REF:-$GITHUB_REF_NAME}")
before sanitizing and building NAME; this removes direct shell interpolation of
workflow expressions and prevents branch names like `foo$(...)` from being
executed.
- Around line 20-25: The cpu_ref_tests job is failing for forked PRs because
secrets.GCP_WORKLOAD_IDENTITY_PROVIDER and secrets.GCP_SERVICE_ACCOUNT are
unavailable; add a same-repo guard to the job so it only runs for PRs
originating from the same repository. Update the cpu_ref_tests job (named "CPU
ref tests (GPU via SkyPilot)") to include an if condition that ensures either
the event is not a pull_request or, when it is, that
github.event.pull_request.head.repo.full_name equals github.repository so the
google-github-actions/auth@v2 step gets valid secrets.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26cfd92e-ea1f-47b0-a01a-bc05590f3d95
📒 Files selected for processing (2)
.github/ci/cpu-ref-tests-gpu.sky.yaml.github/workflows/cpu_ref_tests.yml
✅ Files skipped from review due to trivial changes (1)
- .github/ci/cpu-ref-tests-gpu.sky.yaml
| push: | ||
| branches: [main] | ||
| paths: | ||
| - "tops/cpu/**" | ||
| - "tests/ref/**" | ||
| pull_request: | ||
| branches: [main] | ||
| paths: | ||
| - "tops/cpu/**" | ||
| - "tests/ref/**" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check if the files exist
ls -la .github/workflows/cpu_ref_tests.yml 2>/dev/null && echo "✓ Workflow file exists" || echo "✗ Workflow file missing"
ls -la .github/ci/cpu-ref-tests-gpu.sky.yaml 2>/dev/null && echo "✓ SkyPilot config exists" || echo "✗ SkyPilot config missing"
# List files in these directories
echo -e "\n=== Files in .github/workflows/ ==="
ls -1 .github/workflows/ 2>/dev/null | head -20
echo -e "\n=== Files in .github/ci/ ==="
ls -1 .github/ci/ 2>/dev/null | head -20Repository: primatrix/pallas-kernel
Length of output: 473
🏁 Script executed:
#!/bin/bash
# Read the workflow file
echo "=== .github/workflows/cpu_ref_tests.yml ==="
cat -n .github/workflows/cpu_ref_tests.ymlRepository: primatrix/pallas-kernel
Length of output: 2228
🏁 Script executed:
#!/bin/bash
# Read the SkyPilot config file to check if it exists and what's in lines 10-24
if [ -f ".github/ci/cpu-ref-tests-gpu.sky.yaml" ]; then
echo "=== .github/ci/cpu-ref-tests-gpu.sky.yaml (lines 1-30) ==="
head -30 .github/ci/cpu-ref-tests-gpu.sky.yaml | cat -n
else
echo "File .github/ci/cpu-ref-tests-gpu.sky.yaml not found"
echo "Searching for similar files..."
find .github -name "*.sky.yaml" -o -name "*cpu*ref*" 2>/dev/null
fiRepository: primatrix/pallas-kernel
Length of output: 872
🏁 Script executed:
#!/bin/bash
# Search for any references to the SkyPilot file in the workflow or other CI files
echo "=== References to 'cpu-ref-tests' or '.sky.yaml' in workflows ==="
rg -i "cpu-ref-tests|sky.yaml" .github/workflows/ --no-heading -n 2>/dev/null || true
echo -e "\n=== All YAML files in .github/ ==="
find .github -name "*.yaml" -o -name "*.yml" | sortRepository: primatrix/pallas-kernel
Length of output: 587
Include the delegated CI files in this trigger set.
This workflow fully delegates execution to .github/ci/cpu-ref-tests-gpu.sky.yaml (lines 16-24 contain the dependency setup and pytest execution), but neither that file nor this workflow itself is included in the trigger paths. A PR that only changes the CI wiring will merge without ever exercising this check.
🛠️ Suggested change
push:
branches: [main]
paths:
+ - ".github/workflows/cpu_ref_tests.yml"
+ - ".github/ci/cpu-ref-tests-gpu.sky.yaml"
- "tops/cpu/**"
- "tests/ref/**"
pull_request:
branches: [main]
paths:
+ - ".github/workflows/cpu_ref_tests.yml"
+ - ".github/ci/cpu-ref-tests-gpu.sky.yaml"
- "tops/cpu/**"
- "tests/ref/**"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| push: | |
| branches: [main] | |
| paths: | |
| - "tops/cpu/**" | |
| - "tests/ref/**" | |
| pull_request: | |
| branches: [main] | |
| paths: | |
| - "tops/cpu/**" | |
| - "tests/ref/**" | |
| push: | |
| branches: [main] | |
| paths: | |
| - ".github/workflows/cpu_ref_tests.yml" | |
| - ".github/ci/cpu-ref-tests-gpu.sky.yaml" | |
| - "tops/cpu/**" | |
| - "tests/ref/**" | |
| pull_request: | |
| branches: [main] | |
| paths: | |
| - ".github/workflows/cpu_ref_tests.yml" | |
| - ".github/ci/cpu-ref-tests-gpu.sky.yaml" | |
| - "tops/cpu/**" | |
| - "tests/ref/**" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/cpu_ref_tests.yml around lines 4 - 13, The workflow
trigger is missing the delegated CI file, so changes to the delegated pipeline
won't run this check; update the workflow triggers in cpu_ref_tests.yml to
include the delegated CI file(s) (at least the file named
cpu-ref-tests-gpu.sky.yaml or the .github/ci/** folder) so edits to the
delegated CI wiring cause the workflow to run; modify the push and pull_request
paths blocks to add those paths alongside "tops/cpu/**" and "tests/ref/**".
| cpu_ref_tests: | ||
| name: "CPU ref tests (GPU via SkyPilot)" | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read | ||
| id-token: write |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/cpu_ref_tests.ymlRepository: primatrix/pallas-kernel
Length of output: 2184
🏁 Script executed:
# Let me check if there are any existing guards or conditions on this job
rg "if:" .github/workflows/cpu_ref_tests.ymlRepository: primatrix/pallas-kernel
Length of output: 87
🌐 Web query:
GitHub Actions secrets forked pull requests not available
💡 Result:
Yes, GitHub Actions secrets (repository, organization, or environment secrets, except GITHUB_TOKEN) are not available to workflows triggered by pull requests from forked repositories when using the standard pull_request event. This is a deliberate security feature to prevent malicious actors from accessing secrets via untrusted code in a forked PR. Key points from official documentation: - Workflows on pull_request from forks run in the context of the fork's code with read-only GITHUB_TOKEN and no access to secrets. - To access secrets for fork PRs, use pull_request_target event, which runs in the base repository's context (access to secrets and write permissions). However, this requires careful handling: do not checkout or execute untrusted PR code without validation, as it risks secret exfiltration. - Enable repository/organization settings like "Require approval for first time contributors" or workflow approvals for added safety. - Alternatives: Use workflow_run triggered after a successful pull_request CI job, or OIDC for cloud auth without long-lived secrets. This behavior remains current as of 2026 per GitHub Docs.
Citations:
- 1: https://docs.github.com/actions/security-guides/using-secrets-in-github-actions
- 2: https://docs.github.com/en/actions/security-guides/using-secrets-in-github-actions
- 3: https://docs.github.com/en/actions/concepts/security/compromised-runners
- 4: https://docs.github.com/en/actions/managing-workflow-runs-and-deployments/managing-workflow-runs/approving-workflow-runs-from-public-forks
- 5: https://docs.github.com/en/code-security/getting-started/understanding-github-secret-types
Guard the cloud-backed job on forked PRs.
Forked pull_request runs do not receive repository secrets, so the secrets.GCP_WORKLOAD_IDENTITY_PROVIDER and secrets.GCP_SERVICE_ACCOUNT at lines 37-38 resolve to empty values and google-github-actions/auth@v2 fails before tests start. Add a same-repo guard to prevent this:
🛠️ Suggested change
jobs:
cpu_ref_tests:
name: "CPU ref tests (GPU via SkyPilot)"
+ if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository
runs-on: ubuntu-latest🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/cpu_ref_tests.yml around lines 20 - 25, The cpu_ref_tests
job is failing for forked PRs because secrets.GCP_WORKLOAD_IDENTITY_PROVIDER and
secrets.GCP_SERVICE_ACCOUNT are unavailable; add a same-repo guard to the job so
it only runs for PRs originating from the same repository. Update the
cpu_ref_tests job (named "CPU ref tests (GPU via SkyPilot)") to include an if
condition that ensures either the event is not a pull_request or, when it is,
that github.event.pull_request.head.repo.full_name equals github.repository so
the google-github-actions/auth@v2 step gets valid secrets.
| - name: Generate cluster name | ||
| id: cluster | ||
| run: | | ||
| BRANCH="${{ github.head_ref || github.ref_name }}" | ||
| NAME="ci-ref-$(echo "$BRANCH" | tr '/_' '--' | tr '[:upper:]' '[:lower:]' | head -c 20)-${GITHUB_RUN_NUMBER}" | ||
| echo "name=$NAME" >> "$GITHUB_OUTPUT" | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the actual workflow file to understand the context
cat -n .github/workflows/cpu_ref_tests.yml | head -60Repository: primatrix/pallas-kernel
Length of output: 2049
🌐 Web query:
GitHub Actions context interpolation security github.head_ref command injection
💡 Result:
Yes, direct interpolation of GitHub Actions context variables like github.head_ref into shell commands (run: steps) enables command injection vulnerabilities. github.head_ref represents the source branch of a pull request (available in pull_request or pull_request_target events) and is attacker-controlled, especially in forks or pull requests. Vulnerable pattern: - run: echo "${{ github.head_ref }}" An attacker creates a branch named test"; curl https://attacker.com/?token=$GITHUB_TOKEN;#, injecting and executing arbitrary shell commands, potentially exfiltrating secrets like GITHUB_TOKEN. This is confirmed in official GitHub documentation on script injections, which lists github.head_ref as untrusted input that can lead to shell command injection when interpolated into inline scripts. Real-world examples include CVEs like CVE-2026-33475 (Langflow) and GHSA-7x29-qqmq-v6qc (ultralytics/actions), where unsanitized use of github.head_ref in run: steps allowed RCE via malicious branch names. Mitigations (per GitHub docs): 1. Use environment variables: - env: HEAD_REF: ${{ github.head_ref }} run: echo "$HEAD_REF" The expression evaluates to a safe env var, preventing injection during shell script generation. 2. Prefer JavaScript actions over inline shell scripts for processing untrusted input. 3. Avoid pull_request_target trigger unless necessary, as it provides write access and untrusted checkout. 4. Pin GITHUB_TOKEN permissions to read-only where possible. Use in if: conditions or non-shell contexts (e.g., with:) is safe, as expressions are evaluated by the Actions engine, not shell.
Citations:
- 1: https://docs.github.com/en/actions/reference/contexts-reference
- 2: https://docs.github.com/en/actions/concepts/security/script-injections
- 3: GHSA-87cc-65ph-2j4w
- 4: https://nvd.nist.gov/vuln/detail/CVE-2026-33475
- 5: https://securitylab.github.com/resources/github-actions-untrusted-input/
- 6: GHSA-7x29-qqmq-v6qc
- 7: https://docs.github.com/en/actions/reference/secure-use-reference
- 8: https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions
🌐 Web query:
GitHub Actions env vs run direct interpolation ${{ }} security best practices
💡 Result:
In GitHub Actions, using
Citations:
- 1: https://github.blog/2023-08-09-four-tips-to-keep-your-github-actions-workflows-secure
- 2: https://docs.github.com/en/actions/reference/secure-use-reference
- 3: https://github.blog/security/supply-chain-security/four-tips-to-keep-your-github-actions-workflows-secure/
- 4: https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions
- 5: https://docs.github.com/en/actions/reference/security/secure-use
Don't interpolate github.head_ref directly into the shell.
Line 49 splices attacker-controlled branch names into the script body. A branch like foo$(...) will execute on the runner before BRANCH is assigned. Pass the value through env: and read the env var inside Bash instead.
🛠️ Suggested change
- name: Generate cluster name
id: cluster
+ env:
+ BRANCH_NAME: ${{ github.head_ref || github.ref_name }}
run: |
- BRANCH="${{ github.head_ref || github.ref_name }}"
+ BRANCH="$BRANCH_NAME"
NAME="ci-ref-$(echo "$BRANCH" | tr '/_' '--' | tr '[:upper:]' '[:lower:]' | head -c 20)-${GITHUB_RUN_NUMBER}"
echo "name=$NAME" >> "$GITHUB_OUTPUT"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Generate cluster name | |
| id: cluster | |
| run: | | |
| BRANCH="${{ github.head_ref || github.ref_name }}" | |
| NAME="ci-ref-$(echo "$BRANCH" | tr '/_' '--' | tr '[:upper:]' '[:lower:]' | head -c 20)-${GITHUB_RUN_NUMBER}" | |
| echo "name=$NAME" >> "$GITHUB_OUTPUT" | |
| - name: Generate cluster name | |
| id: cluster | |
| env: | |
| BRANCH_NAME: ${{ github.head_ref || github.ref_name }} | |
| run: | | |
| BRANCH="$BRANCH_NAME" | |
| NAME="ci-ref-$(echo "$BRANCH" | tr '/_' '--' | tr '[:upper:]' '[:lower:]' | head -c 20)-${GITHUB_RUN_NUMBER}" | |
| echo "name=$NAME" >> "$GITHUB_OUTPUT" |
🧰 Tools
🪛 actionlint (1.7.11)
[error] 48-48: "github.head_ref" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/reference/security/secure-use#good-practices-for-mitigating-script-injection-attacks for more details
(expression)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/cpu_ref_tests.yml around lines 46 - 52, The step with id
"cluster" currently interpolates github.head_ref/github.ref_name directly into
the run script, which lets attacker-controlled branch names be executed; change
the step to pass those values via env: (e.g., set GITHUB_HEAD_REF: ${{
github.head_ref }} and GITHUB_REF_NAME: ${{ github.ref_name }}) and then inside
the run block read them from the environment (e.g.,
BRANCH="${GITHUB_HEAD_REF:-$GITHUB_REF_NAME}") before sanitizing and building
NAME; this removes direct shell interpolation of workflow expressions and
prevents branch names like `foo$(...)` from being executed.
Summary by CodeRabbit