Skip to content

Fix desktop file link downloads - #55720

Open
Aculnaj wants to merge 1 commit into
NousResearch:mainfrom
Aculnaj:Aculnaj/hermes-file-links
Open

Fix desktop file link downloads#55720
Aculnaj wants to merge 1 commit into
NousResearch:mainfrom
Aculnaj:Aculnaj/hermes-file-links

Conversation

@Aculnaj

@Aculnaj Aculnaj commented Jun 30, 2026

Copy link
Copy Markdown

What does this PR do?

Fixes Hermes Desktop handling for local file links returned in assistant messages. Streamdown blocks raw file:// markdown links, and remote desktop sessions need gateway-local files to open through Hermes' authenticated download endpoint rather than the client's local filesystem. This PR normalizes local file markdown links into Hermes media links before markdown rendering and renders generic files as immediate Open <name> links.

Related Issue

Related: #44523, #44538

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • Added markdown preprocessing in apps/desktop/src/lib/markdown-preprocess.ts to rewrite local file links to #media: links before Streamdown blocks them.
  • Updated apps/desktop/src/components/assistant-ui/markdown-text.tsx so generic file media links open directly through the existing media download path.
  • Added apps/desktop/src/components/assistant-ui/markdown-file-links.test.tsx covering remote file-link downloads.

How to Test

  1. npm run test:ui --workspace apps/desktop -- src/components/assistant-ui/markdown-file-links.test.tsx src/components/assistant-ui/markdown-text.test.ts src/lib/media.remote.test.ts src/lib/chat-messages.test.ts
  2. npm run typecheck --workspace apps/desktop
  3. From apps/desktop: npx eslint src/components/assistant-ui/markdown-file-links.test.tsx src/components/assistant-ui/markdown-text.tsx src/lib/markdown-preprocess.ts

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

Focused desktop tests and typecheck pass. The full npm run lint --workspace apps/desktop -- ... script still reports pre-existing unrelated lint errors in apps/desktop/electron/titlebar-overlay-width.cjs; the three changed files lint cleanly with direct ESLint from apps/desktop.

@Aculnaj
Aculnaj marked this pull request as ready for review June 30, 2026 15:17
@alt-glitch alt-glitch added type/bug Something isn't working comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have labels Jun 30, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for identifying the missing raw-file-link normalization; current main still has that gap: preprocessMarkdown() does not produce a #media: href, so MarkdownLink() cannot reach MediaAttachment (apps/desktop/src/lib/markdown-preprocess.ts:384-386, apps/desktop/src/components/assistant-ui/markdown-text.tsx:284-313).

Problems

  • The new generic-file handler opens mediaExternalUrl(path) (apps/desktop/src/components/assistant-ui/markdown-text.tsx:222 in this PR). Current main deliberately routes remote media through downloadGatewayMediaFile() (:133-145) because mediaExternalUrl() falls back to client-local file:// when a remote connection lacks a query token (apps/desktop/src/lib/media.remote.test.ts:72-75). This regresses remote OAuth/no-token downloads fixed by 03406ae2553e.
  • isAbsoluteFileHref() rejects extensionless absolute files such as [README](/tmp/README) at PR apps/desktop/src/lib/markdown-preprocess.ts:163.

Suggested changes

  • Salvage the preprocessing conversion, but preserve current main's authenticated useOpenMediaFile() path for generic files.
  • Treat slash-prefixed paths as absolute without an extension check, and cover both extensionless paths and remote no-token downloads.

Automated hermes-sweeper review.

className="font-semibold text-foreground underline underline-offset-4 decoration-current/20 wrap-anywhere"
href="#"
onClick={event => {
event.preventDefault()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bypasses current main's useOpenMediaFile() bridge. mediaExternalUrl() falls back to file:// when a remote connection lacks a query token, so remote OAuth sessions again target the client's filesystem. Keep the current authenticated downloadGatewayMediaFile() path here.

return true
}

const clean = href.split(/[?#]/, 1)[0] || ''

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rejects valid extensionless absolute files such as [README](/tmp/README). The matcher already restricts this path to slash-prefixed hrefs, so accept that form directly and add a regression test.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants