Skip to content

fix(api): mount host docker data for skiller#366

Open
skulidropek wants to merge 3 commits into
mainfrom
issue-365
Open

fix(api): mount host docker data for skiller#366
skulidropek wants to merge 3 commits into
mainfrom
issue-365

Conversation

@skulidropek
Copy link
Copy Markdown
Member

@skulidropek skulidropek commented Jun 3, 2026

Source TZ / Issues

Summary

  • Bind host /var/lib/docker into the default host-mode controller so Skiller can access Docker volume paths returned by docker inspect.
  • Keep isolated runtime on the private docker_git_docker_data volume.
  • Document the host runtime contract and add compose regression coverage.

Requirements Alignment

  • Implemented: host runtime exposes host Docker volume paths to the controller; isolated runtime keeps embedded Docker data isolated; compose/docs/tests updated.
  • Out of scope: frontend Skiller button behavior and Skiller app internals.
  • Security-sensitive changes: host runtime now explicitly bind-mounts /var/lib/docker, matching its existing host Docker socket access model.

Verification

  • bun --cwd packages/app vitest run tests/docker-git/controller-resource-limits.test.ts
  • bun --cwd packages/api vitest run tests/skiller-core.test.ts
  • bun run --cwd packages/app test -- tests/docker-git/controller-resource-limits.test.ts
  • bun run --cwd packages/api test -- tests/skiller-core.test.ts
  • bun run test
  • git diff --check
  • docker compose config for host and isolated volume resolution

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 3, 2026

Review Change Stack

Warning

Review limit reached

@skulidropek, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 30 minutes and 57 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b64c4737-6563-4cc9-a33e-25454f278444

📥 Commits

Reviewing files that changed from the base of the PR and between b652ffb and 1076ed0.

📒 Files selected for processing (3)
  • packages/api/Dockerfile
  • packages/api/tests/skiller-routes.test.ts
  • packages/app/tests/docker-git/controller-resource-limits.test.ts
📝 Walkthrough

Walkthrough

Конфигурации API-контейнера переключены на bind-mount для /var/lib/docker; README и тесты обновлены; Dockerfile теперь материализует Electron-бинарник в сборке; логика запуска Skiller упрощена и соответствующие тесты добавлены.

Изменения

Миграция на bind-mount и обновление Skiller

Layer / File(s) Summary
Конфигурация bind-mount в docker-compose
docker-compose.yml, docker-compose.api.yml
В services.api.volumes заменено монтирование /var/lib/docker с именованного volume docker_git_docker_data на прямой bind-mount /var/lib/docker:/var/lib/docker.
Документация и проверка compose режимов
packages/api/README.md, packages/app/tests/docker-git/controller-resource-limits.test.ts
README уточнён о монтировании Docker data root; в тесты добавлены константы и assertions, проверяющие bind-mount в обычном режиме и named-volume в изолированном режиме.
Dockerfile: материализация Electron в билде
packages/api/Dockerfile, packages/app/tests/docker-git/controller-resource-limits.test.ts
Добавлены шаги сборки, которые ищут electron-v*-linux-*.zip в кеше, очищают старые артефакты, распаковывают в node_modules/electron/dist, записывают path.txt и гарантируют исполняемость бинарника; тесты проверяют наличие этой логики.
Skiller: рефактор команды запуска
packages/api/src/services/skiller.ts
skillerLaunchCommand экспортируется и больше не принимает SkillerProcessUser; launchSkillerProcess вызывает prepareSkillerScopeHome(scope) и использует skillerLaunchCommand() без аргументов.
Тесты Skiller
packages/api/tests/skiller-routes.test.ts
Добавлен импорт skillerLaunchCommand и тест, который проверяет отсутствие setpriv в сформированной команде и присутствие пути к node_modules/electron/dist/electron.

Обзор

Конфигурация API-контейнера изменена для использования bind-mount вместо именованного volume для монтирования /var/lib/docker на хосте. README, тесты и Dockerfile обновлены; поведение запуска Skiller упрощено и покрыто тестом.


🎯 3 (Moderate) | ⏱️ ~20 minutes


Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error, 1 warning)

