ci: add LGTM Bot with merge readiness tracking and AI-powered CI diagnostics#6066
ci: add LGTM Bot with merge readiness tracking and AI-powered CI diagnostics#6066dagil-nvidia wants to merge 61 commits into
Conversation
- Automatic `lgtm` label when all CI checks pass, PR is approved, and branch protection is satisfied; removed on new push - Live merge checklist comment updated on every CI/review/push event - On-demand `/lgtm-bot diagnose` command with LLM-powered CI failure diagnosis via NVIDIA Inference API (Claude Opus 4.6) - Auto-request CODEOWNERS team reviews and post visibility comment on PR open with fork CI detection (/ok to test per-commit) - PR size labels (XS/S/M/L/XL) based on lines changed - Stale review nudge (48h) and merge reminder (24h with lgtm label) - Slack notification when lgtm label is newly added (opt-in) - Update CONTRIBUTING.md with merge readiness section and bot docs Signed-off-by: Dan Gil <dagil@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
/ok to test f8f4cde |
github-actions
Bot
added
ci
Size labels (XS/S/M/L/XL/XXL) are already applied by an existing integration. Remove the redundant Feature 5 logic from lgtm-bot-pr-open.yml to avoid conflicts and label churn. Signed-off-by: Dan Gil <dagil@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
/ok to test 43a992c |
WalkthroughAdds an LGTM bot: multiple GitHub Actions workflows, a diagnostics script, and an error-classification package to automate PR onboarding, CODEOWNERS review requests, live merge checklist and lgtm label management, stale nudges/merge reminders, and on-demand /diagnose diagnostics (optional LLM-assisted CI failure classification). Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 10
🤖 Fix all issues with AI agents
In @.github/scripts/lgtm_bot_diagnose.py:
- Around line 652-661: The code currently assumes pr["head"]["repo"] is always
present and does is_fork = pr["head"]["repo"]["full_name"] != f"{OWNER}/{REPO}",
which crashes when pr["head"]["repo"] is None (deleted fork). Modify main() to
safely fetch head_repo = pr["head"].get("repo") (or use pr["head"]["repo"] and
check for None), then compute is_fork by treating a missing head_repo as a fork
(e.g., is_fork = head_repo is None or head_repo.get("full_name") !=
f"{OWNER}/{REPO}"), and use head_repo when needed instead of indexing
pr["head"]["repo"] directly.
- Around line 622-631: post_diagnostic_comment currently only requests the first
100 comments via github_request and can miss an existing DIAGNOSIS_MARKER when a
PR has >100 comments; update post_diagnostic_comment to paginate through
comments (e.g., loop requesting page=1..N with per_page=100 until no results) or
request comments sorted newest-first (add direction=desc or sort=created if
supported) and search each page for DIAGNOSIS_MARKER before creating a new
comment, using the existing github_request call and OWNER/REPO identifiers to
locate pages and stop when no further comments are returned.
- Around line 134-159: The unanchored matching branch in match_codeowners
currently only tests the basename with fnmatch, which fails when an unanchored
pattern contains '/' (e.g., "docs/*.md"); update the unanchored logic in
match_codeowners to: if the pattern contains a '/' use
pathlib.PurePath(filepath).match(pattern) to allow matching at any depth
(GitHub-style path matching), otherwise keep the existing filename-only fnmatch
against the basename; adjust the branch that computes filename so it only
applies to filename-only patterns and leave the anchored and directory branches
unchanged.
In @.github/workflows/lgtm-bot-diagnose.yml:
- Around line 42-61: The workflow's diagnose job currently triggers for any
commenter and exposes secrets/API quota; update the job "diagnose" condition to
gate execution to either (a) commenters who are repo collaborators/org members
by checking github.event.comment.author_association (e.g., allow MEMBER,
COLLABORATOR, OWNER) or (b) the PR author by allowing github.actor ==
github.event.issue.user.login, so only authorized users can reach the step that
runs .github/scripts/lgtm_bot_diagnose.py and uses
GITHUB_TOKEN/NVIDIA_INFERENCE_API_KEY; implement the chosen check in the job's
if expression to prevent untrusted comment triggers.
In @.github/workflows/lgtm-bot-pr-open.yml:
- Around line 62-63: The size label calculation currently uses the incoming pr
payload (totalChanges = pr.additions + pr.deletions) which can be stale for
synchronize events; update the logic to compute totalChanges from the freshly
fetched PR object you already retrieve for CODEOWNERS (use the re-fetched pr
response instead of the original payload) so SIZE_LABELS and sizeLabel are
derived from the up-to-date PR data (reference: the pr variable, totalChanges,
SIZE_LABELS, and sizeLabel).
- Around line 140-186: The matchFile function incorrectly handles unanchored
CODEOWNERS patterns that contain a slash by matching only the filename; update
matchFile so that when a pattern p (from rules) is not anchored but contains '/'
you test the full filepath (using globMatch(filepath, clean)) instead of
filepath.split('/').pop(), remove the redundant second globMatch(filepath,
clean) branch inside the anchored block, and ensure directory patterns
(clean.endsWith('/')) still respect anchored vs unanchored semantics; refer to
the globMatch and matchFile functions, rule.pattern (p), clean, anchored, and
rules to locate and fix the logic.
In @.github/workflows/lgtm-bot-stale.yml:
- Around line 88-133: The "daysWaiting" value is computed from prAge (derived
from pr.created_at) which can misrepresent how long requested reviewers have
actually been waiting; update the logic that sets daysWaiting to use the
timestamp of the most recent review request instead of pr.created_at: fetch the
PR timeline events (e.g., via github.rest.issues.listEvents or
issues.listEventsForTimeline) and find the latest event with type
"review_requested" or action indicating a review request, compute daysWaiting
from (now - new Date(latestReviewRequest.created_at)), and fall back to prAge
only if no review-request event exists; adjust any references to daysWaiting and
keep the existing NUDGE_MARKER/comment posting flow.
- Line 28: The cron comment is misleading because "9am PT (UTC-8)" only holds
during PST; update the comment next to the cron schedule string cron: '0 17 * *
1-5' to indicate DST, e.g. "9am PT (UTC-8 in standard time, UTC-7 in daylight
saving time)" or similar; locate the cron line cron: '0 17 * * 1-5' and replace
its trailing comment so it accurately reflects both PST and PDT.
In @.github/workflows/lgtm-bot.yml:
- Around line 244-263: The synchronize handler currently rebuilds the checklist
with hasApproval: false and hasChangesRequested: false which causes a transient
incorrect UI; update the branch inside the if (context.eventName ===
'pull_request_target') block (around REQUIRED_CHECKS, buildChecklist,
updateChecklist calls) to fetch current PR reviews and compute proper
hasApproval/hasChangesRequested before calling buildChecklist (or alternatively
pass a flag/message that approvals are being rechecked) so the checklist
reflects actual review state for prNumber/headSha/isFork instead of always
resetting approvals to false; ensure removeLabel remains unchanged and then call
updateChecklist with the computed values.
- Around line 154-162: The current heuristic treats a `pending` pmStatus as "not
triggered"; instead detect whether the check run exists and only show the
"maintainer must run" note when the check run is absent. Update the isFork
branch: compute a boolean like hasPm =
Object.prototype.hasOwnProperty.call(checks, 'pre-merge-status-check'), read
pmStatus = checks['pre-merge-status-check'], then set triggered = hasPm &&
pmStatus !== 'completed' (so present-but-pending/running counts as triggered),
and only include the note when !hasPm; keep using headSha.slice(0,7] and the
same lines.push code paths.
🧹 Nitpick comments (6)
.github/scripts/lgtm_bot_diagnose.py (3)
40-45: Module-level env reads will crash on import if vars are missing.Lines 40 and 42 use
os.environ["..."]which raisesKeyErrorimmediately if the env var is absent. This is acceptable for a script that's only invoked by a controlled workflow, but consider wrapping the reads inmain()or adding a friendlier error message to aid debugging if the workflow configuration drifts.
536-540: Uses globalPR_NUMBERinstead of theprparameter for building URLs.
build_diagnostic_commentreceivespras a parameter but references the module-levelPR_NUMBERat line 538. This couples the function to global state and would break if it were ever called with a different PR number.Proposed fix
- lines.append( - f"- No approved reviews — " - f"[request a review]" - f"(https://github.com/{OWNER}/{REPO}/pull/{PR_NUMBER}" - f"/reviewers)" - ) + pr_number = pr["number"] + lines.append( + f"- No approved reviews — " + f"[request a review]" + f"(https://github.com/{OWNER}/{REPO}/pull/{pr_number}" + f"/reviewers)" + )
97-111: GraphQL helper lacks error handling compared to REST helper.
github_graphqldoesn't handleHTTPError(unlikegithub_request) and also doesn't check for GraphQL-levelerrorsin the response payload. A GraphQL error (e.g., rate limit or schema issue) will return HTTP 200 with anerrorskey, silently producing bad data downstream.Proposed fix — log GraphQL errors
def github_graphql(query: str, variables: dict | None = None) -> Any: """Make a GitHub GraphQL API request.""" payload = {"query": query} if variables: payload["variables"] = variables url = "https://api.github.com/graphql" headers = { "Authorization": f"Bearer {GITHUB_TOKEN}", "Content-Type": "application/json", } body = json.dumps(payload).encode() req = urllib.request.Request(url, data=body, headers=headers, method="POST") - with urllib.request.urlopen(req) as resp: - return json.loads(resp.read().decode()) + try: + with urllib.request.urlopen(req) as resp: + result = json.loads(resp.read().decode()) + if "errors" in result: + print(f"GraphQL errors: {result['errors']}", file=sys.stderr) + return result + except urllib.error.HTTPError as e: + error_body = "" + try: + error_body = e.read().decode() + except Exception: + pass + print(f"GraphQL API error: {e.code} {e.reason} {error_body}", file=sys.stderr) + raise.github/workflows/lgtm-bot-stale.yml (1)
72-133: Each PR triggers at least one API call forlistRequestedReviewers— potential rate-limit concern.For repos with hundreds of open PRs, this loop makes O(N) API calls minimum. GitHub Actions
GITHUB_TOKENhas a rate limit of 1,000 requests/hour for the REST API. Consider filtering PRs by age first (skip recently updated PRs) or adding an earlycontinueifpr.updated_atis recent enough that the review can't be stale..github/workflows/lgtm-bot-pr-open.yml (2)
253-259:teamReasonrelies on keyword matching in team names — fragile.If team names don't contain
'rust','python','Devops', or'deploy', the fallback is the generic "Files changed." This won't provide useful information. Also note the inconsistent casing ('Devops'vs lowercase others).Consider deriving the reason from file extensions or paths instead, which is more robust.
Proposed alternative: derive reason from files
- function teamReason(team) { - if (team.includes('rust')) return 'Rust files changed'; - if (team.includes('python')) return 'Python files changed'; - if (team.includes('Devops')) return 'CI/infra files changed'; - if (team.includes('deploy')) return 'Deploy files changed'; - return 'Files changed'; - } + function teamReason(team, files) { + const exts = files.map(f => f.split('.').pop()).filter(Boolean); + if (exts.some(e => ['rs', 'toml'].includes(e))) return 'Rust files changed'; + if (exts.some(e => ['py', 'pyi'].includes(e))) return 'Python files changed'; + if (files.some(f => f.startsWith('.github/') || f.startsWith('ci/'))) return 'CI/infra files changed'; + if (files.some(f => f.startsWith('deploy/'))) return 'Deploy files changed'; + return 'Files changed'; + }And update the call site:
- commentLines.push(`| ${team} | ${teamReason(team)} | ${fileList}${more} |`); + commentLines.push(`| ${team} | ${teamReason(team, files)} | ${fileList}${more} |`);
25-31:pull_request_targetwithsynchronizeruns on every push — consider the cost.This workflow runs CODEOWNERS parsing, label management, and potentially review requests on every push to a PR. For active PRs with frequent pushes, the CODEOWNERS comment updates and size label changes are redundant if files haven't changed. This is not a blocker but worth noting for optimization later.
- Fix crash when fork repo is deleted (head.repo can be None) - Fix CODEOWNERS matching for unanchored patterns with path separators (both Python and JS implementations) - Add auth gate on /lgtm-bot diagnose (block only NONE association, allow all contributors including first-timers) - Paginate diagnostic comment search from newest first - De-fork /ok to test guidance: show CI trigger instructions for ALL PRs, not just forks (internal members without GPG also need it) - Improve CI trigger heuristic: check if check run exists rather than assuming pending means not triggered - Fetch reviews in synchronize path to keep checklist accurate - Fix stale nudge days calculation (use last update, not PR age) - Fix cron comment (PST/PDT clarification) Signed-off-by: Dan Gil <dagil@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
/ok to test f1fb525 |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @.github/scripts/lgtm_bot_diagnose.py:
- Around line 239-266: The fallback that inspects commit statuses (the
status_match from combined.get("statuses")) wrongly treats any non-"success"
state as a failure; update the mapping in the block that builds results[name] so
that only explicit failure states (e.g., "failure", "error", "cancelled" or
"closed") append to failing and set "status":"fail", while transient states like
"pending" (and possibly "queued" or "in_progress") should set results[name] =
{"status":"pending", "url": ...} instead of "fail"; ensure you only append to
the failing list for the explicit failure states and preserve the original
target_url extraction.
In @.github/workflows/lgtm-bot-pr-open.yml:
- Around line 252-258: The current call to github.rest.issues.listComments (used
to find the CODEOWNERS_MARKER comment) only fetches a single page with per_page:
100 and can miss older comments on very active PRs; update the lookup to
paginate through all pages (or request from newest) using Octokit pagination
(e.g., octokit.paginate or passing page params in a loop) when retrieving
comments into the comments variable so the search for CODEOWNERS_MARKER reliably
finds existing bot comments and prevents duplicates.
In @.github/workflows/lgtm-bot.yml:
- Around line 116-139: The updateChecklist function currently calls
github.rest.issues.listComments with per_page: 100 which only fetches the first
page and can miss the existing checklist comment; modify updateChecklist to
paginate through all comment pages (or request newest-first and stop early) when
calling github.rest.issues.listComments so you reliably find the comment
containing CHECKLIST_MARKER before deciding to update or create; keep the
existing updateComment/createComment logic (references: updateChecklist,
CHECKLIST_MARKER, github.rest.issues.listComments,
github.rest.issues.updateComment, github.rest.issues.createComment).
- Around line 330-344: The fallback that maps commit status in
combinedStatus.statuses (found as statusMatch) currently treats any
non-'success' state as 'fail'; update the conditional around statusMatch.state
in this block to distinguish 'pending' explicitly: set checkResults[name] =
'pass' for 'success', set checkResults[name] = 'pending' for 'pending' (and
ensure allChecksPassing = false), and set checkResults[name] = 'fail' for other
states (also setting allChecksPassing = false); modify the logic around
statusMatch, checkResults, and allChecksPassing accordingly so the checklist
shows running for pending statuses rather than marking them failing.
🧹 Nitpick comments (4)
.github/workflows/lgtm-bot-pr-open.yml (1)
213-220:teamReasonheuristic uses case-sensitive substring matching — may miss teams.
team.includes('Devops')is case-sensitive. If the team slug isdevopsorDevOps, this won't match. Consider usingteam.toLowerCase().includes('devops')for robustness.Suggested fix
function teamReason(team) { - if (team.includes('rust')) return 'Rust files changed'; - if (team.includes('python')) return 'Python files changed'; - if (team.includes('Devops')) return 'CI/infra files changed'; - if (team.includes('deploy')) return 'Deploy files changed'; + const t = team.toLowerCase(); + if (t.includes('rust')) return 'Rust files changed'; + if (t.includes('python')) return 'Python files changed'; + if (t.includes('devops')) return 'CI/infra files changed'; + if (t.includes('deploy')) return 'Deploy files changed'; return 'Files changed'; }.github/workflows/lgtm-bot-stale.yml (1)
52-67: Potential GitHub API rate-limit pressure for repos with many open PRs.This loop makes 1+ API calls per open non-draft PR (at minimum
listRequestedReviewers, plus conditionallylistCommentsandlistEvents). With a GITHUB_TOKEN rate limit of 1,000 requests/hour for Actions, a repo with ~200+ open PRs could approach the limit in a single run.Consider adding a batch limit (e.g., process only the first N PRs sorted by oldest update) or short-circuiting PRs that clearly don't need nudging before making additional API calls.
.github/scripts/lgtm_bot_diagnose.py (2)
464-475:is_forkparameter is unused inbuild_diagnostic_comment.Static analysis (Ruff ARG001) correctly flags that
is_forkon line 472 is never referenced in the function body. It's passed frommain()at line 726 but has no effect.Either remove the parameter or use it for fork-specific messaging (e.g., the CI trigger section currently uses the
ci_not_triggeredheuristic regardless of fork status).Option A: Remove unused parameter
def build_diagnostic_comment( pr: dict, ci_results: dict, failing_checks: list[str], approvals: list[str], changes_requested: list[str], unresolved: list[dict], team_files: dict[str, list[str]], - is_fork: bool, head_sha: str, llm_response: str | None, ) -> str:And in
main():comment_body = build_diagnostic_comment( pr, ci_results, failing_checks, approvals, changes_requested, unresolved, team_files, - is_fork, head_sha, llm_response, )
40-43: Missing env vars will crash with an unhelpfulKeyError.
os.environ["GITHUB_TOKEN"]andint(os.environ["PR_NUMBER"])will raiseKeyError/ValueErrorwith no guidance on what's missing. Consider wrapping in a startup check with a clear error message since this script is user-facing (invoked via/lgtm-bot diagnose).Suggested improvement
-GITHUB_TOKEN = os.environ["GITHUB_TOKEN"] -GITHUB_REPOSITORY = os.environ.get("GITHUB_REPOSITORY", "ai-dynamo/dynamo") -PR_NUMBER = int(os.environ["PR_NUMBER"]) +GITHUB_TOKEN = os.environ.get("GITHUB_TOKEN") +if not GITHUB_TOKEN: + sys.exit("Error: GITHUB_TOKEN environment variable is required") +GITHUB_REPOSITORY = os.environ.get("GITHUB_REPOSITORY", "ai-dynamo/dynamo") +_pr_num = os.environ.get("PR_NUMBER") +if not _pr_num: + sys.exit("Error: PR_NUMBER environment variable is required") +try: + PR_NUMBER = int(_pr_num) +except ValueError: + sys.exit(f"Error: PR_NUMBER must be an integer, got: {_pr_num}")
… comments - Handle pending commit status separately from fail in both Python and JS (DCO check processing shows as "running" not "failing") - Paginate comment search from newest first in updateChecklist and CODEOWNERS comment (prevents duplicates on PRs with 100+ comments) - Brand all bot comments with "LGTM Bot" header for clear identity Note: comments currently post as github-actions[bot]. A dedicated GitHub App account (lgtm-bot[bot]) can be created in a future update once the bot proves useful. Signed-off-by: Dan Gil <dagil@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
/ok to test 8561dd1 |
- Update CONTRIBUTING.md step 7 to explain that /ok to test applies to all PRs, not just forks — org members with GPG-signed commits trigger CI automatically and can skip this step - Add "CI Triggering" subsection with link to GitHub GPG docs - Update bot comments (CODEOWNERS visibility, diagnostics) to mention the GPG signing exception - Change "Fork CI status" to "CI trigger status" since it applies to all PRs Signed-off-by: Dan Gil <dagil@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
/ok to test 8f36874 |
dillon-cullinan
left a comment
dillon-cullinan
left a comment
There was a problem hiding this comment.
What is the purpose of this PR?
I read the CONTRIBUTING and I'm not understanding what this is giving us that the github UI doesn't already show.
🤖 LGTM Bot — Merge Checklist
This checklist updates automatically when CI completes, reviews change, or new commits are pushed. |
Consolidate /lgtm-bot diagnose and /classify-errors into a single /diagnose slash command. Refactor error classification module, fix bugs found during multi-perspective code review, and harden all workflows for production use. Key decision: A single /diagnose command replaces two overlapping commands. The diagnose workflow already embeds the error classifier output when CI is failing, making the standalone /classify-errors redundant. /lgtm-bot diagnose remains as a backward-compatible alias. Changes: Unified /diagnose command: - Trigger on /diagnose (primary) and /lgtm-bot diagnose (alias) using startsWith() to prevent false triggers from casual mentions - Delete lgtm-bot-classify-errors.yml (redundant) - Add concurrency group per PR to prevent duplicate runs Reviewer assignment consolidation: - Merge assign-reviewers job into pr-onboard (eliminates ~200 lines of duplicated CODEOWNERS/glob/file-fetching code) - Support both CODEOWNERS team resolution and YAML roster - Add example YAML format in code and docs Error classification refactor: - Rename anthropic_* config fields to api_key/model (NVIDIA is now the primary backend, not Anthropic) - Deduplicate summary prompt and retry logic in LLM client - Remove dead code: ClassificationResult, call_llm(), 6 unused config fields, redundant run_id computation - Move classify_workflow_errors.py to .github/scripts/ Bug fixes (found via DevOps/Dev/Leadership review simulation): - Fix /lgtm-bot diagnose alias (contains() didn't match substring) - Restore config.max_error_length (removed but still referenced) - Fix stdout contamination in OUTPUT_ONLY mode (bare print() in helper functions polluted markdown output for subprocess capture) - Add threading.Lock to RateLimiter (used concurrently via ThreadPoolExecutor without synchronization) - Fix is_mergeable to check changes_requested - Fix fetch_failed_logs stderr, graphql error handling, silent CODEOWNERS exception swallowing - Fix sys.exit(0) on classifier exception -> sys.exit(1) Hardening: - Pin all actions to SHA (supply chain security) - Add timeout-minutes to all jobs (5-15 min vs 6-hour default) - Add continue-on-error on classifier step (observability, not gate) - Add 5xx retry with exponential backoff in LLM client - Make NVIDIA_INFERENCE_API_KEY optional in reusable workflow - Add NVIDIA_INFERENCE_MODEL as reusable workflow input Contributor experience: - Tier-specific CI trigger guidance (first-time, L1, L2+/org) - Auto-update footer on bot comments - Updated LGTM_BOT.md with all workflows, secrets, and vars - Updated CONTRIBUTING.md with /diagnose as primary command Signed-off-by: Dan Gil <dagil@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
dagil-nvidia
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
1 similar comment
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In @.github/scripts/lgtm_bot_diagnose.py:
- Around line 352-372: The diagnose_reviews function currently calls
github_request once and can miss reviews because GitHub paginates results;
modify diagnose_reviews to page through the reviews endpoint (e.g., add page and
per_page params to github_request calls, using per_page=100 and incrementing
page until an empty page is returned) and accumulate all review objects into the
reviews list before the existing latest_by_user/approvals logic; keep using
OWNER and REPO in the endpoint string and reuse github_request to fetch each
page.
- Around line 82-93: The github_request function calls urllib.request.urlopen
without a timeout and can hang; match github_graphql by passing a timeout (e.g.,
timeout=30) to urllib.request.urlopen(req, timeout=30) or use a shared constant
for the timeout value, keeping the existing HTTPError handling intact (preserve
the error_body read/print/raise logic) so the request won't block indefinitely.
In @.github/workflows/lgtm-bot.yml:
- Around line 386-390: The call to github.rest.pulls.listReviews is not
paginated so PRs with >30 reviews can miss reviews; replace the single-page call
(const { data: reviews } = await github.rest.pulls.listReviews({...})) with a
paginated fetch using the GitHub client's paginate helper (e.g., use
github.paginate(github.rest.pulls.listReviews, { owner: context.repo.owner,
repo: context.repo.repo, pull_number: prNumber }) to collect all review pages
into a single reviews array) and then use that aggregated reviews array for
approval/state logic; apply the identical pagination change to the synchronize
path that also calls github.rest.pulls.listReviews.
- Around line 488-513: The curl invocation using SLACK_WEBHOOK_URL can leak the
secret in curl's error output; update the run step that calls curl (the command
referencing SLACK_WEBHOOK_URL and using -sSf) so stderr is suppressed or removed
from logs—e.g., remove -S or redirect curl's stderr to /dev/null (append
2>/dev/null) or otherwise avoid printing curl's error details; target the curl
command in the workflow where SLACK_WEBHOOK_URL, PR_NUMBER, PR_TITLE, PR_URL,
and PR_AUTHOR are used and ensure the secret URL is not emitted on failure.
- Around line 117-149: The issue: updateChecklist currently passes the
unsupported direction option and fetches comments oldest-first, making the
search inefficient. Fix it in updateChecklist by removing the direction
parameter and use octokit pagination to get all comments then search from
newest: call github.paginate(github.rest.issues.listComments, { owner:
context.repo.owner, repo: context.repo.repo, issue_number: prNumber, per_page:
100 }) to retrieve all comments, then find the most recent comment containing
CHECKLIST_MARKER by iterating the returned array from the end (or using
.reverse().find) and assign it to existing before updating/creating the comment
as done now.
In `@error_classification/classifier.py`:
- Around line 198-211: The token fields (prompt_tokens, completion_tokens,
cached_tokens) are currently copied from result to every ErrorClassification in
classifier.py, inflating aggregated totals in classify_workflow_errors.py; fix
by computing the total tokens once from result (result.get("prompt_tokens",0),
etc.), dividing them evenly across the number of classifications you are
creating (num_errors = len(classifications) or similar), assign tokens_per =
total // num_errors for each, add any remainder to the first classification (or
the first few) so totals sum to the original, and then set
prompt_tokens/completion_tokens/cached_tokens on each ErrorClassification
accordingly (use the existing error_data/result variables and the
ErrorClassification construction block to apply the per-class values).
In `@error_classification/claude_client.py`:
- Around line 38-56: The wait_if_needed method in RateLimiter captures now
before possibly sleeping, then appends that stale timestamp to self.requests;
change the logic so that after any sleep you record the current time (recompute
now or use a new timestamp) and append that fresh timestamp to self.requests
(keeping the operation inside the same self._lock block) so the request reflects
the actual time it occurred and the rate window is correct.
In `@error_classification/config.py`:
- Around line 59-67: The MAX_RPM environment parsing currently uses
int(os.getenv("MAX_RPM", "50")) which raises an unhelpful ValueError if the env
var is non-numeric; change Config construction to use a safe parser (implement a
static/helper method named _parse_int_env(name: str, default: int) that reads
os.getenv(name), returns default on empty, and raises a clear ValueError like
"{name} must be an integer, got: {raw!r}" on invalid input) and call it instead
of int(...) when setting max_rpm in the cls(...) return.
In `@error_classification/pr_commentator.py`:
- Around line 319-340: The _find_pr_by_commit function returns the raw JSON
value for pulls[0].get("number") which may be None or non-int; update
_find_pr_by_commit to coerce and validate the PR number before returning:
extract the value from pulls[0].get("number"), check it's not None and is an
int-like value (e.g., str of digits or int), convert with int(...) and return
that, otherwise log/ignore and return None; ensure the function signature
Optional[int] is honored and keep the existing exception handling in
_find_pr_by_commit.
- Around line 170-207: The requests.patch and requests.post calls that
post/update GitHub comments (using existing_comment_id, pr_number in
pr_commentator) lack a timeout and can hang; add a timeout parameter (e.g.,
timeout=10) to both requests.patch(...) and requests.post(...) calls, and also
ensure the helper _find_existing_comment (referenced elsewhere in this module)
is updated to pass timeout to any requests it makes so all HTTP calls in this
class use a consistent timeout value.
🧹 Nitpick comments (16)
.github/workflows/lgtm-bot-stale.yml (1)
97-103: The cooldown check only scans the last 10 comments — could miss an old nudge in busy PRs.If a PR receives many non-bot comments (>10) within the 24h cooldown window, the bot won't find its previous nudge and may post a duplicate. This is unlikely in practice but worth noting.
A safer approach is to filter by
since(ISO timestamp for 24h ago) instead of relying on a smallper_page:💡 Suggested improvement
+ const sinceDate = new Date(now - NUDGE_COOLDOWN_HOURS * MS_PER_HOUR).toISOString(); const { data: comments } = await github.rest.issues.listComments({ owner: context.repo.owner, repo: context.repo.repo, issue_number: prNumber, - per_page: 10, - direction: 'desc', + since: sinceDate, + per_page: 100, });CONTRIBUTING.md (1)
239-245: Minor: Lines 243–245 describe overlapping functionality — consider consolidating."Automatic", "On-demand", and "Via diagnose" bullets under Error Classification have redundancy — the last two both describe
/diagnosetriggering classification.Suggested simplification
- **Automatic** – The error classifier runs as the final step in CI pipelines (`pre-merge`, `pr`, `ci-test-suite`, `container-validation-dynamo`, `copyright-checks`, `build-frontend-image`). When failures are detected, it posts a PR comment categorizing each as **infrastructure error** (network/runner issues) or **code error** (build/test/compilation failures). -- **On-demand** – Comment `/diagnose` on any PR to run classification along with the full merge-readiness diagnostic. -- **Via diagnose** – The `/diagnose` command automatically runs the error classifier when CI failures are present. +- **On-demand** – Comment `/diagnose` on any PR to run classification along with the full merge-readiness diagnostic. The error classifier is automatically included when CI failures are present..github/workflows/classify-workflow-errors-reusable.yml (1)
47-52:pip installruns outside the venv if the venv creation fails silently.The
sudo apt-get ... || trueon line 49 swallows errors. Ifpython3-venvinstallation fails, thepython3 -m venvon line 50 will also fail, but thepip installon line 52 would then install into the system Python (or fail outright). Since this step doesn't useset -e, a failed venv creation won't halt the step. Consider addingset -eor removing|| trueto fail fast.Suggested fix
- name: Install dependencies run: | - sudo apt-get update && sudo apt-get install -y python3-venv || true + sudo apt-get update && sudo apt-get install -y python3-venv python3 -m venv /tmp/error-classification-venv . /tmp/error-classification-venv/bin/activate pip install -r error_classification/requirements.txt.github/workflows/lgtm-bot-diagnose.yml (1)
67-72: Same|| truesilencing pattern as the reusable workflow — venv failures may go unnoticed.Same issue as in
classify-workflow-errors-reusable.yml:|| trueon line 69 swallows apt-get failures, risking a broken venv and confusing downstream errors..github/workflows/ci-test-suite.yml (1)
1001-1010:enable_pr_commentsdefaults to'true'but this workflow runs in nightly/post-merge contexts without a PR.This workflow is called by
nightly-ci.ymlandpost-merge-ci.yml(per the header comment). In those contexts, there's no PR to comment on. Whilecontinue-on-error: truein the reusable workflow prevents failures, you could explicitly passenable_pr_comments: 'false'to avoid unnecessary API calls and log noise.Suggested fix
classify-errors: if: always() && !cancelled() needs: [build-amd64, build-arm64, build-cuda13-amd64, build-cuda13-arm64, unit-tests, integration-tests, e2e-single-gpu-tests, e2e-multi-gpu-tests, fault-tolerance-tests] permissions: contents: read checks: write pull-requests: write uses: ./.github/workflows/classify-workflow-errors-reusable.yml + with: + enable_pr_comments: 'false' secrets: inherit.github/LGTM_BOT.md (1)
82-88:LGTM_BOT_REVIEWERS_PER_TEAMis documented in two separate tables.Lines 86 and 140 both document the same variable. Consider removing one to avoid drift if the default or description changes.
Also applies to: 136-141
.github/workflows/lgtm-bot-pr-open.yml (1)
310-316:teamReasonuses hardcoded substring checks — fragile for evolving team names.The function checks for substrings like
'rust','python','Devops','deploy'in team names. Note that'Devops'is case-sensitive (capital D), so a team nameddevopsorDevOpswouldn't match. This is minor but could produce less informative comments if team naming conventions change.error_classification/__init__.py (1)
14-31:__all__is not sorted (Ruff RUF022).Static analysis flags that
__all__is not isort-style sorted. Consider sorting for consistency.Proposed fix
__all__ = [ - # Config - "Config", - "get_config", - "ERROR_CATEGORIES", - # Classifier - "ErrorClassifier", - "ErrorClassification", - # Error context - "ErrorContext", - # Claude client + "CATEGORY_SEVERITY", "ClaudeClient", - # Prompts - "get_category_definitions", - # PR comments - "PRCommentator", - "CATEGORY_SEVERITY", + "Config", + "ERROR_CATEGORIES", + "ErrorClassification", + "ErrorClassifier", + "ErrorContext", + "PRCommentator", + "get_category_definitions", + "get_config", ]error_classification/config.py (1)
69-76:validate()is effectively unreachable forapi_keywhen usingfrom_env.
from_envalready raisesValueErrorifapi_keyis missing (Line 48-51), sovalidate()checkingapi_keyagain (Line 73) will never fire for instances created viafrom_env→get_config. This isn't a bug (it guards direct construction), butvalidate()could be extended to also checkapi_formatis one of{"openai", "anthropic"}— an invalidapi_formatsilently falls through to the Anthropic path inClaudeClient.__init__..github/scripts/classify_workflow_errors.py (3)
142-172:github_tokenandrepoparameters are unused.
fetch_job_stepsonly readsjob.get("steps")and never usesgithub_tokenorrepo. This creates a misleading interface suggesting an API call is made.Proposed fix
-def fetch_job_steps( - job: Dict[str, Any], github_token: str, repo: str -) -> List[Dict[str, Any]]: +def fetch_job_steps(job: Dict[str, Any]) -> List[Dict[str, Any]]:Update any call sites accordingly (though this function doesn't appear to be called in this file).
257-272:sys.stdoutreassignment is a global side-effect affecting all imported modules.In OUTPUT_ONLY mode,
sys.stdout = sys.stderr(Line 265) means everyprint()in third-party code (includingPRCommentator,ErrorClassifier, etc.) also writes to stderr. This is intentional, but if any called code caches a reference tosys.stdoutbefore this assignment (e.g.,PRCommentator.__init__), the output routing breaks. ThePRCommentatorclass usesprint()directly so it's fine, but this is fragile coupling.
214-248: Fallback returns the entire job log when step markers aren't found.Line 248 returns
full_logunfiltered, which for large jobs could be hundreds of thousands of characters. Whileclaude_client.pytruncates toMAX_LOG_LENGTH, the fallback still means the caller receives far more data than expected from a "step log extraction" function.Consider adding a size cap in the fallback:
Proposed fix
# Last resort: return full log (filtered later) - return full_log + # Cap to avoid passing excessively large text when markers aren't found + return full_log[-50000:] if len(full_log) > 50000 else full_logerror_classification/pr_commentator.py (1)
342-411:error_contextsparameter is unused in_build_summary_markdown_body.The parameter is declared (Line 343) but never referenced in the method body. Ruff ARG002 flags this. Either remove it or use it to enrich the markdown output.
error_classification/claude_client.py (3)
116-123: Log truncation logic is duplicated betweenanalyze_full_job_logand_analyze_full_log_openai_format.Lines 117-123 and Lines 362-367 contain identical truncation logic. Extract to a shared helper to keep the truncation strategy consistent and DRY.
Proposed fix
+ `@staticmethod` + def _truncate_log(job_log: str) -> str: + """Truncate log to MAX_LOG_LENGTH, keeping the tail (most relevant for failures).""" + if len(job_log) > MAX_LOG_LENGTH: + truncated_length = len(job_log) - MAX_LOG_LENGTH + return ( + f"[... truncated first {truncated_length} chars ...]\n\n" + + job_log[-MAX_LOG_LENGTH:] + ) + return job_logThen replace both occurrences:
- if len(job_log) > MAX_LOG_LENGTH: - truncated_length = len(job_log) - MAX_LOG_LENGTH - job_log = ( - f"[... truncated first {truncated_length} chars ...]\n\n" - + job_log[-MAX_LOG_LENGTH:] - ) + job_log = self._truncate_log(job_log)Also applies to: 361-367
250-254: Re-raised exceptions should chain withfromto preserve the original traceback.Lines 251 and 254 raise new
ValueErrorexceptions from withinexceptblocks without chaining. This loses the original traceback context.Proposed fix
except json.JSONDecodeError as e: - raise ValueError(f"Invalid JSON in response: {e}") + raise ValueError(f"Invalid JSON in response: {e}") from e except Exception as e: - raise ValueError(f"Error parsing response: {e}") + raise ValueError(f"Error parsing response: {e}") from e
204-248: JSON extraction via first{/ last}is fragile if the LLM response contains markdown code fences with nested JSON.
response_text.find("{")andrfind("}")will work for simple responses, but if the LLM wraps the JSON in a markdown code block (e.g.,```json { ... } ```followed by explanatory text containing}),rfindmay capture beyond the intended JSON. Since LLM output is unpredictable, consider first stripping markdown code fences before thefind/rfindapproach.
| req = urllib.request.Request(url, data=body, headers=headers, method=method) | ||
| try: | ||
| with urllib.request.urlopen(req) as resp: | ||
| return json.loads(resp.read().decode()) | ||
| except urllib.error.HTTPError as e: | ||
| error_body = "" | ||
| try: | ||
| error_body = e.read().decode() | ||
| except Exception: | ||
| pass | ||
| print(f"GitHub API error: {e.code} {e.reason} {error_body}", file=sys.stderr) | ||
| raise |
There was a problem hiding this comment.
Missing timeout on REST API calls — could hang indefinitely.
github_request uses urlopen without a timeout, unlike github_graphql which sets timeout=30. In CI, a hung network call would block the workflow until the runner's global timeout kills it.
Suggested fix
- with urllib.request.urlopen(req) as resp:
+ with urllib.request.urlopen(req, timeout=30) as resp:🧰 Tools
🪛 Ruff (0.15.0)
[error] 82-82: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
[error] 84-84: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
[error] 90-91: try-except-pass detected, consider logging the exception
(S110)
[warning] 90-90: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In @.github/scripts/lgtm_bot_diagnose.py around lines 82 - 93, The
github_request function calls urllib.request.urlopen without a timeout and can
hang; match github_graphql by passing a timeout (e.g., timeout=30) to
urllib.request.urlopen(req, timeout=30) or use a shared constant for the timeout
value, keeping the existing HTTPError handling intact (preserve the error_body
read/print/raise logic) so the request won't block indefinitely.
| def diagnose_reviews(pr_number: int, pr_author: str) -> tuple[list[str], list[str]]: | ||
| """Check review status.""" | ||
| reviews = github_request(f"/repos/{OWNER}/{REPO}/pulls/{pr_number}/reviews") | ||
|
|
||
| latest_by_user: dict[str, dict] = {} | ||
| for review in reviews: | ||
| user = review["user"]["login"] | ||
| if user == pr_author: | ||
| continue | ||
| if review["state"] not in ("APPROVED", "CHANGES_REQUESTED"): | ||
| continue | ||
| existing = latest_by_user.get(user) | ||
| if not existing or review["submitted_at"] > existing["submitted_at"]: | ||
| latest_by_user[user] = review | ||
|
|
||
| approvals = [u for u, r in latest_by_user.items() if r["state"] == "APPROVED"] | ||
| changes_requested = [ | ||
| u for u, r in latest_by_user.items() if r["state"] == "CHANGES_REQUESTED" | ||
| ] | ||
|
|
||
| return approvals, changes_requested |
There was a problem hiding this comment.
diagnose_reviews doesn't paginate — may miss reviews on busy PRs.
The GitHub API defaults to 30 results per page. PRs with many review iterations (common on large PRs) could have >30 review entries, causing the function to miss some approvals or change requests.
Suggested fix
def diagnose_reviews(pr_number: int, pr_author: str) -> tuple[list[str], list[str]]:
"""Check review status."""
- reviews = github_request(f"/repos/{OWNER}/{REPO}/pulls/{pr_number}/reviews")
+ reviews: list[dict] = []
+ page = 1
+ while True:
+ batch = github_request(
+ f"/repos/{OWNER}/{REPO}/pulls/{pr_number}/reviews"
+ f"?per_page=100&page={page}"
+ )
+ reviews.extend(batch)
+ if len(batch) < 100:
+ break
+ page += 1🤖 Prompt for AI Agents
In @.github/scripts/lgtm_bot_diagnose.py around lines 352 - 372, The
diagnose_reviews function currently calls github_request once and can miss
reviews because GitHub paginates results; modify diagnose_reviews to page
through the reviews endpoint (e.g., add page and per_page params to
github_request calls, using per_page=100 and incrementing page until an empty
page is returned) and accumulate all review objects into the reviews list before
the existing latest_by_user/approvals logic; keep using OWNER and REPO in the
endpoint string and reuse github_request to fetch each page.
| async function updateChecklist(prNumber, body) { | ||
| // Search from newest first — checklist comment is frequently updated | ||
| let existing = null; | ||
| let page = 1; | ||
| while (!existing) { | ||
| const { data: comments } = await github.rest.issues.listComments({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: prNumber, | ||
| per_page: 100, | ||
| page, | ||
| direction: 'desc', | ||
| }); | ||
| if (comments.length === 0) break; | ||
| existing = comments.find(c => c.body && c.body.includes(CHECKLIST_MARKER)); | ||
| page++; | ||
| } | ||
| if (existing) { | ||
| await github.rest.issues.updateComment({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| comment_id: existing.id, | ||
| body, | ||
| }); | ||
| } else { | ||
| await github.rest.issues.createComment({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: prNumber, | ||
| body, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GitHub REST API list issue comments direction parameter supported
💡 Result:
For List issue comments on a specific issue:
- Endpoint:
GET /repos/{owner}/{repo}/issues/{issue_number}/comments directionis not supported (there’s nosort/directionquery params for this endpoint). Comments are returned ordered by ascending ID; available query params aresince,per_page, andpage. [1]
If you need direction, that’s supported only on the repository-wide listing endpoint:
- Endpoint:
GET /repos/{owner}/{repo}/issues/comments - Supports
sort(createdorupdated) anddirection(asc/desc), butdirectionis ignored unlesssortis provided. [1]
Sources:
[1] GitHub Docs – REST API endpoints for issue comments (parameters for both endpoints) (docs.github.com)
Citations:
direction: 'desc' is not supported by the issue comments endpoint — comments will be fetched oldest-first instead.
The GitHub REST API for listing issue comments does not support a direction parameter on the issue-specific endpoint. The parameter is silently ignored and comments are returned in ascending order by ID. This contradicts the intent of Line 118's comment about searching "from newest first."
The pagination loop will still work correctly, but on active PRs it becomes inefficient by iterating through all comments from oldest to newest.
🤖 Prompt for AI Agents
In @.github/workflows/lgtm-bot.yml around lines 117 - 149, The issue:
updateChecklist currently passes the unsupported direction option and fetches
comments oldest-first, making the search inefficient. Fix it in updateChecklist
by removing the direction parameter and use octokit pagination to get all
comments then search from newest: call
github.paginate(github.rest.issues.listComments, { owner: context.repo.owner,
repo: context.repo.repo, issue_number: prNumber, per_page: 100 }) to retrieve
all comments, then find the most recent comment containing CHECKLIST_MARKER by
iterating the returned array from the end (or using .reverse().find) and assign
it to existing before updating/creating the comment as done now.
| const { data: reviews } = await github.rest.pulls.listReviews({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| pull_number: prNumber, | ||
| }); |
There was a problem hiding this comment.
Reviews are not paginated — PRs with 30+ reviews may lose data.
github.rest.pulls.listReviews defaults to 30 items per page. For high-activity PRs, later reviews (including the most recent approvals or change-requests) may be missed, causing the bot to show stale approval/changes-requested state or to apply/remove the label incorrectly. The same issue applies to the synchronize path at Lines 277-281.
Proposed fix — paginate reviews
- const { data: reviews } = await github.rest.pulls.listReviews({
- owner: context.repo.owner,
- repo: context.repo.repo,
- pull_number: prNumber,
- });
+ let reviews = [];
+ let reviewPage = 1;
+ while (true) {
+ const { data: batch } = await github.rest.pulls.listReviews({
+ owner: context.repo.owner,
+ repo: context.repo.repo,
+ pull_number: prNumber,
+ per_page: 100,
+ page: reviewPage,
+ });
+ reviews = reviews.concat(batch);
+ if (batch.length < 100) break;
+ reviewPage++;
+ }Apply the same pattern to the synchronize path (Lines 277-281).
📝 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.
| const { data: reviews } = await github.rest.pulls.listReviews({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| pull_number: prNumber, | |
| }); | |
| let reviews = []; | |
| let reviewPage = 1; | |
| while (true) { | |
| const { data: batch } = await github.rest.pulls.listReviews({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| pull_number: prNumber, | |
| per_page: 100, | |
| page: reviewPage, | |
| }); | |
| reviews = reviews.concat(batch); | |
| if (batch.length < 100) break; | |
| reviewPage++; | |
| } |
🤖 Prompt for AI Agents
In @.github/workflows/lgtm-bot.yml around lines 386 - 390, The call to
github.rest.pulls.listReviews is not paginated so PRs with >30 reviews can miss
reviews; replace the single-page call (const { data: reviews } = await
github.rest.pulls.listReviews({...})) with a paginated fetch using the GitHub
client's paginate helper (e.g., use
github.paginate(github.rest.pulls.listReviews, { owner: context.repo.owner,
repo: context.repo.repo, pull_number: prNumber }) to collect all review pages
into a single reviews array) and then use that aggregated reviews array for
approval/state logic; apply the identical pagination change to the synchronize
path that also calls github.rest.pulls.listReviews.
| env: | ||
| SLACK_WEBHOOK_URL: ${{ secrets.SLACK_LGTM_WEBHOOK_URL }} | ||
| PR_NUMBER: ${{ steps.lgtm.outputs.pr_number }} | ||
| PR_TITLE: ${{ steps.lgtm.outputs.pr_title }} | ||
| PR_URL: ${{ steps.lgtm.outputs.pr_url }} | ||
| PR_AUTHOR: ${{ steps.lgtm.outputs.pr_author }} | ||
| run: | | ||
| curl -sSf -X POST "$SLACK_WEBHOOK_URL" \ | ||
| -H 'Content-Type: application/json' \ | ||
| -d "$(jq -n \ | ||
| --arg pr_num "$PR_NUMBER" \ | ||
| --arg pr_title "$PR_TITLE" \ | ||
| --arg pr_url "$PR_URL" \ | ||
| --arg pr_author "$PR_AUTHOR" \ | ||
| '{ | ||
| text: ":white_check_mark: PR #\($pr_num) is ready to merge", | ||
| blocks: [ | ||
| { | ||
| type: "section", | ||
| text: { | ||
| type: "mrkdwn", | ||
| text: ":white_check_mark: *PR Ready to Merge*\n<\($pr_url)|#\($pr_num): \($pr_title)>\nAuthor: \($pr_author) | All CI checks passing, approved, DCO signed" | ||
| } | ||
| } | ||
| ] | ||
| }')" |
There was a problem hiding this comment.
Slack webhook URL is passed through environment and logged shell context — ensure curl errors don't leak the URL.
curl -sSf will print the URL in its error message if the request fails. Since SLACK_WEBHOOK_URL is in the URL argument, a failed curl could leak the secret webhook URL into workflow logs.
Proposed fix — suppress URL from error output
- curl -sSf -X POST "$SLACK_WEBHOOK_URL" \
+ curl -sSf --fail-with-body -X POST "$SLACK_WEBHOOK_URL" \
+ --no-progress-meter \
-H 'Content-Type: application/json' \
- -d "$(jq -n \
+ -d "$(jq -n \Alternatively, redirect stderr to suppress the URL:
- curl -sSf -X POST "$SLACK_WEBHOOK_URL" \
+ curl -sS -X POST "$SLACK_WEBHOOK_URL" \
-H 'Content-Type: application/json' \
- -d "$(jq -n \
+ -d "$(jq -n \
...
- }')"
+ }')" 2>&1 | grep -v "$SLACK_WEBHOOK_URL" || true🤖 Prompt for AI Agents
In @.github/workflows/lgtm-bot.yml around lines 488 - 513, The curl invocation
using SLACK_WEBHOOK_URL can leak the secret in curl's error output; update the
run step that calls curl (the command referencing SLACK_WEBHOOK_URL and using
-sSf) so stderr is suppressed or removed from logs—e.g., remove -S or redirect
curl's stderr to /dev/null (append 2>/dev/null) or otherwise avoid printing
curl's error details; target the curl command in the workflow where
SLACK_WEBHOOK_URL, PR_NUMBER, PR_TITLE, PR_URL, and PR_AUTHOR are used and
ensure the secret URL is not emitted on failure.
| primary_category=error_data["primary_category"], | ||
| confidence_score=error_data["confidence_score"], | ||
| root_cause_summary=error_data["root_cause_summary"], | ||
| classification_method="full_log_analysis", | ||
| model_version=result.get("model_version"), | ||
| classified_at=datetime.now(timezone.utc).isoformat(), | ||
| is_duplicate=False, | ||
| occurrence_count=1, | ||
| first_seen=datetime.now(timezone.utc).isoformat(), | ||
| last_seen=datetime.now(timezone.utc).isoformat(), | ||
| prompt_tokens=result.get("prompt_tokens", 0), | ||
| completion_tokens=result.get("completion_tokens", 0), | ||
| cached_tokens=result.get("cached_tokens", 0), | ||
| timestamp=datetime.now(timezone.utc).isoformat(), |
There was a problem hiding this comment.
Token usage is duplicated across all classifications from the same API call — aggregate totals will be inflated.
Lines 208-210 assign the total prompt_tokens, completion_tokens, and cached_tokens from the single LLM API call to every ErrorClassification produced from that call. When classify_workflow_errors.py (Lines 506-508) sums c.prompt_tokens across all classifications, the reported totals are multiplied by the number of errors found per job.
For example, one API call costing 5000 prompt tokens that finds 3 errors will report 15000 prompt tokens total.
Proposed fix — distribute tokens across errors or store only on the first
+ num_errors = len(result.get("errors_found", []))
# Convert results to ErrorClassification objects
classifications = []
- for error_data in result.get("errors_found", []):
+ for i, error_data in enumerate(result.get("errors_found", [])):
error_text = error_data.get("log_excerpt", "")
classification = ErrorClassification(
...
- prompt_tokens=result.get("prompt_tokens", 0),
- completion_tokens=result.get("completion_tokens", 0),
- cached_tokens=result.get("cached_tokens", 0),
+ # Attribute token usage only to the first error to avoid double-counting
+ prompt_tokens=result.get("prompt_tokens", 0) if i == 0 else 0,
+ completion_tokens=result.get("completion_tokens", 0) if i == 0 else 0,
+ cached_tokens=result.get("cached_tokens", 0) if i == 0 else 0,
...
)🤖 Prompt for AI Agents
In `@error_classification/classifier.py` around lines 198 - 211, The token fields
(prompt_tokens, completion_tokens, cached_tokens) are currently copied from
result to every ErrorClassification in classifier.py, inflating aggregated
totals in classify_workflow_errors.py; fix by computing the total tokens once
from result (result.get("prompt_tokens",0), etc.), dividing them evenly across
the number of classifications you are creating (num_errors =
len(classifications) or similar), assign tokens_per = total // num_errors for
each, add any remainder to the first classification (or the first few) so totals
sum to the original, and then set prompt_tokens/completion_tokens/cached_tokens
on each ErrorClassification accordingly (use the existing error_data/result
variables and the ErrorClassification construction block to apply the per-class
values).
| def wait_if_needed(self): | ||
| """Wait if rate limit would be exceeded.""" | ||
| with self._lock: | ||
| now = time.time() | ||
|
|
||
| # Remove requests older than 1 minute | ||
| self.requests = [ | ||
| req_time for req_time in self.requests if now - req_time < 60 | ||
| ] | ||
|
|
||
| # If at limit, wait | ||
| if len(self.requests) >= self.max_rpm: | ||
| oldest_request = self.requests[0] | ||
| wait_time = 60 - (now - oldest_request) | ||
| if wait_time > 0: | ||
| print(f"⏳ Rate limit reached, waiting {wait_time:.1f}s") | ||
| time.sleep(wait_time) | ||
|
|
||
| self.requests.append(now) |
There was a problem hiding this comment.
RateLimiter.wait_if_needed records a stale timestamp after sleeping.
now is captured at Line 41 before the potential sleep at Line 54. After sleeping (potentially tens of seconds), Line 56 appends the pre-sleep now value. This makes the request appear older than it actually is, causing subsequent calls to expire it from the window earlier and potentially allowing the rate limit to be slightly exceeded.
Proposed fix
if len(self.requests) >= self.max_rpm:
oldest_request = self.requests[0]
wait_time = 60 - (now - oldest_request)
if wait_time > 0:
print(f"⏳ Rate limit reached, waiting {wait_time:.1f}s")
time.sleep(wait_time)
+ # Re-capture time after sleep
+ now = time.time()
+ # Clean up again after sleep
+ self.requests = [
+ req_time for req_time in self.requests if now - req_time < 60
+ ]
self.requests.append(now)🤖 Prompt for AI Agents
In `@error_classification/claude_client.py` around lines 38 - 56, The
wait_if_needed method in RateLimiter captures now before possibly sleeping, then
appends that stale timestamp to self.requests; change the logic so that after
any sleep you record the current time (recompute now or use a new timestamp) and
append that fresh timestamp to self.requests (keeping the operation inside the
same self._lock block) so the request reflects the actual time it occurred and
the rate window is correct.
| return cls( | ||
| api_key=api_key, | ||
| model=model, | ||
| api_format=os.getenv("API_FORMAT", "openai"), # default openai for NVIDIA | ||
| api_base_url=os.getenv( | ||
| "API_BASE_URL", "https://inference-api.nvidia.com/v1" | ||
| ), # default NVIDIA | ||
| max_rpm=int(os.getenv("MAX_RPM", "50")), | ||
| ) |
There was a problem hiding this comment.
int(os.getenv("MAX_RPM", "50")) will crash with an unhelpful traceback if the env var is non-numeric.
If someone sets MAX_RPM=fast, this raises a bare ValueError from int(). A small guard or a friendlier message would help operators debug misconfiguration.
Proposed fix
- max_rpm=int(os.getenv("MAX_RPM", "50")),
+ max_rpm=cls._parse_int_env("MAX_RPM", 50),Add a helper:
`@staticmethod`
def _parse_int_env(name: str, default: int) -> int:
raw = os.getenv(name, "").strip()
if not raw:
return default
try:
return int(raw)
except ValueError:
raise ValueError(f"{name} must be an integer, got: {raw!r}")🤖 Prompt for AI Agents
In `@error_classification/config.py` around lines 59 - 67, The MAX_RPM environment
parsing currently uses int(os.getenv("MAX_RPM", "50")) which raises an unhelpful
ValueError if the env var is non-numeric; change Config construction to use a
safe parser (implement a static/helper method named _parse_int_env(name: str,
default: int) that reads os.getenv(name), returns default on empty, and raises a
clear ValueError like "{name} must be an integer, got: {raw!r}" on invalid
input) and call it instead of int(...) when setting max_rpm in the cls(...)
return.
| # Post or update comment via GitHub API | ||
| try: | ||
| if existing_comment_id: | ||
| # Update existing comment | ||
| url = f"https://api.github.com/repos/{self.repo}/issues/comments/{existing_comment_id}" | ||
| response = requests.patch( | ||
| url, headers=self.headers, json={"body": markdown} | ||
| ) | ||
|
|
||
| if response.status_code == 200: | ||
| print( | ||
| f"✅ Updated existing CI-errors comment {existing_comment_id} on PR #{pr_number}" | ||
| ) | ||
| return True | ||
| else: | ||
| print(f"⚠️ Failed to update comment: HTTP {response.status_code}") | ||
| print(f" Response: {response.text[:200]}") | ||
| return False | ||
| else: | ||
| # Create new comment | ||
| url = f"https://api.github.com/repos/{self.repo}/issues/{pr_number}/comments" | ||
| response = requests.post( | ||
| url, headers=self.headers, json={"body": markdown} | ||
| ) | ||
|
|
||
| if response.status_code == 201: | ||
| print(f"✅ Created PR comment on PR #{pr_number}") | ||
| return True | ||
| else: | ||
| print( | ||
| f"⚠️ Failed to create PR comment: HTTP {response.status_code}" | ||
| ) | ||
| print(f" Response: {response.text[:200]}") | ||
| return False | ||
|
|
||
| except Exception as e: | ||
| print(f"⚠️ Error creating PR comment: {e}") | ||
| return False |
There was a problem hiding this comment.
Missing timeout on requests.patch and requests.post calls — can hang indefinitely.
Lines 175 and 191 make HTTP requests without a timeout parameter. If the GitHub API stalls, the workflow step will hang until the 5-minute job timeout. Other request calls in this PR (e.g., _find_pr_by_commit at Line 324) correctly set timeout=10. Static analysis (Ruff S113) flags these as well.
Proposed fix
response = requests.patch(
- url, headers=self.headers, json={"body": markdown}
+ url, headers=self.headers, json={"body": markdown}, timeout=30
) response = requests.post(
- url, headers=self.headers, json={"body": markdown}
+ url, headers=self.headers, json={"body": markdown}, timeout=30
)Also add timeout to _find_existing_comment at Line 221:
- response = requests.get(url, headers=self.headers)
+ response = requests.get(url, headers=self.headers, timeout=10)🧰 Tools
🪛 Ruff (0.15.0)
[error] 175-175: Probable use of requests call without timeout
(S113)
[error] 191-191: Probable use of requests call without timeout
(S113)
[warning] 205-205: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@error_classification/pr_commentator.py` around lines 170 - 207, The
requests.patch and requests.post calls that post/update GitHub comments (using
existing_comment_id, pr_number in pr_commentator) lack a timeout and can hang;
add a timeout parameter (e.g., timeout=10) to both requests.patch(...) and
requests.post(...) calls, and also ensure the helper _find_existing_comment
(referenced elsewhere in this module) is updated to pass timeout to any requests
it makes so all HTTP calls in this class use a consistent timeout value.
| def _find_pr_by_commit(self, commit_sha: str) -> Optional[int]: | ||
| """Find PR number associated with a commit using GitHub API.""" | ||
| try: | ||
| url = f"https://api.github.com/repos/{self.repo}/commits/{commit_sha}/pulls" | ||
| print(f" API request: {url}") | ||
| response = requests.get(url, headers=self.headers, timeout=10) | ||
| print(f" API response status: {response.status_code}") | ||
|
|
||
| if response.status_code == 200: | ||
| pulls = response.json() | ||
| print(f" API returned {len(pulls)} PR(s)") | ||
| if pulls and len(pulls) > 0: | ||
| pr_num = pulls[0].get("number") | ||
| print(f" First PR: #{pr_num}") | ||
| return pr_num | ||
| else: | ||
| print(f" API error: {response.text[:200]}") | ||
| except Exception as e: | ||
| print(f"⚠️ Error finding PR by commit: {e}") | ||
| traceback.print_exc() | ||
|
|
||
| return None |
There was a problem hiding this comment.
_find_pr_by_commit returns raw value from JSON without type coercion — may return None.
Line 331 uses pulls[0].get("number") which can return None if the number key is absent. The caller at Line 306 (pr_num = self._find_pr_by_commit(head_sha)) then checks if pr_num: which correctly gates a None, but the return type is Optional[int] while the actual return could be a non-int JSON value. Compare with Line 298 which wraps in int().
Proposed fix
if pulls and len(pulls) > 0:
pr_num = pulls[0].get("number")
- print(f" First PR: #{pr_num}")
- return pr_num
+ if pr_num is not None:
+ print(f" First PR: #{pr_num}")
+ return int(pr_num)🧰 Tools
🪛 Ruff (0.15.0)
[warning] 336-336: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@error_classification/pr_commentator.py` around lines 319 - 340, The
_find_pr_by_commit function returns the raw JSON value for
pulls[0].get("number") which may be None or non-int; update _find_pr_by_commit
to coerce and validate the PR number before returning: extract the value from
pulls[0].get("number"), check it's not None and is an int-like value (e.g., str
of digits or int), convert with int(...) and return that, otherwise log/ignore
and return None; ensure the function signature Optional[int] is honored and keep
the existing exception handling in _find_pr_by_commit.
| # --- GitHub API helpers --- | ||
|
|
||
|
|
||
| def github_request( |
There was a problem hiding this comment.
Why not just use github CLI for all of these API requests? All of these scripts are completely overcomplicated.
| headers = { | ||
| "Authorization": f"Bearer {GITHUB_TOKEN}", | ||
| "Accept": "application/vnd.github+json", | ||
| "X-GitHub-Api-Version": "2022-11-28", |
There was a problem hiding this comment.
Why are we specifying the API version? Why not use default so that we get updates if they happen?
|
|
||
|
|
||
| def diagnose_ci(head_sha: str) -> tuple[dict, list[str]]: | ||
| """Check CI status for all required checks.""" |
There was a problem hiding this comment.
Why is this better than a user just scrolling down and looking at the status checks?
|
|
||
|
|
||
| def fetch_failed_logs(check_run_id: int) -> str: | ||
| """Fetch annotations for a failed check run.""" |
There was a problem hiding this comment.
Why is this better than a user just looking at any given workflows errors? Annotations aren't 100% accurate.
| def call_llm(failing_checks: list[str], logs: str) -> str | None: | ||
| """Call NVIDIA Inference API for CI failure diagnosis.""" | ||
| if not NVIDIA_API_KEY: | ||
| return None |
There was a problem hiding this comment.
Why are we returning None of the API key is absent? Why wouldn't this be an error?
| id: lgtm | ||
| uses: actions/github-script@v7 | ||
| with: | ||
| script: | |
There was a problem hiding this comment.
These scripts should all be outside the workflow. Defining functions inside workflows is bad practice and reduces re-usability.
| } | ||
| } | ||
|
|
||
| function buildChecklist(checks, reviewStatus, bpState, headSha, ciTriggered) { |
There was a problem hiding this comment.
Once again this information can already be gleaned from the github UI itself, there is no reason to reinvent the wheel for everything that Github already provides.
| pull_request_review: | ||
| types: [submitted, dismissed] | ||
| # Remove label when new commits are pushed (uses _target for fork write access) | ||
| pull_request_target: |
There was a problem hiding this comment.
This is a security vulnerability. Don't use pull_request_target.
| if (context.eventName === 'workflow_run') { | ||
| const prs = context.payload.workflow_run.pull_requests; | ||
| if (prs && prs.length > 0) { | ||
| prNumber = prs[0].number; | ||
| } else { | ||
| // Fallback for fork PRs where pull_requests array may be empty | ||
| const headBranch = context.payload.workflow_run.head_branch; | ||
| const headRepoOwner = context.payload.workflow_run.head_repository?.owner?.login; | ||
| if (headRepoOwner && headBranch) { | ||
| const { data: pulls } = await github.rest.pulls.list({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| state: 'open', | ||
| head: `${headRepoOwner}:${headBranch}`, | ||
| per_page: 1, | ||
| }); | ||
| if (pulls.length > 0) { | ||
| prNumber = pulls[0].number; | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| // pull_request_target and pull_request_review both include PR | ||
| prNumber = context.payload.pull_request?.number; | ||
| } |
There was a problem hiding this comment.
So we are determining the pull request number with 30 lines of complicated code instead of just using {{ github.event.pull_request.number }}?
| return; | ||
| } | ||
|
|
||
| // --- Check required CI statuses --- |
There was a problem hiding this comment.
More redundant code with the python script.
|
/lgtm-bot diagnose |
|
I just spent 30 minutes reviewing over 1000 lines of code and now the PR is 4000 lines of code? |
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In @.github/LGTM_BOT.md:
- Around line 155-162: The permissions doc currently lists "checks: read" but
the workflows' classify-errors job requires "checks: write"; update the
permissions example in .github/LGTM_BOT.md to use checks: write to match actual
workflows. Locate the permissions block and change checks from read to write,
and ensure the comment still explains that write is needed for the
classify-errors job (referencing the classify-errors job name used in the
workflows).
- Around line 136-141: Remove the duplicated "### Optional Variables" table
block that repeats the LGTM_BOT_REVIEWERS_PER_TEAM entry; specifically, delete
the second occurrence of the heading "### Optional Variables" and its table row
that lists `LGTM_BOT_REVIEWERS_PER_TEAM` so only the original documentation of
that variable remains (keep the first table where the variable is defined).
In @.github/scripts/classify_workflow_errors.py:
- Around line 505-508: The totals are inflated because the list classifications
contains multiple ErrorClassification entries per single API call; change the
aggregation to sum tokens once per unique API call instead of per
classification. Build a map keyed by a stable identifier on ErrorClassification
(e.g., an api_call_id or request_id attribute; if not available, use a fallback
unique tuple like (prompt_text, completion_text, timestamp)) and then compute
total_prompt_tokens, total_completion_tokens, and total_cached_tokens by summing
the values from each unique key rather than summing across classifications
directly; update the code around the variables total_prompt_tokens,
total_completion_tokens, total_cached_tokens to use this deduplicated
collection.
- Around line 96-113: The code may call find_latest_failed_pr_run with
github_token == None causing "Authorization: Bearer None" requests; before
invoking find_latest_failed_pr_run (the block using github_event_name and
run_id), ensure github_token is present or return early — e.g., check
github_token (and repo) alongside run_id or move the existing all([github_token,
repo, run_id]) guard above the workflow_dispatch branch; alternatively, add a
defensive check inside find_latest_failed_pr_run to bail when github_token is
falsy before building the Authorization header.
In @.github/workflows/lgtm-bot-stale.yml:
- Around line 96-108: The comments listing currently uses an unsupported
direction parameter and requests the 10 oldest comments, causing the
recent-nudge dedup (recentNudge using NUDGE_MARKER, per_page, MS_PER_HOUR,
NUDGE_COOLDOWN_HOURS) to miss recent nudges on busy PRs; update the logic to
fetch a larger page (e.g., per_page: 100) and search from the end (or take the
last N entries) to find NUDGE_MARKER within the cooldown window, and apply the
same change to the merge reminder dedup check (the equivalent
recentMergeReminder / MERGE_REMINDER_MARKER logic) so you reliably examine the
most recent comments instead of the oldest.
In `@error_classification/claude_client.py`:
- Around line 72-80: The current constructor branch that handles
config.api_format == "openai" silently defaults self.api_base_url to
"https://api.openai.com/v1" which can send the wrong API key; instead, update
the initialization in claude_client.py so that when config.api_format ==
"openai" and config.api_base_url is None you raise a clear exception (e.g.,
ValueError) telling the caller to provide api_base_url (or explicitly choose the
correct NVIDIA default) rather than silently falling back; change the code
around the block that sets self.client and self.api_base_url (the if
config.api_format == "openai" branch) to validate config.api_base_url and raise
an error with a descriptive message if it's missing.
- Around line 383-399: The code currently assumes the OpenAI response has
choices[0].message.content and will throw KeyError/IndexError; wrap the
extraction of content (after calling _openai_request_with_retry and parsing
response.json()) in a defensive check or try/except: verify data is a dict, that
"choices" exists and is a non-empty list and that choices[0] has "message" and
"content" keys, or catch exceptions and raise/log a clear error including the
raw response body; then pass the validated content into _parse_full_log_response
and continue to attach usage/model fields as before. Ensure you reference the
response coming from _openai_request_with_retry, the local variable data, and
the call to _parse_full_log_response when implementing the guard.
In `@error_classification/pr_commentator.py`:
- Around line 294-300: The code calls int(pull_requests[0].get("number")) which
will raise TypeError if "number" is missing; update the extraction to guard
against None by first retrieving the raw value (e.g., num =
pull_requests[0].get("number")), check if num is not None (and optionally
isinstance/isdigit if strings possible), then set pr_num = int(num); if num is
None skip/continue or handle the missing case. Apply the same guard to the other
place that extracts a PR number without checking for None so both branches
consistently validate before calling int().
In `@error_classification/requirements.txt`:
- Around line 5-6: The requirements file uses open-ended version specifiers for
anthropic and requests (anthropic>=0.40.0 and requests>=2.28.0) which violates
the repo policy; update these to bounded ranges or exact pins (for example
change anthropic to a bounded spec like anthropic>=0.40.0,<0.80.0 or an exact
pin, and change requests to a bounded spec like requests>=2.28.0,<3.0.0 or an
exact pin) so the dependency lines referencing "anthropic" and "requests"
conform to the repository's version pinning policy.
🧹 Nitpick comments (15)
CONTRIBUTING.md (1)
239-245: Minor redundancy in the Error Classification section."On-demand" (line 244) and "Via diagnose" (line 245) both describe the same thing: commenting
/diagnosetriggers classification. Consider merging these two bullet points.Suggested simplification
- **Automatic** – The error classifier runs as the final step in CI pipelines (`pre-merge`, `pr`, `ci-test-suite`, `container-validation-dynamo`, `copyright-checks`, `build-frontend-image`). When failures are detected, it posts a PR comment categorizing each as **infrastructure error** (network/runner issues) or **code error** (build/test/compilation failures). -- **On-demand** – Comment `/diagnose` on any PR to run classification along with the full merge-readiness diagnostic. -- **Via diagnose** – The `/diagnose` command automatically runs the error classifier when CI failures are present. +- **On-demand** – Comment `/diagnose` on any PR to run the error classifier (when CI failures are present) along with the full merge-readiness diagnostic..github/scripts/lgtm_bot_diagnose.py (2)
294-325: Subprocess invocation uses a relative path — depends oncwdbeing set correctly.
run_error_classifierinvokespython3 .github/scripts/classify_workflow_errors.pywithcwd=repo_root(whererepo_root = os.getcwd()frommain()). This works in GitHub Actions where the working directory is the repo checkout, but is fragile if the script is ever invoked from a different directory.Consider making the path resolution explicit:
Suggested fix
+ script_path = os.path.join(repo_root, ".github", "scripts", "classify_workflow_errors.py") result = subprocess.run( - ["python3", ".github/scripts/classify_workflow_errors.py"], + ["python3", script_path], capture_output=True, text=True, timeout=300, - cwd=repo_root, env=env, )
41-44: Missing env vars crash on startup with an unhelpfulKeyError.Lines 41 and 43 use
os.environ["GITHUB_TOKEN"]andint(os.environ["PR_NUMBER"]), which raiseKeyError/ValueErrorrespectively if not set. Consider explicit validation with a clear error message, especially since this is a CI entry point where misconfiguration is common.Suggested improvement
-GITHUB_TOKEN = os.environ["GITHUB_TOKEN"] -GITHUB_REPOSITORY = os.environ.get("GITHUB_REPOSITORY", "ai-dynamo/dynamo") -PR_NUMBER = int(os.environ["PR_NUMBER"]) +GITHUB_TOKEN = os.environ.get("GITHUB_TOKEN") +PR_NUMBER_STR = os.environ.get("PR_NUMBER") +GITHUB_REPOSITORY = os.environ.get("GITHUB_REPOSITORY", "ai-dynamo/dynamo") + +if not GITHUB_TOKEN: + print("Error: GITHUB_TOKEN environment variable is required", file=sys.stderr) + sys.exit(1) +if not PR_NUMBER_STR: + print("Error: PR_NUMBER environment variable is required", file=sys.stderr) + sys.exit(1) + +PR_NUMBER = int(PR_NUMBER_STR).github/workflows/lgtm-bot-diagnose.yml (2)
49-55: Authorization gate allows first-time contributors — consider tightening for LLM cost control.The
author_association != 'NONE'check (line 55) permitsFIRST_TIMERandFIRST_TIME_CONTRIBUTORto trigger/diagnose, which invokes the NVIDIA Inference API if the secret is configured. On a public repo, this could be abused to run up API costs.If LLM API usage is a concern, consider restricting to more established associations:
Suggested tightening
) && - github.event.comment.author_association != 'NONE' + ( + github.event.comment.author_association == 'MEMBER' || + github.event.comment.author_association == 'OWNER' || + github.event.comment.author_association == 'COLLABORATOR' || + github.event.comment.author_association == 'CONTRIBUTOR' + )
67-72: Virtual env created but not persisted — works but fragile across steps.The venv at
/tmp/error-classification-venvis created in one step and activated in the next (line 84). This works because both steps share the same runner filesystem, but the pattern is slightly unusual. A common alternative is to usepip install --userorpip installdirectly (sincesetup-pythonalready provides an isolated environment). Minor nit — current approach is functional..github/workflows/lgtm-bot-pr-open.yml (3)
91-98: Glob matcher doesn't handle character classes ([...]) from CODEOWNERS.GitHub CODEOWNERS supports
[...]character class patterns (e.g.,*.[ch]). The regex escaping on line 93 escapes[and](via[.+^${}()|[\]\\]), which breaks character-class patterns. If yourCODEOWNERSfile doesn't use character classes, this is fine for now — but worth noting as a gap.
191-194: Biased shuffle for reviewer selection.
sort(() => Math.random() - 0.5)is a well-known biased shuffle — some orderings are more likely than others. For reviewer selection the impact is negligible, but if fairness across team members matters, a Fisher-Yates shuffle is trivial:♻️ Optional: Fisher-Yates shuffle
function pickRandom(arr, n) { - const shuffled = [...arr].sort(() => Math.random() - 0.5); + const shuffled = [...arr]; + for (let i = shuffled.length - 1; i > 0; i--) { + const j = Math.floor(Math.random() * (i + 1)); + [shuffled[i], shuffled[j]] = [shuffled[j], shuffled[i]]; + } return shuffled.slice(0, n); }
356-368: Comment search direction is suboptimal forsynchronizeevents.With
direction: 'desc', the newest comments come first, but the CODEOWNERS marker comment was created at PR open time. Onsynchronizeevents, you'll paginate through every page before finding it. Consider searchingasc(the default) so the original bot comment is found quickly, or switching to the GraphQL search to avoid pagination altogether..github/workflows/classify-workflow-errors-reusable.yml (1)
47-52:|| truemasksapt-getfailures, but the subsequentpython3 -m venvwould catch it.The
|| trueon Line 49 swallows failures from bothapt-get updateand thepython3-venvinstall. Sinceubuntu-latestrunners already havepython3-venvavailable (andactions/setup-pythonon Line 43 installs the requested version), this is unlikely to be an issue in practice. Just noting that if thevenvmodule were genuinely missing, the failure would surface on Line 50 instead of Line 49.error_classification/config.py (1)
45-51: Consider stripping theapi_keyvalue, consistent withmodelhandling.Lines 53–57 explicitly
.strip()env vars before theorchain to treat whitespace-only values as unset. Theapi_keyon Line 45 does not strip, so a whitespace-only value (e.g.,NVIDIA_INFERENCE_API_KEY=" ") would pass theif not api_keyguard and propagate an invalid key to the API client.Proposed fix
- api_key = os.getenv("NVIDIA_INFERENCE_API_KEY") or os.getenv( - "ANTHROPIC_API_KEY" - ) + api_key = ( + os.getenv("NVIDIA_INFERENCE_API_KEY", "").strip() + or os.getenv("ANTHROPIC_API_KEY", "").strip() + )error_classification/classifier.py (1)
203-211: Multipledatetime.now()calls produce subtly different timestamps for the same classification.
classified_at,first_seen,last_seen, andtimestampare each computed independently. Capture a single timestamp before the loop and reuse it for consistency.Proposed fix
+ now = datetime.now(timezone.utc).isoformat() classifications = [] for error_data in result.get("errors_found", []): ... classification_method="full_log_analysis", model_version=result.get("model_version"), - classified_at=datetime.now(timezone.utc).isoformat(), + classified_at=now, is_duplicate=False, occurrence_count=1, - first_seen=datetime.now(timezone.utc).isoformat(), - last_seen=datetime.now(timezone.utc).isoformat(), + first_seen=now, + last_seen=now, prompt_tokens=result.get("prompt_tokens", 0), completion_tokens=result.get("completion_tokens", 0), cached_tokens=result.get("cached_tokens", 0), - timestamp=datetime.now(timezone.utc).isoformat(), + timestamp=now, ).github/scripts/classify_workflow_errors.py (1)
142-172:github_tokenandrepoparameters are unused.
fetch_job_stepsreads steps directly from thejobdict and never usesgithub_tokenorrepo. If the signature is kept for interface consistency withfetch_job_logs, consider prefixing with_to signal intent.error_classification/pr_commentator.py (1)
342-411:error_contextsparameter is accepted but never used in_build_summary_markdown_body.The parameter is passed through from
build_comment_markdown(Lines 114-115, 118-119) but the method body never references it. Either wire it into the output or drop it from the signature to avoid confusion.error_classification/claude_client.py (2)
109-126: Log truncation logic is duplicated between the Anthropic and OpenAI paths.Lines 117-123 and 362-367 contain identical truncation code. Consider extracting a shared helper to reduce duplication and ensure both paths stay in sync.
Proposed refactor
+ def _truncate_log(self, job_log: str) -> str: + """Truncate log to MAX_LOG_LENGTH, keeping the tail (most relevant for failures).""" + if len(job_log) > MAX_LOG_LENGTH: + truncated_length = len(job_log) - MAX_LOG_LENGTH + return ( + f"[... truncated first {truncated_length} chars ...]\n\n" + + job_log[-MAX_LOG_LENGTH:] + ) + return job_logThen replace both occurrences with
job_log = self._truncate_log(job_log).Also applies to: 344-367
248-254: Re-raised exceptions lose the original traceback.Lines 251 and 254 wrap and re-raise exceptions without chaining (
from e), which drops the original stack trace and makes debugging harder.Proposed fix
except json.JSONDecodeError as e: - raise ValueError(f"Invalid JSON in response: {e}") + raise ValueError(f"Invalid JSON in response: {e}") from e except Exception as e: - raise ValueError(f"Error parsing response: {e}") + raise ValueError(f"Error parsing response: {e}") from e
| ### Optional Variables | ||
|
|
||
| | Variable | Default | Purpose | | ||
| |----------|---------|---------| | ||
| | `LGTM_BOT_REVIEWERS_PER_TEAM` | `1` | Number of individual reviewers to pick per team/roster key | | ||
|
|
There was a problem hiding this comment.
Duplicate "Optional Variables" section.
The LGTM_BOT_REVIEWERS_PER_TEAM variable is already documented in the table at Line 86. This section (lines 136-141) repeats the same information under a second "### Optional Variables" heading.
Proposed fix — remove the duplicate section
-### Optional Variables
-
-| Variable | Default | Purpose |
-|----------|---------|---------|
-| `LGTM_BOT_REVIEWERS_PER_TEAM` | `1` | Number of individual reviewers to pick per team/roster key |
-
## Error Categories📝 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.
| ### Optional Variables | |
| | Variable | Default | Purpose | | |
| |----------|---------|---------| | |
| | `LGTM_BOT_REVIEWERS_PER_TEAM` | `1` | Number of individual reviewers to pick per team/roster key | | |
| ## Error Categories |
🤖 Prompt for AI Agents
In @.github/LGTM_BOT.md around lines 136 - 141, Remove the duplicated "###
Optional Variables" table block that repeats the LGTM_BOT_REVIEWERS_PER_TEAM
entry; specifically, delete the second occurrence of the heading "### Optional
Variables" and its table row that lists `LGTM_BOT_REVIEWERS_PER_TEAM` so only
the original documentation of that variable remains (keep the first table where
the variable is defined).
| ```yaml | ||
| permissions: | ||
| pull-requests: write # Post/update diagnostic comments | ||
| checks: read # Read CI check results | ||
| statuses: read # Read commit status checks | ||
| contents: read # Read CODEOWNERS, repo files | ||
| actions: read # List workflow runs | ||
| ``` |
There was a problem hiding this comment.
Permissions doc says checks: read but workflows use checks: write.
All classify-errors job definitions across the consumed workflows declare checks: write (e.g., ci-test-suite.yml Line 1007, pre-merge.yml Line 169, pr.yaml Line 524). The documentation should reflect the actual requirement.
Proposed fix
permissions:
pull-requests: write # Post/update diagnostic comments
- checks: read # Read CI check results
+ checks: write # Read CI check results, write check annotations
statuses: read # Read commit status checks
contents: read # Read CODEOWNERS, repo files
actions: read # List workflow runs🤖 Prompt for AI Agents
In @.github/LGTM_BOT.md around lines 155 - 162, The permissions doc currently
lists "checks: read" but the workflows' classify-errors job requires "checks:
write"; update the permissions example in .github/LGTM_BOT.md to use checks:
write to match actual workflows. Locate the permissions block and change checks
from read to write, and ensure the comment still explains that write is needed
for the classify-errors job (referencing the classify-errors job name used in
the workflows).
| if not run_id: | ||
| # Fallback to current run ID (for workflow_run trigger) | ||
| run_id = os.getenv("GITHUB_RUN_ID") | ||
|
|
||
| # If still no run_id and we're in workflow_dispatch mode, find latest failed PR run | ||
| if not run_id: | ||
| github_event_name = os.getenv("GITHUB_EVENT_NAME") | ||
| if github_event_name == "workflow_dispatch": | ||
| print("ℹ️ No run_id specified, searching for latest failed PR run...") | ||
| run_id = find_latest_failed_pr_run(github_token, repo) | ||
|
|
||
| if not all([github_token, repo, run_id]): | ||
| print("❌ Missing required GitHub environment variables") | ||
| print(f" GITHUB_TOKEN: {'set' if github_token else 'missing'}") | ||
| print(f" GITHUB_REPOSITORY: {repo or 'missing'}") | ||
| print(f" WORKFLOW_RUN_ID: {os.getenv('WORKFLOW_RUN_ID') or 'not set'}") | ||
| print(f" GITHUB_RUN_ID: {os.getenv('GITHUB_RUN_ID') or 'not set'}") | ||
| return [] |
There was a problem hiding this comment.
github_token may be None when passed to find_latest_failed_pr_run.
If GITHUB_TOKEN is not set, github_token is None at Line 90, and it flows into find_latest_failed_pr_run at Line 105 where it's interpolated into the Authorization header as "Bearer None", producing a confusing 401 before falling back to the guard at Line 107. Consider checking github_token earlier or guarding before the call.
Proposed fix
if not run_id:
github_event_name = os.getenv("GITHUB_EVENT_NAME")
- if github_event_name == "workflow_dispatch":
+ if github_event_name == "workflow_dispatch" and github_token and repo:
print("ℹ️ No run_id specified, searching for latest failed PR run...")
run_id = find_latest_failed_pr_run(github_token, repo)📝 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.
| if not run_id: | |
| # Fallback to current run ID (for workflow_run trigger) | |
| run_id = os.getenv("GITHUB_RUN_ID") | |
| # If still no run_id and we're in workflow_dispatch mode, find latest failed PR run | |
| if not run_id: | |
| github_event_name = os.getenv("GITHUB_EVENT_NAME") | |
| if github_event_name == "workflow_dispatch": | |
| print("ℹ️ No run_id specified, searching for latest failed PR run...") | |
| run_id = find_latest_failed_pr_run(github_token, repo) | |
| if not all([github_token, repo, run_id]): | |
| print("❌ Missing required GitHub environment variables") | |
| print(f" GITHUB_TOKEN: {'set' if github_token else 'missing'}") | |
| print(f" GITHUB_REPOSITORY: {repo or 'missing'}") | |
| print(f" WORKFLOW_RUN_ID: {os.getenv('WORKFLOW_RUN_ID') or 'not set'}") | |
| print(f" GITHUB_RUN_ID: {os.getenv('GITHUB_RUN_ID') or 'not set'}") | |
| return [] | |
| if not run_id: | |
| # Fallback to current run ID (for workflow_run trigger) | |
| run_id = os.getenv("GITHUB_RUN_ID") | |
| # If still no run_id and we're in workflow_dispatch mode, find latest failed PR run | |
| if not run_id: | |
| github_event_name = os.getenv("GITHUB_EVENT_NAME") | |
| if github_event_name == "workflow_dispatch" and github_token and repo: | |
| print("ℹ️ No run_id specified, searching for latest failed PR run...") | |
| run_id = find_latest_failed_pr_run(github_token, repo) | |
| if not all([github_token, repo, run_id]): | |
| print("❌ Missing required GitHub environment variables") | |
| print(f" GITHUB_TOKEN: {'set' if github_token else 'missing'}") | |
| print(f" GITHUB_REPOSITORY: {repo or 'missing'}") | |
| print(f" WORKFLOW_RUN_ID: {os.getenv('WORKFLOW_RUN_ID') or 'not set'}") | |
| print(f" GITHUB_RUN_ID: {os.getenv('GITHUB_RUN_ID') or 'not set'}") | |
| return [] |
🧰 Tools
🪛 Ruff (0.15.0)
[warning] 104-104: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
🤖 Prompt for AI Agents
In @.github/scripts/classify_workflow_errors.py around lines 96 - 113, The code
may call find_latest_failed_pr_run with github_token == None causing
"Authorization: Bearer None" requests; before invoking find_latest_failed_pr_run
(the block using github_event_name and run_id), ensure github_token is present
or return early — e.g., check github_token (and repo) alongside run_id or move
the existing all([github_token, repo, run_id]) guard above the workflow_dispatch
branch; alternatively, add a defensive check inside find_latest_failed_pr_run to
bail when github_token is falsy before building the Authorization header.
| # Token usage summary | ||
| total_prompt_tokens = sum(c.prompt_tokens for c in classifications) | ||
| total_completion_tokens = sum(c.completion_tokens for c in classifications) | ||
| total_cached_tokens = sum(c.cached_tokens for c in classifications) |
There was a problem hiding this comment.
Token usage totals will be inflated due to per-error duplication in classifier.py.
sum(c.prompt_tokens for c in classifications) aggregates tokens that are duplicated across all ErrorClassification objects from the same API call (see error_classification/classifier.py Lines 208-210). Once that upstream issue is resolved, this aggregation will be correct.
🤖 Prompt for AI Agents
In @.github/scripts/classify_workflow_errors.py around lines 505 - 508, The
totals are inflated because the list classifications contains multiple
ErrorClassification entries per single API call; change the aggregation to sum
tokens once per unique API call instead of per classification. Build a map keyed
by a stable identifier on ErrorClassification (e.g., an api_call_id or
request_id attribute; if not available, use a fallback unique tuple like
(prompt_text, completion_text, timestamp)) and then compute total_prompt_tokens,
total_completion_tokens, and total_cached_tokens by summing the values from each
unique key rather than summing across classifications directly; update the code
around the variables total_prompt_tokens, total_completion_tokens,
total_cached_tokens to use this deduplicated collection.
| // Check for recent nudge to avoid spam | ||
| const { data: comments } = await github.rest.issues.listComments({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: prNumber, | ||
| per_page: 10, | ||
| direction: 'desc', | ||
| }); | ||
|
|
||
| const recentNudge = comments.find(c => | ||
| c.body && c.body.includes(NUDGE_MARKER) && | ||
| (now - new Date(c.created_at)) / MS_PER_HOUR < NUDGE_COOLDOWN_HOURS | ||
| ); |
There was a problem hiding this comment.
direction: 'desc' is silently ignored — deduplication may fail on busy PRs.
The GitHub REST API for listing comments on a specific issue (GET /repos/{owner}/{repo}/issues/{issue_number}/comments) does not support the direction parameter. Comments are always returned oldest-first. With per_page: 10, this fetches the 10 oldest comments, so a recent nudge marker would be missed on PRs with 10+ comments, causing duplicate nudges within the 24h cooldown.
The same issue applies to the merge reminder dedup check at lines 160-166.
Proposed fix — fetch last page instead
- const { data: comments } = await github.rest.issues.listComments({
- owner: context.repo.owner,
- repo: context.repo.repo,
- issue_number: prNumber,
- per_page: 10,
- direction: 'desc',
- });
+ // GitHub's per-issue comment endpoint always returns oldest-first.
+ // Fetch all and search from end to find recent nudge markers.
+ const allComments = await github.paginate(
+ github.rest.issues.listComments,
+ {
+ owner: context.repo.owner,
+ repo: context.repo.repo,
+ issue_number: prNumber,
+ per_page: 100,
+ }
+ );
+ const comments = allComments;🤖 Prompt for AI Agents
In @.github/workflows/lgtm-bot-stale.yml around lines 96 - 108, The comments
listing currently uses an unsupported direction parameter and requests the 10
oldest comments, causing the recent-nudge dedup (recentNudge using NUDGE_MARKER,
per_page, MS_PER_HOUR, NUDGE_COOLDOWN_HOURS) to miss recent nudges on busy PRs;
update the logic to fetch a larger page (e.g., per_page: 100) and search from
the end (or take the last N entries) to find NUDGE_MARKER within the cooldown
window, and apply the same change to the merge reminder dedup check (the
equivalent recentMergeReminder / MERGE_REMINDER_MARKER logic) so you reliably
examine the most recent comments instead of the oldest.
| # Initialize appropriate client based on API format | ||
| if config.api_format == "openai": | ||
| # For OpenAI-compatible APIs (like NVIDIA) | ||
| self.client = None # Will use requests directly | ||
| self.api_base_url = config.api_base_url or "https://api.openai.com/v1" | ||
| else: | ||
| # For native Anthropic API | ||
| self.client = Anthropic(api_key=config.api_key) | ||
| self.api_base_url = None |
There was a problem hiding this comment.
Defaulting api_base_url to https://api.openai.com/v1 may silently send requests to the wrong API.
When config.api_format == "openai" but config.api_base_url is None, Line 76 falls back to https://api.openai.com/v1. Since the default config is designed around NVIDIA's inference API (and from_env always sets api_base_url), this fallback is unlikely to trigger in practice — but if it does, it would send an NVIDIA API key to OpenAI's endpoint, producing a confusing auth error. Consider raising an error or using the NVIDIA default here as well.
Proposed fix
if config.api_format == "openai":
# For OpenAI-compatible APIs (like NVIDIA)
self.client = None # Will use requests directly
- self.api_base_url = config.api_base_url or "https://api.openai.com/v1"
+ self.api_base_url = config.api_base_url or "https://inference-api.nvidia.com/v1"📝 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.
| # Initialize appropriate client based on API format | |
| if config.api_format == "openai": | |
| # For OpenAI-compatible APIs (like NVIDIA) | |
| self.client = None # Will use requests directly | |
| self.api_base_url = config.api_base_url or "https://api.openai.com/v1" | |
| else: | |
| # For native Anthropic API | |
| self.client = Anthropic(api_key=config.api_key) | |
| self.api_base_url = None | |
| # Initialize appropriate client based on API format | |
| if config.api_format == "openai": | |
| # For OpenAI-compatible APIs (like NVIDIA) | |
| self.client = None # Will use requests directly | |
| self.api_base_url = config.api_base_url or "https://inference-api.nvidia.com/v1" | |
| else: | |
| # For native Anthropic API | |
| self.client = Anthropic(api_key=config.api_key) | |
| self.api_base_url = None |
🤖 Prompt for AI Agents
In `@error_classification/claude_client.py` around lines 72 - 80, The current
constructor branch that handles config.api_format == "openai" silently defaults
self.api_base_url to "https://api.openai.com/v1" which can send the wrong API
key; instead, update the initialization in claude_client.py so that when
config.api_format == "openai" and config.api_base_url is None you raise a clear
exception (e.g., ValueError) telling the caller to provide api_base_url (or
explicitly choose the correct NVIDIA default) rather than silently falling back;
change the code around the block that sets self.client and self.api_base_url
(the if config.api_format == "openai" branch) to validate config.api_base_url
and raise an error with a descriptive message if it's missing.
| response = self._openai_request_with_retry(payload) | ||
| data = response.json() | ||
|
|
||
| # Parse OpenAI format response | ||
| content = data["choices"][0]["message"]["content"] | ||
| usage = data.get("usage", {}) | ||
|
|
||
| # Parse JSON from response | ||
| result = self._parse_full_log_response(content) | ||
|
|
||
| # Add token usage | ||
| result["prompt_tokens"] = usage.get("prompt_tokens", 0) | ||
| result["completion_tokens"] = usage.get("completion_tokens", 0) | ||
| result["cached_tokens"] = usage.get("cached_tokens", 0) | ||
| result["model_version"] = data.get("model", self.config.model) | ||
|
|
||
| return result |
There was a problem hiding this comment.
Unguarded chained index access on OpenAI response can crash with a confusing error.
data["choices"][0]["message"]["content"] on Line 387 will raise KeyError or IndexError if the API response is malformed (e.g., an error body returned with a 200, or an empty choices array). A defensive check or a try/except with a clear message would improve debuggability.
Proposed fix
response = self._openai_request_with_retry(payload)
data = response.json()
- # Parse OpenAI format response
- content = data["choices"][0]["message"]["content"]
+ # Parse OpenAI format response (with defensive access)
+ try:
+ content = data["choices"][0]["message"]["content"]
+ except (KeyError, IndexError, TypeError) as exc:
+ raise ValueError(
+ f"Unexpected API response structure: {str(data)[:300]}"
+ ) from exc
usage = data.get("usage", {})🤖 Prompt for AI Agents
In `@error_classification/claude_client.py` around lines 383 - 399, The code
currently assumes the OpenAI response has choices[0].message.content and will
throw KeyError/IndexError; wrap the extraction of content (after calling
_openai_request_with_retry and parsing response.json()) in a defensive check or
try/except: verify data is a dict, that "choices" exists and is a non-empty list
and that choices[0] has "message" and "content" keys, or catch exceptions and
raise/log a clear error including the raw response body; then pass the validated
content into _parse_full_log_response and continue to attach usage/model fields
as before. Ensure you reference the response coming from
_openai_request_with_retry, the local variable data, and the call to
_parse_full_log_response when implementing the guard.
| pull_requests = workflow_run.get("pull_requests", []) | ||
| print(f" pull_requests array length: {len(pull_requests)}") | ||
|
|
||
| if pull_requests and len(pull_requests) > 0: | ||
| pr_num = int(pull_requests[0].get("number")) | ||
| print(f" ✅ Found PR #{pr_num} from workflow_run.pull_requests") | ||
| return pr_num |
There was a problem hiding this comment.
int(pull_requests[0].get("number")) will raise TypeError if "number" is absent.
dict.get("number") returns None when the key is missing, and int(None) raises TypeError. Line 331 has the opposite problem (no int() coercion). Both should guard for None.
Proposed fix
if pull_requests and len(pull_requests) > 0:
- pr_num = int(pull_requests[0].get("number"))
- print(f" ✅ Found PR #{pr_num} from workflow_run.pull_requests")
- return pr_num
+ raw_pr_num = pull_requests[0].get("number")
+ if raw_pr_num is not None:
+ pr_num = int(raw_pr_num)
+ print(f" ✅ Found PR #{pr_num} from workflow_run.pull_requests")
+ return pr_num🤖 Prompt for AI Agents
In `@error_classification/pr_commentator.py` around lines 294 - 300, The code
calls int(pull_requests[0].get("number")) which will raise TypeError if "number"
is missing; update the extraction to guard against None by first retrieving the
raw value (e.g., num = pull_requests[0].get("number")), check if num is not None
(and optionally isinstance/isdigit if strings possible), then set pr_num =
int(num); if num is None skip/continue or handle the missing case. Apply the
same guard to the other place that extracts a PR number without checking for
None so both branches consistently validate before calling int().
| anthropic>=0.40.0 | ||
| requests>=2.28.0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and read the requirements.txt file
find . -name "requirements.txt" -type f | head -20Repository: ai-dynamo/dynamo
Length of output: 351
🏁 Script executed:
# Read the specific file mentioned in the review
cat -n error_classification/requirements.txtRepository: ai-dynamo/dynamo
Length of output: 321
🏁 Script executed:
# Check for version pinning patterns in other requirements files
find . -name "requirements*.txt" -type f -exec head -15 {} + 2>/dev/null | head -60Repository: ai-dynamo/dynamo
Length of output: 2954
🌐 Web query:
anthropic python package version 0.40.0 pypi
💡 Result:
PyPI: anthropic Python package — version 0.40.0
- Install:
pip install anthropic==0.40.0[1] - Release date: Nov 28, 2024 [1]
- Summary on PyPI: “The official Python library for the anthropic API” [1]
- Python requirement: Python >= 3.8 [1]
- Note: PyPI indicates a newer version is available (e.g., 0.79.0 uploaded Feb 7, 2026) [1][2]
Sources: [1] [2]
Update dependency versions to comply with the repository's version pinning policy.
The repository has a documented requirement (see container/deps/requirements.txt) that explicitly prohibits open-ended >= operators, stating they "allow untested future versions that may introduce breaking changes, create non-reproducible builds, and cause dependency conflicts."
Both dependencies must be updated:
anthropic>=0.40.0should use bounded syntax (e.g.,anthropic>=0.40.0,<0.80.0) or pinned exactlyrequests>=2.28.0should use bounded syntax or pinned exactly
The anthropic SDK has rapid version churn (0.40.0 released Nov 2024, but 0.79.0 now available as of Feb 2026), making bounded ranges especially important here.
🤖 Prompt for AI Agents
In `@error_classification/requirements.txt` around lines 5 - 6, The requirements
file uses open-ended version specifiers for anthropic and requests
(anthropic>=0.40.0 and requests>=2.28.0) which violates the repo policy; update
these to bounded ranges or exact pins (for example change anthropic to a bounded
spec like anthropic>=0.40.0,<0.80.0 or an exact pin, and change requests to a
bounded spec like requests>=2.28.0,<3.0.0 or an exact pin) so the dependency
lines referencing "anthropic" and "requests" conform to the repository's version
pinning policy.
I would like to see the resulting comment generated by the diagnosis with my pending review comments |
- Paginate reviews in lgtm-bot.yml (main + synchronize paths) and lgtm_bot_diagnose.py to handle PRs with 30+ review entries - Add timeout=30 to all HTTP calls missing timeouts (github_request urlopen, pr_commentator patch/post/get) - Remove unsupported direction=desc parameter from issue comments endpoint in 3 locations (GitHub API silently ignores it) - Suppress Slack webhook URL in curl error output (2>/dev/null) - Fix RateLimiter stale timestamp: use time.time() after sleep instead of pre-sleep value - Guard int() on MAX_RPM env var with clear error message - Fix token count inflation: assign API call tokens only to first ErrorClassification to avoid multiplied totals in aggregates - Add int() coercion to _find_pr_by_commit return value Signed-off-by: Dan Gil <dagil@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
@dillon-cullinan Good question — here is what this gives us beyond the GitHub UI:
The CONTRIBUTING.md update documents these features for contributors so they know what to expect. |
OK - I'll see why that's not working |
|
and sorry for the dramatic increase in size - Nate's PR gave me a lot of idea. I didn't realize you were actively reviewing |
|
/diagnose |
This is all available just on the PR alone. Its not across separate tabs, its all on the same page. It is all completely viewable within the This functionality is just attempting to recreate the Github UI to post an extremely long comment that is very easily lost in history and becomes useless in a PR. In fact, its possible this comment might even hit a character limit. The number of lines used to "build the comment" in the python script is nearing 200. That is an extremely large comment. Additionally, this PR makes countless API requests. Do you know what the API request limit for the token is? And do you have any guesstimate on how many API calls this is doing per hour if it were to be implemented across the repository?
This functionality I will admit has some usefulness, but also could've been done in about 50 lines of code, and not thousands.
This PR is not doing that currently, that is a separate branch and a separate PR with what Nate is doing, of which I'm also trying to tone down heavily what its doing. This PR should be rebased onto Before the code was merged with Nates, there was a small section using Nvidia's inference service to diagnose logs, of which I'm questioning how useful this is anyway. What is the point of diagnosing a Lint Checker? It already tells you exactly how to fix things.
Why does it matter who or why? That is only our concern and not the external contributor. If we are not keeping up with reviews that is a problem with our process, not a problem that needs to be splattered as a comment on every PR after every commit. If our codeowners are hard to understand then we should fix codeowners, not write more code that explains our complicated codeowners.
I don't believe this functionality scales at all, actually. I predict that it will have various problems.
Couldn't we have just documented what the requirements to merge a pull request are instead of doing this then? |
3 participants
Summary
lgtmlabel, live merge checklist, on-demand diagnostics with LLM-powered CI fix suggestionsArchitecture
flowchart LR subgraph triggers[" "] A["PR opened/pushed"] --> B["lgtm-bot-pr-open"] C["CI completes / review changes"] --> D["lgtm-bot"] E["/lgtm-bot diagnose"] --> F["lgtm-bot-diagnose"] G["Daily schedule"] --> H["lgtm-bot-stale"] end B --> B1["Auto-request CODEOWNERS reviews"] B --> B2["Post required teams + /ok to test"] D --> D1["Evaluate all merge gates"] D1 --> D2{"All pass?"} D2 -- Yes --> D3["Add lgtm label + Slack"] D2 -- No --> D4["Update checklist"] F --> F1["Full diagnosis + LLM fix suggestions"] H --> H1["Nudge stale reviews (48h)"] H --> H2["Remind to merge (24h)"]Details
New Workflows
lgtm-bot.ymllgtmlabel, maintains live checklistlgtm-bot-diagnose.yml/lgtm-bot diagnosecommentlgtm-bot-pr-open.yml/ok to testguidancelgtm-bot-stale.ymlNew Script
.github/scripts/lgtm_bot_diagnose.py— Diagnosis engine with CODEOWNERS parsing, GitHub REST/GraphQL API, and NVIDIA LLM API for CI failure fix suggestions.Updated
CONTRIBUTING.md— New "Merge Readiness & LGTM Bot" section. Clarifies/ok to testis per-commit and CODEOWNERS reviews are auto-requested.Required Secrets/Variables
NVIDIA_INFERENCE_API_KEYSLACK_LGTM_WEBHOOK_URLSLACK_LGTM_ENABLEDWhere should the reviewer start?
.github/workflows/lgtm-bot.yml— Core label + checklist logic.github/scripts/lgtm_bot_diagnose.py— Diagnosis engineCONTRIBUTING.md— Documentation updatesTest Plan
yaml.safe_load)py_compile)Summary by CodeRabbit
Release Notes
New Features
Documentation