Repository navigation
Conversation
A task.json `useful` key (`{"command": regex}` for a task graded by its
target tests, `{"edit": glob}` for one graded by an edit to a target file)
names the event that is the run's first useful result. result.json `useful`
records its seconds (the event's issue time) and steps (events up to and
including it), or why there is none (never, unobserved, untimed,
not_declared). repo report shows the observed count and medians per row;
repo compare pairs the per-task medians as first_useful_s and
first_useful_steps. A malformed value is refused by repo validate, tasks
check and repo run. Batches recorded before this have no such rows.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…dian_s Review of the first-useful change before its first push: result.json names the field first_useful (the task.json key is the definition, the result is the hit), its time field is issued_s (the call's issue time, as the repo's other times end in _s), and the report summary's median is median_s. A refusal test drives repo run with a malformed value. The design note says what the medians cover and that the compare means pool the tasks of both kinds. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The medians of the first useful result cover only the cells that hit it, so an arm that rarely reaches it looked no slower. repo compare now pairs first_useful_rate per task (observed over declared cells) and its header counts observed of declared cells per arm. The per-task values move to compare_values.py to keep compare.py under its 800-line cap. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
An Edit whose tool result is an error left the file unchanged, but it counted as the first useful edit at its issue time. The event now carries rc 1 on that error, as a command does, and hits() refuses it. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…ul regex is refused Copilot edit events never read the completion, so a failed edit counted as a first useful result. A regex nested deeply enough to overflow the parser raised RecursionError out of parse_useful instead of UsefulSpecError. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
parse_useful caught re.error and RecursionError only, so a repeat count the regex parser cannot hold escaped as OverflowError and repo validate, tasks check and repo run crashed with a traceback instead of the refusal. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
✅ OpenCodeReview: Review complete: 0 finding(s) across 3 selected item(s). Review details
|
| if score_task | ||
| else 1.0 | ||
| ) | ||
| n = {measure: len(v) for measure, v in nums.items()} |
There was a problem hiding this comment.
Must-fix: useful_task_values() returns singleton lists containing already-aggregated medians/rates, so this calculation reports n_baseline and n_treatment as 1 for every available first-useful measure, regardless of how many cells contributed. For example, three declared cells with two timed hits should report counts of 2 for seconds/steps and 3 for the rate, but all three currently report 1. _pair() exports these counts as the cells behind each value. Preserve the contributing-cell counts separately for these measures (including untimed hits only in steps/rate), and add a multi-repetition JSON regression test.
There was a problem hiding this comment.
| *USEFUL_MEASURES, *(f"metric:{m}" for m in metrics)] | ||
| base_cells = [c for c in cells if selector_matches(base_sel, c)] | ||
| base_vals = task_values(base_cells, metrics, pass_k, path_stability) | ||
| useful = {baseline: useful_counts(base_cells)} |
There was a problem hiding this comment.
Must-fix: the first-useful header counts ignore models, while comparison rows and uptake counts honor that selection. With --model m1, if m1 has one declared cell with no hit and excluded m2 has nine hits, this header reports 9 of 10 rather than 0 of 1, suggesting coverage from a population not being compared. Apply the model filter to both baseline and treatment inputs to useful_counts(), and test a mixed-model batch with different observation rates.
| # a deep nesting or a huge repeat count overflows the parser, not only re.error | ||
| try: | ||
| return Useful("command", command=re.compile(pattern)) | ||
| except (re.error, RecursionError, OverflowError) as exc: |
There was a problem hiding this comment.
Must-fix: On supported Python 3.11+ runtimes with the default integer-string conversion limit, parse_useful({"command": "a{" + "9" * 5000 + "}"}) raises ValueError when the regex parser converts the repeat count. This exception escapes the handler, and _useful_problems() catches only UsefulSpecError, so malformed task metadata aborts validation instead of becoming a task-specific validation error. Include ValueError in this handler and add this input to the parser and manifest validation tests.
Suggestion:
| except (re.error, RecursionError, OverflowError) as exc: | |
| except (re.error, RecursionError, OverflowError, ValueError) as exc: |
| return event.get("kind") == "command" and event.get("rc") == 0 and matched | ||
| path = str(event.get("path") or "") | ||
| failed = event.get("rc") == 1 # an edit whose tool result is an error changed nothing | ||
| return event.get("kind") == "edit" and not failed and any(match_glob(g, path) for g in useful.globs) |
There was a problem hiding this comment.
Must-fix: normalize and check workspace containment before matching edit paths. With useful={"edit":"**/*.py"}, an event such as {"kind":"edit","path":"/tmp/x.py","rc":0,"t":1} (or path ../x.py) is counted as useful. relative_path() preserves outside-workspace paths, and match_glob() accepts them through **/, so unrelated edits incorrectly improve timing, steps and observation rate. Apply the containment/normalization used by process_metrics._workspace_path before matching, and test both outside paths and an in-workspace path containing ...
| "first_useful_s": [_median(_nums(u.get("issued_s") for u in have))], | ||
| "first_useful_steps": [_median(observed)], | ||
| "first_useful_rate": [len(observed) / len(have) if have else None], |
There was a problem hiding this comment.
Must-fix: these singleton aggregate lists make compare's n_baseline and n_treatment equal 1 regardless of contributing cell count. compare_values.task_values() derives those counts from list lengths, and _pair() exposes them as cells behind each value. For example, the four-cell input in test_a_task_value_is_the_median_of_the_cells_that_observed_it should yield counts of 3 for seconds/steps and 4 for rate, not 1 for all three. Preserve contributor counts separately alongside the aggregate values and use them when constructing comparison rows; add assertions covering multiple repetitions.
There was a problem hiding this comment.
There was a problem hiding this comment.
🟡 Changes recommended
Active issues can misclassify useful events, corrupt metrics or comparisons, and obscure genuinely unpaired tasks.
5 open issues
What changed in this PR
5 new findings.
This PR adds first-useful-result measurements to repository evaluations, pairing timing and step counts with an observed-hit rate so comparisons also reflect runs that never reach the target.
Changes:
- Validate optional task definitions and record the first matching successful command or target-file edit.
- Add report summaries and comparison measures for timing, steps, and observed rates.
- Track failed edits, extract comparison aggregation helpers, and add tests and documentation.
| File | Description |
|---|---|
dokimi/tests/test_repo_useful.py |
Tests definitions, matching, recording, and summaries. |
dokimi/tests/test_repo_report_unreadable_cell.py |
Updates error-test patch targets after extraction. |
dokimi/tests/test_repo_manifest.py |
Tests malformed useful definitions. |
dokimi/tests/test_repo_cli_traces.py |
Tests Copilot edit completion status. |
dokimi/tests/test_batch_ledger_golden.py |
Adds the new result field to expected keys. |
dokimi/dokimi/repos/workspace.py |
Exposes parsed task definitions. |
dokimi/dokimi/repos/useful.py |
Implements matching and useful-result summaries. |
dokimi/dokimi/repos/useful_spec.py |
Parses and validates useful definitions. |
dokimi/dokimi/repos/runner.py |
Records each cell’s first useful result. |
dokimi/dokimi/repos/report.py |
Displays observed counts, medians, and reasons. |
dokimi/dokimi/repos/process_counts.py |
Houses shared process-value collection. |
dokimi/dokimi/repos/manifest.py |
Validates useful definitions during task checks. |
dokimi/dokimi/repos/host.py |
Rejects malformed definitions before execution. |
dokimi/dokimi/repos/events.py |
Records Claude edit success or failure. |
dokimi/dokimi/repos/compare.py |
Adds useful measures and header counts. |
dokimi/dokimi/repos/compare_values.py |
Extracts per-task comparison aggregation. |
dokimi/dokimi/repos/cli_traces.py |
Records Copilot edit success or failure. |
dokimi/docs/repo-evaluation-contract.md |
Documents definitions, records, and comparison behavior. |
dokimi/changelog.d/20261031_120000_napetrov_repo-first-useful.md |
Announces the new measurements. |
Review effort: Balanced
| *(["path_distance"] if path_stability else []), "cost_usd", "turns", "first_prompt_tokens", | ||
| *(f"metric:{m}" for m in metrics)] | ||
| base_vals = _task_values([c for c in cells if selector_matches(base_sel, c)], metrics, pass_k, path_stability) | ||
| *USEFUL_MEASURES, *(f"metric:{m}" for m in metrics)] |
| "first_prompt_tokens": [r["agent"].get("first_prompt_tokens") for r in recs], | ||
| **{f"metric:{name}": [r["metrics"].get(name) for r in recs] for name in metrics}, | ||
| } | ||
| found.update({**process_lists(recs), **useful_task_values(recs)}) |
| "base_date": meta["base_date"], "execution": execution, "score_task": bool(task.score), | ||
| "score_max": task.score["max"] if task.score else None, "agent": agent, | ||
| "process": process, "grade": grade} | ||
| "process": process, "first_useful": first_useful(task.useful, events), "grade": grade} |
There was a problem hiding this comment.
Fixed in c36bc80: a run whose trace has dropped lines reports its first useful result as unobserved, not as a number.
The trace counts each dropped line (also valid JSON that is not an object). We kept this all-or-nothing rule: a dropped line can hide an earlier useful event, so cutting at the first drop is a follow-up.
Pushed to ce-ttfur (c36bc80); left unresolved for your check.
There was a problem hiding this comment.
Maintainer decision: keep the all-or-nothing dropped-line policy in this PR (a trace with a dropped line reports unobserved). No code change here. Cut-at-first-drop is a follow-up: #347
| from .manifest import match_glob | ||
| from .useful_spec import Useful | ||
|
|
||
| #: The ``repo compare`` measures of the first useful result, one value per task (see :func:`useful_task_values`). | ||
| USEFUL_MEASURES = ("first_useful_s", "first_useful_steps", "first_useful_rate") | ||
|
|
||
|
|
||
| def hits(useful: Useful, event: Mapping[str, Any]) -> bool: | ||
| """Whether ``event`` satisfies ``useful``: a passing command matching the regex, or an edit matching a glob.""" | ||
| if event.get("denied"): # a refused call never ran, or never changed the file | ||
| return False | ||
| if useful.kind == "command": | ||
| text = str(event.get("text") or "") | ||
| matched = bool(useful.command and useful.command.search(text)) | ||
| return event.get("kind") == "command" and event.get("rc") == 0 and matched | ||
| path = str(event.get("path") or "") | ||
| failed = event.get("rc") == 1 # an edit whose tool result is an error changed nothing | ||
| return event.get("kind") == "edit" and not failed and any(match_glob(g, path) for g in useful.globs) |
|
|
||
|
|
||
| def _nums(values: Iterable[Any]) -> list[float]: | ||
| return [float(v) for v in values if isinstance(v, (int, float)) and not isinstance(v, bool)] |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #332 +/- ##
==========================================
+ Coverage 94.20% 94.35% +0.14%
==========================================
Files 304 311 +7
Lines 38748 39544 +796
Branches 6561 6687 +126
==========================================
+ Hits 36504 37310 +806
+ Misses 1430 1425 -5
+ Partials 814 809 -5
🚀 New features to boost your workflow:
|
…d paths - header counts (first_useful observed/declared) only for the models compared - useful_task_counts: timed, observed and declared counts per task, not a single aggregate; a kind mismatch between arms is unpaired, not compared - hits() normalizes the path into the workspace and refuses paths outside it - a useful_spec repeat count too large for int is refused, not an OverflowError - NaN and huge numbers go through as_measure - a run whose trace had dropped lines reports the result as unobserved - trace lines that are valid JSON but not an object count as dropped lines Thread: PRRT_kwDOVBg7g86yWRo0 Thread: PRRT_kwDOVBg7g86yWRo4 Thread: PRRT_kwDOVBg7g86yWRo_ Thread: PRRT_kwDOVBg7g86yWRpJ Thread: PRRT_kwDOVBg7g86yWRpM Thread: PRRT_kwDOVBg7g86yWRt8 Thread: PRRT_kwDOVBg7g86yWRuM Thread: PRRT_kwDOVBg7g86yWRuT Thread: PRRT_kwDOVBg7g86yWRuc Thread: PRRT_kwDOVBg7g86yWRuk Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| started = bool(transcript.events) or transcript.result_seen # else the agent never started | ||
| if started: | ||
| events = transcript.events | ||
| useful_events = None if transcript.dropped_lines else events |
There was a problem hiding this comment.
Must-fix: dropped_lines == 0 does not yet establish that the Claude trace is complete. For example, an assistant record with message.content: [42], followed by a valid result record, has its malformed block silently filtered out by _content() without incrementing dropped_lines. This branch then accepts the remaining events and records never (or a later hit) rather than unobserved. The earlier incomplete-trace fix therefore still misses unexpected shapes that do not raise exceptions. Have the parser mark invalid content containers/blocks as dropped, and add an integration test asserting that such a trace produces unobserved.
There was a problem hiding this comment.
Fixed in 2909150: a content block of an unknown shape, or a message content that is not a list, now counts as a dropped line, so the run reports unobserved as it does for an unreadable line. A plain string prompt is not a drop. Test: test_a_claude_trace_with_a_malformed_content_block_is_unobserved_not_never.
| path = _workspace_path( | ||
| str(event.get("path") or "") | ||
| ) # outside the workspace, or not a path: no target |
There was a problem hiding this comment.
Must-fix: the outside-workspace check only recognizes POSIX absolute paths. For example, hits(parse_useful({"edit": "**/*.py"}), {"kind": "edit", "path": "C:/tmp/x.py", "rc": 0}) returns true: _workspace_path() treats the drive-qualified path as relative, and the glob matches it. relative_path() deliberately preserves absolute Windows paths outside the supplied workspace, so these can reach this guard. Reject Windows absolute/drive-qualified paths as well before matching, and add a regression test. The same limitation already exists in the shared process-metrics helper; this new caller extends it to first-useful timings and rates.
There was a problem hiding this comment.
Fixed in 2909150: hits() refuses a drive-qualified or backslash-rooted edit path (C:/x, c:x, \\server\share) before the glob. Test: test_a_windows_absolute_or_drive_path_outside_the_workspace_never_hits. The shared _workspace_path is unchanged, so process metrics keep their behaviour; that helper is a separate follow-up.
| path = _workspace_path( | ||
| str(event.get("path") or "") | ||
| ) # outside the workspace, or not a path: no target | ||
| failed = event.get("rc") == 1 # an edit whose tool result is an error changed nothing |
There was a problem hiding this comment.
Must-fix: failed Codex edits still count as useful hits. For example, converting {"type":"item.completed","item":{"type":"file_change","status":"failed","changes":[{"path":"src/x.py","kind":"update"}]}} with codex-json emits an edit without rc, because _codex_item_completed() ignores the file-change status. This guard accepts that event, so first_useful({"edit":"src/*.py"}, ...) records an observed hit even though the patch failed, inflating first_useful_rate. Propagate Codex's failed file-change status into the normalized edit's rc (as done for Claude and Copilot), and add an end-to-end conversion/hit regression test.
There was a problem hiding this comment.
Fixed in 2909150: a Codex file_change with status: failed is an edit with rc 1, and completed is rc 0, so hits() refuses the failed one. An in-progress or unknown status leaves rc unset, as before. Test: test_a_codex_file_change_that_failed_is_no_useful_edit.
… unknown trace blocks count - A drive-qualified or backslash-rooted edit path (C:/x, \\server\share) is outside the workspace; the POSIX check took it for relative and the glob matched (CodeFactor: the test used /tmp, a B108 finding). - A Codex file_change with status failed is an edit with rc 1, as Copilot's is; completed is rc 0. - A Claude stream record whose content holds a block of no known shape, or a content that is not a list, counts as a dropped line, so the run reports unobserved rather than never. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| blocks = [b for b in content if isinstance(b, dict)] | ||
| if len(blocks) != len(content): | ||
| out.dropped_lines += 1 |
There was a problem hiding this comment.
Must-fix: the completeness fix still accepts malformed dictionary blocks. For example, an assistant record with message.content: [{}], followed by a valid result record, leaves dropped_lines == 0: _apply() silently ignores the block because it has no type. The runner therefore reports never (or a later hit) rather than unobserved, even though an event may be missing. Validate dictionary block discriminators as well as their container type, and extend the malformed-content regression to missing/non-string type values while preserving legitimate text/thinking blocks.
There was a problem hiding this comment.
Fixed in 3104341. A Claude content block counts only when it is a dict with a string type; [{}], [{"type": 7}] and [{"type": null}] are dropped lines. Text and thinking blocks still drop nothing. Tests: test_a_claude_block_without_a_string_type_is_a_dropped_line, test_claude_text_and_thinking_blocks_are_not_dropped.
| matched = bool(useful.command and useful.command.search(text)) | ||
| return event.get("kind") == "command" and event.get("rc") == 0 and matched |
There was a problem hiding this comment.
Must-fix: a regex that compiles successfully can take exponential time here. For example, {"command": "(a+)+$"} searched against a command containing 40 a characters followed by ! can stall host-side processing. Command text is not length-limited, and first_useful() runs after the container timeout, outside the runner's error-handling block, so the cell can fail to write result.json indefinitely. Check the event kind/rc before searching, and enforce a matching deadline or use a bounded-time matcher; compilation-error handling alone does not bound execution. Add a regression that verifies pathological matching cannot block result persistence. The existing counter matcher in process_metrics.py has the same pre-existing unbounded-search class.
There was a problem hiding this comment.
Partly fixed in 3104341: hits() checks kind and rc before the pattern runs, so a non-matching event never reaches the regex. Held, your call: a catastrophic pattern such as (a+)+$ on 40 a's then '!' still runs exponentially inside re. Options: (a) regex package with a timeout (new pinned dependency, my recommendation), (b) reject nested quantifiers in the spec (incomplete, may reject valid patterns), (c) match in a child process with a timeout. Which one?
There was a problem hiding this comment.
Decision: option (a). Fixed in 07c495a.
- The command pattern is searched with the
regexmodule under a 2 s budget per event;regexwas already a declared, pinned dependency, so no lock change. - Past the budget the run gets reason
timeout: no verdict, and the rest of the trace is not searched. - Test:
(a+)+$on 40athen!returnsneverin under 5 s; a timeout test covers the reason.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| raw = str(event.get("path") or "") | ||
| # outside the workspace, or not a path: no target | ||
| path = None if _NON_POSIX_ROOT.match(raw) else _workspace_path(raw) |
There was a problem hiding this comment.
Must-fix: a missing or empty edit path becomes "." in _workspace_path(), which matches broad valid globs such as "**" or "*". For example, first_useful(parse_useful({"edit": "**"}), [{"kind": "edit", "path": "", "t": 1}]) reports a timed hit although no target file is named. The Claude converter can emit exactly this empty path when an Edit/Write call lacks a file_path. Reject empty paths (and normalized workspace-root paths) before glob matching, and add a regression test for missing/empty edit paths with a broad glob.
There was a problem hiding this comment.
Fixed in 3104341. An edit with an empty path, ./ or . is no target: hits() rejects it before any glob runs. Test: test_an_edit_with_no_result_or_no_target_is_never_the_first_useful_result (parametrized).
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 4a816d9. The design holds up: the rate next to the medians answers the survivor-bias problem, and the kind and model handling in compare is right. Four things are worth fixing before merge: three tests that leave a reported value or a verdict line unpinned, and one hit rule that is too loose for edits.
What I ran, on a Linux review host at 4a816d9:
make ci-local: 30 passed, 0 failed, 7 skipped; 12,023 tests passed, 59 skipped; the changed-line coverage step passes.- Reproductions for the code findings, with the LLM transport mocked (none needed a model).
- A mutation run over the new code: 55 mutants, 20 of which pass the related suites. Five of those change a verdict or a reported value; they are the test findings inline.
- All nine commits carry a
Signed-off-by.
Checked, no issues
- Pins: the
rc == 0rule for commands,denied, the failed-edit guard, the drive-path guard, the Codex and Copilotrcmapping, kind-mismatch unpairing and the baseline--modelheader filter each fail a mutant. - Holm:
first_useful_*rows stay out ofPRIMARY; the gate row has the samep,p_holmandmdewith and without the new key. - Moved code:
_task_values,_agent_outage,_finiteand_process_listsincompare_values.pymatch the base bodies. - Counts:
n_baselineandn_treatmentare the timed, observed and declared cells, as the contract says.
Bot points I checked that no longer apply at this head (15)
- exai-bot's point on
n_baselineandn_treatmentbeing 1 does not apply: they now come fromuseful_task_counts(timed, observed and declared cells). - exai-bot's point on the header ignoring
--modeldoes not apply: the header counts go through_in_models(the treatment side is untested, see the inline comment on the median). - exai-bot's point on
ValueErrorfrom a huge repeat count does not apply:parse_usefulcatchesValueErrorwithre.errorand refuses the spec. - exai-bot's point on edits outside the workspace does not apply:
hits()normalizes the path with_workspace_pathand refuses a path outside it. - exai-bot's point on singleton aggregate lists does not apply: the counts come from
useful_task_counts, not from list lengths. - Copilot's point on review tasks filling the unpaired list does not apply:
_measure_pairsskips a task that neither arm declares a first useful result for. - Copilot's point on discarding the useful kind does not apply:
useful_kindis kept per task, a mixed or differing kind is unpaired, and the pair row carries the kind. - Copilot's point on partially unreadable traces does not apply in the code:
runner.pypasses no events tofirst_usefulwhen a line was dropped, so the result isunobserved(the test for this is a gap, see the inline comment). - Copilot's point on normalizing edit paths does not apply:
./src/a.pynormalizes and hitssrc/*.py, and../a.pyor/abs/a.pydo not. - Copilot's point on NaN and huge numbers does not apply: the stored values go through
as_measureand are refused. - exai-bot's point on a non-object content block does not apply:
_contentcounts it as a dropped line. - exai-bot's point on Windows absolute paths does not apply:
C:/x.py,c:xand a leading backslash no longer hit**/*.py. - exai-bot's point on failed Codex edits does not apply: a
file_changewith statusfailedgetsrc1 andhits()refuses it. - exai-bot's point on
message.content: [{}]does not apply: the empty dict is kept as a block that carries no event, sodropped_linesstays 0 and no event is lost. - exai-bot's point on a backtracking
commandregex does not apply: the pattern comes from the task author'stask.json, not from the agent, andprocess_metricscompiles and searches task regexes the same way.
…, summary and dropped lines An edit counts only with a success tool result (rc 0) and a real target path: an empty or '.' path is no target. Denied and failed calls never hit, and the kind and rc are checked before the pattern runs. Claude content blocks without a string type are dropped lines. Tests pin the hit rules, the median, the counts, the treatment header, the runner's dropped-line guard and the Claude block typing. The fixture binary gains a tool_result, so the sample-batch golden execution id moves with its digest. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| _codex_command_unstarted(out, item, script, workdir, cwd) | ||
| elif itype == "file_change": | ||
| status = item.get("status") # a failed patch changed nothing | ||
| rc = {"completed": 0, "failed": 1}.get(status) if isinstance(status, str) else None |
There was a problem hiding this comment.
Must-fix: malformed Codex file-change entries are silently skipped without incrementing dropped_lines. For example, {"type":"item.completed","item":{"type":"file_change","status":"completed","changes":[42]}} followed by {"type":"turn.completed","usage":{}} produces no events and dropped_lines == 0. With a declared edit target, run_cell therefore records never rather than unobserved; if a valid edit follows, it reports a first hit and step count despite the missing event. Validate the changes collection and its entries, counting a malformed record as dropped, and add a regression test analogous to the Claude malformed-content test.
There was a problem hiding this comment.
Fixed in 07c495a: a Codex file_change entry that is not an object, or has no path, raises and counts as one dropped line, with no partial edit list.
Test: test_a_codex_file_change_with_a_malformed_entry_is_one_dropped_line_and_no_edits (red before the fix).
…Codex file_change entries A `command` pattern is now searched with the `regex` module under a 2 s budget per event (USEFUL_REGEX_TIMEOUT). A search past the budget ends the run with reason `timeout`: no verdict, and the rest of the trace is not searched. A Codex file_change whose changes list holds a non-object or path-less entry is one dropped line, with no partial edit list. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| observed of declared cells per arm. | ||
| A malformed `useful` is refused by `repo validate`, `repo tasks check` and `repo run`. | ||
| Batches run before this change have no `first_useful` and no such rows. | ||
| - A Codex `file_change` record with a malformed entry (not an object, or no path) is a dropped line, as the other |
There was a problem hiding this comment.
Nit: the following sentence says valid entries were not read before this change, but the previous implementation iterated over changes and emitted every object with a path while silently skipping malformed entries. For changes: [42, {"path": "/ws/src/x.py"}], it previously emitted the valid edit; the new implementation drops the whole record. Describe that partial-processing-to-whole-record-drop change instead.
There was a problem hiding this comment.
Fixed in 8170f8f. The entry now says what the code does: a malformed changes entry, or a non-list value other than null, drops the whole Codex record as one dropped line, and none of it is read. The sentence about the earlier behaviour is gone.
|
|
||
| | Grader | Declare | Hit | | ||
| |---|---|---| | ||
| | test: the grade runs the target tests | `{"command": "<regex>"}` | the first `command` event whose text matches the regex (`re.search`) and whose `rc` is `0`. `rc` 0 says that command passed, not that every target test did, so the regex should name the target tests | |
There was a problem hiding this comment.
Nit: the implementation compiles and searches with the third-party regex module, not re. These engines accept different patterns (for example, variable-length lookbehind), so the documented pattern contract is inaccurate. Name regex.search here, consistent with the changelog and the implemented timeout.
There was a problem hiding this comment.
Already fixed in bce5fc6: §4.1 now names regex.search, the engine the implementation uses.
| if not isinstance(changes, list) or not all(isinstance(c, dict) and c.get("path") for c in changes): | ||
| raise ValueError("file_change with a malformed changes entry") |
There was a problem hiding this comment.
Must-fix: falsy non-list changes values still bypass the malformed-record guard. For example, {"type":"item.completed","item":{"type":"file_change","status":"completed","changes":{}}} followed by {"type":"turn.completed","usage":{}} leaves dropped_lines == 0: changes becomes [] before this check. Consequently, run_cell treats this malformed trace as complete and reports first_useful.reason == "never" rather than "unobserved". Validate the original value before normalizing absent/null changes, and extend the regression test with {}, "", 0, and false.
There was a problem hiding this comment.
Fixed in 5f404c8. Only a null changes counts as absent; {}, "", 0 and false are malformed and drop the record, and the test covers all four. With the fix reverted, test_a_codex_file_change_with_a_malformed_entry_is_one_dropped_line_and_no_edits fails; with it, it passes.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
LGTM. The four earlier points are fixed and pinned, and the new regex timeout, the rc 0 rule for edits and the Codex malformed-entry drop all hold up. Two nits inline.
What I ran, on a Linux review host at 07c495a:
make ci-local: 30 passed, 0 failed, 7 skipped; 12,045 tests passed, 59 skipped; changed-line coverage 97% (259 lines).- Reproductions for the code findings, with the LLM transport mocked (none needed a model).
- A mutation run over the delta: 7 mutants (no
timeout=, aTimeoutErrorhandler that continues, norcguard, nopathcheck for Codex entries, no blocktypecheck, no.guard, no empty-path guard). 6 fail the suites; the seventh, the empty-path check inhits(), is redundant with the.check, so no verdict changes. - All 11 commits carry a
Signed-off-by.
Checked, no issues
- Timeout:
(a|aa)+$on 60athen!gives reasontimeoutafter 2.00 s. A hit before the timed-out event is kept. The rest of the trace is skipped. A timed-out cell counts as declared and not observed inrepo reportand in therepo comparerate (1 of 2 observed gives 0.5). rcrule: an edit or command with norcnever hits, including a Copilot edit with nosuccess.- Dependency:
regexis already a runtime dependency inpyproject.toml.
Bot points I checked that no longer apply at this head (17)
- exai-bot's point on
n_baselineandn_treatmentbeing 1 does not apply: they now come fromuseful_task_counts(timed, observed and declared cells). - exai-bot's point on the header ignoring
--modeldoes not apply: the header counts go through_in_models, and the treatment side now has a test. - exai-bot's point on
ValueErrorfrom a huge repeat count does not apply:parse_usefulcatchesValueErrorwithregex.errorand refuses the spec. - exai-bot's point on edits outside the workspace does not apply:
hits()normalizes the path with_workspace_pathand refuses a path outside it. - exai-bot's point on singleton aggregate lists does not apply: the counts come from
useful_task_counts, not from list lengths. - Copilot's point on review tasks filling the unpaired list does not apply:
_measure_pairsskips a task that neither arm declares a first useful result for. - Copilot's point on discarding the useful kind does not apply:
useful_kindis kept per task and a differing kind is unpaired. - Copilot's point on partially unreadable traces does not apply:
runner.pypasses no events tofirst_usefulwhen a line was dropped, so the result isunobserved, and the runner guard is now tested throughrun_cell. - Copilot's point on normalizing edit paths does not apply:
./src/a.pynormalizes and hitssrc/*.py, and../a.pyor/abs/a.pydo not. - Copilot's point on NaN and huge numbers does not apply: the stored values go through
as_measureand are refused. - exai-bot's point on a non-object content block does not apply:
_contentcounts it as a dropped line. - exai-bot's point on Windows absolute paths does not apply:
C:/x.py,c:xand a leading backslash no longer hit**/*.py. - exai-bot's point on failed Codex edits does not apply: a
file_changewith statusfailedgetsrc1 andhits()refuses it. - exai-bot's point on
message.content: [{}]does not apply: a block without a stringtypenow counts as a dropped line. - exai-bot's point on a backtracking
commandregex does not apply: the pattern is searched with theregexmodule under a 2 s budget per event, and a run past it gets reasontimeout. - exai-bot's point on a missing or empty edit path does not apply:
hits()rejects an empty path,.and a path with norc0 result. - exai-bot's point on malformed Codex
file_changeentries does not apply for a list with a bad entry:[42]and[{"path": ""}]each raise and count one dropped line. The falsy non-list case is the inline nit.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
| file. The first event that hits it (a command whose text matches and whose `rc` is 0, or an | ||
| edit whose path matches) is recorded in `result.json` `first_useful` with its `issued_s` (the |
There was a problem hiding this comment.
Nit: the edit hit definition omits the success and denial guards. For useful: {"edit": "src/x.py"}, an edit of that path with rc: 1, no result, or denied: true does not count: hits() requires rc == 0 and rejects denied events. Describe a successful, non-denied matching event here so the changelog agrees with the implementation and contract §4.1.
Suggestion:
| file. The first event that hits it (a command whose text matches and whose `rc` is 0, or an | |
| edit whose path matches) is recorded in `result.json` `first_useful` with its `issued_s` (the | |
| file. The first non-denied event with `rc` 0 that hits it (a command whose text matches, or an | |
| edit whose path matches) is recorded in `result.json` `first_useful` with its `issued_s` (the |
There was a problem hiding this comment.
Fixed in 8170f8f. The changelog now says the hit is the first non-denied event with rc 0, which matches hits() and §4.1.
| if content is None or isinstance(content, str): # a user's prompt is a plain string: no block, nothing lost | ||
| return [] |
There was a problem hiding this comment.
Must-fix: missing or null assistant content is still treated as a complete trace. For example, {"type":"assistant","message":{"content":null}} followed by {"type":"result","subtype":"success","is_error":false,"usage":{}} leaves dropped_lines == 0 and result_seen == True. The runner therefore evaluates an empty event list and records first_useful.reason: "never", although the malformed assistant record could have contained the first hit. Count missing/null assistant content as a dropped line, preserving the exemption for legitimate string user prompts, and add a regression test for both missing and null content.
There was a problem hiding this comment.
Fixed in 24ab1d1. A missing or null content on an assistant record, or a missing message, is one dropped line; a user record keeps the exemption. The tests cover content: null, {} and a bare assistant record. With the fix reverted, the new test fails.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
LGTM. Both earlier threads are fixed at this head, and the delta holds up: the regex timeout, the rc 0 rule for every hit and the Codex malformed-record drop all behave as the contract says. One nit inline.
What I ran, on a Linux review host at bce5fc6:
make ci-local: 30 passed, 0 failed, 7 skipped; 12,045 tests passed, 59 skipped; changed-line coverage 97% (259 lines).- A mutation run over the delta: 15 mutants (no
rcguard, notimeout=, no.guard,or []for thechangesnormalization, no blocktypecheck, aTimeoutErrorhandler that continues, nodeniedcheck, no non-POSIX root check, and others). 12 fail the suites. The three survivors change no verdict: the 2.0 s value is unpinned (tests patch it), thekindguard under acommandspec is redundant because onlycommandevents carrytext, andnot rawis redundant with the.check. - A reproduction for the nit, with the LLM transport mocked (none needed a model).
- All 13 commits carry a
Signed-off-by.
Checked, no issues
- Threads: a falsy non-list Codex
changesnow counts one dropped line, and the contract names the dropped-lineunobserved, therc0 edit rule andregex.search. - Hit rule: every hit needs
rc0 and nodenied; an empty path or.never hits an edit glob.
Bot points I checked that no longer apply at this head (18)
- exai-bot's point on
n_baselineandn_treatmentbeing 1 does not apply: they come fromuseful_task_counts(timed, observed and declared cells). - exai-bot's point on the header ignoring
--modeldoes not apply: the header counts go through_in_models, and the treatment side has a test. - exai-bot's point on
ValueErrorfrom a huge repeat count does not apply:parse_usefulcatchesValueErrorwithregex.errorand refuses the spec. - exai-bot's point on edits outside the workspace does not apply:
hits()normalizes the path with_workspace_pathand refuses a path outside it. - exai-bot's point on singleton aggregate lists does not apply: the counts come from
useful_task_counts, not from list lengths. - Copilot's point on review tasks filling the unpaired list does not apply:
_measure_pairsskips a task that neither arm declares a first useful result for. - Copilot's point on discarding the useful kind does not apply:
useful_kindis kept per task and a differing kind is unpaired. - Copilot's point on partially unreadable traces does not apply:
runner.pypasses no events tofirst_usefulwhen a line was dropped, so the result isunobserved, and the guard is tested throughrun_cell. - Copilot's point on normalizing edit paths does not apply:
./src/a.pynormalizes and hitssrc/*.py, and../a.pyor/abs/a.pydo not. - Copilot's point on NaN and huge numbers does not apply: the stored values go through
as_measureand are refused. - exai-bot's point on a non-object content block does not apply:
_contentcounts it as a dropped line. - exai-bot's point on Windows absolute paths does not apply:
C:/x.py,c:xand a leading backslash do not hit**/*.py. - exai-bot's point on failed Codex edits does not apply: a
file_changewith statusfailedgetsrc1 andhits()refuses it. - exai-bot's point on
message.content: [{}]does not apply: a block without a stringtypecounts as a dropped line. - exai-bot's point on a backtracking
commandregex does not apply: the pattern is searched with theregexmodule under a 2 s budget per event, and a run past it gets reasontimeout. - exai-bot's point on a missing or empty edit path does not apply:
hits()rejects an empty path,.and an event withoutrc0. - exai-bot's point on malformed Codex
file_changeentries does not apply:[42]and[{"path": ""}]each raise and count one dropped line. - exai-bot's point on falsy non-list Codex
changesdoes not apply:{},"",0andfalsenow count one dropped line, and the test covers them.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
| file. The first event that hits it (a command whose text matches and whose `rc` is 0, or an | ||
| edit whose path matches) is recorded in `result.json` `first_useful` with its `issued_s` (the | ||
| event's issue time) and `steps` (events up to and including it). A run with no hit says | ||
| why (`never`, `unobserved`, `untimed`, or `timeout`: a `command` pattern, searched with the `regex` module, ran past |
There was a problem hiding this comment.
Nit: untimed means a hit was found but its timestamp is unavailable, not that the run had no hit. first_useful() still records steps, and report/compare count that cell as observed (including in first_useful_rate). Separate this case from the no-hit reasons so readers do not interpret an untimed success as a missed useful result.
There was a problem hiding this comment.
Fixed in 65af232. The changelog now lists untimed apart from the no-hit reasons: a hit in a trace without timestamps keeps its steps, counts as observed (rate included), and has no seconds.
| if not isinstance(content, list): | ||
| out.dropped_lines += 1 | ||
| return [] | ||
| blocks = [b for b in content if isinstance(b, dict) and isinstance(b.get("type"), str)] # a block says its type |
There was a problem hiding this comment.
Must-fix: this completeness check only validates the block's outer shape. For example, an assistant block {"type":"tool_use","id":"e","name":"Edit","input":42}, followed by its successful tool result and a final result record, leaves dropped_lines == 0: _tool_event() silently replaces the malformed input with {} and emits an edit with an empty path. A declared {"edit":"src/*.py"} then reports never rather than unobserved, despite an unreadable edit call. Validate tool-use inputs (and required identifying fields) before treating these blocks as complete, increment the dropped count for malformed blocks, and add a runner-level regression test.
There was a problem hiding this comment.
Fixed in 65af232. A tool_use block with no object input, no name or no id is now one dropped line and is skipped, not kept as an empty edit, so the run reads unobserved, not never.
Tests: four block shapes give dropped_lines 1; the input: 42 Edit case with a successful result reads unobserved.
The static job installs only requirements-ci.lock.txt, but its registry-drift-check and guidance self-scan import dokimi.repos, which imports regex. Both failed with ModuleNotFoundError. Add regex to requirements-ci.txt and its hashed block to the tools lock, the same 2026.9.29 pin as requirements.lock.txt. Changelog: a Codex file_change with a malformed or non-list changes value drops the whole record (no partial read); an edit hit needs rc 0 and no denial. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| seconds = _nums(u.get("issued_s") for u in have) | ||
| steps = _nums(u.get("steps") for u in have) |
There was a problem hiding this comment.
Must-fix: timing values are collected independently of observed hits. For the record already covered by test_a_stored_non_finite_or_huge_number_is_no_measure (issued_s=1.0, steps=NaN or 10**400), this summary reports zero observed hits but median_s=1.0; useful_task_values() likewise returns a seconds median and useful_task_counts() counts one timed cell. Stored records reach these functions without a consistency check, so report/compare can include timings from cells they classify as unobserved. Filter to records with measurable steps before collecting issued_s in all three functions, and extend that regression test to check the seconds median and timed-cell count.
There was a problem hiding this comment.
Fixed in 65af232. Timings come only from cells with a measurable steps (new _observed), in useful_summary, useful_task_values and useful_task_counts.
The NaN / 10**400 test now also checks the seconds median (None) and the timed-cell count (0).
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 8170f8f. The delta holds up: a missing or null assistant content is now a dropped line, the Codex and changelog wording match the code, and the regex lock fixes the static job. One test gap on the line that was rewritten.
What I ran, on a Linux review host at 8170f8f:
make ci-local: 30 passed, 0 failed, 7 skipped; 12,048 tests passed, 59 skipped; changed-line coverage 98% (259 lines).- A mutation run over the rewritten
_contentcondition: thetype == "user"clause is pinned (two mutants fail the new tests); theisinstance(content, str)branch is not (inline). - A reproduction with the mutant, no model call.
- All 15 commits carry a
Signed-off-by.
I agree with exai-bot on timings kept from a record whose steps is NaN or 10**400, but only a hand-edited artifact reaches it, so it need not hold the merge; filtering issued_s on a measurable steps would close it.
Checked, no issues
- Other records:
system,resultandrate_limit_eventlines with nomessagestill count 0 dropped lines;_contentis only called forassistantanduser. - Missing message: an assistant record with
message: nullor nomessagecounts one dropped line; a user record with none counts zero. - Lock:
regexis pinned to 2026.9.29 in the tools lock, the same version asrequirements.lock.txt.
Bot points I checked that no longer apply at this head (20)
- exai-bot's point on
n_baselineandn_treatmentbeing 1 does not apply: they come fromuseful_task_counts(timed, observed and declared cells). - exai-bot's point on the header ignoring
--modeldoes not apply: the header counts go through_in_models, and the treatment side has a test. - exai-bot's point on
ValueErrorfrom a huge repeat count does not apply:parse_usefulcatchesValueErrorwithregex.errorand refuses the spec. - exai-bot's point on edits outside the workspace does not apply:
hits()normalizes the path with_workspace_pathand refuses a path outside it. - exai-bot's point on singleton aggregate lists does not apply: the counts come from
useful_task_counts, not from list lengths. - Copilot's point on review tasks filling the unpaired list does not apply:
_measure_pairsskips a task that neither arm declares a first useful result for. - Copilot's point on discarding the useful kind does not apply:
useful_kindis kept per task and a differing kind is unpaired. - Copilot's point on partially unreadable traces does not apply:
runner.pypasses no events tofirst_usefulwhen a line was dropped, so the result isunobserved, and the guard is tested throughrun_cell. - Copilot's point on normalizing edit paths does not apply:
./src/a.pynormalizes and hitssrc/*.py, and../a.pyor/abs/a.pydo not. - Copilot's point on NaN and huge numbers does not apply: the stored values go through
as_measureand are refused. - exai-bot's point on a non-object content block does not apply:
_contentcounts it as a dropped line. - exai-bot's point on Windows absolute paths does not apply:
C:/x.py,c:xand a leading backslash do not hit**/*.py. - exai-bot's point on failed Codex edits does not apply: a
file_changewith statusfailedgetsrc1 andhits()refuses it. - exai-bot's point on
message.content: [{}]does not apply: a block without a stringtypecounts as a dropped line. - exai-bot's point on a backtracking
commandregex does not apply: the pattern is searched with theregexmodule under a 2 s budget per event, and a run past it gets reasontimeout. - exai-bot's point on a missing or empty edit path does not apply:
hits()rejects an empty path,.and an event withoutrc0. - exai-bot's point on malformed Codex
file_changeentries does not apply:[42]and[{"path": ""}]each raise and count one dropped line. - exai-bot's point on falsy non-list Codex
changesdoes not apply:{},"",0andfalsecount one dropped line, and the test covers them. - exai-bot's point on missing or null assistant content does not apply: it is one dropped line now, and a user record keeps the exemption (the string-prompt branch is the inline point).
- exai-bot's point on
tool_usewithinput: 42does not apply: Claude's stream always writesinputas an object, so only a hand-built trace reaches it.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| def first_useful( | ||
| useful: Useful | None, events: Sequence[Mapping[str, Any]] | None | ||
| ) -> dict[str, Any]: | ||
| """``result.json`` ``useful`` of one run; ``events`` is None when its trace could not be read.""" |
There was a problem hiding this comment.
Nit: the persisted field is result.json.first_useful, not result.json.useful (the latter names the task definition). Please update this docstring and the corresponding module and _declared() docstrings to use first_useful, matching the runner and contract.
Suggestion:
| """``result.json`` ``useful`` of one run; ``events`` is None when its trace could not be read.""" | |
| """``result.json`` ``first_useful`` of one run; ``events`` is None when its trace could not be read.""" |
There was a problem hiding this comment.
Fixed in 65af232. The module, first_useful and _declared() docstrings name first_useful, not useful.
…its only A tool_use block with no id, name or object input is one dropped line, not an empty edit: a run whose edit call is unreadable reads unobserved, not never. A cell that observed no hit has no seconds in the summary, the task values or the counts. Docstrings name first_useful; the changelog says untimed is observed. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| rc = {"completed": 0, "failed": 1}.get(status) if isinstance(status, str) else None | ||
| changes = [] if item.get("changes") is None else item["changes"] | ||
| # an entry that is not a change makes the record malformed: it is dropped whole (counted), not read in part | ||
| if not isinstance(changes, list) or not all(isinstance(c, dict) and c.get("path") for c in changes): |
There was a problem hiding this comment.
Must-fix: the malformed-entry guard still accepts truthy non-string paths. For example, {"type":"item.completed","item":{"type":"file_change","status":"completed","changes":[{"path":42}]}} produces an edit of "42" with rc 0 and leaves dropped_lines == 0. With useful: {"edit":"*"}, this reports an observed hit (untimed) instead of unobserved, inflating the first-useful rate from a malformed trace. Require each path to be a nonempty string before emitting any edits, and extend the malformed-entry test with numeric, list, and object paths.
There was a problem hiding this comment.
Fixed in 48df4a5. A Codex file_change path must be a non-empty string now; 42, true and a list are one dropped line with no edits, so {"path": 42} no longer reports an observed edit. Red cases added to test_a_codex_file_change_with_a_malformed_entry_is_one_dropped_line_and_no_edits first.
… entry _apply was at complexity 18 against a baseline of 17, so the growth ratchet failed on both Tests jobs. Each record kind now has its own helper; the behaviour is unchanged. The function is under the cap now, so its baseline entry is removed (check_growth --update). Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
A truthy non-string path (42, true, a list) was accepted and stringified into an edit with rc 0. Such an entry is now one dropped line with no edits, as the other malformed entries are. Red cases added to test_a_codex_file_change_with_a_malformed_entry_is_one_dropped_line_and_no_edits. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| out.dropped_lines += 1 | ||
| out.errors.append(f"line {n}: unexpected shape ({exc})") |
There was a problem hiding this comment.
Must-fix: malformed Copilot tool arguments can bypass this completeness accounting because _copilot_tool() silently normalizes them instead of raising. For example, tool.execution_start with {"toolCallId":"e","toolName":"edit","arguments":42}, followed by its successful completion and a result record, produces only a generic tool event and leaves dropped_lines == 0. run_cell() therefore reports first_useful.reason == "never" for an edit definition, although the edit's path was unreadable and the correct verdict is unobserved. Validate the arguments of recognized tools before normalizing them (preserving the supported string form for apply_patch), count malformed calls as dropped, and add a clean-JSONL regression test. The same silent normalization occurs for malformed shell arguments.
There was a problem hiding this comment.
Real, fixed in 5481ef0. _copilot_tool now raises on non-object arguments for bash/powershell/view/edit-family calls (incl. null), so copilot_json counts a dropped line. apply_patch keeps its string form. Tests: edit/create/view/bash/powershell with 42 and null drop; clean-JSONL edit with 42 gives unobserved, not never. Red before, green after.
| if isinstance(content, str) or (content is None and rec.get("type") == "user"): # a user's prompt is a plain string: no block, nothing lost | ||
| return [] |
There was a problem hiding this comment.
Must-fix: the plain-string exemption also accepts assistant content, although it is intended for user prompts. A trace containing {"type":"assistant","message":{"content":"unexpected shape"}} followed by {"type":"result","result":"done"} leaves dropped_lines == 0. Consequently, run_cell treats the trace as complete and reports first_useful.reason = "never" rather than "unobserved" for a declared definition. Restrict the string/null exemption to user records and add an assistant-string regression test alongside the existing user-string test.
Suggestion:
| if isinstance(content, str) or (content is None and rec.get("type") == "user"): # a user's prompt is a plain string: no block, nothing lost | |
| return [] | |
| if rec.get("type") == "user" and (isinstance(content, str) or content is None): | |
| return [] |
There was a problem hiding this comment.
Real, fixed in 5481ef0. The string/null exemption in _content is now for user records only, as suggested. Added a regression test: an assistant record with string content is a dropped line (red before, green after); the user-string and user-null tests still pass.
…ent count as dropped lines Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| if not isinstance(content, list): | ||
| out.dropped_lines += 1 | ||
| return [] | ||
| blocks = [b for b in content if isinstance(b, dict) and isinstance(b.get("type"), str)] # a block says its type |
There was a problem hiding this comment.
Must-fix: tool-result blocks still pass this completeness check without a usable tool_use_id. For example, a valid matching Edit call followed by {"type":"tool_result","is_error":false,"content":"ok"} and a valid final result leaves dropped_lines == 0: _apply_result() silently ignores the completion because its ID is missing. The runner then reports first_useful.reason = "never" rather than "unobserved", although the completion was unreadable. Validate that tool-result association IDs are non-empty strings and count malformed blocks as dropped, with a regression test covering this trace (also null/numeric IDs).
There was a problem hiding this comment.
Fixed in 2b0e173. A tool_result whose tool_use_id is missing, null, numeric or empty now counts as a dropped line, so the run reads unobserved, not never.
Regression: test_a_claude_tool_result_with_no_usable_id_is_a_dropped_line_not_a_silent_miss.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
LGTM. The delta holds up: the dropped-line rules now cover tool_use blocks, Copilot arguments, Codex paths and assistant string content, and the timings follow the observed hits. Two nits inline.
What I ran, on a Linux review host at 47c0880:
make ci-local: 29 passed, 1 failed, 7 skipped; 12,341 tests passed, 59 skipped. The one failure istest_unbalanced_quote_in_hook_completes_quickly(1.10 s against a 1 s bound) intest_repo_environment.py, a timing test of code this PR does not touch; it passes alone in 0.76 s. Changed-line coverage passes.- Mutation run over the delta: the user-only
_contentexemption, the Codexisinstance(path, str)check, theinputdict check, theTypeErrorin_copilot_tool(each tool family) and the three_observedcall sites each fail a test. Four clauses of_well_formed_tool_useand the non-empty Codex path do not (inline). - An offline repro of cells with a hit,
untimed,never,unobservedandtimeoutthrough summary, task values and counts: all three agree (2 of 5 observed, rate 0.4). - All 20 commits carry a
Signed-off-by.
Checked, no issues
- Real traces: the Copilot fixtures all use object
arguments, andapply_patchkeeps its string form, so no legitimate call is newly dropped. - Timeout: a catastrophic
commandpattern givestimeout, and the rest of the trace is skipped. - Repo rules: the
regexpin in the CI lock matches the runtime lock, the growth baseline only loses the split_applyentry, and no schema changes.
Bot points I checked that no longer apply at this head (25)
- exai-bot's point on
n_baselineandn_treatmentbeing 1 does not apply: they come fromuseful_task_counts(timed, observed and declared cells). - exai-bot's point on the header ignoring
--modeldoes not apply: the header counts go through_in_models. - exai-bot's point on
ValueErrorfrom a huge repeat count does not apply:parse_usefulrefuses the spec. - exai-bot's point on edits outside the workspace does not apply:
hits()normalizes the path and refuses one outside it. - exai-bot's point on singleton aggregate lists does not apply: the counts come from
useful_task_counts. - Copilot's point on review tasks filling the unpaired list does not apply:
_measure_pairsskips a task that neither arm declares. - Copilot's point on discarding the useful kind does not apply:
useful_kindis kept per task and a differing kind is unpaired. - Copilot's point on partially unreadable traces does not apply: a trace with a dropped line gives
unobserved, and the maintainer kept that all-or-nothing rule (follow-up issue 347). - Copilot's point on normalizing edit paths does not apply:
./src/a.pyhitssrc/*.py, and../a.pyor/abs/a.pydo not. - Copilot's point on NaN and huge numbers does not apply: stored values go through
as_measureand are refused. - exai-bot's point on a non-object content block does not apply:
_contentcounts it as a dropped line. - exai-bot's point on Windows absolute paths does not apply:
C:/x.py,c:xand a leading backslash do not hit**/*.py. - exai-bot's point on failed Codex edits does not apply: a
file_changewith statusfailedgetsrc1 and does not hit. - exai-bot's point on
message.content: [{}]does not apply: a block without a stringtypecounts as a dropped line. - exai-bot's point on a backtracking
commandregex does not apply: the pattern is searched with theregexmodule under a 2 s budget per event and gives reasontimeout. - exai-bot's point on a missing or empty edit path does not apply:
hits()rejects an empty path and.. - exai-bot's point on malformed Codex
file_changeentries does not apply:[42]and[{"path": ""}]each count one dropped line. - exai-bot's point on falsy non-list Codex
changesdoes not apply:{},"",0andfalsecount one dropped line. - exai-bot's point on missing or null assistant content does not apply: it is one dropped line.
- exai-bot's point on a
tool_usewithinput: 42does not apply:_well_formed_tool_usecounts it as a dropped line. - exai-bot's point on timings kept from a cell with unmeasurable
stepsdoes not apply:_observedfilters them in all three functions. - exai-bot's point on non-string Codex paths does not apply:
42,trueand a list count one dropped line. - exai-bot's point on malformed Copilot arguments does not apply:
_copilot_toolraises for a non-objectargumentson bash, powershell, view and edit calls, so the line is counted as dropped. - exai-bot's point on assistant string content does not apply: the string and null exemption is for
userrecords only. - exai-bot's point on a
tool_resultwithout a usabletool_use_iddoes not apply as a blocker: Claude's stream always writes the id, so only a hand-built trace reaches it, the same call I made for the non-objectinputcase earlier.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
| seconds = _nums(u.get("issued_s") for u in _observed(have)) | ||
| steps = _nums(u.get("steps") for u in have) | ||
| return { | ||
| "kind": have[0]["kind"], |
There was a problem hiding this comment.
Must-fix: mixed definitions are pooled and labeled as the first cell's kind. For example, a command hit at 10 s/4 steps and an edit hit at 40 s/9 steps produce a summary labeled command with medians 25 s/6.5 steps. Report groups only by (task, arm, model), and the batch ledger permits adding repetitions after changing task content, so no upstream check excludes this case. Apply the mixed-kind protection used by useful_task_values() (or split the summary by kind), and add a report regression test so incompatible results are not presented as one metric.
There was a problem hiding this comment.
Fixed in 2b0e173. A row whose cells name both kinds now reports kind mixed and no median, as repo compare already does.
Regression: test_useful_summary_of_mixed_kinds_names_no_kind_and_no_median.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 4a131a7. One risk to fix before merge: repo report still pools command and edit results into one row. The delta since my last review (tool_use shape checks, Copilot arguments, Codex paths, assistant string content) holds up, and both nits from my last round are fixed. One test nit inline.
What I ran, on a Linux review host at 4a131a7:
make ci-local: 30 passed, 0 failed, 7 skipped; 12,344 tests passed, 59 skipped.- Reproductions for the findings, with the LLM transport mocked (none needed a model).
- Mutation checks on the empty
tool_useid, the empty Codex path and the Copilot edit-tool set: the first two now fail a test; the third does not (inline). - All 22 commits carry a
Signed-off-by.
Checked, no issues
- Threads: the empty
id,nameand Codexpathcases are pinned (droppingbool(block["id"])or thec["path"]clause fails a test), and the contract now says what drops a line. - Hit rule: every hit needs
rc0 and nodenied; an empty path or.never hits an edit glob.
Bot points I checked that no longer apply at this head (25)
- exai-bot's point on
n_baselineandn_treatmentbeing 1 does not apply: they come fromuseful_task_counts(timed, observed and declared cells). - exai-bot's point on the header ignoring
--modeldoes not apply: the header counts go through_in_models. - exai-bot's point on
ValueErrorfrom a huge repeat count does not apply:parse_usefulrefuses the spec. - exai-bot's point on edits outside the workspace does not apply:
hits()normalizes the path and refuses one outside it. - exai-bot's point on singleton aggregate lists does not apply: the counts come from
useful_task_counts. - Copilot's point on review tasks filling the unpaired list does not apply:
_measure_pairsskips a task that neither arm declares. - Copilot's point on discarding the useful kind does not apply:
useful_kindis kept per task and a differing kind is unpaired. - Copilot's point on partially unreadable traces does not apply: a trace with a dropped line gives
unobserved, and the maintainer kept that all-or-nothing rule (follow-up issue 347). - Copilot's point on normalizing edit paths does not apply:
./src/a.pyhitssrc/*.py, and../a.pyor/abs/a.pydo not. - Copilot's point on NaN and huge numbers does not apply: stored values go through
as_measureand are refused. - exai-bot's point on a non-object content block does not apply:
_contentcounts it as a dropped line. - exai-bot's point on Windows absolute paths does not apply:
C:/x.py,c:xand a leading backslash do not hit**/*.py. - exai-bot's point on failed Codex edits does not apply: a
file_changewith statusfailedgetsrc1 and does not hit. - exai-bot's point on
message.content: [{}]does not apply: a block without a stringtypecounts as a dropped line. - exai-bot's point on a backtracking
commandregex does not apply: the pattern is searched with theregexmodule under a 2 s budget per event and gives reasontimeout. - exai-bot's point on a missing or empty edit path does not apply:
hits()rejects an empty path and.. - exai-bot's point on malformed Codex
file_changeentries does not apply:[42]and[{"path": ""}]each count one dropped line. - exai-bot's point on falsy non-list Codex
changesdoes not apply:{},"",0andfalsecount one dropped line. - exai-bot's point on missing or null assistant content does not apply: it is one dropped line.
- exai-bot's point on a
tool_usewithinput: 42does not apply:_well_formed_tool_usecounts it as a dropped line. - exai-bot's point on timings kept from a cell with unmeasurable
stepsdoes not apply:_observedfilters them in all three functions. - exai-bot's point on non-string Codex paths does not apply:
42,trueand a list count one dropped line. - exai-bot's point on malformed Copilot arguments does not apply:
_copilot_toolraises for a non-objectargumentson shell, read and edit calls, so the line is counted as dropped. - exai-bot's point on assistant string content does not apply: the string and null exemption is for
userrecords only. - exai-bot's point on a
tool_resultwithout a usabletool_use_iddoes not apply as a blocker: Claude's stream always writes the id, so only a hand-built trace reaches it.
| seconds = _nums(u.get("issued_s") for u in _observed(have)) | ||
| steps = _nums(u.get("steps") for u in have) | ||
| return { | ||
| "kind": have[0]["kind"], |
There was a problem hiding this comment.
repo report labels a mixed row with the first cell's kind and pools command and edit medians.
I agree with exai-bot here; New evidence: useful_summary on a command hit (issued_s 10, steps 4) and an edit hit (40, 9) in one row returns {'kind': 'command', 'n': 2, 'observed': 2, 'median_s': 25.0, 'median_steps': 6.5}, and repo report prints first useful (command): 2 of 2 observed, median 25.0 s, 6.5 steps. useful_task_values already returns no value for the same cells (useful_kind gives mixed), so report and compare disagree.
Suggestion: when useful_kind(recs) == "mixed", set kind to "mixed" and both medians to None, as useful_task_values does, and add a report test with one cell of each kind.
There was a problem hiding this comment.
Fixed in 2b0e173, the same change as the exai-bot thread: a mixed row is kind mixed with no median.
Regression: test_useful_summary_of_mixed_kinds_names_no_kind_and_no_median.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to 81484ad, which fix the nit from my review:
81484ad:dokimi/tests/test_repo_useful.py:851: the Copilot arguments test now also runs forwriteandstr_replace_editor.
make ci-local at 81484ad: 30 passed, 0 failed, 7 skipped.
Not approving yet. Still open: the risk from my review.
…irst-useful kinds have no median A Claude tool_result that names no call (missing, null, numeric or empty tool_use_id) was ignored silently, so the trace looked complete and the first useful result read "never" instead of "unobserved". It now counts as a dropped line, as other malformed blocks do. repo report named a mixed row by its first cell's kind and pooled the command and edit medians. A mixed row now has kind "mixed" and no median, as repo compare already shows. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Summary
Two rules in
hits(): a failed edit (Claudetool_resultwith an error, copilotsuccess: false) changed nothing, so itis no hit; a command counts only with
rc0 and a matching regex.Evidence
Before: the medians of
repo comparecover only the cells that reached the first useful result, so an armthat rarely reaches it looks no slower.
After:
first_useful_ratepairs the share of declared cells that observed a hit, per task, and the headercounts observed of declared cells per arm.
Break-then-revert, each fix removed and its test run to fail, then restored:
test_a_failed_edit_changed_nothing_so_the_next_successful_one_is_the_first_useful_result;rcfromsuccess: failstest_copilot_edit_rc_is_the_completion_success(2 cases);RecursionErrorandOverflowErrorinparse_useful: failtest_a_malformed_definition_is_refused(the nested-parenthesis case and the
a{99999999999}case).make ci-localon 4586da3:0 failed, 30 passed, 7 skipped; pytest12059 passed, 6 skipped;CI_LOCAL_EXIT=0.Merge Danger
Door: two-way. Additive keys:
result.jsongetsfirst_useful,repo comparegets new measures and a headerline. Batches run before this change have no
first_usefuland show no such rows.Blast Radius:
repo run,repo report,repo compareoutput andresult.json. No change to grading or to theagent's run. A
task.jsonwith a malformedusefulis now refused byrepo validate,repo tasks checkandrepo runbefore any cell runs.
Limits
rc0 is only a proxy for passing the target tests.issued_sis the issue time of the hit, so a test run's duration is not included (completion would need thetool_resulttimestamp, which_apply_resultdrops).repo comparepools command and edit kinds in the mean delta; the per-task rows show which.Open questions (not blocking this PR)
a failed Codex
file_change, an empty or.path, and an outside or non-POSIX path do not hit (round 3).field now yields no hit (a behaviour change in round 3).
first_useful_ratecountsunobservedcells (trace not read) in its denominator withnevercells: a design call.--modeltoo (round 3).first dropped line is a follow-up: First-useful: keep the events before the first dropped trace line #347
useful.py): theusefulpattern is a regex run withre, and acatastrophic pattern can run long. Options: a timeout-capable matcher (new dependency), rejecting nested
quantifiers, or a child process.
🤖 Generated with Claude Code