History

fix: address PR review feedback

Julius Marminge committed 8 months ago

Commit 41734df1 parent 8f42d345 files · +108 −25Browse files

Unified · Split

5 files
  1. apps/server/src/git/Layers/CodexTextGeneration.test.ts +57 −0
  2. apps/server/src/git/Layers/CodexTextGeneration.ts +28 −11
  3. apps/server/src/imageMime.test.ts +12 −1
  4. apps/server/src/imageMime.ts +5 −2
  5. apps/server/src/orchestration/Layers/ProviderCommandReactor.ts +6 −11
apps/server/src/git/Layers/CodexTextGeneration.test.ts+57−0
@@ -349,6 +349,15 @@ it.layer(CodexTextGenerationTestLayer)("CodexTextGenerationLive", (it) => { · 348 unmodified lines
349349 },
350350 ],
351351 })
352+ .pipe(
353+ Effect.tap(() =>
354+ fs.stat(imagePath).pipe(
355+ Effect.map((fileInfo) => {
356+ expect(fileInfo.type).toBe("File");
357+ }),
358+ ),
359+ ),
360+ )
352361 .pipe(
353362 Effect.ensuring(
354363 fs.remove(imagePath).pipe(Effect.catch(() => Effect.void)),
@@ -360,6 +369,54 @@ it.layer(CodexTextGenerationTestLayer)("CodexTextGenerationLive", (it) => { · 5 unmodified lines
360369 ),
361370 );
362371
372+ it.effect("ignores absolute non-route attachment paths for codex image inputs", () =>
373+ withFakeCodexEnv(
374+ {
375+ output: JSON.stringify({
376+ branch: "fix/ui-regression",
377+ }),
378+ requireImage: true,
379+ },
380+ Effect.gen(function* () {
381+ const fs = yield* FileSystem.FileSystem;
382+ const path = yield* Path.Path;
383+ const tempDir = yield* fs.makeTempDirectoryScoped({
384+ prefix: "t3code-codex-attachment-bypass-",
385+ });
386+ const absoluteImagePath = path.join(tempDir, "outside.png");
387+ yield* fs.writeFile(absoluteImagePath, Buffer.from("hello"));
388+
389+ const textGeneration = yield* TextGeneration;
390+ const result = yield* textGeneration
391+ .generateBranchName({
392+ cwd: process.cwd(),
393+ message: "Fix layout bug from screenshot.",
394+ attachments: [
395+ {
396+ type: "image",
397+ name: "outside.png",
398+ mimeType: "image/png",
399+ sizeBytes: 5,
400+ dataUrl: absoluteImagePath,
401+ },
402+ ],
403+ })
404+ .pipe(
405+ Effect.match({
406+ onFailure: (error) => ({ _tag: "Left" as const, left: error }),
407+ onSuccess: (value) => ({ _tag: "Right" as const, right: value }),
408+ }),
409+ );
410+
411+ expect(result._tag).toBe("Left");
412+ if (result._tag === "Left") {
413+ expect(result.left).toBeInstanceOf(TextGenerationError);
414+ expect(result.left.message).toContain("missing --image input");
415+ }
416+ }),
417+ ),
418+ );
419+
363420 it.effect(
364421 "fails with typed TextGenerationError when codex returns wrong branch payload shape",
365422 () =>
apps/server/src/git/Layers/CodexTextGeneration.ts+28−11
@@ -107,6 +107,11 @@ const makeCodexTextGeneration = Effect.gen(function* () { · 106 unmodified lines
107107 const commandSpawner = yield* ChildProcessSpawner.ChildProcessSpawner;
108108 const serverConfig = yield* Effect.service(ServerConfig);
109109
110+ type MaterializedImageAttachments = {
111+ readonly imagePaths: ReadonlyArray<string>;
112+ readonly tempImagePaths: ReadonlyArray<string>;
113+ };
114+
110115 const readStreamAsString = <E>(
111116 operation: string,
112117 stream: Stream.Stream<Uint8Array, E>,
@@ -176,10 +181,12 @@ const makeCodexTextGeneration = Effect.gen(function* () { · 63 unmodified lines
176181 const materializeImageAttachments = (
177182 operation: "generateCommitMessage" | "generatePrContent" | "generateBranchName",
178183 attachments: BranchNameGenerationInput["attachments"],
179− ): Effect.Effect<ReadonlyArray<string>, TextGenerationError> =>
180− Effect.gen(function* () {
184+ ): Effect.Effect<MaterializedImageAttachments, TextGenerationError> => {
185+ const tempImagePaths: string[] = [];
186+
187+ return Effect.gen(function* () {
181188 if (!attachments || attachments.length === 0) {
182− return [];
189+ return { imagePaths: [], tempImagePaths };
183190 }
184191
185192 const imagePaths: string[] = [];
@@ -206,6 +213,7 @@ const makeCodexTextGeneration = Effect.gen(function* () { · 20 unmodified lines
206213 extension,
207214 );
208215 imagePaths.push(imagePath);
216+ tempImagePaths.push(imagePath);
209217 continue;
210218 }
211219
@@ -213,20 +221,26 @@ const makeCodexTextGeneration = Effect.gen(function* () { · 1 unmodified line
213221 stateDir: serverConfig.stateDir,
214222 dataUrl: attachment.dataUrl,
215223 });
216− const localPath = persistedPath ?? attachment.dataUrl;
217− if (!path.isAbsolute(localPath)) {
224+ if (!persistedPath || !path.isAbsolute(persistedPath)) {
218225 continue;
219226 }
220227 const fileInfo = yield* fileSystem
221− .stat(localPath)
228+ .stat(persistedPath)
222229 .pipe(Effect.catch(() => Effect.succeed(null)));
223230 if (!fileInfo || fileInfo.type !== "File") {
224231 continue;
225232 }
226− imagePaths.push(localPath);
233+ imagePaths.push(persistedPath);
227234 }
228− return imagePaths;
229− });
235+ return { imagePaths, tempImagePaths };
236+ }).pipe(
237+ Effect.tapError(() =>
238+ Effect.all(tempImagePaths.map((filePath) => safeUnlink(filePath)), {
239+ concurrency: "unbounded",
240+ }).pipe(Effect.asVoid),
241+ ),
242+ );
243+ };
230244
231245 const runCodexJson = <S extends Schema.Top>({
232246 operation,
@@ -234,12 +248,14 @@ const makeCodexTextGeneration = Effect.gen(function* () { · 1 unmodified line
234248 prompt,
235249 outputSchemaJson,
236250 imagePaths = [],
251+ cleanupPaths = [],
237252 }: {
238253 operation: "generateCommitMessage" | "generatePrContent" | "generateBranchName";
239254 cwd: string;
240255 prompt: string;
241256 outputSchemaJson: S;
242257 imagePaths?: ReadonlyArray<string>;
258+ cleanupPaths?: ReadonlyArray<string>;
243259 }): Effect.Effect<S["Type"], TextGenerationError, S["DecodingServices"]> =>
244260 Effect.gen(function* () {
245261 const schemaPath = yield* writeTempFile(
@@ -313,7 +329,7 @@ const makeCodexTextGeneration = Effect.gen(function* () { · 67 unmodified lines
313329 });
314330
315331 const cleanup = Effect.all(
316− [schemaPath, outputPath, ...imagePaths].map((filePath) => safeUnlink(filePath)),
332+ [schemaPath, outputPath, ...cleanupPaths].map((filePath) => safeUnlink(filePath)),
317333 {
318334 concurrency: "unbounded",
319335 },
@@ -438,7 +454,7 @@ const makeCodexTextGeneration = Effect.gen(function* () { · 118 unmodified lines
438454
439455 const generateBranchName: TextGenerationShape["generateBranchName"] = (input) => {
440456 return Effect.gen(function* () {
441− const imagePaths = yield* materializeImageAttachments(
457+ const { imagePaths, tempImagePaths } = yield* materializeImageAttachments(
442458 "generateBranchName",
443459 input.attachments,
444460 );
@@ -476,6 +492,7 @@ const makeCodexTextGeneration = Effect.gen(function* () { · 31 unmodified lines
476492 branch: Schema.String,
477493 }),
478494 imagePaths,
495+ cleanupPaths: tempImagePaths,
479496 });
480497
481498 return {
apps/server/src/imageMime.test.ts+12−1
@@ -1,6 +1,6 @@
11 import { describe, expect, it } from "vitest";
22
3−import { parseBase64DataUrl } from "./imageMime.ts";
3+import { inferImageExtension, parseBase64DataUrl } from "./imageMime.ts";
44
55 describe("imageMime", () => {
66 it("parses base64 data URL with mime type", () => {
@@ -24,4 +24,15 @@ describe("imageMime", () => { · 17 unmodified lines
2424 it("rejects missing mime type", () => {
2525 expect(parseBase64DataUrl("data:;base64,SGVsbG8=")).toBeNull();
2626 });
27+
28+ it("parses base64 data URL with spaces in payload", () => {
29+ expect(parseBase64DataUrl("data:image/png;base64,SGVs bG8=\n")).toEqual({
30+ mimeType: "image/png",
31+ base64: "SGVsbG8=",
32+ });
33+ });
34+
35+ it("does not read inherited keys from mime extension map", () => {
36+ expect(inferImageExtension({ mimeType: "constructor" })).toBe(".bin");
37+ });
2738 });
apps/server/src/imageMime.ts+5−2
@@ -32,7 +32,7 @@ export const SAFE_IMAGE_FILE_EXTENSIONS = new Set([ · 31 unmodified lines
3232 export function parseBase64DataUrl(
3333 dataUrl: string,
3434 ): { readonly mimeType: string; readonly base64: string } | null {
35− const match = /^data:([^,]+),([a-z0-9+/=\r\n]+)$/i.exec(dataUrl.trim());
35+ const match = /^data:([^,]+),([a-z0-9+/=\r\n ]+)$/i.exec(dataUrl.trim());
3636 if (!match) return null;
3737
3838 const headerParts = (match[1] ?? "")
@@ -55,7 +55,10 @@ export function parseBase64DataUrl( · 16 unmodified lines
5555 }
5656
5757 export function inferImageExtension(input: { mimeType: string; fileName?: string }): string {
58− const fromMime = IMAGE_EXTENSION_BY_MIME_TYPE[input.mimeType.toLowerCase()];
58+ const key = input.mimeType.toLowerCase();
59+ const fromMime = Object.hasOwn(IMAGE_EXTENSION_BY_MIME_TYPE, key)
60+ ? IMAGE_EXTENSION_BY_MIME_TYPE[key]
61+ : undefined;
5962 if (fromMime) {
6063 return fromMime;
6164 }
apps/server/src/orchestration/Layers/ProviderCommandReactor.ts+6−11
@@ -1,5 +1,3 @@
1−import fs from "node:fs";
2−
31 import {
42 type ChatAttachment,
53 CommandId,
@@ -14,7 +12,7 @@ import { · 8 unmodified lines
1412 type ProviderThreadId,
1513 type TurnId,
1614 } from "@t3tools/contracts";
17−import { Cache, Cause, Duration, Effect, Layer, Option, Queue, Stream } from "effect";
15+import { Cache, Cause, Duration, Effect, FileSystem, Layer, Option, Queue, Stream } from "effect";
1816
1917 import { resolveThreadWorkspaceCwd } from "../../checkpointing/Utils.ts";
2018 import {
@@ -111,6 +109,7 @@ const make = Effect.gen(function* () { · 90 unmodified lines
111109 const providerService = yield* ProviderService;
112110 const git = yield* GitCore;
113111 const textGeneration = yield* TextGeneration;
112+ const fileSystem = yield* FileSystem.FileSystem;
114113 const serverConfig = yield* Effect.service(ServerConfig);
115114 const handledTurnStartKeys = yield* Cache.make<string, true>({
116115 capacity: HANDLED_TURN_START_KEY_MAX,
@@ -333,14 +332,10 @@ const make = Effect.gen(function* () { · 216 unmodified lines
333332 return attachment;
334333 }
335334
336− const isFile = yield* Effect.sync(() => {
337− try {
338− return fs.statSync(resolvedMaterializedPath).isFile();
339− } catch {
340− return false;
341− }
342− });
343− if (!isFile) {
335+ const fileInfo = yield* fileSystem
336+ .stat(resolvedMaterializedPath)
337+ .pipe(Effect.catch(() => Effect.succeed(null)));
338+ if (!fileInfo || fileInfo.type !== "File") {
344339 return attachment;
345340 }
346341 return {