diff --git a/apps/api/src/projects/projects.controller.ts b/apps/api/src/projects/projects.controller.ts index 43b98f1..cceafd9 100644 --- a/apps/api/src/projects/projects.controller.ts +++ b/apps/api/src/projects/projects.controller.ts @@ -32,6 +32,8 @@ import { ModelRouter } from '../agents/model.router.js'; import { Vault } from '../common/vault.js'; import { JiraAdapter } from '../tracker/jira.adapter.js'; import { VcsError, normalizeInstanceUrl } from '../vcs/vcs.types.js'; +import { instanceUrlFromRemote, resolveGitLabRoot } from '../vcs/local-review.js'; +import { LocalGitAdapter } from '../vcs/local-git.adapter.js'; import { GitHubAdapter } from '../vcs/github.adapter.js'; import { GitLabAdapter } from '../vcs/gitlab.adapter.js'; @@ -105,6 +107,7 @@ export class ProjectsController { private readonly connections: ConnectionsService, private readonly knowledge: KnowledgeService, private readonly modelRouter: ModelRouter, + private readonly local: LocalGitAdapter, ) {} @Get() @@ -324,21 +327,38 @@ export class ProjectsController { // wrong. let connectedAs: string | undefined; const reviewProvider = dto.provider === 'local' ? dto.reviewProvider ?? null : null; - if (reviewProvider) { - if (!dto.token) { + if (reviewProvider === 'github' && !dto.token) { + // GitHub has no way to open a pull request except the API. + throw new BadRequestException( + 'A GitHub token is needed to open pull requests from local mode. Leave the review host ' + + 'unset to keep local mode credential-free.', + ); + } + + // A GitLab project with no token is a supported, and preferred, setup: + // push options open the merge request over the git transport. There is + // nothing to verify in that case, so nothing is checked. + if (reviewProvider && dto.token) { + // Verified against the host the repository names, not against whatever + // the adapter would default to. A check that passes against gitlab.com + // for a self-managed project is worse than no check at all. + const root = instanceUrl ?? (await this.derivedReviewHost(project.id, reviewProvider)); + if (!root) { throw new BadRequestException( - `A ${reviewProvider === 'gitlab' ? 'GitLab' : 'GitHub'} token is needed to open reviews from local mode. Leave the review host unset to keep local mode credential-free.`, + 'specd could not work out which host to open reviews on. Add a repository with an ' + + '`origin` remote first, or give the instance URL explicitly.', ); } + try { const identity = reviewProvider === 'gitlab' - ? await new GitLabAdapter(dto.token, instanceUrl ?? undefined).verify() + ? await new GitLabAdapter(dto.token, root).verify() : await new GitHubAdapter( dto.token, - instanceUrl ? `${instanceUrl}/api/v3` : undefined, + /^https:\/\/github\.com$/.test(root) ? undefined : `${root}/api/v3`, ).verify(); - connectedAs = identity.username; + connectedAs = `${identity.username} on ${root}`; } catch (err) { throw new BadRequestException( err instanceof VcsError ? err.message : err instanceof Error ? err.message : String(err), @@ -357,6 +377,31 @@ export class ProjectsController { return connectedAs ? { ok: true, connectedAs } : { ok: true }; } + /** + * The review host a project's own checkouts point at. + * + * Local mode already holds a clone, and a clone knows its origin — so the + * instance URL is something specd can read rather than something a person + * has to retype. Only *which software* the host runs cannot be read off a + * remote, which is why the provider is still chosen by hand. + */ + private async derivedReviewHost( + projectId: string, + provider: 'github' | 'gitlab', + ): Promise { + const repos = await this.repositories.list(projectId); + for (const repo of repos) { + if (!repo.localPath) continue; + const origin = await this.local.originUrl(repo.localPath); + const derived = origin ? instanceUrlFromRemote(origin) : null; + if (!derived) continue; + // A subpath-hosted instance is indistinguishable from a group by string + // alone, so GitLab is asked rather than guessed at. + return provider === 'gitlab' ? resolveGitLabRoot(derived, origin!) : derived; + } + return null; + } + @Post(':slug/connections/tracker') async connectTracker( @Param('slug') slug: string, diff --git a/apps/api/src/vcs/local-git.adapter.ts b/apps/api/src/vcs/local-git.adapter.ts index 518bb75..5217b08 100644 --- a/apps/api/src/vcs/local-git.adapter.ts +++ b/apps/api/src/vcs/local-git.adapter.ts @@ -319,6 +319,22 @@ export class LocalGitAdapter implements VcsAdapter { } } + /** + * The `origin` a registered checkout points at, if it has one. + * + * Read at connect time so a review credential can be checked against the + * host the repository actually names, rather than against whatever the + * adapter would have defaulted to. + */ + async originUrl(localPath: string): Promise { + try { + const url = await simpleGit({ baseDir: resolve(localPath) }).remote(['get-url', 'origin']); + return (url || '').trim() || null; + } catch { + return null; + } + } + async describe(path: string): Promise<{ clean: boolean; branch: string; head: string } | null> { const root = await this.repoRootOf(path); if (!root) return null; diff --git a/apps/api/src/vcs/local-review.service.ts b/apps/api/src/vcs/local-review.service.ts index 69d8776..66be2d2 100644 --- a/apps/api/src/vcs/local-review.service.ts +++ b/apps/api/src/vcs/local-review.service.ts @@ -37,7 +37,7 @@ export class LocalReviewService { if (!projectId) return null; const conn = await this.connections.get(projectId, 'vcs').catch(() => null); - if (!conn || conn.provider !== 'local' || !conn.encryptedSecret) return null; + if (!conn || conn.provider !== 'local') return null; const settings = (conn.settings ?? {}) as { reviewProvider?: string; instanceUrl?: string | null }; const provider = settings.reviewProvider; @@ -45,19 +45,21 @@ export class LocalReviewService { // A credential that cannot be decrypted is not a reason to fail a run — // the branch is the work, and the review surface is best-effort by - // construction everywhere else in this path. - const token = (() => { - try { - return this.vault.decrypt(conn.encryptedSecret!, `${projectId}:vcs`); - } catch { - return ''; - } - })(); - if (!token) return null; + // construction everywhere else in this path. Nor is its absence: naming + // the provider is enough for GitLab's push-option route. + const token = conn.encryptedSecret + ? (() => { + try { + return this.vault.decrypt(conn.encryptedSecret!, `${projectId}:vcs`); + } catch { + return null; + } + })() + : null; return { provider, - token, + token: token || null, instanceUrl: settings.instanceUrl ? normalizeInstanceUrl(settings.instanceUrl) : null, }; } diff --git a/apps/api/src/vcs/local-review.test.ts b/apps/api/src/vcs/local-review.test.ts index 606b140..21f3886 100644 --- a/apps/api/src/vcs/local-review.test.ts +++ b/apps/api/src/vcs/local-review.test.ts @@ -4,7 +4,14 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { simpleGit } from 'simple-git'; import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from 'vitest'; -import { detectHost, openLocalReview, projectPathFromRemote } from './local-review.js'; +import { + describeForPushOption, + detectHost, + instanceUrlFromRemote, + openLocalReview, + projectPathFromRemote, + resolveGitLabRoot, +} from './local-review.js'; describe('detectHost', () => { it('recognizes github and gitlab over ssh and https alike', () => { @@ -223,3 +230,146 @@ describe('a configured review credential', () => { } }); }); + +describe('instanceUrlFromRemote', () => { + it('reads the host a clone already names, in either remote syntax', () => { + expect(instanceUrlFromRemote('git@gitlab.example.com:acme/services/api.git')).toBe( + 'https://gitlab.example.com', + ); + expect(instanceUrlFromRemote('https://gitlab.example.com/acme/services/api.git')).toBe( + 'https://gitlab.example.com', + ); + }); + + it('drops an ssh port and keeps a web one', () => { + // `:2222` on an ssh remote is the SSH port. Carrying it onto an API URL + // would be confidently wrong, which is worse than asking. + expect(instanceUrlFromRemote('ssh://git@gitlab.example.com:2222/acme/api.git')).toBe( + 'https://gitlab.example.com', + ); + expect(instanceUrlFromRemote('https://gitlab.example.com:8443/acme/api.git')).toBe( + 'https://gitlab.example.com:8443', + ); + }); + + it('has no answer for a remote with no host', () => { + expect(instanceUrlFromRemote('/srv/git/bare.git')).toBeNull(); + expect(instanceUrlFromRemote('')).toBeNull(); + }); +}); + +describe('a subpath-hosted instance', () => { + const instance = 'https://intranet.example.com/gitlab'; + + it('strips the instance prefix from both remote syntaxes', () => { + // The regression: the prefix was compared against `URL.pathname`, which + // carries a leading slash, while the scp branch produced one without — + // so an ssh remote silently kept the instance path inside the project + // path, and ssh is what a corporate clone actually uses. + expect(projectPathFromRemote('https://intranet.example.com/gitlab/group/project.git', instance)).toBe( + 'group/project', + ); + expect(projectPathFromRemote('git@intranet.example.com:gitlab/group/project.git', instance)).toBe( + 'group/project', + ); + }); + + it('leaves a path alone when it merely starts with similar letters', () => { + // `gitlab-runner/...` is not inside `/gitlab`, and a prefix match without + // the separator would eat four characters off a real group name. + expect( + projectPathFromRemote('git@intranet.example.com:gitlab-runner/project.git', instance), + ).toBe('gitlab-runner/project'); + }); +}); + +describe('resolveGitLabRoot', () => { + const remote = 'git@gitlab.example.com:ET130/services/api.git'; + + it('keeps the root when GitLab answers there — the leading segment is a group', () => { + const probe = async (url: string) => (url === 'https://gitlab.example.com/api/v4/version' ? 401 : 404); + return expect(resolveGitLabRoot('https://gitlab.example.com', remote, probe)).resolves.toBe( + 'https://gitlab.example.com', + ); + }); + + it('walks one segment in when GitLab is served from a subpath', async () => { + const probe = async (url: string) => + url === 'https://gitlab.example.com/ET130/api/v4/version' ? 200 : 404; + await expect(resolveGitLabRoot('https://gitlab.example.com', remote, probe)).resolves.toBe( + 'https://gitlab.example.com/ET130', + ); + }); + + it('prefers the root, and returns it unchanged when neither answers', async () => { + // A wrong instance URL is then reported by describeApiBase404, which says + // which half to drop — better than silently addressing a group. + await expect(resolveGitLabRoot('https://gitlab.example.com', remote, async () => 404)).resolves.toBe( + 'https://gitlab.example.com', + ); + }); +}); + +describe('describeForPushOption', () => { + it('passes a short body through untouched', () => { + expect(describeForPushOption('short')).toBe('short'); + }); + + it('trims a long one and says that it did', () => { + const out = describeForPushOption('x'.repeat(4000)); + expect(out.length).toBeLessThan(1700); + expect(out).toMatch(/truncated/); + }); +}); + +describe('the push-option route against a remote that does not take them', () => { + // A local bare repo refuses push options exactly as GitLab before 11.10 + // does: it rejects the whole push and sends nothing. The branch still has to + // arrive, so the retry is what keeps a working setup working. + let dir = ''; + let remote = ''; + const git = (...args: string[]) => + execFileSync('git', args, { cwd: dir, encoding: 'utf8' as const }); + + beforeAll(() => { + dir = mkdtempSync(join(tmpdir(), 'specd-pushopt-')); + remote = mkdtempSync(join(tmpdir(), 'specd-pushopt-remote-')); + execFileSync('git', ['init', '-q', '--bare', remote]); + git('init', '-q', '-b', 'main'); + git('config', 'user.email', 't@t.dev'); + git('config', 'user.name', 'T'); + git('remote', 'add', 'origin', remote); + writeFileSync(join(dir, 'README.md'), '# x\n'); + git('add', '-A'); + git('commit', '-qm', 'init'); + git('checkout', '-q', '-b', 'spec/E-9-thing'); + writeFileSync(join(dir, 'f.txt'), 'work\n'); + git('add', '-A'); + git('commit', '-qm', 'work'); + git('checkout', '-q', 'main'); + }); + + afterAll(() => { + rmSync(dir, { recursive: true, force: true }); + rmSync(remote, { recursive: true, force: true }); + }); + + it('still publishes the branch, and says the merge request did not come from the push', async () => { + const result = await openLocalReview({ + git: simpleGit({ baseDir: dir }), + cwd: dir, + remoteUrl: 'git@gitlab.example.com:acme/services/api.git', + branch: 'spec/E-9-thing', + base: 'main', + title: '[E-9] - Thing', + body: 'body', + credential: { provider: 'gitlab', token: null, instanceUrl: null }, + }); + + expect( + execFileSync('git', ['branch', '--list', 'spec/E-9-thing'], { cwd: remote, encoding: 'utf8' }), + ).toContain('spec/E-9-thing'); + expect(result.url).toBeNull(); + expect(result.note).toMatch(/did not report a merge request from the push/); + }); +}); diff --git a/apps/api/src/vcs/local-review.ts b/apps/api/src/vcs/local-review.ts index a97559c..986b74c 100644 --- a/apps/api/src/vcs/local-review.ts +++ b/apps/api/src/vcs/local-review.ts @@ -1,5 +1,6 @@ import { spawn } from 'node:child_process'; import type { SimpleGit } from 'simple-git'; +import { fetchOrExplain } from '../common/http-failures.js'; import { GitHubAdapter } from './github.adapter.js'; import { GitLabAdapter } from './gitlab.adapter.js'; import type { LocalReviewCredential } from './vcs.types.js'; @@ -94,8 +95,40 @@ export async function openLocalReview(input: { }; } + // GitLab first, and without asking for anything: push options open the merge + // request over the git transport, so this path needs no credential, no CLI + // and no reachable API. It is the only strategy that survives an access + // portal in front of `/api/v4`. + if (kind === 'gitlab') { + const viaPush = await pushOpeningMergeRequest(cwd, { branch, base, title, body }); + if (viaPush.url) { + return { + url: viaPush.url, + note: 'pushed, and GitLab opened the merge request from the push itself — no API call, no token', + }; + } + if (viaPush.pushed) { + // The branch is published; only the announcement is missing. An older + // GitLab, or a project with merge requests disabled, lands here — and + // the API path below may still succeed where the push option did not. + if (!credential?.token) { + return { + url: null, + note: + 'pushed the branch, but GitLab did not report a merge request from the push ' + + '(push options need GitLab 11.10 or newer, and merge requests enabled on the project)', + }; + } + return openWithToken( + { ...credential, token: credential.token }, + { remoteUrl: input.remoteUrl, branch, base, title, body }, + ); + } + return { url: null, note: `pushing to origin failed (${viaPush.detail})` }; + } + const bin = kind === 'github' ? 'gh' : 'glab'; - const viaCli = !credential; + const viaCli = !credential?.token; if (viaCli && !(await hostCliReady(kind, cwd))) { return { url: null, @@ -107,13 +140,16 @@ export async function openLocalReview(input: { // token below opens the review and does nothing else — it never fetches a // file, scans a tree, or writes a commit. try { - await git.push('origin', branch); + await git.push(['--force-with-lease', 'origin', branch]); } catch (err) { return { url: null, note: `pushing to origin failed (${short(err)})` }; } - if (credential) { - return openWithToken(credential, { remoteUrl: input.remoteUrl, branch, base, title, body }); + if (credential?.token) { + return openWithToken( + { ...credential, token: credential.token }, + { remoteUrl: input.remoteUrl, branch, base, title, body }, + ); } const created = await createReview(kind, { cwd, branch, base, title, body }); @@ -131,6 +167,147 @@ export async function openLocalReview(input: { }; } +/** + * Which URL is the instance root: the host, or the host plus one path segment? + * + * `https://host/ET130/services/api` is two deployments wearing one string — + * GitLab at the root with `ET130` a group, or GitLab served from `/ET130` + * with `services/api` the project. Nothing in the remote distinguishes them, + * and both are legitimate, so specd asks instead of guessing: one unauthenticated + * `GET {candidate}/api/v4/version`, which answers 401 when GitLab is there and + * 404 when it is not. + * + * Root wins on a tie because subpath installs are rare. Returns the candidate + * unchanged when neither answers — a wrong instance URL is then reported by + * `describeApiBase404`, which says which half to drop. + */ +export async function resolveGitLabRoot( + candidateRoot: string, + remoteUrl: string, + probe: (url: string) => Promise = probeStatus, +): Promise { + if (await looksLikeGitLab(candidateRoot, probe)) return candidateRoot; + + const firstSegment = (projectPathFromRemote(remoteUrl) ?? '').split('/')[0]; + if (!firstSegment) return candidateRoot; + + const nested = `${candidateRoot}/${firstSegment}`; + return (await looksLikeGitLab(nested, probe)) ? nested : candidateRoot; +} + +async function looksLikeGitLab( + root: string, + probe: (url: string) => Promise, +): Promise { + // 401 is the *expected* answer: /version needs auth, and only a GitLab + // answers it that way. 200 covers instances that allow it unauthenticated. + const status = await probe(`${root}/api/v4/version`); + return status === 200 || status === 401; +} + +async function probeStatus(url: string): Promise { + try { + const res = await fetchOrExplain(url, {}, { host: url, wrap: (m) => new Error(m) }); + return res.status; + } catch { + return null; + } +} + +/** + * Ask GitLab to open the merge request as part of the push itself. + * + * This is the only strategy here that needs no credential and no reachable + * API. Push options travel over the git transport — the same SSH connection + * the person already pushes through every day — so it works where an access + * portal intercepts `/api/v4` and answers with a login page, which is the one + * failure a token cannot solve. + * + * GitLab announces the result in the push's remote messages, which is why + * `run()` keeps stderr. Both "created" and "an MR already exists" print a URL, + * and either is the right answer for a re-run. + * + * GitHub has no equivalent: pull requests there are API-only. + */ +async function pushOpeningMergeRequest( + cwd: string, + pr: { branch: string; base: string; title: string; body: string }, +): Promise<{ pushed: boolean; url: string | null; detail: string }> { + const { code, stderr, stdout } = await run( + 'git', + [ + 'push', + '-o', + 'merge_request.create', + '-o', + `merge_request.target=${pr.base}`, + '-o', + `merge_request.title=${pr.title}`, + '-o', + `merge_request.description=${describeForPushOption(pr.body)}`, + // The branch belongs to this spec, and a re-run resets it — the same + // reason the hosted adapters force-push. `--force-with-lease` refuses if + // the remote moved in a way we have not seen, which is the safe half of + // that bargain. + '--force-with-lease', + 'origin', + pr.branch, + ], + { cwd }, + ); + + const output = `${stderr}\n${stdout}`; + if (code === 0) { + return { pushed: true, url: firstUrl(pickMergeRequestLine(output) ?? output), detail: '' }; + } + + // A remote that does not take push options rejects the whole push and sends + // nothing — GitLab before 11.10, and every non-GitLab remote. The branch + // still has to get there, so it goes again without them and the merge + // request is left to whatever strategy comes next. + const plain = await run( + 'git', + ['push', '--force-with-lease', 'origin', pr.branch], + { cwd }, + ); + if (plain.code === 0) return { pushed: true, url: null, detail: '' }; + + return { + pushed: false, + url: null, + detail: firstLine(`${plain.stderr}\n${plain.stdout}`) || firstLine(output) || 'git push failed', + }; +} + +/** + * GitLab prints several URLs on a push. The one on the merge-request line is + * the merge request; the others are the project and, on a first push, the + * "create a merge request" form. Taking the first URL anywhere in the output + * would hand someone the form instead of the thing that was just opened. + */ +function pickMergeRequestLine(output: string): string | null { + return ( + output + .split('\n') + .find((line) => /merge[ _]request/i.test(line) && /https?:\/\//.test(line)) ?? null + ); +} + +/** + * A push option is one argv string, and a build's description is a page of + * markdown. Git imposes no limit but servers do, and a push rejected for a + * long description would lose the merge request over its own body — so it is + * trimmed, and says that it was. + */ +export function describeForPushOption(body: string, limit = 1500): string { + if (body.length <= limit) return body; + return `${body.slice(0, limit).trimEnd()}\n\n_(truncated — specd sent this description over a git push option, which cannot carry the whole body.)_`; +} + +function firstLine(output: string): string { + return output.split('\n').map((l) => l.trim()).filter(Boolean)[0] ?? ''; +} + /** * Open the review over the provider's API with the project's own token. * @@ -139,10 +316,21 @@ export async function openLocalReview(input: { * genuinely just picking which of them to call. */ async function openWithToken( - credential: LocalReviewCredential, + credential: LocalReviewCredential & { token: string }, pr: { remoteUrl: string; branch: string; base: string; title: string; body: string }, ): Promise { - const path = projectPathFromRemote(pr.remoteUrl, credential.instanceUrl); + // The host the clone already names. Without this, a blank instance URL fell + // through to the adapter's `https://gitlab.com` default and quietly sent a + // self-managed project's path and token to gitlab.com. + const root = credential.instanceUrl ?? instanceUrlFromRemote(pr.remoteUrl); + if (!root) { + return { + url: null, + note: `pushed the branch, but could not work out which host to open a review on from \`origin\` (${pr.remoteUrl})`, + }; + } + + const path = projectPathFromRemote(pr.remoteUrl, root); if (!path) { return { url: null, @@ -153,13 +341,15 @@ async function openWithToken( try { const opened = credential.provider === 'gitlab' - ? await new GitLabAdapter(credential.token, credential.instanceUrl ?? undefined).openMergeRequest( + ? await new GitLabAdapter(credential.token, root).openMergeRequest( path, { branch: pr.branch, base: pr.base, title: pr.title, body: pr.body }, ) : await new GitHubAdapter( credential.token, - credential.instanceUrl ? `${credential.instanceUrl}/api/v3` : undefined, + // github.com's API lives on its own host; an Enterprise Server + // serves it under /api/v3 on the instance itself. + /^https:\/\/github\.com$/.test(root) ? undefined : `${root}/api/v3`, ).openPullRequest(path, { branch: pr.branch, base: pr.base, @@ -188,12 +378,42 @@ async function openWithToken( * the subpath through would address a project that does not exist. */ export function projectPathFromRemote(remoteUrl: string, instanceRoot?: string | null): string | null { + let path = remotePath(remoteUrl); + if (path === null) return null; + + if (instanceRoot) { + const root = (() => { + try { + return new URL(instanceRoot).pathname.replace(/^\/+|\/+$/g, ''); + } catch { + return ''; + } + })(); + // Compared without leading slashes on either side. They used to be + // compared as-is, and `URL.pathname` has a leading slash while the + // scp-syntax branch does not — so a subpath-hosted instance stripped its + // prefix from an https remote and silently kept it on an ssh one, which is + // the syntax a corporate clone actually uses. + if (root && (path === root || path.startsWith(`${root}/`))) { + path = path.slice(root.length).replace(/^\/+/, ''); + } + } + + return path.includes('/') ? path : null; +} + +/** + * The path part of a remote, in either syntax git accepts, without leading or + * trailing slashes and without `.git`. + */ +function remotePath(remoteUrl: string): string | null { const trimmed = remoteUrl.trim().replace(/\.git\/?$/, ''); + if (!trimmed) return null; // scp-like syntax (`git@host:group/project`) is not a URL, so it is matched // rather than parsed. Everything else goes through URL. const scp = trimmed.match(/^[^/]+@([^:]+):(.+)$/); - let path = scp + const raw = scp ? scp[2]! : (() => { try { @@ -203,19 +423,39 @@ export function projectPathFromRemote(remoteUrl: string, instanceRoot?: string | } })(); - if (instanceRoot) { - const root = (() => { - try { - return new URL(instanceRoot).pathname.replace(/\/+$/, ''); - } catch { - return ''; - } - })(); - if (root && path.startsWith(root)) path = path.slice(root.length); - } + return raw.replace(/^\/+|\/+$/g, ''); +} - path = path.replace(/^\/+|\/+$/g, ''); - return path.includes('/') ? path : null; +/** + * The instance a clone already names. + * + * A local checkout knows its host — asking someone to retype it is asking for + * something the repository is holding. What cannot be read off a remote is + * *which software* the host runs, so the provider is still chosen by hand + * ([[0020-local-mode-borrows-the-host-cli]]); this only supplies the address. + * + * https is assumed, and any port is dropped. An ssh remote's port is the SSH + * port — carrying `:2222` onto an API URL would be confidently wrong, and a + * genuinely non-standard API port is what the override field is for. + */ +export function instanceUrlFromRemote(remoteUrl: string): string | null { + const trimmed = remoteUrl.trim(); + if (!trimmed) return null; + + const scp = trimmed.match(/^[^/]+@([^:]+):/); + if (scp) return `https://${scp[1]!.toLowerCase()}`; + + try { + const url = new URL(trimmed); + if (!url.hostname) return null; + // An https remote's port is the web port and worth keeping; ssh:// carries + // the SSH one, which is not. + const port = url.protocol === 'ssh:' ? '' : url.port ? `:${url.port}` : ''; + const scheme = url.protocol === 'http:' ? 'http' : 'https'; + return `${scheme}://${url.hostname.toLowerCase()}${port}`; + } catch { + return null; + } } function label(kind: HostKind): string { @@ -292,7 +532,7 @@ function run( bin: string, args: string[], opts: { cwd: string; stdin?: string; timeoutMs?: number }, -): Promise<{ code: number | null; stdout: string }> { +): Promise<{ code: number | null; stdout: string; stderr: string }> { return new Promise((resolve) => { const child = spawn(bin, args, { cwd: opts.cwd, @@ -308,12 +548,13 @@ function run( }); let stdout = ''; + let stderr = ''; let settled = false; const done = (code: number | null) => { if (settled) return; settled = true; clearTimeout(timer); - resolve({ code, stdout }); + resolve({ code, stdout, stderr }); }; const timer = setTimeout(() => { @@ -324,10 +565,13 @@ function run( child.stdout.on('data', (d: Buffer) => { stdout += d.toString(); }); - // stderr is drained but not kept: it is where both CLIs put their - // "a pull request already exists" message, which is not an error worth - // repeating to a user — `findReview` answers it with the URL instead. - child.stderr.on('data', () => undefined); + // Kept, not discarded. For the CLIs it holds "a pull request already + // exists", which `findReview` answers with a URL instead — but for `git + // push` it is where the server's own messages arrive, and GitLab announces + // the merge request it just opened in exactly that channel. + child.stderr.on('data', (d: Buffer) => { + stderr += d.toString(); + }); child.on('error', () => done(null)); child.on('close', (code) => done(code)); diff --git a/apps/api/src/vcs/vcs.types.ts b/apps/api/src/vcs/vcs.types.ts index 0d0e004..dc041b0 100644 --- a/apps/api/src/vcs/vcs.types.ts +++ b/apps/api/src/vcs/vcs.types.ts @@ -72,7 +72,12 @@ export interface RepoTarget { */ export interface LocalReviewCredential { provider: 'github' | 'gitlab'; - token: string; + /** + * Null is a real answer, not a missing one: GitLab opens merge requests from + * a push option, so naming the provider is sometimes all specd needs. GitHub + * has no equivalent and does require one. + */ + token: string | null; /** The instance root. gitlab.com / api.github.com when not self-managed. */ instanceUrl: string | null; } diff --git a/apps/web/app/setup/page.tsx b/apps/web/app/setup/page.tsx index f77af93..828c6dd 100644 --- a/apps/web/app/setup/page.tsx +++ b/apps/web/app/setup/page.tsx @@ -1019,36 +1019,43 @@ function SetupWizard() { - specd reads and writes your code on disk either way. A token here is used - for one thing — opening the review — and never to fetch a file or push a - commit; your own git credentials do the push. + specd reads and writes your code on disk either way, and your own git + credentials do the push. This only decides how the review gets opened. {reviewProvider && ( <> + {reviewProvider === 'gitlab' && ( +
+ You may not need a token at all. GitLab can open the merge + request as part of the push itself, over the same SSH connection you + already use — no API call, which is what makes it work behind an + access portal. specd tries that first and only falls back to a token + if your instance does not answer. Leave the fields below empty to rely + on it. +
+ )}
- + setReviewInstanceUrl(e.target.value)} - placeholder={ - reviewProvider === 'gitlab' - ? 'https://gitlab.example.com — blank for gitlab.com' - : 'https://github.example.com — blank for github.com' - } + placeholder="taken from your repository's origin" spellCheck={false} /> - The instance root, not a project page. specd reaches it from the machine - it runs on, so a host behind a VPN needs this machine on that VPN. + Normally blank: specd reads the host out of the clone's{' '} + origin. Set it only for an instance served from a subpath + (https://host/gitlab), plain http, or a non-standard API + port. It is the instance root, never a group or project page.
- + {reviewProvider === 'gitlab' - ? 'Needs the api scope, and permission to open merge requests on the project.' - : 'A token with pull-request write access on the repository.'} + ? 'Only used if the push-option route does not open the merge request. Needs the api scope — and an access portal in front of /api/v4 will defeat it, which the push route sidesteps.' + : 'A token with pull-request write access on the repository. GitHub has no push-option equivalent, so this is required.'}
{reviewCheck && ( diff --git a/apps/web/lib/docs/guides.ts b/apps/web/lib/docs/guides.ts index 132dbbb..c19b115 100644 --- a/apps/web/lib/docs/guides.ts +++ b/apps/web/lib/docs/guides.ts @@ -62,7 +62,17 @@ export const GUIDES: DocCategory = { }, { k: 'p', - text: 'For a **self-managed** GitLab or GitHub Enterprise, that is not enough: specd refuses to guess what software a host runs from its URL, and the host\'s CLI is often not on a corporate machine. So the local step takes an optional **review credential** — pick the host, give the instance URL and a token, and specd opens the merge request with it. The token is checked on the spot, and is used for that one thing.', + text: 'For a **self-managed** GitLab or GitHub Enterprise, that is not enough: specd refuses to guess what software a host runs from its URL, and the host\'s CLI is often not on a corporate machine. So the local step asks which host to open reviews on — and for GitLab, that is usually all it needs.', + }, + { + k: 'note', + tone: 'good', + title: 'GitLab needs no token here', + text: 'GitLab can open the merge request as part of the push (`git push -o merge_request.create`). That travels over the git transport — the connection you already clone and push through — so it needs no API call, no credential, and works where an access portal intercepts `/api/v4`. specd tries it first. A token stays available as a fallback, and GitHub still requires one because pull requests there are API-only.', + }, + { + k: 'p', + text: 'The instance URL is derived from your repository\'s `origin`, so it is normally left blank. Set it only for an instance served from a subpath (`https://host/gitlab`), plain http, or a non-standard API port — and give the instance **root**, never a group or project page. Where the two readings are genuinely ambiguous, specd asks the instance rather than guessing.', }, { k: 'note', diff --git a/docs/gitlab.md b/docs/gitlab.md index 34202a8..95c1f05 100644 --- a/docs/gitlab.md +++ b/docs/gitlab.md @@ -85,8 +85,15 @@ back as a sentence naming the cause, rather than as "Internal server error": | `answered with an HTML page rather than JSON` | An SSO or access portal sits in front of the instance and served its login page at **200**. See below. | | `→ 401` | The instance answered. The token is the problem, not the network. | -**An access portal in front of the API.** This is the one failure specd cannot -work around. A portal that intercepts `/api/v4` and answers with a sign-in page +**An access portal in front of the API.** For a *local-mode* repository there +is a way around it: GitLab can open the merge request from the push itself +(`git push -o merge_request.create`), which travels over the git transport and +is never seen by an HTTP portal. specd tries that first, so a local-mode +project on a portal-protected instance needs no token at all. What follows +applies to a repository connected in **GitLab mode**, which does read and write +over the API. + +This is the one failure specd cannot work around there. A portal that intercepts `/api/v4` and answers with a sign-in page means the request never reached GitLab, so no token specd holds can help: it speaks the API directly and cannot complete a browser sign-in. The symptom used to be `Unexpected token '<', "