Repository navigation
Conversation
Add built-in harness_profile.v1 files codex, codex-review, copilot and copilot-review, so repo run and repo reach can compare guidance arms under codex exec, codex exec review --commit HEAD, copilot -p and Copilot's /review agent, not only Claude Code. - trace: codex-json and copilot-json convert the CLIs' JSON event streams to events.jsonl (cli_traces.py); Codex read events come from shell commands that only print files. - native_prompt: a profile may run the harness's own prompt; reach refuses it. - answer_format: codex-review appends a findings block parsed from Codex's review text (answers.py), so review graders read it unchanged. - bin_companions: helper files mounted beside the executable (Codex's codex-code-mode-host, without which no tool call runs in the container). - repo reach now writes the profile's config_files, as repo run does. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #116 +/- ##
==========================================
- Coverage 92.74% 92.71% -0.03%
==========================================
Files 254 257 +3
Lines 32087 32633 +546
Branches 5344 5467 +123
==========================================
+ Hits 29759 30256 +497
- Misses 1560 1586 +26
- Partials 768 791 +23
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
Trace conversion can distort evaluation results, and reach configuration produces invalid JSON on Windows.
4 open issues
What changed in this PR
Adds Codex and Copilot CLI harnesses so repository evaluations can compare guidance across agents, including native review modes.
Changes:
- Adds four harness profiles and optional profile settings.
- Converts CLI traces and native review answers into evaluation artifacts.
- Applies profile configuration during reach probes, with tests and documentation.
| File | Description |
|---|---|
| dokimi/tests/test_repo_reach.py | Tests reach configuration generation. |
| dokimi/tests/test_repo_cli_harnesses.py | Tests profiles, converters, and runner integration. |
| dokimi/dokimi/schemas/harness_profile.v1.json | Adds optional harness capabilities. |
| dokimi/dokimi/repos/runner.py | Mounts companion binaries and converts answers. |
| dokimi/dokimi/repos/reach.py | Writes configuration and rejects native prompts. |
| dokimi/dokimi/repos/profiles/copilot.yaml | Adds Copilot execution profile. |
| dokimi/dokimi/repos/profiles/copilot-review.yaml | Adds Copilot review profile. |
| dokimi/dokimi/repos/profiles/codex.yaml | Adds Codex execution profile. |
| dokimi/dokimi/repos/profiles/codex-review.yaml | Adds Codex native review profile. |
| dokimi/dokimi/repos/harness_profile.py | Parses and validates new settings. |
| dokimi/dokimi/repos/events.py | Dispatches new trace formats. |
| dokimi/dokimi/repos/cli_traces.py | Normalizes Codex and Copilot events. |
| dokimi/dokimi/repos/answers.py | Converts native review findings. |
| dokimi/docs/repo-evaluation-contract.md | Documents profiles and evaluation behavior. |
| dokimi/docs/architecture.md | Describes new modules and harnesses. |
| dokimi/changelog.d/20261019_050000_nikolay.a.petrov_codex_copilot_profiles.md | Records harness additions and reach fix. |
Review effort: Balanced
…d, cd commands, and incomplete commands On Windows, str(Path) contains backslashes that produce invalid JSON escape sequences when inserted into configuration files. Use Path.as_posix() for both config_dir and support_dir to ensure forward slashes. shell_reads() now splits only on unquoted shell operators (&&, ||, ;, newline), preserving quoted arguments like sed -n '1p;2p'. Previously, it split all semicolons regardless of quoting context, breaking multi-range sed commands. Track directory changes from cd commands in shell scripts so subsequent file reads are resolved to the correct path. For example, 'cd sub && cat AGENTS.md' now correctly reports reading 'sub/AGENTS.md'. Preserve incomplete commands (item.started without item.completed) in Codex JSON traces. When an agent times out mid-command, record the command with rc: null per contract §4.1, instead of losing it entirely. Correlate started and completed items by command text to update rc on completion. Add regression tests for all three fixes. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Create tests/__init__.py and normalize test module imports to use explicit 'tests.' prefix. This resolves the mypy error where artifact_fixtures was found under two module names. All imports of test fixtures now consistently use the 'tests.' prefix (repo_eval_fixture, artifact_fixtures, test_process). Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Fix remaining imports from conftest, test_novelty_analyze, and test_repo_reach to use explicit 'tests.' prefix for consistency now that tests is a proper package. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Fix remaining import from test_novelty_gate in test_novelty_items.py to use explicit 'tests.' prefix for consistency. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
There was a problem hiding this comment.
🟡 Changes recommended
Overlapping identical Codex commands produce incorrect trace events used by graders.
7 open issues
Review effort: Balanced
…ay uv.lock 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.
Review at 27911d6. The profiles, schema widening and mounting logic hold up, but the trace converters have several wrong-result cases, and the Codex findings parser is fragile. Four things are worth fixing before merge; the inline comments have the inputs and outputs.
What I ran, on a Linux review host at 27911d6:
make ci-local: 19 passed, 0 failed, 4 skipped (docker/semgrep/jobs that do not run on a PR); changed-line coverage gate passes.- Reproductions for the code findings, with no model or network calls.
- A mutation run over the new tests: 36 of 83 mutants of
cli_traces.pypass the suite; two of the severity-mapping mutants inanswers.pypass too.
I agree with Copilot that Codex commands are matched by text, not item.id; it still holds (two overlapping make build items give rc [None, 0, 1]).
Checked, no issues
- Schema: the
harness_profile.v1.jsonchange only widens thetraceenum and adds optional keys, so no version bump is needed;schema_compatpasses against the base;bin_companionsnames reject..and/. - Docs: flags, keys, section numbers and the profile table match the code; the changelog fragment is present; no
tests/__init__.pyoruv.lockremains after the reverts. - Mounts and reach: a missing companion fails with a clear error, a symlinked
--binfinds the companion beside the real file, andrepo reachrefusesnative_promptprofiles before--dry-run.
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.
The refactor and the new test hold, and the open thread is closed. Two test gaps are worth closing before merge, and there is one nit.
What I ran, on a Linux review host at 0100221:
make ci-local: 28 passed, 1 failed, 7 skipped; 7,345 tests passed, 1 skipped; changed-line coverage 95% (555 lines). The one failure ispip-audit(gitpython 3.2.0, CVE-2026-114145, fix in 3.2.1). The PR does not touch the lock files, so it comes from the base.- A differential run of
adfe925and0100221againsta22d109: 132,893 inputs (random Codex and Copilot streams, generated shell scripts throughshell_reads,_split_scriptand_sed_files, andparse_profileinputs) give identical transcripts, events andProfileErrormessages. The refactor changes no behaviour. - A mutation run over the code touched by
adfe925(55 mutants; 23 passtest_repo_cli_harnesses.py,test_repo_reach.pyandtest_repo_reach_edges.py). The two that matter are inline.
Findings that do not anchor inline:
- Nit:
docs/preparing-a-repository.md:270still says onlyclaude-codeis built in. The checklist item reads "dokimi repo reach(onlyclaude-codeis built in; others need aharness_profile.v1file)". The PR now shipscodex,codex-review,copilotandcopilot-review, and line 208 says so. Lines 193-195 say "no loading behavior was measured" for Codex and Copilot, while the profiles'guidance.notesrecord measurements. Suggestion: change the checklist item to "dokimi repo reach(claude-code,codex,copilotandcopilot-revieware built in;codex-reviewruns its own prompt and is refused; others need aharness_profile.v1file)". (outside this diff)
Checked, no issues
- Thread on the unknown exit code:
rc not in (None, 0)changed torc != 0fails both parametrizations oftest_codex_command_with_an_unknown_exit_code_keeps_its_read(2 failed). - Refactor:
_Splitter,_Walk,_SedScan, theby_idpop and the CopilotstartNone check behave as before.ruffis clean onrepos/;mypyreports nothing underrepos/. - Profiles and schema: all five built-in profiles load and build argv for
none,readandall; the schema change only widens;schema_compatpasses; the changelog fragment is present; no new doc page, so no nav change.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
|
Re-checked at 0100221 on Linux: the original inputs of every earlier point (ours and every open thread) now pass, including real codex-cli 0.163 and Copilot CLI 1.0.100 against a local stub; adfe925 is behaviour-preserving on a 60k-input differential run against 9667bb4; the full suite, ruff, mypy and bandit are clean at the head and at the head merged into main. |
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to 25dd17d, which fix the nit from my review:
25dd17d:dokimi/docs/preparing-a-repository.md:270: the checklist item now listsclaude-code,codex,copilotandcopilot-reviewas built in and sayscodex-reviewis refused.
make ci-local at 25dd17d: 28 passed, 1 failed, 7 skipped.
Not approving yet. Still open: the two risks from my review.
|
Review-body items from 1f8d127 and b4457f9: contract §9 now names bin_companions beside the mounted executable (d479a0f); token accumulation over several turn.completed is pinned by test_codex_usage_is_summed_and_a_missing_or_zero_usage_is_not_a_measured_zero (earlier commit); the duplicated SUPPORT_HOST and the _PRINTERS spacing are already fixed at the head (reach.py imports SUPPORT_HOST from runner.py). |
… fallback; name bin_companions in contract §9 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
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. Both open threads are fixed: test_codex_failed_command_drops_only_its_own_reads and the two new Copilot exit-code tests each fail under the mutants from my threads. One nit inline.
What I ran, on a Linux review host at a2fa4c2:
make ci-local: 29 passed, 0 failed, 7 skipped (docker, semgrep, micromamba and jobs that do not run on a PR); 8,338 tests passed, 2 skipped; changed-line coverage 95% (555 lines).- Reproductions for the code finding, with no model or network calls.
- A mutation run over the code behind the new tests (18 mutants of
cli_traces.py; the wideneddroppedset, both swappedsuccessbranches,Nonemapped to0, and!= 0for the Copilot result are all killed; two guards on non-dictshellExecutionand non-intexitCodesurvive, which I did not count as findings).
Checked, no issues
- Docs: contract §9 now names
bin_companionsbeside the mounted executable, and §6 andharness_mountsagree. - Schema and profiles: the schema change only widens,
schema_compatpasses, the changelog fragment is present, and the profile argv shapes match the contract table.
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 3cdf566, which fix the nit from my review:
3cdf566:dokimi/dokimi/repos/answers.py:32:_FINDING_MARKnow anchors at column 0, so indented body lines no longer count as findings, and_CODEX_FINDINGaccepts a trailing\r, so CRLF output parses.
make ci-local at 3cdf566: 29 passed, 0 failed, 7 skipped.
GitHub CI has not run on these commits: the branch conflicts with main, and Actions runs no workflow on a conflicting PR. CI will check them once main is merged in.
Not approving until CI passes at this head.
| _SEVERITY = {"P0": "blocker", "P1": "major", "P2": "minor", "P3": "minor"} | ||
| # the title may hold " — ": the path part is what follows the last one and holds none | ||
| _CODEX_FINDING = re.compile(r"^- \[(P\d)\] (.+) — ([^\s—][^—]*?):(\d+)(?:-(\d+))?[ \t\r]*$", re.M) | ||
| _FINDING_MARK = re.compile(r"^(?:[-*]\s*)?\[P\d\]", re.M | re.I) |
There was a problem hiding this comment.
Anchoring the marker at column 0 brings back the clean-review case for indented findings. "Findings:\n - [P1] Leak — /ws/a.c:2\n body\n" now parses 0 findings and counts 0 markers, so the answer gets json [] and is graded as a clean review; before 3cdf566 it got no block. Please count a marker by the header shape instead of the column, for example any line that matches r"\[P\d\].* — \S+:\d+", and add a test for the indented case (and one for CRLF, which this commit also changed without a test).
There was a problem hiding this comment.
Fixed in e60c7d0. A marker is now counted by the header shape, indented or not, and an indented header is parsed as a finding, so it gets a findings block and is not graded as a clean review.
Tests: test_convert_answer_reads_an_indented_finding_header (the indented case) and test_codex_review_findings_accepts_crlf_line_endings (CRLF).
There was a problem hiding this comment.
The indented case is fixed, thanks. A numbered list still reads as a clean review: "1. [P1] Leak — /ws/a.c:2\n body\n" parses no finding and counts no marker, so it gets ```json []. _BULLET_MARK only knows - and *; (?:[-*]|\d+[.)]) would make it a marker, and would also let the body stop @antonwolfy suggests on answers.py:69 catch a numbered header inside a body.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> # Conflicts: # dokimi/dokimi/repos/harness_profile.py # dokimi/dokimi/repos/runner.py
Anchoring the marker at column 0 (3cdf566) let an indented ' - [P1] title — path:line' header through: it parsed no finding and counted no marker, so the answer got a clean json [] block. A marker is now a column-0 [Pn] line (as before) or an indented line in the header shape (ends with ' — path:line'). A bullet inside a body (' - [P2] sub bullet') still does not count. Tests: an indented header is kept without a block; a [Pn] inside a body keeps the one parsed finding's block; CRLF output parses. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
A review's findings can sit in an earlier agent message than the last one. Only the last message was converted, so the findings were dropped and a clean json [] block was appended. The block is now built from every agent message (Transcript.agent_messages). A finding header is read the same way whether it is indented or not, so the marker count and the parser agree: an indented header is a finding, and a path-bearing bullet in a body is parsed instead of counted and dropped. A path no longer runs across a line break, so a header with no :line does not swallow the next body line. Shell reads: NAME=value words, env with its options, and the command's basename (/bin/cat) no longer hide a read; stderr sent to a file (2>err.txt) does not change what a command prints, so the read counts. A 2>/dev/nullfile target is a file, so cat a 2>/dev/nullfile reads a. Tests: the shell-read table rows for each case, the indented and path-bearing header cases, the path-across-line-break case, a Codex trace with two agent messages, and a runner run whose findings are in the first of two messages. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Thanks for the review. Each open item, against e60c7d0:
Open question for the owner, not changed in this push: does Codex's |
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at e60c7d0. The messages= change in convert_answer can both lose a real review and double-count one, and the new shell-read rules add two wrong results. The inline comments have the inputs and outputs. The first three are worth fixing before merge.
What I ran, on a Linux review host at e60c7d0:
make ci-local: 29 passed, 0 failed, 7 skipped (docker, semgrep, micromamba and jobs that do not run on a PR); 8,541 tests passed, 2 skipped; changed-line coverage 95% (576 lines).- Reproductions for the code findings, with no model or network calls, run against the PR head and against 3cdf566.
- A mutation run over the code changed since 3cdf566 (
convert_answer,_FINDING_MARK,_STDERR_FILE,_command_words, the basename match).
Checked, no issues
- Plumbing:
agent_messagesis filled only bycodex_json; Claude and Copilot transcripts leave it[], soconvert_answerfalls back to[answer], and a cell withanswer_formatunset is unchanged. - Parser cost:
_CODEX_FINDINGand_FINDING_MARKrun in under 10 ms on 32,000 repetitions of five adversarial shapes. - Shell reads:
cat a=breadsa=b;FOO=1 cd x && cat fgivesx/f;cat a 2>&1 >out,cat a 12>xandcat "a 2>b"give no read, no read anda 2>b. - Repo rules: schema is widened in place,
schema_compatpasses, the growth ratchet and docs-reference tests pass, the changelog fragment is present.
…codex-copilot Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
… builtin, quote-aware stderr redirect - convert_answer counts finding markers only in the last message and in messages that hold a parsed finding, so a [Pn] narration line in an earlier message does not cancel the block; a finding repeated in two messages is kept once. - an unparsed [Pn] header in any shape (indented without a path, bold) outside a parsed finding's body is a marker: no block. - cd/pushd/popd match words[0] exactly; /bin/cd and env cd do not move the directory; a path-qualified printer counts only under /bin, /usr/bin, /usr/local/bin. - the stderr-to-file redirect is matched with quoted text masked, so a quote glued to it is part of its word; 2> with a space is handled. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…s the block Only a bare [Pn] line in an earlier message with no parsed finding is narration; a bullet-shaped marker there still counts, so a drifted review followed by a short final message is not graded as clean. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Review at e60c7d0 (antonwolfy), against 6b2608f (main merged in at the head~2 merge commit):
Details and test names are in the threads. |
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 6b2608f. All five earlier threads are fixed, but the de-duplication added for the second thread introduces a new wrong result, and one nit. Both inline.
What I ran, on Linux at 6b2608f:
make ci-local: 26 passed, 2 failed, 7 skipped; 9,157 tests passed, 2 skipped; changed-line coverage 95% (616 lines). The two failures are the gitleaks download (the proxy returned 504) and the commit scan that needs it, not this PR.- Reproductions for the code findings, with no model or network calls.
- A mutation run over
convert_answer,_finding_spansand the new shell-read rules.
Checked, no issues
- Earlier threads:
[P1] means a blockerin an earlier message no longer cancels the block; the same finding in two messages is counted once;/bin/cd x && cat fandenv cd x && cat fgive['f'];cat 'a'2>xgives[],cat a 2>x'y'andcat a 2> errgive['a']. - Shell reads:
env cat f,env -i A=1 cat f,/usr/bin/cat astill read;./cat aandtools/sed -n 1p fdo not. - Repo rules: schema widened in place,
schema_compatpasses, growth ratchet and docs-reference tests pass, changelog fragment present, new tests write only undertmp_path.
| if line.strip(): # a blank line may separate paragraphs of the body | ||
| if not line[0].isspace(): # the body is indented; a flush line is prose after the findings | ||
| break | ||
| body_lines.append(line.strip()) |
There was a problem hiding this comment.
Nit: an indented, drifted [Pn] header right after a parsed finding is swallowed as body text, so the block is emitted with a finding missing.
The body loop takes every indented line, and _unparsed_marks removes the body from the marker count. Input: "- [P1] A — /w/a.py:3\n body\n - [P2] B - /w/b.py:4\n body\n" (hyphen instead of the em dash in B). convert_answer("codex-review", T, "/w") returns a block with 1 finding (A); B is lost. The same B flush-left is correctly withheld.
This is a deeper case than the first thread's: the same input gave one finding at e60c7d0, so it is not a regression. test_convert_answer_does_not_count_a_bullet_inside_a_finding_body only uses - [P2] sub bullet, which has no path and reads as prose.
Suggestion: in _finding_spans, stop the body at an indented line that matches _BULLET_MARK and has a — or :<digits> tail (it looks like a header), so _unparsed_marks counts it; keep plain - [P2] sub bullet as body. Add the input above as a test.
A finding listed twice in one message counted as two markers but was kept once after dedup, so a parsed review got no findings block and read as missing. Only markers outside parsed findings withhold the block now. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at 6b2608f. The earlier points hold as fixed: an indented finding header is parsed, the shell-read cases from the first rounds give the right paths, and Codex token counts are no longer a measured zero. What is left is how findings are taken from several agent messages and how Codex tokens are stored; details are in the line comments.
What I ran, at 6b2608f:
make ci-local --offline: 9,103 passed, 0 failed, 56 skipped; changed-line coverage 95%;schema_compatpasses. The branch merges intomainwith no conflict.- Reproductions of the earlier inputs and of the new
convert_answer(messages=...)andshell_readsrules, with no model or network calls.
Checked, no issues
- Earlier rounds: conditional
cd,pushd, subshells, shell comments, all-zero Codex usage and unknown priorities still give the results from their threads. - Shell reads:
NAME=valuewords,env,/bin/cat,/bin/cdand2>err.txt/2> err.txtgive the right reads. - Repo rules: the schema change only widens, the changelog fragment is present, new files have the license header.
| def _codex_usage(out: Transcript, usage: dict[str, Any]) -> None: | ||
| out.num_turns = (out.num_turns or 0) + 1 | ||
| fresh, produced = int(usage.get("input_tokens") or 0), int(usage.get("output_tokens") or 0) | ||
| if fresh or produced: # a missing or all-zero usage is not a measured zero: the counts stay None | ||
| out.input_tokens = (out.input_tokens or 0) + fresh | ||
| out.output_tokens = (out.output_tokens or 0) + produced | ||
|
|
There was a problem hiding this comment.
Codex usage has cached_input_tokens, and with OpenAI's usage rules input_tokens already includes them, while Claude cells report cache reads apart in cache_read_tokens. A turn with input_tokens 500 and cached_input_tokens 400 is stored as input_tokens=500, cache_read_tokens=None and usage_scope=None, though the contract says usage_scope null means the harness reported no tokens. Please set usage_scope to "main" when usage is seen, and, once a run with cache hits confirms that input_tokens includes the cached part, store input_tokens - cached_input_tokens and cached_input_tokens as cache_read_tokens.
| raise ValueError(f"unknown answer format {fmt!r} (known: {', '.join(ANSWER_FORMATS)})") | ||
| findings: list[dict[str, Any]] = [] | ||
| seen: set[tuple[Any, ...]] = set() | ||
| marked = 0 | ||
| for i, t in enumerate(texts): | ||
| parsed = _CONVERTERS[fmt](t, workdir) | ||
| # every marker counts in the last message and in a message that holds a finding; in an earlier | ||
| # message with none, only a [Pn] bullet does: a bare "[P1] means a blocker" line is narration | ||
| full = parsed or i == len(texts) - 1 | ||
| marked += len(parsed) - _repeats(parsed, seen) + _unparsed_marks(t, _FINDING_MARK if full else _BULLET_MARK) | ||
| for f in parsed: | ||
| key = (f["file"], f["line"], f["rule"], f["message"]) | ||
| if key not in seen: # a review posted in an interim message and repeated at the end counts once | ||
| seen.add(key) | ||
| findings.append(f) | ||
| if marked > len(findings): # format drift must not read as a clean review |
There was a problem hiding this comment.
Findings from an earlier agent message are graded even when the final message drops them.
With messages=["- [P1] Maybe leak — /ws/a.c:2\n thinking", "No issues found."] the block holds the P1 finding, so a draft the review dropped is graded as a reported bug. The dedup key also holds message, so the same finding with an edited body in a later message is kept twice ([draft, final] gives 2 findings). This is apart from @antonwolfy's point on the same loop (identical findings in one message withhold the block); a fix for both can share one key.
What Codex trace showed findings in an earlier message than the last one?
Suggestion: if the final message holds the review, read only it, and use the earlier messages only when it has no finding and no marker. Otherwise dedup on (file, line, rule).
| does not read a review that reported a problem as a clean one. | ||
| """ | ||
| texts = list(messages) or [answer] | ||
| if fmt is None or failed or not any(t.strip() for t in texts): |
There was a problem hiding this comment.
An empty final message no longer stays a missing review when an earlier message has text: convert_answer("codex-review", "", messages=["I will inspect the commit first.", ""]) appends json [] . Before messages= an empty answer was returned as is. Please keep that rule: return the answer unchanged when it is empty and no finding was parsed.
| words, via_env = _command_words(words) | ||
| if not words: | ||
| return [] | ||
| name = words[0] |
There was a problem hiding this comment.
Nit: "builtin cd sub; cat a" and "command cd sub; cat a" give ['a'], but both run the cd builtin, so the read is sub/a. "env --chdir=sub cat a" also gives ['a']. Treating builtin/command before cd like cd, and env -C/--chdir as an unknown directory, would cover them.
|
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 8c9986e. The repeated-finding fix is right: G + G in one message now keeps the block with one finding. The review is not ready: @vlad-perevezentsev's open points on convert_answer are not fixed, and the second thread is still open.
What I ran, on a Linux review host at 8c9986e:
make ci-local: 26 passed, 2 failed, 7 skipped; 9,160 tests passed, 2 skipped; changed-line coverage gate passes. The two failures are the gitleaks download and the commit scan that needs it, which this PR does not touch;gitleaks detectoverorigin/main..HEADby hand reports no leaks (42 commits).- Reproductions of the earlier inputs and of the open points, with no model or network calls.
- A mutation run over
convert_answerand the new test (15 mutants). The new test kills the dedup,unparsedaccumulation and span mutants.
I agree with @vlad-perevezentsev's points on a numbered 1. [P1] header reading as a clean review, on a draft finding from an earlier message being graded when the final message drops it, and on an empty final message getting [] appended; all three still hold at this head. New evidence: convert_answer("codex-review", "", messages=["I will inspect the commit first.", ""]) returns the text with ```json [], and messages=["- [P1] Maybe leak — /w/a.c:2\n thinking", "No issues found."] yields one finding.
@vlad-perevezentsev's question on whether Codex input_tokens includes cached input (usage_scope, cache_read_tokens) waits for the author.
Still open from my earlier review:
dokimi/dokimi/repos/answers.py:69: an indented, drifted[Pn]header right after a parsed finding is taken as body text."- [P1] A — /w/a.py:3\n body\n - [P2] B - /w/b.py:4\n body\n"still gets a block with 1 finding; the same B flush-left is withheld.
Checked, no issues
- Earlier thread:
G + Gin one message, in a later message, and after an earlier message each keep the block with one finding. - Repo rules: schema unchanged in the delta,
schema_compatpasses, changelog fragment present, mypy and ruff clean.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Summary
repo runandrepo reachhad one built-in harness, Claude Code. This adds the Codex CLI and the Copilot CLI, including each CLI's own review mode, so the same task can be compared across guidance arms × harness × model.harness_profile.v1is widened in place with three optional keys:Things that only show up when running the real CLIs:
codex review --commitrefuses a prompt.codex-code-mode-hostbeside its binary, so a single-file mount leaves it with no tools.--available-toolslist leaves every tool on, sononenames a tool that doesn't exist.--binhas to name the native binary.Not here: hosted PR reviewers (Copilot code review,
@codex review, CodeRabbit). They are not CLIs and need a separate PR-replay adapter.Evidence
Before:
repo run --harness codexfails withno harness profile 'codex' (built-in: claude-code). A profile with a Codex provider inconfig_filesprobed viarepo reachhitwss://api.openai.comwith a 401, because reach never wrote the file.After: live runs on oneDAL
review_seeded_fc(forkdev/agent-evalsc4e2667f5, imageagent-benchmark/onedal:a28e8a6c33a7, gpt-6-luna through a LiteLLM proxy,guidance:raw, 1 rep):The first codex-review run, without
bin_companions, answered "the repository command tool failed to start" and got recall 0.0.Live
repo reachon oneDAL main 7f6d9f86 (3 scenarios, gpt-6-luna):Both CLIs load AGENTS.md from the git root down to cwd, and reading a file in a subtree does not pull in that subtree's AGENTS.md. In
read=Codex skipped the read, because the reach ask says "use the Read tool" and Codex has no tool by that name. Rewording the ask is left for a follow-up: it would change Claude's recorded ask too.Tests:
tests/test_repo_cli_harnesses.py(29 tests) covers:shell_reads, including pipes, redirects andsed -i;codex_review_findings;repo runcell that grades a native review answer;bin_companionsmounted, or a stop before any agent runs.test_repo_reach.pyadds a test that a profile'sconfig_filesreach the probe.make ci-local: every row passes excepttest. There, every PR-touched test passes, but 1–3 unrelated tests fail at random, a different one each run:test_processPopen ResourceWarning,test_container_agent"AF_UNIX path too long". This happens under 224 xdist workers on a shared box.pytest -n 32gives 5208 passed, 2 skipped. Those two files alone: 149 passed.Merge Danger
Door: two-way
New profiles and optional schema keys only. Profiles that already exist are left as they were.
schema_compatpasses against the base.Blast Radius:
repo reachEvery reach run now writes the profile's
config_files. Forclaude-codethat adds the no-background hooksettings.jsonto each scenario's config dir. Reach scenarios run with toolsnone/read, so the Bash hook never fires. Onlyrepo run/repo reachread the new keys.🤖 Generated with Claude Code