Merge pull request #55 from j-plugins/claude/intellij-platform-analysis-Uaqhj

Add comprehensive test suite covering indexes, references, and providers

Author: xepozz

.gitignore

@@ -4,4 +4,5 @@
 .intellijPlatform
 .kotlin
 .qodana
+.claude/worktrees/
 build

CHANGELOG.md

@@ -2,3 +2,21 @@
 
 # spiral-plugin Changelog
 
+## [Unreleased]
+
+### Added
+- Test suite covering indexes, references, line markers, folding, and run-configuration settings (30 new test classes; full `gradle test` is now meaningful).
+- `com.intellij.modules.json` added to `platformBundledPlugins` so the PHP plugin loads in the test sandbox (was the root cause of an empty effective extension set).
+- `VIEWS_RENDER_SIGNATURE` and `ENV_FUNCTION` / `AUTOWIRE` constants in `SpiralFrameworkClasses` — replaces hardcoded FQNs scattered across `ViewReferenceContributor`, `EnvFoldingBuilder`, and `ContainerReferenceContributor`.
+
+### Fixed
+- **Run-configuration settings corruption** — `SpiralConsoleCommandRunConfigurationSettings` aliased `workingDirectory` and `documentRoot` onto the `binary` XML attribute. Each property now persists under its own slot; existing saved configurations migrate cleanly.
+- **`ObjectStreamDataExternalizer.read` NPE** on null payloads — cast widened from `as T` to `as T?` so callers can decide how to handle null.
+- **CQRS implicit usage over-reporting** — `CqrsHandlersImplicitUsageProvider.isClassWithCustomizedInitialization` returned `true` unconditionally, marking every PHP class as having customized init.
+- **CQRS gutter icons missing** — `CqrsHandlersLineMarkerProvider.getLineMarkerInfo` matched on `PhpClass`, but the platform calls line markers on leaf elements only; re-anchored to the class-name identifier.
+- **Dumb-mode crashes** at three call sites that query `PrototypedIndex` / `CqrsIndexUtil` without `DumbService` guarding — `PrototypedCompletion`, `CqrsHandlersLineMarkerProvider`, and `PrototypedPropertyReference` now early-return while indexing.
+
+### Changed
+- `gradle.properties` `platformBundledPlugins` extended with the PHP plugin's transitive `com.intellij.modules.json` requirement.
+- Folding placeholder for `env('KEY')` calls uses the central `SpiralFrameworkClasses.ENV_FUNCTION` constant.
+

analysis/README.md

@@ -0,0 +1,65 @@
+# Per-file analysis of the Spiral Framework plugin
+
+This directory contains a static code review of every Kotlin source file in
+`src/main/kotlin/com/github/xepozz/spiral/`. Each report is in a sibling file
+whose path mirrors the source layout.
+
+Each report has the same structure (defined in [`_CONTEXT.md`](_CONTEXT.md)):
+short summary, then numbered issues with severity tag, file:line location,
+problem, why it matters, and a suggested fix.
+
+## Severity tag legend
+
+| Tag | Meaning |
+|---|---|
+| `[BUG]` | Behaves incorrectly under some real-world input. |
+| `[PERF]` | Slow / wasteful but functionally correct. |
+| `[API-MISUSE]` | IntelliJ Platform API used in a way the platform discourages. |
+| `[THREAD]` | Threading / EDT / read-action violation. |
+| `[DUMB-MODE]` | Index access not guarded against dumb mode. |
+| `[INDEX]` | `FileBasedIndexExtension` shape / versioning issue. |
+| `[LEAK]` | Disposer / lifecycle leak risk. |
+| `[I18N]` | Hardcoded user-visible string not via `SpiralBundle`. |
+| `[STYLE]` | Style / dead-code / commented `println`. |
+| `[MAINTAINABILITY]` | Readability / convention drift. |
+
+## High-signal hotspots
+
+These files have the most impactful findings — fix them first.
+
+### `[BUG]` — observable misbehavior
+- [console/run/SpiralConsoleCommandRunConfigurationSettings.md](console/run/SpiralConsoleCommandRunConfigurationSettings.md)
+- [console/run/SpiralRunCommandAction.md](console/run/SpiralRunCommandAction.md)
+- [cqrs/index/CqrsCommandHandlerIndex.md](cqrs/index/CqrsCommandHandlerIndex.md)
+- [cqrs/index/CqrsCommandIndex.md](cqrs/index/CqrsCommandIndex.md)
+- [cqrs/index/CqrsQueryHandlerIndex.md](cqrs/index/CqrsQueryHandlerIndex.md)
+- [cqrs/index/CqrsQueryIndex.md](cqrs/index/CqrsQueryIndex.md)
+- [prototyped/PrototypedPropertyReference.md](prototyped/PrototypedPropertyReference.md)
+- [references/FunctionsReferenceContributor.md](references/FunctionsReferenceContributor.md)
+- [router/endpoints/SpiralEndpointsProvider.md](router/endpoints/SpiralEndpointsProvider.md)
+- [views/injection/PHPLanguageInjector.md](views/injection/PHPLanguageInjector.md)
+
+### `[DUMB-MODE]` — `IndexNotReadyException` risk during indexing
+- [config/index/ConfigSectionIndex.md](config/index/ConfigSectionIndex.md)
+- [config/index/PrototypedIndex.md](config/index/PrototypedIndex.md)
+- [console/run/SpiralRunAnythingProvider.md](console/run/SpiralRunAnythingProvider.md)
+- [cqrs/CqrsHandlersLineMarkerProvider.md](cqrs/CqrsHandlersLineMarkerProvider.md)
+- [cqrs/CqrsIndexUtil.md](cqrs/CqrsIndexUtil.md)
+- [prototyped/PrototypedAttributeReferenceContributor.md](prototyped/PrototypedAttributeReferenceContributor.md)
+- [prototyped/PrototypedCompletion.md](prototyped/PrototypedCompletion.md)
+- [prototyped/PrototypedPropertyReference.md](prototyped/PrototypedPropertyReference.md)
+- [references/DirectoryReference.md](references/DirectoryReference.md)
+- [references/FunctionsReferenceContributor.md](references/FunctionsReferenceContributor.md)
+- [views/index/ViewsNamespaceIndexUtil.md](views/index/ViewsNamespaceIndexUtil.md)
+
+### `[LEAK]` — disposer / lifecycle risk
+- [router/endpoints/SpiralGroup.md](router/endpoints/SpiralGroup.md)
+
+## How this was produced
+
+This analysis was generated by spawning a code-review subagent per file, each
+seeded with the IntelliJ-Platform best-practices checklist in
+[`_CONTEXT.md`](_CONTEXT.md). Reports are independent — there is no
+cross-file deduplication of recurring patterns (e.g. the same
+`println`-in-production finding may appear in many files). Use the section
+links above to triage by severity.

analysis/SpellcheckingDictionaryProvider.md

@@ -0,0 +1,13 @@
+# Analysis: src/main/kotlin/com/github/xepozz/spiral/SpellcheckingDictionaryProvider.kt
+
+## Summary
+Implements the IntelliJ Platform's BundledDictionaryProvider to register a spellcheck dictionary for the plugin. This is a minimal extension that points to the bundled dictionary resource.
+
+## Issues
+
+### 1. [MAINTAINABILITY] Path to dictionary file should be a constant
+- Location: SpellcheckingDictionaryProvider.kt:6
+- Problem: The dictionary path `/META-INF/spellcheck.dic` is hardcoded as a string literal instead of being defined as a constant, making it fragile if the resource is ever relocated.
+- Why it matters: If the dictionary file moves, there's no single source of truth to update; the hardcoded string is easy to miss in refactoring.
+- Suggested fix: Extract the path as a companion object constant or object-level constant, similar to how `SpiralBundle.BUNDLE` is defined in `SpiralBundle.kt`.
+

analysis/SpiralBundle.md

@@ -0,0 +1,15 @@
+# Analysis: src/main/kotlin/com/github/xepozz/spiral/SpiralBundle.kt
+
+## Summary
+Provides centralized access to i18n messages via DynamicBundle, with both eager and lazy message retrieval methods. All user-facing strings in the plugin route through this singleton.
+
+## Issues
+
+### 1. [STYLE] Suppress("unused") on messagePointer without suppression scope
+- Location: SpiralBundle.kt:16
+- Problem: The `@Suppress("unused")` annotation is applied to the `messagePointer` method, but it appears to suppress the warning for the entire method definition rather than a specific element. The method itself may be unused from static analysis, but the suppression should clarify what is suppressed.
+- Why it matters: Overly broad suppressions can mask genuine unused code. If the method is indeed used elsewhere or reserved for future use, a comment explaining the suppression would improve maintainability.
+- Suggested fix: Either add a comment explaining why this method is kept despite being unused, or verify it is actually used in the codebase. If truly unused, consider removing it or documenting it as a utility for plugin consumers.
+
+No other significant issues found beyond this minor documentation enhancement.
+

analysis/SpiralFrameworkClasses.md

@@ -0,0 +1,7 @@
+# Analysis: src/main/kotlin/com/github/xepozz/spiral/SpiralFrameworkClasses.kt
+
+## Summary
+Central registry of Spiral framework FQN constants with leading backslash notation (`\Spiral\...`). All framework-specific class references throughout the plugin should use these constants rather than hardcoded strings.
+
+No significant issues found.
+

analysis/SpiralIcons.md

@@ -0,0 +1,13 @@
+# Analysis: src/main/kotlin/com/github/xepozz/spiral/SpiralIcons.kt
+
+## Summary
+Provides centralized icon access via `IconLoader.getIcon()`, serving as the single source of truth for the plugin's Spiral framework icon. Used throughout the plugin for completion items, gutter markers, and UI presentations.
+
+## Issues
+
+### 1. [API-MISUSE] IconLoader.getIcon() can return null; no null safety
+- Location: SpiralIcons.kt:7
+- Problem: `IconLoader.getIcon()` may return `null` if the resource is not found or unavailable, but the result is assigned directly to `val SPIRAL` with no null check. The field type is inferred as `Icon?` (nullable), which means all usages must handle potential null values.
+- Why it matters: Any code that accesses `SpiralIcons.SPIRAL` without null-safety could crash at runtime if the icon fails to load. Completion items, gutter markers, and other UI elements expecting a non-null Icon would throw NPE.
+- Suggested fix: Either (1) assert the result is non-null with `!!` and a comment if the icon is guaranteed to exist, (2) provide a fallback icon using `?:`, or (3) wrap access in a utility function that provides a safe default. Option 2 is preferred: `val SPIRAL = IconLoader.getIcon("/icons/spiral/icon.svg", javaClass) ?: AllIcons.General.Information` or similar.
+

analysis/SpiralViewUtil.md

@@ -0,0 +1,15 @@
+# Analysis: src/main/kotlin/com/github/xepozz/spiral/SpiralViewUtil.kt
+
+## Summary
+Utility object defining view-related constants: the `.dark.php` file suffix for Spiral views and a map of predefined directory aliases (public, vendor, runtime, etc.) used for resolving directory references in completion and navigation.
+
+## Issues
+
+### 1. [STYLE] PREDEFINED_DIRS should be immutable (val with unmodifiable Map)
+- Location: SpiralViewUtil.kt:6
+- Problem: `PREDEFINED_DIRS` is declared as a mutable `mapOf(...)` result, but `mapOf()` returns an immutable `Map`. However, the declaration reads as if it could be modified. The type is implicitly `Map<String, String>`, which is correct, but clarity could be improved.
+- Why it matters: While technically immutable, the absence of an explicit immutability annotation (like `val PREDEFINED_DIRS: Map<String, String> =`) leaves reader uncertainty about mutability intent.
+- Suggested fix: Add an explicit type annotation to clarify immutability: `val PREDEFINED_DIRS: Map<String, String> = mapOf(...)`. This makes the immutability contract explicit and aids IDE inference.
+
+No other significant issues found.
+

