[malibio]

Closes #3267

What changed

Guidance. NODE_REFERENCE_FORMAT and the listing rule now tell the agent to link every node it names as [Title](nodespace://id). The resident-prompt golden is regenerated; its diff is those two lines.

Response normalizer. response_processing.rs used to rewrite [Title](nodespace://id) into **Title** (nodespace://id), which would have undone the new guidance. A titled link now passes through. A link whose label is its own URI is still collapsed to the bare URI, because the renderer wraps bare URIs itself and a URI inside a label would nest.

Ungrounded links (not in the issue). Told to link every node, E4B also links type names and invents the target ([event_venue](nodespace://schema:event_venue)). The fabricated-id guard then replaced the whole reply: 4 replies in the first eval run. A titled link whose target is a type name (lowercase letters and underscores, optionally behind schema:) is now reduced to its label and the reply is kept. Every other ungrounded target still trips the guard, bare or inside a titled link.

Chat reference. The inline reference is a plain hyperlink: no icon, type badge or task status. Its text is pluginRegistry.getNodeTitle(node), so a header shows without its # and a person shows its computed title. The agent's label shows only while the node loads or when the node does not exist (then muted). A resolved node with no title shows "Untitled", not the label.

Sweep for the old bare-URI rule

packages/skill/SKILL.md, its generator and references/ carry no node-reference rule, so nothing changed there. The seeded agent-guidance nodes are built from NODE_REFERENCE_FORMAT, so they follow the constant.

Eval spot-check (eval:decisions, E4B, one rep each, purged database, rebuilt daemon)

	New wording	Old wording (baseline)
pass^1	6/8	8/10
Failing scenarios	schema-on-menu-company, declarative-fact-report-resolved	the same two
Replies replaced by the fabricated-id guard	0	0
Invented type links reduced to their label	4	n/a

The same two scenarios fail under both wordings; the scored counts differ because different scenarios were excluded for empty generations in each run.

Replies use the link form: The record for Northwind Trading already exists as a [company_sold_to](nodespace://9ab0f34c-…) against the baseline's … as a company sold to (nodespace://353b597e-…). Two limits showed up. The label there is the type name, not the title; the rendered link shows the live title regardless. And in the first run one reply named a record with no link at all ("I've updated the booking date for Northwind Trading to April"), so the model does not link every node it names.

Tests

• bun run test:changed passes: frontend, scripts, browser and Rust tiers
• New tests cover the link's title and plain-link markup, label rendering, and the ungrounded-link handling at unit and loop level

Not done here

nodespace-docs/architecture/acp-integration.md still describes the bare-URI rule. It is updated in the docs repo after this merges.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review: PR #3467 (issue #3267)

Review Type: Initial Review (no prior reviews or comments on the PR)

Summary

The change does what the issue asks and is a net improvement: the guidance, the listing rule and the golden agree, the normalizer no longer undoes the link form, and the chat reference takes its text from pluginRegistry.getNodeTitle. The diff is atomic and removes the old card markup and CSS cleanly; no stale class references remain in packages/ or scripts/.

Two things should be handled in this PR. The "missing" style never applies in the real chat, and the new unlink step is broader than the case it was written for: it also keeps replies that name a node the model invented.

Checked by running: the three chat component test files (22 passed) and the targeted nodespace-agent tests for unlink, the fabricated-id guard, the normalizer and the prompt goldens (46 passed). Findings 1 and 3 were confirmed by compiling the two components and by running marked on sample links; finding 2 is from reading the turn-finalization block, not from a failing test.

Requirements Check

#	Acceptance criterion	Status	Notes
1	NODE_REFERENCE_FORMAT and the listing rule instruct [Title](nodespace://id) and "link every node you name"; test updated	✅	agent_guidance.rs:68-69, prompt_assembler.rs:323; both tests assert the new wording.
2	Other model-facing sources swept; SKILL.md drift check passes	✅	packages/skill contains no nodespace:// rule. The seeded guidance node is built from the constant (agent_guidance.rs:171). No old wording is left anywhere in packages/ or scripts/. The drift check was not re-run here; the diff touches no skill file.
3	Title comes from pluginRegistry.getNodeTitle; the label only before the node resolves or when it doesn't exist	✅	node-card-inline.svelte:68-74. A resolved node with no title shows "Untitled", not the label, which is the stricter reading of the issue. See finding 1 for the missing state's styling.
4	Headers show without #; title-template types show their computed title	✅	Tested against the real plugin registry (the global test setup registers core plugins), so the person case exercises hasTitleTemplate.
5	Goldens regenerated and reviewed; eval:decisions spot-check	✅	The golden diff is the two expected lines. The PR body reports the eval run and that one reply named a record with no link, so "link every node" is not fully followed by E4B.

The removal of the icon, type badge and task status is per the user's direction during implementation ("a simple hyperlink"), so it is not scope creep.

Code Review Findings

🟡 Important

1. The muted "missing" style loses to the chat's link colour, so a missing reference looks like a live link
📁 packages/desktop-app/src/lib/components/chat/node-card-inline.svelte:85-88, packages/desktop-app/src/lib/components/chat/chat-markdown.svelte:271-274

Compiled selectors:

.ns-node-card-inline--missing.svelte-41yexu   → specificity (0,2,0)
.chat-markdown.svelte-195me1v a               → specificity (0,2,1)

The parent's a rule is more specific, so color: hsl(var(--primary)) wins and the muted colour never applies inside a chat message, which is the only place this component is mounted. The PR's "then muted" behaviour does not happen. The test only asserts the class is present, so it cannot catch this. I confirmed the specificity from the compiled CSS; I did not render it in a browser.

• Recommendation: put the rule beside the link rule it has to beat, in chat-markdown.svelte: .chat-markdown :global(a.ns-node-card-inline--missing) { color: hsl(var(--muted-foreground)); }. Raising the component's selector to a.ns-node-card-inline--missing only ties at (0,2,1) and then depends on stylesheet order.
• Principle: a stated behaviour needs a check that can fail. A one-time look in the running app (the run-app skill) would cover what Happy-DOM cannot.

2. The unlink step also keeps replies that name an invented node, which the fabricated-id guard used to replace
📁 packages/agent/src/local_agent/agent_loop.rs:1950-1977, 2925-2956

The doc comment's premise is "the sentence around such a link is still a true answer". That holds for the case in the eval (a type name linked to nodespace://schema:event_venue). It does not hold when the label names a node that does not exist:

• search_nodes runs and returns nothing relevant.
• The model writes I found [Budget 2025](nodespace://3f2a9c1e-…).
• The unlink step turns that into I found Budget 2025. No nodespace:// is left, so the fabricated-id guard sees nothing. A tool did run, so the zero-tool-call guard does not fire either. The claim reaches the user.

Before this PR the same reply was replaced by suppressed_response_replacement. Because the guidance now tells the model to write every reference as a titled link, the fabricated-id guard is left covering only the bare form the model is told not to use. An invented id was the one detectable sign of an invented node, and this step removes it while keeping the claim.

• Recommendation: narrow the unlink to targets that cannot be a node id, and leave id-shaped ungrounded targets to the guard. The four eval cases were type names (schema:event_venue, event_venue), which are not UUID-shaped; the original guard's case was an 8-4-4-4-12 shaped invented id. Whichever way this is decided, add a loop-level test for a titled link to an invented id-shaped target, so the behaviour is a recorded choice. A second loop-level test for a reply holding both an ungrounded titled link and a bare invented id (must still be replaced) would pin the ordering of the two steps.
• Principle: a guard should fail closed. A repair that makes the output look clean should not also remove the evidence the guard relies on.

🟢 Suggestions

3. A label with inline markdown shows as literal HTML while loading or missing
📁 packages/desktop-app/src/lib/components/chat/chat-markdown.svelte:41-48

text is parseInline(token.tokens), which is HTML. [**Data** Layer](nodespace://abc) gives data-display-text="<strong>Data</strong> Layer", and the component renders that string as text, so the user sees the tags. The renderer code is unchanged, but before this PR the normalizer rewrote titled links, so labels rarely reached this path; now they are the main path. It is only visible while the node loads or when it is missing.

• Recommendation: build the display text as plain text, this.parser.parseInline(token.tokens, this.parser.textRenderer), and add a case to chat-markdown.test.ts.

4. liveTitle re-implements formatTabTitle
📁 packages/desktop-app/src/lib/components/chat/node-card-inline.svelte:61-71

formatTabTitle in $lib/utils/text-formatting already takes the first line, strips leading #, falls back to "Untitled" and truncates. The two now differ in small ways (^#+\s* against ^#{1,6}\s+, ellipsis against a hard cut at 120). The comment says the link shows "the same value tabs and search show"; calling formatTabTitle(pluginRegistry.getNodeTitle(node) ?? '') would make that true by construction. If the 120-character limit is wanted, say why in the comment. (DRY)

5. The grounded set and the link target are read with different rules
📁 packages/agent/src/local_agent/agent_loop.rs:1827-1830, 1945-1948, 1968

Grounded ids are extracted with node_uri_re, which stops at : and .; the link target is compared whole. That is deliberate for schema:invoice and is tested. It also means any real id containing one of those characters could never be linked. No such id exists today (ids are alphanumeric with - and _), so this is a note for the doc comment, not a bug.

Nitpicks

• Nit: chat-markdown.svelte:43 still says "render as placeholders for rich node cards"; the file header was updated but this line was not.
• Nit: agent_guidance.rs:63-66 and response_processing.rs:143-146 say the renderer turns the link "into a card"; it is now a plain link.
• Nit: dropped_links in the warn log can repeat a target; extract_node_uris dedupes for the neighbouring log line.

Security

No new exposure found. The title and label are rendered through Svelte text interpolation, never as HTML. data-node-id is interpolated unescaped in the link renderer ([x](nodespace://a"><img/src=x/onerror=alert(1)>) reaches the raw HTML string), but that is unchanged by this PR and the output goes through DOMPurify, which is the intended boundary since marked passes inline HTML through anyway. On the local-agent path the unlink step now removes such a target before it reaches the renderer.

CLAUDE.md adherence

No compatibility code, no lint suppression, no raw console.*, no new issue numbers in comments, no docs added to the repo. The node type sequence does not apply (no type, field or relationship changes); the change removes an isTaskNode read from the component. nodespace-docs/architecture/acp-integration.md still describes the bare-URI rule, as the PR body says; it needs the follow-up docs commit.

Recommendation

APPROVE, with findings 1 and 2 to be addressed in this PR before merge. Neither is a blocker on its own: finding 1 is a small visible defect in a stated behaviour, and finding 2 is a narrowing of an existing safety guard that should be an explicit, tested decision.

[malibio]

Automated pragmatic-code-review: APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[malibio]

Review addressed (commit 72e5813)

Addressed

1. Missing-link style never applied. The rule moved to chat-markdown.svelte as .chat-markdown :global(a.ns-node-card-inline--missing), after the link-colour rule. Compiled, it is .chat-markdown.svelte-… a.ns-node-card-inline--missing against .chat-markdown.svelte-… a, so it wins on specificity. Checked from the compiled CSS, not in a running app.
2. Unlink step was too broad. It now drops only a titled link whose target is a name (schema:event_venue, event_venue). A titled link to an ungrounded target shaped like a generated id (8-4-4-4-12) is left for the fabricated-id guard, so I found [Budget 2025](nodespace://3f2a9c1e-…) is replaced again. Two loop-level tests record this: a titled link to an invented id is replaced, and a reply with a dropped type link plus a bare invented id is still replaced.
3. Label with inline markdown showed as HTML. The label is rendered with marked's text renderer and escaped for the attribute. Two component tests cover [**Data** Layer](…) and a label with &, quotes and <.
4. Grounded set and link target read by different rules. Noted in the doc comment.

Nits: the two stale "card" comments are fixed, and dropped_links no longer repeats a target (tested).

Not taken

4. formatTabTitle for the link text. It truncates at 40 characters with an ellipsis, which suits a tab. This text is inline in a message, where it wraps. The comment on liveTitle now says so.

Checks: bun run quality:fix clean; bun run test 298 files / 5519 tests pass; the nodespace-agent guard, normalizer and guidance tests pass (76).

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: Finding 2 changed which replies the fabricated-id guard replaces. That is a behaviour change in a safety guard and should be checked by a second read.

[malibio]

Pragmatic Code Review: PR #3467 (issue #3267)

Review Type: Re-Review (commit 72e5813c, on top of the initial review at 1662c728)

Previous Review Summary

	Count
Previous recommendations	8 (5 findings, 3 nits)
Addressed	7 (findings 1, 2, 3, 5 and all 3 nits)
Not taken, with a sound reason	1 (finding 4)
New issues found	1 Important, 2 nits

Finding 2 is addressed as the first review worded it, but the id-shape test it introduced is too exact, so part of the original hole is still open. That is the one Important finding below.

Checked by running

• chat-markdown-reconcile.test.ts and chat-markdown.test.ts: 14 passed.
• nodespace-agent tests matching unlink_, fabricated_id_guard, ungrounded_node_uris, extract_node_uris: 21 passed.
• The link renderer, run against the repo's marked 16.4.2 and DOMPurify 3.4.12 on ten sample labels and targets.
• The unlink step and guard, modelled with the three regexes copied from agent_loop.rs, on eleven sample replies.

Not run

• The new Rust tests against the pre-change code. The review is read-only, so "would fail without the change" is from reading the code.
• The missing-link colour in a browser. The specificity is from reading the selectors.

Previous findings

#	Finding	Status
1	Missing-link colour lost to the chat link colour	Addressed. .chat-markdown :global(a.ns-node-card-inline--missing) scopes to (0,3,1) against (0,2,1) for .chat-markdown :global(a), so it wins regardless of order. The component's dead style block is removed, and no other file styles that class.
2	Unlink step kept replies naming an invented node	Partly addressed. A well-formed invented UUID is replaced again and two loop tests record it. A malformed or short invented id still gets through: see new finding 1.
3	Label with inline markup showed as HTML	Addressed. See the escaping check below.
4	liveTitle re-implements formatTabTitle	Not taken, and the reason holds: MAX_TAB_TITLE_LENGTH is 40 with an ellipsis, which is wrong for text that wraps inline. The comment on liveTitle now says so.
5	Grounded set and link target read by different rules	Addressed in the doc comment.
Nits	Two stale "card" comments, repeated dropped_links	Addressed. The dedupe has a unit test.

Do the new tests fail without the change?

Test	Fails at 1662c728?
unlink_leaves_a_link_to_an_ungrounded_generated_id_for_the_guard	Yes. The link was reduced to its label.
fabricated_id_guard_replaces_a_titled_link_to_an_invented_id	Yes. The reply became I found Budget 2025.; a tool had run, so no other guard replaced it.
unlink_reports_a_repeated_target_once	Yes. The target was pushed twice.
fabricated_id_guard_still_fires_after_a_type_link_is_dropped	No. It pins the order of the two steps, which was already correct. That is what the first review asked for.
The two ChatMarkdown node link label tests	The **Data** Layer one, yes. The R&D "Plan" a < b one most likely also (the old code put marked's escaped HTML in the attribute, so the label showed &amp;).

Code Review Findings

🟡 Important

1. The id-shape test only matches a well-formed id, so a malformed invented id is unlinked and the claim is kept
📁 packages/agent/src/local_agent/agent_loop.rs:1950-1964, 1986-2002

generated_id_target_re requires exactly 8-4-4-4-12. Anything else that is not grounded is treated as "a name" and reduced to its label. The modelled pipeline gives:

Reply (target ungrounded)	Result
[Budget 2025](nodespace://3f2a9c1e-7b4d-4e8f-9a1b-2c3d4e5f6a7b)	replaced
…-2c3d4e5f6a7) (last group 11 characters)	kept: I found Budget 2025.
…-2c3d4e5f6a7b8) (13 characters)	kept
nodespace://3f2a9c1e7b4d4e8f9a1b2c3d4e5f6a7b (no dashes)	kept
nodespace://3f2a9c1e (short id)	kept
nodespace://abc-123 (the id in NODE_REFERENCE_FORMAT's own example)	kept
nodespace://node_1a2b3c	kept

• Failure scenario: search_nodes finds nothing relevant, the model writes I found [Budget 2025](nodespace://3f2a9c1e-7b4d-4e8f-9a1b-2c3d4e5f6a7), one character short. The link is dropped, no nodespace:// is left for the guard, and "I found Budget 2025." reaches the user. On main every one of these replies is replaced. A small model miscounting 36 characters, or reusing the prompt's example id, is the likely way an invented id arrives, and the function's own comment says an invented id "is often UUID-like without being a valid UUID".

• Cause: the test is the wrong way round. It recognises the dangerous case by an exact shape and lets everything else through. The safe case (a name) is the one that should be recognised.

• Recommendation: unlink only a target that is positively a name, and leave everything else to the guard. Two options:

  • Cheap: unlink only when the target has no ASCII digit. This keeps the four eval cases (schema:event_venue, event_venue) and sends every row in the table above to the guard. A type name with a digit (q3_report) is then replaced, which is main's behaviour. An invented slug with no digit (budget-plan) still gets through.
  • Exact: unlink only when the target, less a schema: prefix, is a type name the session knows. This closes the slug case too, if the type list is reachable from the finalization block.

  Either way, add a table-driven unit test with the near-miss shapes above.

• Principle: a guard should fail closed. An allow-list of what is safe to repair degrades to "reply replaced"; a deny-list of what is dangerous degrades to "false claim shown".

Nitpicks

• Nit: packages/desktop-app/src/lib/components/chat/chat-markdown.svelte:56 — marked's text renderer does not decode entities, so a label written with one ([AT&T](…)) now shows AT&T while loading or missing, where the old path showed AT&T. Rare in model output and only visible in those two states.
• Nit: packages/desktop-app/src/tests/components/chat-markdown.test.ts:21-45 — this file tests a hand copy of the renderer, and the copy now carries its own escapeAttribute. The copy predates this PR, and the new chat-markdown-reconcile.test.ts cases do exercise the real component, so nothing is uncovered; exporting the renderer from a small module would let both files test the same code.

Attribute escaping check

Correct, with no double-escaping. marked 16 keeps token text raw, so escapeAttribute is the only escape applied:

Input	Decoded data-display-text / data-node-id
[R&D "Plan" a < b](nodespace://abc-123)	R&D "Plan" a < b
[use `<b>` & `a&b`](…)	use <b> & a&b
[<img src=x onerror=alert(1)>](…)	the literal text; no element is created
[*em* ~~del~~ [inner](http://x.y)](…)	em del inner
[x](nodespace://a"><img/src=x/onerror=alert(1)>)	id is the literal string; the attribute is not broken out of

DOMPurify keeps both attributes in every case, including a label containing -->. data-node-id is now escaped too, which closes the raw interpolation the first review noted as pre-existing. Both values reach the DOM through getAttribute and Svelte text interpolation, never as HTML.

CLAUDE.md adherence

No compatibility code, lint suppression, raw console.*, issue numbers in comments, or new docs. The node type sequence does not apply.

Recommendation

APPROVE, with new finding 1 to be fixed in this PR before merge. The commit is a net improvement on 1662c728 and nothing in it is a blocker, but the guard is still more permissive than on main for malformed invented ids, and the fix is a small change to one predicate plus a table test.

[malibio]

Automated pragmatic-code-review (re-review): APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[malibio]

Re-review addressed (commit 754bd02)

Addressed

1. Id-shape test let malformed invented ids through. The predicate is turned round as recommended. The unlink step now repairs only a target that is a type name: nodespace://, an optional schema:, then lowercase letters and underscores. Every other ungrounded target stays for the fabricated-id guard. This is stricter than the "no digit" option: a hyphenated slug (budget-plan) and a capitalised name also go to the guard. A table test covers the near-miss shapes from the review (miscounted, undashed, short, the prompt's example id, slugs, a type name with a digit).

   One hole remains and is accepted: an invented target that is itself all lowercase letters and underscores (budget_plan) is unlinked. Closing it needs the session's type list, which the turn-finalization block does not have as a value.

Not taken

• Nit, entity in a label ([AT&T](…) shows undecoded while loading or missing): rare in model output and visible only in those two states.
• Nit, chat-markdown.test.ts tests a hand copy of the renderer: the copy predates this PR, and the new cases in chat-markdown-reconcile.test.ts test the real component.

Checks: cargo fmt and clippy clean; the 18 nodespace-agent unlink and fabricated-id tests pass. The change is Rust-only, so the frontend suite was not re-run.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: One predicate changed, in the direction the re-review recommended and further. It can only send more replies to the existing guard, never fewer, and a table test pins it.