Skip to content

refactor(reports): generate the pricing mirror and split the reports layer - #176

Merged
uipreliga merged 20 commits into
mainfrom
feat/reports-consolidation
Sep 16, 2026
Merged

uipreliga merged 20 commits into
mainfrom
feat/reports-consolidation

Conversation

@uipreliga

@uipreliga uipreliga commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Implements c/2026-09-14-reports-consolidation.md (five phases), then c/2026-09-14-reports-consolidation-review-fixes.md (four phases) for the residual issues a review of the first found. 15 commits.

What changes

  1. Pricing is generated, not copied. evalboard/lib/pricing.generated.ts is rendered from pricing.py by make pricing-mirror and guarded by CE065. Deletes the regex parser, its meta-guard, the DELIBERATELY_UNMIRRORED set, that set's staleness guard, and the comment asking the next reader to keep it honest — five layers of bookkeeping that still shipped a real bug: claude-sonnet-5, gpt-5.6-sol, gpt-5.6-terra and gpt-5.6-luna sat in the exemption set under "the evalboard never runs them" while appearing ~32k / ~2k / ~17k / ~2k times in the run corpus, so every one of those runs rendered for cost with nothing failing. The one rule the set encoded that was not bookkeeping — that three OpenRouter models must stay unpriced so runs.ts apportions the provider's real bill — becomes ModelPricing.per_request_billing, data beside the rate it qualifies.

  2. Two verified DRY fixes, and 18 of 19 function-local imports hoisted. SLOW_PARAMS_PREVIEW_CHARS and TokenUsage.total_tokens each have one definition again. The 19th import (criteria, which runs pkgutil auto-discovery with registry side effects) stays deferred and is pinned by a subprocess test rather than remembered.

  3. reports_stats.py splits three ways. It was three unrelated modules sharing a file, and the orchestrator imported turn_time_buckets / visible_turn_count from it during a run. Now: stats.py (dependency-free by contract — asserted by parsing its AST), result_metrics.py (what the orchestrator reads mid-run), and the report helpers stay. The run.json row serializer moves to run_record.py; its old home was the only reason orchestration/batch.py reached into the reports layer at all.

  4. reports/ becomes a package (markdown / html / experiment / junit / helpers), format_ms moves to durations.py, and CE066 pins the layering: core may import only the package's public writers. An allowlist, not a denylist. The package publishes 16 names, not 25: nine were only ever imported by sibling modules, which now import each other directly.

  5. Documentation truth-up, plus a ## The reports package section in .claude/architecture-notes.md recording the layering, why each extracted module sits where it does, and the rejected section-data layer with the measurement behind the rejection (only 1 of 4 "duplicated" section pairs shares an input shape).

  6. The layering rules now see the whole core layer. CE004 and CE066 share one predicate, _layers.is_core_path. It was a denylist — ten directory names plus a top-level-module regex — and isolation/ was not on it, so isolation/docker_runner.py, the driver: docker evaluation path, was invisible to both rules. It is now an anchored allowlist: everything under src/coder_eval/ is core except cli/ and reports/, so a new subpackage is core by default. Measured over the real tree: +3 core files (isolation/__init__.py, isolation/docker_runner.py, resources/__init__.py), 0 removals, 0 new violations. TestCoreLayerMembership pins the non-core set against the filesystem.

  7. Lint-id bookkeeping is enforced, and CLAUDE.md's tree with it.

    • [tool.ruff.lint] external was missing CE044 and CE065. Both are @pytest.mark.lint classes, and the existing guard derived rule ids from ALL_RULES alone — blind to exactly that half. It now unions both registries and asserts both directions, so a stale id for a deleted rule fails too.
    • New CE067: CLAUDE.md's directory tree must name every top-level package member, and must name nothing that does not exist. The tree had drifted both ways — a phantom optimize/ row, and missing errors/ and plugins.py (the plugin SPI "Adding a New Agent" points at). All three are fixed.

Notable

  • All 21 definitions moved in Phase 3 are byte-identical to their pre-move form, verified by AST comparison. No rendered report byte changes; a characterization test pins the full 47-key run.json row against a pre-move snapshot.
  • Generating the table deliberately adds gpt-5.4-mini, gpt-5.4-nano, gpt-5.4-pro and gpt-5.5-pro to the frontend — those four were pure drift bookkeeping, not a product rule.
  • Fixes a latent packaging break found in passing: .gitignore's bare reports/ (meant for run output) also matched src/coder_eval/reports/, leaving __init__.py untracked and producing a wheel with zero files under coder_eval/reports/.
  • CE004 had the same import blindness as CE066. It matched only node.module, so from ..cli import x — this codebase's dominant idiom — fired on nothing, and had since the rule was written. Both rules now share one matcher covering all three spellings: absolute, relative, and from coder_eval import cli, which binds the package itself and was missed longest.
  • Two lint-harness tests were silently passing or not firing, and are fixed: four CE004 fixtures built paths with no src/ segment (three stopped firing under the anchor, one passed vacuously), and nothing pinned the trailing separator that keeps a module like reports_legacy.py core — deleting it left all 738 tests across test_custom_lint.py and test_lint_runner.py green.
  • The HTML slowest-commands truncation branch (reports/html.py:825) was the one uncovered line in the renderer. It now has a truncation and a boundary test, both mutation-checked.
  • TestRuffExternalCoversEveryRule now carries the lint mark like its siblings. make verify and the Ubuntu lint job still run it; the Windows job (-m "not live and not lint") no longer does.
  • Two deferred items are recorded in .claude/harness-candidates.md rather than done here, because each changes a rule's scope: CE004 inherits CE066's reports/ exemption through the shared predicate (latent — nothing in reports/ imports cli), and the anchored src/coder_eval/ regex is still duplicated across seven rule modules.

Plan errors, corrected by measurement

  • The first plan's criterion "import coder_eval.reports does not import claude_agent_sdk" is unachievable: models/agent_config.py imports ClaudeAgentOptions and every report module needs models. The tests assert what is true instead — durations.py is SDK-free, and reports no longer imports the SDK-shaped formatting module.
  • The first plan's "exactly four names from .stats" contradicted its own Changes body; five is correct (fmt_mean_sd needs stddev).
  • The second plan would have added a new parity class beside TestRuffExternalCoversEveryRule, leaving two checks over one list. The existing class was extended instead.

Verification

At 7a8951f5: ruff format/check clean · pyright 0 errors (src + tests configs) · 673 custom lint · 5730 tests at 92.66% coverage · evalboard 803 tests / 54 files / build ✓. At 0fabe3fb, before the follow-up commits: make pricing-mirror idempotent on a clean tree · wheel ships all 6 coder_eval/reports/ files. No follow-up commit touches src/, .gitignore or evalboard/.

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Sep 15, 2026

Copy link
Copy Markdown

Claude finished @uipreliga's task in 1m 18s —— View job


Code Review in Progress

Todo List:

  • Read .github/code_review.md for review standards
  • Read CLAUDE.md for project conventions
  • Run git diff origin/main...HEAD to see full changes
  • Review architectural changes (pricing mirror generation, reports package split)
  • Check cross-file consistency
  • Verify custom lint rules (CE004, CE065, CE066)
  • Review test coverage and characterization tests
  • Complete "What's Missing" analysis
  • Provide comprehensive review feedback

Starting code review now...

Comment thread tests/test_custom_lint.py Fixed
@uipreliga

Copy link
Copy Markdown
Collaborator Author

@claude please review this PR in full, at head 2f9bb50f (17 commits against main). The previous review stopped at "Code Review in Progress" and never posted findings. It also ran on 0fabe3fb, so it did not see the last 9 commits: the core-layer predicate allowlist, the CE004 scope fix, the ruff external sensor, CE067, and the HTML truncation tests.

@github-actions

github-actions Bot commented Sep 15, 2026

Copy link
Copy Markdown

Claude finished @uipreliga's task in 1m 28s —— View job


Code Review in Progress

Todo List:

  • Read .github/code_review.md for review standards
  • Read CLAUDE.md for project conventions
  • Get full diff with git diff origin/main...HEAD
  • Review changed files with full context
  • Perform cross-file consistency checks
  • Analyze design decisions and what's missing
  • Format and post comprehensive review findings

Starting review of PR #176 at commit 2f9bb50f (17 commits)...

@bai-uipath bai-uipath left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Fix what you agree with, otherwise lgtm. The split itself is clean, and most of the +3187 is relocation rather than new code. The layering work is the real find here: CE004 has been dead since it was written, and CE066 encodes violations that exist today rather than hypothetical ones.

Worth cutting

  • The pricing mirror. The four ids it adds to the frontend table were not drift: they were enumerated exemptions in the parity test this PR deletes, which carried its own stale-membership guard. The generator keeps only per-request billing as an exemption axis, so it reproduces three of the seven exclusions and silently overrides the other four. Latent today, since no experiment config references those models. Fix: carry the exemption set into the mirror, or drop the mirror and add the four ids to the frontend table.
  • durations.py. format_ms was already shared before this PR: both renderers imported it from the formatting module, so there was no duplication to remove. The module's own stated rationale also doesn't hold, since the reports package still pulls the SDK in through models. Fix: leave the function where it was, delete the module and its test.
  • CE067 and the architecture-notes expansion. Neither is about reports, and the notes additions land in a file the docs PR deletes.

Before merge

  • The plugin's framework-contract test. coder_eval_uipath/eval_runner/tests/skills/test_framework_contract.py importorskips the renamed module, so the only guard on the run.json row shape goes green by skipping instead of failing. One-line companion.
  • Pick a minor bump at dispatch. Four public modules are deleted and reports becomes a package with no aliases, while the release default is patch.

Minor, all optional: reports/helpers.py inherits a sixth of the module this PR dissolves, under a name that invites the same accumulation; the layering rule's relative matcher is depth-blind (latent, nothing nested today); importing the reports package now pulls all five submodules for any one name.

Land this before the docs PR either way: that PR's prose gate rejects two of the modules added here.

@uipreliga

uipreliga commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks — went through each item. Updated after rebasing onto main, which had moved under this branch (#174/#177) and settled two of these by itself.

Fixed

The pricing mirror — you were right, and I was wrong to push back. Fixed in 2c47ee5.

My earlier reply argued the generated table was a harmless superset, because the four ids carried a don't bother rationale rather than a correctness constraint. That was the wrong test. Whatever the rationale, the hand-copy did not price them and the generator did, so the refactor changed frontend behaviour — latent only because no experiment config references those models today. Reproducing the surface is the generator's job; deciding it may as well be wider was not a call to make inside this PR.

What survives from that reply is that the exemption set encoded two different claims, so they now survive separately rather than collapsing back into one list:

  • A routed model must not be priced is a fact about the RATE — an estimate would displace the captured actual per-call cost. Stays on ModelPricing.per_request_billing.
  • A heavy frontier variant is not worth pricing on the board is a fact about the FRONTEND. That is DELIBERATELY_UNMIRRORED, beside the generator.

The generated table now reproduces the hand-copy's 52 keys exactly — I diffed the key sets against main: no additions, no removals.

On my "carrying it forward reinstates the bookkeeping that shipped the bug": half right, and it pointed at the wrong fix. What shipped the claude-sonnet-5 / gpt-5.6-* bug was not having an exemption set but letting one go stale unnoticed. So the deleted test's staleness guard comes back as _assert_exemptions_are_live — an id that has left pricing.py now fails make pricing-mirror and CE065, instead of surviving as a comment asking the next reader to re-check it. CE065 asserts both axes from their declared sources, and the evalboard's consumption test pins the four as unpriced.

The layering rule's relative matcher is depth-blind. Real, fixed in 4fb00b9. imports_package compared node.module against the bare package name whenever node.level was non-zero, so from .reports import x inside orchestration/ — i.e. coder_eval.orchestration.reports — read as the reports layer, and from ...reports import x from a sub-package read as it too although it escapes coder_eval entirely. _absolute_module now resolves the dots against the importing file's own package, so both rules compare one fully-qualified name in either spelling. Pinned for both spellings.

One consequence worth naming: test_ce004_scope_is_every_module_outside_cli probed every module path with from ..cli import run_command, which after this fix is correct only from inside a sub-package. It moved to the absolute spelling — that test measures scope, and a relative probe measures depth.

CE067 and the architecture-notes expansion. CE067 is deleted, and main decided it rather than I did. I had argued it should land beside the drift it catches — but #174/#177 replaced CLAUDE.md's fenced directory tree with ls plus selective bullets, and CE067's whole invariant was "that tree names every top-level package member". The invariant is now false by design, and the bullets that replaced it are deliberately not exhaustive, so rebuilding the rule would fight a deliberate upstream choice. The half that is still real — no bullet may name a path that does not exist — is recorded in .claude/harness-candidates.md instead.

The notes expansion moved with the same restructure: .claude/architecture-notes.md is gone, so the reports-layer rationale now lives in .claude/notes/reporting.md, and I retargeted the stale reports_stats / reports_junit references in notes/timing.md and notes/orchestration.md while there.

Your prose-gate prediction also landed exactly. make docs-budget failed on the two modules you named: run_record.py at 40 own-line comments against a budget of 32, and result_metrics.py::_turn_tool_union_ms as a 198-word docstring essay. Both now carry main's own trimmed wording for the prose they inherited, including its .claude/notes/ pointers.

The plugin's framework-contract test. Right, and a plain rename would not have fixed it. coder-eval-uipath pins >=0.9.5,<0.12.0, so pointing importorskip at coder_eval.run_record makes it skip today instead of after the pin bump — the same silent green, just moved. The companion instead gates the skip on the optional dependency (coder_eval), resolves the serializer from run_record with a reports_experiment fallback while the pin still allows a pre-split release, and hard-fails when it is in neither home. Verified green against the currently installed pre-split coder-eval and against this branch's layout. It ships as its own PR on that repo.

Your call

The bump. Agreed on the reasoning — four public modules deleted, no aliases. Owner's call is patch: nothing outside this repo imports them, and the one in-house consumer is the contract test above, which now resolves either name.

Declining

durations.py. Your premise is correct and I had it wrong in the commit framing: format_ms was already shared on main, both renderers importing it from formatting.py, so the move removed no duplication. But the module never claimed to. What it claims is narrower and, I think, still true: durations.py is SDK-free, and reports no longer reaches through an SDK-shaped module for a 14-line formatter. It states in its own docstring that this does not make the package SDK-free, because models/agent_config.py imports ClaudeAgentOptions — and the tests assert only the narrow claim. So the rationale holds as written; reverting is churn with no correctness gain. Happy to drop it if you'd still rather not carry the module.

reports/helpers.py. Fair, but a rename doesn't stop accumulation — splitting it by consumer would, and that's follow-up work rather than a merge blocker.

Eager submodule import. That's what a package __init__ re-exporting a public surface does. Lazy loading would trade a real import cost for __getattr__ indirection, and nothing here sits on a hot path.

The CodeQL duplicate-json comment was already handled in 8fb5c2c.


Edited after the rebase: the pricing-mirror item moved from Declining to Fixed, retracting my earlier disagreement on it, and the CE067 item moved too — main deleted its premise. durations.py is the one item I am still declining.

uipreliga and others added 19 commits September 15, 2026 20:02
The frontend's rate table was a hand-copy of `_PRICING`, kept honest by five
layers of bookkeeping: a regex parser that re-read pricing.py at test time, a
meta-guard against that regex silently narrowing, a DELIBERATELY_UNMIRRORED
exemption set, a staleness guard for the exemption set, and a comment asking the
next reader to keep the set honest. It still shipped a real bug — claude-sonnet-5,
gpt-5.6-sol, gpt-5.6-terra and gpt-5.6-luna sat in the exemption set under "the
evalboard never runs them" while appearing ~32k / ~2k / ~17k / ~2k times in the
run corpus, so those runs rendered "—" for cost with nothing failing.

If a test can read the table, a generator can emit it. `make pricing-mirror` now
renders evalboard/lib/pricing.generated.ts from pricing.builtin_rates(), and
CE065 re-renders and diffs it. All five bookkeeping layers delete.

The one thing the exemption set encoded that was NOT bookkeeping — that three
OpenRouter models must stay unpriced on the frontend so runs.ts apportions the
provider's real per-call bill instead of a static estimate — becomes data on the
rate itself: ModelPricing.per_request_billing. The generator skips those rows, so
nothing has to remember them. The field is defaulted and last, so 4-positional
construction (including the out-of-tree coder_eval_uipath rate card) is unaffected;
it participates in register_pricing's anti-shadow comparison, which is correct.

Generating the table deliberately ADDS gpt-5.4-mini, gpt-5.4-nano, gpt-5.4-pro and
gpt-5.5-pro to the frontend — the four exemption entries that were pure drift.

Also corrects every surface that told a reader to hand-edit pricing.ts or named the
deleted parity test: the Makefile and CI-job comments, litellm/README.md's
"register in both tables" step, CLAUDE.md, and two stale evalboard consumer comments.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SGRvWucHgYgpcGhmyHrdBg
…ports

SLOW_PARAMS_PREVIEW_CHARS was defined in reports.py and used by reports_html.py,
while the module that DEFINES it truncated with the literal 50 twice. The variant
Token Usage card re-derived TokenUsage.total_tokens inline — a fourth home for
arithmetic the model already owns. Both now have exactly one definition; neither
changes a rendered byte at today's values, which is the point.

Of the 19 function-local imports across the three report modules, 15 had their
module edge at top level already, so hoisting them is adding a name to an existing
line. Three add a genuinely new top-level edge (analysis in reports.py and
reports_html.py, reports_html in reports_experiment.py — verified acyclic).

ONE stays deferred: reports.py's `from .criteria import ...`. Importing
coder_eval.criteria runs pkgutil auto-discovery with registry side effects, which
hoisting would put on the path of every `import coder_eval.reports`. It now carries
a comment saying so, and a subprocess test asserts coder_eval.criteria stays out of
sys.modules — the decision is pinned rather than remembered.

Also documents the run.json `input_tokens` seam at its writer: the key carries
uncached_input_tokens, NOT the derived TokenUsage.input_tokens total, and
evalboard/lib/runs.ts depends on that reading. Same word, two quantities — the name
is fixed by the run.json contract and cannot change without breaking archived runs.

No golden value is re-baselined: the test diff is additions only.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SGRvWucHgYgpcGhmyHrdBg
…trics and helpers

reports_stats.py had become three unrelated modules sharing a file: a
distribution-free numeric core, a set of EvaluationResult metrics the
ORCHESTRATOR consumes mid-run, and the report renderers' own helpers. The
orchestrator importing a "reports" module for a number it needs during a run is
the layering wart; the file was the reason it had to.

  stats.py           pure statistics. Imports NOTHING from coder_eval — asserted
                     by a test, not by convention, because that is the whole
                     reason the numeric core can be reasoned about in isolation.
  result_metrics.py  turn_time_buckets, visible_turn_count, has_final_reply,
                     expected_turns_overage. Consumed by the orchestrator during
                     a run as well as by the reporters, so it is not a report.
                     Deliberately NOT folded into timing.py, which has no
                     EvaluationResult dependency and is imported by every agent.
  reports_stats.py   what is left: report-shaped helpers over variant and
                     experiment results, plus the display formatters
                     (fmt_mean_sd, fmt_p) — presentation, not computation.

Its cycle rationale is restated rather than deleted: it is LIVE, not historical.
experiment -> html -> helpers, so folding the helpers into the experiment
reporter would close a cycle.

Also moves eval_result_to_task_dict to run_record.py. It writes one run.json row
— a run-record serializer, not a report — and its placement was the only reason
orchestration/batch.py reached into the reports layer at all. Moving it is what
lets Phase 4's CE066 allowlist be purely writers instead of carrying a serializer
as a permanent exception.

Every one of the 21 moved definitions is byte-identical to its pre-move form,
verified by AST comparison. A characterization test pins the full run.json row
against a snapshot captured before the move; the non-finite sensor is retargeted
to coder_eval.stats and still guards the same 7 functions.

Two test files that held only tests for moved names are merged into the files
named for those names, rather than left behind as misnamed orphans.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SGRvWucHgYgpcGhmyHrdBg
The five reports_*.py modules become a package (markdown/html/experiment/junit/
helpers) whose __init__ re-exports the public writer surface. Private names stay
private: the 12 that tests reach for are imported from their submodule, so the
package's API is not a function of its test suite.

`format_ms` moves to durations.py. formatting.py imports claude_agent_sdk for the
payload formatters, and the reports package should not reach through an SDK-shaped
module for a 14-line duration formatter. NOTE this does NOT make the package
SDK-free — models/agent_config.py imports ClaudeAgentOptions and every report
module needs models. The docstring and tests say what is actually true rather than
what the plan hoped.

CE066 pins the layering the split establishes: core may import only the package's
public WRITERS. An allowlist, not a denylist, so a new report helper is banned from
core by default. It checks BOTH the absolute and the relative spelling — the first
draft matched only `node.module`, which for `from ..reports import X` holds
"reports" with the dots in `node.level`, so it fired on neither of the two real
edges in the tree and its tests passed because they used the absolute form. An
unrun assertion is documentation, not enforcement.

The core-layer predicate moves to a shared _layers.py that CE004 and CE066 both
read, so a package added to one cannot escape the other. It is stated as "every
module directly under src/coder_eval/ is core": naming only orchestrator.py left
result_metrics.py exempt — the module CE066's own fix message tells you to move
your metric into.

Also fixes a latent packaging break: .gitignore's bare `reports/` (meant for run
output) matched src/coder_eval/reports/, leaving __init__.py untracked and building
a wheel with zero files under coder_eval/reports/. Anchored to /reports/.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The tree, the review-command module lists and a handful of prose references still
named modules that no longer exist. CLAUDE.md's `reports_html.py` line also called
it "the evalboard's static twin" — a parity promise nobody was keeping: it has ~40
private renderers and nothing tests it against the 31k-line evalboard. That line
goes with the entry.

