fix(render): self-relationships in-box, ownership as line style - #28
Conversation
PR #28 — review round 1Full round per Angles run: adversarial runner (Opus, 21 fixtures, base vs branch), VerdictThe core fix works — self-loops are gone, zero Mermaid parse errors across 21 BlockingB1 — the ownership fold is broken; redesign it.
Settled design (user decision): fold only a genuine reciprocal — same pair, B2 — self rows drop the declaring-side cardinality. B3 — duplicate identical self-relationships render twice ( Non-blocking
Out of scope — filed separately
Verified cleanDeterminism (25- and 60-run hashes); |
PR #28 — round 1 fixesEvery fix below was demonstrated by running the named fixture under B1 — the ownership fold, redesigned. FIXEDThe fold predicate now lives in
Everything else draws both lines. Two new lint errors in
Before / after
Pinned by Docs corrected to match
B2 — self rows dropped the declaring-side cardinality. FIXED
Pinned by B3 — duplicate identical self-relationships rendered twice. FIXED
N1 — prose-role heuristic, both directions. FIXED
N2 —
|
Round-1 review fixes for #28. The ownership fold was wrong in four directions: it fired on two declarations from the same end that differed only in `ownership` (one edge silently vanished), it swallowed mutual `owned` with no diagnostic, it left a reciprocal pair with differing roles drawing one solid and one dashed line, and `ER()`'s comment plus ADR-0008 asserted a guarantee the code did not deliver. Two declarations now merge in exactly two cases: an exact duplicate from the same end, and a genuine reciprocal (opposite ends, inverse cardinality, same label, at most one end claiming `owned`). Everything else draws both lines, so no declaration disappears. The contradictions the renderer refuses to swallow become lint errors: mutual `owned`, and a reciprocal pair whose ends disagree on ownership under different roles. Also: - Self-relationship rows carry the declared cardinality whole, not the substring after the colon, so `0..5:1` no longer renders as `1`. - Identical self-relationship declarations render one row, not two. - The prose-role heuristic judges the role as the diagram draws it: length, word count, `;` or a trailing `.` — no longer any comma or full stop, which false-positived on role lists, accessors and version numbers, and no longer blind to a 66-character single word. - A role raises at most one finding; the undefined-term check waits until the role is no longer prose. - `sanitize` drops backslashes, so `%q` cannot double-escape them. - ADR-0008, `docs/04`, `docs/06` and the plugin skills describe what the code actually does; the `docs/04` snippets match real CLI output. - `TestProseRoleIsWarning` renamed to `TestADR_0008_ProseRoleIsWarning`; the cosmetic `Symmetric` render case is replaced by real coverage of the Markdown marker. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Joe Beda <joe@stacklok.com>
Round 2 — blocking fix on the round-1 fixThe coordinator was right, and my open question #2 was the cause of open 1. Label equality removed from the fold predicateThe edge key is now One further guard: an edge folds in at most one opposite-end declaration 2. Deterministic label choice on a foldNew The Markdown keeps both roles — verified, not assumed. Rendering Each entity's own relationship list is rendered independently of the fold, so 3. "Reciprocal ownership disagreement" error deletedGone from
4. Regression guard on
|
| round 1 | round 2 | |
|---|---|---|
| diagram | Alpha ||--o{ Beta : "part" and Beta }o..|| Alpha : "whole" (solid + dashed for one relationship) |
Alpha ||--o{ Beta : "part" — one solid line |
| lint | 1 error(s) — reciprocal ownership conflict |
0 error(s) |
| exit code | 1 | 0 |
mermaid-cli 11 re-render (own-roles.png): a single solid line labelled part.
The five other fixtures, re-run — all unchanged from round 1
| fixture | behavior | verdict |
|---|---|---|
f12-own-only-difference |
2 edges (||--o{ and ||..o{), 0 errors |
still correct — nothing dropped |
f13-own-diff-samerole |
2 edges, same role on both | still correct |
f14-self-dup |
1 self row | still correct |
f17-mutual-owned |
2 solid edges + mutual-ownership error | still correct |
f21-self-variants |
1:n, 1:2 — Peer, 0..5:1 owned, n:n — Sibling |
still correct |
correctness/go3 |
2 edges (same end, ownership differs) | still correct |
correctness/own-conflict |
1 solid edge | still correct |
f07-dedupe-match |
2 solid edges + mutual-ownership error | still correct |
| golden example | render-check clean, no regeneration needed |
unchanged |
All six re-rendered through mermaid-cli 11: parse clean, no runaway arcs.
Docs re-corrected
ADR-0008 Decision 2 rewritten again (roles explicitly not in the predicate,
the label-choice rule, the Markdown-keeps-both note, one lint error not two);
its Consequences paragraph rewritten. docs/04 "Ownership belongs to the
relationship" section and the worked-example bullet; docs/06 fold bullet and
error list; both plugin skills.
Verification
task checkgreen; goldens unchanged (render-checkclean).- Determinism: 30 renders × 7 models hashed as one stream, twice —
2b3bdc09…8cda3both times. - mermaid-cli 11 on
own-roles,f12,f13,f14,f17,f21: all parse.
Open questions after round 2
- Round-1 open questions release.yml: guard that releases only cut from main #1 and Migrate .goreleaser.yaml from deprecated brews to homebrew_casks #2 are both resolved and withdrawn. No
legitimate model reaches an ownership error now, soerroris the right
severity for mutualowned. - New, minor: the
foldedFromone-reciprocal-per-edge guard is my own
addition, not in the settled design. Without it, a pair with one declaration
one way and two the other would merge all three into one line and silently
drop a role. It mirrorsrunReciprocity's "exactly one declaration in each
direction" gate, so I believe it is right, but it is a judgment call made
without asking.
PR #28 — review round 3 (final round; cap reached with an open blocker)Delta-only round over Verdict: NOT CLEAN — do not mergeRound 2's correction is right as far as it goes, and I re-verified it by hand: But the B4 — CONFIRMED, blockingFixtures
Three declarations in, two edges out. Which role disappears depends purely on Root cause: the renderer's fold key has no "exactly one declaration per This falsifies ADR-0008's "no declaration disappears" claim — the same class of Output is byte-stable for a fixed input, so the golden test cannot catch this. Suggested fix (not applied — see cap below)Gate the fold: if either side has two or more declarations sharing a fold key, Verified clean this roundMarkdown never folds, so no declaration can be lost there (checked Round cap
|
Round 4 — the ambiguity gateRound 3 was right and my round-2 open question was well-founded. The The fix1. Ambiguity gate, replacing 2. One definition, two consumers. The grouping now lives in 3. New lint warning ( It points at the first declaration on the crowded end. It fires on no 4. ADR-0008 corrected. The "no declaration disappears" sentence round 3 Before / after —
|
| round 2 | round 4 | |
|---|---|---|
| diagram | Policy }o--|| Project : "parent", Project ||..o{ Policy : "overrides" — defaults gone |
parent solid, defaults dashed, overrides dashed — all three |
| swap Project's two declarations | overrides gone instead |
same three lines, listed in the new source order |
| lint | 0 error(s), no diagnostic |
0 error(s), ambiguous-pairing warning |
Order pairs
| pair | edge set | line order |
|---|---|---|
order-a / order-b |
identical (P solid, Q dashed, R dashed) |
follows source |
order-c1 / order-c2 |
identical (P dashed, Q dashed, R solid) |
follows source |
order-d1 / order-d2 |
identical (P solid, R1 dashed, R2 dashed) |
follows source |
On "byte-identical", plainly: it is not, and I did not make it so. The
set of lines is identical for every pair — same labels, same styles, nothing
dropped, which is the property round 3 broke. What still varies is the order the
lines are listed in, because edges are emitted in declaration order
(entity name, then position). Making the bytes identical needs the emitted edge
list sorted, which would reorder the committed golden diagrams
(Account ||--o{ Keycard before Account ||--o{ Car becomes the reverse) and
would discard the author's ordering. That is a separate rendering decision, so
I stopped and am asking rather than doing it. TestERAmbiguousPairingIsOrderIndependent
asserts the sorted line sets match across all three swaps.
Fixtures re-run — no regressions
| fixture | round 4 | expected |
|---|---|---|
correctness/own-roles |
1 solid edge "part", 0 errors |
✅ unchanged |
f12-own-only-difference |
2 edges (||--o{, ||..o{) |
✅ unchanged |
f17-mutual-owned |
2 solid edges + mutual-ownership error | ✅ unchanged |
f14-self-dup |
1 self row | ✅ unchanged |
f21-self-variants |
1:n, 1:2 — Peer, 0..5:1 owned, n:n — Sibling |
✅ unchanged |
f07, own-mutual |
2 solid + error | ✅ unchanged |
f08, own-conflict |
1 solid edge (genuine reciprocal) | ✅ unchanged |
go1, f09 |
2 edges + cardinality error | ✅ unchanged |
go3, f10, f11, f13 |
2 edges each | ✅ unchanged |
all f01–f05, f19–f21, self-* |
self rows unchanged | ✅ unchanged |
| golden example, garage | render-check clean, no regeneration |
✅ unchanged |
Every other fixture's output is byte-identical to round 2. Nothing absorbed.
New tests
TestADR_0008_AmbiguousPairingKeepsEveryDeclaration— the realistic model
draws exactly the three expected lines (full equality on the sorted line set,
not a count).TestERAmbiguousPairingIsOrderIndependent— three-case table, each built
twice with the two declarations swapped; asserts the sorted line sets match
and that every declaration drew. This is the shape
TestERFoldsOneReciprocalPerEdgeshould have had; that test is gone, since
the guard it pinned is gone.TestADR_0008_AmbiguousPairingIsWarning— six cases: fires two-forward-one-back
and one-forward-two-back (asserting severity, category and path); silent for
two-forward-none-back, one-each-way, two-forward-at-different-cardinalities,
and a second forward declaration that is an exact duplicate.
Verification
task checkgreen; goldens untouched (render-checkclean).go test -race -count=5 ./internal/...green.- Determinism: 30 renders × 10 models hashed as one stream, twice —
2936068f…f1028both times. - All 38 generated diagrams (every fixture plus both shipped examples)
rendered throughnpx -y @mermaid-js/mermaid-cli@11: 38/38 parse, zero
errors. PNGs in.scratch/reviews/mmd-round4/.realistic.pngshows all
three lines with all three roles.
Open questions
- Byte-identical output across a source reorder would need the emitted
edge list sorted. See above — not done, needs a call. - Exact duplicates are deduped before the ambiguity count, which the
round-4 brief did not spell out. Two identical forward declarations plus one
back therefore still folds normally, rather than tripping the gate. That is
deliberate: an exact duplicate is indistinguishable, so no pairing question
arises, and counting it would regress the round-1 B3 dedupe. Pinned by the
last case ofTestADR_0008_AmbiguousPairingIsWarning. Flagging it because it
is a refinement of the stated rule, not the stated rule. - The warning is per fold key, not per pair.
Projectdeclaring two1:n
relationships toPolicyplus onen:1back warns once; if it also had a
1:1pair, that would be a separate key and a separate warning. That seems
right (they are different lines) but is a judgment call.
Rendering a real 13-entity model surfaced two ER defects. A self-referential relationship was emitted as an ordinary edge. Mermaid's dagre ER layout has no self-loop handling and draws a runaway arc that swamps the canvas, independent of the label text. It now renders as a row inside the entity's own block, carrying the target-side cardinality, the word `owned` when owned, and the role (issue #26). Relationship labels fell back from role to ownership to raw cardinality. Long prose roles collided, and an owned relationship that also had a role lost its ownership signal entirely. Ownership now rides on the connector — solid (identifying) for owned, dashed otherwise — and the role is the only label. Ownership folds across a pair declared from both ends, so a parent's `owned` and a child's `referenced` draw one solid line. A new semantic lint warning steers prose out of `role` and into `note` (issue #27). The golden example gains a self-referential relationship, which is why neither defect was caught before. Conventions recorded in ADR-0008. Signed-off-by: Joe Beda <joe@stacklok.com>
Round-1 review fixes for #28. The ownership fold was wrong in four directions: it fired on two declarations from the same end that differed only in `ownership` (one edge silently vanished), it swallowed mutual `owned` with no diagnostic, it left a reciprocal pair with differing roles drawing one solid and one dashed line, and `ER()`'s comment plus ADR-0008 asserted a guarantee the code did not deliver. Two declarations now merge in exactly two cases: an exact duplicate from the same end, and a genuine reciprocal (opposite ends, inverse cardinality, same label, at most one end claiming `owned`). Everything else draws both lines, so no declaration disappears. The contradictions the renderer refuses to swallow become lint errors: mutual `owned`, and a reciprocal pair whose ends disagree on ownership under different roles. Also: - Self-relationship rows carry the declared cardinality whole, not the substring after the colon, so `0..5:1` no longer renders as `1`. - Identical self-relationship declarations render one row, not two. - The prose-role heuristic judges the role as the diagram draws it: length, word count, `;` or a trailing `.` — no longer any comma or full stop, which false-positived on role lists, accessors and version numbers, and no longer blind to a 66-character single word. - A role raises at most one finding; the undefined-term check waits until the role is no longer prose. - `sanitize` drops backslashes, so `%q` cannot double-escape them. - ADR-0008, `docs/04`, `docs/06` and the plugin skills describe what the code actually does; the `docs/04` snippets match real CLI output. - `TestProseRoleIsWarning` renamed to `TestADR_0008_ProseRoleIsWarning`; the cosmetic `Symmetric` render case is replaced by real coverage of the Markdown marker. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Joe Beda <joe@stacklok.com>
Round-2 review fix. Round 1 put label equality into the fold predicate and
added a "reciprocal ownership disagreement" error to cover the contradiction
that produced. Both were wrong: a parent owning a child while the child
references the parent back, each naming its own end's role, is the textbook
composition pattern and precisely the genuine reciprocal the fold exists for.
It was drawing one solid and one dashed line and failing lint with advice
("give both ends the same role") that argues against the pattern itself.
The predicate is now what was settled: same pair, inverse cardinality,
opposite ends, at most one end claiming `owned`. Labels are out of it. A fold
picks the label deterministically — the owning end's role, or with neither end
owning, the role from the end whose entity sorts first. The other role is
dropped from the diagram only; the Markdown lists each entity's own
relationships, so both survive in the document (ADR-0002's lossy view).
The reciprocal-ownership-disagreement error is deleted. With labels out of the
predicate no unresolvable case remains: exactly one `owned` folds, both `owned`
is the mutual-ownership error, neither `owned` folds to a dashed edge.
Labels still separate two declarations from the same end, so a User who is both
`Owner` and `Member` of a Project keeps two lines. An edge folds in at most one
opposite-end declaration, mirroring the linter, which reconciles a pair only
when each direction declares it exactly once.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Joe Beda <joe@stacklok.com>
…biguous Round-4 review fix. Round 2's `foldedFrom` guard stopped an edge absorbing a second reciprocal declaration but still absorbed the first, and which one that was depended on list order. A Project declaring `defaults` and `overrides` to Policy, with Policy declaring `parent` back, rendered `parent` and `overrides` and dropped `defaults` — swap the two Project declarations and `overrides` went instead. Both linted clean. The single declaration is the reciprocal of one of the two, and the format cannot say which. So the fold is now gated on that ambiguity: a fold requires each end to declare the line at most once. Where it does not hold, nothing folds and every declaration draws its own line — lossless and independent of declaration order. `foldedFrom` is gone; with the gate it can never fire. A lint warning, never an error, names such a pair: the diagram will show more lines than the author probably means, and declaring each relationship from one end only resolves it. It fires on no existing fixture and neither shipped example. The grouping both the renderer and the linter need now lives in one place, `internal/model/edges.go`, so the fold rule and the diagnostic cannot drift apart — which is what produced the last two rounds. ADR-0008's "no declaration disappears" claim was falsified by this defect. It is replaced with the guarantee stated exactly: every declaration draws, except one indistinguishable from an earlier declaration by the same entity, and the role of the non-labelling end of a folded reciprocal, which the Markdown still carries. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Joe Beda <joe@stacklok.com>
f94e3d1 to
0629f96
Compare
Review loop closed — ready for reviewFour rounds run (three per Independently verified after the final fix, not taken on report:
Rebased onto Decided, not deferred: the edge list is not sorted. Declaration order is authorial, the goldens encode it, and sorting would churn committed output for a property we don't need — same input still produces same output. Reordering source lines reorders diagram lines, which is correct behavior; the defect was the dropped role, and that is fixed. Out of scope and filed separately: #29 (pre-existing |
Round-1 review fixes for #28. The ownership fold was wrong in four directions: it fired on two declarations from the same end that differed only in `ownership` (one edge silently vanished), it swallowed mutual `owned` with no diagnostic, it left a reciprocal pair with differing roles drawing one solid and one dashed line, and `ER()`'s comment plus ADR-0008 asserted a guarantee the code did not deliver. Two declarations now merge in exactly two cases: an exact duplicate from the same end, and a genuine reciprocal (opposite ends, inverse cardinality, same label, at most one end claiming `owned`). Everything else draws both lines, so no declaration disappears. The contradictions the renderer refuses to swallow become lint errors: mutual `owned`, and a reciprocal pair whose ends disagree on ownership under different roles. Also: - Self-relationship rows carry the declared cardinality whole, not the substring after the colon, so `0..5:1` no longer renders as `1`. - Identical self-relationship declarations render one row, not two. - The prose-role heuristic judges the role as the diagram draws it: length, word count, `;` or a trailing `.` — no longer any comma or full stop, which false-positived on role lists, accessors and version numbers, and no longer blind to a 66-character single word. - A role raises at most one finding; the undefined-term check waits until the role is no longer prose. - `sanitize` drops backslashes, so `%q` cannot double-escape them. - ADR-0008, `docs/04`, `docs/06` and the plugin skills describe what the code actually does; the `docs/04` snippets match real CLI output. - `TestProseRoleIsWarning` renamed to `TestADR_0008_ProseRoleIsWarning`; the cosmetic `Symmetric` render case is replaced by real coverage of the Markdown marker. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Joe Beda <joe@stacklok.com>
1 participant
Fixes #26. Fixes #27.
Rendering a real 13-entity model surfaced two ER defects; this fixes both and
records the resulting diagram conventions as ADR-0008.
What changed
dagre ER layout has no self-loop handling and draws a runaway arc that swamps
the canvas, independent of label text — so no label-side workaround exists.
The relationship becomes a row inside the entity's own block
(
Project self "0..1 — Predecessor"), carrying the target-side cardinality,the word
ownedwhen owned (no line to carry it), and the role. Rows arenamed
self,self2, … — verified that Mermaid renders a PascalCase type inthat position and does not disambiguate two attributes sharing a name.
--(identifying) forowned, dashed..(non-identifying) forreferencedand for an omittedownership.relationshipLabelis now the role or the empty string; theownershipandcardinalityfallbacks are gone (ADR-0002: precise countslive in the Markdown table).
ownedand thechild declaring
referencedis one identifying relationship seen from twoends, so the folded edge stays solid. Without this, first-declaration-wins
would have drawn the example's
Project/Policycomposition as dashed. Thededupe key is otherwise unchanged, so genuinely distinct edges and
contradictory reciprocal declarations still both render.
rolereads as prose —more than four words, or
,.;— namingnoteas the right field.(
Project→Predecessor), which is why neither defect was caught before.04-reading-the-diagrams.mdrewritten for the new line style, labelrule, and in-box self-relationships;
06-schema-reference.mdgains theconventions and the
rolevsnoteguidance; the parking-garage narrative'shand-written diagram updated; both plugin skills updated. ADR-0008 added.
Verification
task checkpasses (both goldens regenerated withtask render).@mermaid-js/mermaid-cli@11— all valid, no runaway arcs.row plus one edge.
Open questions
self,self2,self3rather than somethingderived from the role. Deriving an identifier from arbitrary role text risks
collisions and invalid Mermaid identifiers, so the conservative numbering was
chosen; the role is still in the row's comment.
keeps the golden example's
`Owner` or `Member`clean and catches theroles that wrecked the reporting model, but it is a heuristic — a role
containing an entity accessor (
`Project`.owner) would trip thepunctuation rule.
--/..on GitHub. Verified with mermaid-cli 11.16.0 and previouslyagainst GitHub's renderer per the brief; not re-verified on GitHub in this PR.
Draft: the review loop has not run yet.