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 93.50% 93.53% +0.02%
==========================================
Files 273 276 +3
Lines 35569 36153 +584
Branches 6040 6174 +134
==========================================
+ Hits 33260 33816 +556
- Misses 1492 1508 +16
- Partials 817 829 +12
🚀 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.
scripts/check_comment_blocks.py (from main) caps a comment block at two lines and covers YAML. The rationale stays in the contract's §9 profile table and routing paragraph; the comments keep the constraint and point there. Thread: PRRT_kwDOVBg7g86x0azJ Thread: PRRT_kwDOVBg7g86x0azM Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The harness test file passed the 1000-line growth cap (scripts/check_growth.py, from main). The traces section moves whole, unchanged. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…tripped `cat \" 2>&1 "z"` was masked as if \" opened a quoted string, so the fd dup stayed in the command and was recorded as a read of `2>&1`. Backslash escapes are now masked before quotes are matched. Thread: PRRT_kwDOVBg7g86x2UDw Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…gests Thread: PRRT_kwDOVBg7g86x2UD6 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…schema Tightening the name pattern of harness_profile.v1 in place fails schema_compat against main, which already ships v1. The schema is back as it was, and parse_profile refuses the two names with a ProfileError. Thread: PRRT_kwDOVBg7g86x2UD- Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…iles are Thread: PRRT_kwDOVBg7g86x0azJ Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
80736ee to
aa9922b
Compare
…com/napetrov/dokimi into HEAD Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> # Conflicts: # dokimi/dokimi/repos/answers.py # dokimi/dokimi/repos/profiles/codex-review.yaml # dokimi/dokimi/repos/profiles/codex.yaml # dokimi/dokimi/repos/profiles/copilot-review.yaml # dokimi/dokimi/repos/profiles/copilot.yaml # dokimi/tests/test_repo_cli_harnesses.py # dokimi/tests/test_repo_cli_pins.py
Thread: PRRT_kwDOVBg7g86x21H4 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…e body-split test Thread: PRRT_kwDOVBg7g86x21H4 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
This review has no body text, and each of its inline threads has its own answer in the thread, so nothing in the review itself needs a change. |
|
This review has no body text, and each of its inline threads has its own answer in the thread, so nothing in the review itself needs a change. |
|
Both inline comments of this review (comment blocks in the profiles) are fixed in 902d3d6 and answered in their threads. |
|
(a) Waiting on a decision before I change this: an agent run can have an earlier message with a |
|
This review has no body text, and its inline thread has its own answer in the thread, so nothing in the review itself needs a change. |
| `prompt_suffix`), the agent never sees the task prompt, and `repo reach` | ||
| refuses the profile. Use such a profile only on tasks whose grader reads what | ||
| it answers, which `answer_format` provides. | ||
| - `answer_format` converts the final answer before grading. `codex-review` |
There was a problem hiding this comment.
The answer_format text says codex-review appends a findings block, but convert_answer (repos/answers.py) leaves the answer unchanged in three cases: the run failed, the answer is empty, or the text has more [Pn] markers than parsed findings (format drift). It also sets severity to null for a priority outside P0–P3. A reader can't tell from the doc that a missing block means "no review" or "drifted format", not a clean review. A clean review gets []. Add one sentence covering these cases.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 5d4eff6. The changes since d9334b8 hold up: the \n-only split of finding bodies, the companion digests in the execution id, the Copilot failed-read drop and the OPENAI_API_KEY removal all give the right result. Two test gaps in the new code decide which file a read event names and when a finished cell is reused; I'd close them before merge. Two doc nits.
What I ran, on a Linux review host at 5d4eff6:
make ci-local: 29 passed, 0 failed, 7 skipped (docker, semgrep and jobs that do not run on a PR); 10,897 tests passed, 4 skipped; changed-line coverage 96% (656 lines).- Reproductions for the code findings, with the LLM transport mocked.
- A mutation run over the code changed since d9334b8 (
cli_traces.py,runner.py,harness_profile.py,answers.py).
Findings that do not anchor inline:
- Nit: the contract's list of what the execution
idhashes is stale.docs/repo-evaluation-contract.md:1349-1353(outside this diff) does not nameanswer_formator thebin_companionsdigests, whichexecution_idnow includes, and the refusal list on the next lines has the same gap. Suggestion: addanswer_formatand the SHA-256 of eachbin_companionsfile to the hashed list, andanswer_formatto the edits that cause a refusal.
Commits without a sign-off: six non-merge commits carry no Signed-off-by (80736ee, b5f7d86, f992e0b, 2c2fbbf, b637bd9, aa715af). GitHub's DCO check fails on them, so the merge waits for @napetrov to sign them off.
Checked, no issues
- Redirects and quotes:
cat a 2>/dev/null,&>/dev/nulland"a>/dev/null"givea,aand a single quoted word; a\"outside quotes opens no quote. - Computed
cd:builtin cd "$D" && cat a,command cd "$D",FOO=1 cd "$D"and{ cd "$D"; } && cat afromsubgive no read. - Copilot: a PowerShell command records no reads, a
viewwithout a path records no event, a failedviewand a failed one-filecat adrop their reads, a failedcat a bkeeps them. - Execution id: the
claude-codeid is4c4abf90ade2at the head and at the base; removing a companion from a batch with a finished cell stopsrepo runwithneeds ... beside its executablebefore any agent starts. - Profiles and schema: both Codex profiles parse with the
env_allowthe comments describe, no doc or test still saysOPENAI_API_KEYis forwarded, the schema change only widens, andschema_compatpasses.
| rough = part.split() | ||
| while rough and (rough[0] in _KEYWORDS or rough[0] == "{"): | ||
| rough = rough[1:] | ||
| rough, via_env, _ = _command_words(rough) | ||
| if rough[:1] and rough[0] in _DIRS and not via_env: # ``builtin cd "$D"``: where it went is not known |
There was a problem hiding this comment.
The new computed-cd handling is not pinned: two mutants of it pass the whole suite.
- Dropping
or rough[0] == "{"from the skip loop (cli_traces.py:141) passes. - Changing
rough[0] in _DIRStorough[0] == "cd"(cli_traces.py:144) passes. - With both applied,
shell_readsfromcwd="sub"changes from[]to['sub/a']for{ cd "$D"; } && cat a,{ builtin cd "$D"; cat a; },pushd "$D" && cat aandpopd "$D" && cat a. The read event then namessub/a, a file the shell never opened, andreach.classify()takes its path from that event.
Mutation check: both mutants pass test_repo_cli_harnesses.py, test_repo_cli_traces.py and test_repo_cli_pins.py (272 tests).
Suggestion: add a parametrized test with those four commands and cwd="sub", expecting [].
|
|
||
| def execution_record(opts: AgentOptions) -> dict[str, str]: | ||
| """How the cell's agent was started, as ``meta.json`` and ``result.json`` record it.""" | ||
| beside = opts.harness_bin.resolve().parent # where harness_mounts takes the companions from |
There was a problem hiding this comment.
The companion digest is taken from beside the resolved binary, and no test pins the .resolve().
beside = opts.harness_bin.parent(without.resolve()) passes the five listed test files (261 passed).- With a linked
--bin(the npm layout the description names), that mutant gives aNonedigest, so the execution id ignores an edit tocodex-code-mode-hostandrepo runreuses a stale cell. The mount tests cover the link layout; the id test attest_repo_cli_harnesses.py:471uses no link.
Suggestion: add an execution_record test with --bin a symlink to a binary that has a companion beside its target, and assert the id changes when the companion's bytes change.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
| # an indented [Pn] bullet with a dash after its tag, or ending in a path:line, is a header that did not parse, | ||
| # so it withholds the block; body text (a mid-sentence path, a numbered time) passes, and a dash in it fails safe. | ||
| _HEADER_TAIL = re.compile(r"\[P\d\].*?(?:—| [-] )|[\w./-]*[./][\w./-]*:\d+(?:-\d+)?\W*[ \t\r]*$", re.I) | ||
| _FINDING_MARK = re.compile(_BULLET_MARK.pattern + r"|^(?:\*\*)?\[P\d\]", re.M | re.I) |
There was a problem hiding this comment.
Must-fix: indented, unbulleted priority headers bypass the format-drift safeguard. For example, convert_answer("codex-review", " [P1] Incorrect validation — /ws/a.py:12", "/ws") appends an empty JSON findings array: _CODEX_FINDING requires a bullet, while this alternative only recognizes [P1] at column zero. The grader therefore receives a clean review despite a reported issue. Recognize leading whitespace before bare priority markers and add a regression asserting that this answer remains unchanged. Also ensure a location-bearing bare header after a parsed finding is not swallowed as body prose.
| _CONTINUATION = re.compile(r"\\\n") | ||
| _HEREDOC = re.compile(r"<<-?[ \t]*(['\"]?)(\w+)\1([^\n]*)\n.*?^[ \t]*\2[ \t]*$", re.S | re.M) | ||
| # a redirect that writes nowhere a read could be confused with: 2>&1, 2>/dev/null, &>/dev/null | ||
| _HARMLESS_REDIRECT = re.compile(r"(?:\d*>&\d+|&>>?\s*/dev/null|\d*>>?\s*/dev/null)(?=\s|$|[;&|])") |
There was a problem hiding this comment.
Must-fix: the redirect match can consume digits belonging to a filename. For shell_reads("cat AGENTS.md2>/dev/null"), this regex removes 2>/dev/null, leaving cat AGENTS.md and incorrectly recording a read of AGENTS.md. In shell syntax the operand is AGENTS.md2; the adjacent 2 is not a file descriptor because it is part of that word. This affects both CLI converters and can falsely classify guidance as fetched in reach results. Only consume descriptor digits at a shell word boundary (otherwise match the redirect without those digits), and add a filename-adjacent-digit regression.
| if in_subshell: # a subshell: its reads are not tracked | ||
| continue |
There was a problem hiding this comment.
Must-fix: a command substitution is mistaken for a subshell containing the entire command, so a computed cd leaves the tracked directory unchanged. For example, shell_reads('cd $(dirname sub/file); cat AGENTS.md') records AGENTS.md, although the shell reads sub/AGENTS.md. _Splitter marks any part containing unquoted parentheses as in_subshell, so this continue bypasses the computed-cd handling in _Walk.words. Distinguish command substitution from an actual subshell, or invalidate the tracked directory for an outer computed directory-changing command before skipping it. Add a regression asserting no relative read for this input, including a nonempty starting cwd.
| out.errors.append(f"line {n}: unexpected shape ({exc})") | ||
| out.answer = (run.final or run.last).strip() | ||
| out.num_turns = run.turns or None | ||
| out.result_seen = bool(run.turns) |
There was a problem hiding this comment.
Must-fix: a final result record does not mark the run as started unless assistant.turn_start was also present. For example, the accepted trace in test_copilot_answer_falls_back_to_the_last_message_and_a_failed_exit_is_an_error yields an answer and a result but leaves result_seen=False. With no tool events, run_cell consequently treats it as never started, omitting process measurements and marking skill/uptake measurements unmeasured instead of measured zero. Track whether a result was seen and set this flag when either a turn started or a result arrived; add a regression assertion for the no-turn-start trace.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to eb046c8, which fix the two nits from my review:
299ddd9:dokimi/docs/repo-evaluation-contract.md:921: said a failed run, an empty answer or a[Pn]header that does not parse gets no findings block.eb046c8:dokimi/docs/repo-evaluation-contract.md:1350: addedanswer_formatand the SHA-256 of eachbin_companionsfile to the hashed list, andanswer_formatto the edits that cause a refusal.
make ci-local at eb046c8: 29 passed, 0 failed, 7 skipped.
Not approving yet. Still open: the two risks from my review.
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