Skip to content

Commit d4307f5

Browse files
juliusmarmingeclaude
juliusmarminge
and
claude
committed
fix(pull-requests): store only the owning run's guide, and only from its own repository
A run's ownership check and its guide write now take turns with new claims, so a run that loses ownership while writing cannot store its older guide. A pull request from a repository the project is not a checkout of is refused instead of being measured against that checkout's commits. Co-Authored-By: Claude Opus 5.5 <[email protected]>
1 parent 801093b commit d4307f5

2 files changed

Lines changed: 103 additions & 11 deletions

File tree

‎apps/server/src/pullRequest/PullRequestReviewGuideService.test.ts‎

Lines changed: 51 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,20 @@ import * as PullRequestReviewGuideService from "./PullRequestReviewGuideService.
2929
import * as PullRequestService from "./PullRequestService.ts";
3030

3131
const projectId = ProjectId.make("project-1");
32+
/** The test project is a checkout of the repository its pull requests come from. */
33+
const ACME_APP = {
34+
canonicalKey: "github.com/acme/app",
35+
locator: {
36+
source: "git-remote" as const,
37+
remoteName: "origin",
38+
remoteUrl: "https://github.com/acme/app.git",
39+
},
40+
provider: "github",
41+
displayName: "acme/app",
42+
owner: "acme",
43+
name: "app",
44+
};
45+
3246
const reference: PullRequestRef = {
3347
projectId,
3448
host: "github.com",
@@ -151,6 +165,8 @@ const makeLayer = (input: {
151165
readonly enabled?: boolean;
152166
/** The head the host reports now, for a test that pushes between runs. */
153167
readonly currentHead?: Ref.Ref<string>;
168+
/** The project's own repository; defaults to the one the test reference names. */
169+
readonly identity?: typeof ACME_APP;
154170
}) =>
155171
PullRequestReviewGuideService.layer.pipe(
156172
Layer.provide(
@@ -181,7 +197,12 @@ const makeLayer = (input: {
181197
),
182198
Layer.provide(
183199
Layer.succeed(ProjectService.ProjectService, {
184-
getShell: () => Effect.succeedSome({ id: projectId, workspaceRoot: input.workspace }),
200+
getShell: () =>
201+
Effect.succeedSome({
202+
id: projectId,
203+
workspaceRoot: input.workspace,
204+
repositoryIdentity: input.identity ?? ACME_APP,
205+
}),
185206
} as unknown as ProjectService.ProjectService["Service"]),
186207
),
187208
Layer.provide(
@@ -451,4 +472,33 @@ it.layer(layerBase, { excludeTestServices: true })("PullRequestReviewGuideServic
451472
}),
452473
),
453474
);
475+
476+
it.effect("refuses a pull request from a repository the project is not a checkout of", () =>
477+
Effect.scoped(
478+
Effect.gen(function* () {
479+
const repositories = yield* makeRepositories;
480+
const stored = yield* Deferred.make<void>();
481+
const context = yield* Layer.build(
482+
makeLayer({
483+
...repositories,
484+
stored,
485+
identity: {
486+
...ACME_APP,
487+
canonicalKey: "github.com/acme/other",
488+
displayName: "acme/other",
489+
name: "other",
490+
},
491+
generate: () =>
492+
Effect.die("a guide must not be written from another repository's checkout"),
493+
}),
494+
);
495+
const service = Context.get(
496+
context,
497+
PullRequestReviewGuideService.PullRequestReviewGuideService,
498+
);
499+
const result = yield* Effect.flip(service.generate(reference));
500+
assert.include(result.message, "in another repository");
501+
}),
502+
),
503+
);
454504
});

‎apps/server/src/pullRequest/PullRequestReviewGuideService.ts‎

Lines changed: 52 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import * as FileSystem from "effect/FileSystem";
2121
import * as Layer from "effect/Layer";
2222
import * as Option from "effect/Option";
2323
import * as Ref from "effect/Ref";
24+
import * as Semaphore from "effect/Semaphore";
2425

2526
import {
2627
PullRequestOperationError,
@@ -30,10 +31,13 @@ import {
3031
type PullRequestReviewGuideResult,
3132
type PullRequestReviewGuideRun,
3233
type PullRequestStack,
34+
ProjectId,
35+
type RepositoryIdentity,
3336
type ThreadPullRequestKey,
3437
} from "@t3tools/contracts";
3538
import type { ReviewGuideStackLayer } from "@t3tools/provider-core/server/textGeneration";
3639
import { resolveProjectSettings } from "@t3tools/shared/projectSettings";
40+
import { sourceControlRepositorySelector } from "@t3tools/shared/sourceControl";
3741

3842
import * as ServerConfig from "../config.ts";
3943
import * as PullRequestReviewGuides from "../persistence/PullRequestReviewGuides.ts";
@@ -87,6 +91,9 @@ const make = Effect.gen(function* () {
8791
const runs = yield* Ref.make<ReadonlyMap<string, PullRequestReviewGuideRun>>(new Map());
8892
// Names each run's private git refs; unique for the server's lifetime, like the runs map.
8993
let runCounter = 0;
94+
// Claiming a run and storing a finished guide take turns, so a run that still owns its pull
95+
// request when it checks is the one whose guide is stored: a newer claim cannot land between.
96+
const ownership = yield* Semaphore.make(1);
9097

9198
const keyFor = Effect.fn("PullRequestReviewGuideService.keyFor")(function* (
9299
input: PullRequestRef,
@@ -101,9 +108,37 @@ const make = Effect.gen(function* () {
101108
detail: "Could not identify this pull request's host.",
102109
});
103110
}
104-
return { key, workspaceRoot: project.workspaceRoot };
111+
return { key, workspaceRoot: project.workspaceRoot, identity: project.repositoryIdentity };
105112
});
106113

114+
/**
115+
* A guide reads the change in a checkout fetched from the project's own remote, so the pull
116+
* request has to be in that repository. One from another repository is refused rather than
117+
* measured against commits that are not its own.
118+
*/
119+
const requireOwnRepository = Effect.fn("PullRequestReviewGuideService.requireOwnRepository")(
120+
function* (key: ThreadPullRequestKey, identity: RepositoryIdentity | null | undefined) {
121+
const own =
122+
identity == null
123+
? null
124+
: pullRequestSyncKey(
125+
{
126+
projectId: ProjectId.make("own"),
127+
number: key.number,
128+
repository: sourceControlRepositorySelector(identity) ?? "",
129+
},
130+
identity,
131+
);
132+
if (own === null || own.host !== key.host || own.repository !== key.repository) {
133+
return yield* new PullRequestOperationError({
134+
operation: "generateReviewGuide",
135+
detail:
136+
"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.",
137+
});
138+
}
139+
},
140+
);
141+
107142
const readStored = (key: ThreadPullRequestKey) =>
108143
guides.get(key).pipe(
109144
Effect.mapError(
@@ -314,7 +349,11 @@ const make = Effect.gen(function* () {
314349
};
315350
// A newer run for this pull request, say after a push, owns its guide now; this one's result
316351
// is for an older head and is dropped rather than written over it.
317-
if (yield* owns) yield* guides.put(key, guide);
352+
yield* ownership.withPermits(1)(
353+
Effect.gen(function* () {
354+
if (yield* owns) yield* guides.put(key, guide);
355+
}),
356+
);
318357
});
319358

320359
/** What a reader is told when a run fails; the full cause goes to the log. */
@@ -346,7 +385,8 @@ const make = Effect.gen(function* () {
346385
detail: "Review guides are off. Turn them on in Settings → Source Control.",
347386
});
348387
}
349-
const { key, workspaceRoot } = yield* keyFor(input);
388+
const { key, workspaceRoot, identity } = yield* keyFor(input);
389+
yield* requireOwnRepository(key, identity);
350390
const runKey = keyString(key);
351391
const [detail, stored] = yield* Effect.all([pullRequests.detail(input), readStored(key)], {
352392
concurrency: "unbounded",
@@ -364,13 +404,15 @@ const make = Effect.gen(function* () {
364404
startedAt: yield* nowIso,
365405
};
366406
// Claimed atomically, so two readers pressing Generate start one run between them.
367-
const claimed = yield* Ref.modify(runs, (current) => {
368-
const existing = current.get(runKey);
369-
if (existing?._tag === "Running" && existing.headSha === currentHeadSha) {
370-
return [existing, current] as const;
371-
}
372-
return [running, new Map(current).set(runKey, running)] as const;
373-
});
407+
const claimed = yield* ownership.withPermits(1)(
408+
Ref.modify(runs, (current) => {
409+
const existing = current.get(runKey);
410+
if (existing?._tag === "Running" && existing.headSha === currentHeadSha) {
411+
return [existing, current] as const;
412+
}
413+
return [running, new Map(current).set(runKey, running)] as const;
414+
}),
415+
);
374416
if (claimed !== running) return { guide: stored, run: claimed, currentHeadSha };
375417

376418
// The run is identified by its own Running value: a later claim replaces it, and from then

0 commit comments

Comments
 (0)