feat(scripts): typecheck-skill-examples + fix 8 skill imports#957
Merged
Conversation
Adds scripts/typecheck-skill-examples.ts — extracts every fenced TypeScript block from skills/**/*.md, compiles each as a standalone module against the published @adcp/client types, and fails on new typecheck errors. Catches the same drift class that landed PR #945 (server.registerTool) at the cost of a single CI step (~5s, $0). The harness immediately found a real bug across 8 skill files: imports of verifyApiKey, verifyBearer, anyOf (and bridgeFromTestControllerStore in seller) coming from '@adcp/client' (top-level) instead of '@adcp/client/server'. The seller skill had it right; the other 7 had drifted. Agents copy-pasting those examples would compile-fail with "Module has no exported member" — the build-creative-agent skill matrix run that surfaced this in PR #945 explains why so many creative pairs hit auth=401 earlier. How it works: - Walks skills/**/*.md and extracts every ```typescript / ```ts block - Classifies each as "full module" (has imports + a serve()/createAdcpServer()/ createIdempotencyStore()/createComplyController() entry-point) or "fragment" (object literal, partial handler — won't parse standalone) - Writes full modules to .cache/skill-examples/<flattened>.ts and runs tsc --noEmit against a generated tsconfig that resolves @adcp/client to dist/ (testing the *published* surface) - Reports errors mapped back to original .md file:line:column Baseline mode (scripts/skill-examples.baseline.json): - Captures the 142 currently-known errors so the script ships green - Mostly placeholder identifiers in pseudo-code patterns (db, lookupKey, jwksUri) and untyped ctx.store.list returns. Real drift like the import bug above is NOT baselined — those got fixed first. - New errors fail CI; baselined errors don't. Run with --update-baseline to tighten when fixes land. Skip markers: <!-- skill-example-skip: <reason> --> on the line before a ``` fence skips that block. Use sparingly. Run locally: npm run typecheck:skill-examples This is dx-expert priority #2 from the matrix-v18 review. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
8 tasks
bokelley
added a commit
that referenced
this pull request
Apr 25, 2026
…vendor pricing (#961) * feat(types): strict discriminator unions — AssetInstance, sync rows, vendor pricing The codegen emits strict per-variant interfaces (ImageAsset, CpmPricing, etc.) but not the discriminated unions over them. Handlers that returned Record<string, unknown> dodged the compile-time check and hit runtime schema validation instead. This adds three hand-authored unions on top of the generated bases so handler authors can opt into compile-time discriminator checking. Added: - src/lib/types/asset-instances.ts — AssetInstance / CommonAssetInstance / AssetInstanceType discriminated unions over the 14 generated asset-type interfaces, keyed on asset_type - src/lib/types/sync-rows.ts — SyncAccountsResponseRow + SyncGovernanceResponseRow named-type extractions of the inline accounts[] shapes, forcing the action / status literal-union discriminators at compile time - src/lib/types/asset-instances.test.ts — type-level tests using // @ts-expect-error to lock in the constraints; if a future codegen regression loosens a discriminator, the now-unexpected error fails tsc Re-export gaps closed in src/lib/index.ts: - vendor-pricing: PerUnitPricing, CustomPricing, VendorPricing, VendorPricingOption (previously only CpmPricing / PercentOfMediaPricing / FlatFeePricing were exported) - product-pricing: CPMPricingOption, VCPMPricingOption, CPCPricingOption, CPCVPricingOption, CPVPricingOption, CPPPricingOption, FlatRatePricingOption, TimeBasedPricingOption This is dx-expert priority #3 from the matrix-v18 review (CI defenses #1 and #2 shipped in #945 and #957). Catches the same drift class the matrix catches at runtime — discriminator omission, missing required fields, wrong-shaped factory objects — at compile time. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(types): code-review fixes — wire type-checks into typecheck Code reviewer found three blockers in the previous commit: 1. The type-test file was excluded from typecheck. tsconfig.json and tsconfig.lib.json both excluded **/*.test.ts; npm test runs node's test runner against .test.js only. The @ts-expect-error safety net was a no-op. Fixed: rename to *.type-checks.ts so it's part of the normal typecheck. Add explicit exclude in tsconfig.lib.json so it doesn't ship in dist. 2. asset.format on VideoAsset doesn't exist (only container_format). Once typechecked, this would have failed. Fixed both the test file and the JSDoc example in asset-instances.ts. 3. Three @ts-expect-error directives were misplaced. Bare-const-assignment placement is fragile because TS reports object-literal-required-property errors at varying line/col positions. Restructured to use the function- return pattern: the directive lands immediately above the `return` line that triggers the error, which is what TS expects. Smoke-tested: changing one of the negative-test field names to the correct field causes the @ts-expect-error to become unused and tsc fails with TS2578. The regression alarm is now real. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(types): expert-review fixes Three reviewers (dx-expert, ad-tech-protocol-expert, javascript-protocol- expert) green-lit shape but converged on six concrete improvements: - Drop CommonAssetInstance. Excludes VAST which dominates third-party- served video (CTV, Magnite/FreeWheel) — calling it "common" misleads. Removing is the cleanest path; can re-add as HostedAssetInstance later if there's demand. - Fix source-of-truth comment paths in asset-instances.ts and sync-rows.ts — header pointed to schemas/cache/{version}/bundled/... but the actual paths are schemas/cache/{version}/{creative,account}/... - Fix changeset filename reference asset-instances.test.ts → asset-instances.type-checks.ts (ships into CHANGELOG verbatim). - Add explicit exhaustiveness rail to describeAsset() switch via `const _exhaustive: never = asset` — current pattern only catches missing branches via noImplicitReturns, fragile to refactors that move returns out of switch arms. - Drop redundant `void X;` lines in favor of file-level eslint disable + a single `_references` export holding all symbols. - Surface SyncAccountsResponseRow in build-seller-agent SKILL.md alongside the existing pitfall about the action field. Without that, the type ships and nobody finds it. Smoke-tested: typecheck clean, 5854/5862 tests pass, prettier clean, typecheck-skill-examples baseline still 0 new errors. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Merged
This was referenced Apr 25, 2026
bokelley
added a commit
that referenced
this pull request
Apr 25, 2026
* feat(server): cross-domain specialism-declaration runtime check When a domain handler group (creative, signals, brandRights) is wired but capabilities.specialisms doesn't include any of that domain's specialisms, createAdcpServer now logs an error explaining the drift and pointing at the specialism IDs to add. Drift class from matrix issue #785: ~30% of "agent built every tool but storyboard reports no applicable tracks" cases trace to a missing specialism claim. The conformance runner gates tracks on the claim, so an agent with working tools but no claim grades as failing silently. Logged via logger.error (matching the idempotency-disabled precedent) rather than thrown — middleware-only test harnesses legitimately wire handlers without declaring specialisms. Production agents will see the warning in boot logs and conformance failure in the matrix. mediaBuy is intentionally exempt: its specialism choices (sales-non-guaranteed vs sales-guaranteed vs sales-broadcast-tv vs sales-social etc.) are commercially significant and may legitimately defer the declaration. The build-seller-agent skill already covers the right declaration in its cross-cutting pitfalls section. Tests in test/server-create-adcp-server.test.js lock the behavior for creative, signals, brandRights, and the mediaBuy carve-out. This is dx-expert priority #5; defenses #1–#4 shipped in #945, #957, #961, #970. Cheap-CI-defense ladder complete with this PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(server): expert-review fixes — governance + handler-detection Two convergent findings from code-reviewer and dx-expert: 1. governance domain was missing from the cross-domain check despite five governance specialisms in the spec. Per protocol-expert: coarse domain-level check is fine for v0; per-handler-subgroup mapping (createPropertyList → property-lists, calibrateContent → content-standards, etc.) is a follow-up. The coarse rule catches "governance handlers wired, no claim at all" without false positives on legitimate cross-cutting reads. 2. Object.keys(config.creative).length > 0 returned true for { listCreativeFormats: undefined } — common in spread patterns like { ...maybeHandlers }. Replaced with isWired() helper that filters to function-valued keys. Tests added for both: - governance handlers without specialism → warning - governance handlers with property-lists claim → no warning - empty creative {} → no warning - creative with all-undefined values → no warning DX-expert verdict on the runtime-vs-typed trade-off: ship. "Stop here on cheap CI defenses; pivot to skill slimming." 91 tests pass (4 new). Defense ladder #1–#5 complete. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
scripts/typecheck-skill-examples.ts— a deterministic CI harness that extracts every fenced TypeScript block fromskills/**/*.md, compiles each as a standalone module against the published@adcp/clienttypes, and fails on new typecheck errors. Catches the same class of drift that landed PR #945 (server.registerTool) at the cost of a single CI step (~5s, $0).The harness paid for itself on first run: it found a real bug across 8 skill files importing
verifyApiKey,verifyBearer,anyOf,bridgeFromTestControllerStorefrom@adcp/client(top-level) — those symbols only exist under@adcp/client/server. The seller skill had it right; 7 others had drifted. Agents copy-pasting those examples gotModule has no exported memberat compile time.This is dx-expert priority #2 from the matrix-v18 review. Priority #1 (
scripts/conformance-replay.ts) shipped in #945.What's in this PR
scripts/typecheck-skill-examples.ts— extractor, classifier, runnerscripts/skill-examples.baseline.json— captured 142 currently-known errors so CI ships green and ratchets down over timepackage.json—npm run typecheck:skill-examplesshortcut@adcp/client/serverfor the affected symbolspackage.jsonfilesso changes ship)How the harness works
skills/**/*.md, extracts every```typescript/```tsblock.import ... from '...'line and a top-levelserve(,createAdcpServer(,createIdempotencyStore(, orcreateComplyController(call. Fragments (single handler shapes, object literals showing field structure) are skipped — trying to compile them standalone produces parser-error noise that drowns out real signal..cache/skill-examples/<flattened>.ts, generates atsconfig.jsonresolving@adcp/clientto the localdist/(so we test the published surface), runstsc --noEmit..md:line:colfor navigation. Compares each error against the baseline; new errors fail CI.Skip markers:
<!-- skill-example-skip: <reason> -->on the line before a fence skips that block. Use sparingly.What's not in this PR
.github/workflows/ci.ymlas a parallel job.db,lookupKey,jwksUri) and untypedctx.store.listreturns. Tightening these is incremental work — do it as we slim skills (task Add Generated TypeScript Types for AdCP Tool Schemas (v0.2.3) #18).Verification
To verify it actually catches drift, intentionally re-introduce one of the bad imports (
import { verifyApiKey } from '@adcp/client';in a skill) and re-run — the harness will fail with a navigable error.Test plan
npm run typecheck:skill-examplesreturns exit 0 with✓ no new errorsnpm run typecheckclean (project tsc unaffected)npm run typecheck:skill-exampleslocally to confirm greenRelated
🤖 Generated with Claude Code