feat: add component-level context and boundary lint rules#941
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds metadata-driven synchronization for Claude and Cursor agent configuration, with Estimated code review effort: 3 (Moderate) | ~30 minutes 🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (8 passed)
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 |
|
@h0pers wondering if we can make the rules common for both Claude and Cursor as well. Without repeating the md files, perhaps a suggestion like |
Add .claude/rules/ with path-scoped rules for trainer, common/support, and general test conventions. Enable depguard in .golangci.yml with per-suite boundary rules preventing cross-suite imports. Move shared image helpers from tests/kfto/ to tests/common/support/ to eliminate existing violations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… violations The depguard boundary rules added in 1a3736d prevent cross-suite imports, but tests/odh/ was still importing from tests/trainer/utils/. Move PollNotebookLogsForStatus to tests/common/support/core.go and update all callers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
b074505 to
fcfca2c
Compare
… truth Replace hack/sync-agents-config.sh with hack/sync_agents_config.py that handles both skills and rules, generates .cursor/rules/*.mdc files, and validates metadata.json for each source document. Move rule sources to ai/rules/ alongside ai/skills/ with metadata.json for agent-specific fields (globs, description). Add uv setup step to CI verification. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/verify_generated_files.yml (1)
34-36: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSet least-privilege permissions per job (CWE-284).
The
verify-agents-configjob lacks apermissionsblock, causing it to inherit default workflow permissions. If default permissions are broad, this could allow a compromised CI runner to mutate the repository. As per path instructions, set explicit least-privilege permissions per job.Proposed fix
verify-agents-config: runs-on: ubuntu-latest + permissions: + contents: read steps:🤖 Prompt for 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. In @.github/workflows/verify_generated_files.yml around lines 34 - 36, Update the verify-agents-config job to declare an explicit least-privilege permissions block, granting only the permissions required by its existing steps and setting all others to none; do not rely on inherited workflow defaults.Sources: Path instructions, Linters/SAST tools
tests/odh/training_hub_rayjob_test.go (1)
141-169: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winCWE-78: Command injection risk due to unquoted shell variables.
The
scriptConfigMap,algorithm, andpvcNamevariables are interpolated directly into the shell command without shell quoting. Even if these currently originate from trusted Kubernetes names or hardcoded strings, this pattern is a command injection risk. All variables interpolated into shell commands must be properly quoted.Proposed fix
return []string{ "/bin/sh", "-c", s3Exports + "pip install papermill huggingface_hub datasets s3fs && " + "if papermill /opt/app-root/notebooks/{{.NotebookConfigMapFileName}}" + " /opt/app-root/src/mcad-out.ipynb -p namespace {{.Namespace}} -p ray_image " + rayImage + - fmt.Sprintf(" -p script_configmap %s -p algorithm %s -p pvc_name %s", scriptConfigMap, algorithm, pvcName) + + fmt.Sprintf(" -p script_configmap %s -p algorithm %s -p pvc_name %s", trainingHubShellQuote(scriptConfigMap), trainingHubShellQuote(algorithm), trainingHubShellQuote(pvcName)) + " -p openshift_api_url {{.OpenShiftApiUrl}} -p kubernetes_user_bearer_token {{.KubernetesUserBearerToken}}" + " -p num_gpus {{ .NumGpus }} --log-output; " + "then echo 'NOTEBOOK_STATUS: SUCCESS'; else echo 'NOTEBOOK_STATUS: FAILURE'; fi; sleep infinity", }🤖 Prompt for 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. In `@tests/odh/training_hub_rayjob_test.go` around lines 141 - 169, Update getTrainingHubNotebookCommand to shell-quote scriptConfigMap, algorithm, and pvcName before interpolating them into the command, using the existing trainingHubShellQuote helper. Ensure each value remains a single argument and cannot alter shell syntax.
🤖 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/verify_generated_files.yml:
- Around line 37-39: Update the actions/checkout and astral-sh/setup-uv steps in
the workflow to reference immutable full commit SHAs instead of mutable version
tags. Configure actions/checkout to disable credential persistence, while
preserving the existing action usage and setup behavior.
In `@hack/sync_agents_config.py`:
- Around line 3-4: Update the dependency declaration in the script metadata to
pin pyyaml to an explicit tested version, using the existing dependency key and
preserving the Python version requirement.
In `@tests/common/support/core.go`:
- Around line 150-176: Update PollNotebookLogsForStatus to call
test.T().Helper() at the start of the shared helper, before its polling and
assertion logic, so failures are attributed to the calling test.
In `@tests/common/support/environment.go`:
- Around line 260-267: Update GetBloomModelImage and GetAlpacaDatasetImage to
accept the Test interface, call test.T().Helper() at the start of each helper,
and continue using lookupEnvOrDefault with their existing environment keys and
defaults.
---
Outside diff comments:
In @.github/workflows/verify_generated_files.yml:
- Around line 34-36: Update the verify-agents-config job to declare an explicit
least-privilege permissions block, granting only the permissions required by its
existing steps and setting all others to none; do not rely on inherited workflow
defaults.
In `@tests/odh/training_hub_rayjob_test.go`:
- Around line 141-169: Update getTrainingHubNotebookCommand to shell-quote
scriptConfigMap, algorithm, and pvcName before interpolating them into the
command, using the existing trainingHubShellQuote helper. Ensure each value
remains a single argument and cannot alter shell syntax.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: cbb2a6c0-860a-4ff9-a271-d8140745c881
📒 Files selected for processing (45)
.claude/rules/tests-common-support.md.claude/rules/tests-general.md.claude/rules/tests-trainer.md.claude/skills/add-benchmark/SKILL.md.claude/skills/add-e2e-test/SKILL.md.claude/skills/update-support-lib/SKILL.md.cursor/rules/tests-common-support.mdc.cursor/rules/tests-general.mdc.cursor/rules/tests-trainer.mdc.cursor/skills/add-benchmark/SKILL.md.cursor/skills/add-e2e-test/SKILL.md.cursor/skills/update-support-lib/SKILL.md.github/workflows/verify_generated_files.yml.golangci.ymlAGENTS.mdMakefileai/rules/tests-common-support/RULE.mdai/rules/tests-common-support/metadata.jsonai/rules/tests-general/RULE.mdai/rules/tests-general/metadata.jsonai/rules/tests-trainer/RULE.mdai/rules/tests-trainer/metadata.jsonai/skills/add-benchmark/metadata.jsonai/skills/add-e2e-test/metadata.jsonai/skills/update-support-lib/metadata.jsonhack/sync_agents_config.pyhack/verify-agents-config.shtests/common/support/core.gotests/common/support/environment.gotests/fms/kfto/kfto_kueue_sft_GPU_test.gotests/fms/kfto/kfto_kueue_sft_test.gotests/fms/trainer/sft_trainjob_gpu_test.gotests/fms/trainer/sft_trainjob_test.gotests/kfto/environment.gotests/kfto/kfto_sft_llm_test.gotests/odh/training_hub_rayjob_test.gotests/trainer/jobset_workflow_test.gotests/trainer/sdk_tests/failure_traininghub_tests.gotests/trainer/sdk_tests/fashion_mnist_tests.gotests/trainer/sdk_tests/lora_traininghub_tests.gotests/trainer/sdk_tests/mpi_tests.gotests/trainer/sdk_tests/osft_traininghub_tests.gotests/trainer/sdk_tests/sft_traininghub_tests.gotests/trainer/trainer_fashion_mnist_training_test.gotests/trainer/utils/utils_notebook.go
💤 Files with no reviewable changes (2)
- tests/trainer/utils/utils_notebook.go
- tests/kfto/environment.go
🚧 Files skipped from review as they are similar to previous changes (7)
- .claude/rules/tests-general.md
- .golangci.yml
- tests/fms/trainer/sft_trainjob_gpu_test.go
- tests/kfto/kfto_sft_llm_test.go
- tests/fms/kfto/kfto_kueue_sft_GPU_test.go
- tests/fms/kfto/kfto_kueue_sft_test.go
- tests/trainer/jobset_workflow_test.go
| - uses: actions/checkout@v4 | ||
| - name: Set up uv | ||
| uses: astral-sh/setup-uv@v7 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Pin Actions by full SHA and disable credential persistence (CWE-829, CWE-312).
Actions are pinned by mutable tags (@v4, @v7). Tags can be moved to point to malicious commits, enabling supply chain attacks (e.g., CVE-2025-30066). As per path instructions, pin all actions by their full commit SHA.
Additionally, disable credential persistence in actions/checkout to prevent the GITHUB_TOKEN from remaining exposed in the runner's .git/config.
Proposed fix
- - uses: actions/checkout@v4
+ - uses: actions/checkout@<full-sha>
+ with:
+ persist-credentials: false
- name: Set up uv
- uses: astral-sh/setup-uv@v7
+ uses: astral-sh/setup-uv@<full-sha>🧰 Tools
🪛 zizmor (1.26.1)
[warning] 37-37: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 39-39: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): enables caching by default
(cache-poisoning)
🤖 Prompt for 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.
In @.github/workflows/verify_generated_files.yml around lines 37 - 39, Update
the actions/checkout and astral-sh/setup-uv steps in the workflow to reference
immutable full commit SHAs instead of mutable version tags. Configure
actions/checkout to disable credential persistence, while preserving the
existing action usage and setup behavior.
Sources: Path instructions, Linters/SAST tools
| # requires-python = ">=3.11" | ||
| # dependencies = ["pyyaml"] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Pin Python dependencies (CWE-829).
The pyyaml dependency is not pinned to a specific version or hash. Unpinned dependencies in scripts are susceptible to supply chain attacks if a malicious version is published or the package is compromised (e.g., dependency confusion or account takeover).
Provide a pinned version (e.g., pyyaml==6.0.1) to ensure reproducible and secure executions.
Proposed fix
# requires-python = ">=3.11"
-# dependencies = ["pyyaml"]
+# dependencies = ["pyyaml==6.0.1"]📝 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.
| # requires-python = ">=3.11" | |
| # dependencies = ["pyyaml"] | |
| # requires-python = ">=3.11" | |
| # dependencies = ["pyyaml==6.0.1"] |
🤖 Prompt for 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.
In `@hack/sync_agents_config.py` around lines 3 - 4, Update the dependency
declaration in the script metadata to pin pyyaml to an explicit tested version,
using the existing dependency key and preserving the Python version requirement.
Source: Path instructions
| func PollNotebookLogsForStatus(test Test, namespace, podName, containerName string, timeout time.Duration) error { | ||
| var tail int64 = 2000 | ||
| getLogs := PodLog(test, namespace, podName, corev1.PodLogOptions{ | ||
| Container: containerName, | ||
| TailLines: &tail, | ||
| }) | ||
|
|
||
| sawFailure := false | ||
| test.Eventually(func() bool { | ||
| logs := getLogs(test) | ||
| switch { | ||
| case strings.Contains(logs, "NOTEBOOK_STATUS: SUCCESS"): | ||
| return true | ||
| case strings.Contains(logs, "NOTEBOOK_STATUS: FAILURE"): | ||
| sawFailure = true | ||
| return true | ||
| default: | ||
| return false | ||
| } | ||
| }, timeout).Should(gomega.BeTrue(), "Notebook did not reach definitive state") | ||
|
|
||
| if sawFailure { | ||
| return fmt.Errorf("Notebook execution failed") | ||
| } | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Missing test.T().Helper() in shared helper.
Shared helper functions in tests/common/support/ must call test.T().Helper() to ensure correct test failure attribution. As per coding guidelines: "Shared helpers must receive the Test interface for Kubernetes client, context, and Gomega assertion access, and helper functions must call test.T().Helper()."
Proposed fix
func PollNotebookLogsForStatus(test Test, namespace, podName, containerName string, timeout time.Duration) error {
+ test.T().Helper()
var tail int64 = 2000
getLogs := PodLog(test, namespace, podName, corev1.PodLogOptions{📝 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.
| func PollNotebookLogsForStatus(test Test, namespace, podName, containerName string, timeout time.Duration) error { | |
| var tail int64 = 2000 | |
| getLogs := PodLog(test, namespace, podName, corev1.PodLogOptions{ | |
| Container: containerName, | |
| TailLines: &tail, | |
| }) | |
| sawFailure := false | |
| test.Eventually(func() bool { | |
| logs := getLogs(test) | |
| switch { | |
| case strings.Contains(logs, "NOTEBOOK_STATUS: SUCCESS"): | |
| return true | |
| case strings.Contains(logs, "NOTEBOOK_STATUS: FAILURE"): | |
| sawFailure = true | |
| return true | |
| default: | |
| return false | |
| } | |
| }, timeout).Should(gomega.BeTrue(), "Notebook did not reach definitive state") | |
| if sawFailure { | |
| return fmt.Errorf("Notebook execution failed") | |
| } | |
| return nil | |
| } | |
| func PollNotebookLogsForStatus(test Test, namespace, podName, containerName string, timeout time.Duration) error { | |
| test.T().Helper() | |
| var tail int64 = 2000 | |
| getLogs := PodLog(test, namespace, podName, corev1.PodLogOptions{ | |
| Container: containerName, | |
| TailLines: &tail, | |
| }) | |
| sawFailure := false | |
| test.Eventually(func() bool { | |
| logs := getLogs(test) | |
| switch { | |
| case strings.Contains(logs, "NOTEBOOK_STATUS: SUCCESS"): | |
| return true | |
| case strings.Contains(logs, "NOTEBOOK_STATUS: FAILURE"): | |
| sawFailure = true | |
| return true | |
| default: | |
| return false | |
| } | |
| }, timeout).Should(gomega.BeTrue(), "Notebook did not reach definitive state") | |
| if sawFailure { | |
| return fmt.Errorf("Notebook execution failed") | |
| } | |
| return nil | |
| } |
🤖 Prompt for 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.
In `@tests/common/support/core.go` around lines 150 - 176, Update
PollNotebookLogsForStatus to call test.T().Helper() at the start of the shared
helper, before its polling and assertion logic, so failures are attributed to
the calling test.
Source: Coding guidelines
| func GetBloomModelImage() string { | ||
| return lookupEnvOrDefault(bloomModelImageEnvVar, "quay.io/ksuta/bloom-560m@sha256:f6db02bb7b5d09a8d698c04994d747bfb9e581bbb4c07d00290244d207623733") | ||
| } | ||
|
|
||
| func GetAlpacaDatasetImage() string { | ||
| return lookupEnvOrDefault(alpacaDatasetImageEnvVar, "quay.io/ksuta/alpaca-dataset@sha256:2e90f631180c7b2c916f9569b914b336b612e8ae86efad82546adc5c9fcbbb8d") | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update helper function signatures to accept the Test interface.
The GetBloomModelImage and GetAlpacaDatasetImage helpers omit the Test interface and fail to call test.T().Helper(). As per coding guidelines, shared helpers must receive the Test interface for Kubernetes client, context, and Gomega assertion access, and helper functions must call test.T().Helper(). This implementation also deviates from the pattern documented in .claude/rules/tests-common-support.md, which mandates func GetMyValue(test Test) string for environment variable getters.
♻️ Proposed fixes
-func GetBloomModelImage() string {
+func GetBloomModelImage(test Test) string {
+ test.T().Helper()
return lookupEnvOrDefault(bloomModelImageEnvVar, "quay.io/ksuta/bloom-560m@sha256:f6db02bb7b5d09a8d698c04994d747bfb9e581bbb4c07d00290244d207623733")
}
-func GetAlpacaDatasetImage() string {
+func GetAlpacaDatasetImage(test Test) string {
+ test.T().Helper()
return lookupEnvOrDefault(alpacaDatasetImageEnvVar, "quay.io/ksuta/alpaca-dataset@sha256:2e90f631180c7b2c916f9569b914b336b612e8ae86efad82546adc5c9fcbbb8d")
}📝 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.
| func GetBloomModelImage() string { | |
| return lookupEnvOrDefault(bloomModelImageEnvVar, "quay.io/ksuta/bloom-560m@sha256:f6db02bb7b5d09a8d698c04994d747bfb9e581bbb4c07d00290244d207623733") | |
| } | |
| func GetAlpacaDatasetImage() string { | |
| return lookupEnvOrDefault(alpacaDatasetImageEnvVar, "quay.io/ksuta/alpaca-dataset@sha256:2e90f631180c7b2c916f9569b914b336b612e8ae86efad82546adc5c9fcbbb8d") | |
| } | |
| func GetBloomModelImage(test Test) string { | |
| test.T().Helper() | |
| return lookupEnvOrDefault(bloomModelImageEnvVar, "quay.io/ksuta/bloom-560m@sha256:f6db02bb7b5d09a8d698c04994d747bfb9e581bbb4c07d00290244d207623733") | |
| } | |
| func GetAlpacaDatasetImage(test Test) string { | |
| test.T().Helper() | |
| return lookupEnvOrDefault(alpacaDatasetImageEnvVar, "quay.io/ksuta/alpaca-dataset@sha256:2e90f631180c7b2c916f9569b914b336b612e8ae86efad82546adc5c9fcbbb8d") | |
| } |
🤖 Prompt for 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.
In `@tests/common/support/environment.go` around lines 260 - 267, Update
GetBloomModelImage and GetAlpacaDatasetImage to accept the Test interface, call
test.T().Helper() at the start of each helper, and continue using
lookupEnvOrDefault with their existing environment keys and defaults.
Source: Coding guidelines
2 participants
What this PR does / why we need it:
Implements AI Scaffolding Tier 3 checks 3.1 and 3.3 for the distributed-workloads repo:
.claude/rules/with path-scoped rules fortests/trainer/,tests/common/support/, and general test conventions (mandatory tags, namespace isolation, resource naming, import boundaries).depguardin.golangci.ymlwith per-suite boundary rules preventing cross-suite imports betweentests/kfto/,tests/trainer/,tests/fms/, andtests/odh/. Move shared image helpers (GetAlpacaDatasetImage,GetBloomModelImage) fromtests/kfto/totests/common/support/environment.goto resolve existing cross-suite import violations.Which issue(s) this PR fixes:
Fixes #940
Summary by CodeRabbit
Summary by CodeRabbit
Documentation
Bug Fixes
Refactor
Chores