USHIFT-7328: CI Doctor: prow-job-analyzer agent + hook ensuring correct output JSON - #226
Conversation
|
[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
WalkthroughThe PR replaces text-based CI Doctor RCA reports with strict JSON arrays, adds a MicroShift analyzer and output validator, updates workflow persistence, and migrates parsing, aggregation, bug search, continuation, documentation, and plugin versions to the JSON report format. ChangesJSON RCA reporting pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 markdownlint-cli2 (0.23.0)plugins/lvms-ci/skills/doctor/SKILL.mdmarkdownlint-cli2 v0.23.0 (markdownlint v0.41.0) ... [truncated 1446 characters] ... node:internal/modules/esm/resolve:271:11) plugins/lvms-ci/skills/prow-job/SKILL.mdmarkdownlint-cli2 v0.23.0 (markdownlint v0.41.0) ... [truncated 1446 characters] ... node:internal/modules/esm/resolve:271:11) plugins/microshift-ci/agents/prow-job-analyzer.mdmarkdownlint-cli2 v0.23.0 (markdownlint v0.41.0) ... [truncated 1446 characters] ... node:internal/modules/esm/resolve:271:11)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
plugins/microshift-ci/scripts/validate-rca-output.py (1)
56-63: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFull
readlines()on every evidence citation is a scalability risk against the 30s hook timeout.CI
build-log.txtfiles can be tens of MB. Reading the entire file into memory percausal_chainlink (up to 5 entries × multiple links each) adds up, and the hook is registered with"timeout": 30insettings.json— a timeout here fails the hook silently (non-blocking error per Claude Code's hook contract), defeating the validation this PR exists to add. Consider seeking/streaming to the target line instead of loading the whole file.♻️ Proposed fix using itertools.islice
+ import itertools try: - with open(path, errors="replace") as f: - lines = f.readlines() + with open(path, errors="replace") as f: + target_line = next(itertools.islice(f, line_no - 1, line_no), None) + total_lines = sum(1 for _ in f) + line_no # only if bound check needed beyond target except OSError: return [f"{prefix}: evidence file could not be read: {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 `@plugins/microshift-ci/scripts/validate-rca-output.py` around lines 56 - 63, Update the evidence-file validation flow around the current open/readlines logic to stream only through the file until the cited line, rather than loading all lines into memory for each citation. Preserve the existing error messages and line-number validation, including reporting when line_no is beyond the available lines, while ensuring repeated causal_chain validations remain efficient within the hook timeout.
🤖 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/microshift-ci/.claude/settings.json`:
- Around line 5-12: Update the command in the prow-job-analyzer hook
configuration to prefix the validate-rca-output.py path with
$CLAUDE_PROJECT_DIR, ensuring it resolves from any working directory. Leave the
existing matcher and timeout/status settings unchanged.
In `@plugins/microshift-ci/agents/prow-job-analyzer.md`:
- Line 25: Resolve the inconsistent failure-count contract in the analyzer
instructions around the JSON response requirements and the failure-reporting
section near lines 269–273. Choose one maximum failure count, then update the
documented output limit, validator behavior, and downstream consumers to enforce
that same limit consistently without truncating or rejecting otherwise valid
reports.
In `@plugins/microshift-ci/scripts/validate-rca-output.py`:
- Around line 14-19: Update NON_EMPTY_STRING_FIELDS and the validation logic in
validate_rca_output so step_name, root_cause, and remediation are validated as
non-empty strings, not merely checked for key presence. Preserve the existing
validation behavior for the already-covered fields and ensure empty or
non-string values are rejected.
- Around line 65-71: The validation flow in the cited evidence handling must not
silently skip invalid entries: require a non-empty causal_chain before
validation, and update the quote handling in the relevant validation function to
reject empty or shorter-than-10-character quotes as errors rather than returning
[]. Ensure every cited file, line, and quote proceeds through the documented
validation and line-match checks.
- Around line 35-63: Update validate_evidence to canonicalize the parsed
evidence path with os.path.realpath and validate it against the configured
workdir/artifact-root allow-list before any isfile or open call. Reject paths
outside those roots, including traversal and symlink escapes, while preserving
existing format and line-validation behavior for allowed files.
In `@plugins/microshift-ci/skills/prow-job/SKILL.md`:
- Around line 105-125: Define and apply an explicit report-schema validation
gate before formatting or persisting agent output. In
plugins/microshift-ci/skills/prow-job/SKILL.md lines 105-125, validate that the
parsed value is the expected JSON array and that every entry satisfies the
documented fields and types before display or raw-JSON saving; on failure,
report the validation error and do not persist. Apply the same array contract in
plugins/lvms-ci/skills/doctor/SKILL.md lines 78-83 before Write, and gate the
persistence flow in plugins/microshift-ci/skills/doctor/SKILL.md lines 121-123
so invalid reports are skipped.
---
Nitpick comments:
In `@plugins/microshift-ci/scripts/validate-rca-output.py`:
- Around line 56-63: Update the evidence-file validation flow around the current
open/readlines logic to stream only through the file until the cited line,
rather than loading all lines into memory for each citation. Preserve the
existing error messages and line-number validation, including reporting when
line_no is beyond the available lines, while ensuring repeated causal_chain
validations remain efficient within the hook timeout.
🪄 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: Enterprise
Run ID: 9e25e831-aebd-4c1a-881b-c46f086c81b8
📒 Files selected for processing (15)
plugins/lvms-ci/skills/doctor/SKILL.mdplugins/lvms-ci/skills/prow-job/SKILL.mdplugins/microshift-ci/.claude/settings.jsonplugins/microshift-ci/agents/prow-job-analyzer.mdplugins/microshift-ci/agents/references/microshift-ci-primer.mdplugins/microshift-ci/scripts/continue-session.shplugins/microshift-ci/scripts/search-bugs.pyplugins/microshift-ci/scripts/validate-rca-output.pyplugins/microshift-ci/skills/continue-session/SKILL.mdplugins/microshift-ci/skills/create-bugs/SKILL.mdplugins/microshift-ci/skills/doctor/SKILL.mdplugins/microshift-ci/skills/prow-job/SKILL.mdplugins/shared/scripts/aggregate.pyplugins/shared/scripts/doctor.shplugins/shared/scripts/parse.py
3907b29 to
4d62e01
Compare
coderabbitai
Bot
added
the
ready-for-human-review
8f18242 to
749c830
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/microshift-ci/skills/doctor/SKILL.md (1)
88-128: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd error handling for agent orchestration in Step 2.
Per
plugins/docs/SKILL-GUIDELINES.md: "if the skill orchestrates agents, include edge cases and guard checks (e.g., 'block/handle invalid JSON outputs')." Step 2 orchestrates multiple agents but lacks an "Error Handling" block. Based on learnings, you must co-locate failure policies and edge-case rules inline with the specific step that needs them. Please add an "Error Handling" section explicitly detailing how to handle agent failures, timeouts, or invalid JSON outputs for this step.🤖 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/microshift-ci/skills/doctor/SKILL.md` around lines 88 - 128, Add an “Error Handling” subsection within Step 2, after the agent orchestration instructions, defining how to handle analyzer agent failures, timeouts, and invalid or missing JSON responses. Require recording a structured failure result for the affected job, continuing with other agents, and ensuring Step 3 receives only valid saved results.Sources: Path instructions, Learnings
♻️ Duplicate comments (2)
plugins/microshift-ci/scripts/validate-rca-output.py (2)
68-74: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject short quotes instead of silently returning.
As flagged in a previous review, the validation flow silently skips invalid entries by returning an empty error list for short quotes. This breaks the documented requirement that each cited file, line, and quote be rigorously checked. Update the quote handling to reject empty or shorter-than-10-character quotes as validation errors rather than returning
[].🔧 Proposed fix
- if not isinstance(quote, str) or len(quote) < 10: - return [] + if not isinstance(quote, str) or len(quote) < 10: + return [f"{prefix}: quote must be a string of at least 10 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/microshift-ci/scripts/validate-rca-output.py` around lines 68 - 74, Update the quote validation flow around normalized_quote and cited_line so empty or shorter-than-10-character quotes return a validation error containing the existing prefix context, instead of returning an empty list. Preserve the existing line-content comparison and quote-not-found error behavior for valid-length quotes.
50-63: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftConstrain
evidenceto allowed artifact roots before opening it.Per CONTRIBUTING.md: "Path traversal: canonicalize paths, reject ../" and "Validate at trust boundaries with allow-lists, not deny-lists." As flagged in a previous review, the validation using
os.path.isabs()still allows arbitrary paths like/etc/passwdand symlink escapes. Canonicalize the path withos.path.realpath()and reject anything outside the expected workdir or artifact roots before callingisfile()oropen().🤖 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/microshift-ci/scripts/validate-rca-output.py` around lines 50 - 63, Update the evidence-path validation flow around the visible isabs/isfile checks to canonicalize paths with os.path.realpath() and enforce an allow-list of the expected workdir and artifact roots. Reject traversal, absolute paths outside those roots, and symlink escapes before invoking os.path.isfile() or open(), while preserving the existing binary and read-error handling for accepted paths.Sources: Coding guidelines, Path instructions
🤖 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/microshift-ci/scripts/validate-rca-output.py`:
- Around line 107-128: Update the causal_chain validation in the
entry-processing flow to reject an empty list, requiring it to be a non-empty
array while preserving the existing type and link-level checks for populated
lists.
---
Outside diff comments:
In `@plugins/microshift-ci/skills/doctor/SKILL.md`:
- Around line 88-128: Add an “Error Handling” subsection within Step 2, after
the agent orchestration instructions, defining how to handle analyzer agent
failures, timeouts, and invalid or missing JSON responses. Require recording a
structured failure result for the affected job, continuing with other agents,
and ensuring Step 3 receives only valid saved results.
---
Duplicate comments:
In `@plugins/microshift-ci/scripts/validate-rca-output.py`:
- Around line 68-74: Update the quote validation flow around normalized_quote
and cited_line so empty or shorter-than-10-character quotes return a validation
error containing the existing prefix context, instead of returning an empty
list. Preserve the existing line-content comparison and quote-not-found error
behavior for valid-length quotes.
- Around line 50-63: Update the evidence-path validation flow around the visible
isabs/isfile checks to canonicalize paths with os.path.realpath() and enforce an
allow-list of the expected workdir and artifact roots. Reject traversal,
absolute paths outside those roots, and symlink escapes before invoking
os.path.isfile() or open(), while preserving the existing binary and read-error
handling for accepted paths.
🪄 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: Enterprise
Run ID: ac664b80-60b4-4178-804b-6f08311121db
📒 Files selected for processing (13)
plugins/lvms-ci/skills/doctor/SKILL.mdplugins/lvms-ci/skills/prow-job/SKILL.mdplugins/microshift-ci/.claude/settings.jsonplugins/microshift-ci/agents/prow-job-analyzer.mdplugins/microshift-ci/scripts/continue-session.shplugins/microshift-ci/scripts/search-bugs.pyplugins/microshift-ci/scripts/validate-rca-output.pyplugins/microshift-ci/skills/continue-session/SKILL.mdplugins/microshift-ci/skills/create-bugs/SKILL.mdplugins/microshift-ci/skills/doctor/SKILL.mdplugins/shared/scripts/aggregate.pyplugins/shared/scripts/doctor.shplugins/shared/scripts/parse.py
🚧 Files skipped from review as they are similar to previous changes (8)
- plugins/microshift-ci/.claude/settings.json
- plugins/microshift-ci/skills/continue-session/SKILL.md
- plugins/lvms-ci/skills/doctor/SKILL.md
- plugins/microshift-ci/agents/prow-job-analyzer.md
- plugins/microshift-ci/scripts/continue-session.sh
- plugins/microshift-ci/skills/create-bugs/SKILL.md
- plugins/microshift-ci/scripts/search-bugs.py
- plugins/shared/scripts/aggregate.py
749c830 to
2f85054
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/microshift-ci/skills/doctor/SKILL.md (1)
125-128: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd error handling for subagent failures in Step 2.
Step 2 launches multiple
microshift-ci:prow-job-analyzeragents in parallel but lacks anError Handlingblock specifying what to do if an agent fails, times out, or returns a tool error.As per path instructions for
plugins/*/skills/**/SKILL.md, you must flag missing edge cases or safety guards. Additionally, based on learnings, failure policies and edge-case rules must be explicitly co-located inline with the specific step that needs them.Please append an explicit error-handling directive at the end of Step 2 to ensure one failing job does not block the entire analysis.
🔧 Proposed fix
3. Launch **ALL** agents (all releases + PRs) in a **single message** as **foreground** agents (do NOT use `run_in_background`). Foreground agents in the same message run concurrently — this is just as fast as background agents but keeps your turn active until all complete. 4. Say "Analyzing N jobs in parallel..." in your message text alongside the Agent tool calls. 5. When all agents return, immediately proceed to Step 3 in the same turn. Do NOT stop or end your turn between Step 2 and Step 3. + +**Error Handling**: + +- If a subagent fails, times out, or returns an error, note the failure for that specific job and proceed with the rest. Do not let one job's failure block the workflow.🤖 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/microshift-ci/skills/doctor/SKILL.md` around lines 125 - 128, Append an explicit error-handling directive to Step 2 after the parallel `microshift-ci:prow-job-analyzer` launch instructions. Specify that failed, timed-out, or tool-error agents must be recorded as failed and must not block waiting for or processing the remaining agents; continue to Step 3 once all successful results and failure statuses are collected.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/microshift-ci/agents/prow-job-analyzer.md`:
- Around line 149-152: The failure-localization tip in the Tips section is tied
to one specific Prow job. Replace the hardcoded step name in the build-log
search guidance with a generic pattern, and present the existing e2e step name
only as an example while preserving the instruction to use the last matching
step before container logs.
- Line 123: Update the workflow classification instruction in the
prow-job-analyzer documentation to use the canonical labels defined by the JSON
enum near the output schema, including the required test-failure stage values.
Ensure the guidance and schema consistently use the same enum values, or
explicitly document how the prose labels map to those canonical values.
- Around line 156-186: The output schema contract must include the RCA fields
consumed by downstream rendering: failure_type, impact, suspect_prs,
recommendation, same_root_cause, and attempt_analyses. Update the documented
output contract near the “Each entry in the output array” definition with these
exact field names and expected shapes, or synchronously update the consumers to
use the revised contract while preserving all RCA and per-attempt details.
In `@plugins/microshift-ci/scripts/validate-rca-output.py`:
- Around line 115-131: Fix the indentation of the causal-chain validation block
under the surrounding else clause, including the for loop and all of its
contents, so it uses the standard four-space nesting required by PEP 8 and
passes Ruff validation. Preserve the existing validation logic in
validate_evidence and the associated error reporting.
In `@plugins/microshift-ci/skills/create-bugs/SKILL.md`:
- Line 67: Update Step 1 in SKILL.md beside the search-bugs.py invocation to
instruct the agent to relay warnings for job files skipped due to invalid JSON
and stop when the script reports “No valid job reports found.” Ensure incomplete
candidate data is not processed silently.
- Line 577: Update Example 6 in the create-bugs skill documentation to use the
exact failure message emitted by search-bugs.py: “No job files found for <source
label> in <WORKDIR>”. Replace the current “Error: No job analysis files found at
<WORKDIR>/jobs/release-4.19-job-*.json” example without changing surrounding
guidance.
---
Outside diff comments:
In `@plugins/microshift-ci/skills/doctor/SKILL.md`:
- Around line 125-128: Append an explicit error-handling directive to Step 2
after the parallel `microshift-ci:prow-job-analyzer` launch instructions.
Specify that failed, timed-out, or tool-error agents must be recorded as failed
and must not block waiting for or processing the remaining agents; continue to
Step 3 once all successful results and failure statuses are collected.
🪄 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: Enterprise
Run ID: 4eb3d522-5c4e-4248-87e3-dde268cc4d45
📒 Files selected for processing (18)
.claude-plugin/marketplace.jsonplugins/lvms-ci/.claude-plugin/plugin.jsonplugins/lvms-ci/skills/doctor/SKILL.mdplugins/lvms-ci/skills/prow-job/SKILL.mdplugins/microshift-ci/.claude-plugin/plugin.jsonplugins/microshift-ci/.claude/settings.jsonplugins/microshift-ci/agents/prow-job-analyzer.mdplugins/microshift-ci/agents/references/microshift-ci-primer.mdplugins/microshift-ci/scripts/continue-session.shplugins/microshift-ci/scripts/search-bugs.pyplugins/microshift-ci/scripts/validate-rca-output.pyplugins/microshift-ci/skills/continue-session/SKILL.mdplugins/microshift-ci/skills/create-bugs/SKILL.mdplugins/microshift-ci/skills/doctor/SKILL.mdplugins/microshift-ci/skills/prow-job/SKILL.mdplugins/shared/scripts/aggregate.pyplugins/shared/scripts/doctor.shplugins/shared/scripts/parse.py
🚧 Files skipped from review as they are similar to previous changes (9)
- plugins/shared/scripts/doctor.sh
- plugins/microshift-ci/.claude/settings.json
- plugins/microshift-ci/skills/continue-session/SKILL.md
- plugins/lvms-ci/skills/doctor/SKILL.md
- plugins/shared/scripts/aggregate.py
- plugins/microshift-ci/scripts/continue-session.sh
- plugins/microshift-ci/agents/references/microshift-ci-primer.md
- plugins/microshift-ci/skills/prow-job/SKILL.md
- plugins/microshift-ci/scripts/search-bugs.py
2f85054 to
5596844
Compare
ggiguash
left a comment
ggiguash
left a comment
There was a problem hiding this comment.
Code review: 5 findings (3 confirmed, 2 plausible). Details in inline comments.
| m = re.search(r'\[.*\]', content, re.DOTALL) | ||
| if m: | ||
| try: | ||
| entries = json.loads(m.group(0)) |
There was a problem hiding this comment.
[correctness — CONFIRMED] Greedy fallback regex silently drops valid data
The fallback regex r'\[.*\]' with re.DOTALL matches from the first [ to the last ] in the entire file content. When a file contains square brackets in prose before the JSON array (e.g., error messages like index out of range [6], markdown links, or log prefixes like [INFO]), the regex captures a superset of the intended JSON. The inner json.loads then fails, and the function silently returns [], dropping all failure data.
Failure scenario: An old-format .txt file contains panic: runtime error: index out of range [6] with length 6 in the prose before the JSON block. The regex matches from [6] through the JSON array's closing ]. json.loads fails on the garbage, function returns [], and the job is silently dropped from the aggregated report.
The old marker-based regex (--- STRUCTURED SUMMARY ---) was reliable for mixed-content files; this replacement is not. Consider either keeping the old marker regex as an additional fallback, or using a non-greedy approach that anchors to a [ at the start of a line: r"^\[.*\]$" with re.MULTILINE | re.DOTALL.
| When a job has multiple independent test failures across different scenarios, produce **one entry per failure** in the JSON array. Each entry must be self-contained with all fields populated. | ||
| ```text | ||
| Analyze this prow job: | ||
| artifacts_dir: /tmp/microshift-ci-claude-workdir.260710/artifacts/2075422415638237184 |
There was a problem hiding this comment.
[correctness — CONFIRMED] Missing job_url and job_name in agent prompt
The example prompt here passes only artifacts_dir, graphs_dir, and source_dir — but the prow-job-analyzer agent definition marks job_url and job_name as (required) input, and the SubagentStop validator enforces both as non-empty strings.
The skill already has the URL from its <ARGUMENTS> but doesn't forward it. The doctor SKILL.md correctly passes both fields in its version of the same prompt — this is an inconsistency.
Failure scenario: User runs /microshift-ci:prow-job <URL>. Skill downloads artifacts, spawns agent without job_url. Agent must reconstruct from build-log.txt, which may fail for presubmit PR jobs or truncated logs. Validator blocks output if reconstruction fails.
Suggest adding job_url and job_name to the prompt template (and deriving job_name from the URL in step 0).
| - `release`: the release branch — extract from job_name (e.g. 4.22 from release-4.22), or from finished.json metadata repos field, or default to "main" | ||
| - `remediation`: suggested fix or next step — what should be done to address this failure (~120 chars max). For infrastructure failures, state the infra action (e.g. "retry the job", "rotate AWS credentials"). For product bugs, state the code-level fix direction | ||
| - `finished`: the job finish date in YYYY-MM-DD format, extracted from finished.json timestamp field or build log timestamps | ||
| - `causal_chain`: array of links from observed symptom toward root cause. Each link: `{"cause": ..., "evidence": ..., "quote": ...}` where `evidence` is the **absolute** file path with a mandatory line number (`/absolute/path:lineNum`; use `:1` for binary files) and `quote` is a short verbatim excerpt from the cited line (empty for binary files). A SubagentStop hook validates that each cited file exists, the line number is in range, and the quote appears on the cited line |
There was a problem hiding this comment.
[correctness — CONFIRMED] False claim: SubagentStop hook does not exist for lvms-ci
This text states "A SubagentStop hook validates that each cited file exists, the line number is in range, and the quote appears on the cited line" — but no SubagentStop hook or .claude/settings.json was created for the plugins/lvms-ci/ directory. Only plugins/microshift-ci/.claude/settings.json got the hook.
Failure scenario: LVMS prow-job skill produces malformed causal_chain entries (wrong evidence paths, missing quotes). The agent trusts the claimed validation and may produce sloppy citations. No hook catches the errors, and downstream scripts render broken evidence links in reports and Jira bugs.
Either add a matching hook for lvms-ci, or remove this claim from the field description.
There was a problem hiding this comment.
Hooks cannot be attached to skills, that why I had to convert it to agent first.
Do you want me to work on the "feature parity"? So far I was treating LVMS with "don't want to break it"
There was a problem hiding this comment.
I misread the problem. We'll go back to the pretty-please agents for LVMS.
Problem with reusing the script is the delivery: if we want the plugins to be installable, the script needs to belong to the plugin (cannot be in the shared/)
| errors.append(f"entry[{index}]: 'infrastructure_failure' must be a boolean, got {type(infra).__name__}") | ||
|
|
||
| layer = entry.get("stack_layer") | ||
| if not isinstance(layer, str) or layer not in VALID_STACK_LAYERS: |
There was a problem hiding this comment.
[efficiency — PLAUSIBLE] readlines() loads entire evidence files per citation with no caching
CI build-log.txt files routinely reach 30–100 MB. When multiple causal_chain entries cite the same file, readlines() is called once per citation — reading the same large file repeatedly. Combined with the 30-second hook timeout, this risks timeout on I/O-heavy CI hosts.
Failure scenario: Agent produces 4 causal_chain links citing different lines in the same 50 MB build-log.txt. Hook reads ~200 MB total (4 full reads). On a busy host, sequential I/O exceeds the 30s timeout; hook is killed.
Consider caching file contents by path (a simple dict) or using linecache to read only up to the cited line.
| 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: |
There was a problem hiding this comment.
[correctness — PLAUSIBLE] Quote validation checks only the single cited line
The check reads lines[line_no - 1] and tests whether the normalized quote is a substring of that single line. Log lines frequently wrap across multiple lines. When an agent quotes text spanning a line break, the validator rejects it — blocking valid analysis.
Failure scenario: Log has An error occurred (InvalidClientTokenId) when calling\nthe CreateStack operation split across lines 100–101. Agent cites line 100 with quote spanning both lines. Validator checks only line 100, quote not found, blocks the agent despite correct analysis.
Consider checking a small window (e.g., lines line_no-1 through line_no+1 joined) instead of the single line.
There was a problem hiding this comment.
It's by design. Agent instruction says:
quoteis a short verbatim excerpt from the cited line — copied exactly
so it's either contained in the line or it's not. Giving a 3 line window would weaken the validator
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/shared/scripts/parse.py`:
- Around line 41-56: Update the fallback parsing logic in the JSONDecodeError
branch to iterate backwards over every line-ending closing bracket in each
candidate tail, trying the latest possible closing bracket first. Preserve the
existing candidate-array scan and continue to the next opening bracket only
after all closing-bracket endpoints for the current tail fail, so valid JSON
arrays containing internal lines ending in ] are retained.
🪄 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: Enterprise
Run ID: fd7f6ff3-b83a-43fb-9b70-06df1a2130d2
📒 Files selected for processing (18)
.claude-plugin/marketplace.jsonplugins/lvms-ci/.claude-plugin/plugin.jsonplugins/lvms-ci/skills/doctor/SKILL.mdplugins/lvms-ci/skills/prow-job/SKILL.mdplugins/microshift-ci/.claude-plugin/plugin.jsonplugins/microshift-ci/.claude/settings.jsonplugins/microshift-ci/agents/prow-job-analyzer.mdplugins/microshift-ci/agents/references/microshift-ci-primer.mdplugins/microshift-ci/scripts/continue-session.shplugins/microshift-ci/scripts/search-bugs.pyplugins/microshift-ci/scripts/validate-rca-output.pyplugins/microshift-ci/skills/continue-session/SKILL.mdplugins/microshift-ci/skills/create-bugs/SKILL.mdplugins/microshift-ci/skills/doctor/SKILL.mdplugins/microshift-ci/skills/prow-job/SKILL.mdplugins/shared/scripts/aggregate.pyplugins/shared/scripts/doctor.shplugins/shared/scripts/parse.py
🚧 Files skipped from review as they are similar to previous changes (9)
- plugins/microshift-ci/.claude-plugin/plugin.json
- .claude-plugin/marketplace.json
- plugins/shared/scripts/doctor.sh
- plugins/microshift-ci/scripts/continue-session.sh
- plugins/microshift-ci/scripts/search-bugs.py
- plugins/microshift-ci/skills/create-bugs/SKILL.md
- plugins/microshift-ci/skills/prow-job/SKILL.md
- plugins/shared/scripts/aggregate.py
- plugins/microshift-ci/skills/continue-session/SKILL.md
| try: | ||
| entries = json.loads(m.group(1)) | ||
| entries = json.loads(content) | ||
| except json.JSONDecodeError: | ||
| return [] | ||
| entries = None | ||
| for m in reversed(list(re.finditer(r'^\[', content, re.MULTILINE))): | ||
| tail = content[m.start():] | ||
| m2 = re.search(r'\][ \t]*$', tail, re.MULTILINE) | ||
| if not m2: | ||
| continue | ||
| try: | ||
| entries = json.loads(tail[:m2.end()].rstrip()) | ||
| break | ||
| except json.JSONDecodeError: | ||
| continue | ||
| if entries is None: | ||
| return [] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Fix the greedy fallback parser to prevent it from silently dropping valid JSON.
The fallback parser uses re.search to find the first ] at the end of a line within tail. If the generated JSON contains any internal lines ending with a ] (such as "analysis_gaps": []), the regex matches that early bracket, json.loads fails, and the loop moves on to the next ^\[ without ever trying the actual end of the JSON array. This silently discards valid data.
Iterate backwards over all possible closing brackets to ensure the parser evaluates the true end of the JSON array before moving on.
🐛 Proposed fix for the fallback parser
try:
entries = json.loads(content)
except json.JSONDecodeError:
entries = None
for m in reversed(list(re.finditer(r'^\[', content, re.MULTILINE))):
tail = content[m.start():]
- m2 = re.search(r'\][ \t]*$', tail, re.MULTILINE)
- if not m2:
- continue
- try:
- entries = json.loads(tail[:m2.end()].rstrip())
- break
- except json.JSONDecodeError:
- continue
+ for m2 in reversed(list(re.finditer(r'\][ \t]*$', tail, re.MULTILINE))):
+ try:
+ entries = json.loads(tail[:m2.end()].rstrip())
+ break
+ except json.JSONDecodeError:
+ continue
+ if entries is not None:
+ break
if entries is None:
return []🤖 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/parse.py` around lines 41 - 56, Update the fallback
parsing logic in the JSONDecodeError branch to iterate backwards over every
line-ending closing bracket in each candidate tail, trying the latest possible
closing bracket first. Preserve the existing candidate-array scan and continue
to the next opening bracket only after all closing-bracket endpoints for the
current tail fail, so valid JSON arrays containing internal lines ending in ]
are retained.
|
/lgtm |
2 participants
report-microshift-ci-doctor.html
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
.jsonjob artifacts (with updated counting and discovery rules).Bug Fixes
Chores