Шаг сборки инсталлятора Inno Setup в GA #51
Conversation
WalkthroughДобавляет сборку Windows‑инсталлятора в release workflow: версия извлекается из Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Repo as Repository
participant IS as Inno Setup (makensis)
participant Release as GitHub Release
GH->>Repo: checkout + read `packagedef` (extract version)
GH->>GH: create `dist/` dir
GH->>IS: run makensis with /DAppVersion=<version>
IS-->>GH: produce `dist/ovm-setup.exe`
GH->>Release: upload `ovm.exe` and `dist/ovm-setup.exe`
sequenceDiagram
participant User as Пользователь
participant Installer as ovm-setup.exe
participant Reg as HKCU\Environment\Path
User->>Installer: запускает установку
Installer->>Reg: читает PATH
alt путь отсутствует
Installer->>Reg: добавляет `{app}` в PATH
Installer->>User: лог об успешном добавлении
else путь уже есть
Installer->>User: лог — путь уже присутствует
end
User->>Installer: при деинсталляции
Installer->>Reg: удаляет `{app}` из PATH (нормализация)
Installer->>User: лог об удалении/ошибке
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 4
🤖 Fix all issues with AI agents
In @.github/workflows/release.yml:
- Around line 46-52: Update the GitHub Action reference for the release asset
upload step: replace the uses: declaration that currently reads
AButler/upload-release-assets@v2.0.2 with AButler/upload-release-assets@v3.0 in
the "Upload release assets" job; no other parameter changes are required since
v3.0 is compatible with the existing files and repo-token syntax and
GitHub-hosted runners now support the Node 20 requirement.
In @install/ovm.iss:
- Around line 67-103: The comment above the semicolon check in CurStepChanged is
misleading: change the comment "Remove trailing backslash if present" to
accurately describe the code which checks for a trailing semicolon; update the
comment near the OrigPath/ NewPath logic (around the check using
Length(OrigPath) and OrigPath[Length(OrigPath)] = ';') to something like "Remove
trailing semicolon if present" or otherwise reflect that it tests for ';' before
appending AppPath.
- Around line 105-141: CurUninstallStepChanged currently only removes exact
matches of AppPath from PathList, which misses entries that include a trailing
path separator; update the removal logic in CurUninstallStepChanged to normalize
entries and AppPath before comparison (e.g., trim any trailing '\' or '/' and
perform case-insensitive compare) so that both forms (with and without trailing
slash) are detected and deleted; reference PathList, AppPath and NeedsAddPath to
apply the same normalization used when adding paths and ensure Log still reports
the removed path and RegWriteStringValue writes the cleaned NewPath.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/release.ymlinstall/ovm.iss
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: test (default, 1testrunner) / build (windows-latest, default, 1testrunner)
- GitHub Check: test (dev, 1testrunner) / build (ubuntu-latest, dev, 1testrunner)
- GitHub Check: test (dev, 1testrunner) / build (windows-latest, dev, 1testrunner)
- GitHub Check: test (default, 1testrunner) / build (macos-latest, default, 1testrunner)
- GitHub Check: test (dev, 1testrunner) / build (windows-latest, dev, 1testrunner)
- GitHub Check: test (stable, 1testrunner) / build (ubuntu-latest, stable, 1testrunner)
- GitHub Check: test (dev, 1testrunner) / build (ubuntu-latest, dev, 1testrunner)
- GitHub Check: test (default, 1testrunner) / build (ubuntu-latest, default, 1testrunner)
- GitHub Check: test (stable, 1testrunner) / build (windows-latest, stable, 1testrunner)
- GitHub Check: test (default, 1testrunner) / build (windows-latest, default, 1testrunner)
- GitHub Check: test (stable, 1testrunner) / build (macos-latest, stable, 1testrunner)
🔇 Additional comments (5)
install/ovm.iss (3)
1-12: LGTM! Корректное использование условной компиляции.Определения констант выглядят правильно, а условная компиляция версии позволяет как локальную сборку, так и передачу версии из CI/CD.
48-65: LGTM! Корректная проверка существования пути в PATH.Функция правильно обрабатывает оба варианта пути (с/без завершающего слеша) и использует регистронезависимое сравнение. Логика с добавлением точек с запятой для избежания частичных совпадений - правильный подход.
Опционально: Строка 64 довольно длинная, но это не критично для скриптов Inno Setup.
14-36: Относительные пути работают корректно — коррекция не требуется.Конфигурация установки правильная: per-user установка, минимальные привилегии, уведомление об изменении PATH. Относительные пути
..\distи..\ovm.exeработают корректно, так как Inno Setup разрешает пути относительно расположения файла скрипта (каталогinstall/), а не рабочей директории. Поэтому..\distизinstall/ovm.issразрешается какdist/в корне репозитория, а..\ovm.exe— какovm.exeв корне. Оба файла создаются в правильных местах до запуска Docker-контейнера (строки 33-38 в release.yml).Likely an incorrect or invalid review comment.
.github/workflows/release.yml (2)
30-35: Проверьте формат версии в packagedef.Regex-паттерн
\.Версия\("\K[^"]+предполагает специфический формат версии в файле packagedef (с кириллицей "Версия"). Убедитесь, что формат точно соответствует ожидаемому.Скрипт проверки формата уже включён в предыдущий комментарий для install/ovm.iss (показывает содержимое packagedef).
37-38: LGTM!Создание директории dist перед сборкой инсталлятора - правильный подход.
| - name: Build Windows installer with Inno Setup | ||
| run: | | ||
| docker run --rm -i -v "$GITHUB_WORKSPACE:/work" -w /work amake/innosetup \ | ||
| /DMyAppVersion="${{ steps.get_version.outputs.version }}" \ | ||
| install/ovm.iss |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
amake/innosetup Docker image maintenance security vulnerabilities
💡 Result:
Summary — amake/innosetup Docker image
- Maintainer / source: published by user "amake" (Aaron Madlon‑Kay). Image page and maintainer account on Docker Hub. [1][2]
- Activity: Docker Hub shows recent updates (tags like innosetup6-bookworm; last-updated info visible on the Hub). Check the Hub page for current tag timestamps before use. [1]
- Composition / risk surface: the image runs Inno Setup under Wine inside a Debian-based container (see repo/gist). That means security surface includes: the Inno Setup binary, Wine, and the base Linux distro (and any packages the Dockerfile installs). [2][4]
- Known CVE affecting Inno Setup: CVE-2017-20051 (uncontrolled search path / installer issue) is recorded in public vulnerability summaries (CISA bulletin). If you build or run untrusted installer scripts/binaries, treat this as a potential risk. [3]
Recommendations
- Inspect the image Dockerfile / tags on Docker Hub or upstream repo to confirm base OS and package versions before use. [1][2]
- Run an image scan (e.g., Trivy/Grype/Anchore) against the exact tag you intend to use to surface OS and package CVEs. Address any high/critical findings. (Automate scans in CI.)
- If you need stronger guarantee, rebuild from source (Dockerfile) using pinned, up‑to‑date base images and patch versions; run runtime hardening (non‑root user, read‑only filesystem where possible).
- Avoid running untrusted Inno Setup scripts or payloads inside the container; validate inputs and sign artifacts where possible.
Sources
- Docker Hub: amake/innosetup (image page, tags, last-updated). [1]
- Docker Hub Dockerfile / maintainer info and related amake images. [2]
- CISA vulnerability bulletin referencing Inno Setup issue (CVE-2017-20051). [3]
- amake’s Inno Setup on Linux/macOS gist (usage and composition details). [4]
Would you like me to scan a specific tag/digest of amake/innosetup for known CVEs now?
Предусмотрите сканирование Docker-образа на уязвимости перед использованием.
Docker-образ amake/innosetup активно поддерживается (Aaron Madlon-Kay, недавние обновления на Docker Hub). Однако для Inno Setup зарегистрирована CVE-2017-20051 (недопустимое управление путём поиска), и образ включает Wine + Debian, что увеличивает поверхность атаки.
Рекомендации:
- Добавьте сканирование образа в CI-пайплайн (Trivy, Grype, Anchore) для выявления CVE в ОС и пакетах
- Используйте конкретный тег (не
latest) и проверьте Dockerfile образа перед использованием - Убедитесь, что скрипты установщика (.iss) валидируют входные данные и не используют ненадёжные источники
| - name: Upload release assets | ||
| uses: AButler/upload-release-assets@v2.0.2 | ||
| with: | ||
| files: ovm.exe | ||
| files: | | ||
| ovm.exe | ||
| dist/ovm-setup.exe | ||
| repo-token: ${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
AButler upload-release-assets GitHub Action latest version releases
💡 Result:
The latest release is v3.0 (released Nov 20, 2023). [1][2]
Sources:
[1] AButler/upload-release-assets GitHub repository (Releases).
[2] Upload Release Assets — GitHub Marketplace.
🌐 Web query:
AButler upload-release-assets v3.0 breaking changes migration guide from v2.0.2
💡 Result:
Here’s a concise migration checklist from v2.0.2 → v3.0 for AButler/upload-release-assets, with sources.
- Update workflow reference
- Change uses: AButler/upload-release-assets@v2.0.2 → AButler/upload-release-assets@v3.0.
Source: README usage. [1]
- Node runtime / self-hosted runners
- v3.0 upgrades to Node 20 (action built for node20). If you run on self-hosted runners, ensure Node 20 is available or use GitHub-hosted runners.
Source: v3.0 release notes. [2]
- New / changed inputs
- v3.0 explicitly documents/permits release-id to be specified as an input (optional). If you previously relied only on the action-derived release, you can now pass release-id to target a specific release. Confirm any workflows that computed/expected upload_url behavior still work.
Source: README inputs & v3.0 notes. [1][2]
- Dependency / internals changes (non-functional unless you vendor or extend)
- Dependencies bumped (fast-glob, semver, @actions/core). Expect no functional change, but if you fork/patch the action, update accordingly.
Source: v3.0 changelog. [2]
- Verify file-glob formatting remains the same
- The action still expects semicolon-separated globs and forward slashes; no change documented but verify uploads in a CI run. [1]
- Recommended steps to migrate safely
- Pin the new major tag in a test branch (uses: AButler/upload-release-assets@v3.0).
- Run a CI test that uploads to a draft/test release (or specify release-id) to confirm behavior.
- If using self-hosted runner, install/verify Node 20 before switching.
- If anything relies on older dependency behavior, test and open an issue on the repo. [1][2]
References
- README / inputs and usage. [1]
- v3.0 Release notes (what changed: Node upgrade, dependency bumps, allow release-id). [2]
If you’d like, I can produce a one-line replacement snippet for your workflow and a small test workflow that uploads to a draft release.
Обновите версию GitHub Action на v3.0.
Версия AButler/upload-release-assets@v2.0.2 устарела. Доступна более новая версия v3.0 (выпущена 20 ноября 2023). Обновление совместимо с текущей конфигурацией: формат файлов и синтаксис параметров остаются неизменными. v3.0 требует Node 20 (уже доступен на GitHub-hosted runners). Замените uses: AButler/upload-release-assets@v2.0.2 на uses: AButler/upload-release-assets@v3.0.
🤖 Prompt for AI Agents
In @.github/workflows/release.yml around lines 46 - 52, Update the GitHub Action
reference for the release asset upload step: replace the uses: declaration that
currently reads AButler/upload-release-assets@v2.0.2 with
AButler/upload-release-assets@v3.0 in the "Upload release assets" job; no other
parameter changes are required since v3.0 is compatible with the existing files
and repo-token syntax and GitHub-hosted runners now support the Node 20
requirement.
Pull Request Test Coverage Report for Build 20880966028Details
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
This pull request adds Windows installer support for the OVM (OneScript Version Manager) tool using Inno Setup. The installer automatically configures the system PATH environment variable during installation.
Key Changes:
- Added Inno Setup script (install/ovm.iss) that creates a Windows installer with automatic PATH configuration
- Modified GitHub Actions workflow to extract version from packagedef, build the installer using Docker, and upload both portable executable and installer
- Installer supports per-user installation (no admin rights required) and proper PATH cleanup on uninstall
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| install/ovm.iss | New Inno Setup script that installs ovm.exe to %LOCALAPPDATA%\ovm, adds it to user PATH, and handles clean uninstallation |
| .github/workflows/release.yml | Updated release workflow to extract version from packagedef, build Windows installer via Docker, and upload both ovm.exe and the installer as release assets |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Name: "russian"; MessagesFile: "compiler:Languages\Russian.isl" | ||
|
|
||
| [Files] | ||
| Source: "..\ovm.exe"; DestDir: "{app}"; Flags: ignoreversion |
There was a problem hiding this comment.
The source file path uses a relative path '..\ovm.exe' which assumes the script is being run from the install/ directory. The GitHub Actions workflow runs the Inno Setup compiler from the workspace root (/work), so this path should correctly resolve to ovm.exe in the parent directory. However, consider using an absolute path or verifying that this relative path works correctly with the Docker container's working directory.
| Source: "..\ovm.exe"; DestDir: "{app}"; Flags: ignoreversion | |
| Source: "{#SourcePath}\..\ovm.exe"; DestDir: "{app}"; Flags: ignoreversion |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @install/ovm.iss:
- Around line 73-85: Bug: when OrigPath is empty the current logic builds
NewPath as ';' + AppPath producing a leading empty PATH segment. In the
ssPostInstall block (look for CurStep = ssPostInstall, AppPath, NeedsAddPath,
RegQueryStringValue, OrigPath, NewPath, EnvironmentKey) change the concatenation
logic to handle empty OrigPath first: if OrigPath is empty set NewPath :=
AppPath; otherwise keep the existing logic (if last char = ';' then append
AppPath else append ';' + AppPath). Also consider trimming OrigPath before
checking length/last-char to avoid whitespace edge cases.
- Around line 14-16: AppId line contains a mismatched/unterminated brace
("AppId={{8E5F4A2B-9C3D-4F1E-A6B8-2D7C9E4F1A3B}") which will produce an
incorrect AppId; update the AppId value to properly close the GUID with double
braces (i.e., ensure the token is formatted as {{GUID}}) so the opening "{{" has
a matching "}}", and verify the GUID string inside remains unchanged and
correctly formatted.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
install/ovm.iss
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: test (stable, 1testrunner) / build (windows-latest, stable, 1testrunner)
- GitHub Check: test (stable, 1testrunner) / build (macos-latest, stable, 1testrunner)
- GitHub Check: test (default, 1testrunner) / build (ubuntu-latest, default, 1testrunner)
- GitHub Check: test (dev, oneunit) / build (ubuntu-latest, dev, oneunit)
- GitHub Check: test (stable, 1testrunner) / build (ubuntu-latest, stable, 1testrunner)
- GitHub Check: test (dev, 1testrunner) / build (macos-latest, dev, 1testrunner)
- GitHub Check: sonar / test
- GitHub Check: test (dev, 1testrunner) / build (windows-latest, dev, 1testrunner)
- GitHub Check: test (stable, 1testrunner) / build (macos-latest, stable, 1testrunner)
- GitHub Check: test (dev, 1testrunner) / build (windows-latest, dev, 1testrunner)
- GitHub Check: test (dev, 1testrunner) / build (ubuntu-latest, dev, 1testrunner)
- GitHub Check: test (dev, oneunit) / build (ubuntu-latest, dev, oneunit)
- GitHub Check: test (default, 1testrunner) / build (ubuntu-latest, default, 1testrunner)
- GitHub Check: test (dev, oneunit) / build (windows-latest, dev, oneunit)
- GitHub Check: test (default, 1testrunner) / build (windows-latest, default, 1testrunner)
- GitHub Check: test (dev, 1testrunner) / build (macos-latest, dev, 1testrunner)
- GitHub Check: test (default, 1testrunner) / build (macos-latest, default, 1testrunner)
- GitHub Check: test (stable, 1testrunner) / build (ubuntu-latest, stable, 1testrunner)
- GitHub Check: test (stable, 1testrunner) / build (windows-latest, stable, 1testrunner)
- GitHub Check: sonar / test
🔇 Additional comments (1)
install/ovm.iss (1)
30-35: Уточнить поддержку архитектур в установщике.
ArchitecturesInstallIn64BitMode=x64не запрещает установку на 32-bit системы. Еслиovm.exeпредназначен только для x64, добавьте явное ограничение черезArchitecturesAllowed=x64. Если бинарник должен работать на обеих архитектурах (что возможно для .NET exe), соответственно обновите конфигурацию установщика.Возможный фикс (если x64-only)
PrivilegesRequired=lowest PrivilegesRequiredOverridesAllowed=commandline ChangesEnvironment=yes +ArchitecturesAllowed=x64 ArchitecturesInstallIn64BitMode=x64 UninstallDisplayIcon={app}\{#MyAppExeName}
| [Setup] | ||
| ; NOTE: The value of AppId uniquely identifies this application. | ||
| AppId={{8E5F4A2B-9C3D-4F1E-A6B8-2D7C9E4F1A3B} |
There was a problem hiding this comment.
Критично: AppId выглядит незакрытым (экранирование {}), инсталлятор может не собраться/получить некорректный AppId.
Сейчас AppId={{...} заканчивается одиночной }, из-за чего легко получить литерал без закрывающей скобки. Лучше явно закрыть GUID как {{GUID}}.
Возможный фикс
AppId={{8E5F4A2B-9C3D-4F1E-A6B8-2D7C9E4F1A3B}
+; ожидание: литерал "{GUID}" => экранирование "{" и "}" удвоением
+; AppId={{8E5F4A2B-9C3D-4F1E-A6B8-2D7C9E4F1A3B}}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @install/ovm.iss around lines 14 - 16, AppId line contains a
mismatched/unterminated brace ("AppId={{8E5F4A2B-9C3D-4F1E-A6B8-2D7C9E4F1A3B}")
which will produce an incorrect AppId; update the AppId value to properly close
the GUID with double braces (i.e., ensure the token is formatted as {{GUID}}) so
the opening "{{" has a matching "}}", and verify the GUID string inside remains
unchanged and correctly formatted.
There was a problem hiding this comment.
Это частый паттерн использования appId и экранирования appId. Не ошибка
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Inno Setup AppId GUID curly braces escaping syntax
💡 Result:
Short answer: In [Setup] AppId braces must be doubled in the script because single { } starts an Inno constant. Use {{GUID}} (and }} for the close) or generate the doubled-braces string via the preprocessor. Example:
-
Direct (literal doubled braces):
AppId={{FDDFBC15-7B5C-4580-AD69-F54B8087A52E}} -
Using preprocessor to keep a single-brace value for code:
#define AppId "{FDDFBC15-7B5C-4580-AD69-F54B8087A52E}"
AppId={#StringChange(AppId, '{', '{{')}
Notes: Pascal Script and registry code expect the single-brace form (e.g. "{GUID}"), so only the [Setup] directive needs doubled braces in the script. (stackoverflow.com)
Citations:
🌐 Web query:
Inno Setup AppId single closing brace versus double closing brace
💡 Result:
Short answer: In [Setup] directives (like AppId) Inno Setup treats "{" as the start of a constant, so you must double the opening brace to get a literal "{". The closing brace does not need doubling. Therefore to get an AppId value of {XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX} write it in the script as:
AppId={{XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX}
(That produces a single leading "{" and a single trailing "}" at runtime.) [1][2]
References:
- Inno Setup AppId documentation. [1]
- Explanation and example on Stack Overflow. [2]
@EvilBeaver, вы абсолютно правы, приношу извинения за ошибочный комментарий!
Согласно документации Inno Setup, в секции [Setup] только открывающая фигурная скобка требует экранирования удвоением (так как { начинает константу), а закрывающая скобка остаётся одиночной. Поэтому формат AppId={{GUID} корректен и даёт на выходе {GUID}.
Спасибо за разъяснение!
✏️ Learnings added
Learnt from: EvilBeaver
Repo: oscript-library/ovm PR: 51
File: install/ovm.iss:14-16
Timestamp: 2026-01-10T17:56:17.816Z
Learning: В Inno Setup при указании AppId в секции [Setup] только открывающая фигурная скобка должна быть удвоена: `AppId={{GUID}` — это правильный синтаксис, который даёт значение `{GUID}` в рантайме. Закрывающая скобка остаётся одиночной.
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.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/release.yml:
- Around line 37-41: The workflow step "Create dist directory" uses an insecure
chmod 777; replace it with stricter permissions (e.g., change the chmod 777 dist
call to chmod 755 dist or chmod 750 dist) or remove the chmod entirely if the CI
runner/Docker uses the correct user; update the step that runs "mkdir -p dist"
so it either sets proper permissions (chmod 755/750) or relies on the runner
user rather than granting wide open access.
- Around line 30-36: The get_version step currently extracts VERSION from
packagedef but doesn't validate it; update the step (id: get_version) to check
that the VERSION variable is non-empty after the grep, and if empty emit an
error (e.g. echo "::error::Failed to extract version from packagedef") and exit
1 so the workflow fails instead of passing an empty version into Inno Setup;
only write to $GITHUB_OUTPUT when VERSION is valid (echo "version=$VERSION" >>
$GITHUB_OUTPUT) and optionally print the extracted version when successful.
🧹 Nitpick comments (2)
.github/workflows/release.yml (2)
48-54: Рассмотрите добавление проверки существования файлов перед загрузкой.Хотя предыдущие шаги должны создать оба файла, добавление явной проверки их существования перед загрузкой поможет избежать неявных сбоев.
✅ Предлагаемое улучшение с проверкой файлов
Добавьте шаг проверки перед загрузкой:
- name: Verify build artifacts run: | test -f ovm.exe || { echo "Error: ovm.exe not found"; exit 1; } test -f dist/ovm-setup.exe || { echo "Error: dist/ovm-setup.exe not found"; exit 1; } echo "All build artifacts verified" - name: Upload release assets uses: AButler/upload-release-assets@v2.0.2 with: files: | ovm.exe dist/ovm-setup.exe repo-token: ${{ secrets.GITHUB_TOKEN }}
42-47: Флаг-iв Docker можно удалить как излишний, но код функционально корректен.Файл
install/ovm.issсуществует и правильно обрабатывает параметрMyAppVersion(определён на строках 7-9 с fallback-значением и используется на строке 18). Флаг-i(интерактивный режим) не требуется в CI/CD-среде и может быть удалён для чистоты.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/release.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (default, 1testrunner) / build (windows-latest, default, 1testrunner)
- GitHub Check: test (stable, 1testrunner) / build (windows-latest, stable, 1testrunner)
- GitHub Check: test (dev, 1testrunner) / build (ubuntu-latest, dev, 1testrunner)
- GitHub Check: test (default, 1testrunner) / build (ubuntu-latest, default, 1testrunner)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
install/ovm.iss (1)
77-106: Возможна несогласованность при добавлении пути в PATH.В строке 79
AppPathполучается черезExpandConstant('{app}')и добавляется в PATH без нормализации (удаления завершающего обратного слэша), в то время как функцияNeedsAddPath(строки 54-56) нормализует путь перед проверкой. Это может привести к дублированию записей в PATH, например:C:\...\ovmиC:\...\ovm\могут считаться разными путями.Рекомендация: Нормализуйте
AppPathвCurStepChangedперед добавлением в PATH, аналогично логике вNeedsAddPathиCurUninstallStepChanged.♻️ Предлагаемое исправление
if CurStep = ssPostInstall then begin AppPath := ExpandConstant('{app}'); + // Normalize: remove trailing backslash + if (Length(AppPath) > 0) and (AppPath[Length(AppPath)] = '\') then + AppPath := Copy(AppPath, 1, Length(AppPath) - 1); + if NeedsAddPath(AppPath) then beginДополнительно: Хотя
ChangesEnvironment=yesв секции[Setup]должен уведомлять систему об изменениях, стоит проверить, что изменения PATH корректно отображаются в новых процессах без перезагрузки.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
install/ovm.iss
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: sonar / test
- GitHub Check: test (dev, 1testrunner) / build (ubuntu-latest, dev, 1testrunner)
- GitHub Check: test (dev, 1testrunner) / build (windows-latest, dev, 1testrunner)
- GitHub Check: test (default, 1testrunner) / build (windows-latest, default, 1testrunner)
- GitHub Check: test (dev, 1testrunner) / build (ubuntu-latest, dev, 1testrunner)
- GitHub Check: test (stable, 1testrunner) / build (windows-latest, stable, 1testrunner)
- GitHub Check: test (dev, oneunit) / build (ubuntu-latest, dev, oneunit)
- GitHub Check: test (stable, 1testrunner) / build (ubuntu-latest, stable, 1testrunner)
- GitHub Check: test (default, 1testrunner) / build (ubuntu-latest, default, 1testrunner)
- GitHub Check: test (default, 1testrunner) / build (macos-latest, default, 1testrunner)
- GitHub Check: sonar / test
🔇 Additional comments (4)
install/ovm.iss (4)
1-12: Метаданные приложения настроены корректно.Использование условной компиляции для версии с возможностью передачи через
/Dиз GitHub Actions и значением по умолчанию для локальных сборок — правильное решение.
14-35: Конфигурация инсталлятора выглядит валидно.Установка в пользовательский каталог без прав администратора (
PrivilegesRequired=lowest) и автоматическое уведомление об изменениях в окружении (ChangesEnvironment=yes) соответствуют лучшим практикам для менеджера версий.
41-42: Секция Files настроена корректно.Копирование
ovm.exeс флагомignoreversion— стандартная практика для исполняемых файлов.
109-164: Логика удаления из PATH реализована корректно.Процедура деинсталляции правильно обрабатывает PATH:
- Нормализация путей перед сравнением (строки 122-123, 147-148)
- Удаление пустых записей для предотвращения
;;в PATH- Обратная итерация для безопасного удаления элементов
- Регистронезависимое сравнение
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @install/ovm.iss:
- Around line 156-161: Добавьте проверку результата вызова RegWriteStringValue
при записи нового Path (переменные PathList, NewPath) и обработку ошибки
аналогично тому, как это сделано в CurStepChanged: сохранить возвращаемое
значение, при неуспехе залогировать сообщение об ошибке через используемый
логгер/MsgBox с контекстом операции (например "Failed to update PATH during
uninstall") и принять соответствующее действие (например прервать
установку/установить флаг ошибки) вместо игнорирования результата; убедитесь,
что PathList.Free остаётся в finally-блоке.
🧹 Nitpick comments (1)
.github/workflows/release.yml (1)
41-44:chmod 777избыточен и слишком разрешителен.В контексте CI это не создаёт угрозы безопасности, но
chmod 777является плохой практикой. Docker-контейнер запускается от root и сможет писать в директорию без дополнительных разрешений. Достаточно простоmkdir -p dist.♻️ Предлагаемое упрощение
- name: Create dist directory run: | mkdir -p dist - chmod 777 dist
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/release.ymlinstall/ovm.iss
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: test (default, 1testrunner) / build (windows-latest, default, 1testrunner)
- GitHub Check: test (dev, oneunit) / build (windows-latest, dev, oneunit)
- GitHub Check: test (dev, 1testrunner) / build (windows-latest, dev, 1testrunner)
- GitHub Check: test (dev, oneunit) / build (ubuntu-latest, dev, oneunit)
- GitHub Check: test (stable, 1testrunner) / build (windows-latest, stable, 1testrunner)
- GitHub Check: test (dev, 1testrunner) / build (macos-latest, dev, 1testrunner)
- GitHub Check: test (stable, 1testrunner) / build (ubuntu-latest, stable, 1testrunner)
- GitHub Check: test (stable, 1testrunner) / build (macos-latest, stable, 1testrunner)
- GitHub Check: test (default, 1testrunner) / build (macos-latest, default, 1testrunner)
- GitHub Check: test (dev, 1testrunner) / build (windows-latest, dev, 1testrunner)
- GitHub Check: test (default, 1testrunner) / build (ubuntu-latest, default, 1testrunner)
- GitHub Check: test (stable, 1testrunner) / build (ubuntu-latest, stable, 1testrunner)
- GitHub Check: test (dev, 1testrunner) / build (macos-latest, dev, 1testrunner)
- GitHub Check: test (dev, 1testrunner) / build (ubuntu-latest, dev, 1testrunner)
- GitHub Check: test (default, 1testrunner) / build (windows-latest, default, 1testrunner)
- GitHub Check: test (stable, 1testrunner) / build (windows-latest, stable, 1testrunner)
- GitHub Check: test (dev, oneunit) / build (macos-latest, dev, oneunit)
- GitHub Check: test (dev, oneunit) / build (ubuntu-latest, dev, oneunit)
- GitHub Check: sonar / test
🔇 Additional comments (6)
install/ovm.iss (3)
14-35: Конфигурация установки выглядит корректно.Правильно настроена установка на уровне пользователя (
PrivilegesRequired=lowest), что позволяет установку без прав администратора.ChangesEnvironment=yesкорректно уведомит Windows об изменениях переменных окружения.
48-69: Функция проверки PATH реализована корректно.Нормализация пути (удаление завершающего
\) и регистронезависимое сравнение обеспечивают надёжную проверку наличия пути в переменной PATH.
71-107: Процедура добавления в PATH реализована корректно.Корректно обрабатывает случаи существующего и отсутствующего PATH, проверяет результат записи в реестр и логирует действия.
.github/workflows/release.yml (3)
30-39: Извлечение версии реализовано корректно.Паттерн
grep -oP '\.Версия\("\K[^"]+'корректно извлекает версию изpackagedef. Наличие|| trueпредотвращает падение пайплайна при отсутствии совпадений, а последующая проверка[ -z "$VERSION" ]обеспечивает явную ошибку.
56-62: Загрузка артефактов настроена корректно.Оба файла (
ovm.exeиdist/ovm-setup.exe) будут загружены в релиз. Версия действия зафиксирована — это хорошая практика.
46-54: Шаг сборки инсталлятора выглядит корректно.Проверка наличия
ovm.exeперед запуском Docker — хорошая практика. Образamake/innosetupактивно поддерживается (последнее обновление ~16 часов назад, ~500K скачиваний на Docker Hub).
…t-library/ovm into feature/windows-installer
|
0 New Issues
0 Fixed Issues
0 Accepted Issues
No data about coverage (75.60% Estimated after merge)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @install/ovm.iss:
- Around line 85-129: In AddToPath: enforce a conservative PATH length limit and
surface errors to the user, and mitigate the TOCTOU race between
RegQueryStringValue and RegWriteStringValue by re-reading and merging PATH just
before write; specifically, add a constant (e.g., MAX_PATH_LENGTH = 2047), after
computing NewPath re-read the current PATH via RegQueryStringValue (use
EnvironmentKey and 'Path'), merge NormalizedPath into the freshly read value
only if not present, then check Length(NewPath) > MAX_PATH_LENGTH and if so Log
the length and show MsgBox explaining the failure and exit; after
RegWriteStringValue fails, replace the silent Log with a MsgBox informing the
user to add NormalizedPath manually (keep using PathExistsInEnv, NormalizePath,
RegQueryStringValue, and RegWriteStringValue to locate code).
🧹 Nitpick comments (2)
install/ovm.iss (2)
213-222: Рассмотрите проверку результата изменения PATH.Возвращаемое значение
AddToPathне проверяется. Если добавление в PATH не удастся, пользователь не узнает об этом (кроме записи в логе установки).♻️ Рекомендуемое улучшение
procedure CurStepChanged(CurStep: TSetupStep); var AppPath: string; begin if CurStep = ssPostInstall then begin AppPath := ExpandConstant('{app}'); - AddToPath(AppPath); + if not AddToPath(AppPath) then + begin + MsgBox('Не удалось автоматически добавить путь в PATH.' + #13#10 + + 'Пожалуйста, добавьте путь вручную: ' + AppPath + #13#10#13#10 + + 'Инструкции: Настройки Windows → Система → О системе → ' + + 'Дополнительные параметры системы → Переменные среды', + mbInformation, MB_OK); + end; end; end;
224-233: Опционально: добавьте проверку результата удаления из PATH.Аналогично процедуре установки, возвращаемое значение
RemoveFromPathне проверяется. Если удаление не удастся, в PATH останется недействительный путь.Это менее критично, чем при установке, но стоит рассмотреть информирование пользователя.
♻️ Опциональное улучшение
procedure CurUninstallStepChanged(CurUninstallStep: TUninstallStep); var AppPath: string; begin if CurUninstallStep = usPostUninstall then begin AppPath := ExpandConstant('{app}'); - RemoveFromPath(AppPath); + if not RemoveFromPath(AppPath) then + Log('Предупреждение: не удалось удалить путь из PATH: ' + AppPath); end; end;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
install/ovm.iss
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: test (dev, oneunit) / build (ubuntu-latest, dev, oneunit)
- GitHub Check: test (dev, oneunit) / build (macos-latest, dev, oneunit)
- GitHub Check: test (default, 1testrunner) / build (ubuntu-latest, default, 1testrunner)
- GitHub Check: test (stable, 1testrunner) / build (ubuntu-latest, stable, 1testrunner)
- GitHub Check: test (dev, oneunit) / build (windows-latest, dev, oneunit)
- GitHub Check: test (default, 1testrunner) / build (macos-latest, default, 1testrunner)
- GitHub Check: test (default, 1testrunner) / build (windows-latest, default, 1testrunner)
- GitHub Check: test (dev, 1testrunner) / build (windows-latest, dev, 1testrunner)
- GitHub Check: test (dev, 1testrunner) / build (ubuntu-latest, dev, 1testrunner)
- GitHub Check: sonar / test
- GitHub Check: test (default, 1testrunner) / build (ubuntu-latest, default, 1testrunner)
- GitHub Check: test (default, 1testrunner) / build (windows-latest, default, 1testrunner)
- GitHub Check: test (stable, 1testrunner) / build (windows-latest, stable, 1testrunner)
- GitHub Check: test (default, 1testrunner) / build (macos-latest, default, 1testrunner)
- GitHub Check: test (dev, 1testrunner) / build (ubuntu-latest, dev, 1testrunner)
- GitHub Check: test (stable, 1testrunner) / build (macos-latest, stable, 1testrunner)
- GitHub Check: sonar / test
- GitHub Check: test (dev, 1testrunner) / build (macos-latest, dev, 1testrunner)
- GitHub Check: test (dev, 1testrunner) / build (windows-latest, dev, 1testrunner)
- GitHub Check: test (stable, 1testrunner) / build (ubuntu-latest, stable, 1testrunner)
🔇 Additional comments (5)
install/ovm.iss (5)
56-83: Логика проверки пути в PATH выглядит корректно.Функция правильно обрабатывает основные сценарии:
- Нормализует входной путь для единообразия
- Проверяет наличие пути в двух вариантах (с завершающим
\и без)- Использует регистронезависимое сравнение (корректно для Windows)
- Добавляет разделители по краям для точного поиска подстроки
Примечание: В крайне редких случаях, если PATH содержит пути с экранированными точками с запятой, возможны ложные срабатывания. Однако это маловероятно в стандартных сценариях использования Windows PATH.
48-54: LGTM!Функция корректно удаляет завершающий обратный слеш, обеспечивая единообразие путей для последующего сравнения. Реализация простая и эффективная.
14-35: AppId GUID уже уникален в контексте проекта — это единственный установщик в кодовой базе.Поиск показал, что GUID
{8E5F4A2B-9C3D-4F1E-A6B8-2D7C9E4F1A3B}используется только один раз в репозитории и в файлеinstall/ovm.issотсутствуют другие установщики (.issили.nsiфайлы). Конфликтов с другими AppId не обнаружено.
25-25: Проверьте поведение относительных путей при выполнении Inno Setup из репозитория корне.В GitHub Actions workflow (release.yml) Inno Setup запускается из корня репозитория:
- Line 28:
ovm.exeсобирается в корень репозитория- Line 41-44: директория
dist/создаётся в корне репозитория- Line 52-54: docker запускается с
-w /work(корень репозитория)Однако в install/ovm.iss используются относительные пути:
- Line 25:
OutputDir=..\dist- Line 42:
Source: "..\ovm.exe"Эти пути корректны только если Inno Setup автоматически переходит в директорию скрипта (
install/). Убедитесь, что Inno Setup в docker-контейнере именно так себя ведёт. В противном случае пути следует изменить наOutputDir=distиSource: "ovm.exe".
34-34: Уточните намерение относительно поддержки 32-битных систем.Директива
ArchitecturesInstallIn64BitMode=x64не ограничивает установку только на x64 системах. Она указывает, что на x64 системах установщик должен работать в 64-битном режиме. Без явной директивыArchitectures, Inno Setup по умолчанию поддерживает установку на обе архитектуры (32-бит и 64-бит).Так как
ovm.exe— это .NET сборка (скомпилирована командойoscript -make), она может работать на обеих архитектурах и будет использовать соответствующий режим (32-бит на x86, 64-бит на x64).Если предполагается поддерживать только x64, добавьте:
Architectures=x64Если поддерживаются обе архитектуры (что соответствует текущим требованиям в README), текущая конфигурация верна и изменения не требуются.
Likely an incorrect or invalid review comment.
| // Добавляет путь в переменную окружения PATH | ||
| // Возвращает True при успешном добавлении | ||
| function AddToPath(Path: string): Boolean; | ||
| var | ||
| EnvPath: string; | ||
| NewPath: string; | ||
| NormalizedPath: string; | ||
| begin | ||
| Result := False; | ||
| NormalizedPath := NormalizePath(Path); | ||
| // Проверяем, есть ли путь уже в PATH | ||
| if PathExistsInEnv(NormalizedPath) then | ||
| begin | ||
| Log('Путь уже существует в PATH: ' + NormalizedPath); | ||
| Result := True; | ||
| exit; | ||
| end; | ||
| // Получаем текущее значение PATH | ||
| if RegQueryStringValue(HKEY_CURRENT_USER, EnvironmentKey, 'Path', EnvPath) then | ||
| begin | ||
| // Добавляем разделитель, если PATH не заканчивается на него | ||
| if Length(EnvPath) = 0 then | ||
| NewPath := NormalizedPath | ||
| else if EnvPath[Length(EnvPath)] = ';' then | ||
| NewPath := EnvPath + NormalizedPath | ||
| else | ||
| NewPath := EnvPath + ';' + NormalizedPath; | ||
| end | ||
| else | ||
| begin | ||
| // PATH не существует, создаём новый | ||
| NewPath := NormalizedPath; | ||
| end; | ||
| // Записываем обновлённый PATH в реестр | ||
| if RegWriteStringValue(HKEY_CURRENT_USER, EnvironmentKey, 'Path', NewPath) then | ||
| begin | ||
| Log('Добавлено в PATH: ' + NormalizedPath); | ||
| Result := True; | ||
| end | ||
| else | ||
| Log('Ошибка при добавлении в PATH: ' + NormalizedPath); | ||
| end; |
There was a problem hiding this comment.
Добавьте проверку лимита длины PATH и обработку ошибок для пользователя.
В функции AddToPath отсутствуют следующие важные проверки:
-
Лимит длины PATH: Windows ограничивает размер переменных окружения (2047 символов в старых версиях, до 32767 в Windows 10+). Добавление пути без проверки может привести к молчаливому сбою или усечению.
-
Условие гонки (TOCTOU): Между чтением PATH (строка 105) и записью (строка 122) другой процесс может изменить PATH, и эти изменения будут потеряны.
-
Отсутствие обратной связи с пользователем: Если запись в реестр не удалась (строки 127-128), пользователь увидит только сообщение в логе, но не получит видимого предупреждения.
💡 Рекомендации по улучшению
Добавьте проверку длины PATH перед записью:
// В функции AddToPath, перед строкой 122:
const
MAX_PATH_LENGTH = 2047; // Консервативный лимит для совместимости
// После строки 119 (перед записью):
if Length(NewPath) > MAX_PATH_LENGTH then
begin
Log('Ошибка: превышен лимит длины PATH (' + IntToStr(Length(NewPath)) + ' символов)');
MsgBox('Не удалось добавить путь в PATH: превышен лимит длины переменной окружения.' + #13#10 +
'Пожалуйста, добавьте путь вручную: ' + NormalizedPath,
mbError, MB_OK);
exit;
end;Добавьте видимое предупреждение при ошибке записи (замените строки 127-128):
else
begin
Log('Ошибка при добавлении в PATH: ' + NormalizedPath);
MsgBox('Не удалось автоматически добавить путь в PATH.' + #13#10 +
'Пожалуйста, добавьте путь вручную: ' + NormalizedPath,
mbError, MB_OK);
end;🤖 Prompt for AI Agents
In @install/ovm.iss around lines 85 - 129, In AddToPath: enforce a conservative
PATH length limit and surface errors to the user, and mitigate the TOCTOU race
between RegQueryStringValue and RegWriteStringValue by re-reading and merging
PATH just before write; specifically, add a constant (e.g., MAX_PATH_LENGTH =
2047), after computing NewPath re-read the current PATH via RegQueryStringValue
(use EnvironmentKey and 'Path'), merge NormalizedPath into the freshly read
value only if not present, then check Length(NewPath) > MAX_PATH_LENGTH and if
so Log the length and show MsgBox explaining the failure and exit; after
RegWriteStringValue fails, replace the silent Log with a MsgBox informing the
user to add NormalizedPath manually (keep using PathExistsInEnv, NormalizePath,
RegQueryStringValue, and RegWriteStringValue to locate code).
There was a problem hiding this comment.
@EvilBeaver, понял, принял к сведению вашу оценку. Спасибо за ответ!
| // Удаляет путь из переменной окружения PATH | ||
| // Использует TStringList для парсинга и ручную сборку строки для записи, | ||
| // чтобы избежать проблемы с кавычками вокруг путей с пробелами | ||
| function RemoveFromPath(Path: string): Boolean; | ||
| var | ||
| EnvPath: string; | ||
| NewPath: string; | ||
| NormalizedPath: string; | ||
| PathList: TStringList; | ||
| PathItem: string; | ||
| NormalizedItem: string; | ||
| i: Integer; | ||
| Removed: Boolean; | ||
| begin | ||
| Result := False; | ||
| Removed := False; | ||
| NormalizedPath := NormalizePath(Path); | ||
| // Получаем текущее значение PATH | ||
| if not RegQueryStringValue(HKEY_CURRENT_USER, EnvironmentKey, 'Path', EnvPath) then | ||
| begin | ||
| Log('PATH не найден в реестре'); | ||
| exit; | ||
| end; | ||
| PathList := TStringList.Create; | ||
| try | ||
| // Разбираем PATH на элементы | ||
| PathList.Delimiter := ';'; | ||
| PathList.StrictDelimiter := True; | ||
| PathList.DelimitedText := EnvPath; | ||
| // Удаляем наш путь и пустые элементы | ||
| for i := PathList.Count - 1 downto 0 do | ||
| begin | ||
| PathItem := PathList[i]; | ||
| // Удаляем пустые элементы (защита от ";;" в PATH) | ||
| if Trim(PathItem) = '' then | ||
| begin | ||
| PathList.Delete(i); | ||
| Continue; | ||
| end; | ||
| // Нормализуем для сравнения (убираем завершающий слеш) | ||
| NormalizedItem := NormalizePath(PathItem); | ||
| // Сравниваем без учёта регистра | ||
| if Uppercase(NormalizedItem) = Uppercase(NormalizedPath) then | ||
| begin | ||
| Log('Удалено из PATH: ' + PathItem); | ||
| PathList.Delete(i); | ||
| Removed := True; | ||
| end; | ||
| end; | ||
| // Собираем строку вручную, чтобы избежать добавления кавычек | ||
| // (TStringList.DelimitedText добавляет кавычки к путям с пробелами) | ||
| NewPath := ''; | ||
| for i := 0 to PathList.Count - 1 do | ||
| begin | ||
| if i > 0 then | ||
| NewPath := NewPath + ';'; | ||
| NewPath := NewPath + PathList[i]; | ||
| end; | ||
| // Записываем обновлённый PATH в реестр | ||
| if RegWriteStringValue(HKEY_CURRENT_USER, EnvironmentKey, 'Path', NewPath) then | ||
| begin | ||
| if Removed then | ||
| Log('PATH успешно обновлён') | ||
| else | ||
| Log('Путь не найден в PATH: ' + NormalizedPath); | ||
| Result := True; | ||
| end | ||
| else | ||
| Log('Ошибка при обновлении PATH'); | ||
| finally | ||
| PathList.Free; | ||
| end; | ||
| end; |
There was a problem hiding this comment.
Добавьте обработку ошибок с обратной связью для пользователя.
Функция RemoveFromPath имеет те же проблемы, что и AddToPath:
-
Условие гонки (TOCTOU): PATH может быть изменён между чтением (строка 150) и записью (строка 198).
-
Отсутствие видимой обратной связи: При ошибке обновления PATH (строка 207) пользователь не получает предупреждения.
Хорошо реализовано: Ручная сборка строки (строки 187-195) корректно избегает проблемы с добавлением кавычек функцией TStringList.DelimitedText.
💡 Рекомендация по улучшению
Добавьте видимое предупреждение при ошибке (замените строку 207):
else
begin
Log('Ошибка при обновлении PATH');
MsgBox('Не удалось автоматически удалить путь из PATH.' + #13#10 +
'При необходимости удалите путь вручную: ' + NormalizedPath,
mbError, MB_OK);
end;
Джентльмены, я мало что понимаю в GA, но нам точно не помешает инсталлятор для ovm в винде.
Написано роботами, скрипт Inno Setup выглядит валидным, actions - не знаю.
Summary by CodeRabbit
Новые функции
Chores
✏️ Tip: You can customize this high-level summary in your review settings.