Codex review · post-merge readiness pass

Two findings, both real, neither blocking

An independent code review of canvases v0.1 after the Windows-NUL fix and the docs-extension expansion. Two issues surfaced — one P2 (production-of-DOM hazard), one P3 (corner-case parser misrender).

PR #1653 commit 68952859e codex pass 13 (post-merge) model_reasoning_effort=high
Verdict
GATE: PASS — no [P1] findings, both items are bounded-impact follow-ups.
Severity
1 × P2 · 1 × P3 · 0 × P1
Recommendation
Fix the P2 in this PR (10-line schema cap, prevents agent-driven DOM blowup); land the P3 as a follow-up since it only mis-labels a path containing literal " b/".
Codex passes
13
Total findings
20
Fixed pre-merge
19
Open
2

Finding 1 — file-based diff canvases lack a line cap

P2

Cap file-based diff canvas lines

What. The Zod schema for type: "diff" canvases accepts an unlimited number of hunk lines per file. CanvasDiff.tsx renders one DOM row per line, and the dashboard polls every 5s.

Why it matters. readCanvases already permits 32 files of 256 KB each per session — that's 8 MB of canvas data. A well-meaning agent emitting a giant compacted diff (e.g. git log -p dumped into a canvas) creates hundreds of thousands of DOM nodes on every poll. The synthesized git-diff path mitigates this with explicit caps:

// canvas-log.ts (synthesized path — already protected)
const DIFF_MAX_FILES = 200;
const DIFF_MAX_LINES = 5000;

File-emitted diff canvases bypass this. The 256 KB byte cap doesn't protect us — a tiny-line diff can pack tens of thousands of one-character add/del lines into 256 KB.

Recommended fix

// canvas-schema.ts
const DiffHunkSchema = z.object({
  header: z.string(),
- lines: z.array(DiffLineSchema),
+ lines: z.array(DiffLineSchema).max(DIFF_MAX_LINES_PER_HUNK),
});
const DiffFileSchema = z.object({
  path: z.string().min(1),
  status: z.enum(["added", "modified", "deleted", "renamed"]),
- hunks: z.array(DiffHunkSchema),
+ hunks: z.array(DiffHunkSchema).max(DIFF_MAX_HUNKS_PER_FILE),
});

Plus a top-level files.max(DIFF_MAX_FILES) on the diff payload. Reuse the constants from canvas-log.ts, or copy them — the cap values can match the synthesized path (200 files × 5000 lines).

Effort: ~10 lines including 1 test. Risk if not fixed: a single buggy agent freezes any session detail page that loads its workspace.

Finding 2 — diff parser mis-handles paths containing " b/"

P3

Handle ambiguous Git diff paths

What. The diff --git line parser uses lastIndexOf(" b/") to split oldPath and newPath. For a file whose actual repository path contains the substring " b/" — e.g. x b/y.txt — git emits:

diff --git a/x b/y.txt b/x b/y.txt
--- a/x b/y.txt
+++ b/x b/y.txt

lastIndexOf(" b/") picks the wrong split, so the canvas reports oldPath="x b/y.txt b/x" and path="y.txt". Both are bogus.

Context. This is a regression from the ReDoS fix in pass 11. The original regex /(.+) b\/(.+)/ had the same ambiguity but was vulnerable to js/redos. Switching to lastIndexOf killed the security issue but kept the correctness one.

Recommended fix — parse the ---/+++ headers instead

Git emits the unambiguous old/new paths on the next two lines after diff --git. Each line has exactly one path (no separator ambiguity). Use those instead:

// In parseUnifiedDiff(), buffer the next two header lines after
// `diff --git` and read paths from them rather than the diff line itself.

if (line.startsWith("--- a/")) {
  if (current) current.oldPath = line.slice(6);
  continue;
}
if (line.startsWith("+++ b/")) {
  if (current) current.path = line.slice(6);
  continue;
}

Keep the diff --git line as the "open a new file" trigger but stop parsing paths from it. The ---/+++ lines are also where added/deleted-file markers (--- /dev/null) appear, so this gives us correct status detection too.

Effort: ~15 lines. Risk if not fixed: file labels mis-render for any repo path containing literal " b/" — uncommon but legal. No security or DoS impact.

Why this isn't blocking

DimensionP2 capP3 path
User-visible impactPage freeze on bad inputWrong filename in canvas header
Trigger probabilityAny agent emitting large diffRepo path contains " b/"
WorkaroundDon't open the sessionNone needed (renders, just labelled wrong)
Existing protection256 KB byte cap (insufficient)None
Fix complexity~10 lines schema~15 lines parser
Recommendation — fix the P2 in this PR because it's a live DOS-the-dashboard vector that any well-meaning agent could trip with a single large diff JSON; defer the P3 to a follow-up since it only mis-renders a label for repository paths that include literal " b/" substrings, and gating ship on a hand-tweaked corner case is the wrong trade. The P2 fix is bounded (schema + 1 test), reviewable in 5 minutes, and prevents a real failure mode.

Convergence signal

The codex review gauntlet for this PR is now 13 passes deep:

The new findings aren't regressions — they're new analysis paths codex took with more context. Pass 12 didn't look hard at the file-based diff schema; pass 13 did. This is normal for adversarial review and stops being useful once we hit the same "no findings" outcome twice in a row.

Action items

ActionOwnerWhen
Add DIFF_MAX_LINES_PER_HUNK + DIFF_MAX_HUNKS_PER_FILE + DIFF_MAX_FILES caps to canvas-schema.tsClaudeNow (pre-merge)
Add 1 test that proves an over-cap diff canvas is rejectedClaudeNow (pre-merge)
Switch parser to ---/+++ header reading for oldPath/newPathFollow-up PRPost-merge — file an issue, not in this PR