Fix benchmark trace publication

[rgarcia]

summary

• publish useful Braintrust traces with inputs, structured tool turns, phase timelines, browser lifetime, and correct cached-token accounting
• redact typed values, credentials, private-info output, and browser identifiers, then validate the complete payload before publication
• fail before Harbor starts when PurelyMail cannot create a disposable account, and retry isolated setup failures
• classify ungraded step failures as infrastructure instead of task outcomes

validation

• bun test (281 tests)
• bunx tsc --noEmit
• bunx prettier --check ...
• bash -n benchmarks/harbor/clawbench/run.sh
• go run github.com/rhysd/actionlint/cmd/actionlint@v1.7.7 .github/workflows/benchmark-clawbench.yml
• live Braintrust single-trial publication: verified phase spans, non-empty LLM inputs, positive turn durations, exact cached-token cost, redaction, and session-match diagnostics; deleted the temporary experiment afterward
• local PurelyMail preflight correctly rejects the currently invalid token before creating a Harbor dataset

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
mcp	Ready	Preview	Oct 1, 2026 1:37am UTC

[cursor]

Stale Bugbot comment from a previous run.

[vercel]

The TYPED_CALL regex captures .fill()/.type() arguments with [^)]*, which truncates at the first ), so form values containing ) (e.g. strong passwords) or nested-call selectors are neither redacted nor caught by the fail-closed assertSafeToPublish guard, leaking secrets to Braintrust.

[rgarcia]

Fixed in 8f7a8f2. The redactor now balances nested call parentheses while treating quoted and multiline template contents atomically, so ) inside selectors or typed values cannot truncate the scan. The independent publish assertion rejects the same unredacted cases.

[bmsaadat]

Review

Verdict: Request changes (two fixes; direction is right)

The trace shape is a real improvement: inputs on llm spans, phase timeline, browser lifetime, cached-token metrics. The PurelyMail preflight is the right fix for the setup-failure class, and the step-level exception classification matches how harbor 0.21.0 records step failures (multi_step.py stores them on the step and the trial completes normally). Verified locally at 2586a70: 281 tests, tsc --noEmit, prettier, and bash -n run.sh all pass. I also checked the new fields and retry names against the pins (harbor 0.21.0, harbor-hypeman 0.1.2, kernel/ClawBench c7feaa2) rather than the PR text.

Requested changes

1. A ) inside a typed value defeats both the redactor and the safety assert. TYPED_CALL captures with [^)]*, so the argument list is cut at the first ), the selector gets redacted, and the value is published. assertSafeToPublish re-runs the same truncated scan, so it passes. Reproduced at head:

   page.fill('#password', 'Str0ng)Pass!')  -> page.fill('[REDACTED]', 'Str0ng)Pass!')   assert passes
   page.locator('#pw').fill('abc)def')     -> unchanged                                 assert passes
   

   Multi-line template literals, pressSequentially, and keyboard.insertText are also uncovered. The benchmark's own password is token_urlsafe so it cannot trigger this; it needs an agent-invented value, which is what the 38 sign-up tasks and 2 password-manager tasks produce. Suggest tokenizing the argument list to find the matching close paren, adding pressSequentially|insertText, letting the literal regex span newlines, and giving the assert a check that does not share typedCallValues, so fail-closed stays independent of the redactor.

2. Row-level cost is gone, and the per-span replacement is uneven across agents. metricRecord dropped cost_usd and the token counts from the root row (main published harbor's agent_result.cost_usd, which both converters set). The replacement reads step.metrics.cost_usd on llm spans: harbor's Codex converter sets it per API call (litellm estimate), but the Claude Code converter never sets it per step (_build_metrics writes cost_usd=None; only final_metrics.total_cost_usd and agent_result.cost_usd are set), so a Claude Code run publishes no cost anywhere. estimated_cost is Braintrust's documented cost field. Suggest keeping row-level metrics under canonical names (prompt_tokens = n_input_tokens, which harbor documents as cache-inclusive; prompt_cached_tokens = n_cache_tokens; completion_tokens; tokens; estimated_cost = cost_usd) and emitting estimated_cost on llm spans only when ATIF carries it. The synthetic fixture puts cost_usd on the step, which is what hid the Claude Code gap.

Non-blocking

• Private-info harvesting scrubs ordinary words out of the whole trace. privateInfoValues collects every JSON string value of 4+ chars from a my-info read (172 unique values from the pinned alex_green_personal_info.json) and substring-replaces them across every string in the trial. With the real values, {"name":"Email"} <button>Close Window</button> "Single sign-on" {"width":1208} becomes {"name":"[REDACTED]"} <button>Close [REDACTED]</button> "[REDACTED] sign-on" {"width":[REDACTED]}. The fake IDs and account numbers are worth scrubbing; the names and common words are not. Harvesting only from sensitive-shaped keys would keep the protection and the readability.
• --retry-include RuntimeError does not retry step setup failures. harbor's retry loop returns as soon as the trial-level exception_info is None, and a failed setup.sh is stored on the step (multi_step.py:_run_step_setup) while the trial completes normally, which is exactly the shape the new fixture models. AgentSetupTimeoutError is raised at trial level and does retry. The README line should say agent setup only; the preflight is the real fix for the setup.sh class. Matching is by exact class name, so what RuntimeError now catches is harbor-hypeman's bare raises, transient and deterministic alike; a named exception for the transient paths in harbor-hypeman would let us drop the bare include.
• LLM-typed spans carry tool time. ATIF has one timestamp per step, so the turn interval includes the neighbouring tool execution, which dominates browser tasks, and Braintrust charts type: "llm" as model latency. The metadata label is honest; naming the span turn would make it visible in the UI.
• llmInput publishes only the previous agent step. prior.slice(previousAgent, previousAgent + 1) is a one-element slice, so user/system steps between two agent turns are dropped despite the README's "preceding context". prior.slice(previousAgent) gives the claimed behavior; the fixture has one agent step so the branch is untested.
• Fail-closed publish with no locator. assertSafeToPublish runs after all arms are built and throws a generic message with no trial, span, or field, so with the CI gate a red publish is a red multi-hour run debuggable only locally. Keep fail-closed, add the event id and key path to the error.
• Set-Cookie object keys are no longer redacted. SENSITIVE_FIELDS holds "set-cookie" but normalizedField rewrites - to _ first, so it never matches (main caught ^SET-COOKIE$). Small exposure since object keys only come from tool arguments and the string-level cookie rule still covers header text, but it shows the key vocabulary is now hand-copied into five lists that already disagree (the assert omits token, the collector knows five names, the URL regex lacks four). One exported list with a pattern builder would prevent the next drift.
• Smaller: insert batches are count-only (100 events) while llm inputs now carry the previous step's observations, so a byte bound would be cheap insurance; llm/tool span IDs now key on trajectoryIndex, so re-publishing an already-published job dir leaves the old spans orphaned (rows are still replaced); harvested secrets apply to ATIF spans only, not the root row's instruction/error; the preflight runs once per arm and a failed deleteUser leaves a cbpreflight… account behind; sources[arm].stats.nErroredTrials still comes from harbor's trial-level count, so it reads 0 for an arm that benchByArm.infraErrors says had step failures.

Question

• The README previously kept task instructions off experiment rows; this publishes the redacted last user message as input.instruction. ClawBench's instruction is the public prompt plus browser rules, the my-info file list, and extra_info names and descriptions, so it looks fine. Just confirming the policy change is deliberate.

[rgarcia]

Addressed the requested changes in 8f7a8f2 after merging current main:

• hardened typed-value redaction for balanced/nested calls, ) characters, multiline templates, pressSequentially, and keyboard.insertText, with an independent fail-closed assertion
• restored canonical row metrics (prompt_tokens, prompt_cached_tokens, completion_tokens, tokens, estimated_cost) and uses estimated_cost on ATIF spans when present
• added coverage proving row-level cost remains available when ATIF has no per-turn cost

Validated with 769 tests, tsc --noEmit, targeted Prettier, shell syntax, and git diff --check.

[cursor]

Stale Bugbot comment from a previous run.

[bmsaadat]

Re-review

Verdict: Request changes (one regression; everything else looks good)

Thanks for the thorough follow-up. Verified at 4767860: row metrics now match harbor 0.21.0 (step_results[i].agent_result, cache-inclusive n_input_tokens, Claude Code's total_cost_usd), the set_cookie and *_secret fixes work, and every typed-value case from round 1 is covered. Tests and tsc pass locally.

Blocking

The new typed-value scanner skips calls that follow an unmatched quote. typedLiterals treats everything between quotes as one unit at the top level, so an apostrophe outside a string (e.g. in a comment) opens a fake literal that hides the .fill( call. assertTypedCallsRedacted uses the same approach, so the fail-closed check passes too. I ran these through buildExperimentEvents with the fixture trajectory:

Input	2586a70	4767860
execute_playwright_code: // Fill in the user's password\nawait page.locator('#password').fill('Hunter2Pass')	redacted	published
execute_playwright_code: /* we'll sign up now */\nawait page.fill('#password', 'Hunter2Pass')	redacted	published
exec_command: node -e 'await page.fill("#pw", "Hunter2Pass")'	redacted	published

Suggest:

• Start a match at every .fill(-family occurrence regardless of the surrounding text (as round 1 did), and keep the balanced-paren/quote-aware scan only inside the argument list.
• Add the cases above as regression tests.
• Give the assert a different detection strategy rather than a copy of the tokenizer (the two functions are ~70 near-identical lines), so a scanner bug can't slip past both.

Non-blocking (fine as follow-ups)

These round-1 notes are unchanged. A quick reply or ticket on each would be enough:

• privateInfoValues still harvests every 4+ char value, so common words get scrubbed trace-wide.
• The README retry line still implies step setup.sh failures are retried.
• llmInput still uses prior.slice(previousAgent, previousAgent + 1) (only the previous agent step).
• Typed-value and sensitive-value assert errors still carry no trial/span/field locator.
• The sensitive-key vocabulary is still split across several lists (the assert lacks token; the collector knows five names).

[rgarcia]

Addressed the re-review in ac21ec1:

• typed-call redaction now starts at every .fill/.type/pressSequentially/insertText occurrence, including calls after apostrophes in comments and shell-quoted code
• the fail-closed assertion now independently parses each call with the TypeScript AST instead of duplicating the redaction tokenizer
• private-info harvesting keeps ordinary labels/numbers and collects only sensitive-key values
• corrected the README retry semantics for step setup.sh failures
• fixed multi-turn LLM context to include user/system steps after the previous agent turn
• assertion errors now include the Braintrust event ID and nested field path
• string redaction, collection, and assertion now share the same sensitive-field classifier

Added regression coverage for all three reported inputs and each follow-up. Validated with 772 tests, tsc --noEmit, targeted Prettier, shell syntax, and git diff --check.

[cursor]

Stale Bugbot comment from a previous run.

[bmsaadat]

Re-review

Verdict: Approve with fixes. Two small regressions in the new shared classifier; please land these before merging. Everything else is fixed.

Verified at 8975eb9: the three typed-call inputs from last round are redacted in buildExperimentEvents, the AST-based assert rejects them on its own and didn't false-positive on any redacted shape I tried, and the README retry wording, llmInput context, and error paths are all fixed. Tests and tsc pass.

Please fix before merging

Both come from SENSITIVE_ASSIGNMENT, which the redactor, collector and assert share, so the fail-closed check passes too. Results through buildExperimentEvents with the fixture trajectory:

Input	4767860	8975eb9
Playwright result JSON: {"success": true, "result": "Account created.\nTemporary password: Hunter2Pass"}	redacted	published
Result text cut off before its closing quote: {"text": "Step 1...\n (~30 \n escapes) ...password: Hunter2Pass	redacted	published

1. A quoted value under a non-sensitive key hides sensitive pairs inside it. The match consumes the whole "result": "..." value and moves on, so password: ... inside it is never checked. execute_playwright_code returns JSON text, so this is the common case.
2. The quoted-value body (?:\\.|(?!\4)[\s\S])* is ambiguous (both branches match \). On an unterminated value it backtracks exponentially: from ~22 escapes Bun silently stops matching from that point on (later pairs aren't redacted), and just below that it takes ~2s per string. V8 takes seconds and keeps growing.

Fix I tried locally (both rows then redact, existing tests pass):

• body (?:\\[\s\S]|(?!\4)[^\\])*
• in the replace callback, when the key isn't sensitive and the value is quoted, recurse into the value instead of returning the match; do the same in assertSafeString

Worth adding both rows as regression tests.

Non-blocking

• The narrower private-info harvest now collects 6 of 246 string fields from alex_green_personal_info.json, so the card numbers, passport/licence/health-card numbers, SIN and transit numbers are no longer scrubbed if the agent types them elsewhere. The data is a public synthetic persona, so it's just hygiene; key-shaped matches such as number, sin or transit_number under government_ids/financial would bring them back.

[rgarcia]

Addressed the latest review in 3538a03:

• made quoted assignment matching linear-time by making escape consumption unambiguous
• recursively redacts and independently asserts sensitive pairs embedded inside quoted non-sensitive values
• added regressions for nested Playwright result JSON and unterminated escaped output
• also handled the non-blocking private-info note by collecting number, number_formatted, sin, and transit_number under financial, government ID, and insurance sections without restoring broad common-word harvesting

Validation: 773 tests, bunx tsc --noEmit, targeted Prettier, shell syntax, and diff checks all pass.

[cursor]

Stale Bugbot comment from a previous run.

[cursor]

Stale Bugbot comment from a previous run.

[cursor]

Stale Bugbot comment from a previous run.

[cursor]

Stale Bugbot comment from a previous run.

[bmsaadat]

Re-review

Verdict: Approve. One follow-up worth doing soon; it fails closed, so it's not a blocker.

Thanks for the fast turnaround. Verified at 99d98e3: every leak case from earlier rounds is redacted in buildExperimentEvents (including camelCase inside JSON-encoded results), the scanner is linear, the private-info IDs are harvested again without the common words, and tests, tsc, CI and Bugbot are green.

Follow-up

A normal login-page ariaSnapshot() makes the publish throw. Real Playwright output for a form with Email: / Password: labels:

- text: "Email:"
- textbox "Email:"
- text: "Password:"

Wrapped the way execute_playwright_code returns it, buildExperimentEvents throws data after a redacted field value (it published at 8975eb9). The scanner reads "Email:" as a sensitive key followed by a quoted value, swallows the text up to the next quote, and the trailing-data rule rejects what's left. The same happens with prose like Password: don't reuse one from another site, where the value stops at the apostrophe.

Nothing leaks, and the report still renders, but that run loses its Braintrust traces and the benchmark check goes red. Since job dirs aren't uploaded, it can't be republished afterwards.

Suggest treating a sensitive key whose own quoted string closes right after the separator ("Password:", escaped or not) as label text with no value, not ending unquoted values at an in-word apostrophe, and adding both examples as must-publish regression tests. (Dropping the trailing-data rule alone breaks your escaped-quote truncation test, so the fix belongs in the scanner.)

Non-blocking (not new in this PR)

• A few formats still leak: Go :=, YAML | blocks, HTML value= attributes, markdown table cells.
• If the 20k/400-char truncation cuts through password: [REDACTED], the check throws for the whole publish.

[rgarcia]

Addressed the approved follow-up in the latest commit:

• treats quoted Email: / Password: snapshot labels, including escaped labels, as label text rather than assignments
• keeps in-word apostrophes inside unquoted sensitive values, so don't is redacted without leaving trailing text that fails publication
• adds direct redaction coverage and a full buildExperimentEvents must-publish regression using wrapped execute_playwright_code output

Validation: 775 tests, bunx tsc --noEmit, targeted Prettier, shell syntax, and diff checks pass. Per request, no re-review was requested.

[cursor]

Stale Bugbot comment from a previous run.

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit e4632cc. Configure here.}