code_review: обёртка BSL Language Server для метрик кода - #329
Conversation
Новый тул code_review. Стандарты РАЗРАБОТКИ (пакет v8-code-style) уже показываются через get_project_errors; но метрики кода (магическое число, цикломатическая/когнитивная сложность, длина метода/строки, вложенность) в EDT нет вообще. Своих правил не пишем - оборачиваем внешний движок BSL Language Server (github.com/1c-syntax/bsl-language-server, LGPL-3.0). Архитектура: движок = ВНЕШНИЙ jar (не в git, не в bundle) - подпроцесс `java -jar exec.jar --analyze --reporter json`, найденный через env EDT_MCP_BSL_LS_JAR (+ EDT_MCP_BSL_LS_JAVA для выбора Java, т.к. линейка 1.x движка собрана под Java 21, а 0.28.x - под Java 17). Обновление движка = подмена файла без пересборки плагина. Конфиг проверок - проектный .bsl-language-server.json, иначе дефолты движка. Вывод рассчитан на авто-ремедиацию агентом: таблица Severity|Rule| Module path|Line|Message|Docs плюс явная инструкция "чини на Module path+Line через write_module_source, потом перезапусти code_review для проверки". Module path релятивизован к src/ - те же координаты, что read/write_module_source. BslLsReport - чистый парсер JSON-отчёта движка (без IO): конвертирует LSP 0-based строку/колонку в 1-based, нормализует путь (движок пишет file:// с ../ относительно своего рабочего каталога), маппит severity. BslLsRunner - подпроцесс: поиск jar/Java, запуск, парсинг, actionable-ошибка при отсутствии движка вместо исключения. Тесты: unit (BslLsReportTest, BslLsRunnerTest, CodeReviewToolTest) + e2e (happy-пути дают skip, если движок не настроен в окружении - специально для CI без внешней зависимости; негативные сценарии детерминированы и не требуют движка). Зарегистрирован в тулсете PROJECT. Примечание: README-индекс тулов (docs/tools/README.md, таблица в README.md) не тронут в этом PR - это общий сгенерированный блок, конфликтующий между параллельными PR; предлагаем регенерировать один раз после мержа нескольких PR (python docs/generate_tool_docs.py против живого сервера).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bae416f47d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (config != null) | ||
| { | ||
| command.add("--configuration"); //$NON-NLS-1$ | ||
| command.add(config.getAbsolutePath()); |
There was a problem hiding this comment.
When a project has .bsl-language-server.json diagnostics settings such as disabled rules or tuned thresholds, this passes that file via BSL LS --configuration, which the upstream docs describe as the global config path while diagnostics are applied from the workspace-local config in the workspace root; because this runner also never supplies --workspaceDir, the project-root config is not loaded as that local workspace config, so code_review reports findings using default rule settings instead of the project’s configured ones. The analyze help documents --workspaceDir as the project directory for source paths, so the wrapper should run with the EDT project root as the workspace/local-config root and reserve --configuration for true global config.
Useful? React with 👍 / 👎.
| md.append("**").append(report.total()).append("** finding(s): ") //$NON-NLS-1$ //$NON-NLS-2$ | ||
| .append(report.count(Severity.ERROR)).append(" error, ") //$NON-NLS-1$ | ||
| .append(report.count(Severity.WARNING)).append(" warning, ") //$NON-NLS-1$ | ||
| .append(report.count(Severity.INFORMATION)).append(" information, ") //$NON-NLS-1$ | ||
| .append(report.count(Severity.HINT)).append(" hint.\n\n"); //$NON-NLS-1$ |
There was a problem hiding this comment.
When modulePath points at a module in a metadata object directory that also contains sibling modules such as ObjectModule.bsl and ManagerModule.bsl, the runner analyzes the whole parent directory and filters only the table rows back to the exact target file, but this summary still uses the unfiltered report totals. A single-module review can therefore say there are findings, or show counts from sibling modules, even when the requested module is clean; the summary/clean-state decisions need to be based on the target-file filtered findings for module-scoped runs.
Useful? React with 👍 / 👎.
| String targetAbsPath = null; | ||
| if (modulePath != null && !modulePath.isEmpty()) | ||
| { | ||
| IFile moduleFile = BslModuleUtils.resolveModuleFile(project, modulePath); |
There was a problem hiding this comment.
For calls where modulePath is an absolute workspace path (accepted by BslModuleUtils.resolveModuleFile) or resolves through its non-src fallback, the file handle can belong outside this projectName's src tree, but the tool still sets scopeDir to that file's parent and analyzes it under the requested project's heading/config. This lets a module-scoped review silently inspect a different project than the one the user named; after resolving the handle, reject any target whose location is not under this project's srcRoot.
Useful? React with 👍 / 👎.
DitriXNew
left a comment
DitriXNew
left a comment
There was a problem hiding this comment.
Нужны изменения перед merge.
Полезность интеграции понятна, но сейчас заявленный контракт «метрический слой поверх get_project_errors» не обеспечен, а рабочая интеграция с реальным движком в CI фактически не проверяется.
Блокирующие пункты:
- CI красный: не обновлён
tests/e2e/tools_list.golden.json. Для нового публичного tool также не обновлены README: счётчик «67 tools», таблица групп, Available Tools, список типов ответа иdocs/tools/README.md. code_reviewдобавлен только вToolsets.PROJECT, но отсутствует вpreferences/ToolGroup. Поэтому вкладка Tools, групповые переключатели, disable-all и пресеты не знают об инструменте. Добавьте его в подходящую группу (по смыслу скорее BSL_CODE либо явно обоснуйте Problems) и тест на согласованность обоих реестров.- Все четыре happy-path E2E с настоящим BSL LS в текущем CI имеют SKIPPED; прошли только негативные сценарии. Значит обещанная совместимость 0.28.x/Java 17 и 1.x/Java 21, CLI, рабочая директория, формат путей и project config не подтверждены. Нужна обязательная матрица хотя бы по одной версии каждой поддерживаемой ветки либо детерминированный subprocess fixture плюс отдельная live matrix.
- Уже оставленные замечания о выходе
modulePathза пределыsrc/и о неверной summary при module/rule/severity-фильтрах остаются актуальными; не дублирую их inline.
Кроме того, runner запускает тяжёлый внешний JVM без ограничения конкурентности. Несколько параллельных вызовов MCP способны одновременно поднять несколько BSL LS и выбить EDT по памяти. Нужен хотя бы bounded semaphore/single-flight и понятная ошибка/очередь.
Inline оставил четыре независимых замечания с конкретными вариантами исправления.
| String ruleNeedle = rule == null ? null : rule.toLowerCase(Locale.ROOT); | ||
|
|
||
| List<Finding> filtered = new ArrayList<>(); | ||
| for (Finding f : report.findings()) |
There was a problem hiding this comment.
Здесь наружу без отбора попадают все diagnostics движка, хотя описание обещает только метрики, которых нет в EDT. Это включает, например, parse/unused/style diagnostics и тем самым дублирует get_project_errors/v8-code-style; более того, ниже каждая такая запись объявляется «defect to FIX». Либо введите явный whitelist/category для действительно дополнительного метрического слоя, либо честно переопределите контракт как альтернативный полный источник диагностик, задокументируйте пересечение и дайте пользователю способ исключить дубли. Нужен тест с одновременно metric и non-metric rules.
| String json; | ||
| try | ||
| { | ||
| json = new String(Files.readAllBytes(reportPath), StandardCharsets.UTF_8); |
There was a problem hiding this comment.
И stdout процесса (StringBuilder выше), и JSON-отчёт читаются целиком без лимита, после чего Gson строит ещё и полное DOM-дерево. На большом проекте это позволяет внешнему процессу исчерпать heap EDT задолго до OutputSizeGuard, который ограничивает только финальный ответ. Запускайте CLI с --silent, держите bounded tail для логов, проверяйте максимальный размер отчёта/парсите потоково и возвращайте fail-loud ошибку с предложением сузить scope. Добавьте тесты на чрезмерный stdout и report.
| { | ||
| return null; | ||
| } | ||
| // Prefer the lexicographically largest name (roughly the newest version). |
There was a problem hiding this comment.
Лексикографический выбор не является «roughly newest»: ...-1.9.0-exec.jar будет выбран вместо ...-1.10.0-exec.jar. Это особенно опасно при обещанной поддержке двух major-линий с разными требованиями к Java. Разберите SemVer и сравнивайте числовые компоненты (с тестами 0.28/1.9/1.10/pre-release) либо, безопаснее, при нескольких jar требуйте явный EDT_MCP_BSL_LS_JAR, чтобы версия не менялась молча.
There was a problem hiding this comment.
Проверил текущий HEAD 9cc0c776: числовая часть исправлена (1.10.0 > 1.9.0), но условие из этого треда про pre-release всё ещё не выполнено.
Сейчас compareVersionComponent("0-rc1", "0") попадает в строковый fallback и даёт положительный результат, поэтому:
compareVersions("1.10.0-rc1", "1.10.0") > 0То есть автопоиск выберет RC поверх стабильного релиза той же версии — обратный порядок SemVer. Новый testCompareVersionsPreReleaseSuffixDoesNotThrow это не фиксирует: он проверяет только повторяемость результата и прямо снимает требование к precedence, хотя в замечании был запрошен тест pre-release.
Предлагаю один из двух честных вариантов:
- реализовать SemVer precedence: stable выше pre-release; pre-release identifiers сравнивать по правилам numeric/lexical;
- если полный SemVer здесь избыточен — принимать в автопоиске только строгий
MAJOR.MINOR.PATCH, а при нескольких неоднозначных/pre-release jar требоватьEDT_MCP_BSL_LS_JARвместо молчаливого выбора.
Нужны проверки знака для 1.10.0-rc1 < 1.10.0, rc1 < rc2, numeric identifiers и интеграционный scanForExecJar, где рядом лежат stable и RC.
| /** | ||
| * Tests for {@link BslLsRunner}'s resolution logic — the parts that decide WHICH jar, | ||
| * Java and configuration file are used, without spawning the engine. The subprocess | ||
| * path itself is validated live/e2e (it needs the real jar + a Java 21). These tests |
There was a problem hiding this comment.
В текущем workflow это утверждение неверно: все happy-path E2E с движком пропущены при отсутствии jar, поэтому subprocess path в CI вообще не исполняется. Unit-тесты тоже проверяют только resolution и не ловят ошибки command line, CWD, report discovery, exit code, timeout/kill и ограничения вывода. Сделайте наличие хотя бы одной версии jar обязательным job precondition (не SKIP) и добавьте матрицу для обеих заявленных веток 0.28.x и 1.x; инфраструктурные ветки runner лучше покрыть управляемым fixture-процессом.
…ыхода за src/, честный контракт по diagnostics
Три замечания бота плюс одно от DitriX, все по CodeReviewTool:
1. При modulePath движок всё равно анализирует всю содержащую директорию
(это его единица анализа), и раньше сводные счётчики ("N findings: X
error, Y warning...") считались по ВСЕМУ отчёту, включая соседние
модули из той же папки - обзор одного файла мог показать находки чужого
соседа или скрыть, что сам файл чист. Теперь "scoped" (отфильтрованный
по целевому модулю набор) считается ПЕРВЫМ, и именно по нему строится
сводка; severity/rule/excludeRule остаются чисто отображающими
фильтрами поверх него, не влияя на итоговые счётчики.
2. modulePath резолвился через BslModuleUtils.resolveModuleFile, который
принимает абсолютные пути и пути с '..', разрешая их против ВСЕЙ
Eclipse workspace, а не только src/ запрошенного проекта. Без проверки
вызывающий мог указать modulePath на соседний проект (или вообще любое
место в workspace) - и он тихо анализировался бы под шапкой другого
проекта. Добавлена isWithinSrc: путь обязан остаться внутри
срезолвленного srcRoot (сравнение по сегментам пути, не по префиксу
строки - иначе "src-evil" ложно прошёл бы проверку для "src").
3. DitriX справедливо указал: инструмент называет себя "дельтой поверх
get_project_errors" (только метрики, которых нет в EDT), но по факту
отдаёт ВЕСЬ каталог diagnostics движка - включая правила, дублирующие
v8-code-style. Вместо угадывания whitelist "настоящих метрик" (риск
ошибиться в неполном/неточном списке правил движка) контракт честно
переопределён: это полный каталог движка, пересечение с
get_project_errors задокументировано, и добавлен excludeRule - фильтр
по подстроке id правила, исключающий уже дублирующиеся находки (зеркало
существующего rule-фильтра).
Проверено: mvn clean verify -> BUILD SUCCESS, 4105 тестов.
…рсии jar по SemVer, fixture-тесты процесса
1. --configuration у движка документирован как ГЛОБАЛЬНЫЙ слот конфигурации,
а не привязанный к анализируемой директории; раньше --workspaceDir вообще
не передавался. При анализе ОДНОГО модуля (--srcDir сужен до папки файла)
это могло увести относительные пути отчёта и поиск workspace-local
.bsl-language-server.json от корня проекта. Теперь --workspaceDir всегда
передаётся явно и закреплён за src/ проекта независимо от того, насколько
узок --srcDir для конкретного запуска (подтверждено через --help
реального движка: --workspaceDir/-w - самостоятельный флаг analyze).
2. stdout процесса копился в StringBuilder без ограничения, и весь JSON-отчёт
читался в память + строился полный DOM Gson ДО того, как OutputSizeGuard
вообще получал шанс урезать финальный ответ - разросшийся или испорченный
процесс движка мог исчерпать heap EDT. Добавлены: --silent движку
(снижает шум в stdout), ограничение НАКОПЛЕННОГО stdout до 8000 символов
(хвост обрезается по мере поступления, сам поток дочитывается полностью,
чтобы не заблокировать дочерний процесс на переполненном pipe), и проверка
размера файла отчёта (Files.size) ДО чтения - отчёт больше 50 МБ
отклоняется явной ошибкой вместо чтения в память.
3. Выбор jar по лексикографическому сравнению имени файла давал неверный
результат для "roughly newest": "...-1.9.0-exec.jar" лексически больше
"...-1.10.0-exec.jar" ('9' > '1'), из-за чего мог тихо выбираться СТАРЫЙ
релиз - особенно опасно, учитывая что две заявленные major-ветки (0.28.x
и 1.x) требуют разных версий Java. Добавлено числовое сравнение версии
компонент-за-компонентом (extractVersion/compareVersions/
compareJarVersions), с тестами на 0.28/1.9/1.10 и на нечисловой
pre-release-суффикс.
4. Все прежние unit-тесты проверяли только резолвинг (какой jar/Java/config),
а не сам процесс: реальный e2e с движком просто SKIP'ается, если jar не
установлен - код запуска подпроцесса не проходил в CI вообще ни разу.
Добавлен компилируемый на лету "fixture jar" (через
ToolProvider.getSystemJavaCompiler + JarOutputStream, без нового
Maven-модуля), имитирующий движок по сценарию (обычный отчёт / огромный
stdout / огромный отчёт) - тесты гоняют реальный BslLsRunner.run через
реальный дочерний процесс и проверяют его сквозную обработку без
многосотмегабайтного настоящего jar.
Проверено: mvn clean verify -> BUILD SUCCESS, 4105 тестов (fixture-тесты
реально выполняются, не skip - подтверждено по surefire-отчёту).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9cc0c776aa
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| registry.register(new CreateProjectTool()); | ||
| registry.register(new GetProblemSummaryTool()); | ||
| registry.register(new GetProjectErrorsTool()); | ||
| registry.register(new CodeReviewTool()); |
There was a problem hiding this comment.
Because this registers code_review, the live tools/list surface gains a new entry, but the committed golden snapshot still has no "name": "code_review" entry (rg over tests/e2e/tools_list.golden.json returns no matches). The golden e2e test compares live tools/list byte-for-byte against that file, so any run including _tools_list_golden will fail until the snapshot is regenerated.
Useful? React with 👍 / 👎.
| "export_configuration_to_xml", "import_configuration_from_xml", "build_external_objects", //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ | ||
| "create_infobase", "delete_infobase", "set_infobase_credentials", "create_project", //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ | ||
| "get_problem_summary", "get_project_errors", "validate_xdto_package", "get_markers", //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ | ||
| "code_review", //$NON-NLS-1$ |
There was a problem hiding this comment.
This maps the new tool only in the progressive-disclosure Toolsets catalog, but the separate preferences UI/preset layer enumerates ToolGroup.values() and ToolGroup.getGroupForTool(...); code_review is absent there, so Tools tab operations such as Disable All and presets cannot include or disable this external-process tool. Add it to the appropriate ToolGroup as well so the registered tool remains manageable through preferences.
Useful? React with 👍 / 👎.
| try | ||
| { | ||
| return Result.ok(BslLsReport.parse(json)); |
There was a problem hiding this comment.
When the BSL Language Server process exits non-zero but still leaves a bsl-json.json file, this path parses the file and returns Result.ok(...), so callers see a successful review even though the subprocess reported an operational failure. Since the runner already captures exit and its contract describes non-zero exits as errors, check exit != 0 before accepting the report and include the captured engine output in the failure.
Useful? React with 👍 / 👎.
…йденная живым e2e ошибка CWD Мерж origin/master (апстрим ушёл далеко вперёд после последнего пуша). Разбор всех 9 замечаний ревью показал: 6 из них уже были закрыты в предыдущих коммитах ветки (path-эскейп из modulePath, ограничение чтения stdout/отчёта, честная переформулировка контракта "это не строгая дельта над get_project_errors" + excludeRule, инфраструктура fixture-процесса для тестов раннера). Проверил каждое эмпирически, а не по сообщению коммита. Реально открытыми остались: - SemVer precedence для pre-release суффиксов (перепроверка Дитрикса): числовая часть была исправлена раньше, но "1.10.0-rc1" всё ещё сравнивался как строка и оказывался БОЛЬШЕ "1.10.0" — автовыбор жал бы RC поверх стабильного релиза той же версии. Реализована настоящая precedence: релиз без суффикса всегда старше любого pre-release той же версии; сравнение pre-release идентификаторов через точку, числовые — численно, остальные — лексически. Тесты ровно по списку, который просил Дитрикс: rc1 < rc2, релиз > rc, числовые идентификаторы через точку (rc.9 < rc.10), интеграционный scanForExecJar со stable и RC рядом. - Доверие к exit-коду движка: процесс мог упасть (exit != 0), но если файл отчёта всё равно остался на диске, тул парсил его и рапортовал успех. Эмпирически проверил на реальном движке: чистый прогон всегда exit=0, даже когда находит диагностики — так что ненулевой код это гарантированно операционный сбой, а не находки. Добавлена проверка exit перед доверием отчёту + fixture-тест. - code_review отсутствовал в ToolGroup: вкладка Tools/Disable All не могла им управлять (Toolsets — отдельный каталог для progressive disclosure, ToolGroup — для preferences UI, регистрация нужна в обоих). При живом e2e-прогоне новой логики modulePath (не из замечаний бота) поймал реальный баг: процесс запускался с CWD = узкая scopeDir модуля, а движок строит путь в отчёте из CWD + relative-фрагмент от mdoRef, а не из --srcDir — при сужении на один модуль путь задваивался в несуществующий ".../CommonModules/Calc/CommonModules/Calc/Module.bsl", который никогда не совпадал с targetAbsPath, и modulePath-сканирование молча всегда возвращало "находок нет". Починено: CWD процесса = стабильный workspaceDir (уже используется для --workspaceDir), а не узкий srcDir. Живая проверка на реальном движке подтвердила: находки появились. Golden перегенерирован (code_review — новый тул в tools/list), README и docs/tools синхронизированы командой generate_tool_docs.py (заодно обнаружилась более ранняя рассинхронизация — описание в docs всё ещё было на старой, до-excludeRule формулировке). 4348 юнит-тестов зелёные; code_review e2e 8/8 на реальном движке (bsl-language-server 1.0.3, Java 21).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f967849c6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| .configFile(projectConfig(srcRoot, project)) | ||
| // Pin the workspace root to the project's own src/ regardless of how narrow scopeDir is | ||
| // for a single-module review, so report paths and workspace-local settings stay scoped | ||
| // to THIS project (see BslLsRunner#buildCommand). | ||
| .workspaceDir(srcRoot); |
There was a problem hiding this comment.
Fresh evidence in this squashed commit is that the tool still takes the project-root .bsl-language-server.json from projectConfig(...) and passes it as --configuration while pinning --workspaceDir to src/. BSL LS documents --configuration as the global config and says per-workspace diagnostic settings are loaded from .bsl-language-server.json in the workspace root (https://1c-syntax.github.io/bsl-language-server/en/features/ConfigurationFile/), so projects that disable rules or tune thresholds in the documented project-root file will be reviewed with default diagnostics instead of their local policy; make the directory containing that file the workspace-local config root, or otherwise provide it as local workspace config, rather than treating it as global.
Useful? React with 👍 / 👎.
| new BufferedReader(new InputStreamReader(process.getInputStream(), StandardCharsets.UTF_8))) | ||
| { | ||
| String line; | ||
| while ((line = reader.readLine()) != null) |
There was a problem hiding this comment.
When the external engine or an EDT_MCP_BSL_LS_JAR wrapper emits a very large line without a newline, readLine() allocates the entire line before the later MAX_CAPTURED_OUTPUT_CHARS trim runs. That means a bad crash log or chatty wrapper can still grow the EDT heap unbounded despite the intended output cap; drain with a fixed-size character buffer and trim as chunks arrive instead.
Useful? React with 👍 / 👎.
Замечание codex: drainAsync читал вывод подпроцесса через readLine(), то есть материализовал строку целиком ДО того, как срабатывал MAX_CAPTURED_OUTPUT_CHARS. Одна огромная строка без перевода (краш-дамп, обёртка, печатающая файл целиком, движок в цикле без разделителя) съедала кучу EDT независимо от того, насколько мал предел. Javadoc при этом обещал ограниченность — то есть обещал больше, чем код давал. Пункт №3 предпусковой проверки CLAUDE.md ровно про это. Теперь чтение идёт фиксированным буфером char[DRAIN_CHUNK_CHARS]: транзиентно удерживается не больше 8 КБ, где бы ни стояли (и стоят ли вообще) переводы строк. Тест: существующая фикстура huge-stdout печатала 5000 МАЛЕНЬКИХ строк, поэтому readLine никогда не держал больше 2000 символов и дыру не задевала. Добавлена фикстура huge-single-line — ~20 МБ одной строкой через print() без println(). В комментарии к тесту честно оговорено, что он проверяет поведение (поток дочитывается, ребёнок завершается, отчёт разбирается), а сам предел на аллокацию структурный — фиксированный буфер. Второе замечание (--configuration якобы игнорируется как «глобальный», из-за чего проектный .bsl-language-server.json не применяется) ОТКЛОНЕНО — проверено на реальном движке 1.0.3: - прогон без конфига: EmptyCodeBlock + UnusedLocalVariable; - тот же прогон с --configuration на проектный файл, отключающий EmptyCodeBlock: остаётся только UnusedLocalVariable, то есть конфиг применён; - встречная проверка: конфиг, положенный в workspace-корень без --configuration, тоже подхватывается. Движок honours оба способа, наш путь корректен. Также влит origin/master (8 коммитов). Пересечение со стороной ветки только в tools_list.golden.json; проверено, что смёрженный golden совпадает с живым сервером, регистрация code_review уцелела во всех трёх точках. Проверено: сборка 4546 тестов зелёная, живой редеплой (jar проверен на antistale), e2e code_review 8/8, golden сходится, docs без изменений, fixture clean: True.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4c433bb59
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| try | ||
| { | ||
| Files.walk(dir) |
There was a problem hiding this comment.
When code_review is run repeatedly in the long-lived EDT process, this Files.walk(...) stream is never closed; the JDK API keeps directory handles open until the stream is closed, so every runner cleanup can leak file descriptors even though the files themselves are deleted. Wrap the walk in try-with-resources before sorting/deleting the temp tree.
Useful? React with 👍 / 👎.
| // Analysis-only: launches the engine subprocess which READS .bsl files; never mutates the EDT | ||
| // model or writes into the project. readOnly + idempotent so clients don't gate it behind a | ||
| // write-confirmation. | ||
| return new ToolAnnotations(null, Boolean.TRUE, Boolean.FALSE, Boolean.TRUE, Boolean.FALSE); |
There was a problem hiding this comment.
With a project .bsl-language-server.json that sets traceLog, this read-only annotation is no longer reliable: the runner passes that config to BSL LS and runs it with a project directory as the process CWD/workspace root, while BSL LS documents traceLog as a file path that is overwritten when the server starts (https://1c-syntax.github.io/bsl-language-server/en/features/ConfigurationFile/). Clients can therefore invoke code_review without write consent even though it can create or overwrite a project-relative log; either strip/redirect file-writing config such as traceLog to the temp output directory, or stop marking the tool read-only.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4c433bb59
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return Result.error("BSL Language Server timed out after " + request.timeoutSeconds //$NON-NLS-1$ | ||
| + "s. Narrow the scope or raise the timeout."); //$NON-NLS-1$ |
There was a problem hiding this comment.
For configurations whose analysis takes longer than 180 seconds, this path always terminates the engine, but CodeReviewTool neither declares a timeout parameter nor calls Request.timeoutSeconds, and there is no environment or preference override. The suggested recovery is therefore impossible and every whole-project retry hits the same deadline; expose a bounded timeout setting or remove the unactionable advice and use a sufficient fixed bound.
Useful? React with 👍 / 👎.
| File best = jars[0]; | ||
| for (File j : jars) | ||
| { | ||
| if (compareJarVersions(j.getName(), best.getName()) > 0) | ||
| { | ||
| best = j; | ||
| } | ||
| } |
There was a problem hiding this comment.
When EDT runs on Java 17 and the default engine directory contains both a 0.28.x jar and a 1.x jar, this always selects the newer 1.x jar before Java is resolved. The class itself documents that 1.x requires Java 21 while 0.28.x runs on Java 17, so the subprocess fails with an unsupported class version even though a compatible engine is installed; automatic jar selection should account for the selected runtime or report the incompatible pair before launching.
Useful? React with 👍 / 👎.
| File moduleOsFile = moduleFile == null || moduleFile.getLocation() == null | ||
| ? null : moduleFile.getLocation().toFile(); | ||
| if (moduleOsFile == null || !moduleOsFile.isFile()) |
There was a problem hiding this comment.
If modulePath names any existing non-BSL file under src/, such as Configuration/Configuration.mdo, this check accepts it, runs the engine on its containing directory, and then filters findings to the exact .mdo path. Since BSL LS reports diagnostics for BSL modules rather than that metadata file, the scoped list becomes empty and the tool misleadingly reports a clean module; validate that the resolved target is a .bsl module before launching.
Useful? React with 👍 / 👎.
| JsonArray fileInfos = asArray(root, "fileinfos"); //$NON-NLS-1$ | ||
| if (fileInfos == null) | ||
| { | ||
| return new BslLsReport(findings, metrics); |
There was a problem hiding this comment.
When an incompatible engine version or EDT_MCP_BSL_LS_JAR wrapper exits zero but writes a valid JSON error/status object such as {} instead of the expected report schema, this branch converts it into an empty successful report. CodeReviewTool then tells the caller that no code-quality issues were found, masking an operational/report-format failure as a clean project; require fileinfos to be an array and return a parse error when the required top-level field is absent or has the wrong type.
Useful? React with 👍 / 👎.
| targetAbsPath = normalize(moduleOsFile.getAbsolutePath()); | ||
| scopeDir = moduleOsFile.getParentFile(); |
There was a problem hiding this comment.
When a target module contains an exported method referenced from a different metadata object, narrowing --srcDir to only this containing directory prevents project-wide diagnostics such as unused-export analysis from seeing that caller. The stable workspaceDir only roots workspace settings and report paths; the runner still tells the engine to analyze scopeDir, so module-scoped runs can produce false positives or omit findings for rules that require cross-module context. Analyze the project source tree and filter the resulting rows to the requested module instead of reducing the engine's analysis input.
Useful? React with 👍 / 👎.
2 participants
Новый тул code_review. Стандарты РАЗРАБОТКИ (пакет v8-code-style) уже показываются через get_project_errors; но метрики кода (магическое число, цикломатическая/когнитивная сложность, длина метода/строки, вложенность) в EDT нет вообще. Своих правил не пишем - оборачиваем внешний движок BSL Language Server (github.com/1c-syntax/bsl-language-server, LGPL-3.0).
Архитектура: движок = ВНЕШНИЙ jar (не в git, не в bundle) - подпроцесс
java -jar exec.jar --analyze --reporter json, найденный через env EDT_MCP_BSL_LS_JAR (+ EDT_MCP_BSL_LS_JAVA для выбора Java, т.к. линейка 1.x движка собрана под Java 21, а 0.28.x - под Java 17). Обновление движка = подмена файла без пересборки плагина. Конфиг проверок - проектный .bsl-language-server.json, иначе дефолты движка.Вывод рассчитан на авто-ремедиацию агентом: таблица Severity|Rule| Module path|Line|Message|Docs плюс явная инструкция "чини на Module path+Line через write_module_source, потом перезапусти code_review для проверки". Module path релятивизован к src/ - те же координаты, что read/write_module_source.
BslLsReport - чистый парсер JSON-отчёта движка (без IO): конвертирует LSP 0-based строку/колонку в 1-based, нормализует путь (движок пишет file:// с ../ относительно своего рабочего каталога), маппит severity. BslLsRunner - подпроцесс: поиск jar/Java, запуск, парсинг, actionable-ошибка при отсутствии движка вместо исключения.
Тесты: unit (BslLsReportTest, BslLsRunnerTest, CodeReviewToolTest) + e2e (happy-пути дают skip, если движок не настроен в окружении - специально для CI без внешней зависимости; негативные сценарии детерминированы и не требуют движка). Зарегистрирован в тулсете PROJECT.
Примечание: README-индекс тулов (docs/tools/README.md, таблица в README.md) не тронут в этом PR - это общий сгенерированный блок, конфликтующий между параллельными PR; предлагаем регенерировать один раз после мержа нескольких PR (python docs/generate_tool_docs.py против живого сервера).