Skip to content

feat(catalog): known-positive guard for catalog trigger-probe - #320

Open
napetrov wants to merge 8 commits into
intel:mainfrom
napetrov:fix/known-positive-guard
Open

napetrov wants to merge 8 commits into
intel:mainfrom
napetrov:fix/known-positive-guard

Conversation

@napetrov

Copy link
Copy Markdown
Contributor

Summary

catalog trigger-probe now 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.

 catalog trigger-probe --catalog <dir> --trigger-set <set> --out probe.json
+  known_positive(target or first skill)   # one run: "Use the <skill> skill."
+    loaded the skill   -> continue to the labelled queries   "status": "ok"
+    did not            -> write status "invalid: skill not visible" to --out and stdout,
+                          exit 1, no labelled query runs
   labelled queries ...                    (unchanged)
+  --no-known-positive                     skip the check      "status": "unchecked"

The output keeps every existing key; status and known_positive are 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_listed still covers skills missing from the init listing.

Evidence

New tests in tests/test_trigger_probe.py use a fake runner and no network. Against the base sources they fail, and they pass on this branch.

before: harnesses/trigger_probe.py and commands/catalog_probe.py reverted to intel/main, tests kept
  7 failed, 23 deselected   (known_positive, guard refusal, --no-known-positive, target choice)

after (py3.13, uv run --extra dev pytest -n auto -q):
  11286 passed, 26 skipped
ruff check .: passed    mypy dokimi: no issues in 295 files
python -m dokimi.schema_check: ok    python -m dokimi.config_check: ok

ruff format --check is 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

  • Exit code: a catalog whose first (or --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-positive or fix the catalog.
  • One extra paid run per invocation (a single explicit query).
  • A failed known-positive run (CLI cannot start, timeout before any Skill call) also reports invalid: skill not visible; known_positive.error and the stderr message carry the real cause.
  • New flag --no-known-positive is allowlisted in tests/data/cli_flags_allowlist.txt (no registry concept yet) and docs/reference/cli-interface.json is regenerated.

🤖 Generated with Claude Code

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>
@napetrov
napetrov requested a review from a team as a code owner October 31, 2026 04:57
@github-actions

Copy link
Copy Markdown

CLI interface change detected

This 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 cli-interface-change label is on while the diff is non-empty.

@github-actions github-actions Bot added the cli-interface-change Changes dokimi's user-facing CLI surface label Oct 31, 2026
@exai-bot

exai-bot commented Oct 31, 2026 •

Copy link
Copy Markdown

🔍 OpenCodeReview found 1 issue(s) in this PR.

By severity: 1 low

Severity Category Location Finding
low documentation 20261030_120000_napetrov_known-positive-guard.md:12 Nit: The empty-catalog refusal is conditional on the guard being enabled.
Review details
  • Commit reviewed: 66cb163 against merge base b4a0a8c
  • Coverage: 7 files in 1 groups
  • Engine: OpenCodeReview v1.13.3, us.openai.gpt-6.1-sol
  • Usage: 283k tokens (176k from cache), 32s
  • Tool calls: 12 (8 file_read, 3 code_search, 1 code_comment)
  • Known-positive guard for treatment evaluation and probe commands: 7 files

return None
from dokimi.harnesses import trigger_probe as tp

known = tp.known_positive(args.target or sorted(names)[0], catalog, cfg)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

bug · medium
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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

codecov-commenter commented Oct 31, 2026 •

Copy link
Copy Markdown

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

Codecov Report

❌ Patch coverage is 96.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.06%. Comparing base (b4a0a8c) to head (e38ba84).

Files with missing lines Patch % Lines
dokimi/dokimi/harnesses/trigger_probe.py 90.47% 1 Missing and 1 partial ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #320      +/-   ##
==========================================
+ Coverage   94.05%   94.06%   +0.01%     
==========================================
  Files         297      297              
  Lines       38018    38063      +45     
  Branches     6424     6431       +7     
==========================================
+ Hits        35756    35803      +47     
+ Misses       1452     1451       -1     
+ Partials      810      809       -1     
Files with missing lines Coverage Δ
dokimi/dokimi/commands/catalog_probe.py 91.80% <100.00%> (+2.44%) ⬆️
dokimi/dokimi/harnesses/trigger_probe.py 90.85% <90.47%> (+1.05%) ⬆️

... and 1 file with indirect coverage changes

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

@napetrov
napetrov force-pushed the fix/known-positive-guard branch from 1375814 to 48cbd0f Compare October 31, 2026 20:00
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 vlad-perevezentsev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review 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 claude that lists sum-fast in its init event and then returns an is_error result: the probe exits 1 with "status": "invalid: skill not visible".

Checked, no issues

  • Empty catalog (the bot's thread): fixed in 48cbd0f; an empty --catalog is refused before sorted(names)[0], and both new tests cover it.
  • Order of checks: bad input, a bad --out and an unlabelled set still exit before the paid guard run.
  • CLI snapshot and allowlist: --no-known-positive is in cli-interface.json and cli_flags_allowlist.txt; the changelog fragment is under Added.

Comment thread dokimi/dokimi/commands/catalog_probe.py Outdated
Comment on lines +129 to +134
_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")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment on lines +212 to +219
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"]}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The 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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment thread dokimi/dokimi/commands/catalog_probe.py Outdated
Comment on lines +129 to +131
_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": []})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment thread dokimi/dokimi/commands/catalog_probe.py Outdated
cost += (known or {}).get("cost_usd") or 0
out = {
"harness": "claude-code",
"status": "ok" if known else "unchecked",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment thread dokimi/docs/evaluating-treatments.md Outdated
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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copilot AI balanced review requested due to automatic review settings November 1, 2026 05:30
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>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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-positive and 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"],

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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"],

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment thread dokimi/dokimi/commands/catalog_probe.py Outdated
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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment thread dokimi/dokimi/commands/catalog_probe.py Outdated
Comment on lines +141 to +142
_write(out_path, {**_header(args, catalog, known, status, known.get("cost_usd") or 0,
known.get("claude_code_version")), "answers": []})

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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"]}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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],

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

bug · medium
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).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 antonwolfy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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-local with 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 loaded instead of loaded_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 --target all exit before the paid guard run; the fake claude was 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 --catalog does not apply: it is refused with exit 2 before the guard picks a skill.
  • Copilot's point on an errored tool_result does not apply: a Skill call whose result has is_error no 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-fast does not satisfy sum-fast.
  • Copilot's point on a listed but unloaded skill does not apply: that case now reports invalid: skill not loaded, and not visible is kept for an unlisted skill.
  • Copilot's point on the refusal output's keys does not apply: it carries max_turns_hit, not_listed and skill_selection_summary.
  • Copilot's point on a stale <out>.partial does not apply: the refusal removes it.
  • Copilot's point on a timeout after another skill's call does not apply: terminal_error is kept apart from the scoring error, and a test covers it.

"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],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A Skill call with no tool_result counts as a successful load, so the guard passes on a hang or a kill.

loaded_ok keeps every call whose id is not in failed_ids (trigger_probe.py:179), and failed_ids only collects results that arrived with is_error. A call whose result never arrived is therefore a success.

Reproduction, with the transport mocked: a stream with an init event listing sum-fast and one Skill tool_use for sum-fast, then nothing.

  • parse_stream(stream)["loaded_ok"] returns ['sum-fast'].
  • known_positive("sum-fast", ...) with timed_out=True returns passed: True, error: None. run_query computed terminal_error="timeout", but known_positive only reports it when the guard did not pass (trigger_probe.py:234-238).
  • The command then runs the labelled queries and writes status: ok, with no evidence that the target loaded.

Test gap: test_known_positive_passes_only_when_the_explicit_request_loads_the_skill uses _stream, whose tool_use has no id and no result, so it passes only because a missing result counts as success. test_known_positive_keeps_the_timeout_when_another_skill_was_called_first uses a different skill, so no test covers the target call followed by a timeout.

Suggestion: collect the ids of non-error tool_result events (ok_ids) and build loaded_ok from calls whose id is in ok_ids, dropping the tid is None case. Give _stream ids and results, and add a test for a target call followed by timed_out=True that expects passed: False and error: "timeout".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 66cb163, same change as the thread above: loaded_ok needs the call's own successful result, so a hung or killed call does not pass the guard.

Comment thread dokimi/dokimi/commands/catalog_probe.py Resolved
Comment thread dokimi/docs/evaluating-treatments.md Resolved
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Comment thread dokimi/docs/evaluating-treatments.md Outdated
`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`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

documentation · low
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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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"]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

bug · medium
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:

Suggested change
passed = skill in r["skill"]["loaded_ok"]
passed = skill in r["skill"]["loaded_ok"] and r["terminal_error"] is None

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 antonwolfy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

  • 89285d7: dokimi/dokimi/commands/catalog_probe.py:207: the --no-known-positive help now says a failed guard exits 1 with an 'invalid: ...' or 'error: ...' status.
  • e38ba84: dokimi/docs/evaluating-treatments.md:597: the docs now say status is invalid: skill not listed when not_listed is 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

documentation · low
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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cli-interface-change Changes dokimi's user-facing CLI surface

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants