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).
" b/".Finding 1 — file-based diff canvases lack a line cap
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/"
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
| Dimension | P2 cap | P3 path |
|---|---|---|
| User-visible impact | Page freeze on bad input | Wrong filename in canvas header |
| Trigger probability | Any agent emitting large diff | Repo path contains " b/" |
| Workaround | Don't open the session | None needed (renders, just labelled wrong) |
| Existing protection | 256 KB byte cap (insufficient) | None |
| Fix complexity | ~10 lines schema | ~15 lines parser |
" 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:
- Passes 1–10 surfaced 18 distinct bugs, all fixed pre-merge.
- Pass 11 (post-PR) surfaced the deferred-v0.2 scope question (rejected with documented reasoning).
- Pass 12 (post-fixes commit) returned zero findings — the convergence point we acted on for the original ship decision.
- Pass 13 (today, post-Windows fix + docs expansion) surfaces 2 new items, both narrow.
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
| Action | Owner | When |
|---|---|---|
Add DIFF_MAX_LINES_PER_HUNK + DIFF_MAX_HUNKS_PER_FILE + DIFF_MAX_FILES caps to canvas-schema.ts | Claude | Now (pre-merge) |
| Add 1 test that proves an over-cap diff canvas is rejected | Claude | Now (pre-merge) |
Switch parser to ---/+++ header reading for oldPath/newPath | Follow-up PR | Post-merge — file an issue, not in this PR |