feat: add GitHub Skill Sync repository preview - #3225
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
381d679 to
b1a8dfc
Compare
Patrick-Erichsen
changed the title
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: found issues before merge. Reviewed August 4, 2026, 10:09 PM ET / August 5, 2026, 02:09 UTC. ClawSweeper reviewWhat this changesThis draft adds a publisher-settings flow that lists public GitHub repositories and previews discovered skill destinations, replacements, conflicts, and unavailable entries before GitHub Skill Sync enrollment. Merge readiness⛔ Blocked until real behavior proof is added - 11 items remain Keep open for maintainer review: the draft removes the shipped GitHub-source enrollment path while its own activation remains disabled, and it is now incompatible with substantial current-main changes to the sync contract. A maintainer must also decide whether preview access intentionally expands from Official to all verified publishers. Priority: P2 Review scores
Verification
How this fits togetherPublisher settings use verified GitHub identities to enroll public repositories as source-backed skills. The proposed preview reads GitHub repository metadata and archives, then classifies each discovered skill against existing ClawHub destinations before the user proceeds to source enrollment. flowchart LR
A[Publisher settings] --> B[Verified GitHub identity]
B --> C[Public repository list]
C --> D[Repository metadata and archive]
D --> E[Skill discovery]
E --> F[Ownership and destination checks]
F --> G[Preview in settings]
G --> H[Source enrollment]
Decision needed
Why: The checked-in policy and shipped UI retain an Official boundary, while the draft changes that audience as part of a preview feature without a documented policy update. Before merge
Findings
Agent review detailsSecurityNeeds attention: The settings-specific GitHub fetch path must be reconciled with current main’s explicit public-repository authentication policy before merge. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Rebase onto current main, build preview on the shared enrollment authorization and public-GitHub fetch policy, retain the existing enrollment control until a complete enrollment handoff is ready, and document any approved eligibility change. Do we have a high-confidence way to reproduce the issue? Yes, source-reproducible: current main shows the settings form calling the shipped enrollment action, while this draft’s supplied diff replaces that control despite its stated disabled activation boundary. Is this the best way to solve the issue? No: preview can be a useful addition, but removing enrollment before the replacement enrollment contract exists is not the narrowest maintainable path; preserve the current form and reuse its shared authorization and fetch helpers. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 98a6e04e39e6. LabelsLabel changes:
Label justifications:
EvidenceSecurity concerns:
What I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
|
This pull request has been automatically marked as stale due to inactivity. |
Yigtwxx
left a comment
Yigtwxx
left a comment
There was a problem hiding this comment.
Read through this against main at a9d04bb0. Some of the structure here is genuinely nice: the classifier lives in a pure, dependency-free module with a discriminated-union destination and an assertNever in the UI, so the interesting logic is directly unit-testable; assertGitHubSkillSyncRuntimeEnabled() is the first statement in both public actions with a test proving no fetch and no runQuery happen when the rollout is off, which is a well-constructed fail-closed test rather than a nominal one; the useRef request-id guard plus the cancelled flag is a correct double guard with coverage; and pinning snapshot.commit into the preview is the right call for a "what will happen" screen.
Three things I'd want resolved before this leaves draft, in order:
buildGitHubSettingsHeadersomitsuseGitHubApp, which is opt-out rather than opt-in, so every new GitHub call authenticates as the App installation — the sibling helper ingithubSkillSync.tsdisables that explicitly and documents why. ThefetchMock-based tests can't see it.fetchGitHubSkillSourceSnapshotgained a leadingctxparameter onmain, so this branch won't type-check as-is, and the fix propagates into the handler's own ctx type.- The publisher-authorization block is a statement-for-statement fork of
getPublicGitHubSkillSourceSetupContextInternal. Two copies of an authorization boundary is the one duplication that tends to bite silently.
Six more inline: three smaller helpers duplicated from elsewhere in convex/, a source lookup by mutable repo string in the path that already detects renames, an org/personal asymmetry in login verification (plus the specific untested branches), unused pagination whose hasMore disagrees with the filtered list, "no skills found" modelled as an error on an auto-selected repo, and the silently dropped official publisher gate in settings.tsx.
| async function buildGitHubSettingsHeaders(fetcher: typeof fetch) { | ||
| return await buildGitHubApiHeaders({ | ||
| userAgent: "clawhub/github-skill-sync-settings", | ||
| fetchImpl: fetcher, | ||
| }); | ||
| } |
There was a problem hiding this comment.
This sends a GitHub App installation token to endpoints that can't use one.
buildGitHubApiHeaders treats useGitHubApp as opt-out, not opt-in — convex/lib/githubAuth.ts:48 is if (options.useGitHubApp !== false). Since this call omits the flag, every request built from it (/user/{id}, /organizations/{id}, /users/{login}/repos, /orgs/{login}/repos, /repos/{owner}/{name}, and the archive fetch behind the snapshot) authenticates as the installation.
The sibling helper 90 lines away in convex/githubSkillSync.ts:3199-3210 disables it explicitly and states why in a comment: "Installation tokens are repository-scoped and cannot reliably read an arbitrary public repository selected by a publisher." That's the same constraint this file is under — a publisher picking any of their public repos is precisely the case where the installation may not be present.
In a deployment where an installation token is available, repository listing and preview would 403/404 or silently return a truncated repo set for anything outside the installation. The unit tests can't catch it because they inject a fetchMock, so the header contents never reach a real endpoint.
| async function buildGitHubSettingsHeaders(fetcher: typeof fetch) { | |
| return await buildGitHubApiHeaders({ | |
| userAgent: "clawhub/github-skill-sync-settings", | |
| fetchImpl: fetcher, | |
| }); | |
| } | |
| async function buildGitHubSettingsHeaders(fetcher: typeof fetch) { | |
| return await buildGitHubApiHeaders({ | |
| userAgent: "clawhub/github-skill-sync-settings", | |
| fetchImpl: fetcher, | |
| // Installation tokens are repository-scoped and cannot reliably read an | |
| // arbitrary public repository selected by a publisher. | |
| useGitHubApp: false, | |
| useOAuthAppClientCredentials: true, | |
| }); | |
| } |
Reusing buildGitHubSkillSourceHeaders directly would work too, and would keep the two from drifting.
| export async function getGitHubSkillSyncPublisherContextHandler( | ||
| ctx: QueryCtx, | ||
| args: { | ||
| publisherId: Id<"publishers">; | ||
| userId: Id<"users">; | ||
| now?: number; | ||
| }, | ||
| ): Promise<PublisherContext> { | ||
| const { publisher } = await requirePublisherRole(ctx, { | ||
| publisherId: args.publisherId, | ||
| userId: args.userId, | ||
| allowed: ["admin"], | ||
| }); | ||
| if (publisher.kind === "user") { | ||
| if (publisher.linkedUserId !== args.userId) throw new ConvexError("Forbidden"); | ||
| const githubOwnerId = parseGitHubNumericId( | ||
| await getGitHubProviderAccountId(ctx, args.userId), | ||
| "Reconnect GitHub to verify your personal account", | ||
| ); | ||
| return { | ||
| publisherId: publisher._id, | ||
| publisherHandle: publisher.handle, | ||
| publisherKind: "user", | ||
| githubOwnerId, | ||
| }; | ||
| } | ||
|
|
||
| const githubOwnerId = parseGitHubNumericId( | ||
| publisher.githubOrgId, | ||
| "Connect a verified GitHub organization to this publisher", | ||
| ); | ||
| if (!publisher.githubVerifiedAt) { | ||
| throw new ConvexError("Connect a verified GitHub organization to this publisher"); | ||
| } | ||
| const membership = await ctx.db | ||
| .query("githubOrgMemberships") | ||
| .withIndex("by_user_and_github_org", (q) => | ||
| q.eq("userId", args.userId).eq("githubOrgId", githubOwnerId), | ||
| ) | ||
| .unique(); | ||
| const now = args.now ?? Date.now(); | ||
| if ( | ||
| !membership || | ||
| membership.role !== "admin" || | ||
| now - membership.syncedAt > GITHUB_ORG_MEMBERSHIP_VERIFICATION_MAX_AGE_MS |
There was a problem hiding this comment.
This is a statement-for-statement fork of getPublicGitHubSkillSourceSetupContextInternal (convex/githubSkillSync.ts:493-537 on main): same requirePublisherRole({ allowed: ["admin"] }), same publisher.linkedUserId !== userId → "Forbidden", same getGitHubProviderAccountId match, same githubOrgId + githubVerifiedAt gate, same githubOrgMemberships.by_user_and_github_org lookup with role !== "admin" and the same GITHUB_ORG_MEMBERSHIP_VERIFICATION_MAX_AGE_MS staleness check.
That's the authorization boundary for GitHub source enrollment, which makes it the worst place in the file to keep two copies — a future hardening lands in one and quietly misses the other, and nothing fails loudly when it does. Exporting the existing internal query, or lifting the shared body into convex/lib/, would keep one definition of who may enrol a repo.
Worth noting this cuts against the file's own habit elsewhere: slug resolution correctly reuses getSkillBySlugForPublisher / getSkillSlugAliasBySlugForPublisher rather than re-deriving it, so this reads like drift rather than intent.
| const snapshot = await fetchGitHubSkillSourceSnapshot( | ||
| { | ||
| repo: metadata.repo, | ||
| defaultBranch: metadata.defaultBranch, | ||
| }, | ||
| fetcher, | ||
| ); |
There was a problem hiding this comment.
This call no longer type-checks against current main. fetchGitHubSkillSourceSnapshot gained a leading ctx parameter (convex/githubSkillSync.ts:2953-2954):
async function fetchGitHubSkillSourceSnapshot(
ctx: Pick<ActionCtx, "runAction">,
{ repo, defaultBranch }: { repo: string; defaultBranch: string },
fetcher: typeof fetch = fetch,
)It needs runAction for the raster-icon validation that landed with it, and every call site on main now passes ctx first (:2585, :2779, :2865).
That also propagates to this handler's own signature: previewGitHubSkillSyncRepositoryHandler types its ctx as Pick<ActionCtx, "runQuery">, which no longer satisfies the callee. It would need Pick<ActionCtx, "runQuery" | "runAction">, and the same widening on requireActionPublisherContext if you thread it through.
Flagging it because the branch is a couple of weeks old and this is the kind of drift that only surfaces at ci:types-build time.
| function parseGitHubNumericId(value: unknown, message: string) { | ||
| const normalized = | ||
| typeof value === "number" && Number.isSafeInteger(value) && value > 0 | ||
| ? String(value) | ||
| : typeof value === "string" && /^[1-9]\d*$/.test(value.trim()) | ||
| ? value.trim() | ||
| : ""; | ||
| if (!normalized) throw new ConvexError(message); | ||
| return normalized; | ||
| } |
There was a problem hiding this comment.
Three canonical helpers get re-implemented in this file. Grouping them here rather than spreading the same note around:
parseGitHubNumericId is normalizeGitHubNumericId (convex/githubSkillSync.ts:3170) with a different name — same two branches, same /^[1-9]\d*$/. And parseRepositoryMetadata (:317 and :325 in this file) then inlines that same normalization twice more for row.id and owner.id instead of calling even this local copy, so the logic exists four times in one file.
normalizeRepo (:535) is a copy of convex/githubSkillSync.ts:3323 with the regex widened to accept https?, www., a bare github.com/ prefix, and to strip [?#]. If that widening is right — and it looks right — it belongs on the canonical function. Two parsers that disagree about which strings are valid repo references, applied to the same user input on different screens, is a bug waiting for the right paste.
clampInteger (:547) is byte-for-byte convex/githubImport.ts:885, and semantically clampInt at convex/githubSkillSync.ts:202.
None of these are wrong on their own; it's the aggregate that concerns me, given the file is otherwise disciplined about reuse.
| const source = await ctx.db | ||
| .query("githubSkillSources") | ||
| .withIndex("by_repo", (q) => q.eq("repo", args.repo)) | ||
| .unique(); |
There was a problem hiding this comment.
This resolves the existing source by the mutable repo string, in the one code path that already knows the repo may have just been renamed — previewGitHubSkillSyncRepositoryHandler computes redirected: metadata.requestedRepo.toLowerCase() !== metadata.repo.toLowerCase() and passes the post-redirect full_name in as args.repo.
main added by_github_repository_id for exactly this (convex/githubSkillSync.ts:543-546), resolving by immutable id with the repo string as fallback.
Concrete failure: publisher @acme has an enrolled source stored as acme/skills; the repo is renamed to acme/agent-skills; a different publisher previews acme/agent-skills. The by_repo lookup returns null, so source is null, so resolvePreviewDestination can never return source-conflict — and the already-enrolled skills are reported as clean new-destination / replacement. That inverts the guarantee the preview screen exists to give.
metadata.repositoryId is already in hand at the call site, so mirroring the main lookup order should be a small change.
| ): Promise<GitHubSkillSyncRepositoryListResult> { | ||
| assertGitHubSkillSyncRuntimeEnabled(); | ||
| const context = await requireActionPublisherContext(ctx, args.publisherId, authOverride); | ||
| const login = context.githubLogin ?? (await fetchVerifiedOwnerLogin(context, fetcher)); |
There was a problem hiding this comment.
The ?? here creates an asymmetry between the two publisher kinds that's worth making deliberate.
For a personal publisher, context.githubLogin is always undefined, so fetchVerifiedOwnerLogin runs and asserts id === context.githubOwnerId against /user/{id} — the login is verified against the immutable owner id on every call.
For an org, githubLogin is populated from the stored githubOrgMemberships.login, so this short-circuits and the login is never re-verified against githubOwnerId. GitHub org logins are renameable, and that row is only as fresh as GITHUB_ORG_MEMBERSHIP_VERIFICATION_MAX_AGE_MS allows. After a rename, /orgs/{stale-login}/repos either 404s or — if the old login has been claimed by someone else — lists a different org's repositories, which then get filtered by context.githubOwnerId in toRepositoryListItem and come back empty. The ownership filter saves you from showing the wrong repos, but the user just sees "No public repositories were returned for this publisher" with no way to understand why.
The org path is also the one path with no test coverage, which is how the asymmetry stayed invisible. Either drop the ?? so both kinds verify, or keep the shortcut and add a test pinning that a stale login yields a recoverable error rather than an empty list.
Other specific branches with no coverage, while this file is open: the alias-conflict arm of resolvePreviewDestination is only tested through the pure classifier, never at the DB level; parseRepositoryMetadata's row.private !== false / visibility !== "public" rejection is untested; and the stale-response race test covers the repo change but not onPublisherChange, which bumps the same request id.
| page, | ||
| perPage, | ||
| hasMore: body.length === perPage, | ||
| repositories, |
There was a problem hiding this comment.
page / perPage / hasMore are computed and returned, but the settings screen calls this once with { perPage: 100 } and renders result.repositories only — so a publisher with more than 100 public repos has no way to reach repo 101. Either wire a "Load more" or drop the three fields from the return type until something consumes them; shipping an unused pagination contract in a ReturnType<typeof action> is harder to remove later than to not add.
There's also a mismatch between the two: hasMore is derived from the raw body.length, while repositories is the filtered list. A page of 100 repos that all fail the expectedOwnerId / private / visibility checks renders zero rows with hasMore: true, and the UI shows "No public repositories were returned for this publisher" — which is misleading, since there are more pages. Deriving the empty-state copy from hasMore as well as repositories.length would separate "you have none" from "none on this page".
| if (snapshot.skills.length === 0) { | ||
| throw new ConvexError("No skills were found in that public GitHub repo."); | ||
| } |
There was a problem hiding this comment.
"This repo contains no skills" is a legitimate answer to "what would happen if I synced this repo", not a failure — but modelling it as ConvexError means the settings screen turns it into toast.error.
That lands harder than it looks because the repository list auto-selects the first selectable repo, so a publisher whose most-recently-pushed repo happens to have no SKILL.md gets an error toast immediately on opening the screen, before touching anything. An empty preview with total: 0 and the existing summary counts at zero would say the same thing without reading as a fault.
Related: archived and fork are parsed into GitHubRepositoryListItem, asserted selectable: true in the backend test, and then never rendered — and fetchVerifiedRepositoryMetadata only rejects disabled. So a publisher can select an archived fork with no signal anywhere in the flow. Either surface them as badges in the list or drop them from the type, so the data and the UI agree on what matters.
| const officialGitHubSourcePublishers = manageablePublishers.filter( | ||
| (entry) => entry.publisher.official === true, | ||
| ); | ||
| const githubSourcePublishers = manageablePublishers; |
There was a problem hiding this comment.
This drops the official publisher gate, and the PR body doesn't mention it.
Before, this was manageablePublishers.filter((entry) => entry.publisher.official === true), and the query below moved from listForManageableOfficialPublishers — which calls isOfficialPublisher (convex/githubSkillSources.ts:182) — to listForPublisher, which gates on the admin role only. Neither of the two new actions checks official either.
This may well be correct: the configure action on main no longer gates on official at all, so the UI filter could be the last holdout of a policy that has already moved. But it's a change to who is eligible to enrol a GitHub source, riding inside a PR titled "repository preview", with no test asserting the new audience. Worth a line in the description and a -settings.test.tsx case for a managed non-official publisher, so the next person reading this diff doesn't have to reconstruct whether it was intentional.
clawsweeper
Bot
added
merge-risk: 🚨 security-boundary
Patrick-Erichsen
left a comment
Patrick-Erichsen
left a comment
There was a problem hiding this comment.
This draft adds a repository-preview flow for GitHub Skill Sync. It should stay draft: current main changed the snapshot contract and public-source credential policy, while this branch would remove the only shipped enrollment control before its replacement is active.
LOC: +1775/-142 (9 files)
Findings: rebase onto the current action-context API; retain existing source enrollment until preview hands off to an implemented activation path; keep installation tokens disabled for arbitrary public repositories; decide Official-only versus all verified publishers; add real-browser proof for selection, results, conflict, and unavailable states.
Best-fix verdict: too broad and stale. Preview is useful, but it must be layered onto the current immutable snapshot/source contract rather than replacing enrollment prematurely.
Alternatives considered: keep preview read-only alongside the existing enrollment control, then remove the old path only in the later activation slice.
Code read: settings preview UI, GitHub request helpers, snapshot discovery action/query boundary, current enrollment flow, and current-main public-source header policy.
Remaining uncertainty: intended eligibility boundary and activation design.
|
This pull request has been automatically marked as stale due to inactivity. |
2 participants
Summary
Dependency boundary
Activation remains disabled until the canonical GitHub Skill Sync engine exposes the repository enrollment contract. This PR does not enumerate skills from skills.sh, create another mirror, run scans, change shared schema, alter ranking, or mutate production.
Tests
bunx vitest run src/routes/-settings.test.tsx convex/githubSkillSyncSettings.test.ts convex/lib/githubSkillSyncSettings.test.ts(39 passed)bun run ci:unit(5,096 passed, 1 skipped)bun run ci:staticbun run ci:types-buildbunx convex dev --once --typecheck=disableagainst locallocal-amantus-clawdhub-2080