(MOT-4299) fix(ci): run daily Harness benchmarks against Registry - #735
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR adds registry stack identity generation, dynamic and static stack resolution, deployment metadata propagation, and infrastructure-failure reporting across Harness E2E workflows, benchmark collection, snapshots, metrics, and execution reports. ChangesRegistry-backed Harness E2E execution
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant HarnessWorkflow
participant run_deployed_ci
participant registry_stack_identity
participant BenchmarkCollector
HarnessWorkflow->>run_deployed_ci: Pass stack resolution inputs
run_deployed_ci->>registry_stack_identity: Resolve worker versions and lock digest
registry_stack_identity-->>run_deployed_ci: Return stack identity
run_deployed_ci-->>HarnessWorkflow: Write deployment result
HarnessWorkflow->>BenchmarkCollector: Pass deployment and stack metadata
BenchmarkCollector-->>HarnessWorkflow: Produce failure metrics and reports
Possibly related PRs
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 |
skill-check — worker0 verified, 55 skipped (no docs/).
Four for four. Nicely done. |
faee9f7 to
24e4c0d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
.github/scripts/collect_harness_e2e_benchmarks.py (2)
179-196: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the dead
seenset and consider narrowing the upward search.The function returns on the first existing candidate, and the three candidates are distinct paths. The
seenset therefore never suppresses a duplicate. Remove it.The three-level walk also reaches the artifact root. If a stray
deployment.jsonexists above a scenario directory, a scenario without its own deployment file inherits it and gets classified asinfra_failed. The workflow currently writesdeployment.jsoninside each scenario artifact, so two levels are sufficient.♻️ Proposed refactor
def load_deployment(context_path: Path | None) -> dict[str, Any] | None: if context_path is None: return None candidates = ( context_path.parent / "deployment.json", context_path.parent.parent / "deployment.json", - context_path.parent.parent.parent / "deployment.json", ) - seen: set[Path] = set() for candidate in candidates: - if candidate in seen or not candidate.is_file(): + if not candidate.is_file(): continue - seen.add(candidate) value = load_json(candidate) if not isinstance(value, dict): raise CollectionError(f"{candidate} must contain an object") return value return None🤖 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/scripts/collect_harness_e2e_benchmarks.py around lines 179 - 196, Update load_deployment to remove the unused seen set and its duplicate-check logic, and restrict candidates to the scenario directory and its parent (two deployment.json locations). Preserve validation and first-existing-file behavior.
704-708: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated single-observation resolution in
collect. Both blocks apply the same rule: start fromstack_metadata(config, None), then override when exactly one release observation and one stack observation exist. Extract one helper and call it from both places.
.github/scripts/collect_harness_e2e_benchmarks.py#L704-L708: replace the suite block with a call to a shared helper, for exampleresolve_release_and_stack(config, release_observations, stack_observations)..github/scripts/collect_harness_e2e_benchmarks.py#L850-L854: replace the snapshot block with a call to the same helper.🤖 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/scripts/collect_harness_e2e_benchmarks.py around lines 704 - 708, Extract the duplicated release/stack resolution into a shared helper, such as resolve_release_and_stack, that starts with stack_metadata(config, None) and overrides each value only when its corresponding observation list has exactly one item. In .github/scripts/collect_harness_e2e_benchmarks.py lines 704-708, replace the suite resolution block with the helper call; make the same replacement in lines 850-854 for the snapshot resolution block..github/scripts/registry_stack_identity.py (1)
22-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRead the lock file once to guarantee the digest matches the validated content.
Line 24 reads the file as text, and line 50 reads it again as bytes. The digest therefore covers a second read, not the content that was validated. Line 50 is also outside the
tryblock, so anOSErrorthere escapes as a traceback instead of theinvalid_lock:contract. A single read fixes both points.♻️ Proposed refactor
def stack_identity(lock_path: Path) -> dict[str, Any]: try: - document = yaml.safe_load(lock_path.read_text()) or {} + raw = lock_path.read_bytes() + document = yaml.safe_load(raw) or {} except (OSError, yaml.YAMLError) as error: raise SystemExit(f"invalid_lock: cannot read {lock_path}: {error}") from error @@ return { "schema_version": 1, - "lock_digest": hashlib.sha256(lock_path.read_bytes()).hexdigest(), + "lock_digest": hashlib.sha256(raw).hexdigest(), "stack_versions": versions, }🤖 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/scripts/registry_stack_identity.py around lines 22 - 52, Update stack_identity to read lock_path once as bytes inside the existing try block, derive the YAML text from those bytes for validation, and compute lock_digest from the same bytes. Ensure any read failure remains wrapped in the existing invalid_lock SystemExit contract and remove the second lock_path.read_bytes() call..github/scripts/tests/test_collect_harness_e2e_benchmarks.py (1)
61-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd one assertion for precedence over the other blocking statuses.
The new case proves
infra_failedbeatspassed. It does not prove thatinfra_failedbeatstechnical_failedorincomplete, which is the actual ordering guarantee insemantic_result_status.♻️ Proposed addition
assert ( semantic_result_status( passed=True, hard_gate_failures=0, technical_failures=0, infra_failures=1, ) == "infra_failed" ) + assert ( + semantic_result_status( + passed=False, + hard_gate_failures=1, + technical_failures=1, + infra_failures=1, + complete=False, + ) + == "infra_failed" + )🤖 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/scripts/tests/test_collect_harness_e2e_benchmarks.py around lines 61 - 69, Add assertions in the semantic_result_status tests to verify that an infra failure takes precedence over both technical failures and incomplete results, not only passed results. Keep the existing infra_failed expectation and vary the relevant blocking-status inputs while preserving the infra_failures=1 condition.harness/tests/e2e/run-deployed-ci.sh (2)
382-390: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value
release_versionloses the requested value in dynamic mode.Line 389 overwrites
release_versionwith the resolved harness version.write_deployment_resultthen emits the resolved version in bothrelease_versionandactual_release_version. The record no longer states that the run requestedlatest.Keep the requested value and store the resolved value in a separate variable, so the deployment record distinguishes the request from the result.
🤖 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/run-deployed-ci.sh` around lines 382 - 390, In the resolve_stack block, preserve the requested release_version value (latest) and assign the resolved .stack_versions.harness value to a separate variable. Update write_deployment_result and any downstream uses so release_version reports the request while actual_release_version reports the resolved version, using the existing resolved-value flow where applicable.
391-393: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated expected-digest comparison. Both branches compute
lock_digestand then run the identical comparison againstexpected_stack_digest. Move the comparison to a single place after theif/else, so the two branches only compute the digest.
harness/tests/e2e/run-deployed-ci.sh#L391-L393: remove the comparison from the dynamic branch and keep only the digest assignment.harness/tests/e2e/run-deployed-ci.sh#L414-L416: remove the comparison from the static branch and place one shared comparison afterfion line 417.🤖 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/run-deployed-ci.sh` around lines 391 - 393, In harness/tests/e2e/run-deployed-ci.sh, update the dynamic branch at lines 391-393 and static branch at lines 414-416 to only assign lock_digest; remove the duplicated expected_stack_digest comparisons from both sites, then add one shared comparison after the closing fi at line 417 using the existing lock_digest and expected_stack_digest values.
🤖 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-daily.yml:
- Around line 82-90: Update the daily workflow so the build job resolves
harness@latest once, captures its digest, and passes that value as stack_digest
to every matrix job. Disable per-job latest resolution by changing the matrix
configuration around resolve_stack, and ensure the collected benchmark metadata
uses the pinned digest instead of relying on empty stack_versions or
stack_observations fallbacks.
In `@harness/tests/e2e/README.md`:
- Around line 234-240: Update the later daily-lane description in the README to
state that it installs and evaluates registry artifacts resolved from latest,
without falling back to source-built repository binaries; keep the quickstart
validator distinction accurate. Also update the “Harness E2E Daily” table row to
include the infra_failed classification.
In `@harness/tests/e2e/run-deployed-ci.sh`:
- Around line 166-169: Update write_deployment_result to create
"$artifact_dir/results" before copying deployment.json, ensuring the directory
exists when cleanup runs after an early registry-phase failure.
---
Nitpick comments:
In @.github/scripts/collect_harness_e2e_benchmarks.py:
- Around line 179-196: Update load_deployment to remove the unused seen set and
its duplicate-check logic, and restrict candidates to the scenario directory and
its parent (two deployment.json locations). Preserve validation and
first-existing-file behavior.
- Around line 704-708: Extract the duplicated release/stack resolution into a
shared helper, such as resolve_release_and_stack, that starts with
stack_metadata(config, None) and overrides each value only when its
corresponding observation list has exactly one item. In
.github/scripts/collect_harness_e2e_benchmarks.py lines 704-708, replace the
suite resolution block with the helper call; make the same replacement in lines
850-854 for the snapshot resolution block.
In @.github/scripts/registry_stack_identity.py:
- Around line 22-52: Update stack_identity to read lock_path once as bytes
inside the existing try block, derive the YAML text from those bytes for
validation, and compute lock_digest from the same bytes. Ensure any read failure
remains wrapped in the existing invalid_lock SystemExit contract and remove the
second lock_path.read_bytes() call.
In @.github/scripts/tests/test_collect_harness_e2e_benchmarks.py:
- Around line 61-69: Add assertions in the semantic_result_status tests to
verify that an infra failure takes precedence over both technical failures and
incomplete results, not only passed results. Keep the existing infra_failed
expectation and vary the relevant blocking-status inputs while preserving the
infra_failures=1 condition.
In `@harness/tests/e2e/run-deployed-ci.sh`:
- Around line 382-390: In the resolve_stack block, preserve the requested
release_version value (latest) and assign the resolved .stack_versions.harness
value to a separate variable. Update write_deployment_result and any downstream
uses so release_version reports the request while actual_release_version reports
the resolved version, using the existing resolved-value flow where applicable.
- Around line 391-393: In harness/tests/e2e/run-deployed-ci.sh, update the
dynamic branch at lines 391-393 and static branch at lines 414-416 to only
assign lock_digest; remove the duplicated expected_stack_digest comparisons from
both sites, then add one shared comparison after the closing fi at line 417
using the existing lock_digest and expected_stack_digest values.
🪄 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: da3f51d0-243a-44d3-b586-b09e2d763d6e
📒 Files selected for processing (9)
.github/scripts/collect_harness_e2e_benchmarks.py.github/scripts/registry_stack_identity.py.github/scripts/tests/test_collect_harness_e2e_benchmarks.py.github/scripts/tests/test_registry_stack_identity.py.github/workflows/_harness-e2e.yml.github/workflows/harness-e2e-daily.yml.github/workflows/harness-e2e-main.ymlharness/tests/e2e/README.mdharness/tests/e2e/run-deployed-ci.sh
| stack_mode: registry | ||
| resolve_stack: true | ||
| benchmark_lane: daily | ||
| release_tag: daily/${{ needs.context.outputs.benchmark_day }} | ||
| release_worker: main | ||
| release_version: ${{ needs.context.outputs.benchmark_day }} | ||
| release_worker: harness | ||
| release_version: latest | ||
| release_url: ${{ needs.context.outputs.source_url }} | ||
| registry_tag: daily | ||
| coverage: true | ||
| registry_tag: latest | ||
| coverage: false |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Each matrix job resolves latest independently, so the daily data point can mix stack versions.
resolve_stack: true makes every subject/scenario job run its own harness@latest resolution. If a release publishes while the matrix runs, jobs resolve different versions.
collect_harness_e2e_benchmarks.py then records more than one entry in stack_observations, and lines 704-708 and 850-854 fall back to the configured stack. Because the daily lane passes no stack_versions and no stack_digest, that fallback is empty. The suite and snapshot metadata for that day would then carry no resolved stack version.
The reusable workflow already accepts stack_digest. Resolving latest once in the build job and passing the resolved digest to the matrix would pin the whole day to one stack.
🤖 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-daily.yml around lines 82 - 90, Update the
daily workflow so the build job resolves harness@latest once, captures its
digest, and passes that value as stack_digest to every matrix job. Disable
per-job latest resolution by changing the matrix configuration around
resolve_stack, and ensure the collected benchmark metadata uses the pinned
digest instead of relying on empty stack_versions or stack_observations
fallbacks.
| The daily lane uses the registry mode to measure the currently published live | ||
| stack. It resolves `latest` once per matrix job, records the exact versions and | ||
| SHA-256 digest of the resulting `iii.lock`, and treats Registry resolution | ||
| failures as `infra_failed`; it does not fall back to a source build. The main | ||
| lane keeps the source build and LLVM coverage so operational daily metrics stay | ||
| separate from checkout regression coverage. | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
A later paragraph now contradicts this one.
Lines 329-331 still state that the daily lane evaluates repository binaries built from the resolved default branch commit, does not install registry artifacts, and leaves registry installation to the quickstart validator. The new paragraph and harness-e2e-daily.yml make the daily lane registry-based. Update lines 329-331.
The table row for Harness E2E Daily at line 222 also omits the new infra_failed classification.
📝 Proposed replacement for lines 329-331
-The daily lane evaluates repository binaries built from the resolved default
-branch commit. It does not install registry artifacts; registry installation
-remains the responsibility of the quickstart validator.
+The daily lane installs the published registry stack and evaluates it against
+the resolved default branch commit. The main lane evaluates repository
+binaries built from the checkout. The quickstart validator remains the
+coverage for the `iii worker add` CLI path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@harness/tests/e2e/README.md` around lines 234 - 240, Update the later
daily-lane description in the README to state that it installs and evaluates
registry artifacts resolved from latest, without falling back to source-built
repository binaries; keep the quickstart validator distinction accurate. Also
update the “Harness E2E Daily” table row to include the infra_failed
classification.
| stack_lock_digest: $lock_digest, | ||
| elapsed_ms: $elapsed_ms | ||
| }' >"$artifact_dir/deployment.json" | ||
| cp "$artifact_dir/deployment.json" "$artifact_dir/results/deployment.json" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Create the results directory before copying deployment.json into it.
write_deployment_result runs from cleanup. If the run fails during the registry phase, the workflow step that runs mkdir -p target/harness-e2e/results has not executed yet, so $artifact_dir/results may not exist. The cp then fails. cleanup uses set +e, so the failure is silent, but the results artifact loses deployment.json.
🐛 Proposed fix
}' >"$artifact_dir/deployment.json"
+ mkdir -p "$artifact_dir/results"
cp "$artifact_dir/deployment.json" "$artifact_dir/results/deployment.json"📝 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.
| stack_lock_digest: $lock_digest, | |
| elapsed_ms: $elapsed_ms | |
| }' >"$artifact_dir/deployment.json" | |
| cp "$artifact_dir/deployment.json" "$artifact_dir/results/deployment.json" | |
| stack_lock_digest: $lock_digest, | |
| elapsed_ms: $elapsed_ms | |
| }' >"$artifact_dir/deployment.json" | |
| mkdir -p "$artifact_dir/results" | |
| cp "$artifact_dir/deployment.json" "$artifact_dir/results/deployment.json" |
🤖 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/run-deployed-ci.sh` around lines 166 - 169, Update
write_deployment_result to create "$artifact_dir/results" before copying
deployment.json, ensuring the directory exists when cleanup runs after an early
registry-phase failure.
1 participant
The daily Harness benchmark now measures the currently published Registry stack instead of rebuilding every worker from source. Main keeps the source build and LLVM coverage for checkout regressions, while daily records the exact live stack identity and preserves Registry failures as infrastructure failures.\n\n## Technical details\n\n- Resolve harness@latest and the live support/provider stack through Registry, then validate every iii.lock worker has an exact published version.\n- Build and package only the harness-e2e runner for the daily Registry lane.\n- Record stack_versions, the resolved lock SHA-256 digest, the lock snapshot, worker list, and deployment status in benchmark artifacts and compact metrics.\n- Classify Registry resolution and lock-verification failures as infra_failed; there is no source fallback.\n- Move LLVM coverage to the source-based main lane and remove the daily source/integration build.\n\n## Validation\n\n- bash -n harness/tests/e2e/run-deployed-ci.sh\n- Python compilation and workflow YAML parsing passed.\n- Dependency-free metadata smoke test passed.\n- The focused pytest suite could not run locally because pytest is not installed.\n\nRefs MOT-4299
Summary by CodeRabbit
New Features
Bug Fixes
Documentation