(MOT-4299) Add native local Harness E2E dashboard - #749
Conversation
📝 WalkthroughWalkthroughThe PR adds a Rust-based local Harness E2E dashboard with run control, local persistence, catalog discovery, embedded assets, and browser integration. It adds execution comparison views and removes local-mode publishing from the publishing script. ChangesLocal dashboard runtime
Benchmark-site interface
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
skill-check — worker0 verified, 56 skipped (no docs/).
Four for four. Nicely done. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (5)
harness/tests/e2e/src/main.rs (1)
130-139: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winShut down the context before each
bail!.Both capability checks return early without calling
context.shutdown().await. The success path at Line 141 does call it. The equivalent handler inharness/tests/e2e/src/dashboard/api.rs(catalog) shuts the context down before returning each error. Align this function with that convention.♻️ Proposed change
- if !context.function_exists("harness::send").await? { - bail!( - "connected iii stack does not expose harness::send; verify --url points to the Harness stack" - ); - } - if !context.function_exists("router::models::list").await? { - bail!( - "connected Harness stack does not expose router::models::list; start its llm-router before loading models" - ); - } + for (function_id, hint) in [ + ("harness::send", "verify --url points to the Harness stack"), + ( + "router::models::list", + "start its llm-router before loading models", + ), + ] { + if !context.function_exists(function_id).await? { + context.shutdown().await; + bail!("connected iii stack does not expose {function_id}; {hint}"); + } + }🤖 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 `@harness/tests/e2e/src/main.rs` around lines 130 - 139, Update the capability-check error branches in the surrounding function to call context.shutdown().await before each bail!, matching the cleanup pattern used by catalog and the existing success path; preserve the current error messages and checks..github/benchmark-site/local-runner.js (1)
187-191: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winA catalog refresh re-checks every scenario after the user clears the selection.
selectAllis derived fromprevious.size === 0. That condition is true both on the first load and after the user presses "Clear". On refresh the code then re-checks every scenario, discarding the explicit empty selection.Track first load with a separate flag.
♻️ Proposed change
+ let scenariosFilled = false; + function fillScenarios(scenarios) { const previous = new Set( scenarioInputs().filter((input) => input.checked).map((input) => input.value), ); - const selectAll = previous.size === 0; + const selectAll = !scenariosFilled; + scenariosFilled = true; elements.scenarioOptions.replaceChildren();🤖 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/benchmark-site/local-runner.js around lines 187 - 191, Update fillScenarios so select-all behavior distinguishes initial loading from a user-cleared selection: introduce and maintain a separate first-load flag, and only select every scenario on the initial load. Preserve an explicitly empty selection during subsequent catalog refreshes.harness/tests/e2e/src/dashboard.rs (1)
272-281: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPin the fingerprint contract on both sides.
This test hardcodes
"fnv1a32:607c4fd2". It does not read the browser implementation. If.github/benchmark-site/execution-data.jschanges its fingerprint algorithm, this test still passes and the Rust and browser fingerprints diverge silently.Add a matching assertion with the same input in
.github/benchmark-site/execution-data.test.cjs, or move the vector into a shared fixture file that both test suites read.🤖 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 `@harness/tests/e2e/src/dashboard.rs` around lines 272 - 281, Add a matching fingerprint assertion for the same input used by contract_fingerprint_matches_the_browser_implementation in the browser test suite, execution-data.test.cjs, verifying the expected fnv1a32:607c4fd2 value; alternatively place the shared vector in a fixture consumed by both suites so algorithm changes cannot cause silent divergence..github/benchmark-site/overview-structure.test.cjs (2)
111-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the published mode in the Python test, not against the Python source text.
Line 111 matches the literal
"mode": "published"inside the publisher source file..github/scripts/tests/test_publish_harness_e2e_dashboard.pyalready asserts the same contract at Lines 202-203, and it does so against the generated manifest rather than the source text.The assertion here breaks whenever the Python source is reformatted, for example when a formatter changes the spacing or the quote style. It also passes if the literal appears only in a comment. Remove it and rely on the Python test.
🤖 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/benchmark-site/overview-structure.test.cjs at line 111, Remove the `assert.match` for `"mode": "published"` from the overview structure test; the Python end-to-end test already validates this contract against the generated manifest, so do not assert it against publisher source text.
130-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert on structural CSS properties, not on exact design values.
Lines 132, 141, and 143 pin exact declarations such as
padding: 28px 30px. Any cosmetic restyle fails these tests without a behavioral regression, and the failure message points at a regex rather than at the layout intent.The containment properties carry the real contract:
overflow: hidden,max-width: 100%, andoverflow-wrap: anywhere. Keep those assertions and drop the exact padding values.♻️ Proposed change
- assert.match(styles, /\.local-runner\s*\{[^}]*padding:\s*28px 30px;[^}]*overflow:\s*hidden;/s); + assert.match(styles, /\.local-runner\s*\{[^}]*overflow:\s*hidden;/s);- assert.match(styles, /\.compare-content\s*>\s*\.panel\s*\{[^}]*padding:\s*28px 30px;[^}]*overflow:\s*hidden;/s); + assert.match(styles, /\.compare-content\s*>\s*\.panel\s*\{[^}]*overflow:\s*hidden;/s);🤖 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/benchmark-site/overview-structure.test.cjs around lines 130 - 144, Update the tests around the local runner and comparison panel selectors to stop asserting exact padding values such as 28px 30px. Retain assertions for the behavioral containment properties—overflow hidden, max-width 100%, and overflow-wrap anywhere—so cosmetic spacing changes do not fail the tests.
🤖 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/benchmark-site/compare.js:
- Around line 51-57: Update duration to round the absolute value before
calculating minutes and seconds, then derive both fields from that rounded
duration so values such as 119.9 normalize to 2m 00s instead of producing 60
seconds.
- Around line 71-81: Update the status mapping in status() to include the
running execution state with the appropriate active-status label, so local
executions are not rendered as Unknown while in progress. Preserve all existing
status mappings and fallback behavior.
In @.github/benchmark-site/execution-data.js:
- Around line 1010-1025: Update the subject-set construction in the execution
comparison to derive values from left.subjects and right.subjects instead of
leftRows and rightRows. Preserve the existing subjectId::subjectLabel
normalization, deduplication, sorting, and stableJson comparison so subjects
without scenarios still trigger the warning.
In @.github/benchmark-site/index.html:
- Around line 448-452: Replace the dynamic document.write block in the page with
a normal deferred script tag for local-runner.js. Update the local-runner
initialization flow to no-op when `#local-runner` is absent, and guard the
HarnessLocalRunner.initialize() call in overview.js so a missing runner does not
prevent the dashboard’s read-only history view from rendering.
- Around line 321-323: Update the comparison link markup and renderComparisonBar
flow for the disabled state: remove comparisonLink from the tab order while
aria-disabled is true, restore it when enabled, and add a single click handler
during initialization that prevents activation whenever aria-disabled remains
true.
In @.github/benchmark-site/local-runner.js:
- Around line 113-124: Update refreshJob’s catch path so a failed request
reschedules polling when a job is active, using the existing timer/scheduling
mechanism from renderJob. Preserve the current error display and null return
behavior, and avoid scheduling another poll when no job is active.
In @.github/benchmark-site/overview.js:
- Around line 1114-1121: Update toggleComparison to stop calling renderTable
after changing state.comparison. Instead, update the comparison bar and
synchronize only the affected checkbox checked states directly in the existing
DOM, preserving focus on the checkbox that triggered the toggle while retaining
the two-selection limit.
In @.github/benchmark-site/styles.css:
- Around line 2247-2250: Remove the deprecated and redundant word-break:
break-word declaration from the CSS rule containing white-space and
overflow-wrap, leaving overflow-wrap: anywhere to provide the existing wrapping
behavior.
In `@harness/tests/e2e/README.md`:
- Around line 187-192: Update the dashboard command section in the README to
explicitly state that both commands must be run from the repository root, since
their harness-relative paths depend on that working directory. Preserve the
existing build and dashboard commands.
In `@harness/tests/e2e/src/dashboard/api.rs`:
- Around line 106-112: Wrap the E2eContext::connect call in tokio::time::timeout
using the appropriate connection deadline, and map timeout expiration to
ApiError::bad_request while preserving existing connection-error handling. Keep
the validated url and successful E2eContext result flow unchanged.
- Around line 113-132: Update the function-existence checks in the handler
around context.function_exists, replacing direct map_err(ApiError::internal)?
propagation with captured results that call context.shutdown().await before
returning any error. Preserve the existing bad-request handling for missing
functions and apply the cleanup consistently to both harness::send and
router::models::list checks.
- Around line 44-52: Protect the state-changing routes, especially cancel_run
and start_run, against cross-origin form posts by requiring a custom header or
enforcing a same-origin check before handling requests. Update local-runner.js
to send the required custom header so legitimate requests continue to work, and
ensure both POST endpoints reject requests that lack the protection.
- Around line 161-171: Update execution_manifest() to use the shared or defined
schema-version constant instead of the hardcoded 4, keeping the Rust local
manifest aligned with the Python SCHEMA_VERSION value of 3. Ensure the generated
manifest continues to emit the existing schema_version field.
In `@harness/tests/e2e/src/dashboard/controller.rs`:
- Around line 150-176: Update monitor so the child-process reap and subsequent
job status finalization occur under the same state lock acquisition. Keep the
lock held after try_wait clears state.child, then locate the matching job and
apply the existing completed/cancelled result logic before releasing it; remove
the separate reacquisition between these operations.
In `@harness/tests/e2e/src/dashboard/presenter.rs`:
- Around line 367-384: Update execution_identity so its conclusion mapping
handles JobStatus::Cancelled and JobStatus::Cancelling explicitly instead of
treating all non-failed statuses as success. Preserve failure for
JobStatus::Failed, return an empty conclusion for cancelled and non-terminal
statuses to match the top-level payload, and keep success only for completed
successful executions.
In `@harness/tests/e2e/src/dashboard/store.rs`:
- Around line 71-87: In harness/tests/e2e/src/dashboard/store.rs:71-87, update
load_runs to skip directories when read_metadata fails and use None for report
when read_report fails, preserving remaining executions for execution_summary.
In harness/tests/e2e/src/dashboard/store.rs:52-69, update the other store loop
to log and skip unreadable directories instead of propagating errors, so
Controller::new can still start.
---
Nitpick comments:
In @.github/benchmark-site/local-runner.js:
- Around line 187-191: Update fillScenarios so select-all behavior distinguishes
initial loading from a user-cleared selection: introduce and maintain a separate
first-load flag, and only select every scenario on the initial load. Preserve an
explicitly empty selection during subsequent catalog refreshes.
In @.github/benchmark-site/overview-structure.test.cjs:
- Line 111: Remove the `assert.match` for `"mode": "published"` from the
overview structure test; the Python end-to-end test already validates this
contract against the generated manifest, so do not assert it against publisher
source text.
- Around line 130-144: Update the tests around the local runner and comparison
panel selectors to stop asserting exact padding values such as 28px 30px. Retain
assertions for the behavioral containment properties—overflow hidden, max-width
100%, and overflow-wrap anywhere—so cosmetic spacing changes do not fail the
tests.
In `@harness/tests/e2e/src/dashboard.rs`:
- Around line 272-281: Add a matching fingerprint assertion for the same input
used by contract_fingerprint_matches_the_browser_implementation in the browser
test suite, execution-data.test.cjs, verifying the expected fnv1a32:607c4fd2
value; alternatively place the shared vector in a fixture consumed by both
suites so algorithm changes cannot cause silent divergence.
In `@harness/tests/e2e/src/main.rs`:
- Around line 130-139: Update the capability-check error branches in the
surrounding function to call context.shutdown().await before each bail!,
matching the cleanup pattern used by catalog and the existing success path;
preserve the current error messages and checks.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 39eba388-741b-4b64-af6c-f409fc53a700
⛔ Files ignored due to path filters (1)
harness/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.github/benchmark-site/README.md.github/benchmark-site/compare.html.github/benchmark-site/compare.js.github/benchmark-site/execution-data.js.github/benchmark-site/execution-data.test.cjs.github/benchmark-site/index.html.github/benchmark-site/local-runner.js.github/benchmark-site/overview-structure.test.cjs.github/benchmark-site/overview.js.github/benchmark-site/styles.css.github/scripts/publish_harness_e2e_dashboard.py.github/scripts/serve_harness_e2e_dashboard.py.github/scripts/tests/test_publish_harness_e2e_dashboard.py.github/scripts/tests/test_serve_harness_e2e_dashboard.pyharness/tests/e2e/Cargo.tomlharness/tests/e2e/README.mdharness/tests/e2e/src/catalog.rsharness/tests/e2e/src/dashboard.rsharness/tests/e2e/src/dashboard/api.rsharness/tests/e2e/src/dashboard/assets.rsharness/tests/e2e/src/dashboard/controller.rsharness/tests/e2e/src/dashboard/presenter.rsharness/tests/e2e/src/dashboard/store.rsharness/tests/e2e/src/main.rs
💤 Files with no reviewable changes (2)
- .github/scripts/serve_harness_e2e_dashboard.py
- .github/scripts/tests/test_serve_harness_e2e_dashboard.py
| function duration(value) { | ||
| if (typeof value !== "number") return "—"; | ||
| if (Math.abs(value) < 60) return `${number(value, 1)}s`; | ||
| const sign = value < 0 ? "−" : ""; | ||
| const absolute = Math.abs(value); | ||
| return `${sign}${Math.floor(absolute / 60)}m ${String(Math.round(absolute % 60)).padStart(2, "0")}s`; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize the rounded duration before splitting minutes and seconds.
For example, 119.9 renders as 1m 60s. Round the absolute duration first. Then derive both minute and second fields from that rounded value.
Proposed fix
function duration(value) {
if (typeof value !== "number") return "—";
if (Math.abs(value) < 60) return `${number(value, 1)}s`;
const sign = value < 0 ? "−" : "";
- const absolute = Math.abs(value);
- return `${sign}${Math.floor(absolute / 60)}m ${String(Math.round(absolute % 60)).padStart(2, "0")}s`;
+ const seconds = Math.round(Math.abs(value));
+ return `${sign}${Math.floor(seconds / 60)}m ${String(seconds % 60).padStart(2, "0")}s`;
}📝 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.
| function duration(value) { | |
| if (typeof value !== "number") return "—"; | |
| if (Math.abs(value) < 60) return `${number(value, 1)}s`; | |
| const sign = value < 0 ? "−" : ""; | |
| const absolute = Math.abs(value); | |
| return `${sign}${Math.floor(absolute / 60)}m ${String(Math.round(absolute % 60)).padStart(2, "0")}s`; | |
| } | |
| function duration(value) { | |
| if (typeof value !== "number") return "—"; | |
| if (Math.abs(value) < 60) return `${number(value, 1)}s`; | |
| const sign = value < 0 ? "−" : ""; | |
| const seconds = Math.round(Math.abs(value)); | |
| return `${sign}${Math.floor(seconds / 60)}m ${String(seconds % 60).padStart(2, "0")}s`; | |
| } |
🤖 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/benchmark-site/compare.js around lines 51 - 57, Update duration to
round the absolute value before calculating minutes and seconds, then derive
both fields from that rounded duration so values such as 119.9 normalize to 2m
00s instead of producing 60 seconds.
| function status(value) { | ||
| return { | ||
| passed: "Passed", | ||
| quality_advisory: "Quality advisory", | ||
| hard_gate_failed: "Hard gate failed", | ||
| technical_failed: "Technical failure", | ||
| infra_failed: "Infrastructure failure", | ||
| incomplete: "Incomplete", | ||
| cancelled: "Cancelled", | ||
| }[value] || "Unknown"; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Render the active local execution status.
The local presenter emits status: "running" for active executions. This function maps that status to Unknown in the selection card. Add the running label.
Based on supplied upstream contract, the local presenter emits status: "running" for active executions.
Proposed fix
technical_failed: "Technical failure",
infra_failed: "Infrastructure failure",
incomplete: "Incomplete",
+ running: "Running",
cancelled: "Cancelled",📝 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.
| function status(value) { | |
| return { | |
| passed: "Passed", | |
| quality_advisory: "Quality advisory", | |
| hard_gate_failed: "Hard gate failed", | |
| technical_failed: "Technical failure", | |
| infra_failed: "Infrastructure failure", | |
| incomplete: "Incomplete", | |
| cancelled: "Cancelled", | |
| }[value] || "Unknown"; | |
| } | |
| function status(value) { | |
| return { | |
| passed: "Passed", | |
| quality_advisory: "Quality advisory", | |
| hard_gate_failed: "Hard gate failed", | |
| technical_failed: "Technical failure", | |
| infra_failed: "Infrastructure failure", | |
| incomplete: "Incomplete", | |
| running: "Running", | |
| cancelled: "Cancelled", | |
| }[value] || "Unknown"; | |
| } |
🤖 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/benchmark-site/compare.js around lines 71 - 81, Update the status
mapping in status() to include the running execution state with the appropriate
active-status label, so local executions are not rendered as Unknown while in
progress. Preserve all existing status mappings and fallback behavior.
| const leftSubjects = [ | ||
| ...new Set( | ||
| [...leftRows.values()].map( | ||
| (row) => `${row.subjectId}::${row.subjectLabel}`, | ||
| ), | ||
| ), | ||
| ].sort(); | ||
| const rightSubjects = [ | ||
| ...new Set( | ||
| [...rightRows.values()].map( | ||
| (row) => `${row.subjectId}::${row.subjectLabel}`, | ||
| ), | ||
| ), | ||
| ].sort(); | ||
| if (stableJson(leftSubjects) !== stableJson(rightSubjects)) { | ||
| warnings.push("The executions use different subject sets."); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Build subject-set warnings from execution.subjects.
The current code derives subject sets from scenario rows. A subject with no scenarios is omitted. Two executions can then use different subjects without the warning.
Use left.subjects and right.subjects to build these sets.
🤖 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/benchmark-site/execution-data.js around lines 1010 - 1025, Update
the subject-set construction in the execution comparison to derive values from
left.subjects and right.subjects instead of leftRows and rightRows. Preserve the
existing subjectId::subjectLabel normalization, deduplication, sorting, and
stableJson comparison so subjects without scenarios still trigger the warning.
| <a id="comparison-link" class="button" href="./compare.html" aria-disabled="true"> | ||
| Compare selected | ||
| </a> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The disabled comparison link stays keyboard-operable.
aria-disabled="true" does not block activation of an anchor. The CSS rule .comparison-bar a[aria-disabled="true"] { pointer-events: none; } blocks the pointer only. A keyboard user can Tab to this link and press Enter. The browser then opens ./compare.html without the left and right parameters.
Remove the link from the tab order while it is disabled, and block activation in the handler.
♿ Proposed fix
Markup:
- <a id="comparison-link" class="button" href="./compare.html" aria-disabled="true">
+ <a id="comparison-link" class="button" href="./compare.html" aria-disabled="true" tabindex="-1">
Compare selected
</a>.github/benchmark-site/overview.js, inside renderComparisonBar:
elements.comparisonLink.setAttribute("aria-disabled", String(!ready));
+ elements.comparisonLink.tabIndex = ready ? 0 : -1;And register once during initialization:
elements.comparisonLink.addEventListener("click", (event) => {
if (elements.comparisonLink.getAttribute("aria-disabled") === "true") {
event.preventDefault();
}
});🤖 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/benchmark-site/index.html around lines 321 - 323, Update the
comparison link markup and renderComparisonBar flow for the disabled state:
remove comparisonLink from the tab order while aria-disabled is true, restore it
when enabled, and add a single click handler during initialization that prevents
activation whenever aria-disabled remains true.
| <script> | ||
| if (window.HARNESS_EXECUTIONS?.mode === "local") { | ||
| document.write('<script src="./local-runner.js"><\/script>'); | ||
| } | ||
| </script> |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Replace document.write with a static script tag, and guard the consumer.
Two problems combine here.
document.writeis a deprecated parser-blocking pattern. It also couples the load to parse order. Ifoverview.jsis loaded as a module or withdefer, the ordering guarantee is different from a plain classic script..github/benchmark-site/overview.jsLine 1303 callswindow.HarnessLocalRunner.initialize()without a guard. Iflocal-runner.jsfails to load, or ordering changes, that line throws aTypeErrorinsideinitialize(). The throw happens beforerender(), so the whole dashboard renders blank instead of degrading to a read-only history view.
Load local-runner.js with a normal <script defer> tag and let the module no-op when #local-runner is absent. Then guard the call site.
🛠️ Proposed fix
index.html:
- <script>
- if (window.HARNESS_EXECUTIONS?.mode === "local") {
- document.write('<script src="./local-runner.js"><\/script>');
- }
- </script>
+ <script src="./local-runner.js" defer></script>.github/benchmark-site/local-runner.js, make it inert outside local mode:
- global.HarnessLocalRunner = { initialize };
+ if (elements.form) global.HarnessLocalRunner = { initialize };.github/benchmark-site/overview.js Line 1303:
- if (isLocal) window.HarnessLocalRunner.initialize();
+ if (isLocal) window.HarnessLocalRunner?.initialize();#!/bin/bash
# Description: Inspect script tag ordering and loading attributes in the dashboard page.
set -euo pipefail
rg -n '<script' .github/benchmark-site/index.html🤖 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/benchmark-site/index.html around lines 448 - 452, Replace the
dynamic document.write block in the page with a normal deferred script tag for
local-runner.js. Update the local-runner initialization flow to no-op when
`#local-runner` is absent, and guard the HarnessLocalRunner.initialize() call in
overview.js so a missing runner does not prevent the dashboard’s read-only
history view from rendering.
| if !context | ||
| .function_exists("harness::send") | ||
| .await | ||
| .map_err(ApiError::internal)? | ||
| { | ||
| context.shutdown().await; | ||
| return Err(ApiError::bad_request( | ||
| "connected iii stack does not expose harness::send", | ||
| )); | ||
| } | ||
| if !context | ||
| .function_exists("router::models::list") | ||
| .await | ||
| .map_err(ApiError::internal)? | ||
| { | ||
| context.shutdown().await; | ||
| return Err(ApiError::bad_request( | ||
| "connected Harness stack does not expose router::models::list; start its llm-router", | ||
| )); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The function_exists error paths leak the connected E2eContext.
Lines 116 and 126 use map_err(ApiError::internal)?. When function_exists returns Err, the handler returns immediately and never reaches a shutdown().await call. Every other exit path in this handler calls context.shutdown().await explicitly at Lines 118, 128, and 136, so Drop alone is not treated as sufficient cleanup.
Refresh catalog is a documented operator action. Repeated refreshes against an unstable stack leak one connection each time.
Capture the result, shut the context down, then propagate.
🔒 Proposed fix
- if !context
- .function_exists("harness::send")
- .await
- .map_err(ApiError::internal)?
- {
+ let has_send = match context.function_exists("harness::send").await {
+ Ok(value) => value,
+ Err(error) => {
+ context.shutdown().await;
+ return Err(ApiError::internal(error));
+ }
+ };
+ if !has_send {
context.shutdown().await;
return Err(ApiError::bad_request(
"connected iii stack does not expose harness::send",
));
}
- if !context
- .function_exists("router::models::list")
- .await
- .map_err(ApiError::internal)?
- {
+ let has_models = match context.function_exists("router::models::list").await {
+ Ok(value) => value,
+ Err(error) => {
+ context.shutdown().await;
+ return Err(ApiError::internal(error));
+ }
+ };
+ if !has_models {
context.shutdown().await;
return Err(ApiError::bad_request(
"connected Harness stack does not expose router::models::list; start its llm-router",
));
}A guard type with a Drop implementation would remove the repetition across all five exit paths.
📝 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 !context | |
| .function_exists("harness::send") | |
| .await | |
| .map_err(ApiError::internal)? | |
| { | |
| context.shutdown().await; | |
| return Err(ApiError::bad_request( | |
| "connected iii stack does not expose harness::send", | |
| )); | |
| } | |
| if !context | |
| .function_exists("router::models::list") | |
| .await | |
| .map_err(ApiError::internal)? | |
| { | |
| context.shutdown().await; | |
| return Err(ApiError::bad_request( | |
| "connected Harness stack does not expose router::models::list; start its llm-router", | |
| )); | |
| } | |
| let has_send = match context.function_exists("harness::send").await { | |
| Ok(value) => value, | |
| Err(error) => { | |
| context.shutdown().await; | |
| return Err(ApiError::internal(error)); | |
| } | |
| }; | |
| if !has_send { | |
| context.shutdown().await; | |
| return Err(ApiError::bad_request( | |
| "connected iii stack does not expose harness::send", | |
| )); | |
| } | |
| let has_models = match context.function_exists("router::models::list").await { | |
| Ok(value) => value, | |
| Err(error) => { | |
| context.shutdown().await; | |
| return Err(ApiError::internal(error)); | |
| } | |
| }; | |
| if !has_models { | |
| context.shutdown().await; | |
| return Err(ApiError::bad_request( | |
| "connected Harness stack does not expose router::models::list; start its llm-router", | |
| )); | |
| } |
🤖 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 `@harness/tests/e2e/src/dashboard/api.rs` around lines 113 - 132, Update the
function-existence checks in the handler around context.function_exists,
replacing direct map_err(ApiError::internal)? propagation with captured results
that call context.shutdown().await before returning any error. Preserve the
existing bad-request handling for missing functions and apply the cleanup
consistently to both harness::send and router::models::list checks.
| Ok(javascript_response(format!( | ||
| "window.HARNESS_EXECUTIONS = {};\n", | ||
| json!({ | ||
| "schema_version": 4, | ||
| "mode": "local", | ||
| "last_update": last_update, | ||
| "repo_url": repository_url(), | ||
| "retention": { "summaries": MAX_EXECUTIONS, "details": MAX_EXECUTIONS }, | ||
| "executions": executions, | ||
| }) | ||
| ))) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare the manifest schema versions across the publisher, the dashboard, and the browser.
rg -n 'SCHEMA_VERSION\s*=' .github/scripts/publish_harness_e2e_dashboard.py
rg -n 'schema_version' .github/benchmark-site --type js
rg -n 'schema_version|LOCAL_SCHEMA_VERSION' harness/tests/e2e/srcRepository: iii-hq/workers
Length of output: 1586
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== dashboard outline =="
ast-grep outline harness/tests/e2e/src/dashboard.rs --view expanded || true
echo "== relevant dashboard.rs =="
sed -n '1,240p' harness/tests/e2e/src/dashboard.rs
echo "== dashboard api.rs lines 130-185 =="
sed -n '130,185p' harness/tests/e2e/src/dashboard/api.rs
echo "== publisher schema_version references =="
rg -n 'schema_version|SCHEMA_VERSION' .github/scripts/publish_harness_e2e_dashboard.py
sed -n '1020,1060p' .github/scripts/publish_harness_e2e_dashboard.py
echo "== benchmark-site schema_version usages =="
rg -n -C 3 'schema_version|schemaVersion|window.HARNESS_EXECUTIONS' .github/benchmark-site --type js | sed -n '1,220p'
echo "== local store version enforcement =="
sed -n '1,60p' harness/tests/e2e/src/dashboard/store.rs
sed -n '280,320p' harness/tests/e2e/src/dashboard.rsRepository: iii-hq/workers
Length of output: 19843
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== harness execution data consumer sections =="
ast-grep outline .github/benchmark-site/execution-data.js --view expanded || true
rg -n -C 4 'schemaVersion|normalizeExecution|mergeExecutionHistory|schema_version' .github/benchmark-site/execution-data.js
sed -n '1,140p' .github/benchmark-site/execution-data.js
sed -n '300,385p' .github/benchmark-site/execution-data.js
echo "== dashboard builder context =="
rg -n -C 4 'execution_detail|load_execution_summaries|execution_summary|contract_fingerprint|MAX_EXECUTIONS' harness/tests/e2e/src/dashboard -t rustRepository: iii-hq/workers
Length of output: 19782
Do not hardcode the local execution-manifest schema_version.
execution_manifest() writes 4, while the Python publisher uses SCHEMA_VERSION = 3, and the benchmark site currently constructs local manifests with schema_version: 3. This literal can drift from the shared manifest contract. Keep the Rust value in sync with the Python constant and avoid hardcoding it.
🤖 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 `@harness/tests/e2e/src/dashboard/api.rs` around lines 161 - 171, Update
execution_manifest() to use the shared or defined schema-version constant
instead of the hardcoded 4, keeping the Rust local manifest aligned with the
Python SCHEMA_VERSION value of 3. Ensure the generated manifest continues to
emit the existing schema_version field.
| async fn monitor(self: Arc<Self>, id: String) { | ||
| loop { | ||
| tokio::time::sleep(Duration::from_millis(250)).await; | ||
| let finished = { | ||
| let mut state = self.state.lock().await; | ||
| match state | ||
| .child | ||
| .as_mut() | ||
| .and_then(|child| child.try_wait().transpose()) | ||
| { | ||
| Some(Ok(status)) => { | ||
| state.child = None; | ||
| Some(Ok(status)) | ||
| } | ||
| Some(Err(error)) => { | ||
| state.child = None; | ||
| Some(Err(error)) | ||
| } | ||
| None => None, | ||
| } | ||
| }; | ||
| let Some(result) = finished else { continue }; | ||
| let mut state = self.state.lock().await; | ||
| let Some(job) = state.job.as_mut().filter(|job| job.id == id) else { | ||
| return; | ||
| }; | ||
| let cancelling = job.status == JobStatus::Cancelling; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Race between child reaping and the status update can mislabel a completed run as cancelled.
monitor uses two separate lock acquisitions. The first block (Lines 153-170) sets state.child = None and then releases the lock at Line 170. The second acquisition happens at Line 172.
In that gap, cancel can acquire the lock. It observes job.status == JobStatus::Running, so it passes the check at Line 137. state.child is already None, so the if let at Line 141 does not run and no signal is sent. cancel then sets the status to Cancelling and persists it.
monitor re-acquires the lock, reads cancelling == true at Line 176, and marks the run Cancelled. The run finished normally and wrote results.json, but the branch at Line 183 never executes. The dashboard reports a cancelled execution with no report, and execution_summary in presenter.rs maps it to "cancelled" with "availability": "unavailable".
Hold a single lock across the reap and the status update.
🔒 Proposed fix: reap and finalize under one lock
async fn monitor(self: Arc<Self>, id: String) {
loop {
tokio::time::sleep(Duration::from_millis(250)).await;
- let finished = {
- let mut state = self.state.lock().await;
- match state
- .child
- .as_mut()
- .and_then(|child| child.try_wait().transpose())
- {
- Some(Ok(status)) => {
- state.child = None;
- Some(Ok(status))
- }
- Some(Err(error)) => {
- state.child = None;
- Some(Err(error))
- }
- None => None,
- }
- };
- let Some(result) = finished else { continue };
- let mut state = self.state.lock().await;
+ let mut state = self.state.lock().await;
+ let finished = match state
+ .child
+ .as_mut()
+ .and_then(|child| child.try_wait().transpose())
+ {
+ Some(outcome) => {
+ state.child = None;
+ Some(outcome)
+ }
+ None => None,
+ };
+ let Some(result) = finished else {
+ drop(state);
+ continue;
+ };
let Some(job) = state.job.as_mut().filter(|job| job.id == id) else {
return;
};📝 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.
| async fn monitor(self: Arc<Self>, id: String) { | |
| loop { | |
| tokio::time::sleep(Duration::from_millis(250)).await; | |
| let finished = { | |
| let mut state = self.state.lock().await; | |
| match state | |
| .child | |
| .as_mut() | |
| .and_then(|child| child.try_wait().transpose()) | |
| { | |
| Some(Ok(status)) => { | |
| state.child = None; | |
| Some(Ok(status)) | |
| } | |
| Some(Err(error)) => { | |
| state.child = None; | |
| Some(Err(error)) | |
| } | |
| None => None, | |
| } | |
| }; | |
| let Some(result) = finished else { continue }; | |
| let mut state = self.state.lock().await; | |
| let Some(job) = state.job.as_mut().filter(|job| job.id == id) else { | |
| return; | |
| }; | |
| let cancelling = job.status == JobStatus::Cancelling; | |
| async fn monitor(self: Arc<Self>, id: String) { | |
| loop { | |
| tokio::time::sleep(Duration::from_millis(250)).await; | |
| let mut state = self.state.lock().await; | |
| let finished = match state | |
| .child | |
| .as_mut() | |
| .and_then(|child| child.try_wait().transpose()) | |
| { | |
| Some(outcome) => { | |
| state.child = None; | |
| Some(outcome) | |
| } | |
| None => None, | |
| }; | |
| let Some(result) = finished else { | |
| drop(state); | |
| continue; | |
| }; | |
| let Some(job) = state.job.as_mut().filter(|job| job.id == id) else { | |
| return; | |
| }; | |
| let cancelling = job.status == JobStatus::Cancelling; |
🤖 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 `@harness/tests/e2e/src/dashboard/controller.rs` around lines 150 - 176, Update
monitor so the child-process reap and subsequent job status finalization occur
under the same state lock acquisition. Keep the lock held after try_wait clears
state.child, then locate the matching job and apply the existing
completed/cancelled result logic before releasing it; remove the separate
reacquisition between these operations.
| fn execution_identity(metadata: &RunMetadata) -> Value { | ||
| json!({ | ||
| "id": metadata.id, | ||
| "run_id": metadata.id, | ||
| "attempt": 1, | ||
| "event": "local", | ||
| "actor": actor(), | ||
| "workflow_name": "Harness E2E Local", | ||
| "workflow_url": "", | ||
| "label": metadata.label, | ||
| "started_at": metadata.started_at, | ||
| "completed_at": metadata.completed_at, | ||
| "conclusion": if metadata.status == JobStatus::Failed { "failure" } else { "success" }, | ||
| "head_sha": "", | ||
| "head_branch": "local", | ||
| "repository": "", | ||
| }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
A cancelled execution reports conclusion as "success".
Line 379 sets conclusion to "failure" only when the status is JobStatus::Failed. Every other status yields "success", including JobStatus::Cancelled and JobStatus::Cancelling.
This also contradicts the sibling field. For a cancelled run without a report, Line 56 sets the top-level conclusion to "", while execution.conclusion here is "success". One payload carries two different conclusions for the same run, and the nested one marks a cancelled execution as successful.
Map the non-terminal and cancelled statuses explicitly.
🐛 Proposed fix
- "conclusion": if metadata.status == JobStatus::Failed { "failure" } else { "success" },
+ "conclusion": match metadata.status {
+ JobStatus::Failed => "failure",
+ JobStatus::Cancelled => "cancelled",
+ JobStatus::Running | JobStatus::Cancelling => "",
+ JobStatus::Completed => "success",
+ },📝 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.
| fn execution_identity(metadata: &RunMetadata) -> Value { | |
| json!({ | |
| "id": metadata.id, | |
| "run_id": metadata.id, | |
| "attempt": 1, | |
| "event": "local", | |
| "actor": actor(), | |
| "workflow_name": "Harness E2E Local", | |
| "workflow_url": "", | |
| "label": metadata.label, | |
| "started_at": metadata.started_at, | |
| "completed_at": metadata.completed_at, | |
| "conclusion": if metadata.status == JobStatus::Failed { "failure" } else { "success" }, | |
| "head_sha": "", | |
| "head_branch": "local", | |
| "repository": "", | |
| }) | |
| } | |
| fn execution_identity(metadata: &RunMetadata) -> Value { | |
| json!({ | |
| "id": metadata.id, | |
| "run_id": metadata.id, | |
| "attempt": 1, | |
| "event": "local", | |
| "actor": actor(), | |
| "workflow_name": "Harness E2E Local", | |
| "workflow_url": "", | |
| "label": metadata.label, | |
| "started_at": metadata.started_at, | |
| "completed_at": metadata.completed_at, | |
| "conclusion": match metadata.status { | |
| JobStatus::Failed => "failure", | |
| JobStatus::Cancelled => "cancelled", | |
| JobStatus::Running | JobStatus::Cancelling => "", | |
| JobStatus::Completed => "success", | |
| }, | |
| "head_sha": "", | |
| "head_branch": "local", | |
| "repository": "", | |
| }) | |
| } |
🤖 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 `@harness/tests/e2e/src/dashboard/presenter.rs` around lines 367 - 384, Update
execution_identity so its conclusion mapping handles JobStatus::Cancelled and
JobStatus::Cancelling explicitly instead of treating all non-failed statuses as
success. Preserve failure for JobStatus::Failed, return an empty conclusion for
cancelled and non-terminal statuses to match the top-level payload, and keep
success only for completed successful executions.
| pub(super) fn load_runs(runs_dir: &Path) -> Result<Vec<StoredRun>> { | ||
| let mut runs = Vec::new(); | ||
| for entry in fs::read_dir(runs_dir)? { | ||
| let entry = entry?; | ||
| if !entry.file_type()?.is_dir() { | ||
| continue; | ||
| } | ||
| let Some(metadata) = read_metadata(&entry.path())? else { | ||
| continue; | ||
| }; | ||
| runs.push(StoredRun { | ||
| metadata, | ||
| report: read_report(&entry.path())?, | ||
| }); | ||
| } | ||
| Ok(runs) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Per-directory read failures abort the whole traversal in both store loops. read_metadata and read_report return Err for a malformed or schema-mismatched file. Both loops propagate that error with ?, so one bad run directory fails the entire operation instead of being skipped. Cancellation kills the runner mid-write, so a truncated results.json is an expected outcome of the documented cancel feature.
harness/tests/e2e/src/dashboard/store.rs#L71-L87: skip the run directory whenread_metadatafails, and degradereporttoNonewhenread_reportfails, so/executions.jsstill returns the remaining executions.execution_summaryinpresenter.rsalready handlesreport: None.harness/tests/e2e/src/dashboard/store.rs#L52-L69: skip and log the unreadable directory instead of propagating, soController::newdoes not fail and the dashboard still starts.
📍 Affects 1 file
harness/tests/e2e/src/dashboard/store.rs#L71-L87(this comment)harness/tests/e2e/src/dashboard/store.rs#L52-L69
🤖 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 `@harness/tests/e2e/src/dashboard/store.rs` around lines 71 - 87, In
harness/tests/e2e/src/dashboard/store.rs:71-87, update load_runs to skip
directories when read_metadata fails and use None for report when read_report
fails, preserving remaining executions for execution_summary. In
harness/tests/e2e/src/dashboard/store.rs:52-69, update the other store loop to
log and skip unreadable directories instead of propagating errors, so
Controller::new can still start.
1 participant
Summary
Native local Harness E2E dashboard for fast experiment loops: run selected scenarios against the Harness at
III_URL, change the Harness, run again, and compare any two executions side by side.harness-e2e dashboard(aliasserve) serves the embedded UI, discovers the model/scenario catalog from the live stack, executes experiments as child processes of the already-built binary (never Cargo), streams logs, and supports cancellation.target/harness-e2e-local-runs/<id>/as metadata, log, and rawresults.json, and any two executions can be compared — differing subjects, scenario sets, or contracts are shown as warnings, never blockers.mode: "published"and never loadlocal-runner.js, so they cannot call loopback execution APIs.How to run
ws://127.0.0.1:49134).ssh -L 4173:127.0.0.1:4173 user@host.Screenshots
Execution form — catalog discovered from the live stack (models + scenarios), label, and safe-default advanced options:
Live run — status badge, streaming runner log, and cancellation:
Execution history — local persistence with pass/cancel status; select two rows to compare:
Comparison — B minus A deltas for the whole execution and per scenario, with contract-change markers:
Why
Local Harness changes such as system prompt or skill updates need a fast experiment loop. The previous Python path duplicated runner contracts, required compatibility adapters, and could spend time compiling or generating dashboard artifacts.
Validation
cargo test --locked --manifest-path harness/Cargo.toml -p harness-e2e— 99 passedcargo clippy --locked --manifest-path harness/Cargo.toml -p harness-e2e --all-targets -- -D warningsnode --test .github/benchmark-site/*.test.cjs— 39 passed, including executable published/local loader isolationpython3 -m pytest -q .github/scripts/tests— 243 passed, 3 subtests passedcargo fmt --manifest-path harness/Cargo.toml --all -- --checkandgit diff --check origin/main...HEADdirect_answerrun (PASS, 5s, $0.0103, no Cargo invocation), history persistence,/runs/<id>.jsondetail, two-run comparison, and mid-run cancellation (running → cancelling → cancelled) — screenshots above are from that session.Summary by CodeRabbit