Skip to content

docs: Add architecture documentation (#60)#87

Merged
daviburg merged 7 commits into
Azure:mainfrom
daviburg:docs/architecture
May 13, 2026
Merged

docs: Add architecture documentation (#60)#87
daviburg merged 7 commits into
Azure:mainfrom
daviburg:docs/architecture

Conversation

@daviburg
Copy link
Copy Markdown
Member

Summary

Adds Docs/architecture.md — a standalone architecture document for the LSP server and VS Code extension.

Closes #60

What's included

  • Component map — VS Code extension lifecycle, server core (Program.cs, SdkIndex, BufferManager), all 6 handlers, 5 services, diagnostics pipeline with 6 validators, and the 3-slice state store
  • 5 Mermaid sequence diagrams — Startup, Document Edit→Diagnostics, Hover, Completion, Dynamic Values Resolution
  • Analysis strategy — syntax-only vs semantic model trade-off, with a link to validator authoring guidance in .github/copilot-instructions.md\
  • 4 ADRs — MetadataReferences over Assembly.Load, singleton CompilationService caching, syntax-only diagnostics, SDK discovery chain

Also updates README.md to link to the new doc (replaces the planned/issue placeholder).

Verification

  • \dotnet build --nologo\ ✅
  • \dotnet test --nologo\ ✅ (233 passed / 15 skipped / 0 failed — baseline unchanged)
  • No source code changes — documentation only

Copilot AI review requested due to automatic review settings May 13, 2026 06:15
@daviburg daviburg requested a review from a team as a code owner May 13, 2026 06:15
@daviburg daviburg self-assigned this May 13, 2026
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a standalone architecture document for the Connectors SDK LSP server and VS Code extension, and updates the README to link to it.

Changes:

  • Adds Docs/architecture.md with component maps, data flow diagrams, analysis strategy, and ADR-style design decisions.
  • Updates the README repository structure section to link to the new architecture documentation.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
README.md Replaces the planned architecture doc placeholder with a link to the new document.
Docs/architecture.md Documents extension/server architecture, diagnostics, state management, diagrams, and key design decisions.
Comments suppressed due to low confidence (2)

Docs/architecture.md:227

  • This link has the same relative-path issue as the earlier reference: from Docs/architecture.md, .github/copilot-instructions.md resolves under Docs/ instead of the repository root, so readers cannot navigate to the guidelines.
For detailed validator authoring guidelines covering this strategy, see the [Architecture: Diagnostic Validators](.github/copilot-instructions.md#architecture-diagnostic-validators) section in `.github/copilot-instructions.md`.

Docs/architecture.md:107

  • The startup diagram shows only the --sdk invocation, but the extension also starts the server with --sdk-assembly for resolved DLL paths. Please include both CLI paths here so the diagram matches startLanguageServer behavior and the next step's TryCreateFromAssembliesAsync branch.
    VSCode->>Server: Start process: dotnet SdkLspServer.dll --sdk <path>

Comment thread Docs/architecture.md Outdated
Comment thread Docs/architecture.md Outdated
Comment thread Docs/architecture.md Outdated
Copilot AI review requested due to automatic review settings May 13, 2026 06:29
@daviburg
Copy link
Copy Markdown
Member Author

[Dobby] Addressed all 3 review comments in commit f958d10:

Thread 1 (broken relative links, lines 81 & 227): Fixed both links from \.github/copilot-instructions.md\ to \../.github/copilot-instructions.md\ so they resolve correctly from the \Docs/\ directory.

Thread 2 (--sdk vs --sdk-assembly, lines 17 & 107): Updated step 5 and the Mermaid startup diagram to describe both CLI flags:

  • \--sdk-assembly ...\ when SDK resolution returns DLL paths (explicit \.dll\ setting or \project.assets.json\)
  • \--sdk \ when resolution returns a \.nupkg\ file

Thread 3 (semantic analysis in validators, line 225): Updated the analysis strategy section to mention that \DynamicValuesValidator\ and \SdkAntiPatternValidator\ also use \CompilationService.GetCompilation()\ for semantic checks, consistent with ADR 3.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Comment thread Docs/architecture.md Outdated
Comment thread Docs/architecture.md Outdated
Comment thread Docs/architecture.md Outdated
@daviburg
Copy link
Copy Markdown
Member Author

[Dobby] Addressed all 3 round-2 review comments in commit 511b6a2:

Thread (line 37 — cache invalidation): Removed the claim that TextDocumentSyncHandler invalidates the CompilationService cache. The description now clarifies: \CompilationService cache is replaced lazily when GetCompilation detects a changed text length/checksum — there is no explicit invalidation call.\

Thread (line 214 — dynamic values API path): Replaced the incorrect \GET /api/connectors/{connector}/{operation}\ with the two actual request paths shown as a Mermaid alt block: DirectClient calls the runtime URL using the discovered operation path/method; managed API calls POST to the ARM \/dynamicInvoke\ endpoint.

Thread (line 265 — anti-pattern as syntactic): Removed \�nti-pattern detection\ from the syntactic bullet and updated the semantic bullet to explicitly name \SdkAntiPatternValidator\ alongside \DynamicValuesValidator\, consistent with the analysis strategy section above.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comment thread Docs/architecture.md Outdated
Comment thread Docs/architecture.md Outdated
@daviburg
Copy link
Copy Markdown
Member Author

[Dobby] Addressed 2 late round-3 comments in commit 5f2f6d5:

Thread (line 16 — initializationOptions scope): Updated to accurately state the extension only sends API config and connections. Added parenthetical noting the server accepts CodeLens and telemetry config but the VS Code extension does not currently send those.

Thread (line 113 — LSP lifecycle): Fixed the Mermaid diagram to distinguish \initialize response (capabilities)\ from the separate \initialized notification\ that the client sends after. Also updated the server-side parse line to only mention apiConfig and connections.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comment thread Docs/architecture.md Outdated
Comment thread Docs/architecture.md Outdated
@daviburg
Copy link
Copy Markdown
Member Author

[Dobby] Addressed 2 round-4 comments in commit a4ee200:

Thread (line 106 — telemetry in diagram): Removed \ elemetry\ from the diagram's initializationOptions line. Now reads: \Build initializationOptions (API config, connections)\, consistent with the corrected prose on line 16.

Thread (line 290 — restore prompt): Narrowed the condition to: the extension prompts only when the project both references the SDK package AND is missing \obj/project.assets.json\ (i.e., \dotnet restore\ has not been run). Otherwise it starts without the SDK.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Comment thread Docs/architecture.md Outdated
Comment thread Docs/architecture.md Outdated
Comment thread Docs/architecture.md Outdated
@daviburg
Copy link
Copy Markdown
Member Author

[Dobby] Addressed 3 round-5 comments in commit 2fd6761:

Thread (line 20 — command IDs): Added the \connectorSdk.\ prefix to \
estartLanguageServer\ and \openConnectionView\. Now reads: \connectorSdk.restartLanguageServer\, \connectorSdk.openConnectionView\, \sdklsp.applyEdits\.

Thread (line 78 — DynamicValuesValidator): Changed from 'against live API data' to 'against cached dynamic values from \LSPStore.DynamicData\ (populated by hover/completion handlers; no network calls)'.

Thread (line 208 — discovery method): Changed from 'Analyze SDK types via reflection metadata' to 'Decompile SDK method bodies via ICSharpCode.Decompiler to extract API paths'.

@daviburg daviburg requested a review from Copilot May 13, 2026 06:54
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@daviburg daviburg merged commit 3c15c5a into Azure:main May 13, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add architecture documentation covering analysis strategy and design decisions

2 participants