testbot: trim the coverage section and render it in one place by jiaenren · Pull Request #1292 · NVIDIA/OSMO

jiaenren · 2026-08-11T23:18:53Z

Description

#1290 trimmed the per-range checklist out of verify_coverage.render_markdown(), but #1291 still shipped ~300 lines of it. That fix was on the wrong code path.

There were two renderers of the same section:

verify_coverage.render_markdown() → coverage_report.md, which the workflow only cats into the run log.
create_pr._build_coverage_section() → read the JSON sidecar and re-rendered the section independently. This is what builds the PR body.

Changes

Trim the renderer that matters. Keep the per-target summary line, name only the ranges still missing coverage, cap at MAX_REPORTED_RANGES with an "and N more" tail.
Collapse the duplication. _build_coverage_section now parses the JSON back into TargetReports via a new verify_coverage.reports_from_json() and calls render_markdown(). One renderer, so this cannot drift again. create_pr.py loses 59 lines.

Before (#1291, 308 lines)

✅ **`src/service/agent/helpers.py`** — 269/269 listed lines hit (100%)
  - ✅ line 49 — 1/1 hit
  - ✅ line 56 — 1/1 hit
  ... ~300 more, all ✅

After (6 lines)

✅ **`src/service/agent/helpers.py`** — 269/269 listed lines hit (100%)

⚠️ **`src/service/core/workflow.py`** — 12/60 listed lines hit (20%)
  - still uncovered: line 1, line 2, ..., and 11 more

Verification

bazel test //src/scripts/testbot:all //src/scripts/testbot/tests:all — 19/19 pass, including testbot-pylint and test_create_pr-pylint (10.00/10). Rendered a #1291-shaped report through the real _build_coverage_section to confirm 308 → 6 lines. reports_from_json keeps the fail-soft coercion the old renderer had, so a malformed field still degrades the body instead of blocking PR creation.

Issue - None

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

🤖 Generated with Claude Code

Summary by CodeRabbit

New Features
- Coverage reports now highlight only uncovered code ranges.
- Reports limit displayed uncovered ranges and summarize additional ranges with an overflow count.
- Single-line uncovered ranges use a more concise format.
- Simplified coverage entries improve readability by removing detailed hit and total information.
Bug Fixes
- Coverage reports now handle incomplete, malformed, or invalid report data more reliably.
- Missing coverage details are handled with sensible defaults to keep reports usable.

#1290 trimmed verify_coverage.render_markdown(), but that is not what builds the PR body. The workflow writes coverage_report.md and only cats it into the run log; the PR body comes from create_pr.py's _build_coverage_section(), a second renderer of the same section that reads the JSON sidecar. So #1291 still shipped ~300 lines of per-range bullets. Apply the same trim there: keep the per-target summary line, name only the ranges still missing coverage, cap at MAX_REPORTED_RANGES with an "and N more" tail. The constant is imported from verify_coverage rather than redeclared, matching the existing guardrails import. On a #1291-shaped report the section goes from 308 lines to 9. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

coderabbitai · 2026-08-11T23:19:20Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: a1eb7227-a865-40a2-a236-6eddd74b4413

📥 Commits

Reviewing files that changed from the base of the PR and between 18a6335 and 0532fdd.

📒 Files selected for processing (1)

src/scripts/testbot/verify_coverage.py

🚧 Files skipped from review as they are similar to previous changes (1)

src/scripts/testbot/verify_coverage.py

📝 Walkthrough

Walkthrough

Coverage JSON is parsed into verifier report objects and rendered by shared Markdown helpers. Coverage output omits covered-range details, formats single-line uncovered ranges as line 5, and caps reported uncovered ranges with an omission count.

Changes

Coverage reporting

Layer / File(s)	Summary
Parse and render coverage reports `src/scripts/testbot/verify_coverage.py`, `src/scripts/testbot/create_pr.py`	Coverage JSON is converted into verifier reports. `create_pr.py` delegates coverage Markdown generation to the shared renderer while retaining fail-soft handling.
Validate concise coverage output `src/scripts/testbot/tests/test_create_pr.py`	Tests verify covered-range omission, singular `line 5` formatting, and truncation beyond `MAX_REPORTED_RANGES`.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

NVIDIA/OSMO#1290: Both changes update coverage-report parsing, rendering, and related tests.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: reducing the coverage section and centralizing its rendering.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch jiaenr/testbot-trim-pr-coverage

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

🧹 Nitpick comments (1)

src/scripts/testbot/tests/test_create_pr.py (1)

1251-1270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Make the overflow fixture depend on MAX_REPORTED_RANGES.

misses has 14 entries. If MAX_REPORTED_RANGES is 14 or greater, correct output includes line 14 and has no overflow suffix. The test then fails without a renderer defect.

Proposed fix

-        misses = [[n, n] for n in range(1, 15)]
+        misses = [
+            [line_number, line_number]
+            for line_number in range(1, MAX_REPORTED_RANGES + 2)
+        ]
...
-        self.assertNotIn("line 14", section)
+        self.assertNotIn(
+            f"line {MAX_REPORTED_RANGES + 1}", section
+        )

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/scripts/testbot/tests/test_create_pr.py` around lines 1251 - 1270, Update
test_still_uncovered_ranges_are_capped so the misses fixture contains more
entries than MAX_REPORTED_RANGES, while preserving the assertion that the
overflow suffix reports the remaining count and the capped output excludes the
final range.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/scripts/testbot/tests/test_create_pr.py`:
- Around line 1251-1270: Update test_still_uncovered_ranges_are_capped so the
misses fixture contains more entries than MAX_REPORTED_RANGES, while preserving
the assertion that the overflow suffix reports the remaining count and the
capped output excludes the final range.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: f7133c00-8612-45bf-a39a-63c322f0f0e9

📥 Commits

Reviewing files that changed from the base of the PR and between bf7d0cf and 2426314.

📒 Files selected for processing (2)

src/scripts/testbot/create_pr.py
src/scripts/testbot/tests/test_create_pr.py

_build_coverage_section re-implemented what verify_coverage already renders, so trimming one left the other unchanged. Replace it with a call: parse the JSON back into TargetReports, then render_markdown. create_pr.py loses 59 lines and there is one renderer to change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

coderabbitai

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/scripts/testbot/create_pr.py`:
- Around line 335-340: Update the coverage report loading block around
reports_from_json to also catch UnicodeDecodeError from Path.read_text,
alongside the existing OSError and json.JSONDecodeError handling. Preserve the
current warning and empty-string return so invalid UTF-8 reports omit the
coverage section without aborting PR creation.

In `@src/scripts/testbot/verify_coverage.py`:
- Around line 275-287: Add validation while constructing RangeResult and
TargetReport: normalize or reject nonpositive or reversed range coordinates,
constrain range hit counts to their totals, and ensure each target’s hit_lines
does not exceed listed_lines. Apply these checks before reports reach rendering
or pass/fail evaluation, preserving valid metrics unchanged.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: a1dab59f-437e-4756-b0a0-43c6fd9b2b28

📥 Commits

Reviewing files that changed from the base of the PR and between 2426314 and 18a6335.

📒 Files selected for processing (3)

src/scripts/testbot/create_pr.py
src/scripts/testbot/tests/test_create_pr.py
src/scripts/testbot/verify_coverage.py

🚧 Files skipped from review as they are similar to previous changes (1)

src/scripts/testbot/tests/test_create_pr.py

int() has no overload for a bare object, so the fail-soft helper failed CI's mypy aspect. Narrow to (int, float, str) before converting. Caught by `bazel test -- //src/scripts/testbot/...`; the package-scoped `:all` pattern used earlier does not run mypy on the library target. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

jiaenren requested a review from a team as a code owner August 11, 2026 23:18

jiaenren temporarily deployed to internal-ci August 11, 2026 23:19 — with GitHub Actions Inactive

coderabbitai Bot reviewed Aug 11, 2026

View reviewed changes

jiaenren changed the title ~~testbot: trim the coverage section create_pr.py actually renders~~ testbot: trim the coverage section and render it in one place Aug 11, 2026

jiaenren had a problem deploying to internal-ci August 11, 2026 23:44 — with GitHub Actions Failure

coderabbitai Bot reviewed Aug 11, 2026

View reviewed changes

Comment thread src/scripts/testbot/create_pr.py

Comment thread src/scripts/testbot/verify_coverage.py

jiaenren temporarily deployed to internal-ci August 11, 2026 23:51 — with GitHub Actions Inactive

ecolternv approved these changes Aug 12, 2026

View reviewed changes

jiaenren temporarily deployed to nim-env August 13, 2026 22:22 — with GitHub Actions Inactive

fernandol-nvidia approved these changes Aug 17, 2026

View reviewed changes

jiaenren merged commit fcaf156 into main Aug 17, 2026
13 checks passed

jiaenren deleted the jiaenr/testbot-trim-pr-coverage branch August 17, 2026 21:06

Navigation Menu

Uh oh!

testbot: trim the coverage section and render it in one place - #1292

testbot: trim the coverage section and render it in one place#1292
jiaenren merged 3 commits into
mainfrom
jiaenr/testbot-trim-pr-coverage

jiaenren commented Aug 11, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Aug 11, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

jiaenren commented Aug 11, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Before (#1291, 308 lines)

After (6 lines)

Verification

Checklist

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jiaenren commented Aug 11, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Aug 11, 2026 •

edited

Loading