From 3dbee8f4aa5f8b9a3de4fe0508681cbec13b27a7 Mon Sep 17 00:00:00 2001 From: MsNBBc3R <152378630+MsNBBc3R@users.noreply.github.com> Date: Sun, 18 Oct 2026 20:44:39 +0800 Subject: [PATCH] fix(skill-creator): preserve recorded token counts in benchmarks (#7033) * fix(skill-creator): preserve recorded token counts in benchmarks Read timing.json token measurements independently of grading duration and retain recorded zero values. Keep the existing character-count fallback for missing measurements and add benchmark/report regression coverage. * fix(skillscan): distinguish token metrics from credentials Recognize typed token counts, numeric benchmark summaries, and signed deltas so skill-creator documentation passes deterministic review. Preserve credential names, string values, duplicate-key detection, and cloud-token findings; add regression coverage and document the boundary. --------- Co-authored-by: Willem Jiang --- README.md | 6 + .../harness/deerflow/skills/AGENTS.md | 2 + .../deerflow/skills/skillscan/orchestrator.py | 47 +++++++ ...est_skillscan_secret_assignment_formats.py | 61 +++++++++ .../skill-creator/references/schemas.md | 1 + .../scripts/aggregate_benchmark.py | 12 +- tests/skills/test_skill_creator_benchmark.py | 119 ++++++++++++++++++ 7 files changed, 243 insertions(+), 5 deletions(-) create mode 100644 tests/skills/test_skill_creator_benchmark.py diff --git a/README.md b/README.md index 08b2dac968..e5ac3d81dd 100644 --- a/README.md +++ b/README.md @@ -1465,6 +1465,10 @@ Discovery follows operator-managed directory symlinks, but skips links back to a Skill Markdown and bundled text resources use UTF-8. Skill-creator CLI and review utilities read and write text explicitly as UTF-8 so localized skills behave consistently across operating systems. +Skill-creator benchmark aggregation uses recorded `timing.json` token counts even +when `grading.json` already contains the run duration. Recorded zero counts are +preserved; output character counts remain a legacy proxy only when token data is missing. + `SKILL.md` YAML frontmatter must open and close with `---` on its own line (trailing spaces or tabs are allowed). A line such as `---suffix` is not a closing delimiter. Custom skill history preserves Unicode line separators inside saved content and metadata, keeping those revisions readable for history inspection and rollback. Malformed JSON history records are still rejected. @@ -1626,6 +1630,8 @@ SkillScan treats HTTP/HTTPS scheme spellings and HTTP hostnames case-insensitive loopback (`[::1]`) URLs as local. External IPv6 endpoints still trigger network findings, and cloud-metadata hostname detection is case-insensitive. +SkillScan recognizes unquoted integer model-usage counters such as `total_tokens` and `input_tokens` in skill examples as measurements. Benchmark token statistics and signed deltas are also recognized; credential fields and other string values remain subject to secret detection. + For Python credential mappings, SkillScan checks literal values while treating ordinary dictionary keys as labels. A mapping such as `tokens = {"access_token": os.getenv("ACCESS_TOKEN")}` does not report a hardcoded credential. Keys matching a recognized cloud or API token format are still checked as embedded credentials. SkillScan blocks embedded legacy and modern `sk-` API tokens, including OpenAI project, service-account, and admin keys and Anthropic keys, even in Bearer headers without a secret assignment. Token bodies may contain `-` and `_`; finding evidence is fully redacted. Placeholder examples do not suppress detection of later credentials in the same file. Scheduled-task notification summaries also redact these key formats before sending them to external IM platforms. diff --git a/backend/packages/harness/deerflow/skills/AGENTS.md b/backend/packages/harness/deerflow/skills/AGENTS.md index 8048b45b9b..ce3f11478c 100644 --- a/backend/packages/harness/deerflow/skills/AGENTS.md +++ b/backend/packages/harness/deerflow/skills/AGENTS.md @@ -6,6 +6,8 @@ - **Diagnostic scope**: only `yaml.YAMLError` produces a management diagnostic. Missing frontmatter, non-mapping or invalid metadata, non-UTF-8 files and read errors (including permission denied) remain excluded; the UI and README state those limits. An empty diagnostic list is not a package-validity check. `tests/test_skill_load_diagnostics.py` pins these exclusions and permission-error redaction without changing runtime parsing. - **History JSONL**: split records on LF, not Unicode text separators; preserve U+0085/U+2028/U+2029 in saved fields. Keep malformed JSON rejected. Tests: `tests/test_skill_history_jsonl.py`. +- **SkillScan usage counters**: the text assignment sweep recognizes unquoted nonnegative integers under the exact model-usage names `tokens`, `total_tokens`, `input_tokens`, `output_tokens`, `prompt_tokens`, `completion_tokens`, `cached_tokens`, and `reasoning_tokens`. Benchmark `tokens` also accepts a signed numeric delta or an object containing exactly four nonnegative finite numeric statistics (`mean`, `stddev`, `min`, `max`); JSON parsing is capped at 512 characters. Recover the complete key before applying the exception. Quoted unsigned counts, credential prefixes, arbitrary containers, and cloud-token detection remain active. Regressions live in `tests/test_skillscan_secret_assignment_formats.py`. + - **SkillScan API tokens**: `secret-cloud-token` accepts `-` and `_` in the `sk-` family, matching the character class used by PII redaction. Legacy, OpenAI project/service-account/admin, and Anthropic keys must trigger `CRITICAL` even in a Bearer header without a secret assignment. Skip placeholder matches until a real credential is found, retaining at most one cloud-token finding per file. Keep evidence fully redacted and preserve the minimum length and word boundaries; synthetic regressions live in `tests/test_skillscan_native.py`. Keep the mirrored `sk-` pattern in `app/scheduler/notification_text.py` aligned so scheduled-run summaries scrub the same key formats before IM delivery. - **SkillScan shell pipes**: resolve `env` launchers through interpreter paths and `command`/`exec`/`sudo` wrappers, including value-taking sudo flags. Preserve direct-pipe coverage of common shell interpreter paths/names and real backslash-newline continuations; keep repeated regex alternatives disjoint so failed matches on literal backslash text cannot stall scanning. Tokenize only the current simple command, in place, under a shared per-file work budget; only its stdin redirections override the pipe. Keep quoted words distinct from operators. Parse GNU `env -S` escapes separately from shell syntax, with iterative expansion capped at 256 splits/65,536 characters. Unknown runtime expansions remain unresolved; never read the scanner environment. Limit exhaustion retains warnings and already collected critical findings. Tests: `test_skillscan_env_review_regressions.py`, `test_skillscan_env_launcher.py`, and the existing native/shell suites. diff --git a/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py b/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py index aa0b48f5e6..8d693f32a4 100644 --- a/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py +++ b/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py @@ -12,7 +12,9 @@ from __future__ import annotations import ast import io +import json import logging +import math import posixpath import re import stat @@ -126,6 +128,8 @@ _PLACEHOLDER_VALUES = {"", "x", "xx", "xxx", "xxxx", "changeme", "change-me", "e # word (`tokenizer`, `secretive`) still stays quiet, so this is not a blanket match. _SECRET_ASSIGNMENT_RE = re.compile(r"(?im)(? list[SecurityF """``name[:=]value`` sweep for line-oriented text, and for Python that will not parse.""" for match in _SECRET_ASSIGNMENT_RE.finditer(text): value = match.group(2).strip() + if _is_token_usage_measurement(text, match): + continue if not _looks_like_placeholder(value): return [_finding_from_match("secret-env-assignment", rel_path, text, match)] return [] +def _is_token_usage_measurement(text: str, match: re.Match[str]) -> bool: + """Recognize typed usage counts and benchmark summaries, not credentials.""" + if match.group(1).lower() != "token" or text[match.end(1) : match.end(1) + 1].lower() != "s": + return False + start = match.start(1) + # Recover the entire key: matching only its suffix would also exempt an + # unrelated credential such as access_total_tokens. Scan only this key. + while start and (text[start - 1].isalnum() or text[start - 1] in "_.-"): + start -= 1 + end = match.end(1) + 1 + name = text[start:end].lower() + if name not in _TOKEN_COUNT_NAMES: + return False + if text[end : end + 1] in {'"', "'"} and (not start or text[start - 1] != text[end]): + return False + value_start = match.start(2) + quote = text[value_start - 1] if value_start else "" + # The text sweep includes an adjacent JSON delimiter in the value. Require + # the whole value to be numeric so strings and trailing payloads stay visible. + if quote not in {'"', "'"} and re.fullmatch(r"[0-9]+[,}\]]?", match.group(2)): + return True + if name != "tokens": + return False + # Benchmark summaries use four numeric statistics or a signed numeric delta. + # Bound parsing independently of file size; other containers remain findings. + if quote == '"': + value_start -= 1 + try: + value, _ = json.JSONDecoder(object_pairs_hook=tuple).raw_decode(text[value_start : value_start + 512]) + except ValueError: + return False + if isinstance(value, str): + return re.fullmatch(r"[+-][0-9]+(?:\.[0-9]+)?", value) is not None + return ( + isinstance(value, tuple) + and len(value) == 4 + and {key for key, _ in value} == {"mean", "stddev", "min", "max"} + and all(type(number) in {int, float} and number >= 0 and (isinstance(number, int) or math.isfinite(number)) for _, number in value) + ) + + def _python_secret_assignment_target(node: ast.expr) -> str | None: """Final identifier bound by a simple assignment target, else None.""" if isinstance(node, ast.Name): diff --git a/backend/tests/test_skillscan_secret_assignment_formats.py b/backend/tests/test_skillscan_secret_assignment_formats.py index a34781d257..b36c3cd37a 100644 --- a/backend/tests/test_skillscan_secret_assignment_formats.py +++ b/backend/tests/test_skillscan_secret_assignment_formats.py @@ -8,6 +8,7 @@ to read the same way. The quoted-key shapes below are what a JSON config uses. from __future__ import annotations +import json from pathlib import Path import pytest @@ -151,3 +152,63 @@ def test_pretty_printed_json_reports_the_line_the_key_is_on(tmp_path: Path) -> N root = _write_package(tmp_path, "config.json", '{\n "api_key": "' + SECRET + '"\n}') assert [finding["line"] for finding in _secret_findings(root)] == [2] + + +@pytest.mark.parametrize("name", ["tokens", "total_tokens", "input_tokens", "output_tokens", "prompt_tokens", "completion_tokens", "cached_tokens", "reasoning_tokens"]) +@pytest.mark.parametrize("value", [0, 84852]) +@pytest.mark.parametrize("template", ['{"%s": %s}', '{\n "%s": %s,\n "duration_ms": 23332\n}', "%s: %s", "%s=%s"]) +def test_numeric_model_token_counts_are_not_credentials(tmp_path: Path, name: str, value: int, template: str) -> None: + root = _write_package(tmp_path, "measurements.md", template % (name.upper() if "=" in template else name, value)) + + assert _secret_findings(root) == [] + + +@pytest.mark.parametrize( + "text", + [ + '{"total_tokens": "84852"}', + '{"total_tokens": "' + SECRET + '"}', + "total_tokens: 84852secret", + "access_tokens: 84852", + "api_key: 84852", + "password: 84852", + "access_total_tokens: 84852", + '{"access total_tokens": 84852}', + '{"access/total_tokens": 84852}', + '{"tokens": "84852"}', + '{"tokens": {"mean": "' + SECRET + '", "stddev": 1, "min": 0, "max": 2}}', + '{"tokens": {"api_key": "' + SECRET + '"}}', + '{"tokens": {"mean": true, "stddev": 1, "min": 0, "max": 2}}', + '{"tokens": {"mean": NaN, "stddev": 1, "min": 0, "max": 2}}', + '{"tokens": {"mean": 1e999, "stddev": 1, "min": 0, "max": 2}}', + '{"tokens": {"mean": "' + SECRET + '", "mean": 1, "stddev": 1, "min": 0, "max": 2}}', + '{"tokens": [["mean", 1], ["stddev", 1], ["min", 0], ["max", 2]]}', + '{"tokens": "+1700' + SECRET + '"}', + ], +) +def test_token_count_exemption_preserves_credential_detection(tmp_path: Path, text: str) -> None: + root = _write_package(tmp_path, "config.md", text) + + assert [finding["line"] for finding in _secret_findings(root)] == [1] + + +def test_numeric_token_count_does_not_hide_later_credential(tmp_path: Path) -> None: + root = _write_package(tmp_path, "config.md", '{"total_tokens": 84852}\napi_key: ' + SECRET) + + assert [finding["line"] for finding in _secret_findings(root)] == [2] + + +def test_numeric_token_count_does_not_suppress_cloud_token_scan(tmp_path: Path) -> None: + token = "sk-" + "a1b2c3d4" * 4 + root = _write_package(tmp_path, "config.md", '{"total_tokens": 84852}\nAuthorization: Bearer ' + token) + + findings = scan_skill_dir(root)["findings"] + + assert any(finding["rule_id"] == "secret-cloud-token" and finding["severity"] == "CRITICAL" for finding in findings) + + +@pytest.mark.parametrize("value", [{"mean": 3800.5, "stddev": 400, "min": 3200, "max": 4100}, "+1700", "-12.5"]) +def test_benchmark_token_summaries_are_measurements(tmp_path: Path, value) -> None: + root = _write_package(tmp_path, "benchmark.md", json.dumps({"tokens": value})) + + assert _secret_findings(root) == [] diff --git a/skills/public/skill-creator/references/schemas.md b/skills/public/skill-creator/references/schemas.md index b6eeaa2d4a..6f5b143153 100644 --- a/skills/public/skill-creator/references/schemas.md +++ b/skills/public/skill-creator/references/schemas.md @@ -297,6 +297,7 @@ Output from Benchmark mode. Located at `benchmarks//benchmark.json`. - `configuration`: Must be `"with_skill"` or `"without_skill"` (the viewer uses this exact string for grouping and color coding) - `run_number`: Integer run number (1, 2, 3...) - `result`: Nested object with `pass_rate`, `passed`, `total`, `time_seconds`, `tokens`, `errors` + - `tokens`: Uses sibling `timing.json.total_tokens` when present (including zero), independently of duration fields in `grading.json`. If the token count is missing or null, aggregation retains the legacy `execution_metrics.output_chars` proxy, defaulting to zero if that is also missing. - `run_summary`: Statistical aggregates per configuration - `with_skill` / `without_skill`: Each contains `pass_rate`, `time_seconds`, `tokens` objects with `mean` and `stddev` fields - `delta`: Difference strings like `"+0.50"`, `"+13.0"`, `"+1700"` diff --git a/skills/public/skill-creator/scripts/aggregate_benchmark.py b/skills/public/skill-creator/scripts/aggregate_benchmark.py index 891644e435..b190c99ba7 100755 --- a/skills/public/skill-creator/scripts/aggregate_benchmark.py +++ b/skills/public/skill-creator/scripts/aggregate_benchmark.py @@ -133,23 +133,25 @@ def load_run_results(benchmark_dir: Path) -> dict: "total": grading.get("summary", {}).get("total", 0), } - # Extract timing — check grading.json first, then sibling timing.json + # Read recorded tokens independently of the grading duration. timing = grading.get("timing", {}) result["time_seconds"] = timing.get("total_duration_seconds", 0.0) timing_file = run_dir / "timing.json" - if result["time_seconds"] == 0.0 and timing_file.exists(): + if timing_file.exists(): try: with open(timing_file, encoding="utf-8") as tf: timing_data = json.load(tf) - result["time_seconds"] = timing_data.get("total_duration_seconds", 0.0) - result["tokens"] = timing_data.get("total_tokens", 0) + if result["time_seconds"] == 0.0: + result["time_seconds"] = timing_data.get("total_duration_seconds", 0.0) + result["tokens"] = timing_data.get("total_tokens") except json.JSONDecodeError: pass # Extract metrics if available metrics = grading.get("execution_metrics", {}) result["tool_calls"] = metrics.get("total_tool_calls", 0) - if not result.get("tokens"): + # Keep the legacy proxy only when no token count was recorded. + if result.get("tokens") is None: result["tokens"] = metrics.get("output_chars", 0) result["errors"] = metrics.get("errors_encountered", 0) diff --git a/tests/skills/test_skill_creator_benchmark.py b/tests/skills/test_skill_creator_benchmark.py new file mode 100644 index 0000000000..e08fc9e3a2 --- /dev/null +++ b/tests/skills/test_skill_creator_benchmark.py @@ -0,0 +1,119 @@ +"""Regressions for skill-creator benchmark measurements.""" + +import json +import runpy +from pathlib import Path + +import pytest + + +@pytest.fixture +def benchmark_script(): + script = ( + Path(__file__).resolve().parents[2] + / "skills/public/skill-creator/scripts/aggregate_benchmark.py" + ) + return runpy.run_path(str(script)) + + +def write_run(root, *, grading_timing=None, timing=None, config="with_skill"): + run_dir = root / "eval-1" / config / "run-1" + run_dir.mkdir(parents=True) + grading = { + "summary": {"pass_rate": 1.0, "passed": 1, "failed": 0, "total": 1}, + "execution_metrics": {"output_chars": 12450}, + } + if grading_timing is not None: + grading["timing"] = grading_timing + (run_dir / "grading.json").write_text(json.dumps(grading), encoding="utf-8") + if timing is not None: + (run_dir / "timing.json").write_text(json.dumps(timing), encoding="utf-8") + return run_dir + + +def test_recorded_tokens_used_when_grading_already_has_duration( + tmp_path, benchmark_script +): + write_run( + tmp_path, + grading_timing={"total_duration_seconds": 23.3}, + timing={"total_duration_seconds": 25.0, "total_tokens": 84852}, + ) + + benchmark = benchmark_script["generate_benchmark"](tmp_path) + + assert benchmark["runs"][0]["result"]["tokens"] == 84852 + assert benchmark["runs"][0]["result"]["time_seconds"] == 23.3 + assert benchmark["run_summary"]["with_skill"]["tokens"]["mean"] == 84852 + + +@pytest.mark.parametrize("layout", ["workspace", "legacy"]) +@pytest.mark.parametrize( + "grading_timing", + [None, {}, {"total_duration_seconds": 0}, {"total_duration_seconds": 23.3}], +) +@pytest.mark.parametrize("tokens", [0, 84852]) +def test_recorded_tokens_preserved_in_both_layouts( + tmp_path, benchmark_script, layout, grading_timing, tokens +): + root = tmp_path / "runs" if layout == "legacy" else tmp_path + write_run( + root, + grading_timing=grading_timing, + timing={"total_duration_seconds": 23.3, "total_tokens": tokens}, + ) + + benchmark = benchmark_script["generate_benchmark"](tmp_path) + + assert benchmark["runs"][0]["result"]["tokens"] == tokens + assert benchmark["runs"][0]["result"]["time_seconds"] == 23.3 + assert benchmark["run_summary"]["with_skill"]["tokens"]["mean"] == tokens + + +@pytest.mark.parametrize( + "timing_state", ["absent", "missing_tokens", "null_tokens", "invalid_json"] +) +@pytest.mark.parametrize("has_grading_duration", [False, True]) +def test_missing_token_measurements_keep_legacy_character_fallback( + tmp_path, benchmark_script, timing_state, has_grading_duration +): + grading_timing = {"total_duration_seconds": 23.3} if has_grading_duration else None + timing = None + if timing_state in {"missing_tokens", "null_tokens"}: + timing = {"total_duration_seconds": 25.0} + if timing_state == "null_tokens": + timing["total_tokens"] = None + run_dir = write_run(tmp_path, grading_timing=grading_timing, timing=timing) + if timing_state == "invalid_json": + (run_dir / "timing.json").write_text("{invalid", encoding="utf-8") + + benchmark = benchmark_script["generate_benchmark"](tmp_path) + + assert benchmark["runs"][0]["result"]["tokens"] == 12450 + expected_time = 23.3 if has_grading_duration else (25.0 if timing else 0.0) + assert benchmark["runs"][0]["result"]["time_seconds"] == expected_time + + +def test_recorded_tokens_propagate_to_summary_delta_and_markdown( + tmp_path, benchmark_script +): + write_run( + tmp_path, + grading_timing={"total_duration_seconds": 23.3}, + timing={"total_tokens": 84852}, + config="with_skill", + ) + write_run( + tmp_path, + grading_timing={"total_duration_seconds": 20.0}, + timing={"total_tokens": 10000}, + config="without_skill", + ) + + benchmark = benchmark_script["generate_benchmark"](tmp_path) + + assert benchmark["run_summary"]["without_skill"]["tokens"]["mean"] == 10000 + assert benchmark["run_summary"]["delta"]["tokens"] == "+74852" + assert "| Tokens | 84852 ± 0 | 10000 ± 0 | +74852 |" in benchmark_script[ + "generate_markdown" + ](benchmark)