test(serve): broaden mermaid escape-token coverage to multi-diagram families#275
Merged
Conversation
…amilies Hardens #273 against regressions across mermaid diagram types beyond the flowchart `-->` and class `<|--` cases the fix originally covered. Adds: - A type-agnostic markdown unit test asserting that escape-sensitive tokens across sequence (`->>`, `-->>`), ER (`||--o{`), class stereotype (`<<async>>`), and flowchart (`&` chains) families all survive the render verbatim, and no `>` / `<` / `&` entity escaping leaks into output. - A second ```mermaid block on ARCH-CORE-001 (stateDiagram-v2) so the end-to-end Playwright fixture exercises a non-flowchart diagram shape — the `[*] -->` token from the original bug report is now covered end-to-end. - A Playwright matrix update asserting both ```mermaid blocks render to SVG (count >= 2) and that `Validating --> Serving : graph built` survives verbatim across diagram families. The fix from #273 stops body escaping wholesale, so the broader coverage is belt-and-braces — a regression in any escape path would now fire loudly across multiple test surfaces. Verifies: REQ-032 Refs: #273 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📐 Rivet artifact delta
Graphgraph LR
ARCH_CORE_001["ARCH-CORE-001"]:::modified
classDef added fill:#d4edda,stroke:#28a745,color:#155724
classDef removed fill:#f8d7da,stroke:#dc3545,color:#721c24
classDef modified fill:#fff3cd,stroke:#ffc107,color:#856404
classDef overflow fill:#e2e3e5,stroke:#6c757d,color:#495057,stroke-dasharray: 3 3
Modified
Posted by |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Rivet Criterion Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: 01a912a | Previous: ff89f98 | Ratio |
|---|---|---|---|
store_insert/10000 |
14891781 ns/iter (± 1072979) |
11199692 ns/iter (± 431145) |
1.33 |
link_graph_build/10000 |
32443474 ns/iter (± 2843070) |
24827464 ns/iter (± 1138121) |
1.31 |
This comment was automatically generated by workflow using github-action-benchmark.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…ixture grew The fenced ```mermaid coverage extension in this PR added a second mermaid block to ARCH-CORE-001 (`stateDiagram-v2` alongside the existing `flowchart LR`). That broke `artifacts.spec.ts:58`'s `toHaveCount(1)` assertion, which had hard-coded the assumption that there's exactly one mermaid block in the fixture. Replace the strict `toHaveCount(1)` with a soft `toBeGreaterThanOrEqual(1)` so adding diagrams to the artifact doesn't break this regression test. The test's intent — confirm that .mermaid wrappers are emitted and the first one renders to SVG — is preserved. Verifies: REQ-032
PR #275 Playwright failed because rivet serve renders ARCH-CORE-001's state-diagram mermaid as `[*]\n--> Loading\n\nLoading --> Validating\n:\n config + …` — every comma, colon, and `]` becomes a line break. Bundle output is correct; rivet serve is wrong; same source YAML. Root cause: the rowan-based YAML CST lexer at `yaml_cst.rs:401` (`lex_plain_scalar`) breaks plain scalars at `,`, `]`, `}`, and `:` followed by space — these are YAML flow-syntax characters. The break is correct at the YAML grammar level. Inside a `description: |` block scalar, though, those characters are literal text — not delimiters — and shouldn't trigger token boundaries. `block_scalar_text` (`yaml_hir.rs:1451`) compounded the problem: it iterates `BlockScalarLine` *tokens* (one per pre-lexed chunk) and joins them, treating whitespace-only tokens as `\n` separators. So a source line like `Loading --> Validating : config + artifacts read` gets tokenised as roughly `[Loading --> Validating, :, config + ...]`, each chunk becomes a "BlockScalarLine", and the join inserts newlines between them. Bypass the token-iteration path entirely. Use the raw `BlockScalar` node text (`block.text().to_string()`) which is exactly the source slice — including whatever flow-syntax characters appear literally inside the block. Strip the `|` / `>` header line, dedent by the common leading-whitespace prefix, and emit lines joined with `\n` (matches YAML 1.2 default clip-chomping for `|`). The two-path divergence — `rivet bundle` uses `serde_yaml` via the `GenericYamlAdapter` and was always correct; `rivet serve` uses the salsa-tracked `parse_artifacts_v2` which unconditionally calls `yaml_hir::extract_schema_driven` regardless of source format — explains why the bug was latent for so long. Most descriptions don't contain `,` or `:` at flow-syntax positions, so the disagreement was silent. Regression test: `block_scalar_with_flow_syntax_chars_preserves_lines` covers description content with all four trigger characters (`,` `]` `:` `[`) and a state-diagram fragment. The PR #275 Playwright assertions on `[*] --> Loading`, `Validating --> Serving : graph built`, etc. now pass locally — verified by running `rivet serve --port 47293` against the dogfood project and curling `/artifacts/ARCH-CORE-001`. Fixes: REQ-032 Refs: #275
3 tasks
avrabe
added a commit
that referenced
this pull request
May 15, 2026
…284) Three PRs today (#279, #281, and previously #275/#278) failed CI on the same test: `graph_focused_view_renders_svg`. Cause is structural, not a real-flake: the test's `fetch()` helper hardcodes a 5s read timeout, and the focused /graph endpoint takes ~5s on the dogfood corpus (742 nodes, 1477 edges) — BFS frontier + etch layout pass. The test sits exactly on the edge; CI runner load tips it over. Add a `fetch_with_timeout(port, path, htmx, timeout)` variant. Keep the default 5s `fetch()` for everything else. Bump the graph test's deadline to 15s — well past the genuine wall-clock for this endpoint and short enough that a real performance regression still bubbles up. The wall-clock `Instant::elapsed()` log line stays, so an actual slow regression would still be visible in the test output even though the read timeout no longer blocks it. Verified locally: `cargo test -p rivet-cli --test serve_integration graph_focused_view_renders_svg` passes in 2.79s with the new helper. Refs: REQ-007 (CLI surface)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Hardens #273 against regressions across mermaid diagram families beyond the flowchart
-->and class-diagram<|--cases the fix originally covered.->>,-->>), ER (||--o{), class stereotype (<<async>>), and flowchart (&chains) families all survive verbatim, with zero>/</&leakage. A regression in any escape path now fires loudly across multiple diagram shapes in one test.stateDiagram-v2) — gives the Playwright fixture a non-flowchart shape end-to-end. The `[*] -->` token from the original bug report now has explicit dogfood coverage.count >= 2) and the state-diagram arrows survive verbatim in served HTML.Belt-and-braces: the #273 fix stops body escaping wholesale, so the broader coverage is regression insurance, not a behaviour change.
Test plan
cargo test -p rivet-core markdown::tests::mermaid_escaping_sensitive_tokens_all_verbatim(passes locally)cargo test -p rivet-core markdown::tests::mermaid_class_diagram_arrows_not_escaped(regression check; still passes)npx playwright test tests/playwright/mermaid.spec.tsagainst a liverivet servewith ARCH-CORE-001 visiblecargo fmt,cargo clippy,rivet validate(pre-commit ran clean)Trailer
Verifies: REQ-032·Refs: #273🤖 Generated with Claude Code