Conversation
…mpatibility engine Introduces @skillx/contracts as the single framework-free source of truth for SkillX domain types: package manifest schema, package kinds, release states, normalized compatibility with machine-readable reason codes, verification evidence, collection/offer/entitlement contracts, import-vs-publish separation, public-vs-protected DTOs and the payload access decision. Also extends the vitest include globs so contract tests actually run and wires the package into apps/web as TypeScript source (no build step).
…ations Adds publishers, packages, package_releases, release_verification_evidence, entitlements, collections and collection_revisions, plus a runtime schema barrel so the drizzle client sees them. Migration tooling had drifted: drizzle.config.ts pointed at a single schema file and the 0006 snapshot never learned about votes or skill_references, so generate re-emitted statements that migrations 0007 and 0008 had already applied. The schema list is now explicit and 0009 strips those stale statements while its snapshot records them, realigning the baseline.
…esolver Public catalog DTOs omit skills.content by construction, and the protected content resolver is the only thing that may release it. Search, the detail API and SSR now gate the payload through that resolver, so a protected listing cannot leak while free/public listings keep returning content and skillx use keeps working. Also splits hybrid-search.ts (was 242 LOC, over the 200 LOC rule) by moving result projection into search-result-projection.ts, and adds the web adapter over the shared compatibility engine.
…heck pnpm typecheck failed on a clean checkout with 26 errors across 9 files, so the Phase 0 contract of a zero-error typecheck was unreachable. Fixed by parsing untrusted request bodies at the boundary instead of assuming a shape, narrowing the Workers AI embedding result before reading .data, widening the generated ENVIRONMENT literal before comparing it, coercing drizzle aggregate results to numbers, and typing search results explicitly. Verified: error set before and after the earlier commits was byte-identical (26 = 26), so none of these were regressions introduced by this work.
…ation plan Records the gap analysis between the roadmap and the actual codebase (no contracts package, flat skills table, four unguarded content surfaces, no compatibility concept, single-file drizzle schema, two test files) and the Phase 0 breakdown. The legacy listing migration plan states that existing catalog rows become listings and never fabricate an immutable release.
Two public SSR loaders returned FULL skills rows (including content) as loader data, which React Router serializes into the HTML — so a protected listing featured on the home page or listed in a profile would leak its SKILL.md payload to every visitor. Both now pass rows through gateSkillRow before returning. Also replaces the previous grep-based proof with an executable guard (payload-boundary.guard.test.ts) that scans every serve-path source and fails unless each unprojected `skills` read is either boundary-gated or carries an explicit, reasoned allowlist entry. It also fails on stale allowlist entries, so the inventory cannot rot.
…he 200 LOC rule
compatibility.ts (245 LOC) and fixtures.ts (299 LOC) breached the project's 200
LOC per file constraint. Split into compatibility/{types,validate,normalize} and
fixtures/{types,manifest-fixtures,collection-fixtures}; behavior is unchanged and
all 145 tests still pass.
Also moves search boost-stat loading into search-stats.ts (hybrid-search.ts was
206 LOC) and corrects the plan docs, which described the drizzle schema input as
a glob when the implementation deliberately uses an explicit file list.
findEvidence accepted the first evidence row for a runtime whenever no release digest was supplied, so a release with a null artifact_digest could report verified from evidence that was never checked against the evaluated artifact. That contradicts the invariant that a verified badge means 'this exact digest was probed'. Evidence without a digest is now reported as unbound via a new COMPAT_EVIDENCE_UNBOUND reason code and can never promote the status. Adds tests for the unbound case in both the engine and the web adapter, and splits the semver/evidence suite out so no test module exceeds the 200 LOC rule.
…he boundary guard Routes had grown past the 200 LOC rule (home 202, profile 205, skill-detail 248, api.search 240). Loader logic moved into gated modules: featured-skills.ts, profile-skill-queries.ts, skill-detail-data.ts and search-executor.ts. Every file touched by this PR is now <=200 LOC, and api.search.ts also drops its duplicate authenticateRequest in favour of the shared helper. The boundary guard now enforces three separate invariants instead of one weak import check: unprojected skills reads must be gated or allowlisted; gating modules must actually apply a gate (not merely import it); and response paths must import a gating module. The registration route no longer echoes stored rows in its responses, so its allowlist reason is accurate rather than aspirational.
… 200 LOC api.skill-register.ts was 289 LOC. The import mechanics (single skill, repo scan, root fallback, insert+index, error mapping) now live in lib/github/skill-import.ts and the route keeps only auth, input validation and mode dispatch (95 LOC). Responses still carry identity only, so the route never echoes a stored SKILL.md payload. The boundary guard allowlist is updated to point at the module that now holds the read, keeping the allowlist honest.
The previous change reduced the POST /api/skills/register response to
{ id, slug, name, author }. That broke a named consumer: `skillx use
owner/repo/skill` (three-part) and the root-skill fallback call
handleRegisterResult -> displaySkill(res.skill), which prints skill.content,
description, category, avg_rating, risk_label and source_url. Those paths rendered
undefined instead of the SKILL.md payload.
The confirmation now returns the full row again, but passes it through the
protected payload boundary: a free/public listing keeps its payload (the CLI
contract) and a protected listing would not have one echoed. Covered by
skill-import.test.ts, which pins exactly the fields the CLI reads.
Also splits the insert/index step into skill-insert.ts so skill-import.ts stays
within the 200 LOC rule, adds a `~` alias to the vitest config so app modules can
be unit tested, and fixes a renamed-test reference in the legacy migration doc.
The unit tests prove the gate functions behave correctly and the static guard proves every serve path calls one, but nothing exercised the actual route handler against a real Response body. That was the gap flagged in review: criterion 1 was backed by unit and static-import evidence only. api.skill-detail.test.ts runs the unmodified loader with a queue-backed D1 stub and asserts on the HTTP response for a free and a protected listing. It pins the four cases that matter: anonymous and authenticated free listings keep content, and a protected listing exposes none of it to either caller. It also asserts the metadata field set and the response envelope the skill page and the CLI depend on, so a future change cannot satisfy the boundary by deleting fields.
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.
Implements Phase 0 — Foundations and protected data boundaries of the SkillX Marketplace & Agent-Native roadmap (epic #31, phase issue #32).
What shipped
Domain contracts — new
@skillx/contractspackage (framework-free TypeScript, no build step):skillx.package/v1manifest schema with fixtures; package kindsskill/hook-pack/bundle; release statesdraft/quarantined/published/yankedimportvspublishingestion contractsPersistence —
0009_brave_sally_floyd.sqladdspublishers,packages,package_releases,release_verification_evidence,entitlements,collections,collection_revisions.Authorization boundary — public DTO projections omit
contentby construction; the protected content resolver is the only path that may release it. Search, the detail API, both SSR pages, and the API/CLI consumers all go through it.The boundary denies, it does not strip
Every listing that exists today is free and public, and two named consumers read the payload: the skill
detail page renders
content, and theskillx useCLI prints it. So the resolver grants the payloadfor a free/public listing and denies it for a paid/private-capable one. Removing
contentbroadlywould have broken the live product rather than protecting it.
That distinction is load-bearing on the import path.
POST /api/skills/registeris whatskillx use owner/repo/skill(three-part) and theskillx use owner/reporoot-skill fallback call, and the CLIcalls
displaySkill(res.skill), which printscontent,description,category,avg_rating,risk_labelandsource_url. An intermediate revision returned identity only ({ id, slug, name, author }) and silently renderedundefinedfor every one of those fields.adc8699restores the fullrow through the gate instead:
gateSkillRowruns before the response, so a free/public listingkeeps its payload and a protected listing would not have one echoed. No CLI change was needed.
skill-import.test.tsnow pins that contract field-by-field, so the register path is covered by thesame authorization regression evidence as search, detail and SSR.
Phase 0 acceptance criteria cross-check
home.tsx,profile.tsx,skill-detail.tsx,api.skill-detail.ts,api.search.ts,search-result-projection.ts,skill-import.ts; enforced bypayload-boundary.guard.test.tsand exercised route-level byapi.skill-detail.test.tscontent, so the skill page andskillx useare intactdto-boundary.test.ts(10) +skill-import.test.ts(5, the CLI register contract) +api.skill-detail.test.ts(7, route-level)unknown; missing required capability isblocked, with a machine-readable reason codecompatibility.test.ts+compatibility-service.test.ts(29), asserting the status set is exactly{unknown, unsupported, declared, verified, blocked}ingestion.ts; legacy rows map to listings withreleaseId: nullfixtures/; a test asserts every group has a valid and a rejected caselegacy-listing.test.ts(8) assertscanonicalPathpreservationThe boundary is enforced by a test, not a grep
payload-boundary.guard.test.tsscans every serve-path source and enforces three separate invariants:skillsread must be gated or carry an explicit, reasoned allowlist entry;gateSkillRow/ response builders), not merely import it;It also fails on stale allowlist entries, so the inventory cannot rot. This replaced a plain
grepfor
.content, which could not catch the real failure mode: a route returning a full row(including
content) as loader data, which React Router serializes into the SSR HTML. Two routes weredoing exactly that and are fixed in
e6349e3.Security invariant:
verifiedrequires digest bindingfindEvidencepreviously accepted the first evidence row for a runtime whenever no release digest wassupplied, so a release with a null
artifact_digestcould reportverifiedfrom evidence neverchecked against the evaluated artifact. Evidence without a digest is now reported
unboundvia thenew
COMPAT_EVIDENCE_UNBOUNDreason code and can never promote the status (026dbf8). Covered in boththe engine and the web adapter.
Constraints
home202→196,profile205→180,skill-detail248→187,api.search246→~150,api.skill-register289→95) and new modules split on demand (skill-import.ts161 +skill-insert.ts71) by extracting gated loader/import helpers.drizzle.config.tsuses an explicit multi-file schema list, not a glob, becauseskill_referencesis absent from the0006snapshot and a glob would re-emit it.Not proven / deliberately deferred
api.skill-detail.test.tsruns the real, unmodified loader against a queue-backed D1 stub and asserts on the HTTP response body for a free and a protected listing. The SSR routes andapi.search.tsstill rely on unit + static-guard evidence; they are not exercised end-to-end against a stubbed D1/Vectorize.lib/vectorize/index-skill.tsand the admin-seed FTS write readskills.contentdirectly. No paid listings exist yet, so nothing leaks today; entitlement-aware indexing is a Phase 2 gate.--compatible,inspect,check).@skillx/contractsis typechecked throughapps/web's tsconfig rather than its own build step.Repairs carried in this PR
ac7a7c6). Before/after error sets were byte-identical (26 = 26), so none were regressions.generatenow reports "No schema changes".skillx useregister regression fixed (adc8699), with a dedicated regression test.72226ff:api.skill-detail.test.ts`).Verification
Migration safety was proven on a real pre-migration database. Migrations 0000–0008 were replayed
into a separate local D1, backed up as
.wrangler/backups/skillx-db-PRE-0009.sqlite(278 KB, 7 newtables absent,
skillsat 28 columns), and0009was then applied to that same database: all 7 tablesappeared and
skillsstayed at 28 columns. A seconddrizzle-kit generatereports "No schema changes".