Repository navigation
Conversation
The top-level route list now comes from dokimi/cli_routes.py: each root sits in one of four sections (The loop, Subjects, Evals, Analysis and data), and every route's help line ends in [free] or [paid]. A route is paid when any model call is possible in some flag combination; a root is paid if any leaf is. `dokimi data schema list|show KIND` prints a schema as JSON; cli_routes is the first kind. The table also carries tombstones (an old argv prefix that moved prints "moved: dokimi ..." and exits 2, never runs). The table is empty for now, and the test covers the mechanism with a test-only entry. Handler-neutral: no command moves or renames. Route help text stays with each parser, except run and compare, which get a one-line help from the table. Refs intel#94 (A1), intel#276 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
moved_argv matches by argv prefix, but tombstone_problems compared paths to the leaf list only. A tombstone on a group (repo tasks) therefore passed the check and shadowed live routes under it, and a root or alias destination (library) was wrongly reported as not a route. A path now counts as a route when it names a root or alias, or a group or leaf under one. Self-review of the A1 commit (blocking). Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…bels A tombstone whose old path extends a live leaf (compare x, where compare takes files) shadows that leaf's invocations, so tombstone_problems now reports it. The tombstone check resolves aliases through one helper. A route with neither parser help nor a table summary gets its label without a leading space. Self-review round 2 (no blocking; these two nits and the test count fixed). Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…path moved_argv compared raw argv, so `product old` skipped a tombstone on `library old`. It now compares canonical paths (an alias is replaced by its root's name on both sides). An empty old path matched every argv, and an option (--version) is not a command; tombstone_problems reports both. Self-review round 3 (2 blocking, both fixed; the alias-ownership check is new). Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #284 +/- ##
========================================
Coverage 93.48% 93.49%
========================================
Files 272 274 +2
Lines 35363 35512 +149
Branches 6001 6025 +24
========================================
+ Hits 33060 33201 +141
- Misses 1489 1494 +5
- Partials 814 817 +3
🚀 New features to boost your workflow:
|
github-actions
Bot
added
the
cli-interface-change
CLI interface change detectedThis PR changes dokimi's user-facing CLI surface (commands and/or options): + added command `dokimi data`
+ option `--help/-h`
+ added command `dokimi data schema`
+ option `--help/-h`
+ added command `dokimi data schema list`
+ option `--help/-h`
+ added command `dokimi data schema show`
+ argument `kind`
+ option `--help/-h`Updated on every push; the |
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at 08ac435. The route table, the sectioned help and the tombstone check do what #94 A1 asks, and the cost labels match what the commands do. What is left is small and is in the line comments.
What I ran, at 08ac435:
make ci-local --offline: 9,681 passed, 0 failed, 57 skipped; every static step passed (the network-only ones were skipped).dokimi --help,dokimi repo --help,dokimi novelty --helpanddokimi data schema show cli_routes: four sections, every root and leaf ends in[free]or[paid],library (product)keeps its alias.- Each
freeleaf's handler, looked up from the parser: none of them makes a model call. The ones that take--model/--provider/--harness(benchmark preflight,repo budget,repo compare) use them only to check keys, pick a profile or filter rows.
Checked, no issues
- Unknown routes:
cost_ofreturns None for a route the table does not know, so a missing entry shows no label rather than[free], andtest_route_table_matches_the_parser_leaves_and_rootsfails on it. - Tombstones: the check runs before argparse, rewrites an alias argv to the new path, and
tombstone_problemsrejects an empty or option old path, a live route, a path under a live leaf and a target that is not a route. The table is empty, so no user-visible change yet. - Changelog: the fragment says what changed in
--helpand thatdata schemahas one kind for now.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 08ac435. The route table, the sectioned help and the tombstone check do what #94 A1 asks, and the cost labels hold up against the handlers. One thing is worth fixing before merge: the tests do not pin the costs the PR promises.
I agree with @vlad-perevezentsev's points on the missing cli_routes.v1 schema file and on the data help text and the routed flag.
What I ran, on a Linux review host at 08ac435:
make ci-local: 26 passed, 2 failed. Full suite 9,730 passed; changed-line coverage step passed.pip-auditfails onfilelockadvisories in the lock files, which this PR does not touch.Run testsfailed ontest_unbalanced_quote_in_hook_completes_quickly(took 1.44 s, limit 1 s),test_a_cell_finished_with_another_execution_is_not_reused_...and two 60 s timeouts intest_audit_test_quality.py. None touches the CLI. All 764 tests in those three files pass when re-run alone at the same commit.- Mutation run on
test_cli_routes.py: 51 mutants, 24 of which pass the file. The ones that matter are in the inline comment.
Checked, no issues
- Cost labels: I read the handler of every
freeleaf (benchmark preflight,repo tasks check,repo budget,eval grounding,ingest docling,novelty items|view, ...). None reaches an LLM, embeddings or harness call. Thepaidleaves with a no-model flag (--dry-run,--lexical-only) do call a model by default. - Table against parser:
LEAVESandROOTSmatch the parser tree exactly, includingrepo tasks check,data schema list|showand theproductalias. - Help output: a recursive help dump against the base differs only by the sections, the labels and the
datasubtree. Repeatedbuild_parser()calls do not label twice. - Commits: all four are signed off.
|
🔍 OpenCodeReview found 1 issue(s) in this PR. By severity: 1 medium
Review details
|
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
After merging main, `repo tasks replay` and `context consistency` are parser leaves the table did not list, so test_route_table_matches_the_parser_leaves_and_roots and the cost-label test failed on the merged tree. Both are free: replay reads a recorded threads file and writes tasks (no LLM, no GitHub), consistency reads local trace files. Sync with main for PR 284; not a review item. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
dokimi data schema show cli_routes printed a document stamped cli_routes.v1 that no schema defined, so schema_compat could not catch a renamed leaves[].path or a dropped roots[].cost. Add schemas/cli_routes.v1.json, its fixture, register the kind, and test that document() validates and that a drifted shape is rejected. Thread: PRRT_kwDOVBg7g86xg7_0 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
`dokimi data --help` promised "Schemas of the artifacts and the CLI routes" and `data schema` "an artifact or route schema", but cli_routes is the only kind: `data schema show eval` is an invalid choice. Say what is there; widen the text when artifact kinds are added (the test pins the pairing). Thread: PRRT_kwDOVBg7g86xg7_2 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…ntParser add_subparsers builds its children with type(self), so every `dokimi <cmd>` parser was a _RoutedParser that relied on the class default routed = False. Pass parser_class=argparse.ArgumentParser instead and remove the flag; only the top-level parser lists its commands by section. Thread: PRRT_kwDOVBg7g86xg7_4 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…rewrite to literals The cost-label, section and `data schema show` tests compared the output to cli_routes itself, so a wrong `[free]` passed. Mutations, each applied alone to cli_routes.py, all survived tests/test_cli_routes.py: evaluate and tools preference flipped to free, repo tasks check to paid, document() reporting every root free, no sections or no aliases, and moved_argv slicing by len(tomb.new). Now: the paid leaf paths are a literal set (the help labels are checked against it too), one root's section/aliases/cost and the sections are asserted from literals, and a tombstone whose old and new paths differ in length (old starting with `product`) is rewritten. All seven mutants now fail the file. Thread: PRRT_kwDOVBg7g86xh56h Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
… lines The cost-label test guarded with `checked >= len(LEAVES)`, but the walk also counts the root and group entries, which make up for a missing leaf; and a leaf parser added without help= is absent from _choices_actions, so it was skipped. Removing help= from `baseline save` left the file green (and `save` vanished from `dokimi baseline --help`). Compare the set of checked paths with the leaf paths instead; `run` and `compare`, whose help comes from the table's summary, are the exempt ones. Thread: PRRT_kwDOVBg7g86xlZDa Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Codecov listed line 190 of cli_routes.py (the empty-argv return in _canonical) as the patch lines without coverage. An empty argv matches no tombstone and argparse still asks for a command. Comment: IC_kwDOVBg7g88AAAABgsGHDA Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…r line comments Review bodies and the OpenCodeReview summary carry no claim of their own; their points are the line-comment threads fixed in the commits above (schema, data help, routed flag, test pins, help-less leaf), plus the Codecov line. Review: PRR_kwDOVBg7g88AAAABU4jUUQ Review: PRR_kwDOVBg7g88AAAABU5Daiw Review: PRR_kwDOVBg7g88AAAABU66xpw Comment: IC_kwDOVBg7g88AAAABg1oaiw Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
The three points you share with the first review are fixed in 55601ae, 1f26a76 and f97bb3e; the mutation findings in 49df9a8 (thread answered there). The CI failures you list (timing test, cell reuse, two audit timeouts, pip-audit |
|
Its one inline finding (the guard that counts instead of naming leaves) is fixed in fe1c6a7; answered in that thread. |
|
Line 190 of |
|
Informational: the diff lists exactly the new |
|
The one finding it posted inline is thread PRRT_kwDOVBg7g86xlZDa, fixed in fe1c6a7. |
| def test_run_and_compare_have_a_one_line_help_in_the_top_level_list(): | ||
| text = build_parser().format_help() |
There was a problem hiding this comment.
This test depends on the host terminal width. argparse sizes help from COLUMNS or the terminal, and falls back to 80. At 80 columns the help column is about 54 wide. "Run the static benchmark spec and save a snapshot [free]" is 56 characters, so [free] wraps onto a continuation line and the $-anchored regex fails. In a wide terminal the same test passes. A correct label can fail on a narrow host, and a label that is only on a wrapped line cannot be told apart from a missing one. Pin the width, for example monkeypatch.setenv("COLUMNS", "200") before format_help(). Do the same in test_help_lists_each_root_under_its_section, which also reads format_help() output.
Suggestion:
| def test_run_and_compare_have_a_one_line_help_in_the_top_level_list(): | |
| text = build_parser().format_help() | |
| def test_run_and_compare_have_a_one_line_help_in_the_top_level_list(monkeypatch): | |
| monkeypatch.setenv("COLUMNS", "200") # keep argparse from wrapping the cost label | |
| text = build_parser().format_help() |
There was a problem hiding this comment.
Confirmed, with one correction: the test passes at 80 columns and fails from 77 down.
At eb5e612, COLUMNS=79 and COLUMNS=80 pytest tests/test_cli_routes.py pass; COLUMNS=76, 74, 70 and 60 fail test_run_and_compare_have_a_one_line_help_in_the_top_level_list, because [free] wraps onto the next line of run. argparse reads the width from COLUMNS or the real terminal, so CI is fine and a narrow local terminal is not. test_help_lists_each_root_under_its_section still passes at 60 columns, so that one does not need the pin.
Suggestion: monkeypatch.setenv("COLUMNS", "200") in this test, as proposed above.
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at eb5e612. This is a re-check, and all three points from the first round are fixed. cli_routes.v1 now has a JSON Schema and a fixture, and its kind is registered in SCHEMA_VERSIONS. The schemas help no longer promises artifact schemas. The routed flag is gone. The PR looks good to merge once the open line comment is settled.
What I ran, at eb5e612:
make ci-local --offline: 17 passed, 0 failed, 13 skipped; pytest 10,497 passed, 57 skipped;schema_compatgreen. I did not run the smoke CI, because the PR does not touch harnesses, process or repos.- I ran
tests/test_cli_routes.pywithCOLUMNSset to 80, 79, 76, 74, 70 and 60 to find where the help output starts to wrap.
Checked, no issues
- Route table schema: the new tests validate
document()againstschemas/cli_routes.v1.jsonand reject a changed document shape. - Help text: the
schemashelp now says it covers the CLI route table today, and a test pins that text. - Parser:
parser_class=argparse.ArgumentParserreplaces theroutedflag. Only the top-level help is printed by section, andrepo --helplooks as before. - Price labels: the paid leaves are now an explicit list in the test, not taken from the table under test. The tombstone case uses
oldandnewof different lengths. The leaf test compares the set of paths instead of counting help lines. - Routes from main:
repo tasks replayandcontext consistencyare correctly marked[free]. The first reads the recorded threads file with no LLM or GitHub call. The second reads stream-json traces. SCHEMA_VERSIONSreaders:artifactsanddatasets_exportlook entries up by kind, so the addedcli_routeskind does not affect them.- Other PRs: #255 conflicts in
dokimi/dokimi/cli.py. #281 and #279 conflict indokimi/dokimi/artifacts.py, where each PR adds a line at the end ofSCHEMA_VERSIONS, so the conflict is trivial. #285 and #254 sharetests/test_import_architecture.pyand merge cleanly.
Summary
The top-level route list now comes from a table (
dokimi/cli_routes.py) instead of beingspread across the parsers. Each root sits in one section, and every route's help line ends in
[free]or[paid].paidmeans the route can make a model call in some flag combination.A root is
paidwhen any leaf under it is. This is item A1 of the CLI API v2 plan (#94 §5).sits in the section of the operation it will hold once the CLI API: the evaluation loop at the top level, subject and eval groups below, one enforced flag vocabulary (proposal v2.1) #94 §4 renames land.
moved: dokimi <new command>and exits 2 without running anything. No command has moved yet, so the table is empty. The test
covers the mechanism with a test-only entry. The check treats a group, alias, or root as a live
route, so a tombstone cannot shadow the commands under it.
datais a new root withschema listandschema show KIND. M2 adds export, import and models.runandcompare, which have no parser help, takea one-line help from the table.
dokimi/changelog.d/20261028_120000_napetrov_cli-routes-table.md.Evidence
Before (tests written against the route table, behaviour not yet implemented):
After:
Self-review fixes
cf5166bc,a654a8b9and08ac435f: each adds tests that fail beforethe fix and pass after it (the alias case in
08ac435f, for example, fails ona654a8b9);the full suite was re-run after each fix.
Help diff
User-visible: yes. The top-level
dokimi --helpis regrouped into four sections, every routeline ends in
[free]or[paid],runandcompareget a help line, anddatais new. The samelabels also show in each parent's own subcommand list (for example
dokimi baseline --help). Acommand's own
--helptext does not change.Merge Danger
Door: two-way. The help layout and labels revert with one commit. The JSON printed by
data schema show cli_routes(schema_version: cli_routes.v1) becomes a public shape once released,so a later change to it should bump the version.
Blast radius: every
dokimi --helpand every parent's subcommand list. No handler, flag, or exitcode changes; the tombstone path has no entries.
Overlaps:
docs/reference/cli-interface.json) fails on the newdatagroup. WhicheverPR lands second regenerates the snapshot. Its
cli_flags.pyneeds a layer entry in the same tuple intests/test_import_architecture.pythat this PR edits, so expect a conflict there.dokimi/cli.pyand the commands.Open questions
claude-cliprovider (feat(llm): claude-cli provider: run the Q&A track on a logged-in Claude Code CLI #255) paid? The ruleused here is "paid if any model call is possible on some path".
runandcomparebelong in Subjects? Done here, per CLI API: the evaluation loop at the top level, subject and eval groups below, one enforced flag vocabulary (proposal v2.1) #94 §4.data schemascope: onlycli_routesnow. Do artifact schemas underdokimi/schemas/come inF2 or M2?
--jsonerror code: deferred to F2.Refs #94 (A1), #276.
🤖 Generated with Claude Code