3b5504c · 12f8720 · 06c727f
@@ -1049,10 +1049,6 @@
carries token material, a `state`, or a `code`.
- **Contributing needs a workbench.** `contribute` mirrors your fork (and, on
  the push-rights path, the upstream) if it is not mirrored already, so the
  first contribution to a large repository pays for a clone and an import.
- **A review's line comments do not travel with its verdict.** Submitting a
  review through the origin sends the state and the body; anchored comments
  are still posted one at a time. Both forges accept comments inside the
  review payload, which is the obvious next step.
- **The guard's window is a day.** An account that gains push rights to a
  protected repository keeps its credential until the next refresh or the
  next daily sweep, whichever is first.
@@ -78,6 +78,7 @@
import { repoDir } from "../git/repos.ts";
import { isStrangerPull, strangerGuard } from "../crew/stranger.ts";
import { importIssue, importReviewStates } from "../origins/import.ts";
import { err, ok, type Result } from "../kernel/result.ts";
import { createLog } from "../kernel/log.ts";
import {
  refusal,
  writeThroughFor,
@@ -97,6 +98,9 @@
import { carryBranchThrough } from "../origins/push-through.ts";
import { ensureFork, forkHead, pushToFork } from "../origins/fork-route.ts";
import { denied } from "../origins/operator.ts";
import type { WriteThrough } from "../origins/write-through.ts";
import { requestSyncNow } from "../origins/sync.ts";
const log = createLog("pulls");
/**
 * How a fork of a mirror proposes to the mirror's origin: `direct`, its
@@ -715,7 +719,7 @@
export async function submitReview(
  repo: Repo,
  pull: Pull,
  input: SubmitReviewInput,
): Promise<Result<Review, SubmitReviewActionError>> {
): Promise<Result<Review & { readonly note?: string }, SubmitReviewActionError>> {
  if (!isReviewState(input.state)) return err("invalid_state");
  const participant = requireParticipant(app, actor, repo);
  if (!participant.ok) return participant;
@@ -749,7 +753,16 @@
export async function submitReview(
    const sent = await through.value.provider.submitReview(
      through.value.fullName,
      pull.number,
      { state: input.state, body: (input.body ?? "").trim() },
      {
        state: input.state,
        body: (input.body ?? "").trim(),
        comments: anchored.value.map((l) => ({
          path: l.path,
          line: l.line,
          side: l.side,
          body: l.body,
        })),
      },
    );
    if (!sent.ok) return err(refusal(sent.error.error));
    importReviewStates(
@@ -769,8 +782,26 @@
export async function submitReview(
        ...(stored ? { reviewId: stored.id } : {}),
      },
    });
      
        sentinelcrew
        17h ago
        outdated
      
      Untested branch: carried vs refused decides whether lines are posted locally and whether a sync is requested. Add an action-level test that fails if this condition is inverted or removed.
    
    // Line comments stay here, as `addReview`'s always have on a mirror.
    for (const line of anchored.value) postLine(app, pull, user, line);
    // The origin took the lines inside the review, and sync brings them
    // back as its own; posting them here too would show each twice. One that
    // refused the payload got the verdict alone, so the lines stay here, as
    // `addReview`'s always have on a mirror, and the log says so.
    if (anchored.value.length > 0 && (sent.value.carried ?? 0) === 0) {
      log.warn("origin refused the review's line comments; kept here one by one", {
        repo: repo.id,
        number: pull.number,
        lines: anchored.value.length,
      });
      for (const line of anchored.value) postLine(app, pull, user, line);
      return stored
        ? ok({
            ...stored,
            note: "The origin refused line comments inside a review: the verdict went there alone and the line comments are kept here.",
          })
        : err("origin_refused");
    } else if (anchored.value.length > 0) {
      requestSyncNow(app, through.value.origin.id);
    }
    return stored ? ok(stored) : err("origin_refused");
  }
@@ -45,6 +45,7 @@
import type {
  ArchivedPage,
  ArchivedRepo,
  RemoteReviewState,
  ReviewLine,
  RemoteTreeEntry,
  RepoWalk,
} from "../types.ts";
@@ -76,6 +77,7 @@
import {
  labelNames,
  linkNext,
  pathSegments,
  postReview,
  readmeEntry,
  seg,
  sortEntries,
@@ -706,6 +708,7 @@
export class ForgejoProvider implements Provider {
    input: {
      state: "approved" | "changes_requested" | "commented";
      body: string;
      comments?: readonly ReviewLine[];
    },
  ): Promise<ProviderResult<RemoteReviewState>> {
    const named = splitFullName(fullName);
@@ -718,16 +721,24 @@
export class ForgejoProvider implements Provider {
        : input.state === "changes_requested"
          ? "REQUEST_CHANGES"
          : "COMMENT";
    const result = await this.api.call({
      path: `/repos/${seg(named.owner)}/${seg(named.name)}/pulls/${number}/reviews`,
      method: "POST",
      body: { event, body: input.body },
    });
    const lines = (input.comments ?? []).map((l) => ({
      path: l.path,
      body: l.body,
      ...(l.side === "old"
        ? { old_position: l.line }
        : { new_position: l.line }),
    }));
    const result = await postReview(
      this.api,
      `/repos/${seg(named.owner)}/${seg(named.name)}/pulls/${number}/reviews`,
      { event, body: input.body },
      lines,
    );
    if (!result.ok) return result;
    const state = toReviewState(result.value.data);
    return state === undefined
      ? fail("invalid_response", NO_REVIEW)
      : succeed(state);
      : succeed({ ...state, carried: result.value.carried });
  }
  async createIssue(
@@ -29,6 +29,7 @@
import type {
  RemoteReview,
  RemoteMilestone,
  RemoteReviewState,
  ReviewLine,
  RemoteTreeEntry,
  TokenReach,
  RepoWalk,
@@ -62,6 +63,7 @@
import {
  labelNames,
  linkNext,
  pathSegments,
  postReview,
  seg,
  sortEntries,
  splitFullName,
@@ -611,6 +613,7 @@
export class GitHubProvider implements Provider {
    input: {
      state: "approved" | "changes_requested" | "commented";
      body: string;
      comments?: readonly ReviewLine[];
    },
  ): Promise<ProviderResult<RemoteReviewState>> {
    const named = splitFullName(fullName);
@@ -623,16 +626,23 @@
export class GitHubProvider implements Provider {
        : input.state === "changes_requested"
          ? "REQUEST_CHANGES"
          : "COMMENT";
    const result = await this.api.call({
      path: `/repos/${seg(named.owner)}/${seg(named.name)}/pulls/${number}/reviews`,
      method: "POST",
      body: { event, body: input.body },
    });
    const lines = (input.comments ?? []).map((l) => ({
      path: l.path,
      line: l.line,
      side: l.side === "old" ? "LEFT" : "RIGHT",
      body: l.body,
    }));
    const result = await postReview(
      this.api,
      `/repos/${seg(named.owner)}/${seg(named.name)}/pulls/${number}/reviews`,
      { event, body: input.body },
      lines,
    );
    if (!result.ok) return result;
    const state = toReviewState(result.value.data);
    return state === undefined
      ? fail("invalid_response", NO_REVIEW)
      : succeed(state);
      : succeed({ ...state, carried: result.value.carried });
  }
  async createIssue(
@@ -788,3 +788,29 @@
export function readmeEntry(
  }
  return files.find((e) => e.name.toLowerCase().startsWith("readme."));
}
/**
 * Post a review with its line comments inside it, in one call. An origin
 * that refuses the payload (a 4xx that is not about who we are, nor about
 * the pull's existence) is asked again with the verdict alone, so a shape
 * it dislikes never costs the verdict: `carried` says which one it took.
 */
export async function postReview(
  api: ApiClient,
  path: string,
  review: { event: string; body: string },
  lines: readonly Record<string, unknown>[],
): Promise<ProviderResult<{ data: unknown; carried: number }>> {
  if (lines.length > 0) {
    const whole = await api.call({
      path,
      method: "POST",
      body: { ...review, comments: lines },
    });
    if (whole.ok) return succeed({ data: whole.value.data, carried: lines.length });
    if (whole.error.error !== "invalid_response") return whole;
  }
  const bare = await api.call({ path, method: "POST", body: review });
  if (!bare.ok) return bare;
  return succeed({ data: bare.value.data, carried: 0 });
}
@@ -174,6 +174,16 @@
export interface RemoteReviewState {
  readonly commitOid: string;
  readonly createdAt: string;
  readonly htmlUrl: string;
  /** How many line comments the origin took inside this review (`submitReview` only). */
  readonly carried?: number;
}
/** A line comment bound for an origin, inside the review that carries it. */
export interface ReviewLine {
  readonly path: string;
  readonly line: number;
  readonly side: "old" | "new";
  readonly body: string;
}
/** How a pull's check stands on its origin, for the head named. */
@@ -511,6 +521,8 @@
export interface Provider {
    input: {
      state: "approved" | "changes_requested" | "commented";
      body: string;
      /** Sent inside the review; an origin that refuses them gets the verdict alone. */
      comments?: readonly ReviewLine[];
    },
  ): Promise<ProviderResult<RemoteReviewState>>;
  /**
@@ -203,6 +203,8 @@
export interface FakeState {
  conditionalHits: number;
  /** Refuse the next write with this message. */
  refuseWrite: string;
  /** Refuse (422) a review posted with `comments` inside it; a bare verdict goes through. */
  refuseReviewComments?: boolean;
  /** Forgejo webhooks as created, keyed `owner/name`. */
  hooks: Map<string, Record<string, unknown>[]>;
  /**
@@ -904,6 +906,8 @@
function route(
    if (suffix === "/reviews" && method === "POST") {
      if (state.refuseWrite !== "") return refuse(state);
      const input = parse(bodyText);
      if (state.refuseReviewComments && Array.isArray(input.comments))
        return jsonResponse({ message: "comments: unexpected" }, 422);
      const created: FakeReviewState = {
        id: state.nextId++,
        issueNumber: number,
@@ -0,0 +1,123 @@
/**
 * Submitting a review with line comments on a mirrored pull (#11479): when
 * the origin carries the lines inside the review they are not also posted
 * here, and a sync is asked for; when it refuses them they are posted here,
 * once, and the reviewer is told.
 */
import { afterAll, beforeAll, describe, expect, setDefaultTimeout, test } from "bun:test";
import { mkdtemp, rm } from "node:fs/promises";
import { tmpdir } from "node:os";
import { join } from "node:path";
import { createApp, type App } from "../src/app.ts";
import { submitReview } from "../src/actions/pulls.ts";
import { getCursor } from "../src/core/origins.ts";
import { listPulls } from "../src/core/pulls.ts";
import { getRepo } from "../src/core/repos.ts";
import { getUserByName } from "../src/core/users.ts";
import { serve, type Servers } from "../src/http/server.ts";
import { syncsSettled } from "../src/origins/sync.ts";
import { mirrorAndWait, newFakeState, serveFakeForge, type FakeForge } from "./fake-origin.ts";
import { seedAdmin } from "./helpers/admin.ts";
import { quietLogs } from "./helpers/globals.ts";
import { proveAll } from "./helpers/verified.ts";
quietLogs();
setDefaultTimeout(60_000);
const LOGIN = "octo";
const TOKEN = "octo-review-lines-secret-3a9e";
const PASSWORD = "alice-login-secret";
const basic = (u: string, p: string) => `Basic ${Buffer.from(`${u}:${p}`).toString("base64")}`;
describe("a review's lines on a mirrored pull", () => {
  const dirs: string[] = [];
  let app: App;
  let servers: Servers;
  let originApp: App;
  let originServers: Servers;
  let fake: FakeForge;
  let originId = "";
  const api = (path: string, init?: RequestInit) => {
    const headers = new Headers(init?.headers);
    headers.set("content-type", "application/json");
    headers.set("authorization", basic("alice", PASSWORD));
    return fetch(`${servers.url}${path}`, { ...init, headers });
  };
  const repo = () => getRepo(app.db, "alice", "vault")!;
  const pull = () => listPulls(app.db, repo().id)[0]!;
  const posts = () => fake.state.requests.filter((r) => r.method === "POST" && r.path.endsWith("/pulls/1/reviews"));
  const localLines = () =>
    app.db.query<{ n: number }, [string]>("SELECT COUNT(*) AS n FROM comment WHERE issue_id = ?1 AND path IS NOT NULL").get(pull().id)!.n;
  const nextSync = () => getCursor(app.db, originId, "next_sync_at");
  const lines = [{ path: "b.txt", line: 1, side: "new" as const, body: "why this?" }];
  beforeAll(async () => {
    for (const p of ["forge-rl-", "forge-rl-up-"]) dirs.push(await mkdtemp(join(tmpdir(), p)));
    originApp = createApp({ dataDir: dirs[1]!, port: 0 });
    originServers = await serve(originApp);
    await seedAdmin(originApp, { username: LOGIN, email: "[email protected]", password: TOKEN });
    const oh = { "content-type": "application/json", authorization: basic(LOGIN, TOKEN) };
    await fetch(`${originServers.url}/api/v1/repos`, { method: "POST", headers: oh, body: JSON.stringify({ name: "vault", visibility: "public" }) });
    await fetch(`${originServers.url}/api/v1/repos/octo/vault/contents`, {
      method: "POST", headers: oh,
      body: JSON.stringify({ branch: "main", message: "first", files: [{ path: "a.txt", content: "one\n" }] }),
    });
    const made = await fetch(`${originServers.url}/api/v1/repos/octo/vault/contents`, {
      method: "POST", headers: oh,
      body: JSON.stringify({ branch: "feature", message: "more", files: [{ path: "b.txt", content: "two\n" }] }),
    });
    const { commitOid } = (await made.json()) as { commitOid: string };
    const state = newFakeState("github", LOGIN, TOKEN);
    state.repos.push({ id: 9, fullName: "octo/vault", cloneUrl: `${originServers.url}/octo/vault.git`, defaultBranch: "main", htmlUrl: `${originServers.url}/octo/vault`, push: true });
    state.issues.set("octo/vault", [
      { id: 5, number: 1, title: "more", body: "", state: "open", user: LOGIN, labels: [], createdAt: "2026-01-01T00:00:00Z", updatedAt: "2026-01-01T00:00:00Z", pull: { base: "main", head: "feature", headSha: commitOid } },
    ] as never);
    fake = serveFakeForge(state);
    app = createApp({ dataDir: dirs[0]!, port: 0 });
    servers = await serve(app);
    const admin = await seedAdmin(app, { username: "alice", email: "[email protected]", password: PASSWORD });
    if (!admin.ok) throw new Error(admin.error);
    const connected = (await (await api("/api/v1/user/origins", { method: "POST", body: JSON.stringify({ host: fake.url, kind: "github", token: TOKEN }) })).json()) as { id: string };
    originId = connected.id;
    await mirrorAndWait(await api(`/api/v1/user/origins/${originId}/mirror`, { method: "POST", body: JSON.stringify({ fullName: "octo/vault" }) }));
    await syncsSettled();
    proveAll(app);
  });
  afterAll(async () => {
    await syncsSettled();
    fake.stop();
    servers.stop();
    originServers.stop();
    app.close();
    originApp.close();
    for (const d of dirs) await rm(d, { recursive: true, force: true });
  });
  const send = () =>
    submitReview(app, getUserByName(app.db, "alice")!, repo(), pull(), { state: "changes_requested", body: "see lines", comments: lines });
  test("carried by the origin: one request, no local duplicate, a sync asked for", async () => {
    expect(pull().number).toBe(1);
    app.db.query("DELETE FROM origin_cursor WHERE origin_id = ?1 AND key = 'next_sync_at'").run(originId);
    const sent = await send();
    expect(sent.ok).toBe(true);
    expect(posts()).toHaveLength(1);
    expect(localLines()).toBe(0);
    expect(nextSync()).toBe(new Date(0).toISOString());
    if (sent.ok) expect(sent.value.note).toBeUndefined();
  });
  test("refused by the origin: the lines are posted here once, and the reviewer is told", async () => {
    fake.state.refuseReviewComments = true;
    const before = posts().length;
    const sent = await send();
    expect(sent.ok).toBe(true);
    expect(posts().length - before).toBe(2);
    expect(localLines()).toBe(1);
    if (sent.ok) expect(sent.value.note).toContain("kept here");
  });
});
@@ -0,0 +1,80 @@
/**
 * A review's line comments travel inside the review (#11479): one request
 * carries the verdict and the comments, and an origin that refuses that
 * payload still gets the verdict, with the comments posted the old way.
 */
import { afterEach, beforeEach, describe, expect, test } from "bun:test";
import { ForgejoProvider } from "../src/origins/providers/forgejo.ts";
import { GitHubProvider } from "../src/origins/providers/github.ts";
import { staticCredential, type ReviewLine } from "../src/origins/types.ts";
import { newFakeState, serveFakeForge, type FakeForge, type FakeState } from "./fake-origin.ts";
import { quietLogs } from "./helpers/globals.ts";
quietLogs();
const TOKEN = "tok-review-lines-1";
const lines: ReviewLine[] = [
  { path: "a.ts", line: 3, side: "new", body: "one" },
  { path: "a.ts", line: 9, side: "old", body: "two" },
  { path: "b.ts", line: 1, side: "new", body: "three" },
];
for (const kind of ["forgejo", "github"] as const) {
  describe(`${kind}: line comments inside the review`, () => {
    let state: FakeState;
    let fake: FakeForge;
    beforeEach(() => {
      state = newFakeState(kind, "trevato", TOKEN);
      state.repos.push({ id: 1, fullName: "o/r", cloneUrl: "http://127.0.0.1/none.git", admin: true });
      state.issues.set("o/r", [
        { id: 5, number: 1, title: "p", body: "", state: "open", user: "x", labels: [], createdAt: "2026-01-01T00:00:00Z", updatedAt: "2026-01-01T00:00:00Z", pull: { headSha: "a".repeat(40), headRef: "h", baseRef: "main", merged: false } },
      ] as never);
      fake = serveFakeForge(state);
    });
    afterEach(() => fake.stop());
    const provider = () => {
      const credential = staticCredential({ kind, apiUrl: fake.apiUrl, webUrl: fake.url, token: TOKEN });
      return kind === "forgejo" ? new ForgejoProvider(credential) : new GitHubProvider(credential);
    };
    const posts = () => state.requests.filter((r) => r.method === "POST" && r.path.endsWith("/pulls/1/reviews"));
    test("one request carries the verdict and the three comments", async () => {
      const sent = await provider().submitReview("o/r", 1, { state: "changes_requested", body: "see lines", comments: lines });
      expect(sent.ok).toBe(true);
      if (sent.ok) expect(sent.value.carried).toBe(3);
      expect(posts()).toHaveLength(1);
      const body = JSON.parse(posts()[0]!.body);
      expect(body.event).toBe("REQUEST_CHANGES");
      expect(body.comments).toHaveLength(3);
      expect(body.comments[0]).toMatchObject(
        kind === "forgejo" ? { path: "a.ts", new_position: 3, body: "one" } : { path: "a.ts", line: 3, side: "RIGHT", body: "one" },
      );
      expect(body.comments[1]).toMatchObject(kind === "forgejo" ? { old_position: 9 } : { line: 9, side: "LEFT" });
    });
    test("a review without lines sends no comments key", async () => {
      await provider().submitReview("o/r", 1, { state: "approved", body: "" });
      expect(posts()).toHaveLength(1);
      expect("comments" in JSON.parse(posts()[0]!.body)).toBe(false);
    });
    test("an origin that refuses the payload still gets the verdict, alone", async () => {
      state.refuseReviewComments = true;
      const sent = await provider().submitReview("o/r", 1, { state: "changes_requested", body: "ok", comments: lines });
      expect(sent.ok).toBe(true);
      if (sent.ok) expect(sent.value.carried).toBe(0);
      const bodies = posts().map((r) => JSON.parse(r.body));
      expect(bodies).toHaveLength(2);
      expect("comments" in bodies[1]).toBe(false);
      expect(bodies[1].body).toBe("ok");
    });
    test("a refusal about who we are is not retried", async () => {
      state.token = "other";
      const sent = await provider().submitReview("o/r", 1, { state: "approved", body: "", comments: lines });
      expect(sent.ok).toBe(false);
      expect(posts()).toHaveLength(1);
    });
  });
}
		@@ -1049,10 +1049,6 @@ carries token material, a `state`, or a `code`.
1049	1049	- Contributing needs a workbench. `contribute` mirrors your fork (and, on
1050	1050	the push-rights path, the upstream) if it is not mirrored already, so the
1051	1051	first contribution to a large repository pays for a clone and an import.
1052		- A review's line comments do not travel with its verdict. Submitting a
1053		review through the origin sends the state and the body; anchored comments
1054		are still posted one at a time. Both forges accept comments inside the
1055		review payload, which is the obvious next step.
1056	1052	- The guard's window is a day. An account that gains push rights to a
1057	1053	protected repository keeps its credential until the next refresh or the
1058	1054	next daily sweep, whichever is first.
		@@ -78,6 +78,7 @@ import { repoDir } from "../git/repos.ts";
78	78	import { isStrangerPull, strangerGuard } from "../crew/stranger.ts";
79	79	import { importIssue, importReviewStates } from "../origins/import.ts";
80	80	import { err, ok, type Result } from "../kernel/result.ts";
	81	import { createLog } from "../kernel/log.ts";
81	82	import {
82	83	refusal,
83	84	writeThroughFor,
		@@ -97,6 +98,9 @@ import { carryBranchThrough } from "../origins/push-through.ts";
97	98	import { ensureFork, forkHead, pushToFork } from "../origins/fork-route.ts";
98	99	import { denied } from "../origins/operator.ts";
99	100	import type { WriteThrough } from "../origins/write-through.ts";
	101	import { requestSyncNow } from "../origins/sync.ts";
	102
	103	const log = createLog("pulls");
100	104
101	105	/**
102	106	* How a fork of a mirror proposes to the mirror's origin: `direct`, its
		@@ -715,7 +719,7 @@ export async function submitReview(
715	719	repo: Repo,
716	720	pull: Pull,
717	721	input: SubmitReviewInput,
718		): Promise<Result<Review, SubmitReviewActionError>> {
	722	): Promise<Result<Review & { readonly note?: string }, SubmitReviewActionError>> {
719	723	if (!isReviewState(input.state)) return err("invalid_state");
720	724	const participant = requireParticipant(app, actor, repo);
721	725	if (!participant.ok) return participant;
		@@ -749,7 +753,16 @@ export async function submitReview(
749	753	const sent = await through.value.provider.submitReview(
750	754	through.value.fullName,
751	755	pull.number,
752		{ state: input.state, body: (input.body ?? "").trim() },
	756	{
	757	state: input.state,
	758	body: (input.body ?? "").trim(),
	759	comments: anchored.value.map((l) => ({
	760	path: l.path,
	761	line: l.line,
	762	side: l.side,
	763	body: l.body,
	764	})),
	765	},
753	766	);
754	767	if (!sent.ok) return err(refusal(sent.error.error));
755	768	importReviewStates(
		@@ -769,8 +782,26 @@ export async function submitReview(
769	782	...(stored ? { reviewId: stored.id } : {}),
770	783	},
771	784	});
		sentinelcrew 17h ago outdated Untested branch: carried vs refused decides whether lines are posted locally and whether a sync is requested. Add an action-level test that fails if this condition is inverted or removed.
772		// Line comments stay here, as `addReview`'s always have on a mirror.
773		for (const line of anchored.value) postLine(app, pull, user, line);
	785	// The origin took the lines inside the review, and sync brings them
	786	// back as its own; posting them here too would show each twice. One that
	787	// refused the payload got the verdict alone, so the lines stay here, as
	788	// `addReview`'s always have on a mirror, and the log says so.
	789	if (anchored.value.length > 0 && (sent.value.carried ?? 0) === 0) {
	790	log.warn("origin refused the review's line comments; kept here one by one", {
	791	repo: repo.id,
	792	number: pull.number,
	793	lines: anchored.value.length,
	794	});
	795	for (const line of anchored.value) postLine(app, pull, user, line);
	796	return stored
	797	? ok({
	798	...stored,
	799	note: "The origin refused line comments inside a review: the verdict went there alone and the line comments are kept here.",
	800	})
	801	: err("origin_refused");
	802	} else if (anchored.value.length > 0) {
	803	requestSyncNow(app, through.value.origin.id);
	804	}
774	805	return stored ? ok(stored) : err("origin_refused");
775	806	}
776	807
		@@ -45,6 +45,7 @@ import type {
45	45	ArchivedPage,
46	46	ArchivedRepo,
47	47	RemoteReviewState,
	48	ReviewLine,
48	49	RemoteTreeEntry,
49	50	RepoWalk,
50	51	} from "../types.ts";
		@@ -76,6 +77,7 @@ import {
76	77	labelNames,
77	78	linkNext,
78	79	pathSegments,
	80	postReview,
79	81	readmeEntry,
80	82	seg,
81	83	sortEntries,
		@@ -706,6 +708,7 @@ export class ForgejoProvider implements Provider {
706	708	input: {
707	709	state: "approved" \| "changes_requested" \| "commented";
708	710	body: string;
	711	comments?: readonly ReviewLine[];
709	712	},
710	713	): Promise<ProviderResult<RemoteReviewState>> {
711	714	const named = splitFullName(fullName);
		@@ -718,16 +721,24 @@ export class ForgejoProvider implements Provider {
718	721	: input.state === "changes_requested"
719	722	? "REQUEST_CHANGES"
720	723	: "COMMENT";
721		const result = await this.api.call({
722		path: `/repos/${seg(named.owner)}/${seg(named.name)}/pulls/${number}/reviews`,
723		method: "POST",
724		body: { event, body: input.body },
725		});
	724	const lines = (input.comments ?? []).map((l) => ({
	725	path: l.path,
	726	body: l.body,
	727	...(l.side === "old"
	728	? { old_position: l.line }
	729	: { new_position: l.line }),
	730	}));
	731	const result = await postReview(
	732	this.api,
	733	`/repos/${seg(named.owner)}/${seg(named.name)}/pulls/${number}/reviews`,
	734	{ event, body: input.body },
	735	lines,
	736	);
726	737	if (!result.ok) return result;
727	738	const state = toReviewState(result.value.data);
728	739	return state === undefined
729	740	? fail("invalid_response", NO_REVIEW)
730		: succeed(state);
	741	: succeed({ ...state, carried: result.value.carried });
731	742	}
732	743
733	744	async createIssue(
		@@ -29,6 +29,7 @@ import type {
29	29	RemoteReview,
30	30	RemoteMilestone,
31	31	RemoteReviewState,
	32	ReviewLine,
32	33	RemoteTreeEntry,
33	34	TokenReach,
34	35	RepoWalk,
		@@ -62,6 +63,7 @@ import {
62	63	labelNames,
63	64	linkNext,
64	65	pathSegments,
	66	postReview,
65	67	seg,
66	68	sortEntries,
67	69	splitFullName,
		@@ -611,6 +613,7 @@ export class GitHubProvider implements Provider {
611	613	input: {
612	614	state: "approved" \| "changes_requested" \| "commented";
613	615	body: string;
	616	comments?: readonly ReviewLine[];
614	617	},
615	618	): Promise<ProviderResult<RemoteReviewState>> {
616	619	const named = splitFullName(fullName);
		@@ -623,16 +626,23 @@ export class GitHubProvider implements Provider {
623	626	: input.state === "changes_requested"
624	627	? "REQUEST_CHANGES"
625	628	: "COMMENT";
626		const result = await this.api.call({
627		path: `/repos/${seg(named.owner)}/${seg(named.name)}/pulls/${number}/reviews`,
628		method: "POST",
629		body: { event, body: input.body },
630		});
	629	const lines = (input.comments ?? []).map((l) => ({
	630	path: l.path,
	631	line: l.line,
	632	side: l.side === "old" ? "LEFT" : "RIGHT",
	633	body: l.body,
	634	}));
	635	const result = await postReview(
	636	this.api,
	637	`/repos/${seg(named.owner)}/${seg(named.name)}/pulls/${number}/reviews`,
	638	{ event, body: input.body },
	639	lines,
	640	);
631	641	if (!result.ok) return result;
632	642	const state = toReviewState(result.value.data);
633	643	return state === undefined
634	644	? fail("invalid_response", NO_REVIEW)
635		: succeed(state);
	645	: succeed({ ...state, carried: result.value.carried });
636	646	}
637	647
638	648	async createIssue(
		@@ -788,3 +788,29 @@ export function readmeEntry(
788	788	}
789	789	return files.find((e) => e.name.toLowerCase().startsWith("readme."));
790	790	}
	791
	792	/**
	793	* Post a review with its line comments inside it, in one call. An origin
	794	* that refuses the payload (a 4xx that is not about who we are, nor about
	795	* the pull's existence) is asked again with the verdict alone, so a shape
	796	* it dislikes never costs the verdict: `carried` says which one it took.
	797	*/
	798	export async function postReview(
	799	api: ApiClient,
	800	path: string,
	801	review: { event: string; body: string },
	802	lines: readonly Record<string, unknown>[],
	803	): Promise<ProviderResult<{ data: unknown; carried: number }>> {
	804	if (lines.length > 0) {
	805	const whole = await api.call({
	806	path,
	807	method: "POST",
	808	body: { ...review, comments: lines },
	809	});
	810	if (whole.ok) return succeed({ data: whole.value.data, carried: lines.length });
	811	if (whole.error.error !== "invalid_response") return whole;
	812	}
	813	const bare = await api.call({ path, method: "POST", body: review });
	814	if (!bare.ok) return bare;
	815	return succeed({ data: bare.value.data, carried: 0 });
	816	}
		@@ -174,6 +174,16 @@ export interface RemoteReviewState {
174	174	readonly commitOid: string;
175	175	readonly createdAt: string;
176	176	readonly htmlUrl: string;
	177	/** How many line comments the origin took inside this review (`submitReview` only). */
	178	readonly carried?: number;
	179	}
	180
	181	/** A line comment bound for an origin, inside the review that carries it. */
	182	export interface ReviewLine {
	183	readonly path: string;
	184	readonly line: number;
	185	readonly side: "old" \| "new";
	186	readonly body: string;
177	187	}
178	188
179	189	/** How a pull's check stands on its origin, for the head named. */
		@@ -511,6 +521,8 @@ export interface Provider {
511	521	input: {
512	522	state: "approved" \| "changes_requested" \| "commented";
513	523	body: string;
	524	/** Sent inside the review; an origin that refuses them gets the verdict alone. */
	525	comments?: readonly ReviewLine[];
514	526	},
515	527	): Promise<ProviderResult<RemoteReviewState>>;
516	528	/**
		@@ -203,6 +203,8 @@ export interface FakeState {
203	203	conditionalHits: number;
204	204	/** Refuse the next write with this message. */
205	205	refuseWrite: string;
	206	/** Refuse (422) a review posted with `comments` inside it; a bare verdict goes through. */
	207	refuseReviewComments?: boolean;
206	208	/** Forgejo webhooks as created, keyed `owner/name`. */
207	209	hooks: Map<string, Record<string, unknown>[]>;
208	210	/**
		@@ -904,6 +906,8 @@ function route(
904	906	if (suffix === "/reviews" && method === "POST") {
905	907	if (state.refuseWrite !== "") return refuse(state);
906	908	const input = parse(bodyText);
	909	if (state.refuseReviewComments && Array.isArray(input.comments))
	910	return jsonResponse({ message: "comments: unexpected" }, 422);
907	911	const created: FakeReviewState = {
908	912	id: state.nextId++,
909	913	issueNumber: number,
		@@ -0,0 +1,123 @@
	1	/**
	2	* Submitting a review with line comments on a mirrored pull (#11479): when
	3	* the origin carries the lines inside the review they are not also posted
	4	* here, and a sync is asked for; when it refuses them they are posted here,
	5	* once, and the reviewer is told.
	6	*/
	7	import { afterAll, beforeAll, describe, expect, setDefaultTimeout, test } from "bun:test";
	8	import { mkdtemp, rm } from "node:fs/promises";
	9	import { tmpdir } from "node:os";
	10	import { join } from "node:path";
	11	import { createApp, type App } from "../src/app.ts";
	12	import { submitReview } from "../src/actions/pulls.ts";
	13	import { getCursor } from "../src/core/origins.ts";
	14	import { listPulls } from "../src/core/pulls.ts";
	15	import { getRepo } from "../src/core/repos.ts";
	16	import { getUserByName } from "../src/core/users.ts";
	17	import { serve, type Servers } from "../src/http/server.ts";
	18	import { syncsSettled } from "../src/origins/sync.ts";
	19	import { mirrorAndWait, newFakeState, serveFakeForge, type FakeForge } from "./fake-origin.ts";
	20	import { seedAdmin } from "./helpers/admin.ts";
	21	import { quietLogs } from "./helpers/globals.ts";
	22	import { proveAll } from "./helpers/verified.ts";
	23
	24	quietLogs();
	25	setDefaultTimeout(60_000);
	26
	27	const LOGIN = "octo";
	28	const TOKEN = "octo-review-lines-secret-3a9e";
	29	const PASSWORD = "alice-login-secret";
	30	const basic = (u: string, p: string) => `Basic ${Buffer.from(`${u}:${p}`).toString("base64")}`;
	31
	32	describe("a review's lines on a mirrored pull", () => {
	33	const dirs: string[] = [];
	34	let app: App;
	35	let servers: Servers;
	36	let originApp: App;
	37	let originServers: Servers;
	38	let fake: FakeForge;
	39	let originId = "";
	40
	41	const api = (path: string, init?: RequestInit) => {
	42	const headers = new Headers(init?.headers);
	43	headers.set("content-type", "application/json");
	44	headers.set("authorization", basic("alice", PASSWORD));
	45	return fetch(`${servers.url}${path}`, { ...init, headers });
	46	};
	47	const repo = () => getRepo(app.db, "alice", "vault")!;
	48	const pull = () => listPulls(app.db, repo().id)[0]!;
	49	const posts = () => fake.state.requests.filter((r) => r.method === "POST" && r.path.endsWith("/pulls/1/reviews"));
	50	const localLines = () =>
	51	app.db.query<{ n: number }, [string]>("SELECT COUNT(*) AS n FROM comment WHERE issue_id = ?1 AND path IS NOT NULL").get(pull().id)!.n;
	52	const nextSync = () => getCursor(app.db, originId, "next_sync_at");
	53	const lines = [{ path: "b.txt", line: 1, side: "new" as const, body: "why this?" }];
	54
	55	beforeAll(async () => {
	56	for (const p of ["forge-rl-", "forge-rl-up-"]) dirs.push(await mkdtemp(join(tmpdir(), p)));
	57	originApp = createApp({ dataDir: dirs[1]!, port: 0 });
	58	originServers = await serve(originApp);
	59	await seedAdmin(originApp, { username: LOGIN, email: "[email protected]", password: TOKEN });
	60	const oh = { "content-type": "application/json", authorization: basic(LOGIN, TOKEN) };
	61	await fetch(`${originServers.url}/api/v1/repos`, { method: "POST", headers: oh, body: JSON.stringify({ name: "vault", visibility: "public" }) });
	62	await fetch(`${originServers.url}/api/v1/repos/octo/vault/contents`, {
	63	method: "POST", headers: oh,
	64	body: JSON.stringify({ branch: "main", message: "first", files: [{ path: "a.txt", content: "one\n" }] }),
	65	});
	66	const made = await fetch(`${originServers.url}/api/v1/repos/octo/vault/contents`, {
	67	method: "POST", headers: oh,
	68	body: JSON.stringify({ branch: "feature", message: "more", files: [{ path: "b.txt", content: "two\n" }] }),
	69	});
	70	const { commitOid } = (await made.json()) as { commitOid: string };
	71
	72	const state = newFakeState("github", LOGIN, TOKEN);
	73	state.repos.push({ id: 9, fullName: "octo/vault", cloneUrl: `${originServers.url}/octo/vault.git`, defaultBranch: "main", htmlUrl: `${originServers.url}/octo/vault`, push: true });
	74	state.issues.set("octo/vault", [
	75	{ id: 5, number: 1, title: "more", body: "", state: "open", user: LOGIN, labels: [], createdAt: "2026-01-01T00:00:00Z", updatedAt: "2026-01-01T00:00:00Z", pull: { base: "main", head: "feature", headSha: commitOid } },
	76	] as never);
	77	fake = serveFakeForge(state);
	78
	79	app = createApp({ dataDir: dirs[0]!, port: 0 });
	80	servers = await serve(app);
	81	const admin = await seedAdmin(app, { username: "alice", email: "[email protected]", password: PASSWORD });
	82	if (!admin.ok) throw new Error(admin.error);
	83	const connected = (await (await api("/api/v1/user/origins", { method: "POST", body: JSON.stringify({ host: fake.url, kind: "github", token: TOKEN }) })).json()) as { id: string };
	84	originId = connected.id;
	85	await mirrorAndWait(await api(`/api/v1/user/origins/${originId}/mirror`, { method: "POST", body: JSON.stringify({ fullName: "octo/vault" }) }));
	86	await syncsSettled();
	87	proveAll(app);
	88	});
	89
	90	afterAll(async () => {
	91	await syncsSettled();
	92	fake.stop();
	93	servers.stop();
	94	originServers.stop();
	95	app.close();
	96	originApp.close();
	97	for (const d of dirs) await rm(d, { recursive: true, force: true });
	98	});
	99
	100	const send = () =>
	101	submitReview(app, getUserByName(app.db, "alice")!, repo(), pull(), { state: "changes_requested", body: "see lines", comments: lines });
	102
	103	test("carried by the origin: one request, no local duplicate, a sync asked for", async () => {
	104	expect(pull().number).toBe(1);
	105	app.db.query("DELETE FROM origin_cursor WHERE origin_id = ?1 AND key = 'next_sync_at'").run(originId);
	106	const sent = await send();
	107	expect(sent.ok).toBe(true);
	108	expect(posts()).toHaveLength(1);
	109	expect(localLines()).toBe(0);
	110	expect(nextSync()).toBe(new Date(0).toISOString());
	111	if (sent.ok) expect(sent.value.note).toBeUndefined();
	112	});
	113
	114	test("refused by the origin: the lines are posted here once, and the reviewer is told", async () => {
	115	fake.state.refuseReviewComments = true;
	116	const before = posts().length;
	117	const sent = await send();
	118	expect(sent.ok).toBe(true);
	119	expect(posts().length - before).toBe(2);
	120	expect(localLines()).toBe(1);
	121	if (sent.ok) expect(sent.value.note).toContain("kept here");
	122	});
	123	});
		@@ -0,0 +1,80 @@
	1	/**
	2	* A review's line comments travel inside the review (#11479): one request
	3	* carries the verdict and the comments, and an origin that refuses that
	4	* payload still gets the verdict, with the comments posted the old way.
	5	*/
	6
	7	import { afterEach, beforeEach, describe, expect, test } from "bun:test";
	8	import { ForgejoProvider } from "../src/origins/providers/forgejo.ts";
	9	import { GitHubProvider } from "../src/origins/providers/github.ts";
	10	import { staticCredential, type ReviewLine } from "../src/origins/types.ts";
	11	import { newFakeState, serveFakeForge, type FakeForge, type FakeState } from "./fake-origin.ts";
	12	import { quietLogs } from "./helpers/globals.ts";
	13
	14	quietLogs();
	15
	16	const TOKEN = "tok-review-lines-1";
	17	const lines: ReviewLine[] = [
	18	{ path: "a.ts", line: 3, side: "new", body: "one" },
	19	{ path: "a.ts", line: 9, side: "old", body: "two" },
	20	{ path: "b.ts", line: 1, side: "new", body: "three" },
	21	];
	22
	23	for (const kind of ["forgejo", "github"] as const) {
	24	describe(`${kind}: line comments inside the review`, () => {
	25	let state: FakeState;
	26	let fake: FakeForge;
	27	beforeEach(() => {
	28	state = newFakeState(kind, "trevato", TOKEN);
	29	state.repos.push({ id: 1, fullName: "o/r", cloneUrl: "http://127.0.0.1/none.git", admin: true });
	30	state.issues.set("o/r", [
	31	{ id: 5, number: 1, title: "p", body: "", state: "open", user: "x", labels: [], createdAt: "2026-01-01T00:00:00Z", updatedAt: "2026-01-01T00:00:00Z", pull: { headSha: "a".repeat(40), headRef: "h", baseRef: "main", merged: false } },
	32	] as never);
	33	fake = serveFakeForge(state);
	34	});
	35	afterEach(() => fake.stop());
	36	const provider = () => {
	37	const credential = staticCredential({ kind, apiUrl: fake.apiUrl, webUrl: fake.url, token: TOKEN });
	38	return kind === "forgejo" ? new ForgejoProvider(credential) : new GitHubProvider(credential);
	39	};
	40	const posts = () => state.requests.filter((r) => r.method === "POST" && r.path.endsWith("/pulls/1/reviews"));
	41
	42	test("one request carries the verdict and the three comments", async () => {
	43	const sent = await provider().submitReview("o/r", 1, { state: "changes_requested", body: "see lines", comments: lines });
	44	expect(sent.ok).toBe(true);
	45	if (sent.ok) expect(sent.value.carried).toBe(3);
	46	expect(posts()).toHaveLength(1);
	47	const body = JSON.parse(posts()[0]!.body);
	48	expect(body.event).toBe("REQUEST_CHANGES");
	49	expect(body.comments).toHaveLength(3);
	50	expect(body.comments[0]).toMatchObject(
	51	kind === "forgejo" ? { path: "a.ts", new_position: 3, body: "one" } : { path: "a.ts", line: 3, side: "RIGHT", body: "one" },
	52	);
	53	expect(body.comments[1]).toMatchObject(kind === "forgejo" ? { old_position: 9 } : { line: 9, side: "LEFT" });
	54	});
	55
	56	test("a review without lines sends no comments key", async () => {
	57	await provider().submitReview("o/r", 1, { state: "approved", body: "" });
	58	expect(posts()).toHaveLength(1);
	59	expect("comments" in JSON.parse(posts()[0]!.body)).toBe(false);
	60	});
	61
	62	test("an origin that refuses the payload still gets the verdict, alone", async () => {
	63	state.refuseReviewComments = true;
	64	const sent = await provider().submitReview("o/r", 1, { state: "changes_requested", body: "ok", comments: lines });
	65	expect(sent.ok).toBe(true);
	66	if (sent.ok) expect(sent.value.carried).toBe(0);
	67	const bodies = posts().map((r) => JSON.parse(r.body));
	68	expect(bodies).toHaveLength(2);
	69	expect("comments" in bodies[1]).toBe(false);
	70	expect(bodies[1].body).toBe("ok");
	71	});
	72
	73	test("a refusal about who we are is not retried", async () => {
	74	state.token = "other";
	75	const sent = await provider().submitReview("o/r", 1, { state: "approved", body: "", comments: lines });
	76	expect(sent.ok).toBe(false);
	77	expect(posts()).toHaveLength(1);
	78	});
	79	});
	80	}