USHIFT-7335: Trim prow-job-analyzer prose and make it less prescriptive#239
USHIFT-7335: Trim prow-job-analyzer prose and make it less prescriptive#239pmtk wants to merge 3 commits into
prow-job-analyzer prose and make it less prescriptive#239Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe Prow job analyzer prompt now has stricter JSON output, investigation, schema, and root-cause classification guidance. The MicroShift CI primer adds build-log evidence details and revises sosreport and component timeline documentation. Plugin metadata versions are updated to 1.4.4. ChangesMicroShift CI guidance
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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/microshift-ci/agents/prow-job-analyzer.mdmarkdownlint-cli2 v0.23.0 (markdownlint v0.41.0) ... [truncated 1163 characters] ... node:internal/modules/esm/resolve:271:11) plugins/microshift-ci/agents/references/microshift-ci-primer.mdmarkdownlint-cli2 v0.23.0 (markdownlint v0.41.0) ... [truncated 1163 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: 3
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/agents/prow-job-analyzer.md (1)
25-147: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRun the repository markdownlint flow before committing.
Per CONTRIBUTING.md and the Markdown coding guidelines, lint the Markdown files with the repo’s configured
markdownlint-cli2invocation (scripts/lint-markdown.shor the configured.markdownlint-cli2.jsonccommand), not onlynpx markdownlint-cli2 '**/*.md'in this environment.🤖 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/agents/prow-job-analyzer.md` around lines 25 - 147, Run the repository-configured Markdown lint flow for the updated agents documentation before committing, using scripts/lint-markdown.sh or the command specified by .markdownlint-cli2.jsonc rather than a standalone npx markdownlint-cli2 invocation.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/agents/prow-job-analyzer.md`:
- Line 57: Update the validator in validate-rca-output.py to enforce the
documented output contract: reject entries containing keys outside the specified
fields, and require both analysis_gaps and scenarios to be arrays rather than
null. Keep validation of all existing required fields unchanged.
- Line 37: Update the sosreport workflow guidance near the journal-first
instructions so extraction is not limited to journal-detected crashes or
restarts. Require extracting the relevant sosreport whenever pod/container
evidence is needed, including product-versus-test diagnosis or inspecting
previous.log after multiple container creations, while retaining journal files
as the initial source and preferring the on-failure report matched by timestamp.
In `@plugins/microshift-ci/agents/references/microshift-ci-primer.md`:
- Around line 148-149: Correct the artifact-path documentation around the
artifact table in the microshift-ci primer: clarify that
<ARTIFACTS_DIR>/build-log.txt is located at the artifact root, while
<STEP>/build-log.txt remains under the scenario-info/<scenario>/ step paths.
Update the surrounding scope statement so it does not incorrectly apply the
scenario directory prefix to the root-level entry.
---
Outside diff comments:
In `@plugins/microshift-ci/agents/prow-job-analyzer.md`:
- Around line 25-147: Run the repository-configured Markdown lint flow for the
updated agents documentation before committing, using scripts/lint-markdown.sh
or the command specified by .markdownlint-cli2.jsonc rather than a standalone
npx markdownlint-cli2 invocation.
🪄 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: 63f13b8f-5518-4a27-b927-3ba7661e4d24
📒 Files selected for processing (2)
plugins/microshift-ci/agents/prow-job-analyzer.mdplugins/microshift-ci/agents/references/microshift-ci-primer.md
| Two `Created container` events for the same pod means the first instance died. Read `previous.log` for the exit reason before concluding a single-startup narrative. | ||
|
|
||
| ## Glossary | ||
| Journal files (`journal_*.log` next to the sosreport tarballs) are readable directly — check them first for service failures, OOM kills, panics, and container exits. Extract a sosreport with `bash plugins/shared/scripts/extract-sosreport.sh <tarball>` only when the journal shows crashes or restarts — pod and container logs (especially `previous.log`) exist exclusively inside the tarball. Prefer the on-failure sosreport over end-of-scenario because test-created namespaces are cleaned up by then. Match sosreport to failure by timestamp. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not restrict sosreport extraction to journal-detected crashes.
Line 33 requires pod logs to distinguish product failures from test timing issues, and line 35 specifically requires previous.log after detecting multiple container creations. Those signals may appear in build or pod artifacts without an explicit journal crash/restart entry, so this restriction can stop the investigation before the required evidence is read.
-Extract a sosreport ... only when the journal shows crashes or restarts
+Extract a sosreport when the investigation requires pod/container logs,
+including crashes, restarts, readiness flaps, or repeated container creation📝 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.
| Journal files (`journal_*.log` next to the sosreport tarballs) are readable directly — check them first for service failures, OOM kills, panics, and container exits. Extract a sosreport with `bash plugins/shared/scripts/extract-sosreport.sh <tarball>` only when the journal shows crashes or restarts — pod and container logs (especially `previous.log`) exist exclusively inside the tarball. Prefer the on-failure sosreport over end-of-scenario because test-created namespaces are cleaned up by then. Match sosreport to failure by timestamp. | |
| Journal files (`journal_*.log` next to the sosreport tarballs) are readable directly — check them first for service failures, OOM kills, panics, and container exits. Extract a sosreport when the investigation requires pod/container logs, including crashes, restarts, readiness flaps, or repeated container creation — pod and container logs (especially `previous.log`) exist exclusively inside the tarball. Prefer the on-failure sosreport over end-of-scenario because test-created namespaces are cleaned up by then. Match sosreport to failure by timestamp. |
🤖 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/agents/prow-job-analyzer.md` at line 37, Update the
sosreport workflow guidance near the journal-first instructions so extraction is
not limited to journal-detected crashes or restarts. Require extracting the
relevant sosreport whenever pod/container evidence is needed, including
product-versus-test diagnosis or inspecting previous.log after multiple
container creations, while retaining journal files as the initial source and
preferring the on-failure report matched by timestamp.
Restructure the agent definition from ~275 to ~148 lines (46% reduction). Research shows instruction compliance decays exponentially with count and ~65% of typical prompt instructions are redundant — specifying them consumes instruction-following capacity and reduces output predictability. Key changes: - Replace 4-step prescriptive workflow with 7 investigation principles - Remove Glossary, Important Files, Important Links, Tips sections - Eliminate content duplicated with the primer reference file (SOS Report section, journal patterns, container restart detection) - Consolidate sosreport extraction from 3 mentions to 1 - Reduce RAW_ERROR examples from 5 to 3 - Cut ROOT_CAUSE "How it differs" prose (contrastive table teaches this) - Replace aggressive emphasis (MUST/NEVER/CRITICAL) with calm directives All 16 JSON output fields retained — schema, example, severity rubric, and downstream-critical rules (RAW_ERROR determinism, ROOT_CAUSE consistency, CONFIDENCE calibration) are preserved. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Absorb Important Files entries (build-log.txt, step diagram URL)
from the analyzer into the primer's evidence table — this is the
reference file, so file-location info belongs here
- Remove SKIP_SOS paragraph (development-only detail, irrelevant to
CI artifact analysis)
- Rephrase behavioral instruction in journal section to factual
reference ("supports fault attribution" vs "reconstruct before
attributing") — behavioral guidance belongs in the agent definition,
not the reference file
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
a075e82 to
e719d23
Compare
1 participant
Summary by CodeRabbit