[vchamarthi]

Summary

tools preference, catalog trigger-set and catalog scan read a groups file and skill manifests and write --out and --items-out without handling OS or decode errors, so a bad file or path ends in a traceback.

Changes

Each affected read or write now reports the path and reason in a one-line message and exits. One commit per command.

Risk

Error paths only.

Testing

make ci-local (offline), Python 3.13: everything passes except mypy (1 error) and the 10 tests that also fail on main (process, sandbox, container probes; macOS). Load-flaky tests pass alone.

Checklist

• Tests added/updated and pytest passes (apart from the 10 pre-existing failures)
• ruff, mypy, and schema checks pass (mypy: the one pre-existing error)
• Docs updated (or N/A): N/A
• No ad-hoc run artifacts committed
• New files carry the SPDX license header (or are exempt in .github/.licenserc.yaml)

[Copilot]

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

3 open issues

• Catching broad ValueError here can accidentally swallow unrelated ValueErrors raised by… · New
• The match=rf\"... {out} ...\" uses a regex containing the filesystem path. On Windows,… · New
• The docstring ends with an incomplete phrase ('when it cannot be.'). Clarify it to something like… · New

What changed in this PR

Improves CLI error handling for tools preference, catalog trigger-set, and catalog scan by turning file read/write OS + decode failures into clean one-line messages with a non-zero exit, and adds regression tests for these cases.

Changes:

• Add guarded read/write paths for groups files, manifests, and --out / --items-out outputs with clear error messages.
• Refactor catalog trigger-set output writing into a helper that handles unwritable destinations.
• Add/extend tests covering non-UTF-8 inputs and unwritable output paths; update complexity baseline.

File	Description
dokimi/​tests/​test_tools_preference_items_out.py	New tests for tools preference --items-out unwritable path behavior.
dokimi/​tests/​test_tool_preference.py	Adds coverage for non-UTF-8 --groups input in tools preference.
dokimi/​tests/​test_skill_selection.py	Adds coverage for catalog trigger-set unwritable --out.
dokimi/​tests/​test_catalog_command.py	Adds coverage for non-UTF-8/unreadable manifests in catalog scan.
dokimi/​scripts/​growth_baseline.json	Updates complexity budget for the refactor in cmd_catalog_trigger_set.
dokimi/​dokimi/​external/​catalog.py	Converts manifest read/decode errors into user-friendly ValueErrors.
dokimi/​dokimi/​commands/​tools.py	Adds _write_items and improves --groups error reporting.
dokimi/​dokimi/​commands/​catalog.py	Adds _write_trigger_set with clean error reporting on write failures.
dokimi/​changelog.d/​20261025_120000_vchamarthi_cli-input-tools-catalog.md	Changelog entry describing the improved CLI error behavior.

_{Review effort: Lite}

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 97.56098% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.78%. Comparing base (b226dba) to head (f07639e).

Files with missing lines	Patch %	Lines
dokimi/dokimi/commands/tools.py	92.10%	3 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #239      +/-   ##
==========================================
+ Coverage   92.76%   92.78%   +0.02%     
==========================================
  Files         256      256              
  Lines       32414    32498      +84     
  Branches     5421     5426       +5     
==========================================
+ Hits        30068    30153      +85     
+ Misses       1561     1559       -2     
- Partials      785      786       +1     

Files with missing lines	Coverage Δ
dokimi/dokimi/commands/catalog.py	97.81% <100.00%> (+1.08%)	⬆️
dokimi/dokimi/external/catalog.py	97.68% <100.00%> (+0.50%)	⬆️
dokimi/dokimi/external/validate.py	93.93% <100.00%> (+2.02%)	⬆️
dokimi/dokimi/tool_preference.py	99.05% <100.00%> (+0.01%)	⬆️
dokimi/dokimi/commands/tools.py	97.33% <92.10%> (-0.09%)	⬇️

... 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.

[antonwolfy]

