Make the leadtype skill activate for docs authoring and review - #158
Make the leadtype skill activate for docs authoring and review#158KayleeWilliams wants to merge 3 commits into
Conversation
A skill's frontmatter description is the entire activation signal: a client
reads name + description from the discovery manifest and decides whether to
load the body. Both the repo skill and the generated docs-skill described
retrieval only ("read and search the docs"), so the most common documentation
task there is — writing, editing, reviewing, restructuring the pages — routed
elsewhere.
- Expand the repo skill description to name the authoring verbs, and document
that repository signals (a `leadtype` dependency, `docs.config.ts` /
`leadtype.config.ts`, Leadtype components/frontmatter, generated artifacts)
are sufficient context when the prompt never says "Leadtype".
- Add an authoring route to the skill body: establish the project, read the
smallest relevant topic, match sibling-page conventions, update config-owned
navigation, verify with lint + generate, inspect the flattened markdown when
agent readability is at stake.
- Broaden the generated docs-skill description the same way, and align the
AGENTS.md pointer `leadtype init` writes.
- Add activation evals. The deterministic half runs in CI: the shipped
description must name the authoring verbs and repository signals and stay
under the 1024-character discovery cap. The model-driven half
(`evals:activation`) routes 17 labelled prompts and reports precision and
recall — including negatives that are docs-shaped but in a project with no
leadtype signal, and non-docs work inside a leadtype repo, so activating on
repository signal alone scores as overreach.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change expands Leadtype documentation skill activation guidance and generated descriptions. It adds labelled activation cases, frontmatter and scoring libraries, validation tests, package scripts, and a concurrent model-routing evaluator with JSON result output. ChangesSkill activation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant EvalRunner as evals/run-activation-eval.ts
participant RepoSkill as Repository skill
participant Cases as Activation cases
participant Model as Evaluation model
participant Scorer as scoreActivation
participant Results as JSON results
EvalRunner->>RepoSkill: Load skill frontmatter
EvalRunner->>Cases: Load labelled profiles and prompts
EvalRunner->>Model: Send routing prompts
Model-->>EvalRunner: Return activate or skip
EvalRunner->>Scorer: Score collected outcomes
Scorer-->>EvalRunner: Return accuracy, recall, and precision
EvalRunner->>Results: Write metrics and case decisions
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/skills/leadtype/SKILL.md:
- Line 41: Update the “Establish the project” step to preserve the
workspace-documentation fallback from the earlier instructions: when
node_modules/leadtype is absent, use the workspace package metadata or
documentation source instead of requiring node_modules/leadtype/package.json.
Keep the installed-package version check as the preferred path when that file
exists.
In `@docs/reference/skills.mdx`:
- Line 43: Define a single discovery-input contract for activation evaluation:
update docs/reference/skills.mdx at line 43 to state that both skill.name and
skill.description inform routing if the name is part of the client contract, and
update evals/run-activation-eval.ts lines 84-106 to omit skill.name when
measuring description-only activation.
In `@evals/run-activation-eval.ts`:
- Around line 159-167: Update the ActivationOutcome construction and type in the
evaluation flow to retain cell.model and cell.run alongside case, decision, and
correct. Ensure these fields remain present when outcomes are serialized to
activation JSON, including every entry grouped through byModel.
- Around line 54-59: Update parsePositiveInt to require
Number.isSafeInteger(Number(value)) and enforce the explicit MAX_RUNS and
MAX_CONCURRENCY limits for their respective flags before evaluation cells are
created, rejecting values outside those bounds while preserving the existing
positive-integer validation and error behavior.
- Around line 109-111: Update parseDecision to trim the response and accept only
exact, case-insensitive ACTIVATE or SKIP values, throwing a descriptive error
for all other inputs instead of inferring a decision. Add regression coverage
for invalid text and mixed responses such as “SKIP, not ACTIVATE,” while
preserving valid decision handling.
In `@packages/leadtype/src/llm/skills.test.ts`:
- Around line 115-121: Update the discovery JSON handling in the test around
index and index.skills[0] to parse the file as unknown, validate that it
contains a skills array with a valid first entry including name and description,
and only then read description. Replace the unchecked type assertion with an
explicit narrowing step that fails clearly for malformed discovery output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a723f1e4-30e4-49df-bedd-bfbbe5ee15a3
📒 Files selected for processing (13)
.agents/skills/leadtype/SKILL.md.changeset/skill-activation-authoring.mddocs/paths.lock.jsondocs/reference/skills.mdxevals/EVAL-AREAS.mdevals/activation/cases.jsonevals/lib/activation.test.tsevals/lib/activation.tsevals/package.jsonevals/run-activation-eval.tspackages/leadtype/src/cli/init.tspackages/leadtype/src/llm/skills.test.tspackages/leadtype/src/llm/skills.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: pullfrog
⚠️ CI failures not shown inline (2)
GitHub Actions: CI / 0_Validate & test.txt: Make the leadtype skill activate for docs authoring and review
Conclusion: failure
##[group]Run bun run --filter leadtype test
�[36;1mbun run --filter leadtype test�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
leadtype test:
leadtype test: �[1m�[30m�[46m RUN �[49m�[39m�[22m �[36mv4.1.5 �[39m�[90m/home/runner/work/leadtype/leadtype/packages/leadtype�[39m
leadtype test:
leadtype test: �[32m✓�[39m src/llm/llm.test.ts �[2m(�[22m�[2m86 tests�[22m�[2m)�[22m�[33m 328�[2mms�[22m�[39m
leadtype test: �[32m✓�[39m src/openapi/openapi.test.ts �[2m(�[22m�[2m34 tests�[22m�[2m)�[22m�[33m 423�[2mms�[22m�[39m
leadtype test: �[32m✓�[39m src/lint/lint.test.ts �[2m(�[22m�[2m45 tests�[22m�[2m)�[22m�[33m 1185�[2mms�[22m�[39m
leadtype test: �[33m�[2m✓�[22m�[39m warns on rendered components with no flattener but not on code-block examples �[33m 338�[2mms�[22m�[39m
leadtype test: �[33m�[2m✓�[22m�[39m discovers docs.config in --src, applies mounts and lint.rules �[33m 304�[2mms�[22m�[39m
leadtype test: Converted 1 docs in 23 ms
leadtype test: Converted 2 docs in 19 ms
leadtype test: Converted 2 docs in 11 ms
leadtype test: Pruned 1 orphaned .md file(s) from /tmp/leadtype-convert-mZnbHV/public
leadtype test: Converted 2 docs in 3 ms
leadtype test: Converted 1 docs in 2 ms
leadtype test: Pruned 2 orphaned .md file(s) from /tmp/leadtype-convert-LzjUqm/public
leadtype test: Converted 1 docs in 2 ms
leadtype test: Pruned 1 orphaned .md file(s) from /tmp/leadtype-convert-kgTg6S/public
leadtype test: Converted 1 docs in 2 ms
leadtype test: Pruned 1 orphaned .md file(s) from /tmp/leadtype-convert-z6AEcy/public
leadtype test: Converted 1 docs in 1 ms
leadtype test: Error: failed to process /tmp/leadtype-convert-W04IKa/docs/broken.mdx: 3:1: Unexpected character after `<`, expected a valid JSX tag (note: to create a link in MDX, use `[text](url)`) (mdx-jsx:unexpected-character)
leadtype test: Converted 1 docs in 2 ms (1 failed)
leadtype test: Warning: prune skipped: 1 file(s) failed to convert, so the expected output set is incomplete.
leadty...
GitHub Actions: CI / Validate & test: Make the leadtype skill activate for docs authoring and review
Conclusion: failure
##[group]Run bun run --filter leadtype test
�[36;1mbun run --filter leadtype test�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
leadtype test:
leadtype test: �[1m�[30m�[46m RUN �[49m�[39m�[22m �[36mv4.1.5 �[39m�[90m/home/runner/work/leadtype/leadtype/packages/leadtype�[39m
leadtype test:
leadtype test: �[32m✓�[39m src/llm/llm.test.ts �[2m(�[22m�[2m86 tests�[22m�[2m)�[22m�[33m 328�[2mms�[22m�[39m
leadtype test: �[32m✓�[39m src/openapi/openapi.test.ts �[2m(�[22m�[2m34 tests�[22m�[2m)�[22m�[33m 423�[2mms�[22m�[39m
leadtype test: �[32m✓�[39m src/lint/lint.test.ts �[2m(�[22m�[2m45 tests�[22m�[2m)�[22m�[33m 1185�[2mms�[22m�[39m
leadtype test: �[33m�[2m✓�[22m�[39m warns on rendered components with no flattener but not on code-block examples �[33m 338�[2mms�[22m�[39m
leadtype test: �[33m�[2m✓�[22m�[39m discovers docs.config in --src, applies mounts and lint.rules �[33m 304�[2mms�[22m�[39m
leadtype test: Converted 1 docs in 23 ms
leadtype test: Converted 2 docs in 19 ms
leadtype test: Converted 2 docs in 11 ms
leadtype test: Pruned 1 orphaned .md file(s) from /tmp/leadtype-convert-mZnbHV/public
leadtype test: Converted 2 docs in 3 ms
leadtype test: Converted 1 docs in 2 ms
leadtype test: Pruned 2 orphaned .md file(s) from /tmp/leadtype-convert-LzjUqm/public
leadtype test: Converted 1 docs in 2 ms
leadtype test: Pruned 1 orphaned .md file(s) from /tmp/leadtype-convert-kgTg6S/public
leadtype test: Converted 1 docs in 2 ms
leadtype test: Pruned 1 orphaned .md file(s) from /tmp/leadtype-convert-z6AEcy/public
leadtype test: Converted 1 docs in 1 ms
leadtype test: Error: failed to process /tmp/leadtype-convert-W04IKa/docs/broken.mdx: 3:1: Unexpected character after `<`, expected a valid JSX tag (note: to create a link in MDX, use `[text](url)`) (mdx-jsx:unexpected-character)
leadtype test: Converted 1 docs in 2 ms (1 failed)
leadtype test: Warning: prune skipped: 1 file(s) failed to convert, so the expected output set is incomplete.
leadty...
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use explicit types for function parameters and return values when they enhance clarity
Preferunknownoveranywhen the type is genuinely unknown
Use const assertions (as const) for immutable values and literal types
Leverage TypeScript's type narrowing instead of type assertions
Files:
packages/leadtype/src/cli/init.tspackages/leadtype/src/llm/skills.tsevals/lib/activation.test.tspackages/leadtype/src/llm/skills.test.tsevals/run-activation-eval.tsevals/lib/activation.ts
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use meaningful variable names instead of magic numbers - extract constants with descriptive names
Use arrow functions for callbacks and short functions
Preferfor...ofloops over.forEach()and indexedforloops
Use optional chaining (?.) and nullish coalescing (??) for safer property access
Prefer template literals over string concatenation
Use destructuring for object and array assignments
Useconstby default,letonly when reassignment is needed, nevervar
Alwaysawaitpromises in async functions - don't forget to use the return value
Useasync/awaitsyntax instead of promise chains for better readability
Handle errors appropriately in async code with try-catch blocks
Don't use async functions as Promise executors
Removeconsole.log,debugger, andalertstatements from production code
ThrowErrorobjects with descriptive messages, not strings or other values
Usetry-catchblocks meaningfully - don't catch errors just to rethrow them
Prefer early returns over nested conditionals for error cases
Extract complex conditions into well-named boolean variables
Use early returns to reduce nesting
Prefer simple conditionals over nested ternary operators
Don't useeval()or assign directly todocument.cookie
Avoid spread syntax in accumulators within loops
Use top-level regex literals instead of creating them in loops
Prefer specific imports over namespace imports
Use descriptive names for functions, variables, and types for meaningful naming
Add comments for complex logic, but prefer self-documenting code
Files:
packages/leadtype/src/cli/init.tspackages/leadtype/src/llm/skills.tsevals/lib/activation.test.tspackages/leadtype/src/llm/skills.test.tsevals/run-activation-eval.tsevals/lib/activation.ts
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: Write assertions insideit()ortest()blocks
Avoid done callbacks in async tests - use async/await instead
Don't use.onlyor.skipin committed code
Keep test suites reasonably flat - avoid excessivedescribenesting
Files:
evals/lib/activation.test.tspackages/leadtype/src/llm/skills.test.ts
🧠 Learnings (1)
📚 Learning: 2026-06-09T18:30:08.038Z
Learnt from: KayleeWilliams
Repo: inthhq/leadtype PR: 97
File: .changeset/search-prototype-safety-and-scaling.md:5-5
Timestamp: 2026-06-09T18:30:08.038Z
Learning: In this repo, `.changeset/*.md` files must not start the body with an H1/first-line heading (`#`) immediately after the YAML frontmatter. The changesets tool inlines the body as bullet entries into `CHANGELOG.md` during release, and a leading `#` heading would break the generated changelog format. As a result, MD041 (`first-line-heading`) warnings for files under `.changeset/` are expected false positives and should be ignored.
Applied to files:
.changeset/skill-activation-authoring.md
🪛 LanguageTool
.agents/skills/leadtype/SKILL.md
[uncategorized] ~54-~54: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...t components carried their content into markdown instead of vanishing, that internal lin...
(MARKDOWN_NNP)
🪛 markdownlint-cli2 (0.23.1)
.changeset/skill-activation-authoring.md
[warning] 5-5: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🪛 OpenGrep (1.26.0)
evals/lib/activation.ts
[ERROR] 52-52: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 58-58: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 63-63: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🔇 Additional comments (12)
.agents/skills/leadtype/SKILL.md (3)
4-11: LGTM!
30-33: 🎯 Functional CorrectnessVerify that generic markers preserve activation precision.
docs.config.ts,docs.config.js,group:, andrelated:are generic names. A non-Leadtype project can contain them. If any one marker activates the skill, unrelated documentation work can load Leadtype guidance. Require a Leadtype-specific marker, or verify a negative case with these names but no Leadtype dependency or artifact.
31-41: 🗄️ Data Integrity & IntegrationResolve the config that owns the target docs tree.
Line 31 accepts configs anywhere in the project. Line 41 only directs the agent to root or docs-directory configs. In a monorepo, this can select the wrong navigation and docs root. Locate the authoritative config before editing, or restrict the activation rule to supported locations.
packages/leadtype/src/llm/skills.ts (1)
70-74: LGTM!packages/leadtype/src/cli/init.ts (1)
222-223: LGTM!.changeset/skill-activation-authoring.md (1)
1-5: LGTM!packages/leadtype/src/llm/skills.test.ts (1)
115-120: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRemove the duplicate JSON cast.
The code contains the same closing cast three times at Line 120. The extra statements make the test invalid TypeScript. Keep one cast statement.
Proposed fix
) - ) as { skills: { name: string; description: string }[] }; - ) as { skills: { name: string; description: string }[] }; - ) as { skills: { name: string; description: string }[] }; + ) as { skills: { name: string; description: string }[] };> Likely an incorrect or invalid review comment.docs/paths.lock.json (1)
166-166: 🗄️ Data Integrity & IntegrationRegenerate and compare the path lock hash.
docs/paths.lock.jsonstores the SHA-256 hash of the emitted Markdown body, not the raw MDX source. Confirm that generation produces26835c94d375b0e1for/docs/reference/skills; otherwise update the lockfile.evals/lib/activation.ts (1)
1-206: LGTM!evals/activation/cases.json (1)
1-137: LGTM!evals/EVAL-AREAS.md (1)
45-65: LGTM!evals/package.json (1)
14-15: LGTM!
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The deterministic guard this PR presents as its CI safety net does not run in CI. The evals workspace has no test, lint, or typecheck step in any workflow, so a future narrowing of the skill description would ship silently — which is exactly the regression the guard exists to catch.
Reviewed changes — full review of the single commit ecd4eba, covering all 13 files: the skill descriptions, the SKILL.md body additions, the init pointer, and the new activation eval surface.
- Repo skill description widened —
.agents/skills/leadtype/SKILL.mdfrontmatter now names the authoring verbs (write/edit/review/restructure/maintain) and declares that repository signals (leadtypedependency,docs.config.ts/leadtype.config.ts) suffice when the prompt never says "Leadtype". - Three new SKILL.md body sections —
## Activation(the signal list, plus an explicit "absent every one of those signals, skip this skill"), a 6-step## Writing or editing docsroute, and## Reviewing docs for agent readability. - Generated docs-skill description widened —
buildDocsSkill()inpackages/leadtype/src/llm/skills.ts:74goes from "Read and search…" to "Read, search, and maintain… and when writing, editing, reviewing, or restructuring its documentation." Guarded by a newskills.test.tscase that does run in CI. leadtype initpointer aligned — the AGENTS.md block atpackages/leadtype/src/cli/init.ts:222now names the same verbs.- New activation eval —
evals/lib/activation.ts(frontmatter reader +scoreActivationprecision/recall),evals/lib/activation.test.ts(12 deterministic guards),evals/activation/cases.json(3 project profiles, 17 labelled cases: 12 activate / 5 skip, including negatives that punish activating on repository signal alone),evals/run-activation-eval.ts, theevals:activationscripts, and section "3b" inevals/EVAL-AREAS.md. - Docs + changeset — a new "When does a skill activate?" section in
docs/reference/skills.mdx, the correspondingdocs/paths.lock.jsonhash, and a patch changeset.
Verified locally at ecd4eba: the 12 new eval tests pass (under both vitest and the root bun test the pre-commit hook uses), skills.test.ts passes 10/10, the evals workspace typechecks clean, and ultracite is clean on every changed source file. The 1024-character cap the new docs section describes is real and enforced (skills.ts:20 and skills.ts:252), so that claim is accurate.
ℹ️ CI is red at ecd4eba, but not because of this PR
Worth stating plainly so nobody burns time chasing it. The failure is src/cli.test.ts > leadtype CLI > cleans up mirrored sources when the generate pipeline fails, which trips on a leaked /tmp/leadtype-generate-<hash>.lock/ directory — a generate-lock.ts mkdir lock. 673 of 674 tests pass. Nothing in this diff touches generate, temp directories, or locking.
Technical details
# Unrelated pre-existing CI failure
## The actual failure (CI job 91845470496)
```
FAIL src/cli.test.ts > leadtype CLI > cleans up mirrored sources when the generate pipeline fails
AssertionError: expected [ Array(1) ] to deeply equal []
- []
+ [ "/tmp/leadtype-generate-24a4c3a2adcda87f.lock/" ]
at src/cli.test.ts:2239 → expect(leakedTempDirs).toEqual([])
```
## Why it is not attributable to this PR
- The diff touches skill descriptions, SKILL.md prose, `init.ts` string content, and the
`evals` workspace. It does not touch `generate`, `convertAllMdx`, temp-dir handling, or
`generate-lock.ts`.
- `leadtype lint docs` was captured in full on `b5857b4` and on `ecd4eba` and the finding
sets are identical (51 each), so the PR introduces no new docs-lint findings.
- The leaked path is a lock directory, not a mirrored source directory — consistent with a
concurrently-running `generate` test in the same file leaving its per-`outDir` mkdir lock
behind and this test's `/tmp` scan picking it up.
## Required outcome
Nothing in this PR. Re-run CI; if it recurs, the isolation bug belongs in its own change.ℹ️ The activation eval ships a harness but no measured baseline
The PR's premise is that the old description lost authoring prompts, and this adds the machinery to measure exactly that — but evals:activation is opt-in and no result is recorded, so there is no before/after number anywhere. Every other EVAL-AREAS.md section carries a Finding:; section 3b has none. The deterministic guard only asserts that certain substrings are present, which is a proxy for routing rather than evidence of it.
Technical details
# No recorded routing baseline for section 3b
## Affected sites
- `evals/EVAL-AREAS.md:45` — section "3b" is marked "🟡 just added" with no `Finding:` line,
unlike sections 3 and 4 which record their conclusions.
- `evals/run-activation-eval.ts:202-234` — writes `evals/results/activation-<label>.json`, but
no result is committed.
## Required outcome
A recorded precision/recall figure for the new description, so a later change can be compared
against it and so the PR's premise is evidenced rather than asserted.
## Suggested approach
Run `bun run evals:activation --label baseline-authoring` and record the headline numbers in
the section 3b prose the way sections 3 and 4 do. If the old description is still reachable,
routing both and reporting the recall delta would directly demonstrate the motivating claim.ℹ️ Nitpicks
evals/lib/activation.ts:51-92hand-rolls a YAML reader where the monorepo already depends onyamland already haspackages/leadtype/src/internal/frontmatter.tsdoing this properly. It returns a wrong value rather than throwing for several valid YAML shapes — quoted scalars keep their quote characters, a plain multi-line scalar (no>) is silently truncated to its first line, and|is folded to spaces like>instead of preserving newlines. None are hit by today'sSKILL.md, and a truncation would currently fail the verb/signal assertions loudly rather than pass, so this is maintainability rather than a live bug — but addingyamltoevals/package.jsonwould delete ~40 lines, five regexes, and four dedicated tests.evals/run-activation-eval.ts:84-106shows the router exactly one skill and asks ACTIVATE/SKIP. Real clients pick among many competing skills, so precision here is measured against an empty field and will read optimistically. Given how carefullyEVAL-AREAS.mdcaveats the other areas, that framing limit seems worth a sentence in section 3b.
Claude Opus ｜ 𝕏
Review caught a real bug: decisions were read with a substring match, so a reply of "SKIP, not ACTIVATE" — or "do not activate" — scored as ACTIVATE. That inflates recall and deflates precision in the direction you were hoping for, which is the worst way for an eval to be wrong. `parseDecision` now accepts exactly "ACTIVATE" or "SKIP", case-insensitive and trimmed, and throws on anything else rather than guessing. It moved into `lib/activation.ts` so it is covered by tests, including the replies that used to be misread. Also from review: `--runs` and `--concurrency` are bounded, since both multiply into gateway load; outcomes now carry `model` and `run` so the results file stays analyzable per model and across repeats; and the skill's "establish the project" step names the workspace fallback that the Path Priority section already defines, instead of assuming `node_modules/leadtype` exists. The activation contract is now stated once and consistently: a client reads name *and* description from the discovery manifest, so the eval passes both. The docs previously said description was the entire signal while the eval supplied both — measuring a contract that doesn't exist.
There was a problem hiding this comment.
ℹ️ No critical issues in the new commit — one robustness suggestion inline. Note that the two inline threads from the previous review are still open.
Reviewed changes — the delta since the prior pullfrog review at ecd4eba: the single new commit 7967d82, which replaces the eval's verdict parsing and applies three prose/doc adjustments.
- Exact verdict parsing replaces substring matching —
parseDecisionmoves fromrun-activation-eval.tsintoevals/lib/activation.tsand is now exported. It matches/^(activate|skip)$/iagainst the trimmed reply instead of testing for\bactivate\b, and throws on anything else. The old form scored"SKIP, not ACTIVATE"as an activation, which inflated recall — this is the right correction. - Coverage for the new parser — two cases in
evals/lib/activation.test.tspin the accepted shapes and assert that four ambiguous replies (a negated verdict, a prose refusal, the echoed question, and empty) throw. 14/14 pass locally. --runsand--concurrencyare bounded — newMAX_RUNS/MAX_CONCURRENCYconstants threaded throughparsePositiveInt.- Per-outcome
modelandrunrecorded —ActivationOutcomegains both as optional fields and the results JSON now carries them, so per-model and run-to-run variance stay analyzable after the fact. - Discovery contract restated —
docs/reference/skills.mdx:43now says a client reads name and description, which matches whatbuildPromptactually feeds the router; a new JSDoc block onbuildPromptexplains why both go in. - SKILL.md "Establish the project" extended — names the workspace fallback under
packages/leadtype/for whennode_modules/leadtypeis absent.
Verified at 7967d82: the eval suite passes 14/14, and the regenerated docs/paths.lock.json hash for /docs/reference/skills is correct — recomputing it from source (convertMdxFile + hashRedirectContent, with the CLI's exact transform array) yields b0ced16c084e3ecb, matching the committed entry.
ℹ️ Nitpicks
evals/run-activation-eval.ts:63— theNumber.isSafeInteger(parsed)clause can never decide the branch. The^[1-9]\d*$guard on the line above meansparsedis only unsafe once it exceeds 2^53, which already failsparsed > maxat both call sites (maxis100and64). The bound alone is sufficient.
Claude Opus ｜ 𝕏
| temperature: 0, | ||
| }) | ||
| ); | ||
| const decision = parseDecision(result.text); |
There was a problem hiding this comment.
Throwing here takes the whole matrix down over one bad reply. runPool awaits its workers under Promise.all (evals/lib/pool.ts:26), withRetry wraps only the generateText call and retries transient gateway errors only (evals/lib/retry.ts:1-12), and the results file isn't written until line 228 — so a single non-conforming response rejects main() and discards every completed cell, which for evals:activation:full is 340 gateway calls with nothing persisted. The sibling runner deliberately does the opposite: judgeAnswer documents "Never throws … instead of crashing the whole matrix" (evals/lib/judge.ts:121-126), and invokeAgent records a failed call as an error on its own cell (run-llms-eval.ts:219-222).
Technical details
# A malformed router reply aborts the entire activation run
## Affected sites
- `evals/run-activation-eval.ts:175` — `parseDecision(result.text)` is called inside the
`runPool` worker with no `try`/`catch`.
- `evals/lib/pool.ts:26` — `await Promise.all(workers)`, so one rejected worker rejects
`runPool` and therefore `main()`.
- `evals/run-activation-eval.ts:228` — the results JSON is written only after `runPool`
resolves, so an abort persists nothing at all.
- `evals/lib/retry.ts:1-12` — `withRetry` only retries errors matching `TRANSIENT_PATTERN`;
a parse error is terminal. It also wraps only the `generateText` call, not the parse.
## Why this is reachable
- `evals:activation:full` is 4 models × 17 cases × 5 runs = 340 calls. One stray token
anywhere in that matrix loses all of them.
- An empty `result.text` is a realistic outcome for a reasoning model whose output budget
went to reasoning tokens — the sibling runner guards for exactly that with
`result.text ?? ""` (`run-llms-eval.ts:211`). The new test already asserts that `""`
throws.
- `"ACTIVATE."` (bare verdict, trailing period) also throws and is not among the tested
shapes, despite being the most likely way a compliant-ish model deviates from
"No punctuation, no explanation".
## Required outcome
A malformed reply must cost that one cell, not the run. Whatever the run produced up to
that point should still be scored and written.
## Suggested approach
Keep the strict parse — reverting to substring matching would restore the recall inflation
this commit set out to fix. Instead catch at the call site and record the cell, following
the convention `judge.ts` already documents: count the case as incorrect (fail closed) and
carry the raw reply plus the error string into the outcome so the results file shows what
the router actually said. Surfacing a malformed-reply count in the printed summary would
keep it from being silently absorbed.
## Open questions for the human
- Should a malformed reply also be retried once against the same model before it is scored
as a miss? `withRetry` would need a non-transient retry path for that, which may be more
machinery than an opt-in eval warrants.The eval guard was not actually running. EVAL-AREAS said the deterministic half runs in CI, but the only test step is `--filter leadtype`, which selects the package of that name — this workspace is `leadtype-evals`, and nothing invoked its tests. So the guard against someone narrowing the skill description existed and never ran. Added as its own step, and the claim now says why it needs one. The skill routed to `writing.md`, which is not a topic file — the authored pages are `write-for-agents`, `frontmatter`, and `components`, so that route resolved to nothing under every Path Priority root. Discovery JSON is validated before indexing, so malformed output fails as malformed rather than as an undefined-property read.
There was a problem hiding this comment.
ℹ️ No new issues — both threads from the initial review are fixed and now resolved. Two nitpicks below. The
run-activation-eval.ts:175thread from the previous review is still open, which is the only thing holding back an approval.
Reviewed changes — the delta since the prior pullfrog review at 7967d82: the single new commit 5227964 (4 files, +25/-8), which is entirely a response to outstanding review feedback.
- The deterministic activation guard now actually gates PRs —
.github/workflows/ci.ymlgains aTest evalsstep runningbun run --filter leadtype-evals test, with a comment explaining why it needs its own step. This closes the initial review's[!IMPORTANT]finding. - The CI claim in
EVAL-AREAS.md:61is now true and explains itself — reworded to "runs in CI as its own step", plus the reason: the main test job is--filter leadtype, which selects the package of that name, and this workspace isleadtype-evals. SKILL.md:43routes to a page that exists —writing.md→write-for-agents.md.- Discovery JSON is narrowed before indexing in
skills.test.ts—JSON.parseintounknownfollowed byexpectchecks forskillsand a first entry, in place of the single type assertion.
Verified at 5227964: bun run --filter leadtype-evals test passes 46/46 across all six evals/lib/*.test.ts files, exit 0, ~560ms, with no network or .env dependency — so the new step is a real gate, and pulling in the five previously-unrun test files adds no flakiness. skills.test.ts still passes 10/10 after the narrowing refactor. docs/writing/write-for-agents.mdx exists, and the bare filename matches the pre-existing convention in the untouched ## Topic Routing list, so the prefix-less form is deliberate rather than a second bug. The activation.ts header comment claiming "Deterministic (runs in CI)" is now accurate too. docs/paths.lock.json is untouched by this commit, correctly — docs/reference/skills.mdx did not change.
ℹ️ Nitpicks
packages/leadtype/src/llm/skills.test.ts:115-128— this rewrite answers a CodeRabbit comment that CodeRabbit itself flagged as "likely an incorrect or invalid review comment" (it claimed a duplicated cast that was never in the file, which compiled and passed 10/10 throughout). The result trades one idiomatic cast for fourexpectsteps and threeascasts, so it does not actually replace assertions with narrowing — and in a test reading its own generated fixture, a malformed shape already fails loudly on the next line. The original) as { skills: { name: string; description: string }[] };read better; worth considering a revert.- Now that
evalshas a test gate, the asymmetry with lint/typecheck is a little more visible:vitest run libcoversevals/lib/, sorun-activation-eval.ts— the file this PR is actively developing — is still neither tested nor typechecked by any workflow. Pre-existing for the other runners, and reasonable to leave for a follow-up.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/skills/leadtype/SKILL.md:
- Around line 43-46: Update the documentation references in the Leadtype skill
to use the exact generated topic paths, including their source directory
prefixes, for frontmatter.md, components.md, markdown.md, and llm.md. Preserve
the existing descriptions and ensure the links resolve under the appropriate
writing/ and reference/ directories.
In `@packages/leadtype/src/llm/skills.test.ts`:
- Around line 123-128: Update the discovery index test around the skills entry
to use an explicit TypeScript type guard or assertion helper that narrows
index.skills to an array and verifies the first entry has string name and
description properties. Remove the existing type assertions, then destructure
the narrowed entry so malformed data fails at the intended validation point.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7734e8d3-511e-4d3e-a208-edce04603db9
📒 Files selected for processing (4)
.agents/skills/leadtype/SKILL.md.github/workflows/ci.ymlevals/EVAL-AREAS.mdpackages/leadtype/src/llm/skills.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: pullfrog
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use explicit types for function parameters and return values when they enhance clarity
Preferunknownoveranywhen the type is genuinely unknown
Use const assertions (as const) for immutable values and literal types
Leverage TypeScript's type narrowing instead of type assertions
Files:
packages/leadtype/src/llm/skills.test.ts
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use meaningful variable names instead of magic numbers - extract constants with descriptive names
Use arrow functions for callbacks and short functions
Preferfor...ofloops over.forEach()and indexedforloops
Use optional chaining (?.) and nullish coalescing (??) for safer property access
Prefer template literals over string concatenation
Use destructuring for object and array assignments
Useconstby default,letonly when reassignment is needed, nevervar
Alwaysawaitpromises in async functions - don't forget to use the return value
Useasync/awaitsyntax instead of promise chains for better readability
Handle errors appropriately in async code with try-catch blocks
Don't use async functions as Promise executors
Removeconsole.log,debugger, andalertstatements from production code
ThrowErrorobjects with descriptive messages, not strings or other values
Usetry-catchblocks meaningfully - don't catch errors just to rethrow them
Prefer early returns over nested conditionals for error cases
Extract complex conditions into well-named boolean variables
Use early returns to reduce nesting
Prefer simple conditionals over nested ternary operators
Don't useeval()or assign directly todocument.cookie
Avoid spread syntax in accumulators within loops
Use top-level regex literals instead of creating them in loops
Prefer specific imports over namespace imports
Use descriptive names for functions, variables, and types for meaningful naming
Add comments for complex logic, but prefer self-documenting code
Files:
packages/leadtype/src/llm/skills.test.ts
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: Write assertions insideit()ortest()blocks
Avoid done callbacks in async tests - use async/await instead
Don't use.onlyor.skipin committed code
Keep test suites reasonably flat - avoid excessivedescribenesting
Files:
packages/leadtype/src/llm/skills.test.ts
🔇 Additional comments (4)
evals/EVAL-AREAS.md (1)
61-66: 📐 Maintainability & Code QualityVerify the CI claim against the current workflow.
Lines [61]-[66] state that
lib/activation.test.tsruns in CI as a separate step. The workflow is not included in this review. Confirm that CI invokes theleadtype-evalsworkspace or an equivalent activation command, rather than only running--filter leadtype. If the step is absent, remove this claim or add the CI step before merge.#!/usr/bin/env bash set -euo pipefail rg -n -C 5 \ 'leadtype-evals|activation\.test|evals:activation|--filter leadtype' \ .github/workflows/ci.yml evals/package.json.agents/skills/leadtype/SKILL.md (2)
16-16: LGTM!Also applies to: 26-40, 47-55
41-41: 🎯 Functional CorrectnessKeep configuration discovery scoped to the selected project.
loadDocsConfigchecksleadtype.config.*at the project root first, thendocs.config.*in the selected docs directories. The activation signal does not change this resolution rule.> Likely an incorrect or invalid review comment..github/workflows/ci.yml (1)
55-61: LGTM!
| - `write-for-agents.md` / `frontmatter.md` for page structure, required frontmatter, and `related:` links. | ||
| - `components.md` for Leadtype MDX components and when each is appropriate. | ||
| - `markdown.md` for how a component flattens into agent-readable markdown. | ||
| - `llm.md` when the edit changes `llms.txt`, `AGENTS.md`, or navigation-derived artifacts. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use generated topic paths with their directory prefixes.
The authoring route names frontmatter.md, components.md, markdown.md, and llm.md as direct topic paths. Generated documentation preserves the source directories, so these pages resolve under writing/ and reference/. Use the exact generated paths.
Proposed fix
- - `write-for-agents.md` / `frontmatter.md` for page structure, required frontmatter, and `related:` links.
- - `components.md` for Leadtype MDX components and when each is appropriate.
- - `markdown.md` for how a component flattens into agent-readable markdown.
- - `llm.md` when the edit changes `llms.txt`, `AGENTS.md`, or navigation-derived artifacts.
+ - `writing/write-for-agents.md` / `writing/frontmatter.md` for page structure, required frontmatter, and `related:` links.
+ - `writing/components.md` for Leadtype MDX components and when each is appropriate.
+ - `reference/markdown.md` for how a component flattens into agent-readable markdown.
+ - `reference/llm.md` when the edit changes `llms.txt`, `AGENTS.md`, or navigation-derived artifacts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.agents/skills/leadtype/SKILL.md around lines 43 - 46, Update the
documentation references in the Leadtype skill to use the exact generated topic
paths, including their source directory prefixes, for frontmatter.md,
components.md, markdown.md, and llm.md. Preserve the existing descriptions and
ensure the links resolve under the appropriate writing/ and reference/
directories.
| expect(index).toHaveProperty("skills"); | ||
| const { skills } = index as { skills: unknown }; | ||
| expect(Array.isArray(skills)).toBe(true); | ||
| const first = (skills as unknown[])[0]; | ||
| expect(first).toHaveProperty("description"); | ||
| const { description } = first as { description: string }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Narrow the discovery entry before destructuring it.
The expect calls do not narrow unknown for TypeScript. Lines [124] and [128] use type assertions, and the test does not verify that skills[0].name and skills[0].description are strings. A malformed discovery index can therefore fail later with an unrelated error. Add an explicit type guard or assertion helper, then destructure the narrowed entry.
As per coding guidelines, use TypeScript type narrowing instead of type assertions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/leadtype/src/llm/skills.test.ts` around lines 123 - 128, Update the
discovery index test around the skills entry to use an explicit TypeScript type
guard or assertion helper that narrows index.skills to an array and verifies the
first entry has string name and description properties. Remove the existing type
assertions, then destructure the narrowed entry so malformed data fails at the
intended validation point.
Source: Coding guidelines
1 participant
Closes #150. First of a 7-PR stack for #157.
A skill's frontmatter description is the entire activation signal — a client reads name + description from the discovery manifest and decides whether to load the body. Both the repo skill and the generated docs-skill described retrieval only ("read and search the docs"), so the most common documentation task there is — writing, editing, reviewing, restructuring the pages — routed elsewhere.
What changed
leadtypedependency,docs.config.ts/leadtype.config.ts, Leadtype components or frontmatter, generated artifacts) are sufficient when the prompt never says "Leadtype".AGENTS.mdpointerleadtype initwrites is aligned with it.docs/reference/skills.mdxgains a "When does a skill activate?" section — the two rules that carry most of the benefit, and why this is routing rather than distribution.Activation evals
The deterministic half runs in CI (
evals/lib/activation.test.ts): the shipped description must name the authoring verbs and the repository signals, and stay under the 1024-character discovery cap. That catches a narrowed description without spending a token.The model-driven half (
bun run evals:activation) routes 17 labelled prompts and reports precision and recall. The negatives matter as much as the positives: two are docs-shaped prompts in a project with no leadtype signal, and two are non-docs work inside a leadtype repo — so a description that wins by activating on repository signal alone scores as overreach.