Adds `## The reports package` to .claude/architecture-notes.md, which is where
CLAUDE.md's preamble sends a reader for rationale. It records the CE066 layering,
why stats.py / result_metrics.py / run_record.py each sit outside the package, the
relative-import trap CE066's first draft fell into, and — the part a future reader
would otherwise re-litigate — the DECISION NOT to build a shared section-data layer,
with the measurement behind it: only 1 of the 4 "duplicated" section pairs shares an
input shape, and the differences in the rest are per-surface presentation, not drift.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CE004 matched `node.module` against `^coder_eval\.cli`, but a relative import
keeps its dots in `node.level` and leaves the rest in `node.module` — so
`from ..cli import run_command` arrives as `level=2, module="cli"` and matched
nothing. The relative form is this codebase's dominant idiom, so the rule has
been guarding roughly nothing since it was written, with its tests green because
they used the absolute spelling.

Found while fixing the identical bug in CE066 during the reports split. Two rules
independently falling into the same trap is the definition of a shared helper, so
the matching moves to `_layers.imports_package` next to the core-layer predicate
both rules already share — the same reasoning that put `is_core_path` there.

The helper also fixes a narrower bug the regex had: `^coder_eval\.cli` prefix-
matched `coder_eval.client`. Matching is now on a package boundary.