Review at 192057b. The fix does what it says for the cases it names: non-UTF-8 --groups, unwritable --items-out and --out, unreadable or non-UTF-8 manifests all exit with a one-line message. The problems are on the paths right next to them, which still end in a traceback, and in one test that does not pin the widened except. I'd fix these before merge.

What I ran, on a Linux review host at 192057b:

• make ci-local: 29 passed, 0 failed, 7 skipped (docker, network and other-Python legs). The changed-line coverage check passed.
• Reproductions for the code findings, offline.
• Mutation runs: with external/catalog.py or commands/catalog.py reverted to the base, the new catalog tests fail (4 and 3 tests). Seven of eight mutations of the tools code and tests are caught; the survivor is the first inline test comment on test_tool_preference.py.

Not anchored inline:

1. tools preference --out-json is still unguarded, and it is written after all the model calls. tools.py:139 calls save_artifact("tool_preference", data, args.out_json). With --out-json blocker/o.json, where blocker is a regular file, the command ends in a FileExistsError traceback after the paid calls have run. --items-out is now checked before any spend, so this is the remaining unguarded output. This predates the PR, but the PR title and changelog say tools preference exits with a message on an unwritable output. Suggestion: guard it the same way as _write_items, or check the parent of --out-json before the calls.

Checked, no issues

• except ValueError in tools.py: Copilot's point on the broad ValueError does not apply: tp.GroupsError is a ValueError subclass, and I found no input that makes load_groups raise another ValueError (wrong types for ids, names and vendors all become GroupsError). The only other ValueError in the module is raised from build_items, outside the try.
• Callers of the manifest readers: read_skill_manifest and read_task_manifest are called only from the two scan helpers, and both catch ValueError.
• --out failures: a directory, a file as parent, a symlink loop and a dangling symlink all give the one-line message for reach and skill-creator, and for questions apart from the empty path below.
• Repo rules: the changelog fragment passes the fragment check, and the growth baseline entry (16 to 15) is correct per check_growth.py. No schema or docs pages are touched.

[antonwolfy]

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

• 766eae4: dokimi/dokimi/commands/catalog.py:140: the docstring now ends "when it cannot be written."
• 8940627: dokimi/tests/test_skill_selection.py:485: the match= pattern wraps the path in re.escape(str(out)); import re was already there.

Left as is:

• dokimi/dokimi/commands/tools.py:139: the unguarded --out-json is not labelled Nit: or Non-blocking:, and guarding it would change behaviour, so it stays with the author.

make ci-local at 8940627: 29 passed, 0 failed, 7 skipped.

Not approving yet. Still open: the five risks from my review.

[vchamarthi]

@exai-bot /review

[exai-bot]

@vchamarthi I did not start this: 4 requests are already waiting for a worker. Ask again in a few minutes.

[vchamarthi]

@exai-bot /review

[exai-bot]

Note

AI-generated review of 0f9bfd9e68a6, requested by @vchamarthi, using the default review skill. It can be wrong: verify a finding before acting on it. React with a thumbs up or down on a finding to tell the bot whether it helped.
Model us.anthropic.claude-sonnet-5-5, effort default, 7 of 93 turns, 0.4 of 48 minutes, 141k in / 2k out tokens.

The change wraps the reads and writes of tools preference, catalog trigger-set and catalog scan so OS and decode errors exit with a one-line message. It also validates the shape of suites.json and of the groups document. The changes look correct and the tests cover the new paths. I found one minor problem with the message wording. I could not run the tests here because pytest isn't installed in the container.

1 finding is on the changed lines, as comments below.

Not verified

I did not run the new tests. I also did not check that make ci-local, including the growth baseline drop from 16 to 15 for cmd_catalog_trigger_set, is green.

[exai-bot]

Note

AI-generated review of 5d1c627fe3d8, requested by @vchamarthi, using the default review skill. It can be wrong: verify a finding before acting on it. React with a thumbs up or down on a finding to tell the bot whether it helped.
Model us.anthropic.claude-sonnet-5-5, effort default, 7 of 93 turns, 0.5 of 48 minutes, 121k in / 3k out tokens.

Re-review of 5d1c627fe3d8: 1 new finding.

1 finding is on the changed lines, as comments below.

[napetrov]

Review at 5d1c627f.

Summary. All earlier findings (antonwolfy, Copilot, exai-bot round 1) are addressed in 0f9bfd9 / 766eae4 / 8940627 / 5d1c627: wrong-shape suites.json and --groups, the narrowed except with a test that pins it, --out "", and the schema-rejection message. I found nothing new on the changed lines and pushed no commits.

Open items

• Minor, design choice for the author (not a small fix, so left as is): tools preference only discovers an unwritable --out-json in _save_result, after the paid probe run, so the results are lost (exai-bot at tools.py:149, also mentioned by antonwolfy as left to the author). A cheap pre-flight (e.g. parent.mkdir + open-for-append check before run_probe) would avoid paying for a run that cannot be saved. Pre-existing behaviour; the PR adds the clean message but not the early check. OK as a follow-up.

Test evidence (Linux, uv run)

• ruff check dokimi tests: clean.
• test_catalog_command.py test_skill_selection.py test_tool_preference.py test_tools_preference_items_out.py: 147 passed.

Verdict from my side: ready; the --out-json pre-flight can be a follow-up.

[antonwolfy]

Review at 5d1c627. The five risks from my earlier review are fixed, and the tests for them fail when each fix is reverted. Two inputs next to the fixed ones still end in a traceback. I'd fix both before merge.

What I ran, on a Linux review host at 5d1c627:

• make ci-local: 29 passed, 0 failed, 7 skipped (docker, network and other-Python legs). The changed-line coverage check passed.
• Reproductions for the code findings, offline.
• Mutation runs: reverting each of the catalog fixes (15 mutations) and each of the tools fixes (10 mutations) makes a new test fail.

@napetrov's and @exai-bot's question on checking --out-json before the paid run waits for the author. The command now exits with a message, but the results are still lost when the path is unwritable.

Checked, no issues

• Re-indented loop: read_task_manifest returns the same entries as the base for a mixed valid fixture, and git diff -w shows only the new _manifest_tasks call.
• Narrowed except in tools.py: --groups as a directory, a BOM, an empty file and wrong types in cues, system, twins, id and name all exit with --groups <path>: ....
• --out in trigger-set: a directory, a file as parent, ., / and a 5000-character name exit with a message naming --out in all three formats.
• Repo rules: the changelog fragment and the growth baseline (16 to 15) pass; no schema or docs pages are touched.

[antonwolfy]

Review at 53e5e33. Both fixes from my last review hold: "groups": null and --out-json "" now exit with a message. Two things are worth fixing before merge: the same deep-nesting input that was fixed for --groups still ends in a traceback in the manifest readers, and the new tool_preference artifact rejected branch has no test.

What I ran, on a Linux review host at 53e5e33:

• make ci-local: 29 passed, 0 failed, 7 skipped (docker, network and other-Python legs). The changed-line coverage check passed.
• Reproductions for the code findings, offline.
• Mutation runs: dropping the ArtifactValidationError branch in _save_result leaves test_tool_preference.py and test_tools_preference_items_out.py green (77 passed). Reverting the groups: null, deep-nesting and huge-int fixes makes the new parametrized cases fail.

@napetrov's and @exai-bot's question on checking --out-json before the paid run still waits for the author. The command now exits with a message, but the results are still lost when the path is unwritable.

Checked, no issues

• --groups: about 50 inputs (nested wrong types, BOM, UTF-16, empty file, NaN, a directory, a missing path, a 5000-character path, 6000-digit ints, [ and {"a": nested deeply) all exit with --groups <path>: ....
• --out-json: an empty path, a directory, a file as parent and . all exit 1 with Error: cannot write <path>: ....
• --out in trigger-set: three formats against 19 odd paths (directory, file as parent, empty, symlink loop, /dev/null/x, 300-character name) give a one-line message, never a traceback.
• Re-indented loop: git diff -w shows only the new _manifest_tasks call in the loop header.
• Repo rules: the changelog fragment and the growth baseline (16 to 15) pass; no schema or docs pages are touched.

[napetrov]

Delta review of 5d1c627f..53e5e33b.

New commit 53e5e33 (null groups, deep/huge JSON in --groups, empty --out-json, schema rejection reported separately from an unwritable path): reads correct; ArtifactValidationError is caught before the broader ValueError, so the two messages stay distinct. Nothing new from me, no commits pushed.

Still open (unchanged, follow-up OK): --out-json is only checked after the paid probe run.

Evidence: ruff check dokimi tests clean; test_tool_preference.py test_tools_preference_items_out.py 77 passed; CI 20/20 green at 53e5e33.

[antonwolfy]

Review at 6c4295c. Both fixes from my last review hold: a deeply nested skills.yaml or suites.json now exits with an invalid ... at <path> message, and the tool_preference artifact rejected branch has a test that fails when the clause is removed. Two risks remain on the neighbouring catalog paths, which still end in a traceback. Both predate the PR, but the changelog says a bad manifest exits with a message.

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

• make ci-local: 29 passed, 0 failed, 7 skipped (docker, network and other-Python legs). The changed-line coverage check passed.
• Reproductions for the code findings, offline.
• Mutation runs: reverting the RecursionError catch for skills.yaml, the one for suites.json, and the OSError handler around is_task_dir makes exactly one new test fail each. Removing the except ArtifactValidationError clause in _save_result makes the new tools test fail with cannot write.

Not anchored inline (outside this diff):

1. A malformed SKILL.md under --catalog still ends in a traceback in catalog trigger-set. discover_skills(catalog_root) at commands/catalog.py:171 runs outside any try. With --catalog holding a/SKILL.md and a valid b/SKILL.md, and --questions q.json=b --out o.json: bytes \xff\xfe give UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff; frontmatter name: [ or a YAML list gives ValueError: Skill 'a' (...) is missing a 'description' in its frontmatter; frontmatter of [ repeated 100000 times gives RecursionError; a/SKILL.md as a directory gives IsADirectoryError. All exit 1 and write no --out. catalog scan reports the same inputs per row, so the two commands disagree. Suggestion: wrap the call in try/except (OSError, ValueError, RecursionError) and raise SystemExit(f"--catalog {catalog_root}: {exc}"), with a parametrized test for a non-UTF-8 and a missing-description SKILL.md.
2. catalog scan --kind tasks without a suites.json still ends in a traceback on a task directory with no instruction.md. discover_tasks(tasks_root) at external/catalog.py:273 is unguarded, unlike the manifest branch below it. With evaluation/harbor/tasks/t/task.toml present, no instruction.md and no suites.json, catalog scan --root <dir> --kind tasks exits with FileNotFoundError: Task has no instruction.md: .../tasks/t. The same task listed in suites.json gives a failed row. Suggestion: catch (OSError, ValueError) around discover_tasks, raise ValueError(f"cannot read tasks at {tasks_root}: {exc}") (which commands/catalog.py already turns into a message), and add a test with a task directory holding only task.toml.

@napetrov's and @exai-bot's question on checking --out-json before the paid run still waits for the author. The command exits with a message, but the results are still lost when the path is unwritable.

Checked, no issues

• Over-long task name: with the tasks directory present, a 300-character track: executable name gives a failed entry (cannot inspect task directory ...: File name too long) in text and JSON, exit 1. A 255-character name is skipped, exit 0.
• Deep nesting: [ repeated 200000 times in skills.yaml (--kind skills and all) and in suites.json (--kind tasks and all) exits with the invalid-manifest message.
• Repo rules: the changelog fragment and the growth baseline (16 to 15) pass; no schema or docs pages are touched.

[antonwolfy]

Review at 61cdeb2. Both fixes from my last review hold: catalog trigger-set exits with a message on a malformed SKILL.md, and catalog scan --kind tasks does the same on a task directory without instruction.md. A sibling path in catalog scan still ends in a traceback for the nested-frontmatter input the new code handles, and the new tests do not pin two of the five new exception types. There is also one wording nit.

What I ran, on a Linux review host at 61cdeb2:

• make ci-local: 29 passed, 0 failed, 7 skipped (docker, network and other-Python legs). The changed-line coverage check passed.
• Reproductions for the code findings, offline.
• Mutation runs: reverting either call site makes the matching new test fail. Dropping RecursionError from the _discover_catalog_skills tuple, or OSError (a SKILL.md that is a directory), leaves the 23 tests in test_catalog_command.py green.

Not anchored inline:

1. Nit: the task-directory error is prefixed suites.json: when no suites.json exists. _scan_tasks (commands/catalog.py:105, outside this diff) wraps every ValueError from read_task_manifest as suites.json: {exc}. The new _discover_tasks_checked runs only in the branch where the manifest is absent. A task directory holding only task.toml gives suites.json: cannot read tasks at <root>/evaluation/harbor/tasks: Task has no instruction.md: <root>/evaluation/harbor/tasks/t. Suggestion: drop the suites.json: prefix, since the invalid suites.json at <path> messages already name the file.

@napetrov's and @exai-bot's question on checking --out-json before the paid run waits for the author.

Checked, no issues

• trigger-set --catalog: a SKILL.md that is a directory, a symlink loop, mode 0, a list frontmatter and a 100000-deep frontmatter all exit 1 with --catalog <root>: ....
• read_task_manifest success path: hoisting found out of the comprehension leaves a good task plus an empty skills directory printing the same ok row.
• Repo rules: the changelog fragment and the growth baseline (16 to 15) pass; no schema or docs pages are touched.

[antonwolfy]

Review at 263ccde. Both fixes from my last review hold: a deeply nested SKILL.md is now an error row in catalog scan, the new tests pin each added exception, and the suites.json: prefix is gone from the task-directory message. One thing is worth fixing before merge: the new --out-json pre-flight in tools preference does not match what the real save does.

What I ran, on a Linux review host at 263ccde:

• make ci-local: 29 passed, 0 failed, 7 skipped (docker, network and other-Python legs). The changed-line coverage check passed.
• Reproductions for the code finding, offline.
• Mutation runs: dropping RecursionError from external/catalog.py, from external/validate.py or from commands/catalog.py makes exactly one new test fail each.

Checked, no issues

• Prefix removal: no test or doc still expects suites.json: in front of the task-directory error.
• Deep SKILL.md: catalog scan --kind skills on name: followed by 100000 [ prints an ERROR: row plus a frontmatter-parse finding, exits 1, and the other skills still scan.
• Pre-flight order: --dry-run and a missing --model exit before the check, and a missing target file is removed again.
• Repo rules: the changelog fragment and the growth baseline (16 to 15) pass; no schema or docs pages are touched.

[antonwolfy]

The --out-json probe now matches the save in the cases I tried, and make ci-local is green. One thing is worth fixing before merge: the tests do not pin the new probe.

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

• make ci-local: 29 passed, 0 failed, 7 skipped (docker, network and other-Python legs). 8,264 tests; changed-line coverage 97%.
• Reproductions for the probe against the real save, offline.
• Mutation runs: 8 mutants of _check_out_json_writable; 3 are caught (no _write_target, no is_dir check, no mkdir) and 5 pass the full suite (no probe, probing target.parent, is_dir on target, no ValueError in the except, and a changed temp prefix). The first three are in the inline comment; the ValueError one needs an embedded NUL byte in the path, which a shell cannot pass, and the prefix one is cosmetic.

Checked, no issues

• Probe against save: a read-only file in a writable directory, a symlink to one, a dangling symlink, a symlink into a missing directory, a symlink loop, a directory target and an empty path give the same accept or reject in the check and in save_artifact. A dangling symlink leaves no file at its target, and no .tmp file is left behind.
• Call order: _check_out_json_writable runs after --dry-run and the --model check and before the first probe call.
• Repo rules: the changelog fragment and the growth baseline (16 to 15) pass; no schema or docs pages are touched.

[antonwolfy]

Review at 41466dd. The three tests from my last review are in, and they kill the probe mutants I named (no probe, target.parent, no is_dir on the resolved path). One nit inline: the probe's temp name is shorter than the save's, so a very long file name passes the pre-flight and the save then fails after the paid calls.

What I ran, on a Linux review host at 41466dd:

• make ci-local: 29 passed, 0 failed, 7 skipped (docker, network and other-Python legs). 8,267 tests; changed-line coverage 97%.
• Reproduction for the code finding, offline.
• Mutation runs: of 7 mutants of _check_out_json_writable, 6 are caught (no probe, target.parent, no mkdir, no is_dir check, open(..., 'ab') probe, no _write_target). Dropping ValueError from the except passes the suite; it needs an embedded NUL byte in the path, which a shell cannot pass, so I did not raise it.

Checked, no issues

• Probe against save: a read-only file, a symlink into a read-only directory, a dangling symlink, a symlink loop (as file and as parent), a trailing slash, an empty path and a 0555 directory give the same accept or reject in the check and in save_artifact, and the probe leaves no file behind.
• Call order: the check runs after --dry-run and the --model check, and before the first model call.
• Repo rules: the changelog fragment and the growth baseline (16 to 15) pass; no schema or docs pages are touched.

[antonwolfy]

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

• 8f04440: dokimi/dokimi/commands/tools.py:129: the probe now creates <name>.<pid>.<thread id>.tmp, the save's temp name, and a new test pins a 240-character file name to exit 2.

make ci-local at 8f04440: 29 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.

[xaleryb]

• Ran catalog scan on a manifest-listed deep SKILL.md and on falsy wrong-typed suites.json values; full suite, ruff, mypy, bandit on the head with the commits below merged with main.
• skills.yaml listing a skill whose SKILL.md frontmatter is [*100000 -> RecursionError traceback (_skill_entry catches only OSError/ValueError) -> the same ERROR row the no-manifest scan gives. Residual of the deep-SKILL.md thread, which was fixed only in _scan_skill_files.
• {"suites": {}}, {"suites": 0}, {"suites": [{"tasks": {}}]} -> 0 declared, rc 0 -> invalid suites.json, rc 1. or [] turns falsy wrong types into an empty list before the shape check.
• Pushed:
  • 2a23d6a: add RecursionError to the _skill_entry except, plus a manifest test.
  • 3d29187: only null or a missing key counts as empty; {} and 0 are added to the shape test, with a null-is-empty test.
• Left for the author: whether to turn RecursionError into ValueError once, in load_skill, instead.

[antonwolfy]

LGTM. Both new commits are right: a SKILL.md that is too deeply nested and is listed in skills.yaml is now an error row, and suites.json rejects {}, 0 and {"tasks": {}} while null or a missing key still reads as empty. No new findings. @xaleryb left one choice to you: whether to turn RecursionError into ValueError once, in load_skill.

What I ran, on Linux at 3d29187:

• make ci-local: 26 passed, 2 failed, 7 skipped; 8,722 tests; the changed-line coverage check passed. The two failures are the gitleaks install and scan steps, which could not download the gitleaks binary (curl: (22) ... 504); not caused by this PR.
• Mutation runs on the new tests: dropping RecursionError from _skill_entry, restoring or [] for suites or for tasks, and removing either None to [] mapping each make at least one new test fail.

Checked, no issues

• Delta: catalog scan --kind skills on a skills.yaml that lists a SKILL.md with [ repeated 100000 times in the frontmatter prints an ERROR: row, exits 1, and the other skills still scan.
• Repo rules: the changelog fragment is present, the growth baseline entry (16 to 15) passes, and no schema is touched.