From 93a7e8e993991735b37f5e819fc4fcf72c95c6a7 Mon Sep 17 00:00:00 2001 From: Greg Pstrucha <875316+gricha@users.noreply.github.com> Date: Thu, 19 Feb 2026 15:47:33 -0800 Subject: [PATCH 1/4] feat(cli): Preserve original GitHub URLs in agents.toml MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, `dotagents add` normalized all GitHub source forms (SSH, HTTPS, shorthand) to `owner/repo` before storing in agents.toml. This lost the user's protocol intent — SSH users who need `git@` for private repo auth got silently switched to HTTPS cloning. Now the user's original URL is preserved in agents.toml and used for cloning. Source comparisons throughout the codebase use a new `sourcesMatch()` helper that normalizes to `owner/repo` internally. Also adds a helpful error message when HTTPS cloning fails due to authentication, suggesting the SSH URL form. Co-Authored-By: Claude Agent transcript: https://claudescope.sentry.dev/share/HvJlB3fZbyVt7PcG3LydRzvn89uTZD1uYcvyxXrXhfg --- src/cli/commands/add.ts | 33 +++++++++--------- src/cli/commands/install.ts | 6 ++-- src/cli/commands/list.ts | 3 +- src/cli/commands/remove.ts | 3 +- src/cli/commands/sync.ts | 5 +-- src/cli/commands/update.ts | 6 ++-- src/config/writer.ts | 3 +- src/skills/resolver.test.ts | 68 ++++++++++++++++++++++++++++++++++++- src/skills/resolver.ts | 32 ++++++++++++++--- src/sources/git.ts | 18 +++++++++- 10 files changed, 143 insertions(+), 34 deletions(-) diff --git a/src/cli/commands/add.ts b/src/cli/commands/add.ts index 92f93b1..efb7ffb 100644 --- a/src/cli/commands/add.ts +++ b/src/cli/commands/add.ts @@ -4,7 +4,7 @@ import chalk from "chalk"; import { loadConfig } from "../../config/loader.js"; import { isWildcardDep } from "../../config/schema.js"; import { addSkillToConfig, addWildcardToConfig } from "../../config/writer.js"; -import { parseSource, resolveSkill } from "../../skills/resolver.js"; +import { parseSource, resolveSkill, sourcesMatch } from "../../skills/resolver.js"; import { discoverAllSkills } from "../../skills/discovery.js"; import { ensureCached } from "../../sources/cache.js"; import { validateTrustedSource, TrustError } from "../../trust/index.js"; @@ -37,14 +37,14 @@ export async function runAdd(opts: AddOptions): Promise { // Parse the specifier const parsed = parseSource(specifier); - // Normalize GitHub URLs to owner/repo form for storage - const canonicalSource = - parsed.type === "github" - ? `${parsed.owner}/${parsed.repo}${parsed.ref ? `@${parsed.ref}` : ""}` + // Preserve original source form (SSH, HTTPS, or shorthand) — strip inline @ref (stored separately) + const sourceForStorage = + parsed.type === "github" && parsed.ref + ? specifier.replace(/@[^@]+$/, "") : specifier; - // Validate trust against the canonical source (owner/repo form for GitHub) - validateTrustedSource(canonicalSource, config.trust); + // Validate trust against the source + validateTrustedSource(sourceForStorage, config.trust); // Determine ref (flag overrides inline @ref) const effectiveRef = ref ?? parsed.ref; @@ -55,13 +55,13 @@ export async function runAdd(opts: AddOptions): Promise { throw new AddError("Cannot use --all with --name. Use one or the other."); } - if (config.skills.some((s) => isWildcardDep(s) && s.source === canonicalSource)) { + if (config.skills.some((s) => isWildcardDep(s) && sourcesMatch(s.source, sourceForStorage))) { throw new AddError( - `A wildcard entry for "${canonicalSource}" already exists in agents.toml.`, + `A wildcard entry for "${sourceForStorage}" already exists in agents.toml.`, ); } - await addWildcardToConfig(configPath, canonicalSource, { + await addWildcardToConfig(configPath, sourceForStorage, { ...(effectiveRef ? { ref: effectiveRef } : {}), exclude: [], }); @@ -89,12 +89,13 @@ export async function runAdd(opts: AddOptions): Promise { } else { // Git source — clone and discover const url = parsed.url!; + const cloneUrl = parsed.cloneUrl ?? url; const cacheKey = parsed.type === "github" ? `${parsed.owner}/${parsed.repo}` : url.replace(/^https?:\/\//, "").replace(/\.git$/, ""); - const cached = await ensureCached({ url, cacheKey, ref: effectiveRef }); + const cached = await ensureCached({ url: cloneUrl, cacheKey, ref: effectiveRef }); if (nameOverride) { // User specified name, verify it exists @@ -102,8 +103,8 @@ export async function runAdd(opts: AddOptions): Promise { const found = await discoverSkill(cached.repoDir, nameOverride); if (!found) { throw new AddError( - `Skill "${nameOverride}" not found in ${canonicalSource}. ` + - `Use 'dotagents add ${canonicalSource}' without --name to see available skills.`, + `Skill "${nameOverride}" not found in ${sourceForStorage}. ` + + `Use 'dotagents add ${sourceForStorage}' without --name to see available skills.`, ); } skillName = nameOverride; @@ -111,7 +112,7 @@ export async function runAdd(opts: AddOptions): Promise { // Discover all skills and pick const skills = await discoverAllSkills(cached.repoDir); if (skills.length === 0) { - throw new AddError(`No skills found in ${canonicalSource}.`); + throw new AddError(`No skills found in ${sourceForStorage}.`); } if (skills.length === 1) { skillName = skills[0]!.meta.name; @@ -119,7 +120,7 @@ export async function runAdd(opts: AddOptions): Promise { // Multiple skills found — list them and ask user to pick with --name or --all const names = skills.map((s) => s.meta.name).sort(); throw new AddError( - `Multiple skills found in ${canonicalSource}: ${names.join(", ")}. ` + + `Multiple skills found in ${sourceForStorage}: ${names.join(", ")}. ` + `Use --name to specify which one, or --all for all skills.`, ); } @@ -135,7 +136,7 @@ export async function runAdd(opts: AddOptions): Promise { // Add to config await addSkillToConfig(configPath, skillName, { - source: canonicalSource, + source: sourceForStorage, ...(effectiveRef ? { ref: effectiveRef } : {}), }); diff --git a/src/cli/commands/install.ts b/src/cli/commands/install.ts index a5b9242..2b44a5a 100644 --- a/src/cli/commands/install.ts +++ b/src/cli/commands/install.ts @@ -9,7 +9,7 @@ import { loadLockfile } from "../../lockfile/loader.js"; import { writeLockfile } from "../../lockfile/writer.js"; import { isGitLocked } from "../../lockfile/schema.js"; import type { Lockfile, LockedSkill } from "../../lockfile/schema.js"; -import { resolveSkill, resolveWildcardSkills } from "../../skills/resolver.js"; +import { resolveSkill, resolveWildcardSkills, sourcesMatch } from "../../skills/resolver.js"; import { validateTrustedSource, TrustError } from "../../trust/index.js"; import type { ResolvedSkill } from "../../skills/resolver.js"; import { hashDirectory } from "../../utils/hash.js"; @@ -88,7 +88,7 @@ async function expandSkills( // In frozen mode, expand from lockfile — no network needed if (!lockfile) continue; for (const [name, locked] of Object.entries(lockfile.skills)) { - if (locked.source !== wDep.source) continue; + if (!sourcesMatch(locked.source, wDep.source)) continue; if (explicitNames.has(name)) continue; if (excludeSet.has(name)) continue; @@ -103,7 +103,7 @@ async function expandSkills( // Check for conflicts between different wildcards const existingSource = wildcardNames.get(name); - if (existingSource && existingSource !== wDep.source) { + if (existingSource && !sourcesMatch(existingSource, wDep.source)) { throw new InstallError( `Skill "${name}" found in both wildcard sources: "${existingSource}" and "${wDep.source}". ` + `Use an explicit [[skills]] entry or add it to one source's exclude list.`, diff --git a/src/cli/commands/list.ts b/src/cli/commands/list.ts index 6b2bb23..26d8aa2 100644 --- a/src/cli/commands/list.ts +++ b/src/cli/commands/list.ts @@ -5,6 +5,7 @@ import { loadConfig } from "../../config/loader.js"; import { isWildcardDep } from "../../config/schema.js"; import { loadLockfile } from "../../lockfile/loader.js"; import { isGitLocked } from "../../lockfile/schema.js"; +import { sourcesMatch } from "../../skills/resolver.js"; import { hashDirectory } from "../../utils/hash.js"; import { existsSync } from "node:fs"; import { resolveScope, resolveDefaultScope, ScopeError } from "../../scope.js"; @@ -47,7 +48,7 @@ export async function runList(opts: ListOptions): Promise { for (const wDep of wildcardDeps) { const excludeSet = new Set(wDep.exclude); for (const [name, locked] of Object.entries(lockfile.skills)) { - if (locked.source !== wDep.source) continue; + if (!sourcesMatch(locked.source, wDep.source)) continue; if (explicitNames.has(name)) continue; if (excludeSet.has(name)) continue; if (skillEntries.has(name)) continue; diff --git a/src/cli/commands/remove.ts b/src/cli/commands/remove.ts index 4493cb6..baba775 100644 --- a/src/cli/commands/remove.ts +++ b/src/cli/commands/remove.ts @@ -9,6 +9,7 @@ import { removeSkillFromConfig, addExcludeToWildcard } from "../../config/writer import { loadLockfile } from "../../lockfile/loader.js"; import { writeLockfile } from "../../lockfile/writer.js"; import { updateAgentsGitignore } from "../../gitignore/writer.js"; +import { sourcesMatch } from "../../skills/resolver.js"; import { resolveScope, resolveDefaultScope, ScopeError } from "../../scope.js"; import type { ScopeRoot } from "../../scope.js"; @@ -75,7 +76,7 @@ export async function runRemove(opts: RemoveOptions): Promise { const locked = lockfile?.skills[skillName]; if (locked) { const wildcardDep = config.skills.find( - (s) => isWildcardDep(s) && s.source === locked.source, + (s) => isWildcardDep(s) && sourcesMatch(s.source, locked.source), ); if (wildcardDep) { throw new WildcardSkillRemoveError(skillName, locked.source); diff --git a/src/cli/commands/sync.ts b/src/cli/commands/sync.ts index 4948311..5344c12 100644 --- a/src/cli/commands/sync.ts +++ b/src/cli/commands/sync.ts @@ -4,6 +4,7 @@ import { readdir } from "node:fs/promises"; import chalk from "chalk"; import { loadConfig } from "../../config/loader.js"; import { isWildcardDep } from "../../config/schema.js"; +import { normalizeSource } from "../../skills/resolver.js"; import { loadLockfile } from "../../lockfile/loader.js"; import { writeLockfile } from "../../lockfile/writer.js"; import { addSkillToConfig } from "../../config/writer.js"; @@ -54,10 +55,10 @@ export async function runSync(opts: SyncOptions): Promise { if (lockfile) { // Add concrete skill names from wildcard sources const wildcardSources = new Set( - config.skills.filter(isWildcardDep).map((s) => s.source), + config.skills.filter(isWildcardDep).map((s) => normalizeSource(s.source)), ); for (const [name, locked] of Object.entries(lockfile.skills)) { - if (wildcardSources.has(locked.source)) { + if (wildcardSources.has(normalizeSource(locked.source))) { declaredNames.add(name); } } diff --git a/src/cli/commands/update.ts b/src/cli/commands/update.ts index ae484a4..a19924f 100644 --- a/src/cli/commands/update.ts +++ b/src/cli/commands/update.ts @@ -7,7 +7,7 @@ import { isWildcardDep } from "../../config/schema.js"; import type { WildcardSkillDependency } from "../../config/schema.js"; import { loadLockfile } from "../../lockfile/loader.js"; import { isGitLocked } from "../../lockfile/schema.js"; -import { resolveSkill, resolveWildcardSkills } from "../../skills/resolver.js"; +import { resolveSkill, resolveWildcardSkills, sourcesMatch } from "../../skills/resolver.js"; import { validateTrustedSource, TrustError } from "../../trust/index.js"; import { hashDirectory } from "../../utils/hash.js"; import { copyDir } from "../../utils/fs.js"; @@ -75,7 +75,7 @@ export async function runUpdate(opts: UpdateOptions): Promise { if (!locked) { throw new UpdateError(`Skill "${skillName}" not found in agents.toml or lockfile.`); } - const wDep = wildcardDeps.find((w) => w.source === locked.source); + const wDep = wildcardDeps.find((w) => sourcesMatch(w.source, locked.source)); if (!wDep) { throw new UpdateError(`Skill "${skillName}" not found in agents.toml.`); } @@ -180,7 +180,7 @@ async function updateWildcardSource( // Find lockfile entries that belong to this wildcard source const lockedFromSource = Object.entries(lockfile.skills).filter( - ([name, locked]) => locked.source === wDep.source && !explicitNames.has(name), + ([name, locked]) => sourcesMatch(locked.source, wDep.source) && !explicitNames.has(name), ); // Process discovered skills (new + changed) diff --git a/src/config/writer.ts b/src/config/writer.ts index 281af29..f1e9995 100644 --- a/src/config/writer.ts +++ b/src/config/writer.ts @@ -1,6 +1,7 @@ import { readFile, writeFile } from "node:fs/promises"; import { stringify } from "smol-toml"; import type { WildcardSkillDependency, TrustConfig, McpConfig } from "./schema.js"; +import { sourcesMatch } from "../skills/resolver.js"; export interface DefaultConfigOptions { agents?: string[]; @@ -93,7 +94,7 @@ export async function addExcludeToWildcard( const isWildcard = nameLine?.trim().match(/^name\s*=\s*"\*"/); const sourceMatch = sourceLine?.trim().match(/^source\s*=\s*"([^"]+)"/); - if (isWildcard && sourceMatch && sourceMatch[1] === source && !found) { + if (isWildcard && sourceMatch && sourcesMatch(sourceMatch[1]!, source) && !found) { found = true; // Find or create exclude line const excludeIdx = blockLines.findIndex((l) => l.trim().startsWith("exclude")); diff --git a/src/skills/resolver.test.ts b/src/skills/resolver.test.ts index 7cd5ba4..e7f3475 100644 --- a/src/skills/resolver.test.ts +++ b/src/skills/resolver.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from "vitest"; -import { parseSource } from "./resolver.js"; +import { parseSource, normalizeSource, sourcesMatch } from "./resolver.js"; describe("parseSource", () => { it("parses owner/repo as github", () => { @@ -8,6 +8,7 @@ describe("parseSource", () => { expect(result.owner).toBe("anthropics"); expect(result.repo).toBe("skills"); expect(result.url).toBe("https://github.com/anthropics/skills.git"); + expect(result.cloneUrl).toBeUndefined(); expect(result.ref).toBeUndefined(); }); @@ -46,6 +47,7 @@ describe("parseSource", () => { expect(result.owner).toBe("getsentry"); expect(result.repo).toBe("skills"); expect(result.url).toBe("https://github.com/getsentry/skills.git"); + expect(result.cloneUrl).toBe("https://github.com/getsentry/skills"); expect(result.ref).toBeUndefined(); }); @@ -71,6 +73,7 @@ describe("parseSource", () => { expect(result.repo).toBe("skills"); expect(result.ref).toBe("v1.0.0"); expect(result.url).toBe("https://github.com/getsentry/skills.git"); + expect(result.cloneUrl).toBe("https://github.com/getsentry/skills"); }); it("parses SSH GitHub URL", () => { @@ -79,6 +82,7 @@ describe("parseSource", () => { expect(result.owner).toBe("getsentry"); expect(result.repo).toBe("skills"); expect(result.url).toBe("https://github.com/getsentry/skills.git"); + expect(result.cloneUrl).toBe("git@github.com:getsentry/skills"); expect(result.ref).toBeUndefined(); }); @@ -88,6 +92,7 @@ describe("parseSource", () => { expect(result.owner).toBe("getsentry"); expect(result.repo).toBe("skills"); expect(result.url).toBe("https://github.com/getsentry/skills.git"); + expect(result.cloneUrl).toBe("git@github.com:getsentry/skills.git"); }); it("parses SSH GitHub URL with @ref", () => { @@ -97,6 +102,7 @@ describe("parseSource", () => { expect(result.repo).toBe("skills"); expect(result.ref).toBe("v2.0"); expect(result.url).toBe("https://github.com/getsentry/skills.git"); + expect(result.cloneUrl).toBe("git@github.com:getsentry/skills"); }); it("parses HTTPS GitHub URL with dotted repo name", () => { @@ -114,4 +120,64 @@ describe("parseSource", () => { expect(result.repo).toBe("next.js"); expect(result.url).toBe("https://github.com/vercel/next.js.git"); }); + + it("upgrades http:// to https:// in cloneUrl", () => { + const result = parseSource("http://github.com/getsentry/skills"); + expect(result.type).toBe("github"); + expect(result.cloneUrl).toBe("https://github.com/getsentry/skills"); + }); + + it("does not set cloneUrl for owner/repo shorthand", () => { + const result = parseSource("getsentry/skills@v1.0"); + expect(result.cloneUrl).toBeUndefined(); + }); +}); + +describe("normalizeSource", () => { + it("normalizes owner/repo shorthand to itself", () => { + expect(normalizeSource("getsentry/skills")).toBe("getsentry/skills"); + }); + + it("normalizes HTTPS URL to owner/repo", () => { + expect(normalizeSource("https://github.com/getsentry/skills")).toBe("getsentry/skills"); + }); + + it("normalizes SSH URL to owner/repo", () => { + expect(normalizeSource("git@github.com:getsentry/skills.git")).toBe("getsentry/skills"); + }); + + it("normalizes HTTPS URL with .git suffix", () => { + expect(normalizeSource("https://github.com/getsentry/skills.git")).toBe("getsentry/skills"); + }); + + it("returns non-github sources unchanged", () => { + expect(normalizeSource("path:../my-skill")).toBe("path:../my-skill"); + expect(normalizeSource("git:https://example.com/repo.git")).toBe("git:https://example.com/repo.git"); + }); +}); + +describe("sourcesMatch", () => { + it("matches identical shorthand", () => { + expect(sourcesMatch("getsentry/skills", "getsentry/skills")).toBe(true); + }); + + it("matches SSH URL with shorthand", () => { + expect(sourcesMatch("git@github.com:getsentry/skills.git", "getsentry/skills")).toBe(true); + }); + + it("matches HTTPS URL with shorthand", () => { + expect(sourcesMatch("https://github.com/getsentry/skills", "getsentry/skills")).toBe(true); + }); + + it("matches SSH URL with HTTPS URL", () => { + expect(sourcesMatch("git@github.com:getsentry/skills.git", "https://github.com/getsentry/skills")).toBe(true); + }); + + it("does not match different repos", () => { + expect(sourcesMatch("getsentry/skills", "getsentry/other")).toBe(false); + }); + + it("does not match different owners", () => { + expect(sourcesMatch("getsentry/skills", "anthropics/skills")).toBe(false); + }); }); diff --git a/src/skills/resolver.ts b/src/skills/resolver.ts index 73353e0..aced102 100644 --- a/src/skills/resolver.ts +++ b/src/skills/resolver.ts @@ -44,6 +44,8 @@ export type ResolvedSkill = ResolvedGitSkill | ResolvedLocalSkill; export function parseSource(source: string): { type: "github" | "git" | "local"; url?: string; + /** Original URL to use for cloning (preserves SSH/HTTPS protocol). Undefined for owner/repo shorthand. */ + cloneUrl?: string; owner?: string; repo?: string; ref?: string; @@ -62,16 +64,22 @@ export function parseSource(source: string): { source.match(GITHUB_HTTPS_URL) || source.match(GITHUB_SSH_URL); if (githubUrlMatch) { const [, owner, repo, ref] = githubUrlMatch; + // Strip @ref suffix, upgrade http:// to https:// (no-op for SSH URLs) + const cloneUrl = (ref ? source.replace(/@[^@]+$/, "") : source).replace( + /^http:\/\//i, + "https://", + ); return { type: "github", owner, repo, ref, url: `https://github.com/${owner}/${repo}.git`, + cloneUrl, }; } - // owner/repo or owner/repo@ref + // owner/repo or owner/repo@ref — shorthand, no cloneUrl const atIdx = source.indexOf("@"); const base = atIdx !== -1 ? source.slice(0, atIdx) : source; const ref = atIdx !== -1 ? source.slice(atIdx + 1) : undefined; @@ -86,6 +94,18 @@ export function parseSource(source: string): { }; } +/** Normalize any GitHub source to owner/repo canonical form for comparison/dedup. */ +export function normalizeSource(source: string): string { + const parsed = parseSource(source); + if (parsed.type === "github") return `${parsed.owner}/${parsed.repo}`; + return source; +} + +/** Compare two source strings for equivalence (normalizes GitHub URLs to owner/repo). */ +export function sourcesMatch(a: string, b: string): boolean { + return normalizeSource(a) === normalizeSource(b); +} + /** * Resolve a skill dependency to a concrete directory on disk. */ @@ -108,6 +128,7 @@ export async function resolveSkill( // Git source (GitHub or generic git) const url = parsed.url!; + const cloneUrl = parsed.cloneUrl ?? url; const ref = dep.ref ?? parsed.ref; const cacheKey = parsed.type === "github" @@ -115,7 +136,7 @@ export async function resolveSkill( : url.replace(/^https?:\/\//, "").replace(/\.git$/, ""); const cached = await ensureCached({ - url, + url: cloneUrl, cacheKey, ref, pinnedCommit: opts?.lockedCommit, @@ -142,7 +163,7 @@ export async function resolveSkill( return { type: "git", source: dep.source, - resolvedUrl: url, + resolvedUrl: cloneUrl, resolvedPath: discovered.path, resolvedRef: ref, commit: cached.commit, @@ -186,6 +207,7 @@ export async function resolveWildcardSkills( // Git source const url = parsed.url!; + const cloneUrl = parsed.cloneUrl ?? url; const ref = dep.ref ?? parsed.ref; const cacheKey = parsed.type === "github" @@ -193,7 +215,7 @@ export async function resolveWildcardSkills( : url.replace(/^https?:\/\//, "").replace(/\.git$/, ""); const cached = await ensureCached({ - url, + url: cloneUrl, cacheKey, ref, pinnedCommit: opts?.lockedCommit, @@ -208,7 +230,7 @@ export async function resolveWildcardSkills( resolved: { type: "git" as const, source: dep.source, - resolvedUrl: url, + resolvedUrl: cloneUrl, resolvedPath: d.path, resolvedRef: ref, commit: cached.commit, diff --git a/src/sources/git.ts b/src/sources/git.ts index 45e3588..177bd31 100644 --- a/src/sources/git.ts +++ b/src/sources/git.ts @@ -27,7 +27,23 @@ export async function clone( await exec("git", args); } catch (err) { if (err instanceof ExecError) { - throw new GitError(`Failed to clone ${url}: ${err.stderr}`); + const stderr = err.stderr; + if ( + url.startsWith("https://github.com/") && + (/terminal prompts disabled/i.test(stderr) || + /could not read Username/i.test(stderr)) + ) { + // Convert https://github.com/org/repo[.git][/] → git@github.com:org/repo.git + const sshUrl = + "git@github.com:" + + url.replace(/^https:\/\/github\.com\//, "").replace(/(?:\.git)?\/*$/, ".git"); + throw new GitError( + `Failed to clone ${url}: authentication required.\n` + + `Hint: for private repos, use the SSH URL instead:\n` + + ` dotagents add ${sshUrl}`, + ); + } + throw new GitError(`Failed to clone ${url}: ${stderr}`); } throw err; } From b90f7a22283271cf6839a5b4144bf3d4fa7d068e Mon Sep 17 00:00:00 2001 From: Greg Pstrucha <875316+gricha@users.noreply.github.com> Date: Thu, 19 Feb 2026 16:06:24 -0800 Subject: [PATCH 2/4] fix(git): Avoid polynomial regex in SSH URL hint Replace `/(?:\.git)?\/*$/` with a non-backtracking alternative to fix CodeQL high-severity ReDoS alert. The regex could backtrack on strings with many '/' characters. Co-Authored-By: Claude Agent transcript: https://claudescope.sentry.dev/share/4KUqW_ZW1-8zKBCW6XUHKriRopxbS0b4adPpUxhK7c4 --- src/sources/git.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/sources/git.ts b/src/sources/git.ts index 177bd31..ab8dda3 100644 --- a/src/sources/git.ts +++ b/src/sources/git.ts @@ -34,9 +34,8 @@ export async function clone( /could not read Username/i.test(stderr)) ) { // Convert https://github.com/org/repo[.git][/] → git@github.com:org/repo.git - const sshUrl = - "git@github.com:" + - url.replace(/^https:\/\/github\.com\//, "").replace(/(?:\.git)?\/*$/, ".git"); + const path = url.replace(/^https:\/\/github\.com\//, "").replace(/\/+$/, ""); + const sshUrl = "git@github.com:" + (path.endsWith(".git") ? path : path + ".git"); throw new GitError( `Failed to clone ${url}: authentication required.\n` + `Hint: for private repos, use the SSH URL instead:\n` + From e98dc4af3fb369f7e854c3828675abbdd534ed2e Mon Sep 17 00:00:00 2001 From: Greg Pstrucha <875316+gricha@users.noreply.github.com> Date: Thu, 19 Feb 2026 16:07:33 -0800 Subject: [PATCH 3/4] fix: Handle refs containing @ when stripping from clone URLs Use `source.slice(0, -(ref.length + 1))` instead of regex `/@[^@]+$/` to strip the inline @ref from URLs. The regex only removes characters after the last @, so it fails for monorepo-style tags like `packages/foo@1.0.0`. Co-Authored-By: Claude Agent transcript: https://claudescope.sentry.dev/share/OxcFX3PWWoQ_GZIhu0fbKJUQKzYnbDrDM6y9jQv1y-0 --- src/cli/commands/add.ts | 2 +- src/skills/resolver.test.ts | 9 +++++++++ src/skills/resolver.ts | 8 +++----- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/cli/commands/add.ts b/src/cli/commands/add.ts index efb7ffb..f8f9e10 100644 --- a/src/cli/commands/add.ts +++ b/src/cli/commands/add.ts @@ -40,7 +40,7 @@ export async function runAdd(opts: AddOptions): Promise { // Preserve original source form (SSH, HTTPS, or shorthand) — strip inline @ref (stored separately) const sourceForStorage = parsed.type === "github" && parsed.ref - ? specifier.replace(/@[^@]+$/, "") + ? specifier.slice(0, -(parsed.ref.length + 1)) : specifier; // Validate trust against the source diff --git a/src/skills/resolver.test.ts b/src/skills/resolver.test.ts index e7f3475..ef33aa6 100644 --- a/src/skills/resolver.test.ts +++ b/src/skills/resolver.test.ts @@ -131,6 +131,15 @@ describe("parseSource", () => { const result = parseSource("getsentry/skills@v1.0"); expect(result.cloneUrl).toBeUndefined(); }); + + it("strips ref containing @ from cloneUrl correctly", () => { + const result = parseSource("git@github.com:org/repo@packages/foo@1.0.0"); + expect(result.type).toBe("github"); + expect(result.owner).toBe("org"); + expect(result.repo).toBe("repo"); + expect(result.ref).toBe("packages/foo@1.0.0"); + expect(result.cloneUrl).toBe("git@github.com:org/repo"); + }); }); describe("normalizeSource", () => { diff --git a/src/skills/resolver.ts b/src/skills/resolver.ts index aced102..c5407a4 100644 --- a/src/skills/resolver.ts +++ b/src/skills/resolver.ts @@ -64,11 +64,9 @@ export function parseSource(source: string): { source.match(GITHUB_HTTPS_URL) || source.match(GITHUB_SSH_URL); if (githubUrlMatch) { const [, owner, repo, ref] = githubUrlMatch; - // Strip @ref suffix, upgrade http:// to https:// (no-op for SSH URLs) - const cloneUrl = (ref ? source.replace(/@[^@]+$/, "") : source).replace( - /^http:\/\//i, - "https://", - ); + // Strip @ref suffix using known ref length, upgrade http:// to https:// (no-op for SSH URLs) + const withoutRef = ref ? source.slice(0, -(ref.length + 1)) : source; + const cloneUrl = withoutRef.replace(/^http:\/\//i, "https://"); return { type: "github", owner, From 1b0231363fbe2b161d8875faa0288fa32083291e Mon Sep 17 00:00:00 2001 From: Greg Pstrucha <875316+gricha@users.noreply.github.com> Date: Thu, 19 Feb 2026 16:21:06 -0800 Subject: [PATCH 4/4] fix(git): Remove all regex from SSH URL hint to satisfy CodeQL Replace regex-based slash stripping with a simple while loop to avoid CodeQL polynomial regex false positive on /\/+$/. Co-Authored-By: Claude --- src/sources/git.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/sources/git.ts b/src/sources/git.ts index ab8dda3..b555d31 100644 --- a/src/sources/git.ts +++ b/src/sources/git.ts @@ -34,7 +34,8 @@ export async function clone( /could not read Username/i.test(stderr)) ) { // Convert https://github.com/org/repo[.git][/] → git@github.com:org/repo.git - const path = url.replace(/^https:\/\/github\.com\//, "").replace(/\/+$/, ""); + let path = url.slice("https://github.com/".length); + while (path.endsWith("/")) path = path.slice(0, -1); const sshUrl = "git@github.com:" + (path.endsWith(".git") ? path : path + ".git"); throw new GitError( `Failed to clone ${url}: authentication required.\n` +