fix(mcp): keep Playwright MCP handshake nonblocking#321
Conversation
📝 WalkthroughSummary by CodeRabbitЗаметки о выпуске
WalkthroughОбновления MCP Playwright фокусируются на надёжности: добавлен вложенный browser runtime с проверками готовности CDP и явным отключением при ошибках; динамическая генерация конфигурации для Gemini и Grok; смена дефолтного CDP endpoint на localhost; guard/timeout в wrapper‑скриптах; и расширение тестов/адаптеров. ChangesMCP Playwright reliability and nested runtime
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@packages/app/src/lib/core/templates-entrypoint/gemini.ts`:
- Around line 200-233: The shell snippet generated by
renderGeminiMcpPlaywrightConfig uses bare $MCP_PLAYWRIGHT_ENABLE which skips
local safe defaults; update the embedded heredoc so the environment check uses
${MCP_PLAYWRIGHT_ENABLE:-0} (e.g., in the assignment and the if that checks
MCP_PLAYWRIGHT_ENABLE) inside the docker_git_sync_gemini_playwright_mcp block in
renderGeminiMcpPlaywrightConfig, and apply the same ${MCP_PLAYWRIGHT_ENABLE:-0}
pattern to the equivalent syncing functions in grok.ts and claude.ts to maintain
consistent safe defaults.
In `@packages/app/src/lib/core/templates-entrypoint/grok.ts`:
- Around line 185-218: The bootstrap script generated by
renderGrokMcpPlaywrightConfig embeds MCP_PLAYWRIGHT_ENABLE without a default
which can break under nounset; change the environment assignment in the
docker_git_sync_grok_playwright_mcp function to provide a default (use the same
pattern as elsewhere, e.g. "${MCP_PLAYWRIGHT_ENABLE:-0}") so
MCP_PLAYWRIGHT_ENABLE is defined when renderGrokMcpPlaywrightConfig is invoked;
update the string that sets GROK_CONFIG_SETTINGS_FILE and MCP_PLAYWRIGHT_ENABLE
in renderGrokMcpPlaywrightConfig accordingly.
In `@packages/app/src/lib/core/templates/dockerfile.ts`:
- Around line 177-199: The template relies on a CLI contract from
`@playwright/mcp` and should pin that package version: add a Docker ARG like
PLAYWRIGHT_MCP_VERSION with default "0.0.75" and use it wherever the image
installs or references playwright-mcp so the binary/flags (used by the guarded
branch invoking exec playwright-mcp --cdp-endpoint ...) remain stable; update
the template's installation step to install
`@playwright/mcp`@${PLAYWRIGHT_MCP_VERSION} (or otherwise reference the ARG) and
mirror the exact same ARG + usage change in the sibling template (the other
dockerfile template in packages/lib/src/core/templates/dockerfile.ts) so
MCP_PLAYWRIGHT_CDP_GUARD behavior remains deterministic across templates.
In `@packages/app/src/lib/core/templates/playwright-browser-runtime.ts`:
- Around line 47-53: Validate MCP_PLAYWRIGHT_READY_ATTEMPTS before using it with
seq: ensure the local variable attempts (derived from
MCP_PLAYWRIGHT_READY_ATTEMPTS) is a positive integer and if not, reset it to the
default (60); then replace the fragile for attempt in $(seq 1 "$attempts") with
a safe numeric loop (e.g. attempt=1; while [ "$attempt" -le "$attempts" ]; do
...; attempt=$((attempt+1)); done) so docker_git_fetch_playwright_cdp_version
and the subsequent retry logic never crash when the env var is non-numeric.
In `@packages/app/tests/docker-git/core-templates.test.ts`:
- Line 5: В тестах удалите прямой импорт renderDockerfile из legacy/internal
surface и вместо этого проверяйте Dockerfile через уже сгенерированные артефакты
(вызов planFiles(...) или через разрешённый app-адаптер): найдите места где
используется renderDockerfile, уберите импорт и вызовы к renderDockerfile,
вызовите planFiles(...) для получения сгенерированных файлов и делайте ассерты
по содержимому файла Dockerfile из возвращённого набора; при необходимости
адаптируйте ожидания к формату артефакта, вместо прямого рендера внутренней
функции.
In `@packages/lib/src/core/templates-entrypoint/gemini.ts`:
- Around line 199-232: The MCP_PLAYWRIGHT_ENABLE env var is expanded without a
safe default in the docker_git_sync_gemini_playwright_mcp shell wrapper produced
by renderGeminiMcpPlaywrightConfig; update the expansion to provide an explicit
default (e.g. use ${MCP_PLAYWRIGHT_ENABLE:-0}) so MCP_PLAYWRIGHT_ENABLE is "0"
when unset, ensuring the Node check (process.env.MCP_PLAYWRIGHT_ENABLE === "1")
behaves predictably and only enables the playwright entry when the flag is
explicitly set.
In `@packages/lib/src/core/templates/playwright-browser-runtime.ts`:
- Around line 46-52: Перед входом в цикл (где используется seq 1 "$attempts" и
sleep "$delay") добавьте валидацию переменных MCP_PLAYWRIGHT_READY_ATTEMPTS
(attempts) и MCP_PLAYWRIGHT_READY_DELAY (delay) при включённом set -euo
pipefail: проверьте соответствие целочисленному регулярному выражению и диапазон
(например attempts > 0 и delay >= 0), и в случае невалидного значения выведите
понятную ошибку и завершите скрипт с ненулевым кодом; разместите эту проверку до
использования seq и sleep (в той же функции/блоке, где объявлены local attempts
и local delay и где вызывается docker_git_fetch_playwright_cdp_version), чтобы
избежать аварийного выхода командой seq или sleep.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0f4b1d2a-7b4b-48cc-b471-e0075a69b631
⛔ Files ignored due to path filters (2)
.github/pr-screenshots/issue-319/implementation.pngis excluded by!**/*.png.github/pr-screenshots/issue-319/verification.pngis excluded by!**/*.png
📒 Files selected for processing (18)
packages/app/src/docker-git/cli/usage.tspackages/app/src/lib/core/templates-entrypoint.tspackages/app/src/lib/core/templates-entrypoint/codex.tspackages/app/src/lib/core/templates-entrypoint/gemini.tspackages/app/src/lib/core/templates-entrypoint/grok.tspackages/app/src/lib/core/templates-entrypoint/playwright-browser.tspackages/app/src/lib/core/templates/dockerfile.tspackages/app/src/lib/core/templates/playwright-browser-runtime.tspackages/app/src/lib/usecases/mcp-playwright.tspackages/app/tests/docker-git/core-templates.test.tspackages/lib/src/core/templates-entrypoint.tspackages/lib/src/core/templates-entrypoint/codex.tspackages/lib/src/core/templates-entrypoint/gemini.tspackages/lib/src/core/templates-entrypoint/grok.tspackages/lib/src/core/templates/dockerfile.tspackages/lib/src/core/templates/playwright-browser-runtime.tspackages/lib/tests/core/templates.test.tspackages/lib/tests/usecases/mcp-playwright.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: E2E (OpenCode)
- GitHub Check: E2E (Browser command)
- GitHub Check: E2E (Runtime volumes + SSH)
- GitHub Check: E2E (Login context)
- GitHub Check: E2E (Clone auto-open SSH)
- GitHub Check: E2E (Clone cache)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Implement Functional Core, Imperative Shell (FCIS) pattern: CORE layer contains only pure functions with immutable data and mathematical operations; SHELL layer isolates all effects (IO, network, database). Strict dependency direction: SHELL → CORE (never reverse).
Never useany,unknown,eslint-disable,ts-ignore, orastype assertions (except in rigorously justified cases with documentation). Always use exhaustive union type analysis through.exhaustive()pattern matching.
All external dependencies must be wrapped through typed interfaces and injected via Effect-TS Layer pattern. Never call external services directly from CORE functions.
Use monadic composition with Effect-TS for all effects:Effect<Success, Error, Requirements>. Compose effects throughpipe()andEffect.flatMap(). Implement dependency injection via Layer pattern. Handle errors without try/catch blocks.
All functions must be pure in the CORE layer: no side effects (logging, console output, IO operations, mutations). Separate all side effects into the SHELL layer.
Use exhaustive pattern matching with Effect.Match instead of switch statements. Example:Match.value(item).pipe(Match.when(...), Match.exhaustive).
Document all functions with comprehensive TSDoc including:@pure(true/false),@effect(required services),@invariant(mathematical invariants),@precondition,@postcondition,@complexity(time and space),@throwsNever (errors must be typed in Effect).
Use functional comment markers for code clarity: CHANGE (brief description), WHY (mathematical/architectural justification), QUOTE(ТЗ) (requirement citation), REF (RTM or message ID), SOURCE (external source with quote), FORMAT THEOREM (∀x ∈ Domain: P(x) → Q(f(x))), PURITY (CORE|SHELL), EFFECT (Effect type signature), INVARIANT (mathematical invariant), COMPLEXITY (time/space).
Define all external service dependencies as Context.Tag classes with fully typed methods returning Effect types. Example: `class Da...
Files:
packages/app/src/lib/core/templates-entrypoint/playwright-browser.tspackages/app/tests/docker-git/core-templates.test.tspackages/app/src/lib/core/templates-entrypoint/codex.tspackages/app/src/lib/usecases/mcp-playwright.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/app/src/docker-git/cli/usage.tspackages/lib/src/core/templates-entrypoint/codex.tspackages/lib/src/core/templates/playwright-browser-runtime.tspackages/app/src/lib/core/templates-entrypoint.tspackages/app/src/lib/core/templates-entrypoint/grok.tspackages/lib/src/core/templates-entrypoint/grok.tspackages/lib/src/core/templates-entrypoint.tspackages/app/src/lib/core/templates/playwright-browser-runtime.tspackages/lib/tests/core/templates.test.tspackages/lib/src/core/templates-entrypoint/gemini.tspackages/app/src/lib/core/templates-entrypoint/gemini.tspackages/lib/src/core/templates/dockerfile.tspackages/app/src/lib/core/templates/dockerfile.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Forbidden constructs in CORE code:any,eslint-disable,ts-ignore,async/await, raw Promise chains (then/catch),Promise.all,try/catchfor logic control,console.*, switch statements (use Match with .exhaustive() instead)
All functions must use Effect-TS for composing effects:Effect<Success, Error, Requirements>. No direct async/await, Promise chains, or try/catch in product logic.
Functional comments must include: CHANGE, WHY, QUOTE(ТЗ) or n/a, REF, SOURCE or n/a, FORMAT THEOREM, PURITY (CORE|SHELL), EFFECT signature for SHELL functions, INVARIANT, and COMPLEXITY.
All data mutations must use immutable patterns (ReadonlyArray, readonly properties, Object.freeze); mutation in SHELL only when absolutely necessary and documented.
Files:
packages/app/src/lib/core/templates-entrypoint/playwright-browser.tspackages/app/tests/docker-git/core-templates.test.tspackages/app/src/lib/core/templates-entrypoint/codex.tspackages/app/src/lib/usecases/mcp-playwright.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/app/src/docker-git/cli/usage.tspackages/lib/src/core/templates-entrypoint/codex.tspackages/lib/src/core/templates/playwright-browser-runtime.tspackages/app/src/lib/core/templates-entrypoint.tspackages/app/src/lib/core/templates-entrypoint/grok.tspackages/lib/src/core/templates-entrypoint/grok.tspackages/lib/src/core/templates-entrypoint.tspackages/app/src/lib/core/templates/playwright-browser-runtime.tspackages/lib/tests/core/templates.test.tspackages/lib/src/core/templates-entrypoint/gemini.tspackages/app/src/lib/core/templates-entrypoint/gemini.tspackages/lib/src/core/templates/dockerfile.tspackages/app/src/lib/core/templates/dockerfile.ts
**/*.{sh,bash,py,js,ts,jsx,tsx,go,java,rb,php}
📄 CodeRabbit inference engine (Custom checks)
Fail if changed files introduce command injection or unsafe shell/process execution with user-controlled input
Files:
packages/app/src/lib/core/templates-entrypoint/playwright-browser.tspackages/app/tests/docker-git/core-templates.test.tspackages/app/src/lib/core/templates-entrypoint/codex.tspackages/app/src/lib/usecases/mcp-playwright.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/app/src/docker-git/cli/usage.tspackages/lib/src/core/templates-entrypoint/codex.tspackages/lib/src/core/templates/playwright-browser-runtime.tspackages/app/src/lib/core/templates-entrypoint.tspackages/app/src/lib/core/templates-entrypoint/grok.tspackages/lib/src/core/templates-entrypoint/grok.tspackages/lib/src/core/templates-entrypoint.tspackages/app/src/lib/core/templates/playwright-browser-runtime.tspackages/lib/tests/core/templates.test.tspackages/lib/src/core/templates-entrypoint/gemini.tspackages/app/src/lib/core/templates-entrypoint/gemini.tspackages/lib/src/core/templates/dockerfile.tspackages/app/src/lib/core/templates/dockerfile.ts
**/*.{py,js,ts,jsx,tsx,go,java,rb,php,sh,bash,c,cpp}
📄 CodeRabbit inference engine (Custom checks)
Fail if changed files introduce path traversal or writes outside intended project/container state directories
Files:
packages/app/src/lib/core/templates-entrypoint/playwright-browser.tspackages/app/tests/docker-git/core-templates.test.tspackages/app/src/lib/core/templates-entrypoint/codex.tspackages/app/src/lib/usecases/mcp-playwright.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/app/src/docker-git/cli/usage.tspackages/lib/src/core/templates-entrypoint/codex.tspackages/lib/src/core/templates/playwright-browser-runtime.tspackages/app/src/lib/core/templates-entrypoint.tspackages/app/src/lib/core/templates-entrypoint/grok.tspackages/lib/src/core/templates-entrypoint/grok.tspackages/lib/src/core/templates-entrypoint.tspackages/app/src/lib/core/templates/playwright-browser-runtime.tspackages/lib/tests/core/templates.test.tspackages/lib/src/core/templates-entrypoint/gemini.tspackages/app/src/lib/core/templates-entrypoint/gemini.tspackages/lib/src/core/templates/dockerfile.tspackages/app/src/lib/core/templates/dockerfile.ts
**/*.{js,ts,jsx,tsx,py,java,go,rb,php,sh,bash,yml,yaml,json,env*,toml,cfg,config,dockerfile,dockerignore}
📄 CodeRabbit inference engine (Custom checks)
Fail if changed files expose credentials, tokens, private-keys, or PII in source, generated config, logs, or CI output
Files:
packages/app/src/lib/core/templates-entrypoint/playwright-browser.tspackages/app/tests/docker-git/core-templates.test.tspackages/app/src/lib/core/templates-entrypoint/codex.tspackages/app/src/lib/usecases/mcp-playwright.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/app/src/docker-git/cli/usage.tspackages/lib/src/core/templates-entrypoint/codex.tspackages/lib/src/core/templates/playwright-browser-runtime.tspackages/app/src/lib/core/templates-entrypoint.tspackages/app/src/lib/core/templates-entrypoint/grok.tspackages/lib/src/core/templates-entrypoint/grok.tspackages/lib/src/core/templates-entrypoint.tspackages/app/src/lib/core/templates/playwright-browser-runtime.tspackages/lib/tests/core/templates.test.tspackages/lib/src/core/templates-entrypoint/gemini.tspackages/app/src/lib/core/templates-entrypoint/gemini.tspackages/lib/src/core/templates/dockerfile.tspackages/app/src/lib/core/templates/dockerfile.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Ты строгий ревьюер SPEC DRIVEN DEVELOPMENT.Перед выводами изучи README.md, другие *.md файлы, linked issues,
PR description, PR comments/discussion и релевантную кодовую базу.Сверь изменения с исходным ТЗ/спекой и обсуждением. Флагай любой уход
от спеки, недокументированное изменение поведения, отсутствие тестов
для заявленного поведения и security-риск. Если спека не видна,
попроси автора добавить ее в issue или PR description.Проверь решение с точки зрения формальной верификации: какие инварианты,
предусловия и постусловия можно доказать математически, а где доказуемость
слабая. Оцени решение с точки зрения теории игр: устойчивы ли стимулы,
нет ли выгодного обхода правил, и какое решение было бы сильнее.
Files:
packages/app/src/lib/core/templates-entrypoint/playwright-browser.tspackages/app/tests/docker-git/core-templates.test.tspackages/app/src/lib/core/templates-entrypoint/codex.tspackages/app/src/lib/usecases/mcp-playwright.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/app/src/docker-git/cli/usage.tspackages/lib/src/core/templates-entrypoint/codex.tspackages/lib/src/core/templates/playwright-browser-runtime.tspackages/app/src/lib/core/templates-entrypoint.tspackages/app/src/lib/core/templates-entrypoint/grok.tspackages/lib/src/core/templates-entrypoint/grok.tspackages/lib/src/core/templates-entrypoint.tspackages/app/src/lib/core/templates/playwright-browser-runtime.tspackages/lib/tests/core/templates.test.tspackages/lib/src/core/templates-entrypoint/gemini.tspackages/app/src/lib/core/templates-entrypoint/gemini.tspackages/lib/src/core/templates/dockerfile.tspackages/app/src/lib/core/templates/dockerfile.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Implement property-based testing using fast-check for mathematical properties and invariants. Example:fc.property(fc.array(messageArbitrary), (messages) => isChronologicallySorted(sortMessagesByTimestamp(messages))).
Mock external dependencies in unit tests using Effect's testing utilities. Run tests without Effect runtime for speed. Example:Effect.provide(MockService), Effect.runPromise.
Files:
packages/app/tests/docker-git/core-templates.test.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/lib/tests/core/templates.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Property-based tests (fast-check) must verify mathematical invariants; unit tests must use Effect test utilities without async/await.
Files:
packages/app/tests/docker-git/core-templates.test.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/lib/tests/core/templates.test.ts
🪛 GitHub Check: Lint Effect-TS
packages/app/tests/docker-git/core-templates.test.ts
[failure] 5-5:
Direct import or require '../../src/lib/core/templates/dockerfile.js' from legacy lib surfaces is forbidden in package/app frontend surfaces and tests. Use the API client or a local app adapter instead
🔇 Additional comments (10)
packages/app/src/lib/usecases/mcp-playwright.ts (1)
31-33: Формулировка WHY обновлена корректно и соответствует целям PR.Изменение носит документирующий характер, с текущей логикой и инвариантами согласовано.
packages/lib/src/core/templates/dockerfile.ts (1)
177-199: Дубликат замечания про дрейф версии@playwright/mcp.Здесь нужна та же фиксация версии, что и в
packages/app/src/lib/core/templates/dockerfile.ts, иначеappиlibснова разъедутся.packages/app/src/docker-git/cli/usage.ts (1)
105-110: LGTM!packages/app/src/lib/core/templates-entrypoint/codex.ts (1)
101-103: LGTM!packages/lib/src/core/templates-entrypoint/codex.ts (1)
100-102: LGTM!packages/app/src/lib/core/templates/dockerfile.ts (1)
238-242: LGTM!Also applies to: 402-402
packages/lib/src/core/templates/playwright-browser-runtime.ts (1)
42-42: ⚡ Quick winHost заголовок согласован с архитектурой порт-форвардинга и хост-валидации.
На строке 42 Host header указывает на
127.0.0.1:9222— это корректно. Архитектура использует socat прокси (строка 260 в playwright.ts:socat TCP-LISTEN:9223,fork,reuseaddr TCP:127.0.0.1:9222), который слушает на порту 9223 и форвардит запросы на 9222. Сама система (kechangdev/browser-vnc) выполняет хост-валидацию и требует, чтобы Host header совпадал с реальным upstream сервисом (9222), а не с портом прокси (9223). Удаление Host header нарушит валидацию на upstream и приведёт к сбою readiness-проверки. Текущая реализация согласована с dockerfile.ts (строка 205) и project-browser.ts (строка 95).> Likely an incorrect or invalid review comment.packages/app/src/lib/core/templates/playwright-browser-runtime.ts (1)
43-43: ⚡ Quick winКонфликт портов в Host-заголовке разрешён архитектурой проксирования — это не баг.
На Line 43 используется
Host: 127.0.0.1:9222намеренно, несмотря на дефолтный endpoint127.0.0.1:9223на Line 37. Это не создаёт риск ложного fail:
- 127.0.0.1:9223 — слой CDP guard или socat (внешний интерфейс)
- 127.0.0.1:9222 — фактический сервис kechangdev/browser-vnc, который выполняет проверку Host-заголовка (см. dockerfile.ts:202)
Архитектура: клиент → 9223 (guard/socat) → 9222 (browser-vnc). Сервис browser-vnc требует Host-заголовок, указывающий на его фактический порт (9222), а не на прокси-интерфейс. Удаление заголовка нарушит проверку хоста на стороне browser-vnc.
> Likely an incorrect or invalid review comment.packages/app/src/lib/core/templates-entrypoint/playwright-browser.ts (1)
24-24: ⚡ Quick winПеременная
MCP_PLAYWRIGHT_ENABLEвсегда инициализируется в заголовке entrypoint, поэтому риск unbound variable отсутствует.Функция
renderEntrypointHeader(изbase.ts:42) выполняется первой и устанавливает:MCP_PLAYWRIGHT_ENABLE="${MCP_PLAYWRIGHT_ENABLE:-${config.enableMcpPlaywright ? "1" : "0"}}"Это гарантирует, что переменная инициализируется с значением "0" или "1" до того, как будет использована в
renderEntrypointPlaywrightBrowserRuntime(и других renderers: claude, gemini, grok, codex). Порядок композиции вrenderEntrypointподтверждает, что header рендеруется первым, поэтомуset -euo pipefailне вызовет ошибку unbound variable.Предлагаемый фикс (
${MCP_PLAYWRIGHT_ENABLE:-0}) был бы избыточным, так как переменная уже инициализирована выше по потоку выполнения скрипта.> Likely an incorrect or invalid review comment.packages/lib/src/core/templates-entrypoint/grok.ts (1)
184-217: ⚡ Quick winНет проблемы; MCP_PLAYWRIGHT_ENABLE гарантирует инициализацию в renderEntrypointHeader перед использованием в grok.ts.
renderEntrypointHeader(config)выполняется первым в массиве композиции (templates-entrypoint.ts) и устанавливает:MCP_PLAYWRIGHT_ENABLE="${MCP_PLAYWRIGHT_ENABLE:-${config.enableMcpPlaywright ? "1" : "0"}}"К моменту вызова
renderEntrypointGrokConfig(config)переменная уже инициализирована с дефолтом. Даже подset -u(nounset) ошибки не будет. Тесты подтверждают, что текущий код без дефолта на строке 187 — это правильное поведение.> Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/lib/src/core/templates/dockerfile.ts (1)
203-235: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueРассмотрите валидацию
MCP_PLAYWRIGHT_RETRY_ATTEMPTSв non-guarded пути для консистентности.В
playwright-browser-runtime.tsпеременныеattemptsиdelayвалидируются регулярным выражением^[0-9]+$перед использованием в цикле. В non-guarded ветке этого wrapper-скрипта (строка 210)MCP_PLAYWRIGHT_RETRY_ATTEMPTSпередаётся вseqбез проверки.Риск низкий: guard включён по умолчанию, и этот путь используется редко. Однако для консистентности с паттерном в
docker_git_wait_for_playwright_cdpможно добавить аналогичную валидацию.🤖 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 `@packages/lib/src/core/templates/dockerfile.ts` around lines 203 - 235, The non-guarded path calls seq with MCP_PLAYWRIGHT_RETRY_ATTEMPTS without validating it's a positive integer; mirror the validation used in docker_git_wait_for_playwright_cdp/playwright-browser-runtime by validating MCP_PLAYWRIGHT_RETRY_ATTEMPTS (and optionally MCP_PLAYWRIGHT_RETRY_DELAY) with a regex like ^[0-9]+$ before using it in the seq loop inside fetch_cdp_version/for-attempt loop, and if invalid, set a safe default or exit with a clear error so seq can't receive malformed input.
🤖 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 `@packages/lib/src/core/templates/dockerfile.ts`:
- Around line 203-235: The non-guarded path calls seq with
MCP_PLAYWRIGHT_RETRY_ATTEMPTS without validating it's a positive integer; mirror
the validation used in
docker_git_wait_for_playwright_cdp/playwright-browser-runtime by validating
MCP_PLAYWRIGHT_RETRY_ATTEMPTS (and optionally MCP_PLAYWRIGHT_RETRY_DELAY) with a
regex like ^[0-9]+$ before using it in the seq loop inside
fetch_cdp_version/for-attempt loop, and if invalid, set a safe default or exit
with a clear error so seq can't receive malformed input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 30035ba4-0548-48e8-be0e-4ffbc9716c6a
📒 Files selected for processing (14)
packages/app/src/lib/core/templates-entrypoint/claude.tspackages/app/src/lib/core/templates-entrypoint/gemini.tspackages/app/src/lib/core/templates-entrypoint/grok.tspackages/app/src/lib/core/templates/dockerfile.tspackages/app/src/lib/core/templates/playwright-browser-runtime.tspackages/app/test-adapters/core-templates.tspackages/app/tests/docker-git/core-templates.test.tspackages/lib/src/core/templates-entrypoint/claude.tspackages/lib/src/core/templates-entrypoint/gemini.tspackages/lib/src/core/templates-entrypoint/grok.tspackages/lib/src/core/templates/dockerfile.tspackages/lib/src/core/templates/playwright-browser-runtime.tspackages/lib/tests/core/templates.test.tspackages/lib/tests/usecases/mcp-playwright.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: E2E (OpenCode)
- GitHub Check: E2E (Login context)
- GitHub Check: E2E (Clone cache)
- GitHub Check: E2E (Clone auto-open SSH)
- GitHub Check: E2E (Runtime volumes + SSH)
- GitHub Check: E2E (Browser command)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Implement Functional Core, Imperative Shell (FCIS) pattern: CORE layer contains only pure functions with immutable data and mathematical operations; SHELL layer isolates all effects (IO, network, database). Strict dependency direction: SHELL → CORE (never reverse).
Never useany,unknown,eslint-disable,ts-ignore, orastype assertions (except in rigorously justified cases with documentation). Always use exhaustive union type analysis through.exhaustive()pattern matching.
All external dependencies must be wrapped through typed interfaces and injected via Effect-TS Layer pattern. Never call external services directly from CORE functions.
Use monadic composition with Effect-TS for all effects:Effect<Success, Error, Requirements>. Compose effects throughpipe()andEffect.flatMap(). Implement dependency injection via Layer pattern. Handle errors without try/catch blocks.
All functions must be pure in the CORE layer: no side effects (logging, console output, IO operations, mutations). Separate all side effects into the SHELL layer.
Use exhaustive pattern matching with Effect.Match instead of switch statements. Example:Match.value(item).pipe(Match.when(...), Match.exhaustive).
Document all functions with comprehensive TSDoc including:@pure(true/false),@effect(required services),@invariant(mathematical invariants),@precondition,@postcondition,@complexity(time and space),@throwsNever (errors must be typed in Effect).
Use functional comment markers for code clarity: CHANGE (brief description), WHY (mathematical/architectural justification), QUOTE(ТЗ) (requirement citation), REF (RTM or message ID), SOURCE (external source with quote), FORMAT THEOREM (∀x ∈ Domain: P(x) → Q(f(x))), PURITY (CORE|SHELL), EFFECT (Effect type signature), INVARIANT (mathematical invariant), COMPLEXITY (time/space).
Define all external service dependencies as Context.Tag classes with fully typed methods returning Effect types. Example: `class Da...
Files:
packages/app/test-adapters/core-templates.tspackages/app/src/lib/core/templates-entrypoint/claude.tspackages/lib/src/core/templates-entrypoint/claude.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/app/src/lib/core/templates-entrypoint/grok.tspackages/app/src/lib/core/templates/playwright-browser-runtime.tspackages/app/tests/docker-git/core-templates.test.tspackages/lib/tests/core/templates.test.tspackages/app/src/lib/core/templates-entrypoint/gemini.tspackages/lib/src/core/templates/playwright-browser-runtime.tspackages/lib/src/core/templates-entrypoint/grok.tspackages/lib/src/core/templates/dockerfile.tspackages/lib/src/core/templates-entrypoint/gemini.tspackages/app/src/lib/core/templates/dockerfile.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Forbidden constructs in CORE code:any,eslint-disable,ts-ignore,async/await, raw Promise chains (then/catch),Promise.all,try/catchfor logic control,console.*, switch statements (use Match with .exhaustive() instead)
All functions must use Effect-TS for composing effects:Effect<Success, Error, Requirements>. No direct async/await, Promise chains, or try/catch in product logic.
Functional comments must include: CHANGE, WHY, QUOTE(ТЗ) or n/a, REF, SOURCE or n/a, FORMAT THEOREM, PURITY (CORE|SHELL), EFFECT signature for SHELL functions, INVARIANT, and COMPLEXITY.
All data mutations must use immutable patterns (ReadonlyArray, readonly properties, Object.freeze); mutation in SHELL only when absolutely necessary and documented.
Files:
packages/app/test-adapters/core-templates.tspackages/app/src/lib/core/templates-entrypoint/claude.tspackages/lib/src/core/templates-entrypoint/claude.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/app/src/lib/core/templates-entrypoint/grok.tspackages/app/src/lib/core/templates/playwright-browser-runtime.tspackages/app/tests/docker-git/core-templates.test.tspackages/lib/tests/core/templates.test.tspackages/app/src/lib/core/templates-entrypoint/gemini.tspackages/lib/src/core/templates/playwright-browser-runtime.tspackages/lib/src/core/templates-entrypoint/grok.tspackages/lib/src/core/templates/dockerfile.tspackages/lib/src/core/templates-entrypoint/gemini.tspackages/app/src/lib/core/templates/dockerfile.ts
**/*.{sh,bash,py,js,ts,jsx,tsx,go,java,rb,php}
📄 CodeRabbit inference engine (Custom checks)
Fail if changed files introduce command injection or unsafe shell/process execution with user-controlled input
Files:
packages/app/test-adapters/core-templates.tspackages/app/src/lib/core/templates-entrypoint/claude.tspackages/lib/src/core/templates-entrypoint/claude.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/app/src/lib/core/templates-entrypoint/grok.tspackages/app/src/lib/core/templates/playwright-browser-runtime.tspackages/app/tests/docker-git/core-templates.test.tspackages/lib/tests/core/templates.test.tspackages/app/src/lib/core/templates-entrypoint/gemini.tspackages/lib/src/core/templates/playwright-browser-runtime.tspackages/lib/src/core/templates-entrypoint/grok.tspackages/lib/src/core/templates/dockerfile.tspackages/lib/src/core/templates-entrypoint/gemini.tspackages/app/src/lib/core/templates/dockerfile.ts
**/*.{py,js,ts,jsx,tsx,go,java,rb,php,sh,bash,c,cpp}
📄 CodeRabbit inference engine (Custom checks)
Fail if changed files introduce path traversal or writes outside intended project/container state directories
Files:
packages/app/test-adapters/core-templates.tspackages/app/src/lib/core/templates-entrypoint/claude.tspackages/lib/src/core/templates-entrypoint/claude.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/app/src/lib/core/templates-entrypoint/grok.tspackages/app/src/lib/core/templates/playwright-browser-runtime.tspackages/app/tests/docker-git/core-templates.test.tspackages/lib/tests/core/templates.test.tspackages/app/src/lib/core/templates-entrypoint/gemini.tspackages/lib/src/core/templates/playwright-browser-runtime.tspackages/lib/src/core/templates-entrypoint/grok.tspackages/lib/src/core/templates/dockerfile.tspackages/lib/src/core/templates-entrypoint/gemini.tspackages/app/src/lib/core/templates/dockerfile.ts
**/*.{js,ts,jsx,tsx,py,java,go,rb,php,sh,bash,yml,yaml,json,env*,toml,cfg,config,dockerfile,dockerignore}
📄 CodeRabbit inference engine (Custom checks)
Fail if changed files expose credentials, tokens, private-keys, or PII in source, generated config, logs, or CI output
Files:
packages/app/test-adapters/core-templates.tspackages/app/src/lib/core/templates-entrypoint/claude.tspackages/lib/src/core/templates-entrypoint/claude.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/app/src/lib/core/templates-entrypoint/grok.tspackages/app/src/lib/core/templates/playwright-browser-runtime.tspackages/app/tests/docker-git/core-templates.test.tspackages/lib/tests/core/templates.test.tspackages/app/src/lib/core/templates-entrypoint/gemini.tspackages/lib/src/core/templates/playwright-browser-runtime.tspackages/lib/src/core/templates-entrypoint/grok.tspackages/lib/src/core/templates/dockerfile.tspackages/lib/src/core/templates-entrypoint/gemini.tspackages/app/src/lib/core/templates/dockerfile.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Ты строгий ревьюер SPEC DRIVEN DEVELOPMENT.Перед выводами изучи README.md, другие *.md файлы, linked issues,
PR description, PR comments/discussion и релевантную кодовую базу.Сверь изменения с исходным ТЗ/спекой и обсуждением. Флагай любой уход
от спеки, недокументированное изменение поведения, отсутствие тестов
для заявленного поведения и security-риск. Если спека не видна,
попроси автора добавить ее в issue или PR description.Проверь решение с точки зрения формальной верификации: какие инварианты,
предусловия и постусловия можно доказать математически, а где доказуемость
слабая. Оцени решение с точки зрения теории игр: устойчивы ли стимулы,
нет ли выгодного обхода правил, и какое решение было бы сильнее.
Files:
packages/app/test-adapters/core-templates.tspackages/app/src/lib/core/templates-entrypoint/claude.tspackages/lib/src/core/templates-entrypoint/claude.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/app/src/lib/core/templates-entrypoint/grok.tspackages/app/src/lib/core/templates/playwright-browser-runtime.tspackages/app/tests/docker-git/core-templates.test.tspackages/lib/tests/core/templates.test.tspackages/app/src/lib/core/templates-entrypoint/gemini.tspackages/lib/src/core/templates/playwright-browser-runtime.tspackages/lib/src/core/templates-entrypoint/grok.tspackages/lib/src/core/templates/dockerfile.tspackages/lib/src/core/templates-entrypoint/gemini.tspackages/app/src/lib/core/templates/dockerfile.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Implement property-based testing using fast-check for mathematical properties and invariants. Example:fc.property(fc.array(messageArbitrary), (messages) => isChronologicallySorted(sortMessagesByTimestamp(messages))).
Mock external dependencies in unit tests using Effect's testing utilities. Run tests without Effect runtime for speed. Example:Effect.provide(MockService), Effect.runPromise.
Files:
packages/lib/tests/usecases/mcp-playwright.test.tspackages/app/tests/docker-git/core-templates.test.tspackages/lib/tests/core/templates.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Property-based tests (fast-check) must verify mathematical invariants; unit tests must use Effect test utilities without async/await.
Files:
packages/lib/tests/usecases/mcp-playwright.test.tspackages/app/tests/docker-git/core-templates.test.tspackages/lib/tests/core/templates.test.ts
🔇 Additional comments (15)
packages/app/test-adapters/core-templates.ts (1)
1-2: LGTM!packages/app/src/lib/core/templates-entrypoint/claude.ts (1)
196-196: LGTM!packages/lib/src/core/templates-entrypoint/claude.ts (1)
195-195: LGTM!packages/lib/tests/usecases/mcp-playwright.test.ts (1)
128-144: LGTM!Also applies to: 175-182
packages/app/src/lib/core/templates-entrypoint/grok.ts (1)
154-155: LGTM!Also applies to: 185-219, 339-339
packages/app/src/lib/core/templates/playwright-browser-runtime.ts (1)
30-75: LGTM!Also applies to: 133-134, 137-138, 141-142, 150-151, 163-164, 195-201
packages/app/tests/docker-git/core-templates.test.ts (1)
3-4: LGTM!Also applies to: 28-43, 47-67
packages/lib/tests/core/templates.test.ts (1)
233-245: LGTM!Also applies to: 452-455, 741-749, 766-777
packages/app/src/lib/core/templates-entrypoint/gemini.ts (1)
200-233: LGTM!Also applies to: 337-337
packages/lib/src/core/templates/playwright-browser-runtime.ts (2)
29-74: LGTM!
132-200: LGTM!packages/lib/src/core/templates-entrypoint/grok.ts (1)
153-154: LGTM!Also applies to: 184-218, 338-338
packages/lib/src/core/templates/dockerfile.ts (1)
156-201: LGTM!packages/lib/src/core/templates-entrypoint/gemini.ts (1)
199-232: LGTM!Also applies to: 336-336
packages/app/src/lib/core/templates/dockerfile.ts (1)
156-201: LGTM!Also applies to: 239-244, 403-403
1 participant
Summary
Closes #319.
docker-git-playwright-mcpstart Playwright MCP stdio immediately in guarded mode instead of blockinginitializeon CDP readiness.MCP_PLAYWRIGHT_ENABLE=0when the runtime cannot start or CDP never becomes ready.Screenshots / Proof
Verification
Implementation Invariants
Validation
bun --cwd packages/lib vitest run tests/core/templates.test.ts tests/usecases/mcp-playwright.test.tsbun run --cwd packages/lib lintbun run --cwd packages/app typecheckbun run --cwd packages/app lintbun run --cwd packages/app lint:effectbun run --cwd packages/app testgit diff --check