Adds the regression tests both rules were missing: every spelling of a banned
import, plus the prefix-bleed case.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…idation

Both are real gaps with a known shape but neither is ~30 minutes of work: a
hand-edit guard for generated surfaces needs a checksum gate rather than a diff,
and a prose-path resolver has the same tree-parsing problem the plan already
measured and declined for CLAUDE.md alone.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three findings from the cross-phase review, all whole-diff-only:

CE004 and CE066 both missed a THIRD import spelling. `imports_package` handled
the absolute and relative forms, and `is_bare_package_import` handled
`from . import reports` — but `from coder_eval import reports` is level=0 with
module="coder_eval", which matched neither. It binds the package, so every
attribute read through it was invisible to both rules. That spelling is a real
in-tree idiom (`from coder_eval import __version__`), which the tests now pin as
the negative case alongside the positives.

reports/__init__ published nine names nothing imports. The export list was
measured before packaging as "every name imported from a reports module", which
at that point included the five modules importing EACH OTHER; those became
intra-package `from .markdown import …` and need no re-export. 25 -> 16, and the
docstring no longer claims more than it delivers.

Stale cross-repo pointers the phase greps missed by stopping at the src/
boundary: two evalboard tests still cite the deleted pricing-parity test as the
authority on rates (it is CE065 now), runs.ts and variants.ts still name
reports_experiment.py / reports_junit.py, and a workflow comment does too. The
runs.ts one matters most — it is the far half of the `input_tokens` seam Phase 2
deliberately documented at its writer.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`_layers.is_core_path` was a denylist of ten directory names plus a
top-level-module regex, so `isolation/` — the `driver: docker` evaluation
path — was invisible to BOTH CE004 and CE066. Its unanchored
`[/\\]coder_eval[/\\]` also classified a repo-root file as core, because
this project's own checkout directory is named `coder_eval`.

Replace it with the anchored allowlist its own docstring already described:
everything under `src/coder_eval/` is core except the `cli/` and `reports/`
packages, so a new subpackage is core by default rather than exempt until
someone notices. Uses the established `(?:^|[/\\])src[/\\]coder_eval[/\\]`
spelling rather than a new variant — the defect being fixed was a regex that
disagreed with its siblings.

Core-set delta: +isolation/__init__.py, +isolation/docker_runner.py,
+resources/__init__.py; zero removals; zero new CE004/CE066 violations.

Four CE004 fixtures in tests/test_lint_runner.py built paths without a `src/`
segment; three stop firing under the anchor and the fourth passes vacuously,
so all four are re-anchored. `make lint` does not run that file.

`TestCoreLayerMembership` pins the non-core set against the real filesystem
and pins the residual `~/src/coder_eval` collision as unreachable rather than
asserting it away.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`[tool.ruff.lint] external` is what stops ruff reporting RUF102 for a
`# noqa: CE0xx` it does not own. It was two ids short: CE065, and CE044 —
which the review missed and only prototyping the sensor found.

Both are `@pytest.mark.lint` classes rather than BaseRules, and that is
exactly why they slipped: `TestRuffExternalCoversEveryRule` already guarded
this list, but derived the known ids from `ALL_RULES` alone, so it was blind
to half the rule space it was meant to cover.

Extend that class rather than adding a second one beside it: `_known()` now
unions `ALL_RULES` with the `class TestCE\d{3}` ids scraped from this file,
and `test_no_dead_entry_survives` asserts the other direction, so a declared
id for a deleted rule fails too. Both messages name pyproject.toml and the
offending ids.

`tests/lint/runner.py`'s id-claiming note carried the same hand-maintained
enumeration and had fallen behind CE044 identically; it now points at the
grep instead of listing the ids.

