You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
repo run and repo reach had 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.v1 is widened in place with three optional keys:
native_prompt: true # the harness's own prompt; invoke has no {prompt}answer_format: codex-review # "- [P1] title — path:12-14" -> ```json [{file, line, severity, rule, message}]bin_companions: [codex-code-mode-host] # mounted beside /eval/harness/codex
repo run cell (codex-review)
agent container: /eval/harness/codex + codex-code-mode-host
codex exec review --commit HEAD --json # task prompt not passed
convert("codex-json") -> events.jsonl
convert_answer("codex-review") -> answer.txt # text kept, ```json block appended
grade # review grader unchanged
Things that only show up when running the real CLIs:
codex review --commit refuses a prompt.
Codex 0.160 runs every tool call through codex-code-mode-host beside its binary, so a single-file mount leaves it with no tools.
An empty Copilot --available-tools list leaves every tool on, so none names a tool that doesn't exist.
npm installs both CLIs as Node launchers; --bin has 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 codex fails with no harness profile 'codex' (built-in: claude-code). A profile with a Codex provider in config_files probed via repo reach hit wss://api.openai.com with a 401, because reach never wrote the file. After: live runs on oneDAL review_seeded_fc (fork dev/agent-evals c4e2667f5, image agent-benchmark/onedal:a28e8a6c33a7, gpt-6-luna through a LiteLLM proxy, guidance:raw, 1 rep):
harness
recall
events
notes
codex
0.50
12 command, 9 read
codex-review
0.375
7 command, 8 read
same recall as an out-of-tree harness on 10-04
copilot-review
0.625
5 command, 14 read, 13 tool
11 turns
The first codex-review run, without bin_companions, answered "the repository command tool failed to start" and got recall 0.0.
Live repo reach on oneDAL main 7f6d9f86 (3 scenarios, gpt-6-luna):
cwd=. cwd=cpp/daal read=cpp/daal
codex AGENTS.md L L L
cpp/AGENTS.md . L .
cpp/daal/AGENTS . L .
copilot AGENTS.md L L . <- model refused to list tokens (prose answer)
cpp/AGENTS.md . L .
cpp/daal/AGENTS . L .
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.
shell_reads, including pipes, redirects and sed -i;
both converters, on shapes taken from real traces;
codex_review_findings;
a repo run cell that grades a native review answer;
bin_companions mounted, or a stop before any agent runs.
test_repo_reach.py adds a test that a profile's config_files reach the probe.
make ci-local: every row passes except test. There, every PR-touched test passes, but 1–3 unrelated tests fail at random, a different one each run: test_process Popen ResourceWarning, test_container_agent "AF_UNIX path too long". This happens under 224 xdist workers on a shared box. pytest -n 32 gives 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_compat passes against the base.
Blast Radius:repo reach
Every reach run now writes the profile's config_files. For claude-code that adds the no-background hook settings.json to each scenario's config dir. Reach scenarios run with tools none/read, so the Bash hook never fires. Only repo run / repo reach read the new keys.
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>
❌ Patch coverage is 85.75342% with 52 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.21%. Comparing base (6e695ce) to head (b4457f9). ⚠️ Report is 6 commits behind head on main.
…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>
The reason will be displayed to describe this comment to others. Learn more.
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.
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.py pass the suite; two of the severity-mapping mutants in answers.py pass 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.json change only widens the trace enum and adds optional keys, so no version bump is needed; schema_compat passes against the base; bin_companions names reject .. and /.
Docs: flags, keys, section numbers and the profile table match the code; the changelog fragment is present; no tests/__init__.py or uv.lock remains after the reverts.
Mounts and reach: a missing companion fails with a clear error, a symlinked --bin finds the companion beside the real file, and repo reach refuses native_prompt profiles before --dry-run.
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking: the severity mapping has no test.
Mutation check: changing "P3": "minor" to "major", and separately changing the .get(..., "minor") default to "major", both pass the full test_repo_cli_harnesses.py + test_repo_reach.py run (101 passed). The contract documents P2/P3 -> minor. Dropping the "\n\n```json" separator to "\n```json" also passes.
Test to add: one answer with P0, P1, P2, P3 and an unknown P9 findings, asserting all five severities and the exact separator before the block.
The cli_traces.py tests have the same shape of gap: 36 of 83 mutants survive, including no || split, no newline split, no double-quote handling, the sed -i guard, the head/tail -n skipping, and token accumulation over more than one turn.completed.
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 3264d74: test_convert_answer_maps_every_severity_and_separates_the_block covers P0-P3 and unknown P9 and the exact \n\n```json separator. For cli_traces I added cases for ||, newline-free & split, double quotes/expansion, sed -i/-e/-E, $ addresses and cwd; head/tail -n skipping and multi-turn token accumulation are still not pinned (not changed here).
… error flags
- Use item.id for command correlation (handles overlapping commands with same text)
- Handle cd options (-L, -P, -) and reset cwd after || operator
- Normalize paths with posixpath.normpath to handle . and .. components
- Improve sed option parsing to handle -e, -f, -E, -s and -i correctly
- Fix is_error flag so bare error doesn't clear turn.failed
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The reason will be displayed to describe this comment to others. Learn more.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
The reason will be displayed to describe this comment to others. Learn more.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
The tests/ directory was added as a package (tests/__init__.py) in this PR, but sibling imports from test modules and conftest still use top-level names. Update all imports to use the tests.* namespace to match the package structure and avoid ModuleNotFoundError during test collection.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Line 259 annotates event: dict[str, Any], and line 274 reassigns without annotation. Line 283 should also reassign without annotation to avoid mypy's no-redef error when the function handles both item.started and item.completed paths.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Preserve blank-line-separated paragraphs in finding messages
dokimi/dokimi/repos/answers.py:47
A blank line between indented paragraphs ends the finding body here, dropping subsequent paragraphs from the JSON message consumed by graders. Continue across blank lines and stop at the next finding or unindented prose, so the full explanation survives conversion. Add a multi-paragraph regression.
Reject incomplete findings when marker parsing fails
dokimi/dokimi/repos/answers.py:71
When finding markers cannot be parsed, this warning still allows an empty or incomplete findings array to be appended. The lowercase/hyphen headers in the new test therefore become a zero-finding review, and partial parses silently omit defects. Raise a conversion error instead: run_cell() already catches it and records a failed outcome rather than grading incomplete findings. Update the warning-only test to expect failure and cover partial parses too.
Ignore shell comments when parsing commands and operands
dokimi/dokimi/repos/cli_traces.py:192
Ignore shell comments before splitting operators or extracting operands. cat other.txt # AGENTS.md records a read of AGENTS.md, and true # ; cat AGENTS.md records a read from a command the shell never executes. Both converters use these paths, so reach.classify() can misclassify automatically loaded guidance as fetched. Skip unquoted, unescaped comments through the next newline while preserving literal # characters in words; add regressions for both cases.
The reason will be displayed to describe this comment to others. Learn more.
Review at b4457f9. The fixes since 27911d6 hold: cd handling, the sed option parsing, item.id correlation, the turn.failed flag and the findings parser all give the right result on the cases from my earlier threads. What is left is the shell-read heuristics at the edges, one place where a failed Codex run becomes a clean review, and the test gaps.
make ci-local: 19 passed, 0 failed, 4 skipped (docker, semgrep and jobs that do not run on a PR); 5,235 tests passed, 1 skipped; changed-line coverage 92% (365 lines).
Reproductions for the code findings, with no model or network calls.
A mutation run over cli_traces.py (129 mutants, 65 pass test_repo_cli_harnesses.py) and over runner.py / reach.py.
Findings that do not anchor inline:
Two mutants in the new plumbing pass the whole suite. Replacing real.parent with opts.harness_bin.parent in the companion lookup (runner.py:345), and removing scenario.cwd from the convert() call in reach.py:389, both pass test_repo_cli_harnesses.py, test_repo_reach.py and test_repo_reach_edges.py (138 passed). No test uses a symlinked --bin (the npm layout the PR description names), and no reach test runs a Codex or Copilot trace from a cwd=sub scenario. Add one test for each.
I agree with @copilot-pull-request-reviewer's points on the duplicated SUPPORT_HOST (runner.py:65, reach.py:55) and on the _PRINTERS : spacing.
Still open from my earlier review:
dokimi/tests/test_repo_cli_harnesses.py: the head/tail-n skipping and the token accumulation over more than one turn.completed are still not pinned (you said so in the thread). Other cli_traces.py mutants also survive; see the nit and risk comments for the ones that matter.
Checked, no issues
cd and ||:cd - && cat X, cd missing || cat X, cd sub && cd inner && cat X and cat ./X give no read, no read, sub/inner/X and X.
Codex events: a turn.failed followed by an error keeps is_error True; two overlapping make build items with different ids give two commands with their own exit codes; a start with no completion keeps rc: null.
Findings parser: a title with —, a.py:12-14 ranges, a trailing Overall: line and the marker/finding warning behave as the thread asked.
Schema and docs: optional keys only, no version bump needed; schema_compat passes; the changelog fragment is present; mkdocs build passes.
The reason will be displayed to describe this comment to others. Learn more.
Any redirect drops the whole read, including 2>/dev/null and 2>&1.
shell_reads() at the PR head:
cat a 2>&1 -> []
cat a 2>/dev/null -> []
sed -n '1,5p' a 2>/dev/null -> []
Agents add 2>/dev/null to read commands all the time. _computed() treats every < or > outside quotes as a write, so repo reach records no read and reports the guidance as not fetched. The test at test_repo_cli_harnesses.py:110 pins cat a 2>&1 -> [], so this is locked in.
Suggestion: strip \d*>&\d+, \d*>/dev/null and &>/dev/null before the check, and treat only a redirect with a real target (> f, >> f, < f) as not-a-read. Change the pinned case and add the /dev/null ones.
The reason will be displayed to describe this comment to others. Learn more.
Nit: option values of head/nl are recorded as files.
head --lines 5 a -> ['5', 'a'], head -qn 5 a -> ['5', 'a'] (line 127 skips the value only for the exact -n/-c). nl -w 3 a records 3. The extra operand is a read of a file named 5, which only matters if such a path exists, hence a nit.
Suggestion: skip the value of --lines/--bytes and of short clusters that end in n/c, and of nl -w/-s/-b/-n.
The reason will be displayed to describe this comment to others. Learn more.
A failed or empty Codex run is turned into a clean review.
convert_answer("codex-review", "", "/ws") returns '\n\n```json\n[]\n```\n'. For {"type":"turn.failed","message":"x"} the transcript has is_error=True and answer='', and answer.txt still gets the [] block (runner.py:191). The Claude path leaves answer.txt empty in that case. agent.is_error is recorded but does not reach the grader, so a task that scores "no findings" on a clean patch passes with an agent that crashed or timed out.
Suggestion: return answer unchanged when it is empty (or is_error), so a missing review stays a missing review. Add a test with an empty answer.
This branch has not been deployed
No deployments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
antonwolfy
left a comment
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