Skip to content

feat(repo): time to first useful result, with a rate in repo compare - #332

Open
napetrov wants to merge 21 commits into
intel:mainfrom
napetrov:ce-ttfur
Open

napetrov wants to merge 21 commits into
intel:mainfrom
napetrov:ce-ttfur

Conversation

@napetrov

@napetrov napetrov commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

Summary

repo run (per cell)
  events.jsonl  ->  first_useful(task.useful, events)   # first hit, steps, issued_s
  result.json   ->  first_useful {kind, reason, issued_s, steps}

repo report   row: observed of declared, median s, median steps, reasons
repo compare  per task: first_useful_s, first_useful_steps, first_useful_rate
              header: first useful observed: <arm> n of m, ...
src/repos/
├── useful_spec.py    # parses task.json "useful" ({"command": regex} or {"edit": glob(s)}); a malformed one is refused
├── useful.py         # hits(), first_useful(), report summary, compare values
├── compare_values.py # per-task values, moved out of compare.py (800-line cap)
└── cli_traces.py     # copilot edit events take their completion's success as rc

Two rules in hits(): a failed edit (Claude tool_result with an error, copilot success: false) changed nothing, so it
is no hit; a command counts only with rc 0 and a matching regex.

Evidence

  • Before: the medians of repo compare cover only the cells that reached the first useful result, so an arm
    that rarely reaches it looks no slower.
    After: first_useful_rate pairs the share of declared cells that observed a hit, per task, and the header
    counts observed of declared cells per arm.

  • Break-then-revert, each fix removed and its test run to fail, then restored:

    • rate numerator: fails two tests;
    • failed Claude edit guard: fails test_a_failed_edit_changed_nothing_so_the_next_successful_one_is_the_first_useful_result;
    • copilot edit rc from success: fails test_copilot_edit_rc_is_the_completion_success (2 cases);
    • RecursionError and OverflowError in parse_useful: fail test_a_malformed_definition_is_refused
      (the nested-parenthesis case and the a{99999999999} case).
  • make ci-local on 4586da3: 0 failed, 30 passed, 7 skipped; pytest 12059 passed, 6 skipped; CI_LOCAL_EXIT=0.

Merge Danger

Door: two-way. Additive keys: result.json gets first_useful, repo compare gets new measures and a header
line. Batches run before this change have no first_useful and show no such rows.

Blast Radius: repo run, repo report, repo compare output and result.json. No change to grading or to the
agent's run. A task.json with a malformed useful is now refused by repo validate, repo tasks check and repo run
before any cell runs.

Limits

Open questions (not blocking this PR)

  • Edits: a hit needs a success result (rc 0) and a target inside the workspace; a killed or unfinished Claude edit,
    a failed Codex file_change, an empty or . path, and an outside or non-POSIX path do not hit (round 3).
  • Copilot edit paths are relativised against the workdir, not the cwd (unconfirmed). A Copilot edit with no success
    field now yields no hit (a behaviour change in round 3).
  • first_useful_rate counts unobserved cells (trace not read) in its denominator with never cells: a design call.
  • The treatment header counts only the models compared; the rows honour --model too (round 3).
  • Dropped trace lines discard the whole cell's events (maintainer decision: keep all-or-nothing here). Cutting at the
    first dropped line is a follow-up: First-useful: keep the events before the first dropped trace line #347
  • Held for the maintainer (review thread on useful.py): the useful pattern is a regex run with re, and a
    catastrophic pattern can run long. Options: a timeout-capable matcher (new dependency), rejecting nested
    quantifiers, or a child process.

🤖 Generated with Claude Code

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>
@napetrov
napetrov requested a review from a team as a code owner November 1, 2026 01:17
Copilot AI balanced review requested due to automatic review settings November 1, 2026 01:17
@exai-bot

exai-bot commented Nov 1, 2026 •

Copy link
Copy Markdown

✅ OpenCodeReview: Review complete: 0 finding(s) across 24 selected item(s).

Review details
  • Commit reviewed: 47c0880 against merge base d3e2a26
  • Coverage: 24 files in 5 groups
  • Engine: OpenCodeReview v1.13.3, us.openai.gpt-6.1-sol
  • Usage: 568k tokens (343k from cache), 48s
  • Tool calls: 54 (40 file_read, 12 code_search, 2 file_read_diff)
  • First-useful evaluation contract, implementation, and tests: 7 files
  • Repository event and trace accounting: 6 files
  • Comparison values and report output: 5 files
  • Repository execution and workspace integration: 5 files
  • Growth baseline maintenance: 1 file

