Merge pull request #262 from konard/issue-261-2c9f5e5b48ce

feat(terminal): preview images shown by codex viewed image output

Author: skulidropek

packages/api/src/services/terminal-image-fetch-core.ts

@@ -103,17 +103,39 @@ const normalizeTerminalImagePath = (path: string): TerminalImagePathNormalizatio
   }
 }
 
-export const planTerminalImageFetch = (path: string): TerminalImageFetchPlan => {
+export type TerminalImageFetchOptions = {
+  readonly baseDir?: string
+}
+
+const isAbsolutePosixPath = (value: string): boolean => value.startsWith("/")
+
+const joinBaseDirAndRelativePath = (baseDir: string, relativePath: string): string => {
+  const trimmedBase = baseDir.replace(/\/+$/u, "")
+  return `${trimmedBase}/${relativePath}`
+}
+
+export const planTerminalImageFetch = (
+  path: string,
+  options: TerminalImageFetchOptions = {}
+): TerminalImageFetchPlan => {
   if (typeof path !== "string" || path.length === 0) {
     return { _tag: "InvalidTerminalImageFetch", message: "Image path is required." }
   }
   const normalized = normalizeTerminalImagePath(path)
   if (normalized._tag === "InvalidTerminalImagePath") {
     return { _tag: "InvalidTerminalImageFetch", message: normalized.message }
   }
-  const containerPath = normalized.path
-  if (!containerPath.startsWith("/")) {
-    return { _tag: "InvalidTerminalImageFetch", message: "Image path must be absolute." }
+  const normalizedPath = normalized.path
+  let containerPath = normalizedPath
+  if (!isAbsolutePosixPath(containerPath)) {
+    const baseDir = options.baseDir
+    if (baseDir === undefined || !isAbsolutePosixPath(baseDir)) {
+      return { _tag: "InvalidTerminalImageFetch", message: "Image path must be absolute." }
+    }
+    if (invalidCharacterPattern.test(baseDir) || traversalPattern.test(baseDir)) {
+      return { _tag: "InvalidTerminalImageFetch", message: "Image base directory is invalid." }
+    }
+    containerPath = joinBaseDirAndRelativePath(baseDir, containerPath)
   }
   if (invalidCharacterPattern.test(containerPath)) {
     return { _tag: "InvalidTerminalImageFetch", message: "Image path contains invalid characters." }

packages/api/src/services/terminal-sessions.ts

@@ -59,6 +59,7 @@ type TerminalRecord = {
   projectDisplayName: string
   projectId: string
   projectKey: string
+  projectTargetDir: string
   prepared: ReturnType<typeof prepareProjectSsh>
 }
 
@@ -581,7 +582,8 @@ const registerRecord = (
   projectKey: string,
   projectDisplayName: string,
   prepared: ReturnType<typeof prepareProjectSsh>,
-  projectContainerName: string
+  projectContainerName: string,
+  projectTargetDir: string
 ): TerminalSession => {
   const session: TerminalSession = {
     attachedClients: 0,
@@ -600,6 +602,7 @@ const registerRecord = (
     projectDisplayName,
     projectId,
     projectKey,
+    projectTargetDir,
     pty: null,
     session,
     sockets: new Set<WebSocket>()
@@ -662,7 +665,8 @@ export const createTerminalSession = (
         project.projectKey,
         project.displayName,
         prepared,
-        projectItem.containerName
+        projectItem.containerName,
+        projectItem.targetDir
       )
       yield* _(emitTerminalSessionCreated(projectId, session.id, options.requestId))
       return { project, session }
@@ -681,7 +685,8 @@ export const createTerminalSession = (
       project.projectKey,
       project.displayName,
       prepared,
-      reachableProjectItem.containerName
+      reachableProjectItem.containerName,
+      reachableProjectItem.targetDir
     )
     yield* _(emitTerminalSessionCreated(projectId, session.id, options.requestId))
     yield* _(emitTerminalStatus(projectId, "ssh.post-start", "Post-start self-heal continues in background"))
@@ -786,7 +791,7 @@ export const readProjectTerminalImage = (
         Effect.fail(new ApiNotFoundError({ message: `Terminal session not found: ${sessionId}` }))
       )
     }
-    const plan = planTerminalImageFetch(imagePath)
+    const plan = planTerminalImageFetch(imagePath, { baseDir: record.projectTargetDir })
     if (plan._tag === "InvalidTerminalImageFetch") {
       return yield* _(Effect.fail(new ApiBadRequestError({ message: plan.message })))
     }

packages/api/tests/terminal-image-fetch-core.test.ts

@@ -103,4 +103,53 @@ describe("terminal image fetch core", () => {
       _tag: "InvalidTerminalImageFetch"
     })
   })
+
+  it("resolves a relative path against the provided absolute base directory", () => {
+    expect(
+      planTerminalImageFetch(
+        "app/docs/screenshots/issue-237/proof/pr238-proof-06-skiller-submodule-status.png",
+        { baseDir: "/home/dev" }
+      )
+    ).toEqual({
+      _tag: "ValidTerminalImageFetch",
+      containerPath: "/home/dev/app/docs/screenshots/issue-237/proof/pr238-proof-06-skiller-submodule-status.png",
+      mediaType: "image/png"
+    })
+  })
+
+  it("ignores trailing slashes on the base directory when resolving a relative path", () => {
+    expect(planTerminalImageFetch("photos/cover.jpg", { baseDir: "/home/dev/" })).toEqual({
+      _tag: "ValidTerminalImageFetch",
+      containerPath: "/home/dev/photos/cover.jpg",
+      mediaType: "image/jpeg"
+    })
+  })
+
+  it("still rejects relative paths when no base directory is supplied", () => {
+    expect(planTerminalImageFetch("app/cover.png")).toEqual({
+      _tag: "InvalidTerminalImageFetch",
+      message: "Image path must be absolute."
+    })
+  })
+
+  it("rejects relative paths when the supplied base directory is not absolute", () => {
+    expect(planTerminalImageFetch("app/cover.png", { baseDir: "home/dev" })).toEqual({
+      _tag: "InvalidTerminalImageFetch",
+      message: "Image path must be absolute."
+    })
+  })
+
+  it("rejects an unsafe base directory containing traversal segments", () => {
+    expect(planTerminalImageFetch("cover.png", { baseDir: "/home/dev/../etc" })).toEqual({
+      _tag: "InvalidTerminalImageFetch",
+      message: "Image base directory is invalid."
+    })
+  })
+
+  it("rejects relative paths that contain traversal segments after resolution", () => {
+    expect(planTerminalImageFetch("../etc/cover.png", { baseDir: "/home/dev" })).toMatchObject({
+      _tag: "InvalidTerminalImageFetch",
+      message: "Image path must not contain '.' or '..' segments."
+    })
+  })
 })

packages/app/src/web/terminal-image-paths.ts

@@ -8,6 +8,13 @@ const imagePathPattern = new RegExp(
   "giu"
 )
 
+const treePointerImagePathSource =
+  String.raw`[^\s"'(<>\[\]{}|\\/][^\s"'(<>\[\]{}|\\]*\.(?:${extensionAlternation})`
+const treePointerImagePathPattern = new RegExp(
+  String.raw`(?:^|\s)[└├]\s+(${treePointerImagePathSource})(?=$|[\s"')<>\[\]{}|.,;:?!])`,
+  "giu"
+)
+
 const escapeChar = String.fromCodePoint(0x1B)
 const bellChar = String.fromCodePoint(0x07)
 
@@ -26,22 +33,38 @@ export type TerminalImagePathMatch = {
 export const stripTerminalAnsi = (text: string): string =>
   text.replace(ansiOscPattern, "").replace(ansiCsiPattern, "").replace(ansiOtherEscapePattern, "")
 
-export const detectTerminalImagePathMatches = (text: string): ReadonlyArray<TerminalImagePathMatch> => {
-  const plainText = stripTerminalAnsi(text)
-  const matches: Array<TerminalImagePathMatch> = []
-  for (const match of plainText.matchAll(imagePathPattern)) {
+const collectPatternMatches = (
+  plainText: string,
+  pattern: RegExp,
+  matches: Array<TerminalImagePathMatch>,
+  seenStartIndices: Set<number>
+): void => {
+  for (const match of plainText.matchAll(pattern)) {
     const candidate = match[1]
-    if (candidate !== undefined && candidate.length > 0) {
-      const fullMatch = match[0]
-      const fullStartIndex = match.index
-      const startIndex = fullStartIndex + fullMatch.lastIndexOf(candidate)
-      matches.push({
-        endIndex: startIndex + candidate.length,
-        path: candidate,
-        startIndex
-      })
+    if (candidate === undefined || candidate.length === 0) {
+      continue
     }
+    const fullMatch = match[0]
+    const fullStartIndex = match.index
+    const startIndex = fullStartIndex + fullMatch.lastIndexOf(candidate)
+    if (seenStartIndices.has(startIndex)) {
+      continue
+    }
+    seenStartIndices.add(startIndex)
+    matches.push({
+      endIndex: startIndex + candidate.length,
+      path: candidate,
+      startIndex
+    })
   }
+}
+
+export const detectTerminalImagePathMatches = (text: string): ReadonlyArray<TerminalImagePathMatch> => {
+  const plainText = stripTerminalAnsi(text)
+  const matches: Array<TerminalImagePathMatch> = []
+  const seenStartIndices = new Set<number>()
+  collectPatternMatches(plainText, imagePathPattern, matches, seenStartIndices)
+  collectPatternMatches(plainText, treePointerImagePathPattern, matches, seenStartIndices)
   return matches
 }
 

packages/app/tests/docker-git/terminal-image-paths.test.ts

@@ -98,4 +98,37 @@ describe("terminal image path detection", () => {
     expect(isSupportedTerminalImagePath("/var/data/a.bmp")).toBe(false)
     expect(isSupportedTerminalImagePath("/var/data/a.txt")).toBe(false)
   })
+
+  it("detects relative image paths under a tree pointer (issue 261 viewed image format)", () => {
+    const text = "  └ app/docs/screenshots/issue-237/proof/pr238-proof-06-skiller-submodule-status.png"
+    expect(detectTerminalImagePaths(text)).toEqual([
+      "app/docs/screenshots/issue-237/proof/pr238-proof-06-skiller-submodule-status.png"
+    ])
+  })
+
+  it("detects relative image paths under the alternative branch tree pointer", () => {
+    expect(detectTerminalImagePaths("  ├ assets/cover.jpg")).toEqual(["assets/cover.jpg"])
+  })
+
+  it("detects multiple viewed images across separate lines", () => {
+    const text = [
+      "• Viewed Image",
+      "  └ app/docs/screenshots/issue-237/proof/pr238-proof-06-skiller-submodule-status.png",
+      "",
+      "• Viewed Image",
+      "  └ app/docs/screenshots/issue-237/proof/pr238-proof-09-skiller-submodule-checks.png"
+    ].join("\n")
+    expect(detectTerminalImagePaths(text)).toEqual([
+      "app/docs/screenshots/issue-237/proof/pr238-proof-06-skiller-submodule-status.png",
+      "app/docs/screenshots/issue-237/proof/pr238-proof-09-skiller-submodule-checks.png"
+    ])
+  })
+
+  it("does not duplicate absolute paths preceded by a tree pointer", () => {
+    expect(detectTerminalImagePaths("  └ /var/data/foo.png")).toEqual(["/var/data/foo.png"])
+  })
+
+  it("ignores tree pointers that do not precede an image", () => {
+    expect(detectTerminalImagePaths("  └ Viewed File: notes.txt")).toEqual([])
+  })
 })