src/main/java/com/github/_1c_syntax/bsl/languageserver/mcp/McpDocumentReader.java (2)

111-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider extracting the build/downgrade/cleanup logic into a helper method.

The four nested try/finally levels (writeLock build → downgrade → action → writeLock cleanup) are correct but hard to scan in one method. Extracting the "fresh AST" branch into a private helper (e.g. buildAndAnalyze(...), with cleanup in its own clearDocumentQuietly(...)) would make the lock lifecycle easier to verify at a glance.

♻️ Suggested extraction

private <T> T buildAndAnalyze(ServerContext serverContext, ReadWriteLock lock, URI uri,
                               Function<DocumentContext, T> action) {
  lock.writeLock().lock();
  var writeHeld = true;
  try (var ignored = WorkspaceContextHolder.forUri(serverContext.getWorkspaceUri())) {
    var documentContext = serverContext.addDocument(uri);
    serverContext.rebuildDocument(documentContext);

    lock.readLock().lock();
    lock.writeLock().unlock();
    writeHeld = false;
    try {
      return action.apply(documentContext);
    } finally {
      lock.readLock().unlock();
      clearDocumentQuietly(serverContext, lock, documentContext);
    }
  } finally {
    if (writeHeld) {
      lock.writeLock().unlock();
    }
  }
}

private void clearDocumentQuietly(ServerContext serverContext, ReadWriteLock lock,
                                   DocumentContext documentContext) {
  lock.writeLock().lock();
  try {
    serverContext.tryClearDocument(documentContext);
  } finally {
    lock.writeLock().unlock();
  }
}

🤖 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
`@src/main/java/com/github/_1c_syntax/bsl/languageserver/mcp/McpDocumentReader.java`
around lines 111 - 147, Extract the fresh-AST build, lock downgrade, action
execution, and cleanup from the current method into a private helper such as
buildAndAnalyze, preserving its lock and WorkspaceContextHolder lifecycle. Move
the write-locked tryClearDocument operation into a separate clearDocumentQuietly
helper, and have the existing method delegate to the new helper without changing
behavior.

---

133-141: 🚀 Performance & Scalability | 🔵 Trivial

Cleanup re-acquires writeLock synchronously before returning to the caller.

tryClearDocument runs in the finally before action.apply()'s result is actually returned, so the calling thread (likely the MCP request thread) blocks on writeLock().lock() if there's read contention on the same document, adding latency to the response path even though the computed result is already available. This is strictly better than the prior always-writeLocked design, but if cleanup latency ever becomes noticeable under concurrent load, consider offloading it to a background executor instead of blocking the caller.

🤖 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
`@src/main/java/com/github/_1c_syntax/bsl/languageserver/mcp/McpDocumentReader.java`
around lines 133 - 141, In the cleanup block of McpDocumentReader, avoid
synchronously reacquiring writeLock before returning action.apply()’s result.
Offload serverContext.tryClearDocument(documentContext) to the existing
background executor or an appropriate asynchronous cleanup mechanism, while
preserving best-effort cleanup and ensuring the caller returns the computed
result without waiting for lock contention.

src/test/java/com/github/_1c_syntax/bsl/languageserver/mcp/McpToolsTest.java (1)

142-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Strengthen the final assertion to actually validate type inference, not just deadlock-freedom.

TypeService.expressionTypesAt falls back to the non-null TypeSet.EMPTY sentinel when no expression is found at the given position, so assertThat(types).isNotNull() would still pass even if the line/position lookup silently missed the target expression. Given the PR description calls out validating the inferred &Пластилин type, consider asserting on the actual contents (e.g. non-empty, or containing the expected type name). Also, the line-search loop leaves line == -1 if "Логгер.Записать" isn't found, which surfaces as an opaque ArrayIndexOutOfBoundsException at lines[line] rather than a clear failure message.

♻️ Suggested strengthening

-        var position = new Position(line, lines[line].indexOf("Логгер") + 1);
+        assertThat(line).as("fixture line containing 'Логгер.Записать' not found").isNotEqualTo(-1);
+        var position = new Position(line, lines[line].indexOf("Логгер") + 1);

-    assertThat(types).isNotNull();
+    assertThat(types).isNotNull();
+    assertThat(types.isEmpty()).isFalse();

🤖 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 `@src/test/java/com/github/_1c_syntax/bsl/languageserver/mcp/McpToolsTest.java`
around lines 142 - 165, Strengthen the test around the expressionTypesAt call by
making the line lookup fail clearly when "Логгер.Записать" is absent, rather
than indexing with the initial -1 value. Replace the final non-null assertion on
types with an assertion that verifies the inferred result is non-empty and
contains the expected "&Пластилин" type name, while preserving the existing
timeout and interruption handling.