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. |