Integrate known failure pattern classification into RCA skill - #13
Integrate known failure pattern classification into RCA skill#13PalmPalm7 wants to merge 5 commits into
Conversation
|
Warning Review limit reached
Next review available in: 53 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds YAML-backed regex error classification with URL or local-file loading and URL-scoped caching. Integrates classification into the analysis CLI through ChangesKnown Failure Classification
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Analyze as analyze
participant Resolver as resolve_known_failures
participant Cache
participant Classifier as classify_job_errors
participant Artifact as classification.json
Analyze->>Resolver: Resolve CLI or environment source
Resolver->>Cache: Read or write URL-scoped YAML cache
Resolver-->>Analyze: Return parsed failure patterns
Analyze->>Classifier: Classify job and timeline errors
Classifier-->>Analyze: Return deduplicated matches
Analyze->>Artifact: Write classification results
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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 (1)
skills/root-cause-analysis/scripts/cli.py (1)
10-22:⚠️ Potential issue | 🟠 MajorDirect-execution support is incomplete.
The conditional imports on lines 10-27 correctly support both direct and module execution, but
cmd_query()uses a relative import (from .splunk_client import SplunkClienton line 316) that will fail when the script is run directly. Whenpython scripts/cli.py query ...is executed,__package__will beNone, causing the relative import to raise an error. Thequerycommand will not work in direct-execution mode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/root-cause-analysis/scripts/cli.py` around lines 10 - 22, The cmd_query function currently does a relative import "from .splunk_client import SplunkClient" which fails when the script is executed directly; update the import resolution to match the top-of-file pattern by using a conditional/try-import fallback so SplunkClient is imported correctly in both direct and module execution modes (e.g., attempt relative import first, and if that fails import using the top-level path or vice versa), and apply this change where SplunkClient is referenced (cmd_query) so the query command works for both "python scripts/cli.py" and "python -m scripts.cli".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pyproject.toml`:
- Around line 23-26: The pyproject.toml dependencies list is missing the runtime
dependency for the requests library used by fetch_known_failures_from_url() in
classify.py; add "requests>=2.0.0" (or an appropriate minimum version) to the
dependencies array so installations include requests and the URL-backed
known-failures code path (e.g., --known-failures-url / KNOWN_FAILED_YAML_URL)
does not raise ModuleNotFoundError.
In `@skills/root-cause-analysis/scripts/classify.py`:
- Around line 19-49: fetch_known_failures_from_url currently writes every
fetched YAML to a single global _CACHE_FILE so a later failed fetch for a
different URL can incorrectly return another URL's patterns; change the caching
to be URL-specific by deriving a per-source cache filename (e.g., hash the url
or sanitize it) inside _CACHE_DIR, write/read that per-URL cache instead of
_CACHE_FILE, and when falling back in the except block call load_known_failures
with the per-URL cache path (ensure _CACHE_DIR.mkdir remains and keep
headers/github token logic intact).
- Around line 64-69: The YAML loader in _parse_yaml_content should validate the
parsed shape before using it: ensure the top-level result is a dict and that the
"failures" key is a list of dicts; if not, return an empty list. Update
_parse_yaml_content to treat non-dict top-level values (e.g., [], str, int) as
invalid, treat missing or non-list "failures" as empty, and filter the failures
list to include only dict entries so downstream classify_error() never receives
unexpected types.
In `@skills/root-cause-analysis/scripts/cli.py`:
- Around line 209-237: The code currently skips writing classification.json when
no known failures are configured; change the logic so classification_path is
always written with a stable payload. Specifically, when
resolve_known_failures(...) returns falsy, set classifications = [] and
patterns_loaded = 0 (or compute patterns_loaded = len(known_failures) in the
truthy case), then always build classification_result = {"patterns_loaded":
patterns_loaded, "matches": classifications} and json.dump it to
classification_path; keep the existing prints for matched vs no matches but
ensure the file is created even in the else branch — update calls around
resolve_known_failures, classify_job_errors, classification_path, and
classification_result accordingly.
In `@skills/root-cause-analysis/SKILL.md`:
- Line 100: The fallback category list in the "Root Cause" instruction of
SKILL.md includes invalid enums ("workload_bug" and "credential") that don't
match schemas/summary.schema.json and will produce invalid
step5_analysis_summary.json; update the fallback list in the "Root Cause" line
to only include enum values that exist in schemas/summary.schema.json (remove or
replace "workload_bug" and "credential" with the schema-approved equivalents),
and ensure any example outputs or references to step5_analysis_summary.json are
updated accordingly and validate against schemas/summary.schema.json.
---
Outside diff comments:
In `@skills/root-cause-analysis/scripts/cli.py`:
- Around line 10-22: The cmd_query function currently does a relative import
"from .splunk_client import SplunkClient" which fails when the script is
executed directly; update the import resolution to match the top-of-file pattern
by using a conditional/try-import fallback so SplunkClient is imported correctly
in both direct and module execution modes (e.g., attempt relative import first,
and if that fails import using the top-level path or vice versa), and apply this
change where SplunkClient is referenced (cmd_query) so the query command works
for both "python scripts/cli.py" and "python -m scripts.cli".
🪄 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: Pro
Run ID: cbad026d-413d-4ac5-a4d5-8d718dd66cb3
📒 Files selected for processing (7)
pyproject.tomlskills/root-cause-analysis/README.mdskills/root-cause-analysis/SKILL.mdskills/root-cause-analysis/schemas/summary.schema.jsonskills/root-cause-analysis/scripts/classify.pyskills/root-cause-analysis/scripts/cli.pyskills/root-cause-analysis/tests/test_classify.py
|
Re: CodeRabbit outside-diff comment on |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/root-cause-analysis/SKILL.md`:
- Line 80: The doc is contradictory about classification.json being "REQUIRED"
yet "if present"; update the text in SKILL.md to state classification.json is an
unconditional/deterministic input (since scripts/cli.py always writes it),
remove "if present", and clearly instruct: always consult classification.json
for known failure pattern matches and use its error_category when a match
exists, otherwise mark the failure as novel/unclassified; reference
classification.json and scripts/cli.py in the text so readers know the source
and determinism.
- Line 100: The preferred category list in SKILL.md omits `general_failure`,
causing mismatch with scripts/classify.py which may emit `general_failure` for
matched patterns; update the preferred categories line (the list containing
`platform_failure|connectivity_failure|authentication_failure|resource_failure|timeout_failure|automation_failure|infrastructure_failure`)
to include `general_failure` among the preferred classifications so that
`general_failure` is treated as a primary match and not force fallback
handling—ensure the doc text still mentions the fallback set
(`configuration|infrastructure|application_bug|secrets|resource|dependency`)
remains unchanged and keep summary and confidence requirements intact.
🪄 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: Pro
Run ID: 6e94fc05-c7c8-44d4-b0c9-9815a9472120
📒 Files selected for processing (3)
skills/root-cause-analysis/SKILL.mdskills/root-cause-analysis/scripts/classify.pyskills/root-cause-analysis/scripts/cli.py
🚧 Files skipped from review as they are similar to previous changes (2)
- skills/root-cause-analysis/scripts/cli.py
- skills/root-cause-analysis/scripts/classify.py
| 2. **REQUIRED**: `step3_correlation.json` - Correlated timeline with relevant pod logs (DO NOT read step2 unless needed) | ||
| 3. **REQUIRED**: `step4_github_fetch_history.json` - Configuration and code context | ||
| 4. **CONDITIONAL**: `step2_splunk_logs.json` - Only read if step3 indicates errors needing deeper investigation | ||
| 4. **REQUIRED**: `classification.json` - Known failure pattern matches (if present). Use these verified categories instead of guessing. If a match exists, use its `error_category` as the root cause category. If no matches, flag as novel/unclassified failure. |
There was a problem hiding this comment.
Clarify classification.json as unconditionally required input.
Line 80 is internally contradictory (REQUIRED vs “if present”). scripts/cli.py always writes classification.json, so Step 5 guidance should be deterministic.
Proposed doc fix
-4. **REQUIRED**: `classification.json` - Known failure pattern matches (if present). Use these verified categories instead of guessing. If a match exists, use its `error_category` as the root cause category. If no matches, flag as novel/unclassified failure.
+4. **REQUIRED**: `classification.json` - Known failure pattern matching result. Always read this file. If a match exists, use its `error_category` as the root cause category. If no matches, treat as novel/unclassified failure.As per coding guidelines, “Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/root-cause-analysis/SKILL.md` at line 80, The doc is contradictory
about classification.json being "REQUIRED" yet "if present"; update the text in
SKILL.md to state classification.json is an unconditional/deterministic input
(since scripts/cli.py always writes it), remove "if present", and clearly
instruct: always consult classification.json for known failure pattern matches
and use its error_category when a match exists, otherwise mark the failure as
novel/unclassified; reference classification.json and scripts/cli.py in the text
so readers know the source and determinism.
| ### Summary Requirements | ||
|
|
||
| 1. **Root Cause**: Category (`configuration|infrastructure|workload_bug|credential|resource|dependency`), summary, confidence | ||
| 1. **Root Cause**: Category — prefer `classification.json` categories when matched (`platform_failure|connectivity_failure|authentication_failure|resource_failure|timeout_failure|automation_failure|infrastructure_failure`). Fall back to (`configuration|infrastructure|application_bug|secrets|resource|dependency`) only for novel/unclassified errors. Include summary and confidence. |
There was a problem hiding this comment.
Include general_failure in preferred classification categories.
Line 100’s preferred list omits general_failure, but scripts/classify.py can emit general_failure as the default category for a matched pattern entry. That mismatch can force unnecessary fallback handling and weaken category consistency.
Proposed doc fix
-1. **Root Cause**: Category — prefer `classification.json` categories when matched (`platform_failure|connectivity_failure|authentication_failure|resource_failure|timeout_failure|automation_failure|infrastructure_failure`). Fall back to (`configuration|infrastructure|application_bug|secrets|resource|dependency`) only for novel/unclassified errors. Include summary and confidence.
+1. **Root Cause**: Category — prefer `classification.json` categories when matched (`platform_failure|connectivity_failure|authentication_failure|resource_failure|timeout_failure|automation_failure|infrastructure_failure|general_failure`). Fall back to (`configuration|infrastructure|application_bug|secrets|resource|dependency`) only for novel/unclassified errors. Include summary and confidence.As per coding guidelines, “Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/root-cause-analysis/SKILL.md` at line 100, The preferred category list
in SKILL.md omits `general_failure`, causing mismatch with scripts/classify.py
which may emit `general_failure` for matched patterns; update the preferred
categories line (the list containing
`platform_failure|connectivity_failure|authentication_failure|resource_failure|timeout_failure|automation_failure|infrastructure_failure`)
to include `general_failure` among the preferred classifications so that
`general_failure` is treated as a primary match and not force fallback
handling—ensure the doc text still mentions the fallback set
(`configuration|infrastructure|application_bug|secrets|resource|dependency`)
remains unchanged and keep summary and confidence requirements intact.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@skills/root-cause-analysis/scripts/classify.py`:
- Line 180: Update the details assignment in the relevant event-processing
function to use event.get("details") or {}, ensuring both missing and None
values produce an empty dictionary before subsequent .get() calls.
- Around line 71-72: The cache is written before validating the fetched content,
allowing invalid responses to overwrite a valid cache. In the relevant loading
function, parse content with _parse_yaml_content first, store the result, then
call cache_file.write_text(content) only after parsing succeeds, and return the
parsed result.
🪄 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: 2ffeca03-4c48-40ce-9dfd-1668b3bcc255
📒 Files selected for processing (3)
skills/root-cause-analysis/README.mdskills/root-cause-analysis/scripts/classify.pyskills/root-cause-analysis/tests/test_classify.py
✅ Files skipped from review due to trivial changes (1)
- skills/root-cause-analysis/README.md
| cache_file.write_text(content) | ||
| return _parse_yaml_content(content) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm _parse_yaml_content lets yaml.YAMLError propagate (does not catch it internally).
sed -n '116,125p' skills/root-cause-analysis/scripts/classify.pyRepository: redhat-et/rhdp-rca-plugin
Length of output: 518
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant function and its cache fallback path.
sed -n '1,160p' skills/root-cause-analysis/scripts/classify.pyRepository: redhat-et/rhdp-rca-plugin
Length of output: 6106
Write the cache only after parsing succeeds. cache_file.write_text(content) runs before _parse_yaml_content(content), so a transient HTML/error response can overwrite a previously good cache. If the next fetch fails, load_known_failures() falls back to that poisoned file and returns no patterns.
🤖 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 `@skills/root-cause-analysis/scripts/classify.py` around lines 71 - 72, The
cache is written before validating the fetched content, allowing invalid
responses to overwrite a valid cache. In the relevant loading function, parse
content with _parse_yaml_content first, store the result, then call
cache_file.write_text(content) only after parsing succeeds, and return the
parsed result.
| # details.error_message (aap_job). Fall back to a top-level message key so text | ||
| # that only surfaces in pod/Splunk logs is still classified. | ||
| for event in correlation.get("timeline_events", []): | ||
| details = event.get("details", {}) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
event.get("details", {}) returns None when the key exists with a None value, causing AttributeError on the subsequent .get() calls.
Use or {} to handle both missing and None cases:
🛡️ Proposed fix
- details = event.get("details", {})
+ details = event.get("details") or {}📝 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.
| details = event.get("details", {}) | |
| details = event.get("details") or {} |
🤖 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 `@skills/root-cause-analysis/scripts/classify.py` at line 180, Update the
details assignment in the relevant event-processing function to use
event.get("details") or {}, ensuring both missing and None values produce an
empty dictionary before subsequent .get() calls.
Add optional error classification step that matches job errors against a curated YAML of regex-based failure patterns. This gives Claude verified ground-truth categories instead of guessing, using a standardized 8-category taxonomy (platform_failure, connectivity_failure, authentication_failure, resource_failure, timeout_failure, etc.). The known_failed.yaml is fetched at runtime from a configurable URL or local path — nothing is vendored. Configure via: - CLI: --known-failures-url <url> or --known-failures-file <path> - Env: KNOWN_FAILED_YAML_URL or KNOWN_FAILED_YAML in .claude/settings.local.json Classification is fully optional — the pipeline works without it. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The `requests` import in classify.py was top-level, causing ModuleNotFoundError in CI since it's not in pyproject.toml deps. Move it to a lazy import inside fetch_known_failures_from_url() where it's actually needed. Add PyYAML to pyproject.toml since classify.py directly uses it. Fix ruff I001 import sorting in test_classify.py. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…n, schema-aligned categories - Validate YAML shape in _parse_yaml_content: reject non-dict top-level, non-list failures, and non-dict entries - Always emit classification.json even when no patterns are configured, so Step 5 has a stable pipeline contract - Fix SKILL.md fallback categories to match summary.schema.json enum (workload_bug → application_bug, credential → secrets) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
b801bef to
7be5c49
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@skills/root-cause-analysis/README.md`:
- Line 49: Update the baseline configuration example around SPLUNK_VERIFY_SSL so
it does not default to disabled TLS verification: remove the setting or set it
to "true". Mention "false" only as a temporary, narrowly scoped exception.
In `@skills/root-cause-analysis/scripts/classify.py`:
- Line 227: Update the no-override branch in classify.py so it loads the bundled
known_failed.yaml taxonomy before returning no patterns, using the existing
classification flow in the surrounding function instead of immediately returning
an empty list. Keep the empty-list fallback only when the bundled source cannot
be resolved or loaded, and update any documentation that currently says
classification is skipped without configuration to reflect the bundled fallback
behavior.
- Around line 58-65: Update the URL classification around is_github_api to parse
url and require both HTTPS and an exact hostname of api.github.com, rather than
using substring matching. Use this stricter predicate for both the raw Accept
header and the Authorization header so GITHUB_TOKEN is never sent to
attacker-controlled hosts.
🪄 Autofix
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: Pro Plus
Run ID: f3a428c6-1f53-4f3c-9e7f-e23c1af5f8c0
📒 Files selected for processing (7)
pyproject.tomlskills/root-cause-analysis/README.mdskills/root-cause-analysis/SKILL.mdskills/root-cause-analysis/schemas/summary.schema.jsonskills/root-cause-analysis/scripts/classify.pyskills/root-cause-analysis/scripts/cli.pyskills/root-cause-analysis/tests/test_classify.py
🚧 Files skipped from review as they are similar to previous changes (2)
- pyproject.toml
- skills/root-cause-analysis/schemas/summary.schema.json
| "SPLUNK_OCP_APP_INDEX": "your_ocp_app_index", | ||
| "SPLUNK_OCP_INFRA_INDEX": "your_ocp_infra_index", | ||
| "SPLUNK_VERIFY_SSL": "false" | ||
| "SPLUNK_VERIFY_SSL": "false", |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not document disabled TLS verification as the default.
SPLUNK_VERIFY_SSL: "false" disables certificate validation. This can expose Splunk credentials and job data to a network attacker. Remove this setting from the baseline example, or set it to "true". Document false only as a temporary, narrowly scoped exception.
🤖 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 `@skills/root-cause-analysis/README.md` at line 49, Update the baseline
configuration example around SPLUNK_VERIFY_SSL so it does not default to
disabled TLS verification: remove the setting or set it to "true". Mention
"false" only as a temporary, narrowly scoped exception.
Source: Path instructions
| is_github_api = "api.github.com" in url | ||
| if is_github_api: | ||
| # Request raw content even without a token; unauthenticated | ||
| # api.github.com/contents requests otherwise return JSON metadata. | ||
| headers["Accept"] = "application/vnd.github.v3.raw" | ||
| github_token = os.environ.get("GITHUB_TOKEN", "") | ||
| if github_token and is_github_api: | ||
| headers["Authorization"] = f"token {github_token}" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Authenticate only the exact GitHub API origin.
The substring check accepts attacker-controlled hosts such as https://api.github.com.attacker.invalid/.... When GITHUB_TOKEN is set, Line 65 sends the token to that host. Parse the URL and require https with hostname exactly api.github.com before adding GitHub-specific headers.
Proposed fix
+from urllib.parse import urlsplit
+
- is_github_api = "api.github.com" in url
+ parsed_url = urlsplit(url)
+ is_github_api = (
+ parsed_url.scheme == "https"
+ and parsed_url.hostname == "api.github.com"
+ )📝 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.
| is_github_api = "api.github.com" in url | |
| if is_github_api: | |
| # Request raw content even without a token; unauthenticated | |
| # api.github.com/contents requests otherwise return JSON metadata. | |
| headers["Accept"] = "application/vnd.github.v3.raw" | |
| github_token = os.environ.get("GITHUB_TOKEN", "") | |
| if github_token and is_github_api: | |
| headers["Authorization"] = f"token {github_token}" | |
| from urllib.parse import urlsplit | |
| parsed_url = urlsplit(url) | |
| is_github_api = ( | |
| parsed_url.scheme == "https" | |
| and parsed_url.hostname == "api.github.com" | |
| ) | |
| if is_github_api: | |
| # Request raw content even without a token; unauthenticated | |
| # api.github.com/contents requests otherwise return JSON metadata. | |
| headers["Accept"] = "application/vnd.github.v3.raw" | |
| github_token = os.environ.get("GITHUB_TOKEN", "") | |
| if github_token and is_github_api: | |
| headers["Authorization"] = f"token {github_token}" |
🤖 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 `@skills/root-cause-analysis/scripts/classify.py` around lines 58 - 65, Update
the URL classification around is_github_api to parse url and require both HTTPS
and an exact hostname of api.github.com, rather than using substring matching.
Use this stricter predicate for both the raw Accept header and the Authorization
header so GITHUB_TOKEN is never sent to attacker-controlled hosts.
Source: Path instructions
| if env_path: | ||
| return load_known_failures(env_path) | ||
|
|
||
| return [] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Load the bundled failure taxonomy before returning no patterns.
The PR objective requires a bundled known_failed.yaml fallback. This branch returns [] when no override is configured, so normal analysis skips classification and cannot use verified categories. Resolve the bundled source here, and retain the empty result only when that fallback is unavailable. Update documentation that states classification is skipped without configuration.
🤖 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 `@skills/root-cause-analysis/scripts/classify.py` at line 227, Update the
no-override branch in classify.py so it loads the bundled known_failed.yaml
taxonomy before returning no patterns, using the existing classification flow in
the surrounding function instead of immediately returning an empty list. Keep
the empty-list fallback only when the bundled source cannot be resolved or
loaded, and update any documentation that currently says classification is
skipped without configuration to reflect the bundled fallback behavior.
…cache, timeline message fallback - classify.py fetch_known_failures_from_url: send the GitHub raw `Accept` header for api.github.com/contents URLs regardless of whether a token is set, so unauthenticated fetches return file content instead of JSON metadata; token is now optional (used only for private repos). Add _extract_yaml_text() to defensively decode base64 JSON-metadata responses. - classify.py: scope the download cache per source via _cache_path_for(url) (sha256-keyed filename); a failed fetch now only falls back to the same URL's cache, never patterns cached from a different URL. - classify.py classify_job_errors: read timeline text from details.message / details.error_message and fall back to a top-level `message` key so pod/ Splunk log failures are always classified. - README: document local-file and plain-curl/HTTP (no-token) options, the --known-failures-file / --known-failures-url flags, per-URL caching, and graceful skip when no source is configured; use placeholder URLs only. - tests: add regression coverage for top-level message fallback, JSON metadata decoding, raw-YAML passthrough, and per-URL cache scoping. Note: the summary.schema.json enum already includes the new failure categories (platform_failure, connectivity_failure, timeout_failure, etc.) and is consistent with SKILL.md and classify.py, so no schema change was needed for that finding. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
7be5c49 to
2a41ee7
Compare
1 participant
What
Adds an optional classification step between Step 4 and Step 5. Job errors are regex-matched against a curated
known_failed.yamland the result is written toclassification.json, so Step 5 reads verified categories — the same taxonomy as the RHDP ETL pipeline — instead of Claude guessing them.Fully optional: with no pattern source configured the step is skipped,
classification.jsonrecords"skipped": true, and nothing else in the pipeline changes.Pattern source
Any one of, in priority order — no GitHub token required:
--known-failures-url/KNOWN_FAILED_YAML_URL— any HTTP(S) URL; a plainraw.githubusercontent.comURL works unauthenticated--known-failures-file/KNOWN_FAILED_YAML— local pathGITHUB_TOKENis used only to reach privateapi.github.comURLs. Downloads are cached per-URL (keyed by hash), so a failed fetch never silently falls back to a different source's cache.Files
scripts/classify.py— new; load, cache, matchscripts/cli.py— wires the step in after Step 4, adds the two flagsSKILL.md,README.md— instruct Claude to prefer verified categoriesschemas/summary.schema.json— additional failure-specific categoriestests/test_classify.py— new; 15 testsVerification
pytest76 passed (61 existing + 15 new);ruff check+ruff format --checkclean; no newmypyerrors vsmainmain@ 25d62cf — conflict-freeReviewer note
This is complementary to
deploy/batch-rca-automation/scripts/fetch_known_issues.py, which surfaces learned history (prior high-confidence results from Postgres). This PR supplies a curated ground-truth taxonomy. If you'd rather have one front door for "verified categories", say so and I'll align them.