if score_task
else 1.0
)
n = {measure: len(v) for measure, v in nums.items()}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c36bc80: aggregate counts are replaced by useful_task_counts, per task: timed, observed and declared.
The header counts come from the compared models only. Tests added in test_repo_useful.py.
Pushed to ce-ttfur (c36bc80); left unresolved for your check.

Comment thread dokimi/dokimi/repos/compare.py Outdated
*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)}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c36bc80: the header counts use only the models compared (_in_models).
A test covers the header with two models in the batch.
Pushed to ce-ttfur (c36bc80); left unresolved for your check.

Comment thread dokimi/dokimi/repos/useful_spec.py Outdated
# 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:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
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:

Suggested change
except (re.error, RecursionError, OverflowError) as exc:
except (re.error, RecursionError, OverflowError, ValueError) as exc:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c36bc80: an int repeat count too large for the int-str limit is refused as a spec error (ValueError is caught with re.error).
The pattern is refused, not raised as an OverflowError.
Pushed to ce-ttfur (c36bc80); left unresolved for your check.

Comment thread dokimi/dokimi/repos/useful.py Outdated
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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
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 ...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c36bc80: hits() normalizes the path into the workspace (_workspace_path) and returns False for a path outside it.
Tests cover an absolute path outside the workspace and an absent path.
Pushed to ce-ttfur (c36bc80); left unresolved for your check.

Comment thread dokimi/dokimi/repos/useful.py Outdated
Comment on lines +101 to +103
"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],

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c36bc80: same change as the aggregate-count thread: per-task counts from useful_task_counts.
The count keys are timed, observed and declared, so a declared-only result is not counted as observed.
Pushed to ce-ttfur (c36bc80); left unresolved for your check.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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)]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c36bc80: a pair where both arms have no first-useful value is skipped, not reported as a measure.
_measure_pairs checks both arms before it adds first_useful.
Pushed to ce-ttfur (c36bc80); left unresolved for your check.

"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)})

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c36bc80: each task entry keeps useful_kind; a mixed kind gives no values.
An arm pair whose kinds differ is unpaired, with a row line saying which kinds meet.
Pushed to ce-ttfur (c36bc80); left unresolved for your check.

Comment thread dokimi/dokimi/repos/runner.py Outdated
"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}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread dokimi/dokimi/repos/useful.py Outdated
Comment on lines +19 to +36
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)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c36bc80: same path fix as the hits() thread above (normalized into the workspace, outside paths refused).
See the tests added in test_repo_useful.py.
Pushed to ce-ttfur (c36bc80); left unresolved for your check.

Comment thread dokimi/dokimi/repos/useful.py Outdated


def _nums(values: Iterable[Any]) -> list[float]:
return [float(v) for v in values if isinstance(v, (int, float)) and not isinstance(v, bool)]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c36bc80: NaN and huge numbers go through dokimi.measure.as_measure, so they are refused, not counted.
A test covers NaN and an out-of-range value.
Pushed to ce-ttfur (c36bc80); left unresolved for your check.

@codecov-commenter

codecov-commenter commented Nov 1, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 98.05825% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.32%. Comparing base (d3e2a26) to head (47c0880).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
dokimi/dokimi/repos/cli_traces.py 91.66% 2 Missing ⚠️
dokimi/dokimi/repos/events.py 96.61% 0 Missing and 2 partials ⚠️
dokimi/dokimi/repos/useful.py 97.87% 0 Missing and 2 partials ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #332      +/-   ##
==========================================
+ Coverage   94.20%   94.32%   +0.12%     
==========================================
  Files         304      311       +7     
  Lines       38748    39537     +789     
  Branches     6561     6686     +125     
==========================================
+ Hits        36504    37295     +791     
+ Misses       1430     1429       -1     
+ Partials      814      813       -1     
Files with missing lines Coverage Δ
dokimi/dokimi/repos/compare.py 100.00% <100.00%> (+0.22%) ⬆️
dokimi/dokimi/repos/compare_values.py 100.00% <100.00%> (ø)
dokimi/dokimi/repos/host.py 91.00% <100.00%> (+0.18%) ⬆️
dokimi/dokimi/repos/manifest.py 94.45% <100.00%> (+0.11%) ⬆️
dokimi/dokimi/repos/process_counts.py 100.00% <100.00%> (ø)
dokimi/dokimi/repos/report.py 97.80% <100.00%> (+0.04%) ⬆️
dokimi/dokimi/repos/runner.py 96.87% <100.00%> (+0.02%) ⬆️
dokimi/dokimi/repos/transcript.py 96.77% <100.00%> (+0.05%) ⬆️
dokimi/dokimi/repos/useful_spec.py 100.00% <100.00%> (ø)
dokimi/dokimi/repos/workspace.py 90.25% <100.00%> (+0.08%) ⬆️
... and 3 more

... and 16 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread dokimi/dokimi/repos/useful.py Outdated
Comment on lines +36 to +38
path = _workspace_path(
str(event.get("path") or "")
) # outside the workspace, or not a path: no target

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread dokimi/dokimi/repos/useful.py Outdated
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Comment thread dokimi/dokimi/repos/events.py Outdated
Comment on lines +206 to +208
blocks = [b for b in content if isinstance(b, dict)]
if len(blocks) != len(content):
out.dropped_lines += 1

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread dokimi/dokimi/repos/useful.py Outdated
Comment on lines +39 to +40
matched = bool(useful.command and useful.command.search(text))
return event.get("kind") == "command" and event.get("rc") == 0 and matched

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

performance · medium
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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decision: option (a). Fixed in 07c495a.

  • The command pattern is searched with the regex module under a 2 s budget per event; regex was 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 40 a then ! returns never in under 5 s; a timeout test covers the reason.

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Comment thread dokimi/dokimi/repos/useful.py Outdated
Comment on lines +41 to +43
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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 antonwolfy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 == 0 rule for commands, denied, the failed-edit guard, the drive-path guard, the Codex and Copilot rc mapping, kind-mismatch unpairing and the baseline --model header filter each fail a mutant.
  • Holm: first_useful_* rows stay out of PRIMARY; the gate row has the same p, p_holm and mde with and without the new key.
  • Moved code: _task_values, _agent_outage, _finite and _process_lists in compare_values.py match the base bodies.
  • Counts: n_baseline and n_treatment are 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_baseline and n_treatment being 1 does not apply: they now come from useful_task_counts (timed, observed and declared cells).
  • exai-bot's point on the header ignoring --model does 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 ValueError from a huge repeat count does not apply: parse_useful catches ValueError with re.error and refuses the spec.
  • exai-bot's point on edits outside the workspace does not apply: hits() normalizes the path with _workspace_path and 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_pairs skips a task that neither arm declares a first useful result for.
  • Copilot's point on discarding the useful kind does not apply: useful_kind is 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.py passes no events to first_useful when a line was dropped, so the result is unobserved (the test for this is a gap, see the inline comment).
  • Copilot's point on normalizing edit paths does not apply: ./src/a.py normalizes and hits src/*.py, and ../a.py or /abs/a.py do not.
  • Copilot's point on NaN and huge numbers does not apply: the stored values go through as_measure and are refused.
  • exai-bot's point on a non-object content block does not apply: _content counts it as a dropped line.
  • exai-bot's point on Windows absolute paths does not apply: C:/x.py, c:x and a leading backslash no longer hit **/*.py.
  • exai-bot's point on failed Codex edits does not apply: a file_change with status failed gets rc 1 and hits() refuses it.
  • exai-bot's point on message.content: [{}] does not apply: the empty dict is kept as a block that carries no event, so dropped_lines stays 0 and no event is lost.
  • exai-bot's point on a backtracking command regex does not apply: the pattern comes from the task author's task.json, not from the agent, and process_metrics compiles and searches task regexes the same way.

Comment thread dokimi/dokimi/repos/useful.py Resolved
Comment thread dokimi/dokimi/repos/useful.py Resolved
Comment thread dokimi/dokimi/repos/runner.py Resolved
Comment thread dokimi/dokimi/repos/useful.py Resolved
…, 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

documentation · low
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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread dokimi/docs/repo-evaluation-contract.md Outdated

| 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 |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

documentation · low
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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already fixed in bce5fc6: §4.1 now names regex.search, the engine the implementation uses.

Comment thread dokimi/dokimi/repos/cli_traces.py Outdated
Comment on lines +627 to +628
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")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 antonwolfy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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=, a TimeoutError handler that continues, no rc guard, no path check for Codex entries, no block type check, no . guard, no empty-path guard). 6 fail the suites; the seventh, the empty-path check in hits(), is redundant with the . check, so no verdict changes.
  • All 11 commits carry a Signed-off-by.

Checked, no issues

  • Timeout: (a|aa)+$ on 60 a then ! gives reason timeout after 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 in repo report and in the repo compare rate (1 of 2 observed gives 0.5).
  • rc rule: an edit or command with no rc never hits, including a Copilot edit with no success.
  • Dependency: regex is already a runtime dependency in pyproject.toml.
