Repository navigation
Conversation
Before the labelled queries, one explicit run ('Use the <skill> skill.') must
load that skill. If it does not, the probe writes status
'invalid: skill not visible', prints it, exits 1 and runs no labelled query,
so a skill the harness never shows cannot read as a 0% trigger rate.
--no-known-positive skips the check (status 'unchecked').
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
CLI interface change detectedThis PR changes dokimi's user-facing CLI surface (commands and/or options): ~ `dokimi catalog trigger-probe` parameters changed:
+ added option `--no-known-positive`Updated on every push; the |
github-actions
Bot
added
the
cli-interface-change
|
✅ OpenCodeReview: Review complete: 0 finding(s) across 7 selected item(s). Review details
|
| return None | ||
| from dokimi.harnesses import trigger_probe as tp | ||
|
|
||
| known = tp.known_positive(args.target or sorted(names)[0], catalog, cfg) |
There was a problem hiding this comment.
Must-fix: names can be empty here. For example, use an empty existing --catalog directory and a trigger set containing {"questions":[{"id":"a","question":"Hello","expected_skill":null}]} with no --target. Discovery returns no skills, and _labelled_queries accepts the explicitly labelled negative query because its acceptable-skill list is empty. The new default guard then raises IndexError at sorted(names)[0], instead of reporting a controlled input error. Reject an empty catalog before selecting the guard skill, and add a regression test for this negative-only set.
There was a problem hiding this comment.
Fixed in 1375814. An empty --catalog is now refused with --catalog holds no skills before the known-positive guard picks its skill, so a negative-only set no longer reaches sorted(names)[0]. Runs with --no-known-positive are unchanged.
Regression tests: test_cli_refuses_an_empty_catalog_before_the_guard_picks_a_skill (red before the fix, IndexError at catalog_probe.py:123), and test_no_known_positive_still_runs_a_negative_only_set_on_an_empty_catalog.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #320 +/- ##
==========================================
+ Coverage 94.05% 94.07% +0.02%
==========================================
Files 297 297
Lines 38018 38063 +45
Branches 6424 6431 +7
==========================================
+ Hits 35756 35807 +51
+ Misses 1452 1450 -2
+ Partials 810 806 -4
🚀 New features to boost your workflow:
|
…-positive-guard
1375814 to
48cbd0f
Compare
A negative-only trigger set against a catalog with no skills reached sorted(names)[0] in the known-positive guard and raised IndexError. Refuse the empty catalog as an input error before the guard picks its skill. Runs with --no-known-positive are unchanged. Cover both with regression tests. Thread: PRRT_kwDOVBg7g86yOGu2 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at 48cbd0f. The guard does what it says on the happy path: it runs once before any labelled query, its cost is added to the total, and --no-known-positive keeps the old behaviour. The main gap is that a failed run (API error, timeout) is reported as "skill not visible"; the details are in the line comments.
What I ran, at 48cbd0f:
make ci-local --offline: 0 failed, 19 passed, 13 skipped (network/Docker jobs); the test suite has 11,980 passed, 58 skipped.- A fake
claudethat listssum-fastin its init event and then returns anis_errorresult: the probe exits 1 with"status": "invalid: skill not visible".
Checked, no issues
- Empty catalog (the bot's thread): fixed in 48cbd0f; an empty
--catalogis refused beforesorted(names)[0], and both new tests cover it. - Order of checks: bad input, a bad
--outand an unlabelled set still exit before the paid guard run. - CLI snapshot and allowlist:
--no-known-positiveis incli-interface.jsonandcli_flags_allowlist.txt; the changelog fragment is under Added.
| _write(out_path, {"harness": "claude-code", "status": _INVALID, "known_positive": known, | ||
| "mode": "sealed" if args.sealed else "stock", "model": args.model, | ||
| "catalog": str(catalog), "answers": []}) | ||
| print(json.dumps({"status": _INVALID, "known_positive": known}, indent=2)) | ||
| raise SystemExit(f"{_INVALID}: {known['query']!r} did not load {known['skill']!r}" | ||
| + (f" ({known['error']})" if known["error"] else "") + "; no labelled query was run") |
There was a problem hiding this comment.
Any failure of the guard run is written as status: "invalid: skill not visible", including failures that say nothing about visibility.
A timeout, no result event, an API or auth error (result.is_error), or an OSError/RuntimeError from staging all reach this branch, because it only checks known["passed"]. Repro: a fake claude that prints an init event listing sum-fast and then {"type":"result","subtype":"error_during_execution","is_error":true}. The probe exits 1 and probe.json says "status": "invalid: skill not visible", with "error": "error_during_execution" inside known_positive, even though that same run listed the skill. Someone reading the artifact concludes the harness hides the skill when the run simply failed, which is the "an error reads as a result" case dokimi tries to avoid.
Suggestion: when known["error"] is set, write a separate status such as error: known-positive run failed (and say so in the SystemExit message and the docs); keep invalid: skill not visible for a run that finished without an error and did not load the skill. A test with an is_error result would cover it.
There was a problem hiding this comment.
Fixed in c60be7d. A guard run that errors now writes status: "error: known-positive run failed", exits 1, and names that status in the message. invalid: skill not visible stays for a run that finished without an error and did not load the skill.
Test: test_cli_reports_a_guard_run_that_errored_apart_from_a_skill_not_shown in dokimi/tests/test_trigger_probe.py (a fake guard run with error_during_execution). Docs and changelog updated.
| query = f"Use the {skill} skill." | ||
| try: | ||
| r = run_query(query, catalog, cfg, runner=runner, source_env=source_env) | ||
| except (OSError, RuntimeError) as exc: | ||
| return {"skill": skill, "query": query, "passed": False, "loaded": [], "cost_usd": None, | ||
| "error": f"{type(exc).__name__}: {exc}"} | ||
| return {"skill": skill, "query": query, "passed": skill in r["skill"]["loaded"], | ||
| "loaded": r["skill"]["loaded"], "cost_usd": r["cost_usd"], "error": r["error"]} |
There was a problem hiding this comment.
The guard shows that the Skill tool accepts the name from the prompt, not that the harness listed or described the skill.
Use the <skill> skill. names the skill, so the model can call Skill(<skill>) even if the listing budget (--listing-budget-fraction) cut its entry or description. In that case the guard passes and the labelled queries can still score about 0% for the reason the docs say the guard rules out ("the harness never showed it", evaluating-treatments.md:603-604). The guard run already parses the init event, but known_positive drops r["skill"]["listed"].
Suggestion: keep listed in the known_positive record, and either require the skill to be listed when the init event lists any skills, or narrow the docs to "the skill can be loaded". Do you know whether Claude Code accepts a Skill call for a skill it did not list?
There was a problem hiding this comment.
Partly fixed in c60be7d: known_positive keeps listed (and claude_code_version), so the output shows whether the harness listed the skill. The guard still passes on the Skill call alone.
Open: whether the guard should also require the skill to be listed, or the docs should narrow to "the skill can be loaded". Not changed here; I did not verify whether Claude Code accepts a Skill call for an unlisted skill.
| _write(out_path, {"harness": "claude-code", "status": _INVALID, "known_positive": known, | ||
| "mode": "sealed" if args.sealed else "stock", "model": args.model, | ||
| "catalog": str(catalog), "answers": []}) |
There was a problem hiding this comment.
The invalid output has no top-level cost_usd and drops header fields the normal output always has.
The guard run is paid, but its cost is only in known_positive.cost_usd; trials, max_turns, listing_budget_fraction, claude_code_version are missing too. A script that sums out["cost_usd"] over probe files raises KeyError or misses the spend. The header is also rebuilt by hand here, so it will keep drifting from the dict at lines 82-89.
Suggestion: build the shared header (harness, status, known_positive, mode, model, catalog, settings) in one helper used by both writes, and set cost_usd to the guard's cost in the invalid output.
There was a problem hiding this comment.
Fixed in c60be7d. One _header helper builds the header for both writes: trials, max_turns, listing_budget_fraction, claude_code_version, cost_usd. The refused output carries the guard cost in cost_usd.
Test: test_the_invalid_output_has_the_header_fields_and_the_guard_cost in dokimi/tests/test_trigger_probe.py.
| cost += (known or {}).get("cost_usd") or 0 | ||
| out = { | ||
| "harness": "claude-code", | ||
| "status": "ok" if known else "unchecked", |
There was a problem hiding this comment.
Nit: status is ok whenever the guard passed, even when the same run then exits 1 because not_listed is non-empty.
The output then says "status": "ok" while the CLI raises catalog skills Claude Code did not list: .... A consumer that filters on status == "ok" accepts a run the command rejected.
Suggestion: either set a non-ok status when not_listed is non-empty, or name the field after what it reports (for example known_positive_status).
There was a problem hiding this comment.
Fixed in c60be7d: a run with not_listed now has status invalid: skill not listed, not ok. The stdout status and the probe file match.
Test: test_cli_fails_when_claude_code_did_not_list_a_catalog_skill now asserts the status.
| labelled query exits 2 before any run. Records are kept in `<out>.partial` while the | ||
| probe runs: | ||
| labelled query exits 2 before any run. Before the labelled queries, one explicit run | ||
| (`Use the <skill> skill.`, for the `--target` skill or the first skill of the catalog) must |
There was a problem hiding this comment.
Nit: "The first skill of the catalog" is the alphabetically first frontmatter name (sorted(names)[0]), not the first directory or listing entry.
With sum-fast/ and io-read/ the guard checks io-read. Also, since 48cbd0f an empty --catalog is refused unless --no-known-positive is passed; before this PR a negative-only set ran on it. Neither the docs nor the changelog fragment say so.
Suggestion: write "the alphabetically first skill name" here and in the changelog fragment, and add a sentence that an empty catalog is refused while the guard is on.
There was a problem hiding this comment.
Fixed in c60be7d: the docs and changelog say "the alphabetically first skill name", and that an empty catalog is refused (exit 2) before the guard runs, unless --no-known-positive is passed.
An error in the known-positive run (timeout, API or auth error, an error result) wrote "invalid: skill not visible", which reads as the harness hiding the skill. It now writes "error: known-positive run failed"; "invalid: skill not visible" is kept for a run that finished without an error and did not load the skill. - not_listed no longer reports status "ok" when the same run exits 1: the status is "invalid: skill not listed". - Every output carries the same header (trials, max_turns, listing_budget_fraction, claude_code_version, cost_usd), built by one helper; the refused output carries the guard's cost. - known_positive keeps "listed" and "claude_code_version". - The docs and changelog say "alphabetically first skill name", and that an empty catalog is refused before the guard runs. Tests: an errored guard run, the not-listed status, the invalid output's header and cost. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
There was a problem hiding this comment.
🟡 Changes recommended
The guard can accept failed or wrong-namespace skill loads, while its refusal path can misdiagnose failures, break report compatibility, and leave stale results.
6 open issues
What changed in this PR
8 new findings.
This PR adds a known-positive check to catalog trigger-probe before scoring labelled queries, aiming to distinguish harness loading failures from poor skill selection.
Changes:
- Adds an explicit skill-loading probe, failure reporting, and guard-cost accounting.
- Introduces
--no-known-positiveand new report status fields. - Adds regression tests and updates CLI references, documentation, and release notes.
| File | Description |
|---|---|
dokimi/tests/test_trigger_probe.py |
Tests guard outcomes, target selection, bypass behavior, and empty catalogs. |
dokimi/tests/data/cli_flags_allowlist.txt |
Allows the new bypass flag. |
dokimi/dokimi/harnesses/trigger_probe.py |
Implements the explicit known-positive probe. |
dokimi/dokimi/commands/catalog_probe.py |
Integrates the guard, report statuses, costs, and CLI flag. |
dokimi/docs/reference/cli-interface.json |
Records the new CLI option. |
dokimi/docs/evaluating-treatments.md |
Documents guard behavior and failure statuses. |
dokimi/changelog.d/20261030_120000_napetrov_known-positive-guard.md |
Announces the guard and bypass option. |
Review effort: Balanced
| return {"skill": skill, "query": query, "passed": False, "loaded": [], "listed": [], "cost_usd": None, | ||
| "claude_code_version": None, "error": f"{type(exc).__name__}: {exc}"} | ||
| # The guard passes on the Skill call alone; ``listed`` records whether the harness showed the skill. | ||
| return {"skill": skill, "query": query, "passed": skill in r["skill"]["loaded"], |
There was a problem hiding this comment.
Fixed in ca1849a. A Skill call counts for the guard only when its tool_result is not an error; parse_stream gives the successful loads as loaded_ok. Test: a Skill result with is_error no longer passes the guard.
| return {"skill": skill, "query": query, "passed": False, "loaded": [], "listed": [], "cost_usd": None, | ||
| "claude_code_version": None, "error": f"{type(exc).__name__}: {exc}"} | ||
| # The guard passes on the Skill call alone; ``listed`` records whether the harness showed the skill. | ||
| return {"skill": skill, "query": query, "passed": skill in r["skill"]["loaded"], |
There was a problem hiding this comment.
Fixed in ca1849a. The guard now compares the raw skill name from the transcript, so plug:sum-fast does not satisfy sum-fast. The labelled scoring still uses the stripped names. Test: a plugin-namespaced load fails the guard.
| if known["passed"]: | ||
| return known | ||
| # A run that errored says so: "not visible" would read as the harness hiding the skill. | ||
| status = _ERROR if known["error"] else _INVALID |
There was a problem hiding this comment.
Fixed in ca1849a. A skill that the init event lists but the run did not load is now invalid: skill not loaded; invalid: skill not visible is kept for a skill that is not listed. The docs paragraph is updated to match.
| _write(out_path, {**_header(args, catalog, known, status, known.get("cost_usd") or 0, | ||
| known.get("claude_code_version")), "answers": []}) |
There was a problem hiding this comment.
Fixed in ca1849a. The refusal output now carries max_turns_hit 0, not_listed [] and skill_selection_summary {}, the same keys as a normal run. Test: the keys are present.
| status = _ERROR if known["error"] else _INVALID | ||
| _write(out_path, {**_header(args, catalog, known, status, known.get("cost_usd") or 0, | ||
| known.get("claude_code_version")), "answers": []}) | ||
| print(json.dumps({"status": status, "known_positive": known}, indent=2)) |
There was a problem hiding this comment.
Fixed in ca1849a. A guard refusal removes a leftover .partial after writing the final report, as the normal path does. Test: a stale partial is gone after a refusal.
| # The guard passes on the Skill call alone; ``listed`` records whether the harness showed the skill. | ||
| return {"skill": skill, "query": query, "passed": skill in r["skill"]["loaded"], | ||
| "loaded": r["skill"]["loaded"], "listed": r["skill"]["listed"], "cost_usd": r["cost_usd"], | ||
| "claude_code_version": r["skill"]["claude_code_version"], "error": r["error"]} |
There was a problem hiding this comment.
Fixed in ca1849a. run_query returns terminal_error without the Skill-call suppression; the guard reports it, so a timeout after another skill's call shows as timeout. Labelled scoring keeps its error field. Test added.
… the real failure Skill results with is_error, plugin-namespaced names and a timeout after another skill call no longer pass or mask the guard; a listed-but-unloaded skill is 'invalid: skill not loaded'; the refusal output carries the probe keys and drops a stale .partial. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| "first_turn_loaded": first, | ||
| "loaded": [s for t in turns for s in t], | ||
| # Raw names (plugin namespace kept) of Skill calls whose result was not an error. | ||
| "loaded_ok": [name for tid, name in calls if tid is None or tid not in failed_ids], |
There was a problem hiding this comment.
Must-fix: This treats a missing tool result as a successful load. For example, remove the user tool-result event from _skill_stream("sum-fast", result=False) and run it with timed_out=True: loaded_ok still contains sum-fast, so known_positive returns passed: true and clears the timeout via error: None if passed .... The command then runs labelled queries without evidence that the target loaded. Require an observed, matching non-error tool_result rather than merely the absence of an error result, and add a regression test for a target call followed by timeout before its result (the current timeout test calls another skill).
There was a problem hiding this comment.
Fixed in 66cb163. A Skill call now counts as loaded only with a non-error tool_result for its id (ok_ids). A call with no result is not a load, so the guard fails. Test: test_known_positive_fails_when_the_target_call_timed_out_before_its_result.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at ca1849a. The guard logic is sound on the paths the tests cover, and the earlier threads are fixed. One risk before merge: a Skill call with no result counts as a load. Two nits inline.
What I ran, on a Linux review host at ca1849a:
make ci-localwith Docker: 35 passed, 0 failed, 7 skipped (job conditions, no semgrep or micromamba). The changelog fragment, CLI snapshot and changed-line coverage steps pass.- Reproductions for the code finding, with the LLM transport mocked.
- Mutation runs on the new tests: dropping the error filter, using
loadedinstead ofloaded_ok, removing the partial cleanup and reordering the status all fail the suite. Making a target call followed by a timeout pass is not caught.
Checked, no issues
- Order of checks: a bad
--out, an unlabelled set, an empty catalog and an unknown--targetall exit before the paid guard run; the fakeclaudewas never called. - Cost: the guard cost is added once (0.05 + 2 x 0.02 = 0.09).
--no-known-positive:status: unchecked,known_positive: null, no extra run.- Sign-off: every non-merge commit carries
Signed-off-by.
I agree with @vlad-perevezentsev that the guard shows the Skill tool accepts the name, not that the harness listed it. That question waits for the author.
Bot points I checked that no longer apply at this head (7)
- exai-bot's point on an empty
--catalogdoes not apply: it is refused with exit 2 before the guard picks a skill. - Copilot's point on an errored
tool_resultdoes not apply: a Skill call whose result hasis_errorno longer counts (the missing-result case is a separate finding inline). - Copilot's point on plugin namespaces does not apply: the guard compares the raw skill name, so
plug:sum-fastdoes not satisfysum-fast. - Copilot's point on a listed but unloaded skill does not apply: that case now reports
invalid: skill not loaded, andnot visibleis kept for an unlisted skill. - Copilot's point on the refusal output's keys does not apply: it carries
max_turns_hit,not_listedandskill_selection_summary. - Copilot's point on a stale
<out>.partialdoes not apply: the refusal removes it. - Copilot's point on a timeout after another skill's call does not apply:
terminal_erroris kept apart from the scoringerror, and a test covers it.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
| `plugin:skill` of another plugin, does not count as a load. If that run ended in an error, `status` is `error: known-positive run failed` instead. An empty catalog is | ||
| refused (exit 2) before the guard runs, unless `--no-known-positive` is passed. This | ||
| known-positive check is what separates "the model did not pick the skill" from "the | ||
| harness never showed it"; `--no-known-positive` skips it, and `status` is then `unchecked`. |
There was a problem hiding this comment.
Nit: --no-known-positive does not always produce unchecked. For example, skip the guard with catalog skills sum-fast and io-read, then have the labelled runs list only sum-fast: catalog_probe.py gives invalid: skill not listed precedence and exits 1. Qualify this promise with “unless listing validation fails,” and update the same unconditional statement in the changelog fragment.
There was a problem hiding this comment.
Narrowed in 66cb163. The docs and changelog now say status is unchecked, or invalid: skill not listed when a catalog skill is never listed. The unlisted-target claim is gone.
| "claude_code_version": None, "error": f"{type(exc).__name__}: {exc}"} | ||
| # The guard passes on a successful call of exactly this skill (a `plugin:skill` is another one); | ||
| # ``listed`` records whether the harness showed it. A failure is reported even after a Skill call. | ||
| passed = skill in r["skill"]["loaded_ok"] |
There was a problem hiding this comment.
Must-fix: A successful target Skill result suppresses a subsequent terminal run error. For example, keep the successful tool_result in _skill_stream("sum-fast") but change its final event to {"type":"result","subtype":"error_during_execution","is_error":true}. run_query preserves that terminal error, yet this returns passed: true, error: null, so the CLI runs the labelled set instead of writing the documented error: known-positive run failed refusal. This is independent of the already-reported missing-tool-result issue: the tool result exists here. Require no terminal error when deciding passed, preserve the error, and add a regression for an error after a successful target load.
Suggestion:
| passed = skill in r["skill"]["loaded_ok"] | |
| passed = skill in r["skill"]["loaded_ok"] and r["terminal_error"] is None |
There was a problem hiding this comment.
Fixed in 66cb163. A terminal error after the target loaded now fails the guard and is reported. Test: test_known_positive_keeps_a_terminal_error_after_the_target_loaded.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to e38ba84, which fix the two nits from my review:
89285d7:dokimi/dokimi/commands/catalog_probe.py:207: the--no-known-positivehelp now says a failed guard exits 1 with an 'invalid: ...' or 'error: ...' status.e38ba84:dokimi/docs/evaluating-treatments.md:597: the docs now saystatusisinvalid: skill not listedwhennot_listedis non-empty, and the labelled results are still written.
make ci-local at e38ba84: 35 passed, 0 failed, 7 skipped.
Not approving yet. Still open: the risk from my review; @vlad-perevezentsev on whether the guard should also require the skill to be listed, or the docs narrow to 'the skill can be loaded'.
…say so A Skill call counts as loaded only when its tool_result arrived and was not an error, so a call that hung or was killed is no longer a pass. A terminal error after the target loaded still fails the guard and is reported. The docs no longer say an unlisted target is refused first with `invalid: skill not listed`: that status comes from the catalog check, and the guard reports an unlisted target as `invalid: skill not visible`. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| exits 1. A skill call whose result is an error, or another plugin's `plugin:skill`, does | ||
| not count as a load. So a skill the harness never shows cannot read as a 0% trigger rate. A guard run | ||
| that ended in an error writes `"status": "error: known-positive run failed"` instead. An | ||
| empty `--catalog` is refused (exit 2) before the guard runs. Pass |
There was a problem hiding this comment.
Nit: The empty-catalog refusal is conditional on the guard being enabled. With --no-known-positive and a negative-only trigger set, an empty catalog is supported, as test_no_known_positive_still_runs_a_negative_only_set_on_an_empty_catalog confirms. Qualify this statement with “unless --no-known-positive is passed,” matching the evaluation guide.
There was a problem hiding this comment.
Fixed in 5a0e2b0. The sentence now reads "unless --no-known-positive is passed", matching the evaluation guide. test_no_known_positive_still_runs_a_negative_only_set_on_an_empty_catalog is the case it describes.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| def fake_known(skill, catalog, cfg, **kw): | ||
| seen.setdefault("known", []).append(skill) |
There was a problem hiding this comment.
Nit: The refusal tests check the empty output, but do not verify that labelled queries never ran. Moving _known_positive_guard(...) below pool.map(...) would still satisfy them: the guard overwrites the output with empty answers and removes the partial file, after incurring the labelled-run costs. The success test named ...before_the_set also checks only the selected target. Assert in fake_known that no queries have been recorded yet; this protects execution order across the successful, invisible-skill, not-loaded, and error scenarios without interfering with --no-known-positive.
Suggestion:
| def fake_known(skill, catalog, cfg, **kw): | |
| seen.setdefault("known", []).append(skill) | |
| def fake_known(skill, catalog, cfg, **kw): | |
| assert not seen.get("queries"), "labelled queries ran before the known-positive guard" | |
| seen.setdefault("known", []).append(skill) |
There was a problem hiding this comment.
Fixed in 75bfb9c. The fake guard records a labelled query that ran before it, and the test helper checks that record in a finally block, after the CLI returns or exits, so no handler in the CLI can hide it. Mutation check: moving the guard after the labelled runs fails 13 tests.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 5a0e2b0. The merge waits for @vlad-perevezentsev's question on whether the guard should also require the skill to be listed, which is with the author. My earlier risk is fixed: a Skill call with no result no longer passes the guard. One nit inline.
What I ran, on a Linux review host at 5a0e2b0:
make ci-localwith Docker: 35 passed, 0 failed, 7 skipped (job conditions, no semgrep or micromamba). 11,990 tests passed, 59 skipped; changed-line coverage 100% (50 lines).- Reproductions with the LLM transport mocked: result shapes (content list, no
is_errorkey,is_error: null), parallel Skill calls, subagent events, a target call followed by a timeout, a terminal error after a load. - A mutation run on the guard: 54 mutants, 38 killed. Every mutant on
passed,loaded_ok,terminal_error, the guard order, the exit code and the.partialcleanup was killed.
Still open from my earlier review: none.
Checked, no issues
- Delta:
loaded_oktakes only calls whose id has a non-error result, andpassedalso needsterminal_error is None.error_max_turnsis not a terminal error, so a load followed by it still passes. - Labelled scoring:
loaded_okis read only inknown_positive;run_queryand the labelled paths useselected,first_turn_loadedandloaded, unchanged. - Sign-off: every non-merge commit carries
Signed-off-by.
Bot points I checked that no longer apply at this head (9)
- exai-bot's point on an empty
--catalogdoes not apply: it is refused with exit 2 before the guard picks a skill. - Copilot's point on an errored
tool_resultdoes not apply: a Skill call whose result hasis_errordoes not count as a load. - Copilot's point on plugin namespaces does not apply: the guard compares the raw skill name, so
plug:sum-fastdoes not satisfysum-fast. - Copilot's point on a listed but unloaded skill does not apply: that case reports
invalid: skill not loaded, andnot visibleis kept for an unlisted skill. - Copilot's point on the refusal output's keys does not apply: it carries
max_turns_hit,not_listedandskill_selection_summary. - Copilot's point on a stale
<out>.partialdoes not apply: the refusal removes it. - Copilot's point on a timeout after another skill's call does not apply:
terminal_erroris kept apart from the scoringerror, and a test covers it. - exai-bot's point on a missing tool result does not apply:
loaded_oknow needs a non-errortool_resultfor the call's own id, so a hang or kill fails the guard. - exai-bot's point on a terminal error after a successful load does not apply:
passednow also requiresterminal_error is None.
The fake guard records a labelled query recorded before it, and the test helper checks that record after the CLI returns or exits, so no handler in the CLI can swallow it. A refusal then costs no labelled runs, as the tests intend. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The guard's run is part of the reported cost; a success run that drops it now fails the test (mutation: cost += 0 fails it). Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
The merge waits for @vlad-perevezentsev's question on whether the guard should also require the skill to be listed, or the docs narrow to "the skill can be loaded"; the author handed it back with "not changed here", so it waits for a decision. My earlier nit on the success-path cost is fixed. One nit inline on the changelog.
What I ran, on a Linux review host at c8769ea:
make ci-localwith Docker: 35 passed, 0 failed, 7 skipped (job conditions, no semgrep or micromamba). The changelog fragment, CLI snapshot and changed-line coverage steps pass.- Mutation runs on the delta: 12 mutants of
loaded_ok,passed, the guard cost, the guard order and the status; 11 fail the suite. The survivor is a Skill call without anidcounting as loaded; real streams carry ids, so I do not raise it. - Fake-runner probes of result shapes, parallel and duplicate ids, and
error_max_turnsafter a load, with the transport mocked.
Checked, no issues
- Delta:
loaded_oktakes only calls whose id has a non-error result, andpassedalso needsterminal_error is None.error_max_turnsafter a load still passes. - Labelled scoring:
run_query,selected,first_turn_loadedandloadedare unchanged against the PR's base. - Sign-off: every non-merge commit carries
Signed-off-by.
Bot points I checked that no longer apply at this head (9)
- exai-bot's point on an empty
--catalogdoes not apply: it is refused with exit 2 before the guard picks a skill. - Copilot's point on an errored
tool_resultdoes not apply: a Skill call whose result hasis_errordoes not count as a load. - Copilot's point on plugin namespaces does not apply: the guard compares the raw skill name, so
plug:sum-fastdoes not satisfysum-fast. - Copilot's point on a listed but unloaded skill does not apply: that case reports
invalid: skill not loaded, andnot visibleis kept for an unlisted skill. - Copilot's point on the refusal output's keys does not apply: it carries
max_turns_hit,not_listedandskill_selection_summary. - Copilot's point on a stale
<out>.partialdoes not apply: the refusal removes it. - Copilot's point on a timeout after another skill's call does not apply:
terminal_erroris kept apart from the scoringerror, and a test covers it. - exai-bot's point on a missing tool result does not apply:
loaded_okneeds a non-errortool_resultfor the call's own id, so a hang or kill fails the guard. - exai-bot's point on a terminal error after a successful load does not apply:
passedalso requiresterminal_error is None.
| explicit run (`Use the <skill> skill.`, for `--target` or the alphabetically first skill name of the catalog) | ||
| must load that skill. If it does not, the probe writes `"status": "invalid: skill not | ||
| loaded"` (Claude Code listed it) or `"invalid: skill not visible"` (it did not), with | ||
| `known_positive`, the probe's `max_turns_hit`/`not_listed`/`skill_selection_summary` keys |
There was a problem hiding this comment.
Nit: The entry says known_positive is "empty" in the refused output, but it holds the guard run.
On a refusal known_positive carries skill, query, passed, loaded, listed, cost_usd and error, and it is the only place the cause shows. Only max_turns_hit (0), not_listed ([]), answers ([]) and skill_selection_summary ({}) are empty.
Suggestion: write line 7 as with `known_positive` holding the guard run, the probe's `max_turns_hit`/`not_listed`/`skill_selection_summary` keys and start line 8 with at 0, `[]` and `{}`, no answers, no stale `<out>.partial`,.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Summary
catalog trigger-probenow checks that the harness can load the skill before it scores the labelled set. A skill the harness never shows scores every query 0%, which reads as a finding.The output keeps every existing key;
statusandknown_positiveare added. The refusal costs one query. The check runs one skill, so it shows that the harness loads skills at all, not that every skill in the catalog is visible;not_listedstill covers skills missing from the init listing.Evidence
New tests in
tests/test_trigger_probe.pyuse a fake runner and no network. Against the base sources they fail, and they pass on this branch.ruff format --checkis not enforced in this repo (the[tool.ruff]comment says so), and the touched files were already unformatted at the base, so the diff keeps the existing layout.Merge Danger
--target) skill is not loaded now exits 1 before any labelled run, where it used to write a 0% report and exit 0. Scripts that expect a report must pass--no-known-positiveor fix the catalog.invalid: skill not visible;known_positive.errorand the stderr message carry the real cause.--no-known-positiveis allowlisted intests/data/cli_flags_allowlist.txt(no registry concept yet) anddocs/reference/cli-interface.jsonis regenerated.🤖 Generated with Claude Code