feat: add deterministic quality gate for SKILL.md submissions#44
feat: add deterministic quality gate for SKILL.md submissions#44Benkapner wants to merge 2 commits into
Conversation
Add QualityGate with 6 deterministic checks: - Description quality: frontmatter validation (name, description) - Broken references: relative markdown links to missing files - File completeness: thin instruction.md, tests without assertions - Imprecise instructions: vague language (hedging, vague conditions) - Unfinished content: TODO, FIXME, placeholder markers, TBD - Generic advice: filler text (follow best practices, handle errors properly) Patterns adapted from harness-eval-lab (setup-eval). 44 tests.
Detect circular reference chains between skills in submissions with multiple SKILL.md files (nested layout). Ported from harness-eval-lab circular_references.py. 4 new tests.
|
Update (773754c): Added circular reference detection as a 7th check category. It builds a reference graph from SKILL.md files in nested layouts and flags cycles (e.g., skill-a -> skill-b -> skill-a), preventing potential agent loops during evaluation. Ported from harness-eval-lab |
GuyZivRH
left a comment
There was a problem hiding this comment.
Overview
This PR adds a deterministic quality scanner and gate with 7 check categories: description quality, broken references, file completeness, imprecise instructions, unfinished content, generic advice, and circular references. The architecture correctly mirrors PRs #42/#43 (security gates), and the 48-unit test suite is thorough.
However, three reviewers independently identified 4 bugs and several design issues that need addressing before merge.
Critical (Bugs)
1. Anchor fragments not stripped from broken reference checks
File: abevalflow/quality/skillmd_quality_scanner.py — check_broken_references()
Reported by: All 3 reviewers
Links like [Installation](./README.md#installation) are flagged as broken. The anchor-skip guard only catches pure anchors (#section):
if ref.startswith(("http://", "https://", "#", "mailto:")):
continue./README.md#installation doesn't start with #, so the full string (including fragment) is used as the file path — README.md#installation won't exist on disk.
Fix:
ref_path = file_path.parent / ref.split("#")[0]Missing test: Add coverage for file.md#anchor links:
def test_skips_file_with_fragment(self, tmp_path):
(tmp_path / "guide.md").write_text("# Guide")
p = tmp_path / "SKILL.md"
p.write_text("See [Installation](guide.md#install) for details.")
assert len(check_broken_references(p)) == 02. Broken reference check does not skip code fences/blockquotes
File: abevalflow/quality/skillmd_quality_scanner.py — check_broken_references()
Reported by: Agent 1
Other checks (check_imprecise_instructions, check_unfinished_content, check_generic_advice) correctly skip content inside code fences and blockquotes. However, check_broken_references() does not implement this filtering.
Example links shown in markdown code blocks are being reported as broken references, creating avoidable noise. The PR description explicitly states "All checks skip content inside code blocks and blockquotes to avoid false positives."
Fix: Add the same code-fence tracking logic used in other check functions:
in_code_fence = False
for line_num, line in enumerate(content.splitlines(), start=1):
stripped = line.strip()
if stripped.startswith("```"):
in_code_fence = not in_code_fence
continue
if in_code_fence or stripped.startswith(">"):
continue
# ... existing link detection logic3. Circular reference detection disabled for nested submissions in Tekton
File: pipeline/tasks/phases/test.yaml
Reported by: Agent 2 (mrkpv)
The scan step adjusts SUBMISSION_PATH for nested skill layouts:
if [ -d "$SUBMISSION_PATH/skills" ] && [ -f "$SUBMISSION_PATH/skills/SKILL.md" ]; then
SUBMISSION_PATH="$SUBMISSION_PATH/skills"
fiBut check_circular_references(directory) looks for directory / "skills". After the adjustment, the scanner is given …/submission/skills/ as its root, so it looks for …/submission/skills/skills/ — which won't exist. Circular reference detection is never triggered for any real multi-skill submission.
Fix options:
- (Preferred) Pass the original submission path to the scanner and let
scan_directoryhandle the layout detection internally — keeps the scanner self-contained. - Remove the
SUBMISSION_PATHadjustment entirely and let the scanner findSKILL.mdinsideskills/viarglob.
4. Circular reference detection uses folder names, not frontmatter skill names
File: abevalflow/quality/skillmd_quality_scanner.py — check_circular_references()
Reported by: Agent 1
Graph nodes are keyed by directory name (skill_md.parent.name), not the skill identifier from frontmatter. If content references declared skill names (common practice) and folder names differ, cycles are not detected.
Reproduction: Two skills referencing each other by frontmatter names produce no findings when folder names don't match.
Fix: Parse the frontmatter name field and use it as the canonical node key:
fm_match = _FRONTMATTER_RE.match(content)
if fm_match:
try:
fm = yaml.safe_load(fm_match.group(1))
if isinstance(fm, dict) and "name" in fm:
skill_name = fm["name"]
except yaml.YAMLError:
passAlternatively, if folder name is the canonical skill identifier by contract, enforce/validate that contract in scanner logic or metadata validation.
Medium
5. Circular reference patterns too broad (false positive risk)
File: abevalflow/quality/skillmd_quality_scanner.py
Reported by: All 3 reviewers
re.compile(r"/(\w[\w-]+)(?:\s|$|[),\]])"),This matches any /word path segment, including URL paths in prose (POST /api/skills, /etc/config). If a skill directory shares a name with a URL path fragment, spurious circular references could be reported.
Suggestion: Require leading space/line-start context, or limit matching to only known skill names discovered during the scan.
6. Score formula: average-per-finding, not total-quality signal
File: abevalflow/gates/quality/skillmd_quality.py
Reported by: Agent 2 (mrkpv), Agent 3 (gziv)
weighted_score = (
severity_counts["critical"] * 0.0
+ severity_counts["high"] * 0.25
...
)
score = weighted_score / total_findingsA submission with 1 high finding scores 0.25. A submission with 10 medium findings also scores 0.5. Volume has no downward pressure — only the severity distribution matters. This can give a false sense of quality for submissions with many low-severity issues.
Options:
- Use
1.0 - (weighted_penalty / max_possible_penalty) - Cap score based on finding count
- At minimum, document this as intentional with a comment
7. Missing critical count in gate message
File: abevalflow/gates/quality/skillmd_quality.py
Reported by: Agent 3 (gziv)
message = (
f"SKILL.md quality: {total_findings} findings "
f"(high={severity_counts['high']},"
f" medium={severity_counts['medium']},"
f" low={severity_counts['low']})"
)The message omits the critical count even though it's tracked in severity_counts and used in pass/fail logic. Add critical={severity_counts['critical']}, for consistency.
Minor
8. Duplicated comment header in test file
File: tests/test_skillmd_quality_scanner.py, lines 1111–1116
Reported by: All 3 reviewers
# ---------------------------------------------------------------------------
# scan_directory tests <-- misplaced
# ---------------------------------------------------------------------------
# Circular references tests
# ---------------------------------------------------------------------------The scan_directory tests separator appears before TestCircularReferences instead of before TestScanDirectory. Cosmetic but confusing.
9. No test for non-directory input to scan_directory
File: tests/test_skillmd_quality_scanner.py
Reported by: Agent 2 (mrkpv)
The function handles it gracefully, but there's no unit test:
def test_nonexistent_directory(self, tmp_path):
result = scan_directory(tmp_path / "does_not_exist")
assert result["findings"] == []Open Questions
-
Test plan item — The PR checklist shows
./scripts/misc/trigger_test_runs.shunchecked. Has this been tested in a real pipeline run? -
Code fence behavior in broken references — If you intentionally want link checks to include fenced examples, that should be documented explicitly because current PR text says checks skip code blocks/quotes.
What Looks Good
- Clean architecture — Same pattern as security gates (scan → JSON → gate → aggregate), easy to follow
- Comprehensive tests — 48 tests with clear class-per-check organization and false-positive resistance coverage
- Code-block skipping — Correctly implemented and tested in
check_imprecise_instructions,check_unfinished_content,check_generic_advice - Deduplication —
checked: set[str]in broken references,sorted(set(cycle[:-1]))in cycle detection - Tekton integration —
|| trueguard consistent with security scan pattern; exit 0 always, gate decides - Mode behavior — DISABLED/WARN/BLOCK handling aligned with other deterministic gates
Required Before Merge
| # | Issue | Severity |
|---|---|---|
| 1 | Strip anchor fragments in check_broken_references |
Critical |
| 2 | Add code-fence/blockquote skipping to check_broken_references |
Critical |
| 3 | Fix Tekton SUBMISSION_PATH adjustment for circular reference detection |
Critical |
| 4 | Fix circular reference node keying (folder name vs frontmatter name) | Critical |
Recommended (Not Blocking)
| # | Issue | Severity |
|---|---|---|
| 5 | Tighten circular reference regex patterns | Medium |
| 6 | Document or revise score formula | Medium |
| 7 | Add critical count to gate message | Medium |
| 8 | Fix test file comment headers | Minor |
| 9 | Add test for non-directory input | Minor |
Summary
Adds a new QualityGate implementation with deterministic checks for skill submission content quality, complementing the existing LLM quality review. Patterns adapted from harness-eval-lab (setup-eval).
Follows the same architecture as the security gate contributions (PRs #42, #43): scanning module produces JSON, gate class reads it, auto-discovered by aggregate_scorecard.py.
What it checks
6 categories of content quality issues:
[text](./path)) pointing to files that don't exist. Skips URLs, anchors, templates. Deduplicates.[INSERT YOUR KEY HERE]), "coming soon", "work in progress", "not yet implemented"All checks skip content inside code blocks and blockquotes to avoid false positives.
Files
abevalflow/quality/skillmd_quality_scanner.pyscripts/skillmd_quality_scan.pyabevalflow/gates/quality/skillmd_quality.pyabevalflow/gates/quality/__init__.pypipeline/tasks/phases/test.yamltests/test_skillmd_quality_scanner.pyHow it integrates
The gate registers via
@register_quality_gate(\"skillmd-quality\")and is auto-discovered byaggregate_scorecard.pythroughget_all_quality_gates(). No changes needed to scorecard, certification, or Compass facts.Task always exits 0; gate decides pass/fail via policy (same pattern as security gates).
Test plan
./scripts/misc/trigger_test_runs.sh)">