-
Notifications
You must be signed in to change notification settings - Fork 0
preserve(harness): checkpoint benchmark envelope work #612
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,8 +10,6 @@ | |
| from datetime import UTC, datetime | ||
| from pathlib import Path | ||
|
|
||
| from jsonschema import validate | ||
|
|
||
| ROOT = Path(__file__).resolve().parents[1] / "src" | ||
| if str(ROOT) not in sys.path: | ||
| sys.path.insert(0, str(ROOT)) | ||
|
|
@@ -174,6 +172,8 @@ def run_runner(repo: str, profile: str, out: str, args) -> None: | |
|
|
||
| if args.dry_run: | ||
| result["result_code"] = "WARN" if not commands else "PASS" | ||
| from harness.benchmark_envelope import add_envelope | ||
| result = add_envelope(result, repo=repo, profile=profile, plan_hash=command_hash) | ||
| Path(out).write_text(json.dumps(result, indent=2)) | ||
| return | ||
|
|
||
|
|
@@ -195,6 +195,8 @@ def run_runner(repo: str, profile: str, out: str, args) -> None: | |
| payload["reproducibility"] = _reproducibility_metadata(profile, args) | ||
| payload["created_at"] = datetime.now(tz=UTC).isoformat() | ||
| payload["command_count"] = len(commands) | ||
| from harness.benchmark_envelope import add_envelope | ||
| payload = add_envelope(payload, repo=repo, profile=profile, plan_hash=command_hash) | ||
|
Comment on lines
+198
to
+199
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Replacing the normal run payload with envelope-only JSON removes Severity Level: Critical 🚨- ❌ Replay CLI cannot compare plans from prior run envelope.
- ❌ second.json replay.same_plan check in tests fails.
- ⚠️ Plan_diff computed against empty prior_commands is misleading.Steps of Reproduction ✅1. Run the first non-dry harness run as in `harness/tests/test_run_harness.py:75-88`,
which executes `python3 harness/scripts/run-harness.py run --repo <repo> --out <out_first>
--timeout 2` and triggers `run_runner()` normal path.
2. Inside `run_runner()` at `harness/scripts/run-harness.py:191-197`, `payload =
evidence_payload(discovery, runs, normalization)` builds the harness evidence (with
`commands`, `runs`, `result_code`, etc.), and `payload["plan_hash"] = command_hash` adds
the current plan hash.
3. At `harness/scripts/run-harness.py:198-199`, `payload = add_envelope(payload,
repo=repo, profile=profile, plan_hash=command_hash)` wraps this evidence into the envelope
defined in `harness/src/harness/benchmark_envelope.py:15-45`, discarding top-level `plan`,
`commands`, and `plan_hash` from the persisted JSON written at `run-harness.py:245`.
4. The second run with replay at `harness/tests/test_run_harness.py:90-104` passes
`--replay <out_first>`, so `run_runner()` reads the envelope via `prior =
json.loads(replay_path.read_text())` at `harness/scripts/run-harness.py:204`, then
attempts `prior_commands = prior.get("plan")` and `prior_commands = prior.get("commands",
[])` plus `prior_hash = prior.get("plan_hash")` at lines 205-208; because the envelope
lacks these keys, `prior_commands` becomes an empty list and `prior_hash` is recomputed
from an empty plan, making `same_plan = prior_hash == command_hash` at line 235 false and
`plan_diff` at line 236 a diff against empty commands, causing
`second_payload["replay"]["same_plan"]` checked in
`harness/tests/test_run_harness.py:108-109` to be incorrect and assertions to fail.(Use Cmd/Ctrl + Click for best experience) Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** harness/scripts/run-harness.py
**Line:** 198:199
**Comment:**
*Logic Error: Replacing the normal run payload with envelope-only JSON removes `plan`/`plan_hash`, so a later `--replay` run cannot recover the prior command plan and will compute `same_plan`/diffs from empty data. Preserve replay-critical fields in persisted output (or embed them in the envelope) so replay comparisons remain correct.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix |
||
|
|
||
| if args.replay: | ||
| replay_path = Path(args.replay) | ||
|
|
@@ -277,6 +279,8 @@ def normalize_run(input_file: str, out: str) -> None: | |
|
|
||
|
|
||
| def validate_artifacts(schema: str, file: str) -> None: | ||
| from jsonschema import validate | ||
|
|
||
| payload = json.loads(Path(file).read_text()) | ||
| schema_json = json.loads(Path(schema).read_text()) | ||
| validate(instance=payload, schema=schema_json) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| """Deterministic umbrella evidence envelope for harness runs.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import hashlib | ||
| import json | ||
| from datetime import UTC, datetime | ||
|
|
||
|
|
||
| def _digest(value: object) -> str: | ||
| encoded = json.dumps(value, sort_keys=True, separators=(",", ":")).encode() | ||
| return hashlib.sha256(encoded).hexdigest() | ||
|
|
||
|
|
||
| def add_envelope(payload: dict, *, repo: str, profile: str, plan_hash: str) -> dict: | ||
|
Check warning on line 15 in harness/src/harness/benchmark_envelope.py
|
||
| identity = {"repo": repo, "commit": "0000000", "harness": "helios-harness", "model": "unknown", "task_id": f"plan:{plan_hash}", "hardware": "unknown"} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For benchmark runs on the same repo path with the same discovered plan but different Git revisions, this hard-coded commit keeps the Useful? React with 👍 / 👎. |
||
| digest = _digest(identity) | ||
| run_id = f"run_{digest}" | ||
| session_id = f"ses_{_digest({'run_id': run_id, 'session': 'default'})}" | ||
| attempt_id = f"att_{_digest({'run_id': run_id, 'attempt': 0})}" | ||
| causality = {"tenant_id": "phenotype", "session_id": session_id, "run_id": run_id, "attempt_id": attempt_id} | ||
| now = datetime.now(UTC).isoformat() | ||
| checkpoint_id = f"cp_{_digest({'run_id': run_id, 'checkpoint': 0})[:16]}" | ||
| events = [] | ||
| for seq, event_type in enumerate(("run_started", "checkpoint", "compaction", "run_finished")): | ||
| event = {"event_id": f"evt_{_digest({'run_id': run_id, 'seq': seq, 'type': event_type})}", "seq": seq, "ts": now, "type": event_type, "payload_sha256": _digest({"type": event_type, "seq": seq}), "causality": causality} | ||
| if event_type in ("checkpoint", "compaction"): | ||
| event["checkpoint_id"] = checkpoint_id | ||
| if event_type == "compaction": | ||
| event["details"] = {"tokens_before": 0, "tokens_after": 0, "retained_event_ids": [], "dropped_event_ids": []} | ||
| events.append(event) | ||
| passed = payload.get("result_code") == "PASS" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: This maps every non- Severity Level: Major
|
||
| legacy_digest = _digest(payload) | ||
| return { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎. |
||
| "schema_version": "1.0.0", | ||
| "tenant_id": "phenotype", "session_id": session_id, "run_id": run_id, "attempt_id": attempt_id, | ||
| "deterministic_identity": {"algorithm": "sha256(canonical-json(inputs))", "canonical_json_sha256": digest, "inputs": identity}, | ||
| "subject": {"repo": repo, "commit": "unknown", "harness": "helios-harness", "runtime": "python", "model": "unknown", "hardware": "unknown"}, | ||
| "lease": {"lease_id": f"lease_{attempt_id[4:20]}", "owner": "helios-harness", "ttl_seconds": 120, "heartbeat_interval_seconds": 20}, | ||
| "task_manifest": {"task_id": f"plan:{plan_hash}", "input_sha256": plan_hash, "timeout_seconds": 1, "assertions": [{"id": "plan_discovered", "kind": "command_plan", "expected": True}], "judge": {"name": "helios-harness", "version": "0.1.0"}}, | ||
| "events": events, | ||
| "result": {"status": "passed" if passed else "failed", "outcome_sha256": legacy_digest, "replay_hash": _digest(events), "failure_class": "none" if passed else "unknown", "artifacts": [{"kind": "report", "uri": f"urn:helios:legacy-evidence:{legacy_digest}", "sha256": legacy_digest}]}, | ||
| "provenance": {"collector": "helios-harness", "collected_at": now, "source_hashes": {"plan": plan_hash}}, | ||
| "signature": {"algorithm": "placeholder", "key_id": "unconfigured", "signature_b64": ""}, | ||
| } | ||
|
Comment on lines
+34
to
+45
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: This returns a brand-new envelope and drops the original payload fields, which breaks existing consumers that still read top-level legacy keys like Severity Level: Major
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,17 +53,23 @@ class RepoManifest: | |
|
|
||
| @classmethod | ||
| def from_repo(cls, repo_root: Path, repo_id: str) -> RepoManifest: | ||
| from subprocess import CalledProcessError, check_output | ||
| from subprocess import CalledProcessError, TimeoutExpired, check_output | ||
|
|
||
| def git_output(*args: str) -> str: | ||
| try: | ||
| return check_output(["git", "-C", root, *args], text=True, timeout=2).strip() | ||
| except (CalledProcessError, TimeoutExpired, FileNotFoundError): | ||
| return "" | ||
|
|
||
| root = str(repo_root.resolve()) | ||
| try: | ||
| remote = check_output(["git", "-C", root, "remote", "get-url", "origin"], text=True).strip() | ||
| except CalledProcessError: | ||
| remote = git_output("remote", "get-url", "origin") | ||
| except (CalledProcessError, TimeoutExpired): | ||
| remote = "(no-remote)" | ||
| try: | ||
| branch = check_output(["git", "-C", root, "branch", "--show-current"], text=True).strip() | ||
| commit = check_output(["git", "-C", root, "rev-parse", "--short", "HEAD"], text=True).strip() | ||
| except CalledProcessError: | ||
| branch = git_output("branch", "--show-current") | ||
| commit = git_output("rev-parse", "--short", "HEAD") | ||
|
Comment on lines
+58
to
+71
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The helper swallows git exceptions and returns an empty string, so the surrounding fallback logic never triggers and sentinel values like Severity Level: Major
|
||
| except (CalledProcessError, TimeoutExpired): | ||
| branch = "(no-git)" | ||
| commit = "" | ||
| default_branch = branch or "main" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| """Importable harness command entrypoints.""" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| """Importable wrapper for the repository's legacy hyphenated runner script.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import importlib.util | ||
| from pathlib import Path | ||
| from types import ModuleType | ||
|
|
||
|
|
||
| def _legacy_module() -> ModuleType: | ||
| script = Path(__file__).resolve().parents[3] / "scripts" / "run-harness.py" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In a non-editable wheel install, Useful? React with 👍 / 👎. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: This wrapper hard-codes a filesystem jump to a top-level script that is not included in the wheel build ( Severity Level: Critical 🚨- ❌ Installed helios-harness CLI fails with ImportError on start.
- ⚠️ Packaging excludes legacy runner required by entrypoint wrapper.Steps of Reproduction ✅1. Build and install the harness wheel using the configuration in
`/workspace/helios-cli/harness/pyproject.toml:1-8`, which declares the script entrypoint
`helios-harness = "harness.scripts.run_harness:main"` and limits wheel contents to
`packages = ["src/harness"]`, excluding the top-level `harness/scripts` directory.
2. After installation, execute the CLI entrypoint `helios-harness`, which calls `main()`
in `/workspace/helios-cli/harness/src/harness/scripts/run_harness.py:20-22`. `main()`
immediately calls `_legacy_module()` defined at lines 10-17 in the same file.
3. `_legacy_module()` computes `script = Path(__file__).resolve().parents[3] / "scripts" /
"run-harness.py"` at
`/workspace/helios-cli/harness/src/harness/scripts/run_harness.py:11`, expecting to find
`harness/scripts/run-harness.py` three levels above the installed module path.
4. In the installed wheel, only `src/harness` is packaged, so the sibling
`scripts/run-harness.py` file does not exist at runtime.
`importlib.util.spec_from_file_location` at run_harness.py:12 either returns a spec with
`loader is None` or fails due to the missing file; `_legacy_module()` then raises
`ImportError(f"Unable to load harness runner: {script}")` at line 13-14, causing the
`helios-harness` CLI entrypoint to fail on startup in installed environments.(Use Cmd/Ctrl + Click for best experience) Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** harness/src/harness/scripts/run_harness.py
**Line:** 11:11
**Comment:**
*Api Mismatch: This wrapper hard-codes a filesystem jump to a top-level script that is not included in the wheel build (`packages = ["src/harness"]`), so the installed CLI entrypoint can fail at runtime with a missing file. Load the runner from package code (or include the legacy script in the package data) instead of traversing outside the installed package tree.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix |
||
| spec = importlib.util.spec_from_file_location("helios_harness_legacy_runner", script) | ||
| if spec is None or spec.loader is None: | ||
| raise ImportError(f"Unable to load harness runner: {script}") | ||
| module = importlib.util.module_from_spec(spec) | ||
| spec.loader.exec_module(module) | ||
| return module | ||
|
|
||
|
|
||
| def main() -> None: | ||
| """Run the canonical harness CLI.""" | ||
| _legacy_module().main() | ||
|
|
||
|
|
||
| def run_runner(repo: str, profile: str, out: str, args) -> None: | ||
| """Expose the runner seam for integration tests and adapters.""" | ||
| _legacy_module().run_runner(repo, profile, out, args) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| import json | ||
| import tempfile | ||
| from types import SimpleNamespace | ||
|
|
||
| from harness.scripts.run_harness import run_runner | ||
|
|
||
|
|
||
| def test_run_runner_emits_benchmark_envelope(tmp_path): | ||
| repo = tmp_path / "repo" | ||
| repo.mkdir() | ||
| (repo / "Makefile").write_text("check:\n\t@echo check\n") | ||
| out = tmp_path / "run.json" | ||
| run_runner(str(repo), "strict-full", str(out), SimpleNamespace( | ||
| replay=None, dry_run=True, max_parallel=2, timeout=2, retries=0, | ||
| retry_delay=1.0, budget=None, continue_on_fail=False, | ||
| )) | ||
| payload = json.loads(out.read_text()) | ||
| try: | ||
| import jsonschema | ||
| from pathlib import Path | ||
| schema_path = Path(__file__).parents[3] / "docs/sessions/20260722-agent-harness-portfolio/artifacts/benchmark_run.schema.json" | ||
| jsonschema.Draft202012Validator(json.loads(schema_path.read_text())).validate(payload) | ||
| except ModuleNotFoundError: | ||
|
Comment on lines
+21
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: This test only ignores missing Severity Level: Major
|
||
| pass | ||
| assert payload["tenant_id"] == "phenotype" | ||
| assert payload["session_id"].startswith("ses_") | ||
| assert payload["run_id"].startswith("run_") | ||
| assert payload["attempt_id"].startswith("att_") | ||
| assert payload["subject"]["harness"] == "helios-harness" | ||
| assert payload["provenance"]["collector"] == "helios-harness" | ||
| assert payload["signature"]["algorithm"] == "placeholder" | ||
| assert {event["type"] for event in payload["events"]} >= {"checkpoint", "compaction"} | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| with tempfile.TemporaryDirectory() as directory: | ||
| from pathlib import Path | ||
| test_run_runner_emits_benchmark_envelope(Path(directory)) | ||
| print("direct_envelope_test_pass") | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| from importlib import import_module | ||
|
|
||
|
|
||
| def test_harness_entrypoint_is_importable(): | ||
| module = import_module("harness.scripts.run_harness") | ||
| assert callable(module.main) | ||
| assert callable(module.run_runner) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Wrapping the dry-run payload by replacing
resultwithadd_envelope(...)drops legacy top-level fields likeresult_codeandplan_hashthat existing consumers and tests still read. Keep those fields in the emitted JSON (for compatibility) or include them in the envelope output before writing the file. [api mismatch]Severity Level: Critical 🚨
Steps of Reproduction ✅
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