Skip to content

[#313] feat: trigger/effectiveness eval harness + conformance (task T7)#324

Merged
rucka merged 5 commits into
mainfrom
feat/#313-t7-eval-harness
Jul 13, 2026
Merged

[#313] feat: trigger/effectiveness eval harness + conformance (task T7)#324
rucka merged 5 commits into
mainfrom
feat/#313-t7-eval-harness

Conversation

@rucka

@rucka rucka commented Jul 12, 2026

Copy link
Copy Markdown
Collaborator

Testing: trigger/effectiveness eval harness + skills conformance gate (T7)

Story: #313 (T7 — Trigger/effectiveness eval + conformance)
Type: Testing · Priority: P1 · Risk: Low (no skill content changed; harness + gate only)

Motivation

AC7 requires before/after trigger-test evidence for the T3 description rewrites; AC8 requires static conformance in CI. This PR delivers the eval that GATES T3 (per the story's dependency graph) plus the conformance check, and records the baseline ("before") run against the current descriptions. The eval procedure follows principle 9 of the authoring standard (PR #319); the conformance rules follow principle 8.

Changes Made

  • Eval prompt setspackages/knowledge-hub/eval/trigger-prompts.json: 79 prompts (53 should-trigger / 26 should-not-trigger) across 12 skill families; every one of the 35 skills has ≥1 should-trigger prompt; near-misses name the sibling they decoy (near_miss_for) and the correct destination. Grounded in the current frontmatter descriptions.
  • Runner procedurepackages/knowledge-hub/eval/README.md: catalog extraction, fresh-session cold-start runs, PASS/AMBIGUOUS/FAIL verdicts, before/after comparison, regression ⇒ revert that skill's description (iterate on the description, not the eval).
  • Baseline run (committed evidence)packages/knowledge-hub/eval/results/2026-07-12-baseline.md: per-prompt expected vs selected + summary. 76 PASS / 3 AMBIGUOUS / 0 FAIL (96.2% strict). All 3 ambiguities are assess-* overlap (assess-pm↔setup-pm ×2, assess-stack↔verify-adoption) — the known near-clone problem showing up as designed; the qualitative section documents that ~60% of each assess-* description is trigger-inert scaffold. Methodology (single-model, single-pass, author=executor bias disclosure) stated in-file.
  • Static conformance gatepackages/knowledge-hub/src/skills-conformance-check.ts (typed module, check-broken-links.ts style; run via ts-node, exported primitives + require.main CLI guard): (a) frontmatter portability — agentskills.io-core top-level fields + tolerated version/author Pair extension (principle 8), assistant-specific fields are errors; (b) name ≤64, description ≤1024, combined ≤1024; (c) relative references in skill bodies resolve (fenced code + placeholder paths skipped); (d) catalog counts in next's SKILL.md vs real corpus count.
  • Wiring — package script skills:conformance = ts-node src/skills-conformance-check.ts (mirrors check:links); root skills:conformance delegates to the package (pnpm --filter @pair/knowledge-hub skills:conformance); wired into quality-gate (pre-push) and CI after docs-staleness.
  • Testspackages/knowledge-hub/src/skills-conformance-check.test.ts: sibling test importing the module directly; 18 unit tests incl. end-to-end fixture corpus.

Known warning-only rule (documented, not weakened)

Rule (d) currently finds 3 mismatches: next/SKILL.md states a 33-skill catalog, corpus has 35 (checkpoint + grill missing — exactly T1's bug). Implemented as WARN, non-blocking, with TODO(#313/T1) in the script to promote to error once T1 regenerates the catalog. All other rules PASS on the current corpus.

Files Changed

Evolved across two review rounds: (a) conformance logic moved from a root script into a tested package module called by the CLI/gate (commit 7c81769); (b) knowledge-hub/src reorganized by role into tools/ (runnable CLIs + siblings) and conformance/ (dataset-assertion tests) (commit 1593ad2). This PR carries the reorg because it introduced skills-conformance-check, so a separate chore would create a merge-order dependency. See the two follow-up comments below.

New knowledge-hub/src layout:

src/
├── tools/         check-broken-links.{ts,test.ts}, transfer-dataset.{ts,test.ts}, skills-conformance-check.{ts,test.ts}
└── conformance/   agents-md.test.ts, assess-output-only.test.ts, quality-model.test.ts
  • Added (eval harness): packages/knowledge-hub/eval/{README.md,trigger-prompts.json,results/2026-07-12-baseline.md}
  • Added (conformance module + sibling test): packages/knowledge-hub/src/tools/skills-conformance-check.{ts,test.ts} (typed module: exported primitives + require.main CLI guard; test imports the sibling directly)
  • Moved by role (git mv, history preserved): check-broken-links.{ts,test.ts}, transfer-dataset.{ts,test.ts}src/tools/; agents-md.test.ts, assess-output-only.test.ts, quality-model.test.tssrc/conformance/. Path-depth fixups (one extra ..) applied to every moved file's dataset/root paths and verified by a live 35-skill conformance run.
  • Modified: root package.json (skills:conformance delegates to the package + wired into quality-gate); packages/knowledge-hub/package.json (check:links/transfer:dataset/skills:conformancets-node src/tools/<file>.ts); .github/workflows/ci.yml (CI step — unchanged command, delegates); packages/knowledge-hub/vitest.config.ts (dropped stale src/index.ts coverage exclude); eval/results/2026-07-12-baseline.md (review finding 2 wording).
  • Deleted: scripts/skills-conformance-check.js (logic moved into the module; single source of truth).

AC coverage

  • AC7 — prompt set per family, procedure documented, baseline ("before") results committed; "after" half lands with T3.
  • AC8 — conformance (portability, ≤1024, pointers, counts) wired into quality-gate + CI; corpus green with the documented T1 warning.

Testing / Quality Gates

knowledge-hub vitest:     98/98 pass (incl. sibling conformance suite 18/18)
knowledge-hub ts:check:   PASS (module typechecks under strict/NodeNext)
knowledge-hub lint:       PASS
KB broken-links:          all markdown links valid
mdlint:                   PASS
docs-staleness:           PASS — 35 skills, 8 commands in sync
skills:conformance:       PASS (root → package delegation) — 35 skills; WARN ×3 (next 33-vs-35, gated on #313/T1)
pre-push quality-gate:    full run passed on push

Risk Assessment

Low — additive harness + gate; no skill/dataset content modified. Only failure surface is the new CI step, verified green on the current corpus.

🤖 Generated with Claude Code

- eval prompt sets (79 prompts, 12 families, all 35 skills) + runner README
- baseline run committed: 76 PASS / 3 AMBIGUOUS (all assess-* overlap — motivates T3)
- scripts/skills-conformance-check.js: frontmatter portability, name+description <=1024,
  pointer resolution, catalog counts (warning-only until #313/T1)
- wired as skills:conformance in quality-gate + CI; 18 unit tests

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t (review)

- port scripts/skills-conformance-check.js -> packages/knowledge-hub/src/skills-conformance-check.ts
  (typed ESM module, check-broken-links.ts style; require.main CLI guard prints same PASS/WARN/FAIL)
- sibling test imports the module directly (drop createRequire bridge); 18 cases unchanged
- package script skills:conformance = ts-node src/skills-conformance-check.ts (mirrors check:links)
- root skills:conformance delegates to the package; quality-gate + CI unchanged
- delete root script (single source of truth); eval README path ref updated

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@rucka

rucka commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator Author

Restructure: conformance logic moved into the tested module (review follow-up)

Addressed the fair objection that the conformance logic lived in a root scripts/*.js while its test reached across the package boundary via createRequire('../../../scripts/...'). Now it follows the exact check-broken-links.{ts,test.ts} precedent.

What changed

  • Logic ported into the package: scripts/skills-conformance-check.jspackages/knowledge-hub/src/skills-conformance-check.ts — a typed ESM module exporting all primitives (parseFrontmatter, checkFrontmatterFields, checkSizeLimits, extractLinkTargets, isCheckableTarget, checkLinks, checkCatalogCounts, collectSkillFiles, runChecks, SPEC_FIELDS, PAIR_EXTENSIONS), plus a require.main === module CLI guard that prints the same PASS/WARN/FAIL output and exit codes. Same module style as check-broken-links.ts.
  • Test is now a real sibling: src/skills-conformance-check.test.ts imports directly — import { … } from './skills-conformance-check'. The createRequire bridge and the ConformanceModule interface shim are gone; the 18 cases (unit + fixture-corpus e2e) are unchanged in intent and now exercise the actual sibling.
  • CLI calls the primitives: package script skills:conformance = ts-node src/skills-conformance-check.ts (mirrors check:links / transfer:dataset).
  • Root delegates: root skills:conformance = pnpm --filter @pair/knowledge-hub skills:conformance, same position in quality-gate. The CI step (run: pnpm skills:conformance) is unchanged — it now delegates into the package.
  • Root script deleted — single source of truth, zero duplication.
  • Flat in src/ (no subfolder, no package-root test). eval/README.md path reference updated to the module.

Why this dissolves the concern

  • Single source of truth: the logic and its test live together in the tested module; nothing crosses the boundary anymore.
  • The CLI/gate is a thin caller of the exported primitives — no logic in the script layer, no duplication.
  • The test is a genuine sibling of the code it tests, identical to check-broken-links.
  • The gate still runs in CI and pre-push via pnpm skills:conformance (root → package delegation).

Gates (all green)

@pair/knowledge-hub test:   98/98 pass (incl. sibling-importing conformance suite 18/18) + check:links valid
@pair/knowledge-hub ts:check: PASS (new .ts typechecks under strict/NodeNext)
@pair/knowledge-hub lint:    PASS
pnpm skills:conformance (root → package): PASS — 35 skills; WARN ×3 (next 33-vs-35 catalog, gated on #313/T1 via TODO)
pnpm docs:staleness:         PASS — 35 skills, 8 commands in sync
pnpm mdlint:check:           PASS

The 3 WARNs (next's stated 33-skill catalog vs the real 35) are unchanged and remain warning-only with TODO(#313/T1) until T1 (#325) regenerates the catalog.

@rucka

rucka commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator Author

Code Review — PR #324

Review Information

PR Number: #324
Author: rucka
Reviewer: Independent reviewer (adversarial, code+AC+PR only)
Review Date: 2026-07-13
Story/Epic: #313 — T7 (Trigger/effectiveness eval harness + static conformance gate)
Review Type: Testing (harness + CI gate; also a post-review restructure of the conformance logic)
Head reviewed: 7c81769

Review Summary

Overall Assessment

  • Approved with Comments — Minor, non-blocking notes only; can merge.

Key Changes Summary

T7 delivers: (a) the trigger-eval prompt set eval/trigger-prompts.json; (b) the runner procedure eval/README.md; (c) the committed "before" baseline eval/results/2026-07-12-baseline.md; (d) the static conformance gate, restructured from the original root scripts/skills-conformance-check.js into a tested package module packages/knowledge-hub/src/skills-conformance-check.{ts,test.ts} called by a thin CLI guard, wired into quality-gate (pre-push) and CI. This review covers the whole PR with extra scrutiny on the restructure (never independently reviewed; first attempt died mid-run).

Business Value Validation

Satisfies AC7 (before-evidence + documented eval procedure — the "after" half correctly deferred to T3 per the dependency graph) and AC8's conformance requirement (portability, ≤1024 limits, pointer resolution, catalog counts) wired into CI. Delivers the gate that GATES T3.

Verification performed (independent, at head 7c81769)

Note: the pre-existing detached worktree was stale at e705370 (pre-restructure); all results below were re-run after moving it to 7c81769.

Check Result
pnpm skills:conformance (root → package delegation) PASS — 35 skills, exactly 3 WARN (next 33-vs-35), exit 0
pnpm --filter @pair/knowledge-hub test 98/98 pass (incl. skills-conformance-check.test.ts 18/18) + check:links valid
pnpm --filter @pair/knowledge-hub ts:check PASS
pnpm --filter @pair/knowledge-hub lint PASS
pnpm docs:staleness PASS — 35 skills, 8 commands in sync
Real-violation exit code (fabricated corpus) exit 1 for assistant-specific field + over-limit description (+combined bound) + dead pointer

Restructure correctness (scripts/*.js → tested module + thin CLI)

  1. Behavioral parity — CONFIRMED. Function-by-function diff of the deleted scripts/skills-conformance-check.js (e705370) vs the new module: parseFrontmatter, checkFrontmatterFields, checkSizeLimits, extractLinkTargets, isCheckableTarget, checkLinks, checkCatalogCounts, collectSkillFiles, runChecks, SPEC_FIELDS, PAIR_EXTENSIONS are semantically identical. Only intra-refactor: quote-stripping extracted into unquote() (same logic). All four checks preserved (frontmatter portability incl. assistant-specific=error; name≤64 / description≤1024 / combined≤1024; fenced-code + placeholder-skipping pointer resolution; catalog-count WARN).
  2. CLI + path resolution — CONFIRMED. New ROOT = join(__dirname,'..')packages/knowledge-hub, SKILLS_DIR = dataset/.skills → resolves to the same packages/knowledge-hub/dataset/.skills as the old scripts/-relative path. require.main guard runs correctly under ts-node; identical PASS/WARN/FAIL output + exit codes. Verified by running it.
  3. Test rewire — CONFIRMED. createRequire('../../../scripts/...') bridge + ConformanceModule interface shim removed; now import { … } from './skills-conformance-check'. Diff shows that is the ONLY change to the test — every case survives (parseFrontmatter, checkFrontmatterFields incl. disable-model-invocation rejection, checkSizeLimits incl. combined bound, isCheckableTarget, extractLinkTargets fenced-code skip, checkCatalogCounts, runChecks fixture corpus). 18/18 green.
  4. Single source of truth — CONFIRMED. scripts/skills-conformance-check.js deleted; repo grep (excl. node_modules/.git) for scripts/skills-conformance-check and skills-conformance-check.jszero dangling refs. Root skills:conformance delegates to the package; quality-gate keeps it in the same position (after docs:staleness, before dup:check); CI adds a step running the unchanged pnpm skills:conformance right after docs-staleness. The net diff vs origin/main reflects the delegation state (what merges), not the intermediate script.
  5. Layout — CONFIRMED. Module + sibling test flat in src/, mirroring check-broken-links.{ts,test.ts}; no package-root test, no subfolder.

Original T7 substance

  1. Prompt sets — CONFIRMED. 79 prompts (53 should-trigger + 26 should-not-trigger), 12 families, corpus_size=35=actual SKILL.md count. Every one of the 35 corpus skills has ≥1 should-trigger prompt; no expected/near_miss_for/family skill falls outside the corpus (one intentional family glob — see F1); no duplicate ids. Should-not-trigger prompts are genuine sibling near-misses (setup-pm vs assess-pm, review vs verify-done, estimate vs refine-story, plan-tasks vs write-issue, verify-quality vs assess-code-quality, …).
  2. Baseline — CONFIRMED. Methodology disclosed (single LLM, single pass, catalog-only cold start, author=executor bias disclosure). Stats internally consistent: 51/53, 25/26, 76/79 all ≈96.2%; 3 AMBIGUOUS = AD-12, AD-14, PB-N2, matching the per-prompt tables. The assess-* ambiguities trace accurately to the real current descriptions (assess-pm "proposes a tool choice" vs setup-pm "guiding selection"; assess-stack "implementation (add dependency)" vs verify-adoption "checks code and configuration against adoption files").
  3. README vs principle 9 — CONFIRMED. README aligns with writing-skills.mdx §9 Evaluation on docs/#313-authoring-standard (hypothesis/experiment framing, fresh-session cold start, before/after comparison, and the regression⇒revert-that-description rule). Correct post-restructure module path referenced.
  4. WARN-not-error scoping — CONFIRMED empirically. Catalog-count is warning-only (TODO(#313/T1), does not affect exit). A fabricated corpus with an assistant-specific field, an over-limit description, and a dead pointer produced 4 errors and exit 1 — the hard rules genuinely fail the gate. Fixture test corroborates.

Detailed Review Comments

Positive Feedback

  • The restructure fully dissolves the original cross-boundary-test objection: logic and test are now genuine siblings, byte-for-byte behavior preserved, CLI reduced to a thin caller — verified by running the full gate, not just reading it.
  • Prompt set is disciplined: complete skill coverage, realistic phrasing, decoys that name the sibling they target and the correct destination; the baseline is honest about author=executor bias and marks near-ties rather than silently passing them.
  • WARN/error split is exactly right: the one known drift (next 33-vs-35) is a non-blocking warning tied to T1, while portability/limits/pointer violations hard-fail — proven empirically.

Minor Issues (non-blocking)

  • eval/trigger-prompts.json (AD-N3)near_miss_for: "assess-*" uses a family glob rather than a concrete skill name, unlike every other entry. Semantically clear (family-level decoy, expected: none, discussed in the baseline) but schema-inconsistent. nonActionable — intentional family decoy; documented. Recommendation (optional): note the glob convention in the file's $comment.
  • eval/results/2026-07-12-baseline.md (Ambiguities section) — prose says "Both ambiguities … (AD-12, PB-N2, AD-14)" while listing three ids; it groups two root-cause kinds, but the wording reads as a miscount. The summary stats table is correct (3 AMBIGUOUS). Recommendation (optional): reword to "The ambiguities … fall into two kinds".
  • eval methodology (baseline.md) — single-model, single-pass, author==executor. A real self-grading / no-inter-rater-reliability limitation, so the 96.2% is a soft number and the before/after gate's reliability rests on disciplined manual re-runs. nonActionable — explicitly disclosed; AC7 asks for a documented procedure + recorded evidence, both present; appropriate for a "before" baseline.
  • src/skills-conformance-check.ts:extractLinkTargets — strips only triple-backtick fenced blocks, not inline code spans, so a link inside `…` would still be pointer-checked. nonActionable — inherited unchanged from the deleted script (behavioral-parity requirement); no current false positive (conformance PASSes clean).

Questions

  • None blocking. (Scope note, not a defect: this PR ships only the "before" baseline; the "after" run lands with T3 per the dependency graph — correct.)

Testing Review

New tests are clear, independent, well-named; the fixture-corpus e2e exercises the real runChecks including the negative cases (assistant-specific field, broken link, catalog warning). Coverage of the module's primitives is complete. All 98 knowledge-hub tests pass.

Risk Assessment

Risk Impact Probability Mitigation
Manual eval not reproducible mechanically Low Med Documented procedure + committed evidence; conformance (the automatable half) is fully gated
Catalog WARN masks a future real count drift Low Low Scoped to next/SKILL.md, TODO(#313/T1) to promote to error

Adoption Compliance

No new dependencies, libraries, or architectural decisions introduced (harness + gate only). No ADR required. No skill/dataset content changed, so mirror byte-consistency is not exercised by this PR.


Decision: Approved with Comments — no critical or major issues; all AC in T7's scope met; restructure verified behavior-preserving. The four Minor notes are non-blocking (three nonActionable / by-design).

…eline wording

- git mv 9 files: CLIs+siblings -> src/tools/, dataset-assertion tests -> src/conformance/
- path-depth fixups (one extra ..): tools/ ROOT/DATASET, conformance/ readFileSync dataset paths
- package.json check:links/transfer:dataset/skills:conformance -> src/tools/<file>.ts
- drop stale src/index.ts from vitest.config exclude
- eval baseline: 'Both ambiguities'->'two kinds' (review finding 2); README path

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@rucka

rucka commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator Author

Role-based src/ layout + review finding 2 (remediation round)

Two changes, one round.

1. Role-based src/ reorg (repo-owner-approved)

knowledge-hub/src was a flat pile of two species — runnable CLIs and dataset-assertion tests. Split by role:

packages/knowledge-hub/src/
├── tools/         # runnable CLIs + their sibling tests
│   ├── check-broken-links.{ts,test.ts}
│   ├── transfer-dataset.{ts,test.ts}
│   └── skills-conformance-check.{ts,test.ts}
└── conformance/   # dataset-assertion tests
    ├── agents-md.test.ts
    ├── assess-output-only.test.ts
    └── quality-model.test.ts

All 9 files moved with git mv (history preserved — GitHub shows them as renames). This PR carries the reorg because it's where skills-conformance-check was introduced, so a separate chore would create a merge-order dependency.

Path-depth fixups (the main risk — every moved file resolves paths relative to its own location; one dir deeper needs one extra ..), verified by running:

File → new dir old → new
tools/check-broken-links.ts ROOT = join(__dirname, '..')join(__dirname, '..', '..')
tools/transfer-dataset.ts DATASET = join(__dirname, '..', 'dataset')join(__dirname, '..', '..', 'dataset') (+ usage string)
tools/skills-conformance-check.ts ROOT = join(__dirname, '..')join(__dirname, '..', '..')
conformance/agents-md.test.ts ../dataset/AGENTS.md../../dataset/AGENTS.md
conformance/assess-output-only.test.ts ../dataset/…../../dataset/…; mirror ../../../.claude/skills../../../../.claude/skills
conformance/quality-model.test.ts all 4 ../dataset/…../../dataset/…

Verified by live run, not assumption:

  • pnpm skills:conformance (root → package delegation) reports 35 skills (a broken SKILLS_DIR would silently report 0) with exactly 3 WARN (next 33-vs-35) and exit 0.
  • The 3 dataset tests still read real files (their fixtures load via top-level readFileSync; a wrong path would ENOENT at collection and fail the suite). Counts unchanged: agents-md 1, assess-output-only 24, quality-model 20 — all passing on real content, not vacuous.
  • The sibling import ./skills-conformance-check in the conformance test is unaffected (module + test moved together).

Wiring: package scripts check:links / transfer:dataset / skills:conformance now point at src/tools/<file>.ts; root delegation unchanged; CI step unchanged. Also dropped the stale src/index.ts entry from vitest.config.ts coverage exclude (no such file).

2. Review finding 2 (Minor, actionable)

eval/results/2026-07-12-baseline.md: the prose "Both ambiguities … (AD-12, PB-N2, AD-14)" contradicted its own three-id list. Reworded to "the ambiguities fall into two kinds" — the summary table's count of 3 rows was already correct; only the prose is fixed.

Findings 1, 3, 4 are non-actionable / by-design and acknowledged without code change: (1) single-model/single-pass methodology is deliberately disclosed in-file as the baseline's stated method; (3) the assess-* family-glob near-miss (AD-N3) is an intentional decoy proving the bare word "assess" over-activates the family; (4) extractLinkTargets' fenced-code-only exclusion is inherited unchanged for parity with the reviewed logic.

Gates (all green)

@pair/knowledge-hub test:   98/98 (tools/ + conformance/ suites) + check:links valid
@pair/knowledge-hub ts:check: PASS
@pair/knowledge-hub lint:    PASS
@pair/knowledge-hub prettier:check: PASS (moved files normalized)
pnpm skills:conformance (root → package): 35 skills, 3 WARN, exit 0
pnpm docs:staleness:         PASS — 35 skills, 8 commands
pnpm mdlint:check:           PASS
pre-push quality-gate:       full run on push

@rucka

rucka commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator Author

Re-review of reorg 1593ad2 — Approve

Independent re-review of the delta 7c81769..1593ad2 (role-based src/ reorg + baseline wording fix), verified in a fresh detached worktree at 1593ad2. All six checks hold; no new findings.

Verified

  1. Pure renames — CONFIRMED. git diff --find-renames reports all nine files as renames (similarity 85–100%). The only content deltas are: (a) path-depth .. fixups, (b) prettier reformatting in quality-model.test.ts (multi-line→single-line join(...), (m)=>m=>, long-line wraps — no logic change), (c) cosmetic help-text/path-ref updates, (d) the finding-2 wording fix. The checker's four checks are byte-identical in behavior — skills-conformance-check.ts changed only ROOT = join(__dirname,'..')join(__dirname,'..','..'); SKILLS_DIR = join(ROOT,'dataset','.skills') is unchanged.

  2. Path-depth fixups correct — PROVEN by running (not just reading):

    • pnpm skills:conformance (root → ts-node src/tools/skills-conformance-check.ts) → PASS, 35 skills (a broken SKILLS_DIR would silently report 0), exit 0, exactly the 3 next 33-vs-35 catalog WARNs.
    • pnpm --filter @pair/knowledge-hub test98/98 pass across all 6 suites. The three dataset tests read real content (not vacuous/ENOENT): conformance/agents-md.test.ts 1, conformance/assess-output-only.test.ts 24 (incl. the ../../../../.claude/skills mirror path), conformance/quality-model.test.ts 20 — all green after the ../../../ depth bumps. check:links and transfer:dataset resolve via src/tools/.
  3. Wiring — CONFIRMED. packages/knowledge-hub/package.json now points check:links/transfer:dataset/skills:conformance at src/tools/<file>.ts; root delegation unchanged (root package.json untouched); vitest.config.ts drops the stale 'src/index.ts' coverage exclude — and src/index.ts indeed does not exist, so the drop is justified.

  4. Sibling import — CONFIRMED. src/tools/skills-conformance-check.test.ts still import … from './skills-conformance-check' (both moved into tools/ together); no createRequire, no ../../../ cross-boundary reach reintroduced.

  5. Finding 2 — RESOLVED. baseline.md reworded to "The ambiguities fall into two kinds (AD-12, PB-N2, AD-14) — both sitting exactly where the domain noun stops discriminating…". Only other eval/ change is the README module-path reference (src/tools/skills-conformance-check.ts). No other content drift.

  6. Gates — GREEN (spot-verified): vitest 98/98, ts:check PASS, lint PASS, docs:staleness PASS (35 skills, 8 commands in sync), mdlint:check PASS.

Carryover

The three prior Minor findings remain non-blocking and unchanged (F1 AD-N3 family-glob near_miss — nonActionable/intentional; F3 single-model/single-pass methodology — nonActionable/disclosed; F4 extractLinkTargets inline-code — nonActionable/parity-inherited). Finding 2 is now fixed.

Verdict: Approved. The reorg is behavior-preserving; the SKILLS_DIR path-depth risk is empirically clean (35 skills, not 0). No critical/major issues.

@rucka

rucka commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator Author

Code Review — PR #324 (canonical /pair-process-review run)

Review Information

PR Number: #324
Author: rucka
Reviewer: Independent reviewer — canonical /pair-process-review (6-phase; phases 1–5, merge gate deferred to human)
Review Date: 2026-07-13
Story/Epic: #313 — T7 (Trigger/effectiveness eval harness + static conformance gate)
Review Type: Testing (harness + CI gate) — includes a two-round restructure of the conformance logic
Head reviewed: 1593ad2 (current PR head)

This is the process-of-record review. Two prior ad-hoc independent reviews (Approved with Comments @ 7c81769; Approved @ 1593ad2) exist as PR comments; this run re-derives the verdict from AC + diff + code and re-runs every gate in a fresh detached worktree pinned to 1593ad2. Prior findings are treated as review artifacts and re-verified.

Review Summary

Overall Assessment

  • Approved with Comments — no critical/major issues; all findings Minor and non-blocking.

Key Changes Summary

T7 ships: (a) eval/trigger-prompts.json — 79 prompts / 12 families; (b) eval/README.md — the runner procedure; (c) eval/results/2026-07-12-baseline.md — the committed "before" run; (d) src/tools/skills-conformance-check.{ts,test.ts} — the static conformance gate (tested module + thin CLI guard), wired into quality-gate (pre-push) and CI after docs-staleness; (e) a role-based src/ reorg (tools/ runnables + conformance/ dataset-assertion tests) carried in this PR because it introduced the new module.

Business Value Validation

Satisfies AC7 (documented eval procedure + per-family prompt set + recorded "before" evidence; the "after" half correctly deferred to T3, which this eval gates per the dependency graph) and the conformance portion of AC8 (portability, ≤1024 limits, pointer resolution, catalog counts) wired into CI with the corpus green.

Phases run

Phase Outcome
1 — Validation PR open, mergeable; story #313 loaded; type = Testing.
2 — Technical review + verify-quality All gates re-run at 1593ad2, green (table below). No critical/major.
3 — Adoption compliance No new deps/libraries/architecture. No ADR required. Conformance correctly scoped to dataset/.skills (portable source), not the assistant-specific .claude/skills mirror. PASS.
4 — Completeness (verify-done + assess-debt) T7-scope DoD met; debt surfaced (output-only, non-blocking).
5 — Decision Approved with Comments.
6 — Merge NOT run — human merge gate.

Verification performed (independent, at head 1593ad2)

Check Result
pnpm --filter @pair/knowledge-hub test 98/98 pass — 6 suites (tools/ + conformance/), incl. skills-conformance-check.test.ts 18/18; check:links valid
pnpm --filter @pair/knowledge-hub ts:check PASS (strict/NodeNext)
pnpm --filter @pair/knowledge-hub lint PASS
pnpm --filter @pair/knowledge-hub mdlint:check PASS
pnpm skills:conformance (root → package delegation) PASS — 35 skills, exactly 3 WARN (next 33-vs-35), exit 0
pnpm docs:staleness PASS — 35 skills, 8 commands in sync
Negative test (fabricated corpus: assistant field + 1100-char desc + dead pointer) 4 errors → exit 1 — the hard rules genuinely fail the gate

Prompt-set validated programmatically against the live corpus: 35 skills / declared corpus_size 35; 12 families; 79 prompts (53 ST / 26 SNT — matches baseline); 0 duplicate ids; every one of the 35 skills has ≥1 should-trigger prompt; every expected/near_miss_for ref resolves to a real corpus skill (or none/glob). Baseline stats internally consistent (76 PASS / 3 AMBIGUOUS / 0 FAIL; the 3 ambiguities = AD-12, AD-14, PB-N2, all real assess-* overlap). Behavioral parity of the restructure (deleted scripts/*.js → module) and the path-depth .. fixups from the reorg were confirmed by the live 35-skill run reporting 35 (a broken SKILLS_DIR would report 0) and by all 6 suites reading real fixtures.

Detailed Review Comments

Positive

  • Gate is fail-closed on the rules that matter: assistant-specific frontmatter, over-limit name/description, combined>1024, and dead pointers each produce exit 1 (verified live), while the one known drift (next 33-vs-35) is WARN-only with TODO(#313/T1) — the right split.
  • Restructure is genuinely behavior-preserving: logic + test are now siblings, the CLI is a thin caller of exported primitives, single source of truth (scripts/skills-conformance-check.js deleted, no dangling refs). The conformance CLI imports only fs/path, so the new CI step adds no build-order dependency on @pair/content-ops.
  • Prompt set is disciplined and honest: complete skill coverage, realistic phrasing, decoys that name their sibling + the correct destination, and a baseline that discloses the author==executor bias rather than hiding it.

Minor Issues (non-blocking)

  1. src/tools/skills-conformance-check.ts:68-83 (parseFrontmatter) — size gate bypassable by a legal YAML construct. [NEW — not raised by prior reviews] A block-scalar description (description: > or | + indented lines) parses to the value ">"/"|" (length ~1); the indented continuation lines are skipped as non-top-level. checkSizeLimits then measures ~1 char and passes, so an over-limit description written in block style silently satisfies AC8's ≤1024 enforcement. Not currently triggered — the whole corpus uses single-line quoted descriptions (max 528 chars), verified — so this is latent, not an active failure. Recommendation (optional, low priority): either resolve block scalars to their full value before length-checking, or fail-closed by rejecting block-scalar name/description so the size rule cannot be bypassed.
  2. src/tools/skills-conformance-check.ts:110-116 + comment L12-13 — spec-attribution inverted in messages. [NEW — minor] The message calls description≤1024 the "spec max" and the combined≤1024 bound "Pair max", but the agentskills.io/Claude constraint the story cites is the combined name+description ≤ 1024. Behavior is correct (both are enforced); only the attribution in the comment/error string is backwards. Recommendation (optional): swap the "spec"/"Pair" labels.
  3. eval/trigger-prompts.json (AD-N3) — near_miss_for: "assess-*" uses a family glob rather than a concrete sibling, unlike every other entry. nonActionable — intentional family-level decoy (expected: none), documented in the baseline; schema-consistent enough. Optional: note the glob convention in a $comment.
  4. src/tools/skills-conformance-check.ts:123-131 (extractLinkTargets) — strips fenced blocks only, not inline code spans, so a link inside `…` would still be pointer-checked. nonActionable — inherited unchanged from the deleted script (parity requirement); no active false positive (conformance PASSes clean).
  5. eval/results/2026-07-12-baseline.md — methodology is single-model, single-pass, author==executor. Self-grading with no inter-rater reliability, so 96.2% is a soft number and the before/after gate rests on disciplined manual re-runs. nonActionable — explicitly disclosed in-file; AC7 asks for a documented procedure + recorded "before" evidence, both present; appropriate for a baseline. The automatable half (conformance) is fully mechanized and gated.
  6. next/SKILL.md states a 33-skill catalog; corpus is 35 (checkpoint + grill). A real router bug — but it is T1's fix (AC1, [#313] fix: router/catalog + correctness fixes (task T1) #325), not T7's. T7's remit is detection, and it does so correctly as a non-blocking WARN with TODO(#313/T1) to promote to error post-T1. nonActionable here — by design; fixing it in this PR would be out of task scope.

Trivial: the else { /* allow importing */ } no-op at L245-247 is dead but mirrors the check-broken-links sibling convention — leave for consistency.

Carryover / re-verification of prior findings

Prior "finding 2" (baseline "Both ambiguities" vs three ids) is resolved — reworded to "fall into two kinds (AD-12, PB-N2, AD-14)". Mild residual awkwardness ("both" after a 3-id parenthetical) but the summary table's count of 3 is correct; non-blocking.

Testing Review

New tests clear, independent, well-named; the fixture-corpus e2e exercises the real runChecks including negatives (assistant-specific field, broken link, catalog warning). Primitive coverage complete. All 98 knowledge-hub tests pass. test script is vitest run && check:links (no coverage-threshold enforcement in this command), so the new module is not gated on a coverage number — acceptable for this suite's configuration.

Adoption Compliance

Level: inline (no new dependencies to resolve). No libraries/patterns/tech-stack entries added — only package.json script wiring + a CI step. No new architectural decision ⇒ no ADR (the story's ADR obligation is AC2/T2, delivered separately in #319). Conformance scope (dataset/.skills only, not the .claude/skills mirror) is correct: dataset is the portable source that must be assistant-agnostic; the mirror is intentionally Claude-specific. No mirror byte-consistency concern — this PR changes no skill content.

Tech Debt (assess-debt — output-only, non-blocking)

  • Latent size-gate bypass via block-scalar frontmatter (finding 1) — low impact/low effort; fold into a future hardening if block-style descriptions ever appear.
  • next 33-vs-35 catalog drift — already ledgered to T1/[#313] fix: router/catalog + correctness fixes (task T1) #325; the gate tracks it as WARN.
  • Manual (non-mechanized) trigger eval — inherent to LLM trigger testing; disclosed. No action.

None of these block; none warrant CHANGES-REQUESTED.

Risk Assessment

Risk Impact Probability Mitigation
Over-limit description slips past gate via block-scalar YAML Low Low Corpus uses single-line quoted descriptions; hardening noted (finding 1)
Manual eval not mechanically reproducible Low Med Documented procedure + committed evidence; the automatable half is gated
Catalog WARN masks a future real count drift Low Low Scoped to next/SKILL.md; TODO(#313/T1) to promote to error

Decision: Approved with Comments. No critical or major issues; all T7-scope acceptance criteria (AC7 + AC8-conformance) met; restructure and reorg verified behavior-preserving by re-running the full gate set, not by reading alone. Six Minor findings, all non-blocking (two new/actionable — a latent block-scalar size-gate bypass and an inverted spec label; four nonActionable/by-design). Merge remains a human gate (Phase 6 not run).

🤖 Generated with Claude Code

…tribution (review)

- parseFrontmatter folds YAML block scalars (>, |, chomping) to real length so an
  over-1024 block-style description is measured, not read as len ~1 (was bypassable)
- negative tests: >1024 block-scalar description -> violation (drives CLI exit 1) + fold unit tests
- checkSizeLimits: comment citing principle 8 (per-field 1024 = spec, combined = Pair);
  labels verified correct, not flipped

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@rucka

rucka commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator Author

Review remediation — two conformance-gate Minors (commit 1549fdc)

Finding 1 — block-scalar size-gate bypass (real correctness gap) → FIXED

parseFrontmatter read a description from its own line; a legal YAML block scalar (description: > or description: |, with indented continuation) left the value as the bare indicator (length ~1) while the indented lines were skipped as non-key lines — so an over-1024 block-style description silently PASSED the ≤1024 check, defeating AC8. Latent today (corpus is single-line quoted, max ~528 chars) but the gate must not be bypassable.

Fix (option a — measure real length, no invented rule): added foldBlockScalar + a BLOCK_SCALAR_RE (/^[|>][+-]?$/, covering >/| and -/+ chomping). When a key's inline value is a block indicator, parseFrontmatter now consumes the indented continuation lines (block ends at the first column-0 dedent = next top-level key), folds them into a single string, and resumes top-level parsing after the block. The folded text flows into checkSizeLimits, so the real length is measured. This enforces AC8 for any style without inventing a "no block scalars" portability rule the standard doesn't state — a legitimate sub-1024 block scalar still passes; an over-1024 one fails.

Negative test (test-first regression evidence): runChecks — block-scalar size-gate cannot be bypassed builds a fixture skill with description: > + a 1100-char continuation line and asserts a description is … (spec max 1024) violation with a non-empty errors list — which is exactly what drives the CLI's process.exit(1). This test FAILS against the pre-fix parser (folded value would be >, length 1, no violation) and PASSES now. Plus two parseFrontmatter unit tests confirming > and |- fold to their real content and parsing resumes at the next key.

Finding 2 — size-limit message attribution → VERIFIED CORRECT, comment-only (not flipped)

Read principle 8 first (docs/#313-authoring-standard): "the spec caps name at 64 chars and description at 1024 chars separately; Pair adopts a stricter combined ≤1024 bound for both together."

The current labels match this exactly:

  • name is N chars (spec max 64) — spec, per-field ✓
  • description is N chars (spec max 1024) — spec, per-field ✓
  • name+description is N chars combined (Pair max 1024) — Pair's stricter combined bound ✓

So the reviewer's "possibly backwards" is not borne out — the attribution is right. Per the directive ("only adjust if it genuinely contradicts principle 8; otherwise add a one-line comment so the attribution isn't mistaken again"), I did not flip any wording and added a citation comment above the checks in checkSizeLimits recording the principle-8 basis (per-field 1024 = spec; combined = Pair).

Gates (all green)

@pair/knowledge-hub test:   101/101 (conformance suite 21/21, incl. 3 new block-scalar tests) + check:links valid
@pair/knowledge-hub ts:check: PASS
@pair/knowledge-hub lint:    PASS (foldBlockScalar/parseFrontmatter under the complexity cap)
@pair/knowledge-hub prettier:check: PASS
pnpm skills:conformance (root → package): 35 skills, exit 0, same 3 next-catalog WARNs (33-vs-35, gated on #313/T1)
pnpm docs:staleness:         PASS — 35 skills, 8 commands
pnpm mdlint:check:           PASS
pre-push quality-gate:       full run on push

@rucka

rucka commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator Author

Re-review of hardening 1549fdc — Approve

Independent adversarial re-review of 1593ad2..1549fdc (block-scalar size-gate hardening + principle-8 attribution citation). Only src/tools/skills-conformance-check.{ts,test.ts} changed. Verified in a fresh detached worktree at 1549fdc — edge cases exercised by running, not just reading.

1. Block-scalar fold correctness — CONFIRMED (empirically)

BLOCK_SCALAR_RE = /^[|>][+-]?$/ + foldBlockScalar consume the indented continuation and fold to a single measurable string. Ran an adversarial probe against the new parser (and the inlined pre-fix parser). All green:

  • (a) over-1024 vs sub-1024 — a > description of 1100 chars now yields description is 1100 chars (spec max 1024) (FAIL); a 500-char > description has no violation (PASS).
  • (b) block scalar as the LAST keydescription: > with two continuation lines and no following key: fold terminates at the closing --- (the end bound), keys = [name, description], body is empty — the closing --- and block content are not swallowed.
  • (c) name as a block scalar — folded and measured; a 70-char name: > fires name is 70 chars (spec max 64).
  • (d) no over-consumptiondescription: > followed by version: 1.2.3 resumes correctly: keys = [name, description, version], version = 1.2.3 (not swallowed).
  • (e) parity — single-line quoted descriptions parse byte-identically ("Does things."Does things., body intact); the 35-skill corpus still PASSES, exit 0, same 3 next 33-vs-35 WARNs. |- chomp folds to alpha beta.

2. Negative test is genuine red→green — CONFIRMED

Ran the new fixture's frontmatter through the pre-fix (1593ad2) parser vs the new parser:

  • pre-fix: description folds to length 1 (bare >), checkSizeLimits returns no violation ⇒ the new test would be RED on the old code.
  • 1549fdc: description measures 1100, checkSizeLimits emits the description is … spec max violation ⇒ GREEN, and a non-empty errors list drives the CLI process.exit(1).

The two new parseFrontmatter unit tests assert real folded content and resume-at-next-key ([name, description, version] / value line one line two / version 0.1.0; and |-alpha beta). pnpm --filter @pair/knowledge-hub test101/101 (conformance suite 18→21).

3. Finding 2 (attribution) NOT wrongly flipped — CONFIRMED against principle 8

Standard §8 (writing-skills.mdx, line 200): "the spec caps name at 64 chars and description at 1024 chars separately; Pair adopts a stricter combined ≤1024 bound for both together" (checklist line 223 repeats this). The kept labels are therefore correct: spec max 64 (name), spec max 1024 (description), Pair max 1024 (combined). The added comment restates §8 faithfully and does not contradict the standard. Correct call to keep the labels rather than flip them.

4. Scope — CONFIRMED

Only parseFrontmatter (added foldBlockScalar + BLOCK_SCALAR_RE) and a comment in checkSizeLimits (no logic change), plus a JSDoc CLI-path fix and the three new tests. checkFrontmatterFields, extractLinkTargets/isCheckableTarget/checkLinks, checkCatalogCounts, collectSkillFiles, runChecks are untouched. ts:check, lint, check:links PASS; skills:conformance = 35 skills + 3 WARN + exit 0.

Informational (non-blocking, nonActionable)

  • BLOCK_SCALAR_RE intentionally covers the idiomatic indicators (|, >, |-/|+/>-/>+); explicit indentation-indicator or trailing-comment forms (|2, > # note) are not recognized and would under-measure. These are non-idiomatic for a one-line frontmatter field, absent from the corpus, and out of scope for a lightweight heuristic (not a full YAML parser). By design — the realistic bypass vector (bare >/|) is now closed.

Verdict: Approved. The parser hardening is correct and well-tested (genuine red→green, folds without over/under-consuming), the attribution is right per §8, scope is tight, and no regression to the other checks or the corpus.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant