cf-ux: pin the skill-name matcher's false-positive trade (follow-up to #161) - #171
Conversation
Review follow-up to constructorfabric#161, which merged before this landed. `_invoked_names` compares every identifier-shaped value in a `Skill` tool input, because which key holds the skill name is not part of any stable contract. Prose beside the name is excluded by shape, and that case had a test — but a single bare token in an unrelated field is not, so a rival skill invoked as `{"command": "superpowers:brainstorming", "mode": "cf"}` reads as this skill running. Nothing pinned that. Pinned rather than closed, with the reasoning in the docstring. Narrowing to a fixed set of name keys would close this hole and open a worse one: guess the key wrong and every run errors, because the name would never be found where it actually lives. A rare false pass beats a certain false failure here, and `skill_call_inputs` carries the raw input so the verdict stays inspectable. If the key is ever pinned down, that test is where the trade gets renegotiated. Second test covers the other side: the comparison stays whole-value in every field, so `mode: "cf-generate"` still does not match. Two mutations, two caught. Narrowing the matcher to named keys fails the first test and nothing else — which is what made this worth a test. Signed-off-by: ou <ou@constructor.tech>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe Claude provider now scans nested and non-dictionary tool inputs for skill identifiers. It reports additional candidate identifiers in warnings and result metadata. Tests and README content define the updated matching behavior. ChangesClaude skill matching
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Other Sequence Diagram(s)sequenceDiagram
participant ClaudeProvider
participant SkillInputScanner
participant ResultMetadata
ClaudeProvider->>SkillInputScanner: scan tool input for identifiers
SkillInputScanner-->>ClaudeProvider: return matched skill and other candidates
ClaudeProvider->>ResultMetadata: record skill state and candidate metadata
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The matcher changes are covered by the documented behavior and regression tests with no unresolved merge-blocking risk. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
code-ranker report for this PR (built on fork): https://reports.code-ranker.com/0RRHRCy4m8OYGvQxqGXc5g/ |
README omits the newly pinned bare-token false-positive tradeSeverity: Minor Problem Reproduction, impact, suggested fix, verificationHow to reproduce
Expected behavior Actual behavior Impact Suggested correction How to verify Original location: tests/prompts/cf-ux/README.md:65 -- inline anchoring could not be resolved after 1 attempt(s). |
| }}, | ||
| ]}} | ||
|
|
||
| out, _seen = run_provider(_stream(rival, _skill_result(), _result())) |
There was a problem hiding this comment.
Is the false positive ever surfaced, or only inspectable on demand?
Non-blocking review challenge -- [product]
The docstring says skill_call_inputs 'keeps the raw input so the verdict stays inspectable' — but is anything in the harness or its reporting actually surfacing/flagging these false-pass cases, or does catching one require a human to manually diff skill_call_inputs after the fact? If nobody's expected to look, this is a silent metric-inflation risk for whatever downstream benchmark consumes skill_state/skills_invoked.
Why this is worth asking
Pinning a known false positive is reasonable, but 'inspectable' only mitigates the risk if someone is actually inspecting. Since this measures skill-invocation success for evaluation purposes, a rare-but-real false pass that goes unnoticed silently overstates the skill's reliability.
There was a problem hiding this comment.
Fair hit, and fixed in 7303349e — the honest answer to your question as asked was no, nothing surfaced it. "Inspectable" was doing real work in that docstring, and it should not have been.
You are right about the shape of the risk too: this feeds an evaluation, so a rare-but-real false pass that nobody notices overstates skill reliability rather than merely being untidy. Left as-is, the mitigation was "someone diffs skill_call_inputs after the fact," which is not a mitigation, it is a hope.
So the false positive now reports itself. The signature is specific: a ran verdict that rests on one of several candidate identifiers in the matched call — which is exactly the shape a false pass takes, since a genuine call names cf and little else (claude_provider.py:229-241):
others = [name for name in named[call_id] if name != _SKILL_NAME]
if others:
logger.warning(_LOG_AMBIGUOUS_MATCH, _SKILL_NAME, others)
return _SkillTrace("ran", names, inputs, "", bool(others))Surfaced two ways per run, not on demand:
metadata["skill_match_ambiguous"]— so whatever consumesskill_statecan filter or flag these instead of counting them blind. That is the downstream-benchmark concern you raised.- a stderr warning naming the other candidate — so it is visible in the run output itself, following the same house rule the dropped-line warning follows (
architecture/DESIGN.md:277).
Deliberately a flag and not a verdict change: it does not know that the match is wrong, only that it could not tell which value was the name, so turning it into an error would manufacture failures on inputs like {"command": "cf", "mode": "auto"}. It does mean a false pass is now countable rather than merely discoverable.
Tests: test_an_ambiguous_match_is_surfaced_not_merely_inspectable (:514) asserts both the flag and the warning text, and test_an_unambiguous_match_is_not_flagged (:220) asserts the clean case is False — a flag that never distinguishes says nothing. Two mutations (flag hard-wired to False; warning removed), both caught, one test each.
Also written into the README as part of the documented trade rather than left in a docstring.
…biguous
Review on the pinned trade found the code did not implement the argument
made for it.
The trade is recall over precision: because the name's key is unknown, every
identifier-shaped value is a candidate, accepting a rare false pass to avoid
a certain false failure. But the scan stopped at the top level, so an input
nesting its identifier one level down — {"options": {"skill": "cf"}} — found
nothing and every run would error. That is the failure the trade exists to
avoid, sitting inside its own implementation. The scan now descends through
dicts and lists.
The other half of the review: "inspectable" only mitigates a false positive
if someone inspects, and nothing surfaced these. A `ran` verdict resting on
one of several candidate identifiers in the matched call is the shape a false
pass takes, so it now reports itself — `skill_match_ambiguous` in the
metadata plus a stderr warning naming the other candidate — instead of
waiting for someone to diff `skill_call_inputs` after the fact.
Five test gaps from the same review, all real:
- the false-positive match combined with an errored tool_result (the loose
match buys a name, not a verdict)
- a namespaced value in an unrelated field, which reaches the same false
positive through the separator-stripping path
- the positive test never asserted an answer was actually delivered, which
is the whole point of the false positive
- the negative test never pinned `skills_invoked`, so silently dropping one
candidate would have passed
- no test distinguished an unambiguous match from an ambiguous one
README documents the accepted trade, both mitigations, and which test pins
it — it previously listed only the false positives that were closed.
7 mutations, 7 caught. Reverting to the top-level-only scan fails the new
nested test and nothing else.
Signed-off-by: ou <ou@constructor.tech>
README omits the pinned trade — fixed in
|
ainetx
left a comment
ainetx
left a comment
There was a problem hiding this comment.
Clean change overall — the traversal rewrite and the new ambiguity flag are both sound, well-motivated by the docstring/README trade-off writeup, and covered by a solid set of tests for the core false-positive scenarios. A few minor gaps worth a glance before they bite someone later, none blocking:
- Non-dict top-level
inputnow silently scanned — the old guard that rejected a non-dictSkillinputoutright is gone; a bare list or stringinputnow feeds the traversal same as nested values, but this isn't mentioned in the docstring or exercised by a test. (comment) - "Any depth" claim only tested at depth one — the docs promise traversal at arbitrary nesting, but the only new test covers a single level of dict nesting; two-level or mixed list/dict nesting isn't pinned. (comment)
skill_match_ambiguousoverfires on unrelated co-occurring identifiers — it flags any second identifier-shaped value in the input, even one that has nothing to do with the actual name field, so a genuinely correct match can get the same flag as a real false positive. (comment)- Ambiguous-warning content untested for duplicates/multiples — the
otherslist has no dedup and its order depends on LIFO traversal, but only the single-other-candidate case is asserted. (comment)
…pin the walk
Four review findings, all valid.
The ambiguity flag claimed more than the signal supports. It fired whenever a
matched call's input named a second identifier, which cannot distinguish a
wrong-field match from a correct call that merely carries one — telling those
apart needs the very knowledge whose absence created the trade. So it is now
`skill_match_other_candidates`, a list naming what was observed, rather than a
boolean asserting a suspicion. Documented as the over-approximation it is: a
genuine `{"command": "cf", "mode": "auto"}` is listed too, and the run is still
scored.
That list reaches a warning a person reads, and it was neither deduplicated nor
ordered — the traversal is a stack, so a value appearing twice was repeated and
the rest came out in an implementation-detail order. Two runs of the same
transcript could produce different prose for the same finding. `_invoked_names`
now returns its names sorted and deduplicated.
Dropping the `isinstance(payload, dict)` guard was a real boundary change made
in passing: a bare string or list input is now scanned rather than refused.
Kept, because it follows from the same reasoning as the depth walk — an input
shape that cannot be ruled out must not go unscanned, or the name is never found
and every run errors — but now stated in the docstring, the README, and a test,
instead of being a side effect nobody declared.
The "any depth" claim was pinned only at depth one. Parametrized over two
levels, a list of dicts, a dict inside a list inside a dict, and nested lists.
6 mutations, 6 caught. The precise one: traversal that works at depth one but
stops below it fails four of the five depth cases and leaves `one-level`
passing, which is what makes the parametrization worth having.
Signed-off-by: ou <ou@constructor.tech>
…r text The negative tests asserted the absence of output, a detail substring, and sometimes the parsed names — never the state label itself. That label is what downstream reads, and it was free to be wrong. Demonstrated rather than assumed: mislabelling the none-named branch "absent" while leaving its detail text correct passed all 52 tests. The error string still read sensibly, because it interpolates whatever state it was handed. `skill_state` is now asserted on every negative case, one per branch of the ladder. Mutating each of the five labels in turn — including the "absent" branch in the other direction — is caught, each by the tests that exercise that branch. Signed-off-by: ou <ou@constructor.tech>
|
3 participants
Review follow-up to #161, which merged before this could land on it.
One of the two findings on that PR was a genuine test gap. This is the test.
The gap
_invoked_namescompares every identifier-shaped string value in aSkilltool input, because which key holds the skill name is not part of any stable contract. Multi-word prose is excluded by shape (_NAME_SHAPE), and that case had a test. A single bare token in an unrelated field is not excluded, so:{"command": "superpowers:brainstorming", "mode": "cf"}reads as the
cfskill running. Reproduced before writing anything:Pinned, not closed — and why
Narrowing the match to a fixed set of name keys (
command,skill,name, …) would close this and open a worse hole. Guess the key wrong and every run errors, because the name would never be found where it actually lives. I do not know the real key: the fixtures in this suite use{"command": …}, which I chose, not something I measured —claudeis not installed in my environment, as stated on #161.So the trade is deliberate, and it errs the way that keeps the harness working without that knowledge:
cffieldskill_call_inputscarries the raw input verbatim, so aranverdict reached this way is inspectable — a maintainer sees{"command": "superpowers:brainstorming", "mode": "cf"}and spots it. That field exists for exactly this.If the real key is ever pinned down by someone who can run the CLI, the new test is where the trade gets renegotiated — narrowing the matcher fails it, loudly, with the reasoning in the docstring.
Tests
test_a_bare_cf_in_an_unrelated_field_still_countstest_a_single_token_that_is_not_cf_does_not_count_eithermode: "cf-generate"does not matchTwo mutations, two caught:
_invoked_namesto a fixed set of name keysThat first row is the finding restated as a measurement: before this commit, the matcher's scope could be changed and the suite would not notice.
Review round 2 (
7303349e)Six findings and a README gap. All actioned — and one of them found a contradiction between the argument above and the code implementing it.
The scan stopped at the top level. The trade is recall over precision, accepting a rare false pass to avoid a certain false failure. But
{"options": {"skill": "cf"}}found nothing, so an input that nests its identifier one level down would make every run error while reportingabsent— the exact failure the trade exists to avoid, inside its own implementation. The docstring was not overstating the policy; the code was under-delivering it._invoked_namesnow walks dicts and lists to any depth, iteratively so transcript nesting cannot reach a recursion limit."Inspectable" was doing work it had not earned. Asked whether anything actually surfaces these false passes, the honest answer was no — the mitigation was "someone diffs the metadata afterwards," which is a hope, not a mitigation. A
ranverdict resting on one of several candidate identifiers in the matched call is the shape a false pass takes, so it now reports itself:metadata["skill_match_ambiguous"]plus a stderr warning naming the other candidate. A flag, not a verdict change — it knows it could not tell which value was the name, not that the match is wrong — but a false pass is now countable by whatever consumesskill_state, which was the risk raised.Four test gaps, all real:
tool_resultplugin:cf), the same false positive through the separator-stripping pathassert out["output"] == "the answer"skills_invoked, so silently dropping a candidate would passREADME documented the false positives that were closed and was silent on the one left open. Now carries the trade, both mitigations, the unverified-CLI caveat, and which test pins it.
7 mutations, 7 caught. Reverting to the top-level-only scan fails the new nested test and nothing else.
Review round 3 (
f75c71dc)Four findings. Five of round 2's six were independently re-verified as resolved; the sixth was accepted as answered.
skill_match_ambiguousfired whenever a matched call named a second identifier, which cannot distinguish a wrong-field match from a correct call that merely carries one. I had conceded exactly that on a thread and then shipped the assertion anyway.skill_match_other_candidates— a list of what was observed, not a boolean asserting a suspicion. Narrowing it is impossible by construction: it would need the key whose absence created the trade. Documented as an over-approximation; the run is still scored._invoked_namesreturnssorted(set(...)), so traversal order becomes an implementation detail nothing depends on. Test asserts the rendered warning text, not just the list.isinstance(payload, dict)guard was an undeclared boundary change — a bare string or list input is now scanned rather than refused.6 mutations, 6 caught. The precise one for the last row: a traversal depth-limited to one level leaves
one-levelpassing and fails the other four — the regression as described, which the previous single test would have missed.Review round 4 (
fa829c10)One finding: the negative tests asserted the absence of output and a detail substring, never the
skill_statelabel itself — the field downstream actually reads.Checked before fixing, and it was exploitable: mislabelling the none-named branch
"absent"while leaving its detail text correct passed all 52 tests. The error string interpolates whatever state it is handed (f"cf skill did not run ({state}): {detail}"), so asserting the detail proves nothing about the label.skill_stateis now asserted on every negative case, one per branch of the ladder. 5 mutations, 5 caught — each label swapped in turn, including theabsentbranch mutated in the opposite direction so it is pinned asabsentrather than as "not the default".Scope
tests/prompts/is outside the traced population, so no@cptobligation.make testcfs validatespec-coverage --system studioThe other finding on #161, and one I found while answering it
The second finding was an architecture challenge — should the skill-run verdict live in the provider or in a promptfoo assertion? Answered on its thread rather than here, since it is a design question and non-blocking.
While answering it I found something worth a maintainer's ruling, in
promptfooconfig.yaml:Three scenarios each assert the absence of strings the CLI does not emit — the assertion-level twin of the
skill_load_warningguard #161 removed from the provider, inert for the same reason. Now that the provider returns an error on the real signature before any assertion runs, these are dead weight rather than a second line of defence.I have not touched them: deleting per-scenario assertions is a behaviour change on a shared config, and the disposition is a maintainer call — delete, or repoint at something that can actually fire. Say which and I will open it.
Summary by CodeRabbit
Enhancements
Documentation