.vscode/launch.json-99-103 (1)

99-103: ⚠️ Potential issue | 🟡 Minor

Use runtimeArgs for the Node condition flag.

The --conditions=sources flag is a Node runtime option and should be in runtimeArgs, not args. With "runtimeExecutable": "tsx", args are passed to the program being debugged, while runtimeArgs are passed to tsx itself, which then passes them to the underlying Node process for proper condition resolution.

Proposed fix

       "program": "${file}",
-      "args": [
-        "--conditions=sources"
-      ],
+      "runtimeArgs": [
+        "--conditions=sources"
+      ],

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.vscode/launch.json around lines 99 - 103, The launch configuration
incorrectly places the Node runtime flag "--conditions=sources" under "args"
(which are passed to the program) instead of "runtimeArgs" (which are passed to
the runtime); update the launch.json so that when "runtimeExecutable": "tsx" is
used the "--conditions=sources" entry is moved from the "args" array into the
"runtimeArgs" array (remove it from "args" and add it to "runtimeArgs") so
tsx/Node receives the flag correctly.

packages/language-server/src/lsp/DocumentSymbolProvider.ts-164-167 (1)

164-167: ⚠️ Potential issue | 🟡 Minor

parentFqn is not actually propagated, and fqn is redundantly computed.

Line 166 adds parentFqn, but it is never consumed in getElementsSymbol. Also, Line 187 computes fqn while Line 195 recomputes the same value inline. Please either wire parentFqn through downstream symbol naming logic or remove it for now, and at minimum reuse fqn to avoid dead code.

Suggested cleanup diff

   protected getExtendElementSymbol(astElement: ast.ExtendElement): DocumentSymbol[] {
@@
-    const fqn = readStrictFqn(astElement.element)
+    const fqn = readStrictFqn(astElement.element)
@@
-        name: readStrictFqn(astElement.element),
+        name: fqn,
         range: cst.range,
         selectionRange: nameNode.range,
-        children: body.elements.flatMap(e => this.getElementsSymbol(e, readStrictFqn(astElement.element))),
+        children: body.elements.flatMap(e => this.getElementsSymbol(e, fqn)),
       },
     ]
   }

Also applies to: 187-195

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/language-server/src/lsp/DocumentSymbolProvider.ts` around lines 164
- 167, The getElementsSymbol method currently accepts parentFqn but never uses
it and also computes fqn twice; fix by either wiring parentFqn into downstream
naming (pass parentFqn into any recursive calls or symbol-name builders used in
getElementsSymbol) or remove the unused parentFqn parameter, and in either case
stop recomputing the same value — reuse the local fqn variable (computed at line
~187) wherever the inline recomputation occurs (line ~195) and ensure recursive
calls (if any) use the chosen fqn propagation logic so symbols are named
consistently; update the signature of getElementsSymbol (and callers) only if
you remove parentFqn.

packages/language-server/src/lsp/CompletionProvider.ts-92-95 (1)

92-95: ⚠️ Potential issue | 🟡 Minor

Handle empty project lists before building the import choice snippet.

If ProjectsManager.all is empty, this produces \${1||}, which is not a valid choice placeholder. In an empty or not-yet-indexed workspace, the import completion will insert malformed snippet text instead of a usable placeholder.

Suggested fix

-      case 'import' === keyword.value:
-        acceptSnippet({
-          insertText: `{ $0 } from '\${1|${this.services.shared.workspace.ProjectsManager.all.join(',')}|}'`,
-        })
-        break
+      case 'import' === keyword.value: {
+        const projectIds = this.services.shared.workspace.ProjectsManager.all
+        const projectRef = projectIds.length > 0
+          ? `\${1|${projectIds.join(',')}|}`
+          : '${1:project}'
+        acceptSnippet({
+          insertText: `{ $0 } from '${projectRef}'`,
+        })
+        break
+      }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/language-server/src/lsp/CompletionProvider.ts` around lines 92 - 95,
The import completion builds a choice snippet from
this.services.shared.workspace.ProjectsManager.all which yields an invalid
`\${1||}` when the list is empty; update the case handling for when
keyword.value === 'import' (the acceptSnippet call) to detect if
ProjectsManager.all is empty and, if so, use a simple placeholder (e.g.
`${1:project}` or `${1:''}`) instead of a choice list, otherwise build the
`\${1|...|}` choice from the joined project names; ensure the conditional
selects the correct insertText string before calling acceptSnippet so no
malformed snippet is emitted.

packages/vscode-preview/protocol.ts-30-39 (1)

30-39: ⚠️ Potential issue | 🟡 Minor

Add an error state to communicate failed AI layout operations to the webview.

The state union only covers 'in-progress' | 'completed'. When AI layout operations fail (API errors, timeouts, cancellations), no completion notification is sent, leaving the webview's loading spinner stuck indefinitely. Add 'error' to the state union and ensure handlers send a completion notification on failures:

Current notification definition

export const BroadcastAILayoutStateUpdate: NotificationType<{
  viewId: ViewId
  projectId: ProjectId
-  state: 'in-progress' | 'completed'
+  state: 'in-progress' | 'completed' | 'error'
}> = {
  method: 'ai-layout-update',
}

The handlers in semanticLayoutWithAI.ts and useLikeC4ChatParticipant.ts need to send the error state when operations fail so the webview can hide the spinner.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/vscode-preview/protocol.ts` around lines 30 - 39, Update the
BroadcastAILayoutStateUpdate notification type to include an 'error' variant in
the state union (change state: 'in-progress' | 'completed' to 'in-progress' |
'completed' | 'error'), then update the failure paths in the AI layout handlers
(referencing the semanticLayoutWithAI.ts handler functions and the
useLikeC4ChatParticipant.ts participants) to send a BroadcastAILayoutStateUpdate
with state: 'error' (and include any error info in the payload if available)
whenever an API error, timeout, or cancellation occurs so the webview can stop
the spinner.

packages/vscode-preview/src/state.ts-12-12 (1)

12-12: ⚠️ Potential issue | 🟡 Minor

Remove unused onMount import (Line 12).

onMount is imported but never used; this will keep showing up in static analysis/lint output.

Suggested fix

-import { atom, batched, onMount, onSet } from 'nanostores'
+import { atom, batched, onSet } from 'nanostores'

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/vscode-preview/src/state.ts` at line 12, The import list in state.ts
includes an unused symbol onMount which triggers lint/static analysis warnings;
remove onMount from the import statement (keep atom, batched, onSet) so the
top-level import reads only the actually used symbols, ensuring no other code
references onMount before removing it.

packages/layouts/src/graphviz/ai/prompt-system.md-52-52 (1)

52-52: ⚠️ Potential issue | 🟡 Minor

Fix typo in rule field name.

excludeFromRanking arra should be excludeFromRanking array.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/layouts/src/graphviz/ai/prompt-system.md` at line 52, Fix the typo
in the documentation: change the text fragment that reads "excludeFromRanking
arra" to "excludeFromRanking array" in the sentence describing the
excludeFromRanking behavior (the rule name is excludeFromRanking); ensure only
the word "arra" is replaced with "array" so the rule name and surrounding
explanation remain unchanged.

packages/vscode/data/skills/likec4-dsl/references/deployment.md-108-108 (1)

108-108: ⚠️ Potential issue | 🟡 Minor

Tighten this sentence for the skill data.

"Strict eval requires named instance identifier" is awkward English and harder for the model to learn from. "Strict eval requires a named instance identifier" or "requires the instance to be named" is clearer.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/vscode/data/skills/likec4-dsl/references/deployment.md` at line 108,
The sentence "Strict eval requires named instance identifier" is awkward; update
the text in deployment.md (the table row that currently reads "Strict eval
requires named instance identifier" under the feature column and the example
column showing "Named: `IDENTIFIER = instanceOf ELEMENT`") to clearer wording
such as "Strict eval requires a named instance identifier" or "Strict eval
requires the instance to be named" so the skill data reads unambiguously.

packages/vscode/data/skills/likec4-dsl/references/model.md-139-140 (1)

139-140: ⚠️ Potential issue | 🟡 Minor

Keep this invalid-extension example consistent with the contract above.

Lines 139-140 say omitting KIND "silently targets the wrong relation", but Lines 51-57 define that case as wrong/invalid whenever a typed relationship exists. That contradiction will train the skill toward the wrong matcher behavior.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/vscode/data/skills/likec4-dsl/references/model.md` around lines 139
- 140, Update the invalid-extension example wording so it matches the contract
defined earlier: change the phrase on the example around KIND (lines referencing
the comment about omitting KIND) from "silently targets the wrong relation" to
state that omitting KIND is invalid whenever a typed relationship exists; ensure
the example text for the snippet extend cloud.ui.dashboard -> cloud.backend.api
"calls" { ... } explicitly labels that omission as invalid (per the rule in the
block around lines 51-57) rather than implying silent targeting.

packages/vscode/tsdown.config.ts-112-116 (1)

112-116: ⚠️ Potential issue | 🟡 Minor

Clean destination before copying skills to avoid stale artifacts.

Line 115 copies recursively but never removes previously copied files. If a skill file is removed upstream, stale files can remain in ./data/skills/likec4-dsl.

💡 Suggested fix

 async function copySkills() {
   const skillDir = resolve('../../skills/likec4-dsl')
   console.info('Copy SKILLs: %s', skillDir)
+  emptyDir('./data/skills/likec4-dsl')
+  await mkdir('./data/skills/likec4-dsl', { recursive: true })
   await cp(skillDir, './data/skills/likec4-dsl', { recursive: true })
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/vscode/tsdown.config.ts` around lines 112 - 116, The copySkills
function currently copies from resolve('../../skills/likec4-dsl') into
'./data/skills/likec4-dsl' without removing any existing files, causing stale
artifacts; update copySkills to check for the destination
'./data/skills/likec4-dsl', delete it recursively (e.g., using fs.promises.rm or
fs.rmSync with recursive/force) before calling cp, and ensure any errors during
removal are handled or propagated so the subsequent await cp(skillDir,
'./data/skills/likec4-dsl', { recursive: true }) writes a fresh copy.

packages/vscode/data/skills/likec4-dsl/references/predicates.md-9-10 (1)

9-10: ⚠️ Potential issue | 🟡 Minor

Remove the stray quote in the predicate-types list.

The trailing ' after ``include only makes this read like a broken code token in the skill reference.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/vscode/data/skills/likec4-dsl/references/predicates.md` around lines
9 - 10, Remove the stray trailing apostrophe in the predicate-types list: locate
the line containing "Custom predicates - used to override properties of
elements/relationships (can be used with `include` only)`" in predicates.md and
delete the final single-quote character so the text ends after the closing
backtick; ensure the code token remains as `include` only with no extra
punctuation.

packages/vscode/src/commands/semanticLayoutWithAI.ts-132-140 (1)

132-140: ⚠️ Potential issue | 🟡 Minor

Handle CancellationError separately to avoid showing error toast on user cancellation.

When sendRequest() within enhanceLayoutWithAI() throws vscode.CancellationError() (thrown at line 286 when cancellation is detected), it currently falls through to the generic error handler and displays an error toast. User cancellations should exit silently instead.

Add an explicit check before the current else clause to handle error instanceof vscode.CancellationError without showing any message.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/vscode/src/commands/semanticLayoutWithAI.ts` around lines 132 - 140,
The catch block in enhanceLayoutWithAI currently treats vscode.CancellationError
like a real failure and shows an error toast; update the catch to check for
cancellation first (if error instanceof vscode.CancellationError) and
return/exit silently without logging or showing a message, before the existing
"No language model" and generic error branches; reference the catch in
semanticLayoutWithAI.ts that surrounds the call to
sendRequest()/enhanceLayoutWithAI() and use the vscode.CancellationError type
for the check.

packages/vscode/src/useRpc.ts (1)

91-103: Consider using true as const for consistent type narrowing.

The error case uses false as const for precise type narrowing, but the success case uses res.success which loses the literal type. For consistency and better discriminated union support:

       return {
         location: res.location ? client.protocol2CodeConverter.asLocation(res.location) : null,
-        success: res.success,
+        success: true as const,
       }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/vscode/src/useRpc.ts` around lines 91 - 103, In changeView, the
success branch returns success: res.success which loses the literal boolean
type—change it to return success: true as const (while keeping the mapped
location logic) so the returned discriminated union uses a literal true; locate
this in the async changeView method that calls queue(() =>
client.sendRequest(ChangeView.req, req)) and adjust the success return object to
use true as const instead of res.success.

packages/layouts/turbo.json (1)

7-17: Consider adding dependsOn to the generate task.

This helps Turbo orchestrate generation order predictably when multiple packages define generate.

🔁 Suggested update

   "tasks": {
     "generate": {
+      "dependsOn": [
+        "^generate"
+      ],
       "inputs": [
         "scripts/**",
         "src/**/*.md",
         "package.json"
       ],

Based on learnings: Configure Turbo task dependencies in `turbo.json` to orchestrate build order across the monorepo.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/layouts/turbo.json` around lines 7 - 17, Add a "dependsOn" entry to
the "generate" task so Turbo can order generation across packages; update the
"generate" object to include dependsOn (e.g. ["^generate"]) to make each
package's generate task depend on its workspace ancestors' generate tasks,
ensuring predictable orchestration when multiple packages define "generate".

packages/vscode/src/useMessenger.ts (1)

129-130: Reuse the shared AI-layout payload type instead of re-declaring it inline.

This anonymous { viewId; projectId; state } shape duplicates the contract already owned by BroadcastAILayoutStateUpdate. Pull it into an exported type and reuse it on both sides so the extension and preview can't drift independently.

As per coding guidelines, "Agent types and interfaces should be defined separately and reused across the codebase".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/vscode/src/useMessenger.ts` around lines 129 - 130, The inline
anonymous payload in broadcastAiLayoutUpdate should be replaced with a shared
exported type so both sender and receiver use the same contract; define and
export a reusable interface/type (e.g., AILayoutStatePayload or reuse the
existing BroadcastAILayoutStateUpdate payload type) and update
broadcastAiLayoutUpdate's params signature to reference that exported type
instead of the inline { viewId; projectId; state } shape, ensuring the
messenger.sendNotification call still passes the same object shape.

packages/vscode/src/node/useLikeC4ChatParticipant.ts (1)

190-197: Remove obsolete commented block to keep the participant lean.

Lines 190-197 are stale commented code paths. Keeping them around makes this hot path noisier to maintain.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/vscode/src/node/useLikeC4ChatParticipant.ts` around lines 190 - 197,
Remove the stale commented block that defines participant.titleProvider and the
commented useChatParticipant call in useLikeC4ChatParticipant.ts: delete the
commented lines referencing participant.titleProvider, provideChatTitle, and the
commented useChatParticipant('likec4', ..., { iconPath:
vscode.Uri.joinPath(extensionContext.value!.extensionUri,
'data/icon-256-light.png') }) so the file only contains active, relevant
participant setup and remains lean.

packages/layouts/src/graphviz/ElementViewPrinter.ts (1)

98-100: Avoid as any for the rank-block marker.

The new coordination flag is now hidden behind as any, which makes key drift harder to catch. A small typed helper/constant would keep this path aligned with the repo’s TypeScript rules.

As per coding guidelines, "Use TypeScript with explicit types; avoid using any and type casts with as".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/layouts/src/graphviz/ElementViewPrinter.ts` around lines 98 - 100,
Replace the ad-hoc cast in ElementViewPrinter where
this.G.set('likec4_rankBlocks' as any, applied) is used: define a typed
constant/helper (e.g. const LIKEC4_RANK_BLOCKS_KEY = 'likec4_rankBlocks' as
const or an appropriately typed keyof/enum value) and use that constant in the
this.G.set call instead of using as any, keeping the key name likec4_rankBlocks
and matching the G map's expected key type so TypeScript catches any drift.

packages/layouts/src/graphviz/AiLayoutPrinter.ts (1)

166-170: Model synthetic invisible edges explicitly instead of casting them through any.

origin: 'invisible' as any hides that these injected edges do not match GraphologyEdgeAttributes. Please widen the edge-attribute model for synthetic edges rather than lying to the type system here.

As per coding guidelines, "Use TypeScript with explicit types; avoid using any and type casts with as."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/layouts/src/graphviz/AiLayoutPrinter.ts` around lines 166 - 170, The
code casts synthetic edges with origin: 'invisible' as any; instead extend the
Graphology edge-attribute type to explicitly allow synthetic attributes and use
that type when calling graphology.addEdgeWithKey. Create a new type (e.g.,
SyntheticEdgeAttributes extends GraphologyEdgeAttributes { origin?: 'invisible';
hierarchyDistance?: number; weight?: number }) or widen the existing
GraphologyEdgeAttributes union, then replace the inline cast and pass an object
typed as SyntheticEdgeAttributes (referencing addEdgeWithKey, edgeId, source,
target and the origin property) so the injected invisible edges are correctly
typed without using any.

packages/vscode/vscode.proposed.chatParticipantAdditions.d.ts (1)

1-6: Vendored VS Code proposed API declaration file - looks appropriate.

This file correctly mirrors VS Code's proposed chatParticipantAdditions API declarations needed for the experimental chat participant feature. The Microsoft copyright and version marker indicate this is sourced from VS Code's proposed APIs.

Consider adding a brief comment after the version marker to clarify that this file is vendored and should be updated by copying from VS Code's proposed API definitions rather than edited manually:

📝 Suggested documentation

 // version: 3
+// This file is vendored from VS Code's proposed APIs. Do not edit manually.
+// Source: https://github.com/microsoft/vscode/blob/main/src/vscode-dts/vscode.proposed.chatParticipantAdditions.d.ts

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/vscode/vscode.proposed.chatParticipantAdditions.d.ts` around lines 1
- 6, Add a short clarifying comment immediately after the "// version: 3" marker
stating that this is a vendored VS Code proposed API declaration and should be
refreshed by copying from VS Code's proposed API definitions rather than edited
in place; update the file header near the existing copyright block and the "//
version: 3" line so maintainers can quickly find and recognize the vendored
chatParticipantAdditions declaration.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/layouts/src/graphviz/ai/llm-output.ts (2)

69-80: Minor: Use optional chaining for cleaner null check.

The conditional can be simplified using optional chaining as suggested by static analysis.

♻️ Suggested fix

 function extractJson(text: string): string {
   const match = jsonRegex.exec(text)
-  if (match && match[1]) {
+  if (match?.[1]) {
     return match[1]
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/layouts/src/graphviz/ai/llm-output.ts` around lines 69 - 80, In
extractJson, simplify the null check by using optional chaining on the regex
match (jsonRegex.exec(text)) so you can directly test and return match?.[1] (or
equivalent) instead of if (match && match[1]); update the reference to jsonRegex
and the local variable match accordingly and keep the rest of the fallback logic
(finding '{' and '}' and returning text.trim()) unchanged.

---

191-200: Escape regex special characters in ID replacement for defense-in-depth.

While the serialized IDs (n0, e1, etc.) are programmatically generated and unlikely to contain regex metacharacters, escaping them prevents potential issues if the ID format ever changes. This also addresses the static analysis ReDoS warning.

🛡️ Proposed fix

+  const escapeRegex = (s: string) => s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')
+
   let reasoning = parsed.reasoning
   if (reasoning) {
     reasoning = reasoning.replaceAll('][', '] [')
     entries(params.mapping.nodes).forEach(([id, nd]) => {
-      reasoning = reasoning.replaceAll(new RegExp(`\\[${id}\\]`, 'g'), '`' + nd.id + '`')
+      reasoning = reasoning.replaceAll(new RegExp(`\\[${escapeRegex(id)}\\]`, 'g'), '`' + nd.id + '`')
     })
     entries(params.mapping.edges).forEach(([id, e]) => {
-      reasoning = reasoning.replaceAll(new RegExp(`\\[${id}\\]`, 'g'), '`' + e.source + ' -> ' + e.target + '`')
+      reasoning = reasoning.replaceAll(new RegExp(`\\[${escapeRegex(id)}\\]`, 'g'), '`' + e.source + ' -> ' + e.target + '`')
     })
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/layouts/src/graphviz/ai/llm-output.ts` around lines 191 - 200, The
code constructs RegExp objects from node/edge IDs in the reasoning replacement
loop (see reasoning, entries(params.mapping.nodes) and
entries(params.mapping.edges) where replaceAll(new RegExp(`\\[${id}\\]`, 'g') is
used), which can be unsafe if IDs contain regex metacharacters; update the
replacement to escape regex-special characters in the ID before interpolating
into the RegExp (add a small utility to escape characters like .*+?^${}()|[]\\
and use that when building the RegExp) so the pattern matches the literal
bracketed ID and avoids potential ReDoS or mis-matching.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/vscode/src/panel/activateMessenger.ts (1)

138-148: LGTM — conversion correctly centralized in useRpc.

Removing the panel-side protocol2CodeConverter.asLocation call is consistent with packages/vscode/src/useRpc.ts (lines 91-103), where changeView already returns a vscode.Location | null. The null-guard at line 139 keeps the subsequent showEditorNextToPreview call (which requires a non-null vscode.Location per packages/vscode/src/utils.ts) and vscode.workspace.save(location.uri) safe.

Optional nit: if result.location is typed as vscode.Location | null at the RPC boundary, the ?? null on line 138 is redundant and can be dropped:

♻️ Optional simplification

-      const location = result.location ?? null
-      if (!location) {
+      const { location } = result
+      if (!location) {
         logger.warn(`rpc.changeView returned null`)
         return result
       }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/vscode/src/panel/activateMessenger.ts` around lines 138 - 148, The
code uses a redundant null-coalescing on result.location (result.location ??
null); remove the "?? null" so you simply read result.location (which is already
typed as vscode.Location | null at the RPC boundary) and keep the existing
null-guard and subsequent calls to showEditorNextToPreview and
vscode.workspace.save(location.uri) unchanged; update the reference in
activateMessenger where result.location is used to rely on its declared type
rather than coalescing to null.

packages/vscode/tsdown.config.ts (1)

27-27: Inconsistent ...shared spread placement across build targets.

The node-CJS build spreads ...shared at the top (line 27) so local fields win, while node-ESM (line 60) and both browser targets (lines 72, 86) spread it at the bottom so shared wins. Today there are no overlapping keys so behavior is equivalent, but any future addition to shared (e.g., sourcemap, define, external) will silently apply differently per target and be easy to miss. Consider normalizing placement (preferably ...shared first everywhere, with per-target overrides after) for predictable merge semantics.

Also applies to: 60-60, 72-72, 86-86

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/vscode/tsdown.config.ts` at line 27, The build target objects
currently mix placement of the shared spread (`...shared`) inconsistently
(node-CJS spreads it last while node-ESM and both browser targets spread it
first), which can change merge semantics; update each target object (node-CJS,
node-ESM, and both browser target objects) to place `...shared` consistently at
the start of the object literal so per-target fields defined after the spread
always override shared values, and ensure no other target reverses this
ordering.

packages/mcp/src/server/createMCPServer.ts (4)

146-146: Unused resource binding.

The return value of mcp.registerResource(...) is never referenced. Drop the assignment to avoid the dead variable (also fixes SonarCloud's useless assignment warning).

🧹 Cleanup

-  const resource = mcp.registerResource(
+  mcp.registerResource(

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mcp/src/server/createMCPServer.ts` at line 146, The code assigns the
result of mcp.registerResource(...) to a local binding named resource but never
uses it; remove the dead binding by calling mcp.registerResource(...) directly
(i.e., replace "const resource = mcp.registerResource(...)" with just
"mcp.registerResource(...)" or otherwise discard the return) so the useless
assignment is eliminated and SonarCloud's warning is resolved.

---

120-120: Avoid the as ProjectId cast.

Coding guidelines discourage as casts. ctx?.arguments?.['projectId'] is typed as unknown/string; if parseModel requires a branded ProjectId, prefer going through services.projectsManager.ensureProjectId(...) (used elsewhere in this PR) which validates and returns the branded type, rather than a blind cast. As per coding guidelines: "Use TypeScript with explicit types; avoid using any and type casts with as".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mcp/src/server/createMCPServer.ts` at line 120, Replace the unsafe
cast to ProjectId when calling services.builder.parseModel: instead of passing
ctx?.arguments?.['projectId'] as ProjectId, validate and convert it using
services.projectsManager.ensureProjectId(...) (the same helper used elsewhere in
this PR) and pass the returned branded ProjectId into
services.builder.parseModel; update the call at services.builder.parseModel(...)
to use the ensured ProjectId to remove the as ProjectId cast.

---

48-75: apply-semantic-layout missing from the instructions tool inventory.

The new tool is registered at Line 144 but is not listed in the instructions block passed to the model. Since this string is the primary cue an LLM uses to discover capabilities, add a one-liner (and note its experimental status) so callers know it exists.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mcp/src/server/createMCPServer.ts` around lines 48 - 75, The
instructions string passed to the model is missing the new tool entry for
apply-semantic-layout; add a one-line entry to the tools/instructions block (the
multi-line string in createMCPServer.ts that lists "Available tools:")
describing "apply-semantic-layout — Apply semantic auto-layout to a view. Input:
{ viewId, project?, options? } (experimental)" so callers can discover this
capability and see its experimental status; locate the tools list in the
createMCPServer.ts instructions string and append the apply-semantic-layout
one-liner near the other tool entries.

---

109-112: Prefer a default parameter over nullish reassignment.

Matches the SonarCloud hint and aligns with the switch(true) / idiomatic guideline style used across the repo. The same nit applies to the inner const value = _value ?? '' at Line 124.

♻️ Proposed tidy-up

-        projectId: completable(projectIdSchema, (_value) => {
-          const value = _value ?? ''
-          return services.projects().filter(project => project.id.startsWith(value)).map(prop('id'))
-        }),
+        projectId: completable(projectIdSchema, (value = '') => {
+          return services.projects().filter(project => project.id.startsWith(value)).map(prop('id'))
+        }),
         viewId: completable(
           z.string().describe('View ID to apply semantic layout to'),
-          async (_value, ctx) => {
+          async (value = '', ctx) => {
             const projectId = ctx?.arguments?.['projectId']
             if (!projectId) {
               return []
             }
             const model = await services.builder.parseModel(projectId as ProjectId)
             if (!model) {
               return []
             }
-            const value = _value ?? ''
             return Array.from(model.views()).filter((view) => view.id.startsWith(value)).map(prop('id'))
           },
         ),

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mcp/src/server/createMCPServer.ts` around lines 109 - 112, The
inline completable for projectId uses a nullish reassignment; change the lambda
to use a default parameter and remove the redundant const assignment: update the
parameter signature of the completable callback (currently _value) to default to
'' (e.g., _value = '') and delete the inner const value = _value ?? '' so the
body can directly use the parameter when filtering via services.projects(). Also
apply the same change to the other similar lambda that creates const value to
keep style consistent with projectIdSchema and the repo's idiomatic pattern.

packages/mcp/src/tools/apply-semantic-layout.ts (2)

17-20: Remove commented-out completable block.

Dead/commented code should not land in main. If the completable variant is the intended direction, switch to it; otherwise delete these lines.

🧹 Proposed cleanup

       inputSchema: {
         projectId: projectIdSchema,
-        // projectId: completable(projectIdSchema, (_value) => {
-        //   const value = _value ?? ''
-        //   return languageServices.projects().filter(project => project.id.startsWith(value)).map(prop('id'))
-        // }),
         viewId: z.string().describe('View ID to apply semantic layout to'),
       },

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mcp/src/tools/apply-semantic-layout.ts` around lines 17 - 20, Remove
the dead commented-out completable block around the projectId field in
apply-semantic-layout.ts: either delete the three commented lines starting with
"projectId: completable(projectIdSchema, ..." or, if you intend to enable the
completable behavior, uncomment and wire it up properly by using
completable(projectIdSchema, ...) and ensure it uses
languageServices.projects().filter(...).map(prop('id')); specifically address
the symbols projectId, completable, projectIdSchema, and
languageServices.projects() so the file contains no leftover commented code.

---

50-58: Dead fallback branch after zod validation.

The response is already validated against z.object({ content: z.object({ type: z.literal('text'), text: z.string() }) }), so response.content.type !== 'text' is unreachable. The ternary reduces to response.content.text.

🧹 Simplify

-          return response.content.type === 'text' ? response.content.text : JSON.stringify(response.content)
+          return response.content.text

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mcp/src/tools/apply-semantic-layout.ts` around lines 50 - 58, The
ternary returning response.content.type === 'text' ? response.content.text :
JSON.stringify(response.content) is dead because the response is validated by
z.object({ content: z.object({ type: z.literal('text'), text: z.string() }) }),
so simplify the function to directly return response.content.text; update the
code around the response handling in apply-semantic-layout (the validation and
the return line) to remove the unreachable branch and return the validated text
property unconditionally.

packages/mcp/src/server/StreamableLikeC4MCPServer.ts (1)

147-166: Minor: context is cleared before the HTTP server actually closes.

setMcpServerCtx(undefined) runs synchronously at Line 155, but server.close(...) completes later inside the Promise. In-flight requests on /mcp that call useMcpServer() between the unset and the actual close will throw (unctx .use() throws on unset). Consider deferring the unset until after server.close resolves, or gating it on an isStopping flag. Since stop() is typically called during process shutdown, impact is small — flagging as a chill-level refinement.

♻️ Suggested reorder

-    logger.info('Stopping MCP server')
-    this.server = undefined
-    setMcpServerCtx(undefined)
-    return new Promise((resolve) => {
-      server.close((err) => {
-        if (err) {
-          logger.error('Failed to stop MCP server', { err })
-        } else {
-          logger.info('MCP server stopped')
-        }
-        resolve()
-      })
-    })
+    logger.info('Stopping MCP server')
+    this.server = undefined
+    return new Promise((resolve) => {
+      server.close((err) => {
+        setMcpServerCtx(undefined)
+        if (err) {
+          logger.error('Failed to stop MCP server', { err })
+        } else {
+          logger.info('MCP server stopped')
+        }
+        resolve()
+      })
+    })

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mcp/src/server/StreamableLikeC4MCPServer.ts` around lines 147 - 166,
The stop() implementation clears the MCP context (setMcpServerCtx(undefined))
before the HTTP server actually finishes closing, allowing in-flight handlers
that call useMcpServer() to throw; change stop() to either set an isStopping
flag (e.g., this.isStopping = true) before calling server.close and only call
setMcpServerCtx(undefined) inside the server.close callback (or after the close
Promise resolves), and ensure useMcpServer() checks the isStopping flag or the
context existence to avoid throwing for in-flight requests during shutdown;
update references to stop(), setMcpServerCtx, server.close, and useMcpServer
accordingly.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/layouts/src/graphviz/wasm/GraphvizWasmAdapter.ts (1)

61-63: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Retry still bypasses opsCount bookkeeping (unresolved from previous review).

Line 63 calls fn() directly, so a successful retry never increments GraphvizWasmAdapter.opsCount. This means the unload-after-20-ops memory guard will fire 20 operations later than intended for every retry that succeeds. Although the error path already resets opsCount to 0 and unloads WASM, the bookkeeping asymmetry will compound if retries become frequent.

🔧 Proposed fix – extract a shared execution wrapper

   private async attempt<T>(logMessage: string, dot: string, fn: () => Promise<T>): Promise<T> {
     return await limit(async () => {
       const logger = rootLogger.getChild(['layouter', 'wasm', logMessage, '_', randomString(4).toLowerCase()])
+      const runOnce = async () => {
+        logger.trace`execute`
+        const result = await fn()
+        if (++GraphvizWasmAdapter.opsCount >= 20) {
+          GraphvizWasmAdapter.opsCount = 0
+          logger.trace`reached 20 operations, unloading wasm`
+          Graphviz.unload()
+        }
+        return result
+      }
       try {
-        logger.trace`execute`
-        const result = await fn()
-        if (++GraphvizWasmAdapter.opsCount >= 20) {
-          GraphvizWasmAdapter.opsCount = 0
-          // very hacky, but helps
-          logger.trace`reached 20 operations, unloading wasm`
-          Graphviz.unload()
-        }
-        return result
+        return await runOnce()
       } catch (error) {
         logger.trace('FAILED DOT\n' + dot)
         const errorStr = loggable(error)

         // don't retry on syntax errors
         if (errorStr.includes('syntax error')) {
           throw error
         }
         logger.warn(errorStr)

         GraphvizWasmAdapter.opsCount = 0
         Graphviz.unload()
       }
       logger.warn('Retrying...')
       await delay(30, 300)
-      return await fn()
+      return await runOnce()
     })
   }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/layouts/src/graphviz/wasm/GraphvizWasmAdapter.ts` around lines 61 -
63, The retry path currently calls fn() directly so successful retries don't
update GraphvizWasmAdapter.opsCount; extract a shared execution wrapper (e.g.,
executeWithOpBookkeeping or wrapCall) that always increments this.opsCount and
performs the same success/error handling, then replace direct calls to fn()
(including the retry branch) with that wrapper so every successful retry
increments GraphvizWasmAdapter.opsCount and the unload-after-20-ops guard
remains correct.

packages/layouts/src/graphviz/ai/llm-input.ts (1)

108-115: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Don't reject the entire view on compound endpoints.

This unconditional throw still aborts the whole AI path as soon as any single edge touches a non-leaf node. As previously noted, ElementViewPrinter.addEdge() already supports compound endpoints, so container-to-node and container-to-container views lose AI layout entirely here. Prefer per-edge handling: serialize compound endpoints when supported, or skip individual unsupported edges with a logger.warn instead of failing the whole view.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/layouts/src/graphviz/ai/llm-input.ts` around lines 108 - 115, The
current code finds a single nonLeafEdge and throws, which aborts the entire AI
path; instead, remove the unconditional throw and handle edges per-edge: iterate
view.edges and for each edge check leafs.has(edge.source) and
leafs.has(edge.target); if both are leaves, process/serialize the edge as before
(via ElementViewPrinter.addEdge()), if one or both endpoints are compound but
ElementViewPrinter.addEdge() supports compound endpoints, serialize them
accordingly, otherwise skip that single edge and call logger.warn with view.id
and the edge {source,target} explaining it's unsupported; ensure you no longer
use the single nonLeafEdge throw and that logging is per-edge rather than
aborting the whole view.

packages/layouts/src/graphviz/GraphvizLayoter.ts (1)

158-176: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Dynamic-view post-processing still missing in aiLayout.

layout() attaches sequenceLayout for dynamic views via calcSequenceLayout, but aiLayout() returns the parsed diagram directly. AI-enhancing a dynamic view will therefore yield a LayoutResult with a different shape than the standard path, breaking downstream consumers that rely on LayoutedDynamicView.sequenceLayout. Mirror the dynamic-view branch from layout() here.

Proposed fix

       let diagram = parseGraphvizJson(json, view)
+      if (isDynamicView(diagram)) {
+        Object.assign(
+          diagram,
+          {
+            sequenceLayout: calcSequenceLayout(diagram),
+          } satisfies Partial<LayoutedDynamicView<A>>,
+        )
+      }
       dot = normalizeDot(dot)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/layouts/src/graphviz/GraphvizLayoter.ts` around lines 158 - 176,
aiLayout currently returns dot+diagram without applying the dynamic-view
post-processing that layout() does, so AI-laid-out dynamic views lack the
sequenceLayout field; update aiLayout (inside the AiLayoutLayouter.aiLayout
method) to detect dynamic views the same way layout() does, call
calcSequenceLayout(view, diagram) (or the same helper used by layout()), attach
the returned sequenceLayout onto the LayoutResult (i.e., return a
LayoutedDynamicView with sequenceLayout for dynamic views), and keep the
existing error handling and logger behavior; reference aiLayout, layout,
calcSequenceLayout, sequenceLayout, LayoutResult, and LayoutedDynamicView when
implementing the branch.

packages/layouts/src/graphviz/AiLayoutPrinter.ts (2)

97-98: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Unused destructured tuple elements sourceNode/targetNode.

sourceNode and targetNode are bound but never read. Static analysis is consistently flagging both. Drop the leading slot to remove the noise without altering tuple positions.

♻️ Diff

-    const [sourceNode, source, ltail] = this.edgeEndpoint(sourceFqn, nodes => last(nodes))
-    const [targetNode, target, lhead] = this.edgeEndpoint(targetFqn, first)
+    const [, source, ltail] = this.edgeEndpoint(sourceFqn, nodes => last(nodes))
+    const [, target, lhead] = this.edgeEndpoint(targetFqn, first)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/layouts/src/graphviz/AiLayoutPrinter.ts` around lines 97 - 98, The
destructured variables sourceNode and targetNode are unused; update the two
destructuring assignments that call edgeEndpoint to drop the leading slot so
tuple positions stay correct—change const [sourceNode, source, ltail] =
this.edgeEndpoint(sourceFqn, nodes => last(nodes)) to const [, source, ltail] =
this.edgeEndpoint(sourceFqn, nodes => last(nodes)) and change const [targetNode,
target, lhead] = this.edgeEndpoint(targetFqn, first) to const [, target, lhead]
= this.edgeEndpoint(targetFqn, first).

---

9-11: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Remove unused imports.

nonNullable from @likec4/core (line 9) and isEmptyish from remeda (line 11) are not referenced in this file. SonarCloud and previous review iterations already flagged these.

♻️ Diff

 import {
   type AnyAux,
   type ComputedEdge,
   type ComputedNode,
   type ComputedView,
   type EdgeId,
   type HexColor,
   type LikeC4Styles,
-  nonNullable,
 } from '@likec4/core'
-import { entries, first, isEmptyish, isNonNullish, isNumber, last } from 'remeda'
+import { entries, first, isNonNullish, isNumber, last } from 'remeda'

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/layouts/src/graphviz/AiLayoutPrinter.ts` around lines 9 - 11, Remove
the unused named imports nonNullable (from '@likec4/core') and isEmptyish (from
'remeda') in AiLayoutPrinter.ts: locate the import statements that currently
include nonNullable and isEmptyish, delete those symbols from their respective
import lists, adjust remaining commas/spacing so the imports remain valid, and
then run the TypeScript build/linter to ensure no other references remain.

packages/language-server/src/Rpc.ts (1)

166-169: 💤 Low value

Optional: include hints presence in the debug log.

The current log records viewId, projectId, and layoutType but not whether AI hints were supplied. Adding a small flag (e.g., hints: hints ? 'present' : 'none') would help diagnose issues unique to the AI-enhanced path without leaking the hint payload.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/language-server/src/Rpc.ts` around lines 166 - 169, The debug log
for the layoutView request currently reports viewId, projectId and layoutType;
update the logger.debug call (the template literal using logger.debug`received
request ${'layoutView'} ...`) to also include a small hints-presence flag such
as "hints: " + (hints ? "present" : "none") so the log shows whether AI hints
were supplied without printing the hint payload; keep the rest of the message
unchanged and reference the existing variables viewId, projectId, layoutType and
hints.

packages/layouts/src/graphviz/ai/llm-output.ts (3)

174-189: 💤 Low value

weight: 1 and minlen: 1 are dropped silently.

When the LLM specifies weight: 1 or minlen: 1 explicitly on an invisibleEdge, the !== 1 checks strip them, treating them as defaults. That matches the destructuring defaults and is probably intentional, but the contract is implicit. Consider documenting this normalization (e.g., a brief comment) so downstream consumers understand undefined means "use default" rather than "LLM didn't provide".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/layouts/src/graphviz/ai/llm-output.ts` around lines 174 - 189, The
parsed.invisibleEdges mapping drops explicit weight: 1 and minlen: 1 by
converting them to undefined (seen in the invisibleEdges const using
parsed.invisibleEdges, nodeId, and exact to build AIEnforcementEdge), which is
implicit; add a concise comment immediately above the invisibleEdges computation
explaining that weight/minlen are normalized to undefined when equal to 1 to
indicate "use default" (not "value absent"), so downstream consumers understand
that undefined means "apply graphviz default" rather than "property not
provided."

---

143-157: 💤 Low value

Use mapToNonEmpty consistently for excludeFromRanking and reverseRank.

mapToNonEmpty (defined at lines 138-141) already encapsulates "map → filter undefined → return undefined when empty". Both excludeFromRanking and reverseRank re-implement essentially the same flow inline (with an added unique()). Extracting a small helper or extending mapToNonEmpty to optionally dedupe would remove the duplication.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/layouts/src/graphviz/ai/llm-output.ts` around lines 143 - 157, The
two blocks computing excludeFromRanking and reverseRank duplicate map → filter
undefined → empty-to-undefined logic already implemented by mapToNonEmpty;
refactor them to call mapToNonEmpty(parsed.excludeFromRanking, edgeId) and
mapToNonEmpty(parsed.reverseRank, edgeId) and handle deduplication by either
extending mapToNonEmpty to accept an optional { unique: true } flag or by
wrapping its result with unique() only when non-undefined; update references to
excludeFromRanking and reverseRank accordingly and preserve the existing
unique() behavior.

---

191-200: 💤 Low value

Replace dynamic RegExp with literal replaceAll for reasoning normalization.

new RegExp(\[${id}\], 'g') constructs regexes from variable input on every iteration. Since you're matching exact [id] substrings (where id is n1/c1/e1-style), a plain string replaceAll is simpler, faster, and sidesteps ReDoS concerns flagged by static analysis.

♻️ Proposed refactor

     entries(params.mapping.nodes).forEach(([id, nd]) => {
-      reasoning = reasoning.replaceAll(new RegExp(`\\[${id}\\]`, 'g'), '`' + nd.id + '`')
+      reasoning = reasoning.replaceAll(`[${id}]`, '`' + nd.id + '`')
     })
     entries(params.mapping.edges).forEach(([id, e]) => {
-      reasoning = reasoning.replaceAll(new RegExp(`\\[${id}\\]`, 'g'), '`' + e.source + ' -> ' + e.target + '`')
+      reasoning = reasoning.replaceAll(`[${id}]`, '`' + e.source + ' -> ' + e.target + '`')
     })

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/layouts/src/graphviz/ai/llm-output.ts` around lines 191 - 200, The
code currently builds RegExp objects inside the loops when replacing occurrences
like `[${id}]` in the reasoning string; change those to use literal string
replaceAll calls instead: in the loop over entries(params.mapping.nodes) replace
the RegExp-based replaceAll with reasoning.replaceAll(`[${id}]`, '`' + nd.id +
'`'), and in the loop over entries(params.mapping.edges) replace the
RegExp-based replaceAll with reasoning.replaceAll(`[${id}]`, '`' + e.source + '
-> ' + e.target + '`'), preserving the initial reasoning.replaceAll('][', '] [')
normalization.

packages/layouts/src/graphviz/ai/llm-input.ts (2)

96-97: ⚡ Quick win

Avoid O(N²) lookups in findNodeById.

findNodeById runs view.nodes.find(...) once per node and once per edge endpoint, giving O(N²) behavior on every view passed to the LLM. Pre-build an index Map once and reuse it.

♻️ Proposed refactor

-  const findNodeById = (id: NodeId) => view.nodes.find(n => n.id === id)!
-  const nodeId = (id: NodeId) => nodeIds.get(findNodeById(id))
+  const nodeById = new Map(view.nodes.map(n => [n.id, n] as const))
+  const findNodeById = (id: NodeId) => {
+    const node = nodeById.get(id)
+    if (!node) throw new Error(`Node ${id} not found in view ${view.id}`)
+    return node
+  }
+  const nodeId = (id: NodeId) => nodeIds.get(findNodeById(id))

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/layouts/src/graphviz/ai/llm-input.ts` around lines 96 - 97, The
current findNodeById (used by nodeId which calls nodeIds.get(findNodeById(id)))
performs view.nodes.find(...) repeatedly causing O(N²) lookups; instead, build a
Map index of node id -> node (e.g., nodeById) once for the given view before any
nodeId calls and use that Map in findNodeById/nodeId to look up nodes in O(1),
ensuring you populate nodeById from view.nodes and replace all view.nodes.find
usages in this module with nodeById.get(id).

---

120-136: 💤 Low value

Stale comment: edges are not actually filtered.

The comment says "Filter edges between leaf nodes and create serialized edges", but view.edges.map(...) only maps — no filter. The non-leaf rejection happens earlier via throw. Either remove "Filter" from the comment or, preferably, replace the throw with a real per-edge filter (see related comment on lines 108-115).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/layouts/src/graphviz/ai/llm-input.ts` around lines 120 - 136, The
comment is stale because view.edges.map(...) doesn't filter; change to
explicitly filter out non-leaf-edge entries before mapping: replace
view.edges.map(...) with view.edges.filter(e => /* check both source and target
are leaf nodes using the same predicate used where a throw currently occurs
*/).map(...) and remove the earlier throw-based rejection (or convert it to a
non-fatal check) so only valid leaf-to-leaf edges are serialized; keep the rest
of the mapping logic (nodeId, edgeIds.get, edgeLabel, handling e.dir and
e.parent) intact.

packages/layouts/src/graphviz/ai/types.ts (1)

63-97: 💤 Low value

Optional: Consider consistency between required/optional collections.

ranks, edgeWeight, and edgeMinlen are required (potentially empty), while other collection-style fields use optional NonEmptyReadonlyArray. Aligning all collection fields to either "optional + NonEmpty" or "required + Readonly" would simplify consumer code (e.g., AiLayoutPrinter, parseOutput). This is purely a shape-consistency suggestion.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/layouts/src/graphviz/ai/types.ts` around lines 63 - 97, The
AILayoutHints type mixes required-empty collections (ranks: ReadonlyArray,
edgeWeight: Record, edgeMinlen: Record) with optional NonEmptyReadonlyArray
fields (reverseRank, excludeFromRanking, edgeOrder, nodeOrder, invisibleEdges),
which complicates consumers like AiLayoutPrinter and parseOutput; pick a
consistent shape and update the type and callers accordingly — either make
ranks/edgeWeight/edgeMinlen optional and use
NonEmptyReadonlyArray/NonEmptyRecord variants (or nullable) to indicate
presence-only semantics, or convert the optional NonEmptyReadonlyArray fields to
required ReadonlyArray/Record (possibly empty) so all collection fields share
the same required-or-optional convention; update AILayoutHints and then adjust
parseOutput and AiLayoutPrinter to handle the chosen convention consistently.

packages/layouts/src/graphviz/GraphvizLayoter.ts (2)

97-105: 💤 Low value

Doc string is now inconsistent with behavior.

The JSDoc states the method does not perform unflattening or any other post-processing on the DOT output, but the implementation now applies normalizeDot(...) to the result, which is a post-processing step. Update the doc to reflect that DOT-level normalization is applied (only unflatten is skipped), or remove the misleading sentence.

📝 Suggested doc update

   /**
    * Generates DOT source for the given view and styles.
    * If `hints` are provided, they will be used to influence the layout (e.g. by specifying node/edge order).
-   * This method does not perform unflattening or any other post-processing on the DOT output, so it may be used for debugging or to generate DOT for external processing.
+   * The DOT is normalized (graph-level margin attribute stripped) but not run through `unflatten`,
+   * so the result is suitable for debugging or external processing.
    */

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/layouts/src/graphviz/GraphvizLayoter.ts` around lines 97 - 105, The
JSDoc for printToDot is misleading because the method does apply DOT
post-processing via normalizeDot; update the comment for printToDot to state
that the output is normalized (normalizeDot is applied) but that unflattening
and other higher-level post-processing are not performed; reference the
printToDot method and mention normalizeDot, AiLayoutViewPrinter and getPrinter
so reviewers can locate the relevant code to change.

---

64-69: 💤 Low value

Fragile string-based filter for graph margin.

normalizeDot uses substring matching (l.includes('margin') && l.includes('${GraphClusterSpace}')) to strip a line set in DotPrinter.createGraph ([_.margin]: GraphClusterSpace). This is brittle:

• If GraphClusterSpace (currently 50.1) is ever used as a numeric value elsewhere in DOT (e.g., a node margin or padding that happens to render as 50.1), the filter will incorrectly drop those lines.
• The trailing comment // see DotPrinter.ts#L175 no longer points at the relevant line in the refactored DotPrinter.ts (see line 326).

Consider emitting a sentinel/marker comment in the printer or removing the attribute on the graph after rendering instead of substring-grepping the output. At minimum, anchor the match (e.g., regex matching ^\s*margin\s*=\s*"?50.1"? on the graph attribute line only) and update the line reference.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/layouts/src/graphviz/GraphvizLayoter.ts` around lines 64 - 69,
normalizeDot currently removes lines by substring matching which is brittle;
update it to only remove the specific graph attribute line (not any occurrence
of the number) by either (preferred) having DotPrinter.createGraph emit a
sentinel comment next to the attribute (e.g., append a node like `/*
__GRAPH_CLUSTER_MARGIN__ */` when setting [_.margin]: GraphClusterSpace) and
then have normalizeDot filter only the line containing that sentinel, or
(alternative) change normalizeDot to use an anchored regex that only matches a
graph attribute line like /^\s*margin\s*=\s*"?${GraphClusterSpace}"?\s*;?$/ so
unrelated occurrences of the numeric value are not removed; also update the
trailing comment reference to point to the new location in
DotPrinter.createGraph. Ensure references: normalizeDot, DotPrinter.createGraph,
and GraphClusterSpace are used to find the code to change.

packages/layouts/src/graphviz/AiLayoutPrinter.ts (2)

74-75: 💤 Low value

Dead code: reduceDefaultRankSeparation is defined but never invoked.

The only call site in postBuild is commented out (line 74), so reduceDefaultRankSeparation() (lines 185–211) is unreachable. Either wire it into the build pipeline (and drop the comment) or remove it until it's needed; leaving disabled logic in place tends to drift from the rest of the printer.

Also applies to: 185-211

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/layouts/src/graphviz/AiLayoutPrinter.ts` around lines 74 - 75, The
function reduceDefaultRankSeparation is dead code because its only call in
postBuild is commented out; either re-enable the call by uncommenting and wiring
reduceDefaultRankSeparation() into the postBuild pipeline (ensuring any required
inputs/state in postBuild are available) or remove the
reduceDefaultRankSeparation method entirely. Locate postBuild and the
reduceDefaultRankSeparation definition in AiLayoutPrinter and either restore the
invocation (remove the comment and run tests/formatting) or delete the unused
method to keep the class clean.

---

37-67: 💤 Low value

O(N·M) reordering can be simplified with a Map.

Both selectViewNodes and selectViewEdges repeatedly findIndex + splice on the working array, which is O(N·M) and mutates a copy on each iteration. For large diagrams this is wasteful; an index map keeps it linear and is easier to read.

♻️ Suggested refactor

 protected override selectViewNodes(): Iterable<ComputedNode> {
-  if (!this.aiHints.nodeOrder) {
-    return this.view.nodes
-  }
-  const viewnodes = [...this.view.nodes]
-  const sorted: ComputedNode[] = []
-  for (const nodeId of this.aiHints.nodeOrder) {
-    const index = viewnodes.findIndex(n => n.id === nodeId)
-    if (index !== -1) {
-      sorted.push(...viewnodes.splice(index, 1))
-    }
-  }
-  sorted.push(...viewnodes)
-  return sorted
+  if (!this.aiHints.nodeOrder) {
+    return this.view.nodes
+  }
+  const byId = new Map(this.view.nodes.map(n => [n.id, n]))
+  const sorted: ComputedNode[] = []
+  for (const nodeId of this.aiHints.nodeOrder) {
+    const node = byId.get(nodeId)
+    if (node) {
+      sorted.push(node)
+      byId.delete(nodeId)
+    }
+  }
+  sorted.push(...byId.values())
+  return sorted
 }

Apply the same shape to selectViewEdges.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/layouts/src/graphviz/AiLayoutPrinter.ts` around lines 37 - 67, The
current selectViewNodes and selectViewEdges implementations repeatedly call
findIndex and splice on a copy of this.view.nodes/edges, causing O(N·M)
complexity; replace this with a Map-based approach: build a map from node/edge
id to the original item (using this.view.nodes and this.view.edges), then
iterate aiHints.nodeOrder / aiHints.edgeOrder to push items in that order from
the map (removing matched ids from the map as needed), and finally append any
remaining items from the map to preserve unspecified items; update symbols:
selectViewNodes, selectViewEdges, aiHints.nodeOrder, aiHints.edgeOrder,
this.view.nodes, this.view.edges, ComputedNode and ComputedEdge to implement the
O(N) reordering without mutating arrays via splice.

packages/layouts/src/graphviz/DotPrinter.ts (1)

357-380: 💤 Low value

Prefer string | undefined over string | false for reserveNodeId.

reserveNodeId returns string | false and generateGraphvizId does let id = reserve...; while (id === false) .... Using false as a sentinel mixes a boolean into a string-typed pipeline and is mildly confusing in a TypeScript-first codebase; string | undefined (or returning null) reads more naturally and aligns with the rest of the file (e.g., getGraphNode/getSubgraph returning T | null).

♻️ Suggested shape

-private reserveNodeId(name: string, isCompound = false): string | false {
+private reserveNodeId(name: string, isCompound = false): string | undefined {
   if (isCompound) {
     name = 'cluster_' + name
   } else if (name.toLowerCase().startsWith('cluster')) {
     name = 'nd_' + name
   }
   if (this.ids.has(name)) {
-    return false
+    return undefined
   }
   this.ids.add(name)
   return name
 }

 protected generateGraphvizId(node: NodeOf<V>) {
   const _compound = isCompound(node)
   const name = nameFromFqn(node.id).toLowerCase()
   let id = this.reserveNodeId(name, _compound)
   let i = 1
-  while (id === false) {
+  while (id === undefined) {
     id = this.reserveNodeId(name + '_' + i++, _compound)
   }
   return id
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/layouts/src/graphviz/DotPrinter.ts` around lines 357 - 380, Change
reserveNodeId to return string | undefined instead of string | false: update its
signature and return undefined where it currently returns false, and keep adding
to this.ids and returning the name on success. Update generateGraphvizId to
handle undefined by initializing let id = this.reserveNodeId(name, _compound)
and change the loop to while (id === undefined) { id = this.reserveNodeId(name +
'_' + i++, _compound) } (or use id == null) so the sentinel is undefined/null
rather than false; adjust any type annotations or callers expecting boolean to
match the new signature (reserveNodeId, generateGraphvizId, and references to
this.ids).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/mcp/src/tools/apply-semantic-layout.ts (2)

83-95: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard aiLayout() before dereferencing result.diagram.

This path assumes the layouter always returns a layout. If it returns a falsy value, result.diagram throws and the tool fails with an exception instead of an MCP error response.

Suggested fix

       const result = await languageServices.views.layouter.aiLayout({
         view: view.$view,
         styles: model.$styles,
       }, hints)
+      if (!result) {
+        await mcpServer.sendLoggingMessage({
+          level: 'error',
+          data: 'AI hints were generated but layout could not be applied',
+        }, ctx.sessionId)
+        return toolError('Generated layout hints but failed to apply them')
+      }

       await languageServices.editor.applyChange({
         change: {
           op: 'save-view-snapshot',
           layout: result.diagram,

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/mcp/src/tools/apply-semantic-layout.ts` around lines 83 - 95, The
call to languageServices.views.layouter.aiLayout may return a falsy value but
the code immediately dereferences result.diagram; update
apply-semantic-layout.ts to guard the aiLayout result (the variable result from
languageServices.views.layouter.aiLayout) before accessing result.diagram and
before calling languageServices.editor.applyChange: if result is falsy or
result.diagram is missing, short-circuit and return or throw an MCP-friendly
error (instead of calling applyChange with undefined layout), otherwise call
languageServices.editor.applyChange with change.op 'save-view-snapshot' and
layout set to result.diagram.

---

24-35: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Return a structured tool error for unknown projects.

ensureProjectId() can still throw before you reach the findView() guard, so an invalid projectId escapes as a raw MCP exception instead of a normal tool result.

Suggested fix

     async (args, ctx) => {
-      const projectId = languageServices.projectsManager.ensureProjectId(args.projectId)
+      let projectId: string
+      try {
+        projectId = languageServices.projectsManager.ensureProjectId(args.projectId)
+      } catch {
+        return toolError(`Project "${args.projectId}" not found`)
+      }
       if (projectId !== args.projectId) {
         await mcpServer.sendLoggingMessage({
           level: 'notice',
           data: `Using project "${projectId}" instead of "${args.projectId}"`,
         }, ctx.sessionId)
       }
-      const model = await languageServices.computedModel(projectId)
+      const model = await languageServices.computedModel(projectId)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/mcp/src/tools/apply-semantic-layout.ts` around lines 24 - 35,
ensureProjectId may throw for invalid project IDs so wrap the call to
languageServices.projectsManager.ensureProjectId(args.projectId) in a try/catch
and convert any thrown error into a structured tool result via toolError instead
of letting the exception escape; specifically, catch errors around
ensureProjectId, return toolError(`Project "${args.projectId}" not found` or
include the caught error.message), and otherwise proceed to call
languageServices.computedModel(projectId) and the existing view lookup
(referencing ensureProjectId, languageServices.computedModel, and toolError).

packages/vscode/src/node/useTelemetry.ts (1)

113-119: 💤 Low value

logError/logUsage are gated on isDev but not on vscode.env.isTelemetryEnabled.

vscode.env.createTelemetryLogger calls sendErrorData/sendEventData only when the user has telemetry enabled, so the telemetryLogger path is already protected. However, the legacy sendTelemetry and sendTelemetryError (also in the same return object) only check isDev and rely on TelemetryReporter to enforce the isTelemetryEnabled flag internally. This inconsistency is pre-existing and not introduced by this PR, but exposing two parallel telemetry APIs (logError/logUsage vs sendTelemetry/sendTelemetryError) with subtly different enforcement models may surprise callers.

Consider documenting the distinction (or unifying the guard logic) before these methods are adopted more broadly.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/vscode/src/node/useTelemetry.ts` around lines 113 - 119, The
returned telemetry methods expose two inconsistent guards: logError/logUsage
(bound to telemetryLogger) are only gated by isDev while
sendTelemetry/sendTelemetryError only rely on TelemetryReporter, so unify
behavior by checking vscode.env.isTelemetryEnabled (and preserving the existing
isDev short-circuit) for all four exports: wrap
telemetryLogger.logError/logUsage and the legacy
sendTelemetry/sendTelemetryError with a no-op when
!vscode.env.isTelemetryEnabled, or alternatively add a comment above the return
explaining the distinction between telemetryLogger (which internally respects
vscode.env.isTelemetryEnabled) and the legacy TelemetryReporter APIs; update
references to telemetryLogger, logError, logUsage, sendTelemetry,
sendTelemetryError and isDev accordingly so callers see consistent behavior or
clear documentation.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes