Repository navigation
Conversation
|
🔍 OpenCodeReview found 3 issue(s) in this PR. By severity: 1 medium, 2 low
Review details
|
| - cron: '37 4 * * *' | ||
| workflow_dispatch: | ||
|
|
||
| permissions: {} |
There was a problem hiding this comment.
Nit: The multiline credential checks and merge script rely on implicit shell selection, including on the Intel runner. Declare defaults.run.shell: bash at workflow level so all four jobs use the same explicit shell and Bash error-handling behavior.
Suggestion:
| permissions: {} | |
| permissions: {} | |
| defaults: | |
| run: | |
| shell: bash |
There was a problem hiding this comment.
Fixed in 21452d2: defaults.run.shell: bash at workflow level, so all four jobs use one explicit shell.
| - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 | ||
| with: | ||
| python-version: '3.13' |
There was a problem hiding this comment.
Nit: All four jobs reinstall the locked Python dependencies nightly without enabling the setup-python cache. Existing workflows cache pip using dokimi/requirements.lock.txt; use the same cache and cache-dependency-path settings in each setup-python step to reduce repeated downloads and package-index dependence.
Suggestion:
| - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 | |
| with: | |
| python-version: '3.13' | |
| - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 | |
| with: | |
| python-version: '3.13' | |
| cache: pip | |
| cache-dependency-path: dokimi/requirements.lock.txt |
There was a problem hiding this comment.
Fixed in 21452d2: all four setup-python steps use cache: pip with dokimi/requirements.lock.txt as the cache dependency path.
| REACH_MODEL: ${{ vars.REACH_MODEL_CLAUDE_CODE }} | ||
| ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} | ||
| AWS_BEARER_TOKEN_BEDROCK: ${{ secrets.AWS_BEARER_TOKEN_BEDROCK }} |
There was a problem hiding this comment.
Must-fix: With only AWS_BEARER_TOKEN_BEDROCK and REACH_MODEL_CLAUDE_CODE configured, this job installs Claude but never selects the Bedrock backend or supplies AWS_REGION. The harness profile only forwards these settings; it does not infer them from the token. On a clean runner Claude therefore attempts the default Anthropic backend instead of the advertised Bedrock configuration. Pass CLAUDE_CODE_USE_BEDROCK=1 and a configured AWS_REGION for the Bedrock path, while preserving direct Anthropic authentication when selected.
There was a problem hiding this comment.
Fixed in 21452d2: the claude-code run sets CLAUDE_CODE_USE_BEDROCK=1 and AWS_REGION when the Bedrock token is the credential in use.
| python scripts/reach_conformance.py merge reach-in/*.matrix.json \ | ||
| --out reach-out/reach-matrix.json --md reach-out/reach-matrix.md |
There was a problem hiding this comment.
Must-fix: Because this job runs with always(), a harness that fails during dependency/CLI installation or times out before writing its matrix is silently absent from the published report. For example, if the Codex npm install fails, the glob contains only the other two matrices; merge accepts them and reports no failed or skipped Codex runs. Validate that all three expected matrix files exist before publishing, or explicitly record missing harness jobs as failures/incomplete coverage.
There was a problem hiding this comment.
Fixed in 21452d2: the matrix job fails and publishes nothing when any of the three harness matrices is missing, including one whose job died before upload.
| `<harness>-runs/`. Each scenario is capped by `--budget` (USD, default 0.10) and `--timeout` (seconds per | ||
| scenario, default 300). `run` exits 1 only when a run is `failed`. |
There was a problem hiding this comment.
Must-fix: --budget is not a spending cap for every supported harness. run_case only forwards it to reach, and the Codex and Copilot profiles omit the budget argument entirely; reach adds no spending enforcement. Thus the documented Codex command can exceed $0.10 per scenario. Describe this as a harness-dependent limit (Claude Code forwards --max-budget-usd; Codex/Copilot are timeout-bounded only), and correct the same cap claim in the Cost bullet under Limits.
There was a problem hiding this comment.
Fixed in 21452d2: docs/reach-matrix.md says --budget reaches Claude Code only (--max-budget-usd); Codex and Copilot are capped by --timeout alone.
| kind, _, where = scenario.partition("=") | ||
| if kind not in ("cwd", "read") or not (root / where).is_dir(): |
There was a problem hiding this comment.
Must-fix: A malformed scenario key such as cwd passes both schema validation and this check: partition yields kind cwd and an empty directory, and root / '' exists. Thus an expectations entry AGENTS.md: {cwd: loaded} makes check succeed, but the real cwd=. receipt never matches it and is silently scored as no_claim. Require the = separator and a nonempty directory (or validate the scenario-key grammar in the schema), and add a regression test for this malformed key.
Suggestion:
| kind, _, where = scenario.partition("=") | |
| if kind not in ("cwd", "read") or not (root / where).is_dir(): | |
| kind, separator, where = scenario.partition("=") | |
| if separator != "=" or not where or kind not in ("cwd", "read") or not (root / where).is_dir(): |
There was a problem hiding this comment.
Fixed in 21452d2: check rejects a scenario key with no directory (a bare cwd); the drift test covers cwd: loaded.
| lines += [f"| {r['case']} | {r['harness']} | {r['version']} | {r['model'] or '-'} | {r['status']} | " | ||
| f"{(r['reason'] or '-').replace('|', chr(92) + '|')} |" for r in matrix["runs"]] |
There was a problem hiding this comment.
Must-fix: _reason retains embedded newlines from stderr, but this table row only escapes pipes. For example, a reason of exit 1: first error\nsecond error splits the row across two Markdown lines, putting the remainder outside the reason cell and breaking the published report. Normalize CR/LF to spaces or <br> when rendering reasons, and add a multiline-stderr rendering test.
There was a problem hiding this comment.
Fixed in 21452d2: _table_text collapses newlines in the reason cell as well as escaping |. A test covers a multi-line reason row.
| for harness, model in args.harness: | ||
| matrix = cf.run_harness(harness, model, args.fixtures, expect, args.out / f"{harness.name}-runs", | ||
| timeout=args.timeout, budget=args.budget, env=os.environ) | ||
| cf.write_matrix(matrix, args.out / f"{harness.name}.matrix.json") |
There was a problem hiding this comment.
Must-fix: run --harness codex=model-a --harness codex=model-b --out DIR accepts both entries and makes both sets of paid calls, but both use codex-runs/ and codex.matrix.json. The second run deletes the first run's receipts via run_case and overwrites its matrix, silently losing the first results. Reject duplicate harness names before starting any calls, or give each entry distinct output paths.
There was a problem hiding this comment.
Fixed in 21452d2: run rejects a repeated --harness name before any model call. A CLI test covers it.
| return 1 | ||
| cf.write_matrix(merged, args.out) | ||
| summaries = {case: block["summary"] for case, block in expect["cases"].items()} | ||
| args.md.write_text(cf.render_markdown(merged, summaries), encoding="utf-8") |
There was a problem hiding this comment.
Must-fix: The Markdown output's parent directory is not created. With valid matrix inputs, merge input.matrix.json --out results/reach-matrix.json --md reports/reach-matrix.md crashes with FileNotFoundError when reports/ does not exist, after already writing the JSON. write_matrix creates the JSON parent through save_artifact, but that only helps Markdown when both outputs share a directory. Create args.md.parent before writing and add a test using distinct, initially missing output directories.
Suggestion:
| args.md.write_text(cf.render_markdown(merged, summaries), encoding="utf-8") | |
| args.md.parent.mkdir(parents=True, exist_ok=True) | |
| args.md.write_text(cf.render_markdown(merged, summaries), encoding="utf-8") |
There was a problem hiding this comment.
Fixed in 21452d2: merge creates the parent directory of --md first. A CLI test covers it.
|
|
||
|
|
||
| def _matrix(runs=None, cells=None, generated_at="2026-10-31T00:00:00+00:00", digest=DIGEST) -> dict: | ||
| return cf.build_matrix(runs or [_run()], cells or [_cell()], digest, generated_at=generated_at) |
There was a problem hiding this comment.
Must-fix: cells=[] is replaced with a fabricated default cell because an empty list is falsy. Consequently, test_merge_keeps_two_failed_runs_of_one_harness supplies cells for both supposedly receipt-less failed runs and never checks that the merged cells remain empty. Distinguish None from empty lists for both arguments, and assert merged['cells'] == [] in that test.
Suggestion:
| return cf.build_matrix(runs or [_run()], cells or [_cell()], digest, generated_at=generated_at) | |
| return cf.build_matrix([_run()] if runs is None else runs, | |
| [_cell()] if cells is None else cells, digest, generated_at=generated_at) |
There was a problem hiding this comment.
Fixed in 21452d2: cells=[] is kept as given, and the two-failed-runs merge test passes no cells.
There was a problem hiding this comment.
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.
Copilot review overview
5 open issues
run_harness(..., env=...)usesenvto decide availability, butrun_case()always invokes the… · Newrun_harness(..., env=...)usesenvto decide availability, butrun_case()always invokes the… · Newrun_harness(..., env=...)usesenvto decide availability, butrun_case()always invokes the… · Newnpm install -g ...but does not explicitly provision Node.js/npm (e.g., via… · Newfailedruns (and anyerrorsruns) as long as they… · New
What changed in this PR
Adds an offline/fixture-based “reach conformance suite” and a nightly workflow that produces a harness × version conformance matrix artifact, comparing observed reach receipts to vendor-documented expectations.
Changes:
- Introduces fixture repos + expectations schema/data for reach conformance, plus matrix schema/artifact kind.
- Adds conformance runner/merger implementation and a CLI driver script.
- Wires in docs, tests, and a nightly GitHub Actions workflow to generate/publish the matrix.
| File | Description |
|---|---|
| dokimi/tests/test_runs_show.py | Registers reach_matrix.v1 counts for runs show output expectations. |
| dokimi/tests/test_runs_scan.py | Adds reach_matrix.v1 to scan fixtures with a generated timestamp. |
| dokimi/tests/test_reach_conformance.py | Adds offline unit tests covering conformance rules, merging, rendering, and CLI behavior. |
| dokimi/tests/fixtures/artifacts/reach_matrix.v1.json | Adds a fixture matrix artifact used by tests. |
| dokimi/tests/fixtures/artifacts/reach_conformance.v1.json | Adds a fixture expectations artifact used by tests. |
| dokimi/scripts/reach_conformance.py | Adds CLI entrypoints (check, run, merge) for the conformance suite. |
| dokimi/mkdocs.yml | Adds reach-matrix.md to MkDocs nav. |
| dokimi/dokimi/schemas/reach_matrix.v1.json | Adds JSON schema for reach_matrix.v1 artifact. |
| dokimi/dokimi/schemas/reach_conformance.v1.json | Adds JSON schema for reach_conformance.v1 expectations. |
| dokimi/dokimi/runs/kinds.py | Registers reach_matrix kind fields for run views (time + counts). |
| dokimi/dokimi/repos/conformance.py | Implements harness registry, availability checks, run/merge logic, and markdown rendering. |
| dokimi/dokimi/artifacts.py | Registers reach_matrix as an artifact kind and updates module docs. |
| dokimi/docs/repo-evaluation-contract.md | Links reach conformance matrix from reach receipt contract section. |
| dokimi/docs/reference/cli-interface.json | Adds reach_matrix to CLI interface reference types. |
| dokimi/docs/reach-matrix.md | Adds design/usage docs for the conformance suite and nightly workflow. |
| dokimi/docs/README.md | Adds docs index entry for reach conformance matrix. |
| dokimi/data/reach-conformance/symlinked-file/CLAUDE.md | Adds fixture file for symlink case (note: not a symlink in-repo). |
| dokimi/data/reach-conformance/symlinked-file/AGENTS.md | Adds target fixture file for symlink case. |
| dokimi/data/reach-conformance/subtree-claude-shadow/sub/CLAUDE.md | Adds fixture file for subtree CLAUDE shadowing. |
| dokimi/data/reach-conformance/subtree-claude-shadow/sub/AGENTS.md | Adds fixture file for subtree AGENTS behavior under shadowing. |
| dokimi/data/reach-conformance/root-agents/AGENTS.md | Adds baseline “root AGENTS” fixture. |
| dokimi/data/reach-conformance/override-file/AGENTS.override.md | Adds override fixture file. |
| dokimi/data/reach-conformance/override-file/AGENTS.md | Adds base file for override fixture. |
| dokimi/data/reach-conformance/nested-agents/packages/app/AGENTS.md | Adds nested AGENTS fixture file. |
| dokimi/data/reach-conformance/nested-agents/AGENTS.md | Adds root file for nested fixture. |
| dokimi/data/reach-conformance/large-file/AGENTS.md | Adds oversized fixture guidance file for size-cap behavior checks. |
| dokimi/data/reach-conformance/guidance.yaml | Adds a guidance-only manifest used by all fixtures/harnesses. |
| dokimi/data/reach-conformance/expectations.yaml | Adds vendor-documented expectations data for fixtures. |
| dokimi/data/reach-conformance/claude-imports-agents/CLAUDE.md | Adds fixture exercising Claude @ imports. |
| dokimi/data/reach-conformance/claude-imports-agents/AGENTS.md | Adds imported AGENTS file for import fixture. |
| dokimi/data/reach-conformance/claude-and-agents/CLAUDE.md | Adds fixture exercising CLAUDE-vs-AGENTS precedence. |
| dokimi/data/reach-conformance/claude-and-agents/AGENTS.md | Adds sibling AGENTS file for precedence fixture. |
| dokimi/changelog.d/20261031_160000_napetrov_reach-matrix.md | Adds changelog entry for the new suite/workflow. |
| dokimi/benchmarks/spec.v1.yaml | Updates doc-count benchmark cap due to new documentation. |
| .github/workflows/reach-matrix.yml | Adds nightly workflow to run per-harness jobs and merge/publish the matrix artifact. |
Review effort: Lite
| expect: Mapping[str, Any], *, timeout: int, budget: float) -> tuple[dict[str, Any], list[dict[str, Any]]]: | ||
| """Run ``repo reach`` over one fixture; its run record and cells. The receipt is read from ``out``.""" | ||
| case = fixture.name | ||
| shutil.rmtree(out, ignore_errors=True) # a receipt left by an earlier run is not this run's | ||
| argv = [sys.executable, "-m", "dokimi", "repo", "reach", str(fixture), "--manifest-file", str(guidance), | ||
| "--harness", harness.name, "--model", model, "--out", str(out), "--format", "receipt", | ||
| "--timeout", str(timeout), "--budget", f"{budget:g}"] | ||
| proc = process.run(argv, timeout=timeout * 16 + 300, env=process.Env.inherit()) |
There was a problem hiding this comment.
Maintainer decision: plumb env through to the runs (option a). Done in d32307e: run_case passes env as the whole child environment, and run_harness uses the same mapping for availability.
| def run_harness(harness: Harness, model: str, fixtures: Path, expect: Mapping[str, Any], out_dir: Path, *, | ||
| timeout: int = DEFAULT_TIMEOUT_S, budget: float = DEFAULT_BUDGET_USD, | ||
| env: Mapping[str, str], which: Callable[[str], str | None] = shutil.which) -> dict[str, Any]: | ||
| """One harness over every fixture, as a matrix. A harness that cannot run here gets a skipped run per fixture.""" | ||
| reason = unavailable(harness, model, env, which) |
There was a problem hiding this comment.
Maintainer decision: plumb env through to the runs (option a). Done in d32307e: run_case passes env as the whole child environment, and run_harness uses the same mapping for availability.
| record, found = run_case(harness, model, fixtures / case, guidance, out_dir / case, expect, | ||
| timeout=timeout, budget=budget) |
There was a problem hiding this comment.
Maintainer decision: plumb env through to the runs (option a). Done in d32307e: run_case passes env as the whole child environment, and run_harness uses the same mapping for availability.
| - name: Install the CLI | ||
| if: steps.creds.outputs.present == 'true' | ||
| run: npm install -g @anthropic-ai/claude-code@2.1.316 |
There was a problem hiding this comment.
Fixed in 21452d2: each harness job runs actions/setup-node (SHA-pinned, Node 22) before its npm install.
| """The matrix as a markdown page: the runs, then one table per fixture with a column per harness version.""" | ||
| summaries = summaries or {} | ||
| counts = matrix["summary"] | ||
| columns = sorted({(r["harness"], r["version"]) for r in matrix["runs"] if r["status"] != "skipped"}) |
There was a problem hiding this comment.
Fixed in 21452d2: the markdown version columns come from ran and errors runs only; failed runs stay in the Runs table. A test covers it.
github-actions
Bot
added
the
cli-interface-change
CLI interface change detectedThis PR changes dokimi's user-facing CLI surface (commands and/or options): ~ `dokimi runs list` parameters changed:
~ changed option `--kind`: choices: ["'answers'", "'arms'", "'cli_routes'", "'effective_instructions'", "'eval'", "'eval_coverage'", "'explore_runs'", "'guidance_reach'", "'matrix_rollup'", "'need_graph'", "'novelty'", "'plan'", "'plugin_delta'", "'questions'", "'redteam'", "'repo_budget'", "'repo_environment'", "'repo_readiness'", "'result'", "'skill_evidence'", "'subject_scorecard'", "'task_inspect'", "'task_runs'", "'tool_preference'"] -> ["'answers'", "'arms'", "'cli_routes'", "'effective_instructions'", "'eval'", "'eval_coverage'", "'explore_runs'", "'guidance_reach'", "'matrix_rollup'", "'need_graph'", "'novelty'", "'plan'", "'plugin_delta'", "'questions'", "'reach_matrix'", "'redteam'", "'repo_budget'", "'repo_environment'", "'repo_readiness'", "'result'", "'skill_evidence'", "'subject_scorecard'", "'task_inspect'", "'task_runs'", "'tool_preference'"]Updated on every push; the |
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at c2be5c5. The suite is well built: the agreement rules, the digest check on merge and the receipt-failure handling hold up under a 79-mutant run. The merge waits on two unsigned commits and a stale CLI snapshot, both failing CI, a nightly workflow that can publish a matrix with a harness silently missing, and a few untested lines that decide an exit code or a merge refusal.
What I ran, on a Linux review host at c2be5c5:
make ci-local: 28 passed, 2 failed, 7 skipped; 12,530 tests. One failure is the stale CLI snapshot below. The other,test_the_regex_limit_is_shared_by_every_pattern_of_one_check, is a wall-clock bound (1.77 s against 1.5 s) in code this PR does not touch; it is not the PR's.- Mutation run of
conformance.pyandscripts/reach_conformance.pyagainsttests/test_reach_conformance.py: 79 mutants, 31 of which pass the full suite. - Reproductions for the code findings, with the LLM transport mocked.
Two commits have no Signed-off-by: 3417ac7 and c2be5c5. ci-local does not run the DCO check, so it will fail on GitHub until both are signed off.
Open bot points I confirm:
- I agree with Copilot that
run_harnesstakesenvto decide availability butrun_casealways spawns withprocess.Env.inherit(). Nit: the parameter is misleading; in the nightly workflow each job holds only its own secret, so nothing leaks today. - I agree with exai-bot's point on the scenario key:
AGENTS.md: {cwd: loaded}passescheck. New evidence:check_expectationsreturned[]for that entry onroot-agents, and thecwdcell is then scoredno_claim. Nit: only a hand-edited file reaches it. - I agree with exai-bot's point that
run --harness codex=a --harness codex=boverwritescodex.matrix.json. Nit: reject duplicate names incmd_run. - I agree with exai-bot's point that the test helper's
cells or defaulthides empty cells. Nit: useis Noneand assertmerged["cells"] == []. - I agree with Copilot's point on the markdown columns. New evidence: a lone
failedcodex run with no cells renders a| file | scenario | codex unknown |header. Nit.
The large-file fixture cannot show truncation: the receipt maps a truncated file to observed: unknown (receipt.py:36, :213), which the matrix scores as no_claim and the legend calls "no usable answer". The page lists large-file as a size-cap test. Say in the page that a cut file reads as unknown, or add a state for it.
check also accepts entries a run never produces: a typo in a harness key (claude: for claude-code:), read=. and cwd=packages all return [], so those cells silently become no_claim. Check harness keys against HARNESSES.
merge and run print a traceback on a ConformanceError (mixed digests, duplicate run, bad expectations file). The exit code is non-zero either way; an error line would read better.
The page says every fixture has a root guidance file, but subtree-claude-shadow holds only sub/AGENTS.md and sub/CLAUDE.md.
Bot points I checked that no longer apply at this head (1)
- Copilot's point on
actions/setup-nodedoes not apply as a defect: the page states that the jobs neednpmon the runner, and the workflow installs only pinned CLI versions with it.
| "'plugin_delta'", | ||
| "'questions'", | ||
| "'redteam'", | ||
| "'reach_matrix'", |
There was a problem hiding this comment.
The CLI snapshot is stale: reach_matrix is inserted after redteam, but the dump sorts it before.
tests/test_cli_flags.py::test_the_cli_interface_snapshot_is_current fails on this head: dokimi/docs/reference/cli-interface.json is stale: run make cli-snapshot in dokimi/ and commit the result. make ci-local reports FAIL ci.yml:typecheck / CLI snapshot is current. The generated diff in the PR's own comment lists 'reach_matrix' between 'questions' and 'redteam'.
Suggestion: run make cli-snapshot in dokimi/ and commit the result. Do not edit the order by hand.
There was a problem hiding this comment.
Fixed in 21452d2: make cli-snapshot regenerated; reach_matrix now sits before redteam.
| env: | ||
| PYTHONPATH: . | ||
| run: | | ||
| python scripts/reach_conformance.py merge reach-in/*.matrix.json \ |
There was a problem hiding this comment.
I agree with exai-bot here; a harness job that dies before its upload leaves its matrix out, and the merge publishes the other two without a warning.
The matrix job runs with if: always(), and the upload steps use if-no-files-found: warn. If the Codex npm install fails, reach-in/ holds two files. cf.merge accepts any non-empty list, so the page reports no failed or skipped Codex runs: the harness is absent, not failed. By the code, not a run: merge has no expected-harness list.
Suggestion: before the merge, fail the step when any of claude-code.matrix.json, codex.matrix.json or copilot.matrix.json is missing from reach-in/, or give merge an --expect-harness list that records the absent ones as failed.
There was a problem hiding this comment.
Fixed in 21452d2: the matrix job fails and publishes nothing when a harness matrix is missing, as in the thread above.
| PYTHONPATH: . | ||
| REACH_MODEL: ${{ vars.REACH_MODEL_CLAUDE_CODE }} | ||
| ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} | ||
| AWS_BEARER_TOKEN_BEDROCK: ${{ secrets.AWS_BEARER_TOKEN_BEDROCK }} |
There was a problem hiding this comment.
I agree with exai-bot here; the Bedrock-only setup the page advertises does not select Bedrock.
The job passes AWS_BEARER_TOKEN_BEDROCK and REACH_MODEL, and the availability check accepts the token. It sets neither CLAUDE_CODE_USE_BEDROCK nor AWS_REGION, which the profile only forwards (env_allow) and docs/reviewing.md:105 lists as required for Bedrock. By that doc, not a run, Claude Code falls back to the Anthropic backend with no ANTHROPIC_API_KEY, and every Claude scenario fails. The page (reach-matrix.md:128-130) names the Bedrock token as an alternative secret without these two.
Suggestion: pass CLAUDE_CODE_USE_BEDROCK and AWS_REGION (a repository variable) to this step, and name them in the Secrets and Variables paragraph; or drop the Bedrock alternative from the page.
There was a problem hiding this comment.
Fixed in 21452d2: the claude-code run selects Bedrock (CLAUDE_CODE_USE_BEDROCK=1) and passes AWS_REGION when only the Bedrock token is set.
| "## Runs", "", "| fixture | harness | version | model | status | reason |", "|---|---|---|---|---|---|", | ||
| ] | ||
| lines += [f"| {r['case']} | {r['harness']} | {r['version']} | {r['model'] or '-'} | {r['status']} | " | ||
| f"{(r['reason'] or '-').replace('|', chr(92) + '|')} |" for r in matrix["runs"]] |
There was a problem hiding this comment.
I agree with exai-bot here; a multi-line reason splits its table row.
The row escapes | only. _reason keeps the last 300 bytes of stderr, which is almost always several lines. Reproduction with build_matrix and render_markdown on a run with reason exit 1: a\nb renders:
| root-agents | codex | unknown | m | failed | exit 1: a
b |
The second line leaves the table, and the run summary the nightly job appends to $GITHUB_STEP_SUMMARY shows broken rows exactly for the failed runs.
Suggestion: collapse whitespace in the reason when rendering, e.g. " ".join(reason.split()), before the pipe escape, and add a test with a two-line stderr.
There was a problem hiding this comment.
Fixed in 21452d2: multi-line reasons are collapsed to one line in the table row. A test covers it.
| print(f"{harness.name}: ran {counts['ran']}, errors {counts['errors']}, " | ||
| f"failed {counts['failed']}, skipped {counts['skipped']}") | ||
| failed = failed or counts["failed"] > 0 | ||
| return 1 if failed else 0 |
There was a problem hiding this comment.
Nothing tests cmd_run, so its exit code and the receipt cleanup in run_case can change unnoticed.
The only run test is the unknown-harness SystemExit. Mutation check, each passing all 40 tests in test_reach_conformance.py:
return 1 if failed else 0replaced byreturn 0: a nightly job whose every run isfailedgoes green.shutil.rmtree(out, ignore_errors=True)removed fromrun_case(conformance.py:199): a staleeffective_instructions.jsonfrom an earlier run is read when the new run writes none, and the run is recordedranwith old cells. The no-receipt test uses a freshtmp_path, so it cannot see this.
Suggestion: add a cmd_run test with run_harness faked to return a failed run (assert exit 1) and an errors-only run (assert the exit you intend, which the page says is 0), and a run_case test whose out already holds a receipt while the faked process writes none (assert failed).
There was a problem hiding this comment.
Fixed in 21452d2: CLI tests cover run: the duplicate-harness rejection, exit 1 for a failed run, and exit 0 for errors-only runs.
| for run in matrix["runs"]: | ||
| if run["status"] in ("failed", "skipped"): # no receipt, so no cells to count twice and no version to key on | ||
| continue | ||
| key = (run["harness"], run["version"], run["case"]) |
There was a problem hiding this comment.
The merge repeat check is pinned only for one case, one version and ran runs.
Mutation check, each passing the full suite: the key (harness, version, case) without case, without version or without harness; the status filter reduced to ("failed",); and errors added to it. The case mutant refuses every real merge of a harness with more than one fixture, and the errors mutant lets two errors runs of one version through, which is the double count this check exists to stop.
Suggestion: add merge tests with (a) two cases of one harness version (accepted), (b) two versions of one harness on one case (accepted), (c) two errors runs of one harness version on one case (refused), and (d) a skipped plus a ran run of one version (accepted).
There was a problem hiding this comment.
Fixed in 21452d2: merge tests cover two fixtures, two versions, refusal of a duplicate errors run, and acceptance of skipped plus ran runs.
| is skipped, as is one without its credential or its CLI (`unavailable` in `dokimi/repos/conformance.py` | ||
| names which). `run` writes `<harness>.matrix.json` to `--out`, and the per-fixture receipts under | ||
| `<harness>-runs/`. Each scenario is capped by `--budget` (USD, default 0.10) and `--timeout` (seconds per | ||
| scenario, default 300). `run` exits 1 only when a run is `failed`. |
There was a problem hiding this comment.
I agree with exai-bot here; --budget is not a spending cap for Codex or Copilot, and the page says it is.
codex.yaml:12 states that {budget} is not passed and the timeout bounds the run, and copilot.yaml:8 says {budget} has no flag. Only claude-code.yaml passes --max-budget-usd. The page says each scenario is "capped by --budget" (line 82) and "capped at --budget" in the Cost bullet (line 107). A nightly Codex run over 8 fixtures can spend more than the page promises.
Suggestion: state that --budget is enforced by Claude Code only and that Codex and Copilot are bounded by --timeout, in both places.
There was a problem hiding this comment.
Fixed in 21452d2: docs/reach-matrix.md says --budget is passed only to Claude Code, and Codex and Copilot are bounded by the timeout.
| if: steps.creds.outputs.present == 'true' | ||
| run: npm install -g @openai/codex@0.166.0 |
There was a problem hiding this comment.
Must-fix: CLI installation is gated only on credentials, although a missing model must produce a skipped matrix. For example, with CODEX_API_KEY set and REACH_MODEL_CODEX unset, npm still runs; if installation fails, the run step is skipped and no missing-model record is written. Gate installation on both the credential and the corresponding nonempty model variable, while leaving the run step enabled to record skips. The same issue occurs in the Claude Code and Copilot jobs.
Suggestion:
| if: steps.creds.outputs.present == 'true' | |
| run: npm install -g @openai/codex@0.166.0 | |
| if: steps.creds.outputs.present == 'true' && vars.REACH_MODEL_CODEX != '' | |
| run: npm install -g @openai/codex@0.166.0 |
There was a problem hiding this comment.
Fixed in 21452d2: CLI install runs only when both the credential and the model variable are set; otherwise the run is skipped.
| argv = [sys.executable, "-m", "dokimi", "repo", "reach", str(fixture), "--manifest-file", str(guidance), | ||
| "--harness", harness.name, "--model", model, "--out", str(out), "--format", "receipt", |
There was a problem hiding this comment.
Must-fix: the Codex run has no authentication setup. With CODEX_API_KEY, a model and codex on PATH, this always selects the built-in codex profile, but that profile explicitly says codex exec reads neither API-key variable and ships no config_files. Reach also creates a fresh CODEX_HOME for each scenario, so an existing login cannot supply authentication. Consequently the advertised key-only setup produces authentication errors for every Codex scenario rather than observations. Supply a conformance profile with an explicitly selected provider using env_key = "CODEX_API_KEY" (or initialize authentication in each isolated config directory), and test that the key is actually consumed rather than merely forwarded.
There was a problem hiding this comment.
Maintainer decision: codex auth via a profile with env_key = "CODEX_API_KEY". Done in d32307e: the suite runs data/reach-conformance/codex.profile.yaml, the built-in codex profile plus a provider with that env_key.
A test checks the provider wiring and the forwarded key. No live codex run was made (no paid call), so consumption by codex itself is not verified here.
| if args.command == "run": | ||
| return cmd_run(args) | ||
| if args.command == "merge": | ||
| return cmd_merge(args) | ||
| return cmd_check(args) |
There was a problem hiding this comment.
Must-fix: expected input/refusal errors are not handled at the CLI boundary. For example, python scripts/reach_conformance.py --expectations missing.yaml check raises ConformanceError, and merging the same valid matrix twice raises ConformanceError from merge; both print a traceback rather than a concise diagnostic. Missing fixture directories and missing/malformed matrix inputs likewise escape as OSError/ValueError. Catch these expected exceptions around command dispatch, print their message to stderr, and return a nonzero status. Add CLI-level tests for these failure paths (the current duplicate-merge tests exercise only the library).
There was a problem hiding this comment.
Fixed in 21452d2: main prints a one-line ✗ message and exits 1 for an unusable input, instead of a traceback. CLI tests cover it.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to a2fd806, which fix the six nits from my review:
d67b111:dokimi/scripts/reach_conformance.py:77:mergenow creates the parent directory of--mdbefore writing the page.1409cc2:dokimi/dokimi/repos/conformance.py:221: therun_harnessdocstring saysenvonly decides availability and each run inherits the process environment.d4fdb5e:dokimi/dokimi/repos/conformance.py:113:checkreports a scenario key with no directory, such ascwd.21b1adb:dokimi/scripts/reach_conformance.py:55:runrejects a repeated--harnessname before running anything.4b3ae0a:dokimi/tests/test_reach_conformance.py:67: the_matrixhelper testsis None, and the failed-runs merge test assertsmerged["cells"] == [].a2fd806:dokimi/dokimi/repos/conformance.py:282: failed runs no longer add a version column to the markdown tables.
make ci-local at a2fd806: 28 passed, 2 failed (the stale CLI snapshot, as at the reviewed head), 7 skipped.
Not approving yet. Still open: the bug and the six risks from my review.
a2fd806 to
21452d2
Compare
`repo reach --format receipt` prints, and writes to effective_instructions.json in --out, the effective-instructions/v0 receipt (agentsmd/agents.md#213) of each probed scenario. It joins the probe's marker tokens with budget's start-up rules (Claude Code only): per source the path, sha256, scope, expected state, observed state, observation_method, the harness name and version, the displacements seen in the run, and the unknowns. followed is always unknown. The mapping and its limits are in docs/repo-evaluation-contract.md §7.1. The receipt is validated against schemas/effective_instructions.v1.json and registered in artifacts.SCHEMA_VERSIONS. The default output of repo reach is unchanged. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
A local source's tree label is its absolute path, and the receipt copied it verbatim: a shared effective-instructions receipt then carried the home path. An absolute or drive-letter or ~ label now keeps only its last directory; URLs and relative labels are unchanged. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The reach probe says what a harness loaded in one run. This adds the suite that runs it over a set of fixture repositories for several harnesses and versions, and compares each observed state with what the harness's vendor documentation says. - data/reach-conformance/: eight fixture repositories (root, nested, CLAUDE.md with AGENTS.md, a Claude Code @ import, an override file, a subdirectory shadow, a file over the size cap, a symbolic link), a guidance-only manifest that probes every guidance name for every harness, and expectations.yaml (reach_conformance.v1) with the documented state per file and scenario; anything undocumented is unknown and reported as no_claim. - repos/conformance.py: harness registry, availability (a harness with no model, credential or CLI is a skipped run), the agreement rules, one run per fixture from the effective-instructions receipt, and the merge and markdown rendering of reach_matrix.v1. - scripts/reach_conformance.py: check (expectations against the tree), run (paid calls, only for harnesses that can run here) and merge. - .github/workflows/reach-matrix.yml: nightly and workflow_dispatch, one job per harness with only its own secret, then a merge job that publishes the matrix as an artifact. Not a pull-request check. - docs/reach-matrix.md: design, limits and how to run and extend it. - reach_matrix.v1 is registered as an artifact kind and added to the runs views and the CLI snapshot. Offline tests cover the rules, the run outcomes with the harness faked, merge refusal, rendering, drift checks and the CLI. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…ness load_artifact raised on a receipt that did not validate, which aborted the remaining fixtures and harnesses of the nightly run. Such a run is now failed with the validation error as its reason. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The static comment-block check fails a block over two lines. The sources and the design move to docs/reach-matrix.md, which both comments point to. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The catch named only the schema error, so a receipt that is not JSON or not UTF-8 still aborted the harness. Catch OSError and ValueError, which covers both, and test a truncated and a non-object receipt. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…atrix Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…ses a mix reach_matrix.v1 gains expectations_digest, the sha256 of the expectations as canonical JSON. merge refuses matrices built from different revisions, and the merge command refuses matrices whose digest is not that of the expectations file it renders against. Failed and skipped runs, which name no version, no longer count as a repeated harness version on merge. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…k; document the digest Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com> Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com> Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com> Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com> Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com> Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com> Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
- CI: exempt the fixtures' CLAUDE.md and AGENTS.override.md from the license header check, add the header to docs/reach-matrix.md, and regenerate the CLI snapshot (reach_matrix sorts before redteam). - Nightly workflow: default bash shell; pip cache; Node.js set up for the CLI installs; a CLI is installed only with its model variable; Claude Code uses Bedrock (CLAUDE_CODE_USE_BEDROCK, AWS_REGION) only when the Anthropic key is absent; the merge fails when a harness wrote no matrix. - Markdown: a multi-line stderr reason stays on one table row. - CLI: an unusable expectations file, matrix or merge input prints one line and exits 1, no traceback. - Docs: --budget is enforced by Claude Code only; "marker self-report" label; versioning rule matches the persisted-format policy. - Tests for the duplicate harness, the markdown directory, the failed-run column, the malformed scenario key, the receipt cleanup, the run exit code and the merge repeat rules. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| python scripts/reach_conformance.py check | ||
| python scripts/reach_conformance.py run --harness codex=<model> --out reach-out | ||
| python scripts/reach_conformance.py merge reach-out/*.matrix.json --out reach-matrix.json --md reach-matrix.md |
There was a problem hiding this comment.
Must-fix: These commands fail with ModuleNotFoundError: No module named 'dokimi' in a fresh environment prepared with the documented make dev. That target installs dependencies/tools, not the package (the editable install belongs to the scheduled test-latest job, which tool_specs() excludes). Running a file under scripts/ puts that directory, not the project root, on Python's import path. Prefix these examples with PYTHONPATH=. (as the nightly workflow does), or explicitly require an editable package installation first.
Suggestion:
| python scripts/reach_conformance.py check | |
| python scripts/reach_conformance.py run --harness codex=<model> --out reach-out | |
| python scripts/reach_conformance.py merge reach-out/*.matrix.json --out reach-matrix.json --md reach-matrix.md | |
| PYTHONPATH=. python scripts/reach_conformance.py check | |
| PYTHONPATH=. python scripts/reach_conformance.py run --harness codex=<model> --out reach-out | |
| PYTHONPATH=. python scripts/reach_conformance.py merge reach-out/*.matrix.json --out reach-matrix.json --md reach-matrix.md |
There was a problem hiding this comment.
Fixed in d32307e: the examples now prefix PYTHONPATH=.. Without it, scripts/reach_conformance.py check fails with ModuleNotFoundError: No module named 'dokimi' (checked in the make dev venv).
| - **Cost.** Each scenario makes at least one model call. Claude Code runs are capped at `--budget`; Codex and | ||
| Copilot runs are bounded only by `--timeout`. The nightly run covers every scenario of every fixture for each |
There was a problem hiding this comment.
Must-fix: --budget is applied separately to each scenario, not to the fixture run defined above. run_reach passes the full budget to _run_one on every iteration; for example, nested-agents has three scenarios, so the default permits up to $0.30 across that fixture rather than $0.10. Clarify the per-scenario scope here and in the Running section so users do not mistake it for a total spending cap.
Suggestion:
| - **Cost.** Each scenario makes at least one model call. Claude Code runs are capped at `--budget`; Codex and | |
| Copilot runs are bounded only by `--timeout`. The nightly run covers every scenario of every fixture for each | |
| - **Cost.** Each scenario makes at least one model call. Each Claude Code scenario is capped at `--budget`, | |
| not the whole fixture or suite; Codex and Copilot scenarios are bounded only by `--timeout`. The nightly run covers every scenario of every fixture for each |
There was a problem hiding this comment.
Fixed in d32307e: the Cost note now says the budget is per Claude Code scenario, not per fixture or suite. A fixture with three scenarios can spend three times the cap, as nested-agents does.
| expect: Mapping[str, Any], *, timeout: int, budget: float) -> tuple[dict[str, Any], list[dict[str, Any]]]: | ||
| """Run ``repo reach`` over one fixture; its run record and cells. The receipt is read from ``out``.""" | ||
| case = fixture.name | ||
| shutil.rmtree(out, ignore_errors=True) # a receipt left by an earlier run is not this run's |
There was a problem hiding this comment.
Must-fix: Cleanup failures are ignored, so a stale receipt can still be scored as this run's result. For example, if out contains a valid old receipt but is no longer writable, rmtree leaves it behind; repo reach then fails to write its output, but the existing receipt passes is_file() and is loaded, producing an errors run with the previous run's cells/version instead of a failed run. Treat cleanup failure as a failed run and do not launch/read the probe unless the old receipt is gone. Add a regression test for a failed cleanup (the current stale-receipt test covers only successful deletion).
There was a problem hiding this comment.
Fixed in d32307e: if an earlier receipt cannot be removed, run_case returns a failed run and neither launches nor reads the probe. Regression test: test_run_case_fails_when_an_earlier_receipt_cannot_be_cleared.
| seen.add(key) | ||
| runs += matrix["runs"] | ||
| cells += matrix["cells"] | ||
| return build_matrix(runs, cells, digests[0], generated_at=max(m["generated_at"] for m in matrices)) |
There was a problem hiding this comment.
Must-fix: max orders timestamp strings, not instants. Two otherwise valid matrices with generated_at values 2026-11-02T10:00:00+02:00 and 2026-11-02T09:00:00+00:00 select the first, although the second is an hour newer. The matrix schema permits both, and merge does not require UTC-normalized inputs. Parse and compare timezone-aware datetimes (with a clear error for invalid timestamps), or validate and normalize timestamps before merging; add a different-offset test.
There was a problem hiding this comment.
Fixed in d32307e: merge orders generated_at as timezone-aware instants; a naive or unparsable timestamp raises ConformanceError. The test covers the +02:00 vs +00:00 case.
| lines += [f"| {r['case']} | {r['harness']} | {r['version']} | {r['model'] or '-'} | {r['status']} | " | ||
| f"{_table_text(r['reason'] or '-')} |" for r in matrix["runs"]] |
There was a problem hiding this comment.
Must-fix: Table escaping is applied only to the reason, not the other run fields. For example, a schema-valid run with model="provider|model" produces an extra column, and a newline in a model or version splits the row. The same bug affects harness/version headers and path/scenario cells in _case_section. Apply _table_text to every dynamic table field and add coverage for delimiters outside the reason field.
There was a problem hiding this comment.
Fixed in d32307e: every dynamic field of the runs table and the per-fixture tables goes through _table_text, not only the reason. The test covers a pipe in model, path and scenario.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #364 +/- ##
==========================================
+ Coverage 94.29% 94.34% +0.04%
==========================================
Files 308 310 +2
Lines 39322 39654 +332
Branches 6651 6707 +56
==========================================
+ Hits 37079 37411 +332
- Misses 1430 1431 +1
+ Partials 813 812 -1
🚀 New features to boost your workflow:
|
- run_harness: each repo reach run gets exactly the environment it is given, not the inherited one - codex: the suite runs data/reach-conformance/codex.profile.yaml, whose provider reads CODEX_API_KEY (codex exec sends no bearer for the built-in profile) - run_case: a receipt that cannot be cleared fails the run instead of being read as this run's - merge: generated_at is compared as an instant; a timestamp that is not timezone-aware is refused - markdown: every dynamic table field goes through _table_text, not only the reason - docs: PYTHONPATH=. for the scripts; --budget is per Claude Code scenario Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
|
||
|
|
||
| # The codex profile with a provider that reads CODEX_API_KEY: the built-in profile sends no bearer (see the file). | ||
| CODEX_PROFILE = Path(__file__).resolve().parents[2] / "data" / "reach-conformance" / "codex.profile.yaml" |
There was a problem hiding this comment.
Must-fix: this runtime dependency is outside the installed package. In a wheel installation, parents[2] is site-packages, so this resolves to site-packages/data/reach-conformance/codex.profile.yaml; the package-data configuration includes only profiles under dokimi/repos/profiles/, and load_profile has no fallback for a missing absolute path. Consequently, run_harness(HARNESSES['codex'], ...) fails every fixture even when the caller supplies valid fixtures, credentials and a Codex executable. Move the suite profile inside the package, include it in package-data, and add an installed-package profile-loading test.
There was a problem hiding this comment.
Fixed in 0b7a8bf. The suite profile now lives in the package (dokimi/repos/conformance_profiles/codex.yaml) and pyproject ships it.
A test loads it through importlib.resources, so an installed wheel finds it.
| runs.append(run_record(harness.name, case, "skipped", reason, UNKNOWN, model, 0)) | ||
| continue | ||
| record, found = run_case(harness, model, fixtures / case, guidance, out_dir / case, expect, | ||
| timeout=timeout, budget=budget, env=env) |
There was a problem hiding this comment.
Must-fix: subprocess execution now uses the supplied environment, but availability still uses shutil.which(harness.bin) against the parent's PATH. For example, when Codex exists only in /opt/codex/bin and the caller supplies env={'CODEX_API_KEY': 'k', 'PATH': '/opt/codex/bin:/usr/bin'}, run_harness incorrectly skips every fixture if that directory is absent from the parent's PATH. Conversely, a CLI present only on the parent's PATH passes availability and then fails in the child. Resolve availability using the supplied PATH while retaining an injectable resolver, and test differing parent/child PATH values without the always-successful _found stub.
There was a problem hiding this comment.
Fixed in 0b7a8bf. Availability now looks for the CLI on the PATH of the env the run gets, not the parent's. which stays injectable.
A test uses a different parent and run PATH, without the always-found stub.
…n runs.md Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Petrov, Nikolay A <nikolay.a.petrov@intel.com>
| names which). `run` writes `<harness>.matrix.json` to `--out`, and the per-fixture receipts under | ||
| `<harness>-runs/`. Each scenario is capped by `--timeout` (seconds per scenario, default 300). `--budget` (USD, | ||
| per scenario, default 0.10) is passed to Claude Code as `--max-budget-usd` only: Codex and Copilot have no budget | ||
| flag, so for them the timeout is the only cap. `run` exits 1 only when a run is `failed`. `run` gives each |
There was a problem hiding this comment.
Must-fix: run also exits 1 before executing any fixture when expectations drift (cmd_run, lines 52–55), when a harness is specified twice (lines 56–59), or when an input cannot be loaded (main, lines 112–114). Thus exit 1 does not necessarily mean a run is failed, and a caller relying on this contract may look for a matrix that was never written. Document these preflight/input failures alongside failed runs, while retaining that errors-only and skipped runs exit 0.
Suggestion:
| flag, so for them the timeout is the only cap. `run` exits 1 only when a run is `failed`. `run` gives each | |
| flag, so for them the timeout is the only cap. `run` exits 1 on preflight/input errors or when a run is `failed`; | |
| errors-only and skipped runs exit 0. `run` gives each |
There was a problem hiding this comment.
Fixed in 0b7a8bf. docs/reach-matrix.md now lists the preflight errors that also exit 1 (expectations drift, a harness given twice, an input that cannot be loaded).
Errors-only and skipped runs still exit 0.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…on the run's PATH The reach suite's codex profile lived under data/, outside the package, so an installed wheel could not find it. It moves to repos/conformance_profiles/ and pyproject's package-data ships it; a test checks the wheel list and loads it through importlib.resources. Availability looked for the codex CLI on this process's PATH, but the run gets only the env it is given. It now searches the PATH of that env, so a CLI on the parent's PATH alone is not available and one only on the run's PATH is. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
run also exits 1 before any fixture runs: expectations drift, a harness given twice, an input that cannot be loaded. Errors-only and skipped runs still exit 0. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| PYTHONPATH=. python scripts/reach_conformance.py run --harness codex=<model> --out reach-out | ||
| PYTHONPATH=. python scripts/reach_conformance.py merge reach-out/*.matrix.json --out reach-matrix.json --md reach-matrix.md |
There was a problem hiding this comment.
Nit: These examples write receipts to dokimi/reach-out/ and merged artifacts to the project root, none of which .gitignore excludes. Copying them leaves generated run data in the working tree, contrary to the repository's output-directory convention. Use results/ for run data and reports/ for the merged outputs.
Suggestion:
| PYTHONPATH=. python scripts/reach_conformance.py run --harness codex=<model> --out reach-out | |
| PYTHONPATH=. python scripts/reach_conformance.py merge reach-out/*.matrix.json --out reach-matrix.json --md reach-matrix.md | |
| PYTHONPATH=. python scripts/reach_conformance.py run --harness codex=<model> --out results/reach-out | |
| PYTHONPATH=. python scripts/reach_conformance.py merge results/reach-out/*.matrix.json --out reports/reach-matrix.json --md reports/reach-matrix.md |
| for r in matrix["runs"]] | ||
| lines += ["", "Cell legend: `loaded` or `not_loaded` as observed; `fetched` is read with a tool first; " | ||
| "`unknown` is no usable answer. `(docs: X)` marks a disagreement with the documentation; a " | ||
| "cell with no `(docs: ...)` agrees, or the documentation makes no claim.", ""] |
There was a problem hiding this comment.
Nit: The legend incorrectly classifies inconclusive cells. A cell with observed unknown, documented loaded, and agreement inconclusive renders as unknown without (docs: ...), yet this sentence says it agrees or has no documented claim. Include inconclusive observations and aliases among the possibilities, or render their agreement explicitly.
| metavar="NAME[=MODEL]", help="repeatable; no model makes the harness skipped") | ||
| run.add_argument("--out", type=Path, required=True, help="directory for <harness>.matrix.json") | ||
| run.add_argument("--timeout", type=int, default=cf.DEFAULT_TIMEOUT_S, help="per-scenario seconds") | ||
| run.add_argument("--budget", type=float, default=cf.DEFAULT_BUDGET_USD, help="per-scenario USD cap") |
There was a problem hiding this comment.
Must-fix: This help text promises a USD cap for every scenario, but Codex and Copilot do not consume {budget} in their profiles, and run_reach() only records cost after the process completes. For example, run --harness codex=<model> --budget 0.01 can spend more than $0.01 per scenario. Describe this as a Claude Code-only cap (as the new documentation already does), or implement enforcement for the other harnesses.
Suggestion:
| run.add_argument("--budget", type=float, default=cf.DEFAULT_BUDGET_USD, help="per-scenario USD cap") | |
| run.add_argument("--budget", type=float, default=cf.DEFAULT_BUDGET_USD, | |
| help="per-scenario USD cap for Claude Code only; ignored by Codex and Copilot") |
Problem
repo reach --format receipt(#335) says what one harness loaded in one run. Nothing yet runs it across harnesses and versions, or compares what they load with what their vendor documentation says.Change
data/reach-conformance/holds eight fixture repositories and two files:AGENTS.md, nested,CLAUDE.mdwithAGENTS.md, a Claude Code@import,AGENTS.override.md, a subdirectory shadow, a file over the size cap, and a symbolic link;expectations.yaml(reach_conformance.v1), which gives the documented state per harness, file and scenario. Anything the docs don't cover isunknownand is scored asno_claim.repos/conformance.pyholds:agrees,disagrees,inconclusive,no_claim,alias);reach_matrix.v1.reach_matrix.v1carriesexpectations_digest.mergerefuses matrices built from different revisions of the expectations. A failed or skipped run is never counted as a repeated harness version.scripts/reach_conformance.pyhas three subcommands:check: the expectations against the fixture tree;run: makes paid calls, only for harnesses that can run here;merge: refuses matrices whose digest doesn't match the--expectationsfile..github/workflows/reach-matrix.ymlruns nightly and onworkflow_dispatch. It has one job per harness, each with only its own secret, then a merge job that uploads the matrix as an artifact. It is not a pull-request check.docs/reach-matrix.mdcovers the design, the limits, and how to run and extend the suite.reach_matrix.v1is registered as an artifact kind, and is added to the runs views and to the CLI snapshot.Evidence
The offline tests (
tests/test_reach_conformance.py) cover:No paid run is part of this PR. The matrix comes from the nightly workflow once it has secrets.
Local review: three self-review passes (design lens, finder, a probing verifier, then two round passes) on the merge of
mainwith #335.fetchedis now documented.test_reach_conformance.py, the runs, artifact and CLI-snapshot tests, and ruff pass. On this Windows host one symlink test fails becausecore.symlinks=false; CI runs on Linux.🤖 Generated with Claude Code