From aa5a144d04c5067b9344289c6bc6f28a8acb9765 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Thu, 26 Feb 2026 00:42:57 -0800 Subject: [PATCH] small nits --- .plans/16c-pr89-remediation-checklist.md | 8 ++++---- .../Layers/CheckpointDiffQuery.test.ts | 12 ++++-------- apps/server/src/codexAppServerManager.ts | 2 +- apps/server/src/open.ts | 15 +++++++++++---- apps/server/src/terminal/Layers/Manager.ts | 5 ++++- apps/web/src/markdown-links.test.ts | 12 ++++++++++++ apps/web/src/markdown-links.ts | 15 ++++++++++++--- 7 files changed, 48 insertions(+), 21 deletions(-) diff --git a/.plans/16c-pr89-remediation-checklist.md b/.plans/16c-pr89-remediation-checklist.md index 74d16d5e851805da2050a3a8697cb20b588f4907..8531c5f860ba2f612f4311791c04fda670f857cb 100644 --- a/.plans/16c-pr89-remediation-checklist.md +++ b/.plans/16c-pr89-remediation-checklist.md @@ -286,8 +286,8 @@ Counts: active `51` (`valid=33`, `partially-valid=18`), closed-invalid `6` - Threads: PRRT_kwDORLtfbc5wj4cM - Audit note: -ilc shell invocation can pollute captured PATH output. -- [ ] `C029` `parseFileUrlHref` already decodes the path (line 46), but `safeDecode` is called again here, corrupting filenames containing `%` sequences. Consider skipping the decode when `fileUrlTarget` is non-null. - - Status: `TODO` +- [x] `C029` `parseFileUrlHref` already decodes the path (line 46), but `safeDecode` is called again here, corrupting filenames containing `%` sequences. Consider skipping the decode when `fileUrlTarget` is non-null. + - Status: `DONE` - Verdict: `valid` - Severity: `Medium` - Area: `Edge-case parsing/platform behavior` @@ -295,8 +295,8 @@ Counts: active `51` (`valid=33`, `partially-valid=18`), closed-invalid `6` - Threads: PRRT_kwDORLtfbc5wnVsU - Audit note: file URL decoding still double-decodes in one path. -- [ ] `C030` `EXTERNAL_SCHEME_PATTERN` matches `script.ts:10` as a scheme because `.ts:` looks like `scheme:`. Consider requiring `://` after the colon, or checking that what follows the colon is not just digits.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent: - - Status: `TODO` +- [x] `C030` `EXTERNAL_SCHEME_PATTERN` matches `script.ts:10` as a scheme because `.ts:` looks like `scheme:`. Consider requiring `://` after the colon, or checking that what follows the colon is not just digits.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent: + - Status: `DONE` - Verdict: `valid` - Severity: `Medium` - Area: `Edge-case parsing/platform behavior` diff --git a/apps/server/src/checkpointing/Layers/CheckpointDiffQuery.test.ts b/apps/server/src/checkpointing/Layers/CheckpointDiffQuery.test.ts index 4f619c3b989faee781b1e1c9828029fd267a59f8..e01081ee3fc3464196bd1a4b547eeb6f68d51042 100644 --- a/apps/server/src/checkpointing/Layers/CheckpointDiffQuery.test.ts +++ b/apps/server/src/checkpointing/Layers/CheckpointDiffQuery.test.ts @@ -14,10 +14,6 @@ import { CheckpointDiffQueryLive } from "./CheckpointDiffQuery.ts"; import { CheckpointStore, type CheckpointStoreShape } from "../Services/CheckpointStore.ts"; import { CheckpointDiffQuery } from "../Services/CheckpointDiffQuery.ts"; -const asProjectId = (value: string): ProjectId => ProjectId.makeUnsafe(value); -const asTurnId = (value: string): TurnId => TurnId.makeUnsafe(value); -const asCheckpointRef = (value: string): CheckpointRef => CheckpointRef.makeUnsafe(value); - function makeSnapshot(input: { readonly projectId: ProjectId; readonly threadId: ThreadId; @@ -49,7 +45,7 @@ function makeSnapshot(input: { model: "gpt-5-codex", branch: null, worktreePath: input.worktreePath, - latestTurnId: asTurnId("turn-1"), + latestTurnId: TurnId.makeUnsafe("turn-1"), createdAt: "2026-01-01T00:00:00.000Z", updatedAt: "2026-01-01T00:00:00.000Z", deletedAt: null, @@ -57,7 +53,7 @@ function makeSnapshot(input: { activities: [], checkpoints: [ { - turnId: asTurnId("turn-1"), + turnId: TurnId.makeUnsafe("turn-1"), checkpointTurnCount: input.checkpointTurnCount, checkpointRef: input.checkpointRef, status: "ready", @@ -74,9 +70,9 @@ function makeSnapshot(input: { describe("CheckpointDiffQueryLive", () => { it("computes diffs using canonical turn-0 checkpoint refs", async () => { - const projectId = asProjectId("project-1"); + const projectId = ProjectId.makeUnsafe("project-1"); const threadId = ThreadId.makeUnsafe("thread-1"); - const toCheckpointRef = asCheckpointRef("refs/t3/checkpoints/thread-1/turn/1"); + const toCheckpointRef = checkpointRefForThreadTurn(threadId, 1); const hasCheckpointRefCalls: Array = []; const diffCheckpointsCalls: Array<{ readonly fromCheckpointRef: CheckpointRef; diff --git a/apps/server/src/codexAppServerManager.ts b/apps/server/src/codexAppServerManager.ts index f388baef17b0e684d08523b51ffc3cdf229b72b7..c2b5945048494d471b4cf0b71e3dc7e5f38beffc 100644 --- a/apps/server/src/codexAppServerManager.ts +++ b/apps/server/src/codexAppServerManager.ts @@ -986,7 +986,7 @@ function readResumeThreadId(input: ProviderSessionStartInput): ProviderThreadId typeof input.resumeCursor !== "object" || Array.isArray(input.resumeCursor) ) { - return undefined; + return input.resumeThreadId; } const rawThreadId = (input.resumeCursor as Record).threadId; return typeof rawThreadId === "string" ? toProviderThreadId(rawThreadId) : input.resumeThreadId; diff --git a/apps/server/src/open.ts b/apps/server/src/open.ts index dbfadbfce9ad78014eb566aa36a004b47eb1bfe8..3cb4e9338e2fb755a538c7cb59519a8471425cd6 100644 --- a/apps/server/src/open.ts +++ b/apps/server/src/open.ts @@ -85,10 +85,17 @@ export const resolveEditorLaunch = Effect.fnUntraced(function* ( export const launchDetached = (launch: EditorLaunch) => Effect.callback((resume) => { - const child = spawn(launch.command, [...launch.args], { - detached: true, - stdio: "ignore", - }); + let child; + try { + child = spawn(launch.command, [...launch.args], { + detached: true, + stdio: "ignore", + }); + } catch (error) { + return resume( + Effect.fail(new OpenError({ message: "failed to spawn detached process", cause: error })), + ); + } const handleSpawn = () => { child.unref(); diff --git a/apps/server/src/terminal/Layers/Manager.ts b/apps/server/src/terminal/Layers/Manager.ts index 59a45f06650c40d04a27923f0df8abf90a355832..3c3a9a1322d7dcca930b1c00b67299a5c254261e 100644 --- a/apps/server/src/terminal/Layers/Manager.ts +++ b/apps/server/src/terminal/Layers/Manager.ts @@ -1033,7 +1033,10 @@ export const TerminalManagerLive = Layer.effect( const logsDir = join(stateDir, "logs", "terminals"); const ptyAdapter = yield* PtyAdapter; - const runtime = new TerminalManagerRuntime({ logsDir, ptyAdapter }); + const runtime = yield* Effect.acquireRelease( + Effect.sync(() => new TerminalManagerRuntime({ logsDir, ptyAdapter })), + (r) => Effect.sync(() => r.dispose()), + ); return { open: (input) => Effect.promise(() => runtime.open(input)), diff --git a/apps/web/src/markdown-links.test.ts b/apps/web/src/markdown-links.test.ts index 900a5b6b2ce64e5badf504016aa3cc8a49cafca3..5f0a0cd71157d094ef56a0f83763e9693f15eef2 100644 --- a/apps/web/src/markdown-links.test.ts +++ b/apps/web/src/markdown-links.test.ts @@ -15,6 +15,12 @@ describe("resolveMarkdownFileLinkTarget", () => { ); }); + it("does not treat filename line references as external schemes", () => { + expect(resolveMarkdownFileLinkTarget("script.ts:10", "/Users/julius/project")).toBe( + "/Users/julius/project/script.ts:10", + ); + }); + it("resolves bare file names against cwd", () => { expect(resolveMarkdownFileLinkTarget("AGENTS.md", "/Users/julius/project")).toBe( "/Users/julius/project/AGENTS.md", @@ -31,6 +37,12 @@ describe("resolveMarkdownFileLinkTarget", () => { expect(resolveMarkdownFileLinkTarget("https://example.com/docs")).toBeNull(); }); + it("does not double-decode file URLs", () => { + expect(resolveMarkdownFileLinkTarget("file:///Users/julius/project/file%2520name.md")).toBe( + "/Users/julius/project/file%20name.md", + ); + }); + it("does not treat app routes as file links", () => { expect(resolveMarkdownFileLinkTarget("/chat/settings")).toBeNull(); }); diff --git a/apps/web/src/markdown-links.ts b/apps/web/src/markdown-links.ts index 7013c4029a5ca887f0c0abbb6d1c2292fd2dccfa..aa5f0f149808c2f368b570e0ccda9eb31328d07d 100644 --- a/apps/web/src/markdown-links.ts +++ b/apps/web/src/markdown-links.ts @@ -2,11 +2,12 @@ import { resolvePathLinkTarget } from "./terminal-links"; const WINDOWS_DRIVE_PATH_PATTERN = /^[A-Za-z]:[\\/]/; const WINDOWS_UNC_PATH_PATTERN = /^\\\\/; -const EXTERNAL_SCHEME_PATTERN = /^[A-Za-z][A-Za-z0-9+.-]*:/; +const EXTERNAL_SCHEME_PATTERN = /^([A-Za-z][A-Za-z0-9+.-]*):(.*)$/; const RELATIVE_PATH_PREFIX_PATTERN = /^(~\/|\.{1,2}\/)/; const RELATIVE_FILE_PATH_PATTERN = /^[A-Za-z0-9._-]+(?:\/[A-Za-z0-9._-]+)+(?::\d+){0,2}$/; const RELATIVE_FILE_NAME_PATTERN = /^[A-Za-z0-9._-]+\.[A-Za-z0-9_-]+(?::\d+){0,2}$/; const POSITION_SUFFIX_PATTERN = /:\d+(?::\d+)?$/; +const POSITION_ONLY_PATTERN = /^\d+(?::\d+)?$/; const POSIX_FILE_ROOT_PREFIXES = [ "/Users/", "/home/", @@ -89,6 +90,14 @@ function isRelativePath(path: string): boolean { ); } +function hasExternalScheme(path: string): boolean { + const match = path.match(EXTERNAL_SCHEME_PATTERN); + if (!match) return false; + const rest = match[2] ?? ""; + if (rest.startsWith("//")) return true; + return !POSITION_ONLY_PATTERN.test(rest); +} + export function resolveMarkdownFileLinkTarget( href: string | undefined, cwd?: string, @@ -101,14 +110,14 @@ export function resolveMarkdownFileLinkTarget( ? parseFileUrlHref(rawHref) : null; const source = fileUrlTarget ?? stripSearchAndHash(rawHref); - const decodedPath = safeDecode(source.path.trim()); + const decodedPath = fileUrlTarget ? source.path.trim() : safeDecode(source.path.trim()); const decodedHash = safeDecode(source.hash.trim()); if (decodedPath.length === 0) return null; if ( !WINDOWS_DRIVE_PATH_PATTERN.test(decodedPath) && !WINDOWS_UNC_PATH_PATTERN.test(decodedPath) && - EXTERNAL_SCHEME_PATTERN.test(decodedPath) + hasExternalScheme(decodedPath) ) { return null; } -- 2.54.0