Repository navigation
Conversation
A guidance rule is a bet that an agent reading it errs less on the cases it was written to prevent. repo_ledger.v1 records, per rule, its hypothesis, the arm that carries it, the linked replay or executable cases, and the date a person last reviewed it. repo compare --ledger runs compare() per rule over its linked cases and gives keep, review or insufficient-evidence per rule and model, with Holm correction across the rules. A review older than the policy is a warning. Verdicts are computed; cutting a rule stays with a person. Adds dokimi/repos/ledger.py, the schema and its fixture, the --ledger flag (inputs concept), the design note docs/rule-ledger.md, and tests on synthetic batches. The benchmark golden manifest allows one more doc. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
A --model with no cell on a rule's linked cases no longer fails the whole rule: each model is compared on its own. Linked cases that do not pair are named on the result. An empty --ledger path is refused, not ignored. The docs say which baseline errors exit 2 and which are not measured; the changelog no longer calls a stale review a non-verdict. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…ns once A model whose linked cases do not pair now gets its own not-measured result when other models are measured. The result of a --model with no cell names that model. A review dated after today is stale with a date warning, not silently fresh. Each result keeps its own warnings list, so a stale review is warned once per result. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…model once A not-measured model no longer carries the not-paired warning of another model, and a model given twice is compared once, not counted as mixed. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
A rule whose linked cases mix gate and score tasks has no single test, so its p value no longer takes a slot in the Holm family of the other rules. Such a result is insufficient-evidence with that reason. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
json.loads reads NaN, and the schema bound does not reject it. NaN compares false with every p value, so every rule came out insufficient-evidence with a reason that names the alpha. Refuse it at load time. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
An alpha of 1 passed the schema (inclusive maximum), so any non-zero delta with two changed cases came out keep. Make the bound exclusive and test the edges: 0, 1, 2, -0.5, NaN and both infinities. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
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 repo compare` parameters changed:
+ added option `--ledger`
~ `dokimi runs list` parameters changed:
~ changed option `--kind`: choices: ["'answers'", "'arms'", "'cli_routes'", "'eval'", "'explore_runs'", "'guidance_reach'", "'matrix_rollup'", "'novelty'", "'plan'", "'plugin_delta'", "'questions'", "'repo_budget'", "'repo_environment'", "'repo_readiness'", "'result'", "'subject_scorecard'", "'task_inspect'", "'task_runs'", "'tool_preference'"] -> ["'answers'", "'arms'", "'cli_routes'", "'eval'", "'explore_runs'", "'guidance_reach'", "'matrix_rollup'", "'novelty'", "'plan'", "'plugin_delta'", "'questions'", "'repo_budget'", "'repo_environment'", "'repo_ledger'", "'repo_readiness'", "'result'", "'subject_scorecard'", "'task_inspect'", "'task_runs'", "'tool_preference'"]Updated on every push; the |
|
✅ OpenCodeReview: Review complete: 0 finding(s) across 20 selected item(s). Review details
|
| "rule": "Revert edits to files the change does not need.", | ||
| "source": "AGENTS.md#commits-and-pull-requests", | ||
| "hypothesis": "With the rule, agents stop touching unrelated files on the replayed PRs.", | ||
| "arm": "guidance:rules", |
There was a problem hiding this comment.
Must-fix: the example uses an arm that repo run cannot produce. repo run ... --arm guidance:rules is rejected by workspace._parse_guidance as an unknown arm; only guidance:none/raw/bridge, guidance:rev:REV and patch:FILE are supported guidance forms. Copying this ledger against normally generated batches therefore yields not measured, rather than testing the rule. Use a supported arm such as guidance:raw, or show how to create a rule-specific patch arm and use its recorded name.
Suggestion:
| "arm": "guidance:rules", | |
| "arm": "guidance:raw", |
There was a problem hiding this comment.
Fixed in cf3c659. The example now uses guidance:raw, an arm repo run can produce, so a copied ledger tests the rule instead of reporting not measured.
| ordinary comparison's flags (`--arm`, `--task`, `--metric`, ...) do not change the | ||
| ledger, which uses the rules' own cases and arms. |
There was a problem hiding this comment.
Must-fix: this blanket statement does not match the CLI. cmd_repo_compare passes args.model to evaluate, so --model m-1 excludes other models from the ledger and can change its Holm family and verdicts. Also, the ordinary comparison runs first: for example, --task ghost or --arm absent exits 2 before any ledger is evaluated. Document that successful comparisons' arm/task/metric filters do not filter ledger cases, that --model does apply, and that ordinary comparison validation must still succeed.
There was a problem hiding this comment.
Fixed in cf3c659. The section now says the ordinary comparison runs first (a rejected --task or --arm exits 2), that its filters do not change the ledger once it succeeds, and that --model does apply, since it changes the Holm family.
| return [{**head, "model": models[0] if models else None, "error": str(exc)}] | ||
| model = models[0] if models else None | ||
| warnings.extend(summary["warnings"]) | ||
| linked = [u for u in summary["unpaired"] if u["task"] in present] |
There was a problem hiding this comment.
Must-fix: compare's unpaired list includes failures to pair descriptive measures, not just gate/score outcomes. For example, take the eight fully paired improved gate cases and set one baseline cell's agent.cost_usd to null (a valid stored value). The ledger still reports paired=8 of linked=8 and keep, but also warns '1 linked case(s) not paired: no cost_usd in baseline'. Filter this list to measure=None or a primary measure before deriving missing-outcome warnings and error reasons. Add a regression test where cost or turns is unavailable but the primary outcome pairs.
There was a problem hiding this comment.
Fixed in b1af294. Only linked cases whose outcome (gate, score, or the whole task) did not pair count toward the missing-case warnings and reasons, so a missing cost or turn count no longer weakens the verdict. Regression test: test_a_missing_cost_does_not_unpair_a_gate_case.
| if not rows: | ||
| reasons = sorted({u["reason"] for u in linked}) | ||
| return [{**head, "model": model, "error": "; ".join(reasons) or "no linked case has cells in both arms"}] |
There was a problem hiding this comment.
Must-fix: when no model has a paired primary row and --model is omitted, this collapses all known models into one result with model=None. For example, create t1 in BASE only for m-1 and in ARM only for m-2, then evaluate a rule linking t1. compare() supplies unpaired entries for both models, but the ledger returns a single unnamed result combining 'no cells in treatment' and 'no cells in baseline', rather than an insufficient-evidence result for each model. Group linked failures by model before returning, as the later unpaired-model branch does. Also preserve explicitly requested models in _rule_entries' early 'not present' return, which currently collapses them into an unnamed result too. Add coverage where every model is unpaired, not only where another model successfully pairs.
There was a problem hiding this comment.
Fixed in b1af294. With no paired primary row, the ledger now emits one insufficient-evidence result per model its unpaired linked cases name, instead of one with model: null. Regression test: test_every_model_unpaired_on_the_rule_gets_its_own_result.
| for entry, adjusted in zip(scored, adjust_pvalues([e["p"] for e in scored], rounded=False), strict=True): | ||
| entry["p_holm"] = adjusted |
There was a problem hiding this comment.
Must-fix: Holm is being applied to already-rounded p values. compare.sign_flip_p() rounds to six decimals, so rounded=False here cannot restore the lost precision. For one rule with eight uniformly improved gate cases and policy.alpha=0.0078124, the exact p is 0.0078125, but this path uses 0.007812 and incorrectly returns keep (the same issue can incorrectly return review for uniformly worse cases). Preserve unrounded test p values through compare() and ledger adjustment, rounding only for display, and add a regression at this alpha boundary.
There was a problem hiding this comment.
Fixed in b1af294. compare now keeps the exact sign-flip p (no rounding), so Holm and the verdict use it. Regression test: test_a_p_on_the_alpha_boundary_is_not_rounded_across_it.
| p, mean = entry["p_holm"], entry["delta"]["mean"] | ||
| if p < alpha and abs(mean) > _EPS: |
There was a problem hiding this comment.
Must-fix: the verdict uses a display-rounded mean. compare.paired_summary() gets delta.mean from bootstrap_ci(), which rounds to four decimals. With eight score cases changing from 0.2 to 0.20001, all eight deltas exceed _EPS and p_holm is about 0.007812, but mean becomes 0.0, so this returns insufficient-evidence (and says 'no significant difference') instead of keep. The reverse change similarly loses review. Preserve the unrounded mean for the decision and round only when formatting; add both small positive and negative score-delta tests.
There was a problem hiding this comment.
Fixed in b1af294. The verdict reads the unrounded paired mean, so a small positive or negative score change keeps its keep or review verdict. Regression: test_a_small_score_change_keeps_its_sign_in_the_verdict.
| if e["paired"]: | ||
| d = e["delta"] | ||
| ci = "-" if d["lo"] is None else f"[{d['lo']:+.3f}, {d['hi']:+.3f}]" | ||
| lines.append(f" paired {e['paired']} of {e['linked']} linked: pass rate {_num(e['baseline_mean'])} -> " |
There was a problem hiding this comment.
Must-fix: this labels score-only task means as pass rates. For the score batch in test_a_score_rule_is_measured_on_score, the text reports 'pass rate 0.200 -> 0.900', although these are average scores, not success probabilities; scores can also exceed 1. Select 'pass rate' for gate and 'mean score' for score, and cover score text output in a test.
There was a problem hiding this comment.
Fixed in b1af294. Score results are labelled mean score; only gate results say pass rate. Regression: test_a_score_rule_is_reported_as_a_mean_score_not_a_pass_rate.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved ledger issues can produce incorrect verdicts from rounded p-values and mean deltas, lose model identities, and misreport pairing evidence and score metrics.
6 open issues
What changed in this PR
6 new findings.
This PR adds rule-level evaluation to repo compare, connecting guidance rules to their linked cases and reporting evidence without automatically deciding to remove rules.
Changes:
- Adds
repo_ledger.v1validation and per-rule, per-model verdicts with Holm correction and review-date warnings. - Exposes
--ledgerin the CLI and registers ledgers with artifact inspection. - Adds synthetic tests, documentation, and CLI reference updates.
| File | Description |
|---|---|
dokimi/tests/test_runs_show.py |
Adds ledger rule-count expectations. |
dokimi/tests/test_runs_scan.py |
Adds ledger scan expectations. |
dokimi/tests/test_repo_ledger.py |
Tests validation, verdicts, warnings, and CLI integration. |
dokimi/tests/fixtures/artifacts/repo_ledger.v1.json |
Provides a sample ledger artifact. |
dokimi/mkdocs.yml |
Adds the ledger documentation to navigation. |
dokimi/dokimi/schemas/repo_ledger.v1.json |
Defines the ledger format and policy constraints. |
dokimi/dokimi/runs/kinds.py |
Registers ledger rule counts for inspection. |
dokimi/dokimi/repos/ledger.py |
Implements loading, evaluation, and text output. |
dokimi/dokimi/commands/repo_run.py |
Integrates --ledger into comparison output. |
dokimi/dokimi/cli_flags.py |
Registers the ledger path flag. |
dokimi/dokimi/artifacts.py |
Registers the ledger schema version. |
dokimi/docs/rule-ledger.md |
Documents the format, verdicts, workflow, and limits. |
dokimi/docs/reference/cli-interface.json |
Updates the CLI snapshot. |
dokimi/docs/README.md |
Links the ledger guide. |
dokimi/docs/architecture.md |
Describes the ledger module’s role. |
dokimi/changelog.d/20261031_120000_napetrov_repo-rule-ledger.md |
Announces the new capability. |
dokimi/benchmarks/spec.v1.yaml |
Adjusts the documentation-count allowance. |
Review effort: Balanced
| base = rule.baseline or default_baseline | ||
| head = _head(rule, base, len(rule.cases), missing) | ||
| if not present: | ||
| return [{**head, "error": "no linked case has a finished cell"}] |
There was a problem hiding this comment.
Fixed in b1af294. The not-present early return now yields one insufficient-evidence result per requested model, with the model name kept. Regression: test_a_rule_with_no_finished_case_names_each_requested_model.
| if not rows: | ||
| reasons = sorted({u["reason"] for u in linked}) | ||
| return [{**head, "model": model, "error": "; ".join(reasons) or "no linked case has cells in both arms"}] |
There was a problem hiding this comment.
Fixed in b1af294. The per-model unpaired results are kept when no model pairs, each with its own reason. Regression: test_every_model_unpaired_on_the_rule_gets_its_own_result.
| return [{**head, "model": model, "error": "; ".join(reasons) or "no linked case has cells in both arms"}] | ||
| out = [_entry(head, r) for r in rows] | ||
| for entry in out: | ||
| own = [u for u in linked if u["model"] == entry["model"]] |
There was a problem hiding this comment.
Fixed in b1af294. Warnings and error reasons take only gate, score, or whole-task unpaired cases, so missing cost or turn metadata is ignored. Regression: test_a_missing_cost_does_not_unpair_a_gate_case.
| for entry, adjusted in zip(scored, adjust_pvalues([e["p"] for e in scored], rounded=False), strict=True): | ||
| entry["p_holm"] = adjusted |
There was a problem hiding this comment.
Fixed in b1af294. compare's sign_flip_p is unrounded, so Holm works on the exact p for gate and score rules alike. Regression: test_a_p_on_the_alpha_boundary_is_not_rounded_across_it.
| p, mean = entry["p_holm"], entry["delta"]["mean"] | ||
| if p < alpha and abs(mean) > _EPS: |
There was a problem hiding this comment.
Fixed in b1af294. The verdict uses the unrounded mean delta and rounds only for display. Regression: test_a_small_score_change_keeps_its_sign_in_the_verdict (both signs).
| if e["paired"]: | ||
| d = e["delta"] | ||
| ci = "-" if d["lo"] is None else f"[{d['lo']:+.3f}, {d['hi']:+.3f}]" | ||
| lines.append(f" paired {e['paired']} of {e['linked']} linked: pass rate {_num(e['baseline_mean'])} -> " |
There was a problem hiding this comment.
Fixed in b1af294. The label comes from the result's measure: mean score for score, pass rate for gate. Regression: test_a_score_rule_is_reported_as_a_mean_score_not_a_pass_rate.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #336 +/- ##
==========================================
+ Coverage 94.06% 94.13% +0.07%
==========================================
Files 297 303 +6
Lines 38018 38634 +616
Branches 6424 6546 +122
==========================================
+ Hits 35760 36368 +608
- Misses 1451 1454 +3
- Partials 807 812 +5
🚀 New features to boost your workflow:
|
…el applies The example used guidance:rules, which workspace._parse_guidance rejects, so a copied ledger gave "not measured". It now uses guidance:raw. The compare section says which flags reach the ledger: the ordinary comparison runs first (a --task or --arm it rejects exits 2), its filters do not change the ledger once it succeeds, and --model does apply, changing the Holm family. Thread: PRRT_kwDOVBg7g86yWhr1 Thread: PRRT_kwDOVBg7g86yWhr3 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…r identity compare's sign-flip p was rounded to six places and the paired mean was taken from a bootstrap rounded to four, so a verdict could flip across alpha or read a tie on a small real change. Both are now unrounded; display rounds them. The ledger now counts only linked cases whose outcome (gate or score, or the whole task) did not pair, so a missing cost or turn count no longer warns. When no model pairs, each model that has an unpaired linked case gets its own insufficient-evidence result, and a rule with no finished case names every requested model. Score results say "mean score", not "pass rate". Regression tests cover the alpha boundary, small score deltas both ways, per-model unpaired and missing-case results, a missing cost, and the score label. The new tests fail on the previous code. Thread: PRRT_kwDOVBg7g86yWhr5 Thread: PRRT_kwDOVBg7g86yWhr7 Thread: PRRT_kwDOVBg7g86yWhr- Thread: PRRT_kwDOVBg7g86yWhsC Thread: PRRT_kwDOVBg7g86yWhsF Thread: PRRT_kwDOVBg7g86yWh0W Thread: PRRT_kwDOVBg7g86yWh0m Thread: PRRT_kwDOVBg7g86yWh08 Thread: PRRT_kwDOVBg7g86yWh1J Thread: PRRT_kwDOVBg7g86yWh1S Thread: PRRT_kwDOVBg7g86yWh1Z Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…ding in a newline is refused The ledger's verdicts read unrounded p and mean, but the change removed the rounding from compare's own output too, so plain `repo compare --json` changed for every user (delta.mean 0.1778 became 0.17777..., p gained digits). compare, _compare_model, _compare_measure, paired_summary and sign_flip_p now take exact=False by default, and only the ledger passes exact=True. The rule id pattern ends in `$`, which Python's re also matches before a trailing newline, so "R1\n" loaded as a rule distinct from "R1" and both printed as R1. load_ledger now checks the id with fullmatch. Red-then-green tests: test_the_printed_statistics_stay_rounded_and_full_precision_is_opt_in, test_a_rule_id_with_a_trailing_newline_is_refused_not_a_second_R1. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The ledger's exact statistics add 7 lines to repos/compare.py (807 lines), and the growth ratchet fails a new over-cap file without a baseline entry. The entry is the documented route; split the file in its own PR. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| except CompareError as exc: # the arm or the baseline has no cell among this rule's cases | ||
| return [{**head, "model": models[0] if models else None, "error": str(exc)}] |
There was a problem hiding this comment.
Must-fix: without an explicit model filter, this exception path collapses all affected models into one result with model: null. For example, create baseline cells for linked task t1 on m-1 and m-2, and treatment cells only on an unrelated task. compare(..., tasks=['t1']) raises before producing model-specific unpaired records, so the ledger returns one anonymous result rather than two insufficient-evidence results. The same loss occurs when the baseline is absent from the linked cases. Discover the models from linked cells matching either selector and preserve one error result per model; add a regression covering an entirely absent arm without --model.
There was a problem hiding this comment.
Fixed in 6f94dbe. Without --model, each model with a linked cell on either side keeps its own result, each with the same reason. Pinned by test_a_rule_whose_arm_or_baseline_is_absent_names_each_model (arm and baseline).
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Not ready to merge: one model-identity bug in the ledger and two test gaps that let a changed verdict through. The rest holds up. The per-model results, the unrounded p and mean, the Holm family and the docs examples all behave as described.
What I ran, on a Linux review host at 3a7c966:
make ci-local: 30 passed, 0 failed, 7 skipped (other Python legs, semgrep, micromamba not available locally).- Reproductions for the code finding, with the LLM transport mocked.
- A mutation run on
ledger.py, thecompare.pychange and the--ledgerwiring: 49 mutants, 16 of which pass the full suite. Most of the 16 are equivalent or change printed text only. Two change a verdict and are in the inline comments.
Checked, no issues
- Docs: the example ledger and the committed fixture load through
load_ledger;--ledgeris inrepo compare --help;mkdocs build --strictpasses with the page innav:. - Docs vs behaviour:
--task nope,--arm guidance:nopeand an unknown--metricexit 2 before any ledger output;--task t_0leaves the ledger unchanged;--model m-1changes the Holm family from 3 results to 1, asrule-ledger.mdsays. - Schema:
repo_ledgeris a new kind registered inSCHEMA_VERSIONS; alpha 0, 1, NaN and infinity are refused by schema and loader alike; the growth baseline entry matches the file. - Plain
repo compare:exactdefaults to False, and the rounded output is pinned by a test. - Commits: all 11 are signed off.
Bot points I checked that no longer apply at this head (12)
- exai-bot's point on the
guidance:rulesexample arm does not apply: the example now usesguidance:raw, an armrepo runcan produce. - exai-bot's point on the blanket filter statement does not apply: the section now says the ordinary comparison runs first, its filters do not change the ledger, and
--modeldoes apply, which I confirmed by running it. - exai-bot's point on auxiliary measures in the unpaired warnings does not apply: only gate, score or whole-task failures are counted at
ledger.py:170. - exai-bot's point on the all-unpaired case collapsing to
model: nulldoes not apply to the unpaired-cells path:_unmeasurednames each model. - exai-bot's point on rounded p values in Holm does not apply: the ledger passes
exact=Trueand the alpha-boundary test pins it. - exai-bot's point on the rounded mean does not apply: the verdict reads the unrounded mean.
- Copilot's point on the early return losing requested models does not apply: line 148 returns one result per requested model.
- Copilot's point on per-model unpaired results does not apply to the unpaired-cells path:
_unmeasuredreturns one result per model. - Copilot's point on auxiliary measures in the unpaired warnings does not apply: they are filtered to gate, score or whole-task.
- Copilot's point on rounded p values does not apply: exact p is used for Holm and the verdict.
- Copilot's point on the rounded mean delta does not apply: the verdict uses the unrounded mean.
- Copilot's point on the pass-rate label does not apply: score results print
mean score.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to f9ead26, which fix the nit from my review:
f9ead26:dokimi/docs/rule-ledger.md:86: the id pattern now ends in{0,63}and states the 64-character limit, as the schema and loader enforce.
make ci-local at f9ead26: 30 passed, 0 failed, 7 skipped.
Not approving yet. Still open: the bug and the two risks from my review.
A rule whose arm or baseline has no cell among its linked tasks went to insufficient-evidence with one result for the first --model or none, so models with linked cells on the other side lost their identity. Without --model, every model with a linked cell on either side now keeps its own result, each with the same reason. Pins the alpha boundary (p_holm equal to alpha is not a keep) and that compare --ledger with --model judges only that model. Refs PRRT_kwDOVBg7g86yX9Jj, PRRT_kwDOVBg7g86yavjT, PRRT_kwDOVBg7g86yavjW, PRRT_kwDOVBg7g86yavjZ Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
compare.py is over the 800-line cap by 7 lines. The growth baseline entry stays (reason in scripts/growth_baseline.json). The file split is a follow-up: #348. |
| |---|---|---| | ||
| | Replay tasks from review threads | `dokimi/repos/replay/`, `repo tasks replay` ([contract §4.6](repo-evaluation-contract.md#46-replay-tasks-from-review-threads)) | One `dokimi:review_replay` task per merged PR, with the change as reviewed and the reviewers' threads as gold. No GitHub call. | | ||
| | Deterministic review grader | `dokimi/repos/graders/review.py` | Matches the agent's findings JSON to gold threads by file and a line window (`window`, default 5). No LLM. | | ||
| | Executable tasks and self-check | `repo tasks check` (`dokimi/repos/selfcheck.py`, [contract §4.3](repo-evaluation-contract.md#43-self-check-tasks-check-no-llm)) | A task must fail on the untouched workspace and pass after its oracle, so a case with a broken check cannot reach a paid run. | |
There was a problem hiding this comment.
Must-fix: repo run does not run the null/oracle self-check or read its check.json; _run_locked() performs an isolation probe and then calls run_batch(). Thus, running an executable task with a broken verifier directly through repo run can still start paid cells. The existing selfcheck docstring makes the same claim, but this new page should not repeat it as an enforced guarantee. Describe repo tasks check as a prerequisite the user must run and inspect, rather than saying broken checks cannot reach paid runs.
Suggestion:
| | Executable tasks and self-check | `repo tasks check` (`dokimi/repos/selfcheck.py`, [contract §4.3](repo-evaluation-contract.md#43-self-check-tasks-check-no-llm)) | A task must fail on the untouched workspace and pass after its oracle, so a case with a broken check cannot reach a paid run. | | |
| | Executable tasks and self-check | `repo tasks check` (`dokimi/repos/selfcheck.py`, [contract §4.3](repo-evaluation-contract.md#43-self-check-tasks-check-no-llm)) | Checks that a task fails on the untouched workspace and passes after its oracle. Run this check and resolve broken cases before starting paid runs; `repo run` does not enforce its results. | |
There was a problem hiding this comment.
Fixed in 45c34c2: the row now says to run repo tasks check and resolve broken cases first, and that repo run does not enforce its results.
Checked in code: _run_locked (dokimi/commands/repo_run.py) calls isolation_probe, not the per-task check, so the old wording was wrong.
Docs-only change. test_repo_ledger.py, test_repo_compare.py, test_growth_ratchet.py and the docs tests pass (479 passed, 1 skipped).
The new page said a case with a broken check cannot reach a paid run. repo run runs only an isolation probe; the per-task check is a step the user runs first. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| "hypothesis": "With the rule, agents stop touching unrelated files on the replayed PRs.", | ||
| "arm": "guidance:raw", | ||
| "baseline": "guidance:none", | ||
| "cases": ["review_replay_pr3580", "review_replay_pr3659", "fix_df_extratrees_reg"], |
There was a problem hiding this comment.
Must-fix: this example mixes score-only replay tasks (dokimi:review_replay) with the executable gate task fix_df_extratrees_reg. When both kinds have paired cells, the ledger marks the results insufficient-evidence and excludes them from Holm regardless of the measured improvement (test_a_rule_with_mixed_kinds covers this behavior). Use only replay cases here, or put the executable cases in a separate rule, so the example follows the one-kind-per-rule requirement documented below.
Suggestion:
| "cases": ["review_replay_pr3580", "review_replay_pr3659", "fix_df_extratrees_reg"], | |
| "cases": ["review_replay_pr3580", "review_replay_pr3659"], |
There was a problem hiding this comment.
Fixed in f847271. The example now lists the two replay cases only (review_replay_pr3580, review_replay_pr3659). The gate task fix_df_extratrees_reg is left out of this rule, as the doc's "gate and score cases do not mix in one rule" requires.
| A ledger that does not validate, or a default `--baseline` the batches do not have, | ||
| exits 2 with a `✗` line naming the file (and the rule, where the error is the rule's). |
There was a problem hiding this comment.
Nit: the file-naming guarantee does not apply to an unavailable default baseline. For example, repo compare BATCH --baseline absent --ledger rules.json fails in the ordinary compare() before loading the ledger and prints ✗ no finished cell matches the baseline 'absent', with no ledger filename. Separate the two error descriptions: ledger validation errors name the ledger file; an unavailable default baseline exits 2 with the ordinary comparison diagnostic.
There was a problem hiding this comment.
Fixed in f847271 (docs only). You are right that the comparison runs first: an unmatched --baseline exits 2 with the ordinary no finished cell matches the baseline line and never names the ledger. The sentence now says so. A ledger that does not validate still names the file and, where it applies, the rule.
The example rule mixed a score-only replay case with a gate task, which the ledger marks insufficient-evidence. Keep the replay cases only. A --baseline the batches do not have fails in compare() before the ledger loads, so its exit-2 line does not name the ledger file. Say so. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| "id": "R13", | ||
| "rule": "Revert edits to files the change does not need.", | ||
| "source": "AGENTS.md#commits-and-pull-requests", | ||
| "hypothesis": "With the rule, agents stop touching unrelated files on the replayed PRs.", |
There was a problem hiding this comment.
Must-fix: these review_replay_pr* cases measure recall of human review threads, not whether an agent edits unrelated files. grade_replay() scores findings matched to gold file/line anchors, so even a significant keep result cannot test the stated hypothesis that agents stop touching files. Make this a review-oriented rule/hypothesis (for example, detecting unrelated edits in reviewed changes), or use executable cases whose grader checks the editing behavior. Otherwise the example encourages interpreting a verdict as evidence for an unmeasured outcome.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
LGTM. All three of my earlier threads are fixed with tests that fail on the old code, and the per-model results for an absent arm or baseline now behave as described. One nit inline.
What I ran, on a Linux review host at f847271:
make ci-local: 30 passed, 0 failed, 7 skipped; 12,062 tests; changed-line coverage 99%- Reproductions for the per-model results with the LLM transport mocked, and a mutation run on
ledger.pyand the--ledgerwiring: 17 mutants, 11 killed. The 6 survivors change the order or label of an error row, not a verdict. - The three new tests fail on the previous code:
p < alphatop <= alpha,args.modeltoNonein theevaluatecall, and the old_comparedat f9ead26.
Checked, no issues
- Docs vs behaviour:
--baseline absentexits 2 with the ordinary✗ no finished cell matches the baseline 'absent'line, which does not name the ledger. An invalid ledger exits 2 and names the file, as the page now says. - Commits: all non-merge commits are signed off.
- Repo rules:
repo_ledgeris a new kind registered inSCHEMA_VERSIONS, the page is innav:, the changelog fragment and the growth baseline entry are present.
Bot points I checked that no longer apply at this head (15)
- exai-bot's point on the
guidance:rulesexample arm does not apply: the example usesguidance:raw, an armrepo runcan produce. - exai-bot's point on the blanket filter statement does not apply: the page says the ordinary comparison runs first, its filters do not change the ledger, and
--modeldoes apply. - exai-bot's point on auxiliary measures in the unpaired warnings does not apply: only gate, score or whole-task failures are counted, and a test pins a null
cost_usd. - exai-bot's point on the all-unpaired case collapsing to
model: nulldoes not apply: each model gets its own result, and a test covers the arm-only-on-one-model case. - exai-bot's point on rounded p values in Holm does not apply: the ledger passes
exact=True, and the alpha-boundary tests pin it. - exai-bot's point on the rounded mean does not apply: the verdict reads the unrounded mean.
- Copilot's point on the early return losing requested models does not apply: it returns one result per requested model.
- Copilot's point on per-model unpaired results does not apply: each model keeps its own result and reason.
- Copilot's point on auxiliary measures in the unpaired warnings does not apply: they are filtered to gate, score or whole-task.
- Copilot's point on rounded p values does not apply: the exact p is used for Holm and the verdict.
- Copilot's point on the rounded mean delta does not apply: the verdict uses the unrounded mean.
- Copilot's point on the pass-rate label does not apply: score results print
mean score. - exai-bot's point on the exception path collapsing models into
model: nulldoes not apply: with the arm or baseline absent, each model with a linked cell on either side gets its own result, and a parametrized test covers both cases. - exai-bot's point on the self-check row does not apply: the row now says
repo rundoes not enforce therepo tasks checkresults. - exai-bot's point on mixing replay and gate cases in the example does not apply: the example lists the two replay cases only.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to 05371b9, which fix the nit from my review:
05371b9:dokimi/docs/rule-ledger.md:74: the example hypothesis now says the agent's review findings land on more of the reviewers' threads, as the suggestion worded it.
make ci-local at 05371b9: 30 passed, 0 failed, 7 skipped.
GitHub CI has not run on these commits: the branch conflicts with main, and Actions runs no workflow on a conflicting PR. CI will check them once main is merged in.
Not approving until CI passes at this head.
Summary
A
repo_ledger.v1file lists agent guidance rules, each with the cases its failures show up in.repo compare --ledger FILEscores every rule against its baseline over its own linked cases and returns one verdict per rule.Files:
Motivating study: agentsmd/agents.md#213 (jimy-r, held-out ablation).
Evidence
Before:
repo compare --ledgerdoes not exist;--ledgeris not in the CLI registry.After:
tests/test_repo_ledger.py: 44 passed, ruff clean.Before: a rule with NaN alpha, or alpha 1, reached
evaluate. NaN turned every ruleinsufficient-evidencewithalpha nanin the reason, exit 0.After:
load_ledgerrefuses NaN, both infinities, 0, 1, 2 and -0.5 with a message that namespolicy.alpha. Covered by a parametrized test.Before: a rule whose linked cases mixed gate and score tasks entered the Holm family, where its p value had no single test behind it.
After: such a rule is
insufficient-evidencewith a reason and takes no slot in the family. Test:test_a_mixed_rule_takes_no_slot_in_the_holm_family.ci-local on
ef75b275: 1 FAIL,tests/test_process.py::test_sighup_kills_the_live_groups_too(asubprocess.TimeoutExpiredatwait(timeout=30)). This branch does not touchprocess.pyor that test. It passes alone 3 of 3. The box was under load from other sessions (load average 190 to 320) during the runs, and in earlier full runstest_reference_checksalso failed a timing assertion. Those tests pass alone too. The FAIL is therefore a load flake, not a result of this change; the rest of the suite is green (12070 passed).Self-review: sonnet fresh-context passes on rounds 2 and 3 and on
ca364528: blocking none.Round 2-3 threads: a model with no linked cell keeps its own result (
6f94dbe6); the verdict and Holm p use the unrounded values (b1af2947); score rules are labelledmean score(b1af2947).CI on
6f94dbe6: mypy failed onledger.py:166(aNonein alist[str]). Fixed inca364528by typing the local list;mypyclean.Earlier pr-review pass on
54b9992a: plain pass 0 rows, edge pass 3 candidates. One blocker (NaN alpha), fixed ine8e42b36. Declined nits: a rule id with a trailing newline loads as distinct fromR1;compare()runs once per rule and model, reloading cells each time (cost on a real batch not measured).Merge Danger
Door: two-way for the flag and the code path. One-way for the
repo_ledger.v1format once people write ledgers against it, so the schema is the part to read closely.Without
--ledger,repo comparebehaves as before. The only change on that path is thatcmd_repo_comparenow checksargs.ledger is not None(the empty string is no longer ignored).Blast Radius: low. New module, new flag, new schema, new artifact kind
repo_ledger, CLI snapshot regenerated (docs/reference/cli-interface.json).Ratchet:
compare.pyis over the 800-line growth cap by 7 lines;scripts/growth_baseline.jsoncarries a reason-bearing entry. The split is tracked in #348.Coordination:
benchmarks/spec.v1.yamlgoldenmax_docsis bumped 37 to 38 because the new doc counts. #334 bumps it too. Whichever merges second rebases to 39.🤖 Generated with Claude Code