feat(browser): delegate MCP/noVNC runtime to rust-browser-connection#349
Open
skulidropek wants to merge 8 commits into
Open
feat(browser): delegate MCP/noVNC runtime to rust-browser-connection#349skulidropek wants to merge 8 commits into
skulidropek wants to merge 8 commits 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
…tion per #347 - New package based on rust-ai-driven-development-pipeline-template - BrowserConnection with start_browser, get_novnc_url, get_cdp_url, invariant - Uses bollard for Docker, replicates docker-git MCP Playright + noVNC behavior - Out of the box for both MCP and Hermes built-in browser tools Closes #347
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughPR мигрирует встроенный Playwright browser runtime на единый Rust-модуль ChangesBrowser Runtime Migration: Playwright → Rust
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Member
Author
AI Session BackupCommit: 72b751d
|
…eline-template + our noVNC/browser module for #347
Member
Author
AI Session BackupCommit: 572a163
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 59
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/rust-browser-connection/docs/case-studies/issue-38/template-data/js-template-ci-tree.txt (1)
1-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winНепереносимые абсолютные пути в template-data.
На Line 1-23 зафиксированы локальные
/tmp/...пути. Для репозиторных template-data лучше хранить относительные пути от корня проекта, иначе артефакт зависит от окружения генерации и шумит в сравнении.🤖 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/rust-browser-connection/docs/case-studies/issue-38/template-data/js-template-ci-tree.txt` around lines 1 - 23, В файле js-template-ci-tree.txt найдено множество зафиксированных абсолютных путей вида /tmp/js-ai-driven-development-pipeline-template/..., которые делают артефакт зависимым от окружения; замените все такие абсолютные пути на репозиторно-относительные пути (например относительные от корня проекта или шаблонные маркеры ./ или {repo_root}/), обновите перечисления в файле (каждую строку начинающуюся с /tmp/js-ai-driven-development-pipeline-template/) чтобы не включать /tmp префикс, и убедитесь, что после изменения сравнениe шаблон-данных стабильно между машинами сборки.packages/rust-browser-connection/docs/case-studies/issue-38/raw-data/issue-38.json (1)
1-2:⚠️ Potential issue | 🟠 Major | ⚡ Quick winСанитизируйте author/commenter данные в case-study raw JSON.
Сейчас в артефакт включены персональные поля (
name,login,id) и прочие user-идентификаторы. Для анализа CI/CD проблемы они не обязательны; удалите/обезличьте user-блоки при сохранении в репозиторий.As per coding guidelines, "
**/*.{js,ts,jsx,tsx,py,java,go,rb,php,sh,bash,yml,yaml,json,env*,toml,cfg,config,dockerfile,dockerignore}: Fail if changed files expose credentials, tokens, private-keys, or PII in source, generated config, logs, or CI output".🤖 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/rust-browser-connection/docs/case-studies/issue-38/raw-data/issue-38.json` around lines 1 - 2, Sanitize the case-study raw JSON by removing/obfuscating all personal user fields inside author and comments.author blocks in the artifact (e.g., the top-level "author" object and each "comments[].author"); specifically strip or replace id, login, name, url and any other user-identifiers with non-PII placeholders (e.g., "anon_user_1") while preserving non-PII fields needed for analysis (timestamps, body, labels, title, number, state), and update the code that writes issue-38.json to perform this transformation before saving so generated artifacts never contain PII and thus satisfy the repository CI credential/PII scanning rules.packages/lib/src/core/browser-connection.ts (1)
16-131:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winКритично: browser-connection.ts содержит дублирующиеся экспортируемые объявления и неразрешённый тип
- В файле
packages/lib/src/core/browser-connection.tsдважды объявлены и экспортируютсяBrowserError,BrowserConnection,BrowserConnectionLive, а также дважды встречаетсяexport default BrowserConnection(пример: строки ~16–67 и ~72–130) — это блокирует сборку.- Тип
ProjectBrowserProxyPathиспользуется вparseProxyPath, но не импортируется/не объявлен в модуле — ещё один компиляционный блокер (строка ~82).- Есть запрещённые по coding-guidelines конструкции:
unknown(в интерфейсах/конструкторе) иas-утверждения (as const,return undefined as void) в коде этого модуля.🤖 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/browser-connection.ts` around lines 16 - 131, This file contains duplicate exported declarations (BrowserError, BrowserConnection, BrowserConnectionLive and two default exports) and uses forbidden constructs (`unknown`, `as const`, `as void`) and an undeclared type (ProjectBrowserProxyPath). Consolidate into a single set of exports: keep one BrowserError class (replace cause: unknown with cause?: Error | undefined), one BrowserConnection interface (use parseProxyPath: Effect.Effect<ProjectBrowserProxyPath | null, never>), one BrowserConnectionLive implementation, and a single default export BrowserConnection; remove the duplicate blocks and merge the intended runtime stubs (startBrowser/getCdpUrl/getNoVncUrl/getVncUrl/parseProxyPath/rewriteCdpUrl) into that single implementation. Replace `as const` and `as void` assertions with proper typing/returns, and import or declare the ProjectBrowserProxyPath type at the top so parseProxyPath compiles. Ensure rewriteCdpUrl/getCdpUrl implementations remain consistent after consolidation.
🤖 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/package.json`:
- Around line 35-43: В package.json пакета `@prover-coder-ai/browser-connection`
зафиксируйте версии зависимостей — замените caret-диапазоны в полях
"dependencies" и "devDependencies" на конкретные версии (удалите "^" у effect,
typescript, vitest, `@effect/vitest`, `@types/node` и т.д.) чтобы публикуемый пакет
через bun publish содержал точные версии и не расширял supply-chain поверхность
для потребителей.
In `@packages/browser-connection/src/index.ts`:
- Around line 23-33: BrowserConnectionLive currently doesn't implement the
runtime contract: implement startBrowser to actually start/record a single
browser session for the given projectId (create/store session state and log
errors instead of returning undefined), implement parseProxyPath to parse the
incoming pathname and return a project id or session descriptor instead of
always null, make rewriteCdpUrl use the passed externalOrigin and projectId to
rewrite the CDP URL correctly (not just return value), and make getVncUrl
include projectId (and any per-project port or path mapping) rather than
returning a constant; update the implementations in BrowserConnectionLive
(startBrowser, parseProxyPath, rewriteCdpUrl, getVncUrl) so they coordinate with
whatever session storage/registry the runtime uses to ensure a single managed
browser session per project.
- Around line 3-47: Add required functional TSDoc comments for all exported
symbols: BrowserError, BrowserConnection, BrowserConnectionLive, renderNoVncUrl,
renderCdpUrl, and isSingleBrowserSession. For each exported type/class/function
place a TSDoc block immediately above its declaration and include at minimum: a
short description, and the required markers such as `@CHANGE` or `@WHY` (purpose),
`@INVARIANT` (behavior guarantees), `@COMPLEXITY` (complexity/cost), and
`@PURITY/`@EFFECT as applicable; for BrowserConnectionLive document side-effects
and environment wiring with `@EFFECT`, for helpers document purity with `@PURITY`
and examples/usage where helpful. Ensure tags and descriptions follow the
project coding-guidelines format and appear for every exported symbol listed.
- Around line 3-15: Fix the public TS contract by removing "as" casts and
"unknown" from exported types: change BrowserError to use a plain string literal
tag (no "as const") and give cause a concrete domain/error type (not unknown),
update BrowserConnection.parseProxyPath to return
Effect.Effect<ProjectBrowserProxyPath | null, never> (and replace
Effect.succeed(null) with the typed success), and stop using "as void" in
startBrowser implementation—return a proper Effect like Effect.unit or
Effect.succeedVoid instead; also add minimal TSDoc/functional comment markers
above the exported BrowserError and BrowserConnection to document the public
API. Ensure you reference and update the symbols BrowserError,
BrowserConnection, startBrowser, and parseProxyPath in the file.
In `@packages/lib/src/core/browser-connection.ts`:
- Around line 32-46: The current BrowserConnectionLive implementation provides
stubbed startBrowser and URL methods that are not tied to the actual
dg-*-browser container, so it does not enforce the required single unified
browser session invariant; update BrowserConnectionLive so startBrowser
negotiates/creates or looks up a single shared browser container/session (e.g.,
by calling the container manager or session registry), store and reuse the
session handle, and make getCdpUrl, getNoVncUrl, getVncUrl, parseProxyPath and
rewriteCdpUrl derive their values from that stored session (failing or
refreshing the session if missing/expired) instead of returning hardcoded
values; ensure rewriteCdpUrl performs origin-aware rewriting using the real
external origin and stored project/session metadata.
- Line 17: The core module violates project bans on "as" casts and unknown:
update the BrowserError class to declare readonly _tag: "BrowserError" =
"BrowserError" (remove as const); change the constructor signature from cause?:
unknown to a concrete type such as cause?: Error | undefined (remove unknown),
and remove any duplicate cause?: unknown declarations; change parseProxyPath's
Effect type from Effect.Effect<unknown, never> to a concrete environment type
(e.g., Effect.Effect<never, never> or a specific env/interface used by your
effects) instead of unknown; and replace any return undefined as void with an
explicit bare return (or return void 0) to avoid using as. Ensure you update all
occurrences of these symbols (BrowserError, constructor(cause), parseProxyPath,
and the return site) consistently.
- Line 82: The parseProxyPath signature references the missing type
ProjectBrowserProxyPath; add a proper type definition or an import for
ProjectBrowserProxyPath into this CORE module and update the declaration for
readonly parseProxyPath accordingly; locate parseProxyPath in
browser-connection.ts and either (a) import the ProjectBrowserProxyPath type
from the API package where it’s defined, or (b) copy/declare a compatible local
type in this file/module and export it if needed, then run type-checks to ensure
no unresolved type errors remain.
In `@packages/rust-browser-connection/.github/workflows/release.yml`:
- Around line 478-483: The run step in the "Version and commit" job is
interpolating workflow_dispatch inputs directly into the shell command
(rust-script ... --bump-type "${{ github.event.inputs.bump_type }}"
--description "${{ github.event.inputs.description }}"), which is unsafe; update
the step to use the already-declared environment variables (BUMP_TYPE and
DESCRIPTION) instead (e.g., reference $BUMP_TYPE and $DESCRIPTION in the run
command) and make the same change for the other occurrence referenced (lines
596-600) so all inputs are consumed via env variables rather than direct
github.event.inputs interpolation; keep the step id/version and env names (id:
version, env: BUMP_TYPE, DESCRIPTION) intact.
- Around line 655-657: В workflow_dispatch-triggered job the checkout is pinned
to "ref: main", so manual releases started from a non-main branch will still
build/deploy docs for main; update the workflow to either restrict manual
dispatches to main (add a condition limiting the workflow/job to github.ref ==
'refs/heads/main' or restrict triggers to branches: main) or change the
actions/checkout step (actions/checkout@v6) to check out the current run
ref/commit by using the workflow context (use github.ref or github.sha as the
ref) so the docs are built from the exact commit that triggered the run.
In `@packages/rust-browser-connection/Cargo.toml`:
- Line 21: Remove the unused Tokio dependency entry "tokio = { version = \"1\",
features = [\"full\"] }" from Cargo.toml (or if you plan to use Tokio, replace
it with a minimal feature set) and clean up the unused import "use tokio;" in
src/lib.rs so the crate does not declare an unnecessary dependency; ensure no
other code references tokio::... or #[tokio::...] before removing.
In
`@packages/rust-browser-connection/changelog.d/20251227_224645_changeset_support.md`:
- Around line 7-14: The changelog lists JavaScript script names (.mjs) but the
package uses Rust scripts; update the fragment to reference the actual Rust
filenames and documentation: replace `get-bump-type.mjs` → `get-bump-type.rs`,
`collect-changelog.mjs` → `collect-changelog.rs`, and `version-and-commit.mjs` →
`version-and-commit.rs`, and ensure the `changelog.d/README.md` reference
remains correct; keep wording consistent in the Added/Changed bullets so the
names match the repository's scripts (e.g., in the "Enhanced release workflow"
bullet mention `get-bump-type.rs` if applicable).
In
`@packages/rust-browser-connection/changelog.d/20260111_multi_language_support.md`:
- Around line 6-19: Запись в changelog о `rust-paths.mjs` и ряде `scripts/*.mjs`
не соответствует текущему toolchain, где используются Rust-скрипты
(`scripts/*.rs`); приведи changelog (файл 20260111_multi_language_support.md) в
соответствие с реальным состоянием: либо заменяй упоминания `*.mjs` на
соответствующие `*.rs`-скрипты и обнови список затронутых скриптов (например
заменить `scripts/bump-version.mjs` → соответствующий Rust-скрипт), либо если
проект действительно добавил `rust-paths.mjs`, обнови кодовую базу/CI чтобы
включить этот файл и параметры (`--rust-root`, `RUST_ROOT`) — выбор должен быть
последовательным; проверь и поправь все упоминания `rust-paths.mjs`,
`--rust-root` и `RUST_ROOT` в changelog и в скриптах (`scripts/*.rs` или
`rust-paths.mjs`) чтобы документация отражала фактическую реализацию.
In
`@packages/rust-browser-connection/docs/case-studies/issue-19/pr-114-data/pr-commits.json`:
- Line 1: The commit dump includes personal email addresses in the
commits[*].authors.email fields (see the "commits" array and individual commit
"oid" entries); remove or redact all authors.email values (replace with null,
empty string, or a non-PII placeholder) for every commit object in the JSON and
commit the sanitized file so the file no longer contains personal emails while
preserving other fields (authoredDate, authors.login/name, messageHeadline,
oid).
In
`@packages/rust-browser-connection/docs/case-studies/issue-19/pr-114-data/pr-conversation-comments.json`:
- Line 1: The JSON file currently contains PII fields (login, id, avatar_url,
url, html_url and similar profile fields) committed into the repo; remove or
redact those fields and update the generator so it emits anonymized placeholders
instead of real values, or move the file out of source control and add it to
.gitignore; specifically edit the object entries that contain "login", "id",
"avatar_url", "url", and "html_url" to replace real values with non-identifying
placeholders (e.g., user_N, id_N, avatar_placeholder) and add/enable a CI lint
check to fail on commits containing these keys with non-placeholder formats to
prevent future PII leaks.
In
`@packages/rust-browser-connection/docs/case-studies/issue-19/pr-114-data/pr-details.json`:
- Line 1: The PR dump JSON contains PII in fields like author.id, author.login,
author.name and in comments[].author.login and comments[].body; update the
generated pr-details.json content to remove or anonymize all personal
identifiers and free-text user comments (e.g., replace author.id/login/name with
"<REDACTED>" or remove the author object, and strip or redact comments[].body),
ensure any similar keys (author, comments, createdAt) are sanitized
consistently, and add a small sanitizer step in the generation process that
avoids embedding raw user identifiers into the exported JSON.
In `@packages/rust-browser-connection/docs/case-studies/issue-21/README.md`:
- Around line 167-173: В текущем примерe env-мэппинга используется ключ
CARGO_TOKEN, что конфликтует с остальной документацией, где используется
CARGO_REGISTRY_TOKEN; исправьте пример так, чтобы ключ был CARGO_REGISTRY_TOKEN,
а значение падало назад на старое имя секрета (например: CARGO_REGISTRY_TOKEN:
${{ secrets.CARGO_REGISTRY_TOKEN || secrets.CARGO_TOKEN }}), чтобы устранить
неоднозначность и сохранить совместимость с существующими секретами.
In
`@packages/rust-browser-connection/docs/case-studies/issue-38/raw-data/downstream-meta-ontology-issue-3.json`:
- Line 1: The raw JSON commit contains PII in the author object (author.id,
author.login, author.name); remove or mask these fields before committing the
file in docs/case-studies/issue-{id} (e.g., replace values with null, hashes, or
a generic placeholder) and ensure any other PII-sensitive keys in the snapshot
(emails, user IDs, tokens) are similarly redacted; add a validation step
(pre-commit hook or CI check) that scans files matching the repository
sensitive-glob pattern (*.json, *.yml, *.env*, etc.) and fails the commit if
unmasked PII is detected.
In
`@packages/rust-browser-connection/docs/case-studies/issue-38/raw-data/downstream-meta-ontology-pr-4.json`:
- Line 1: The raw JSON includes personal identifiers in keys like "author",
"author.id", "author.login", "author.name", and within "comments[].author" which
must be depersonalized before archiving: implement a sanitization step that
walks the JSON, replaces PII fields (id, login, name, email, viewerDidAuthor)
with stable non-reversible hashes or fixed placeholders, strips or redacts
free-text fields in "body" that contain email/IDs, and ensure comments array
entries are sanitized too; locate and update the code that writes this artifact
(the routine that serializes or saves the PR raw-data JSON) to call this
sanitizer prior to persist, and add tests verifying that "author" and
"comments[].author" no longer contain raw PII.
In
`@packages/rust-browser-connection/docs/case-studies/issue-38/raw-data/issue-38-comments.json`:
- Line 1: The committed raw JSON contains PII fields (e.g., "login", "id",
"html_url", "url", "avatar_url", "gravatar_id", "followers_url",
"following_url", "gists_url", "starred_url", "subscriptions_url",
"organizations_url", "repos_url", "events_url", "received_events_url") in
packages/rust-browser-connection/docs/case-studies/issue-38/raw-data/issue-38-comments.json;
redact or anonymize those fields before committing by removing or replacing
values with non-identifying placeholders or hashes (keep only necessary metadata
for analysis such as "created_at", "body", "author_association", and the comment
"id" if required but obfuscated), update the file in the case-studies/issue-38
folder accordingly, and re-run any CI/validation that checks for exposed PII to
ensure the change passes.
In
`@packages/rust-browser-connection/docs/case-studies/issue-38/raw-data/pr-39-reviews.json`:
- Line 1: The raw-data file
packages/rust-browser-connection/docs/case-studies/issue-38/raw-data/pr-39-reviews.json
was wiped to [] with no explanation; either restore the original raw review
array or, within the same PR, add an explicit justification for clearing it
(e.g., a note in the PR description and a documented change entry next to
pr-39-reviews.json explaining why data was removed, what replaced it, and how to
reproduce previous results) so the change is traceable and not treated as an
undocumented loss of artifacts.
In
`@packages/rust-browser-connection/docs/case-studies/issue-38/raw-data/pr-39.json`:
- Line 1: The PR export is currently writing raw author PII (author.id,
author.login, author.name) into the saved JSON; update the PR
serialization/export routine that produces this JSON to redact or hash those
fields before writing (e.g., replace author.id/login/name with hashed tokens or
remove them and keep a non-reversible fingerprint), ensure the sanitized shape
preserves any needed non-PII fields (e.g., createdAt, title, number), and add a
unit/test verifying that author.id, author.login and author.name are not present
in the persisted output.
In
`@packages/rust-browser-connection/docs/case-studies/issue-52/raw-data/issue-52.json`:
- Line 1: The JSON snapshot contains PII in the "user" block (fields like login,
id, node_id, avatar_url, gravatar_id, url, html_url, followers_url,
following_url, gists_url, starred_url, subscriptions_url, organizations_url,
repos_url, events_url, received_events_url) and similar actor fields (assignee,
closed_by) — redact or remove those PII fields and replace them with an
anonymized minimal user object (e.g., keep only a non-identifying type/status
and optionally an anonymized id or "anonymous": true) while preserving technical
issue fields (title, body, number, created_at, updated_at, labels, state,
repository_url, html_url) so the case study remains reproducible and compliant
with the project's PII rule.
In
`@packages/rust-browser-connection/docs/case-studies/issue-52/raw-data/js-issue-62.json`:
- Line 1: Sanity check: the exported issue JSON includes PII (user.login,
user.id, node_id, avatar_url, profile endpoint URLs, gravatar_id, and many
user-related URLs); redact or remove those fields and preserve only minimal
technical fields (e.g., title, number, state, created_at, updated_at, body,
html_url) and non-identifying user metadata if needed. Fix by updating the JSON
export/generator to strip or hash these keys (user.login, user.id, user.node_id,
user.avatar_url, user.gravatar_id, user.url, user.html_url, user.followers_url,
user.following_url, user.gists_url, user.starred_url, user.subscriptions_url,
user.organizations_url, user.repos_url, user.events_url,
user.received_events_url, user.site_admin) and replace with null or a
deterministic hash; update consumers that expect the "user" object to accept a
reduced shape (or emit a sanitized placeholder like {"type":"User"}), and add a
test to assert no PII keys remain in the produced js-issue-62.json dump.
In
`@packages/rust-browser-connection/docs/case-studies/issue-52/raw-data/vk-bot-desktop-issue-51.json`:
- Line 1: The raw JSON snapshot contains PII on Line 1 (fields like user.login,
user.id, user.avatar_url, html_url and any profile URLs); remove or redact those
fields and keep only minimal technical metadata (e.g., number, state,
created_at, aggregated status fields). Replace personal identifiers with hashed
or anonymized placeholders (not reversible), strip avatar and profile URLs, and
ensure only non-PII keys remain in the object; update the snapshot for
vk-bot-desktop issue 51 accordingly and add/verify a validation step that
rejects commits exposing user.* fields to prevent regressions.
In
`@packages/rust-browser-connection/docs/case-studies/issue-52/raw-data/vk-bot-desktop-pr-52.json`:
- Line 1: Summary: The raw JSON snapshot contains PII (user objects: login, id,
avatar_url and many profile URLs) that must be removed or anonymized. Fix: edit
the raw snapshot file vk-bot-desktop-pr-52.json to strip or hash all personal
fields inside "user", "head.user", "base.user", "assignees", "merged_by" and any
nested profile URLs — remove keys like login, id, avatar_url, url, html_url,
followers_url, following_url, gists_url, starred_url, subscriptions_url,
organizations_url, repos_url, events_url, received_events_url; keep only
necessary technical PR fields (number, title, state, created_at, updated_at,
merged_at, merge_commit_sha, commits/additions/deletions/changed_files,
html_url/diff_url) or replace user entries with a single anonymized object (e.g.
{"user_id":"anon-1234"}). Commit the sanitized file and run the repo’s
PII/credentials check before push.
In `@packages/rust-browser-connection/docs/ci-cd/troubleshooting.md`:
- Around line 94-99: В документе осталась ссылка на несуществующий Node-скрипт
`scripts/publish-crate.mjs`; нужно заменить вызов на реальный Rust-entrpoint:
вместо `node scripts/publish-crate.mjs` использовать команду запуска бинарного
скрипта через Cargo (например `cargo run --bin publish-crate` или `cargo run
--package <crate_name> --bin <bin_name>` в зависимости от настроек Cargo.toml) и
сохранить переменную окружения `CARGO_REGISTRY_TOKEN`; также обновите все
аналогичные вхождения `scripts/publish-crate.mjs` в разделе (207-225) на
соответствующие `cargo run` команды и при необходимости укажите имя бина из
`scripts/*.rs`.
In `@packages/rust-browser-connection/examples/basic_usage.rs`:
- Around line 1-7: Текущий пример демонстрирует только функцию sum, но PR обязан
показать поведение BrowserConnection/noVNC в single-session workflow; замените
содержимое examples/basic_usage.rs на пример, который импортирует и использует
BrowserConnection (или функцию-конструктор, например BrowserConnection::new /
start_browser_connection), создает одну сессию, вызывает методы получения
адресов (например generate_novnc_url и get_cdp_url или аналогичные методы в API)
и выводит оба URL в stdout, затем корректно закрывает сессию/соединение;
убедитесь что используются реальные символы из библиотеки (BrowserConnection,
Session, generate_novnc_url, get_cdp_url, close_session) и что пример
компилируется и демонстрирует single-session flow для верификации issue `#347`.
In
`@packages/rust-browser-connection/experiments/test-version-check-dependencies.sh`:
- Around line 9-22: The script hardcodes a restore to issue-14-9d4fe6371f90 and
blindly suppresses errors, leaving the repo in an inconsistent state on
failures; update the flow in test-version-check-dependencies.sh to capture the
current branch (e.g. via git rev-parse --abbrev-ref HEAD) into an ORIG_BRANCH
variable, create a deterministically-named temporary branch
(test-deps-change-branch) only when needed, and register a trap on EXIT to
perform idempotent cleanup: checkout $ORIG_BRANCH only if it exists, restore
Cargo.toml from Cargo.toml.bak only if the backup exists, and delete the
temporary branch only if it was created; ensure errors are not suppressed
silently and that the check invocation (scripts/check-version-modification.mjs)
runs between creation and cleanup.
In `@packages/rust-browser-connection/experiments/test-version-check.sh`:
- Line 5: The script calls the wrong entrypoint: it invokes node
scripts/check-version-modification.mjs but the package provides a Rust
implementation scripts/check-version-modification.rs, causing
MODULE_NOT_FOUND/ENOENT. Update each invocation (the strings "node
scripts/check-version-modification.mjs") to run the Rust implementation instead
(for example replace with a Cargo invocation such as "cargo run --bin
check-version-modification" or the appropriate project-specific command that
runs scripts/check-version-modification.rs), ensuring all occurrences referenced
(the lines that call scripts/check-version-modification.mjs) are changed to the
Rust-run command so the experiment runs successfully.
- Around line 24-37: The script mutates git state unsafely: replace the
hardcoded branch/checkout and add robust cleanup by saving the current branch
(git rev-parse --abbrev-ref HEAD) and trapping EXIT/ERR to restore it, restore
the original Cargo.toml from a backup, and delete the temporary branch; ensure
the temp branch name (test-version-change-branch) is deterministic or randomized
to avoid collisions, create a Cargo.toml.bak before sed, and run the version
check (node scripts/check-version-modification.mjs) within this guarded section
so any failure triggers the trap that runs the cleanup steps.
In `@packages/rust-browser-connection/README.md`:
- Around line 1-3: README currently describes a different template and must be
replaced to reflect the actual package and PR purpose: rewrite the README for
the rust-browser-connection module to document the BrowserConnection API/CLI
(public structs/functions/classes and commands), state the single-session
invariant (how BrowserConnection enforces one active session), and include
concrete usage examples for the MCP and Hermes scenarios referenced in PR `#347`
(example CLI commands, minimal code snippets showing session creation/teardown,
error handling and configuration flags). Update the sections noted (lines ~6-7,
213-219, 283-291) to remove template references (e.g.,
"rust-ai-driven-development-pipeline-template" and "example-sum-package-name"),
replace with package name rust-browser-connection, list public entrypoints
(e.g., BrowserConnection::new, BrowserConnection::start_session,
BrowserConnection::stop_session or the actual function/class names used in the
crate), and add a short compatibility/contract note for integrators describing
expected behavior, versioning and migration notes.
In `@packages/rust-browser-connection/scripts/bump-version.rs`:
- Around line 127-128: The CLI `--rust-root` value is never passed into
rust_paths::get_rust_root (it currently calls get_rust_root(None, true)), so the
parsed argument is ignored; change the call to pass the parsed CLI value (the
variable that holds the `--rust-root` option) instead of None so
rust_paths::get_rust_root receives the user-specified path and rust_root is
resolved correctly; update the call site in bump-version.rs (the rust_root
assignment) to use the CLI variable name used when parsing `--rust-root` (e.g.,
the matches/value variable) and preserve the existing second argument (true).
- Around line 100-102: The current replacement uses the Regex stored in re and
blindly replaces the first "version = ..." in content when constructing
new_content, which can match outside the [package] section; update
bump-version.rs to target the package table specifically by either (a) parsing
content with a TOML-aware crate (e.g., toml_edit) and set package.version =
new_version, then write the serialized document back to new_content, or (b)
modify the regex to first locate the "[package]" header and only replace the
"version" key within that block (use a regex/search for
r"(?ms)^\[package\].*?^\s*version\s*=\s*\"[^\"]+\"" and replace the captured
value with new_version). Ensure you still reference new_version and produce
new_content from the modified document/string.
In `@packages/rust-browser-connection/scripts/check-changelog-fragment.rs`:
- Around line 64-77: The current implementation in get_changed_files uses "git
diff --name-only" which returns any modified/removed files; change the git
invocation to only list newly added files by using the diff filter (e.g. "git
diff --diff-filter=A --name-only origin/...") so only files added in the PR are
returned. Update the exec call in get_changed_files to include "--diff-filter=A"
(and likewise replace any other similar git diff invocation later in the file
that gathers changed filenames) so the changelog fragment check only considers
fragments newly added by the PR.
- Around line 80-90: The regex for detecting the "scripts/" path in
is_source_file() currently uses the literal "?scripts" token
(format!(r"^{}?scripts/", ...)) which produces an invalid regex when rust_root
== "." and causes a panic on Regex::new(...). Fix by building the scripts
pattern without the stray '?': ensure prefix already includes or omits the
trailing slash and use format!(r"^{}scripts/", regex::escape(&prefix)) (or
conditionally choose "^scripts/" when prefix is empty) inside the
source_patterns array so Regex::new(...) always receives a valid pattern.
In `@packages/rust-browser-connection/scripts/check-file-size.rs`:
- Around line 90-93: В текущем обходе директорий вызов
filter_map(std::result::Result::ok) подавляет ошибки WalkDir и может вызвать
ложный успех; замените подавление на явную обработку ошибок: при итерации
результата из WalkDir::new(cwd) (в месте с filter_map(Result::ok)) проверяйте
Result и для Err логируйте ошибку (или пишите в stderr) и завершайте процесс с
ненулевым кодом (или возвращайте Err из main), чтобы CI провалил прогон, а для
Ok продолжайте с file entries.
In `@packages/rust-browser-connection/scripts/check-release-needed.rs`:
- Around line 194-199: Функция docker_hub_image_to_check привязывает проверку
наличия Dockerfile к текущей рабочей директории; замените проверку
Path::new("Dockerfile").exists() на проверку относительно rust_root (например
проверять rust_root.join("Dockerfile").exists()) so that dockerhub_required
корректно определяется для multi-root проектов; аналогично исправьте другие
проверки на строках, упомянутых в комментарии (около 316-317) чтобы везде
использовать rust_root при поиске Dockerfile/путей.
- Around line 371-382: The branch where release_is_complete(...) returns true
currently calls set_output("should_release","false") but leaves skip_bump
implicit; update that branch (around the release_is_complete check in
check-release-needed.rs) to also call set_output("skip_bump","true") so both
outputs are always explicitly set when a release is complete, ensuring
determined downstream behavior; keep the existing message and other logic
unchanged.
In `@packages/rust-browser-connection/scripts/check-version-modification.rs`:
- Around line 51-67: The current should_skip_version_check function uses broad
prefixes in automated_branch_prefixes (e.g. "release/" and "automated-release/")
which lets any developer branch named release/* bypass the check; tighten the
whitelist by replacing those generic prefixes with the exact branch names or
strictly-patterned identifiers produced by automation (or a specific regex list)
and update the matching logic that checks head_ref to only skip when head_ref
exactly equals or strictly matches those automated branch identifiers; locate
and modify automated_branch_prefixes and the for-loop that checks head_ref in
should_skip_version_check to implement the stricter matching.
- Around line 111-120: The current has_version_change function uses a regex that
only matches literal version = "" and thus misses typical bumps like 0.2.0 ->
0.2.1; update the Regex in has_version_change to match any added/removed line
that assigns version (i.e., start-of-line + or - followed by optional
whitespace, the token "version", optional whitespace, =, optional whitespace,
then any quoted or unquoted version string or semantic-version characters) so
lines like "+version = \"0.2.1\"" or "- version=0.2.0" are detected; replace the
pattern on the Regex::new call accordingly and keep the rest of
has_version_change unchanged.
In `@packages/rust-browser-connection/scripts/collect-changelog.rs`:
- Around line 84-95: The get_version_from_cargo function currently scans the
entire Cargo.toml with a global regex and may grab a version from another table;
change it to only read the [package] table (or better: parse the file as TOML)
and extract package.version. Concretely, update get_version_from_cargo to either
(a) use a TOML parser to deserialize into a struct and return package.version,
or (b) first locate the "[package]" section and apply the version regex only
within that section; ensure the function still returns Result<String, String>
and keep error messages informative (e.g., "Could not find version in [package]
of Cargo.toml" or include parse errors) while preserving the existing function
name get_version_from_cargo and its signature.
In `@packages/rust-browser-connection/scripts/create-changelog-fragment.rs`:
- Around line 88-113: The code currently uses fs::write on fragment_file which
silently overwrites an existing fragment if the timestamp collides; replace the
direct fs::write call with logic that attempts to create the file using
OpenOptions::new().write(true).create_new(true).open(...) (referencing
fragment_file and fragment_content), and if it fails with AlreadyExists, retry
by appending a short unique suffix or counter to the filename (use timestamp +
bump_type + suffix pattern) until create_new succeeds or a max retry count is
reached; on other errors, log and exit as before. Ensure the directory creation
and error logging around create_dir_all, fragment_file and fragment_content
remain unchanged.
In `@packages/rust-browser-connection/scripts/create-github-release.rs`:
- Around line 325-339: The current check treats any "Validation Failed" as
"already exists", which masks other validation errors; change the conditional
around the output handling (variables output, stderr, stdout, combined) to only
skip when the API indicates a definite "already_exists" condition — e.g., parse
stderr/stdout as JSON and look for errors[].code == "already_exists" or a
message explicitly saying "release already exists" (or check for a specific 422
error with that code), and otherwise treat "Validation Failed" as a real
failure: log the full combined output (stderr/stdout) with eprintln and exit(1).
Ensure you replace the broad combined.contains("Validation Failed") branch in
the release creation logic with this targeted JSON/code check.
In `@packages/rust-browser-connection/scripts/git-config.rs`:
- Line 49: The current println! call prints sensitive PII by outputting the
variables name and email; update the logging at the println! invocation so it no
longer emits raw PII — either log a neutral message like "Configuring git user"
or mask the values before logging (e.g., redact email and name) and keep the
actual name/email usage for the git configuration logic that uses the name and
email variables; modify the println! that references name and email to use the
neutral or masked text instead.
In `@packages/rust-browser-connection/scripts/rust-paths.rs`:
- Around line 208-218: The loop that returns the first publishable crate
(iterating over members and using has_package_section/is_publish_false) is
unsafe because it silently picks one when multiple publishable members exist;
change rust-paths.rs to instead collect all candidate manifests into a Vec (use
the existing manifest variable and predicates), then enforce an invariant: if
vec.len() == 1 return that manifest, if 0 return an Err explaining "no
publishable crate found", and if >1 return an Err asking for an explicit target
(or read a provided env/arg like TARGET_CRATE and validate it against the
collected manifests); ensure errors include the list of candidates to aid the
caller.
- Around line 40-95: get_rust_root currently accepts explicit_root, CLI
--rust-root and RUST_ROOT as-is which allows paths like ../.. or absolute paths
to escape the repo; update get_rust_root to canonicalize and validate any
candidate root (explicit_root, CLI-supplied root, env var, or discovered
"."/"rust") using Path::new(...).canonicalize() and ensure the canonicalized
path is a descendant of the repository root (use
env::current_dir().canonicalize() as the repo root) and reject any path that is
outside that root (return Err). Reference get_rust_root, explicit_root, args
(CLI parsing) and env::var("RUST_ROOT") and perform the
normalization+containment check immediately after selecting a candidate before
returning Ok(...).
In `@packages/rust-browser-connection/scripts/version-and-commit.rs`:
- Around line 351-419: collect_changelog currently gathers markdown fragments
from changelog_dir into files and writes a new entry to changelog_file but never
removes the source fragments, causing repeated release bumps; after successfully
writing the updated changelog in collect_changelog (i.e., after
fs::write(changelog_file, content) succeeds), iterate the same files vector and
remove each fragment file (use fs::remove_file on each Path in files),
handling/remotely logging/remove-file errors gracefully (e.g., ignore non-fatal
errors or log them) so fragments are cleaned up and won't be reprocessed; ensure
you use the existing variables files and changelog_file and only delete after a
successful write.
- Around line 139-143: The current regex (re) replaces any "version ="
occurrence in the file (including dependency tables) and should be changed to
only update the package.version; fix this by parsing and updating the
Cargo.toml's [package] table (e.g., using the toml or toml_edit crate to load
content, set package.version = new_version, then serialize and write to
cargo_toml_path), or alternatively restrict the regex to operate only inside the
[package] section before creating new_content; update the code that computes
new_content (and keep variables content, new_version, cargo_toml_path) so only
the package.version is modified.
In `@packages/rust-browser-connection/src/lib.rs`:
- Around line 81-86: The test calls BrowserConnection::new() which requires a
live Docker daemon; instead construct a pure BrowserConnection value or call a
new test-only constructor so URL helpers can be tested without Docker. Modify
the test to use a lightweight constructor (e.g.,
BrowserConnection::from_base_url or BrowserConnection::with_mock) or directly
instantiate BrowserConnection with fake base values, then call
get_novnc_url("issue-347"), get_cdp_url("issue-347") and assert
is_single_browser_session(&cdp, &novnc); avoid calling BrowserConnection::new()
in the unit test.
- Around line 72-74: Метод is_single_browser_session неправильно считает
совпадение по наличию подстрок; вместо этого распарсьте cdp_url и novnc_url и
убедитесь, что они относятся к одной и той же сессии: сравните их origin (scheme
+ host + port) и/или явные идентификаторы сессии в пути/параметрах, и только
если origin совпадает и cdp_url содержит порт 9223, а novnc_url содержит путь
/vnc.html — возвращайте true; обновите функцию is_single_browser_session, чтобы
использовать URL-парсинг и чёткое сравнение origin/session-id вместо простого
contains.
- Around line 24-42: The port bindings in Config -> HostConfig -> port_bindings
currently bind CDP/noVNC/VNC to "0.0.0.0", which exposes ports 5900, 6080 and
9223 to the network; change the host_ip for the PortBinding entries for
"5900/tcp", "6080/tcp", and "9223/tcp" from "0.0.0.0" to "127.0.0.1" so they are
only accessible locally (update the PortBinding host_ip values where
port_bindings is defined in Config/HostConfig).
In `@packages/rust-browser-connection/src/main.rs`:
- Around line 5-18: The current CLI (struct Args, main and the sum usage)
implements a simple sum calculator which deviates from the PR goal for a
BrowserConnection module; replace or refactor this file to implement the
BrowserConnection entrypoint as specified: remove the ad-hoc Args/sum behavior
and instead wire up the BrowserConnection initializer that enforces the
single-session invariant and exposes the unified MCP/Hermes interface and
backends (noVNC/CDP/VNC) per the spec; ensure clear constructors/traits for
BrowserConnection, session lifecycle enforcement, configuration via CLI/env
parsing matching the spec, add unit/integration tests for single-session and
interface behavior, and update or add docs/comments to document the behavior and
security considerations rather than leaving the sample sum CLI.
In `@packages/rust-browser-connection/tests/unit/ci-cd/mod.rs`:
- Around line 8-10: Replace the broad #[allow(clippy::all, clippy::nursery,
clippy::pedantic, dead_code)] on the version_and_commit module with targeted
allows: remove clippy::all, clippy::nursery and clippy::pedantic and only keep
specific lints that actually appear for mod version_and_commit (for example keep
dead_code if needed, or allow particular lints such as clippy::needless_return
or clippy::missing_docs_in_private_items) — run cargo clippy to see the exact
warnings, then update the attribute to #[allow(...)] listing only those specific
clippy lints and/or dead_code instead of the blanket suppressions so mod
version_and_commit is no longer hiding unrelated clippy issues.
In
`@packages/rust-browser-connection/tests/unit/ci-cd/workspace_manifest_resolution.rs`:
- Around line 143-146: The test
rust_root_cli_parser_returns_none_without_configuration currently removes the
RUST_ROOT env var without restoring it, risking future flakiness; modify the
test to capture the original value of RUST_ROOT (e.g., via std::env::var_os or
var) before calling std::env::remove_var, then ensure the original value is
restored at the end of the test (use a drop guard or a defer-like closure/RAII
pattern) so parse_rust_root_from_args() is executed with RUST_ROOT removed but
the environment is returned to its prior state afterwards.
In `@packages/rust-browser-connection/tests/unit/mod.rs`:
- Around line 1-4: The unit test aggregator currently only includes `mod sum;`
and `mod ci_cd;` but lacks the unit module that verifies BrowserConnection
invariants; add and include a unit test module (e.g., `mod browser_connection;`
or `mod browser_connection_tests;`) that contains tests for the single-session
invariant (test name like `single_session_invariant`), the noVNC/CDP URL
formation helpers (tests for `make_no_vnc_url` and `make_cdp_url`), and
container start error handling (test for `start_container` error paths); update
the aggregator to `mod browser_connection;` so the CI will run these required
tests for issue `#347`.
In `@packages/rust-browser-connection/tests/unit/sum.rs`:
- Line 1: В тесте sum.rs импорт крейта указан как шаблонный плейсхолдер
`example_sum_package_name`, поэтому сборка упадёт; откройте Cargo.toml и
замените этот импорт на фактическое имя библиотечного крейта или, если тест
находится внутри того же крейта, переключитесь на внутренний путь (например
`crate::sum`) в строке, которая импортирует функцию `sum`, чтобы ссылка на
функцию `sum` была корректной.
In `@rust-browser-connection/src/lib.rs`:
- Around line 20-22: start_browser currently builds container_name and creates a
container without enforcing the single-session invariant;
is_single_browser_session only checks URL substrings so concurrent starts and
port conflicts are possible. Fix by making start_browser perform an atomic
existence/creation check: derive the exact container_name (use the same
project_id -> container_name logic), acquire a project-scoped lock (e.g.,
file/OS mutex or in-process async mutex) around the docker inspect/create
sequence, call Docker to inspect for an existing container with that exact name
and if running return its URL instead of creating a new one, otherwise create
the container with that name (letting Docker fail if the name already exists) or
explicitly remove/stop stale containers first; also ensure ports are either
dynamically allocated or checked for availability before binding to avoid
fixed-port conflicts; update is_single_browser_session to match exact
container_name or container label rather than substring URL checks so the
single-session invariant holds.
- Around line 80-83: The test test_urls_and_invariant currently calls
BrowserConnection::new().unwrap(), which depends on the local Docker daemon;
change the unit test to avoid that by constructing a BrowserConnection with a
deterministic base URL (e.g., add/use a constructor like
BrowserConnection::with_base_url or build the struct directly) and then call
BrowserConnection::get_novnc_url("issue-347") to assert the expected URL; move
the Docker-dependent BrowserConnection::new() usage into a separate integration
test that runs with Docker available.
- Around line 27-38: В текущем месте, где задаются PortBinding для портов
"5900/tcp", "6080/tcp" и "9223/tcp", host_ip жестко стоит "0.0.0.0" — нужно
заменить это на безопасный адрес по умолчанию (например "127.0.0.1") и/или
сделать адрес настраиваемым через переменную окружения (напр. BIND_ADDRESS) так,
чтобы вместо "0.0.0.0" в этих вызовах PortBinding использовалось
Some(bind_addr.to_string()); обновите код, который создает эти кортежи
("5900/tcp", ...), ("6080/tcp", ...), ("9223/tcp", ...) и добавьте
документированный конфиг/переменную окружения с безопасным значением по
умолчанию.
---
Outside diff comments:
In `@packages/lib/src/core/browser-connection.ts`:
- Around line 16-131: This file contains duplicate exported declarations
(BrowserError, BrowserConnection, BrowserConnectionLive and two default exports)
and uses forbidden constructs (`unknown`, `as const`, `as void`) and an
undeclared type (ProjectBrowserProxyPath). Consolidate into a single set of
exports: keep one BrowserError class (replace cause: unknown with cause?: Error
| undefined), one BrowserConnection interface (use parseProxyPath:
Effect.Effect<ProjectBrowserProxyPath | null, never>), one BrowserConnectionLive
implementation, and a single default export BrowserConnection; remove the
duplicate blocks and merge the intended runtime stubs
(startBrowser/getCdpUrl/getNoVncUrl/getVncUrl/parseProxyPath/rewriteCdpUrl) into
that single implementation. Replace `as const` and `as void` assertions with
proper typing/returns, and import or declare the ProjectBrowserProxyPath type at
the top so parseProxyPath compiles. Ensure rewriteCdpUrl/getCdpUrl
implementations remain consistent after consolidation.
In
`@packages/rust-browser-connection/docs/case-studies/issue-38/raw-data/issue-38.json`:
- Around line 1-2: Sanitize the case-study raw JSON by removing/obfuscating all
personal user fields inside author and comments.author blocks in the artifact
(e.g., the top-level "author" object and each "comments[].author"); specifically
strip or replace id, login, name, url and any other user-identifiers with
non-PII placeholders (e.g., "anon_user_1") while preserving non-PII fields
needed for analysis (timestamps, body, labels, title, number, state), and update
the code that writes issue-38.json to perform this transformation before saving
so generated artifacts never contain PII and thus satisfy the repository CI
credential/PII scanning rules.
In
`@packages/rust-browser-connection/docs/case-studies/issue-38/template-data/js-template-ci-tree.txt`:
- Around line 1-23: В файле js-template-ci-tree.txt найдено множество
зафиксированных абсолютных путей вида
/tmp/js-ai-driven-development-pipeline-template/..., которые делают артефакт
зависимым от окружения; замените все такие абсолютные пути на
репозиторно-относительные пути (например относительные от корня проекта или
шаблонные маркеры ./ или {repo_root}/), обновите перечисления в файле (каждую
строку начинающуюся с /tmp/js-ai-driven-development-pipeline-template/) чтобы не
включать /tmp префикс, и убедитесь, что после изменения сравнениe шаблон-данных
стабильно между машинами сборки.
🪄 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: ab128a98-5902-48ec-ac98-62d7a529be3c
⛔ Files ignored due to path filters (7)
packages/rust-browser-connection/Cargo.lockis excluded by!**/*.lockpackages/rust-browser-connection/docs/case-studies/issue-19/pr-114-data/solution-draft-log-1.txt.gzis excluded by!**/*.gzpackages/rust-browser-connection/docs/case-studies/issue-19/pr-114-data/solution-draft-log-2.txt.gzis excluded by!**/*.gzpackages/rust-browser-connection/docs/case-studies/issue-38/raw-data/downstream-meta-after-run-24985948212.log.gzis excluded by!**/*.gzpackages/rust-browser-connection/docs/case-studies/issue-38/raw-data/downstream-meta-before-run-24983875003.log.gzis excluded by!**/*.gzpackages/rust-browser-connection/docs/case-studies/issue-38/raw-data/main-run-24465255225.log.gzis excluded by!**/*.gzpackages/rust-browser-connection/docs/case-studies/issue-38/raw-data/pr-run-25212295127.log.gzis excluded by!**/*.gz
📒 Files selected for processing (133)
.hermes/plans/2026-05-23_092139-initial-formal-architecture-plan.md.hermes/plans/2026-05-23_092712-mcp-playwright-hermes-noVNC-integration.md.hermes/plans/2026-05-23_095118-clean-builtin-browser-noVNC-no-mcp.md.hermes/plans/2026-05-23_102144-rust-only-noVNC-browser-module-separate-repo.mdpackages/browser-connection/package.jsonpackages/browser-connection/src/index.tspackages/browser-connection/tsconfig.jsonpackages/lib/src/core/browser-connection.tspackages/rust-browser-connection/.github/workflows/release.ymlpackages/rust-browser-connection/.gitignorepackages/rust-browser-connection/.gitkeeppackages/rust-browser-connection/.pre-commit-config.yamlpackages/rust-browser-connection/CHANGELOG.mdpackages/rust-browser-connection/CONTRIBUTING.mdpackages/rust-browser-connection/Cargo.tomlpackages/rust-browser-connection/LICENSEpackages/rust-browser-connection/README.mdpackages/rust-browser-connection/changelog.d/20251227_224645_changeset_support.mdpackages/rust-browser-connection/changelog.d/20251229_143823_fix_ci_workflow_dependencies.mdpackages/rust-browser-connection/changelog.d/20251231_115800_fix_readme_script_references.mdpackages/rust-browser-connection/changelog.d/20260107_apply_best_practices.mdpackages/rust-browser-connection/changelog.d/20260108_171124_fix_changelog_check.mdpackages/rust-browser-connection/changelog.d/20260108_171435_prevent_manual_version_modification.mdpackages/rust-browser-connection/changelog.d/20260108_apply_lino_objects_codec_fixes.mdpackages/rust-browser-connection/changelog.d/20260111_multi_language_support.mdpackages/rust-browser-connection/changelog.d/20260119_best_practices_from_browser_commander.mdpackages/rust-browser-connection/changelog.d/20260311_translate_scripts_to_rust.mdpackages/rust-browser-connection/changelog.d/20260413-ci-cd-best-practices.mdpackages/rust-browser-connection/changelog.d/20260413_fix_cargo_token_fallback.mdpackages/rust-browser-connection/changelog.d/20260413_fix_crates_io_check.mdpackages/rust-browser-connection/changelog.d/20260413_fix_lookahead_regex.mdpackages/rust-browser-connection/changelog.d/20260414_fix_per_commit_diff.mdpackages/rust-browser-connection/changelog.d/20260415_fix_workspace_release_scripts.mdpackages/rust-browser-connection/changelog.d/20260501_decouple_docs_deploy.mdpackages/rust-browser-connection/changelog.d/20260503_111500_ci_timeouts.mdpackages/rust-browser-connection/changelog.d/20260503_111700_file_size_warning_threshold.mdpackages/rust-browser-connection/changelog.d/20260509_031015_human_readable_release_titles.mdpackages/rust-browser-connection/changelog.d/20260509_205000_docker_hub_release_publishing.mdpackages/rust-browser-connection/changelog.d/20260512_172908_github_pages_artifact_deploy.mdpackages/rust-browser-connection/changelog.d/20260512_230053_document_pages_source_prerequisite.mdpackages/rust-browser-connection/changelog.d/20260515_074404_track_browser_commander_preview_regen.mdpackages/rust-browser-connection/changelog.d/20260515_223000_cargo_lock_release_sync.mdpackages/rust-browser-connection/changelog.d/README.mdpackages/rust-browser-connection/docs/case-studies/issue-11/README.mdpackages/rust-browser-connection/docs/case-studies/issue-11/analysis-crates-io.mdpackages/rust-browser-connection/docs/case-studies/issue-11/analysis-set-output.mdpackages/rust-browser-connection/docs/case-studies/issue-11/analysis-workflow-dispatch.mdpackages/rust-browser-connection/docs/case-studies/issue-11/online-research.mdpackages/rust-browser-connection/docs/case-studies/issue-17/README.mdpackages/rust-browser-connection/docs/case-studies/issue-19/README.mdpackages/rust-browser-connection/docs/case-studies/issue-19/pr-114-data/issue-113-details.txtpackages/rust-browser-connection/docs/case-studies/issue-19/pr-114-data/pr-commits.jsonpackages/rust-browser-connection/docs/case-studies/issue-19/pr-114-data/pr-conversation-comments.jsonpackages/rust-browser-connection/docs/case-studies/issue-19/pr-114-data/pr-details.jsonpackages/rust-browser-connection/docs/case-studies/issue-19/pr-114-data/pr-diff.patchpackages/rust-browser-connection/docs/case-studies/issue-19/pr-114-data/pr-review-comments.jsonpackages/rust-browser-connection/docs/case-studies/issue-19/pr-114-data/pr-reviews.jsonpackages/rust-browser-connection/docs/case-studies/issue-21/README.mdpackages/rust-browser-connection/docs/case-studies/issue-21/browser-commander-issue-27.mdpackages/rust-browser-connection/docs/case-studies/issue-21/browser-commander-issue-29.mdpackages/rust-browser-connection/docs/case-studies/issue-21/browser-commander-issue-31.mdpackages/rust-browser-connection/docs/case-studies/issue-21/browser-commander-issue-33.mdpackages/rust-browser-connection/docs/case-studies/issue-21/browser-commander-rust.ymlpackages/rust-browser-connection/docs/case-studies/issue-25/README.mdpackages/rust-browser-connection/docs/case-studies/issue-29/README.mdpackages/rust-browser-connection/docs/case-studies/issue-32/README.mdpackages/rust-browser-connection/docs/case-studies/issue-34/README.mdpackages/rust-browser-connection/docs/case-studies/issue-38/README.mdpackages/rust-browser-connection/docs/case-studies/issue-38/raw-data/downstream-meta-after-run-24985948212.jsonpackages/rust-browser-connection/docs/case-studies/issue-38/raw-data/downstream-meta-before-run-24983875003.jsonpackages/rust-browser-connection/docs/case-studies/issue-38/raw-data/downstream-meta-ontology-issue-3.jsonpackages/rust-browser-connection/docs/case-studies/issue-38/raw-data/downstream-meta-ontology-pr-4.jsonpackages/rust-browser-connection/docs/case-studies/issue-38/raw-data/issue-38-comments.jsonpackages/rust-browser-connection/docs/case-studies/issue-38/raw-data/issue-38.jsonpackages/rust-browser-connection/docs/case-studies/issue-38/raw-data/js-template-issue-search.jsonpackages/rust-browser-connection/docs/case-studies/issue-38/raw-data/main-run-24465255225.jsonpackages/rust-browser-connection/docs/case-studies/issue-38/raw-data/main-runs.jsonpackages/rust-browser-connection/docs/case-studies/issue-38/raw-data/pr-39-conversation-comments.jsonpackages/rust-browser-connection/docs/case-studies/issue-38/raw-data/pr-39-review-comments.jsonpackages/rust-browser-connection/docs/case-studies/issue-38/raw-data/pr-39-reviews.jsonpackages/rust-browser-connection/docs/case-studies/issue-38/raw-data/pr-39.jsonpackages/rust-browser-connection/docs/case-studies/issue-38/raw-data/pr-branch-runs.jsonpackages/rust-browser-connection/docs/case-studies/issue-38/raw-data/pr-run-25212295127.jsonpackages/rust-browser-connection/docs/case-studies/issue-38/raw-data/rust-template-issue-search.jsonpackages/rust-browser-connection/docs/case-studies/issue-38/template-data/js-template-ci-tree.txtpackages/rust-browser-connection/docs/case-studies/issue-38/template-data/js-template-links.ymlpackages/rust-browser-connection/docs/case-studies/issue-38/template-data/js-template-release.ymlpackages/rust-browser-connection/docs/case-studies/issue-38/template-data/rust-template-ci-tree.txtpackages/rust-browser-connection/docs/case-studies/issue-38/template-data/rust-template-release-after.ymlpackages/rust-browser-connection/docs/case-studies/issue-38/template-data/rust-template-release-before.ymlpackages/rust-browser-connection/docs/case-studies/issue-52/README.mdpackages/rust-browser-connection/docs/case-studies/issue-52/raw-data/issue-52-comments.jsonpackages/rust-browser-connection/docs/case-studies/issue-52/raw-data/issue-52.jsonpackages/rust-browser-connection/docs/case-studies/issue-52/raw-data/js-issue-62.jsonpackages/rust-browser-connection/docs/case-studies/issue-52/raw-data/vk-bot-desktop-issue-51.jsonpackages/rust-browser-connection/docs/case-studies/issue-52/raw-data/vk-bot-desktop-pr-52.jsonpackages/rust-browser-connection/docs/ci-cd/troubleshooting.mdpackages/rust-browser-connection/examples/basic_usage.rspackages/rust-browser-connection/experiments/test-changelog-parsing.rspackages/rust-browser-connection/experiments/test-crates-io-check.rspackages/rust-browser-connection/experiments/test-detect-code-changes.shpackages/rust-browser-connection/experiments/test-version-check-dependencies.shpackages/rust-browser-connection/experiments/test-version-check.shpackages/rust-browser-connection/scripts/bump-version.rspackages/rust-browser-connection/scripts/check-changelog-fragment.rspackages/rust-browser-connection/scripts/check-file-size.rspackages/rust-browser-connection/scripts/check-release-needed.rspackages/rust-browser-connection/scripts/check-version-modification.rspackages/rust-browser-connection/scripts/collect-changelog.rspackages/rust-browser-connection/scripts/create-changelog-fragment.rspackages/rust-browser-connection/scripts/create-github-release.rspackages/rust-browser-connection/scripts/detect-code-changes.rspackages/rust-browser-connection/scripts/get-bump-type.rspackages/rust-browser-connection/scripts/get-version.rspackages/rust-browser-connection/scripts/git-config.rspackages/rust-browser-connection/scripts/publish-crate.rspackages/rust-browser-connection/scripts/rust-paths.rspackages/rust-browser-connection/scripts/version-and-commit.rspackages/rust-browser-connection/scripts/wait-for-crate.rspackages/rust-browser-connection/src/lib.rspackages/rust-browser-connection/src/main.rspackages/rust-browser-connection/src/sum.rspackages/rust-browser-connection/tests/integration/mod.rspackages/rust-browser-connection/tests/integration/sum.rspackages/rust-browser-connection/tests/unit/ci-cd/changelog_parsing.rspackages/rust-browser-connection/tests/unit/ci-cd/mod.rspackages/rust-browser-connection/tests/unit/ci-cd/workflow_release.rspackages/rust-browser-connection/tests/unit/ci-cd/workspace_manifest_resolution.rspackages/rust-browser-connection/tests/unit/mod.rspackages/rust-browser-connection/tests/unit/sum.rspnpm-workspace.yamlrust-browser-connection/Cargo.tomlrust-browser-connection/src/lib.rs
Comment on lines
+35
to
+43
| "devDependencies": { | ||
| "@effect/vitest": "^0.29.0", | ||
| "@types/node": "^25.9.1", | ||
| "typescript": "^6.0.3", | ||
| "vitest": "^4.1.7" | ||
| }, | ||
| "dependencies": { | ||
| "effect": "^3.12.0" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import json, urllib.request, urllib.parse
packages = {
"`@effect/vitest`": "0.29.0",
"`@types/node`": "25.9.1",
"typescript": "6.0.3",
"vitest": "4.1.7",
"effect": "3.12.0",
}
for name, version in packages.items():
url = "https://registry.npmjs.org/" + urllib.parse.quote(name, safe='@')
with urllib.request.urlopen(url) as r:
data = json.load(r)
exists = version in data.get("versions", {})
print(f"{name}@{version}: {'OK' if exists else 'MISSING'}")
PYRepository: ProverCoderAI/docker-git
Length of output: 2651
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Target file =="
if [ -f "packages/browser-connection/package.json" ]; then
nl -ba packages/browser-connection/package.json | sed -n '1,120p'
else
echo "MISSING: packages/browser-connection/package.json"
fi
echo
echo "== Check for lockfiles near this package =="
ls -la packages/browser-connection 2>/dev/null || true
find packages/browser-connection -maxdepth 2 -type f \( -name 'package-lock.json' -o -name 'pnpm-lock.yaml' -o -name 'yarn.lock' \) -print 2>/dev/null || true
echo
echo "== Check repo root for lockfiles =="
ls -la 2>/dev/null || true
find . -maxdepth 3 -type f \( -name 'package-lock.json' -o -name 'pnpm-lock.yaml' -o -name 'yarn.lock' -o -name 'bun.lockb' \) -print 2>/dev/null || trueRepository: ProverCoderAI/docker-git
Length of output: 147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check file existence =="
ls -la packages/browser-connection/package.json 2>/dev/null || echo "MISSING: packages/browser-connection/package.json"
echo
echo "== Print first ~120 lines of package.json (if exists) =="
if [ -f "packages/browser-connection/package.json" ]; then
cat -n packages/browser-connection/package.json | sed -n '1,140p'
fi
echo
echo "== List files in packages/browser-connection (maxdepth 2) =="
find packages/browser-connection -maxdepth 2 -type f -print 2>/dev/null || true
echo
echo "== Search for lockfiles near this package =="
find packages/browser-connection -maxdepth 4 -type f \( -name 'package-lock.json' -o -name 'pnpm-lock.yaml' -o -name 'yarn.lock' -o -name 'bun.lockb' \) -print 2>/dev/null || true
echo
echo "== Search for lockfiles in repo (maxdepth 4) =="
find . -maxdepth 4 -type f \( -name 'package-lock.json' -o -name 'pnpm-lock.yaml' -o -name 'yarn.lock' -o -name 'bun.lockb' \) -print 2>/dev/null || trueRepository: ProverCoderAI/docker-git
Length of output: 1965
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Lockfiles anywhere in repo (bun/yarn/pnpm/npm) =="
find . -type f \( -name 'bun.lockb' -o -name 'pnpm-lock.yaml' -o -name 'yarn.lock' -o -name 'package-lock.json' \) -print || true
echo
echo "== git diff for target package.json (if git history present) =="
git diff -- packages/browser-connection/package.json || true
echo
echo "== Search other package.json files for packageManager/bun/pnpm =="
rg -n "\"packageManager\"|pnpm-lock.yaml|bun.lockb" --glob 'packages/**/package.json' . || trueRepository: ProverCoderAI/docker-git
Length of output: 612
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Root README(s) =="
ls -la README* 2>/dev/null || true
find . -maxdepth 3 -type f -name 'README*.md' -print 2>/dev/null || true
echo
echo "== Search in README/MD for lockfile / pinning policy =="
rg -n --hidden -S "bun\.lockb|lockfile|pin(n|ning)|^\\^|caret|reproduc|reproducib|dependency versions|version policy" --glob '**/*.md' . || true
echo
echo "== Search for publishing steps (npm publish / bun publish) =="
rg -n --hidden -S "npm publish|pnpm publish|yarn publish|bun publish|publishConfig|prepack|files\" *: *\\[ *\"dist\"\\]|`@prover-coder-ai/browser-connection`" . || true
echo
echo "== Search for bun.lockb mentions in non-MD files =="
rg -n --hidden -S "bun\.lockb" . || trueRepository: ProverCoderAI/docker-git
Length of output: 11398
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find lockfiles anywhere in repo (no maxdepth) =="
find . -type f \( -name 'bun.lockb' -o -name 'pnpm-lock.yaml' -o -name 'yarn.lock' -o -name 'package-lock.json' -o -name 'npm-shrinkwrap.json' \) -print || true
echo
echo "== Inspect release workflow publish section =="
if [ -f ".github/workflows/release.yml" ]; then
sed -n '330,460p' .github/workflows/release.yml
else
echo "MISSING: .github/workflows/release.yml"
fiRepository: ProverCoderAI/docker-git
Length of output: 5660
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== git status --porcelain =="
git status --porcelain || true
echo
echo "== git diff --stat =="
git diff --stat || true
echo
echo "== git diff --name-only =="
git diff --name-only || true
echo
echo "== git diff packages/browser-connection/package.json (first 40 lines) =="
git diff -- packages/browser-connection/package.json | head -n 40 || trueRepository: ProverCoderAI/docker-git
Length of output: 222
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== git status --porcelain =="
git status --porcelain || true
echo
echo "== git diff --stat =="
git diff --stat || true
echo
echo "== git diff --name-only =="
git diff --name-only || true
echo
echo "== git diff packages/browser-connection/package.json (first 40 lines) =="
git diff -- packages/browser-connection/package.json | head -n 40 || trueRepository: ProverCoderAI/docker-git
Length of output: 222
Зафиксируйте версии зависимостей в публикуемом пакете @prover-coder-ai/browser-connection
В packages/browser-connection/package.json зависимости указаны с caret-диапазонами (^), например effect, typescript, vitest, что ухудшает воспроизводимость и расширяет supply-chain поверхность для потребителей (публикация идёт через bun publish без lockfile-фиксации).
Предлагаемое изменение
"devDependencies": {
- "`@effect/vitest`": "^0.29.0",
- "`@types/node`": "^25.9.1",
- "typescript": "^6.0.3",
- "vitest": "^4.1.7"
+ "`@effect/vitest`": "0.29.0",
+ "`@types/node`": "25.9.1",
+ "typescript": "6.0.3",
+ "vitest": "4.1.7"
},
"dependencies": {
- "effect": "^3.12.0"
+ "effect": "3.12.0"
}📝 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.
Suggested change
| "devDependencies": { | |
| "@effect/vitest": "^0.29.0", | |
| "@types/node": "^25.9.1", | |
| "typescript": "^6.0.3", | |
| "vitest": "^4.1.7" | |
| }, | |
| "dependencies": { | |
| "effect": "^3.12.0" | |
| } | |
| "devDependencies": { | |
| "`@effect/vitest`": "0.29.0", | |
| "`@types/node`": "25.9.1", | |
| "typescript": "6.0.3", | |
| "vitest": "4.1.7" | |
| }, | |
| "dependencies": { | |
| "effect": "3.12.0" | |
| } |
🤖 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/package.json` around lines 35 - 43, В
package.json пакета `@prover-coder-ai/browser-connection` зафиксируйте версии
зависимостей — замените caret-диапазоны в полях "dependencies" и
"devDependencies" на конкретные версии (удалите "^" у effect, typescript,
vitest, `@effect/vitest`, `@types/node` и т.д.) чтобы публикуемый пакет через bun
publish содержал точные версии и не расширял supply-chain поверхность для
потребителей.
Comment on lines
+3
to
+15
| export class BrowserError { | ||
| readonly _tag = "BrowserError" as const | ||
| constructor(readonly message: string, readonly cause?: unknown) {} | ||
| } | ||
|
|
||
| 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.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and display the target file with line numbers (bounded output)
FILE="packages/browser-connection/src/index.ts"
if [ -f "$FILE" ]; then
echo "=== $FILE (first 200 lines) ==="
nl -ba "$FILE" | sed -n '1,200p'
else
echo "File not found: $FILE" >&2
exit 1
fi
# Show any other occurrences of BrowserError / parseProxyPath / rewriteCdpUrl in the repo
rg -n --hidden --no-ignore -S "class BrowserError|interface BrowserConnection|parseProxyPath|rewriteCdpUrl" .Repository: ProverCoderAI/docker-git
Length of output: 196
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/browser-connection/src/index.ts"
echo "=== existence/state ==="
ls -la "$FILE" 2>/dev/null || { echo "File not found: $FILE" >&2; exit 1; }
wc -l "$FILE"
echo
echo "=== content (up to 200 lines) ==="
# cat -n is generally available; fallback to sed
if command -v cat >/dev/null 2>&1; then
cat -n "$FILE" | sed -n '1,200p'
else
sed -n '1,200p' "$FILE"
fi
echo
echo "=== repo search for key symbols ==="
rg -n --hidden --no-ignore -S "class BrowserError|interface BrowserConnection|parseProxyPath|rewriteCdpUrl" .Repository: ProverCoderAI/docker-git
Length of output: 4893
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/lib/src/core/browser-connection.ts"
ls -la "$FILE"
wc -l "$FILE"
echo "=== content (first 180 lines) ==="
sed -n '1,180p' "$FILE" | nl -ba | sed -n '1,180p'
echo "=== search for key type names ==="
rg -n "ProjectBrowserProxyPath|parseProxyPath|rewriteCdpUrl|BrowserError|_tag" "$FILE"Repository: ProverCoderAI/docker-git
Length of output: 304
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/lib/src/core/browser-connection.ts"
echo "=== content (first 200 lines) ==="
cat -n "$FILE" | sed -n '1,220p'
echo "=== search for key symbols/types in core file ==="
rg -n --hidden --no-ignore -S "export class BrowserError|interface BrowserConnection|ProjectBrowserProxyPath|parseProxyPath|rewriteCdpUrl|_tag" "$FILE"Repository: ProverCoderAI/docker-git
Length of output: 8922
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden --no-ignore -S "type ProjectBrowserProxyPath|interface ProjectBrowserProxyPath|enum ProjectBrowserProxyPath|export (type|interface|enum) ProjectBrowserProxyPath" packages/lib/src/core/browser-connection.ts
rg -n --hidden --no-ignore -S "ProjectBrowserProxyPath" packages/lib/src/core/browser-connection.tsRepository: ProverCoderAI/docker-git
Length of output: 50
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/lib/src/core/browser-connection.ts"
echo "=== imports / top (1-40) ==="
sed -n '1,60p' "$FILE" | cat -n
echo "=== around second BrowserConnection interface (60-110) ==="
sed -n '60,120p' "$FILE" | cat -n
echo "=== all occurrences of 'ProjectBrowserProxyPath' in the file ==="
rg -n --hidden --no-ignore "ProjectBrowserProxyPath" "$FILE" || true
echo "=== search repo for ProjectBrowserProxyPath definition ==="
rg -n --hidden --no-ignore "ProjectBrowserProxyPath" packages/lib/src/core packages/lib/src -SRepository: ProverCoderAI/docker-git
Length of output: 8072
Исправить публичный TS-контракт BrowserConnection в packages/browser-connection/src/index.ts: убрать as/unknown из API
BrowserError:_tag = "BrowserError" as constиcause?: unknown(строки 4-5) нарушают запреты наasиunknownв типах контракта.startBrowser:return undefined as void(строки 23-27) — запрещённыйasв обычном коде.parseProxyPath:Effect.Effect<unknown, never>(строка 13) и реализацияEffect.succeed(null)(строка 31) должны быть типизированы доменно (согласовать с CORE, гдеparseProxyPathобъявлен какEffect.Effect<ProjectBrowserProxyPath | null, never>).- Экспортируемые сущности (
BrowserError,BrowserConnection) не имеют обязательных TSDoc/functional comment markers.
🤖 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 - 15, Fix the public
TS contract by removing "as" casts and "unknown" from exported types: change
BrowserError to use a plain string literal tag (no "as const") and give cause a
concrete domain/error type (not unknown), update
BrowserConnection.parseProxyPath to return Effect.Effect<ProjectBrowserProxyPath
| null, never> (and replace Effect.succeed(null) with the typed success), and
stop using "as void" in startBrowser implementation—return a proper Effect like
Effect.unit or Effect.succeedVoid instead; also add minimal TSDoc/functional
comment markers above the exported BrowserError and BrowserConnection to
document the public API. Ensure you reference and update the symbols
BrowserError, BrowserConnection, startBrowser, and parseProxyPath in the file.
Comment on lines
+3
to
+47
| export class BrowserError { | ||
| readonly _tag = "BrowserError" as const | ||
| constructor(readonly message: string, readonly cause?: unknown) {} | ||
| } | ||
|
|
||
| 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 | ||
| } | ||
|
|
||
| export const BrowserConnection = Context.GenericTag<BrowserConnection>("@prover-coder-ai/browser-connection/BrowserConnection") | ||
|
|
||
| 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 | ||
| } | ||
| }) | ||
| ) | ||
|
|
||
| // 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") | ||
|
|
||
| export default BrowserConnection |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/browser-connection/src/index.ts"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
echo "== File: $FILE =="
wc -l "$FILE"
echo
echo "== Exported symbols + nearby lines (1-200) =="
nl -ba "$FILE" | sed -n '1,200p'
echo
echo "== TSDoc marker occurrences =="
rg -n "CHANGE|WHY|QUOTE\\(ТЗ\\)|REF\\b|SOURCE\\b|FORMAT THEOREM|PURITY\\b|EFFECT\\b|INVARIANT\\b|COMPLEXITY\\b|`@pure`\\b|`@effect`\\b|`@invariant`\\b|`@precondition`\\b|`@postcondition`\\b|`@complexity`\\b|`@throws`\\b|`@invariant`\\b" "$FILE" || true
echo
echo "== Exported declarations without leading /** =="
python3 - <<'PY'
import re, pathlib
p = pathlib.Path("packages/browser-connection/src/index.ts")
s = p.read_text(encoding='utf-8')
# Heuristic: exported ... lines and ensure preceding non-empty line is /** ... */
lines = s.splitlines()
export_re = re.compile(r'^\s*(export\s+(default\s+)?(class|interface|const|function|type|enum)\b|export\s*{\b)')
# For each export line, check last preceding line that's non-empty.
for i, line in enumerate(lines, start=1):
if export_re.search(line):
j=i-1
while j>=1 and lines[j-1].strip()=="":
j-=1
prev=lines[j-1] if j-1>=0 else ""
# if prev doesn't start with /** or *
ok = prev.strip().startswith("*/") or prev.strip().startswith("*") or prev.strip().startswith("/**")
# If prev starts with "*" or "/**" it's likely part of block.
# If immediately previous non-empty line isn't doc, report.
if not prev.strip().startswith("*/") and not prev.strip().startswith("*") and not prev.strip().startswith("/**"):
print(f"{i}: {line.strip()}")
PYRepository: ProverCoderAI/docker-git
Length of output: 274
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/browser-connection/src/index.ts"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
echo "== File: $FILE =="
wc -l "$FILE"
echo
echo "== File excerpt (1-120) =="
sed -n '1,120p' "$FILE" | cat -n
echo
echo "== TSDoc/marker occurrences =="
rg -n "CHANGE|WHY|QUOTE\\(ТЗ\\)|REF\\b|SOURCE\\b|FORMAT THEOREM|PURITY\\b|EFFECT\\b|INVARIANT\\b|COMPLEXITY\\b|`@pure`\\b|`@effect`\\b|`@invariant`\\b|`@precondition`\\b|`@postcondition`\\b|`@complexity`\\b|`@throws`\\b|`@throws`\\b" "$FILE" || true
echo
echo "== Exported declarations likely missing leading /** =="
python3 - <<'PY'
import re, pathlib
p = pathlib.Path("packages/browser-connection/src/index.ts")
s = p.read_text(encoding='utf-8')
lines = s.splitlines()
export_re = re.compile(r'^\s*export\s+(default\s+)?(class|interface|const|function|type|enum)\b')
# also handle export default BrowserConnection; not relevant
results=[]
for i,line in enumerate(lines, start=1):
if export_re.search(line):
j=i-1
while j>=2 and lines[j-2].strip()=="":
j-=1
prev = lines[j-2] if j-2>=0 else ""
if not (prev.strip().startswith("/**") or prev.strip().startswith("*") or prev.strip().startswith("*/")):
results.append((i,line.strip()))
for i,line in results:
print(f"{i}: {line}")
PYRepository: ProverCoderAI/docker-git
Length of output: 3247
Не хватает обязательной функциональной TSDoc для экспортируемых типов и helper-функций (packages/browser-connection/src/index.ts)
Для BrowserError, BrowserConnection, BrowserConnectionLive, а также renderNoVncUrl, renderCdpUrl, isSingleBrowserSession отсутствует требуемая функциональная документация и маркеры (CHANGE/WHY/INVARIANT/COMPLEXITY/PURITY/EFFECT и т.д.) в TSDoc-формате по coding guidelines.
🤖 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 - 47, Add required
functional TSDoc comments for all exported symbols: BrowserError,
BrowserConnection, BrowserConnectionLive, renderNoVncUrl, renderCdpUrl, and
isSingleBrowserSession. For each exported type/class/function place a TSDoc
block immediately above its declaration and include at minimum: a short
description, and the required markers such as `@CHANGE` or `@WHY` (purpose),
`@INVARIANT` (behavior guarantees), `@COMPLEXITY` (complexity/cost), and
`@PURITY/`@EFFECT as applicable; for BrowserConnectionLive document side-effects
and environment wiring with `@EFFECT`, for helpers document purity with `@PURITY`
and examples/usage where helpful. Ensure tags and descriptions follow the
project coding-guidelines format and appear for every exported symbol listed.
Comment on lines
+23
to
+33
| 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.
BrowserConnectionLive сейчас не реализует заявленный runtime-контракт.
startBrowser не поднимает сессию, parseProxyPath всегда null, rewriteCdpUrl игнорирует аргументы, getVncUrl не учитывает projectId. Это расходится с целью PR про управляемое подключение к единственной browser-сессии.
🤖 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 23 - 33,
BrowserConnectionLive currently doesn't implement the runtime contract:
implement startBrowser to actually start/record a single browser session for the
given projectId (create/store session state and log errors instead of returning
undefined), implement parseProxyPath to parse the incoming pathname and return a
project id or session descriptor instead of always null, make rewriteCdpUrl use
the passed externalOrigin and projectId to rewrite the CDP URL correctly (not
just return value), and make getVncUrl include projectId (and any per-project
port or path mapping) rather than returning a constant; update the
implementations in BrowserConnectionLive (startBrowser, parseProxyPath,
rewriteCdpUrl, getVncUrl) so they coordinate with whatever session
storage/registry the runtime uses to ensure a single managed browser session per
project.
Comment on lines
+1
to
+4
| mod sum; | ||
|
|
||
| #[path = "ci-cd/mod.rs"] | ||
| mod ci_cd; |
There was a problem hiding this comment.
В unit-агрегаторе нет тестов для BrowserConnection-инвариантов из ТЗ.
Подключены только sum и ci_cd, но не видно unit-модуля, который проверяет заявленные в PR свойства (single-session invariant, формирование noVNC/CDP URL, ошибки старта контейнера). Для issue #347 это обязательный проверяемый контракт.
As per coding guidelines, "Флагай ... отсутствие тестов для заявленного поведения ..."
🤖 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/rust-browser-connection/tests/unit/mod.rs` around lines 1 - 4, The
unit test aggregator currently only includes `mod sum;` and `mod ci_cd;` but
lacks the unit module that verifies BrowserConnection invariants; add and
include a unit test module (e.g., `mod browser_connection;` or `mod
browser_connection_tests;`) that contains tests for the single-session invariant
(test name like `single_session_invariant`), the noVNC/CDP URL formation helpers
(tests for `make_no_vnc_url` and `make_cdp_url`), and container start error
handling (test for `start_container` error paths); update the aggregator to `mod
browser_connection;` so the CI will run these required tests for issue `#347`.
| @@ -0,0 +1,26 @@ | |||
| use example_sum_package_name::sum; | |||
There was a problem hiding this comment.
Неверный путь импорта крейта в тесте
example_sum_package_name выглядит как шаблонный плейсхолдер; в таком виде тесты, вероятно, не соберутся. Замените импорт на фактическое имя library-крейта из Cargo.toml (или используйте crate::..., если уместно для структуры тестов).
Предлагаемая правка
-use example_sum_package_name::sum;
+use rust_browser_connection::sum;📝 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.
Suggested change
| use example_sum_package_name::sum; | |
| use rust_browser_connection::sum; |
🤖 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/rust-browser-connection/tests/unit/sum.rs` at line 1, В тесте sum.rs
импорт крейта указан как шаблонный плейсхолдер `example_sum_package_name`,
поэтому сборка упадёт; откройте Cargo.toml и замените этот импорт на фактическое
имя библиотечного крейта или, если тест находится внутри того же крейта,
переключитесь на внутренний путь (например `crate::sum`) в строке, которая
импортирует функцию `sum`, чтобы ссылка на функцию `sum` была корректной.
Comment on lines
+20
to
+22
| pub async fn start_browser(&self, project_id: &str) -> Result<String> { | ||
| let container_name = format!("dg-{}-browser", project_id.replace('/', '-')); | ||
|
|
There was a problem hiding this comment.
Инвариант single-session не обеспечен, только декларируется.
start_browser создает контейнер по project_id, а is_single_browser_session проверяет лишь подстроки URL. Это не предотвращает конкурирующие запуски и конфликты по фиксированным портам, хотя PR заявляет single-session поведение.
Also applies to: 50-58, 71-73
🤖 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 `@rust-browser-connection/src/lib.rs` around lines 20 - 22, start_browser
currently builds container_name and creates a container without enforcing the
single-session invariant; is_single_browser_session only checks URL substrings
so concurrent starts and port conflicts are possible. Fix by making
start_browser perform an atomic existence/creation check: derive the exact
container_name (use the same project_id -> container_name logic), acquire a
project-scoped lock (e.g., file/OS mutex or in-process async mutex) around the
docker inspect/create sequence, call Docker to inspect for an existing container
with that exact name and if running return its URL instead of creating a new
one, otherwise create the container with that name (letting Docker fail if the
name already exists) or explicitly remove/stop stale containers first; also
ensure ports are either dynamically allocated or checked for availability before
binding to avoid fixed-port conflicts; update is_single_browser_session to match
exact container_name or container label rather than substring URL checks so the
single-session invariant holds.
Comment on lines
+32
to
+46
| 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.
Реализация не обеспечивает заявленный инвариант единой браузерной сессии из ТЗ/целей PR.
startBrowser и URL-методы — заглушки без проверки/связывания с фактическим контейнером dg-*-browser, поэтому требование “single unified browser session via CDP + noVNC” не доказуемо.
Also applies to: 89-103
🤖 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/browser-connection.ts` around lines 32 - 46, The
current BrowserConnectionLive implementation provides stubbed startBrowser and
URL methods that are not tied to the actual dg-*-browser container, so it does
not enforce the required single unified browser session invariant; update
BrowserConnectionLive so startBrowser negotiates/creates or looks up a single
shared browser container/session (e.g., by calling the container manager or
session registry), store and reuse the session handle, and make getCdpUrl,
getNoVncUrl, getVncUrl, parseProxyPath and rewriteCdpUrl derive their values
from that stored session (failing or refreshing the session if missing/expired)
instead of returning hardcoded values; ensure rewriteCdpUrl performs
origin-aware rewriting using the real external origin and stored project/session
metadata.
Comment on lines
+478
to
+483
| - name: Version and commit | ||
| id: version | ||
| env: | ||
| BUMP_TYPE: ${{ github.event.inputs.bump_type }} | ||
| DESCRIPTION: ${{ github.event.inputs.description }} | ||
| run: rust-script scripts/version-and-commit.rs --bump-type "${{ github.event.inputs.bump_type }}" --description "${{ github.event.inputs.description }}" |
There was a problem hiding this comment.
Не подставляйте workflow_dispatch inputs прямо в shell-команду.
github.event.inputs.description и другие inputs интерполируются до запуска shell, поэтому значения вроде $(...) или обратных кавычек будут выполнены на runner'е. Здесь уже объявлены безопасные env, их и нужно использовать.
💡 Безопасный вариант
- run: rust-script scripts/version-and-commit.rs --bump-type "${{ github.event.inputs.bump_type }}" --description "${{ github.event.inputs.description }}"
+ run: rust-script scripts/version-and-commit.rs --bump-type "$BUMP_TYPE" --description "$DESCRIPTION"- run: rust-script scripts/create-changelog-fragment.rs --bump-type "${{ github.event.inputs.bump_type }}" --description "${{ github.event.inputs.description }}"
+ run: rust-script scripts/create-changelog-fragment.rs --bump-type "$BUMP_TYPE" --description "$DESCRIPTION"As per coding guidelines, **/*: Флагай любой уход от спеки, недокументированное изменение поведения, отсутствие тестов для заявленного поведения и security-риск.
Also applies to: 596-600
🤖 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/rust-browser-connection/.github/workflows/release.yml` around lines
478 - 483, The run step in the "Version and commit" job is interpolating
workflow_dispatch inputs directly into the shell command (rust-script ...
--bump-type "${{ github.event.inputs.bump_type }}" --description "${{
github.event.inputs.description }}"), which is unsafe; update the step to use
the already-declared environment variables (BUMP_TYPE and DESCRIPTION) instead
(e.g., reference $BUMP_TYPE and $DESCRIPTION in the run command) and make the
same change for the other occurrence referenced (lines 596-600) so all inputs
are consumed via env variables rather than direct github.event.inputs
interpolation; keep the step id/version and env names (id: version, env:
BUMP_TYPE, DESCRIPTION) intact.
Comment on lines
+351
to
+419
| fn collect_changelog(changelog_dir: &str, changelog_file: &str, version: &str) { | ||
| let dir_path = Path::new(changelog_dir); | ||
| if !dir_path.exists() { | ||
| return; | ||
| } | ||
|
|
||
| let mut files: Vec<_> = match fs::read_dir(dir_path) { | ||
| Ok(entries) => entries | ||
| .filter_map(|e| e.ok()) | ||
| .map(|e| e.path()) | ||
| .filter(|p| { | ||
| p.extension().map_or(false, |ext| ext == "md") | ||
| && p.file_name().map_or(false, |name| name != "README.md") | ||
| }) | ||
| .collect(), | ||
| Err(_) => return, | ||
| }; | ||
|
|
||
| if files.is_empty() { | ||
| return; | ||
| } | ||
|
|
||
| files.sort(); | ||
|
|
||
| let fragments: Vec<String> = files | ||
| .iter() | ||
| .filter_map(|f| fs::read_to_string(f).ok()) | ||
| .map(|c| strip_frontmatter(&c)) | ||
| .filter(|c| !c.is_empty()) | ||
| .collect(); | ||
|
|
||
| if fragments.is_empty() { | ||
| return; | ||
| } | ||
|
|
||
| let date_str = Utc::now().format("%Y-%m-%d").to_string(); | ||
| let new_entry = format!( | ||
| "\n## [{}] - {}\n\n{}\n", | ||
| version, | ||
| date_str, | ||
| fragments.join("\n\n") | ||
| ); | ||
|
|
||
| if Path::new(changelog_file).exists() { | ||
| let mut content = fs::read_to_string(changelog_file).unwrap_or_default(); | ||
| let lines: Vec<&str> = content.lines().collect(); | ||
| let mut insert_index = None; | ||
|
|
||
| for (i, line) in lines.iter().enumerate() { | ||
| if line.starts_with("## [") { | ||
| insert_index = Some(i); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if let Some(idx) = insert_index { | ||
| let mut new_lines: Vec<String> = lines[..idx].iter().map(|s| s.to_string()).collect(); | ||
| new_lines.push(new_entry.clone()); | ||
| new_lines.extend(lines[idx..].iter().map(|s| s.to_string())); | ||
| content = new_lines.join("\n"); | ||
| } else { | ||
| content.push_str(&new_entry); | ||
| } | ||
|
|
||
| fs::write(changelog_file, content).expect("Failed to write changelog"); | ||
| } | ||
|
|
||
| println!("Collected {} changelog fragment(s)", files.len()); | ||
| } |
There was a problem hiding this comment.
После сборки changelog фрагменты не удаляются, что провоцирует повторные релизные бампы.
В текущем потоке auto-release это может оставлять changelog.d/*.md в ветке и триггерить повторную обработку одних и тех же фрагментов на следующих пушах.
Предложение исправления
fn collect_changelog(changelog_dir: &str, changelog_file: &str, version: &str) {
@@
println!("Collected {} changelog fragment(s)", files.len());
+
+ for file in files {
+ if let Err(e) = fs::remove_file(&file) {
+ eprintln!("Warning: Failed to remove {}: {}", file.display(), e);
+ }
+ }
}🤖 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/rust-browser-connection/scripts/version-and-commit.rs` around lines
351 - 419, collect_changelog currently gathers markdown fragments from
changelog_dir into files and writes a new entry to changelog_file but never
removes the source fragments, causing repeated release bumps; after successfully
writing the updated changelog in collect_changelog (i.e., after
fs::write(changelog_file, content) succeeds), iterate the same files vector and
remove each fragment file (use fs::remove_file on each Path in files),
handling/remotely logging/remove-file errors gracefully (e.g., ignore non-fatal
errors or log them) so fragments are cleaned up and won't be reprocessed; ensure
you use the existing variables files and changelog_file and only delete after a
successful write.
Comment on lines
+24
to
+42
| let config = Config { | ||
| image: Some("dg-docker-git-browser:latest".to_string()), // adjust to your image | ||
| host_config: Some(HostConfig { | ||
| port_bindings: Some(HashMap::from([ | ||
| ("5900/tcp".to_string(), Some(vec![bollard::models::PortBinding { | ||
| host_ip: Some("0.0.0.0".to_string()), | ||
| host_port: Some("5900".to_string()), | ||
| }])), | ||
| ("6080/tcp".to_string(), Some(vec![bollard::models::PortBinding { | ||
| host_ip: Some("0.0.0.0".to_string()), | ||
| host_port: Some("6080".to_string()), | ||
| }])), | ||
| ("9223/tcp".to_string(), Some(vec![bollard::models::PortBinding { | ||
| host_ip: Some("0.0.0.0".to_string()), | ||
| host_port: Some("9223".to_string()), | ||
| }])), | ||
| ])), | ||
| ..Default::default() | ||
| }), |
There was a problem hiding this comment.
Не биньдите CDP/noVNC/VNC на 0.0.0.0 по умолчанию.
Сейчас любой узел, который видит эту машину, может подключиться к CDP или noVNC и перехватить браузерную сессию. Для локального bridge безопаснее публиковать порты на 127.0.0.1 и делать внешний доступ только через явно настроенный proxy.
💡 Минимальное ужесточение
- host_ip: Some("0.0.0.0".to_string()),
+ host_ip: Some("127.0.0.1".to_string()),Примените это ко всем трём binding'ам.
📝 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.
Suggested change
| let config = Config { | |
| image: Some("dg-docker-git-browser:latest".to_string()), // adjust to your image | |
| host_config: Some(HostConfig { | |
| port_bindings: Some(HashMap::from([ | |
| ("5900/tcp".to_string(), Some(vec![bollard::models::PortBinding { | |
| host_ip: Some("0.0.0.0".to_string()), | |
| host_port: Some("5900".to_string()), | |
| }])), | |
| ("6080/tcp".to_string(), Some(vec![bollard::models::PortBinding { | |
| host_ip: Some("0.0.0.0".to_string()), | |
| host_port: Some("6080".to_string()), | |
| }])), | |
| ("9223/tcp".to_string(), Some(vec![bollard::models::PortBinding { | |
| host_ip: Some("0.0.0.0".to_string()), | |
| host_port: Some("9223".to_string()), | |
| }])), | |
| ])), | |
| ..Default::default() | |
| }), | |
| let config = Config { | |
| image: Some("dg-docker-git-browser:latest".to_string()), // adjust to your image | |
| host_config: Some(HostConfig { | |
| port_bindings: Some(HashMap::from([ | |
| ("5900/tcp".to_string(), Some(vec![bollard::models::PortBinding { | |
| host_ip: Some("127.0.0.1".to_string()), | |
| host_port: Some("5900".to_string()), | |
| }])), | |
| ("6080/tcp".to_string(), Some(vec![bollard::models::PortBinding { | |
| host_ip: Some("127.0.0.1".to_string()), | |
| host_port: Some("6080".to_string()), | |
| }])), | |
| ("9223/tcp".to_string(), Some(vec![bollard::models::PortBinding { | |
| host_ip: Some("127.0.0.1".to_string()), | |
| host_port: Some("9223".to_string()), | |
| }])), | |
| ])), | |
| ..Default::default() | |
| }), |
🤖 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/rust-browser-connection/src/lib.rs` around lines 24 - 42, The port
bindings in Config -> HostConfig -> port_bindings currently bind CDP/noVNC/VNC
to "0.0.0.0", which exposes ports 5900, 6080 and 9223 to the network; change the
host_ip for the PortBinding entries for "5900/tcp", "6080/tcp", and "9223/tcp"
from "0.0.0.0" to "127.0.0.1" so they are only accessible locally (update the
PortBinding host_ip values where port_bindings is defined in Config/HostConfig).
Comment on lines
+72
to
+74
| pub fn is_single_browser_session(&self, cdp_url: &str, novnc_url: &str) -> bool { | ||
| cdp_url.contains("9223") && novnc_url.contains("/vnc.html") | ||
| } |
There was a problem hiding this comment.
is_single_browser_session не проверяет инвариант одной сессии.
Этот метод возвращает true для любых несвязанных URL, если в одном есть 9223, а в другом /vnc.html. Для API, которое заявляет single-session invariant из #347, такая проверка даёт ложное чувство корректности.
🤖 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/rust-browser-connection/src/lib.rs` around lines 72 - 74, Метод
is_single_browser_session неправильно считает совпадение по наличию подстрок;
вместо этого распарсьте cdp_url и novnc_url и убедитесь, что они относятся к
одной и той же сессии: сравните их origin (scheme + host + port) и/или явные
идентификаторы сессии в пути/параметрах, и только если origin совпадает и
cdp_url содержит порт 9223, а novnc_url содержит путь /vnc.html — возвращайте
true; обновите функцию is_single_browser_session, чтобы использовать URL-парсинг
и чёткое сравнение origin/session-id вместо простого contains.
- Remove TS Playwright browser runtime and vendored Rust duplicate from docker-git - Install ProverCoderAI/rust-browser-connection as the single noVNC/CDP browser provider - Preserve invariant: generated Playwright MCP startup calls docker-git-browser-connection start --project <id> exactly once before MCP config
Member
Author
AI Session BackupCommit: 3dbf864
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/lib/tests/core/templates.test.ts (1)
767-774:⚠️ Potential issue | 🟠 Major | ⚡ Quick winЗафиксируйте инвариант “ровно один запуск” для Rust browser connection.
Сейчас тест валидирует только порядок, но не исключает дублирование вызова
docker_git_start_rust_browser_connection. Это может нарушить single-session контракт.Предлагаемое усиление теста
it("renders Rust browser startup before MCP client config", () => { const entrypoint = renderEntrypoint(makeTemplateConfig({ enableMcpPlaywright: true })) const browserRuntimeIndex = entrypoint.indexOf("docker_git_start_rust_browser_connection") const mcpConfigIndex = entrypoint.indexOf("[mcp_servers.playwright]") + const startupCalls = entrypoint.match(/docker_git_start_rust_browser_connection/g) ?? [] expect(browserRuntimeIndex).toBeGreaterThanOrEqual(0) expect(mcpConfigIndex).toBeGreaterThan(browserRuntimeIndex) + expect(startupCalls).toHaveLength(1) })As per coding guidelines
**/*:Сверь изменения с исходным ТЗ/спекой ... Флагай ... отсутствие тестов для заявленного поведения.🤖 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/tests/core/templates.test.ts` around lines 767 - 774, The test "renders Rust browser startup before MCP client config" currently only checks order using entrypoint.indexOf and allows multiple occurrences of "docker_git_start_rust_browser_connection"; update the test to also assert exactly one occurrence to enforce the single-session invariant by computing the count of occurrences of "docker_git_start_rust_browser_connection" in entrypoint (e.g., split/regex or Array.from(entrypoint.matchAll(...))). Keep the existing order assertions using renderEntrypoint(makeTemplateConfig({ enableMcpPlaywright: true })), browserRuntimeIndex and mcpConfigIndex but add an assertion that the occurrence count equals 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 `@packages/app/src/lib/core/templates/dockerfile.ts`:
- Around line 86-96: Pin the Rust toolchain and the rust-browser-connection
dependency to fixed, reviewable versions: replace the floating
"--default-toolchain stable" invocation in the Dockerfile template with a
specific toolchain triple/version (e.g., "1.72.1" or the project's chosen pinned
toolchain), and replace the cargo install --git ... --branch main
docker-git-browser-connection with a cargo install using a specific tag or
commit hash (use --rev <commit-hash> or --tag <vX.Y.Z>) so the binary is
reproducible; apply the same changes in both templates referenced
(packages/app/src/lib/core/templates/dockerfile.ts and
packages/lib/src/core/templates/dockerfile.ts) and ensure the Dockerfile still
verifies the installed versions with rustc --version and
docker-git-browser-connection --version.
In `@packages/app/tests/docker-git/core-templates.test.ts`:
- Around line 60-63: Add an assertion to ensure the legacy file
"mcp-playwright-start-extra.sh" is not present by adding
expect(filePaths).not.toContain("mcp-playwright-start-extra.sh") alongside the
existing negative checks that use the filePaths variable (the same block that
checks "Dockerfile.browser", "docker-git-cdp-guard", and
"docker-git-browser-runtime.sh"), placing it before the dockerfile.contents
assertion so the test will fail if the legacy script is present.
---
Outside diff comments:
In `@packages/lib/tests/core/templates.test.ts`:
- Around line 767-774: The test "renders Rust browser startup before MCP client
config" currently only checks order using entrypoint.indexOf and allows multiple
occurrences of "docker_git_start_rust_browser_connection"; update the test to
also assert exactly one occurrence to enforce the single-session invariant by
computing the count of occurrences of "docker_git_start_rust_browser_connection"
in entrypoint (e.g., split/regex or Array.from(entrypoint.matchAll(...))). Keep
the existing order assertions using renderEntrypoint(makeTemplateConfig({
enableMcpPlaywright: true })), browserRuntimeIndex and mcpConfigIndex but add an
assertion that the occurrence count equals 1.
🪄 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: c876aa85-3b75-470d-bef9-ce5f30776d9a
📒 Files selected for processing (17)
packages/app/src/lib/core/templates-entrypoint.tspackages/app/src/lib/core/templates-entrypoint/playwright-browser.tspackages/app/src/lib/core/templates-entrypoint/tasks.tspackages/app/src/lib/core/templates.tspackages/app/src/lib/core/templates/dockerfile.tspackages/app/src/lib/core/templates/playwright-browser-runtime.tspackages/app/src/lib/core/templates/playwright.tspackages/app/tests/docker-git/core-templates.test.tspackages/lib/src/core/templates-entrypoint.tspackages/lib/src/core/templates-entrypoint/playwright-browser.tspackages/lib/src/core/templates-entrypoint/tasks.tspackages/lib/src/core/templates.tspackages/lib/src/core/templates/dockerfile.tspackages/lib/src/core/templates/playwright-browser-runtime.tspackages/lib/src/core/templates/playwright.tspackages/lib/tests/core/templates.test.tspackages/lib/tests/usecases/mcp-playwright.test.ts
💤 Files with no reviewable changes (6)
- packages/app/src/lib/core/templates/playwright-browser-runtime.ts
- packages/lib/src/core/templates/playwright-browser-runtime.ts
- packages/lib/src/core/templates-entrypoint/playwright-browser.ts
- packages/app/src/lib/core/templates-entrypoint/playwright-browser.ts
- packages/app/src/lib/core/templates/playwright.ts
- packages/lib/src/core/templates/playwright.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). (9)
- GitHub Check: E2E (Runtime volumes + SSH)
- GitHub Check: Final build (windows-latest)
- GitHub Check: E2E (Login context)
- GitHub Check: E2E (Clone auto-open SSH)
- GitHub Check: E2E (Clone cache)
- GitHub Check: Lint
- GitHub Check: E2E (Browser command)
- GitHub Check: Test
- GitHub Check: E2E (OpenCode)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{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/lib/src/core/templates-entrypoint/tasks.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/lib/src/core/templates.tspackages/app/tests/docker-git/core-templates.test.tspackages/app/src/lib/core/templates/dockerfile.tspackages/app/src/lib/core/templates-entrypoint/tasks.tspackages/app/src/lib/core/templates.tspackages/app/src/lib/core/templates-entrypoint.tspackages/lib/src/core/templates-entrypoint.tspackages/lib/tests/core/templates.test.tspackages/lib/src/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/lib/src/core/templates-entrypoint/tasks.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/lib/src/core/templates.tspackages/app/tests/docker-git/core-templates.test.tspackages/app/src/lib/core/templates/dockerfile.tspackages/app/src/lib/core/templates-entrypoint/tasks.tspackages/app/src/lib/core/templates.tspackages/app/src/lib/core/templates-entrypoint.tspackages/lib/src/core/templates-entrypoint.tspackages/lib/tests/core/templates.test.tspackages/lib/src/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/lib/src/core/templates-entrypoint/tasks.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/lib/src/core/templates.tspackages/app/tests/docker-git/core-templates.test.tspackages/app/src/lib/core/templates/dockerfile.tspackages/app/src/lib/core/templates-entrypoint/tasks.tspackages/app/src/lib/core/templates.tspackages/app/src/lib/core/templates-entrypoint.tspackages/lib/src/core/templates-entrypoint.tspackages/lib/tests/core/templates.test.tspackages/lib/src/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/lib/src/core/templates-entrypoint/tasks.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/lib/src/core/templates.tspackages/app/tests/docker-git/core-templates.test.tspackages/app/src/lib/core/templates/dockerfile.tspackages/app/src/lib/core/templates-entrypoint/tasks.tspackages/app/src/lib/core/templates.tspackages/app/src/lib/core/templates-entrypoint.tspackages/lib/src/core/templates-entrypoint.tspackages/lib/tests/core/templates.test.tspackages/lib/src/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/lib/src/core/templates-entrypoint/tasks.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/lib/src/core/templates.tspackages/app/tests/docker-git/core-templates.test.tspackages/app/src/lib/core/templates/dockerfile.tspackages/app/src/lib/core/templates-entrypoint/tasks.tspackages/app/src/lib/core/templates.tspackages/app/src/lib/core/templates-entrypoint.tspackages/lib/src/core/templates-entrypoint.tspackages/lib/tests/core/templates.test.tspackages/lib/src/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/lib/src/core/templates-entrypoint/tasks.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/lib/src/core/templates.tspackages/app/tests/docker-git/core-templates.test.tspackages/app/src/lib/core/templates/dockerfile.tspackages/app/src/lib/core/templates-entrypoint/tasks.tspackages/app/src/lib/core/templates.tspackages/app/src/lib/core/templates-entrypoint.tspackages/lib/src/core/templates-entrypoint.tspackages/lib/tests/core/templates.test.tspackages/lib/src/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
packages/app/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (README.md)
App layer (APP) should work only with API and not have direct access to LIB layer
Files:
packages/app/tests/docker-git/core-templates.test.tspackages/app/src/lib/core/templates/dockerfile.tspackages/app/src/lib/core/templates-entrypoint/tasks.tspackages/app/src/lib/core/templates.tspackages/app/src/lib/core/templates-entrypoint.ts
🔇 Additional comments (3)
packages/app/src/lib/core/templates-entrypoint/tasks.ts (1)
285-287: ⚡ Quick winHermes built-in браузер не должен запускаться только при
MCP_PLAYWRIGHT_ENABLE=1В
packages/app/src/lib/core/templates-entrypoint/tasks.tsранний guard возвращает0, покаMCP_PLAYWRIGHT_ENABLE != "1"(стр. 285–287), из‑за чего контейнерdg-*-browser/CDP (9223) может не подниматься в сценарии Hermes без MCP. При этом по ТЗ095118-clean-builtin-browser-noVNC-no-mcp.mdдля Hermes no‑MCP требуется оставлятьbrowser.engine=cdpиbrowser.cdp_url, указывающие на CDP живого браузер‑контейнера.Нужен общий признак “browser required” (Hermes built-in), а не только MCP-флаг, либо Hermes no‑MCP должен гарантированно поднимать
dg-*-browser/CDP endpoint и при этом не оставлять MCP Playwright tools.packages/lib/tests/core/templates.test.ts (1)
744-766: LGTM!packages/lib/tests/usecases/mcp-playwright.test.ts (1)
145-148: LGTM!Also applies to: 153-156
Comment on lines
+86
to
+96
| RUN set -eu; \ | ||
| curl --proto '=https' --tlsv1.2 -fsSL https://sh.rustup.rs -o /tmp/rustup-init.sh; \ | ||
| sh /tmp/rustup-init.sh -y --profile minimal --default-toolchain stable; \ | ||
| rm -f /tmp/rustup-init.sh; \ | ||
| rustc --version; \ | ||
| cargo --version | ||
|
|
||
| # Install unified Rust browser connection (noVNC + CDP + single dg-*-browser guarantee) | ||
| # Replaces all previous TS/MCP browser-connection duplication (per issue #347) | ||
| RUN cargo install --git https://github.com/ProverCoderAI/rust-browser-connection --branch main docker-git-browser-connection --locked \ | ||
| && docker-git-browser-connection --version |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show exact lines around the reported snippet
FILE="packages/app/src/lib/core/templates/dockerfile.ts"
echo "=== $FILE (lines 60-120) ==="
nl -ba "$FILE" | sed -n '60,120p'
# Find other dockerfile templates that mention rustup / rust-browser-connection
echo
echo "=== Search for rust-browser-connection in repo ==="
rg -n "rust-browser-connection" -S .
echo
echo "=== Search for rustup default-toolchain stable ==="
rg -n "default-toolchain[[:space:]]+stable|rustup\.rs|sh\.rustup\.rs" -S .
# Locate any lib mirror template
echo
echo "=== Search for packages/lib core templates dockerfile.ts ==="
fd -a -e ts -e tsx -e js -e jsx "dockerfile.ts" packages/lib packages/app || true
# Print any other dockerfile.ts containing cargo install --git ...rust-browser-connection
echo
echo "=== Files with cargo install git rust-browser-connection ==="
rg -l "cargo install --git .*rust-browser-connection" -S . || trueRepository: ProverCoderAI/docker-git
Length of output: 202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/app/src/lib/core/templates/dockerfile.ts"
echo "=== $FILE (lines 70-110) ==="
awk 'NR>=70 && NR<=110 {printf "%d\t%s\n", NR, $0}' "$FILE"
echo
echo "=== Search for rust-browser-connection in repo ==="
rg -n "rust-browser-connection" -S .
echo
echo
echo "=== Search for rustup default-toolchain stable or rustup install ==="
rg -n "default-toolchain[[:space:]]+stable|sh\.rustup\.rs|rustup\.rs" -S .
echo
echo
echo "=== Locate any lib mirror template dockerfile.ts ==="
# list candidate dockerfile.ts under packages/lib and packages/app
fd -a -e ts -e tsx -e js -e jsx "dockerfile.ts" packages/lib packages/app || true
echo
echo
echo "=== Files with cargo install git rust-browser-connection ==="
rg -l "cargo install --git .*rust-browser-connection" -S . || true
echo
echo
echo "=== Files containing --branch main and docker-git-browser-connection ==="
rg -n "--branch[[:space:]]+main|docker-git-browser-connection" -S packages/lib packages/app || trueRepository: ProverCoderAI/docker-git
Length of output: 6264
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for pinning args/vars for rust toolchain/crate ==="
rg -n "RUST_TOOLCHAIN|DOCKER_GIT_BROWSER_CONNECTION_REV|BROWSER_CONNECTION_REV|rust-toolchain|toolchain=" -S packages/app packages/lib . || true
echo
echo "=== Show relevant section in packages/lib/src/core/templates/dockerfile.ts (lines 70-110) ==="
awk 'NR>=70 && NR<=110 {printf "%d\t%s\n", NR, $0}' "packages/lib/src/core/templates/dockerfile.ts"
echo
echo "=== Show relevant cargo install section in packages/lib/src/core/templates/dockerfile.ts (lines 90-110) ==="
awk 'NR>=88 && NR<=110 {printf "%d\t%s\n", NR, $0}' "packages/lib/src/core/templates/dockerfile.ts"Repository: ProverCoderAI/docker-git
Length of output: 3478
Пиньте версии Rust toolchain и rust-browser-connection (в обоих Dockerfile-шаблонах)
Сейчас в packages/app/src/lib/core/templates/dockerfile.ts и packages/lib/src/core/templates/dockerfile.ts стоят плавающие зависимости: --default-toolchain stable и cargo install ... --branch main. Это делает сборки нерепродуцируемыми и позволяет тихо менять поведение browser-модуля без изменений в репозитории (подрывается проверяемость заявленного “single dg-*-browser guarantee” и supply-chain устойчивость).
💡 Вариант фикса
+ARG RUST_TOOLCHAIN=1.87.0
RUN set -eu; \
curl --proto '=https' --tlsv1.2 -fsSL https://sh.rustup.rs -o /tmp/rustup-init.sh; \
- sh /tmp/rustup-init.sh -y --profile minimal --default-toolchain stable; \
+ sh /tmp/rustup-init.sh -y --profile minimal --default-toolchain ${RUST_TOOLCHAIN}; \
rm -f /tmp/rustup-init.sh; \
rustc --version; \
cargo --version
+ARG DOCKER_GIT_BROWSER_CONNECTION_REV=<commit-or-tag>
-RUN cargo install --git https://github.com/ProverCoderAI/rust-browser-connection --branch main docker-git-browser-connection --locked \
+RUN cargo install --git https://github.com/ProverCoderAI/rust-browser-connection --rev ${DOCKER_GIT_BROWSER_CONNECTION_REV} docker-git-browser-connection --locked \
&& docker-git-browser-connection --version🤖 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/app/src/lib/core/templates/dockerfile.ts` around lines 86 - 96, Pin
the Rust toolchain and the rust-browser-connection dependency to fixed,
reviewable versions: replace the floating "--default-toolchain stable"
invocation in the Dockerfile template with a specific toolchain triple/version
(e.g., "1.72.1" or the project's chosen pinned toolchain), and replace the cargo
install --git ... --branch main docker-git-browser-connection with a cargo
install using a specific tag or commit hash (use --rev <commit-hash> or --tag
<vX.Y.Z>) so the binary is reproducible; apply the same changes in both
templates referenced (packages/app/src/lib/core/templates/dockerfile.ts and
packages/lib/src/core/templates/dockerfile.ts) and ensure the Dockerfile still
verifies the installed versions with rustc --version and
docker-git-browser-connection --version.
Comment on lines
+60
to
63
| expect(filePaths).not.toContain("Dockerfile.browser") | ||
| expect(filePaths).not.toContain("docker-git-cdp-guard") | ||
| expect(filePaths).not.toContain("docker-git-browser-runtime.sh") | ||
| expect(dockerfile.contents).toContain( |
There was a problem hiding this comment.
Добавьте проверку отсутствия mcp-playwright-start-extra.sh в app-планировании.
Сейчас тест не ловит возможный legacy leftover этого файла, хотя миграция заявлена как единая Rust-only. Добавьте expect(filePaths).not.toContain("mcp-playwright-start-extra.sh").
As per coding guidelines **/*: Флагай ... отсутствие тестов для заявленного поведения.
🤖 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/app/tests/docker-git/core-templates.test.ts` around lines 60 - 63,
Add an assertion to ensure the legacy file "mcp-playwright-start-extra.sh" is
not present by adding
expect(filePaths).not.toContain("mcp-playwright-start-extra.sh") alongside the
existing negative checks that use the filePaths variable (the same block that
checks "Dockerfile.browser", "docker-git-cdp-guard", and
"docker-git-browser-runtime.sh"), placing it before the dockerfile.contents
assertion so the test will fail if the legacy script is present.
Split Dockerfile fragments so vibecode-linter accepts the generated-template sources and install rust-browser-connection through rustup stable instead of the base image's Cargo 1.75. Invariant: generated project images either contain /usr/local/bin/docker-git-browser-connection built by current stable cargo or fail the Docker build before MCP Playwright can start without a browser.
Member
Author
AI Session BackupCommit: 733be1d
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/app/src/lib/core/templates/dockerfile-prelude.ts (1)
1-98:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftДублирование: файл идентичен
packages/lib/src/core/templates/dockerfile-prelude.ts.Весь файл — копия из LIB-слоя. Нарушает QUOTE(ТЗ) "без TS-дублирования" и план по удалению дубликатов.
APP должен реэкспортировать из LIB:
-// ... (весь файл) +export { renderDockerfilePrelude } from "`@prover-coder-ai/lib/core/templates/dockerfile-prelude.js`"🤖 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/app/src/lib/core/templates/dockerfile-prelude.ts` around lines 1 - 98, This file duplicates the LIB implementation; remove the duplicate definitions (renderDockerfileBase, renderDockerfileRustBrowserConnection, renderDockerfilePrelude) and instead re-export the implementation from the lib package; replace the module body with a forward/export that imports and re-exports renderDockerfilePrelude (or the individual functions) from the LIB module (the symbols to reference: renderDockerfileBase, renderDockerfileRustBrowserConnection, renderDockerfilePrelude) so APP no longer contains TS duplication and simply re-exports the canonical implementations.packages/app/src/lib/core/templates/dockerfile-playwright-mcp.ts (1)
1-93:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftДублирование кода нарушает QUOTE(ТЗ): "без TS-дублирования".
Этот файл — точная копия
packages/lib/src/core/templates/dockerfile-playwright-mcp.ts. Согласно плану.hermes/plans/2026-05-23_102144-rust-only-noVNC-browser-module-separate-repo.md, требуется "delete TS browser/noVNC connection duplicates".APP-слой должен импортировать из LIB-слоя или использовать общий пакет, а не дублировать реализацию.
-// CHANGE: isolate the long Playwright MCP wrapper from the primary Dockerfile renderer. -// ... (весь файл) -export const renderDockerfilePlaywrightMcp = (): string => dockerfilePlaywrightMcpBlock.replaceAll("\\${", "${") +export { renderDockerfilePlaywrightMcp } from "`@prover-coder-ai/lib/core/templates/dockerfile-playwright-mcp.js`"🤖 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/app/src/lib/core/templates/dockerfile-playwright-mcp.ts` around lines 1 - 93, The file duplicates the Playwright MCP Docker wrapper already implemented in packages/lib; remove this duplicate and re-export or import the canonical implementation instead: delete the local constant dockerfilePlaywrightMcpBlock and renderDockerfilePlaywrightMcp here, and replace usages with an import from the existing package implementation (the version in packages/lib that defines the same dockerfile block and render function); ensure any callers now import renderDockerfilePlaywrightMcp from the lib package so the APP layer no longer contains TS duplication.
♻️ Duplicate comments (1)
packages/lib/src/core/templates/dockerfile-prelude.ts (1)
65-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winПлавающие зависимости нарушают воспроизводимость сборок и supply-chain безопасность.
Как уже отмечалось в предыдущем ревью:
--default-toolchain stableи--branch mainделают сборки нерепродуцируемыми. Это противоречит заявленному "single dg-*-browser guarantee" — бинарник может меняться без изменений в репозитории.🔧 Рекомендуемый фикс
+ARG RUST_TOOLCHAIN=1.87.0 RUN set -eu; \ curl --proto '=https' --tlsv1.2 -fsSL https://sh.rustup.rs -o /tmp/rustup-init.sh; \ - HOME=/root sh /tmp/rustup-init.sh -y --profile minimal --default-toolchain stable --no-modify-path; \ + HOME=/root sh /tmp/rustup-init.sh -y --profile minimal --default-toolchain ${RUST_TOOLCHAIN} --no-modify-path; \ rm -f /tmp/rustup-init.sh; \ rustc --version; \ cargo --version +ARG DOCKER_GIT_BROWSER_CONNECTION_REV=<commit-sha-or-tag> -RUN cargo install --git https://github.com/ProverCoderAI/rust-browser-connection --branch main --locked --root /usr/local docker-git-browser-connection \ +RUN cargo install --git https://github.com/ProverCoderAI/rust-browser-connection --rev ${DOCKER_GIT_BROWSER_CONNECTION_REV} --locked --root /usr/local docker-git-browser-connection \ && /usr/local/bin/docker-git-browser-connection --version🤖 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-prelude.ts` around lines 65 - 84, The Dockerfile prelude uses floating refs (renderDockerfileRustBrowserConnection): replace --default-toolchain stable and --branch main with pinned, reproducible values—set rustup-init to install a specific toolchain version (e.g., --default-toolchain 1.XX.X or pass the exact rustup toolchain string) and change cargo install --git ... --branch main to reference an exact commit/tag via --rev <commit-sha> (or a semantically versioned tag), or vendor the binary into the repo; update the inline comment to note the pinned toolchain and git rev to preserve the "single dg-*-browser guarantee."
🤖 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/tasks.ts`:
- Around line 281-319: This function renderEntrypointRustBrowserConnection
duplicates the LIB implementation; remove the duplicated implementation and
re-export the canonical one from the LIB package instead. Replace the local
definition of renderEntrypointRustBrowserConnection (and avoid redefining
docker_git_start_rust_browser_connection) with an import from the lib package
and a direct export (e.g. import { renderEntrypointRustBrowserConnection } from
'...lib...' and export it), and update any local consumers to use that re-export
so only the single LIB definition remains.
---
Outside diff comments:
In `@packages/app/src/lib/core/templates/dockerfile-playwright-mcp.ts`:
- Around line 1-93: The file duplicates the Playwright MCP Docker wrapper
already implemented in packages/lib; remove this duplicate and re-export or
import the canonical implementation instead: delete the local constant
dockerfilePlaywrightMcpBlock and renderDockerfilePlaywrightMcp here, and replace
usages with an import from the existing package implementation (the version in
packages/lib that defines the same dockerfile block and render function); ensure
any callers now import renderDockerfilePlaywrightMcp from the lib package so the
APP layer no longer contains TS duplication.
In `@packages/app/src/lib/core/templates/dockerfile-prelude.ts`:
- Around line 1-98: This file duplicates the LIB implementation; remove the
duplicate definitions (renderDockerfileBase,
renderDockerfileRustBrowserConnection, renderDockerfilePrelude) and instead
re-export the implementation from the lib package; replace the module body with
a forward/export that imports and re-exports renderDockerfilePrelude (or the
individual functions) from the LIB module (the symbols to reference:
renderDockerfileBase, renderDockerfileRustBrowserConnection,
renderDockerfilePrelude) so APP no longer contains TS duplication and simply
re-exports the canonical implementations.
---
Duplicate comments:
In `@packages/lib/src/core/templates/dockerfile-prelude.ts`:
- Around line 65-84: The Dockerfile prelude uses floating refs
(renderDockerfileRustBrowserConnection): replace --default-toolchain stable and
--branch main with pinned, reproducible values—set rustup-init to install a
specific toolchain version (e.g., --default-toolchain 1.XX.X or pass the exact
rustup toolchain string) and change cargo install --git ... --branch main to
reference an exact commit/tag via --rev <commit-sha> (or a semantically
versioned tag), or vendor the binary into the repo; update the inline comment to
note the pinned toolchain and git rev to preserve the "single dg-*-browser
guarantee."
🪄 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: 64521a46-ed72-4b25-8d08-ea34bb77ac86
📒 Files selected for processing (8)
packages/app/src/lib/core/templates-entrypoint/tasks.tspackages/app/src/lib/core/templates/dockerfile-playwright-mcp.tspackages/app/src/lib/core/templates/dockerfile-prelude.tspackages/app/src/lib/core/templates/dockerfile.tspackages/lib/src/core/templates-entrypoint/tasks.tspackages/lib/src/core/templates/dockerfile-playwright-mcp.tspackages/lib/src/core/templates/dockerfile-prelude.tspackages/lib/src/core/templates/dockerfile.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). (8)
- GitHub Check: E2E (OpenCode)
- GitHub Check: E2E (Login context)
- GitHub Check: E2E (Runtime volumes + SSH)
- GitHub Check: E2E (Clone cache)
- GitHub Check: Test
- GitHub Check: E2E (Clone auto-open SSH)
- GitHub Check: E2E (Browser command)
- GitHub Check: Lint
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{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/lib/src/core/templates-entrypoint/tasks.tspackages/lib/src/core/templates/dockerfile-playwright-mcp.tspackages/app/src/lib/core/templates/dockerfile-playwright-mcp.tspackages/app/src/lib/core/templates-entrypoint/tasks.tspackages/lib/src/core/templates/dockerfile-prelude.tspackages/app/src/lib/core/templates/dockerfile-prelude.tspackages/app/src/lib/core/templates/dockerfile.tspackages/lib/src/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/lib/src/core/templates-entrypoint/tasks.tspackages/lib/src/core/templates/dockerfile-playwright-mcp.tspackages/app/src/lib/core/templates/dockerfile-playwright-mcp.tspackages/app/src/lib/core/templates-entrypoint/tasks.tspackages/lib/src/core/templates/dockerfile-prelude.tspackages/app/src/lib/core/templates/dockerfile-prelude.tspackages/app/src/lib/core/templates/dockerfile.tspackages/lib/src/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/lib/src/core/templates-entrypoint/tasks.tspackages/lib/src/core/templates/dockerfile-playwright-mcp.tspackages/app/src/lib/core/templates/dockerfile-playwright-mcp.tspackages/app/src/lib/core/templates-entrypoint/tasks.tspackages/lib/src/core/templates/dockerfile-prelude.tspackages/app/src/lib/core/templates/dockerfile-prelude.tspackages/app/src/lib/core/templates/dockerfile.tspackages/lib/src/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/lib/src/core/templates-entrypoint/tasks.tspackages/lib/src/core/templates/dockerfile-playwright-mcp.tspackages/app/src/lib/core/templates/dockerfile-playwright-mcp.tspackages/app/src/lib/core/templates-entrypoint/tasks.tspackages/lib/src/core/templates/dockerfile-prelude.tspackages/app/src/lib/core/templates/dockerfile-prelude.tspackages/app/src/lib/core/templates/dockerfile.tspackages/lib/src/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/lib/src/core/templates-entrypoint/tasks.tspackages/lib/src/core/templates/dockerfile-playwright-mcp.tspackages/app/src/lib/core/templates/dockerfile-playwright-mcp.tspackages/app/src/lib/core/templates-entrypoint/tasks.tspackages/lib/src/core/templates/dockerfile-prelude.tspackages/app/src/lib/core/templates/dockerfile-prelude.tspackages/app/src/lib/core/templates/dockerfile.tspackages/lib/src/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/lib/src/core/templates-entrypoint/tasks.tspackages/lib/src/core/templates/dockerfile-playwright-mcp.tspackages/app/src/lib/core/templates/dockerfile-playwright-mcp.tspackages/app/src/lib/core/templates-entrypoint/tasks.tspackages/lib/src/core/templates/dockerfile-prelude.tspackages/app/src/lib/core/templates/dockerfile-prelude.tspackages/app/src/lib/core/templates/dockerfile.tspackages/lib/src/core/templates/dockerfile.ts
packages/app/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (README.md)
App layer (APP) should work only with API and not have direct access to LIB layer
Files:
packages/app/src/lib/core/templates/dockerfile-playwright-mcp.tspackages/app/src/lib/core/templates-entrypoint/tasks.tspackages/app/src/lib/core/templates/dockerfile-prelude.tspackages/app/src/lib/core/templates/dockerfile.ts
🔇 Additional comments (8)
packages/lib/src/core/templates-entrypoint/tasks.ts (1)
281-319: LGTM!packages/lib/src/core/templates/dockerfile-playwright-mcp.ts (1)
1-93: LGTM!packages/lib/src/core/templates/dockerfile-prelude.ts (2)
10-54: LGTM!
85-98: LGTM!packages/app/src/lib/core/templates/dockerfile.ts (2)
4-5: LGTM!
110-118: LGTM!packages/lib/src/core/templates/dockerfile.ts (2)
4-5: LGTM!
88-118: LGTM!
Comment on lines
+281
to
+319
| export const renderEntrypointRustBrowserConnection = (): string => | ||
| [ | ||
| "# Unified Rust browser connection (noVNC + CDP) for MCP Playwright + Hermes — per #347.", | ||
| "# Defaults are safe no-ops unless MCP Playwright is enabled.", | ||
| "docker_git_start_rust_browser_connection() {", | ||
| " if [[ \"${MCP_PLAYWRIGHT_ENABLE:-0}\" != \"1\" ]]; then", | ||
| " return 0", | ||
| " fi", | ||
| "", | ||
| " local browser_bin=\"\"", | ||
| " local candidate", | ||
| " for candidate in /root/.cargo/bin/docker-git-browser-connection /usr/local/cargo/bin/docker-git-browser-connection $(command -v docker-git-browser-connection 2>/dev/null || true); do", | ||
| " if [[ -x \"$candidate\" ]]; then", | ||
| " browser_bin=\"$candidate\"", | ||
| " break", | ||
| " fi", | ||
| " done", | ||
| "", | ||
| " if [[ -z \"$browser_bin\" ]]; then", | ||
| " echo \"[browser] WARNING: docker-git-browser-connection not found; Playwright MCP browser is unavailable\" >&2", | ||
| " MCP_PLAYWRIGHT_ENABLE=0", | ||
| " export MCP_PLAYWRIGHT_ENABLE", | ||
| " return 0", | ||
| " fi", | ||
| "", | ||
| " local project_container=\"${DOCKER_GIT_PROJECT_CONTAINER_NAME:-$(hostname)}\"", | ||
| " local network_mode=\"container:${project_container}\"", | ||
| " mkdir -p /var/log", | ||
| " \"$browser_bin\" start --project \"$project_container\" --network \"$network_mode\" >> /var/log/docker-git-browser.log 2>&1 || {", | ||
| " echo \"[browser] WARNING: Rust browser connection failed; see /var/log/docker-git-browser.log\" >&2", | ||
| " MCP_PLAYWRIGHT_ENABLE=0", | ||
| " export MCP_PLAYWRIGHT_ENABLE", | ||
| " return 0", | ||
| " }", | ||
| " echo \"[browser] Rust browser connection is ready via $browser_bin on $network_mode\"", | ||
| "}", | ||
| "", | ||
| "docker_git_start_rust_browser_connection" | ||
| ].join("\n") |
There was a problem hiding this comment.
Дублирование: renderEntrypointRustBrowserConnection идентична версии в packages/lib.
Та же проблема дублирования, что и с dockerfile-playwright-mcp.ts. APP-слой должен реэкспортировать из LIB.
🤖 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/app/src/lib/core/templates-entrypoint/tasks.ts` around lines 281 -
319, This function renderEntrypointRustBrowserConnection duplicates the LIB
implementation; remove the duplicated implementation and re-export the canonical
one from the LIB package instead. Replace the local definition of
renderEntrypointRustBrowserConnection (and avoid redefining
docker_git_start_rust_browser_connection) with an import from the lib package
and a direct export (e.g. import { renderEntrypointRustBrowserConnection } from
'...lib...' and export it), and update any local consumers to use that re-export
so only the single LIB definition remains.
- Add build-essential to generated project images before cargo install - Assert Dockerfile templates keep the cc toolchain needed by Rust build scripts Root cause: cargo install of rust-browser-connection failed in E2E images with linker 'cc' not found.
Member
Author
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 (2)
packages/app/src/lib/core/templates/dockerfile-prelude.ts (1)
78-79: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winНесоответствие пути установки бинарника и путей поиска в entrypoint.
cargo install --root /usr/localразмещает бинарник в/usr/local/bin/docker-git-browser-connection, но entrypoint (см.packages/lib/src/core/templates-entrypoint/tasks.ts:290) сначала проверяет/root/.cargo/bin/и/usr/local/cargo/bin/. Бинарник будет найден только через fallbackcommand -v.Рекомендуется либо добавить
/usr/local/bin/docker-git-browser-connectionпервым в список кандидатов entrypoint, либо изменить--rootна/usr/local/cargoдля консистентности.♻️ Вариант 1: Изменить путь установки
-RUN cargo install --git https://github.com/ProverCoderAI/rust-browser-connection --branch main --locked --root /usr/local docker-git-browser-connection \ - && /usr/local/bin/docker-git-browser-connection --version +RUN cargo install --git https://github.com/ProverCoderAI/rust-browser-connection --branch main --locked docker-git-browser-connection \ + && /usr/local/cargo/bin/docker-git-browser-connection --version🤖 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/app/src/lib/core/templates/dockerfile-prelude.ts` around lines 78 - 79, The Dockerfile prelude installs the binary with "cargo install --root /usr/local" which places docker-git-browser-connection at /usr/local/bin, but the entrypoint candidate search (packages/lib/src/core/templates-entrypoint/tasks.ts) checks /root/.cargo/bin and /usr/local/cargo/bin first, causing a mismatch; fix by making the install destination and the entrypoint search consistent: either change the cargo install root from /usr/local to /usr/local/cargo so the binary lands in /usr/local/cargo/bin, or update the entrypoint candidate list to include /usr/local/bin (prioritized before fallback command -v) so /usr/local/bin/docker-git-browser-connection is found immediately.packages/lib/src/core/templates/dockerfile-prelude.ts (1)
1-97: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoffДублирование
renderDockerfilePreludeмеждуpackages/libиpackages/appтребует обоснования (или синхронизации).
packages/lib/src/core/templates/dockerfile-prelude.tsиpackages/app/src/lib/core/templates/dockerfile-prelude.tsбайт-в-байт одинаковы, при этомpackages/appне импортирует@effect-template/libкак общий источник: вpackages/app/eslint/no-lib-imports.mjs@effect-template/libявно запрещён (правило репортит прямые/алиасные импорты из@effect-template/lib).Если текущее дублирование не намеренное — это нужно устранить; если намеренное из‑за архитектурного ограничения — добавьте ссылку/обоснование (RTM/issue) и механизм, чтобы исключить drift между двумя копиями.
🤖 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-prelude.ts` around lines 1 - 97, Two identical implementations of renderDockerfilePrelude (renderDockerfilePrelude, renderDockerfileBase, renderDockerfileRustBrowserConnection) exist in lib and app causing unwanted duplication; either remove duplication by importing the shared implementation from `@effect-template/lib` (update app to allow controlled import) or, if duplication is intentional, add a short justification (RTM/issue link) and a sync mechanism (e.g., a test or script that diffs the two files) to prevent drift and document the architectural constraint, and make sure the app-level lint rule no-lib-imports is adjusted or an exception is recorded so the import is permitted or the duplication is explicitly allowed.
🤖 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/app/src/lib/core/templates/dockerfile-prelude.ts`:
- Around line 78-79: The Dockerfile prelude installs the binary with "cargo
install --root /usr/local" which places docker-git-browser-connection at
/usr/local/bin, but the entrypoint candidate search
(packages/lib/src/core/templates-entrypoint/tasks.ts) checks /root/.cargo/bin
and /usr/local/cargo/bin first, causing a mismatch; fix by making the install
destination and the entrypoint search consistent: either change the cargo
install root from /usr/local to /usr/local/cargo so the binary lands in
/usr/local/cargo/bin, or update the entrypoint candidate list to include
/usr/local/bin (prioritized before fallback command -v) so
/usr/local/bin/docker-git-browser-connection is found immediately.
In `@packages/lib/src/core/templates/dockerfile-prelude.ts`:
- Around line 1-97: Two identical implementations of renderDockerfilePrelude
(renderDockerfilePrelude, renderDockerfileBase,
renderDockerfileRustBrowserConnection) exist in lib and app causing unwanted
duplication; either remove duplication by importing the shared implementation
from `@effect-template/lib` (update app to allow controlled import) or, if
duplication is intentional, add a short justification (RTM/issue link) and a
sync mechanism (e.g., a test or script that diffs the two files) to prevent
drift and document the architectural constraint, and make sure the app-level
lint rule no-lib-imports is adjusted or an exception is recorded so the import
is permitted or the duplication is explicitly allowed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 05f0cfed-cce8-4a10-bb9f-ecff0099daa4
📒 Files selected for processing (4)
packages/app/src/lib/core/templates/dockerfile-prelude.tspackages/app/tests/docker-git/core-templates.test.tspackages/lib/src/core/templates/dockerfile-prelude.tspackages/lib/tests/core/templates.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). (5)
- GitHub Check: E2E (Clone cache)
- GitHub Check: E2E (Login context)
- GitHub Check: E2E (Clone auto-open SSH)
- GitHub Check: E2E (OpenCode)
- GitHub Check: E2E (Runtime volumes + SSH)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{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/dockerfile-prelude.tspackages/app/tests/docker-git/core-templates.test.tspackages/lib/tests/core/templates.test.tspackages/lib/src/core/templates/dockerfile-prelude.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/dockerfile-prelude.tspackages/app/tests/docker-git/core-templates.test.tspackages/lib/tests/core/templates.test.tspackages/lib/src/core/templates/dockerfile-prelude.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/dockerfile-prelude.tspackages/app/tests/docker-git/core-templates.test.tspackages/lib/tests/core/templates.test.tspackages/lib/src/core/templates/dockerfile-prelude.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/dockerfile-prelude.tspackages/app/tests/docker-git/core-templates.test.tspackages/lib/tests/core/templates.test.tspackages/lib/src/core/templates/dockerfile-prelude.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/dockerfile-prelude.tspackages/app/tests/docker-git/core-templates.test.tspackages/lib/tests/core/templates.test.tspackages/lib/src/core/templates/dockerfile-prelude.ts
packages/app/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (README.md)
App layer (APP) should work only with API and not have direct access to LIB layer
Files:
packages/app/src/lib/core/templates/dockerfile-prelude.tspackages/app/tests/docker-git/core-templates.test.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/dockerfile-prelude.tspackages/app/tests/docker-git/core-templates.test.tspackages/lib/tests/core/templates.test.tspackages/lib/src/core/templates/dockerfile-prelude.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/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/core/templates.test.ts
🔇 Additional comments (5)
packages/lib/src/core/templates/dockerfile-prelude.ts (1)
78-79: Несоответствие пути установки (аналогично app-версии).Та же проблема:
--root /usr/local→ бинарник в/usr/local/bin/, но entrypoint ищет сначала в/root/.cargo/bin/и/usr/local/cargo/bin/.packages/app/tests/docker-git/core-templates.test.ts (2)
60-63: Отсутствует проверкаmcp-playwright-start-extra.sh.Тест в
packages/lib/tests/core/templates.test.ts:760проверяетexpect(filePaths).not.toContain("mcp-playwright-start-extra.sh"), но здесь эта проверка отсутствует. Для консистентности между app и lib тестами следует добавить.
66-70: LGTM!packages/app/src/lib/core/templates/dockerfile-prelude.ts (1)
1-10: LGTM!Also applies to: 20-53, 65-83, 96-97
packages/lib/tests/core/templates.test.ts (1)
92-92: LGTM!Also applies to: 745-775
- remove legacy docker-git Playwright MCP wrapper generation - install pinned Rust browser lifecycle and MCP binaries - configure Codex/Claude/Gemini/Grok to use browser-connection - add tests for safe project fallback and Rust-only browser artifacts Verified-by: bun run typecheck; bun run test; bun run lint; runtime MCP/noVNC smoke
Member
Author
skulidropek
changed the title
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
experiments/render-examples-output.txt (1)
333-383: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueНесоответствие initial state между experiments и auth-helpers.
В
experiments/render-examples-output.txtначальныйsettings.json(строки 333-383) не содержит блокmcpServers.playwright, полагаясь на последующий вызовdocker_git_sync_gemini_playwright_mcp()для его добавления.Однако
auth-gemini-helpers.ts(строки 299-306) включаетmcpServers.playwrightсcommand: "browser-connection"иargs: []вdefaultGeminiSettings.Это создаёт два источника истины:
- auth-helpers: записывает начальный
mcpServers.playwrightс пустымиargs.- experiments sync: добавляет/обновляет
mcpServers.playwrightс динамическимиargs.Функция синхронизации перезапишет начальное состояние, поэтому оба подхода работают, но:
- Увеличивается когнитивная нагрузка на поддержку.
- Неясно, какой файл является каноническим источником конфигурации.
Рекомендация: документируйте в комментариях, что
auth-helpersпредоставляет начальное состояние, аdocker_git_sync_gemini_playwright_mcp()— runtime-обновление, либо унифицируйте подход (убратьmcpServers.playwrightиз обоих начальных состояний).🤖 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 `@experiments/render-examples-output.txt` around lines 333 - 383, defaultGeminiSettings currently defines mcpServers.playwright (with command "browser-connection" and empty args) while docker_git_sync_gemini_playwright_mcp() adds/updates that same mcpServers.playwright at runtime, creating two sources of truth; pick one approach and make it explicit: either remove mcpServers.playwright from the defaultGeminiSettings (so docker_git_sync_gemini_playwright_mcp() is the canonical source) or keep it but document with an inline comment and initialize its args to the same placeholder/dynamic format used by docker_git_sync_gemini_playwright_mcp(); update the code in the module that defines defaultGeminiSettings (and the experiments-generated settings) so both use the same initialization strategy and ensure docker_git_sync_gemini_playwright_mcp() still overwrites or merges consistently.
♻️ Duplicate comments (2)
packages/lib/src/usecases/auth-grok-helpers.ts (1)
237-246:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winКритическая проблема: отсутствует runtime-синхронизация args для Grok (lib версия).
Аналогично
packages/appверсии, для Grok не обнаружена функция синхронизацииargsс параметрами--projectи--network. Без этого MCP Playwright для Grok не сможет подключиться к браузеру.См. комментарий к
packages/app/src/lib/usecases/auth-grok-helpers.ts:238-246для деталей и скрипта проверки.🤖 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/usecases/auth-grok-helpers.ts` around lines 237 - 246, The defaultGrokProjectSettings object is missing runtime synchronization of Playwright MCP args for --project and --network, so update the Playwright config to include and populate args and add a small sync helper (or call-site logic) that injects the current project and network flags into defaultGrokProjectSettings.mcpServers.playwright.args at runtime; specifically, ensure the mcpServers.playwright.args array (and any creation path that uses defaultGrokProjectSettings) receives ["--project", currentProject, "--network", currentNetwork] (or omits each flag when value is empty) via a function like syncGrokArgs or inline logic where settings are instantiated so Grok’s MCP Playwright can connect to the browser.packages/lib/src/usecases/auth-gemini-helpers.ts (1)
299-306:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffПустой массив
argsсоздаёт зависимость от runtime-синхронизации (lib версия).Аналогично
packages/appверсии, значениеargs: []означает неполную конфигурацию до выполненияdocker_git_sync_gemini_playwright_mcp(). Убедитесь, что синхронизация выполняется до запуска Gemini CLI с MCP-серверами, иначеbrowser-connectionне получит параметры--projectи--network.См. комментарий к
packages/app/src/lib/usecases/auth-gemini-helpers.ts:300-306для деталей и скрипта проверки.🤖 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/usecases/auth-gemini-helpers.ts` around lines 299 - 306, The mcpServers.playwright block currently uses args: [] which defers required CLI params to runtime and creates a race; either populate the args array with the expected parameters (e.g. include --project and --network values) in the static config for the "browser-connection" command or ensure docker_git_sync_gemini_playwright_mcp() is invoked and completes before starting the Gemini CLI so the MCP servers receive their parameters; update the mcpServers.playwright configuration or the startup flow that calls the Gemini CLI (the code path that reads this mcpServers entry) to guarantee docker_git_sync_gemini_playwright_mcp() runs to completion before launching "browser-connection".
🤖 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
@.hermes/plans/2026-05-24_181145-integrate-rust-browser-connection-in-docker-git.md:
- Line 114: Validate the project_container chosen in
docker_git_start_rust_browser_connection (currently from
DOCKER_GIT_PROJECT_CONTAINER_NAME or fallback $(hostname)) against a safe
whitelist (e.g., only [A-Za-z0-9._-]) to prevent command injection; if
validation fails, echo an error to stderr, set MCP_PLAYWRIGHT_ENABLE=0 and
return 1, and when calling browser_lifecycle_bin use the quoted --project
"$project_container" and --network "container:${project_container}" forms so the
variable is never interpolated into an unquoted shell context.
- Around line 70-73: The Dockerfile RUN that uses cargo install --git ... (the
command installing rust-browser-connection) lacks authenticity/integrity checks;
update the build to verify authenticity by either (preferably) switching to a
signed git tag and verify its GPG signature before running cargo install
(validate tag signature for the repo/commit), or (minimally) add post-install
checksum verification of the installed binaries
(/usr/local/bin/docker-git-browser-connection and
/usr/local/bin/browser-connection) using known SHA256 values, or document and
enforce a controlled PR audit process to rotate the pinned rev; reference the
cargo install invocation and the two binaries mentioned to locate where to add
the verification steps.
- Line 409: Add a formal proof obligation and automated checks to ensure the
Rust lifecycle binary (the executable invoked by browser-connection) is
idempotent: guarantee that repeating start with the same --project/--network
does not create a new container but reuses an existing one; implement a
unit/integration test in the rust-browser-connection repo that asserts calling
start twice with identical flags leaves exactly one *-browser container; and add
a verification script and a Definition of Done entry that runs
`docker-git-browser-connection start && browser-connection --project ...
--no-start-browser` and asserts exactly one *-browser container exists; update
docs (AGENTS.md) to record this invariant and link to the test and proof.
- Around line 416-424: Update the "Definition of done" section to include the
formal requirements: add a property-based testing requirement (mention test glob
"**/*.test.{ts,tsx}" and an invariant test using fast-check with
projectIdArbitrary and getBrowserContainers() to assert one project → one
browser container), require automatic generation of proof artifacts (reference
AGENTS.md and example filenames like issue-347-proof.json and screenshots),
require README.md updates for the `--mcp-playwright` CLI flag semantics, add a
migration guide for users of the old wrapper, and require explicit
preconditions/postconditions and invariant-preservation notes for each plan step
(mention "Rust toolchain available", "cargo succeeds", "both binaries present
and executable", "old wrapper absent") so tests and docs tie to the stated
invariants.
In `@experiments/render-examples-output.txt`:
- Around line 8-15: Duplicate definitions of docker_git_decode_unicode_escapes()
are present; remove the duplicates and keep a single canonical definition (the
function docker_git_decode_unicode_escapes) located once near the top of the
file or moved into a shared prelude if this is a generated/template-based repo;
delete the other two definitions found in the Codex and Gemini sections so the
script uses the single authoritative implementation.
- Around line 398-434: The code in docker_git_sync_gemini_playwright_mcp needs
robust error handling and input validation: wrap fs.readFileSync + JSON.parse
(use settingsPath and parsed) in a try/catch that falls back to an empty
settings object and logs or comments the parse error; validate browserProject
before building browserArgs so you never push empty strings (only add
["--project", browserProject, "--network", ...] when browserProject.trim() !==
""), and ensure browserArgs uses process.env.DOCKER_GIT_BROWSER_NETWORK only
when browserProject is valid; keep the existing idempotent flow
(MCP_PLAYWRIGHT_ENABLE, nextServers, nextSettings, writeFileSync with mode
0o600) unchanged, and consider adding a corresponding docker_git_sync_grok_*
helper mirroring this logic for Grok.
In `@packages/app/src/docker-git/cli/usage.ts`:
- Line 80: The help text for the --mcp-playwright option is inconsistent: update
the usageText/Commands strings so the command description and the flag
description both reflect the new Rust browser-connection + noVNC/CDP lifecycle
(replace references to "nested Chromium" with the new wording), ensuring the
--mcp-playwright/--no-mcp-playwright flag description and the Commands section
(usageText) use the same phrasing and semantics for mcp-playwright and that
README semantics remain consistent with the Rust/noVNC/CDP runtime.
In `@packages/app/src/lib/core/templates-entrypoint/codex.ts`:
- Around line 55-60: DOCKER_GIT_BROWSER_PROJECT (sourced from
DOCKER_GIT_PROJECT_CONTAINER_NAME) is written unescaped into
CODEX_CONFIG_FILE/config.toml and can inject quotes or newlines breaking
mcp_servers.playwright; sanitize the value before writing by validating and/or
escaping unsafe characters (remove or reject newlines, strip or escape quotes
and control chars, or enforce a safe whitelist like [A-Za-z0-9._-]) and use the
sanitized variable when composing the TOML entry; update all places referencing
DOCKER_GIT_BROWSER_PROJECT (including the similar usage at the other occurrence
around lines 124-125) to use the sanitized value so the TOML stays well-formed
and injection-safe.
In `@packages/lib/src/core/templates-entrypoint/codex.ts`:
- Around line 55-59: The value assigned to DOCKER_GIT_BROWSER_PROJECT (sourced
from DOCKER_GIT_PROJECT_CONTAINER_NAME) is written into TOML unescaped and can
corrupt config.toml/MCP-block; before writing or embedding
DOCKER_GIT_BROWSER_PROJECT, sanitize/escape it (e.g., remove or percent-encode
newlines, control chars, TOML-special chars like quotes/backslashes, or use a
safe quoting/escaping routine) so it becomes a valid TOML string; update the
assignment/serialization logic around
DOCKER_GIT_BROWSER_PROJECT/DOCKER_GIT_PROJECT_CONTAINER_NAME to perform this
sanitization and then write the sanitized value to config.toml/MCP-block.
In `@packages/lib/tests/core/templates.test.ts`:
- Around line 784-785: The test currently only checks
expect(entrypoint).not.toContain('"$DOCKER_GIT_PROJECT_CONTAINER_NAME"') which
misses unsafe usages like ${DOCKER_GIT_PROJECT_CONTAINER_NAME} without a
default; update the assertion for entrypoint to reject both the quoted
"$DOCKER_GIT_PROJECT_CONTAINER_NAME" case and any bare brace expansion form
(e.g., ${DOCKER_GIT_PROJECT_CONTAINER_NAME}) that does not include a safe
default (':-'), so strengthen the check to assert entrypoint does not contain
the quoted form and does not match the unbound-brace pattern for
DOCKER_GIT_PROJECT_CONTAINER_NAME.
---
Outside diff comments:
In `@experiments/render-examples-output.txt`:
- Around line 333-383: defaultGeminiSettings currently defines
mcpServers.playwright (with command "browser-connection" and empty args) while
docker_git_sync_gemini_playwright_mcp() adds/updates that same
mcpServers.playwright at runtime, creating two sources of truth; pick one
approach and make it explicit: either remove mcpServers.playwright from the
defaultGeminiSettings (so docker_git_sync_gemini_playwright_mcp() is the
canonical source) or keep it but document with an inline comment and initialize
its args to the same placeholder/dynamic format used by
docker_git_sync_gemini_playwright_mcp(); update the code in the module that
defines defaultGeminiSettings (and the experiments-generated settings) so both
use the same initialization strategy and ensure
docker_git_sync_gemini_playwright_mcp() still overwrites or merges consistently.
---
Duplicate comments:
In `@packages/lib/src/usecases/auth-gemini-helpers.ts`:
- Around line 299-306: The mcpServers.playwright block currently uses args: []
which defers required CLI params to runtime and creates a race; either populate
the args array with the expected parameters (e.g. include --project and
--network values) in the static config for the "browser-connection" command or
ensure docker_git_sync_gemini_playwright_mcp() is invoked and completes before
starting the Gemini CLI so the MCP servers receive their parameters; update the
mcpServers.playwright configuration or the startup flow that calls the Gemini
CLI (the code path that reads this mcpServers entry) to guarantee
docker_git_sync_gemini_playwright_mcp() runs to completion before launching
"browser-connection".
In `@packages/lib/src/usecases/auth-grok-helpers.ts`:
- Around line 237-246: The defaultGrokProjectSettings object is missing runtime
synchronization of Playwright MCP args for --project and --network, so update
the Playwright config to include and populate args and add a small sync helper
(or call-site logic) that injects the current project and network flags into
defaultGrokProjectSettings.mcpServers.playwright.args at runtime; specifically,
ensure the mcpServers.playwright.args array (and any creation path that uses
defaultGrokProjectSettings) receives ["--project", currentProject, "--network",
currentNetwork] (or omits each flag when value is empty) via a function like
syncGrokArgs or inline logic where settings are instantiated so Grok’s MCP
Playwright can connect to the browser.
🪄 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: cfa3a362-0661-4df4-93e8-f12886962c14
📒 Files selected for processing (33)
.hermes/plans/2026-05-24_181145-integrate-rust-browser-connection-in-docker-git.mdexperiments/render-examples-output.txtpackages/app/src/docker-git/cli/usage.tspackages/app/src/lib/core/templates-entrypoint/base.tspackages/app/src/lib/core/templates-entrypoint/claude.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/tasks.tspackages/app/src/lib/core/templates.tspackages/app/src/lib/core/templates/dockerfile-prelude.tspackages/app/src/lib/core/templates/dockerfile.tspackages/app/src/lib/usecases/actions/prepare-files.tspackages/app/src/lib/usecases/auth-gemini-helpers.tspackages/app/src/lib/usecases/auth-grok-helpers.tspackages/app/tests/docker-git/core-templates.test.tspackages/lib/src/core/templates-entrypoint/base.tspackages/lib/src/core/templates-entrypoint/claude.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-entrypoint/tasks.tspackages/lib/src/core/templates.tspackages/lib/src/core/templates/dockerfile-prelude.tspackages/lib/src/core/templates/dockerfile.tspackages/lib/src/usecases/actions/prepare-files.tspackages/lib/src/usecases/auth-gemini-helpers.tspackages/lib/src/usecases/auth-grok-helpers.tspackages/lib/tests/core/templates.test.tspackages/lib/tests/usecases/auth-gemini.test.tspackages/lib/tests/usecases/auth-grok.test.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/lib/tests/usecases/prepare-files.test.ts
💤 Files with no reviewable changes (6)
- packages/app/src/lib/usecases/actions/prepare-files.ts
- packages/app/src/lib/core/templates-entrypoint/base.ts
- packages/lib/src/core/templates-entrypoint/base.ts
- packages/app/src/lib/core/templates/dockerfile.ts
- packages/lib/src/usecases/actions/prepare-files.ts
- packages/lib/src/core/templates/dockerfile.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). (5)
- GitHub Check: E2E (Login context)
- GitHub Check: E2E (Clone auto-open SSH)
- GitHub Check: E2E (Clone cache)
- GitHub Check: E2E (Runtime volumes + SSH)
- GitHub Check: E2E (OpenCode)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{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/lib/tests/usecases/auth-gemini.test.tspackages/lib/tests/usecases/auth-grok.test.tspackages/lib/src/usecases/auth-grok-helpers.tspackages/lib/tests/usecases/prepare-files.test.tspackages/app/src/docker-git/cli/usage.tspackages/lib/src/usecases/auth-gemini-helpers.tspackages/app/src/lib/usecases/auth-gemini-helpers.tspackages/app/src/lib/usecases/auth-grok-helpers.tspackages/app/src/lib/core/templates-entrypoint/grok.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/app/src/lib/core/templates-entrypoint/codex.tspackages/app/tests/docker-git/core-templates.test.tspackages/app/src/lib/core/templates-entrypoint/tasks.tspackages/app/src/lib/core/templates/dockerfile-prelude.tspackages/lib/src/core/templates-entrypoint/grok.tspackages/app/src/lib/core/templates.tspackages/lib/src/core/templates-entrypoint/tasks.tspackages/lib/src/core/templates-entrypoint/gemini.tspackages/lib/src/core/templates/dockerfile-prelude.tspackages/app/src/lib/core/templates-entrypoint/gemini.tspackages/app/src/lib/core/templates-entrypoint/claude.tspackages/lib/src/core/templates-entrypoint/codex.tspackages/lib/src/core/templates.tspackages/lib/tests/core/templates.test.tspackages/lib/src/core/templates-entrypoint/claude.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/auth-gemini.test.tspackages/lib/tests/usecases/auth-grok.test.tspackages/lib/tests/usecases/prepare-files.test.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/app/tests/docker-git/core-templates.test.tspackages/lib/tests/core/templates.test.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/lib/tests/usecases/auth-gemini.test.tspackages/lib/tests/usecases/auth-grok.test.tspackages/lib/src/usecases/auth-grok-helpers.tspackages/lib/tests/usecases/prepare-files.test.tspackages/app/src/docker-git/cli/usage.tspackages/lib/src/usecases/auth-gemini-helpers.tspackages/app/src/lib/usecases/auth-gemini-helpers.tspackages/app/src/lib/usecases/auth-grok-helpers.tspackages/app/src/lib/core/templates-entrypoint/grok.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/app/src/lib/core/templates-entrypoint/codex.tspackages/app/tests/docker-git/core-templates.test.tspackages/app/src/lib/core/templates-entrypoint/tasks.tspackages/app/src/lib/core/templates/dockerfile-prelude.tspackages/lib/src/core/templates-entrypoint/grok.tspackages/app/src/lib/core/templates.tspackages/lib/src/core/templates-entrypoint/tasks.tspackages/lib/src/core/templates-entrypoint/gemini.tspackages/lib/src/core/templates/dockerfile-prelude.tspackages/app/src/lib/core/templates-entrypoint/gemini.tspackages/app/src/lib/core/templates-entrypoint/claude.tspackages/lib/src/core/templates-entrypoint/codex.tspackages/lib/src/core/templates.tspackages/lib/tests/core/templates.test.tspackages/lib/src/core/templates-entrypoint/claude.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/auth-gemini.test.tspackages/lib/tests/usecases/auth-grok.test.tspackages/lib/tests/usecases/prepare-files.test.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/app/tests/docker-git/core-templates.test.tspackages/lib/tests/core/templates.test.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/lib/tests/usecases/auth-gemini.test.tspackages/lib/tests/usecases/auth-grok.test.tspackages/lib/src/usecases/auth-grok-helpers.tspackages/lib/tests/usecases/prepare-files.test.tspackages/app/src/docker-git/cli/usage.tspackages/lib/src/usecases/auth-gemini-helpers.tspackages/app/src/lib/usecases/auth-gemini-helpers.tspackages/app/src/lib/usecases/auth-grok-helpers.tspackages/app/src/lib/core/templates-entrypoint/grok.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/app/src/lib/core/templates-entrypoint/codex.tspackages/app/tests/docker-git/core-templates.test.tspackages/app/src/lib/core/templates-entrypoint/tasks.tspackages/app/src/lib/core/templates/dockerfile-prelude.tspackages/lib/src/core/templates-entrypoint/grok.tspackages/app/src/lib/core/templates.tspackages/lib/src/core/templates-entrypoint/tasks.tspackages/lib/src/core/templates-entrypoint/gemini.tspackages/lib/src/core/templates/dockerfile-prelude.tspackages/app/src/lib/core/templates-entrypoint/gemini.tspackages/app/src/lib/core/templates-entrypoint/claude.tspackages/lib/src/core/templates-entrypoint/codex.tspackages/lib/src/core/templates.tspackages/lib/tests/core/templates.test.tspackages/lib/src/core/templates-entrypoint/claude.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/lib/tests/usecases/auth-gemini.test.tspackages/lib/tests/usecases/auth-grok.test.tspackages/lib/src/usecases/auth-grok-helpers.tspackages/lib/tests/usecases/prepare-files.test.tspackages/app/src/docker-git/cli/usage.tspackages/lib/src/usecases/auth-gemini-helpers.tspackages/app/src/lib/usecases/auth-gemini-helpers.tspackages/app/src/lib/usecases/auth-grok-helpers.tspackages/app/src/lib/core/templates-entrypoint/grok.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/app/src/lib/core/templates-entrypoint/codex.tspackages/app/tests/docker-git/core-templates.test.tspackages/app/src/lib/core/templates-entrypoint/tasks.tspackages/app/src/lib/core/templates/dockerfile-prelude.tspackages/lib/src/core/templates-entrypoint/grok.tspackages/app/src/lib/core/templates.tspackages/lib/src/core/templates-entrypoint/tasks.tspackages/lib/src/core/templates-entrypoint/gemini.tspackages/lib/src/core/templates/dockerfile-prelude.tspackages/app/src/lib/core/templates-entrypoint/gemini.tspackages/app/src/lib/core/templates-entrypoint/claude.tspackages/lib/src/core/templates-entrypoint/codex.tspackages/lib/src/core/templates.tspackages/lib/tests/core/templates.test.tspackages/lib/src/core/templates-entrypoint/claude.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/lib/tests/usecases/auth-gemini.test.tspackages/lib/tests/usecases/auth-grok.test.tspackages/lib/src/usecases/auth-grok-helpers.tspackages/lib/tests/usecases/prepare-files.test.tspackages/app/src/docker-git/cli/usage.tspackages/lib/src/usecases/auth-gemini-helpers.tspackages/app/src/lib/usecases/auth-gemini-helpers.tspackages/app/src/lib/usecases/auth-grok-helpers.tspackages/app/src/lib/core/templates-entrypoint/grok.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/app/src/lib/core/templates-entrypoint/codex.tspackages/app/tests/docker-git/core-templates.test.tspackages/app/src/lib/core/templates-entrypoint/tasks.tspackages/app/src/lib/core/templates/dockerfile-prelude.tspackages/lib/src/core/templates-entrypoint/grok.tspackages/app/src/lib/core/templates.tspackages/lib/src/core/templates-entrypoint/tasks.tspackages/lib/src/core/templates-entrypoint/gemini.tspackages/lib/src/core/templates/dockerfile-prelude.tspackages/app/src/lib/core/templates-entrypoint/gemini.tspackages/app/src/lib/core/templates-entrypoint/claude.tspackages/lib/src/core/templates-entrypoint/codex.tspackages/lib/src/core/templates.tspackages/lib/tests/core/templates.test.tspackages/lib/src/core/templates-entrypoint/claude.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Ты строгий ревьюер SPEC DRIVEN DEVELOPMENT.Перед выводами изучи README.md, другие *.md файлы, linked issues,
PR description, PR comments/discussion и релевантную кодовую базу.Сверь изменения с исходным ТЗ/спекой и обсуждением. Флагай любой уход
от спеки, недокументированное изменение поведения, отсутствие тестов
для заявленного поведения и security-риск. Если спека не видна,
попроси автора добавить ее в issue или PR description.Проверь решение с точки зрения формальной верификации: какие инварианты,
предусловия и постусловия можно доказать математически, а где доказуемость
слабая. Оцени решение с точки зрения теории игр: устойчивы ли стимулы,
нет ли выгодного обхода правил, и какое решение было бы сильнее.
Files:
packages/lib/tests/usecases/auth-gemini.test.tspackages/lib/tests/usecases/auth-grok.test.tspackages/lib/src/usecases/auth-grok-helpers.tspackages/lib/tests/usecases/prepare-files.test.tspackages/app/src/docker-git/cli/usage.tspackages/lib/src/usecases/auth-gemini-helpers.tspackages/app/src/lib/usecases/auth-gemini-helpers.tspackages/app/src/lib/usecases/auth-grok-helpers.tspackages/app/src/lib/core/templates-entrypoint/grok.tspackages/lib/tests/usecases/mcp-playwright.test.tspackages/app/src/lib/core/templates-entrypoint/codex.tspackages/app/tests/docker-git/core-templates.test.tspackages/app/src/lib/core/templates-entrypoint/tasks.tspackages/app/src/lib/core/templates/dockerfile-prelude.tspackages/lib/src/core/templates-entrypoint/grok.tspackages/app/src/lib/core/templates.tspackages/lib/src/core/templates-entrypoint/tasks.tspackages/lib/src/core/templates-entrypoint/gemini.tspackages/lib/src/core/templates/dockerfile-prelude.tspackages/app/src/lib/core/templates-entrypoint/gemini.tspackages/app/src/lib/core/templates-entrypoint/claude.tspackages/lib/src/core/templates-entrypoint/codex.tspackages/lib/src/core/templates.tspackages/lib/tests/core/templates.test.tspackages/lib/src/core/templates-entrypoint/claude.tsexperiments/render-examples-output.txt
packages/app/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (README.md)
App layer (APP) should work only with API and not have direct access to LIB layer
Files:
packages/app/src/docker-git/cli/usage.tspackages/app/src/lib/usecases/auth-gemini-helpers.tspackages/app/src/lib/usecases/auth-grok-helpers.tspackages/app/src/lib/core/templates-entrypoint/grok.tspackages/app/src/lib/core/templates-entrypoint/codex.tspackages/app/tests/docker-git/core-templates.test.tspackages/app/src/lib/core/templates-entrypoint/tasks.tspackages/app/src/lib/core/templates/dockerfile-prelude.tspackages/app/src/lib/core/templates.tspackages/app/src/lib/core/templates-entrypoint/gemini.tspackages/app/src/lib/core/templates-entrypoint/claude.ts
🧠 Learnings (1)
📚 Learning: 2026-05-22T21:08:18.083Z
Learnt from: skulidropek
Repo: ProverCoderAI/docker-git PR: 344
File: packages/app/src/docker-git/controller-compose.ts:34-40
Timestamp: 2026-05-22T21:08:18.083Z
Learning: In this repo’s docker-git controller compose generation, `${DOCKER_GIT_CONTROLLER_BUILD_SKILLER:-1}` should be treated as standard bash parameter expansion: when `DOCKER_GIT_CONTROLLER_BUILD_SKILLER` is unset, it defaults to the string "1". There is no "-1" mode. The runtime contract enforced by `packages/app/src/docker-git/controller-compose.ts` is: unset / "1" / "true" => output "1"; "0" / "false" => output "0". If review code shows branching/behavior for "-1" or any numeric value other than this 0/1 contract, flag it. Also ensure the Dockerfile ARG `DOCKER_GIT_CONTROLLER_BUILD_SKILLER` stays consistent with default `1`.
Applied to files:
packages/app/src/docker-git/cli/usage.ts
🪛 LanguageTool
.hermes/plans/2026-05-24_181145-integrate-rust-browser-connection-in-docker-git.md
[typographical] ~410-~410: Непарный символ: «‘» скорей всего пропущен
Context: ...tics are now the Rust MCP implementation's tools, not upstream Playwright MCP. Th...
(RU_UNPAIRED_BRACKETS)
🪛 markdownlint-cli2 (0.22.1)
.hermes/plans/2026-05-24_181145-integrate-rust-browser-connection-in-docker-git.md
[warning] 300-300: Ordered list item prefix
Expected: 1; Actual: 2; Style: 1/1/1
(MD029, ol-prefix)
[warning] 306-306: Ordered list item prefix
Expected: 1; Actual: 3; Style: 1/1/1
(MD029, ol-prefix)
[warning] 314-314: Ordered list item prefix
Expected: 1; Actual: 4; Style: 1/1/1
(MD029, ol-prefix)
[warning] 320-320: Ordered list item prefix
Expected: 1; Actual: 5; Style: 1/1/1
(MD029, ol-prefix)
[warning] 335-335: Ordered list item prefix
Expected: 1; Actual: 6; Style: 1/1/1
(MD029, ol-prefix)
🔇 Additional comments (17)
packages/app/tests/docker-git/core-templates.test.ts (1)
60-63: Добавьте проверку отсутствияmcp-playwright-start-extra.shв этом же наборе legacy-assertions.Сейчас на Line 60-63 проверяются не все legacy-артефакты, и регрессия по
mcp-playwright-start-extra.shостанется незамеченной.As per coding guidelines
**/*:Флагай ... отсутствие тестов для заявленного поведения..hermes/plans/2026-05-24_181145-integrate-rust-browser-connection-in-docker-git.md (1)
168-172: ⚡ Quick winПрименить ту же валидацию для переменных браузера.
Вычисление
DOCKER_GIT_BROWSER_PROJECTиDOCKER_GIT_BROWSER_NETWORKиспользует тот же fallback наhostnameбез валидации. Применить те же меры защиты от command/injection, что и дляproject_container(см. комментарий к lines 114, 122-125).Дополнительно: убедиться, что node snippets, записывающие эти значения в JSON/TOML конфиги, экранируют спецсимволы для предотвращения JSON injection (хотя если node использует proper JSON serialization API, риск минимален).
packages/app/src/lib/core/templates-entrypoint/tasks.ts (1)
292-292: LGTM!packages/lib/src/core/templates-entrypoint/tasks.ts (1)
292-292: LGTM!packages/app/src/lib/core/templates.ts (1)
16-18: LGTM!packages/lib/src/core/templates.ts (1)
15-17: LGTM!packages/app/src/lib/core/templates/dockerfile-prelude.ts (1)
60-63: LGTM!Also applies to: 78-80, 92-94
packages/lib/src/core/templates/dockerfile-prelude.ts (1)
60-63: LGTM!Also applies to: 78-80, 92-94
packages/app/src/lib/core/templates-entrypoint/claude.ts (1)
196-198: LGTM!Also applies to: 203-203, 207-207, 215-215, 222-223
packages/app/src/lib/core/templates-entrypoint/gemini.ts (1)
203-205: LGTM!Also applies to: 218-219, 222-222
packages/app/src/lib/core/templates-entrypoint/grok.ts (1)
188-193: LGTM!Also applies to: 206-208, 211-211
packages/lib/src/core/templates-entrypoint/claude.ts (1)
195-197: LGTM!Also applies to: 202-202, 206-207, 214-214, 221-222
packages/lib/src/core/templates-entrypoint/gemini.ts (1)
202-204: LGTM!Also applies to: 217-218, 221-221
packages/lib/src/core/templates-entrypoint/grok.ts (1)
187-192: LGTM!Also applies to: 205-207, 210-210
experiments/render-examples-output.txt (1)
8-15: Паттерн Unicode-экранирования реализован корректно.Замена кириллических строк на
\uXXXX-последовательности с последующим декодированием черезprintf "%b"решает проблемы с locale/encoding в shell-окружении. Реализация последовательна для Claude, Codex и Gemini:
- ✅ Декодирование происходит после присваивания переменных (строки 66, 161, 518).
- ✅ Условная проверка
grep -q '\\u[0-9a-fA-F]'предотвращает избыточную обработку.- ✅ Паттерны
sed(строки 193-200) обновлены для удаления как старых, так и новых форматов.Минорное замечание: строка 213 добавляет дополнительный перевод строки при декодировании
AGENTS_PATH, что может создать лишнюю пустую строку в конце файла.Also applies to: 17-66, 98-161, 193-216, 464-518
packages/app/src/lib/usecases/auth-gemini-helpers.ts (1)
300-306: ⚖️ Poor tradeoffСмягчение:
args: []само по себе не проблема, но важна гарантия порядка выполнения sync
- В
packages/app/src/lib/usecases/auth-gemini-helpers.tsдляdefaultGeminiSettings.mcpServers.playwrightзаданоcommand: "browser-connection", args: [].- В entrypoint-инициализации присутствует вызов
docker_git_sync_gemini_playwright_mcp(), который приMCP_PLAYWRIGHT_ENABLE === "1"перезаписываетmcpServers.playwrightнаargs: ["--project", <project>, "--network", "container:<project>"](а при отключённом флаге — удаляетplaywrightизmcpServers).- Следовательно, зависимость от пустых
argsпроявится только еслиsettings.jsonбудет прочитан Gemini CLI до выполненияdocker_git_sync_gemini_playwright_mcp()(или если sync фактически не отработает по условиям/ошибке).Нужна явная гарантия в каркасе entrypoint, что запуск Gemini CLI происходит после патча
settings.json(желательно с тестом/проверкой порядка).packages/app/src/lib/usecases/auth-grok-helpers.ts (1)
238-246: ⚡ Quick winGrok: нужна проверка runtime-синхронизации args для MCP Playwright
ВdefaultGrokProjectSettings.mcpServers.playwrightдля Grok заданоcommand: "browser-connection"и пустыеargs: []. Выполненный поиск не обнаружилdocker_git_sync_grok*в.sh/.txtи не нашёлGROK.*settingsвexperiments/, поэтому runtime-переопределениеargsдля Grok не подтверждено. Убедиться, что приMCP_PLAYWRIGHT_ENABLE=1в Grok entrypoint формируются обязательныеargsдляbrowser-connection(--projectи--network, как минимум аналогично Gemini); иначе Grok MCP Playwright не сможет подключиться к браузеру.
Comment on lines
+70
to
+73
| RUN cargo install --git https://github.com/ProverCoderAI/rust-browser-connection --rev c36f263ebc5d0acdf155113914f08cafefa69c56 --locked --bins --root /usr/local \ | ||
| && /usr/local/bin/docker-git-browser-connection --version \ | ||
| && /usr/local/bin/browser-connection --version | ||
| ``` |
There was a problem hiding this comment.
Добавить integrity verification для cargo install из GitHub.
cargo install --git с pinned rev обеспечивает reproducibility, но не authenticity/integrity. Если GitHub repo скомпрометирован или rev force-pushed, generated образы могут содержать malicious code.
Рекомендации (по возрастанию строгости):
- Предпочтительно: Использовать signed git tags вместо raw rev и проверять signature перед install.
- Минимально: Добавить checksum verification бинарников после установки:
RUN cargo install --git ... --locked --bins --root /usr/local \ && echo "<expected-sha256> /usr/local/bin/docker-git-browser-connection" | sha256sum -c - \ && echo "<expected-sha256> /usr/local/bin/browser-connection" | sha256sum -c -
- Альтернатива: Документировать процесс регулярного аудита pinned rev и обновления через controlled PR workflow.
Без integrity check это security posture gap, как указано в coding guidelines для dependency management.
🤖 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
@.hermes/plans/2026-05-24_181145-integrate-rust-browser-connection-in-docker-git.md
around lines 70 - 73, The Dockerfile RUN that uses cargo install --git ... (the
command installing rust-browser-connection) lacks authenticity/integrity checks;
update the build to verify authenticity by either (preferably) switching to a
signed git tag and verify its GPG signature before running cargo install
(validate tag signature for the repo/commit), or (minimally) add post-install
checksum verification of the installed binaries
(/usr/local/bin/docker-git-browser-connection and
/usr/local/bin/browser-connection) using known SHA256 values, or document and
enforce a controlled PR audit process to rotate the pinned rev; reference the
cargo install invocation and the two binaries mentioned to locate where to add
the verification steps.
|
|
||
| - lifecycle binary ищется как `/usr/local/bin/docker-git-browser-connection` или через `command -v`; | ||
| - browser стартует только если `MCP_PLAYWRIGHT_ENABLE=1`; | ||
| - project берётся из `DOCKER_GIT_PROJECT_CONTAINER_NAME`, fallback `hostname`; |
There was a problem hiding this comment.
Валидировать project_container и hostname против command injection.
Fallback на $(hostname) (line 114) и передача $project_container в shell команду (lines 122-125) могут быть небезопасны, если hostname содержит спецсимволы, newlines или escape sequences. В Docker hostname может быть установлен через --hostname flag и частично контролируется пользователем.
Рекомендуемые меры:
# В функции docker_git_start_rust_browser_connection добавить validation:
project_container="${DOCKER_GIT_PROJECT_CONTAINER_NAME:-$(hostname)}"
# Validate: только alphanumeric, dash, underscore, dot (типичные Docker container names)
if ! echo "$project_container" | grep -qE '^[a-zA-Z0-9._-]+$'; then
echo "ERROR: Invalid project container name: $project_container" >&2
export MCP_PLAYWRIGHT_ENABLE=0
return 1
fi
# Затем использовать quoted variable безопасно
"$browser_lifecycle_bin" start --project "$project_container" --network "container:${project_container}"Как указано в coding guidelines: "Fail if changed files introduce command injection or unsafe shell/process execution with user-controlled input".
Also applies to: 122-125
🤖 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
@.hermes/plans/2026-05-24_181145-integrate-rust-browser-connection-in-docker-git.md
at line 114, Validate the project_container chosen in
docker_git_start_rust_browser_connection (currently from
DOCKER_GIT_PROJECT_CONTAINER_NAME or fallback $(hostname)) against a safe
whitelist (e.g., only [A-Za-z0-9._-]) to prevent command injection; if
validation fails, echo an error to stderr, set MCP_PLAYWRIGHT_ENABLE=0 and
return 1, and when calling browser_lifecycle_bin use the quoted --project
"$project_container" and --network "container:${project_container}" forms so the
variable is never interpolated into an unquoted shell context.
|
|
||
| ## Risks / tradeoffs / open questions | ||
|
|
||
| - `browser-connection` starts the browser on MCP server startup. If entrypoint also calls `docker-git-browser-connection start`, this is still safe only if Rust lifecycle is idempotent. This should be verified in runtime smoke. |
There was a problem hiding this comment.
Идемпотентность lifecycle не может быть "open question" — требуется proof obligation.
Риск двойного старта браузера (entrypoint вызывает docker-git-browser-connection start + MCP server вызывает browser-connection который тоже стартует браузер) является критичным для инварианта "один проект = один browser контейнер" (line 10). Проверка идемпотентности отложена на runtime smoke test, но это должно быть формальным требованием с:
- Proof obligation: Rust lifecycle binary гарантирует, что повторный
startс теми же--project/--networkне создаёт новый контейнер (reuses existing). - Test requirement: Unit/integration test в
rust-browser-connectionrepo, подтверждающий идемпотентность. - Verification script: В Definition of Done добавить проверку, что после
docker-git-browser-connection start && browser-connection --project ... --no-start-browserсуществует ровно один-browserконтейнер.
Без этих гарантий миграция может нарушить core invariant проекта.
Как указано в AGENTS.md guidance: "document invariants and expected before/after properties for runtime behavior" и "back them with tests".
🤖 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
@.hermes/plans/2026-05-24_181145-integrate-rust-browser-connection-in-docker-git.md
at line 409, Add a formal proof obligation and automated checks to ensure the
Rust lifecycle binary (the executable invoked by browser-connection) is
idempotent: guarantee that repeating start with the same --project/--network
does not create a new container but reuses an existing one; implement a
unit/integration test in the rust-browser-connection repo that asserts calling
start twice with identical flags leaves exactly one *-browser container; and add
a verification script and a Definition of Done entry that runs
`docker-git-browser-connection start && browser-connection --project ...
--no-start-browser` and asserts exactly one *-browser container exists; update
docs (AGENTS.md) to record this invariant and link to the test and proof.
Comment on lines
+416
to
+424
| ## Definition of done | ||
|
|
||
| - `rg "docker-git-playwright-mcp|@playwright/mcp" packages` has no runtime hits. | ||
| - Generated Dockerfile installs both Rust binaries. | ||
| - Generated MCP configs use `browser-connection`. | ||
| - Generated entrypoint delegates browser lifecycle to `docker-git-browser-connection`. | ||
| - Tests/typecheck pass. | ||
| - Runtime smoke proves MCP navigation changes the same noVNC-visible Chromium session. | ||
| - PR is open against docker-git `main` with green CI. |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Дополнить Definition of Done формальными требованиями из guidelines.
Текущий DoD неполон с точки зрения spec-driven development и formal verification requirements. Необходимо добавить:
-
Property-based testing (coding guideline для
**/*.test.{ts,tsx}): Автоматический тест инварианта "один проект → один browser контейнер" через fast-check:fc.property(fc.array(projectIdArbitrary), (projectIds) => { // For each unique project, exactly one browser container exists const containers = getBrowserContainers(); return new Set(projectIds).size === containers.length; });
-
Proof artifacts (AGENTS.md requirement): Recorded evidence как в issue
#347(issue-347-proof.json, screenshots) должны быть автоматически генерируемы через test suite, не только manual smoke test. -
Documentation updates: Обновление README.md секции про
--mcp-playwrightflag, если CLI semantics изменились (referenced guidance из README.md: "CLI flag contract must be aligned with new implementation"). -
Migration guide: Инструкция для пользователей с existing projects на старом wrapper (если они существуют).
-
Preconditions/Postconditions: Для каждого Step в плане задокументировать:
- Precondition: что должно быть true перед выполнением (e.g., "Rust toolchain available", "cargo succeeds")
- Postcondition: что гарантируется после (e.g., "both binaries present and executable", "old wrapper absent")
- Invariant preservation: как step сохраняет core invariants
Как указано в AGENTS.md: "document invariants and expected before/after properties for runtime behavior and back them with tests".
🤖 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
@.hermes/plans/2026-05-24_181145-integrate-rust-browser-connection-in-docker-git.md
around lines 416 - 424, Update the "Definition of done" section to include the
formal requirements: add a property-based testing requirement (mention test glob
"**/*.test.{ts,tsx}" and an invariant test using fast-check with
projectIdArbitrary and getBrowserContainers() to assert one project → one
browser container), require automatic generation of proof artifacts (reference
AGENTS.md and example filenames like issue-347-proof.json and screenshots),
require README.md updates for the `--mcp-playwright` CLI flag semantics, add a
migration guide for users of the old wrapper, and require explicit
preconditions/postconditions and invariant-preservation notes for each plan step
(mention "Rust toolchain available", "cargo succeeds", "both binaries present
and executable", "old wrapper absent") so tests and docs tie to the stated
invariants.
Comment on lines
+8
to
+15
| docker_git_decode_unicode_escapes() { | ||
| local value="$1" | ||
| if printf "%s" "$value" | grep -q '\\u[0-9a-fA-F]'; then | ||
| printf "%b" "$value" | ||
| else | ||
| printf "%s" "$value" | ||
| fi | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Дублирование функции docker_git_decode_unicode_escapes().
Функция docker_git_decode_unicode_escapes() определена три раза (строки 8-15 для Claude, 98-105 для Codex, 464-471 для Gemini). В bash более поздние определения переопределяют предыдущие, но:
- Увеличивается размер файла и сложность поддержки.
- Если это генерируемый файл из шаблонов — рассмотрите вынос функции в общий prelude.
- Если ручной — объедините в одно определение в начале файла.
♻️ Возможный рефакторинг
Переместите единственное определение в начало файла перед всеми секциями:
================================================================================
+# Common utilities
+================================================================================
+
+docker_git_decode_unicode_escapes() {
+ local value="$1"
+ if printf "%s" "$value" | grep -q '\\u[0-9a-fA-F]'; then
+ printf "%b" "$value"
+ else
+ printf "%s" "$value"
+ fi
+}
+
+================================================================================
CLAUDE.md prompt setup (~/.claude/CLAUDE.md)
================================================================================
-docker_git_decode_unicode_escapes() {
- local value="$1"
- if printf "%s" "$value" | grep -q '\\u[0-9a-fA-F]'; then
- printf "%b" "$value"
- else
- printf "%s" "$value"
- fi
-}
CLAUDE_AUTO_SYSTEM_PROMPT="${CLAUDE_AUTO_SYSTEM_PROMPT:-1}"(Аналогично удалите из Codex и Gemini секций.)
Also applies to: 98-105, 464-471
🤖 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 `@experiments/render-examples-output.txt` around lines 8 - 15, Duplicate
definitions of docker_git_decode_unicode_escapes() are present; remove the
duplicates and keep a single canonical definition (the function
docker_git_decode_unicode_escapes) located once near the top of the file or
moved into a shared prelude if this is a generated/template-based repo; delete
the other two definitions found in the Codex and Gemini sections so the script
uses the single authoritative implementation.
Comment on lines
+398
to
+434
| # Gemini CLI: keep Playwright MCP config in sync with container settings | ||
| docker_git_sync_gemini_playwright_mcp() { | ||
| local browser_project="${DOCKER_GIT_PROJECT_CONTAINER_NAME:-}"; [[ -n "$browser_project" ]] || browser_project="$(hostname)" | ||
| local browser_network="container:$browser_project" | ||
| GEMINI_CONFIG_SETTINGS_FILE="$GEMINI_CONFIG_SETTINGS_FILE" MCP_PLAYWRIGHT_ENABLE="${MCP_PLAYWRIGHT_ENABLE:-0}" DOCKER_GIT_BROWSER_PROJECT="$browser_project" DOCKER_GIT_BROWSER_NETWORK="$browser_network" node - <<'NODE' | ||
| const fs = require("node:fs") | ||
| const path = require("node:path") | ||
| const settingsPath = process.env.GEMINI_CONFIG_SETTINGS_FILE | ||
| const isRecord = (value) => typeof value === "object" && value !== null && !Array.isArray(value) | ||
| if (typeof settingsPath !== "string" || settingsPath.length === 0) process.exit(0) | ||
|
|
||
| let settings = {} | ||
| try { | ||
| const parsed = JSON.parse(fs.readFileSync(settingsPath, "utf8")) | ||
| if (isRecord(parsed)) settings = parsed | ||
| } catch {} | ||
|
|
||
| const browserProject = process.env.DOCKER_GIT_BROWSER_PROJECT || "" | ||
| const browserArgs = browserProject.length > 0 ? ["--project", browserProject, "--network", process.env.DOCKER_GIT_BROWSER_NETWORK || "container:" + browserProject] : [] | ||
| const nextServers = { ...(isRecord(settings.mcpServers) ? settings.mcpServers : {}) } | ||
| if (process.env.MCP_PLAYWRIGHT_ENABLE === "1") { | ||
| nextServers.playwright = { command: "browser-connection", args: browserArgs, trust: true } | ||
| } else { | ||
| delete nextServers.playwright | ||
| } | ||
|
|
||
| const nextSettings = { ...settings } | ||
| Object.keys(nextServers).length > 0 ? nextSettings.mcpServers = nextServers : delete nextSettings.mcpServers | ||
|
|
||
| if (JSON.stringify(settings) === JSON.stringify(nextSettings)) process.exit(0) | ||
|
|
||
| fs.mkdirSync(path.dirname(settingsPath), { recursive: true }) | ||
| fs.writeFileSync(settingsPath, JSON.stringify(nextSettings, null, 2) + "\n", { mode: 0o600 }) | ||
| NODE | ||
| } | ||
|
|
||
| docker_git_sync_gemini_playwright_mcp |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Функция docker_git_sync_gemini_playwright_mcp() корректно реализует динамическую синхронизацию.
Данная функция заполняет пробел пустого args: [] из auth-gemini-helpers.ts, динамически добавляя --project и --network в settings.json. Реализация идемпотентна и безопасна:
- ✅ Использует
DOCKER_GIT_PROJECT_CONTAINER_NAMEилиhostnameдля определения проекта. - ✅ Записывает только при изменениях (строка 427).
- ✅ Устанавливает права 0o600 (строка 430).
- ✅ Команда
"browser-connection"соответствуетauth-gemini-helpers.ts.
Однако:
- Отсутствует обработка ошибок:
JSON.parse()(строка 411) завершится с исключением при невалидном JSON, процесс молча завершится. Рассмотрите обёртку вtry/catchс fallback. - Нет валидации
browserProject: еслиDOCKER_GIT_PROJECT_CONTAINER_NAMEиhostnameпусты,argsбудет содержать пустые строки. - Только для Gemini: отсутствует аналогичная функция для Grok (см. критические комментарии к
auth-grok-helpers.ts).
🛡️ Рекомендуемое улучшение обработки ошибок
try {
const parsed = JSON.parse(fs.readFileSync(settingsPath, "utf8"))
if (isRecord(parsed)) settings = parsed
-} catch {}
+} catch (err) {
+ console.error("docker_git_sync_gemini_playwright_mcp: failed to parse settings.json:", err.message)
+ process.exit(0)
+}
const browserProject = process.env.DOCKER_GIT_BROWSER_PROJECT || ""
+if (browserProject.length === 0) {
+ console.warn("docker_git_sync_gemini_playwright_mcp: DOCKER_GIT_BROWSER_PROJECT is empty, skipping MCP config")
+ process.exit(0)
+}
const browserArgs = browserProject.length > 0 ? ["--project", browserProject, "--network", process.env.DOCKER_GIT_BROWSER_NETWORK || "container:" + browserProject] : []🤖 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 `@experiments/render-examples-output.txt` around lines 398 - 434, The code in
docker_git_sync_gemini_playwright_mcp needs robust error handling and input
validation: wrap fs.readFileSync + JSON.parse (use settingsPath and parsed) in a
try/catch that falls back to an empty settings object and logs or comments the
parse error; validate browserProject before building browserArgs so you never
push empty strings (only add ["--project", browserProject, "--network", ...]
when browserProject.trim() !== ""), and ensure browserArgs uses
process.env.DOCKER_GIT_BROWSER_NETWORK only when browserProject is valid; keep
the existing idempotent flow (MCP_PLAYWRIGHT_ENABLE, nextServers, nextSettings,
writeFileSync with mode 0o600) unchanged, and consider adding a corresponding
docker_git_sync_grok_* helper mirroring this logic for Grok.
| --up | --no-up Run docker compose up after init (default: --up) | ||
| --ssh | --no-ssh Auto-open SSH after create/clone (default: clone=--ssh, create=--no-ssh) | ||
| --mcp-playwright | --no-mcp-playwright Enable Playwright MCP + nested Chromium browser (default: --no-mcp-playwright) | ||
| --mcp-playwright | --no-mcp-playwright Enable Rust browser MCP + noVNC/CDP session (default: --no-mcp-playwright) |
There was a problem hiding this comment.
Синхронизируйте и описание команды mcp-playwright в help.
На Line 80 опция уже описана через новый Rust/noVNC/CDP runtime, но раздел Commands на Line 29 всё ещё обещает nested Chromium. Сейчас usageText публикует два разных контракта для одной и той же функции.
As per coding guidelines, README.md требует, чтобы семантика --mcp-playwright оставалась согласованной с новым browser-connection + noVNC/CDP lifecycle.
🤖 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/app/src/docker-git/cli/usage.ts` at line 80, The help text for the
--mcp-playwright option is inconsistent: update the usageText/Commands strings
so the command description and the flag description both reflect the new Rust
browser-connection + noVNC/CDP lifecycle (replace references to "nested
Chromium" with the new wording), ensuring the
--mcp-playwright/--no-mcp-playwright flag description and the Commands section
(usageText) use the same phrasing and semantics for mcp-playwright and that
README semantics remain consistent with the Rust/noVNC/CDP runtime.
Comment on lines
55
to
+60
| CODEX_CONFIG_FILE="__CODEX_HOME__/config.toml" | ||
| DOCKER_GIT_BROWSER_PROJECT="${"$"}{DOCKER_GIT_PROJECT_CONTAINER_NAME:-}" | ||
| if [[ -z "$DOCKER_GIT_BROWSER_PROJECT" ]]; then | ||
| DOCKER_GIT_BROWSER_PROJECT="$(hostname)" | ||
| fi | ||
| DOCKER_GIT_BROWSER_NETWORK="container:$DOCKER_GIT_BROWSER_PROJECT" |
There was a problem hiding this comment.
Санитизируйте DOCKER_GIT_BROWSER_PROJECT перед записью в TOML.
Значение из DOCKER_GIT_PROJECT_CONTAINER_NAME попадает в config.toml без фильтрации. Символы вроде "/переносов могут сломать блок mcp_servers.playwright и подменить итоговую MCP-конфигурацию.
Предлагаемый фикс
DOCKER_GIT_BROWSER_PROJECT="${DOCKER_GIT_PROJECT_CONTAINER_NAME:-}"
if [[ -z "$DOCKER_GIT_BROWSER_PROJECT" ]]; then
DOCKER_GIT_BROWSER_PROJECT="$(hostname)"
fi
+DOCKER_GIT_BROWSER_PROJECT="$(printf "%s" "$DOCKER_GIT_BROWSER_PROJECT" | tr -cd '[:alnum:]._-')"
+if [[ -z "$DOCKER_GIT_BROWSER_PROJECT" ]]; then
+ DOCKER_GIT_BROWSER_PROJECT="$(hostname | tr -cd '[:alnum:]._-')"
+fi
DOCKER_GIT_BROWSER_NETWORK="container:$DOCKER_GIT_BROWSER_PROJECT"As per coding guidelines "**/*.{sh,bash,py,js,ts,jsx,tsx,go,java,rb,php}: Fail if changed files introduce command injection or unsafe shell/process execution with user-controlled input".
Also applies to: 124-125
🤖 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/app/src/lib/core/templates-entrypoint/codex.ts` around lines 55 -
60, DOCKER_GIT_BROWSER_PROJECT (sourced from DOCKER_GIT_PROJECT_CONTAINER_NAME)
is written unescaped into CODEX_CONFIG_FILE/config.toml and can inject quotes or
newlines breaking mcp_servers.playwright; sanitize the value before writing by
validating and/or escaping unsafe characters (remove or reject newlines, strip
or escape quotes and control chars, or enforce a safe whitelist like
[A-Za-z0-9._-]) and use the sanitized variable when composing the TOML entry;
update all places referencing DOCKER_GIT_BROWSER_PROJECT (including the similar
usage at the other occurrence around lines 124-125) to use the sanitized value
so the TOML stays well-formed and injection-safe.
Comment on lines
+55
to
+59
| DOCKER_GIT_BROWSER_PROJECT="${"$"}{DOCKER_GIT_PROJECT_CONTAINER_NAME:-}" | ||
| if [[ -z "$DOCKER_GIT_BROWSER_PROJECT" ]]; then | ||
| DOCKER_GIT_BROWSER_PROJECT="$(hostname)" | ||
| fi | ||
| DOCKER_GIT_BROWSER_NETWORK="container:$DOCKER_GIT_BROWSER_PROJECT" |
There was a problem hiding this comment.
Санитизируйте DOCKER_GIT_BROWSER_PROJECT перед записью в TOML.
Значение из DOCKER_GIT_PROJECT_CONTAINER_NAME вставляется в TOML без экранирования. При спецсимволах можно повредить config.toml и исказить MCP-блок.
Предлагаемый фикс
DOCKER_GIT_BROWSER_PROJECT="${DOCKER_GIT_PROJECT_CONTAINER_NAME:-}"
if [[ -z "$DOCKER_GIT_BROWSER_PROJECT" ]]; then
DOCKER_GIT_BROWSER_PROJECT="$(hostname)"
fi
+DOCKER_GIT_BROWSER_PROJECT="$(printf "%s" "$DOCKER_GIT_BROWSER_PROJECT" | tr -cd '[:alnum:]._-')"
+if [[ -z "$DOCKER_GIT_BROWSER_PROJECT" ]]; then
+ DOCKER_GIT_BROWSER_PROJECT="$(hostname | tr -cd '[:alnum:]._-')"
+fi
DOCKER_GIT_BROWSER_NETWORK="container:$DOCKER_GIT_BROWSER_PROJECT"As per coding guidelines "**/*.{sh,bash,py,js,ts,jsx,tsx,go,java,rb,php}: Fail if changed files introduce command injection or unsafe shell/process execution with user-controlled input".
Also applies to: 123-124
🤖 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-entrypoint/codex.ts` around lines 55 - 59,
The value assigned to DOCKER_GIT_BROWSER_PROJECT (sourced from
DOCKER_GIT_PROJECT_CONTAINER_NAME) is written into TOML unescaped and can
corrupt config.toml/MCP-block; before writing or embedding
DOCKER_GIT_BROWSER_PROJECT, sanitize/escape it (e.g., remove or percent-encode
newlines, control chars, TOML-special chars like quotes/backslashes, or use a
safe quoting/escaping routine) so it becomes a valid TOML string; update the
assignment/serialization logic around
DOCKER_GIT_BROWSER_PROJECT/DOCKER_GIT_PROJECT_CONTAINER_NAME to perform this
sanitization and then write the sanitized value to config.toml/MCP-block.
Comment on lines
+784
to
+785
| expect(entrypoint).not.toContain('"$DOCKER_GIT_PROJECT_CONTAINER_NAME"') | ||
| }) |
There was a problem hiding this comment.
Усилите проверку unbound-переменной для ${DOCKER_GIT_PROJECT_CONTAINER_NAME}.
На Line 784 текущий not.toContain('"$DOCKER_GIT_PROJECT_CONTAINER_NAME"') ловит только один частный случай и не покрывает небезопасный ${DOCKER_GIT_PROJECT_CONTAINER_NAME} без :-.
Предложение по усилению проверки
- expect(entrypoint).not.toContain('"$DOCKER_GIT_PROJECT_CONTAINER_NAME"')
+ expect(entrypoint).not.toContain('"$DOCKER_GIT_PROJECT_CONTAINER_NAME"')
+ expect(entrypoint).not.toContain("${DOCKER_GIT_PROJECT_CONTAINER_NAME}")📝 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.
Suggested change
| expect(entrypoint).not.toContain('"$DOCKER_GIT_PROJECT_CONTAINER_NAME"') | |
| }) | |
| expect(entrypoint).not.toContain('"$DOCKER_GIT_PROJECT_CONTAINER_NAME"') | |
| expect(entrypoint).not.toContain("${DOCKER_GIT_PROJECT_CONTAINER_NAME}") | |
| }) |
🤖 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/tests/core/templates.test.ts` around lines 784 - 785, The test
currently only checks
expect(entrypoint).not.toContain('"$DOCKER_GIT_PROJECT_CONTAINER_NAME"') which
misses unsafe usages like ${DOCKER_GIT_PROJECT_CONTAINER_NAME} without a
default; update the assertion for entrypoint to reject both the quoted
"$DOCKER_GIT_PROJECT_CONTAINER_NAME" case and any bare brace expansion form
(e.g., ${DOCKER_GIT_PROJECT_CONTAINER_NAME}) that does not include a safe
default (':-'), so strengthen the check to assert entrypoint does not contain
the quoted form and does not match the unbound-brace pattern for
DOCKER_GIT_PROJECT_CONTAINER_NAME.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #347.
This PR finishes the Rust-only browser/noVNC/MCP integration for docker-git:
docker-git-playwright-mcp/@playwright/mcpwrapper pathProverCoderAI/rust-browser-connectionwith an immutable pinned rev:c36f263ebc5d0acdf155113914f08cafefa69c56docker-git-browser-connectionfor browser lifecycle startupbrowser-connection--project <project-container>and--network container:<project-container>so MCP and noVNC share the same Rust-managed browser containerset -uby falling back from${DOCKER_GIT_PROJECT_CONTAINER_NAME:-}tohostnameRuntime proof
Fresh local smoke test against
/home/dev/rust-browser-connectionproved MCP + CDP + noVNC on the same Chromium/X11 session:dg-rust-browser-smoke-185718dg-rust-browser-smoke-185718-browser1MCP_NOVNC_RUNTIME_PROOF_1779649038browser-connection0.3.0browser_navigate,browser_snapshot,browser_evaluate,browser_click,browser_type,browser_press_key,browser_take_screenshotbrowser_evaluatereturned the marker from the page title/body/json/listreturned the same page title/URLRFB 003.008Local proof artifacts:
/tmp/dg-rust-browser-smoke-185718-proof/summary.txt/tmp/dg-rust-browser-smoke-185718-proof/mcp-verify-stdout.jsonl/tmp/dg-rust-browser-smoke-185718-proof/cdp-list.txt/tmp/dg-rust-browser-smoke-185718-proof/novnc-rfb.txt/tmp/dg-rust-browser-smoke-185718-proof/x11-framebuffer.pngVerification
Local checks after the final fixes:
bun run typecheck— passedbun run test— passed (52lib test files,267tests)bun run lint— passedgit diff --check— passed0findingsdocker-git-playwright-mcp/@playwright/mcp/ old browser runtime files outside negative tests and the implementation planNotes
The browser runtime remains in the separate
ProverCoderAI/rust-browser-connectionrepo. docker-git only installs and invokes the published Rust binaries; there is no local Rust source copy or TS browser-runtime duplication in this PR.