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'", "'effective_instructions'", "'eval'", "'eval_coverage'", "'explore_runs'", "'guidance_reach'", "'headroom'", "'matrix_rollup'", "'need_graph'", "'novelty'", "'plan'", "'plugin_delta'", "'questions'", "'redteam'", "'repo_budget'", "'repo_environment'", "'repo_hits_judged'", "'repo_readiness'", "'result'", "'skill_discovery'", "'skill_evidence'", "'subject_scorecard'", "'task_inspect'", "'task_runs'", "'tool_preference'"] -> ["'answers'", "'arms'", "'cli_routes'", "'effective_instructions'", "'eval'", "'eval_coverage'", "'explore_runs'", "'guidance_reach'", "'headroom'", "'matrix_rollup'", "'need_graph'", "'novelty'", "'plan'", "'plugin_delta'", "'questions'", "'redteam'", "'repo_budget'", "'repo_environment'", "'repo_hits_judged'", "'repo_ledger'", "'repo_readiness'", "'result'", "'skill_discovery'", "'skill_evidence'", "'subject_scorecard'", "'task_inspect'", "'task_runs'", "'tool_preference'"]Updated on every push; the |
| "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.56% 94.58% +0.01%
==========================================
Files 323 324 +1
Lines 42031 42237 +206
Branches 7115 7152 +37
==========================================
+ Hits 39748 39951 +203
- Misses 1457 1458 +1
- Partials 826 828 +2
🚀 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>
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.
There was a problem hiding this comment.
Fixed on this branch by 05371b9: the example hypothesis now states review recall on the replayed PRs ("...land on more of the reviewers' threads..."), which is what grade_replay() measures. No unrelated-file-edit claim is left in the example.
Head is now c406409, a merge of intel/main that resolves the conflicts (no change to this example).
Full suite on c406409: 12498 passed, 27 skipped, 6 xfailed.
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.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> # Conflicts: # dokimi/dokimi/artifacts.py # dokimi/dokimi/runs/kinds.py # dokimi/scripts/growth_baseline.json # dokimi/tests/test_runs_scan.py # dokimi/tests/test_runs_show.py
…fficient-evidence compare scores such a cell 0 and says the measurement failed, but the ledger read that 0 as an outcome: an outage on the arm gave review, on the baseline keep. Carry the count of such pairs into the ledger, void the verdict and keep the result out of the Holm family. The plain comparison rows are unchanged. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
runs/kinds.py and the runs tests: keep repo_ledger.v1 from this branch and headroom.v1 from intel#279. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
cli_flags.py: keep --ledger from this branch and --index from intel#349. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
Merged main again after #349 (198d7ac): |
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at 198d7ac. The ledger does what the page says: verdicts read the exact p and mean, Holm runs over the rule results only, every model keeps its own result, and plain repo compare output is unchanged. What is left is small and in the line comments.
Agreed with @xaleryb on the grader-failure and outage fix: voiding the whole result is the right default, since a 0 from a failed measurement must not read as an outcome.
What I ran, at 198d7ac:
make ci-local --offline: 19 passed, 0 failed, 13 skipped; 12,888 tests passed.- A repro with hand-written batches: a rule whose arm is its own baseline, and one whose arm is the
--baseline, both end asinsufficient-evidencewith exit 0.
Checked, no issues
- Earlier threads: the bot points on rounded p and mean, the score label, the cost-only unpaired cases and the per-model results are fixed, each with a test; those bot threads are still open on GitHub but settled.
exactincompare: only the ledger passes it, sorepo compare --jsonkeeps its rounded numbers and its rows gain nounmeasuredkey.- Schema and registries:
repo_ledgeris a new kind inSCHEMA_VERSIONSandruns/kinds.py, with a fixture; alpha 0, 1 and NaN are refused. - Docs and changelog: the page is in
nav:anddocs/README.md; the fragment is under Added.
| """The rule's results of one compare() call, with its linked cases that did not pair, as warnings.""" | ||
| try: | ||
| summary = compare(batch_dirs, base, [rule.arm], models, (), seed=seed, tasks=present, exact=True) | ||
| except CompareError as exc: # the arm or the baseline has no cell among this rule's cases |
There was a problem hiding this comment.
A rule whose arm is its own baseline is reported as insufficient-evidence with exit 0, instead of being refused as a broken ledger.
This except catches every compare() error, not only "no cell among the cases". A rule with "baseline": "guidance:rules" and "arm": "guidance:rules" (or "arm": "guidance:none" under --baseline guidance:none) prints:
not measured: treatment 'guidance:rules' is the baseline
That is a typo in the ledger, shown as a verdict.
Suggestion: raise a LedgerError naming the rule, in _rule when baseline == arm and in evaluate when the arm equals rule.baseline or baseline; test both.
There was a problem hiding this comment.
Fixed in 4482d7a. A rule whose arm is its baseline (named in the rule or the default) is now refused with LedgerError: repo compare --ledger exits 2 instead of printing a verdict. Tests cover both.
| def evaluate(ledger: Ledger, batch_dirs: list[Path], baseline: str, models: list[str] | None = None, | ||
| seed: int = 0, today: date | None = None, ledger_name: str = "") -> dict[str, Any]: | ||
| """One result per rule and model: paired statistics, Holm-adjusted p, verdict and reason.""" | ||
| today = today or datetime.now(UTC).date() |
There was a problem hiding this comment.
Nit: today is the UTC date, but reviewed is a date a person writes in their own time zone.
In UTC+3 at 01:30 on 2026-11-02, a rule reviewed that day gets review date 2026-11-02 is after 2026-11-01: check the date and stale: true.
Suggestion: today = today or date.today().
There was a problem hiding this comment.
Fixed in 4482d7a. The default review date is the local date now, not UTC. The ruff DTZ011 is noqa'd with that reason, and a test pins it.
| text = format_compare(summary) | ||
| if "ledger" in summary: | ||
| text += "\n\n" + format_ledger(summary["ledger"]) | ||
| print(json.dumps(summary, indent=2) if args.json else text) |
There was a problem hiding this comment.
Nit: The text output is now built even under --json, where it is thrown away.
Before, format_compare ran only without --json. Now a formatting error in format_compare or format_ledger would also break --json.
Suggestion: build text only when not args.json.
There was a problem hiding this comment.
Fixed in 4482d7a. With --json only the JSON is built and printed, so a text formatting error no longer breaks it. A test covers this.
|
|
||
| KEEP, REVIEW, INSUFFICIENT = "keep", "review", "insufficient-evidence" | ||
| DEFAULT_REVIEW_DAYS = 180 | ||
| _EPS = 1e-9 # a mean delta this close to zero is a tie, as in compare |
There was a problem hiding this comment.
Nit: _EPS ("as in compare") and _num copy compare.EPS and compare._num.
If compare's tie tolerance changes, abs(mean) > _EPS stops matching the up/down/tied counts printed next to it.
Suggestion: import EPS from .compare, which this module already imports from.
There was a problem hiding this comment.
Fixed in 4482d7a. ledger.py imports EPS and _num from .compare instead of copying them.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
The new outage check is right, but its tests do not pin it for a single bad pair, and a typo in a ledger (a rule's arm equal to its baseline) is still reported as a verdict. Not ready to merge until both are settled.
I agree with @vlad-perevezentsev that a rule whose arm is its own baseline should be refused as a broken ledger, not reported as insufficient-evidence with exit 0. New evidence: with arm and baseline both guidance:rules, and again with arm: guidance:none under --baseline guidance:none, repo compare --ledger prints not measured: treatment ... is the baseline and exits 0.
I also agree with @vlad-perevezentsev's nits on the UTC date, the text built under --json, and the duplicated _EPS and _num. New evidence: at UTC+14 a rule reviewed on the local date 2026-11-04, with UTC still on 2026-11-03, gets review date 2026-11-04 is after 2026-11-03: check the date; the duplicated constants have equal values today.
What I ran, on a Linux review host at 198d7ac:
make ci-local: 30 passed, 0 failed, 7 skipped (other Python legs, semgrep and micromamba are not available locally); 12,941 tests passed; the changed-line coverage step passes.- A mutation run on the grader-failure and outage change in
ledger.py: 12 mutants, 4 of which pass the full suite. Two of them change a verdict and are in the inline comment. - Reproductions for the arm-equals-baseline case and the date case, with the LLM transport mocked.
Checked, no issues
- Delta since 05371b9: the
unmeasuredcount issum(failed)over the paired tasks only, and is added only on theexactpath, so plainrepo comparerows and--jsonkeep their keys. Outage and grader failure on either side, and a score rule, are pinned. - Commits: all 18 non-merge commits are signed off.
- Repo rules:
repo_ledgeris registered inSCHEMA_VERSIONS, the page is innav:, the CLI snapshot regenerates byte-identical, themax_docsbump matches the doc count, and the growth baseline entry forcompare.py(808 lines) matches the file. - Tests:
test_repo_ledger.pywrites only undertmp_pathand makes no network calls.
Bot points I checked that no longer apply at this head (16)
- 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.
- exai-bot's point on the example hypothesis does not apply: it now says the agent's review findings land on more of the reviewers' threads, which is what the replay grader measures.
# Conflicts: # dokimi/dokimi/repos/compare.py
Thread: PRRT_kwDOVBg7g86zMNi9
A rule whose arm is its own baseline (a typo, or the default baseline being the arm) was reported as insufficient-evidence with exit 0. It is now refused as a broken ledger. Also: the default review date is the local date, not UTC; `compare --json` no longer formats the text; the ledger reuses compare's EPS and _num instead of copies. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
after/guidance:rules against guidance:rules over two batches read as insufficient-evidence with exit 0; plain repo compare exits 2 on the same pair. The ledger now refuses it, naming the file and the rule. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
docs/reference/cli-interface.json regenerated with make cli-snapshot after the merge. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> # Conflicts: # dokimi/docs/architecture.md
|
DCO is the only check still failing here: some of your commits on this branch have no |
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
The merge waits for the author to sign off two commits; I found nothing to fix in the code. Everything I asked for since my last review is in: the single-pair outage case gives insufficient-evidence with no Holm slot, and a rule whose arm is its baseline, or whose arm also selects the baseline's cells, is refused with exit 2 and a message that names the file and the rule. The UTC date, the --json text build and the duplicated _EPS/_num points from @vlad-perevezentsev are fixed too.
@napetrov's DCO point waits for the author: commits bc436ca and de4a7a2 have no Signed-off-by. @xaleryb's question on whether one outage pair should void the whole result is still unanswered, and waits for the author too.
What I ran, on a Linux review host at 8574317:
make ci-local: 30 passed, 0 failed, 7 skipped (other Python legs, semgrep and micromamba not available locally); 13,600 tests passed; the changed-line coverage step passes.- A mutation run on the new refusals, the local-date default and the
--jsonpath incmd_repo_compare: 25 mutants, 23 killed. The 2 survivors change only the text of the refusal message. - CLI reproductions with the LLM transport mocked: plain
repo comparetext and--jsonare byte-identical to the base on the same batch; one outage pair among eight good ones givesinsufficient-evidencewith no Holm slot; a list, non-UTF-8, malformed, missing or directory ledger file exits 2 and names the file.
Checked, no issues
- No false refusals:
guidance:rawagainstguidance:none, andafter/guidance:rulesagainstbefore/guidance:rules, load and givekeepover 8 pairs. - Docs:
mkdocs build --strictpasses; the contract anchors the page links exist; the page's commands and flags exist at this head. - Repo rules:
repo_ledgeris inSCHEMA_VERSIONS, the page is innav:, the changelog fragment is present, and thecompare.pygrowth entry is gone because the file is back under the cap.
Bot points I checked that no longer apply at this head (16)
- 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 the self-check results. - exai-bot's point on mixing replay and gate cases in the example does not apply: the example lists the two replay cases only.
- exai-bot's point on the example hypothesis does not apply: it now says the agent's review findings land on more of the reviewers' threads, which is what the replay grader measures.
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