Skip to content

fix: keep shell fences rendered as code blocks - #58023

Open
danspicytaco wants to merge 4 commits into
NousResearch:mainfrom
danspicytaco:fix/ui-output-formatting
Open

fix: keep shell fences rendered as code blocks#58023
danspicytaco wants to merge 4 commits into
NousResearch:mainfrom
danspicytaco:fix/ui-output-formatting

Conversation

@danspicytaco

@danspicytaco danspicytaco commented Jul 4, 2026

Copy link
Copy Markdown

Context

Hermes Desktop can demote fenced snippets into prose when they look text-like. That breaks two related cases:

Solution

  • Treat explicit text, plain, and plaintext fences as code by author intent in both preprocessing and render-time classification.
  • Keep common shell fence languages (zsh, fish, powershell, cmd, etc.) on the code path.
  • Continue demoting untagged / Markdown-tagged prose-looking fences.

Verification

  • npm exec vitest run src/lib/markdown-code.test.ts src/components/assistant-ui/markdown-text.test.ts -- --environment node
  • npx eslint src/lib/markdown-code.ts src/lib/markdown-code.test.ts src/components/assistant-ui/markdown-text.test.ts
  • npm run typecheck
  • git diff --check

Screenshots

Before:

Before: shell command fence rendered as prose

After:

After: shell command fence rendered as a code block

@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 Jul 4, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #49972 (keep plain-text fences copyable) and #57540 (Desktop leaks explicit text fence language into prose) — all three touch the same apps/desktop/src/lib/markdown-code.ts fence-classification heuristics from different angles. Not a duplicate; a maintainer should reconcile the overlapping isLikelyProseFence() / fence-language logic across them.

@Kinkoolino-Hermes

Copy link
Copy Markdown
Contributor

This looks related to the explicit plain-text fence leak tracked in #57540 / #49972, but it also covers shell-fence aliases and Markdown-math side effects.

A useful consolidation criterion would be: explicit text / plain / plaintext fences must stay code blocks, and shell-like fences should not be demoted into prose where $HOME / $USER can be interpreted by Markdown/math rendering.

@danspicytaco

Copy link
Copy Markdown
Author

Thanks @Kinkoolino-Hermes for linking the related work. My intent with this PR is the narrower scoped shell-command case:

  • keep common shell aliases (zsh, fish, powershell, cmd, etc.) on the code path
  • avoid demoting shell-looking text fences into prose, where $HOME / $USER can hit markdown/math parsing
  • leave prose-looking text fences demoted as before

I agree this overlaps with #49972 / #57540 around explicit text / plain / plaintext handling. Happy for that to be the change that merges.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for targeting the shell-fence misclassification. The current-main premise is real: isLikelyProseCodeBlock() demotes unknown language blocks at apps/desktop/src/lib/markdown-code.ts:327, while the Desktop preprocessor removes a fence when isLikelyProseFence() succeeds at apps/desktop/src/lib/markdown-preprocess.ts:284-288.

Problems

  • The new tests cover isLikelyProseCodeBlock() only. The reported fence removal happens through preprocessMarkdown()isLikelyProseFence(); add an integration regression for the text-labeled shell fixture.
  • This intentionally narrow command-signal change does not resolve the adjacent explicit text/plain/plaintext fence case in [Bug]: Desktop leaks explicit text fence language into rendered prose #57540: ordinary file-path or prose-like bodies still satisfy apps/desktop/src/lib/markdown-code.ts:301-308 and are demoted.

Suggested changes

Automated hermes-sweeper review.

@teknium1 teknium1 added 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
@danspicytaco

Copy link
Copy Markdown
Author

thanks @teknium1, this has been addressed in the latest push.

Changes:

Verified:

  • npm exec vitest run src/lib/markdown-code.test.ts src/components/assistant-ui/markdown-text.test.ts -- --environment node
  • npx eslint src/lib/markdown-code.ts src/lib/markdown-code.test.ts src/components/assistant-ui/markdown-text.test.ts
  • npm run typecheck
  • git diff --check

RE the overlapping PRs: #49972 is the earliest plain-text fence PR and #57542 directly targets #57540. I’m happy for you to pick whichever consolidation target is best; this PR now covers the shell-fence regression plus the explicit plain-text fence contract, and I can transfer the shell-specific coverage if another PR becomes the merge target.

@alt-glitch alt-glitch added needs-decision Awaiting maintainer decision before any implementation and removed sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Jul 16, 2026
@danspicytaco

Copy link
Copy Markdown
Author

hey @OutThisLife, thank you for your note on #64721 . RE splitting out the unrelated markdown shell-fence change - this PR is the standalone version of that change.

It keeps shell-like fenced blocks on the code path, including common aliases like zsh, fish, powershell, cmd, and text-labeled shell command blocks. I also folded in the related explicit text / plain / plaintext fence handling and added preprocessing coverage so those fences stay intact end-to-end.

Happy to adjust the scope if you’d prefer this PR to stay shell-only and leave the plain-text fence contract to #49972 / #57540. LMK what you think 😄

# Conflicts:
#	apps/desktop/src/lib/markdown-code.test.ts
#	apps/desktop/src/lib/markdown-code.ts
@danspicytaco

Copy link
Copy Markdown
Author

Hi @alt-glitch, bumping this for your review. It preserves explicit text/plain/plaintext fences and shell aliases as code blocks while retaining the newer structured-text protections. The focused Markdown checks, full Desktop UI and platform suites, typecheck, ESLint and build pass. Since you previously flagged the overlapping fence-rendering work, could you also advise whether this should be consolidated with #49972, #57542 or #77258?

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/*) needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants