Conversation
|
🤖
|
📦 npm preview published —
|
|
🤖 |
pat-lewczuk
left a comment
There was a problem hiding this comment.
Code Review: fix(cockpit): keep local markdown paths inert
Verdict: 🔴 Changes requested — 1 major, 2 minor, 2 nits. No blockers.
The core fix is right and lands cleanly. I verified the before/after in jsdom: on main, [lesson](/Users/me/notes.md) renders as Streamdown's link-safety <button data-streamdown="link"> — click it and you get the "Open link?" dialog and then a bogus localhost navigation, exactly the reported bug. With this patch it renders <a data-streamdown="link">lesson</a> with no href, inert, text intact. https:// links are untouched. Reusing the existing isHttpUrl guard (#431) rather than inventing a second protocol allowlist is the right call, and the CSS rule is correctly placed in the deliberately-UNLAYERED block (index.css:481) so it outranks Streamdown's text-primary underline font-medium utilities. The a[…]:not([href]) selector also can't misfire: a live link renders as a <button>, so only the stripped case is ever an href-less <a>.
One thing needs to change before merge.
🟠 Major
1. urlTransform also strips image src, and a non-http image then disappears completely — alt text and all.
packages/web/src/routes/task-thread/markdown.tsx:101-104
Streamdown applies urlTransform to every URL attribute in html-url-attributes (href, src, poster, cite, …), not just href:
// node_modules/streamdown/dist/chunk-BO2N2NFS.js
for (let n in urlAttributes)
if (… Object.hasOwn(e.properties, n))
e.properties[n] = t(String(r || ''), n, e) ?? void 0markdownUrlTransform ignores its key argument, so image sources go through isHttpUrl too. Verified in jsdom:
| markdown | on main |
with this PR |
|---|---|---|
 |
<img alt="Screenshot" src="/home/me/qa/shot.png"> |
nothing — container renders as <div class="… thread-markdown"></div> |
 |
<img alt="diagram" src="/a.png"> |
nothing |
 |
renders | renders (unaffected) |
This contradicts the contract the new comment itself states — "local file paths and other non-http destinations stay visible but inert". For links that is true; for images the content is silently deleted, with no trace that anything was suppressed. It's reachable in the PR's own primary surface (agents routinely write local screenshot paths into transcripts) and in the GitHub tab, which renders issue/PR bodies that commonly use repo-relative image paths.
The key argument already in the signature confines it precisely:
const markdownUrlTransform: UrlTransform = (url, key, node) => {
const transformed = defaultUrlTransform(url, key, node)
// Only link destinations navigate; an `img src` cannot turn into a bogus SPA route.
if (key !== 'href') return transformed
return isHttpUrl(transformed) ? transformed : undefined
}If suppressing non-http images is intended, that's defensible — but then say so in the comment and pin it with a test asserting what the reader is left with.
🟡 Minor
2. The behavioural blast radius is wider than the description says, and that's what QA will scope from.
The PR body says "scoped to the task-thread markdown renderer". True of the file, not of the behaviour — Markdown is imported by five modules:
packages/web/src/routes/task-thread/thread-items.tsx:24,run-header.tsx:79packages/web/src/routes/github/github.tsx:59— GitHub issue/PR bodies (:739) and comments (:1396)packages/web/src/routes/compare-variants.tsx:34packages/web/src/components/skill-detail.tsx:9—SKILL.mdbodies
GitHub bodies and SKILL.md text are full of relative links (references/claim-pr.md, docs/arch.md), which now render as inert text there too. That's arguably correct — those links already went nowhere useful — but this PR is needs-qa, and a tester reading the description will click through the task thread only. Please name the GitHub tab and the skill-detail view so they get covered.
3. The new test asserts only negatives, so it doesn't pin what the fix actually depends on.
packages/web/src/routes/task-thread/markdown.test.tsx:248-257
It checks that no dialog opens and window.open isn't called. It never checks the two things the behaviour rests on: that the anchor has no href (the exact hook a[data-streamdown='link']:not([href]) keys on), and that the path text survives — the "stays visible" half of the contract. A future transform returning '' instead of undefined would leave href="" on the anchor: this test stays green, the CSS rule stops matching, and the path renders as a styled link again. Two lines close it, and both hold today:
expect(rendered.hasAttribute('href')).toBe(false)
expect(rendered.textContent).toBe('lesson')🔵 Nits
4. CODE_REVIEW.md asks that non-obvious code cite the spec or issue behind it, and this file's neighbours do (#524 at markdown.tsx:56, #431 via isHttpUrl). The new markdownUrlTransform block and the new CSS rule carry no citation.
5. mailto: and tel: destinations are now inert too (verified). Consistent with how isHttpUrl is used across the app, and I found nothing in-tree that emits them — just worth one line in the description.
Validation gate
Run in an isolated worktree at cf0d51b6.
| Command | Result |
|---|---|
npm run typecheck |
✅ pass |
npm test |
|
npm run test:unit |
✅ pass (36) |
npm run build (incl. check:pack) |
✅ pass |
npm run test:package |
✅ pass (16) |
The 6 npm test failures are not from this PR — all are in packages/cezar (the PR touches only packages/web) and all assert "outside a git repository" (git.test.ts:70, git-worktree.test.ts, git-changes.test.ts, health-forge.test.ts, projects-api.test.ts, automations-api.test.ts). My review sandbox redirects TMPDIR to …/.ai/cezar/tmp/<runId>, which sits inside the cezar checkout, so mkdtempSync(join(tmpdir(), …)) lands in a git repo and getRepoInfo correctly returns {root: '/home/cezar/cezar', …} instead of null. Re-running those six files with TMPDIR=/tmp: 176/176 pass. PR CI is green on all three required checks.
Targeted: packages/web/src/routes/task-thread/markdown.test.tsx — 23/23 pass, including the new case.
needs-qa is the right call and stays on. Manual QA should cover the task thread, the GitHub tab, and skill detail.
|
🤖
|
|
@matgren, the link fix itself is good — I confirmed the before/after in jsdom and One thing to change before this can merge: Also worth picking up while you're in there: two assertions on the new test to pin the href-absence and the surviving text (the CSS rule keys on Details and the reproduction table are in the review. Push the update and re-request review. |
|
🤖 1 major, 2 minor, 2 nits — the link fix is correct, but Validation gate ran at autofix: skipped (not my PR — re-run with |
🎯 What changes
Transcript markdown now keeps only
http://andhttps://destinations active. Agent-written local paths such as/Users/.../*.mdremain visible, but they no longer trigger the “Open link?” confirmation or send the SPA to a bogus localhost route after confirmation.📋 Scope
This is scoped to the task-thread markdown renderer and the thread markdown styling for Streamdown links that lose their
href. Existing browser-openable links still use the link-safety confirmation dialog, and no server/API contract changes.🧪 Validation
npm run test -w @open-mercato/cezar-web -- src/routes/task-thread/markdown.test.tsxnpm run build:servernpm run typecheck -w @open-mercato/cezar-webnpm run build -w @open-mercato/cezar-webManual browser QA has not been run in this PR; the change is user-facing click behavior, so it is marked
needs-qaunder the repo gate.