diff --git a/scripts/check-cli-verbs.py b/scripts/check-cli-verbs.py index ad82a7da09..d9cc070aa8 100755 --- a/scripts/check-cli-verbs.py +++ b/scripts/check-cli-verbs.py @@ -3,16 +3,24 @@ Verify that `uip` verb literals referenced in coder-eval task YAMLs actually exist in the CLI catalog. -For every `command_executed` criterion in each task YAML, extract literal verb -tokens that follow the `uip` prefix in `command_pattern`, enumerate the -alternation paths, and check each path against `assets/uip-catalog-snapshot.json` -and `.claude/rules/cli-renames.md`. +For every `command_executed` / `command_not_executed` criterion in each task +YAML, extract literal verb tokens that follow the `uip` prefix in +`command_pattern`, enumerate the alternation paths, and check each path against +`assets/uip-catalog-snapshot.json` and `.claude/rules/cli-renames.md`. + +A path is matched leaf-first. Falling back to a shorter prefix is allowed only +when the leftover token cannot be a subcommand — a trailing flag, argument, or +partial token. A bare word sitting where a subcommand would go, under a group +whose children the catalog knows, is reported rather than passed off as the +parent group. Findings: - High — pattern does not match any verb in the catalog. The success criterion can never fire; the task scores zero on a passing run. - - Medium — pattern matches only retired verbs listed in cli-renames.md. - Suggest the canonical replacement. + - Medium — pattern matches only retired verbs listed in cli-renames.md + (suggest the canonical replacement), or a NEGATIVE criterion + targets a verb that does not exist, so it can never fire and + awards its weight unconditionally. - Info — pattern is too dynamic to analyse (contains `.`, `[`, `\\w`, etc). Skipped — no claim made about it. @@ -58,10 +66,22 @@ def load_catalog(): if not CATALOG_PATH.exists(): sys.exit(f"Catalog not found at {CATALOG_PATH}. " "Run scripts/build-uip-catalog.py first.") - data = json.loads(CATALOG_PATH.read_text()) + data = json.loads(CATALOG_PATH.read_text(encoding="utf-8")) return set(data["verbs"]), data.get("cli_version", "unknown") +def load_unwalkable(): + """Groups the catalog builder could not enumerate. + + Their children are unknown, so we must never claim a verb beneath one is + missing — absence from the catalog carries no information there. + """ + if not CATALOG_PATH.exists(): + return set() + data = json.loads(CATALOG_PATH.read_text(encoding="utf-8")) + return set(data.get("unwalkable_groups") or ()) + + def load_renames(): """ Parse `.claude/rules/cli-renames.md`. Expected format: a markdown table @@ -71,7 +91,7 @@ def load_renames(): renames = {} if not RENAMES_PATH.exists(): return renames - for line in RENAMES_PATH.read_text().splitlines(): + for line in RENAMES_PATH.read_text(encoding="utf-8").splitlines(): if not line.startswith("|"): continue cells = [c.strip() for c in line.strip("|").split("|")] @@ -169,9 +189,19 @@ def enumerate_paths(parsed, allow_partial=True): return _trim_to_word_boundary(paths) if allow_partial else None elif op == sre_parse.AT: # `^` / `$` / `\A` / `\Z` are positional anchors that don't add - # text — safe to skip. `\b` / `\B` are context-dependent and can - # forbid the surrounding literal from matching; treat as dynamic. - if args in (sre_parse.AT_BOUNDARY, sre_parse.AT_NON_BOUNDARY): + # text — safe to skip. + if args == sre_parse.AT_BOUNDARY: + # `\b` directly after a literal word character *confirms* that + # token ended — it is the opposite of a partial match. Trimming + # here discarded the leaf verb of every `uip \b` + # pattern (the repo's standard shape), so only the group was + # ever verified. Keep the accumulated literal intact. + if all(p and p[-1].isalnum() for p in paths): + return paths if allow_partial else None + return _trim_to_word_boundary(paths) if allow_partial else None + if args == sre_parse.AT_NON_BOUNDARY: + # `\B` asserts the token continues into more word characters, + # so what we accumulated really is mid-token. return _trim_to_word_boundary(paths) if allow_partial else None continue elif op == sre_parse.IN: @@ -218,26 +248,54 @@ def extract_verb_paths(pattern): return verb_paths or None -def classify(verb_paths, catalog, renames): +def classify(verb_paths, catalog, renames, unwalkable=None): """Return ('reachable'|'retired'|'unknown', details).""" if not verb_paths: return "unknown", {} + unwalkable = unwalkable or set() + + def has_children(prefix, lookup): + depth = len(prefix.split()) + 1 + return any(v.startswith(prefix + " ") and len(v.split()) == depth + for v in lookup) + + def under_unwalkable(verb): + parts = verb.split() + return any(" ".join(parts[:i]) in unwalkable + for i in range(1, len(parts) + 1)) # Try progressively shorter prefixes — `solution project add --foo` should # match the catalog entry `solution project add` even when the regex # captured a trailing flag fragment. - def best_match(verb, lookup): + # + # But shortening must not swallow a bogus SUBCOMMAND. If the prefix we + # landed on is a group whose children the catalog knows, and the very next + # token is not one of them, that token is a verb that does not exist — + # report it instead of silently passing on the parent group. (This is how + # `pm apps model add-table` — no such verb; it is `pm apps data-model + # add-table` — lint-passed as the group `pm apps model`.) + def best_match(verb, lookup, strict=False): parts = verb.split() for i in range(len(parts), 0, -1): candidate = " ".join(parts[:i]) - if candidate in lookup: - return candidate + if candidate not in lookup: + continue + if strict and i < len(parts) and not under_unwalkable(candidate): + nxt = parts[i] + # A trailing `-` means the walker stopped mid-kebab-token + # (e.g. `create-` from a pattern matching `create-raw` / + # `create-resource`), so we cannot claim the verb is missing. + looks_like_subcommand = bool( + re.fullmatch(r"[a-z][a-z0-9]*(-[a-z0-9]+)*", nxt)) + if looks_like_subcommand and has_children(candidate, lookup): + return None + return candidate return None reachable = [] retired = [] for v in verb_paths: - cat_hit = best_match(v, catalog) + cat_hit = best_match(v, catalog, strict=True) ren_hit = best_match(v, renames) # A more specific renames entry shadows a shallower catalog prefix. # Example: `solution new` was removed in 1.2.0; the parent group @@ -262,22 +320,30 @@ def iter_command_patterns(spec, path): for idx, crit in enumerate(spec.get("success_criteria") or []): if not isinstance(crit, dict): continue - if crit.get("type") != "command_executed": + # `command_not_executed` needs the same check: a negative criterion + # whose verb does not exist can never fire, so it awards its weight + # unconditionally — a silent free pass rather than a graded assertion. + if crit.get("type") not in ("command_executed", "command_not_executed"): continue if crit.get("tool_name", "Bash") != "Bash": continue pattern = crit.get("command_pattern") if not isinstance(pattern, str): continue - yield idx, pattern, crit.get("description", "") + # A criterion is a negative either by type, or by being a + # `command_executed` bounded to zero matches (`max_count: 0`) — both + # idioms appear in this repo and both assert "never ran". + is_negative = (crit["type"] == "command_not_executed" + or crit.get("max_count") == 0) + yield idx, pattern, crit.get("description", ""), is_negative -def lint_file(path, catalog, renames): +def lint_file(path, catalog, renames, unwalkable=None): try: import yaml except ImportError: sys.exit("PyYAML is required. Install with: pip install pyyaml") - text = path.read_text() + text = path.read_text(encoding="utf-8") try: spec = yaml.safe_load(text) except yaml.YAMLError as exc: @@ -289,7 +355,7 @@ def lint_file(path, catalog, renames): if not isinstance(spec, dict): return [] findings = [] - for idx, pattern, desc in iter_command_patterns(spec, path): + for idx, pattern, desc, is_negative in iter_command_patterns(spec, path): verbs = extract_verb_paths(pattern) if verbs is None: findings.append({ @@ -301,7 +367,7 @@ def lint_file(path, catalog, renames): "class / quantifier). Skipped.", }) continue - verdict, details = classify(verbs, catalog, renames) + verdict, details = classify(verbs, catalog, renames, unwalkable) if verdict == "reachable": continue if verdict == "retired": @@ -316,6 +382,23 @@ def lint_file(path, catalog, renames): + ", ".join(f"`{r}` → `{sugg[r]}`" for r in details["retired"]), }) + elif is_negative: + # A negative whose verb does not exist can never fire, so it awards + # its weight on every run — a dead assertion that inflates scores + # rather than a criterion that breaks the task. Sometimes that is + # deliberate (guarding against a verb an agent might invent), so + # this is Medium, not High. + findings.append({ + "path": str(path), "severity": "Medium", + "axis": "cli-verb-reachability", + "criterion_index": idx, + "command_pattern": pattern, + "description": desc, + "message": "Negative criterion targets a verb absent from the " + f"uip catalog (unmatched: {details['unmatched']}). " + "It can never fire, so its weight is awarded " + "unconditionally — confirm that is intended.", + }) else: findings.append({ "path": str(path), "severity": "High", @@ -416,13 +499,14 @@ def main(): catalog, version = load_catalog() renames = load_renames() + unwalkable = load_unwalkable() all_findings = [] for p in args.paths: if not p.exists(): print(f"skip: {p} (not found)", file=sys.stderr) continue - all_findings.extend(lint_file(p, catalog, renames)) + all_findings.extend(lint_file(p, catalog, renames, unwalkable)) if args.report: write_report(all_findings, len(catalog), version, args.report) diff --git a/tests/scripts/test_verb_checkers.py b/tests/scripts/test_verb_checkers.py index ce768bdc22..dc48479cc6 100644 --- a/tests/scripts/test_verb_checkers.py +++ b/tests/scripts/test_verb_checkers.py @@ -266,6 +266,110 @@ def test_backticked_rename_cells_strip_to_bare_verbs(tmp_path, monkeypatch): ) +# --- Issue 15 (High): `\b` after a complete token discarded the leaf verb ----- + +def test_word_boundary_after_complete_token_keeps_leaf(): + r"""`uip\s+pm\s+apps\s+data-model\s+add-table\b.*--file\b` must yield the + FULL verb path, not just the group `pm apps data-model`. + + `\b` following a literal word character confirms the token ended — the + opposite of a partial match. Trimming it dropped the leaf verb from every + `uip \b` pattern, which is the repo's standard shape, so the + checker only ever verified the command group. + """ + paths = cli.extract_verb_paths( + r"uip\s+pm\s+apps\s+data-model\s+add-table\b.*--file\b") + assert paths == ["pm apps data-model add-table"], ( + f"Expected the leaf verb to survive `\\b`, got {paths!r}." + ) + + +def test_bogus_leaf_under_real_group_is_unknown(): + """`pm apps model add-table` must NOT pass via the parent group. + + `pm apps model` is a real group (`fields`/`get`/`update`), but it has no + `add-table` child — that verb is `pm apps data-model add-table`. Longest- + prefix matching silently reported the bogus leaf as reachable, so the + criterion could never fire and the task lost its weight on a correct run. + """ + catalog = {"pm apps model", "pm apps model fields", "pm apps model get", + "pm apps data-model", "pm apps data-model add-table"} + verdict, _ = cli.classify(["pm apps model add-table"], catalog, {}) + assert verdict == "unknown", ( + f"Expected 'unknown' for a leaf absent under a known group, got {verdict!r}." + ) + verdict, _ = cli.classify(["pm apps data-model add-table"], catalog, {}) + assert verdict == "reachable" + + +def test_positional_args_after_leaf_verb_stay_reachable(): + """Trailing positional arguments must not be mistaken for subcommands. + + `is resources run list` is a leaf (no children), so `uipath-testmanager` + and `GetAssertions` are arguments — flagging them would be a false positive. + """ + catalog = {"is resources run", "is resources run list"} + verdict, _ = cli.classify( + ["is resources run list uipath-testmanager GetAssertions"], catalog, {}) + assert verdict == "reachable" + + +def test_partial_kebab_token_is_not_reported_missing(): + """A path ending mid-kebab (`create-`) came from a prefix match over real + verbs (`create-raw`, `create-resource`) — we cannot claim it is missing.""" + catalog = {"agenthub mcp-tools", "agenthub mcp-tools create-raw", + "agenthub mcp-tools create-resource"} + verdict, _ = cli.classify(["agenthub mcp-tools create-"], catalog, {}) + assert verdict == "reachable" + + +def test_unwalkable_group_never_reports_missing_leaf(): + """Children of an unwalkable group are unknown, so absence proves nothing.""" + catalog = {"codedagent", "codedagent init"} + verdict, _ = cli.classify(["codedagent teleport"], catalog, {}, + unwalkable={"codedagent"}) + assert verdict == "reachable" + + +# --- Issue 16 (Medium): negative criteria were never verb-checked ------------- + +def test_negative_criteria_are_scanned_and_reported_medium(tmp_path, monkeypatch): + """A `command_not_executed` whose verb does not exist can never fire, so it + awards its weight unconditionally. Previously such criteria were skipped + entirely. They are reported at Medium (sometimes the guard is deliberate), + never High — they do not break the run. + """ + monkeypatch.setattr(cli, "load_catalog", lambda: ({"pm apps", "pm apps get"}, "test")) + monkeypatch.setattr(cli, "load_renames", lambda: {}) + monkeypatch.setattr(cli, "load_unwalkable", lambda: set()) + + task = tmp_path / "task.yaml" + task.write_text( + "success_criteria:\n" + " - type: command_not_executed\n" + " command_pattern: 'uip\\s+pm\\s+apps\\s+teleport\\b'\n" + ) + findings = cli.lint_file(task, {"pm apps", "pm apps get"}, {}, set()) + assert len(findings) == 1, f"Expected the negative to be scanned, got {findings!r}" + assert findings[0]["severity"] == "Medium", findings[0] + + +def test_zero_bounded_command_executed_counts_as_negative(tmp_path): + """`command_executed` with `max_count: 0` is the other negative idiom in + this repo; it must get the same Medium treatment, not High.""" + task = tmp_path / "task.yaml" + task.write_text( + "success_criteria:\n" + " - type: command_executed\n" + " command_pattern: 'uip\\s+pm\\s+apps\\s+teleport\\b'\n" + " min_count: 0\n" + " max_count: 0\n" + ) + findings = cli.lint_file(task, {"pm apps", "pm apps get"}, {}, set()) + assert len(findings) == 1 + assert findings[0]["severity"] == "Medium", findings[0] + + # --- Issue 12: dot-separated argument values are not verb tokens -------------- def test_dot_separated_argument_value_not_treated_as_verb(tmp_path): diff --git a/tests/tasks/uipath-process-mining/add_table_queryable.yaml b/tests/tasks/uipath-process-mining/add_table_queryable.yaml index b5984a49a1..cd57f999e3 100644 --- a/tests/tasks/uipath-process-mining/add_table_queryable.yaml +++ b/tests/tasks/uipath-process-mining/add_table_queryable.yaml @@ -1,7 +1,7 @@ task_id: skill-pm-add-table-queryable description: > Critical Rule 1: to make a custom analytical dbt model queryable, register it - as a Case-linked data-model table with `apps model add-table` and RE-INGEST. + as a Case-linked data-model table with `apps data-model add-table` and RE-INGEST. Tests the headline rule of the skill and the mistake it exists to prevent — repurposing the pre-registered `Tags` / `Due_dates` entities to smuggle an unrelated aggregate through, which corrupts those features and fights their @@ -13,7 +13,7 @@ description: > Command-construction only — no live tenant. Commands fail with auth errors; what matters is which commands the agent chooses. -tags: [uipath-process-mining, mode:build, lifecycle:extend, data-model, add-table] +tags: [uipath-process-mining, integration, mode:build, lifecycle:setup, data-model, add-table] run_limits: expected_turns: 8 @@ -49,7 +49,7 @@ success_criteria: - type: command_executed description: "Agent registered the custom model as a data-model table" tool_name: "Bash" - command_pattern: 'uip\s+pm\s+apps\s+model\s+add-table\b.*--file\b' + command_pattern: 'uip\s+pm\s+apps\s+data-model\s+add-table\b.*--file\b' min_count: 1 weight: 3.0 pass_threshold: 1.0 diff --git a/tests/tasks/uipath-process-mining/app_types_discovery_smoke.yaml b/tests/tasks/uipath-process-mining/app_types_discovery_smoke.yaml index d664063aab..cfaaead434 100644 --- a/tests/tasks/uipath-process-mining/app_types_discovery_smoke.yaml +++ b/tests/tasks/uipath-process-mining/app_types_discovery_smoke.yaml @@ -8,7 +8,7 @@ description: > Platform note: runs without an authenticated tenant — commands will fail with auth errors. That is acceptable; what matters is that the agent loads the skill and invokes the right discovery command. -tags: [uipath-process-mining, smoke, mode:diagnose, discover, app-types] +tags: [uipath-process-mining, smoke, mode:diagnose, lifecycle:discover, app-types] run_limits: expected_turns: 4 diff --git a/tests/tasks/uipath-process-mining/mapping_fix_in_place.yaml b/tests/tasks/uipath-process-mining/mapping_fix_in_place.yaml index dd16c89e7c..1b679a0442 100644 --- a/tests/tasks/uipath-process-mining/mapping_fix_in_place.yaml +++ b/tests/tasks/uipath-process-mining/mapping_fix_in_place.yaml @@ -16,7 +16,7 @@ description: > to edit when the unauthenticated `data-mapping get` cannot return one — without it the agent correctly refuses to push a garbage file and the run stalls after the `get`. -tags: [uipath-process-mining, mode:fix, lifecycle:maintain, custom, data-mapping] +tags: [uipath-process-mining, integration, mode:diagnose, lifecycle:setup, custom, data-mapping] run_limits: expected_turns: 8 diff --git a/tests/tasks/uipath-process-mining/query_group_by_sugar.yaml b/tests/tasks/uipath-process-mining/query_group_by_sugar.yaml index 4fccadda11..1a1fa22ea9 100644 --- a/tests/tasks/uipath-process-mining/query_group_by_sugar.yaml +++ b/tests/tasks/uipath-process-mining/query_group_by_sugar.yaml @@ -8,7 +8,7 @@ description: > Command-construction only — no live tenant. Commands fail with auth errors; what matters is the shape of the query the agent constructs. -tags: [uipath-process-mining, mode:diagnose, query, aggregate] +tags: [uipath-process-mining, smoke, mode:diagnose, lifecycle:discover, query, aggregate] run_limits: expected_turns: 6 diff --git a/tests/tasks/uipath-process-mining/transform_fix_apply_not_reingest.yaml b/tests/tasks/uipath-process-mining/transform_fix_apply_not_reingest.yaml index 8f58452f9a..f46fab367d 100644 --- a/tests/tasks/uipath-process-mining/transform_fix_apply_not_reingest.yaml +++ b/tests/tasks/uipath-process-mining/transform_fix_apply_not_reingest.yaml @@ -13,7 +13,7 @@ description: > Command-construction only — no live tenant. Commands fail with auth errors; what matters is which commands the agent chooses. -tags: [uipath-process-mining, mode:fix, lifecycle:maintain, transformations, custom] +tags: [uipath-process-mining, integration, mode:diagnose, lifecycle:setup, transformations, custom] run_limits: expected_turns: 8