Repository navigation
Conversation
- --per-skill reviews each skill alone, so the cross-catalog checks (duplicate, name-conflict, mirror-drift, description-near-duplicate) do not fire between unrelated skills. - A SKILL.md nested in another skill's directory is marked nested_in (JSON) and noted (text); --skip-nested leaves it out. - review_skill_dir(path) -> SkillReview is the public per-skill review. - With --per-skill: --jsonl PATH streams one row per skill, --jobs N reviews in parallel, --resume skips skills already in PATH. --jsonl/--jobs/--resume need --per-skill; misuse exits 2. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…lier run wrote Prior rows are counted only when their skill is in this run's selection, so --skip-nested hides a nested error row written before, and a skill removed from the catalog no longer counts. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
✅ OpenCodeReview: Review complete: 0 finding(s) across 12 selected item(s). CI at Review details
|
| from dokimi.external import nested_in_bundles, read_skill_manifest | ||
|
|
||
| try: | ||
| skills = read_skill_manifest(root) |
There was a problem hiding this comment.
Must-fix — Impact: JSONL mode still retains every skill body in memory before writing its first result, undermining the million-skill corpus use case. Trigger: --per-skill --jsonl on a large catalog (for example, 1M skills with 10 KB bodies requires roughly 10 GB for body text alone). Evidence: read_skill_manifest eagerly calls load_skill for each entry, SkillCatalogEntry retains Skill.body, and pairs/todo keep these entries alive throughout review; --resume also loads bodies for skills it will skip. Discover lightweight paths/manifest metadata first and load each selected skill inside the review worker. Workaround: partition the catalog into smaller scans. Severity: medium because memory exhaustion blocks large corpus scans, the explicit scale targeted by #413 A7.
| if not path.is_file(): | ||
| return [] | ||
| try: | ||
| lines = path.read_text(encoding="utf-8").splitlines() |
There was a problem hiding this comment.
Must-fix — Impact: --resume cannot recover earlier complete rows after an interrupted non-ASCII write. Trigger: a killed run leaves the last line ending partway through a multibyte UTF-8 character. Evidence: write() uses ensure_ascii=False, but read_text raises UnicodeDecodeError before the incomplete-line repair loop; the documented half-written-line recovery therefore does not run. Read and split bytes first, preserving validated complete lines and dropping the incomplete trailing line. Workaround: manually remove the incomplete trailing line. Severity: medium because this blocks the supported interruption-recovery feature for a bounded input case.
| if isinstance(row, dict): | ||
| rows.append(row) | ||
| kept.append(line) |
There was a problem hiding this comment.
Must-fix — Impact: malformed resume input crashes with a traceback rather than a usage error, and may be rewritten before validation. Trigger: run catalog scan --root ROOT --per-skill --jsonl out.jsonl --resume with out.jsonl containing {"name":"alpha"}. Evidence: any JSON object is accepted here; _row_key then indexes the missing relpath. Objects with a matching relpath but missing/invalid state or error similarly fail in _Tally.add. Validate the required row fields and state values before accepting or repairing the file, and report invalid complete records with usage_exit. Workaround: remove the invalid record or regenerate without --resume. Severity justification: medium because a bounded invalid-input case prevents the supported resume operation and bypasses its diagnostic handling.
| if isinstance(row, dict): | ||
| rows.append(row) | ||
| kept.append(line) | ||
| if len(kept) != len(lines): |
There was a problem hiding this comment.
Must-fix — Impact: resuming can merge two rows into an invalid JSONL line, losing both on the next resume. Trigger: PATH ends with a complete JSON object but no trailing newline, and another skill remains to review. Evidence: splitlines() accepts the final object, this condition skips repair, and append mode writes the next object directly after it. Normalize the final newline before appending and add a regression test. Workaround: manually add a newline before --resume. Severity: medium because this causes bounded loss of persisted scan results.
| return failed | ||
|
|
||
|
|
||
| def _check_scan_flags(args: argparse.Namespace) -> None: |
There was a problem hiding this comment.
Must-fix — Impact: a successful tasks-only scan silently ignores the requested JSONL output and resume/parallel options. Trigger: catalog scan --root TASK_CATALOG --kind tasks --per-skill --jsonl out.jsonl (also with --resume or --jobs 2). Evidence: this validation accepts the combination, but cmd_catalog_scan only calls _scan_skills for skills/all; _scan_tasks never creates the JSONL file or applies those options. Reject skill-only options with --kind tasks using a usage error. Workaround: use --kind skills or all when requesting corpus output. Severity justification: medium because an accepted command omits its explicitly requested artifact, a bounded supported-contract failure.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #414 +/- ##
========================================
Coverage 94.78% 94.78%
========================================
Files 353 356 +3
Lines 46527 46754 +227
Branches 7967 8003 +36
========================================
+ Hits 44100 44317 +217
- Misses 1540 1545 +5
- Partials 887 892 +5
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
5 open issues
_load_prior()reads the entire JSONL into memory (read_text().splitlines()), which scales… · New--jobsis documented/positioned as a per-skill-only flag, but this validation only rejects… · Newsink.close()isn’t protected by afinally. If an exception occurs during_review_rows()… · Newsink.close()isn’t protected by afinally. If an exception occurs during_review_rows()… · New--jobsordering “is not caught by any test”, but this test asserts that… · New
What changed in this PR
Adds “corpus mode” to dokimi catalog scan so large catalogs can be scanned skill-by-skill (optionally streaming to JSONL, parallelized, and resumable), while also exposing a public per-skill review API and nested-skill marking.
Changes:
- Add
--per-skillscan mode with--jsonl,--jobs,--resume, and--skip-nestedsupport and flag-contract validation. - Introduce
dokimi.external.review_skill_dir()andnested_in_bundles()to support per-skill review and nested-skill detection. - Add docs/changelog updates and new test coverage for per-skill behavior, nested marking, and JSONL/resume.
| File | Description |
|---|---|
| dokimi/dokimi/commands/catalog.py | Wires new per-skill scan mode into catalog scan, adds flag-contract checks, and refactors shared row printing/building. |
| dokimi/dokimi/commands/catalog_scan.py | Implements per-skill scan execution, JSONL streaming/resume, multiprocessing workers, and nested filtering/marking. |
| dokimi/dokimi/external/catalog_review.py | Adds public review_skill_dir() API and nested-in-bundles detection helpers. |
| dokimi/dokimi/external/__init__.py | Exports the new external APIs via __all__. |
| dokimi/tests/test_catalog_review_dir.py | Tests the public review_skill_dir() behavior and export surface. |
| dokimi/tests/test_catalog_per_skill.py | Verifies cross-catalog checks are suppressed in per-skill mode while per-skill checks still run. |
| dokimi/tests/test_catalog_nested.py | Tests nested skill marking, skipping behavior, and parity across scan modes. |
| dokimi/tests/test_catalog_jsonl.py | Tests JSONL streaming, ordering with jobs, resume behavior, and invalid flag combinations/exits. |
| dokimi/tests/data/cli_flags_allowlist.txt | Allowlists new CLI flags with rationale. |
| dokimi/docs/reference/cli-interface.json | Updates CLI reference snapshot with new flags. |
| dokimi/docs/external-content.md | Documents corpus/per-skill scan behavior, nested marking, and flag constraints. |
| dokimi/changelog.d/20261105_120000_napetrov_catalog-corpus-mode.md | Adds changelog entry describing the new corpus mode/features. |
Review effort: Lite
| try: | ||
| lines = path.read_text(encoding="utf-8").splitlines() | ||
| except (OSError, UnicodeDecodeError) as exc: | ||
| usage_exit(f"cannot read --jsonl {path} to resume: {exc}") | ||
| rows: list[dict] = [] | ||
| kept: list[str] = [] | ||
| for line in lines: | ||
| try: | ||
| row = json.loads(line) | ||
| except ValueError: | ||
| continue | ||
| if isinstance(row, dict): | ||
| rows.append(row) | ||
| kept.append(line) | ||
| if len(kept) != len(lines): | ||
| tmp = path.with_name(path.name + ".part") | ||
| tmp.write_text("".join(line + "\n" for line in kept), encoding="utf-8") | ||
| tmp.replace(path) |
| usage_exit("--jsonl writes one row per skill to a file; drop --format json") | ||
| if args.resume and not args.jsonl: | ||
| usage_exit("--resume needs --jsonl: the rows it skips are read from that file") | ||
| if (args.jsonl or args.jobs > 1) and not args.per_skill: |
| tally = _Tally(args.strict) | ||
| prior = sink.prior if sink is not None else [] | ||
| # Only the rows of the skills this run selects count: --skip-nested must hide a nested row an | ||
| # earlier run wrote, and a skill removed from the catalog is not part of this scan. | ||
| selected = {_entry_key(entry) for entry, _ in pairs} | ||
| prior = [row for row in prior if _row_key(row) in selected] | ||
| for row in prior: | ||
| tally.add(row) | ||
| done = {_row_key(row) for row in prior} | ||
| todo = [pair for pair in pairs if _entry_key(pair[0]) not in done] | ||
| for row in _review_rows(todo, args.jobs): | ||
| tally.add(row) | ||
| if sink is not None: | ||
| sink.write(row) | ||
| else: | ||
| report["skills"].append(row) | ||
| if sink is not None: | ||
| sink.close() | ||
| if not as_json: | ||
| _print_text(root, declared, tally, len(prior), jsonl, report["skills"]) | ||
| return tally.failed, declared |
| tally = _Tally(args.strict) | ||
| prior = sink.prior if sink is not None else [] | ||
| # Only the rows of the skills this run selects count: --skip-nested must hide a nested row an | ||
| # earlier run wrote, and a skill removed from the catalog is not part of this scan. | ||
| selected = {_entry_key(entry) for entry, _ in pairs} | ||
| prior = [row for row in prior if _row_key(row) in selected] | ||
| for row in prior: | ||
| tally.add(row) | ||
| done = {_row_key(row) for row in prior} | ||
| todo = [pair for pair in pairs if _entry_key(pair[0]) not in done] | ||
| for row in _review_rows(todo, args.jobs): | ||
| tally.add(row) | ||
| if sink is not None: | ||
| sink.write(row) | ||
| else: | ||
| report["skills"].append(row) | ||
| if sink is not None: | ||
| sink.close() | ||
| if not as_json: | ||
| _print_text(root, declared, tally, len(prior), jsonl, report["skills"]) | ||
| return tally.failed, declared |
| def test_more_jobs_write_the_same_rows_in_the_same_order(tmp_path, capsys): | ||
| _catalog(tmp_path) | ||
| one = tmp_path / "jobs1.jsonl" | ||
| many = tmp_path / "jobs3.jsonl" | ||
|
|
||
| _run(["--root", str(tmp_path), "--per-skill", "--jsonl", str(one), "--jobs", "1"], capsys) | ||
| _run(["--root", str(tmp_path), "--per-skill", "--jsonl", str(many), "--jobs", "3"], capsys) | ||
|
|
||
| assert _rows(many) == _rows(one) |
…le on error Python 3.14 makes forkserver the Linux default; its socket tripped the test socket guard, and the failure left the --jsonl file open. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| skill_dir = Path(path) | ||
| if not skill_dir.is_dir(): | ||
| raise ValueError(f"not a skill directory: {skill_dir}") | ||
| own = [e for e in _scan_skill_files(skill_dir) if e.path is not None and _same_dir(e.path, skill_dir)] |
There was a problem hiding this comment.
Must-fix — Impact: the single-directory API loads and retains every descendant skill body before discarding them, so reviewing one outer skill can consume memory proportional to an entire nested corpus rather than that skill. Trigger: call review_skill_dir(outer) with many large nested SKILL.md files (e.g. 100,000 descendants with 10 KB bodies retains roughly 1 GB of body text). Evidence: _scan_skill_files walks all descendant SKILL.md files, calls load_skill on each, and returns a fully materialized list; this comprehension filters only after that work completes. The nested test checks findings but does not catch descendant loading. Workaround: review a standalone copy without nested skills. Severity justification: medium because this causes a material, bounded performance/memory failure of the new public API. Load only skill_dir / 'SKILL.md' into a SkillCatalogEntry with source_root=skill_dir, preserving loader-error handling, and add a test proving nested SKILL.md files are not loaded.
Regenerate docs/reference/cli-interface.json with the corpus-mode flags on catalog check. Name catalog check (the intel#396 path) in the PR's changelog fragment, docstrings, flag allowlist comments and test argv. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 6f5ddcf. The per-skill review, the nested marking and the flag contract work as described. The problems are in the --resume path and at the edges of the flag checks: seven inline points, four of them bugs I reproduced. I would fix those four before merge.
What I ran, on a Linux review host at 6f5ddcf:
make ci-local: 29 passed, 1 failed, 7 skipped; 15,889 tests; changed-line coverage 95%. The failure is the test step: two tests hit the 60 s timeout and one slow-hook test took 1.9 s against a 1 s limit, while other jobs shared the machine. All four of those tests pass when run alone (174 passed), and none touches this PR's files. I do not count it against the PR.- Reproductions for the code findings, with the LLM transport mocked.
- A mutation run on the four new test files: 61 mutants, 27 pass the full suite. Most change text or counts; the ones that decide an exit code or a selection are the
--strictand path-less-key points above. - All three non-merge commits carry a
Signed-off-by.
Checked, no issues
- Flag contract:
--jsonlwith--format json,--resumewithout--jsonl, and--jsonlor--jobs 2without--per-skillexit 2. - Nested marking: a root
SKILL.mdencloses nothing, symlinked skill directories are not walked,--skip-nestedalso drops the skill from the declared count. - Jobs:
--jobs 2gives the same rows and exit code as--jobs 1under spawn, including a load-error skill. - Housekeeping: the five flags are in the CLI snapshot and the allowlist with reasons, the changelog fragment is in place, and
mkdocs buildpasses.
Bot points I checked that no longer apply at this head (5)
- Copilot's point on reading the whole JSONL into memory does not apply as a defect: the file holds one small row per skill, and the loaded skill bodies from the point above are the larger cost.
- Copilot's point on
--jobs 1without--per-skilldoes not apply: 1 is the default, so the parser cannot tell it from an absent flag, and the run is the same as without it. - Copilot's point on
sink.close()not being in afinallydoes not apply: the loop inscan_per_skillis wrapped intry/finallythat closes the sink (catalog_scan.py:202-211). - Copilot's second point on
sink.close()repeats the first and does not apply for the same reason: the close sits in thefinallyblock. - exai-bot's point on
review_skill_dirretaining nested skill bodies does not apply: it does load every descendantSKILL.mdto pick the first, which costs time, but nothing is kept after the call and a broken nested skill does not change the outer result.
| if isinstance(row, dict): | ||
| rows.append(row) | ||
| kept.append(line) | ||
| if len(kept) != len(lines): |
There was a problem hiding this comment.
I agree with exai-bot here; --resume glues the next row onto a last line that has no trailing newline.
_load_prior rewrites the file only when a line was dropped (len(kept) != len(lines)). A last line that parses but lacks \n is kept as it is, and path.open("a") at :157 appends right after it. Repro: three skills alpha, bravo, charlie; keep only the first row of the --jsonl file, without its newline; run again with --resume. The file is then:
- line 0:
{"name": "alpha", ... }{"name": "bravo", ...}(does not parse) - line 1: the
charlierow
The run prints 3 ok and exits 0. The next --resume drops line 0 as unparseable, so alpha and bravo are reviewed again, and any other consumer of the file reads a corrupt line.
Suggestion: after the loop, rewrite the file whenever it does not end in \n (or write "\n" before the first append in that case), and add a test with a complete last row and no newline.
| continue | ||
| if isinstance(row, dict): | ||
| rows.append(row) | ||
| kept.append(line) |
There was a problem hiding this comment.
I agree with exai-bot here; a complete JSON object that is not a row crashes --resume with a traceback.
_load_prior keeps any dict. With out.jsonl holding {"name":"alpha"}, --per-skill --jsonl out.jsonl --resume dies at _row_key (catalog_scan.py:86) with KeyError: 'relpath'. A row with "state": "weird" dies the same way at _Tally.add (:120, KeyError: 'weird'). A file from an older version or another tool is enough.
Suggestion: keep a row only if relpath and name are present and state is one of ok, warn, error. For a complete line that fails this check, usage_exit naming the file and the line number, rather than dropping it.
| if not path.is_file(): | ||
| return [] | ||
| try: | ||
| lines = path.read_text(encoding="utf-8").splitlines() |
There was a problem hiding this comment.
I agree with exai-bot here; a run killed inside a multibyte character cannot be resumed.
write() uses ensure_ascii=False (:162), so non-ASCII text from a description or a message reaches the file as UTF-8. A kill can cut a character in half. Repro: keep one complete row, append the byte 0xc3 (first byte of é), run --resume. Output, exit 2: cannot read --jsonl o.jsonl to resume: 'utf-8' codec can't decode byte 0xc3 in position 341: unexpected end of data. The half-written-line recovery in the docstring never runs.
Suggestion: read bytes, split on b"\n", decode each line on its own, and treat a line that fails to decode as the half-written one.
There was a problem hiding this comment.
One more failure from the same ensure_ascii=False: a directory name that is not valid UTF-8 crashes --jsonl with UnicodeEncodeError: surrogates not allowed, while --format json escapes it. Reading bytes line by line would not fix that case. json.dumps(row) with the default ensure_ascii fixes both: every line is ASCII, so a kill cannot cut a character in half.
| usage_exit("--jsonl writes one row per skill to a file; drop --format json") | ||
| if args.resume and not args.jsonl: | ||
| usage_exit("--resume needs --jsonl: the rows it skips are read from that file") | ||
| if (args.jsonl or args.jobs > 1) and not args.per_skill: |
There was a problem hiding this comment.
I agree with exai-bot here; --kind tasks accepts the per-skill flags and ignores them.
_check_scan_flags does not look at --kind, and _scan_skills is only called for skills and all. Repro: catalog check --root <root> --kind tasks --per-skill --jsonl d.jsonl prints # tasks (0 declared ...), exits 0 and creates no d.jsonl. A CI step that then reads d.jsonl finds no file, or treats a missing file as no findings.
Suggestion: if args.kind == "tasks" and (args.per_skill or args.jsonl or args.resume or args.skip_nested or args.jobs > 1): usage_exit("--per-skill, --skip-nested, --jsonl, --jobs and --resume apply to skills; drop them or use --kind skills|all").
| def _entry_key(entry) -> str: | ||
| from dokimi.external.catalog_review import entry_relpath | ||
|
|
||
| return entry_relpath(entry) or f"name:{entry.name}" |
There was a problem hiding this comment.
--resume can skip a skill that was never reviewed: path-less entries share the key name:?.
A skills.yaml entry without a name gets the name ?, so every such entry has the resume key name:? (_entry_key, _row_key). Repro: a skills.yaml with two nameless entries (- {} and - {foo: 1}) among valid ones; scan with --per-skill --jsonl, cut the file after the first ? row, run again with --resume. The second run prints 6 declared ... 3 error; 3 resumed, where a full run prints 4 error. The second ? entry is counted as done and its error never appears. Entries that point at the same directory collide on the relpath the same way.
Suggestion: key path-less entries by their position in the manifest (for example entry:{index}), and key every row the same way so resume matches by position.
|
|
||
| def add(self, row: dict) -> None: | ||
| self.counts[row["state"]] += 1 | ||
| if row["error"] is not None or (self.strict and row["state"] == "error"): |
There was a problem hiding this comment.
No test pins --strict on the per-skill path.
This line decides the exit code of a corpus scan: a spec error fails only under --strict. Mutation check: dropping the self.strict and clause, and making every error fail regardless of --strict, both pass the four new test files. The only --strict tests are the older ones on the non-per-skill path. A regression here makes --per-skill --strict exit 0 on spec errors, or exit 1 without --strict.
Suggestion: a test with one skill that has a spec error and no load error, run --per-skill --jsonl with and without --strict, expecting exit 0 and 1.
| from dokimi.external import nested_in_bundles, read_skill_manifest | ||
|
|
||
| try: | ||
| skills = read_skill_manifest(root) |
There was a problem hiding this comment.
I agree with exai-bot here; the module promises a million-skill corpus, but every skill body is loaded before the first row is written. This is from reading the code, not a run.
read_skill_manifest (external/catalog.py:113, :231) calls load_skill for every entry and keeps the result in SkillCatalogEntry.skill. pairs, todo and the worker arguments then hold all of them for the whole scan. --resume loads the skills it will skip, too. --jsonl bounds the output, not the memory. The docstring at :7 says a corpus of a million skills can be scanned one row at a time.
Suggestion: either list the skill paths first and load each skill inside _review_row, or drop the million-skill wording from the docstring and docs/external-content.md and say the whole catalog is held in memory.
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at 6f5ddcf. The per-skill review and the nested marking work, and the default scan gives the same rows as before plus relpath and nested_in. The --resume path is not ready yet: the file can disagree with the counts, and some files it should recover crash or are refused. Details are in the line comments.
Agreed with @exai-bot: the corpus is still loaded whole before the first row; review_skill_dir loads every nested skill to review one; a foreign row crashes --resume with KeyError; a last row with no newline gets glued to the next one; --kind tasks ignores --jsonl.
Agreed with @antonwolfy: path-less entries share the resume key name:?; no test pins --strict on the per-skill path. On the cut UTF-8 line I added a second case and a simpler fix in his thread.
Agreed with @Copilot that _load_prior reads the whole file, a minor cost next to the skill bodies. The sink.close() threads are fixed by the try/finally in 05a8b29, and --jobs 1 without --per-skill is the default, so it changes nothing.
What I ran, at 6f5ddcf:
make ci-local --offline: 0 failed, 19 passed, 13 skipped (network and Docker jobs).- CLI repros on scratch catalogs:
--resume --skip-nestedleaves the nested row in the file while printing1 declared;{"name":"alpha"}in the file givesKeyError: 'relpath'; a last line cut inside a UTF-8 character exits 2; a last row with no newline gets the next row glued to it;--kind tasks --per-skill --jsonlwrites no file; a non-UTF-8 directory name crashes--jsonlwithUnicodeEncodeError; askills.yamlpath through a symlinkedskills/givesnested_in: real_skills/afor a parent whoserelpathisskills/a.
Checked, no issues
- Flag contract:
--jsonlwith--format json,--resumewithout--jsonland--jsonlwithout--per-skillall exit 2, and the tests cover each one. - Per-skill review: each skill goes through
review_skills([entry])alone, so the four cross-catalog checks cannot fire, and--jobskeeps the catalog order throughimap. The spawn context runs_review_rowat module level. - Symlinks: a
SKILL.mdsymlinked out of the root does not crashnested_in_bundles, because the loader does not resolveskill.path. - Docs and changelog: the fragment is under Added and names every flag and the new API. The new files have the SPDX header, and the flags are in the allowlist and the CLI snapshot.
| # Only the rows of the skills this run selects count: --skip-nested must hide a nested row an | ||
| # earlier run wrote, and a skill removed from the catalog is not part of this scan. | ||
| selected = {_entry_key(entry) for entry, _ in pairs} | ||
| prior = [row for row in prior if _row_key(row) in selected] | ||
| for row in prior: | ||
| tally.add(row) | ||
| done = {_row_key(row) for row in prior} | ||
| todo = [pair for pair in pairs if _entry_key(pair[0]) not in done] |
There was a problem hiding this comment.
--resume leaves the rows this run does not select in the --jsonl file, so the file no longer matches the printed counts.
A first run writes skills/a and skills/a/sub. Then --per-skill --skip-nested --jsonl s.jsonl --resume prints 1 declared ... 1 resumed and exits 0, but s.jsonl still holds both rows, the nested one included. A skill removed from the catalog keeps its row too. Whoever reads the file gets rows the scan says it left out. test_resume_with_skip_nested_... checks only stdout.
Suggestion: when prior loses rows here, rewrite the file with the kept ones (as _load_prior already does through .part), and assert in the test that the file has one row.
| The cross-catalog checks (``duplicate``, ``name-conflict``, ``mirror-drift``, | ||
| ``description-near-duplicate``) need every skill loaded at once. This mode gives them up, | ||
| so a corpus of a million skills can be scanned one row at a time: rows are written in | ||
| catalog order, a run killed midway keeps the rows it wrote, and ``--resume`` reviews only |
There was a problem hiding this comment.
Nit: The docstring says rows are written in catalog order, but after --resume the new rows come after the old ones.
test_resume_reviews_only_the_skills_without_a_row itself expects alpha, charlie, delta, bravo.
Suggestion: say "in catalog order within one run; --resume appends".
| """ | ||
| root_r = Path(root).resolve() | ||
| bundles: dict[Path, str] = {} | ||
| for entry in entries: |
There was a problem hiding this comment.
Nit: nested_in is built from resolved paths, but a row's relpath is not resolved, so the two can differ.
With catalog_path: skills/a where skills is a symlink to real_skills, the outer row has relpath: skills/a and the inner row nested_in: real_skills/a, which matches no row.
Suggestion: build bundles from entry_relpath(entry) so both use the same form.
| except ValueError as exc: | ||
| usage_exit(f"skills.yaml: {exc}") | ||
| pairs = catalog_scan.read_marked_skills(args, root) | ||
| if args.per_skill or args.jsonl: |
There was a problem hiding this comment.
Nit: args.per_skill or args.jsonl can be just args.per_skill: _check_scan_flags already exits on --jsonl without --per-skill.
The default branch below also repeats the counting, the exit-1 rule and the header that _Tally and _print_text now do; using them here keeps the two modes from drifting apart.
Summary
Corpus mode for
dokimi catalog scan, for large skill catalogs (#413, items A3, A4, A6, A7):--per-skillreviews each skill alone, so the cross-catalog checks (duplicate,name-conflict,mirror-drift,description-near-duplicate) do not fire between unrelated skills.SKILL.mdnested in another skill's directory is markednested_in(JSON) and noted (text);--skip-nestedleaves it out.dokimi.external.review_skill_dir(path) -> SkillReviewis the public per-skill review.--per-skill:--jsonl PATHstreams one row per skill,--jobs Nreviews in parallel,--resumeskips skills already in PATH.--jsonl,--jobs,--resumeneed--per-skill; misuse exits 2.Docs:
docs/external-content.mdsection 1; changelog fragment added.--jobswas already on the flag allowlist; the new spellings have allowlist rows with a reason.Evidence
make ci-local --offlineon the head: 0 failed, 19 passed, 13 skipped.test_catalog_per_skill,test_catalog_review_dir,test_catalog_nested,test_catalog_jsonl), plus the import-cycle and flag-contract gates.--jobsordering is not caught by any test (known gap).pr-review, local mode): one blocking finding (--resume --skip-nestedcounted a nested row from an earlier run), fixed in2795d517with a regression test.Overlap:
commands/catalog.pyis also moved by #396; the change there is small.Part of #413
🤖 Generated with Claude Code