USHIFT-7419: Deterministic CI Doctor Orchestration - #257
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
007a6e4 to
cdfe127
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesThe CI Doctor workflow now uses a deterministic shared CI Doctor migration
Estimated code review effort: 4 (Complex) | ~60 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.2)plugins/lvms-ci/README.mdmarkdownlint-cli2 v0.23.2 (markdownlint v0.41.1) ... [truncated 1441 characters] ... node:internal/modules/esm/resolve:271:11) plugins/microshift-ci/README.mdmarkdownlint-cli2 v0.23.2 (markdownlint v0.41.1) ... [truncated 1441 characters] ... node:internal/modules/esm/resolve:271:11) plugins/microshift-ci/scripts/pcp-graphs/README.mdmarkdownlint-cli2 v0.23.2 (markdownlint v0.41.1) ... [truncated 1441 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: 7
🧹 Nitpick comments (3)
plugins/shared/scripts/doctor.py (3)
376-388: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider
ThreadPoolExecutorinstead ofProcessPoolExecutor.
_analyze_single_jobspawns aclaudesubprocess and blocks on it. The work is I/O-bound and releases the GIL.ProcessPoolExecutorwith the default--parallel 64forks 64 Python interpreters to supervise 64 subprocesses, and it must pickleagent_system_promptonce per job.ThreadPoolExecutorprovides the same concurrency at much lower memory cost and removes the picklability constraint on the worker arguments.The current code is correct. This is a resource-efficiency improvement.
🤖 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/doctor.py` around lines 376 - 388, Replace ProcessPoolExecutor with ThreadPoolExecutor in the job-analysis execution around _analyze_single_job, preserving the existing max_parallel setting, submitted arguments, and future-to-job mapping.
285-291: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the substring match on the error text with a structured flag.
Line 285 detects a timeout by searching for the literal
"Timed out"insidevalidation_errors. The string is produced at Line 821 asf"Timed out after {limits['timeout']}s". If that message is reworded, this check silently returnsFalseand the report showsNO STOP HOOK (validation did not run!)for a job that timed out.Return an explicit timeout flag from
_analyze_single_joband read it here.🤖 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/doctor.py` around lines 285 - 291, Update _analyze_single_job to return an explicit timeout flag alongside validation_errors, and replace the substring-based timed_out calculation in the report-building block with that structured value. Ensure every return path supplies the flag, including timeout and non-timeout cases, so the status continues to distinguish TIMED OUT from NO STOP HOOK without relying on error-message wording.
593-625: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBuild the cost summary lines once.
_print_cost_summaryand_write_cost_diagnosticscompute the same per-release job count, cost, average, and duration, and format them into nearly identical text. The two implementations can drift. Extract one function that returns the formatted lines, then log them and append them todiagnostics.txt.🤖 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/doctor.py` around lines 593 - 625, Extract the shared cost-summary construction from _print_cost_summary and _write_cost_diagnostics into one helper that returns the fully formatted lines, preserving the existing stage, release, total, and grand-total text. Update _print_cost_summary to log each returned line and _write_cost_diagnostics to append the same lines to diagnostics.txt, removing the duplicated calculations and formatting.
🤖 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/create-report.py`:
- Around line 2103-2108: Update the diagnostics loading block in main around
diagnostics_path so it reads diagnostics.txt with an explicit UTF-8 encoding and
catches read-related UnicodeDecodeError or OSError exceptions. On any read
failure, keep diagnostics_text absent and allow report generation to continue.
In `@plugins/shared/scripts/doctor.py`:
- Around line 685-717: Remove the duplicated parsing logic from
_extract_result_text_standalone and reuse
_extract_last_assistant_message_from_transcript from validate-rca-output.py.
Expose that helper through the module already loaded by _run_validation via
importlib, then call the shared helper for extraction while preserving the
existing return and error-handling behavior.
- Around line 720-748: Add a test module covering the new parsing and timeout
helpers, including _extract_job_stats cases for missing result records,
malformed JSON mixed with valid records, and synthetic user Stop hook feedback.
Also test _extract_result_text_standalone, prepare() summary scanning,
extract_cost, _run_claude_session timeout handling, and run_doctor_sh timeout
behavior with both successful and failure paths where applicable.
- Around line 342-349: Update _load_prepare_summary to validate that the parsed
JSON result is a dictionary before assigning it to self.prepare_summary and
returning True; for any other JSON type, emit the existing diagnostics message
and return False so _collect_jobs_to_analyze never receives a non-mapping
summary.
- Around line 139-144: Reset diagnostics.txt during pipeline initialization in
the class __init__ method, immediately after assigning self.diagnostics_file, by
truncating or recreating the file while preserving its path for the existing
append-mode writers.
- Around line 206-229: Update the process completion logic in run_doctor_sh so
it confirms proc.returncode indicates success before reporting a timer-triggered
timeout. Preserve timeout reporting for processes that were actually killed or
did not exit successfully, ensuring a successful doctor.sh run cannot return
False due to a late _kill invocation.
- Around line 115-116: Restrict the --component argument in the argument-parser
setup to the supported microshift and lvm-operator values, using argparse
validation so invalid or misspelled components fail immediately instead of
reaching ALL_STAGES_BY_COMPONENT with an empty stage list.
---
Nitpick comments:
In `@plugins/shared/scripts/doctor.py`:
- Around line 376-388: Replace ProcessPoolExecutor with ThreadPoolExecutor in
the job-analysis execution around _analyze_single_job, preserving the existing
max_parallel setting, submitted arguments, and future-to-job mapping.
- Around line 285-291: Update _analyze_single_job to return an explicit timeout
flag alongside validation_errors, and replace the substring-based timed_out
calculation in the report-building block with that structured value. Ensure
every return path supplies the flag, including timeout and non-timeout cases, so
the status continues to distinguish TIMED OUT from NO STOP HOOK without relying
on error-message wording.
- Around line 593-625: Extract the shared cost-summary construction from
_print_cost_summary and _write_cost_diagnostics into one helper that returns the
fully formatted lines, preserving the existing stage, release, total, and
grand-total text. Update _print_cost_summary to log each returned line and
_write_cost_diagnostics to append the same lines to diagnostics.txt, removing
the duplicated calculations and formatting.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: ad234619-2d0a-463a-9e19-e9e08e655b24
📒 Files selected for processing (10)
.claude-plugin/marketplace.jsonplugins/lvms-ci/.claude-plugin/plugin.jsonplugins/lvms-ci/hooks/hooks.jsonplugins/lvms-ci/scripts/doctor.pyplugins/microshift-ci/.claude-plugin/plugin.jsonplugins/microshift-ci/hooks/hooks.jsonplugins/microshift-ci/scripts/doctor.pyplugins/shared/scripts/create-report.pyplugins/shared/scripts/doctor.pyplugins/shared/scripts/validate-rca-output.py
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 `@plugins/shared/scripts/doctor.py`:
- Line 654: Wrap the lines.append call formatting release, job count, cost, and
average cost in plugins/shared/scripts/doctor.py lines 654-654 across multiple
lines, and wrap the delegated parser call in parentheses at lines 721-721;
preserve both statements’ behavior while meeting PEP 8 line-length requirements.
- Around line 234-240: In plugins/shared/scripts/doctor.py lines 234-240, update
run_doctor_sh’s _kill timeout handler to send SIGTERM, wait a short grace
period, then send SIGKILL to the process group if it remains alive so the output
loop cannot stay blocked. In plugins/shared/scripts/doctor.py lines 786-792,
update _run_claude_session to use a bounded proc.wait() after SIGTERM, then send
SIGKILL when needed and wait again to ensure the process is reaped.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: d4f14dbd-a3e8-4169-9d78-3c9cc509f6ec
📒 Files selected for processing (1)
plugins/shared/scripts/doctor.py
93068b3 to
f626119
Compare
|
Addressed CodeRabbit review-body nitpick findings:
|
Auto-applied: - doctor.py:252: Fix race between proc.wait() and timeout timer - create-report.py:2107: Guard diagnostics read with UTF-8 encoding - doctor.py:170: Truncate diagnostics.txt at pipeline start - doctor.py:141: Restrict --component to supported values via argparse choices Co-Authored-By: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Auto-applied: - doctor.py:252: Fix race between proc.wait() and timeout timer - create-report.py:2107: Guard diagnostics read with UTF-8 encoding - doctor.py:170: Truncate diagnostics.txt at pipeline start - doctor.py:141: Restrict --component to supported values via argparse choices Co-Authored-By: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
20599f8 to
a8137e9
Compare
coderabbitai
Bot
added
the
ready-for-human-review
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/shared/scripts/create-report.py (1)
2115-2115: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winWrap the extended
generate_htmlcall.Per CONTRIBUTING.md, Python code must follow PEP 8. This call is 267 characters and leaves the line overlong; format the arguments across multiple lines. Severity: medium.
🤖 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/create-report.py` at line 2115, Reformat the generate_html call in the report-generation flow so its arguments are split across multiple indented lines according to PEP 8, including the keyword arguments, while preserving the existing argument order and behavior.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/shared/scripts/create-report.py`:
- Around line 2107-2111: Update the HTML report output file opening in the
report-generation flow to explicitly use UTF-8 encoding, matching the decoding
of diagnostics_text. Preserve the existing output behavior while ensuring
non-ASCII content is written consistently regardless of the system default
encoding.
In `@plugins/shared/scripts/doctor.py`:
- Around line 170-171: Update the new-run initialization around diagnostics_file
and the prepare-stage flow to remove or namespace existing stream *.log files
when prepare starts a fresh pipeline. Ensure downstream-only invocations that
reuse the workdir preserve those logs, while compute_costs() considers only logs
belonging to the current run.
---
Outside diff comments:
In `@plugins/shared/scripts/create-report.py`:
- Line 2115: Reformat the generate_html call in the report-generation flow so
its arguments are split across multiple indented lines according to PEP 8,
including the keyword arguments, while preserving the existing argument order
and behavior.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 4001ec73-2c9f-43aa-84b8-e87a3eee45fd
📒 Files selected for processing (2)
plugins/shared/scripts/create-report.pyplugins/shared/scripts/doctor.py
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/shared/scripts/run-doctor.py (1)
576-579: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winLLM Security (CWE-74): Improper Neutralization of Special Elements in Output Used by a Downstream Component ('Injection')
Reachability: Internal
Reachability path
● Entry plugins/shared/scripts/doctor-helper.sh:536 prepare │ ▼ ● Sink plugins/shared/scripts/run-doctor.pyRemove Jira write MCP tools from dry-run bug correlation.
bugs()invokes/microshift-ci:create-bugswithout--create, whileallowed_toolsincludesmcp__jira__jira_create_issue,mcp__jira__jira_update_issue, andmcp__jira__jira_add_comment. The pipeline report can surface CI artifacts to the agent, and the skill text does not enforce authorization. Keep this stage read-only; add these write tools only through a validated pipeline-level create 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/shared/scripts/run-doctor.py` around lines 576 - 579, Update the allowed_tools configuration used by bugs() for the dry-run /microshift-ci:create-bugs invocation to remove mcp__jira__jira_create_issue, mcp__jira__jira_update_issue, and mcp__jira__jira_add_comment. Keep Jira read tools available, and only expose the removed write tools through a validated pipeline-level create path.
🤖 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/pcp-graphs/README.md`:
- Around line 61-64: Update the command example in the README’s graph-generation
section to invoke the helper using the repository-root-relative path
plugins/microshift-ci/scripts/doctor-helper.sh instead of doctor-helper.sh,
while preserving the existing graphs and --workdir arguments.
In `@plugins/microshift-ci/skills/close-stale-bugs/SKILL.md`:
- Around line 189-191: Update the workflow references in
plugins/microshift-ci/skills/close-stale-bugs/SKILL.md lines 189-191 to use the
deterministic run-doctor.py pipeline, microshift-ci:create-bugs,
close-stale-bugs, and run-doctor.py --stages finalize sequence instead of
retired doctor workflow names. Update the corresponding “doctor skill”
references in plugins/microshift-ci/skills/continue-session/SKILL.md line 61 to
describe the same deterministic pipeline; no other workflow behavior changes are
needed.
In `@plugins/microshift-ci/skills/create-bugs/SKILL.md`:
- Line 597: Split the create-bugs skill so SKILL.md stays under 500 lines:
retain only the execution workflow there, and move the detailed Jira query
recipe and lengthy reference sections, including the run-doctor.py reference,
into a separate reference file. Update SKILL.md links or instructions to point
to the relocated material while preserving the workflow behavior.
---
Outside diff comments:
In `@plugins/shared/scripts/run-doctor.py`:
- Around line 576-579: Update the allowed_tools configuration used by bugs() for
the dry-run /microshift-ci:create-bugs invocation to remove
mcp__jira__jira_create_issue, mcp__jira__jira_update_issue, and
mcp__jira__jira_add_comment. Keep Jira read tools available, and only expose the
removed write tools through a validated pipeline-level create path.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 19922644-0734-4bac-9a17-3defc2704b2f
📒 Files selected for processing (19)
plugins/lvms-ci/README.mdplugins/lvms-ci/scripts/doctor-helper.shplugins/lvms-ci/scripts/doctor.shplugins/lvms-ci/scripts/run-doctor.pyplugins/lvms-ci/skills/doctor/SKILL.mdplugins/microshift-ci/README.mdplugins/microshift-ci/scripts/doctor-helper.shplugins/microshift-ci/scripts/doctor.shplugins/microshift-ci/scripts/pcp-graphs/README.mdplugins/microshift-ci/scripts/run-doctor.pyplugins/microshift-ci/skills/close-stale-bugs/SKILL.mdplugins/microshift-ci/skills/continue-session/SKILL.mdplugins/microshift-ci/skills/create-bugs/SKILL.mdplugins/microshift-ci/skills/doctor-refresh/SKILL.mdplugins/microshift-ci/skills/doctor/SKILL.mdplugins/microshift-ci/skills/fix-test-bugs/SKILL.mdplugins/shared/scripts/doctor-helper.shplugins/shared/scripts/repo-log.shplugins/shared/scripts/run-doctor.py
💤 Files with no reviewable changes (5)
- plugins/microshift-ci/scripts/doctor.sh
- plugins/lvms-ci/skills/doctor/SKILL.md
- plugins/lvms-ci/scripts/doctor.sh
- plugins/microshift-ci/skills/doctor/SKILL.md
- plugins/microshift-ci/skills/doctor-refresh/SKILL.md
ggiguash
left a comment
ggiguash
left a comment
There was a problem hiding this comment.
Code review — top 3 findings (from automated analysis)
| _active_children.discard(proc) | ||
|
|
||
|
|
||
| def _kill_all_children(signum, frame): |
There was a problem hiding this comment.
Deadlock risk in signal handler
_kill_all_children acquires _children_lock, which is a non-reentrant threading.Lock. If SIGTERM arrives while the main thread holds this same lock inside _register_child or _unregister_child (e.g. during the prepare/graphs/finalize stages when run_doctor_sh spawns subprocesses), the signal handler — which runs on the main thread — will try to acquire the already-held lock and deadlock permanently. The process hangs instead of shutting down.
Fix: switch to threading.RLock(), or avoid locking in the signal handler entirely (e.g. iterate a snapshot of the set and tolerate races).
| def extract_cost(self, log_path): | ||
| """Extract cost_usd and duration_ms from a stream-json result event.""" | ||
| try: | ||
| with open(log_path) as f: |
There was a problem hiding this comment.
UnicodeDecodeError can crash the finalize stage
extract_cost() opens log files in strict text mode (no errors="replace"). If a claude -p session writes non-UTF-8 bytes to its stream-json log (stderr is merged via subprocess.STDOUT), a UnicodeDecodeError is raised. This is a subclass of ValueError, not OSError, so it escapes the except OSError handler, crashing compute_costs() and preventing the finalize stage from producing an HTML report.
The sister function _extract_job_stats (line 745) already correctly uses errors="replace" — this one should too.
| return True | ||
|
|
||
| releases_str = ",".join(self.releases) | ||
| prompt = f"/microshift-ci:create-bugs {releases_str}" |
There was a problem hiding this comment.
bugs() stage silently finds no data with custom --workdir
When --workdir /tmp/my-custom-dir is used, the analyze stage writes job files to /tmp/my-custom-dir/jobs/. However, the bugs() method spawns a claude -p session invoking /microshift-ci:create-bugs, and that skill's SKILL.md (line 74) hardcodes the workdir to /tmp/microshift-ci-claude-workdir.<YYMMDD>. The skill looks in the hardcoded path, finds no job files, and bug correlation silently produces no results.
The --add-dir flag only grants filesystem access — it does not override the skill's internal workdir. Consider passing the actual workdir as an argument to the skill.
ggiguash
left a comment
ggiguash
left a comment
There was a problem hiding this comment.
Code review — remaining findings (4–10)
Finding 7 (not inline — file not in this diff):
CONTRIBUTING.md lines 97 and 161 cite /microshift-ci:doctor as the canonical example of a skill, but this PR deletes plugins/microshift-ci/skills/doctor/SKILL.md. Contributors following the guide will reference a skill that no longer exists.
| Include only keys that were closed successfully (not failed). Do not write this file in dry-run mode or if no bugs were closed. | ||
|
|
||
| This file is consumed by `/microshift-ci:doctor-refresh` to exclude closed bugs from the HTML report. | ||
| This file is consumed by `run-doctor.py --stages finalize` to exclude closed bugs from the HTML report. |
There was a problem hiding this comment.
Documentation claims finalize excludes closed bugs, but it doesn't
This line states that closed-bugs.json is consumed by run-doctor.py --stages finalize to exclude closed bugs from the HTML report. However, the finalize() method (run-doctor.py line 586–588) calls doctor-helper.sh finalize with no --ignore flag and does not read close-stale-bugs/closed-bugs.json. A user who runs close-stale-bugs --close and then re-runs finalize will still see the closed bugs in the HTML report.
The refresh subcommand in doctor-helper.sh supports --ignore, but finalize() never uses it.
| log.info("Bug correlation is microshift-only, skipping") | ||
| return True | ||
|
|
||
| releases_str = ",".join(self.releases) |
There was a problem hiding this comment.
Rebase PR failures excluded from bug correlation
The deleted doctor skill (Step 3) checked for rebase PR source identifiers from the PR jobs JSON (e.g. rebase-release-4.22) and appended them to the sources passed to create-bugs. The new bugs() method only passes release versions:
prompt = f'/microshift-ci:create-bugs {releases_str}'When --pull-requests is used, rebase PR failures are analyzed but never correlated with Jira bugs, and no bug-matches-rebase-*.json files are created. The HTML report's Bugs tab will be incomplete for PR failures.
| if final_text: | ||
| validation_errors.extend(_run_validation(final_text)) | ||
| try: | ||
| data = json.loads(final_text) |
There was a problem hiding this comment.
Schema-invalid RCA output counted as success
If the LLM returns valid JSON that fails schema validation (e.g. a dict instead of an array, or missing required fields), _run_validation appends errors to validation_errors but json.loads succeeds, the data is written to output_path, and saved=True is returned. The job is counted as [OK] in diagnostics.
"Post-hoc validation failed" warnings are logged but not reflected in the success/failure tally.
| # Module-level function for ProcessPoolExecutor (must be picklable) | ||
| # ---------------------------------------------------------------------- | ||
|
|
||
| def _load_validate_module(): |
There was a problem hiding this comment.
Redundant importlib loads per job + stale comment
_load_validate_module() re-imports validate-rca-output.py via importlib on every call with no caching. In _analyze_single_job, both _run_validation() and _extract_result_text_standalone() call it, so each job triggers 2 full module loads. With 64 parallel workers, that's 128+ redundant imports.
Also, the comment at line 714 says "for ProcessPoolExecutor (must be picklable)" but the code uses ThreadPoolExecutor (line 426), so the picklability constraint is moot and a simple module-level or cached import would work.
| return False, None | ||
| return success, final_text | ||
|
|
||
| def extract_cost(self, log_path): |
There was a problem hiding this comment.
Triple-parse of same log files with divergent error handling
In _analyze_single_job, each log file is parsed by _extract_result_text_standalone (line 884/891) and _extract_job_stats (line 904). Then compute_costs() (line 600) re-reads every log file a third time via extract_cost(). Both extract_cost and _extract_job_stats look for type=="result" records to get cost_usd, but extract_cost lacks errors="replace" (see the UnicodeDecodeError finding).
The stats dict from _extract_job_stats already contains cost_usd but compute_costs() doesn't use it.
| _unregister_child(proc) | ||
| final_text = _extract_result_text_standalone(log_path) | ||
| return proc.returncode == 0, final_text | ||
| except OSError: |
There was a problem hiding this comment.
Missing binary misdiagnosed as timeout
If the claude binary is not found or not executable, subprocess.Popen raises OSError, which is caught here and returns (None, None) — the same sentinel as a timeout. The caller at line 285 checks if success is None and logs ERROR: claude -p timed out after Ns, which is misleading — the process never started.
Consider returning a distinct sentinel or logging the OSError message so the operator sees the real problem.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation