From e957c43c162c1e86c352dd5ff3358fe381fb02e5 Mon Sep 17 00:00:00 2001 From: unitypark Date: Mon, 17 Aug 2026 18:24:57 +0200 Subject: [PATCH] A self-managed host connects, and says which half is wrong when it does not MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things, found by connecting a project on a corporate GitLab. Connecting one answered "Internal server error". `GitLabAdapter.api()` called fetch unguarded, and fetch rejects with a plain TypeError in exactly the two situations a self-managed instance produces: a bare host is not a URL (WHATWG reads it as a scheme), and an unreachable host fails at the transport with the reason on `cause.code`. A TypeError is not an HttpException, so it sailed past the controller's `instanceof VcsError` catch — the catch whose own docstring says it exists to stop "your token is wrong" reading as "Internal server error". gitlab.com always resolves, so the gap only ever opened on self-managed. `normalizeInstanceUrl` now reads a bare host as https and refuses what it cannot use, at the point the URL is stored so the complaint lands on the field somebody typed it into; `describeTransportFailure` turns cause.code into the sentence for that cause — VPN, DNS, refused, timeout, an untrusted internal CA pointed at NODE_EXTRA_CA_CERTS. The same guard goes on the GitHub adapter, which has the identical unguarded fetch and hides it behind api.github.com always being up. The instance URL keeps its path, deliberately. Stripping to the origin would be a kindness to someone pasting a project URL bought by breaking every GitLab served from a relative URL root, and only one of those is a deployment somebody chose. A pasted project URL instead 404s at the API base, and `describeApiBase404` says which half to drop. Local mode could not open a merge request on such a host at all: `detectHost` refuses to guess what software a self-managed host runs (0020, property 2), and `glab` is not on most corporate machines. But that reasoning forbids *guessing* the host, not being *told* it. So a local-mode project may now carry an optional review credential — provider, instance URL and token, on the `vcs` connection it already owns, which needed no migration because that row's encrypted_secret and settings.instanceUrl were both unused for provider 'local'. The token opens the review and nothing else: it never reads a file, lists a tree, clones or pushes, and git still pushes with the machine's own credentials. Absent, which is the default, local mode behaves exactly as before, down to the wording of the hint. It is proved at connect time with verify() on either adapter, the way JiraAdapter.verify() already worked, so a wrong token fails in the wizard rather than as a merge request that never appears. 0020 carries the amendment, including the honest version of the cost: local mode's promise narrows from "specd holds no credential for your host" to "unless you give it one, and then only to open reviews". --- README.md | 18 +- apps/api/src/app.module.ts | 2 + apps/api/src/projects/projects.controller.ts | 57 +++++- apps/api/src/vcs/github.adapter.ts | 53 ++++- apps/api/src/vcs/gitlab.adapter.test.ts | 186 ++++++++++++++++++ apps/api/src/vcs/gitlab.adapter.ts | 70 ++++++- apps/api/src/vcs/local-git.adapter.test.ts | 12 +- apps/api/src/vcs/local-git.adapter.ts | 7 +- apps/api/src/vcs/local-review.service.ts | 64 ++++++ apps/api/src/vcs/local-review.test.ts | 158 ++++++++++++++- apps/api/src/vcs/local-review.ts | 132 +++++++++++-- apps/api/src/vcs/vcs.service.ts | 1 + apps/api/src/vcs/vcs.types.ts | 123 ++++++++++++ apps/api/src/vcs/workspace.test.ts | 9 +- apps/api/src/vcs/workspace.ts | 9 +- apps/web/app/setup/page.tsx | 138 ++++++++++++- apps/web/lib/docs/guides.ts | 12 +- apps/web/lib/docs/integrations.ts | 16 ++ docs/gitlab.md | 18 +- .../0020-local-mode-borrows-the-host-cli.md | 35 ++++ knowledge/glossary.md | 3 +- 21 files changed, 1062 insertions(+), 61 deletions(-) create mode 100644 apps/api/src/vcs/local-review.service.ts diff --git a/README.md b/README.md index c82a731..9d7d95b 100644 --- a/README.md +++ b/README.md @@ -569,14 +569,16 @@ write path, same fail-closed webhook rule using the mechanism GitLab actually offers (token echo, constant-time compare). Walkthrough: [`docs/gitlab.md`](docs/gitlab.md). -**Local mode** — a repository registered by path (`specd connect .`). specd -holds no credential for any host here and never will. Where `origin` points at -GitHub or GitLab and that host's CLI (`gh`, `glab`) is installed and signed in -on the same machine, setup and build branches are pushed and opened as real -PRs through *your* account; the CLI is checked before anything is pushed, so a -repository specd cannot open a review on is never published to. -`SPECD_LOCAL_OPEN_PR=0` keeps everything local — the branch is committed -either way +**Local mode** — a repository registered by path (`specd connect .`), read and +written on disk. Where `origin` is github.com or gitlab.com and that host's CLI +(`gh`, `glab`) is signed in on the same machine, setup and build branches are +pushed and opened as real PRs through *your* account. For a **self-managed** +instance — where specd refuses to guess what software a host runs — the local +step takes an optional **review credential**: pick the host, give the instance +URL and a token, and it opens the merge request with that. The token opens +reviews and nothing else; your own git does the push, and leaving it unset +keeps local mode credential-free. `SPECD_LOCAL_OPEN_PR=0` disables the whole +path — the branch is committed either way ([`knowledge/decisions/0020-local-mode-borrows-the-host-cli.md`](knowledge/decisions/0020-local-mode-borrows-the-host-cli.md)). **Jira** — connect, import issues, backlink comments and status mirroring work diff --git a/apps/api/src/app.module.ts b/apps/api/src/app.module.ts index 512835a..15f0f79 100644 --- a/apps/api/src/app.module.ts +++ b/apps/api/src/app.module.ts @@ -14,6 +14,7 @@ import { RepositoriesService } from './projects/repositories.service.js'; import { ConnectionsService } from './projects/connections.service.js'; import { LocalGitAdapter } from './vcs/local-git.adapter.js'; +import { LocalReviewService } from './vcs/local-review.service.js'; import { VcsService } from './vcs/vcs.service.js'; import { GitHubAppService } from './vcs/github-app.service.js'; import { GitHubWebhookService } from './vcs/github-webhook.service.js'; @@ -78,6 +79,7 @@ import { RunnersController } from './runners/runners.controller.js'; RepositoriesService, ConnectionsService, LocalGitAdapter, + LocalReviewService, VcsService, GitHubAppService, GitHubWebhookService, diff --git a/apps/api/src/projects/projects.controller.ts b/apps/api/src/projects/projects.controller.ts index 6d46d06..43b98f1 100644 --- a/apps/api/src/projects/projects.controller.ts +++ b/apps/api/src/projects/projects.controller.ts @@ -31,6 +31,9 @@ import { KnowledgeService } from '../knowledge/knowledge.service.js'; 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 { GitHubAdapter } from '../vcs/github.adapter.js'; +import { GitLabAdapter } from '../vcs/gitlab.adapter.js'; class CreateProjectDto { @IsString() @MinLength(1) @MaxLength(80) name!: string; @@ -73,6 +76,11 @@ class ConnectVcsDto { @IsIn(['local', 'github', 'gitlab']) provider!: string; @IsOptional() @IsString() token?: string; @IsOptional() @IsString() instanceUrl?: string; + /** + * Local mode only: the host to open pull/merge requests on, if any. Absent + * keeps local mode exactly as it was — a branch, and no credential held. + */ + @IsOptional() @IsIn(['github', 'gitlab']) reviewProvider?: 'github' | 'gitlab'; } class ConnectTrackerDto { @@ -295,15 +303,58 @@ export class ProjectsController { ) { const project = await this.projects.bySlug(slug); await this.projects.requireRole(user.sub, project.id, ['owner', 'maintainer']); + + // Normalized before it is stored, so the complaint lands on the field + // somebody just typed into rather than on the repository list one call + // later — and so every later caller (webhooks, indexing, builds) reads a + // URL that is already in a shape `fetch` accepts. + let instanceUrl: string | null = null; + if (dto.instanceUrl?.trim()) { + try { + instanceUrl = normalizeInstanceUrl(dto.instanceUrl); + } catch (err) { + throw new BadRequestException(err instanceof VcsError ? err.message : String(err)); + } + } + + // Local mode may carry a credential used for one thing: opening the + // review. It is proved here, live, because the wizard must not claim a + // connection that fails later inside a run (§6 guardrail) — and because + // "the merge request never appeared" is a bad way to learn a token is + // wrong. + let connectedAs: string | undefined; + const reviewProvider = dto.provider === 'local' ? dto.reviewProvider ?? null : null; + if (reviewProvider) { + if (!dto.token) { + 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.`, + ); + } + try { + const identity = + reviewProvider === 'gitlab' + ? await new GitLabAdapter(dto.token, instanceUrl ?? undefined).verify() + : await new GitHubAdapter( + dto.token, + instanceUrl ? `${instanceUrl}/api/v3` : undefined, + ).verify(); + connectedAs = identity.username; + } catch (err) { + throw new BadRequestException( + err instanceof VcsError ? err.message : err instanceof Error ? err.message : String(err), + ); + } + } + await this.connections.upsert({ projectId: project.id, kind: 'vcs', provider: dto.provider, - label: dto.provider === 'local' ? 'local runner' : dto.instanceUrl ?? dto.provider, - settings: { instanceUrl: dto.instanceUrl ?? null }, + label: dto.provider === 'local' ? 'local runner' : instanceUrl ?? dto.provider, + settings: { instanceUrl, reviewProvider }, secret: dto.token ?? null, }); - return { ok: true }; + return connectedAs ? { ok: true, connectedAs } : { ok: true }; } @Post(':slug/connections/tracker') diff --git a/apps/api/src/vcs/github.adapter.ts b/apps/api/src/vcs/github.adapter.ts index 2a577d0..fd7fb70 100644 --- a/apps/api/src/vcs/github.adapter.ts +++ b/apps/api/src/vcs/github.adapter.ts @@ -2,6 +2,7 @@ import { collectSamples } from './scan-targets.js'; import { IGNORED_DIRS, VcsError, + describeTransportFailure, reviewHint, type ChangeResult, type OpenedReview, @@ -27,6 +28,15 @@ import { * (which mints installation tokens per run) is P1-scope wiring on top of this * class, not a change to it. */ +/** The origin of an API base, for an error message. Falls back to the raw value. */ +function hostOf(apiBase: string): string { + try { + return new URL(apiBase).origin; + } catch { + return apiBase; + } +} + export class GitHubAdapter implements VcsAdapter { readonly provider = 'github'; @@ -42,16 +52,27 @@ export class GitHubAdapter implements VcsAdapter { } private async api(path: string, init: RequestInit = {}): Promise { - const res = await fetch(`${this.apiBase}${path}`, { - ...init, - headers: { - Accept: 'application/vnd.github+json', - Authorization: `Bearer ${this.token}`, - 'X-GitHub-Api-Version': '2022-11-28', - 'Content-Type': 'application/json', - ...(init.headers ?? {}), - }, - }); + let res: Response; + try { + res = await fetch(`${this.apiBase}${path}`, { + ...init, + headers: { + Accept: 'application/vnd.github+json', + Authorization: `Bearer ${this.token}`, + 'X-GitHub-Api-Version': '2022-11-28', + 'Content-Type': 'application/json', + ...(init.headers ?? {}), + }, + }); + } catch (err) { + // As in the GitLab adapter: a request that never reached the host + // rejects with a TypeError, which is not an HttpException and so + // reaches the caller as an opaque 500. Rare against api.github.com, + // routine against an Enterprise Server behind a VPN. + const explained = describeTransportFailure(err, hostOf(this.apiBase)); + if (explained) throw new VcsError(explained, err); + throw err; + } if (!res.ok) { const body = await res.text(); @@ -286,6 +307,18 @@ export class GitHubAdapter implements VcsAdapter { } } + /** + * Prove a token, and say who it belongs to. + * + * Only meaningful for a user token — an App installation token has no user, + * and the App path proves itself by listing what it was granted instead. + * Used at connect time by local mode's review credential. + */ + async verify(): Promise<{ username: string; name: string }> { + const me = await this.api<{ login: string; name: string | null }>('/user'); + return { username: me.login, name: me.name ?? me.login }; + } + /** Repo picker source: exactly what the installation was granted (§6 step 2). */ async listInstallationRepositories(): Promise< { id: string; fullName: string; defaultBranch: string; language: string | null }[] diff --git a/apps/api/src/vcs/gitlab.adapter.test.ts b/apps/api/src/vcs/gitlab.adapter.test.ts index 9c99171..3137c9a 100644 --- a/apps/api/src/vcs/gitlab.adapter.test.ts +++ b/apps/api/src/vcs/gitlab.adapter.test.ts @@ -1,5 +1,6 @@ import { afterEach, describe, expect, it, vi } from 'vitest'; import { GitLabAdapter } from './gitlab.adapter.js'; +import { VcsError } from './vcs.types.js'; /** * Against a stubbed transport, not a live instance — the same arrangement as @@ -133,4 +134,189 @@ describe('construction', () => { it('refuses to exist without a token, naming the fix', () => { expect(() => new GitLabAdapter('')).toThrow(/reconnect it in project settings/i); }); + + it('accepts the host people actually type for a self-managed instance', () => { + // `gitlab.example.com` is not a URL — WHATWG reads the host as a scheme — + // and `fetch` answers an unparseable URL with a TypeError, which used to + // reach the wizard as "Internal server error". + expect(new GitLabAdapter('tok', 'gitlab.example.com').instanceUrl).toBe( + 'https://gitlab.example.com', + ); + expect(new GitLabAdapter('tok', 'https://gitlab.example.com/').instanceUrl).toBe( + 'https://gitlab.example.com', + ); + // http stays http — a self-managed instance may genuinely be served on it. + expect(new GitLabAdapter('tok', 'http://gitlab.internal:8080').instanceUrl).toBe( + 'http://gitlab.internal:8080', + ); + }); + + it('keeps a subpath, because GitLab can be served from one', () => { + // `external_url 'https://host/gitlab'` is a supported deployment, and its + // API really is at {origin}/gitlab/api/v4. Reducing the URL to its origin + // to be helpful to someone pasting a project URL would break every one of + // these — a deployment somebody chose, traded for a typo somebody made. + expect(new GitLabAdapter('tok', 'https://intranet.example.com/gitlab/').instanceUrl).toBe( + 'https://intranet.example.com/gitlab', + ); + }); + + it('names what is wrong with a URL it cannot use', () => { + expect(() => new GitLabAdapter('tok', 'ftp://gitlab.example.com')).toThrow( + /speaks only http and https/i, + ); + expect(() => new GitLabAdapter('tok', 'not a url at all')).toThrow(/is not a URL specd can reach/i); + }); + +}); + +describe('a nested group on a self-managed instance', () => { + // GitLab mode against the shape a real corporate instance has: a host that + // is not gitlab.com, and a project two groups deep. Both are places a URL + // can be built wrong without any test noticing. + const INSTANCE = 'https://gitlab.example.com'; + const PROJECT = 'acme/services/aurora-api'; + const ENCODED = 'acme%2Fservices%2Faurora-api'; + + it('addresses the instance and the full namespace path, encoded as GitLab wants it', async () => { + const seen: string[] = []; + vi.stubGlobal( + 'fetch', + vi.fn(async (url: string) => { + seen.push(url); + return { + ok: true, + status: 201, + json: async () => ({ web_url: `${INSTANCE}/${PROJECT}/-/merge_requests/3`, iid: 3 }), + text: async () => '', + }; + }), + ); + + const opened = await new GitLabAdapter('glpat-x', INSTANCE).openMergeRequest(PROJECT, { + branch: 'spec/E-101-add-csv-export', + base: 'main', + title: '[E-101] - Add CSV export', + body: 'body', + }); + + // A nested group is one path with slashes in it, not a namespace plus a + // project — so it is percent-encoded whole. Splitting it would address + // `acme/services`, which is a group and not a project. + expect(seen[0]).toBe(`${INSTANCE}/api/v4/projects/${ENCODED}/merge_requests`); + expect(opened.url).toBe(`${INSTANCE}/${PROJECT}/-/merge_requests/3`); + }); + + it('proves a token against the instance it was given, not gitlab.com', async () => { + const seen: string[] = []; + vi.stubGlobal( + 'fetch', + vi.fn(async (url: string) => { + seen.push(url); + return { ok: true, status: 200, json: async () => ({ username: 'jpark', name: 'J Park' }), text: async () => '' }; + }), + ); + + await expect(new GitLabAdapter('glpat-x', INSTANCE).verify()).resolves.toEqual({ + username: 'jpark', + name: 'J Park', + }); + expect(seen[0]).toBe(`${INSTANCE}/api/v4/user`); + }); +}); + +describe('an instance URL that carries a project path', () => { + // The shape people paste out of the address bar: a nested-group project on + // a self-managed instance. The host is right, the path is not GitLab's root, + // and `{that}/api/v4` is a 404 nobody can work backwards from unaided. + const PASTED = 'https://gitlab.example.com/acme/services/aurora-api'; + + it('is kept verbatim, and the 404 explains which half is wrong', async () => { + vi.stubGlobal( + 'fetch', + vi.fn(async () => ({ + ok: false, + status: 404, + statusText: 'Not Found', + text: async () => '404 Not Found', + json: async () => ({}), + })), + ); + + const adapter = new GitLabAdapter('tok', PASTED); + expect(adapter.instanceUrl).toBe(PASTED); + + await expect(adapter.listRepositories()).rejects.toThrow( + /includes the path "\/acme\/services\/aurora-api".*https:\/\/gitlab\.example\.com/is, + ); + }); + + it('stops second-guessing the URL once the instance has answered once', async () => { + // A 404 after a successful call is a missing project, not a wrong host, + // and telling someone to fix their instance URL then would be wrong. + const responses = [ + { ok: true, status: 200, json: async () => [], text: async () => '[]' }, + { ok: false, status: 404, statusText: 'Not Found', text: async () => '404', json: async () => ({}) }, + ]; + vi.stubGlobal('fetch', vi.fn(async () => responses.shift())); + + const adapter = new GitLabAdapter('tok', 'https://gitlab.example.com'); + await adapter.listRepositories(); + + await expect(adapter.listRepositories()).rejects.toThrow(/→ 404/); + }); +}); + +describe('a request that never reaches the instance', () => { + /** + * The reported bug. Everything about a self-managed GitLab that can go wrong + * — VPN off, internal DNS, an untrusted internal CA — fails this way, and + * `fetch` reports all of it as `TypeError: fetch failed` with the reason on + * `cause.code`. A TypeError is not an HttpException, so Nest rendered every + * one of them as "Internal server error". + */ + const transportError = (code: string) => { + const err = new TypeError('fetch failed'); + (err as { cause?: unknown }).cause = { code }; + return err; + }; + + it('explains an unresolvable host instead of failing opaquely', async () => { + vi.stubGlobal( + 'fetch', + vi.fn(async () => { + throw transportError('ENOTFOUND'); + }), + ); + + await expect( + new GitLabAdapter('tok', 'gitlab.internal').listRepositories(), + ).rejects.toThrow(/does not resolve.*VPN/is); + }); + + it('points an untrusted certificate at the CA store rather than at the token', async () => { + vi.stubGlobal( + 'fetch', + vi.fn(async () => { + throw transportError('DEPTH_ZERO_SELF_SIGNED_CERT'); + }), + ); + + await expect(new GitLabAdapter('tok', 'gitlab.internal').listRepositories()).rejects.toThrow( + /self-signed certificate.*NODE_EXTRA_CA_CERTS/is, + ); + }); + + it('is a VcsError, which is what the controller turns into a 400', async () => { + vi.stubGlobal( + 'fetch', + vi.fn(async () => { + throw transportError('ECONNREFUSED'); + }), + ); + + await expect( + new GitLabAdapter('tok', 'gitlab.internal').listRepositories(), + ).rejects.toBeInstanceOf(VcsError); + }); }); diff --git a/apps/api/src/vcs/gitlab.adapter.ts b/apps/api/src/vcs/gitlab.adapter.ts index a794435..3885919 100644 --- a/apps/api/src/vcs/gitlab.adapter.ts +++ b/apps/api/src/vcs/gitlab.adapter.ts @@ -2,6 +2,9 @@ import { collectSamples } from './scan-targets.js'; import { IGNORED_DIRS, VcsError, + describeApiBase404, + describeTransportFailure, + normalizeInstanceUrl, reviewHint, type ChangeResult, type OpenedReview, @@ -37,33 +40,68 @@ import { export class GitLabAdapter implements VcsAdapter { readonly provider = 'gitlab'; private readonly apiBase: string; + /** The instance root, for error messages — `apiBase` has `/api/v4` glued on. */ + private readonly origin: string; + /** Has any call to this instance succeeded? Decides what a 404 means. */ + private reachedApi = false; constructor( private readonly token: string, - readonly instanceUrl = 'https://gitlab.com', + instanceUrl = 'https://gitlab.com', ) { if (!token) { throw new VcsError('GitLab is connected but no token is available. Reconnect it in project settings.'); } - this.apiBase = `${instanceUrl.replace(/\/+$/, '')}/api/v4`; + // Normalized here as well as at the point it is stored, because a + // connection saved before that existed still has whatever was typed, and + // this class is the last place that can turn it into something `fetch` + // will accept rather than a 500. + this.origin = normalizeInstanceUrl(instanceUrl); + this.apiBase = `${this.origin}/api/v4`; + } + + /** The instance this adapter talks to, normalized. */ + get instanceUrl(): string { + return this.origin; } private async api(path: string, init: RequestInit = {}): Promise { - const res = await fetch(`${this.apiBase}${path}`, { - ...init, - headers: { - 'PRIVATE-TOKEN': this.token, - 'Content-Type': 'application/json', - ...(init.headers ?? {}), - }, - }); + let res: Response; + try { + res = await fetch(`${this.apiBase}${path}`, { + ...init, + headers: { + 'PRIVATE-TOKEN': this.token, + 'Content-Type': 'application/json', + ...(init.headers ?? {}), + }, + }); + } catch (err) { + // `fetch` rejects with a TypeError when the request never reached the + // host at all. Left alone it escapes as an opaque 500, which is the + // least useful thing to tell someone whose instance is behind a VPN. + const explained = describeTransportFailure(err, this.origin); + if (explained) throw new VcsError(explained, err); + throw err; + } if (!res.ok) { const body = await res.text(); + // A 404 straight off the API base is a wrong instance URL far more often + // than a missing resource, and "404" alone sends people to check their + // token — the one thing that is not the problem. + if (res.status === 404 && !this.reachedApi) { + throw new VcsError(describeApiBase404(this.origin)); + } throw new VcsError( `GitLab ${init.method ?? 'GET'} ${path} → ${res.status}: ${body.slice(0, 300)}`, ); } + + // From here on a 404 means what it says: this instance is a GitLab, so a + // missing project is a missing project. + this.reachedApi = true; + // 204 (branch delete) has no body. return res.status === 204 ? (undefined as T) : ((await res.json()) as T); } @@ -280,6 +318,18 @@ export class GitLabAdapter implements VcsAdapter { } /** Repo picker source (§6 step 2, §11): what the token can see, searchable. */ + /** + * Prove the token, and say who it belongs to. + * + * Called at connect time so a bad token fails in the wizard, where a person + * is looking at it, rather than later inside a run — the same role + * `JiraAdapter.verify()` plays, and the same reason. + */ + async verify(): Promise<{ username: string; name: string }> { + const me = await this.api<{ username: string; name: string }>('/user'); + return { username: me.username, name: me.name }; + } + async listRepositories( search?: string, ): Promise<{ id: string; fullName: string; defaultBranch: string; namespace: string }[]> { diff --git a/apps/api/src/vcs/local-git.adapter.test.ts b/apps/api/src/vcs/local-git.adapter.test.ts index 8b0919a..6e76313 100644 --- a/apps/api/src/vcs/local-git.adapter.test.ts +++ b/apps/api/src/vcs/local-git.adapter.test.ts @@ -6,6 +6,10 @@ import { afterAll, beforeAll, describe, expect, it } from 'vitest'; import { hostedCompareUrl, LocalGitAdapter } from './local-git.adapter.js'; import type { Config } from '../config.js'; import type { RepoTarget } from './vcs.types.js'; +import type { LocalReviewService } from './local-review.service.js'; + +/** No project carries a review credential in these fixtures. */ +const noReviews = { credentialFor: async () => null } as unknown as LocalReviewService; /** * The real adapter against a real git repository. @@ -38,7 +42,7 @@ describe('LocalGitAdapter against a real repository', () => { git('add', '-A'); git('commit', '-qm', 'first'); - adapter = new LocalGitAdapter({ localRepoRoot: null } as Config); + adapter = new LocalGitAdapter({ localRepoRoot: null } as Config, noReviews); target = { name: 'test/repo', localPath: dir, defaultBranch: 'main' } as RepoTarget; }); @@ -123,7 +127,7 @@ describe('propose review hint against a real repository', () => { // PR opening off: this suite is about the hint a repository gets when there // is no review surface, and leaving it on would have the test shell out to // whichever `gh` happens to be signed in on the machine running it. - const adapter = new LocalGitAdapter({ localRepoRoot: null, localOpenPr: false } as Config); + const adapter = new LocalGitAdapter({ localRepoRoot: null, localOpenPr: false } as Config, noReviews); const propose = (branch: string) => adapter.propose({ name: 'hint', localPath: dir } as RepoTarget, { @@ -159,7 +163,7 @@ describe('propose review hint against a real repository', () => { it('says why no pull request was opened, rather than letting the silence speak', async () => { // An unrecognized host short-circuits before any CLI or push, so this // exercises the enabled path without leaving the machine. - const enabled = new LocalGitAdapter({ localRepoRoot: null, localOpenPr: true } as Config); + const enabled = new LocalGitAdapter({ localRepoRoot: null, localOpenPr: true } as Config, noReviews); git('remote', 'set-url', 'origin', 'git@git.internal:team/repo.git'); const change = await enabled.propose({ name: 'hint', localPath: dir } as RepoTarget, { @@ -171,7 +175,7 @@ describe('propose review hint against a real repository', () => { expect(change.url).toBeNull(); expect(change.reviewHint).toContain('No pull request was opened'); - expect(change.reviewHint).toContain('not a host specd can open a review on'); + expect(change.reviewHint).toContain('will not guess what software a self-managed host runs'); // Nothing to link to either — specd refuses to guess a self-managed host. expect(change.reviewHint).not.toContain('http'); }); diff --git a/apps/api/src/vcs/local-git.adapter.ts b/apps/api/src/vcs/local-git.adapter.ts index fbef7d8..518bb75 100644 --- a/apps/api/src/vcs/local-git.adapter.ts +++ b/apps/api/src/vcs/local-git.adapter.ts @@ -6,6 +6,7 @@ import { simpleGit, type SimpleGit } from 'simple-git'; import { Config } from '../config.js'; import { parseCommitLog, type HistoryCommit } from '../knowledge/history.js'; import { detectHost, openLocalReview } from './local-review.js'; +import { LocalReviewService } from './local-review.service.js'; import { collectSamples } from './scan-targets.js'; import { IGNORED_DIRS, @@ -32,7 +33,10 @@ import { export class LocalGitAdapter implements VcsAdapter { readonly provider = 'local'; - constructor(private readonly config: Config) {} + constructor( + private readonly config: Config, + private readonly reviews: LocalReviewService, + ) {} private git(repo: RepoTarget): { git: SimpleGit; root: string } { const root = this.repoRoot(repo); @@ -205,6 +209,7 @@ export class LocalGitAdapter implements VcsAdapter { base: startingBranch, title: change.title, body: change.body, + credential: await this.reviews.credentialFor(repo.projectId), }).catch((err: unknown) => ({ url: null, note: `opening a review failed (${err instanceof Error ? err.message : String(err)})`, diff --git a/apps/api/src/vcs/local-review.service.ts b/apps/api/src/vcs/local-review.service.ts new file mode 100644 index 0000000..69d8776 --- /dev/null +++ b/apps/api/src/vcs/local-review.service.ts @@ -0,0 +1,64 @@ +import { Injectable } from '@nestjs/common'; +import { ConnectionsService } from '../projects/connections.service.js'; +import { Vault } from '../common/vault.js'; +import { normalizeInstanceUrl, type LocalReviewCredential } from './vcs.types.js'; + +/** + * The optional credential a local-mode project may hold for opening reviews. + * + * Local mode's promise is that specd does not hold a key to your host, and + * that stands: without this, nothing changes. What it adds is a way to say + * "open the merge request for me" for the case the `gh`/`glab` path cannot + * serve — a self-managed instance, where specd deliberately refuses to guess + * which software is running ([[0020-local-mode-borrows-the-host-cli]]) and the + * host's CLI is often not installed on a corporate machine anyway. + * + * It lives on the project's existing `vcs` connection rather than a new row: + * that connection is already `provider: 'local'` with an unused + * `encrypted_secret` and an unused `settings.instanceUrl`, so this needed no + * migration. `settings.reviewProvider` is the switch — absent means local mode + * behaves exactly as it did. + */ +@Injectable() +export class LocalReviewService { + constructor( + private readonly connections: ConnectionsService, + private readonly vault: Vault, + ) {} + + /** + * The review credential for a project, or null when there is none. + * + * Null is the ordinary answer and never an error: every local-mode project + * that predates this has one, and the caller falls back to the CLI path and + * then to a branch you diff. + */ + async credentialFor(projectId: string | undefined): Promise { + if (!projectId) return null; + + const conn = await this.connections.get(projectId, 'vcs').catch(() => null); + if (!conn || conn.provider !== 'local' || !conn.encryptedSecret) return null; + + const settings = (conn.settings ?? {}) as { reviewProvider?: string; instanceUrl?: string | null }; + const provider = settings.reviewProvider; + if (provider !== 'github' && provider !== 'gitlab') return null; + + // 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; + + return { + provider, + token, + 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 08647e3..aa13ba1 100644 --- a/apps/api/src/vcs/local-review.test.ts +++ b/apps/api/src/vcs/local-review.test.ts @@ -3,8 +3,8 @@ import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { simpleGit } from 'simple-git'; -import { afterAll, beforeAll, describe, expect, it } from 'vitest'; -import { detectHost, openLocalReview } from './local-review.js'; +import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from 'vitest'; +import { detectHost, openLocalReview, projectPathFromRemote } from './local-review.js'; describe('detectHost', () => { it('recognizes github and gitlab over ssh and https alike', () => { @@ -61,8 +61,160 @@ describe('openLocalReview', () => { }); expect(review.url).toBeNull(); - expect(review.note).toContain('not a host specd can open a review on'); + expect(review.note).toContain('will not guess what software a self-managed host runs'); // Nothing was written to the repository's config or refs on the way out. expect(git('remote').trim()).toBe(''); }); }); + +describe('projectPathFromRemote', () => { + it('reads the path out of every spelling git uses', () => { + const want = 'acme/services/aurora-api'; + expect(projectPathFromRemote('git@gitlab.example.com:acme/services/aurora-api.git')).toBe(want); + expect(projectPathFromRemote('https://gitlab.example.com/acme/services/aurora-api.git')).toBe(want); + expect(projectPathFromRemote('https://gitlab.example.com/acme/services/aurora-api')).toBe(want); + expect(projectPathFromRemote('ssh://git@gitlab.example.com:2222/acme/services/aurora-api.git')).toBe(want); + }); + + it('removes the instance subpath, which is not part of the project path', () => { + // GitLab at https://host/gitlab serves the project `group/project` — the + // subpath belongs to the instance, and passing it through would address a + // project that does not exist. + expect( + projectPathFromRemote('https://intranet.example.com/gitlab/group/project.git', 'https://intranet.example.com/gitlab'), + ).toBe('group/project'); + }); + + it('answers null when there is no namespace to be had', () => { + // Every GitLab and GitHub project path has at least one slash; something + // without one is not one, and inventing a namespace would be worse. + expect(projectPathFromRemote('git@host:project.git')).toBeNull(); + expect(projectPathFromRemote('')).toBeNull(); + }); +}); + +describe('a configured review credential', () => { + let dir = ''; + const git = (...args: string[]) => + execFileSync('git', args, { cwd: dir, encoding: 'utf8' as const }); + + beforeAll(() => { + dir = mkdtempSync(join(tmpdir(), 'specd-cred-')); + git('init', '-q', '-b', 'main'); + git('config', 'user.email', 't@t.dev'); + git('config', 'user.name', 'T'); + writeFileSync(join(dir, 'README.md'), '# x\n'); + git('add', '-A'); + git('commit', '-qm', 'init'); + }); + + afterAll(() => rmSync(dir, { recursive: true, force: true })); + afterEach(() => vi.unstubAllGlobals()); + + const review = (over: Partial[0]> = {}) => + openLocalReview({ + git: simpleGit({ baseDir: dir }), + cwd: dir, + remoteUrl: 'git@gitlab.example.com:acme/services/aurora-api.git', + branch: 'spec/E-101-add-csv-export', + base: 'main', + title: '[E-101] - Add CSV export', + body: 'body', + credential: { provider: 'gitlab', token: 'glpat-x', instanceUrl: 'https://gitlab.example.com' }, + ...over, + }); + + it('reaches a self-managed host the CLI path refuses to guess at', async () => { + // The whole point: `detectHost` answers null for this remote, so without a + // credential there is no review. With one, the host is not being guessed — + // it was named. + expect(detectHost('git@gitlab.example.com:acme/services/aurora-api.git')).toBeNull(); + + let opened: { url: string } | null = null; + vi.stubGlobal( + 'fetch', + vi.fn(async (url: string) => { + opened = { url }; + return { + ok: true, + status: 201, + json: async () => ({ web_url: 'https://gitlab.example.com/acme/services/aurora-api/-/merge_requests/7', iid: 7 }), + text: async () => '', + }; + }), + ); + + // The push has nowhere to go in a bare fixture, so the failure is the + // push, not the credential — which is itself the ordering guarantee worth + // pinning: nothing is opened for a branch that never reached the remote. + const result = await review(); + expect(result.url).toBeNull(); + expect(result.note).toMatch(/pushing to origin failed/); + expect(opened).toBeNull(); + }); + + it('pushes the branch, then opens the merge request against the named instance', async () => { + // A real remote, so the push is real; only the GitLab API is stubbed. + const remote = mkdtempSync(join(tmpdir(), 'specd-remote-')); + execFileSync('git', ['init', '-q', '--bare', remote]); + git('remote', 'add', 'origin', remote); + git('checkout', '-q', '-b', 'spec/E-101-add-csv-export'); + writeFileSync(join(dir, 'feature.txt'), 'work\n'); + git('add', '-A'); + git('commit', '-qm', 'work'); + git('checkout', '-q', 'main'); + + const seen: { url: string; body: unknown }[] = []; + vi.stubGlobal( + 'fetch', + vi.fn(async (url: string, init: RequestInit = {}) => { + seen.push({ url, body: init.body ? JSON.parse(String(init.body)) : null }); + return { + ok: true, + status: 201, + json: async () => ({ + web_url: 'https://gitlab.example.com/acme/services/aurora-api/-/merge_requests/7', + iid: 7, + }), + text: async () => '', + }; + }), + ); + + try { + // `origin` is the bare repo so the push is genuinely exercised, while + // `remoteUrl` stays the GitLab URL that names the project. In production + // both come from the same `git remote get-url origin`; splitting them + // here is what lets the push be real without a GitLab to push to. + const result = await review(); + + // The branch really landed on the remote — a review for a branch that + // never got there would be a broken link. + expect( + execFileSync('git', ['branch', '--list', 'spec/E-101-add-csv-export'], { + cwd: remote, + encoding: 'utf8', + }), + ).toContain('spec/E-101-add-csv-export'); + + expect(result.url).toBe( + 'https://gitlab.example.com/acme/services/aurora-api/-/merge_requests/7', + ); + expect(result.note).toMatch(/opened a merge request with this project's token/); + + // Addressed to the instance that was named, for the project the remote + // named, with the title the build station chose. + expect(seen[0]?.url).toBe( + 'https://gitlab.example.com/api/v4/projects/acme%2Fservices%2Faurora-api/merge_requests', + ); + expect(seen[0]?.body).toMatchObject({ + source_branch: 'spec/E-101-add-csv-export', + target_branch: 'main', + title: '[E-101] - Add CSV export', + }); + } finally { + rmSync(remote, { recursive: true, force: true }); + git('remote', 'remove', 'origin'); + } + }); +}); diff --git a/apps/api/src/vcs/local-review.ts b/apps/api/src/vcs/local-review.ts index b113c4c..a97559c 100644 --- a/apps/api/src/vcs/local-review.ts +++ b/apps/api/src/vcs/local-review.ts @@ -1,5 +1,8 @@ import { spawn } from 'node:child_process'; import type { SimpleGit } from 'simple-git'; +import { GitHubAdapter } from './github.adapter.js'; +import { GitLabAdapter } from './gitlab.adapter.js'; +import type { LocalReviewCredential } from './vcs.types.js'; /** * Turning a local-mode branch into a real pull or merge request. @@ -73,43 +76,148 @@ export async function openLocalReview(input: { base: string; title: string; body: string; + /** Configured for the project, if any. Takes precedence over the host CLI. */ + credential?: LocalReviewCredential | null; }): Promise { - const { git, cwd, branch, base, title, body } = input; + const { git, cwd, branch, base, title, body, credential } = input; - const host = detectHost(input.remoteUrl); - if (!host) { - return { url: null, note: 'its `origin` is not a host specd can open a review on' }; + // A configured credential settles what the host is, which is the thing + // `detectHost` refuses to guess. So a self-managed instance is reachable + // through this path and only this path. + const kind: HostKind | null = credential?.provider ?? detectHost(input.remoteUrl)?.kind ?? null; + if (!kind) { + return { + url: null, + note: + 'its `origin` is not github.com or gitlab.com, and no review credential is configured — ' + + 'specd will not guess what software a self-managed host runs', + }; } - const bin = host.kind === 'github' ? 'gh' : 'glab'; - if (!(await hostCliReady(host.kind, cwd))) { + const bin = kind === 'github' ? 'gh' : 'glab'; + const viaCli = !credential; + if (viaCli && !(await hostCliReady(kind, cwd))) { return { url: null, - note: `\`${bin}\` is not on PATH here, or is not signed in — specd holds no token of its own in local mode, so it had nothing else to open one with`, + note: `\`${bin}\` is not on PATH here, or is not signed in — and this project has no review credential, so specd had nothing to open one with`, }; } + // The push is git's, with the machine's own credentials, in every case. The + // 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); } catch (err) { return { url: null, note: `pushing to origin failed (${short(err)})` }; } - const created = await createReview(host.kind, { cwd, branch, base, title, body }); - if (created) return { url: created, note: `pushed and opened ${label(host.kind)}` }; + if (credential) { + return openWithToken(credential, { remoteUrl: input.remoteUrl, branch, base, title, body }); + } + + const created = await createReview(kind, { cwd, branch, base, title, body }); + if (created) return { url: created, note: `pushed and opened ${label(kind)}` }; // A review for this branch may already be open — a second setup run is a // normal thing to do, and both CLIs refuse to create a duplicate. Finding // the existing one is the correct outcome, not a fallback. - const existing = await findReview(host.kind, { cwd, branch }); - if (existing) return { url: existing, note: `pushed; ${label(host.kind)} was already open` }; + const existing = await findReview(kind, { cwd, branch }); + if (existing) return { url: existing, note: `pushed; ${label(kind)} was already open` }; return { url: null, - note: `pushed the branch, but \`${bin}\` could not open ${label(host.kind)}`, + note: `pushed the branch, but \`${bin}\` could not open ${label(kind)}`, }; } +/** + * Open the review over the provider's API with the project's own token. + * + * Both adapters already know how to open one *or* update the one that is + * already open for the branch, which is what a re-run needs — so this is + * genuinely just picking which of them to call. + */ +async function openWithToken( + credential: LocalReviewCredential, + pr: { remoteUrl: string; branch: string; base: string; title: string; body: string }, +): Promise { + const path = projectPathFromRemote(pr.remoteUrl, credential.instanceUrl); + if (!path) { + return { + url: null, + note: `pushed the branch, but could not read a project path out of \`origin\` (${pr.remoteUrl})`, + }; + } + + try { + const opened = + credential.provider === 'gitlab' + ? await new GitLabAdapter(credential.token, credential.instanceUrl ?? undefined).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, + ).openPullRequest(path, { + branch: pr.branch, + base: pr.base, + title: pr.title, + body: pr.body, + }); + + const what = label(credential.provider); + return { + url: opened.url, + note: opened.existing + ? `pushed; ${what} was already open and was brought up to date` + : `pushed and opened ${what} with this project's token`, + }; + } catch (err) { + return { url: null, note: `pushed the branch, but opening a review failed (${short(err)})` }; + } +} + +/** + * The `namespace/project` path a remote URL points at. + * + * Handles the three spellings git uses and, when the instance is served from + * a subpath, removes it — `https://host/gitlab/group/project.git` is the + * project `group/project` on the GitLab at `https://host/gitlab`, and passing + * the subpath through would address a project that does not exist. + */ +export function projectPathFromRemote(remoteUrl: string, instanceRoot?: string | null): string | null { + const trimmed = remoteUrl.trim().replace(/\.git\/?$/, ''); + + // 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 + ? scp[2]! + : (() => { + try { + return new URL(trimmed).pathname; + } catch { + return ''; + } + })(); + + if (instanceRoot) { + const root = (() => { + try { + return new URL(instanceRoot).pathname.replace(/\/+$/, ''); + } catch { + return ''; + } + })(); + if (root && path.startsWith(root)) path = path.slice(root.length); + } + + path = path.replace(/^\/+|\/+$/g, ''); + return path.includes('/') ? path : null; +} + function label(kind: HostKind): string { return kind === 'github' ? 'a pull request' : 'a merge request'; } diff --git a/apps/api/src/vcs/vcs.service.ts b/apps/api/src/vcs/vcs.service.ts index 5a556b8..a665e4a 100644 --- a/apps/api/src/vcs/vcs.service.ts +++ b/apps/api/src/vcs/vcs.service.ts @@ -106,6 +106,7 @@ export class VcsService { localPath: repo.localPath, externalId: repo.externalId, defaultBranch: repo.defaultBranch, + projectId: repo.projectId, }; } diff --git a/apps/api/src/vcs/vcs.types.ts b/apps/api/src/vcs/vcs.types.ts index 2c532d8..8fc7ab5 100644 --- a/apps/api/src/vcs/vcs.types.ts +++ b/apps/api/src/vcs/vcs.types.ts @@ -45,6 +45,28 @@ export interface RepoTarget { localPath: string | null; externalId: string | null; defaultBranch: string; + /** + * The project this repository belongs to. Carried because local mode's + * review credential lives on the project's connection, and `propose()` is + * handed a target rather than a `Repository` row it could read it from. + */ + projectId?: string; +} + +/** + * A credential local mode may use to open a review, and nothing else. + * + * Deliberately not the same thing as a VCS *connection*: a local-mode project + * reads and writes its repository on disk, so this token never fetches a file, + * never scans a tree and never pushes — the machine's own git does that. It + * opens the pull or merge request and stops. That narrowness is what makes it + * safe to add to the mode whose promise is that specd holds nothing. + */ +export interface LocalReviewCredential { + provider: 'github' | 'gitlab'; + token: string; + /** The instance root. gitlab.com / api.github.com when not self-managed. */ + instanceUrl: string | null; } export interface VcsAdapter { @@ -119,6 +141,107 @@ export function reviewHint( return `${verb} ${ref} on ${repoName}.${caveat} Merging is adopting.`; } +/** + * A self-managed instance URL, in a shape `fetch` will accept. + * + * Everything here exists because `fetch` rejects with a bare `TypeError` on a + * URL it cannot parse, and a `TypeError` is not an `HttpException` — so it + * reaches the client as "Internal server error", which tells someone who typed + * their host without a scheme precisely nothing. + * + * `gitlab.example.com` is what people type, and it is not a URL: WHATWG reads + * the host as a *scheme*. Rather than refuse it, assume https — that is what + * was meant every time, and a self-managed GitLab served over plain http is + * still reachable by typing `http://` explicitly. + */ +export function normalizeInstanceUrl(raw: string): string { + const trimmed = raw.trim(); + if (!trimmed) throw new VcsError('No instance URL was given.'); + + const candidate = /^[a-z][a-z0-9+.-]*:\/\//i.test(trimmed) ? trimmed : `https://${trimmed}`; + + let url: URL; + try { + url = new URL(candidate); + } catch { + throw new VcsError( + `"${raw}" is not a URL specd can reach. Give the instance's origin, e.g. ` + + 'https://gitlab.example.com — or leave it blank for gitlab.com.', + ); + } + + if (url.protocol !== 'https:' && url.protocol !== 'http:') { + throw new VcsError( + `"${raw}" uses ${url.protocol.replace(':', '')}, and specd speaks only http and https. ` + + 'Give the instance\'s origin, e.g. https://gitlab.example.com.', + ); + } + + // The path is KEPT, and that is not an oversight. GitLab supports being + // served from a relative URL root — `external_url 'https://host/gitlab'` — + // where the API really is at `{origin}/gitlab/api/v4`. Reducing this to the + // origin would be a convenience for someone pasting a project URL bought by + // breaking every subpath-hosted instance, and only one of those two is a + // deployment somebody chose. A pasted project URL instead 404s at the API + // base, which `describeApiBase404` explains. + const path = url.pathname.replace(/\/+$/, ''); + return `${url.origin}${path}`; +} + +/** + * The 404 a wrong instance URL produces, explained. + * + * Reaching a real host that answers 404 to `/api/v4/...` means one of two + * things, and they look identical from here: the URL carries a path that is + * not GitLab's root (a project URL pasted out of the address bar), or the + * instance is not a GitLab. Both are worth saying, because a bare "404" sends + * someone to check their token, which is the one thing that is fine. + */ +export function describeApiBase404(instanceUrl: string): string { + const path = (() => { + try { + return new URL(instanceUrl).pathname.replace(/\/+$/, ''); + } catch { + return ''; + } + })(); + + return ( + `${instanceUrl} answered, but has no GitLab API at ${instanceUrl}/api/v4.` + + (path + ? ` The instance URL includes the path "${path}" — if that is a group or project rather than ` + + `GitLab's own root, drop it and use ${new URL(instanceUrl).origin}. Keep it only if GitLab ` + + 'itself is served from that subpath.' + : ' Check this is a GitLab instance and that the host is right.') + ); +} + +/** + * Why a request never reached the host, phrased for the person who configured + * it. `fetch` reports every transport failure as `TypeError: fetch failed` + * with the real reason on `cause.code`, and a self-managed instance is where + * every one of these actually happens: behind a VPN, on an internal DNS name, + * behind a certificate the machine does not trust. + */ +export function describeTransportFailure(err: unknown, host: string): string | null { + if (!(err instanceof TypeError)) return null; + + const code = (err.cause as { code?: string } | undefined)?.code ?? ''; + const detail = + { + ENOTFOUND: `${host} does not resolve from the machine specd runs on. Check the hostname, and whether this machine needs to be on your VPN.`, + EAI_AGAIN: `${host} could not be resolved right now — a DNS failure rather than a wrong name. Check the machine's network.`, + ECONNREFUSED: `${host} refused the connection. The host resolves, so check the port and that the instance is actually serving there.`, + ETIMEDOUT: `${host} did not answer in time — typically a firewall or a VPN that is not connected.`, + UNABLE_TO_VERIFY_LEAF_SIGNATURE: `${host} presented a certificate this machine does not trust. Self-managed instances behind an internal CA need that CA installed where specd runs (NODE_EXTRA_CA_CERTS).`, + DEPTH_ZERO_SELF_SIGNED_CERT: `${host} presented a self-signed certificate. Install its CA where specd runs (NODE_EXTRA_CA_CERTS) rather than disabling verification.`, + SELF_SIGNED_CERT_IN_CHAIN: `${host} presented a self-signed certificate in its chain. Install its CA where specd runs (NODE_EXTRA_CA_CERTS).`, + CERT_HAS_EXPIRED: `${host} presented an expired certificate.`, + }[code] ?? `${host} could not be reached (${code || err.message}).`; + + return `Could not reach ${host}. ${detail}`; +} + /** * Root files worth reading in full during a scan — small, high-signal, cheap. * This is the first tier of the scan; `scan-targets.ts` adds the rest (CI, diff --git a/apps/api/src/vcs/workspace.test.ts b/apps/api/src/vcs/workspace.test.ts index dfabe64..2f413d6 100644 --- a/apps/api/src/vcs/workspace.test.ts +++ b/apps/api/src/vcs/workspace.test.ts @@ -6,6 +6,7 @@ import type { Repository } from '@specd/db'; import { afterEach, beforeEach, describe, expect, it } from 'vitest'; import type { Config } from '../config.js'; import type { VcsService } from './vcs.service.js'; +import type { LocalReviewService } from './local-review.service.js'; import { WorkspaceService } from './workspace.js'; /** @@ -20,9 +21,11 @@ describe('WorkspaceService.create — local', () => { const git = (...args: string[]) => execFileSync('git', args, { cwd: root, encoding: 'utf8' as const }); - const service = new WorkspaceService(null as unknown as VcsService, { - localOpenPr: false, - } as Config); + const service = new WorkspaceService( + null as unknown as VcsService, + { localOpenPr: false } as Config, + { credentialFor: async () => null } as unknown as LocalReviewService, + ); const repo = () => ({ provider: 'local', name: 'acme/api', localPath: root }) as Repository; diff --git a/apps/api/src/vcs/workspace.ts b/apps/api/src/vcs/workspace.ts index 1e5d0cd..dcd0d81 100644 --- a/apps/api/src/vcs/workspace.ts +++ b/apps/api/src/vcs/workspace.ts @@ -8,6 +8,7 @@ import { Config } from '../config.js'; import { GitHubAdapter } from './github.adapter.js'; import { GitLabAdapter } from './gitlab.adapter.js'; import { openLocalReview, type LocalReview } from './local-review.js'; +import { LocalReviewService } from './local-review.service.js'; import { VcsService } from './vcs.service.js'; import { VcsError, reviewHint } from './vcs.types.js'; @@ -50,6 +51,7 @@ export class WorkspaceService { constructor( private readonly vcs: VcsService, private readonly config: Config, + private readonly reviews: LocalReviewService, ) {} async create(repo: Repository, branch: string): Promise { @@ -114,7 +116,7 @@ export class WorkspaceService { // not, this is what it always was (`local-review.ts`, decision 0020). publish: async (pr) => { const review = this.config.localOpenPr - ? await this.openLocalReview(dir, { branch, base: baseBranch, ...pr }) + ? await this.openLocalReview(dir, repo.projectId, { branch, base: baseBranch, ...pr }) : null; if (review?.url) { @@ -147,6 +149,7 @@ export class WorkspaceService { */ private async openLocalReview( dir: string, + projectId: string | undefined, pr: { branch: string; base: string; title: string; body: string }, ): Promise { const git = simpleGit({ baseDir: dir }); @@ -156,7 +159,9 @@ export class WorkspaceService { .catch(() => ''); if (!remoteUrl) return null; - return openLocalReview({ git, cwd: dir, remoteUrl, ...pr }).catch((err: unknown) => ({ + const credential = await this.reviews.credentialFor(projectId).catch(() => null); + + return openLocalReview({ git, cwd: dir, remoteUrl, credential, ...pr }).catch((err: unknown) => ({ url: null, note: `opening a pull request failed (${err instanceof Error ? err.message : String(err)})`, })); diff --git a/apps/web/app/setup/page.tsx b/apps/web/app/setup/page.tsx index 2adbe30..f77af93 100644 --- a/apps/web/app/setup/page.tsx +++ b/apps/web/app/setup/page.tsx @@ -117,6 +117,12 @@ function SetupWizard() { const [pathCheck, setPathCheck] = useState<{ ok: boolean; clean?: boolean; branch?: string; reason?: string } | null>(null); const [repos, setRepos] = useState([]); + // step 2 · local mode's optional review credential + const [reviewProvider, setReviewProvider] = useState<'' | 'github' | 'gitlab'>(''); + const [reviewInstanceUrl, setReviewInstanceUrl] = useState(''); + const [reviewToken, setReviewToken] = useState(''); + const [reviewCheck, setReviewCheck] = useState<{ ok: boolean; detail: string } | null>(null); + // step 2 · GitLab const [gitlabToken, setGitlabToken] = useState(''); const [gitlabInstanceUrl, setGitlabInstanceUrl] = useState(''); @@ -431,9 +437,11 @@ function SetupWizard() { setBusyAction('continue-2'); setError(null); try { - await post(`/projects/${project.slug}/connections/vcs`, { provider: vcs }); + await post(`/projects/${project.slug}/connections/vcs`, localVcsBody()); goTo(3); } catch (err) { + // A rejected review token must not read as "local mode is broken" — it + // is the one optional thing on this step. fail(err); } finally { setBusy(false); @@ -441,6 +449,48 @@ function SetupWizard() { } } + /** Local mode's connection, with its optional review credential if given. */ + function localVcsBody() { + return reviewProvider + ? { + provider: 'local', + reviewProvider, + token: reviewToken, + instanceUrl: reviewInstanceUrl.trim() || undefined, + } + : { provider: 'local' }; + } + + /** + * Prove the review token before Continue does, so a bad one is answered on + * the field rather than as a failure to advance. Storing it here as well is + * deliberate: this *is* the connect call, and repeating it on Continue is + * idempotent. + */ + async function checkReviewCredential() { + if (!project || !reviewProvider || !reviewToken) return; + setBusy(true); + setBusyAction('check-review'); + setReviewCheck(null); + try { + const res = await post<{ ok: boolean; connectedAs?: string }>( + `/projects/${project.slug}/connections/vcs`, + localVcsBody(), + ); + setReviewCheck({ + ok: true, + detail: res.connectedAs + ? `Token accepted — ${reviewProvider === 'gitlab' ? 'GitLab' : 'GitHub'} knows it as ${res.connectedAs}.` + : 'Token accepted.', + }); + } catch (err) { + setReviewCheck({ ok: false, detail: err instanceof Error ? err.message : String(err) }); + } finally { + setBusy(false); + setBusyAction(null); + } + } + async function connectAi() { if (!project || !aiMode) return; setBusy(true); @@ -843,9 +893,14 @@ function SetupWizard() { id="glurl" value={gitlabInstanceUrl} onChange={(e) => setGitlabInstanceUrl(e.target.value)} - placeholder="gitlab.com — leave blank for gitlab.com" + placeholder="https://gitlab.example.com — leave blank for gitlab.com" spellCheck={false} /> + + The instance's origin. specd reaches it from the machine it runs on, so + a host behind a VPN needs this machine on that VPN, and an internal CA + needs to be trusted here (NODE_EXTRA_CA_CERTS). + {gitlabError &&
{gitlabError}
} )} + + {/* Optional, and off by default: local mode's promise is that + specd holds no key to your host. This is the way to say + "open the merge request for me anyway" — used for that and + nothing else. */} +
+ + + + 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. + +
+ + {reviewProvider && ( + <> +
+ + setReviewInstanceUrl(e.target.value)} + placeholder={ + reviewProvider === 'gitlab' + ? 'https://gitlab.example.com — blank for gitlab.com' + : 'https://github.example.com — blank for github.com' + } + 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. + +
+
+ + setReviewToken(e.target.value)} + placeholder={reviewProvider === 'gitlab' ? 'glpat-…' : 'ghp_…'} + spellCheck={false} + /> + + {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.'} + +
+ {reviewCheck && ( +
+ {reviewCheck.ok ? <>✓ {reviewCheck.detail} : <>✕ {reviewCheck.detail}} +
+ )} + + + )} )} diff --git a/apps/web/lib/docs/guides.ts b/apps/web/lib/docs/guides.ts index 65924e9..132dbbb 100644 --- a/apps/web/lib/docs/guides.ts +++ b/apps/web/lib/docs/guides.ts @@ -58,7 +58,17 @@ export const GUIDES: DocCategory = { }, { k: 'p', - text: 'specd holds no host credential in this mode and never will — but where `origin` points at GitHub or GitLab and that host\'s CLI (`gh`, `glab`) is installed and signed in on the same machine, setup and build branches are pushed and opened as real pull requests through **your** account. The CLI is checked before anything is pushed, so a repository specd cannot open a review on is never published to. Set `SPECD_LOCAL_OPEN_PR=0` to keep everything local; the branch is committed either way.', + text: 'Where `origin` points at github.com or gitlab.com and that host\'s CLI (`gh`, `glab`) is installed and signed in on the same machine, setup and build branches are pushed and opened as real pull requests through **your** account. The CLI is checked before anything is pushed, so a repository specd cannot open a review on is never published to. Set `SPECD_LOCAL_OPEN_PR=0` to keep everything local; the branch is committed either way.', + }, + { + 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.', + }, + { + k: 'note', + tone: 'rule', + title: 'The review credential is not a second connection', + text: 'It never reads a file, lists a tree, clones or pushes — your own git credentials still do the push, and specd still reads the repository from disk. Leave it unset and local mode holds no credential at all, exactly as before.', }, { k: 'h2', text: 'GitHub — as an App, not a personal token' }, { diff --git a/apps/web/lib/docs/integrations.ts b/apps/web/lib/docs/integrations.ts index 93ef38e..3246ba8 100644 --- a/apps/web/lib/docs/integrations.ts +++ b/apps/web/lib/docs/integrations.ts @@ -191,6 +191,22 @@ curl -H "Authorization: Bearer $TOKEN" \\ k: 'p', text: 'The complete walkthrough — self-managed instance URLs, webhook registration, verification — is in `docs/gitlab.md` in the repository.', }, + { k: 'h2', text: 'When a self-managed instance does not connect' }, + { + k: 'p', + text: 'specd reaches your instance **from the machine specd runs on**, not from your browser — so every check is about that machine. A bare host like `gitlab.example.com` is accepted and read as https; anything that cannot become an http(s) origin is refused on the connect call itself.', + }, + { + k: 'table', + head: ['What the error says', 'What to fix'], + rows: [ + ['`does not resolve from the machine specd runs on`', 'Wrong hostname, or this machine is not on the VPN that can see it.'], + ['`refused the connection`', 'The host resolves; the port is wrong, or nothing is serving there.'], + ['`did not answer in time`', 'Typically a firewall, or a VPN that is not connected.'], + ['`presented a certificate this machine does not trust`', 'An internal CA — point `NODE_EXTRA_CA_CERTS` at it where specd runs, rather than disabling verification.'], + ['`→ 401`', 'The instance answered. The token is the problem, not the network.'], + ], + }, { k: 'h2', text: 'Webhook trust' }, { k: 'p', diff --git a/docs/gitlab.md b/docs/gitlab.md index 34ee329..0f64eec 100644 --- a/docs/gitlab.md +++ b/docs/gitlab.md @@ -66,7 +66,23 @@ curl -X POST "$SPECD_API/projects/$PROJECT_SLUG/connections/vcs" \ ``` Self-managed instance: add `"instanceUrl": "https://gitlab.example.com"`. -Omit it for gitlab.com. +Omit it for gitlab.com. A bare host (`gitlab.example.com`) is accepted and +read as https; anything specd cannot turn into an http(s) origin is refused +here, on this call, rather than at the repository listing one call later. + +### When a self-managed instance does not connect + +specd reaches your instance **from the machine specd runs on**, not from your +browser — so the checks are about that machine. Every one of these now comes +back as a sentence naming the cause, rather than as "Internal server error": + +| What you see | What it means | +| --- | --- | +| `does not resolve from the machine specd runs on` | Wrong hostname, or this machine is not on the VPN that can see it. | +| `refused the connection` | The host resolves; the port is wrong, or nothing is serving there. | +| `did not answer in time` | Typically a firewall, or a VPN that is not connected. | +| `presented a certificate this machine does not trust` | An internal CA. Point `NODE_EXTRA_CA_CERTS` at it where specd runs — do not disable verification. | +| `→ 401` | The instance answered. The token is the problem, not the network. | **3. Find and add a repository.** The picker reads live from the token — specd cannot see anything it was not granted: diff --git a/knowledge/decisions/0020-local-mode-borrows-the-host-cli.md b/knowledge/decisions/0020-local-mode-borrows-the-host-cli.md index 9df5825..2b4d9e7 100644 --- a/knowledge/decisions/0020-local-mode-borrows-the-host-cli.md +++ b/knowledge/decisions/0020-local-mode-borrows-the-host-cli.md @@ -84,3 +84,38 @@ and re-building a spec are both normal things to do. repository the user registered and to the remote that repository already points at, with the user's own credentials. It does not acquire a token, a webhook, or an account. + +## Amendment, 2026-08-17 — a named host may be given a token + +Property 2 above says specd will not guess a self-managed host's software from +its URL. That was right, and it left a hole: a repository on a self-managed +GitLab reached the end of every local-mode run with a branch and no review, +because `detectHost` answered null and `glab` is not on most corporate +machines. The fix people asked for is the obvious one — *"I have a token, use +it"* — and the reasoning above does not forbid it. **Guessing** the host is +what was refused. Being **told** the host is not a guess. + +So a local-mode project may now carry an optional review credential: +`settings.reviewProvider` plus a token on the `vcs` connection it already owns +(no migration — that row's `encrypted_secret` and `settings.instanceUrl` were +both unused for `provider: 'local'`). When present, `openLocalReview` opens the +review through `GitLabAdapter.openMergeRequest` / `GitHubAdapter.openPullRequest` +instead of a CLI, and the host restriction lifts, because there is nothing left +to infer. + +What this deliberately does **not** become is a second VCS connection: + +- The token opens a review. It never reads a file, lists a tree, clones, or + pushes — the machine's own git does the push, exactly as before. Local mode + still reads and writes the repository on disk. +- It is optional and absent by default. A project without it behaves as this + decision originally specified, down to the wording of the hint. +- It is proved at connect time (`verify()` on either adapter, mirroring + `JiraAdapter.verify()`), so a wrong token fails in the wizard rather than as + a merge request that never appears. + +The honest cost: local mode's promise narrows from "specd holds no credential +for your host" to "specd holds no credential for your host unless you give it +one, and then only to open reviews". That is a real change to the sentence, and +it is why the credential is opt-in, single-purpose, and named as such in the +UI rather than folded into the repository form. diff --git a/knowledge/glossary.md b/knowledge/glossary.md index 6d4e47f..00ebd76 100644 --- a/knowledge/glossary.md +++ b/knowledge/glossary.md @@ -25,7 +25,8 @@ Domain terms mined from code and docs. | **subscription_runner** | AI-connection mode (`connections.settings.mode`) that drives a locally logged-in Claude Code CLI instead of an API key. Executed on a paired self-hosted runner (`apps/runner`), so hosted specd can use it too — the credential never reaches the platform. The `SPECD_AI_MODE` env var sets it only for the headless loop. | | **loop (`pnpm --filter @specd/api loop`)** | Headless end-to-end exercise of every station over the real HTTP API, reporting pass/skip per station. | | **VCS adapter** | `VcsAdapter` interface (`apps/api/src/vcs/vcs.types.ts`) implemented by GitHub, GitLab and local repos — everything above it (onboarding, indexing, the build station) is provider-agnostic against it. | -| **local mode** | Repo registered via `specd connect .`; code stays on the machine and specd holds no host credential. Output is a branch — pushed and opened as a real PR/MR where `gh`/`glab` is signed in on that machine (`knowledge/decisions/0020-local-mode-borrows-the-host-cli.md`), and a branch you diff where it is not. | +| **local mode** | Repo registered via `specd connect .`; code is read and written on disk. Output is a branch — pushed and opened as a real PR/MR where `gh`/`glab` is signed in on that machine, or where the project was given an optional **review credential** (`knowledge/decisions/0020-local-mode-borrows-the-host-cli.md`), and a branch you diff where neither applies. | +| **review credential** | Optional token on a local-mode project's `vcs` connection (`settings.reviewProvider` + the encrypted secret), used for one thing: opening the pull/merge request. It never reads a file, clones, or pushes — git does the push with the machine's own credentials. It is what lets local mode reach a self-managed host, which specd refuses to identify by URL alone. | | **delivery id** | Per-webhook-delivery identifier used to deduplicate replayed deliveries: GitHub's `X-GitHub-Delivery`, GitLab's `X-Gitlab-Event-UUID`. | | **installation** | GitHub App installation; a webhook is acted on only when repository *and* installation match a registered project. GitLab has no equivalent — it authenticates with a personal/project access token instead (`knowledge/decisions/0002-gitlab-via-personal-access-token.md`). | | **instance URL** | Self-managed GitLab host stored on a connection (`connections.settings.instanceUrl`); absent means gitlab.com. Scopes webhook resolution so two instances cannot share a numeric project id. |