Check name Status Explanation Resolution
Requirements Alignment ❌ Error skillerLaunchCommand() removed UID/GID switching but prepareSkillerScopeHome() chowns directories for scope. Process runs as controller, breaking ownership and OS-level isolation. Restore UID/GID process isolation for Skiller or explicitly document intentional privilege drop removal as safe per threat model.
Security Regression ⚠️ Warning PR вводит path traversal риск в Dockerfile (unzip без валидации) и удаляет UID/GID privilege drop в Skiller запуске, нарушая OS-level изоляцию. Добавьте валидацию zip-содержимого перед распаковкой в Dockerfile; верните setpriv для запуска Skiller с UID/GID владельца project scope.
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed Заголовок PR полностью описывает основное изменение: монтирование Docker данных хоста для Skiller, что отражено во всех модифицированных файлах.
Description check ✅ Passed Описание PR следует шаблону репозитория с заполненными разделами: указана связанная issue #365, предоставлена подробная сводка, описано выравнивание требований и шаги проверки.
Linked Issues check ✅ Passed PR полностью соответствует требованиям issue #365: добавлено bind-монтирование /var/lib/docker для хост-режима, сохранён изолированный volume, обновлена документация и добавлены тесты.
Out of Scope Changes check ✅ Passed Все изменения находятся в области видимости issue #365: Docker Compose файлы, документация, тесты и логика Skiller для доступа к Docker данным хоста.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-365

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

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@skulidropek
Copy link
Copy Markdown
Member Author

Скриншоты работы приложения для issue #365. Это не CI и не файлы: кадры сняты с web UI после реального CDP click по кнопке Skiller.

Проверка в proof-среде:

  • POST /api/projects/by-key/53a9af0e027e/terminal-sessions/15aca7aa-9b45-4845-8872-0dba1f0ff8bb/skiller/open -> 202 Accepted
  • /api/ssh/session/15aca7aa-9b45-4845-8872-0dba1f0ff8bb/skiller/app/ -> 200, assets загружены
  • /api/ssh/session/15aca7aa-9b45-4845-8872-0dba1f0ff8bb/skiller/trpc/get_app_version -> 200
  1. Terminal UI: кнопка Skiller видна рядом с Open browser.

01 before Skiller

  1. Кнопка Skiller нажимается. Красный marker добавлен поверх кадра, потому что headless Chrome screenshots не рисуют курсор; в этот момент был отправлен Input.dispatchMouseEvent(mousePressed).

02 Skiller mouse down

  1. После отпускания кнопки backend принял open-запрос (202 Accepted).

03 after click 202

  1. Страница Skiller открылась через session-scoped route.

04 Skiller app loaded

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/api/Dockerfile`:
- Around line 124-130: The unzip step that writes into
node_modules/electron/dist using electron_zip is unsafe to path traversal;
before calling unzip in the Dockerfile, ensure electron_zip is non-empty (fail
with a clear error) and validate the archive contents for unsafe paths (reject
entries with '..' or leading '/') and/or verify a checksum for the expected
Electron version, then only run unzip when validation passes; update the block
referencing electron_zip and node_modules/electron/dist to perform these checks
and fail the build on detection of unsafe paths or missing zip.

In `@packages/api/src/services/skiller.ts`:
- Around line 366-369: The code currently launches Skiller via
skillerLaunchCommand as root/bash which runs the process under the controller
UID/GID; instead, ensure Skiller is executed with the owner UID/GID of
scope.hostHomePath (the same user chown'd in prepareSkillerScopeHome) by
changing skillerLaunchCommand to either wrap launchScript with a uid/gid switch
(e.g., gosu/su-exec or sudo -u equivalent) or by invoking an exec path that sets
process credentials before exec; also ensure the runtime mounts map
/home/<sshUser> and the project targetDir to the controller-visible Docker
volume paths so filesystem access is limited to the active project container.
Use the symbols skillerLaunchCommand and prepareSkillerScopeHome to locate where
to apply these changes.

In `@packages/api/tests/skiller-routes.test.ts`:
- Around line 35-41: The test 'launches Electron as the controller process user'
pins an implementation detail (absence of "setpriv") rather than the contract;
remove the expect(...not.toContain("setpriv")) assertion and instead assert that
skillerLaunchCommand() produces the expected Docker volume mappings for
controller-visible paths. Concretely, in the test that calls
skillerLaunchCommand(), keep the command and electron path assertions, drop the
"setpriv" negative check, and add assertions on the args array (e.g., using
expect.arrayContaining and expect.stringContaining or regex) that verify docker
volume flags ("-v" or "--volume") map "/home/<sshUser>" and the project
targetDir to controller-visible volume paths (look for strings containing
"/home/" and the project targetDir identifier) so the test verifies filesystem
scoping rather than forbidding a specific privilege-drop mechanism.

In `@packages/app/tests/docker-git/controller-resource-limits.test.ts`:
- Around line 98-108: The test in describe("controller Skiller Dockerfile") is
brittle because it asserts exact shell lines via toContain; update the test to
assert the key elements more flexibly: read the Dockerfile with
readComposeFile("packages/api/Dockerfile") as before but replace the exact
toContain checks with either regex matches or separate contains for the variable
definition (electron_zip), the unzip invocation (unzip -q ...
node_modules/electron/dist), and the executable check (test -x
node_modules/electron/dist/electron); also consider renaming the suite to
something specific (e.g., "materialize Electron binary in API Dockerfile") and
keep the test as a regression check if you must assert an exact pattern.
🪄 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: a1240847-ec81-4a84-9d05-f0db3923db98

📥 Commits

Reviewing files that changed from the base of the PR and between 6eabb7e and b652ffb.

📒 Files selected for processing (4)
  • packages/api/Dockerfile
  • packages/api/src/services/skiller.ts
  • packages/api/tests/skiller-routes.test.ts
  • packages/app/tests/docker-git/controller-resource-limits.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). (9)
  • GitHub Check: E2E (Clone cache)
  • GitHub Check: E2E (Browser command)
  • GitHub Check: E2E (Login context)
  • GitHub Check: Lint
  • GitHub Check: E2E (Clone auto-open SSH)
  • GitHub Check: E2E (Runtime volumes + SSH)
  • GitHub Check: Test
  • GitHub Check: E2E (OpenCode)
  • GitHub Check: Final build (windows-latest)
🧰 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 use any, unknown, eslint-disable, ts-ignore, or as type 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 through pipe() and Effect.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), @throws Never (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/api/tests/skiller-routes.test.ts
  • packages/api/src/services/skiller.ts
  • packages/app/tests/docker-git/controller-resource-limits.test.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/api/tests/skiller-routes.test.ts
  • packages/app/tests/docker-git/controller-resource-limits.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/catch for 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/api/tests/skiller-routes.test.ts
  • packages/api/src/services/skiller.ts
  • packages/app/tests/docker-git/controller-resource-limits.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/api/tests/skiller-routes.test.ts
  • packages/app/tests/docker-git/controller-resource-limits.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/api/tests/skiller-routes.test.ts
  • packages/api/src/services/skiller.ts
  • packages/app/tests/docker-git/controller-resource-limits.test.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/skiller-routes.test.ts
  • packages/api/src/services/skiller.ts
  • packages/app/tests/docker-git/controller-resource-limits.test.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/api/tests/skiller-routes.test.ts
  • packages/api/src/services/skiller.ts
  • packages/app/tests/docker-git/controller-resource-limits.test.ts
**/*

⚙️ CodeRabbit configuration file

**/*: Ты строгий ревьюер SPEC DRIVEN DEVELOPMENT.

Перед выводами изучи README.md, другие *.md файлы, linked issues,
PR description, PR comments/discussion и релевантную кодовую базу.

Сверь изменения с исходным ТЗ/спекой и обсуждением. Флагай любой уход
от спеки, недокументированное изменение поведения, отсутствие тестов
для заявленного поведения и security-риск. Если спека не видна,
попроси автора добавить ее в issue или PR description.

Проверь решение с точки зрения формальной верификации: какие инварианты,
предусловия и постусловия можно доказать математически, а где доказуемость
слабая. Оцени решение с точки зрения теории игр: устойчивы ли стимулы,
нет ли выгодного обхода правил, и какое решение было бы сильнее.

Files:

  • packages/api/tests/skiller-routes.test.ts
  • packages/api/Dockerfile
  • packages/api/src/services/skiller.ts
  • packages/app/tests/docker-git/controller-resource-limits.test.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:

  • packages/api/Dockerfile
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: ProverCoderAI/docker-git PR: 0
File: docs/integrations/skiller.md:0-0
Timestamp: 2026-05-14T16:02:05.012Z
Learning: Applies to docs/integrations/**/*.(ts|js)?(@(routes|handlers|api|middleware))* : API must scope Skiller filesystem access to the active project container by mapping `/home/<sshUser>` and project `targetDir` to controller-visible Docker volume paths
Learnt from: CR
Repo: ProverCoderAI/docker-git PR: 0
File: docs/integrations/skiller.md:0-0
Timestamp: 2026-05-14T16:02:05.012Z
Learning: docker-git must not import Skiller source code into the docker-git web bundle; serve Skiller browser UI from Skiller's own built renderer and proxy tRPC requests to Skiller's separate backend process
Learnt from: CR
Repo: ProverCoderAI/docker-git PR: 0
File: docs/integrations/skiller.md:0-0
Timestamp: 2026-05-14T16:02:05.012Z
Learning: Applies to docs/integrations/**/*.(ts|js)?(@(routes|handlers|api))* : API endpoint `POST /projects/by-key/:projectKey/terminal-sessions/:sessionId/skiller/open` must launch Skiller Electron app, register terminal session filesystem scope, and write output to ~/.docker-git/logs/skiller.log
Learnt from: CR
Repo: ProverCoderAI/docker-git PR: 0
File: docs/integrations/skiller.md:0-0
Timestamp: 2026-05-14T16:02:05.012Z
Learning: Run `bun run skiller:check` after updating the Skiller pin in addition to docker-git `typecheck` and `check` scripts to verify integration
📚 Learning: 2026-05-14T16:02:05.012Z
Learnt from: CR
Repo: ProverCoderAI/docker-git PR: 0
File: docs/integrations/skiller.md:0-0
Timestamp: 2026-05-14T16:02:05.012Z
Learning: Applies to docs/integrations/**/*.(ts|js)?(@(routes|handlers|api))* : API endpoint `POST /projects/by-key/:projectKey/terminal-sessions/:sessionId/skiller/open` must launch Skiller Electron app, register terminal session filesystem scope, and write output to ~/.docker-git/logs/skiller.log

Applied to files:

  • packages/api/tests/skiller-routes.test.ts
  • packages/api/src/services/skiller.ts
  • packages/app/tests/docker-git/controller-resource-limits.test.ts
📚 Learning: 2026-05-14T16:02:05.012Z
Learnt from: CR
Repo: ProverCoderAI/docker-git PR: 0
File: docs/integrations/skiller.md:0-0
Timestamp: 2026-05-14T16:02:05.012Z
Learning: Applies to docs/integrations/**/*.(ts|js)?(@(routes|handlers|api|services))* : API must resolve Codex global skills from `~/.codex/skills` within the selected container volume, not falling back to controller's own `~/.codex/skills`

Applied to files:

  • packages/api/tests/skiller-routes.test.ts
  • packages/app/tests/docker-git/controller-resource-limits.test.ts
📚 Learning: 2026-05-14T16:02:05.012Z
Learnt from: CR
Repo: ProverCoderAI/docker-git PR: 0
File: docs/integrations/skiller.md:0-0
Timestamp: 2026-05-14T16:02:05.012Z
Learning: docker-git must not import Skiller source code into the docker-git web bundle; serve Skiller browser UI from Skiller's own built renderer and proxy tRPC requests to Skiller's separate backend process

Applied to files:

  • packages/api/tests/skiller-routes.test.ts
  • packages/api/Dockerfile
  • packages/app/tests/docker-git/controller-resource-limits.test.ts
📚 Learning: 2026-05-14T16:02:05.012Z
Learnt from: CR
Repo: ProverCoderAI/docker-git PR: 0
File: docs/integrations/skiller.md:0-0
Timestamp: 2026-05-14T16:02:05.012Z
Learning: Applies to docs/integrations/**/*.(ts|js|sh|bash)?(launcher|skiller)* : Skiller launcher must use `xvfb-run` if available when API process has no `$DISPLAY` environment variable to enable headless startup

Applied to files:

  • packages/api/tests/skiller-routes.test.ts
  • packages/api/src/services/skiller.ts
📚 Learning: 2026-05-14T16:02:05.012Z
Learnt from: CR
Repo: ProverCoderAI/docker-git PR: 0
File: docs/integrations/skiller.md:0-0
Timestamp: 2026-05-14T16:02:05.012Z
Learning: Applies to docs/integrations/**/*.(ts|js)?(@(routes|handlers|api|middleware))* : API must scope Skiller filesystem access to the active project container by mapping `/home/<sshUser>` and project `targetDir` to controller-visible Docker volume paths

Applied to files:

  • packages/api/tests/skiller-routes.test.ts
  • packages/app/tests/docker-git/controller-resource-limits.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/skiller.ts
📚 Learning: 2026-05-22T21:08:22.631Z
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:22.631Z
Learning: In the docker-git project, `${DOCKER_GIT_CONTROLLER_BUILD_SKILLER:-1}` in compose files is standard bash parameter expansion with default value `"1"` (not a literal "-1" mode). The supported runtime contract for `DOCKER_GIT_CONTROLLER_BUILD_SKILLER` in `packages/app/src/docker-git/controller-compose.ts` is: unset/`"1"`/`"true"` → `"1"`, `"0"`/`"false"` → `"0"`. There is no `-1` mode. The Dockerfile also declares `ARG DOCKER_GIT_CONTROLLER_BUILD_SKILLER=1`.

Applied to files:

  • packages/app/tests/docker-git/controller-resource-limits.test.ts
🔇 Additional comments (3)
packages/app/tests/docker-git/controller-resource-limits.test.ts (3)

21-22: LGTM!


56-61: LGTM!


88-93: LGTM!

Comment thread packages/api/Dockerfile
Comment on lines +366 to +369
// Electron aborts under setpriv in the controller image even with --no-sandbox.
// Project scope still comes from explicit host paths and the browser bootstrap.
export const skillerLaunchCommand = (): readonly [string, ReadonlyArray<string>] =>
["bash", ["-lc", launchScript]]
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Не запускайте Skiller от пользователя контроллера.

После удаления privilege drop процесс Electron пишет в scope.hostHomePath с UID/GID контроллера, а не владельца project volume. Здесь prepareSkillerScopeHome() лишь подготавливает и chown-ит каталоги, но больше не применяется к самому процессу, поэтому вы одновременно ломаете ownership внутри project volume и снимаете OS-level изоляцию между активным scope и остальным /var/lib/docker. Верните запуск под UID/GID владельца scope.hostHomePath или эквивалентный механизм до exec. Based on learnings: API must scope Skiller filesystem access to the active project container by mapping /home/<sshUser> and project targetDir to controller-visible Docker volume paths.

🤖 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` around lines 366 - 369, The code
currently launches Skiller via skillerLaunchCommand as root/bash which runs the
process under the controller UID/GID; instead, ensure Skiller is executed with
the owner UID/GID of scope.hostHomePath (the same user chown'd in
prepareSkillerScopeHome) by changing skillerLaunchCommand to either wrap
launchScript with a uid/gid switch (e.g., gosu/su-exec or sudo -u equivalent) or
by invoking an exec path that sets process credentials before exec; also ensure
the runtime mounts map /home/<sshUser> and the project targetDir to the
controller-visible Docker volume paths so filesystem access is limited to the
active project container. Use the symbols skillerLaunchCommand and
prepareSkillerScopeHome to locate where to apply these changes.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am intentionally not applying this suggestion literally in this PR. I verified the UID/GID drop path in the proof controller container before posting the screenshots: Electron aborts under setpriv as the project UID (Trace/breakpoint trap) even with --no-sandbox; chowning the Skiller app directory and adding common Chromium flags did not make that path reliable. Running the Electron process as the controller user is therefore the deterministic launch path for the current controller image.

The filesystem scope is still selected through the existing SkillerContainerScope: /home/<sshUser>, .codex/skills, the project path, and DOCKER_GIT_SKILLER_HOST_ENV_GLOBAL_PATH are resolved through the project container mounts and injected into Skiller via HOME, XDG_*, DOCKER_GIT_SKILLER_CONTAINER_HOME_PATH, the browser bootstrap, and add_project(scope.hostProjectPath). The controller itself is already root-equivalent in this mode because this PR intentionally mounts /var/run/docker.sock and /var/lib/docker so it can map Docker volume paths for issue #365.

I did remove the test assertion that pinned not setpriv, so a future dedicated Electron sandbox/credential-drop fix will not be blocked by this test.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ezocomp118-source, понял — аргументация обоснована. setpriv + Electron — известная проблема в этом классе контейнеров, где seccomp/capabilities отличается от desktopного окружения, и Trace/breakpoint trap при старте Chromium — это именно тот сигнал. Scope-изоляция через SkillerContainerScope + явные HOME/XDG_*/add_project(scope.hostProjectPath) — корректный software-level контроль при уже root-equivalent контроллере.

Остаточный риск, который стоит зафиксировать: Electron пишет в scope.hostHomePath (и в .config, .cache, .skiller внутри него) под UID/GID контроллера, поэтому в project volume могут появиться файлы с неправильным owner — это сломает chown-проверки при следующем вызове prepareSkillerScopeHome. Это не блокер для данного PR (контроллер root-equivalent), но стоит отследить отдельно.

Хотите, чтобы я создал follow-up issue для будущего credential-drop (gosu/su-exec или Electron sandbox-compatible mechanism), чтобы не потерять контекст?


✏️ Learnings added
Learnt from: ezocomp118-source
Repo: ProverCoderAI/docker-git PR: 366
File: packages/api/src/services/skiller.ts:366-369
Timestamp: 2026-06-03T17:35:44.689Z
Learning: In ProverCoderAI/docker-git, launching Electron via `setpriv --reuid/--regid` in the controller container causes a `Trace/breakpoint trap` crash even with `--no-sandbox`. The confirmed workaround is to launch Skiller as the controller user (bash -lc) and enforce project scope at the software level via SkillerContainerScope env vars (HOME, XDG_*, DOCKER_GIT_SKILLER_CONTAINER_HOME_PATH) and the add_project tRPC call. A future fix should use gosu/su-exec or a Chromium-sandbox-compatible credential-drop mechanism instead of setpriv. See PR `#366` for full context.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: CR
Repo: ProverCoderAI/docker-git PR: 0
File: docs/integrations/skiller.md:0-0
Timestamp: 2026-05-14T16:02:05.012Z
Learning: Applies to docs/integrations/**/*.(ts|js)?(@(routes|handlers|api))* : API endpoint `POST /projects/by-key/:projectKey/terminal-sessions/:sessionId/skiller/open` must launch Skiller Electron app, register terminal session filesystem scope, and write output to ~/.docker-git/logs/skiller.log

Learnt from: CR
Repo: ProverCoderAI/docker-git PR: 0
File: docs/integrations/skiller.md:0-0
Timestamp: 2026-05-14T16:02:05.012Z
Learning: Applies to docs/integrations/**/*.(ts|js|sh|bash)?(launcher|skiller)* : Skiller launcher must use `xvfb-run` if available when API process has no `$DISPLAY` environment variable to enable headless startup

Learnt from: CR
Repo: ProverCoderAI/docker-git PR: 0
File: docs/integrations/skiller.md:0-0
Timestamp: 2026-05-14T16:02:05.012Z
Learning: Applies to docs/integrations/**/*.(ts|js)?(@(routes|handlers|api|middleware))* : API must scope Skiller filesystem access to the active project container by mapping `/home/<sshUser>` and project `targetDir` to controller-visible Docker volume paths

Learnt from: CR
Repo: ProverCoderAI/docker-git PR: 0
File: docs/integrations/skiller.md:0-0
Timestamp: 2026-05-14T16:02:05.012Z
Learning: docker-git must not import Skiller source code into the docker-git web bundle; serve Skiller browser UI from Skiller's own built renderer and proxy tRPC requests to Skiller's separate backend process

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.

Comment thread packages/api/tests/skiller-routes.test.ts Outdated
Comment thread packages/app/tests/docker-git/controller-resource-limits.test.ts Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Почему-то кнопка skiller не работает

2 participants