chore(core): backend stabilization — ARCH rules, comment hygiene, decomposition (pre-UX) - #163
Merged
Conversation
…group 1) Strip spec/process tracking metadata from non-test source comments across all three packages, keeping the genuine why-explanations. Removes Slice/FR/US/T0xx/ S8/M7/CodeRabbit/CodeQL/"this slice" tags; rewrites comments that named the four extractor classes deleted in the indexing rewrite to describe the generic engine. Also fixes three user-facing strings that leaked slice numbers into the UI (Mermaid sequence-diagram unavailable label, decorator-classification label, sequence-preview message) and adds an ESLint `no-restricted-imports` rule banning `vscode` in `packages/core/src` (RULE-ARCH-005), making the boundary self-policing. Comments only — no code logic changed. Global spec-tag grep is now 0 in non-test source. typecheck 5/5, tests unchanged (core 263 / exporters 161 / extension 638), lint green with the new ban active. The `@duckdb/node-api` ban is deferred to group 3, after storage access moves behind the repository adapter.
…group 2) extractImportRefs matched only the TS/JS `import_statement` node type and hardcoded language:"typescript", so Python/Go/Rust/Java/etc. emitted zero IMPORTS edges (its sole caller runs for every language). Drive import extraction off the provider (RULE-ARCH-009). - ProviderConfig.importNodeTypes: each language declares its grammar's import node types; the engine reads the specifier and emits IMPORTS generically. - extractImportRefs tags the edge with the file's language and keeps the raw specifier when TS-style path resolution doesn't apply, so the edge always exists; the generator emits importNodeTypes per language. - Tests: Python/Go/Rust each produce language-tagged IMPORTS edges; TS unchanged. typecheck 5/5; core 266 (+3), exporters 161, extension 638; lint green.
…RULE-ARCH-003) Isolate the database driver so the rest of core depends on an abstraction, not @duckdb/node-api directly. - storage/adapters/duckdb.ts: the only module allowed to import the driver. Owns instantiation (openDatabase / openReadOnlyDatabase / runInTransaction / readRows) and re-exports the connection type as GraphDbConnection (+ GraphDbResult / GraphDbValue). - storage/db.ts is now a thin facade re-exporting the adapter; all 15 storage + query modules import GraphDbConnection from it instead of the driver. - workspaceRegistry's two inline foreign-DB opens go through openReadOnlyDatabase. - ESLint: ban @duckdb/node-api in packages/core/src except storage/adapters/** (alongside the existing no-vscode ban) — both boundaries now self-policing. Scope note: a full method-per-query GraphRepository interface was a multi-day, high-risk rewrite and is deferred to its own PR; this delivers RULE-ARCH-003's intent (driver isolated to the adapter, modules off the driver, lint-enforced) with zero behavior change. Pure relocation: typecheck 5/5, tests unchanged (core 266 / exporters 161 / extension 638), lint green with both bans active. Public Indexer contract unchanged — no consumer edits.
…ARCH-004/010) Implement two declared-but-unused architecture rules as real interfaces. RULE-ARCH-004 — serializer strategy: - SubgraphSerializer interface + a SERIALIZERS registry keyed by diagram type in scopedSerializer.ts; the hardcoded `switch (options.diagram)` becomes a registry lookup, so adding a diagram type is registration, not a new switch arm. Behavior-preserving (same three serializers). RULE-ARCH-010 — resolver interface: - core gains PreciseLocationResolver (+ shared NodeLocation / PreciseCallEdge): a position-based precise-resolver contract, since the LSP answers by file+position, not by graph node id (core's existing CallResolver is the node-id/heuristic contract). LspCallResolver now `implements PreciseLocationResolver` — the host/core seam is real and IntelliJ-portable. - deduped the webview protocol's PreciseCallEdgeResult to alias core's PreciseCallEdge (RULE-ARCH-007 — one shape across resolver/message/webview). - SCIP ingest is a bulk index importer, not a per-node resolver, so it stays as its own shape rather than be forced into either interface (documented). typecheck 5/5; tests unchanged (266/161/638); lint green; additive, no consumer ripple.
…/007) Make the edge-metadata seam typed and the public boundaries fail-fast. RULE-ARCH-007 — typed metadata: - storage/edgeMetadata.ts: typed EdgeMetadata + EDGE_META_KEYS constants + metaPath() helper. The engine writer and all 10 resolution-SQL sites now reference the shared constants instead of duplicated string literals, so a drifted key is a single edit and a TS typo is a compile error. RULE-ARCH-006 — boundary hardening: - DuckTreeIndexer.indexFile rejects non-absolute paths; neighborhood rejects an empty nodeId — fail fast with a descriptive error rather than a deep fs/SQL failure later. - watcher re-index failure now logs at error (was debug — silent staleness); panel workspace-list + precise-calls catches log before degrading. - synthesizeFolderTree's load-bearing `!!` assertions replaced with a guard that throws a clear invariant error. Deferred: 5.2 typed serializer result (a serializer throw-contract change with its own test ripple) → its own follow-up. typecheck 5/5; tests unchanged (266/161/638); lint green. Behavior-preserving (the metaPath SQL is byte-equivalent to the prior quoted literals).
Behavior-preserving structure cleanup with clean seams; GraphView.tsx + activate decomposition deferred to the UX phase (that phase reworks GraphView, so splitting it now would be done twice). - storage/repository.ts 915→507 LOC: extracted resolution.ts (resolveCallEdgeSymbols / stampResolutionTier / resolveWorkspaceCrossFileEdges) and folderTree.ts (folderId / synthesizeFolderTree). Re-exported so index.ts is untouched. - query/subgraph.ts: extracted queryResolvedEdges() — the four byte-identical CALLS/INHERITS/INSTANTIATES/IMPLEMENTS query blocks (~95 LOC) collapse to four calls. - webview/panel.ts: navigate is now an explicit branch and unknown message types are logged instead of silently falling through to navigate-validation. typecheck 5/5; tests unchanged (266/161/638); lint green. Zero behavior change.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (9)
📒 Files selected for processing (79)
📝 WalkthroughSummaryUser-visible and reviewer-relevant changes:
Risks and coverage gaps:
Deferred validation:
Package & schema boundaries:
Test status: Core 266, exporters 161, extension 640 remain green. Behavior-preserving except multi-language import fix and symbol-tree nesting feature. WalkthroughSystematic backend stabilization that isolates the DuckDB driver, abstracts database connections, standardizes edge metadata, enables multi-language import extraction, adds graph resolution and folder synthesis, refactors Mermaid serialization to a registry-based non-throwing API, centralizes lens logic, nests symbol trees, and removes all internal spec-tracking tags. ChangesBackend stabilization & frontend unification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Rationale: Large scope spanning storage abstraction, multi-file connection-type migration, edge metadata refactoring, new resolution logic, Mermaid serializer rewrite, and concurrent frontend changes (lens hook, symbol nesting, export shortcuts). High density of interconnected changes across 80+ files. No single file dominates, but several critical paths (adapter layer, GraphDbConnection propagation, serializer registry) demand careful review. Moderate logic density; most changes are systematic refactors with clear intent. Extensive comment cleanup makes signal harder to spot but doesn't add review complexity. Possibly related PRs
Suggested labels
Poem
|
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
… re-run (5.2)
The preview router caught the serializer's Error, regex-matched its message to
reconstruct the failure status, and re-ran the whole extract→collapse→validate
pipeline to recover the soft-cap warning. Replace both with typed data.
- scopedSerializer: add serializeScopedMermaidResult() returning
{ source, validation } (ScopedExportValidation). source is non-null exactly
for ok/warning; null for empty/oversized/unsupported. serializeFlowchart is now
a thin throwing wrapper over the result-returning serializeFlowchartResult.
- preview.ts: generateMermaidPreview branches on validation.status and reads the
soft-cap warning directly — no try/catch-and-regex (G4), no pipeline re-run (G5).
typecheck 5/5; tests unchanged (266/161/638); lint green. Behavior-preserving.
… partial) Begin decomposing GraphView.tsx (1747 LOC) by lifting the self-contained lens concern into a hook. - hooks/useGraphLenses.ts: owns activeLensId state + all lens-derived values (lensCounts, lensMatchSet, lensColorOf, onLensToggle, lensResultRows) and the formatLensMetric helper. Reads the live graph via the shared ref. - GraphView consumes it via one destructured call; ~60 LOC + 5 lens imports removed from the component (1747 → 1680). The remaining concerns (search, trace, layout, selection) are more entangled with selection state + Sigma refs; they're left for the UX phase that reworks GraphView, to avoid decomposing twice. Behavior-preserving: typecheck 5/5, tests 266/161/638, lint green.
…6.4, partial)
The selection-aware Mermaid export shortcuts (callers / callees / class hierarchy
/ package) were inline in activate's subscriptions block. Lift them into a small
factory.
- commands/exportShortcuts.ts: createExportShortcutCommands({ getIndexer,
openMermaidPreview, pickSymbol }) returns the four disposables; the three
symbol-scoped ones share one pickSymbol→startInferredMermaidExport helper.
- activate spreads the factory result instead of four inline command bodies
(548 → 519 LOC).
Scope note: the rest of the command wiring is already factored into commands/*
modules and is composed in activate (the correct place for a composition root);
a registerCommands(...) split would need a ~14-dependency bag for cosmetic gain,
so it's not done. Behavior-preserving: typecheck 5/5, tests 266/161/638, lint green.
Adds scripts/check-no-spec-tags.mjs and a static-checks step running it via pnpm lint:spec-tags. Asserts zero FR-/TR-/slice-/CodeRabbit-style tracking tags in packages/**/*.ts(x), excluding test-support dirs (__fixtures__, __fuzz__, *.test/spec/fuzz) where naming the pinned requirement is legitimate. Prevents the comment-hygiene cleanup from Group 1 from rotting back.
These optional fields were readonly unknown[] with a producer (GenericTagsExtractor emitted []) and an aggregator (registry forwarded them) but zero consumer — the indexer only reads result.annotations, never .modules/.tests. Untyped, unwired, dropped on the floor. Remove the field, the 4 registry sites, the producer's empty arrays, and the test helper. recomputeGraphHealth (the other half of 8.1) is kept: it is a documented public seam (referenced by lenses.ts/repository.ts comments + design.md 7.5), not dead code. Wiring it needs the unimplemented algorithm; unexporting would break the seam.
…on guard (8.2) clearWorkspace/clearFile/clearAll hand-rolled BEGIN/COMMIT/ROLLBACK, bypassing runInTransaction's defensive pre-flight rollback, nesting guard, and logger hooks. Route all three through it (clearAll's post-commit initializeSchema stays outside the transaction). All three are top-level callers, so the nesting guard never trips. Move the transaction-in-flight flag off a module-global boolean onto a WeakMap<connection> so the nesting guard is per-connection: a process holding several handles (writable workspace DB + read-only foreign DBs) can no longer have one connection's transaction flip another's guard. Signature unchanged. Document pathAliasCache as module-global + never-evicted (bounded in practice; no clear-on-workspace hook like frameworkCache has).
…s serializer (8.3) - Add firstCapBreach() in validator.ts; adopt it in the class-diagram validator to collapse the two byte-identical oversized branches (classes/methods) into one labeled-dimension check. The sequence validator keeps its single combined message (one check, no benefit from the helper); the scoped validator is two-tier (soft+hard) and is not a consumer — forcing all three under one type would be a worse abstraction than the explicit per-diagram unions. - sequenceDiagram: build an id->node / id->edge index once per pass instead of .find over the whole subgraph inside the trace loops (was O(trace*subgraph)). - Split serializeToClassDiagram into emitClassBodies + collectRelationshipLines; the top function is now a readable assembly. Output byte-identical. All behavior-preserving: exporters 161/161, typecheck clean.
…tial)
Make the diagram / granularity / direction vocabularies canonical as-const
arrays in scopedSerializer.ts (MERMAID_DIAGRAMS / _GRANULARITIES / _DIRECTIONS),
with each type derived from its array so values and type can't drift. panel.ts's
VALID_* host-side validation Sets and the exporters fuzz harness now derive from
them instead of re-listing the literals (RULE-ARCH-007). Theme stays separate:
the webview protocol's lowercase light/dark differs from the serializer's
MermaidTheme ('Light'/'Dark'/'Print'). The MermaidPreviewPanel dropdowns keep
their explicit {value,label} lists — values are already type-bound to the
canonical types, and the lists carry deliberate disabled/partial UI states.
The WebviewPanelManager singleton-encapsulation half of 8.4 is deferred to the
UX phase: it's ~30 call sites across extension.ts/panel.ts, the same files the
UX rework touches (cf. deferred 6.3/6.4). Group 8 is optional spin-offs.
Behavior-preserving: exporters 161/161, extension 638/638, typecheck clean.
…8.5) The symbols tree returned a file's symbols flat — a class and its methods as siblings. Nest them via the existing StoredSymbol.enclosingSymbolId: a class/ interface renders collapsible with its members as children, member-less symbols stay leaves. A symbol whose enclosing id is absent from the file is surfaced at top level rather than dropped; a malformed enclosing cycle is guarded so the recursion can't loop. getChildren returns a symbol node's nested members instead of always []. Extension 640/640 (two new tests for nesting + orphan).
dgtalbug
marked this pull request as ready for review
June 7, 2026 10:37
This was referenced Jun 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The pre-UX stable cut from the stability audit (
scratch/stability-audit/). Implements the architecture rules that.dextree/rules.mddeclared but the code never enforced, fixes a multi-language correctness bug, removes spec/process comment rot, decomposes backend god-objects, and clears the opportunistic P3 backlog. Every group is behavior-preserving and the full test suite stays green throughout (core 266 / exporters 161 / extension 640).Linked spec / issue
openspec/changes/backend-stabilization/(OpenSpec change; openspec/ is gitignored)Slice classification
refactor— internal change, no behavior delta (onefixfor the import bug, one additivefeatfor symbol-tree nesting)Affected surfaces
packages/corepackages/extension(VS Code host)packages/exportersGroups (commit-by-commit)
ff4f4ebcomment hygiene (122→0 spec tags, 3 user-facing slice-leak fixes) + ESLintno-vscode-in-core4630091fix: multi-language imports — IMPORTS edges for Python/Go/Rust/etc., not just TS/JS1269c97DuckDB driver confined tostorage/adapters/+@duckdb/node-apiESLint ban (RULE-ARCH-003)bb901dfserializer strategy registry (RULE-ARCH-004) +PreciseLocationResolverinterface (RULE-ARCH-010)e6a98c0typedEdgeMetadata+ key constants (RULE-ARCH-007) + boundary hardening (RULE-ARCH-006)ff16c09backend decomposition (repository.ts 915→507 via resolution.ts/folderTree.ts; subgraph edge-query extract; panel unknown-type branch)f02af8b5.2 typed serializer result{ source, validation }— preview router stops reconstructing status by regex overError.messageand stops re-running the pipeline for the soft-cap warning9ae8ddb6.3 (partial) extractuseGraphLenseshook fromGraphView.tsx(1747→1680)e2935bb6.4 (partial) extractcreateExportShortcutCommandsfactory fromactivate(548→519)51cfb831.6 CI guard (scripts/check-no-spec-tags.mjs+ static-checks step) so the comment-hygiene cleanup can't rot back7f3fe318.1 drop unusedExtractionResult.modules/.tests(producer+aggregator, zero consumer); keeprecomputeGraphHealthas a documented seamdf5c52c8.2 routeclear.tsthroughrunInTransaction; per-connection transaction guard (WeakMap<connection>, was a module-global boolean)9d5b86d8.3firstCapBreachcap helper (class-diagram); indexed sequenceDiagram lookups (was O(trace·subgraph)); splitserializeToClassDiagrame4c19d28.4 (partial) single source of truth for Mermaid option strings (RULE-ARCH-007) — canonical as-const arrays, types derived, validators/fuzz derive from them90d741a8.5 nest methods under their enclosing class inSymbolsTreeProviderviaenclosingSymbolIdTest plan
pnpm checkgreen (prettier + eslint --max-warnings=0 + lint + test + typecheck) on the committed treevscodein core, no@duckdb/node-apioutsidestorage/adapters/pnpm lint:spec-tags)pnpm dedupe --checkclean (no deps added)Deferred to follow-ups (documented in the change)
WebviewPanelManagersingleton encapsulation (the other half of 8.4) → the UX phase: ~30 call sites across extension.ts/panel.ts, the same files the UX rework touches (same rationale as the remaining 6.3/6.4 hooks). The Mermaid-option single-source half shipped here.registerCommands) → the UX phase, which reworks GraphView; splitting all of them now would be done twice. The clean seams (lens hook, export-shortcuts factory) shipped here.GraphRepositoryinterface → its own PR. Design is written (scratch/stability-audit/graph-repository-design.md, grounded in the real query/storage surface). This PR already delivers ARCH-003's intent: the driver is isolated to the adapter and ESLint-enforced.scratch/stability-audit/findings-outline.md).Notes for reviewers
.dextree/rules.mdbut never implemented — this makes them real and lint-enforced.recomputeGraphHealthis a documented seam, not dead code (8.1);MermaidThemeis intentionally a separate vocabulary from the webview's lowercase theme (8.4).recomputeGraphHealth-style judgments and per-package findings:scratch/stability-audit/(gitignored, local).🤖 Generated with Claude Code