Bot points I checked that no longer apply at this head (17)
  • exai-bot's point on n_baseline and n_treatment being 1 does not apply: they now come from useful_task_counts (timed, observed and declared cells).
  • exai-bot's point on the header ignoring --model does not apply: the header counts go through _in_models, and the treatment side now has a test.
  • exai-bot's point on ValueError from a huge repeat count does not apply: parse_useful catches ValueError with regex.error and refuses the spec.
  • exai-bot's point on edits outside the workspace does not apply: hits() normalizes the path with _workspace_path and 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_pairs skips a task that neither arm declares a first useful result for.
  • Copilot's point on discarding the useful kind does not apply: useful_kind is kept per task and a differing kind is unpaired.
  • Copilot's point on partially unreadable traces does not apply: runner.py passes no events to first_useful when a line was dropped, so the result is unobserved, and the runner guard is now tested through run_cell.
  • Copilot's point on normalizing edit paths does not apply: ./src/a.py normalizes and hits src/*.py, and ../a.py or /abs/a.py do not.
  • Copilot's point on NaN and huge numbers does not apply: the stored values go through as_measure and are refused.
  • exai-bot's point on a non-object content block does not apply: _content counts it as a dropped line.
  • exai-bot's point on Windows absolute paths does not apply: C:/x.py, c:x and a leading backslash no longer hit **/*.py.
  • exai-bot's point on failed Codex edits does not apply: a file_change with status failed gets rc 1 and hits() refuses it.
  • exai-bot's point on message.content: [{}] does not apply: a block without a string type now counts as a dropped line.
  • exai-bot's point on a backtracking command regex does not apply: the pattern is searched with the regex module under a 2 s budget per event, and a run past it gets reason timeout.
  • exai-bot's point on a missing or empty edit path does not apply: hits() rejects an empty path, . and a path with no rc 0 result.
  • exai-bot's point on malformed Codex file_change entries 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.

Comment thread dokimi/dokimi/repos/cli_traces.py Resolved
Comment thread dokimi/docs/repo-evaluation-contract.md Resolved
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Comment on lines +6 to +7
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

documentation · low
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:

Suggested change
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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8170f8f. The changelog now says the hit is the first non-denied event with rc 0, which matches hits() and §4.1.

Comment thread dokimi/dokimi/repos/events.py Outdated
Comment on lines +201 to +202
if content is None or isinstance(content, str): # a user's prompt is a plain string: no block, nothing lost
return []

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 antonwolfy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 rc guard, no timeout=, no . guard, or [] for the changes normalization, no block type check, a TimeoutError handler that continues, no denied check, 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), the kind guard under a command spec is redundant because only command events carry text, and not raw is 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 changes now counts one dropped line, and the contract names the dropped-line unobserved, the rc 0 edit rule and regex.search.
  • Hit rule: every hit needs rc 0 and no denied; 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_baseline and n_treatment being 1 does not apply: they come from useful_task_counts (timed, observed and declared cells).
  • exai-bot's point on the header ignoring --model does not apply: the header counts go through _in_models, and the treatment side has a test.
  • exai-bot's point on ValueError from a huge repeat count does not apply: parse_useful catches ValueError with regex.error and refuses the spec.
  • exai-bot's point on edits outside the workspace does not apply: hits() normalizes the path with _workspace_path and 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_pairs skips a task that neither arm declares a first useful result for.
  • Copilot's point on discarding the useful kind does not apply: useful_kind is kept per task and a differing kind is unpaired.
  • Copilot's point on partially unreadable traces does not apply: runner.py passes no events to first_useful when a line was dropped, so the result is unobserved, and the guard is tested through run_cell.
  • Copilot's point on normalizing edit paths does not apply: ./src/a.py normalizes and hits src/*.py, and ../a.py or /abs/a.py do not.
  • Copilot's point on NaN and huge numbers does not apply: the stored values go through as_measure and are refused.
  • exai-bot's point on a non-object content block does not apply: _content counts it as a dropped line.
  • exai-bot's point on Windows absolute paths does not apply: C:/x.py, c:x and a leading backslash do not hit **/*.py.
  • exai-bot's point on failed Codex edits does not apply: a file_change with status failed gets rc 1 and hits() refuses it.
  • exai-bot's point on message.content: [{}] does not apply: a block without a string type counts as a dropped line.
  • exai-bot's point on a backtracking command regex does not apply: the pattern is searched with the regex module under a 2 s budget per event, and a run past it gets reason timeout.
  • exai-bot's point on a missing or empty edit path does not apply: hits() rejects an empty path, . and an event without rc 0.
  • exai-bot's point on malformed Codex file_change entries does not apply: [42] and [{"path": ""}] each raise and count one dropped line.
  • exai-bot's point on falsy non-list Codex changes does not apply: {}, "", 0 and false now count one dropped line, and the test covers them.

Comment thread dokimi/dokimi/repos/events.py Resolved
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

documentation · low
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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Comment thread dokimi/dokimi/repos/useful.py Outdated
Comment on lines +108 to +109
seconds = _nums(u.get("issued_s") for u in have)
steps = _nums(u.get("steps") for u in have)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 antonwolfy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 _content condition: the type == "user" clause is pinned (two mutants fail the new tests); the isinstance(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, result and rate_limit_event lines with no message still count 0 dropped lines; _content is only called for assistant and user.
  • Missing message: an assistant record with message: null or no message counts one dropped line; a user record with none counts zero.
  • Lock: regex is pinned to 2026.9.29 in the tools lock, the same version as requirements.lock.txt.
Bot points I checked that no longer apply at this head (20)
  • exai-bot's point on n_baseline and n_treatment being 1 does not apply: they come from useful_task_counts (timed, observed and declared cells).
  • exai-bot's point on the header ignoring --model does not apply: the header counts go through _in_models, and the treatment side has a test.
  • exai-bot's point on ValueError from a huge repeat count does not apply: parse_useful catches ValueError with regex.error and refuses the spec.
  • exai-bot's point on edits outside the workspace does not apply: hits() normalizes the path with _workspace_path and 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_pairs skips a task that neither arm declares a first useful result for.
  • Copilot's point on discarding the useful kind does not apply: useful_kind is kept per task and a differing kind is unpaired.
  • Copilot's point on partially unreadable traces does not apply: runner.py passes no events to first_useful when a line was dropped, so the result is unobserved, and the guard is tested through run_cell.
  • Copilot's point on normalizing edit paths does not apply: ./src/a.py normalizes and hits src/*.py, and ../a.py or /abs/a.py do not.
  • Copilot's point on NaN and huge numbers does not apply: the stored values go through as_measure and are refused.
  • exai-bot's point on a non-object content block does not apply: _content counts it as a dropped line.
  • exai-bot's point on Windows absolute paths does not apply: C:/x.py, c:x and a leading backslash do not hit **/*.py.
  • exai-bot's point on failed Codex edits does not apply: a file_change with status failed gets rc 1 and hits() refuses it.
  • exai-bot's point on message.content: [{}] does not apply: a block without a string type counts as a dropped line.
  • exai-bot's point on a backtracking command regex does not apply: the pattern is searched with the regex module under a 2 s budget per event, and a run past it gets reason timeout.
  • exai-bot's point on a missing or empty edit path does not apply: hits() rejects an empty path, . and an event without rc 0.
  • exai-bot's point on malformed Codex file_change entries does not apply: [42] and [{"path": ""}] each raise and count one dropped line.
  • exai-bot's point on falsy non-list Codex changes does not apply: {}, "", 0 and false count 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_use with input: 42 does not apply: Claude's stream always writes input as an object, so only a hand-built trace reaches it.

Comment thread dokimi/dokimi/repos/events.py Outdated
"""The object blocks of a message; a record with a block or a content of another shape counts as dropped."""
content = (rec.get("message") or {}).get("content")
return [b for b in content if isinstance(b, dict)] if isinstance(content, list) else []
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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plain-string branch of _content is not pinned, and losing it turns every Claude cell unobserved.

Mutation check: replacing the condition with if content is None and rec.get("type") == "user": (the isinstance(content, str) branch removed) passes all 72 tests in test_repo_useful.py and the related repo suites. With that mutant, convert("claude-stream-json", json.dumps({"type": "user", "message": {"content": "hi"}}), "/ws").dropped_lines is 1 (the same for an assistant record), where the head gives 0. A plain-string prompt is the case the line's own comment names. runner.py then passes no events to first_useful, so any cell with such a record reads unobserved instead of its hit or never. The new tests cover only content: null and a missing message.

Suggestion: add to test_repo_useful.py a case that a record with "content": "hi" (type user) gives dropped_lines == 0, next to test_a_user_record_without_content_is_not_dropped.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 65af232. The plain-string user prompt now has its own test: dropped_lines == 0, next to the null-content case.

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Comment thread dokimi/dokimi/repos/useful.py Outdated
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."""

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

documentation · low
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:

Suggested change
"""``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."""

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Comment thread dokimi/dokimi/repos/cli_traces.py Outdated
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):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Comment on lines +543 to 544
out.dropped_lines += 1
out.errors.append(f"line {n}: unexpected shape ({exc})")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread dokimi/dokimi/repos/events.py Outdated
Comment on lines +224 to +225
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 []

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
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:

Suggested change
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 []

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
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).

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants