Skip to content

feat(mcp): WS-mode к v8-client-session-manager + skill bootstrap reference - #3

Open
zeegin wants to merge 15 commits into
IngvarConsulting:masterfrom
SteelMorgan:feat/mcp-ws-mode
Open

feat(mcp): WS-mode к v8-client-session-manager + skill bootstrap reference#3
zeegin wants to merge 15 commits into
IngvarConsulting:masterfrom
SteelMorgan:feat/mcp-ws-mode

Conversation

@zeegin

@zeegin zeegin commented Sep 2, 2026

Copy link
Copy Markdown
Member

Migrated from alkoleft/v8-runner-rust pr #3.
Original author: steel-code-agent; created: 2026-05-07T10:22:13Z; last upstream update: 2026-06-27T14:54:33Z.
GitHub shows the migration account as the new author; this block preserves actual attribution.
Upstream historical base: master at 30871f6f49077fc83bf01317157e44e839660859; head: SteelMorgan:feat/mcp-ws-mode at aa3e45ac52132e3e856d183773db00847a99e6ca. Active migrated PRs compare that exact head with the current IngvarConsulting/v8-runner-rust:master.


Summary

Добавляет WS-режим для клиентского MCP: 1С-клиент может подключаться к v8-client-session-manager по WebSocket, а при transport: auto без доступного менеджера v8-runner использует локальный HTTP MCP (runMcp).

После review исправлено имя публичного режима: локальный MCP-транспорт называется mcp, не legacy.

What Changed

  • Добавлен transport selector для клиентского MCP: ws | mcp | auto.
  • --mcp-transport=mcp форсирует локальный HTTP MCP без probe.
  • --mcp-transport=auto делает короткий TCP-probe manager_url: доступен manager -> ws, иначе -> mcp.
  • --json-message для launch mcp возвращает типизированный transport: "ws" | "mcp".
  • tools.client_mcp.transport в JSON Schema ограничен enum-ом ws | mcp | auto; legacy теперь отклоняется на schema/runtime boundary.
  • WS /C payload hardened: значения кодируются для защиты от ;/= format injection; CLI/config дополнительно валидируют опасные значения.
  • manager_url валидируется как IP:port, чтобы auto-probe не зависал на блокирующем DNS lookup.
  • test yaxunit / test va больше не глотают ошибку explicit WS resolution, а возвращают structured setup failure.
  • Обновлены README, docs/CONFIGURATION.md, schemas и repo-local skill v8-runner/SKILL.md.

Config

tools:
  client_mcp:
    transport: auto         # auto | ws | mcp
    manager_url: ws://127.0.0.1:4000/sessions
    log_level: info
    ws_timeout_ms: 1000

port под tools.client_mcp сохраняется для локального MCP (runMcp) режима.

Review Fixes Included

  • README link to v8-client-session-manager is absolute.
  • v8-runner/references/bootstrap.md: tools.connection -> infobase.connection.
  • ws_timeout_ms schema minimum is 1.
  • transport=legacy replaced with transport=mcp in CLI/config/docs/tests.
  • Launch result transport is typed, not a free-form string.
  • default_corr_id pads short client ids to keep vr-<8 chars> shape.
  • Flaky TCP no-listener test no longer depends on dropped ephemeral port reuse.
  • log_level validation is case-insensitive and normalized for payload output.

Test Plan

  • CC=/usr/bin/gcc UPDATE_CONFIG_SCHEMAS=1 ~/.cargo/bin/cargo test generated_schema_artifacts_are_current
  • CC=/usr/bin/gcc ~/.cargo/bin/cargo test schemas_and_loader_reject_invalid_client_mcp_transport
  • CC=/usr/bin/gcc ~/.cargo/bin/cargo test mcp_ws
  • CC=/usr/bin/gcc ~/.cargo/bin/cargo test --test cli_launch

Full cargo test was also run locally: 684 passed, 2 environment-related failures unrelated to this PR surface (/usr/local/bin/1cedtcli autodiscovery in a not-found test, and one process kill liveness check). Existing warning remains: src/use_cases/tool_extension.rs:126 unused variable.

Summary by CodeRabbit

  • Новые возможности
    • Добавлена поддержка WebSocket-режима подключения 1С-клиента к session-manager для команд MCP/VA и тестов: transport=auto/ws/mcp с настройками manager_url, client_uid, corr_id, log_level, ws_timeout_ms, включая автопроверку и fallback на legacy MCP.
  • Конфигурация и схемы
    • Расширены и ужесточены схемы v8project.yaml для client_mcp (транспортные параметры, ограничения и валидация).
  • Документация
    • Обновлены руководства по MCP WS-режиму, добавлены материалы по bootstrap и уточнены команды/гайд по сценариям.
  • Тесты
    • Расширены интеграционные и валидационные тесты для WS/auto-поведения, генерации /C и проверок параметров.

SteelMorgan and others added 15 commits May 7, 2026 08:21
…layout

Anthropic Skills convention: каталог скила = его имя в kebab-case, внутри
обязательно SKILL.md + references/. Готовый каталог можно дроп-копировать
в ~/.claude/skills/<name>/ без переименования.

В этом репо frontmatter скила декларирует name: v8-runner, поэтому
каталог приводим в соответствие.

Что сделано:
- git mv SKILL v8-runner (sохраняет историю)
- ссылки обновлены: AGENTS.md (×2), spec/decisions/0022-*.md (×1)

После рефакторинга разработчику достаточно сделать
  cp -r v8-runner ~/.claude/skills/
чтобы скил стал доступен Claude Code.
Adds src/use_cases/mcp_ws with:
- McpClientTransport (Ws/Legacy/Auto) selector
- ClientKind enum mapping entry-points to manager-side kind values
- WsLaunchParams::payload_snippet() that produces the
  'mcpMode=ws;manager_url=...;client_uid=...;kind=...;corr_id=...;
  mcp_log_level=...;mcp_ws_timeout_ms=...' segment for /C
- select_transport() with probe-callback (Auto -> WS or Legacy)
- probe_tcp() built on std::net::TcpStream::connect_timeout
- parse_manager_addr() that extracts host:port from a ws://... URL
- validate_log_level() for off|error|warn|info|debug|trace

Pure module without project plumbing yet; later commits wire it into
launch_app, run_tests, CLI args and config loading.
Adds new optional fields under tools.client_mcp:
- transport: ws | legacy | auto
- manager_url: ws://host:port/...
- log_level: off | error | warn | info | debug | trace
- ws_timeout_ms: u64 (>= 1)

Reuses tools.client_mcp instead of mcp.client because the loader still
rejects mcp.client as a legacy migration guard (LegacyMcpClientConfig).
Putting the new keys next to port/extension keeps client-MCP config in
one place.

Schemas (ClientMcpToolSchema, PartialClientMcpToolSchema) and
validate.rs are extended with bound checks: transport/log_level use the
mcp_ws helpers, manager_url is parsed via parse_manager_addr,
ws_timeout_ms must be > 0.
…id / --corr-id / --mcp-log-level / --mcp-ws-timeout-ms

Adds a shared McpClientWsArgs struct flattened into LaunchArgs and
TestArgs. Values are validated at the CLI boundary
(map_mcp_ws_args): transport must be ws|legacy|auto, log_level must
be one of off|error|warn|info|debug|trace, ws_timeout_ms must be >=
1, manager_url must include host:port, client_uid/corr_id must not
contain ';' (the /C payload is semicolon-delimited).

Wired through transport-neutral McpClientWsRequest in
use_cases::request, threaded into LaunchRequest and TestRequest so
that downstream MCP service callers can also opt into the WS-mode
without changes once they need it. The actual /C payload assembly
happens in the next commit.

Per-launch random client_uid is left to the use-case layer. By
contract the kind is computed internally from the entry-point and is
NOT exposed via CLI.
launch_app:
- effective_launch_options now returns (LaunchOptions,
  Option<McpResolutionMeta>); meta drives the new LaunchResult
  fields (transport, client_uid, kind, manager_url, corr_id,
  mcp_port).
- decide_mcp_transport probes manager_url with PROBE_TIMEOUT_MS
  and returns ws|legacy according to CLI/config preference; auto
  falls back to legacy when probe fails.
- WS-mode emits 'mcpMode=ws;manager_url=...;client_uid=<UUID>;kind=
  v8_runner_client|vanessa_test_client;corr_id=vr-<uid8>;
  mcp_log_level=...;mcp_ws_timeout_ms=...'.
- Legacy branch keeps the existing 'runMcp[=...][;mcpPort=...]'
  exactly as before for back-compat.

run_tests:
- After build_platform_launch the coordinator calls
  apply_test_mcp_ws_payload, which appends the WS snippet to the
  existing /C (RunUnitTests=... or VA player payload). Resolution
  errors are logged as warn and skipped, preserving prior behavior.
- yaxunit_runner / vanessa_test_client kinds are picked from
  PreparedRun.

LaunchResult: new optional fields are skip-if-none, so JSON envelopes
for non-MCP launches are byte-identical to the previous shape.
- launch_app::tests::client_mcp_launch_does_not_prepare_configured_tool_extension:
  pin transport to Legacy so the test stays deterministic in
  environments where a session-manager listener happens to be live
  on 127.0.0.1:4000 (auto would otherwise pick WS).
- tests/cli_launch.rs::launch_mcp_va_builds_payload_from_configured_port_and_ordinary_mode:
  add --mcp-transport legacy and assert the new JSON fields
  transport=legacy and mcp_port=9874 in the launch envelope.
- Regenerate docs/schemas/v8project.schema.json and
  docs/schemas/v8project.local.schema.json to include the new
  tools.client_mcp.{transport,manager_url,log_level,ws_timeout_ms}
  fields (UPDATE_CONFIG_SCHEMAS=1 cargo test
  generated_schema_artifacts_are_current).
tests/cli_launch.rs: 6 new integration tests covering the four
behaviors required by the spec:
- launch mcp --mcp-transport=legacy emits /C"runMcp;mcpPort=..."
  and a JSON envelope with transport=legacy + mcp_port.
- launch mcp --mcp-transport=ws against a live ephemeral listener
  emits /C"mcpMode=ws;manager_url=...;client_uid=...;kind=
  v8_runner_client;corr_id=...;mcp_log_level=...;mcp_ws_timeout_ms=
  ..." plus matching JSON fields (transport=ws, client_uid, kind,
  manager_url, corr_id).
- launch mcp --mcp-transport=ws against an unreachable port fails
  with 'session-manager unreachable' diagnostic.
- launch mcp --mcp-transport=auto falls back to legacy when the
  manager is unreachable.
- launch mcp --manager-url with bare host (no port) is rejected at
  argument-mapping time.
- launch mcp --mcp-ws-timeout-ms 0 is rejected as zero.

src/use_cases/run_tests/helpers.rs: 3 unit tests for
append_mcp_ws_snippet covering existing /C, missing /C and empty /C.
Real 1C runs are not invoked in any of these tests — the script in
$tempdir/platform/bin/1cv8c just dumps args to a log.
- README: add a 'Подключение к session-manager (WS-режим)' subsection
  pointing at docs/CONFIGURATION.md and the v8-client-session-manager
  repo.
- docs/CONFIGURATION.md: extend tools.client_mcp YAML example with
  the new transport/manager_url/log_level/ws_timeout_ms fields and
  add a 'WS-режим к session-manager' subsection that documents the
  internal kind mapping, the /C payload shape, the CLI flags and
  the auto-probe behavior.
- src/use_cases/mcp_ws.rs: simplify — derive Default for
  McpClientTransport instead of a hand-written impl, drop unused
  validate_log_level/UnsupportedLogLevel pair (the boundary uses
  is_supported_log_level directly via map_mcp_ws_args), and switch
  the matching test over.
- cargo fmt.
Дополняет агентный скил v8-runner новой секцией. Источник правды —
docs/CONFIGURATION.md (`tools.client_mcp` + «WS-режим к session-manager»),
здесь — оперативная справка для агента.

Что добавлено:
- v8-runner/references/project-workflows.md: новая секция «WS-режим к
  session-manager» — транспорт/auto-probe, /C payload, internal kind
  mapping (v8_runner_client / vanessa_test_client / yaxunit_runner),
  override-флаги, JSON-output, замечание что менеджер v8-runner не
  поднимает (отдельный шаг).
- v8-runner/references/command-selection.md: после блока launch mcp
  добавлены примеры WS-флагов (--mcp-transport, --manager-url,
  --client-uid, --corr-id, --mcp-log-level) с указанием на полный
  раздел в project-workflows.md.
- v8-runner/SKILL.md: в Default Use-Case Routing — буллет про WS-режим
  с краткой логикой auto-probe и фиксированным kind-mapping (без CLI
  override).
…init

Скил знал про команду config init, но не описывал, как агенту решить
какие флаги передать без лишних вопросов пользователю. Decision-tree
прятался в коде src/use_cases/config_init.rs (auto-detect format,
discover sources, choose builder).

Что добавлено:
- v8-runner/references/bootstrap.md — новый reference. Содержит:
  - проверка существующего v8project.yaml (force-семантика);
  - signals в файловой системе для format=designer|edt|auto;
  - когда переключать builder с DESIGNER на IBCMD;
  - три формы --connection (File auto-managed / File existing /
    Srvr server-bound) с дефолтом File=build/ib;
  - правила «когда задавать вопросы»: 4 ситуации и формулировки;
  - примеры flow для типичных проектов (File-Designer, EDT, mixed,
    server-bound);
  - чек-лист what-to-inspect после config init.
- v8-runner/SKILL.md: новый bullet в decision entrypoint, ссылающийся
  на bootstrap.md.

Источник правды по фактам — src/use_cases/config_init.rs (discover_sources,
choose_format, build_source_sets) + src/cli/args.rs (ConfigInitArgs).
- replace legacy transport config value with mcp\n- harden WS payload validation and schema boundaries\n- update docs, schemas, and launch tests
- clarify WS timeout documentation\n- include manager_url parse diagnostics\n- remove legacy wording from public docs
`/C"value"` was passed as a single argv element with literal `"` chars
in the payload — `std::process::Command` bypasses the shell on both Linux
(execve) and Windows (CreateProcess), so the platform received `/C"…"`
as one unknown key and replied with «Неверные или отсутствующие
параметры соединения с информационной базой». Splitting into `/C` and
the payload as two argv tokens makes the platform parse `/C` as the
command-line parameter key and the next token as its value.

Tests adjusted to match the new format: argv pairs `["/C", "<value>"]`
in unit tests and `/C\n<value>\n` in the integration args.log dump
(printf '%s\n' "$@" puts each argv on its own line).

Recovered from a dangling stash (cdb1e0a0 "fix/c-arg-quoting WIP") that
had been authored locally but never committed; rebuilds lost the patch.
Plain enterprise launches now honor MCP WS flags by appending mcpMode=ws to /C without a kind field. Specialized launch mcp flows keep the existing kind-bearing payload.

Tests: CC=/usr/bin/gcc cargo test --test cli_launch
Launch mcp va now mirrors the canonical Vanessa manager startup: /TESTMANAGER, unsafe-action protection disabled, /Execute vanessa-automation.epf, and VAParams without StartFeaturePlayer. WS mode publishes kind=vanessa_test_client and keeps the detached process alive in its own session.
@zeegin zeegin added the upstream-migrated Migrated from the historical upstream tracker label Sep 2, 2026
@zeegin

zeegin commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

Upstream comment by coderabbitai[bot] at 2026-05-07T10:22:39Z: original

Walkthrough

PR adds WS transport support for MCP launch and test flows, extends config/CLI/request contracts, wires transport resolution into use cases, and updates launch argument handling plus detached Unix process startup behavior.

Changes

MCP WebSocket Session-Manager Support

Layer / File(s) Summary
Docs, routing, and repo layout
.gitignore, AGENTS.md, README.md, v8-runner/SKILL.md, v8-runner/references/*, spec/decisions/*
Documentation paths and skill references are updated, and the new workflow references describe bootstrap and MCP WS mode entry points.
Config and schema contracts
docs/CONFIGURATION.md, docs/schemas/v8project*.schema.json, src/config/model.rs, src/config/schema.rs, src/config/validate.rs
Client MCP config models, generated schemas, validation rules, and configuration docs add transport, manager URL, log level, and WS timeout fields.
CLI arguments and request mapping
src/cli/args.rs, src/use_cases/request.rs, src/cli/execute.rs, src/mcp/service.rs, src/use_cases/mod.rs
Launch CLI parsing, shared request types, and service adapters add MCP WS request fields and map them into test and launch requests.
WebSocket transport core
src/use_cases/mcp_ws.rs
The new mcp_ws module resolves transports, validates WS parameters, parses manager URLs, probes TCP reachability, and serializes WS payload snippets.
Launch and test use-case integration
src/use_cases/launch_app.rs, src/use_cases/run_tests/helpers.rs, src/use_cases/run_tests/coordinator.rs, src/use_cases/run_tests.rs, src/use_cases/vanessa.rs
Launch and test flows consume the new WS request data, choose transports, build payloads, and propagate resolved MCP metadata into results.
Integration tests
tests/cli_launch.rs, tests/cli_test.rs
CLI integration tests cover MCP WS, auto fallback, invalid overrides, and the updated Vanessa raw-key behavior.

Launch argument and process behavior

Layer / File(s) Summary
Launch argv and raw keys
src/platform/enterprise.rs, src/use_cases/launch_keys.rs, src/use_cases/vanessa.rs, tests/cli_test.rs
Enterprise launch argument assembly now emits /C as separate argv tokens and handles the unsafe-action raw key alongside the Vanessa test-manager key.
Detached process session setup
src/platform/process.rs
Detached Unix process spawning now calls setsid(), and the test checks for a new session and process group.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐰 I hop through /C with a WS cheer,
Manager pinged, the route is clear.
Logs and keys now line up neat,
One mcp or ws heartbeat.
Thump-thump! 🥕

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Заголовок точно отражает основное изменение: добавление WS-режима MCP к v8-client-session-manager и обновление ссылки на skill bootstrap.
Docstring Coverage ✅ Passed Docstring coverage is 83.59% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @​coderabbitai help to get the list of available commands.

@zeegin

zeegin commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

Upstream comment by steel-code-agent at 2026-06-27T14:47:16Z: original

Обновил этот WS PR связанными follow-up fixes из fork:

  • fix(launch): split /C and payload into separate argv tokens — корректная передача /C через std::process::Command, без shell quoting внутри argv.
  • fix(launch): pass WS payload for plain client launches — обычные launch thin/thick/ordinary --mcp-transport=ws теперь получают WS payload в /C.
  • fix(launch): start VA MCP as test managerlaunch mcp va запускает Vanessa test-manager: /TESTMANAGER, /DisableUnsafeActionProtection, /Execute <vanessa-automation.epf>, runtime VAParams без StartFeaturePlayer, WS kind=vanessa_test_client.

Зачем это в этом PR: это не отдельная фича поверх WS, а доведение WS-режима до рабочего контура с v8-client-session-manager, включая VA MCP manager session и последующий connect_test_client.

Targeted checks in the update worktree:

  • CC=/usr/bin/gcc cargo fmt --check
  • CC=/usr/bin/gcc cargo test launch_mcp_va -- --nocapture
  • CC=/usr/bin/gcc cargo test --bin v8-runner platform::enterprise::tests -- --nocapture
  • CC=/usr/bin/gcc cargo test --bin v8-runner spawn_detached_starts_new_session_and_process_group -- --nocapture
  • CC=/usr/bin/gcc cargo test test_va_builds_vanessa_command_and_overlay -- --nocapture
  • CC=/usr/bin/gcc cargo test test_va_does_not_duplicate_explicit_vanessa_raw_keys -- --nocapture
  • CC=/usr/bin/gcc cargo test test_va_adds_testmanager_when_raw_value_matches_name -- --nocapture

@zeegin

zeegin commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

Upstream review by coderabbitai[bot]; state: COMMENTED; submitted: 2026-05-07T10:34:37Z; commit: 17f3e054caf9eb441c0aac4ab26206fd50eb58b3. Original review

Actionable comments posted: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/config/schema.rs (1)

234-236: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Синхронизируй schema-boundary для ws_timeout_ms с runtime-правилами.

Сейчас в генерации ограничений для ClientMcpToolSchema/PartialClientMcpToolSchema учитывается только port, поэтому ws_timeout_ms=0 проходит JSON Schema и падает позже на прикладной валидации. Лучше отклонять это сразу на boundary-слое.

💡 Предлагаемый патч
     for def in ["ClientMcpToolSchema", "PartialClientMcpToolSchema"] {
         set_numeric_bounds(schema, &[def], "port", Some(1), None);
+        set_numeric_bounds(schema, &[def], "ws_timeout_ms", Some(1), None);
     }

As per coding guidelines: **/*.rs: Apply /rust-expert-best-practices-code-review skill for tasks involving Rust code review, refactoring, error handling, type safety, API design, or performance-sensitive changes.

🤖 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/config/schema.rs` around lines 234 - 236, The JSON Schema generator
currently only applies numeric bounds for "port" in
ClientMcpToolSchema/PartialClientMcpToolSchema, so ws_timeout_ms can be zero and
fails later; update the same boundary generation to also set a minimum > 0 for
"ws_timeout_ms" by calling set_numeric_bounds(schema, &[def], "ws_timeout_ms",
Some(1), None) alongside the existing call for "port" (refer to the loop over
def values and the set_numeric_bounds function to locate where to add the new
call).
🧹 Nitpick comments (4)
src/domain/launch.rs (1)

18-21: ⚡ Quick win

transport стоит выразить через типизированный enum вместо Option<String>

Поле документировано двумя значениями — "ws" и "legacy". В том же файле LaunchMode демонстрирует устоявшийся паттерн enum + #[serde(rename_all = "snake_case")], который даёт ту же JSON-сериализацию, но исключает опечатки и делает ветвление по значению безопасным на уровне типов. Поле новое — сменить тип сейчас дёшево.

♻️ Предлагаемое изменение
+/// MCP-транспорт для данного запуска.
+#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
+#[serde(rename_all = "snake_case")]
+pub enum McpTransport {
+    Ws,
+    Legacy,
+}

 pub struct LaunchResult {
     ...
-    /// MCP transport selected for this launch (`ws` or `legacy`).
-    /// Present only for `launch mcp` and `test` flows.
+    /// MCP transport selected for this launch.
+    /// Present only for `launch mcp` and `test` flows.
     #[serde(skip_serializing_if = "Option::is_none", default)]
-    pub transport: Option<String>,
+    pub transport: Option<McpTransport>,
🤖 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/domain/launch.rs` around lines 18 - 21, Поле transport в структуре Launch
нужно заменить с Option<String> на типизированный enum — создать enum Transport
{ Ws, Legacy } с #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)]
и #[serde(rename_all = "snake_case")] и использовать Option<Transport> с теми же
атрибутами #[serde(skip_serializing_if = "Option::is_none", default)] на поле
transport; затем поправить все места, где сравнивали или использовали transport
(паттерн-матчинг в коде) чтобы работать с Transport::Ws / Transport::Legacy, а
также обновить тесты/сериализацию, где ожидалось строковое значение.
src/use_cases/launch_app.rs (1)

243-257: ⚡ Quick win

WS-ветка молча игнорирует client_mcp.port и client_mcp.config_path.

В TransportDecision::Ws client_mcp.config_path/client_mcp.port (CLI или конфиг) не используются вовсе — это корректно семантически (порт назначает session-manager), но плохо для UX: пользователь, явно передавший --port 9874 и получивший WS через auto-detect, не получит ни warning'а, ни ошибки.

Минимум — лог уровня warn! при WS+port.is_some()/config_path.is_some(). Опционально — отклонять как validation-ошибку, если транспорт указан явно как Ws.

📝 Минимальный warn
         TransportDecision::Ws => {
+            if client_mcp.port.is_some() || config.tools.client_mcp.port.is_some() {
+                tracing::warn!("ws transport: client_mcp.port is ignored; session-manager controls the port");
+            }
+            if client_mcp.config_path.is_some() {
+                tracing::warn!("ws transport: client_mcp.config_path is ignored");
+            }
             let params = resolve_ws_launch_params(config, &args.mcp_ws, kind);
🤖 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/use_cases/launch_app.rs` around lines 243 - 257, Ветка
TransportDecision::Ws игнорирует переданные client_mcp.port и
client_mcp.config_path без уведомления пользователя; добавь проверку
перед/внутри матча (в функции где вызываются
launch_mcp_client_kind/decide_mcp_transport — вокруг переменных args.mcp_ws и
client_mcp) и при выборе Ws логгируй warn! если client_mcp.port.is_some() ||
client_mcp.config_path.is_some(); дополнительно, если транспорт явно задан как
Ws в args.mcp_ws, преобразуй это в validation error (Err) вместо молчалого
игнорирования, используя те же символы TransportDecision::Ws,
resolve_ws_launch_params и McpResolutionMeta::Ws для локализации изменения.
src/use_cases/mcp_ws.rs (2)

345-356: 💤 Low value

Тест probe_tcp_fails_when_no_listener теоретически flaky.

Между drop(listener) и probe_tcp(addr, …) ОС может выдать тот же ephemeral-порт другому процессу (CI с пулом параллельных тестов), и тогда connect неожиданно успеет. Безопаснее использовать заведомо «тёмный» адрес: 127.0.0.1:1 или IpAddr::from([127,0,0,2]) без листенера, либо 192.0.2.1:65535 (TEST-NET-1, RFC 5737) с очень коротким таймаутом.

🤖 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/use_cases/mcp_ws.rs` around lines 345 - 356, The test
probe_tcp_fails_when_no_listener is flaky because dropping an ephemeral
TcpListener can let the OS assign that port to another process before
probe_tcp(addr, …) runs; update the test to use a guaranteed-unused "dark"
address instead of binding/dropping a listener — e.g., call
probe_tcp(SocketAddr::from(([127,0,0,1], 1)), Duration::from_millis(50)) or
probe_tcp(SocketAddr::from(([192,0,2,1], 65535)), short_timeout) in the
probe_tcp_fails_when_no_listener test so the OS cannot hand the port to another
process and the assertion reliably fails when no listener exists.

73-79: ⚡ Quick win

Несоответствие в обработке регистра букв: log_level более строг, чем transport.

Обнаружена реальная проблема, но анализ в исходном комментарии содержит неточности:

  1. Case-sensitivity несоответствие подтверждено: McpClientTransport::from_str_value (строка 45) использует .to_ascii_lowercase() и принимает LEGACY, но is_supported_log_level (строка 77-79) случай-чувствителен и отклонит log_level = "INFO" в конфиге.

  2. Исходный анализ неправильно указал точки вызова: is_supported_log_level вызывается не в launch_app, а в:

    • src/config/validate.rs:841 — для валидации конфига
    • src/cli/execute.rs:1212 — для валидации CLI аргументов

    Для CLI аргументов также существует value_parser = ["off", "error", "warn", "info", "debug", "trace"] в src/cli/args.rs:348, который обеспечивает case-sensitive валидацию на уровне partera clap.

Рекомендация: унифицировать обработку регистра в is_supported_log_level, сделав её case-insensitive, или документировать требование lowercase в комментарии функции и в схеме конфига.

♻️ Унификация case-insensitive сравнения
-pub fn is_supported_log_level(level: &str) -> bool {
-    ALLOWED_LOG_LEVELS.contains(&level)
+pub fn is_supported_log_level(level: &str) -> bool {
+    ALLOWED_LOG_LEVELS
+        .iter()
+        .any(|allowed| allowed.eq_ignore_ascii_case(level))
 }
🤖 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/use_cases/mcp_ws.rs` around lines 73 - 79, Функция is_supported_log_level
и константа ALLOWED_LOG_LEVELS обрабатывают регистр по-разному относительно
McpClientTransport::from_str_value; исправьте это, сделав в
is_supported_log_level сравнение нечувствительным к регистру: внутри функции
привести входной параметр level к нижнему регистру (например через
to_ascii_lowercase()) и затем проверять наличие в ALLOWED_LOG_LEVELS, либо явно
задокументировать требование lowercase в комментарии функции и в схеме конфига;
проверьте связанные места вызова (validate.rs и cli/execute.rs) и при
необходимости обновите сообщения/доки для согласованности.
🤖 Prompt for all review comments with 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.

Inline comments:
In `@​README.md`:
- Around line 96-97: Замените относительную ссылку ../v8-client-session-manager/
в README.md на абсолютный URL репозитория на GitHub (например
https://github.com/OWNER/v8-client-session-manager) или на правильную внешнюю
ссылку, сохранив текст ссылки `v8-client-session-manager`; обновите
соответствующую Markdown-скобочную запись, чтобы GitHub корректно переходил по
ссылке из README.

In `@​src/config/validate.rs`:
- Around line 852-858: The current validation in validate.rs only checks
parse_manager_addr but does not forbid semicolons, allowing injection via
config.tools.client_mcp.manager_url; update the validation to reject any
manager_url containing ';' (or otherwise enforce a payload-safe character set)
before calling crate::use_cases::mcp_ws::parse_manager_addr and return
ConfigValidationError::InvalidMcpClientManagerUrl for such values; also reuse
the same payload-safe check in the CLI converter code path so both config
loading and CLI-derived manager_url use identical validation.

In `@​src/use_cases/mcp_ws.rs`:
- Around line 154-157: Функция default_corr_id молча возвращает слишком короткий
идентификатор при коротком client_uid (например "abc" -> "vr-abc"), нарушая
контракт "vr-<uid8>"; исправь функцию default_corr_id так, чтобы она выравнивала
результат до 8 символов: при фильтрации символов из client_uid (в существующей
логике в default_corr_id) дополни строку справа нулями (или другим выбранным
fill-символом) до длины 8, либо — если предпочитаешь — при коротком/пустом
client_uid вместо паддинга генерируй новый идентификатор на основе
Uuid::new_v4() и используйте его первые 8 символов; внеси изменения именно в
default_corr_id и убедись, что формат финальной строки всегда "vr-<8char>".
- Around line 92-106: The payload_snippet in WsLaunchParams naively inlines
fields (manager_url, client_uid, kind.as_str(), corr_id, log_level,
ws_timeout_ms) into a semicolon/equals-delimited string, allowing
format-injection if any value contains ';' or '='; fix by adding validation or
escaping: either (a) validate each input with an allowlist regex and return
Result<String, Error> from payload_snippet on invalid input, or (b)
sanitize/escape forbidden characters (at minimum replace or percent-encode ';'
and '=') before formatting the snippet so the resulting "mcpMode=ws;..." cannot
be split into extra key/value pairs. Ensure changes are applied inside
WsLaunchParams::payload_snippet (or change its signature to return Result) and
include the field names above to locate the code.
- Around line 216-252: parse_manager_addr currently does a blocking DNS lookup
via host_port.to_socket_addrs() which can stall startup; replace the blocking
resolution with an async, timeout-bound lookup (use tokio::net::lookup_host
wrapped with tokio::time::timeout using PROBE_TIMEOUT_MS) and make
parse_manager_addr async (or add an async helper called e.g.
resolve_manager_addr_async) so you enforce the 200ms probe timeout and return
WsResolveError on timeout; alternatively, if you cannot make the call async,
update the doc-comment for parse_manager_addr to explicitly forbid hostnames
(only accept literal IP:port) and validate/return a clear error for non-IP
inputs. Ensure you update call sites that use parse_manager_addr to await the
new async resolver or to reject hostnames.

In `@​src/use_cases/run_tests/helpers.rs`:
- Around line 448-453: The current match on
crate::use_cases::launch_app::decide_mcp_transport in apply_test_mcp_ws_payload
swallows the error (tracing::warn! then return), which lets an explicit
--mcp-transport ws fall back silently; change apply_test_mcp_ws_payload to
return Result<..., Error> and when decide_mcp_transport returns Err propagate
that Err upward (i.e., return Err(err.into())) for the explicit WS mode, while
preserving a soft fallback only for the auto path; update callers to handle the
Result and remove the early silent return so failures for explicit WS are
surfaced.

In `@​v8-runner/references/bootstrap.md`:
- Line 48: The documentation incorrectly references the non-existent config key
`tools.connection`; update the text and any examples to use the correct key
`infobase.connection` so bootstrap produces a valid v8project.yaml; search for
and replace occurrences of `tools.connection` in the
v8-runner/references/bootstrap.md content (including the sentence describing the
three common connection shapes) and verify examples and sample YAML show
`infobase.connection` consistently.

---

Outside diff comments:
In `@​src/config/schema.rs`:
- Around line 234-236: The JSON Schema generator currently only applies numeric
bounds for "port" in ClientMcpToolSchema/PartialClientMcpToolSchema, so
ws_timeout_ms can be zero and fails later; update the same boundary generation
to also set a minimum > 0 for "ws_timeout_ms" by calling
set_numeric_bounds(schema, &[def], "ws_timeout_ms", Some(1), None) alongside the
existing call for "port" (refer to the loop over def values and the
set_numeric_bounds function to locate where to add the new call).

---

Nitpick comments:
In `@​src/domain/launch.rs`:
- Around line 18-21: Поле transport в структуре Launch нужно заменить с
Option<String> на типизированный enum — создать enum Transport { Ws, Legacy } с
#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)] и
#[serde(rename_all = "snake_case")] и использовать Option<Transport> с теми же
атрибутами #[serde(skip_serializing_if = "Option::is_none", default)] на поле
transport; затем поправить все места, где сравнивали или использовали transport
(паттерн-матчинг в коде) чтобы работать с Transport::Ws / Transport::Legacy, а
также обновить тесты/сериализацию, где ожидалось строковое значение.

In `@​src/use_cases/launch_app.rs`:
- Around line 243-257: Ветка TransportDecision::Ws игнорирует переданные
client_mcp.port и client_mcp.config_path без уведомления пользователя; добавь
проверку перед/внутри матча (в функции где вызываются
launch_mcp_client_kind/decide_mcp_transport — вокруг переменных args.mcp_ws и
client_mcp) и при выборе Ws логгируй warn! если client_mcp.port.is_some() ||
client_mcp.config_path.is_some(); дополнительно, если транспорт явно задан как
Ws в args.mcp_ws, преобразуй это в validation error (Err) вместо молчалого
игнорирования, используя те же символы TransportDecision::Ws,
resolve_ws_launch_params и McpResolutionMeta::Ws для локализации изменения.

In `@​src/use_cases/mcp_ws.rs`:
- Around line 345-356: The test probe_tcp_fails_when_no_listener is flaky
because dropping an ephemeral TcpListener can let the OS assign that port to
another process before probe_tcp(addr, …) runs; update the test to use a
guaranteed-unused "dark" address instead of binding/dropping a listener — e.g.,
call probe_tcp(SocketAddr::from(([127,0,0,1], 1)), Duration::from_millis(50)) or
probe_tcp(SocketAddr::from(([192,0,2,1], 65535)), short_timeout) in the
probe_tcp_fails_when_no_listener test so the OS cannot hand the port to another
process and the assertion reliably fails when no listener exists.
- Around line 73-79: Функция is_supported_log_level и константа
ALLOWED_LOG_LEVELS обрабатывают регистр по-разному относительно
McpClientTransport::from_str_value; исправьте это, сделав в
is_supported_log_level сравнение нечувствительным к регистру: внутри функции
привести входной параметр level к нижнему регистру (например через
to_ascii_lowercase()) и затем проверять наличие в ALLOWED_LOG_LEVELS, либо явно
задокументировать требование lowercase в комментарии функции и в схеме конфига;
проверьте связанные места вызова (validate.rs и cli/execute.rs) и при
необходимости обновите сообщения/доки для согласованности.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0eb191ec-f001-4036-963e-b9572403f54b

📥 Commits

Reviewing files that changed from the base of the PR and between 30871f6 and 17f3e05.

📒 Files selected for processing (32)
  • .gitignore
  • AGENTS.md
  • README.md
  • SKILL/references/project-workflows.md
  • docs/CONFIGURATION.md
  • docs/schemas/v8project.local.schema.json
  • docs/schemas/v8project.schema.json
  • spec/decisions/0022-universalnyy-mehanizm-podgotovki-rasshireniy-i-client-mcp-extension.md
  • src/cli/args.rs
  • src/cli/execute.rs
  • src/config/model.rs
  • src/config/schema.rs
  • src/config/validate.rs
  • src/domain/launch.rs
  • src/mcp/service.rs
  • src/use_cases/launch_app.rs
  • src/use_cases/mcp_ws.rs
  • src/use_cases/mod.rs
  • src/use_cases/request.rs
  • src/use_cases/run_tests.rs
  • src/use_cases/run_tests/coordinator.rs
  • src/use_cases/run_tests/helpers.rs
  • tests/cli_launch.rs
  • v8-runner/SKILL.md
  • v8-runner/agents/openai.yaml
  • v8-runner/references/bootstrap.md
  • v8-runner/references/command-selection.md
  • v8-runner/references/config-and-backends.md
  • v8-runner/references/file-and-artifact-workflows.md
  • v8-runner/references/project-workflows.md
  • v8-runner/references/testing.md
  • v8-runner/references/troubleshooting.md
💤 Files with no reviewable changes (1)
  • SKILL/references/project-workflows.md

@zeegin

zeegin commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

Upstream review by coderabbitai[bot]; state: COMMENTED; submitted: 2026-05-13T12:18:45Z; commit: 68acc8ff12e93e1c6963fd7e7986451797c7ad4e. Original review

Actionable comments posted: 4

🤖 Prompt for all review comments with 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.

Inline comments:
In `@​docs/CONFIGURATION.md`:
- Around line 457-458: Описание параметра ws_timeout_ms неверно ограничивает его
только режимом auto; обновите текст так, чтобы оно отражало, что ws_timeout_ms
передаётся как mcp_ws_timeout_ms в параметры WebSocket в целом (не только для
auto) и указывает таймаут установки WS-сессии в миллисекундах (>0). Обновите
упоминание `ws_timeout_ms` и ссылку на `mcp_ws_timeout_ms` в документации, убрав
фразу про режим `auto` и сохранив требование >0, чтобы соответствовать
фактической реализации.

In `@​src/cli/execute.rs`:
- Around line 1234-1238: Вместо всегда возвращать одно и то же сообщение при
ошибке разбора менеджера, поймайте результат parse_manager_addr(url) и при
Err(e) пробросьте диагностичную ошибку, включающую текст причины из e (например
через e.to_string()) в UseCaseError::new(UseCaseErrorKind::Validation, ...),
заменив текущее фиксированное "must include host:port" на сообщение,
комбинирующее контекст ("--manager-url parse error") и самую причину из
parse_manager_addr; отредактируйте соответствующий условный блок в execute.rs
вокруг вызова parse_manager_addr(url).

In `@​tests/cli_launch.rs`:
- Around line 721-723: The test hardcodes an invalid/unstable CLI contract by
passing "--mcp-transport" "mcp"; update the test(s) in tests/cli_launch.rs to
use the supported public transport value (e.g. "legacy" or "auto") or, better,
derive the expected value from the CLI enum/constant used by the parser instead
of the literal "mcp". Locate the occurrences of the "--mcp-transport" / "mcp"
pair in the test and replace them with either a valid literal ("legacy"/"auto")
or a reference to the canonical constant/enum used by the CLI implementation so
the test stays correct if the contract changes.

In `@​v8-runner/references/command-selection.md`:
- Around line 169-173: Документирует несогласованность публичного флага
--mcp-transport: примеры и текст используют "mcp", тогда как контракт в PR —
{ws|legacy|auto}; приведите документацию в соответствие с контрактом: в примерах
команды (например "v8-runner launch mcp" и упоминания "test yaxunit ...", "test
va ...") замените значение "mcp" на "legacy" или добавьте явный алиас/примечание
о совместимости, и обновите поясняющий абзац про поведение по умолчанию
(--mcp-transport=auto) чтобы перечислить корректные варианты {ws|legacy|auto} и
описать коротко, как происходит выбор; убедитесь, что терминология в разделе
"WS-режим" и упоминания internal kind mapping/--json-message согласованы с этим
изменением.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0a8ab6df-b707-4da3-ad5c-89ccc9185e91

📥 Commits

Reviewing files that changed from the base of the PR and between 17f3e05 and 68acc8f.

📒 Files selected for processing (20)
  • README.md
  • docs/CONFIGURATION.md
  • docs/schemas/v8project.local.schema.json
  • docs/schemas/v8project.schema.json
  • src/cli/args.rs
  • src/cli/execute.rs
  • src/config/model.rs
  • src/config/schema.rs
  • src/config/validate.rs
  • src/domain/launch.rs
  • src/use_cases/launch_app.rs
  • src/use_cases/mcp_ws.rs
  • src/use_cases/request.rs
  • src/use_cases/run_tests/coordinator.rs
  • src/use_cases/run_tests/helpers.rs
  • tests/cli_launch.rs
  • v8-runner/SKILL.md
  • v8-runner/references/bootstrap.md
  • v8-runner/references/command-selection.md
  • v8-runner/references/project-workflows.md
✅ Files skipped from review due to trivial changes (2)
  • v8-runner/SKILL.md
  • README.md
🚧 Files skipped from review as they are similar to previous changes (7)
  • src/config/validate.rs
  • docs/schemas/v8project.local.schema.json
  • src/config/model.rs
  • docs/schemas/v8project.schema.json
  • src/use_cases/run_tests/coordinator.rs
  • src/cli/args.rs
  • src/use_cases/launch_app.rs

@zeegin

zeegin commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

Upstream review by coderabbitai[bot]; state: COMMENTED; submitted: 2026-05-13T13:16:24Z; commit: cbaf405442a07a4e79dd3664d96019d2620fc0d6. Original review

Actionable comments posted: 1

🤖 Prompt for all review comments with 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.

Inline comments:
In `@​docs/CONFIGURATION.md`:
- Line 460: В строке документации с фрагментом `/C"runMcp...` есть непарная
двойная кавычка; исправьте эту последовательность на корректную пару кавычек
(например `/C"runMcp..."`) в описании команды `launch mcp` где упоминается
передача `port` как `mcpPort`, чтобы закрывающая кавычка после `...` была
добавлена и синтаксис строки стал корректным.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 35a61bfd-fdce-48d5-aece-67d3b895fd3c

📥 Commits

Reviewing files that changed from the base of the PR and between 68acc8f and cbaf405.

📒 Files selected for processing (3)
  • docs/CONFIGURATION.md
  • src/cli/execute.rs
  • v8-runner/references/bootstrap.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/cli/execute.rs

@zeegin

zeegin commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

Upstream review by coderabbitai[bot]; state: COMMENTED; submitted: 2026-06-27T14:54:33Z; commit: aa3e45ac52132e3e856d183773db00847a99e6ca. Original review

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
v8-runner/SKILL.md (1)

70-103: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Уточните transport-контракт для обычных launch thin|thick|ordinary.

Сейчас этот блок читается так, будто для обычного UI-клиента поддержаны auto и local HTTP MCP fallback, но apply_direct_mcp_ws_launch реально пропускает там только WS-вариант; ветка Mcp для не-launch mcp завершается validation-ошибкой. Лучше явно сузить этот текст до --mcp-transport=ws для обычных клиентских launch-ов и оставить auto/mcp только за launch mcp*. As per coding guidelines, "Keep v8-runner/SKILL.md up to date when commands, workflow, configuration contracts, limits, or diagnostic practices change in ways that matter to external 1C projects; updates should be brief and avoid internal implementation detail."

🤖 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 `@​v8-runner/SKILL.md` around lines 70 - 103, Clarify the transport contract for
the ordinary UI launch modes by updating the `launch thin|thick|ordinary`
guidance to mention only `--mcp-transport=ws`; the current wording incorrectly
implies `auto` and local HTTP MCP fallback are supported there. Adjust the
affected bullets in `SKILL.md` around `launch mcp` / `launch mcp va` and the
`Launch Modes` table so `auto` and `mcp` are documented only for `launch mcp*`,
matching the actual behavior enforced by `apply_direct_mcp_ws_launch` and the
non-`launch mcp` validation path.

Source: Coding guidelines

🤖 Prompt for all review comments with 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.

Outside diff comments:
In `@​v8-runner/SKILL.md`:
- Around line 70-103: Clarify the transport contract for the ordinary UI launch
modes by updating the `launch thin|thick|ordinary` guidance to mention only
`--mcp-transport=ws`; the current wording incorrectly implies `auto` and local
HTTP MCP fallback are supported there. Adjust the affected bullets in `SKILL.md`
around `launch mcp` / `launch mcp va` and the `Launch Modes` table so `auto` and
`mcp` are documented only for `launch mcp*`, matching the actual behavior
enforced by `apply_direct_mcp_ws_launch` and the non-`launch mcp` validation
path.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2830f5b6-b094-4fe8-95c1-74b604b251fe

📥 Commits

Reviewing files that changed from the base of the PR and between cbaf405 and aa3e45a.

📒 Files selected for processing (11)
  • docs/CAPABILITIES.md
  • docs/CONFIGURATION.md
  • src/platform/enterprise.rs
  • src/platform/process.rs
  • src/use_cases/launch_app.rs
  • src/use_cases/launch_keys.rs
  • src/use_cases/mcp_ws.rs
  • src/use_cases/vanessa.rs
  • tests/cli_launch.rs
  • tests/cli_test.rs
  • v8-runner/SKILL.md
✅ Files skipped from review due to trivial changes (1)
  • docs/CAPABILITIES.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • docs/CONFIGURATION.md
  • src/use_cases/mcp_ws.rs

@zeegin

zeegin commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

Upstream inline review comment by coderabbitai[bot] at 2026-05-07T10:34:35Z; path: README.md, original line: 97. Original comment

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Относительная ссылка ../v8-client-session-manager/ не работает на GitHub

GitHub не умеет переходить за пределы репозитория по относительным путям в README — ссылка приведёт к 404. Если v8-client-session-manager — отдельный публичный репозиторий, замените на абсолютный URL.

🛠️ Предлагаемое исправление
-1С-клиента к [`v8-client-session-manager`](../v8-client-session-manager/) вместо запуска
+1С-клиента к [`v8-client-session-manager`](https://github.com/<org>/v8-client-session-manager) вместо запуска
🤖 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 `@​README.md` around lines 96 - 97, Замените относительную ссылку
../v8-client-session-manager/ в README.md на абсолютный URL репозитория на
GitHub (например https://github.com/OWNER/v8-client-session-manager) или на
правильную внешнюю ссылку, сохранив текст ссылки `v8-client-session-manager`;
обновите соответствующую Markdown-скобочную запись, чтобы GitHub корректно
переходил по ссылке из README.

✅ Addressed in commit 68acc8f

@zeegin

zeegin commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

Upstream inline review comment by coderabbitai[bot] at 2026-05-07T10:34:35Z; path: src/config/validate.rs, line: 863. Original comment

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Добавьте защиту от ; в manager_url для WS payload.

Сейчас на Line 852-858 проверяется только формат адреса, но не запрещается ;. Для /C с ;-разделителем это ломает разбор и позволяет инъекцию дополнительных пар key=value.

Тут лучше валидировать manager_url как payload-safe (минимум запрет ;) и переиспользовать ту же проверку в CLI-конвертере.

Предлагаемое изменение
     if let Some(url) = config.tools.client_mcp.manager_url.as_deref() {
+        if url.contains(';') {
+            return Err(ConfigValidationError::InvalidMcpClientManagerUrl(
+                url.to_owned(),
+            ));
+        }
         if crate::use_cases::mcp_ws::parse_manager_addr(url).is_err() {
             return Err(ConfigValidationError::InvalidMcpClientManagerUrl(
                 url.to_owned(),
             ));
         }
     }
🤖 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/config/validate.rs` around lines 852 - 858, The current validation in
validate.rs only checks parse_manager_addr but does not forbid semicolons,
allowing injection via config.tools.client_mcp.manager_url; update the
validation to reject any manager_url containing ';' (or otherwise enforce a
payload-safe character set) before calling
crate::use_cases::mcp_ws::parse_manager_addr and return
ConfigValidationError::InvalidMcpClientManagerUrl for such values; also reuse
the same payload-safe check in the CLI converter code path so both config
loading and CLI-derived manager_url use identical validation.

✅ Addressed in commit 68acc8f

@zeegin

zeegin commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

Upstream inline review comment by coderabbitai[bot] at 2026-05-07T10:34:36Z; path: src/use_cases/mcp_ws.rs, line: 127. Original comment

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

В payload_snippet нет валидации/экранирования спецсимволов разделителей.

manager_url, client_uid, corr_id, log_level инлайнятся в строку с разделителями ; и = без проверки. Если в --manager-url или --corr-id попадёт значение с ; или = (а пользовательские UUID/идентификаторы CI часто содержат =), снэпет молча развалится на лишние пары и BSL-парсер Мсп_ПараметрыЗапускаКлиент интерпретирует мусор как валидный ключ — это полноценный format-injection.

Предлагаю либо валидировать значения здесь же (regex-allowlist для каждого поля), либо добавить дешёвый guard:

🛡️ Минимальный guard на forbidden chars
 impl WsLaunchParams {
+    fn assert_no_separator(field: &str, value: &str) {
+        debug_assert!(
+            !value.contains(|c: char| c == ';' || c == '='),
+            "ws payload field '{field}' must not contain ';' or '=': {value:?}"
+        );
+    }
+
     pub fn payload_snippet(&self) -> String {
+        Self::assert_no_separator("manager_url", &self.manager_url);
+        Self::assert_no_separator("client_uid", &self.client_uid);
+        Self::assert_no_separator("corr_id", &self.corr_id);
+        Self::assert_no_separator("log_level", &self.log_level);
         format!(
             "mcpMode=ws;manager_url={};client_uid={};kind={};corr_id={};mcp_log_level={};mcp_ws_timeout_ms={}",

Чище — поднять валидацию на CLI/config-уровне и возвращать Result<String, _> отсюда.

🤖 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/use_cases/mcp_ws.rs` around lines 92 - 106, The payload_snippet in
WsLaunchParams naively inlines fields (manager_url, client_uid, kind.as_str(),
corr_id, log_level, ws_timeout_ms) into a semicolon/equals-delimited string,
allowing format-injection if any value contains ';' or '='; fix by adding
validation or escaping: either (a) validate each input with an allowlist regex
and return Result<String, Error> from payload_snippet on invalid input, or (b)
sanitize/escape forbidden characters (at minimum replace or percent-encode ';'
and '=') before formatting the snippet so the resulting "mcpMode=ws;..." cannot
be split into extra key/value pairs. Ensure changes are applied inside
WsLaunchParams::payload_snippet (or change its signature to return Result) and
include the field names above to locate the code.

✅ Addressed in commit 68acc8f

@zeegin

zeegin commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

Upstream inline review comment by coderabbitai[bot] at 2026-05-07T10:34:36Z; path: src/use_cases/mcp_ws.rs, line: 189. Original comment

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

default_corr_id молча выдаёт укороченный id для коротких user-supplied client_uid.

take(8) без pad-а: если пользователь передал свой client_uid (например, тестовое "abc") — corr_id станет "vr-abc", что нарушает контракт «vr-» из PR-описания. На production UUIDv4 не проявляется, но регрессия проявится в CI с искусственными значениями.

Предлагаю либо паддить нулями до 8 символов, либо генерировать corr_id из свежего Uuid::new_v4() независимо, если на входе нестандартный uid.

🤖 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/use_cases/mcp_ws.rs` around lines 154 - 157, Функция default_corr_id
молча возвращает слишком короткий идентификатор при коротком client_uid
(например "abc" -> "vr-abc"), нарушая контракт "vr-<uid8>"; исправь функцию
default_corr_id так, чтобы она выравнивала результат до 8 символов: при
фильтрации символов из client_uid (в существующей логике в default_corr_id)
дополни строку справа нулями (или другим выбранным fill-символом) до длины 8,
либо — если предпочитаешь — при коротком/пустом client_uid вместо паддинга
генерируй новый идентификатор на основе Uuid::new_v4() и используйте его первые
8 символов; внеси изменения именно в default_corr_id и убедись, что формат
финальной строки всегда "vr-<8char>".

✅ Addressed in commit 68acc8f

@zeegin

zeegin commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

Upstream inline review comment by coderabbitai[bot] at 2026-05-07T10:34:36Z; path: src/use_cases/mcp_ws.rs, line: 290. Original comment

⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoff

🧩 Analysis chain

🌐 Web query:

Rust std::net::ToSocketAddrs blocking DNS no timeout best practices

💡 Result:

std::net::ToSocketAddrs performs blocking DNS resolution via the OS's getaddrinfo (or equivalent), with no configurable timeout in std; it relies on OS defaults (e.g., ~5s on Linux, 15s on Windows) [1][2][3]. The docs explicitly warn it "may block the current thread while resolution is performed" [1]. TcpStream::connect(A) where A: ToSocketAddrs first calls to_socket_addrs (blocking), then attempts connections sequentially without connect timeout [4]. TcpStream::connect_timeout requires a single &SocketAddr, so cannot directly use ToSocketAddrs [4]. Best practices to avoid indefinite blocking: 1. Use async runtimes: - Tokio: tokio::net::lookup_host(host_port).await? resolves non-blockingly (uses spawn_blocking internally) [5][6]. Then tokio::net::TcpStream::connect(addr).await? with timeout via tokio::time::timeout [6]. - async-std: similar async ToSocketAddrs [7]. 2. Synchronous with timeout: Spawn a thread for resolution, use std::thread::spawn and crossbeam_channel or similar with timeout [3][8]. Example: use std::net::{ToSocketAddrs, SocketAddr}; use std::time::Duration; use crossbeam_channel::{bounded, RecvTimeoutError}; fn resolve_with_timeout<T: ToSocketAddrs + Send + 'static>(addr: T, timeout: Duration) -> Result<Vec, Box> { let (tx, rx) = bounded(1); std::thread::spawn(move || { tx.send(addr.to_socket_addrs.unwrap.collect).unwrap; }); match rx.recv_timeout(timeout) { Ok(addrs) => Ok(addrs), Err(RecvTimeoutError::TimedOut) => Err("DNS timeout".into), _ => Err("channel error".into), } } [8][3] 3. Custom resolvers with timeouts: - hickory-resolver (formerly trust-dns-resolver): Tokio-based, configurable timeouts via ResolverBuilder [9]. Use resolver.lookup_ip(host)?.into_iter.map(|ip| SocketAddr::new(ip, port)) [9][10]. - hickory-to-socket-addrs crate: Direct ToSocketAddrs replacement using hickory-dns [10][11]. - c-ares-resolver: Configurable timeouts [12]. For production, prefer async (Tokio/async-std) or dedicated resolvers like hickory-resolver to avoid blocking and enable timeouts [10][3][6][9]. OS defaults prevent true indefinite hangs, but explicit control is best.

Citations:


🏁 Script executed:

# Find PROBE_TIMEOUT_MS definition
rg "PROBE_TIMEOUT_MS" --type rust

Repository: alkoleft/v8-runner-rust

Length of output: 239


🏁 Script executed:

# Find all call sites of parse_manager_addr
rg "parse_manager_addr" --type rust -B 2 -A 2

Repository: alkoleft/v8-runner-rust

Length of output: 2877


🏁 Script executed:

# Check the file context around parse_manager_addr
head -n 300 src/use_cases/mcp_ws.rs | tail -n 100

Repository: alkoleft/v8-runner-rust

Length of output: 3257


🏁 Script executed:

# Look for async context and timeout usage in the file
rg "tokio::|timeout|async" src/use_cases/mcp_ws.rs -A 2 -B 2

Repository: alkoleft/v8-runner-rust

Length of output: 2806


parse_manager_addr выполняет синхронный DNS-lookup без таймаута, что нарушает бюджет 200 мс для запуска приложения.

to_socket_addrs() вызывает блокирующий getaddrinfo(3) в текущем потоке. Константа PROBE_TIMEOUT_MS=200 применяется только к connect_timeout, а не к резолюции имён. Для IP-адреса (127.0.0.1:4000) это неопасно, но любой hostname в manager_url на упавшем или медленном DNS может заблокировать запуск на 5+ секунд (OS defaults: ~5s Linux, ~15s Windows), что особенно критично для режима transport=auto, который предполагает быструю откатку на Legacy при недоступности сервера.

Минимум — добавить в doc-комментарий предупреждение о блокировке DNS. Лучше — либо перейти на асинхронное разрешение (tokio::net::lookup_host() + tokio::time::timeout), либо ограничить поддерживаемые форматы manager_url IP-адресами для режима auto.

🤖 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/use_cases/mcp_ws.rs` around lines 216 - 252, parse_manager_addr currently
does a blocking DNS lookup via host_port.to_socket_addrs() which can stall
startup; replace the blocking resolution with an async, timeout-bound lookup
(use tokio::net::lookup_host wrapped with tokio::time::timeout using
PROBE_TIMEOUT_MS) and make parse_manager_addr async (or add an async helper
called e.g. resolve_manager_addr_async) so you enforce the 200ms probe timeout
and return WsResolveError on timeout; alternatively, if you cannot make the call
async, update the doc-comment for parse_manager_addr to explicitly forbid
hostnames (only accept literal IP:port) and validate/return a clear error for
non-IP inputs. Ensure you update call sites that use parse_manager_addr to await
the new async resolver or to reject hostnames.

✅ Addressed in commit 68acc8f

@zeegin

zeegin commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

Upstream inline review comment by coderabbitai[bot] at 2026-05-07T10:34:36Z; path: src/use_cases/run_tests/helpers.rs, original line: 453. Original comment

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Не глотайте ошибку резолва транспорта для test при явном --mcp-transport ws.

На Line 448-453 ошибка decide_mcp_transport только логируется и выполнение продолжается. Это нарушает ожидаемый контракт «ws обязателен», и тест может тихо уйти в legacy-поведение.

Сделайте apply_test_mcp_ws_payload возвращающей Result и пробрасывайте ошибку вверх хотя бы для принудительного WS-режима (для auto можно оставить мягкий fallback).

🤖 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/use_cases/run_tests/helpers.rs` around lines 448 - 453, The current match
on crate::use_cases::launch_app::decide_mcp_transport in
apply_test_mcp_ws_payload swallows the error (tracing::warn! then return), which
lets an explicit --mcp-transport ws fall back silently; change
apply_test_mcp_ws_payload to return Result<..., Error> and when
decide_mcp_transport returns Err propagate that Err upward (i.e., return
Err(err.into())) for the explicit WS mode, while preserving a soft fallback only
for the auto path; update callers to handle the Result and remove the early
silent return so failures for explicit WS are surfaced.

✅ Addressed in commit 68acc8f

@zeegin

zeegin commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

Upstream inline review comment by coderabbitai[bot] at 2026-05-07T10:34:36Z; path: v8-runner/references/bootstrap.md, original line: 48. Original comment

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Неверный путь ключа в конфиге: tools.connectioninfobase.connection.

На Line 48 указан несуществующий ключ. Это вводит в заблуждение при bootstrap и приводит к невалидному v8project.yaml.

🤖 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 `@​v8-runner/references/bootstrap.md` at line 48, The documentation incorrectly
references the non-existent config key `tools.connection`; update the text and
any examples to use the correct key `infobase.connection` so bootstrap produces
a valid v8project.yaml; search for and replace occurrences of `tools.connection`
in the v8-runner/references/bootstrap.md content (including the sentence
describing the three common connection shapes) and verify examples and sample
YAML show `infobase.connection` consistently.

✅ Addressed in commit 68acc8f

@zeegin

zeegin commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

Upstream inline review comment by coderabbitai[bot] at 2026-05-13T12:18:43Z; path: docs/CONFIGURATION.md, original line: 458. Original comment

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Уточните описание ws_timeout_ms: сейчас оно излишне ограничено auto-режимом.

Текст говорит, что это таймаут только для auto, но поле прокидывается как mcp_ws_timeout_ms в WS-параметры в целом. Лучше убрать ограничение на auto, чтобы документация соответствовала поведению.

🤖 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 `@​docs/CONFIGURATION.md` around lines 457 - 458, Описание параметра
ws_timeout_ms неверно ограничивает его только режимом auto; обновите текст так,
чтобы оно отражало, что ws_timeout_ms передаётся как mcp_ws_timeout_ms в
параметры WebSocket в целом (не только для auto) и указывает таймаут установки
WS-сессии в миллисекундах (>0). Обновите упоминание `ws_timeout_ms` и ссылку на
`mcp_ws_timeout_ms` в документации, убрав фразу про режим `auto` и сохранив
требование >0, чтобы соответствовать фактической реализации.

✅ Addressed in commit cbaf405

@zeegin

zeegin commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

Upstream inline review comment by coderabbitai[bot] at 2026-05-13T12:18:43Z; path: src/cli/execute.rs, original line: 1238. Original comment

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Сделайте ошибку --manager-url диагностичной, а не общей.

Сейчас для разных причин возвращается одно и то же сообщение must include host:port; при hostname пользователь получает вводящий в заблуждение текст. Лучше пробрасывать причину из parse_manager_addr.

💡 Предлагаемый патч
-        if crate::use_cases::mcp_ws::parse_manager_addr(url).is_err() {
+        if let Err(err) = crate::use_cases::mcp_ws::parse_manager_addr(url) {
             return Err(UseCaseError::new(
                 UseCaseErrorKind::Validation,
-                format!("--manager-url must include host:port (got: {url})"),
+                format!("--manager-url is invalid: {err}"),
             ));
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

        if let Err(err) = crate::use_cases::mcp_ws::parse_manager_addr(url) {
            return Err(UseCaseError::new(
                UseCaseErrorKind::Validation,
                format!("--manager-url is invalid: {err}"),
            ));
        }
🤖 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/cli/execute.rs` around lines 1234 - 1238, Вместо всегда возвращать одно и
то же сообщение при ошибке разбора менеджера, поймайте результат
parse_manager_addr(url) и при Err(e) пробросьте диагностичную ошибку, включающую
текст причины из e (например через e.to_string()) в
UseCaseError::new(UseCaseErrorKind::Validation, ...), заменив текущее
фиксированное "must include host:port" на сообщение, комбинирующее контекст
("--manager-url parse error") и самую причину из parse_manager_addr;
отредактируйте соответствующий условный блок в execute.rs вокруг вызова
parse_manager_addr(url).

✅ Addressed in commit cbaf405

@zeegin

zeegin commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

Upstream inline review comment by coderabbitai[bot] at 2026-05-13T12:18:43Z; path: tests/cli_launch.rs, line: 908. Original comment

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Тесты закрепляют потенциально неверный CLI-контракт (mcp вместо legacy).

Здесь и далее новые тесты утверждают --mcp-transport mcp; если целевой публичный контракт действительно {ws|legacy|auto}, такие проверки консервируют несовместимость и маскируют регрессию в интерфейсе.

🤖 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 `@​tests/cli_launch.rs` around lines 721 - 723, The test hardcodes an
invalid/unstable CLI contract by passing "--mcp-transport" "mcp"; update the
test(s) in tests/cli_launch.rs to use the supported public transport value (e.g.
"legacy" or "auto") or, better, derive the expected value from the CLI
enum/constant used by the parser instead of the literal "mcp". Locate the
occurrences of the "--mcp-transport" / "mcp" pair in the test and replace them
with either a valid literal ("legacy"/"auto") or a reference to the canonical
constant/enum used by the CLI implementation so the test stays correct if the
contract changes.

✅ Addressed in commits eb61928 to aa3e45a

@zeegin

zeegin commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

Upstream inline review comment by coderabbitai[bot] at 2026-05-13T12:18:43Z; path: v8-runner/references/command-selection.md, line: 173. Original comment

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Синхронизируйте публичный контракт --mcp-transport (mcp vs legacy).

В этих строках фиксируется значение mcp, но в целях PR заявлен контракт --mcp-transport={ws|legacy|auto}. Это риск несовместимости для пользователей и CI: либо документация неверна, либо фактический CLI-контракт изменён без явной миграции/алиаса.

🤖 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 `@​v8-runner/references/command-selection.md` around lines 169 - 173,
Документирует несогласованность публичного флага --mcp-transport: примеры и
текст используют "mcp", тогда как контракт в PR — {ws|legacy|auto}; приведите
документацию в соответствие с контрактом: в примерах команды (например
"v8-runner launch mcp" и упоминания "test yaxunit ...", "test va ...") замените
значение "mcp" на "legacy" или добавьте явный алиас/примечание о совместимости,
и обновите поясняющий абзац про поведение по умолчанию (--mcp-transport=auto)
чтобы перечислить корректные варианты {ws|legacy|auto} и описать коротко, как
происходит выбор; убедитесь, что терминология в разделе "WS-режим" и упоминания
internal kind mapping/--json-message согласованы с этим изменением.

@zeegin

zeegin commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

Upstream inline review comment by coderabbitai[bot] at 2026-05-13T13:16:23Z; path: docs/CONFIGURATION.md, line: 460. Original comment

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Исправьте непарную кавычку.

В строке /C"runMcp..." отсутствует закрывающая двойная кавычка после .... Должно быть /C"runMcp..." с парными кавычками.

📝 Предлагаемое исправление
-`launch mcp` передаёт `port` как `mcpPort` внутри `/C"runMcp..."`
+`launch mcp` передаёт `port` как `mcpPort` внутри `/C"runMcp..."`
🧰 Tools
🪛 LanguageTool

[typographical] ~460-~460: Непарный символ: «"» скорей всего пропущен
Context: ...pпередаётportкакmcpPortвнутри/C"runMcp..."если CLI не указал--mcp-port` и выбр...

(RU_UNPAIRED_BRACKETS)

🤖 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 `@​docs/CONFIGURATION.md` at line 460, В строке документации с фрагментом
`/C"runMcp...` есть непарная двойная кавычка; исправьте эту последовательность
на корректную пару кавычек (например `/C"runMcp..."`) в описании команды `launch
mcp` где упоминается передача `port` как `mcpPort`, чтобы закрывающая кавычка
после `...` была добавлена и синтаксис строки стал корректным.

✅ Addressed in commits eb61928 to aa3e45a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

upstream-migrated Migrated from the historical upstream tracker

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants