Skip to content

fix(forgejo): review guides work for checkouts whose SSH remote is not the web host - #29784

Open
juliusmarminge wants to merge 4 commits into
mainfrom
fix/issue-29753
Open

juliusmarminge wants to merge 4 commits into
mainfrom
fix/issue-29753

Conversation

@juliusmarminge

Copy link
Copy Markdown
Member

Problem

In a Forgejo project whose origin is an SSH remote on a different host from the web UI (for example ssh://git@git-ssh.internal:222/owner/repo.git for a server at https://git.example.com), "Write review guide" on the project's own PR fails with "this pull request is in another repository". The project's identity already carries the resolved webUrl, but Forgejo's changeRequestHost ignored it for non-HTTP remotes and returned the SSH host, so the guide service's own-repository check compared git-ssh.internal against the PR's git.example.com.

Reproduced on main against a real Forgejo 11.0.16 in Docker with a tea 0.16 login whose ssh_host differs from its URL host: both ssh:// and scp-style remotes are refused, an HTTP remote passes.

Change

Forgejo's changeRequestHost now returns the host (with port) of the resolved webUrl when the identity has one, and otherwise falls back to the HTTP remote's host and then the canonical-key host as before. This matches the definition's changeRequestUrl, isChangeRequestInRepository and canReadChangeRequestOnHost, which already prefer the resolved web URL. ChangeRequestHostIdentity gains the optional webUrl field so the existing resolvedWebUrl helper can read it; every caller already passes a full RepositoryIdentity.

HTTP remotes are unaffected: the login that sets webUrl is matched by the HTTP remote's host, so both name the same host. Identities without a webUrl behave exactly as before.

Scope and approval

Fixes #29753

Verification

  • PullRequestReviewGuideService.test.ts › "writes a guide for a Forgejo checkout whose SSH remote is not the web host": fails on main with the "another repository" error, passes with the fix (the guide is stored for the PR head).
  • source-control-forgejo definition.test.ts › "Forgejo change request host": new case, an SSH remote with web URL http://forge.example:3000/... resolves to forge.example:3000 (fails on main with forge.example).
  • vp run check:changed on the merged head: format, lint, knip, typecheck (18 packages) and tests pass.
  • Runtime, real server and real Forgejo 11.0.16 (Docker), tea 0.16.0 login url=http://localhost:3000, ssh_host=127.0.0.1. Each run starts the checkout's server on a fresh base dir, adds the checkout as a project, and calls pullRequests.generateReviewGuide for localhost:3000 owner/repo#1:
Remote Before (5b9731c) After (da3df0e)
ssh://git@127.0.0.1:2222/owner/repo.git refused, "another repository" guide run starts
git@127.0.0.1:owner/repo.git refused, "another repository" guide run starts
http://localhost:3000/owner/repo.git (control) guide run starts guide run starts
Runtime transcript excerpt
== before-ssh
checkout @ 5b9731cedc
  "canonicalKey": "127.0.0.1/owner/repo",
  "remoteUrl": "ssh://git@127.0.0.1:2222/owner/repo.git"
  "webUrl": "http://localhost:3000/owner/repo",
  "_tag": "Failure",
  "detail": "Guides are written from this project's checkout, and this pull request is in another repository. Write it from a project that is a checkout of that repository."
== before-scp
checkout @ 5b9731cedc
  "canonicalKey": "127.0.0.1/owner/repo",
  "remoteUrl": "git@127.0.0.1:owner/repo.git"
  "webUrl": "http://localhost:3000/owner/repo",
  "_tag": "Failure",
  "detail": "Guides are written from this project's checkout, and this pull request is in another repository. Write it from a project that is a checkout of that repository."
== before-http
checkout @ 5b9731cedc
  "canonicalKey": "localhost/owner/repo",
  "remoteUrl": "http://localhost:3000/owner/repo.git"
  "webUrl": "http://localhost:3000/owner/repo",
  "_tag": "Success",
  "_tag": "Running",
== after-ssh
checkout @ da3df0e6f0
  "canonicalKey": "127.0.0.1/owner/repo",
  "remoteUrl": "ssh://git@127.0.0.1:2222/owner/repo.git"
  "webUrl": "http://localhost:3000/owner/repo",
  "_tag": "Success",
  "_tag": "Running",
== after-scp
checkout @ da3df0e6f0
  "canonicalKey": "127.0.0.1/owner/repo",
  "remoteUrl": "git@127.0.0.1:owner/repo.git"
  "webUrl": "http://localhost:3000/owner/repo",
  "_tag": "Success",
  "_tag": "Running",
== after-http
checkout @ da3df0e6f0
  "canonicalKey": "localhost/owner/repo",
  "remoteUrl": "http://localhost:3000/owner/repo.git"
  "webUrl": "http://localhost:3000/owner/repo",
  "_tag": "Success",
  "_tag": "Running",
Driver (scratch, not committed; synthetic Forgejo data only)
// Scratch driver for #29753 (never committed). Usage: node repro.ts <checkout> <runDir>
// Starts the checkout's server on a fresh base dir, adds the SSH-remote Forgejo checkout as a
// project, prints its repository identity, then asks for a review guide of PR #1.
import * as NodeChildProcess from "node:child_process";
import * as NodeFS from "node:fs";
import * as NodePath from "node:path";
import * as NodeSocket from "@effect/platform-node/NodeSocket";
import {
  ORCHESTRATION_PROTOCOL_HEADER,
  ORCHESTRATION_PROTOCOL_VERSION,
  ProjectId,
  WsRpcGroup,
} from "@t3tools/contracts";
import * as Effect from "effect/Effect";
import * as Layer from "effect/Layer";
import { FetchHttpClient, HttpClient, HttpClientRequest, HttpClientResponse } from "effect/http";
import { RpcClient, RpcSerialization } from "effect/rpc";
import { Socket } from "effect/socket";
import * as Schema from "effect/Schema";

const [checkout, runDir] = process.argv.slice(2) as [string, string];
const SCRATCH = "/tmp/issue-lead-29753-r1";
const WORKSPACE = `${SCRATCH}/project`;
const PORT = 3973;
const log = (...parts: unknown[]) => console.log(...parts);

NodeFS.mkdirSync(NodePath.join(runDir, "userdata"), { recursive: true });
NodeFS.writeFileSync(
  NodePath.join(runDir, "userdata/settings.json"),
  JSON.stringify({ reviewGuidesEnabled: true }),
);
const env = {
  ...process.env,
  PATH: `${SCRATCH}/bin:${process.env.PATH}`,
  XDG_CONFIG_HOME: `${SCRATCH}/xdg`,
  GIT_SSH_COMMAND: `ssh -i ${SCRATCH}/id_repro -o IdentitiesOnly=yes -o UserKnownHostsFile=${SCRATCH}/known_hosts`,
  T3CODE_DEV_SEED: "0",
};
const serverLog = NodeFS.openSync(NodePath.join(runDir, "server.log"), "w");
const server = NodeChildProcess.spawn(
  "node",
  ["apps/server/src/bin.ts", "serve", "--host", "127.0.0.1", "--port", String(PORT),
    "--base-dir", runDir, "--no-browser", "--log-level", "warn", WORKSPACE],
  { cwd: checkout, stdio: ["ignore", serverLog, serverLog], env },
);
log(`server pid ${server.pid} on ${PORT}, checkout ${checkout} @ ${NodeChildProcess.execFileSync("git", ["-C", checkout, "rev-parse", "--short", "HEAD"], { encoding: "utf8" }).trim()}`);

const program = Effect.gen(function* () {
  for (let attempt = 0; ; attempt++) {
    const ok = yield* Effect.promise(() =>
      fetch(`http://127.0.0.1:${PORT}/.well-known/t3/environment`).then(() => true, () => false),
    );
    if (ok) break;
    if (attempt > 120) return yield* Effect.die("server never listened");
    yield* Effect.sleep("500 millis");
  }
  const issued = NodeChildProcess.spawnSync(
    "node",
    ["apps/server/src/bin.ts", "auth", "session", "issue", "--base-dir", runDir, "--token-only"],
    { cwd: checkout, encoding: "utf8", env },
  );
  const token = `${issued.stdout}\n${issued.stderr}`.split("\n").filter((l) => l.trim()).at(-1)!.trim();
  const origin = `http://127.0.0.1:${PORT}`;
  const http = (yield* HttpClient.HttpClient).pipe(
    HttpClient.mapRequest((request) =>
      request.pipe(
        HttpClientRequest.bearerToken(token),
        HttpClientRequest.setHeader(ORCHESTRATION_PROTOCOL_HEADER, String(ORCHESTRATION_PROTOCOL_VERSION)),
      ),
    ),
    HttpClient.filterStatusOk,
  );
  const projectId = ProjectId.make(crypto.randomUUID());
  yield* http.execute(
    HttpClientRequest.post(`${origin}/api/projects/mutate`).pipe(
      HttpClientRequest.bodyJsonUnsafe({
        type: "project.create",
        commandId: crypto.randomUUID(),
        projectId,
        title: "repo",
        workspaceRoot: WORKSPACE,
      }),
    ),
  );
  const ticket = yield* http
    .execute(HttpClientRequest.post(`${origin}/api/auth/websocket-ticket`))
    .pipe(Effect.flatMap(HttpClientResponse.schemaBodyJson(Schema.Struct({ ticket: Schema.String }))));
  const protocol = yield* Layer.build(
    RpcClient.layerProtocolSocket().pipe(
      Layer.provide(
        Socket.layerWebSocket(
          `ws://127.0.0.1:${PORT}/ws?orchestrationProtocol=${ORCHESTRATION_PROTOCOL_VERSION}&wsTicket=${encodeURIComponent(ticket.ticket)}`,
        ),
      ),
      Layer.provide(RpcSerialization.layerJson),
      Layer.provide(NodeSocket.layerWebSocketConstructorWS),
    ),
  );
  const client = yield* RpcClient.make(WsRpcGroup).pipe(Effect.provideContext(protocol));

  // Wait for the project's repository identity, refined through the tea login, to resolve.
  let identity: unknown = null;
  for (let attempt = 0; attempt < 60 && identity === null; attempt++) {
    const shell = yield* http.get(`${origin}/api/orchestration/shell`).pipe(Effect.flatMap((r) => r.json));
    const project = (shell as { projects: Array<{ id: string; repositoryIdentity?: { webUrl?: string } }> })
      .projects.find((p) => p.id === projectId);
    if (project?.repositoryIdentity?.webUrl) identity = project.repositoryIdentity;
    else yield* Effect.sleep("500 millis");
  }
  log("project repositoryIdentity:", JSON.stringify(identity, null, 2));

  for (const host of (process.env.DETAIL_HOSTS ?? "").split(",").filter(Boolean)) {
    const detail = yield* client["pullRequests.detail"]({ projectId, host, repository: "owner/repo", number: 1 }).pipe(Effect.result);
    log(`pullRequests.detail host=${host}:`, detail._tag === "Success" ? `Success headSha=${detail.success.headSha} url=${detail.success.url}` : JSON.stringify(detail.failure));
  }
  const reference = { projectId, host: "localhost:3000", repository: "owner/repo", number: 1 };
  log("pullRequests.generateReviewGuide", JSON.stringify(reference));
  const result = yield* client["pullRequests.generateReviewGuide"](reference).pipe(Effect.result);
  log("result:", JSON.stringify(result, null, 2).slice(0, 1500));
});

Effect.runPromise(
  program.pipe(Effect.scoped, Effect.provide(FetchHttpClient.layer)),
).then(
  () => server.kill("SIGTERM"),
  (error) => (console.error("driver failed:", error), server.kill("SIGTERM")),
);

No visual change, so no captures. Not checked: a full guide completion with a real model (the runtime runs show the run starting; the unit test covers storing the guide), and a Forgejo mounted below a path with an SSH remote, which is a separate case.

Triaged, reproduced and fixed by Claude Opus 5.5 (claude-opus-5-5) through the Claude Code harness in T3 Code, with review by gpt-6.1-sol (Codex) and Claude Opus 5.5.

🤖 Generated with Claude Code

juliusmarminge and others added 3 commits November 3, 2026 00:31
…te checkouts

A Forgejo project whose origin is an SSH remote on a host other than the web
UI keys its own repository under the SSH host, so "Write review guide" says
the pull request is in another repository. This test reproduces #29753 and
fails until the fix lands.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t the web host

Forgejo's changeRequestHost ignored the resolved web URL for a non-HTTP
remote and keyed the project under the SSH host, so the review guide
refused Forgejo pull requests as being in another repository. Prefer the
web URL's host (port kept), as changeRequestUrl and the repository
matchers already do.

Fixes #29753

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Nov 3, 2026
@macroscopeapp

macroscopeapp Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 4204569

Macroscope's review found this PR approvable — Forgejo pull-request host matching now uses the resolved browser URL when SSH and web hosts differ, preserving ports and existing fallbacks. The change is limited to host identification and retains existing repository-boundary checks.

No code changes detected at 2ede7db. Prior analysis still applies.

You can add or adjust custom eligibility rules. Learn more.

@github-actions

github-actions Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 5.0 KiB 5.0 KiB 0 B (0.0%) 6.8 KiB ✅
Codex Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Codex Live turn WebSocket wire 1.2 KiB 1.2 KiB 0 B (0.0%) 2.0 KiB ✅
Codex Live turn WebSocket decoded 21.6 KiB 21.6 KiB 0 B (0.0%) 24.4 KiB ✅
Codex Live turn messages 2 2 0 (0.0%) 8 ✅
Claude Total thread wire 5.0 KiB 5.0 KiB 0 B (0.0%) 6.8 KiB ✅
Claude Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Claude Live turn WebSocket wire 1.2 KiB 1.2 KiB 0 B (0.0%) 2.0 KiB ✅
Claude Live turn WebSocket decoded 21.9 KiB 21.9 KiB 0 B (0.0%) 24.4 KiB ✅
Claude Live turn messages 2 2 0 (0.0%) 8 ✅

Baseline: f6503f0 · PR result: 2ede7db · Source CI: failure

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

Run configuration
Commits

Reviewing files that changed from the base of the PR and between 4204569 and 2ede7db.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review
Walkthrough

Forgejo host resolution now prefers the host and port from the project identity’s web URL. Tests cover host resolution for an SSH remote and review guide generation with a Forgejo identity.

Changes: Forgejo SSH and Web Host Identity

Layer / File(s) Summary
Resolve Forgejo host from web URL
packages/source-control-core/src/shared/definition.ts, packages/source-control-forgejo/src/shared/definition.ts, packages/source-control-forgejo/src/shared/definition.test.ts
ChangeRequestHostIdentity now accepts an optional webUrl. Forgejo uses the lowercased web URL host, including its port, before falling back to the HTTP(S) remote URL or project host. A test covers an SSH remote with a resolved web URL.
Validate review guide generation
apps/server/src/pullRequest/PullRequestReviewGuideService.test.ts
The test configures a Forgejo identity with an SSH-style canonical key and web URL. It checks that the generated guide uses the repository head SHA.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested labels: bugfix, size:S

Suggested reviewers: maria-rcks, t3dotgg

Merge Risk: Low · previous review 42045

The fix lets Forgejo checkouts with a different SSH host start review guides. Guides previously saved under the SSH host for hostless references may no longer be found and would need to be regenerated. The impact is narrow and recoverable, so the change is mergeable with that awareness.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly describes the main change: Forgejo review guides now work when the SSH remote host differs from the web host.
Description check Passed The description covers the problem, fix, scope, and focused verification in detail. The scope section links the issue but does not include an explicit maintainer approval comment or explain why the fi…
Linked Issues check Passed Issue #29753 requires review guides to work when a Forgejo checkout uses an SSH host that differs from the web host. Forgejo changeRequestHost now prefers the validated webUrl host, including its …
Out of Scope Changes check Passed All changes support issue #29753. The optional webUrl field enables Forgejo host resolution, the host-selection change fixes the reported mismatch, and both tests cover the fix. No unrelated changes…
Full details: Description check

Explanation

The description covers the problem, fix, scope, and focused verification in detail. The scope section links the issue but does not include an explicit maintainer approval comment or explain why the fix qualifies without one.

Full details: Linked Issues check

Explanation

Issue #29753 requires review guides to work when a Forgejo checkout uses an SSH host that differs from the web host. Forgejo changeRequestHost now prefers the validated webUrl host, including its port, and retains the HTTP-remote and canonical-key fallbacks. The PullRequestReviewGuideService.test.ts regression test verifies that guide generation succeeds and stores the repository head SHA. The Forgejo definition test verifies host and port selection from webUrl.

Full details: Out of Scope Changes check

Explanation

All changes support issue #29753. The optional webUrl field enables Forgejo host resolution, the host-selection change fixes the reported mismatch, and both tests cover the fix. No unrelated changes appear in the reviewed diff.

Finishing Touches
Generate docstrings
  • Commit to this branch
  • Create a new PR
Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between d8a829e and 4204569.

Files selected for processing: 4
  • apps/server/src/pullRequest/PullRequestReviewGuideService.test.ts
  • packages/source-control-core/src/shared/definition.ts
  • packages/source-control-forgejo/src/shared/definition.test.ts
  • packages/source-control-forgejo/src/shared/definition.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.

@coderabbitai coderabbitai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Fix comments: Fix CodeRabbit comments on this PR

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Review guide refuses Forgejo PRs when the project's SSH remote host differs from the web host

1 participant