Nothing was red for want of these two entries — no `# noqa: CE044` or
`# noqa: CE065` exists in the tree — so the fix is pre-emptive.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`reports/html.py:825` — the `params_preview += "..."` arm — was the one
uncovered line in the renderer. Its markdown twin got both a truncation and
a boundary test when SLOW_PARAMS_PREVIEW_CHARS was introduced; the HTML side
got neither.

Adds the twin pair. Both read the constant rather than the literal 50, so
they survive a change to it, and both were mutation-checked: flipping `>` to
`>=` fails the boundary case, deleting the ellipsis arm fails the truncation
case.

`str(dict)` emits single quotes that `_esc` renders as `&#x27;`, so the raw
cell is longer than the preview. The assertion unescapes before measuring;
the renderer's escaping is untouched.

No source change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The consolidation's own greps could not reach these: nine present-tense
references in tests/ naming `reports.py`, `reports_html.py`,
`reports_experiment.py` or `reports_junit.py` — modules that no longer
exist — plus CE053's fixture default, whose `: str = ` spacing hid it from
a `filepath="src/…"` grep.

Classified rather than sed'd: eight further sites keep their wording because
they date a past incident in past tense, name a test file that still exists,
or are the deliberate `reports_html` local alias. All eight verified
unedited.

Also deletes CLAUDE.md's `optimize/` tree entry. The directory is absent
from HEAD and from main; it lives only on the unmerged
`feat/plugin-optimize-skill` branch, so the tree as documented did not
match the tree as shipped.

No source file changed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two Medium findings, each flagged independently by a reviewer:

- `_layers.py` spelled the 30-char package prefix twice, in the module whose
  own docstring argues a second copy of a definition is how the two drift.
  `_NON_CORE` now derives from `_PKG.pattern`, so widening one cannot leave
  the other behind — a divergence no test could have seen, because both
  assertions only ever feed src-layout paths. Compiled pattern unchanged.

- The trailing `[/\\]` in `(cli|reports)[/\\]` is what keeps a top-level
  module whose name merely STARTS with `reports` or `cli` core. Nothing
  pinned it: deleting the separator left all 738 tests green. Pins added for
  `reports_legacy.py` and `cli_helpers.py`; both fail under that mutation.

Also, from the same reviews:

- Pin the reachability argument that made the `~/src/coder_eval` residual
  safe. It lived only as docstring prose; adding CE004 or CE066 to
  `_ALSO_SCAN_TESTS` would hand them a whole `tests/` tree that matches
  `_PKG` on an ordinary clone layout. Now one assertion.

- `tests/lint/runner.py`'s new grep told the next author to run
  `^class Test(CE\d{3})`. GNU and BSD `grep -E` read `\d` as a literal `d`
  and report zero hits — "no ids taken", the exact miss the note exists to
  prevent. Respelled `[0-9]{3}`.

- CLAUDE.md's tree audit ran one way only. Deleting the phantom `optimize/`
  entry was right, but `errors/` and `plugins.py` exist and were absent —
  and `plugins.py` is the SPI the "Adding a New Agent" section points at.

- Record `ce048`'s near-variant of the shared path regex in the candidates
  entry.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e member

Harness loop, closed in-session. The directory tree is the map an assistant
reads before touching this package, and it drifted in both directions across
two consecutive plans while only one direction was ever audited: a phantom
`optimize/` row survived for a directory that lives solely on an unmerged
branch, and the sweep that removed it walked entries -> filesystem, so it
could not see that `errors/` and `plugins.py` were missing — the second being
the plugin SPI that "Adding a New Agent" tells you to use.

Asserts both directions over the top-level rows only; nested rows stay
illustrative, so a new sibling module is not a forced docs edit. Proven to
fire each way: re-adding the `optimize/` row and deleting the `plugins.py`
row each fail with the offending name.

Writing it also found a third omission the hand audit missed — `__init__.py`,
here deliberately ignored along with the build and typing markers.

Claims id 067; the Phase 2 parity sensor required the pyproject entry
immediately, which is the sensor doing its job.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
No test datum should carry a developer's home directory. The repo-root case
now uses `/home/dev/src/exp/coder_eval/conftest.py`, which keeps the shape
that mattered — a `src` component that is NOT the package's parent, so it
exercises the anchor rather than merely the absence of `src`.

`ISOLATION` and a verbatim three-line docstring were copied into both the
CE004 and CE066 test classes. Hoisted to one module-level `CORE_ISOLATION`
with the rationale stated once, so the two pins cannot drift to different
paths. Verified the pins still bite: re-exempting `isolation/` in `_NON_CORE`
fails four tests.

Also renames `test_ce008_skips_files_outside_scope` to `ce009` — it sits in
the CE009 block and exercises `YamlModelsForbidExtras` (CE009), while CE008's
own tests cover `ReadTextExplicitEncoding`. Pre-existing mislabel, flagged in
review.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…le one

`tests/test_custom_lint.py` imports `json` at module level, and two tests
re-imported it inside the function body. CodeQL flagged the one this PR
added (in the pricing-mirror test); the other, in the activation-rows test,
predates the PR and has the same shape, so both go.

Ruff has no rule for a repeated import inside a function body, which is
why `make check` never saw either.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CE004 and CE066 shared one predicate, `is_core_path`, whose exemption set is
`{cli, reports}`. That set is CE066's: the reports package may reach into
itself. CE004's is only `{cli}` — the reports package runs without the CLI
(the orchestrator writes a task report mid-run), so a `cli` import added
inside `reports/` closes a cli -> orchestration -> reports -> cli cycle, and
CE004 would have stayed silent. Latent, not live: widening the scope finds
0 violations.

The rules still share what must not drift — the anchored package regex and
the `cli/` boundary, now `is_package_path` and `is_cli_path` — but each
states its own exemptions. CE066 keeps `is_core_path`; CE004's scope is the
package minus `cli/`.

Tests, each mutation-checked against CE004 going back to the core predicate:
- `test_the_reports_package_is_in_scope` — a `cli` import in reports/ violates.
- `TestCoreLayerMembership.test_ce004_scope_is_every_module_outside_cli` runs
  the RULE at every real module path. Its first draft recomputed the scope
  from the helpers and passed under that mutation, so it now calls the rule.
- `test_the_reports_package_itself_stays_exempt` pins that CE066's scope did
  not widen with CE004's.

Closes the deferred harness candidate; architecture notes updated.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CE004 and CE066 matched `node.module` against the bare package name whenever
`node.level` was non-zero, so `from .reports import x` inside `orchestration/` —
i.e. `coder_eval.orchestration.reports` — read as the reports layer, and
`from ...reports import x` from a sub-package read as it too although it escapes
`coder_eval` entirely. Latent: nothing in the tree is nested that way today.

`_absolute_module` now resolves the dots against the file's own package, so both
rules compare one fully-qualified name in either spelling.

The CE004 scope probe moves to the absolute spelling: a relative one resolves
against the importing file, so `..cli` names the cli layer only from inside a
sub-package and that test would measure depth instead of scope.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rebase fallout from #174/#177, which replaced CLAUDE.md's fenced directory
tree with `ls` plus selective bullets, split `.claude/architecture-notes.md`
into `.claude/notes/`, and added the `make docs-budget` prose gate.

- Drop CE067. It asserted that CLAUDE.md's tree names every top-level package
  member; that tree no longer exists, and the bullets that replaced it are
  deliberately not exhaustive, so the rule's invariant is now false by design.
  The surviving half — no bullet may name a path that does not exist — is
  recorded in .claude/harness-candidates.md rather than rebuilt here.
- Move the reports-package rationale into `.claude/notes/reporting.md`, and
  retarget `reports_stats` / `reports_junit` in notes/ to their new homes.
- Bring `result_metrics.py` and `run_record.py` under the prose budget by
  adopting main's trimmed wording for the comments and docstrings they
  inherited, including its `.claude/notes/` pointers.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@uipreliga
uipreliga force-pushed the feat/reports-consolidation branch from 4fb00b9 to b79f7df Compare September 16, 2026 03:14
The generator kept only `per_request_billing` as an exemption axis, so it
reproduced three of the hand-copy's seven exclusions and silently priced the
other four. Those four were not drift: `gpt-5.4-mini`, `gpt-5.4-nano`,
`gpt-5.4-pro` and `gpt-5.5-pro` were enumerated exemptions in the parity test
this branch deletes. Pricing them is a behaviour change smuggled in as a
refactor — latent only because no experiment config references them today.

The exemption set encoded two different claims and they now survive separately.
That a routed model MUST NOT be priced is a fact about the rate, and stays on
`ModelPricing.per_request_billing`. That a heavy frontier variant is not worth
pricing on the board is a fact about the FRONTEND, so it is
`DELIBERATELY_UNMIRRORED` beside the generator.

What the hand-copy got wrong was not having an exemption set but letting it go
stale unnoticed, so the deleted test's staleness guard comes back as
`_assert_exemptions_are_live`: an id that has left `pricing.py` fails
`make pricing-mirror` and CE065 rather than sitting there silencing nothing.

