(MOT-4295) feat(ci): publish harness integration and e2e coverage to the metrics site - #664
Merged
Conversation
… site Instrument the harness stack with LLVM source-based coverage in the daily E2E cycle and publish browsable HTML reports plus a line-coverage trend to the gh-pages metrics site under dev/harness-e2e/coverage/. - coverage input on _harness-integration.yml and _harness-e2e.yml: instrumented builds (-Cinstrument-coverage with runtime counter relocation), continuous-mode LLVM_PROFILE_FILE so profiles survive the SIGTERM/SIGKILL stack teardown, dedicated rust-cache keys, and coverage report artifacts - new e2e coverage job merges per-matrix profraw artifacts against the packaged stack binaries; the integration job reports in-job - harness-e2e-daily.yml enables coverage on both suites so the reports ride the workflow_run the benchmark publisher already consumes - harness-e2e-benchmark.yml installs the reports with replace-latest semantics, emits a coverage summary manifest, and records a line-coverage time series via github-action-benchmark - shared .github/scripts/coverage_report.sh (llvm-profdata merge + llvm-cov show/report/export) reused by CI and the new local 'make -C harness integration-coverage' target - the integration process supervisor now passes LLVM_PROFILE_FILE through env_clear so instrumented children keep writing profiles
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
skill-check — worker0 verified, 51 skipped (no docs/).
Four for four. Nicely done. |
📝 WalkthroughWalkthroughChangesCoverage reporting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DailyWorkflow
participant CoverageWorkflow
participant coverage_report.sh
participant BenchmarkSite
DailyWorkflow->>CoverageWorkflow: enable coverage for E2E and integration runs
CoverageWorkflow->>coverage_report.sh: provide binaries and profraw profiles
coverage_report.sh-->>CoverageWorkflow: produce HTML, text, and JSON reports
CoverageWorkflow->>BenchmarkSite: upload suite coverage artifacts
BenchmarkSite-->>BenchmarkSite: publish summaries and dashboard manifest
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/_harness-integration.yml (1)
151-159: 🩺 Stability & Availability | 🔵 TrivialConfirm the job's timeout accounts for instrumented builds.
_harness-e2e.ymlexplicitly increases its build timeout wheninputs.coverageis true, with a comment noting instrumented cold builds are slower. This job adds the same instrumentation overhead (coverage RUSTFLAGS on the integration build, Console build, and Playwright run) but the provided diff shows no equivalent timeout adjustment. Confirm the job's existing timeout already has enough margin for the daily coverage run, since this file's job-leveltimeout-minutesvalue is not part of the current diff.🤖 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 @.github/workflows/_harness-integration.yml around lines 151 - 159, Confirm the job-level timeout for the integration workflow provides sufficient margin when inputs.coverage enables instrumented builds and test execution. If it does not, update the timeout-minutes setting for the job containing “Run integration scenarios,” matching the established coverage-aware timeout approach used by the related end-to-end workflow.
🤖 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 @.github/workflows/_harness-e2e.yml:
- Around line 557-559: Update the checkout steps in
.github/workflows/_harness-e2e.yml lines 557-559 and
.github/workflows/_harness-integration.yml lines 27-29 to set
persist-credentials to false, preserving the existing checkout references and
inputs.
In @.github/workflows/harness-e2e-benchmark.yml:
- Around line 132-173: Add failure isolation to both the “Collect coverage trend
metrics” and “Store coverage history” steps so malformed or incomplete coverage
data cannot prevent subsequent dashboard and GitHub Pages steps from running.
Apply the existing continue-on-error pattern to these two steps, preserving the
current coverage metric behavior while allowing later essential steps to
proceed.
---
Nitpick comments:
In @.github/workflows/_harness-integration.yml:
- Around line 151-159: Confirm the job-level timeout for the integration
workflow provides sufficient margin when inputs.coverage enables instrumented
builds and test execution. If it does not, update the timeout-minutes setting
for the job containing “Run integration scenarios,” matching the established
coverage-aware timeout approach used by the related end-to-end workflow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 431af6d1-e5a7-4400-af72-55dd0192ba44
📒 Files selected for processing (10)
.github/benchmark-site/coverage/index.html.github/benchmark-site/index.html.github/scripts/coverage_report.sh.github/workflows/_harness-e2e.yml.github/workflows/_harness-integration.yml.github/workflows/harness-e2e-benchmark.yml.github/workflows/harness-e2e-daily.yml.gitignoreharness/Makefileharness/tests/integration/src/process/spec.rs
Comment on lines
+557
to
+559
| - uses: actions/checkout@v5 | ||
| with: | ||
| ref: ${{ inputs.source_ref }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Set persist-credentials: false on checkout steps in coverage jobs.
Both checkout steps run in jobs that only read the repository and never push back. Static analysis (zizmor, artipacked) flags both for persisting the git credential by default, which widens the credential-exposure surface unnecessarily.
.github/workflows/_harness-e2e.yml#L557-L559: addpersist-credentials: falseto the coverage job's checkout step..github/workflows/_harness-integration.yml#L27-L29: addpersist-credentials: falseto the primary checkout step.
🔒 Proposed fix (apply to both sites)
- uses: actions/checkout@v5
with:
ref: ${{ inputs.source_ref }}
+ persist-credentials: false📝 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.
Suggested change
| - uses: actions/checkout@v5 | |
| with: | |
| ref: ${{ inputs.source_ref }} | |
| - uses: actions/checkout@v5 | |
| with: | |
| ref: ${{ inputs.source_ref }} | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 557-559: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
📍 Affects 2 files
.github/workflows/_harness-e2e.yml#L557-L559(this comment).github/workflows/_harness-integration.yml#L27-L29
🤖 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 @.github/workflows/_harness-e2e.yml around lines 557 - 559, Update the
checkout steps in .github/workflows/_harness-e2e.yml lines 557-559 and
.github/workflows/_harness-integration.yml lines 27-29 to set
persist-credentials to false, preserving the existing checkout references and
inputs.
Source: Linters/SAST tools
Comment on lines
+132
to
+173
| - name: Collect coverage trend metrics | ||
| id: coverage-metrics | ||
| if: steps.benchmark.outputs.source_sha != '' | ||
| run: | | ||
| set -euo pipefail | ||
| entries='[]' | ||
| add_entry() { | ||
| local name=$1 summary=$2 | ||
| [[ -f "$summary" ]] || return 0 | ||
| entries=$( | ||
| jq --arg name "$name" --slurpfile summary "$summary" \ | ||
| '. + [{ | ||
| name: $name, | ||
| unit: "%", | ||
| value: (($summary[0].totals.lines.percent * 100 | round) / 100) | ||
| }]' <<<"$entries" | ||
| ) | ||
| } | ||
| add_entry "E2E line coverage" target/coverage/e2e/summary.json | ||
| add_entry "Integration line coverage" target/coverage/integration/summary.json | ||
| if [[ $(jq 'length' <<<"$entries") -eq 0 ]]; then | ||
| echo "No coverage summaries in this run; skipping trend history." | ||
| echo "has_metrics=false" >> "$GITHUB_OUTPUT" | ||
| exit 0 | ||
| fi | ||
| printf '%s\n' "$entries" > target/coverage/coverage-metrics.json | ||
| echo "has_metrics=true" >> "$GITHUB_OUTPUT" | ||
|
|
||
| - name: Store coverage history | ||
| if: steps.coverage-metrics.outputs.has_metrics == 'true' | ||
| uses: benchmark-action/github-action-benchmark@v1 | ||
| with: | ||
| name: Harness Stack Coverage | ||
| tool: customBiggerIsBetter | ||
| output-file-path: target/coverage/coverage-metrics.json | ||
| ref: ${{ steps.benchmark.outputs.source_sha }} | ||
| gh-pages-branch: gh-pages | ||
| benchmark-data-dir-path: dev/harness-e2e/coverage-trend | ||
| max-items-in-chart: 100 | ||
| github-token: ${{ github.token }} | ||
| auto-push: true | ||
| summary-always: true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add failure isolation to the new coverage-trend steps so a coverage hiccup cannot block the whole dashboard publish.
"Collect coverage trend metrics" and "Store coverage history" have no continue-on-error or if: always(). Both run under set -euo pipefail. A truncated or malformed summary.json (partial artifact) or a totals.lines.percent field that is null will make jq --slurpfile ... '... * 100 | round ...' fail (jq errors on arithmetic with null), which fails the step. Because none of the later steps ("Checkout benchmark history", "Install coverage reports", "Install dashboard and execution history", "Upload GitHub Pages artifact") declare an if: condition, GitHub Actions skips all of them by default when a prior step fails. That means an unrelated coverage-summary problem would prevent publishing the execution-history dashboard and the GitHub Pages deployment for that run entirely.
This directly undermines the resilience goal stated in the comment at lines 192-193 ("a partial daily failure keeps yesterday's pages"), since the failure would happen upstream of that safeguard. Mirror the continue-on-error: true pattern already used for the coverage artifact downloads (lines 64-83) on these two steps, or guard the later essential steps with if: always().
🛡️ Proposed fix
- name: Collect coverage trend metrics
id: coverage-metrics
if: steps.benchmark.outputs.source_sha != ''
+ continue-on-error: true
run: | - name: Store coverage history
if: steps.coverage-metrics.outputs.has_metrics == 'true'
+ continue-on-error: true
uses: benchmark-action/github-action-benchmark@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.
Suggested change
| - name: Collect coverage trend metrics | |
| id: coverage-metrics | |
| if: steps.benchmark.outputs.source_sha != '' | |
| run: | | |
| set -euo pipefail | |
| entries='[]' | |
| add_entry() { | |
| local name=$1 summary=$2 | |
| [[ -f "$summary" ]] || return 0 | |
| entries=$( | |
| jq --arg name "$name" --slurpfile summary "$summary" \ | |
| '. + [{ | |
| name: $name, | |
| unit: "%", | |
| value: (($summary[0].totals.lines.percent * 100 | round) / 100) | |
| }]' <<<"$entries" | |
| ) | |
| } | |
| add_entry "E2E line coverage" target/coverage/e2e/summary.json | |
| add_entry "Integration line coverage" target/coverage/integration/summary.json | |
| if [[ $(jq 'length' <<<"$entries") -eq 0 ]]; then | |
| echo "No coverage summaries in this run; skipping trend history." | |
| echo "has_metrics=false" >> "$GITHUB_OUTPUT" | |
| exit 0 | |
| fi | |
| printf '%s\n' "$entries" > target/coverage/coverage-metrics.json | |
| echo "has_metrics=true" >> "$GITHUB_OUTPUT" | |
| - name: Store coverage history | |
| if: steps.coverage-metrics.outputs.has_metrics == 'true' | |
| uses: benchmark-action/github-action-benchmark@v1 | |
| with: | |
| name: Harness Stack Coverage | |
| tool: customBiggerIsBetter | |
| output-file-path: target/coverage/coverage-metrics.json | |
| ref: ${{ steps.benchmark.outputs.source_sha }} | |
| gh-pages-branch: gh-pages | |
| benchmark-data-dir-path: dev/harness-e2e/coverage-trend | |
| max-items-in-chart: 100 | |
| github-token: ${{ github.token }} | |
| auto-push: true | |
| summary-always: true | |
| - name: Collect coverage trend metrics | |
| id: coverage-metrics | |
| if: steps.benchmark.outputs.source_sha != '' | |
| continue-on-error: true | |
| run: | | |
| set -euo pipefail | |
| entries='[]' | |
| add_entry() { | |
| local name=$1 summary=$2 | |
| [[ -f "$summary" ]] || return 0 | |
| entries=$( | |
| jq --arg name "$name" --slurpfile summary "$summary" \ | |
| '. + [{ | |
| name: $name, | |
| unit: "%", | |
| value: (($summary[0].totals.lines.percent * 100 | round) / 100) | |
| }]' <<<"$entries" | |
| ) | |
| } | |
| add_entry "E2E line coverage" target/coverage/e2e/summary.json | |
| add_entry "Integration line coverage" target/coverage/integration/summary.json | |
| if [[ $(jq 'length' <<<"$entries") -eq 0 ]]; then | |
| echo "No coverage summaries in this run; skipping trend history." | |
| echo "has_metrics=false" >> "$GITHUB_OUTPUT" | |
| exit 0 | |
| fi | |
| printf '%s\n' "$entries" > target/coverage/coverage-metrics.json | |
| echo "has_metrics=true" >> "$GITHUB_OUTPUT" | |
| - name: Store coverage history | |
| if: steps.coverage-metrics.outputs.has_metrics == 'true' | |
| continue-on-error: true | |
| uses: benchmark-action/github-action-benchmark@v1 | |
| with: | |
| name: Harness Stack Coverage | |
| tool: customBiggerIsBetter | |
| output-file-path: target/coverage/coverage-metrics.json | |
| ref: ${{ steps.benchmark.outputs.source_sha }} | |
| gh-pages-branch: gh-pages | |
| benchmark-data-dir-path: dev/harness-e2e/coverage-trend | |
| max-items-in-chart: 100 | |
| github-token: ${{ github.token }} | |
| auto-push: true | |
| summary-always: true |
🤖 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 @.github/workflows/harness-e2e-benchmark.yml around lines 132 - 173, Add
failure isolation to both the “Collect coverage trend metrics” and “Store
coverage history” steps so malformed or incomplete coverage data cannot prevent
subsequent dashboard and GitHub Pages steps from running. Apply the existing
continue-on-error pattern to these two steps, preserving the current coverage
metric behavior while allowing later essential steps to proceed.
… detail page The detail page rendered everything eagerly: ~40 metric tiles and 15 run accordions before any interaction, an unbounded failure list, the full prompt expanded per run, and a multi-megabyte JSON.stringify of the whole detail at page load. Reorganize it failure-first: - failure strip becomes a grouped triage: top-5 chips per failing run (count badge + first message), rest behind 'Show all'; the reliability KPI folds into the strip title (5 -> 4 KPIs) - deep links now open every collapsed ancestor <details> (hashchange + delegated clicks), so triage chips land expanded on the right run - Overview and Configuration collapse into disclosures with one-line digests; scenario cards become one-line accordions (open only when failed or sole scenario) with the metric tiles moved into the body - expanded run bodies switch to five lazily-rendered tabs (Evaluation, Usage, Prompt, Sessions, Raw); prompt clamps at 260px with expand; trace/run/page JSON and the download blob serialize on demand - content-visibility: auto on scenario and run accordions New additive helper HarnessExecutionData.groupRunFailures with unit tests; no existing module APIs changed (23/23 site tests pass).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes MOT-4295
Summary
Adds LLVM source-based code coverage for the harness integration and E2E suites and publishes browsable HTML reports plus a line-coverage trend to the gh-pages metrics site at
dev/harness-e2e/coverage/, riding the daily E2E cycle.coverageinput (defaultfalse) on_harness-integration.ymland_harness-e2e.yml— when on:llvm-toolscomponent, per-stepRUSTFLAGS="-Cinstrument-coverage -Cllvm-args=-runtime-counter-relocation", absolute continuous-modeLLVM_PROFILE_FILE(%c) so profiles survive the SIGTERM/SIGKILL stack teardown, and-coverage-suffixed rust-cache keys so normal caches are never poisoned. PR CI and cache-warm are untouched.…-profrawartifact; a newcoveragejob merges them against the packagedharness-e2e-stackbinaries and uploadsharness-e2e-coverage. The prebuiltiiiengine and registry-installed workers stay uninstrumented.harness-integration-coverage) is generated in-job. The process supervisor now passesLLVM_PROFILE_FILEthroughenv_clear(inert outside coverage runs).harness-e2e-daily.ymlenables coverage on the e2e call and adds anintegrationjob, so both artifacts land on the sameworkflow_runthe benchmark publisher consumes.harness-e2e-benchmark.ymlinstalls both HTML reports with replace-latest semantics (a partial daily failure keeps yesterday's report), writes asummary.jsmanifest, and records "E2E line coverage" / "Integration line coverage" as agithub-action-benchmarktime series underdev/harness-e2e/coverage-trend/..github/scripts/coverage_report.sh(llvm-profdata merge + llvm-cov show/report/export) reused by CI and the new localmake -C harness integration-coverage III_BIN=<engine>target.Verification
coverage_report.shsmoke-tested against real profraw/binaries (missing objects degrade to warnings; empty profraw dir no-ops).cargo check -p harness-integrationpasses;make -n integration-coveragedry-runs.gh workflow run "Harness E2E Daily" -f source_sha=<sha>and checkhttps://iii-hq.github.io/workers/dev/harness-e2e/coverage/.First daily run builds the
-coveragecaches cold (slower); subsequent runs reuse them.Summary by CodeRabbit
Also in this PR: execution detail page UX redesign
refactor(benchmark-site): the execution detail page rendered everything eagerly (~40 tiles + 15 run accordions before any interaction, unbounded failure list, full prompt expanded, multi-MBJSON.stringifyat load). It is now failure-first with progressive disclosure:hashchange)HarnessExecutionData.groupRunFailureshelper with unit tests; no existing module APIs changed (23/23 site tests)Verified live with a Playwright-driven Chromium against the static site: triage chip → auto-expanded run on Evaluation, lazy tab activation, prompt clamp, transcript dialog, aggregate/error states, fresh-load hash deep links; DOM pre-expansion dropped to ~550 nodes on the failing sample execution.