Skip to content

test(cli): flag registry, ratchet contract test, CLI snapshot (#94 F1a) - #254

Merged
antonwolfy merged 15 commits into
intel:mainfrom
napetrov:test/cli-flag-registry
Oct 30, 2026
Merged

antonwolfy merged 15 commits into
intel:mainfrom
napetrov:test/cli-flag-registry

Conversation

@napetrov

@napetrov napetrov commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Refs #94 (§5 stage F1a). Draft: D10 is still under review. No flag, default or output changes; the numbers are unchanged.

Summary

dokimi/dokimi/cli_flags.py          data only: 23 concepts (§2.4 rows)
  Concept(name, canonical=(Flag,..), banned=(...))
  registry_problems(concepts=CONCEPTS): one case per check in tests/test_cli_flags.py
tests/test_cli_flags.py             ratchet over dokimi.cli.build_parser()
  every option: canonical, or on tests/data/cli_flags_allowlist.txt
  banned synonym (not allowlisted): fails, names the canonical spelling
  allowlist row unused, or now canonical: fails (the list keeps no stale row)
  spellings only: an option's type and action are not compared with the registry's `kind`
  snapshot content: checked on Python 3.13 (see below)
docs/reference/cli-interface.json   #88 surface dump; make cli-snapshot regenerates it
tests/data/cli_flags_allowlist.txt  7 rows added for flags main gained (#172, #217), each with a reason
.github/workflows/ci.yml            typecheck job: "CLI snapshot is current" step (3.13)
CONTRIBUTING.md                     "Adding or renaming a CLI flag" (how to add a flag)
changelog.d/..._test-cli-flag-registry.md   Documentation

Scope is the registry and its checks only. cli_flags.py is read by no command; F1b moves each leaf onto it and tombstones old spellings.

Review round (f309ca5 → this head)

  • The docs page docs/contributing-cli-flags.md (the 33rd Markdown file under docs/, past max_docs: 32) is now a section of CONTRIBUTING.md. The page, its mkdocs.yml nav entry and its docs/README.md entry are gone. max_docs is unchanged: docs/**/*.md counts 32.
  • intel/main is merged in. Main gained flags the registry did not know: catalog trigger-probe (--claude, --sealed, --listing-budget-fraction, --trials, feat(catalog): trigger-probe in real Claude Code; payload leak audit #217) and novelty routing (--all-vs-all, --include-disabled, --queries-by-skill, feat(novelty): routing takes the catalog field, skips disabled and mirrored skills, adds all-vs-all #172). Each has an allowlist row with its reason. The snapshot is regenerated with make cli-snapshot.
  • --trials is a banned synonym of reps. The earlier table here asked for a rename to --reps; this PR does not do it, because renaming a shipped flag changes the CLI. It stays on the allowlist until F1b.
  • The banned-synonym probe in test_a_new_banned_synonym_fails_and_names_the_registry_spelling used --trials, which is now allowlisted, so it runs with that row removed.
  • The nit from the review (registry_problems untested per check) was fixed in f309ca5 and is pinned by one test per check.

Friction for flag-adding PRs

  • Canonical spelling, nothing else: run make cli-snapshot and commit the JSON.
  • Non-canonical spelling: one allowlist row with a reason, or rename to the canonical one.
  • Banned synonym (not allowlisted): rename.

An allowlist row exempts its spelling on every command, not only the one it was added for. A new command may therefore use an allowlisted banned spelling without failing. F1a accepts this: the check does not stop an allowlisted banned spelling from spreading to new commands. Keying the allowlist on (command, option) would close that gap; it is left for F1b. Adding an allowlist row is a review decision; the test does not check that a new row has a reason.

Open PRs that add flags (#155 is the only one still open; #172 and #217 merged and are now on the allowlist): #155 adds --arm --contract --json --outcomes --pass-k --path-stability --shell --threshold. --outcomes, --contract, --shell, --pass-k and --path-stability need one allowlist row each when #155 rebases onto this branch.

Evidence

Temporary edit to dokimi repo scan (reverted): --probe-flag (not in the registry) and --trials (banned for reps). Excerpt of the failing run (messages wrapped here, otherwise verbatim):

$ pytest tests/test_cli_flags.py -rf
E   AssertionError: dokimi repo scan: `--probe-flag` is neither a canonical spelling in dokimi/dokimi/cli_flags.py
    nor on dokimi/tests/data/cli_flags_allowlist.txt. Use the registry spelling of its concept, or, with a
    reason, add `--probe-flag  # <reason>` to dokimi/tests/data/cli_flags_allowlist.txt. Then run
    `make cli-snapshot` if the CLI surface changed.
E         dokimi repo scan: `--trials` is a banned synonym of `reps`. Use `--reps` (dokimi/dokimi/cli_flags.py).
FAILED tests/test_cli_flags.py::test_every_spelling_on_the_tree_is_canonical_or_allowlisted
2 failed, 9 passed

After merging main and before this round's fixes, the ratchet failed on the seven flags above and the snapshot was stale (2 failed, 9 passed). With the rows added and the snapshot regenerated, the banned-synonym probe still failed (1 failed, 15 passed), because --trials is now allowlisted; the probe now runs without that row. After this round: pytest tests/test_cli_flags.py tests/test_import_architecture.py gives 37 passed.

make ci-local on 393b7a5 (box run, TMPDIR=/tmp/...): every gate passes except two. pip-audit: CVEs against filelock==3.32.7, which main pins; this PR changes no dependency file, so it is inherited. The test step: 2 failed on the first run (AF_UNIX path too long under this box's long scratch path, and an unraisable socket warning), and both pass alone. The full suite on this head, pytest -n auto, gives 9707 passed, 5 skipped.

Merge Danger

Door: two-way. Test, data, docs and CI-step only; no runtime path imports cli_flags. Reverting the commits removes the registry, the snapshot and the typecheck step.

Blast Radius: CI. A PR that changes the CLI surface must regenerate the snapshot (the typecheck step fails it otherwise). Two open PRs that each change the surface can both pass CI against an older main and merge one after the other: main then holds a stale snapshot and the typecheck step goes red until someone regenerates it. Nearby edits to the 13k-line JSON also conflict. Branch protection with "require branches to be up to date" closes the first gap; it is not configured here. The cli-interface workflow (#88) is unchanged and report-only.

Changelog: the fragment is filed under Documentation, the existing category for contributor-facing docs. scriv's configured categories (pyproject.toml) have no Internal. The changelog.d/README.md route for internal changes is the skip-changelog label; a maintainer may prefer that.

🤖 Generated with Claude Code

… F1a)

Data and tests only; no flag, default or output changes.

- dokimi/cli_flags.py: each flag concept of intel#94 section 2.4 with its canonical
  spellings, type and banned synonyms. Nothing reads it yet (F1b moves leaves).
- tests/test_cli_flags.py: walks dokimi.cli.build_parser() in ratchet mode. A
  spelling that is neither canonical nor allowlisted fails, a banned synonym
  fails with the canonical spelling named, and allowlist rows that are unused or
  already canonical fail, so the allowlist only shrinks. An allowlist row exempts
  its spelling on every command.
- tests/data/cli_flags_allowlist.txt: today's non-canonical spellings, one reason
  per row where it is not obvious.
- docs/reference/cli-interface.json: the intel#88 surface dump, regenerated by
  `make cli-snapshot` (venv Python). The content check runs on Python 3.13, the
  pinned version: the dumper hashes validators with ast.dump, which differs by
  version. CI's typecheck job runs it on every PR (a PR runs no 3.13 test leg);
  the ratchet and structural checks run on every test leg of the matrix.
- tests/test_import_architecture.py: cli_flags sits in the foundation layer.
- docs/contributing-cli-flags.md: how to add or rename a flag.
- Internal changelog fragment.

Refs intel#94

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
@codecov-commenter

codecov-commenter commented Oct 27, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.49%. Comparing base (f0f95a2) to head (6be19a2).
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #254   +/-   ##
=======================================
  Coverage   93.48%   93.49%           
=======================================
  Files         272      273    +1     
  Lines       35363    35419   +56     
  Branches     6001     6013   +12     
=======================================
+ Hits        33060    33114   +54     
- Misses       1489     1491    +2     
  Partials      814      814           
Files with missing lines Coverage Δ
dokimi/dokimi/cli_flags.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@napetrov
napetrov marked this pull request as ready for review October 28, 2026 06:49

@antonwolfy antonwolfy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The registry, the ratchet and the snapshot step hold up, but the PR adds a docs page that pushes the docs benchmark over its cap, so make ci-local fails on the benchmark step. One bug inline (the page that crosses the cap), one nit on the registry self-check test.

What I ran, on a Linux review host at ada2d05:

  • make ci-local: 28 passed, 1 failed, 7 skipped. The failure is agent-quality.yml: Run benchmark (below). The new CLI snapshot is current step, mypy, deptry, mkdocs build and the wheel smoke tests pass.
  • 9,043 tests passed, 3 skipped; changed-line coverage 90% (cli_flags.py lines 312, 314, 320, 322, 325 are not covered).
  • A mutation run over the new test file and registry: 30 mutants, the ratchet and walker mutants are all killed; the five registry_problems checks and the CANONICAL/BANNED tables survive.

Checked, no issues

  • Snapshot: the dumper run twice gives byte-identical output, equal to the committed cli-interface.json, with no host paths in it. The CI step's working directory, PYTHONPATH and Python 3.13 match the snapshot's ast.dump hashing.
  • Ratchet: every option on build_parser() is canonical or allowlisted; all 194 allowlist rows are used, none duplicated; unused and now-canonical rows fail, as do banned synonyms that are not allowlisted.
  • Layering and docs: dokimi.cli_flags sits in the foundation layer and test_import_architecture.py passes; the new page is in mkdocs.yml nav and docs/README.md.

Comment thread dokimi/docs/contributing-cli-flags.md Outdated
Comment thread dokimi/tests/test_cli_flags.py
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>

@antonwolfy antonwolfy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I pushed a commit per nit, up to f309ca5, which fix the nit from my review:

  • f309ca5: dokimi/dokimi/cli_flags.py:305: registry_problems takes the concepts as an argument (default CONCEPTS), and dokimi/tests/test_cli_flags.py adds one case per check asserting the message.

make ci-local at f309ca5: 28 passed, 1 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 yet. Still open: the bug from my review.

…flag-registry

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>

# Conflicts:
#	dokimi/tests/test_import_architecture.py
The new page made the 33rd Markdown file under docs/, past the docs benchmark's
max_docs of 32. The how-to now lives in a section of CONTRIBUTING.md under
"Adding things"; the page, its nav entry and its docs/README.md entry are removed.
max_docs is unchanged.

Thread: PRRT_kwDOVBg7g86xE1Df
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…obe strict

Main now has flags the registry does not know: catalog trigger-probe's --claude,
--sealed, --listing-budget-fraction and --trials (intel#217), and novelty routing's
--all-vs-all, --include-disabled and --queries-by-skill (intel#172). Each gets an
allowlist row with its reason. --trials is a banned synonym of reps; renaming it
would change a shipped command, so it waits for the F1b migration.

The banned-synonym probe in test_cli_flags.py used --trials, which is now
allowlisted, so the probe would pass for the wrong reason. It runs with that row
removed instead.

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Written by make cli-snapshot on the pinned interpreter; no hand edits.

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
There is no Internal category in scriv's configuration; the fragment now uses
Documentation, the existing category for contributor-facing docs.

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
f309ca5 makes registry_problems take the concepts and adds one case per check
(tests/test_cli_flags.py), so each of the five checks is shown to fail.

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
catalog trigger-probe's own --trials produces the same message, so the probe
passed whether or not the injected dokimi run leaf was walked. The assertion
now requires the message to name dokimi run.

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>

@vchamarthi vchamarthi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review: Needs fixes before merge

Adds a CLI flag registry (concepts, canonical spellings, banned synonyms) and a ratchet contract test over the CLI tree, plus the #88 interface snapshot regenerated. Data only: no command reads the registry yet.

Blocking

  • dokimi/benchmarks/spec.v1.yaml:83: this PR adds a 33rd markdown page under docs/ while golden_manifest.max_docs stays at 32, so the benchmark step raises golden_manifest violation: matched 33 document(s) ... more than max_docs=32 on the file's own stated rule (raise max_docs in the same commit that adds the doc). Fix: bump max_docs to 33 in this PR.
  • merge conflict with main: this PR cannot merge as is and needs a rebase or merge-conflict resolution against the current main branch, independent of the review findings below.

Earlier review threads

Status Count Threads
Fixed, with a test 1 untested registry_problems branches (now parametrized, 5 cases)
Still open 1 docs golden_manifest max_docs vs 33rd doc

CI

Check Result Cause
benchmark / Run benchmark fail introduced (max_docs, same as the open thread above)
static / schema_compat vs the PR base fail pre-existing on the merge base (no schema file in this PR's diff)
test / Run tests fail pre-existing (3 failures, none in files this PR touches: two need an optional dependency not installed, one is a flaky timeout)
pip-audit fail pre-existing (known CVEs in a transitively pinned package, unrelated to this PR)
everything else (lint, type check, CLI snapshot check, coverage, package/install smoke, docs build, CLI surface diff) pass

This PR also has a merge conflict against main that must be resolved before it can land.

Lines outside the diff, so listed here:

dokimi/benchmarks/spec.v1.yaml:83: blocking (still open from @antonwolfy): max_docs is still 32 while this PR's new page (docs/contributing-cli-flags.md) brings the matched set to 33.

This line's own comment (above, line 80) says to raise max_docs in the same commit that adds a doc. The benchmark step fails with golden_manifest violation: matched 33 document(s) under .../dokimi, more than max_docs=32. include=['docs/**/*.md'] exclude=['docs/archive/**'].

Suggested fix:

  max_docs: 33

dokimi/tests/test_cli_flags.py:144: Fixed (was raised by @antonwolfy): registry_problems now takes a concepts argument (default CONCEPTS), and test_registry_problems_flags_each_inconsistency exercises each of the 5 inconsistency checks with a two-concept fixture, asserting the exact message. The earlier gap (only the real registry was exercised, so a dropped check still passed) is closed.

@napetrov

Copy link
Copy Markdown
Contributor Author

Re the review "Not approving yet" by antonwolfy: the one open item was the docs-page cap. It is fixed in 46bed29: the page is removed, its how-to is a section of dokimi/CONTRIBUTING.md, and docs/**/*.md counts 32 again. main is merged in (fee540d). The nit is already fixed in f309ca5.

Declined: raising max_docs. The cap is what makes a new doc a deliberate choice.

@napetrov

Copy link
Copy Markdown
Contributor Author

Reply to the "Needs fixes" review, on head 393b7a5.

Blocking: max_docs 32 vs the 33rd doc. Fixed in 46bed29; max_docs is not raised. The page, its mkdocs.yml nav entry and its docs/README.md entry are removed, and the how-to is the section "Adding or renaming a CLI flag" in dokimi/CONTRIBUTING.md. On head, select_docs over benchmarks/spec.v1.yaml matches 32 documents against max_docs: 32. Raising it to 33 is declined: no doc is added, so there is nothing to raise it for, and the cap is what makes a new doc a deliberate choice.

Blocking: merge conflict with main. Resolved. main is merged in (fee540d). Head is MERGEABLE, and a merge of head with current origin/main (114cb6c) is clean.

Earlier thread: untested registry_problems branches. Fixed in f309ca5. registry_problems takes concepts (default CONCEPTS), and test_registry_problems_flags_each_inconsistency has one case per check, each asserting the message. On 393b7a5, tests/test_cli_flags.py and tests/test_import_architecture.py pass (36 passed, 1 skipped).

…flag-registry

Signed-off-by: Petrov, Nikolay A <nikolay.a.petrov@intel.com>


The docs page that crossed max_docs is removed in 46bed29; the registry_problems
case is covered by the parametrized test in f309ca5.

Signed-off-by: Petrov, Nikolay A <nikolay.a.petrov@intel.com>
The nit this review reports (registry_problems takes the concepts) is in f309ca5.

Signed-off-by: Petrov, Nikolay A <nikolay.a.petrov@intel.com>
antonwolfy
antonwolfy previously approved these changes Oct 29, 2026

@antonwolfy antonwolfy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. The docs page that crossed max_docs is gone, its how-to is a section of CONTRIBUTING.md, and the golden set is back at 32 documents against max_docs: 32. The new allowlist rows and the regenerated snapshot match the tree after the merge of main.

What I ran, on a Linux review host at 33d8ff2:

  • make ci-local: 27 passed, 2 failed, 7 skipped. tests/test_cli_flags.py and tests/test_import_architecture.py pass. pip-audit fails on filelock==3.32.7 in the lock files, which this PR does not touch. Run tests has one failure, tests/test_reference_checks.py::test_the_regex_limit_is_shared_by_every_pattern_of_one_check, a wall-clock bound (4695.04 - 4693.45 < 1.5) that passes when run alone; the PR does not touch that file.
  • A mutation run over cli_flags.py and the new test file: 21 mutants. The registry_problems checks, canonical renames and the walker are killed.
  • The snapshot dumper on Python 3.13, run on a clean export of the head: byte-identical to the committed cli-interface.json.

Checked, no issues

  • Docs cap: docs/**/*.md without docs/archive counts 32; nothing in the tree still names contributing-cli-flags.md.
  • Banned-synonym probe: it now runs with the --trials row removed and requires the message to start with dokimi run: , so catalog trigger-probe's own --trials cannot satisfy it.
  • Sign-off: every non-merge commit carries a Signed-off-by.

@antonwolfy

Copy link
Copy Markdown
Contributor

@napetrov merged into the current main (0a63f4e), this PR fails these tests:

  • tests/test_cli_flags.py::test_every_spelling_on_the_tree_is_canonical_or_allowlisted
  • tests/test_cli_flags.py::test_the_cli_interface_snapshot_is_current
AssertionError: dokimi context consistency: `--outcomes` is neither a canonical spelling in dokimi/dokimi/cli_flags.py nor on dokimi/tests/data/cli_flags_allowlist.txt. Use the registry spelling of its concept, or, with a reason, add `--outcomes  # <reason>` to dokimi/tests/data/cli_flags_allowlist.txt. Then run `make cli-snapshot` if the CLI surface changed.
dokimi context consistency: `--contract` is neither a canonical spelling in dokimi/dokimi/cli_flags.py nor on dokimi/tests/data/cli_flags_allowlist.txt. Use the registry spelling of its concept, or, with a reason, add `--contract  # <reason>` to dokimi/tests/data/cli_flags_allowlist.txt. Then run `make cli-snapshot` if the CLI surface changed.
dokimi context consistency: `--shell` is neither a canonical spelling in dokimi/dokimi/cli_flags.py nor on dokimi/tests/data/cli_flags_allowlist.txt. Use the registry spelling of its concept, or, with a reason, add `--shell  # <reason>` to dokimi/tests/data/cli_flags_allowlist.txt. Then run `make cli-snapshot` if the CLI surface changed.
AssertionError: dokimi/docs/reference/cli-interface.json is stale: run `make cli-snapshot` in dokimi/ and commit the result
assert {'path': 'dok...d': True, ...} == {'allow_abbre...d': True, ...}

The PR's checks ran before main moved to 0a63f4e, so they did not see it. Merging main into the branch reproduces it; once that is fixed I can merge.

@antonwolfy
antonwolfy dismissed their stale review October 29, 2026 11:58

Dismissing until the failure above is fixed: merged into the current main (0a63f4e) this PR fails 2 test(s). I'll review again after that.

@vchamarthi

vchamarthi commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

✅ OpenCodeReview: Review complete: 0 finding(s) across 8 selected item(s).

Review details
  • Commit reviewed: 6be19a2 against merge base f0f95a2
  • Coverage: 8 files in 2 groups
  • Engine: OpenCodeReview v1.13.3, us.anthropic.claude-sonnet-5-5
  • Usage: 745k tokens (406k from cache), 36s
  • Tool calls: 16 (9 code_search, 4 file_read, 3 file_find)
  • CLI flag registry implementation, reference snapshot, and tests: 4 files
  • CI, Makefile, docs and changelog for CLI flag registry: 4 files

…apshot

After main moved to 0a63f4e and then f0f95a2, the ratchet saw seven spellings
that are neither canonical nor allowlisted (--outcomes, --contract, --shell of
context consistency; --pass-k, --path-stability of repo compare; --threads,
--prompt-file of repo tasks replay), and the committed cli-interface.json was
stale. Add one allowlist row per spelling with its reason and run
`make cli-snapshot`. Both tests were red on the merged tree and are green.

Thread: IC_kwDOVBg7g88AAAABgzGjEw
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>


The 33rd docs page is removed (46bed29), so max_docs is not raised, and main is merged (1c60eb7, then again in this round).

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
@napetrov

Copy link
Copy Markdown
Contributor Author

Re your first review (the docs page over max_docs, and the registry_problems nit). The page is removed in 46bed29, its how-to is a section of dokimi/CONTRIBUTING.md, and max_docs is not raised. The nit is fixed in f309ca5 (yours), pinned by test_registry_problems_flags_each_inconsistency.

@napetrov

Copy link
Copy Markdown
Contributor Author

Re "Not approving yet; still open: the bug from my review". That bug is the docs page over max_docs, fixed in 46bed29 (page removed, cap unchanged); your nit commit f309ca5 is already in the branch.

@napetrov

Copy link
Copy Markdown
Contributor Author

Re "Needs fixes before merge". (a) max_docs: 33: not applied, because the 33rd page no longer exists (removed in 46bed29; the how-to lives in dokimi/CONTRIBUTING.md), so the spec is untouched. (b) Merge conflict: main is merged (1c60eb7), and merged again now at f0f95a2 (44f1959), clean. The CI rows you list as pre-existing are not touched here.

@napetrov

Copy link
Copy Markdown
Contributor Author

Thanks. Nothing in this review asks for a change. The snapshot and allowlist state after main moved again is handled in the answer to your later comment (IC_kwDOVBg7g88AAAABgzGjEw).

@napetrov

Copy link
Copy Markdown
Contributor Author

Codecov report (IC_kwDOVBg7g88AAAABf5EGHA): it reports no problem, so no change.

@napetrov

Copy link
Copy Markdown
Contributor Author

Reproduced after merging main (f0f95a2, newer than the 0a63f4e you named): both tests were red, with seven unregistered spellings (--outcomes, --contract, --shell, --pass-k, --path-stability, --threads, --prompt-file) and a stale snapshot. Fixed in c9c36e0: one allowlist row per spelling with its reason, and make cli-snapshot regenerated. tests/test_cli_flags.py and tests/test_import_architecture.py pass (37 passed).

@napetrov

Copy link
Copy Markdown
Contributor Author

OpenCodeReview summary (IC_kwDOVBg7g88AAAABg1-kkg): 0 findings, so no change.

@exai-bot

Copy link
Copy Markdown

✅ OpenCodeReview: Review complete: 0 finding(s) across 4 selected item(s).

Review details
  • Commit reviewed: 6be19a2 against merge base f0f95a2
  • Coverage: 4 files in 1 groups
  • Engine: OpenCodeReview v1.13.3, us.anthropic.claude-sonnet-5-5
  • Usage: 494k tokens (168k from cache), 29s
  • Tool calls: 3 (2 code_search, 1 file_find)
  • CLI interface reference generation and CI integration: 4 files

@antonwolfy antonwolfy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. The docs page that crossed max_docs is gone, the seven flags that main gained are allowlisted with a reason each, and the regenerated snapshot matches the tree.

What I ran, on a Linux review host at 6be19a2:

  • make ci-local: 28 passed, 1 failed, 7 skipped. Run tests passes (10,534 passed, 4 skipped), as do Coverage of changed lines, the CLI snapshot is current step, mkdocs build and the wheel smoke tests. pip-audit fails on filelock==3.32.7 in the lock files, which this PR does not touch.
  • A mutation run over cli_flags.py and the new test file: 21 mutants. The registry_problems checks, canonical renames in use and the walker are killed.
  • The snapshot dumper on Python 3.13: byte-identical to the committed cli-interface.json, and all seven new flags are in it.

Checked, no issues

  • Allowlist rows: --contract, --outcomes, --shell appear only on dokimi context consistency; --pass-k, --path-stability only on dokimi repo compare; --prompt-file, --threads only on dokimi repo tasks replay. All are used, and none is canonical or banned.
  • Makefile, CI and CONTRIBUTING: the cli-snapshot target, the 3.13 typecheck step and the "Adding or renaming a CLI flag" section agree.
  • Sign-off: all 12 non-merge commits carry a Signed-off-by.

@antonwolfy
antonwolfy merged commit 311fbe9 into intel:main Oct 30, 2026
17 checks passed
antonwolfy added a commit to antonwolfy/dokimi that referenced this pull request Oct 30, 2026
The merge of main brought intel#254's flag registry and its snapshot check:
docs/reference/cli-interface.json lacked the --task option this branch adds
to `repo compare`, so test_the_cli_interface_snapshot_is_current failed.
Regenerated with scripts/cli_surface.py dump (`make cli-snapshot`).

--task needs no allowlist row: it is already a canonical spelling of the
registry's `inputs` concept ("a filter on --tasks", text, repeatable),
which is what the compare option is (an append of task ids).

intel#298's check_comment_blocks.py --base origin/main --head HEAD finds no
comment block over two lines among the lines this branch adds or edits,
so no comment changes.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
antonwolfy added a commit to antonwolfy/dokimi that referenced this pull request Oct 30, 2026
The merge with main brings the flag registry of intel#254: its ratchet test
fails a spelling that is neither canonical in dokimi/cli_flags.py nor on
tests/data/cli_flags_allowlist.txt, and CI fails a stale
docs/reference/cli-interface.json. `--hits` switches repo report from the
table to the list of credited replay hits; no registry concept names a
view switch, so it gets an allowlist row with that reason, and the
snapshot is regenerated with what `make cli-snapshot` runs (the only
change is the --hits option).

intel#298's scripts/check_comment_blocks.py --base origin/main --head HEAD
finds no comment block over two lines among the lines this PR adds or
edits, so no comment changes.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
antonwolfy added a commit that referenced this pull request Oct 30, 2026
…gs inconclusive rows (#296)

* feat(repo): compare records start context, filters tasks, flags rows

An evaluation of AGENTS.md guidance files (26 review_replay tasks, two
arms, three reps) showed three gaps in reading a batch with repo compare.

Start context: a batch did not say what each cell started with, so
whether an arm's guidance was delivered had to be read by hand from
every agent.stdout. The Transcript now carries first_prompt_tokens, the
input + cache_creation + cache_read tokens of the first main-thread call,
read with dokimi.context.trace (one call per message id, subagent calls
apart). repo run writes it as result.json agent.first_prompt_tokens (null
until set, and for a trace without such a call), and repo compare
compares it as a descriptive measure next to cost_usd and turns. Like a
process count, it is compared only when both arms report it, so a batch
written before the field has no row rather than a row of zeros.

Task selection: repo compare --task ID (repeatable) compares only those
task ids. An id no finished cell has is refused through CompareError;
the tasks left out are not listed as unpaired nor their cells as
missing; the summary records the selection as task_filter and the text
header prints "tasks: N of M (--task)".

Reading notes, with no number changed: a gate or score row of 2+ tasks
whose interval includes 0 gets an "inconclusive" note with mde, the
smallest effect 80% power detects at that spread,
(1.959964 + 0.841621) * sd / sqrt(n) (mde is in every row of --json);
a row whose interval and sign-flip p disagree gets interval_p_disagree
and a note naming the two methods, saying for gate and score that p is
the one to read; descriptive rows with p < 0.05 are named on one line as
uncorrected and outside Holm's family.

Contract sections 4 and 9, docs/context-window.md and the compare
module docstring describe the field, the flag and the notes.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>

* test(repo): pin the one-arm skip both ways and the usage merge

Review comment 4415626666: the one-arm skip of first_prompt_tokens was
tested only with the baseline lacking it. The test now also compares a
batch whose treatment lacks it and asserts the same warning and no
unpaired entries, so a skip that checks only the baseline arm fails.

Review comment 4416413441: the main call's message id now has a third,
sparser usage line after the full one, so the first prompt is 19_380
only when the lines merge by maximum; "last line wins" gives 16_319
and fails the test, as "first line wins" already did.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>

* fix(repo): compare's mde uses the t quantiles of its interval

The inconclusive note gave mde with normal quantiles (1.96 + 0.84), but the
interval beside it is a t interval at n-1 degrees of freedom. At 4 tasks the
note said +-0.639 where the t quantiles give +-0.950; at 26 the gap is small.
The contract's formula is updated to match.

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>

* fix(repo): compare --task matches a missing cell by its task, not a prefix

A task id may contain '__', so a missing cell of fix__a starts with fix__ and
was counted as a cell of fix. The task is now split off the cell name from the
right, as the arm, model and rep are the last three parts.

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>

* fix(repo): compare's arm and metric errors say when --task is the reason

With --task the arm and metric checks run on the selected cells, so an arm
that ran only other tasks read as a wrong name. Those errors now add 'among the
--task selection' when a selection is set.

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>

* docs(context-window): say what first_prompt_tokens records

The sentence bolded an orphan 'first'. It now names the first prompt.

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>

* docs(repo): what the --task of count counts

of is the tasks with a finished cell, the only ids --task can name, not the
task count of the batches: a task whose cells all crashed before result.json
is not in it.

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>

* docs(compare): rewrap the module docstring after the mde change

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>

* refactor(repo): move compare's --task selection into compare_tasks.py

repos/compare.py was 787 lines, and the review answers that follow
(4418945747, 4420755924, 4418945753, 4420755898, 4418945763) take it past
the 800-line limit of the growth ratchet. The --task selection
(_select_tasks, _cell_task) moves unchanged into repos/compare_tasks.py,
where the missing-cell fix of 4420755898 adds its code. An unknown id now
raises TaskSelectionError there, which compare() re-raises as the same
CompareError with the same message. No behaviour changes.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>

* fix(repo): compare gives no mde where the sign-flip p cannot reach 0.05

Answers review comments 4418945747 and 4420755924. The row's p is an exact
sign-flip test, whose smallest p over k changed (non-tied) tasks is
2 / 2**k: at 5 or fewer changed tasks no effect of any size reaches
p < 0.05, yet the inconclusive note named an effect that 80% power would
detect (gate deltas [1, 1, 0]: p 0.5, mde 1.788; the four-task score row:
mde 0.950, p floor 0.125).

min_sign_flip_p gives that floor (1 / (N + 1) for the Monte Carlo test
above 16 changed tasks). Where it is 0.05 or more, mde is null in every
row, and the gate or score note says instead "with 2 changed tasks the
sign-flip p cannot go below 0.5: no effect can reach p < 0.05 here". From
6 changed tasks on, mde is computed as before. The module docstring, the
contract and the changelog fragment say so, and no longer claim the notes
change no number: they change no delta, interval or p.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>

* fix(repo): compare flags a gate or score row against its Holm p

Answers review comment 4418945753. interval_p_disagree was set in
paired_summary, before _holm ran, so a gate or score row compared its
interval with the raw p while p_holm is what decides it. With two
treatments and gate deltas six +1 and one tie, each row printed
+0.857 [+0.571, +1.000], p 0.031, p_holm 0.062 and no note.

_holm now sets interval_p_disagree on the gate and score rows from Holm's
verdict (correct_family, unrounded against 0.05), and their note quotes
p_holm and says to read p_holm. Descriptive rows keep the flag on their
raw p. The module docstring, the contract and the changelog fragment say
so.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>

* fix(repo): compare --task reads a missing cell's task from its meta.json

Answers review comment 4420755898 (and finishes 4418945759). 349d544 took
a missing cell's task from its name, split from the right, which assumed
the arm and model slugs carry no "__". They can: runner.cell_name keeps
the "__" of a model (cell_name("fix", "guidance:raw", "m__v1", 2) is
fix__guidance-raw__m__v1__r2, read as task fix__guidance-raw) and arms.slug
keeps that of an arm such as skill:lint__v2, so --task fix silently left
such an unfinished cell out of the missing list.

The task now comes from the cell's meta.json, which repo run writes with
the task id when the cell starts, read with the same guarded read as the
module's other cell files. A cell only plan.json names (no files) gets the
longest task id of its batch's finished cells that its name starts with
followed by "__". A cell neither gives a task for is missing only without
--task. The name-splitting _cell_task and its comment about slugs are
gone. The contract and the changelog fragment say how the task is found.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>

* fix(repo): compare names the arms --task leaves without a cell

Answers the second half of review comment 4418945763. Without --arm,
compare takes every other arm that has a finished cell among the
--task selection, so an arm that ran none of the selected tasks dropped
out of the comparison with no word.

task_filter now carries dropped_arms: without --arm, the arms of the
batches with no finished cell among the selection ([] with --arm, where
such an arm is refused). The text header prints them under the tasks
line ("not compared, no finished cell among the --task selection:
guidance:raw"). The default arm list is one helper, _other_arms, for
both. The module docstring, the contract and the changelog fragment say
so; the fragment also says what M of "tasks: N of M" counts (the tasks
with a finished cell), which e00edbc said in the contract only
(4418945768).

To keep repos/compare.py under the 800-line limit, CompareError moves
into compare_tasks.py, which now raises it for an unknown --task id
directly; compare.py imports it, so dokimi.repos.compare.CompareError
is unchanged for its callers.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>

* test(cli): regenerate the CLI snapshot for repo compare --task

The merge of main brought #254's flag registry and its snapshot check:
docs/reference/cli-interface.json lacked the --task option this branch adds
to `repo compare`, so test_the_cli_interface_snapshot_is_current failed.
Regenerated with scripts/cli_surface.py dump (`make cli-snapshot`).

--task needs no allowlist row: it is already a canonical spelling of the
registry's `inputs` concept ("a filter on --tasks", text, repeatable),
which is what the compare option is (an append of task ids).

#298's check_comment_blocks.py --base origin/main --head HEAD finds no
comment block over two lines among the lines this branch adds or edits,
so no comment changes.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>

* fix(repo): compare gives no mde where Holm keeps p_holm from 0.05

Review comment 4422703568: mde and the inconclusive note read the raw
sign-flip floor, but gate and score rows are decided by p_holm. Two score
treatments with six changed tasks each both got an mde and an "80% power
detects" note, though Holm over the two rows cannot take either p_holm
below 2 * 2/64 = 0.0625.

After Holm, each gate or score row gets p_holm_floor: its p_holm with its
own p put at its floor (min_sign_flip_p over its changed tasks) and the
family's other p as observed, from correct_family. Where that is not
below 0.05, mde is null and the inconclusive note says "with Holm over N
rows, p_holm cannot go below X here: no effect can reach p_holm < 0.05".
A family of one row is the raw rule, as before; descriptive rows keep it.

min_detectable, ALPHA and POWER move to the new repos/compare_reach.py
with the floor helper, holm_floor, so compare.py stays under 800 lines;
compare re-imports them. Tests pin the two-treatment case (both rows
null, the Holm note) and one where a row of eight changed tasks keeps
its mde beside a six-task row that loses it. The module docstring,
contract §9, architecture.md and the changelog fragment follow.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>

* docs(repo): name mde an uncorrected t-based estimate, not Holm power

Review comments 4422911586 and 4422911592: `mde` is computed as
(t(0.975, n-1) + t(0.80, n-1)) * sd / sqrt(n), the effect a one-sample
t test at the uncorrected 5% level detects with 80% power. It is not the
power of the sign-flip test nor of the Holm-corrected decision: with two
rows and the other row's p at 1, Holm needs this row's raw p below 0.025,
and the figure is still the one-row value. `p_holm_floor` only decides
whether `mde` is given.

The inconclusive note now reads "an uncorrected t-test estimate puts the
effect 80% power detects at this spread at about ±X (N tasks), before
Holm and not for the sign-flip test". The `compare.py` module docstring,
`compare_reach.min_detectable`, contract section 9, architecture.md and
the changelog fragment say the same; the contract says why no
Holm-calibrated figure is given (it would depend on the other rows'
observed p, which a planning figure cannot know). How `mde` is computed
and every JSON key and value are unchanged; the two tests that assert
the note text take the new wording.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>

---------

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Co-authored-by: Nikolay Petrov <nikolay.a.petrov@intel.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.

5 participants