feat(browser-connection): new dedicated package for noVNC + browser connection (issue #347)#348
feat(browser-connection): new dedicated package for noVNC + browser connection (issue #347)#348skulidropek wants to merge 1 commit into
Conversation
…onnection (issue #347) - Extracted BrowserConnection Effect Layer, pure helpers, invariants - Supports both MCP and built-in Hermes browser tools out of the box - Single browser session with noVNC/CDP (port 9223) - Follows effect-template + AGENTS.md style (formal comments, types, Layer) Closes #347
📝 WalkthroughSummary by CodeRabbitВыпуск обновлений
WalkthroughДобавлен новый пакет ChangesBrowserConnection — новый пакет с управлением браузером
🎯 2 (Simple) | ⏱️ ~10 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (6 passed)
✨ 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/browser-connection/src/index.ts (1)
1-48:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftНесоответствие между заявлениями в PR description и фактическим кодом.
PR objectives утверждают: "Mathematical guarantees stated: invariant that getCdpUrl(projectId) and getNoVncUrl(projectId) point to the same dg-*-browser container; purity of core functions; no duplication". Однако в коде отсутствуют:
- Документированные инварианты - ни один метод не содержит
@invariantилиINVARIANTкомментариев- Маркеры чистоты - функции
renderNoVncUrl,renderCdpUrl,isSingleBrowserSessionне содержат@pureилиPURITY: COREмаркеров- Формальная спецификация - отсутствуют
FORMAT THEOREMкомментарии с математическими утверждениями вида∀x ∈ Domain: P(x) → Q(f(x))Согласно guidelines: "Ты строгий ревьюер SPEC DRIVEN DEVELOPMENT. Сверь изменения с исходным ТЗ/спекой и обсуждением. Флагай любой уход от спеки, недокументированное изменение поведения".
Заявленные математические гарантии должны быть явно задокументированы в коде через functional comments и TSDoc, чтобы обеспечить формальную верифицируемость.
Согласно coding guidelines: "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" и требование spec-driven development: "Сверь изменения с исходным ТЗ/спекой и обсуждением."
🤖 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/browser-connection/src/index.ts` around lines 1 - 48, The PR claims formal invariants and purity but the code lacks the required functional/TSDoc annotations; add TSDoc-style functional comments for the core API and pure helpers — specifically annotate BrowserConnection methods (startBrowser, getCdpUrl, getNoVncUrl, getVncUrl, parseProxyPath, rewriteCdpUrl) and the pure helper functions renderNoVncUrl, renderCdpUrl, isSingleBrowserSession with the required fields: CHANGE, WHY, QUOTE or n/a, REF, SOURCE or n/a, FORMAT THEOREM expressing the invariant relating getCdpUrl(projectId) and getNoVncUrl(projectId) (e.g. ∀projectId: getCdpUrl(projectId) ↔ getNoVncUrl(projectId) map to same dg-*-browser container), PURITY (mark helpers as PURITY: CORE and shell functions appropriately with EFFECT signature), INVARIANT (explicitly restate the container equivalence), and COMPLEXITY; ensure the theorem references the exact function names (getCdpUrl, getNoVncUrl, renderCdpUrl, renderNoVncUrl, isSingleBrowserSession) so reviewers can verify the formal claims.
🤖 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/browser-connection/src/index.ts`:
- Line 28: getCdpUrl currently returns a different string than the helper
renderCdpUrl which will cause inconsistent CDP endpoints; update the Layer
implementation of getCdpUrl to call/render via the existing renderCdpUrl helper
(or, alternatively, make renderCdpUrl match the literal used by getCdpUrl) so
both functions produce the exact same URL format—locate getCdpUrl and
renderCdpUrl in packages/browser-connection/src/index.ts and replace the literal
in getCdpUrl with a call to renderCdpUrl(projectId) to avoid duplicated string
literals.
- Around line 3-6: Add a comprehensive TSDoc block for the BrowserError class:
document the class purpose, the readonly _tag, and the constructor parameters
(message: string, cause?: unknown) with `@param` entries, include an example
showing usage (e.g., new BrowserError("Failed to start browser",
originalError)), and add required markers such as `@pure`, `@effect` (if any
side-effects/dependencies), `@invariant` (if applicable),
`@precondition/`@postcondition for constructor behavior, and `@complexity` O(1);
attach the doc block immediately above the export class BrowserError declaration
and reference the constructor, _tag, message, and cause symbols in the comments.
- Line 26: Replace the forbidden type assertion "return undefined as void" with
a non-casted return (either remove the return entirely so Effect.gen infers
Effect<void, never>, or use a plain "return" / "return undefined" without "as
void"); locate the exact statement "return undefined as void" in
packages/browser-connection/src/index.ts and remove the "as void" cast so no
type assertion is used.
- Around line 19-35: Add the required functional comments for the
BrowserConnectionLive Layer and each exposed method (BrowserConnectionLive,
startBrowser, getCdpUrl, getNoVncUrl, getVncUrl, parseProxyPath, rewriteCdpUrl):
for every function provide the header including CHANGE, WHY, QUOTE(ТЗ) or n/a,
REF, SOURCE or n/a, FORMAT THEOREM, PURITY (CORE|SHELL) and for SHELL functions
include the EFFECT signature, plus INVARIANT and COMPLEXITY. Place the comment
block immediately above the Layer implementation and above each method
implementation so the formal verification tooling can parse them; ensure the
EFFECT signature matches the returned Effect types (e.g.,
Effect.succeed/Effect.gen) and that each comment references the same function
name to uniquely identify the behavior.
- Around line 8-15: Add comprehensive TSDoc comments to the BrowserConnection
interface methods: startBrowser, getCdpUrl, getNoVncUrl, getVncUrl,
parseProxyPath, and rewriteCdpUrl. For each method document parameters and
return type, mark `@pure` where applicable, add `@effect` dependencies (e.g.,
Effect), include `@invariant` (mathematical contract), `@precondition` (input
requirements), `@postcondition` (output guarantees), and `@complexity` (Big-O), and
ensure tags reflect any thrown BrowserError or never effects; place these
comments directly above the respective method signatures in the
BrowserConnection interface.
- Around line 37-45: Add comprehensive TSDoc for the three pure helper
functions: renderNoVncUrl, renderCdpUrl, and isSingleBrowserSession; for each
function include a short functional description, `@param` and `@returns` tags, a
`@pure` marker set to true (or PURE: CORE in the top-line comment), `@effect` (none
or required services), `@precondition` (e.g. non-empty projectId or valid URL
strings), `@postcondition` (output string contains expected path or query),
`@invariant` (mathematical properties like idempotence or substring containment),
and `@complexity` with time/space O-notation; place the TSDoc immediately above
each function declaration so tools and linters recognize them and ensure the
documented invariants mention the exact behavior of renderNoVncUrl (produces
/b/{projectId}/vnc.html URL), renderCdpUrl (produces localhost 9223 JSON URL
with project query), and isSingleBrowserSession (returns true when cdpUrl
includes "9223" and noVncUrl includes "/vnc.html").
- Around line 44-45: Функция isSingleBrowserSession слишком примитивно проверяет
URL через includes; нужно извлечь и сопоставить реальный projectId/имя
контейнера из cdpUrl и noVncUrl вместо простых подстрок. Измените
isSingleBrowserSession так, чтобы она парсила переданные URL (например
регуляркой) и вытягивала идентификатор dg-*-browser или projectId/namespace из
хоста/пути/параметров для обоих URL, затем возвращала true только если оба
извлечённых projectId/имени контейнера совпадают и порт/маршрут соответствует
ожидаемому (CDP порт 9223 и noVnc путь /vnc.html) — используйте имена
функций/констант getCdpUrl/getNoVncUrl и саму isSingleBrowserSession как точки
внесения правок.
- Line 13: The parseProxyPath method currently returns Effect.Effect<unknown,
never>; change its signature to return a typed effect such as
Effect.Effect<ProxyPathInfo | null, never> (or another concrete discriminated
type you define) so boundary data is fully typed, update the parseProxyPath
implementation to produce that ProxyPathInfo | null shape (including any
decoding/validation logic) and update all call sites to expect the new type;
ensure you add or reuse a ProxyPathInfo interface/type and adjust
imports/exports accordingly so no unknown is leaked from parseProxyPath.
---
Outside diff comments:
In `@packages/browser-connection/src/index.ts`:
- Around line 1-48: The PR claims formal invariants and purity but the code
lacks the required functional/TSDoc annotations; add TSDoc-style functional
comments for the core API and pure helpers — specifically annotate
BrowserConnection methods (startBrowser, getCdpUrl, getNoVncUrl, getVncUrl,
parseProxyPath, rewriteCdpUrl) and the pure helper functions renderNoVncUrl,
renderCdpUrl, isSingleBrowserSession with the required fields: CHANGE, WHY,
QUOTE or n/a, REF, SOURCE or n/a, FORMAT THEOREM expressing the invariant
relating getCdpUrl(projectId) and getNoVncUrl(projectId) (e.g. ∀projectId:
getCdpUrl(projectId) ↔ getNoVncUrl(projectId) map to same dg-*-browser
container), PURITY (mark helpers as PURITY: CORE and shell functions
appropriately with EFFECT signature), INVARIANT (explicitly restate the
container equivalence), and COMPLEXITY; ensure the theorem references the exact
function names (getCdpUrl, getNoVncUrl, renderCdpUrl, renderNoVncUrl,
isSingleBrowserSession) so reviewers can verify the formal claims.
🪄 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: a496f201-8d73-448c-83f2-ac7480ca0d15
📒 Files selected for processing (4)
packages/browser-connection/package.jsonpackages/browser-connection/src/index.tspackages/browser-connection/tsconfig.jsonpnpm-workspace.yaml
📜 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). (11)
- GitHub Check: E2E (Clone cache)
- GitHub Check: E2E (Login context)
- GitHub Check: E2E (Runtime volumes + SSH)
- GitHub Check: Types
- GitHub Check: E2E (OpenCode)
- GitHub Check: Lint
- GitHub Check: E2E (Browser command)
- GitHub Check: Build
- GitHub Check: Test
- GitHub Check: E2E (Clone auto-open SSH)
- GitHub Check: Final build (windows-latest)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{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:
pnpm-workspace.yamlpackages/browser-connection/tsconfig.jsonpackages/browser-connection/package.jsonpackages/browser-connection/src/index.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Ты строгий ревьюер SPEC DRIVEN DEVELOPMENT.Перед выводами изучи README.md, другие *.md файлы, linked issues,
PR description, PR comments/discussion и релевантную кодовую базу.Сверь изменения с исходным ТЗ/спекой и обсуждением. Флагай любой уход
от спеки, недокументированное изменение поведения, отсутствие тестов
для заявленного поведения и security-риск. Если спека не видна,
попроси автора добавить ее в issue или PR description.Проверь решение с точки зрения формальной верификации: какие инварианты,
предусловия и постусловия можно доказать математически, а где доказуемость
слабая. Оцени решение с точки зрения теории игр: устойчивы ли стимулы,
нет ли выгодного обхода правил, и какое решение было бы сильнее.
Files:
pnpm-workspace.yamlpackages/browser-connection/tsconfig.jsonpackages/browser-connection/package.jsonpackages/browser-connection/src/index.ts
**/{package*.json,requirements*.txt,setup.py,setup.cfg,Pipfile,Pipfile.lock,pyproject.toml,pom.xml,build.gradle,Gemfile,Gemfile.lock,go.mod,go.sum,composer.json,Cargo.toml,Cargo.lock}
📄 CodeRabbit inference engine (Custom checks)
Fail if dependency or package-manager changes materially increase supply-chain risk without justification
Files:
packages/browser-connection/package.json
**/*.{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/browser-connection/src/index.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/browser-connection/src/index.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/browser-connection/src/index.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/browser-connection/src/index.ts
🔇 Additional comments (6)
packages/browser-connection/tsconfig.json (1)
1-10: LGTM!pnpm-workspace.yaml (1)
4-4: LGTM!packages/browser-connection/package.json (1)
38-39: Проверьте актуальность и уязвимостиtypescript/vitestвpackages/browser-connection/package.json(стр. 38–39)"typescript": "^6.0.3", "vitest": "^4.1.7"
typescript@^6.0.3: npm latest совпадает с6.0.3, для пакета не найдено security-advisories в текущей выборке.vitest@^4.1.7: npm latest совпадает с4.1.7; уязвимости в текущей выборке относятся к старым диапазонам (например,<=0.0.125,2.0.0–2.1.9,3.0.0–3.0.4), тогда как^4.1.7(>=4.1.7 <5.0.0) попадает вне этих уязвимых range.- Дополнительно: в security-выборке также фигурировал
effect(есть advisories для версий<3.20.0и для очень старых версий); проверьте, какая версияeffectреально объявлена в этомpackage.json, чтобы убедиться, что она не попадает в уязвимые диапазоны.packages/browser-connection/src/index.ts (3)
17-17: LGTM!
47-47: LGTM!
14-14:⚠️ Potential issue | 🟠 Major | ⚡ Quick winМетод rewriteCdpUrl должен возвращать Effect для соответствия паттерну Effect-TS композиции.
Согласно guidelines для
**/*.{ts,tsx}: "Use monadic composition with Effect-TS for all effects:Effect<Success, Error, Requirements>. Compose effects throughpipe()andEffect.flatMap()". МетодrewriteCdpUrlвозвращает plainstring, что нарушает возможность композиции черезpipe().🔄 Предлагаемое исправление
- readonly rewriteCdpUrl: (value: string, externalOrigin: string, projectId: string) => string + readonly rewriteCdpUrl: (value: string, externalOrigin: string, projectId: string) => Effect.Effect<string, never>И обновить реализацию в Layer:
- rewriteCdpUrl: (value: string, _externalOrigin: string, _projectId: string) => value + rewriteCdpUrl: (value: string, _externalOrigin: string, _projectId: string) => Effect.succeed(value)Согласно coding guidelines: "All functions must use Effect-TS for composing effects:
Effect<Success, Error, Requirements>."> Likely an incorrect or invalid review comment.
| export class BrowserError { | ||
| readonly _tag = "BrowserError" as const | ||
| constructor(readonly message: string, readonly cause?: unknown) {} | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Требуется comprehensive TSDoc для класса BrowserError.
Согласно guidelines для **/*.{ts,tsx}, все функции и типы должны включать comprehensive TSDoc с параметрами, типами возвращаемых значений, маркерами @pure, @effect, @invariant, @complexity и другими. Класс BrowserError не содержит документации.
📝 Предлагаемая документация
+/**
+ * Error type for browser connection failures.
+ *
+ * `@remarks`
+ * PURITY: SHELL - error class for service layer
+ * INVARIANT: message is non-empty string
+ * COMPLEXITY: O(1) construction
+ *
+ * `@example`
+ * ```ts
+ * new BrowserError("Failed to start browser", originalError)
+ * ```
+ */
export class BrowserError {
readonly _tag = "BrowserError" as const
+ /**
+ * `@param` message - Human-readable error description
+ * `@param` cause - Optional underlying error that caused this failure
+ */
constructor(readonly message: string, readonly cause?: unknown) {}
}Согласно coding guidelines для TypeScript: "TypeScript functions must include comprehensive TSDoc with parameters, return types, @pure marker, @effect dependencies, @invariant (mathematical), @precondition, @postcondition, and @complexity O-notation."
🤖 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/browser-connection/src/index.ts` around lines 3 - 6, Add a
comprehensive TSDoc block for the BrowserError class: document the class
purpose, the readonly _tag, and the constructor parameters (message: string,
cause?: unknown) with `@param` entries, include an example showing usage (e.g.,
new BrowserError("Failed to start browser", originalError)), and add required
markers such as `@pure`, `@effect` (if any side-effects/dependencies), `@invariant`
(if applicable), `@precondition/`@postcondition for constructor behavior, and
`@complexity` O(1); attach the doc block immediately above the export class
BrowserError declaration and reference the constructor, _tag, message, and cause
symbols in the comments.
| export interface BrowserConnection { | ||
| readonly startBrowser: (projectId: string) => Effect.Effect<void, BrowserError> | ||
| readonly getCdpUrl: (projectId: string) => Effect.Effect<string, BrowserError> | ||
| readonly getNoVncUrl: (projectId: string) => Effect.Effect<string, BrowserError> | ||
| readonly getVncUrl: (projectId: string) => Effect.Effect<string, BrowserError> | ||
| readonly parseProxyPath: (pathname: string) => Effect.Effect<unknown, never> | ||
| readonly rewriteCdpUrl: (value: string, externalOrigin: string, projectId: string) => string | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Отсутствует comprehensive TSDoc для методов интерфейса BrowserConnection.
Согласно guidelines для **/*.{ts,tsx}, все функции должны включать TSDoc с @effect dependencies, @invariant, @precondition, @postcondition, @complexity. Интерфейс не содержит документации для методов.
📝 Пример требуемой документации
+/**
+ * Browser connection service for managing single shared browser sessions.
+ *
+ * `@remarks`
+ * PURITY: SHELL - service interface with side effects
+ * EFFECT: Depends on Logger, potentially Docker/Process services
+ * INVARIANT: ∀projectId: getCdpUrl(projectId) and getNoVncUrl(projectId) point to same dg-{projectId}-browser container
+ */
export interface BrowserConnection {
+ /**
+ * Start browser container for the given project.
+ *
+ * `@effect` Launches Docker container, creates network resources
+ * `@complexity` O(1) API call, O(n) container startup time
+ * `@throws` BrowserError if container fails to start
+ */
readonly startBrowser: (projectId: string) => Effect.Effect<void, BrowserError>
+ // ... документация для остальных методовСогласно coding guidelines: "TypeScript functions must include comprehensive TSDoc with parameters, return types, @pure marker, @effect dependencies, @invariant (mathematical), @precondition, @postcondition, and @complexity O-notation."
🤖 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/browser-connection/src/index.ts` around lines 8 - 15, Add
comprehensive TSDoc comments to the BrowserConnection interface methods:
startBrowser, getCdpUrl, getNoVncUrl, getVncUrl, parseProxyPath, and
rewriteCdpUrl. For each method document parameters and return type, mark `@pure`
where applicable, add `@effect` dependencies (e.g., Effect), include `@invariant`
(mathematical contract), `@precondition` (input requirements), `@postcondition`
(output guarantees), and `@complexity` (Big-O), and ensure tags reflect any thrown
BrowserError or never effects; place these comments directly above the
respective method signatures in the BrowserConnection interface.
| readonly getCdpUrl: (projectId: string) => Effect.Effect<string, BrowserError> | ||
| readonly getNoVncUrl: (projectId: string) => Effect.Effect<string, BrowserError> | ||
| readonly getVncUrl: (projectId: string) => Effect.Effect<string, BrowserError> | ||
| readonly parseProxyPath: (pathname: string) => Effect.Effect<unknown, never> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Метод parseProxyPath должен возвращать типизированное значение вместо unknown.
Согласно guidelines для **/*.{ts,tsx}: "No any type allowed; all boundary data must be typed, with unknown only at SHELL entry points before decoding". Метод parseProxyPath возвращает Effect.Effect<unknown, never>, что ослабляет type safety. Следует определить конкретный тип возвращаемого значения (например, ProxyPathInfo | null).
🔧 Предлагаемое улучшение типизации
+/**
+ * Parsed proxy path information
+ */
+export interface ProxyPathInfo {
+ readonly projectId: string
+ readonly path: string
+}
+
export interface BrowserConnection {
// ...
- readonly parseProxyPath: (pathname: string) => Effect.Effect<unknown, never>
+ readonly parseProxyPath: (pathname: string) => Effect.Effect<ProxyPathInfo | null, BrowserError>Согласно coding guidelines: "No any type allowed; all boundary data must be typed, with unknown only at SHELL entry points before decoding."
🤖 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/browser-connection/src/index.ts` at line 13, The parseProxyPath
method currently returns Effect.Effect<unknown, never>; change its signature to
return a typed effect such as Effect.Effect<ProxyPathInfo | null, never> (or
another concrete discriminated type you define) so boundary data is fully typed,
update the parseProxyPath implementation to produce that ProxyPathInfo | null
shape (including any decoding/validation logic) and update all call sites to
expect the new type; ensure you add or reuse a ProxyPathInfo interface/type and
adjust imports/exports accordingly so no unknown is leaked from parseProxyPath.
| export const BrowserConnectionLive = Layer.effect( | ||
| BrowserConnection, | ||
| Effect.gen(function* () { | ||
| return { | ||
| startBrowser: (projectId: string) => | ||
| Effect.gen(function* () { | ||
| yield* Effect.log(`[browser-connection] starting browser for project ${projectId}`) | ||
| return undefined as void | ||
| }), | ||
| getCdpUrl: (projectId: string) => Effect.succeed(`http://localhost:9223?project=${projectId}`), | ||
| getNoVncUrl: (projectId: string) => Effect.succeed(`/b/${projectId}/vnc.html?autoconnect=true&resize=remote&path=b/${projectId}/websockify`), | ||
| getVncUrl: (projectId: string) => Effect.succeed(`vnc://localhost:5900`), | ||
| parseProxyPath: (_pathname: string) => Effect.succeed(null), | ||
| rewriteCdpUrl: (value: string, _externalOrigin: string, _projectId: string) => value | ||
| } | ||
| }) | ||
| ) |
There was a problem hiding this comment.
Отсутствуют обязательные functional comments для всех методов Layer.
Согласно guidelines для **/*.{ts,tsx}: "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". В реализации BrowserConnectionLive полностью отсутствуют эти комментарии, что делает невозможной формальную верификацию, заявленную в PR objectives.
📋 Пример требуемых functional comments
+// CHANGE: Implement BrowserConnectionLive Layer with stub methods for single-session browser management
+// WHY: Consolidate browser control logic to ensure single shared session invariant
+// QUOTE(ТЗ): "When MCP Playwright is installed, it should automatically start noVNC to manage a single shared browser with an agent" (issue `#347`)
+// REF: issue `#347`, PR `#348`
+// SOURCE: docker-git-session-sync patterns, effect-template style
+// FORMAT THEOREM: ∀projectId ∈ String: getCdpUrl(projectId) ∧ getNoVncUrl(projectId) → same container(dg-${projectId}-browser)
+// PURITY: SHELL - contains side effects (logging, future Docker operations)
+// EFFECT: Effect.Effect<BrowserConnection, never, never>
+// INVARIANT: Single browser session per projectId; CDP port 9223, noVNC port 6080 are fixed
+// COMPLEXITY: O(1) for all operations (current stub implementation)
export const BrowserConnectionLive = Layer.effect(
BrowserConnection,
Effect.gen(function* () {
return {
+ // CHANGE: Log browser start event
+ // WHY: Provide observability for browser lifecycle
+ // PURITY: SHELL (logging side effect)
+ // EFFECT: Effect.Effect<void, BrowserError>
+ // COMPLEXITY: O(1)
startBrowser: (projectId: string) =>
Effect.gen(function* () {
yield* Effect.log(`[browser-connection] starting browser for project ${projectId}`)
}),
+ // CHANGE: Return deterministic CDP URL for projectId
+ // PURITY: CORE logic (deterministic string generation)
+ // EFFECT: Effect.Effect<string, BrowserError>
+ // INVARIANT: Port 9223 is fixed for CDP endpoint
+ // COMPLEXITY: O(1)
getCdpUrl: (projectId: string) => Effect.succeed(renderCdpUrl(projectId)),
// ... аналогично для остальных методовСогласно coding guidelines: "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." Также см. требование из guidelines: "Ты строгий ревьюер SPEC DRIVEN DEVELOPMENT. Сверь изменения с исходным ТЗ/спекой и обсуждением."
🤖 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/browser-connection/src/index.ts` around lines 19 - 35, Add the
required functional comments for the BrowserConnectionLive Layer and each
exposed method (BrowserConnectionLive, startBrowser, getCdpUrl, getNoVncUrl,
getVncUrl, parseProxyPath, rewriteCdpUrl): for every function provide the header
including CHANGE, WHY, QUOTE(ТЗ) or n/a, REF, SOURCE or n/a, FORMAT THEOREM,
PURITY (CORE|SHELL) and for SHELL functions include the EFFECT signature, plus
INVARIANT and COMPLEXITY. Place the comment block immediately above the Layer
implementation and above each method implementation so the formal verification
tooling can parse them; ensure the EFFECT signature matches the returned Effect
types (e.g., Effect.succeed/Effect.gen) and that each comment references the
same function name to uniquely identify the behavior.
| startBrowser: (projectId: string) => | ||
| Effect.gen(function* () { | ||
| yield* Effect.log(`[browser-connection] starting browser for project ${projectId}`) | ||
| return undefined as void |
There was a problem hiding this comment.
Запрещено использование type assertion as void.
Согласно guidelines для **/*.{ts,tsx}: "Type casting with as is strictly forbidden in regular code; permitted ONLY in a single axiomatic module (brands/constructors/constants)". Строка 26 использует return undefined as void, что нарушает это правило.
🔧 Предлагаемое исправление
startBrowser: (projectId: string) =>
Effect.gen(function* () {
yield* Effect.log(`[browser-connection] starting browser for project ${projectId}`)
- return undefined as void
}),Effect.gen автоматически выведет тип Effect.Effect<void, never> если не возвращать значение явно.
Согласно coding guidelines: "Type casting with as is strictly forbidden in regular code; permitted ONLY in a single axiomatic module."
📝 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.
| return undefined as void | |
| startBrowser: (projectId: string) => | |
| Effect.gen(function* () { | |
| yield* Effect.log(`[browser-connection] starting browser for project ${projectId}`) | |
| }), |
🤖 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/browser-connection/src/index.ts` at line 26, Replace the forbidden
type assertion "return undefined as void" with a non-casted return (either
remove the return entirely so Effect.gen infers Effect<void, never>, or use a
plain "return" / "return undefined" without "as void"); locate the exact
statement "return undefined as void" in packages/browser-connection/src/index.ts
and remove the "as void" cast so no type assertion is used.
| yield* Effect.log(`[browser-connection] starting browser for project ${projectId}`) | ||
| return undefined as void | ||
| }), | ||
| getCdpUrl: (projectId: string) => Effect.succeed(`http://localhost:9223?project=${projectId}`), |
There was a problem hiding this comment.
Несоответствие URL между getCdpUrl и renderCdpUrl.
Метод getCdpUrl (строка 28) возвращает http://localhost:9223?project=${projectId}, в то время как helper renderCdpUrl (строка 42) возвращает http://localhost:9223/json/version?project=${projectId}. Это приведет к ошибкам при использовании разных методов для получения одного и того же URL.
🔧 Предлагаемое исправление
Вариант 1: Использовать helper в реализации Layer:
- getCdpUrl: (projectId: string) => Effect.succeed(`http://localhost:9223?project=${projectId}`),
+ getCdpUrl: (projectId: string) => Effect.succeed(renderCdpUrl(projectId)),Вариант 2: Привести helper в соответствие с реализацией Layer:
export const renderCdpUrl = (projectId: string): string =>
- `http://localhost:9223/json/version?project=${projectId}`
+ `http://localhost:9223?project=${projectId}`Рекомендуется вариант 1, чтобы избежать дублирования строковых литералов.
Also applies to: 41-42
🤖 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/browser-connection/src/index.ts` at line 28, getCdpUrl currently
returns a different string than the helper renderCdpUrl which will cause
inconsistent CDP endpoints; update the Layer implementation of getCdpUrl to
call/render via the existing renderCdpUrl helper (or, alternatively, make
renderCdpUrl match the literal used by getCdpUrl) so both functions produce the
exact same URL format—locate getCdpUrl and renderCdpUrl in
packages/browser-connection/src/index.ts and replace the literal in getCdpUrl
with a call to renderCdpUrl(projectId) to avoid duplicated string literals.
| // Pure helpers | ||
| export const renderNoVncUrl = (projectId: string): string => | ||
| `/b/${projectId}/vnc.html?autoconnect=true&resize=remote&path=b/${projectId}/websockify` | ||
|
|
||
| export const renderCdpUrl = (projectId: string): string => | ||
| `http://localhost:9223/json/version?project=${projectId}` | ||
|
|
||
| export const isSingleBrowserSession = (cdpUrl: string, noVncUrl: string): boolean => | ||
| cdpUrl.includes("9223") && noVncUrl.includes("/vnc.html") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Отсутствует документация для pure helper functions.
Согласно guidelines для **/*.{ts,tsx}, все функции должны включать TSDoc с @pure marker, @complexity, @invariant и functional comments (PURITY: CORE для чистых функций). Функции renderNoVncUrl, renderCdpUrl и isSingleBrowserSession не содержат никакой документации.
📝 Пример требуемой документации
-// Pure helpers
+// CHANGE: Pure helper functions for URL generation and session validation
+// WHY: Separate pure logic (CORE) from effectful service methods (SHELL)
+// PURITY: CORE - deterministic, no side effects
+// INVARIANT: URL format stability across project lifecycle
+/**
+ * Generate noVNC URL for browser session.
+ *
+ * `@pure` true
+ * `@param` projectId - Project identifier
+ * `@returns` noVNC URL with autoconnect and websockify path
+ * `@invariant` Result always includes `/b/${projectId}/vnc.html`
+ * `@complexity` O(1) time, O(n) space where n = projectId.length
+ */
export const renderNoVncUrl = (projectId: string): string =>
`/b/${projectId}/vnc.html?autoconnect=true&resize=remote&path=b/${projectId}/websockify`
+/**
+ * Generate CDP (Chrome DevTools Protocol) URL for browser session.
+ *
+ * `@pure` true
+ * `@param` projectId - Project identifier
+ * `@returns` CDP endpoint URL on port 9223
+ * `@invariant` Port 9223 is fixed for all projects
+ * `@complexity` O(1) time, O(n) space where n = projectId.length
+ */
export const renderCdpUrl = (projectId: string): string =>
`http://localhost:9223/json/version?project=${projectId}`
+/**
+ * Validate that CDP and noVNC URLs belong to single browser session.
+ *
+ * `@pure` true
+ * `@param` cdpUrl - CDP endpoint URL
+ * `@param` noVncUrl - noVNC web interface URL
+ * `@returns` true if both URLs point to same browser session
+ * `@precondition` cdpUrl and noVncUrl are non-empty strings
+ * `@complexity` O(n) where n = max(cdpUrl.length, noVncUrl.length)
+ */
export const isSingleBrowserSession = (cdpUrl: string, noVncUrl: string): boolean =>
cdpUrl.includes("9223") && noVncUrl.includes("/vnc.html")Согласно coding guidelines: "TypeScript functions must include comprehensive TSDoc with parameters, return types, @pure marker, @effect dependencies, @invariant (mathematical), @precondition, @postcondition, and @complexity O-notation" и "Document all functions with comprehensive TSDoc including: @pure (true/false), @effect (required services), @invariant (mathematical invariants), @precondition, @postcondition, @complexity (time and space)."
🤖 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/browser-connection/src/index.ts` around lines 37 - 45, Add
comprehensive TSDoc for the three pure helper functions: renderNoVncUrl,
renderCdpUrl, and isSingleBrowserSession; for each function include a short
functional description, `@param` and `@returns` tags, a `@pure` marker set to true (or
PURE: CORE in the top-line comment), `@effect` (none or required services),
`@precondition` (e.g. non-empty projectId or valid URL strings), `@postcondition`
(output string contains expected path or query), `@invariant` (mathematical
properties like idempotence or substring containment), and `@complexity` with
time/space O-notation; place the TSDoc immediately above each function
declaration so tools and linters recognize them and ensure the documented
invariants mention the exact behavior of renderNoVncUrl (produces
/b/{projectId}/vnc.html URL), renderCdpUrl (produces localhost 9223 JSON URL
with project query), and isSingleBrowserSession (returns true when cdpUrl
includes "9223" and noVncUrl includes "/vnc.html").
| export const isSingleBrowserSession = (cdpUrl: string, noVncUrl: string): boolean => | ||
| cdpUrl.includes("9223") && noVncUrl.includes("/vnc.html") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Слишком упрощенная логика валидации в isSingleBrowserSession.
Функция isSingleBrowserSession использует только проверку подстрок (includes("9223") и includes("/vnc.html")), что не гарантирует, что URL действительно указывают на один и тот же browser container. Согласно PR objectives, должен быть математический инвариант: "getCdpUrl(projectId) and getNoVncUrl(projectId) point to the same dg-*-browser container".
Текущая реализация пропустит любые URL, содержащие эти подстроки, даже если они указывают на разные проекты или контейнеры.
🔧 Предлагаемое улучшение валидации
+/**
+ * Extract projectId from browser URL.
+ * `@pure` true
+ */
+const extractProjectId = (url: string): string | null => {
+ // For CDP: http://localhost:9223/json/version?project=abc
+ // For noVNC: /b/abc/vnc.html...
+ const cdpMatch = url.match(/[?&]project=([^&]+)/)
+ if (cdpMatch) return cdpMatch[1]
+
+ const vncMatch = url.match(/\/b\/([^\/]+)\/vnc\.html/)
+ if (vncMatch) return vncMatch[1]
+
+ return null
+}
+
export const isSingleBrowserSession = (cdpUrl: string, noVncUrl: string): boolean => {
- cdpUrl.includes("9223") && noVncUrl.includes("/vnc.html")
+ const cdpProjectId = extractProjectId(cdpUrl)
+ const vncProjectId = extractProjectId(noVncUrl)
+
+ return cdpProjectId !== null &&
+ vncProjectId !== null &&
+ cdpProjectId === vncProjectId &&
+ cdpUrl.includes("9223") &&
+ noVncUrl.includes("/vnc.html")
+}Это обеспечит математический инвариант: оба URL указывают на один projectId.
🤖 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/browser-connection/src/index.ts` around lines 44 - 45, Функция
isSingleBrowserSession слишком примитивно проверяет URL через includes; нужно
извлечь и сопоставить реальный projectId/имя контейнера из cdpUrl и noVncUrl
вместо простых подстрок. Измените isSingleBrowserSession так, чтобы она парсила
переданные URL (например регуляркой) и вытягивала идентификатор dg-*-browser или
projectId/namespace из хоста/пути/параметров для обоих URL, затем возвращала
true только если оба извлечённых projectId/имени контейнера совпадают и
порт/маршрут соответствует ожидаемому (CDP порт 9223 и noVnc путь /vnc.html) —
используйте имена функций/констант getCdpUrl/getNoVncUrl и саму
isSingleBrowserSession как точки внесения правок.
1 participant
Extracted noVNC + browser CDP connection into its own package
@prover-coder-ai/browser-connection.Changes
docker-git-session-syncand effect-template styleBrowserConnectionEffect Layer with pure functions, invariants, formal commentsMathematical guarantees
Closes #347
Testing