docs(bench): add pr-agent-bench skill (how-to-read runbook) - #218
Draft
hfurkanbozkurt wants to merge 40 commits into
Draft
docs(bench): add pr-agent-bench skill (how-to-read runbook)#218hfurkanbozkurt wants to merge 40 commits into
hfurkanbozkurt wants to merge 40 commits into
Conversation
added 16 commits
July 16, 2026 23:51
… Overview/Δ column Collapse the two-table report (Overview glyphs + Detailed numbers) into ONE results table. Each metric cell now shows the current value on line 1 and the signed delta vs the main baseline on line 2 (via <br>), colored by the significance+direction of the change: - green/red beyond a per-metric threshold, yellow within the noise band, ⚪ when the baseline has no value for that field (value still shown, tagged (new)). - thresholds centralized in DELTA_THRESHOLDS: composite/score ±5, judge ±0.3, tests ±1, cost ±10%, tokens ±10%; direction per metric (tests/judge/score up, cost/tokens down). No arrows — color only. Drops the whole-row schema-2 gate (baselineHasMetrics/perMetricBaseline): the baseline is diffed PER FIELD, so a field the baseline lacks degrades to ⚪ (new) for that cell only instead of forcing the entire row to 🆕. Removes renderOverview and the separate Δ-vs-base column. Presentational only — buildAggregate/ diffAgainstBaseline data shape (current/delta) unchanged, so analyze.mjs is unaffected.
Harden renderDetailed so a crashed/errored cell (0 tests, no tokens → null cost/score, no judge) can never drop a metric column and misalign the row: build the 5 metric cells into an array and coerce any empty string to the NONE placeholder before joining, guaranteeing the 8-column invariant structurally even if a future cell helper returns ''. Add a crashed-cell regression test that asserts every rendered row has exactly 8 columns with no empty cell. Presentational only.
…judge_dimensions is aggregate-only Sync the docs to the shipped single-table render: - README: replace the two-table (Overview colors-only + Detailed baseline->pr) description with the one results table (renderDetailed) — each metric cell is current value + colored signed delta (two lines via <br>), ⚪ (new) when the baseline lacks that field, value always shown. Swap MARGIN_PCT/metricColor/ renderOverview for DELTA_THRESHOLDS + deltaColor and list the per-metric bands and directions. Drop the baseline->pr / multi-line per-dimension Judge-cell wording from Re-derivability. - overview.mjs: one-line comment that cellMetrics.judge_dimensions is carried for the persisted S3 aggregate / re-derivability only and is NOT consumed by the rendered table (behavior unchanged). - changeset: refresh the body to describe the single table + baked-in per-cell signed delta + DELTA_THRESHOLDS coloring; still non-releasing. Docs + one comment only — no functional code change.
…C/files cols, inline delta, preword Render (overview.mjs, summary.mjs): - Scalar cells (tests/cost/turns/score) now inline: <ball> <value> (<Δ>), dropping the two-line <br> form. - JUDGE column stacks one <br>-joined line per rubric dimension (functional/selectors/persistence/code/blocks) with per-dim baseline deltas; overall judge folded into the preword. - TOKENS column stacks in / out / cached in / cached out; cache lines render ⚪ '(new)' until a baseline carries them. - New columns: Turns (cycle_count, lower-better), LOC (created/edited), Files (created/edited). - LOC & Files are NEUTRAL — always ⚪, value + signed delta, never green/red (no inherent good/bad direction). Cache tokens are DISPLAYED only, not deducted from cost/SCORE. - New DELTA_THRESHOLDS bands: turns ±3, cache read/write ±20%. humanTokens now compacts ≥1M to 'M'. - renderPreword: bulleted run summary (mean composite + Δ vs main, verdict counts, totals, biggest gains/drops, config). Replaces the old headline. - N-column guard extended to 11 columns; every row (incl. crashed/removed) emits all 11. Aggregate (overview.mjs buildAggregate/cellMetrics): - Persist cycle_count, cache_read_tokens, cache_write_tokens, loc_created/edited, files_created/edited so future baselines carry them. Instrumentation (2-agent-run.ts, partial-envelope.mjs): - Accumulate usage.cacheReadInputTokens/cacheWriteInputTokens in the ModelStreamUpdate hook; persist cache_read/write_tokens on every envelope path (checkpoint, partial, error, success), success taking max(hook, winnerUsage). - workspace-diff.mjs: snapshot the scaffolded workspace before the agent runs via a throwaway external GIT_DIR (never touches the workspace's own .git), diff after for LOC/files churn on the success path. Degrades to null on any failure. Tests: overview.test.mjs updated for the 11-column layout (per-dim judge, 4-line tokens, turns/LOC/files, ⚪ '(new)', 11-column guard, M formatting); workspace-diff.test.mjs added (real git against a real temp workspace + pure parseChurn). tsc clean; 182 pass / 4 skipped (isolation tests need unshare).
…ead-server classify, render guards, log scrub
- truthy(v) helper in analysis.mjs mirrors scoring.mjs; isFailureCell + analyze-cell
now treat GITHUB_OUTPUT string bools ("false"/"true") correctly, so the deep
failure pass receives build-log + dev-server evidence on genuine build failures.
- overview.mjs judgeCell renders every common dimension unconditionally (null -> ⚪ —)
so stacked judge lines stay row-aligned across cells.
- render guards: token totals show NONE (not 0 in/0 out) for all-failed runs;
removed-row omits (was …) when the baseline test denom is null; cost/token/cache
delta bands floor at an absolute threshold (costAbs/tokensAbs) so a ~0 baseline
can't over-color; humanTokens/fmtCost/fmtScore return NONE for non-finite input.
- 3-build-and-test.sh: when the dev server never comes up (APP_BASE_URL empty),
skip Playwright instead of launching with an empty BLOCKS_URL, and record a
distinct dev_server_status=dead signal. Semantics change: a dead server now reads
verdict 'unknown' (EXCLUDED from the mean) rather than a masked composite-0 fail.
Also rm -f stale /tmp evidence before staging fresh copies.
- agent-bench.yml: analyze-cell timeout 6 -> 20 min (Bedrock retry budget).
- security: scrub AWS/GitHub credential patterns from dev/build log tails before
they enter the Bedrock prompt.
- analysis.mjs extractFailingTests caps each spec title at 120 chars.
- extend overview.test.mjs + analysis.test.mjs for all of the above.
…ean) — stop masking backend crashes
…ale logs, never abort under set -e (fixes false build-fail from 2d53028)
…ace analysis confabulation - scoring: rename AGENT_HARNESS_TEARDOWN_REASON -> AGENT_HARNESS_TIMEOUT_REASON (value 'agent_harness_teardown' -> 'wall_clock_timeout'). Under active isolation an ungraceful step-2 death cannot be an agent pkill-storm (cross-uid EPERM), so it is an infra wall-clock timeout, not a teardown. Exclusion behavior unchanged (still harness_error, still excluded). - analysis: add pure deterministicCellAnalysis + NO_TRACE_ANALYSIS; when a cell has no agent trace (timeout path writes none) emit a deterministic 'undetermined' note instead of letting the model confabulate an owner/cause. - analyze-cell: route through deterministicCellAnalysis. - tests: scoring.test + analysis.test cover both changes.
…tic and add email-digest scope-hygiene note
…ability falsy-echo and oidc unauthenticated probe
…rrors (all 10 specs)
…staging (EACCES), judge rubric recognizes real auth APIs / #185 footgun
… compact Judge to one line The single-table agent-bench comparison report was too verbose. Reduce it: - Remove the Tokens, LOC, and Files columns (8 cols now: Task|Template|Tests| Judge|Cost|Turns|Score|Stop reason). buildAggregate still persists the token/cache/LOC/file counts to the S3 baseline for offline analysis — only the rendered columns are dropped. - Compact Judge to ONE line: overall score + per-dimension shorthand (F/S/P/C/B) with per-dim signed deltas, instead of one line per dimension. - Update the glossary (summary.mjs), the README table description, the changeset, and the overview.test.mjs snapshot expectations to match. - Fix a latent $-escaping bug in signedCost surfaced while removing signedTokens.
Fixes 3 harness/analysis bugs that let the bench silently score, exclude, or misattribute cells for the harness's own failures (cognito-profile MaxTokensError mislabeled a timeout; kb-chat ungraceful kill given a fabricated "build failed"). - 2-agent-run.ts: persist trace.json on EVERY exit path via a per-turn MessageAddedEvent accumulator (captures toolResult blocks, not just toolUse), so a non-returning invoke() (MaxTokensError / wall-clock kill) is still analyzable. Factored pure buildTraceArtifact() into partial-envelope.mjs. - 2-agent-run.ts: self-imposed internal deadline (AGENT_DEADLINE_SEC, ~33.5min, < the 35min GH step timeout) proactively flushes a graceful partial envelope + trace; agent-bench.yml execs tsx so signals reach the process. - scoring.mjs: MaxTokensError -> klass_reason='max_tokens' (kept agent_fail / composite 0 / INCLUDED), no longer mislabeled 'agent_timeout'. - analysis.mjs + analyze-cell.mjs: deterministicFailureAnalysis() short-circuits the model for harness_error / no-trace / failed_at==='2-agent' (no confabulation); buildEvidenceObserved() stops reporting seeded pessimistic build defaults from a skipped step-3 as observed build failures. - maxTokens left at the SDK default (64000); tests added for classify + honesty guard + trace-artifact.
This was referenced Jul 17, 2026
added 5 commits
July 17, 2026 20:43
…tric; fix renamed-file churn parsing
- README: drop stale tokens ±10% threshold clause (turns ±3 now); tokens->turns in lower-better list and results-table description (tokens still persisted for offline analysis)
- workspace-diff parseChurn: normalize inline 'old => new' and '{old => new}' brace numstat rename paths to the new path before the name-status lookup so renamed-file churn isn't silently dropped
- add parseChurn tests for both inline-arrow and brace rename numstat shapes
…inverted agent deadline - analysis.extractFailingTests firstError: use truthiness (||) not nullish (??) so an empty top-level error.message falls through to errors[0].message instead of reporting '(no error message captured)' - 2-agent-run: startup guard warns (stderr) if BENCH_AGENT_DEADLINE_SEC isn't comfortably below the agent step's hard timeout-minutes:35, so the internal deadline can't silently invert past GitHub's SIGKILL - add analysis test for the empty error.message -> errors[0] fall-through
…or slug - trace caveat: only an ungraceful GitHub SIGKILL (hard timeout-minutes:35 kill before the graceful flush) writes no trace.json; the internal-deadline path (BENCH_AGENT_DEADLINE_SEC, stop_reason wall_clock_timeout) now flushes a message-only trace, so timed-out cells stay analyzable; genuinely trace-less => stop_reason is the answer (infra -> re-run) - fix intra-doc anchor: change em-dash to colon in the 'Reference:' header so its GitHub slug is single-hyphen, matching the existing [reference](#reference-what-the-current-v1-report-contains) link
🦋 Changeset detectedLatest commit: 686a576 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…er are counted (INCLUDED), only harness_error is excluded Step 2 previously bucketed dead_server, wall_clock_timeout, max_tokens and agent_timeout as 'infra/harness noise -> NOT a PR signal -> re-run', contradicting scoring.mjs (isCountedFailKlass: agent_fail + dead_server are INCLUDED as composite 0) and the SKILL's own Reference section. Rewrote the triage table into 3 classes matching scoring.mjs exactly: harness_error (EXCLUDED, re-run/not a PR signal), agent_fail + dead_server (INCLUDED composite-0 counted failures that moved the mean; re-run only to disambiguate transient-vs-real), scored (INCLUDED, graded). Added a 'trap to avoid' note and corrected the worked-example cognito bullet to call the max_tokens 0 a counted agent_fail, not excluded noise.
… F/S/P/C/B shorthand from table The single-table report's Judge column now shows only the averaged judge score + color ball + delta (e.g. `🟡 8.2 (0)`). The per-dimension shorthand tail (` · F.. S.. P.. C.. B..`) is no longer rendered in the table cell. Per-dimension judge data is untouched: judge scoring (scoring.mjs) and the per-dim persistence in buildAggregate/artifacts remain — the breakdown lives in the judge artifact JSON (progressive disclosure), not the at-a-glance row. Removed now-dead render code: DIM_SHORT map + the COMMON_DIMENSIONS import in overview.mjs. Updated overview.test.mjs to assert the Judge cell has ball+score+ delta and NO per-dim shorthand.
…/B live in judge artifact JSON, not the row
hfurkanbozkurt
marked this pull request as ready for review
July 19, 2026 20:35
… docs The JUDGE cell renders the overall judge score + signed delta only (judgeCell in overview.mjs); the per-dimension breakdown lives in the judge artifact JSON. Update the summary glossary, README, and changeset to match shipped behavior — no more 'per-dimension shorthand' claim.
…ess-integrity Brings in the #216 review-nit doc fixes (drop stale F/S/P/C/B per-dimension render claims from summary.mjs, README.md, and the changeset). Additive merge; no history rewrite.
…nt-bench-skill Brings in the #216 review-nit doc fixes (via the harness-integrity forward-merge). Additive merge; no history rewrite.
dev_server_dead sat under a column headed 'Signal (stop_reason / status)' but it is a klass_reason, not a stop_reason. Broaden the heading to 'stop_reason / klass_reason / status' so the dead_server cell is labeled accurately, consistent with the Reference section's stop_reason enumeration (which correctly omits dev_server_dead).
hfurkanbozkurt
marked this pull request as draft
July 24, 2026 21:18
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.
PR 3 of 3 — split of #194. Stacked on PR 2 (base branch
bench/harness-integrity).Concern: docs only. Adds
.agents/skills/pr-agent-bench/SKILL.md— a passive skill / runbook for reading and interpreting an aws-blocks PR agent-bench run: what the report's columns, colored balls (🟢 ⚪ 🟡 🔴), Judge dimensions (F/S/P/C/B), composite/Score, andstop_reasonvalues mean, plus a 60-second regression-triage runbook.Single commit
ab85029; the diff touches only that one new file (+187). No code, no published@aws-blocks/*package changes.Base:
bench/harness-integrity(PR 2).Stack (bottom → top): #216 ← #217 ← #218 · supersedes #194.
Supersedes #194.