USHIFT-7425: Update LVMS CI Doctor with recent updates to MicroShift CI Doctor - #244
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pmtk The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
WalkthroughChangesProw RCA workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
pmtk
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugins/lvms-ci/skills/prow-job/SKILL.md (1)
74-84: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake the URL-input workflow use the initialized variables.
Step 0 sets
JOB_URL, but Step 1 buildsGCS_PATHfrom undefinedPROW_URL; URL-based analysis therefore downloads from an invalid source. Also create<WORKDIR>beforemktemp -d <WORKDIR>/..., otherwise a first-time invocation fails. Use${JOB_URL}consistently and addmkdir -p <WORKDIR>.🤖 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/lvms-ci/skills/prow-job/SKILL.md` around lines 74 - 84, Update the URL-input workflow in SKILL.md to derive GCS_PATH from the initialized JOB_URL variable, replacing the undefined PROW_URL reference. Before invoking mktemp for the temporary analysis directory, create WORKDIR with mkdir -p.plugins/lvms-ci/skills/doctor/SKILL.md (1)
76-96: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDefine an inline failure policy for each analyzer agent.
Specify how to handle an Agent failure or validation block before writing its JSON: retry policy, how to record the affected job, and whether finalization may continue without that result. Step 3 is mandatory, so leaving the result set unspecified makes aggregation behavior unreliable.
As per path instructions, “Flag missing edge cases or safety guards as severity ‘high’.” Based on learnings, failure policies should be co-located with the relevant tool invocation.
🤖 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/lvms-ci/skills/doctor/SKILL.md` around lines 76 - 96, Add an inline failure policy alongside each analyzer Agent invocation in the Step 2 instructions: define validation handling, a bounded retry policy, and how to record jobs whose results remain unavailable after retries. State explicitly that Step 3 must continue with those jobs marked as failed or missing, rather than silently omitting them, and ensure the policy is co-located with the tool invocation guidance.Sources: Path instructions, Learnings
🤖 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 `@plugins/lvms-ci/.claude-plugin/plugin.json`:
- Line 4: Update the matching lvms-ci entry in marketplace.json to version
1.3.0, keeping it synchronized with the version declared in the plugin manifest
and ensuring the marketplace file is included in the change.
In `@plugins/lvms-ci/agents/prow-job-analyzer.md`:
- Around line 131-140: Update the RAW_ERROR rules to define an exact
deterministic truncation: after removing the specified leading timestamp prefix,
retain exactly the first 150 characters when the error exceeds that length.
Replace the subjective “~150 characters” and “keep the distinctive part” wording
while preserving the requirement to copy one primary error verbatim.
In `@plugins/shared/scripts/validate-rca-output.py`:
- Around line 152-214: Add tests for the validation flow centered on
validate_json_text and validate_message: include a valid RCA array, malformed
JSON, invalid schema values, and invalid evidence citations. Assert that valid
input produces no errors and each invalid case returns the expected validation
errors, following the repository’s existing test conventions.
- Around line 57-72: Update the evidence validation flow around the validator
function containing the path checks to accept the trusted artifacts directory,
canonicalize both the artifact root and evidence path, and reject evidence
containing traversal or resolving outside that root before any file existence
check or read. Preserve valid citations to files within the supplied artifacts
directory and pass the trusted root through all callers.
- Around line 118-180: Extend validate_entry to enforce the complete RCA schema:
require causal-chain cause, evidence, and quote values to be strings; require
every analysis_gaps and scenarios item to be a string; reject entry keys outside
the documented schema; and enforce the documented maximum of 10 entries in
validate_json_text before processing them. Preserve the existing required-field
and evidence validation errors while reporting each new violation consistently.
---
Outside diff comments:
In `@plugins/lvms-ci/skills/doctor/SKILL.md`:
- Around line 76-96: Add an inline failure policy alongside each analyzer Agent
invocation in the Step 2 instructions: define validation handling, a bounded
retry policy, and how to record jobs whose results remain unavailable after
retries. State explicitly that Step 3 must continue with those jobs marked as
failed or missing, rather than silently omitting them, and ensure the policy is
co-located with the tool invocation guidance.
In `@plugins/lvms-ci/skills/prow-job/SKILL.md`:
- Around line 74-84: Update the URL-input workflow in SKILL.md to derive
GCS_PATH from the initialized JOB_URL variable, replacing the undefined PROW_URL
reference. Before invoking mktemp for the temporary analysis directory, create
WORKDIR with mkdir -p.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: cef326e6-95fb-4b55-9ef0-3acb2f7a3268
📒 Files selected for processing (10)
plugins/lvms-ci/.claude-plugin/plugin.jsonplugins/lvms-ci/.claude/settings.jsonplugins/lvms-ci/agents/prow-job-analyzer.mdplugins/lvms-ci/scripts/validate-rca-output.pyplugins/lvms-ci/skills/doctor/SKILL.mdplugins/lvms-ci/skills/prow-job/SKILL.mdplugins/microshift-ci/scripts/validate-rca-output.pyplugins/microshift-ci/scripts/validate-rca-output.pyplugins/microshift-ci/skills/prow-job/SKILL.mdplugins/shared/scripts/validate-rca-output.py
| ### RAW_ERROR rules | ||
|
|
||
| The `raw_error` field is used by downstream scripts for deterministic grouping. Two runs analyzing the same job MUST produce the same `raw_error`. Keep it simple — fewer rules mean less room for variation. | ||
|
|
||
| 1. **Copy-paste the exact error text** from the log — do NOT paraphrase, summarize, or reword | ||
| 2. **Pick only ONE error** — the primary error that caused the step to fail. If multiple errors exist, pick the first fatal one. | ||
| 3. **Only strip timestamps** — remove leading timestamps like `2026-04-01T06:21:48Z`. Keep everything else verbatim. | ||
| 4. **Never concatenate multiple errors** — pick ONE error, not a semicolon-separated list | ||
| 5. **Truncate to ~150 characters** if the raw message is very long — keep the distinctive part | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make raw_error truncation deterministic.
“~150 characters” and “keep the distinctive part” permit different truncation choices for the same log line, contradicting the requirement that repeated analyses produce identical values. Specify an exact transformation, such as stripping a defined timestamp prefix and retaining the first 150 characters.
🤖 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/lvms-ci/agents/prow-job-analyzer.md` around lines 131 - 140, Update
the RAW_ERROR rules to define an exact deterministic truncation: after removing
the specified leading timestamp prefix, retain exactly the first 150 characters
when the error exceeds that length. Replace the subjective “~150 characters” and
“keep the distinctive part” wording while preserving the requirement to copy one
primary error verbatim.
There was a problem hiding this comment.
Let's not get into this right now - deterministic grouping is out of scope for this PR
There was a problem hiding this comment.
@pmtk, acknowledged. I’ll defer this for the current PR. Would you like me to open a follow-up issue to track deterministic raw_error grouping?
| m = re.fullmatch(r"(.+):(\d+)", evidence) | ||
| if not m: | ||
| return [f"{prefix}: evidence must be absolute_path:line_number, got: {evidence}"] | ||
|
|
||
| path, line_no = m.group(1), int(m.group(2)) | ||
|
|
||
| if not os.path.isabs(path): | ||
| return [f"{prefix}: evidence path must be absolute, got: {path}"] | ||
|
|
||
| if not os.path.isfile(path): | ||
| return [f"{prefix}: evidence file not found: {path}"] | ||
|
|
||
| if any(path.endswith(ext) for ext in BINARY_EXTENSIONS): | ||
| return [] | ||
|
|
||
| lines = _read_lines(path, file_cache) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Restrict evidence reads to the supplied artifacts directory.
evidence comes from agent output, yet any absolute path is accepted and opened. A prompt-injected artifact can therefore make the hook read arbitrary host-readable files. Pass a trusted artifact root into the validator, resolve both paths, and reject citations outside that root before opening them.
As per path instructions, “Path traversal: canonicalize paths, reject ../”.
🤖 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/shared/scripts/validate-rca-output.py` around lines 57 - 72, Update
the evidence validation flow around the validator function containing the path
checks to accept the trusted artifacts directory, canonicalize both the artifact
root and evidence path, and reject evidence containing traversal or resolving
outside that root before any file existence check or read. Preserve valid
citations to files within the supplied artifacts directory and pass the trusted
root through all callers.
Source: Path instructions
There was a problem hiding this comment.
The validator runs as a SubagentStop hook on the local machine. The "attacker" would be the AI agent itself, which already has full Bash access to the same filesystem. Path-restricting evidence validation adds no security boundary — the agent can already read any file.
There was a problem hiding this comment.
@pmtk, agreed. Since the analyzer already has equivalent local filesystem access through Bash, constraining this hook’s read-only evidence verification would not provide a meaningful security boundary. I’m withdrawing this finding.
✏️ Learnings added
Learnt from: pmtk
Repo: openshift-eng/edge-tooling PR: 244
File: plugins/shared/scripts/validate-rca-output.py:57-72
Timestamp: 2026-07-28T13:25:49.557Z
Learning: In openshift-eng/edge-tooling, `plugins/shared/scripts/validate-rca-output.py` runs as a local Claude `SubagentStop` hook for the prow-job-analyzer. That analyzer already has Bash access to the same local filesystem, so do not raise path-restriction findings solely because the validator reads absolute evidence paths from agent output; restricting those reads to an artifacts directory does not create a meaningful security boundary in this local execution model.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: kasturinarra
Repo: openshift-eng/edge-tooling PR: 186
File: plugins/shared/scripts/extract-index-image.sh:79-124
Timestamp: 2026-06-12T14:09:05.662Z
Learning: When reviewing scripts in plugins/shared/scripts (e.g., extract-index-image.sh and related helpers like create-report.py), don’t insist on path-traversal sanitization/validation for parameters such as `release` and `artifacts_dir` if they are only used by a local operator running these CLI tools and the values originate solely from `doctor.sh`’s own argument parsing (i.e., there’s no untrusted/external input path being fed into these parameters). In other words: skip “missing path validation” findings for this tooling when the trust boundary is the local shell operator, and only flag path-validation gaps if the scripts can receive untrusted data from outside this doctor.sh-controlled flow.
| chain = entry.get("causal_chain") | ||
| if not isinstance(chain, list): | ||
| if chain is not None: | ||
| errors.append(f"entry[{index}]: 'causal_chain' must be an array") | ||
| else: | ||
| errors.append(f"entry[{index}]: 'causal_chain' must be a non-empty array, got null") | ||
| elif not chain: | ||
| errors.append(f"entry[{index}]: 'causal_chain' must be a non-empty array") | ||
| else: | ||
| for ci, link in enumerate(chain): | ||
| if not isinstance(link, dict): | ||
| errors.append(f"entry[{index}].causal_chain[{ci}]: must be an object") | ||
| continue | ||
| if "cause" not in link: | ||
| errors.append(f"entry[{index}].causal_chain[{ci}]: missing required key 'cause'") | ||
| if "evidence" not in link: | ||
| errors.append(f"entry[{index}].causal_chain[{ci}]: missing required key 'evidence'") | ||
| if "quote" not in link: | ||
| errors.append(f"entry[{index}].causal_chain[{ci}]: missing required key 'quote'") | ||
| evidence = link.get("evidence", "") | ||
| quote = link.get("quote", "") | ||
| if isinstance(evidence, str) and evidence: | ||
| errors.extend(validate_evidence( | ||
| evidence, quote, | ||
| f"entry[{index}].causal_chain[{ci}]", file_cache)) | ||
|
|
||
| for field in ("analysis_gaps", "scenarios"): | ||
| val = entry.get(field) | ||
| if val is not None and not isinstance(val, list): | ||
| errors.append(f"entry[{index}]: '{field}' must be an array") | ||
|
|
||
| return errors | ||
|
|
||
|
|
||
| def validate_json_text(text): | ||
| try: | ||
| data = json.loads(text) | ||
| except json.JSONDecodeError as e: | ||
| if "--- STRUCTURED SUMMARY ---" in text: | ||
| return [ | ||
| "Output contains prose and STRUCTURED SUMMARY markers. " | ||
| "Your entire response must be a valid JSON array only — no prose, no markers." | ||
| ] | ||
| return [f"Output is not valid JSON: {e}. Your entire response must be a valid JSON array."] | ||
|
|
||
| if isinstance(data, dict): | ||
| return [ | ||
| "Output is a JSON object, not an array. " | ||
| "Wrap your output in [...] — single failures must still be a JSON array." | ||
| ] | ||
| elif not isinstance(data, list): | ||
| return [f"Expected a JSON array, got {type(data).__name__}"] | ||
|
|
||
| if not data: | ||
| return ["JSON array is empty. Expected at least one failure entry."] | ||
|
|
||
| file_cache = {} | ||
| all_errors = [] | ||
| for i, entry in enumerate(data): | ||
| if not isinstance(entry, dict): | ||
| all_errors.append(f"entry[{i}]: expected an object, got {type(entry).__name__}") | ||
| continue | ||
| all_errors.extend(validate_entry(entry, i, file_cache)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Enforce the complete RCA schema before accepting output.
The validator accepts malformed values such as non-string causal-chain fields, non-string scenarios items, extra fields despite the “exactly these fields” contract, and more than the documented 10 entries. This allows invalid agent output into downstream aggregation/reporting; for example, ", ".join(scenarios) fails for non-string items. Validate link field types, array element types, unexpected keys, and the entry limit.
🤖 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/shared/scripts/validate-rca-output.py` around lines 118 - 180, Extend
validate_entry to enforce the complete RCA schema: require causal-chain cause,
evidence, and quote values to be strings; require every analysis_gaps and
scenarios item to be a string; reject entry keys outside the documented schema;
and enforce the documented maximum of 10 entries in validate_json_text before
processing them. Preserve the existing required-field and evidence validation
errors while reporting each new violation consistently.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '=== Validator structure ==='
ast-grep outline plugins/shared/scripts/validate-rca-output.py --items all \
--type function --view expanded
printf '%s\n' '=== Relevant schema checks ==='
rg -n -C 4 \
'unexpected|extra|allowed|REQUIRED_FIELDS|causal_chain|analysis_gaps|scenarios|10|len\(data\)' \
plugins/shared/scripts/validate-rca-output.py
printf '%s\n' '=== Candidate focused tests ==='
fd -t f . | rg '(test|validate-rca|rca-output)' || trueLength of output: 6559
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '=== Complete validation implementation ==='
sed -n '90,214p' plugins/shared/scripts/validate-rca-output.py
printf '%s\n' '=== Analyzer schema and entry-limit contract ==='
rg -n -C 5 \
'exactly|fields|10|maximum|JSON array|causal_chain|analysis_gaps|scenarios' \
plugins/lvms-ci/agents/prow-job-analyzer.md \
plugins/microshift-ci/agents/prow-job-analyzer.md 2>/dev/null || true
printf '%s\n' '=== Working-tree state for the validator ==='
git diff --check -- plugins/shared/scripts/validate-rca-output.py
git status --short -- plugins/shared/scripts/validate-rca-output.pyLength of output: 29598
@pmtk, rechecked. The causal-chain field-type validation and string-item checks for analysis_gaps/scenarios are now addressed.
The finding still applies to two documented constraints:
- Unexpected fields:
validate_entry()checks only missing keys; it does not reject keys outsideREQUIRED_FIELDS, while both analyzer schemas say each entry has exactly those fields. - Maximum entries:
validate_json_text()processes arbitrary-length arrays; both analyzer schemas require at most 10 entries.
So I’m leaving the comment unresolved, narrowed to those two remaining validations.
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@plugins/lvms-ci/skills/prow-job/SKILL.md`:
- Around line 113-118: Update the Causal Chain template to iterate over every
item in causal_chain and render each link’s cause, evidence, and quote with
dynamically generated numbering. Remove the hard-coded causal_chain[0] and
causal_chain[1] references so non-empty chains, including a single-link chain,
render correctly.
- Around line 74-76: Create the computed WORKDIR parent directory before the
mktemp invocation in the URL-processing branch, and check that directory
creation succeeds before continuing. Keep the existing TMP assignment and
artifact-download flow unchanged after successful creation.
- Around line 74-76: Strengthen the local-directory branch in the input handling
step by explicitly validating that <ARGUMENTS> is a directory containing both
build-log.txt and finished.json before setting TMP or skipping download. If any
check fails, stop in that workflow step with an actionable error identifying the
missing or invalid artifact; leave URL handling unchanged.
- Around line 74-76: Update the URL handling in the prow-job workflow to use the
same variable throughout: the `JOB_URL` assigned in the input-type setup must be
the variable consumed when constructing `GCS_PATH` and invoking the downloader.
Remove the stale `PROW_URL` reference while preserving the existing
local-directory and URL input behavior.
- Around line 105-121: Update the “Display results” workflow to validate the
parsed agent output with plugins/shared/scripts/validate-rca-output.py before
formatting or saving any entries. Stop immediately when validation fails, and
proceed to the existing formatting and persistence steps only after the shared
RCA schema validation succeeds.
- Around line 123-125: Update the raw JSON save-path guidance in the prow-job
skill to strictly allow-list the derived RELEASE and JOB_ID identifiers,
rejecting empty or path-traversal values such as slashes and “..”. Before
writing, resolve the destination and verify it remains within <WORKDIR>/jobs;
only then save the file.
In `@plugins/shared/scripts/validate-rca-output.py`:
- Around line 124-137: Update the causal-chain validation loop around
validate_evidence so quote may be an empty string when the evidence is an image
supported by validate_evidence’s binary-extension exemption. Keep quote required
as a non-empty string for non-image evidence, while preserving the existing
evidence and type validation.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: f44577cb-73a6-4cd7-8f31-783d07eac841
📒 Files selected for processing (12)
.claude-plugin/marketplace.jsonplugins/lvms-ci/.claude-plugin/plugin.jsonplugins/lvms-ci/.claude/settings.jsonplugins/lvms-ci/agents/prow-job-analyzer.mdplugins/lvms-ci/scripts/validate-rca-output.pyplugins/lvms-ci/skills/doctor/SKILL.mdplugins/lvms-ci/skills/prow-job/SKILL.mdplugins/microshift-ci/.claude-plugin/plugin.jsonplugins/microshift-ci/scripts/validate-rca-output.pyplugins/microshift-ci/scripts/validate-rca-output.pyplugins/microshift-ci/skills/prow-job/SKILL.mdplugins/shared/scripts/validate-rca-output.py
🚧 Files skipped from review as they are similar to previous changes (5)
- plugins/lvms-ci/.claude/settings.json
- plugins/lvms-ci/skills/doctor/SKILL.md
- plugins/microshift-ci/skills/prow-job/SKILL.md
- plugins/lvms-ci/.claude-plugin/plugin.json
- plugins/microshift-ci/scripts/validate-rca-output.py
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
|
||
| 1. There are many setup and teardown stages so fatal errors may be buried by log output from the teardown phase. It is not common to find the fatal error at the end of the log. | ||
| 2. You can quickly determine the failed step from the build-log.txt by reading the last `Running step ...` line before the container logs appear. | ||
| 3. Check the CatalogSource and operator setup steps (`lvms-catalogsource`, `operatorhub-subscribe-lvm-operator`, `storage-create-lvm-cluster`) early — if any failed, the operator was never fully deployed and all downstream test failures are secondary. |
There was a problem hiding this comment.
Investigation Principles and Tips have overlapping content
CatalogSource check — appears in both Investigation Principles ("Check the operator setup chain early...") and here ("Check the CatalogSource and operator setup steps..."). Nearly
identical wording.
|
/lgtm |
2 participants
Summary by CodeRabbit
New Features
Improvements