From afabd044c3e431d3f5f4d54e67a67211a523dc17 Mon Sep 17 00:00:00 2001 From: unitypark Date: Tue, 18 Aug 2026 08:55:29 +0200 Subject: [PATCH] GitLab opens the merge request from the push, so local mode needs no token MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The review credential assumed a review has to be opened over an API. For GitLab it does not: `git push -o merge_request.create` creates it over the git transport, on the same connection the repository was cloned through. That is not a convenience. The failure behind all of this was an access portal answering /api/v4 with its own login page at 200 — a request that never reaches GitLab, which no token can fix. The git transport is not intercepted, so the push route works exactly where the API route cannot. Order is now push options, then a token, then the host CLI; for GitLab the token is optional, and GitHub still requires one because pull requests there are API-only. Three things it had to get right. A remote that refuses push options rejects the whole push and sends nothing — GitLab before 11.10, and every non-GitLab remote — so the branch is pushed again without them and the review falls to the next strategy. The description is one argv string against a page of markdown, so it is trimmed and says so rather than risking a push rejected over its own body. And push options create but do not rewrite, so a rebuild leaves an open MR's description describing the previous run, which is why the token path stays. The instance URL stopped being something to type. A clone knows its origin, so the host is read from it and the field is an override for the three things a remote cannot express: a subpath install, plain http, a non-standard API port. Two bugs closed on the way. A blank field used to fall through to the adapter default and send a self-managed project's path and token to gitlab.com. And projectPathFromRemote compared the instance subpath against URL.pathname, which has a leading slash, while its scp branch produced one without — so a subpath-hosted instance stripped its prefix from an https remote and silently kept it on an ssh one, which is the syntax corporate clones use. Where a URL is genuinely ambiguous — host/ET130/services/api is either a group on a root install or a project on a subpath one — specd asks instead of guessing: one GET {candidate}/api/v4/version, root first, one segment in on a 404. Connect-time verification uses the same resolution, because a token checked against the wrong host is worse than a token not checked at all. Also: the local push now uses --force-with-lease. A build resets its branch every run, so the plain push it used to do would have been rejected as non-fast-forward on the second attempt. --- apps/api/src/projects/projects.controller.ts | 57 +++- apps/api/src/vcs/local-git.adapter.ts | 16 + apps/api/src/vcs/local-review.service.ts | 24 +- apps/api/src/vcs/local-review.test.ts | 152 ++++++++- apps/api/src/vcs/local-review.ts | 298 ++++++++++++++++-- apps/api/src/vcs/vcs.types.ts | 7 +- apps/web/app/setup/page.tsx | 39 ++- apps/web/lib/docs/guides.ts | 12 +- docs/gitlab.md | 11 +- .../0020-local-mode-borrows-the-host-cli.md | 53 ++++ knowledge/glossary.md | 3 +- 11 files changed, 606 insertions(+), 66 deletions(-) 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 '<', "