The generated table now reproduces the hand-copy's 52 keys exactly — no
additions, no removals. CE065 asserts both axes from their declared sources,
and the evalboard's consumption guard pins the four as unpriced.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@uipreliga
uipreliga merged commit 396c22c into main Sep 16, 2026
15 checks passed
@uipreliga
uipreliga deleted the feat/reports-consolidation branch September 16, 2026 03:30
uipreliga added a commit that referenced this pull request Sep 16, 2026
Rebased onto a249877, which turned the prose gate on for tests/ (#180) and
hoisted function-local imports out of src/ (#176). Brings this branch's own
additions in line:

- Three test docstrings stated history ("Previously ...", "used to hide
  behind", "Accepting it recorded"); they now state the contract.
- `report_command` and `evaluate_command` import `run_summary_rebuild` at module
  level. `evaluate_command` keeps the module import because its tests patch
  functions on that module.
- `tests/test_container_context.py` (new in this branch) imports at module level.
- Reflowed one note paragraph to the notes' 88-column width and rejoined a
  split docstring line in `regrade._should_grade_in_container`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
uipreliga added a commit that referenced this pull request Sep 16, 2026
…into report --rebuild (#178)

* feat(isolation): 1/6 — parse context.json as a ContainerContext contract

Replace the json.loads + dict.get + isinstance ladder at the host→container
boundary with a Pydantic model: every field required, extra="forbid",
StrictBool for grade/regrade, StrictInt for replicate_index. The host builds
the model in _stage_inputs and keeps it on DockerRunner._staged_context.

BREAKING CHANGE: a container image and host that disagree about the
context.json keys now fail at parse time (exit 2, naming the field) instead
of falling back to defaults (grade=True, host_task_file=None, the staged
source_yaml).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* feat(isolation): 2/6 — refuse image skew and assert the container echoed its contract

The image version preflight now refuses instead of warning, for dockerfile_path
images too, and is the single reader of the org.coder-eval.version label. The
container echoes the ContainerContext it parsed into
environment_info["container_contract"], written after the regrade seed so the
prior row cannot overwrite it. One host guard, _assert_contract_echoed,
replaces _assert_grade_honored, _assert_regrade_honored and
_warn_on_version_mismatch, and quarantines a refused record to
task.json.unhonored. A host grade drops a prior row's stale echo, and the echo
is kept out of the rendered Environment table.

BREAKING CHANGE: a driver: docker run is refused before the container starts
when the image's org.coder-eval.version label is missing or differs from the
host's installed coder-eval. Set ALLOW_IMAGE_SKEW=1 (not an empty string) to
run a deliberately different image; a missing label is never excused. An image
that predates the host→container contract is refused after the run, and its
record is moved to task.json.unhonored instead of task.json.graded /
task.json.rerun.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* refactor(isolation): 3/6 — move the docker→tempdir driver rewrite host-side

DockerRunner._stage_inputs now stages task.yaml as the execution copy with
driver: tempdir and sends the authored sandbox block in the contract as
ContainerContext.authored_sandbox. The in-container entry point no longer
rewrites the driver; it records authored_task from ctx.authored_sandbox, so a
docker run's task.json still says driver: docker. The CE051 exemption moves
with the rewrite: the two sites are now host staging and opt-in host grading.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* feat(cli): 4/6 — replace `aggregate` with `report --rebuild`

Extract the run-summary rebuild into a printer-free
orchestration/run_summary_rebuild.py (rebuild_run_summary, find_run_root) and
expose it as `coder-eval report <run_dir> --rebuild`, which prints the same
counts line and note. --rebuild refuses --format, --output, a task directory
and a directory inside another run. The rebuilt run id now comes from the
resolved directory, so `runs/latest --rebuild` records the real run id.

BREAKING CHANGE: `coder-eval aggregate` is removed; use
`coder-eval report <run_dir> --rebuild`. Its `-o/--output <dir>` option has no
replacement: --rebuild writes in place, so gather task directories into the
target directory first.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* feat(evaluate): 5/6 — refresh the run-level run.json after a detached grade

`coder-eval evaluate <task_run_dir>` now rebuilds the owning run's run.json
and run.md itself after writing the verdict back, instead of telling the user
to run a second command. The refresh is best-effort: any failure only warns,
and the exit code is always the verdict's.

find_run_root accepts only a run.json that is a coder-eval run summary (a JSON
object with run_id and task_results), so a row copied into another tool's tree
never overwrites that tool's run.json. rebuild_run_summary refuses to write
through a symlinked run.json or run.md; both evaluate and report --rebuild
inherit that, since it is their one shared write path. The refresh is skipped
when the grading --run-dir sits inside the owning run, where its own task.json
would count as a second row. write_run_summary now writes atomically, and the
new console lines escape Rich markup in paths.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* test: guard the contract echo over a maximal sandbox and the Typer-command exemption list

Two review findings from the container-contract work, closed as tests:
- the host compares the container's echo with its own dump, so a validator
  under authored_sandbox that is not idempotent across a JSON round trip would
  refuse every docker run; the round trip now runs over a maximal SandboxConfig;
- prose_budget's _TYPER_COMMANDS kept a dead entry for the deleted aggregate
  command; it must now equal the commands the CLI registers.

Two more candidates that need type information or are conventions are recorded
in .claude/harness-candidates.md.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* test(cli): 6/6 — pin every shared run/execute flag as declared identically

Phase 6 planned to declare the flags `run` and `execute` share once, through
option factories. The factory module was built and produced byte-identical
--help output, but review found it saved no lines and turned each new shared
flag into a five-place edit, so it was dropped and cli/ is unchanged.

What remains is the drift guard: a test that compares every click attribute a
shared flag can show or validate between the two commands, and asserts that
--resume and --format stay command-specific.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: code review fixes for container-contract-and-command-surface

- Strip the prior row's container_contract from the staged prior.json. An image
  that honors regrade but predates the echo keeps the prior environment_info,
  so a matching echo from an earlier identical dispatch passed as its own.
- A result refused by the contract echo now leaves a synthetic ERROR task.json
  in place of the quarantined record, like every other container failure, so
  the row no longer vanishes from the next run.json rebuild.
- On a detached grade, the refused task.json.unhonored is folded back from the
  scratch directory beside the graded row, and the error no longer suggests
  --allow-host-grading when the image itself was refused.
- Docstrings, the stale "version checks only warn" wording, the dead None branch
  in _quarantine_record, the run.json environment note, and a cwd-independent
  task path for the evaluate refresh tests.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs: apply main's prose rules to the container-contract branch

Rebased onto a249877, which turned the prose gate on for tests/ (#180) and
hoisted function-local imports out of src/ (#176). Brings this branch's own
additions in line:

- Three test docstrings stated history ("Previously ...", "used to hide
  behind", "Accepting it recorded"); they now state the contract.
- `report_command` and `evaluate_command` import `run_summary_rebuild` at module
  level. `evaluate_command` keeps the module import because its tests patch
  functions on that module.
- `tests/test_container_context.py` (new in this branch) imports at module level.
- Reflowed one note paragraph to the notes' 88-column width and rejoined a
  split docstring line in `regrade._should_grade_in_container`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants