feat(harness): targeted runs, budget guards, lifecycle hooks, observability#13
Merged
Conversation
…oks, observability Implements the confirmed work items from evaluating a live side-effect skill (commit/push/MR workflow) with this harness: - --eval-id on run and validate: filter the suite before task creation, so all artifacts describe only the selected cases; unknown ids fail fast before any model call - per-case budget guards (--max-input/non-cached-input/output/total/ reasoning-tokens-per-case, --max-cost-per-case, --max-duration-per-case) with warn|fail|stop-suite actions, checked post-run - lifecycle hooks: --pre-run-command (aborts suite on failure before any model call), --post-grade-command (stdout JSON merges into grading.json), --post-run-command; all receive structured ASE_* env metadata - live progress lines plus progress.jsonl per-run event stream - summary.json per iteration plus `status` command; report --failures-only - cost semantics: cost_usd null means unavailable (not $0), cost_usd_source tracking, --pricing-config for CLIs that report no cost, OpenRouter reconciliation for claude-code, cached vs non-cached input split - reasoning token persistence (codex turn.completed, opencode step_finish); null means unreported, not zero - --reasoning-effort passed through to codex (model_reasoning_effort); unsupported agents warn and record null - run_meta.json records effective config: model, reasoning effort, base URL, agent CLI version, harness version Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several significant features to the agent-skill-eval harness, including targeted case filtering via --eval-id, per-case budget guards, lifecycle hooks (pre-run, post-grade, and post-run commands), pricing configuration support, live progress logging, and a new status command backed by a unified summary.json artifact. The reviewer's feedback focuses on enhancing the robustness of these additions, specifically by suggesting the use of errors="replace" in subprocess execution and byte decoding to prevent UnicodeDecodeError crashes, checking that the pricing configuration path is a file to avoid IsADirectoryError, and defensively handling potential null values in JSON fields to prevent TypeError exceptions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+50
to
+58
| result = subprocess.run( | ||
| command, | ||
| shell=True, | ||
| capture_output=True, | ||
| text=True, | ||
| encoding="utf-8", | ||
| timeout=timeout, | ||
| env=env, | ||
| ) |
There was a problem hiding this comment.
To prevent UnicodeDecodeError from crashing the harness when a hook command outputs non-UTF-8 data, specify errors="replace" in subprocess.run.
result = subprocess.run(
command,
shell=True,
capture_output=True,
text=True,
encoding="utf-8",
errors="replace",
timeout=timeout,
env=env,
)
Comment on lines
+67
to
+68
| stdout = (e.stdout or b"").decode() if isinstance(e.stdout, bytes) else (e.stdout or "") | ||
| stderr = (e.stderr or b"").decode() if isinstance(e.stderr, bytes) else (e.stderr or "") |
There was a problem hiding this comment.
If e.stdout or e.stderr are bytes, calling .decode() without arguments can raise UnicodeDecodeError if the output contains non-UTF-8 characters. Specify errors="replace" to make this decoding robust.
Suggested change
| stdout = (e.stdout or b"").decode() if isinstance(e.stdout, bytes) else (e.stdout or "") | |
| stderr = (e.stderr or b"").decode() if isinstance(e.stderr, bytes) else (e.stderr or "") | |
| stdout = (e.stdout or b"").decode("utf-8", errors="replace") if isinstance(e.stdout, bytes) else (e.stdout or "") | |
| stderr = (e.stderr or b"").decode("utf-8", errors="replace") if isinstance(e.stderr, bytes) else (e.stderr or "") |
| if not command: | ||
| return None | ||
| try: | ||
| result = subprocess.run(command, capture_output=True, text=True, encoding="utf-8", timeout=15, check=False) |
There was a problem hiding this comment.
Specify errors="replace" in subprocess.run to prevent UnicodeDecodeError from crashing the version probe if the CLI outputs non-UTF-8 characters.
Suggested change
| result = subprocess.run(command, capture_output=True, text=True, encoding="utf-8", timeout=15, check=False) | |
| result = subprocess.run( | |
| command, capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=15, check=False | |
| ) |
Comment on lines
+39
to
+52
| input_tokens = sum(t.get("input_tokens", 0) for t in timings) | ||
| cached = sum(t.get("cached_tokens", 0) for t in timings) | ||
| output = sum(t.get("output_tokens", 0) for t in timings) | ||
| total = sum(t.get("total_tokens", 0) for t in timings) | ||
| reported_input = max(input_tokens, cached) | ||
| # Prefer the per-run value the harness recorded (it knows whether its | ||
| # CLI's input_tokens include cache reads); fall back to the | ||
| # subtraction lower bound only for timing.json files predating it. | ||
| non_cached = 0 | ||
| for t in timings: | ||
| nc = t.get("non_cached_input_tokens") | ||
| if nc is None: | ||
| nc = max(t.get("input_tokens", 0) - t.get("cached_tokens", 0), 0) | ||
| non_cached += nc |
There was a problem hiding this comment.
If any of the token fields are null in the raw JSON files, t.get("input_tokens", 0) will return None, causing a TypeError during summation. Use t.get("input_tokens") or 0 to safely handle both missing and null values.
Suggested change
| input_tokens = sum(t.get("input_tokens", 0) for t in timings) | |
| cached = sum(t.get("cached_tokens", 0) for t in timings) | |
| output = sum(t.get("output_tokens", 0) for t in timings) | |
| total = sum(t.get("total_tokens", 0) for t in timings) | |
| reported_input = max(input_tokens, cached) | |
| # Prefer the per-run value the harness recorded (it knows whether its | |
| # CLI's input_tokens include cache reads); fall back to the | |
| # subtraction lower bound only for timing.json files predating it. | |
| non_cached = 0 | |
| for t in timings: | |
| nc = t.get("non_cached_input_tokens") | |
| if nc is None: | |
| nc = max(t.get("input_tokens", 0) - t.get("cached_tokens", 0), 0) | |
| non_cached += nc | |
| input_tokens = sum(t.get("input_tokens") or 0 for t in timings) | |
| cached = sum(t.get("cached_tokens") or 0 for t in timings) | |
| output = sum(t.get("output_tokens") or 0 for t in timings) | |
| total = sum(t.get("total_tokens") or 0 for t in timings) | |
| reported_input = max(input_tokens, cached) | |
| # Prefer the per-run value the harness recorded (it knows whether its | |
| # CLI's input_tokens include cache reads); fall back to the | |
| # subtraction lower bound only for timing.json files predating it. | |
| non_cached = 0 | |
| for t in timings: | |
| nc = t.get("non_cached_input_tokens") | |
| if nc is None: | |
| nc = max((t.get("input_tokens") or 0) - (t.get("cached_tokens") or 0), 0) | |
| non_cached += nc |
Comment on lines
+299
to
+301
| if not path.exists(): | ||
| console.print(f"[red]Pricing config not found: {path}[/red]") | ||
| raise typer.Exit(1) |
There was a problem hiding this comment.
If path is a directory, path.read_text() will raise an IsADirectoryError. Check path.is_file() to exit cleanly.
Suggested change
| if not path.exists(): | |
| console.print(f"[red]Pricing config not found: {path}[/red]") | |
| raise typer.Exit(1) | |
| if not path.exists(): | |
| console.print(f"[red]Pricing config not found: {path}[/red]") | |
| raise typer.Exit(1) | |
| if not path.is_file(): | |
| console.print(f"[red]Pricing config {path} is not a file[/red]") | |
| raise typer.Exit(1) |
Comment on lines
+865
to
+876
| console.print( | ||
| f"skill={summary.get('skill_name')} iteration={summary.get('iteration')} " | ||
| f"agents={','.join(summary.get('agents', []))} " | ||
| f"eval_ids={','.join(str(e) for e in summary.get('eval_ids', []))}" | ||
| ) | ||
| console.print( | ||
| f"runs: {summary.get('runs_completed', 0)} completed, " | ||
| f"{summary.get('runs_failed_grading', 0)} with failed assertions, " | ||
| f"{len(summary.get('errors', []))} errored, " | ||
| f"{len(summary.get('skipped_runs', []))} skipped; " | ||
| f"duration {summary.get('duration_seconds', 0):.0f}s" | ||
| ) |
There was a problem hiding this comment.
Use summary.get('agents') or [] and summary.get('eval_ids') or [] to prevent TypeError if those fields are null in the JSON. This is a robust defensive programming practice.
Suggested change
| console.print( | |
| f"skill={summary.get('skill_name')} iteration={summary.get('iteration')} " | |
| f"agents={','.join(summary.get('agents', []))} " | |
| f"eval_ids={','.join(str(e) for e in summary.get('eval_ids', []))}" | |
| ) | |
| console.print( | |
| f"runs: {summary.get('runs_completed', 0)} completed, " | |
| f"{summary.get('runs_failed_grading', 0)} with failed assertions, " | |
| f"{len(summary.get('errors', []))} errored, " | |
| f"{len(summary.get('skipped_runs', []))} skipped; " | |
| f"duration {summary.get('duration_seconds', 0):.0f}s" | |
| ) | |
| console.print( | |
| f"skill={summary.get('skill_name')} iteration={summary.get('iteration')} " | |
| f"agents={','.join(summary.get('agents') or [])} " | |
| f"eval_ids={','.join(str(e) for e in summary.get('eval_ids') or [])}" | |
| ) | |
| console.print( | |
| f"runs: {summary.get('runs_completed') or 0} completed, " | |
| f"{summary.get('runs_failed_grading') or 0} with failed assertions, " | |
| f"{len(summary.get('errors') or [])} errored, " | |
| f"{len(summary.get('skipped_runs') or [])} skipped; " | |
| f"duration {summary.get('duration_seconds') or 0:.0f}s" | |
| ) |
Comment on lines
+902
to
+911
| for failed in summary.get("failed_runs", []): | ||
| config = "with_skill" if failed.get("with_skill") else "without_skill" | ||
| console.print( | ||
| f" [red]FAILED {failed.get('eval_id')}/{failed.get('agent')}/{config}: " | ||
| f"{', '.join(failed.get('failed_assertions', [])) or 'see grading.json'}[/red]" | ||
| ) | ||
| for skipped in summary.get("skipped_runs", []): | ||
| console.print(f" [yellow]SKIPPED {skipped.get('run')}: {skipped.get('reason')}[/yellow]") | ||
| for error in summary.get("errors", []): | ||
| console.print(f" [red]ERROR {error.get('run')}: {error.get('error')}[/red]") |
There was a problem hiding this comment.
Use summary.get("failed_runs") or [], summary.get("skipped_runs") or [], and summary.get("errors") or [] to prevent TypeError if those fields are null in the JSON.
Suggested change
| for failed in summary.get("failed_runs", []): | |
| config = "with_skill" if failed.get("with_skill") else "without_skill" | |
| console.print( | |
| f" [red]FAILED {failed.get('eval_id')}/{failed.get('agent')}/{config}: " | |
| f"{', '.join(failed.get('failed_assertions', [])) or 'see grading.json'}[/red]" | |
| ) | |
| for skipped in summary.get("skipped_runs", []): | |
| console.print(f" [yellow]SKIPPED {skipped.get('run')}: {skipped.get('reason')}[/yellow]") | |
| for error in summary.get("errors", []): | |
| console.print(f" [red]ERROR {error.get('run')}: {error.get('error')}[/red]") | |
| for failed in summary.get("failed_runs") or []: | |
| config = "with_skill" if failed.get("with_skill") else "without_skill" | |
| console.print( | |
| f" [red]FAILED {failed.get('eval_id')}/{failed.get('agent')}/{config}: " | |
| f"{', '.join(failed.get('failed_assertions') or []) or 'see grading.json'}[/red]" | |
| ) | |
| for skipped in summary.get("skipped_runs") or []: | |
| console.print(f" [yellow]SKIPPED {skipped.get('run')}: {skipped.get('reason')}[/yellow]") | |
| for error in summary.get("errors") or []: | |
| console.print(f" [red]ERROR {error.get('run')}: {error.get('error')}[/red]") |
Comment on lines
+913
to
+914
| budget = summary.get("budget") or {} | ||
| for exceeded in budget.get("exceeded_runs", []): |
There was a problem hiding this comment.
Use budget.get("exceeded_runs") or [] to prevent TypeError if "exceeded_runs" is null in the JSON.
Suggested change
| budget = summary.get("budget") or {} | |
| for exceeded in budget.get("exceeded_runs", []): | |
| budget = summary.get("budget") or {} | |
| for exceeded in budget.get("exceeded_runs") or []: |
Comment on lines
+921
to
+923
| cleanup_info = summary.get("cleanup") or {} | ||
| branches = cleanup_info.get("remote_branches", []) | ||
| prs = cleanup_info.get("pr_numbers", []) |
There was a problem hiding this comment.
Use cleanup_info.get("remote_branches") or [] and cleanup_info.get("pr_numbers") or [] to prevent TypeError if those fields are null in the JSON.
Suggested change
| cleanup_info = summary.get("cleanup") or {} | |
| branches = cleanup_info.get("remote_branches", []) | |
| prs = cleanup_info.get("pr_numbers", []) | |
| cleanup_info = summary.get("cleanup") or {} | |
| branches = cleanup_info.get("remote_branches") or [] | |
| prs = cleanup_info.get("pr_numbers") or [] |
…feedback) - errors="replace" on all subprocess output decoding (hooks, agent run, CLI version probe) so non-UTF-8 output can't crash the harness - treat present-but-null JSON fields as defaults in summary token totals and the status command - reject a directory passed as --pricing-config Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Summary
Implements the confirmed work items from a real-world evaluation of this harness against a live side-effect skill (commit/push/MR workflow). Full live suites there cost 12–20 min and 1–2M reported input tokens per iteration; this batch makes targeted, budgeted, well-reported runs the default path.
Case selection
run --eval-id/validate --eval-id(repeatable): filter the suite before task creation soevals_meta.json, workspaces, and benchmarks naturally describe only the selected cases. Unknown ids fail fast, listing available ids, before any model call.Budget guards
--max-input-tokens-per-case,--max-non-cached-input-tokens-per-case,--max-output-tokens-per-case,--max-total-tokens-per-case,--max-reasoning-tokens-per-case,--max-cost-per-case,--max-duration-per-case--budget-action warn|fail|stop-suite—failappends a failedmethod: "budget"assertion;stop-suiteadditionally skips runs not yet started (stops a live side-effect suite before more remote state is created)Lifecycle hooks
--pre-run-command: setup/preflight; non-zero exit aborts the suite before any model call--post-grade-command: per-run external grader; stdout JSON[{text, passed, evidence}]merges intograding.jsonasmethod: "hook"results--post-run-command: teardown; failures recorded but don't fail the runASE_*env metadata (workspace, state files, timing, grading paths)Observability
progress.jsonlevent stream (tail -f a long run)summary.jsonper iteration + newstatuscommand (pass rates, failures, token/cost totals, budget verdicts, cleanup state — no model call);report --failures-onlyCost & token semantics
cost_usd: null= unavailable, not $0; stats computed only over runs that reported costcost_usd_sourcetracking (cli/openrouter-pricing/pricing-config/cli-unreconciled); OpenRouter reconciliation for claude-code list-price estimates--pricing-configfor CLIs that report no cost (codex)timing.jsonpersistsnon_cached_input_tokens(per-CLI semantics: codex/opencode input includes cache reads, claude-code doesn't) andreasoning_output_tokens(codexturn.completed, opencodestep_finish; null = unreported, not zero)Reproducibility
--reasoning-effortpassed through to codex (model_reasoning_effort); without it codex silently inherits~/.codex/config.toml, so two machines can benchmark different settings under the same command line. Unsupported agents warn and record null.run_meta.jsonnow records the effective config: model, reasoning effort, base URL, agent CLI version (probed once per suite), harness versionOut of scope (deliberately)
--pre-run-commandor environment-specificTest plan
pytest -m "not live": 309 passedruff check+ruff format --checkcleanmake test-livebefore merge (per repo workflow)🤖 Generated with Claude Code