Add autofix support to plugins-doc-up-to-date rule - #475
Closed
not-stbenjam wants to merge 6 commits into
Closed
not-stbenjam wants to merge 6 commits into
not-stbenjam wants to merge 6 commits into
Conversation
Enable `skillsaw fix` to automatically regenerate PLUGINS.md and docs/data.json when they drift from plugin metadata, instead of requiring a manual `make update`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: not-stbenjam 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 |
Contributor
WalkthroughThis PR introduces autofix support to the plugin documentation rule and implements a comprehensive reproducible evaluation framework for the payload analysis skill. The framework enables deterministic evaluation by archiving external dependencies (GCS artifacts, API responses) locally, intercepting network calls via shims, and comparing skill outputs against expected results using structured judges. ChangesPlugin Documentation Rule Autofix Support
Payload Analysis Evaluation Framework
🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
openshift-ci
Bot
added
the
needs-ok-to-test
Contributor
|
Hi @not-stbenjam. Thanks for your PR. I'm waiting for a openshift-eng member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.skillsaw/plugindocs_rule.py (1)
42-62: ⚡ Quick winPrefer
sys.executableover hard-coded"python3"and consider DRY-ing the two subprocess blocks.Using a bare
"python3"(Ruff S607) means the generator runs under whatever interpreterpython3happens to resolve to inPATH, which may differ from the interpreter running skillsaw (e.g., in virtualenvs / CI images without apython3symlink). Usingsys.executablekeeps the generator on the same interpreter and avoids the partial-path lint. The twosubprocess.run(...)invocations are also near-duplicates and would read better as a small inner helper.♻️ Sketch of a DRY-ed helper using sys.executable
import subprocess +import sys from pathlib import Path from typing import Any, List @@ def _run_generators(self, context: RepositoryContext): """Run doc generation scripts. Returns (original_plugins_md, generated_plugins_md, original_data_json, generated_data_json) or raises on failure.""" plugins_md_path = context.root_path / "PLUGINS.md" data_json_path = context.root_path / "docs" / "data.json" script_path = context.root_path / "scripts" / "generate_plugin_docs.py" original_plugins_md = plugins_md_path.read_text() original_data_json = data_json_path.read_text() if data_json_path.exists() else None - result = subprocess.run( - ["python3", str(script_path)], - cwd=str(context.root_path), - capture_output=True, - text=True, - timeout=30, - ) - if result.returncode != 0: - raise RuntimeError(f"generate_plugin_docs.py failed: {result.stderr}") - - website_script_path = context.root_path / "scripts" / "build-website.py" - if website_script_path.exists(): - result = subprocess.run( - ["python3", str(website_script_path)], - cwd=str(context.root_path), - capture_output=True, - text=True, - timeout=30, - ) - if result.returncode != 0: - raise RuntimeError(f"build-website.py failed: {result.stderr}") + def _run(script: Path) -> None: + result = subprocess.run( + [sys.executable, str(script)], + cwd=str(context.root_path), + capture_output=True, + text=True, + timeout=30, + ) + if result.returncode != 0: + raise RuntimeError(f"{script.name} failed: {result.stderr}") + + _run(script_path) + website_script_path = context.root_path / "scripts" / "build-website.py" + if website_script_path.exists(): + _run(website_script_path)Please confirm whether the generator scripts in
scripts/require a specific interpreter (e.g., a pinnedpython3) before swapping tosys.executable.🤖 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 @.skillsaw/plugindocs_rule.py around lines 42 - 62, Replace hard-coded "python3" invocations in the two subprocess.run calls with sys.executable and DRY the duplicated logic by extracting a small helper (e.g., run_python_script(script_path, context_root, timeout=30)) that calls subprocess.run([sys.executable, str(script_path)], cwd=str(context.root_path), capture_output=True, text=True, timeout=timeout) and raises RuntimeError with result.stderr on non-zero returncode; update calls for script_path and website_script_path to use this helper and verify first whether the scripts in scripts/ require a specific interpreter before switching to sys.executable.
🤖 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 @.skillsaw/plugindocs_rule.py:
- Around line 103-111: The check currently leaves a newly-created docs/data.json
on disk when original_data_json is None; update the post-generator cleanup in
the method that calls _run_generators() to remove the file if it didn't exist
originally: when data_json_path.exists() and original_data_json !=
generated_data_json, if original_data_json is None call unlink/delete on
data_json_path (otherwise restore original_data_json as before) and then append
the same violation; reference data_json_path, original_data_json,
generated_data_json, and _run_generators() in your change.
- Around line 141-147: The try/except around self._run_generators currently
swallows all exceptions and returns results; change it to catch Exception as e
and either log the exception via the module logger or return an AutofixResult
sentinel (e.g., with confidence=UNSAFE) so callers can distinguish failures from
"no fixes"; also decide and implement consistent file-mutation semantics for
fix(): either (A) treat fix() like check() by restoring original files after
producing AutofixResult (so keep original_content/fixed_content fields and
ensure PLUGINS.md and docs/data.json are restored after generation), or (B)
treat fix() as the authoritative writer (have fix() write the files and remove
original_content/fixed_content from AutofixResult); update the fix()
implementation accordingly and document the chosen behavior so it aligns with
check(). Ensure you reference and modify _run_generators, fix(), check(), and
AutofixResult logic when making these changes.
---
Nitpick comments:
In @.skillsaw/plugindocs_rule.py:
- Around line 42-62: Replace hard-coded "python3" invocations in the two
subprocess.run calls with sys.executable and DRY the duplicated logic by
extracting a small helper (e.g., run_python_script(script_path, context_root,
timeout=30)) that calls subprocess.run([sys.executable, str(script_path)],
cwd=str(context.root_path), capture_output=True, text=True, timeout=timeout) and
raises RuntimeError with result.stderr on non-zero returncode; update calls for
script_path and website_script_path to use this helper and verify first whether
the scripts in scripts/ require a specific interpreter before switching to
sys.executable.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 92dda6d0-ac48-44fa-a4f7-38bffb57550d
📒 Files selected for processing (1)
.skillsaw/plugindocs_rule.py
Comment on lines
+103
to
+111
| if data_json_path.exists() and original_data_json != generated_data_json: | ||
| if original_data_json is not None: | ||
| data_json_path.write_text(original_data_json) | ||
| violations.append( | ||
| self.violation( | ||
| "docs/data.json is out of sync with plugin metadata. Run 'make update' to update.", | ||
| file_path=data_json_path, | ||
| ) | ||
| ) |
Contributor
There was a problem hiding this comment.
Newly-generated docs/data.json is left in the working tree when it didn't exist before.
If data_json_path does not exist prior to running the generators, original_data_json is None. After _run_generators() runs, the generator may create the file, so line 103's data_json_path.exists() is true and a violation is appended — but the restoration at line 104 is gated on original_data_json is not None, so the newly-created file is left on disk. This makes check() mutate the working tree, which violates the intent of a read-only check and can leak into commits or confuse later runs of the rule.
🛠️ Proposed fix: unlink the file if it didn't exist originally
if data_json_path.exists() and original_data_json != generated_data_json:
if original_data_json is not None:
data_json_path.write_text(original_data_json)
+ else:
+ data_json_path.unlink()
violations.append(
self.violation(
"docs/data.json is out of sync with plugin metadata. Run 'make update' to update.",
file_path=data_json_path,
)
)📝 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.
Suggested change
| if data_json_path.exists() and original_data_json != generated_data_json: | |
| if original_data_json is not None: | |
| data_json_path.write_text(original_data_json) | |
| violations.append( | |
| self.violation( | |
| "docs/data.json is out of sync with plugin metadata. Run 'make update' to update.", | |
| file_path=data_json_path, | |
| ) | |
| ) | |
| if data_json_path.exists() and original_data_json != generated_data_json: | |
| if original_data_json is not None: | |
| data_json_path.write_text(original_data_json) | |
| else: | |
| data_json_path.unlink() | |
| violations.append( | |
| self.violation( | |
| "docs/data.json is out of sync with plugin metadata. Run 'make update' to update.", | |
| file_path=data_json_path, | |
| ) | |
| ) |
🤖 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 @.skillsaw/plugindocs_rule.py around lines 103 - 111, The check currently
leaves a newly-created docs/data.json on disk when original_data_json is None;
update the post-generator cleanup in the method that calls _run_generators() to
remove the file if it didn't exist originally: when data_json_path.exists() and
original_data_json != generated_data_json, if original_data_json is None call
unlink/delete on data_json_path (otherwise restore original_data_json as before)
and then append the same violation; reference data_json_path,
original_data_json, generated_data_json, and _run_generators() in your change.
Comment on lines
+141
to
+147
| try: | ||
| ( | ||
| original_plugins_md, generated_plugins_md, | ||
| original_data_json, generated_data_json, | ||
| ) = self._run_generators(context) | ||
| except Exception: | ||
| return results |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for how AutofixResult is consumed and whether the framework writes fixed_content to disk.
fd -t f 'rule.py' .skillsaw skillsaw src 2>/dev/null
rg -nP -C5 '\bAutofixResult\b' --type=py
rg -nP -C5 '\bfixed_content\b|\bapply_fix\b|\bapply\s*\(' --type=py
# Also check other rules in this repo to see the established fix() contract.
fd -t f '_rule.py|rule.py' .skillsawRepository: openshift-eng/ai-helpers
Length of output: 196
🏁 Script executed:
# First, check the size and read the plugindocs_rule.py file
wc -l .skillsaw/plugindocs_rule.py
head -150 .skillsaw/plugindocs_rule.py | tail -50Repository: openshift-eng/ai-helpers
Length of output: 1853
🏁 Script executed:
# Read the full check() method to understand the restore logic
rg -n "def check" .skillsaw/plugindocs_rule.py -A 30Repository: openshift-eng/ai-helpers
Length of output: 1261
🏁 Script executed:
# Read the full fix() method to understand current behavior
rg -n "def fix" .skillsaw/plugindocs_rule.py -A 30Repository: openshift-eng/ai-helpers
Length of output: 1297
🏁 Script executed:
# Look at the other rule file to understand the established pattern
cat -n .skillsaw/promptfoo_budget_rule.py | head -80Repository: openshift-eng/ai-helpers
Length of output: 3100
🏁 Script executed:
# Search for AutofixResult definition and usage
rg -n "class AutofixResult|AutofixResult\(" --type=py -B2 -A5Repository: openshift-eng/ai-helpers
Length of output: 50
🏁 Script executed:
# Look for how the autofix framework handles results
rg -n "def.*fix|AutofixResult" --type=py | head -20Repository: openshift-eng/ai-helpers
Length of output: 845
🏁 Script executed:
# Search for AutofixResult definition and imports
rg -n "AutofixResult|AutofixConfidence" --type=py -B2 -A3 | head -50Repository: openshift-eng/ai-helpers
Length of output: 50
🏁 Script executed:
# Get the full fix() method and what comes after
sed -n '127,178p' .skillsaw/plugindocs_rule.pyRepository: openshift-eng/ai-helpers
Length of output: 2062
🏁 Script executed:
# Search for imports at the top of the file
head -40 .skillsaw/plugindocs_rule.pyRepository: openshift-eng/ai-helpers
Length of output: 1557
🏁 Script executed:
# Look for how AutofixResult is used elsewhere in the repo
rg -n "AutofixResult\(" --type=py -B1 -A2Repository: openshift-eng/ai-helpers
Length of output: 50
🏁 Script executed:
# Search for AutofixResult definition
rg -n "class AutofixResult|def.*AutofixResult" --type=py -B2 -A10Repository: openshift-eng/ai-helpers
Length of output: 50
🏁 Script executed:
# Look for where AutofixResult is imported from and how it's consumed
rg -n "from.*AutofixResult|import.*AutofixResult" --type=pyRepository: openshift-eng/ai-helpers
Length of output: 50
🏁 Script executed:
# Search for how the framework processes/applies AutofixResult
rg -n "fixed_content|apply.*fix|_apply" --type=py | head -30Repository: openshift-eng/ai-helpers
Length of output: 351
🏁 Script executed:
# Check if there's a base Rule class or interface that documents the fix() contract
fd -name "*.py" -path "*/skillsaw/*" | head -20Repository: openshift-eng/ai-helpers
Length of output: 299
🏁 Script executed:
# Look at the complete _run_generators method to understand what it does
sed -n '40,70p' .skillsaw/plugindocs_rule.pyRepository: openshift-eng/ai-helpers
Length of output: 1260
🏁 Script executed:
# Look for src/rule.py or skillsaw module in the repo
find . -type f -name "rule.py" -o -name "__init__.py" | grep -E "src|skillsaw" | head -20Repository: openshift-eng/ai-helpers
Length of output: 50
🏁 Script executed:
# Check what's imported from skillsaw - look for the package
rg -n "from skillsaw|import skillsaw" --type=py | head -10Repository: openshift-eng/ai-helpers
Length of output: 50
🏁 Script executed:
# Search the repo for where AutofixResult is actually consumed/applied
rg -n "\.fixed_content|result\." --type=py | grep -i fix | head -20Repository: openshift-eng/ai-helpers
Length of output: 50
🏁 Script executed:
# Look for the full file to understand the complete implementation
wc -l .skillsaw/plugindocs_rule.py
cat -n .skillsaw/plugindocs_rule.py | tail -30Repository: openshift-eng/ai-helpers
Length of output: 1664
🏁 Script executed:
# Check if there are any git references or docs about the autofix contract
find . -type f -name "*.md" -o -name "*.txt" | xargs grep -l "AutofixResult\|autofix" 2>/dev/null | head -5Repository: openshift-eng/ai-helpers
Length of output: 50
🏁 Script executed:
# Check the git log for this file to understand intent
git log --oneline .skillsaw/plugindocs_rule.py 2>/dev/null | head -10Repository: openshift-eng/ai-helpers
Length of output: 127
🏁 Script executed:
# Check if there are any comments or TODOs in the file
grep -n "TODO\|FIXME\|XXX\|NOTE" .skillsaw/plugindocs_rule.pyRepository: openshift-eng/ai-helpers
Length of output: 50
🏁 Script executed:
# Look at the complete check() method once more to see the full restoration pattern
sed -n '72,126p' .skillsaw/plugindocs_rule.pyRepository: openshift-eng/ai-helpers
Length of output: 2079
🏁 Script executed:
# Search for other uses of AutofixResult in the repo to understand the pattern
rg -n "AutofixResult\|fixed_content\|original_content" --type=py -B2 -A4 | head -80Repository: openshift-eng/ai-helpers
Length of output: 50
🏁 Script executed:
# Check if promptfoo_budget_rule.py has a fix() method
rg -n "def fix" .skillsaw/promptfoo_budget_rule.py -A 20Repository: openshift-eng/ai-helpers
Length of output: 50
🏁 Script executed:
# Look for any other custom rules
fd -t f "_rule.py\|_rule.py" .skillsawRepository: openshift-eng/ai-helpers
Length of output: 50
🏁 Script executed:
# Check git diff for the commit that added autofix support
git show 9fae9a0 --stat 2>/dev/nullRepository: openshift-eng/ai-helpers
Length of output: 33320
🏁 Script executed:
# Check if there's documentation or README about how fix() should work
find . -name "README*" -o -name "*.md" | xargs grep -l "fix\|autofix" 2>/dev/null | head -5Repository: openshift-eng/ai-helpers
Length of output: 204
🏁 Script executed:
# Check if skillsaw is a package we can inspect
python3 -c "import sys; sys.path.insert(0, '.'); from skillsaw import AutofixResult; help(AutofixResult)" 2>/dev/null || echo "skillsaw not importable"Repository: openshift-eng/ai-helpers
Length of output: 92
Address exception handling and clarify the autofix contract for file mutation.
Two issues in this block:
-
except Exceptionat line 146 silently returns emptyresultswith no logging, making failed regeneration indistinguishable from "nothing to fix" to the caller. This matches Ruff BLE001. Log the exception or return a sentinel (e.g.,confidence=UNSAFE) so the autofix runner can surface the error. -
fix()leaves PLUGINS.md and docs/data.json mutated on disk as side effects, while also providingoriginal_content/fixed_contentinAutofixResult. Unlikecheck(), which restores originals after generation,fix()does not. Clarify whether the autofix framework appliesfixed_contentto disk (in which casefix()should restore originals and not mutate files) or whetherfix()is expected to write directly (in which case omit the redundantoriginal_content/fixed_contentfields fromAutofixResult). Align this method's behavior withcheck()'s restore-on-mismatch semantics.
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 146-146: Do not catch blind exception: Exception
(BLE001)
🤖 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 @.skillsaw/plugindocs_rule.py around lines 141 - 147, The try/except around
self._run_generators currently swallows all exceptions and returns results;
change it to catch Exception as e and either log the exception via the module
logger or return an AutofixResult sentinel (e.g., with confidence=UNSAFE) so
callers can distinguish failures from "no fixes"; also decide and implement
consistent file-mutation semantics for fix(): either (A) treat fix() like
check() by restoring original files after producing AutofixResult (so keep
original_content/fixed_content fields and ensure PLUGINS.md and docs/data.json
are restored after generation), or (B) treat fix() as the authoritative writer
(have fix() write the files and remove original_content/fixed_content from
AutofixResult); update the fix() implementation accordingly and document the
chosen behavior so it aligns with check(). Ensure you reference and modify
_run_generators, fix(), check(), and AutofixResult logic when making these
changes.
Member
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
Evaluation harness for the analyze-payload skill using archived CI artifacts and cached API responses for reproducible results. - Eval config (eval.yaml) with 6 judges (4 inline, 2 LLM) - 4 archived test cases (006-009) with ground-truth annotations - gcloud/gh shims to intercept external calls and serve from archives - Session tarball extraction script for creating new archives - Cached response support in fetch_payloads.py and fetch_new_prs.py - archive-payload-result command for archiving new payloads Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
eval/README.md (1)
43-43: ⚡ Quick winAdd language specifiers to fenced code blocks.
Several fenced code blocks are missing language identifiers. Adding
textorbashidentifiers improves rendering and syntax highlighting.📋 Suggested language specifiers
-``` +```text eval/archives/{payload-tag}/-``` +```bash /ci:archive-payload-result 4.22.0-0.nightly-2026-03-20-053450-``` +```text eval/cases/{case-name}/-``` +```bash /eval-mlflow --action log-results --run-id <id>-``` +```text eval/Also applies to: 111-111, 122-122, 262-262, 306-306
🤖 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 `@eval/README.md` at line 43, The README.md has multiple fenced code blocks without language specifiers; update each fenced block (e.g., the blocks containing "eval/archives/{payload-tag}/", "/ci:archive-payload-result 4.22.0-0.nightly-2026-03-20-053450", "eval/cases/{case-name}/", "/eval-mlflow --action log-results --run-id <id>", and "eval/") to include an appropriate language tag (use text for plain paths and bash for commands) so they become ```text or ```bash as appropriate to enable correct rendering and syntax highlighting.plugins/ci/commands/archive-payload-result.md (1)
13-13: ⚡ Quick winAdd language specifiers to fenced code blocks.
Several fenced code blocks are missing language identifiers. Adding appropriate identifiers improves rendering and syntax highlighting.
📋 Suggested language specifiers
-``` +```bash /ci:archive-payload-result <payload-tag> [--output-dir DIR] [--limit N]-``` +```yaml humanurl: https://prow.ci.openshift.org/view/gs/test-platform-results/logs/{underlying-job-name}/{build-id}- ``` + ```bash /ci:archive-payload-result 4.22.0-0.nightly-2026-03-20-053450- ``` + ```bash /ci:archive-payload-result 4.22.0-0.nightly-2026-03-20-053450 --output-dir /tmp/payload-archives- ``` + ```bash /ci:archive-payload-result 4.22.0-0.nightly-2026-03-20-053450 --limit 30- ``` + ```bash /ci:archive-payload-result 4.22.0-0.nightly-arm64-2026-03-20-053450Also applies to: 164-164, 196-196, 201-201, 206-206, 211-211
🤖 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 `@plugins/ci/commands/archive-payload-result.md` at line 13, Several fenced code blocks in plugins/ci/commands/archive-payload-result.md are missing language specifiers; update each fenced block that contains shell commands (e.g. lines with "/ci:archive-payload-result <payload-tag> ..." and examples like "/ci:archive-payload-result 4.22.0-0.nightly-...") to use ```bash and add ```yaml for the block containing the humanurl mapping (humanurl: https://prow.ci.openshift.org/...), and apply the same fixes to the other example blocks referenced in the comment so all code fences include the appropriate language identifier for proper rendering and syntax highlighting.eval/scripts/extract-session-data.py (3)
191-206: ⚡ Quick winRename unused loop variable.
The loop variable
dirsis not used. Rename it to_dirs.🔧 Proposed fix
- for root, dirs, files in os.walk(tmpdir): + for root, _dirs, files in os.walk(tmpdir): for fname in files:🤖 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 `@eval/scripts/extract-session-data.py` around lines 191 - 206, The loop over os.walk(tmpdir) uses an unused variable named dirs; rename it to _dirs in the for root, dirs, files in os.walk(tmpdir) header (i.e., change to for root, _dirs, files in os.walk(tmpdir)) to follow the unused-variable convention and avoid linter warnings, ensuring no other references to dirs exist in the surrounding function or scope.
50-53: ⚡ Quick winConsider logging JSON decode errors.
The try-except block silently continues on any exception when parsing JSON lines. For debugging failed extractions, consider logging parse errors at least at a debug level.
📊 Suggested enhancement
try: msg = json.loads(line) - except Exception: + except json.JSONDecodeError as e: + # Uncomment for debugging: print(f"DEBUG: Failed to parse line: {e}", file=sys.stderr) continue🤖 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 `@eval/scripts/extract-session-data.py` around lines 50 - 53, The JSON parsing silently drops all errors; change the except to catch json.JSONDecodeError (or Exception if broader) and emit a debug-level log including the exception and the offending line before continuing. Use the existing logger (or create one via logging.getLogger(__name__) if absent) and log a message like "Failed to parse JSON line" with the exception and line content near the json.loads(line) call so you can debug failed extractions without changing behavior.
39-42: ⚡ Quick winRename unused loop variable.
The loop variable
dirsis not used within the loop body. Rename it to_dirsto indicate it's intentionally unused.🔧 Proposed fix
- for root, dirs, files in os.walk(session_dir): + for root, _dirs, files in os.walk(session_dir): for fname in sorted(files):🤖 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 `@eval/scripts/extract-session-data.py` around lines 39 - 42, Rename the unused loop variable in the os.walk loop to indicate it's intentionally unused: change the for loop header "for root, dirs, files in os.walk(session_dir):" to use "_dirs" instead of "dirs" (i.e., "for root, _dirs, files in os.walk(session_dir):") so the unused variable is clearly marked; update any matching usages if present (there should be none) and run tests/lint to ensure no unused-variable warnings remain.eval/shims/gcloud (1)
9-21: ⚖️ Poor tradeoffrealpath may not be available on all systems.
The
find_real_gcloudfunction usesrealpathwhich may not be available on older macOS or BSD systems. Consider using a fallback or checking for availability.💡 Portable alternative
find_real_gcloud() { local self self="$(cd "$(dirname "$0")" && pwd)/$(basename "$0")" local IFS=: for dir in $PATH; do local candidate="$dir/gcloud" - if [[ -x "$candidate" && "$(realpath "$candidate" 2>/dev/null)" != "$(realpath "$self" 2>/dev/null)" ]]; then + local candidate_real candidate_self + candidate_real="$(realpath "$candidate" 2>/dev/null || readlink -f "$candidate" 2>/dev/null || echo "$candidate")" + self_real="$(realpath "$self" 2>/dev/null || readlink -f "$self" 2>/dev/null || echo "$self")" + if [[ -x "$candidate" && "$candidate_real" != "$self_real" ]]; then echo "$candidate" return fiHowever, this is only necessary if the eval environment includes such systems.
🤖 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 `@eval/shims/gcloud` around lines 9 - 21, The find_real_gcloud function relies on the external realpath command which may be missing on some systems; modify find_real_gcloud to detect if realpath is available (e.g., with command -v realpath) and if not, use a portable fallback to canonicalize paths (for example use readlink -f when present, or a small python3/perl one-liner to resolve paths) and then use that chosen resolver wherever realpath is currently invoked (both for "$candidate" and "$self") so the comparison remains correct; ensure the resolver selection happens once at function start and falls back to a no-op or default (/usr/bin/gcloud) if no resolver exists.
🤖 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 `@eval.yaml`:
- Around line 56-57: Update the misleading comment in eval.yaml that claims "No
test cases exist yet" to reflect that test cases now exist (specifically the
added directory eval/cases/case-003-accepted-with-failures), e.g., change the
sentence to note that test cases have been added and mention the new case path;
edit the string in eval.yaml where that sentence appears so it accurately
describes current state.
In `@eval/cases/case-003-accepted-with-failures/annotations.yaml`:
- Around line 1-10: The annotations file is missing the expected_candidates
field that the revert_scoring_accuracy judge expects; either add an explicit
empty list by inserting expected_candidates: [] alongside has_revert_candidates:
false in the annotations (so the judge sees an explicit no-candidates case), or
modify the revert_scoring_accuracy judge logic to treat a missing
expected_candidates the same as an empty list when has_revert_candidates is
false (update the judge that reads expected_candidates to default to [] if
undefined and has_revert_candidates === false).
In `@eval/cases/case-006-archived-4.22-rejected/annotations.yaml`:
- Line 14: The job name in the structured list
("e2e-aws-ovn-techpreview-serial") does not match the job referenced in the
notes ("aws-ovn-techpreview-serial-3of3"); update either the list entry or the
note so the job name strings match exactly (choose one canonical name and
replace all occurrences of "e2e-aws-ovn-techpreview-serial" or
"aws-ovn-techpreview-serial-3of3" accordingly) to ensure consistent validation.
In `@eval/README.md`:
- Around line 62-63: The README line about the gh shim is ambiguous; update the
sentence describing eval/shims/gh to explicitly state its behavior with
EVAL_GH_CACHE: either "When EVAL_GH_CACHE is set, eval/shims/gh returns cached
responses from the EVAL_GH_CACHE directory (if a matching cached response
exists) and falls back to normal gh behavior if not" or "When EVAL_GH_CACHE is
set, eval/shims/gh always returns empty results (no network calls)", depending
on the actual implementation; reference the shim name eval/shims/gh and the
environment variable EVAL_GH_CACHE in the new wording so readers know exactly
which behavior to expect.
In `@eval/scripts/download-gcs-artifacts.sh`:
- Around line 10-14: Add explicit argument validation before using positional
parameters: check that both $1 and $2 are provided (the script computes PREFIX
from "${1%/}" and DEST from "$2"), and if either is missing print a short usage
message (e.g. "Usage: $0 <prefix> <dest>") to stderr and exit non‑zero. Place
this check immediately after the shebang/set -euo pipefail block and before
computing PREFIX and DEST so the script fails fast with a clear error rather
than proceeding with empty values.
In `@eval/scripts/extract-session-data.py`:
- Around line 22-26: In extract_api_data, the call to re.match(...).group(1)
(assigning version) can fail if payload_tag doesn't match; check the match
result before calling group()—e.g., capture the match into a variable
(version_match), verify it's not None, and either set a sensible default or
raise a clear error; similarly ensure stream_match is checked (already done) and
keep the variable names version and stream_match to locate the changes.
In `@plugins/ci/skills/fetch-payloads/fetch_payloads.py`:
- Around line 146-168: The merged-cache branch prints the merged list with
json.dumps(result) which lacks indentation and so is inconsistent with
single-cache (raw file content) and live-fetch (json.dumps(..., indent=2));
update the merged output to use the same pretty JSON formatting by replacing
_json.dumps(result) with _json.dumps(result, indent=2) (symbols: cached_files,
merged, result, and the _json import) so consumers receive consistent formatted
JSON.
- Around line 126-128: cache_file_name is built using args.version before the
script resolves a default version, causing filenames like
fetch-payloads-amd64-None-nightly.json; change the flow so you compute the
effective version (use the resolved variable version returned by
get_latest_version() when args.version is None) before constructing
cache_file_name and use that resolved variable (not args.version) when building
the filename; update references to cache_file_name and any subsequent uses to
rely on version instead of args.version.
---
Nitpick comments:
In `@eval/README.md`:
- Line 43: The README.md has multiple fenced code blocks without language
specifiers; update each fenced block (e.g., the blocks containing
"eval/archives/{payload-tag}/", "/ci:archive-payload-result
4.22.0-0.nightly-2026-03-20-053450", "eval/cases/{case-name}/", "/eval-mlflow
--action log-results --run-id <id>", and "eval/") to include an appropriate
language tag (use text for plain paths and bash for commands) so they become
```text or ```bash as appropriate to enable correct rendering and syntax
highlighting.
In `@eval/scripts/extract-session-data.py`:
- Around line 191-206: The loop over os.walk(tmpdir) uses an unused variable
named dirs; rename it to _dirs in the for root, dirs, files in os.walk(tmpdir)
header (i.e., change to for root, _dirs, files in os.walk(tmpdir)) to follow the
unused-variable convention and avoid linter warnings, ensuring no other
references to dirs exist in the surrounding function or scope.
- Around line 50-53: The JSON parsing silently drops all errors; change the
except to catch json.JSONDecodeError (or Exception if broader) and emit a
debug-level log including the exception and the offending line before
continuing. Use the existing logger (or create one via
logging.getLogger(__name__) if absent) and log a message like "Failed to parse
JSON line" with the exception and line content near the json.loads(line) call so
you can debug failed extractions without changing behavior.
- Around line 39-42: Rename the unused loop variable in the os.walk loop to
indicate it's intentionally unused: change the for loop header "for root, dirs,
files in os.walk(session_dir):" to use "_dirs" instead of "dirs" (i.e., "for
root, _dirs, files in os.walk(session_dir):") so the unused variable is clearly
marked; update any matching usages if present (there should be none) and run
tests/lint to ensure no unused-variable warnings remain.
In `@eval/shims/gcloud`:
- Around line 9-21: The find_real_gcloud function relies on the external
realpath command which may be missing on some systems; modify find_real_gcloud
to detect if realpath is available (e.g., with command -v realpath) and if not,
use a portable fallback to canonicalize paths (for example use readlink -f when
present, or a small python3/perl one-liner to resolve paths) and then use that
chosen resolver wherever realpath is currently invoked (both for "$candidate"
and "$self") so the comparison remains correct; ensure the resolver selection
happens once at function start and falls back to a no-op or default
(/usr/bin/gcloud) if no resolver exists.
In `@plugins/ci/commands/archive-payload-result.md`:
- Line 13: Several fenced code blocks in
plugins/ci/commands/archive-payload-result.md are missing language specifiers;
update each fenced block that contains shell commands (e.g. lines with
"/ci:archive-payload-result <payload-tag> ..." and examples like
"/ci:archive-payload-result 4.22.0-0.nightly-...") to use ```bash and add
```yaml for the block containing the humanurl mapping (humanurl:
https://prow.ci.openshift.org/...), and apply the same fixes to the other
example blocks referenced in the comment so all code fences include the
appropriate language identifier for proper rendering and syntax highlighting.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a67286ad-8da5-4c09-a8f2-870460a6565d
📒 Files selected for processing (27)
eval.yamleval/README.mdeval/cases/case-001-rejected-single-failure/annotations.yamleval/cases/case-001-rejected-single-failure/input.yamleval/cases/case-002-rejected-multiple-failures/annotations.yamleval/cases/case-002-rejected-multiple-failures/input.yamleval/cases/case-003-accepted-with-failures/annotations.yamleval/cases/case-003-accepted-with-failures/input.yamleval/cases/case-004-ready-in-progress/annotations.yamleval/cases/case-004-ready-in-progress/input.yamleval/cases/case-005-rejected-streak/annotations.yamleval/cases/case-005-rejected-streak/input.yamleval/cases/case-006-archived-4.22-rejected/annotations.yamleval/cases/case-006-archived-4.22-rejected/input.yamleval/cases/case-007-archived-4.22-ci-cco-revert/annotations.yamleval/cases/case-007-archived-4.22-ci-cco-revert/input.yamleval/cases/case-008-archived-4.22-ci-hypershift-revert/annotations.yamleval/cases/case-008-archived-4.22-ci-hypershift-revert/input.yamleval/cases/case-009-archived-4.22-cvo-revert/annotations.yamleval/cases/case-009-archived-4.22-cvo-revert/input.yamleval/scripts/download-gcs-artifacts.sheval/scripts/extract-session-data.pyeval/shims/gcloudeval/shims/ghplugins/ci/commands/archive-payload-result.mdplugins/ci/skills/fetch-new-prs-in-payload/fetch_new_prs_in_payload.pyplugins/ci/skills/fetch-payloads/fetch_payloads.py
✅ Files skipped from review due to trivial changes (13)
- eval/cases/case-002-rejected-multiple-failures/input.yaml
- eval/cases/case-004-ready-in-progress/annotations.yaml
- eval/cases/case-005-rejected-streak/input.yaml
- eval/cases/case-001-rejected-single-failure/input.yaml
- eval/cases/case-008-archived-4.22-ci-hypershift-revert/input.yaml
- eval/cases/case-007-archived-4.22-ci-cco-revert/annotations.yaml
- eval/cases/case-001-rejected-single-failure/annotations.yaml
- eval/cases/case-007-archived-4.22-ci-cco-revert/input.yaml
- eval/shims/gh
- eval/cases/case-002-rejected-multiple-failures/annotations.yaml
- eval/cases/case-009-archived-4.22-cvo-revert/annotations.yaml
- eval/cases/case-004-ready-in-progress/input.yaml
- eval/cases/case-005-rejected-streak/annotations.yaml
Comment on lines
+56
to
+57
| No test cases exist yet. Run /eval-dataset to generate cases with real | ||
| payload tags from recent CI history. |
Contributor
There was a problem hiding this comment.
Inconsistent claim about test cases.
The comment states "No test cases exist yet" but this PR adds eval/cases/case-003-accepted-with-failures/, indicating that test cases do exist. Please update this comment to reflect the current state.
📝 Suggested fix
- No test cases exist yet. Run /eval-dataset to generate cases with real
- payload tags from recent CI history.
+ Test cases in eval/cases/ contain real payload tags from OpenShift CI
+ history. Run /eval-dataset to generate additional cases.📝 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.
Suggested change
| No test cases exist yet. Run /eval-dataset to generate cases with real | |
| payload tags from recent CI history. | |
| Test cases in eval/cases/ contain real payload tags from OpenShift CI | |
| history. Run /eval-dataset to generate additional cases. |
🤖 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 `@eval.yaml` around lines 56 - 57, Update the misleading comment in eval.yaml
that claims "No test cases exist yet" to reflect that test cases now exist
(specifically the added directory eval/cases/case-003-accepted-with-failures),
e.g., change the sentence to note that test cases have been added and mention
the new case path; edit the string in eval.yaml where that sentence appears so
it accurately describes current state.
Comment on lines
+1
to
+10
| expected_phase: Accepted | ||
| expected_failed_job_count: 1 | ||
| has_revert_candidates: false | ||
| force_accept_expected: false | ||
| notes: > | ||
| Edge case: payload was Accepted despite 1 failed blocking job | ||
| (hypershift-ovn-conformance-4.20). Tests that the skill correctly handles | ||
| force-accepted payloads — it should still analyze the failed job and note | ||
| that the payload was accepted despite the failure. Should NOT recommend | ||
| force-accept since the payload was already accepted. |
Contributor
There was a problem hiding this comment.
Missing expected_candidates field for revert scoring judge.
The revert_scoring_accuracy judge (eval.yaml lines 247-294) expects annotations to include expected_candidates with fields like pr_url, min_confidence, expected_confidence, and expected_failing_jobs (lines 273-277). This annotations file doesn't include that field.
Since has_revert_candidates: false, you should either:
- Add
expected_candidates: []to explicitly indicate no candidates expected, or - Update the judge to handle missing
expected_candidateswhenhas_revert_candidatesis false
📝 Suggested fix
expected_phase: Accepted
expected_failed_job_count: 1
has_revert_candidates: false
+expected_candidates: []
force_accept_expected: false
notes: >📝 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.
Suggested change
| expected_phase: Accepted | |
| expected_failed_job_count: 1 | |
| has_revert_candidates: false | |
| force_accept_expected: false | |
| notes: > | |
| Edge case: payload was Accepted despite 1 failed blocking job | |
| (hypershift-ovn-conformance-4.20). Tests that the skill correctly handles | |
| force-accepted payloads — it should still analyze the failed job and note | |
| that the payload was accepted despite the failure. Should NOT recommend | |
| force-accept since the payload was already accepted. | |
| expected_phase: Accepted | |
| expected_failed_job_count: 1 | |
| has_revert_candidates: false | |
| expected_candidates: [] | |
| force_accept_expected: false | |
| notes: > | |
| Edge case: payload was Accepted despite 1 failed blocking job | |
| (hypershift-ovn-conformance-4.20). Tests that the skill correctly handles | |
| force-accepted payloads — it should still analyze the failed job and note | |
| that the payload was accepted despite the failure. Should NOT recommend | |
| force-accept since the payload was already accepted. |
🤖 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 `@eval/cases/case-003-accepted-with-failures/annotations.yaml` around lines 1 -
10, The annotations file is missing the expected_candidates field that the
revert_scoring_accuracy judge expects; either add an explicit empty list by
inserting expected_candidates: [] alongside has_revert_candidates: false in the
annotations (so the judge sees an explicit no-candidates case), or modify the
revert_scoring_accuracy judge logic to treat a missing expected_candidates the
same as an empty list when has_revert_candidates is false (update the judge that
reads expected_candidates to default to [] if undefined and
has_revert_candidates === false).
| description: "Replaced OLM-based Istio install with Sail Library, causing GatewayAPIController failures" | ||
| expected_failing_jobs: | ||
| - "e2e-aws-ovn-techpreview" | ||
| - "e2e-aws-ovn-techpreview-serial" |
Contributor
There was a problem hiding this comment.
Job name inconsistency between structured data and notes.
Line 14 lists the job as "e2e-aws-ovn-techpreview-serial", but line 27 in the notes refers to it as "aws-ovn-techpreview-serial-3of3". These names should match exactly to avoid confusion during test validation.
📝 Proposed fix to align the job name
If the actual job name includes the -3of3 suffix and the e2e- prefix, update line 14:
- - "e2e-aws-ovn-techpreview-serial"
+ - "e2e-aws-ovn-techpreview-serial-3of3"Or, if the notes should match the structured data, update line 27:
- blocking jobs: aws-ovn-techpreview (test failure: GatewayAPI/OSSM Sail
- Library migration), aws-ovn-techpreview-serial-3of3 (same root cause),
+ blocking jobs: e2e-aws-ovn-techpreview (test failure: GatewayAPI/OSSM Sail
+ Library migration), e2e-aws-ovn-techpreview-serial (same root cause),Also applies to: 27-27
🤖 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 `@eval/cases/case-006-archived-4.22-rejected/annotations.yaml` at line 14, The
job name in the structured list ("e2e-aws-ovn-techpreview-serial") does not
match the job referenced in the notes ("aws-ovn-techpreview-serial-3of3");
update either the list entry or the note so the job name strings match exactly
(choose one canonical name and replace all occurrences of
"e2e-aws-ovn-techpreview-serial" or "aws-ovn-techpreview-serial-3of3"
accordingly) to ensure consistent validation.
Comment on lines
+10
to
+14
| set -euo pipefail | ||
|
|
||
| BUCKET="test-platform-results" | ||
| PREFIX="${1%/}" | ||
| DEST="$2" |
Contributor
There was a problem hiding this comment.
Missing argument validation.
The script uses positional parameters $1 and $2 without validating that they were provided. If a user runs the script without arguments, PREFIX and DEST will be empty, leading to errors or unexpected behavior.
🛡️ Proposed fix
set -euo pipefail
+if [[ $# -lt 2 ]]; then
+ echo "Usage: $0 <gcs-prefix> <dest-dir>" >&2
+ exit 1
+fi
+
BUCKET="test-platform-results"
PREFIX="${1%/}"
DEST="$2"🤖 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 `@eval/scripts/download-gcs-artifacts.sh` around lines 10 - 14, Add explicit
argument validation before using positional parameters: check that both $1 and
$2 are provided (the script computes PREFIX from "${1%/}" and DEST from "$2"),
and if either is missing print a short usage message (e.g. "Usage: $0 <prefix>
<dest>") to stderr and exit non‑zero. Place this check immediately after the
shebang/set -euo pipefail block and before computing PREFIX and DEST so the
script fails fast with a clear error rather than proceeding with empty values.
Comment on lines
+126
to
+128
| cache_dir = os.environ.get("EVAL_CACHED_RESPONSES") | ||
| archives_dir = os.environ.get("EVAL_ARCHIVES_DIR") | ||
| cache_file_name = f"fetch-payloads-{args.architecture}-{args.version}-{args.stream}.json" |
Contributor
There was a problem hiding this comment.
Cache filename uses version before it's resolved.
Line 128 constructs cache_file_name using args.version, but args.version can be None (when not provided as a CLI argument). The version is resolved to the latest Sippy version later at lines 180-181. This will result in a cache filename like fetch-payloads-amd64-None-nightly.json when no version is specified.
🐛 Proposed fix
args = parser.parse_args()
+ # Resolve version early for cache lookup
+ version = args.version
+ if version is None:
+ version = get_latest_version()
+
cache_dir = os.environ.get("EVAL_CACHED_RESPONSES")
archives_dir = os.environ.get("EVAL_ARCHIVES_DIR")
- cache_file_name = f"fetch-payloads-{args.architecture}-{args.version}-{args.stream}.json"
+ cache_file_name = f"fetch-payloads-{args.architecture}-{version}-{args.stream}.json"And later, use the resolved version instead of calling get_latest_version() again:
architecture = args.architecture
if architecture not in KNOWN_ARCHITECTURES:
print(
f"Error: Unknown architecture '{architecture}'. "
f"Known architectures: {', '.join(KNOWN_ARCHITECTURES)}",
file=sys.stderr,
)
sys.exit(1)
- version = args.version
- if version is None:
- version = get_latest_version()
-
stream = args.stream🤖 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 `@plugins/ci/skills/fetch-payloads/fetch_payloads.py` around lines 126 - 128,
cache_file_name is built using args.version before the script resolves a default
version, causing filenames like fetch-payloads-amd64-None-nightly.json; change
the flow so you compute the effective version (use the resolved variable version
returned by get_latest_version() when args.version is None) before constructing
cache_file_name and use that resolved variable (not args.version) when building
the filename; update references to cache_file_name and any subsequent uses to
rely on version instead of args.version.
Comment on lines
+146
to
+168
| if len(cached_files) == 1: | ||
| with open(cached_files[0], "r") as f: | ||
| print(f.read(), end="") | ||
| sys.exit(0) | ||
| else: | ||
| # Merge multiple caches: deduplicate by tag, keep the most | ||
| # complete entry (largest JSON) for each tag. | ||
| import json as _json | ||
| merged = {} | ||
| for cf in cached_files: | ||
| with open(cf, "r") as f: | ||
| try: | ||
| data = _json.load(f) | ||
| except _json.JSONDecodeError: | ||
| continue | ||
| for entry in data: | ||
| tag = entry.get("tag", "") | ||
| existing = merged.get(tag) | ||
| if existing is None or len(_json.dumps(entry)) > len(_json.dumps(existing)): | ||
| merged[tag] = entry | ||
| result = sorted(merged.values(), key=lambda e: e.get("tag", ""), reverse=True) | ||
| print(_json.dumps(result)) | ||
| sys.exit(0) |
Contributor
There was a problem hiding this comment.
Merged cache output format differs from single-cache and live-fetch formats.
When merging multiple caches (line 167), the output uses json.dumps(result) without indentation. However:
- Single cache (line 148) prints the raw file content, which likely has indentation
- Live fetch (line 234) uses
json.dumps(output, indent=2)
This inconsistency may break consumers expecting consistent formatting.
🔧 Proposed fix for consistency
result = sorted(merged.values(), key=lambda e: e.get("tag", ""), reverse=True)
- print(_json.dumps(result))
+ print(_json.dumps(result, indent=2))
sys.exit(0)📝 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.
Suggested change
| if len(cached_files) == 1: | |
| with open(cached_files[0], "r") as f: | |
| print(f.read(), end="") | |
| sys.exit(0) | |
| else: | |
| # Merge multiple caches: deduplicate by tag, keep the most | |
| # complete entry (largest JSON) for each tag. | |
| import json as _json | |
| merged = {} | |
| for cf in cached_files: | |
| with open(cf, "r") as f: | |
| try: | |
| data = _json.load(f) | |
| except _json.JSONDecodeError: | |
| continue | |
| for entry in data: | |
| tag = entry.get("tag", "") | |
| existing = merged.get(tag) | |
| if existing is None or len(_json.dumps(entry)) > len(_json.dumps(existing)): | |
| merged[tag] = entry | |
| result = sorted(merged.values(), key=lambda e: e.get("tag", ""), reverse=True) | |
| print(_json.dumps(result)) | |
| sys.exit(0) | |
| if len(cached_files) == 1: | |
| with open(cached_files[0], "r") as f: | |
| print(f.read(), end="") | |
| sys.exit(0) | |
| else: | |
| # Merge multiple caches: deduplicate by tag, keep the most | |
| # complete entry (largest JSON) for each tag. | |
| import json as _json | |
| merged = {} | |
| for cf in cached_files: | |
| with open(cf, "r") as f: | |
| try: | |
| data = _json.load(f) | |
| except _json.JSONDecodeError: | |
| continue | |
| for entry in data: | |
| tag = entry.get("tag", "") | |
| existing = merged.get(tag) | |
| if existing is None or len(_json.dumps(entry)) > len(_json.dumps(existing)): | |
| merged[tag] = entry | |
| result = sorted(merged.values(), key=lambda e: e.get("tag", ""), reverse=True) | |
| print(_json.dumps(result, indent=2)) | |
| sys.exit(0) |
🤖 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 `@plugins/ci/skills/fetch-payloads/fetch_payloads.py` around lines 146 - 168,
The merged-cache branch prints the merged list with json.dumps(result) which
lacks indentation and so is inconsistent with single-cache (raw file content)
and live-fetch (json.dumps(..., indent=2)); update the merged output to use the
same pretty JSON formatting by replacing _json.dumps(result) with
_json.dumps(result, indent=2) (symbols: cached_files, merged, result, and the
_json import) so consumers receive consistent formatted JSON.
- compress-archives.sh: compress/decompress payload archives to tar.gz - trim-archives.sh: remove heavy files (audit logs, metrics, observers) while preserving directory structure for realistic eval runs - gcloud shim: transparent extraction of compressed archives on demand with flock to prevent concurrent extraction races - Install failure eval case for metal-ipi-ovn-ipv6 CKAO#2032 root cause - TODO.md tracking remaining eval framework tasks Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
New eval cases from payload agent analysis of 331 jobs: - case-010: CNO NetworkPolicy regression (true positive, cross-platform) - case-011: CMO monitoring regression (true positive, multi-payload) - case-012: HyperShift builder image (false positive, cloud platform issue) - case-013: NTO testdata embedding (true positive, build-system change) - case-014: Infrastructure-only rejection (no candidates expected) Reports: - payload-agent-analysis.md: 66.7% precision (10/15 TPs), 33% FP rate, only 2.5% of rejected payloads get high-confidence candidates - improvement-recommendations.md: 8-priority action plan targeting detection gap, sub-component awareness, cross-run state, and cloud platform issue detection Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Added case descriptions for cases 010-014 (CNO, CMO, HyperShift FP, NTO, infra-only) to the test cases table - Updated TODO.md: Tasks 0,1,3,4,5,6 complete; Tasks 2,7 in progress Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The archive skill now derives everything from the original Claude session tarball instead of calling live APIs. Adds curl shim, fixes extract script to handle persisted-output references and object-format fetch_payloads responses, and consolidates env vars to single EVAL_ARCHIVES_DIR. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
plugins/ci/skills/fetch-payloads/fetch_payloads.py (2)
161-161:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMerged cache output format differs from single-cache and live-fetch formats.
The merged cache path (line 161) uses
json.dumps(result)without indentation, while single-cache (line 142) prints raw file content (likely indented) and live-fetch (line 228) usesindent=2. This formatting inconsistency may break consumers expecting uniform JSON.🔧 Proposed fix
result = sorted(merged.values(), key=lambda e: e.get("tag", ""), reverse=True) - print(_json.dumps(result)) + print(_json.dumps(result, indent=2)) sys.exit(0)🤖 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 `@plugins/ci/skills/fetch-payloads/fetch_payloads.py` at line 161, The merged-cache output uses _json.dumps(result) without indentation, causing inconsistent JSON formatting versus single-cache (raw file content) and live-fetch (which uses indent=2); update the merged-cache print call (the print of _json.dumps in fetch_payloads.py where merged results are emitted) to produce pretty-printed JSON (e.g., include indent=2) so consumers receive the same formatted JSON as live-fetch.
127-127:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCache filename uses
args.versionbefore it's resolved.When
args.versionisNone(the default), the cache filename will befetch-payloads-{arch}-None-{stream}.json. The actual version is resolved later at lines 173-175. This breaks cache lookups when the--versionflag is omitted.🐛 Proposed fix
Resolve the version early, before constructing the cache filename:
args = parser.parse_args() + # Resolve version early for cache lookup + version = args.version if args.version is not None else get_latest_version() + archives_dir = os.environ.get("EVAL_ARCHIVES_DIR") - cache_file_name = f"fetch-payloads-{args.architecture}-{args.version}-{args.stream}.json" + cache_file_name = f"fetch-payloads-{args.architecture}-{version}-{args.stream}.json"Then update lines 173-175 to use the already-resolved
version:architecture = args.architecture if architecture not in KNOWN_ARCHITECTURES: print( f"Error: Unknown architecture '{architecture}'. " f"Known architectures: {', '.join(KNOWN_ARCHITECTURES)}", file=sys.stderr, ) sys.exit(1) - version = args.version - if version is None: - version = get_latest_version() - stream = args.stream🤖 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 `@plugins/ci/skills/fetch-payloads/fetch_payloads.py` at line 127, The cache filename is built using args.version before it is resolved, causing "None" in the filename when --version is omitted; move the version-resolution logic (the code that computes the resolved variable currently run at lines 173-175, producing a local variable like version) to before the cache_file_name assignment and then construct cache_file_name using that resolved version variable (e.g., version) instead of args.version, and remove or reuse the later redundant resolution so code uses the single resolved variable throughout (refer to cache_file_name and the version-resolution code block around lines 173-175 in fetch_payloads.py).
🧹 Nitpick comments (6)
eval/README.md (1)
40-62: 💤 Low valueAdd language specifiers to fenced code blocks.
The static analysis tool correctly identified that several fenced code blocks are missing language specifiers (lines 40, 125, 137, 368, 410). Adding appropriate language identifiers improves syntax highlighting and readability.
📝 Suggested additions
Line 40 (directory structure):
-``` +```text archives/{payload-tag}/Line 125 (command example):
-``` +```bash /ci:archive-payload-result 4.22.0-0.nightly-2026-03-20-053450Line 137 (directory structure):
-``` +```text eval/cases/{case-name}/Line 368 (command example):
-``` +```bash /eval-mlflow --action log-results --run-id <id>Line 410 (directory structure):
-``` +```text eval/🤖 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 `@eval/README.md` around lines 40 - 62, The README has multiple fenced code blocks missing language specifiers (e.g., the directory tree starting with "archives/{payload-tag}/", the command lines like "/ci:archive-payload-result 4.22.0-0.nightly-2026-03-20-053450" and "/eval-mlflow --action log-results --run-id <id>", and other directory examples such as "eval/cases/{case-name}/" and "eval/")—update each fence to include an appropriate language tag (use "text" for plain directory listings and "bash" for command examples) so the blocks around those snippets render correctly; search for the shown snippet strings in README.md and replace the opening ``` with ```text or ```bash as appropriate.eval/shims/gh (1)
60-60: ⚡ Quick winDeclare temporary variables as local.
The variables
pr_numandrepoare used only within theviewcase block but are not declared withlocal, which pollutes the global scope.♻️ Proposed fix
view) # Try to find cached response # Parse: gh pr view <number> --repo <org/repo> [--json ...] - pr_num="" repo="" + local pr_num="" repo="" shift 2🤖 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 `@eval/shims/gh` at line 60, In the "view" case block declare the temporary variables pr_num and repo as local to avoid polluting global scope: locate the case handling code that currently assigns pr_num="" and repo="" and change those declarations to use local variables (e.g., local pr_num and local repo) so they are scoped to the view branch only.plugins/ci/skills/archive-payload-result/SKILL.md (1)
131-134: 💤 Low valueAdd language identifier to code fence.
The code fence should specify
yamlfor proper syntax highlighting.📝 Proposed fix
2. Parse each JUnit XML to extract underlying job URLs from `<system-out>` blocks. The `<system-out>` contains YAML with a `humanurl` field: - ``` + ```yaml humanurl: https://prow.ci.openshift.org/view/gs/test-platform-results/logs/{underlying-job-name}/{build-id} ```🤖 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 `@plugins/ci/skills/archive-payload-result/SKILL.md` around lines 131 - 134, Add the YAML language identifier to the code fence showing the sample snippet containing the humanurl (the fenced block that currently contains "humanurl: https://prow.ci.openshift.org/view/gs/test-platform-results/logs/{underlying-job-name}/{build-id}") so the fence becomes ```yaml and enables proper syntax highlighting; update the fenced block surrounding the humanurl example accordingly.plugins/ci/skills/archive-payload-result/extract_session_data.py (3)
293-293: 💤 Low valueClarify operator precedence with parentheses.
The expression mixes
orandandoperators without explicit grouping, which can reduce readability even though Python's precedence rules make the intent clear.♻️ Proposed fix
- if cmd.strip().startswith("gh ") or "&&" in cmd and "gh " in cmd: + if cmd.strip().startswith("gh ") or ("&&" in cmd and "gh " in cmd): _extract_gh_responses(cmd, rc, output_dir, found)🤖 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 `@plugins/ci/skills/archive-payload-result/extract_session_data.py` at line 293, The conditional mixes or and and without grouping, which hampers readability; update the condition that uses cmd (the expression calling cmd.strip().startswith("gh ")) to include explicit parentheses so the intended grouping is clear — e.g., group the "&&" and "gh " checks together as ("&&" in cmd and "gh " in cmd) while leaving cmd.strip().startswith("gh ") as the other operand — so the logic in that if statement is unambiguous.
165-165: 💤 Low valueRename unused loop variables.
The loop variables
dirsandfilesare not used within the loop body. By convention, unused variables should be prefixed with_to signal intent.♻️ Proposed fix
- for root, dirs, files in os.walk(session_dir): + for root, _dirs, _files in os.walk(session_dir): candidate = os.path.join(root, tr_match.group(1))Apply the same fix to lines 190 and 378.
🤖 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 `@plugins/ci/skills/archive-payload-result/extract_session_data.py` at line 165, The for-loop unpacks os.walk into variables that aren't used; rename the unused loop variables dirs and files to _dirs and _files (e.g., change "for root, dirs, files in os.walk(session_dir):" to "for root, _dirs, _files in os.walk(session_dir):") to signal they are intentionally unused, and apply the same renaming to the other two identical os.walk loops in this module.
203-204: ⚡ Quick winConsider logging or narrowing the exception handler.
The bare
except Exceptionwithout logging makes it impossible to debug why certain lines are skipped. Either narrow tojson.JSONDecodeErroror add logging.♻️ Proposed improvements
Option 1 (narrow the exception):
try: msg = json.loads(line) - except Exception: + except json.JSONDecodeError: continueOption 2 (add logging):
try: msg = json.loads(line) except Exception as e: + print(f" WARNING: Failed to parse line: {e}", file=sys.stderr) continue🤖 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 `@plugins/ci/skills/archive-payload-result/extract_session_data.py` around lines 203 - 204, The bare "except Exception: continue" in extract_session_data.py should be changed so failures to parse a line are not silently dropped: either narrow the handler to "except json.JSONDecodeError as e" (and ensure json is imported) or keep a broader except but add logging (e.g., process_logger.warning or logging.exception) that includes the exception and the offending line. Locate the exact "except Exception: continue" snippet and replace it with one of these options so parsing errors are both specific and observable.
🤖 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 `@eval-install-failure.yaml`:
- Around line 236-241: The fallback partial-match is too permissive: when
expected_stage contains multiple words the current logic (stage_words/partial
using any w in all_text_lower) flags a match if any single long token appears.
Change it so multi-word expected_stage requires a stricter condition — e.g.,
split expected_stage into stage_words and for len(stage_words) > 1 require at
least N (recommend 2 or ceil(len(stage_words)/2)) distinct long-word matches in
all_text_lower (using whole-word checks or regex word boundaries) before
returning the partial match; keep the original behavior for single-word stages.
Update the variables referenced (stage_found, expected_stage, stage_words,
partial, all_text_lower) accordingly.
- Around line 100-116: The current output_files_exist logic (variables report,
installer_summary, bundle_summary, analysis_files, transcript) treats any file
under an /analysis/ path or certain transcript text as equivalent to the
required artifacts, weakening the check; update the function so that only
explicitly-named artifact files satisfy the check: consider analysis_files valid
only if they are the same required filenames (e.g., end with "/report.txt",
"/installer-summary.txt", or "/log-bundle-summary.txt"), remove the branch that
returns True based on transcript content, and only return True when at least one
of report, installer_summary, or bundle_summary (or matching analysis_files by
exact filename) is present; otherwise return False with the missing list as
before.
In `@eval.yaml`:
- Around line 36-38: The dataset at eval/cases is picking up install-failure
fixtures that lack payloads, breaking resolution of arguments "{payload_tag}";
add a case_pattern under dataset to exclude those (e.g. a regex that excludes
names starting with "case-install-" or only matches payload-bearing fixtures) so
the eval only loads payload cases; update the dataset block (dataset.path:
eval/cases) to include case_pattern: with the appropriate negative or positive
regex to filter out case-install-* fixtures.
- Around line 161-171: The current check treats an empty JSON array as a hard
failure; change that so an empty list is accepted as a valid "no candidates"
outcome: replace the block that returns (False, "JSON array is empty") when
len(data) == 0 with a non-failure return (e.g., (True, "No rows / no
candidates")) or otherwise allow processing to continue for zero-length data.
Update the logic around variables data, required, row, and missing so you only
attempt to validate row fields when len(data) > 0 (i.e., check 'row = data[0]'
and the missing-fields check only when data is non-empty).
In `@eval/cases/case-013-5.0-nightly-nto-testdata-revert/annotations.yaml`:
- Line 1: The expected_phase field currently contains an empty string which
violates the eval.yaml schema (allowed values: Rejected, Ready, Accepted);
update the expected_phase value in annotations.yaml (field name: expected_phase)
to one of those three literals—use Rejected if the payload was rejected, Ready
if it is staged/awaiting acceptance, or Accepted if it was formally accepted—so
the file validates against the schema.
In `@eval/scripts/trim-archives.sh`:
- Line 115: The script uses GNU-only "stat --printf='%s' \"$file\"" to set the
variable size (seen in the size= assignment), which breaks on macOS/BSD; replace
both occurrences (the two size= assignments) with a portable fallback: try GNU
stat (--printf='%s'), then BSD stat (-f '%z'), and as a last resort use wc -c,
capturing the output safely and defaulting to 0 on error. Implement this as a
small helper snippet (or inline conditional) that sets size reliably for $file
and mirrors the approach used in compress-archives.sh's fallback.
In `@eval/shims/curl`:
- Around line 80-82: The case branch incorrectly groups --compressed (no arg)
with --retry and --retry-delay (which require arguments) and performs a single
shift; update the option handling so that the --compressed branch consumes only
the option (single shift) while the --retry and --retry-delay branches each
consume the option plus its argument (shift twice or shift 2), referencing the
existing case pattern handling for --compressed, --retry and --retry-delay so
their arguments are not treated as the URL.
In `@eval/TODO.md`:
- Line 5: Fix the typographical errors in the TODO content by replacing the
misspelled tokens: change "peformance" to "performance", "recocmend" to
"recommend", "one off" (when used as an adjective) to "one-off", and "insatll"
to "install" wherever they appear (notably the occurrences matching those tokens
in the document).
In `@plugins/ci/skills/archive-payload-result/extract_session_data.py`:
- Line 375: The tar extraction call tf.extractall(tmpdir) is vulnerable to path
traversal; update the extraction logic that creates/uses the TarFile object (tf)
and tmpdir to validate member paths before writing: either pass a safe filter
function to TarFile.extractall (Python 3.12+), or implement a manual check that
constructs the absolute destination for each TarInfo member and ensures it is
inside tmpdir (reject members with absolute paths or path components like ..),
then only extract validated members; reference the TarFile variable tf and the
target directory tmpdir when making these changes.
---
Duplicate comments:
In `@plugins/ci/skills/fetch-payloads/fetch_payloads.py`:
- Line 161: The merged-cache output uses _json.dumps(result) without
indentation, causing inconsistent JSON formatting versus single-cache (raw file
content) and live-fetch (which uses indent=2); update the merged-cache print
call (the print of _json.dumps in fetch_payloads.py where merged results are
emitted) to produce pretty-printed JSON (e.g., include indent=2) so consumers
receive the same formatted JSON as live-fetch.
- Line 127: The cache filename is built using args.version before it is
resolved, causing "None" in the filename when --version is omitted; move the
version-resolution logic (the code that computes the resolved variable currently
run at lines 173-175, producing a local variable like version) to before the
cache_file_name assignment and then construct cache_file_name using that
resolved version variable (e.g., version) instead of args.version, and remove or
reuse the later redundant resolution so code uses the single resolved variable
throughout (refer to cache_file_name and the version-resolution code block
around lines 173-175 in fetch_payloads.py).
---
Nitpick comments:
In `@eval/README.md`:
- Around line 40-62: The README has multiple fenced code blocks missing language
specifiers (e.g., the directory tree starting with "archives/{payload-tag}/",
the command lines like "/ci:archive-payload-result
4.22.0-0.nightly-2026-03-20-053450" and "/eval-mlflow --action log-results
--run-id <id>", and other directory examples such as "eval/cases/{case-name}/"
and "eval/")—update each fence to include an appropriate language tag (use
"text" for plain directory listings and "bash" for command examples) so the
blocks around those snippets render correctly; search for the shown snippet
strings in README.md and replace the opening ``` with ```text or ```bash as
appropriate.
In `@eval/shims/gh`:
- Line 60: In the "view" case block declare the temporary variables pr_num and
repo as local to avoid polluting global scope: locate the case handling code
that currently assigns pr_num="" and repo="" and change those declarations to
use local variables (e.g., local pr_num and local repo) so they are scoped to
the view branch only.
In `@plugins/ci/skills/archive-payload-result/extract_session_data.py`:
- Line 293: The conditional mixes or and and without grouping, which hampers
readability; update the condition that uses cmd (the expression calling
cmd.strip().startswith("gh ")) to include explicit parentheses so the intended
grouping is clear — e.g., group the "&&" and "gh " checks together as ("&&" in
cmd and "gh " in cmd) while leaving cmd.strip().startswith("gh ") as the other
operand — so the logic in that if statement is unambiguous.
- Line 165: The for-loop unpacks os.walk into variables that aren't used; rename
the unused loop variables dirs and files to _dirs and _files (e.g., change "for
root, dirs, files in os.walk(session_dir):" to "for root, _dirs, _files in
os.walk(session_dir):") to signal they are intentionally unused, and apply the
same renaming to the other two identical os.walk loops in this module.
- Around line 203-204: The bare "except Exception: continue" in
extract_session_data.py should be changed so failures to parse a line are not
silently dropped: either narrow the handler to "except json.JSONDecodeError as
e" (and ensure json is imported) or keep a broader except but add logging (e.g.,
process_logger.warning or logging.exception) that includes the exception and the
offending line. Locate the exact "except Exception: continue" snippet and
replace it with one of these options so parsing errors are both specific and
observable.
In `@plugins/ci/skills/archive-payload-result/SKILL.md`:
- Around line 131-134: Add the YAML language identifier to the code fence
showing the sample snippet containing the humanurl (the fenced block that
currently contains "humanurl:
https://prow.ci.openshift.org/view/gs/test-platform-results/logs/{underlying-job-name}/{build-id}")
so the fence becomes ```yaml and enables proper syntax highlighting; update the
fenced block surrounding the humanurl example accordingly.
🪄 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: Enterprise
Run ID: 6a4aabf2-5aca-47b9-8521-7fe965ce2355
📒 Files selected for processing (29)
.claude-plugin/marketplace.jsondocs/data.jsoneval-install-failure.yamleval.yamleval/README.mdeval/TODO.mdeval/cases/case-010-5.0-ci-cno-networkpolicy-revert/annotations.yamleval/cases/case-010-5.0-ci-cno-networkpolicy-revert/input.yamleval/cases/case-011-5.0-nightly-cmo-monitoring-revert/annotations.yamleval/cases/case-011-5.0-nightly-cmo-monitoring-revert/input.yamleval/cases/case-012-5.0-ci-hypershift-builder-fp/annotations.yamleval/cases/case-012-5.0-ci-hypershift-builder-fp/input.yamleval/cases/case-013-5.0-nightly-nto-testdata-revert/annotations.yamleval/cases/case-013-5.0-nightly-nto-testdata-revert/input.yamleval/cases/case-014-5.0-ci-infra-only-no-candidates/annotations.yamleval/cases/case-014-5.0-ci-infra-only-no-candidates/input.yamleval/cases/case-install-001-metal-ipi-ipv6-ckao/annotations.yamleval/cases/case-install-001-metal-ipi-ipv6-ckao/input.yamleval/reports/improvement-recommendations.mdeval/reports/payload-agent-analysis.mdeval/scripts/compress-archives.sheval/scripts/trim-archives.sheval/shims/curleval/shims/gcloudeval/shims/ghplugins/ci/.claude-plugin/plugin.jsonplugins/ci/skills/archive-payload-result/SKILL.mdplugins/ci/skills/archive-payload-result/extract_session_data.pyplugins/ci/skills/fetch-payloads/fetch_payloads.py
✅ Files skipped from review due to trivial changes (11)
- eval/cases/case-013-5.0-nightly-nto-testdata-revert/input.yaml
- eval/cases/case-011-5.0-nightly-cmo-monitoring-revert/input.yaml
- plugins/ci/.claude-plugin/plugin.json
- eval/cases/case-010-5.0-ci-cno-networkpolicy-revert/input.yaml
- eval/cases/case-014-5.0-ci-infra-only-no-candidates/annotations.yaml
- eval/cases/case-install-001-metal-ipi-ipv6-ckao/input.yaml
- eval/cases/case-install-001-metal-ipi-ipv6-ckao/annotations.yaml
- eval/cases/case-014-5.0-ci-infra-only-no-candidates/input.yaml
- eval/cases/case-011-5.0-nightly-cmo-monitoring-revert/annotations.yaml
- eval/reports/payload-agent-analysis.md
- eval/cases/case-012-5.0-ci-hypershift-builder-fp/input.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- eval/shims/gcloud
Comment on lines
+100
to
+116
| report = [k for k in all_f if k.endswith("/report.txt") and "prow-job-analyze-install-failure" in k] | ||
| installer_summary = [k for k in all_f if k.endswith("/installer-summary.txt") and "prow-job-analyze-install-failure" in k] | ||
| bundle_summary = [k for k in all_f if k.endswith("/log-bundle-summary.txt") and "prow-job-analyze-install-failure" in k] | ||
| # Also check for any analysis files in the .work tree | ||
| analysis_files = [k for k in all_f if "prow-job-analyze-install-failure" in k and "/analysis/" in k] | ||
| missing = [] | ||
| if not report and not analysis_files: | ||
| missing.append("analysis report (report.txt or any analysis file)") | ||
| if not installer_summary and not bundle_summary and not analysis_files: | ||
| missing.append("installer-summary.txt or log-bundle-summary.txt") | ||
| if missing: | ||
| # Check conversation transcript as fallback — the skill may | ||
| # present analysis inline rather than writing files | ||
| transcript = outputs.get("transcript", "") | ||
| if "Failure Stage" in transcript or "failure stage" in transcript.lower() or "root cause" in transcript.lower(): | ||
| return (True, "Analysis found in conversation transcript (no separate files)") | ||
| return (False, f"Missing: {', '.join(missing)}") |
Contributor
There was a problem hiding this comment.
Keep output_files_exist strict about file artifacts.
This check currently passes even when no report files are written, either because any file exists under /analysis/ or because the transcript mentions a failure stage. That weakens the contract enough that a file-generation regression can still clear output_files_exist.
Suggested fix
- # Also check for any analysis files in the .work tree
- analysis_files = [k for k in all_f if "prow-job-analyze-install-failure" in k and "/analysis/" in k]
missing = []
- if not report and not analysis_files:
- missing.append("analysis report (report.txt or any analysis file)")
- if not installer_summary and not bundle_summary and not analysis_files:
+ if not report:
+ missing.append("analysis report (report.txt)")
+ if not installer_summary and not bundle_summary:
missing.append("installer-summary.txt or log-bundle-summary.txt")
if missing:
- # Check conversation transcript as fallback — the skill may
- # present analysis inline rather than writing files
- transcript = outputs.get("transcript", "")
- if "Failure Stage" in transcript or "failure stage" in transcript.lower() or "root cause" in transcript.lower():
- return (True, "Analysis found in conversation transcript (no separate files)")
return (False, f"Missing: {', '.join(missing)}")📝 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.
Suggested change
| report = [k for k in all_f if k.endswith("/report.txt") and "prow-job-analyze-install-failure" in k] | |
| installer_summary = [k for k in all_f if k.endswith("/installer-summary.txt") and "prow-job-analyze-install-failure" in k] | |
| bundle_summary = [k for k in all_f if k.endswith("/log-bundle-summary.txt") and "prow-job-analyze-install-failure" in k] | |
| # Also check for any analysis files in the .work tree | |
| analysis_files = [k for k in all_f if "prow-job-analyze-install-failure" in k and "/analysis/" in k] | |
| missing = [] | |
| if not report and not analysis_files: | |
| missing.append("analysis report (report.txt or any analysis file)") | |
| if not installer_summary and not bundle_summary and not analysis_files: | |
| missing.append("installer-summary.txt or log-bundle-summary.txt") | |
| if missing: | |
| # Check conversation transcript as fallback — the skill may | |
| # present analysis inline rather than writing files | |
| transcript = outputs.get("transcript", "") | |
| if "Failure Stage" in transcript or "failure stage" in transcript.lower() or "root cause" in transcript.lower(): | |
| return (True, "Analysis found in conversation transcript (no separate files)") | |
| return (False, f"Missing: {', '.join(missing)}") | |
| report = [k for k in all_f if k.endswith("/report.txt") and "prow-job-analyze-install-failure" in k] | |
| installer_summary = [k for k in all_f if k.endswith("/installer-summary.txt") and "prow-job-analyze-install-failure" in k] | |
| bundle_summary = [k for k in all_f if k.endswith("/log-bundle-summary.txt") and "prow-job-analyze-install-failure" in k] | |
| missing = [] | |
| if not report: | |
| missing.append("analysis report (report.txt)") | |
| if not installer_summary and not bundle_summary: | |
| missing.append("installer-summary.txt or log-bundle-summary.txt") | |
| if missing: | |
| return (False, f"Missing: {', '.join(missing)}") |
🤖 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 `@eval-install-failure.yaml` around lines 100 - 116, The current
output_files_exist logic (variables report, installer_summary, bundle_summary,
analysis_files, transcript) treats any file under an /analysis/ path or certain
transcript text as equivalent to the required artifacts, weakening the check;
update the function so that only explicitly-named artifact files satisfy the
check: consider analysis_files valid only if they are the same required
filenames (e.g., end with "/report.txt", "/installer-summary.txt", or
"/log-bundle-summary.txt"), remove the branch that returns True based on
transcript content, and only return True when at least one of report,
installer_summary, or bundle_summary (or matching analysis_files by exact
filename) is present; otherwise return False with the missing list as before.
Comment on lines
+236
to
+241
| if not stage_found: | ||
| # Check for partial matches | ||
| stage_words = expected_stage.split() | ||
| partial = any(w in all_text_lower for w in stage_words if len(w) > 4) | ||
| if partial: | ||
| return (True, f"Failure stage '{expected_stage}' partially identified (related terms found)") |
Contributor
There was a problem hiding this comment.
The fallback stage match is too permissive.
For multi-word stages like cluster operator stability, this branch passes if the analysis contains any long token such as cluster or operator. That can mark the wrong stage as correct and overstate failure_classification.
Suggested fix
- stage_words = expected_stage.split()
- partial = any(w in all_text_lower for w in stage_words if len(w) > 4)
+ stage_words = [w for w in expected_stage.split() if len(w) > 4]
+ partial = bool(stage_words) and all(w in all_text_lower for w in stage_words)
if partial:
return (True, f"Failure stage '{expected_stage}' partially identified (related terms found)")🤖 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 `@eval-install-failure.yaml` around lines 236 - 241, The fallback partial-match
is too permissive: when expected_stage contains multiple words the current logic
(stage_words/partial using any w in all_text_lower) flags a match if any single
long token appears. Change it so multi-word expected_stage requires a stricter
condition — e.g., split expected_stage into stage_words and for len(stage_words)
> 1 require at least N (recommend 2 or ceil(len(stage_words)/2)) distinct
long-word matches in all_text_lower (using whole-word checks or regex word
boundaries) before returning the partial match; keep the original behavior for
single-word stages. Update the variables referenced (stage_found,
expected_stage, stage_words, partial, all_text_lower) accordingly.
Comment on lines
+36
to
+38
| dataset: | ||
| path: eval/cases | ||
| schema: | |
Contributor
There was a problem hiding this comment.
Filter install-failure fixtures out of this dataset.
eval/cases now contains both payload and case-install-* fixtures. Without a case_pattern, this eval will also pick up install-failure cases, which only provide prow_url, so arguments: "{payload_tag}" cannot be resolved.
Suggested fix
dataset:
path: eval/cases
+ case_pattern: "case-[0-9]*"
schema: |🤖 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 `@eval.yaml` around lines 36 - 38, The dataset at eval/cases is picking up
install-failure fixtures that lack payloads, breaking resolution of arguments
"{payload_tag}"; add a case_pattern under dataset to exclude those (e.g. a regex
that excludes names starting with "case-install-" or only matches
payload-bearing fixtures) so the eval only loads payload cases; update the
dataset block (dataset.path: eval/cases) to include case_pattern: with the
appropriate negative or positive regex to filter out case-install-* fixtures.
Comment on lines
+161
to
+171
| if isinstance(data, dict) and "rows" in data: | ||
| data = data["rows"] | ||
| if not isinstance(data, list): | ||
| return (False, "JSON root is not an array (or dict with 'rows' key)") | ||
| if len(data) == 0: | ||
| return (False, "JSON array is empty") | ||
| required = ["payload_tag", "job_name", "failure_type", "root_cause_summary"] | ||
| row = data[0] | ||
| missing = [f for f in required if f not in row] | ||
| if missing: | ||
| return (False, f"Missing fields: {', '.join(missing)}") |
Contributor
There was a problem hiding this comment.
Allow empty autodl output for no-candidate cases.
This judge hard-fails an empty array, but this stack already includes case-014-5.0-ci-infra-only-no-candidates. For cases with failed jobs and no revert candidates, a zero-row (job, candidate) export is a valid outcome.
Suggested fix
- if len(data) == 0:
- return (False, "JSON array is empty")
+ if len(data) == 0:
+ if annotations.get("has_revert_candidates") is False:
+ return (True, "Valid JSON: 0 rows for a no-candidate case")
+ return (False, "JSON array is empty")
required = ["payload_tag", "job_name", "failure_type", "root_cause_summary"]
row = data[0]
+ if not isinstance(row, dict):
+ return (False, "JSON rows must be objects")
missing = [f for f in required if f not in row]🤖 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 `@eval.yaml` around lines 161 - 171, The current check treats an empty JSON
array as a hard failure; change that so an empty list is accepted as a valid "no
candidates" outcome: replace the block that returns (False, "JSON array is
empty") when len(data) == 0 with a non-failure return (e.g., (True, "No rows /
no candidates")) or otherwise allow processing to continue for zero-length data.
Update the logic around variables data, required, row, and missing so you only
attempt to validate row fields when len(data) > 0 (i.e., check 'row = data[0]'
and the missing-fields check only when data is non-empty).
| @@ -0,0 +1,24 @@ | |||
| expected_phase: "" | |||
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check eval.yaml schema for expected_phase validation rules
rg -A5 -B5 'expected_phase' eval.yaml eval-install-failure.yamlRepository: openshift-eng/ai-helpers
Length of output: 911
Fix expected_phase to use a valid schema value.
Line 1 uses an empty string for expected_phase, but the schema defined in eval.yaml only allows: Rejected, Ready, or Accepted. The empty string violates this constraint and may cause validation errors or unexpected behavior.
Update line 1 to use one of the three valid values. If the payload was neither rejected nor formally accepted, use Ready or Accepted depending on the actual phase.
🤖 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 `@eval/cases/case-013-5.0-nightly-nto-testdata-revert/annotations.yaml` at line
1, The expected_phase field currently contains an empty string which violates
the eval.yaml schema (allowed values: Rejected, Ready, Accepted); update the
expected_phase value in annotations.yaml (field name: expected_phase) to one of
those three literals—use Rejected if the payload was rejected, Ready if it is
staged/awaiting acceptance, or Accepted if it was formally accepted—so the file
validates against the schema.
|
|
||
| while IFS= read -r -d '' file; do | ||
| local size | ||
| size=$(stat --printf='%s' "$file" 2>/dev/null || echo 0) |
Contributor
There was a problem hiding this comment.
GNU-specific stat syntax breaks macOS compatibility.
Line 115 (and line 132) use stat --printf='%s', which is GNU-specific and will fail on macOS/BSD systems. The compress-archives.sh script (line 77) handles this correctly with a fallback.
🔧 Proposed fix for portability
remove_matching_files() {
local base_dir="$1"
local pattern="$2"
[[ -d "$base_dir" ]] || return 0
while IFS= read -r -d '' file; do
local size
- size=$(stat --printf='%s' "$file" 2>/dev/null || echo 0)
+ size=$(stat --format='%s' "$file" 2>/dev/null || stat -f '%z' "$file" 2>/dev/null || echo 0)
if $DRY_RUN; thenApply the same fix to line 132:
remove_large_files() {
local base_dir="$1"
local max_bytes="$2"
while IFS= read -r -d '' file; do
local size
- size=$(stat --printf='%s' "$file" 2>/dev/null || echo 0)
+ size=$(stat --format='%s' "$file" 2>/dev/null || stat -f '%z' "$file" 2>/dev/null || echo 0)
if (( size > max_bytes )); then📝 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.
Suggested change
| size=$(stat --printf='%s' "$file" 2>/dev/null || echo 0) | |
| size=$(stat --format='%s' "$file" 2>/dev/null || stat -f '%z' "$file" 2>/dev/null || echo 0) |
🤖 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 `@eval/scripts/trim-archives.sh` at line 115, The script uses GNU-only "stat
--printf='%s' \"$file\"" to set the variable size (seen in the size=
assignment), which breaks on macOS/BSD; replace both occurrences (the two size=
assignments) with a portable fallback: try GNU stat (--printf='%s'), then BSD
stat (-f '%z'), and as a last resort use wc -c, capturing the output safely and
defaulting to 0 on error. Implement this as a small helper snippet (or inline
conditional) that sets size reliably for $file and mirrors the approach used in
compress-archives.sh's fallback.
Comment on lines
+80
to
+82
| --compressed|--retry|--retry-delay) | ||
| shift | ||
| ;; |
Contributor
There was a problem hiding this comment.
Incorrect shift count for options that take arguments.
Line 80 groups --compressed (which takes no argument) with --retry and --retry-delay (which both require arguments) and applies a single shift. This will cause the argument values for --retry and --retry-delay to be misinterpreted as the URL.
🐛 Proposed fix
-X|--request)
shift 2
;;
- --compressed|--retry|--retry-delay)
+ --compressed)
shift
;;
+ --retry|--retry-delay)
+ shift 2
+ ;;
-*)🤖 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 `@eval/shims/curl` around lines 80 - 82, The case branch incorrectly groups
--compressed (no arg) with --retry and --retry-delay (which require arguments)
and performs a single shift; update the option handling so that the --compressed
branch consumes only the option (single shift) while the --retry and
--retry-delay branches each consume the option plus its argument (shift twice or
shift 2), referencing the existing case pattern handling for --compressed,
--retry and --retry-delay so their arguments are not treated as the URL.
|
|
||
| Please keep iterating on the below task list and mark them complete when done. | ||
| Ensure all evals generated include skill activation judges, efficacy, etc. Use | ||
| your best judgement to optimize and analyze our peformance. |
Contributor
There was a problem hiding this comment.
Fix typographical errors.
Several typos reduce the document's professionalism:
- Line 5: "peformance" → "performance"
- Line 65: "recocmend" → "recommend"
- Line 78: "one off" → "one-off" (when used as an adjective before a noun)
- Line 79: "insatll" → "install"
✏️ Proposed fixes
-your best judgement to optimize and analyze our peformance.
+your best judgement to optimize and analyze our performance.
-we could build evals on, etc, and also recocmend ideas I may not have
+we could build evals on, etc, and also recommend ideas I may not have
-I would also like to add an eval specifically for just one off job failures, we
-can start with insatll: /ci:analyze-prow-job-install-failure on
+I would also like to add an eval specifically for just one-off job failures, we
+can start with install: /ci:analyze-prow-job-install-failure onAlso applies to: 65-65, 78-79
🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: Ensure spelling is correct
Context: ...t judgement to optimize and analyze our peformance. Do not ask me any questions; just keep i...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_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 `@eval/TODO.md` at line 5, Fix the typographical errors in the TODO content by
replacing the misspelled tokens: change "peformance" to "performance",
"recocmend" to "recommend", "one off" (when used as an adjective) to "one-off",
and "insatll" to "install" wherever they appear (notably the occurrences
matching those tokens in the document).
| with tempfile.TemporaryDirectory() as tmpdir: | ||
| print(f"Extracting {tarball}...") | ||
| with tarfile.open(tarball) as tf: | ||
| tf.extractall(tmpdir) |
Contributor
There was a problem hiding this comment.
Potential path traversal vulnerability with tarfile.extractall().
Extracting tarballs without validating member paths can allow malicious archives to write files outside the target directory (e.g., paths like ../../etc/passwd). While session tarballs are expected to come from trusted GCS sources, defense-in-depth suggests validating extraction paths.
🛡️ Proposed fix
For Python 3.12+, use the filter parameter:
print(f"Extracting {tarball}...")
with tarfile.open(tarball) as tf:
- tf.extractall(tmpdir)
+ tf.extractall(tmpdir, filter='data')For older Python versions, validate paths manually:
print(f"Extracting {tarball}...")
with tarfile.open(tarball) as tf:
+ for member in tf.getmembers():
+ member_path = os.path.normpath(os.path.join(tmpdir, member.name))
+ if not member_path.startswith(tmpdir + os.sep):
+ raise ValueError(f"Unsafe path in tarball: {member.name}")
tf.extractall(tmpdir)📝 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.
Suggested change
| tf.extractall(tmpdir) | |
| tf.extractall(tmpdir, filter='data') |
🧰 Tools
🪛 Ruff (0.15.12)
[error] 375-375: Uses of tarfile.extractall()
(S202)
🤖 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 `@plugins/ci/skills/archive-payload-result/extract_session_data.py` at line
375, The tar extraction call tf.extractall(tmpdir) is vulnerable to path
traversal; update the extraction logic that creates/uses the TarFile object (tf)
and tmpdir to validate member paths before writing: either pass a safe filter
function to TarFile.extractall (Python 3.12+), or implement a manual check that
constructs the absolute destination for each TarInfo member and ensures it is
inside tmpdir (reject members with absolute paths or path components like ..),
then only extract validated members; reference the TarFile variable tf and the
target directory tmpdir when making these changes.
Contributor
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
fix()method to thePluginsDocUpToDateRulecustom skillsaw rule soskillsaw fixcan automatically regenerate PLUGINS.md and docs/data.json when they drift from plugin metadata_run_generators()helper to avoid duplicating subprocess logic betweencheck()andfix()AutofixConfidence.SAFEsince the fix is deterministic (runs the same generation scripts asmake update)Test plan
make lintpassessupports_autofix: Trueskillsaw fixregenerates docs when they're out of sync🤖 Generated with Claude Code