GitLab opens the merge request from the push, so local mode needs no token

apps/api/src/projects/projects.controller.ts

@@ -32,6 +32,8 @@ import { ModelRouter } from '../agents/model.router.js';
 import { Vault } from '../common/vault.js';
 import { JiraAdapter } from '../tracker/jira.adapter.js';
 import { VcsError, normalizeInstanceUrl } from '../vcs/vcs.types.js';
+import { instanceUrlFromRemote, resolveGitLabRoot } from '../vcs/local-review.js';
+import { LocalGitAdapter } from '../vcs/local-git.adapter.js';
 import { GitHubAdapter } from '../vcs/github.adapter.js';
 import { GitLabAdapter } from '../vcs/gitlab.adapter.js';
 
@@ -105,6 +107,7 @@ export class ProjectsController {
     private readonly connections: ConnectionsService,
     private readonly knowledge: KnowledgeService,
     private readonly modelRouter: ModelRouter,
+    private readonly local: LocalGitAdapter,
   ) {}
 
   @Get()
@@ -324,21 +327,38 @@ export class ProjectsController {
     // wrong.
     let connectedAs: string | undefined;
     const reviewProvider = dto.provider === 'local' ? dto.reviewProvider ?? null : null;
-    if (reviewProvider) {
-      if (!dto.token) {
+    if (reviewProvider === 'github' && !dto.token) {
+      // GitHub has no way to open a pull request except the API.
+      throw new BadRequestException(
+        'A GitHub token is needed to open pull requests from local mode. Leave the review host ' +
+          'unset to keep local mode credential-free.',
+      );
+    }
+
+    // A GitLab project with no token is a supported, and preferred, setup:
+    // push options open the merge request over the git transport. There is
+    // nothing to verify in that case, so nothing is checked.
+    if (reviewProvider && dto.token) {
+      // Verified against the host the repository names, not against whatever
+      // the adapter would default to. A check that passes against gitlab.com
+      // for a self-managed project is worse than no check at all.
+      const root = instanceUrl ?? (await this.derivedReviewHost(project.id, reviewProvider));
+      if (!root) {
         throw new BadRequestException(
-          `A ${reviewProvider === 'gitlab' ? 'GitLab' : 'GitHub'} token is needed to open reviews from local mode. Leave the review host unset to keep local mode credential-free.`,
+          'specd could not work out which host to open reviews on. Add a repository with an ' +
+            '`origin` remote first, or give the instance URL explicitly.',
         );
       }
+
       try {
         const identity =
           reviewProvider === 'gitlab'
-            ? await new GitLabAdapter(dto.token, instanceUrl ?? undefined).verify()
+            ? await new GitLabAdapter(dto.token, root).verify()
             : await new GitHubAdapter(
                 dto.token,
-                instanceUrl ? `${instanceUrl}/api/v3` : undefined,
+                /^https:\/\/github\.com$/.test(root) ? undefined : `${root}/api/v3`,
               ).verify();
-        connectedAs = identity.username;
+        connectedAs = `${identity.username} on ${root}`;
       } catch (err) {
         throw new BadRequestException(
           err instanceof VcsError ? err.message : err instanceof Error ? err.message : String(err),
@@ -357,6 +377,31 @@ export class ProjectsController {
     return connectedAs ? { ok: true, connectedAs } : { ok: true };
   }
 
+  /**
+   * The review host a project's own checkouts point at.
+   *
+   * Local mode already holds a clone, and a clone knows its origin — so the
+   * instance URL is something specd can read rather than something a person
+   * has to retype. Only *which software* the host runs cannot be read off a
+   * remote, which is why the provider is still chosen by hand.
+   */
+  private async derivedReviewHost(
+    projectId: string,
+    provider: 'github' | 'gitlab',
+  ): Promise<string | null> {
+    const repos = await this.repositories.list(projectId);
+    for (const repo of repos) {
+      if (!repo.localPath) continue;
+      const origin = await this.local.originUrl(repo.localPath);
+      const derived = origin ? instanceUrlFromRemote(origin) : null;
+      if (!derived) continue;
+      // A subpath-hosted instance is indistinguishable from a group by string
+      // alone, so GitLab is asked rather than guessed at.
+      return provider === 'gitlab' ? resolveGitLabRoot(derived, origin!) : derived;
+    }
+    return null;
+  }
+
   @Post(':slug/connections/tracker')
   async connectTracker(
     @Param('slug') slug: string,

apps/api/src/vcs/local-git.adapter.ts

@@ -319,6 +319,22 @@ export class LocalGitAdapter implements VcsAdapter {
     }
   }
 
+  /**
+   * The `origin` a registered checkout points at, if it has one.
+   *
+   * Read at connect time so a review credential can be checked against the
+   * host the repository actually names, rather than against whatever the
+   * adapter would have defaulted to.
+   */
+  async originUrl(localPath: string): Promise<string | null> {
+    try {
+      const url = await simpleGit({ baseDir: resolve(localPath) }).remote(['get-url', 'origin']);
+      return (url || '').trim() || null;
+    } catch {
+      return null;
+    }
+  }
+
   async describe(path: string): Promise<{ clean: boolean; branch: string; head: string } | null> {
     const root = await this.repoRootOf(path);
     if (!root) return null;

apps/api/src/vcs/local-review.service.ts

@@ -37,27 +37,29 @@ export class LocalReviewService {
     if (!projectId) return null;
 
     const conn = await this.connections.get(projectId, 'vcs').catch(() => null);
-    if (!conn || conn.provider !== 'local' || !conn.encryptedSecret) return null;
+    if (!conn || conn.provider !== 'local') return null;
 
     const settings = (conn.settings ?? {}) as { reviewProvider?: string; instanceUrl?: string | null };
     const provider = settings.reviewProvider;
     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;
+    // construction everywhere else in this path. Nor is its absence: naming
+    // the provider is enough for GitLab's push-option route.
+    const token = conn.encryptedSecret
+      ? (() => {
+          try {
+            return this.vault.decrypt(conn.encryptedSecret!, `${projectId}:vcs`);
+          } catch {
+            return null;
+          }
+        })()
+      : null;
 
     return {
       provider,
-      token,
+      token: token || null,
       instanceUrl: settings.instanceUrl ? normalizeInstanceUrl(settings.instanceUrl) : null,
     };
   }

apps/api/src/vcs/local-review.test.ts

@@ -4,7 +4,14 @@ import { tmpdir } from 'node:os';
 import { join } from 'node:path';
 import { simpleGit } from 'simple-git';
 import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from 'vitest';
-import { detectHost, openLocalReview, projectPathFromRemote } from './local-review.js';
+import {
+  describeForPushOption,
+  detectHost,
+  instanceUrlFromRemote,
+  openLocalReview,
+  projectPathFromRemote,
+  resolveGitLabRoot,
+} from './local-review.js';
 
 describe('detectHost', () => {
   it('recognizes github and gitlab over ssh and https alike', () => {
@@ -223,3 +230,146 @@ describe('a configured review credential', () => {
     }
   });
 });
+
+describe('instanceUrlFromRemote', () => {
+  it('reads the host a clone already names, in either remote syntax', () => {
+    expect(instanceUrlFromRemote('git@gitlab.example.com:acme/services/api.git')).toBe(
+      'https://gitlab.example.com',
+    );
+    expect(instanceUrlFromRemote('https://gitlab.example.com/acme/services/api.git')).toBe(
+      'https://gitlab.example.com',
+    );
+  });
+
+  it('drops an ssh port and keeps a web one', () => {
+    // `:2222` on an ssh remote is the SSH port. Carrying it onto an API URL
+    // would be confidently wrong, which is worse than asking.
+    expect(instanceUrlFromRemote('ssh://git@gitlab.example.com:2222/acme/api.git')).toBe(
+      'https://gitlab.example.com',
+    );
+    expect(instanceUrlFromRemote('https://gitlab.example.com:8443/acme/api.git')).toBe(
+      'https://gitlab.example.com:8443',
+    );
+  });
+
+  it('has no answer for a remote with no host', () => {
+    expect(instanceUrlFromRemote('/srv/git/bare.git')).toBeNull();
+    expect(instanceUrlFromRemote('')).toBeNull();
+  });
+});
+
+describe('a subpath-hosted instance', () => {
+  const instance = 'https://intranet.example.com/gitlab';
+
+  it('strips the instance prefix from both remote syntaxes', () => {
+    // The regression: the prefix was compared against `URL.pathname`, which
+    // carries a leading slash, while the scp branch produced one without —
+    // so an ssh remote silently kept the instance path inside the project
+    // path, and ssh is what a corporate clone actually uses.
+    expect(projectPathFromRemote('https://intranet.example.com/gitlab/group/project.git', instance)).toBe(
+      'group/project',
+    );
+    expect(projectPathFromRemote('git@intranet.example.com:gitlab/group/project.git', instance)).toBe(
+      'group/project',
+    );
+  });
+
+  it('leaves a path alone when it merely starts with similar letters', () => {
+    // `gitlab-runner/...` is not inside `/gitlab`, and a prefix match without
+    // the separator would eat four characters off a real group name.
+    expect(
+      projectPathFromRemote('git@intranet.example.com:gitlab-runner/project.git', instance),
+    ).toBe('gitlab-runner/project');
+  });
+});
+
+describe('resolveGitLabRoot', () => {
+  const remote = 'git@gitlab.example.com:ET130/services/api.git';
+
+  it('keeps the root when GitLab answers there — the leading segment is a group', () => {
+    const probe = async (url: string) => (url === 'https://gitlab.example.com/api/v4/version' ? 401 : 404);
+    return expect(resolveGitLabRoot('https://gitlab.example.com', remote, probe)).resolves.toBe(
+      'https://gitlab.example.com',
+    );
+  });
+
+  it('walks one segment in when GitLab is served from a subpath', async () => {
+    const probe = async (url: string) =>
+      url === 'https://gitlab.example.com/ET130/api/v4/version' ? 200 : 404;
+    await expect(resolveGitLabRoot('https://gitlab.example.com', remote, probe)).resolves.toBe(
+      'https://gitlab.example.com/ET130',
+    );
+  });
+
+  it('prefers the root, and returns it unchanged when neither answers', async () => {
+    // A wrong instance URL is then reported by describeApiBase404, which says
+    // which half to drop — better than silently addressing a group.
+    await expect(resolveGitLabRoot('https://gitlab.example.com', remote, async () => 404)).resolves.toBe(
+      'https://gitlab.example.com',
+    );
+  });
+});
+
+describe('describeForPushOption', () => {
+  it('passes a short body through untouched', () => {
+    expect(describeForPushOption('short')).toBe('short');
+  });
+
+  it('trims a long one and says that it did', () => {
+    const out = describeForPushOption('x'.repeat(4000));
+    expect(out.length).toBeLessThan(1700);
+    expect(out).toMatch(/truncated/);
+  });
+});
+
+describe('the push-option route against a remote that does not take them', () => {
+  // A local bare repo refuses push options exactly as GitLab before 11.10
+  // does: it rejects the whole push and sends nothing. The branch still has to
+  // arrive, so the retry is what keeps a working setup working.
+  let dir = '';
+  let remote = '';
+  const git = (...args: string[]) =>
+    execFileSync('git', args, { cwd: dir, encoding: 'utf8' as const });
+
+  beforeAll(() => {
+    dir = mkdtempSync(join(tmpdir(), 'specd-pushopt-'));
+    remote = mkdtempSync(join(tmpdir(), 'specd-pushopt-remote-'));
+    execFileSync('git', ['init', '-q', '--bare', remote]);
+    git('init', '-q', '-b', 'main');
+    git('config', 'user.email', 't@t.dev');
+    git('config', 'user.name', 'T');
+    git('remote', 'add', 'origin', remote);
+    writeFileSync(join(dir, 'README.md'), '# x\n');
+    git('add', '-A');
+    git('commit', '-qm', 'init');
+    git('checkout', '-q', '-b', 'spec/E-9-thing');
+    writeFileSync(join(dir, 'f.txt'), 'work\n');
+    git('add', '-A');
+    git('commit', '-qm', 'work');
+    git('checkout', '-q', 'main');
+  });
+
+  afterAll(() => {
+    rmSync(dir, { recursive: true, force: true });
+    rmSync(remote, { recursive: true, force: true });
+  });
+
+  it('still publishes the branch, and says the merge request did not come from the push', async () => {
+    const result = await openLocalReview({
+      git: simpleGit({ baseDir: dir }),
+      cwd: dir,
+      remoteUrl: 'git@gitlab.example.com:acme/services/api.git',
+      branch: 'spec/E-9-thing',
+      base: 'main',
+      title: '[E-9] - Thing',
+      body: 'body',
+      credential: { provider: 'gitlab', token: null, instanceUrl: null },
+    });
+
+    expect(
+      execFileSync('git', ['branch', '--list', 'spec/E-9-thing'], { cwd: remote, encoding: 'utf8' }),
+    ).toContain('spec/E-9-thing');
+    expect(result.url).toBeNull();
+    expect(result.note).toMatch(/did not report a merge request from the push/);
+  });
+});