fix(api): stabilize skiller and browser launch#430
Conversation
skulidropek
commented
Jun 18, 2026
skulidropek
commented
- Route external Skiller launches through short backend records and active project filtering.
- Repair browser sidecar startup so noVNC scales cleanly and Chromium opens about:blank without first-run/restore screens.
- Proof of fix: packages/api browser/skiller tests passed, packages/app browser action tests passed, API build/lint passed, and live noVNC/CDP flow was verified.
- Route external Skiller launches through short backend records and active project filtering. - Repair browser sidecar startup so noVNC scales cleanly and Chromium opens about:blank without first-run/restore screens. - Proof of fix: packages/api browser/skiller tests passed, packages/app browser action tests passed, API build/lint passed, and live noVNC/CDP flow was verified.
📝 WalkthroughSummary by CodeRabbitПримечания к выпуску
WalkthroughРеализован полный поток внешнего запуска Skiller Web: контроллер формирует ChangesВнешний запуск Skiller Web и формирование backendUrl
Browser window manager repair и прочие исправления
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
note over Browser, Registry: Внешний запуск Skiller Web
end
participant Browser as Браузер
participant HttpRouter as HTTP Router
participant resolveSkillerBackendUrl as resolveSkillerBackendUrl
participant CloudflareTunnel as Cloudflare Quick Tunnel
participant openSkiller as openSkiller
participant Registry as externalSkillerLaunches
Browser->>HttpRouter: POST /skiller/open
HttpRouter->>resolveSkillerBackendUrl: request (x-forwarded-prefix, origin, env)
alt DOCKER_GIT_SKILLER_BACKEND_URL задан
resolveSkillerBackendUrl-->>HttpRouter: Configured backendUrl
else origin HTTPS
resolveSkillerBackendUrl-->>HttpRouter: Request backendUrl
else локальный HTTP + внешний Skiller Web
resolveSkillerBackendUrl->>CloudflareTunnel: startPanelCloudflareTunnel
CloudflareTunnel-->>resolveSkillerBackendUrl: publicUrl (*.trycloudflare.com)
resolveSkillerBackendUrl-->>HttpRouter: Tunnel backendUrl
end
HttpRouter->>openSkiller: openSkiller(backendUrl, projectKey, sessionId)
openSkiller->>Registry: registerExternalSkillerLaunch(targetUrl, ttl)
Registry-->>openSkiller: launchId
openSkiller-->>HttpRouter: { mode: external, appPath: /api/skiller/external-launch/<id> }
HttpRouter-->>Browser: { appPath, backendUrl, mode: external }
Browser->>HttpRouter: GET /api/skiller/external-launch/<id>
HttpRouter->>Registry: readExternalSkillerLaunchHtml(launchId)
Registry-->>HttpRouter: HTML (iframe → external Skiller Web URL)
HttpRouter-->>Browser: text/html с iframe
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/api/tests/skiller-core.test.ts (1)
52-127: 🧹 Nitpick | 🔵 TrivialДобавьте property-based проверки математических инвариантов вместо только фиксированных примеров.
В тестовом файле функции
escapeHtmlAttribute,joinSkillerBackendUrl,resolveDockerGitSkillerBackendUrlDecisionиactiveSkillerConnectProjectsпроверяются только конкретными примерами. Вместо этого используйтеfc.propertyдля верификации инвариантов:
- escapeHtmlAttribute: идемпотентность (
escape(escape(x)) === escape(x)), безопасное экранирование спецсимволов- joinSkillerBackendUrl: корректная склейка URL при любых валидных входах (результат должен содержать оба компонента в правильном порядке)
- activeSkillerConnectProjects: фильтрует только по
status === "running", сохраняет порядок,length(result) ≤ length(input)- resolveDockerGitSkillerBackendUrlDecision: всегда возвращает валидный discriminated union (Configured | Request | Tunnel), без undefined/null
Fast-check 4.8.0 уже установлен в проекте. Property-based тесты обнаружат граничные случаи, которые фиксированные примеры упускают.
🤖 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/api/tests/skiller-core.test.ts` around lines 52 - 127, The test suite currently uses only fixed example-based assertions for functions like escapeHtmlAttribute, joinSkillerBackendUrl, resolveDockerGitSkillerBackendUrlDecision, and activeSkillerConnectProjects, which may miss edge cases. Replace these fixed-example tests with property-based tests using fast-check's fc.property to verify mathematical invariants: for escapeHtmlAttribute verify idempotence (applying escape twice yields same result as once) and proper HTML entity encoding, for joinSkillerBackendUrl verify correct concatenation with both components present in proper order, for activeSkillerConnectProjects verify filtering only includes running status items while preserving order and maintains length(result) <= length(input), and for resolveDockerGitSkillerBackendUrlDecision verify it always returns a valid discriminated union type without undefined or null values. Use fast-check 4.8.0 which is already installed to generate arbitrary valid inputs.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/integrations/skiller.md`:
- Around line 43-63: Add a formal "Proof of fix" section and "Математические
гарантии" (Mathematical guarantees) subsection to the Skiller documentation
after the environment variables explanation and before the legacy bundled mode
details. The "Proof of fix" block should document the root cause of the external
launch/tunnel flow issue, the solution implemented, and proof that relevant
tests pass. The "Математические гарантии" section should specify the invariants
(properties that must always hold), preconditions (requirements before the flow
executes), postconditions (guaranteed results after execution), and complexity
analysis for the external Skiller Web URL callback mechanism and automatic
Cloudflare tunnel selection logic. This formalizes the behavioral guarantees for
strict verification against the SDD process requirements.
In `@packages/api/src/http.ts`:
- Around line 653-656: The validation in the dockerGitApiPort function is too
permissive. The regex pattern /^\d{1,5}$/u accepts port numbers up to 99999, but
valid TCP port numbers must be in the range 0-65535. Fix the validation logic to
ensure the configured port number is both in the correct numeric format and
within the valid port range. Convert the string to a number and check that it
falls within the valid port range (0-65535) before returning it as a valid port.
In `@packages/api/src/services/project-browser.ts`:
- Around line 356-359: The condition in the Effect.flatMap callback that checks
output.includes("changed") has a substring matching issue where "unchanged" also
contains "changed" as a substring, causing false positives and unnecessary
container restarts. Fix this by replacing the includes("changed") check with a
more precise comparison, such as checking output === "changed" for exact
matching, or using a regex pattern that checks for the word "changed" as a
standalone word rather than as a substring. This will ensure that "unchanged"
outputs do not trigger the restartBrowserContainer call in the conditional block
where restartBrowserContainer, cwd, and containerName are used.
In `@packages/api/src/services/skiller.ts`:
- Line 516: The patch-marker caching mechanism in the build condition (line 516)
only checks for file existence, not content changes. When the content of
$DOCKER_GIT_SKILLER_PATCH changes, the existing marker remains valid and
prevents rebuilds from triggering. To fix this, implement content-based
validation by either: (1) modifying the condition at line 516 to compare a hash
of the current patch content against a stored hash in the marker file, or (2)
adding logic in the skiller-apply-docker-git-patches.mjs script to remove or
invalidate the $DOCKER_GIT_SKILLER_PATCH_MARKER when patch content changes are
detected, ensuring that subsequent build conditions properly trigger when the
patch file is modified.
In `@packages/api/tests/skiller-cors.test.ts`:
- Around line 168-193: The test "normalizes safe forwarded prefixes and ignores
unsafe ones for external launch URLs" currently verifies only three hardcoded
examples instead of using property-based testing. Convert this test to use
fast-check's fc.property to verify the mathematical invariant that safe prefixes
are normalized correctly into the backendUrl. Generate a variety of safe
prefixes (like different path patterns) and unsafe prefixes (like full URLs with
schemes) using fast-check generators, and verify that each input produces the
expected normalized backendUrl output. Keep the Effect.runPromise and
requestExternalLaunch structure but apply the property-based testing approach
across multiple generated inputs rather than fixed test cases.
- Around line 58-63: When restoring environment variables from previousEntries,
the current forward iteration order can cause incorrect state restoration if any
key appears multiple times in the entries. Reverse the iteration order of
previousEntries so that duplicated keys are processed in reverse chronological
order, ensuring the most recent state is restored first and preventing
intermediate or stale values from leaking into subsequent tests. Modify the for
loop iteration in the Effect.sync block where setOptionalEnv is called to
iterate through previousEntries in reverse order instead of forward order.
In `@packages/api/tests/skiller-routes.test.ts`:
- Around line 90-95: Replace the async test function with Effect test utilities.
Change the test declaration from `it(...)` to `it.effect(...)` and replace the
`async () => { await Effect.runPromise(...) }` pattern with an `Effect.gen()`
function. Inside the generator, use `yield*` to unwrap Effect operations instead
of calling `await Effect.runPromise()` directly. Apply this pattern to compose
all Effect-based operations like `openSkiller()` and
`readExternalSkillerLaunchTarget()` to align with the Effect testing guidelines
used in other test files in the repository.
---
Outside diff comments:
In `@packages/api/tests/skiller-core.test.ts`:
- Around line 52-127: The test suite currently uses only fixed example-based
assertions for functions like escapeHtmlAttribute, joinSkillerBackendUrl,
resolveDockerGitSkillerBackendUrlDecision, and activeSkillerConnectProjects,
which may miss edge cases. Replace these fixed-example tests with property-based
tests using fast-check's fc.property to verify mathematical invariants: for
escapeHtmlAttribute verify idempotence (applying escape twice yields same result
as once) and proper HTML entity encoding, for joinSkillerBackendUrl verify
correct concatenation with both components present in proper order, for
activeSkillerConnectProjects verify filtering only includes running status items
while preserving order and maintains length(result) <= length(input), and for
resolveDockerGitSkillerBackendUrlDecision verify it always returns a valid
discriminated union type without undefined or null values. Use fast-check 4.8.0
which is already installed to generate arbitrary valid inputs.
🪄 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: 87b5e29a-a634-4f54-8686-aeb046718bb5
📒 Files selected for processing (21)
README.mddocker-compose.api.ymldocker-compose.ymldocs/integrations/skiller.mdpackages/api/src/http.tspackages/api/src/services/project-browser-core.tspackages/api/src/services/project-browser.tspackages/api/src/services/skiller-core.tspackages/api/src/services/skiller.tspackages/api/src/services/terminal-sessions.tspackages/api/tests/project-browser.test.tspackages/api/tests/project-port-forward-core.test.tspackages/api/tests/skiller-core.test.tspackages/api/tests/skiller-cors.test.tspackages/api/tests/skiller-routes.test.tspackages/api/tests/terminal-sessions.test.tspackages/app/scripts/serve-dist-web-forwarding.mjspackages/app/scripts/serve-dist-web.mjspackages/app/tests/docker-git/actions-skiller.test.tspackages/app/tests/docker-git/controller-resource-limits.test.tspackages/app/tests/docker-git/serve-dist-web.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 (Runtime volumes + SSH)
- GitHub Check: E2E (Login context)
- GitHub Check: E2E (OpenCode)
- GitHub Check: E2E (Clone auto-open SSH)
- GitHub Check: E2E (Clone cache)
🧰 Additional context used
📓 Path-based instructions (11)
**/*.{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:
docker-compose.ymlpackages/api/tests/project-port-forward-core.test.tspackages/api/tests/skiller-core.test.tsdocker-compose.api.ymlpackages/app/tests/docker-git/controller-resource-limits.test.tspackages/api/tests/project-browser.test.tspackages/api/src/services/project-browser-core.tspackages/app/tests/docker-git/actions-skiller.test.tspackages/api/src/services/terminal-sessions.tspackages/api/tests/skiller-routes.test.tspackages/app/tests/docker-git/serve-dist-web.test.tspackages/api/src/services/project-browser.tspackages/api/tests/terminal-sessions.test.tspackages/api/tests/skiller-cors.test.tspackages/api/src/services/skiller.tspackages/api/src/http.tspackages/api/src/services/skiller-core.ts
**/{Dockerfile*,docker-compose*.{yml,yaml},.dockerignore}
📄 CodeRabbit inference engine (Custom checks)
Fail if changed files introduce unsafe Docker configuration such as privileged containers, broad host mounts, unbounded Docker socket access, or unnecessary write permissions
Files:
docker-compose.ymldocker-compose.api.yml
**/*
⚙️ CodeRabbit configuration file
**/*: Ты строгий ревьюер SPEC DRIVEN DEVELOPMENT.Перед выводами изучи README.md, другие *.md файлы, linked issues,
PR description, PR comments/discussion и релевантную кодовую базу.Сверь изменения с исходным ТЗ/спекой и обсуждением. Флагай любой уход
от спеки, недокументированное изменение поведения, отсутствие тестов
для заявленного поведения и security-риск. Если спека не видна,
попроси автора добавить ее в issue или PR description.Проверь решение с точки зрения формальной верификации: какие инварианты,
предусловия и постусловия можно доказать математически, а где доказуемость
слабая. Оцени решение с точки зрения теории игр: устойчивы ли стимулы,
нет ли выгодного обхода правил, и какое решение было бы сильнее.
Files:
docker-compose.ymlpackages/api/tests/project-port-forward-core.test.tspackages/api/tests/skiller-core.test.tsdocker-compose.api.ymlpackages/app/tests/docker-git/controller-resource-limits.test.tspackages/api/tests/project-browser.test.tspackages/api/src/services/project-browser-core.tspackages/app/tests/docker-git/actions-skiller.test.tspackages/api/src/services/terminal-sessions.tspackages/api/tests/skiller-routes.test.tspackages/app/tests/docker-git/serve-dist-web.test.tsREADME.mdpackages/app/scripts/serve-dist-web-forwarding.mjspackages/api/src/services/project-browser.tspackages/api/tests/terminal-sessions.test.tsdocs/integrations/skiller.mdpackages/api/tests/skiller-cors.test.tspackages/app/scripts/serve-dist-web.mjspackages/api/src/services/skiller.tspackages/api/src/http.tspackages/api/src/services/skiller-core.ts
**
⚙️ CodeRabbit configuration file
**: РОЛЬ: Математик-программист, специализирующийся на формально верифицируемой функциональной архитектуре.ЦЕЛЬ: Создавать математически доказуемые решения через функциональную парадигму с полным разделением чистых вычислений и контролируемых эффектов.
МОДЕЛЬ РАССУЖДЕНИЯ:
- Не выдавать “личные мнения”. Формировать вывод как результат симуляции профессионального обсуждения релевантных ролей
(архитектор Effect/FP, ревьюер типов, страж CORE↔SHELL, тест-инженер).- Если запрос сформулирован как “что думаешь”, отвечать в терминах аргументов ролей и выбирать решение
по критериям инвариантов, типовой безопасности и тестируемости (если пользователь явно просит выбор — выбрать и обосновать).ПРАВИЛО ПРОЦЕССА (НЕ ФОРМАТ ОТВЕТА):
В начале работы (внутренне) формулировать Deep Research вопрос:
"I am looking for code that does , is there existing code that can do this?"
Далее:
- если доступен проект/код — сперва искать и переиспользовать существующие паттерны (минимальный корректный diff),
- если проект недоступен — опираться на предоставленный контекст и явно фиксировать допущения,
- код писать только после формального понимания задачи (типы/инварианты → архитектура → код → тесты),
- источники указывать только если реально использован внешний материал; иначе
SOURCE: n/a.ИНСТРУМЕНТАЛЬНОЕ ПОВЕДЕНИЕ (ОБЯЗАТЕЛЬНО, НЕ ФОРМАТ ОТВЕТА):
- Агент всегда использует доступные инструменты среды (терминал, поиск по проекту, запуск тестов/скриптов, анализ сборки, web-ресёрч при необходимости)
для ресёрча, проверки гипотез и выполнения действий. Приоритет: проверяемость, воспроизводимость, минимальный риск.- Агент не предлагает “гайд” как замену действия. Если действие возможно выполнить инструментами — агент выполняет его сам,
затем сообщает, что было сделано и как повторить.- Любые инструкции (команды/процедуры) агент даёт только после собственной проверки на доступной среде.
Если проверить невозможно — явно фиксирует ограничение и перечисляе...
Files:
docker-compose.ymlpackages/api/tests/project-port-forward-core.test.tspackages/api/tests/skiller-core.test.tsdocker-compose.api.ymlpackages/app/tests/docker-git/controller-resource-limits.test.tspackages/api/tests/project-browser.test.tspackages/api/src/services/project-browser-core.tspackages/app/tests/docker-git/actions-skiller.test.tspackages/api/src/services/terminal-sessions.tspackages/api/tests/skiller-routes.test.tspackages/app/tests/docker-git/serve-dist-web.test.tsREADME.mdpackages/app/scripts/serve-dist-web-forwarding.mjspackages/api/src/services/project-browser.tspackages/api/tests/terminal-sessions.test.tsdocs/integrations/skiller.mdpackages/api/tests/skiller-cors.test.tspackages/app/scripts/serve-dist-web.mjspackages/api/src/services/skiller.tspackages/api/src/http.tspackages/api/src/services/skiller-core.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/api/tests/project-port-forward-core.test.tspackages/api/tests/skiller-core.test.tspackages/app/tests/docker-git/controller-resource-limits.test.tspackages/api/tests/project-browser.test.tspackages/api/src/services/project-browser-core.tspackages/app/tests/docker-git/actions-skiller.test.tspackages/api/src/services/terminal-sessions.tspackages/api/tests/skiller-routes.test.tspackages/app/tests/docker-git/serve-dist-web.test.tspackages/api/src/services/project-browser.tspackages/api/tests/terminal-sessions.test.tspackages/api/tests/skiller-cors.test.tspackages/api/src/services/skiller.tspackages/api/src/http.tspackages/api/src/services/skiller-core.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/api/tests/project-port-forward-core.test.tspackages/api/tests/skiller-core.test.tspackages/app/tests/docker-git/controller-resource-limits.test.tspackages/api/tests/project-browser.test.tspackages/api/src/services/project-browser-core.tspackages/app/tests/docker-git/actions-skiller.test.tspackages/api/src/services/terminal-sessions.tspackages/api/tests/skiller-routes.test.tspackages/app/tests/docker-git/serve-dist-web.test.tspackages/api/src/services/project-browser.tspackages/api/tests/terminal-sessions.test.tspackages/api/tests/skiller-cors.test.tspackages/api/src/services/skiller.tspackages/api/src/http.tspackages/api/src/services/skiller-core.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: FUNCTIONAL CORE: Write only pure functions with immutable data and mathematical operations in core modules; no side effects, mutations, or external service calls
IMPERATIVE SHELL: Isolate all side effects (IO, network, database, environment/process) in a thin SHELL layer; CORE never calls SHELL, only SHELL → CORE
Never useanytype annotation in TypeScript; useunknownonly at SHELL boundaries for decoding, never exportunknownoutside boundary modules
Never useastype assertions in normal code; only permitasin a single 'axiomatic' module (brands, constructors, constants) after which types flow safely without casts
Always use exhaustive pattern matching for union types through.exhaustive()orMatch.exhaustive()from effect-ts; never use switch statements or unhandled type branches
Use Effect<Success, Error, Requirements> monad from effect-ts for all effects; compose through pipe() and Effect.flatMap(); never use async/await, raw Promise chains (then/catch), or Promise.all in product code
Interoperate with Promise/exceptions only in SHELL through Effect.try/Effect.tryPromise with typed error mapping; never leave raw exceptions or untyped errors in the domain
Use Effect.acquireRelease + Effect.scoped for resource management with guaranteed finalization; never manage resources with try/finally or manual cleanup
All external services (database, HTTP, environment) must be accessed through Effect-based interfaces and Layer-based dependency injection; never call external APIs directly
Provide comprehensive TSDoc comments with mathematical notation:@pure,@effect,@invariant,@precondition,@postcondition,@complexity,@throws, and CHANGE/WHY/REF/SOURCE/FORMAT THEOREM functional comment markers
No console.*, process direct calls, or untyped environment access in product code; all such operations must be abstracted through Layer-based services in SHELL
Boundary data from external sources (HTTP, database, environment) must be decoded/valida...
Files:
packages/api/tests/project-port-forward-core.test.tspackages/api/tests/skiller-core.test.tspackages/app/tests/docker-git/controller-resource-limits.test.tspackages/api/tests/project-browser.test.tspackages/api/src/services/project-browser-core.tspackages/app/tests/docker-git/actions-skiller.test.tspackages/api/src/services/terminal-sessions.tspackages/api/tests/skiller-routes.test.tspackages/app/tests/docker-git/serve-dist-web.test.tspackages/api/src/services/project-browser.tspackages/api/tests/terminal-sessions.test.tspackages/api/tests/skiller-cors.test.tspackages/api/src/services/skiller.tspackages/api/src/http.tspackages/api/src/services/skiller-core.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.{ts,tsx}: Write property-based tests using fast-check (fc.property) to verify mathematical invariants; unit tests must use Effect test utilities without async/await
Every bug fix must be accompanied by a reproducing test case; the test must fail before the fix and pass after; document the Proof of Fix with root cause and solution
Files:
packages/api/tests/project-port-forward-core.test.tspackages/api/tests/skiller-core.test.tspackages/app/tests/docker-git/controller-resource-limits.test.tspackages/api/tests/project-browser.test.tspackages/app/tests/docker-git/actions-skiller.test.tspackages/api/tests/skiller-routes.test.tspackages/app/tests/docker-git/serve-dist-web.test.tspackages/api/tests/terminal-sessions.test.tspackages/api/tests/skiller-cors.test.ts
*.md
📄 CodeRabbit inference engine (AGENTS.md)
Document all PR changes with mathematical proof obligations: invariants, preconditions, postconditions, variant functions, and complexity analysis in Markdown format
Files:
README.md
docs/integrations/**
⚙️ CodeRabbit configuration file
docs/integrations/**: # Skiller IntegrationSkiller is included as an isolated git submodule so docker-git can reuse the upstream desktop skills manager without mixing Electron dependencies into the docker-git Bun workspace.
Upstream
- Repository: https://github.com/beautyfree/skiller-desktop-skills-manager
- Path:
third_party/skiller-desktop-skills-manager- Pinned version:
v0.2.14- Pinned commit:
6ff6b9ca1ff2d78d3af7dac47b03ed1c315dab6b- License: MIT, copyright 2025 Skiller Contributors
The submodule is intentionally outside
packages/*and is not listed in the root workspace. This keeps the existing docker-gitbuild,check,typecheck, andtestscripts scoped to docker-git packages unless a Skiller-specific script is run.Commands
Initialize the pinned submodule:
bun run skiller:initInstall Skiller dependencies inside the submodule:
bun run skiller:installRun Skiller as its own Electron app:
bun run skiller:devRun Skiller checks:
bun run skiller:checkdocker-git Web Launch
The docker-git web terminal header includes a
Skillerbutton next toOpen browser. In a project terminal the button callsPOST /projects/by-key/:projectKey/terminal-sessions/:sessionId/skiller/openfirst, which launches the pinned submodule Electron app as a separate process, registers the terminal session filesystem scope, and writes launcher output to~/.docker-git/logs/skiller.log. After that response succeeds, the browser opens the returned/api/ssh/session/:sessionId/skiller/app/URL using the same terminal session id that is present in/ssh/session/:sessionId.docker-git serves Skiller's built renderer from the submodule and proxies
/api/ssh/session/:sessionId/skiller/trpc/*to the running Skiller tRPC backend, so the user sees the actual Skiller UI instead of an invisible background desktop process. The session id is part of the URL so a Skiller tab can be tied back to the terminal...
Files:
docs/integrations/skiller.md
docs/**
⚙️ CodeRabbit configuration file
docs/**: # Процесс разработки (SDD: Spec-Driven Development)Этот документ описывает канонический цикл разработки в
docker-git: от issue до
проверяемого PR. Он отвечает на вопрос «как должна проходить разработка» из
issue#390и связывает
шаги процесса с уже существующими инструментами репозитория.Базовая философия задана в
AGENTS.mdиCLAUDE.md:«Если это нельзя доказать — это нельзя доверить продакшену.»
Каждая функция — теорема, каждый тест — доказательство, каждый тип — утверждение.SDD означает, что сначала формализуем (спецификация + инварианты), потом
программируем. Спецификация и её инварианты живут вместе с кодом и проверяются
автоматически.Обзор цикла
issue ──▶ clone (среда) ──▶ /plan ──▶ уточнение ТЗ + Deep Research │ │ │ ▼ │ Story + Prove (инварианты проверяемости) │ │ │ апрув плана ──▶ plan-to-git ──▶ PR (память) │ │ │ ▼ └──────────────── разработка (код + тесты + Playwright MCP) │ ToDos + subagents (декомпозиция) │ ▼ CI/CD ──▶ сверка с инвариантами плана ──▶ merge1. Issue — источник истины
Разработка всегда начинается с issue. Issue фиксирует намерение и становится
точкой привязки для плана, PR и итоговой проверки инвариантов.2. Среда: клонирование с изоляцией
Для каждой issue поднимается отдельное Docker-окружение. Браузерная автоматизация
включается флагом--mcp-playwright(Playwright MCP + Chromium sidecar):bun run docker-git clone https://...
Files:
docs/integrations/skiller.md
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: ProverCoderAI/docker-git
Timestamp: 2026-06-18T16:07:56.571Z
Learning: Use Bun as the package manager and runtime for the docker-git project
Learnt from: CR
Repo: ProverCoderAI/docker-git
Timestamp: 2026-06-18T16:07:56.571Z
Learning: Store authentication tokens in environment variables with the format `GIT_AUTH_TOKEN__<HOST>` and `GIT_AUTH_USER__<HOST>`
Learnt from: CR
Repo: ProverCoderAI/docker-git
Timestamp: 2026-06-18T16:07:56.571Z
Learning: Web version should listen on `0.0.0.0` by default for LAN accessibility, controlled via `DOCKER_GIT_WEB_HOST` environment variable
Learnt from: CR
Repo: ProverCoderAI/docker-git
Timestamp: 2026-06-18T16:07:56.571Z
Learning: Store `backendUrl`, `projectKey`, and `sessionId` in short launch-wrapper URLs at `/api/skiller/external-launch/<id>` instead of long callback URLs in the address bar
Learnt from: CR
Repo: ProverCoderAI/docker-git
Timestamp: 2026-06-18T16:07:56.571Z
Learning: Automatically launch or reuse Cloudflare Quick Tunnel when web-version is accessed via local HTTP/LAN address to provide HTTPS `backendUrl`
Learnt from: CR
Repo: ProverCoderAI/docker-git
Timestamp: 2026-06-18T16:07:56.571Z
Learning: Separate project architecture: APP (CLI + React Frontend) communicates only with API; API communicates only with LIB (backend business logic); APP cannot directly access LIB
Learnt from: CR
Repo: ProverCoderAI/docker-git
Timestamp: 2026-06-18T16:08:09.250Z
Learning: Skiller submodule must be kept intentionally outside packages/* and not listed in the root workspace to keep docker-git build, check, typecheck, and test scripts scoped to docker-git packages only
Learnt from: CR
Repo: ProverCoderAI/docker-git
Timestamp: 2026-06-18T16:08:09.250Z
Learning: After updating the Skiller pin in third_party/skiller-desktop-skills-manager, run bun run typecheck, bun run check, and bun run skiller:check to validate both docker-git and Skiller changes
Learnt from: CR
Repo: ProverCoderAI/docker-git
Timestamp: 2026-06-18T16:08:09.250Z
Learning: The DOCKER_GIT_SKILLER_WEB_URL environment variable controls external Skiller Web mode; set to empty string to force legacy bundled renderer flow
Learnt from: CR
Repo: ProverCoderAI/docker-git
Timestamp: 2026-06-18T16:08:09.250Z
Learning: The DOCKER_GIT_SKILLER_BACKEND_URL and DOCKER_GIT_API_PUBLIC_URL environment variables override the callback base URL sent to Skiller Web and disable automatic tunnel URL selection
Learnt from: CR
Repo: ProverCoderAI/docker-git
Timestamp: 2026-06-18T16:08:09.250Z
Learning: The DOCKER_GIT_SKILLER_ALLOWED_ORIGINS environment variable must be set with comma-separated trusted Skiller Web origins for CORS
Learnt from: CR
Repo: ProverCoderAI/docker-git
Timestamp: 2026-06-18T16:08:09.250Z
Learning: Do not import Skiller source into the docker-git web bundle; keep the integration boundary with Skiller runtime and filesystem scope owned by docker-git while the default browser UI is served by external Skiller Web
Learnt from: CR
Repo: ProverCoderAI/docker-git
Timestamp: 2026-06-18T16:08:09.250Z
Learning: In legacy bundled mode, the API must scope Skiller to the active project container filesystem by inspecting selected project container mounts, mapping /home/<sshUser> and targetDir, and registering the mapped project directory in Skiller
Learnt from: CR
Repo: ProverCoderAI/docker-git
Timestamp: 2026-06-18T16:08:09.250Z
Learning: For Codex skills in legacy bundled mode, Skiller must resolve ~/.codex/skills against the selected container home volume; do not fall back to the controller's own ~/.codex/skills
Learnt from: CR
Repo: ProverCoderAI/docker-git
Timestamp: 2026-06-18T16:08:09.250Z
Learning: When the API process has no $DISPLAY, the legacy bundled launcher must use xvfb-run if available so Skiller can start in headless controller environments
📚 Learning: 2026-06-10T10:21:51.286Z
Learnt from: konard
Repo: ProverCoderAI/docker-git PR: 386
File: packages/app/tests/docker-git/gridland-react-singleton.test.ts:0-0
Timestamp: 2026-06-10T10:21:51.286Z
Learning: In `packages/app/tests/docker-git` tests that inspect the `packages/terminal` workspace (e.g., for React-related versions), remember that `react-dom` is intentionally declared as a `devDependency` in `packages/terminal` (the terminal exports components but does not render to the DOM itself). Therefore, test assertions should not require `react-dom` to appear in `dependencies`; when resolving versions, accept either `dependencies` or `devDependencies` (as `resolveDepVersion` does in `gridland-react-singleton.test.ts`).
Applied to files:
packages/app/tests/docker-git/controller-resource-limits.test.tspackages/app/tests/docker-git/actions-skiller.test.tspackages/app/tests/docker-git/serve-dist-web.test.ts
📚 Learning: 2026-05-18T07:43:38.131Z
Learnt from: skulidropek
Repo: ProverCoderAI/docker-git PR: 313
File: packages/api/src/services/panel-cloudflare-tunnel.ts:123-135
Timestamp: 2026-05-18T07:43:38.131Z
Learning: In this repo’s ProverCoderAI/docker-git architecture, it is intentional for service modules under packages/api/src/services/ to call Node.js APIs directly (e.g., fs, child_process, fetch). Do not treat direct Node API usage in these service modules as a dependency-injection (DI) violation. This codebase provides the NodeContext at the program boundary (entry point) rather than injecting Context.Tag/Layer services into individual service modules; reviewers should only flag DI/context issues if the expected boundary setup is missing.
Applied to files:
packages/api/src/services/project-browser-core.tspackages/api/src/services/terminal-sessions.tspackages/api/src/services/project-browser.tspackages/api/src/services/skiller.tspackages/api/src/services/skiller-core.ts
🔇 Additional comments (8)
packages/api/src/services/skiller-core.ts (1)
50-83: LGTM!Also applies to: 129-167, 193-210, 224-270
packages/api/src/services/skiller.ts (1)
2-462: LGTM!Also applies to: 894-933, 950-982
packages/app/scripts/serve-dist-web-forwarding.mjs (1)
1-37: LGTM!packages/app/scripts/serve-dist-web.mjs (1)
10-14: LGTM!Also applies to: 104-113, 127-135
docker-compose.api.yml (1)
30-33: LGTM!docker-compose.yml (1)
30-33: LGTM!packages/app/tests/docker-git/actions-skiller.test.ts (1)
176-207: LGTM!packages/app/tests/docker-git/controller-resource-limits.test.ts (1)
23-28: LGTM!Also applies to: 69-75
| The docker-git web terminal header includes a `Skiller` button next to `Open browser`. In a project terminal the button calls `POST /projects/by-key/:projectKey/terminal-sessions/:sessionId/skiller/open` first. By default the controller stores the external Skiller Web launch URL for `https://skiller-web-henna.vercel.app/launch` and returns a short docker-git wrapper path such as `/api/skiller/external-launch/<id>`. The saved target carries `backendUrl`, `projectKey`, and `sessionId` parameters, while the browser address bar keeps the short wrapper URL. When docker-git web is served through the `/api` proxy, `backendUrl` includes that forwarded prefix so Skiller Web can call back to the same browser-reachable docker-git endpoint. | ||
|
|
||
| docker-git serves Skiller's built renderer from the submodule and proxies `/api/ssh/session/:sessionId/skiller/trpc/*` to the running Skiller tRPC backend, so the user sees the actual Skiller UI instead of an invisible background desktop process. The session id is part of the URL so a Skiller tab can be tied back to the terminal container that opened it. | ||
| Hosted Skiller Web is served over HTTPS, so browser fetches to a plain `http://192.168.x.x:4174/api` backend can be blocked before they reach docker-git. If external Skiller Web is enabled and no explicit backend override is configured, docker-git automatically starts or reuses the panel Cloudflare Quick Tunnel for local/private HTTP origins and sends Skiller Web an HTTPS callback URL such as `https://<name>.trycloudflare.com/api`. Follow-up API calls through that tunnel preserve the HTTPS forwarded protocol so the controller does not try to create a nested tunnel. | ||
|
|
||
| For project terminals, docker-git scopes Skiller to the active project container filesystem. The API inspects the selected project container mounts, maps `/home/<sshUser>` and the project `targetDir` to the controller-visible Docker volume path, launches Skiller with `HOME` set to that mapped home directory, and registers the mapped project directory in Skiller. This makes global skill operations target the selected container home and project skill operations target the selected container project directory. If the controller cannot access the Docker volume path, the endpoint fails instead of opening Skiller against the wrong filesystem. | ||
| External Skiller Web is controlled by these API/controller environment variables: | ||
|
|
||
| - `DOCKER_GIT_SKILLER_WEB_URL` sets the Skiller Web base URL. The compose default is `https://skiller-web-henna.vercel.app`. | ||
| - `DOCKER_GIT_SKILLER_BACKEND_URL` overrides the callback base URL sent to Skiller Web and disables automatic tunnel URL selection. | ||
| - `DOCKER_GIT_API_PUBLIC_URL` is the secondary callback override when `DOCKER_GIT_SKILLER_BACKEND_URL` is unset and also disables automatic tunnel URL selection. | ||
| - `DOCKER_GIT_SKILLER_ALLOWED_ORIGINS` adds comma-separated trusted Skiller Web origins for CORS. | ||
|
|
||
| Set `DOCKER_GIT_SKILLER_WEB_URL=` to force the legacy bundled renderer flow. `DOCKER_GIT_CONTROLLER_BUILD_SKILLER=0` only controls whether the controller image bundles the Skiller submodule; it does not enable external Web mode by itself. | ||
|
|
||
| In the legacy bundled mode, docker-git launches the pinned submodule Electron app as a separate process, registers the terminal session filesystem scope, writes launcher output to `~/.docker-git/logs/skiller.log`, serves Skiller's built renderer from the submodule, and proxies `/api/ssh/session/:sessionId/skiller/trpc/*` to the running Skiller tRPC backend. The session id is part of the URL so a Skiller tab can be tied back to the terminal container that opened it. | ||
|
|
||
| For project terminals, docker-git scopes Skiller to the active project container filesystem. The API inspects the selected project container mounts, maps `/home/<sshUser>` and the project `targetDir` to the controller-visible Docker volume path, launches or reuses the local Skiller runtime with `HOME` set to that mapped home directory, and registers the mapped project directory in Skiller. This makes global skill operations target the selected container home and project skill operations target the selected container project directory. If the controller cannot access the Docker volume path, the endpoint fails instead of opening Skiller against the wrong filesystem. | ||
|
|
||
| For Codex, Skiller resolves `~/.codex/skills` against the selected container home volume. For example, `/home/dev/.codex/skills` inside the selected container is exposed to the controller as the mapped Docker volume path and is the only Codex global skill tree used for that session. docker-git does not fall back to the controller's own `~/.codex/skills`. | ||
|
|
||
| When the API process has no `$DISPLAY`, the launcher uses `xvfb-run` if it is available so Skiller can still start in a headless controller environment. | ||
| When the API process has no `$DISPLAY`, the legacy bundled launcher uses `xvfb-run` if it is available so Skiller can still start in a headless controller environment. | ||
|
|
There was a problem hiding this comment.
Добавьте формальный Proof of fix блок для нового external launch/tunnel потока.
Сейчас зафиксировано поведение, но не зафиксированы формальные обязательства (инварианты, предусловия, постусловия, сложность), из-за чего нельзя строго сверить фикс по SDD-процессу.
🧩 Минимальный шаблон для добавления в документ
## Proof of fix
- Причина: <корневая причина>
- Решение: <что изменено>
- Доказательство: <тест/прогон, который падал до и проходит после>
## Математические гарантии
- Инвариант: <...>
- Предусловия: <...>
- Постусловия: <...>
- Сложность: O(...)/O(...)As per coding guidelines: *.md: Document all PR changes with mathematical proof obligations: invariants, preconditions, postconditions, variant functions, and complexity analysis in Markdown format.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/integrations/skiller.md` around lines 43 - 63, Add a formal "Proof of
fix" section and "Математические гарантии" (Mathematical guarantees) subsection
to the Skiller documentation after the environment variables explanation and
before the legacy bundled mode details. The "Proof of fix" block should document
the root cause of the external launch/tunnel flow issue, the solution
implemented, and proof that relevant tests pass. The "Математические гарантии"
section should specify the invariants (properties that must always hold),
preconditions (requirements before the flow executes), postconditions
(guaranteed results after execution), and complexity analysis for the external
Skiller Web URL callback mechanism and automatic Cloudflare tunnel selection
logic. This formalizes the behavioral guarantees for strict verification against
the SDD process requirements.
Source: Coding guidelines
| const dockerGitApiPort = (env: Record<string, string | undefined>): string => { | ||
| const configured = env["DOCKER_GIT_API_PORT"]?.trim() | ||
| return configured !== undefined && /^\d{1,5}$/u.test(configured) ? configured : "3334" | ||
| } |
There was a problem hiding this comment.
Проверка DOCKER_GIT_API_PORT пропускает недопустимые порты.
На Line 655 регулярка принимает значения до 99999, из-за чего формируется невалидный panelUrl и tunnel-ветка падает при ошибочной конфигурации.
💡 Предлагаемый фикс
const dockerGitApiPort = (env: Record<string, string | undefined>): string => {
const configured = env["DOCKER_GIT_API_PORT"]?.trim()
- return configured !== undefined && /^\d{1,5}$/u.test(configured) ? configured : "3334"
+ if (configured === undefined || !/^\d{1,5}$/u.test(configured)) {
+ return "3334"
+ }
+ const port = Number(configured)
+ return Number.isInteger(port) && port >= 1 && port <= 65_535 ? configured : "3334"
}🤖 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/api/src/http.ts` around lines 653 - 656, The validation in the
dockerGitApiPort function is too permissive. The regex pattern /^\d{1,5}$/u
accepts port numbers up to 99999, but valid TCP port numbers must be in the
range 0-65535. Fix the validation logic to ensure the configured port number is
both in the correct numeric format and within the valid port range. Convert the
string to a number and check that it falls within the valid port range (0-65535)
before returning it as a valid port.
| Effect.flatMap((output) => | ||
| output.includes("changed") || output.includes("restart") | ||
| ? restartBrowserContainer(cwd, containerName) | ||
| : Effect.void |
There was a problem hiding this comment.
Ложный рестарт контейнера из-за проверки includes("changed")
На Line 357 значение "unchanged" тоже удовлетворяет includes("changed"), поэтому контейнер будет перезапускаться даже когда ремонт ничего не изменил.
Предлагаемый фикс
- Effect.flatMap((output) =>
- output.includes("changed") || output.includes("restart")
- ? restartBrowserContainer(cwd, containerName)
- : Effect.void
- ),
+ Effect.flatMap((output) => {
+ const statuses = output
+ .split(/\r?\n/u)
+ .map((line) => line.trim())
+ .filter((line) => line.length > 0)
+ return statuses.includes("changed") || statuses.includes("restart")
+ ? restartBrowserContainer(cwd, containerName)
+ : Effect.void
+ }),Также стоит добавить регрессионный тест на ответ unchanged, чтобы этот кейс не возвращался.
🤖 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/api/src/services/project-browser.ts` around lines 356 - 359, The
condition in the Effect.flatMap callback that checks output.includes("changed")
has a substring matching issue where "unchanged" also contains "changed" as a
substring, causing false positives and unnecessary container restarts. Fix this
by replacing the includes("changed") check with a more precise comparison, such
as checking output === "changed" for exact matching, or using a regex pattern
that checks for the word "changed" as a standalone word rather than as a
substring. This will ensure that "unchanged" outputs do not trigger the
restartBrowserContainer call in the conditional block where
restartBrowserContainer, cwd, and containerName are used.
| "if [ -f ../../scripts/skiller-apply-docker-git-patches.mjs ]; then bun ../../scripts/skiller-apply-docker-git-patches.mjs; fi", | ||
| "if [ ! -d node_modules ]; then bun install --frozen-lockfile; fi", | ||
| "if [ ! -f out/main/index.js ] || [ ! -f out/renderer/index.html ] || { [ -f \"$DOCKER_GIT_SKILLER_PATCH\" ] && [ ! -f \"$DOCKER_GIT_SKILLER_PATCH_MARKER\" ]; } || { [ -f \"$DOCKER_GIT_SKILLER_PATCH\" ] && [ \"$DOCKER_GIT_SKILLER_PATCH\" -nt \"$DOCKER_GIT_SKILLER_PATCH_MARKER\" ]; }; then", | ||
| "if [ ! -f out/main/index.js ] || [ ! -f out/renderer/index.html ] || { [ -f \"$DOCKER_GIT_SKILLER_PATCH\" ] && [ ! -f \"$DOCKER_GIT_SKILLER_PATCH_MARKER\" ]; }; then", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Показать условие сборки и все операции с PATCH/PATCH_MARKER в этом модуле
rg -n 'DOCKER_GIT_SKILLER_PATCH(_MARKER)?|out/main/index.js|out/renderer/index.html|touch .*PATCH_MARKER|rm .*PATCH_MARKER' packages/api/src/services/skiller.ts -C3
# Проверить, есть ли инвалидация marker в других файлах репозитория
rg -n 'DOCKER_GIT_SKILLER_PATCH_MARKER|SKILLER_PATCH_MARKER' -g '*.ts' -g '*.js' -g '*.sh' -g '*.bash'Repository: ProverCoderAI/docker-git
Length of output: 1766
🏁 Script executed:
# Check if skiller-apply-docker-git-patches.mjs has any patch validation/invalidation logic
find . -name 'skiller-apply-docker-git-patches.mjs' -o -name '*skiller*patch*.mjs' -o -name '*skiller*patch*.js' | head -20Repository: ProverCoderAI/docker-git
Length of output: 115
🏁 Script executed:
# Look for any hash/checksum computation of patch files
rg -i 'checksum|hash|md5|sha256|digest' packages/api/src/services/skiller.ts -C2Repository: ProverCoderAI/docker-git
Length of output: 50
🏁 Script executed:
# Check if there's any external invalidation of marker (e.g., in docker-compose, cleanup scripts, etc.)
rg -r 'rm.*PATCH_MARKER|rm.*\.docker-git.*patch|PATCH_MARKER.*rm' --type sh --type bash --type ts --type jsRepository: ProverCoderAI/docker-git
Length of output: 465
🏁 Script executed:
# Read the skiller-apply-docker-git-patches.mjs script to see if it validates/invalidates patch marker
cat ./scripts/skiller-apply-docker-git-patches.mjsRepository: ProverCoderAI/docker-git
Length of output: 5554
🏁 Script executed:
# Look for any external marker invalidation in shell scripts, docker-compose files, or initialization scripts
find . -type f \( -name '*.sh' -o -name '*.bash' -o -name 'docker-compose*' -o -name 'Dockerfile*' \) | xargs grep -l 'PATCH_MARKER\|docker-git.*patch' 2>/dev/null | head -10Repository: ProverCoderAI/docker-git
Length of output: 94
🏁 Script executed:
# Check README or docs for patch management/invalidation process
find . -name 'README*' -o -name '*.md' | xargs grep -l -i 'patch.*marker\|patch.*invalidat\|docker.*patch' 2>/dev/null | head -5Repository: ProverCoderAI/docker-git
Length of output: 173
Инвалидация patch-marker при изменении содержимого патча не реализована.
Текущий механизм использует файловый маркер (строка 519: touch "$DOCKER_GIT_SKILLER_PATCH_MARKER") для кэширования состояния сборки. Условие в строке 516 триггерит bun run build только если:
- отсутствуют выходные файлы (
out/main/index.js,out/renderer/index.html), или - патч существует И маркер отсутствует
Проблема: маркер основан на существовании файла, а не на его содержимом. Если содержимое $DOCKER_GIT_SKILLER_PATCH изменится, маркер останется, и сборка не будет переделана. Скрипт skiller-apply-docker-git-patches.mjs (строка 514) применяет патч-изменения, но не инвалидирует маркер, поэтому кэш остаётся невалидным.
Рекомендуемое решение: либо использовать хэш содержимого патча вместо простого файлового маркера, либо удалять маркер при обнаружении изменений в патче.
🤖 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/api/src/services/skiller.ts` at line 516, The patch-marker caching
mechanism in the build condition (line 516) only checks for file existence, not
content changes. When the content of $DOCKER_GIT_SKILLER_PATCH changes, the
existing marker remains valid and prevents rebuilds from triggering. To fix
this, implement content-based validation by either: (1) modifying the condition
at line 516 to compare a hash of the current patch content against a stored hash
in the marker file, or (2) adding logic in the
skiller-apply-docker-git-patches.mjs script to remove or invalidate the
$DOCKER_GIT_SKILLER_PATCH_MARKER when patch content changes are detected,
ensuring that subsequent build conditions properly trigger when the patch file
is modified.
| (previousEntries) => | ||
| Effect.sync(() => { | ||
| for (const [key, value] of previousEntries) { | ||
| setOptionalEnv(key, value) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Восстановление process.env некорректно при повторяющихся ключах.
На Line 60 значения восстанавливаются в прямом порядке; если один ключ передан несколько раз (например default + override), финальное состояние после release может протечь в следующие тесты.
💡 Предлагаемый фикс
(previousEntries) =>
Effect.sync(() => {
- for (const [key, value] of previousEntries) {
- setOptionalEnv(key, value)
+ for (let i = previousEntries.length - 1; i >= 0; i -= 1) {
+ const [key, value] = previousEntries[i]!
+ setOptionalEnv(key, value)
}
})
)🤖 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/api/tests/skiller-cors.test.ts` around lines 58 - 63, When restoring
environment variables from previousEntries, the current forward iteration order
can cause incorrect state restoration if any key appears multiple times in the
entries. Reverse the iteration order of previousEntries so that duplicated keys
are processed in reverse chronological order, ensuring the most recent state is
restored first and preventing intermediate or stale values from leaking into
subsequent tests. Modify the for loop iteration in the Effect.sync block where
setOptionalEnv is called to iterate through previousEntries in reverse order
instead of forward order.
| it("normalizes safe forwarded prefixes and ignores unsafe ones for external launch URLs", () => | ||
| Effect.runPromise( | ||
| Effect.gen(function*(_) { | ||
| const baseHeaders = { | ||
| "x-forwarded-host": "192.168.0.206:4174", | ||
| "x-forwarded-proto": "https" | ||
| } | ||
| const trailing = yield* _(requestExternalLaunch({ | ||
| ...baseHeaders, | ||
| "x-forwarded-prefix": "/api/" | ||
| })) | ||
| const root = yield* _(requestExternalLaunch({ | ||
| ...baseHeaders, | ||
| "x-forwarded-prefix": "/" | ||
| })) | ||
| const invalid = yield* _(requestExternalLaunch({ | ||
| ...baseHeaders, | ||
| "x-forwarded-prefix": "https://evil.example/api" | ||
| })) | ||
|
|
||
| expect(trailing.body.backendUrl).toBe("https://192.168.0.206:4174/api") | ||
| expect(root.body.backendUrl).toBe("https://192.168.0.206:4174") | ||
| expect(invalid.body.backendUrl).toBe("https://192.168.0.206:4174") | ||
| }) | ||
| )) | ||
|
|
There was a problem hiding this comment.
Инварианты нормализации x-forwarded-prefix лучше зафиксировать property-based тестом.
Сейчас проверяются отдельные примеры, но это математический инвариант (safe prefix -> ожидаемый backendUrl) и его стоит покрыть через fc.property для широкого множества входов.
As per coding guidelines: **/*.test.{ts,tsx}: Write property-based tests using fast-check (fc.property) to verify mathematical invariants; unit tests must use Effect test utilities without async/await.
🤖 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/api/tests/skiller-cors.test.ts` around lines 168 - 193, The test
"normalizes safe forwarded prefixes and ignores unsafe ones for external launch
URLs" currently verifies only three hardcoded examples instead of using
property-based testing. Convert this test to use fast-check's fc.property to
verify the mathematical invariant that safe prefixes are normalized correctly
into the backendUrl. Generate a variety of safe prefixes (like different path
patterns) and unsafe prefixes (like full URLs with schemes) using fast-check
generators, and verify that each input produces the expected normalized
backendUrl output. Keep the Effect.runPromise and requestExternalLaunch
structure but apply the property-based testing approach across multiple
generated inputs rather than fixed test cases.
Source: Coding guidelines
| const launchId = launch.appPath.split("/").at(-1) | ||
| if (launchId === undefined) { | ||
| throw new Error("Expected external Skiller launch id.") | ||
| } | ||
| const targetUrl = await Effect.runPromise(readExternalSkillerLaunchTarget(decodeURIComponent(launchId))) | ||
| const launchUrl = new URL(targetUrl) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Проверка, что в файле не осталось async/await после рефактора
rg -n --type=ts '\basync\b|\bawait\b' packages/api/tests/skiller-routes.test.tsRepository: ProverCoderAI/docker-git
Length of output: 341
🏁 Script executed:
# Посмотрим на полный контекст теста
cat -n packages/api/tests/skiller-routes.test.ts | sed -n '75,105p'Repository: ProverCoderAI/docker-git
Length of output: 1901
🏁 Script executed:
# Найдём примеры других тестов в проекте, чтобы понять правильный паттерн
find packages -name "*.test.ts" -o -name "*.spec.ts" | head -5Repository: ProverCoderAI/docker-git
Length of output: 305
🏁 Script executed:
# Проверим, какой паттерн используется в других тестах
rg -A 3 'it\(' packages/api/tests/ | head -30Repository: ProverCoderAI/docker-git
Length of output: 2762
🏁 Script executed:
# Поищем it.effect или другие Effect test utilities
rg -n 'it\.effect|Effect\.test|describe\.effect' packages/api/tests/ | head -20Repository: ProverCoderAI/docker-git
Length of output: 1900
🏁 Script executed:
# Посмотрим, как используется Effect в других тестах - ищем patterns без async/await
rg -B 2 -A 5 'it\(".*", \(\) => \{' packages/api/tests/ | head -40Repository: ProverCoderAI/docker-git
Length of output: 2842
🏁 Script executed:
# Проверим, есть ли effect test utilities в зависимостях
grep -A 5 '"effect"' packages/api/package.json 2>/dev/null || echo "not found in api package.json"Repository: ProverCoderAI/docker-git
Length of output: 181
🏁 Script executed:
# Посмотрим на пример правильного it.effect использования
cat -n packages/api/tests/events.test.ts | sed -n '1,20p'Repository: ProverCoderAI/docker-git
Length of output: 1009
🏁 Script executed:
# Найдём пример композиции Effect в тестах
rg -A 10 'it\.effect.*opens?.*\=>' packages/api/tests/ | head -40Repository: ProverCoderAI/docker-git
Length of output: 178
🏁 Script executed:
# Посмотрим на примеры it.effect с несколькими операциями
rg -A 15 'it\.effect' packages/api/tests/schema.test.ts | head -50Repository: ProverCoderAI/docker-git
Length of output: 1787
🏁 Script executed:
# Проверим, как в skiller-routes.test.ts должны быть структурированы тесты - посмотрим на весь файл
wc -l packages/api/tests/skiller-routes.test.tsRepository: ProverCoderAI/docker-git
Length of output: 114
🏁 Script executed:
# Посмотрим на начало файла skiller-routes.test.ts, чтобы увидеть import'ы
head -20 packages/api/tests/skiller-routes.test.tsRepository: ProverCoderAI/docker-git
Length of output: 739
Рефакторьте тест, чтобы использовать it.effect из @effect/vitest вместо async/await.
Тест использует async () => { await Effect.runPromise(...) } (линии 83, 87, 94), что нарушает guideline: **/*.test.{ts,tsx}: unit tests must use Effect test utilities without async/await.
Используйте it.effect с Effect.gen() для композиции операций:
it.effect("returns an external Skiller Web launch when DOCKER_GIT_SKILLER_WEB_URL is configured", () =>
Effect.gen(function* (_) {
const launch = yield* _(openSkiller(undefined, undefined, "https://docker-git.example").pipe(Effect.provide(NodeContext.layer)))
const launchId = launch.appPath.split("/").at(-1)
if (launchId === undefined) {
throw new Error("Expected external Skiller launch id.")
}
const targetUrl = yield* _(readExternalSkillerLaunchTarget(decodeURIComponent(launchId)))
// ... assertions
})
)Это соответствует паттернам, используемым в других тестах репозитория (schema.test.ts, events.test.ts).
🤖 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/api/tests/skiller-routes.test.ts` around lines 90 - 95, Replace the
async test function with Effect test utilities. Change the test declaration from
`it(...)` to `it.effect(...)` and replace the `async () => { await
Effect.runPromise(...) }` pattern with an `Effect.gen()` function. Inside the
generator, use `yield*` to unwrap Effect operations instead of calling `await
Effect.runPromise()` directly. Apply this pattern to compose all Effect-based
operations like `openSkiller()` and `readExternalSkillerLaunchTarget()` to align
with the Effect testing guidelines used in other test files in the repository.
Source: Coding guidelines
1 participant
