feat(import): add Mermaid flowchart importer for typed architecture IR - #140
feat(import): add Mermaid flowchart importer for typed architecture IR#140santhiprakash wants to merge 15 commits into
Conversation
80a02ea to
fedb28e
Compare
FenjuFu
left a comment
There was a problem hiding this comment.
I reproduced four cases where the importer returns a successful result while changing or inventing Mermaid semantics. The focused suite passes (node --test test/flowchart-import.test.mjs: 17/17), but issue #92 requires direction/grouping/text to be preserved and unsupported or ambiguous syntax to produce a stable diagnostic instead of being silently dropped or invented.
The inline comments cover: RL/BT direction being laid out as LR/TD, later explicit node declarations losing their labels, subgraph direction creating fake components, and Mermaid's normal --- link being emitted as dashed. These need regression tests alongside the fixes.
There is also no user-facing documentation or runnable example in this PR for the supported subset and target-mode selection, which is an explicit acceptance item in #92. Please add that documentation.
Finally, the PR is currently behind main and GitHub reports no checks for the head commit. Please update it against current main and run the repository-required checks before the next review. No remote DCO check is reported for this commit.
| { re: /^-\.\.->/, variant: 'dashed' }, | ||
| { re: /^-\.->/, variant: 'dashed' }, | ||
| { re: /^-->/, variant: 'solid' }, | ||
| { re: /^---/, variant: 'dashed' }, |
There was a problem hiding this comment.
P1: This maps Mermaid's normal open link --- to Archify variant dashed. Mermaid distinguishes a normal --- link from dotted -.- / -.-> links (see https://mermaid.js.org/syntax/flowchart.html). Repro: flowchart LR\n A[One] --- B[Two] currently returns a directed dashed connection. Please preserve the supported semantics (including the absence of an arrow), or reject this syntax with a stable unsupported diagnostic; the current result silently changes it.
| continue; | ||
| } | ||
|
|
||
| // Check for unsupported keywords. |
There was a problem hiding this comment.
P1: Handle Mermaid's direction directive inside a subgraph, or return a stable unsupported diagnostic. Repro: flowchart LR\n subgraph API\n direction TB\n A[One] --> B[Two]\n end succeeds but invents components named direction and TB and adds them to the boundary. This violates #92's requirement not to invent or silently drop content.
|
|
||
| // Register components. | ||
| for (const comp of stmtResult.components) { | ||
| if (!components.has(comp.id)) { |
There was a problem hiding this comment.
P1: First occurrence wins here, so a later explicit Mermaid node declaration silently loses its text/type. Repro: flowchart LR\n A --> B\n A[Named source]\n B[Named target] returns labels A and B. Mermaid permits a node to be defined more than once and uses the latest text. Please update/merge explicit declarations (and diagnose genuinely conflicting ambiguous declarations) and add a regression test.
| const id = layerIds[i]; | ||
| if (isHorizontal) { | ||
| // LR/RL: depth = column, index within layer = row. | ||
| const x = ORIGIN_X + d * (CELL_W + GAP_X); |
There was a problem hiding this comment.
P1: The layout only distinguishes horizontal from vertical, so accepted RL and BT declarations are rendered in the opposite direction. Repro: flowchart RL\n A[Source] --> B[Target] gives A x=40, B x=260; flowchart BT gives A y=40, B y=180, identical to LR/TD. Reverse depth placement for RL/BT, or reject those declarations until supported, and cover both with tests.
|
Thank you for the careful reproduction — all four cases confirmed against 1. 2. Subgraph 3. First-occurrence-wins declarations — confirmed. A later explicit declaration now updates the earlier implicit one (latest text wins, Mermaid-compatible); two different explicit declarations for the same id exit non-zero with 4. RL/BT rendered as LR/TD — confirmed: Documentation — added Branch and checks — merged current Verification
|
- Problem: Archify could not import existing Mermaid flowchart/graph diagrams; users had to re-author topology by hand. - Fix: Add a focused Mermaid flowchart parser (archify/importers/flowchart.mjs) that maps a documented subset of flowchart syntax to typed architecture IR, with auto-layout, stable diagnostics for unsupported/malformed syntax, and a new 'archify import flowchart' CLI command. - Verification: npm test in archify/ — 751 tests, 730 pass, 0 fail, 21 skipped (Chrome-dependent). Full import→validate→render pipeline verified on all valid fixtures. Closes tt-a1i#92
…ections - Problem: Reviewer FenjuFu reproduced four cases where the flowchart importer silently changed or invented Mermaid semantics: open link --- became a dashed directed edge, subgraph direction invented components named direction/TB, later explicit node declarations lost their labels, and RL/BT diagrams laid out identically to LR/TD. Imported IR with edge labels could also fail showcase layout validation (labels biased into the target component), against issue tt-a1i#92's acceptance criterion that imported IR passes the existing quality gates. - Fix: Reject open links and the direction directive with stable unsupported diagnostics (import/unsupported-edge-syntax, import/unsupported-direction-directive); apply later explicit declarations over implicit ones and diagnose conflicting explicit redeclarations (import/flowchart-conflicting-node-declaration); mirror depth placement for RL/BT; compensate the Viewer's source-anchored straight-route labels only where needed (vertical half-cell shift, horizontal centered) so every valid fixture passes showcase validation; document the supported subset, target-mode selection, and diagnostic codes in references/mermaid-flowchart-import.md linked from SKILL.md. - Verification: node --test test/flowchart-import.test.mjs — 26/26 pass (8 new; sabotage run first showed the 7 behavioral tests failing on the original head). npm test — 776 tests, 746 pass, 0 fail, 30 skipped (Chrome-dependent). All 8 valid fixtures now pass import → validate --quality showcase; the labeled-edges and labeled-subgraph fixtures failed showcase before the label fix.
a5ca7af to
cf6a8ca
Compare
|
Rebased the branch onto current I also updated the PR body to reflect the current supported edge forms and verification numbers. Verification (re-run on the rebased head):
No other changes were introduced during the rebase. |
FenjuFu
left a comment
There was a problem hiding this comment.
Thank you for addressing the original four findings. I re-ran the focused suite on cf6a8ca (26/26 pass), confirmed the packaged importer/CLI/SKILL/reference match the tracked content after line-ending normalization, and verified the new RL/BT and label-layout cases. Three contract blockers remain:
-
Nested subgraphs produce IR that cannot pass the required schema/quality gate (
archify/importers/flowchart.mjs:198, contract atarchify/references/mermaid-flowchart-import.md:79). Membership is added only tosubgraphStack[subgraphStack.length - 1]. Minimal input:Loadingflowchart TD subgraph Outer subgraph Inner A[Node] end end
parseFlowchartreturns success withOuter.wraps: []andInner.wraps: [A];validate architecture --quality showcase --jsonthen fails withschema/minItemson/boundaries/0/wraps. This contradicts both the documented “Nested subgraphs are tracked” statement and #92’s requirement that supported imports pass existing gates. Please either represent nested membership in valid IR or reject nested subgraphs with a stable unsupported diagnostic and narrow the contract, plus add an import→showcase regression. -
The explicit-redeclaration fix is still bypassed within one statement (
archify/importers/flowchart.mjs:379-381and:415-417).parseStatementde-duplicates its localcomponentsarray by id before the global explicit/implicit precedence logic sees the later node. Consequently:A --> A[Label]succeeds but keeps labelAinstead ofLabel.A[One] --> A[Two]succeeds withOneinstead of returningimport/flowchart-conflicting-node-declaration.
This is the same silent first-occurrence behavior the previous review requested to remove. Please preserve the later declaration (or diagnose the conflict) even when both occurrences are in one chain, with regression tests for both cases.
-
The documented dotted open-link contract disagrees with the parser and Mermaid semantics (
archify/references/mermaid-flowchart-import.md:67). The table says both-.-and-.->become a directed dashed connection. Mermaid defines-.-as a dotted link without an arrowhead and-.->as the dotted link with an arrowhead: https://mermaid.js.org/syntax/flowchart#minimum-length-of-a-link. The current parser rejectsA -.- B, but with the unrelatedimport/flowchart-invalid-node-iddiagnostic. Since Archify cannot preserve an open link, this should be aligned with the---handling: reject it with the stable unsupported-edge diagnostic and document it as unsupported (or otherwise preserve its no-arrow semantics).
Please add these cases to the fixture-level import→validation coverage. I am not treating my Windows full-suite timeout as a test failure; the focused suite is green. GitHub still reports no checks on the current head, so a maintainer will also need to approve/run the repository workflows before final review.
…redeclarations faithfully - Problem: a node inside nested Mermaid subgraphs was recorded only in the innermost boundary, so outer boundaries shipped with empty wraps lists and failed the showcase schema gate (boundaries[].wraps minItems). Explicit redeclarations inside a single statement (A --> A[Label], A[One] --> A[Two]) were silently dropped in favor of the first occurrence. The contract documented dotted open links (-.-) as directed dashed edges while the parser rejected them with import/flowchart-invalid-node-id. - Fix: record nested membership in every enclosing boundary; merge same-statement occurrences with the cross-statement precedence rules (later explicit wins, conflicting explicit definitions diagnosed); reject -.- / -..- with the stable import/unsupported-edge-syntax diagnostic and correct the contract table. - Verification: node --test test/flowchart-import.test.mjs 31/31 pass; sabotage run confirms the 5 new tests fail on pre-fix code; CLI import->showcase repro of all three review cases; archify.zip rebuilt with Node 22 (canonical toolchain).
|
All three contract blockers are addressed on the pushed head 1. Nested subgraphs emitted an empty parent 2. Same-statement redeclarations bypassed the precedence rules. 3. The dotted open link Fixture-level coverage: new Verification (all on
The branch is based on current |
…ranch - No source conflicts: archify/SKILL.md and archify/bin/archify.mjs auto-merged (their update-awareness additions vs our import-contract edits are disjoint). - archify.zip regenerated from the merged tree with Node 22 (deterministic build) to resolve the binary conflict. - Verification: flowchart suite 31/31; npm test 896 tests / 865 pass / 0 fail / 31 skipped (Chrome-dependent); check-release-identity ok.
…art-import # Conflicts: # archify.zip # archify/bin/archify.mjs
tt-a1i
left a comment
There was a problem hiding this comment.
Reviewed current head433a0bfbf3f10a2997eb162f8afa439148421702. Thanks for addressing the previous nested-membership/redeclaration/open-link review: those cases now pass, as do all31 importer tests, and all78 ZIP payload files match. Additional public-CLI cases still need fixes.
Standards / output safety
- [P1] archify/bin/archify.mjs:1990-1992 writes without the shared input-alias guard.
import flowchart diagram.mmd diagram.mmd --jsonexits0/ok:true and replaces the user's Mermaid source with JSON. Reject same-path/symlink/hard-link aliases before commit and preserve the source. - [P2] The same write path leaves --json stdout empty and throws a raw EISDIR stack when the output is a directory. Return a stable diagnostic receipt for output preparation/write failures.
Spec / topology and valid output
- [P2] flowchart.mjs:91-105 ignores declaration-line remainder:
flowchart LR; A[Lost] --> B[Lost]followed byC[Kept]imports successfully with only C and zero edges. Parse the remainder or reject it explicitly rather than dropping topology. - [P2] :138-145 turns a subgraph endpoint into a new backend component:
subgraph Group,A[Inside],end,B[Outside] --> Groupproduces a fictitious Group service plus the Group boundary and passes showcase. Model supported grouping faithfully or reject the unsupported endpoint. - [P2] :632-641 fixes every box to140px.
A[Customer subscription management service] --> B[Backend]imports ok but fails the advertised validation handoff (approximately264px label). Measure preserved labels and size/space the output accordingly. - [P2] :326-330 emits wraps:[] for an empty subgraph and overwrites output with ok:true even though the resulting IR fails schema/minItems. Diagnose unrepresentable empty groups before writing the last valid output.
These are new reproductions on this head, separate from the resolved previous findings. No full-suite/browser acceptance claimed; no source edits or merge.
- Problem: import could overwrite the Mermaid source via same-path/ symlink/hard-link output aliases, crashed with a raw EISDIR stack on a directory output with no JSON receipt, silently dropped statement topology after 'flowchart LR;', invented a component when an edge named a subgraph, fixed every cell at 140px so long labels failed the advertised validation handoff, and emitted empty-subgraph wraps:[] that violates schema minItems while reporting ok:true. - Fix: reject aliased outputs before writing (realpath + dev/ino identity), emit a stable output/write receipt for write failures, reject declaration-line remainder and subgraph endpoints with named diagnostics, size cells from the validator's own label measurement (textUnits*6.6) with width-aware column/row strides, and reject empty subgraphs at 'end'. - Verification: sabotage-first — 8 new tests fail on 433a0bf, pass on this head; flowchart-import 40/40; full suite 1053 tests, 1022 pass, 0 fail, 31 skipped.
…owchart-import; rebuild archify.zip canonically on Node 22.14.0
|
Pushed fixes for all six findings on Standards / output safety
Spec / topology and valid output
Verification on
|
tt-a1i
left a comment
There was a problem hiding this comment.
Review fixed to head 0a8b4973823e172510087c2f8fb836cc0e307019 against current main 199360cc6687a7857b54dd188d4922b09e466a4b.
Requesting changes for three confirmed contract gaps:
-
[P1] Commit the import output without a source-overwrite race.
commandImportchecks input/output aliasing once before parsing, then later followsoutputPathwithwriteFileSync. With a 100,000-node input, changing an initially safe output symlink to point at the Mermaid input during parsing made the command exit 0 withok: truewhile replacing the source with JSON (inputPreserved: false). This breaks the source-preservation contract documented at lines 1935–1937 and asserted by the current alias tests. Use a non-following atomic candidate/rename path and recheck identity at the commit point; add a race regression. -
[P1] Track actual Mermaid subgraph identity instead of labels plus synthetic
sgNnames. Forsubgraph G [Group Label] ... endfollowed byB --> G, the importer returnsok: true, invents a third ordinary component{id:"G", label:"G"}, and emits a boundary labeledG [Group Label]; showcase validation then passes the corrupted topology. Conversely, after any subgraph, a legitimate node namedsg1is rejected as a subgraph endpoint because lines 317–318 reserve synthetic counter names that Mermaid never reserved. Parse/store the authored subgraph id and title separately, and reject or faithfully map edges using only authored identities. -
[P2] Ensure every successful supported import can pass the advertised validation handoff. A supported labeled edge such as
A[Alpha] -->|This is an extremely long relationship label that is likely wider than the available route gap| B[Beta]imports with exit 0/ok: true, but immediatevalidate architecture --quality showcase --jsonexits 1 because the label overlaps both components;deliveralso fails. The current layout expands cells only for node labels and keeps an 80px relationship gap. A small supported cycle (A→B→C→B) likewise imports successfully but fails validation withclean-flow/edge-through-node. Either generate gate-valid geometry for these supported topologies or reject them during import with stable source diagnostics; add import→validate regressions for both.
Evidence on the synthesized current-main integration: merge completed without conflicts; git diff --check passed; focused importer/CLI tests passed 81/81; full npm test passed 1,030 with 31 environment-dependent skips and 0 failures; staged skill vs archify.zip matched byte-for-byte across all 78 packaged files. These green tests do not cover the three reproductions above. Remote CI has not run on this head.
- Problem: the import write path re-checked input/output aliasing only
before parsing, so an output symlink swapped mid-parse made the CLI
exit 0 while replacing the Mermaid source with the import result;
authored subgraph ids ("subgraph G [Group Label]") were not tracked,
so edges to G invented a phantom component, boundary labels carried
raw declaration text, and synthetic sgN names wrongly reserved
legitimate node ids; straight horizontal routes with labels wider
than the route gap and small cycles imported ok but failed the
advertised validate --quality showcase handoff.
- Fix: commit the import output through a non-following O_EXCL
candidate/rename with an alias recheck at the commit point; parse and
store authored subgraph id/title separately and reject subgraph-edge
endpoints on authored identities only (sgN is no longer reserved;
an explicit node declaration sharing a subgraph identity keeps the
node); move over-wide horizontal edge labels below the route using
the validator's own textUnits measurement and make layer assignment
first-assignment-wins so cycles no longer strand a node under a
straight route.
- Verification: sabotage runs fail pre-fix (1 CLI race test; 11
importer tests); flowchart-import suite 57/57; full npm test 1074
tests / 1043 pass / 0 fail / 31 env-skips (one update-notifier timing
flake, green 4/4 on rerun); archify.zip byte-identical to a canonical
Node 22.14.0 rebuild (78 files).
|
Thanks for the three confirmed reproductions — all three are fixed on head 1. Source-overwrite race in the import write path (P1). Reproduced on 2. Authored subgraph identity (P1). Reproduced on 3. Import→validate handoff (P2). Both reproductions confirmed on Verification on |
…t-import # Conflicts: # archify.zip
…t-import # Conflicts: # archify.zip
…/mermaid-flowchart-import; rebuild archify.zip canonically on Node 22.14.0
…ermaid-flowchart-import - Problem: upstream tt-a1i#299 (3c42a59) rewrote archify.zip, conflicting with the PR's tracked zip (4th occurrence of the recurring binary-zip conflict class). - Fix: merged origin/main; rebuilt archify.zip canonically on Node 22.14.0 (two builds byte-identical, sha256 84bab05dcaaf133f...) and staged it. - Verification: full suite on merged tree 1089 tests / 1058 pass / 0 fail / 31 skipped; focused flowchart-import 57/57.
…HTML output guard) into feat/mermaid-flowchart-import - Problem: upstream main rewrote archify.zip (tt-a1i#321 rebuild, tt-a1i#322 output-path extension guard) and touched renderers/shared/output-path.mjs, conflicting with the flowchart-import PR head d1626ee. - Fix: merged origin/main; auto-merge kept the disjoint regions (the CLI .html extension guard inside resolveOutputPath vs the import alias/commit helpers appended after it — the import output path does not route through resolveOutputPath); archify.zip rebuilt canonically on Node 22.14.0, byte-identical across two runs. - Verification: focused flowchart-import 57/57; full suite on the merged tree 1107 tests / 1079 pass / 27 skipped with the single failure being upstream's update-notifier concurrency flake (reproduced with the same signature on a pristine origin/main control run).
…t-import # Conflicts: # archify.zip # archify/bin/archify.mjs
Problem and value
Archify could not import existing Mermaid
flowchart/graphdiagrams. Users had to re-author topology by hand even when a Mermaid source already existed. Issue #92 asks for an end-to-end import path that maps a documented Mermaid subset to typed Archify architecture IR, validates it through the existing gates, and delivers it as a standalone artifact.Scope
archify/importers/flowchart.mjs— a focused Mermaid flowchart parser that maps a documented subset offlowchart/graphsyntax to typed architecture IR. Supported: direction declarations (TB/TD,BT,LR,RL), node shapes ([...],(...),((...)),[(...)],{...},>...]), directed edges (-->,-.->,==>,-- text -->,-. Text .->,|label|), subgraphs, comments, and chained edges. Node text, edge labels, subgraph grouping, and mirroredRL/BTplacement are preserved.archify import flowchart <input.mmd> [output.json] [--json]CLI command. Without--json, the IR is written to the output file (or stdout). With--json, a machine-readable receipt is emitted on stdout.archify/test/flowchart-import.test.mjswith 26 regression tests covering valid, malformed, unsupported, adversarial, and showcase-layout fixtures.archify/test/fixtures/flowchart/, including 8 valid fixtures.archify/references/mermaid-flowchart-import.mddocumenting the supported subset, target-mode selection, shape/edge mapping, and diagnostic-code table; linked fromarchify/SKILL.md§ Mermaid input.importcommand uses a lazy dynamic import so it does not affectdoctoror other commands in incomplete installations.Stability impact
import/flowchart-*orimport/unsupported-*codes) and source location. Open links (---/ long-arrow forms) and subgraphdirectiondirectives are explicitly rejected. No node or edge is silently discarded. The--jsonreceipt includes the fulldiagnostics[]array. Rollback: remove theimporters/directory and revertbin/archify.mjs.Tests run
Result: 776 tests, 746 pass, 0 fail, 30 skipped (Chrome-dependent browser tests, skipped because
ARCHIFY_CHROMEis not set). Duration: ~97s.Targeted flowchart import tests:
Result: 26 tests, 26 pass, 0 fail, 0 skipped. Duration: ~2.9s.
Full
import flowchart→validate architecture --quality showcasepipeline verified on all 8 valid fixtures:Result: all 8 valid fixtures (
simple,subgraph,labeled-edges,labeled-subgraph,chained,redeclared-labels,direction-rl,direction-bt) pass the full pipeline.Visual evidence
visual review: skipped — Chrome is not available in this environment to inspect the rendered HTML visually. The rendered HTML files were generated successfully and pass all non-visual checks (single SVG, finite coordinates, orthogonal arrows, label clearance). A maintainer with Chrome available can run
ARCHIFY_CHROME=/path/to/chrome node --test test/desktop-reader-browser.test.mjsor open the rendered HTML to confirm visual quality.Generated artifacts
archify.zipwas rebuilt with Node 22 and byte-verified locally because the package contents (importers/flowchart.mjs,bin/archify.mjs,SKILL.md, and the new reference) changed. No other generated artifacts (Gallery, guides, README proofs) were touched.Checklist
npm testinarchify/.Closes #92