analysis/_CONTEXT.md

@@ -0,0 +1,98 @@
+# Shared analysis context — Spiral Framework plugin
+
+This file is shared context for per-file analysis subagents.
+
+## Project facts (from CLAUDE.md)
+- Language: Kotlin (JVM toolchain 21). IntelliJ Platform Gradle Plugin 2.x.
+- Plugin ID: `com.github.xepozz.spiral`. Mandatory deps: `com.intellij.modules.platform`, `com.jetbrains.php`.
+- Since-build: 251 (IDE 2025.1+). `platformType = IU`. PhpStorm-only APIs from `com.jetbrains.php` are available.
+- Tests: JUnit 4 + `BasePlatformTestCase`.
+- All Spiral FQNs MUST live in `SpiralFrameworkClasses.kt` with leading backslash (`\Spiral\...`).
+- All user-facing strings MUST go through `SpiralBundle.message(...)`.
+- Icons via `SpiralIcons` only.
+- No `println` in new code — use `com.intellij.openapi.diagnostic.Logger`.
+- Indexes extend `common/index/AbstractIndex.kt`, must bump `getVersion()` when output shape changes.
+- `ObjectStreamDataExternalizer` uses Java serialization — value classes must stay `Serializable`.
+- Configuration cache enabled — don't break it.
+
+## IntelliJ Platform best-practices checklist (use as a lens when reviewing)
+
+### Threading & locks
+- Long PSI traversal, VFS scans, index queries must NOT run on EDT.
+- Reading PSI/VFS requires a read action (or smart-mode read access). Writing requires a write action on EDT.
+- Re-validate PSI / VirtualFile (`isValid()`) when re-entering a read action after suspension.
+- `ProcessCanceledException` must NEVER be swallowed — always rethrow. Catch only specific exceptions.
+- Avoid `runReadAction` on EDT for heavy work — prefer `ReadAction.nonBlocking(...).submit(...)` or background actions.
+
+### Dumb mode & indexes
+- `FileBasedIndex.getInstance().getValues(...)` / `getAllKeys(...)` is forbidden in dumb mode — guard with `DumbService.isDumb` or `runReadActionInSmartMode`.
+- `LineMarkerProvider`, `ReferenceContributor`, `CompletionContributor` may be invoked during indexing — handle dumb mode.
+- Concrete `FileBasedIndexExtension` must bump `getVersion()` if the indexed shape changes (keys, value class, input filter, key descriptor).
+- `dependsOnFileContent()` defaults to true in `AbstractIndex` — fine for content indexes, but path-only indexes should override.
+- Value externalizer using `ObjectStreamDataExternalizer` (Java serialization) is fragile across IDE/plugin versions — prefer `DataExternalizer<T>` with explicit `save`/`read`.
+
+### PSI references
+- `PsiReference.resolve()` must be fast — cache where possible, use `CachedValuesManager` for repeated resolves.
+- `PsiReferenceContributor` patterns should be specific to avoid invocation on every PSI element.
+- `getRangeInElement()` must be relative to the host element (not absolute).
+- Multi-resolve references should implement `PsiPolyVariantReference`.
+- `getVariants()` returning many items: use `LookupElementBuilder` and consider `withIcon`, `withTypeText`, `withTailText` for UX.
+
+### Completion contributors
+- `addCompletions` must be fast; avoid blocking I/O.
+- Always check `result.isStopped`/`ProgressManager.checkCanceled()` in long loops.
+- `extend(...)` patterns should be as specific as possible (use `psiElement().withParent(...)`).
+- Don't call `result.stopHere()` unless intentionally suppressing other contributors.
+
+### LineMarkerProvider
+- `LineMarkerProvider.getLineMarkerInfo` is invoked on the leaf element (not the whole class/function) — anchor to identifier, not container.
+- `RelatedItemLineMarkerProvider` is the modern alternative.
+- Marker must be cheap — heavy resolution should be deferred via `NavigationGutterIconBuilder`.
+
+### Run configurations
+- `ConfigurationFactory.getId()` must be stable across versions — changing it breaks user-saved configs.
+- `RunConfiguration.checkConfiguration()` should throw `RuntimeConfigurationError` / `RuntimeConfigurationWarning`.
+- `RunProfileState.execute()` should not block EDT.
+- Read/write external state via `JDOMExternalizer` / explicit XML; `Element` content is persisted to workspace.xml.
+
+### Service lifecycle / Disposers
+- Never use `Application` or `Project` directly as a Disposable parent inside plugin code — use the plugin's service.
+- Listeners registered without a `Disposable` parent leak across dynamic-plugin reloads.
+- Project services should be `@Service(Service.Level.PROJECT)`; app services `@Service(Service.Level.APP)`.
+
+### Other quality concerns
+- Hard-coded user-visible strings (must be in `SpiralBundle`).
+- Hard-coded FQNs scattered around (must be in `SpiralFrameworkClasses`).
+- `println` / `System.out` — replace with `Logger.getInstance(...)`.
+- `!!` on platform-returned nullables — risky; use `?:` or graceful fallback.
+- Companion-object constants should be `const val` when possible.
+- Unused imports / unused parameters / dead code.
+- Public visibility on classes that should be `internal`.
+- `var` instead of `val` where mutation isn't needed.
+
+## Output format expected from each subagent
+
+Each subagent writes ONE markdown file at the absolute path it is given, with this structure:
+
+```
+# Analysis: <relative source path>
+
+## Summary
+1-3 sentences describing the file's responsibility.
+
+## Issues
+For each issue: a numbered heading with severity tag.
+
+### 1. [SEVERITY] Short title
+- Location: <relative path>:<line range>
+- Problem: <what's wrong>
+- Why it matters: <impact>
+- Suggested fix: <concrete remediation>
+
+Severity tags: [BUG], [PERF], [API-MISUSE], [STYLE], [MAINTAINABILITY], [I18N], [THREAD], [DUMB-MODE], [INDEX], [LEAK].
+
+## No-issue note
+If nothing meaningful is found, write "No significant issues found." and stop.
+```
+
+Keep findings concrete, with file:line references. Don't speculate about issues that aren't present.

analysis/common/index/AbstractIndex.md

@@ -0,0 +1,21 @@
+# Analysis: common/index/AbstractIndex.kt
+
+## Summary
+This is a minimal abstract base class for file-based indexes used across the plugin. It provides default implementations for key descriptor, content dependency, and version handling, requiring concrete subclasses to implement name, input filter, and the indexer itself.
+
+## Issues
+
+### 1. [INDEX] Missing version override guidance in subclasses
+- Location: AbstractIndex.kt:21
+- Problem: The class defines `getVersion() = 1` as a static default, but the CLAUDE.md documentation explicitly states "Bump `getVersion()` in the concrete index whenever you change the indexer output shape." This base implementation provides no mechanism or comment to enforce version bumping, and subclasses may inherit the version value without realizing it needs to change.
+- Why it matters: File-based index version mismatches cause stale caches to corrupt results. Silently inheriting version 1 across all subclasses means any index logic change risks silent data corruption.
+- Suggested fix: Mark `getVersion()` as `open` and add a KDoc comment above it warning subclasses to override and increment when the indexer output shape changes (keys, value type, input filter).
+
+### 2. [MAINTAINABILITY] Ambiguous default for dependsOnFileContent
+- Location: AbstractIndex.kt:15
+- Problem: `dependsOnFileContent() = true` is hardcoded as the default. Per the CLAUDE.md conventions, "path-only indexes should override" this. However, there is no documentation at the class or method level indicating which indexes should or should not depend on file content.
+- Why it matters: Path-only indexes (e.g., detecting file names without examining content) will inefficiently rebuild on every file content change. Developers may not realize they need to override.
+- Suggested fix: Add a KDoc comment explaining that content-dependent indexes can use the default, but path-only indexes must override with `override fun dependsOnFileContent() = false`.
+
+## No-issue note
+The core design is sound for a minimal base class. Both issues are about improving guidance rather than structural flaws.